15003: Warn about unknown keys and bad value types.

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tclegg@veritasgenetics.com>

diff --git a/lib/config/cmd.go b/lib/config/cmd.go
index e3419cee54e1d64689ddab47b2495203b8c6763c..41a1d7d2143483b2fc67f647ab8e24492e037420 100644 (file)
--- a/lib/config/cmd.go
+++ b/lib/config/cmd.go
@@ -63,7 +63,7 @@ func (checkCommand) RunCommand(prog string, args []string, stdin io.Reader, stdo
                err = fmt.Errorf("usage: %s <config-src.yaml && echo 'no changes needed'", prog)
                return 2
        }
-       log := ctxlog.New(stderr, "text", "info")
+       log := &plainLogger{w: stderr}
        buf, err := ioutil.ReadAll(stdin)
        if err != nil {
                return 1
@@ -72,7 +72,7 @@ func (checkCommand) RunCommand(prog string, args []string, stdin io.Reader, stdo
        if err != nil {
                return 1
        }
-       withDepr, err := load(bytes.NewBuffer(buf), log, true)
+       withDepr, err := load(bytes.NewBuffer(buf), nil, true)
        if err != nil {
                return 1
        }
@@ -91,9 +91,28 @@ func (checkCommand) RunCommand(prog string, args []string, stdin io.Reader, stdo
                cmd.ExtraFiles = append(cmd.ExtraFiles, pr)
        }
        diff, err := cmd.CombinedOutput()
-       if err == nil {
-               return 0
+       if bytes.HasPrefix(diff, []byte("--- ")) {
+               fmt.Fprintln(stdout, "Your configuration is relying on deprecated entries. Suggest making the following changes.")
+               stdout.Write(diff)
+               return 1
+       } else if len(diff) > 0 {
+               fmt.Fprintf(stderr, "Unexpected diff output:\n%s", diff)
+               return 1
+       } else if err != nil {
+               return 1
+       }
+       if log.used {
+               return 1
        }
-       _, err = stdout.Write(diff)
-       return 1
+       return 0
+}
+
+type plainLogger struct {
+       w    io.Writer
+       used bool
+}
+
+func (pl *plainLogger) Warnf(format string, args ...interface{}) {
+       pl.used = true
+       fmt.Fprintf(pl.w, format+"\n", args...)
 }

diff --git a/lib/config/cmd_test.go b/lib/config/cmd_test.go
index 925bbe6483facc9c7791c598bdf4fbe5983eb12c..bedcc0dd8c6c586f4f75fb8f9686ce9a2c4ffef3 100644 (file)
--- a/lib/config/cmd_test.go
+++ b/lib/config/cmd_test.go
@@ -55,7 +55,31 @@ Clusters:
        c.Check(stdout.String(), check.Matches, `(?ms).*API:\n\- +.*MaxItemsPerResponse: 1000\n\+ +MaxItemsPerResponse: 1234\n.*`)
 }
 
-func (s *CommandSuite) TestUnknownKey(c *check.C) {
+func (s *CommandSuite) TestCheckUnknownKey(c *check.C) {
+       var stdout, stderr bytes.Buffer
+       in := `
+Clusters:
+ z1234:
+  Bogus1: foo
+  BogusSection:
+    Bogus2: foo
+  API:
+    Bogus3:
+     Bogus4: true
+  PostgreSQL:
+    ConnectionPool:
+      {Bogus5: true}
+`
+       code := CheckCommand.RunCommand("arvados config-check", nil, bytes.NewBufferString(in), &stdout, &stderr)
+       c.Log(stderr.String())
+       c.Check(code, check.Equals, 1)
+       c.Check(stderr.String(), check.Matches, `(?ms).*deprecated or unknown config entry: Clusters.z1234.Bogus1\n.*`)
+       c.Check(stderr.String(), check.Matches, `(?ms).*deprecated or unknown config entry: Clusters.z1234.BogusSection\n.*`)
+       c.Check(stderr.String(), check.Matches, `(?ms).*deprecated or unknown config entry: Clusters.z1234.API.Bogus3\n.*`)
+       c.Check(stderr.String(), check.Matches, `(?ms).*unexpected object in config entry: Clusters.z1234.PostgreSQL.ConnectionPool\n.*`)
+}
+
+func (s *CommandSuite) TestDumpUnknownKey(c *check.C) {
        var stdout, stderr bytes.Buffer
        in := `
 Clusters:
@@ -65,7 +89,7 @@ Clusters:
 `
        code := DumpCommand.RunCommand("arvados config-dump", nil, bytes.NewBufferString(in), &stdout, &stderr)
        c.Check(code, check.Equals, 0)
-       c.Check(stderr.String(), check.Equals, "")
+       c.Check(stderr.String(), check.Matches, `(?ms).*deprecated or unknown config entry: Clusters.z1234.UnknownKey.*`)
        c.Check(stdout.String(), check.Matches, `(?ms)Clusters:\n  z1234:\n.*`)
        c.Check(stdout.String(), check.Matches, `(?ms).*\n *ManagementToken: secret\n.*`)
        c.Check(stdout.String(), check.Not(check.Matches), `(?ms).*UnknownKey.*`)

diff --git a/lib/config/deprecated.go b/lib/config/deprecated.go
index 2fc839cf281650bada1b16907229584309119d0f..c8f943f3ccafe2b97a54a6edcacdff04d71d60fa 100644 (file)
--- a/lib/config/deprecated.go
+++ b/lib/config/deprecated.go
@@ -45,7 +45,7 @@ func applyDeprecatedConfig(cfg *arvados.Config, configdata []byte, log logger) e
                for name, np := range dcluster.NodeProfiles {
                        if name == "*" || name == os.Getenv("ARVADOS_NODE_PROFILE") || name == hostname {
                                name = "localhost"
-                       } else {
+                       } else if log != nil {
                                log.Warnf("overriding Clusters.%s.Services using Clusters.%s.NodeProfiles.%s (guessing %q is a hostname)", id, id, name, name)
                        }
                        applyDeprecatedNodeProfile(name, np.RailsAPI, &cluster.Services.RailsAPI)

diff --git a/lib/config/load.go b/lib/config/load.go
index 644dcfdb8c70d8138876e0adc79ba4b162cb8ae9..526a050fbbdf923a076faab9ad4ce9b7954a1487 100644 (file)
--- a/lib/config/load.go
+++ b/lib/config/load.go
@@ -78,6 +78,7 @@ func load(rdr io.Reader, log logger, useDeprecated bool) (*arvados.Config, error
        if err != nil {
                return nil, fmt.Errorf("loading config data: %s", err)
        }
+       logExtraKeys(log, merged, src, "")
        err = mergo.Merge(&merged, src, mergo.WithOverride)
        if err != nil {
                return nil, fmt.Errorf("merging config data: %s", err)
@@ -121,9 +122,28 @@ func checkKeyConflict(label string, m map[string]string) error {
        for k := range m {
                k = strings.ToLower(k)
                if saw[k] {
-                       return fmt.Errorf("%s: multiple keys with tolower(key)==%q (use same case as defaults)", label, k)
+                       return fmt.Errorf("%s: multiple entries for %q (fix by using same capitalization as default/example file)", label, k)
                }
                saw[k] = true
        }
        return nil
 }
+
+func logExtraKeys(log logger, expected, supplied map[string]interface{}, prefix string) {
+       if log == nil {
+               return
+       }
+       for k, vsupp := range supplied {
+               if vexp, ok := expected[k]; !ok {
+                       log.Warnf("deprecated or unknown config entry: %s%s", prefix, k)
+               } else if vsupp, ok := vsupp.(map[string]interface{}); !ok {
+                       // if vsupp is a map but vexp isn't map, this
+                       // will be caught elsewhere; see TestBadType.
+                       continue
+               } else if vexp, ok := vexp.(map[string]interface{}); !ok {
+                       log.Warnf("unexpected object in config entry: %s%s", prefix, k)
+               } else {
+                       logExtraKeys(log, vexp, vsupp, prefix+k+".")
+               }
+       }
+}

diff --git a/lib/config/load_test.go b/lib/config/load_test.go
index fddf3660d270fe7eff18d6ea503ce4681923f8cd..2bf341fe1deda02369930e52427463ec160e70cc 100644 (file)
--- a/lib/config/load_test.go
+++ b/lib/config/load_test.go
@@ -62,7 +62,43 @@ Clusters:
      dbname: dbname
      host: host
 `), ctxlog.TestLogger(c))
-       c.Assert(err, check.ErrorMatches, `Clusters.zzzzz.PostgreSQL.Connection: multiple keys with .*"(dbname|host)".*`)
+       c.Check(err, check.ErrorMatches, `Clusters.zzzzz.PostgreSQL.Connection: multiple entries for "(dbname|host)".*`)
+}
+
+func (s *LoadSuite) TestBadType(c *check.C) {
+       for _, data := range []string{`
+Clusters:
+ zzzzz:
+  PostgreSQL: true
+`, `
+Clusters:
+ zzzzz:
+  PostgreSQL:
+   ConnectionPool: true
+`, `
+Clusters:
+ zzzzz:
+  PostgreSQL:
+   ConnectionPool: "foo"
+`, `
+Clusters:
+ zzzzz:
+  PostgreSQL:
+   ConnectionPool: []
+`, `
+Clusters:
+ zzzzz:
+  PostgreSQL:
+   ConnectionPool: [] # {foo: bar} isn't caught here; we rely on config-check
+`,
+       } {
+               c.Log(data)
+               v, err := Load(bytes.NewBufferString(data), ctxlog.TestLogger(c))
+               if v != nil {
+                       c.Logf("%#v", v.Clusters["zzzzz"].PostgreSQL.ConnectionPool)
+               }
+               c.Check(err, check.ErrorMatches, `.*cannot unmarshal .*PostgreSQL.*`)
+       }
 }
 
 func (s *LoadSuite) TestMovedKeys(c *check.C) {