Arvados

Tom Clegg wrote:

17803-config-key-case-warning @ 4e65a41bea4b892ef7232bfe6b9b20ca35380368 -- developer-run-tests: #2520

Warning with this fix:

[...]

The patch works, thanks. One question, consider this config file:

Clusters:
  xxxxx:
    # Token used internally by Arvados components to authenticate to
    # one another. Use a string of at least 50 random alphanumerics.
    SystemrootToken: "" 

    ServiceS:
      # In each of the service sections below, the keys under
      # InternalURLs are the endpoints where the service should be
      # listening, and reachable from other hosts in the cluster.
      KeepStore:
        Internalurls:
          "http://host1.example:12345": {}
          "http://host2.example:12345":
            # Rendezvous is normally empty/omitted. When changing the
            # URL of a Keepstore service, Rendezvous should be set to
            # the old URL (with trailing slash omitted) to preserve
            # rendezvous ordering.
            RendezVous: "xxxx" 
        ExternalURL: "-" 

    Containers:
      RunTimeEngine: "xxxx" 

I've deliberately screwed up the case of `SystemrootToken`, `ServiceS`, `KeepStore`, `Internalurls`, `RendezVous`, and `RunTimeEngine`.

This the output from `config-check`:

time="2021-06-14T13:01:30-04:00" level=warning msg="deprecated or unknown config entry: Clusters.xxxxx.Containers.RunTimeEngine (perhaps you meant RuntimeEngine?)" 
time="2021-06-14T13:01:30-04:00" level=warning msg="deprecated or unknown config entry: Clusters.xxxxx.ServiceS (perhaps you meant Services?)" 
time="2021-06-14T13:01:30-04:00" level=warning msg="deprecated or unknown config entry: Clusters.xxxxx.SystemrootToken (perhaps you meant SystemRootToken?)" 
time="2021-06-14T13:01:30-04:00" level=warning msg="Clusters.xxxxx.ManagementToken: secret token is not set (use 32+ random characters from a-z, A-Z, 0-9)" 
time="2021-06-14T13:01:30-04:00" level=warning msg="Clusters.xxxxx.SystemRootToken: secret token is not set (use 32+ random characters from a-z, A-Z, 0-9)" 
time="2021-06-14T13:01:30-04:00" level=warning msg="Clusters.xxxxx.Collections.BlobSigningKey: secret token is not set (use 32+ random characters from a-z, A-Z, 0-9)" 
time="2021-06-14T13:01:30-04:00" level=warning msg="keepstore configured at http://host1.example:12345/ does not have access to any volumes" 
time="2021-06-14T13:01:30-04:00" level=warning msg="keepstore configured at http://host2.example:12345/ does not have access to any volumes" 

It complains about `RunTimeEngine`, `ServiceS`, `SystemrootToken`. It does not see the mistake in `KeepStore`, `Internalurls` and `RendezVous`, because it ignored the `ServiceS` section.

But... it is clearly still parsing the `ServiceS` block, as it complains about the defined keepstores not having access to any volumes. What's up with that?

Another example, this one has the spelling of `Services` fixed:

Clusters:
  xxxxx:
    # Token used internally by Arvados components to authenticate to
    # one another. Use a string of at least 50 random alphanumerics.
    SystemrootToken: "" 

    Services:
      # In each of the service sections below, the keys under
      # InternalURLs are the endpoints where the service should be
      # listening, and reachable from other hosts in the cluster.
      KeepStore:
        Internalurls:
          "http://host1.example:12345": {}
          "http://host2.example:12345":
            # Rendezvous is normally empty/omitted. When changing the
            # URL of a Keepstore service, Rendezvous should be set to
            # the old URL (with trailing slash omitted) to preserve
            # rendezvous ordering.
            RendezVous: "xxxx" 
        ExternalURL: "-" 

    Containers:
      RunTimeEngine: "xxxx" 

That leads to this output:

