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..2a723896 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. 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. + +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..4f08017c 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,262 @@ 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 +} + +// 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 +// 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 && 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 + // 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 +464,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..e4537f11 --- /dev/null +++ b/manifest/generate_test.go @@ -0,0 +1,627 @@ +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") +} + +// 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") + }) +}