e2e cleanup of orphaned service principals and identities #4557

slawande2 · 2026-01-14T21:11:54Z

Which issue this PR addresses:

Fixes ARO-22824

What this PR does / why we need it:

Managed identities, cluster service principals, and role assignments are cleaned up as part of e2e cluster deletion, but this doesn't occur if the e2e job times out and doesn't make it to the cleanup step.
While running miwi E2E tests in the E2E Build and Release pipeline, we observed that managed identities and cluster service principals are not being cleaned up when the E2E job times out. This PR adds cleanup logic specifically for orphaned resource groups from prod e2e clusters and service principals, by creating a dedicated cleanup job in pkg/util/purge.

Test plan for issue:

Added unit tests for orphaned service principals cleanup

Is there any documentation that needs to be updated for this PR?

no

shubhadapaithankar

LGTM

mociarain

Left some thoughts and questions but overall it looks great. This will be a nice change

hack/clean/clean.go

pkg/util/purge/serviceprincipals.go

pkg/util/purge/serviceprincipals_test.go

mociarain · 2026-01-16T10:37:42Z

This LGTM. Just wondering if you tested it with the dry-run mode to see if there were any edge cases that appeared?

slawande2 · 2026-01-16T18:58:36Z

@mociarain I tested the changes in dryRun mode: https://msazure.visualstudio.com/AzureRedHatOpenShift/_build/results?buildId=149649712&view=logs&j=a4f1910f-c367-5697-edcd-724d81cde23b&t=bc103e47-9b88-5c56-fcad-f3f2f0b2ed91
and the only concern I have is that the purgeTTL is set to 3h for the purge pipeline. This would delete all SPs older than 3h, which could delete SPs mid e2e tests. Should we change it to 48h instead?

kimorris27

I took a look at the pipeline run, and I thought it was interesting how there were exactly 100 mock-msi SPs. I think there are more than 100 and we're only getting 100 because the list response is paginated, and we're not checking the other pages.

Some more info here: https://learn.microsoft.com/en-us/graph/paging?tabs=http

TBH I'm not sure if going through all of the pages is worth the trouble. Since the clean pipeline runs on a schedule, we'll get all of the SPs eventually.

I would be cool merging this without any changes, but I wanted to point this out in case anyone else has other thoughts.

mociarain · 2026-01-20T09:56:48Z

the only concern I have is that the purgeTTL is set to 3h for the purge pipeline. This would delete all SPs older than 3h, which could delete SPs mid e2e tests. Should we change it to 48h instead?

Agreed. Bump it to 48hrs

TBH I'm not sure if going through all of the pages is worth the trouble. Since the clean pipeline runs on a schedule, we'll get all of the SPs eventually.

+1 on this. I think we don't need to change it but can you add a comment to it just outlining that this is the case and we left it this way assuming it will eventually clear them out.

pkg/util/purge/serviceprincipals.go

…tion sets deletion

slawande2 · 2026-01-20T22:08:39Z

I realized after running the pipeline in dry run mode that using rc.graphClient.Applications().ByApplicationId(objectID).Delete(ctx, nil) would only delete the mock-msi SP and v4-e2e-* SPs, but not Disk Encryption Sets. These need to be deleted using rc.graphClient.ServicePrincipals().ByServicePrincipalId(objectID).Delete(ctx, nil) as no application object exists to delete for DESs. And if we use ServicePrincipals().Delete() for everything, the SPs would get deleted but leave orphaned application registration.

…palId()

slawande2 · 2026-01-22T20:16:58Z

I'm still getting an error for deleting disk encryption sets in dry run mode.

slawande2 · 2026-01-23T22:22:58Z

For disk encryption sets, Caden suggested deleting the resource groups that contain these DES instead of deleting individual DESs, which makes sense to me. There's already existing logic for cleaning up resourcegroups with aro-v4-e2e and v4-e2e resourceGroupDeletePrefixes which should work but isn't? there are resource groups as old as 2 months that haven't been cleaned up.

kimorris27 · 2026-01-26T14:38:21Z

