TESTING E2E: move Karpenter to ArgoCD with pure GitOps management - #733
TESTING E2E: move Karpenter to ArgoCD with pure GitOps management#733theautoroboto wants to merge 7 commits into
Conversation
Replaces EKS Auto Mode with OSS Karpenter for workload node provisioning on both Regional and Management Clusters. Karpenter is installed via Helm during bootstrap and adopted by ArgoCD via the eks-nodepool chart. Node pools are configured with FIPS-compliant AL2023 AMIs and CriticalAddonsOnly tolerations for bootstrap nodes. Key changes: - Remove EKS Auto Mode configuration; add Karpenter Helm install to ECS bootstrap with idempotency guard - Add Karpenter controller IAM role and SQS interruption queue via Terraform; move KMS CreateGrant to Karpenter controller role - Add EC2NodeClass and NodePool resources to eks-nodepool chart - Fix hypershift-install Job shell and add verbose exit code logging - Fix CSI Secret / Prometheus CRD chicken-and-egg in hypershift-install - Update monitoring test to use explicit ginkgo invocation with 15m timeout (Makefile target was timing out at 5m) - Add .claude/worktrees/ to .gitignore Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add extensive comments to Terraform code distinguishing between: - WHERE workloads run: karpenter-bootstrap managed node group (2× t3.medium) - HOW they get installed: ECS Fargate bootstrap task This addresses confusion about the bootstrap architecture. The ECS task is not a replacement for managed node groups - it's the installation mechanism that solves the chicken-and-egg problem of installing ArgoCD into a fully private cluster that Terraform cannot reach. Key clarifications: - terraform/modules/eks-cluster: karpenter-bootstrap node group is the runtime - terraform/modules/ecs-bootstrap: ECS task is the one-time installer - terraform/config/*-cluster: tie the two modules together in context The current architecture is correct: ECS provides the secure installation path for fully private clusters, while managed node groups provide the runtime capacity. See docs/design/fully-private-eks-bootstrap.md for full rationale. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Replace ECS bootstrap installation of Karpenter with ArgoCD Application
management, enabling full GitOps lifecycle (version control, drift detection,
self-healing). Remove imperative NodePool seeding in favor of sync wave
ordering.
This addresses reviewer feedback: "ECS task installs Karpenter via Helm but
there's no ArgoCD Application to manage it afterward. We have no version
management, no drift detection, no GitOps ownership."
Changes:
- Add argocd/config/{regional,management}-cluster/karpenter/ charts wrapping
upstream Karpenter OCI chart (v1.13.0)
- Remove Karpenter helm install and NodePool seeding from ECS bootstrap
- Add sync wave annotations: karpenter (wave 0) → eks-nodepool (wave 10)
- Inject Karpenter settings via ApplicationSet valuesObject:
- karpenter.settings.clusterName
- karpenter.settings.interruptionQueue
- karpenter.serviceAccount.annotations (IRSA role ARN)
- Override Karpenter Application namespace to kube-system
Before:
ECS bootstrap → helm install karpenter → seed NodePool → install ArgoCD
↓ (orphaned, no GitOps ownership)
ArgoCD adopts NodePool later (but not Karpenter controller)
After:
ECS bootstrap → install ArgoCD → done
ArgoCD (wave 0) → install Karpenter controller
ArgoCD (wave 10) → create NodePool (CRDs now exist)
(Pure GitOps, ArgoCD manages entire stack)
Benefits:
- Version management: Karpenter version pinned in Chart.yaml
- Drift detection: ArgoCD monitors and auto-heals configuration
- GitOps ownership: All changes via git commits and ArgoCD sync
- Simplified bootstrap: ECS removed 40+ lines of installation logic
- Guaranteed ordering: Sync waves ensure Karpenter CRDs exist before NodePool
- Consistent pattern: Same Application model as ArgoCD self-management
The karpenter-bootstrap managed node group (2× t3.medium) provides day-1
capacity for ArgoCD and Karpenter controller pods to run with CriticalAddonsOnly
tolerations. After bootstrap, Karpenter provisions all workload nodes.
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
[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 |
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThe pull request replaces EKS Auto Mode with OSS Karpenter. It adds Karpenter and AWS Load Balancer Controller infrastructure, updates private-cluster bootstrap sequencing, changes Kubernetes APIs, configures ArgoCD ordering, and updates platform, CI, Terraform, and design documentation. ChangesOSS Karpenter migration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Terraform
participant EKS
participant ECSBootstrap
participant ArgoCD
participant Karpenter
participant AWS
Terraform->>EKS: Create bootstrap node group and addons
Terraform->>AWS: Create Karpenter IAM, SQS, and EventBridge resources
ECSBootstrap->>ArgoCD: Install Karpenter and bootstrap applications
ArgoCD->>Karpenter: Sync Karpenter before eks-nodepool
Karpenter->>AWS: Provision and manage EC2 nodes
ArgoCD->>EKS: Apply node pools and platform applications
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
theautoroboto
changed the title
|
/test on-demand-e2e |
|
/test unit-tests |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
argocd/config/regional-cluster/aws-load-balancer-controller/values.yaml (1)
7-18: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd the bootstrap toleration to AWS Load Balancer Controller.
The bootstrap node group has
CriticalAddonsOnly:NoSchedule. The controller has no tolerations, and its Application syncs in wave 5 beforeeks-nodepoolin wave 10. Add theCriticalAddonsOnlyNoScheduletoleration used by Karpenter, or provision schedulable capacity before wave 5.🤖 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 7 - 18, Add the bootstrap node-group toleration to the AWS Load Balancer Controller configuration alongside its existing pod security settings: tolerate key CriticalAddonsOnly with effect NoSchedule, matching the toleration used by Karpenter so the controller can schedule before eks-nodepool.Source: Path instructions
🟡 Minor comments (10)
argocd/config/regional-cluster/monitoring/values.yaml-71-80 (1)
71-80: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRestore Prometheus Operator admission validation after bootstrap.
admissionWebhooks.enabled: falseremains applied by ArgoCD and suppresses the webhook configurations and certificate Jobs in kube-prometheus-stack 72.6.2. If this is bootstrap-only, use a bootstrap-specific override and restoreadmissionWebhooks.enabled: trueandprometheusOperator.tls.enabled: true.🤖 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/monitoring/values.yaml` around lines 71 - 80, Update the regional monitoring configuration so the bootstrap-only override disables admission webhooks and TLS only during bootstrap, then restores admissionWebhooks.enabled and prometheusOperator.tls.enabled to true afterward. Ensure the persistent values in the kube-prometheus-stack configuration do not leave either setting disabled under ArgoCD.Source: Path instructions
ci/e2e-tests.sh-186-191 (1)
186-191: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve the monitoring target behavior.
- Use
${RHOBS_API_URL:-}becauseset -uexits before Ginkgo can skip tests when the URL is unavailable.- Use Ginkgo v2.32.0 and preserve
--junit-reportand--output-dir; the direct command currently uses v2.28.1 and drops CI reports.- Remove the comment about a remote
--timeout=5m; the target does not set that flag.🤖 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 `@ci/e2e-tests.sh` around lines 186 - 191, Update the monitoring Ginkgo invocation in the E2E script to use E2E_RHOBS_API_URL="${RHOBS_API_URL:-}" so unset URLs do not trigger set -u termination, and align it with Ginkgo v2.32.0 while preserving the existing --junit-report and --output-dir options. Remove the inaccurate comment claiming the remote Makefile target uses --timeout=5m.Source: Path instructions
terraform/modules/ecs-bootstrap/README.md-77-77 (1)
77-77: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
karpenter_versiondefault in the inputs table is wrong.The table lists the default as
"", butterraform/modules/ecs-bootstrap/variables.tfsetsdefault = "1.13.0", and this README's own usage example on line 31 setskarpenter_version = "1.13.0". Correct the table entry to match the actual default.📝 Proposed fix
-| `karpenter_version` | Karpenter Helm chart version to install (e.g., `"1.13.0"`) | `string` | `""` | no | +| `karpenter_version` | Karpenter Helm chart version to install (e.g., `"1.13.0"`) | `string` | `"1.13.0"` | no |🤖 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/ecs-bootstrap/README.md` at line 77, Update the karpenter_version entry in the README inputs table to list "1.13.0" as its default, matching the variable definition and usage example.ci/ephemeral-provider/orchestrator.py-228-239 (1)
228-239: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winReplace
defaults.yamlfor--override-dir.The documented contract replaces
config/ephemeral/entirely, but_inject_ephemeral_configdeep-merges the existingdefaults.yaml. Omitted or removed keys therefore remain active. Copy the override file instead and add a regression test for removed keys.🤖 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 `@ci/ephemeral-provider/orchestrator.py` around lines 228 - 239, The _inject_ephemeral_config method must replace defaults.yaml from --override-dir rather than deep-merging it with the existing file, so omitted keys are removed and the override directory fully replaces config/ephemeral/. Update the defaults.yaml handling to copy the override file directly, and add a regression test covering removal of keys absent from the override.terraform/modules/rhobs-api-gateway/README.md-41-41 (1)
41-41: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse
elbv2.k8s.aws/v1beta1in the example.The documented example must match what operators can apply. Set
apiVersion: elbv2.k8s.aws/v1beta1, which is the storage version served by the AWS Load Balancer Controller.🤖 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/rhobs-api-gateway/README.md` at line 41, Update the apiVersion in the documented AWS Load Balancer example to elbv2.k8s.aws/v1beta1 so operators can apply the example using the controller’s served storage version.terraform/modules/eks-cluster/README.md-125-135 (1)
125-135: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the bootstrap sequence; ArgoCD now installs Karpenter.
Steps 4, 5, and 6 state that the ECS bootstrap task installs Karpenter, applies the FIPS
EC2NodeClassandNodePool, and prewarms a node. The PR objectives state the opposite: the bootstrap task installs ArgoCD and exits, and ArgoCD manages Karpenter at sync wave 0 andeks-nodepoolat wave 10. Readers of this module will follow an installation path that no longer exists.Rewrite the sequence to describe the ArgoCD-managed flow.
🤖 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/README.md` around lines 125 - 135, Update the bootstrap sequence in the module README to reflect that the ECS bootstrap task installs ArgoCD and exits, while ArgoCD installs Karpenter at sync wave 0 and manages the eks-nodepool resources at sync wave 10. Remove or revise the existing Karpenter installation, FIPS EC2NodeClass, and prewarm-node steps so the documented flow matches the current implementation.terraform/modules/bastion/log-collection-task.tf-83-85 (1)
83-85: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the duplicate resource entry.
Line 84 repeats
ec2nodeclasses.karpenter.k8s.aws, which line 61 already lists. The collector then inspects the same resource twice and the batch counter advances for no reason.♻️ Proposed fix
targetgroupbindings.elbv2.k8s.aws - ec2nodeclasses.karpenter.k8s.aws secretproviderclasses.secrets-store.csi.x-k8s.io🤖 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/bastion/log-collection-task.tf` around lines 83 - 85, Remove the duplicate ec2nodeclasses.karpenter.k8s.aws entry from the resource list, keeping the existing earlier entry and preserving the other resources.terraform/modules/eks-cluster/README.md-118-121 (1)
118-121: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse
EC2NodeClass.spec.instanceProfilein the output description.The
eks-nodepooltemplates setspec.instanceProfile, notspec.role. Update the description accordingly.📝 Proposed documentation fix
-| `karpenter_node_instance_profile_name` | Instance profile name for Karpenter-provisioned nodes (matches `EC2NodeClass.spec.role`) | +| `karpenter_node_instance_profile_name` | Instance profile name for Karpenter-provisioned nodes (used as `EC2NodeClass.spec.instanceProfile`) |🤖 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/README.md` around lines 118 - 121, Update the karpenter_node_instance_profile_name output description in the module README to reference EC2NodeClass.spec.instanceProfile instead of EC2NodeClass.spec.role, matching the eks-nodepool templates.argocd/config/regional-cluster/platform-api/templates/targetgroupbinding.yaml-2-2 (1)
2-2: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winUse
elbv2.k8s.aws/v1beta1for everyTargetGroupBinding.v1alpha1remains served, butv1beta1is the storage version. Update the platform-api, thanos, argocd, grafana, loki, and monitoring templates, plus theterraform/modules/rhobs-api-gateway/README.mdexample.🤖 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/platform-api/templates/targetgroupbinding.yaml` at line 2, Update the TargetGroupBinding apiVersion from elbv2.k8s.aws/v1alpha1 to elbv2.k8s.aws/v1beta1 in argocd/config/regional-cluster/platform-api/templates/targetgroupbinding.yaml, argocd/config/regional-cluster/thanos/templates/targetgroupbinding.yaml, and argocd/config/shared/argocd/templates/sre-targetgroupbinding.yaml, and update the corresponding example in terraform/modules/rhobs-api-gateway/README.md. Ensure every TargetGroupBinding uses the v1beta1 storage version.argocd/config/regional-cluster/aws-load-balancer-controller/Chart.yaml-7-10 (1)
7-10: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winOrder the generated Applications with Progressive Syncs. All regional Applications currently use sync wave
5, andsync-wavedoes not order Applications generated by anApplicationSet. ConfigureRollingSyncsoaws-load-balancer-controllerinstalls itsTargetGroupBindingCRD before consumer Applications. Keep Helm CRD installation enabled.🤖 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/Chart.yaml` around lines 7 - 10, Configure the ApplicationSet Progressive Syncs RollingSync strategy for the regional Applications so aws-load-balancer-controller is selected and completes before consumer Applications, using the existing ApplicationSet labels or generators to define the ordering. Preserve Helm CRD installation by keeping the chart’s CRD installation enabled.Source: Coding guidelines
🧹 Nitpick comments (3)
terraform/modules/eks-cluster/iam.tf (2)
114-120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a Checkov skip with justification, as done in the new LBC module.
Checkov reports CKV_AWS_355 for this policy. The
Resource = "*"statements cover EC2 and pricing describe actions that do not support resource-level ARNs, so the finding is expected.terraform/modules/aws-load-balancer-controller/iam.tfline 35 documents the same case with an inline skip. Use the same convention here to keep scan output clean and consistent.♻️ Proposed change
resource "aws_iam_role_policy" "karpenter_controller" { + `#checkov`:skip=CKV_AWS_355: EC2 describe and pricing actions do not support resource-level ARNs; mutating statements are scoped by kubernetes.io/cluster tag conditions. name = "karpenter-controller" role = aws_iam_role.karpenter_controller.id🤖 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/iam.tf` around lines 114 - 120, Add an inline Checkov skip for CKV_AWS_355 to the aws_iam_role_policy.karpenter_controller policy, matching the convention used by the aws-load-balancer-controller module. Include a justification that the wildcard resources are required for EC2 and pricing describe actions that do not support resource-level ARNs.Source: Linters/SAST tools
206-234: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winRemove the instance-profile lifecycle permissions or correct the comment at lines 68-70.
Lines 68-70 state that pre-creating the instance profile "removes the need for
iam:CreateInstanceProfilein the Karpenter controller policy". The policy still grantsiam:CreateInstanceProfile,iam:TagInstanceProfile,iam:AddRoleToInstanceProfile,iam:DeleteInstanceProfile, andiam:RemoveRoleFromInstanceProfile. If theEC2NodeClasssetsspec.instanceProfile, Karpenter does not manage profiles and these grants are unnecessary. If it setsspec.role, Karpenter needs them and the comment is wrong.Pick one model and make the code and comment agree.
As per coding guidelines: "Follow AWS IAM least-privilege practices for service roles."
🤖 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/iam.tf` around lines 206 - 234, Choose one instance-profile model and align both the IAM policy and the related comment: for a pre-created profile referenced by EC2NodeClass.spec.instanceProfile, remove the IAMInstanceProfileCreate and IAMInstanceProfileModify lifecycle permissions; otherwise retain the permissions and revise the comment to state that EC2NodeClass.spec.role requires Karpenter to manage profiles. Follow least-privilege practices and update the relevant policy blocks and comment consistently.Source: Coding guidelines
terraform/config/regional-cluster/main.tf (1)
182-196: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAlign ECS bootstrap documentation and inputs with ArgoCD ownership.
ECS bootstrap does not install Karpenter. Update the comments and README. Remove the unused
karpenter_queue_urlandkarpenter_versioninputs. Retainkarpenter_controller_role_arn, because ArgoCD uses it for Karpenter IRSA.🤖 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/regional-cluster/main.tf` around lines 182 - 196, Update the ECS bootstrap documentation near the regional_cluster module and the associated README to state that ECS bootstrap installs ArgoCD only, not Karpenter. Remove the unused karpenter_queue_url and karpenter_version inputs and their references, while retaining karpenter_controller_role_arn for ArgoCD’s Karpenter IRSA configuration.Sources: Coding guidelines, Path instructions
🤖 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 7-8: Update the EC2NodeClass amiSelectorTerms in both
argocd/config/management-cluster/eks-nodepool/templates/00-nodeclass.yaml lines
7-8 and argocd/config/regional-cluster/eks-nodepool/templates/00-nodeclass.yaml
lines 7-8 to select approved FIPS Bottlerocket AMI IDs or a supported custom SSM
parameter returning FIPS AMI IDs, replacing the standard bottlerocket@latest
alias.
In `@argocd/config/management-cluster/monitoring/values.yaml`:
- Around line 19-22: Update the monitoring pod tolerations in values.yaml so
both kube-state-metrics and prometheus.prometheusSpec also tolerate the
CriticalAddonsOnly=true:NoSchedule bootstrap-node taint, matching
prometheusOperator and allowing all required monitoring pods to schedule before
NodePools sync.
In `@docs/design/fips-eks-compute.md`:
- Around line 56-65: Align the Karpenter ownership and sync sequence across the
design documents: in docs/design/fips-eks-compute.md lines 56-65, replace the
pre-ArgoCD Karpenter/prewarm sequence with ECS bootstrap → ArgoCD, Karpenter at
wave 0, and eks-nodepool at wave 10; update lines 80-81 to remove pre-ArgoCD
prewarm validation, lines 84-85 to assign FIPS EC2NodeClass and NodePool
management to ArgoCD, and lines 129-132 to describe day-2 ownership and cluster
selection through the ArgoCD/ApplicationSet flow. In
docs/design/karpenter-node-provisioning.md lines 31-32, remove the ECS bootstrap
installation reference.
In `@docs/design/karpenter-node-provisioning.md`:
- Around line 7-10: Update the IRSA rationale in
docs/design/karpenter-node-provisioning.md at lines 7-10 and 27-30 to describe
IRSA as a deliberate repository choice, state that EKS Pod Identity is the ZOA
platform standard while IRSA remains supported, remove admission-webhook and
“predates” claims, and remove “upstream-recommended” wording; update
docs/design/zoa-trusted-actions.md at line 825 consistently, noting AWS
recommends EKS Pod Identity for new workloads.
In `@scripts/buildspec/register.sh`:
- Around line 67-71: Update the readiness loop around the live-endpoint curl and
retry sleep to use a 40-minute wall-clock deadline rather than only MAX_RETRIES
* RETRY_DELAY. Track elapsed or remaining time, cap each curl’s --max-time and
the final sleep to the remaining deadline, and ensure the loop exits when the
deadline is reached while preserving the existing success and failure handling.
In `@scripts/verify-fips.sh`:
- Around line 130-132: Update the CRD check around kubectl get crd
nodeclasses.karpenter.k8s.aws to distinguish a missing CRD from a failed
Kubernetes API query: capture the command result or output without suppressing
its status, skip only when the query succeeds and confirms the CRD is absent,
and fail verification when kubectl returns an API or authorization error.
In `@terraform/modules/aws-load-balancer-controller/iam.tf`:
- Around line 210-252: Add elasticloadbalancing:AddTags to the IAM policy for
tag-on-create operations, conditioned on elasticloadbalancing:CreateAction
matching CreateTargetGroup or CreateLoadBalancer. Update the ELBCreateTagged
statement or add a dedicated statement, while preserving ELBMutateTags for
non-create tag mutations.
In `@terraform/modules/ecs-bootstrap/README.md`:
- Around line 35-46: Update the “Bootstrap Sequence” section to remove the
obsolete Karpenter installation, EC2NodeClass/NodePool application, and prewarm
validation steps. Document that ECS bootstrap installs ArgoCD and exits, while
ArgoCD subsequently installs Karpenter and the eks-nodepool Application through
sync waves.
In `@terraform/modules/eks-cluster/iam.tf`:
- Around line 339-352: Update the aws_sqs_queue_policy.karpenter_interruption
policy to restrict EventBridge sends with aws:SourceAccount set to
data.aws_caller_identity.current.account_id and aws:SourceArn covering the four
aws_cloudwatch_event_rule.karpenter ARNs, and add an explicit deny for non-TLS
access using aws:SecureTransport. Preserve the existing AllowEventBridge send
permissions while scoping them to the account and rules.
In `@terraform/modules/eks-cluster/main.tf`:
- Around line 130-146: Update the EC2 instance filter in the Karpenter
termination script to use the Kubernetes cluster tag key
`kubernetes.io/cluster/$CLUSTER_NAME` with value `owned` alongside the existing
`karpenter.sh/nodeclaim` filter. Remove the `aws:eks:cluster-name` filter while
preserving the existing state filter and termination flow.
- Around line 223-231: Add a top-level tolerations entry to the Secrets Store
CSI provider’s configuration_values, targeting the CriticalAddonsOnly taint with
operator "Exists" and effect "NoSchedule"; do not alter the existing EKS
node-group taint or discovery tags.
---
Outside diff comments:
In `@argocd/config/regional-cluster/aws-load-balancer-controller/values.yaml`:
- Around line 7-18: Add the bootstrap node-group toleration to the AWS Load
Balancer Controller configuration alongside its existing pod security settings:
tolerate key CriticalAddonsOnly with effect NoSchedule, matching the toleration
used by Karpenter so the controller can schedule before eks-nodepool.
---
Minor comments:
In `@argocd/config/regional-cluster/aws-load-balancer-controller/Chart.yaml`:
- Around line 7-10: Configure the ApplicationSet Progressive Syncs RollingSync
strategy for the regional Applications so aws-load-balancer-controller is
selected and completes before consumer Applications, using the existing
ApplicationSet labels or generators to define the ordering. Preserve Helm CRD
installation by keeping the chart’s CRD installation enabled.
In `@argocd/config/regional-cluster/monitoring/values.yaml`:
- Around line 71-80: Update the regional monitoring configuration so the
bootstrap-only override disables admission webhooks and TLS only during
bootstrap, then restores admissionWebhooks.enabled and
prometheusOperator.tls.enabled to true afterward. Ensure the persistent values
in the kube-prometheus-stack configuration do not leave either setting disabled
under ArgoCD.
In
`@argocd/config/regional-cluster/platform-api/templates/targetgroupbinding.yaml`:
- Line 2: Update the TargetGroupBinding apiVersion from elbv2.k8s.aws/v1alpha1
to elbv2.k8s.aws/v1beta1 in
argocd/config/regional-cluster/platform-api/templates/targetgroupbinding.yaml,
argocd/config/regional-cluster/thanos/templates/targetgroupbinding.yaml, and
argocd/config/shared/argocd/templates/sre-targetgroupbinding.yaml, and update
the corresponding example in terraform/modules/rhobs-api-gateway/README.md.
Ensure every TargetGroupBinding uses the v1beta1 storage version.
In `@ci/e2e-tests.sh`:
- Around line 186-191: Update the monitoring Ginkgo invocation in the E2E script
to use E2E_RHOBS_API_URL="${RHOBS_API_URL:-}" so unset URLs do not trigger set
-u termination, and align it with Ginkgo v2.32.0 while preserving the existing
--junit-report and --output-dir options. Remove the inaccurate comment claiming
the remote Makefile target uses --timeout=5m.
In `@ci/ephemeral-provider/orchestrator.py`:
- Around line 228-239: The _inject_ephemeral_config method must replace
defaults.yaml from --override-dir rather than deep-merging it with the existing
file, so omitted keys are removed and the override directory fully replaces
config/ephemeral/. Update the defaults.yaml handling to copy the override file
directly, and add a regression test covering removal of keys absent from the
override.
In `@terraform/modules/bastion/log-collection-task.tf`:
- Around line 83-85: Remove the duplicate ec2nodeclasses.karpenter.k8s.aws entry
from the resource list, keeping the existing earlier entry and preserving the
other resources.
In `@terraform/modules/ecs-bootstrap/README.md`:
- Line 77: Update the karpenter_version entry in the README inputs table to list
"1.13.0" as its default, matching the variable definition and usage example.
In `@terraform/modules/eks-cluster/README.md`:
- Around line 125-135: Update the bootstrap sequence in the module README to
reflect that the ECS bootstrap task installs ArgoCD and exits, while ArgoCD
installs Karpenter at sync wave 0 and manages the eks-nodepool resources at sync
wave 10. Remove or revise the existing Karpenter installation, FIPS
EC2NodeClass, and prewarm-node steps so the documented flow matches the current
implementation.
- Around line 118-121: Update the karpenter_node_instance_profile_name output
description in the module README to reference EC2NodeClass.spec.instanceProfile
instead of EC2NodeClass.spec.role, matching the eks-nodepool templates.
In `@terraform/modules/rhobs-api-gateway/README.md`:
- Line 41: Update the apiVersion in the documented AWS Load Balancer example to
elbv2.k8s.aws/v1beta1 so operators can apply the example using the controller’s
served storage version.
---
Nitpick comments:
In `@terraform/config/regional-cluster/main.tf`:
- Around line 182-196: Update the ECS bootstrap documentation near the
regional_cluster module and the associated README to state that ECS bootstrap
installs ArgoCD only, not Karpenter. Remove the unused karpenter_queue_url and
karpenter_version inputs and their references, while retaining
karpenter_controller_role_arn for ArgoCD’s Karpenter IRSA configuration.
In `@terraform/modules/eks-cluster/iam.tf`:
- Around line 114-120: Add an inline Checkov skip for CKV_AWS_355 to the
aws_iam_role_policy.karpenter_controller policy, matching the convention used by
the aws-load-balancer-controller module. Include a justification that the
wildcard resources are required for EC2 and pricing describe actions that do not
support resource-level ARNs.
- Around line 206-234: Choose one instance-profile model and align both the IAM
policy and the related comment: for a pre-created profile referenced by
EC2NodeClass.spec.instanceProfile, remove the IAMInstanceProfileCreate and
IAMInstanceProfileModify lifecycle permissions; otherwise retain the permissions
and revise the comment to state that EC2NodeClass.spec.role requires Karpenter
to manage profiles. Follow least-privilege practices and update the relevant
policy blocks and comment consistently.
🪄 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: 1d650289-ae57-4368-971b-d916576bb750
📒 Files selected for processing (79)
.gitignoreMakefileargocd/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/management-cluster/karpenter/Chart.yamlargocd/config/management-cluster/karpenter/templates/.gitkeepargocd/config/management-cluster/karpenter/values.yamlargocd/config/management-cluster/monitoring/values.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/grafana/templates/sre-targetgroupbinding.yamlargocd/config/regional-cluster/karpenter/Chart.yamlargocd/config/regional-cluster/karpenter/templates/.gitkeepargocd/config/regional-cluster/karpenter/values.yamlargocd/config/regional-cluster/loki/templates/targetgroupbinding.yamlargocd/config/regional-cluster/monitoring/templates/sre-targetgroupbinding.yamlargocd/config/regional-cluster/monitoring/values.yamlargocd/config/regional-cluster/platform-api/templates/targetgroupbinding.yamlargocd/config/regional-cluster/thanos/templates/targetgroupbinding.yamlargocd/config/shared/argocd/templates/sre-targetgroupbinding.yamlargocd/config/shared/argocd/values.yamlargocd/config/shared/storageclass/templates/gp3.yamlci/e2e-tests.shci/ephemeral-provider/__init__.pyci/ephemeral-provider/orchestrator.pyconfig/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/karpenter-node-provisioning.mddocs/design/logging-platform.mddocs/design/thanos-metrics-infrastructure.mddocs/design/zoa-trusted-actions.mdscripts/buildspec/bootstrap-argocd-mc.shscripts/buildspec/provision-infra-mc.shscripts/buildspec/provision-infra-rc.shscripts/buildspec/register.shscripts/verify-fips.shterraform/config/management-cluster/main.tfterraform/config/pipeline-management-cluster/main.tfterraform/config/pipeline-regional-cluster/main.tfterraform/config/regional-cluster/imports.shterraform/config/regional-cluster/main.tfterraform/modules/api-gateway/alb.tfterraform/modules/api-gateway/variables.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/bastion/log-collection-task.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/elasticache-valkey/main.tfterraform/modules/elasticache-valkey/variables.tfterraform/modules/rhobs-api-gateway/README.mdterraform/modules/rhobs-api-gateway/alb.tfterraform/modules/rhobs-api-gateway/variables.tfterraform/modules/sre-ui-alb/alb.tfterraform/modules/sre-ui-alb/variables.tf
| amiSelectorTerms: | ||
| - alias: bottlerocket@latest |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -i -C 4 \
'FIPS|fips|bottlerocket|amiSelectorTerms|ami_kms_key_arn' \
docs/design/fips-eks-compute.md terraform/modules/eks-cluster \
argocd/config/management-cluster/eks-nodepool \
argocd/config/regional-cluster/eks-nodepoolRepository: openshift-online/rosa-hyperfleet
Length of output: 27381
🌐 Web query:
For Karpenter v1.13.0 EC2NodeClass AMI aliases, does bottlerocket@latest select a FIPS-enabled Bottlerocket EKS AMI? What AMI selector is required for FIPS-compliant Bottlerocket nodes?
💡 Result:
For Karpenter v1.13.0, the bottlerocket@latest alias does not select a FIPS-enabled Bottlerocket EKS AMI [1][2]. It specifically selects the standard (non-FIPS) Bottlerocket variant [3]. To use FIPS-compliant Bottlerocket nodes with Karpenter, you cannot rely on the bottlerocket@latest alias [3]. Instead, the recommended approach is to use an amiSelectorTerm that targets the specific FIPS AMI ID or name, typically retrieved via an SSM parameter [4][5][6]. While there has been community discussion and development regarding a potential bottlerocket-fips@latest alias, it is not part of the standard supported families documented for Karpenter v1.13.0 [4][2][3]. Consequently, to ensure your nodes are running a FIPS-compliant variant (e.g., BOTTLEROCKET_x86_64_FIPS or BOTTLEROCKET_ARM_64_FIPS), you should: 1. Identify the current SSM parameter path for the desired FIPS Bottlerocket AMI (e.g., /aws/service/bottlerocket/aws-k8s-1.31-fips/x86_64/latest/image_id) [7][6]. 2. Configure your EC2NodeClass to use the ssmParameter selector [1]: spec: amiFamily: Bottlerocket amiSelectorTerms: - ssmParameter: "/aws/service/bottlerocket/aws-k8s-1.31-fips/x86_64/latest/image_id" This method ensures Karpenter dynamically selects the latest approved FIPS-validated AMI while avoiding the standard (non-FIPS) alias [8][5].
Citations:
- 1: https://karpenter.sh/v1.13/concepts/nodeclasses/
- 2: https://github.com/aws/karpenter-provider-aws/blob/f0d0bba0/pkg/apis/crds/karpenter.k8s.aws_ec2nodeclasses.yaml
- 3: feat: add support for Bottlerocket FIPS AMIs aws/karpenter-provider-aws#7756
- 4: FIPS family for bottlerocket does not appear to be supported for amiSelectorTerms.alias aws/karpenter-provider-aws#8198
- 5: https://kerneltalks.com/linux/migrating-from-amazon-linux-2-to-bottlerocket-ami-in-eks-nodes/
- 6: https://docs.aws.amazon.com/eks/latest/userguide/bottlerocket-fips-amis.html
- 7: feat: Add Bottlerocket FIPS image variants terraform-aws-modules/terraform-aws-eks#3275
- 8: https://karpenter.sh/v1.12/tasks/managing-amis/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- candidate files ---'
git ls-files \
'argocd/config/management-cluster/eks-nodepool/**' \
'argocd/config/regional-cluster/eks-nodepool/**' \
'docs/design/fips-eks-compute.md' \
'terraform/modules/eks-cluster/**' | sed -n '1,160p'
printf '%s\n' '--- management chart ---'
for f in \
argocd/config/management-cluster/eks-nodepool/templates/00-nodeclass.yaml \
argocd/config/management-cluster/eks-nodepool/values.yaml \
argocd/config/management-cluster/eks-nodepool/Chart.yaml \
argocd/config/regional-cluster/eks-nodepool/templates/00-nodeclass.yaml \
argocd/config/regional-cluster/eks-nodepool/values.yaml \
argocd/config/regional-cluster/eks-nodepool/Chart.yaml \
docs/design/fips-eks-compute.md
do
if [ -f "$f" ]; then
printf '\n--- %s ---\n' "$f"
cat -n "$f" | sed -n '1,180p'
fi
done
printf '%s\n' '--- FIPS and AMI references ---'
rg -n -i -C 3 \
'amiFamily|amiSelectorTerms|ssmParameter|image_id|fips|bottlerocket@latest|kubernetes_version|cluster_version' \
argocd/config/management-cluster/eks-nodepool \
argocd/config/regional-cluster/eks-nodepool \
docs/design/fips-eks-compute.md \
terraform/modules/eks-clusterRepository: openshift-online/rosa-hyperfleet
Length of output: 38958
🌐 Web query:
site:karpenter.sh/v1.13/concepts/nodeclasses EC2NodeClass bottlerocket alias FIPS ssmParameter
💡 Result:
In Karpenter, the EC2NodeClass resource is used to configure node properties, including AMI selection [1]. Within the spec.amiSelectorTerms field, you can select AMIs using several methods: tags, specific IDs, or ssmParameter (which accepts an SSM parameter name or ARN) [1]. While Karpenter documentation explicitly discusses the use of ssmParameter to select custom AMIs [1], there is no provided documentation confirming a specific FIPS-related alias or a built-in ssmParameter specifically designated for FIPS-enabled Bottlerocket images. If you are using an ssmParameter, ensure the SSM parameter stores the correct AMI ID for your FIPS-compliant Bottlerocket image. If you intended to use an alias for version selection, note that aliases like al2023@vYYYYMMDD are documented for certain families [1], but you must verify if a specific alias exists for your required FIPS-enabled Bottlerocket version through your standard AMI discovery process.
Citations:
🌐 Web query:
site:docs.aws.amazon.com/eks/latest/userguide/bottlerocket-fips-amis.html Bottlerocket FIPS AMI SSM parameter
💡 Result:
There is no dedicated SSM parameter path specifically for Bottlerocket FIPS AMIs that is documented in the official AWS resources [1]. While Bottlerocket provides FIPS-compliant AMIs for EKS in four variants (BOTTLEROCKET_x86_64_FIPS, BOTTLEROCKET_ARM_64_FIPS, BOTTLEROCKET_x86_64_NVIDIA_FIPS, and BOTTLEROCKET_ARM_64_NVIDIA_FIPS), these are typically selected by choosing the applicable AMI type when creating a managed node group [1]. If you are not using a managed node group, you can use these AMIs by creating a launch template [1].
Citations:
🌐 Web query:
site:github.com/aws/karpenter-provider-aws v1.13 bottlerocket@latest AMI alias source
💡 Result:
In Karpenter, the bottlerocket@latest AMI alias acts as a pointer to the most recent Bottlerocket AMI version available via AWS Systems Manager (SSM) Parameter Store [1]. Karpenter resolves this alias by querying specific SSM paths maintained by AWS that map to the latest image IDs for different Kubernetes versions and architectures [2][3]. When you use the alias bottlerocket@latest in an EC2NodeClass (or the deprecated AWSNodeTemplate), Karpenter dynamically fetches the latest AMI ID [1]. The source for these aliases is a set of managed SSM parameters following a standard path structure: /aws/service/bottlerocket/aws-k8s-//latest/image_id [4][5][6] For example, when resolving an image for a specific Kubernetes version and architecture, Karpenter constructs a query to these SSM paths [2][3]. If you pin the version (e.g., bottlerocket@1.13.0 or bottlerocket@v1.13.0), Karpenter substitutes the version string into the path, allowing it to retrieve the specific image ID for that version [2][7][3]. Key points regarding this mechanism: 1. Version Formatting: Karpenter accepts version strings with or without the 'v' prefix (e.g., v1.13.0 and 1.13.0 are both valid) [2][7][3]. 2. Resolution: The resolution process requires the Karpenter controller to have IAM permissions for ssm:GetParameter to access these public SSM paths [5][6]. 3. Risks of 'latest': Using bottlerocket@latest is generally not recommended for production environments because it can lead to unintentional upgrades when a new AMI is released, resulting in drift [1]. Pinning to a specific version is considered best practice for stable cluster operations [1]. If resolution fails, it is commonly due to missing IAM permissions or network connectivity issues (such as VPC endpoints blocking SSM access) that prevent the controller from reaching the Parameter Store [5][6].
Citations:
- 1: https://github.com/aws/karpenter-provider-aws/blob/main/pkg/apis/crds/karpenter.k8s.aws_ec2nodeclasses.yaml
- 2: https://github.com/aws/karpenter-provider-aws/blob/main/pkg/providers/amifamily/bottlerocket.go
- 3: https://github.com/aws/karpenter-provider-aws/blob/dee70a56/pkg/providers/amifamily/bottlerocket.go
- 4: Adding initial support for bottlerocket without custom launch template aws/karpenter-provider-aws#1110
- 5: EC2NodeClass failing to resolve AMI alias for Bottlerocket and AL2023 aws/karpenter-provider-aws#8564
- 6: fix: ami discovery error message aws/karpenter-provider-aws#7940
- 7: fix: allow v prefix for bottlerocket aliases aws/karpenter-provider-aws#6736
🌐 Web query:
site:docs.aws.amazon.com/eks/latest/userguide/bottlerocket-fips-amis.html "launch template" Karpenter
💡 Result:
Karpenter does not directly use AWS Launch Templates to provision nodes. Instead, Karpenter uses the EC2NodeClass custom resource to define the configuration of the nodes it manages. In the Karpenter architecture, the EC2NodeClass is used to specify configuration details such as subnets, security groups, and Amazon Machine Images (AMIs). Karpenter dynamically handles the instance provisioning process based on the requirements defined in your NodePool and the settings provided in the EC2NodeClass. While AWS Launch Templates are commonly used for managing manual or Auto Scaling group-based node configurations, Karpenter automates the provisioning process by interacting directly with the EC2 API, bypassing the need for you to pre-create or manually reference AWS Launch Templates. If you need to perform advanced node customization that might traditionally be done via a launch template (such as custom user data or specific bootstrap settings), you define these within the EC2NodeClass configuration rather than in a separate launch template resource [1].
Citations:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import json
import urllib.request
urls = [
"https://raw.githubusercontent.com/aws/karpenter-provider-aws/v1.13.0/pkg/providers/amifamily/bottlerocket.go",
"https://raw.githubusercontent.com/aws/karpenter-provider-aws/v1.13.0/pkg/providers/amifamily/ami.go",
]
for url in urls:
print(f"--- {url} ---")
try:
with urllib.request.urlopen(url, timeout=10) as response:
text = response.read().decode()
for i, line in enumerate(text.splitlines(), 1):
if any(term in line.lower() for term in ("bottlerocket", "ssm", "alias", "fips", "aws-k8s")):
print(f"{i}:{line}")
except Exception as exc:
print(f"ERROR: {exc}")
PYRepository: openshift-online/rosa-hyperfleet
Length of output: 679
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for url in \
"https://raw.githubusercontent.com/aws/karpenter-provider-aws/v1.13.0/pkg/providers/amifamily/bottlerocket.go" \
"https://raw.githubusercontent.com/aws/karpenter-provider-aws/v1.13.0/pkg/providers/amifamily/ami.go" \
"https://docs.aws.amazon.com/eks/latest/userguide/bottlerocket-fips-amis.html"
do
printf '\n--- %s ---\n' "$url"
curl -k -L --fail --max-time 20 "$url" |
grep -in -E 'bottlerocket|aws-k8s|image_id|fips|ssm|launch template|ami' |
sed -n '1,180p'
doneRepository: openshift-online/rosa-hyperfleet
Length of output: 24801
Select FIPS Bottlerocket AMIs in both EC2NodeClass resources.
Karpenter resolves bottlerocket@latest to standard Bottlerocket SSM parameters, not FIPS variants. Select approved FIPS AMI IDs or a supported custom SSM parameter that returns those IDs.
📍 Affects 2 files
argocd/config/management-cluster/eks-nodepool/templates/00-nodeclass.yaml#L7-L8(this comment)argocd/config/regional-cluster/eks-nodepool/templates/00-nodeclass.yaml#L7-L8
🤖 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, Update the EC2NodeClass amiSelectorTerms in both
argocd/config/management-cluster/eks-nodepool/templates/00-nodeclass.yaml lines
7-8 and argocd/config/regional-cluster/eks-nodepool/templates/00-nodeclass.yaml
lines 7-8 to select approved FIPS Bottlerocket AMI IDs or a supported custom SSM
parameter returning FIPS AMI IDs, replacing the standard bottlerocket@latest
alias.
| resource "aws_sqs_queue_policy" "karpenter_interruption" { | ||
| queue_url = aws_sqs_queue.karpenter_interruption.id | ||
|
|
||
| policy = jsonencode({ | ||
| Version = "2012-10-17" | ||
| Statement = [{ | ||
| Sid = "AllowEventBridge" | ||
| Effect = "Allow" | ||
| Principal = { Service = "events.amazonaws.com" } | ||
| Action = "sqs:SendMessage" | ||
| Resource = aws_sqs_queue.karpenter_interruption.arn | ||
| }] | ||
| }) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for an existing caller identity data source in the module.
fd -t f -e tf . terraform/modules/eks-cluster | xargs -r rg -n 'aws_caller_identity|aws_partition'Repository: openshift-online/rosa-hyperfleet
Length of output: 3285
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- queue policy and related resources ---'
sed -n '320,430p' terraform/modules/eks-cluster/iam.tf
printf '%s\n' '--- event rule declarations and queue targets ---'
rg -n -C 8 'aws_cloudwatch_event_rule|karpenter_interruption' terraform/modules/eks-cluster
printf '%s\n' '--- module data sources ---'
cat -n terraform/modules/eks-cluster/data.tfRepository: openshift-online/rosa-hyperfleet
Length of output: 11631
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
text = Path("terraform/modules/eks-cluster/iam.tf").read_text()
rules = re.search(
r'locals\s*\{\s*karpenter_event_rules\s*=\s*\{(.*?)\n\s*\}\s*\}\s*'
r'resource\s+"aws_cloudwatch_event_rule"\s+"karpenter"',
text,
re.S,
)
targets = re.search(
r'resource\s+"aws_cloudwatch_event_target"\s+"karpenter"\s*\{(.*?)\n\}',
text,
re.S,
)
assert rules and targets, "Karpenter rule or target block not found"
rule_keys = re.findall(r'^\s{4}([A-Za-z0-9-]+)\s*=\s*\{', rules.group(1), re.M)
print("rule_keys:", rule_keys)
print("rule_count:", len(rule_keys))
print("rules_use_for_each:", 'for_each = local.karpenter_event_rules' in text)
print("targets_use_same_for_each:", 'for_each = local.karpenter_event_rules' in targets.group(1))
print("targets_use_indexed_rule:", 'aws_cloudwatch_event_rule.karpenter[each.key].name' in targets.group(1))
print(
"caller_identity_data_source:",
Path("terraform/modules/eks-cluster/data.tf").read_text().count(
'data "aws_caller_identity" "current"'
),
)
PYRepository: openshift-online/rosa-hyperfleet
Length of output: 414
🌐 Web query:
AWS EventBridge SQS target queue policy aws:SourceArn aws:SourceAccount confused deputy documentation
💡 Result:
To prevent the confused deputy problem when using Amazon EventBridge with Amazon SQS as a target, you must apply a resource-based policy to the SQS queue that restricts access using the aws:SourceArn and/or aws:SourceAccount global condition context keys [1][2]. The confused deputy problem occurs when a service with higher privileges (in this case, EventBridge) is manipulated to perform actions on a resource (your SQS queue) on behalf of an unauthorized actor [2]. By using these condition keys, you ensure that the SQS queue only accepts requests from the specific EventBridge rule or account that you have explicitly authorized [1]. Implementation Details: 1. Use aws:SourceArn: This is the most effective protection [1]. You should set this to the full Amazon Resource Name (ARN) of the EventBridge rule that is authorized to send events to the queue [3][1]. Example policy condition: "Condition": { "ArnEquals": { "aws:SourceArn": "arn:aws:events:region:account-id:rule/rule-name" } } 2. Use aws:SourceAccount: Use this if you want to allow any resource within a specific AWS account to interact with the queue [1]. When combined with aws:SourceArn, it provides an additional layer of verification [1]. 3. Combining with Principal: In your SQS resource policy, the Principal should generally be set to the EventBridge service principal, events.amazonaws.com [3]. Best Practices: - Always use the most specific ARN possible for aws:SourceArn [1]. - If you do not know the full ARN or need to allow multiple resources, you can use wildcard characters in the aws:SourceArn condition (e.g., arn:aws:events::123456789012:rule/), though this is less restrictive [1]. - This approach is part of implementing least-privilege access for cross-service interactions in AWS [2]. For detailed configuration steps, refer to the official AWS documentation on resource-based policies for EventBridge and SQS [3][1][2].
Citations:
- 1: https://docs.aws.amazon.com/eventbridge/latest/userguide/cross-service-confused-deputy-prevention.html
- 2: https://docs.aws.amazon.com/AWSSimpleQueueService/latest/SQSDeveloperGuide/sqs-least-privilege-policy.html
- 3: https://docs.aws.amazon.com/eventbridge/latest/userguide/eb-use-resource-based.html
Scope the SQS queue policy to this account and its four EventBridge rules.
The unrestricted events.amazonaws.com principal allows unauthorized EventBridge rules to send interruption messages. Karpenter can process these messages and drain nodes. Add aws:SourceAccount and aws:SourceArn conditions, using data.aws_caller_identity.current.account_id and the ARNs from aws_cloudwatch_event_rule.karpenter. Also deny non-TLS access.
🤖 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/iam.tf` around lines 339 - 352, Update the
aws_sqs_queue_policy.karpenter_interruption policy to restrict EventBridge sends
with aws:SourceAccount set to data.aws_caller_identity.current.account_id and
aws:SourceArn covering the four aws_cloudwatch_event_rule.karpenter ARNs, and
add an explicit deny for non-TLS access using aws:SecureTransport. Preserve the
existing AllowEventBridge send permissions while scoping them to the account and
rules.
Source: Coding guidelines
| CLUSTER_NAME="${self.name}" | ||
| REGION=$(echo "${self.arn}" | cut -d: -f4) | ||
| echo "Terminating Karpenter EC2 instances for cluster: $CLUSTER_NAME" | ||
| INSTANCE_IDS=$(aws ec2 describe-instances \ | ||
| --region "$REGION" \ | ||
| --filters \ | ||
| "Name=tag:aws:eks:cluster-name,Values=$CLUSTER_NAME" \ | ||
| "Name=tag-key,Values=karpenter.sh/nodeclaim" \ | ||
| "Name=instance-state-name,Values=pending,running,stopping,stopped" \ | ||
| --query 'Reservations[].Instances[].InstanceId' \ | ||
| --output text) | ||
| if [ -z "$INSTANCE_IDS" ]; then | ||
| echo "No Karpenter-managed instances found." | ||
| exit 0 | ||
| fi | ||
| echo "Terminating: $INSTANCE_IDS" | ||
| aws ec2 terminate-instances --region "$REGION" --instance-ids $INSTANCE_IDS |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Which AWS tags does Karpenter v1 apply to EC2 instances it launches?
💡 Result:
Karpenter v1 applies a set of default tags to the EC2 instances it launches, in addition to any custom tags defined by the user in the EC2NodeClass [1]. The default tags applied by Karpenter include [1]: - Name: - karpenter.sh/nodeclaim: - karpenter.sh/nodepool: - karpenter.k8s.aws/ec2nodeclass: - kubernetes.io/cluster/: owned - eks:eks-cluster-name: Users can define additional custom tags via the spec.tags field in the EC2NodeClass [1]. While the default Name tag can be overridden, users are restricted from overriding tags within protected domains, such as karpenter.sh, karpenter.k8s.aws, and kubernetes.io/cluster [1]. Karpenter also ensures these tags are propagated to associated resources, such as EBS volumes, where appropriate [2][3].
Citations:
- 1: https://www.mintlify.com/aws/karpenter-provider-aws/concepts/nodeclasses
- 2: https://github.com/aws/karpenter-provider-aws/blob/main/pkg/providers/instance/types.go
- 3: Tags fail to propagate to resources aws/karpenter-provider-aws#3198
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
git ls-files 'terraform/modules/eks-cluster/main.tf' 'terraform/modules/eks-cluster/iam.tf'
printf '%s\n' '--- destroy provisioner ---'
sed -n '110,165p' terraform/modules/eks-cluster/main.tf
printf '%s\n' '--- IAM tag references ---'
rg -n -C 3 'kubernetes\.io/cluster|aws:eks:cluster-name|nodeclaim|RequestTag' terraform/modules/eks-cluster/iam.tf terraform/modules/eks-cluster/main.tf
printf '%s\n' '--- cluster name and ID definitions ---'
rg -n -C 3 'cluster_id|cluster_name|aws_eks_cluster|self\.name|self\.arn' terraform/modules/eks-cluster/main.tf terraform/modules/eks-cluster/variables.tf terraform/modules/eks-cluster/locals.tf 2>/dev/null || trueRepository: openshift-online/rosa-hyperfleet
Length of output: 16158
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
p = Path("terraform/modules/eks-cluster/main.tf")
text = p.read_text()
start = text.index('INSTANCE_IDS=$(aws ec2 describe-instances')
end = text.index('aws ec2 terminate-instances', start)
block = text[start:end]
print("describe-instances block:")
print(block)
print("filters are evaluated as AND within one AWS EC2 request; this verifier checks the literal tag keys used:")
for line in block.splitlines():
if 'Name=tag:' in line or 'Name=tag-key' in line:
print(line.strip())
PYRepository: openshift-online/rosa-hyperfleet
Length of output: 940
Use the Karpenter cluster tag in the instance filter.
Karpenter-launched instances use kubernetes.io/cluster/<cluster-name>=owned and karpenter.sh/nodeclaim. They do not use aws:eks:cluster-name. EC2 combines these filters with AND semantics, so the query returns no Karpenter instances and can leave ENIs that block VPC teardown.
🐛 Proposed fix
- "Name=tag:aws:eks:cluster-name,Values=$CLUSTER_NAME" \
+ "Name=tag:kubernetes.io/cluster/$CLUSTER_NAME,Values=owned" \📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CLUSTER_NAME="${self.name}" | |
| REGION=$(echo "${self.arn}" | cut -d: -f4) | |
| echo "Terminating Karpenter EC2 instances for cluster: $CLUSTER_NAME" | |
| INSTANCE_IDS=$(aws ec2 describe-instances \ | |
| --region "$REGION" \ | |
| --filters \ | |
| "Name=tag:aws:eks:cluster-name,Values=$CLUSTER_NAME" \ | |
| "Name=tag-key,Values=karpenter.sh/nodeclaim" \ | |
| "Name=instance-state-name,Values=pending,running,stopping,stopped" \ | |
| --query 'Reservations[].Instances[].InstanceId' \ | |
| --output text) | |
| if [ -z "$INSTANCE_IDS" ]; then | |
| echo "No Karpenter-managed instances found." | |
| exit 0 | |
| fi | |
| echo "Terminating: $INSTANCE_IDS" | |
| aws ec2 terminate-instances --region "$REGION" --instance-ids $INSTANCE_IDS | |
| CLUSTER_NAME="${self.name}" | |
| REGION=$(echo "${self.arn}" | cut -d: -f4) | |
| echo "Terminating Karpenter EC2 instances for cluster: $CLUSTER_NAME" | |
| INSTANCE_IDS=$(aws ec2 describe-instances \ | |
| --region "$REGION" \ | |
| --filters \ | |
| "Name=tag:kubernetes.io/cluster/$CLUSTER_NAME,Values=owned" \ | |
| "Name=tag-key,Values=karpenter.sh/nodeclaim" \ | |
| "Name=instance-state-name,Values=pending,running,stopping,stopped" \ | |
| --query 'Reservations[].Instances[].InstanceId' \ | |
| --output text) | |
| if [ -z "$INSTANCE_IDS" ]; then | |
| echo "No Karpenter-managed instances found." | |
| exit 0 | |
| fi | |
| echo "Terminating: $INSTANCE_IDS" | |
| aws ec2 terminate-instances --region "$REGION" --instance-ids $INSTANCE_IDS |
🤖 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/main.tf` around lines 130 - 146, Update the EC2
instance filter in the Karpenter termination script to use the Kubernetes
cluster tag key `kubernetes.io/cluster/$CLUSTER_NAME` with value `owned`
alongside the existing `karpenter.sh/nodeclaim` filter. Remove the
`aws:eks:cluster-name` filter while preserving the existing state filter and
termination flow.
|
/test check-rendered-files |
The render.py doc checker was treating 'else' and 'end' from escaped Go
template syntax as undocumented Jinja2 variables. These appear in
argocd-bootstrap/applicationset.yaml.j2 within string literals like:
{{ '{{ if eq ... }}0{{ else }}5{{ end }}' }}
The outer {{ }} is Jinja2 escaping to produce literal Go template text for
ArgoCD ApplicationSet. Add 'else' and 'end' to CONTEXT_VARS so they're
excluded from variable documentation requirements.
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/render.py`:
- Around line 371-372: Update the template keyword allowlist and scanner logic
around the Go template keyword handling so “end” is exempted only when it comes
from escaped Go-template syntax, not as a global Jinja variable allowlist entry.
Add a regression test covering {{ end }} to ensure it remains detected
appropriately while escaped Go-template tokens continue to pass.
🪄 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: e245af5e-3795-4a6d-ae17-d6f6db862a11
📒 Files selected for processing (1)
scripts/render.py
Update design documentation to reflect current ArgoCD-managed Karpenter implementation (PR openshift-online#733). Remove outdated references to ECS bootstrap installing Karpenter and creating NodePools. Changes to docs/design/fips-eks-compute.md: - Lines 56-65: Replace ECS bootstrap Karpenter installation sequence with ArgoCD sync wave ordering (Karpenter wave 0, eks-nodepool wave 10) - Lines 80-85: Remove prewarm validation references, describe ArgoCD sync wave guarantees and ApplicationSet values injection - Lines 129-132: Update day-2 management to reflect ArgoCD-exclusive ownership and ApplicationSet directory-based cluster selection Changes to docs/design/karpenter-node-provisioning.md: - Line 31-32: Remove ECS bootstrap task reference, simplify to ArgoCD Application configuration These docs previously described an imperative ECS bootstrap approach that was replaced by declarative ArgoCD Applications with sync wave ordering for proper GitOps lifecycle management. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Five fixes identified and verified by parallel agents: 1. Monitoring tolerations: Added CriticalAddonsOnly=true:NoSchedule toleration to kube-state-metrics and prometheus.prometheusSpec in argocd/config/management-cluster/monitoring/values.yaml. Both now match prometheusOperator, allowing all monitoring pods to schedule on bootstrap nodes before Karpenter NodePools sync. 2. IRSA documentation: Updated karpenter-node-provisioning.md and zoa-trusted-actions.md to describe IRSA as a deliberate repository choice. Clarified that EKS Pod Identity is the ZOA platform standard and AWS recommendation for new workloads, while IRSA remains fully supported. Removed "admission-webhook", "predates", and "upstream-recommended" claims that were factually incorrect. 3. Register.sh readiness loop: Replaced retry counting with 40-minute wall-clock deadline in scripts/buildspec/register.sh. Now tracks elapsed time, caps curl --max-time and sleep to remaining deadline, and exits when deadline reached. Ensures timeout happens at exactly 40 minutes regardless of retry count. 4. Verify-fips.sh CRD check: Improved scripts/verify-fips.sh kubectl get crd check to distinguish missing CRD from API errors. Captures output and status separately, skips only when query succeeds and confirms CRD is absent, fails verification when kubectl returns API or authorization error. 5. ALB controller IAM tags: Added elasticloadbalancing:AddTags permission to terraform/modules/aws-load-balancer-controller/iam.tf for tag-on-create operations. New ELBAddTagsOnCreate statement grants AddTags conditioned on CreateAction matching CreateTargetGroup or CreateLoadBalancer. Preserves existing ELBMutateTags for non-create tag mutations. All changes validated: documentation formatting, bash syntax, terraform format. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Three fixes identified and verified by parallel agents:
1. ECS bootstrap README: Updated terraform/modules/ecs-bootstrap/README.md to
reflect current ArgoCD-managed Karpenter implementation. Removed obsolete
steps describing ECS-based Karpenter installation, EC2NodeClass/NodePool
application, and prewarm validation. Documented that ECS bootstrap installs
ArgoCD and exits, with ArgoCD subsequently installing Karpenter (sync wave 0)
and eks-nodepool Application (sync wave 10) via GitOps.
2. Secrets Store CSI tolerations: Added CriticalAddonsOnly=true:NoSchedule
toleration to aws_eks_addon.aws_secrets_store_csi_driver_provider
configuration in terraform/modules/eks-cluster/main.tf. Allows CSI driver
pods to schedule on bootstrap nodes before Karpenter NodePools sync.
Preserved existing node group taints and discovery tags.
3. render.py Go template handling: Improved scripts/render.py to handle Go
template keywords (else, end) more precisely than global allowlist. Removed
"else" and "end" from CONTEXT_VARS. Added _ESCAPED_GO_TEMPLATE_RE regex
to detect Jinja2-escaped Go template strings. Updated scan_template_variables()
to build skip ranges for escaped regions and filter variables found inside
them. Go template keywords now only exempted when inside escaped syntax like
{{ '{{ if ... }}X{{ else }}Y{{ end }}' }}, not as regular Jinja2 variables.
All changes validated: documentation formatting, terraform format, render.py
doc check passes.
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
1 participant
Summary
Replaces ECS bootstrap installation of Karpenter with ArgoCD Application management, enabling full GitOps lifecycle (version control, drift detection, self-healing). Removes imperative NodePool seeding in favor of sync wave ordering.
This addresses reviewer feedback about orphaned Karpenter installation:
Changes
ArgoCD Applications for Karpenter
argocd/config/{regional,management}-cluster/karpenter/chartskube-systemnamespaceSimplified ECS Bootstrap
helm install(~25 lines)Sync Wave Ordering
karpenterApplication: wave 0 (installs controller + CRDs first)eks-nodepoolApplication: wave 10 (waits for Karpenter CRDs)ApplicationSet Values Injection
karpenter.settings.clusterNamekarpenter.settings.interruptionQueuekarpenter.serviceAccount.annotations(IRSA role ARN)Architecture
Before (Orphaned Karpenter)
After (Pure GitOps)
Benefits
✅ Version Management - Karpenter version pinned in
Chart.yaml✅ Drift Detection - ArgoCD monitors and auto-heals configuration
✅ GitOps Ownership - All changes via git commits and ArgoCD sync
✅ Simplified Bootstrap - ECS removed 40+ lines of installation logic
✅ Guaranteed Ordering - Sync waves ensure CRDs exist before NodePool
✅ Consistent Pattern - Same Application model as ArgoCD self-management
Testing
Files Changed
12 files changed, +199/-53 lines
New files:
argocd/config/regional-cluster/karpenter/(Chart.yaml, values.yaml)argocd/config/management-cluster/karpenter/(Chart.yaml, values.yaml)Modified:
terraform/modules/ecs-bootstrap/main.tf(removed Karpenter install)config/templates/argocd-bootstrap/applicationset.yaml.j2(sync waves, values injection)Summary by CodeRabbit