Skip to content

feat(eks): remove all kubernetes_* resources (v5.0.0 Chunk C) #54

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
obezpalko merged 1 commit into from
Jul 31, 2026
Merged

feat(eks): remove all kubernetes_* resources (v5.0.0 Chunk C) #54

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
18 changes: 0 additions & 18 deletions main.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,10 +324,6 @@ module "comet_eks" {
external_secrets_iam_role_name_override = var.external_secrets_iam_role_name_override
secretsmanager_environment = var.secretsmanager_environment

# Storage class configuration
storage_class_reclaim_policy = var.eks_storage_class_reclaim_policy
create_comet_generic_storage_class = var.eks_create_comet_generic_storage_class

# Loki IRSA for S3 access
enable_loki = var.enable_loki_bucket
loki_s3_bucket_arn = var.enable_s3 && var.enable_loki_bucket ? module.comet_s3[0].comet_loki_bucket_arn : null
Expand All @@ -340,13 +336,6 @@ module "comet_eks" {
enable_cloudwatch_exporter = var.enable_cloudwatch_exporter
cloudwatch_exporter_iam_role_name_override = var.cloudwatch_exporter_iam_role_name_override

# Monitoring namespace and Grafana credentials
enable_monitoring_setup = var.enable_monitoring_setup
manage_monitoring_secret = var.manage_monitoring_secret
monitoring_namespace = var.monitoring_namespace
grafana_admin_user = var.grafana_admin_user
grafana_admin_password = var.grafana_admin_password

# EKS Auto Mode (mutually exclusive with Karpenter)
enable_auto_mode = var.eks_enable_auto_mode
auto_mode_node_pools = var.eks_auto_mode_node_pools
Expand All @@ -373,13 +362,6 @@ module "comet_eks" {
enable_ci_runners_eks_api_access = var.enable_ci_runners_eks_api_access
ci_runners_cidr = var.ci_runners_cidr

# Namespace nodegroup pinning
enable_namespace_nodegroup_pinning = var.enable_namespace_nodegroup_pinning
app_namespace = var.app_namespace
admin_pinned_namespaces = var.admin_pinned_namespaces

# Redis Insights namespace + agentro port-forward RBAC
enable_redis_insights_ns = var.enable_redis_insights_ns
}

module "comet_elasticache" {
Expand Down
188 changes: 15 additions & 173 deletions modules/comet_eks/main.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -730,93 +730,11 @@ resource "aws_iam_role_policy" "external_dns" {
policy = data.aws_iam_policy_document.external_dns[0].json
}

# Best-effort settle delay before this module creates Services/objects that a
# webhook might mutate.
#
# The AWS Load Balancer Controller is now deployed out-of-band by ArgoCD, NOT by
# this module, so its webhook readiness is OUTSIDE Terraform's dependency graph —
# there is no in-graph resource to wait on. We therefore cannot truly gate on the
# ALB webhook here; this is a fixed post-cluster-access delay only. Real ordering
# for anything that depends on the ALB webhook must be enforced ArgoCD-side (sync
# waves / health checks), not here. depends_on is on wait_for_cluster_access
# (which this module DOES own), not on eks_blueprints_addons (which no longer
# installs the controller).
resource "time_sleep" "wait_for_alb_webhook" {
count = var.eks_aws_load_balancer_controller ? 1 : 0

depends_on = [time_sleep.wait_for_cluster_access]
create_duration = "60s"
}

locals {
# Build tag specifications for EBS CSI driver
# Each tag needs to be a separate tagSpecification_N parameter with format "key=value"
# Note: common_tags passed from root module already includes Terraform=true and Environment tags
common_tags_list = [for k, v in var.common_tags : "${k}=${v}"]

# Base tags for gp3 storage class (only StorageClass identifier, other tags come from common_tags)
gp3_base_tags = ["StorageClass=gp3"]
gp3_all_tags = concat(local.gp3_base_tags, local.common_tags_list)
gp3_tag_params = { for idx, tag in local.gp3_all_tags : "tagSpecification_${idx + 1}" => tag }

# Base tags for comet-generic storage class (only StorageClass identifier, other tags come from common_tags)
comet_generic_base_tags = ["StorageClass=comet-generic"]
comet_generic_all_tags = concat(local.comet_generic_base_tags, local.common_tags_list)
comet_generic_tag_params = { for idx, tag in local.comet_generic_all_tags : "tagSpecification_${idx + 1}" => tag }
}

resource "kubernetes_storage_class" "gp3" {
depends_on = [time_sleep.wait_for_cluster_access]

metadata {
name = "gp3"
labels = var.common_tags
annotations = {
"storageclass.kubernetes.io/is-default-class" = "true"
}
}

storage_provisioner = "ebs.csi.aws.com"

parameters = merge(
{
type = "gp3"
# Optionally, set iops and throughput:
# iops = "3000"
# throughput = "125"
},
local.gp3_tag_params
)

reclaim_policy = var.storage_class_reclaim_policy
volume_binding_mode = "WaitForFirstConsumer"
allow_volume_expansion = true
}

resource "kubernetes_storage_class" "comet_generic" {
# Some deployments have comet-generic created by the comet-ml Helm chart
# (Helm/ArgoCD-owned). Set create_comet_generic_storage_class=false there so
# this module does not fight the chart over ownership of the same SC.
count = var.create_comet_generic_storage_class ? 1 : 0

depends_on = [time_sleep.wait_for_cluster_access]

metadata {
name = "comet-generic"
labels = var.common_tags
}

storage_provisioner = "ebs.csi.aws.com"

parameters = merge(
{ type = "gp3" },
local.comet_generic_tag_params
)

reclaim_policy = var.storage_class_reclaim_policy
volume_binding_mode = "WaitForFirstConsumer"
allow_volume_expansion = true
}
# StorageClasses (gp3 default + comet-generic) moved to the comet-infra umbrella
# chart (ArgoCD-owned) — see comet-devops-helm/charts/comet-infra. The former
# wait_for_alb_webhook settle delay went with them; ordering for ALB-webhook
# dependents is enforced ArgoCD-side (sync waves), not in Terraform. This module
# no longer touches the Kubernetes API for storage classes.
Comment on lines +733 to +737

@baz-reviewer baz-reviewer Bot Jul 31, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Terraform apply may destroy K8s SCs

modules/comet_eks/main.tf drops ownership of kubernetes_storage_class.gp3 and kubernetes_storage_class.comet_generic without any moved {} or other in-repo state cutover, so upgrades that still have them in Terraform state will plan to destroy them — should we add migration blocks here, and do the same for the monitoring and redis-insights namespace/Secret resources?

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
modules/comet_eks/main.tf, add Terraform state migration blocks for the resources
removed from config so upgrades don’t plan destructive changes. Around lines 733-819,
create `moved {}` (preferred) or `removed {}` blocks for kubernetes_storage_class.gp3
and kubernetes_storage_class.comet_generic, matching the correct old resource addresses
exactly as they existed in state. Around lines 1067-1183, do the same for
kubernetes_namespace.monitoring and kubernetes_secret.monitoring. Around lines
1522-1683, do the same for kubernetes_namespace.redis_insights. Follow the same
moved-block style/location already used for EKS addons in this file (lines ~612-635),
and map `to` addresses to where these objects are now managed in-repo
(comet-infra/comet-devops) or use `removed` if they’re now fully external.


#########################################
#### Cluster Autoscaler IRSA Role ####
Expand Down Expand Up @@ -1146,41 +1064,10 @@ module "cloudwatch_exporter_irsa_role" {
#########################################
#### Monitoring Namespace and Secrets ####
#########################################
resource "kubernetes_namespace" "monitoring" {
count = var.enable_monitoring_setup ? 1 : 0

metadata {
name = var.monitoring_namespace
}

depends_on = [
module.eks,
time_sleep.wait_for_alb_webhook
]
}

resource "kubernetes_secret" "monitoring" {
# Set manage_monitoring_secret = false where the monitoring Secret is owned by
# External Secrets Operator (ExternalSecret with creationPolicy: Owner). Letting
# Terraform also manage it causes a reconcile fight: TF strips ESO's labels and
# replaces the whole data map (dropping ESO-only keys) on every apply.
count = var.enable_monitoring_setup && var.manage_monitoring_secret ? 1 : 0

metadata {
name = "monitoring"
namespace = kubernetes_namespace.monitoring[0].metadata[0].name
}

data = {
grafana-admin-user = var.grafana_admin_user
grafana-admin-password = var.grafana_admin_password
}

type = "Opaque"
immutable = false

depends_on = [kubernetes_namespace.monitoring]
}
# The monitoring namespace moved to the comet-infra umbrella chart (ArgoCD-owned).
# The monitoring Secret is owned by External Secrets Operator (ExternalSecret with
# creationPolicy: Owner) — Terraform no longer creates it. Both are out of this
# module so it never touches the Kubernetes API for monitoring bootstrap.

#########################################
#### Karpenter Prerequisites ####
Expand Down Expand Up @@ -1626,58 +1513,13 @@ resource "aws_vpc_security_group_ingress_rule" "eks_api" {
#########################################
#### Namespace nodegroup pinning ####
#########################################
# Annotates namespaces with scheduler.alpha.kubernetes.io/node-selector to route
# every Pod admitted into the namespace onto a specific node group. Skips
# kube-system + monitoring (they host DaemonSets and must schedule everywhere).
# Patches in place — does NOT create the namespace; create via Helm first.

resource "kubernetes_annotations" "app_ns_node_selector" {
count = var.enable_namespace_nodegroup_pinning ? 1 : 0

depends_on = [time_sleep.wait_for_cluster_access]

api_version = "v1"
kind = "Namespace"
metadata {
name = coalesce(var.app_namespace, var.environment)
}
annotations = {
"scheduler.alpha.kubernetes.io/node-selector" = "nodegroup_name=comet"
}
force = true
}

resource "kubernetes_annotations" "admin_ns_node_selector" {
for_each = var.enable_namespace_nodegroup_pinning ? toset(var.admin_pinned_namespaces) : []

depends_on = [time_sleep.wait_for_cluster_access]

api_version = "v1"
kind = "Namespace"
metadata {
name = each.value
}
annotations = {
"scheduler.alpha.kubernetes.io/node-selector" = "nodegroup_name=admin"
}
force = true
}
# DROPPED. The scheduler.alpha.kubernetes.io/node-selector annotations pinned
# app/admin namespaces onto legacy managed node groups (nodegroup_name=…). Under
# EKS Auto Mode, scheduling is handled by NodePools/NodeClasses (comet-infra), so
# this in-cluster patching is obsolete and has been removed.

#########################################
#### Redis Insights namespace ####
#########################################
# Operational debug surface — provides a namespace for the redis-insights helm
# chart (installed by FRED-helm-apply) pinned to the admin NG.

resource "kubernetes_namespace" "redis_insights" {
count = var.enable_redis_insights_ns ? 1 : 0

depends_on = [time_sleep.wait_for_cluster_access]

metadata {
name = "redis-insights"
annotations = {
"scheduler.alpha.kubernetes.io/node-selector" = "nodegroup_name=admin"
}
}
}
# Moved to the agentro-role/rbac local module (comet-devops), which owns agentro's
# in-cluster objects in one place. Not created by this module anymore.
Comment on lines 1522 to +1525

@baz-reviewer baz-reviewer Bot Jul 31, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Redis Insights namespace not provisioned

redis-insights namespace creation is dropped from the EKS module and the root enable_redis_insights_ns pass-through, so nothing in-repo now guarantees the namespace exists before operator/port-forward workflows need its RBAC bindings — should we make the agentro-role/rbac replacement create redis-insights on the same enablement paths?

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
modules/comet_eks/main.tf around lines 1522-1525 (the “Redis Insights namespace”
section), the code now only contains a comment claiming the redis-insights namespace was
moved and is no longer created here. Fix this by checking the replacement
agentro-role/rbac local module in the comet-devops code (search for redis-insights
namespace/RBAC/port-forward bindings); if it does not create `namespace redis-insights`
(and any required Role/RoleBinding/etc.), add those resources there so the namespace
exists before any workflows rely on it. Also verify how `enable_redis_insights_ns` was
previously used and either (a) re-wire that enablement into the agentro-role/rbac
module, or (b) deliberately make creation unconditional and remove/adjust the old
variable without changing behavior unexpectedly. Finally, ensure there is Terraform
dependency ordering between the provisioning of redis-insights objects and whatever
module/workflow uses them (e.g., via explicit depends_on or module output dependencies).

115 changes: 13 additions & 102 deletions modules/comet_eks/variables.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -498,36 +498,10 @@ variable "byo_s3_irsa_roles" {
}
}

variable "enable_monitoring_setup" {
description = "Enable monitoring namespace and Grafana credentials secret"
type = bool
default = false
}

variable "manage_monitoring_secret" {
description = "When true (default), Terraform creates/manages the monitoring Grafana credentials Secret. Set false when External Secrets Operator owns that Secret (ExternalSecret with creationPolicy: Owner) so Terraform does not fight ESO over its labels and data. Only takes effect when enable_monitoring_setup = true."
type = bool
default = true
}

variable "monitoring_namespace" {
description = "Kubernetes namespace for monitoring resources"
type = string
default = "monitoring"
}

variable "grafana_admin_user" {
description = "Grafana admin username"
type = string
default = "admin"
}

variable "grafana_admin_password" {
description = "Grafana admin password"
type = string
sensitive = true
default = null
}
# Monitoring bootstrap (namespace + Grafana Secret) moved out of this module:
# namespace -> comet-infra umbrella (ArgoCD); Secret -> External Secrets Operator.
# Vars enable_monitoring_setup / manage_monitoring_secret / monitoring_namespace /
# grafana_admin_user / grafana_admin_password removed in v5.0.0.

# Druid Node Group Variables
variable "eks_druid_name" {
Expand Down Expand Up @@ -797,22 +771,9 @@ variable "karpenter_extra_tags" {
default = {}
}

variable "storage_class_reclaim_policy" {
description = "Reclaim policy for the gp3 and comet-generic StorageClasses. Use 'Retain' to preserve volumes after PVC deletion (recommended for production), or 'Delete' to automatically delete volumes."
type = string
default = "Retain"

validation {
condition = contains(["Retain", "Delete"], var.storage_class_reclaim_policy)
error_message = "Must be 'Retain' or 'Delete'."
}
}

variable "create_comet_generic_storage_class" {
description = "Create the comet-generic StorageClass. Set false when comet-generic is owned by the comet-ml Helm chart (Helm/ArgoCD), to avoid dual ownership. The gp3 StorageClass is always created by this module."
type = bool
default = true
}
# StorageClasses (gp3 + comet-generic) moved to the comet-infra umbrella chart
# (ArgoCD). Vars storage_class_reclaim_policy / create_comet_generic_storage_class
# removed in v5.0.0 — reclaim policy and comet-generic creation are chart values now.

# Per-Node-Group Subnet Pinning
# When set, restricts a specific node group to a subset of subnets (typically a
Expand Down Expand Up @@ -891,60 +852,10 @@ variable "ci_runners_cidr" {
}

#####################
#### Namespace nodegroup pinning — scheduler.alpha annotations
#### Namespace nodegroup pinning + Redis Insights — REMOVED in v5.0.0
#####################

variable "enable_namespace_nodegroup_pinning" {
description = <<-EOT
Annotate the application namespace with scheduler.alpha.kubernetes.io/node-selector=nodegroup_name=comet
and the admin_pinned_namespaces with nodegroup_name=admin. Skips kube-system and monitoring
(they host DaemonSets and need to schedule everywhere).

PREREQUISITE: kubernetes_annotations PATCHES an existing namespace — it does NOT create one.
The target namespaces must already exist before this toggle is enabled. The expected order is:

1. Apply terraform with enable_namespace_nodegroup_pinning = false
2. Run Helm (FRED-helm-apply / ArgoCD / chart install) — creates the namespaces
3. Apply terraform with enable_namespace_nodegroup_pinning = true

For brownfield customers (the typical migration path from a wrapper that already managed these
annotations), step 2 is already done; flipping the toggle on the next apply is safe.

For greenfield, attempting to apply with the toggle enabled before namespaces exist will fail
with "namespace ... not found" at apply time.
EOT
type = bool
default = false
}

variable "app_namespace" {
description = "Application namespace to pin to the comet node group. Defaults to the module environment (which matches the Helm chart's default namespace). Reserved namespaces (kube-system, kube-public, kube-node-lease, default, monitoring) are rejected — they host DaemonSets and must schedule freely."
type = string
default = null

validation {
condition = var.app_namespace == null || !contains(["kube-system", "kube-public", "kube-node-lease", "default", "monitoring"], coalesce(var.app_namespace, "unset"))
error_message = "app_namespace cannot be a reserved Kubernetes namespace (kube-system, kube-public, kube-node-lease, default, monitoring). Those host DaemonSets and must schedule across all nodes."
}
}

variable "admin_pinned_namespaces" {
description = "Namespaces to pin to the admin node group via scheduler.alpha annotations. Skipped if the namespace does not yet exist (annotation patches an existing namespace; create the namespace via Helm or terraform first). Reserved namespaces (kube-system, kube-public, kube-node-lease, default, monitoring) are rejected."
type = list(string)
default = ["cert-manager", "external-dns", "external-secrets"]

validation {
condition = length(setintersection(toset(var.admin_pinned_namespaces), toset(["kube-system", "kube-public", "kube-node-lease", "default", "monitoring"]))) == 0
error_message = "admin_pinned_namespaces cannot include reserved Kubernetes namespaces (kube-system, kube-public, kube-node-lease, default, monitoring). Those host DaemonSets and must schedule across all nodes."
}
}

#####################
#### Redis Insights — operational debug surface
#####################

variable "enable_redis_insights_ns" {
description = "Create the redis-insights Kubernetes namespace with scheduler.alpha annotation pinning to admin NG."
type = bool
default = false
}
# The scheduler.alpha node-selector annotations (enable_namespace_nodegroup_pinning,
# app_namespace, admin_pinned_namespaces) targeted legacy managed node groups and are
# obsolete under EKS Auto Mode (NodePools/NodeClasses handle scheduling). The
# redis-insights namespace (enable_redis_insights_ns) moved to the agentro-role/rbac
# local module. All four variables were removed with the resources they fed.
Loading