values.schema.json ships with chart and configuration reference now covers all options #2033

consideRatio · 2021-02-13T05:21:06Z

Summary

schema.yaml and values.yaml are now in parity making the configuration reference complete, preview here - closes schema.yaml: not all configuration options are documented #1829
helm validation logic is packaged with the chart in values.schema.json, generated from schema.yaml - closes Helm 3: Values.yaml schema validations #1316

Breaking changes

If someone has a configuration that overrides a default value to null or similarly, then they can get validation errors before the k8s api-server has been contacted by the helm CLI.

Implementation notes

Helper scripts

tools/compare-values-schema-content.py
This new script isn't run as part of the CI system, but was helping me to spot if there were values in values.yaml that weren't in
schema.yaml and vice versa.
tools/validate-against-schema.py
This script was previously located in the chart folder and named validate.py, I've just moved it and made it validate with the lint-
and-validate-values.yaml file as well. This script now runs in a dedicated job part of the test-chart workflow, before it was undocumented and not part of the CI system.
tools/generate-json-schema.py
This new script reads schema.yaml, cleans it from descriptions that isn't relevant for the helm CLI's logic as far as I know, and emits jupyterhub/values.schema.json. This script runs in all chart tests and before we package and publish the Helm chart.

Accept null or not?

I have considered the type enforcement of all values that require a string or integer and considered if they should allow null or not. The principle I concluded was that if the field is a Helm chart native configuration is should typically not allow null. But, if it is a passthrough configuration that isn't required by the Helm chart templates, then we should allow it be null in order to be able to use the default values of the actual software such as JupyterHub or KubeSpawner. For such values, null implies the default value used in the software rather than the default value of the Helm chart typically.

If we would support null for all values, we would risk introducing unneeded runtime errors helm template rendering errors that are far harder to interpret. Due to this, I have opted to avoid allowing null.

Sometimes the chart has given a blank string different meaning to null, such as for fullnameOverride. At this times null is also allowed.

Notes on `helm` validation logic

I don't think it is possible to be very strict and warn for passed configuration not part of the schema which would be useful if a user has misspelled an option. What we can do, is to declare required fields (required), and enforce the fields passed have accepted data types (type).

In other words, this is not an end to the mistakes you can make, but it is a good improvement. While there may be some fields that are required, I have only used that keyword to require the root objects such as hub and proxy to be set.

yuvipanda

I AM ABSOLUTELY AMAZED BY THE AMOUNT OF WORK AND CARE YOU PUT INTO THIS. WOW

yuvipanda · 2021-02-15T05:17:56Z

tools/compare-values-schema-content.py

+import yaml
+
+# Change current directory to this directory
+os.chdir(os.path.dirname(sys.argv[0]))


Suggested change

os.chdir(os.path.dirname(sys.argv[0]))

os.chdir(os.path.abspath(os.path.dirname(__file__)))

Also see comment below in validate-against-schema - I don't think os.chdir in scripts is usually something we want to do.

I'd like the script to function the same way independently of where they are executed from.

I guess the crux of using os.chdir is that it can influence the environment calling the script? Running it from my terminal didn't influence the terminal, but it could perhaps?

I'm not sure on the path forward here, suggestions?

UPDATE: Will resolve this using https://github.com/jupyterhub/zero-to-jupyterhub-k8s/pull/2033/files#r576103689 logic

Resolved by cb0ae7d

yuvipanda · 2021-02-15T10:24:53Z

jupyterhub/values.yaml

-    url:
-    password:
+      subPath: ""
+      storageClassName: ""


If it's set to empty rather than unset, will the resulting field be present? I think many fields in Kubernetes have adifferent effect when it's set to empty string - particularly, the defaults might not apply.

For example, an empty storageClassName has different behavior from having it be not present (see doc).

So my instinct is to allow anything that can be a non-container type (so strings, numbers, bools) be nullable.

I agree, I think null for unset is often the right thing to do.

Ah yes storageClassName was one of those fields where we differentiate between unset and empty string!

{{- if typeIs "string" .Values.hub.db.pvc.storageClassName }} storageClassName: {{ .Values.hub.db.pvc.storageClassName | quote }} {{- end }}

I agree. Resolved by 1153aa3.

tools/compare-values-schema-content.py

