schema: catch typos in template values using JSONSchema's additionalProperties

[consideRatio]

We have a schema.yaml file that we render into values.schema.json and ship with our Helm chart as supported by Helm 3. This file is a JSONSchema that covers all our charts configuration options since #2033. helm will use it during helm template and helm install|upgrade to catch values that the schema considers invalid.

What this PR does is to start make use of a JSONSchema feature called additionalProperties. With additionalProperties: false we can make the schema invalidate any object type field that declare a property that isn't declared in the schema.

Since we have declared almost all properties, we can set additionalProperties: false on all those object type fields and catch all kinds of typos before helm install, helm upgrade, or helm template would start requesting changes to the k8s api-server.

Closes #2199

Suggested action points

• We test it against deployments we control - any failures will be caught before k8s api-server is contacted!
• We merge this without much delay
• We release 1.0.0-beta.1
• We release 1.0.0

Example UX

$ helm template jh jupyterhub --set singleuser.image.tagg=hello --set proxxy.secretToken=hello 

Error: values don't meet the specifications of the schema(s) in the following chart(s):
jupyterhub:
- (root): Additional property proxxy is not allowed
- singleuser.image: Additional property tagg is not allowed

Local testing

# By generating the values.schema.json file and
# testing against charts default values
tools/generate-json-schema.py
helm template jh jupyterhub --set proxxy.secretToken=hello

# By using the charts default values but combined with the
# tools/templates/lint-and-validate-values.yaml file
tools/validate-against-schema.py

[yuvipanda]

Awesome :)

How do we test this against github.com/2i2c-org/pilot-hubs/ and github.com/berkeley-dsep-infra/datahub/? Their config is already public...

[consideRatio]

@yuvipanda the easiest that I suggest is that we merge this and iterate after merge.

The easiest form of test is to helm template referencing all your values and the helm chart with a bundled values.schema.json.

How to reference a non-merged chart?

• The chart is packaged and made available here: https://github.com/jupyterhub/zero-to-jupyterhub-k8s/suites/2750369571/artifacts/60820128. This is an artifact for the publish job when triggered as part of a pull request. I think one can point to a packaged file like this.
• The values.schema.json file needs to be generated if you clone the repo and reference a local version.

How to test specifically against github.com/berkeley-dsep-infra/datahub/ for example?

• I suggest you create a workflow that has a job running helm template once for each deployment. Then you will get feedback for every deployment without aborting a job midway even during a PR referencing a local chart or a published chart version.
• It would be nice to have a way to just enter a github artifact URL like https://github.com/jupyterhub/zero-to-jupyterhub-k8s/suites/2750369571/artifacts/60820128 to automatically run tests against it. Hmm... That is probably doable with workflow_dispatch btw where we could enter a url and design a workflow for this purpose...

[consideRatio]

@yuvipanda I tried this locally and the template validation worked fine, but when actually doing an upgrade I ran into a breaking change form a PR that I didn't think would be breaking.

This is being addressed in #2201.

[consideRatio]

@yuvipanda I brainfarted and didn't realize I could trial this myself. I tried the basehub Helm chart in pilot-hubs using the default values.

Error: values don't meet the specifications of the schema(s) in the following chart(s):
jupyterhub:
- (root): Additional property cloudResources is not allowed
- singleuser: Additional property admin is not allowed

You can keep passing information to the hub pod through custom.anything-you-choose-here, but to inject variables in the root context or singleuser isn't allowed. The added options could in practice be typo's so I think it is correct for the schema to complain here.

---

I also tried the hub Helm chart in berkeley-dsep-infra/datahub using its default values. No validation errors at all there!

---

How i tried it

• I had this PR checked out in a local clone
• I ran tools/generate-json-schema.py (to emit values.schema.json)
• I cloned the repos with deployments to test.
• I Updated Chart.yaml for the locally cloned basehub / hub charts to depend on my local z2jh chart.
  • repository: file:///home/erik/dev/contrib/jupyterhub/zero-to-jupyterhub-k8s/jupyterhub
• I ran helm template . from the basehub / hub folders.

