Merge pull request #759 from jvanz/domain-finalizers

fix: use domain qualified finalizers.

controllers/admissionpolicy_controller_test.go

@@ -22,6 +22,7 @@ import (
 
 	. "github.com/onsi/ginkgo/v2" //nolint:revive
 	. "github.com/onsi/gomega"    //nolint:revive
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
 	"github.com/kubewarden/kubewarden-controller/internal/pkg/constants"
 	policiesv1 "github.com/kubewarden/kubewarden-controller/pkg/apis/policies/v1"
@@ -318,4 +319,16 @@ var _ = Describe("AdmissionPolicy controller", func() {
 			)
 		})
 	})
+
+	It("should adds policy's missing finalizer", func() {
+		policyName := newName("missing-finalizer-policy")
+		policy := admissionPolicyFactory(policyName, policyNamespace, "", false)
+		controllerutil.RemoveFinalizer(policy, constants.KubewardenFinalizer)
+		Expect(k8sClient.Create(ctx, policy)).To(haveSucceededOrAlreadyExisted())
+		Eventually(func() (*policiesv1.AdmissionPolicy, error) {
+			return getTestAdmissionPolicy(policyNamespace, policyName)
+		}, timeout, pollInterval).Should(
+			HaveField("Finalizers", ContainElement(constants.KubewardenFinalizer)),
+		)
+	})
 })

controllers/clusteradmissionpolicy_controller_test.go

@@ -21,6 +21,7 @@ import (
 	. "github.com/onsi/ginkgo/v2" //nolint:revive
 	. "github.com/onsi/gomega"    //nolint:revive
 	admissionregistrationv1 "k8s.io/api/admissionregistration/v1"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
 	"github.com/kubewarden/kubewarden-controller/internal/pkg/constants"
 	policiesv1 "github.com/kubewarden/kubewarden-controller/pkg/apis/policies/v1"
@@ -220,6 +221,18 @@ var _ = Describe("ClusterAdmissionPolicy controller", func() {
 		)
 	})
 
