Revert "CNTRLPLANE-2205: Auto-detect shared role use and enable for e2e" - #7365
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@sjenning: This pull request references CNTRLPLANE-2205 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.21.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. |
|
@sjenning: This pull request references CNTRLPLANE-2205 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.21.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. |
WalkthroughThe changes add a SharedRole flag to control AWS IAM role deletion behavior, updating the DestroyOIDCRole method signature and conditional destruction logic while simplifying shared OIDC role creation to exclusively use inline policies instead of ROSA-managed policies. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
@sjenning: This pull request references CNTRLPLANE-2205 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.21.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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sjenning The full list of commands accepted by this bot can be found here. The pull request process is described 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
|
/override ci/prow/e2e-aks |
|
@sjenning: Overrode contexts on behalf of sjenning: ci/prow/e2e-aks, ci/prow/e2e-aks-4-20, ci/prow/e2e-aws, ci/prow/e2e-aws-upgrade-hypershift-operator, ci/prow/e2e-kubevirt-aws-ovn-reduced 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. |
|
/override ci/prow/e2e-aws-4-20 |
|
@sjenning: Overrode contexts on behalf of sjenning: ci/prow/e2e-aws-4-20 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/infra/aws/destroy_iam.go (1)
191-191: Unused parameterincludeAssumePolicy.The
includeAssumePolicyparameter is never referenced within the function body. The function deletes all inline policies viaListRolePolicies(lines 222-248), which inherently includes assume policies, making this parameter ineffective.Since this is a revert PR to restore job reliability, this can be addressed in a follow-up cleanup.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (5)
cmd/cluster/aws/destroy.go(1 hunks)cmd/infra/aws/destroy_iam.go(8 hunks)cmd/infra/aws/iam.go(1 hunks)test/e2e/create_cluster_test.go(0 hunks)test/e2e/util/options.go(0 hunks)
💤 Files with no reviewable changes (2)
- test/e2e/util/options.go
- test/e2e/create_cluster_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
cmd/cluster/aws/destroy.gocmd/infra/aws/destroy_iam.gocmd/infra/aws/iam.go
🧬 Code graph analysis (1)
cmd/cluster/aws/destroy.go (1)
api/hypershift/v1beta1/hostedcluster_types.go (1)
AWSPlatform(1188-1188)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Red Hat Konflux / hypershift-operator-main-on-pull-request
- GitHub Check: Red Hat Konflux / hypershift-cli-mce-211-on-pull-request
- GitHub Check: Red Hat Konflux / hypershift-release-mce-211-on-pull-request
- GitHub Check: Cursor Bugbot
🔇 Additional comments (6)
cmd/infra/aws/iam.go (1)
1032-1044: LGTM!The simplified inline policy creation loop correctly attaches each binding's policy to the shared role. Error handling and logging are appropriate.
cmd/infra/aws/destroy_iam.go (4)
32-32: LGTM!The
SharedRolefield is correctly added to control IAM deletion behavior, aligning with the broader SharedRole concept.
53-53: LGTM!CLI flag correctly exposed for operator control.
151-185: LGTM!The conditional deletion logic correctly handles both shared and individual role scenarios. The list of component roles matches those created in
CreateOIDCResources.
360-366: LGTM!The calls are consistent with the updated signature.
cmd/cluster/aws/destroy.go (1)
100-109: LGTM!The
SharedRolefield is correctly passed through toDestroyIAMOptions, enabling consistent handling of shared vs. individual role deletion.
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
|
Fix found #7366 |
|
/verified by @sjenning |
sjenning
added
the
acknowledge-critical-fixes-only
openshift-ci-robot
added
the
verified
|
@sjenning: This PR has been marked as verified by 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. |
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
@sjenning: 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. |
2 participants
Reverts #7356
There is some transient issue with the ingress-operator cloud cred secret not being reconciled.
Per TRT SOP, reverting to get jobs reliable again.
Note
Adds CLI flags to delete either the shared IAM role or per-component roles, refactors role deletion API/flows (incl. shared VPC), and makes shared role creation use inline policies; updates e2e defaults accordingly.
--shared-roleflag tocmd/cluster/aws destroyandcmd/infra/aws destroy-iamto choose deleting the shared IAM role vs per-component roles.AWSPlatform.SharedRolethrough toDestroyIAMOptions.DestroyOIDCRolesignature to returnerrorand acceptincludeAssumePolicyboolean; update all call sites (incl. shared VPC roles).shared-rolewhenSharedRoleis true; otherwise delete individual roles (e.g.,openshift-ingress,control-plane-operator, etc.).CreateSharedOIDCRole, attach all permissions as inline policies (remove managed policy attachment path) and add optional assume policy inline.TestCreateClusterand remove defaultSharedRolesetting from AWS test options.Written by Cursor Bugbot for commit 0f08f53. This will update automatically on new commits. Configure here.