ROSAENG-62444: feat: add SNS/SQS messaging infrastructure for hyperfleet notifications - #710
ROSAENG-62444: feat: add SNS/SQS messaging infrastructure for hyperfleet notifications#710rrp-bot wants to merge 18 commits into
Conversation
|
@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. |
openshift-ci-robot
added
the
jira/valid-reference
|
[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 |
📝 WalkthroughWalkthroughAdds cross-account SNS/SQS messaging for kube-applier. The change provisions encrypted Terraform resources, replica-aware queues, IAM policies, bootstrap propagation, Helm configuration, and post-registration subscriptions. ChangesKube-applier cross-account messaging
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant HyperfleetOperator
participant RegionalSpecsSNSTopic
participant ManagementSpecsSQSQueue
participant KubeApplier
participant ManagementStatusSNSTopic
participant RegionalStatusSQSQueue
HyperfleetOperator->>RegionalSpecsSNSTopic: Publish specs notification
RegionalSpecsSNSTopic->>ManagementSpecsSQSQueue: Deliver subscribed message
KubeApplier->>ManagementStatusSNSTopic: Publish status notification
ManagementStatusSNSTopic->>RegionalStatusSQSQueue: Deliver subscribed message
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 2⚔️ Resolve merge conflicts 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@terraform/config/kube-applier-dynamodb-provisioning/main.tf`:
- Around line 111-122: Break the initial RC/MC messaging bootstrap cycle across
terraform/config/kube-applier-dynamodb-provisioning/main.tf lines 111-122,
scripts/buildspec/provision-infra-mc.sh lines 89-105, and
scripts/buildspec/provision-kube-applier-dynamodb.sh lines 42-74. Update module
kube_applier_rc_messaging so RC resources can be created without requiring both
MC ARNs; coordinate provision-infra-mc.sh to perform staged applies rather than
proceeding with an empty RC topic ARN; and have
provision-kube-applier-dynamodb.sh create a bootstrap-capable MC base first,
provision RC resources, then apply the final restrictive queue policy.
In `@terraform/config/management-cluster/main.tf`:
- Around line 205-214: Add the enable_kube_applier_messaging boolean input and
update the kube_applier_mc_messaging count condition to require both this flag
and a non-empty var.rc_specs_sns_topic_arn. Preserve the module’s existing
configuration while ensuring deployment is explicitly controlled by the feature
flag.
In `@terraform/modules/kube-applier-mc-messaging/main.tf`:
- Around line 64-78: Add a separate KMS key-policy statement alongside
AllowSNSDelivery for the RC SNS topic, granting sns.amazonaws.com Decrypt and
GenerateDataKey* and constraining access with aws:SourceArn equal to
var.rc_specs_sns_topic_arn. Leave the existing MC-account AllowSNSDelivery
statement unchanged and do not broaden its source-account condition.
In `@terraform/modules/kube-applier-rc-messaging/main.tf`:
- Around line 84-88: Update the KMS key policy near AllowSNSDelivery by adding a
separate statement authorizing SNS delivery from var.mc_aws_account_id, scoped
to var.mc_status_sns_topic_arn when supported by the policy conditions. Preserve
the existing RC-account AllowSNSDelivery statement and grant only the
permissions required for the MC status topic to use the RC queue key.
- Around line 176-185: The specs_to_mc_queue aws_sns_topic_subscription must be
created by the MC queue-owner module rather than the RC-side module. Move this
subscription to the MC-side configuration, preserve its topic, SQS endpoint,
protocol, and raw delivery settings, and update the RC topic policy to grant the
MC account sns:Subscribe in addition to the existing publish permission.
In `@terraform/modules/kube-applier-rc-messaging/variables.tf`:
- Around line 40-42: Update the validation for operator_replica_count to require
a whole-number value in addition to the existing 1–10 range, so fractional
values are rejected before count uses the variable. Preserve the current bounds
and error-message intent.
🪄 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: Pro Plus
Run ID: 80df1ce1-f3fe-4128-bb00-13814c1c7742
📒 Files selected for processing (16)
scripts/buildspec/provision-infra-mc.shscripts/buildspec/provision-kube-applier-dynamodb.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/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
| module "kube_applier_mc_messaging" { | ||
| count = var.rc_specs_sns_topic_arn != "" ? 1 : 0 | ||
| source = "../../modules/kube-applier-mc-messaging" | ||
|
|
||
| mc_name = var.management_id | ||
| rc_aws_account_id = var.regional_aws_account_id | ||
| rc_specs_sns_topic_arn = var.rc_specs_sns_topic_arn | ||
| eks_cluster_name = module.management_cluster.cluster_name | ||
| aws_region = var.region | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Gate messaging with an explicit feature flag.
The optional module is enabled solely when an upstream ARN is non-empty. Add an enable_kube_applier_messaging boolean and require both the flag and ARN, so deployment intent is explicit and independently controllable. As per coding guidelines, optional infrastructure must be gated by explicit enable_* flags.
🤖 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 205 - 214, Add the
enable_kube_applier_messaging boolean input and update the
kube_applier_mc_messaging count condition to require both this flag and a
non-empty var.rc_specs_sns_topic_arn. Preserve the module’s existing
configuration while ensuring deployment is explicitly controlled by the feature
flag.
Source: Coding guidelines
| Sid = "AllowSNSDelivery" | ||
| Effect = "Allow" | ||
| Principal = { | ||
| Service = "sns.amazonaws.com" | ||
| } | ||
| Action = [ | ||
| "kms:Decrypt", | ||
| "kms:GenerateDataKey*", | ||
| ] | ||
| Resource = "*" | ||
| Condition = { | ||
| StringEquals = { | ||
| "aws:SourceAccount" = data.aws_caller_identity.current.account_id | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Permit the RC SNS topic to use the queue KMS key.
Line 76 limits the only SNS KMS grant to the MC account, but the inbound specs topic is explicitly in the RC account. SNS delivery to the encrypted MC queue will therefore be denied by KMS. Add a separate SNS statement constrained to var.rc_specs_sns_topic_arn; do not broaden the existing MC-account grant. AWS requires the queue key to allow SNS Decrypt and GenerateDataKey*, preferably scoped by source ARN. (docs.aws.amazon.com)
Proposed fix
+ {
+ Sid = "AllowRCSpecsSNSDelivery"
+ Effect = "Allow"
+ Principal = {
+ Service = "sns.amazonaws.com"
+ }
+ Action = [
+ "kms:Decrypt",
+ "kms:GenerateDataKey*",
+ ]
+ Resource = "*"
+ Condition = {
+ ArnEquals = {
+ "aws:SourceArn" = var.rc_specs_sns_topic_arn
+ }
+ }
+ },📝 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.
| Sid = "AllowSNSDelivery" | |
| Effect = "Allow" | |
| Principal = { | |
| Service = "sns.amazonaws.com" | |
| } | |
| Action = [ | |
| "kms:Decrypt", | |
| "kms:GenerateDataKey*", | |
| ] | |
| Resource = "*" | |
| Condition = { | |
| StringEquals = { | |
| "aws:SourceAccount" = data.aws_caller_identity.current.account_id | |
| } | |
| } | |
| Sid = "AllowSNSDelivery" | |
| Effect = "Allow" | |
| Principal = { | |
| Service = "sns.amazonaws.com" | |
| } | |
| Action = [ | |
| "kms:Decrypt", | |
| "kms:GenerateDataKey*", | |
| ] | |
| Resource = "*" | |
| Condition = { | |
| StringEquals = { | |
| "aws:SourceAccount" = data.aws_caller_identity.current.account_id | |
| } | |
| } | |
| }, | |
| { | |
| Sid = "AllowRCSpecsSNSDelivery" | |
| Effect = "Allow" | |
| Principal = { | |
| Service = "sns.amazonaws.com" | |
| } | |
| Action = [ | |
| "kms:Decrypt", | |
| "kms:GenerateDataKey*", | |
| ] | |
| Resource = "*" | |
| Condition = { | |
| ArnEquals = { | |
| "aws:SourceArn" = var.rc_specs_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 `@terraform/modules/kube-applier-mc-messaging/main.tf` around lines 64 - 78,
Add a separate KMS key-policy statement alongside AllowSNSDelivery for the RC
SNS topic, granting sns.amazonaws.com Decrypt and GenerateDataKey* and
constraining access with aws:SourceArn equal to var.rc_specs_sns_topic_arn.
Leave the existing MC-account AllowSNSDelivery statement unchanged and do not
broaden its source-account condition.
Sources: Coding guidelines, Path instructions
| Condition = { | ||
| StringEquals = { | ||
| "aws:SourceAccount" = data.aws_caller_identity.current.account_id | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Authorize the MC SNS topic to use the RC queue KMS key.
AllowSNSDelivery only permits aws:SourceAccount equal to the RC account, but the status topic is in var.mc_aws_account_id. SNS therefore cannot use this key to deliver to the encrypted RC queues. Add a separate SNS statement scoped to the MC account and, preferably, var.mc_status_sns_topic_arn. AWS requires the encrypted queue’s KMS key policy to authorize SNS delivery. (docs.aws.amazon.com)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@terraform/modules/kube-applier-rc-messaging/main.tf` around lines 84 - 88,
Update the KMS key policy near AllowSNSDelivery by adding a separate statement
authorizing SNS delivery from var.mc_aws_account_id, scoped to
var.mc_status_sns_topic_arn when supported by the policy conditions. Preserve
the existing RC-account AllowSNSDelivery statement and grant only the
permissions required for the MC status topic to use the RC queue key.
| validation { | ||
| condition = var.operator_replica_count >= 1 && var.operator_replica_count <= 10 | ||
| error_message = "operator_replica_count must be between 1 and 10" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the Terraform variable and related usages.
if [ -f terraform/modules/kube-applier-rc-messaging/variables.tf ]; then
echo "== variables.tf =="
cat -n terraform/modules/kube-applier-rc-messaging/variables.tf
else
echo "variables.tf not found"
fd -a 'variables\.tf$' terraform || true
fi
echo "== count usages in module =="
rg -n "operator_replica_count|count\s*=" terraform/modules/kube-applier-rc-messaging || true
echo "== available terraform/bin tools =="
command -v terraform || true
command -v tofu || true
command -v python3 || trueRepository: openshift-online/rosa-hyperfleet
Length of output: 3568
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== main.tf relevant sections =="
sed -n '35,110p' terraform/modules/kube-applier-rc-messaging/main.tf
sed -n '185,250p' terraform/modules/kube-applier-rc-messaging/main.tf
echo "== Terraform version/docs if available =="
if command -v terraform >/dev/null 2>&1; then
terraform version || true
else
echo "terraform not installed"
fi
echo "== hashicorp/terraform docs from source cache if present =="
for f in /usr/share/doc/terraform*/*; do [ -d "$f" ] && find "$f" -maxdepth 2 -type f | head -20 || true; done || true
echo "== Behavioral probe: Terraform count/validation semantics based on docs from installed terraform, if available =="
python3 - <<'PY'
print("No external repo execution; source checked existing Terraform variable type/validation and count usages.")
PYRepository: openshift-online/rosa-hyperfleet
Length of output: 5099
🌐 Web query:
Terraform count meta argument number must be whole number count.index range fractional validation
💡 Result:
In Terraform, the count meta-argument requires a whole number [1][2]. If you provide a fractional number (a float) to the count argument, Terraform will raise a validation error because it cannot create a fractional number of resource instances [3]. Key points regarding this requirement: 1. Whole Number Requirement: The value assigned to count must be a non-negative integer (whole number) [1][2]. 2. Floating Point Errors: If an expression results in a floating-point value (for example, using division), Terraform will not automatically round it. You must explicitly convert the result to a whole number using a function like floor, ceil, or round [3]. 3. Indexing: Similarly, when using count.index to access elements in a collection, the index must be a whole number [3]. Attempting to use a non-integer index (such as a result of a division that produces a decimal) will cause an error [3]. 4. Known Values: The value of count must be known before Terraform performs remote resource operations (during the plan phase) [1][2][4]. It cannot depend on values that are only determined after a resource is created, as Terraform must know the total number of instances to plan the infrastructure graph [1][4]. If you are performing calculations for count that might result in decimals, ensure your expression wraps the calculation in an explicit rounding function, such as: count = floor(var.total_items / var.items_per_instance) This ensures the value passed to count is always a whole number, satisfying the validation requirement [3].
Citations:
- 1: https://developer.hashicorp.com/terraform/language/meta-arguments/count
- 2: https://docs.hashicorp.com/terraform/language/meta-arguments/count
- 3: v0.12: Division computation results in floating point value for indices hashicorp/terraform#20496
- 4: https://developer.hashicorp.com/terraform/language/expressions/references
Reject fractional replica counts.
operator_replica_count is used for count, but the current validation only checks the range, so fractional values like 1.5 pass validation and fail planning because Terraform count requires a whole number.
Proposed fix
- condition = var.operator_replica_count >= 1 && var.operator_replica_count <= 10
- error_message = "operator_replica_count must be between 1 and 10"
+ condition = var.operator_replica_count >= 1 && var.operator_replica_count <= 10 && var.operator_replica_count == floor(var.operator_replica_count)
+ error_message = "operator_replica_count must be a whole number between 1 and 10"📝 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.
| validation { | |
| condition = var.operator_replica_count >= 1 && var.operator_replica_count <= 10 | |
| error_message = "operator_replica_count must be between 1 and 10" | |
| validation { | |
| condition = var.operator_replica_count >= 1 && var.operator_replica_count <= 10 && var.operator_replica_count == floor(var.operator_replica_count) | |
| error_message = "operator_replica_count must be a whole number between 1 and 10" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@terraform/modules/kube-applier-rc-messaging/variables.tf` around lines 40 -
42, Update the validation for operator_replica_count to require a whole-number
value in addition to the existing 1–10 range, so fractional values are rejected
before count uses the variable. Preserve the current bounds and error-message
intent.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/buildspec/provision-infra-mc.sh`:
- Around line 102-110: Update the RC state lookup around _KA_DDB_STATE_KEY to
distinguish a confirmed missing/empty specs_sns_topic_arn from aws s3 cp or jq
failures: invoke the AWS CLI with --region "$TARGET_REGION", split command
assignment from export to address SC2155, and fail the script or preserve the
existing non-empty ARN when retrieval or parsing fails. Only export an empty
TF_VAR_rc_specs_sns_topic_arn for a confirmed first-run absence, preventing
Terraform from destroying existing messaging resources.
🪄 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: Pro Plus
Run ID: 7317b789-6f2e-4c57-a1a9-eae7dea182b0
📒 Files selected for processing (13)
argocd/config/management-cluster/kube-applier/templates/deployment.yamlargocd/config/management-cluster/kube-applier/values.yamlconfig/templates/argocd-bootstrap/applicationset.yaml.j2deploy/ephemeral/us-east-1/argocd-bootstrap-management-cluster/applicationset.yamldeploy/integration/us-east-1/argocd-bootstrap-management-cluster/applicationset.yamlscripts/bootstrap-argocd.shscripts/buildspec/provision-infra-mc.shterraform/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-mc-messaging/main.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 (1)
- argocd/config/management-cluster/kube-applier/values.yaml
| _KA_DDB_STATE_KEY="kube-applier-dynamodb/${CLUSTER_ID}.tfstate" | ||
| export TF_VAR_rc_specs_sns_topic_arn=$( | ||
| aws s3 cp "s3://${_RC_STATE_BUCKET}/${_KA_DDB_STATE_KEY}" - 2>/dev/null \ | ||
| | jq -r '.outputs.specs_sns_topic_arn.value // ""' 2>/dev/null \ | ||
| || echo "" | ||
| ) | ||
| if [ -z "${TF_VAR_rc_specs_sns_topic_arn}" ]; then | ||
| echo "INFO: RC specs SNS topic not yet provisioned for ${CLUSTER_ID} — kube-applier messaging will be skipped until next MC apply." | ||
| fi |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Fail closed when the RC state lookup fails.
aws s3 cp/jq failures are collapsed to "". When messaging already exists, terraform/config/management-cluster/main.tf interprets that as count = 0, so a transient S3, region, access, or parse failure can destroy the MC SQS/SNS resources and subscription. Distinguish confirmed first-run absence from read failures, pass --region "$TARGET_REGION", and preserve or fail on existing state instead of applying with an empty ARN. Split assignment from export while fixing this to address SC2155.
As per path instructions, shell scripts must provide proper error handling and secure AWS CLI usage.
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 103-103: Declare and assign separately to avoid masking return values.
(SC2155)
🤖 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` around lines 102 - 110, Update the
RC state lookup around _KA_DDB_STATE_KEY to distinguish a confirmed
missing/empty specs_sns_topic_arn from aws s3 cp or jq failures: invoke the AWS
CLI with --region "$TARGET_REGION", split command assignment from export to
address SC2155, and fail the script or preserve the existing non-empty ARN when
retrieval or parsing fails. Only export an empty TF_VAR_rc_specs_sns_topic_arn
for a confirmed first-run absence, preventing Terraform from destroying existing
messaging resources.
Sources: Path instructions, Linters/SAST tools
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
terraform/modules/kube-applier-rc-messaging/main.tf (1)
90-94: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAllow remote SNS topics to use the destination queue key.
AllowSNSDeliverypermits only the RC account, but the status topic is in the MC account. MC SNS delivery to the encrypted RC queues will fail. Add a statement scoped to the MC account and status-topic ARN; apply the reciprocal RC allowance to the MC queue CMK for the specs path. SNS requires access to the encrypted destination SQS key. (docs.aws.amazon.com)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@terraform/modules/kube-applier-rc-messaging/main.tf` around lines 90 - 94, Update the KMS queue-key policy around AllowSNSDelivery to permit the MC account’s status-topic ARN to use the encrypted RC destination queue, and add the reciprocal RC-account allowance to the MC queue CMK for the specs path. Scope each statement to the appropriate SNS topic ARN and preserve the existing account restriction.
🤖 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 145-161: The specs topic policy must allow the MC account to
subscribe, and the specs queue subscription must use MC credentials and confirm
successfully. In terraform/modules/kube-applier-rc-messaging/main.tf lines
145-161, extend aws_sns_topic_policy.specs with least-privilege sns:Subscribe
permission for the MC account. In scripts/buildspec/register.sh lines 181-187,
assume MC credentials for the specs subscription, verify the returned
subscription is confirmed, and leave status-queue subscriptions using RC
credentials.
---
Duplicate comments:
In `@terraform/modules/kube-applier-rc-messaging/main.tf`:
- Around line 90-94: Update the KMS queue-key policy around AllowSNSDelivery to
permit the MC account’s status-topic ARN to use the encrypted RC destination
queue, and add the reciprocal RC-account allowance to the MC queue CMK for the
specs path. Scope each statement to the appropriate SNS topic ARN and preserve
the existing account restriction.
🪄 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: Pro Plus
Run ID: 1c45ca4f-b484-4fac-9ca1-2418793f61c8
📒 Files selected for processing (28)
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.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-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
🚧 Files skipped from review as they are similar to previous changes (15)
- argocd/config/management-cluster/kube-applier/templates/deployment.yaml
- deploy/integration/us-east-1/argocd-bootstrap-management-cluster/applicationset.yaml
- argocd/config/regional-cluster/hyperfleet/values.yaml
- deploy/ephemeral/us-east-1/argocd-bootstrap-management-cluster/applicationset.yaml
- terraform/modules/kube-applier-rc-messaging/outputs.tf
- terraform/modules/kube-applier-mc-messaging/versions.tf
- terraform/modules/kube-applier-rc-messaging/versions.tf
- argocd/config/management-cluster/kube-applier/values.yaml
- config/templates/argocd-bootstrap/applicationset.yaml.j2
- terraform/modules/ecs-bootstrap/main.tf
- terraform/modules/kube-applier-mc-messaging/main.tf
- terraform/modules/kube-applier-mc-messaging/outputs.tf
- terraform/modules/kube-applier-rc-messaging/variables.tf
- terraform/modules/ecs-bootstrap/variables.tf
- scripts/bootstrap-argocd.sh
openshift-ci
Bot
added
the
needs-rebase
Replace DynamoDB Streams with SNS/SQS for bidirectional change notifications between the operator (RC) and kube-applier (MC). Add two new Terraform modules: kube-applier-mc-messaging (specs SQS + status SNS on MC side) and kube-applier-rc-messaging (specs SNS + status SQS queues on RC side, with cross-account subscriptions and IAM policies). Wire both into existing management-cluster and kube-applier-dynamodb-provisioning configs, and update buildspec scripts to exchange ARNs between the two stacks. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…cies Both the RC and MC messaging modules had a missing closing bracket after kms:GenerateDataKey* in the AllowSNSDelivery statement, causing the SNS and SQS statements to be merged into a single malformed block. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
kube-applier-aws: quay.io/psav/kube-applier-aws:252920c hyperfleet-operator: quay.io/psav/hyperfleet-operator:6d51902 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
sns:Unsubscribe is not a valid SNS resource-based policy action and causes AWS to reject the policy with 'action out of service scope'. Subscribers can always remove their own subscriptions without an explicit grant. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ation pattern Pass specs_queue_url and status_topic_arn from MC Terraform outputs through the established annotation pipeline (Terraform output → bootstrap-argocd.sh → ECS task env → local-cluster-identity annotation → ApplicationSet valuesObject → kube-applier Helm chart → deployment args). - bootstrap-argocd.sh: read kube_applier_specs_queue_url and kube_applier_status_topic_arn from terraform output -json for MC; pass both as KUBE_APPLIER_SQS_QUEUE_URL / KUBE_APPLIER_SNS_STATUS_TOPIC_ARN to ECS task - ecs-bootstrap/variables.tf: add kube_applier_specs_queue_url and kube_applier_status_topic_arn input variables - ecs-bootstrap/main.tf: inject both as container env vars and as annotations on the local-cluster-identity cluster secret - config/management-cluster/main.tf: pass new vars to ecs-bootstrap module - config/management-cluster/outputs.tf: add kube_applier_specs_queue_url output - applicationset.yaml.j2: add MC-only kubeApplier.config valuesObject block reading the two new annotations; fix pre-existing argo-cd targetGroup indent - deploy/*/argocd-bootstrap-management-cluster/applicationset.yaml: re-rendered - kube-applier/values.yaml: add sqsQueueUrl and snsStatusTopicArn fields - kube-applier/deployment.yaml: add --sqs-queue-url and --sns-status-topic-arn args (conditional on non-empty value) - kube-applier-mc-messaging/main.tf: remove SSM parameters (never consumed) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Remove circular-dependency count gates from both messaging modules and their configs. Each module now constructs the other account's ARNs locally from predictable patterns — no cross-module variable threading. Both SNS→SQS subscriptions are created at the end of register.sh (Stage 4), the first point where all four resources are guaranteed to exist. The script already runs under RC account credentials; the MC status topic policy grants sns:Subscribe to the RC account so both subscriptions can be wired from one place without account switching. AWS auto-removes subscriptions when their SNS topic is deleted, so no explicit teardown step is needed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ionSet The hyperfleet-operator StatefulSet pods call os.Hostname() to get their ordinal and append it to a --sqs-queue-url-prefix flag to form their full SQS queue URL. This flag was never passed through the ArgoCD chart wiring, causing pod crashes when they could not find their queue. The prefix follows the predictable pattern: https://sqs.<region>.amazonaws.com/<account_id>/<cluster_name>-hyperfleet-operator- All three values (aws_region, aws_account_id, cluster_name) are already present on the RC cluster secret as labels/annotations, so the prefix is constructed inline in the ApplicationSet valuesObject — no new Terraform outputs, bootstrap wiring, or ECS variables needed. Changes: - applicationset.yaml.j2: add hyperfleet.sqsQueueUrlPrefix to RC valuesObject - hyperfleet/values.yaml: add sqsQueueUrlPrefix default - hyperfleet/templates/application.yaml: pipe prefix into external chart values - Re-render deploy/ applicationsets Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
quay.io/psav/hyperfleet-operator is from the dead typeid repo and does not contain the /compactor binary. The rosa-hyperfleet-api Makefile publishes to quay.io/openshift-online/hyperfleet-operator. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
register.sh runs as OrganizationAccountAccessRole in the RC account and calls aws sns subscribe on the RC-owned specs topic. Without an explicit sns:Subscribe grant in the topic policy, AWS returns 'pending confirmation' even when the MC SQS queue policy is correct. The MC status topic already has this pattern (AllowRCAccountSubscribe) which is why the status subscriptions confirm immediately. Mirror it on the specs topic. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
openshift-ci
Bot
removed
the
needs-rebase
hyperfleet-operator: 9ae1c99 -> bfe629c (rosa-hyperfleet-api) kube-applier-aws: 252920c -> 9106cf5 Also remove duplicate sqsQueueUrlPrefix key in hyperfleet values.yaml. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… delivery For the specs subscription (RC SNS → MC SQS), SNS acts on behalf of the RC account when encrypting messages into the MC-side KMS-encrypted SQS queue. The aws:SourceAccount condition on AllowSNSDelivery was MC-only, so the KMS key denied the RC-account SNS service principal, preventing subscription auto-confirmation (pending confirmation). Add rc_aws_account_id to the StringEquals condition so both MC-local and cross-account SNS delivery can use the key. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
AWS only auto-confirms an SNS→SQS subscription when the subscribe caller is from the same account as the queue. The specs SQS queue is in the MC account, so register.sh must call sns:Subscribe as the MC account (use_mc_account), not the RC account. Correspondingly, the RC specs topic policy now grants sns:Subscribe to the MC account root (AllowMCAccountSubscribe) instead of the RC account root — the RC account already owns the topic and never needed an explicit Allow to subscribe to its own resources. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
For the status path (MC SNS → RC SQS), SNS delivers on behalf of the MC account. The RC KMS key must permit aws:SourceAccount for both the RC and MC accounts so SNS can encrypt messages before writing to the RC SQS queues. Without this, SNS silently drops all status notifications. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
PR needs rebase. 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. |
openshift-ci
Bot
added
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
terraform/modules/kube-applier-rc-messaging/main.tf (1)
43-54: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winHardcoded
arn:aws:partition breaks cross-partition correctness and consistency.Line 52 and line 53 hardcode the
awspartition prefix, but line 73, line 167, and line 182 usedata.aws_partition.current.partition.local.mc_status_sns_topic_arnfeeds theArnEqualscondition inaws_sqs_queue_policy.status(line 231-233). In a non-standard partition, the constructed ARN will not match the real source ARN, and delivery will be silently denied.Use
data.aws_partition.current.partitionconsistently in both ARN constructions.♻️ Proposed fix
- mc_specs_queue_arn = "arn:aws:sqs:${var.aws_region}:${var.mc_aws_account_id}:${var.mc_name}-specs-notifications" - mc_status_sns_topic_arn = "arn:aws:sns:${var.aws_region}:${var.mc_aws_account_id}:${var.mc_name}-status-notifications" + mc_specs_queue_arn = "arn:${data.aws_partition.current.partition}:sqs:${var.aws_region}:${var.mc_aws_account_id}:${var.mc_name}-specs-notifications" + mc_status_sns_topic_arn = "arn:${data.aws_partition.current.partition}:sns:${var.aws_region}:${var.mc_aws_account_id}:${var.mc_name}-status-notifications"🤖 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 43 - 54, Update the local ARN constructions for mc_specs_queue_arn and mc_status_sns_topic_arn to use data.aws_partition.current.partition instead of the hardcoded aws partition, matching the existing ARN construction patterns elsewhere in the module.terraform/modules/kube-applier-mc-messaging/main.tf (1)
26-43: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winHardcoded
arn:aws:partition breaks cross-partition correctness and consistency.Line 37 and line 42 hardcode the
awspartition prefix, but line 62 and line 211 usedata.aws_partition.current.partition.local.rc_specs_sns_topic_arn(line 42) is compared against the real SNS source ARN in theAllowRCSpecsSNSDeliverycondition (line 158). In a non-standard partition (for example GovCloud), the constructed ARN will not match the real ARN, and theArnEqualscondition will fail closed, silently denying legitimate cross-account delivery.Use
data.aws_partition.current.partitionconsistently in both ARN constructions.♻️ Proposed fix
- kube_applier_role_arn = "arn:aws:iam::${data.aws_caller_identity.current.account_id}:role/${var.mc_name}-kube-applier" + kube_applier_role_arn = "arn:${data.aws_partition.current.partition}:iam::${data.aws_caller_identity.current.account_id}:role/${var.mc_name}-kube-applier" - rc_specs_sns_topic_arn = "arn:aws:sns:${var.aws_region}:${var.rc_aws_account_id}:${var.mc_name}-specs-notifications" + rc_specs_sns_topic_arn = "arn:${data.aws_partition.current.partition}:sns:${var.aws_region}:${var.rc_aws_account_id}:${var.mc_name}-specs-notifications"🤖 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 26 - 43, Update the ARN constructions in the locals block, including kube_applier_role_arn and rc_specs_sns_topic_arn, to interpolate data.aws_partition.current.partition instead of hardcoding the aws partition. Preserve the existing account, region, role, and topic naming components.
🤖 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/hyperfleet/values.yaml`:
- Line 10: Update the regional Hyperfleet values entry sqsQueueUrlPrefix from
its empty default to the Terraform/bootstrap SQS queue-name output or the
existing source used by the regional bootstrap ApplicationSet, so
templates/application.yaml passes the required regional queue URL prefix to the
Hyperfleet chart.
- Around line 12-15: Update the Argo CD source configuration in values.yaml to
use the canonical openshift-online/rosa-hyperfleet-api repository instead of
rrp-bot, and replace the mutable feature/sns-sqs targetRevision with the
approved immutable commit SHA. Keep the existing hyperfleet-operator/charts path
unchanged.
- Around line 24-25: Update the hyperfleet-operator image reference in the
values configuration to use an immutable quay.io digest with the sha256 format
instead of the mutable c756faf tag, preserving the existing repository and
ensuring the deployment pulls the pinned image content.
In `@terraform/modules/kube-applier-mc-messaging/main.tf`:
- Around line 49-53: Update the AWS provider constraint in versions.tf from >=
5.0 to >= 5.49.0, keeping the aws_kms_key.messaging rotation_period_in_days
configuration unchanged.
---
Nitpick comments:
In `@terraform/modules/kube-applier-mc-messaging/main.tf`:
- Around line 26-43: Update the ARN constructions in the locals block, including
kube_applier_role_arn and rc_specs_sns_topic_arn, to interpolate
data.aws_partition.current.partition instead of hardcoding the aws partition.
Preserve the existing account, region, role, and topic naming components.
In `@terraform/modules/kube-applier-rc-messaging/main.tf`:
- Around line 43-54: Update the local ARN constructions for mc_specs_queue_arn
and mc_status_sns_topic_arn to use data.aws_partition.current.partition instead
of the hardcoded aws partition, matching the existing ARN construction patterns
elsewhere in the module.
🪄 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: Pro Plus
Run ID: 413ce488-a7e9-4395-be33-6ab0b8a6d89f
📒 Files selected for processing (28)
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.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-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
🚧 Files skipped from review as they are similar to previous changes (24)
- terraform/config/kube-applier-dynamodb-provisioning/main.tf
- deploy/ephemeral/us-east-1/argocd-bootstrap-management-cluster/applicationset.yaml
- terraform/modules/kube-applier-rc-messaging/versions.tf
- deploy/integration/us-east-1/argocd-bootstrap-management-cluster/applicationset.yaml
- scripts/buildspec/register.sh
- terraform/modules/ecs-bootstrap/variables.tf
- terraform/modules/ecs-bootstrap/main.tf
- terraform/modules/kube-applier-mc-messaging/versions.tf
- scripts/bootstrap-argocd.sh
- argocd/config/regional-cluster/hyperfleet/templates/application.yaml
- argocd/config/management-cluster/kube-applier/templates/deployment.yaml
- deploy/ephemeral/us-east-1/argocd-bootstrap-regional-cluster/applicationset.yaml
- terraform/config/management-cluster/main.tf
- terraform/modules/kube-applier-mc-messaging/outputs.tf
- argocd/config/management-cluster/kube-applier/values.yaml
- terraform/config/kube-applier-dynamodb-provisioning/outputs.tf
- terraform/modules/kube-applier-rc-messaging/outputs.tf
- scripts/buildspec/provision-kube-applier-dynamodb.sh
- deploy/integration/us-east-1/argocd-bootstrap-regional-cluster/applicationset.yaml
- terraform/config/management-cluster/outputs.tf
- terraform/modules/kube-applier-mc-messaging/variables.tf
- terraform/modules/kube-applier-rc-messaging/variables.tf
- terraform/config/kube-applier-dynamodb-provisioning/variables.tf
- config/templates/argocd-bootstrap/applicationset.yaml.j2
|
|
||
| project: default | ||
|
|
||
| sqsQueueUrlPrefix: "" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate hyperfleet manifests =="
fd -a 'values.yaml|application.yaml|applicationset.yaml' . | rg 'argocd/config/regional-cluster/hyperfleet|argocd/config/regional-cluster|argocd/config' | sed 's#^\./##' | sort
echo
echo "== changed files/stat =="
git diff --stat || true
echo
echo "== relevant hyperfleet files =="
for f in argocd/config/regional-cluster/hyperfleet/values.yaml argocd/config/regional-cluster/hyperfleet/templates/application.yaml; do
if [ -f "$f" ]; then
echo "--- $f ($(wc -l < "$f") lines) ---"
cat -n "$f"
fi
done
echo
echo "== search sqsQueueUrlPrefix in argocd config =="
rg -n "sqsQueueUrlPrefix|sqs|sns" argocd/config || true
echo
echo "== search sqsQueueUrlPrefix in repository =="
rg -n "sqsQueueUrlPrefix|sqsQueueUrl|queueUrlPrefix" . || trueRepository: openshift-online/rosa-hyperfleet
Length of output: 8968
Populate the regional SQS queue URL prefix.
argocd/config/regional-cluster/hyperfleet/templates/application.yaml passes this value directly to the Hyperfleet chart, and no overlay overrides it. Sync renders the regional Application with an empty sqsQueueUrlPrefix, so the Hyperfleet SQS messaging path does not receive the required queue URL prefix. Populate this regional cluster value from the Terraform/bootstrap SQS queue-name output or the same source used for the regional bootstrap ApplicationSet.
🤖 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` at line 10, Update the
regional Hyperfleet values entry sqsQueueUrlPrefix from its empty default to the
Terraform/bootstrap SQS queue-name output or the existing source used by the
regional bootstrap ApplicationSet, so templates/application.yaml passes the
required regional queue URL prefix to the Hyperfleet chart.
Source: Path instructions
| source: | ||
| repoURL: https://github.com/typeid/hyperfleet-operator.git | ||
| targetRevision: main | ||
| path: charts/hyperfleet-operator | ||
| repoURL: https://github.com/rrp-bot/rosa-hyperfleet-api.git | ||
| targetRevision: feature/sns-sqs | ||
| path: hyperfleet-operator/charts |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate file =="
if [ -f argocd/config/regional-cluster/hyperfleet/values.yaml ]; then
nl -ba argocd/config/regional-cluster/hyperfleet/values.yaml | sed -n '1,40p'
else
echo "missing argocd/config/regional-cluster/hyperfleet/values.yaml"
fi
echo
echo "== all rosa-hyperfleet-api refs in argocd =="
rg -n "rosa-hyperfleet-api|source:|repoURL:|targetRevision:" argocd/config/regional-cluster || true
echo
echo "== git status/stat =="
git status --shortRepository: openshift-online/rosa-hyperfleet
Length of output: 231
🌐 Web query:
GitHub rrp-bot rosa-hyperfleet-api fork openshift-online rosa-hyperfleet-api
💡 Result:
The term "rrp-bot" refers to a GitHub bot (frequently appearing as rrp-bot in contribution lists) associated with the ROSA Regional Platform (RRP) ecosystem managed under the openshift-online organization [1]. The repository openshift-online/rosa-hyperfleet-api (also referred to as rosa-regional-platform-api in some contexts) is a project focused on ROSA Hosted Control Plane (HCP) regional cluster management [2][3]. It provides a stateless gateway API, an operator for managing Kubernetes custom resources (such as Cluster, NodePool, and Placement), and a library for PostgreSQL-backed controller-runtime operations [2][3]. Regarding the "fork" mentioned in your query: - There is an organization named rosa-regional-platform-ci that contains repositories named rosa-hyperfleet, rosa-hyperfleet-api, and rosa-hyperfleet-cli [4]. These repositories appear to be forks or mirrors created for Continuous Integration (CI) purposes within the OpenShift ecosystem [5][4]. - The main authoritative source for these components is the openshift-online organization (e.g., github.com/openshift-online/rosa-hyperfleet-api and github.com/openshift-online/rosa-regional-platform-api) [2][3]. It is also important to distinguish this from openshift-hyperfleet/hyperfleet-api, which is a separate repository [6]. While sharing similar naming conventions, the openshift-hyperfleet project serves as a general-purpose, event-driven data layer for cluster lifecycle management, distinct from the specific ROSA-focused RRP project [6].
Citations:
- 1: https://github.com/openshift-online/rosa-regional-platform
- 2: https://github.com/openshift-online/rosa-hyperfleet-api
- 3: https://github.com/openshift-online/rosa-regional-platform-api
- 4: https://github.com/rosa-regional-platform-ci
- 5: https://explore.market.dev/ecosystems/go/projects/rosa-regional-platform-api
- 6: https://github.com/openshift-hyperfleet/hyperfleet-api
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== argocd file excerpt =="
while IFS= read -r line; do
printf '%s\n' "$line"
done < argocd/config/regional-cluster/hyperfleet/values.yaml | sed -n '1,60p'
echo
echo "== rosa-hyperfleet-api refs in argocd =="
rg -n "rosa-hyperfleet-api|repoURL:|targetRevision:" argocd/config/regional-cluster || true
echo
echo "== git status/stat =="
git status --shortRepository: openshift-online/rosa-hyperfleet
Length of output: 1492
Pin the Argo CD chart source to an approved immutable revision.
This config points to the mutable feature/sns-sqs branch on rrp-bot/rosa-hyperfleet-api, while openshift-online/rosa-hyperfleet-api is the canonical repository. Automated sync can later deploy branch changes without a change in this repository. Use the approved source repository and an immutable commit SHA for Argo CD chart source pinning.
🤖 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 - 15,
Update the Argo CD source configuration in values.yaml to use the canonical
openshift-online/rosa-hyperfleet-api repository instead of rrp-bot, and replace
the mutable feature/sns-sqs targetRevision with the approved immutable commit
SHA. Keep the existing hyperfleet-operator/charts path unchanged.
Sources: Coding guidelines, Path instructions
| repository: quay.io/psav/hyperfleet-operator | ||
| tag: "c756faf" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- values.yaml context ---\n'
if [ -f argocd/config/regional-cluster/hyperfleet/values.yaml ]; then
nl -ba argocd/config/regional-cluster/hyperfleet/values.yaml | sed -n '1,80p'
else
fd -a 'values.yaml' . | sed -n '1,50p'
fi
printf '\n--- image references nearby ---\n'
rg -n "repository:|tag:|pullPolicy:|quay.io/psav/hyperfleet-operator|c756faf" argocd/config/regional-cluster/hyperfleet || trueRepository: openshift-online/rosa-hyperfleet
Length of output: 261
🌐 Web query:
Quay.io repository immutable tags setting documentation
💡 Result:
As of August 2026, Red Hat Quay and Project Quay support image tag immutability, which prevents existing tags from being overwritten, modified, or deleted [1]. This feature can be managed via the Web UI or the API [1]. There are three primary ways to implement tag immutability: 1. Individual Tag Settings: Users with write access can manually toggle the immutable status of a specific tag using the Red Hat Quay UI or the API [1]. For the API, you can use the PUT /api/v1/repository/{repository}/tag/{tag} endpoint with the "immutable" boolean field [2][3]. Setting this to true marks the tag as immutable; unsetting it requires admin permissions [2][1][3]. 2. Immutability Policies: Administrators can define regex-based policies at the organization or repository level [1]. These policies automatically mark new tags that match the specified regex pattern as immutable [1]. This is configured via the Red Hat Quay v2 UI under the Immutability Policies tab or through the API endpoints under /api/v1/organization/{orgname}/immutabilitypolicy/ or /api/v1/repository/{repository}/immutabilitypolicy/ [4][1][5][6]. 3. Manifest Labels: During the build process, you can trigger immutability by including the quay.immutable=true label in your Containerfile or Dockerfile [1]. These features are enabled by the IMMUTABLE_TAGS feature flag within the Quay platform [5][6]. Note that older discussions suggesting Quay lacks this feature are outdated, as these capabilities were added in recent versions (e.g., version 3.17) [4][1][7][5].
Citations:
- 1: https://docs.redhat.com/en/documentation/red_hat_quay/3/html/use_red_hat_quay/image-tags-overview
- 2: https://docs.redhat.com/en/documentation/red_hat_quay/3/html/red_hat_quay_api_reference/tag
- 3: https://docs.redhat.com/en/documentation/red_hat_quay/3.17/html/red_hat_quay_api_reference/tag
- 4: https://docs.redhat.com/en/documentation/red_hat_quay/3.17/html/red_hat_quay_api_reference/immutability_policy
- 5: feat(web): add immutability policies UI for orgs and repos (PROJQUAY-10163) quay/quay#4942
- 6: feat(endpoints): add immutability policy API endpoints (PROJQUAY-10160) quay/quay#4934
- 7: feat(endpoints): add tag immutability API layer (PROJQUAY-10159) quay/quay#4839
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Quay metadata probe for tag and repo ---'
python3 - <<'PY'
import json
import urllib.request
import urllib.error
base = 'https://quay.io/api/v1/repository/psav/hyperfleet-operator/public'
urls = [
base,
base + '?includeTags=true&tag=100',
base + '/tag/?specificTag=c756faf',
'https://quay.io/api/v1/repository/psav/hyperfleet-operator/',
'https://quay.io/api/v1/repository/psav/hyperfleet-operator/tag/?specificTag=c756faf&limit=10',
'https://quay.io/api/v1/repository/psav/hyperfleet-operator/tag/?specificTag=c756faf',
]
for url in urls:
print(f'\n### {url}')
try:
req = urllib.request.Request(url)
req.add_header('Accept', 'application/json')
with urllib.request.urlopen(req, timeout=20) as rsp:
data = rsp.read()
print(f'status={rsp.status}')
text = data.decode('utf-8', 'replace')
if text.startswith('{') or text.startswith('['):
obj = json.loads(text)
print('top_level_keys=', sorted(obj.keys()) if isinstance(obj, dict) else None)
print(json.dumps(obj, indent=2, sort_keys=True)[:8000])
else:
print(text[:8000])
except Exception as e:
print(f'error={type(e).__name__}: {e}')
PYRepository: openshift-online/rosa-hyperfleet
Length of output: 1636
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- values.yaml context ---'
if [ -f argocd/config/regional-cluster/hyperfleet/values.yaml ]; then
nl -ba argocd/config/regional-cluster/hyperfleet/values.yaml | sed -n '1,80p'
else
fd -a 'values.yaml' . | sed -n '1,50p'
fi || true
printf '%s\n' '--- image references nearby ---'
rg -n "repository:|tag:|pullPolicy:|quay.io/psav/hyperfleet-operator|c756faf" argocd/config/regional-cluster/hyperfleet || true
python3 - <<'PY'
import json
import ssl
import urllib.request
import urllib.error
ctx = ssl._create_unverified_context()
base = 'https://quay.io/api/v1/repository/psav/hyperfleet-operator/public'
urls = [
base,
base + '?includeTags=true&tag=100',
base + '/tag/?specificTag=c756faf',
'https://quay.io/api/v1/repository/psav/hyperfleet-operator/tag/?specificTag=c756faf&limit=10',
]
for url in urls:
print(f'\n### {url}')
try:
req = urllib.request.Request(url)
req.add_header('Accept', 'application/json')
with urllib.request.urlopen(req, context=ctx, timeout=20) as rsp:
data = rsp.read()
print(f'status={rsp.status}')
text = data.decode('utf-8', 'replace')
if text.startswith('{') or text.startswith('['):
obj = json.loads(text)
print('top_level_keys=', sorted(obj.keys()) if isinstance(obj, dict) else None)
print(json.dumps(obj, indent=2, sort_keys=True)[:10000])
else:
print(text[:10000])
except Exception as e:
print(f'error={type(e).__name__}: {e}')
PYRepository: openshift-online/rosa-hyperfleet
Length of output: 1462
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- values.yaml excerpt ---'
awk 'NR>=1 && NR<=40 { printf "%6d %s\n", NR, $0 }' argocd/config/regional-cluster/hyperfleet/values.yaml
python3 - <<'PY'
import json
import ssl
import urllib.request
import urllib.error
ctx = ssl._create_unverified_context()
urls = [
'https://quay.io/api/v1/repository/psav/hyperfleet-operator/tag/?includeManifestSummary=true',
'https://quay.io/api/v1/repository/psav/hyperfleet-operator/tag/?includeManifestSummary=true&limit=1',
]
for url in urls:
print(f'\n### {url}')
try:
req = urllib.request.Request(url)
req.add_header('Accept', 'application/json')
with urllib.request.urlopen(req, context=ctx, timeout=20) as rsp:
data = rsp.read()
print(f'status={rsp.status}')
obj = json.loads(data.decode('utf-8', 'replace'))
tags = obj.get('tags', [])
print('tag_count=', len(tags))
for tag in tags[:5]:
print('name=', tag.get('name'))
ms = tag.get('manifest_summary') or {}
print(' manifest_digest=', ms.get('manifest_digest'))
for v in (tag.get('target') or {}).values() if isinstance(tag.get('target'), dict) else []:
if isinstance(v, dict):
print(' image_digest=', v.get('manifest_digest'))
if tags:
print('manifest_deduplicated=', sorted(set(t.get('manifest_summary', {}).get('manifest_digest') for t in tags)))
except Exception as e:
print(f'error={type(e).__name__}: {e}')
PYRepository: openshift-online/rosa-hyperfleet
Length of output: 1764
Pin the hyperfleet-operator image to an immutable digest.
c756faf is a tag, not a digest. With pullPolicy: Always, each pod start resolves the current content behind that tag unless immutability is separately enforced in Quay. Use quay.io/psav/hyperfleet-operator@sha256:<digest> for the pinned pull target.
🤖 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 24 - 25,
Update the hyperfleet-operator image reference in the values configuration to
use an immutable quay.io digest with the sha256 format instead of the mutable
c756faf tag, preserving the existing repository and ensuring the deployment
pulls the pinned image content.
| resource "aws_kms_key" "messaging" { | ||
| description = "KMS key for ${var.mc_name} kube-applier messaging (SQS + SNS)" | ||
| deletion_window_in_days = 7 | ||
| enable_key_rotation = true | ||
| rotation_period_in_days = 90 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check the pinned AWS provider version for this module.
fd versions.tf terraform/modules/kube-applier-mc-messaging --exec cat {}Repository: openshift-online/rosa-hyperfleet
Length of output: 317
🌐 Web query:
hashicorp Terraform AWS provider aws_kms_key rotation_period_in_days v5.49.0 documentation
💡 Result:
The rotation_period_in_days argument for the aws_kms_key resource was introduced in version 5.49.0 of the Terraform AWS Provider [1][2]. This argument allows you to specify a custom period of time, in days, between each key rotation date [3][4]. Key specifications for this argument include: - Purpose: It enables a custom rotation period when key rotation is enabled [2][5]. - Requirement: To use this argument, the enable_key_rotation argument must be set to true [3][4]. - Range: The value must be an integer between 90 and 2560 (inclusive) [3][4]. You can find the official documentation for this resource in the Terraform Registry under the aws_kms_key resource documentation [3][4].
Citations:
- 1: https://github.com/hashicorp/terraform-provider-aws/releases/tag/v5.49.0
- 2:
r/kms_key: add support forrotation_period_in_dayshashicorp/terraform-provider-aws#37140 - 3: https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/kms_key
- 4: https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/kms_key.html
- 5:
r/kms_key: add support forrotation_period_in_dayshashicorp/terraform-provider-aws#37140
Raise the AWS provider lower bound for this module.
terraform/modules/kube-apchild-mc-messaging/versions.tf declares aws = ">= 5.0", but rotation_period_in_days on aws_kms_key requires Terraform AWS provider >= 5.49.0. Set the module constraint to >= 5.49.0 so terraform plan/apply does not fail with unsupported-argument.
🤖 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 49 - 53,
Update the AWS provider constraint in versions.tf from >= 5.0 to >= 5.49.0,
keeping the aws_kms_key.messaging rotation_period_in_days configuration
unchanged.
2 participants
Summary
Provisions the SNS/SQS messaging infrastructure that replaces DynamoDB Streams for bidirectional change notifications between the hyperfleet-operator (RC account) and kube-applier-aws (MC account).
Related
Summary by CodeRabbit