Repository navigation
Conversation
…irements)
Basis for the v6.0.1-migration tag — the one-apply stepping stone the remaining
11 brownfield envs need to reach v6.0.0.
v5.6.2-migration cannot be reused. It predates DND-875, so it lacks
rds_auto_minor_version_upgrade (which porsche, si, waystar and zoox all pass)
plus rds_parameter_group_family, rds_use_proxy_endpoint and
rds_proxy_ack_no_iam_auth. Those envs fail on an unsupported argument before the
removed{} blocks ever run.
Cut from v6.0.0 rather than v5.7.0 — both carry the full DND-875 surface, but
starting at v6 means Stage 1 lands on the final surface and Stage 2 is only
"drop the providers map", with no second behaviour change (the DND-1522 toggle
removal and redis_vpn re-index happen once, in Stage 1).
- modules/comet_eks/removed.tf: 13 removed{} blocks, all destroy = false.
Every address verified present on both v2.1.2 and v1.20.11.
6 shared: blueprints{alb,cert_manager,external_dns},
storage_class.gp3, namespace.monitoring, secret.monitoring
1 zoox: blueprints.aws_cloudwatch_metrics
6 v1.20.x: helm_release{karpenter_stsaas,external_secrets,
external_secrets_crds}, storage_class.comet_generic,
annotations{admin,app}_ns_node_selector
- kubernetes + helm requirements restored in both versions.tf files
(requirements only — no provider config blocks; the wrapper passes its own).
- MIGRATION.md: restored and rewritten for the v6 sequence.
Coverage checked against real state: waystar 6/6, zoox 5/5, si 6 of 11,
porsche 4 of 11. The remainder on si and porsche sit at the STATE ROOT
(agentro_extras, agentro_view, agentro_portforward, and on porsche also
namespace/secret.monitoring) — not module-addressed, so they are removed{} in
the wrapper. Documented in MIGRATION.md and in the file header.
terraform validate passes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
| removed { | ||
| from = module.eks_blueprints_addons.module.aws_load_balancer_controller | ||
| lifecycle { | ||
| destroy = false | ||
| } |
There was a problem hiding this comment.
Advertised Terraform floor rejects migration syntax
The migration tag adds removed blocks while the root and child versions.tf files still declare required_version = ">= 1.5.7", so Terraform 1.5.7–1.6.x consumers fail during configuration parsing before state cleanup runs — should we raise the constraint to >= 1.7 and document that floor for wrappers, as the official removed block documentation and v1.7.0 release notes require?
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/removed.tf around lines 52-56, the migration module introduces
Terraform `removed` blocks that require Terraform 1.7+, but the module still advertises
support for 1.5.7–1.6.x. Update the relevant root and child `versions.tf` constraints
to require Terraform >=1.7, and document this minimum version for wrapper consumers.
Verify the migration configuration and version checks consistently enforce the new
floor.
| removed { | ||
| from = module.eks_blueprints_addons.module.cert_manager | ||
| lifecycle { | ||
| destroy = false | ||
| } | ||
| } | ||
|
|
||
| removed { | ||
| from = module.eks_blueprints_addons.module.external_dns | ||
| lifecycle { | ||
| destroy = false | ||
| } |
There was a problem hiding this comment.
Native add-on cutover fails on retained Helm objects
The cert_manager and external_dns removed blocks leave their Helm-managed objects live with destroy = false, so matching aws_eks_addon resources fail in terraform-aws-eks/aws v21.24 because resolve_conflicts_on_create defaults to "NONE" instead of adopting them. Could we provide an executable pre-cutover/adoption sequence or configure and test explicit conflict handling before removing the old state?
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/removed.tf` around lines 59-70, fix the `cert_manager` and
`external_dns` migration logic so removing the Helm submodule state does not leave
conflicting live objects before v6 creates the corresponding `aws_eks_addon` resources.
Implement a real pre-cutover/adoption sequence in the migration workflow, or update the
related add-on configuration to use an explicitly supported conflict strategy such as
overwrite/preserve and verify it against the pinned provider and EKS behavior. Do not
rely solely on optional `terraform state rm` or import instructions, since they cannot
establish an EKS managed add-on from an existing Helm release.
There was a problem hiding this comment.
Commit 65220e6 addressed this comment by configuring OVERWRITE conflict handling for the native add-ons on creation and update, allowing brownfield adoption instead of default conflict failures. It also documents the required external-dns Pod Identity cleanup, though no executable migration sequence or tests were added.
| helm = { | ||
| source = "hashicorp/helm" | ||
| version = ">= 2.0" | ||
| } |
There was a problem hiding this comment.
Fresh migration selects incompatible Helm provider
helm >= 2.0 permits Helm provider 3.x, but legacy helm_release state may use schema 0, which Helm 3 cannot upgrade directly, so terraform init can fail decoding the state before the removed blocks execute. Should we constrain the migration wrapper and provider lock to a compatible Helm 2.17.x release for the first apply in both changed versions.tf files, or enforce the documented two-step upgrade?
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 `versions.tf`
around lines 26-29, update the migration-only `helm` provider constraint so a fresh
`terraform init` selects a compatible Helm 2.17.x release that can upgrade legacy
schema-0 `helm_release` state before the `removed` blocks execute. Apply the matching
constraint in the other changed `versions.tf` file as well, or otherwise enforce an
explicit two-step Helm 2.17.x then Helm 3 upgrade path.
| - **`mysql_vpn` import** — v6/DND-1522 creates the VPN→MySQL rule unconditionally, | ||
| but every env already has it out of state from the DND-752 era. Without an | ||
| `import{}` the apply fails `InvalidPermission.Duplicate`. See the rule-id table in | ||
| the rollout plan. |
There was a problem hiding this comment.
Stage 1 omits required MySQL imports
module.comet.module.comet_rds[0].aws_vpc_security_group_ingress_rule.mysql_vpn is created without importing the pre-existing DND-752 sgr-... rule, so the first brownfield apply attempts a duplicate MySQL TCP/3306 rule and fails with InvalidPermission.Duplicate — should we add one-shot per-environment imports at the complete module-qualified address before Stage 1?
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 MIGRATION.md around
lines 56-59, the Stage 1 `mysql_vpn` guidance is incomplete: the migration creates the
existing DND-752 security-group rule unconditionally, but no import block or
wrapper-side `imports.tf` is provided. Add explicit one-shot per-environment import
instructions, including the actual AWS security-group-rule IDs, targeting the complete
address
`module.comet.module.comet_rds[0].aws_vpc_security_group_ingress_rule.mysql_vpn`, and
require applying these imports before Stage 1 to prevent `InvalidPermission.Duplicate`.
| # Ignore CLI configuration files | ||
| .terraformrc | ||
| terraform.rc | ||
| *.local.md |
There was a problem hiding this comment.
Unrelated Markdown files become ignored
The *.local.md pattern ignores every Markdown file ending in .local.md, not just CLI configuration files, so unrelated local documentation such as README.local.md is silently hidden from Git — should we narrow the pattern to the intended filename(s) or broaden the comment?
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 `.gitignore` around
lines 40-40, review the `*.local.md` pattern under the CLI configuration files section
because it can silently ignore unrelated documents such as `README.local.md`. Narrow the
ignore rule to the specific CLI configuration filename(s) intended by the project; if
the broad pattern is deliberate, revise the surrounding comment to clearly describe that
scope.
waystar's v6 apply (#2213) failed on:
Error: updating EKS Cluster (waystar-use1) Auto Mode settings:
InvalidRequestException: Cannot modify EKS Auto Mode configuration.
Auto Mode is not enabled on this cluster.
comet_eks sent compute_config unconditionally, so a cluster with
enable_auto_mode = false got an explicit { enabled = false }. EKS rejects that
on a cluster that never had Auto Mode — it is not a no-op, it is an error, and
it fails the whole UpdateClusterConfig.
Only bayer and stsaasuat set eks_enable_auto_mode = true. The other ten envs
leave it unset, so every one of them would have hit this on its v6 bump. It did
not surface on bayer (#2205) precisely because bayer has Auto Mode on.
Fix: pass null when disabled. The upstream module guards with
`for_each = var.compute_config != null ? [var.compute_config] : []`, so null
omits the block rather than sending a disable.
Trade-off, kept deliberate: the previous comment argued the block should always
be sent so a cluster with Auto Mode on could be turned back off. That still
works, but now needs an explicit one-off (set enabled = false for that apply, or
disable out of band). Turning Auto Mode off is the rarer operation, and unlike
this failure it is not silent — you are doing it on purpose.
validate passes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
v6.0.1-migration is abandoned — it shipped compute_config unconditionally and EKS rejects that on clusters that never had Auto Mode (comet-devops#2213). Tags are immutable, so the fix ships as a new tag rather than moving the old one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
v6.0.1-migration-2 is deleted — it was cut after v6.0.1-migration had already been force-moved onto the same commit, so the two were identical and the 'superseded' note was wrong. v6.0.1-migration is now restored to its original commit (47845c7) so the compare against -3 shows the real one-file fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| compute_config = var.enable_auto_mode ? { | ||
| enabled = true | ||
| node_pools = var.auto_mode_node_pools | ||
| } : null |
There was a problem hiding this comment.
Unsafe Auto Mode disable transition
Setting enable_auto_mode = false now passes null, so the upstream EKS module omits aws_eks_cluster.compute_config and an already-enabled cluster can retain Auto Mode while its SG rules and auto_mode_pin are removed, leaving addons misplaced and nodes without data-layer connectivity. Since MIGRATION.md:14-18 also claims Stage 2 has no infrastructure change even though v6.0.0 restores { enabled = false, node_pools = [] } and can trigger a rejected EKS update, should we coordinate disabling Auto Mode before removing its consumers and keep both tags' configuration semantics consistent?
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 258-261, refactor the `compute_config` and
related Auto Mode consumers so disabling Auto Mode explicitly updates clusters that have
it enabled, while avoiding rejected updates for clusters that never had it enabled.
Coordinate the transition so Auto Mode scheduling, security-group rules, and data-layer
connectivity remain in place until disabling completes, rather than removing them
immediately when the toggle becomes false. Update `MIGRATION.md` around lines 14-18 and
the migration/permanent configurations so Stage 2 accurately preserves these semantics
and does not unexpectedly reintroduce an explicit `enabled = false` update.
| `v6.0.1-migration` (the unsuffixed tag) is abandoned — it shipped `compute_config` | ||
| unconditionally, which EKS rejects on any cluster that never had Auto Mode. Do not | ||
| use it. |
There was a problem hiding this comment.
Migration guide names wrong deliverable
The migration guide marks v6.0.1-migration abandoned and directs users to v6.0.1-migration-3, while the PR metadata names the unsuffixed tag as the deliverable, so users may select the wrong artifact — should we align the tag names or update the PR metadata if -3 is intended?
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 MIGRATION.md around
lines 23-25, update the migration-tag guidance so it matches the intended published
deliverable identified by the PR metadata. If v6.0.1-migration is the canonical tag,
remove the abandoned-tag warning and update the example reference accordingly; otherwise
retain the -3 guidance and correct the PR metadata to identify v6.0.1-migration-3 as the
deliverable.
…d-port 4b6b30e) waystar's apply cleared the Auto Mode error and then failed on the next one: Error: waiting for EKS Add-On (waystar-use1:cert-manager) create: CREATE_FAILED ... ConfigurationConflict: Conflicts found when trying to apply. Will not continue due to resolve conflicts mode. Same for external-dns. On a brownfield cluster the native EKS add-on refuses to take over the objects the eks_blueprints_addons Helm release still owns, because module.eks defaults resolve_conflicts_on_create = NONE. Alex fixed exactly this for bayer in 4b6b30e, but that commit only ever existed on the v5.6.2-migration side branch — it was never merged to main, so v6.0.0 lost it. v6 had 1 occurrence of resolve_conflicts_on_create (the ebs_csi addon); v5.6.2-migration had 3. This restores the other two verbatim, comments included. The native add-on adopts the Helm-owned objects in place, relabelling them managed-by=EKS; pods keep running. Harmless on greenfield. Operational note carried over from 4b6b30e: resolve_conflicts does NOT cover EKS Pod Identity associations. A pre-existing external-dns:external-dns association must be deleted before apply or it fails with ResourceInUseException. Ships as v6.0.1-migration-4; -3 is superseded. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| resolve_conflicts_on_create = "OVERWRITE" | ||
| resolve_conflicts_on_update = "OVERWRITE" |
There was a problem hiding this comment.
Legacy external-dns credentials remain active
The migration retains the legacy eks_blueprints_addons external-dns IRSA role and ServiceAccount annotation, so old pods can continue assuming the Route53-write role alongside the native add-on — should we revoke or isolate them after the native add-on is healthy and enforce that step in every environment?
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 351-352, fix the external-dns add-on migration so
setting resolve_conflicts to OVERWRITE does not leave the legacy eks_blueprints_addons
IRSA role and ServiceAccount usable in parallel. Trace the retained Helm resources and,
after confirming the native add-on is healthy, remove the old ServiceAccount IRSA
annotation and revoke/delete or otherwise isolate the legacy Route53-writable role,
enforcing this cutover and ordering in every environment. Preserve the old resources
only as long as required for the handoff, with explicit dependencies or validation
preventing cleanup before the native workload is ready.
1 participant
User description
Basis for the
v6.0.1-migrationtag — the one-apply stepping stone the remaining 11 brownfield envs need to reachv6.0.0.v5.6.2-migration(feat/v5-brownfield-removed-blocks): the tag is the deliverable, the branch never merges.removed.tfmust not exist on the permanent line — that is the whole point of it being a temporary version. This PR is open for review of the tag's contents.Tag is pushed:
v6.0.1-migration.Why not reuse
v5.6.2-migrationIt predates DND-875. Missing
rds_auto_minor_version_upgrade— which porsche, si, waystar and zoox all pass today — plusrds_parameter_group_family,rds_use_proxy_endpointandrds_proxy_ack_no_iam_auth. Those envs fail on an unsupported argument before theremoved{}blocks ever run.Why cut from v6.0.0, not v5.7.0
Both carry the full DND-875 surface. Starting at v6 means Stage 1 lands on the final surface, so Stage 2 is only "drop the providers map" with no second behaviour change — the DND-1522 toggle removal and the
redis_vpnre-index happen once, in Stage 1. Via v5.7.0 they would happen in Stage 2, on top of a migration.Contents
v6.0.0+ exactly two things:1.
modules/comet_eks/removed.tf— 13 blocks, alldestroy = false. Every address verified present on bothv2.1.2andv1.20.11:blueprints{alb, cert_manager, external_dns},storage_class.gp3,namespace.monitoring,secret.monitoringblueprints.aws_cloudwatch_metricshelm_release{karpenter_stsaas, external_secrets, external_secrets_crds},storage_class.comet_generic,annotations.{admin,app}_ns_node_selector2.
kubernetes+helmrequirements back in bothversions.tffiles — requirements only, no provider config blocks. The wrapper passes its own configured providers in; an emptyprovider "aws" {}here would hijack credentials.Plus
MIGRATION.md, restored (v6.0.0 deleted it) and rewritten for this sequence.Coverage — checked against real state, not assumed
si and porsche carry
kubernetes_cluster_role.agentro_extras,kubernetes_cluster_role_binding.{agentro_extras,agentro_view},kubernetes_role{,_binding}.agentro_portforward(and on porsche alsonamespace/secret.monitoring) at the state root, not undermodule.comet. Not module-addressed, so they getremoved{}in the wrapper. Called out in both the file header and MIGRATION.md.Verification
terraform validatepassesfmt -checkcleanremoved{}address confirmed to exist as a real resource on the source tags — a typo'd address is a silent no-op that strands the orphanRollout
Canary waystar — its 6 orphans are exactly the set proven against bayer (#2205), no root-level extras, no
aws_cloudwatch_metrics. Then zoox, si, porsche. Group C (v1.20.x) after Group B; those additionally hit Karpenter-Helm → EKS Auto Mode, which is its own piece of work.🤖 Generated with Claude Code
Generated description
Below is a concise technical summary of the changes proposed in this PR:
Enable a temporary, one-apply migration path for brownfield EKS clusters moving from legacy module versions to
v6.0.0, preserving live workloads while removing obsolete Terraform state and restoring provider requirements. Prevent invalid Auto Mode updates, support native add-on adoption through conflict overwrites, and document the staged rollout, provider wiring, orphan coverage, and post-migration cleanup.removed{}blocks, temporary provider requirements, wrapper provider passing, and a staged brownfield rollout that transitions ownership to GitOps and native EKS add-ons.Modified files (4)
Latest Contributors(2)
compute_configwhen disabled, and let native cert-manager and external-dns add-ons adopt conflicting brownfield resources through overwrite conflict resolution while documenting Pod Identity limitations.Modified files (1)
Latest Contributors(2)
Modified files (1)
Latest Contributors(2)