Switch regional and management clusters from EKS Auto Mode to OSS Karpenter#674
Open
theautoroboto wants to merge 14 commits into
Open
Switch regional and management clusters from EKS Auto Mode to OSS Karpenter#674theautoroboto wants to merge 14 commits into
theautoroboto wants to merge 14 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR migrates the cluster stack toward OSS Karpenter, adds bootstrap and validation logic for the new provisioning flow, updates Argo CD and TargetGroupBinding handling, and refreshes related documentation and maintenance scripts. ChangesKarpenter platform migration
Estimated code review effort: 5 (Critical) | ~120 minutes Argo CD and Load Balancer Controller wiring
Estimated code review effort: 3 (Moderate) | ~25 minutes Argo CD bootstrap node toleration
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
theautoroboto
added
the
tide/merge-method-squash
Contributor
Author
|
/test on-demand-e2e |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
terraform/modules/aws-load-balancer-controller/iam.tf (1)
34-317: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueBroad
Resource: "*"and unscoped read/create actions match the documented upstream policy.The remaining Checkov findings (CKV_AWS_355/CKV_AWS_290) on Describe*/Read actions and
iam:CreateServiceLinkedRolereflect AWS's own recommended AWS Load Balancer Controller policy, which inherently requires broad EC2/ELB describe permissions andResource: "*"for actions that don't support resource-level ARNs. This is expected and consistent with the upstream reference; consider adding inline#checkov:skip=CKV_AWS_355/CKV_AWS_290comments with justification so future scans don't need re-triage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@terraform/modules/aws-load-balancer-controller/iam.tf` around lines 34 - 317, The IAM policy in aws_lbc intentionally uses broad read and service-linked-role permissions that Checkov flags, so the fix is to document the exception rather than changing the policy. Add inline Checkov skip comments for CKV_AWS_355 and CKV_AWS_290 on the relevant statements in aws_iam_role_policy.aws_lbc, especially the Describe*/read blocks and the iam:CreateServiceLinkedRole statement, with a brief justification that this matches the upstream AWS Load Balancer Controller policy and requires Resource = "*".Source: Linters/SAST tools
argocd/config/regional-cluster/aws-load-balancer-controller/values.yaml (1)
12-14: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider dropping all Linux capabilities.
securityContextsetsallowPrivilegeEscalation: falseandreadOnlyRootFilesystem: truebut doesn't drop capabilities. The upstream chart ships this commented out by default too, so this matches defaults, but explicitly addingcapabilities.drop: ["ALL"]would further harden the pod per common CIS benchmarks.🛡️ Suggested hardening
securityContext: allowPrivilegeEscalation: false readOnlyRootFilesystem: true + capabilities: + drop: + - ALL🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@argocd/config/regional-cluster/aws-load-balancer-controller/values.yaml` around lines 12 - 14, The pod securityContext is missing an explicit capability drop, so harden the AWS Load Balancer Controller values by adding a capabilities.drop entry that removes all Linux capabilities alongside the existing allowPrivilegeEscalation and readOnlyRootFilesystem settings. Update the securityContext block in the regional-cluster/aws-load-balancer-controller values to include the capabilities field so the chart deployment uses a more restrictive runtime profile.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@argocd/config/management-cluster/eks-nodepool/templates/00-nodeclass.yaml`:
- Around line 1-18: The EC2NodeClass named fips is still selecting a standard
Bottlerocket AMI via the amiSelectorTerms alias, so update the node class to use
an explicit FIPS AMI selection method instead of the generic alias. Adjust the
EC2NodeClass template in 00-nodeclass.yaml to reference a FIPS-compliant
Bottlerocket image using tags, an SSM parameter, or a specific AMI ID while
keeping the existing cluster-specific role and selector logic intact.
In `@argocd/config/regional-cluster/eks-nodepool/templates/00-nodeclass.yaml`:
- Around line 1-18: The EC2NodeClass named fips is still selecting the default
Bottlerocket image via amiSelectorTerms, so update that selector to point to the
region-specific RHEL FIPS AMI instead. Adjust the amiSelectorTerms block in the
node class template so the fips NodeClass resolves the intended FIPS-compliant
image, keeping the existing clusterName-based role and selectors unchanged.
In `@terraform/modules/aws-load-balancer-controller/iam.tf`:
- Around line 152-206: The EC2 tag-related IAM conditions in the policy block
need to match the reference policy more closely. Update the EC2CreateTags
statement in the aws-load-balancer-controller IAM policy to guard on
aws:RequestTag/elbv2.k8s.aws/cluster instead of aws:RequestedRegion, and change
the EC2MutateTags Null condition for aws:RequestTag/elbv2.k8s.aws/cluster to
"true" so tag mutations are permitted as intended. Use the existing
EC2CreateTags and EC2MutateTags entries in iam.tf to make these adjustments.
In `@terraform/modules/ecs-bootstrap/main.tf`:
- Around line 165-182: The heredoc used in the bootstrap script is being
indented with spaces, so the current `kubectl apply -f - <<-PREWARM_EOF` form
will not terminate correctly. Update the heredoc in the bootstrap/user-data
block to use a quoted heredoc or remove the indentation so the `PREWARM_EOF`
delimiter is left-aligned, and keep this change near the `kubectl apply` prewarm
pod manifest.
In `@terraform/modules/eks-cluster/iam.tf`:
- Around line 194-266: The EC2Fleet and IAMInstanceProfile statements in the
Karpenter controller policy are still too broad because they allow mutating EC2
and instance profile APIs on wildcard resources. Update the IAM policy in the
EKS cluster IAM configuration so these actions are restricted using the same
tag-based conditions used by the EC2NodeClass and NodePool, while keeping the
Describe* permissions unchanged and leaving iam:PassRole scoped to
aws_iam_role.karpenter_node[0].arn.
In `@terraform/modules/eks-cluster/main.tf`:
- Around line 197-224: The karpenter bootstrap node group can be created before
the VPC CNI addon exists, which can prevent nodes from joining in time. Update
the aws_eks_node_group "karpenter_bootstrap" resource to explicitly wait on
aws_eks_addon.vpc_cni by adding it to depends_on, and keep kube-proxy out of the
gate so only the required addon blocks this bootstrap path.
---
Nitpick comments:
In `@argocd/config/regional-cluster/aws-load-balancer-controller/values.yaml`:
- Around line 12-14: The pod securityContext is missing an explicit capability
drop, so harden the AWS Load Balancer Controller values by adding a
capabilities.drop entry that removes all Linux capabilities alongside the
existing allowPrivilegeEscalation and readOnlyRootFilesystem settings. Update
the securityContext block in the regional-cluster/aws-load-balancer-controller
values to include the capabilities field so the chart deployment uses a more
restrictive runtime profile.
In `@terraform/modules/aws-load-balancer-controller/iam.tf`:
- Around line 34-317: The IAM policy in aws_lbc intentionally uses broad read
and service-linked-role permissions that Checkov flags, so the fix is to
document the exception rather than changing the policy. Add inline Checkov skip
comments for CKV_AWS_355 and CKV_AWS_290 on the relevant statements in
aws_iam_role_policy.aws_lbc, especially the Describe*/read blocks and the
iam:CreateServiceLinkedRole statement, with a brief justification that this
matches the upstream AWS Load Balancer Controller policy and requires Resource =
"*".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6bfad381-32e1-427b-82fb-8a331ff1d03c
📒 Files selected for processing (38)
Makefileargocd/config/management-cluster/eks-nodepool/templates/00-nodeclass.yamlargocd/config/management-cluster/eks-nodepool/templates/10-nodepool.yamlargocd/config/management-cluster/eks-nodepool/values.yamlargocd/config/management-cluster/hypershift/templates/05-job.yamlargocd/config/regional-cluster/aws-load-balancer-controller/Chart.yamlargocd/config/regional-cluster/aws-load-balancer-controller/values.yamlargocd/config/regional-cluster/eks-nodepool/templates/00-nodeclass.yamlargocd/config/regional-cluster/eks-nodepool/templates/10-nodepool.yamlargocd/config/regional-cluster/eks-nodepool/values.yamlargocd/config/regional-cluster/platform-api/templates/targetgroupbinding.yamlargocd/config/shared/argocd/values.yamlargocd/config/shared/storageclass/templates/gp3.yamlconfig/templates/argocd-bootstrap/applicationset.yaml.j2deploy/ephemeral/us-east-1/argocd-bootstrap-management-cluster/applicationset.yamldeploy/ephemeral/us-east-1/argocd-bootstrap-regional-cluster/applicationset.yamldeploy/integration/us-east-1/argocd-bootstrap-management-cluster/applicationset.yamldeploy/integration/us-east-1/argocd-bootstrap-regional-cluster/applicationset.yamldocs/design/gitops-cluster-configuration.mddocs/design/karpenter-node-provisioning.mddocs/design/rate-limiting-architecture.mdscripts/buildspec/register.shterraform/config/management-cluster/main.tfterraform/config/regional-cluster/main.tfterraform/modules/aws-load-balancer-controller/iam.tfterraform/modules/aws-load-balancer-controller/main.tfterraform/modules/aws-load-balancer-controller/outputs.tfterraform/modules/aws-load-balancer-controller/variables.tfterraform/modules/aws-load-balancer-controller/versions.tfterraform/modules/ecs-bootstrap/main.tfterraform/modules/ecs-bootstrap/variables.tfterraform/modules/eks-cluster/data.tfterraform/modules/eks-cluster/iam.tfterraform/modules/eks-cluster/locals.tfterraform/modules/eks-cluster/main.tfterraform/modules/eks-cluster/outputs.tfterraform/modules/eks-cluster/variables.tfterraform/modules/eks-cluster/versions.tf
Contributor
Author
|
/test on-demand-e2e |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
terraform/modules/eks-cluster/variables.tf (1)
84-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTemporary version pin lacks a tracking marker.
Per the commit history, this default was reverted from 1.13.0 to 1.5.0 to isolate a prewarm/IAM issue, with a planned re-bump once root-caused. Since there's no TODO/comment here, this temporary pin risks being forgotten.
📝 Suggested tracking comment
variable "karpenter_version" { description = "OSS Karpenter version string (e.g. \"1.5.0\"). Used to select the Helm chart version when deploying via ArgoCD." type = string + # TODO: temporarily reverted from 1.13.0 to isolate prewarm/IAM failure; re-bump once root cause is confirmed. default = "1.5.0" }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@terraform/modules/eks-cluster/variables.tf` around lines 84 - 88, The karpenter_version default in variable karpenter_version is a temporary rollback and needs a tracking marker so it is not forgotten. Add a clear TODO/comment рядом with the default value noting that it is intentionally pinned for the prewarm/IAM investigation and should be re-bumped once the issue is resolved, keeping the comment attached to the karpenter_version variable definition.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@terraform/modules/eks-cluster/variables.tf`:
- Around line 84-88: The karpenter_version default in variable karpenter_version
is a temporary rollback and needs a tracking marker so it is not forgotten. Add
a clear TODO/comment рядом with the default value noting that it is
intentionally pinned for the prewarm/IAM investigation and should be re-bumped
once the issue is resolved, keeping the comment attached to the
karpenter_version variable definition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1452293d-994b-47db-9261-bb6eae2019cb
📒 Files selected for processing (1)
terraform/modules/eks-cluster/variables.tf
theautoroboto
force-pushed
the
remove-auto-mode
branch
5 times, most recently
from
692aae2 to
f1eeb2e
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
argocd/config/management-cluster/eks-nodepool/templates/00-nodeclass.yaml (1)
7-8: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftFIPS NodeClass still selects a non-FIPS Bottlerocket AMI.
The
fipsEC2NodeClassresolves- alias: bottlerocket@latestto the standard Bottlerocket family. If this node class must remain FIPS-compliant, select the FIPS AMI explicitly (tags, SSM parameter, or AMI ID) rather than the generic alias.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@argocd/config/management-cluster/eks-nodepool/templates/00-nodeclass.yaml` around lines 7 - 8, The EC2NodeClass currently uses the generic Bottlerocket alias in amiSelectorTerms, which resolves to a non-FIPS AMI; update the fips node class definition to select a FIPS-compliant Bottlerocket image explicitly instead. Use the existing EC2NodeClass template and replace the alias-based selection with a FIPS-specific selector such as a matching tag, SSM parameter, or fixed AMI ID so the class consistently resolves to the intended FIPS image.argocd/config/regional-cluster/eks-nodepool/templates/00-nodeclass.yaml (1)
7-8: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftFIPS NodeClass still selects the default Bottlerocket image.
- alias: bottlerocket@latestpoints to the standard Bottlerocket AMI, so thefipsNodeClass launches non-FIPS nodes. PointamiSelectorTermsat the intended FIPS-compliant image (tags, SSM parameter, or AMI ID) to match the regional cluster's FIPS design.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@argocd/config/regional-cluster/eks-nodepool/templates/00-nodeclass.yaml` around lines 7 - 8, The FIPS NodeClass is still using the default Bottlerocket AMI selector, so update the amiSelectorTerms in the NodeClass template to point to the FIPS-compliant image instead of alias: bottlerocket@latest. Use the existing NodeClass/amiSelectorTerms block to switch to the correct FIPS AMI source, such as the intended tags, SSM parameter, or AMI ID, so the fips nodepool launches compliant nodes.
🧹 Nitpick comments (1)
terraform/config/management-cluster/main.tf (1)
70-72: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify null-to-empty-string fallback with
coalesce().The ternary pattern can be replaced with the built-in
coalesce()function for conciseness, sincecoalescetakes any number of arguments and returns the first one that isn't null or an empty string.♻️ Proposed refactor
- karpenter_controller_role_arn = module.management_cluster.karpenter_controller_role_arn != null ? module.management_cluster.karpenter_controller_role_arn : "" - karpenter_queue_url = module.management_cluster.karpenter_queue_url != null ? module.management_cluster.karpenter_queue_url : "" + karpenter_controller_role_arn = coalesce(module.management_cluster.karpenter_controller_role_arn, "") + karpenter_queue_url = coalesce(module.management_cluster.karpenter_queue_url, "")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@terraform/config/management-cluster/main.tf` around lines 70 - 72, Simplify the null-to-empty-string fallback in the management cluster configuration by replacing the explicit ternary checks on module.management_cluster.karpenter_controller_role_arn and module.management_cluster.karpenter_queue_url with coalesce(), so the locals/inputs use the first non-null, non-empty value directly. Update the assignments in the terraform main configuration where these karpenter_* values are set, keeping the same empty-string fallback behavior but using the built-in function for clarity and consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@argocd/config/management-cluster/eks-nodepool/templates/00-nodeclass.yaml`:
- Around line 7-8: The EC2NodeClass currently uses the generic Bottlerocket
alias in amiSelectorTerms, which resolves to a non-FIPS AMI; update the fips
node class definition to select a FIPS-compliant Bottlerocket image explicitly
instead. Use the existing EC2NodeClass template and replace the alias-based
selection with a FIPS-specific selector such as a matching tag, SSM parameter,
or fixed AMI ID so the class consistently resolves to the intended FIPS image.
In `@argocd/config/regional-cluster/eks-nodepool/templates/00-nodeclass.yaml`:
- Around line 7-8: The FIPS NodeClass is still using the default Bottlerocket
AMI selector, so update the amiSelectorTerms in the NodeClass template to point
to the FIPS-compliant image instead of alias: bottlerocket@latest. Use the
existing NodeClass/amiSelectorTerms block to switch to the correct FIPS AMI
source, such as the intended tags, SSM parameter, or AMI ID, so the fips
nodepool launches compliant nodes.
---
Nitpick comments:
In `@terraform/config/management-cluster/main.tf`:
- Around line 70-72: Simplify the null-to-empty-string fallback in the
management cluster configuration by replacing the explicit ternary checks on
module.management_cluster.karpenter_controller_role_arn and
module.management_cluster.karpenter_queue_url with coalesce(), so the
locals/inputs use the first non-null, non-empty value directly. Update the
assignments in the terraform main configuration where these karpenter_* values
are set, keeping the same empty-string fallback behavior but using the built-in
function for clarity and consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 162aa7e5-3550-450e-b58d-0b15e9fe0e15
📒 Files selected for processing (37)
Makefileargocd/config/management-cluster/eks-nodepool/templates/00-nodeclass.yamlargocd/config/management-cluster/eks-nodepool/templates/10-nodepool.yamlargocd/config/management-cluster/eks-nodepool/values.yamlargocd/config/management-cluster/hypershift/templates/05-job.yamlargocd/config/regional-cluster/aws-load-balancer-controller/Chart.yamlargocd/config/regional-cluster/aws-load-balancer-controller/values.yamlargocd/config/regional-cluster/eks-nodepool/templates/00-nodeclass.yamlargocd/config/regional-cluster/eks-nodepool/templates/10-nodepool.yamlargocd/config/regional-cluster/eks-nodepool/values.yamlargocd/config/regional-cluster/platform-api/templates/targetgroupbinding.yamlargocd/config/shared/argocd/values.yamlargocd/config/shared/storageclass/templates/gp3.yamlconfig/templates/argocd-bootstrap/applicationset.yaml.j2deploy/ephemeral/us-east-1/argocd-bootstrap-management-cluster/applicationset.yamldeploy/ephemeral/us-east-1/argocd-bootstrap-regional-cluster/applicationset.yamldeploy/integration/us-east-1/argocd-bootstrap-management-cluster/applicationset.yamldeploy/integration/us-east-1/argocd-bootstrap-regional-cluster/applicationset.yamldocs/design/gitops-cluster-configuration.mddocs/design/rate-limiting-architecture.mdscripts/buildspec/register.shterraform/config/management-cluster/main.tfterraform/config/regional-cluster/main.tfterraform/modules/aws-load-balancer-controller/iam.tfterraform/modules/aws-load-balancer-controller/main.tfterraform/modules/aws-load-balancer-controller/outputs.tfterraform/modules/aws-load-balancer-controller/variables.tfterraform/modules/aws-load-balancer-controller/versions.tfterraform/modules/ecs-bootstrap/main.tfterraform/modules/ecs-bootstrap/variables.tfterraform/modules/eks-cluster/data.tfterraform/modules/eks-cluster/iam.tfterraform/modules/eks-cluster/locals.tfterraform/modules/eks-cluster/main.tfterraform/modules/eks-cluster/outputs.tfterraform/modules/eks-cluster/variables.tfterraform/modules/eks-cluster/versions.tf
✅ Files skipped from review due to trivial changes (6)
- argocd/config/regional-cluster/aws-load-balancer-controller/Chart.yaml
- deploy/ephemeral/us-east-1/argocd-bootstrap-regional-cluster/applicationset.yaml
- terraform/modules/aws-load-balancer-controller/main.tf
- terraform/modules/aws-load-balancer-controller/versions.tf
- docs/design/rate-limiting-architecture.md
- docs/design/gitops-cluster-configuration.md
🚧 Files skipped from review as they are similar to previous changes (27)
- config/templates/argocd-bootstrap/applicationset.yaml.j2
- deploy/integration/us-east-1/argocd-bootstrap-management-cluster/applicationset.yaml
- deploy/integration/us-east-1/argocd-bootstrap-regional-cluster/applicationset.yaml
- argocd/config/regional-cluster/platform-api/templates/targetgroupbinding.yaml
- deploy/ephemeral/us-east-1/argocd-bootstrap-management-cluster/applicationset.yaml
- terraform/modules/eks-cluster/locals.tf
- argocd/config/management-cluster/eks-nodepool/values.yaml
- terraform/modules/aws-load-balancer-controller/variables.tf
- Makefile
- argocd/config/shared/storageclass/templates/gp3.yaml
- terraform/modules/aws-load-balancer-controller/outputs.tf
- terraform/modules/eks-cluster/data.tf
- terraform/modules/eks-cluster/versions.tf
- terraform/modules/ecs-bootstrap/variables.tf
- argocd/config/regional-cluster/eks-nodepool/values.yaml
- argocd/config/management-cluster/eks-nodepool/templates/10-nodepool.yaml
- argocd/config/management-cluster/hypershift/templates/05-job.yaml
- terraform/modules/eks-cluster/variables.tf
- argocd/config/shared/argocd/values.yaml
- argocd/config/regional-cluster/eks-nodepool/templates/10-nodepool.yaml
- terraform/modules/eks-cluster/outputs.tf
- terraform/config/regional-cluster/main.tf
- argocd/config/regional-cluster/aws-load-balancer-controller/values.yaml
- terraform/modules/aws-load-balancer-controller/iam.tf
- terraform/modules/eks-cluster/main.tf
- terraform/modules/eks-cluster/iam.tf
- terraform/modules/ecs-bootstrap/main.tf
Contributor
Author
|
/test on-demand-e2e |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/karpenter-node-provisioning.md`:
- Around line 140-150: The implementation note in the
karpenter-node-provisioning design doc is duplicated verbatim, which creates
markdownlint MD028 noise and can drift over time. Remove one of the repeated
blockquote paragraphs so only a single implementation note remains, keeping the
existing wording tied to the EC2NodeClass, amiSelectorTerms, amiFamily, and
nodeadm guidance.
In `@terraform/modules/ecs-bootstrap/README.md`:
- Line 67: Clarify the management_clusters README entry by replacing the vague
“Comma-separated colon-delimited” description with an explicit grammar and
example such as mc01:123456789012,mc02:210987654321. Update the documentation
text in the management_clusters table so readers can clearly see that entries
are comma-separated pairs using a colon between cluster name and account ID.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ebd3dff6-2373-4d63-b0a4-8b5b44e64dd6
📒 Files selected for processing (6)
docs/README.mddocs/design/fips-eks-compute.mddocs/design/karpenter-node-provisioning.mdterraform/modules/aws-load-balancer-controller/README.mdterraform/modules/ecs-bootstrap/README.mdterraform/modules/eks-cluster/README.md
✅ Files skipped from review due to trivial changes (3)
- docs/design/fips-eks-compute.md
- terraform/modules/aws-load-balancer-controller/README.md
- docs/README.md
theautoroboto
force-pushed
the
remove-auto-mode
branch
4 times, most recently
from
60a6fbf to
3f58005
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/buildspec/register.sh (1)
76-96: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHandle curl failures locally instead of disabling
errexit.
set +emakes every command in the polling block non-fatal. Keeperrexitenabled and handle only the expected curl failure path.As per path instructions,
scripts/**: “Review shell scripts for command injection vulnerabilities and proper error handling.”Proposed fix
-set +e MAX_RETRIES=80 RETRY_DELAY=30 RETRY_COUNT=0 LIVE_OK=false @@ - HTTP_CODE=$(curl -s -o /dev/null -w "%{http_code}" \ + if ! HTTP_CODE=$(curl -s -o /dev/null -w "%{http_code}" \ --connect-timeout 10 \ --max-time 30 \ --aws-sigv4 "aws:amz:${TARGET_REGION}:execute-api" \ --user "${AWS_ACCESS_KEY_ID}:${AWS_SECRET_ACCESS_KEY}" \ "${SECURITY_TOKEN_HEADER[@]}" \ - -X GET "$API_GATEWAY_URL/api/v0/live") + -X GET "$API_GATEWAY_URL/api/v0/live"); then + HTTP_CODE="000" + fi @@ -done -set -e +doneAlso applies to: 105-105
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/buildspec/register.sh` around lines 76 - 96, The polling loop in register.sh disables errexit with set +e, causing all commands in the block to become non-fatal; keep errexit enabled and handle only the expected curl failure path locally around the HTTP request in the retry loop. Update the logic around RETRY_COUNT, SECURITY_TOKEN_HEADER, and the curl invocation that sets HTTP_CODE so failures are captured and retried without masking unrelated errors.Sources: Path instructions, Linters/SAST tools
🧹 Nitpick comments (1)
argocd/config/management-cluster/hypershift/templates/05-job.yaml (1)
83-116: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo adjacent identical conditionals could be merged.
Lines 83-116 reopen
{{- if .Values.hypershift.externalDns.domain }}immediately after the block closed at line 82 with the same condition. Functionally fine (once the backslash issue above is fixed), but a single contiguousifblock spanning the install flags and the post-install DNS patching would be simpler to reason about.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@argocd/config/management-cluster/hypershift/templates/05-job.yaml` around lines 83 - 116, The HyperShift template has two back-to-back conditionals with the same .Values.hypershift.externalDns.domain guard, which should be merged into one contiguous block. Update the conditional around the install flags and the post-install DNS patching in 05-job.yaml so there is a single if/fi section spanning both behaviors, using the existing .Values.hypershift.externalDns.domain check and the DNS_ZONE_OPERATOR_ROLE_ARN patch logic together.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/validate-mc-k8s.sh`:
- Around line 115-137: The HyperShift CRD check is incorrectly piping kubectl
output into grep because stdout is redirected before grep runs, so HostedCluster
detection never works. Update the detection logic in validate-mc-k8s.sh around
the HostedCluster validation block to let kubectl api-resources output flow into
grep, then keep the existing HostedCluster loop and availability checks gated
behind that corrected condition. Use the kubectl api-resources / grep
HostedCluster check as the locating symbol, and apply the same fix to the other
referenced HostedCluster check block.
In `@scripts/validate-rc-aws.sh`:
- Around line 94-96: The AWS probe commands in validate-rc-aws.sh are still
allowed to abort the script under set -euo pipefail, so failures from aws eks
list-addons and the nodegroup health query should be handled explicitly instead
of terminating early. Update the addon_json assignment and the later
health-check logic to capture nonzero exit status, continue execution, and
record the validation as failed while still reaching the summary output. Keep
the fix localized around the addon_json and nodegroup-check sections so the
validator remains resilient to transient AWS CLI errors.
- Around line 223-239: The warning in the PLATFORM_API_TG_ARN fallback is
pointing users to the wrong ARN source: describe-target-health in
validate-rc-aws.sh expects a target group ARN, but the suggested kubectl
jsonpath reads the service’s load balancer ARN annotation. Update the warn
message near the PLATFORM_API_TG_ARN check to tell users how to obtain the
correct target group ARN instead, using the existing validate-rc-aws.sh target
health logic as the reference point.
In `@terraform/modules/eks-cluster/iam.tf`:
- Around line 373-386: The `aws_sqs_queue_policy.karpenter_interruption` policy
currently allows `events.amazonaws.com` to send messages without source
restrictions. Update the `Statement` in this resource to add conditions that
scope access to the existing `aws_cloudwatch_event_rule.karpenter[*].arn` values
via `aws:SourceArn` and to `data.aws_caller_identity.current.account_id` via
`aws:SourceAccount`, while keeping the `Sid`, `Principal`, and `sqs:SendMessage`
permissions aligned with the current `karpenter_interruption` queue setup.
---
Outside diff comments:
In `@scripts/buildspec/register.sh`:
- Around line 76-96: The polling loop in register.sh disables errexit with set
+e, causing all commands in the block to become non-fatal; keep errexit enabled
and handle only the expected curl failure path locally around the HTTP request
in the retry loop. Update the logic around RETRY_COUNT, SECURITY_TOKEN_HEADER,
and the curl invocation that sets HTTP_CODE so failures are captured and retried
without masking unrelated errors.
---
Nitpick comments:
In `@argocd/config/management-cluster/hypershift/templates/05-job.yaml`:
- Around line 83-116: The HyperShift template has two back-to-back conditionals
with the same .Values.hypershift.externalDns.domain guard, which should be
merged into one contiguous block. Update the conditional around the install
flags and the post-install DNS patching in 05-job.yaml so there is a single
if/fi section spanning both behaviors, using the existing
.Values.hypershift.externalDns.domain check and the DNS_ZONE_OPERATOR_ROLE_ARN
patch logic together.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: df3881f9-1928-4d9d-9248-578c79167100
📒 Files selected for processing (54)
Makefileargocd/config/management-cluster/eks-nodepool/templates/00-nodeclass.yamlargocd/config/management-cluster/eks-nodepool/templates/10-nodepool.yamlargocd/config/management-cluster/eks-nodepool/values.yamlargocd/config/management-cluster/hypershift/templates/05-job.yamlargocd/config/regional-cluster/aws-load-balancer-controller/Chart.yamlargocd/config/regional-cluster/aws-load-balancer-controller/values.yamlargocd/config/regional-cluster/eks-nodepool/templates/00-nodeclass.yamlargocd/config/regional-cluster/eks-nodepool/templates/10-nodepool.yamlargocd/config/regional-cluster/eks-nodepool/values.yamlargocd/config/regional-cluster/platform-api/templates/targetgroupbinding.yamlargocd/config/shared/argocd/values.yamlargocd/config/shared/storageclass/templates/gp3.yamlconfig/templates/argocd-bootstrap/applicationset.yaml.j2deploy/ephemeral/us-east-1/argocd-bootstrap-management-cluster/applicationset.yamldeploy/ephemeral/us-east-1/argocd-bootstrap-regional-cluster/applicationset.yamldeploy/integration/us-east-1/argocd-bootstrap-management-cluster/applicationset.yamldeploy/integration/us-east-1/argocd-bootstrap-regional-cluster/applicationset.yamldocs/README.mddocs/design/fips-eks-compute.mddocs/design/fully-private-eks-bootstrap.mddocs/design/gitops-cluster-configuration.mddocs/design/karpenter-node-provisioning.mddocs/design/logging-platform.mddocs/design/rate-limiting-architecture.mddocs/design/thanos-metrics-infrastructure.mddocs/design/zoa-trusted-actions.mdscripts/buildspec/register.shscripts/validate-mc-aws.shscripts/validate-mc-k8s.shscripts/validate-rc-aws.shscripts/validate-rc-k8s.shterraform/config/management-cluster/main.tfterraform/config/regional-cluster/imports.shterraform/config/regional-cluster/main.tfterraform/modules/api-gateway/outputs.tfterraform/modules/aws-load-balancer-controller/README.mdterraform/modules/aws-load-balancer-controller/iam.tfterraform/modules/aws-load-balancer-controller/main.tfterraform/modules/aws-load-balancer-controller/outputs.tfterraform/modules/aws-load-balancer-controller/variables.tfterraform/modules/aws-load-balancer-controller/versions.tfterraform/modules/ecs-bootstrap/README.mdterraform/modules/ecs-bootstrap/main.tfterraform/modules/ecs-bootstrap/variables.tfterraform/modules/eks-cluster/README.mdterraform/modules/eks-cluster/data.tfterraform/modules/eks-cluster/iam.tfterraform/modules/eks-cluster/locals.tfterraform/modules/eks-cluster/main.tfterraform/modules/eks-cluster/outputs.tfterraform/modules/eks-cluster/variables.tfterraform/modules/eks-cluster/versions.tfterraform/modules/hyperfleet-infrastructure/amazonmq.tf
✅ Files skipped from review due to trivial changes (17)
- deploy/integration/us-east-1/argocd-bootstrap-management-cluster/applicationset.yaml
- docs/design/fully-private-eks-bootstrap.md
- docs/design/rate-limiting-architecture.md
- argocd/config/management-cluster/eks-nodepool/values.yaml
- terraform/modules/aws-load-balancer-controller/versions.tf
- docs/README.md
- config/templates/argocd-bootstrap/applicationset.yaml.j2
- terraform/modules/hyperfleet-infrastructure/amazonmq.tf
- docs/design/logging-platform.md
- docs/design/karpenter-node-provisioning.md
- docs/design/zoa-trusted-actions.md
- docs/design/gitops-cluster-configuration.md
- terraform/modules/aws-load-balancer-controller/README.md
- docs/design/fips-eks-compute.md
- argocd/config/regional-cluster/platform-api/templates/targetgroupbinding.yaml
- terraform/modules/ecs-bootstrap/README.md
- docs/design/thanos-metrics-infrastructure.md
🚧 Files skipped from review as they are similar to previous changes (29)
- argocd/config/regional-cluster/eks-nodepool/values.yaml
- deploy/integration/us-east-1/argocd-bootstrap-regional-cluster/applicationset.yaml
- deploy/ephemeral/us-east-1/argocd-bootstrap-management-cluster/applicationset.yaml
- terraform/modules/eks-cluster/locals.tf
- deploy/ephemeral/us-east-1/argocd-bootstrap-regional-cluster/applicationset.yaml
- argocd/config/management-cluster/eks-nodepool/templates/10-nodepool.yaml
- terraform/modules/eks-cluster/versions.tf
- terraform/modules/aws-load-balancer-controller/main.tf
- Makefile
- terraform/modules/eks-cluster/data.tf
- argocd/config/regional-cluster/aws-load-balancer-controller/values.yaml
- terraform/modules/api-gateway/outputs.tf
- terraform/modules/eks-cluster/outputs.tf
- terraform/config/management-cluster/main.tf
- terraform/config/regional-cluster/imports.sh
- terraform/config/regional-cluster/main.tf
- argocd/config/shared/argocd/values.yaml
- terraform/modules/aws-load-balancer-controller/iam.tf
- terraform/modules/eks-cluster/README.md
- terraform/modules/eks-cluster/main.tf
- argocd/config/shared/storageclass/templates/gp3.yaml
- terraform/modules/aws-load-balancer-controller/variables.tf
- terraform/modules/ecs-bootstrap/main.tf
- argocd/config/regional-cluster/eks-nodepool/templates/10-nodepool.yaml
- scripts/validate-mc-aws.sh
- terraform/modules/aws-load-balancer-controller/outputs.tf
- terraform/modules/eks-cluster/variables.tf
- terraform/modules/ecs-bootstrap/variables.tf
- argocd/config/regional-cluster/aws-load-balancer-controller/Chart.yaml
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/validate-mc-k8s.sh`:
- Around line 276-283: The diagnostic output in the application health check
still prints a raw null message for conditions because the jq expression in the
kubectl get application path uses .message without a fallback. Update the
condition-printing jq filter in the health/degraded branch so it mirrors the
existing .health.message handling by defaulting missing condition messages to a
placeholder like "-" to avoid literal "null" in the output.
In `@scripts/validate-rc-k8s.sh`:
- Around line 235-241: The `_pia_ready` check in `validate-rc-k8s.sh` can still
be empty even when `kubectl get daemonset` succeeds, which makes the `[[
"$_pia_ready" -ge 1 ]]` comparison emit a bash unary-operator warning. Update
the `eks-pod-identity-agent` readiness logic to treat empty jsonpath output like
zero by normalizing `_pia_ready` before the numeric test, or by using a default
in the assignment, so the `pass`/`fail` branch remains the same without noisy
stderr output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b89ad365-99c5-48c6-bb52-f7d5adc2946e
📒 Files selected for processing (8)
config/templates/argocd-bootstrap/applicationset.yaml.j2deploy/ephemeral/us-east-1/argocd-bootstrap-management-cluster/applicationset.yamldeploy/ephemeral/us-east-1/argocd-bootstrap-regional-cluster/applicationset.yamldeploy/integration/us-east-1/argocd-bootstrap-management-cluster/applicationset.yamldeploy/integration/us-east-1/argocd-bootstrap-regional-cluster/applicationset.yamlscripts/validate-mc-k8s.shscripts/validate-rc-k8s.shterraform/modules/eks-cluster/iam.tf
🚧 Files skipped from review as they are similar to previous changes (1)
- terraform/modules/eks-cluster/iam.tf
Contributor
Author
|
/test on-demand-e2e |
2 similar comments
Contributor
Author
|
/test on-demand-e2e |
Contributor
Author
|
/test on-demand-e2e |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
argocd/config/management-cluster/hypershift/templates/05-job.yaml (1)
49-49: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winWait for the PrometheusRule CRD too. This gate only checks
servicemonitors.monitoring.coreos.com, buthypershift installalso appliesPrometheusRulemanifests. Add a wait forprometheusrules.monitoring.coreos.comas well so the Job doesn’t race the monitoring chart.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@argocd/config/management-cluster/hypershift/templates/05-job.yaml` at line 49, The wait gate in the hypershift Job only checks for the ServiceMonitor CRD, but the install flow also creates PrometheusRule resources, so the Job can still race the monitoring chart. Update the CRD readiness check in the job template to wait for prometheusrules.monitoring.coreos.com as well, alongside the existing servicemonitors.monitoring.coreos.com check, using the existing shell block in the job definition.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@argocd/config/management-cluster/hypershift/templates/05-job.yaml`:
- Around line 66-72: The wait loop only checks for the ServiceMonitor CRD, so
hypershift install can still race if PrometheusRule is not ready yet. Update the
readiness gate in the hypershift job template so the same wait block also
verifies prometheusrules.monitoring.coreos.com before proceeding, keeping the
existing kubectl get crd loop and waiting message aligned with the install
requirements.
---
Outside diff comments:
In `@argocd/config/management-cluster/hypershift/templates/05-job.yaml`:
- Line 49: The wait gate in the hypershift Job only checks for the
ServiceMonitor CRD, but the install flow also creates PrometheusRule resources,
so the Job can still race the monitoring chart. Update the CRD readiness check
in the job template to wait for prometheusrules.monitoring.coreos.com as well,
alongside the existing servicemonitors.monitoring.coreos.com check, using the
existing shell block in the job definition.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 73c029cf-959b-4d74-ac54-fadcd90ef5e1
📒 Files selected for processing (3)
argocd/config/management-cluster/hypershift/templates/05-job.yamlscripts/validate-mc-k8s.shscripts/validate-rc-k8s.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/validate-mc-k8s.sh
- scripts/validate-rc-k8s.sh
…troller Replaces EKS Auto Mode with OSS Karpenter for node provisioning and OSS AWS Load Balancer Controller for TargetGroupBinding CRDs on both RC and MC. Key changes: - eks-cluster module: Karpenter bootstrap node group, IRSA/Pod Identity for Karpenter and LBC, EC2NodeClaimTagging IAM statement for nodeclaim.tagging controller, CoreDNS/metrics-server/pod-identity addon ordering on bootstrap NG - ecs-bootstrap module: skip guard checks readyReplicas > 0 instead of deployment existence - eks-nodepool chart: EC2NodeClass + NodePool for both RC and MC - ApplicationSet: RespectIgnoreDifferences + ignoreDifferences for LBC webhook caBundle, TLS secret, and TargetGroupBinding status to prevent controller- managed fields from causing OutOfSync - loki, thanos, platform-api: TargetGroupBinding apiVersion updated from eks.amazonaws.com/v1 (Auto Mode) to elbv2.k8s.aws/v1beta1 (OSS LBC) - MQ: fix log group CloudWatch import ID; api-gateway: handle null invoke_url Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
theautoroboto
force-pushed
the
remove-auto-mode
branch
from
e2c3e96 to
3e0a87a
Compare
…r CRD race
Guard all external-dns install flags behind {{ if .Values.hypershift.externalDns.domain }}
so the job does not pass empty flags when external-dns is not configured.
Add a wait loop before hypershift install that blocks until
servicemonitors.monitoring.coreos.com is registered. The monitoring chart
(kube-prometheus-stack) deploys concurrently and may not have installed
Prometheus Operator CRDs before the install job runs; without the wait,
hypershift install exits non-zero causing the Job and ArgoCD app to be Degraded.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
validate-rc-k8s.sh and validate-mc-k8s.sh check nodes, Karpenter, LBC, core add-ons, platform services, HyperShift operator, and ArgoCD health on the Regional Cluster and Management Cluster respectively. ArgoCD Degraded diagnostic emits the app-level health message, conditions, and per-resource health for Missing/Unknown/Degraded resources. HyperShift CRD presence check uses 2>/dev/null (not &>/dev/null) so the grep receives kubectl stdout. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
theautoroboto
force-pushed
the
remove-auto-mode
branch
from
3e0a87a to
1a0fabb
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/validate-rc-k8s.sh`:
- Around line 113-128: The Karpenter placement check in the shell validation
script can incorrectly report PASS when no Karpenter pods are found because
kp_nodes is empty and off_bootstrap stays at 0. Update the validation logic
around the kp_nodes loop so pass "Karpenter pods scheduled on bootstrap node
group" only runs when at least one pod/node was observed, and otherwise emit a
warning or failure consistent with the earlier pod check. Use the existing
kp_nodes, off_bootstrap, and pass/warn flow to keep the behavior aligned with
the rest of validate-rc-k8s.sh.
- Around line 314-316: The ArgoCD diagnostics in the kubectl/jq pipeline
currently print literal null when a condition message is missing; update the
logic around the application status parsing to default absent messages to a
readable placeholder before formatting the output. Keep the change in the same
diagnostics block that uses kubectl get application and jq, and ensure the
printed condition line still shows the type while substituting a sensible
fallback for empty or null .message values.
In `@terraform/modules/eks-cluster/iam.tf`:
- Around line 4-10: The compute mode comment is out of date: it still says
var.enable_karpenter defaults to false even though the PR changes the default to
true. Update the documentation block in iam.tf near the enable_karpenter
description so the default branch and the listed role/policy behavior match the
current default, keeping the EKS Auto Mode vs OSS Karpenter summary aligned with
the actual variable behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8c4c89ac-7098-4fe6-aa69-2a5a5f0ce992
📒 Files selected for processing (45)
Makefileargocd/config/management-cluster/eks-nodepool/templates/00-nodeclass.yamlargocd/config/management-cluster/eks-nodepool/templates/10-nodepool.yamlargocd/config/management-cluster/eks-nodepool/values.yamlargocd/config/management-cluster/hypershift/templates/05-job.yamlargocd/config/regional-cluster/eks-nodepool/templates/00-nodeclass.yamlargocd/config/regional-cluster/eks-nodepool/templates/10-nodepool.yamlargocd/config/regional-cluster/eks-nodepool/values.yamlargocd/config/regional-cluster/loki/templates/targetgroupbinding.yamlargocd/config/regional-cluster/platform-api/templates/targetgroupbinding.yamlargocd/config/regional-cluster/thanos/templates/targetgroupbinding.yamlargocd/config/shared/argocd/values.yamlargocd/config/shared/storageclass/templates/gp3.yamlconfig/templates/argocd-bootstrap/applicationset.yaml.j2deploy/ephemeral/us-east-1/argocd-bootstrap-management-cluster/applicationset.yamldeploy/ephemeral/us-east-1/argocd-bootstrap-regional-cluster/applicationset.yamldeploy/integration/us-east-1/argocd-bootstrap-management-cluster/applicationset.yamldeploy/integration/us-east-1/argocd-bootstrap-regional-cluster/applicationset.yamldocs/README.mddocs/design/fips-eks-compute.mddocs/design/fully-private-eks-bootstrap.mddocs/design/gitops-cluster-configuration.mddocs/design/logging-platform.mddocs/design/rate-limiting-architecture.mddocs/design/thanos-metrics-infrastructure.mddocs/design/zoa-trusted-actions.mdscripts/buildspec/register.shscripts/validate-mc-k8s.shscripts/validate-rc-k8s.shterraform/config/management-cluster/main.tfterraform/config/regional-cluster/imports.shterraform/config/regional-cluster/main.tfterraform/modules/api-gateway/outputs.tfterraform/modules/ecs-bootstrap/README.mdterraform/modules/ecs-bootstrap/main.tfterraform/modules/ecs-bootstrap/variables.tfterraform/modules/eks-cluster/README.mdterraform/modules/eks-cluster/data.tfterraform/modules/eks-cluster/iam.tfterraform/modules/eks-cluster/locals.tfterraform/modules/eks-cluster/main.tfterraform/modules/eks-cluster/outputs.tfterraform/modules/eks-cluster/variables.tfterraform/modules/eks-cluster/versions.tfterraform/modules/hyperfleet-infrastructure/amazonmq.tf
✅ Files skipped from review due to trivial changes (12)
- argocd/config/regional-cluster/platform-api/templates/targetgroupbinding.yaml
- docs/README.md
- docs/design/rate-limiting-architecture.md
- terraform/modules/hyperfleet-infrastructure/amazonmq.tf
- argocd/config/management-cluster/eks-nodepool/values.yaml
- docs/design/fully-private-eks-bootstrap.md
- docs/design/logging-platform.md
- Makefile
- docs/design/gitops-cluster-configuration.md
- docs/design/zoa-trusted-actions.md
- terraform/modules/eks-cluster/README.md
- terraform/modules/ecs-bootstrap/README.md
🚧 Files skipped from review as they are similar to previous changes (27)
- argocd/config/management-cluster/eks-nodepool/templates/10-nodepool.yaml
- deploy/ephemeral/us-east-1/argocd-bootstrap-regional-cluster/applicationset.yaml
- argocd/config/regional-cluster/eks-nodepool/values.yaml
- deploy/ephemeral/us-east-1/argocd-bootstrap-management-cluster/applicationset.yaml
- argocd/config/regional-cluster/loki/templates/targetgroupbinding.yaml
- terraform/modules/eks-cluster/versions.tf
- terraform/modules/api-gateway/outputs.tf
- deploy/integration/us-east-1/argocd-bootstrap-regional-cluster/applicationset.yaml
- terraform/config/regional-cluster/imports.sh
- deploy/integration/us-east-1/argocd-bootstrap-management-cluster/applicationset.yaml
- argocd/config/regional-cluster/eks-nodepool/templates/10-nodepool.yaml
- terraform/modules/eks-cluster/variables.tf
- config/templates/argocd-bootstrap/applicationset.yaml.j2
- terraform/config/management-cluster/main.tf
- argocd/config/shared/storageclass/templates/gp3.yaml
- argocd/config/regional-cluster/thanos/templates/targetgroupbinding.yaml
- terraform/modules/eks-cluster/locals.tf
- terraform/modules/ecs-bootstrap/variables.tf
- docs/design/thanos-metrics-infrastructure.md
- argocd/config/management-cluster/hypershift/templates/05-job.yaml
- terraform/config/regional-cluster/main.tf
- terraform/modules/eks-cluster/data.tf
- argocd/config/shared/argocd/values.yaml
- terraform/modules/eks-cluster/outputs.tf
- terraform/modules/eks-cluster/main.tf
- docs/design/fips-eks-compute.md
- terraform/modules/ecs-bootstrap/main.tf
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/validate-mc-k8s.sh`:
- Around line 256-260: The ArgoCD application check in the validation script is
failing open because `kubectl get applications` errors are hidden and treated as
empty output. Update the `not_synced`/`total` logic in `validate-mc-k8s.sh` to
first capture the application list with an explicit `if ! apps=$(kubectl get
applications ...)` failure branch, and only run the `awk`/`grep` filtering when
the list was successfully retrieved. Make sure the `pass "All ${total} ArgoCD
applications Synced"` path is only reachable when the `kubectl` call succeeds
and applications are actually listed.
- Around line 43-56: The pods_running helper currently treats any pod with phase
Running or Completed as healthy, which misses pods that are still not ready.
Update pods_running to check readiness from kubectl output in addition to the
pod phase, and only return success when all matching pods are both Running and
fully READY. Keep the logic localized to pods_running and its kubectl get
pods/awk/grep pipeline so unhealthy add-ons are not reported as passing.
- Around line 167-174: The NodePool readiness check in validate-mc-k8s.sh should
guard against an unset .status.replicas before doing a numeric test, since the
current ready variable can be empty and trigger a bash integer-comparison
warning. Update the NodePool status handling around the kubectl jsonpath reads
and the pass/fail branch so ready defaults to 0 when missing, then keep the [[
"$ready" -ge 1 ]] comparison safe and deterministic.
- Around line 71-78: The node readiness check in validate-mc-k8s.sh is treating
a failed or empty kubectl get nodes result as success because the pipeline is
masked with || true. Update the readiness logic around not_ready/node_count so
kubectl failures are handled explicitly: capture the command status, fail if
node listing does not succeed, and only call pass in the success path when
kubectl returns nodes and all are Ready.
In `@scripts/validate-rc-k8s.sh`:
- Around line 74-80: The node readiness check in validate-rc-k8s.sh is treating
a failed kubectl get nodes call as success because stderr is discarded and the
empty result falls through to the “All nodes are Ready” branch. Update the
readiness logic around not_ready/node_count so kubectl failures are detected and
cause fail immediately, using the existing pass and fail helpers; only report
all nodes ready after a successful node listing, and avoid relying on an empty
not_ready value when the command itself did not succeed.
- Around line 290-294: The ArgoCD readiness check in the not_synced validation
block is failing open because both kubectl queries suppress errors, so missing
CRDs or RBAC failures can be reported as “All 0 applications Synced.” Update the
application listing logic to detect kubectl failures explicitly and treat them
as a hard failure instead of continuing to pass, and apply the same fail-closed
handling to the related application checks referenced by the not_synced and
total query flow. Use the existing pass/fail reporting helpers in
scripts/validate-rc-k8s.sh so any listing error is surfaced clearly rather than
masked by awk/grep/wc pipelines.
- Around line 45-58: The pods_running helper currently checks only pod phase via
kubectl get pods and awk, so it can miss pods that are Running but not Ready.
Update pods_running to verify container readiness using kubectl’s ready
status/condition (for example, inspect READY or a Ready condition) for the
matching pods, while keeping the existing namespace/selector handling intact.
Ensure the function still returns non-zero when no pods are found or when any
selected pod is not ready.
- Around line 90-96: The bootstrap taint check in validate-rc-k8s.sh is counting
taint entries instead of tainted nodes, so it can pass even when some bootstrap
nodes lack CriticalAddonsOnly. Update the kubectl/jq pipeline around taint_count
to count only nodes whose spec.taints includes that key, and compare that number
against the total bootstrap node count from the same label selector. Keep the
pass/warn logic in the same validation block, but base it on tainted nodes
versus all bootstrap nodes rather than raw taint occurrences.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4def09c8-aea6-4a1c-a152-b31d4ab21f93
📒 Files selected for processing (3)
argocd/config/management-cluster/hypershift/templates/05-job.yamlscripts/validate-mc-k8s.shscripts/validate-rc-k8s.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- argocd/config/management-cluster/hypershift/templates/05-job.yaml
- pods_running(): check READY column (e.g. 1/1) in addition to STATUS phase, so Running-but-not-Ready containers are caught - Node readiness: fail-closed on kubectl error instead of passing "All 0 nodes Ready" - RC taint check: count tainted nodes (not taint entries) to avoid double-counting - RC Karpenter placement: guard empty nodeName list to prevent false pass - MC NodePool replicas: default empty .status.replicas to 0 so arithmetic is safe - Condition diag: use // "-" fallback so null .message doesn't print literal "null" MC-6 (ArgoCD listing) already fixed in a prior commit. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Documents the architecture decision to use OSS Karpenter with a dedicated bootstrap node group instead of EKS Auto Mode, covering EC2NodeClass FIPS configuration, NodePool design, and pod scheduling constraints for bootstrap-critical workloads. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
EKS Auto Mode bundled LBC natively; OSS Karpenter does not. Without LBC, TargetGroupBinding CRDs are absent and platform-api stays OutOfSync/Missing. - terraform/modules/aws-load-balancer-controller: IAM role with Pod Identity association and the full LBC IAM policy (ec2/elasticloadbalancing permissions) - argocd/config/regional-cluster/aws-load-balancer-controller: Helm wrapper chart deploying the upstream aws-load-balancer-controller chart - scripts/validate-rc-aws.sh, validate-mc-aws.sh: AWS-level checks (node groups, load balancers, IAM role bindings) complementing the K8s-level scripts Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
Author
|
/retest |
Contributor
Author
|
/test on-demand-e2e |
Contributor
Author
|
/test images |
Hardcoded comparison against 2 breaks if the bootstrap node group is ever scaled. Compare tainted node count against the observed bootstrap_nodes value instead, and include the ratio in the message.
Karpenter-provisioned EC2 nodes are not in Terraform state. When terraform destroy reaches subnet/VPC deletion it fails with DependencyViolation because orphaned instances still hold ENIs. _drain_karpenter_nodes() runs after _purge_clusters() (so Karpenter can no longer reprovision) and before _run_teardown(). It finds all instances tagged karpenter.sh/managed-by=<cluster-name> for the RC and each MC in the region config, terminates them, and waits for termination before handing off to terraform destroy. Tracks ROSAENG-61588.
… destroy Karpenter-provisioned EC2 nodes are not in Terraform state. When the EKS cluster is deleted the Karpenter controller dies with it, leaving orphaned instances whose ENIs block VPC/subnet deletion with DependencyViolation. Adds a local-exec destroy provisioner to aws_eks_cluster.main. It runs immediately before the EKS API call to delete the cluster, finds all EC2 instances tagged karpenter.sh/managed-by=<cluster-name>, terminates them, and waits for completion. on_failure=continue ensures the destroy is not blocked if no instances are found or the CLI is unavailable. Applies to both RC and MC clusters via the shared eks-cluster module, covering all terraform destroy paths. Tracks ROSAENG-61588.
Terraform forbids referencing data sources in destroy-time provisioners. Extract the region from self.arn (arn:aws:eks:<region>:...) instead.
This reverts commit 3238753.
The API Gateway access log group is auto-created by AWS when logging is enabled. Import it so Terraform can manage retention and KMS encryption. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
karpenter.sh/managed-by was a v0.x tag. Karpenter v1.x nodes are tagged with karpenter.sh/nodeclaim (per node) and aws:eks:cluster-name (added by EKS). The old filter matched nothing, leaving nodes alive to block subnet deletion with DependencyViolation. New filter: aws:eks:cluster-name=<cluster> AND tag-key karpenter.sh/nodeclaim. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
Author
|
/test on-demand-e2e |
|
@theautoroboto: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
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.
Summary
Context
EKS Auto Mode is not available in GovCloud (us-gov-west-1, us-gov-east-1), which are required for FedRAMP Moderate delivery. This PR migrates both RC and MC cluster types to OSS Karpenter so the same codebase works across commercial and GovCloud regions.
Integration note: This cannot be applied to an existing Auto Mode cluster via `terraform apply`. The existing integration cluster must be decommissioned and a new one provisioned with `enable_karpenter=true` from the start. See ROSAENG-61244 for the full root cause analysis and conversion rationale.
What's included
Test plan
./scripts/validate-rc-k8s.sh— 21 PASS, 0 FAIL./scripts/validate-mc-k8s.sh— 8 PASS, 0 FAIL🤖 Generated with Claude Code
Summary by CodeRabbit