Skip to content

test: new positive E2E - reuse managed MIs within cluster - #6141

Draft
mvacula02 wants to merge 12 commits into
Azure:mainfrom
mvacula02:mvacula/cluster-mi-reuse
Draft

mvacula02 wants to merge 12 commits into
Azure:mainfrom
mvacula02:mvacula/cluster-mi-reuse

Conversation

@mvacula02

@mvacula02 mvacula02 commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator

ARO-28671

What

adding a new positive E2E test case that reuses managed identities within a cluster. The test assigns the same managed identity resource ID to two different control plane operators (ingress and cluster-api-azure) and verifies that cluster creation completes successfully.

removing validation that prevents MI reuse within cluster.

Depends on https://gitlab.cee.redhat.com/service/aro-hcp-clusters-service/-/merge_requests/401

Why

MI reuse within a cluster is a valid scenario. This test demonstrates that a cluster provisions successfully when two operators share the same managed identity, providing justification to remove the validateManagedIdentitiesUniqueWithinCluster validation added in #6082 #6121

Testing

The PR adds an E2E test for existing feature.

PR Checklist

  • restore MustFilter and config
  • PR is scoped to a single task (no mixed concerns)
  • Title follows Conventional Commits format
  • Summary explains the "Why" behind the change
  • Linked to relevant ticket/issue
  • Screenshots included (if graph/UI/metrics changes)
  • Self-reviewed the diff
  • CI/CD checks are passing (ignore Tide)
  • Draft PR used for WIP (if applicable)
  • Commit history is clean (rebased/squashed)
  • Tricky code blocks are commented
  • Specific reviewers tagged
  • All comment threads resolved before merge

If E2E tests are included:

  • E2E tests follow Principles of Good E2E Test Case Design
  • If new E2E use case is covered (via a new test or new check/verifier),
    demonstrate that the test is able to detect a defect/error and fail with
    proper error message and logs which communicates nature of the problem.

@openshift-ci

openshift-ci Bot commented Jul 17, 2026

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

@mvacula02
mvacula02 force-pushed the mvacula/cluster-mi-reuse branch from 6181b03 to 5552a3f Compare July 17, 2026 10:32
@mvacula02 mvacula02 changed the title test: new negative E2E - reuse managed MIs within cluster test: new positive E2E - reuse managed MIs within cluster Jul 17, 2026
@mvacula02

Copy link
Copy Markdown
Collaborator Author

/test ?

@mvacula02

Copy link
Copy Markdown
Collaborator Author

/test integration-e2e-parallel

@mvacula02

Copy link
Copy Markdown
Collaborator Author

/test stage-e2e-parallel

@mvacula02

Copy link
Copy Markdown
Collaborator Author

/test integration-e2e-parallel

@mvacula02

Copy link
Copy Markdown
Collaborator Author

/test e2e-parallel

@mvacula02

mvacula02 commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator Author

Status update 17-07-2026:

  • added E2E test
  • removed validation that rejects MI reuse
  • blocked until 6082 replaces CS functionality

Action Items:

  • validate once 6082 merges
  • modify validateManagedIdentitiesUniqueWithinCluster, L747-777 in validate_cluster.go (6082)
  • revert mustFilter
  • regenerate fixtures

@JakobGray

Copy link
Copy Markdown
Collaborator

mvacula02 Just as a heads up, 6082 replicates the check done in CS, but doesn't remove it from CS. So that will likely need to happen for this to pass.

The validation: https://gitlab.cee.redhat.com/service/aro-hcp-clusters-service/-/blob/master/cmd/clusters-service/servecmd/apiserver/validation_helpers_aro_hcp.go#L149

@mvacula02
mvacula02 force-pushed the mvacula/cluster-mi-reuse branch from 06c6c0a to dd100ee Compare July 24, 2026 11:51
@mvacula02
mvacula02 marked this pull request as ready for review July 27, 2026 09:16
Copilot AI lite review requested due to automatic review settings July 27, 2026 09:16

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@mvacula02

Copy link
Copy Markdown
Collaborator Author

Copilot AI review requested due to automatic review settings July 27, 2026 14:16

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Comment thread test/e2e/cluster_create_mi_reuse.go
@mvacula02
mvacula02 force-pushed the mvacula/cluster-mi-reuse branch from 4f15578 to 1372819 Compare July 31, 2026 11:59
Copilot AI review requested due to automatic review settings July 31, 2026 11:59

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@mvacula02
mvacula02 force-pushed the mvacula/cluster-mi-reuse branch from 1372819 to bdf64ac Compare August 4, 2026 11:15
@mvacula02

Copy link
Copy Markdown
Collaborator Author

/test e2e-parallel

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new E2E test currently risks nil-pointer panics when dereferencing identity profile entries and should add explicit assertions to fail cleanly.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

internal/validation/validate_cluster_comprehensive_test.go:784

  • This test case now expects no validation errors, but the case name still says "duplicate managed identity…" which reads like a failure scenario. Rename the case to reflect that identity reuse is allowed to keep the test intent clear.
			}(),
			expectErrors: []utils.ExpectedError{},
  • Files reviewed: 23/23 changed files
  • Comments generated: 2
  • Review effort level: Lite

Comment on lines +64 to +76
By("reusing the same managed identity for all control plane operators")
cpOps := clusterParams.UserAssignedIdentitiesProfile.ControlPlaneOperators
sharedMI := cpOps["cluster-api-azure"]
for name := range cpOps {
if name == "cluster-api-azure" {
continue
}
originalMI := *cpOps[name]
cpOps[name] = sharedMI
if originalMI != *sharedMI {
delete(clusterParams.Identity.UserAssignedIdentities, originalMI)
}
}
Comment on lines 763 to +764
}(),
expectErrors: []utils.ExpectedError{
{Message: "must be unique within the cluster", FieldPath: "customerProperties.platform.operatorsAuthentication.userAssignedIdentities.dataPlaneOperators"},
},
expectErrors: []utils.ExpectedError{},
@mvacula02

Copy link
Copy Markdown
Collaborator Author

/test e2e-parallel

2 similar comments
@mvacula02

Copy link
Copy Markdown
Collaborator Author

/test e2e-parallel

@mvacula02

Copy link
Copy Markdown
Collaborator Author

/test e2e-parallel

@openshift-ci

openshift-ci Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mvacula02
Once this PR has been reviewed and has the lgtm label, please assign deads2k for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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

@mvacula02
mvacula02 force-pushed the mvacula/cluster-mi-reuse branch from a86f006 to 3f731a7 Compare September 7, 2026 07:54
@mvacula02

Copy link
Copy Markdown
Collaborator Author

/test e2e-parallel

2 similar comments
@mvacula02

Copy link
Copy Markdown
Collaborator Author

/test e2e-parallel

@mvacula02

Copy link
Copy Markdown
Collaborator Author

/test e2e-parallel

@openshift-ci

openshift-ci Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

mvacula02: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/stage-e2e-parallel f6f9ea8 link false /test stage-e2e-parallel
ci/prow/integration-e2e-parallel 680d27e link false /test integration-e2e-parallel
ci/prow/bicep-lint 8d2fd84 link true /test bicep-lint
ci/prow/e2e-parallel 3f731a7 link true /test e2e-parallel

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.

@openshift-ci

openshift-ci Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

PR needs rebase.

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.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants