Fix for Whisker/Goldmane FV Test Finalizer Issue

pkg/controller/goldmane/controller.go

@@ -18,7 +18,9 @@ import (
 	"context"
 	"fmt"
 
+	"github.com/go-logr/logr"
 	v1 "k8s.io/api/apps/v1"
+	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -154,13 +156,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, request reconcile.Request) (
 	if err != nil {
 		r.status.SetDegraded(operatorv1.ResourceReadError, "Error querying Goldmane CR", err, reqLogger)
 		return reconcile.Result{}, err
-	} else if goldmaneCR == nil {
-		r.status.OnCRNotFound()
-		return reconcile.Result{}, r.maintainFinalizer(ctx, nil)
 	}
-	r.status.OnCRFound()
-	// SetMetaData in the TigeraStatus such as observedGenerations.
-	defer r.status.SetMetaData(&goldmaneCR.ObjectMeta)
 
 	variant, installation, err := utils.GetInstallation(ctx, r.cli)
 	if err != nil {
@@ -169,6 +165,15 @@ func (r *Reconciler) Reconcile(ctx context.Context, request reconcile.Request) (
 		return reconcile.Result{}, nil
 	}
 
+	if goldmaneCR == nil {
+		r.status.OnCRNotFound()
+		// Handle deletion: delete deployment before removing finalizer
+		return r.handleDeletion(ctx, variant, installation, reqLogger)
+	}
+	r.status.OnCRFound()
+	// SetMetaData in the TigeraStatus such as observedGenerations.
+	defer r.status.SetMetaData(&goldmaneCR.ObjectMeta)
+
 	mgmtClusterConnectionCR, err := utils.GetIfExists[operatorv1.ManagementClusterConnection](ctx, utils.DefaultTSEEInstanceKey, r.cli)
 	if err != nil {
 		r.status.SetDegraded(operatorv1.ResourceReadError, "Error querying ManagementClusterConnection CR", err, reqLogger)
@@ -254,6 +259,31 @@ func (r *Reconciler) Reconcile(ctx context.Context, request reconcile.Request) (
 	return reconcile.Result{}, nil
 }
 
+func (r *Reconciler) handleDeletion(ctx context.Context, variant operatorv1.ProductVariant, installation *operatorv1.InstallationSpec, reqLogger logr.Logger) (reconcile.Result, error) {
+	// When the Goldmane CR is deleted, we need to clean up the deployment before removing the finalizer.
+	// Delete the goldmane deployment directly
+	goldmaneDeployment := &v1.Deployment{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      goldmane.GoldmaneDeploymentName,
+			Namespace: goldmane.GoldmaneNamespace,
+		},
+	}
+
+	err := r.cli.Delete(ctx, goldmaneDeployment)
+	if err != nil && !errors.IsNotFound(err) {
+		reqLogger.Error(err, "Error deleting goldmane deployment during cleanup")
+		return reconcile.Result{}, err
+	}
+
+	// Now try to maintain the finalizer - it will only be removed if all resources and pods are cleaned up
+	if err := r.maintainFinalizer(ctx, nil); err != nil {
+		reqLogger.Error(err, "Error maintaining finalizer during deletion")
+		return reconcile.Result{}, err
+	}
+
+	return reconcile.Result{}, nil
+}
+
 func (r *Reconciler) maintainFinalizer(ctx context.Context, goldmaneCr client.Object) error {
 	// These objects require graceful termination before the CNI plugin is torn down.
 	goldmaneDeployment := &v1.Deployment{ObjectMeta: metav1.ObjectMeta{Namespace: common.CalicoNamespace, Name: goldmane.GoldmaneDeploymentName}}

pkg/controller/policyrecommendation/policyrecommendation_controller.go

@@ -471,26 +471,8 @@ func (r *ReconcilePolicyRecommendation) Reconcile(ctx context.Context, request r
 		return reconcile.Result{}, err
 	}
 
-	setUp := render.NewSetup(&render.SetUpConfiguration{
-		OpenShift:       r.provider.IsOpenShift(),
-		Installation:    installation,
-		PullSecrets:     pullSecrets,
-		Namespace:       helper.InstallNamespace(),
-		PSS:             render.PSSRestricted,
-		CreateNamespace: !tenant.MultiTenant(),
-	})
-
 	// Prepend PolicyRecommendation before certificate creation
 	components = append([]render.Component{component}, components...)
-	setupHandler := defaultHandler
-	if tenant.MultiTenant() {
-		// In standard installs, the PolicyRecommendation CR owns all the objects. For multi-tenant, pull secrets are owned by the Tenant instance.
-		setupHandler = utils.NewComponentHandler(log, r.client, r.scheme, tenant)
-	}
-	if err := setupHandler.CreateOrUpdateOrDelete(ctx, setUp, r.status); err != nil {
-		r.status.SetDegraded(operatorv1.ResourceUpdateError, "Error creating / updating resource", err, logc)
-		return reconcile.Result{}, err
-	}
 
 	for _, cmp := range components {
 		if err := defaultHandler.CreateOrUpdateOrDelete(context.Background(), cmp, r.status); err != nil {

pkg/controller/policyrecommendation/policyrecommendation_controller_test.go

@@ -169,45 +169,6 @@ var _ = Describe("PolicyRecommendation controller tests", func() {
 		r.policyRecScopeWatchReady.MarkAsReady()
 	})
 
-	It("should reconcile namespace, role binding and pull secrts", func() {
-		result, err := r.Reconcile(ctx, reconcile.Request{})
-		Expect(err).NotTo(HaveOccurred())
-		Expect(result.RequeueAfter).To(Equal(0 * time.Second))
-
-		namespace := corev1.Namespace{
-			TypeMeta: metav1.TypeMeta{Kind: "Namespace", APIVersion: "v1"},
-		}
-		Expect(c.Get(ctx, client.ObjectKey{
-			Name: render.PolicyRecommendationNamespace,
-		}, &namespace)).NotTo(HaveOccurred())
-		Expect(namespace.Labels["pod-security.kubernetes.io/enforce"]).To(Equal("restricted"))
-		Expect(namespace.Labels["pod-security.kubernetes.io/enforce-version"]).To(Equal("latest"))
-
-		// Expect operator role binding to be created
-		rb := rbacv1.RoleBinding{
-			ObjectMeta: metav1.ObjectMeta{},
-		}
-		Expect(c.Get(ctx, client.ObjectKey{
-			Name:      render.TigeraOperatorSecrets,
-			Namespace: render.PolicyRecommendationNamespace,
-		}, &rb)).NotTo(HaveOccurred())
-		Expect(rb.OwnerReferences).To(HaveLen(1))
-		ownerRoleBinding := rb.OwnerReferences[0]
-		Expect(ownerRoleBinding.Kind).To(Equal("PolicyRecommendation"))
-
-		// Expect pull secrets to be created
-		pullSecrets := corev1.Secret{
-			TypeMeta: metav1.TypeMeta{Kind: "Secret", APIVersion: "v1"},
-		}
-		Expect(c.Get(ctx, client.ObjectKey{
-			Name:      "tigera-pull-secret",
-			Namespace: render.PolicyRecommendationNamespace,
-		}, &pullSecrets)).NotTo(HaveOccurred())
-		Expect(pullSecrets.OwnerReferences).To(HaveLen(1))
-		pullSecret := pullSecrets.OwnerReferences[0]
-		Expect(pullSecret.Kind).To(Equal("PolicyRecommendation"))
-	})
-
 	Context("image reconciliation", func() {
 		It("should use builtin images", func() {
 			_, err := r.Reconcile(ctx, reconcile.Request{})
@@ -492,64 +453,6 @@ var _ = Describe("PolicyRecommendation controller tests", func() {
 				_, err = r.Reconcile(ctx, reconcile.Request{NamespacedName: types.NamespacedName{Namespace: tenantANamespace}})
 				Expect(err).ShouldNot(HaveOccurred())
 			})
-
-			It("should reconcile pull secrets and role bindings", func() {
-				// Create the Tenant resources for tenant-a.
-				tenantA := &operatorv1.Tenant{
-					ObjectMeta: metav1.ObjectMeta{
-						Name:      "default",
-						Namespace: tenantANamespace,
-					},
-					Spec: operatorv1.TenantSpec{ID: "tenant-a"},
-				}
-				Expect(c.Create(ctx, tenantA)).NotTo(HaveOccurred())
-				certificateManagerTenantA, err := certificatemanager.Create(c, nil, "", tenantANamespace, certificatemanager.AllowCACreation(), certificatemanager.WithTenant(tenantA))
-				Expect(err).NotTo(HaveOccurred())
-				Expect(c.Create(ctx, certificateManagerTenantA.KeyPair().Secret(tenantANamespace)))
-				Expect(c.Create(ctx, certificateManagerTenantA.CreateTrustedBundle().ConfigMap(tenantANamespace))).NotTo(HaveOccurred())
-
-				linseedTLSTenantA, err := certificateManagerTenantA.GetOrCreateKeyPair(c, render.TigeraLinseedSecret, tenantANamespace, []string{render.TigeraLinseedSecret})
-				Expect(err).NotTo(HaveOccurred())
-				Expect(c.Create(ctx, linseedTLSTenantA.Secret(tenantANamespace))).NotTo(HaveOccurred())
-
-				Expect(c.Create(ctx, &operatorv1.PolicyRecommendation{
-					ObjectMeta: metav1.ObjectMeta{
-						Name:      "tigera-secure",
-						Namespace: tenantANamespace,
-					},
-				})).NotTo(HaveOccurred())
-
-				_, err = r.Reconcile(ctx, reconcile.Request{
-					NamespacedName: types.NamespacedName{
-						Namespace: tenantANamespace,
-					},
-				})
-				Expect(err).ShouldNot(HaveOccurred())
-
-				// Expect operator role binding to be created
-				rb := rbacv1.RoleBinding{
-					ObjectMeta: metav1.ObjectMeta{},
-				}
-				Expect(c.Get(ctx, client.ObjectKey{
-					Name:      render.TigeraOperatorSecrets,
-					Namespace: tenantANamespace,
-				}, &rb)).NotTo(HaveOccurred())
-				Expect(rb.OwnerReferences).To(HaveLen(1))
-				ownerRoleBinding := rb.OwnerReferences[0]
-				Expect(ownerRoleBinding.Kind).To(Equal("Tenant"))
-
-				// Expect pull secrets to be created
-				pullSecrets := corev1.Secret{
-					TypeMeta: metav1.TypeMeta{Kind: "Secret", APIVersion: "v1"},
-				}
-				Expect(c.Get(ctx, client.ObjectKey{
-					Name:      "tigera-pull-secret",
-					Namespace: tenantANamespace,
-				}, &pullSecrets)).NotTo(HaveOccurred())
-				Expect(pullSecrets.OwnerReferences).To(HaveLen(1))
-				pullSecret := pullSecrets.OwnerReferences[0]
-				Expect(pullSecret.Kind).To(Equal("Tenant"))
-			})
 		})
 	})
 })

pkg/controller/whisker/controller.go

@@ -18,7 +18,9 @@ import (
 	"context"
 	"fmt"
 
+	"github.com/go-logr/logr"
 	v1 "k8s.io/api/apps/v1"
+	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -157,13 +159,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, request reconcile.Request) (
 	if err != nil {
 		r.status.SetDegraded(operatorv1.ResourceReadError, "Error querying Whisker CR", err, reqLogger)
 		return reconcile.Result{}, err
-	} else if whiskerCR == nil {
-		r.status.OnCRNotFound()
-		return reconcile.Result{}, r.maintainFinalizer(ctx, nil)
 	}
-	r.status.OnCRFound()
-	// SetMetaData in the TigeraStatus such as observedGenerations.
-	defer r.status.SetMetaData(&whiskerCR.ObjectMeta)
 
 	variant, installation, err := utils.GetInstallation(ctx, r.cli)
 	if err != nil {
@@ -172,6 +168,15 @@ func (r *Reconciler) Reconcile(ctx context.Context, request reconcile.Request) (
 		return reconcile.Result{}, nil
 	}
 
+	if whiskerCR == nil {
+		r.status.OnCRNotFound()
+		// Handle deletion: process components to clean up resources before removing finalizer
+		return r.handleDeletion(ctx, variant, installation, reqLogger)
+	}
+	r.status.OnCRFound()
+	// SetMetaData in the TigeraStatus such as observedGenerations.
+	defer r.status.SetMetaData(&whiskerCR.ObjectMeta)
+
 	if goldmaneCR, err := utils.GetIfExists[operatorv1.Goldmane](ctx, utils.DefaultInstanceKey, r.cli); err != nil {
 		r.status.SetDegraded(operatorv1.ResourceReadError, "Error querying for Goldmane CR", err, reqLogger)
 		return reconcile.Result{}, err
@@ -278,6 +283,31 @@ func (r *Reconciler) Reconcile(ctx context.Context, request reconcile.Request) (
 	return reconcile.Result{}, nil
 }
 
+func (r *Reconciler) handleDeletion(ctx context.Context, variant operatorv1.ProductVariant, installation *operatorv1.InstallationSpec, reqLogger logr.Logger) (reconcile.Result, error) {
+	// When the Whisker CR is deleted, we need to clean up the deployment before removing the finalizer.
+	// Delete the whisker deployment directly
+	whiskerDeployment := &v1.Deployment{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      whisker.WhiskerDeploymentName,
+			Namespace: whisker.WhiskerNamespace,
+		},
+	}
+
+	err := r.cli.Delete(ctx, whiskerDeployment)
+	if err != nil && !errors.IsNotFound(err) {
+		reqLogger.Error(err, "Error deleting whisker deployment during cleanup")
+		return reconcile.Result{}, err
+	}
+
+	// Now try to maintain the finalizer - it will only be removed if all resources and pods are cleaned up
+	if err := r.maintainFinalizer(ctx, nil); err != nil {
+		reqLogger.Error(err, "Error maintaining finalizer during deletion")
+		return reconcile.Result{}, err
+	}
+
+	return reconcile.Result{}, nil
+}
+
 func updateWhiskerWithDefaults(instance *operatorv1.Whisker) {
 	if instance.Spec.Notifications == nil {
 		instance.Spec.Notifications = ptr.ToPtr(operatorv1.Enabled)

test/whisker_test.go

@@ -75,6 +75,9 @@ var _ = Describe("Tests for Whisker installation", func() {
 	})
 
 	AfterEach(func() {
+		By("Cleaning up resources after the test")
+		cleanupResources(c)
+
 		defer func() {
 			cancel()
 			Eventually(func() error {
@@ -87,9 +90,6 @@ var _ = Describe("Tests for Whisker installation", func() {
 			}, 60*time.Second).ShouldNot(HaveOccurred())
 		}()
 
-		By("Cleaning up resources after the test")
-		cleanupResources(c)
-
 		// Clean up Calico data that might be left behind.
 		Eventually(func() error {
 			cs := kubernetes.NewForConfigOrDie(mgr.GetConfig())
@@ -143,15 +143,15 @@ var _ = Describe("Tests for Whisker installation", func() {
 		Expect(install.ObjectMeta.Finalizers).To(ContainElement(render.WhiskerFinalizer))
 		Expect(install.ObjectMeta.Finalizers).To(ContainElement(render.GoldmaneFinalizer))
 
-		By("Verifying that the whisker finalizer is removed in the installation CR")
+		By("Verifying that the whisker and goldmane finalizers are removed from the installation CR")
 		Expect(c.Delete(context.Background(), whiskerCR)).To(BeNil())
 		Expect(c.Delete(context.Background(), goldmaneCR)).To(BeNil())
 		Eventually(func() error {
 			Expect(GetResource(c, install)).To(BeNil())
 			fmt.Println("Finalizers: ", install.ObjectMeta.Finalizers)
 			if containsFinalizer(install.ObjectMeta.Finalizers, render.WhiskerFinalizer) ||
 				containsFinalizer(install.ObjectMeta.Finalizers, render.GoldmaneFinalizer) {
-				return fmt.Errorf("expected finalizers to be removed, but found: %v", install.ObjectMeta.Finalizers)
+				return fmt.Errorf("expected whisker and goldmane finalizers to be removed, but found: %v", install.ObjectMeta.Finalizers)
 			}
 			return nil
 		}, 1*time.Minute, 1*time.Second).Should(BeNil())