Skip to content

CNTRLPLANE-2205: Auto-detect shared role use and enable for e2e - #7356

Merged
openshift-merge-bot[bot] merged 3 commits into
openshift:mainfrom
sjenning:e2e-shared-role-2
Dec 9, 2025
Merged

CNTRLPLANE-2205: Auto-detect shared role use and enable for e2e#7356
openshift-merge-bot[bot] merged 3 commits into
openshift:mainfrom
sjenning:e2e-shared-role-2

Conversation

@sjenning

@sjenning sjenning commented Dec 9, 2025

Copy link
Copy Markdown
Contributor

Follow on to #7331

This PR improves the AWS IAM role management by making the shared role detection automatic during cluster destruction and enabling shared roles by default in e2e tests. The changes eliminate the need for users to manually specify whether a cluster uses a shared role when destroying it.

Key Changes

  • Auto-detection of shared role during destruction: The IAM destroy logic now automatically detects whether a cluster was created with a shared role or individual component roles by attempting to delete the shared role first. If it exists, only the shared role is removed, avoiding unnecessary API calls for component roles that don't exist.
    Removed --shared-role flag: The --shared-role flag has been removed from the destroy iam command since detection is now automatic. This simplifies the user experience and reduces the chance of errors from mismatched flags.
  • Fixed managed policy handling for shared roles: When using ROSA managed policies with shared roles, the policies are now correctly attached as managed policies rather than inline policies, aligning with the individual component role behavior.
  • Enabled shared roles in e2e tests: The e2e test suite now creates clusters with SharedRole: true by default, ensuring the shared role path is properly tested.

Technical Details

  • The destroy logic returns a boolean indicating whether a role was removed, allowing the code to short-circuit once the shared role is deleted (if it existed) or fall through to delete individual component roles (if it was an older cluster).

Note

Auto-detects shared-role usage during IAM destroy (removing the flag), updates role deletion API, attaches ROSA managed policies for shared roles, and defaults e2e AWS clusters to shared roles (with a targeted per-component test).

  • AWS IAM Destroy:
    • Auto-detect shared role by deleting "<infraID>-shared-role" first; short-circuit if removed.
    • Remove SharedRole from DestroyIAMOptions and drop --shared-role in infra aws destroy-iam.
    • Change DestroyOIDCRole to func(..., name string) (bool, error) and update all callers.
  • IAM Creation (shared role):
    • When UseROSAManagedPolicies is enabled, attach ROSA managed policies to the shared role instead of creating inline policies.
  • E2E:
    • Default AWS create options to SharedRole: true.
    • In TestCreateCluster, explicitly set SharedRole: false on AWS to validate per-component roles.
  • CLI (cluster destroy):
    • Stop passing SharedRole to IAM destroy; behavior now inferred.

Written by Cursor Bugbot for commit 74f3691. This will update automatically on new commits. Configure here.

@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 9, 2025
@openshift-ci

openshift-ci Bot commented Dec 9, 2025

Copy link
Copy Markdown
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@coderabbitai

coderabbitai Bot commented Dec 9, 2025

Copy link
Copy Markdown
Contributor

Walkthrough

Refactored IAM role destruction by removing the SharedRole boolean flag from DestroyIAMOptions and replacing conditional deletion logic with a unified approach that attempts shared role deletion first, then falls back to component roles. Updated DestroyOIDCRole method signature to return a boolean removal indicator alongside error. Added conditional ROSA managed policy attachment in policy binding logic.

Changes

Cohort / File(s) Summary
IAM Role Destruction Refactoring
cmd/cluster/aws/destroy.go, cmd/infra/aws/destroy_iam.go
Removed SharedRole field from DestroyIAMOptions struct and eliminated its initialization. Replaced conditional deletion paths with unified logic that attempts shared role deletion first, then falls back to individual component roles. Updated DestroyOIDCRole method signature from (client, name, includeAssumePolicy bool) error to (client, name) (removed bool, reterr error) and adapted all call sites to handle the two-return-value form for short-circuiting logic.
ROSA Managed Policy Integration
cmd/infra/aws/iam.go
Added conditional logic to attach ROSA managed policies to the shared role when both ROSA managed policies are enabled and a ROSA ARN is provided; otherwise, falls back to inline policy creation. Added logging for both attachment and inline policy paths; skips inline policy creation when managed policy is used.
Test Configuration Updates
test/e2e/create_cluster_test.go, test/e2e/util/options.go
Updated test setup: DefaultAWSOptions now initializes SharedRole: true in AWS platform configuration; create_cluster_test.go overrides this with SharedRole: false for AWS platform deployments during cluster creation.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • DestroyOIDCRole signature change: Verify all call sites correctly handle the new two-return-value form and that the removal boolean is properly used to skip subsequent deletions.
  • Unified deletion logic in destroy_iam.go: Confirm that the fallback logic from shared role to component roles operates correctly and doesn't skip necessary cleanup.
  • ROSA managed policy conditional flow: Validate that the condition logic correctly branches between managed and inline policies, and that inline policy creation is properly skipped when managed policies are used.
  • Test configuration coherence: Ensure the SharedRole flag modifications in tests align with the removal of the field from production code and don't introduce test-only inconsistencies.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent 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