minrk · 2021-02-15T10:45:50Z

This is awesome! ❤️

Accept null or not?

I think we need to be careful with this a bit. Setting to null and leaving unset are not always equivalent, but often are, and that seems to be the norm for helm/kubernetes, but not for traitlets. If they are interpreted by the helm chart with things like with .Values.something allowing null is usually fine as the result is to leave it unset. With passthrough config to traitlets, though, null and unset are not generally the same, and the schema should generally distinguish. Similarly, with kubernetes, setting many fields to null is the same as leaving it unset (imagePullPolicy is one, I believe, where we should allow null to select the cluster default behavior).

The exception is the subset of config that is not direct passthrough, and instead is specifically transformed by the chart from values.yaml to traitlets config (e.g. singleuser.cmd). For those, I agree that allowing explicit null to mean using the default is the right thing to do. This is what we've implemented with the set_config_if_not_none utility in juptyerhub_config.py. That utility function is required to implement this different interpretation - helm values null means apply no config, while None in traitlets config means explicit override default with None.

jupyterhub/schema.yaml

.github/workflows/test-chart.yaml

tools/validate-against-schema.py

minrk · 2021-02-15T10:52:22Z

tools/compare-values-schema-content.py

+import yaml
+
+# Change current directory to this directory
+os.chdir(os.path.dirname(sys.argv[0]))


Also see comment below in validate-against-schema - I don't think os.chdir in scripts is usually something we want to do.

minrk · 2021-02-15T10:53:42Z

jupyterhub/values.yaml

-    url:
-    password:
+      subPath: ""
+      storageClassName: ""


I agree, I think null for unset is often the right thing to do.

consideRatio · 2021-02-15T11:28:42Z

@minrk regarding comments on Accept null or not? I think we are in agreement and have arrived at the same conclusions, but I find the topic quite complicated in general.

Is it correct that you don't suggest changes other than those you have already explicitly pointed out?

yuvipanda · 2021-02-16T15:20:56Z

jupyterhub/values.yaml

@@ -35,11 +35,11 @@ hub:
    annotations: {}
    ports:
      nodePort:
-    loadBalancerIP:
+    loadBalancerIP: ""


I think this should also be unset, no?

Trying to understand why some vars are empty string and some are unset. My intuition is that default should be either a value we want (like uid: 1000), unset, or empty container ([], {}, etc)

The principle ive had while writing these evolved to be:

1: if the helm chart provide an explicit default value: set it.
2: else if the helm chart config is a passthrough config for
jupyterhub/kubespawner/authenticator etc: we set it to null to represent the intent for the helm chart to not change the default value of the underlying config, and a falsy value to represent a wish for it to be explicitly set to falsy value.
3: else if, null snd a falsy instance type both imply a unset value, set it to the falsy instance type
4: else set it to null

To me, the most controversial point is the third. The question in my mind boils down to: if both null and a falsy type ("", {}, [], 0) by the helm templates imply that a value wont be set, should we specify the values falsy type of null?

I lean towards specifying the falsy instance type rather than null if the config isn't passthrough.

I think we break this logic on point 2 for array/objects passthrough config, which i think is motivated by avoiding warnings and help helm templste logic.

So with loadBalancerIP, I see the following code

{{- with .Values.proxy.service.loadBalancerIP }} loadBalancerIP: {{ . }} {{- end }}

So if I understand correctly, the loadBalancerIP key in the Service object won't be present at all in the following circumstances:

.Values.proxy.service.loadBalancerIP is specified in helm chart values but not set (current master situation)

.Values.proxy.service.loadBalancerIP is set to "" (as in the PR)

Is this understanding correct?

Yepp! The with keyword using a falsy value is like a do nothing statement, it has become a practice to use it throughout the helm ecosystem over if ... Then ... - probably to avoid duplicating the reference needed in both the conditional statement and within

Right. So if I see the values.yaml file, it looks like I'll actually get loadBalancerIP: "" in my Service object, which is confusing! Since k8s differentiates between unset values and "" values, so should we - otherwise it can be pretty confusing. I think the mental model of if a value is unspecified (null), it is not present in the target is better than if a value is falsey (by go template's definition of falsey) it will not be present in the target. The latter also has exceptions, like storageClassName, making it even more confusing.

