Merge pull request #55 from bharath-b-rh/eso-167

openshift-merge-bot[bot] · web-flow · commit 3f97c27452ab · 2025-08-21T12:56:53.000Z
ESO-167: Removes resources created for cert-controller component when cert-manager is enabled
diff --git 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-18T11:50:12Z"
+    createdAt: "2025-08-20T18:17:42Z"
     features.operators.openshift.io/cnf: "false"
     features.operators.openshift.io/cni: "false"
     features.operators.openshift.io/csi: "false"
@@ -403,6 +403,7 @@ spec:
           - deployments
           verbs:
           - create
+          - delete
           - get
           - list
           - update
diff --git a/bundle/manifests/operator.openshift.io_externalsecrets.yaml b/bundle/manifests/operator.openshift.io_externalsecrets.yaml
@@ -2,7 +2,7 @@ apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
   annotations:
-    controller-gen.kubebuilder.io/version: v0.16.1
+    controller-gen.kubebuilder.io/version: v0.17.3
   creationTimestamp: null
   name: externalsecrets.operator.openshift.io
 spec:
diff --git a/bundle/manifests/operator.openshift.io_externalsecretsmanagers.yaml b/bundle/manifests/operator.openshift.io_externalsecretsmanagers.yaml
@@ -2,7 +2,7 @@ apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
   annotations:
-    controller-gen.kubebuilder.io/version: v0.16.1
+    controller-gen.kubebuilder.io/version: v0.17.3
   creationTimestamp: null
   name: externalsecretsmanagers.operator.openshift.io
 spec:
diff --git a/config/crd/bases/operator.openshift.io_externalsecrets.yaml b/config/crd/bases/operator.openshift.io_externalsecrets.yaml
@@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
   annotations:
-    controller-gen.kubebuilder.io/version: v0.16.1
+    controller-gen.kubebuilder.io/version: v0.17.3
   name: externalsecrets.operator.openshift.io
 spec:
   group: operator.openshift.io
diff --git a/config/crd/bases/operator.openshift.io_externalsecretsmanagers.yaml b/config/crd/bases/operator.openshift.io_externalsecretsmanagers.yaml
@@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
   annotations:
-    controller-gen.kubebuilder.io/version: v0.16.1
+    controller-gen.kubebuilder.io/version: v0.17.3
   name: externalsecretsmanagers.operator.openshift.io
 spec:
   group: operator.openshift.io
diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml
@@ -65,6 +65,7 @@ rules:
   - deployments
   verbs:
   - create
+  - delete
   - get
   - list
   - update
diff --git a/pkg/controller/common/utils.go b/pkg/controller/common/utils.go
@@ -12,6 +12,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
 	crdv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
+	"k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/serializer"
 	"k8s.io/apimachinery/pkg/types"
