diff --git a/bundle/manifests/external-secrets-operator.clusterserviceversion.yaml b/bundle/manifests/external-secrets-operator.clusterserviceversion.yaml index 68e75fcb1..c00b4f55a 100644 --- a/bundle/manifests/external-secrets-operator.clusterserviceversion.yaml +++ b/bundle/manifests/external-secrets-operator.clusterserviceversion.yaml @@ -204,7 +204,7 @@ metadata: categories: Security console.openshift.io/disable-operand-delete: "true" containerImage: openshift.io/external-secrets-operator:latest - createdAt: "2025-08-20T18:17:42Z" + createdAt: "2025-09-08T08:44:22Z" features.operators.openshift.io/cnf: "false" features.operators.openshift.io/cni: "false" features.operators.openshift.io/csi: "false" @@ -412,6 +412,16 @@ spec: - cert-manager.io resources: - certificates + verbs: + - create + - delete + - get + - list + - update + - watch + - apiGroups: + - cert-manager.io + resources: - clusterissuers - issuers verbs: diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index c790aa378..82d648ed4 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -74,6 +74,16 @@ rules: - cert-manager.io resources: - certificates + verbs: + - create + - delete + - get + - list + - update + - watch +- apiGroups: + - cert-manager.io + resources: - clusterissuers - issuers verbs: diff --git a/pkg/controller/common/utils.go b/pkg/controller/common/utils.go index 1f3886a3d..cdea9da3e 100644 --- a/pkg/controller/common/utils.go +++ b/pkg/controller/common/utils.go @@ -480,6 +480,10 @@ func DeleteObject(ctx context.Context, ctrlClient operatorclient.CtrlClient, obj o = DecodeSecretObjBytes(assets.MustAsset(assetName)) case *corev1.ServiceAccount: o = DecodeServiceAccountObjBytes(assets.MustAsset(assetName)) + case *corev1.Service: + o = DecodeServiceObjBytes(assets.MustAsset(assetName)) + case *certmanagerv1.Certificate: + o = DecodeCertificateObjBytes(assets.MustAsset(assetName)) default: panic(fmt.Sprintf("unsupported object type: %T", obj)) } diff --git a/pkg/controller/external_secrets/certificate.go b/pkg/controller/external_secrets/certificate.go index 359ebf394..a745c28f4 100644 --- a/pkg/controller/external_secrets/certificate.go +++ b/pkg/controller/external_secrets/certificate.go @@ -21,20 +21,53 @@ var ( ) func (r *Reconciler) createOrApplyCertificates(es *operatorv1alpha1.ExternalSecrets, resourceLabels map[string]string, recon bool) error { - if isCertManagerConfigEnabled(es) { - if err := r.createOrApplyCertificate(es, resourceLabels, webhookCertificateAssetName, recon); err != nil { - return err - } + // Handle webhook certificate + if err := r.handleWebhookCertificate(es, resourceLabels, recon); err != nil { + return err + } + + // Handle bitwarden certificate + if err := r.handleBitwardenCertificate(es, resourceLabels, recon); err != nil { + return err } + return nil +} + +// handleWebhookCertificate manages the webhook certificate lifecycle +func (r *Reconciler) handleWebhookCertificate(es *operatorv1alpha1.ExternalSecrets, resourceLabels map[string]string, recon bool) error { + // Only create webhook certificate if cert-manager is enabled + if !isCertManagerConfigEnabled(es) { + return nil + } + + return r.createOrApplyCertificate(es, resourceLabels, webhookCertificateAssetName, recon) +} + +// 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 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) + } + + // If bitwarden is not enabled, clean up any existing bitwarden certificate + return r.cleanupBitwardenCertificate() +} + +// cleanupBitwardenCertificate removes the bitwarden certificate when bitwarden is disabled +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) } return nil } diff --git a/pkg/controller/external_secrets/certificate_test.go b/pkg/controller/external_secrets/certificate_test.go index 12883d64f..6f708930d 100644 --- a/pkg/controller/external_secrets/certificate_test.go +++ b/pkg/controller/external_secrets/certificate_test.go @@ -440,6 +440,91 @@ func TestCreateOrApplyCertificates(t *testing.T) { }, recon: false, }, + { + name: "bitwarden certificate deleted when bitwarden disabled", + preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { + m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) (bool, error) { + if ns.Name == serviceExternalSecretWebhookName { + return false, nil // webhook certificate doesn't exist, will be created + } + if ns.Name == "bitwarden-tls-certs" { + // Bitwarden certificate exists and needs to be deleted + cert := testCertificate(bitwardenCertificateAssetName) + cert.DeepCopyInto(obj.(*certmanagerv1.Certificate)) + return true, nil + } + if ns.Name == "test-issuer" { + return true, nil // issuer exists + } + return false, nil + }) + m.DeleteCalls(func(ctx context.Context, obj client.Object, opts ...client.DeleteOption) error { + return commontest.TestClientError // Simulates deletion failure + }) + m.CreateCalls(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + cert, ok := obj.(*certmanagerv1.Certificate) + if !ok { + return fmt.Errorf("expected *certmanagerv1.Certificate, got %T", obj) + } + if cert.Name == serviceExternalSecretWebhookName { + return nil // webhook certificate creation succeeds + } + t.Errorf("Unexpected create call for %s", cert.Name) + return nil + }) + m.GetCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) error { + if ns.Name == "test-issuer" && ns.Namespace == commontest.TestExternalSecretsNamespace { + testIssuer().DeepCopyInto(obj.(*certmanagerv1.Issuer)) + return nil + } + return fmt.Errorf("object not found") + }) + }, + es: func(es *v1alpha1.ExternalSecrets) { + es.Spec.ExternalSecretsConfig.CertManagerConfig.Enabled = "true" + es.Spec.ExternalSecretsConfig.CertManagerConfig.IssuerRef.Name = "test-issuer" + // Bitwarden is disabled (explicitly set to false), so deletion should be triggered + es.Spec.ExternalSecretsConfig.BitwardenSecretManagerProvider = &v1alpha1.BitwardenSecretManagerProvider{ + Enabled: "false", + } + }, + recon: false, + wantErr: "failed to delete bitwarden certificate: test client error", + }, + { + name: "no certificate operations when cert-manager disabled", + preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { + // Mock functions that should NOT be called + m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) (bool, error) { + t.Errorf("Exists() should not be called when cert-manager is disabled, but was called for %s", ns.Name) + return false, nil + }) + m.CreateCalls(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + t.Errorf("Create() should not be called when cert-manager is disabled") + return nil + }) + m.DeleteCalls(func(ctx context.Context, obj client.Object, opts ...client.DeleteOption) error { + t.Errorf("Delete() should not be called when cert-manager is disabled") + return nil + }) + m.GetCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) error { + t.Errorf("Get() should not be called when cert-manager is disabled") + return nil + }) + }, + es: func(es *v1alpha1.ExternalSecrets) { + // cert-manager is explicitly disabled (or not configured) + // This should trigger early return in createOrApplyCertificates + es.Spec.ExternalSecretsConfig = &v1alpha1.ExternalSecretsConfig{ + // No CertManagerConfig means cert-manager is disabled + BitwardenSecretManagerProvider: &v1alpha1.BitwardenSecretManagerProvider{ + Enabled: "true", // Even with Bitwarden enabled, no cert operations should happen + }, + } + }, + recon: false, + // No error expected - should return successfully without any operations + }, } for _, tt := range tests { diff --git a/pkg/controller/external_secrets/controller.go b/pkg/controller/external_secrets/controller.go index dd0a88fee..55e9321df 100644 --- a/pkg/controller/external_secrets/controller.go +++ b/pkg/controller/external_secrets/controller.go @@ -100,7 +100,8 @@ type Reconciler struct { // +kubebuilder:rbac:groups=admissionregistration.k8s.io,resources=validatingwebhookconfigurations,verbs=get;list;watch;create;update;patch // +kubebuilder:rbac:groups="",resources=events;secrets;services;serviceaccounts,verbs=get;list;watch;create;update;delete;patch // +kubebuilder:rbac:groups=apps,resources=deployments,verbs=get;list;watch;create;update;delete -// +kubebuilder:rbac:groups=cert-manager.io,resources=certificates;clusterissuers;issuers,verbs=get;list;watch;create;update +// +kubebuilder:rbac:groups=cert-manager.io,resources=clusterissuers;issuers,verbs=get;list;watch;create;update +// +kubebuilder:rbac:groups=cert-manager.io,resources=certificates,verbs=get;list;watch;create;update;delete // +kubebuilder:rbac:groups="",resources=namespaces,verbs=get;list;watch;create // +kubebuilder:rbac:groups="",resources=endpoints,verbs=get;list;watch;create diff --git a/pkg/controller/external_secrets/service_test.go b/pkg/controller/external_secrets/service_test.go index a43fcac32..2b29e6347 100644 --- a/pkg/controller/external_secrets/service_test.go +++ b/pkg/controller/external_secrets/service_test.go @@ -112,6 +112,38 @@ func TestCreateOrApplyServices(t *testing.T) { }, wantErr: `failed to create service external-secrets/external-secrets-webhook: test client error`, }, + { + name: "bitwarden service deleted when bitwarden disabled", + preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { + m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) (bool, error) { + switch o := obj.(type) { + case *corev1.Service: + if ns.Name == "bitwarden-sdk-server" { + svc := testService(bitwardenServiceAssetName) + svc.DeepCopyInto(o) + return true, nil // Service exists and needs to be deleted + } + // For webhook service + svc := testService("external-secrets/resources/service_external-secrets-webhook.yml") + svc.DeepCopyInto(o) + return true, nil + } + return false, nil + }) + m.DeleteCalls(func(ctx context.Context, obj client.Object, opts ...client.DeleteOption) error { + return commontest.TestClientError // Simulates deletion failure + }) + }, + updateExternalSecretsObj: func(es *operatorv1alpha1.ExternalSecrets) { + // Bitwarden is disabled (default), so deletion should be triggered + es.Spec.ExternalSecretsConfig = &operatorv1alpha1.ExternalSecretsConfig{ + BitwardenSecretManagerProvider: &operatorv1alpha1.BitwardenSecretManagerProvider{ + Enabled: "false", // This triggers deletion logic + }, + } + }, + wantErr: `failed to delete service object: test client error`, + }, } for _, tt := range tests { diff --git a/pkg/controller/external_secrets/services.go b/pkg/controller/external_secrets/services.go index 15ed73649..cdbee50af 100644 --- a/pkg/controller/external_secrets/services.go +++ b/pkg/controller/external_secrets/services.go @@ -29,6 +29,9 @@ func (r *Reconciler) createOrApplyServices(externalsecrets *operatorv1alpha1.Ext 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) + } continue } if err := r.createOrApplyServiceFromAsset(externalsecrets, service.assetName, resourceLabels, externalsecretsCreateRecon); err != nil {