e2e: app registration cleanup of orphaned app registrations that are expired#4813
e2e: app registration cleanup of orphaned app registrations that are expired#4813
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bennerv 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 |
There was a problem hiding this comment.
Pull request overview
Adds a new cleanup subcommand to the aro-hcp-tests tooling to remove orphaned/expired e2e Microsoft Entra app registrations, and extends the curated Microsoft Graph Kiota spec + generated client to support listing owned applications.
Changes:
- Add
cleanup app-registrationscommand and supporting options/run logic to delete expired e2e app registrations (with--dry-runsupport). - Extend the Graph Kiota OpenAPI subset + generated client with
/me/ownedObjects/graph.application. - Standardize the e2e app registration name prefix via a shared
graphutil.AppRegistrationPrefixconstant.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tooling/kiota/README.md | Documents how to maintain the curated Graph OpenAPI subset. |
| tooling/kiota/openapi.yaml | Adds the /me/ownedObjects/graph.application endpoint to the curated spec. |
| test/util/framework/per_test_framework.go | Uses the shared app registration name prefix constant. |
| test/util/cleanup/appregistrations/options.go | Wires up Graph client acquisition for the new cleanup job. |
| test/util/cleanup/appregistrations/run.go | Implements list-and-delete flow for expired app registrations (dry-run supported). |
| test/cmd/aro-hcp-tests/cleanup/cmd.go | Adds the new cleanup app-registrations cobra subcommand. |
| internal/graph/util/applications.go | Adds ListOwnedExpiredApplications and the AppRegistrationPrefix constant. |
| internal/graph/graphsdk/me/* | Adds generated request builders for the /me/ownedObjects/graph.application calls. |
| internal/graph/graphsdk/graph_base_service_client.go | Exposes .Me() on the generated Graph service client. |
| internal/graph/graphsdk/kiota-lock.json | Updates Kiota lock hash due to spec changes. |
| internal/graph/graphsdk/models/odataerrors/inner_error.go | Fixes typo in generated comments (“occurred”). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/hold - need to confirm against a service principal login vs. a user login. |
bennerv
force-pushed
the
bvesel/clean-app-registrations-periodically
branch
from
9b23298 to
d76c603
Compare
bennerv
force-pushed
the
bvesel/clean-app-registrations-periodically
branch
from
d76c603 to
765e641
Compare
|
/hold cancel |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for resp != nil { | ||
| for _, app := range resp.GetValue() { | ||
| // Skip app registrations that have credentials not yet expired | ||
| skipApp := false | ||
| for _, cred := range app.GetPasswordCredentials() { | ||
| if cred.GetEndDateTime() != nil && cred.GetEndDateTime().After(time.Now()) { | ||
| skipApp = true | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
In ListOwnedExpiredApplications, apps with no passwordCredentials (or with credentials missing endDateTime) will currently be treated as eligible for deletion because skipApp stays false. For a safety-critical cleanup, consider requiring at least one credential and treating a missing endDateTime as "not safe to delete" (i.e., skip), then only include apps where all credentials have endDateTime <= now (compute now := time.Now() once).
|
|
||
| // Build a fetcher function that abstracts user vs service principal endpoints. | ||
| // Both endpoints return the same ApplicationCollectionResponse type. | ||
| // This one is the service principal version, which is the default for this client since it can be used for both SPs and users, while the /me version only works for users. |
There was a problem hiding this comment.
The comment above getPage says the /servicePrincipals/{id}/ownedObjects variant "can be used for both SPs and users". That endpoint is service-principal scoped (it requires a servicePrincipal id), so this wording is misleading and makes the control-flow harder to reason about. Please reword to reflect that the SP endpoint is used only when the caller is a service principal, and /me/... is used for user tokens.
| // This one is the service principal version, which is the default for this client since it can be used for both SPs and users, while the /me version only works for users. | |
| // Default to the service-principal-scoped /servicePrincipals/{id}/ownedObjects endpoint; | |
| // when the caller is a user token, this is overridden below to use /me/ownedObjects instead. |
| required: true | ||
| schema: | ||
| type: string | ||
| x-ms-docs-key-type: servicePrincipal |
There was a problem hiding this comment.
Trailing whitespace after x-ms-docs-key-type: servicePrincipal can cause unnecessary churn in generated artifacts/hashes and may trip whitespace linters. Please remove the trailing spaces.
| x-ms-docs-key-type: servicePrincipal | |
| x-ms-docs-key-type: servicePrincipal |
|
@bennerv: The following tests 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. |
ARO-25877
What
Creates a cleanup job for orphaned app registrations that didn't get cleaned up due to e2e panics or other things.
Why
Ensures that we clean up app registrations with expired credentials so they don't fill our object quota in our tenants.
Testing
openshift/release pj-rehearse plugin, otherwise ran locally.
Special notes for your reviewer