fix: added wait functionality for portal secret #123
Conversation
• Load kubeconfig from file to enhance flexibility • Update manager initialization to use the new configuration Signed-off-by: Bastian Echterhölter <[email protected]> On-behalf-of: @SAP <[email protected]>
• Introduces KCP kubeconfig handling in the config • Simplifies lifecycle manager import in the initializer Signed-off-by: Bastian Echterhölter <[email protected]> On-behalf-of: @SAP <[email protected]>
On-behalf-of: SAP [email protected]
On-behalf-of: SAP [email protected]
On-behalf-of: SAP [email protected]
WalkthroughRefactors controller reconcilers to rename Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Reasoning: Many controller field renames are repetitive (low effort) but the remove-initializer secret wait logic, command kubeconfig changes, and test updates introduce distinct behavioral changes that need focused review. Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (1)internal/controller/initializer_controller.go (6)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (6)
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 |
On-behalf-of: SAP [email protected]
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
internal/subroutine/remove_initializer.go (3)
62-64: Reconsider retry behavior for missing workspace name.If the workspace name is empty, the error is marked as retryable (
true, false), which will cause indefinite retries. However, the workspace annotation is unlikely to change between reconciliation attempts. Consider marking this as non-retryable or adding a more specific error message.- return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("failed to get workspace path"), true, false) + return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("workspace path annotation 'kcp.io/path' is missing or invalid"), false, false)
73-73: Consider making requeue interval configurable.The 5-second requeue delay is hardcoded. For large-scale deployments or slower secret creation processes, this might be too aggressive or too slow. Consider making this configurable via the config object.
70-77: Review error classification for secret Get failures.Non-NotFound errors are marked as retryable (
true, true), but some errors like permission issues are not transient and won't resolve with retries. Consider checking for specific error types or adjusting the retry classification.Example:
if err := r.runtimeClient.Get(ctx, key, &secret); err != nil { if apierrors.IsNotFound(err) { log.Info().Msg(fmt.Sprintf("portal secret %s is not ready yet, trying again", secretName)) return ctrl.Result{RequeueAfter: 5 * time.Second}, nil } // Check if error is retryable (e.g., network, temporary server errors) retryable := !apierrors.IsForbidden(err) && !apierrors.IsUnauthorized(err) log.Error().Err(err).Msg(fmt.Sprintf("failed to get portal secret %s", secretName)) return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("failed to get secret %s: %w", secretName, err), retryable, true) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/controller/initializer_controller.go(1 hunks)internal/subroutine/remove_initializer.go(3 hunks)internal/subroutine/remove_initializer_test.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
internal/controller/initializer_controller.go (1)
internal/subroutine/remove_initializer.go (1)
NewRemoveInitializer(91-97)
internal/subroutine/remove_initializer_test.go (4)
internal/subroutine/mocks/mock_Client.go (1)
NewMockClient(835-845)internal/subroutine/mocks/mock_Cluster.go (1)
NewMockCluster(509-519)internal/subroutine/remove_initializer.go (1)
NewRemoveInitializer(91-97)internal/subroutine/mocks/mock_Manager.go (1)
NewMockManager(827-837)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: pipe / dockerBuild / docker
- GitHub Check: pipe / lint / lint
🔇 Additional comments (13)
internal/controller/initializer_controller.go (1)
36-36: LGTM!The updated call correctly passes the
inClusterClientto match the newNewRemoveInitializersignature.internal/subroutine/remove_initializer_test.go (6)
12-12: LGTM!The
typesimport is correctly added to support theNamespacedNametype used in mock expectations.
47-57: LGTM!The test correctly creates a
runtimeClientmock without expectations since the early return at line 55-58 in the implementation prevents the secret check from executing when the initializer is absent.
62-80: LGTM!The test properly:
- Mocks the runtime client
- Sets expectations for the secret Get call with the correct namespace and constructed secret name
- Adds the workspace annotation so
getWorkspaceNamecan extract "test" from the path- Verifies the initializer removal flow completes successfully
90-108: LGTM!The test correctly validates the error path when status patching fails, with proper mock setup for the secret check that must succeed before reaching the patch failure.
113-121: LGTM!The miscellaneous tests are correctly updated with the new
runtimeClientparameter.
123-132: LGTM!The test correctly creates a
runtimeClientmock without expectations since the manager error causes an early return before the secret check.internal/subroutine/remove_initializer.go (6)
7-7: LGTM!The new imports are correctly added to support the secret verification logic.
Also applies to: 15-17
23-25: LGTM!The constant is appropriately defined for the portal client secret namespace.
30-30: LGTM!The
runtimeClientfield is correctly added to support runtime secret lookups.
60-77: Verify no circular dependency with secret creation.The code waits for the portal secret before removing the initializer. Ensure that the Keycloak/Crossplane provider can create the secret while the initializer is still present. If secret creation depends on the initializer being removed, this creates a deadlock scenario.
Can you confirm whether the portal secret creation by the Keycloak Crossplane provider is independent of the initializer state, or provide documentation about the initialization flow dependencies?
91-96: LGTM!The constructor correctly accepts and stores the
runtimeClientparameter.
61-64: Ignore missing helper alert for getWorkspaceName. The function is defined in internal/subroutine/realm.go:167–173.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
internal/subroutine/remove_initializer.go (2)
60-60: Fix typo in comment.The typo "untill" should be "until".
This issue was already flagged in a previous review. Based on past review comments.
70-76: Inconsistent terminology and missing test coverage.Two issues:
The log messages use "realm secret" at line 72, which is inconsistent with "portal secret" terminology used in the comment at line 60 and throughout the code.
The pipeline indicates missing test coverage for these lines, particularly for the error handling paths.
The terminology inconsistency was already flagged in a previous review. Based on past review comments.
For test coverage, ensure tests cover:
- Secret not found scenario (line 71-73)
- Other error scenarios (line 75)
- Successful secret retrieval
🧹 Nitpick comments (2)
internal/subroutine/remove_initializer.go (2)
23-25: LGTM!Defining the namespace as a constant is good practice. Consider making this configurable via environment variable or configuration if the namespace might differ across deployments.
73-73: Consider using a constant for the requeue delay.The 5-second requeue delay is hardcoded. Consider extracting it to a constant for better maintainability and to make it easier to adjust if needed.
+const ( + portalClientSecretNamespace = "platform-mesh-system" + secretNotReadyRequeueDelay = 5 * time.Second +)- return ctrl.Result{RequeueAfter: 5 * time.Second}, nil + return ctrl.Result{RequeueAfter: secretNotReadyRequeueDelay}, nil
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/subroutine/remove_initializer.go(3 hunks)
🧰 Additional context used
🪛 GitHub Actions: ci
internal/subroutine/remove_initializer.go
[error] 62-64: Uncovered lines detected by coverage tool: 62-64
[error] 70-75: Uncovered lines detected by coverage tool: 70-75
🔇 Additional comments (5)
internal/subroutine/remove_initializer.go (5)
7-7: LGTM!The new imports are necessary for the secret fetching functionality and are used appropriately throughout the code.
Also applies to: 15-17
30-30: LGTM!The
runtimeClientfield is properly integrated into the struct and used appropriately in theProcessmethod.
66-67: LGTM!The secret name construction follows a clear naming convention. The use of
types.NamespacedNameis appropriate for Kubernetes resource lookup.
90-90: LGTM! Breaking change noted.The constructor signature has been updated to accept the
runtimeClientparameter, which is properly assigned to the struct field. This is a breaking change for any existing callers ofNewRemoveInitializer, but it's necessary for the new secret-fetching functionality.Ensure all call sites have been updated to pass the
runtimeClientargument.Also applies to: 94-94
61-61: No action needed:getWorkspaceNameis defined in internal/subroutine/realm.go.
On-behalf-of: SAP [email protected]
On-behalf-of: SAP [email protected]
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
internal/subroutine/remove_initializer_test.go (2)
113-134: Add test coverage for other Get error scenarios.The test only covers the
NotFoundcase. Other errors fromruntimeClient.Get(e.g., permission denied, transient network errors) should also be tested to verify proper error handling and whether they should requeue, fail, or behave differently.
113-134: Add test case for empty workspace scenario.The AI summary mentions "if the workspace is empty it errors," but there's no test covering this case. Add a test where the
kcp.io/pathannotation is missing or empty to verify the expected error behavior.Would you like me to generate a test case for the empty workspace scenario?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/subroutine/remove_initializer_test.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/subroutine/remove_initializer_test.go (4)
internal/subroutine/mocks/mock_Client.go (1)
NewMockClient(835-845)internal/subroutine/mocks/mock_Cluster.go (1)
NewMockCluster(509-519)internal/subroutine/remove_initializer.go (1)
NewRemoveInitializer(90-96)internal/subroutine/mocks/mock_Manager.go (1)
NewMockManager(827-837)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: pipe / dockerBuild / docker
🔇 Additional comments (4)
internal/subroutine/remove_initializer_test.go (4)
6-6: LGTM!The new imports align with the added wait/requeue functionality for portal secrets.
Also applies to: 13-15
137-147: LGTM!The test correctly updates the constructor call to include the new
runtimeClientparameter.
149-158: LGTM!The test correctly handles the early error path and updates the constructor signature.
48-60: The test is correctly written. The implementation checks for the absent initializer at line 55 and returns early at line 57, before extracting the workspace name from the annotation at line 61. The test does not need thekcp.io/pathannotation because the code path requiring it is never reached when the initializer is absent.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/subroutine/remove_initializer.go (1)
62-64: Add logging for the empty workspace error case.As flagged in previous reviews, when
workspaceNameis empty, no log message is emitted before returning an error. This makes debugging harder and pipeline coverage indicates these lines are untested.Apply this diff to add logging:
if workspaceName == "" { + log.Error(). + Str("logicalCluster", lc.Name). + Str("uid", string(lc.UID)). + Msg("workspace name is empty, cannot construct portal client secret name") return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("failed to get workspace path"), true, false) }Also ensure test coverage includes this error path to address the pipeline failure.
🧹 Nitpick comments (1)
internal/subroutine/remove_initializer.go (1)
60-88: Consider operational improvements for the wait mechanism.The current implementation waits for the portal secret before removing the initializer, but lacks observability and resilience patterns:
- No metrics: Track how long secrets take to appear or how often retries occur.
- No circuit breaker: If the Keycloak crossplane provider is broken, this will requeue indefinitely without alerting.
- Tight coupling: The initializer removal is now dependent on an external system's secret creation.
Consider these improvements:
- Add a metric to track secret wait duration and retry counts
- Add a maximum retry limit (e.g., 20 retries = ~2 minutes) before failing fast with a clear error
- Emit an event on the LogicalCluster when waiting for the secret, so users can see the status
- Consider if the secret check could be moved to a separate controller or reconciliation phase
Example metric addition:
// After line 76, before continuing log.Info(). Str("secretName", secretName). Str("logicalCluster", lc.Name). Msg("portal secret found, proceeding with initializer removal") // TODO: Add metric here: portal_secret_wait_duration_seconds
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/subroutine/remove_initializer.go(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: pipe / dockerBuild / docker
🔇 Additional comments (3)
internal/subroutine/remove_initializer.go (3)
90-95: All call sites updated
All calls to NewRemoveInitializer now include the runtimeClient parameter; no further updates needed.
66-67: Ignore validation suggestion for secretName:getWorkspaceNamederives a DNS-1123-compliant label from KCP’skcp.io/pathannotation; the constructed name meets Kubernetes naming constraints.Likely an incorrect or invalid review comment.
23-25: Hardcoded namespace needs confirmation
The constantportalClientSecretNamespace = "platform-mesh-system"(remove_initializer.go:24) has no override via config or environment; confirm this is valid for all environments or make the namespace configurable.
| var secret corev1.Secret | ||
| if err := r.runtimeClient.Get(ctx, key, &secret); err != nil { | ||
| if apierrors.IsNotFound(err) { | ||
| log.Info().Msg(fmt.Sprintf("portal secret %s is not ready yet, trying again", secretName)) | ||
| return ctrl.Result{RequeueAfter: 5 * time.Second}, nil | ||
| } | ||
| return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("failed to get secret %s: %w", secretName, err), true, true) | ||
| } |
There was a problem hiding this comment.
Improve error handling and requeue mechanism.
Several concerns with the current implementation:
- Log level mismatch (line 72): Using
Infolevel for a condition that triggers a requeue. Should beDebugorWarnto indicate it's a transient condition. - No maximum retry limit: The code will requeue indefinitely every 5 seconds if the secret never appears, potentially creating noise in logs and wasting controller cycles.
- Hardcoded requeue delay: The 5-second delay has no exponential backoff and isn't configurable.
- No secret content validation: After successfully retrieving the secret, the code doesn't verify it contains expected data.
Apply these improvements:
var secret corev1.Secret
if err := r.runtimeClient.Get(ctx, key, &secret); err != nil {
if apierrors.IsNotFound(err) {
- log.Info().Msg(fmt.Sprintf("portal secret %s is not ready yet, trying again", secretName))
+ log.Debug().
+ Str("secretName", secretName).
+ Str("namespace", portalClientSecretNamespace).
+ Msg("portal secret not ready yet, requeuing")
return ctrl.Result{RequeueAfter: 5 * time.Second}, nil
}
- return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("failed to get secret %s: %w", secretName, err), true, true)
+ log.Error().
+ Err(err).
+ Str("secretName", secretName).
+ Str("namespace", portalClientSecretNamespace).
+ Msg("failed to get portal secret")
+ return ctrl.Result{}, errors.NewOperatorError(
+ fmt.Errorf("failed to get portal secret %s: %w", secretName, err),
+ true, true)
}
+
+ // Optional: Validate secret contains expected keys
+ if secret.Data == nil || len(secret.Data) == 0 {
+ log.Warn().
+ Str("secretName", secretName).
+ Msg("portal secret exists but contains no data, proceeding anyway")
+ }Consider adding a maximum retry count or timeout to prevent indefinite requeuing. This could be tracked in the LogicalCluster status or annotations.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In internal/subroutine/remove_initializer.go around lines 69 to 76, the
secret-get branch uses Info logging, always requeues every 5s with no limit or
backoff, and doesn't validate secret contents; change the log to Debug or Warn,
implement a configurable retry policy with a maximum retry count (stored in the
LogicalCluster status or an annotation) and exponential backoff for requeue
delay instead of the hardcoded 5s, increment and persist the retry counter when
the secret is missing and stop requeuing (or surface a non-retriable error) once
the max is reached, and after a successful Get validate required secret
keys/values and return a clear operator error if validation fails.
On-behalf-of: SAP [email protected]
OlegErshov
changed the title
| if err := r.runtimeClient.Get(ctx, key, &secret); err != nil { | ||
| if apierrors.IsNotFound(err) { | ||
| log.Info().Msg(fmt.Sprintf("portal secret %s is not ready yet, trying again", secretName)) | ||
| return ctrl.Result{RequeueAfter: 5 * time.Second}, nil |
There was a problem hiding this comment.
I also think it would be good to transition to return an error after a certain time has passed, that would also be the time where a sentry notification should be send and a error log should be printed. Maybe we start with a 1 minute (if more then a minute has passed since logicalcluster creation)
On-behalf-of: SAP [email protected]
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
internal/subroutine/remove_initializer.go (2)
61-64: Log the empty-workspace error for observability.Add a clear error log before returning when workspaceName is empty.
workspaceName := getWorkspaceName(lc) if workspaceName == "" { + log.Error().Str("lc", lc.Name).Msg("workspace name is empty; cannot construct portal client secret name") return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("failed to get workspace path"), true, false) }
72-79: Consistent terminology in logs and errors.Use one term everywhere (“portal client secret”) to avoid confusion.
- return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("keycloak client secret %s was not created within 1m", secretName), true, true) + return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("portal client secret %s was not created within 1m", secretName), true, true)internal/subroutine/remove_initializer_test.go (1)
70-71: Don’t hardcode the secret name; derive via the same helper as production.Use subroutine.PortalClientSecretName(workspace) to build NamespacedName so tests don’t drift from implementation.
- runtimeClient.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "portal-client-secret-test", Namespace: subroutine.PortalClientSecretNamespace}, mock.Anything).Return(nil) + secret := types.NamespacedName{ + Name: subroutine.PortalClientSecretName("test"), + Namespace: subroutine.PortalClientSecretNamespace, + } + runtimeClient.EXPECT().Get(mock.Anything, secret, mock.Anything).Return(nil) @@ - runtimeClient.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "portal-client-secret-test", Namespace: subroutine.PortalClientSecretNamespace}, mock.Anything).Return(nil) + secret := types.NamespacedName{ + Name: subroutine.PortalClientSecretName("test"), + Namespace: subroutine.PortalClientSecretNamespace, + } + runtimeClient.EXPECT().Get(mock.Anything, secret, mock.Anything).Return(nil) @@ - runtimeClient.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "portal-client-secret-test", Namespace: subroutine.PortalClientSecretNamespace}, mock.Anything).Return(apierrors.NewNotFound(schema.GroupResource{Group: "", Resource: "secrets"}, "portal-client-secret-test")) + secret := types.NamespacedName{ + Name: subroutine.PortalClientSecretName("test"), + Namespace: subroutine.PortalClientSecretNamespace, + } + runtimeClient.EXPECT().Get(mock.Anything, secret, mock.Anything). + Return(apierrors.NewNotFound(schema.GroupResource{Group: "", Resource: "secrets"}, secret.Name)) @@ - runtimeClient.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "portal-client-secret-test", Namespace: subroutine.PortalClientSecretNamespace}, mock.Anything).Return(apierrors.NewNotFound(schema.GroupResource{Group: "", Resource: "secrets"}, "portal-client-secret-test")) + secret := types.NamespacedName{ + Name: subroutine.PortalClientSecretName("test"), + Namespace: subroutine.PortalClientSecretNamespace, + } + runtimeClient.EXPECT().Get(mock.Anything, secret, mock.Anything). + Return(apierrors.NewNotFound(schema.GroupResource{Group: "", Resource: "secrets"}, secret.Name))Also applies to: 98-99, 120-121, 142-143
🧹 Nitpick comments (5)
internal/subroutine/remove_initializer.go (4)
23-26: Avoid hardcoded namespace; add a shared name helper.Make the namespace configurable and expose a helper for the secret name to keep impl/tests in sync.
-const ( - PortalClientSecretNamespace = "platform-mesh-system" -) +const ( + // Default namespace; consider overriding via config/env/flags. + PortalClientSecretNamespace = "platform-mesh-system" + PortalClientSecretPrefix = "portal-client-secret-" +) + +// PortalClientSecretName returns the fully-qualified Secret name for a workspace. +func PortalClientSecretName(workspace string) string { + return PortalClientSecretPrefix + workspace +}
72-79: Tighten transient handling: zero timestamp guard, log level, consistent wording, and error log.
- Treat zero CreationTimestamp as “new” to avoid premature 1m timeout.
- Use Debug for NotFound requeue to reduce noise; include namespace.
- Standardize on “portal client secret”.
- Log non-NotFound errors with context.
- if apierrors.IsNotFound(err) { - age := time.Since(lc.CreationTimestamp.Time) - if age <= time.Minute { - log.Info().Str("secret", secretName).Msg("portal secret not ready yet, requeueing") - return ctrl.Result{RequeueAfter: 5 * time.Second}, nil - } - return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("keycloak client secret %s was not created within 1m", secretName), true, true) - } - return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("failed to get secret %s: %w", secretName, err), true, true) + if apierrors.IsNotFound(err) { + age := time.Duration(0) + if !lc.CreationTimestamp.IsZero() { + age = time.Since(lc.CreationTimestamp.Time) + } + if age <= time.Minute { + log.Debug(). + Str("secret", secretName). + Str("namespace", PortalClientSecretNamespace). + Msg("portal client secret not ready yet; requeuing") + return ctrl.Result{RequeueAfter: 5 * time.Second}, nil + } + return ctrl.Result{}, errors.NewOperatorError( + fmt.Errorf("portal client secret %s was not created within 1m", secretName), + true, true) + } + log.Error(). + Err(err). + Str("secret", secretName). + Str("namespace", PortalClientSecretNamespace). + Msg("failed to get portal client secret") + return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("failed to get portal client secret %s: %w", secretName, err), true, true)
89-89: Prefer structured logging over fmt.Sprintf.Emit fields for name/uid instead of formatting into the message.
-log.Info().Msg(fmt.Sprintf("Removed initializer from LogicalCluster status, name %s,uuid %s", lc.Name, lc.UID)) +log.Info(). + Str("logicalCluster", lc.Name). + Str("uid", string(lc.UID)). + Msg("removed initializer from LogicalCluster status")
66-70: Centralize secret name building.Now that a helper exists, use it to avoid repeated format strings.
- secretName := fmt.Sprintf("portal-client-secret-%s", workspaceName) + secretName := PortalClientSecretName(workspaceName)internal/subroutine/remove_initializer_test.go (1)
122-133: Add tests for empty workspace and zero CreationTimestamp behavior.
- Empty workspace should return OperatorError and not call Status().Patch.
- If you adopt the zero timestamp guard, ensure NotFound under 1m still requeues when CreationTimestamp is zero.
Example additions:
t.Run("errors when workspace is empty", func(t *testing.T) { mgr := mocks.NewMockManager(t) cluster := mocks.NewMockCluster(t) runtimeClient := mocks.NewMockClient(t) mgr.EXPECT().ClusterFromContext(mock.Anything).Return(cluster, nil) lc := &kcpv1alpha1.LogicalCluster{} lc.Status.Initializers = []kcpv1alpha1.LogicalClusterInitializer{ kcpv1alpha1.LogicalClusterInitializer(initializerName), } // No kcp.io/path annotation r := subroutine.NewRemoveInitializer(mgr, initializerName, runtimeClient) _, err := r.Process(context.Background(), lc) assert.NotNil(t, err) }) t.Run("requeues when CreationTimestamp is zero and secret NotFound", func(t *testing.T) { mgr := mocks.NewMockManager(t) cluster := mocks.NewMockCluster(t) runtimeClient := mocks.NewMockClient(t) mgr.EXPECT().ClusterFromContext(mock.Anything).Return(cluster, nil) secret := types.NamespacedName{ Name: subroutine.PortalClientSecretName("test"), Namespace: subroutine.PortalClientSecretNamespace, } runtimeClient.EXPECT().Get(mock.Anything, secret, mock.Anything). Return(apierrors.NewNotFound(schema.GroupResource{Group: "", Resource: "secrets"}, secret.Name)) lc := &kcpv1alpha1.LogicalCluster{} lc.Status.Initializers = []kcpv1alpha1.LogicalClusterInitializer{ kcpv1alpha1.LogicalClusterInitializer(initializerName), } lc.Annotations = map[string]string{"kcp.io/path": "root:org:test"} // CreationTimestamp is zero by default r := subroutine.NewRemoveInitializer(mgr, initializerName, runtimeClient) res, err := r.Process(context.Background(), lc) assert.Nil(t, err) assert.Equal(t, 5*time.Second, res.RequeueAfter) })Also applies to: 144-153
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/subroutine/remove_initializer.go(3 hunks)internal/subroutine/remove_initializer_test.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/subroutine/remove_initializer_test.go (4)
internal/subroutine/mocks/mock_Client.go (1)
NewMockClient(835-845)internal/subroutine/mocks/mock_Cluster.go (1)
NewMockCluster(509-519)internal/subroutine/remove_initializer.go (2)
NewRemoveInitializer(94-100)PortalClientSecretNamespace(24-24)internal/subroutine/mocks/mock_Manager.go (1)
NewMockManager(827-837)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: pipe / dockerBuild / docker
| runtimeClient client.Client | ||
| } |
There was a problem hiding this comment.
Defensive nil check for runtimeClient to prevent panics.
If runtimeClient is ever nil, r.runtimeClient.Get will panic.
func (r *removeInitializer) Process(ctx context.Context, instance runtimeobject.RuntimeObject) (ctrl.Result, errors.OperatorError) {
@@
- secretName := fmt.Sprintf("portal-client-secret-%s", workspaceName)
+ secretName := fmt.Sprintf("portal-client-secret-%s", workspaceName)
key := types.NamespacedName{Name: secretName, Namespace: PortalClientSecretNamespace}
+ if r.runtimeClient == nil {
+ return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("runtime client is not configured"), false, true)
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In internal/subroutine/remove_initializer.go around lines 30 to 31, add a
defensive nil check for r.runtimeClient before any use: if r.runtimeClient is
nil return an appropriate error (or wrap and return context-aware error) instead
of calling r.runtimeClient.Get which would panic; update the function to detect
nil early, return the error up the call stack, and ensure callers handle that
error.
On-behalf-of: SAP [email protected]
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
internal/subroutine/remove_initializer.go (2)
64-66: Log the empty-workspace error for observability.Emit an error log before returning to aid debugging and satisfy coverage goals noted earlier.
if workspaceName == "" { - return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("failed to get workspace path"), true, false) + log.Error().Msg("workspace name is empty, cannot construct portal client secret name") + return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("failed to get workspace path"), true, false) }
71-73: Add nil check for runtimeClient before Get() to avoid panic.A nil runtimeClient will panic on Get. Fail fast with a clear OperatorError.
Apply this diff:
key := types.NamespacedName{Name: secretName, Namespace: PortalClientSecretNamespace} + if r.runtimeClient == nil { + return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("runtime client is not configured"), false, true) + } + var secret corev1.Secret if err := r.runtimeClient.Get(ctx, key, &secret); err != nil {internal/subroutine/remove_initializer_test.go (1)
71-72: Remove hardcoded secret name; derive it to mirror production logic.Compute expected name from workspace ("test") or share a helper to avoid drift.
- runtimeClient.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "portal-client-secret-test", Namespace: subroutine.PortalClientSecretNamespace}, mock.Anything).Return(nil) + runtimeClient.EXPECT().Get( + mock.Anything, + types.NamespacedName{Name: fmt.Sprintf("portal-client-secret-%s", "test"), Namespace: subroutine.PortalClientSecretNamespace}, + mock.Anything, + ).Return(nil)Also applies to: 99-100, 121-123, 143-144
🧹 Nitpick comments (5)
internal/config/config.go (1)
23-23: Consider using time.Duration or validate non-positive values.Model the timeout as time.Duration (e.g., "1m") or ensure values <= 0 are sanitized to a safe default during construction.
internal/subroutine/remove_initializer.go (2)
74-79: Guard zero CreationTimestamp to avoid immediate timeout on fresh objects.If CreationTimestamp is zero, age becomes huge; treat as 0 to allow initial requeues.
- age := time.Since(lc.CreationTimestamp.Time) + age := time.Since(lc.CreationTimestamp.Time) + if lc.CreationTimestamp.IsZero() { + age = 0 + } if age <= r.secretWaitTimeout {
96-102: Clamp non-positive SecretWaitingTimeoutInSeconds to a safe default.Prevents accidental 0/negative timeouts from disabling the wait logic.
-func NewRemoveInitializer(mgr mcmanager.Manager, cfg config.Config, runtimeClient client.Client) *removeInitializer { - return &removeInitializer{ - initializerName: cfg.InitializerName, - mgr: mgr, - runtimeClient: runtimeClient, - secretWaitTimeout: time.Duration(cfg.SecretWaitingTimeoutInSeconds) * time.Second, - } -} +func NewRemoveInitializer(mgr mcmanager.Manager, cfg config.Config, runtimeClient client.Client) *removeInitializer { + timeoutSec := cfg.SecretWaitingTimeoutInSeconds + if timeoutSec <= 0 { + timeoutSec = 60 + } + return &removeInitializer{ + initializerName: cfg.InitializerName, + mgr: mgr, + runtimeClient: runtimeClient, + secretWaitTimeout: time.Duration(timeoutSec) * time.Second, + } +}internal/subroutine/remove_initializer_test.go (2)
119-131: Add test for empty workspace annotation path.Covers the early error path and improves stability.
t.Run("errors when secret not found after 1 minute", func(t *testing.T) { ... }) + +t.Run("errors when workspace annotation is missing", func(t *testing.T) { + mgr := mocks.NewMockManager(t) + cluster := mocks.NewMockCluster(t) + runtimeClient := mocks.NewMockClient(t) + mgr.EXPECT().ClusterFromContext(mock.Anything).Return(cluster, nil) + + lc := &kcpv1alpha1.LogicalCluster{} + lc.Status.Initializers = []kcpv1alpha1.LogicalClusterInitializer{ + kcpv1alpha1.LogicalClusterInitializer(initializerName), + } + // no annotations -> workspaceName == "" + + r := subroutine.NewRemoveInitializer(mgr, + config.Config{InitializerName: initializerName, SecretWaitingTimeoutInSeconds: 60}, + runtimeClient) + _, err := r.Process(context.Background(), lc) + assert.NotNil(t, err) +})
46-61: Add test for nil runtimeClient (paired with nil-check fix).Ensures Process returns a clear OperatorError instead of panicking.
t.Run("skips when initializer is absent", func(t *testing.T) { ... }) + +t.Run("returns error when runtime client is nil", func(t *testing.T) { + mgr := mocks.NewMockManager(t) + cluster := mocks.NewMockCluster(t) + mgr.EXPECT().ClusterFromContext(mock.Anything).Return(cluster, nil) + + lc := &kcpv1alpha1.LogicalCluster{} + lc.Status.Initializers = []kcpv1alpha1.LogicalClusterInitializer{ + kcpv1alpha1.LogicalClusterInitializer(initializerName), + } + lc.Annotations = map[string]string{"kcp.io/path": "root:org:test"} + + r := subroutine.NewRemoveInitializer(mgr, + config.Config{InitializerName: initializerName, SecretWaitingTimeoutInSeconds: 60}, + /* runtimeClient */ nil) + _, err := r.Process(context.Background(), lc) + assert.NotNil(t, err) +})
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/config/config.go(1 hunks)internal/controller/initializer_controller.go(1 hunks)internal/subroutine/remove_initializer.go(3 hunks)internal/subroutine/remove_initializer_test.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
internal/controller/initializer_controller.go (1)
internal/subroutine/remove_initializer.go (1)
NewRemoveInitializer(96-103)
internal/subroutine/remove_initializer_test.go (5)
internal/subroutine/mocks/mock_Client.go (1)
NewMockClient(835-845)internal/subroutine/mocks/mock_Cluster.go (1)
NewMockCluster(509-519)internal/subroutine/remove_initializer.go (2)
NewRemoveInitializer(96-103)PortalClientSecretNamespace(25-25)internal/config/config.go (1)
Config(11-42)internal/subroutine/mocks/mock_Manager.go (1)
NewMockManager(827-837)
internal/subroutine/remove_initializer.go (1)
internal/config/config.go (1)
Config(11-42)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: pipe / lint / lint
- GitHub Check: pipe / dockerBuild / docker
🔇 Additional comments (1)
internal/controller/initializer_controller.go (1)
36-37: Call site is correct; inClusterClient is non-nil and safely scoped.The
NewRemoveInitializer(mgr, cfg, inClusterClient)call matches the function signature exactly. TheinClusterClientis constructed with error handling incmd/initializer.go(lines 91–94), and the program exits if creation fails, guaranteeing it is non-nil and points to the in-cluster API when passed to the controller. No downstream panic risk.
…t-is-created On-behalf-of: SAP [email protected]
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/initializer.go (1)
34-38: Consider returning error instead of os.Exit(1).While the kubeconfig loading is correct, Line 37 calls
os.Exit(1)instead of returning the error. For consistency with other error handling in this file (lines 59, 94, etc.), consider returning the error and letting the cobra framework handle the exit.Apply this diff:
restCfg, err := getKubeconfigFromPath(initializerCfg.KCP.Kubeconfig) if err != nil { log.Error().Err(err).Msg("unable to get KCP kubeconfig") - os.Exit(1) + return err }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
cmd/initializer.go(5 hunks)cmd/model_generator.go(2 hunks)cmd/operator.go(2 hunks)cmd/root.go(5 hunks)internal/config/config.go(2 hunks)internal/controller/apibinding_controller.go(2 hunks)internal/controller/authorization_model_controller.go(3 hunks)internal/controller/initializer_controller.go(3 hunks)internal/controller/invite_controller.go(4 hunks)internal/controller/store_controller.go(4 hunks)internal/subroutine/tuples.go(1 hunks)internal/subroutine/workspace_initializer.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- internal/subroutine/tuples.go
🧰 Additional context used
🧬 Code graph analysis (5)
cmd/initializer.go (2)
internal/subroutine/invite/subroutine.go (1)
New(45-68)internal/controller/initializer_controller.go (1)
NewLogicalClusterReconciler(29-41)
internal/controller/authorization_model_controller.go (1)
api/v1alpha1/authorizationmodel_types.go (1)
AuthorizationModel(31-37)
internal/controller/apibinding_controller.go (1)
internal/subroutine/authorization_model_generation.go (1)
NewAuthorizationModelGenerationSubroutine(28-32)
internal/controller/initializer_controller.go (2)
internal/config/config.go (1)
Config(11-45)internal/subroutine/remove_initializer.go (1)
NewRemoveInitializer(96-103)
cmd/root.go (1)
internal/config/config.go (1)
Config(11-45)
🔇 Additional comments (29)
internal/subroutine/workspace_initializer.go (1)
27-27: LGTM! Clean refactor.Removing the intermediate variable and reading directly from
cfg.CoreModulePathimproves code clarity without changing behavior.cmd/model_generator.go (2)
34-38: LGTM! Proper error handling for kubeconfig loading.The shift from
ctrl.GetConfigOrDie()to path-based kubeconfig loading with explicit error handling improves resilience and aligns with the PR's broader kubeconfig management strategy.
73-81: LGTM! Consistent propagation of restCfg.Both
apiexport.Newandmcmanager.Newnow receive therestCfgobtained from the kubeconfig path, ensuring consistent configuration across components.internal/controller/apibinding_controller.go (3)
14-14: LGTM! Import path update.The import now directly references the multicluster package, aligning with the broader refactoring pattern across controllers.
21-28: LGTM! Consistent field naming.Renaming the lifecycle manager field to
mclifecycleimproves clarity by making the multicluster nature explicit.
31-43: LGTM! Updated to use mclifecycle.All references properly updated to use the renamed
mclifecyclefield, maintaining consistency throughout the struct and its methods.internal/controller/authorization_model_controller.go (3)
9-9: LGTM! Import path alignment.Consistent with other controllers, this updates the import to directly reference the multicluster package.
21-34: LGTM! Field renamed to mclifecycle.The rename from
lifecycletomclifecycleis consistently applied in the struct declaration and constructor, improving code clarity.
36-43: LGTM! Method calls updated.All method invocations correctly reference the renamed
mclifecyclefield.cmd/operator.go (2)
80-84: LGTM! Path-based kubeconfig loading.Consistent with the broader pattern, this replaces in-cluster config retrieval with path-based loading and adds proper error handling.
128-140: LGTM! Consistent restCfg usage.Both the provider and manager now use the
restCfgobtained from the kubeconfig path, maintaining consistency with other command entrypoints.cmd/root.go (5)
4-4: LGTM! Required import added.The
errorspackage is now properly imported for use ingetKubeconfigFromPath.
13-14: LGTM! K8s client imports added.The
restandclientcmdpackages are required for the new kubeconfig loading functionality.
24-24: LGTM! Generator config variable added.The
generatorCfgvariable is properly declared to support the model generator command configuration.
50-53: LGTM! Generator config binding.The generator configuration is properly bound to flags using a dedicated Viper instance, consistent with the pattern used for operator and initializer configs.
62-75: LGTM! Robust kubeconfig loading implementation.The
getKubeconfigFromPathfunction provides proper validation and error handling:
- Validates that the path is not empty
- Loads the kubeconfig file with error handling
- Builds the REST config using clientcmd
- Returns appropriate errors at each step
internal/config/config.go (2)
15-17: LGTM! KCP kubeconfig configuration added.The new
KCPstruct with aKubeconfigfield (defaulting to/api-kubeconfig/kubeconfig) properly supports the path-based kubeconfig loading pattern introduced in this PR.
26-26: LGTM! Secret waiting timeout configuration added.The
SecretWaitingTimeoutInSecondsfield (defaulting to 60 seconds) aligns with the PR's objective to add wait functionality for portal secrets.cmd/initializer.go (3)
64-73: LGTM! Dual configuration approach.The use of
restCfg(from path-based KCP kubeconfig) for the provider and manager initialization is appropriate for multicluster operations.
89-95: LGTM! Runtime client initialization.Creating a separate
runtimeClientfrom the in-cluster config (k8sCfg) is appropriate for runtime operations distinct from KCP multicluster management. This supports the secret-waiting functionality mentioned in the PR objectives.
101-105: LGTM! Runtime client propagation.Passing the
runtimeClientto theLogicalClusterReconcilerenables the secret-waiting functionality that is the core objective of this PR.internal/controller/store_controller.go (3)
20-20: LGTM: Consistent refactoring to multicluster lifecycle manager.The import path and field declaration have been updated consistently to use the direct multicluster package and the renamed
mclifecyclefield.Also applies to: 33-35
68-68: LGTM: Constructor updated correctly.The lifecycle manager is now correctly assigned to the
mclifecyclefield.
79-79: LGTM: All method calls updated consistently.The
ReconcileandSetupWithManagerBuildercalls correctly reference the renamedmclifecyclefield.Also applies to: 84-84
internal/controller/initializer_controller.go (3)
9-9: LGTM: Consistent lifecycle manager refactoring.The import and field declaration follow the same refactoring pattern applied across all controllers.
Also applies to: 26-26
36-36: LGTM: NewRemoveInitializer signature correctly updated.The call now passes
cfgandinClusterClient, which aligns with the PR objective to add wait functionality for portal secrets. The signature matches the updated definition shown in the relevant snippets.
32-32: LGTM: All lifecycle manager references updated consistently.The field assignment and method calls correctly use the renamed
mclifecyclefield throughout.Also applies to: 45-45, 49-49
internal/controller/invite_controller.go (2)
9-9: LGTM: Consistent refactoring pattern applied.The import and field declaration match the refactoring pattern used in the other controllers.
Also applies to: 23-23
38-38: LGTM: All references updated consistently.The lifecycle manager field is correctly assigned and referenced throughout using the new
mclifecyclename.Also applies to: 49-49, 53-53
On-behalf-of: SAP [email protected]
On-behalf-of: SAP [email protected]
The PR is related to #122
Summary by CodeRabbit
New Features
Bug Fixes
Chores