From 727c8c37baa64fe63bec04aacea870ea47a7daf0 Mon Sep 17 00:00:00 2001 From: Tom Clegg Date: Mon, 26 Apr 2021 15:11:58 -0400 Subject: [PATCH] 16997: Test map key order in check/dump. Fix duplicate warnings. Arvados-DCO-1.1-Signed-off-by: Tom Clegg --- lib/config/cmd.go | 27 ++---------------- lib/config/cmd_test.go | 65 ++++++++++++++++++++++++++++++++++++++++++ lib/config/load.go | 8 ++++-- 3 files changed, 74 insertions(+), 26 deletions(-) diff --git a/lib/config/cmd.go b/lib/config/cmd.go index 347e8519a9..8e638e6ecb 100644 --- a/lib/config/cmd.go +++ b/lib/config/cmd.go @@ -12,7 +12,6 @@ import ( "os" "os/exec" - "git.arvados.org/arvados.git/sdk/go/arvados" "git.arvados.org/arvados.git/sdk/go/ctxlog" "github.com/ghodss/yaml" "github.com/sirupsen/logrus" @@ -120,16 +119,15 @@ func (checkCommand) RunCommand(prog string, args []string, stdin io.Reader, stdo if err != nil { return 1 } + // Reset() to avoid printing the same warnings twice when they + // are logged by both without-legacy and with-legacy loads. + logbuf.Reset() loader.SkipDeprecated = false loader.SkipLegacy = false withDepr, err := loader.Load() if err != nil { return 1 } - problems := false - if warnAboutProblems(logger, withDepr) { - problems = true - } cmd := exec.Command("diff", "-u", "--label", "without-deprecated-configs", "--label", "relying-on-deprecated-configs", "/dev/fd/3", "/dev/fd/4") for _, obj := range []interface{}{withoutDepr, withDepr} { y, _ := yaml.Marshal(obj) @@ -165,28 +163,9 @@ func (checkCommand) RunCommand(prog string, args []string, stdin io.Reader, stdo return 1 } } - - if problems { - return 1 - } return 0 } -func warnAboutProblems(logger logrus.FieldLogger, cfg *arvados.Config) bool { - warned := false - for id, cc := range cfg.Clusters { - if cc.SystemRootToken == "" { - logger.Warnf("Clusters.%s.SystemRootToken is empty; see https://doc.arvados.org/master/install/install-keepstore.html", id) - warned = true - } - if cc.ManagementToken == "" { - logger.Warnf("Clusters.%s.ManagementToken is empty; see https://doc.arvados.org/admin/management-token.html", id) - warned = true - } - } - return warned -} - var DumpDefaultsCommand defaultsCommand type defaultsCommand struct{} diff --git a/lib/config/cmd_test.go b/lib/config/cmd_test.go index bb8d7dca10..d4d2f3d631 100644 --- a/lib/config/cmd_test.go +++ b/lib/config/cmd_test.go @@ -134,6 +134,18 @@ Clusters: c.Check(stderr.String(), check.Matches, `(?ms).*unexpected object in config entry: Clusters.z1234.PostgreSQL.ConnectionPool"\n.*`) } +func (s *CommandSuite) TestCheck_DuplicateWarnings(c *check.C) { + var stdout, stderr bytes.Buffer + in := ` +Clusters: + z1234: {} +` + code := CheckCommand.RunCommand("arvados config-check", []string{"-config", "-"}, bytes.NewBufferString(in), &stdout, &stderr) + c.Check(code, check.Equals, 1) + c.Check(stderr.String(), check.Matches, `(?ms).*SystemRootToken.*`) + c.Check(stderr.String(), check.Not(check.Matches), `(?ms).*SystemRootToken.*SystemRootToken.*`) +} + func (s *CommandSuite) TestDump_Formatting(c *check.C) { var stdout, stderr bytes.Buffer in := ` @@ -168,3 +180,56 @@ Clusters: c.Check(stdout.String(), check.Matches, `(?ms).*\n *ManagementToken: secret\n.*`) c.Check(stdout.String(), check.Not(check.Matches), `(?ms).*UnknownKey.*`) } + +func (s *CommandSuite) TestDump_KeyOrder(c *check.C) { + in := ` +Clusters: + z1234: + Login: + Test: + Users: + a: {} + d: {} + c: {} + b: {} + e: {} +` + for trial := 0; trial < 20; trial++ { + var stdout, stderr bytes.Buffer + code := DumpCommand.RunCommand("arvados config-dump", []string{"-config", "-"}, bytes.NewBufferString(in), &stdout, &stderr) + c.Assert(code, check.Equals, 0) + if !c.Check(stdout.String(), check.Matches, `(?ms).*a:.*b:.*c:.*d:.*e:.*`) { + c.Logf("config-dump did not use lexical key order on trial %d", trial) + c.Log("stdout:\n", stdout.String()) + c.Log("stderr:\n", stderr.String()) + c.FailNow() + } + } +} + +func (s *CommandSuite) TestCheck_KeyOrder(c *check.C) { + in := ` +Clusters: + z1234: + ManagementToken: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + SystemRootToken: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + Collections: + BlobSigningKey: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + InstanceTypes: + a: {} + d: {} + c: {} + b: {} + e: {} +` + for trial := 0; trial < 20; trial++ { + var stdout, stderr bytes.Buffer + code := CheckCommand.RunCommand("arvados config-check", []string{"-config=-", "-strict=true"}, bytes.NewBufferString(in), &stdout, &stderr) + if !c.Check(code, check.Equals, 0) || stdout.String() != "" || stderr.String() != "" { + c.Logf("config-check returned error or non-empty output on trial %d", trial) + c.Log("stdout:\n", stdout.String()) + c.Log("stderr:\n", stderr.String()) + c.FailNow() + } + } +} diff --git a/lib/config/load.go b/lib/config/load.go index b42e1a3c1e..292e3f6d6f 100644 --- a/lib/config/load.go +++ b/lib/config/load.go @@ -313,11 +313,15 @@ var acceptableTokenLength = 32 func (ldr *Loader) checkToken(label, token string) error { if token == "" { - ldr.Logger.Warnf("%s: secret token is not set (use %d+ random characters from a-z, A-Z, 0-9)", label, acceptableTokenLength) + if ldr.Logger != nil { + ldr.Logger.Warnf("%s: secret token is not set (use %d+ random characters from a-z, A-Z, 0-9)", label, acceptableTokenLength) + } } else if !acceptableTokenRe.MatchString(token) { return fmt.Errorf("%s: unacceptable characters in token (only a-z, A-Z, 0-9 are acceptable)", label) } else if len(token) < acceptableTokenLength { - ldr.Logger.Warnf("%s: token is too short (should be at least %d characters)", label, acceptableTokenLength) + if ldr.Logger != nil { + ldr.Logger.Warnf("%s: token is too short (should be at least %d characters)", label, acceptableTokenLength) + } } return nil } -- 2.30.2