time="2021-06-14T13:09:59-04:00" level=warning msg="deprecated or unknown config entry: Clusters.xxxxx.Containers.RunTimeEngine (perhaps you meant RuntimeEngine?)" 
time="2021-06-14T13:09:59-04:00" level=warning msg="deprecated or unknown config entry: Clusters.xxxxx.Services.KeepStore.Internalurls (perhaps you meant InternalURLs?)" 
time="2021-06-14T13:09:59-04:00" level=warning msg="deprecated or unknown config entry: Clusters.xxxxx.SystemrootToken (perhaps you meant SystemRootToken?)" 
time="2021-06-14T13:09:59-04:00" level=warning msg="Clusters.xxxxx.ManagementToken: secret token is not set (use 32+ random characters from a-z, A-Z, 0-9)" 
time="2021-06-14T13:09:59-04:00" level=warning msg="Clusters.xxxxx.SystemRootToken: secret token is not set (use 32+ random characters from a-z, A-Z, 0-9)" 
time="2021-06-14T13:09:59-04:00" level=warning msg="Clusters.xxxxx.Collections.BlobSigningKey: secret token is not set (use 32+ random characters from a-z, A-Z, 0-9)" 
time="2021-06-14T13:09:59-04:00" level=warning msg="keepstore configured at http://host1.example:12345/ does not have access to any volumes" 
time="2021-06-14T13:09:59-04:00" level=warning msg="keepstore configured at http://host2.example:12345/ does not have access to any volumes" 

In this case, it ignores the case difference in `KeepStore`, but it complains about `Internalurls`. And it still complains about the access to volumes.

Ah. The "merge actual into defaults" operation, given differing-case keys with different map values, merges the two maps (because unmarshal into a non-empty map adds/updates but doesn't clear first).

https://play.golang.org/p/0EkcL5Ii5Wl

I think we can fix the strangeness by deleting the unrecognized keys as well as reporting them.

Added ServiceS, KeepStore, and RendezVous to the test case and made the necessary adjustments so they all get reported.

• We could warn about using Rendezvous in a Services entry other than Keepstore (but we don't).
• We should reject inputs with duplicate keys (but we don't). It looks like upgrading to gopkg.in/yaml.v3 would help, but that means upgrading ghodss/yaml to use it, and... at this point it starts to feel out-of-scope.

17803-config-key-case-warning @ 4b5ec8651ba83f2a79fe40708021e03c86275093 -- developer-run-tests: #2524

Tom Clegg wrote:

Added ServiceS, KeepStore, and RendezVous to the test case and made the necessary adjustments so they all get reported.

I noticed a couple of remaining oddities:

• We could warn about using Rendezvous in a Services entry other than Keepstore (but we don't).
• We should reject inputs with duplicate keys (but we don't). It looks like upgrading to gopkg.in/yaml.v3 would help, but that means upgrading ghodss/yaml to use it, and... at this point it starts to feel out-of-scope.

17803-config-key-case-warning @ 4b5ec8651ba83f2a79fe40708021e03c86275093 -- developer-run-tests: #2524

This works; the only thing I don't like is the use of an invalid config (rendezvous on Keepproxy record) in the tests. Can we not define a volume in the mock to make the "keepstore has no volumes" warning go away instead? It seems awkward to do it this way, particularly because there's a comment in the mock that Rendezvous on keepproxy should be another warning.

Otherwise, LGTM. Should we list the known limitations you mentioned above as FIXME's in `lib/config/load.go` ? I like how e.g. the tailscale codebase is full of those kinds of comments. It seems more easily actionable to record them that way (as opposed to creating tickets we may not look at for years).

define a volume in the mock to make the "keepstore has no volumes" warning go away

Oh, yeah, that's a better solution! Done.

FIXME's

Added.

17803-config-key-case-warning @ 2d3723c301085ebfcb1f3a8940451edc15f10f93 -- developer-run-tests: #2529

Tom Clegg wrote:

define a volume in the mock to make the "keepstore has no volumes" warning go away

Oh, yeah, that's a better solution! Done.

FIXME's

Added.

17803-config-key-case-warning @ 2d3723c301085ebfcb1f3a8940451edc15f10f93 -- developer-run-tests: #2529

LGTM, thanks!