diff --git a/cmd/upgrade.go b/cmd/upgrade.go index 7e824937..3c3f7a33 100644 --- a/cmd/upgrade.go +++ b/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 { diff --git a/cmd/upgrade_test.go b/cmd/upgrade_test.go index 283e4597..bab777f9 100644 --- a/cmd/upgrade_test.go +++ b/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) + } +} diff --git a/scripts/issues/782.sh b/scripts/issues/782.sh new file mode 100755 index 00000000..45f1883a --- /dev/null +++ b/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 +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