fix: skip hooks in the --take-ownership ownership check (#782) - #1074
Conversation
yxxhero
left a comment
yxxhero
left a comment
There was a problem hiding this comment.
I did a deep pass over this and everything holds together. What I verified beyond the tests:
- Hook detection matches Helm's own. Helm v4 splits hooks out of the manifest with a key-presence check on the same constant (
pkg/release/v1/util/manifest_sorter.gousesentry.Metadata.Annotations[release.HookAnnotation],if !ok -> generic), so_, isHook := accessor.GetAnnotations()[releasev1.HookAnnotation]is exactly the right predicate, and skipping beforehelper.Getavoids a pointless API round-trip per hook. - The annotation survives
manifest.Generate.checkOwnershipbuilds its resource list from the post-Generate install manifest, so this only works ifhelm.sh/hookis still there afterwards. It is: the to-be-created branch marshals the rendered object, the to-be-updated branch marshals the live hook (Helm creates hooks with their template annotations intact), anddeleteStatusAndTidyMetadataonly prunesmeta.helm.sh/release-*,deployment.kubernetes.io/revisionand friends. That's a non-obvious invariant — noted inline. - No diff coverage is lost. Skipping hooks here doesn't make them invisible:
Generatealready appends the live hook to the release manifest and the merged hook to the install manifest, so an unchanged hook still diffs as unchanged. The fix only removes the bogus ownership report (and with--detailed-exitcode, the spurious exit code 2). - The test-hook crash claim checks out.
isHook()compares the annotation with==, so a livehelm.sh/hook: testobject is dropped byparseContent,ParseObjectthen returnsfailed to parse content of Kubernetes resourceand the whole diff used to die on it. - CI is green on all legs, including the integration runs that execute
scripts/issues/782.shon kind against Helm v3.18.6 / v3.22.0 / v4.3.0.
On the deferred second case of #782: agreed it shouldn't block this. For the follow-up, one direction that mirrors Helm's recreate-not-patch behavior would be to treat target resources that carry the hook annotation specially in manifest.Generate — take the previous release's hook manifest (from helm get hooks) as the original side and emit the rendered target as the new side, instead of three-way patching the live object. That would also kill the field is immutable failure for Jobs whose template changed. Happy to see it as a separate PR.
| accessor, err := meta.Accessor(info.Object) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
One invariant worth keeping in mind for the future: this checks the annotation on the object built from the post-manifest.Generate install manifest, not the raw rendered one. That works today because Generate preserves helm.sh/hook in both of its branches (rendered object marshaled for to-be-created, live object for to-be-updated) and deleteStatusAndTidyMetadata only prunes meta.helm.sh/release-* style annotations. If someone ever adds helm.sh/hook to that tidy list, hooks would silently start being reported as ownership changes again — the unit test here would catch it, which is good.
| currentSpecs := make(map[string]*manifest.MappingResult) | ||
|
|
||
| newOwnedReleases, err := checkOwnership(&diffCmd{release: "rel", namespaces: namespaces{namespace: "default"}}, resources, currentSpecs) | ||
| if err != nil { |
There was a problem hiding this comment.
Nice regression property: this fails on master in two independent ways — mustNotGet fires the moment checkOwnership fetches the live hook, and the hook entries pollute newOwnedReleases/currentSpecs even if the fetch were allowed. The owned case (same release) and unmanaged case (no release) also pin down that the skip doesn't over-suppress.
| if grep -q '^Error:' "$WORK/stripped.out"; then | ||
| echo "FAIL: scenario [$scenario] helm diff failed (#${ISSUE})" | ||
| FAIL=1 | ||
| return |
There was a problem hiding this comment.
The ^Error: guard is a good call — without it a hard failure would print no ownership lines at all and every not-reported assertion would vacuously pass. Same shape as the guard the 1064 script needs for its noassert baseline.
yxxhero
left a comment
yxxhero
left a comment
There was a problem hiding this comment.
Approving from my side — the fix matches Helm's own hook-splitting semantics, the analysis in my earlier review holds (annotation survives manifest.Generate, no diff coverage lost, test-hook parse failure also addressed), and all CI legs including the kind integration runs of scripts/issues/782.sh are green.
2 participants
With
--take-ownership,checkOwnershipvisits every rendered resource, hooks included. Helm keeps hooks out of the release manifest and never adds themeta.helm.sh/release-*annotations to them, so every live hook was reported aschanged ownershipeven when nothing changed (the first case in #782). With--detailed-exitcodethat also turns a no-op upgrade into exit code 2.This skips resources whose rendered object has the
helm.sh/hookannotation, which is the same check Helm uses to split hooks out of the manifest. It also stops a leftover test hook (e.g. the pod fromhelm test) from failing the whole diff withfailed to parse content of Kubernetes resource.Tests:
TestCheckOwnershipSkipsHooksfails on master and passes with this change.scripts/issues/782.shfollows the Bug: helm-diff shows- labels, if resource contains onlyapp.kubernetes.io/managed-bylabel #1064 script. On kind it fails with the current build and passes with this one, on Helm v3.18.1 and v4.3.0.The second case in #782 (a changed hook Job fails with
field is immutable) is not fixed here.manifest.Generatethree-way patches live hooks like any other resource, and that also happens with plain--three-way-merge. Fixing it means deciding what a hook should be compared against, since Helm recreates hooks instead of patching them. Happy to follow up if you have a preferred direction.Refs #782
AI disclosure: I used Claude Code while working on this.