feat: introduced initializer predicate

[OlegErshov]

On-behalf-of: SAP aleh.yarshou@sap.com

This pr is related to #175.

Changes:

1. Introduced a controller which reconciles logical cluster resource and apply the same logic as initializer does
2. Introduced predicate to filter logical clusters and reconcile only logical clusters which have been initialized by security-operator

[aaronschweig]

LGTM 👍

@nexus49 is this something we want to merge this now and include it in the 0.2 release?

[nexus49]

@coderabbitai full review

[coderabbitai]

✅ Actions performed

Full review triggered.

[coderabbitai]

✅ Actions performed

Full review triggered.

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (3)

internal/controller/accountlogicalcluster_controller.go (1)

29-30: Align constructor name with returned type.

The constructor name and returned type now diverge, which makes the API surface less consistent.

Proposed rename

-func NewAccountLogicalClusterReconciler(log *logger.Logger, cfg config.Config, fga openfgav1.OpenFGAServiceClient, mgr mcmanager.Manager) *AccountTypeLogicalClusterReconciler {
+func NewAccountTypeLogicalClusterReconciler(log *logger.Logger, cfg config.Config, fga openfgav1.OpenFGAServiceClient, mgr mcmanager.Manager) *AccountTypeLogicalClusterReconciler {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/controller/accountlogicalcluster_controller.go` around lines 29 -
30, The constructor function NewAccountLogicalClusterReconciler does not match
the returned type *AccountTypeLogicalClusterReconciler; rename the constructor
to NewAccountTypeLogicalClusterReconciler (or alternatively change the returned
type to AccountLogicalClusterReconciler) so the function name and the concrete
type align—update the function declaration and any callers to use
NewAccountTypeLogicalClusterReconciler to keep the API surface consistent.

internal/controller/orglogicalcluster_initializer_controller.go (1)

47-47: Use an initializer-specific lifecycle identifier.

The builder name still uses a reconciler label in initializer wiring; this can blur logs/metrics attribution.

Proposed tweak

-		mclifecycle: builder.NewBuilder("logicalcluster", "OrgLogicalClusterReconciler", subroutines, log).
+		mclifecycle: builder.NewBuilder("logicalcluster", "OrgLogicalClusterInitializer", subroutines, log).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/controller/orglogicalcluster_initializer_controller.go` at line 47,
Replace the reconciler-centric lifecycle name used in the initializer wiring so
logs/metrics reflect the initializer; update the builder.NewBuilder call
(currently using "logicalcluster", "OrgLogicalClusterReconciler") to use an
initializer-specific identifier (e.g., "logicalcluster",
"OrgLogicalClusterInitializer" or similar) where mclifecycle is created, keeping
the same subroutines and log variables so only the name changes.

internal/subroutine/account_tuples.go (1)

52-63: Avoid resolving cluster context twice in Initialize.

accountAndInfoForLogicalCluster already resolves cluster from context. Resolving it again before Update adds another failure point; consider returning the resolved cluster/client from the helper and reusing it.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/subroutine/account_tuples.go` around lines 52 - 63, The helper
accountAndInfoForLogicalCluster currently resolves the cluster but the caller
re-resolves it again before updating; change accountAndInfoForLogicalCluster to
return the resolved cluster or client along with acc and ai (e.g., return values
acc, ai, cl, opErr) and then reuse that returned cluster/client for the Update
call instead of calling s.mgr.ClusterFromContext(ctx) again so the Update uses
the already-resolved cluster (replace the s.mgr.ClusterFromContext call and cl
variable with the returned cluster/client).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@internal/controller/accountlogicalcluster_initializer_controller.go`:
- Around line 23-28: The top-of-file comment incorrectly references
"AccountLogicalClusterReconciler"; update that comment to use the actual type
name "AccountLogicalClusterInitializer" so it reads e.g.
"AccountLogicalClusterInitializer acts as an initializer for account
workspaces." and keep the rest of the comment unchanged to maintain clarity
around the AccountLogicalClusterInitializer type and its fields (log and
mclifecycle).

In `@internal/predicates/initializer.go`:
- Around line 17-19: The panic message inside HasInitializerPredicate
incorrectly references "LogicalClusterIsAccountTypeOrg"; update the error text
to accurately name HasInitializerPredicate (or a generic
"HasInitializerPredicate received non-LogicalCluster resource") so the panic
reflects the correct predicate; locate the panic in function
HasInitializerPredicate and change the fmt.Errorf string accordingly to match
the predicate name.

In `@internal/subroutine/idp.go`:
- Around line 119-127: The kubectl OAuth client block is setting SecretRef even
though ClientType is IdentityProviderClientTypePublic (which uses
TokenEndpointAuthMethodNone); remove the SecretRef field from the kubectl client
literal (or wrap it so SecretRef is only added when ClientType is confidential)
so that the public client object (the one using i.kubectlClientRedirectURLs and
kubectlClientName) does not include SecretRef; update the literal that builds
the client (the struct with ClientName/ClientType/RedirectURIs) accordingly and
ensure only the confidential client retains its SecretRef.

---

Nitpick comments:
In `@internal/controller/accountlogicalcluster_controller.go`:
- Around line 29-30: The constructor function NewAccountLogicalClusterReconciler
does not match the returned type *AccountTypeLogicalClusterReconciler; rename
the constructor to NewAccountTypeLogicalClusterReconciler (or alternatively
change the returned type to AccountLogicalClusterReconciler) so the function
name and the concrete type align—update the function declaration and any callers
to use NewAccountTypeLogicalClusterReconciler to keep the API surface
consistent.

In `@internal/controller/orglogicalcluster_initializer_controller.go`:
- Line 47: Replace the reconciler-centric lifecycle name used in the initializer
wiring so logs/metrics reflect the initializer; update the builder.NewBuilder
call (currently using "logicalcluster", "OrgLogicalClusterReconciler") to use an
initializer-specific identifier (e.g., "logicalcluster",
"OrgLogicalClusterInitializer" or similar) where mclifecycle is created, keeping
the same subroutines and log variables so only the name changes.

In `@internal/subroutine/account_tuples.go`:
- Around line 52-63: The helper accountAndInfoForLogicalCluster currently
resolves the cluster but the caller re-resolves it again before updating; change
accountAndInfoForLogicalCluster to return the resolved cluster or client along
with acc and ai (e.g., return values acc, ai, cl, opErr) and then reuse that
returned cluster/client for the Update call instead of calling
s.mgr.ClusterFromContext(ctx) again so the Update uses the already-resolved
cluster (replace the s.mgr.ClusterFromContext call and cl variable with the
returned cluster/client).

---

ℹ️ Review info

Configuration used: Repository: platform-mesh/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0bbbe02 and 22c4fb8.

📒 Files selected for processing (13)

• Taskfile.yaml
• cmd/initializer.go
• cmd/operator.go
• cmd/terminator.go
• internal/controller/accountlogicalcluster_controller.go
• internal/controller/accountlogicalcluster_initializer_controller.go
• internal/controller/apibinding_controller.go
• internal/controller/orglogicalcluster_controller.go
• internal/controller/orglogicalcluster_initializer_controller.go
• internal/predicates/initializer.go
• internal/subroutine/account_tuples.go
• internal/subroutine/idp.go
• internal/subroutine/workspace_initializer.go

💤 Files with no reviewable changes (1)

• internal/controller/apibinding_controller.go

[nexus49]

pls rebase

[OlegErshov]

@coderabbitai full review

[coderabbitai]

✅ Actions performed

Full review triggered.

[coderabbitai]

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

cmd/initializer.go (1)

82-95: ⚠️ Potential issue | 🔴 Critical

Wire runtimeScheme into an actual client or remove it.

After this refactor, runtimeScheme is never referenced again. That makes the file fail to compile with declared and not used, and the Flux AddToScheme calls become dead code. If those registrations are still required, they need to be applied to the scheme passed into the client that will read those CRDs.

Suggested fix

-		runtimeScheme := runtime.NewScheme()
-		utilruntime.Must(sourcev1.AddToScheme(runtimeScheme))
-		utilruntime.Must(helmv2.AddToScheme(runtimeScheme))
+		utilruntime.Must(sourcev1.AddToScheme(scheme))
+		utilruntime.Must(helmv2.AddToScheme(scheme))

Also remove the now-unused runtime import.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmd/initializer.go` around lines 82 - 95, runtimeScheme is created and has
sourcev1.AddToScheme and helmv2.AddToScheme applied but is never used, causing
an unused variable/error; either apply those registrations to the scheme that is
actually passed into the controller-runtime client or pass runtimeScheme into
client.New. Fix by wiring the registrations into the client Options: replace the
client.New call that uses client.Options{Scheme: scheme} with
client.Options{Scheme: runtimeScheme} (or move the utilruntime.Must(...) calls
to register into the existing 'scheme' variable used elsewhere), and remove the
now-unused runtime import if you choose to delete runtimeScheme.

🧹 Nitpick comments (1)

internal/subroutine/workspace_authorization.go (1)

53-58: Sort audiences before storing WorkspaceAuthenticationConfiguration.Spec.

Now that this path runs from Process as well, the map iteration over accountInfo.Spec.OIDC.Clients below can reorder JWT.Issuer.Audiences on different reconciles. That makes the spec non-deterministic and can trigger needless updates. A slices.Sort(audiences) before building the spec keeps this idempotent.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/subroutine/workspace_authorization.go` around lines 53 - 58, The
spec is non-deterministic because JWT.Issuer.Audiences can be built in different
orders when iterating accountInfo.Spec.OIDC.Clients; in the reconcile path
(called by Process) sort the audiences slice before assigning it to
WorkspaceAuthenticationConfiguration.Spec to ensure idempotency—locate the
audiences construction in workspaceAuthSubroutine.reconcile (and any helper used
by workspaceAuthSubroutine.Process) and call slices.Sort(audiences) (or
equivalent) prior to storing the spec so
WorkspaceAuthenticationConfiguration.Spec always has a stable, deterministic
Audiences ordering.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@internal/subroutine/authorization_model_generation.go`:
- Around line 202-205: The Finalizers method on
AuthorizationModelGenerationSubroutine is gating finalizer registration using
binding.Name (APIBinding.metadata.name) which can differ from the actual
referenced export; change the check to use the export name at
binding.Spec.Reference.Export.Name (the same check used in Process) so you only
skip finalizer registration for internal exports like core.platform-mesh.io or
*.kcp.io; update AuthorizationModelGenerationSubroutine.Finalizers to read the
referenced export name from binding.Spec.Reference.Export.Name and test for
strings.Contains on that value instead of binding.Name.

In `@internal/subroutine/idp.go`:
- Around line 101-124: The code currently overwrites idp.Spec.Clients with
exactly two entries; instead, in the controllerutil.CreateOrUpdate callback
(where idp.Spec.RegistrationAllowed is set), preserve any existing clients and
upsert only the two clients this subroutine owns (identified by ClientName
workspaceName and kubectlClientName). Implement logic to iterate
idp.Spec.Clients, replace the entry whose ClientName equals workspaceName with
the desired workspace client (using i.additionalRedirectURLs, i.baseDomain,
secret name pattern), replace the entry whose ClientName equals
kubectlClientName with the kubectl client (using i.kubectlClientRedirectURLs and
its secret name), and append either entry if missing; leave all other clients
intact so other controllers’ clients are not removed. Ensure SecretRef and
RedirectURIs for both upserted clients match the existing intent.

In `@internal/subroutine/idp/subroutine.go`:
- Around line 293-301: The current fix only handles Secret NotFound; create a
shared helper getOrRefreshRegistrationToken(ctx, adminClient, existingClient,
clientConfig) that encapsulates the logic to read the registration_access_token
(via readRegistrationAccessToken or Secret lookup), treats a missing key or
empty string the same as a NotFound error, and calls
adminClient.RefreshToken(ctx, existingClient.ClientID) to regenerate the token
when needed, returning the valid token or an error; replace the inline
token-recovery code in the Update flow (where registrationAccessToken is set)
and in deleteRemovedClients() so both paths call
getOrRefreshRegistrationToken(...) and properly handle all broken
registration-token states.

In `@internal/subroutine/workspace_initializer.go`:
- Around line 78-81: The error format string in the workspace initialization
code is missing a colon before the wrapped error; update the fmt.Errorf call in
the block that calls w.mgr.ClusterFromContext(ctx) to include a colon (e.g.,
"failed to get cluster from context: %w") so the returned error message reads
clearly when wrapped and logged; locate the code that returns subroutines.OK(),
fmt.Errorf(...) and change only the format string.

---

Outside diff comments:
In `@cmd/initializer.go`:
- Around line 82-95: runtimeScheme is created and has sourcev1.AddToScheme and
helmv2.AddToScheme applied but is never used, causing an unused variable/error;
either apply those registrations to the scheme that is actually passed into the
controller-runtime client or pass runtimeScheme into client.New. Fix by wiring
the registrations into the client Options: replace the client.New call that uses
client.Options{Scheme: scheme} with client.Options{Scheme: runtimeScheme} (or
move the utilruntime.Must(...) calls to register into the existing 'scheme'
variable used elsewhere), and remove the now-unused runtime import if you choose
to delete runtimeScheme.

---

Nitpick comments:
In `@internal/subroutine/workspace_authorization.go`:
- Around line 53-58: The spec is non-deterministic because JWT.Issuer.Audiences
can be built in different orders when iterating accountInfo.Spec.OIDC.Clients;
in the reconcile path (called by Process) sort the audiences slice before
assigning it to WorkspaceAuthenticationConfiguration.Spec to ensure
idempotency—locate the audiences construction in
workspaceAuthSubroutine.reconcile (and any helper used by
workspaceAuthSubroutine.Process) and call slices.Sort(audiences) (or equivalent)
prior to storing the spec so WorkspaceAuthenticationConfiguration.Spec always
has a stable, deterministic Audiences ordering.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository: platform-mesh/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 568155f2-8ea0-486e-a031-a85622e57a07

📥 Commits

Reviewing files that changed from the base of the PR and between ebb4a84 and 8129121.

📒 Files selected for processing (23)

• cmd/initializer.go
• cmd/operator.go
• cmd/system.go
• cmd/terminator.go
• internal/client/all_platformmesh.go
• internal/client/kcp_helper.go
• internal/client/logicalcluster.go
• internal/controller/accountlogicalcluster_controller.go
• internal/controller/accountlogicalcluster_initializer_controller.go
• internal/controller/apibinding_controller.go
• internal/controller/orglogicalcluster_controller.go
• internal/controller/orglogicalcluster_initializer_controller.go
• internal/predicates/initializer.go
• internal/subroutine/account_tuples.go
• internal/subroutine/apiexportpolicy.go
• internal/subroutine/apiexportpolicy_test.go
• internal/subroutine/authorization_model_generation.go
• internal/subroutine/idp.go
• internal/subroutine/idp/subroutine.go
• internal/subroutine/invite.go
• internal/subroutine/mocks/mock_KcpHelper.go
• internal/subroutine/workspace_authorization.go
• internal/subroutine/workspace_initializer.go