Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new Rancher SCIM client implementation under clients/rancher/auth/scim to support interacting with SCIM Users, Groups, and discovery endpoints.
Changes:
- Introduces SCIM data models (User, Group, PatchOp) and a lightweight HTTP transport wrapper.
- Adds client sub-resources for Users, Groups, and Discovery with CRUD-style methods.
- Provides helper response utilities (
DecodeJSON,IDFromBody) and aBoolPtrhelper.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return &Client{t: &scimTransport{ | ||
| baseURL: fmt.Sprintf("%s/v1-scim/%s", opts.URL, provider), |
There was a problem hiding this comment.
rancher/rancher#52996 (comment)
SCIM (System for Cross-domain Identity Management) is a new feature in Rancher 2.14 that allows Identity Providers like Okta, Azure AD, and OpenLDAP to provision and deprovision users and groups automatically via a standardised REST API.
When an IdP provisions a user it calls POST /v1-scim/{provider}/Users. When it deactivates a user it calls PATCH /v1-scim/{provider}/Users/{id} with active: false. When it removes a user from a group it calls PATCH /v1-scim/{provider}/Groups/{id} with a remove operation. Each of these maps directly to one of the CRUD methods in the client
0dd5a8c to
66c21b1
Compare
| httpClient *http.Client | ||
| } | ||
|
|
||
| func (t *scimTransport) do(method, resource, id string, query url.Values, body interface{}) (*Response, error) { |
There was a problem hiding this comment.
Why do we need this ? Can we do similar to what we have here: https://github.com/rancher/shepherd/blob/main/clients/rancher/v1/client.go#L100 to create a SCIM client ? It looks like a lot of functionality in this file may already be covered and reimplementing what clientbase.NewAPIClient already does.
Something like:
func NewClient(host, provider, token string) (*Client, error) {
opts := &clientbase.ClientOpts{
URL: "https://" + host + "/v1-scim/" + provider,
TokenKey: token,
Insecure: true,
}
baseClient, err := clientbase.NewAPIClient(opts)
if err != nil {
return nil, fmt.Errorf("failed creating Client: %v", err)
}
return &Client{APIBaseClient: baseClient}, nil
}
There was a problem hiding this comment.
Hi Priya, thanks for the suggestion — I tried exactly this approach first. Unfortunately v1.NewClient cannot be used for SCIM due to a hard constraint in clientbase.NewAPIClient.
Specifically, newAPIClientInternal (common.go L~95) makes a GET to opts.URL during construction and requires an X-API-Schemas response header to bootstrap its schema registry. The SCIM endpoint (/v1-scim/) returns no such header — it's not a Norman/Steve API. So v1.NewClient(opts) always returns:
Failed to find schema at [https:///v1-scim/openldap]
This means the client construction itself fails before any SCIM call is made.
The second constraint is Ops.DoModify returns an error for any >= 300 status (ops.go). Our tests deliberately assert on 409, 401, and 404 as expected responses — TestSCIMInvalidTokenReturns401, TestSCIMCreateDuplicateUserReturns409 etc. Using DoModify would turn those into errors and break all negative-path tests.
The scimTransport.do() exists solely because of these two constraints — it's a thin HTTP wrapper (30 lines) that bypasses the schema layer while still reusing ClientOpts for TLS, timeout, CACerts and proxy config (addressed in the latest commit). Happy to discuss further if helpful.
There was a problem hiding this comment.
This do method does not belong here. It should apart of an extension of clientbase. Let's maintain the patterns we have in place.
There was a problem hiding this comment.
The reason do() is here rather than in clientbase is the same schema bootstrap constraint above. Additionally clientbase.Ops.DoModify returns an error for any >= 300 HTTP status. Our SCIM tests deliberately assert on 409, 401 and 404 as expected values — TestSCIMCreateDuplicateUserReturns409, TestSCIMInvalidTokenReturns401 etc. Using DoModify would convert those expected responses into errors and break all negative-path tests. do() is 30 lines and exists specifically because of these two constraints.
| return &Client{t: &scimTransport{ | ||
| baseURL: fmt.Sprintf("%s/v1-scim/%s", opts.URL, provider), |
There was a problem hiding this comment.
rancher/rancher#52996 (comment)
SCIM (System for Cross-domain Identity Management) is a new feature in Rancher 2.14 that allows Identity Providers like Okta, Azure AD, and OpenLDAP to provision and deprovision users and groups automatically via a standardised REST API.
When an IdP provisions a user it calls POST /v1-scim/{provider}/Users. When it deactivates a user it calls PATCH /v1-scim/{provider}/Users/{id} with active: false. When it removes a user from a group it calls PATCH /v1-scim/{provider}/Groups/{id} with a remove operation. Each of these maps directly to one of the CRUD methods in the client
igomez06
left a comment
There was a problem hiding this comment.
I left comments. Also there is no underscores in go files.
| httpClient *http.Client | ||
| } | ||
|
|
||
| func (t *scimTransport) do(method, resource, id string, query url.Values, body interface{}) (*Response, error) { |
There was a problem hiding this comment.
This do method does not belong here. It should apart of an extension of clientbase. Let's maintain the patterns we have in place.
| t *scimTransport | ||
| } | ||
|
|
||
| func NewClient(opts *clientbase.ClientOpts, provider string) *Client { |
There was a problem hiding this comment.
Also here when you instantiate the client how do you pass in the opts? How does it get the bearer token? Why isn't this added to the parent/umbrella rancher client?
There was a problem hiding this comment.
The token is passed via opts.TokenKey — same as every other shepherd client. Harvester does exactly this: clientOptsV1 sets TokenKey: restConfig.BearerToken. The reason SCIM is not on the parent rancher.Client is that SCIM uses a separate per-provider bearer token stored in a Kubernetes secret in cattle-global-data — not the Rancher admin token. The parent client is constructed with the admin token. If SCIM were added there, either the admin token would be used for SCIM (which returns 401 ), or the parent client constructor would need a second token parameter which breaks the existing interface for all callers.
|
Also another question should this client be in its own directory in rancher/clients, or be under auth directory. The schema is different that's why I'm asking |
66c21b1 to
4c5ad76
Compare
|
@igomez06 Thanks for the review and comments. All four comments are related to the same root question. The SCIM endpoint is a plain HTTP server (gorilla/mux) that does not speak the Norman/Steve protocol and does not return X-API-Schemas. Both v1.NewClient and clientbase.NewAPIClient fail at construction because of this. I have Postman screenshots in the thread above proving it. The 30-line transport exists solely because of this constraint — not to duplicate clientbase. Why not in common.go: Moving SCIM types and the client there would mean: SCIM data models (User, Group, PatchOp, Member) living alongside generic HTTP infrastructure Happy to jump on a quick call to walk through it if that would help close this faster. |
igomez06
left a comment
There was a problem hiding this comment.
This is good Das, please get a backport and then I will approve it officially
I created this client at clients/rancher/auth/scim/scim_client.go
Why not in common.go:
pkg/clientbase/common.go contains low-level HTTP infrastructure: ClientOpts, APIBaseClient, APIError, NewAPIClient, IsNotFound. It's the foundation that all shepherd clients build on top of.
Moving SCIM types and the client there would mean:
The current location clients/rancher/auth/scim/ is the correct parallel to how OpenLDAP and Active Directory are structured.
They each have their own subdirectory under clients/rancher/auth/. SCIM follows the same pattern.