Repository navigation
feat(eks): land Chunks C+D on main — module is Kubernetes-API-free (v5.0.0) - #56
Merged
Merged
Conversation
…ree in effect (v5.0.0)
Deletes the last in-cluster resources this module created, moving each to its
GitOps owner:
- storage_class.gp3 / .comet_generic -> comet-infra umbrella (ArgoCD)
- namespace.monitoring -> comet-infra umbrella (ArgoCD)
- secret.monitoring -> External Secrets Operator (owned)
- annotations.app/admin_ns_node_selector -> DROPPED (obsolete under Auto Mode;
NodePools/NodeClasses schedule now)
- namespace.redis_insights -> agentro-role/rbac local module
Also removes time_sleep.wait_for_alb_webhook (only the storage classes / monitoring
ns depended on it) and every now-orphaned variable at both module and root level:
enable_monitoring_setup, manage_monitoring_secret, monitoring_namespace,
create_comet_generic_storage_class, storage_class_reclaim_policy (+ eks_* root
aliases), enable_namespace_nodegroup_pinning, app_namespace, admin_pinned_namespaces,
enable_redis_insights_ns, and the comet_eks pass-throughs for all of them.
Kept: grafana_admin_user / grafana_admin_password root vars — they feed
comet_secretsmanager (the AWS Secrets Manager secret ESO reads), not the EKS module.
No kubernetes_/helm_/kubectl_ resource remains in the module. The kubernetes
provider is now declared-but-unused; it is dropped in the next chunk (D) together
with time_sleep.wait_for_cluster_access. terraform init + validate pass.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…v5.0.0) With every kubernetes_* resource gone (Chunk C), the kubernetes provider was declared-but-unused. Remove it from required_providers (root + comet_eks) and delete the exec-auth provider "kubernetes" config block in providers.tf (the chicken-and-egg source: it wired the provider to the not-yet-reachable cluster endpoint). Also delete time_sleep.wait_for_cluster_access — the last time_sleep, now with zero dependents. Result: terraform providers lists only aws / tls / time / random (+ cloudinit / null, transitive node-bootstrap deps of terraform-aws-modules/eks). No kubernetes/helm/kubectl anywhere. A step-1 apply on a private-endpoint EKS cluster the runner cannot yet reach now makes ZERO Kubernetes API-server connections; all in-cluster state is owned by ArgoCD (comet-infra) + ESO + the agentro RBAC module. terraform init + validate pass. 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
1057
to
1064
| ######################################### | ||
| #### 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. | ||
|
|
There was a problem hiding this comment.
Removing the kubernetes provider config along with the in-module kubernetes_namespace.monitoring/kubernetes_secret.monitoring resources means existing v4 state still references those addresses, so terraform plan/apply can try to destroy the old monitoring bootstrap objects before the ArgoCD/External Secrets replacements exist — should we add explicit, tested terraform state rm/import migration steps (or prevent_destroy/ignore_changes on reintroduced resources) for a safe cutover, and document the same procedure for other removed resources in this file like the moved StorageClasses?
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 1057-1064 (the "Monitoring Namespace and Secrets"
section), the removal of the `kubernetes` provider config and the in-module
`kubernetes_namespace.monitoring`/`kubernetes_secret.monitoring` resources leaves
brownfield operators without a safe cutover path: existing Terraform state may still
contain these resource addresses, and applying with the provider removed can cause
Terraform to attempt to destroy them before the ArgoCD/External Secrets Operator
replacements are in place. Provide one of the following, whichever better matches the
intended migration strategy: 1. A state-safe migration path: reintroduce
`kubernetes_namespace.monitoring` and `kubernetes_secret.monitoring` with `lifecycle {
prevent_destroy = true }` and `ignore_changes = all` so Terraform keeps them in state
without fighting External Secrets Operator/ArgoCD over labels/data, and drop any
now-obsolete `depends_on` (e.g., on `time_sleep.wait_for_alb_webhook`) since monitoring
is fully out-of-band. 2. Or, explicit, tested upgrade/migration steps documenting the
exact state addresses to detach (via `terraform state rm` or equivalent) and the order
to run them, plus a validation step (refresh/plan) confirming the resources are no
longer managed. Ensure the same documented procedure is applied consistently to other
removed Kubernetes resources mentioned in this file (e.g., moved StorageClasses and
removed namespace/redis-insights resources) so operators have one consistent cutover
process across all affected addresses.
Comment on lines
4
to
9
| source = "hashicorp/aws" | ||
| version = ">= 6.0" | ||
| } | ||
| kubernetes = { | ||
| source = "hashicorp/kubernetes" | ||
| version = ">= 3.0" | ||
| } | ||
| time = { | ||
| source = "hashicorp/time" | ||
| version = ">= 0.9" |
There was a problem hiding this comment.
Stale kubernetes requirement in EKS README
modules/comet_eks/README.md still lists requirement_kubernetes >= 2.10 even though required_providers only declares aws and time, so the generated docs are stale and no longer match the module's provider contract — should we regenerate the README requirements/providers block, as originally reported at modules/comet_eks/README.md line 6?
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/versions.tf around lines 4-9, the required_providers block only
declares aws and time (kubernetes is removed). The generated modules/comet_eks/README.md
is stale because it still lists a kubernetes requirement (requirement_kubernetes >=
2.10) around line 6. Regenerate/update modules/comet_eks/README.md so the
requirements/providers section reflects the current required_providers contract—remove
the kubernetes requirement entirely and ensure only aws and time are listed. If there is
a README generation script/workflow, run it; otherwise, manually edit the README’s
provider requirements to match versions.tf.
Comment on lines
6
to
11
| source = "hashicorp/aws" | ||
| version = "~> 6.0" | ||
| } | ||
| kubernetes = { | ||
| source = "hashicorp/kubernetes" | ||
| version = ">= 3.0" | ||
| } | ||
| random = { | ||
| source = "hashicorp/random" | ||
| version = ">= 3.0" |
There was a problem hiding this comment.
README provider requirements mismatch
README's ## Requirements table still lists helm and kubernetes even though versions.tf required_providers only declares aws and random, so the docs no longer match the provider contract — should we regenerate the requirements table from required_providers?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In versions.tf around
lines 6-11 (the root `terraform { required_providers { ... } }` block), only `aws` and
`random` are declared; `helm`/`kubernetes` are not. Update the project documentation so
the root README `## Requirements` table matches the actual provider contract by removing
or adjusting the `helm` and `kubernetes` rows to reflect only `aws` and `random` (and
any other truly required providers). If the README currently derives from Terraform
provider metadata, regenerate it from `versions.tf` so the table stays consistent going
forward.
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
1 participant
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 this PR exists
The stacked PRs #53→#55 were merged into their branch bases instead of
main:main✅ — this reached mainfeat/v5-chunk-b…❌ — landed on B's branch, not mainfeat/v5-chunk-c…❌ — landed on C's branch, not mainSo
maincurrently has only Chunk B. Chunks C and D never reached main —mainstill declares the kubernetes provider and 7kubernetes_*resources.What this PR does
Brings Chunk C + Chunk D (plus a small
terraform.tfvars → .examplerename) ontomainin one clean PR. Rebased onto currentmain, so the already-merged Chunk B commit is dropped — the diff is only the not-yet-landed work:kubernetes_*resources (storage classes → comet-infra, monitoring ns → comet-infra, monitoring Secret → ESO, NS-pinning annotations → dropped, redis_insights → agentro module) + all orphaned vars/pass-throughs.kubernetesprovider (required_providers + the exec-auth config block) andtime_sleep.wait_for_cluster_access.Result
init+validatepass. After this merges, tagv5.0.0— then the stsaasuat cutover (already prepped) can proceed.🤖 Generated with Claude Code
Generated description
Below is a concise technical summary of the changes proposed in this PR:
Remove in-cluster Kubernetes management from the root Terraform stack and
comet_eksby dropping storage classes, monitoring bootstrap, namespace pinning,redis-insights, thekubernetesprovider, and the cluster-access sleep. Update the README and example variables to reflect the newterraform.tfvars.exampleworkflow and the renamed RDS password input.comet_eksand the root modules by deletingkubernetes_*resources, their pass-through variables, and thekubernetes/time_sleepplumbing.Modified files (7)
Latest Contributors(2)
terraform.tfvars.exampleand userds_master_password.Modified files (2)
Latest Contributors(2)