Add SCIM ConfigMap orchestration helpers and refactor OpenLDAP SCIM tests#721
Add SCIM ConfigMap orchestration helpers and refactor OpenLDAP SCIM tests#721dasarinaidu wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds reusable SCIM ConfigMap orchestration helpers to the actions/ layer and refactors the OpenLDAP SCIM validation suite to use a shared SCIM base fixture, expanding test coverage around ConfigMap-driven route gating, URN-prefixed attribute handling, and rate limiting behaviors.
Changes:
- Introduces SCIM ConfigMap lifecycle helpers (set/delete/restore baseline), rate-limit helpers, and SCIM error/Retry-After parsing utilities in
actions/scim. - Adds generic Kubernetes ConfigMap update/delete helpers (wrangler-based) under
actions/kubeapi/configmaps. - Creates a shared
SCIMBaseSuitefixture and rebases/extensively expands the OpenLDAP SCIM test suite on top of it.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
validation/auth/scim/scim.go |
Adds SCIMBaseSuite shared fixture + small SCIM helper utilities for provider-specific suites. |
validation/auth/scim/scim_openldap_test.go |
Refactors suite to embed SCIMBaseSuite; adds many new tests for ConfigMap gating, URN handling, and rate limiting. |
actions/scim/scim.go |
Adds SCIM ConfigMap orchestration + rate-limit / error-body helpers used by validation tests. |
actions/kubeapi/configmaps/update.go |
Adds a retry-on-conflict ConfigMap update helper. |
actions/kubeapi/configmaps/delete.go |
Adds a ConfigMap delete-and-wait helper. |
| time.Sleep(time.Duration(retryAfter+1) * time.Second) | ||
|
|
||
| resp, err := s.scim.Users().List(nil) | ||
| require.NoError(s.T(), err) | ||
| require.NoError(s.T(), scimactions.CheckStatus(resp, http.StatusOK, "request after Retry-After wait should return 200")) |
| func (b *SCIMBaseSuite) createUserWithCleanup(externalID string, active bool) (string, string) { | ||
| userName, userID, err := scimactions.CreateSCIMUser(b.scim, externalID, active) | ||
| require.NoError(b.T(), err) | ||
| b.T().Cleanup(func() { _, _ = b.scim.Users().Delete(userID) }) |
| func (b *SCIMBaseSuite) createGroupWithCleanup(externalID string) (string, string) { | ||
| groupName, groupID, err := scimactions.CreateSCIMGroup(b.scim, externalID) | ||
| require.NoError(b.T(), err) | ||
| b.T().Cleanup(func() { _, _ = b.scim.Groups().Delete(groupID) }) |
| mgmtUser, getErr := b.client.Management.User.ByID(userID) | ||
| if getErr != nil { | ||
| return false, nil | ||
| } |
| if err != nil { | ||
| return nil, fmt.Errorf("timed out updating config map %s: %w", configMapName, lastErr) | ||
| } |
3f489ba to
3d80076
Compare
dd4d0dc to
4511a4d
Compare
| } | ||
|
|
||
| // WaitForCRTBDeletion waits until the ClusterRoleTemplateBinding is deleted | ||
| func WaitForCRTBDeletion(client *rancher.Client, crtbNamespace, crtbName string) error { |
There was a problem hiding this comment.
This is not needed. We already have an extension for this. Please use that instead : https://github.com/rancher/shepherd/blob/main/extensions/kubeapi/rbac/rbac.go#L230 instead
|
|
||
| type SCIMOpenLDAPTestSuite struct { | ||
| suite.Suite | ||
| session *session.Session | ||
| client *rancher.Client | ||
| cluster *v3.Cluster | ||
| scim *scimclient.Client | ||
| provider string | ||
| adminUser *v3.User | ||
| authConfig *authactions.AuthConfig |
There was a problem hiding this comment.
I think this was discussed a while ago to limit our structs to only clients, cluster and session. Can you update this struct to match our other test suites?
type SCIMOpenLDAPTestSuite struct {
suite.Suite
session *session.Session
client *rancher.Client
cluster *v3.Cluster
scimClient *scimclient.Client
}
adminUser and authConfig are each only used by a single test, so they can be defined locally in those tests instead of in the struct. Similarly, provider doesn't need to be a field - authactions.OpenLdap can be used directly in the tests.
| crtb, err := s.client.WranglerContext.Mgmt.ClusterRoleTemplateBinding().Create(crtbObj) | ||
| require.NoError(s.T(), err, "Should be able to create CRTB for auth user %s", authUser.Username) | ||
| crtbName, crtbNamespace := crtb.Name, crtb.Namespace | ||
| subSession.RegisterCleanupFunc(func() error { | ||
| delErr := s.client.WranglerContext.Mgmt.ClusterRoleTemplateBinding().Delete(crtbNamespace, crtbName, &metav1.DeleteOptions{}) | ||
| if delErr != nil && !k8serrors.IsNotFound(delErr) { | ||
| return delErr | ||
| } | ||
| return rbacapi.WaitForCRTBDeletion(s.client, crtbNamespace, crtbName) | ||
| }) |
There was a problem hiding this comment.
Cleanup code here is not necessary if you use https://github.com/rancher/shepherd/blob/main/extensions/kubeapi/rbac/create.go#L193 to create CRTB
There was a problem hiding this comment.
Addressed — removed all explicit defer s.scimClient.Users().Delete() calls. Resources are now created with subSession so defer subSession.Cleanup() handles cleanup automatically.
bf213ba to
6e6d981
Compare
| } | ||
|
|
||
| if !enabled { | ||
| if err = features.UpdateFeatureFlag(client, SCIMFeatureFlag, true); err != nil { |
There was a problem hiding this comment.
Instead of using the norman API, could you use the shepherd extension that uses public API: https://github.com/rancher/shepherd/blob/main/extensions/kubeapi/features/features.go#L23
There was a problem hiding this comment.
Done — switched to extfeatures "github.com/rancher/shepherd/extensions/kubeapi/features" for both IsFeatureEnabled and UpdateFeatureFlag in SetupSCIMClient.
| // cleanup that deletes the user when the session ends. | ||
| func CreateSCIMUser(scimClient *scimclient.Client, ts *session.Session, externalID string, active bool) (string, string, error) { | ||
| var userName, userID string | ||
| err := kwait.PollUntilContextTimeout(context.Background(), defaults.FiveSecondTimeout, defaults.OneMinuteTimeout, false, func(ctx context.Context) (bool, error) { |
There was a problem hiding this comment.
qq, why do you need to poll when you are creating a user?
There was a problem hiding this comment.
Good catch — removed the poll. CreateSCIMUser is now a direct single call
| } | ||
|
|
||
| // SCIMConfigMapName returns the per-provider SCIM ConfigMap name in cattle-global-data. | ||
| func SCIMConfigMapName(providerName string) string { |
There was a problem hiding this comment.
I don't think we need a helper for such a simple operation - it is just formatting a string. It seems clearer to inline it in the test itself:
configMapName := fmt.Sprintf("scim-config-%s", providerName)
There was a problem hiding this comment.
Removed SCIMConfigMapName — inlined fmt.Sprintf("scim-config-%s", providerName) directly at both call sites.
| } | ||
|
|
||
| // ObserveSCIMThrottle reports whether GET /Users returned 429 within maxAttempts. | ||
| func ObserveSCIMThrottle(scimClient *scimclient.Client, maxAttempts int) (bool, error) { |
There was a problem hiding this comment.
nit, "Observe" is a little vague. Could you rename it to VerifySCIMThrottle instead?
There was a problem hiding this comment.
Done — renamed to VerifySCIMThrottle
|
|
||
| // FirstThrottledResponse polls GET /Users up to maxAttempts times and returns the first | ||
| // response with status 429. Returns nil if no 429 was observed within maxAttempts. | ||
| func FirstThrottledResponse(scimClient *scimclient.Client, maxAttempts int) (*scimclient.Response, error) { |
There was a problem hiding this comment.
nit, could you rename this to FindFirstThrottledResponse to make the intent of the helper clearer?
There was a problem hiding this comment.
Done — renamed to FindFirstThrottledResponse.
| @@ -129,16 +124,18 @@ func (s *SCIMOpenLDAPTestSuite) TestSCIMZFeatureFlagDisableAndReenableEndpoint() | |||
| err := features.UpdateFeatureFlag(s.client, scimactions.SCIMFeatureFlag, false) | |||
There was a problem hiding this comment.
Since actions/features uses norman API, could you switch to using the shepherd extension that uses public API instead for enabling/disaling/updating the feature flag? https://github.com/rancher/shepherd/blob/main/extensions/kubeapi/features/features.go#L23
There was a problem hiding this comment.
Done — switched to extfeatures.UpdateFeatureFlag (shepherd wrangler extension).
|
|
||
| logrus.Info("Re-enabling SCIM flag after test") | ||
| err = features.UpdateFeatureFlag(s.client, scimactions.SCIMFeatureFlag, true) | ||
| err = kwait.PollUntilContextTimeout(context.Background(), defaults.FiveSecondTimeout, defaults.FiveMinuteTimeout, false, func(ctx context.Context) (bool, error) { |
There was a problem hiding this comment.
Polling may not be necessary in this case
There was a problem hiding this comment.
Removed — using a direct extfeatures.UpdateFeatureFlag call. Added WaitForSCIMEndpointStatus after the flag update to wait for the side effect (endpoint returning 404), since the wrangler write is async unlike the Norman API.
| var crtb *cattlev3.ClusterRoleTemplateBinding | ||
| err = kwait.PollUntilContextTimeout(context.Background(), defaults.FiveSecondTimeout, defaults.OneMinuteTimeout, false, func(ctx context.Context) (bool, error) { | ||
| obj := &cattlev3.ClusterRoleTemplateBinding{ | ||
| ObjectMeta: metav1.ObjectMeta{Namespace: s.cluster.ID, GenerateName: "crtb-"}, | ||
| ClusterName: s.cluster.ID, | ||
| UserName: mgmtUser.ID, | ||
| RoleTemplateName: string(rbac.ClusterMember), | ||
| } | ||
| created, createErr := rbacext.CreateClusterRoleTemplateBinding(s.client, obj) | ||
| if createErr != nil { | ||
| return false, nil | ||
| } | ||
| crtb = created | ||
| return true, nil | ||
| }) |
There was a problem hiding this comment.
You could replace this snippet and use the helper here to create the CRTB: https://github.com/rancher/tests/blob/main/actions/kubeapi/rbac/create.go#L92
There was a problem hiding this comment.
Tried using rbacapi.CreateClusterRoleTemplateBinding but it calls client.AsUser() internally, which attempts a local login — this fails with 401 for LDAP auth users. Reverted to client.WranglerContext.Mgmt.ClusterRoleTemplateBinding().Create() +
▎ rbacapi.WaitForCrtbStatus instead, which doesn't require local authentication.
0f19ca0 to
33fb383
Compare
…ests Moves SCIM test helpers into a dedicated package under actions/kubeapi/scim/ and replaces the old actions/scim/ stub. Refactors the OpenLDAP SCIM test suite with the following changes: Actions (actions/kubeapi/scim/scim.go): - SetSCIMConfigMap / SetSCIMConfigMapAndWait: create-or-update the scim-config-<provider> ConfigMap and wait for the expected endpoint status - RestoreSCIMBaseline: resets ConfigMap to enabled=true and waits for both the read path (GET /Users) and write path (POST /Users) to be stable using two consecutive 201 probes to guard against transient 500s during ConfigMap reload - WaitForSCIMEndpointStatus: polls GET /Users until an expected HTTP status is returned - WaitForSCIMCreateReady / WaitForRancherRestart: probe the SCIM write path with configurable timeouts; WaitForRancherRestart uses a 5-min timeout and adds a Kubernetes API proxy readiness check for use after a Rancher process restart triggered by a feature flag change - WaitForWranglerAPIReady (internal): polls Core.Secret().List until Rancher's K8s API proxy is operational after a restart Actions (actions/kubeapi/projects/projects.go): - WaitForProjectBackingNamespace: polls project Status.BackingNamespace until the project controller provisions it (1-min timeout), avoiding the 10-second WaitForProjectFinalizerToUpdate timeout that is too short for slower clusters Test suite (validation/auth/scim/scim_openldap_test.go): - All CreateSCIMUser / CreateSCIMGroup calls pass subSession so per-test cleanup fires at test teardown rather than suite teardown - TestSCIMZFeatureFlagDisableAndReenableEndpoint uses WaitForRancherRestart instead of a shallow 401 probe, ensuring both SCIM and the K8s API proxy are fully operational before the test exits - TestSCIMUserProjectRoleBinding uses WaitForProjectBackingNamespace and WaitForNamespaceReady instead of CreateProject, avoiding the short finalizer-count timeout Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary
scim-config-<provider>ConfigMap:SetSCIMConfigMap,DeleteSCIMConfigMap,RestoreSCIMBaseline, plus baseline/rate-limit data builders (BaselineSCIMConfigMap,RateLimitSCIMConfigMap) and helpers (BurstSCIMRequests,ValidateSCIMErrorBody,WaitForSCIMEndpointStatus,GetRetryAfterSeconds).actions/kubeapi/configmapsUpdate/Delete helpers used by the SCIM actions.SCIMBaseSuitetest fixture invalidation/auth/scim/scim.go; rebase the OpenLDAP SCIM suite onto it and expand coverage for rate limiting, error body shape, and ConfigMap-driven enable/disable flows.NOTE:
The generic ConfigMap
Create,Update, andDeletehelpers underactions/kubeapi/configmaps/will be moved toshepherd/extensions/kubeapi/configmaps/in a follow-up after this PR is merged, in line with the Actions vs Extensions placement rules. This PR keeps them here temporarily to unblock the SCIM test work.