feat: idp resource creation #227
Conversation
On-behalf-of: SAP aleh.yarshou@sap.com
On-behalf-of: SAP aleh.yarshou@sap.com
On-behalf-of: SAP aleh.yarshou@sap.com
On-behalf-of: SAP aleh.yarshou@sap.com
On-behalf-of: SAP aleh.yarshou@sap.com
On-behalf-of: SAP aleh.yarshou@sap.com
On-behalf-of: SAP aleh.yarshou@sap.com
On-behalf-of: SAP aleh.yarshou@sap.com
On-behalf-of: SAP aleh.yarshou@sap.com
On-behalf-of: SAP aleh.yarshou@sap.com
On-behalf-of: SAP aleh.yarshou@sap.com
On-behalf-of: SAP aleh.yarshou@sap.com
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThis change introduces a new IDP subroutine implementation to replace the existing Realm subroutine, refactors configuration structure by adding CoreModulePath and HttpClientTimeoutSeconds while removing APIExportEndpointSliceName and WorkspaceDir, and simplifies SMTP credential handling in the IDP configuration. The RemoveInitializer function is streamlined by removing runtime client dependency and portal secret waiting logic. The invite subroutine is updated to use AccountInfo from accountsv1alpha1 and resolve client IDs from IdentityProviderConfiguration. Workspace authorization is enhanced with WorkspaceType patching functionality. Test coverage is expanded for IDP and workspace authorization workflows, with improved mock specificity in invite subroutine tests. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
internal/subroutine/idp/subroutine.go (1)
162-172: Missing error propagation when reading registration access token fails.If
readRegistrationAccessTokenFromSecretreturns an error that is notNotFound, the code logs and continues viacontinuestatement, but this only skips processing for that managed client. However, the error is not propagated, potentially masking issues like permission errors.Consider explicitly returning an error for non-NotFound failures:
Suggested fix
registrationAccessToken, err := s.readRegistrationAccessTokenFromSecret(ctx, managedClient.SecretRef) if err != nil { if kerrors.IsNotFound(err) { log.Info().Str("secretName", managedClient.SecretRef.Name).Msg("Secret not found, client was likely deleted") continue } + return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("failed to get registration access token from secret: %w", err), true, true) }internal/subroutine/remove_initializer.go (1)
20-22: Remove unused constantPortalClientSecretNamespace.The constant is defined at line 21 but is not referenced anywhere in the codebase. Remove the constant declaration.
internal/subroutine/worksapce_authorization.go (1)
1-1: Filename contains a typo.The file is named
worksapce_authorization.gobut should beworkspace_authorization.go. Consider renaming for consistency and discoverability.
🧹 Nitpick comments (7)
internal/test/integration/authorization_model_generation_test.go (2)
117-127: Consider increasing the timeout for finalizer verification.The 1-second timeout may be too short in CI environments with variable latency, potentially causing flaky tests. Other
Eventuallyblocks in this test use 10-second timeouts.Suggested fix
suite.Assert().Eventually(func() bool { var testApiBinding1, testApiBinding2 kcpapiv1alpha1.APIBinding if err := testAccount1Client.Get(ctx, client.ObjectKey{Name: apiBinding1.Name}, &testApiBinding1); err != nil { return false } if err := testAccount2Client.Get(ctx, client.ObjectKey{Name: apiBinding2.Name}, &testApiBinding2); err != nil { return false } return suite.Equal(expectedFinalizers, testApiBinding1.Finalizers) && suite.Equal(expectedFinalizers, testApiBinding2.Finalizers) - }, 1*time.Second, 200*time.Millisecond, "APIBindings should have the expected finalizers") + }, 10*time.Second, 200*time.Millisecond, "APIBindings should have the expected finalizers")
111-113: Unused variable declarations.The variables
testApiBinding1andtestApiBinding2declared at lines 111-113 are now unused since fresh variables are declared inside theEventuallyblock at line 118. These outer declarations can be removed.Suggested fix
- var testApiBinding1, testApiBinding2 kcpapiv1alpha1.APIBinding - suite.Require().NoError(testAccount1Client.Get(ctx, client.ObjectKey{Name: apiBinding1.Name}, &testApiBinding1)) - suite.Require().NoError(testAccount2Client.Get(ctx, client.ObjectKey{Name: apiBinding2.Name}, &testApiBinding2)) - expectedFinalizers := []string{"apis.kcp.io/apibinding-finalizer", "core.platform-mesh.io/apibinding-finalizer"}internal/subroutine/worksapce_authorization.go (2)
111-124: Parameter shadows imported package name; simplify return.The parameter
clienton line 111 shadows the importedsigs.k8s.io/controller-runtime/pkg/clientpackage. Additionally, the return logic can be simplified.Suggested fix
-func (r *workspaceAuthSubroutine) patchWorkspaceType(ctx context.Context, client client.Client, workspaceTypeName, authConfigurationRefName string) error { +func (r *workspaceAuthSubroutine) patchWorkspaceType(ctx context.Context, c client.Client, workspaceTypeName, authConfigurationRefName string) error { wsType := kcptenancyv1alphav1.WorkspaceType{ ObjectMeta: metav1.ObjectMeta{Name: workspaceTypeName}, } - _, err := controllerutil.CreateOrUpdate(ctx, client, &wsType, func() error { + _, err := controllerutil.CreateOrUpdate(ctx, c, &wsType, func() error { wsType.Spec.AuthenticationConfigurations = []kcptenancyv1alphav1.AuthenticationConfigurationReference{{Name: authConfigurationRefName}} return nil }) - if err != nil { - return err - } - - return nil + return err }
98-106: Error messages are identical; consider distinguishing them.Both error messages on lines 100 and 105 read "failed to create patch workspace type resource". Consider including the workspace type suffix (
-orgvs-acc) to aid debugging.Suggested fix
err = r.patchWorkspaceType(ctx, r.orgClient, fmt.Sprintf("%s-org", workspaceName), workspaceName) if err != nil { - return reconcile.Result{}, errors.NewOperatorError(fmt.Errorf("failed to create patch workspace type resource: %w", err), true, true) + return reconcile.Result{}, errors.NewOperatorError(fmt.Errorf("failed to patch workspace type resource %s-org: %w", workspaceName, err), true, true) } err = r.patchWorkspaceType(ctx, r.orgClient, fmt.Sprintf("%s-acc", workspaceName), workspaceName) if err != nil { - return reconcile.Result{}, errors.NewOperatorError(fmt.Errorf("failed to create patch workspace type resource: %w", err), true, true) + return reconcile.Result{}, errors.NewOperatorError(fmt.Errorf("failed to patch workspace type resource %s-acc: %w", workspaceName, err), true, true) }internal/subroutine/idp.go (2)
61-64: Enhance error message specificity.The error message could provide more context about why the workspace name could not be extracted (e.g., missing or invalid annotation).
🔎 Apply this diff to improve the error message:
workspaceName := getWorkspaceName(lc) if workspaceName == "" { - return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("failed to get workspace name"), true, false) + return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("failed to get workspace name from kcp.io/path annotation"), true, false) }
126-132: Add defensive boundary checking in path parsing.The path splitting logic assumes the annotation format is well-formed. Consider adding validation to prevent potential issues if the path format is unexpected.
🔎 Apply this diff to add boundary checking:
func getWorkspaceName(lc *kcpv1alpha1.LogicalCluster) string { if path, ok := lc.Annotations["kcp.io/path"]; ok { pathElements := strings.Split(path, ":") + if len(pathElements) == 0 { + return "" + } return pathElements[len(pathElements)-1] } return "" }internal/config/config.go (1)
25-25: Consider documenting the HttpClientTimeout unit.The
HttpClientTimeoutfield uses an integer type. Consider adding a comment to clarify the unit (seconds) for maintainability.
📜 Review details
Configuration used: Repository: platform-mesh/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
internal/config/config.go(2 hunks)internal/controller/initializer_controller.go(1 hunks)internal/subroutine/idp.go(1 hunks)internal/subroutine/idp/subroutine.go(2 hunks)internal/subroutine/idp_test.go(1 hunks)internal/subroutine/invite/subroutine.go(6 hunks)internal/subroutine/invite/subroutine_test.go(13 hunks)internal/subroutine/realm.go(0 hunks)internal/subroutine/realm_test.go(0 hunks)internal/subroutine/remove_initializer.go(2 hunks)internal/subroutine/remove_initializer_test.go(4 hunks)internal/subroutine/worksapce_authorization.go(3 hunks)internal/subroutine/workspace_authorization_test.go(4 hunks)internal/test/integration/authorization_model_generation_test.go(1 hunks)
💤 Files with no reviewable changes (2)
- internal/subroutine/realm_test.go
- internal/subroutine/realm.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-24T06:20:22.789Z
Learnt from: aaronschweig
Repo: platform-mesh/security-operator PR: 136
File: internal/subroutine/authorization_model.go:74-81
Timestamp: 2025-10-24T06:20:22.789Z
Learning: In internal/subroutine/authorization_model.go, the NewDiscoveryClientFunc factory does not need to return an error because the *rest.Config passed to it is copied from an already validated manager config (a.mgr.GetLocalManager().GetConfig()), so the config is guaranteed to be valid at that point.
Applied to files:
internal/subroutine/remove_initializer_test.gointernal/subroutine/idp/subroutine.go
🧬 Code graph analysis (4)
internal/subroutine/idp_test.go (6)
internal/subroutine/mocks/mock_Client.go (2)
NewMockClient(19-29)MockClient(32-34)internal/subroutine/mocks/mock_Manager.go (2)
NewMockManager(25-35)MockManager(38-40)internal/config/config.go (1)
Config(10-43)internal/subroutine/idp.go (1)
NewIDPSubroutine(30-37)internal/subroutine/mocks/mock_Cluster.go (2)
MockCluster(35-37)NewMockCluster(22-32)api/v1alpha1/identityproviderconfiguration_types.go (1)
IdentityProviderConfiguration(51-65)
internal/subroutine/invite/subroutine_test.go (1)
api/v1alpha1/identityproviderconfiguration_types.go (3)
IdentityProviderConfiguration(51-65)IdentityProviderConfigurationSpec(29-31)IdentityProviderClientConfig(16-26)
internal/subroutine/remove_initializer.go (1)
internal/config/config.go (1)
Config(10-43)
internal/controller/initializer_controller.go (3)
internal/subroutine/idp.go (1)
NewIDPSubroutine(30-37)internal/subroutine/invite.go (1)
NewInviteSubroutine(29-34)internal/subroutine/remove_initializer.go (1)
NewRemoveInitializer(68-73)
🔇 Additional comments (23)
internal/subroutine/idp/subroutine.go (4)
50-52: HTTP client timeout configuration is correct.The timeout is properly configured using the
HttpClientTimeoutvalue from the configuration, converted to seconds.
262-291: Implementation looks correct.The
createOrUpdateSecretfunction properly initializes the data map if nil and conditionally sets secret values only when they are non-empty.
293-324: Patch functions are well-implemented.Both
patchIdentityProviderConfigurationandpatchIdentityProviderConfigurationStatusfollow the correct pattern of creating a deep copy before mutation and usingMergeFromfor the patch operation.
196-212: No action required.The code correctly handles an empty registration access token. The
executeUpdateRequestmethod explicitly checksif token != ""before setting the Authorization header. When the secret is not found and the token is empty, the initial request proceeds without authentication, receives a 401 Unauthorized response, and triggers automatic token regeneration viaregenerateRegistrationAccessToken. The subsequent retry uses the regenerated token. This is intentional design, not a defect.Likely an incorrect or invalid review comment.
internal/subroutine/invite/subroutine.go (2)
249-264: Implementation looks correct.The
getClientIDhelper properly fetches theIdentityProviderConfigurationby name and usesslices.IndexFuncto find the matching client configuration.
146-154: Client ID resolution integration is well-implemented.The new
getClientIDcall properly integrates with theIdentityProviderConfiguration-based workflow, replacing the previous realm-based client ID.internal/subroutine/invite/subroutine_test.go (3)
69-98: Test mocks correctly updated for the new workflow.The mock expectations now explicitly target the
AccountInfoandIdentityProviderConfigurationresources with proper typed assertions, aligning with the updatedgetClientIDimplementation.
131-159: Test case correctly omitsIdentityProviderConfigurationmock.The error occurs at the Keycloak users endpoint before
getClientIDis invoked, so the mock forIdentityProviderConfigurationis not required.
160-217: Test case for user creation error is properly structured.The mock setup includes both
AccountInfoandIdentityProviderConfiguration, correctly reflecting the flow where users are first queried (returning empty), thengetClientIDis called before attempting user creation.internal/subroutine/remove_initializer.go (3)
24-27: Struct simplification is appropriate.The removal of
runtimeClientandsecretWaitTimeoutaligns with the PR objectives where the secret is now created during IDP resource reconciliation, eliminating the need for wait logic.
68-73: Constructor signature updated correctly.The constructor now accepts only
mgrandcfg, properly initializing the struct with the initializer name from the configuration.
40-66: Process implementation is clean and idempotent.The method correctly checks if the initializer is already absent before patching, avoiding unnecessary operations. The use of
initialization.EnsureInitializerAbsentensures idempotency.internal/controller/initializer_controller.go (1)
32-42: LGTM.The lifecycle chain is correctly wired with the new
NewIDPSubroutineand updatedNewRemoveInitializersignatures. The addition ofWithStaticThenExponentialRateLimiter()improves resilience by preventing aggressive retries during transient failures.internal/subroutine/workspace_authorization_test.go (2)
323-394: Good addition of error path test cases.The new test cases for
patchWorkspaceTypefailures (-organd-acc) and domain CA secret retrieval failure provide comprehensive coverage for the error handling paths introduced in the implementation.
55-74: Test mock expectations correctly validate WorkspaceType creation.The mock expectations properly verify both the resource name and the
AuthenticationConfigurationReferencelinkage for the-organd-accworkspace types.internal/subroutine/remove_initializer_test.go (1)
42-99: Tests correctly updated for simplified constructor signature.The test cases properly use the new
NewRemoveInitializer(mgr, config.Config{InitializerName: ...})signature, aligning with the removal of secret-wait logic from the implementation.internal/subroutine/idp_test.go (3)
23-37: Constructor test validates all field assignments.The test properly verifies that
NewIDPSubroutinecorrectly initializes all fields from the provided configuration.
163-174: Mock sets status during Create, which may not reflect actual API behavior.The mock on line 166 sets
Status.Conditionswithin theCreatecallback. In Kubernetes, resource status is typically not populated during creation and requires a subsequent status update. While this works for unit testing the code path, the mock behavior does not accurately represent the actual API semantics.This is acceptable for unit testing if the implementation re-fetches the resource after creation (which it appears to do on line 169), but verify that the implementation handles the case where status is not immediately available after creation.
77-250: Comprehensive table-driven tests for Process method.The test cases cover the key scenarios including early exits, error propagation, account type filtering, and the create-then-verify flow. The use of
testloggerfor context wrapping is appropriate.internal/subroutine/idp.go (3)
30-37: LGTM: Constructor properly initializes the subroutine.The constructor correctly initializes all required fields from the provided configuration.
48-50: Verify that no cleanup is required for IDP resources.The Finalize method is empty. Ensure that the IdentityProviderConfiguration and associated secrets do not require cleanup when the LogicalCluster is deleted.
93-111: LGTM: CreateOrUpdate logic correctly handles both create and update cases.The implementation properly updates existing client configurations or appends new ones, ensuring idempotent behavior.
internal/config/config.go (1)
37-38: SMTP credentials are properly sourced from environment variables via Viper.The configuration uses Viper's
AutomaticEnv()with an environment key replacer that mapsidp-smtp-passwordto theIDP_SMTP_PASSWORDenvironment variable. The mapstructure tags define configuration structure, not storage mechanism. Credentials are not stored in plain-text configuration files; they are loaded from environment variables, which is the recommended approach for Go applications.Likely an incorrect or invalid review comment.
On-behalf-of: SAP aleh.yarshou@sap.com
On-behalf-of: SAP aleh.yarshou@sap.com
On-behalf-of: SAP aleh.yarshou@sap.com
On-behalf-of: SAP aleh.yarshou@sap.com
Co-authored-by: Bastian Echterhölter <bastian.echterhoelter@sap.com>
On-behalf-of: SAP aleh.yarshou@sap.com
| if err != nil { | ||
| return err | ||
|
|
||
| if err := cl.Get(ctx, client.ObjectKey{Name: workspaceTypeName}, wsType); err != nil { |
There was a problem hiding this comment.
The error is not processed. Also deal with the not found case
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/subroutine/workspace_authorization.go (1)
116-118: Add explicit NotFound error handling for WorkspaceType retrieval.The current implementation treats all Get errors uniformly. When program logic needs to branch based on why an API call failed (e.g., NotFound vs. Forbidden), explicit error checking improves diagnostics. Since WorkspaceType resources should be pre-initialized before this subroutine runs, a NotFound error would indicate a configuration issue that warrants distinct handling from transient failures like timeouts or service unavailability.
Consider using
apierrors.IsNotFound()to distinguish resource-missing errors from retryable failures in the Get call at line 116.
📜 Review details
Configuration used: Repository: platform-mesh/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
internal/config/config.go(2 hunks)internal/subroutine/idp.go(1 hunks)internal/subroutine/idp/subroutine.go(3 hunks)internal/subroutine/idp_test.go(1 hunks)internal/subroutine/invite.go(2 hunks)internal/subroutine/remove_initializer.go(2 hunks)internal/subroutine/workspace_authorization.go(3 hunks)internal/subroutine/workspace_authorization_test.go(4 hunks)internal/test/integration/authorization_model_generation_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/test/integration/authorization_model_generation_test.go
- internal/subroutine/idp/subroutine.go
- internal/subroutine/idp_test.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-18T13:47:30.914Z
Learnt from: OlegErshov
Repo: platform-mesh/security-operator PR: 227
File: internal/subroutine/idp.go:87-90
Timestamp: 2025-12-18T13:47:30.914Z
Learning: In the platform-mesh/security-operator repository, the secret naming convention for portal client secrets follows the pattern `portal-client-secret-{realmName}-{clientName}` to ensure uniqueness when a realm has multiple clients. This is by design and not a bug.
Applied to files:
internal/subroutine/idp.go
📚 Learning: 2025-10-27T08:44:10.551Z
Learnt from: aaronschweig
Repo: platform-mesh/security-operator PR: 139
File: internal/subroutine/invite/subroutine.go:182-188
Timestamp: 2025-10-27T08:44:10.551Z
Learning: In the platform-mesh/security-operator repository, when creating Keycloak realms, a client is created with the same identifier as the realm name. Therefore, using the realm name as the `client_id` parameter in Keycloak API calls (such as execute-actions-email) is correct and intentional.
Applied to files:
internal/subroutine/idp.go
🧬 Code graph analysis (4)
internal/subroutine/workspace_authorization_test.go (2)
internal/config/config.go (1)
Config(10-41)internal/subroutine/mocks/mock_Client.go (1)
MockClient(32-34)
internal/subroutine/workspace_authorization.go (1)
internal/config/config.go (1)
Config(10-41)
internal/subroutine/remove_initializer.go (1)
internal/config/config.go (1)
Config(10-41)
internal/subroutine/idp.go (2)
internal/config/config.go (1)
Config(10-41)api/v1alpha1/identityproviderconfiguration_types.go (3)
IdentityProviderClientConfig(16-26)IdentityProviderClientTypeConfidential(12-12)IdentityProviderConfiguration(51-65)
🔇 Additional comments (10)
internal/subroutine/invite.go (1)
93-93: Log message improvements look good.The grammatical refinements to the log messages improve readability.
Also applies to: 108-108
internal/config/config.go (1)
17-40: Config restructuring aligns with the IDP-centric refactor.The consolidation of fields and the shift from secret-name/key references to direct SMTP credentials simplify configuration management and align with the new IDP subroutine implementation.
internal/subroutine/remove_initializer.go (1)
20-23: Simplified initializer removal aligns with new IDP lifecycle.The removal of runtime client dependency and secret-waiting logic streamlines the subroutine, as the IDP subroutine now ensures resource readiness before this point.
Also applies to: 64-69
internal/subroutine/workspace_authorization.go (1)
98-106: WorkspaceType patching correctly links authentication configuration.The addition of WorkspaceType patching for both
-organd-accvariants ensures proper authentication reference propagation after creating the WorkspaceAuthenticationConfiguration.internal/subroutine/workspace_authorization_test.go (2)
56-82: Comprehensive test coverage for WorkspaceType patching.The expanded tests properly validate the new patching behavior for both
-organd-accWorkspaceType resources, including DomainCALookup scenarios and single-workspace cases.Also applies to: 121-147, 258-284, 324-350
355-438: Error path coverage ensures robustness.The addition of error tests for patchWorkspaceType failures and domain CA secret retrieval failures provides good coverage of failure scenarios.
internal/subroutine/idp.go (4)
30-37: IDP subroutine initialization and validation.The constructor properly extracts configuration fields, and the Process method correctly validates that only organization-type accounts trigger IDP creation.
Also applies to: 58-80
82-111: Client configuration and idempotent CreateOrUpdate logic.The ClientConfig setup correctly handles redirect URIs and secret references. The CreateOrUpdate logic properly updates existing clients by name or appends new ones, ensuring idempotency.
126-132: Workspace name extraction helper.The getWorkspaceName helper correctly extracts the workspace name from the kcp.io/path annotation by taking the last path element.
115-120: Add explicit retry logic with timeout for IDP readiness check, matching the pattern in invite.go.The code checks if the IDP resource is Ready but returns an error immediately if not. While the controller's reconciliation loop provides retry behavior through
WithStaticThenExponentialRateLimiter(), this lacks the explicit timeout and retry logic used elsewhere in the codebase. Theinvite.gosubroutine useswait.ExponentialBackoffWithContext()to wait for the Ready condition before returning an error (lines 95-102). Apply the same pattern here to ensure consistent and more robust readiness handling.
On-behalf-of: SAP aleh.yarshou@sap.com
On-behalf-of: SAP aleh.yarshou@sap.com
On-behalf-of: SAP aleh.yarshou@sap.com
On-behalf-of: SAP aleh.yarshou@sap.com
On-behalf-of: SAP aleh.yarshou@sap.com
This PR is related to #147.
Changes:
remove_initializersubroutine, as the secret is now created during idp resource reconciliation. The secret is already generated at the time of removing the initializerclientIdusage in theinvitereconciliation subroutine. TheclientIdis now generated on the Keycloak side, instead of being just a realm nameworkspace_authorizationsubroutine to set theAuthenticationConfigurationReferencerealmsubroutine, as the IDP resource is now responsible for realm and client setup