fix: strip stale system labels from release labels on upgrade - #32421
fix: strip stale system labels from release labels on upgrade#32421bhuvan-somisetty wants to merge 2 commits into
Conversation
Secrets.List/Query and ConfigMaps.List/Query returned the raw object labels, including system labels (name, owner, status, version, createdAt, modifiedAt), unlike Get which already filters them. This let system labels ride along in lastRelease.Labels on upgrade and get merged into the new release, permanently baking stale createdAt values and system-label keys into the stored release body. Fixes helm#32404 Signed-off-by: bhuvan-somisetty <somisettybhuvan5@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR aligns the Kubernetes storage drivers’ read paths (List/Query) with Get by filtering out Helm system labels from the returned release .Labels, preventing those labels from being carried forward and merged into subsequent upgrades.
Changes:
- Apply
filterSystemLabels(...)toSecrets.List/Secrets.Queryresults. - Apply
filterSystemLabels(...)toConfigMaps.List/ConfigMaps.Queryresults. - Update Secrets/ConfigMaps driver tests to assert
List/Querydo not return system labels (e.g.,name) inrls.Labels.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/storage/driver/secrets.go | Filters system labels from .Labels in List and Query to match Get. |
| pkg/storage/driver/secrets_test.go | Updates list/query tests to assert system labels are not present in results. |
| pkg/storage/driver/cfgmaps.go | Filters system labels from .Labels in List and Query to match Get. |
| pkg/storage/driver/cfgmaps_test.go | Updates list/query tests to assert system labels are not present in results. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Thanks for jumping on this so fast @bhuvan-somisetty, and it's a clean diff — this does fix the upgrade path I reported ( One thing I'd want a maintainer to sign off on before it lands, though: filtering in That interaction is actually why I left the issue asking where the filter should live instead of sending a PR myself — the candidate spots have pretty different blast radius:
Not saying mirroring |
|
Hey @TerryHowe @gjenkins8 @mattfarina @robertsirc — whenever you get a chance, would appreciate a look at this one. Also looks like the CI workflows (CodeQL/build-test/golangci-lint) are stuck in |
…Query Filtering system labels in the k8s drivers' List/Query (as done in the previous commit) also strips them from helm list --selector results, since filterSelector matches against rls.Labels. That silently breaks `helm list -l owner=helm`, `-l status=deployed`, etc., which currently rely on List/Query returning system labels alongside custom ones. Move the fix to where the leak actually turns into a stored value: mergeCustomLabels in the upgrade path, which is what folds the previous release's labels into the new one. Stripping system labels there keeps List/Query/selector behavior unchanged while still preventing stale createdAt/modifiedAt and system-label keys from being carried into the new release's persisted Labels. Fixes helm#32404 Signed-off-by: bhuvan-somisetty <somisettybhuvan5@gmail.com>
|
@Mukuwul Good catch, thanks — you're right, I hadn't traced it through to Went with this over the write-side ( |
| // current comes from the previously stored release, which the k8s | ||
| // drivers (unlike Get) return with system labels still attached; strip | ||
| // them here so they don't ride along into the new release's Labels and | ||
| // clobber the fresh createdAt/modifiedAt set when it's persisted. | ||
| for _, k := range driver.GetSystemLabels() { | ||
| delete(labels, k) | ||
| } |
|
Fair point — the description was stale from before I switched approaches. Went with option (b): updated the title/description just now to reflect that the actual fix lives in |
Summary
helm upgradefetches the previous release viaLast→Query, and the k8s Secrets/ConfigMaps drivers'List/Queryreturn release labels straight from the object's labels (unlikeGet, which strips system labels viafilterSystemLabels).mergeCustomLabels(pkg/action/upgrade.go) then merged those labels into the new release'sLabelswithout stripping the system ones (name,owner,status,version,createdAt,modifiedAt), so each new revision inherited the previous revision'screatedAt/modifiedAt, and the release body's.Labelsmap permanently accumulated system-label keys.mergeCustomLabelsitself, right where the previous release's labels get folded into the new one.Note on approach: an earlier version of this PR filtered system labels directly in
Secrets.List/QueryandConfigMaps.List/Queryto mirrorGet. That was reverted —helm list --selectormatches against the labelsList/Queryreturn (pkg/action/list.gofilterSelector), so filtering there would silently break selectors like-l owner=helmor-l status=deployed. Fixing it inmergeCustomLabelsinstead only changes what upgrade persists, leavingList/Query/selector behavior untouched.Fixes #32404
Test plan
go test ./pkg/action/...andgo test ./pkg/storage/driver/...passTestMergeCustomLabelscovers a stale system label riding in viacurrentand confirms it's stripped from the merged resultTestUpgradeRelease_Labels/TestUpgradeRelease_SystemLabelsstill pass unchangedTestSecretList/TestSecretQuery/TestConfigMapList/TestConfigMapQueryare back to their original assertions (system labels still present inList/Queryresults, preserving selector behavior)