raise on invalid top-level keys in hub.config

[minrk]

Traitlets requires that valid sub-config keys must be class names starting with uppercase. If they are lowercase, something is wrong and config will be ignored. Probably best not to do so silently, which can lead to long, frustrating debugging before realizing that config is being ignored.

e.g. if you put something like hub.loadRoles under hub.config.loadRoles, it will silently have no effect. This change makes it an informative, fatal error.

In the unlikely event that this shouldn't be fatal somewhere, we could reduce this to a warning, but I think it's safe to make it fatal.

We should probably raise (or at least warn) on this in traitlets itself, too. There's a design constraint there that a top-level Config object doesn't distinguish from sub-config (i.e. type(c.JupyterHub) is type(c)), so raising on assignment may be hard, but that's not insurmountable since it is known at load time, but we know more about what hub.config is and this is a more likely mistake in our mixed helm/traitlets config situation than any other traitlets config application.

cc @agoose77

[consideRatio]

I think it would be even better if this can be done using helm chart's JSONSchema, perhaps not easy or possible - do you know? Then the error is raised on template rendering, which means that you will not upgrade into a state that errors at all, you will simply fail to initialize the upgrade.

[manics]

@consideRatio It should be doable, there's some ideas in #2746

[agoose77]

A suggestion:

diff --git a/jupyterhub/values.schema.yaml b/jupyterhub/values.schema.yaml
index a739554f..9bb9ba50 100644
--- a/jupyterhub/values.schema.yaml
+++ b/jupyterhub/values.schema.yaml
@@ -207,7 +207,7 @@ properties:
           for more info.
       config:
         type: object
-        additionalProperties: true
+        additionalProperties: false
         description: |
           JupyterHub and its components (authenticators, spawners, etc), are
           Python classes that expose its configuration through
@@ -271,6 +271,9 @@ properties:
           the `--values` or `-f` flag. During merging, lists are replaced while
           dictionaries are updated.
           ```
+        patternProperties:
+          "^[A-Z].*$": {}
+
         properties:
           JupyterHub:
             type: object

This would only allow leading upper-case entries.

@minrk thank you so much for doing this. I've seen this foot-gun in production, and recently shot myself in the foot, so it's fantastic that we're looking to make it harder

[minrk]

Great idea! I didn't think of that.

[minrk]

This doesn't go nearly as far as #2746, which I think is a cool idea, but it now validates the bare minimum to be valid traitlets pass-through config:

• top-level keys start with a capital
• top-level values are objects

Produces the validation output with this config:

hub:
  config:
    JupyterHub:
      foo: bar
    
    UnrecognizedClass:
      is: okay

    String: "string"
    
    Array:
      - list
      - of
      - items

    loadRoles:
      sec: key

output:

Error: values don't meet the specifications of the schema(s) in the following chart(s):
jupyterhub:
- hub.config.Array: Invalid type. Expected: object, given: array
- hub.config.String: Invalid type. Expected: object, given: string
- hub.config: Additional property loadRoles is not allowed

which I think is about right, catching the three definitely-invalid cases, and leaving the maybe-a-typo case alone, since it can't be guaranteed to be wrong at validation time.

[manics]

One broken linkcheck is due to jupyterhub/repo2docker#1435 , the other is probably temporary