So ideally, I'd like to hold the mental model of 'if it is null in my values.yaml, it will not be present in the output Service object'.

@yuvipanda I can make it so for strings/integers without problem I think, but if we apply the same principle to {} and [] I believe there will be trouble such as helm emitting warnings, broken assumptions by helm template logic, and potentially a schema that fail to capture otherwise easy to capture issues.

I suggest we apply the principle that you suggest anyhow to our scalar strings and numbers specifically, but don't try to apply it on falsy values for object {} and array [], does that sound okay to you?

That's perfect! I totally agree, @consideRatio :)

Thanks for quickly iterating with me on this @yuvipanda!! I love it when I arrive at a concrete action point!

It was a quite hard rule principle to follow without making exceptions. I was able to allow many strings to be null so our default values are now null rather than "" by default, but plenty of booleans remain required to be "boolean" rather than "boolean or null" still.

Example of complexity arisen:
If helm templates have logic making an eq comparison between a variable and a string, then it will error if it holds a null value. And, since helm template logic will evaluate Y in statements like like if X and Y, one is forced to do if X then if Y then ... or if X and (Y | default "") etc.

Anyhow, things work okay now, don't want to change back, I don't see a perfect tradeoff with regards to these considerations.

Resolved by 2c1cc57 (that also includes some misc changes deemed relevant)

consideRatio · 2021-02-17T01:28:57Z

I think I've addressed all review points again and that this PR is potentially ready for a merge.

yuvipanda · 2021-02-17T08:40:13Z

@consideRatio happy to hit merge after the merge conflict is resolved.

The idea is to first bootstrap our schema with entries, and then do the work to describe all entries further for the configuration reference.

I decided that schema native values should always be set and not allowed to be null. But, for configuration values that are pass through configuration to JupyterHub and KubeSpawner for example, it made more sense to let them be null as the default of the Helm chart may be to not set it at all unless explicitly passed by a user.

consideRatio · 2021-02-17T09:06:13Z

@yuvipanda 🎉 rebased to resolve simple merge conflict

yuvipanda · 2021-02-17T09:37:15Z

\o/ THANK YOU, @consideRatio!

consideRatio · 2021-02-17T10:24:35Z

Wieeee ❤️ 🎉 thank you @yuvipanda!!

consideRatio marked this pull request as draft February 13, 2021 05:30

consideRatio force-pushed the pr/100-pct-schema.yaml branch from af9f7d7 to a3d6140 Compare February 13, 2021 05:31

consideRatio added documentation maintenance labels Feb 14, 2021

consideRatio changed the title ~~Increasing coverage of schema.yaml / configuration reference~~ values.schema.json ships with chart and configuration reference now covers all options Feb 15, 2021

consideRatio added enhancement breaking and removed maintenance labels Feb 15, 2021

consideRatio requested review from yuvipanda, minrk and manics February 15, 2021 03:35

consideRatio marked this pull request as ready for review February 15, 2021 03:36

yuvipanda approved these changes Feb 15, 2021

View reviewed changes

minrk reviewed Feb 15, 2021

View reviewed changes

consideRatio requested a review from minrk February 15, 2021 11:45

yuvipanda reviewed Feb 16, 2021

View reviewed changes

consideRatio added 10 commits February 17, 2021 10:01

Add FIXME note about userPlaceholder resources

d05132c

Remove unused hub.publicURL config

7027dd6

Add scheduling.userPlaceholder.resources default value

230c6bc

schema: don't require proxy.secretToken

0d6d8cc

schema: remove entry about deprecated schedulerStrategy

4e6e3ee

Add boilerplate schema entries with DESCRIBE ME comments

e03b247

The idea is to first bootstrap our schema with entries, and then do the work to describe all entries further for the configuration reference.

Specify a specific json schema version

8bf079a

schema: make root objects required

90df806

schema: refactor type from yaml to json array

cd66cf6

schema: add resources anchor

e90a185

consideRatio added 18 commits February 17, 2021 10:03

schema: add missing entry for hub.uid

19c4433

tool: add tool to compare schema/values content

906ec1f

tool: add script to generate values.schema.json from schema.yaml

8b58e80

schema: give string / integer not in lists be allowed to be null

b6619a4

ci: generate values.schema.json and run schema tests

