Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -37,3 +37,4 @@ override.tf.json
# Ignore CLI configuration files
.terraformrc
terraform.rc
*.local.md

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Severity

Want Baz to fix this for you? Activate Fixer

Other fix methods

Fix in Cursor

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.

99 changes: 99 additions & 0 deletions MIGRATION.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
# Brownfield migration to v6.0.0 (DND-1573 / DND-1257)

For clusters still on a v1.20.x or v2.1.x module version. Greenfield clusters and
anything already on v5.x/v6.x do not need this — bump straight to `v6.0.0`.

## Why a temporary tag

The v5 "Infra-Only + ArgoCD" refactor deleted the module's in-cluster and
eks-blueprints-addons resources; GitOps and native EKS add-ons own them now.
A cluster upgrading from v1/v2 still carries those objects in state, and because
the module also dropped its kubernetes/helm provider *config blocks*, `plan` fails
with `Provider configuration not present` for every one of them.

`v6.0.1-migration-4` is `v6.0.0` plus:

- `modules/comet_eks/removed.tf` — 13 `removed{}` blocks, all `destroy = false`
- `kubernetes` + `helm` back in both `versions.tf` files (requirements only, no
provider config blocks)

It is consumed for exactly ONE apply per cluster, then the wrapper moves to the
permanent `v6.0.0`.

`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.
Comment on lines +23 to +25

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Severity

Want Baz to fix this for you? Activate Fixer

Other fix methods

Fix in Cursor

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.


Do not use `v5.6.2-migration` for this. It predates DND-875 and lacks
`rds_auto_minor_version_upgrade`, `rds_parameter_group_family`,
`rds_use_proxy_endpoint` and `rds_proxy_ack_no_iam_auth` — envs that pass any of
them fail on an unsupported argument before the removed blocks run.

## Stage 1 — drop the orphans

Wrapper:

```hcl
module "comet" {
source = "github.com/comet-ml/terraform-aws-comet-stsaas?ref=v6.0.1-migration-4"

providers = {
aws = aws
kubernetes = kubernetes
helm = helm
}
...
}
```

The `providers` map is required: the orphans bind to
`module.comet.provider["...{aws,kubernetes,helm}"]`, and this populates that with
the wrapper's real assume_role/region. An empty `provider "aws" {}` inside the
module would hijack credentials instead (AccessDenied).

Also in the same PR:

- **Root-level orphans** — porsche and si carry `kubernetes_cluster_role.agentro_extras`,
`kubernetes_cluster_role_binding.{agentro_extras,agentro_view}` and
`kubernetes_role{,_binding}.agentro_portforward` at the STATE ROOT. They are not
module-addressed, so add `removed{}` blocks for them in the WRAPPER.
- **`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.
Comment on lines +60 to +63

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Severity web_search

Want Baz to fix this for you? Activate Fixer

Other fix methods

