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.

[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

[akafazov]

Approve with minor comments.

Some controllers are missing rate-limiters.

#175 says new controller must reconcile Accounts, but here it reconciles LogicalClusters?

[akafazov]

Consider filtering for Status.Phase == Ready to make sure the initializer has finished.

[OlegErshov]

It's a nice catch! For this purpose here serves !slices.Contains(lc.Status.Initializers, initializer). When logical cluster is initialized, initializer is removed from the status but he's still present in the spec

[OlegErshov]

Approve with minor comments.

Some controllers are missing rate-limiters.

#175 says new controller must reconcile Accounts, but here it reconciles LogicalClusters?

Yes, it should be like this. The idea is to be able to apply the same logic as Initializer has, but after the Initializer has finished his job. Initializer is working with logical clusters and to have more identical behavior it has been decided to reconcile logical clusters too

[simontesar]

I am late to the discussion here and am fine with merging this for now in case it is a priority.

What bothers me that we now have two reconcilers instead of one but that could actually be simplified to one by passing predicates and lifecycle options to the constructor, could it not?

Also, Processor, Finalizer and Terminator interface implementations now referring to a reconcile function in my opinion misses the point of the subroutines package and seems like working around a bad architecture.

I'm wondering if this is something that should be integrated into https://github.com/platform-mesh/subroutines in some way, e.g. be able to use WithReconcilingTerminator("foo") on a single controller that achieves what we're doing here manually with multiple reconcilers.

[OlegErshov]

Thanks for review! I've updated the way controllers are configured and removed extra files after it.

Currently reconcile function is used just to hold common code between Initializer and Processor interfaces, Terminator one is untouched. I think if we want to get rid of reconcile function we need to change how lifecycle works with Processor and Initializer interfaces. I reckon it's better to do in a follow up pr