fix(serviceaccountaccesstokens): unblock self-managed token deletion#387
Merged
Merged
Conversation
17449fa to
593023a
Compare
In self-managed mode the ProviderConfig authenticates with the very token the resource manages. On delete, the self-revoke succeeds, but the subsequent Observe calls GET /personal_access_tokens/self with the now revoked credential and gets a 401 (or 403 once repeated unauthorized polls are rate-limited). observeSelf treated any error as terminal, so Observe kept erroring and the managed reconciler never reached the finalizer-removal path. The resource was stuck deleting forever. Fix, applied to the group, project and instance resources: - observeSelf: when the resource is being deleted (meta.WasDeleted) and the self endpoint rejects the credential (401/403), report ResourceExists: false so the finalizer is removed. A revoked self-token can never be positively read, so the auth failure is the only "gone" signal available. Non-auth errors (5xx, transport) and the live (non-deletion) path still surface as errSelfInformFailed. - self-mode Delete: treat a 401/403 from RevokeServiceAccountSelf as an idempotent success, so a second reconcile against an already-revoked token does not wedge the delete. - add IsResponseUnauthorized (401/403), nil-safe on the embedded *http.Response, next to IsResponseNotFound. Cluster (zz_) counterparts regenerated from the namespaced sources. Tests: delete-time 401/403 -> gone and 5xx -> still errors in observeSelf; idempotent 401/403 self-revoke in Delete. Signed-off-by: Markus Siebert <markus.siebert@deutschebahn.com>
593023a to
55f471d
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes a deletion deadlock for ServiceAccountAccessToken resources in self-managed mode by treating self-endpoint auth failures (401/403) as the terminal “gone” signal during deletion, allowing finalizers to be removed and deletes to complete.
Changes:
- Update
observeSelfto returnResourceExists: falseon delete-time401/403from the self endpoint. - Make self-mode
Deletetreat401/403fromRevokeServiceAccountSelfas an idempotent success. - Add
clients.IsResponseUnauthorizedhelper and extend unit tests across group/project/instance controllers (namespaced + generated cluster).
Reviewed changes
Copilot reviewed 7 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/namespaced/controller/projects/serviceaccountaccesstokens/controller.go | Treat delete-time self-endpoint 401/403 as “gone”; make self-revoke 401/403 idempotent. |
| pkg/namespaced/controller/projects/serviceaccountaccesstokens/controller_test.go | Add tests for delete-time 401/403 observe behavior and idempotent self-revoke. |
| pkg/namespaced/controller/instance/serviceaccountaccesstokens/controller.go | Same deletion-unblock behavior for instance-scoped controller. |
| pkg/namespaced/controller/instance/serviceaccountaccesstokens/controller_test.go | Tests covering delete-time observe and delete idempotency for instance controller. |
| pkg/namespaced/controller/groups/serviceaccountaccesstokens/controller.go | Same deletion-unblock behavior for group-scoped controller. |
| pkg/namespaced/controller/groups/serviceaccountaccesstokens/controller_test.go | Tests covering delete-time observe and delete idempotency for group controller. |
| pkg/namespaced/clients/gitlab.go | Add IsResponseUnauthorized helper for 401/403 detection. |
| pkg/cluster/controller/projects/serviceaccountaccesstokens/zz_controller.go | Regenerated cluster controller with the same self-managed deletion fixes. |
| pkg/cluster/controller/projects/serviceaccountaccesstokens/zz_controller_test.go | Regenerated cluster tests for the same scenarios. |
| pkg/cluster/controller/instance/serviceaccountaccesstokens/zz_controller.go | Regenerated cluster controller with the same self-managed deletion fixes. |
| pkg/cluster/controller/instance/serviceaccountaccesstokens/zz_controller_test.go | Regenerated cluster tests for the same scenarios. |
| pkg/cluster/controller/groups/serviceaccountaccesstokens/zz_controller.go | Regenerated cluster controller with the same self-managed deletion fixes. |
| pkg/cluster/controller/groups/serviceaccountaccesstokens/zz_controller_test.go | Regenerated cluster tests for the same scenarios. |
| pkg/cluster/clients/zz_gitlab.go | Add cluster-side IsResponseUnauthorized helper. |
Files not reviewed (7)
- pkg/cluster/clients/zz_gitlab.go: Generated file
- pkg/cluster/controller/groups/serviceaccountaccesstokens/zz_controller.go: Generated file
- pkg/cluster/controller/groups/serviceaccountaccesstokens/zz_controller_test.go: Generated file
- pkg/cluster/controller/instance/serviceaccountaccesstokens/zz_controller.go: Generated file
- pkg/cluster/controller/instance/serviceaccountaccesstokens/zz_controller_test.go: Generated file
- pkg/cluster/controller/projects/serviceaccountaccesstokens/zz_controller.go: Generated file
- pkg/cluster/controller/projects/serviceaccountaccesstokens/zz_controller_test.go: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
dariozachow
approved these changes
Jul 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this does
Fixes a deletion deadlock for
ServiceAccountAccessTokenresources running in self-managed mode (group, project and instance).In self-managed mode the
ProviderConfigauthenticates with the very token the resource manages. On delete, the self-revoke (DELETE /personal_access_tokens/self) succeeds, but the reconciler then re-runsObserve, which callsGET /personal_access_tokens/selfwith the now-revoked credential and receives a401(or403once repeated unauthorized polls get rate-limited).observeSelftreated any error as terminal, soObservekept erroring and the managed reconciler never reached the finalizer-removal path — the resource was stuckTerminatingforever.A revoked self-token can never be positively read (authentication fails before the record is returned), so the auth failure is the only "gone" signal the self endpoint can give.
Changes
observeSelf: when the resource is being deleted (meta.WasDeleted) and the self endpoint rejects the credential (401/403), reportResourceExists: falseso the finalizer is removed. Non-auth errors (5xx, transport) and the live (non-deletion) path still surface aserrSelfInformFailed.Delete: treat a401/403fromRevokeServiceAccountSelfas an idempotent success, so a second reconcile against an already-revoked token does not wedge the delete.IsResponseUnauthorized(401/403) added next toIsResponseNotFound, nil-safe on the embedded*http.Response.zz_) counterparts regenerated from the namespaced sources.Behavior preserved
reseed the credentials secret).200) self-inform.Tests
observeSelf: delete-time401→ gone,403→ gone,500→ still errors.Delete: idempotent401/403self-revoke.All added across group, project and instance controllers (namespaced and generated cluster).