H4HIP: Default to strict Chart.yaml loading
#371
Conversation
Signed-off-by: George Jenkins <[email protected]>
gjenkins8
force-pushed
the
hip_strict_chart_loading
branch
from
f6da5f7 to
c6136b6
Compare
gjenkins8
changed the title
Chart.yaml loadingChart.yaml loading
| Helm 4 allows breaking behavioral changes. | ||
| However it is a core premise that Helm 4 will remain compatibility with existing chart. | ||
| Moving to strict mode for loading yaml will prevent charts from being loaded that contain unknown/invalid fields. | ||
| Requiring a command line flag / SDK option allows fallback compatibility if required. |
There was a problem hiding this comment.
If we think about backwards compatibility in the future, it means we will never be able to add another field to the Chart.yaml file in the current apiVersion. Older versions of Helm, that will be in wide use, will fail to load the chart.
Do we want to catch any issues with strict YAML loading or a different method?
Consider the case of a chart consumer who didn't create it and just uses Helm to install an app into their cluster. They aren't a Helm expert and likely not a k8s expert.
There was a problem hiding this comment.
An alternative to the CLI flag would be to attempt to strict load, and fall back to non-strict load? Emit a diagnostic that Chart.yaml had unknown fields if non-strict loading succeeds where strict loading failed.
The behavior here, aside from the error message, would be the same from the user perspective. But slightly more seamless.
There was a problem hiding this comment.
I should probably word this more into the HIP. The two behaviors I want to prevent are:
- Typos in Chart.yaml e.g.
2dependenciesfix(pkg/lint): unmarshals Chart.yaml strictly helm#12382 (comment) - A new field is added to
Chart.yaml(which describes some new functionality Helm should do). Where the chart author uses a new version of Helm (which understands this field), and the chart consumer uses an old version of Helm (which does not)
From the chart consumer perspective, both are bad scenarios. Though 1. the onus of the error is on the chart producer, and much more amendable catching via linting (I don't think it is contentious here to make helm lint load yaml strictly..) I'll focus on 2., which is more IMHO a function of the Chart ecosystem. Though 1. will benefit from strict loading if (as a user) you manage to get a chart with a typo'd yaml.
To illustrate 2., an example could be we added to Chart.yaml a "resource order" field, which describes the order for which Helm will need to deploy resources in order for the chart to deploy successfully. A chart consumer who (attempts to) install this chart with an old version of Helm is in a broken situation (either the chart fails to install and Helm returns an error. Or Helm thinks the chart installs correctly and succeeds, but the application is in a broken state).
I do see #370 (a minimumHelmVersion field) as a prerequisite to this strict loading / complimentary to this (proposed) HIP. I very much want the user error to be "this chart requires Helm version 4.3.0 or greater", and not "unknown field 'foo' loading Chart.yaml".
And the general theme of these HIPs, is how do we protect / support users (chart consumers) on old versions of Helm. ie. how to we manage forward compatibility with Helm. Given significant changes to Helm are likely in the future. When they invariably attempt to use a Chart that does take advantage of new Helm functionality.
No description provided.