From 7820d27f4115f15407aa5c1904114047cd0fa4ff Mon Sep 17 00:00:00 2001 From: Rael Garcia Date: Fri, 4 Sep 2026 07:04:40 +0000 Subject: [PATCH 1/4] cleanup-sweeper: purge aged soft-deleted directory objects backing role 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. --- docs/ci/cleanup.md | 2 + tooling/cleanup-sweeper/cmd/root/options.go | 77 +++- .../cmd/workflow/shared/run.go | 7 + .../pkg/engine/role_assignments_sweeper.go | 68 ++- .../directoryobjects/purge_aged_deleted.go | 397 ++++++++++++++++++ .../purge_aged_deleted_test.go | 214 ++++++++++ .../pkg/engine/workflows_test.go | 35 ++ 7 files changed, 765 insertions(+), 35 deletions(-) create mode 100644 tooling/cleanup-sweeper/pkg/engine/steps/directoryobjects/purge_aged_deleted.go create mode 100644 tooling/cleanup-sweeper/pkg/engine/steps/directoryobjects/purge_aged_deleted_test.go diff --git a/docs/ci/cleanup.md b/docs/ci/cleanup.md index cb242f8fa0f..a8d2c717d34 100644 --- a/docs/ci/cleanup.md +++ b/docs/ci/cleanup.md @@ -94,6 +94,8 @@ The `cleanup-sweeper` `shared-leftovers` workflow runs per environment. Alongsid To resolve the principals behind orphaned role assignments, `shared-leftovers` reads the Microsoft Graph directory, which requires `Directory.Read.All`. A role assignment is deleted only when its principal is absent from both the active directory and `directory/deletedItems`; assignments for soft-deleted principals remain intact during Entra's restore window. If either directory lookup fails, discovery fails closed without deleting assignments. The per-environment ARM identity (`VAULT_SECRET_PROFILE`) usually lacks that tenant-wide grant, so the step exports a dedicated Graph identity from `GRAPH_SECRET_PROFILE` (the dev bot, which holds `Directory.Read.All`) whenever its mounted profile differs from the ARM profile. When the two resolve to the same profile, no separate Graph credential is used and the ARM identity serves both. The sweeper binary reads that dedicated identity from `GRAPH_AZURE_CLIENT_ID` / `GRAPH_AZURE_TENANT_ID` / `GRAPH_AZURE_CLIENT_SECRET`, which the `aro-hcp-deprovision-cleanup-sweeper` step in `openshift/release` exports from the mounted Graph profile. +Because `directory/deletedItems` protects a principal for the full 30-day Entra restore window, a subscription that keeps recreating short-lived service principals with the same role assignments (for example, e2e-test tooling) can build up a large backlog of role assignments pinned to soft-deleted principals well before that window closes. `shared-leftovers` can optionally purge those principals from `deletedItems` itself, once they are older than a grace period, so their role assignments become eligible for deletion in the same run. This is a separate, higher-privilege, opt-in step: it needs `Directory.ReadWrite.All` / `Application.ReadWrite.All`, well beyond the read-only Graph identity used for discovery, and it only ever acts on service principals or applications that already hold a role assignment in the target subscription (it never scans the tenant's `deletedItems` at large). It is enabled by setting `DIRECTORY_WRITE_AZURE_CLIENT_ID` / `DIRECTORY_WRITE_AZURE_TENANT_ID` / `DIRECTORY_WRITE_AZURE_CLIENT_SECRET`; when unset (the default), this step is skipped entirely and behavior is unchanged. The default grace period is 7 days from the object's `deletedDateTime`, configurable in code via `PurgeAgedDeletedStepConfig.MinAge`. + For `cleanup-sweeper` `rg-ordered`, candidate resource groups are chosen using `tooling/cleanup-sweeper/resourcegroups.policy.yaml`. Discovery treats the `createdAt` tag (RFC3339 timestamp on the resource group) as required for any `action: delete` rule: groups without a parseable tag are not candidates. The policy excludes long-lived slot-managed identity pools whose resource-group diff --git a/tooling/cleanup-sweeper/cmd/root/options.go b/tooling/cleanup-sweeper/cmd/root/options.go index 53f59fca70d..09dd12886df 100644 --- a/tooling/cleanup-sweeper/cmd/root/options.go +++ b/tooling/cleanup-sweeper/cmd/root/options.go @@ -101,7 +101,13 @@ type completedOptions struct { // dedicated Graph identity is supplied via the GRAPH_AZURE_* environment // variables. GraphCredential azcore.TokenCredential - Policy *policy.Policy + // DirectoryWriteCredential backs the aged-deleted-directory-object purge + // step's Graph client. Unlike GraphCredential, it is opt-in: it is only + // set when a dedicated DIRECTORY_WRITE_AZURE_* identity is configured, and + // never falls back to AzureCredential or GraphCredential, since this + // credential needs materially higher (directory-write) privilege. + DirectoryWriteCredential azcore.TokenCredential + Policy *policy.Policy Workflow WorkflowMode @@ -185,26 +191,32 @@ func (o *ValidatedOptions) Complete(_ context.Context) (*Options, error) { // This keeps a partial GRAPH_AZURE_* configuration from failing workflows // that never touch Graph (for example, rg-ordered). var graphCred azcore.TokenCredential = cred + var directoryWriteCred azcore.TokenCredential if o.workflow == WorkflowSharedLeftovers { graphCred, err = newGraphCredential(cred) if err != nil { return nil, err } + directoryWriteCred, err = newDirectoryWriteCredential() + if err != nil { + return nil, err + } } return &Options{ completedOptions: &completedOptions{ - AzureCredential: cred, - GraphCredential: graphCred, - Policy: o.policy, - Workflow: o.workflow, - SubscriptionID: subscriptionID, - PolicyFile: policyFile, - ReferenceTime: referenceTime, - DryRun: o.DryRun, - Wait: o.Wait, - Parallelism: o.Parallelism, - ResourceGroups: resourceGroups, + AzureCredential: cred, + GraphCredential: graphCred, + DirectoryWriteCredential: directoryWriteCred, + Policy: o.policy, + Workflow: o.workflow, + SubscriptionID: subscriptionID, + PolicyFile: policyFile, + ReferenceTime: referenceTime, + DryRun: o.DryRun, + Wait: o.Wait, + Parallelism: o.Parallelism, + ResourceGroups: resourceGroups, }, }, nil } @@ -240,12 +252,13 @@ func (o *Options) Run(ctx context.Context) error { } case WorkflowSharedLeftovers: err := sharedworkflow.Run(ctx, sharedworkflow.RunOptions{ - SubscriptionID: o.SubscriptionID, - AzureCredential: o.AzureCredential, - GraphCredential: o.GraphCredential, - DryRun: o.DryRun, - Wait: o.Wait, - Parallelism: o.Parallelism, + SubscriptionID: o.SubscriptionID, + AzureCredential: o.AzureCredential, + GraphCredential: o.GraphCredential, + DirectoryWriteCredential: o.DirectoryWriteCredential, + DryRun: o.DryRun, + Wait: o.Wait, + Parallelism: o.Parallelism, }) if err != nil { return err @@ -283,6 +296,34 @@ func newGraphCredential(fallback azcore.TokenCredential) (azcore.TokenCredential return cred, nil } +// newDirectoryWriteCredential returns the credential used by the +// aged-deleted-directory-object purge step, which needs directory-write +// permission (Directory.ReadWrite.All / Application.ReadWrite.All). Unlike +// newGraphCredential, it never falls back to another credential: the step is +// materially more privileged (it permanently deletes directory objects), so +// it is only enabled when a dedicated identity is explicitly configured via +// the DIRECTORY_WRITE_AZURE_* environment variables. Returns a nil credential +// (and nil error) when unset, which the shared-leftovers workflow treats as +// "omit this step". +func newDirectoryWriteCredential() (azcore.TokenCredential, error) { + tenantID := strings.TrimSpace(os.Getenv("DIRECTORY_WRITE_AZURE_TENANT_ID")) + clientID := strings.TrimSpace(os.Getenv("DIRECTORY_WRITE_AZURE_CLIENT_ID")) + clientSecret := strings.TrimSpace(os.Getenv("DIRECTORY_WRITE_AZURE_CLIENT_SECRET")) + + if tenantID == "" && clientID == "" && clientSecret == "" { + return nil, nil + } + if tenantID == "" || clientID == "" || clientSecret == "" { + return nil, fmt.Errorf("DIRECTORY_WRITE_AZURE_TENANT_ID, DIRECTORY_WRITE_AZURE_CLIENT_ID and DIRECTORY_WRITE_AZURE_CLIENT_SECRET must all be set to enable the aged-deleted-directory-object purge step") + } + + cred, err := azidentity.NewClientSecretCredential(tenantID, clientID, clientSecret, nil) + if err != nil { + return nil, fmt.Errorf("failed to create directory-write credential: %w", err) + } + return cred, nil +} + func parseWorkflowMode(raw string) (WorkflowMode, error) { switch WorkflowMode(raw) { case WorkflowRGOrdered: diff --git a/tooling/cleanup-sweeper/cmd/workflow/shared/run.go b/tooling/cleanup-sweeper/cmd/workflow/shared/run.go index 4ddbb9687f8..a66e9aad8f3 100644 --- a/tooling/cleanup-sweeper/cmd/workflow/shared/run.go +++ b/tooling/cleanup-sweeper/cmd/workflow/shared/run.go @@ -32,6 +32,12 @@ type RunOptions struct { // GraphCredential is used exclusively for Microsoft Graph directory reads. // When nil it defaults to AzureCredential. GraphCredential azcore.TokenCredential + // DirectoryWriteCredential backs a second Graph client used only by the + // aged-deleted-directory-object purge step, which requires + // Directory.ReadWrite.All / Application.ReadWrite.All - materially higher + // privilege than GraphCredential needs. When nil, that step is omitted + // entirely rather than reusing GraphCredential or AzureCredential. + DirectoryWriteCredential azcore.TokenCredential DryRun bool Wait bool @@ -51,6 +57,7 @@ func Run(ctx context.Context, opts RunOptions) error { opts.SubscriptionID, opts.AzureCredential, opts.GraphCredential, + opts.DirectoryWriteCredential, cleanupengine.WorkflowOptions{ DryRun: opts.DryRun, Wait: opts.Wait, diff --git a/tooling/cleanup-sweeper/pkg/engine/role_assignments_sweeper.go b/tooling/cleanup-sweeper/pkg/engine/role_assignments_sweeper.go index 85e6db962fd..d13ce3ea0f7 100644 --- a/tooling/cleanup-sweeper/pkg/engine/role_assignments_sweeper.go +++ b/tooling/cleanup-sweeper/pkg/engine/role_assignments_sweeper.go @@ -25,6 +25,7 @@ import ( "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armresources" "github.com/Azure/ARO-HCP/tooling/cleanup-sweeper/pkg/engine/runner" + directoryobjectsteps "github.com/Azure/ARO-HCP/tooling/cleanup-sweeper/pkg/engine/steps/directoryobjects" kvsteps "github.com/Azure/ARO-HCP/tooling/cleanup-sweeper/pkg/engine/steps/keyvault" roleassignmentsteps "github.com/Azure/ARO-HCP/tooling/cleanup-sweeper/pkg/engine/steps/roleassignments" ) @@ -32,6 +33,7 @@ import ( const ( orphanedRoleAssignmentStepRetries = 3 orphanedVaultStepRetries = 3 + agedDeletedObjectStepRetries = 3 ) // RoleAssignmentsSweeperWorkflow builds the shared-leftovers cleanup workflow. @@ -40,11 +42,20 @@ const ( // groups). graphCredential is used exclusively for the Microsoft Graph // directory reads performed by the orphaned role-assignment step; when nil it // defaults to credential, preserving single-identity behavior. +// +// directoryWriteCredential, when non-nil, backs a second Graph client used +// only by the aged-deleted-directory-object purge step. That step permanently +// deletes directory objects (Directory.ReadWrite.All / Application.ReadWrite.All), +// a materially higher privilege than the read-only access graphCredential +// needs, so the two are kept separate rather than reusing graphCredential. +// When nil, the aged-deleted-directory-object purge step is omitted entirely - +// it is opt-in, since most callers won't have an identity holding that grant. func RoleAssignmentsSweeperWorkflow( _ context.Context, subscriptionID string, credential azcore.TokenCredential, graphCredential azcore.TokenCredential, + directoryWriteCredential azcore.TokenCredential, opts WorkflowOptions, ) (*runner.Engine, error) { if strings.TrimSpace(subscriptionID) == "" { @@ -85,26 +96,49 @@ func RoleAssignmentsSweeperWorkflow( return resp.Success, nil } + steps := []runner.Step{} + + if directoryWriteCredential != nil { + directoryWriteGraphClient, err := roleassignmentsteps.NewGraphClient(directoryWriteCredential) + if err != nil { + return nil, fmt.Errorf("failed to create directory-write graph client: %w", err) + } + // Runs before the orphaned-role-assignment step: purging an aged + // deletedItems object here means its role assignment is picked up as + // orphaned by that step in this very same run instead of waiting for + // Entra's 30-day recycle-bin timer. + steps = append(steps, directoryobjectsteps.MustNewPurgeAgedDeletedStep(directoryobjectsteps.PurgeAgedDeletedStepConfig{ + RoleAssignmentsClient: roleAssignmentsClient, + GraphClient: directoryWriteGraphClient, + SubscriptionID: subscriptionID, + Name: "Purge aged deleted directory objects", + Retries: agedDeletedObjectStepRetries, + ContinueOnError: true, + })) + } + + steps = append(steps, + roleassignmentsteps.MustNewDeleteOrphanedStep(roleassignmentsteps.DeleteOrphanedStepConfig{ + RoleAssignmentsClient: roleAssignmentsClient, + GraphClient: graphClient, + SubscriptionID: subscriptionID, + Name: "Delete orphaned role assignments", + Retries: orphanedRoleAssignmentStepRetries, + ContinueOnTargetDeleteError: true, + }), + kvsteps.MustNewPurgeOrphanedDeletedStep(kvsteps.PurgeOrphanedDeletedStepConfig{ + VaultsClient: vaultsClient, + ResourceGroupExists: resourceGroupExists, + Name: "Purge orphaned soft-deleted Key Vaults", + Retries: orphanedVaultStepRetries, + ContinueOnError: true, + }), + ) + return &runner.Engine{ Parallelism: opts.Parallelism, DryRun: opts.DryRun, Wait: opts.Wait, - Steps: []runner.Step{ - roleassignmentsteps.MustNewDeleteOrphanedStep(roleassignmentsteps.DeleteOrphanedStepConfig{ - RoleAssignmentsClient: roleAssignmentsClient, - GraphClient: graphClient, - SubscriptionID: subscriptionID, - Name: "Delete orphaned role assignments", - Retries: orphanedRoleAssignmentStepRetries, - ContinueOnTargetDeleteError: true, - }), - kvsteps.MustNewPurgeOrphanedDeletedStep(kvsteps.PurgeOrphanedDeletedStepConfig{ - VaultsClient: vaultsClient, - ResourceGroupExists: resourceGroupExists, - Name: "Purge orphaned soft-deleted Key Vaults", - Retries: orphanedVaultStepRetries, - ContinueOnError: true, - }), - }, + Steps: steps, }, nil } diff --git a/tooling/cleanup-sweeper/pkg/engine/steps/directoryobjects/purge_aged_deleted.go b/tooling/cleanup-sweeper/pkg/engine/steps/directoryobjects/purge_aged_deleted.go new file mode 100644 index 00000000000..7e4afa1a80a --- /dev/null +++ b/tooling/cleanup-sweeper/pkg/engine/steps/directoryobjects/purge_aged_deleted.go @@ -0,0 +1,397 @@ +// Copyright 2026 Microsoft Corporation +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Package directoryobjects permanently purges Microsoft Graph directory +// objects (service principals / application registrations) that have sat in +// Entra's deletedItems recycle bin long enough that a restore is implausible. +// +// This closes the gap the roleassignments package's SAFETY CONTRACT +// deliberately leaves open: that step never deletes a role assignment while +// its principal is still recoverable from deletedItems, so those assignments +// (and the directory quota they consume) only ever clear once Entra's own +// 30-day recycle-bin timer expires the object. This step reclaims that +// quota sooner, on a much shorter, still-safe grace period, and only for +// principals that already hold a role assignment in the target subscription - +// it never scans the tenant's directory blindly. +package directoryobjects + +import ( + "context" + "errors" + "fmt" + "net/http" + "strings" + "time" + + "github.com/go-logr/logr" + msgraphsdk "github.com/microsoftgraph/msgraph-sdk-go" + graphdirectoryobjects "github.com/microsoftgraph/msgraph-sdk-go/directoryobjects" + "github.com/microsoftgraph/msgraph-sdk-go/models" + graphodataerrors "github.com/microsoftgraph/msgraph-sdk-go/models/odataerrors" + + "k8s.io/apimachinery/pkg/util/sets" + + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/authorization/armauthorization/v3" + + "github.com/Azure/ARO-HCP/tooling/cleanup-sweeper/pkg/engine/runner" + "github.com/Azure/ARO-HCP/tooling/cleanup-sweeper/pkg/engine/steps/common" +) + +const ( + // ServicePrincipalResourceType is the runner.Target resource type reported + // for a permanently purged deleted-items service principal. + ServicePrincipalResourceType = "directory.deletedItems/servicePrincipal" + // ApplicationResourceType is the runner.Target resource type reported for + // a permanently purged deleted-items application registration. + ApplicationResourceType = "directory.deletedItems/application" + + // DefaultMinAge is how long a directory object must have sat in + // deletedItems before this step will purge it. Kept well short of Entra's + // 30-day auto-expiry, so this step (not the tenant-wide timer) is what + // reclaims role-assignment quota, while still leaving a real window to + // notice and restore an accidental deletion. + DefaultMinAge = 7 * 24 * time.Hour + + graphGetByIDsBatchSize = 1000 +) + +// PurgeAgedDeletedStepConfig configures aged-deleted-directory-object purge +// behavior. +type PurgeAgedDeletedStepConfig struct { + RoleAssignmentsClient *armauthorization.RoleAssignmentsClient + // GraphClient must be backed by a credential holding directory-write + // permission (e.g. Application.ReadWrite.All / Directory.ReadWrite.All), + // unlike the read-only credential the roleassignments step uses. Keep + // these as two distinct client/credential pairs, mirroring how + // RoleAssignmentsClient and the roleassignments package's GraphClient are + // already split by permission scope. + GraphClient *msgraphsdk.GraphServiceClient + SubscriptionID string + + // MinAge overrides DefaultMinAge when non-zero. + MinAge time.Duration + // Now overrides time.Now for tests. + Now func() time.Time + + Name string + Retries int + ContinueOnError bool + Verify runner.VerifyFn +} + +type purgeAgedDeletedStep struct { + cfg PurgeAgedDeletedStepConfig + name string + retries int + continueOnError bool + verify runner.VerifyFn + minAge time.Duration + now func() time.Time +} + +var _ runner.Step = (*purgeAgedDeletedStep)(nil) + +// NewPurgeAgedDeletedStep builds the aged-deleted-directory-object purge step. +func NewPurgeAgedDeletedStep(cfg PurgeAgedDeletedStepConfig) (runner.Step, error) { + if cfg.RoleAssignmentsClient == nil { + return nil, fmt.Errorf("role assignments client is required") + } + if cfg.GraphClient == nil { + return nil, fmt.Errorf("graph client is required") + } + if strings.TrimSpace(cfg.SubscriptionID) == "" { + return nil, fmt.Errorf("subscription ID is required") + } + + stepName := cfg.Name + if strings.TrimSpace(stepName) == "" { + stepName = "Purge aged deleted directory objects" + } + + minAge := cfg.MinAge + if minAge <= 0 { + minAge = DefaultMinAge + } + + now := cfg.Now + if now == nil { + now = time.Now + } + + return &purgeAgedDeletedStep{ + cfg: cfg, + name: stepName, + retries: cfg.Retries, + continueOnError: cfg.ContinueOnError, + verify: cfg.Verify, + minAge: minAge, + now: now, + }, nil +} + +// MustNewPurgeAgedDeletedStep builds the step and panics on invalid config. +func MustNewPurgeAgedDeletedStep(cfg PurgeAgedDeletedStepConfig) runner.Step { + step, err := NewPurgeAgedDeletedStep(cfg) + if err != nil { + panic(err) + } + return step +} + +func (s *purgeAgedDeletedStep) Name() string { + return s.name +} + +func (s *purgeAgedDeletedStep) RetryLimit() int { + if s.retries < runner.DefaultRetries { + return runner.DefaultRetries + } + return s.retries +} + +func (s *purgeAgedDeletedStep) ContinueOnError() bool { + return s.continueOnError +} + +func (s *purgeAgedDeletedStep) Verify(ctx context.Context) error { + if s.verify == nil { + return nil + } + return s.verify(ctx) +} + +// deletedObjectRecord is a candidate directory object discovered in +// deletedItems along with the metadata needed to decide whether it is safe +// and old enough to purge. +type deletedObjectRecord struct { + ID string + DisplayName string + ResourceType string + DeletedDateTime *time.Time +} + +func (r deletedObjectRecord) ToTarget() runner.Target { + name := r.DisplayName + if name == "" { + name = r.ID + } + return runner.Target{ + ID: r.ID, + Name: name, + Type: r.ResourceType, + } +} + +func (s *purgeAgedDeletedStep) Discover(ctx context.Context) ([]runner.Target, error) { + logger, err := logr.FromContext(ctx) + if err != nil { + panic(err) + } + skipReporter := common.NewDiscoverySkipReporter(s.Name()) + defer skipReporter.Flush(logger) + + // 1) Collect the distinct principal IDs holding a role assignment in this + // subscription. This step never scans the tenant's directory blindly - it + // only ever considers principals already tied to this subscription's own + // role-assignment quota pressure. + principalIDs, err := listRoleAssignmentPrincipalIDs(ctx, s.cfg.RoleAssignmentsClient, s.cfg.SubscriptionID, logger, skipReporter) + if err != nil { + return nil, fmt.Errorf("failed listing role assignment principal IDs: %w", err) + } + if principalIDs.Len() == 0 { + return nil, nil + } + + // 2) Resolve which of those principals are still active. Active + // principals are never purge candidates. + 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) + } + candidatePrincipalIDs := principalIDs.Difference(activePrincipalIDs) + + // 3) For each inactive principal, look it up in deletedItems. Only a + // principal Graph itself reports as soft-deleted, of a purge-eligible + // type, and older than minAge becomes a purge target. Anything not found + // in deletedItems either (already gone, or never a directory principal) + // is left alone - there is nothing here for this step to purge. + targets := make([]runner.Target, 0) + for _, principalID := range sets.List(candidatePrincipalIDs) { + record, found, err := lookupDeletedDirectoryObject(ctx, s.cfg.GraphClient, principalID) + if err != nil { + skipReporter.Record(logger, "deleted_item_lookup_failed", "principalID", principalID, "error", err) + continue + } + if !found { + continue + } + if record.DeletedDateTime == nil { + skipReporter.Record(logger, "deleted_item_missing_deleted_date_time", "principalID", principalID) + continue + } + age := s.now().Sub(*record.DeletedDateTime) + if age < s.minAge { + continue + } + targets = append(targets, record.ToTarget()) + } + + if len(targets) == 0 { + logger.Info( + "No aged deleted directory objects discovered", + "principalsScanned", principalIDs.Len(), + "minAge", s.minAge.String(), + ) + return targets, nil + } + + logger.Info( + "Discovered aged deleted directory objects", + "count", len(targets), + "principalsScanned", principalIDs.Len(), + "minAge", s.minAge.String(), + ) + + return targets, nil +} + +func (s *purgeAgedDeletedStep) Delete(ctx context.Context, target runner.Target, _ bool) error { + err := s.cfg.GraphClient.Directory().DeletedItems().ByDirectoryObjectId(target.ID).Delete(ctx, nil) + if err != nil { + if isGraphNotFoundError(err) { + return nil + } + return fmt.Errorf("failed to purge deleted directory object %q: %w", target.ID, err) + } + return nil +} + +func listRoleAssignmentPrincipalIDs( + ctx context.Context, + roleAssignmentsClient *armauthorization.RoleAssignmentsClient, + subscriptionID string, + logger logr.Logger, + skipReporter *common.DiscoverySkipReporter, +) (sets.Set[string], error) { + pager := roleAssignmentsClient.NewListForSubscriptionPager(nil) + principalIDs := sets.New[string]() + + for pager.More() { + page, err := pager.NextPage(ctx) + if err != nil { + return nil, fmt.Errorf("failed listing role assignments: %w", err) + } + for _, roleAssignment := range page.Value { + if roleAssignment == nil || roleAssignment.Properties == nil || roleAssignment.Properties.PrincipalID == nil { + skipReporter.Record(logger, "invalid_role_assignment_payload") + continue + } + principalID := normalizeID(*roleAssignment.Properties.PrincipalID) + if principalID == "" { + skipReporter.Record(logger, "missing_principal_id") + continue + } + principalIDs.Insert(principalID) + } + } + + return principalIDs, nil +} + +func resolveActivePrincipalIDs( + ctx context.Context, + graphClient *msgraphsdk.GraphServiceClient, + principalIDs sets.Set[string], +) (sets.Set[string], error) { + resolved := sets.New[string]() + ids := sets.List(principalIDs) + for start := 0; start < len(ids); start += graphGetByIDsBatchSize { + end := min(start+graphGetByIDsBatchSize, len(ids)) + body := graphdirectoryobjects.NewGetByIdsPostRequestBody() + body.SetIds(ids[start:end]) + + response, err := graphClient.DirectoryObjects().GetByIds().PostAsGetByIdsPostResponse(ctx, body, nil) + if err != nil { + return nil, err + } + if response == nil { + continue + } + for _, object := range response.GetValue() { + if object == nil || object.GetId() == nil { + continue + } + resolved.Insert(normalizeID(*object.GetId())) + } + } + return resolved, nil +} + +// lookupDeletedDirectoryObject fetches a single deletedItems entry and +// classifies it into a purge-eligible resource type. Only service principals +// and applications are purge-eligible; any other recovered type (for example +// a soft-deleted user or group) is reported as not found so the caller never +// purges it. +func lookupDeletedDirectoryObject( + ctx context.Context, + graphClient *msgraphsdk.GraphServiceClient, + principalID string, +) (deletedObjectRecord, bool, error) { + object, err := graphClient.Directory().DeletedItems().ByDirectoryObjectId(principalID).Get(ctx, nil) + if err != nil { + if isGraphNotFoundError(err) { + return deletedObjectRecord{}, false, nil + } + return deletedObjectRecord{}, false, err + } + if object == nil || object.GetId() == nil { + return deletedObjectRecord{}, false, fmt.Errorf("deleted item %q was returned without a valid ID", principalID) + } + + var resourceType string + var displayName string + switch v := object.(type) { + case models.ServicePrincipalable: + resourceType = ServicePrincipalResourceType + if v.GetDisplayName() != nil { + displayName = *v.GetDisplayName() + } + case models.Applicationable: + resourceType = ApplicationResourceType + if v.GetDisplayName() != nil { + displayName = *v.GetDisplayName() + } + default: + // Not a type this step is willing to purge (e.g. a soft-deleted user + // or group holding a stale role assignment). + return deletedObjectRecord{}, false, nil + } + + record := deletedObjectRecord{ + ID: normalizeID(*object.GetId()), + DisplayName: displayName, + ResourceType: resourceType, + DeletedDateTime: object.GetDeletedDateTime(), + } + return record, true, nil +} + +func isGraphNotFoundError(err error) bool { + var odataErr *graphodataerrors.ODataError + return errors.As(err, &odataErr) && odataErr.ResponseStatusCode == http.StatusNotFound +} + +func normalizeID(raw string) string { + return strings.ToLower(strings.TrimSpace(raw)) +} diff --git a/tooling/cleanup-sweeper/pkg/engine/steps/directoryobjects/purge_aged_deleted_test.go b/tooling/cleanup-sweeper/pkg/engine/steps/directoryobjects/purge_aged_deleted_test.go new file mode 100644 index 00000000000..a961b0f5781 --- /dev/null +++ b/tooling/cleanup-sweeper/pkg/engine/steps/directoryobjects/purge_aged_deleted_test.go @@ -0,0 +1,214 @@ +// Copyright 2026 Microsoft Corporation +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package directoryobjects + +import ( + "net/http" + "testing" + "time" + + msgraphsdk "github.com/microsoftgraph/msgraph-sdk-go" + graphodataerrors "github.com/microsoftgraph/msgraph-sdk-go/models/odataerrors" + + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/authorization/armauthorization/v3" +) + +func validPurgeAgedDeletedStepConfig() PurgeAgedDeletedStepConfig { + return PurgeAgedDeletedStepConfig{ + RoleAssignmentsClient: &armauthorization.RoleAssignmentsClient{}, + GraphClient: &msgraphsdk.GraphServiceClient{}, + SubscriptionID: "00000000-0000-0000-0000-000000000000", + } +} + +func TestIsGraphNotFoundError(t *testing.T) { + t.Parallel() + + notFound := graphodataerrors.NewODataError() + notFound.ResponseStatusCode = http.StatusNotFound + if !isGraphNotFoundError(notFound) { + t.Fatalf("expected Graph 404 to be recognized") + } + + serverError := graphodataerrors.NewODataError() + serverError.ResponseStatusCode = http.StatusInternalServerError + if isGraphNotFoundError(serverError) { + t.Fatalf("expected Graph 500 not to be recognized as not found") + } +} + +func TestNewPurgeAgedDeletedStep_ExecutionOptions(t *testing.T) { + t.Parallel() + + defaultStep, err := NewPurgeAgedDeletedStep(validPurgeAgedDeletedStepConfig()) + if err != nil { + t.Fatalf("expected constructor to succeed, got error: %v", err) + } + if got := defaultStep.Name(); got != "Purge aged deleted directory objects" { + t.Fatalf("expected default step name %q, got %q", "Purge aged deleted directory objects", got) + } + if got := defaultStep.RetryLimit(); got != 1 { + t.Fatalf("expected default retry limit 1, got %d", got) + } + if got := defaultStep.ContinueOnError(); got { + t.Fatalf("expected continueOnError false, got %t", got) + } + + customCfg := validPurgeAgedDeletedStepConfig() + customCfg.Name = "custom-name" + customCfg.Retries = 3 + customCfg.ContinueOnError = true + customStep, err := NewPurgeAgedDeletedStep(customCfg) + if err != nil { + t.Fatalf("expected constructor to succeed, got error: %v", err) + } + if got := customStep.Name(); got != "custom-name" { + t.Fatalf("expected step name %q, got %q", "custom-name", got) + } + if got := customStep.RetryLimit(); got != 3 { + t.Fatalf("expected retry limit 3, got %d", got) + } + if got := customStep.ContinueOnError(); !got { + t.Fatalf("expected continueOnError true, got %t", got) + } +} + +func TestNewPurgeAgedDeletedStep_ReturnsErrorWhenInvalid(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + mutate func(*PurgeAgedDeletedStepConfig) + }{ + { + name: "missing role assignments client", + mutate: func(cfg *PurgeAgedDeletedStepConfig) { + cfg.RoleAssignmentsClient = nil + }, + }, + { + name: "missing graph client", + mutate: func(cfg *PurgeAgedDeletedStepConfig) { + cfg.GraphClient = nil + }, + }, + { + name: "missing subscription ID", + mutate: func(cfg *PurgeAgedDeletedStepConfig) { + cfg.SubscriptionID = "" + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + cfg := validPurgeAgedDeletedStepConfig() + tc.mutate(&cfg) + if _, err := NewPurgeAgedDeletedStep(cfg); err == nil { + t.Fatalf("expected validation error") + } + }) + } +} + +func TestMustNewPurgeAgedDeletedStep_PanicsWhenInvalid(t *testing.T) { + t.Parallel() + + cfg := validPurgeAgedDeletedStepConfig() + cfg.GraphClient = nil + + defer func() { + if recover() == nil { + t.Fatalf("expected panic for invalid config") + } + }() + _ = MustNewPurgeAgedDeletedStep(cfg) +} + +func TestNewPurgeAgedDeletedStep_DefaultsMinAge(t *testing.T) { + t.Parallel() + + cfg := validPurgeAgedDeletedStepConfig() + step, err := NewPurgeAgedDeletedStep(cfg) + if err != nil { + t.Fatalf("expected constructor to succeed, got error: %v", err) + } + concrete, ok := step.(*purgeAgedDeletedStep) + if !ok { + t.Fatalf("expected *purgeAgedDeletedStep, got %T", step) + } + if concrete.minAge != DefaultMinAge { + t.Fatalf("expected default min age %v, got %v", DefaultMinAge, concrete.minAge) + } + + cfg.MinAge = 3 * 24 * time.Hour + step, err = NewPurgeAgedDeletedStep(cfg) + if err != nil { + t.Fatalf("expected constructor to succeed, got error: %v", err) + } + concrete, ok = step.(*purgeAgedDeletedStep) + if !ok { + t.Fatalf("expected *purgeAgedDeletedStep, got %T", step) + } + if concrete.minAge != cfg.MinAge { + t.Fatalf("expected overridden min age %v, got %v", cfg.MinAge, concrete.minAge) + } +} + +func TestDeletedObjectRecord_ToTarget(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + record deletedObjectRecord + want string + }{ + { + name: "uses display name when present", + record: deletedObjectRecord{ID: "id-1", DisplayName: "aro-hcp-e2e-sp", ResourceType: ServicePrincipalResourceType}, + want: "aro-hcp-e2e-sp", + }, + { + name: "falls back to ID when display name is empty", + record: deletedObjectRecord{ID: "id-2", ResourceType: ApplicationResourceType}, + want: "id-2", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + target := tc.record.ToTarget() + if target.Name != tc.want { + t.Fatalf("expected name %q, got %q", tc.want, target.Name) + } + if target.ID != tc.record.ID { + t.Fatalf("expected ID %q, got %q", tc.record.ID, target.ID) + } + if target.Type != tc.record.ResourceType { + t.Fatalf("expected type %q, got %q", tc.record.ResourceType, target.Type) + } + }) + } +} + +func TestNormalizeID(t *testing.T) { + t.Parallel() + + if got := normalizeID(" /SUBSCRIPTIONS/ABC "); got != "/subscriptions/abc" { + t.Fatalf("expected normalized ID, got %q", got) + } +} diff --git a/tooling/cleanup-sweeper/pkg/engine/workflows_test.go b/tooling/cleanup-sweeper/pkg/engine/workflows_test.go index 332d16ba5aa..2ca0d3ab11b 100644 --- a/tooling/cleanup-sweeper/pkg/engine/workflows_test.go +++ b/tooling/cleanup-sweeper/pkg/engine/workflows_test.go @@ -44,6 +44,7 @@ func TestWorkflowBuilders(t *testing.T) { "00000000-0000-0000-0000-000000000000", workflowsTestCredential{}, workflowsTestCredential{}, + nil, WorkflowOptions{ DryRun: true, Wait: true, @@ -76,6 +77,7 @@ func TestWorkflowBuilders(t *testing.T) { "00000000-0000-0000-0000-000000000000", workflowsTestCredential{}, nil, + nil, WorkflowOptions{ DryRun: true, Wait: true, @@ -97,6 +99,39 @@ func TestWorkflowBuilders(t *testing.T) { } }, }, + { + name: "role assignments workflow adds directory-object purge step when directory-write credential set", + execute: func(_ *testing.T) (interface{}, error) { + return RoleAssignmentsSweeperWorkflow( + context.Background(), + "00000000-0000-0000-0000-000000000000", + workflowsTestCredential{}, + workflowsTestCredential{}, + workflowsTestCredential{}, + WorkflowOptions{ + DryRun: true, + Wait: true, + Parallelism: 7, + }, + ) + }, + assertions: func(t *testing.T, workflow interface{}, err error) { + t.Helper() + if err != nil { + t.Fatalf("expected no error while building workflow with directory-write credential, got %v", err) + } + builtWorkflow, ok := workflow.(*runner.Engine) + if !ok || builtWorkflow == nil { + t.Fatalf("expected *runner.Engine workflow") + } + if len(builtWorkflow.Steps) != 3 { + t.Fatalf("expected three steps, got %d", len(builtWorkflow.Steps)) + } + if got := builtWorkflow.Steps[0].Name(); got != "Purge aged deleted directory objects" { + t.Fatalf("expected directory-object purge step to run first, got %q", got) + } + }, + }, { name: "resource group ordered workflow propagates canceled context", execute: func(_ *testing.T) (interface{}, error) { From 16bd02670d877fa69cc1b3caf754238c23814796 Mon Sep 17 00:00:00 2001 From: Rael Garcia Date: Fri, 4 Sep 2026 08:06:28 +0000 Subject: [PATCH 2/4] fix(cleanup-sweeper): fail open on purge discovery errors (AROSLSRE-2023) --- .../cleanup-sweeper/cmd/root/options_test.go | 44 ++++++++ .../directoryobjects/purge_aged_deleted.go | 83 +++++++++++---- .../purge_aged_deleted_test.go | 100 +++++++++++++++++- 3 files changed, 207 insertions(+), 20 deletions(-) diff --git a/tooling/cleanup-sweeper/cmd/root/options_test.go b/tooling/cleanup-sweeper/cmd/root/options_test.go index 18f69a12827..8177e20143f 100644 --- a/tooling/cleanup-sweeper/cmd/root/options_test.go +++ b/tooling/cleanup-sweeper/cmd/root/options_test.go @@ -179,3 +179,47 @@ func TestNewGraphCredential(t *testing.T) { } }) } + +func TestNewDirectoryWriteCredential(t *testing.T) { + t.Run("returns nil when no DIRECTORY_WRITE_AZURE_* variables are set", func(t *testing.T) { + t.Setenv("DIRECTORY_WRITE_AZURE_TENANT_ID", "") + t.Setenv("DIRECTORY_WRITE_AZURE_CLIENT_ID", "") + t.Setenv("DIRECTORY_WRITE_AZURE_CLIENT_SECRET", "") + + got, err := newDirectoryWriteCredential() + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + if got != nil { + t.Fatalf("expected nil credential when unset, got %T", got) + } + }) + + t.Run("errors when DIRECTORY_WRITE_AZURE_* variables are partially set", func(t *testing.T) { + t.Setenv("DIRECTORY_WRITE_AZURE_TENANT_ID", "00000000-0000-0000-0000-000000000000") + t.Setenv("DIRECTORY_WRITE_AZURE_CLIENT_ID", "") + t.Setenv("DIRECTORY_WRITE_AZURE_CLIENT_SECRET", "secret") + + _, err := newDirectoryWriteCredential() + if err == nil { + t.Fatalf("expected error for partial configuration") + } + if !strings.Contains(err.Error(), "must all be set") { + t.Fatalf("unexpected error: %v", err) + } + }) + + t.Run("builds a dedicated credential when all DIRECTORY_WRITE_AZURE_* variables are set", func(t *testing.T) { + t.Setenv("DIRECTORY_WRITE_AZURE_TENANT_ID", "00000000-0000-0000-0000-000000000000") + t.Setenv("DIRECTORY_WRITE_AZURE_CLIENT_ID", "11111111-1111-1111-1111-111111111111") + t.Setenv("DIRECTORY_WRITE_AZURE_CLIENT_SECRET", "secret") + + got, err := newDirectoryWriteCredential() + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + if got == nil { + t.Fatalf("expected a credential") + } + }) +} diff --git a/tooling/cleanup-sweeper/pkg/engine/steps/directoryobjects/purge_aged_deleted.go b/tooling/cleanup-sweeper/pkg/engine/steps/directoryobjects/purge_aged_deleted.go index 7e4afa1a80a..f8972a216cb 100644 --- a/tooling/cleanup-sweeper/pkg/engine/steps/directoryobjects/purge_aged_deleted.go +++ b/tooling/cleanup-sweeper/pkg/engine/steps/directoryobjects/purge_aged_deleted.go @@ -91,13 +91,30 @@ type PurgeAgedDeletedStepConfig struct { } type purgeAgedDeletedStep struct { - cfg PurgeAgedDeletedStepConfig - name string - retries int - continueOnError bool - verify runner.VerifyFn - minAge time.Duration - now func() time.Time + cfg PurgeAgedDeletedStepConfig + name string + retries int + continueOnError bool + verify runner.VerifyFn + minAge time.Duration + now func() time.Time + listRoleAssignmentPrincipalIDs func( + ctx context.Context, + roleAssignmentsClient *armauthorization.RoleAssignmentsClient, + subscriptionID string, + logger logr.Logger, + skipReporter *common.DiscoverySkipReporter, + ) (sets.Set[string], error) + resolveActivePrincipalIDs func( + ctx context.Context, + graphClient *msgraphsdk.GraphServiceClient, + principalIDs sets.Set[string], + ) (sets.Set[string], error) + lookupDeletedDirectoryObject func( + ctx context.Context, + graphClient *msgraphsdk.GraphServiceClient, + principalID string, + ) (deletedObjectRecord, bool, error) } var _ runner.Step = (*purgeAgedDeletedStep)(nil) @@ -130,13 +147,16 @@ func NewPurgeAgedDeletedStep(cfg PurgeAgedDeletedStepConfig) (runner.Step, error } return &purgeAgedDeletedStep{ - cfg: cfg, - name: stepName, - retries: cfg.Retries, - continueOnError: cfg.ContinueOnError, - verify: cfg.Verify, - minAge: minAge, - now: now, + cfg: cfg, + name: stepName, + retries: cfg.Retries, + continueOnError: cfg.ContinueOnError, + verify: cfg.Verify, + minAge: minAge, + now: now, + listRoleAssignmentPrincipalIDs: listRoleAssignmentPrincipalIDs, + resolveActivePrincipalIDs: resolveActivePrincipalIDs, + lookupDeletedDirectoryObject: lookupDeletedDirectoryObject, }, nil } @@ -205,9 +225,10 @@ func (s *purgeAgedDeletedStep) Discover(ctx context.Context) ([]runner.Target, e // subscription. This step never scans the tenant's directory blindly - it // only ever considers principals already tied to this subscription's own // role-assignment quota pressure. - principalIDs, err := listRoleAssignmentPrincipalIDs(ctx, s.cfg.RoleAssignmentsClient, s.cfg.SubscriptionID, logger, skipReporter) + principalIDs, err := s.listRoleAssignmentPrincipalIDs(ctx, s.cfg.RoleAssignmentsClient, s.cfg.SubscriptionID, logger, skipReporter) if err != nil { - return nil, fmt.Errorf("failed listing role assignment principal IDs: %w", err) + skipReporter.Record(logger, "role_assignment_principal_listing_failed", "error", err) + return nil, nil } if principalIDs.Len() == 0 { return nil, nil @@ -215,9 +236,10 @@ func (s *purgeAgedDeletedStep) Discover(ctx context.Context) ([]runner.Target, e // 2) Resolve which of those principals are still active. Active // principals are never purge candidates. - activePrincipalIDs, err := resolveActivePrincipalIDs(ctx, s.cfg.GraphClient, principalIDs) + activePrincipalIDs, err := s.resolveActivePrincipalIDs(ctx, s.cfg.GraphClient, principalIDs) if err != nil { - return nil, fmt.Errorf("failed resolving active principals with Microsoft Graph getByIds: %w", err) + skipReporter.Record(logger, "active_principal_resolution_failed", "error", err) + return nil, nil } candidatePrincipalIDs := principalIDs.Difference(activePrincipalIDs) @@ -228,7 +250,7 @@ func (s *purgeAgedDeletedStep) Discover(ctx context.Context) ([]runner.Target, e // is left alone - there is nothing here for this step to purge. targets := make([]runner.Target, 0) for _, principalID := range sets.List(candidatePrincipalIDs) { - record, found, err := lookupDeletedDirectoryObject(ctx, s.cfg.GraphClient, principalID) + record, found, err := s.lookupDeletedDirectoryObject(ctx, s.cfg.GraphClient, principalID) if err != nil { skipReporter.Record(logger, "deleted_item_lookup_failed", "principalID", principalID, "error", err) continue @@ -286,6 +308,7 @@ func listRoleAssignmentPrincipalIDs( ) (sets.Set[string], error) { pager := roleAssignmentsClient.NewListForSubscriptionPager(nil) principalIDs := sets.New[string]() + subscriptionScopePrefix := "/subscriptions/" + normalizeID(subscriptionID) + "/" for pager.More() { page, err := pager.NextPage(ctx) @@ -293,6 +316,9 @@ func listRoleAssignmentPrincipalIDs( return nil, fmt.Errorf("failed listing role assignments: %w", err) } for _, roleAssignment := range page.Value { + if !assignmentWithinSubscriptionScope(roleAssignment, subscriptionScopePrefix) { + continue + } if roleAssignment == nil || roleAssignment.Properties == nil || roleAssignment.Properties.PrincipalID == nil { skipReporter.Record(logger, "invalid_role_assignment_payload") continue @@ -395,3 +421,22 @@ func isGraphNotFoundError(err error) bool { func normalizeID(raw string) string { return strings.ToLower(strings.TrimSpace(raw)) } + +func roleAssignmentID(roleAssignment *armauthorization.RoleAssignment) (string, bool) { + if roleAssignment == nil || roleAssignment.ID == nil { + return "", false + } + id := strings.TrimSpace(*roleAssignment.ID) + return id, id != "" +} + +func assignmentWithinSubscriptionScope( + roleAssignment *armauthorization.RoleAssignment, + subscriptionScopePrefix string, +) bool { + id, ok := roleAssignmentID(roleAssignment) + if !ok { + return false + } + return strings.HasPrefix(normalizeID(id), subscriptionScopePrefix) +} diff --git a/tooling/cleanup-sweeper/pkg/engine/steps/directoryobjects/purge_aged_deleted_test.go b/tooling/cleanup-sweeper/pkg/engine/steps/directoryobjects/purge_aged_deleted_test.go index a961b0f5781..18da7706f70 100644 --- a/tooling/cleanup-sweeper/pkg/engine/steps/directoryobjects/purge_aged_deleted_test.go +++ b/tooling/cleanup-sweeper/pkg/engine/steps/directoryobjects/purge_aged_deleted_test.go @@ -15,14 +15,19 @@ package directoryobjects import ( + "context" + "errors" "net/http" "testing" "time" + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/authorization/armauthorization/v3" + "github.com/go-logr/logr" msgraphsdk "github.com/microsoftgraph/msgraph-sdk-go" graphodataerrors "github.com/microsoftgraph/msgraph-sdk-go/models/odataerrors" + "k8s.io/apimachinery/pkg/util/sets" - "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/authorization/armauthorization/v3" + "github.com/Azure/ARO-HCP/tooling/cleanup-sweeper/pkg/engine/steps/common" ) func validPurgeAgedDeletedStepConfig() PurgeAgedDeletedStepConfig { @@ -212,3 +217,96 @@ func TestNormalizeID(t *testing.T) { t.Fatalf("expected normalized ID, got %q", got) } } + +func TestAssignmentWithinSubscriptionScope(t *testing.T) { + t.Parallel() + + strPtr := func(s string) *string { return &s } + + testCases := []struct { + name string + role *armauthorization.RoleAssignment + want bool + }{ + { + name: "accepts nested scope within subscription", + role: &armauthorization.RoleAssignment{ + ID: strPtr("/subscriptions/abc/resourceGroups/rg-one/providers/Microsoft.Authorization/roleAssignments/ra1"), + }, + want: true, + }, + { + name: "rejects management group scope", + role: &armauthorization.RoleAssignment{ + ID: strPtr("/providers/Microsoft.Management/managementGroups/mg1/providers/Microsoft.Authorization/roleAssignments/ra1"), + }, + want: false, + }, + { + name: "rejects different subscription with shared prefix", + role: &armauthorization.RoleAssignment{ + ID: strPtr("/subscriptions/abc123/providers/Microsoft.Authorization/roleAssignments/ra1"), + }, + want: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got := assignmentWithinSubscriptionScope(tc.role, "/subscriptions/abc/") + if got != tc.want { + t.Fatalf("expected %t, got %t", tc.want, got) + } + }) + } +} + +func TestPurgeAgedDeletedStepDiscover_SkipsWhenRoleAssignmentListingFails(t *testing.T) { + t.Parallel() + + step, err := NewPurgeAgedDeletedStep(validPurgeAgedDeletedStepConfig()) + if err != nil { + t.Fatalf("expected constructor to succeed, got error: %v", err) + } + + concrete := step.(*purgeAgedDeletedStep) + concrete.listRoleAssignmentPrincipalIDs = func(context.Context, *armauthorization.RoleAssignmentsClient, string, logr.Logger, *common.DiscoverySkipReporter) (sets.Set[string], error) { + return nil, errors.New("arm unavailable") + } + + ctx := logr.NewContext(context.Background(), logr.Discard()) + targets, err := concrete.Discover(ctx) + if err != nil { + t.Fatalf("expected discover to fail open, got error: %v", err) + } + if len(targets) != 0 { + t.Fatalf("expected no targets when listing role assignments fails, got %d", len(targets)) + } +} + +func TestPurgeAgedDeletedStepDiscover_SkipsWhenActivePrincipalResolutionFails(t *testing.T) { + t.Parallel() + + step, err := NewPurgeAgedDeletedStep(validPurgeAgedDeletedStepConfig()) + if err != nil { + t.Fatalf("expected constructor to succeed, got error: %v", err) + } + + concrete := step.(*purgeAgedDeletedStep) + concrete.listRoleAssignmentPrincipalIDs = func(context.Context, *armauthorization.RoleAssignmentsClient, string, logr.Logger, *common.DiscoverySkipReporter) (sets.Set[string], error) { + return sets.New("/principals/a"), nil + } + concrete.resolveActivePrincipalIDs = func(context.Context, *msgraphsdk.GraphServiceClient, sets.Set[string]) (sets.Set[string], error) { + return nil, errors.New("graph unavailable") + } + + ctx := logr.NewContext(context.Background(), logr.Discard()) + targets, err := concrete.Discover(ctx) + if err != nil { + t.Fatalf("expected discover to fail open, got error: %v", err) + } + if len(targets) != 0 { + t.Fatalf("expected no targets when resolving active principals fails, got %d", len(targets)) + } +} From 867b587f3df880858f59ed32c2c4f1fd19d16237 Mon Sep 17 00:00:00 2001 From: Rael Garcia Arnes Date: Fri, 4 Sep 2026 08:52:34 +0000 Subject: [PATCH 3/4] fix(cleanup-sweeper): gci import ordering in purge_aged_deleted_test.go (AROSLSRE-2023) --- .../engine/steps/directoryobjects/purge_aged_deleted_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tooling/cleanup-sweeper/pkg/engine/steps/directoryobjects/purge_aged_deleted_test.go b/tooling/cleanup-sweeper/pkg/engine/steps/directoryobjects/purge_aged_deleted_test.go index 18da7706f70..f4d2c2b84b3 100644 --- a/tooling/cleanup-sweeper/pkg/engine/steps/directoryobjects/purge_aged_deleted_test.go +++ b/tooling/cleanup-sweeper/pkg/engine/steps/directoryobjects/purge_aged_deleted_test.go @@ -21,12 +21,14 @@ import ( "testing" "time" - "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/authorization/armauthorization/v3" "github.com/go-logr/logr" msgraphsdk "github.com/microsoftgraph/msgraph-sdk-go" graphodataerrors "github.com/microsoftgraph/msgraph-sdk-go/models/odataerrors" + "k8s.io/apimachinery/pkg/util/sets" + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/authorization/armauthorization/v3" + "github.com/Azure/ARO-HCP/tooling/cleanup-sweeper/pkg/engine/steps/common" ) From 0422e628fc719ec63685d621a21a424ee5fa61ca Mon Sep 17 00:00:00 2001 From: Rael Garcia Date: Mon, 7 Sep 2026 16:49:10 +0000 Subject: [PATCH 4/4] fix(cleanup-sweeper): revalidate deleted directory object before purge (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. --- .../directoryobjects/purge_aged_deleted.go | 22 ++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/tooling/cleanup-sweeper/pkg/engine/steps/directoryobjects/purge_aged_deleted.go b/tooling/cleanup-sweeper/pkg/engine/steps/directoryobjects/purge_aged_deleted.go index f8972a216cb..d295b17fd53 100644 --- a/tooling/cleanup-sweeper/pkg/engine/steps/directoryobjects/purge_aged_deleted.go +++ b/tooling/cleanup-sweeper/pkg/engine/steps/directoryobjects/purge_aged_deleted.go @@ -288,8 +288,28 @@ func (s *purgeAgedDeletedStep) Discover(ctx context.Context) ([]runner.Target, e return targets, nil } +// SAFETY CONTRACT: +// A deleted directory object is purged only after re-reading it from +// deletedItems immediately before the destructive call. This re-read guards +// against the object having been restored (it no longer appears in +// deletedItems), reclassified into a non-purge-eligible type, or not yet +// having aged past minAge, in the time between Discover and Delete. func (s *purgeAgedDeletedStep) Delete(ctx context.Context, target runner.Target, _ bool) error { - err := s.cfg.GraphClient.Directory().DeletedItems().ByDirectoryObjectId(target.ID).Delete(ctx, nil) + record, found, err := lookupDeletedDirectoryObject(ctx, s.cfg.GraphClient, target.ID) + if err != nil { + return fmt.Errorf("failed revalidating deleted directory object %q: %w", target.ID, err) + } + if !found { + return nil + } + if record.DeletedDateTime == nil { + return fmt.Errorf("%w: deleted directory object %q lost its deletedDateTime on revalidation", runner.ErrTargetRetained, target.ID) + } + if age := s.now().Sub(*record.DeletedDateTime); age < s.minAge { + return fmt.Errorf("%w: deleted directory object %q no longer meets minAge on revalidation", runner.ErrTargetRetained, target.ID) + } + + err = s.cfg.GraphClient.Directory().DeletedItems().ByDirectoryObjectId(target.ID).Delete(ctx, nil) if err != nil { if isGraphNotFoundError(err) { return nil