Fix in Cursor

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`.

- **Drop deleted vars** — `enable_argocd_management_eks_access`,
`enable_vpn_eks_api_access`, `enable_ci_runners_eks_api_access`,
`enable_vpn_redis_access`, `enable_monitoring_setup`, `monitoring_namespace`,
`eks_create_comet_generic_storage_class`, `enable_redis_insights_ns`.

Apply. The objects leave state; live infrastructure is untouched.

## Stage 1.5 — GitOps adoption

comet-infra (ArgoCD) / native EKS add-ons must own the ALB controller, cert-manager,
external-dns, the gp3 StorageClass and the monitoring namespace/secret. Coordinate
this with Stage 1 — it is the real risk in the sequence, not the terraform.

## Stage 2 — land on the permanent tag

```hcl
source = "github.com/comet-ml/terraform-aws-comet-stsaas?ref=v6.0.0"
```

Drop the `providers` map and `helm` from the wrapper's `required_providers`. **Keep
`kubernetes`** if the wrapper's own modules use it — `agentro_k8s_rbac` and
`automation_smoke_rbac` own kubernetes resources outside `module.comet`, and dropping
the provider strands them.

Delete the wrapper's one-shot `imports.tf` and any Stage 1 `removed{}` blocks once
applied.

Expected plan: no infrastructure change. Stage 1 already landed on the v6 surface.

## Order

Canary **waystar** — its orphan set is exactly the six blocks proven against bayer,
with no root-level extras and no `aws_cloudwatch_metrics`. Then zoox, si, porsche.
Group C (v1.20.x: circuit, circuit-dev, eonnext, fetch, mercedesamgf1, netflix)
after Group B is complete; those additionally hit the Karpenter-Helm → EKS Auto Mode
migration, which is its own piece of work.
38 changes: 30 additions & 8 deletions modules/comet_eks/main.tf
Original file line number Diff line number Diff line change
Expand Up @@ -244,14 +244,21 @@ module "eks" {

# EKS Auto Mode. When enabled, the control plane provisions nodes via the
# built-in node pools and the upstream module auto-creates/wires the Auto Mode
# node IAM role (so node_role_arn is intentionally omitted). The block is
# always sent — enabled = false explicitly disables Auto Mode so a cluster that
# previously had it on can be turned back off (a bare null would omit the
# argument and leave the last-applied config in place).
compute_config = {
enabled = var.enable_auto_mode
node_pools = var.enable_auto_mode ? var.auto_mode_node_pools : []
}
# node IAM role (so node_role_arn is intentionally omitted).
#
# null when disabled, NOT { enabled = false }. Sending an explicit disable to a
# cluster that never had Auto Mode makes EKS reject the whole UpdateClusterConfig
# with "Cannot modify EKS Auto Mode configuration. Auto Mode is not enabled on
# this cluster." That failed waystar's v6 apply (#2213) and would break every env
# where eks_enable_auto_mode is unset — 10 of 13.
#
# Turning Auto Mode back OFF on a cluster that has it therefore needs a
# deliberate one-off: set enabled = false here for that apply, or disable it out
# of band. That is the rarer operation, and unlike this failure it is not silent.
compute_config = var.enable_auto_mode ? {
enabled = true
node_pools = var.auto_mode_node_pools
} : null
Comment on lines +258 to +261

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Severity web_search

Want Baz to fix this for you? Activate Fixer

Other fix methods

Fix in Cursor

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.


# Bake the Karpenter discovery tag directly into the node SG so it is never
# dropped when Terraform modifies the security group during subsequent applies.
Expand Down Expand Up @@ -312,6 +319,14 @@ module "eks" {
} : {},
{
configuration_values = local.coredns_config
# OVERWRITE so a brownfield cluster (upgrading from the eks_blueprints_addons
# Helm cert-manager) lets the native add-on ADOPT the existing Helm-owned
# objects (SAs/CRDs/Deployments/webhooks) instead of failing with
# "ConfigurationConflict … resolve conflicts mode" (default NONE). The add-on
# relabels them managed-by=EKS in place — cert-manager keeps running. Harmless
# on greenfield (nothing to conflict). DND-1573.
resolve_conflicts_on_create = "OVERWRITE"
resolve_conflicts_on_update = "OVERWRITE"
}
)
} : {},
Expand All @@ -328,6 +343,13 @@ module "eks" {
role_arn = aws_iam_role.external_dns[0].arn
service_account = "external-dns"
}]
# OVERWRITE so a brownfield cluster adopts any external-dns k8s objects left by
# the old eks_blueprints_addons Helm release rather than conflict-failing. NOTE:
# a pre-existing Pod Identity association for external-dns:external-dns must be
# deleted first (ResourceInUseException otherwise) — resolve_conflicts does not
# cover Pod Identity associations. DND-1573.
resolve_conflicts_on_create = "OVERWRITE"
resolve_conflicts_on_update = "OVERWRITE"
Comment on lines +351 to +352

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Severity web_search

Want Baz to fix this for you? Activate Fixer

Other fix methods

Fix in Cursor

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.

},
var.eks_external_dns_addon_version != null ? {
addon_version = var.eks_external_dns_addon_version
Expand Down
149 changes: 149 additions & 0 deletions modules/comet_eks/removed.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1,149 @@
# Brownfield state migration (v1.x/v2.x → v6.x) — DND-1573 / DND-1257.
#
# ⚠️ THIS FILE MAKES THIS A TEMPORARY "MIGRATION" MODULE VERSION. It is meant to be
# consumed for exactly ONE apply per brownfield cluster, then the wrapper bumps to
# the permanent v6.0.0 tag WITHOUT this file. See MIGRATION.md at the repo root.
#
# Supersedes v5.6.2-migration, which cannot be used by the remaining brownfield envs:
# 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 would fail on an unsupported argument
# before the removed{} blocks below ever ran.
#
# Cut from v6.0.0 rather than v5.7.0 so Stage 1 lands directly on the v6 surface —
# Stage 2 is then only "drop the providers map", with no second behaviour change.
#
# The v5 "Infra-Only + ArgoCD" refactor DELETED these in-cluster / eks-blueprints-addons
# resources from the module (they are now owned by GitOps / native EKS add-ons). Clusters
# upgrading from a v1/v2 module version still carry them in state, and because the module
# root's kubernetes/helm/aws provider *config blocks* were also removed (child-module
# design), `terraform plan` fails with "Provider configuration not present" for all of
# them until the objects leave state.
#
# HOW THIS WORKS (proven against bayer, DND-1573 / #2205):
# - These `removed` blocks drop the objects from state WITHOUT destroying the live
# resources (`lifecycle { destroy = false }`) — the live workloads are adopted by
# comet-infra (ArgoCD) / native EKS add-ons as part of the coordinated cutover.
# - The orphans bind in state to `module.comet.provider["...{aws,kubernetes,helm}"]`.
# To satisfy that binding the module declares the kubernetes+helm *requirements*
# (versions.tf) — but declares NO provider config blocks. Instead the WRAPPER passes
# its fully-configured providers in:
# module "comet" {
# providers = { aws = aws, kubernetes = kubernetes, helm = helm }
# }
# This populates module.comet.provider[...] with the wrapper's real assume_role/region
# (an empty `provider "aws" {}` here would instead HIJACK credentials → AccessDenied).
# - Greenfield v6 clusters never had these resources, so the blocks are a harmless no-op.
#
# NOT COVERED HERE — root-level orphans. porsche and si additionally carry
# kubernetes_cluster_role.agentro_extras, kubernetes_cluster_role_binding.agentro_extras,
# kubernetes_cluster_role_binding.agentro_view, kubernetes_role.agentro_portforward and
# kubernetes_role_binding.agentro_portforward at the STATE ROOT, not under module.comet.
# They are not module-addressed, so they must be removed{} in the WRAPPER, not here.
#
# NOTE: do NOT `removed` the whole `module.eks_blueprints_addons` — its `aws_eks_addon.this[*]`
# children (coredns/kube-proxy/vpc-cni/metrics-server/ebs-csi) are relocated via the `moved`
# blocks in main.tf. Only the Helm-based sub-modules below were deleted.

# --- eks-blueprints-addons Helm sub-modules ---
# Removing the sub-module call drops all of its resources (aws_iam_policy/role/attachment +
# helm_release). ALB controller → comet-infra (ArgoCD); cert-manager + external-dns → native
# EKS managed add-ons (see main.tf module.eks.addons).
removed {
from = module.eks_blueprints_addons.module.aws_load_balancer_controller
lifecycle {
destroy = false
}
Comment on lines +52 to +56

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Severity web_search

Want Baz to fix this for you? Activate Fixer

Other fix methods

Fix in Cursor

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
}
Comment on lines +59 to +70

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Severity web_search

Want Baz to fix this for you? Activate Fixer

Other fix methods

Fix in Cursor

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

# zoox only — enabled there via eks_aws_cloudwatch_metrics, which v6 removed. Harmless
# no-op on every other cluster.
removed {
from = module.eks_blueprints_addons.module.aws_cloudwatch_metrics
lifecycle {
destroy = false
}
}

# --- In-cluster resources now owned by comet-infra (ArgoCD) ---
# gp3 StorageClass + monitoring namespace/secret moved to the comet-infra Helm chart.
removed {
from = kubernetes_storage_class.gp3
lifecycle {
destroy = false
}
}

removed {
from = kubernetes_namespace.monitoring
lifecycle {
destroy = false
}
}

removed {
from = kubernetes_secret.monitoring
lifecycle {
destroy = false
}
}

# --- v1.20.x-only in-cluster resources (Group C) ---
# Present on circuit, circuit-dev, eonnext, fetch, mercedesamgf1 and netflix; a no-op on
# the v2.1.x envs. Karpenter-via-Helm is superseded by EKS Auto Mode, external-secrets and
# the comet-generic StorageClass by comet-infra GitOps.
removed {
from = helm_release.karpenter_stsaas
lifecycle {
destroy = false
}
}

removed {
from = helm_release.external_secrets
lifecycle {
destroy = false
}
}

removed {
from = helm_release.external_secrets_crds
lifecycle {
destroy = false
}
}

removed {
from = kubernetes_storage_class.comet_generic
lifecycle {
destroy = false
}
}

removed {
from = kubernetes_annotations.admin_ns_node_selector
lifecycle {
destroy = false
}
}

removed {
from = kubernetes_annotations.app_ns_node_selector
lifecycle {
destroy = false
}
}
14 changes: 14 additions & 0 deletions modules/comet_eks/versions.tf
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,19 @@ terraform {
source = "hashicorp/time"
version = ">= 0.9"
}
# kubernetes + helm are declared ONLY so brownfield clusters (upgrading from a
# v1/v2 module version) can associate their leftover in-cluster / helm_release
# resources with a provider long enough for the `removed` blocks in removed.tf to
# drop them from state (destroy = false). v6 no longer CREATES any kubernetes/helm
# resource — greenfield clusters never instantiate these providers. Removed again
# in the permanent v6.0.0 tag (DND-1573 / DND-1257 brownfield migration).
kubernetes = {
source = "hashicorp/kubernetes"
version = ">= 2.0"
}
helm = {
source = "hashicorp/helm"
version = ">= 2.0"
}
}
}
14 changes: 14 additions & 0 deletions versions.tf
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,19 @@ terraform {
source = "hashicorp/random"
version = ">= 3.0"
}
# kubernetes + helm are declared ONLY so brownfield clusters (upgrading from a
# v1/v2 module version) can associate their leftover in-cluster / helm_release
# resources with a provider long enough for the `removed` blocks in removed.tf to
# drop them from state (destroy = false). v6 no longer CREATES any kubernetes/helm
# resource — greenfield clusters never instantiate these providers. Removed again
# in the permanent v6.0.0 tag (DND-1573 / DND-1257 brownfield migration).
kubernetes = {
source = "hashicorp/kubernetes"
version = ">= 2.0"
}
helm = {
source = "hashicorp/helm"
version = ">= 2.0"
}
Comment on lines +26 to +29

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Severity web_search

Want Baz to fix this for you? Activate Fixer

Other fix methods

Fix in Cursor

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.

}
}