For disk encryption sets, Caden suggested deleting the resource groups that contain these DES instead of deleting individual DESs, which makes sense to me. There's already existing logic for cleaning up resourcegroups with aro-v4-e2e and v4-e2e resourceGroupDeletePrefixes which should work but isn't? there are resource groups as old as 2 months that haven't been cleaned up.

The existing code does clean up RGs with those prefixes, but it also checks for the purge tag. I looked in the E2E sub just now, and it seems like recent CI E2E RGs don't have the purge tag... but I'm not sure why that would be.

I think we'll be fine once your changes merge though, since we won't be checking that tag anymore.

slawande2 · 2026-01-26T17:55:48Z

/azp run ci

azure-pipelines · 2026-01-26T17:56:02Z

Azure Pipelines successfully started running 1 pipeline(s).

cadenmarchese

This is functional as-is, but I have a few suggestions for readability.

cadenmarchese · 2026-01-27T18:38:23Z

pkg/util/purge/serviceprincipals.go

+	return nil
+}
+
+func (rc *ResourceCleaner) cleanServicePrincipals(ctx context.Context, prefix string, ttl time.Duration) error {


I would consider breaking this function up at this point to make it easier on the reader. I see cleanServicePrincipals currently used for the following:

Get entra applications matching x filter

Evaluate whether or not they should be deleted

Actually delete them

In my mind these are 3 separate functions - wdyt?

I agree, this function has three distinct responsibilities and would be much cleaner if separated. I've divided it into three functions:

getApplicationsByPrefix - to list applications with prefix aro-v4-e2e or mock-msi-

shouldDelete to check for deletion eligibility

cleanServicePrincipals - to actually perform the deletion

cadenmarchese · 2026-01-27T18:41:27Z

pkg/util/purge/serviceprincipals.go

+		if app.GetDisplayName() != nil {
+			displayName = *app.GetDisplayName()
+		}
+
+		if app.GetAppId() != nil {
+			appID = *app.GetAppId()
+		}
+
+		if app.GetId() != nil {
+			objectID = *app.GetId()
+		}


It seems that we need displayName, appID, and objectID to execute the remaining code. Should we simplify these three if to check all 3 at the same time and continue if any of them come back missing?

Suggested change

if app.GetDisplayName() != nil {

displayName = *app.GetDisplayName()

}

if app.GetAppId() != nil {

appID = *app.GetAppId()

}

if app.GetId() != nil {

objectID = *app.GetId()

}

if app.GetDisplayName() == nil || app.GetAppId() == nil || app.GetId() == nil {

rc.log.Warnf("SKIP: Application missing required fields")

continue

}

displayName := *app.GetDisplayName()

appID := *app.GetAppId()

objectID := *app.GetId()

Then, we can get rid of line 80.

cadenmarchese · 2026-01-27T18:53:13Z

pkg/util/purge/serviceprincipals.go

+
+		if age < ttl {
+			rc.log.Debugf("SKIP '%s': Age %v < TTL %v", displayName, age.Round(time.Hour), ttl)
+			continue


We should execute this age check first in the for loop (unless there's something specific about the isMockMSI that changes that behavior that I'm missing).

Related to that - I see we are checking age in another spot, on line 102-104:

} else if createdDateTime == nil { rc.log.Warnf("SKIP '%s' (objectID: %s): No createdDateTime", displayName, objectID) continue }

Can we merge this behavior into a single, top-level age check that does the continue if the SP is not yet eligible to be deleted? It seems that it doesn't matter if it's a mock MSI, or a CSP - if it's not old enough, we'll leave it alone.

if we merge this behavior with something like:

if createdDateTime == nil || time.Since(*createdDateTime) < ttl { rc.log.Infof( "SKIP '%s' (objectID: %s): createdDateTime missing or age < TTL", displayName, objectID, ) continue }

if createdDateTime is nil, the time.Since(*createdDateTime) will panic when it tries to dereference the nil pointer.

cadenmarchese · 2026-01-27T19:04:44Z

pkg/util/purge/serviceprincipals.go

+
+		createdDateTime = app.GetCreatedDateTime()
+
+		isMockMSI := strings.HasPrefix(displayName, "mock-msi-")


We use this var twice, but always as !isMockMSI, which makes things harder to read. Should we replace this var with an isE2eClusterServicePrincipal instead so we can wrap our CSP-specific logic?

For example:

if isE2eClusterServicePrincipal { resourceGroupName := determineResourceGroupName(displayName) shouldKeep, reason := rc.checkSPNeededBasedOnRGStatus(ctx, resourceGroupName, ttl) if shouldKeep { rc.log.Infof("SKIP '%s': %s", displayName, reason) continue }

It seems that we don't have any special handling for mock MSIs - only CSPs, so we can have a dedicated if for CSPs. And then we can avoid the else if stuff that's tougher to read.

cadenmarchese · 2026-01-27T19:19:10Z

pkg/util/purge/serviceprincipals.go

+	return nil
+}
+
+func determineResourceGroupName(displayName string) string {


Conversely, we probably don't need this function. We can just use strings.TrimPrefix(displayName, "aro-") inline.

e2e cleanup of orphaned service principals and identities

45077ec

slawande2 requested review from bennerv, cadenmarchese, hawkowl, hlipsig, jharrington22, kimorris27, mociarain, mrWinston, pepedocs, rogbas, sankur-codes, tiguelu, tsatam, wanghaoran1988 and yjst2012 as code owners January 14, 2026 21:11

slawande2 added chainsaw Pull requests or issues owned by Team Chainsaw ready-for-review labels Jan 14, 2026

shubhadapaithankar previously approved these changes Jan 14, 2026

View reviewed changes

mociarain requested changes Jan 15, 2026

View reviewed changes

slawande2 dismissed shubhadapaithankar’s stale review via 4d0ecb0 January 15, 2026 21:34

apply suggestions from code review

4d0ecb0

kimorris27 reviewed Jan 19, 2026

View reviewed changes

cadenmarchese reviewed Jan 20, 2026

View reviewed changes

pkg/util/purge/serviceprincipals.go Outdated Show resolved Hide resolved

slawande2 added 2 commits January 20, 2026 12:45

check for prefix and suffix for disk encryption sets deletion

1648f58

use ServicePrincipals().ByServicePrincipalId().Delete for disk encryp…

617f840

…tion sets deletion

slawande2 force-pushed the slawande/ARO-22824/e2e-cleanup branch from a4d2cef to 1648f58 Compare January 20, 2026 23:03

delete disk-encryption-sets using ServicePrincipals().ByServicePrinci…

4f9d0ca

…palId()

remove DES cleanup, use resource group cascade deletion

1d1d658

cadenmarchese reviewed Jan 27, 2026

View reviewed changes

apply suggestions from code review

0a2264a


		createdDateTime = app.GetCreatedDateTime()

		isMockMSI := strings.HasPrefix(displayName, "mock-msi-")

e2e cleanup of orphaned service principals and identities #4557

Are you sure you want to change the base?

e2e cleanup of orphaned service principals and identities #4557

Conversation

slawande2 commented Jan 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue this PR addresses:

What this PR does / why we need it:

Test plan for issue:

Is there any documentation that needs to be updated for this PR?

Uh oh!

shubhadapaithankar left a comment

Choose a reason for hiding this comment

Uh oh!

mociarain left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mociarain commented Jan 16, 2026

Uh oh!

slawande2 commented Jan 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kimorris27 left a comment

Choose a reason for hiding this comment

Uh oh!

mociarain commented Jan 20, 2026

Uh oh!

Uh oh!

slawande2 commented Jan 20, 2026

Uh oh!

slawande2 commented Jan 22, 2026

Uh oh!

slawande2 commented Jan 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kimorris27 commented Jan 26, 2026

Uh oh!

slawande2 commented Jan 26, 2026

Uh oh!

azure-pipelines bot commented Jan 26, 2026

Uh oh!

cadenmarchese left a comment

Choose a reason for hiding this comment

Uh oh!

cadenmarchese Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

slawande2 Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

cadenmarchese Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

cadenmarchese Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

slawande2 Jan 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cadenmarchese Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

cadenmarchese Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

slawande2 commented Jan 14, 2026 •

edited

Loading

slawande2 commented Jan 16, 2026 •

edited

Loading

slawande2 commented Jan 23, 2026 •

edited

Loading

slawande2 Jan 28, 2026 •

edited

Loading