cf6c8c2

tools/ci: don't forget to install pyyaml

962dc50

tool: generate-json-schema, emit completion message

1f20b3f

schema: fix storageClassName details

4f30f41

ci: correctly helm lint with lint-and-validate-values.yaml

5668891

schema: accept null pullPolicy

c525ee8

schema tools: avoid use of os.chdir

d5a5c1e

docs: handle if/then in schema logic

d7188a2

microfix: ensure consistent behavior for baseUrl

dbdd49a

schema: require key/cert when proxy.https.type=manual

dfca9b5

values/schema: prefer null strings over blank strings

c1eae4e

schema: remove too ambitious FIXME

b76335a

fix: accept user-placeholder resources

6c2a38a

consideRatio force-pushed the pr/100-pct-schema.yaml branch from 35fd5db to 6c2a38a Compare February 17, 2021 09:03

yuvipanda merged commit 6778795 into jupyterhub:master Feb 17, 2021

consideRatio mentioned this pull request Feb 25, 2021

Type mismatch between Kubespawner and Helm Schema #2068

Closed

consideRatio added new and removed enhancement labels Mar 9, 2021

This was referenced May 15, 2021

Schema validation to catch typos etc! #2199

Closed

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

Merged

consideRatio mentioned this pull request Jul 7, 2021

Add schema.yaml for helm3 compliant chart validation jupyterhub/binderhub#1331

Merged

consideRatio mentioned this pull request Aug 20, 2021

Helm chart: Package a values.schema.json file with the chart to provide end user config validation logic dask/dask-gateway#418

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

values.schema.json ships with chart and configuration reference now covers all options #2033

values.schema.json ships with chart and configuration reference now covers all options #2033

consideRatio commented Feb 13, 2021 •

edited

Loading

yuvipanda left a comment

yuvipanda Feb 15, 2021 •

edited by minrk

Loading

minrk Feb 15, 2021

consideRatio Feb 15, 2021 •

edited

Loading

consideRatio Feb 15, 2021

yuvipanda Feb 15, 2021

minrk Feb 15, 2021

consideRatio Feb 15, 2021

minrk commented Feb 15, 2021

minrk Feb 15, 2021

minrk Feb 15, 2021

consideRatio commented Feb 15, 2021

yuvipanda Feb 16, 2021

yuvipanda Feb 16, 2021

consideRatio Feb 16, 2021

yuvipanda Feb 16, 2021

consideRatio Feb 16, 2021

yuvipanda Feb 16, 2021

consideRatio Feb 16, 2021 •

edited

Loading

yuvipanda Feb 16, 2021

consideRatio Feb 16, 2021

consideRatio Feb 17, 2021 •

edited

Loading

consideRatio commented Feb 17, 2021

yuvipanda commented Feb 17, 2021

consideRatio commented Feb 17, 2021

yuvipanda commented Feb 17, 2021

consideRatio commented Feb 17, 2021

	os.chdir(os.path.dirname(sys.argv[0]))
	os.chdir(os.path.abspath(os.path.dirname(__file__)))

values.schema.json ships with chart and configuration reference now covers all options #2033

values.schema.json ships with chart and configuration reference now covers all options #2033

Conversation

consideRatio commented Feb 13, 2021 • edited Loading

Summary

Breaking changes

Implementation notes

Helper scripts

Accept null or not?

Notes on helm validation logic

yuvipanda left a comment

Choose a reason for hiding this comment

yuvipanda Feb 15, 2021 • edited by minrk Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

consideRatio Feb 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minrk commented Feb 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

consideRatio commented Feb 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

consideRatio Feb 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

consideRatio Feb 17, 2021 • edited Loading

Choose a reason for hiding this comment

consideRatio commented Feb 17, 2021

yuvipanda commented Feb 17, 2021

consideRatio commented Feb 17, 2021

yuvipanda commented Feb 17, 2021

consideRatio commented Feb 17, 2021

consideRatio commented Feb 13, 2021 •

edited

Loading

Notes on `helm` validation logic

yuvipanda Feb 15, 2021 •

edited by minrk

Loading

consideRatio Feb 15, 2021 •

edited

Loading

consideRatio Feb 16, 2021 •

edited

Loading

consideRatio Feb 17, 2021 •

edited

Loading