SREP-3880: Request AWS additional SQS permissions for ROSA Managed Policies (Spot Instance Support)#8134
SREP-3880: Request AWS additional SQS permissions for ROSA Managed Policies (Spot Instance Support)#8134ratnam915 wants to merge 4 commits intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
@ratnam915: This pull request references SREP-3880 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 "4.22.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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe IAM policy Sequence Diagram(s)mermaid ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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
do-not-merge/needs-area
area/cli
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ratnam915 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
area/platform/aws
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8134 +/- ##
==========================================
+ Coverage 26.85% 28.47% +1.62%
==========================================
Files 1090 1099 +9
Lines 105254 108872 +3618
==========================================
+ Hits 28263 31000 +2737
- Misses 74563 75248 +685
- Partials 2428 2624 +196
🚀 New features to boost your workflow:
|
| "Resource": "*", | ||
| "Condition": { | ||
| "StringEquals": { | ||
| "aws:ResourceTag/red-hat-managed": "true" |
There was a problem hiding this comment.
I believe this should be red-hat: true since it is not actually redhat managed it is more like a shared resource. Would that be a correct?
Managed tag is mostly used for resources created as part of install or RH setup instead of customer input
There was a problem hiding this comment.
red-hat-managed: true is for things we manage.
red-hat: true is for things that are part of the platform but not necessarily managed by Red Hat.
There was a problem hiding this comment.
those perms are for selfhosted hcp, they don't impact rosa in any way.
You'll want to include that in the rosa managed policy. Besides, what guarantees the sqs has that tag in rosa?
There was a problem hiding this comment.
@gdbranco @arendej : This has been fixed now, thanks for the suggestion.
@enxebre : You're right — the inline policy in iam.go is for self-hosted HCP and doesn't directly impact ROSA, which uses AWS managed policies. The managed policy update to
ROSANodePoolManagementPolicy is being tracked separately via the AWS submission process https://redhat.atlassian.net/browse/SREP-3880?focusedCommentId=16584538
This change aligns the self-hosted inline policy with the tag condition that will be included in the managed policy update, so that dev/test environments using inline
policies surface any permission issues early rather than only in ROSA with managed policies.
Add required IAM permissions to support infrastructure provisioning.
Correct IAM permission configuration per review comments.
ratnam915
force-pushed
the
feature/SREP-3880
branch
from
803e11e to
ce32fa8
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
@ratnam915: This pull request references SREP-3880 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 "4.22.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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/infra/aws/iam.go`:
- Line 526: The IAM policy condition uses the wrong tag key
"aws:ResourceTag/red-hat"; change it to "aws:ResourceTag/red-hat-managed" so it
matches the actual resource tag used across the codebase (see where
support/awsutil/platform.go checks tag.Key == "red-hat-managed" and
cmd/cluster/aws/create.go appends red-hat-managed=true); update the string
literal in the IAM policy definition in cmd/infra/aws/iam.go to
"aws:ResourceTag/red-hat-managed".
- Around line 516-529: Add a TagQueue method to the SQSAPI interface and ensure
all queue creation paths tag queues with "red-hat":"true"; specifically, modify
support/awsapi/sqs.go to add TagQueue(ctx context.Context, input
*sqs.TagQueueInput) (*sqs.TagQueueOutput, error) (or equivalent signature used
elsewhere), then update every place that creates or initializes queues (e.g.,
calls to CreateQueue or any helper like CreateSQSQueue/CreateQueueIfNotExists)
to include the tag either in CreateQueueInput.Tags or immediately call
SQSAPI.TagQueue after creation using the returned QueueUrl/QueueArn; ensure
callers import and use the new method name (TagQueue) and handle errors returned
from tagging.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 67d1a0bb-22ad-4ebc-b0dc-3188c297e00b
📒 Files selected for processing (1)
cmd/infra/aws/iam.go
| { | ||
| "Sid": "NodePoolSQSActions", | ||
| "Effect": "Allow", | ||
| "Action": [ | ||
| "sqs:DeleteMessage", | ||
| "sqs:ReceiveMessage" | ||
| ], | ||
| "Resource": "*", | ||
| "Condition": { | ||
| "StringEquals": { | ||
| "aws:ResourceTag/red-hat": "true" | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for SQS queue tagging implementation and tag key usage
# Search for SQS tagging operations
echo "=== Searching for SQS TagQueue or TagResource calls ==="
rg -n 'TagQueue|TagResource' --type=go -C3
echo -e "\n=== Searching for red-hat tag key usage ==="
rg -n '"red-hat"' --type=go -C3
echo -e "\n=== Searching for SQS queue creation with tags ==="
rg -nP 'CreateQueue.*tags?|tags?.*CreateQueue' --type=go -C5
echo -e "\n=== Searching for QueueAttributes or Tags in SQS operations ==="
rg -n 'QueueAttributes|Tags.*sqs' --type=go -C3Repository: openshift/hypershift
Length of output: 50377
🏁 Script executed:
#!/bin/bash
# Find the SQSAPI interface definition
echo "=== SQSAPI interface in support/awsapi/sqs.go ==="
find . -path ./vendor -prune -o -name "sqs.go" -type f -print | grep -v vendor | head -5
for file in $(find . -path ./vendor -prune -o -name "sqs.go" -type f -print | grep -v vendor); do
echo "File: $file"
cat "$file"
done
echo -e "\n=== Searching for TagQueue calls outside vendor directory ==="
rg -n 'TagQueue' --type=go --glob='!vendor/**' -C2
echo -e "\n=== Searching for red-hat tag assignment outside vendor ==="
rg -n '"red-hat"' --type=go --glob='!vendor/**' -B2 -A2
echo -e "\n=== Search for SQS queue creation with tags ==="
rg -n 'CreateQueue' --type=go --glob='!vendor/**' -B3 -A5Repository: openshift/hypershift
Length of output: 4459
Add TagQueue method to SQSAPI interface and implement queue tagging with the red-hat: true tag.
The IAM policy restricts sqs:DeleteMessage and sqs:ReceiveMessage operations to queues tagged with red-hat: true. However, the SQSAPI interface lacks a TagQueue method, and there is no code implementing queue tagging. This will cause permission denied errors at runtime when the nodePool attempts to receive or delete messages on untagged queues.
The SQSAPI interface in support/awsapi/sqs.go must include TagQueue, and all SQS queue creation in the application must apply the red-hat: true tag (either during queue creation via the Tags parameter in CreateQueueInput or immediately after creation via TagQueue).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/infra/aws/iam.go` around lines 516 - 529, Add a TagQueue method to the
SQSAPI interface and ensure all queue creation paths tag queues with
"red-hat":"true"; specifically, modify support/awsapi/sqs.go to add TagQueue(ctx
context.Context, input *sqs.TagQueueInput) (*sqs.TagQueueOutput, error) (or
equivalent signature used elsewhere), then update every place that creates or
initializes queues (e.g., calls to CreateQueue or any helper like
CreateSQSQueue/CreateQueueIfNotExists) to include the tag either in
CreateQueueInput.Tags or immediately call SQSAPI.TagQueue after creation using
the returned QueueUrl/QueueArn; ensure callers import and use the new method
name (TagQueue) and handle errors returned from tagging.
| "Resource": "*", | ||
| "Condition": { | ||
| "StringEquals": { | ||
| "aws:ResourceTag/red-hat": "true" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify tag key consistency and check for both variants
echo "=== Searching for 'red-hat-managed' tag usage ==="
rg -n 'red-hat-managed' --type=go -C3
echo -e "\n=== Searching for 'red-hat' tag usage (without '-managed') ==="
rg -n '"red-hat":\s*"true"' --type=go -C3
echo -e "\n=== Checking IAM policies for tag conditions ==="
rg -nP 'aws:ResourceTag/(red-hat|red-hat-managed)' --type=go -C3Repository: openshift/hypershift
Length of output: 10216
Fix tag key mismatch in IAM policy condition.
The IAM policy at line 526 uses aws:ResourceTag/red-hat, but the actual infrastructure tags across the codebase use red-hat-managed=true:
support/awsutil/platform.go:14checks fortag.Key == "red-hat-managed"cmd/cluster/aws/create.go:121appendsred-hat-managed=trueto resource tags- The webhook and tests all reference
red-hat-managed
The IAM condition must be changed to aws:ResourceTag/red-hat-managed to match the tags actually applied to resources. With the current code, the policy condition will never match, resulting in access denied errors.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/infra/aws/iam.go` at line 526, The IAM policy condition uses the wrong
tag key "aws:ResourceTag/red-hat"; change it to
"aws:ResourceTag/red-hat-managed" so it matches the actual resource tag used
across the codebase (see where support/awsutil/platform.go checks tag.Key ==
"red-hat-managed" and cmd/cluster/aws/create.go appends red-hat-managed=true);
update the string literal in the IAM policy definition in cmd/infra/aws/iam.go
to "aws:ResourceTag/red-hat-managed".
|
@ratnam915: This pull request references SREP-3880 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 "4.22.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. |
ratnam915
force-pushed
the
feature/SREP-3880
branch
from
ab28b72 to
585df87
Compare
|
@ratnam915: This pull request references SREP-3880 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 "4.22.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. |
Add TagQueue method to SQSAPI interface in support/awsapi/sqs.go and tag SQS queues with "red-hat":"true" at creation time in the e2e test to match the IAM policy condition on aws:ResourceTag/red-hat.
ratnam915
force-pushed
the
feature/SREP-3880
branch
from
585df87 to
45db83b
Compare
|
@ratnam915: This pull request references SREP-3880 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 "4.22.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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/e2e/nodepool_spot_termination_handler_test.go`:
- Around line 151-153: The test attaches an inline IAM policy that grants
sqs:ReceiveMessage and sqs:DeleteMessage without enforcing the
aws:ResourceTag/red-hat tag condition, so the test bypasses the production gate;
update the test's inline policy (the block at Lines ~113–125 that grants SQS
actions) to include a Condition enforcing "StringEquals":
{"aws:ResourceTag/red-hat": "true"} for those actions (or alternately scope the
policy to the specific SQS resource ARN that has the Tags map set in the test),
keeping the Tags map (the "red-hat": "true" entry around Line 151) as the tag
that satisfies the condition.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: a898f08c-512c-465e-91dc-2cf1415f7727
📒 Files selected for processing (3)
cmd/infra/aws/delegatingclientgenerator/main.gosupport/awsapi/sqs.gotest/e2e/nodepool_spot_termination_handler_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- support/awsapi/sqs.go
Add StringEquals condition for aws:ResourceTag/red-hat to the test's inline IAM policy so it matches the production gate rather than granting unconditional SQS access.
|
@ratnam915: This pull request references SREP-3880 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 "4.22.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. |
|
@ratnam915: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
5 participants
PR Refinement: Scope SQS Permissions with red-hat-managed Tag Condition in Nodepool Inline PolicyTarget
Summary
Scope the AWS IAM actions sqs:DeleteMessage and sqs:ReceiveMessage within the nodePoolPolicy inline policy by applying an aws:ResourceTag/red-hat-managed: "true" condition. This change ensures the development/test inline policy aligns with the security posture update in the upcoming ROSANodePoolManagementPolicy managed policy.Change Description
Remove sqs:DeleteMessage and sqs:ReceiveMessage from the existing general statement (the one encompassing all EC2 actions and Resource: [""]).
Add a new, separate statement after the existing ones:
{
"Sid": "NodePoolSQSActions",
"Effect": "Allow",
"Action": [
"sqs:DeleteMessage",
"sqs:ReceiveMessage"
],
"Resource": "",
"Condition": {
"StringEquals": {
"aws:ResourceTag/red-hat-managed": "true"
}
}
}
Reference
Enhancement: https://github.com/openshift/enhancements/pull/1951
NTH Implementation: https://github.com/openshift/hypershift/pull/7567
API Changes: https://github.com/openshift/hypershift/pull/7625
Jira: OCPSTRAT-1677 / SREP-698
Summary by CodeRabbit