fix: use interruptionQueue Helm value for Karpenter >= v0.33.0 - #8844
fix: use interruptionQueue Helm value for Karpenter >= v0.33.0#8844warren830 wants to merge 1 commit into
Conversation
The Karpenter chart renamed its interruption queue Helm value when it flattened `settings`. Charts before v0.33.0 read `settings.aws.interruptionQueueName`; the flattened layout reads `settings.interruptionQueue`. eksctl built a single `settings` map and re-nested it under `aws` for older charts, so both version branches shared the `interruptionQueueName` spelling. On charts >= v0.33.0 Helm silently ignores that unknown key, so `INTERRUPTION_QUEUE` is never set on the Karpenter pod and spot interruption handling is disabled with no error surfaced to the user. Instances are terminated without Karpenter draining them first. Select the queue key per version branch instead of sharing one map: the `< 0.33.0` path keeps `settings.aws.interruptionQueueName` unchanged, and the `>= 0.33.0` path now sends `settings.interruptionQueue`. No API, flag or documentation change; `withSpotInterruptionQueue` is untouched. The existing `>= 0.33.0` unit expectation encoded the wrong key, so it is corrected to the key the chart actually reads. The two `settings.aws.interruptionQueueName` specs are left as-is and act as the regression guard for the legacy contract. Signed-off-by: warren <warren.chen830@gmail.com>
|
Hello warren830 👋 Thank you for opening a Pull Request in |
gustavodiaz7722
left a comment
There was a problem hiding this comment.
Note
AI-generated review. Produced by an AI agent (Kiro) at a maintainer's request. Findings were verified by execution against upstream charts, not by inspection alone.
Summary
The diagnosis is correct and well-evidenced, the fix is minimal, and the 0.33.0 pivot is exactly right. One change is required before merge: gate the queue name on withSpotInterruptionQueue.
Verification performed
Checks were run against 199c36195 (one commit on main at 99984adb6).
- Chart keys read at upstream tags v0.31.0 → v1.2.1:
settings.aws.interruptionQueueNamebefore flattening,settings.interruptionQueuefrom v0.32.0 onward. Root cause confirmed. - The real published OCI charts were rendered with the values
Installemits, which reproduces the bug and confirms the fix:- charts v0.33.0 and 1.2.1 + current
main→INTERRUPTION_QUEUEabsent - charts v0.33.0 and 1.2.1 + this PR →
INTERRUPTION_QUEUE=<cluster> - chart v0.31.0 → no such env var exists; the queue arrives via the
karpenter-global-settingsConfigMap, and the full 689-line rendered manifest is byte-identical betweenmainand this PR. The legacy path is unchanged at the Kubernetes-object level, not merely at the values-map level.
- charts v0.33.0 and 1.2.1 + current
- Reverting only
karpenter.gowhile keeping the new test reproduces the failure quoted in the description (3 passed / 1 failed), so the test is genuinely bug-first. - The version table was reproduced independently across 16 version strings, including the
0.33.0-rc.1pre-release boundary and unparseable input. No map aliasing and no state carryover across repeatedInstallcalls. go build -tags=release ./...,gofmt,go vetclean; integration tests compile; nogo.mod/go.sumdrift; 85 packages pass and the 2 failures are environmental.
Required change: gate on withSpotInterruptionQueue
Adjacent question 2 should be resolved here rather than deferred, because the affected configuration is the default one:
WithSpotInterruptionQueuehas no defaulting,IsEnabled(nil)is false, and the documented default is false.pkg/cfn/builder/karpenter.go:197creates the SQS queue and grants the controller rolesqs:ReceiveMessageonly when the flag is enabled.
The misspelled key was suppressing this second defect on >= 0.33.0. Correcting the key alone activates it: a default-configuration cluster on >= 0.33.0 begins pointing Karpenter at a queue that was never created and that it has no permission to poll, producing continuous SQS errors. Parity with the legacy branch is reached by propagating the defect rather than containing it.
Gating is purely additive. The name sent is already correct when the flag is enabled — CFN creates the queue as QueueName: <cluster name> — so no working value is ever suppressed.
Suggested fix
--- a/pkg/karpenter/karpenter.go
+++ b/pkg/karpenter/karpenter.go
@@ -87,12 +87,23 @@ func (k *Installer) Install(ctx context.Context, serviceAccountRoleARN string, i
// disables spot interruption handling without reporting an error.
version := k.ClusterConfig.Karpenter.Version
compareVersions, err := utils.CompareVersions(version, "0.33.0")
- if err == nil && compareVersions < 0 {
- settingsValues[interruptionQueueName] = k.ClusterConfig.Metadata.Name
+ legacyChart := err == nil && compareVersions < 0
+
+ // Only advertise the interruption queue when eksctl actually provisioned
+ // it. pkg/cfn/builder creates the SQS queue -- and grants the controller
+ // role sqs:ReceiveMessage on it -- only when withSpotInterruptionQueue is
+ // enabled, so sending the name unconditionally points Karpenter at a queue
+ // that does not exist and that it has no permission to poll.
+ queueEnabled := api.IsEnabled(k.ClusterConfig.Karpenter.WithSpotInterruptionQueue)
+
+ if legacyChart {
+ if queueEnabled {
+ settingsValues[interruptionQueueName] = k.ClusterConfig.Metadata.Name
+ }
settingsValues = map[string]interface{}{
aws: settingsValues,
}
- } else {
+ } else if queueEnabled {
settingsValues[interruptionQueue] = k.ClusterConfig.Metadata.Name
}Three existing specs assert the queue is delivered while the flag is unset, so the current suite encodes the ungated behaviour. Enabling the flag in BeforeEach preserves their intent, and two new specs cover the disabled case:
--- a/pkg/karpenter/karpenter_test.go
+++ b/pkg/karpenter/karpenter_test.go
@@ -30,6 +30,9 @@ var _ = Describe("Install", func() {
Version: "0.15.3",
CreateServiceAccount: api.Disabled(),
DefaultInstanceProfile: nil,
+ // The queue name is only sent when eksctl provisioned the
+ // queue, so the specs that assert on it enable it explicitly.
+ WithSpotInterruptionQueue: api.Enabled(),
}
@@ -101,6 +104,41 @@ var _ = Describe("Install", func() {
Expect(opts.Values[settings]).To(Equal(values[settings]))
})
+ When("withSpotInterruptionQueue is disabled", func() {
+
+ BeforeEach(func() {
+ cfg.Karpenter.WithSpotInterruptionQueue = api.Disabled()
+ })
+
+ // pkg/cfn/builder only creates the SQS queue, and only grants the
+ // controller role sqs:ReceiveMessage on it, when the queue is
+ // enabled. Advertising a queue name in either chart layout would
+ // point Karpenter at a queue that does not exist and that it
+ // cannot poll.
+ It("omits the queue name from the legacy settings.aws values", func() {
+ Expect(installerUnderTest.Install(context.Background(), "dummy", "dummy")).To(Succeed())
+ _, opts := fakeHelmInstaller.InstallChartArgsForCall(0)
+ Expect(opts.Values[settings]).To(Equal(map[string]interface{}{
+ aws: map[string]interface{}{
+ defaultInstanceProfile: "dummy",
+ clusterName: cfg.Metadata.Name,
+ clusterEndpoint: cfg.Status.Endpoint,
+ },
+ }))
+ })
+
+ It("omits the queue name from the flattened settings values", func() {
+ installerUnderTest.ClusterConfig.Karpenter.Version = "0.33.0"
+ Expect(installerUnderTest.Install(context.Background(), "dummy", "dummy")).To(Succeed())
+ _, opts := fakeHelmInstaller.InstallChartArgsForCall(0)
+ Expect(opts.Values[settings]).To(Equal(map[string]interface{}{
+ defaultInstanceProfile: "dummy",
+ clusterName: cfg.Metadata.Name,
+ clusterEndpoint: cfg.Status.Endpoint,
+ }))
+ })
+ })
+
When("install chart fails", func() {The gated build was rendered against the real charts. All four combinations behave correctly:
| chart | withSpotInterruptionQueue |
rendered result |
|---|---|---|
| v0.31.0 | true |
ConfigMap aws.interruptionQueueName: <cluster> |
| v0.31.0 | false |
ConfigMap key empty — the chart's own default, handling off |
| 1.2.1 | true |
INTERRUPTION_QUEUE=<cluster> |
| 1.2.1 | false |
no queue advertised |
6/6 specs pass; go build -tags=release ./..., gofmt and go vet clean.
Minor
- Adjacent question 1 can be withdrawn rather than filed. The
0.32.xcharts read{{- with or .Values.settings.aws.interruptionQueueName .Values.settings.interruptionQueue }}— confirmed present in v0.32.0, v0.32.5, v0.32.9 and v0.32.10, and absent from v0.33.0.0.32.xaccepts both spellings and was never broken, so the0.33.0pivot is exact rather than approximate. The description currently undersells the fix. - One test failure is misattributed.
pkg/iam/oidcfails becausecfssl/cfssljsonare not installed, not for want of a Docker credential helper; onlypkg/karpenter/providers/helmneeds registry auth. The "environmental and unrelated" conclusion holds — neither package depends onpkg/karpenter. - Optional: the flattened-path spec asserts only
opts.Values[settings], while the legacy specs assert the whole values map, leaving top-levelawsandserviceAccountunguarded on the>= 0.33.0path.
Escalating the queue-gating question instead of silently resolving it was the right call, and moving the key choice into the version branch removes the shared-map mutation that allowed one spelling to serve two incompatible chart layouts.
Description
The Karpenter chart renamed its interruption queue Helm value when it flattened
settings, andeksctlstill sends the old spelling to new charts.Symptom. With
withSpotInterruptionQueue: trueand a Karpenter version >=0.33.0, spot instances are terminated without Karpenter draining them first.INTERRUPTION_QUEUEis absent from the Karpenter pod's environment and nothing is logged — the misconfiguration is completely silent.Root cause.
pkg/karpenter/karpenter.gobuilt a singlesettingsmap and then re-nested it underawsfor older charts:Because both branches shared that one map, both sent
interruptionQueueName. That is correct only for the pre-flattening charts. The flattened chart readssettings.interruptionQueue, so the key eksctl sends is unknown to it and Helm drops it without complaint. The chart gates the env var on that exact value:charts/karpenter/templates/deployment.yaml, unchanged from v0.33.0 through v1.2.1Chart key by version, read from
charts/karpenter/values.yamlat upstream tags:settings.aws.interruptionQueueNamesettings.interruptionQueue(plus a deprecated emptyaws: {})settings.interruptionQueueThe flattened key has never been spelled
interruptionQueueName.Change. Select the queue key per version branch instead of sharing one map. The
< 0.33.0path keepssettings.aws.interruptionQueueNamebyte-for-byte as before; only the>= 0.33.0path changes, tosettings.interruptionQueue. Deliberately minimal: no API, flag, CloudFormation or documentation change, andwithSpotInterruptionQueueis untouched.Verified behaviour across every supported version (min is
v0.20.0persupportedKarpenterVersion)karpenter.version0.20.0,v0.20.0,0.28.0,0.31.0settings.aws{…}interruptionQueueName— unchanged0.32.0,0.32.9settings.aws{…}interruptionQueueName— unchanged (see note below)0.33.0,v0.33.0settings{…}interruptionQueue— fixed0.34.0,0.37.0,1.0.0,1.2.1,v1.2.1settings{…}interruptionQueue— fixed0.33.0-rc.1settings.aws{…}interruptionQueueName— pre-release sorts below0.33.0; pre-existing boundary semantics, unchangedsettings{…}interruptionQueue— same partition as before (rejected earlier by validation anyway)The
vprefix is handled identically on both sides of the boundary. Hoisting the map into a local also removes the pre-existing aliasing where the same map object was reachable at bothsettingsandsettings.aws; eachInstallnow builds a fresh map, verified to not carry state across calls.Two adjacent questions left out of this PR on purpose — both pre-existing, neither introduced here, happy to file separate issues if you'd like them tracked:
0.32.xboundary. Upstream flattenedsettingsat chart v0.32.0, but eksctl pivots at0.33.0, so0.32.xstill receives the deprecatedsettings.aws.*shape. Independent of the key rename.pkg/karpenternever consultsWithSpotInterruptionQueue(zero references), so the cluster name is passed as the queue name even when the user setwithSpotInterruptionQueue: false— whilepkg/cfn/builder/karpenter.goonly creates the SQS queue when it is enabled. On the legacy path this has always been delivered (v0.31.0 flattenssettingsinto thekarpenter-global-settingsConfigMap). On>= 0.33.0the typo was masking it, so this fix makes that pre-existing behaviour effective there: a user on>= 0.33.0withwithSpotInterruptionQueue: falsewill now getINTERRUPTION_QUEUEset to a queue that was never created, and Karpenter will log SQS polling errors. That is the same behaviour the< 0.33.0path already has, so this PR brings the two branches to parity rather than diverging them — but gating the value onWithSpotInterruptionQueueis a real behaviour change affecting both branches, so I have not made that call here. Flagging it explicitly as a maintainer decision.Checklist
README.md, or theuserdocsdirectory)area/nodegroup) and kind (e.g.kind/improvement)Documentation: not applicable. The bug is entirely in the Helm value name eksctl sends internally; no user-facing field, flag, or default changes, so
userdocs/src/usage/eksctl-karpenter.mdstays accurate as written.Manual testing: not done — I have no AWS account able to stand up an EKS cluster with spot capacity, so I could not observe a real interruption end to end. What I did verify, entirely offline with no AWS/EKS/cluster/Docker/credentials:
>= 0.33.0unit expectation encoded the wrong key. Correcting it to the key the chart reads fails against unmodified production and passes with this change — a genuine RED → GREEN, not a test written to fit the code:karpenter.goreproduces the failure (3 passed / 1 failed); restoring it returnsok.go test -tags=release ./pkg/karpenter/→ok../pkg/actions/karpenter/,./pkg/cfn/builder/,./pkg/apis/eksctl.io/v1alpha5/,./pkg/utils/→ allok../pkg/...→ 85 packages ok, 2 failing:pkg/karpenter/providers/helmandpkg/iam/oidc. Both fail identically on unmodifiedmainin this environment (they reachpublic.ecr.awsand need a Docker credential helper), so they are environmental and unrelated to this diff.gofmtclean,go vet -tags=release ./pkg/karpenter/clean,go build -tags=release ./...clean, integration tests compile (go test -tags integration -run=^$ ./integration/...).go mod tidyproduces nogo.mod/go.sumdrift; regeneratingassets/schema.jsonproduces a byte-identical file, socheck-gomodandcheck-schemaare unaffected.charts/karpenter/values.yamlandtemplates/deployment.yamlat upstream tags v0.31.0, v0.32.0, v0.32.9, v0.33.0, v0.34.0, v0.37.0, v1.0.0 and v1.2.1 — not from memory.Verified against
mainat99984adb6164ece593c0728523b7f497ea61d60e.BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯
The version branch now owns the key choice rather than mutating a shared map after the fact, which is what allowed one spelling to serve two incompatible chart layouts in the first place.
Fixes #7697