From ec4e15382c684f7f752f85b27b81f082f7a78228 Mon Sep 17 00:00:00 2001 From: Oreon Lothamer Date: Mon, 31 Aug 2026 11:23:18 -1000 Subject: [PATCH 1/2] feat: make --three-way-merge work without patch permissions --three-way-merge computed the merge patch locally but asked the API server to apply it as a dry-run, which needs the `patch` verb on every diffed resource. A read-only account got: cannot patch "x" with kind Deployment: ... is forbidden The patch is now applied locally when the server round-trip is not available, using the same strategic-merge (or JSON merge patch, for custom resources) logic the API server would use. Only `get` is required. A new --three-way-merge-mode flag (env HELM_DIFF_THREE_WAY_MERGE_MODE) selects between: auto server dry-run, falling back to the local merge on Forbidden or MethodNotAllowed. Any other error still aborts, so a genuinely bad patch is not masked. The default. server the previous behaviour; a missing permission is a hard error. client never sends the patch at all. Two post-processing steps keep the local result close to what the API server returns, since client-go does not ship the defaulting functions: * The merged object is round-tripped through its Go type, the way the API server does before it answers. That drops the empty values a manifest spells out but the type omits - `initialDelaySeconds: 0`, `hostNetwork: false`, `sysctls: []` - which would otherwise show up as additions the upgrade does not make. * A field is copied back from the live object when the old and the new release manifest agree about it. The patch replaces `retainKeys` structs and atomic lists as a whole, and a manifest that renders an unset value as an explicit `null` (a bare `replicas:`) reaches the patch as a change rather than a deletion; both wipe values the API server had defaulted in and re-defaults immediately after. Fields the two manifests disagree about are left deleted, because that is a change the chart really makes. manifest.Generate takes variadic options rather than a new parameter, so existing callers keep compiling. Co-Authored-By: Claude Opus 5 --- .github/copilot-instructions.md | 1 + README.md | 37 ++ cmd/upgrade.go | 24 +- cmd/upgrade_test.go | 46 +++ manifest/generate.go | 327 ++++++++++++++++-- manifest/generate_test.go | 583 ++++++++++++++++++++++++++++++++ 6 files changed, 996 insertions(+), 22 deletions(-) create mode 100644 manifest/generate_test.go diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index be06c72b..ac3f88c3 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -91,6 +91,7 @@ Always reference these instructions first and fallback to search or bash command - `HELM_BIN` - Path to helm binary (for direct testing) - `HELM_DIFF_USE_UPGRADE_DRY_RUN` - Use helm upgrade --dry-run instead of template - `HELM_DIFF_THREE_WAY_MERGE` - Enable three-way merge diffing +- `HELM_DIFF_THREE_WAY_MERGE_MODE` - Apply the three-way merge patch via the API server (`auto`/`server`) or locally (`client`) - `HELM_DIFF_NORMALIZE_MANIFESTS` - Normalize YAML before diffing - `HELM_DIFF_OUTPUT_CONTEXT` - Configure output context lines diff --git a/README.md b/README.md index 51a89a3c..26af9c10 100644 --- a/README.md +++ b/README.md @@ -188,6 +188,7 @@ Flags: -q, --suppress-secrets suppress secrets in the output --take-ownership if set, upgrade will ignore the check for helm annotations and take ownership of the existing resources --three-way-merge use three-way-merge to compute patch and generate diff output + --three-way-merge-mode string how --three-way-merge applies the computed patch. Must be "auto", "server" or "client". "server" dry-runs the patch against the API server, which requires the patch permission. "client" merges locally and needs read access only, at the cost of not applying server-side defaulting and mutating webhooks. "auto" uses the server and falls back to the client when patching is not permitted (default "auto") -f, --values valueFiles specify values in a YAML file (can specify multiple) (default []) --version string specify the exact chart version to use. If this is not specified, the latest version is used @@ -256,6 +257,35 @@ Notes: - helm-diff's own exit code is unaffected by the tool: `--detailed-exitcode` still returns `2` based on the changes helm-diff detected. - `--context`/`-C` is not applied; use the equivalent option of the external tool (for example `diff -U3`). +### Three-way merge + +`--three-way-merge` diffs against what is actually in the cluster rather than against the manifests of the last release, so changes made outside of Helm show up too. To do that helm-diff has to compute the object that the upgrade would produce: it reads the live object, builds a three-way merge patch from the old release manifest, the new release manifest and the live object, and then applies that patch. + +`--three-way-merge-mode` controls how the patch is applied: + +- `server` sends the patch to the API server as a dry-run (`PATCH ...?dryRun=All`). The API server fills in defaults and runs mutating webhooks, so the result is the most faithful preview of the upgrade — but the credentials need the `patch` permission on every diffed resource. +- `client` applies the patch locally, using the same strategic-merge (or JSON merge patch, for custom resources) logic the API server would use. Only `get` is required. The merged object is then round-tripped through its Go type, the way the API server does before it answers, and a field is copied back from the live object whenever the old and the new release manifest agree about it — without that, the defaults the API server re-applies after patching would show up as spurious removals. Validation and mutating webhooks are still not applied. +- `auto` (the default) tries `server` first and falls back to `client` per run when the API server rejects the dry-run with `Forbidden` or `MethodNotAllowed`, printing a note on stderr. Any other error still aborts the diff. + +Because the defaulting functions are not part of client-go, `client` mode cannot reproduce them exactly. What it can do is leave the live value alone: a field is only reported as gone when the two release manifests disagree about it. That matters more than it sounds, because a chart that leaves a value unset usually renders the field as an explicit `null` — a bare `replicas:` — and a `null` in a manifest reaches the patch as a change rather than a deletion, wiping a value the API server had defaulted in even when the chart did not change at all. + +The remaining known deviation from `server` mode is the case where the manifests really do disagree and the field is one the API server defaults: a chart that stops pinning `replicas: 3` is reported as removing the field, where `server` mode shows it changing from `3` to the defaulted `1`. The change is reported either way, but `client` mode cannot name the value that replaces it. + +So a read-only account is enough for a three-way merge diff out of the box. Set `--three-way-merge-mode=client` (or `HELM_DIFF_THREE_WAY_MERGE_MODE=client`) to skip the rejected dry-run request entirely, and `--three-way-merge-mode=server` to make a missing `patch` permission a hard error instead of silently degrading the diff. + +The minimal RBAC for the `client` mode is read access to the diffed kinds plus the release storage: + +```yaml +apiVersion: rbac.authorization.k8s.io/v1 +kind: Role +metadata: + name: helm-diff +rules: + - apiGroups: ["*"] + resources: ["*"] + verbs: ["get", "list"] +``` + ## Commands: ### local: @@ -349,6 +379,12 @@ Examples: # Read the flag usage below for more information on --three-way-merge. HELM_DIFF_THREE_WAY_MERGE=true helm diff upgrade my-release datadog/datadog + # Set HELM_DIFF_THREE_WAY_MERGE_MODE=client to compute the three-way merge + # locally, so that no permission to patch the cluster resources is needed. + # This is equivalent to specifying the --three-way-merge-mode flag. + # Read the flag usage below for more information on --three-way-merge-mode. + HELM_DIFF_THREE_WAY_MERGE_MODE=client helm diff upgrade my-release datadog/datadog + # Set HELM_DIFF_NORMALIZE_MANIFESTS=true to # normalize the yaml file content when using helm diff. # This is equivalent to specifying the --normalize-manifests flag. @@ -418,6 +454,7 @@ Flags: -q, --suppress-secrets suppress secrets in the output --take-ownership if set, upgrade will ignore the check for helm annotations and take ownership of the existing resources --three-way-merge use three-way-merge to compute patch and generate diff output + --three-way-merge-mode string how --three-way-merge applies the computed patch. Must be "auto", "server" or "client". "server" dry-runs the patch against the API server, which requires the patch permission. "client" merges locally and needs read access only, at the cost of not applying server-side defaulting and mutating webhooks. "auto" uses the server and falls back to the client when patching is not permitted (default "auto") -f, --values valueFiles specify values in a YAML file (can specify multiple) (default []) --version string specify the exact chart version to use. If this is not specified, the latest version is used diff --git a/cmd/upgrade.go b/cmd/upgrade.go index e47e1933..63d112b0 100644 --- a/cmd/upgrade.go +++ b/cmd/upgrade.go @@ -68,6 +68,7 @@ type diffCmd struct { normalizeManifests bool takeOwnership bool threeWayMerge bool + threeWayMergeMode string serverSide string extraAPIs []string kubeVersion string @@ -162,6 +163,12 @@ func newChartCommand() *cobra.Command { " # Read the flag usage below for more information on --three-way-merge.", " HELM_DIFF_THREE_WAY_MERGE=true helm diff upgrade my-release datadog/datadog", "", + " # Set HELM_DIFF_THREE_WAY_MERGE_MODE=client to compute the three-way merge", + " # locally, so that no permission to patch the cluster resources is needed.", + " # This is equivalent to specifying the --three-way-merge-mode flag.", + " # Read the flag usage below for more information on --three-way-merge-mode.", + " HELM_DIFF_THREE_WAY_MERGE_MODE=client helm diff upgrade my-release datadog/datadog", + "", " # Set HELM_DIFF_NORMALIZE_MANIFESTS=true to", " # normalize the yaml file content when using helm diff.", " # This is equivalent to specifying the --normalize-manifests flag.", @@ -187,6 +194,10 @@ func newChartCommand() *cobra.Command { return fmt.Errorf("flag %q must be %q, %q or %q, but got %q", "server-side", envTrue, envFalse, serverSideAuto, diff.serverSide) } + if !slices.Contains(manifest.ValidThreeWayMergeModes, diff.threeWayMergeMode) { + return fmt.Errorf("flag %q must be one of %q, but got %q", "three-way-merge-mode", manifest.ValidThreeWayMergeModes, diff.threeWayMergeMode) + } + if err := diff.validateRevision(cmd.Flags().Changed("revision")); err != nil { return err } @@ -206,6 +217,15 @@ func newChartCommand() *cobra.Command { } } + if !cmd.Flags().Changed("three-way-merge-mode") { + if mode := os.Getenv("HELM_DIFF_THREE_WAY_MERGE_MODE"); mode != "" { + if !slices.Contains(manifest.ValidThreeWayMergeModes, mode) { + return fmt.Errorf("env var %q must be one of %q, but got %q", "HELM_DIFF_THREE_WAY_MERGE_MODE", manifest.ValidThreeWayMergeModes, mode) + } + diff.threeWayMergeMode = mode + } + } + if !diff.normalizeManifests && !cmd.Flags().Changed("normalize-manifests") { enabled := os.Getenv("HELM_DIFF_NORMALIZE_MANIFESTS") == envTrue diff.normalizeManifests = enabled @@ -241,6 +261,7 @@ func newChartCommand() *cobra.Command { f.StringVar(&kubeconfig, "kubeconfig", "", "This flag is ignored, to allow passing of this top level flag to helm") addNamespaceFlags(f, &diff.namespaces) f.BoolVar(&diff.threeWayMerge, "three-way-merge", false, "use three-way-merge to compute patch and generate diff output") + f.StringVar(&diff.threeWayMergeMode, "three-way-merge-mode", string(manifest.ThreeWayMergeAuto), `how --three-way-merge applies the computed patch. Must be "auto", "server" or "client". "server" dry-runs the patch against the API server, which requires the patch permission. "client" merges locally and needs read access only, at the cost of not applying server-side defaulting and mutating webhooks. "auto" uses the server and falls back to the client when patching is not permitted`) f.StringVar(&diff.kubeContext, "kube-context", "", "name of the kubeconfig context to use") f.StringVar(&diff.chartVersion, "version", "", "specify the exact chart version to use. If this is not specified, the latest version is used") f.StringVar(&diff.chartRepo, "repo", "", "specify the chart repository url to locate the requested chart") @@ -344,7 +365,8 @@ func (d *diffCmd) runHelm3() error { } if d.threeWayMerge { - releaseManifest, installManifest, err = manifest.Generate(actionConfig, releaseManifest, installManifest) + releaseManifest, installManifest, err = manifest.Generate(actionConfig, releaseManifest, installManifest, + manifest.WithThreeWayMergeMode(manifest.ThreeWayMergeMode(d.threeWayMergeMode))) if err != nil { return fmt.Errorf("unable to generate manifests: %w", err) } diff --git a/cmd/upgrade_test.go b/cmd/upgrade_test.go index 900cc375..008a48aa 100644 --- a/cmd/upgrade_test.go +++ b/cmd/upgrade_test.go @@ -367,3 +367,49 @@ data: } }) } + +func TestThreeWayMergeModeFlag(t *testing.T) { + if f := newChartCommand().Flags().Lookup("three-way-merge-mode"); f == nil { + t.Fatal("expected flag --three-way-merge-mode to be registered") + } else if f.DefValue != "auto" { + t.Errorf("expected --three-way-merge-mode to default to auto, got %q", f.DefValue) + } + + cases := []struct { + name string + args []string + env string + expectErr string + }{ + {name: "server", args: []string{"--three-way-merge-mode", "server"}}, + {name: "client", args: []string{"--three-way-merge-mode", "client"}}, + {name: "auto", args: []string{"--three-way-merge-mode", "auto"}}, + {name: "invalid flag", args: []string{"--three-way-merge-mode", "local"}, expectErr: "three-way-merge-mode"}, + {name: "empty flag", args: []string{"--three-way-merge-mode", ""}, expectErr: "three-way-merge-mode"}, + {name: "env var", env: "client"}, + {name: "invalid env var", env: "local", expectErr: "HELM_DIFF_THREE_WAY_MERGE_MODE"}, + {name: "flag wins over invalid env var", args: []string{"--three-way-merge-mode", "client"}, env: "local"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Setenv("HELM_DIFF_THREE_WAY_MERGE_MODE", tc.env) + + chartDir := t.TempDir() + setupFakeHelm(t, "capture_args", "", chartDir+"/args", "") + + cmd := newChartCommand() + cmd.SetArgs(append([]string{"my-release", chartDir}, tc.args...)) + + err := cmd.Execute() + switch { + case tc.expectErr == "" && err != nil: + t.Fatalf("unexpected error: %v", err) + case tc.expectErr != "" && err == nil: + t.Fatalf("expected an error mentioning %q, got none", tc.expectErr) + case tc.expectErr != "" && !strings.Contains(err.Error(), tc.expectErr): + t.Fatalf("expected error mentioning %q, got %v", tc.expectErr, err) + } + }) + } +} diff --git a/manifest/generate.go b/manifest/generate.go index 23b8f6bd..803adc77 100644 --- a/manifest/generate.go +++ b/manifest/generate.go @@ -4,6 +4,8 @@ import ( "bytes" "encoding/json" "fmt" + "os" + "reflect" jsonpatch "github.com/evanphx/json-patch/v5" jsoniter "github.com/json-iterator/go" @@ -23,7 +25,51 @@ const ( Helm3TestHook = "test" ) -func Generate(actionConfig *action.Configuration, originalManifest, targetManifest []byte) ([]byte, []byte, error) { +// ThreeWayMergeMode selects how the three-way merge patch is turned into the +// object that the diff is computed against. +type ThreeWayMergeMode string + +const ( + // ThreeWayMergeAuto sends the patch to the API server as a dry-run and + // falls back to merging locally when the server refuses the request because + // the user has no permission to patch the resource. + ThreeWayMergeAuto ThreeWayMergeMode = "auto" + // ThreeWayMergeServer always sends the patch to the API server as a dry-run + // and fails when that is not permitted. + ThreeWayMergeServer ThreeWayMergeMode = "server" + // ThreeWayMergeClient always merges locally and never sends a patch to the + // API server, so that only read permissions are required. + ThreeWayMergeClient ThreeWayMergeMode = "client" +) + +// ValidThreeWayMergeModes lists every accepted ThreeWayMergeMode value. +var ValidThreeWayMergeModes = []string{ + string(ThreeWayMergeAuto), + string(ThreeWayMergeServer), + string(ThreeWayMergeClient), +} + +type generateOptions struct { + mergeMode ThreeWayMergeMode +} + +// GenerateOption customizes the behaviour of Generate. +type GenerateOption func(*generateOptions) + +// WithThreeWayMergeMode selects how the three-way merge patch is applied. +// It defaults to ThreeWayMergeAuto. +func WithThreeWayMergeMode(mode ThreeWayMergeMode) GenerateOption { + return func(o *generateOptions) { + o.mergeMode = mode + } +} + +func Generate(actionConfig *action.Configuration, originalManifest, targetManifest []byte, opts ...GenerateOption) ([]byte, []byte, error) { + options := generateOptions{mergeMode: ThreeWayMergeAuto} + for _, opt := range opts { + opt(&options) + } + var err error original, err := actionConfig.KubeClient.Build(bytes.NewBuffer(originalManifest), false) if err != nil { @@ -72,6 +118,19 @@ func Generate(actionConfig *action.Configuration, originalManifest, targetManife return nil }) + // The warning about the fallback to the local merge is only interesting + // once, no matter how many resources the release contains. + warned := false + warnClientSideMerge := func(cause error) { + if warned { + return + } + warned = true + fmt.Fprintf(os.Stderr, "Not allowed to dry-run the patch against the cluster (%v).\n"+ + "Falling back to computing the three-way merge locally. The diff may deviate from the\n"+ + "actual upgrade result because server-side defaulting and mutating webhooks are not applied.\n", cause) + } + err = target.Visit(func(info *resource.Info, err error) error { if err != nil { return err @@ -109,18 +168,18 @@ func Generate(actionConfig *action.Configuration, originalManifest, targetManife return fmt.Errorf("could not find %q", info.Name) } - patch, patchType, err := createPatch(originalInfo.Object, currentObj, info) + patch, err := createPatch(originalInfo.Object, currentObj, info) if err != nil { return err } - helper.ServerDryRun = true - targetObj, err := helper.Patch(info.Namespace, info.Name, patchType, patch, nil) + // `out` still holds the live object, which is what the patch applies to. + merged, err := applyPatch(helper, info, patch, out, options.mergeMode, warnClientSideMerge) if err != nil { - return fmt.Errorf("cannot patch %q with kind %s: %w", info.Name, kind, err) + return err } - out, _ = jsoniter.ConfigCompatibleWithStandardLibrary.Marshal(targetObj) - pruneObj, err = deleteStatusAndTidyMetadata(out) + + pruneObj, err = deleteStatusAndTidyMetadata(merged) if err != nil { return fmt.Errorf("prune current obj %q with kind %s: %w", info.Name, kind, err) } @@ -136,26 +195,238 @@ func Generate(actionConfig *action.Configuration, originalManifest, targetManife return releaseManifest, installManifest, err } -func createPatch(originalObj, currentObj runtime.Object, target *resource.Info) ([]byte, types.PatchType, error) { +// resourcePatch is the patch computed for a single resource, together with +// everything that is needed to apply it locally instead of on the API server. +type resourcePatch struct { + data []byte + patchType types.PatchType + // patchMeta describes how the individual fields of the object have to be + // merged. It is only set for strategic merge patches. + patchMeta strategicpatch.LookupPatchMeta + // versionedObject is the target object in its versioned type. It is nil for + // unstructured resources, which have no Go type to normalize against. + versionedObject runtime.Object + // originalData and modifiedData are the manifests of the old and the new + // release, the two inputs that tell which fields a chart actually asks for. + originalData []byte + modifiedData []byte +} + +// apply merges the patch into the live object without contacting the API +// server. Unlike the server-side dry-run this needs no permission to patch, but +// it also skips defaulting, validation and mutating webhooks, so the result is +// post-processed to stay as close to the server's answer as possible. +func (p *resourcePatch) apply(liveData []byte) ([]byte, error) { + var merged []byte + var err error + + switch p.patchType { + case types.MergePatchType: + merged, err = jsonpatch.MergePatch(liveData, p.data) + case types.StrategicMergePatchType: + merged, err = strategicpatch.StrategicMergePatchUsingLookupPatchMeta(liveData, p.data, p.patchMeta) + default: + return nil, fmt.Errorf("unsupported patch type %q", p.patchType) + } + if err != nil { + return nil, err + } + + if merged, err = p.normalize(merged); err != nil { + return nil, err + } + + return p.restoreServerPopulatedFields(merged, liveData) +} + +// normalize round-trips the merged object through its Go type, the way the API +// server does before it answers a patch request. That drops the empty values +// the manifests spell out explicitly but the type omits, such as +// `initialDelaySeconds: 0`, `hostNetwork: false` or `sysctls: []`, which would +// otherwise show up as additions that the upgrade does not actually make. +func (p *resourcePatch) normalize(merged []byte) ([]byte, error) { + if p.versionedObject == nil { + return merged, nil + } + + objType := reflect.TypeOf(p.versionedObject) + if objType.Kind() != reflect.Ptr { + return merged, nil + } + typed, ok := reflect.New(objType.Elem()).Interface().(runtime.Object) + if !ok { + return merged, nil + } + if err := json.Unmarshal(merged, typed); err != nil { + return nil, fmt.Errorf("decoding the merged object: %w", err) + } + out, err := json.Marshal(typed) + if err != nil { + return nil, fmt.Errorf("encoding the merged object: %w", err) + } + return out, nil +} + +// restoreServerPopulatedFields copies back the fields that only the API server +// knows how to fill in. +// +// The merged object loses defaulted values in two ways. A three-way merge patch +// replaces `retainKeys` structs and atomic lists as a whole, which drops what +// the API server had defaulted into them - the rolling update strategy of a +// Deployment, the `protocol: TCP` of a NetworkPolicy port, the `volumeMode` of a +// volume claim template. And a manifest that renders a field as an explicit +// `null`, the way a chart writes `replicas:` for a value it leaves unset, turns +// into a `null` in the patch: the key is in both manifests, so it counts as a +// change rather than a deletion and deletes the live value. Either way the API +// server defaults the field straight back, but client-go does not ship the +// defaulting functions, so the local merge has to recover the value differently. +// +// A field is copied back from the live object when the old and the new release +// manifest agree about it - neither mentions it, or both give it the same value, +// `null` included. Nothing then asked for the live value to go, so its +// disappearance is an artifact of the patch rather than a change to report. Once +// the two manifests disagree the deletion is honoured, because that is a change +// the chart really makes. +// +// Only fields missing from the merged object are restored, never values that it +// already carries, so drift between the cluster and the chart is still +// reported. +func (p *resourcePatch) restoreServerPopulatedFields(merged, liveData []byte) ([]byte, error) { + var mergedObj, liveObj, originalObj, modifiedObj interface{} + + for _, in := range []struct { + data []byte + out *interface{} + }{ + {merged, &mergedObj}, + {liveData, &liveObj}, + {p.originalData, &originalObj}, + {p.modifiedData, &modifiedObj}, + } { + if len(in.data) == 0 { + continue + } + if err := json.Unmarshal(in.data, in.out); err != nil { + return nil, fmt.Errorf("decoding the object to restore defaulted fields: %w", err) + } + } + + restored := restoreMissing(mergedObj, liveObj, originalObj, modifiedObj) + + out, err := json.Marshal(restored) + if err != nil { + return nil, fmt.Errorf("encoding the object with the restored defaulted fields: %w", err) + } + return out, nil +} + +// restoreMissing walks merged and live in parallel and copies over the parts of +// live that merged lost without either release manifest asking for it. It +// returns the updated merged value and never overwrites a value merged already +// has. +func restoreMissing(merged, live, original, modified interface{}) interface{} { + switch live := live.(type) { + case map[string]interface{}: + mergedMap, ok := merged.(map[string]interface{}) + if !ok { + return merged + } + originalMap, _ := original.(map[string]interface{}) + modifiedMap, _ := modified.(map[string]interface{}) + + for key, liveValue := range live { + mergedValue, inMerged := mergedMap[key] + if !inMerged { + // A key that is absent and a key that holds `null` both read as + // nil here, which is what makes an unset `replicas:` in the + // chart compare equal to the field the manifests never mention. + if reflect.DeepEqual(originalMap[key], modifiedMap[key]) { + mergedMap[key] = liveValue + } + continue + } + mergedMap[key] = restoreMissing(mergedValue, liveValue, originalMap[key], modifiedMap[key]) + } + return mergedMap + + case []interface{}: + mergedList, ok := merged.([]interface{}) + if !ok || len(mergedList) != len(live) { + return merged + } + // Elements are paired by position, which is only sound as long as the + // chart itself left the list alone. Once the old and the new manifest + // disagree about it, the positions may mean different things and the + // list is left as the patch produced it. + originalList, _ := original.([]interface{}) + modifiedList, _ := modified.([]interface{}) + if len(originalList) != len(mergedList) || !reflect.DeepEqual(originalList, modifiedList) { + return mergedList + } + + for i := range mergedList { + mergedList[i] = restoreMissing(mergedList[i], live[i], originalList[i], modifiedList[i]) + } + return mergedList + + default: + return merged + } +} + +// applyPatch computes the patched object, either by letting the API server +// dry-run the patch or by merging locally, depending on mode. warn is called at +// most once, when mode is ThreeWayMergeAuto and the API server refused the +// dry-run. +func applyPatch(helper *resource.Helper, info *resource.Info, patch *resourcePatch, liveData []byte, mode ThreeWayMergeMode, warn func(cause error)) ([]byte, error) { + kind := info.Mapping.GroupVersionKind.Kind + + if mode != ThreeWayMergeClient { + helper.ServerDryRun = true + targetObj, err := helper.Patch(info.Namespace, info.Name, patch.patchType, patch.data, nil) + switch { + case err == nil: + out, err := jsoniter.ConfigCompatibleWithStandardLibrary.Marshal(targetObj) + if err != nil { + return nil, fmt.Errorf("serializing patched %q with kind %s: %w", info.Name, kind, err) + } + return out, nil + case mode == ThreeWayMergeAuto && isPatchNotAllowed(err): + warn(err) + default: + return nil, fmt.Errorf("cannot patch %q with kind %s: %w", info.Name, kind, err) + } + } + + out, err := patch.apply(liveData) + if err != nil { + return nil, fmt.Errorf("cannot merge %q with kind %s: %w", info.Name, kind, err) + } + return out, nil +} + +// isPatchNotAllowed reports whether the API server rejected the patch because +// the caller is not allowed to perform it, rather than because the patch itself +// is bad. +func isPatchNotAllowed(err error) bool { + return apierrors.IsForbidden(err) || apierrors.IsMethodNotSupported(err) +} + +func createPatch(originalObj, currentObj runtime.Object, target *resource.Info) (*resourcePatch, error) { oldData, err := json.Marshal(originalObj) if err != nil { - return nil, types.StrategicMergePatchType, fmt.Errorf("serializing current configuration: %w", err) + return nil, fmt.Errorf("serializing current configuration: %w", err) } newData, err := json.Marshal(target.Object) if err != nil { - return nil, types.StrategicMergePatchType, fmt.Errorf("serializing target configuration: %w", err) + return nil, fmt.Errorf("serializing target configuration: %w", err) } // Even if currentObj is nil (because it was not found), it will marshal just fine currentData, err := json.Marshal(currentObj) if err != nil { - return nil, types.StrategicMergePatchType, fmt.Errorf("serializing live configuration: %w", err) + return nil, fmt.Errorf("serializing live configuration: %w", err) } - // kind := target.Mapping.GroupVersionKind.Kind - // if kind == "Deployment" { - // curr, _ := yaml.Marshal(currentObj) - // fmt.Println(string(curr)) - // } // Get a versioned object versionedObject := kube.AsVersioned(target) @@ -169,19 +440,33 @@ func createPatch(originalObj, currentObj runtime.Object, target *resource.Info) // On newer K8s versions, CRDs aren't unstructured but has this dedicated type _, isCRD := versionedObject.(*apiextv1.CustomResourceDefinition) + patch := &resourcePatch{originalData: oldData, modifiedData: newData} + if !isUnstructured { + patch.versionedObject = versionedObject + } + if isUnstructured || isCRD { // fall back to generic JSON merge patch - patch, err := jsonpatch.CreateMergePatch(oldData, newData) - return patch, types.MergePatchType, err + patch.data, err = jsonpatch.CreateMergePatch(oldData, newData) + if err != nil { + return nil, err + } + patch.patchType = types.MergePatchType + return patch, nil } patchMeta, err := strategicpatch.NewPatchMetaFromStruct(versionedObject) if err != nil { - return nil, types.StrategicMergePatchType, fmt.Errorf("unable to create patch metadata from object: %w", err) + return nil, fmt.Errorf("unable to create patch metadata from object: %w", err) } - patch, err := strategicpatch.CreateThreeWayMergePatch(oldData, newData, currentData, patchMeta, true) - return patch, types.StrategicMergePatchType, err + patch.data, err = strategicpatch.CreateThreeWayMergePatch(oldData, newData, currentData, patchMeta, true) + if err != nil { + return nil, err + } + patch.patchType = types.StrategicMergePatchType + patch.patchMeta = patchMeta + return patch, nil } func objectKey(r *resource.Info) string { diff --git a/manifest/generate_test.go b/manifest/generate_test.go new file mode 100644 index 00000000..4020b892 --- /dev/null +++ b/manifest/generate_test.go @@ -0,0 +1,583 @@ +package manifest + +import ( + "encoding/json" + "fmt" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/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/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" + "k8s.io/cli-runtime/pkg/resource" + "sigs.k8s.io/yaml" +) + +func infoFor(t *testing.T, obj runtime.Object, gvk schema.GroupVersionKind) *resource.Info { + t.Helper() + return &resource.Info{ + Object: obj, + Namespace: "default", + Name: "nginx", + Mapping: &meta.RESTMapping{GroupVersionKind: gvk}, + } +} + +func deployment(replicas int32, image string, labels map[string]string) *appsv1.Deployment { + return &appsv1.Deployment{ + TypeMeta: metav1.TypeMeta{APIVersion: "apps/v1", Kind: "Deployment"}, + ObjectMeta: metav1.ObjectMeta{Name: "nginx", Namespace: "default", Labels: labels}, + Spec: appsv1.DeploymentSpec{ + Replicas: &replicas, + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "nginx", Image: image}}, + }, + }, + }, + } +} + +// TestCreatePatchApplyLocally_Strategic asserts that applying the three-way +// merge patch locally yields the same object the API server would have returned +// from the dry-run patch: the change from the chart is applied, while the field +// only present in the cluster is preserved. +func TestCreatePatchApplyLocally_Strategic(t *testing.T) { + gvk := appsv1.SchemeGroupVersion.WithKind("Deployment") + + original := deployment(1, "nginx:1.0", nil) + target := deployment(2, "nginx:2.0", nil) + + // The live object carries a field nobody in the release manifests knows + // about, e.g. set by a mutating webhook or another controller. + live := deployment(1, "nginx:1.0", nil) + live.Spec.Template.Spec.Containers[0].Env = []corev1.EnvVar{{Name: "INJECTED", Value: "yes"}} + + patch, err := createPatch(original, live, infoFor(t, target, gvk)) + require.NoError(t, err) + require.Equal(t, types.StrategicMergePatchType, patch.patchType) + require.NotNil(t, patch.patchMeta) + + liveData, err := yaml.Marshal(live) + require.NoError(t, err) + liveJSON, err := yaml.YAMLToJSON(liveData) + require.NoError(t, err) + + merged, err := patch.apply(liveJSON) + require.NoError(t, err) + + var got appsv1.Deployment + require.NoError(t, yaml.Unmarshal(merged, &got)) + + assert.Equal(t, int32(2), *got.Spec.Replicas) + assert.Equal(t, "nginx:2.0", got.Spec.Template.Spec.Containers[0].Image) + assert.Equal(t, + []corev1.EnvVar{{Name: "INJECTED", Value: "yes"}}, + got.Spec.Template.Spec.Containers[0].Env, + "a field only present in the cluster must survive the local merge") +} + +// TestCreatePatchApplyLocally_StrategicRemoval asserts that a field dropped from +// the chart is removed by the local merge, which is what distinguishes the +// three-way merge from a plain two-way merge. +func TestCreatePatchApplyLocally_StrategicRemoval(t *testing.T) { + gvk := appsv1.SchemeGroupVersion.WithKind("Deployment") + + original := deployment(1, "nginx:1.0", map[string]string{"keep": "me", "drop": "me"}) + target := deployment(1, "nginx:1.0", map[string]string{"keep": "me"}) + live := deployment(1, "nginx:1.0", map[string]string{"keep": "me", "drop": "me"}) + + patch, err := createPatch(original, live, infoFor(t, target, gvk)) + require.NoError(t, err) + + liveJSON, err := yaml.Marshal(live) + require.NoError(t, err) + liveJSON, err = yaml.YAMLToJSON(liveJSON) + require.NoError(t, err) + + merged, err := patch.apply(liveJSON) + require.NoError(t, err) + + var got appsv1.Deployment + require.NoError(t, yaml.Unmarshal(merged, &got)) + + assert.Equal(t, map[string]string{"keep": "me"}, got.ObjectMeta.Labels) +} + +// TestCreatePatchApplyLocally_Unstructured covers custom resources, which use a +// plain JSON merge patch instead of a strategic merge patch. +func TestCreatePatchApplyLocally_Unstructured(t *testing.T) { + gvk := schema.GroupVersionKind{Group: "example.com", Version: "v1", Kind: "Widget"} + + newWidget := func(size string) *unstructured.Unstructured { + return &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": "example.com/v1", + "kind": "Widget", + "metadata": map[string]interface{}{"name": "nginx", "namespace": "default"}, + "spec": map[string]interface{}{"size": size}, + }} + } + + original := newWidget("small") + target := newWidget("large") + + live := newWidget("small") + require.NoError(t, unstructured.SetNestedField(live.Object, "set-by-the-cluster", "spec", "extra")) + + patch, err := createPatch(original, live, infoFor(t, target, gvk)) + require.NoError(t, err) + require.Equal(t, types.MergePatchType, patch.patchType) + + liveJSON, err := live.MarshalJSON() + require.NoError(t, err) + + merged, err := patch.apply(liveJSON) + require.NoError(t, err) + + var got unstructured.Unstructured + require.NoError(t, got.UnmarshalJSON(merged)) + + size, _, _ := unstructured.NestedString(got.Object, "spec", "size") + assert.Equal(t, "large", size) + extra, _, _ := unstructured.NestedString(got.Object, "spec", "extra") + assert.Equal(t, "set-by-the-cluster", extra, "a field only present in the cluster must survive the local merge") +} + +func TestIsPatchNotAllowed(t *testing.T) { + gr := schema.GroupResource{Group: "apps", Resource: "deployments"} + + assert.True(t, isPatchNotAllowed(apierrors.NewForbidden(gr, "nginx", assert.AnError))) + assert.True(t, isPatchNotAllowed(apierrors.NewMethodNotSupported(gr, "patch"))) + assert.False(t, isPatchNotAllowed(apierrors.NewNotFound(gr, "nginx"))) + assert.False(t, isPatchNotAllowed(apierrors.NewInternalError(assert.AnError))) + assert.False(t, isPatchNotAllowed(assert.AnError)) +} + +func TestResourcePatchApplyUnsupportedType(t *testing.T) { + p := &resourcePatch{data: []byte("{}"), patchType: types.JSONPatchType} + _, err := p.apply([]byte("{}")) + assert.ErrorContains(t, err, "unsupported patch type") +} + +// The cases below reproduce the differences that were reported between the +// client-side merge and the server-side dry-run. +// +// The manifests are decoded into unstructured objects because that is what +// helm's kube.Client.Build hands to Generate: they keep whatever the chart +// spelled out, including the zero values a typed object would omit. The live +// object is written the way the API server returns it, with the defaults filled +// in and the zero values gone. + +func fromYAML(t *testing.T, manifest string) *unstructured.Unstructured { + t.Helper() + obj := &unstructured.Unstructured{} + require.NoError(t, yaml.Unmarshal([]byte(manifest), &obj.Object)) + return obj +} + +// applyLocally is the client-side path of applyPatch: build the patch from the +// three inputs and merge it into the live object without the API server. +func applyLocally(t *testing.T, original, live, target string) map[string]interface{} { + t.Helper() + + targetObj := fromYAML(t, target) + liveObj := fromYAML(t, live) + gvk := targetObj.GroupVersionKind() + + patch, err := createPatch(fromYAML(t, original), liveObj, infoFor(t, targetObj, gvk)) + require.NoError(t, err) + + liveJSON, err := liveObj.MarshalJSON() + require.NoError(t, err) + + merged, err := patch.apply(liveJSON) + require.NoError(t, err) + + var got map[string]interface{} + require.NoError(t, json.Unmarshal(merged, &got)) + return got +} + +func nested(t *testing.T, obj map[string]interface{}, fields ...string) (interface{}, bool) { + t.Helper() + value, found, err := unstructured.NestedFieldNoCopy(obj, fields...) + require.NoError(t, err) + return value, found +} + +// The rolling update strategy is defaulted by the API server and pruned by the +// $retainKeys directive the patch carries for spec.strategy. +func TestLocalMerge_KeepsDefaultedRollingUpdate(t *testing.T) { + chart := ` +apiVersion: apps/v1 +kind: Deployment +metadata: {name: nginx, namespace: default} +spec: + strategy: {type: RollingUpdate} + selector: {matchLabels: {app: nginx}} + template: + metadata: {labels: {app: nginx}} + spec: + containers: [{name: nginx, image: "%s"}] +` + live := ` +apiVersion: apps/v1 +kind: Deployment +metadata: {name: nginx, namespace: default} +spec: + replicas: 1 + strategy: + type: RollingUpdate + rollingUpdate: {maxSurge: 25%, maxUnavailable: 25%} + selector: {matchLabels: {app: nginx}} + template: + metadata: {labels: {app: nginx}} + spec: + containers: [{name: nginx, image: "nginx:1.0"}] +` + got := applyLocally(t, fmt.Sprintf(chart, "nginx:1.0"), live, fmt.Sprintf(chart, "nginx:2.0")) + + rollingUpdate, found := nested(t, got, "spec", "strategy", "rollingUpdate") + require.True(t, found, "the defaulted rolling update strategy must not be dropped") + assert.Equal(t, map[string]interface{}{"maxSurge": "25%", "maxUnavailable": "25%"}, rollingUpdate) + + replicas, found := nested(t, got, "spec", "replicas") + require.True(t, found, "the defaulted replica count must not be dropped") + assert.EqualValues(t, 1, replicas) + + containers, _ := nested(t, got, "spec", "template", "spec", "containers") + assert.Equal(t, "nginx:2.0", containers.([]interface{})[0].(map[string]interface{})["image"], + "the actual change must still be applied") +} + +// Values the chart spells out but the Go type omits must not show up as +// additions: the API server drops them when it answers the patch. +func TestLocalMerge_DropsExplicitZeroValues(t *testing.T) { + chart := ` +apiVersion: apps/v1 +kind: StatefulSet +metadata: {name: meilisearch, namespace: default} +spec: + serviceName: meilisearch + selector: {matchLabels: {app: meilisearch}} + template: + metadata: {labels: {app: meilisearch}} + spec: + hostIPC: false + hostNetwork: false + securityContext: {supplementalGroups: [], sysctls: []} + containers: + - name: meilisearch + image: "%s" + livenessProbe: {initialDelaySeconds: 0, exec: {command: [ok]}} + readinessProbe: {initialDelaySeconds: 0, exec: {command: [ok]}} +` + live := ` +apiVersion: apps/v1 +kind: StatefulSet +metadata: {name: meilisearch, namespace: default} +spec: + serviceName: meilisearch + selector: {matchLabels: {app: meilisearch}} + template: + metadata: {labels: {app: meilisearch}} + spec: + securityContext: {} + containers: + - name: meilisearch + image: getmeili/meilisearch:v1.0 + livenessProbe: {exec: {command: [ok]}} + readinessProbe: {exec: {command: [ok]}} +` + got := applyLocally(t, fmt.Sprintf(chart, "getmeili/meilisearch:v1.0"), live, fmt.Sprintf(chart, "getmeili/meilisearch:v1.1")) + + podSpec, found := nested(t, got, "spec", "template", "spec") + require.True(t, found) + pod := podSpec.(map[string]interface{}) + + assert.NotContains(t, pod, "hostIPC", "hostIPC: false must be normalized away") + assert.NotContains(t, pod, "hostNetwork", "hostNetwork: false must be normalized away") + assert.Equal(t, map[string]interface{}{}, pod["securityContext"], + "supplementalGroups: [] and sysctls: [] must be normalized away") + + container := pod["containers"].([]interface{})[0].(map[string]interface{}) + assert.Equal(t, "getmeili/meilisearch:v1.1", container["image"], "the actual change must still be applied") + for _, probe := range []string{"livenessProbe", "readinessProbe"} { + assert.NotContains(t, container[probe], "initialDelaySeconds", + "initialDelaySeconds: 0 must be normalized away in the %s", probe) + } +} + +// NetworkPolicy ports are an atomic list, so the patch replaces the whole list +// and drops the protocol the API server defaulted in. +func TestLocalMerge_KeepsDefaultedFieldsInAtomicLists(t *testing.T) { + chart := ` +apiVersion: networking.k8s.io/v1 +kind: NetworkPolicy +metadata: {name: mongodb, namespace: default} +spec: + podSelector: {matchLabels: {app: "%s"}} + ingress: + - ports: [{port: 27017}] +` + live := ` +apiVersion: networking.k8s.io/v1 +kind: NetworkPolicy +metadata: {name: mongodb, namespace: default} +spec: + podSelector: {matchLabels: {app: mongo}} + policyTypes: [Ingress] + ingress: + - ports: [{port: 27017, protocol: TCP}] +` + got := applyLocally(t, fmt.Sprintf(chart, "mongo"), live, fmt.Sprintf(chart, "mongodb")) + + ingress, found := nested(t, got, "spec", "ingress") + require.True(t, found) + port := ingress.([]interface{})[0].(map[string]interface{})["ports"].([]interface{})[0].(map[string]interface{}) + assert.Equal(t, "TCP", port["protocol"], "the defaulted protocol must not be dropped") + assert.EqualValues(t, 27017, port["port"]) + + app, _ := nested(t, got, "spec", "podSelector", "matchLabels", "app") + assert.Equal(t, "mongodb", app, "the actual change must still be applied") +} + +// Volume claim templates are an atomic list too, and the API server populates +// them with a type, a status and a volume mode. +func TestLocalMerge_KeepsServerPopulatedVolumeClaimTemplates(t *testing.T) { + chart := ` +apiVersion: apps/v1 +kind: StatefulSet +metadata: {name: mongodb, namespace: default} +spec: + serviceName: mongodb + selector: {matchLabels: {app: mongodb}} + template: + metadata: {labels: {app: mongodb}} + spec: + containers: [{name: mongodb, image: "%s"}] + volumeClaimTemplates: + - metadata: {name: data} + spec: + accessModes: [ReadWriteOnce] + resources: {requests: {storage: 8Gi}} +` + live := ` +apiVersion: apps/v1 +kind: StatefulSet +metadata: {name: mongodb, namespace: default} +spec: + serviceName: mongodb + selector: {matchLabels: {app: mongodb}} + template: + metadata: {labels: {app: mongodb}} + spec: + containers: [{name: mongodb, image: "mongo:6.0"}] + volumeClaimTemplates: + - apiVersion: v1 + kind: PersistentVolumeClaim + metadata: {name: data} + spec: + accessModes: [ReadWriteOnce] + resources: {requests: {storage: 8Gi}} + volumeMode: Filesystem + status: {phase: Pending} +` + got := applyLocally(t, fmt.Sprintf(chart, "mongo:6.0"), live, fmt.Sprintf(chart, "mongo:7.0")) + + templates, found := nested(t, got, "spec", "volumeClaimTemplates") + require.True(t, found) + claim := templates.([]interface{})[0].(map[string]interface{}) + + assert.Equal(t, "v1", claim["apiVersion"], "the server-populated apiVersion must not be dropped") + assert.Equal(t, "PersistentVolumeClaim", claim["kind"], "the server-populated kind must not be dropped") + assert.Equal(t, map[string]interface{}{"phase": "Pending"}, claim["status"], "the status must not be dropped") + assert.Equal(t, "Filesystem", claim["spec"].(map[string]interface{})["volumeMode"], + "the defaulted volumeMode must not be dropped") +} + +// A field the chart itself stops setting is a real change and must stay +// visible, even where the API server would default it back. +func TestLocalMerge_ReportsFieldsTheChartRemoves(t *testing.T) { + deploy := ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx + namespace: default + labels: {%s} +spec: + selector: {matchLabels: {app: nginx}} + template: + metadata: {labels: {app: nginx}} + spec: + containers: [{name: nginx, image: "nginx:1.0"}] +` + withLabel := fmt.Sprintf(deploy, "drop: me") + got := applyLocally(t, withLabel, withLabel, fmt.Sprintf(deploy, "")) + + labels, found := nested(t, got, "metadata", "labels") + assert.False(t, found, "a label the chart no longer sets must be reported as removed, got %v", labels) +} + +// Drift must survive the restoration pass: a value the cluster and the chart +// disagree about is not a defaulted field. +func TestLocalMerge_KeepsReportingDrift(t *testing.T) { + chart := ` +apiVersion: apps/v1 +kind: Deployment +metadata: {name: nginx, namespace: default} +spec: + replicas: 1 + selector: {matchLabels: {app: nginx}} + template: + metadata: {labels: {app: nginx}} + spec: + containers: [{name: nginx, image: nginx:1.0}] +` + // Someone scaled and re-imaged the deployment by hand. + live := ` +apiVersion: apps/v1 +kind: Deployment +metadata: {name: nginx, namespace: default} +spec: + replicas: 5 + selector: {matchLabels: {app: nginx}} + template: + metadata: {labels: {app: nginx}} + spec: + containers: [{name: nginx, image: nginx:9.9}] +` + got := applyLocally(t, chart, live, chart) + + replicas, _ := nested(t, got, "spec", "replicas") + assert.EqualValues(t, 1, replicas, "drifted replicas must be reset to the chart value") + containers, _ := nested(t, got, "spec", "template", "spec", "containers") + assert.Equal(t, "nginx:1.0", containers.([]interface{})[0].(map[string]interface{})["image"], + "a drifted image must be reset to the chart value") +} + +// A chart that leaves a value unset renders the field as an explicit `null` +// (`replicas:` with nothing after it). The key is present in both manifests, so +// the patch carries it as a change rather than a deletion and wipes the value +// the API server had defaulted into the cluster - even though the chart itself +// did not change at all. +func TestLocalMerge_KeepsDefaultsUnderExplicitNulls(t *testing.T) { + chart := ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: librechat-librechat-rag-api + namespace: librechat + labels: {app.kubernetes.io/instance: librechat} +spec: + replicas: + selector: + matchLabels: {app.kubernetes.io/name: rag} + template: + metadata: + annotations: + labels: {app.kubernetes.io/name: rag} + spec: + securityContext: {} + containers: + - name: rag + image: "ghcr.io/danny-avila/librechat-rag-api-dev-lite:%s" + imagePullPolicy: IfNotPresent + ports: [{name: http, containerPort: 8000, protocol: TCP}] + livenessProbe: + null + readinessProbe: + null + resources: {} + volumes: +` + live := ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: librechat-librechat-rag-api + namespace: librechat + labels: {app.kubernetes.io/instance: librechat} +spec: + replicas: 1 + revisionHistoryLimit: 10 + progressDeadlineSeconds: 600 + selector: + matchLabels: {app.kubernetes.io/name: rag} + strategy: + type: RollingUpdate + rollingUpdate: {maxSurge: 25%, maxUnavailable: 25%} + template: + metadata: + labels: {app.kubernetes.io/name: rag} + spec: + securityContext: {} + dnsPolicy: ClusterFirst + restartPolicy: Always + containers: + - name: rag + image: "ghcr.io/danny-avila/librechat-rag-api-dev-lite:latest" + imagePullPolicy: IfNotPresent + ports: [{name: http, containerPort: 8000, protocol: TCP}] + resources: {} + terminationMessagePath: /dev/termination-log +` + // The chart is byte-for-byte the same on both sides. + unchanged := fmt.Sprintf(chart, "latest") + got := applyLocally(t, unchanged, live, unchanged) + + replicas, found := nested(t, got, "spec", "replicas") + require.True(t, found, "an unset `replicas:` must not wipe the replica count the API server defaulted in") + assert.EqualValues(t, 1, replicas) + + rollingUpdate, found := nested(t, got, "spec", "strategy", "rollingUpdate") + require.True(t, found, "the defaulted rolling update strategy must survive too") + assert.Equal(t, map[string]interface{}{"maxSurge": "25%", "maxUnavailable": "25%"}, rollingUpdate) + + for _, field := range []string{"revisionHistoryLimit", "progressDeadlineSeconds"} { + _, found = nested(t, got, "spec", field) + assert.True(t, found, "the defaulted %s must survive too", field) + } +} + +// The same explicit `null`, but this time the chart really does change what it +// asks for. The removal is then a change and has to stay visible. +func TestLocalMerge_ReportsExplicitNullThatTheChartIntroduces(t *testing.T) { + deploy := ` +apiVersion: apps/v1 +kind: Deployment +metadata: {name: rag, namespace: default} +spec: + replicas: %s + selector: {matchLabels: {app: rag}} + template: + metadata: {labels: {app: rag}} + spec: + containers: [{name: rag, image: rag:1.0}] +` + live := ` +apiVersion: apps/v1 +kind: Deployment +metadata: {name: rag, namespace: default} +spec: + replicas: 3 + selector: {matchLabels: {app: rag}} + template: + metadata: {labels: {app: rag}} + spec: + containers: [{name: rag, image: rag:1.0}] +` + // The chart used to pin three replicas and now leaves the field unset. + got := applyLocally(t, fmt.Sprintf(deploy, "3"), live, fmt.Sprintf(deploy, "")) + + _, found := nested(t, got, "spec", "replicas") + assert.False(t, found, "a replica count the chart stops pinning is a real change and must be reported") +} From e4cae5b806e0f369b677efbad4f5ad9911b71bb1 Mon Sep 17 00:00:00 2001 From: Oreon Lothamer Date: Mon, 31 Aug 2026 12:27:24 -1000 Subject: [PATCH 2/2] fix: treat empty collections and null as the same value MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A chart that renders an empty collection literally - `rules: []`, the branch grafana's Role and ClusterRole take when no sidecar is enabled - disagrees with the cluster about how to write "nothing". Kubernetes stores objects as protobuf, which cannot tell an empty repeated field from an absent one, so the API server answers with `rules: null`. The patch then carries `{"rules":[]}` even though the chart did not change, and the client-side merge reported: rules (rbac.authorization.k8s.io/v1/Role/grafana/grafana) ± type change from to list server mode shows nothing, because its answer goes back through the same storage round-trip. The local merge now keeps whichever spelling the cluster reports whenever the merged and the live value are both empty. Emptying a collection that actually had entries is still a change and is still reported. Co-Authored-By: Claude Opus 5 --- README.md | 2 +- manifest/generate.go | 24 +++++++++++++++++++++ manifest/generate_test.go | 44 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 69 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 26af9c10..2a723896 100644 --- a/README.md +++ b/README.md @@ -264,7 +264,7 @@ Notes: `--three-way-merge-mode` controls how the patch is applied: - `server` sends the patch to the API server as a dry-run (`PATCH ...?dryRun=All`). The API server fills in defaults and runs mutating webhooks, so the result is the most faithful preview of the upgrade — but the credentials need the `patch` permission on every diffed resource. -- `client` applies the patch locally, using the same strategic-merge (or JSON merge patch, for custom resources) logic the API server would use. Only `get` is required. The merged object is then round-tripped through its Go type, the way the API server does before it answers, and a field is copied back from the live object whenever the old and the new release manifest agree about it — without that, the defaults the API server re-applies after patching would show up as spurious removals. Validation and mutating webhooks are still not applied. +- `client` applies the patch locally, using the same strategic-merge (or JSON merge patch, for custom resources) logic the API server would use. Only `get` is required. The merged object is then round-tripped through its Go type, the way the API server does before it answers, and a field is copied back from the live object whenever the old and the new release manifest agree about it — without that, the defaults the API server re-applies after patching would show up as spurious removals. An empty list, an empty map and a `null` are also treated as the same value, because Kubernetes stores objects as protobuf and cannot tell them apart: a chart that writes `rules: []` gets `rules: null` back from the cluster. Validation and mutating webhooks are still not applied. - `auto` (the default) tries `server` first and falls back to `client` per run when the API server rejects the dry-run with `Forbidden` or `MethodNotAllowed`, printing a note on stderr. Any other error still aborts the diff. Because the defaulting functions are not part of client-go, `client` mode cannot reproduce them exactly. What it can do is leave the live value alone: a field is only reported as gone when the two release manifests disagree about it. That matters more than it sounds, because a chart that leaves a value unset usually renders the field as an explicit `null` — a bare `replicas:` — and a `null` in a manifest reaches the patch as a change rather than a deletion, wiping a value the API server had defaulted in even when the chart did not change at all. diff --git a/manifest/generate.go b/manifest/generate.go index 803adc77..4f08017c 100644 --- a/manifest/generate.go +++ b/manifest/generate.go @@ -320,6 +320,21 @@ func (p *resourcePatch) restoreServerPopulatedFields(merged, liveData []byte) ([ return out, nil } +// isEmpty reports whether v carries nothing - a null, an empty list or an empty +// map. The three are interchangeable in a stored Kubernetes object, so telling +// them apart in a diff only ever produces noise. +func isEmpty(v interface{}) bool { + switch v := v.(type) { + case nil: + return true + case []interface{}: + return len(v) == 0 + case map[string]interface{}: + return len(v) == 0 + } + return false +} + // restoreMissing walks merged and live in parallel and copies over the parts of // live that merged lost without either release manifest asking for it. It // returns the updated merged value and never overwrites a value merged already @@ -336,6 +351,15 @@ func restoreMissing(merged, live, original, modified interface{}) interface{} { for key, liveValue := range live { mergedValue, inMerged := mergedMap[key] + if inMerged && isEmpty(mergedValue) && isEmpty(liveValue) { + // Two empty values that differ only in how they are written are + // not a change: the API server stores objects as protobuf, + // which cannot tell an empty list from an absent one, so a + // chart's `rules: []` comes back from the cluster as + // `rules: null`. Keep whichever the cluster reports. + mergedMap[key] = liveValue + continue + } if !inMerged { // A key that is absent and a key that holds `null` both read as // nil here, which is what makes an unset `replicas:` in the diff --git a/manifest/generate_test.go b/manifest/generate_test.go index 4020b892..e4537f11 100644 --- a/manifest/generate_test.go +++ b/manifest/generate_test.go @@ -581,3 +581,47 @@ spec: _, found := nested(t, got, "spec", "replicas") assert.False(t, found, "a replica count the chart stops pinning is a real change and must be reported") } + +// A chart that renders an empty collection literally (`rules: []`, the branch +// grafana takes when no sidecar is enabled) disagrees with the cluster about how +// to write "nothing": the API server stores objects as protobuf, which cannot +// tell an empty list from an absent one, and answers with `rules: null`. The two +// are the same object and must not be reported as a change. +func TestLocalMerge_TreatsEmptyCollectionsAsEqual(t *testing.T) { + role := func(rules string) string { + return fmt.Sprintf(` +apiVersion: rbac.authorization.k8s.io/v1 +kind: Role +metadata: {name: grafana, namespace: grafana} +rules: %s +`, rules) + } + + t.Run("chart writes [], cluster answers null", func(t *testing.T) { + got := applyLocally(t, role("[]"), role("null"), role("[]")) + rules, _ := nested(t, got, "rules") + assert.Nil(t, rules, "an empty list must not be reported as a change against a null") + }) + + t.Run("chart writes null, cluster answers []", func(t *testing.T) { + got := applyLocally(t, role("null"), role("[]"), role("null")) + rules, _ := nested(t, got, "rules") + assert.Equal(t, []interface{}{}, rules, "a null must not be reported as a change against an empty list") + }) + + // Emptying a collection that actually had entries is a real change. + t.Run("chart empties a populated list", func(t *testing.T) { + populated := ` +apiVersion: rbac.authorization.k8s.io/v1 +kind: Role +metadata: {name: grafana, namespace: grafana} +rules: + - apiGroups: [""] + resources: [configmaps] + verbs: [get] +` + got := applyLocally(t, populated, populated, role("[]")) + rules, _ := nested(t, got, "rules") + assert.Empty(t, rules, "emptying a populated list is a real change and must be reported") + }) +}