📥 Commits

Reviewing files that changed from the base of the PR and between 9bd5121 and 74f3691.

📒 Files selected for processing (5)
  • cmd/cluster/aws/destroy.go (0 hunks)
  • cmd/infra/aws/destroy_iam.go (6 hunks)
  • cmd/infra/aws/iam.go (1 hunks)
  • test/e2e/create_cluster_test.go (1 hunks)
  • test/e2e/util/options.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:

  • test/e2e/util/options.go
  • test/e2e/create_cluster_test.go
  • cmd/infra/aws/iam.go
  • cmd/infra/aws/destroy_iam.go
🧬 Code graph analysis (1)
test/e2e/create_cluster_test.go (2)
cmd/cluster/core/create.go (1)
  • Platform (823-833)
api/hypershift/v1beta1/hostedcluster_types.go (1)
  • AWSPlatform (1188-1188)
🪛 golangci-lint (2.5.0)
cmd/infra/aws/destroy_iam.go

[error] 228-228: : # github.com/openshift/hypershift/sync-global-pullsecret [github.com/openshift/hypershift/sync-global-pullsecret.test]
sync-global-pullsecret/sync-global-pullsecret_test.go:228:23: undefined: MockdbusConn
sync-global-pullsecret/sync-global-pullsecret_test.go:234:26: undefined: MockdbusConn
sync-global-pullsecret/sync-global-pullsecret_test.go:247:26: undefined: MockdbusConn
sync-global-pullsecret/sync-global-pullsecret_test.go:257:26: undefined: MockdbusConn
sync-global-pullsecret/sync-global-pullsecret_test.go:270:26: undefined: MockdbusConn
sync-global-pullsecret/sync-global-pullsecret_test.go:283:26: undefined: MockdbusConn
sync-global-pullsecret/sync-global-pullsecret_test.go:296:26: undefined: MockdbusConn
sync-global-pullsecret/sync-global-pullsecret_test.go:309:26: undefined: MockdbusConn
sync-global-pullsecret/sync-global-pullsecret_test.go:327:12: undefined: NewMockdbusConn

(typecheck)

⏰ 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 (6)
cmd/infra/aws/iam.go (1)

1032-1056: LGTM - Clean conditional policy attachment logic.

The implementation correctly branches between attaching ROSA managed policies and adding inline policies for the shared role. Error handling and logging are appropriate for both paths.

test/e2e/util/options.go (1)

287-287: LGTM - Shared roles enabled by default for e2e tests.

This aligns with the PR objective. The TestCreateCluster test explicitly overrides this to false to validate per-component roles, ensuring both paths are tested.

test/e2e/create_cluster_test.go (1)

2316-2319: LGTM - Per-component role validation in TestCreateCluster.

Good approach: setting SharedRole = false specifically for this test ensures the per-component role path gets tested, while other tests use the default shared role configuration.

cmd/infra/aws/destroy_iam.go (3)

149-186: LGTM - Clean auto-detection of shared role vs per-component roles.

The destruction logic correctly:

  1. Attempts shared role deletion first
  2. Short-circuits if the shared role was removed (cluster used shared roles)
  3. Falls back to per-component role deletion for older clusters

This removes the need for an explicit --shared-role flag.


192-260: LGTM - Well-structured role deletion with proper return semantics.

The updated DestroyOIDCRole signature correctly returns:

  • (false, nil) when the role doesn't exist (already deleted)
  • (true, nil) when the role was successfully deleted
  • (false, error) on any failure

This enables the calling code to distinguish between "role didn't exist" and "role was removed" for the auto-detection logic.


361-366: LGTM - Correctly ignores the removed boolean for shared VPC roles.

Since these are dedicated roles (not auto-detected), discarding the removed return value with _ is appropriate.


Comment @coderabbitai help to get the list of available commands and usage tips.

@openshift-ci

openshift-ci Bot commented Dec 9, 2025

Copy link
Copy Markdown
Contributor

