Remove default shared secret name and add workaround #191
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: slintes The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughRemoved CRD default for SharedSecretName, added a controller workaround that sets/clears the field based on the legacy secret's existence, unified secret-parameter collection by removing the Changes
Sequence Diagram(s)sequenceDiagram
participant Reconciler as Controller.Reconcile
participant FAR as FenceAgentsRemediation (CR)
participant K8s as Kubernetes API (Secrets)
participant Updater as Controller.Update
Reconciler->>FAR: read FAR
Reconciler->>K8s: Get Secret "fence-agents-credentials-shared"
alt secret exists and FAR.SharedSecretName == nil
Reconciler->>FAR: set SharedSecretName = "fence-agents-credentials-shared"
Reconciler->>Updater: update FAR
Updater-->>Reconciler: update OK
Reconciler-->>Reconciler: requeue
else secret missing and FAR.SharedSecretName == "fence-agents-credentials-shared"
Reconciler->>FAR: clear SharedSecretName
Reconciler->>Updater: update FAR
Updater-->>Reconciler: update OK
Reconciler-->>Reconciler: requeue
else
Reconciler->>K8s: proceed with collectSecretParams for required secrets
Reconciler-->>Reconciler: continue reconcile flow
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 2
🤖 Fix all issues with AI agents
In @api/v1alpha1/fenceagentsremediation_params.go:
- Around line 379-391: The comment for collectSecretParams is outdated: update
the function-level comment to remove the claim that IsNotFound is ignored for
shared secrets and instead state that IsNotFound errors are returned as errors
(i.e., the function returns an error when the secret is not found), ensuring the
comment accurately describes current behavior and references
collectSecretParams, k8sClient.Get and the NotFound handling.
In @controllers/fenceagentsremediation_controller.go:
- Line 134: Fix the typo in the inline comment: change "desription" to
"description" in the comment line that reads "// Apply the shared secret default
name workaround. See function desription for details." in
controllers/fenceagentsremediation_controller.go so the comment correctly
references "description".
🧹 Nitpick comments (3)
controllers/fenceagentsremediation_controller.go (2)
134-138: Verify reconciliation behavior after CR update.The workaround is applied after node validation but before other processing. When
applySharedSecretDefaultNameWorkaroundupdates the CR (lines 413-416), the reconcile continues with the in-memoryfarobject that now has modifiedSharedSecretName. However, theUpdatecall triggers a new reconcile, leading to redundant processing.Consider returning
requeueImmediatelyafter the workaround modifies and updates the CR to ensure the next reconcile starts with a fresh object from the API server.♻️ Suggested approach
You could modify
applySharedSecretDefaultNameWorkaroundto return a boolean indicating whether an update was made, then requeue:- // Apply the shared secret default name workaround. See function desription for details. - if err := applySharedSecretDefaultNameWorkaround(far, r.Client, r.Log); err != nil { + // Apply the shared secret default name workaround. See function description for details. + updated, err := applySharedSecretDefaultNameWorkaround(far, r.Client, r.Log) + if err != nil { r.Log.Error(err, "Failed to apply shared secret default name workaround") return emptyResult, err } + if updated { + return requeueImmediately, nil + }And update the function signature to return
(bool, error).
388-420: Consider using the reconcile context instead ofcontext.Background().Lines 393 and 414 use
context.Background()for API calls. While this works, using the context passed from theReconcilefunction would allow these operations to be properly cancelled if the reconcile is interrupted (e.g., during controller shutdown).This function is called from
Reconcilewhich has access toctx. Consider passing the context as a parameter.♻️ Suggested change
-func applySharedSecretDefaultNameWorkaround(far *v1alpha1.FenceAgentsRemediation, c client.Client, log logr.Logger) error { +func applySharedSecretDefaultNameWorkaround(ctx context.Context, far *v1alpha1.FenceAgentsRemediation, c client.Client, log logr.Logger) error { // Check if the secret with the old default name exists secret := &corev1.Secret{} secretKey := client.ObjectKey{Name: OldDefaultSecretName, Namespace: far.Namespace} secretExists := true - if err := c.Get(context.Background(), secretKey, secret); err != nil { + if err := c.Get(ctx, secretKey, secret); err != nil { if !apiErrors.IsNotFound(err) { return fmt.Errorf("failed to check for shared secret: %w", err) } secretExists = false } // ... if needsUpdate { - if err := c.Update(context.Background(), far); err != nil { + if err := c.Update(ctx, far); err != nil { return fmt.Errorf("failed to update FAR with SharedSecretName workaround: %w", err) } }controllers/fenceagentsremediation_controller_test.go (1)
178-181: Consider usingEventuallyfor async assertions.The workaround logic runs during reconciliation, which is asynchronous. While there's a 1-second sleep in
JustBeforeEach, usingEventuallywould make these tests more robust against timing variations, consistent with patterns used elsewhere in this file (e.g., lines 233-245).♻️ Suggested refactor for more robust async assertion
It("should keep the name", func() { - Expect(k8sClient.Get(context.Background(), client.ObjectKeyFromObject(underTestFAR), underTestFAR)).To(Succeed()) - Expect(*underTestFAR.Spec.SharedSecretName).To(Equal(OldDefaultSecretName)) + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(context.Background(), client.ObjectKeyFromObject(underTestFAR), underTestFAR)).To(Succeed()) + g.Expect(*underTestFAR.Spec.SharedSecretName).To(Equal(OldDefaultSecretName)) + }, timeoutPreRemediation, pollInterval).Should(Succeed()) })Apply similar pattern to the other three
Itblocks (lines 188-191, 199-202, 209-212).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
api/v1alpha1/fenceagentsremediation_params.goapi/v1alpha1/fenceagentsremediation_types.goapi/v1alpha1/fenceagentsremediationtemplate_webhook_test.gobundle/manifests/fence-agents-remediation.clusterserviceversion.yamlbundle/manifests/fence-agents-remediation.medik8s.io_fenceagentsremediations.yamlbundle/manifests/fence-agents-remediation.medik8s.io_fenceagentsremediationtemplates.yamlconfig/crd/bases/fence-agents-remediation.medik8s.io_fenceagentsremediations.yamlconfig/crd/bases/fence-agents-remediation.medik8s.io_fenceagentsremediationtemplates.yamlconfig/manifests/bases/fence-agents-remediation.clusterserviceversion.yamlcontrollers/fenceagentsremediation_controller.gocontrollers/fenceagentsremediation_controller_test.go
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: slintes
Repo: medik8s/fence-agents-remediation PR: 171
File: controllers/fenceagentsremediation_controller_test.go:153-181
Timestamp: 2025-06-10T17:25:54.191Z
Learning: In the FenceAgentsRemediation controller, validateUniqueParam is not applied for conflicts between secret parameters from different sources (shared secret vs node secret). Node secret parameters are allowed to override shared secret parameters without validation errors.
Learnt from: slintes
Repo: medik8s/fence-agents-remediation PR: 171
File: controllers/fenceagentsremediation_controller_test.go:153-181
Timestamp: 2025-06-10T17:25:54.191Z
Learning: In FenceAgentsRemediation controller, validateUniqueParam is applied to secret parameters to check conflicts with non-secret parameters (shared and node params), but not for conflicts within secret parameters themselves. Node secret parameters override shared secret parameters during the collection phase via maps.Copy() before validation occurs.
Learnt from: slintes
Repo: medik8s/fence-agents-remediation PR: 178
File: config/manager/manager.yaml:72-87
Timestamp: 2025-08-29T14:25:35.903Z
Learning: In fence-agents-remediation operator, kustomize adds app.kubernetes.io/name: fence-agents-remediation-operator labels to all resources via commonLabels in config/default/kustomization.yaml, so the podAntiAffinity selector that requires both control-plane=controller-manager and app.kubernetes.io/name=fence-agents-remediation-operator labels will work correctly even though the base config/manager/manager.yaml only shows the control-plane label.
📚 Learning: 2025-08-29T14:25:35.903Z
Learnt from: slintes
Repo: medik8s/fence-agents-remediation PR: 178
File: config/manager/manager.yaml:72-87
Timestamp: 2025-08-29T14:25:35.903Z
Learning: In fence-agents-remediation operator, kustomize adds app.kubernetes.io/name: fence-agents-remediation-operator labels to all resources via commonLabels in config/default/kustomization.yaml, so the podAntiAffinity selector that requires both control-plane=controller-manager and app.kubernetes.io/name=fence-agents-remediation-operator labels will work correctly even though the base config/manager/manager.yaml only shows the control-plane label.
Applied to files:
bundle/manifests/fence-agents-remediation.medik8s.io_fenceagentsremediationtemplates.yamlconfig/manifests/bases/fence-agents-remediation.clusterserviceversion.yamlapi/v1alpha1/fenceagentsremediation_types.goconfig/crd/bases/fence-agents-remediation.medik8s.io_fenceagentsremediationtemplates.yamlcontrollers/fenceagentsremediation_controller.gobundle/manifests/fence-agents-remediation.medik8s.io_fenceagentsremediations.yamlbundle/manifests/fence-agents-remediation.clusterserviceversion.yamlconfig/crd/bases/fence-agents-remediation.medik8s.io_fenceagentsremediations.yaml
📚 Learning: 2025-06-10T17:25:54.191Z
Learnt from: slintes
Repo: medik8s/fence-agents-remediation PR: 171
File: controllers/fenceagentsremediation_controller_test.go:153-181
Timestamp: 2025-06-10T17:25:54.191Z
Learning: In FenceAgentsRemediation controller, validateUniqueParam is applied to secret parameters to check conflicts with non-secret parameters (shared and node params), but not for conflicts within secret parameters themselves. Node secret parameters override shared secret parameters during the collection phase via maps.Copy() before validation occurs.
Applied to files:
bundle/manifests/fence-agents-remediation.medik8s.io_fenceagentsremediationtemplates.yamlconfig/manifests/bases/fence-agents-remediation.clusterserviceversion.yamlapi/v1alpha1/fenceagentsremediation_types.goconfig/crd/bases/fence-agents-remediation.medik8s.io_fenceagentsremediationtemplates.yamlapi/v1alpha1/fenceagentsremediation_params.gocontrollers/fenceagentsremediation_controller.gobundle/manifests/fence-agents-remediation.medik8s.io_fenceagentsremediations.yamlbundle/manifests/fence-agents-remediation.clusterserviceversion.yamlcontrollers/fenceagentsremediation_controller_test.goapi/v1alpha1/fenceagentsremediationtemplate_webhook_test.goconfig/crd/bases/fence-agents-remediation.medik8s.io_fenceagentsremediations.yaml
📚 Learning: 2025-06-10T17:25:54.191Z
Learnt from: slintes
Repo: medik8s/fence-agents-remediation PR: 171
File: controllers/fenceagentsremediation_controller_test.go:153-181
Timestamp: 2025-06-10T17:25:54.191Z
Learning: In the FenceAgentsRemediation controller, validateUniqueParam is not applied for conflicts between secret parameters from different sources (shared secret vs node secret). Node secret parameters are allowed to override shared secret parameters without validation errors.
Applied to files:
bundle/manifests/fence-agents-remediation.medik8s.io_fenceagentsremediationtemplates.yamlconfig/manifests/bases/fence-agents-remediation.clusterserviceversion.yamlapi/v1alpha1/fenceagentsremediation_types.goconfig/crd/bases/fence-agents-remediation.medik8s.io_fenceagentsremediationtemplates.yamlapi/v1alpha1/fenceagentsremediation_params.gocontrollers/fenceagentsremediation_controller.gobundle/manifests/fence-agents-remediation.medik8s.io_fenceagentsremediations.yamlbundle/manifests/fence-agents-remediation.clusterserviceversion.yamlcontrollers/fenceagentsremediation_controller_test.goconfig/crd/bases/fence-agents-remediation.medik8s.io_fenceagentsremediations.yaml
📚 Learning: 2026-01-06T10:20:03.052Z
Learnt from: mshitrit
Repo: medik8s/fence-agents-remediation PR: 190
File: controllers/fenceagentsremediationtemplate_controller.go:189-189
Timestamp: 2026-01-06T10:20:03.052Z
Learning: In the FenceAgentsRemediationTemplate controller (controllers/fenceagentsremediationtemplate_controller.go), the ValidationPassed status map is used as a temporary tracking mechanism during incremental reconciliation to avoid re-processing nodes across requeue loops, but is intentionally cleared after all nodes are processed (line 189). The final status only reports failures in the ValidationFailures map; an empty ValidationFailures map indicates all validations passed.
Applied to files:
api/v1alpha1/fenceagentsremediationtemplate_webhook_test.go
🧬 Code graph analysis (1)
controllers/fenceagentsremediation_controller_test.go (3)
api/v1alpha1/fenceagentsremediation_types.go (1)
ResourceDeletionRemediationStrategy(48-48)pkg/utils/nodes.go (1)
GetNode(31-53)controllers/fenceagentsremediation_controller.go (1)
OldDefaultSecretName(52-52)
⏰ 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: Test and build PRs
- GitHub Check: Test Scorecard Tests
🔇 Additional comments (14)
config/manifests/bases/fence-agents-remediation.clusterserviceversion.yaml (1)
62-72: LGTM! Documentation updates are clear and consistent.The updated descriptions for
SharedSecretNamein bothFenceAgentsRemediationandFenceAgentsRemediationTemplateaccurately document:
- The removal of the default value
- The temporary workaround behavior
- The planned future removal of the workaround
Both descriptions are consistent, which is appropriate for the shared spec structure.
Also applies to: 132-142
api/v1alpha1/fenceagentsremediationtemplate_webhook_test.go (1)
406-406: LGTM! Test expectation updated correctly.The error message expectation change from "node secret" to "secret" aligns with the generalized error handling in
collectSecretParamswhere theisSharedSecretparameter was removed.bundle/manifests/fence-agents-remediation.medik8s.io_fenceagentsremediations.yaml (1)
90-100: LGTM! CRD schema correctly updated.The
sharedSecretNamefield no longer has a default value in the OpenAPI schema, and the description properly documents the temporary workaround behavior. This ensures new CRs will have the field empty when not explicitly set by users.config/crd/bases/fence-agents-remediation.medik8s.io_fenceagentsremediationtemplates.yaml (1)
97-107: LGTM! Template CRD schema correctly updated.The
sharedSecretNamefield in the template spec mirrors the changes in the mainFenceAgentsRemediationCRD - default removed and description updated to document the temporary workaround behavior.api/v1alpha1/fenceagentsremediation_types.go (1)
107-119: LGTM! API type definition correctly updated.The removal of the
+kubebuilder:defaultannotation is the key change that enables distinguishing between user-set and unset values. The expanded comment effectively documents the temporary workaround behavior for developers working with this codebase.The pointer type
*stringforSharedSecretName(line 119) is appropriate as it allows the controller to differentiate between:
nil(field not set by user)- Empty string (explicitly cleared)
- Non-empty string (user-provided value)
bundle/manifests/fence-agents-remediation.clusterserviceversion.yaml (2)
146-154: LGTM - Well-documented workaround behavior.The expanded description clearly explains the rationale for removing the default and documents the temporary workaround behavior. This provides good visibility for users about the backward-compatibility handling.
216-224: Description consistent with root spec.The template spec description mirrors the root spec changes appropriately.
bundle/manifests/fence-agents-remediation.medik8s.io_fenceagentsremediationtemplates.yaml (1)
101-108: LGTM - CRD description updated correctly.The description update aligns with the PR objectives and consistently documents the workaround behavior.
controllers/fenceagentsremediation_controller.go (1)
41-53: LGTM - Import and constant addition.The
ptrimport andOldDefaultSecretNameconstant are correctly added for the workaround implementation.config/crd/bases/fence-agents-remediation.medik8s.io_fenceagentsremediations.yaml (1)
90-97: LGTM - CRD description updated correctly.The description changes are consistent with the PR objectives and align with the other manifest updates.
api/v1alpha1/fenceagentsremediation_params.go (2)
348-351: LGTM - Simplified function call.The removal of the
isSharedSecretparameter simplifies the API. Since the controller workaround now ensuresSharedSecretNameis only set when the secret exists, this unified error handling is appropriate.
368-374: LGTM - Node secret collection simplified.The call site is updated consistently with the new signature.
controllers/fenceagentsremediation_controller_test.go (2)
148-151: LGTM!The conditional creation of
sharedSecretenables proper testing of scenarios where the secret doesn't exist. The nil-guard withDeferCleanupinside the conditional block ensures proper cleanup only when the secret was created.
166-214: Good test coverage for the workaround scenarios.The four test cases comprehensively cover all combinations of the workaround logic:
- Secret name set + secret exists → preserve
- Secret name set + secret missing → clear
- Secret name unset + secret exists → set
- Secret name unset + secret missing → no-op
This aligns well with the
applySharedSecretDefaultNameWorkaroundimplementation described in the PR objectives.
slintes
force-pushed
the
revert-shared-secret-default-name
branch
from
12796db to
494b7c1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
controllers/fenceagentsremediation_controller.go (1)
391-418: Use the passedctxparameter instead ofcontext.Background().Line 396 uses
context.Background()for the secret lookup, but the function receivesctxas a parameter. This inconsistency could cause issues if the parent context has timeouts, deadlines, or cancellation signals that should be respected.Proposed fix
func applySharedSecretDefaultNameWorkaround(ctx context.Context, far *v1alpha1.FenceAgentsRemediation, c client.Client, log logr.Logger) (bool, error) { // Check if the secret with the old default name exists secret := &corev1.Secret{} secretKey := client.ObjectKey{Name: OldDefaultSecretName, Namespace: far.Namespace} secretExists := true - if err := c.Get(context.Background(), secretKey, secret); err != nil { + if err := c.Get(ctx, secretKey, secret); err != nil { if !apiErrors.IsNotFound(err) { return false, fmt.Errorf("failed to check for shared secret: %w", err) } secretExists = false }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
api/v1alpha1/fenceagentsremediation_params.goapi/v1alpha1/fenceagentsremediation_types.goapi/v1alpha1/fenceagentsremediationtemplate_webhook_test.gobundle/manifests/fence-agents-remediation.clusterserviceversion.yamlbundle/manifests/fence-agents-remediation.medik8s.io_fenceagentsremediations.yamlbundle/manifests/fence-agents-remediation.medik8s.io_fenceagentsremediationtemplates.yamlconfig/crd/bases/fence-agents-remediation.medik8s.io_fenceagentsremediations.yamlconfig/crd/bases/fence-agents-remediation.medik8s.io_fenceagentsremediationtemplates.yamlconfig/manifests/bases/fence-agents-remediation.clusterserviceversion.yamlcontrollers/fenceagentsremediation_controller.gocontrollers/fenceagentsremediation_controller_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- api/v1alpha1/fenceagentsremediationtemplate_webhook_test.go
- config/crd/bases/fence-agents-remediation.medik8s.io_fenceagentsremediationtemplates.yaml
- controllers/fenceagentsremediation_controller_test.go
- config/crd/bases/fence-agents-remediation.medik8s.io_fenceagentsremediations.yaml
- bundle/manifests/fence-agents-remediation.medik8s.io_fenceagentsremediationtemplates.yaml
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: slintes
Repo: medik8s/fence-agents-remediation PR: 171
File: controllers/fenceagentsremediation_controller_test.go:153-181
Timestamp: 2025-06-10T17:25:54.191Z
Learning: In the FenceAgentsRemediation controller, validateUniqueParam is not applied for conflicts between secret parameters from different sources (shared secret vs node secret). Node secret parameters are allowed to override shared secret parameters without validation errors.
Learnt from: slintes
Repo: medik8s/fence-agents-remediation PR: 171
File: controllers/fenceagentsremediation_controller_test.go:153-181
Timestamp: 2025-06-10T17:25:54.191Z
Learning: In FenceAgentsRemediation controller, validateUniqueParam is applied to secret parameters to check conflicts with non-secret parameters (shared and node params), but not for conflicts within secret parameters themselves. Node secret parameters override shared secret parameters during the collection phase via maps.Copy() before validation occurs.
Learnt from: slintes
Repo: medik8s/fence-agents-remediation PR: 178
File: config/manager/manager.yaml:72-87
Timestamp: 2025-08-29T14:25:35.903Z
Learning: In fence-agents-remediation operator, kustomize adds app.kubernetes.io/name: fence-agents-remediation-operator labels to all resources via commonLabels in config/default/kustomization.yaml, so the podAntiAffinity selector that requires both control-plane=controller-manager and app.kubernetes.io/name=fence-agents-remediation-operator labels will work correctly even though the base config/manager/manager.yaml only shows the control-plane label.
📚 Learning: 2025-08-29T14:25:35.903Z
Learnt from: slintes
Repo: medik8s/fence-agents-remediation PR: 178
File: config/manager/manager.yaml:72-87
Timestamp: 2025-08-29T14:25:35.903Z
Learning: In fence-agents-remediation operator, kustomize adds app.kubernetes.io/name: fence-agents-remediation-operator labels to all resources via commonLabels in config/default/kustomization.yaml, so the podAntiAffinity selector that requires both control-plane=controller-manager and app.kubernetes.io/name=fence-agents-remediation-operator labels will work correctly even though the base config/manager/manager.yaml only shows the control-plane label.
Applied to files:
config/manifests/bases/fence-agents-remediation.clusterserviceversion.yamlapi/v1alpha1/fenceagentsremediation_params.gobundle/manifests/fence-agents-remediation.clusterserviceversion.yamlapi/v1alpha1/fenceagentsremediation_types.gocontrollers/fenceagentsremediation_controller.gobundle/manifests/fence-agents-remediation.medik8s.io_fenceagentsremediations.yaml
📚 Learning: 2025-06-10T17:25:54.191Z
Learnt from: slintes
Repo: medik8s/fence-agents-remediation PR: 171
File: controllers/fenceagentsremediation_controller_test.go:153-181
Timestamp: 2025-06-10T17:25:54.191Z
Learning: In FenceAgentsRemediation controller, validateUniqueParam is applied to secret parameters to check conflicts with non-secret parameters (shared and node params), but not for conflicts within secret parameters themselves. Node secret parameters override shared secret parameters during the collection phase via maps.Copy() before validation occurs.
Applied to files:
config/manifests/bases/fence-agents-remediation.clusterserviceversion.yamlapi/v1alpha1/fenceagentsremediation_params.gobundle/manifests/fence-agents-remediation.clusterserviceversion.yamlapi/v1alpha1/fenceagentsremediation_types.gocontrollers/fenceagentsremediation_controller.gobundle/manifests/fence-agents-remediation.medik8s.io_fenceagentsremediations.yaml
📚 Learning: 2025-06-10T17:25:54.191Z
Learnt from: slintes
Repo: medik8s/fence-agents-remediation PR: 171
File: controllers/fenceagentsremediation_controller_test.go:153-181
Timestamp: 2025-06-10T17:25:54.191Z
Learning: In the FenceAgentsRemediation controller, validateUniqueParam is not applied for conflicts between secret parameters from different sources (shared secret vs node secret). Node secret parameters are allowed to override shared secret parameters without validation errors.
Applied to files:
config/manifests/bases/fence-agents-remediation.clusterserviceversion.yamlapi/v1alpha1/fenceagentsremediation_params.gobundle/manifests/fence-agents-remediation.clusterserviceversion.yamlapi/v1alpha1/fenceagentsremediation_types.gocontrollers/fenceagentsremediation_controller.gobundle/manifests/fence-agents-remediation.medik8s.io_fenceagentsremediations.yaml
🧬 Code graph analysis (1)
controllers/fenceagentsremediation_controller.go (1)
api/v1alpha1/fenceagentsremediation_types.go (1)
FenceAgentsRemediation(150-156)
⏰ 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: Test Scorecard Tests
- GitHub Check: Test and build PRs
🔇 Additional comments (12)
bundle/manifests/fence-agents-remediation.clusterserviceversion.yaml (2)
146-156: LGTM!The updated description clearly documents the removal of the default value and the temporary workaround behavior. This aligns with the controller implementation.
216-226: LGTM!The description for the template's SharedSecretName is consistent with the FenceAgentsRemediation spec descriptor, maintaining uniformity across both CRDs.
config/manifests/bases/fence-agents-remediation.clusterserviceversion.yaml (2)
62-72: LGTM!The base manifest description is consistent with the bundle manifest.
132-142: LGTM!Consistent description update for the template's SharedSecretName.
bundle/manifests/fence-agents-remediation.medik8s.io_fenceagentsremediations.yaml (1)
90-100: LGTM!The CRD schema description clearly documents the workaround behavior and its temporary nature. The removal of the default value from the schema is correct.
controllers/fenceagentsremediation_controller.go (3)
41-41: LGTM!The
ptrpackage import is appropriate for safely creating pointers to string values.
51-53: LGTM!The constant
OldDefaultSecretNameis appropriately defined and matches the value referenced in the CRD documentation and workaround logic.
134-140: LGTM!The workaround is correctly positioned after node validation and before other reconciliation logic. Returning
requeueImmediatelywhen applied ensures the next reconcile iteration processes the updated CR state.api/v1alpha1/fenceagentsremediation_types.go (1)
107-119: LGTM!The field documentation clearly explains the historical context, the temporary workaround, and the deprecation plan. Removing the
+kubebuilder:defaultannotation ensures the API server won't auto-populate this field, enabling proper validation and the controller-side workaround to function as intended.api/v1alpha1/fenceagentsremediation_params.go (3)
347-363: LGTM on the simplified call site.The removal of the
isSharedSecretparameter streamlines the API. The nil check on line 347 ensurescollectSecretParamsis only called when a secret name is explicitly provided, which correctly aligns with the new semantics where missing secrets should fail.
365-374: LGTM on the node secret collection update.The call site is correctly updated to match the new signature. Based on learnings, node secret parameters override shared secret parameters via
maps.Copy()(line 373), and this behavior remains intact.
380-402: LGTM on the unified error handling.The removal of
isSharedSecretand unified NotFound behavior is a clean simplification. Previously, the shared secret's NotFound was silently ignored because the default was auto-set. Now that the API default is removed, if a user explicitly setsSharedSecretName, the secret must exist—this error path correctly enforces that contract.
slintes
force-pushed
the
revert-shared-secret-default-name
branch
from
494b7c1 to
03eb618
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/v1alpha1/fenceagentsremediation_params.go (1)
380-402: LGTM! Unified error handling simplifies the logic.The removal of
isSharedSecretand consistent not-found error handling is appropriate. The design ensures all referenced secrets exist through two mechanisms: (1) the controller workaround manages the old defaultSharedSecretNameby clearing it if the secret doesn't exist, ensuring it's only set when present, and (2) the webhook validation enforces that explicitly-setSharedSecretNameandNodeSecretNamesexist before the reconciliation uses them. This two-layer approach makes the behavior more predictable.
🤖 Fix all issues with AI agents
In @api/v1alpha1/fenceagentsremediationtemplate_webhook_test.go:
- Around line 406-407: The assertion that checks the full error string is
brittle; update the test to assert the error contains the secret name and the
namespace separately instead of the entire quoted sentence — replace the single
Expect(err.Error()).To(ContainSubstring("secret 'non-existent-node-secret' not
found in namespace 'test-namespace'")) with two assertions such as
Expect(err.Error()).To(ContainSubstring("non-existent-node-secret")) and
Expect(err.Error()).To(ContainSubstring("test-namespace")) so the test remains
resilient to formatting changes while still verifying the key details from
err.Error().
In @controllers/fenceagentsremediation_controller_test.go:
- Around line 166-214: The tests assert CR mutations synchronously after
reconcile which is flaky; replace the immediate k8sClient.Get + Expect checks
with Eventually polling (e.g., Gomega Eventually) that repeatedly k8sClient.Get
the underTestFAR and asserts the expected condition (SharedSecretName equals
OldDefaultSecretName or is nil) to wait for
applySharedSecretDefaultNameWorkaround to complete, and update test expectations
to reference the controller constant for the default name instead of the
duplicated OldDefaultSecretName to avoid drift (use the controller's default
name constant where tests currently reference OldDefaultSecretName and update
the four When/It cases to use Eventually on underTestFAR retrieved via
k8sClient).
In @controllers/fenceagentsremediation_controller.go:
- Around line 374-419: In applySharedSecretDefaultNameWorkaround replace the
unconditional c.Get(context.Background(), ...) with conditional GETs that use
the passed-in ctx only when you actually need to check the Secret (i.e., when
far.Spec.SharedSecretName == nil or when it equals OldDefaultSecretName) to
avoid unnecessary calls, and when mutating FAR use c.Patch(ctx, far,
client.MergeFrom(origFar)) instead of c.Update(ctx, far) to reduce conflict
risk; keep logging and the SharedSecretName nil/assign logic but capture origFar
before changes and call c.Patch with ctx.
🧹 Nitpick comments (4)
config/crd/bases/fence-agents-remediation.medik8s.io_fenceagentsremediationtemplates.yaml (1)
97-107: CRD doc update matches the behavioral change (no default + temporary workaround).Looks consistent and sets expectations for the legacy secret behavior. Consider explicitly stating “the controller may update
spec.sharedSecretName” to avoid surprising users/operators who don’t expect spec mutation.config/crd/bases/fence-agents-remediation.medik8s.io_fenceagentsremediations.yaml (1)
88-99: CRD doc update matches the behavioral change (no default + temporary workaround).Same as the template CRD: good context, and it’s clear the workaround is temporary. Optional: call out explicitly that the controller may mutate
spec.sharedSecretName.controllers/fenceagentsremediation_controller_test.go (1)
47-51: Timeout bump is fine, but prefer fixing flakiness at the source.If the motivation is reconcile timing, the more robust fix is to
Eventuallyon the expected state transitions rather than stretching timeouts globally.controllers/fenceagentsremediation_controller.go (1)
41-53: Prefer unexported constant for internal workaround.Since tests are in the same
controllerspackage,OldDefaultSecretNamedoesn’t need to be exported. Keeping it unexported reduces accidental coupling.Proposed change
const ( - OldDefaultSecretName = "fence-agents-credentials-shared" + oldDefaultSecretName = "fence-agents-credentials-shared" )
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
api/v1alpha1/fenceagentsremediation_params.goapi/v1alpha1/fenceagentsremediation_types.goapi/v1alpha1/fenceagentsremediationtemplate_webhook_test.gobundle/manifests/fence-agents-remediation.clusterserviceversion.yamlbundle/manifests/fence-agents-remediation.medik8s.io_fenceagentsremediations.yamlbundle/manifests/fence-agents-remediation.medik8s.io_fenceagentsremediationtemplates.yamlconfig/crd/bases/fence-agents-remediation.medik8s.io_fenceagentsremediations.yamlconfig/crd/bases/fence-agents-remediation.medik8s.io_fenceagentsremediationtemplates.yamlconfig/manifests/bases/fence-agents-remediation.clusterserviceversion.yamlcontrollers/fenceagentsremediation_controller.gocontrollers/fenceagentsremediation_controller_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- api/v1alpha1/fenceagentsremediation_types.go
- bundle/manifests/fence-agents-remediation.clusterserviceversion.yaml
- bundle/manifests/fence-agents-remediation.medik8s.io_fenceagentsremediationtemplates.yaml
- config/manifests/bases/fence-agents-remediation.clusterserviceversion.yaml
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: slintes
Repo: medik8s/fence-agents-remediation PR: 171
File: controllers/fenceagentsremediation_controller_test.go:153-181
Timestamp: 2025-06-10T17:25:54.191Z
Learning: In the FenceAgentsRemediation controller, validateUniqueParam is not applied for conflicts between secret parameters from different sources (shared secret vs node secret). Node secret parameters are allowed to override shared secret parameters without validation errors.
Learnt from: slintes
Repo: medik8s/fence-agents-remediation PR: 171
File: controllers/fenceagentsremediation_controller_test.go:153-181
Timestamp: 2025-06-10T17:25:54.191Z
Learning: In FenceAgentsRemediation controller, validateUniqueParam is applied to secret parameters to check conflicts with non-secret parameters (shared and node params), but not for conflicts within secret parameters themselves. Node secret parameters override shared secret parameters during the collection phase via maps.Copy() before validation occurs.
Learnt from: slintes
Repo: medik8s/fence-agents-remediation PR: 178
File: config/manager/manager.yaml:72-87
Timestamp: 2025-08-29T14:25:35.903Z
Learning: In fence-agents-remediation operator, kustomize adds app.kubernetes.io/name: fence-agents-remediation-operator labels to all resources via commonLabels in config/default/kustomization.yaml, so the podAntiAffinity selector that requires both control-plane=controller-manager and app.kubernetes.io/name=fence-agents-remediation-operator labels will work correctly even though the base config/manager/manager.yaml only shows the control-plane label.
📚 Learning: 2025-08-29T14:25:35.903Z
Learnt from: slintes
Repo: medik8s/fence-agents-remediation PR: 178
File: config/manager/manager.yaml:72-87
Timestamp: 2025-08-29T14:25:35.903Z
Learning: In fence-agents-remediation operator, kustomize adds app.kubernetes.io/name: fence-agents-remediation-operator labels to all resources via commonLabels in config/default/kustomization.yaml, so the podAntiAffinity selector that requires both control-plane=controller-manager and app.kubernetes.io/name=fence-agents-remediation-operator labels will work correctly even though the base config/manager/manager.yaml only shows the control-plane label.
Applied to files:
config/crd/bases/fence-agents-remediation.medik8s.io_fenceagentsremediations.yamlcontrollers/fenceagentsremediation_controller.goconfig/crd/bases/fence-agents-remediation.medik8s.io_fenceagentsremediationtemplates.yamlbundle/manifests/fence-agents-remediation.medik8s.io_fenceagentsremediations.yaml
📚 Learning: 2025-06-10T17:25:54.191Z
Learnt from: slintes
Repo: medik8s/fence-agents-remediation PR: 171
File: controllers/fenceagentsremediation_controller_test.go:153-181
Timestamp: 2025-06-10T17:25:54.191Z
Learning: In FenceAgentsRemediation controller, validateUniqueParam is applied to secret parameters to check conflicts with non-secret parameters (shared and node params), but not for conflicts within secret parameters themselves. Node secret parameters override shared secret parameters during the collection phase via maps.Copy() before validation occurs.
Applied to files:
config/crd/bases/fence-agents-remediation.medik8s.io_fenceagentsremediations.yamlapi/v1alpha1/fenceagentsremediationtemplate_webhook_test.gocontrollers/fenceagentsremediation_controller.goconfig/crd/bases/fence-agents-remediation.medik8s.io_fenceagentsremediationtemplates.yamlbundle/manifests/fence-agents-remediation.medik8s.io_fenceagentsremediations.yamlapi/v1alpha1/fenceagentsremediation_params.gocontrollers/fenceagentsremediation_controller_test.go
📚 Learning: 2025-06-10T17:25:54.191Z
Learnt from: slintes
Repo: medik8s/fence-agents-remediation PR: 171
File: controllers/fenceagentsremediation_controller_test.go:153-181
Timestamp: 2025-06-10T17:25:54.191Z
Learning: In the FenceAgentsRemediation controller, validateUniqueParam is not applied for conflicts between secret parameters from different sources (shared secret vs node secret). Node secret parameters are allowed to override shared secret parameters without validation errors.
Applied to files:
config/crd/bases/fence-agents-remediation.medik8s.io_fenceagentsremediations.yamlcontrollers/fenceagentsremediation_controller.goconfig/crd/bases/fence-agents-remediation.medik8s.io_fenceagentsremediationtemplates.yamlbundle/manifests/fence-agents-remediation.medik8s.io_fenceagentsremediations.yamlapi/v1alpha1/fenceagentsremediation_params.gocontrollers/fenceagentsremediation_controller_test.go
🧬 Code graph analysis (1)
controllers/fenceagentsremediation_controller_test.go (3)
api/v1alpha1/fenceagentsremediation_types.go (1)
ResourceDeletionRemediationStrategy(48-48)pkg/utils/nodes.go (1)
GetNode(31-53)controllers/fenceagentsremediation_controller.go (1)
OldDefaultSecretName(52-52)
⏰ 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: Test and build PRs
- GitHub Check: Test Scorecard Tests
🔇 Additional comments (5)
controllers/fenceagentsremediation_controller_test.go (1)
148-151: Nil-guard around sharedSecret creation is good.controllers/fenceagentsremediation_controller.go (1)
134-140: Reconcile integration looks correct (apply workaround, then requeue).bundle/manifests/fence-agents-remediation.medik8s.io_fenceagentsremediations.yaml (1)
90-100: LGTM! Clear documentation of the migration strategy.The updated description effectively communicates the breaking change, the rationale (enabling validation distinction), and the temporary workaround behavior. This provides good guidance for users during the transition period.
api/v1alpha1/fenceagentsremediation_params.go (2)
347-363: LGTM! Simplified call aligns with unified error handling.The removal of the
isSharedSecretparameter correctly delegates the responsibility of handling secret existence to the controller's workaround logic. IfSharedSecretNameis set, the secret must now exist.
365-374: LGTM!Node secret collection maintains the same error-on-not-found behavior with the simplified signature.
- Removed old default (`fence-agents-credentials-shared`) from API
- Introduced a temporary workaround:
- If the old default SharedSecret exists, set the value automatically.
- If not, remove the value when it exists.
- This preserves backward compatibility until the workaround
is removed in a future version.
Signed-off-by: Marc Sluiter <[email protected]>
slintes
force-pushed
the
revert-shared-secret-default-name
branch
from
03eb618 to
9cbdfb4
Compare
- Renamed constant `OldDefaultSecretName` to `oldDefaultSecretName` - Converted shared secret workaround func into a reconciler method - Prevent unneeded requeue when workaround was applied - Updated test to ensure automatic requeue happens Signed-off-by: Marc Sluiter <[email protected]>
Signed-off-by: Marc Sluiter <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@controllers/fenceagentsremediation_controller_test.go`:
- Around line 177-181: The Eventually closures in the
fenceagentsremediation_controller_test.go workaround tests use the global Expect
which prevents retries; update each closure (including the "should keep the
name" test that reads underTestFAR and the three other workaround test cases) to
use the provided Gomega instance’s assertions (g.Expect(...)) for all assertions
inside the Eventually(func(g Gomega) { ... }) blocks so the assertions are
retried properly (replace Expect(...) with g.Expect(...) wherever inside those
Eventually closures that reference underTestFAR and related checks).
🧹 Nitpick comments (1)
controllers/fenceagentsremediation_controller_test.go (1)
781-781: Consider using the constant for the shared secret name.The hardcoded string
"fence-agents-credentials-shared"duplicates the value ofv1alpha1.OldDefaultSecretName. Using the constant would prevent drift if the value ever changes.Proposed change
func getFenceAgentsRemediation(nodeName, agent string, sharedparameters map[v1alpha1.ParameterName]string, nodeparameters map[v1alpha1.ParameterName]map[v1alpha1.NodeName]string, strategy v1alpha1.RemediationStrategyType) *v1alpha1.FenceAgentsRemediation { - sharedSecretName := "fence-agents-credentials-shared" + sharedSecretName := v1alpha1.OldDefaultSecretName return &v1alpha1.FenceAgentsRemediation{
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
api/v1alpha1/fenceagentsremediation_params.goapi/v1alpha1/fenceagentsremediationtemplate_webhook_test.gocontrollers/fenceagentsremediation_controller.gocontrollers/fenceagentsremediation_controller_test.go
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: slintes
Repo: medik8s/fence-agents-remediation PR: 171
File: controllers/fenceagentsremediation_controller_test.go:153-181
Timestamp: 2025-06-10T17:25:54.191Z
Learning: In the FenceAgentsRemediation controller, validateUniqueParam is not applied for conflicts between secret parameters from different sources (shared secret vs node secret). Node secret parameters are allowed to override shared secret parameters without validation errors.
Learnt from: slintes
Repo: medik8s/fence-agents-remediation PR: 171
File: controllers/fenceagentsremediation_controller_test.go:153-181
Timestamp: 2025-06-10T17:25:54.191Z
Learning: In FenceAgentsRemediation controller, validateUniqueParam is applied to secret parameters to check conflicts with non-secret parameters (shared and node params), but not for conflicts within secret parameters themselves. Node secret parameters override shared secret parameters during the collection phase via maps.Copy() before validation occurs.
Learnt from: slintes
Repo: medik8s/fence-agents-remediation PR: 178
File: config/manager/manager.yaml:72-87
Timestamp: 2025-08-29T14:25:35.903Z
Learning: In fence-agents-remediation operator, kustomize adds app.kubernetes.io/name: fence-agents-remediation-operator labels to all resources via commonLabels in config/default/kustomization.yaml, so the podAntiAffinity selector that requires both control-plane=controller-manager and app.kubernetes.io/name=fence-agents-remediation-operator labels will work correctly even though the base config/manager/manager.yaml only shows the control-plane label.
📚 Learning: 2025-06-10T17:25:54.191Z
Learnt from: slintes
Repo: medik8s/fence-agents-remediation PR: 171
File: controllers/fenceagentsremediation_controller_test.go:153-181
Timestamp: 2025-06-10T17:25:54.191Z
Learning: In FenceAgentsRemediation controller, validateUniqueParam is applied to secret parameters to check conflicts with non-secret parameters (shared and node params), but not for conflicts within secret parameters themselves. Node secret parameters override shared secret parameters during the collection phase via maps.Copy() before validation occurs.
Applied to files:
controllers/fenceagentsremediation_controller_test.gocontrollers/fenceagentsremediation_controller.goapi/v1alpha1/fenceagentsremediation_params.goapi/v1alpha1/fenceagentsremediationtemplate_webhook_test.go
📚 Learning: 2025-06-10T17:25:54.191Z
Learnt from: slintes
Repo: medik8s/fence-agents-remediation PR: 171
File: controllers/fenceagentsremediation_controller_test.go:153-181
Timestamp: 2025-06-10T17:25:54.191Z
Learning: In the FenceAgentsRemediation controller, validateUniqueParam is not applied for conflicts between secret parameters from different sources (shared secret vs node secret). Node secret parameters are allowed to override shared secret parameters without validation errors.
Applied to files:
controllers/fenceagentsremediation_controller_test.gocontrollers/fenceagentsremediation_controller.goapi/v1alpha1/fenceagentsremediation_params.goapi/v1alpha1/fenceagentsremediationtemplate_webhook_test.go
📚 Learning: 2025-08-29T14:25:35.903Z
Learnt from: slintes
Repo: medik8s/fence-agents-remediation PR: 178
File: config/manager/manager.yaml:72-87
Timestamp: 2025-08-29T14:25:35.903Z
Learning: In fence-agents-remediation operator, kustomize adds app.kubernetes.io/name: fence-agents-remediation-operator labels to all resources via commonLabels in config/default/kustomization.yaml, so the podAntiAffinity selector that requires both control-plane=controller-manager and app.kubernetes.io/name=fence-agents-remediation-operator labels will work correctly even though the base config/manager/manager.yaml only shows the control-plane label.
Applied to files:
controllers/fenceagentsremediation_controller.goapi/v1alpha1/fenceagentsremediation_params.go
📚 Learning: 2026-01-06T10:20:03.052Z
Learnt from: mshitrit
Repo: medik8s/fence-agents-remediation PR: 190
File: controllers/fenceagentsremediationtemplate_controller.go:189-189
Timestamp: 2026-01-06T10:20:03.052Z
Learning: In the FenceAgentsRemediationTemplate controller (controllers/fenceagentsremediationtemplate_controller.go), the ValidationPassed status map is used as a temporary tracking mechanism during incremental reconciliation to avoid re-processing nodes across requeue loops, but is intentionally cleared after all nodes are processed (line 189). The final status only reports failures in the ValidationFailures map; an empty ValidationFailures map indicates all validations passed.
Applied to files:
api/v1alpha1/fenceagentsremediation_params.goapi/v1alpha1/fenceagentsremediationtemplate_webhook_test.go
🧬 Code graph analysis (4)
controllers/fenceagentsremediation_controller_test.go (3)
api/v1alpha1/fenceagentsremediation_types.go (1)
ResourceDeletionRemediationStrategy(48-48)pkg/utils/nodes.go (1)
GetNode(31-53)api/v1alpha1/fenceagentsremediation_params.go (1)
OldDefaultSecretName(50-50)
controllers/fenceagentsremediation_controller.go (2)
api/v1alpha1/fenceagentsremediation_types.go (1)
FenceAgentsRemediation(150-156)api/v1alpha1/fenceagentsremediation_params.go (1)
OldDefaultSecretName(50-50)
api/v1alpha1/fenceagentsremediation_params.go (1)
api/v1alpha1/fenceagentsremediationtemplate_types.go (1)
FenceAgentsRemediationTemplate(52-58)
api/v1alpha1/fenceagentsremediationtemplate_webhook_test.go (2)
api/v1alpha1/fenceagentsremediation_types.go (1)
ResourceDeletionRemediationStrategy(48-48)api/v1alpha1/fenceagentsremediation_params.go (1)
OldDefaultSecretName(50-50)
⏰ 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: Test Scorecard Tests
- GitHub Check: Test and build PRs
🔇 Additional comments (13)
api/v1alpha1/fenceagentsremediationtemplate_webhook_test.go (2)
286-450: Comprehensive test coverage for the shared secret default name validation.The test context covers all critical scenarios:
- Old template without SharedSecretName
- Old template with non-default name
- Old template with old default name but new template still has a name
- Secret exists vs does not exist scenarios
- Error handling for secret fetch failures
The mock setup with
DeferCleanupproperly restores the original function after each test, preventing test pollution.
572-572: Error message update aligns with unified secret handling.The error message change from
"node secret"to"secret"reflects the refactoredcollectSecretParamsfunction that no longer distinguishes between shared and node secrets.controllers/fenceagentsremediation_controller_test.go (3)
48-48: Timeout increase improves test stability.Increasing
timeoutPreRemediationfrom"1s"to"5s"provides more headroom for the controller to complete reconciliation, reducing flakiness under load.
148-151: Conditional shared secret creation enables workaround testing.The nil check before creating the shared secret allows test scenarios where the secret does not exist, which is essential for testing the workaround behavior.
524-527: Good integration test for the workaround with annotation-based node naming.Setting
SharedSecretNametonilwhile the secret exists verifies that the controller correctly applies the workaround by auto-populating the field, ensuring backward compatibility with escalation remediation scenarios.controllers/fenceagentsremediation_controller.go (3)
41-41: Import added forptr.Tousage.The
k8s.io/utils/ptrimport is required for theptr.Tohelper used in the workaround function.
130-136: Workaround applied early in reconcile flow.Placing the workaround call before finalizer logic ensures the
SharedSecretNamefield is correctly populated or cleared before any remediation actions begin. ReturningemptyResultwhen applied triggers an immediate requeue with the updated spec.
369-415: Well-documented workaround with clear rationale and removal plan.The function documentation clearly explains:
- Why the workaround is needed (API default value preventing validation)
- The specific behavior (set/clear based on secret existence)
- That it will be removed in a future version
The implementation correctly:
- Uses
ctxfor the secret lookup (addressed from past review)- Returns
(true, nil)to trigger requeue after spec update- Handles both scenarios: nil → set, and old-default → nil
api/v1alpha1/fenceagentsremediation_params.go (5)
49-51: Constant centralizes the deprecated default secret name.Defining
OldDefaultSecretNameas a constant ensures consistency across the codebase (controller, params, tests) and makes future changes easier.
79-85: Error aggregation properly combines validation errors.Using
utilErrors.NewAggregateto combine the main validation error with the shared secret default name validation ensures all issues are reported in a single response, improving user experience.
87-114: Validation logic correctly prevents removal of default name when secret exists.The function properly checks:
- Early return if old template doesn't have the old default name
- Early return if new template still has a non-empty SharedSecretName
- Secret existence check before blocking removal
- Clear error message guiding the user to delete the secret first
One edge case to verify: Line 98-99 checks
newTemplate.Spec.Template.Spec.SharedSecretName != nil && *... != "". This correctly handles bothnil(field removed) and empty string (field set to"").
384-384: SimplifiedcollectSecretParamscall removes type branching.Removing the
isSharedSecretparameter unifies the error handling for both shared and node secrets, which aligns with the updated behavior where all missing secrets are now treated as errors.
416-428: Unified secret error handling with clear error messages.The simplified
collectSecretParamsfunction now returns errors for all missing secrets, with consistent error messages that include both the secret name and namespace. This aligns with the PR objective where the workaround handles the default secret existence check separately.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
slintes
force-pushed
the
revert-shared-secret-default-name
branch
from
4b9e318 to
71dc322
Compare
Signed-off-by: Marc Sluiter <[email protected]>
slintes
force-pushed
the
revert-shared-secret-default-name
branch
from
71dc322 to
2985a02
Compare
| func (v *customValidator) ValidateUpdate(ctx context.Context, old runtime.Object, new runtime.Object) (admission.Warnings, error) { | ||
| return v.validate(ctx, new) | ||
| warnings, err := v.validate(ctx, new) | ||
| aggregated := utilErrors.NewAggregate([]error{ |
There was a problem hiding this comment.
Nit: I think that validating validateTemplateForSharedSecretDefaultName outside of validate is a bit confusing.
IMO a nicer way would be to have to path inside validate: validateFar and validateFarTemplate
There was a problem hiding this comment.
I think it makes sense to have it separated like it is because a) this is for the template only, while validate() is for both CRs and b) this is a temporary thing and this way it's easier to revert
There was a problem hiding this comment.
well, not sure about "this is a temporary thing" actually, we can also keep it when we remove the workaround. a) still applies
| func (v *customValidator) validateTemplateForSharedSecretDefaultName(ctx context.Context, old, new runtime.Object) error { | ||
| // prevent removing the default shared secret name while such a secret exists | ||
| oldTemplate, ok := old.(*FenceAgentsRemediationTemplate) | ||
| if !ok || |
There was a problem hiding this comment.
IMO ignoring !ok use case is confusing as I'd expect it to be an indication of a problem.
In case two validation path are implemented this check can be dropped, otherwise I suggest at least adding a comment explaining why it's ignored (i.e because applied on FenceAgentsRemediation and not FenceAgentsRemediationTemplate)
There was a problem hiding this comment.
I do not understand your concern. When this isn't a template CR, it's not a problem. It's just something else (a normal FAR CR), which we don't want to validate here. And the "template" in the method name is a clear indication that this validates templates only, not?
| } | ||
|
|
||
| newTemplate, ok := new.(*FenceAgentsRemediationTemplate) | ||
| if !ok || |
There was a problem hiding this comment.
IIUC getting !ok at this stage is a problem and should be reported somehow
There was a problem hiding this comment.
no, it's not a problem, see above
clobrano
left a comment
clobrano
left a comment
There was a problem hiding this comment.
Just a typo and a suggestion, otherwise it looks good to me
| Expect(k8sClient.Get(context.Background(), client.ObjectKeyFromObject(underTestFAR), underTestFAR)).To(Succeed()) | ||
| Expect(*underTestFAR.Spec.SharedSecretName).To(Equal(OldDefaultSecretName)) |
There was a problem hiding this comment.
It should be g.Expect (there are several occurrences)
There was a problem hiding this comment.
outdated, already fixed 🙂
| paramsLog.Error(err, "node secret not found", "secret name", secretName, "namespace", namespace) | ||
| return nil, fmt.Errorf("node secret '%s' not found in namespace '%s': %w", secretName, namespace, err) | ||
|
|
||
| paramsLog.Error(err, "secret not found", "secret name", secretName, "namespace", namespace) |
There was a problem hiding this comment.
I love this simplification 😀
| // - updates of the default value are challenging and can result in backwards compatibility issues | ||
| // | ||
| // - because of that we decided to | ||
| // - remove the default value, so the field wil stay empty for new CRs when it's empty |
| // - and remove the value on existing CRs when no such Secret exists | ||
| // | ||
| // This workaround will be removed in a future version | ||
| // Returns true if the workaround was applied, false otherwise |
There was a problem hiding this comment.
It is worth adding a TODO here mentioning the need to remove the workaround in the future. Maybe even pointing to a specific RHWA ticket won't hurt.
There was a problem hiding this comment.
added TODO to the "// This workaround will be removed in a future version" comment
Signed-off-by: Marc Sluiter <[email protected]>
3 participants
Why we need this PR
in the first version of FAR which introduced the usage of secrets, we added the new API field "SharedSecretName"
like every new API field, it has to be optional, to be backwards compatible
however, we also set a default value "fence-agents-credentials-shared" via API
that introduced issues:
because we don't know if it was set by the user (meaning the Secret should exist) or not
Changes made
This workaround will be removed in a future version
Which issue(s) this PR fixes
RHWA-613
Test plan
Added unit tests for covering the workaround
Summary by CodeRabbit
Features
Bug Fixes
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.