feat(aws-eks): support ECO_DR cold standby - #1354
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an ECO_DR input to the modules/aws-eks Terraform module to support “cold standby” DR workspaces by conditionally skipping EKS/Karpenter/IAM creation and guarding outputs so the module can still evaluate without an EKS control plane.
Changes:
- Introduces
ECO_DRinput andlocal.eco_dr_enabledto gate creation of EKS, AWS lookups, and related resources. - Updates EKS module and data sources to use
count(0/1) and adjusts outputs to tolerate missing resources (viatry(...)). - Updates generated docs (
docs/header.md+ moduleREADME.md) to describe the new DR guard behavior.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/aws-eks/variables.tf | Changes Karpenter defaults and adds new ECO_DR input with validation. |
| modules/aws-eks/README.md | Regenerated docs reflecting ECO_DR and updated input defaults/table formatting. |
| modules/aws-eks/outputs.tf | Guards outputs for ECO_DR mode and avoids direct references when EKS is disabled. |
| modules/aws-eks/main.tf | Adds eco_dr_enabled local and gates EKS module creation with count. |
| modules/aws-eks/locals.tf | Ensures subnet selection is safe when ECO_DR is enabled. |
| modules/aws-eks/karpenter.tf | Prevents Karpenter module creation when ECO_DR is enabled. |
| modules/aws-eks/iam_parameter_store.tf | Gates Parameter Store IAM resources when ECO_DR is enabled and updates EKS module addressing. |
| modules/aws-eks/iam_karpenter.tf | Gates extra Karpenter IAM policy when ECO_DR is enabled. |
| modules/aws-eks/iam_external_dns.tf | Gates ExternalDNS IAM resources when ECO_DR is enabled and updates EKS module addressing. |
| modules/aws-eks/iam_efs_csi_driver.tf | Gates EFS CSI IAM resources when ECO_DR is enabled and updates EKS module addressing. |
| modules/aws-eks/iam_ebs_csi_driver.tf | Makes EBS CSI addon/IAM derivation ECO_DR-aware and updates account-id sourcing. |
| modules/aws-eks/iam_cloudwatch.tf | Gates CloudWatch IAM resources when ECO_DR is enabled and updates EKS module addressing. |
| modules/aws-eks/iam_alb.tf | Gates ALB IAM resources when ECO_DR is enabled and updates EKS module addressing. |
| modules/aws-eks/docs/header.md | Documents the new “Cold Standby DR Guard” feature. |
| modules/aws-eks/data.tf | Adds count guards to AWS lookups (caller identity, VPC, subnets) for ECO_DR mode. |
| modules/aws-eks/addons_locals.tf | Filters addons when ECO_DR is enabled and changes default addon set behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Suppressed comments (3)
modules/aws-eks/variables.tf:238
- Defaults for
karpenter_service_account_nameandkarpenter_namespace_namechanged (tokarpenter/kube-system). This is a potentially breaking behavior change unrelated to ECO_DR (it can change IRSA subject strings and resource names) and isn’t called out in the PR description. If this isn’t required for ECO_DR, consider reverting these defaults (users can still override explicitly).
variable "karpenter_service_account_name" {
description = "Set the name of K8s service account karpenter controller"
type = string
default = "karpenter"
}
modules/aws-eks/addons_locals.tf:38
eks-pod-identity-agentwas removed from the module’s defaultbase_addons. This is unrelated to ECO_DR and changes default behavior; it also makes the provided examples (which still configureeks-pod-identity-agent) misleading. Consider restoring the addon inbase_addonsso existing/example configurations continue to work as expected.
aws-ebs-csi-driver = {
enabled = true
resolve_conflicts_on_create = "OVERWRITE"
resolve_conflicts_on_update = "OVERWRITE"
most_recent = "false"
modules/aws-eks/README.md:139
- The Providers table lists the AWS provider version as
6.57.1, but the module constraint inversions.tfis~> 6.33. This makes the generated README misleading for consumers; the Providers section should reflect the configured constraint (or the constraint should be updated in code if6.57.1is now required).
| Name | Version |
| ---- | ------- |
| <a name="provider_aws"></a> [aws](#provider\_aws) | 6.57.1 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
Suppressed comments (5)
modules/aws-eks/variables.tf:237
- Changing the default Karpenter service account name is a backward-incompatible behavior change (workloads/IAM trust policies may be bound to the previous default). If this isn’t intentional, keep the prior default to avoid breaking existing installs.
default = "karpenter"
modules/aws-eks/variables.tf:243
- Changing the default Karpenter namespace is a backward-incompatible behavior change (it affects where the controller is expected to run and what IRSA subject is trusted). If this isn’t intentional, keep the prior default to avoid breaking existing installs.
default = "kube-system"
modules/aws-eks/addons_locals.tf:41
eks-pod-identity-agentwas removed from the module’s default addon set. This changes default cluster behavior (and the module examples still reference this addon). Consider restoring it inbase_addonsso existing consumers keep the prior defaults and users can still disable it viacluster_addons.
},
}
modules/aws-eks/outputs.tf:17
- The
account_idoutput returns a sentinel string whenECO_DRis enabled. That value is not a valid AWS account ID and can break downstream consumers that expect a real ID; prefer returningnullwhen disabled.
value = local.account_id_output
modules/aws-eks/.terraform-docs.yml:44
- This module’s
.terraform-docs.ymlsetslockfile: false, but other modules in the repo uselockfile: true(e.g.modules/aws-ecs/.terraform-docs.yml). Flipping this here can cause inconsistent docs generation behavior across modules; consider keeping it enabled for consistency.
lockfile: false
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Suppressed comments (7)
modules/aws-eks/variables.tf:243
- The default Karpenter namespace was changed to "kube-system". Changing this default is a breaking behavior change (it changes where Karpenter is expected to run and the namespace in IRSA/permissions assumptions). If the goal is ECO_DR only, reverting this default avoids unrelated drift for existing clusters.
default = "kube-system"
modules/aws-eks/variables.tf:237
- The default Karpenter service account name was changed to "karpenter". Changing defaults is a breaking interface change for existing consumers (it will change the IRSA subject and can force replacement of the service account/role bindings). If this isn’t an intentional breaking change, keep the previous default and let users override explicitly.
default = "karpenter"
modules/aws-eks/variables.tf:276
- New input variable name is uppercase (ECO_DR). This module otherwise uses lowercase snake_case variable names; introducing an all-caps input is inconsistent and can surprise consumers (e.g., TF_VAR naming expectations and docs/searchability). Consider renaming the input to something like "eco_dr" or "eco_dr_enabled" and updating references + docs in this PR to keep the public interface consistent.
variable "ECO_DR" {
modules/aws-eks/outputs.tf:18
output.account_idcan now return the literal string "disabled by ECO_DR", which contradicts the output description and can break downstream modules expecting a 12-digit account ID. Prefer returningnull(or the real account id) when ECO_DR is enabled so the output’s semantics remain consistent.
output "account_id" {
description = "AWS Account ID where the EKS cluster is deployed"
value = local.account_id_output
}
modules/aws-eks/outputs.tf:90
- The subnet list in the summary uses
coalesce(...)with list values. If the filtered data source returns an empty list (not null),coalescewill pick it and ignorevar.subnet_ids, resulting in an empty subnet list even when subnet_ids are provided.coalescelistis the correct helper here because it falls back when the first list is empty.
for subnet_key, subnet_value in zipmap(
range(length(coalesce(try(data.aws_subnets.filtered[0].ids, null), var.subnet_ids, []))),
coalesce(try(data.aws_subnets.filtered[0].ids, null), var.subnet_ids, [])
) :
modules/aws-eks/addons_locals.tf:38
eks-pod-identity-agentis no longer inbase_addons, which changes the module’s default addon set independently of ECO_DR. This can remove pod identity from existing deployments, and the module examples still reference this addon. If ECO_DR is the only intended behavior change, restore this addon inbase_addons(users can still disable it viacluster_addons).
aws-ebs-csi-driver = {
enabled = true
resolve_conflicts_on_create = "OVERWRITE"
resolve_conflicts_on_update = "OVERWRITE"
most_recent = "false"
modules/aws-eks/.terraform-docs.yml:44
.terraform-docs.ymlsetssettings.lockfile: false, but other modules in this repo keep terraform-docs lockfiles enabled. Disabling it here is inconsistent and can make README generation less reproducible across contributors/CI.
lockfile: false
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Suppressed comments (4)
modules/aws-eks/variables.tf:238
- The defaults for
karpenter_service_account_nameandkarpenter_namespace_namechanged (fromkarpenter-sa/karpentertokarpenter/kube-system). This is a potentially breaking change for existing users who relied on the previous defaults and is unrelated to the stated ECO_DR goal.
If this is intentional, it should be explicitly called out as a behavior change (and ideally paired with a migration note). If unintentional, consider reverting the defaults to preserve compatibility.
variable "karpenter_service_account_name" {
description = "Set the name of K8s service account karpenter controller"
type = string
default = "karpenter"
}
modules/aws-eks/addons_locals.tf:38
eks-pod-identity-agentis no longer present inbase_addons, which changes the module’s default addon set even whenECO_DRis disabled. The module examples still referenceeks-pod-identity-agent(e.g._examples/*/values.yaml), so removing it from defaults makes those examples misleading and can be a breaking behavior change for existing consumers.
If the intent of this PR is only to gate EKS creation for cold-standby DR, consider restoring eks-pod-identity-agent in base_addons (users can still disable it via cluster_addons).
aws-ebs-csi-driver = {
enabled = true
resolve_conflicts_on_create = "OVERWRITE"
resolve_conflicts_on_update = "OVERWRITE"
most_recent = "false"
modules/aws-eks/.terraform-docs.yml:44
settings.lockfileis set tofalsehere, while other modules in this repo keep terraform-docs lockfiles enabled (e.g.modules/aws-ecs/.terraform-docs.yml). Disabling the lockfile can make generated docs less deterministic and may cause CI/docs drift across contributors.
lockfile: false
modules/aws-eks/variables.tf:280
- This PR introduces an input variable named
ECO_DRin all-caps. It’s the only uppercase Terraform variable name in the repo, which makes the module interface inconsistent with the existingsnake_caseinputs and can be surprising for consumers (and forTF_VAR_...usage).
Consider renaming this to eco_dr (or eco_dr_enabled) and updating references accordingly.
variable "ECO_DR" {
description = "When set to true/1/ON, disables creation of EKS resources for cold standby DR workspaces. Defaults to false so normal workspaces create all configured resources."
type = any
default = false
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Suppressed comments (3)
modules/aws-eks/addons_locals.tf:38
eks-pod-identity-agentis no longer included inbase_addons, but the module’s examples still reference it (e.g._examples/*/values.yaml). This creates a surprising behavior change unrelated to ECO_DR and can cause example-driven installs to fail or drift from expectations. Re-add it to the default addon set (users can still disable it viacluster_addons).
aws-ebs-csi-driver = {
enabled = true
resolve_conflicts_on_create = "OVERWRITE"
resolve_conflicts_on_update = "OVERWRITE"
most_recent = "false"
modules/aws-eks/.terraform-docs.yml:44
settings.lockfileis set tofalsefor this module, while other modules in this repo consistently uselockfile: true. Disabling the lockfile can make README generation non-deterministic across terraform-docs versions and increase churn in autogenerated docs.
lockfile: false
modules/aws-eks/variables.tf:238
- The default Karpenter service account name is now
karpenter, but the bundled Karpenter example values still setserviceAccount.name: "karpenter-sa"(_examples/karpenter/karpenter_config/values.yaml). Unless callers overridekarpenter_service_account_name, this mismatch will break IRSA trust (role expects one SA name, Helm deploys another). Update the example (and/or defaults) so they stay aligned.
variable "karpenter_service_account_name" {
description = "Set the name of K8s service account karpenter controller"
type = string
default = "karpenter"
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 22 changed files in this pull request and generated no new comments.
Suppressed comments (2)
modules/aws-eks/variables.tf:284
- The validation uses
lower(tostring(var.ECO_DR)), which will error (not just fail validation) if a caller passesnull. Ifnullshould be treated as "false" (or at least produce a friendly validation error), coalesce it before converting to string.
validation {
condition = contains(["true", "false", "1", "0", "on", "off", "yes", "no"], lower(tostring(var.ECO_DR)))
error_message = "ECO_DR must be one of: true, false, 1, 0, ON, OFF, yes, no."
modules/aws-eks/main.tf:10
tostring(var.ECO_DR)will throw if callers passnull(common when passing through optional inputs), which would fail evaluation before you get a helpful validation error. Consider coercingnulltofalsebeforetostring()so ECO_DR behaves safely when unset/optional.
account_id = local.eco_dr_enabled ? "" : data.aws_caller_identity.current[0].account_id
account_id_output = local.eco_dr_enabled ? null : local.account_id
account_id_summary = local.eco_dr_enabled ? "disabled by ECO_DR" : local.account_id
eco_dr_enabled = contains(["true", "1", "on", "yes"], lower(tostring(var.ECO_DR)))
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 22 changed files in this pull request and generated 2 comments.
Suppressed comments (1)
modules/aws-eks/outputs.tf:57
coalesce(lookup(...), try(...))can error at plan time iflaunch_template_versionis a number in the node group input, becausecoalescerequires arguments of the same type. Since this value is embedded in a string anyway, casting to string before coalescing avoids type-mismatch failures.
- Launch template version: ${local.eco_dr_enabled ? "disabled by ECO_DR" : coalesce(lookup(node_group_value, "launch_template_version", null), try(module.eks[0].eks_managed_node_groups[node_group_key].launch_template_latest_version, "unknown"))}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 22 changed files in this pull request and generated no new comments.
Suppressed comments (3)
modules/aws-eks/main.tf:11
coalesce(var.ECO_DR, false)can throw a type-unification error whenECO_DRis provided as a non-bool value (e.g., numeric1or string"on"via TF_VAR_*). That would prevent planning/applying even though ECO_DR is meant to be flexible. Usetry(tostring(var.ECO_DR), "false")(or similar) instead ofcoalesceto safely handle null/strings/numbers.
account_id = local.eco_dr_enabled ? "" : data.aws_caller_identity.current[0].account_id
account_id_output = local.eco_dr_enabled ? null : local.account_id
account_id_summary = local.eco_dr_enabled ? "disabled by ECO_DR" : local.account_id
eco_dr_enabled = contains(["true", "1", "on", "yes"], lower(tostring(coalesce(var.ECO_DR, false))))
}
modules/aws-eks/variables.tf:284
- The validation uses
coalesce(var.ECO_DR, false), which can error whenECO_DRis set as a number/string (common with TF_VAR_* +type = any). That would maketerraform planfail before the ECO_DR guard can take effect. Prefer acan(tostring(...))+contains(...)validation so strings/numbers/bools work, null is allowed, and other types still fail with the validation error message.
validation {
condition = contains(["true", "false", "1", "0", "on", "off", "yes", "no"], lower(tostring(coalesce(var.ECO_DR, false))))
error_message = "ECO_DR must be one of: true, false, 1, 0, ON, OFF, yes, no."
}
modules/aws-eks/variables.tf:279
- This module’s variables appear to use lower_snake_case (e.g.,
cloudwatch_log_group_class,enabled_log_types). Introducing an all-caps input name (ECO_DR) is inconsistent with the existing public interface and makes it harder for consumers to discover/compose inputs (and impliesTF_VAR_ECO_DRinstead of Terraform’s usual lower-case TF_VAR_* usage). Consider renaming the input toeco_drand updating references/docs accordingly.
variable "ECO_DR" {
description = "Set via TF_VAR_ECO_DR when this module is the root module, or pass the value through from the root module when used as a child. When set to true, 1, ON, or yes, disables creation of EKS resources for cold standby DR workspaces. Enabling it in a state with existing EKS resources can cause Terraform to plan their destruction, including the EKS cluster, related IAM resources, and other managed resources. false, 0, OFF, and no keep normal resource creation enabled. Values are case-insensitive."
type = any
default = false
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 22 changed files in this pull request and generated no new comments.
Suppressed comments (2)
modules/aws-eks/variables.tf:283
- The validation uses
tostring(coalesce(var.ECO_DR, false)), which can fail for common TF_VAR use-cases (e.g.,TF_VAR_ECO_DR="true") becausecoalescerequires all arguments to be the same type. This makes the input harder to use and can fail before the validation message is shown. Prefer a null check +tostring()and addcan()so non-scalar types fail validation cleanly.
validation {
condition = contains(["true", "false", "1", "0", "on", "off", "yes", "no"], lower(tostring(coalesce(var.ECO_DR, false))))
error_message = "ECO_DR must be one of: true, false, 1, 0, ON, OFF, yes, no."
modules/aws-eks/main.tf:11
coalesce(var.ECO_DR, false)can still raise a type-unification error whenECO_DRis provided viaTF_VAR_ECO_DRas a string/number (e.g., "true"), becausecoalescerequires all arguments to be the same type. This can break planning even when trying to enable ECO_DR. Use a null check +tostring()(and let invalid types fail) instead ofcoalescehere.
account_id = local.eco_dr_enabled ? "" : data.aws_caller_identity.current[0].account_id
account_id_output = local.eco_dr_enabled ? null : local.account_id
account_id_summary = local.eco_dr_enabled ? "disabled by ECO_DR" : local.account_id
eco_dr_enabled = contains(["true", "1", "on", "yes"], lower(tostring(coalesce(var.ECO_DR, false))))
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 22 changed files in this pull request and generated no new comments.
Suppressed comments (2)
modules/aws-eks/variables.tf:280
ECO_DRis declared astype = any, which weakens type safety and makes the generated docs less clear. Since the value is immediately string-parsed anyway, usingstringkeeps the same behavior for env-var use while rejecting complex types early.
type = any
default = false
modules/aws-eks/variables.tf:284
- The new input is named
ECO_DR, which is the only all-caps variable name in this repository’s modules. This makes the module interface inconsistent with the existing snake_case inputs and forces callers to use an atypical attribute name in module blocks.
Consider renaming it to eco_dr (and documenting TF_VAR_eco_dr for root usage) to match the established module API style.
This issue also appears on line 278 of the same file.
variable "ECO_DR" {
description = "Set via TF_VAR_ECO_DR when this module is the root module, or pass the value through from the root module when used as a child. When set to true, 1, ON, or yes, disables creation of EKS resources for cold standby DR workspaces. Enabling it in a state with existing EKS resources can cause Terraform to plan their destruction, including the EKS cluster, related IAM resources, and other managed resources. false, 0, OFF, and no keep normal resource creation enabled. Values are case-insensitive."
type = any
default = false
validation {
condition = can(var.ECO_DR == null ? "false" : tostring(var.ECO_DR)) && contains(["true", "false", "1", "0", "on", "off", "yes", "no"], lower(var.ECO_DR == null ? "false" : tostring(var.ECO_DR)))
error_message = "ECO_DR must be one of: true, false, 1, 0, ON, OFF, yes, no."
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 22 changed files in this pull request and generated no new comments.
Suppressed comments (1)
modules/aws-eks/variables.tf:284
- The new input is named
ECO_DR(uppercase), but all other module inputs in this file use lower_snake_case. Uppercase variable names are unusual in Terraform modules and can make module usage inconsistent (includingTF_VAR_naming). Consider renaming this input toeco_dr(or similar) and updating references/docs accordingly before consumers start depending on the uppercase name.
variable "ECO_DR" {
description = "Set via TF_VAR_ECO_DR when this module is the root module, or pass the value through from the root module when used as a child. When set to true, 1, ON, or yes, disables creation of EKS resources for cold standby DR workspaces. Enabling it in a state with existing EKS resources can cause Terraform to plan their destruction, including the EKS cluster, related IAM resources, and other managed resources. false, 0, OFF, and no keep normal resource creation enabled. Values are case-insensitive."
type = string
default = "false"
validation {
condition = can(var.ECO_DR == null ? "false" : tostring(var.ECO_DR)) && contains(["true", "false", "1", "0", "on", "off", "yes", "no"], lower(var.ECO_DR == null ? "false" : tostring(var.ECO_DR)))
error_message = "ECO_DR must be one of: true, false, 1, 0, ON, OFF, yes, no."
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 22 changed files in this pull request and generated no new comments.
Suppressed comments (1)
modules/aws-eks/variables.tf:279
- This module’s inputs consistently use lower_snake_case (e.g.,
karpenter_namespace_name,create_auto_mode_iam_resources). Introducing an all-caps input name (ECO_DR) is a departure from the repository convention and may surprise callers (Terraform variable names are case-sensitive, soTF_VAR_eco_drwon’t work). Consider adding a conventionaleco_drinput (and treatingECO_DRas an alias/deprecated passthrough) so child-module consumers can follow the existing naming pattern while still supportingTF_VAR_ECO_DRfor root-module usage.
variable "ECO_DR" {
description = "Set via TF_VAR_ECO_DR when this module is the root module, or pass the value through from the root module when used as a child. When set to true, 1, ON, or yes, disables creation of EKS resources for cold standby DR workspaces. Enabling it in a state with existing EKS resources can cause Terraform to plan their destruction, including the EKS cluster, related IAM resources, and other managed resources. false, 0, OFF, and no keep normal resource creation enabled. Values are case-insensitive."
type = string
default = "false"
|
Given the test results, with ECO_DR enabled no resources are created for a new EKS cluster, since all of them depend on the EKS cluster itself, which incurs costs. Therefore, the implementation of the change for EKS ECO-DR is dismissed. |
Summary
Validation