Repository navigation
feat(eks): manage AWS Load Balancer Controller via ArgoCD (IAM stays in TF) - #47
Merged
obezpalko merged 4 commits intoJul 24, 2026
Merged
Conversation
Upgrade the helm provider from v2 to v3 (plugin-framework, protocol v6)
and bring related modules/providers in line with the comet-devops
reference config.
Provider constraints (root + modules/comet_eks):
- helm: ~> 2.10 -> >= 3.1.0
- kubernetes: ~> 2.21 -> >= 3.0
Helm v3 syntax migration:
- providers.tf: helm `kubernetes {}` / `exec {}` blocks -> attribute
assignment (`= {}`), matching the v3 schema and the comet-devops repo.
- modules/comet_eks/main.tf: convert 18 `set {}` blocks -> `set = [{...}]`
list-of-objects form across external_secrets_crds, external_secrets,
and karpenter_stsaas helm_release resources.
Module upgrades:
- eks-blueprints-addons: 1.9.1 -> ~> 1.24 (first release supporting
helm v3; all consumed inputs verified unchanged).
- irsa-ebs-csi: migrate from iam//iam-assumable-role-with-oidc v4.7.0 to
iam//iam-role-for-service-accounts-eks ~> 5.39 with attach_ebs_csi_policy,
unifying all IAM submodules on v5 and dropping the now-redundant
aws_iam_policy.ebs_csi_policy data source.
Lock file:
- Stop tracking .terraform.lock.hcl and add it to .gitignore. This repo is
a reusable module (no active backend); the consuming root owns the
authoritative lock, which Terraform ignores from a child module.
Validated with `terraform validate` (passes; only upstream deprecation
warnings). NOT yet planned against a live cluster — the helm v3 state
migration, kubernetes provider v3 bump, and irsa-ebs-csi submodule swap
(possible IRSA role replacement) require plan review before apply.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rivy, secrets) (#43) * ci: add pre-commit hooks and GitHub Actions CI Add a pre-commit config and a CI workflow to enforce Terraform hygiene and secret scanning on every PR. .pre-commit-config.yaml: - terraform_fmt, terraform_validate (init with -backend=false so it runs on a fresh checkout with no state). - gitleaks for secret/credential scanning. - generic hygiene: trailing-whitespace, end-of-file-fixer, check-merge-conflict, check-yaml. - tflint is deliberately CI-only (needs a separately installed binary), so local `pre-commit run` doesn't hard-fail for contributors without it. .github/workflows/ci.yml — three jobs intended as required status checks: - pre-commit: runs the hooks above (fmt/validate/secrets/hygiene). - tflint: installs tflint and lints root + child modules recursively. - trivy: IaC misconfiguration scan (config), failing on MEDIUM+. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * style: apply pre-commit fixes (whitespace, EOF newlines) Mechanical cleanup applied by the newly-added pre-commit hooks across pre-existing files: trim trailing whitespace and ensure a single final newline. No semantic changes. Committed separately from the tooling change so each stays reviewable on its own. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * ci: scope trivy to our code and make scanners report-only The first CI run surfaced two issues with the scanner setup: - trivy was scanning `.terraform/` (the provider/module cache), flagging ~30 findings in downloaded third-party modules (AWS eks/vpc/alb/iam) and their example manifests — code we don't own. Add skip-dirs `**/.terraform` so only our own modules are scanned (~19 real findings remain). - Both scanners failed the build on pre-existing findings. Make them report-only for now (trivy exit-code 0; tflint `|| true`) so PRs aren't blocked on a day-one backlog. The remaining trivy findings (S3 public-access/encryption, ALB SG rules, elasticache at-rest encryption) and tflint warnings are tracked for a dedicated hardening follow-up; flip the flags back to blocking once triaged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * ci: run pre-commit manually to drop Node-20 deprecation pre-commit/action@v3.0.1 (its latest release) bundles a Node-20-era actions/cache, which GitHub now flags as deprecated. Replace it with an explicit pip install + `pre-commit run`, caching hook environments via actions/cache@v4 (Node 24). Behavior is identical; the deprecation warning is gone. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * ci: use actions/cache@v6 (latest) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The ALB controller has no native EKS add-on, so it stays a Helm chart — but move its installation out of eks_blueprints_addons and into ArgoCD (comet-gitops), deployed per stsaas customer. Terraform keeps the IAM. - Add module.aws_load_balancer_controller_irsa_role (iam-role-for-service-accounts-eks, attach_load_balancer_controller_policy), OIDC-trusted for kube-system:aws-load-balancer-controller. Gated by the existing eks_aws_load_balancer_controller toggle. - Drop enable_aws_load_balancer_controller from the eks_blueprints_addons call — the module no longer installs the chart. - Expose outputs for the gitops repo to consume: the IRSA role arn/name plus cluster facts (region, vpc_id; cluster_name already exported). The ArgoCD Application annotates the controller ServiceAccount with the role ARN and sets clusterName/region/vpcId in the Helm values. MIGRATION: on a cluster where eks_blueprints_addons currently runs the controller, adopt or remove that Helm release before ArgoCD takes over, to avoid two controllers reconciling the same Ingresses. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
Comment on lines
+429
to
+432
| # aws_load_balancer_controller is NOT installed here — the Helm chart is deployed | ||
| # per stsaas customer via ArgoCD (comet-gitops). This module only creates its IRSA | ||
| # role (module.aws_load_balancer_controller_irsa_role) and exposes the ARN as an | ||
| # output for the ArgoCD Application to wire onto the controller ServiceAccount. |
There was a problem hiding this comment.
Unsafe controller handoff
Removing enable_aws_load_balancer_controller stops Terraform from installing the AWS Load Balancer Controller, but main.tf still forwards the same var.eks_aws_load_balancer_controller toggle from existing callers, so an upgrade can drop the old Helm release before ArgoCD has adopted it or run two controllers against the same Ingresses — should we add an explicit adoption/state-move step or keep the Helm release active until the GitOps app is live?
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 429-432 (the EKS add-ons configuration where
`enable_aws_load_balancer_controller` was removed/passed), update the cutover logic so
upgrades can’t temporarily remove the controller or cause duplicate controllers.
Refactor this section to either (1) keep the existing Terraform-managed Helm release
enabled until a new explicit “gitops_adopted”/“arogcd_app_live” input is set, or
(2) implement a Terraform state move/adoption step (e.g., using `moved` blocks or a
clear migration procedure plus guardrails) so the old release is only removed after
ArgoCD has successfully taken over. Also add a clear validation/warning that prevents
users from disabling the Helm release while `var.eks_aws_load_balancer_controller` is
still true but the ArgoCD app is not confirmed live, and adjust any dependent logic so
the new IRSA role wiring doesn’t lead to a window with no controller.
jms200
reviewed
Jul 20, 2026
jms200
left a comment
jms200
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed against alexb/helm-v3-provider-upgrade and the live module source. IRSA-in-TF / chart-in-ArgoCD split is correct and mirrors the external-secrets/karpenter pattern. One cleanup.
🟡 time_sleep.wait_for_alb_webhook is now dead / misleading
This PR drops enable_aws_load_balancer_controller from eks_blueprints_addons, but leaves the webhook wait in place (modules/comet_eks/main.tf, resource "time_sleep" "wait_for_alb_webhook"), still gated on var.eks_aws_load_balancer_controller and still depends_on = [module.eks_blueprints_addons]. Three resources depend on it — external_secrets_crds, external_secrets, and kubernetes_namespace.monitoring.
Since the ALB mutating webhook is no longer installed by Terraform (ArgoCD installs it later), this 60s sleep no longer waits for anything real and the ordering guarantee it implied is gone. Please remove the time_sleep block and its three depends_on references (or explicitly document that the wait is intentionally retired). Likely benign at runtime, but it's stale code that reads as protective when it isn't.
✅ Otherwise correct
The IRSA role, attach_load_balancer_controller_policy, OIDC SA binding (kube-system:aws-load-balancer-controller), and the new outputs (cluster_region = var.region, cluster_vpc_id = var.vpc_id — both exist in the module) are all correct.
Note (cross-PR)
This PR and #46 both edit the same enable_* lines in the eks_blueprints_addons block (branched independently off #42) → guaranteed merge conflict. Suggest merging #46 first, then rebasing this on top. Also rebase onto main after #42 merges.
obezpalko
force-pushed
the
alexb/helm-v3-provider-upgrade
branch
from
July 23, 2026 15:22
e8656d1 to
9b0382f
Compare
…into feat/alb-controller-argocd # Conflicts: # modules/comet_eks/main.tf
obezpalko
added a commit
that referenced
this pull request
Jul 28, 2026
…ia ArgoCD (#50) * feat(eks): Auto Mode + native add-ons (cert-manager/external-dns/observability) + addon HA (#49) * feat(providers): upgrade helm to v3 and align kubernetes/IAM modules Upgrade the helm provider from v2 to v3 (plugin-framework, protocol v6) and bring related modules/providers in line with the comet-devops reference config. Provider constraints (root + modules/comet_eks): - helm: ~> 2.10 -> >= 3.1.0 - kubernetes: ~> 2.21 -> >= 3.0 Helm v3 syntax migration: - providers.tf: helm `kubernetes {}` / `exec {}` blocks -> attribute assignment (`= {}`), matching the v3 schema and the comet-devops repo. - modules/comet_eks/main.tf: convert 18 `set {}` blocks -> `set = [{...}]` list-of-objects form across external_secrets_crds, external_secrets, and karpenter_stsaas helm_release resources. Module upgrades: - eks-blueprints-addons: 1.9.1 -> ~> 1.24 (first release supporting helm v3; all consumed inputs verified unchanged). - irsa-ebs-csi: migrate from iam//iam-assumable-role-with-oidc v4.7.0 to iam//iam-role-for-service-accounts-eks ~> 5.39 with attach_ebs_csi_policy, unifying all IAM submodules on v5 and dropping the now-redundant aws_iam_policy.ebs_csi_policy data source. Lock file: - Stop tracking .terraform.lock.hcl and add it to .gitignore. This repo is a reusable module (no active backend); the consuming root owns the authoritative lock, which Terraform ignores from a child module. Validated with `terraform validate` (passes; only upstream deprecation warnings). NOT yet planned against a live cluster — the helm v3 state migration, kubernetes provider v3 bump, and irsa-ebs-csi submodule swap (possible IRSA role replacement) require plan review before apply. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * ci: add pre-commit hooks and GitHub Actions (fmt, validate, tflint, trivy, secrets) (#43) * ci: add pre-commit hooks and GitHub Actions CI Add a pre-commit config and a CI workflow to enforce Terraform hygiene and secret scanning on every PR. .pre-commit-config.yaml: - terraform_fmt, terraform_validate (init with -backend=false so it runs on a fresh checkout with no state). - gitleaks for secret/credential scanning. - generic hygiene: trailing-whitespace, end-of-file-fixer, check-merge-conflict, check-yaml. - tflint is deliberately CI-only (needs a separately installed binary), so local `pre-commit run` doesn't hard-fail for contributors without it. .github/workflows/ci.yml — three jobs intended as required status checks: - pre-commit: runs the hooks above (fmt/validate/secrets/hygiene). - tflint: installs tflint and lints root + child modules recursively. - trivy: IaC misconfiguration scan (config), failing on MEDIUM+. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * style: apply pre-commit fixes (whitespace, EOF newlines) Mechanical cleanup applied by the newly-added pre-commit hooks across pre-existing files: trim trailing whitespace and ensure a single final newline. No semantic changes. Committed separately from the tooling change so each stays reviewable on its own. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * ci: scope trivy to our code and make scanners report-only The first CI run surfaced two issues with the scanner setup: - trivy was scanning `.terraform/` (the provider/module cache), flagging ~30 findings in downloaded third-party modules (AWS eks/vpc/alb/iam) and their example manifests — code we don't own. Add skip-dirs `**/.terraform` so only our own modules are scanned (~19 real findings remain). - Both scanners failed the build on pre-existing findings. Make them report-only for now (trivy exit-code 0; tflint `|| true`) so PRs aren't blocked on a day-one backlog. The remaining trivy findings (S3 public-access/encryption, ALB SG rules, elasticache at-rest encryption) and tflint warnings are tracked for a dedicated hardening follow-up; flip the flags back to blocking once triaged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * ci: run pre-commit manually to drop Node-20 deprecation pre-commit/action@v3.0.1 (its latest release) bundles a Node-20-era actions/cache, which GitHub now flags as deprecated. Replace it with an explicit pip install + `pre-commit run`, caching hook environments via actions/cache@v4 (Node 24). Behavior is identical; the deprecation warning is gone. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * ci: use actions/cache@v6 (latest) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(eks): move cert-manager and external-dns to native EKS add-ons Replace the eks_blueprints_addons Helm releases for cert-manager and external-dns with native EKS managed add-ons. Add-ons install via the EKS control-plane API (no data-plane/cluster access needed — works on private clusters) and are version-managed by AWS. - cert-manager -> eks_addons entry. No IAM required. - external-dns -> eks_addons entry using EKS Pod Identity (not IRSA): adds a pods.eks.amazonaws.com-trusted role with Route53 permissions scoped to eks_external_dns_r53_zones, a pod_identity_association, and the eks-pod-identity-agent add-on (enabled whenever external-dns is on). - Drop enable_cert_manager / enable_external_dns / external_dns_route53_zone_arns from the eks_blueprints_addons call. - New optional vars: eks_cert_manager_addon_version, eks_external_dns_addon_version (null = EKS default for the cluster ver). aws_load_balancer_controller stays a Helm release — no native add-on exists for it. Behavior is gated by the existing eks_cert_manager / eks_external_dns toggles (both default false), so existing consumers are unaffected until they opt in. MIGRATION: on a cluster already running the Helm-based cert-manager/ external-dns, adopt or remove the old release before enabling the add-on to avoid two controllers fighting. See PR description. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(eks): add toggles for 3 observability EKS add-ons Add opt-in native EKS managed add-ons (all no-IAM), each gated by its own toggle, default off: - kube-state-metrics (eks_enable_kube_state_metrics) - prometheus-node-exporter (eks_enable_prometheus_node_exporter) - eks-node-monitoring-agent (eks_enable_node_monitoring_agent) Each has an optional *_addon_version var (null = AWS default for the cluster version). Wired root -> comet_eks -> eks_addons map. Deferred: aws-secrets-store-csi-driver-provider — it requires the base secrets-store-csi-driver (not an EKS add-on; install via helm/ArgoCD) and overlaps with the existing external-secrets setup; decide separately. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(eks): add EKS Auto Mode support (Part A) Add opt-in EKS Auto Mode as an alternative to Karpenter/managed node groups. Default off, so existing consumers are unaffected. - New vars (root + comet_eks): enable_auto_mode / auto_mode_node_pools (default node pools: system, general-purpose). - Wire compute_config into module.eks (v21). When enabled, the upstream module auto-creates and wires the Auto Mode node IAM role, so node_role_arn is intentionally omitted; left null when disabled. - Mutual-exclusion validation: enable_karpenter and enable_auto_mode cannot both be true. Scope is Part A only (control-plane node provisioning). Custom NodePool/NodeClass CRDs and the data-plane cleanup (EBS CSI, gp3 SC, ALB controller redundant under Auto Mode) are follow-ups — see docs/auto-mode-migration-plan.md. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(eks): gate managed node groups off under Auto Mode and make disable explicit Address review findings on PR #45: - Managed node groups (admin/comet/druid/airflow/clickhouse) and additional_node_groups now gate on !enable_auto_mode, so Auto Mode truly replaces MNGs instead of running double provisioners alongside the Auto Mode node pools. - Add a validation guard on enable_auto_mode rejecting any MNG toggle left enabled (admin defaults true, so callers must disable it). - compute_config is now always sent with enabled = var.enable_auto_mode (was null when disabled), so a cluster that previously had Auto Mode on can actually be turned back off. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(eks): support Auto Mode + managed node groups coexistence Reverses the MNG gating from the previous review-fix commit. Product decision: Auto Mode is intended to run ALONGSIDE managed node groups (MNGs for pinned/system workloads, Auto Mode node pools for elastic capacity), not replace them. - Drop the `&& !var.enable_auto_mode` guards from the admin/comet/druid/ airflow/clickhouse node groups and additional_node_groups. - Remove the validation block that rejected MNGs alongside Auto Mode. - Update enable_auto_mode docs (module + root) to describe coexistence. The compute_config disable fix (enabled = var.enable_auto_mode) is retained. Karpenter remains mutually exclusive with Auto Mode. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(eks): move native addons out of eks_blueprints_addons The native EKS managed addons were plain aws_eks_addon passthroughs through the blueprints wrapper, which added nothing. Move them to where they belong and slim the wrapper down to just the Helm-based controllers (ALB controller, cert-manager, external-dns, cloudwatch-metrics). - coredns/kube-proxy/vpc-cni/metrics-server -> module.eks.addons (vpc-cni provisioned before_compute). - aws-ebs-csi-driver -> standalone aws_eks_addon.ebs_csi. Kept out of module.eks.addons because its IRSA role depends on the module's OIDC output, which would create an eks -> irsa -> eks cycle. - moved blocks map every addon from its old blueprints address to the new one so existing clusters see a no-op, not destroy/recreate. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(eks): pin schedulable add-ons to the system node pool under Auto Mode When enable_auto_mode is true, pin the schedulable add-ons onto the built-in `system` node pool (labeled karpenter.sh/nodepool=system, tainted CriticalAddonsOnly=true:NoSchedule) via a nodeSelector plus a matching toleration: - coredns, metrics-server -> configuration_values (JSON) - aws-ebs-csi-driver controller -> configuration_values (nested under `controller`; the node DaemonSet is left alone) - ALB controller, cert-manager, external-dns, cloudwatch-metrics -> Helm `values` on blueprints vpc-cni and kube-proxy are DaemonSets and are intentionally not pinned. external-dns folds the module's default `provider: aws` back into its values so the override doesn't drop it. All payloads are empty when Auto Mode is off, so MNG/Karpenter deployments are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(eks): stop node groups tracking "$Latest"/"$Default" LT aliases Managed node groups passed launch_template_version = "$Latest" (or "$Default" when pinned) straight into aws_eks_node_group. AWS resolves that alias to a concrete version number and stores the number, while the config keeps the literal string, so every plan shows a perpetual `launch_template { version = "N" -> "$Latest" }` diff on all node groups that never converges after apply. It also cascades: the pending node group change forces dependent IRSA data sources to re-read, flipping several IAM roles/policies to "known after apply". Point launch_template_version at null instead. The upstream eks-managed-node-group module then coalesces to the launch template's numeric default_version, which already tracks latest-vs-pinned via update_launch_template_default_version (unchanged). Behaviour is identical; the spurious diff is gone. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(rds): stop perpetual diff on final_snapshot_identifier final_snapshot_identifier was built from timestamp(), which re-evaluates on every plan, so the aws_rds_cluster resource always showed a change (final_snapshot_identifier -> (known after apply)) even immediately after a successful apply. The attribute is only consumed at destroy time as the final snapshot's name. Add lifecycle { ignore_changes = [final_snapshot_identifier] } so the value first stored is retained and the resource stops churning. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(eks): inline coredns system-pool nodeSelector on the addon Spell the nodeSelector + toleration out directly in the coredns configuration_values instead of pulling from the shared auto_mode_addon_config local, so the pinning is visible at the addon. Behavior is unchanged (still gated on enable_auto_mode). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(eks): stop blueprints controller config collapsing to a typed map The eks-blueprints-addons controller vars are typed `any` and read with a mix of try()/lookup(). Passing a single-key { values = [...] } object collapsed them to map(list(string)), breaking the module's lookup() calls (role_policies default {}, role_permissions_boundary_arn default null) and the typed IRSA sub-module inputs — surfacing as "string required, but have list of string" and lookup default-type errors on plan. Pass the looked-up keys explicitly with their native default types so each object stays a heterogeneous object (not a typed map), which the module's lookups accept. Empty {} when Auto Mode is off. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(eks): make controller config ternary branches type-consistent The previous commit's `var.enable_auto_mode ? {rich object} : {}` failed validate with "Inconsistent conditional result types". Give both paths the same object shape (empty values list when Auto Mode is off), which matches the blueprints module's own lookup() defaults so it's a no-op when disabled. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(eks): explicit HA for schedulable control-plane addons Set replicaCount/replicas=2, a PDB (maxUnavailable=1), and soft topologySpreadConstraints (whenUnsatisfiable=ScheduleAnyway) on the schedulable control-plane addons via each addon's configuration_values: coredns, metrics-server, cert-manager, kube-state-metrics, and the EBS CSI controller. - coredns / ebs-csi already ship a PDB + anti-affinity by default; making it explicit keeps HA visible in code and overrides the addon's own PDB (no conflicting second one). - metrics-server / kube-state-metrics had a single replica and no PDB. - external-dns is single-replica by design (leader election) and its addon schema exposes no replicaCount/PDB, so it takes system-pool pinning only, not the replica/PDB HA. - node-exporter / node-monitoring-agent are DaemonSets — untouched. ScheduleAnyway lets pods still schedule on a single-node pool (e.g. a 1-node Auto Mode `system` pool) instead of going Pending. HA applies regardless of Auto Mode; the system-pool nodeSelector/toleration layer in only when Auto Mode is enabled. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(eks): drop parentheses from external-dns role Description tag The external-dns Pod Identity role tagged Description with "(Route53)". AWS IAM tag values must match [\p{L}\p{Z}\p{N}_.:/=+\-@]*, which does not allow parentheses, so CreateRole fails with: ValidationError: Value at 'tags.N.member.value' failed to satisfy constraint: Member must satisfy regular expression pattern ... blocking apply on any env with eks_external_dns = true. Replace the parentheses with " - Route53" to stay within the allowed character set. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(eks): manage AWS Load Balancer Controller via ArgoCD (IAM stays in TF) (#47) * feat(providers): upgrade helm to v3 and align kubernetes/IAM modules Upgrade the helm provider from v2 to v3 (plugin-framework, protocol v6) and bring related modules/providers in line with the comet-devops reference config. Provider constraints (root + modules/comet_eks): - helm: ~> 2.10 -> >= 3.1.0 - kubernetes: ~> 2.21 -> >= 3.0 Helm v3 syntax migration: - providers.tf: helm `kubernetes {}` / `exec {}` blocks -> attribute assignment (`= {}`), matching the v3 schema and the comet-devops repo. - modules/comet_eks/main.tf: convert 18 `set {}` blocks -> `set = [{...}]` list-of-objects form across external_secrets_crds, external_secrets, and karpenter_stsaas helm_release resources. Module upgrades: - eks-blueprints-addons: 1.9.1 -> ~> 1.24 (first release supporting helm v3; all consumed inputs verified unchanged). - irsa-ebs-csi: migrate from iam//iam-assumable-role-with-oidc v4.7.0 to iam//iam-role-for-service-accounts-eks ~> 5.39 with attach_ebs_csi_policy, unifying all IAM submodules on v5 and dropping the now-redundant aws_iam_policy.ebs_csi_policy data source. Lock file: - Stop tracking .terraform.lock.hcl and add it to .gitignore. This repo is a reusable module (no active backend); the consuming root owns the authoritative lock, which Terraform ignores from a child module. Validated with `terraform validate` (passes; only upstream deprecation warnings). NOT yet planned against a live cluster — the helm v3 state migration, kubernetes provider v3 bump, and irsa-ebs-csi submodule swap (possible IRSA role replacement) require plan review before apply. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * ci: add pre-commit hooks and GitHub Actions (fmt, validate, tflint, trivy, secrets) (#43) * ci: add pre-commit hooks and GitHub Actions CI Add a pre-commit config and a CI workflow to enforce Terraform hygiene and secret scanning on every PR. .pre-commit-config.yaml: - terraform_fmt, terraform_validate (init with -backend=false so it runs on a fresh checkout with no state). - gitleaks for secret/credential scanning. - generic hygiene: trailing-whitespace, end-of-file-fixer, check-merge-conflict, check-yaml. - tflint is deliberately CI-only (needs a separately installed binary), so local `pre-commit run` doesn't hard-fail for contributors without it. .github/workflows/ci.yml — three jobs intended as required status checks: - pre-commit: runs the hooks above (fmt/validate/secrets/hygiene). - tflint: installs tflint and lints root + child modules recursively. - trivy: IaC misconfiguration scan (config), failing on MEDIUM+. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * style: apply pre-commit fixes (whitespace, EOF newlines) Mechanical cleanup applied by the newly-added pre-commit hooks across pre-existing files: trim trailing whitespace and ensure a single final newline. No semantic changes. Committed separately from the tooling change so each stays reviewable on its own. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * ci: scope trivy to our code and make scanners report-only The first CI run surfaced two issues with the scanner setup: - trivy was scanning `.terraform/` (the provider/module cache), flagging ~30 findings in downloaded third-party modules (AWS eks/vpc/alb/iam) and their example manifests — code we don't own. Add skip-dirs `**/.terraform` so only our own modules are scanned (~19 real findings remain). - Both scanners failed the build on pre-existing findings. Make them report-only for now (trivy exit-code 0; tflint `|| true`) so PRs aren't blocked on a day-one backlog. The remaining trivy findings (S3 public-access/encryption, ALB SG rules, elasticache at-rest encryption) and tflint warnings are tracked for a dedicated hardening follow-up; flip the flags back to blocking once triaged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * ci: run pre-commit manually to drop Node-20 deprecation pre-commit/action@v3.0.1 (its latest release) bundles a Node-20-era actions/cache, which GitHub now flags as deprecated. Replace it with an explicit pip install + `pre-commit run`, caching hook environments via actions/cache@v4 (Node 24). Behavior is identical; the deprecation warning is gone. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * ci: use actions/cache@v6 (latest) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(eks): manage AWS Load Balancer Controller via ArgoCD (IAM in TF) The ALB controller has no native EKS add-on, so it stays a Helm chart — but move its installation out of eks_blueprints_addons and into ArgoCD (comet-gitops), deployed per stsaas customer. Terraform keeps the IAM. - Add module.aws_load_balancer_controller_irsa_role (iam-role-for-service-accounts-eks, attach_load_balancer_controller_policy), OIDC-trusted for kube-system:aws-load-balancer-controller. Gated by the existing eks_aws_load_balancer_controller toggle. - Drop enable_aws_load_balancer_controller from the eks_blueprints_addons call — the module no longer installs the chart. - Expose outputs for the gitops repo to consume: the IRSA role arn/name plus cluster facts (region, vpc_id; cluster_name already exported). The ArgoCD Application annotates the controller ServiceAccount with the role ARN and sets clusterName/region/vpcId in the Helm values. MIGRATION: on a cluster where eks_blueprints_addons currently runs the controller, adopt or remove that Helm release before ArgoCD takes over, to avoid two controllers reconciling the same Ingresses. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(eks): add manage_monitoring_secret toggle to cede secret to ESO The monitoring Grafana-credentials Secret (monitoring/monitoring) is increasingly owned by External Secrets Operator: an ExternalSecret with creationPolicy: Owner syncs it from Secrets Manager, and its data is a superset of what Terraform seeds (adds MYSQL_EXPORTER_PASSWORD). With both managing the object, every apply strips ESO's labels and replaces the whole data map, briefly dropping ESO-only keys until the next reconcile. Add manage_monitoring_secret (default true, backward-compatible) so an environment where ESO owns the Secret can set it false and gate kubernetes_secret.monitoring off. Wired root -> comet_eks alongside enable_monitoring_setup; the resource count now requires both. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(eks): drop parentheses from ALB IRSA role Description tag Same IAM tag-regex constraint as the external-dns role fix: AWS IAM tag values must match [\p{L}\p{Z}\p{N}_.:/=+\-@]*, which disallows parens, so CreateRole for stsaasuat-use1-aws-load-balancer-controller fails with ValidationError on tags.N.member.value. Replace "(deployed via ArgoCD)" with " - deployed via ArgoCD". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(eks): SG rules for Auto Mode + managed node group coexistence Auto Mode nodes attach the EKS cluster primary SG; managed node groups use the module node SG. Neither allows the other, so cross-node-type pod traffic is dropped — managed-node pods cannot reach coredns on an Auto Mode node, breaking DNS cluster-wide for the managed-node fleet (and crash-looping cluster-autoscaler on STS/DNS timeouts). Add all-traffic ingress rules both ways between the two SGs, gated on enable_auto_mode (only needed during coexistence). Codifies the manual mitigation applied out-of-band on stsaasuat-use1. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(eks): set launch_template_version=null once in node-group defaults --------- Co-authored-by: Claude Opus 4.8 (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
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
Why
The AWS Load Balancer Controller has no native EKS add-on, so it stays a Helm chart. Per the GitOps direction, move its installation out of `eks_blueprints_addons` and into ArgoCD (comet-gitops), deployed per stsaas customer. Terraform keeps the IAM (the controller pod needs a role); ArgoCD owns the chart.
This mirrors the split already used for external-secrets and karpenter: AWS-side IAM in Terraform, in-cluster workload in GitOps.
Changes
How the gitops repo wires it (per customer)
The ArgoCD Application / Helm values for `aws-load-balancer-controller`:
Adopt or remove the eks_blueprints_addons Helm release before ArgoCD takes over, or two controllers will reconcile the same Ingresses. Sequence: stand up the ArgoCD app (SA annotated with this role) → adopt/remove the old release → verify a test Ingress provisions an ALB.
Verification
Note: also interacts with EKS Auto Mode (#45) — Auto Mode provides load balancing natively, which may make even this controller redundant on Auto Mode clusters. Out of scope here.
🤖 Generated with Claude Code
Generated description
Below is a concise technical summary of the changes proposed in this PR:
Move the AWS Load Balancer Controller installation out of
module.comet_eksand into ArgoCD-managed GitOps while keeping its IAM role in Terraform. Expose the controller role ARN plus cluster region and VPC details so the GitOps layer can wire theaws-load-balancer-controllerServiceAccountand Helm values.eks_blueprints_addonsand create an IRSA role inmodule.comet_eksfor the ArgoCD-managed controller.Modified files (1)
Latest Contributors(2)
module.comet_eksand the root module so ArgoCD can annotate theServiceAccountand configureclusterName,region, andvpcId.Modified files (2)
Latest Contributors(2)