@@ -22,6 +23,7 @@ import (
 
 	operatorv1alpha1 "github.com/openshift/external-secrets-operator/api/v1alpha1"
 	operatorclient "github.com/openshift/external-secrets-operator/pkg/controller/client"
+	"github.com/openshift/external-secrets-operator/pkg/operator/assets"
 )
 
 var (
@@ -460,3 +462,35 @@ func (n *Now) Reset() {
 
 	n.done.Store(0)
 }
+
+// DeleteObject is for deleting an object mentioned in the asset file passed.
+// Does not treat NotFound as an error, and can be extended in future with arg, whether to
+// return an error.
+// TODO: Extend for other object types as and when required.
+func DeleteObject(ctx context.Context, ctrlClient operatorclient.CtrlClient, obj client.Object, assetName string) error {
+	var o client.Object
+	switch obj.(type) {
+	case *rbacv1.ClusterRole:
+		o = DecodeClusterRoleObjBytes(assets.MustAsset(assetName))
+	case *rbacv1.ClusterRoleBinding:
+		o = DecodeClusterRoleBindingObjBytes(assets.MustAsset(assetName))
+	case *appsv1.Deployment:
+		o = DecodeDeploymentObjBytes(assets.MustAsset(assetName))
+	case *corev1.Secret:
+		o = DecodeSecretObjBytes(assets.MustAsset(assetName))
+	case *corev1.ServiceAccount:
+		o = DecodeServiceAccountObjBytes(assets.MustAsset(assetName))
+	default:
+		panic(fmt.Sprintf("unsupported object type: %T", obj))
+	}
+	exists, err := ctrlClient.Exists(ctx, types.NamespacedName{Name: o.GetName(), Namespace: o.GetNamespace()}, o)
+	if err != nil {
+		return err
+	}
+	if exists {
+		if err := ctrlClient.Delete(ctx, o); err != nil && !errors.IsNotFound(err) {
+			return err
+		}
+	}
+	return nil
+}
diff --git a/pkg/controller/external_secrets/controller.go b/pkg/controller/external_secrets/controller.go
@@ -99,7 +99,7 @@ type Reconciler struct {
 // +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=roles;rolebindings;clusterroles;clusterrolebindings,verbs=get;list;watch;create;update;patch;delete
 // +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
+// +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="",resources=namespaces,verbs=get;list;watch;create
 
diff --git a/pkg/controller/external_secrets/deployments.go b/pkg/controller/external_secrets/deployments.go
@@ -48,6 +48,9 @@ func (r *Reconciler) createOrApplyDeployments(externalsecrets *operatorv1alpha1.
 	// Apply deployments based on the specified conditions.
 	for _, d := range deployments {
 		if !d.condition {
+			if err := common.DeleteObject(r.ctx, r.CtrlClient, &appsv1.Deployment{}, d.assetName); err != nil {
+				return fmt.Errorf("failed to delete deployment resource: %w", err)
+			}
 			continue
 		}
 		if err := r.createOrApplyDeploymentFromAsset(externalsecrets, d.assetName, resourceLabels, externalsecretsCreateRecon); err != nil {
diff --git a/pkg/controller/external_secrets/deployments_test.go b/pkg/controller/external_secrets/deployments_test.go
@@ -6,8 +6,10 @@ import (
 
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
@@ -352,6 +354,57 @@ func TestCreateOrApplyDeployments(t *testing.T) {
 			},
 			wantErr: `failed to update resource requirements: invalid resource requirements: [spec.resources.requests[test]: Invalid value: test: must be a standard resource type or fully qualified, spec.resources.requests[test]: Invalid value: test: must be a standard resource for containers]`,
 		},
+		{
+			name: "cert-controller deployment deletion fails",
+			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 *appsv1.Deployment:
+						deployment := testDeployment(certControllerDeploymentAssetName)
+						deployment.DeepCopyInto(o)
+					}
+					return true, nil
+				})
+				m.DeleteCalls(func(ctx context.Context, obj client.Object, opts ...client.DeleteOption) error {
+					return commontest.TestClientError
+				})
+			},
+			updateExternalSecrets: func(i *v1alpha1.ExternalSecrets) {
+				if i.Spec.ExternalSecretsConfig == nil {
+					i.Spec.ExternalSecretsConfig = &v1alpha1.ExternalSecretsConfig{
+						CertManagerConfig: &v1alpha1.CertManagerConfig{
+							Enabled: "true",
+						},
+					}
+				}
+			},
+			wantErr: `failed to delete deployment resource: test client error`,
+		},
+		{
+			name: "cert-controller deployment NotFound, deletion is marked successful",
+			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 *appsv1.Deployment:
+						deployment := testDeployment(certControllerDeploymentAssetName)
+						deployment.DeepCopyInto(o)
+					}
+					return true, nil
+				})
+				m.DeleteCalls(func(ctx context.Context, obj client.Object, opts ...client.DeleteOption) error {
+					return errors.NewNotFound(schema.GroupResource{}, obj.GetName())
+				})
+			},
+			updateExternalSecrets: func(i *v1alpha1.ExternalSecrets) {
+				if i.Spec.ExternalSecretsConfig == nil {
+					i.Spec.ExternalSecretsConfig = &v1alpha1.ExternalSecretsConfig{
+						CertManagerConfig: &v1alpha1.CertManagerConfig{
+							Enabled: "true",
+						},
+					}
+				}
+			},
+		},
 	}
 
 	for _, tt := range tests {
diff --git a/pkg/controller/external_secrets/rbacs.go b/pkg/controller/external_secrets/rbacs.go
@@ -6,6 +6,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
 	"k8s.io/apimachinery/pkg/types"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	operatorv1alpha1 "github.com/openshift/external-secrets-operator/api/v1alpha1"
 	"github.com/openshift/external-secrets-operator/pkg/controller/common"
@@ -78,7 +79,15 @@ func (r *Reconciler) createOrApplyControllerRBACResources(es *operatorv1alpha1.E
 // the main external-secrets operand cert-controller.
 func (r *Reconciler) createOrApplyCertControllerRBACResources(es *operatorv1alpha1.ExternalSecrets, serviceAccountName string, resourceLabels map[string]string, recon bool) error {
 	if isCertManagerConfigEnabled(es) {
-		r.log.V(4).Info("skipping cert-controller rbac resources reconciliation, as cert-manager config is enabled")
+		r.log.V(4).Info("deleting cert-controller rbac resources if exists, as cert-manager config is enabled")
+		for asset, assetType := range map[string]client.Object{
+			certControllerClusterRoleAssetName:        &rbacv1.ClusterRole{},
+			certControllerClusterRoleBindingAssetName: &rbacv1.ClusterRoleBinding{},
+		} {
+			if err := common.DeleteObject(r.ctx, r.CtrlClient, assetType, asset); err != nil {
+				return fmt.Errorf("failed to delete cert-controller rbac resource: %w", err)
+			}
+		}
 		return nil
 	}
 
diff --git a/pkg/controller/external_secrets/rbacs_test.go b/pkg/controller/external_secrets/rbacs_test.go
@@ -289,6 +289,53 @@ func TestCreateOrApplyRBACResource(t *testing.T) {
 		{
 			name: "rolebindings creation successful",
 		},
+		{
+			name: "cert-controller clusterrole deletion fails",
+			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 *rbacv1.ClusterRole:
+						sa := testClusterRole(certControllerClusterRoleAssetName)
+						sa.DeepCopyInto(o)
+					}
+					return true, nil
+				})
+				m.DeleteCalls(func(ctx context.Context, obj client.Object, opts ...client.DeleteOption) error {
+					return commontest.TestClientError
+				})
+			},
+			updateExternalSecretsObj: func(es *operatorv1alpha1.ExternalSecrets) {
+				if es.Spec.ExternalSecretsConfig == nil {
+					es.Spec.ExternalSecretsConfig = &operatorv1alpha1.ExternalSecretsConfig{
+						CertManagerConfig: &operatorv1alpha1.CertManagerConfig{
+							Enabled: "true",
+						},
+					}
+				}
+			},
+			wantErr: `failed to delete cert-controller rbac resource: test client error`,
+		},
+		{
+			name: "cert-controller clusterrole deletion is successful when object not exists",
+			preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) {
+				m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) (bool, error) {
+					return false, nil
+				})
+				m.DeleteCalls(func(ctx context.Context, obj client.Object, opts ...client.DeleteOption) error {
+					return commontest.TestClientError
+				})
+			},
+			updateExternalSecretsObj: func(es *operatorv1alpha1.ExternalSecrets) {
+				if es.Spec.ExternalSecretsConfig == nil {
+					es.Spec.ExternalSecretsConfig = &operatorv1alpha1.ExternalSecretsConfig{
+						CertManagerConfig: &operatorv1alpha1.CertManagerConfig{
+							Enabled: "true",
+						},
+					}
+				}
+			},
+			wantErr: ``,
+		},
 	}
 
 	for _, tt := range tests {
diff --git a/pkg/controller/external_secrets/secret.go b/pkg/controller/external_secrets/secret.go
@@ -14,7 +14,10 @@ import (
 func (r *Reconciler) createOrApplySecret(es *operatorv1alpha1.ExternalSecrets, resourceLabels map[string]string, recon bool) error {
 	// secrets are only created if isCertManagerConfig is not enabled
 	if isCertManagerConfigEnabled(es) {
-		r.log.V(4).Info("cert-manager config is enabled, skipping webhook component secret resource creation")
+		r.log.V(4).Info("cert-manager config is enabled, deleting webhook component secret resource if exists")
+		if err := common.DeleteObject(r.ctx, r.CtrlClient, &corev1.Secret{}, webhookTLSSecretAssetName); err != nil {
+			return fmt.Errorf("failed to delete secret resource of webhook component: %w", err)
+		}
 		return nil
 	}
 
diff --git a/pkg/controller/external_secrets/secret_test.go b/pkg/controller/external_secrets/secret_test.go
@@ -183,6 +183,30 @@ func TestCreateOrApplySecret(t *testing.T) {
 			preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) {
 			},
 		},
+		{
+			name: "webhook secret deletion fails",
+			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.Secret:
+						secret := testSecret(webhookTLSSecretAssetName)
+						secret.DeepCopyInto(o)
+					}
+					return true, nil
+				})
+				m.DeleteCalls(func(ctx context.Context, obj client.Object, opts ...client.DeleteOption) error {
+					return commontest.TestClientError
+				})
+			},
+			es: func(es *v1alpha1.ExternalSecrets) {
+				es.Spec.ExternalSecretsConfig = &v1alpha1.ExternalSecretsConfig{
+					CertManagerConfig: &v1alpha1.CertManagerConfig{
+						Enabled: "true",
+					},
+				}
+			},
+			wantErr: `failed to delete secret resource of webhook component: test client error`,
+		},
 	}
 
 	for _, tt := range tests {
diff --git a/pkg/controller/external_secrets/serviceaccounts.go b/pkg/controller/external_secrets/serviceaccounts.go
@@ -37,6 +37,9 @@ func (r *Reconciler) createOrApplyServiceAccounts(externalsecrets *operatorv1alp
 
 	for _, serviceAccount := range serviceAccountsToCreate {
 		if !serviceAccount.condition {
+			if err := common.DeleteObject(r.ctx, r.CtrlClient, &corev1.ServiceAccount{}, serviceAccount.assetName); err != nil {
+				return fmt.Errorf("failed to delete cert-controller serviceaccount: %w", err)
+			}
 			continue
 		}
 
diff --git a/pkg/controller/external_secrets/serviceaccounts_test.go b/pkg/controller/external_secrets/serviceaccounts_test.go
@@ -116,6 +116,30 @@ func TestCreateOrApplyServiceAccounts(t *testing.T) {
 			},
 			wantErr: "failed to create serviceaccount external-secrets/external-secrets: test client error",
 		},
+		{
+			name: "cert-controller serviceaccount skipped when cert-manager enabled",
+			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.ServiceAccount:
+						sa := testServiceAccount(certControllerServiceAccountAssetName)
+						sa.DeepCopyInto(o)
+					}
+					return true, nil
+				})
+				m.DeleteCalls(func(ctx context.Context, obj client.Object, opts ...client.DeleteOption) error {
+					return commontest.TestClientError
+				})
+			},
+			updateExternalSecretsObj: func(es *operatorv1alpha1.ExternalSecrets) {
+				es.Spec.ExternalSecretsConfig = &operatorv1alpha1.ExternalSecretsConfig{
+					CertManagerConfig: &operatorv1alpha1.CertManagerConfig{
+						Enabled: "true",
+					},
+				}
+			},
+			wantErr: `failed to delete cert-controller serviceaccount: test client error`,
+		},
 	}
 
 	for _, tt := range tests {

-Original file line number
+Diff line change
   - deployments
   verbs:
   - create
 +  - delete
   - get
   - list
   - update