fix: don't render a null pod template annotations key in the chart - #9460
Open
oreonl wants to merge 1 commit into
Open
fix: don't render a null pod template annotations key in the chart#9460oreonl wants to merge 1 commit into
oreonl wants to merge 1 commit into
Conversation
The pod template's `annotations:` key was emitted unconditionally with its
only content behind `{{- with .Values.podAnnotations }}`. Since
`podAnnotations` defaults to `{}`, the chart rendered a bare mapping key,
i.e. `annotations: null`.
That is not equivalent to omitting the key. `helm upgrade` computes a
three-way strategic merge patch, and a present-but-null key hits the
type-mismatch branch of `diffMaps` rather than the deletion path that
`IgnoreDeletions` protects. The resulting patch carries
`spec.template.metadata.annotations: null`, which deletes the whole map --
destroying any annotation written by another actor (e.g. `kubectl rollout
restart`) and showing up as permanent phantom drift in the meantime.
The key was correctly guarded through v0.23.0. aws#3345 moved it out of the
guard to make room for an unconditional `checksum/settings`, which was fine
while the map always had a child; aws#5159 later dropped that checksum and left
the bare key behind. This restores the guard, matching how the chart already
handles `additionalAnnotations` and the adjacent `podLabels`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Fixes #N/A
Description
The pod template's
annotations:key is emitted unconditionally, with its onlycontent behind
{{- with .Values.podAnnotations }}. SincepodAnnotationsdefaults to
{}, the chart renders a mapping key with no children:annotations: nullis not equivalent to omitting the key, and the difference isload-bearing for anything that computes a strategic merge patch from the
rendered manifest.
helm upgradebuilds its patch withstrategicpatch.CreateThreeWayMergePatch(old_release, rendered, live).That function protects fields written by other actors by only emitting deletions
found between the old release manifest and the rendered manifest — live-only
fields fall into the deletion path and are skipped via
IgnoreDeletions. But akey that is present and null in the rendered manifest never reaches that
path. It hits the type-mismatch branch in
diffMaps,which is gated only on
IgnoreChangesAndAdditions:mapvsnilare different types, so the computed patch containsspec.template.metadata.annotations: null, and in a strategic merge patch a nullvalue deletes the entire map.
The practical effect is that every annotation any other actor has put on
karpenter's pod template is destroyed on upgrade, and shows up as permanent
phantom drift in the meantime. Easiest reproduction is
kubectl rollout restart,which sets
kubectl.kubernetes.io/restartedAt:A server-side dry run confirms the API server drops the whole map. The same shows
up as a never-reconciling diff in Argo CD and helmfile. Charts that guard the key
are unaffected: the annotation is preserved and no drift is reported.
Worse, the deletion is invisible in advance unless the diff consults the cluster.
helm diff upgradewithout--three-way-mergecompares the stored releasemanifest against the newly rendered one; both contain
annotations: null, so itreports no changes — and the upgrade then rolls the Deployment anyway, because
the patch is only computed once the live object is the third input. A chart that
guards the key does not have this discrepancy: there, "no changes" is accurate,
since a live-only annotation is classified as a deletion and skipped. So the
rendered
nullturns an unrequested restart of the karpenter controller into onethat no pre-flight diff will warn about.
This is not limited to Helm 3. Helm 4 defaults new installs to server-side
apply, but
helm upgradedefaults--server-sidetoauto, andautoisreleaseApplyMethod == "ssa"— it preserves whatever the release already used rather than migrating it. A
release created before Helm 4 has an empty
ApplyMethod, which istreated as client-side,
so its first Helm 4 upgrade resolves to client-side and records
"csa", andevery upgrade after that reads it back and stays there. Nothing migrates a
release to server-side apply short of an explicit
--server-side=true.Server-side apply resolves annotation conflicts by field ownership instead and is
not affected the same way, but the rendered
nullis wrong regardless.Why the key is currently outside the guard
It was correct until the checksum it was sharing a block with went away:
karpenter-global-settingsspacing causing changed checksum #3345 (73a1a35a, v0.24.0) movedannotations:out of the guard to make roomfor an unconditional
checksum/settings. Correct at the time — the map alwayshad exactly one child and could never render empty.
7c3a7aa7, v0.33.0) dropped alpha settings and with them thechecksum/settingsline, leaving the bare key behind. Nothing in that PR wasabout annotations; this was collateral.
So the chart has rendered
annotations: nullfrom v0.33.0 through v1.14.0 andmain.Change
Restores the guard, which is also how this chart already handles the
Deployment's own object-level annotations
(
{{- with .Values.additionalAnnotations }}) and the adjacentpodLabels:No values changed, so no
values.yamlor generatedREADME.mdupdate is needed.How was this change tested?
helm template charts/karpenter, inspecting.spec.template.metadata:annotationskey is now absent, where before it renderedas an explicit null.
--set podAnnotations.foo=bar— rendersannotations: {foo: bar}, unchangedfrom before.
--set-json 'podAnnotations={}'— key absent, as with the default.End to end on a live cluster:
kubectl rollout restart deployment/karpenter,then
helm diff upgrade --three-way-merge. Before the change it reports thewhole annotations map being removed; after it reports no diff, and the
restartedAtannotation survives a subsequenthelm upgrade.Does this change impact docs?
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
🤖 Generated with Claude Code