fix: skip hooks in the --take-ownership ownership check (#782)

cmd/upgrade.go

@@ -15,7 +15,9 @@ import (
 	"helm.sh/helm/v4/pkg/action"
 	"helm.sh/helm/v4/pkg/cli"
 	"helm.sh/helm/v4/pkg/kube"
+	releasev1 "helm.sh/helm/v4/pkg/release/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/api/meta"
 	"k8s.io/cli-runtime/pkg/resource"
 
 	"github.com/databus23/helm-diff/v3/diff"
@@ -433,6 +435,16 @@ func checkOwnership(d *diffCmd, resources kube.ResourceList, currentSpecs map[st
 			return err
 		}
 
+		// Helm only adopts resources from the release manifest. Hooks are kept
+		// out of it and carry no ownership annotations, so skip them here.
+		accessor, err := meta.Accessor(info.Object)
+		if err != nil {
+			return err
+		}
+		if _, isHook := accessor.GetAnnotations()[releasev1.HookAnnotation]; isHook {
+			return nil
+		}
+
 		helper := resource.NewHelper(info.Client, info.Mapping)
 		currentObj, err := helper.Get(info.Namespace, info.Name)
 		if err != nil {

cmd/upgrade_test.go

@@ -1,11 +1,23 @@
 package cmd
 
 import (
+	"io"
+	"net/http"
 	"os"
+	"path"
 	"path/filepath"
 	"slices"
 	"strings"
 	"testing"
+
+	"helm.sh/helm/v4/pkg/kube"
+	"k8s.io/apimachinery/pkg/api/meta"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/cli-runtime/pkg/resource"
+	"k8s.io/client-go/rest/fake"
+
+	"github.com/databus23/helm-diff/v3/manifest"
 )
 
 func TestIsRemoteAccessAllowed(t *testing.T) {
@@ -453,3 +465,80 @@ func TestThreeWayMergeModeEnvVarOnlyAppliesToThreeWayMerge(t *testing.T) {
 		})
 	}
 }
+
+// ownershipTestInfo returns a rendered ConfigMap backed by a fake API server
+// that serves the given live objects by name. Fetching a name listed in
+// mustNotGet fails the test.
+func ownershipTestInfo(t *testing.T, name string, annotations map[string]interface{}, live map[string]string, mustNotGet map[string]bool) *resource.Info {
+	t.Helper()
+	obj := &unstructured.Unstructured{Object: map[string]interface{}{
+		"apiVersion": "v1",
+		"kind":       "ConfigMap",
+		"metadata": map[string]interface{}{
+			"name":        name,
+			"namespace":   "default",
+			"annotations": annotations,
+		},
+	}}
+	client := &fake.RESTClient{
+		NegotiatedSerializer: resource.UnstructuredPlusDefaultContentConfig().NegotiatedSerializer,
+		Client: fake.CreateHTTPClient(func(req *http.Request) (*http.Response, error) {
+			header := http.Header{"Content-Type": []string{"application/json"}}
+			liveName := path.Base(req.URL.Path)
+			if mustNotGet[liveName] {
+				t.Errorf("checkOwnership fetched the live object of hook %q", liveName)
+			}
+			body, ok := live[liveName]
+			if !ok {
+				return &http.Response{StatusCode: http.StatusNotFound, Header: header, Body: io.NopCloser(strings.NewReader(`{"kind":"Status","apiVersion":"v1","status":"Failure","reason":"NotFound","code":404}`))}, nil
+			}
+			return &http.Response{StatusCode: http.StatusOK, Header: header, Body: io.NopCloser(strings.NewReader(body))}, nil
+		}),
+	}
+	return &resource.Info{
+		Client:    client,
+		Namespace: "default",
+		Name:      name,
+		Object:    obj,
+		Mapping: &meta.RESTMapping{
+			Resource:         schema.GroupVersionResource{Version: "v1", Resource: "configmaps"},
+			GroupVersionKind: schema.GroupVersionKind{Version: "v1", Kind: "ConfigMap"},
+			Scope:            meta.RESTScopeNamespace,
+		},
+	}
+}
+
+func TestCheckOwnershipSkipsHooks(t *testing.T) {
+	live := map[string]string{
+		// A live object without the hook annotation: the check has to rely on
+		// the rendered object, like Helm does.
+		"hook":      `{"apiVersion":"v1","kind":"ConfigMap","metadata":{"name":"hook","namespace":"default"}}`,
+		"test-hook": `{"apiVersion":"v1","kind":"ConfigMap","metadata":{"name":"test-hook","namespace":"default","annotations":{"helm.sh/hook":"test"}}}`,
+		"unmanaged": `{"apiVersion":"v1","kind":"ConfigMap","metadata":{"name":"unmanaged","namespace":"default"}}`,
+		"owned":     `{"apiVersion":"v1","kind":"ConfigMap","metadata":{"name":"owned","namespace":"default","annotations":{"meta.helm.sh/release-name":"rel","meta.helm.sh/release-namespace":"default"}}}`,
+	}
+	hooks := map[string]bool{"hook": true, "test-hook": true}
+	resources := kube.ResourceList{
+		ownershipTestInfo(t, "hook", map[string]interface{}{"helm.sh/hook": "pre-install,pre-upgrade"}, live, hooks),
+		ownershipTestInfo(t, "test-hook", map[string]interface{}{"helm.sh/hook": "test"}, live, hooks),
+		ownershipTestInfo(t, "unmanaged", nil, live, hooks),
+		ownershipTestInfo(t, "owned", nil, live, hooks),
+	}
+	currentSpecs := make(map[string]*manifest.MappingResult)
+
+	newOwnedReleases, err := checkOwnership(&diffCmd{release: "rel", namespaces: namespaces{namespace: "default"}}, resources, currentSpecs)
+	if err != nil {
+		t.Fatalf("checkOwnership returned an error: %v", err)
+	}
+
+	const unmanagedKey = "default, unmanaged, ConfigMap (v1)"
+	if len(newOwnedReleases) != 1 {
+		t.Fatalf("expected an ownership change for %q only, got %v", unmanagedKey, newOwnedReleases)
+	}
+	if got := newOwnedReleases[unmanagedKey]; got.OldRelease != "" || got.NewRelease != "default/rel" {
+		t.Errorf("unexpected ownership change for %q: %+v", unmanagedKey, got)
+	}
+	if _, ok := currentSpecs[unmanagedKey]; !ok || len(currentSpecs) != 1 {
+		t.Errorf("expected only %q in the current specs, got %v", unmanagedKey, currentSpecs)
+	}
+}

scripts/issues/782.sh

@@ -0,0 +1,105 @@
+#!/usr/bin/env bash
+# Reproduction test for https://github.com/databus23/helm-diff/issues/782
+#
+# Bug: "Diff failing when diffing helm hook jobs with --take-ownership flag".
+#
+# Helm only adopts resources from the release manifest. Hooks are kept out of
+# it and never get the meta.helm.sh/release-* annotations, so an unchanged hook
+# must not be reported as "changed ownership" by --take-ownership.
+#
+# Scenarios (each prints its full diff output to the CI log):
+#   A. plain diff of an unchanged chart with a hook Job (baseline, not checked)
+#   B. --take-ownership diff of the same unchanged chart
+#   C. --take-ownership still reports a resource that no release owns
+
+set -euo pipefail
+
+ISSUE=782
+# shellcheck source=lib.sh
+source "$(dirname "$0")/lib.sh"
+
+# check_ownership <scenario> <resource name> <reported|not-reported>
+check_ownership() {
+  local scenario="$1" name="$2" want="$3" got
+  local out="$WORK/${scenario// /_}.out"
+  strip_ansi < "$out" > "$WORK/stripped.out"
+  # A failed diff prints no ownership changes at all, so it must not pass.
+  if grep -q '^Error:' "$WORK/stripped.out"; then
+    echo "FAIL: scenario [$scenario] helm diff failed (#${ISSUE})"
+    FAIL=1
+    return
+  fi
+  if grep -q ", ${name}, .* changed ownership:" "$WORK/stripped.out"; then
+    got=reported
+  else
+    got=not-reported
+  fi
+  if [ "$got" = "$want" ]; then
+    echo "OK: scenario [$scenario] ownership change for $name is $want"
+  else
+    echo "FAIL: scenario [$scenario] expected ownership change for $name to be $want, got $got (#${ISSUE})"
+    FAIL=1
+  fi
+}
+
+kubectl create namespace "$NS" 2>/dev/null || true
+
+HOOK_CHART='apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: res-a
+data:
+  foo: bar
+---
+apiVersion: batch/v1
+kind: Job
+metadata:
+  name: hook-a
+  annotations:
+    "helm.sh/hook": pre-install,pre-upgrade
+spec:
+  template:
+    spec:
+      containers:
+      - name: hook
+        image: busybox:1.36
+        command: ["true"]
+      restartPolicy: Never
+'
+
+chart "$WORK/a" "$HOOK_CHART"
+helm upgrade -i rel-a "$WORK/a" -n "$NS" >/dev/null
+echo "===== live hook annotations ====="
+kubectl get job hook-a -n "$NS" -o jsonpath='{.metadata.annotations}'; echo
+
+###############################################################################
+# Variant A: plain diff, logged as a baseline (no ownership check involved)
+###############################################################################
+run_diff "A plain" noassert rel-a "$WORK/a" -n "$NS"
+
+###############################################################################
+# Variant B: --take-ownership must skip the hook
+###############################################################################
+run_diff "B take-ownership" noassert rel-a "$WORK/a" -n "$NS" --take-ownership
+check_ownership "B take-ownership" hook-a not-reported
+check_ownership "B take-ownership" res-a not-reported
+
+###############################################################################
+# Variant C: a resource created outside Helm is still reported
+###############################################################################
+kubectl create configmap unowned-c -n "$NS" --from-literal=foo=bar --dry-run=client -o yaml \
+  | kubectl apply -f - >/dev/null
+chart "$WORK/c" "${HOOK_CHART}---
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: unowned-c
+data:
+  foo: bar
+"
+run_diff "C take-ownership" noassert rel-a "$WORK/c" -n "$NS" --take-ownership
+check_ownership "C take-ownership" unowned-c reported
+check_ownership "C take-ownership" hook-a not-reported
+
+###############################################################################
+finish