test: PriorityClass instead of CriticalAddonsOnly taint, Bottlerocket bootstrap nodes (pipeline test only) - #743
Open
theautoroboto wants to merge 7 commits into
Conversation
Replaces AWS EKS Auto Mode with self-managed Karpenter for more granular control over node provisioning and scaling across Regional and Management Clusters. Key changes: - Remove Auto Mode enablement from EKS module - Deploy Karpenter controller via Helm with GitOps - Configure EC2NodeClass and NodePool resources, pinned to bottlerocket v1.64.0 instead of the floating @latest alias - Add AWS Load Balancer Controller for RC (Karpenter dependency), using the current elbv2.k8s.aws/v1beta1 TargetGroupBinding API, pinned to the same AWS provider version (~> 6.56.0) as the rest of the repo - Upgrade bootstrap node group from t3.medium to t3.large for ArgoCD HA - Applications sync concurrently with no sync-wave ordering, consistent with the project's eventual-consistency ArgoCD model (selfHeal + retry.limit=-1 with backoff). Cross-Application CRD races (eks-nodepool/karpenter, ServiceMonitor+PrometheusRule/monitoring, TargetGroupBinding/aws-load-balancer-controller) are handled via per-resource argocd.argoproj.io/sync-options annotations, matching the existing cert-manager ClusterIssuer pattern, rather than a global SkipDryRunOnMissingResource flag that would mask real bugs everywhere - Remove CriticalAddonsOnly tolerations from the monitoring stack (prometheusOperator, kube-state-metrics, prometheus) — monitoring has no chicken-and-egg bootstrap dependency like ArgoCD/Karpenter do, so it can wait for Karpenter to provision a regular workload node instead of competing for fixed bootstrap-node capacity - Update Helm download with retry logic and SHA-256 checksum verification - Improve HyperShift install error handling (capture output, check stderr) - Restore hyperfleet values.yaml to upstream defaults - Remove stale hyperfleetApi/Sentinel/Adapter valuesObject keys left over from the pre-consolidation chart layout - Restore a response_templates block on the API Gateway default_4xx response that was silently dropped in this branch's rebase history, reverting a real upstream Terraform state-drift fix - Restore force_destroy variable on the regional-oidc module and revert an incorrect platform-monitoring test workaround, both reverted to match upstream/main Hardening added during review: - Gate the post-bootstrap HyperShift Synced+Healthy wait behind an explicit WAIT_FOR_HYPERSHIFT_HEALTH flag so ordinary bootstraps (not just the E2E workflow) skip it, and add --request-timeout to every kubectl call in that loop so a hung connection can't block past the loop's own deadline - Require a non-empty karpenter_controller_role_arn via variable validation instead of allowing a silent empty default - Build the ECS RunTask --overrides payload with jq -n --arg for every value instead of interpolating shell variables into hand-built JSON, removed a duplicated CLUSTER_TYPE entry, and fail clearly before the RunTask call if the serialized payload nears ECS's 8192-character limit - Correct documentation that had fallen out of sync with the implementation: sync-wave ordering claims, IAM role inventory, FIPS/ FedRAMP compliance scope, and stale version references Terminology: "OSS Karpenter" renamed to "self-managed Karpenter" throughout docs, comments, and Terraform descriptions — EKS Auto Mode also runs Karpenter internally, so "OSS" didn't capture the actual distinction; "self-managed" states it directly (ArgoCD-managed lifecycle vs AWS-managed under Auto Mode). Known gaps: - karpenter's own chart-managed ServiceMonitor (from the upstream oci://public.ecr.aws/karpenter chart) has no annotations passthrough for SkipDryRunOnMissingResource, so it still relies on retry/selfHeal alone to recover from a missing monitoring CRD on first bootstrap. - Disabling Prometheus Operator's admissionWebhooks (needed to avoid a PreSync hook Job race during ArgoCD's own bootstrap self-sync — a ttlSecondsAfterFinished-based workaround was tried first and found insufficient) also disables admission-time promql validation for PrometheusRule; malformed rules are applied instead of rejected. - WAIT_FOR_HYPERSHIFT_HEALTH is defined and validated but nothing in this repo sets it to "true" yet — the external CI job definition that chains provisioning immediately into E2E tests needs to export it. Infrastructure validation: - Tested in ephemeral environment with full RC + MC provisioning - Verified node provisioning metrics in CloudWatch - Confirmed Karpenter scales nodes based on pending pods Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…gional-cluster/ argocd/config/management-cluster/karpenter and .../regional-cluster/karpenter were symlinks to shared/karpenter with no consumer: find (used by make helm-lint) doesn't follow symlinked directories by default, so they were never actually linted via these paths, and no script or template references them. The ApplicationSet already discovers shared/karpenter directly for both cluster types. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ing gap ArgoCD HA (server, application-controller, repo-server, applicationset, dex, notifications, plus a redis-ha subchart with 3 redis + 3 haproxy pods carrying zero resource requests anywhere in the dependency chain) and the Karpenter controller run on a fixed 2-node group. Explicit resource requests alone already total ~1.75 vCPU / ~3.2 GiB across the two nodes, against ~1.9 vCPU / ~7 GiB allocatable per t3.large (2 vCPU / 8 GiB raw) node, before counting the zero-request redis-ha/haproxy pods or system DaemonSets. Bump to m7i.xlarge (4 vCPU / 16 GiB) for real headroom, and sync every doc/comment reference to the old instance type. Also add the CriticalAddonsOnly toleration to the AWS Load Balancer Controller, which had none — a verified, independent bug that left it unable to schedule until Karpenter provisions untainted nodes. Investigated and explicitly declined a proposal to replace CriticalAddonsOnly tolerations with a PriorityClass on ArgoCD/Karpenter instead: the "toleration sprawl" premise doesn't hold (prometheusOperator, kube-state-metrics, and prometheus carry no such toleration today), and the claim that this would let us drop the monitoring admissionWebhooks/tls workaround doesn't hold either — that workaround exists because ArgoCD's own controller restarts during its own GitOps self-adoption, a reconciliation-timing race that PriorityClass-based preemption has no mechanism to prevent. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ctl path The destroy provisioner tried to reach the cluster API (update-kubeconfig, kubectl delete nodepools) before falling back to tag-based EC2 termination. The CodeBuild project that runs terraform destroy has no VPC connectivity to this fully-private cluster's API, so that path never actually succeeds in practice -- it silently fell through the 2>/dev/null fallback to the same tag-based termination every time, masking a real reachability gap behind what looked like an intentional "cluster already deleted" skip. Since the whole cluster is being destroyed there's no workload to protect by draining gracefully first, so remove the kubectl-dependent phase entirely rather than building a VPC-connected execution path for a step that provides no benefit. Terminating by tag has no dependency on kubeconfig, cluster reachability, or kubectl being installed. Also wrap the instance-terminated wait in an explicit 300s timeout instead of relying on the AWS CLI waiter's implicit one, and document that LBC only reconciles TargetGroupBinding in this architecture (confirmed no Ingress or LBC-provisioned ALB/NLB usage anywhere in argocd/config/), so there's no orphaned load balancer to worry about here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…nodes to Bottlerocket Switches the karpenter-bootstrap node group from AL2023 to Bottlerocket (matching the AMI family Karpenter-provisioned workload nodes already use) and replaces the CriticalAddonsOnly:NoSchedule exclusion taint with a new bootstrap-critical PriorityClass (value 100000, applied to both ArgoCD and Karpenter, deliberately downgrading Karpenter from its chart's default system-cluster-critical). Preemption lets ArgoCD/Karpenter reclaim capacity if the fixed-size node group fills up, instead of excluding every other pod via a taint. All 6 CriticalAddonsOnly toleration sites in the repo are updated accordingly: ArgoCD and Karpenter move to priorityClassName; AWS Load Balancer Controller, the hypershift-install Job, and the Secrets Store CSI Driver DaemonSet just drop the now-unnecessary toleration outright. This is an experimental test branch off remove-auto-mode-v2, not intended to merge directly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[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
📝 WalkthroughWalkthroughThis change migrates EKS clusters from Auto Mode to self-managed Karpenter. It adds AWS Load Balancer Controller infrastructure, updates Argo CD and bootstrap flows, changes Kubernetes API resources, and revises related documentation and operational scripts. ChangesSelf-managed Karpenter migration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
theautoroboto
marked this pull request as ready for review
August 12, 2026 15:20
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
![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: 10
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (14)
terraform/modules/eks-cluster/variables.tf-78-82 (1)
78-82: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
ami_kms_key_arndocumentation contradicts the implemented policy. The variable description promiseskms:Decryptandkms:CreateGranton both the node role and the controller role. The implementation grantskms:CreateGrantandkms:DescribeKeyto the controller role only. Both locations also still describe a RHEL FIPS AMI, while the bootstrap node group and the NodeClass now use Bottlerocket.
terraform/modules/eks-cluster/variables.tf#L78-L82: rewrite the description to state the actual actions (kms:CreateGrant,kms:DescribeKey) and the single controller role, and drop the RHEL-specific wording if Bottlerocket is the target AMI.terraform/modules/eks-cluster/iam.tf#L264-L295: update the block comment to match the Bottlerocket AMI, or add the missing node-role permissions if a KMS-encrypted custom AMI is still in scope.🤖 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 78 - 82, Update terraform/modules/eks-cluster/variables.tf lines 78-82 to document the implemented KMS actions, kms:CreateGrant and kms:DescribeKey, granted only to the controller role, and remove the RHEL-specific wording. Update the related block comment in terraform/modules/eks-cluster/iam.tf lines 264-295 to describe the Bottlerocket AMI; do not add node-role permissions unless a KMS-encrypted custom AMI remains in scope.argocd/config/shared/karpenter/Chart.yaml-6-9 (1)
6-9: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAdd explicit Karpenter CRD management.
Karpenter
1.14.0is published. This release adds thecapacitybuffersCRD. Add thekarpenter-crdchart at version1.14.0, or use an equivalent Argo CD strategy, and upgrade it with the controller for both cluster types.🤖 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/shared/karpenter/Chart.yaml` around lines 6 - 9, Add explicit Karpenter CRD management in the Chart.yaml dependency configuration alongside the existing karpenter controller dependency. Include the karpenter-crd chart at version 1.14.0 and ensure the dependency is applied for both cluster types while keeping it synchronized with the controller upgrade.argocd/config/management-cluster/hypershift/templates/05-job.yaml-9-9 (1)
9-9: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winCheck the time budget against the CRD wait.
activeDeadlineSecondsis 3600. The Prometheus Operator CRD wait alone can consume 1800 seconds. A slow monitoring sync therefore leaves under 30 minutes forhypershift installand both patches, and the Job is killed by the deadline instead of printing the CRD diagnostic output at line 84.Consider a shorter CRD deadline, for example 900 seconds, or a larger
activeDeadlineSeconds.🤖 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 9, Adjust activeDeadlineSeconds in the Job configuration to accommodate the Prometheus Operator CRD wait plus hypershift install and both patch operations, ensuring the Job is not terminated before the diagnostic output executes. Prefer shortening the CRD wait to the suggested 900 seconds if that preserves the intended behavior; otherwise increase activeDeadlineSeconds accordingly.terraform/config/management-cluster/main.tf-61-66 (1)
61-66: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the comment: the bootstrap task does not install Karpenter.
This comment says the bootstrap task performs
helm installof Karpenter and ArgoCD. The module header interraform/modules/ecs-bootstrap/main.tf(lines 4-7) states the opposite: the module does not install Karpenter, and ArgoCD installs Karpenter through GitOps after bootstrap.📝 Proposed comment fix
# This ecs_bootstrap module creates ECS Fargate infrastructure that runs in the # cluster's VPC and can reach the private EKS API. A one-time bootstrap task -# performs `helm install` of Karpenter and ArgoCD onto the bootstrap nodes, then -# exits. After bootstrap, Karpenter and ArgoCD continue running on the managed -# node group, and the ECS infrastructure remains available for future audited -# SRE operations. +# performs `helm install` of ArgoCD onto the bootstrap nodes and creates the root +# ArgoCD Application, then exits. ArgoCD then installs Karpenter through GitOps. +# After bootstrap, Karpenter and ArgoCD run on the managed node group, and the ECS +# infrastructure remains available for future audited SRE operations.🤖 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 61 - 66, Correct the module comment describing the one-time bootstrap task: remove the claim that it installs Karpenter, and state that it installs ArgoCD, which subsequently installs Karpenter through GitOps. Preserve the existing description of bootstrap completion and ongoing managed-node-group operation.terraform/modules/aws-load-balancer-controller/iam.tf-8-10 (1)
8-10: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd a
cluster_namelength validation.The IAM role name uses a 29-character suffix, which limits
cluster_nameto 35 characters. The module has no validation for this limit, so longer cluster names can fail during IAM role creation.🤖 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 8 - 10, Add validation for the cluster_name input used by aws_iam_role.aws_lbc, enforcing a maximum length of 35 characters to keep the generated IAM role name within its limit. Preserve existing cluster_name behavior for valid values and provide a clear validation error for longer names.argocd/config/management-cluster/hypershift/templates/05-job.yaml-153-180 (1)
153-180: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winMake the external-dns patches idempotent.
Replace=true,Force=truerecreates this Job on every Argo CD sync. Ifhypershift installpreserves the existing arrays, each run appends duplicate arguments and RBAC rules. The scalar--aws-assume-roleflag uses the last value, but the ClusterRole still accumulates duplicate rules. Add read-before-patch guards for both resources.🤖 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 153 - 180, Make the external-dns patches in the Job idempotent by reading each resource before patching and checking whether the desired argument or RBAC rules already exist. Only execute the deployment PATCH when the exact --aws-assume-role value is absent, and only add each ClusterRole rule when it is absent; retain the existing HTTP status logging for patches that run and skip already-configured entries.argocd/config/management-cluster/monitoring/values.yaml-20-39 (1)
20-39: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd management-cluster monitoring validation
CI validates only
PrometheusRulespecs from the regionalalerting-ruleschart. It does not render the management-cluster monitoring chart or coverProbeandAlertmanagerConfig. Add equivalent validation before promotion, or restore the admission webhook after bootstrap.🤖 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/monitoring/values.yaml` around lines 20 - 39, Add CI validation for the management-cluster monitoring chart before promotion, including rendered PrometheusRule, Probe, and AlertmanagerConfig specs, or restore monitoring.admissionWebhooks and tls after bootstrap so admission-time validation is enabled. Update the configuration around admissionWebhooks.enabled and tls.enabled while preserving the bootstrap requirement that certgen hooks do not stall synchronization.docs/design/fips-eks-compute.md-127-128 (1)
127-128: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDocument the
EC2NodeClass.spec.instanceProfilereference.The manifests use the pre-created
${cluster_id}-karpenter-node-roleinstance profile, which wraps the IAM role. Replace “IAM role … is referenced directly” with “instance profile … is referenced” to match the rendered manifests and Terraform resources.🤖 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 `@docs/design/fips-eks-compute.md` around lines 127 - 128, Update the documentation around the EC2NodeClass to state that its spec.instanceProfile references the pre-created ${cluster_id}-karpenter-node-role instance profile, removing the claim that the IAM role is referenced directly.Source: MCP tools
docs/design/fips-eks-compute.md-100-103 (1)
100-103: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCorrect the
metrics-serverownership statement.
metrics-serveris provisioned through Terraform’saws_eks_addon; ArgoCD does not install it. Identify it as an EKS community add-on managed by Terraform, and keepdocs/design/fully-private-eks-bootstrap.mdconsistent.🤖 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 `@docs/design/fips-eks-compute.md` around lines 100 - 103, Update the metrics-server ownership statement in the FIPS EKS compute design and the corresponding statement in fully-private-eks-bootstrap.md: identify metrics-server as an EKS community add-on provisioned and managed through Terraform’s aws_eks_addon, not installed by ArgoCD. Leave the Karpenter ownership description unchanged.Source: MCP tools
docs/design/karpenter-node-provisioning.md-48-48 (1)
48-48: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAlign both documents with Kubernetes scheduling semantics.
bootstrap-criticalonly controls pod priority and preemption. It does not select the bootstrap node group.
docs/design/karpenter-node-provisioning.md#L48-L48: Change the Mermaid edge label to describe priority protection, or document the node selector or affinity that provides placement.docs/sop/karpenter-lifecycle.md#L41-L48: Change the table and prose so they do not claim thatPriorityClassguarantees placement.🤖 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 `@docs/design/karpenter-node-provisioning.md` at line 48, Align the scheduling documentation with Kubernetes semantics: in docs/design/karpenter-node-provisioning.md lines 48-48, revise the Mermaid edge label to describe priority protection or document the actual selector/affinity used for placement; in docs/sop/karpenter-lifecycle.md lines 41-48, update the table and prose so PriorityClass is not described as guaranteeing bootstrap node-group placement.docs/design/zoa-trusted-actions.md-837-837 (1)
837-837: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winScope the OIDC claim to ZOA workloads.
This repository still requires a cluster OIDC provider for Karpenter IRSA. EKS Pod Identity avoids OIDC management for ZOA service accounts; it does not eliminate OIDC management for the cluster. Change the wording to avoid implying that the platform has no OIDC provider management.
Proposed wording
-3. **IRSA (IAM Roles for Service Accounts)**: Allows per-SA roles via annotations. Rejected because EKS Pod Identity is the ZOA platform standard auth mechanism for workload SAs and simplifies IAM configuration by eliminating per-cluster OIDC provider management. IRSA remains supported by AWS but was not chosen for this feature. +3. **IRSA (IAM Roles for Service Accounts)**: Allows per-SA roles via annotations. Rejected because EKS Pod Identity is the ZOA platform standard auth mechanism for workload SAs and avoids an OIDC dependency for ZOA workload ServiceAccounts. IRSA remains supported by AWS but was not chosen for this feature.🤖 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 `@docs/design/zoa-trusted-actions.md` at line 837, Update the IRSA explanation in the EKS authentication comparison to scope the OIDC-management benefit specifically to ZOA workload service accounts, while acknowledging that cluster OIDC may still be required for other integrations such as Karpenter; avoid stating or implying that EKS Pod Identity eliminates OIDC provider management platform-wide.docs/design/thanos-metrics-infrastructure.md-29-29 (1)
29-29: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the document timestamp.
Line 29 changes the compute strategy, but Line 3 still says
2026-03-27. Update the timestamp to the date of this change, such as2026-08-12.Proposed fix
-**Last Updated**: 2026-03-27 +**Last Updated**: 2026-08-12🤖 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 `@docs/design/thanos-metrics-infrastructure.md` at line 29, Update the document timestamp near the top of the design document from the stale date to the date of this change, 2026-08-12, while preserving the revised compute-strategy assumption.docs/sop/karpenter-lifecycle.md-80-90 (1)
80-90: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the shared Karpenter chart path.
Update
argocd/config/shared/karpenter/Chart.yaml. Both ApplicationSets discover this chart throughargocd/config/shared/*; the environment-specific paths do not exist.🤖 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 `@docs/sop/karpenter-lifecycle.md` around lines 80 - 90, Update the Karpenter chart upgrade instructions to reference argocd/config/shared/karpenter/Chart.yaml, since both ApplicationSets discover the shared chart and the environment-specific paths are invalid. Preserve the existing version-update and make pre-push workflow.terraform/modules/eks-cluster/README.md-134-134 (1)
134-134: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winResolve the Argo CD ordering contradiction.
The rendered ApplicationSet manifests define no sync waves. Update both READMEs to state that
karpenterandeks-nodepoolApplications sync concurrently and that retries continue until Karpenter CRDs are available.🤖 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` at line 134, Update the documentation in both README files to remove any Argo CD sync-wave ordering claim. State that the karpenter and eks-nodepool Applications sync concurrently, with retries continuing until Karpenter’s CRDs are available.
🧹 Nitpick comments (6)
terraform/modules/eks-cluster/main.tf (1)
274-291: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider pinning addon versions.
vpc-cni,kube-proxy, andaws-ebs-csi-driverresolve to the EKS default version at create time. Different regions provisioned on different dates then run different addon versions. Pinningaddon_versionand settingresolve_conflicts_on_updatemakes the platform reproducible across regions.🤖 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 274 - 291, Update the aws_eks_addon resources vpc_cni, kube_proxy, and ebs_csi to set explicit, centrally managed addon_version values and configure resolve_conflicts_on_update for deterministic upgrades. Use versions compatible with the cluster Kubernetes version and preserve the existing dependency relationships.Source: Coding guidelines
scripts/buildspec/provision-infra-mc.sh (1)
140-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the parallelism value configurable.
Terraform defaults to 10 concurrent operations, so
-parallelism=20raises concurrency rather than limiting it. Higher concurrency increases the chance of AWS API throttling during large EKS and IAM applies, and the value is hardcoded in this one script only.Read the value from an environment variable so the pipelines can tune it without a code change.
♻️ Proposed change
-terraform "${TERRAFORM_ACTION}" -auto-approve -parallelism=20 +terraform "${TERRAFORM_ACTION}" -auto-approve -parallelism="${TF_PARALLELISM:-20}"Note for the record: the change summary describes this as limiting concurrent operations. It increases them, because the Terraform default is 10.
🤖 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/provision-infra-mc.sh` at line 140, Make the Terraform concurrency setting configurable in the command invoking terraform, replacing the hardcoded 20 in the provisioning flow with an environment-variable value and retaining an appropriate default for existing callers. Use the existing script’s environment-variable conventions and ensure the resulting value is passed to Terraform’s parallelism option.terraform/modules/ecs-bootstrap/variables.tf (1)
85-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winValidate the ARN format, not only the length.
The value is injected into a Kubernetes Secret annotation and an ECS environment variable. A wrong value fails later, when Karpenter starts, and the failure is hard to trace back to Terraform. An ARN pattern check fails at plan time instead.
nullable = falsealso gives a clear error when a caller passesnull.♻️ Proposed validation
variable "karpenter_controller_role_arn" { description = "IAM role ARN for the Karpenter controller (IRSA). Required when the EKS cluster uses self-managed Karpenter." type = string + nullable = false validation { - condition = length(var.karpenter_controller_role_arn) > 0 - error_message = "karpenter_controller_role_arn must not be empty." + condition = can(regex("^arn:aws[a-zA-Z-]*:iam::[0-9]{12}:role/.+$", var.karpenter_controller_role_arn)) + error_message = "karpenter_controller_role_arn must be an IAM role ARN, for example arn:aws:iam::123456789012:role/example." } }🤖 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/variables.tf` around lines 85 - 93, Update the karpenter_controller_role_arn variable validation to require a valid IAM role ARN pattern rather than only a non-empty string, and set nullable = false so null inputs fail during Terraform validation. Preserve the existing descriptive error behavior while making invalid values fail at plan time.argocd/config/regional-cluster/aws-load-balancer-controller/values.yaml (1)
7-17: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSet
regionandvpcIdwhen pods cannot reach IMDS. The controller otherwise depends on IMDS for AWS region and VPC discovery. AddnodeSelectorortolerationsonly if bootstrap nodes are required before Karpenter provisions capacity.🤖 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 - 17, Update the AWS Load Balancer Controller values configuration to explicitly set the AWS region and VPC ID so it does not depend on IMDS for discovery. Use the chart’s existing region and vpcId configuration keys, and add nodeSelector or tolerations only when required for bootstrap-node scheduling before Karpenter provisions capacity.argocd/config/management-cluster/kube-applier/templates/servicemonitor.yaml (1)
7-9: 🩺 Stability & Availability | 🔵 TrivialVerify Argo CD synchronization after the monitoring CRD becomes available.
SkipDryRunOnMissingResource=trueskips the dry run only. It does not install theServiceMonitorCRD or guarantee that the apply succeeds. Confirm that thekube-applierapplication retries or is ordered after the monitoring application, then reachesSyncedandHealthy. Argo CD documents this option as a dry-run-only bypass. (argo-cd.readthedocs.io)As per coding guidelines, “Verify ArgoCD applications sync successfully after relevant changes.”
🤖 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/kube-applier/templates/servicemonitor.yaml` around lines 7 - 9, Verify the kube-applier Argo CD application configuration and synchronization flow after the monitoring Application installs the ServiceMonitor CRD. Ensure kube-applier retries or is ordered after the monitoring Application, then confirm it reaches Synced and Healthy; retain SkipDryRunOnMissingResource only as the dry-run bypass.Sources: Coding guidelines, MCP tools
argocd/config/regional-cluster/monitoring/values.yaml (1)
71-80: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd offline PrometheusRule validation when admission webhooks are disabled.
prometheusOperator.admissionWebhooks.enabled=falseremoves admission-time PromQL validation. Keep this setting if bootstrap requires it, but add a CI check for renderedPrometheusRuleresources with the repository’s supported validator. This prevents malformed rules from reaching the cluster. The chart documents the webhook as the syntax-validation path and documents disabling it as a workaround. (github.com)🤖 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, The monitoring configuration disables PrometheusOperator admission webhooks without adding the required replacement validation. Keep the existing disabled settings in the regional-cluster values, and update the repository’s CI/render-validation workflow to run the supported offline PrometheusRule validator against rendered resources when this chart configuration is used, rejecting malformed rules before deployment.Source: MCP tools
🤖 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 @.spec/002-spec-to-pr-agent/requirements.md:
- Line 57: Expand the dump-env requirement to define safe database-output
boundaries before upload: specify an allowlist of exported fields, redaction of
sensitive spec/status data, encryption, restricted access controls, and
retention/deletion requirements, including behavior when S3_ONLY=true. Ensure
local redaction occurs before any archive is uploaded.
In `@argocd/config/shared/karpenter/values.yaml`:
- Around line 13-16: Update the ApplicationSet value injection for
settings.interruptionQueue to use the SQS queue name from
aws_sqs_queue.karpenter_interruption.name rather than the Terraform
karpenter_queue_url output, while leaving settings.clusterName unchanged.
In `@ci/ephemeral-provider/__init__.py`:
- Line 3: Update the pipeline completion timeout handling associated with
PIPELINE_COMPLETION_TIMEOUT so reaching the 90-minute limit records the timeout
without aborting teardown. Ensure cleanup proceeds through the
pipeline-provisioner and remaining infrastructure phases, or dispatches an
independent cleanup job when continuation in the current flow is not possible.
In `@docs/design/fips-eks-compute.md`:
- Around line 7-11: Update the EKS bootstrap placement design to define explicit
constraints for the karpenter-bootstrap node group and its workloads: add a
stable node label with required node affinity or nodeSelector for Karpenter,
ArgoCD, CoreDNS, and metrics-server, or define a taint on the bootstrap nodes
with matching tolerations. Document the corresponding placement configuration
alongside the bootstrap-critical PriorityClass.
In `@scripts/verify-fips.sh`:
- Around line 130-140: Update the NodePool validation after the CRD check to
require at least one NodePool and ensure each expected NodePool references
EC2NodeClass/fips. Fetch EC2NodeClass/fips itself and validate its specification
contains the required FIPS configuration rather than checking only
nodeClassRef.name; reject standard bottlerocket@v1.64.0 AMI settings without
FIPS support before reporting success.
In `@terraform/modules/ecs-bootstrap/main.tf`:
- Around line 155-168: Update the bootstrap-critical PriorityClass application
block to handle immutable value or preemptionPolicy changes: detect the specific
kubectl apply validation error, then recreate the existing PriorityClass with
kubectl replace --force before reapplying the manifest. Preserve immediate
failure for all other apply errors, and keep the current manifest and idempotent
apply behavior when no immutable-field change occurs.
In `@terraform/modules/ecs-bootstrap/README.md`:
- Line 77: Update the README input table entry for karpenter_controller_role_arn
to mark it as required and remove the empty-string default, matching the
validation in the corresponding variable definition.
In `@terraform/modules/eks-cluster/iam.tf`:
- Around line 311-324: Update the aws_sqs_queue_policy.karpenter_interruption
statement to add an IAM Condition restricting events.amazonaws.com to the owning
AWS account and/or the specific authorized EventBridge rule ARN(s), using the
module’s existing account or rule symbols where available. Preserve the existing
SendMessage permission while preventing other accounts or rules from publishing
to the interruption queue.
In `@terraform/modules/eks-cluster/main.tf`:
- Around line 139-176: Configure the destroy provisioner containing the
Karpenter instance cleanup command to use an explicit Bash interpreter instead
of Terraform’s default /bin/sh. Update the provisioner’s interpreter settings
near the existing command so set -euo pipefail executes reliably, while
preserving the current cleanup behavior.
In `@terraform/modules/eks-cluster/outputs.tf`:
- Around line 99-102: Update the karpenter_queue_url output to export the
interruption queue name required by Karpenter settings.interruptionQueue, using
the existing aws_sqs_queue.karpenter_interruption resource’s name attribute
instead of its URL while preserving the output’s current purpose and
description.
---
Minor comments:
In `@argocd/config/management-cluster/hypershift/templates/05-job.yaml`:
- Line 9: Adjust activeDeadlineSeconds in the Job configuration to accommodate
the Prometheus Operator CRD wait plus hypershift install and both patch
operations, ensuring the Job is not terminated before the diagnostic output
executes. Prefer shortening the CRD wait to the suggested 900 seconds if that
preserves the intended behavior; otherwise increase activeDeadlineSeconds
accordingly.
- Around line 153-180: Make the external-dns patches in the Job idempotent by
reading each resource before patching and checking whether the desired argument
or RBAC rules already exist. Only execute the deployment PATCH when the exact
--aws-assume-role value is absent, and only add each ClusterRole rule when it is
absent; retain the existing HTTP status logging for patches that run and skip
already-configured entries.
In `@argocd/config/management-cluster/monitoring/values.yaml`:
- Around line 20-39: Add CI validation for the management-cluster monitoring
chart before promotion, including rendered PrometheusRule, Probe, and
AlertmanagerConfig specs, or restore monitoring.admissionWebhooks and tls after
bootstrap so admission-time validation is enabled. Update the configuration
around admissionWebhooks.enabled and tls.enabled while preserving the bootstrap
requirement that certgen hooks do not stall synchronization.
In `@argocd/config/shared/karpenter/Chart.yaml`:
- Around line 6-9: Add explicit Karpenter CRD management in the Chart.yaml
dependency configuration alongside the existing karpenter controller dependency.
Include the karpenter-crd chart at version 1.14.0 and ensure the dependency is
applied for both cluster types while keeping it synchronized with the controller
upgrade.
In `@docs/design/fips-eks-compute.md`:
- Around line 127-128: Update the documentation around the EC2NodeClass to state
that its spec.instanceProfile references the pre-created
${cluster_id}-karpenter-node-role instance profile, removing the claim that the
IAM role is referenced directly.
- Around line 100-103: Update the metrics-server ownership statement in the FIPS
EKS compute design and the corresponding statement in
fully-private-eks-bootstrap.md: identify metrics-server as an EKS community
add-on provisioned and managed through Terraform’s aws_eks_addon, not installed
by ArgoCD. Leave the Karpenter ownership description unchanged.
In `@docs/design/karpenter-node-provisioning.md`:
- Line 48: Align the scheduling documentation with Kubernetes semantics: in
docs/design/karpenter-node-provisioning.md lines 48-48, revise the Mermaid edge
label to describe priority protection or document the actual selector/affinity
used for placement; in docs/sop/karpenter-lifecycle.md lines 41-48, update the
table and prose so PriorityClass is not described as guaranteeing bootstrap
node-group placement.
In `@docs/design/thanos-metrics-infrastructure.md`:
- Line 29: Update the document timestamp near the top of the design document
from the stale date to the date of this change, 2026-08-12, while preserving the
revised compute-strategy assumption.
In `@docs/design/zoa-trusted-actions.md`:
- Line 837: Update the IRSA explanation in the EKS authentication comparison to
scope the OIDC-management benefit specifically to ZOA workload service accounts,
while acknowledging that cluster OIDC may still be required for other
integrations such as Karpenter; avoid stating or implying that EKS Pod Identity
eliminates OIDC provider management platform-wide.
In `@docs/sop/karpenter-lifecycle.md`:
- Around line 80-90: Update the Karpenter chart upgrade instructions to
reference argocd/config/shared/karpenter/Chart.yaml, since both ApplicationSets
discover the shared chart and the environment-specific paths are invalid.
Preserve the existing version-update and make pre-push workflow.
In `@terraform/config/management-cluster/main.tf`:
- Around line 61-66: Correct the module comment describing the one-time
bootstrap task: remove the claim that it installs Karpenter, and state that it
installs ArgoCD, which subsequently installs Karpenter through GitOps. Preserve
the existing description of bootstrap completion and ongoing managed-node-group
operation.
In `@terraform/modules/aws-load-balancer-controller/iam.tf`:
- Around line 8-10: Add validation for the cluster_name input used by
aws_iam_role.aws_lbc, enforcing a maximum length of 35 characters to keep the
generated IAM role name within its limit. Preserve existing cluster_name
behavior for valid values and provide a clear validation error for longer names.
In `@terraform/modules/eks-cluster/README.md`:
- Line 134: Update the documentation in both README files to remove any Argo CD
sync-wave ordering claim. State that the karpenter and eks-nodepool Applications
sync concurrently, with retries continuing until Karpenter’s CRDs are available.
In `@terraform/modules/eks-cluster/variables.tf`:
- Around line 78-82: Update terraform/modules/eks-cluster/variables.tf lines
78-82 to document the implemented KMS actions, kms:CreateGrant and
kms:DescribeKey, granted only to the controller role, and remove the
RHEL-specific wording. Update the related block comment in
terraform/modules/eks-cluster/iam.tf lines 264-295 to describe the Bottlerocket
AMI; do not add node-role permissions unless a KMS-encrypted custom AMI remains
in scope.
---
Nitpick comments:
In `@argocd/config/management-cluster/kube-applier/templates/servicemonitor.yaml`:
- Around line 7-9: Verify the kube-applier Argo CD application configuration and
synchronization flow after the monitoring Application installs the
ServiceMonitor CRD. Ensure kube-applier retries or is ordered after the
monitoring Application, then confirm it reaches Synced and Healthy; retain
SkipDryRunOnMissingResource only as the dry-run bypass.
In `@argocd/config/regional-cluster/aws-load-balancer-controller/values.yaml`:
- Around line 7-17: Update the AWS Load Balancer Controller values configuration
to explicitly set the AWS region and VPC ID so it does not depend on IMDS for
discovery. Use the chart’s existing region and vpcId configuration keys, and add
nodeSelector or tolerations only when required for bootstrap-node scheduling
before Karpenter provisions capacity.
In `@argocd/config/regional-cluster/monitoring/values.yaml`:
- Around line 71-80: The monitoring configuration disables PrometheusOperator
admission webhooks without adding the required replacement validation. Keep the
existing disabled settings in the regional-cluster values, and update the
repository’s CI/render-validation workflow to run the supported offline
PrometheusRule validator against rendered resources when this chart
configuration is used, rejecting malformed rules before deployment.
In `@scripts/buildspec/provision-infra-mc.sh`:
- Line 140: Make the Terraform concurrency setting configurable in the command
invoking terraform, replacing the hardcoded 20 in the provisioning flow with an
environment-variable value and retaining an appropriate default for existing
callers. Use the existing script’s environment-variable conventions and ensure
the resulting value is passed to Terraform’s parallelism option.
In `@terraform/modules/ecs-bootstrap/variables.tf`:
- Around line 85-93: Update the karpenter_controller_role_arn variable
validation to require a valid IAM role ARN pattern rather than only a non-empty
string, and set nullable = false so null inputs fail during Terraform
validation. Preserve the existing descriptive error behavior while making
invalid values fail at plan time.
In `@terraform/modules/eks-cluster/main.tf`:
- Around line 274-291: Update the aws_eks_addon resources vpc_cni, kube_proxy,
and ebs_csi to set explicit, centrally managed addon_version values and
configure resolve_conflicts_on_update for deterministic upgrades. Use versions
compatible with the cluster Kubernetes version and preserve the existing
dependency relationships.
🪄 Autofix
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: 571f45af-2932-40ac-92a7-5a721616bb88
📒 Files selected for processing (84)
.gitignore.spec/002-spec-to-pr-agent/requirements.mdMakefileargocd/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/kube-applier/templates/servicemonitor.yamlargocd/config/management-cluster/monitoring/values.yamlargocd/config/regional-cluster/alerting-rules/templates/hcp-installation.yamlargocd/config/regional-cluster/alerting-rules/templates/hcp-sla.yamlargocd/config/regional-cluster/alerting-rules/templates/hcp-state.yamlargocd/config/regional-cluster/alerting-rules/templates/ratelimit.yamlargocd/config/regional-cluster/alerting-rules/templates/remote-write-health.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/loki/templates/targetgroupbinding.yamlargocd/config/regional-cluster/monitoring/templates/sre-targetgroupbinding.yamlargocd/config/regional-cluster/monitoring/values.yamlargocd/config/regional-cluster/platform-api/templates/servicemonitor.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/karpenter/Chart.yamlargocd/config/shared/karpenter/templates/.gitkeepargocd/config/shared/karpenter/values.yamlargocd/config/shared/storageclass/templates/gp3.yamlci/ephemeral-provider/__init__.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.mddocs/sop/karpenter-lifecycle.mdscripts/bootstrap-argocd.shscripts/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
Comment on lines
+7
to
+11
| All EKS clusters in the ROSA HyperFleet use self-managed Karpenter v1 with an `EC2NodeClass` (`fips`) | ||
| and a cluster-type-specific `NodePool` for platform and application workloads. A dedicated | ||
| `karpenter-bootstrap` managed node group (m7i.xlarge, 2 nodes, scheduled via the | ||
| `bootstrap-critical` PriorityClass) provides stable capacity for Karpenter itself, CoreDNS, and | ||
| metrics-server. All other workloads land on Karpenter-provisioned nodes. |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Expect: bootstrap-critical workloads have explicit placement constraints.
rg -n -C 8 \
'bootstrap-critical|CriticalAddonsOnly|priorityClassName|nodeSelector|nodeAffinity|tolerations|karpenter-bootstrap' \
--glob '*.yaml' --glob '*.yml' --glob '*.tpl' --glob '*.j2' .Repository: openshift-online/rosa-hyperfleet
Length of output: 15788
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- design document ---'
sed -n '1,180p' docs/design/fips-eks-compute.md
printf '%s\n' '--- bootstrap and placement definitions ---'
rg -n -C 6 \
'karpenter-bootstrap|bootstrap-critical|CriticalAddonsOnly|nodeSelector|nodeAffinity|taints:|labels:|metrics-server|karpenter-node-role|instanceProfile|EC2NodeClass' \
--glob '*.tf' --glob '*.yaml' --glob '*.yml' --glob '*.tpl' --glob '*.j2' --glob '*.md' .
printf '%s\n' '--- candidate files ---'
git ls-files | rg '(^|/)(fips-eks-compute\.md|.*bootstrap.*|.*karpenter.*|.*metrics.*|.*monitoring.*)$' | head -200Repository: openshift-online/rosa-hyperfleet
Length of output: 50391
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- bootstrap node-group configuration ---'
rg -n -C 12 \
'karpenter.?bootstrap|bootstrap.?node|managed_node_group|node_group|taints|labels|capacity_type|instance_types' \
terraform config --glob '*.tf' --glob '*.tfvars' --glob '*.yaml' --glob '*.yml' \
| rg -n -C 4 'karpenter|bootstrap|taints|labels|node_group|managed_node_group'
printf '%s\n' '--- addon configuration ---'
rg -n -C 12 \
'coredns|metrics-server|addon|configuration_values|resolve_conflicts|nodeSelector|affinity|tolerations' \
terraform config argocd --glob '*.tf' --glob '*.yaml' --glob '*.yml' --glob '*.tpl' --glob '*.j2' \
| rg -n -C 4 'coredns|metrics-server|addon|nodeSelector|affinity|tolerations|bootstrap'
printf '%s\n' '--- exact relevant SOP and values ---'
sed -n '37,49p' docs/sop/karpenter-lifecycle.md
sed -n '1,45p' argocd/config/shared/karpenter/values.yaml
sed -n '1,35p' argocd/config/shared/argocd/values.yamlRepository: openshift-online/rosa-hyperfleet
Length of output: 50388
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- node-group resource ---'
sed -n '214,265p' terraform/modules/eks-cluster/main.tf
printf '%s\n' '--- addon resources ---'
sed -n '180,212p' terraform/modules/eks-cluster/main.tf
printf '%s\n' '--- Karpenter and ArgoCD scheduling values ---'
sed -n '1,55p' argocd/config/shared/karpenter/values.yaml
sed -n '1,25p' argocd/config/shared/argocd/values.yaml
printf '%s\n' '--- all bootstrap placement fields in relevant configuration ---'
rg -n \
'nodeSelector|nodeAffinity|affinity:|tolerations:|taints:|labels:|bootstrap-critical|priorityClassName' \
terraform/modules/eks-cluster/main.tf \
terraform/modules/ecs-bootstrap/main.tf \
argocd/config/shared/karpenter/values.yaml \
argocd/config/shared/argocd/values.yamlRepository: openshift-online/rosa-hyperfleet
Length of output: 8237
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
node_group = Path("terraform/modules/eks-cluster/main.tf").read_text()
resource = re.search(
r'resource "aws_eks_node_group" "karpenter_bootstrap"\s*\{(.*?)\n\}',
node_group,
re.S,
).group(1)
karpenter = Path("argocd/config/shared/karpenter/values.yaml").read_text()
argocd = Path("argocd/config/shared/argocd/values.yaml").read_text()
addons = node_group[node_group.index('resource "aws_eks_addon" "coredns"'):
node_group.index("# -----------------------------------------------------------------------------\n# Karpenter Bootstrap Node Group")]
print("bootstrap node group has taints:", bool(re.search(r"^\s+taints\s*\{", resource, re.M)))
print("bootstrap node group has Kubernetes labels:", bool(re.search(r"^\s+labels\s*\{", resource, re.M)))
print("bootstrap node group has only AWS tags:", bool(re.search(r"^\s+tags\s*\{", resource, re.M)))
print("Karpenter sets bootstrap-critical:", "priorityClassName: bootstrap-critical" in karpenter)
print("Karpenter sets nodeSelector:", bool(re.search(r"^\s+nodeSelector:", karpenter, re.M)))
print("Karpenter sets node affinity:", bool(re.search(r"^\s+affinity:", karpenter, re.M)))
print("ArgoCD sets bootstrap-critical:", "priorityClassName: bootstrap-critical" in argocd)
print("ArgoCD sets nodeSelector:", bool(re.search(r"^\s+nodeSelector:", argocd, re.M)))
print("ArgoCD sets node affinity:", bool(re.search(r"^\s+affinity:", argocd, re.M)))
print("CoreDNS addon supplies placement configuration:", "configuration_values" in addons[addons.index('resource "aws_eks_addon" "coredns"'):addons.index('resource "aws_eks_addon" "pod_identity"')])
print("metrics-server addon supplies placement configuration:", "configuration_values" in addons[addons.index('resource "aws_eks_addon" "metrics_server"'):addons.index('resource "aws_eks_addon" "pod_identity"')])
PYRepository: openshift-online/rosa-hyperfleet
Length of output: 633
Add explicit bootstrap-node placement constraints. bootstrap-critical only controls priority and preemption. The bootstrap node group has no Kubernetes labels or taints, and Karpenter, ArgoCD, CoreDNS, and metrics-server have no node-selection rules. Add a stable node label with required node affinity or nodeSelector for these workloads, or use a taint with matching tolerations.
🤖 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 `@docs/design/fips-eks-compute.md` around lines 7 - 11, Update the EKS
bootstrap placement design to define explicit constraints for the
karpenter-bootstrap node group and its workloads: add a stable node label with
required node affinity or nodeSelector for Karpenter, ArgoCD, CoreDNS, and
metrics-server, or define a taint on the bootstrap nodes with matching
tolerations. Document the corresponding placement configuration alongside the
bootstrap-critical PriorityClass.
Source: MCP tools
Comment on lines
+311
to
+324
| 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 | ||
| }] | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Restrict the SQS queue policy to your own EventBridge rules.
The statement allows events.amazonaws.com to call sqs:SendMessage with no aws:SourceArn or aws:SourceAccount condition. Any AWS account can point an EventBridge rule at this queue and inject messages that Karpenter processes as interruption events. Add a source condition.
Least-privilege IAM for service roles is required by the coding guidelines for terraform/**/*.tf.
🔒 Proposed fix
Statement = [{
Sid = "AllowEventBridge"
Effect = "Allow"
Principal = { Service = "events.amazonaws.com" }
Action = "sqs:SendMessage"
Resource = aws_sqs_queue.karpenter_interruption.arn
+ Condition = {
+ ArnEquals = {
+ "aws:SourceArn" = [for r in aws_cloudwatch_event_rule.karpenter : r.arn]
+ }
+ StringEquals = {
+ "aws:SourceAccount" = data.aws_caller_identity.current.account_id
+ }
+ }
}]📝 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.
Suggested change
| 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 | |
| }] | |
| }) | |
| } | |
| 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 | |
| Condition = { | |
| ArnEquals = { | |
| "aws:SourceArn" = [for r in aws_cloudwatch_event_rule.karpenter : r.arn] | |
| } | |
| StringEquals = { | |
| "aws:SourceAccount" = data.aws_caller_identity.current.account_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 311 - 324, Update the
aws_sqs_queue_policy.karpenter_interruption statement to add an IAM Condition
restricting events.amazonaws.com to the owning AWS account and/or the specific
authorized EventBridge rule ARN(s), using the module’s existing account or rule
symbols where available. Preserve the existing SendMessage permission while
preventing other accounts or rules from publishing to the interruption queue.
Source: Coding guidelines
Contributor
Author
|
/test on-demand-e2e |
Verified each review finding against current code before fixing. Real bugs fixed: teardown orchestrator could abort before destroying the pipeline-provisioner on a monitor timeout; the bootstrap-critical PriorityClass couldn't actually be re-applied with a changed value/ preemptionPolicy since those fields are immutable; the destroy-time provisioner relied on /bin/sh defaulting to bash semantics (pipefail); the SQS interruption queue policy allowed any account's EventBridge to publish; verify-fips.sh vacuously passed with zero NodePools and never validated the "fips" EC2NodeClass's actual AMI; the hypershift-install Job's external-dns JSON patches would duplicate args/rules on every re-run since the Job replaces itself on each ArgoCD sync. Also corrected several stale docs (removed karpenter symlink paths, wrong sync-wave claims, an overclaimed IRSA/OIDC scope, a stale timestamp) and tightened a few Terraform variable validations/descriptions to match actual behavior. Skipped as stale or out of scope (with reasons): karpenter/values.yaml interruptionQueue already resolves to a queue name via ApplicationSet templating, not the unused karpenter_queue_url output; the karpenter Helm chart already bundles its own CRDs (verified via `helm pull`), so no separate karpenter-crd dependency exists to add; the monitoring admissionWebhooks tradeoff is an already-documented, deliberate decision (ArgoCD#21055) — adding CI-side PrometheusRule validation is a larger, separate change; pinning EKS addon versions can't be done safely without live AWS compatibility data; a few other findings were already correct or already covered by the project's established eventual-consistency ArgoCD sync model. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
Author
|
/test on-demand-e2e |
Contributor
Author
|
/test check-docs |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 @.spec/002-spec-to-pr-agent/requirements.md:
- Around line 57-62: Expand the dump-env requirement with concrete archive
controls: enumerate the permitted exported fields and local redaction rules,
require encryption using a specified KMS key and policy, identify the permitted
AWS IAM principals, and define the retention duration and deletion owner. Add
verifiable acceptance criteria covering both normal and S3_ONLY=true uploads,
ensuring redaction completes before upload and that both paths enforce the same
allowlist, encryption, access, and retention controls.
In `@ci/ephemeral-provider/orchestrator.py`:
- Around line 794-800: Update the teardown flow around
_destroy_pipeline_provisioner so its exception is caught and appended to the
existing failed collection before raising the final RuntimeError. Preserve the
pipeline failures and include the Phase 3 destroy failure in the single final
error, while still attempting the provisioner destroy.
- Around line 749-753: Update the teardown orchestration around the Phase
1/Phase 2 failure handling and _destroy_pipeline_provisioner so failures stop
before deleting target pipelines or destroying the provisioner. Preserve the
failed targets for retry, and invoke Phase 3 only after target teardown
completes successfully; otherwise implement a verified fallback that removes the
target RC/MC infrastructure before provisioner destruction.
In `@scripts/verify-fips.sh`:
- Around line 176-184: Update the FIPS NodeClass configuration to select the
approved FIPS-validated Bottlerocket image instead of standard alias
bottlerocket@v1.64.0, and revise the verifier logic around bottlerocket_alias to
require explicit evidence of that approved configuration. Reject empty
selectors, AL2023, arbitrary custom AMIs, and standard Bottlerocket aliases;
keep the success message contingent on the configured NodeClass matching the
approved FIPS image.
- Around line 143-150: Update the nodepool retrieval in the FIPS verification
function to preserve kubectl’s exit status and capture its stderr instead of
discarding it. Check for command failure before running jq, report the captured
kubectl error through the existing failure path, and only report “No NodePools
found” when the command succeeds with an empty result.
In `@terraform/modules/aws-load-balancer-controller/variables.tf`:
- Around line 6-9: Update the cluster_name validation condition in the variable
definition to allow a maximum length of 35 characters, reflecting the exact
29-character suffix used by aws_iam_role.aws_lbc.name. Revise the adjacent
comment and error_message to state the 35-character limit and resulting
64-character IAM role-name boundary.
🪄 Autofix
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: 44e07a2c-fa85-40e1-911d-383fce8a25fd
📒 Files selected for processing (19)
.spec/002-spec-to-pr-agent/requirements.mdargocd/config/management-cluster/hypershift/templates/05-job.yamlci/ephemeral-provider/orchestrator.pydocs/design/fips-eks-compute.mddocs/design/karpenter-node-provisioning.mddocs/design/thanos-metrics-infrastructure.mddocs/design/zoa-trusted-actions.mddocs/sop/karpenter-lifecycle.mdscripts/buildspec/provision-infra-mc.shscripts/verify-fips.shterraform/config/management-cluster/main.tfterraform/modules/aws-load-balancer-controller/variables.tfterraform/modules/ecs-bootstrap/README.mdterraform/modules/ecs-bootstrap/main.tfterraform/modules/ecs-bootstrap/variables.tfterraform/modules/eks-cluster/iam.tfterraform/modules/eks-cluster/main.tfterraform/modules/eks-cluster/outputs.tfterraform/modules/eks-cluster/variables.tf
🚧 Files skipped from review as they are similar to previous changes (12)
- terraform/modules/ecs-bootstrap/variables.tf
- terraform/config/management-cluster/main.tf
- terraform/modules/eks-cluster/variables.tf
- docs/design/thanos-metrics-infrastructure.md
- argocd/config/management-cluster/hypershift/templates/05-job.yaml
- docs/design/karpenter-node-provisioning.md
- terraform/modules/eks-cluster/outputs.tf
- terraform/modules/ecs-bootstrap/main.tf
- docs/design/fips-eks-compute.md
- terraform/modules/eks-cluster/main.tf
- docs/sop/karpenter-lifecycle.md
- terraform/modules/eks-cluster/iam.tf
Comment on lines
+57
to
+62
| - **dump-env**: Gather Kubernetes logs and DB state from clusters. Exported fields MUST be | ||
| limited to an explicit allowlist; sensitive spec/status data (credentials, tokens, customer | ||
| data) MUST be redacted locally before archiving, and redaction MUST complete before any | ||
| archive is uploaded. Uploaded archives MUST be encrypted and access-restricted, with a defined | ||
| retention/deletion period. When `S3_ONLY=true`, the same allowlist/redaction/encryption | ||
| requirements apply to the uploaded archive — local-only output is not a substitute |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Define the archive security controls.
The requirement names an allowlist, encryption, access restriction, and retention. It does not define them. An implementation cannot verify compliance without the allowed fields, redaction rules, KMS key policy, permitted IAM principals, retention duration, and deletion owner.
Specify these controls and add acceptance criteria for normal and S3_ONLY=true uploads.
As per coding guidelines: “Use AWS IAM for all authentication and authorization.”
🤖 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 @.spec/002-spec-to-pr-agent/requirements.md around lines 57 - 62, Expand the
dump-env requirement with concrete archive controls: enumerate the permitted
exported fields and local redaction rules, require encryption using a specified
KMS key and policy, identify the permitted AWS IAM principals, and define the
retention duration and deletion owner. Add verifiable acceptance criteria
covering both normal and S3_ONLY=true uploads, ensuring redaction completes
before upload and that both paths enforce the same allowlist, encryption,
access, and retention controls.
Source: Coding guidelines
Comment on lines
+794
to
+800
| self._destroy_pipeline_provisioner(git) | ||
|
|
||
| if failed: | ||
| raise RuntimeError( | ||
| f"{len(failed)} pipeline(s) failed during teardown: {', '.join(failed)} " | ||
| "(pipeline-provisioner destroy was still attempted)" | ||
| ) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Preserve all teardown failures in the final error.
If _destroy_pipeline_provisioner() raises, execution skips Lines 796-800. The caller then receives only the Phase 3 exception, not the earlier pipeline failures.
Catch the Phase 3 exception, add it to failed, and raise one error that reports every failed phase.
🤖 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 794 - 800, Update the
teardown flow around _destroy_pipeline_provisioner so its exception is caught
and appended to the existing failed collection before raising the final
RuntimeError. Preserve the pipeline failures and include the Phase 3 destroy
failure in the single final error, while still attempting the provisioner
destroy.
Comment on lines
+143
to
+150
| local nodepools_json | ||
| nodepools_json=$(kubectl get nodepool -o json 2>/dev/null) | ||
|
|
||
| local pool_count | ||
| pool_count=$(echo "$nodepools_json" | jq -r '.items | length') | ||
| if [[ "$pool_count" -eq 0 ]]; then | ||
| fail "No NodePools found — cannot verify FIPS NodeClass usage" | ||
| return |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Handle kubectl get nodepool failures.
Line 144 discards the command error and does not check its exit status. A Kubernetes API or RBAC failure can appear as “No NodePools found” instead of the real failure.
Capture stdout and stderr with the exit status. Report the kubectl error before parsing JSON.
As per path instructions: “Review shell scripts for command injection vulnerabilities and proper error handling.”
🤖 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/verify-fips.sh` around lines 143 - 150, Update the nodepool retrieval
in the FIPS verification function to preserve kubectl’s exit status and capture
its stderr instead of discarding it. Check for command failure before running
jq, report the captured kubectl error through the existing failure path, and
only report “No NodePools found” when the command succeeds with an empty result.
Source: Path instructions
Comment on lines
+176
to
+184
| local bottlerocket_alias | ||
| bottlerocket_alias=$(echo "$nodeclass_check" | jq -r \ | ||
| '.spec.amiSelectorTerms[]? | select((.alias // "") | startswith("bottlerocket@")) | .alias') | ||
| if [[ -n "$bottlerocket_alias" ]]; then | ||
| fail "EC2NodeClass/fips uses a standard Bottlerocket AMI ($bottlerocket_alias), not a FIPS-validated AMI" | ||
| return | ||
| fi | ||
|
|
||
| pass "All NodePools reference the FIPS NodeClass, and EC2NodeClass/fips does not use a standard Bottlerocket AMI" |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Align the FIPS NodeClass configuration with this validation.
The deployed argocd/config/regional-cluster/eks-nodepool/templates/00-nodeclass.yaml selects bottlerocket@v1.64.0. Line 178 detects that alias, so this check fails for the configured NodeClass.
The negative predicate also accepts an empty selector, AL2023, or an arbitrary custom AMI. Require explicit evidence of the approved FIPS image configuration. Update the NodeClass and verifier together.
🤖 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/verify-fips.sh` around lines 176 - 184, Update the FIPS NodeClass
configuration to select the approved FIPS-validated Bottlerocket image instead
of standard alias bottlerocket@v1.64.0, and revise the verifier logic around
bottlerocket_alias to require explicit evidence of that approved configuration.
Reject empty selectors, AL2023, arbitrary custom AMIs, and standard Bottlerocket
aliases; keep the success message contingent on the configured NodeClass
matching the approved FIPS image.
Comment on lines
+6
to
+9
| # IAM role names are capped at 64 chars; the "-aws-load-balancer-controller" | ||
| # suffix (30 chars) leaves 34 chars for cluster_name. | ||
| condition = length(var.cluster_name) <= 34 | ||
| error_message = "cluster_name must be 34 characters or fewer so the generated IAM role name (\"<cluster_name>-aws-load-balancer-controller\") stays within IAM's 64-character role name limit." |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
suffix='-aws-load-balancer-controller'
test "${`#suffix`}" -eq 29
test "$((64 - ${`#suffix`}))" -eq 35
rg -n -C 6 \
'resource "aws_iam_role" "aws_lbc"|name\s*=.*aws-load-balancer-controller' \
terraform/modules/aws-load-balancer-controller/iam.tfRepository: openshift-online/rosa-hyperfleet
Length of output: 1483
Correct the IAM role-name boundary.
aws_iam_role.aws_lbc.name uses the exact 29-character suffix. Set the maximum cluster_name length to 35 characters and update the comment and error message accordingly. A 35-character cluster_name produces a 64-character IAM role name.
🤖 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/variables.tf` around lines 6 -
9, Update the cluster_name validation condition in the variable definition to
allow a maximum length of 35 characters, reflecting the exact 29-character
suffix used by aws_iam_role.aws_lbc.name. Revise the adjacent comment and
error_message to state the 35-character limit and resulting 64-character IAM
role-name boundary.
…e ones" This reverts commit 595e732.
Contributor
Author
|
/test on-demand-e2e |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/design/thanos-metrics-infrastructure.md (1)
29-29: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRefresh
Last Updatedfor this architecture change.Line 29 changes an architecture assumption, but the header still says
2026-03-27. Set it to the PR’s actual change date. For this review, that date is August 12, 2026.Suggested update
-**Last Updated**: 2026-03-27 +**Last Updated**: 2026-08-12As per coding guidelines: “Use the documentation-updater agent for documentation freshness reviews.”
🤖 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 `@docs/design/thanos-metrics-infrastructure.md` at line 29, Update the document’s “Last Updated” header to August 12, 2026 to reflect the architecture change, leaving the revised assumptions unchanged.Source: Coding guidelines
scripts/verify-fips.sh (1)
148-153:⚠️ Potential issue | 🟠 MajorDo not report FIPS success from a name-only binding check.
The empty string is treated as success, so a cluster with no NodePools passes. A NodePool that references a missing
EC2NodeClass/fipsalso passes. The check does not validategroup,kind, orEC2NodeClass/fips.spec. Require at least one expected NodePool, validate the full reference, fetch the class, and validate the approved FIPS configuration beforepass.The NodePool contract in
argocd/config/regional-cluster/eks-nodepool/templates/10-nodepool.yaml:1-15includesgroup,kind, andname. Confirm the approved FIPS configuration before hard-coding the verifier.#!/usr/bin/env bash set -euo pipefail rg -n -C 8 \ 'kind: EC2NodeClass|name: fips|nodeClassRef|amiSelectorTerms|userData' \ argocd/config/management-cluster/eks-nodepool \ argocd/config/regional-cluster/eks-nodepool \ scripts/verify-fips.sh🤖 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/verify-fips.sh` around lines 148 - 153, Replace the name-only check in the FIPS verification flow with validation that at least one expected NodePool exists, every NodePool references group, kind, and name exactly as required, and the referenced EC2NodeClass named fips exists. Validate that EC2NodeClass fips.spec matches the approved FIPS configuration established by the NodePool templates before calling pass; retain failure handling for missing or invalid resources.
🤖 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 @.spec/002-spec-to-pr-agent/requirements.md:
- Line 57: Expand the dump-env requirement with acceptance criteria covering an
exported-field allowlist, pre-upload redaction, encryption, AWS IAM principals,
retention and deletion, and S3_ONLY=true behavior. State that identical controls
apply to both local and S3-only archive paths, and require AWS IAM for all
authentication and authorization.
- Line 57: Update the dump-env requirement to explicitly state that Kubernetes
logs are gathered from management and regional clusters, while PostgreSQL
database state is gathered from the regional cluster only.
In `@argocd/config/management-cluster/hypershift/templates/05-job.yaml`:
- Around line 161-179: Update the external-dns PATCH handling in the job script
so any non-200/201 response exits the Job with a nonzero status instead of only
warning. Apply this to both the deployment patch check and the required
ClusterRole patch check, preserving the HTTP status in the error output before
exiting so Argo CD can retry the failed operation.
---
Outside diff comments:
In `@docs/design/thanos-metrics-infrastructure.md`:
- Line 29: Update the document’s “Last Updated” header to August 12, 2026 to
reflect the architecture change, leaving the revised assumptions unchanged.
In `@scripts/verify-fips.sh`:
- Around line 148-153: Replace the name-only check in the FIPS verification flow
with validation that at least one expected NodePool exists, every NodePool
references group, kind, and name exactly as required, and the referenced
EC2NodeClass named fips exists. Validate that EC2NodeClass fips.spec matches the
approved FIPS configuration established by the NodePool templates before calling
pass; retain failure handling for missing or invalid resources.
🪄 Autofix
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: 52da8997-db60-49b1-9647-527eb31ea3dd
📒 Files selected for processing (18)
.spec/002-spec-to-pr-agent/requirements.mdargocd/config/management-cluster/hypershift/templates/05-job.yamldocs/design/fips-eks-compute.mddocs/design/karpenter-node-provisioning.mddocs/design/thanos-metrics-infrastructure.mddocs/design/zoa-trusted-actions.mddocs/sop/karpenter-lifecycle.mdscripts/buildspec/provision-infra-mc.shscripts/verify-fips.shterraform/config/management-cluster/main.tfterraform/modules/aws-load-balancer-controller/variables.tfterraform/modules/ecs-bootstrap/README.mdterraform/modules/ecs-bootstrap/main.tfterraform/modules/ecs-bootstrap/variables.tfterraform/modules/eks-cluster/iam.tfterraform/modules/eks-cluster/main.tfterraform/modules/eks-cluster/outputs.tfterraform/modules/eks-cluster/variables.tf
💤 Files with no reviewable changes (1)
- terraform/modules/aws-load-balancer-controller/variables.tf
🚧 Files skipped from review as they are similar to previous changes (11)
- terraform/config/management-cluster/main.tf
- docs/design/zoa-trusted-actions.md
- terraform/modules/ecs-bootstrap/variables.tf
- terraform/modules/eks-cluster/variables.tf
- docs/sop/karpenter-lifecycle.md
- docs/design/karpenter-node-provisioning.md
- terraform/modules/eks-cluster/outputs.tf
- terraform/modules/eks-cluster/iam.tf
- terraform/modules/ecs-bootstrap/README.md
- docs/design/fips-eks-compute.md
- terraform/modules/eks-cluster/main.tf
| - **list**: Display all tracked environments with status | ||
| - **e2e**: Run end-to-end tests against an environment | ||
| - **dump-env**: Dump environment state (Kubernetes logs and DB state) from clusters | ||
| - **dump-env**: Gather Kubernetes logs and DB state from clusters |
Contributor
There was a problem hiding this comment.
Define archive controls for dump-env.
The new line names Kubernetes logs and database state, but it still does not define the exported-field allowlist, redaction, encryption, IAM principals, retention, deletion, or S3_ONLY=true behavior. Add acceptance criteria that require redaction before upload and apply the same controls to local and S3-only paths.
This repeats the existing archive-control finding.
As per coding guidelines: “Use AWS IAM for all authentication and authorization.”
🤖 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 @.spec/002-spec-to-pr-agent/requirements.md at line 57, Expand the dump-env
requirement with acceptance criteria covering an exported-field allowlist,
pre-upload redaction, encryption, AWS IAM principals, retention and deletion,
and S3_ONLY=true behavior. State that identical controls apply to both local and
S3-only archive paths, and require AWS IAM for all authentication and
authorization.
Source: Coding guidelines
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Name the source cluster for each dump.
scripts/dev/dump-env.sh collects Kubernetes logs from management and regional clusters, but it collects PostgreSQL database state from the regional cluster only. “DB state from clusters” is ambiguous and can lead to an incorrect implementation. State the exact source for each data type.
Suggested wording
- - **dump-env**: Gather Kubernetes logs and DB state from clusters
+ - **dump-env**: Gather Kubernetes logs from management and regional clusters and PostgreSQL database state from the regional cluster📝 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.
Suggested change
| - **dump-env**: Gather Kubernetes logs and DB state from clusters | |
| - **dump-env**: Gather Kubernetes logs from management and regional clusters and PostgreSQL database state from the regional cluster |
🤖 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 @.spec/002-spec-to-pr-agent/requirements.md at line 57, Update the dump-env
requirement to explicitly state that Kubernetes logs are gathered from
management and regional clusters, while PostgreSQL database state is gathered
from the regional cluster only.
Comment on lines
+161
to
+179
| if [ "$_patch_http" != "200" ] && [ "$_patch_http" != "201" ]; then | ||
| echo "WARNING: external-dns deployment patch returned HTTP ${_patch_http} — skipping" >&2 | ||
| fi | ||
|
|
||
| # TODO(hypershift): Upstream --aws-assume-role + Pod Identity support | ||
| # to hypershift install CLI, then replace this post-install patch | ||
| # with native flags. | ||
| if [ -n "${DNS_ZONE_OPERATOR_ROLE_ARN:-}" ]; then | ||
| echo "Patching external-dns with --aws-assume-role=${DNS_ZONE_OPERATOR_ROLE_ARN}" | ||
| _CA=/var/run/secrets/kubernetes.io/serviceaccount/ca.crt | ||
| _TOKEN=$(cat /var/run/secrets/kubernetes.io/serviceaccount/token) | ||
| _API="https://kubernetes.default.svc" | ||
| # Upstream v0.21.0 needs discovery.k8s.io and networking.k8s.io API groups | ||
| # that HyperShift's generated ClusterRole doesn't include. | ||
| # Use JSON Patch (RFC 6902) to add rules without replacing HyperShift's existing rules. | ||
| _patch_http=$(curl -s --cacert "$_CA" -H "Authorization: Bearer $_TOKEN" \ | ||
| -H "Content-Type: application/json-patch+json" -X PATCH \ | ||
| "${_API}/apis/rbac.authorization.k8s.io/v1/clusterroles/external-dns" \ | ||
| -d '[ | ||
| {"op":"add","path":"/rules/-","value":{"apiGroups":["","discovery.k8s.io"],"resources":["services","endpoints","pods","nodes","endpointslices"],"verbs":["get","watch","list"]}}, | ||
| {"op":"add","path":"/rules/-","value":{"apiGroups":["extensions","networking.k8s.io"],"resources":["ingresses","ingressroutes","ingressroutetcps","ingressrouteudps"],"verbs":["get","list","watch"]}}, | ||
| {"op":"add","path":"/rules/-","value":{"apiGroups":["route.openshift.io"],"resources":["routes"],"verbs":["get","list","watch"]}} | ||
| ]' \ | ||
| -o /dev/null -w "%{http_code}" 2>/dev/null || echo "000") | ||
| echo " external-dns clusterrole patch: HTTP ${_patch_http}" | ||
| if [ "$_patch_http" != "200" ] && [ "$_patch_http" != "201" ]; then | ||
| echo "WARNING: external-dns clusterrole patch returned HTTP ${_patch_http} — skipping" >&2 |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Fail the Job when a required external-dns patch fails.
Line 161 and Line 178 convert all failed PATCH responses into warnings. The ClusterRole patch is required by Lines 165-167. A failed patch leaves external-dns without required permissions, but the Job reports success and Argo CD does not retry it.
Exit nonzero after a non-success response. Preserve the HTTP status in the error output.
Proposed fix
if [ "$_patch_http" != "200" ] && [ "$_patch_http" != "201" ]; then
- echo "WARNING: external-dns deployment patch returned HTTP ${_patch_http} — skipping" >&2
+ echo "ERROR: external-dns deployment patch returned HTTP ${_patch_http}" >&2
+ exit 1
fi
...
if [ "$_patch_http" != "200" ] && [ "$_patch_http" != "201" ]; then
- echo "WARNING: external-dns clusterrole patch returned HTTP ${_patch_http} — skipping" >&2
+ echo "ERROR: external-dns clusterrole patch returned HTTP ${_patch_http}" >&2
+ exit 1
fi📝 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.
Suggested change
| if [ "$_patch_http" != "200" ] && [ "$_patch_http" != "201" ]; then | |
| echo "WARNING: external-dns deployment patch returned HTTP ${_patch_http} — skipping" >&2 | |
| fi | |
| # TODO(hypershift): Upstream --aws-assume-role + Pod Identity support | |
| # to hypershift install CLI, then replace this post-install patch | |
| # with native flags. | |
| if [ -n "${DNS_ZONE_OPERATOR_ROLE_ARN:-}" ]; then | |
| echo "Patching external-dns with --aws-assume-role=${DNS_ZONE_OPERATOR_ROLE_ARN}" | |
| _CA=/var/run/secrets/kubernetes.io/serviceaccount/ca.crt | |
| _TOKEN=$(cat /var/run/secrets/kubernetes.io/serviceaccount/token) | |
| _API="https://kubernetes.default.svc" | |
| # Upstream v0.21.0 needs discovery.k8s.io and networking.k8s.io API groups | |
| # that HyperShift's generated ClusterRole doesn't include. | |
| # Use JSON Patch (RFC 6902) to add rules without replacing HyperShift's existing rules. | |
| _patch_http=$(curl -s --cacert "$_CA" -H "Authorization: Bearer $_TOKEN" \ | |
| -H "Content-Type: application/json-patch+json" -X PATCH \ | |
| "${_API}/apis/rbac.authorization.k8s.io/v1/clusterroles/external-dns" \ | |
| -d '[ | |
| {"op":"add","path":"/rules/-","value":{"apiGroups":["","discovery.k8s.io"],"resources":["services","endpoints","pods","nodes","endpointslices"],"verbs":["get","watch","list"]}}, | |
| {"op":"add","path":"/rules/-","value":{"apiGroups":["extensions","networking.k8s.io"],"resources":["ingresses","ingressroutes","ingressroutetcps","ingressrouteudps"],"verbs":["get","list","watch"]}}, | |
| {"op":"add","path":"/rules/-","value":{"apiGroups":["route.openshift.io"],"resources":["routes"],"verbs":["get","list","watch"]}} | |
| ]' \ | |
| -o /dev/null -w "%{http_code}" 2>/dev/null || echo "000") | |
| echo " external-dns clusterrole patch: HTTP ${_patch_http}" | |
| if [ "$_patch_http" != "200" ] && [ "$_patch_http" != "201" ]; then | |
| echo "WARNING: external-dns clusterrole patch returned HTTP ${_patch_http} — skipping" >&2 | |
| if [ "$_patch_http" != "200" ] && [ "$_patch_http" != "201" ]; then | |
| echo "ERROR: external-dns deployment patch returned HTTP ${_patch_http}" >&2 | |
| exit 1 | |
| fi | |
| # Upstream v0.21.0 needs discovery.k8s.io and networking.k8s.io API groups | |
| # that HyperShift's generated ClusterRole doesn't include. | |
| # Use JSON Patch (RFC 6902) to add rules without replacing HyperShift's existing rules. | |
| _patch_http=$(curl -s --cacert "$_CA" -H "Authorization: Bearer $_TOKEN" \ | |
| -H "Content-Type: application/json-patch+json" -X PATCH \ | |
| "${_API}/apis/rbac.authorization.k8s.io/v1/clusterroles/external-dns" \ | |
| -d '[ | |
| {"op":"add","path":"/rules/-","value":{"apiGroups":["","discovery.k8s.io"],"resources":["services","endpoints","pods","nodes","endpointslices"],"verbs":["get","watch","list"]}}, | |
| {"op":"add","path":"/rules/-","value":{"apiGroups":["extensions","networking.k8s.io"],"resources":["ingresses","ingressroutes","ingressroutetcps","ingressrouteudps"],"verbs":["get","list","watch"]}}, | |
| {"op":"add","path":"/rules/-","value":{"apiGroups":["route.openshift.io"],"resources":["routes"],"verbs":["get","list","watch"]}} | |
| ]' \ | |
| -o /dev/null -w "%{http_code}" 2>/dev/null || echo "000") | |
| echo " external-dns clusterrole patch: HTTP ${_patch_http}" | |
| if [ "$_patch_http" != "200" ] && [ "$_patch_http" != "201" ]; then | |
| echo "ERROR: external-dns clusterrole patch returned HTTP ${_patch_http}" >&2 | |
| exit 1 | |
| fi |
🤖 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 161 - 179, Update the external-dns PATCH handling in the job script so any
non-200/201 response exits the Job with a nonzero status instead of only
warning. Apply this to both the deployment patch check and the required
ClusterRole patch check, preserving the HTTP status in the error output before
exiting so Argo CD can retry the failed operation.
Contributor
Author
|
/test on-demand-e2e |
Contributor
Author
|
/test images |
2 similar comments
Contributor
Author
|
/test images |
Contributor
Author
|
/test images |
Contributor
Author
|
/test on-demand-e2e |
Contributor
Author
|
/test images |
Contributor
Author
|
/test images |
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
Experimental test branch, opened as draft only to exercise CI pipelines — not intended for review or merge.
karpenter-bootstrapmanaged node group AMI from AL2023 to Bottlerocket (matching the AMI family Karpenter-provisioned workload nodes already use).CriticalAddonsOnly:NoScheduleexclusion taint with a newbootstrap-criticalPriorityClass (value100000), applied to both ArgoCD and Karpenter (deliberately downgrading Karpenter from its chart's defaultsystem-cluster-critical).Built off
remove-auto-mode-v2.Test plan
terraform validate,make helm-lint,make check-docs,make check-rendered-files🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation