WIP: e2e baseline test — 6e14d0eb (last known-passing Karpenter commit) - #725
Closed
theautoroboto wants to merge 32 commits into
Closed
WIP: e2e baseline test — 6e14d0eb (last known-passing Karpenter commit)#725theautoroboto wants to merge 32 commits into
theautoroboto wants to merge 32 commits into
Conversation
Replaces EKS Auto Mode compute with OSS Karpenter v1 on both the
Regional Cluster and Management Clusters.
Infrastructure changes (terraform/modules/eks-cluster/):
- Remove Auto Mode cluster compute_config block; switch to manual node
group bootstrap (AL2023, CriticalAddonsOnly:NoSchedule taint)
- Add Karpenter controller role (IRSA), node role + instance profile,
SQS interruption queue, and four EventBridge rules
- EC2NodeClass uses spec.instanceProfile referencing the Terraform-
managed profile; avoids granting Karpenter IAM profile lifecycle
permissions
- IAM PassRole scoped with iam:PassedToService = ec2.amazonaws.com
- All managed-policy ARNs and SQS/EventBridge ARNs parameterised on
data.aws_partition.current.partition for GovCloud compatibility
- kms:CreateGrant for RHEL FIPS AMI decryption moved to controller role
(the RunInstances caller); constrained with kms:GrantIsForAWSResource
ArgoCD node provisioning (argocd/config/{management,regional}-cluster/):
- eks-nodepool chart: EC2NodeClass (FIPS Bottlerocket, httpTokens=required)
and NodePool (management-workloads, consolidation after 30s)
- monitoring/values.yaml: CriticalAddonsOnly tolerations on
prometheus-operator pod and admissionWebhooks patch Jobs so they
schedule during bootstrap before workload nodes are ready
ECS bootstrap (terraform/modules/ecs-bootstrap/main.tf):
- Install Karpenter via Helm before seeding NodePool CRDs
- Apply EC2NodeClass + NodePool from the chart on every run (idempotent
server-side apply) to prevent stale specs from surviving failed runs
- Pre-warm one Karpenter node before ArgoCD install; diagnostic dump on
timeout covers all NodePools and NodeClaims
Bootstrap timeout headroom:
- hypershift-install Job: activeDeadlineSeconds 1800→3600;
CriticalAddonsOnly toleration added so Job schedules during bootstrap
- CodeBuild management_bootstrap + regional_bootstrap: 30→75 min to
outlive the ECS task's ArgoCD install and readiness wait
- PIPELINE_COMPLETION_TIMEOUT: 3600→5400 s (90 min) for the full
provision-ephemeral phase
- provision-infra-mc.sh: -parallelism=20 on terraform apply to reduce
mc01 wall-clock time
Validation script updates (scripts/validate-rc-k8s.sh,
scripts/validate-rc-aws.sh):
- Promote CriticalAddonsOnly taint check and Karpenter pod placement
check from warn to fail
- Remove unused kp_instances variable
Documentation (docs/design/):
- karpenter-node-provisioning.md: update spec.instanceProfile reference
- fips-eks-compute.md: bootstrap node group described as AL2023
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
kms:CreateGrant for RHEL FIPS AMI decryption belongs on the controller role, not the node role. The controller calls RunInstances — that is the principal AWS KMS sees when it creates the internal grant allowing EC2 to decrypt the AMI's EBS snapshot at launch. The node role (instance profile) never makes this call and did not need the permission. - Remove aws_iam_role_policy.karpenter_node_kms entirely - Rename karpenter_controller_kms → rhel-ami-kms; split into two statements so kms:GrantIsForAWSResource condition targets only kms:CreateGrant, preventing the controller from granting arbitrary IAM principals key access - Update stale spec.role references in iam.tf comment and outputs.tf description to spec.instanceProfile Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The ECS bootstrap task previously returned immediately after creating the ArgoCD root Application. The E2E test starts as soon as the provision CodeBuild step returns, creating a race where HyperShift may not be installed yet when the work agent tries to apply HostedCluster ManifestWorks (resulting in AppliedManifestWorkFailed). Add a 30-minute polling loop (MC only) that blocks until the hypershift ArgoCD Application is Healthy — ensuring HyperShift CRDs and the operator are fully installed before the E2E test begins. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Two issues caused MC bootstrap to fail on the Karpenter branch:
1. hypershift-install Job: CRD wait used `kubectl`, which is absent from
the hypershift-operator image. Replace with `curl` against the in-cluster
API server so the wait works without kubectl.
2. monitoring: `admissionWebhooks.enabled: false` skips the certgen Job that
creates the TLS secret, but `prometheusOperator.tls.enabled` defaults to
true, causing the PrometheusOperator pod to crash on the missing mount.
Replace ttlSecondsAfterFinished workaround with `enabled: false` +
`tls.enabled: false` to fully disable the webhook path.
Also fix Terraform heredoc escaping for bash variables in the ECS bootstrap
hypershift-health wait block (${CLUSTER_TYPE:-} and ${_HS_STATUS}).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The conflict resolution during rebase incorrectly took the branch version of imports.sh which included imports for module.maestro_infrastructure and module.hyperfleet_infrastructure — neither of which exist in the regional cluster Terraform configuration. These caused terraform import to fail with "Import to non-existent module." Keep only the valid rhobs_api_gateway and api_gateway log group imports. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
CLOUDFRONT_DOMAIN is not exported in the register buildspec environment and CLOUDFRONT_URL is never referenced in the script. The variable was incorrectly included during rebase conflict resolution. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tor TLS
The monitoring E2E suite was failing after exactly 5m because the remote
Makefile target uses --timeout=5m, which preempts the two Eventually("10m",...)
observability specs before they can see metrics in Thanos. Replace the make
invocation with a direct ginkgo call at --timeout=25m to give both specs
room to run.
Also apply the same admissionWebhooks.enabled/tls.enabled=false fix to the RC
monitoring chart that was already applied to MC in 23b650e. The old TTL
workaround (ttlSecondsAfterFinished: 600) doesn't protect against certgen Job
failures, only against Jobs being cleaned up before ArgoCD observes them.
Disabling webhooks/TLS entirely removes the failure mode on both cluster types.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…naws.com removed with Auto Mode)
Remove all EKS Auto Mode infrastructure and the enable_karpenter feature flag. Karpenter is now the sole, unconditional compute strategy. Changes: - eks-cluster module: remove compute_config, storage_config, kubernetes_network_config.elastic_load_balancing dynamic blocks - iam.tf: remove aws_iam_role.eks_auto_mode_node and all four Auto Mode managed policies (AmazonEKSComputePolicy, etc.); simplify eks_cluster_managed to a single AmazonEKSClusterPolicy attachment; make all Karpenter resources unconditional (drop count/for_each ternaries) - variables.tf: remove enable_karpenter variable - outputs.tf: remove null conditionals, always return Karpenter ARNs - data.tf/locals.tf: remove count from tls_certificate.eks_oidc, remove enable_karpenter ternary from oidc_issuer local - bastion/log-collection-task.tf: eks.amazonaws.com CRD group → elbv2.k8s.aws / karpenter.k8s.aws - api-gateway, rhobs-api-gateway, sre-ui-alb: remove Auto Mode comments and variable description references - scripts/verify-fips.sh: replace Auto Mode NodeClass detection with OSS Karpenter (karpenter.k8s.aws) CRD check - scripts/validate-mc-k8s.sh: include upstream HCP API reachability checks from merge (missed in merge commit 6514531) - README.md: remove enable_karpenter input row, update output descriptions Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ArgoCD does not upgrade existing CRDs on Helm chart updates, so the LBC CRD in the cluster is still at v1alpha1 only. Switch all TGB templates from v1beta1 → v1alpha1 to match the installed CRD schema. Both versions share the same core fields we use (targetGroupARN, targetType, serviceRef). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The RC state terraform init was fully silenced with >/dev/null 2>&1, causing the MC bootstrap to exit with status 1 in ~13 seconds with no diagnostic output. Remove 2>&1 so stderr (errors) is visible while stdout (verbose init messages) remains suppressed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
kubectl get nodepool is ambiguous on clusters with HyperShift CRDs installed: both karpenter.sh and hypershift.openshift.io register Kind NodePool. kubectl resolves to the HyperShift group, returning zero results even when Karpenter NodePools are healthy and running. All five kubectl get nodepool calls now use kubectl get nodepools.karpenter.sh to target the correct API group explicitly. Also reformats eks-cluster README.md table columns (prettier --write). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
When make test-e2e-cli fails with a DNS resolution error on the HCP API endpoint, there is currently no visibility into whether the failure is due to external-dns not running, an IAM permission error on the cross-account Route53 assume, HyperShift CPO not emitting DNSEndpoint CRs, a missing zone shard, or a pure timing race. Add diag_dns() called pre-test (baseline infrastructure state) and post-failure (targeted A record and Route53 lookup for the cluster API hostname) to distinguish between these five root causes. Tracked in ROSAENG-61244. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Two bugs found from first real diagnostic run (eph-106c37): 1. aws eks update-kubeconfig always exits 0 for private clusters — it only writes the kubeconfig, it does not test reachability. mc_kube_ok was therefore always set to true, but every subsequent kubectl call timed out (MC EKS API is a private VPC endpoint unreachable from the CI runner). Add an explicit kubectl cluster-info --request-timeout=5s probe so mc_kube_ok reflects actual reachability. 2. Base domain was derived by stripping the hostname from BASE_URL, which is an API Gateway URL. The sed expression yielded execute-api.us-east-1.amazonaws.com — useless for HCP DNS. Derive the HCP base domain from CLUSTER_PREFIX + AWS_DEFAULT_REGION instead, which matches the actual Route53 zone pattern (e.g. us-east-1-eph-106c37.ci00.rosa.devshift.net). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
dig and nslookup are not present in the CI container image. Replace the NS delegation check with an aws route53 list-resource-record-sets call that retrieves the zone's NS record set directly from the Route53 API. This works without any DNS tools in the container and also distinguishes two cases: - zone exists → show authoritative NS records (compare against parent delegation) - zone not found → explicit "root cause D" message Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The direct ginkgo invocation used ./test/e2e/... which does not exist in rosa-hyperfleet-api. Ginkgo found no test suites and exited non-zero, causing monitoring=1 on every run. The correct path matches what make test-e2e-platform-monitoring uses: ./test/e2e-platform-monitoring. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ootstrap The Karpenter bootstrap node group carries CriticalAddonsOnly:NoSchedule taint. external-dns (deployed by hypershift install) has no matching toleration, so it sits Pending until Karpenter provisions an untainted compute node (~2-4 min). During that window no HCP API DNS records are written, causing e2e DNS failures. Add a third JSON patch operation to inject the CriticalAddonsOnly toleration so external-dns can schedule on bootstrap nodes immediately after hypershift install. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ArgoCD holds the Application in Progressing until all Jobs complete. Adding this wait ensures external-dns is scheduling and serving before the e2e pipeline begins HCP cluster creation, closing the window where no DNS records can be written. The per-cluster DNS propagation wait (after HCP Available, before kubectl) belongs in the CLI repo's Go e2e test using cluster.Status.ControlPlaneEndpoint.Host + net.LookupHost polling. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Emit [DNS-DEBUG] banners before and after terraform apply to surface: - AWS account ID at time of zone creation - Whether ENVIRONMENT_DOMAIN / TF_VAR_environment_domain are set - Source file (.domain field in provisioner JSON) - Expected zone name that Terraform would create - Post-apply: terraform outputs for zone ID and name servers - Post-apply: aws route53 list-hosted-zones filtered by deployment_name Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Every 60s while waiting for the hypershift ArgoCD Application to become Healthy, emit: - ArgoCD health message (inline on each status line) - kubectl get job hypershift-install -n hypershift-install - kubectl get pods -n hypershift-install - kubectl logs job/hypershift-install --tail=30 Distinguishes: pod stuck Pending (scheduling), Prometheus CRD wait, hypershift install, external-dns availability wait. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The hypershift-install Job checks external-dns readiness via jq, but jq is not installed in the HyperShift operator image. This caused the until-loop condition to never be true, so the Job always ran until the 5-minute _EXTDNS_DEADLINE then exited with error 1. With backoffLimit: 1 and ArgoCD's Replace=true,Force=true sync option, this produced an infinite restart loop keeping the hypershift Application Degraded until the 30-minute ECS bootstrap timeout. Replace jq with grep -q to check for a non-zero availableReplicas field, and use grep -oE for the error diagnostic dump. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The post-failure DNS diagnostic had two bugs: - Hardcoded 'ci00' in base_domain construction; local dev envs use 'dev0', causing false "Zone not found" reports that masked the real zone state. - Tried to look up a specific A record by constructing the hostname, but the format requires a 4-char UUID prefix that only the platform API knows, so the constructed hostname was always wrong and matched nothing in Route53. Fix 1: Auto-detect the env domain suffix by querying Route53 for the parent zone that matches the env prefix (us-east-1-eph-9c235d39.*) and stripping the trailing dot. Works for dev0, ci00, and any future suffix. Fix 2: Instead of looking up a specific A record, list all A records in the shard zone (0.<env-domain>). external-dns writes all api.* A records here, so their presence/absence directly answers "did external-dns write anything?", regardless of which cluster triggered the failure. Also remove the wrong _api_host construction at the call site; the Route53 shard zone listing gives us the information we need without a hostname. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
provision-infra-rc.sh reads individual keys from terraform.json and exports them as TF_VAR_* env vars, but hyperfleet_db_deletion_protection was never wired. This caused Terraform to always use the module default (true), ignoring the ephemeral environment setting of false, which blocked RDS cluster teardown.
Resolves conflicts between the Karpenter migration work on this branch and the ElastiCache Valkey rate-limiting additions merged to main. Both sets of changes are independent: kept all Karpenter env vars/variables alongside the new REDIS_ENDPOINT env var, redis_endpoint variable, and rc_aws_account_id / redis_endpoint module inputs in regional-cluster main.tf. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds temporary diagnostic instrumentation (marked TODO(dns-troubleshooting)) to identify the root cause of missing HCP API DNS records in e2e tests: - ci/e2e-tests.sh: Route53 background poller (30s interval, A record timeline), CloudTrail queries for ChangeResourceRecordSets and AssumeRole to dns-zone-operator, pre/post-failure diag_dns() calls - scripts/dev/collect-cluster-logs.sh: thread DIAG_BASE_DOMAIN through to ECS log-collector task as BASE_DOMAIN - terraform/modules/bastion/log-collection-task.tf: DNS probe from inside VPC (Route53 A records + dig NS delegation), BASE_DOMAIN env var, route53:List* IAM policy on log-collector role - terraform/modules/ecs-bootstrap/main.tf: expand hypershift-install DIAG block to show logs from all pods (not just first) and add external-dns pod status + logs - argocd/config/management-cluster/hypershift/templates/05-job.yaml: show external-dns pod phase/reason/message at the error exit path All diagnostic blocks are marked for removal before merging to main. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The wait was blocking MC bootstrap when external-dns failed to reach availableReplicas >= 1 within 5 minutes. Removing the gate unblocks provisioning; external-dns readiness is addressed separately. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The Red Hat external-dns image (registry.redhat.io/edo/external-dns-rhel9) does not have a binary at /ko-app/external-dns — that path is specific to the upstream ko-built image that was replaced in c68f7fc. commit 668d19e inadvertently re-introduced this command replacement when adding the CriticalAddonsOnly toleration patch. The override caused external-dns to crash immediately on start (exec not found), which meant no A records were ever written to Route53 shard zones, leaving HCP clusters stuck in Provisioning indefinitely. Keep the --aws-assume-role arg injection and CriticalAddonsOnly toleration; remove only the command replacement. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The existing diag_dns() only checked RC account CloudTrail for external-dns Route53 activity. But external-dns first assumes a Pod Identity role in the MC account before cross-account assuming the RC dns-zone-operator role. If external-dns is not running (root cause A) or Pod Identity is misconfigured, the RC CloudTrail stays silent — which matches the current CI failure pattern (25+ min, zero A records, cert-manager is the only actor in the RC shard zone). The new check looks for AssumeRoleWithWebIdentity events on *dns-operator roles in the MC account: - Empty → pod is dead (confirms root cause A) - AccessDenied → Pod Identity association missing or wrong role - OK → pod is running; investigate DNSEndpoint CRs or the RC cross-account step Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Root cause (go directive version mismatch) was fixed in rosa-hyperfleet-api. Removing the diag_dns() function, Route53 background poller, and all associated TODO(dns-troubleshooting) blocks. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
openshift-ci
Bot
added
do-not-merge/work-in-progress
|
[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 PR migrates EKS compute from Auto Mode to OSS Karpenter, adds Karpenter IAM and bootstrap infrastructure, introduces AWS Load Balancer Controller integration, updates TargetGroupBinding resources, adds cluster validation scripts, and revises bootstrap configuration and documentation. ChangesOSS Karpenter infrastructure
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: ✨ Finishing Touches🧪 Generate unit tests (beta)
|
openshift-ci
Bot
removed
the
needs-rebase
Contributor
Author
|
/test on-demand-e2e |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/buildspec/register.sh (1)
67-104: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winIncrease the register CodeBuild timeout
In
terraform/config/pipeline-management-cluster/main.tf:443-446,aws_codebuild_project.registerstill hasbuild_timeout = 15, but this flow can take ~90 minutes worst case (45m waiting for RC outputs, 40m for/live, 5m for registration). Bump it with buffer or the build will be cut off early.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/buildspec/register.sh` around lines 67 - 104, The register CodeBuild project timeout is shorter than the maximum duration of the registration flow. Update aws_codebuild_project.register in the pipeline-management-cluster Terraform configuration to increase build_timeout to at least 90 minutes, preferably with additional buffer, while leaving the existing retry behavior unchanged.
🟡 Minor comments (7)
docs/design/karpenter-node-provisioning.md-7-10 (1)
7-10: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winUpdate the Karpenter auth wording.
docs/design/karpenter-node-provisioning.md#L7-L10and#L25-L32: frame IRSA as the chosen bootstrap/compatibility path for this deployment; don’t imply Pod Identity is unsupported or that IRSA is the only valid option.docs/design/zoa-trusted-actions.md#L825: replace “IRSA is being deprecated” with “Pod Identity is preferred for new workload service accounts, while IRSA remains supported.”🤖 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` around lines 7 - 10, Update docs/design/karpenter-node-provisioning.md at lines 7-10 and 25-32 to describe IRSA as this deployment’s chosen bootstrap and compatibility path, without implying that EKS Pod Identity is unsupported or that IRSA is the only valid option. Update docs/design/zoa-trusted-actions.md at line 825 to state that Pod Identity is preferred for new workload service accounts while IRSA remains supported.Source: MCP tools
argocd/config/management-cluster/hypershift/templates/05-job.yaml-134-144 (1)
134-144: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winThe merge patch replaces
ruleswholesale, it does not augment them.
application/merge-patch+jsonreplaces arrays, so HyperShift's generatedexternal-dnsClusterRole rules are discarded and only these three rules remain. That works today only if this list is a superset; any upstream rule addition is silently dropped on the next bump. Worth aligning the comment with the actual semantics, and ideally reading the existing rules and appending via JSON patch instead.🤖 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 134 - 144, Update the external-dns ClusterRole patch in the job template so existing HyperShift-generated rules are preserved and the required discovery, networking, and route permissions are appended rather than replacing the entire rules array. Prefer reading the current rules and applying an appropriate JSON Patch operation; also revise the nearby comment to accurately describe the augmentation behavior.argocd/config/regional-cluster/monitoring/values.yaml-69-80 (1)
69-80: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd the
CriticalAddonsOnlytoleration to RCprometheusOperator
terraform/modules/eks-cluster/main.tftaints thekarpenter-bootstrapnode group withCriticalAddonsOnly:NoSchedule, andterraform/config/regional-cluster/main.tfalso uses that module. RCprometheusOperatoris missing the same toleration that MC already sets, so it can stay unschedulable during bootstrap and block CRD reconciliation.🤖 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 69 - 80, Update the RC prometheusOperator configuration to add the CriticalAddonsOnly toleration matching the existing MC prometheusOperator settings, allowing scheduling on nodes tainted CriticalAddonsOnly:NoSchedule. Preserve the current admissionWebhooks and tls settings.scripts/validate-rc-aws.sh-229-232 (1)
229-232: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winHint points at a non-existent Service annotation.
Post-LBC migration the target group ARN lives on the TargetGroupBinding, not a Service annotation.
📝 Proposed fix
- warn " Set it to: kubectl get svc -n platform-api -o jsonpath='{.items[0].metadata.annotations.service\.beta\.kubernetes\.io/aws-load-balancer-arn}'" + warn " Set it to: kubectl get targetgroupbinding -n platform-api -o jsonpath='{.items[0].spec.targetGroupARN}'"🤖 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/validate-rc-aws.sh` around lines 229 - 232, Update the PLATFORM_API_TG_ARN guidance in the target health check fallback within the validation script to retrieve the target group ARN from the TargetGroupBinding resource rather than the Service annotation. Preserve the existing skip warning and provide a kubectl command that queries the TargetGroupBinding’s targetGroupARN field.terraform/modules/bastion/log-collection-task.tf-83-84 (1)
83-84: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDuplicate resource entry —
ec2nodeclasses.karpenter.k8s.awsis already listed at line 61.Costs an extra
oc adm inspectrun and shifts the batch-of-5 grouping.♻️ Proposed fix
targetgroupbindings.elbv2.k8s.aws - ec2nodeclasses.karpenter.k8s.aws secretproviderclasses.secrets-store.csi.x-k8s.io🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@terraform/modules/bastion/log-collection-task.tf` around lines 83 - 84, Remove the duplicate ec2nodeclasses.karpenter.k8s.aws entry from the resource list, keeping its existing earlier occurrence and preserving the intended batch-of-5 grouping.scripts/validate-rc-aws.sh-135-151 (1)
135-151: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHealth check reports PASS when the node group doesn't exist.
When
describe-nodegroupfails,jq 'length'receives empty input andng_readyis empty, so[[ "$ng_ready" -eq 0 ]]evaluates true and the script prints "0 nodes, no health issues" right after the FAIL on line 132. Nest this under the ACTIVE branch, asscripts/validate-mc-aws.shdoes.🐛 Proposed fix
if [[ "$ng_status" == "ACTIVE" ]]; then pass "Node group '${ng_name}': ACTIVE" + ng_desired=$(aws eks describe-nodegroup \ + --cluster-name "$CLUSTER_ID" \ + --nodegroup-name "$ng_name" \ + --query "nodegroup.scalingConfig.desiredSize" \ + --output text 2>/dev/null || echo "0") + ng_issues=$(aws eks describe-nodegroup \ + --cluster-name "$CLUSTER_ID" \ + --nodegroup-name "$ng_name" \ + --query "length(nodegroup.health.issues)" \ + --output text 2>/dev/null || echo 0) + if [[ "${ng_issues:-0}" -eq 0 ]]; then + pass "Node group '${ng_name}': ${ng_desired} nodes, no health issues" + else + fail "Node group '${ng_name}': ${ng_issues} health issue(s)" + fi else fail "Node group '${ng_name}': ${ng_status}" fi(drop the now-redundant standalone blocks on lines 135-151)
🤖 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/validate-rc-aws.sh` around lines 135 - 151, Move the node-group health-check block containing ng_desired, ng_ready, and the pass/fail messages inside the existing ACTIVE branch, matching the control flow in validate-mc-aws.sh. Remove the standalone duplicate block so describe-nodegroup failures cannot be interpreted as zero health issues after a prior failure.terraform/modules/eks-cluster/variables.tf-78-82 (1)
78-82: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDescription overstates what is created.
Only
aws_iam_role_policy.karpenter_controller_kmsis added (kms:CreateGrant+kms:DescribeKeyon the controller role); there is nokms:Decryptgrant and no node-role policy interraform/modules/eks-cluster/iam.tf. Align the wording with the implementation.📝 Proposed wording
- description = "ARN of the Red Hat KMS key used to encrypt RHEL FIPS AMI EBS snapshots. When set, IAM policies granting kms:Decrypt and kms:CreateGrant on this key are added to the Karpenter node and controller roles. Leave empty to skip KMS policy creation." + description = "ARN of the Red Hat KMS key used to encrypt RHEL FIPS AMI EBS snapshots. When set, an inline policy granting kms:CreateGrant (restricted via kms:GrantIsForAWSResource) and kms:DescribeKey on this key is attached to the Karpenter controller role. Leave empty to skip KMS policy 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/eks-cluster/variables.tf` around lines 78 - 82, Update the description of the ami_kms_key_arn variable to accurately describe only the Karpenter controller IAM policy that is created, including its kms:CreateGrant and kms:DescribeKey permissions; remove claims about kms:Decrypt and node-role policy creation while preserving the empty-value behavior.
🧹 Nitpick comments (9)
scripts/buildspec/bootstrap-argocd-mc.sh (1)
33-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood change — stderr from
terraform initis now visible. The sameinitinscripts/buildspec/provision-infra-mc.sh(line 45) still uses>/dev/null 2>&1; aligning both would keep backend failures diagnosable in either pipeline.🤖 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/bootstrap-argocd-mc.sh` at line 33, Update the Terraform init invocation in provision-infra-mc.sh to stop redirecting stderr to /dev/null, matching the visible stderr behavior already applied to the bootstrap-argocd-mc.sh init command while preserving the existing stdout handling.scripts/buildspec/provision-infra-mc.sh (1)
140-140: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
-parallelism=20doubles concurrent AWS API calls, including on destroy.Faster applies, but watch for IAM/EC2/EKS throttling (
RequestLimitExceeded) in this account, which surfaces as flaky pipeline failures rather than a clean error. If throttling shows up,TF_VAR-free knobs likeAWS_MAX_ATTEMPTS/AWS_RETRY_MODE=adaptivein the buildspec env pair well with the higher parallelism.🤖 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, Review the Terraform invocation in the infrastructure provisioning script and mitigate AWS API throttling introduced by -parallelism=20, including destroy operations. Configure AWS_MAX_ATTEMPTS and AWS_RETRY_MODE=adaptive in the buildspec environment, or reduce parallelism if appropriate, while preserving the existing Terraform action flow.argocd/config/management-cluster/monitoring/values.yaml (1)
19-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDisabling the admission webhook drops server-side
PrometheusRulevalidation.Deliberate and well documented, but note the side effect: malformed rules/relabel configs will now be accepted by the API server and only fail inside Prometheus. Given
defaultRules.create: falseand rules coming from this repo, consider adding apromtool check rulesstep in CI to compensate.🤖 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 19 - 33, Because admissionWebhooks are disabled in the monitoring Helm values, add a CI validation step for repository-managed PrometheusRule manifests that runs promtool check rules before deployment. Keep defaultRules.create false and retain the existing webhook and TLS settings, ensuring malformed rules or relabel configurations are rejected during CI.ci/ephemeral-provider/orchestrator.py (1)
227-239: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocstring now contradicts the behavior, and the glob misses
.yml.The method docstring still says the override directory "replaces the
config/ephemeral/directory entirely", butdefaults.yamlis now merged rather than replaced — update it so the merge semantics are discoverable. Also, both globs only match*.yaml, so a*.ymloverride (or a stale*.ymlregion file) is silently ignored; worth normalizing to both suffixes or asserting the convention.🤖 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 227 - 239, The orchestrator method’s documentation must describe merging defaults.yaml rather than entirely replacing the ephemeral configuration directory, and its file handling must include both .yaml and .yml files. Update the method docstring and both override/stale-file discovery loops, preserving the special defaults.yaml merge behavior while copying other overrides normally.argocd/config/management-cluster/eks-nodepool/templates/00-nodeclass.yaml (1)
7-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider pinning the Bottlerocket AMI alias.
bottlerocket@latestlets AMI upgrades land unreviewed on a NodeClass namedfips; pinning (e.g.bottlerocket@v1.x.y) keeps node images deterministic and auditable for the FIPS story.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@argocd/config/management-cluster/eks-nodepool/templates/00-nodeclass.yaml` around lines 7 - 8, Update the amiSelectorTerms configuration in the fips NodeClass to replace the floating bottlerocket@latest alias with a specific versioned Bottlerocket AMI alias. Use the approved bottlerocket@v1.x.y version so node images remain deterministic and auditable.terraform/modules/ecs-bootstrap/main.tf (1)
229-240: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winScope the Helm ownership re-stamp to chart-owned objects.
This annotates every object of those kinds in
argocd— including secrets/configmaps created by external-secrets or other controllers. Marking foreign objects as Helm-owned makes them adoptable (and prunable onhelm uninstall). Narrow it with a selector such as-l app.kubernetes.io/part-of=argocd(or an explicit name list) so only chart resources get re-stamped.🤖 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/main.tf` around lines 229 - 240, Scope the resource queries in the Helm ownership re-stamp loop to chart-owned objects only, using a selector such as app.kubernetes.io/part-of=argocd or an explicit name list. Update the kubectl get logic within the resource loop while preserving annotation behavior for matched resources and excluding externally managed objects.terraform/modules/eks-cluster/versions.tf (1)
13-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePin
tlsconsistently withaws.
awsuses~> 6.56.0whiletlsallows any>= 4.0, so a future majortlsrelease would be accepted here.~> 4.0keeps it in the 4.x line.♻️ Proposed change
tls = { source = "hashicorp/tls" - version = ">= 4.0" + version = "~> 4.0" }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@terraform/modules/eks-cluster/versions.tf` around lines 13 - 16, Update the tls provider constraint in the versions configuration to use ~> 4.0 instead of >= 4.0, keeping accepted versions within the 4.x release line while preserving the existing provider declaration.terraform/config/management-cluster/main.tf (1)
71-73: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNull guards are dead code — both outputs are unconditional.
karpenter_controller_role_arnandkarpenter_queue_urlresolve toaws_iam_role.karpenter_controller.arnandaws_sqs_queue.karpenter_interruption.url, neither of which iscount/for_eachgated, so the!= nullbranches can never be taken and only serve to mask a future regression as an empty string. Based on learnings that outputs and their guards should stay aligned with whether the underlying resources are actually always created, pass them through directly.♻️ Proposed simplification
- karpenter_controller_role_arn = module.management_cluster.karpenter_controller_role_arn != null ? module.management_cluster.karpenter_controller_role_arn : "" - karpenter_queue_url = module.management_cluster.karpenter_queue_url != null ? module.management_cluster.karpenter_queue_url : "" + karpenter_controller_role_arn = module.management_cluster.karpenter_controller_role_arn + karpenter_queue_url = module.management_cluster.karpenter_queue_url🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@terraform/config/management-cluster/main.tf` around lines 71 - 73, Update the management-cluster output assignments for karpenter_controller_role_arn and karpenter_queue_url to pass through the corresponding module.management_cluster values directly, removing the redundant null checks and empty-string fallbacks.Source: Learnings
argocd/config/regional-cluster/grafana/templates/sre-targetgroupbinding.yaml (1)
2-2: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winStandardize these TargetGroupBindings on
elbv2.k8s.aws/v1beta1.
v1alpha1is the legacy API version here; switch every TargetGroupBinding manifest to the currentv1beta1form:
argocd/config/shared/argocd/templates/sre-targetgroupbinding.yamlargocd/config/regional-cluster/grafana/templates/sre-targetgroupbinding.yamlargocd/config/regional-cluster/monitoring/templates/sre-targetgroupbinding.yamlargocd/config/regional-cluster/loki/templates/targetgroupbinding.yamlargocd/config/regional-cluster/platform-api/templates/targetgroupbinding.yamlargocd/config/regional-cluster/thanos/templates/targetgroupbinding.yaml🤖 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/grafana/templates/sre-targetgroupbinding.yaml` at line 2, Update the apiVersion for every TargetGroupBinding manifest to elbv2.k8s.aws/v1beta1, including argocd/config/regional-cluster/grafana/templates/sre-targetgroupbinding.yaml (2-2), argocd/config/regional-cluster/loki/templates/targetgroupbinding.yaml (2-2, 20-20), argocd/config/regional-cluster/monitoring/templates/sre-targetgroupbinding.yaml (2-2), argocd/config/regional-cluster/platform-api/templates/targetgroupbinding.yaml (2-2), argocd/config/regional-cluster/thanos/templates/targetgroupbinding.yaml (2-2, 21-21, 39-39), and argocd/config/shared/argocd/templates/sre-targetgroupbinding.yaml (2-2); preserve the existing TargetGroupBinding metadata and specifications.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@argocd/config/regional-cluster/eks-nodepool/templates/00-nodeclass.yaml`:
- Around line 5-9: Update the fips NodeClass AMI selector to use a
FIPS-specific, version-pinned Bottlerocket alias or explicit AMI selector,
preserving the resource’s FIPS behavior and avoiding the drifting
bottlerocket@latest reference.
In `@argocd/config/shared/storageclass/templates/gp3.yaml`:
- Line 7: Add the Argo CD Replace=true sync option to the gp3 StorageClass
metadata annotations so changes to the immutable provisioner trigger resource
replacement rather than patching.
In
`@deploy/integration/us-east-1/argocd-bootstrap-management-cluster/applicationset.yaml`:
- Around line 60-68: Scope the ignoreDifferences rules for
MutatingWebhookConfiguration and ValidatingWebhookConfiguration to only the AWS
Load Balancer Controller webhook resources, using their rendered webhook names
or isolating the rules to the LBC Application. Apply the same resource scoping
in
deploy/integration/us-east-1/argocd-bootstrap-management-cluster/applicationset.yaml
lines 60-68 and
deploy/integration/us-east-1/argocd-bootstrap-regional-cluster/applicationset.yaml
lines 60-68.
In `@scripts/buildspec/provision-infra-rc.sh`:
- Around line 194-213: Update the terraform invocation and surrounding logic in
the build flow so its non-zero exit status is captured without triggering set
-e, allowing the post-run DNS debug block to execute on failures. Preserve the
existing _TF_EXIT-based final exit behavior and ensure successful runs continue
with status 0.
- Around line 151-167: Apply the default for ENVIRONMENT before the DNS debug
block constructs _DBG_PROVISIONER_JSON, so unset values remain safe under set
-u; preserve the existing staging fallback used by the later export. Remove the
temporary DNS troubleshooting output blocks, including the block around
_DBG_PROVISIONER_JSON and the related block around lines 194-213, rather than
merging debug-only logging into main.
In `@scripts/validate-mc-aws.sh`:
- Around line 298-310: Replace the unsafe post-increment expressions with
pre-increment forms so set -e does not abort validation on zero-valued counters:
update total_subnets and no_ips in scripts/validate-mc-aws.sh lines 298-310, and
off_bootstrap in scripts/validate-rc-k8s.sh lines 118-136. Preserve the existing
counting and warning behavior.
- Around line 174-186: Update the wrong_vpc calculation in the cluster VPC
validation block to flatten the filtered instance projection before counting, so
it counts mismatched instances rather than reservations. Replace the current
paste/bc aggregation with the flattened JMESPath count, while preserving the
existing zero-versus-nonzero pass/fail behavior.
In `@scripts/validate-mc-k8s.sh`:
- Around line 165-183: Update the NodePool queries in the validation block to
use the fully qualified resource name for the intended NodePool CRD, including
both kubectl get calls inside the loop and the kubectl get command feeding the
loop. Keep the existing replica checks and reporting unchanged.
In `@scripts/verify-fips.sh`:
- Around line 130-133: Update the CRD existence check in the FIPS verification
flow to query the EC2NodeClass resource name used by the repository,
ec2nodeclasses.karpenter.k8s.aws, so the NodePool-to-NodeClass validation is not
skipped when Karpenter is installed. Preserve the existing skip behavior when
that CRD is absent.
In `@terraform/config/regional-cluster/imports.sh`:
- Around line 23-26: Update the CloudWatch log group identifier in the
import_if_needed call to use the module’s stage-name value rather than
TF_VAR_stage_name or its prod fallback, matching the ${var.stage_name}-based
naming used by the module.
In `@terraform/modules/aws-load-balancer-controller/iam.tf`:
- Around line 234-289: Update the ELB permission statements ELBMutateTags,
ELBMutateListenerRuleTags, and ELBMutateTagged to add StringEquals conditions
binding aws:ResourceTag/elbv2.k8s.aws/cluster to var.cluster_name while
preserving the existing Null checks where present. Also update the related
create/tag permission statements to require aws:RequestTag/elbv2.k8s.aws/cluster
equals var.cluster_name, so this role cannot operate on resources belonging to
other clusters.
In `@terraform/modules/bastion/log-collection-task.tf`:
- Around line 243-263: Update aws_iam_role_policy.log_collector_route53 to split
the Route53 actions into separate statements: retain Resource "*" for
route53:ListHostedZones, and scope route53:ListResourceRecordSets to
arn:aws:route53:::hostedzone/*. Preserve the existing role policy and
permissions otherwise.
In `@terraform/modules/ecs-bootstrap/main.tf`:
- Around line 117-137: Update the Karpenter bootstrap guard around
KARPENTER_CONTROLLER_ROLE_ARN to also require a non-empty KARPENTER_QUEUE_URL
before installing Karpenter, and fail fast with a clear error when the role ARN
is set but the queue URL is missing. Ensure basename and the Helm
settings.interruptionQueue value are only used with a validated queue URL,
including the null-to-empty value supplied by the regional cluster
configuration.
- Around line 165-182: The kubectl heredoc in the prewarm manifest has a
space-indented terminator that <<- cannot recognize. Update the heredoc in the
ECS bootstrap script so its terminator is flush-left, or replace it with a
quoted heredoc/emission approach that safely preserves the manifest without
relying on space stripping.
In `@terraform/modules/eks-cluster/iam.tf`:
- Around line 206-247: Remove the unused IAMInstanceProfileCreate and
IAMInstanceProfileModify statements from the Karpenter controller policy, along
with the IAMInstanceProfileRead statement and its GC-specific justification,
since the configured EC2NodeClass instanceProfile is pre-created and Karpenter
does not manage profiles.
- Around line 339-352: Update the aws_sqs_queue_policy.karpenter_interruption
statement to restrict EventBridge sends to this AWS account using the existing
data.aws_caller_identity.current account ID, adding the appropriate
source-account condition while preserving the queue ARN and send-message
permissions.
In `@terraform/modules/eks-cluster/main.tf`:
- Around line 126-150: Update the destroy-time provisioner’s EC2 discovery
filter to match Karpenter instances using the
kubernetes.io/cluster/${local.cluster_id} ownership tag alongside the existing
nodeclaim tag, rather than relying on aws:eks:cluster-name. Preserve the
no-instance handling, and add a bounded retry/timeout mechanism around aws ec2
wait instance-terminated so cluster destruction cannot stall indefinitely.
- Around line 169-185: Update the aws_eks_addon.coredns and
aws_eks_addon.metrics_server resources to provide a schedulable target by adding
configuration_values with a CriticalAddonsOnly=true:NoSchedule toleration. Keep
the existing cluster_name and depends_on settings unchanged; the pod_identity
addon does not require this change.
---
Outside diff comments:
In `@scripts/buildspec/register.sh`:
- Around line 67-104: The register CodeBuild project timeout is shorter than the
maximum duration of the registration flow. Update aws_codebuild_project.register
in the pipeline-management-cluster Terraform configuration to increase
build_timeout to at least 90 minutes, preferably with additional buffer, while
leaving the existing retry behavior unchanged.
---
Minor comments:
In `@argocd/config/management-cluster/hypershift/templates/05-job.yaml`:
- Around line 134-144: Update the external-dns ClusterRole patch in the job
template so existing HyperShift-generated rules are preserved and the required
discovery, networking, and route permissions are appended rather than replacing
the entire rules array. Prefer reading the current rules and applying an
appropriate JSON Patch operation; also revise the nearby comment to accurately
describe the augmentation behavior.
In `@argocd/config/regional-cluster/monitoring/values.yaml`:
- Around line 69-80: Update the RC prometheusOperator configuration to add the
CriticalAddonsOnly toleration matching the existing MC prometheusOperator
settings, allowing scheduling on nodes tainted CriticalAddonsOnly:NoSchedule.
Preserve the current admissionWebhooks and tls settings.
In `@docs/design/karpenter-node-provisioning.md`:
- Around line 7-10: Update docs/design/karpenter-node-provisioning.md at lines
7-10 and 25-32 to describe IRSA as this deployment’s chosen bootstrap and
compatibility path, without implying that EKS Pod Identity is unsupported or
that IRSA is the only valid option. Update docs/design/zoa-trusted-actions.md at
line 825 to state that Pod Identity is preferred for new workload service
accounts while IRSA remains supported.
In `@scripts/validate-rc-aws.sh`:
- Around line 229-232: Update the PLATFORM_API_TG_ARN guidance in the target
health check fallback within the validation script to retrieve the target group
ARN from the TargetGroupBinding resource rather than the Service annotation.
Preserve the existing skip warning and provide a kubectl command that queries
the TargetGroupBinding’s targetGroupARN field.
- Around line 135-151: Move the node-group health-check block containing
ng_desired, ng_ready, and the pass/fail messages inside the existing ACTIVE
branch, matching the control flow in validate-mc-aws.sh. Remove the standalone
duplicate block so describe-nodegroup failures cannot be interpreted as zero
health issues after a prior failure.
In `@terraform/modules/bastion/log-collection-task.tf`:
- Around line 83-84: Remove the duplicate ec2nodeclasses.karpenter.k8s.aws entry
from the resource list, keeping its existing earlier occurrence and preserving
the intended batch-of-5 grouping.
In `@terraform/modules/eks-cluster/variables.tf`:
- Around line 78-82: Update the description of the ami_kms_key_arn variable to
accurately describe only the Karpenter controller IAM policy that is created,
including its kms:CreateGrant and kms:DescribeKey permissions; remove claims
about kms:Decrypt and node-role policy creation while preserving the empty-value
behavior.
---
Nitpick comments:
In `@argocd/config/management-cluster/eks-nodepool/templates/00-nodeclass.yaml`:
- Around line 7-8: Update the amiSelectorTerms configuration in the fips
NodeClass to replace the floating bottlerocket@latest alias with a specific
versioned Bottlerocket AMI alias. Use the approved bottlerocket@v1.x.y version
so node images remain deterministic and auditable.
In `@argocd/config/management-cluster/monitoring/values.yaml`:
- Around line 19-33: Because admissionWebhooks are disabled in the monitoring
Helm values, add a CI validation step for repository-managed PrometheusRule
manifests that runs promtool check rules before deployment. Keep
defaultRules.create false and retain the existing webhook and TLS settings,
ensuring malformed rules or relabel configurations are rejected during CI.
In
`@argocd/config/regional-cluster/grafana/templates/sre-targetgroupbinding.yaml`:
- Line 2: Update the apiVersion for every TargetGroupBinding manifest to
elbv2.k8s.aws/v1beta1, including
argocd/config/regional-cluster/grafana/templates/sre-targetgroupbinding.yaml
(2-2), argocd/config/regional-cluster/loki/templates/targetgroupbinding.yaml
(2-2, 20-20),
argocd/config/regional-cluster/monitoring/templates/sre-targetgroupbinding.yaml
(2-2),
argocd/config/regional-cluster/platform-api/templates/targetgroupbinding.yaml
(2-2), argocd/config/regional-cluster/thanos/templates/targetgroupbinding.yaml
(2-2, 21-21, 39-39), and
argocd/config/shared/argocd/templates/sre-targetgroupbinding.yaml (2-2);
preserve the existing TargetGroupBinding metadata and specifications.
In `@ci/ephemeral-provider/orchestrator.py`:
- Around line 227-239: The orchestrator method’s documentation must describe
merging defaults.yaml rather than entirely replacing the ephemeral configuration
directory, and its file handling must include both .yaml and .yml files. Update
the method docstring and both override/stale-file discovery loops, preserving
the special defaults.yaml merge behavior while copying other overrides normally.
In `@scripts/buildspec/bootstrap-argocd-mc.sh`:
- Line 33: Update the Terraform init invocation in provision-infra-mc.sh to stop
redirecting stderr to /dev/null, matching the visible stderr behavior already
applied to the bootstrap-argocd-mc.sh init command while preserving the existing
stdout handling.
In `@scripts/buildspec/provision-infra-mc.sh`:
- Line 140: Review the Terraform invocation in the infrastructure provisioning
script and mitigate AWS API throttling introduced by -parallelism=20, including
destroy operations. Configure AWS_MAX_ATTEMPTS and AWS_RETRY_MODE=adaptive in
the buildspec environment, or reduce parallelism if appropriate, while
preserving the existing Terraform action flow.
In `@terraform/config/management-cluster/main.tf`:
- Around line 71-73: Update the management-cluster output assignments for
karpenter_controller_role_arn and karpenter_queue_url to pass through the
corresponding module.management_cluster values directly, removing the redundant
null checks and empty-string fallbacks.
In `@terraform/modules/ecs-bootstrap/main.tf`:
- Around line 229-240: Scope the resource queries in the Helm ownership re-stamp
loop to chart-owned objects only, using a selector such as
app.kubernetes.io/part-of=argocd or an explicit name list. Update the kubectl
get logic within the resource loop while preserving annotation behavior for
matched resources and excluding externally managed objects.
In `@terraform/modules/eks-cluster/versions.tf`:
- Around line 13-16: Update the tls provider constraint in the versions
configuration to use ~> 4.0 instead of >= 4.0, keeping accepted versions within
the 4.x release line while preserving the existing provider declaration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 351d22fe-739f-4d3e-996c-7e1a26c606c3
📒 Files selected for processing (74)
Makefileargocd/config/management-cluster/eks-nodepool/templates/00-nodeclass.yamlargocd/config/management-cluster/eks-nodepool/templates/10-nodepool.yamlargocd/config/management-cluster/eks-nodepool/values.yamlargocd/config/management-cluster/hypershift/templates/05-job.yamlargocd/config/management-cluster/monitoring/values.yamlargocd/config/regional-cluster/aws-load-balancer-controller/Chart.yamlargocd/config/regional-cluster/aws-load-balancer-controller/values.yamlargocd/config/regional-cluster/eks-nodepool/templates/00-nodeclass.yamlargocd/config/regional-cluster/eks-nodepool/templates/10-nodepool.yamlargocd/config/regional-cluster/eks-nodepool/values.yamlargocd/config/regional-cluster/grafana/templates/sre-targetgroupbinding.yamlargocd/config/regional-cluster/loki/templates/targetgroupbinding.yamlargocd/config/regional-cluster/monitoring/templates/sre-targetgroupbinding.yamlargocd/config/regional-cluster/monitoring/values.yamlargocd/config/regional-cluster/platform-api/templates/targetgroupbinding.yamlargocd/config/regional-cluster/thanos/templates/targetgroupbinding.yamlargocd/config/shared/argocd/templates/sre-targetgroupbinding.yamlargocd/config/shared/argocd/values.yamlargocd/config/shared/storageclass/templates/gp3.yamlci/e2e-tests.shci/ephemeral-provider/__init__.pyci/ephemeral-provider/orchestrator.pyconfig/templates/argocd-bootstrap/applicationset.yaml.j2deploy/ephemeral/us-east-1/argocd-bootstrap-management-cluster/applicationset.yamldeploy/ephemeral/us-east-1/argocd-bootstrap-regional-cluster/applicationset.yamldeploy/integration/us-east-1/argocd-bootstrap-management-cluster/applicationset.yamldeploy/integration/us-east-1/argocd-bootstrap-regional-cluster/applicationset.yamldocs/README.mddocs/design/fips-eks-compute.mddocs/design/fully-private-eks-bootstrap.mddocs/design/karpenter-node-provisioning.mddocs/design/logging-platform.mddocs/design/thanos-metrics-infrastructure.mddocs/design/zoa-trusted-actions.mdscripts/buildspec/bootstrap-argocd-mc.shscripts/buildspec/provision-infra-mc.shscripts/buildspec/provision-infra-rc.shscripts/buildspec/register.shscripts/validate-mc-aws.shscripts/validate-mc-k8s.shscripts/validate-rc-aws.shscripts/validate-rc-k8s.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/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
5
to
+9
| name: fips | ||
| spec: | ||
| role: "{{ .Values.global.cluster_name }}-auto-node-role" | ||
| amiSelectorTerms: | ||
| - alias: bottlerocket@latest | ||
| instanceProfile: {{ $clusterName }}-karpenter-node-role |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
fips NodeClass now points at Bottlerocket @latest, which drops the FIPS-specific behavior.
- If FIPS is still required, this regresses the node class; switch back to a FIPS AMI or rename the resource so it doesn’t promise FIPS.
bottlerocket@latestalso leaves the node image drifting on future releases; pin a versioned alias or explicit AMI selector.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@argocd/config/regional-cluster/eks-nodepool/templates/00-nodeclass.yaml`
around lines 5 - 9, Update the fips NodeClass AMI selector to use a
FIPS-specific, version-pinned Bottlerocket alias or explicit AMI selector,
preserving the resource’s FIPS behavior and avoiding the drifting
bottlerocket@latest reference.
| values: | ||
| - auto | ||
| provisioner: ebs.csi.eks.amazonaws.com | ||
| provisioner: ebs.csi.aws.com |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1) Is the EBS CSI driver addon provisioned for both clusters?
rg -n 'aws-ebs-csi-driver|ebs.csi.aws.com|ebs_csi' terraform argocd
# 2) Does the storageclass app declare Replace/Force sync options?
fd . argocd/config/shared/storageclass --exec cat -n {}Repository: openshift-online/rosa-hyperfleet
Length of output: 1774
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- search for sync options / replace settings ---'
rg -n 'Replace=true|Force=true|syncOptions|Replace|Force' argocd/config/shared argocd -g '*.yaml' -g '*.yml' -g '*.json' -g '*.tf' || true
echo '--- list storageclass chart files ---'
fd -t f . argocd/config/shared/storageclass
echo '--- outline storageclass chart files if ast-grep available ---'
for f in $(fd -t f . argocd/config/shared/storageclass); do
echo "### $f"
ast-grep outline "$f" --view compact || true
done
echo '--- read relevant files ---'
for f in \
argocd/config/shared/storageclass/Chart.yaml \
argocd/config/shared/storageclass/values.yaml \
argocd/config/shared/storageclass/templates/gp3.yaml \
argocd/config/shared/storageclass/templates/* \
argocd/config/shared/storageclass/*.yaml \
argocd/config/shared/storageclass/*.yml
do
[ -f "$f" ] && { echo "### $f"; cat -n "$f"; }
doneRepository: openshift-online/rosa-hyperfleet
Length of output: 3196
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- find the storageclass application manifest(s) ---'
rg -n 'storageclass' argocd/config -g '*.yaml' -g '*.yml' || true
echo '--- inspect sync options around the storageclass app ---'
rg -n -A6 -B6 'storageclass|Replace=true|Force=true|syncOptions' argocd/config/shared argocd/config/regional-cluster argocd/config/management-cluster -g '*.yaml' -g '*.yml' || true
echo '--- inspect EBS CSI addon wiring in the eks-cluster module ---'
sed -n '240,430p' terraform/modules/eks-cluster/main.tf
sed -n '1,80p' terraform/modules/eks-cluster/iam.tfRepository: openshift-online/rosa-hyperfleet
Length of output: 8582
Add Replace=true for this StorageClass
provisioner is immutable on existing StorageClass objects, so Argo CD will fail the sync unless this resource is replaced instead of patched.
🤖 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/storageclass/templates/gp3.yaml` at line 7, Add the Argo
CD Replace=true sync option to the gp3 StorageClass metadata annotations so
changes to the immutable provisioner trigger resource replacement rather than
patching.
Comment on lines
+60
to
+68
| ignoreDifferences: | ||
| - group: admissionregistration.k8s.io | ||
| kind: MutatingWebhookConfiguration | ||
| jqPathExpressions: | ||
| - .webhooks[].clientConfig.caBundle | ||
| - group: admissionregistration.k8s.io | ||
| kind: ValidatingWebhookConfiguration | ||
| jqPathExpressions: | ||
| - .webhooks[].clientConfig.caBundle |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Scope webhook CA-bundle ignores to LBC resources.
The shared ApplicationSet template currently suppresses CA-bundle drift for every admission webhook, not just the AWS Load Balancer Controller. Scope each rule by rendered webhook name or isolate it to the LBC Application.
deploy/integration/us-east-1/argocd-bootstrap-management-cluster/applicationset.yaml#L60-L68: Scope the webhook rules to the LBC webhook configurations.deploy/integration/us-east-1/argocd-bootstrap-regional-cluster/applicationset.yaml#L60-L68: Apply the same resource scoping in the regional-cluster ApplicationSet.
📍 Affects 2 files
deploy/integration/us-east-1/argocd-bootstrap-management-cluster/applicationset.yaml#L60-L68(this comment)deploy/integration/us-east-1/argocd-bootstrap-regional-cluster/applicationset.yaml#L60-L68
🤖 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
`@deploy/integration/us-east-1/argocd-bootstrap-management-cluster/applicationset.yaml`
around lines 60 - 68, Scope the ignoreDifferences rules for
MutatingWebhookConfiguration and ValidatingWebhookConfiguration to only the AWS
Load Balancer Controller webhook resources, using their rendered webhook names
or isolating the rules to the LBC Application. Apply the same resource scoping
in
deploy/integration/us-east-1/argocd-bootstrap-management-cluster/applicationset.yaml
lines 60-68 and
deploy/integration/us-east-1/argocd-bootstrap-regional-cluster/applicationset.yaml
lines 60-68.
Comment on lines
+151
to
+167
| # ── [DEBUG] DNS zone creation inputs ───────────────────────────────────────── | ||
| echo "=== [DNS-DEBUG] RC Route53 zone inputs ===" | ||
| echo " AWS account (sts): $(aws sts get-caller-identity --query Account --output text 2>&1)" | ||
| echo " DEPLOY_CONFIG_FILE: ${DEPLOY_CONFIG_FILE}" | ||
| _DBG_PROVISIONER_JSON="deploy/${ENVIRONMENT}/${TARGET_REGION}/pipeline-provisioner-inputs/terraform.json" | ||
| echo " Provisioner JSON: ${_DBG_PROVISIONER_JSON}" | ||
| if [ -f "${_DBG_PROVISIONER_JSON}" ]; then | ||
| echo " .domain in provisioner JSON: $(jq -r '.domain // "(null)"' "${_DBG_PROVISIONER_JSON}")" | ||
| else | ||
| echo " Provisioner JSON NOT FOUND — ENVIRONMENT_DOMAIN will be empty" | ||
| fi | ||
| echo " ENVIRONMENT_DOMAIN: ${ENVIRONMENT_DOMAIN:-<unset>}" | ||
| echo " TF_VAR_environment_domain: ${TF_VAR_environment_domain:-<unset — no DNS zones will be created>}" | ||
| echo " TF_VAR_deployment_name: ${TF_VAR_deployment_name:-<unset>}" | ||
| echo " TF_VAR_zone_shard_count: ${TF_VAR_zone_shard_count:-<unset>}" | ||
| echo " Expected zone name: ${TF_VAR_deployment_name:-<unset>}.${TF_VAR_environment_domain:-<unset>}" | ||
| echo "=== [DNS-DEBUG] end ===" |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
${ENVIRONMENT} is dereferenced before its default is applied.
Line 155 expands ${ENVIRONMENT} unguarded, but the fallback export ENVIRONMENT="${ENVIRONMENT:-staging}" only happens at line 172. Under set -u an unset ENVIRONMENT aborts provisioning inside a debug-only block.
Also, this block (and the one at 194-213) looks like temporary DNS troubleshooting output — want me to open an issue to track removing it before merge to main?
🐛 Proposed fix
-_DBG_PROVISIONER_JSON="deploy/${ENVIRONMENT}/${TARGET_REGION}/pipeline-provisioner-inputs/terraform.json"
+_DBG_PROVISIONER_JSON="deploy/${ENVIRONMENT:-staging}/${TARGET_REGION}/pipeline-provisioner-inputs/terraform.json"📝 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
| # ── [DEBUG] DNS zone creation inputs ───────────────────────────────────────── | |
| echo "=== [DNS-DEBUG] RC Route53 zone inputs ===" | |
| echo " AWS account (sts): $(aws sts get-caller-identity --query Account --output text 2>&1)" | |
| echo " DEPLOY_CONFIG_FILE: ${DEPLOY_CONFIG_FILE}" | |
| _DBG_PROVISIONER_JSON="deploy/${ENVIRONMENT}/${TARGET_REGION}/pipeline-provisioner-inputs/terraform.json" | |
| echo " Provisioner JSON: ${_DBG_PROVISIONER_JSON}" | |
| if [ -f "${_DBG_PROVISIONER_JSON}" ]; then | |
| echo " .domain in provisioner JSON: $(jq -r '.domain // "(null)"' "${_DBG_PROVISIONER_JSON}")" | |
| else | |
| echo " Provisioner JSON NOT FOUND — ENVIRONMENT_DOMAIN will be empty" | |
| fi | |
| echo " ENVIRONMENT_DOMAIN: ${ENVIRONMENT_DOMAIN:-<unset>}" | |
| echo " TF_VAR_environment_domain: ${TF_VAR_environment_domain:-<unset — no DNS zones will be created>}" | |
| echo " TF_VAR_deployment_name: ${TF_VAR_deployment_name:-<unset>}" | |
| echo " TF_VAR_zone_shard_count: ${TF_VAR_zone_shard_count:-<unset>}" | |
| echo " Expected zone name: ${TF_VAR_deployment_name:-<unset>}.${TF_VAR_environment_domain:-<unset>}" | |
| echo "=== [DNS-DEBUG] end ===" | |
| # ── [DEBUG] DNS zone creation inputs ───────────────────────────────────────── | |
| echo "=== [DNS-DEBUG] RC Route53 zone inputs ===" | |
| echo " AWS account (sts): $(aws sts get-caller-identity --query Account --output text 2>&1)" | |
| echo " DEPLOY_CONFIG_FILE: ${DEPLOY_CONFIG_FILE}" | |
| _DBG_PROVISIONER_JSON="deploy/${ENVIRONMENT:-staging}/${TARGET_REGION}/pipeline-provisioner-inputs/terraform.json" | |
| echo " Provisioner JSON: ${_DBG_PROVISIONER_JSON}" | |
| if [ -f "${_DBG_PROVISIONER_JSON}" ]; then | |
| echo " .domain in provisioner JSON: $(jq -r '.domain // "(null)"' "${_DBG_PROVISIONER_JSON}")" | |
| else | |
| echo " Provisioner JSON NOT FOUND — ENVIRONMENT_DOMAIN will be empty" | |
| fi | |
| echo " ENVIRONMENT_DOMAIN: ${ENVIRONMENT_DOMAIN:-<unset>}" | |
| echo " TF_VAR_environment_domain: ${TF_VAR_environment_domain:-<unset — no DNS zones will be created>}" | |
| echo " TF_VAR_deployment_name: ${TF_VAR_deployment_name:-<unset>}" | |
| echo " TF_VAR_zone_shard_count: ${TF_VAR_zone_shard_count:-<unset>}" | |
| echo " Expected zone name: ${TF_VAR_deployment_name:-<unset>}.${TF_VAR_environment_domain:-<unset>}" | |
| echo "=== [DNS-DEBUG] end ===" |
🤖 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-rc.sh` around lines 151 - 167, Apply the
default for ENVIRONMENT before the DNS debug block constructs
_DBG_PROVISIONER_JSON, so unset values remain safe under set -u; preserve the
existing staging fallback used by the later export. Remove the temporary DNS
troubleshooting output blocks, including the block around _DBG_PROVISIONER_JSON
and the related block around lines 194-213, rather than merging debug-only
logging into main.
Comment on lines
+194
to
+213
| echo "=== [DNS-DEBUG] Running: terraform ${TERRAFORM_ACTION} (account: $(aws sts get-caller-identity --query Account --output text 2>&1)) ===" | ||
|
|
||
| terraform "${TERRAFORM_ACTION}" -auto-approve | ||
| _TF_EXIT=$? | ||
|
|
||
| echo "=== [DNS-DEBUG] Post-apply Route53 zone status ===" | ||
| echo " terraform exit code: ${_TF_EXIT}" | ||
| if [ -n "${TF_VAR_environment_domain:-}" ]; then | ||
| echo " terraform output regional_hosted_zone_id: $(terraform output -raw regional_hosted_zone_id 2>&1 || echo '<not available>')" | ||
| echo " terraform output regional_name_servers: $(terraform output -json regional_name_servers 2>&1 || echo '<not available>')" | ||
| echo " Route53 zones matching '${TF_VAR_deployment_name:-}.${TF_VAR_environment_domain:-}' in RC account:" | ||
| aws route53 list-hosted-zones \ | ||
| --query "HostedZones[?contains(Name, '${TF_VAR_deployment_name:-}')].{Name:Name,Id:Id,PrivateZone:Config.PrivateZone}" \ | ||
| --output table 2>&1 || echo " (aws route53 list-hosted-zones failed)" | ||
| else | ||
| echo " TF_VAR_environment_domain was empty — no DNS zone resources were declared; skipping Route53 check" | ||
| fi | ||
| echo "=== [DNS-DEBUG] end ===" | ||
|
|
||
| [ "${_TF_EXIT}" -eq 0 ] || exit "${_TF_EXIT}" |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
_TF_EXIT=$? is dead code under set -e; the post-run debug block never runs on failure.
With set -euo pipefail (line 4), a non-zero terraform ${TERRAFORM_ACTION} aborts the script at line 196, so lines 197-213 only execute when terraform succeeded (_TF_EXIT always 0). Capture the status explicitly.
🐛 Proposed fix
-terraform "${TERRAFORM_ACTION}" -auto-approve
-_TF_EXIT=$?
+_TF_EXIT=0
+terraform "${TERRAFORM_ACTION}" -auto-approve || _TF_EXIT=$?📝 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
| echo "=== [DNS-DEBUG] Running: terraform ${TERRAFORM_ACTION} (account: $(aws sts get-caller-identity --query Account --output text 2>&1)) ===" | |
| terraform "${TERRAFORM_ACTION}" -auto-approve | |
| _TF_EXIT=$? | |
| echo "=== [DNS-DEBUG] Post-apply Route53 zone status ===" | |
| echo " terraform exit code: ${_TF_EXIT}" | |
| if [ -n "${TF_VAR_environment_domain:-}" ]; then | |
| echo " terraform output regional_hosted_zone_id: $(terraform output -raw regional_hosted_zone_id 2>&1 || echo '<not available>')" | |
| echo " terraform output regional_name_servers: $(terraform output -json regional_name_servers 2>&1 || echo '<not available>')" | |
| echo " Route53 zones matching '${TF_VAR_deployment_name:-}.${TF_VAR_environment_domain:-}' in RC account:" | |
| aws route53 list-hosted-zones \ | |
| --query "HostedZones[?contains(Name, '${TF_VAR_deployment_name:-}')].{Name:Name,Id:Id,PrivateZone:Config.PrivateZone}" \ | |
| --output table 2>&1 || echo " (aws route53 list-hosted-zones failed)" | |
| else | |
| echo " TF_VAR_environment_domain was empty — no DNS zone resources were declared; skipping Route53 check" | |
| fi | |
| echo "=== [DNS-DEBUG] end ===" | |
| [ "${_TF_EXIT}" -eq 0 ] || exit "${_TF_EXIT}" | |
| echo "=== [DNS-DEBUG] Running: terraform ${TERRAFORM_ACTION} (account: $(aws sts get-caller-identity --query Account --output text 2>&1)) ===" | |
| _TF_EXIT=0 | |
| terraform "${TERRAFORM_ACTION}" -auto-approve || _TF_EXIT=$? | |
| echo "=== [DNS-DEBUG] Post-apply Route53 zone status ===" | |
| echo " terraform exit code: ${_TF_EXIT}" | |
| if [ -n "${TF_VAR_environment_domain:-}" ]; then | |
| echo " terraform output regional_hosted_zone_id: $(terraform output -raw regional_hosted_zone_id 2>&1 || echo '<not available>')" | |
| echo " terraform output regional_name_servers: $(terraform output -json regional_name_servers 2>&1 || echo '<not available>')" | |
| echo " Route53 zones matching '${TF_VAR_deployment_name:-}.${TF_VAR_environment_domain:-}' in RC account:" | |
| aws route53 list-hosted-zones \ | |
| --query "HostedZones[?contains(Name, '${TF_VAR_deployment_name:-}')].{Name:Name,Id:Id,PrivateZone:Config.PrivateZone}" \ | |
| --output table 2>&1 || echo " (aws route53 list-hosted-zones failed)" | |
| else | |
| echo " TF_VAR_environment_domain was empty — no DNS zone resources were declared; skipping Route53 check" | |
| fi | |
| echo "=== [DNS-DEBUG] end ===" | |
| [ "${_TF_EXIT}" -eq 0 ] || exit "${_TF_EXIT}" |
🤖 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-rc.sh` around lines 194 - 213, Update the
terraform invocation and surrounding logic in the build flow so its non-zero
exit status is captured without triggering set -e, allowing the post-run DNS
debug block to execute on failures. Preserve the existing _TF_EXIT-based final
exit behavior and ensure successful runs continue with status 0.
Comment on lines
+165
to
+182
| kubectl apply -f - <<-PREWARM_EOF | ||
| apiVersion: v1 | ||
| kind: Pod | ||
| metadata: | ||
| name: karpenter-prewarm | ||
| namespace: kube-system | ||
| labels: | ||
| app: karpenter-prewarm | ||
| spec: | ||
| containers: | ||
| - name: pause | ||
| image: public.ecr.aws/eks-distro/kubernetes/pause:3.9 | ||
| resources: | ||
| requests: | ||
| cpu: 100m | ||
| memory: 128Mi | ||
| terminationGracePeriodSeconds: 0 | ||
| PREWARM_EOF |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Show whether the heredoc body/terminator use tabs (^I) or spaces
fd -t f -g 'main.tf' terraform/modules/ecs-bootstrap --exec sed -n '160,205p' | cat -A | sed -n '1,50p'Repository: openshift-online/rosa-hyperfleet
Length of output: 2593
Indent the heredoc terminator to column 1, or drop <<-
<<- strips leading tabs only. With space-indented body lines and PREWARM_EOF, the shell won't recognize the terminator and the bootstrap script fails at parse time. Use a flush-left quoted heredoc or another manifest emission path so indentation can't break this.
🤖 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/main.tf` around lines 165 - 182, The kubectl
heredoc in the prewarm manifest has a space-indented terminator that <<- cannot
recognize. Update the heredoc in the ECS bootstrap script so its terminator is
flush-left, or replace it with a quoted heredoc/emission approach that safely
preserves the manifest without relying on space stripping.
Comment on lines
+206
to
+247
| { | ||
| Sid = "IAMInstanceProfileCreate" | ||
| Effect = "Allow" | ||
| Action = [ | ||
| "iam:CreateInstanceProfile", | ||
| "iam:TagInstanceProfile", | ||
| ] | ||
| Resource = "*" | ||
| Condition = { | ||
| StringEquals = { | ||
| "aws:RequestTag/kubernetes.io/cluster/${local.cluster_id}" = "owned" | ||
| } | ||
| } | ||
| }, | ||
| { | ||
| Sid = "IAMInstanceProfileModify" | ||
| Effect = "Allow" | ||
| Action = [ | ||
| "iam:AddRoleToInstanceProfile", | ||
| "iam:DeleteInstanceProfile", | ||
| "iam:RemoveRoleFromInstanceProfile", | ||
| ] | ||
| Resource = "*" | ||
| Condition = { | ||
| StringEquals = { | ||
| "aws:ResourceTag/kubernetes.io/cluster/${local.cluster_id}" = "owned" | ||
| } | ||
| } | ||
| }, | ||
| { | ||
| # GetInstanceProfile and ListInstanceProfiles are read-only and must be | ||
| # unconditional: Karpenter calls GetInstanceProfile before creating (and | ||
| # tagging) a profile, so a ResourceTag condition always denies it. | ||
| # ListInstanceProfiles is required by the instance-profile GC controller. | ||
| Sid = "IAMInstanceProfileRead" | ||
| Effect = "Allow" | ||
| Action = [ | ||
| "iam:GetInstanceProfile", | ||
| "iam:ListInstanceProfiles", | ||
| ] | ||
| Resource = "*" | ||
| }, |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Instance-profile write permissions contradict the pre-created profile design.
Lines 68-70 state that pre-creating aws_iam_instance_profile.karpenter_node "removes the need for iam:CreateInstanceProfile in the Karpenter controller policy", yet IAMInstanceProfileCreate/IAMInstanceProfileModify still grant CreateInstanceProfile, TagInstanceProfile, AddRoleToInstanceProfile, DeleteInstanceProfile, and RemoveRoleFromInstanceProfile on Resource = "*". With EC2NodeClass.spec.instanceProfile set (as in argocd/config/regional-cluster/eks-nodepool/templates/00-nodeclass.yaml Line 9), Karpenter does not manage instance profiles, so these are unused write grants on an IRSA role.
Either drop these statements (and the read statement's GC justification) or fix the comment to explain why they're retained.
🔒 Proposed least-privilege trim
- {
- Sid = "IAMInstanceProfileCreate"
- Effect = "Allow"
- Action = [
- "iam:CreateInstanceProfile",
- "iam:TagInstanceProfile",
- ]
- Resource = "*"
- Condition = {
- StringEquals = {
- "aws:RequestTag/kubernetes.io/cluster/${local.cluster_id}" = "owned"
- }
- }
- },
- {
- Sid = "IAMInstanceProfileModify"
- Effect = "Allow"
- Action = [
- "iam:AddRoleToInstanceProfile",
- "iam:DeleteInstanceProfile",
- "iam:RemoveRoleFromInstanceProfile",
- ]
- Resource = "*"
- Condition = {
- StringEquals = {
- "aws:ResourceTag/kubernetes.io/cluster/${local.cluster_id}" = "owned"
- }
- }
- },
{
- # GetInstanceProfile and ListInstanceProfiles are read-only and must be
- # unconditional: Karpenter calls GetInstanceProfile before creating (and
- # tagging) a profile, so a ResourceTag condition always denies it.
- # ListInstanceProfiles is required by the instance-profile GC controller.
+ # Read-only: Karpenter resolves EC2NodeClass.spec.instanceProfile via
+ # GetInstanceProfile; ListInstanceProfiles is used by the GC controller.
Sid = "IAMInstanceProfileRead"📝 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
| { | |
| Sid = "IAMInstanceProfileCreate" | |
| Effect = "Allow" | |
| Action = [ | |
| "iam:CreateInstanceProfile", | |
| "iam:TagInstanceProfile", | |
| ] | |
| Resource = "*" | |
| Condition = { | |
| StringEquals = { | |
| "aws:RequestTag/kubernetes.io/cluster/${local.cluster_id}" = "owned" | |
| } | |
| } | |
| }, | |
| { | |
| Sid = "IAMInstanceProfileModify" | |
| Effect = "Allow" | |
| Action = [ | |
| "iam:AddRoleToInstanceProfile", | |
| "iam:DeleteInstanceProfile", | |
| "iam:RemoveRoleFromInstanceProfile", | |
| ] | |
| Resource = "*" | |
| Condition = { | |
| StringEquals = { | |
| "aws:ResourceTag/kubernetes.io/cluster/${local.cluster_id}" = "owned" | |
| } | |
| } | |
| }, | |
| { | |
| # GetInstanceProfile and ListInstanceProfiles are read-only and must be | |
| # unconditional: Karpenter calls GetInstanceProfile before creating (and | |
| # tagging) a profile, so a ResourceTag condition always denies it. | |
| # ListInstanceProfiles is required by the instance-profile GC controller. | |
| Sid = "IAMInstanceProfileRead" | |
| Effect = "Allow" | |
| Action = [ | |
| "iam:GetInstanceProfile", | |
| "iam:ListInstanceProfiles", | |
| ] | |
| Resource = "*" | |
| }, | |
| { | |
| # Read-only: Karpenter resolves EC2NodeClass.spec.instanceProfile via | |
| # GetInstanceProfile; ListInstanceProfiles is used by the GC controller. | |
| Sid = "IAMInstanceProfileRead" | |
| Effect = "Allow" | |
| Action = [ | |
| "iam:GetInstanceProfile", | |
| "iam:ListInstanceProfiles", | |
| ] | |
| Resource = "*" | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@terraform/modules/eks-cluster/iam.tf` around lines 206 - 247, Remove the
unused IAMInstanceProfileCreate and IAMInstanceProfileModify statements from the
Karpenter controller policy, along with the IAMInstanceProfileRead statement and
its GC-specific justification, since the configured EC2NodeClass instanceProfile
is pre-created and Karpenter does not manage profiles.
Comment on lines
+339
to
+352
| 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
SQS queue policy is missing a source condition — any AWS account's EventBridge can enqueue interruption messages.
The statement allows events.amazonaws.com to SendMessage with no aws:SourceArn/aws:SourceAccount condition, which is the classic confused-deputy pattern: a rule in a foreign account targeting this queue ARN would be authorized, letting an outside party inject fabricated interruption events that cause Karpenter to cordon and drain nodes. Scope it to this account's rules.
🔒 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
+ }
+ }
}]Confirm data.aws_caller_identity.current exists in this module (it is referenced elsewhere in the repo's Terraform); otherwise add it to data.tf.
📝 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 339 - 352, Update the
aws_sqs_queue_policy.karpenter_interruption statement to restrict EventBridge
sends to this AWS account using the existing data.aws_caller_identity.current
account ID, adding the appropriate source-account condition while preserving the
queue ARN and send-message permissions.
Comment on lines
+126
to
+150
| provisioner "local-exec" { | ||
| when = destroy | ||
| on_failure = continue | ||
| command = <<-EOT | ||
| CLUSTER_NAME="${self.name}" | ||
| REGION=$(echo "${self.arn}" | cut -d: -f4) | ||
| echo "Terminating Karpenter EC2 instances for cluster: $CLUSTER_NAME" | ||
| INSTANCE_IDS=$(aws ec2 describe-instances \ | ||
| --region "$REGION" \ | ||
| --filters \ | ||
| "Name=tag:aws:eks:cluster-name,Values=$CLUSTER_NAME" \ | ||
| "Name=tag-key,Values=karpenter.sh/nodeclaim" \ | ||
| "Name=instance-state-name,Values=pending,running,stopping,stopped" \ | ||
| --query 'Reservations[].Instances[].InstanceId' \ | ||
| --output text) | ||
| if [ -z "$INSTANCE_IDS" ]; then | ||
| echo "No Karpenter-managed instances found." | ||
| exit 0 | ||
| fi | ||
| echo "Terminating: $INSTANCE_IDS" | ||
| aws ec2 terminate-instances --region "$REGION" --instance-ids $INSTANCE_IDS | ||
| aws ec2 wait instance-terminated --region "$REGION" --instance-ids $INSTANCE_IDS | ||
| echo "Done." | ||
| EOT | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
The destroy-time cleanup likely matches zero instances: Karpenter does not set aws:eks:cluster-name.
aws:* tags are AWS-managed and applied by EKS for managed node groups / Auto Mode; instances launched by Karpenter's own CreateFleet/RunInstances carry karpenter.sh/nodeclaim, karpenter.sh/nodepool and kubernetes.io/cluster/<name>=owned — the latter being exactly the tag this module's controller policy forces via aws:RequestTag/kubernetes.io/cluster/${local.cluster_id} (terraform/modules/eks-cluster/iam.tf Line 149). With on_failure = continue and the exit 0 short-circuit, a non-matching filter degrades silently into the original DependencyViolation teardown failure this block is meant to prevent.
Separately, aws ec2 wait instance-terminated has no bound and can stall a destroy for a long time; consider capping it.
🐛 Proposed fix
INSTANCE_IDS=$(aws ec2 describe-instances \
--region "$REGION" \
--filters \
- "Name=tag:aws:eks:cluster-name,Values=$CLUSTER_NAME" \
+ "Name=tag:kubernetes.io/cluster/$CLUSTER_NAME,Values=owned" \
"Name=tag-key,Values=karpenter.sh/nodeclaim" \
"Name=instance-state-name,Values=pending,running,stopping,stopped" \
--query 'Reservations[].Instances[].InstanceId' \
--output text)
if [ -z "$INSTANCE_IDS" ]; then
echo "No Karpenter-managed instances found."
exit 0
fi
echo "Terminating: $INSTANCE_IDS"
aws ec2 terminate-instances --region "$REGION" --instance-ids $INSTANCE_IDS
- aws ec2 wait instance-terminated --region "$REGION" --instance-ids $INSTANCE_IDS
+ aws ec2 wait instance-terminated --region "$REGION" --instance-ids $INSTANCE_IDS || \
+ echo "WARNING: instances not confirmed terminated; VPC teardown may hit DependencyViolation."📝 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
| provisioner "local-exec" { | |
| when = destroy | |
| on_failure = continue | |
| command = <<-EOT | |
| CLUSTER_NAME="${self.name}" | |
| REGION=$(echo "${self.arn}" | cut -d: -f4) | |
| echo "Terminating Karpenter EC2 instances for cluster: $CLUSTER_NAME" | |
| INSTANCE_IDS=$(aws ec2 describe-instances \ | |
| --region "$REGION" \ | |
| --filters \ | |
| "Name=tag:aws:eks:cluster-name,Values=$CLUSTER_NAME" \ | |
| "Name=tag-key,Values=karpenter.sh/nodeclaim" \ | |
| "Name=instance-state-name,Values=pending,running,stopping,stopped" \ | |
| --query 'Reservations[].Instances[].InstanceId' \ | |
| --output text) | |
| if [ -z "$INSTANCE_IDS" ]; then | |
| echo "No Karpenter-managed instances found." | |
| exit 0 | |
| fi | |
| echo "Terminating: $INSTANCE_IDS" | |
| aws ec2 terminate-instances --region "$REGION" --instance-ids $INSTANCE_IDS | |
| aws ec2 wait instance-terminated --region "$REGION" --instance-ids $INSTANCE_IDS | |
| echo "Done." | |
| EOT | |
| } | |
| provisioner "local-exec" { | |
| when = destroy | |
| on_failure = continue | |
| command = <<-EOT | |
| CLUSTER_NAME="${self.name}" | |
| REGION=$(echo "${self.arn}" | cut -d: -f4) | |
| echo "Terminating Karpenter EC2 instances for cluster: $CLUSTER_NAME" | |
| INSTANCE_IDS=$(aws ec2 describe-instances \ | |
| --region "$REGION" \ | |
| --filters \ | |
| "Name=tag:kubernetes.io/cluster/$CLUSTER_NAME,Values=owned" \ | |
| "Name=tag-key,Values=karpenter.sh/nodeclaim" \ | |
| "Name=instance-state-name,Values=pending,running,stopping,stopped" \ | |
| --query 'Reservations[].Instances[].InstanceId' \ | |
| --output text) | |
| if [ -z "$INSTANCE_IDS" ]; then | |
| echo "No Karpenter-managed instances found." | |
| exit 0 | |
| fi | |
| echo "Terminating: $INSTANCE_IDS" | |
| aws ec2 terminate-instances --region "$REGION" --instance-ids $INSTANCE_IDS | |
| aws ec2 wait instance-terminated --region "$REGION" --instance-ids $INSTANCE_IDS || \ | |
| echo "WARNING: instances not confirmed terminated; VPC teardown may hit DependencyViolation." | |
| echo "Done." | |
| EOT | |
| } |
🤖 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 126 - 150, Update the
destroy-time provisioner’s EC2 discovery filter to match Karpenter instances
using the kubernetes.io/cluster/${local.cluster_id} ownership tag alongside the
existing nodeclaim tag, rather than relying on aws:eks:cluster-name. Preserve
the no-instance handling, and add a bounded retry/timeout mechanism around aws
ec2 wait instance-terminated so cluster destruction cannot stall indefinitely.
Comment on lines
169
to
+185
| resource "aws_eks_addon" "coredns" { | ||
| cluster_name = aws_eks_cluster.main.name | ||
| addon_name = "coredns" | ||
| depends_on = [aws_eks_node_group.karpenter_bootstrap] | ||
| } | ||
|
|
||
| resource "aws_eks_addon" "metrics_server" { | ||
| cluster_name = aws_eks_cluster.main.name | ||
| addon_name = "metrics-server" | ||
| depends_on = [aws_eks_node_group.karpenter_bootstrap] | ||
| } | ||
|
|
||
| resource "aws_eks_addon" "pod_identity" { | ||
| cluster_name = aws_eks_cluster.main.name | ||
| addon_name = "eks-pod-identity-agent" | ||
| depends_on = [aws_eks_node_group.karpenter_bootstrap] | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for CriticalAddonsOnly tolerations across charts and terraform addon config
rg -n -C4 'CriticalAddonsOnly' --glob '!**/vendor/**'
# Check karpenter controller chart/values for tolerations + nodeSelector
fd -i karpenter --type f | head -50Repository: openshift-online/rosa-hyperfleet
Length of output: 170
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant region in terraform/modules/eks-cluster/main.tf
cat -n terraform/modules/eks-cluster/main.tf | sed -n '150,240p'
echo
echo '--- search addon/toleration references ---'
rg -n -C3 '(aws_eks_addon|coredns|metrics-server|eks-pod-identity-agent|CriticalAddonsOnly|toleration|tolerations|karpenter_bootstrap|bootstrap taint|taint)' terraform/modules terraform -g '!**/vendor/**' || true
echo
echo '--- find karpenter-related files ---'
fd -i karpenter . -t f | sed -n '1,80p'Repository: openshift-online/rosa-hyperfleet
Length of output: 38533
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- search for system pool / additional node groups in eks-cluster ---'
rg -n -C3 'system pool|system" pool|node_group_name|managed node group|aws_eks_node_group|CriticalAddonsOnly' terraform/modules/eks-cluster terraform/modules -g '!**/vendor/**' || true
echo
echo '--- inspect nearby node group definitions in terraform/modules/eks-cluster/main.tf ---'
cat -n terraform/modules/eks-cluster/main.tf | sed -n '1,160p'
echo
echo '--- inspect docs describing cluster node pools ---'
cat -n terraform/modules/eks-cluster/README.md | sed -n '120,170p'Repository: openshift-online/rosa-hyperfleet
Length of output: 29181
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- search for any other EKS node groups or Fargate profiles in the repo ---'
rg -n -C2 'resource "aws_eks_node_group"|resource "aws_eks_fargate_profile"|system pool|System pool|managed node group|Fargate' terraform modules docs -g '!**/vendor/**' || true
echo
echo '--- search for addon configuration_values or toleration settings in EKS addons ---'
rg -n -C3 'configuration_values|tolerations|CriticalAddonsOnly|addon_name\s*=\s*"(coredns|metrics-server|eks-pod-identity-agent)"' terraform/modules/eks-cluster terraform/modules/ecs-bootstrap -g '!**/vendor/**' || true
echo
echo '--- inspect the addon section after line 239 for any related configs ---'
cat -n terraform/modules/eks-cluster/main.tf | sed -n '239,290p'Repository: openshift-online/rosa-hyperfleet
Length of output: 36109
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- search EKS cluster settings for auto mode / system pools / capacity type ---'
rg -n -C3 'compute_config|auto_mode|system pool|system_pool|managed node group|karpenter-bootstrap|bootstrap_self_managed_addons|fargate|capacity_type|node group' terraform/modules/eks-cluster terraform/config -g '!**/vendor/**' || true
echo
echo '--- inspect top of terraform/modules/eks-cluster/main.tf for any mode flags or locals ---'
cat -n terraform/modules/eks-cluster/main.tf | sed -n '1,120p'Repository: openshift-online/rosa-hyperfleet
Length of output: 10811
CoreDNS and metrics-server still need a schedulable target. depends_on only orders creation; this module only defines the tainted karpenter_bootstrap node group, so those addons can remain Pending until Karpenter comes up. Add a CriticalAddonsOnly=true:NoSchedule toleration via configuration_values, or place them on separate untainted 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 `@terraform/modules/eks-cluster/main.tf` around lines 169 - 185, Update the
aws_eks_addon.coredns and aws_eks_addon.metrics_server resources to provide a
schedulable target by adding configuration_values with a
CriticalAddonsOnly=true:NoSchedule toleration. Keep the existing cluster_name
and depends_on settings unchanged; the pod_identity addon does not require this
change.
|
@theautoroboto: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
5 tasks
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
Temporary PR to confirm the e2e baseline before stacking more commits.
6e14d0eb("Reduce platform monitoring e2e suite timeout from 25m to 15m") — the last commit on the Karpenter migration with a recorded e2e SUCCESS (PR ROSAENG-60886: Migrate from EKS Auto Mode to OSS Karpenter #698 run 2082243737093672960)remove-auto-mode-v2Test plan
/test on-demand-e2e🤖 Generated with Claude Code
Summary by CodeRabbit