Repository navigation
fix: Auto Mode + addon conflict resolution on the permanent v6 line (v6.0.3) - #75
Merged
Merged
Conversation
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
GuySaar8
added a commit
that referenced
this pull request
Sep 8, 2026
Review catch (baz, #75). The null-compute_config fix made enable_auto_mode = false mean two different things: "never had Auto Mode" (omit the block — required, EKS rejects a disable) and "turn it off" (send the disable). Only the first worked. Flipping enable_auto_mode true -> false on a cluster that HAS Auto Mode omitted compute_config, so AWS kept Auto Mode running, while the same flag removed the coexistence SG rules (auto_mode_cluster_from_node / auto_mode_node_from_cluster, both count-gated on it) and the addon nodeSelector/tolerations. Still-running Auto Mode nodes would lose their cross-SG path to the managed-node fleet — including pods reaching coredns. My previous comment told the operator to "set enabled = false here", i.e. hand-edit the module. That is not a mechanism. Replaced with disable_auto_mode (exposed as eks_disable_auto_mode at the root): keep enable_auto_mode = true and set this for one apply, which sends the explicit disable AND keeps the SG rules and pinning in place while AWS drains the nodes. Clear both once the nodes are gone. Only bayer and stsaasuat run with Auto Mode on, so only they could reach this path. validate passes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
GuySaar8
marked this pull request as draft
September 8, 2026 07:17
GuySaar8
force-pushed
the
GuySaar8/DND-875/v6-auto-mode-and-conflicts
branch
2 times, most recently
from
September 8, 2026 07:32
bb31f51 to
4799e09
Compare
Both fixes existed only on the v5.6.2-migration / v6.0.1-migration-4 tags, so
every env re-hit them on reaching the permanent line. waystar's Stage 2 apply
(comet-devops#2215) moved to v6.0.0 and failed with:
InvalidRequestException: Cannot modify EKS Auto Mode configuration.
Auto Mode is not enabled on this cluster.
1. compute_config = null when Auto Mode is off, not { enabled = false }. EKS
rejects an explicit disable on a cluster that never had Auto Mode, failing the
whole UpdateClusterConfig. Upstream guards with
`for_each = var.compute_config != null ? [...] : []`, so null omits the block.
Only bayer and stsaasuat set eks_enable_auto_mode = true — this affects the
other 10 of 13 envs.
2. resolve_conflicts_on_create/update = OVERWRITE on the cert-manager and
external-dns addons, so a brownfield cluster's native add-on adopts objects the
old eks_blueprints_addons Helm release owns instead of failing with
ConfigurationConflict. Forward-port of 4b6b30e, which only ever landed on
v5.6.2-migration.
3. disable_auto_mode (eks_disable_auto_mode at the root). Fix 1 alone made
enable_auto_mode = false mean two incompatible things: "never had Auto Mode"
(omit the block) and "turn it off" (send the disable). Only the first worked —
flipping the flag on a cluster that HAS Auto Mode left AWS running it while
removing the coexistence SG rules and addon pinning, stranding live Auto Mode
nodes with no path to the managed-node fleet. Keep enable_auto_mode = true and
set disable_auto_mode for one apply instead.
Neither 1 nor 2 is migration-specific: any env on the permanent line with Auto
Mode off hits the first, any brownfield cluster the second.
Based on v6.0.2. validate passes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
GuySaar8
force-pushed
the
GuySaar8/DND-875/v6-auto-mode-and-conflicts
branch
from
September 8, 2026 07:32
4799e09 to
73d6bfa
Compare
GuySaar8
changed the title
GuySaar8
marked this pull request as ready for review
September 11, 2026 15:19
Comment on lines
+252
to
+255
| compute_config = var.enable_auto_mode ? { | ||
| enabled = !var.disable_auto_mode | ||
| node_pools = var.disable_auto_mode ? [] : var.auto_mode_node_pools | ||
| } : null |
There was a problem hiding this comment.
Contradictory flags silently strand Auto Mode nodes
The eks_disable_auto_mode=true path reaches the ternary with enable_auto_mode=false and disable_auto_mode=true, which returns null instead of an explicit disable, so planning can remove Auto Mode protections while Auto Mode remains active — should we reject this contradictory combination or handle it explicitly before planning?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
modules/comet_eks/main.tf around lines 252-255, fix the `compute_config` logic and its
inputs so `disable_auto_mode` cannot be true when `enable_auto_mode` is false. Add
validation or a plan-time precondition that rejects this contradictory combination
before planning, rather than allowing the ternary to return null and disabling the Auto
Mode-dependent security protections. Preserve the documented requirement that disabling
Auto Mode requires it to be explicitly enabled.
Comment on lines
+338
to
+339
| resolve_conflicts_on_create = "OVERWRITE" | ||
| resolve_conflicts_on_update = "OVERWRITE" |
There was a problem hiding this comment.
Pre-existing association blocks addon migration
OVERWRITE does not resolve ownership conflicts for the external-dns pod_identity_associations, so EKS rejects addon creation when a brownfield association exists and leaves the transition incomplete — should we delete the existing association before recreating it through the addon operation?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`modules/comet_eks/main.tf` around lines 338-339, update the external-dns addon logic
and its `pod_identity_associations` configuration; `resolve_conflicts_on_create/update =
"OVERWRITE"` cannot resolve an already-existing EKS Pod Identity association. Add an
explicit, ordered brownfield migration that detects and deletes the pre-existing
external-dns association before creating the addon association, then lets the addon
recreate and own it, or fail fast with a clear remediation message when safe deletion
cannot be automated. Ensure the solution is idempotent and does not leave the addon
partially configured.
jms200
approved these changes
Sep 11, 2026
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
2 participants
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.
User description
Two v6 bugs plus a follow-up. Based on
v6.0.2; ships asv6.0.3.Draft — testing
v6.0.3-rc.1on waystar (comet-devops#2215) first. Marking ready once that apply is clean.Why
Both fixes existed only on the migration tags, so every env re-hit them on reaching the permanent line. waystar Stage 1 applied fine against
v6.0.1-migration-4; Stage 2 moved tov6.0.0and failed:1.
compute_config = nullwhen Auto Mode is offAn explicit
{ enabled = false }on a cluster that never had Auto Mode is not a no-op — EKS rejects it and fails the wholeUpdateClusterConfig. Upstream guards withfor_each = var.compute_config != null ? [...] : [], sonullomits the block.Only bayer and stsaasuat set
eks_enable_auto_mode = true; the other 10 of 13 envs hit this.2.
resolve_conflicts = OVERWRITEon cert-manager / external-dnsLets a brownfield cluster's native add-on adopt objects the old
eks_blueprints_addonsHelm release owns, instead of failing onConfigurationConflict. Forward-port of4b6b30e, which only landed onv5.6.2-migration. Running on bayer since August — not new behaviour, just missing from the permanent line.3.
disable_auto_mode— review follow-upFix 1 alone made
enable_auto_mode = falsemean two incompatible things: "never had Auto Mode" (omit the block) and "turn it off" (send the disable). Only the first worked. Flipping the flag on a cluster that has Auto Mode left AWS running it while removing the coexistence SG rules and addon pinning — stranding live Auto Mode nodes with no path to the managed-node fleet, including coredns.Keep
enable_auto_mode = true, seteks_disable_auto_mode = truefor one apply, clear both once the nodes drain. Chose a variable over state-awareness: the module cannot read live cluster state at plan time without a data source on the cluster it also manages.Untested against a live Auto Mode cluster — only bayer and stsaasuat qualify, and it defaults
falseso it is inert everywhere today.Verified
validatepasses,fmt -checkclean. Diff ismodules/comet_eks/{main,variables}.tf+ root{main,variables}.tf— noremoved.tfor kubernetes/helm requirements leaked from the migration branch.Rollout
v6.0.3-rc.1← herev6.0.3v6.0.3v6.0.0and will hit the same Auto Mode failure. Untouched for now.Follow-up
OVERWRITEis a migration affordance living permanently in the module. Once the fleet is on v6 it is dead weight and should be removed — tracked separately.🤖 Generated with Claude Code
Generated description
Below is a concise technical summary of the changes proposed in this PR:
Prevent invalid EKS Auto Mode updates by omitting configuration for clusters that have never enabled it, while adding a staged disable flow for existing Auto Mode clusters. Enable native
corednsandexternal-dnsadd-ons to adopt legacy Helm-managed resources through overwrite conflict resolution, expose the lifecycle controls through the root module, and ignore local Markdown files.corednsandexternal-dnsadd-ons to overwrite conflicting objects left by the former Helm-based implementation, improving brownfield migration and add-on adoption behavior.Modified files (1)
Latest Contributors(2)
Modified files (1)
Latest Contributors(2)
compute_configfor clusters that never enabled Auto Mode and introducingdisable_auto_modeso existing Auto Mode nodes can drain safely before the configuration is removed.Modified files (4)
Latest Contributors(2)