feat: make --three-way-merge work without patch permissions - #1063
Open
oreonl wants to merge 2 commits into
Open
feat: make --three-way-merge work without patch permissions#1063oreonl wants to merge 2 commits into
oreonl wants to merge 2 commits into
Conversation
--three-way-merge computed the merge patch locally but asked the API
server to apply it as a dry-run, which needs the `patch` verb on every
diffed resource. A read-only account got:
cannot patch "x" with kind Deployment: ... is forbidden
The patch is now applied locally when the server round-trip is not
available, using the same strategic-merge (or JSON merge patch, for
custom resources) logic the API server would use. Only `get` is
required.
A new --three-way-merge-mode flag (env HELM_DIFF_THREE_WAY_MERGE_MODE)
selects between:
auto server dry-run, falling back to the local merge on Forbidden
or MethodNotAllowed. Any other error still aborts, so a
genuinely bad patch is not masked. The default.
server the previous behaviour; a missing permission is a hard error.
client never sends the patch at all.
Two post-processing steps keep the local result close to what the API
server returns, since client-go does not ship the defaulting functions:
* The merged object is round-tripped through its Go type, the way the
API server does before it answers. That drops the empty values a
manifest spells out but the type omits - `initialDelaySeconds: 0`,
`hostNetwork: false`, `sysctls: []` - which would otherwise show up
as additions the upgrade does not make.
* A field is copied back from the live object when the old and the new
release manifest agree about it. The patch replaces `retainKeys`
structs and atomic lists as a whole, and a manifest that renders an
unset value as an explicit `null` (a bare `replicas:`) reaches the
patch as a change rather than a deletion; both wipe values the API
server had defaulted in and re-defaults immediately after. Fields
the two manifests disagree about are left deleted, because that is a
change the chart really makes.
manifest.Generate takes variadic options rather than a new parameter, so
existing callers keep compiling.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A chart that renders an empty collection literally - `rules: []`, the
branch grafana's Role and ClusterRole take when no sidecar is enabled -
disagrees with the cluster about how to write "nothing". Kubernetes
stores objects as protobuf, which cannot tell an empty repeated field
from an absent one, so the API server answers with `rules: null`.
The patch then carries `{"rules":[]}` even though the chart did not
change, and the client-side merge reported:
rules (rbac.authorization.k8s.io/v1/Role/grafana/grafana)
± type change from <nil> to list
server mode shows nothing, because its answer goes back through the same
storage round-trip. The local merge now keeps whichever spelling the
cluster reports whenever the merged and the live value are both empty.
Emptying a collection that actually had entries is still a change and is
still reported.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
auto mode does not currently “fall back per run” (it keeps attempting forbidden PATCHes) and the docs/RBAC wording plus env-var handling need small corrections to match intended behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a new three-way-merge “mode” capability so helm diff upgrade --three-way-merge can work with read-only Kubernetes credentials by falling back (or opting in) to a client-side patch application that approximates the API server’s results.
Changes:
- Introduces
--three-way-merge-mode(auto/server/client) and envHELM_DIFF_THREE_WAY_MERGE_MODE, wiring it throughcmd/upgradeintomanifest.Generate(...)via variadic options. - Implements client-side patch application for three-way-merge, including normalization (round-trip through Go types) and a restoration pass for server-populated/defaulted fields.
- Adds focused unit tests for local merge behavior and CLI flag/env validation; updates README and Copilot instructions.
File summaries
| File | Description |
|---|---|
| README.md | Documents --three-way-merge-mode, behavior tradeoffs, and RBAC guidance. |
| manifest/generate.go | Adds merge-mode options and client-side patch application with post-processing to reduce diff noise. |
| manifest/generate_test.go | New tests covering strategic/merge patch local application and default-restoration behaviors. |
| cmd/upgrade.go | Adds flag/env plumbing and validation; passes merge-mode into manifest generation. |
| cmd/upgrade_test.go | Adds validation coverage for the new flag/env behavior. |
| .github/copilot-instructions.md | Documents the new environment variable. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+121
to
+132
| // 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) | ||
| } |
Comment on lines
+220
to
+227
| 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 | ||
| } | ||
| } |
|
|
||
| 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: |
Comment on lines
458
to
465
| // 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. | ||
| _, isUnstructured := versionedObject.(runtime.Unstructured) | ||
|
|
||
| // On newer K8s versions, CRDs aren't unstructured but has this dedicated type | ||
| _, isCRD := versionedObject.(*apiextv1.CustomResourceDefinition) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
--three-way-mergecurrently requires thepatchpermission on every diffed resource. It computes the merge patch locally but asks the API server to apply it as a dry-run:A read-only account gets
cannot patch "x" with kind Deployment: ... is forbidden, so the feature is unavailable to exactly the credentials a "show me what this upgrade would do" tool tends to run under.This applies the patch locally when the server round-trip is not available, using the same strategic-merge (or JSON merge patch, for custom resources) logic the API server would use. Only
getis required.Interface
A new
--three-way-merge-modeflag (envHELM_DIFF_THREE_WAY_MERGE_MODE):auto(default)Forbidden/MethodNotAllowed. Any other error still aborts, so a genuinely bad patch is not masked.serverpatchpermission is a hard error.clientautomeans a read-only account works out of the box; it only degrades when the accurate path is unavailable anyway, and says so on stderr.manifest.Generatetakes variadic options rather than a new parameter, so existing callers (helmfile vendors this package) keep compiling.Matching what the server returns
A naive local merge produces a diff full of noise, because the API server does more than apply the patch.
scheme.Scheme.Default()is a no-op in client-go — theSetDefaults_*functions live ink8s.io/kubernetesand are not importable — so the defaults cannot simply be recomputed. Two post-processing steps recover most of the gap:1. Round-trip through the Go type, the way the API server does before it answers. That drops the empty values a manifest spells out but the type omits —
initialDelaySeconds: 0,hostNetwork: false,sysctls: [],supplementalGroups: []— which otherwise appear as additions the upgrade does not make. (Helm'skube.Client.Buildreturns unstructured objects, so these survive from the chart YAML verbatim.)2. Copy a field back from the live object when the old and the new release manifest agree about it. Values disappear from the merged object two ways:
retainKeysstructs (spec.strategy) and atomic lists (ports,volumeClaimTemplates) as a whole, dropping what the server had defaulted into them;null— a barereplicas:, which is what a chart writes for a value it leaves unset — puts the key in both manifests, so it reaches the patch as a change rather than a deletion. That deletes a defaulted value even when the chart did not change at all.The server re-defaults both cases immediately after applying the patch. Locally, a field is restored only when nothing in either manifest asked for it to go. Where the two manifests disagree the deletion is honoured, because that is a change the chart really makes — restoring there would hide real removals (a dropped
nodeSelectorentry looks identical at the JSON level, and the server does not default that back). Values already present in the merged object are never overwritten, so drift between the cluster and the chart is still reported and still corrected.Known deviation from
servermodeWhen the manifests genuinely disagree about a field the API server defaults,
clientmode reports it as removed whereservermode shows it changing to the default — a chart that stops pinningreplicas: 3shows the field going away rather than3→1. The change is reported either way;clientmode just cannot name the value that replaces it. Mutating webhooks and validation are also not applied. All of this is documented in the new README section, along with the minimal RBAC role.Tests
manifest/generate_test.gois new. EightTestLocalMerge_*cases are built from real manifests (unstructured, askube.Client.Buildsupplies them; the live side written as the API server returns it) and cover each symptom above, plus:replicasand hand-edited image in the cluster — still reset to the chart values, i.e. drift detection intact.I checked the tests are not vacuous by reverting each fix independently; each fails without the step it covers.
cmd/upgrade_test.goadds flag/env validation cases.go test ./...,go vetand gofmt pass, and the README flag tables are regenerated withmake readme.Testing done
Built with
make install/helmand run against a real cluster with an account lackingpatch, on a release of ~15 resources (LibreChat and its subcharts). Before the two post-processing steps the client-side output differed from--three-way-merge-mode=serveron defaulted fields; the cases in the tests are taken from that output.🤖 Generated with Claude Code