feat(cleanup-sweeper): purge aged soft-deleted directory objects backing role assignments (AROSLSRE-2023) - #6825
Conversation
…le assignments shared-leftovers only deletes an orphaned role assignment once its principal is gone from both the active directory and directory/deletedItems, so it always waits out Entra's 30-day restore window. Subscriptions that keep recreating short-lived service principals with the same role assignments (e.g. e2e-test tooling) build up a role-assignment backlog well before that window closes, since the recycle-bin objects those assignments point to just sit there for 30 days no matter how many times the assignments get recreated. Add an opt-in shared-leftovers step that purges deletedItems entries permanently once they are older than a grace period (default 7 days), but only for service principals/applications that already hold a role assignment in the target subscription. It never scans the tenant's deletedItems at large. It runs before the existing role-assignment delete step so purged objects' assignments become eligible for cleanup in the same run. This needs Directory.ReadWrite.All / Application.ReadWrite.All, well beyond the read-only Graph identity used for discovery, so it is gated behind a separate DIRECTORY_WRITE_AZURE_* credential and defaults to disabled (nil credential) when unset.
There was a problem hiding this comment.
🟡 Changes recommended
The new opt-in purge step’s discovery errors will currently abort the entire shared-leftovers workflow, which is a significant operational risk when the feature is enabled.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds an opt-in cleanup-sweeper shared-leftovers step that can permanently purge aged soft-deleted Entra directory objects (service principals / applications) via Microsoft Graph, so their role assignments become eligible for deletion sooner than Entra’s 30-day recycle-bin window.
Changes:
- Introduces a new
directoryobjectsstep that discovers role-assignment principals, checks for ageddeletedItemsentries, and purges eligible objects. - Wires an explicit, higher-privilege
DIRECTORY_WRITE_AZURE_*credential through CLI options → shared workflow runner → workflow builder to gate the new step. - Updates workflow-builder tests and CI cleanup documentation to cover the opt-in behavior.
File summaries
| File | Description |
|---|---|
| tooling/cleanup-sweeper/pkg/engine/workflows_test.go | Extends workflow builder tests to validate the new optional first step when a directory-write credential is provided. |
| tooling/cleanup-sweeper/pkg/engine/steps/directoryobjects/purge_aged_deleted.go | Adds the new step implementation that discovers and purges aged soft-deleted directory objects. |
| tooling/cleanup-sweeper/pkg/engine/steps/directoryobjects/purge_aged_deleted_test.go | Adds unit tests for step construction/validation and helper behavior. |
| tooling/cleanup-sweeper/pkg/engine/role_assignments_sweeper.go | Adds the optional step to the shared-leftovers workflow and threads the new credential parameter. |
| tooling/cleanup-sweeper/cmd/workflow/shared/run.go | Passes the directory-write credential into the workflow builder. |
| tooling/cleanup-sweeper/cmd/root/options.go | Adds env-driven DIRECTORY_WRITE_AZURE_* credential creation and plumbs it into shared-leftovers execution. |
| docs/ci/cleanup.md | Documents the optional purge behavior, required permissions, and enabling env vars. |
Review details
Suppressed comments (1)
tooling/cleanup-sweeper/pkg/engine/steps/directoryobjects/purge_aged_deleted.go:221
- If Microsoft Graph getByIds fails, this Discover() currently returns an error, which will abort the whole shared-leftovers workflow (discovery failures are fatal in runner.Engine). Since this purge step is optional and ContinueOnError doesn’t affect Discover(), it’s safer operationally to skip the step on Graph errors and let the rest of the cleanup proceed.
activePrincipalIDs, err := resolveActivePrincipalIDs(ctx, s.cfg.GraphClient, principalIDs)
if err != nil {
return nil, fmt.Errorf("failed resolving active principals with Microsoft Graph getByIds: %w", err)
}
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
This branch now treats the new directory-object purge step as best-effort during discovery as well as deletion. |
There was a problem hiding this comment.
🟢 Approval recommended
The change is additive/opt-in with aligned tests and docs, and the remaining feedback is a minor maintainability nit.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
tooling/cleanup-sweeper/pkg/engine/steps/directoryobjects/purge_aged_deleted.go:305
subscriptionIDis accepted bylistRoleAssignmentPrincipalIDsbut never used (the RoleAssignmentsClient is already scoped to the subscription). Consider marking it as intentionally unused (_ string) to avoid implying that the helper filters by subscription ID.
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
4e1123b to
eb5558e
Compare
|
This branch keeps the same discovery fail-open fix and also carries the import order that |
There was a problem hiding this comment.
🟡 Changes recommended
The new DIRECTORY_WRITE_AZURE_* opt-in credential gating logic is not covered by the existing cmd/root options unit tests, increasing regression risk for enabling/disabling this high-privilege step.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Lite
eb5558e to
661f306
Compare
|
This branch now covers the new directory-write opt-in gate in |
There was a problem hiding this comment.
🟡 Changes recommended
The new step’s role-assignment principal discovery currently has a compilation/safety issue (unused subscriptionID and missing subscription-scope filtering) that must be corrected before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Lite
661f306 to
f13e0b7
Compare
|
This branch now applies the same subscription-scope guard to the new directory-object discovery that the existing role-assignment cleanup already uses. The unresolved review thread on |
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces an opt-in but highly destructive Graph directory-write capability (permanent purging of deletedItems) that warrants final human review before merge.
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
/hold |
f13e0b7 to
3a6c3ec
Compare
|
This branch now keeps the new |
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces a new high-privilege Microsoft Graph purge path (permanent directory-object deletion) whose operational safety warrants final human review despite being opt-in and unit-tested.
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
…go (AROSLSRE-2023)
|
/unhold |
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces an opt-in path that can permanently delete Entra directory objects using a high-privilege Graph credential, which warrants final human review and careful tenant validation before merge.
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
…e (AROSLSRE-2023) Per review, Delete() purged deletedItems entries using only the state captured at Discover() time, unlike the existing role-assignment delete step which always re-reads its target immediately before the destructive call. Re-read the deletedItems entry right before purging and bail out if the object was restored, reclassified into a non-purge-eligible type, or no longer meets minAge.
636e83f to
0422e62
Compare
|
This branch now re-reads the deletedItems entry immediately before purging it, mirroring the roleassignments delete step's revalidation pattern. If the object was restored, reclassified into a non-purge-eligible type, or no longer meets |
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces a new high-privilege Microsoft Graph destructive operation (directory-object permanent purge) that can’t be exercised via live integration tests in CI and should receive final human security review before approval.
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mmazur, raelga 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 |
Jira: AROSLSRE-2023
What
Adds an opt-in
cleanup-sweepershared-leftoversstep that permanently purges aged soft-deleted (recycle-bin) service principals/applications from Microsoft Graph, scoped only to principals that already hold a role assignment in the target subscription.Why
shared-leftoversonly deletes an orphaned role assignment once its principal is absent from both the active directory anddirectory/deletedItems, so it always waits out Entra's 30-day restore window. Subscriptions that keep recreating short-lived service principals with the same role assignments (e2e-test tooling) build up a role-assignment backlog well before that window closes, since the assignments just sit there pinned to recycle-bin objects. This is what caused subscription1d3378d3-5a3f-4712-85a1-2485495dfc4bto repeatedly climb back toward its 8000 role-assignment quota limit, most recently requiring a manual purge (7159 → 3351) tracked in AROSLSRE-2002.Testing
go build ./...,go vet ./...andgo test ./...all pass fortooling/cleanup-sweeper, including new unit tests for the step (config validation, defaults,ToTarget/normalizeIDhelpers) and updated workflow-builder tests covering both the opt-in-enabled and opt-in-disabled cases.No integration test was run against a live Graph tenant since the step needs a dedicated
Directory.ReadWrite.All/Application.ReadWrite.Allcredential; that's intentional given the higher privilege involved.Special notes for your reviewer
DIRECTORY_WRITE_AZURE_CLIENT_ID/DIRECTORY_WRITE_AZURE_TENANT_ID/DIRECTORY_WRITE_AZURE_CLIENT_SECRETunset (the default everywhere today), this step is skipped and behavior is unchanged.deletedItemsat large, it only looks up principals that already have a role assignment in the subscription, then checks age against a default 7-day grace period before purging.PR Checklist
docs/ci/cleanup.md)DIRECTORY_WRITE_AZURE_*credential is provisioned for an environment)