[consideRatio]

@yuvipanda this is now tested successfully against:

• 2i2c/pilot-hubs: basehub, daskhub (three custom values need to relocate: Make hubs compatible with Z2JH 1.0.0 2i2c-org/infrastructure#414)
• berkeley-dsep-infra/datahub (all ok using default values)
• jupyterhub/binderhub (all ok)
• jupyterhub/mybinder.org-deploy (actual bug caught: Fix outdated configuration reference (nginx -> traefik -> not relevant and removed) mybinder.org-deploy#1909)
• Sandvik's deployment that I manage (all ok)

I'll do some work today that remains before we can cut 1.0.0-beta.1 but besides that work this is the remaining piece that I'd like to have merged for such beta release.

[manics]

Sounds good to me! I don't have anywhere suitable to test this, but I trust @yuvipanda's review 😄

[yuvipanda]

I'm very happy to have this!

This is a breaking change though, since config that deployed earlier will no longer deploy. Can we document this as a breaking change? Happy to merge after that

[consideRatio]

@yuvipanda I plan to put this in the CHANGELOG.md which is drafted in another PR currently. Would you like me to make a stub in the changelog from here and then merge them?

PS: Test error in publish seems unrelated to docker importing six that isn't available...

[consideRatio]

Preview of WIP of the changelog regarding breakign changes for this PR: https://github.com/consideRatio/zero-to-jupyterhub-k8s/blob/pr/12.0.0-release/CHANGELOG.md#breaking-changes

---

• Schema validation of chart config (#2033, #2200)

  The Helm chart now bundles with a values.schema.json file that will validate
  all use of the Helm chart during template rendering. If the Helm chart's
  passed values doesn't comply with the schema, then helm will error before
  the k8s api-server has become involved and anything has changed in the k8s
  cluster.

  The most common validation errors are:

  • Unrecognized config values

    For example if you have misspelled something.

    Note that if you want to pass your custom values for inspection by custom
    logic in the hub pod, then you should pass these values via the custom
    config section where anything will be accepted.

  • Recognized config values with the wrong type

    For example if you have passed a numerical value to a configuration that
    expected a string.

[yuvipanda]

The test failure looks unrelated. THIS IS AWESOME, THANKS @consideRatio!

[consideRatio]

Thank you @yuvipanda and @manics! ❤️

[mriedem]

I'm working on upgrading from z2jh 0.11.1 to 1.2.0 and hit this error:

Error: UPGRADE FAILED: values don't meet the specifications of the schema(s) in the following chart(s):

jupyterhub:

- (root): Additional property profiles is not allowed

- (root): Additional property secrets_checksum is not allowed

- (root): Additional property values_checksum is not allowed

I think I can get around the profiles one which is just a JSON string we pass through hub.extraEnv and is read by our custom authenticator, it eventually sets c.KubeSpawner.profile_list. Why we don't just do this using hub.config.KubeSpawner.profile_list I'm not sure, except our custom authenticator does some logic based on the profile list. 🤷

The bigger issue I have is our deployment process runs helm upgrade with calculated secrets_checksum and values_checksum values which are hashes of the (encrypted) secrets and (unencrypted) values files which we use. The idea being we trigger an upgrade if the checksum changes because something in one of the values files changed. Specifying the extra value on the command line is triggering the jsonschema validation error.

Is the solution to just specify those in custom, e.g.:

custom:
  secrets_checksum: ''
  values_checksum: ''

It seems like this might be a decent advanced sub-topic for https://zero-to-jupyterhub.readthedocs.io/en/latest/jupyterhub/customizing/extending-jupyterhub.html#applying-configuration-changes.

[consideRatio]

Is the solution to just specify those in custom, e.g.:

custom:
  secrets_checksum: ''
  values_checksum: ''

Correct! All things not part of the chart, but you want to pass to as values so they can be accessed by the hub pod, should go there.

---

Please extract anything else to a dedicated issue or discourse thread @mriedem, I didn't instantly understand your situation because it was a mix of things in it and I'm low on time atm.