CNTRLPLANE-2205: cmd/aws: fix managed role handling and auto-detect on destroy - #7373
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. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughRemoved the SharedRole option from AWS destroy paths, changed DestroyOIDCRole/DestroyOIDCRoleWithRetry to return a removal boolean, updated callers to short-circuit when a shared role is removed, and added conditional attachment of ROSA managed policies instead of always creating inline policies. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
[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
approved
Original commit that introduced the shared role did not include handling for using ROSA managed policies. The commit adds code to handle it.
1347fe1 to
ebb68d4
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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 (3)
cmd/cluster/aws/destroy.go(0 hunks)cmd/infra/aws/destroy_iam.go(7 hunks)cmd/infra/aws/iam.go(1 hunks)
💤 Files with no reviewable changes (1)
- cmd/cluster/aws/destroy.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/infra/aws/iam.gocmd/infra/aws/destroy_iam.go
⏰ 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). (3)
- 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
🔇 Additional comments (4)
cmd/infra/aws/iam.go (1)
1032-1056: LGTM!The conditional logic properly differentiates between ROSA managed policies and inline policies. Error handling and logging are appropriate for each path. Bindings without a
rosaManagedPolicyARN(likekarpenterPolicy) will correctly fall back to inline policies.cmd/infra/aws/destroy_iam.go (3)
149-158: LGTM!The auto-detection approach is a clean improvement - it eliminates the need for a
SharedRoleconfiguration flag during destruction by probing for the shared role first and short-circuiting if found. This simplifies the API and makes the destroy flow self-adapting.
208-277: LGTM!The new return signature
(removed bool, reterr error)enables clean auto-detection of shared vs individual roles. The semantics are clear:removed=trueindicates a role was found and deleted,removed=falsewithnilerror indicates the role didn't exist.
377-383: LGTM!The calls correctly use the new
DestroyOIDCRolesignature. Ignoring theremovedreturn value is appropriate here since the function is performing cleanup regardless of whether roles existed.
ebb68d4 to
fff2699
Compare
|
@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.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. |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
@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.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. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
We added a --shared-role flag to the iam destroy path, but this creates and issue where the create and destroy must be run with the same flags, otherwise the cluster doesn't clean up properly. We can detect if a cluster was created with --shared-role in the destory path so just Do The Right Thing and remove the flag.
fff2699 to
0f3e8b2
Compare
|
@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.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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/infra/aws/destroy_iam.go (1)
192-205: Propagate caller context (don’t usecontext.Background()), and avoid string-matchingDeleteConflict.
Usingcontext.Background()makesdestroy-iamignore cancellation/timeouts from the command context and can hang up to 3 minutes after the user cancels. Also,strings.Contains(err.Error(), "DeleteConflict")is fragile; preferawserr.Errorcode matching.-// DestroyOIDCRoleWithRetry retries the entire DestroyOIDCRole operation if it fails due to attached policies -func (o *DestroyIAMOptions) DestroyOIDCRoleWithRetry(client iamiface.IAMAPI, name string) error { - return wait.PollUntilContextTimeout(context.Background(), 10*time.Second, 3*time.Minute, true, func(ctx context.Context) (bool, error) { +// DestroyOIDCRoleWithRetry retries the entire DestroyOIDCRole operation if it fails due to attached policies +func (o *DestroyIAMOptions) DestroyOIDCRoleWithRetry(ctx context.Context, client iamiface.IAMAPI, name string) error { + return wait.PollUntilContextTimeout(ctx, 10*time.Second, 3*time.Minute, true, func(ctx context.Context) (bool, error) { _, err := o.DestroyOIDCRole(client, name) if err != nil { - // Check if the error message indicates a delete conflict - if strings.Contains(err.Error(), "DeleteConflict") { + // Retry when IAM reports a delete conflict (typically due to still-attached policies) + if aerr, ok := err.(awserr.Error); ok && aerr.Code() == "DeleteConflict" { o.Log.Info("Role deletion failed due to attached policies, retrying entire operation", "role", fmt.Sprintf("%s-%s", o.InfraID, name)) return false, nil } return false, err } return true, nil }) }(And update the call site at Line 160 accordingly.)
🧹 Nitpick comments (1)
cmd/infra/aws/destroy_iam.go (1)
208-277: Good API change returning(removed bool, error); consider paginating IAM policy listings.
Returningremovedcleanly supports the shared-role short-circuit. One concern:ListAttachedRolePoliciesandListRolePoliciesare paginated; a role with >1 page of policies will not be fully cleaned up andDeleteRolecan keep failing withDeleteConflict.- attachedPolicies, err := client.ListAttachedRolePolicies(&iam.ListAttachedRolePoliciesInput{ - RoleName: aws.String(roleName), - }) - if err != nil { - return false, fmt.Errorf("failed to list attached policies for role %s: %w", roleName, err) - } - - for _, policy := range attachedPolicies.AttachedPolicies { - _, err = client.DetachRolePolicy(&iam.DetachRolePolicyInput{ - PolicyArn: policy.PolicyArn, - RoleName: aws.String(roleName), - }) - if err != nil { - return false, fmt.Errorf("failed to detach policy %s from role %s: %w", *policy.PolicyArn, roleName, err) - } - o.Log.Info("Detached role policy", "role", roleName, "policy", *policy.PolicyArn) - } + err = client.ListAttachedRolePoliciesPages( + &iam.ListAttachedRolePoliciesInput{RoleName: aws.String(roleName)}, + func(page *iam.ListAttachedRolePoliciesOutput, lastPage bool) bool { + for _, policy := range page.AttachedPolicies { + _, detachErr := client.DetachRolePolicy(&iam.DetachRolePolicyInput{ + PolicyArn: policy.PolicyArn, + RoleName: aws.String(roleName), + }) + if detachErr != nil { + reterr = fmt.Errorf("failed to detach policy %s from role %s: %w", aws.StringValue(policy.PolicyArn), roleName, detachErr) + return false + } + o.Log.Info("Detached role policy", "role", roleName, "policy", aws.StringValue(policy.PolicyArn)) + } + return !lastPage + }, + ) + if err != nil { + return false, fmt.Errorf("failed to list attached policies for role %s: %w", roleName, err) + } + if reterr != nil { + return false, reterr + }(Same idea for
ListRolePoliciesviaListRolePoliciesPages.)
📜 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 (3)
cmd/cluster/aws/destroy.go(0 hunks)cmd/cluster/core/destroy.go(0 hunks)cmd/infra/aws/destroy_iam.go(7 hunks)
💤 Files with no reviewable changes (2)
- cmd/cluster/core/destroy.go
- cmd/cluster/aws/destroy.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/infra/aws/destroy_iam.go
⏰ 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-release-mce-211-on-pull-request
- GitHub Check: Red Hat Konflux / hypershift-cli-mce-211-on-pull-request
- GitHub Check: Cursor Bugbot
🔇 Additional comments (2)
cmd/infra/aws/destroy_iam.go (2)
377-382: Updated callers for new(removed bool, error)signature look consistent.
No issues with ignoring the boolean here since you always want to attempt both role deletions.
149-186: Shared-role short-circuit and role-name conventions verified as correct.The destroy logic properly tries
${infraID}-shared-rolefirst and returns early when deleted, saving API calls. All ten per-component role-name suffixes (openshift-ingress, openshift-image-registry, aws-ebs-csi-driver-controller, cloud-controller, node-pool, control-plane-operator, cloud-network-config-controller, kms-provider, karpenter) exactly match the bindings created inCreateOIDCResources. The implementation gracefully handles missing roles by returning(false, nil)when a role doesn't exist, so unconditionally attempting to delete conditional roles (karpenter if not requested, kms-provider if no KMS key) poses no issue.
sjenning
added
the
acknowledge-critical-fixes-only
|
/override ci/prow/e2e-aws failure due to branching and needing #7315 |
|
@sjenning: Overrode contexts on behalf of sjenning: ci/prow/e2e-aws 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. |
|
/lgtm |
|
also for aks /override ci/prow/e2e-aks |
|
@sjenning: Overrode contexts on behalf of sjenning: ci/prow/e2e-aks 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-kubevirt-aws-ovn-reduced /hold for #7315 to merge and not retest |
openshift-ci
Bot
added
the
do-not-merge/hold
|
@sjenning: Overrode contexts on behalf of sjenning: 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. |
|
/hold cancel /verified by @sjenning |
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. |
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. |
3 participants
#7356 got reverted.
This PR contains two of the three commits in that original PR, dropping the commit that caused CI issues by actually enabling it in CI, which didn't work due to #7356.
However, thses fixes to the
--shared-rolefunctionality incmdneed to be merged, independent of CI enabling of this feature.Note
Auto-detect and delete a shared IAM role during AWS destroy (fallback to per-component roles), remove the --shared-role flag/field, adjust IAM deletion APIs, and allow attaching ROSA managed policies when creating a shared role.
--shared-roleflag andAWSPlatformDestroyOptions.SharedRolefield."<infraID>-shared-role"; if found, stop; otherwise delete individual component roles.DestroyOIDCRolenow returns(removed bool, error);DestroyOIDCRoleWithRetry(name)updated accordingly.UseROSAManagedPolicies, attach ROSA managed policies to the shared role; otherwise add inline policies.Written by Cursor Bugbot for commit 0f3e8b2. This will update automatically on new commits. Configure here.