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/.golangci.yaml b/.golangci.yaml index 70dd2b73..03ef9b49 100644 --- a/.golangci.yaml +++ b/.golangci.yaml @@ -57,8 +57,11 @@ linters: - github.com/spf13/pflag - golang.org/x/term - gopkg.in/yaml.v2 + - github.com/stretchr/testify/assert - github.com/stretchr/testify/require - helm.sh/helm/v4 + - k8s.io/api/apps/v1 + - k8s.io/api/authorization/v1 - k8s.io/api/core/v1 - k8s.io/apiextensions-apiserver - k8s.io/apimachinery diff --git a/README.md b/README.md index 51a89a3c..b6a139f1 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,64 @@ 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. + +Three known deviations from `server` mode remain. + +**A field the chart stops pinning is reported as removed** rather than as changing to its default. `server` mode shows `replicas: 3` becoming the defaulted `1`; `client` mode reports the field going away, because it cannot name the value that replaces it. The change is reported either way. + +**Drift can be hidden inside a `retainKeys` struct or an atomic list.** The restoration described above copies back every live-only field that the patch replaced wholesale, and locally there is no way to tell a value the API server defaulted from one somebody set by hand — both are simply fields neither manifest mentions. If a NetworkPolicy port carries a hand-added `endPort`, the upgrade removes it and `server` mode says so, while `client` mode restores it along with the defaulted `protocol: TCP` and reports nothing. This is the one case where `client` mode can be quieter than the truth; everywhere else it errs towards reporting a change that does not happen. Use `--three-way-merge-mode=server` where that matters. + +**Server defaults are dropped from an atomic-list entry the chart changes.** Changing a NetworkPolicy port from `27017` to `27018` also drops the defaulted `protocol: TCP` from the diff, because positions in a list the chart rewrote can no longer be matched up safely. + +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`, which is only read once the three-way merge is enabled) 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. + +`client` mode needs `get` on every kind the chart renders, plus `get` **and `list`** on the Secret or ConfigMap holding the release — without an explicit `--revision`, Helm lists the storage backend to find the newest one. The role below is the blunt version, read access to everything, which is convenient but broader than a diff requires: + +```yaml +apiVersion: rbac.authorization.k8s.io/v1 +kind: Role +metadata: + name: helm-diff +rules: + - apiGroups: ["*"] + resources: ["*"] + verbs: ["get", "list"] +``` + +For least privilege, list only the kinds the chart actually renders, for example: + +```yaml +apiVersion: rbac.authorization.k8s.io/v1 +kind: Role +metadata: + name: helm-diff +rules: + # Release storage: `list` is what lets Helm find the newest revision. + - apiGroups: [""] + resources: ["configmaps", "secrets"] + verbs: ["get", "list"] + # Everything else the chart renders needs `get` only. + - apiGroups: [""] + resources: ["services", "serviceaccounts"] + verbs: ["get"] + - apiGroups: ["apps"] + resources: ["deployments", "statefulsets", "daemonsets"] + verbs: ["get"] +``` + +A kind that is missing from the role makes the diff fail on that resource, so the set has to cover everything the chart renders — including the kinds it only renders under some values. + ## Commands: ### local: @@ -349,6 +408,13 @@ 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. + # It is only read once the three-way merge is on, hence the flag below. + # 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 --three-way-merge + # 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 +484,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..7e824937 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,13 @@ 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.", + " # It is only read once the three-way merge is on, hence the flag below.", + " # 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 --three-way-merge", + "", " # 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 +195,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 +218,19 @@ func newChartCommand() *cobra.Command { } } + // Only consulted when the run actually performs a three-way merge: + // the variable may well be set for a different invocation, and an + // unrelated `helm diff upgrade` should not fail over a value it is + // never going to use. --take-ownership turns the merge on as well. + if (diff.threeWayMerge || diff.takeOwnership) && !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 +266,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 +370,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..283e4597 100644 --- a/cmd/upgrade_test.go +++ b/cmd/upgrade_test.go @@ -367,3 +367,89 @@ 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"}, + // The env var is only consulted when the run performs a three-way merge, + // so on its own it can neither take effect nor fail the command. + {name: "env var without three-way-merge", env: "client"}, + {name: "invalid env var without three-way-merge", env: "local"}, + {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) + } + }) + } +} + +// The env var is rejected only where it is actually used, so that a value left +// over in the environment cannot fail an unrelated `helm diff upgrade`. +func TestThreeWayMergeModeEnvVarOnlyAppliesToThreeWayMerge(t *testing.T) { + cases := []struct { + name string + args []string + env string + expectErr bool + }{ + {name: "--three-way-merge", args: []string{"--three-way-merge"}, env: "local", expectErr: true}, + {name: "--take-ownership", args: []string{"--take-ownership"}, env: "local", expectErr: true}, + {name: "neither", env: "local", expectErr: false}, + {name: "valid value with --three-way-merge", args: []string{"--three-way-merge"}, env: "client", expectErr: false}, + } + + 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() + mentionsEnvVar := err != nil && strings.Contains(err.Error(), "HELM_DIFF_THREE_WAY_MERGE_MODE") + + // A run that gets past validation goes on to reach for the cluster, + // which is not available here, so only the env var complaint itself + // is meaningful - not whether the command as a whole succeeded. + if mentionsEnvVar != tc.expectErr { + t.Fatalf("expected the env var to be rejected=%v, got err=%v", tc.expectErr, err) + } + }) + } +} diff --git a/manifest/generate.go b/manifest/generate.go index 23b8f6bd..95425fcf 100644 --- a/manifest/generate.go +++ b/manifest/generate.go @@ -2,15 +2,21 @@ package manifest import ( "bytes" + "context" "encoding/json" "fmt" + "io" + "os" + "reflect" jsonpatch "github.com/evanphx/json-patch/v5" jsoniter "github.com/json-iterator/go" "helm.sh/helm/v4/pkg/action" "helm.sh/helm/v4/pkg/kube" + authorizationv1 "k8s.io/api/authorization/v1" apiextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/strategicpatch" @@ -23,7 +29,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 behavior 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 +122,12 @@ func Generate(actionConfig *action.Configuration, originalManifest, targetManife return nil }) + fallback := &clientSideFallback{ + mode: options.mergeMode, + warn: os.Stderr, + canPatch: patchPermissionCheck(actionConfig), + } + err = target.Visit(func(info *resource.Info, err error) error { if err != nil { return err @@ -109,18 +165,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, fallback.currentMode(), fallback.patchDenied) 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,52 +192,414 @@ func Generate(actionConfig *action.Configuration, originalManifest, targetManife return releaseManifest, installManifest, err } -func createPatch(originalObj, currentObj runtime.Object, target *resource.Info) ([]byte, types.PatchType, error) { +// clientSideFallback decides, for the whole run, whether a patch still goes to +// the API server. +// +// A refused dry-run says something about the credentials rather than about the +// resource, so once the server has turned one patch down the remaining +// resources are merged locally as well. Retrying each of them would only collect +// one denied request - and one audit log entry - per object in the release. +type clientSideFallback struct { + mode ThreeWayMergeMode + warn io.Writer + // canPatch reports whether the credentials may patch a resource at all. It + // is nil when the question cannot be put, and a refusal is then taken at + // face value. + canPatch func(*resource.Info) (bool, error) +} + +// currentMode is how the next resource should be merged. +func (f *clientSideFallback) currentMode() ThreeWayMergeMode { + return f.mode +} + +// patchDenied records that the API server refused to dry-run a patch. It reports +// whether the run may carry on with the local merge; false means the refusal was +// not about permissions, so the caller has to surface it instead. +func (f *clientSideFallback) patchDenied(info *resource.Info, cause error) bool { + if f.mode == ThreeWayMergeClient { + return true + } + + // A 403 need not come from RBAC. An admission webhook or a quota controller + // can turn down a request the credentials are entitled to make, and that + // refusal is precisely what the upgrade would run into as well - working + // around it locally would hide the outcome the diff exists to predict. Ask + // whether patching is permitted at all before reading a refusal as a + // permission problem. + // + // Only a 403 is ambiguous that way. A 405 says the API server takes no patch + // for this resource whoever is asking, so being entitled to send one changes + // nothing and the local merge is the only way left. + if apierrors.IsForbidden(cause) && f.canPatch != nil { + if allowed, err := f.canPatch(info); err == nil && allowed { + return false + } + } + + f.mode = ThreeWayMergeClient + if _, err := fmt.Fprintf(f.warn, "Not allowed to dry-run the patch against the cluster (%v).\n"+ + "Falling back to computing the three-way merge locally for the rest of this run. The diff\n"+ + "may deviate from the actual upgrade result because server-side defaulting and mutating\n"+ + "webhooks are not applied.\n", cause); err != nil { + _, _ = fmt.Fprintf(os.Stderr, "failed writing fallback warning: %v\n", err) + } + + return true +} + +// patchPermissionCheck asks the API server whether the credentials may patch a +// resource. It returns nil when the question cannot be put - helm hands over a +// *kube.Client in practice, but the interface does not promise one - and a +// refusal is then assumed to be about permissions, which is how the fallback +// behaved before the check existed. +func patchPermissionCheck(actionConfig *action.Configuration) func(*resource.Info) (bool, error) { + client, ok := actionConfig.KubeClient.(*kube.Client) + if !ok || client.Factory == nil { + return nil + } + + clientset, err := client.Factory.KubernetesClientSet() + if err != nil { + return nil + } + + return func(info *resource.Info) (bool, error) { + review := &authorizationv1.SelfSubjectAccessReview{ + Spec: authorizationv1.SelfSubjectAccessReviewSpec{ + ResourceAttributes: &authorizationv1.ResourceAttributes{ + Namespace: info.Namespace, + Name: info.Name, + Verb: "patch", + Group: info.Mapping.Resource.Group, + Resource: info.Mapping.Resource.Resource, + }, + }, + } + + result, err := clientset.AuthorizationV1().SelfSubjectAccessReviews(). + Create(context.Background(), review, metav1.CreateOptions{}) + if err != nil { + return false, err + } + + return result.Status.Allowed, nil + } +} + +// 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 + } + + // A JSON merge patch carries only what the chart changed between the two + // release manifests, so it never prunes a field the cluster populated and + // there is nothing to put back. Custom resources are also stored as JSON + // rather than protobuf, which keeps `null`, `[]` and `{}` apart, so the + // restoration pass would misread a chart changing one into another as noise + // and undo it. + if p.patchType == types.MergePatchType { + return merged, nil + } + + 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.Pointer { + 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 honored, 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, so a field the chart does specify keeps whatever the patch made of it. +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 { + // Only where neither manifest carries a value for the key: an + // absent key and a `null` both read as nil here, which is what + // lets an unset `replicas:` count as "the chart says nothing". + // Agreeing on an actual value is not enough - two manifests that + // both say `hostNetwork: false` are asking for false, and + // copying a drifted `true` back would hide the correction the + // upgrade makes. + if originalMap[key] == nil && modifiedMap[key] == nil { + 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, patchDenied func(*resource.Info, error) bool) ([]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): + if !patchDenied(info, err) { + return nil, fmt.Errorf("cannot patch %q with kind %s: %w", info.Name, kind, 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 original 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) - // Unstructured objects, such as CRDs, may not have an not registered error - // returned from ConvertToVersion. Anything that's unstructured should - // use the jsonpatch.CreateMergePatch. Strategic Merge Patch is not supported - // on objects like CRDs. + // Unstructured objects, such as CRDs, may not return a "not registered" + // error from ConvertToVersion. Anything that is unstructured should use + // jsonpatch.CreateMergePatch, because a strategic merge patch is not + // supported on objects like CRDs. _, isUnstructured := versionedObject.(runtime.Unstructured) - // On newer K8s versions, CRDs aren't unstructured but has this dedicated type + // On newer Kubernetes versions CRDs are not unstructured but have 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..e4fef573 --- /dev/null +++ b/manifest/generate_test.go @@ -0,0 +1,809 @@ +package manifest + +import ( + "bytes" + "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") + }) +} + +// Once the API server has refused one dry-run patch it will refuse the rest, so +// the whole run switches to the local merge instead of collecting a denied +// request, and an audit log entry, per resource. +func TestClientSideFallbackAppliesToTheWholeRun(t *testing.T) { + forbidden := apierrors.NewForbidden( + schema.GroupResource{Group: "apps", Resource: "deployments"}, "nginx", assert.AnError) + + info := infoFor(t, deployment(1, "nginx:1.0", nil), appsv1.SchemeGroupVersion.WithKind("Deployment")) + + var warnings bytes.Buffer + fallback := &clientSideFallback{mode: ThreeWayMergeAuto, warn: &warnings} + require.Equal(t, ThreeWayMergeAuto, fallback.currentMode()) + + assert.True(t, fallback.patchDenied(info, forbidden)) + assert.Equal(t, ThreeWayMergeClient, fallback.currentMode(), + "the rest of the run must not retry a patch the API server already refused") + + firstWarning := warnings.String() + assert.Contains(t, firstWarning, "Falling back to computing the three-way merge locally") + + assert.True(t, fallback.patchDenied(info, forbidden)) + assert.Equal(t, firstWarning, warnings.String(), "the fallback must be reported once per run") + assert.Equal(t, ThreeWayMergeClient, fallback.currentMode()) +} + +// The explicit modes are never overridden by the fallback. +func TestClientSideFallbackLeavesExplicitModesAlone(t *testing.T) { + var warnings bytes.Buffer + fallback := &clientSideFallback{mode: ThreeWayMergeClient, warn: &warnings} + + assert.True(t, fallback.patchDenied(infoFor(t, deployment(1, "nginx:1.0", nil), appsv1.SchemeGroupVersion.WithKind("Deployment")), assert.AnError)) + assert.Equal(t, ThreeWayMergeClient, fallback.currentMode()) + assert.Empty(t, warnings.String(), "a run that never asks the server has nothing to report") +} + +// Both manifests asking for the same value is not the same as neither asking for +// anything. normalize drops an explicit `hostNetwork: false` through omitempty, +// and restoring the live value there would undo the very correction the upgrade +// makes. +func TestLocalMerge_KeepsReportingDriftOnChartOwnedZeroValues(t *testing.T) { + pod := func(hostNetwork string) string { + return fmt.Sprintf(` +apiVersion: apps/v1 +kind: Deployment +metadata: {name: nginx, namespace: default} +spec: + selector: {matchLabels: {app: nginx}} + template: + metadata: {labels: {app: nginx}} + spec: + hostNetwork: %s + containers: [{name: nginx, image: nginx:1.0}] +`, hostNetwork) + } + + // The chart pins false on both sides; someone flipped the cluster to true. + got := applyLocally(t, pod("false"), pod("true"), pod("false")) + + value, found := nested(t, got, "spec", "template", "spec", "hostNetwork") + assert.False(t, found && value == true, + "a chart-owned value must not be restored from the cluster, got hostNetwork=%v", value) +} + +// A custom resource is stored as JSON, which keeps null, [] and {} apart, and a +// JSON merge patch already leaves untouched live fields alone. Neither the +// empty-value equivalence nor the restoration pass may run over one. +func TestLocalMerge_LeavesCustomResourcesToTheMergePatch(t *testing.T) { + widget := func(items string) string { + return fmt.Sprintf(` +apiVersion: example.com/v1 +kind: Widget +metadata: {name: w, namespace: default} +spec: + items: %s + size: large +`, items) + } + + t.Run("null to empty list is a change the chart asked for", func(t *testing.T) { + got := applyLocally(t, widget("null"), widget("null"), widget("[]")) + items, _ := nested(t, got, "spec", "items") + assert.Equal(t, []interface{}{}, items, "the chart changed null to [], which must survive") + }) + + t.Run("empty list to null is a change the chart asked for", func(t *testing.T) { + got := applyLocally(t, widget("[]"), widget("[]"), widget("null")) + items, found := nested(t, got, "spec", "items") + assert.Nil(t, items, "the chart changed [] to null, which must survive (found=%v)", found) + }) + + t.Run("fields the chart never mentions are left alone by the merge patch", func(t *testing.T) { + live := ` +apiVersion: example.com/v1 +kind: Widget +metadata: {name: w, namespace: default} +spec: + items: null + size: large + status: {observed: 3} +` + got := applyLocally(t, widget("null"), live, widget("null")) + observed, found := nested(t, got, "spec", "status", "observed") + require.True(t, found, "a JSON merge patch must not prune a field it never mentions") + assert.EqualValues(t, 3, observed) + }) +} + +// A Forbidden that RBAC did not produce - an admission webhook or a quota +// controller refusing an authorized request - is the upgrade's own outcome and +// must not be worked around locally. +func TestClientSideFallbackDistinguishesAdmissionFromPermissions(t *testing.T) { + info := infoFor(t, deployment(1, "nginx:1.0", nil), appsv1.SchemeGroupVersion.WithKind("Deployment")) + info.Mapping.Resource = appsv1.SchemeGroupVersion.WithResource("deployments") + + denial := apierrors.NewForbidden( + schema.GroupResource{Group: "apps", Resource: "deployments"}, "nginx", assert.AnError) + + t.Run("patching is allowed, so the refusal came from elsewhere", func(t *testing.T) { + var warnings bytes.Buffer + fallback := &clientSideFallback{ + mode: ThreeWayMergeAuto, + warn: &warnings, + canPatch: func(*resource.Info) (bool, error) { return true, nil }, + } + + assert.False(t, fallback.patchDenied(info, denial), "the caller must surface an admission refusal") + assert.Equal(t, ThreeWayMergeAuto, fallback.currentMode(), "the run must not degrade") + assert.Empty(t, warnings.String()) + }) + + t.Run("patching is not allowed, so it is a permission problem", func(t *testing.T) { + var warnings bytes.Buffer + fallback := &clientSideFallback{ + mode: ThreeWayMergeAuto, + warn: &warnings, + canPatch: func(*resource.Info) (bool, error) { return false, nil }, + } + + assert.True(t, fallback.patchDenied(info, denial)) + assert.Equal(t, ThreeWayMergeClient, fallback.currentMode()) + assert.Contains(t, warnings.String(), "Falling back") + }) + + t.Run("the check itself fails, so the refusal is taken at face value", func(t *testing.T) { + var warnings bytes.Buffer + fallback := &clientSideFallback{ + mode: ThreeWayMergeAuto, + warn: &warnings, + canPatch: func(*resource.Info) (bool, error) { return false, assert.AnError }, + } + + assert.True(t, fallback.patchDenied(info, denial)) + assert.Equal(t, ThreeWayMergeClient, fallback.currentMode()) + }) +} + +// A 405 says the API server takes no patch for this resource whoever is asking, +// so being entitled to send one changes nothing: the run still has to fall back. +// Only a 403 is ambiguous enough to be worth an authorization review. +func TestClientSideFallbackAlwaysFallsBackOnMethodNotAllowed(t *testing.T) { + info := infoFor(t, deployment(1, "nginx:1.0", nil), appsv1.SchemeGroupVersion.WithKind("Deployment")) + notSupported := apierrors.NewMethodNotSupported( + schema.GroupResource{Group: "apps", Resource: "deployments"}, "patch") + + reviewed := false + var warnings bytes.Buffer + fallback := &clientSideFallback{ + mode: ThreeWayMergeAuto, + warn: &warnings, + canPatch: func(*resource.Info) (bool, error) { + reviewed = true + return true, nil + }, + } + + assert.True(t, fallback.patchDenied(info, notSupported), + "a 405 must fall back even where the credentials may patch") + assert.Equal(t, ThreeWayMergeClient, fallback.currentMode()) + assert.False(t, reviewed, "a 405 is unambiguous, so it is not worth an authorization review") +}