feat(groups): add ServiceAccountAccessToken managed resource#326
Conversation
1b52b0f to
929ff4b
Compare
Adds a group-scoped ServiceAccountAccessToken managed resource that manages the personal access token of a group service account. Owner mode (default): the ProviderConfig is a group owner and the token is managed via the service-account endpoints: - Create -> Groups.CreateServiceAccountPersonalAccessToken - Observe -> Groups.ListServiceAccountPersonalAccessTokens (match by token id) - Rotate -> Groups.RotateServiceAccountPersonalAccessToken - Revoke -> Groups.RevokeServiceAccountPersonalAccessToken Self-managed mode: when the referenced ProviderConfig authenticates with the very token this resource writes to its connection secret (detected when the PersonalAccessToken credential secretRef matches writeConnectionSecretToRef by namespace, name and key), the provider acts as the service account itself and uses the self endpoints instead: - Observe -> GET /personal_access_tokens/self (self-inform; external name is auto-adopted from the response) - Rotate -> RotatePersonalAccessTokenSelf - Revoke -> RevokePersonalAccessTokenSelf This enables a self-sustaining loop of short-lived, self-rotating tokens used to reconcile a group. A dead self-token surfaces as a clear terminal error (reseed the credentials secret). A SelfManaged status condition reports the detected mode. The external name is the token id and the token value is written to the connection secret on create/rotate. Rotation, expiresAt/renewalPeriodDays and renewBeforeDays semantics match the group AccessToken controller. groupId, serviceAccountId, name and scopes are immutable (enforced via CEL); the rotation-timing fields stay mutable. Fixes #324 Signed-off-by: Markus Siebert <markus.siebert@deutschebahn.com>
929ff4b to
a211e99
Compare
|
Thanks, this looks solid overall. I found three issues worth addressing before merge, and I also verified the behavior end to end in a local
Optional usability notes:
Happy path and tests look good otherwise. |
- self mode: validate the adopted PAT belongs to spec.forProvider.serviceAccountId (PersonalAccessToken.UserID) and surface a terminal error on mismatch, so a miswired credentials secret can no longer rotate/revoke the wrong service account's token while the reconcile still succeeds. - owner mode: narrow the rotate->create fallback. Only fall through to a fresh create when the existing token is genuinely gone (404, or the specific 'Token already revoked' 400). Any other error (generic 400, 403, 5xx, transport) now returns instead of minting a second token. - cluster scope: fix self-mode detection for cluster-scoped resources. A cluster resource has no namespace and writes to a full SecretReference, so isSelfManaged must compare the credential secret namespace against the write reference's namespace. Implemented as a generator replacement so the cluster zz_ code is regenerated correctly. - client: filter the service-account token list with state=active to avoid paginating through revoked/expired tokens on every Observe poll. - crd: add the 'saat' shortName for friendlier kubectl usage. Regenerated cluster-scoped code and CRDs via make generate. Signed-off-by: Markus Siebert <markus.siebert@deutschebahn.com>
|
Thanks for the thorough review @derbauer97 — and especially for verifying it end to end. All four points addressed in e5f52a3:
Plus the optional usability note: added the Tests: added self-mode match/mismatch cases, four owner-mode rotate-fallback cases (404 + revoked-400 fall through; generic-400 + transient do not, asserting create is never called), and a client test asserting |
|
I reran the provider locally in What now works from my retest:
What I still see not working:
I reproed this again by:
Expected:
Actual:
I checked the code again and the remaining root cause seems to be in the legacy cluster
That means for cluster-scoped resources:
The modern namespaced path does populate it:
So from my retest, items 1/2/4 look good, but cluster self-managed mode still appears broken until the legacy config builder also carries |
…nfig path Cluster-scoped resources are LegacyManaged and resolve their credentials via UseLegacyProviderConfig, which built a Config without CredentialsSecretRef. As a result isSelfManaged() always saw a nil ref and self-managed mode never activated for cluster-scoped ServiceAccountAccessTokens, even when the ProviderConfig read from the exact secret the resource writes to (it stayed SelfManaged=False / OwnerManaged and then 404'd on the owner path). Populate CredentialsSecretRef from pc.Spec.Credentials.SecretRef in the legacy path, mirroring the modern namespaced path, so self-mode detection works for cluster-scoped resources too. Co-authored fix and test verifying the legacy builder carries the ref. Signed-off-by: Markus Siebert <markus.siebert@deutschebahn.com>
|
Good catch @derbauer97 — that's exactly right. The Fixed in 7be643b: Added a unit test ( With this plus the earlier |
Move the provider-gitlab-local imports into their own trailing gci group to satisfy the project's gci section order (standard, default, prefix). Signed-off-by: Markus Siebert <markus.siebert@deutschebahn.com>
|
LGTM @henrysachs or @dariozachow needs to approve :) |
Description of your changes
Why: Running Crossplane against GitLab usually means handing the provider one broad, long-lived personal access token through a
ProviderConfig— tied to a real user, shared across everything.GitLab group service accounts change the economics: they don't consume a license seat* so you can create as many as you want. They're also a relatively new feature — introduced in GitLab 16.1 (API), rolled out to GitLab.com in 16.3, and generally available on GitLab Self-Managed from 17.6 — so this fills a gap the provider hasn't covered before. Pair them with Crossplane's namespaced
ProviderConfigs — every namespace gets its own service account and its own scoped credential.This PR makes that usable end to end:
Secret, is consumed by a namespacedProviderConfig, and reconciles namespaced resources.To enable this, the PR adds a new namespaced
groups.gitlab.m.crossplane.io/v1alpha1ServiceAccountAccessTokenmanaged resource — the token half that the existingServiceAccountand groupAccessTokenresources don't cover. It has two modes, selected automatically.Owner mode (default)
The
ProviderConfigis a group owner (Group AccessToken is not enough - It must be an Instance ServiceAccount or User); the token is managed through the service-account endpoints ofgitlab.com/gitlab-org/api/client-go:Groups.CreateServiceAccountPersonalAccessTokenGroups.ListServiceAccountPersonalAccessTokens(matched by token id — no get-by-id endpoint)Groups.RotateServiceAccountPersonalAccessTokenGroups.RevokeServiceAccountPersonalAccessTokenexpiresAt/renewalPeriodDays+renewBeforeDaysrotation semantics match the groupAccessTokencontroller. The external name is the token id, and the token value is written to the connection secret on create/rotate.Self-managed mode (the self-rotating loop)
When the
ProviderConfigauthenticates with the very token this resource manages, the provider is acting as the service account and can only use the self endpoints. This is detected deterministically — no guessing: self-managed mode is entered when theProviderConfigusesmethod: PersonalAccessTokenand itscredentials.secretRef(namespace, name, key) matches the resource'swriteConnectionSecretToRef(i.e. the resource writes its token into the very secret theProviderConfigreads).In that mode:
GET /personal_access_tokens/self(self-inform; external name auto-adopted from the response)RotatePersonalAccessTokenSelfRevokePersonalAccessTokenSelfA dead/expired self-token surfaces as a clear terminal error (reseed the credentials secret) rather than thrashing doomed rotations.
Other details
SelfManagedstatus condition (+ aSELFprint column).groupId,serviceAccountId,name,scopes) are immutable, enforced with CELXValidation; the rotation-timing fields stay mutable.make generate.Follow-up: The same secret-reference detection could replace the group
AccessTokencontroller's 401-based self-rotate fallback; I'll track that as a separate issue rather than widen this PR.Fixes #324
I have:
make reviewable testto ensure this PR is ready for review. (See testing notes — equivalent steps were run individually; local golangci-lint is 2.5.0 vs CI's 2.11.2.)How has this code been tested
GenerateCreate/Rotate/Observation,ShouldRotate, paginatedGetServiceAccountAccessTokenlookup) and for the controllerObserve/Create/Update/Deletein both modes:SelfManagedcondition.make generate,go build ./...,go vet ./...,golangci-lint run ./...(clean; local 2.5.0), and the fullgo test ./...suite pass (the cluster-scoped variant is generated and tested too).