ESO-166:Removes resources created for bitwarden server when bitwardenSecretManager is disable #59
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@siddhibhor-56: This pull request references ESO-166 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughDecoding-based deletion added for Service and cert-manager Certificate assets; certificate handling split into webhook/Bitwarden flows with early exit when cert-manager disabled; Bitwarden service/certificate deleted when Bitwarden is disabled (errors propagated); RBAC/CSV updated for cert-manager resources; tests added/extended for these cases. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Reconciler
participant Config
participant CertManager as "cert-manager API"
participant K8s as "Kubernetes API"
rect rgba(230,245,255,0.5)
note over Reconciler: Certificate reconciliation
Reconciler->>Config: Is cert-manager enabled?
alt Disabled
note right of Reconciler: Early return — no certificate ops
else Enabled
Reconciler->>CertManager: Get/Ensure Issuer
alt Bitwarden disabled
Reconciler->>K8s: Delete Bitwarden Certificate
note right of K8s: Error bubbles up if deletion fails
else Bitwarden enabled
Reconciler->>K8s: Create/Update Certificates (webhook/Bitwarden)
end
end
end
sequenceDiagram
autonumber
actor Reconciler
participant Assets as "Asset decoder"
participant K8s as "Kubernetes API"
rect rgba(240,255,240,0.5)
note over Reconciler: Service handling
Reconciler->>Reconciler: Evaluate service condition
alt condition true
Reconciler->>K8s: Create/Update Service
else condition false
Reconciler->>Assets: Decode Service asset bytes
Assets-->>Reconciler: Service object
Reconciler->>K8s: Delete Service
note right of K8s: Propagate delete errors
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
|
@siddhibhor-56: This pull request references ESO-166 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@siddhibhor-56: This pull request references ESO-166 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
pkg/controller/common/utils.go(1 hunks)pkg/controller/external_secrets/certificate.go(2 hunks)pkg/controller/external_secrets/certificate_test.go(1 hunks)pkg/controller/external_secrets/service_test.go(1 hunks)pkg/controller/external_secrets/services.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
pkg/controller/external_secrets/certificate_test.go (3)
pkg/controller/client/fakes/fake_ctrl_client.go (1)
FakeCtrlClient(12-135)pkg/controller/commontest/utils.go (2)
TestClientError(30-30)TestExternalSecretsNamespace(22-22)api/v1alpha1/external_secrets_types.go (4)
ExternalSecrets(40-52)ExternalSecretsConfig(76-124)CertManagerConfig(171-206)BitwardenSecretManagerProvider(144-158)
pkg/controller/common/utils.go (1)
pkg/operator/assets/bindata.go (1)
MustAsset(1428-1435)
pkg/controller/external_secrets/service_test.go (4)
pkg/controller/external_secrets/controller.go (1)
Reconciler(82-91)pkg/controller/client/fakes/fake_ctrl_client.go (1)
FakeCtrlClient(12-135)pkg/controller/commontest/utils.go (1)
TestClientError(30-30)api/v1alpha1/external_secrets_types.go (3)
ExternalSecrets(40-52)ExternalSecretsConfig(76-124)BitwardenSecretManagerProvider(144-158)
pkg/controller/external_secrets/services.go (2)
pkg/controller/common/utils.go (1)
DeleteObject(470-500)pkg/controller/client/client.go (1)
CtrlClient(21-31)
pkg/controller/external_secrets/certificate.go (2)
pkg/controller/common/utils.go (1)
DeleteObject(470-500)pkg/controller/client/client.go (1)
CtrlClient(21-31)
🔇 Additional comments (7)
pkg/controller/common/utils.go (1)
483-486: LGTM! Support for Service and Certificate deletion added correctly.The addition of Service and Certificate cases to the DeleteObject function is consistent with the existing pattern and correctly uses the appropriate decoder functions for each object type.
pkg/controller/external_secrets/certificate.go (3)
24-28: LGTM! Early guard improves efficiency and clarity.The early return when cert-manager is disabled prevents unnecessary processing and makes the function's behavior more predictable.
30-33: LGTM! Webhook certificate creation properly scoped.Moving the webhook certificate creation inside the cert-manager enabled block ensures it only runs when cert-manager is available, which is logical since certificates require cert-manager.
42-46: LGTM! Bitwarden certificate cleanup when disabled.The addition of certificate deletion when Bitwarden is disabled ensures proper cleanup of resources. The error wrapping provides clear context about which operation failed.
pkg/controller/external_secrets/certificate_test.go (2)
443-493: LGTM! Comprehensive test for certificate deletion.This test case thoroughly validates the Bitwarden certificate deletion path when Bitwarden is disabled:
- Properly sets up the webhook certificate as non-existent (to be created)
- Configures the bitwarden certificate as existing (to be deleted)
- Simulates deletion failure to test error handling
- Validates the expected error message format
494-527: LGTM! Important test for cert-manager disabled scenario.This test ensures that when cert-manager is disabled, no certificate operations occur, which prevents unnecessary API calls and potential errors. The test correctly validates that all client operations (Exists, Create, Delete, Get) are not called.
pkg/controller/external_secrets/services.go (1)
32-34: LGTM! Service deletion when condition is false.The service deletion logic correctly calls
common.DeleteObjectwhen the service condition is false, and provides clear error context with "failed to delete bitwarden-server service".
|
@siddhibhor-56: This pull request references ESO-166 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/controller/external_secrets/controller.go (1)
283-296: Certificate watch is unreachable; add explicit Watches outside the loop.
controllerManagedResourcesdoesn’t include&certmanagerv1.Certificate{}, so this switch case never runs.Apply:
for _, res := range controllerManagedResources { switch res { case &appsv1.Deployment{}: mgrBuilder.Watches(res, handler.EnqueueRequestsFromMapFunc(mapFunc), withIgnoreStatusUpdatePredicates) - case &certmanagerv1.Certificate{}: - if _, ok := r.optionalResourcesList[certificateCRDGKV]; ok { - mgrBuilder.Watches(res, handler.EnqueueRequestsFromMapFunc(mapFunc), managedResourcePredicate) - } case &corev1.Secret{}: mgrBuilder.WatchesMetadata(res, handler.EnqueueRequestsFromMapFunc(mapFunc), builder.WithPredicates(predicate.LabelChangedPredicate{})) default: mgrBuilder.Watches(res, handler.EnqueueRequestsFromMapFunc(mapFunc), managedResourcePredicate) } } +// Watch Certificates explicitly when CRD exists. +if _, ok := r.optionalResourcesList[certificateCRDGKV]; ok { + mgrBuilder.Watches(&certmanagerv1.Certificate{}, handler.EnqueueRequestsFromMapFunc(mapFunc), managedResourcePredicate) +}pkg/controller/external_secrets/service_test.go (1)
115-146: Align error text with actual resource name and consider asserting only bitwarden service is deleted.
Code reports "bitwarden-server", but the object is "bitwarden-sdk-server". Prefer consistent naming to reduce confusion. Also, assert that webhook service is not deleted.Update expected error (after fixing services.go to use the real name):
- wantErr: `failed to delete bitwarden-server service: test client error`, + wantErr: `failed to delete bitwarden-sdk-server service: test client error`,And in services.go (reference):
- return fmt.Errorf("failed to delete bitwarden-server service: %w", err) + return fmt.Errorf("failed to delete bitwarden-sdk-server service: %w", err)Optionally, assert only bitwarden service deletion was attempted (example):
// after call if got := mock.DeleteCallCount(); got != 1 { t.Fatalf("expected 1 delete call, got %d", got) } _, deletedObj, _ := mock.DeleteArgsForCall(0) if svc, ok := deletedObj.(*corev1.Service); !ok || svc.Name != "bitwarden-sdk-server" { t.Fatalf("expected delete of bitwarden-sdk-server, got %T/%s", deletedObj, client.ObjectKeyFromObject(deletedObj).Name) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
bundle/manifests/external-secrets-operator.clusterserviceversion.yaml(1 hunks)bundle/manifests/operator.openshift.io_externalsecrets.yaml(1 hunks)bundle/manifests/operator.openshift.io_externalsecretsmanagers.yaml(1 hunks)config/crd/bases/operator.openshift.io_externalsecrets.yaml(1 hunks)config/crd/bases/operator.openshift.io_externalsecretsmanagers.yaml(1 hunks)config/rbac/role.yaml(1 hunks)pkg/controller/external_secrets/controller.go(1 hunks)pkg/controller/external_secrets/service_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- bundle/manifests/operator.openshift.io_externalsecretsmanagers.yaml
- bundle/manifests/operator.openshift.io_externalsecrets.yaml
- config/crd/bases/operator.openshift.io_externalsecrets.yaml
- config/crd/bases/operator.openshift.io_externalsecretsmanagers.yaml
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/controller/external_secrets/service_test.go (4)
pkg/controller/external_secrets/controller.go (1)
Reconciler(82-91)pkg/controller/client/fakes/fake_ctrl_client.go (1)
FakeCtrlClient(12-135)pkg/controller/commontest/utils.go (1)
TestClientError(30-30)api/v1alpha1/external_secrets_types.go (3)
ExternalSecrets(40-52)ExternalSecretsConfig(76-124)BitwardenSecretManagerProvider(144-158)
🔇 Additional comments (5)
pkg/controller/external_secrets/controller.go (1)
103-104: Verified no issuer deletions or certificate patch usage.config/rbac/role.yaml (2)
77-84: RBAC: add delete on cert-manager Certificates — OK.
Matches controller intent to delete Certificate objects.
85-94: RBAC: issuers/clusterissuers missing “delete” verbs — manual verification needed
I didn’t find any.Delete(calls targeting Issuer or ClusterIssuer in the Go code; please double-check (including any dynamic-client or generated clientset usage) that nothing ever deletes these resources to avoid unexpected 403s.bundle/manifests/external-secrets-operator.clusterserviceversion.yaml (2)
415-422: CSV RBAC updated for cert-manager Certificates — looks consistent with ClusterRole.
423-432: CSV RBAC for issuers/clusterissuers excludes delete — confirm code doesn’t delete them.
Keep CSV and code behaviors aligned to avoid permission errors.Use the same repo-wide search shared in role.yaml comment.
There was a problem hiding this comment.
[claude generated]
Code Review Summary
This PR implements cleanup functionality for Bitwarden server resources when disabled. Overall, this is a solid improvement that addresses a real operational issue.
Key Strengths
- Clear problem resolution for orphaned resources
- Comprehensive cleanup across multiple resource types
- Good test coverage for new functionality
- Proper RBAC updates
Main Concerns
- Error message specificity in services.go
- Controller-gen version regression
- Early return in certificate logic may skip cleanup
Recommendation: Approve with minor fixes suggested in line comments.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mytreya-rh, siddhibhor-56 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| for _, service := range servicesToCreate { | ||
| if !service.condition { | ||
| if err := common.DeleteObject(r.ctx, r.CtrlClient, &corev1.Service{}, service.assetName); err != nil { | ||
| return fmt.Errorf("failed to delete bitwarden-server service: %w", err) |
There was a problem hiding this comment.
[claude generated] Error message is hardcoded to 'bitwarden-server service' but this logic applies to any service where condition=false. Consider making it dynamic based on the actual service being deleted.
| metadata: | ||
| annotations: | ||
| controller-gen.kubebuilder.io/version: v0.17.3 | ||
| controller-gen.kubebuilder.io/version: v0.16.1 |
There was a problem hiding this comment.
[claude generated] Controller-gen version downgraded from v0.17.3 to v0.16.1. Please verify if this regression is intentional. Same issue exists in other CRD files.
| } | ||
| // Only handle certificates if cert-manager is enabled | ||
| if !isCertManagerConfigEnabled(es) { | ||
| // If cert-manager is not enabled, we don't create or delete any certificates |
There was a problem hiding this comment.
[claude generated] This early return might skip necessary cleanup. If cert-manager becomes disabled after certificates were created, they won't be cleaned up. Consider adding cleanup logic even when cert-manager is disabled.
| o = DecodeServiceAccountObjBytes(assets.MustAsset(assetName)) | ||
| case *corev1.Service: | ||
| o = DecodeServiceObjBytes(assets.MustAsset(assetName)) | ||
| case *certmanagerv1.Certificate: |
There was a problem hiding this comment.
[claude generated] Good addition of Certificate and Service support to DeleteObject function. This properly enables cleanup of these resource types.
| - certificates | ||
| verbs: | ||
| - create | ||
| - delete |
There was a problem hiding this comment.
[claude generated] Excellent addition of delete permissions for certificates. This is necessary for the cleanup functionality and properly scoped to cert-manager.io resources.
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
siddhibhor-56
force-pushed
the
eso-166
branch
from
44260b9 to
cf64854
Compare
openshift-merge-robot
added
the
needs-rebase
|
New changes are detected. LGTM label has been removed. |
…nagerProvider is disabled
siddhibhor-56
force-pushed
the
eso-166
branch
from
22194da to
2bffb9f
Compare
openshift-merge-robot
removed
the
needs-rebase
|
@siddhibhor-56: This pull request references ESO-166 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bundle/manifests/external-secrets-operator.clusterserviceversion.yaml (1)
301-319: Remove unsupported MFA and SSHKey CRDs from CSV
No local reconcilers or scheme registrations were found for MFA or SSHKey (only AWS SDK vendor code matched), so these generators are unused—remove them from bundle/manifests/external-secrets-operator.clusterserviceversion.yaml to prevent misleading installs.
♻️ Duplicate comments (1)
pkg/controller/external_secrets/services.go (1)
32-34: Make error message generic and align with existing error-wrapping helperThe message is hardcoded to “bitwarden-server service”. Use the asset that’s actually being deleted and the common.FromClientError helper for consistency with the rest of the controller.
- if err := common.DeleteObject(r.ctx, r.CtrlClient, &corev1.Service{}, service.assetName); err != nil { - return fmt.Errorf("failed to delete bitwarden-server service: %w", err) - } + if err := common.DeleteObject(r.ctx, r.CtrlClient, &corev1.Service{}, service.assetName); err != nil { + return common.FromClientError(err, "failed to delete service asset %s", service.assetName) + }Note: If you have a test asserting on the old string, update it to match the new, generic message.
🧹 Nitpick comments (5)
pkg/controller/external_secrets/services.go (3)
31-36: Emit a “deleted” event for observabilityCreation/updates emit events; deletions don’t. Add a Normal event when a service is removed so operators understand why resources disappeared.
- if !service.condition { - if err := common.DeleteObject(r.ctx, r.CtrlClient, &corev1.Service{}, service.assetName); err != nil { - return common.FromClientError(err, "failed to delete service asset %s", service.assetName) - } - continue - } + if !service.condition { + // derive service name for eventing + svc := common.DecodeServiceObjBytes(assets.MustAsset(service.assetName)) + updateNamespace(svc, externalsecrets) + svcName := fmt.Sprintf("%s/%s", svc.GetNamespace(), svc.GetName()) + + if err := common.DeleteObject(r.ctx, r.CtrlClient, &corev1.Service{}, service.assetName); err != nil { + return common.FromClientError(err, "failed to delete service %s", svcName) + } + r.eventRecorder.Eventf(externalsecrets, corev1.EventTypeNormal, "Reconciled", "Service %s deleted", svcName) + continue + }
14-15: Update the doc comment to reflect deletion behaviorThis function now deletes resources when conditions are false; adjust the comment.
-// createOrApplyServices handles conditional and default creation of Services. +// createOrApplyServices creates/updates Services and deletes them when their conditions evaluate to false.
16-20: Rename servicesToCreate → servicesToManage for clarityThe slice governs both create/apply and delete paths now.
- servicesToCreate := []struct { + servicesToManage := []struct { assetName string condition bool }{ ... } - for _, service := range servicesToCreate { + for _, service := range servicesToManage {Also applies to: 30-30
config/rbac/role.yaml (1)
77-84: Addpatchverb to Certificates RBAC for parity
No client.Patch, Apply, or serverSideApply calls on Certificates were found in the codebase (only in vendored client-go), so adding thepatchverb is safe and ensures parity with other resources.
File:config/rbac/role.yaml, lines 77–84verbs: - create - delete - get - list + - patch - update - watchbundle/manifests/external-secrets-operator.clusterserviceversion.yaml (1)
415-422: Add patch verb for Certificates in CSV clusterPermissionsKeeps CSV in sync with code and prevents 403 on Patch/SSA if introduced.
Proposed change:
verbs: - create - delete - get - list + - patch - update - watch
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (8)
bundle/manifests/external-secrets-operator.clusterserviceversion.yaml(2 hunks)config/rbac/role.yaml(1 hunks)pkg/controller/common/utils.go(1 hunks)pkg/controller/external_secrets/certificate.go(2 hunks)pkg/controller/external_secrets/certificate_test.go(1 hunks)pkg/controller/external_secrets/controller.go(1 hunks)pkg/controller/external_secrets/service_test.go(1 hunks)pkg/controller/external_secrets/services.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/controller/common/utils.go
- pkg/controller/external_secrets/certificate_test.go
- pkg/controller/external_secrets/certificate.go
- pkg/controller/external_secrets/service_test.go
🔇 Additional comments (6)
pkg/controller/external_secrets/services.go (1)
32-34: Confirm DeleteObject uses correct namespaceDeleteObject (pkg/controller/common/utils.go:470–497) decodes the asset and deletes the object without calling updateNamespace, so it relies on the asset’s own metadata.namespace—ensure your service asset YAML includes the managed namespace or extend DeleteObject to call updateNamespace before deletion. It already treats NotFound as success.
config/rbac/role.yaml (1)
85-95: No Delete calls detected for Issuer/ClusterIssuer; omission of “delete” verb is safepkg/controller/external_secrets/controller.go (2)
111-116: Removal of pruned resources verified: ripgrep scan returned onlyvendor/aws-sdk-gohits forClusterPushSecret,MFA, andSSHKey, with no references in project controllers—RBAC removals are safe.
103-105: Skip addingpatchpermission to Certificates RBAC
Noclient.Patchor server-side apply usage on Certificate resources was found inpkg/controller/external_secrets/controller.go, so the additionalpatchverb isn’t required.bundle/manifests/external-secrets-operator.clusterserviceversion.yaml (2)
207-207: CSV createdAt refresh OKTimestamp update is expected from regeneration.
423-433: Confirm no deletion operations for Issuer/ClusterIssuer
We found no.Delete(...)calls against these resources; if the operator never deletes issuers/clusterissuers, the current verbs (create, get, list, update, watch) suffice—otherwise add thedeleteverb.
|
@siddhibhor-56: This pull request references ESO-166 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
config/rbac/role.yaml (1)
74-84: LGTM: certificate delete verb is correctly scoped to cert-manager.io/CertificatesNeeded for cleanup when Bitwarden is disabled; nothing overly permissive.
🧹 Nitpick comments (1)
config/rbac/role.yaml (1)
77-84: Add patch verb for Certificates to avoid update conflictsMany controllers use Patch to reduce write conflicts; safe, least-privilege expansion.
Apply:
verbs: - create - delete - get - list + - patch - update - watch
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (3)
bundle/manifests/external-secrets-operator.clusterserviceversion.yaml(2 hunks)config/rbac/role.yaml(1 hunks)pkg/controller/external_secrets/controller.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/controller/external_secrets/controller.go
- bundle/manifests/external-secrets-operator.clusterserviceversion.yaml
🔇 Additional comments (1)
config/rbac/role.yaml (1)
85-95: Consider patching Issuer/ClusterIssuer only if metadata modifications are introduced
No.Patchcalls against Issuer or ClusterIssuer were found in the controller code; retaining nodeleteis correct. Addpatchverbs here when you introduce any metadata (ownerRefs/labels) tweaks on these CRs.
| for _, service := range servicesToCreate { | ||
| if !service.condition { | ||
| if err := common.DeleteObject(r.ctx, r.CtrlClient, &corev1.Service{}, service.assetName); err != nil { | ||
| return fmt.Errorf("failed to delete bitwarden-server service: %w", err) |
There was a problem hiding this comment.
nit: log better to be generic. Error will include the object details, hence below change should be enough?
| return fmt.Errorf("failed to delete bitwarden-server service: %w", err) | |
| return fmt.Errorf("failed to delete service object: %w", err) |
| serviceExternalSecretWebhookName string = "external-secrets-webhook" | ||
| ) | ||
|
|
||
| func (r *Reconciler) createOrApplyCertificates(es *operatorv1alpha1.ExternalSecrets, resourceLabels map[string]string, recon bool) error { |
There was a problem hiding this comment.
I think this function requires refactoring, probably new functions for handling certificate needs of webhook and bitwarden.
And the comment If cert-manager is not enabled, we don't create or delete any certificates is misleading, adding two different functions will improve the code readability I feel.
|
@siddhibhor-56: This pull request references ESO-166 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@siddhibhor-56: This pull request references ESO-166 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
pkg/controller/external_secrets/services.go (1)
32-34: Good fix: generic, object-agnostic error wrapping.This addresses prior feedback about hardcoded object names and keeps the wrapped error informative.
🧹 Nitpick comments (4)
pkg/controller/external_secrets/services.go (1)
30-36: Make deletion idempotent and observable (ignore NotFound, emit event).If the Service never existed or was already removed, bubbling up a NotFound degrades the reconcile loop unnecessarily. Also, emitting a Normal event on successful deletion helps operators trace cleanup actions.
Apply:
for _, service := range servicesToCreate { if !service.condition { - if err := common.DeleteObject(r.ctx, r.CtrlClient, &corev1.Service{}, service.assetName); err != nil { - return fmt.Errorf("failed to delete service object: %w", err) - } + // derive name/namespace for better logs/events + svc := common.DecodeServiceObjBytes(assets.MustAsset(service.assetName)) + updateNamespace(svc, externalsecrets) + svcName := fmt.Sprintf("%s/%s", svc.GetNamespace(), svc.GetName()) + + if err := common.DeleteObject(r.ctx, r.CtrlClient, &corev1.Service{}, service.assetName); err != nil { + if apierrors.IsNotFound(err) { + r.log.V(4).Info("Service already absent; nothing to delete", "name", svcName) + } else { + return fmt.Errorf("failed to delete service object: %w", err) + } + } else { + r.eventRecorder.Eventf(externalsecrets, corev1.EventTypeNormal, "Reconciled", "Service %s deleted", svcName) + } continue }Add import:
+ apierrors "k8s.io/apimachinery/pkg/api/errors"pkg/controller/external_secrets/certificate.go (3)
39-45: Optional: also clean up webhook certificate when cert-manager is disabled.If cert-manager is toggled off after initial install, the webhook Certificate may persist. Consider mirroring the Bitwarden cleanup to avoid stale resources. If that’s out of scope for ESO-166, feel free to defer.
49-50: Comment contradicts behavior.“independent of cert-manager configuration” is misleading because Certificate CRs require cert-manager. Align the comment with the guard above.
63-65: Make Bitwarden cleanup idempotent; ignore NotFound and (optionally) emit an event.Deletion should not fail reconcile if the Certificate is already gone.
Apply:
-func (r *Reconciler) cleanupBitwardenCertificate() error { - if err := common.DeleteObject(r.ctx, r.CtrlClient, &certmanagerv1.Certificate{}, bitwardenCertificateAssetName); err != nil { - return fmt.Errorf("failed to delete bitwarden certificate: %w", err) - } +func (r *Reconciler) cleanupBitwardenCertificate() error { + if err := common.DeleteObject(r.ctx, r.CtrlClient, &certmanagerv1.Certificate{}, bitwardenCertificateAssetName); err != nil { + if apierrors.IsNotFound(err) { + return nil + } + return fmt.Errorf("failed to delete bitwarden certificate: %w", err) + } return nil }Add import:
+ apierrors "k8s.io/apimachinery/pkg/api/errors"Optional (if you prefer stronger observability), after successful deletion:
// r.eventRecorder.Eventf(es, corev1.EventTypeNormal, "Reconciled", "certificate resource %s deleted", certificateName)Also applies to: 69-71
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (3)
pkg/controller/external_secrets/certificate.go(1 hunks)pkg/controller/external_secrets/service_test.go(1 hunks)pkg/controller/external_secrets/services.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/controller/external_secrets/service_test.go
🔇 Additional comments (2)
pkg/controller/external_secrets/services.go (1)
32-34: No changes needed: common.DeleteObject swallows NotFound (see utils.go:495) and your code has no redundant NotFound check.pkg/controller/external_secrets/certificate.go (1)
24-35: Nice separation of concerns via small helpers.Splitting webhook and Bitwarden flows makes the control paths clearer and easier to reason about.
| // Bitwarden certificate handling is independent of cert-manager configuration | ||
| // Only handle bitwarden certificates when bitwarden is enabled | ||
| if isBitwardenConfigEnabled(es) { | ||
| bitwardenConfig := es.Spec.ExternalSecretsConfig.BitwardenSecretManagerProvider | ||
|
|
||
| // If a secret reference is provided, validate it exists instead of creating a certificate | ||
| if bitwardenConfig.SecretRef.Name != "" { | ||
| return r.assertSecretRefExists(es, es.Spec.ExternalSecretsConfig.BitwardenSecretManagerProvider) | ||
| } | ||
| if err := r.createOrApplyCertificate(es, resourceLabels, bitwardenCertificateAssetName, recon); err != nil { | ||
| return err | ||
| return r.assertSecretRefExists(es, bitwardenConfig) | ||
| } | ||
|
|
||
| // Create or update bitwarden certificate | ||
| return r.createOrApplyCertificate(es, resourceLabels, bitwardenCertificateAssetName, recon) | ||
| } |
There was a problem hiding this comment.
Bug: Bitwarden certificate is created even when cert-manager is disabled (will fail).
Current flow calls createOrApplyCertificate without guarding on cert-manager. With CertManagerConfig disabled/missing, updateCertificateParams errors on empty IssuerRef (or the CRD may not exist), causing reconcile failures. Guard creation behind isCertManagerConfigEnabled and no-op (or warn) when SecretRef is absent.
Apply:
-// handleBitwardenCertificate manages the bitwarden certificate lifecycle
+// handleBitwardenCertificate manages the Bitwarden certificate lifecycle
func (r *Reconciler) handleBitwardenCertificate(es *operatorv1alpha1.ExternalSecrets, resourceLabels map[string]string, recon bool) error {
- // Bitwarden certificate handling is independent of cert-manager configuration
- // Only handle bitwarden certificates when bitwarden is enabled
+ // Only handle Bitwarden certificates when Bitwarden is enabled
if isBitwardenConfigEnabled(es) {
bitwardenConfig := es.Spec.ExternalSecretsConfig.BitwardenSecretManagerProvider
-
- // If a secret reference is provided, validate it exists instead of creating a certificate
+
+ // If a secret reference is provided, validate it exists instead of creating a certificate
if bitwardenConfig.SecretRef.Name != "" {
return r.assertSecretRefExists(es, bitwardenConfig)
}
-
- // Create or update bitwarden certificate
- return r.createOrApplyCertificate(es, resourceLabels, bitwardenCertificateAssetName, recon)
+
+ // Create or update Bitwarden certificate only when cert-manager is enabled.
+ if !isCertManagerConfigEnabled(es) {
+ r.log.V(2).Info("Skipping Bitwarden certificate creation: cert-manager disabled and SecretRef not provided")
+ return nil
+ }
+ return r.createOrApplyCertificate(es, resourceLabels, bitwardenCertificateAssetName, recon)
}
// If bitwarden is not enabled, clean up any existing bitwarden certificate
return r.cleanupBitwardenCertificate()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Bitwarden certificate handling is independent of cert-manager configuration | |
| // Only handle bitwarden certificates when bitwarden is enabled | |
| if isBitwardenConfigEnabled(es) { | |
| bitwardenConfig := es.Spec.ExternalSecretsConfig.BitwardenSecretManagerProvider | |
| // If a secret reference is provided, validate it exists instead of creating a certificate | |
| if bitwardenConfig.SecretRef.Name != "" { | |
| return r.assertSecretRefExists(es, es.Spec.ExternalSecretsConfig.BitwardenSecretManagerProvider) | |
| } | |
| if err := r.createOrApplyCertificate(es, resourceLabels, bitwardenCertificateAssetName, recon); err != nil { | |
| return err | |
| return r.assertSecretRefExists(es, bitwardenConfig) | |
| } | |
| // Create or update bitwarden certificate | |
| return r.createOrApplyCertificate(es, resourceLabels, bitwardenCertificateAssetName, recon) | |
| } | |
| // handleBitwardenCertificate manages the Bitwarden certificate lifecycle | |
| func (r *Reconciler) handleBitwardenCertificate(es *operatorv1alpha1.ExternalSecrets, resourceLabels map[string]string, recon bool) error { | |
| // Only handle Bitwarden certificates when Bitwarden is enabled | |
| if isBitwardenConfigEnabled(es) { | |
| bitwardenConfig := es.Spec.ExternalSecretsConfig.BitwardenSecretManagerProvider | |
| // If a secret reference is provided, validate it exists instead of creating a certificate | |
| if bitwardenConfig.SecretRef.Name != "" { | |
| return r.assertSecretRefExists(es, bitwardenConfig) | |
| } | |
| // Create or update Bitwarden certificate only when cert-manager is enabled. | |
| if !isCertManagerConfigEnabled(es) { | |
| r.log.V(2).Info("Skipping Bitwarden certificate creation: cert-manager disabled and SecretRef not provided") | |
| return nil | |
| } | |
| return r.createOrApplyCertificate(es, resourceLabels, bitwardenCertificateAssetName, recon) | |
| } | |
| // If Bitwarden is not enabled, clean up any existing Bitwarden certificate | |
| return r.cleanupBitwardenCertificate() | |
| } |
🤖 Prompt for AI Agents
In pkg/controller/external_secrets/certificate.go around lines 49 to 61, the
code creates or updates a Bitwarden certificate even when cert-manager is
disabled, causing reconcile failures; modify the flow so that after checking
isBitwardenConfigEnabled(es) and handling SecretRef.Name, you additionally check
isCertManagerConfigEnabled(es) (or equivalent CertManagerConfig presence/enabled
flag) before calling createOrApplyCertificate; if cert-manager is disabled and
no SecretRef is provided, skip certificate creation and return a no-op (or log a
warning and return nil) so reconcile does not fail due to missing issuer/CRD.
| // If bitwarden is not enabled, clean up any existing bitwarden certificate | ||
| return r.cleanupBitwardenCertificate() | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify DeleteObject uses the correct namespace for deletion.
We pass only assetName here; if DeleteObject decodes the asset without applying updateNamespace(es, ...), it may target the wrong namespace and fail to delete. If so, consider decoding the object, calling updateNamespace, and deleting via the client.
Also applies to: 69-71
🏁 Script executed:
#!/bin/bash
# Inspect DeleteObject and decode helpers for namespace handling.
rg -n 'func\s+DeleteObject\(' -n -C5 pkg/controller/common | cat
rg -n 'Decode(Service|Certificate)ObjBytes' -C3 pkg/controller/common | cat
rg -n 'updateNamespace\(' -n pkg/controller/common | catLength of output: 2503
🏁 Script executed:
#!/bin/bash
# Inspect cleanupBitwardenCertificate implementation for DeleteObject usage.
rg -n 'cleanupBitwardenCertificate' -C5 pkg/controller/external_secrets/certificate.goLength of output: 672
🏁 Script executed:
#!/bin/bash
# Search for updateNamespace definition and usage across repository
rg -n 'updateNamespace' -C3 .Length of output: 6973
Ensure delete uses ExternalSecrets namespace in cleanupBitwardenCertificate
common.DeleteObject decodes the Certificate asset without applying updateNamespace, so it will delete in the asset’s own namespace (from the YAML) rather than the ExternalSecrets CR’s namespace—making cleanup silently fail. In pkg/controller/external_secrets/certificate.go (cleanupBitwardenCertificate), decode the asset, call updateNamespace(cert, es), then delete via r.CtrlClient.Delete.
🤖 Prompt for AI Agents
In pkg/controller/external_secrets/certificate.go around lines 63 to 65, the
cleanupBitwardenCertificate call deletes the Certificate using
common.DeleteObject which decodes the asset without applying updateNamespace,
causing a delete in the asset's YAML namespace; decode the Certificate asset
into a cert object, call updateNamespace(&cert, es) to set the ExternalSecrets
CR namespace, then perform the delete using r.CtrlClient.Delete(ctx, &cert)
(handle NotFound gracefully and return any other errors).
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@siddhibhor-56: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
PR is for removing resources created external-secrets bitwarden-server component, when externalsecrets.operator.openshift.io.spec.externalSecretsConfig.bitwardenSecretManagerProvider.enabled is disable,
Below resources will deleted, if exists, when cert-manager is configured:
Certificates
Deployment
Service
ServiceAccount