feat: support instance applications#322
Conversation
|
I'm interested in this change @henrysachs @janwillies @derbauer97 Is it good for you ? |
There was a problem hiding this comment.
Pull request overview
Adds a new managed resource for GitLab instance-scoped OAuth Applications (self-hosted GitLab), including API types/CRDs, controller reconciliation logic, and supporting GitLab client helpers for both namespaced (instance.gitlab.m.crossplane.io) and cluster-scoped (instance.gitlab.crossplane.io) variants.
Changes:
- Introduces
ApplicationAPI types and generated scaffolding (deepcopy/managed interfaces/registration) for cluster + namespaced scopes. - Adds controllers that reconcile Applications (observe via paginated list lookup; create returns secret via connection details; update implemented as delete-to-recreate).
- Adds instance application client helpers, tests, and an example manifest.
Reviewed changes
Copilot reviewed 10 out of 23 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/namespaced/controller/instance/setup.go | Registers the new namespaced Application controller in instance setup. |
| pkg/namespaced/controller/instance/applications/controller.go | Namespaced Application managed reconciler implementation (observe/create/update/delete). |
| pkg/namespaced/controller/instance/applications/controller_test.go | Unit tests for the namespaced Application controller behavior. |
| pkg/namespaced/clients/instance/application.go | Namespaced GitLab Applications client wrapper + helpers (options/observation/up-to-date checks). |
| pkg/namespaced/clients/instance/application_test.go | Unit tests for namespaced application client helpers. |
| pkg/cluster/controller/instance/zz_setup.go | Registers the new cluster-scoped Application controller in instance setup. |
| pkg/cluster/controller/instance/applications/zz_controller.go | Cluster-scoped (generated) Application controller implementation. |
| pkg/cluster/controller/instance/applications/zz_controller_test.go | Unit tests for the cluster-scoped Application controller behavior. |
| pkg/cluster/clients/instance/zz_application.go | Cluster-scoped (generated) GitLab Applications client wrapper + helpers. |
| pkg/cluster/clients/instance/zz_application_test.go | Unit tests for cluster-scoped application client helpers. |
| package/crds/instance.gitlab.m.crossplane.io_applications.yaml | Namespaced CRD for Application. |
| package/crds/instance.gitlab.crossplane.io_applications.yaml | Cluster-scoped CRD for Application. |
| examples/instance/application.yaml | Example manifest for creating a namespaced Application. |
| apis/namespaced/instance/v1alpha1/zz_generated.managedlist.go | Generated managed list support for namespaced ApplicationList. |
| apis/namespaced/instance/v1alpha1/zz_generated.managed.go | Generated managed interface helpers for namespaced Application. |
| apis/namespaced/instance/v1alpha1/zz_generated.deepcopy.go | Generated deepcopy implementations for namespaced Application types. |
| apis/namespaced/instance/v1alpha1/register.go | Registers namespaced Application types with the scheme. |
| apis/namespaced/instance/v1alpha1/application_types.go | Source API type definitions and kubebuilder markers for namespaced Application. |
| apis/cluster/instance/v1alpha1/zz_register.go | Registers cluster-scoped Application types with the scheme. |
| apis/cluster/instance/v1alpha1/zz_generated.managedlist.go | Generated managed list support for cluster-scoped ApplicationList. |
| apis/cluster/instance/v1alpha1/zz_generated.managed.go | Generated managed interface helpers for cluster-scoped Application. |
| apis/cluster/instance/v1alpha1/zz_generated.deepcopy.go | Generated deepcopy implementations for cluster-scoped Application types. |
| apis/cluster/instance/v1alpha1/zz_application_types.go | Generated cluster-scoped Application API types and markers. |
Files not reviewed (13)
- apis/cluster/instance/v1alpha1/zz_application_types.go: Generated file
- apis/cluster/instance/v1alpha1/zz_generated.deepcopy.go: Generated file
- apis/cluster/instance/v1alpha1/zz_generated.managed.go: Generated file
- apis/cluster/instance/v1alpha1/zz_generated.managedlist.go: Generated file
- apis/cluster/instance/v1alpha1/zz_register.go: Generated file
- apis/namespaced/instance/v1alpha1/zz_generated.deepcopy.go: Generated file
- apis/namespaced/instance/v1alpha1/zz_generated.managed.go: Generated file
- apis/namespaced/instance/v1alpha1/zz_generated.managedlist.go: Generated file
- pkg/cluster/clients/instance/zz_application.go: Generated file
- pkg/cluster/clients/instance/zz_application_test.go: Generated file
- pkg/cluster/controller/instance/applications/zz_controller.go: Generated file
- pkg/cluster/controller/instance/applications/zz_controller_test.go: Generated file
- pkg/cluster/controller/instance/zz_setup.go: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
3566e1a to
7829337
Compare
markussiebert
left a comment
There was a problem hiding this comment.
Thanks for this @BoxBoxJason — nice addition. I dug into the immutability/update story and want to align with you on the model before this lands. (I pushed a small follow-up commit to the branch first: fixes the double-wrapped delete error in Update() and the connection-secret doc comment, with regenerated zz_*/CRDs.)
The core constraint: there is no update API for instance applications
I checked both APIs and the client library:
- REST — the Applications API only exposes
POST /applications(create),GET /applications(list),DELETE /applications/:id(delete) andPOST /applications/:id/renew-secret. There is noPUT/PATCH. The update endpoint is only a stalled draft: MR gitlab-org/gitlab!151480 (stillDraftsince Apr 2024) against issue gitlab-org/gitlab#458617 (still open). - GraphQL — no OAuth-application mutation exists at all. The only
*ApplicationCreatemutation iscdApplicationCreate(continuous-deployment apps), which is unrelated. - The WebUI can edit scopes in place (keeping the same
id) because it uses the internalAdmin::ApplicationsController#updateRails route, which is not exposed programmatically.
So in-place update is simply not reachable from a provider today, and a delete+recreate would rotate both the application_id and the secret, breaking every existing OAuth client. We should not do that silently.
Proposed model
-
Mark all spec fields immutable (
name,redirectURI,scopes,confidential). I'd enforce it with a CEL rule (self == oldSelf) so the apiserver rejects edits directly, rather than relying only on a doc comment. When/if thePUTendpoint lands upstream, we can remove the immutability and implement real updates — relaxing a validation constraint is backward-compatible, so this is not a breaking change later. -
Implement a full diff in
Observeand report drift even though we can't fix it. The whole point of the controller/GitOps model is to reflect whether actual == desired. If someone changes scopes out-of-band in the WebUI, the resource should goSynced=False, not silently pretend it's fine.This needs a client-go bump: the pinned v2.34.0
Applicationresponse struct has noScopesfield, so we currently can't read actual scopes. v2.39.0 added it — seeapplications.go#L56-L63:type Application struct { ID int64 `json:"id"` ApplicationID string `json:"application_id"` ... Confidential bool `json:"confidential"` Scopes []string `json:"scopes"` }
The
GET /applicationsresponse already returnsscopes(it's in the docs response example), so bumping client-go is all that's needed to observe it.application_name,callback_urlandconfidentialare already observable on v2.34.0. -
Updatereturns a clear error instead of recreating, e.g.cannot update GitLab application in place: <field> is immutable in the GitLab API; delete and recreate to change it. Combined with (1), self-inflicted spec edits are rejected at the apiserver, and (2) surfaces any external drift.
Open question: the secret
The secret is only returned at creation. There is a rotation endpoint — POST /applications/:id/renew-secret (GitLab 16.11+) — but client-go does not wrap it (no method in v2.34.0 or v2.39.0; only Create/List/Delete). So rotation is possible at the REST level but would need a client-go addition or a raw call, and we'd have to decide how to trigger it (annotation? separate field?). I'd treat that as a follow-up rather than part of this PR.
What's your take on the immutable-now / observe-and-report / error-on-update model? And do you want secret rotation in scope here or split out?
|
Hey there ! Thanks for this nice and complete review. I did not think it was a problem to rotate the application completely when the spec was mismatched. However, as you mentionned, it will indeed break other applications that do not support "hot" reloads. So I agree with you, this is something to avoid. Secret RotationAbout the SDK modification, it seems like they proceed with an ultra fast release cycle, I created the issue about one week ago and they only took 3 days to complete and release it: https://gitlab.com/gitlab-org/api/client-go/-/work_items/2268. I will implement the drift detection for the Scopes too, now that this is available. I just created an issue to request to add the I think we could wait another 3 days to see if they implement it fast this time too. Then I'd like to go with the What do you think about that ? |
Signed-off-by: BoxBoxJason <contact@boxboxjason.dev>
…connection-secret docs Update() called Delete() and re-wrapped the already-wrapped error with errDeleteFailed, producing duplicated context. Return the Delete() error directly. Drop the publishConnectionDetailsTo mention from the Application godoc: Create() only accepts writeConnectionSecretToRef. Cluster-scoped zz_* files and CRDs regenerated via make generate. Signed-off-by: Markus Siebert <markus.siebert@deutschebahn.com>
Signed-off-by: BoxBoxJason <contact@boxboxjason.dev>
5b57fc1 to
96eafde
Compare
Description of your changes
This PR adds the support of instance scoped (Oauth) Applications. This only works on self hosted instances.
This comes packaged with its full CRD api definition, controller lifecycle management logic, and associated clients methods.
Closes #321
I have:
make reviewableto ensure this PR is ready for review.backport release-x.ylabels to auto-backport this PR if necessary.How has this code been tested
I have:
unexpected drift. (should result in recreation of the resource if applicable)
unexpected drift. (should result in an update of the resource if applicable)