[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

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. area/cli Indicates the PR includes changes for CLI area/platform/aws PR/issue for AWS (AWSPlatform) platform area/testing Indicates the PR includes changes for e2e testing and removed do-not-merge/needs-area labels Dec 9, 2025
@sjenning sjenning changed the title E2e shared role 2 NO-JIRA: Auto-detect shared role use and enable for e2e Dec 9, 2025
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Dec 9, 2025
@openshift-ci-robot

Copy link
Copy Markdown

@sjenning: This pull request explicitly references no jira issue.

Details

In response to this:

This PR improves the AWS IAM role management by making the shared role detection automatic during cluster destruction and enabling shared roles by default in e2e tests. The changes eliminate the need for users to manually specify whether a cluster uses a shared role when destroying it.

Key Changes

  • Auto-detection of shared role during destruction: The IAM destroy logic now automatically detects whether a cluster was created with a shared role or individual component roles by attempting to delete the shared role first. If it exists, only the shared role is removed, avoiding unnecessary API calls for component roles that don't exist.
    Removed --shared-role flag: The --shared-role flag has been removed from the destroy iam command since detection is now automatic. This simplifies the user experience and reduces the chance of errors from mismatched flags.
  • Fixed managed policy handling for shared roles: When using ROSA managed policies with shared roles, the policies are now correctly attached as managed policies rather than inline policies, aligning with the individual component role behavior.
  • Enabled shared roles in e2e tests: The e2e test suite now creates clusters with SharedRole: true by default, ensuring the shared role path is properly tested.

Technical Details

  • The destroy logic returns a boolean indicating whether a role was removed, allowing the code to short-circuit once the shared role is deleted (if it existed) or fall through to delete individual component roles (if it was an older cluster).

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.

Enable the use of a single shared IAM role for all
test clusters except TestCreateCluster.
Original commit that introduced the shared role did not include
handling for using ROSA managed policies.  The commit adds code
to handle it.
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.
@sjenning

sjenning commented Dec 9, 2025

Copy link
Copy Markdown
Contributor Author

/test verify
/test e2e-aws

@sjenning sjenning changed the title NO-JIRA: Auto-detect shared role use and enable for e2e CNTRLPLANE-2205: Auto-detect shared role use and enable for e2e Dec 9, 2025
@openshift-ci-robot

openshift-ci-robot commented Dec 9, 2025

Copy link
Copy Markdown

@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.

Details

In response to this:

Follow on to #7331

This PR improves the AWS IAM role management by making the shared role detection automatic during cluster destruction and enabling shared roles by default in e2e tests. The changes eliminate the need for users to manually specify whether a cluster uses a shared role when destroying it.

Key Changes

  • Auto-detection of shared role during destruction: The IAM destroy logic now automatically detects whether a cluster was created with a shared role or individual component roles by attempting to delete the shared role first. If it exists, only the shared role is removed, avoiding unnecessary API calls for component roles that don't exist.
    Removed --shared-role flag: The --shared-role flag has been removed from the destroy iam command since detection is now automatic. This simplifies the user experience and reduces the chance of errors from mismatched flags.
  • Fixed managed policy handling for shared roles: When using ROSA managed policies with shared roles, the policies are now correctly attached as managed policies rather than inline policies, aligning with the individual component role behavior.
  • Enabled shared roles in e2e tests: The e2e test suite now creates clusters with SharedRole: true by default, ensuring the shared role path is properly tested.

Technical Details

  • The destroy logic returns a boolean indicating whether a role was removed, allowing the code to short-circuit once the shared role is deleted (if it existed) or fall through to delete individual component roles (if it was an older cluster).

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

sjenning commented Dec 9, 2025

Copy link
Copy Markdown
Contributor Author

@bryan-cox

Copy link
Copy Markdown
Member

/lgtm
/hold

Hold for Cesar and/or others to review as well.

@openshift-ci openshift-ci Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 9, 2025
@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Dec 9, 2025
@sjenning

sjenning commented Dec 9, 2025

Copy link
Copy Markdown
Contributor Author

/verified by @sjenning

with presub e2e artifacts

@openshift-ci-robot openshift-ci-robot added the verified Signifies that the PR passed pre-merge verification criteria label Dec 9, 2025
@openshift-ci-robot

Copy link
Copy Markdown

@sjenning: This PR has been marked as verified by @sjenning.

Details

In response to this:

/verified by @sjenning

with presub e2e artifacts

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
sjenning marked this pull request as ready for review December 9, 2025 18:22
@openshift-ci openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 9, 2025
@openshift-ci-robot

openshift-ci-robot commented Dec 9, 2025

Copy link
Copy Markdown

@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.

Details

In response to this:

Follow on to #7331

This PR improves the AWS IAM role management by making the shared role detection automatic during cluster destruction and enabling shared roles by default in e2e tests. The changes eliminate the need for users to manually specify whether a cluster uses a shared role when destroying it.

Key Changes

  • Auto-detection of shared role during destruction: The IAM destroy logic now automatically detects whether a cluster was created with a shared role or individual component roles by attempting to delete the shared role first. If it exists, only the shared role is removed, avoiding unnecessary API calls for component roles that don't exist.
    Removed --shared-role flag: The --shared-role flag has been removed from the destroy iam command since detection is now automatic. This simplifies the user experience and reduces the chance of errors from mismatched flags.
  • Fixed managed policy handling for shared roles: When using ROSA managed policies with shared roles, the policies are now correctly attached as managed policies rather than inline policies, aligning with the individual component role behavior.
  • Enabled shared roles in e2e tests: The e2e test suite now creates clusters with SharedRole: true by default, ensuring the shared role path is properly tested.

Technical Details

  • The destroy logic returns a boolean indicating whether a role was removed, allowing the code to short-circuit once the shared role is deleted (if it existed) or fall through to delete individual component roles (if it was an older cluster).

[!NOTE]
Cursor Bugbot is generating a summary for commit 74f3691. Configure here.

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

openshift-ci-robot commented Dec 9, 2025

Copy link
Copy Markdown

@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.

Details

In response to this:

Follow on to #7331

This PR improves the AWS IAM role management by making the shared role detection automatic during cluster destruction and enabling shared roles by default in e2e tests. The changes eliminate the need for users to manually specify whether a cluster uses a shared role when destroying it.

Key Changes

  • Auto-detection of shared role during destruction: The IAM destroy logic now automatically detects whether a cluster was created with a shared role or individual component roles by attempting to delete the shared role first. If it exists, only the shared role is removed, avoiding unnecessary API calls for component roles that don't exist.
    Removed --shared-role flag: The --shared-role flag has been removed from the destroy iam command since detection is now automatic. This simplifies the user experience and reduces the chance of errors from mismatched flags.
  • Fixed managed policy handling for shared roles: When using ROSA managed policies with shared roles, the policies are now correctly attached as managed policies rather than inline policies, aligning with the individual component role behavior.
  • Enabled shared roles in e2e tests: The e2e test suite now creates clusters with SharedRole: true by default, ensuring the shared role path is properly tested.

Technical Details

  • The destroy logic returns a boolean indicating whether a role was removed, allowing the code to short-circuit once the shared role is deleted (if it existed) or fall through to delete individual component roles (if it was an older cluster).

[!NOTE]
Auto-detects shared-role usage during IAM destroy (removing the flag), updates role deletion API, attaches ROSA managed policies for shared roles, and defaults e2e AWS clusters to shared roles (with a targeted per-component test).

  • AWS IAM Destroy:
    • Auto-detect shared role by deleting "<infraID>-shared-role" first; short-circuit if removed.
    • Remove SharedRole from DestroyIAMOptions and drop --shared-role in infra aws destroy-iam.
    • Change DestroyOIDCRole to func(..., name string) (bool, error) and update all callers.
  • IAM Creation (shared role):
    • When UseROSAManagedPolicies is enabled, attach ROSA managed policies to the shared role instead of creating inline policies.
  • E2E:
    • Default AWS create options to SharedRole: true.
    • In TestCreateCluster, explicitly set SharedRole: false on AWS to validate per-component roles.
  • CLI (cluster destroy):
    • Stop passing SharedRole to IAM destroy; behavior now inferred.

Written by Cursor Bugbot for commit 74f3691. This will update automatically on new commits. Configure here.

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
openshift-ci Bot requested review from bryan-cox and muraee December 9, 2025 18:24
@sjenning sjenning added the acknowledge-critical-fixes-only Indicates if the issuer of the label is OK with the policy. label Dec 9, 2025
@csrwng

csrwng commented Dec 9, 2025

Copy link
Copy Markdown
Contributor

/hold cancel

@openshift-ci openshift-ci Bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 9, 2025
@sjenning

sjenning commented Dec 9, 2025

Copy link
Copy Markdown
Contributor Author

/override ci/prow/e2e-aks

@openshift-ci

openshift-ci Bot commented Dec 9, 2025

Copy link
Copy Markdown
Contributor

@sjenning: Overrode contexts on behalf of sjenning: ci/prow/e2e-aks

Details

In response to this:

/override ci/prow/e2e-aks

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.

@openshift-ci

openshift-ci Bot commented Dec 9, 2025

Copy link
Copy Markdown
Contributor

@sjenning: all tests passed!

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

acknowledge-critical-fixes-only Indicates if the issuer of the label is OK with the policy. approved Indicates a PR has been approved by an approver from all required OWNERS files. area/cli Indicates the PR includes changes for CLI area/platform/aws PR/issue for AWS (AWSPlatform) platform area/testing Indicates the PR includes changes for e2e testing jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. verified Signifies that the PR passed pre-merge verification criteria

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants