Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

H4HIP: Default to strict Chart.yaml loading #371

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+61 −0
Open

H4HIP: Default to strict Chart.yaml loading #371

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 61 additions & 0 deletions hips/hip-9999.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
---
hip: 9999
title: "Default to strict `Chart.yaml` loading"
authors: [ "George Jenkins <[email protected]>" ]
created: "2024-11-18"
type: "feature"
status: "draft"
---

## Abstract

Helm should default to using strict-mode yaml loading for loading charts ie. `yaml.UnmarshalStrict(..)` rather than `yaml.Unmarshal(..)`.


## Motivation

Currently Helm loads chart's Chart.yaml in "non-strict" yaml loading mode.
The main consequence of this behavior is that Helm ignores unknown and duplicate fields in `Chart.yaml`.
Either unknown to the present version of Helm, or simply user typos.
The result is that the behavior the user expects from Helm as described by these unknown/duplicate fields is effectively ignored.
Presenting a confusing experience to the user.


## Specification

Helm will default to using strict-mode yaml loading for loading charts ie. ie. `yaml.UnmarshalStrict(..)` rather than `yaml.Unmarshal(..)`.

The CLI/SDK will contain an option to allow non-strict loading.
This is to allow users to continue to utilize charts that are malformed.


## Backwards compatibility

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.
Comment on lines +34 to +37
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should probably word this more into the HIP. The two behaviors I want to prevent are:

  1. Typos in Chart.yaml e.g. 2dependencies fix(pkg/lint): unmarshals Chart.yaml strictly helm#12382 (comment)
  2. 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.


## Security implications

N/A


## How to teach this

Update `Chart.yaml` docs


## Reference implementation

See PR links in [References](#references)


## References

- \[chart/loader\] use strict yaml unmarshaling for chart files <https://github.com/helm/helm/pull/11818>
- fix(pkg/lint): unmarshals Chart.yaml strictly <https://github.com/helm/helm/pull/12382>
- [HIP-9999][hip-9999] ("Forward compatibility: `Chart.yaml` `minimumHelmVersion`"), for a complementary HIP that allows further tightening of Helm's forward compatibility behavior.

[hip-9999]: ./hip-9999.md