fix(test): don't fail E2E cleanup when deletedItems purge is forbidden (AROSLSRE-1982) - #6839
Conversation
…n (AROSLSRE-1982) The app-only credential used by the Integration environment's E2E tests lacks the Microsoft Graph permission to purge or restore soft-deleted directory objects. permanentlyDeleteDirectoryObject was returning that 403 Authorization_RequestDenied as fatal, and the restore-after-failed-purge fallback hit the same 403, so a passing test scenario was reported as failed purely due to best-effort cleanup lacking a permission grant. Treat a 403 Authorization_RequestDenied on purge the same way the delegated-user credential path already does: log it and leave the object soft-deleted instead of returning an error or attempting a restore (which needs the same missing permission and would just undo a delete that otherwise succeeded). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟢 Approval recommended
The change is narrowly scoped, aligns with the PR description, and is covered by new unit tests for both purge paths.
Pull request overview
This PR adjusts the Microsoft Graph directory object cleanup helpers so E2E teardown does not fail when an app-only credential lacks permission to purge deleted items (Graph 403 Authorization_RequestDenied), treating permanent purge as best-effort and leaving the objects soft-deleted instead.
Changes:
- Detect 403
Authorization_RequestDeniedduring deletedItems purge and skip restore/fatal errors, logging a best-effort skip instead. - Add
isInsufficientPrivilegeshelper to precisely identify the forbidden-purge condition. - Add unit tests covering forbidden purge behavior for both service principal and application purge paths.
File summaries
| File | Description |
|---|---|
| internal/graph/util/directory_objects.go | Adds targeted 403 handling to avoid failing teardown on forbidden purge and introduces isInsufficientPrivileges. |
| internal/graph/util/directory_objects_test.go | Extends test helpers to emit custom Graph error codes and adds tests validating the new forbidden-purge behavior. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gmfrasca, 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 |
|
Rael Garcia (@raelga): The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
/test e2e-parallel Retesting: job pull-ci-Azure-ARO-HCP-main-e2e-parallel/2096280897421578240 failed only in the post-step aro-hcp-gather-observability, not in the actual test suite (aro-hcp-test-local ran 61 passed / 0 failed / 40 skipped). The gather-observability container log shows it completed normally (wrote observability-summary.html) before exiting 1, with no test-specific error in its output. Same job failed at the same time on unrelated open PRs (#6838, #6842), confirming this is a fleet-wide infra flake in the observability-gathering step, not caused by this PR's diff. |
AROSLSRE-1982
What
Stop failing E2E tests when the Microsoft Graph deletedItems purge/restore call returns 403 Authorization_RequestDenied. When purge is forbidden, log it and leave the object soft-deleted, matching the existing behavior for delegated-user credentials, instead of returning a fatal error or attempting a restore that needs the same missing permission.
Why
The Integration environment's E2E app-only credential doesn't have the Graph permission grant that permanent purge requires.
pull-ci-Azure-ARO-HCP-main-e2e-integration-e2e-parallel/2095925191069667328shows three otherwise-passing specs (Authorized CIDRs Connectivity, aggregated advanced features, external auth config) failing only in teardown with:The purge introduced in #6781 is a best-effort quota optimization on top of the underlying soft-delete; it should never fail an otherwise-passing test, and restoring the object after a forbidden purge only undoes a delete that already succeeded, for no benefit, since restore needs the same permission and fails identically.
Testing
cd internal && GOTOOLCHAIN=go1.25.7 go test ./graph/util/...GOTOOLCHAIN=go1.25.7 golangci-lint run ./internal/graph/util/...TestDeleteApplicationPermanentlySkipsRestoreWhenServicePrincipalPurgeIsForbiddenandTestDeleteApplicationPermanentlySkipsRestoreWhenApplicationPurgeIsForbiddencovering the new 403 handling for both the service principal and application purge paths.