diff --git a/cmd/upgrade.go b/cmd/upgrade.go index 3c3f7a33..31c4fdb0 100644 --- a/cmd/upgrade.go +++ b/cmd/upgrade.go @@ -356,9 +356,13 @@ func (d *diffCmd) runHelm3() error { } var actionConfig *action.Configuration + releaseNs := d.namespace if d.threeWayMerge || d.takeOwnership { actionConfig = new(action.Configuration) localEnv := prepareEnvSettings(d.kubeContext) + if releaseNs == "" { + releaseNs = localEnv.Namespace() + } storageNs := d.storage() if storageNs == "" { storageNs = localEnv.Namespace() @@ -373,7 +377,8 @@ func (d *diffCmd) runHelm3() error { if d.threeWayMerge { releaseManifest, installManifest, err = manifest.Generate(actionConfig, releaseManifest, installManifest, - manifest.WithThreeWayMergeMode(manifest.ThreeWayMergeMode(d.threeWayMergeMode))) + manifest.WithThreeWayMergeMode(manifest.ThreeWayMergeMode(d.threeWayMergeMode)), + manifest.WithRelease(d.release, releaseNs)) if err != nil { return fmt.Errorf("unable to generate manifests: %w", err) } diff --git a/manifest/generate.go b/manifest/generate.go index cf3731a7..402e6238 100644 --- a/manifest/generate.go +++ b/manifest/generate.go @@ -16,6 +16,7 @@ import ( authorizationv1 "k8s.io/api/authorization/v1" apiextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" @@ -54,7 +55,9 @@ var ValidThreeWayMergeModes = []string{ } type generateOptions struct { - mergeMode ThreeWayMergeMode + mergeMode ThreeWayMergeMode + release string + releaseNamespace string } // GenerateOption customizes the behavior of Generate. @@ -68,6 +71,16 @@ func WithThreeWayMergeMode(mode ThreeWayMergeMode) GenerateOption { } } +// WithRelease names the release the manifests belong to. The three-way merge +// then adds the release's ownership metadata to the new manifest, the way helm +// upgrade does, so that the computed patch matches the one the upgrade sends. +func WithRelease(name, namespace string) GenerateOption { + return func(o *generateOptions) { + o.release = name + o.releaseNamespace = namespace + } +} + func Generate(actionConfig *action.Configuration, originalManifest, targetManifest []byte, opts ...GenerateOption) ([]byte, []byte, error) { options := generateOptions{mergeMode: ThreeWayMergeAuto} for _, opt := range opts { @@ -165,7 +178,16 @@ func Generate(actionConfig *action.Configuration, originalManifest, targetManife return fmt.Errorf("could not find %q", info.Name) } - patch, err := createPatch(originalInfo.Object, currentObj, info) + target := info + if options.release != "" { + withMetadata := *info + if withMetadata.Object, err = withReleaseMetadata(info.Object, options.release, options.releaseNamespace); err != nil { + return fmt.Errorf("adding release metadata to %q with kind %s: %w", info.Name, kind, err) + } + target = &withMetadata + } + + patch, err := createPatch(originalInfo.Object, currentObj, target) if err != nil { return err } @@ -673,6 +695,38 @@ func createPatch(originalObj, currentObj runtime.Object, target *resource.Info) return patch, nil } +// withReleaseMetadata returns a copy of obj carrying the ownership metadata that +// helm install, upgrade and rollback merge into every object before computing +// the patch. A three-way patch built without it nulls a map the chart renders +// empty, and with it everything that was added to the map outside the chart. +func withReleaseMetadata(obj runtime.Object, release, namespace string) (runtime.Object, error) { + obj = obj.DeepCopyObject() + accessor, err := meta.Accessor(obj) + if err != nil { + return nil, err + } + + accessor.SetLabels(withEntries(accessor.GetLabels(), map[string]string{ + "app.kubernetes.io/managed-by": "Helm", + })) + accessor.SetAnnotations(withEntries(accessor.GetAnnotations(), map[string]string{ + "meta.helm.sh/release-name": release, + "meta.helm.sh/release-namespace": namespace, + })) + return obj, nil +} + +// withEntries adds entries to m, allocating m if it is nil. +func withEntries(m, entries map[string]string) map[string]string { + if m == nil { + m = make(map[string]string, len(entries)) + } + for k, v := range entries { + m[k] = v + } + return m +} + // dropNulls removes every null-valued key from the JSON object in data, at any // depth of nested objects. A list is left as it is, since a merge patch // replaces a list whole. diff --git a/manifest/generate_test.go b/manifest/generate_test.go index 99367087..1d28e1b3 100644 --- a/manifest/generate_test.go +++ b/manifest/generate_test.go @@ -18,6 +18,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/strategicpatch" "k8s.io/cli-runtime/pkg/resource" "sigs.k8s.io/yaml" ) @@ -113,6 +114,60 @@ func TestCreatePatchApplyLocally_StrategicRemoval(t *testing.T) { assert.Equal(t, map[string]string{"keep": "me"}, got.ObjectMeta.Labels) } +// A chart that renders `annotations:` or `labels:` with nothing under it leaves +// the map null in both manifests. helm upgrade adds its ownership metadata to +// the object before it computes the patch, so the patch it sends does not null +// the map, and what was added to it outside the chart survives. The patch sent +// for the server-side dry-run must do the same. +func TestCreatePatch_KeepsMetadataAddedOutsideTheChartUnderANullMap(t *testing.T) { + chart := ` +apiVersion: v1 +kind: ConfigMap +metadata: + name: res + namespace: default + annotations: + labels: +data: {foo: bar} +` + live := fromYAML(t, ` +apiVersion: v1 +kind: ConfigMap +metadata: + name: res + namespace: default + annotations: + meta.helm.sh/release-name: rel + meta.helm.sh/release-namespace: default + example.com/added-outside: "yes" + labels: + app.kubernetes.io/managed-by: Helm + example.com/added-outside: "yes" +data: {foo: bar} +`) + + target, err := withReleaseMetadata(fromYAML(t, chart), "rel", "default") + require.NoError(t, err) + + patch, err := createPatch(fromYAML(t, chart), live, infoFor(t, target, corev1.SchemeGroupVersion.WithKind("ConfigMap"))) + require.NoError(t, err) + + liveJSON, err := live.MarshalJSON() + require.NoError(t, err) + // Apply the patch the way the API server does, without the post-processing + // of the local merge. + merged, err := strategicpatch.StrategicMergePatchUsingLookupPatchMeta(liveJSON, patch.data, patch.patchMeta) + require.NoError(t, err) + + var got map[string]interface{} + require.NoError(t, json.Unmarshal(merged, &got)) + for _, field := range []string{"annotations", "labels"} { + value, found := nested(t, got, "metadata", field, "example.com/added-outside") + assert.True(t, found, "the %s added outside the chart must survive", field) + assert.Equal(t, "yes", value) + } +} + // TestCreatePatchApplyLocally_Unstructured covers custom resources, which use a // plain JSON merge patch instead of a strategic merge patch. func TestCreatePatchApplyLocally_Unstructured(t *testing.T) { diff --git a/scripts/issues/1101.sh b/scripts/issues/1101.sh new file mode 100755 index 00000000..26773e2b --- /dev/null +++ b/scripts/issues/1101.sh @@ -0,0 +1,119 @@ +#!/usr/bin/env bash +# Reproduction test for https://github.com/databus23/helm-diff/issues/1101 +# +# Bug: --three-way-merge reports the removal of every live +# annotation (or label) when the chart renders the map as null. +# +# A chart that renders `annotations:` with nothing under it produces a null map. +# helm upgrade merges its ownership metadata (meta.helm.sh/release-name and +# meta.helm.sh/release-namespace, app.kubernetes.io/managed-by) into every +# object before it computes the three-way patch, so the map it sends is never +# null and annotations added outside the chart survive the upgrade. helm-diff +# must compute the same patch and report no change. +# +# Scenarios (each prints its full diff output to the CI log): +# A. null annotations, live object annotated outside the chart +# B. null labels, live object labelled outside the chart +# C. A with --three-way-merge-mode client +# D. an annotation the chart sets, edited outside the chart, is still drift + +set -euo pipefail + +ISSUE=1101 +# shellcheck source=lib.sh +source "$(dirname "$0")/lib.sh" + +kubectl create namespace "$NS" 2>/dev/null || true + +# expect_exit fails the +# test unless helm diff --detailed-exitcode exits with the expected code. +expect_exit() { + local scenario="$1" want="$2"; shift 2 + local out="$WORK/${scenario// /_}.out" + echo "" + echo "===== helm diff $* [$scenario] =====" + set +e + helm diff upgrade --detailed-exitcode "$@" > "$out" 2>&1 + local rc="$?" + set -e + strip_ansi < "$out" + echo "----- exit code: $rc -----" + if [ "$rc" -ne "$want" ]; then + echo "FAIL: scenario [$scenario] exited $rc, want $want" + FAIL=1 + else + echo "OK: scenario [$scenario] exited $want" + fi +} + +############################################################################### +# Variant A: null annotations, annotation added outside the chart +############################################################################### +chart "$WORK/a" 'apiVersion: v1 +kind: ConfigMap +metadata: + name: res-a + annotations: +data: + foo: bar +' +helm upgrade -i rel-a "$WORK/a" -n "$NS" >/dev/null +kubectl annotate configmap res-a -n "$NS" example.com/added-outside=yes --overwrite >/dev/null + +expect_exit "A three-way" 0 rel-a "$WORK/a" -n "$NS" --three-way-merge + +############################################################################### +# Variant B: null labels, label added outside the chart +############################################################################### +chart "$WORK/b" 'apiVersion: v1 +kind: ConfigMap +metadata: + name: res-b + labels: +data: + foo: bar +' +helm upgrade -i rel-b "$WORK/b" -n "$NS" >/dev/null +kubectl label configmap res-b -n "$NS" example.com/added-outside=yes --overwrite >/dev/null + +expect_exit "B three-way" 0 rel-b "$WORK/b" -n "$NS" --three-way-merge + +############################################################################### +# Variant C: variant A, merged locally +############################################################################### +expect_exit "C three-way client" 0 rel-a "$WORK/a" -n "$NS" --three-way-merge --three-way-merge-mode client + +############################################################################### +# Variant D: the chart sets an annotation that is edited outside the chart +############################################################################### +chart "$WORK/d" 'apiVersion: v1 +kind: ConfigMap +metadata: + name: res-d + annotations: + example.com/owned-by-chart: chart +data: + foo: bar +' +helm upgrade -i rel-d "$WORK/d" -n "$NS" >/dev/null +kubectl annotate configmap res-d -n "$NS" example.com/owned-by-chart=edited --overwrite >/dev/null + +expect_exit "D three-way drift" 2 rel-d "$WORK/d" -n "$NS" --three-way-merge + +############################################################################### +# The upgrade itself keeps what was added outside the chart; this is the +# behavior the diffs above must agree with. +############################################################################### +helm upgrade rel-a "$WORK/a" -n "$NS" >/dev/null +helm upgrade rel-b "$WORK/b" -n "$NS" >/dev/null +for check in "res-a {.metadata.annotations.example\.com/added-outside}" \ + "res-b {.metadata.labels.example\.com/added-outside}"; do + read -r name path <<< "$check" + if [ "$(kubectl get configmap "$name" -n "$NS" -o jsonpath="$path")" != "yes" ]; then + echo "FAIL: helm upgrade removed the metadata added outside the chart from $name; the premise of this test is wrong" + FAIL=1 + fi +done + +############################################################################### +finish