Draft
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
aaronschweig
approved these changes
Jan 26, 2026
Contributor
There was a problem hiding this comment.
LGTM 👍
@nexus49 is this something we want to merge this now and include it in the 0.2 release?
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
Contributor
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThis change introduces an initializer pattern for logical cluster reconciliation alongside existing reconcilers. The codebase is refactored to separate initialization concerns into dedicated initializer controllers (for both org and account logical clusters) while maintaining corresponding reconciler controllers for ongoing reconciliation. Predicates are implemented to route logical clusters to the appropriate controller based on account type and initializer status. Multicluster client dependencies are removed from several command entry points, and internal subroutines are updated to retrieve cluster clients from context via the manager instead of explicit client construction. Identity provider and workspace initialization flows are simplified through revised resource update patterns. 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/subroutine/account_tuples.go (1)
116-130: 🛠️ Refactor suggestion | 🟠 MajorInconsistent client retrieval pattern between Initialize and Terminate.
The
Initializemethod (lines 58-61) usess.mgr.ClusterFromContext(ctx)for client retrieval, butTerminatestill uses the oldiclient.NewForLogicalClusterpattern. This inconsistency should be addressed for maintainability and alignment with the PR's refactoring goals.♻️ Proposed fix to use consistent ClusterFromContext pattern
// Remove finalizer from AccountInfo. if updated := controllerutil.RemoveFinalizer(&ai, accountTuplesTerminatorFinalizer); updated { - lcID, ok := mccontext.ClusterFrom(ctx) - if !ok { - return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("cluster name not found in context"), true, true) - } - - lcClient, err := iclient.NewForLogicalCluster(s.mgr.GetLocalManager().GetConfig(), s.mgr.GetLocalManager().GetScheme(), logicalcluster.Name(lcID)) + cl, err := s.mgr.ClusterFromContext(ctx) if err != nil { - return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("getting client: %w", err), true, true) + return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("failed to get cluster from context: %w", err), true, true) } - if err := lcClient.Update(ctx, &ai); err != nil { + if err := cl.GetClient().Update(ctx, &ai); err != nil { return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("updating AccountInfo to remove finalizer: %w", err), true, true) } }This would also allow removing the
iclient,mccontext, andlogicalclusterimports if no longer needed.🤖 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 116 - 130, The Terminate block currently creates a logical-cluster client via iclient.NewForLogicalCluster (inside the controllerutil.RemoveFinalizer branch); replace that with the same pattern used in Initialize by calling s.mgr.ClusterFromContext(ctx) (or s.mgr.ClusterFromContext to match the existing method) to obtain the logical-cluster client and cluster ID, handle the missing-cluster case the same way Initialize does, then call lcClient.Update(ctx, &ai) to persist the finalizer removal; after changing the retrieval, remove the now-unused iclient, mccontext, and logicalcluster imports if they become unused.
🧹 Nitpick comments (6)
internal/subroutine/workspace_initializer.go (1)
155-158: DuplicateClusterFromContextcall.The cluster is already retrieved on line 87 and stored in
cl. This second call on line 155 is redundant. Reuse the existingclvariable.♻️ Proposed fix
- cluster, err := w.mgr.ClusterFromContext(ctx) - if err != nil { - return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("unable to get cluster from context: %w", err), true, false) - } - accountInfo := accountsv1alpha1.AccountInfo{ ObjectMeta: metav1.ObjectMeta{Name: "account"}, } - _, err = controllerutil.CreateOrUpdate(ctx, cluster.GetClient(), &accountInfo, func() error { + _, err = controllerutil.CreateOrUpdate(ctx, cl.GetClient(), &accountInfo, func() error { accountInfo.Spec.FGA.Store.Id = store.Status.StoreID return nil })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/subroutine/workspace_initializer.go` around lines 155 - 158, Remove the redundant call to w.mgr.ClusterFromContext(ctx) and the local variable cluster; reuse the previously obtained cl variable (from the earlier ClusterFromContext call) wherever cluster is used in this block. Replace references to cluster with cl and remove the duplicated error handling for ClusterFromContext so there is only the original retrieval/err check that populated cl.internal/subroutine/idp.go (1)
106-139: Redundant Get after CreateOrUpdate.The
controllerutil.CreateOrUpdatefunction updates the passedidpobject in place with the server's response, including the currentresourceVersion. The subsequentGetcall on line 137 is unnecessary and adds latency.♻️ Proposed fix to remove redundant Get
if err != nil { return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("failed to create idp resource %w", err), true, true) } log.Info().Str("workspace", workspaceName).Msg("idp configuration resource is created") - if err := cl.GetClient().Get(ctx, types.NamespacedName{Name: workspaceName}, idp); err != nil { - return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("failed to get idp resource %w", err), true, true) - } - if !meta.IsStatusConditionTrue(idp.GetConditions(), "Ready") {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/subroutine/idp.go` around lines 106 - 139, The CreateOrUpdate call on the idp object (controllerutil.CreateOrUpdate with idp) already returns the updated object in-place (including resourceVersion), so remove the redundant cl.GetClient().Get(ctx, types.NamespacedName{Name: workspaceName}, idp) call and its error handling; rely on the idp instance populated by CreateOrUpdate and continue using that variable (and its metadata) for any subsequent logic or logging to avoid the extra API call and latency.internal/controller/accountlogicalcluster_controller.go (1)
23-35: Constructor name does not match return type.The constructor
NewAccountLogicalClusterReconcilerreturns*AccountTypeLogicalClusterReconciler. This naming inconsistency is confusing. Consider renaming toNewAccountTypeLogicalClusterReconcilerfor clarity and consistency.♻️ Proposed fix
-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 {Note: This would require updating call sites in
cmd/operator.go(line 207) and potentially other files.🤖 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 23 - 35, The constructor function name NewAccountLogicalClusterReconciler does not match the returned type AccountTypeLogicalClusterReconciler; rename the constructor to NewAccountTypeLogicalClusterReconciler and update all call sites to use the new name, ensuring any references to NewAccountLogicalClusterReconciler are replaced; keep the function signature and returned struct (AccountTypeLogicalClusterReconciler, fields log and mclifecycle built via builder.NewBuilder(...).BuildMultiCluster(mgr)) unchanged so only the constructor identifier is updated for consistency.internal/subroutine/account_tuples.go (1)
175-175: Extraneous blank line.Line 175 contains a trailing whitespace/blank line that should be removed for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/subroutine/account_tuples.go` at line 175, Remove the extraneous trailing blank line at line 175 in internal/subroutine/account_tuples.go to eliminate the unnecessary whitespace; open the file, locate the blank/whitespace-only line near the surrounding function or type declaration in account_tuples.go (e.g., within the account tuple-related functions) and delete that empty line so the file has no trailing blank line at that location.internal/controller/orglogicalcluster_initializer_controller.go (1)
47-47: Use initializer-specific builder name for consistent observability.Line 47 passes
"OrgLogicalClusterReconciler"even though this controller is an initializer. Rename it to keep logs and metrics aligned with the actual controller role.Proposed fix
- 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, The builder invocation using builder.NewBuilder currently uses the reconciler name "OrgLogicalClusterReconciler" which is incorrect for this initializer; update the string argument to an initializer-specific name (e.g. "OrgLogicalClusterInitializer" or "OrgLogicalClusterInitializerController") so logs/metrics align with the controller's role where mclifecycle is constructed via builder.NewBuilder("logicalcluster", "...", subroutines, log).internal/controller/accountlogicalcluster_initializer_controller.go (1)
23-34: Align controller naming to initializer semantics.Line 23 and Line 33 still use
Reconcilernaming although this type is an initializer. This makes logs/metrics/controller identity harder to trace.Proposed fix
-// AccountLogicalClusterReconciler acts as an initializer for account workspaces. +// AccountLogicalClusterInitializer acts as an initializer for account workspaces. @@ - mclifecycle: builder.NewBuilder("security", "AccountLogicalClusterReconciler", []lifecyclesubroutine.Subroutine{ + mclifecycle: builder.NewBuilder("security", "AccountLogicalClusterInitializer", []lifecyclesubroutine.Subroutine{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/accountlogicalcluster_initializer_controller.go` around lines 23 - 34, The controller name still uses "Reconciler" which conflicts with the initializer semantics; update the identifier passed into builder.NewBuilder to use "AccountLogicalClusterInitializer" (and any other occurrences of "AccountLogicalClusterReconciler") so logs/metrics and controller identity reflect the initializer. Specifically, edit the NewAccountLogicalClusterInitializer function and change the name argument in builder.NewBuilder from "AccountLogicalClusterReconciler" to "AccountLogicalClusterInitializer" and replace any other literal or symbol usages of "Reconciler" related to AccountLogicalCluster with "Initializer".
🤖 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_controller.go`:
- Around line 3-21: Reorder the import block in
internal/controller/accountlogicalcluster_controller.go to satisfy the project's
gci rules: run the project's import formatter (or gci) to group and sort imports
(standard library first like context, then external modules such as
kcpcorev1alpha1 and openfgav1, then github.com/platform-mesh/... and
local/internal packages), ensuring symbols like kcpcorev1alpha1, openfgav1,
platformeshconfig, builder, multicluster, lifecyclesubroutine, logger, config,
predicates, subroutine, ctrl, predicate, mccontext, mcmanager, and mcreconcile
remain imported but correctly ordered.
In `@internal/controller/orglogicalcluster_controller.go`:
- Around line 60-63: The SetupWithManager method constructs a
HasInitializerPredicate from initializerName without validating it, which can
silently drop events if initializerName is empty; add a guard at the start of
LogicalClusterReconciler.SetupWithManager to return an error when
initializerName == "" (use a clear fmt.Errorf or errors.New message) so the
manager setup fails fast, and keep the rest of the function unchanged (still
build allPredicates and call r.mclifecycle.SetupWithManager).
In `@internal/predicates/initializer.go`:
- Around line 14-28: Replace the unchecked type assertions in the predicate
handlers (CreateFunc, UpdateFunc, DeleteFunc, GenericFunc) with safe type checks
before calling shouldReconcile: use the comma-ok form to assert that e.Object
(and e.ObjectNew for UpdateFunc) is a *kcpcorev1alpha1.LogicalCluster, return
false if the cast fails, and only then call shouldReconcile with the typed
value; ensure UpdateFunc handles e.ObjectNew safely and DeleteFunc/GenericFunc
also guard against nil or unexpected types to avoid panics.
- Around line 3-9: The import block in initializer.go is mis-grouped causing gci
lint failures; reorder the imports into the configured groups (standard library
first: "slices", then third-party modules like
"sigs.k8s.io/controller-runtime/pkg/event" and
"sigs.k8s.io/controller-runtime/pkg/predicate", then project/other prefixed
imports such as kcpcorev1alpha1 "github.com/kcp-dev/sdk/apis/core/v1alpha1") or
run your gci formatter (e.g. gci write -s standard -s default -s
"prefix(github.com/platform-mesh)" internal/predicates/initializer.go) to
automatically fix the grouping.
In `@internal/subroutine/account_tuples.go`:
- Around line 171-173: The error returned from mgr.ClusterFromContext(ctx) in
accountAndInfoForLogicalCluster is being discarded; update the error handling to
wrap the original err when constructing the OperatorError so the underlying
cause is preserved (i.e., pass the original err into fmt.Errorf or into
errors.NewOperatorError rather than creating a new generic error). Locate the
accountAndInfoForLogicalCluster function and the call to
mgr.ClusterFromContext(ctx) and modify the return to include the original error
(err) inside the OperatorError so callers can inspect the root cause.
In `@internal/subroutine/idp.go`:
- Line 112: The RedirectURIs assignment is appending directly to the config
slice i.additionalRedirectURLs which can mutate/shared state; instead make a
fresh slice copy of i.additionalRedirectURLs before adding the formatted URL so
the original config isn't modified (e.g., create a new slice from
i.additionalRedirectURLs and then append fmt.Sprintf("https://%s.%s/*",
workspaceName, i.baseDomain) to that copy) and assign that new slice to
RedirectURIs; update the code around the RedirectURIs field where
i.additionalRedirectURLs is referenced to use this non-mutating copy.
In `@internal/subroutine/workspace_initializer.go`:
- Around line 87-90: The returned error discards the original diagnostic
context; update the error construction where you call
w.mgr.ClusterFromContext(ctx) so the original err is wrapped/propagated into
errors.NewOperatorError (i.e., include the original err when creating the
OperatorError returned from that block). Locate the call to
w.mgr.ClusterFromContext(ctx) and change the errors.NewOperatorError invocation
to pass a wrapped error (or fmt.Errorf with %w) that includes the original err
so the underlying cause is preserved.
---
Outside diff comments:
In `@internal/subroutine/account_tuples.go`:
- Around line 116-130: The Terminate block currently creates a logical-cluster
client via iclient.NewForLogicalCluster (inside the
controllerutil.RemoveFinalizer branch); replace that with the same pattern used
in Initialize by calling s.mgr.ClusterFromContext(ctx) (or
s.mgr.ClusterFromContext to match the existing method) to obtain the
logical-cluster client and cluster ID, handle the missing-cluster case the same
way Initialize does, then call lcClient.Update(ctx, &ai) to persist the
finalizer removal; after changing the retrieval, remove the now-unused iclient,
mccontext, and logicalcluster imports if they become unused.
---
Nitpick comments:
In `@internal/controller/accountlogicalcluster_controller.go`:
- Around line 23-35: The constructor function name
NewAccountLogicalClusterReconciler does not match the returned type
AccountTypeLogicalClusterReconciler; rename the constructor to
NewAccountTypeLogicalClusterReconciler and update all call sites to use the new
name, ensuring any references to NewAccountLogicalClusterReconciler are
replaced; keep the function signature and returned struct
(AccountTypeLogicalClusterReconciler, fields log and mclifecycle built via
builder.NewBuilder(...).BuildMultiCluster(mgr)) unchanged so only the
constructor identifier is updated for consistency.
In `@internal/controller/accountlogicalcluster_initializer_controller.go`:
- Around line 23-34: The controller name still uses "Reconciler" which conflicts
with the initializer semantics; update the identifier passed into
builder.NewBuilder to use "AccountLogicalClusterInitializer" (and any other
occurrences of "AccountLogicalClusterReconciler") so logs/metrics and controller
identity reflect the initializer. Specifically, edit the
NewAccountLogicalClusterInitializer function and change the name argument in
builder.NewBuilder from "AccountLogicalClusterReconciler" to
"AccountLogicalClusterInitializer" and replace any other literal or symbol
usages of "Reconciler" related to AccountLogicalCluster with "Initializer".
In `@internal/controller/orglogicalcluster_initializer_controller.go`:
- Line 47: The builder invocation using builder.NewBuilder currently uses the
reconciler name "OrgLogicalClusterReconciler" which is incorrect for this
initializer; update the string argument to an initializer-specific name (e.g.
"OrgLogicalClusterInitializer" or "OrgLogicalClusterInitializerController") so
logs/metrics align with the controller's role where mclifecycle is constructed
via builder.NewBuilder("logicalcluster", "...", subroutines, log).
In `@internal/subroutine/account_tuples.go`:
- Line 175: Remove the extraneous trailing blank line at line 175 in
internal/subroutine/account_tuples.go to eliminate the unnecessary whitespace;
open the file, locate the blank/whitespace-only line near the surrounding
function or type declaration in account_tuples.go (e.g., within the account
tuple-related functions) and delete that empty line so the file has no trailing
blank line at that location.
In `@internal/subroutine/idp.go`:
- Around line 106-139: The CreateOrUpdate call on the idp object
(controllerutil.CreateOrUpdate with idp) already returns the updated object
in-place (including resourceVersion), so remove the redundant
cl.GetClient().Get(ctx, types.NamespacedName{Name: workspaceName}, idp) call and
its error handling; rely on the idp instance populated by CreateOrUpdate and
continue using that variable (and its metadata) for any subsequent logic or
logging to avoid the extra API call and latency.
In `@internal/subroutine/workspace_initializer.go`:
- Around line 155-158: Remove the redundant call to
w.mgr.ClusterFromContext(ctx) and the local variable cluster; reuse the
previously obtained cl variable (from the earlier ClusterFromContext call)
wherever cluster is used in this block. Replace references to cluster with cl
and remove the duplicated error handling for ClusterFromContext so there is only
the original retrieval/err check that populated cl.
ℹ️ Review info
Configuration used: Repository: platform-mesh/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
cmd/initializer.gocmd/operator.gocmd/terminator.gointernal/controller/accountlogicalcluster_controller.gointernal/controller/accountlogicalcluster_initializer_controller.gointernal/controller/orglogicalcluster_controller.gointernal/controller/orglogicalcluster_initializer_controller.gointernal/predicates/initializer.gointernal/subroutine/account_tuples.gointernal/subroutine/idp.gointernal/subroutine/workspace_initializer.go
Comment on lines
3
to
21
| import ( | ||
| "context" | ||
|
|
||
| kcpcorev1alpha1 "github.com/kcp-dev/sdk/apis/core/v1alpha1" | ||
| openfgav1 "github.com/openfga/api/proto/openfga/v1" | ||
| platformeshconfig "github.com/platform-mesh/golang-commons/config" | ||
| "github.com/platform-mesh/golang-commons/controller/lifecycle/builder" | ||
| "github.com/platform-mesh/golang-commons/controller/lifecycle/multicluster" | ||
| lifecyclesubroutine "github.com/platform-mesh/golang-commons/controller/lifecycle/subroutine" | ||
| "github.com/platform-mesh/golang-commons/logger" | ||
| "github.com/platform-mesh/security-operator/internal/config" | ||
| "github.com/platform-mesh/security-operator/internal/predicates" | ||
| "github.com/platform-mesh/security-operator/internal/subroutine" | ||
| ctrl "sigs.k8s.io/controller-runtime" | ||
| "sigs.k8s.io/controller-runtime/pkg/predicate" | ||
| mccontext "sigs.k8s.io/multicluster-runtime/pkg/context" | ||
| mcmanager "sigs.k8s.io/multicluster-runtime/pkg/manager" | ||
| mcreconcile "sigs.k8s.io/multicluster-runtime/pkg/reconcile" | ||
|
|
||
| mcclient "github.com/kcp-dev/multicluster-provider/client" | ||
| kcpcorev1alpha1 "github.com/kcp-dev/sdk/apis/core/v1alpha1" | ||
| ) |
There was a problem hiding this comment.
Fix import formatting to resolve pipeline failure.
The CI pipeline reports a gci formatting error on line 6. Reorder imports according to project conventions.
Run the project's import formatter or gci tool to auto-fix import ordering.
🧰 Tools
🪛 GitHub Actions: ci
[error] 6-6: golangci-lint: File is not properly formatted (gci). Command: 'bin/golangci-lint run --timeout 15m ./...'
🪛 GitHub Check: pipe / lint / lint
[failure] 6-6:
File is not properly formatted (gci)
🤖 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 3 - 21,
Reorder the import block in
internal/controller/accountlogicalcluster_controller.go to satisfy the project's
gci rules: run the project's import formatter (or gci) to group and sort imports
(standard library first like context, then external modules such as
kcpcorev1alpha1 and openfgav1, then github.com/platform-mesh/... and
local/internal packages), ensuring symbols like kcpcorev1alpha1, openfgav1,
platformeshconfig, builder, multicluster, lifecyclesubroutine, logger, config,
predicates, subroutine, ctrl, predicate, mccontext, mcmanager, and mcreconcile
remain imported but correctly ordered.
Comment on lines
+60
to
63
| func (r *LogicalClusterReconciler) SetupWithManager(mgr mcmanager.Manager, cfg *platformeshconfig.CommonServiceConfig, initializerName string, evp ...predicate.Predicate) error { | ||
| allPredicates := append([]predicate.Predicate{predicates.HasInitializerPredicate(initializerName)}, evp...) | ||
| return r.mclifecycle.SetupWithManager(mgr, cfg.MaxConcurrentReconciles, "LogicalCluster", &kcpcorev1alpha1.LogicalCluster{}, cfg.DebugLabelValue, r, r.log, allPredicates...) | ||
| } |
There was a problem hiding this comment.
Fail fast when initializerName is empty.
Line 61 always constructs a predicate from initializerName, but there is no guard for empty input. A blank value can silently mis-filter events and leave the reconciler effectively inactive.
Proposed fix
import (
"context"
+ "fmt"
@@
func (r *LogicalClusterReconciler) SetupWithManager(mgr mcmanager.Manager, cfg *platformeshconfig.CommonServiceConfig, initializerName string, evp ...predicate.Predicate) error {
+ if initializerName == "" {
+ return fmt.Errorf("initializerName must not be empty")
+ }
allPredicates := append([]predicate.Predicate{predicates.HasInitializerPredicate(initializerName)}, evp...)
return r.mclifecycle.SetupWithManager(mgr, cfg.MaxConcurrentReconciles, "LogicalCluster", &kcpcorev1alpha1.LogicalCluster{}, cfg.DebugLabelValue, r, r.log, allPredicates...)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func (r *LogicalClusterReconciler) SetupWithManager(mgr mcmanager.Manager, cfg *platformeshconfig.CommonServiceConfig, initializerName string, evp ...predicate.Predicate) error { | |
| allPredicates := append([]predicate.Predicate{predicates.HasInitializerPredicate(initializerName)}, evp...) | |
| return r.mclifecycle.SetupWithManager(mgr, cfg.MaxConcurrentReconciles, "LogicalCluster", &kcpcorev1alpha1.LogicalCluster{}, cfg.DebugLabelValue, r, r.log, allPredicates...) | |
| } | |
| func (r *LogicalClusterReconciler) SetupWithManager(mgr mcmanager.Manager, cfg *platformeshconfig.CommonServiceConfig, initializerName string, evp ...predicate.Predicate) error { | |
| if initializerName == "" { | |
| return fmt.Errorf("initializerName must not be empty") | |
| } | |
| allPredicates := append([]predicate.Predicate{predicates.HasInitializerPredicate(initializerName)}, evp...) | |
| return r.mclifecycle.SetupWithManager(mgr, cfg.MaxConcurrentReconciles, "LogicalCluster", &kcpcorev1alpha1.LogicalCluster{}, cfg.DebugLabelValue, r, r.log, allPredicates...) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/controller/orglogicalcluster_controller.go` around lines 60 - 63,
The SetupWithManager method constructs a HasInitializerPredicate from
initializerName without validating it, which can silently drop events if
initializerName is empty; add a guard at the start of
LogicalClusterReconciler.SetupWithManager to return an error when
initializerName == "" (use a clear fmt.Errorf or errors.New message) so the
manager setup fails fast, and keep the rest of the function unchanged (still
build allPredicates and call r.mclifecycle.SetupWithManager).
Comment on lines
3
to
9
| import ( | ||
| "slices" | ||
|
|
||
| kcpcorev1alpha1 "github.com/kcp-dev/sdk/apis/core/v1alpha1" | ||
| "sigs.k8s.io/controller-runtime/pkg/event" | ||
| "sigs.k8s.io/controller-runtime/pkg/predicate" | ||
| ) |
There was a problem hiding this comment.
Fix import formatting to resolve linter failure.
The pipeline indicates a gci formatting error on line 6. Import grouping does not match the expected format.
🔧 Proposed fix
import (
"slices"
+ kcpcorev1alpha1 "github.com/kcp-dev/sdk/apis/core/v1alpha1"
"sigs.k8s.io/controller-runtime/pkg/event"
"sigs.k8s.io/controller-runtime/pkg/predicate"
-
- kcpcorev1alpha1 "github.com/kcp-dev/sdk/apis/core/v1alpha1"
)Note: The exact grouping depends on your project's gci configuration. Run gci write -s standard -s default -s "prefix(github.com/platform-mesh)" internal/predicates/initializer.go or equivalent to auto-fix.
🧰 Tools
🪛 GitHub Check: pipe / lint / lint
[failure] 6-6:
File is not properly formatted (gci)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/predicates/initializer.go` around lines 3 - 9, The import block in
initializer.go is mis-grouped causing gci lint failures; reorder the imports
into the configured groups (standard library first: "slices", then third-party
modules like "sigs.k8s.io/controller-runtime/pkg/event" and
"sigs.k8s.io/controller-runtime/pkg/predicate", then project/other prefixed
imports such as kcpcorev1alpha1 "github.com/kcp-dev/sdk/apis/core/v1alpha1") or
run your gci formatter (e.g. gci write -s standard -s default -s
"prefix(github.com/platform-mesh)" internal/predicates/initializer.go) to
automatically fix the grouping.
Comment on lines
+14
to
+28
| CreateFunc: func(e event.CreateEvent) bool { | ||
| lc := e.Object.(*kcpcorev1alpha1.LogicalCluster) | ||
| return shouldReconcile(lc, initializer) | ||
| }, | ||
| UpdateFunc: func(e event.UpdateEvent) bool { | ||
| newLC := e.ObjectNew.(*kcpcorev1alpha1.LogicalCluster) | ||
| return shouldReconcile(newLC, initializer) | ||
| }, | ||
| DeleteFunc: func(e event.DeleteEvent) bool { | ||
| lc := e.Object.(*kcpcorev1alpha1.LogicalCluster) | ||
| return shouldReconcile(lc, initializer) | ||
| }, | ||
| GenericFunc: func(e event.GenericEvent) bool { | ||
| lc := e.Object.(*kcpcorev1alpha1.LogicalCluster) | ||
| return shouldReconcile(lc, initializer) |
There was a problem hiding this comment.
Unchecked type assertions will panic on unexpected object types.
The type assertions e.Object.(*kcpcorev1alpha1.LogicalCluster) in all event handlers will panic if a non-LogicalCluster object is passed. While this predicate is intended only for LogicalCluster watches, defensive coding with type checks prevents runtime panics from misconfiguration.
🛡️ Proposed fix with safe type assertions
CreateFunc: func(e event.CreateEvent) bool {
- lc := e.Object.(*kcpcorev1alpha1.LogicalCluster)
+ lc, ok := e.Object.(*kcpcorev1alpha1.LogicalCluster)
+ if !ok {
+ return false
+ }
return shouldReconcile(lc, initializer)
},
UpdateFunc: func(e event.UpdateEvent) bool {
- newLC := e.ObjectNew.(*kcpcorev1alpha1.LogicalCluster)
+ newLC, ok := e.ObjectNew.(*kcpcorev1alpha1.LogicalCluster)
+ if !ok {
+ return false
+ }
return shouldReconcile(newLC, initializer)
},
DeleteFunc: func(e event.DeleteEvent) bool {
- lc := e.Object.(*kcpcorev1alpha1.LogicalCluster)
+ lc, ok := e.Object.(*kcpcorev1alpha1.LogicalCluster)
+ if !ok {
+ return false
+ }
return shouldReconcile(lc, initializer)
},
GenericFunc: func(e event.GenericEvent) bool {
- lc := e.Object.(*kcpcorev1alpha1.LogicalCluster)
+ lc, ok := e.Object.(*kcpcorev1alpha1.LogicalCluster)
+ if !ok {
+ return false
+ }
return shouldReconcile(lc, initializer)
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/predicates/initializer.go` around lines 14 - 28, Replace the
unchecked type assertions in the predicate handlers (CreateFunc, UpdateFunc,
DeleteFunc, GenericFunc) with safe type checks before calling shouldReconcile:
use the comma-ok form to assert that e.Object (and e.ObjectNew for UpdateFunc)
is a *kcpcorev1alpha1.LogicalCluster, return false if the cast fails, and only
then call shouldReconcile with the typed value; ensure UpdateFunc handles
e.ObjectNew safely and DeleteFunc/GenericFunc also guard against nil or
unexpected types to avoid panics.
Comment on lines
171
to
173
| cl, err := mgr.ClusterFromContext(ctx) | ||
| if err != nil { | ||
| return accountsv1alpha1.Account{}, accountsv1alpha1.AccountInfo{}, errors.NewOperatorError(fmt.Errorf("failed to get cluster from context"), true, true) |
There was a problem hiding this comment.
Missing error wrapping in accountAndInfoForLogicalCluster.
Same issue as in workspace_initializer.go - the original error is not wrapped.
🔧 Proposed fix
cl, err := mgr.ClusterFromContext(ctx)
if err != nil {
- return accountsv1alpha1.Account{}, accountsv1alpha1.AccountInfo{}, errors.NewOperatorError(fmt.Errorf("failed to get cluster from context"), true, true)
+ return accountsv1alpha1.Account{}, accountsv1alpha1.AccountInfo{}, errors.NewOperatorError(fmt.Errorf("failed to get cluster from context: %w", err), true, true)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| cl, err := mgr.ClusterFromContext(ctx) | |
| if err != nil { | |
| return accountsv1alpha1.Account{}, accountsv1alpha1.AccountInfo{}, errors.NewOperatorError(fmt.Errorf("failed to get cluster from context"), true, true) | |
| cl, err := mgr.ClusterFromContext(ctx) | |
| if err != nil { | |
| return accountsv1alpha1.Account{}, accountsv1alpha1.AccountInfo{}, errors.NewOperatorError(fmt.Errorf("failed to get cluster from context: %w", err), true, true) | |
| } |
🤖 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 171 - 173, The error
returned from mgr.ClusterFromContext(ctx) in accountAndInfoForLogicalCluster is
being discarded; update the error handling to wrap the original err when
constructing the OperatorError so the underlying cause is preserved (i.e., pass
the original err into fmt.Errorf or into errors.NewOperatorError rather than
creating a new generic error). Locate the accountAndInfoForLogicalCluster
function and the call to mgr.ClusterFromContext(ctx) and modify the return to
include the original error (err) inside the OperatorError so callers can inspect
the root cause.
| { | ||
| ClientName: workspaceName, | ||
| ClientType: v1alpha1.IdentityProviderClientTypeConfidential, | ||
| RedirectURIs: append(i.additionalRedirectURLs, fmt.Sprintf("https://%s.%s/*", workspaceName, i.baseDomain)), |
There was a problem hiding this comment.
Potential slice mutation issue with append.
Using append(i.additionalRedirectURLs, ...) may mutate the original slice if it has sufficient capacity. Since additionalRedirectURLs comes from config and may be reused across invocations, this could cause data corruption or race conditions.
🛡️ Proposed fix to avoid mutating the config slice
- RedirectURIs: append(i.additionalRedirectURLs, fmt.Sprintf("https://%s.%s/*", workspaceName, i.baseDomain)),
+ RedirectURIs: append(append([]string{}, i.additionalRedirectURLs...), fmt.Sprintf("https://%s.%s/*", workspaceName, i.baseDomain)),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| RedirectURIs: append(i.additionalRedirectURLs, fmt.Sprintf("https://%s.%s/*", workspaceName, i.baseDomain)), | |
| RedirectURIs: append(append([]string{}, i.additionalRedirectURLs...), fmt.Sprintf("https://%s.%s/*", workspaceName, i.baseDomain)), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/subroutine/idp.go` at line 112, The RedirectURIs assignment is
appending directly to the config slice i.additionalRedirectURLs which can
mutate/shared state; instead make a fresh slice copy of i.additionalRedirectURLs
before adding the formatted URL so the original config isn't modified (e.g.,
create a new slice from i.additionalRedirectURLs and then append
fmt.Sprintf("https://%s.%s/*", workspaceName, i.baseDomain) to that copy) and
assign that new slice to RedirectURIs; update the code around the RedirectURIs
field where i.additionalRedirectURLs is referenced to use this non-mutating
copy.
Comment on lines
87
to
90
| cl, err := w.mgr.ClusterFromContext(ctx) | ||
| if err != nil { | ||
| return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("getting client: %w", err), true, true) | ||
| return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("failed to get cluster from context"), true, true) | ||
| } |
There was a problem hiding this comment.
Missing error wrapping loses diagnostic context.
The error message on line 89 does not wrap the original error, making debugging more difficult.
🔧 Proposed fix
cl, err := w.mgr.ClusterFromContext(ctx)
if err != nil {
- return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("failed to get cluster from context"), true, true)
+ return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("failed to get cluster from context: %w", err), true, true)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| cl, err := w.mgr.ClusterFromContext(ctx) | |
| if err != nil { | |
| return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("getting client: %w", err), true, true) | |
| return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("failed to get cluster from context"), true, true) | |
| } | |
| cl, err := w.mgr.ClusterFromContext(ctx) | |
| if err != nil { | |
| return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("failed to get cluster from context: %w", err), true, true) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/subroutine/workspace_initializer.go` around lines 87 - 90, The
returned error discards the original diagnostic context; update the error
construction where you call w.mgr.ClusterFromContext(ctx) so the original err is
wrapped/propagated into errors.NewOperatorError (i.e., include the original err
when creating the OperatorError returned from that block). Locate the call to
w.mgr.ClusterFromContext(ctx) and change the errors.NewOperatorError invocation
to pass a wrapped error (or fmt.Errorf with %w) that includes the original err
so the underlying cause is preserved.
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 file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
On-behalf-of: SAP aleh.yarshou@sap.com
This pr is related to #175.
Changes: