Skip to content

test: prototype DEV mock-MSI ACL isolation (ARO-29287) - #6818

Draft
Mike Gahagan (mgahagan73) wants to merge 1 commit into
Azure:mainfrom
mgahagan73:ARO-29287-mock-msi-acl-isolation
Draft

Mike Gahagan (mgahagan73) wants to merge 1 commit into
Azure:mainfrom
mgahagan73:ARO-29287-mock-msi-acl-isolation

Conversation

@mgahagan73

@mgahagan73 Mike Gahagan (mgahagan73) commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

https://issues.redhat.com/browse/ARO-29287

Related: ARO-29290, ARO-29288, ARO-29289

What

Adds a DEV-only e2e spec and dedicated suite development/mock-msi-acl (Parallelism is the literal 1) that strips, then restores, the job-leased mock-MSI subscription grants so cluster delete can be shown to work when the identity used for cluster ops has no ACL.

  • New spec: test/e2e/cluster_delete_missing_identities_dev_mock_msi.go (Mock-MSI-ACL, MIContainers:0, dedicated UAMIs).
  • Helpers: resolve lease from LEASED_MSI_MOCK_SP / ARO_HCP_MSI_MOCK_PRINCIPAL_ID, list with assignedTo('{id}'), strip, restore the same assignment GUIDs. Refuses aro-dev-msi-mock2.
  • Excluded from every existing parallel suite. test/e2e/cluster_delete_missing_identities.go stays StageAndProdOnly.
  • Docs: docs/ci/mock-msi-acl-isolation.md (false-pass if Helm drifted, personal-dev overlay).

Does not wire openshift/release (ARO-29289) or change the hardcoded MI dataplane client.

Why

In DEV every operator authenticates as one mock SP. Deleting customer UAMIs does not remove that SP's subscription dev-msi-mock / Key Vault Crypto User grants, so the STG/PROD missing-identity delete spec is vacuous if run in DEV. The high-value ARO-29096 signal is that delete still works when that impersonated identity has no ACL.

Ginkgo Serial does not serialize across OTE workers, and the default DEV suite is Parallelism: 24, so stripping the job's mock SP in-place would fail sibling specs. Isolation is "this invocation owns the leased pool SP exclusively", not "mutate a shared grant while 24 specs are using it". Shared personal-dev aro-dev-msi-mock2 must never be stripped.

Testing

  • Unit tests: test/util/framework/mock_msi_permissions_test.go — lease resolve (mock2 refuse, pool lookup, env mismatch), assignedTo list filter, strip/restore (RoleAssignmentExists is success on restore).

  • Integration tests: none. This is e2e + suite wiring; no RP/Cosmos artifact change.

  • E2E: personal-dev soak on pool member 19 (aro-hcp-msi-mock-cs-sp-dev-19). Create + viability, strip both subscription assignments, delete customer UAMIs, cluster delete to ARM 404 and managed RG 404, DeferCleanup restored both GUIDs. SUCCESS! -- 1 Passed | 0 Failed.

    Failure modes seen while bringing pers up (spec/helpers did fail closed as intended): ARM 400 UnsupportedQuery on principalId eq (filter is assignedTo); AADSTS700027 when Helm clientId and mounted cert disagree; spec refuses to strip mock2. A 4.21 complete-create sanity created a viable cluster first; that spec later failed on a local :8443 port-forward drop during node pool wait — this PR spec does not create a node pool.

    Not in the default DEV parallel job. LEASED_MSI_MOCK_SP is still provision-only until ARO-29290.

Special notes for your reviewer

  • Parallelism: 1 must stay a literal. ARO_HCP_SUITE_PARALLELISM must not raise it.
  • Env export is not enough: if CS/backend Helm still impersonate aro-dev-msi-mock2 while the test strips a pool member, delete can false-pass. See the false-pass section in the isolation doc; ARO-29290 should smoke running pods, not only rendered config.
  • Restore is RBAC only. Personal-dev Helm miMock* must be put back on mock2 after overlay testing.
  • Suggested reviewers: bvesel Steve Kuznetsov (@stevekuznetsov)

PR Checklist

  • 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 dashboards or other UI 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.

Made with Cursor

DEV cluster-delete-missing-identities is vacuous while the job-leased mock SP
still has subscription ACL. Isolate strip/restore on a Parallelism-1 suite so
sibling specs and personal-dev mock2 are never touched.

Co-authored-by: Cursor <cursoragent@cursor.com>
@openshift-ci

openshift-ci Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mgahagan73
Once this PR has been reviewed and has the lgtm label, please assign geoberle 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

@openshift-ci

openshift-ci Bot commented Sep 3, 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

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.

1 participant