ROSAENG-62444: feat: replace DynamoDB Streams with EventBridge Pipes for specs and status notification paths - #736
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
needs-ok-to-test
|
Hi @rrp-bot. Thanks for your PR. I'm waiting for a openshift-online member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
📝 WalkthroughWalkthroughThe pull request adds cross-account kube-applier messaging through DynamoDB streams, EventBridge Pipes, SQS, SNS, KMS, IAM, Terraform, Helm, and bootstrap integration. It also adds DynamoDB, SNS, and Manifest timing utilities for ephemeral environments. ChangesKube-applier messaging
Ephemeral timing tools
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant DynamoDB
participant EventBridgePipes
participant SNSTopics
participant SQS
participant KubeApplier
DynamoDB->>EventBridgePipes: Emit INSERT/MODIFY stream events
EventBridgePipes->>SNSTopics: Publish specs or status notifications
SNSTopics->>SQS: Deliver messages to management or regional queues
SQS->>KubeApplier: Provide notification messages
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
argocd/config/regional-cluster/hyperfleet/values.yaml (1)
12-25: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winPin the chart revision and image digest for regional
hyperfleet-operator.
targetRevisioncurrently points to the mutable branchfeature/eventbridge-pipes, andimage.taguses the mutable registry tag7965486. Argo CD can deploy changed chart content or container bytes without a change in this repository. Use a full commit SHA for this branch and an image digest for the container bytes. Runmake pre-pushand verify the regionalhyperfleet-operatorApplication syncs and becomes healthy.🤖 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/hyperfleet/values.yaml` around lines 12 - 25, Update the regional hyperfleet-operator source to use the branch’s full immutable commit SHA instead of targetRevision feature/eventbridge-pipes, and replace image.tag 7965486 with the corresponding immutable container digest. Run make pre-push and verify the regional hyperfleet-operator Application synchronizes successfully and becomes healthy.
🧹 Nitpick comments (1)
scripts/desire_timing_test_sns.py (1)
109-281: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExtract the shared logic instead of forking the whole script.
Lines 109-281 duplicate
scripts/desire_timing_test.pylines 91-263 verbatim, and the polling helpers and summary block duplicate it too. Theexpect_typedefect already exists in both copies, which shows the divergence cost.Move
parse_envs_file,pick_env, the item builders, and the polling helpers into a shared module. Keep only the SNS publish steps in this file. Alternatively, add an--sns-topic-arnflag toscripts/desire_timing_test.pyand delete this file.🤖 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/desire_timing_test_sns.py` around lines 109 - 281, Remove the duplicated environment parsing, item construction, polling, and summary logic from this SNS script by reusing shared helpers extracted from desire_timing_test.py. Keep this file limited to SNS-specific publish orchestration, or instead add an --sns-topic-arn option to desire_timing_test.py and delete the duplicate script; ensure fixes such as expect_type are maintained in only one implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/bootstrap-argocd.sh`:
- Around line 111-117: Update the management-cluster branch around
KUBE_APPLIER_SQS_QUEUE_URL so a missing or empty Terraform queue output causes
bootstrap to fail before any aws ecs run-task invocation. Keep the default empty
value and current behavior unchanged for regional-cluster, and validate the
populated value after parsing OUTPUTS.
In `@scripts/buildspec/provision-kube-applier-dynamodb.sh`:
- Around line 46-47: Validate operator_replica_count as an integer in the
provisioning flow before exporting TF_VAR_operator_replica_count, rejecting
fractional values while preserving the existing allowed range of 1 through 10.
Implement this in the variable validation for operator_replica_count or
immediately before its export in the build script.
In `@scripts/desire_timing_test_sns.py`:
- Around line 424-434: Remove the STS fallback in the SNS ARN setup around
rc_account_id and require an explicit --rc-account-id value, validating it
before constructing sns_topic_arn. Preserve the documented RC account ID
behavior and avoid using session.client("sts").get_caller_identity(), which
identifies the MC account.
In `@scripts/desire_timing_test.py`:
- Around line 271-310: Update poll_apply_status in
scripts/desire_timing_test.py#L271-L310 and
scripts/desire_timing_test_sns.py#L308-L347 to use expect_type when matching the
Successful=True condition, or otherwise gate on the incremented status version.
Ensure the step 7 delete-phase calls wait for the delete confirmation rather
than reusing the prior apply confirmation.
In `@scripts/manifest-timing-test.sh`:
- Around line 67-78: Update the timestamp validation in elapsed_ms to verify
that START_NS and now_ns contain numeric values before using the nanosecond
arithmetic branch, rather than relying on string length. Ensure unsupported date
+%s%N output such as a trailing literal N falls back to the existing
seconds-based calculation.
In `@terraform/modules/ecs-bootstrap/variables.tf`:
- Around line 85-89: Update the description of the kube_applier_specs_queue_url
variable to identify the URL as belonging to the regional-cluster SQS queue,
using “RC-side” or equivalent wording that states it is consumed by the
management-cluster kube-applier; leave the variable type and default unchanged.
In `@terraform/modules/kube-applier-mc-messaging/main.tf`:
- Around line 52-54: Add an explicit depends_on = [module.kube_applier] to the
module "kube_applier_mc_messaging" call in the management-cluster configuration
so the kube-applier role is created before its messaging policy when both
modules are applied together.
- Around line 69-76: Update the kube-applier messaging module wiring to accept
and pass module.kube_applier_rc_messaging.kms_key_arn, then change the
SpecsQueueKMSDecrypt statement’s Resource from local.rc_kms_key_arn_pattern to
that specific ARN. Retain kms:GenerateDataKey* only if the queue consumer
requires it; otherwise remove the action.
In `@terraform/modules/kube-applier-rc-messaging/main.tf`:
- Around line 321-325: Update all four dynamodb_stream_parameters blocks in
terraform/modules/kube-applier-rc-messaging/main.tf at lines 321-325, 358-362,
465-469, and 500-504 to use TRIM_HORIZON instead of LATEST for
starting_position; no site requires a separate change beyond applying this same
setting consistently.
In `@terraform/modules/kube-applier-rc-messaging/variables.tf`:
- Around line 35-44: Keep operator_replica_count independent of DynamoDB Stream
reader count in terraform/modules/kube-applier-rc-messaging/variables.tf:35-44
and terraform/config/kube-applier-dynamodb-provisioning/variables.tf:57-61. In
terraform/modules/kube-applier-rc-messaging/main.tf:458-491 and :493-526,
replace each per-replica status_applydesires and status_readdesires Pipe set
with one source Pipe per stream and downstream fan-out to the operator replicas,
ensuring each DynamoDB stream has only one reader.
In `@terraform/modules/kube-applier-rc-messaging/versions.tf`:
- Around line 5-8: Update the aws provider constraint in the versions
configuration to use a minimum version of 5.49.0, ensuring the
rotation_period_in_days usage in aws_kms_key remains supported.
---
Outside diff comments:
In `@argocd/config/regional-cluster/hyperfleet/values.yaml`:
- Around line 12-25: Update the regional hyperfleet-operator source to use the
branch’s full immutable commit SHA instead of targetRevision
feature/eventbridge-pipes, and replace image.tag 7965486 with the corresponding
immutable container digest. Run make pre-push and verify the regional
hyperfleet-operator Application synchronizes successfully and becomes healthy.
---
Nitpick comments:
In `@scripts/desire_timing_test_sns.py`:
- Around line 109-281: Remove the duplicated environment parsing, item
construction, polling, and summary logic from this SNS script by reusing shared
helpers extracted from desire_timing_test.py. Keep this file limited to
SNS-specific publish orchestration, or instead add an --sns-topic-arn option to
desire_timing_test.py and delete the duplicate script; ensure fixes such as
expect_type are maintained in only one implementation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6768e5c8-7e96-4453-8efd-3ee9bb308560
📒 Files selected for processing (32)
argocd/config/management-cluster/kube-applier/templates/deployment.yamlargocd/config/management-cluster/kube-applier/values.yamlargocd/config/regional-cluster/hyperfleet/templates/application.yamlargocd/config/regional-cluster/hyperfleet/values.yamlconfig/templates/argocd-bootstrap/applicationset.yaml.j2deploy/ephemeral/us-east-1/argocd-bootstrap-management-cluster/applicationset.yamldeploy/ephemeral/us-east-1/argocd-bootstrap-regional-cluster/applicationset.yamldeploy/integration/us-east-1/argocd-bootstrap-management-cluster/applicationset.yamldeploy/integration/us-east-1/argocd-bootstrap-regional-cluster/applicationset.yamlscripts/bootstrap-argocd.shscripts/buildspec/provision-kube-applier-dynamodb.shscripts/buildspec/register.shscripts/desire_timing_test.pyscripts/desire_timing_test_sns.pyscripts/manifest-timing-test.shterraform/config/kube-applier-dynamodb-provisioning/main.tfterraform/config/kube-applier-dynamodb-provisioning/outputs.tfterraform/config/kube-applier-dynamodb-provisioning/variables.tfterraform/config/management-cluster/main.tfterraform/config/management-cluster/outputs.tfterraform/config/management-cluster/variables.tfterraform/modules/ecs-bootstrap/main.tfterraform/modules/ecs-bootstrap/variables.tfterraform/modules/kube-applier-dynamodb/outputs.tfterraform/modules/kube-applier-mc-messaging/main.tfterraform/modules/kube-applier-mc-messaging/outputs.tfterraform/modules/kube-applier-mc-messaging/variables.tfterraform/modules/kube-applier-mc-messaging/versions.tfterraform/modules/kube-applier-rc-messaging/main.tfterraform/modules/kube-applier-rc-messaging/outputs.tfterraform/modules/kube-applier-rc-messaging/variables.tfterraform/modules/kube-applier-rc-messaging/versions.tf
💤 Files with no reviewable changes (1)
- terraform/config/management-cluster/variables.tf
| TF_VAR_operator_replica_count=$(jq -r '.operator_replica_count // 3' "$DEPLOY_CONFIG_FILE") | ||
| export TF_VAR_operator_replica_count |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 4 \
'operator_replica_count|count\s*=\s*var\.operator_replica_count' \
scripts terraform/modulesRepository: openshift-online/rosa-hyperfleet
Length of output: 6198
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== shell script relevant section =="
sed -n '1,90p' scripts/buildspec/provision-kube-applier-dynamodb.sh
echo
echo "== terraform variable validation relevant section =="
sed -n '30,45p' terraform/modules/kube-applier-rc-messaging/variables.tf
echo
echo "== count/range usages in module =="
rg -n -C 2 'count\s*=|range\(var\.operator_replica_count\)|operator_replica_count' terraform/modules/kube-applier-rc-messaging
echo
echo "== deterministic validation probe for fractional input from current script =="
python3 - <<'PY'
import json, math, re
val = 1.5
# jq '.operator_replica_count // 3' on the JSON number 1.5 outputs 1.5
parsed = json.loads('1.5')
print("parsed", parsed, "type", type(parsed).__name__)
print("current condition", parsed >= 1 and parsed <= 10)
print("integer condition", parsed >= 1 and parsed <= 10 and parsed == math.floor(parsed))
PY
echo
echo "== Terraform installed? =="
if command -v terraform >/dev/null 2>&1; then
terraform version -json | python3 -c "import sys,json; print(json.load(sys.stdin).get('terraform_version','unknown'))"
else
echo "terraform not found"
fiRepository: openshift-online/rosa-hyperfleet
Length of output: 6526
Reject fractional operator_replica_count values before Terraform.
operator_replica_count is exported directly from JSON and Terraform uses it as count for queues and pipes. The current validation only checks 1 to 10, so 1.5 reaches Terraform and fails evaluation. Enforce integer values in terraform/modules/kube-applier-rc-messaging/variables.tf or reject them in scripts/buildspec/provision-kube-applier-dynamodb.sh before export.
🤖 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-kube-applier-dynamodb.sh` around lines 46 - 47,
Validate operator_replica_count as an integer in the provisioning flow before
exporting TF_VAR_operator_replica_count, rejecting fractional values while
preserving the existing allowed range of 1 through 10. Implement this in the
variable validation for operator_replica_count or immediately before its export
in the build script.
Source: Path instructions
| # -- resolve RC account ID for SNS topic ARN -- | ||
| # Topic ARN: arn:aws:sns:<region>:<rc_account_id>:<cluster_name>-specs-notifications | ||
| # (matches snspublisher.Publisher.TopicARN() in hyperfleet-operator) | ||
| rc_account_id = args.rc_account_id | ||
| if not rc_account_id: | ||
| log("Resolving RC account ID via sts:GetCallerIdentity...") | ||
| rc_account_id = session.client("sts").get_caller_identity()["Account"] | ||
| sns_topic_arn = ( | ||
| f"arn:aws:sns:{region}:{rc_account_id}:{cluster_name}-specs-notifications" | ||
| ) | ||
| log(f"SNS topic ARN : {sns_topic_arn}") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The STS fallback resolves the MC account, not the RC account.
sts:GetCallerIdentity returns the account of the current credentials. The same credentials drive DynamoDB against the MC tables. If the RC and MC live in different accounts, rc_account_id gets the MC account ID and the constructed sns_topic_arn points at a topic that does not exist. The publish then fails with NotFound or an authorization error, after the DynamoDB write already succeeded.
The docstring at lines 15-16 documents this default as the RC account ID. Either require --rc-account-id, or read the RC account from the ephemeral env record.
🐛 Proposed fix to require an explicit RC account ID
rc_account_id = args.rc_account_id
if not rc_account_id:
- log("Resolving RC account ID via sts:GetCallerIdentity...")
- rc_account_id = session.client("sts").get_caller_identity()["Account"]
+ rc_account_id = env.get("RC_ACCOUNT_ID")
+ if not rc_account_id:
+ die(
+ "RC account ID not resolved. Pass --rc-account-id, or add "
+ "RC_ACCOUNT_ID to the .ephemeral-envs record."
+ )📝 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.
| # -- resolve RC account ID for SNS topic ARN -- | |
| # Topic ARN: arn:aws:sns:<region>:<rc_account_id>:<cluster_name>-specs-notifications | |
| # (matches snspublisher.Publisher.TopicARN() in hyperfleet-operator) | |
| rc_account_id = args.rc_account_id | |
| if not rc_account_id: | |
| log("Resolving RC account ID via sts:GetCallerIdentity...") | |
| rc_account_id = session.client("sts").get_caller_identity()["Account"] | |
| sns_topic_arn = ( | |
| f"arn:aws:sns:{region}:{rc_account_id}:{cluster_name}-specs-notifications" | |
| ) | |
| log(f"SNS topic ARN : {sns_topic_arn}") | |
| # -- resolve RC account ID for SNS topic ARN -- | |
| # Topic ARN: arn:aws:sns:<region>:<rc_account_id>:<cluster_name>-specs-notifications | |
| # (matches snspublisher.Publisher.TopicARN() in hyperfleet-operator) | |
| rc_account_id = args.rc_account_id | |
| if not rc_account_id: | |
| rc_account_id = env.get("RC_ACCOUNT_ID") | |
| if not rc_account_id: | |
| die( | |
| "RC account ID not resolved. Pass --rc-account-id, or add " | |
| "RC_ACCOUNT_ID to the .ephemeral-envs record." | |
| ) | |
| sns_topic_arn = ( | |
| f"arn:aws:sns:{region}:{rc_account_id}:{cluster_name}-specs-notifications" | |
| ) | |
| log(f"SNS topic ARN : {sns_topic_arn}") |
🤖 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/desire_timing_test_sns.py` around lines 424 - 434, Remove the STS
fallback in the SNS ARN setup around rc_account_id and require an explicit
--rc-account-id value, validating it before constructing sns_topic_arn. Preserve
the documented RC account ID behavior and avoid using
session.client("sts").get_caller_identity(), which identifies the MC account.
| def poll_apply_status( | ||
| ddb, | ||
| table: str, | ||
| doc_id: str, | ||
| expect_type: str, | ||
| timeout: int, | ||
| poll_interval: float, | ||
| start: float, | ||
| ) -> float | None: | ||
| """ | ||
| Poll status-applydesires until Successful=True for the given doc_id. | ||
| Returns elapsed_ms or None on timeout. | ||
| """ | ||
| deadline = time.monotonic() + timeout | ||
| while time.monotonic() < deadline: | ||
| try: | ||
| resp = ddb.get_item( | ||
| TableName=table, | ||
| Key={"documentID": {"S": doc_id}}, | ||
| ConsistentRead=True, | ||
| ) | ||
| except Exception as e: | ||
| warn(f"get_item error: {e}") | ||
| time.sleep(poll_interval) | ||
| continue | ||
|
|
||
| item = resp.get("Item", {}) | ||
| if item: | ||
| conditions = ( | ||
| item.get("status", {}).get("M", {}).get("conditions", {}).get("L", []) | ||
| ) | ||
| for cond in conditions: | ||
| m = cond.get("M", {}) | ||
| if ( | ||
| m.get("Type", {}).get("S") == "Successful" | ||
| and m.get("Status", {}).get("S") == "True" | ||
| ): | ||
| return (time.monotonic() - start) * 1000 | ||
| time.sleep(poll_interval) | ||
| return None |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
poll_apply_status ignores expect_type in both timing scripts. Both files carry the same copy of poll_apply_status. The function declares expect_type but never reads it, so the delete-phase poll matches the Successful=True condition that the apply phase already wrote for the same documentID. Both scripts then report a delete latency near zero and hide the real delete round-trip time.
scripts/desire_timing_test.py#L271-L310: useexpect_typein the condition match, or gate on the incremented statusversion, so the step 7 call at line 529 waits for the delete confirmation.scripts/desire_timing_test_sns.py#L308-L347: apply the same change so the step 7 call at line 592 waits for the delete confirmation.
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 292-292: Do not catch blind exception: Exception
(BLE001)
📍 Affects 2 files
scripts/desire_timing_test.py#L271-L310(this comment)scripts/desire_timing_test_sns.py#L308-L347
🤖 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/desire_timing_test.py` around lines 271 - 310, Update
poll_apply_status in scripts/desire_timing_test.py#L271-L310 and
scripts/desire_timing_test_sns.py#L308-L347 to use expect_type when matching the
Successful=True condition, or otherwise gate on the incremented status version.
Ensure the step 7 delete-phase calls wait for the delete confirmation rather
than reusing the prior apply confirmation.
| START_S=$(date +%s) | ||
| START_NS=$(date +%s%N 2>/dev/null || echo "") | ||
|
|
||
| elapsed_ms() { | ||
| local now_ns | ||
| now_ns=$(date +%s%N 2>/dev/null || echo "") | ||
| if [[ -n "$START_NS" && -n "$now_ns" && "${#now_ns}" -gt 10 ]]; then | ||
| echo $(( (now_ns - START_NS) / 1000000 )) | ||
| else | ||
| echo $(( ($(date +%s) - START_S) * 1000 )) | ||
| fi | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
date +%s%N is not portable, and the length guard does not catch the failure.
BSD date (macOS default) does not support %N. It prints the literal character N instead, so date +%s%N returns a value like 1712345678N. The command still exits 0, so the || echo "" fallback never runs.
The guard "${#now_ns}" -gt 10 then passes, because that string is 11 characters long. The script enters the nanosecond branch and evaluates $(( (now_ns - START_NS) / 1000000 )) on a non-numeric value. Bash reports an arithmetic error and set -euo pipefail at line 4 aborts the run.
Validate that the value is numeric instead of checking its length.
🐛 Proposed fix for the portability guard
START_S=$(date +%s)
-START_NS=$(date +%s%N 2>/dev/null || echo "")
+START_NS=$(date +%s%N 2>/dev/null || echo "")
+[[ "$START_NS" =~ ^[0-9]+$ ]] || START_NS=""
elapsed_ms() {
local now_ns
now_ns=$(date +%s%N 2>/dev/null || echo "")
- if [[ -n "$START_NS" && -n "$now_ns" && "${`#now_ns`}" -gt 10 ]]; then
+ if [[ -n "$START_NS" && "$now_ns" =~ ^[0-9]+$ ]]; then
echo $(( (now_ns - START_NS) / 1000000 ))
else
echo $(( ($(date +%s) - START_S) * 1000 ))
fi
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| START_S=$(date +%s) | |
| START_NS=$(date +%s%N 2>/dev/null || echo "") | |
| elapsed_ms() { | |
| local now_ns | |
| now_ns=$(date +%s%N 2>/dev/null || echo "") | |
| if [[ -n "$START_NS" && -n "$now_ns" && "${#now_ns}" -gt 10 ]]; then | |
| echo $(( (now_ns - START_NS) / 1000000 )) | |
| else | |
| echo $(( ($(date +%s) - START_S) * 1000 )) | |
| fi | |
| } | |
| START_S=$(date +%s) | |
| START_NS=$(date +%s%N 2>/dev/null || echo "") | |
| [[ "$START_NS" =~ ^[0-9]+$ ]] || START_NS="" | |
| elapsed_ms() { | |
| local now_ns | |
| now_ns=$(date +%s%N 2>/dev/null || echo "") | |
| if [[ -n "$START_NS" && "$now_ns" =~ ^[0-9]+$ ]]; then | |
| echo $(( (now_ns - START_NS) / 1000000 )) | |
| else | |
| echo $(( ($(date +%s) - START_S) * 1000 )) | |
| fi | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/manifest-timing-test.sh` around lines 67 - 78, Update the timestamp
validation in elapsed_ms to verify that START_NS and now_ns contain numeric
values before using the nanosecond arithmetic branch, rather than relying on
string length. Ensure unsupported date +%s%N output such as a trailing literal N
falls back to the existing seconds-based calculation.
Source: Path instructions
| resource "aws_iam_role_policy" "kube_applier_messaging" { | ||
| name = "${var.mc_name}-kube-applier-messaging" | ||
| role = "${var.mc_name}-kube-applier" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the kube-applier IAM role, its module call, and explicit dependencies.
rg -n -P -C3 \
'resource\s+"aws_iam_role"\s+"[^"]+"|module\s+"kube_applier[^"]*"|kube_applier_mc_messaging|depends_on\s*=' \
terraformRepository: openshift-online/rosa-hyperfleet
Length of output: 43824
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== management-cluster kube-applier module block =="
sed -n '175,215p' terraform/config/management-cluster/main.tf
echo
echo "== kube-applier-mc-messaging relevant policy and provider resources =="
sed -n '45,60p' terraform/modules/kube-applier-mc-messaging/main.tf
echo
echo "== kube-applier module role and any explicit outputs =="
sed -n '1,40p' terraform/modules/kube-applier/iam.tf
rg -n -C2 '"kube_applier"|aws_iam_role\.kube_applier|outputs|role_arn\.' terraform/modules/kube-applier/ terraform/config/management-cluster/main.tf
echo
echo "== all references to kube-applier-mc-messaging module =="
rg -n -C2 'module\s+"kube_applier_mc_messaging"|depends_on\s*=\s*\[' terraform/config/management-cluster/main.tfRepository: openshift-online/rosa-hyperfleet
Length of output: 7767
Order kube-applier creation before the messaging policy.
module "kube_applier_mc_messaging" in terraform/config/management-cluster/main.tf uses a literal role name, so it does not depend on module "kube_applier". The role is created by module "kube_applier"; add depends_on = [module.kube_applier] to the messaging module call if kube_applier can be created in the same apply.
🤖 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/kube-applier-mc-messaging/main.tf` around lines 52 - 54,
Add an explicit depends_on = [module.kube_applier] to the module
"kube_applier_mc_messaging" call in the management-cluster configuration so the
kube-applier role is created before its messaging policy when both modules are
applied together.
| { | ||
| Sid = "SpecsQueueKMSDecrypt" | ||
| Effect = "Allow" | ||
| Action = [ | ||
| "kms:Decrypt", | ||
| "kms:GenerateDataKey*", | ||
| ] | ||
| Resource = local.rc_kms_key_arn_pattern |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
According to AWS documentation, which KMS actions must an IAM principal have to consume messages from an Amazon SQS queue encrypted with SSE-KMS?
💡 Result:
To consume messages from an Amazon SQS queue encrypted with SSE-KMS, an IAM principal must have the kms:Decrypt permission for the AWS KMS key used to encrypt the queue [1][2][3]. According to AWS documentation, when a consumer calls ReceiveMessage, the service uses this permission to decrypt the data key that protects the messages [2]. Additionally, if the queue is configured as a dead-letter queue, the consumer also requires the kms:Decrypt permission for the KMS key used to encrypt messages in the source queue [2]. These permissions can be granted via the KMS key policy or the IAM policy assigned to the consumer [2][3]. If you are using a customer managed key, ensure the key policy explicitly allows the IAM principal to perform this action [1][2]. If the queue uses an AWS managed KMS key, the key policy automatically includes permissions for authorized principals within the account, though cross-account access is not supported for AWS managed keys [2][4].
Citations:
- 1: https://docs.aws.amazon.com/AWSSimpleQueueService/latest/SQSDeveloperGuide/troubleshooting-access-denied.html
- 2: https://docs.aws.amazon.com/AWSSimpleQueueService/latest/SQSDeveloperGuide/sqs-key-management.html
- 3: https://repost.aws/knowledge-center/sqs-accessdenied-errors
- 4: https://docs.aws.amazon.com/AWSSimpleQueueService/latest/SQSDeveloperGuide/sqs-server-side-encryption.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files matching kube-applier-mc-messaging:"
fd -a 'main.tf$' . | grep -E 'kube-applier-mc-messaging' || true
echo
echo "Relevant target file:"
wc -l terraform/modules/kube-applier-mc-messaging/main.tf 2>/dev/null || true
sed -n '1,140p' terraform/modules/kube-applier-mc-messaging/main.tf 2>/dev/null || true
echo
echo "Search RC KMRS key references in target module/files:"
rg -n "rc_kms_key|kms_key|Messaging|SQS|GenerateDataKey|Decrypt|KmsKey" terraform/modules/kube-applier-mc-messaging terraform/modules/kube-applier-mc-messaging* -S 2>/dev/null || trueRepository: openshift-online/rosa-hyperfleet
Length of output: 6675
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Find kube-applier-rc-messaging module files:"
fd -a '(.tf|variables.tf|outputs.tf)$' terraform/modules | grep -E 'kube-applier-rc-messaging' || true
echo
echo "Relevant references to messaging KMS/SQS across repository:"
rg -n "kube-applier-messaging|messaging|specs-notifications|kms:Decrypt|GenerateDataKey|aws_kms_key|aws_sqs_queue" terraform/modules/kube-applier-rc-messaging terraform/modules/kube-applier-mc-messaging terraform -S 2>/dev/null | head -n 200 || trueRepository: openshift-online/rosa-hyperfleet
Length of output: 20148
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "kube-applier-rc-messaging main outline/sections:"
wc -l terraform/modules/kube-applier-rc-messaging/main.tf
sed -n '1,280p' terraform/modules/kube-applier-rc-messaging/main.tf
echo
echo "kube-applier-rc-messaging variables/outputs:"
sed -n '1,90p' terraform/modules/kube-applier-rc-messaging/variables.tf
sed -n '1,80p' terraform/modules/kube-applier-rc-messaging/outputs.tf
echo
echo "management-cluster kube_applier_mc_messaging call:"
sed -n '198,235p' terraform/config/management-cluster/main.tfRepository: openshift-online/rosa-hyperfleet
Length of output: 14601
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
import json
target = Path("terraform/modules/kube-applier-mc-messaging/main.tf")
producer = Path("terraform/modules/kube-applier-rc-messaging/main.tf")
target_vars = Path("terraform/modules/kube-applier-mc-messaging/variables.tf")
producer_output = Path("terraform/modules/kube-applier-rc-messaging/outputs.tf")
target_src = target.read_text()
producer_src = producer.read_text()
patt = re.compile(r'"(arn:[^"]*):kms:[^"]*:[^"]*:key/([^"]*)"')
matches = list(patt.finditer(target_src))
print("current target kms identity-policy key/resources")
for m in matches:
print(f"{target}:{target_src.count(chr(10), 0, m.start()) + 1}: key/...={m.group(1)}/{m.group(2)}")
print("\nidentity-policy has key/* pattern:", bool(re.search(r'key/\*', target_src)))
print("identity-policy contains kms_key_arn variable ref:", "kms_key_arn" in target_src)
print("target variable declarations:", [line.strip() for line in target_vars.read_text().splitlines() if line.startswith("variable " or "description")])
print("producer output contains kms_key_arn:", 'output "kms_key_arn"' in producer_output.read_text())
print("target call passes kms_key_arn:", bool(re.search(r'kms_key_arn\s*=', Path("terraform/config/management-cluster/main.tf").read_text() or "")))
# Deterministic parser: extract the block containing rc_kms_key_arn_pattern by string matching,
# not executing Terraform.
start = target_src.index("rc_kms_key_arn_pattern")
block = target_src[start:start+120]
print("\nrc_kms_key_arn_pattern value:")
for line in block.splitlines():
print(line)
PYRepository: openshift-online/rosa-hyperfleet
Length of output: 795
Scope kube-applier KMS permissions to the messaging key ARN.
rc_kms_key_arn_pattern currently allows kms:Decrypt and kms:GenerateDataKey* on every RC key in the region. Wire module.kube_applier_rc_messaging.kms_key_arn into kube-applier-mc-messaging and set resource to that ARN; keep kms:GenerateDataKey* only if the queue consumer needs it. A KMS key policy is additional protection, but this role should not be granted broader KMS access than required.
🤖 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/kube-applier-mc-messaging/main.tf` around lines 69 - 76,
Update the kube-applier messaging module wiring to accept and pass
module.kube_applier_rc_messaging.kms_key_arn, then change the
SpecsQueueKMSDecrypt statement’s Resource from local.rc_kms_key_arn_pattern to
that specific ARN. Retain kms:GenerateDataKey* only if the queue consumer
requires it; otherwise remove the action.
Sources: Coding guidelines, Path instructions
327c0f2 to
15bc25e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@terraform/modules/kube-applier-rc-messaging/main.tf`:
- Around line 328-335: Add depends_on = [aws_sqs_queue_policy.status] to both
status subscriptions at terraform/modules/kube-applier-rc-messaging/main.tf
lines 328-335 and 337-344. Add each status pipe’s dependency on its
corresponding subscription at lines 603-634 and 636-667, preserving
replica-to-subscription alignment so every pipe starts only after its
subscription and queue policy exist.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2d015d3e-8bea-4c68-9158-ae9e2b17530b
📒 Files selected for processing (7)
scripts/bootstrap-argocd.shterraform/modules/ecs-bootstrap/variables.tfterraform/modules/kube-applier-mc-messaging/main.tfterraform/modules/kube-applier-mc-messaging/versions.tfterraform/modules/kube-applier-rc-messaging/main.tfterraform/modules/kube-applier-rc-messaging/outputs.tfterraform/modules/kube-applier-rc-messaging/versions.tf
💤 Files with no reviewable changes (1)
- terraform/modules/kube-applier-mc-messaging/main.tf
🚧 Files skipped from review as they are similar to previous changes (3)
- terraform/modules/ecs-bootstrap/variables.tf
- terraform/modules/kube-applier-rc-messaging/versions.tf
- terraform/modules/kube-applier-mc-messaging/versions.tf
| resource "aws_sns_topic_subscription" "status_applydesires" { | ||
| count = var.operator_replica_count | ||
| topic_arn = aws_sns_topic.status_applydesires.arn | ||
| protocol = "sqs" | ||
| endpoint = aws_sqs_queue.status[count.index].arn | ||
|
|
||
| raw_message_delivery = true | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
make terraform-fmt
terraform -chdir=terraform/config/kube-applier-dynamodb-provisioning validate
terraform -chdir=terraform/config/kube-applier-dynamodb-provisioning plan
make pre-pushRepository: openshift-online/rosa-hyperfleet
Length of output: 212
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- target resources ---'
sed -n '280,365p' terraform/modules/kube-applier-rc-messaging/main.tf
sed -n '575,685p' terraform/modules/kube-applier-rc-messaging/main.tf
printf '%s\n' '--- related queue policy and topics ---'
rg -n -C 8 'aws_sqs_queue_policy|aws_sns_topic|aws_sqs_queue|aws_pipes_pipe|aws_sns_topic_subscription' terraform/modules/kube-applier-rc-messaging/main.tf
printf '%s\n' '--- module references and input shape ---'
rg -n -C 4 'kube-applier-rc-messaging|operator_replica_count|status_applydesires|status_readdesires' terraformRepository: openshift-online/rosa-hyperfleet
Length of output: 46554
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- status pipe IAM resources ---'
sed -n '500,595p' terraform/modules/kube-applier-rc-messaging/main.tf
printf '%s\n' '--- pipe lifecycle configuration in repository ---'
rg -n -C 5 'desired_state|aws_pipes_pipe|aws_sns_topic_policy|aws_iam_role_policy.*status|status_pipe' terraform/modules/kube-applier-rc-messaging terraform/config
printf '%s\n' '--- provider and lock versions ---'
rg -n -C 3 'required_providers|hashicorp/aws|aws_pipes_pipe' terraform -g '*.tf' -g '*.lock.hcl' -g '*.md'
printf '%s\n' '--- local Terraform availability ---'
command -v terraform || true
terraform version 2>/dev/null || trueRepository: openshift-online/rosa-hyperfleet
Length of output: 44186
🌐 Web query:
AWS EventBridge Pipes aws_pipes_pipe desired_state default RUNNING Terraform provider documentation SNS SQS subscription queue policy creation order
💡 Result:
In the AWS provider for Terraform, the aws_pipes_pipe resource supports the desired_state argument, which accepts either RUNNING or STOPPED [1]. If not explicitly defined, the default behavior of a pipe upon creation is to be in an active (running) state [2]. Regarding the creation order for EventBridge Pipes, SNS, and SQS, Terraform automatically manages dependencies based on resource references [3]. By passing the Amazon Resource Name (ARN) of your source or target (such as an aws_sqs_queue or aws_sns_topic) into the aws_pipes_pipe configuration, Terraform understands that the infrastructure must be created in the correct sequence (e.g., the queue or topic must exist before the pipe can be successfully provisioned) [3]. If you are specifically using SNS to push messages to an SQS queue, you must create the SNS topic, the SQS queue, and the aws_sns_topic_subscription [4]. To ensure successful delivery, the SQS queue policy must grant permission for the SNS topic to send messages (sns:Publish) to it [5]. Key implementation considerations: 1. Desired State: Use desired_state = "RUNNING" in your aws_pipes_pipe resource if you want to ensure the pipe is active immediately upon creation [1]. 2. Implicit Dependencies: You do not need to manually specify depends_on in most cases if you reference the attributes of your SQS or SNS resources (e.g., source = aws_sqs_queue.example.arn) directly within the pipe resource [3]. 3. Policy Ordering: Ensure that the SQS Queue Policy and SNS Topic Policy are defined to allow the necessary interactions before the pipe or subscription attempts to operate [4][5]. Terraform will handle the creation order of these policies relative to the resources they attach to, provided the resources are referenced correctly in the policy document [4].
Citations:
- 1: https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/pipes_pipe
- 2: https://docs.aws.amazon.com/eventbridge/latest/userguide/eb-pipes-create.html
- 3: https://oneuptime.com/blog/post/2026-02-23-how-to-create-eventbridge-pipes-with-terraform/view
- 4: https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/sns_topic_subscription
- 5: https://registry.terraform.io/modules/terraform-aws-modules/sns/aws/latest
🌐 Web query:
site:docs.aws.amazon.com/sns/latest/dg/subscribe-sqs-queue-to-sns-topic.html SQS queue policy before subscribe SNS subscription
💡 Result:
To allow an Amazon SNS topic to send messages to an Amazon SQS queue, you must configure a queue policy that grants the SNS service permission to perform the sqs:SendMessage action [1]. It is considered a best practice to set this policy on the SQS queue before creating the SNS subscription, although the subscription process may still succeed in the AWS console depending on your permissions; however, the queue will not receive messages until the policy is in place [1]. The required queue policy must explicitly allow the sns.amazonaws.com service principal to perform sqs:SendMessage on the queue resource [1]. To prevent a confused deputy security vulnerability, you should use a condition key to restrict access to the specific SNS topic [1]. Example Policy: { "Statement": [ { "Effect": "Allow", "Principal": { "Service": "sns.amazonaws.com" }, "Action": "sqs:SendMessage", "Resource": "arn:aws:sqs:region:account-id:queue-name", "Condition": { "ArnEquals": { "aws:SourceArn": "arn:aws:sns:region:account-id:topic-name" } } } ] } If the queue is subscribed to multiple topics, you can use the aws:SourceAccount condition key instead of aws:SourceArn to manage permissions more broadly [1].
Citations:
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
text = Path("terraform/modules/kube-applier-rc-messaging/main.tf").read_text()
resources = {
"subscription.apply": r'resource\s+"aws_sns_topic_subscription"\s+"status_applydesires"\s*\{(.*?)\n\}',
"subscription.read": r'resource\s+"aws_sns_topic_subscription"\s+"status_readdesires"\s*\{(.*?)\n\}',
"pipe.apply": r'resource\s+"aws_pipes_pipe"\s+"status_applydesires"\s*\{(.*?)\n\}',
"pipe.read": r'resource\s+"aws_pipes_pipe"\s+"status_readdesires"\s*\{(.*?)\n\}',
}
for name, pattern in resources.items():
match = re.search(pattern, text, re.S)
if not match:
raise SystemExit(f"missing resource: {name}")
body = match.group(1)
refs = sorted(set(re.findall(r'aws_[A-Za-z0-9_]+\.[A-Za-z0-9_]+', body)))
depends = re.findall(r'(?m)^\s*depends_on\s*=\s*(.+)$', body)
print(f"{name}:")
print(f" references: {', '.join(refs) or '(none)'}")
print(f" depends_on: {', '.join(depends) or '(none)'}")
print("policy resources:")
for resource in (
"aws_sqs_queue_policy.status",
"aws_sns_topic_policy.status_applydesires",
"aws_sns_topic_policy.status_readdesires",
"aws_iam_role_policy.status_pipe",
):
print(f" {resource}: {'present' if re.search(r'resource\\s+\"' + re.escape(resource.split('.')[0]) + r'\"\\s+\"' + re.escape(resource.split('.')[1]) + r'\"', text) else 'missing'}")
PYRepository: openshift-online/rosa-hyperfleet
Length of output: 635
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
text = Path("terraform/modules/kube-applier-rc-messaging/main.tf").read_text()
resources = {
"subscription.apply": r'resource\s+"aws_sns_topic_subscription"\s+"status_applydesires"\s*\{(.*?)\n\}',
"subscription.read": r'resource\s+"aws_sns_topic_subscription"\s+"status_readdesires"\s*\{(.*?)\n\}',
"pipe.apply": r'resource\s+"aws_pipes_pipe"\s+"status_applydesires"\s*\{(.*?)\n\}',
"pipe.read": r'resource\s+"aws_pipes_pipe"\s+"status_readdesires"\s*\{(.*?)\n\}',
}
for name, pattern in resources.items():
match = re.search(pattern, text, re.S)
if not match:
raise SystemExit(f"missing resource: {name}")
body = match.group(1)
refs = sorted(set(re.findall(r'aws_[A-Za-z0-9_]+\.[A-Za-z0-9_]+', body)))
depends = re.findall(r'(?m)^\s*depends_on\s*=\s*(.+)$', body)
print(f"{name}:")
print(f" references: {', '.join(refs) or '(none)'}")
print(f" depends_on: {', '.join(depends) or '(none)'}")
print("policy resources:")
for resource in (
"aws_sqs_queue_policy.status",
"aws_sns_topic_policy.status_applydesires",
"aws_sns_topic_policy.status_readdesires",
"aws_iam_role_policy.status_pipe",
):
kind, name = resource.split(".")
pattern = rf'resource\s+"{re.escape(kind)}"\s+"{re.escape(name)}"'
present = bool(re.search(pattern, text))
print(f" {resource}: {'present' if present else 'missing'}")
PYRepository: openshift-online/rosa-hyperfleet
Length of output: 805
Create the status subscriptions before enabling the status pipes.
The status pipes start in RUNNING state and have no dependency on their subscriptions. With TRIM_HORIZON, a pipe can publish while a replica subscription or its SQS queue policy is absent. SNS does not deliver messages to subscriptions that do not yet exist.
Add depends_on = [aws_sqs_queue_policy.status] to both status subscriptions. Add a dependency from each status pipe to its corresponding subscription. This creates the required ordering for all replica queues.
📍 Affects 1 file
terraform/modules/kube-applier-rc-messaging/main.tf#L328-L335(this comment)terraform/modules/kube-applier-rc-messaging/main.tf#L337-L344terraform/modules/kube-applier-rc-messaging/main.tf#L603-L634terraform/modules/kube-applier-rc-messaging/main.tf#L636-L667
🤖 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/kube-applier-rc-messaging/main.tf` around lines 328 - 335,
Add depends_on = [aws_sqs_queue_policy.status] to both status subscriptions at
terraform/modules/kube-applier-rc-messaging/main.tf lines 328-335 and 337-344.
Add each status pipe’s dependency on its corresponding subscription at lines
603-634 and 636-667, preserving replica-to-subscription alignment so every pipe
starts only after its subscription and queue policy exist.
Source: Coding guidelines
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@terraform/modules/kube-applier-rc-messaging/main.tf`:
- Around line 186-192: Move the aws_sns_topic_subscription.specs resource from
the RC messaging module to the MC messaging module. Configure it with
local.rc_specs_sns_topic_arn as topic_arn, aws_sqs_queue.specs.arn as endpoint,
retain SQS protocol and raw_message_delivery, and add depends_on =
[aws_sqs_queue_policy.specs].
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 26a453db-3c78-4c8e-9cc4-6a420225e504
📒 Files selected for processing (8)
scripts/desire_timing_test.pyterraform/config/kube-applier-dynamodb-provisioning/outputs.tfterraform/config/management-cluster/main.tfterraform/config/management-cluster/outputs.tfterraform/modules/kube-applier-mc-messaging/main.tfterraform/modules/kube-applier-mc-messaging/outputs.tfterraform/modules/kube-applier-rc-messaging/main.tfterraform/modules/kube-applier-rc-messaging/outputs.tf
🚧 Files skipped from review as they are similar to previous changes (3)
- terraform/config/management-cluster/outputs.tf
- terraform/config/management-cluster/main.tf
- scripts/desire_timing_test.py
psav
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@rrp-bot: This pull request references ROSAENG-62444 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/buildspec/register.sh`:
- Around line 166-167: Update the RC specs topic policy and registration flow
around AllowMCAccountSubscribe in register.sh to target the dedicated MC
registration role instead of the MC account root. Ensure the role is granted
only sns:Subscribe on this topic, and change the subscription setup to assume
and use that same registration role rather than the broad
OrganizationAccountAccessRole.
- Around line 169-171: Update the teardown flow in register.sh so MC queue
destruction cannot occur before the RC SNS topic or its subscription is removed.
When DELETE_FLAG=true, either enforce an RC-topic-first dependency or explicitly
unsubscribe the MC queue from the SNS topic before destroying the queue, while
preserving the existing cleanup behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6b5cbeee-29f7-43ea-8ea7-b96afc329dc5
📒 Files selected for processing (9)
scripts/buildspec/register.shscripts/desire_timing_test.pyterraform/config/kube-applier-dynamodb-provisioning/outputs.tfterraform/config/management-cluster/main.tfterraform/config/management-cluster/outputs.tfterraform/modules/kube-applier-mc-messaging/main.tfterraform/modules/kube-applier-mc-messaging/outputs.tfterraform/modules/kube-applier-rc-messaging/main.tfterraform/modules/kube-applier-rc-messaging/outputs.tf
🚧 Files skipped from review as they are similar to previous changes (8)
- terraform/config/kube-applier-dynamodb-provisioning/outputs.tf
- terraform/modules/kube-applier-mc-messaging/outputs.tf
- terraform/modules/kube-applier-mc-messaging/main.tf
- terraform/config/management-cluster/main.tf
- terraform/modules/kube-applier-rc-messaging/outputs.tf
- terraform/config/management-cluster/outputs.tf
- terraform/modules/kube-applier-rc-messaging/main.tf
- scripts/desire_timing_test.py
| # The RC specs topic policy (AllowMCAccountSubscribe) grants sns:Subscribe to | ||
| # the MC account root, permitting this cross-account call. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 8 \
'AllowMCAccountSubscribe|sns:Subscribe|use_mc_account|mc-\$\{CLUSTER_ID' \
terraform/modules/kube-applier-rc-messaging \
terraform/modules/kube-applier-mc-messaging \
scripts/pipeline-common/lib.shRepository: openshift-online/rosa-hyperfleet
Length of output: 4729
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- candidate files ---'
git ls-files \
'terraform/modules/kube-applier-rc-messaging/*' \
'terraform/modules/kube-applier-mc-messaging/*' \
'scripts/pipeline-common/lib.sh' \
'scripts/buildspec/register.sh'
printf '%s\n' '--- relevant declarations and references ---'
rg -n -C 12 \
'specs_pipe|AllowMCAccountSubscribe|sns:Subscribe|aws_iam_role|aws_iam_policy|OrganizationAccountAccessRole|_assume_account|TARGET_ACCOUNT_ID|mc_aws_account_id' \
terraform/modules/kube-applier-rc-messaging \
terraform/modules/kube-applier-mc-messaging \
scripts/pipeline-common/lib.sh \
scripts/buildspec/register.shRepository: openshift-online/rosa-hyperfleet
Length of output: 46850
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- MC messaging module ---'
cat -n terraform/modules/kube-applier-mc-messaging/main.tf
printf '%s\n' '--- account assumption implementation ---'
sed -n '107,145p' scripts/pipeline-common/lib.sh
printf '%s\n' '--- all SNS subscribe identity-policy entries ---'
rg -n -C 10 \
'"sns:Subscribe"|sns:Subscribe|aws_iam_role_policy' \
terraform scriptsRepository: openshift-online/rosa-hyperfleet
Length of output: 50389
Restrict AllowMCAccountSubscribe to a dedicated MC registration role. The current account principal permits any authorized identity in the MC account to subscribe. The MC module defines no sns:Subscribe identity policy, and register.sh assumes the broad OrganizationAccountAccessRole. Grant sns:Subscribe only on this topic to the registration role and use that role for the subscription.
🤖 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 166 - 167, Update the RC specs
topic policy and registration flow around AllowMCAccountSubscribe in register.sh
to target the dedicated MC registration role instead of the MC account root.
Ensure the role is granted only sns:Subscribe on this topic, and change the
subscription setup to assume and use that same registration role rather than the
broad OrganizationAccountAccessRole.
Source: Path instructions
| # AWS automatically removes subscriptions when their SNS topic is deleted, so | ||
| # no explicit teardown is needed — Terraform destroying a topic cleans up its | ||
| # subscriptions. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 8 \
'DELETE_FLAG|unsubscribe|delete-queue|aws_sns_topic|aws_sqs_queue' \
scripts/buildspec \
terraform/modules/kube-applier-rc-messaging \
terraform/modules/kube-applier-mc-messaging \
terraform/configRepository: openshift-online/rosa-hyperfleet
Length of output: 49656
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- register.sh ---'
cat -n scripts/buildspec/register.sh | sed -n '1,210p'
printf '%s\n' '--- pipeline references and ordering ---'
rg -n -C 6 \
'register\.sh|provision-infra-rc\.sh|provision-infra-mc\.sh|kube-applier-rc-messaging|kube-applier-mc-messaging|DELETE_FLAG|IS_DESTROY' \
buildspec.yml scripts .github deploy terraform/config 2>/dev/null
printf '%s\n' '--- messaging module calls and dependencies ---'
rg -n -C 8 \
'module .*messaging|kube-applier-(rc|mc)-messaging|depends_on|register' \
terraform/config terraform/modules 2>/dev/nullRepository: openshift-online/rosa-hyperfleet
Length of output: 37489
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- pipeline destroy function and invocation order ---'
cat -n scripts/provision-pipelines.sh | sed -n '95,175p'
cat -n scripts/provision-pipelines.sh | sed -n '270,385p'
printf '%s\n' '--- regional and management CodePipeline stage order ---'
cat -n terraform/config/pipeline-regional-cluster/main.tf | sed -n '370,445p'
cat -n terraform/config/pipeline-management-cluster/main.tf | sed -n '570,690p'
printf '%s\n' '--- infrastructure destroy commands ---'
rg -n -C 12 \
'TERRAFORM_ACTION|terraform (apply|destroy)|kube-applier-dynamodb-provisioning|management-cluster' \
scripts/buildspec/provision-infra-rc.sh \
scripts/buildspec/provision-infra-mc.shRepository: openshift-online/rosa-hyperfleet
Length of output: 26066
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- destroy pipeline execution ---'
rg -n -C 12 \
'IS_DESTROY|FORCE_DELETE_ALL_PIPELINES|start-pipeline-execution|StartPipeline|provision-pipelines\.sh|pipeline.*destroy|destroy.*pipeline' \
ci scripts .github Makefile terraform 2>/dev/null
printf '%s\n' '--- CodePipeline execution configuration ---'
rg -n -C 10 \
'variables|IS_DESTROY|pipeline.*name|start_pipeline|aws codepipeline' \
terraform/config/pipeline-regional-cluster/main.tf \
terraform/config/pipeline-management-cluster/main.tf \
scripts 2>/dev/null
printf '%s\n' '--- management infrastructure module dependencies ---'
cat -n terraform/config/management-cluster/main.tf | sed -n '180,225p'
cat -n terraform/config/kube-applier-dynamodb-provisioning/main.tf | sed -n '95,145p'Repository: openshift-online/rosa-hyperfleet
Length of output: 50389
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
path = Path("ci/ephemeral-provider/orchestrator.py")
text = path.read_text()
required = [
'git.modify_config(TARGET_ENVIRONMENT, self.region, set_delete_flag)',
'with ThreadPoolExecutor(max_workers=len(teardown_pipelines)) as executor:',
'executor.submit(self.target_monitor.wait_for_completion, name, exec_id)',
]
missing = [item for item in required if item not in text]
if missing:
raise SystemExit(f"missing expected teardown markers: {missing}")
start = text.index("def _run_teardown")
end = text.index("def _destroy_pipeline_provisioner", start)
section = text[start:end]
print("infrastructure delete flags are set before pipeline monitoring:", section.index("git.modify_config") < section.index("ThreadPoolExecutor"))
print("RC and MC teardown pipelines are monitored concurrently:", "ThreadPoolExecutor" in section and "executor.submit" in section)
print("register.sh skips teardown before the subscription command:", end=" ")
register = Path("scripts/buildspec/register.sh").read_text()
print(register.index('exit 0') < register.index('aws sns subscribe'))
PYRepository: openshift-online/rosa-hyperfleet
Length of output: 366
Serialize SNS topic and SQS queue teardown.
RC and MC infrastructure destruction runs concurrently. register.sh skips unsubscribe when DELETE_FLAG=true. If MC queue destruction finishes first, the subscription remains while the RC topic exists. Enforce RC topic deletion first or unsubscribe from the MC side before destroying the queue.
🤖 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 169 - 171, Update the teardown
flow in register.sh so MC queue destruction cannot occur before the RC SNS topic
or its subscription is removed. When DELETE_FLAG=true, either enforce an
RC-topic-first dependency or explicitly unsubscribe the MC queue from the SNS
topic before destroying the queue, while preserving the existing cleanup
behavior.
Source: Path instructions
…tatus notification paths Replace the DynamoDB Streams-based cross-account messaging infrastructure with EventBridge Pipes reading directly from DynamoDB Streams and delivering to SQS queues. Previously: - Specs path: a cross-account subscription wired at deploy time delivered desire-write events from the RC account to an MC SQS queue. - Status path: kube-applier in the MC account wrote status updates that were delivered to per-replica SQS queues in the RC account via a separate pipe. - Required register.sh to wire cross-account subscriptions at deploy time, and STS calls to resolve account IDs at runtime. Now: - Specs path: EventBridge Pipes reads INSERT/MODIFY events directly from the RC specs DynamoDB Streams and delivers to an SQS queue in the RC account. kube-applier polls this queue cross-account using its existing Pod Identity role. Two pipes per MC (one per specs table: applydesires, readdesires). - Status path: EventBridge Pipes reads INSERT/MODIFY events from the RC status DynamoDB Streams and delivers to per-replica SQS queues in the RC account. 2×N pipes (2 tables × N operator replicas). hyperfleet-operator polls its own queue by index. - No cross-account subscriptions, no register.sh wiring. Terraform changes (kube-applier-rc-messaging module): - Replace cross-account subscription resources with aws_pipes_pipe sourcing from DynamoDB stream → RC SQS - All SQS queues (specs + status) now in RC account (EventBridge Pipes cannot target cross-account SQS) - Queue URLs/ARNs are fully deterministic — constructed from known inputs without cross-stack output passing - KMS/SQS policies use account-root + aws:PrincipalArn condition pattern for cross-account access - MC-side IAM (kube-applier-mc-messaging) uses arn:aws:kms:region:rc-account:key/* wildcard for Decrypt (KMS alias ARNs are invalid as Resource in identity-based policies) - Pipes filter: INSERT and MODIFY only (DELETE events dropped before SQS) - Pipes input_template: raw string (not jsonencode) to avoid JSONPath placeholders being double-quoted and passed literally ArgoCD/Helm changes: - Remove register.sh subscription wiring from ApplicationSet - Wire --sqs-queue-url-prefix to hyperfleet-operator for pod self-discovery - Update image tags for kube-applier and hyperfleet-operator Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…nput_template, filter schema Fixes discovered during testing of the EventBridge Pipes infrastructure: - KMS policy: use account-root + aws:PrincipalArn condition for cross-account access instead of per-role ARNs. MC kube-applier role uses key/* wildcard for Decrypt (KMS alias ARNs are invalid as Resource in identity-based policies and cause silent failures). - aws_pipes_pipe schema: filter_criteria is a sibling of dynamodb_stream_parameters inside source_parameters, not nested within it. sqs_queue_parameters block left empty (batching defaults are correct). - Move specs SQS queue from MC account to RC account. EventBridge Pipes cannot target cross-account SQS; all queues must be in the same account as the pipe (RC account). kube-applier polls cross-account instead. - Remove duplicate resource definitions from kube-applier-rc-messaging module left over from the refactor. - All queue URLs/ARNs are now fully deterministic — constructable from known inputs (account ID, region, MC name) without cross-stack output passing. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Update kube-applier image tag to c03cf6d (full-resync post-startup tests) - Update hyperfleet-operator image tag to 258bf36 (EventBridge placeholder guard) - Wire --full-resync-period flag through ArgoCD ApplicationSet values and kube-applier Deployment template; defaults to empty string (uses the 5-minute in-code default) so existing deployments are unaffected - Remove stale comment referencing old notification path from management-cluster main.tf - Fix input_template to use raw string instead of jsonencode() — jsonencode double-quotes the JSONPath placeholders causing EventBridge to pass them through literally rather than substituting the actual field values Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- kube-applier: 67ee1f4 → 65fe596 (test: end-to-end coverage for full-resync post-startup path) - hyperfleet-operator: 7965486 → b00a2bf (fix: detect unresolved EventBridge JSONPath placeholders in status consumer) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ucture - bootstrap-argocd.sh: fail fast if kube_applier_specs_queue_url is empty for management-cluster bootstrap; a missing queue URL would silently produce a kube-applier with no SQS notifications, falling back entirely to the full-resync period - ecs-bootstrap/variables.tf: correct variable description from 'MC-side' to 'RC-side' — the queue lives in the RC account and is consumed cross-account by the MC kube-applier - kube-applier-rc-messaging/main.tf: change all four pipe starting_position from LATEST to TRIM_HORIZON so a pipe restart does not miss events that arrived while the pipe was stopped - kube-applier-rc-messaging/versions.tf: bump AWS provider constraint to >= 5.49.0 (rotation_period_in_days on aws_kms_key requires 5.49) - kube-applier-mc-messaging/versions.tf: same AWS provider bump - kube-applier-mc-messaging/main.tf: remove kms:GenerateDataKey* from the kube-applier SQS consumer IAM policy — consumers only call ReceiveMessage which requires kms:Decrypt; GenerateDataKey* is only needed by producers Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The previous design created N EventBridge Pipes per DynamoDB status stream (one per operator replica), all reading from the same stream. This violates DynamoDB's 2-concurrent-reader-per-shard limit documented at: https://docs.aws.amazon.com/amazondynamodb/latest/developerguide/Streams.html New design: one pipe per status stream targets an SNS topic. SNS fans out to one SQS subscription per operator replica (raw_message_delivery=true to strip the SNS envelope). SQS queue names, ARNs and URLs are unchanged — no consumer code changes required. Changes: - Add aws_sns_topic for status-applydesires and status-readdesires - Add aws_sns_topic_policy allowing the status pipe role to Publish - Add aws_sns_topic_subscription (count=N) per topic with raw delivery - Add aws_sqs_queue_policy (count=N) allowing SNS SendMessage to status queues - Add AllowSNS statement to KMS key policy for SNS re-encryption - Update aws_iam_role_policy.status_pipe: sqs:SendMessage → sns:Publish - Update aws_pipes_pipe.status_*: count=1, target=SNS ARN, sns_topic_parameters - Update outputs.tf: add SNS topic ARN outputs, retain status_queue_urls/arns Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ters The Terraform AWS provider's aws_pipes_pipe resource does not support an sns_topic_parameters block in target_parameters — SNS targets only need the input_template, no nested block required. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The delete poll (Step 7) was immediately matching the stale Successful=True condition written during the create reconciliation (Step 4), returning in ~114ms without waiting for the operator to actually process the delete. Fix mirrors the operator's CheckApplyDesireStatuses two-gate logic: - Gate 1: status.observedDesireUpdateTime >= desire.updateTime (freshness) - Gate 2: conditions[type=Successful].status == True poll_apply_status now accepts after_time (str | None) instead of expect_type. Step 4 passes None (any Successful=True is fine for create). Step 7 passes delete_item[updateTime][S] so only a status written after the delete desire was issued is accepted as confirmation. Also guard Step 7 poll behind the same resp.get(Item) check as Step 6 to avoid a NameError if the spec item was already cleaned up. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Append a 6-char hex suffix to the ConfigMap name so each run creates a distinct resource rather than colliding with a previous run's ConfigMap that may still exist on the management cluster. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace cross-account SQS polling (RC account, CMK required) with an SNS push model: RC EventBridge Pipes now publish to an RC-side SNS topic, which delivers cross-account to an SQS queue in the MC account. kube-applier polls its own same-account queue — no CMK or cross-account SQS permissions needed. Status queues switch from CMK to SSE-SQS since they are same-account only. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… timeout The cross-account SNS→SQS subscription must be created after the MC-side queue policy exists — otherwise SNS cannot deliver the confirmation message and the subscription times out. Moving the subscription to the MC module lets Terraform express the dependency on aws_sqs_queue_policy.specs via depends_on, ensuring the queue policy is in place before SNS attempts confirmation. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ster.sh Terraform's aws_sns_topic_subscription always calls SNS APIs in the topic owner's account (RC), which cannot auto-confirm a subscription to a queue in a different account (MC). Move subscription creation to register.sh, calling aws sns subscribe as the MC account (queue owner) so AWS auto-confirms it immediately. Remove the Terraform aws_sns_topic_subscription resource from the MC module entirely — it caused a 403 on plan because the MC provider cannot call GetSubscriptionAttributes on a subscription owned by the RC account. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
e6c001a to
3e43281
Compare
e6c001a to
3e43281
Compare
|
/ok-to-test |
|
@rrp-bot: Cannot trigger testing until a trusted user reviews the PR and leaves an DetailsIn response to this:
Instructions 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. |
|
/ok-to-test |
openshift-ci
Bot
added
ok-to-test
|
/retest |
|
@rrp-bot: 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. |
3 participants
Summary
Replace the DynamoDB Streams-based cross-account messaging infrastructure with EventBridge Pipes reading directly from DynamoDB Streams and delivering to SQS queues in the RC account.
Why: The previous approach required cross-account subscription wiring at deploy time and STS calls to resolve account IDs at runtime. EventBridge Pipes eliminates both: it reads INSERT/MODIFY events directly from DynamoDB Streams and delivers documentIDs to SQS queues, with kube-applier and hyperfleet-operator polling cross-account using their existing Pod Identity roles.
Commits
feat: replace DynamoDB Streams with EventBridge Pipes for specs and status notification paths— newkube-applier-rc-messagingandkube-applier-mc-messagingTerraform modules, removes cross-account subscription resources, removesregister.shwiring from ArgoCD ApplicationSet, wires--sqs-queue-url-prefixfix: EventBridge Pipes correctness — KMS policy, cross-account SQS, input_template, filter schema— fixesfilter_criteriaplacement inaws_pipes_pipe, moves specs SQS queue to RC account, fixes KMS key wildcard for cross-account Decryptchore: update image tags and wire --full-resync-period flag— updates kube-applier and hyperfleet-operator image tags, wires--full-resync-periodthrough ArgoCD values and Deployment template, fixesinput_templateto use raw stringInfrastructure changes
terraform/modules/kube-applier-rc-messaging: new module — EventBridge Pipes, RC SQS queues (specs + status), KMS keys, IAM policiesterraform/modules/kube-applier-mc-messaging: new module — MC-side IAM for cross-account SQS + KMS accessargocd/config/management-cluster/kube-applier: wires--sqs-queue-url-prefixand--full-resync-periodargocd/config/regional-cluster/hyperfleet: wires--sqs-queue-url-prefixRelated PRs
Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
Summary by CodeRabbit
New Features
Bug Fixes
Documentation