+	It("should adds policy's missing finalizer", func() {
+		policyName := newName("missing-finalizer-policy")
+		policy := clusterAdmissionPolicyFactory(policyName, "", false)
+		controllerutil.RemoveFinalizer(policy, constants.KubewardenFinalizer)
+		Expect(k8sClient.Create(ctx, policy)).To(haveSucceededOrAlreadyExisted())
+		Eventually(func() (*policiesv1.ClusterAdmissionPolicy, error) {
+			return getTestClusterAdmissionPolicy(policyName)
+		}, timeout, pollInterval).Should(
+			HaveField("Finalizers", ContainElement(constants.KubewardenFinalizer)),
+		)
+	})
+
 	When("creating a ClusterAdmissionPolicy with a PolicyServer assigned but not running yet", func() {
 		var policyName string
 

controllers/policy_utils.go

@@ -72,6 +72,13 @@ func startReconciling(ctx context.Context, client client.Client, reconciler admi
 		return reconcilePolicyDeletion(ctx, client, policy)
 	}
 
+	if !controllerutil.ContainsFinalizer(policy, constants.KubewardenFinalizer) {
+		controllerutil.AddFinalizer(policy, constants.KubewardenFinalizer)
+		if err := client.Update(ctx, policy); err != nil {
+			return ctrl.Result{}, fmt.Errorf("cannot update policy finalizer: %w", err)
+		}
+	}
+
 	reconcileResult, reconcileErr := reconcilePolicy(ctx, client, reconciler, policy)
 
 	_ = setPolicyStatus(ctx, reconciler.DeploymentsNamespace, reconciler.APIReader, policy)
@@ -193,6 +200,11 @@ func reconcilePolicyDeletion(ctx context.Context, client client.Client, policy p
 			return ctrl.Result{}, err
 		}
 	}
+	// Remove the old finalizer used to ensure that the policy server created
+	// before this controller version is delete as well. As the upgrade path
+	// supported by the Kubewarden project does not allow jumping versions, we
+	// can safely remove this line of code after a few releases.
+	controllerutil.RemoveFinalizer(policy, constants.KubewardenFinalizerPre114)
 	controllerutil.RemoveFinalizer(policy, constants.KubewardenFinalizer)
 	if err := client.Update(ctx, policy); err != nil {
 		return ctrl.Result{}, fmt.Errorf("cannot update admission policy: %w", err)

controllers/policyserver_controller.go

@@ -75,6 +75,13 @@ func (r *PolicyServerReconciler) Reconcile(ctx context.Context, req ctrl.Request
 		return r.reconcileDeletion(ctx, &policyServer, policies)
 	}
 
+	if !controllerutil.ContainsFinalizer(&policyServer, constants.KubewardenFinalizer) {
+		controllerutil.AddFinalizer(&policyServer, constants.KubewardenFinalizer)
+		if err := r.Client.Update(ctx, &policyServer); err != nil {
+			return ctrl.Result{}, fmt.Errorf("cannot update policy server finalizer: %w", err)
+		}
+	}
+
 	reconcileResult, reconcileErr := r.reconcile(ctx, &policyServer, policies)
 
 	if err := r.Client.Status().Update(ctx, &policyServer); err != nil {
@@ -105,6 +112,11 @@ func (r *PolicyServerReconciler) reconcileDeletion(ctx context.Context, policySe
 		return r.deletePoliciesAndRequeue(ctx, policyServer, policies)
 	}
 
+	// Remove the old finalizer used to ensure that the policy server created
+	// before this controller version is delete as well. As the upgrade path
+	// supported by the Kubewarden project does not allow jumping versions, we
+	// can safely remove this line of code after a few releases.
+	controllerutil.RemoveFinalizer(policyServer, constants.KubewardenFinalizerPre114)
 	controllerutil.RemoveFinalizer(policyServer, constants.KubewardenFinalizer)
 	if err := r.Update(ctx, policyServer); err != nil {
 		// return if PolicyServer was previously deleted

controllers/policyserver_controller_test.go

@@ -62,6 +62,38 @@ var _ = Describe("PolicyServer controller", func() {
 				)
 			})
 
+			It("should get its old not domain-qualified finalizer removed", func() {
+				Eventually(func() error {
+					policyServer, err := getTestPolicyServer(policyServerName)
+					if err != nil {
+						return err
+					}
+					controllerutil.AddFinalizer(policyServer, constants.KubewardenFinalizerPre114)
+					return k8sClient.Update(ctx, policyServer)
+				}, timeout, pollInterval).Should(Succeed())
+
+				Eventually(func() error {
+					policyServer, err := getTestPolicyServer(policyServerName)
+					if err != nil {
+						return err
+					}
+					if controllerutil.ContainsFinalizer(policyServer, constants.KubewardenFinalizerPre114) {
+						return nil
+					}
+					return errors.New("finalizer not found")
+				}, timeout, pollInterval).Should(Succeed())
+
+				Expect(
+					k8sClient.Delete(ctx, policyServerFactory(policyServerName)),
+				).To(Succeed())
+
+				Eventually(func() (*policiesv1.PolicyServer, error) {
+					return getTestPolicyServer(policyServerName)
+				}, timeout, pollInterval).ShouldNot(
+					HaveField("Finalizers", ContainElement(constants.KubewardenFinalizerPre114)),
+				)
+			})
+
 			It("policy server resources should be gone after it being deleted", func() {
 				// It's necessary remove the test finalizer to make the
 				// Kubernetes garbage collector to remove the resources
@@ -137,6 +169,41 @@ var _ = Describe("PolicyServer controller", func() {
 				)
 			})
 
+			It("should get its old not domain-qualidied finalizer removed from policies", func() {
+				Eventually(func() error {
+					policy, err := getTestClusterAdmissionPolicy(policyName)
+					if err != nil {
+						return err
+					}
+					controllerutil.AddFinalizer(policy, constants.KubewardenFinalizerPre114)
+					return k8sClient.Update(ctx, policy)
+				}, timeout, pollInterval).Should(Succeed())
+				Eventually(func() error {
+					policy, err := getTestClusterAdmissionPolicy(policyName)
+					if err != nil {
+						return err
+					}
+					if controllerutil.ContainsFinalizer(policy, constants.KubewardenFinalizerPre114) {
+						return nil
+					}
+					return errors.New("old finalizer not found")
+				}, timeout, pollInterval).Should(Succeed())
+
+				Expect(
+					k8sClient.Delete(ctx, policyServerFactory(policyServerName)),
+				).To(Succeed())
+
+				Eventually(func() (*policiesv1.ClusterAdmissionPolicy, error) {
+					return getTestClusterAdmissionPolicy(policyName)
+				}, timeout, pollInterval).Should(And(
+					HaveField("DeletionTimestamp", Not(BeNil())),
+					HaveField("Finalizers", Not(ContainElement(constants.KubewardenFinalizer))),
+					HaveField("Finalizers", Not(ContainElement(constants.KubewardenFinalizerPre114))),
+					HaveField("Finalizers", ContainElement(IntegrationTestsFinalizer)),
+				))
+
+			})
+
 			It("should not delete its managed resources until all the scheduled policies are gone", func() {
 				Expect(
 					k8sClient.Delete(ctx, policyServerFactory(policyServerName)),
@@ -626,6 +693,17 @@ var _ = Describe("PolicyServer controller", func() {
 
 		})
 
+		It("should add missing finalizer", func() {
+			policyServer := policyServerFactory(policyServerName)
+			controllerutil.RemoveFinalizer(policyServer, constants.KubewardenFinalizer)
+			createPolicyServerAndWaitForItsService(policyServer)
+			Eventually(func() (*policiesv1.PolicyServer, error) {
+				return getTestPolicyServer(policyServerName)
+			}, timeout, pollInterval).Should(
+				HaveField("Finalizers", ContainElement(constants.KubewardenFinalizer)),
+			)
+		})
+
 	})
 
 	When("updating the PolicyServer", func() {

internal/pkg/admission/testing_common.go

@@ -3,6 +3,7 @@ package admission
 import (
 	"time"
 
+	"github.com/kubewarden/kubewarden-controller/internal/pkg/constants"
 	policiesv1 "github.com/kubewarden/kubewarden-controller/pkg/apis/policies/v1"
 	admissionregistrationv1 "k8s.io/api/admissionregistration/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -14,7 +15,7 @@ import (
 func createReconciler() (Reconciler, policiesv1.ClusterAdmissionPolicy, policiesv1.ClusterAdmissionPolicy, policiesv1.ClusterAdmissionPolicy) {
 	admissionPolicyName := "admissionPolicy"
 	validationPolicy := policiesv1.ClusterAdmissionPolicy{
-		ObjectMeta: metav1.ObjectMeta{DeletionTimestamp: &metav1.Time{Time: time.Now()}, Name: admissionPolicyName, Finalizers: []string{"kubewarden"}},
+		ObjectMeta: metav1.ObjectMeta{DeletionTimestamp: &metav1.Time{Time: time.Now()}, Name: admissionPolicyName, Finalizers: []string{constants.KubewardenFinalizer}},
 		Spec: policiesv1.ClusterAdmissionPolicySpec{
 			PolicySpec: policiesv1.PolicySpec{
 				Mutating: false,
@@ -29,7 +30,7 @@ func createReconciler() (Reconciler, policiesv1.ClusterAdmissionPolicy, policies
 	}
 	mutatingPolicyName := "mutatingPolicy"
 	mutatingPolicy := policiesv1.ClusterAdmissionPolicy{
-		ObjectMeta: metav1.ObjectMeta{DeletionTimestamp: &metav1.Time{Time: time.Now()}, Name: mutatingPolicyName, Finalizers: []string{"kubewarden"}},
+		ObjectMeta: metav1.ObjectMeta{DeletionTimestamp: &metav1.Time{Time: time.Now()}, Name: mutatingPolicyName, Finalizers: []string{constants.KubewardenFinalizer}},
 		Spec: policiesv1.ClusterAdmissionPolicySpec{
 			PolicySpec: policiesv1.PolicySpec{
 				Mutating: true,
@@ -45,7 +46,7 @@ func createReconciler() (Reconciler, policiesv1.ClusterAdmissionPolicy, policies
 
 	contextAwarePolicyName := "contextAwarePolicy"
 	contextAwarePolicy := policiesv1.ClusterAdmissionPolicy{
-		ObjectMeta: metav1.ObjectMeta{DeletionTimestamp: &metav1.Time{Time: time.Now()}, Name: contextAwarePolicyName, Finalizers: []string{"kubewarden"}},
+		ObjectMeta: metav1.ObjectMeta{DeletionTimestamp: &metav1.Time{Time: time.Now()}, Name: contextAwarePolicyName, Finalizers: []string{constants.KubewardenFinalizer}},
 		Spec: policiesv1.ClusterAdmissionPolicySpec{
 			PolicySpec: policiesv1.PolicySpec{
 				Mutating: false,

internal/pkg/constants/constants.go

@@ -42,7 +42,8 @@ const (
 	PolicyServerIndexKey = ".spec.policyServer"
 
 	// Finalizers
-	KubewardenFinalizer = "kubewarden"
+	KubewardenFinalizerPre114 = "kubewarden"
+	KubewardenFinalizer       = "kubewarden.io/finalizer"
 
 	// Kubernetes
 	KubernetesRevisionAnnotation = "deployment.kubernetes.io/revision"