Conversation
ef34886 to
4c65745
Compare
|
@scottrigby @gjenkins8 here is the hip we talked about at contribfest |
|
+1 |
|
Some notes that were brought up to improve the proposal were to:
|
Here is an example of getting flux CRs to control if an upgrade job runs. It was part of this MR. It was used for a default name change on immutable resources by deleting them in a hook. |
d819dc7 to
b48d8f5
Compare
hips/hip-0027.md
Outdated
|
|
||
| ## Rejected Ideas | ||
|
|
||
| - **Full Release Object:** Security/performance concerns; chart metadata sufficient |
There was a problem hiding this comment.
my main thought is, would you not also want to expose the values of helm history to check things like whether a previous release was successful?
Some thoughts about this: What about adding "History" key to the existing .Release built-in: .Release.History when a --release-history-depth flag is passed?
This could be a filtered version of the Release object that's currently stored in helm release secrets by default.
Agree that the full release object could cause performance issues. perhaps this could be mitigated by:
- any history might be best to opt-into with a flag (default history level 0 rather than 1 as currently proposed)
- including only a filtered subset of the release object, omitting potentially very large data like templates, manifest etc.
Not yet sure what security concerns adding the Release object would add that other built-ins don't already have (eg, .Values, .Files, etc). But good to consider here.
There was a problem hiding this comment.
Thank you for the thoughtful suggestions! .Release.History feels natural and being part of an existing path makes it that much easier to find and use. Pulling the wider set of data is certainly more useful, it may be worth considering letting the user control the filter and default most things to discarded by default. This keeps with the opt-in nature of this proposal. I agree defaulting to 0 is better for similar reasons.
One suggestion I have on top of this would be to ensure that the user's decisions are available to the templates at the same time. This enables a "negotiation" between the user and author, i.e. a chart author may need a depth of 2 to render a chart properly and error out if not given it, but it doesn't force the condition on the user, they must accept first. It could also enable helpful error messages, extend testing, and branching logic based on available history.
Example use case:
{{- if lt .Release.HistoryDepth 3 }}
{{- fail "This chart requires --release-history-depth=3 for safe upgrades from v2.x" }}
{{- end }}Proposed field: .Release.HistoryDepth (aligns with flag name)
This would be a simple integer value (0, 1, 2, etc.) always available in template context, similar to how .Release.Revision is currently available.
What do you think of that addition? Should we determine what things should be filtered by default as part of this?
Again, really appreciate the review. I'll update the HIP to use .Release.History and associated changes.
There was a problem hiding this comment.
Release.HistoryDepth is redundant here, and equivalent to:
{{- if lt (len .Release.History) 3 }}There was a problem hiding this comment.
will reply with this and the rest inline
There was a problem hiding this comment.
agreed, removed and referenced .Release.History and .Release.Revision checks instead. Also, when reviewing the hip and your comments, I think adding max makes sense because that's the terminology history uses.
scottrigby
left a comment
There was a problem hiding this comment.
getting closer. just a few more questions and suggestions.
hips/hip-0027.md
Outdated
|
|
||
| This HIP proposes exposing release history metadata during template rendering. Currently, Helm templates have access to `.Chart` for the chart being installed but no equivalent access to deployed release history. This forces chart authors to use complex workarounds like post-renderers, pre-upgrade hooks, or manual values conventions to implement version-aware upgrade logic. | ||
|
|
||
| The proposal introduces `.Release.History` (array of historical releases) available in template contexts, populated during `helm upgrade` and `helm rollback` operations. The `--release-history-depth` flag controls how many historical releases to retrieve (default: 0), requiring explicit opt-in. The `.Release.HistoryDepth` field exposes the requested depth value to enable chart authors to require minimum depth for safe upgrades. |
There was a problem hiding this comment.
I'd remove .Release.HistoryDepth, as it's functionally equivalent to len .Release.History in gotemplate. Eg,
{{- if lt (len .Release.History) 3 }}
hips/hip-0027.md
Outdated
| {{- end }} | ||
|
|
||
| # Enforce minimum depth requirement | ||
| {{- if and (gt (len .Release.History) 0) (lt .Release.HistoryDepth 2) }} |
There was a problem hiding this comment.
I was going to suggest a change here but I don't actually understand how this use case differs from the above cases.
There was a problem hiding this comment.
the logic is bad here, the intent was to be able to require a depth if a depth is available, but that achievable with what you already recommended, see #421 (comment)
hips/hip-0027.md
Outdated
|
|
||
| **Excluded by default:** | ||
|
|
||
| - Values (may contain secrets) |
There was a problem hiding this comment.
I don't understand excluding this one. Templates already have access to .Values. And helm templating can already access previous secrets if needed via lookup() (it would be very cumbersome, but already accessible in templating).
There was a problem hiding this comment.
I was being overly conservative. I think a better approach is to keep values out by default, for both attack surface and performance reasons, but still enable it's use/requirement from users/chart maintainers.
hips/hip-0027.md
Outdated
|
|
||
| ## Security Implications | ||
|
|
||
| **Not Exposed:** Previous values (may contain secrets) or previous manifests (may contain sensitive data). Only filtered release metadata is exposed (chart metadata, release status, timestamps, revision numbers). |
There was a problem hiding this comment.
unclear why this would increase exposure. see note about existing .Values and lookup() func above.
8527341 to
f1cfad4
Compare
f1cfad4 to
cd01bdc
Compare
|
I accidentally messed up the history on my branch, so I ended up squashing it all into one commit to fix |
There was a problem hiding this comment.
@mrlunchbox777 nice work 👍 lgtm. Giving github approval even though standard HIP changes will need to be made before merging.
- the
hipfrontmatter field value with the assigned number. the next available number will be assigned once we get another approval from a core maintainer (because this HIP is aimed at helm core)- currently the next available number would be 0029, since 0028 is assigned to #370 (comment)
- make sure the hip file name matches the assigned number
- add a link to the file from hips/README.md
For process, see hip-0001.
hips/hip-0028.md
Outdated
| {{- end }} | ||
|
|
||
| # Require minimum depth for safe upgrades | ||
| {{- if lt .Release.HistoryDepth 3 }} |
There was a problem hiding this comment.
you're right, i missed this in my re-review - thought my suggestion had been merged. Needs to be fixed.
There was a problem hiding this comment.
Thanks! Good catch, fixed
hips/hip-0028.md
Outdated
|
|
||
| ### Populated Only During Upgrades/Rollbacks | ||
|
|
||
| `.Release.History` contains release metadata only during `helm upgrade` and `helm rollback` when deployed releases exist and `--release-history-max` is greater than 0. During rollback, the history reflects releases deployed before the rollback target. It's empty for: |
There was a problem hiding this comment.
Is it correct that rollback adds to history, it never deletes?
There was a problem hiding this comment.
yes it would follow helm's normal process or adding release history (per helm history --help)
There was a problem hiding this comment.
Clarified rollback and release history behavior here
hips/hip-0028.md
Outdated
|
|
||
| **Not Exposed:** Previous values (may contain secrets) or previous manifests (may contain sensitive data). Only filtered release metadata is exposed (chart metadata, release status, timestamps, revision numbers). | ||
|
|
||
| **Considerations:** Chart authors should not store sensitive data in Chart.yaml. The default `--release-history-depth 0` provides opt-out by default. Higher depth values increase data exposure; use the minimum required. Release status and metadata are already stored in cluster secrets by Helm, so this doesn't expose data that isn't already persisted. |
There was a problem hiding this comment.
| **Considerations:** Chart authors should not store sensitive data in Chart.yaml. The default `--release-history-depth 0` provides opt-out by default. Higher depth values increase data exposure; use the minimum required. Release status and metadata are already stored in cluster secrets by Helm, so this doesn't expose data that isn't already persisted. | |
| **Considerations:** Chart authors should not store sensitive data in Chart.yaml. The default `--release-history-max 0` provides opt-out by default. Higher depth values increase data exposure; use the minimum required. Release status and metadata are already stored in cluster secrets by Helm, so this doesn't expose data that isn't already persisted. |
hips/hip-0028.md
Outdated
|
|
||
| # Require minimum depth for safe upgrades | ||
| {{- if lt .Release.HistoryDepth 3 }} | ||
| {{- fail "This chart requires --release-history-depth=3 for safe upgrades from v2.x" }} |
There was a problem hiding this comment.
| {{- fail "This chart requires --release-history-depth=3 for safe upgrades from v2.x" }} | |
| {{- fail "This chart requires --release-history-max=3 for safe upgrades from v2.x" }} |
8d5b968 to
4d5ba34
Compare
4d5ba34 to
ab30b96
Compare
all threads addressed and replied to, steps above taken. Thank you to everyone who worked on this! |
|
DCO issue |
Signed-off-by: Andrew Shoell <mrlunchbox777@gmail.com>
… to focus on using the history verbage Signed-off-by: Andrew Shoell <mrlunchbox777@gmail.com>
Signed-off-by: Andrew Shoell <mrlunchbox777@gmail.com>
Signed-off-by: Andrew Shoell <mrlunchbox777@gmail.com>
61ad3d2 to
94d7ba4
Compare
ooops, thanks, fixed |
Adding a HIP to expose the currently deployed chart at render time.