Merge pull request #654 from jvanz/issue650

fix: policy server resource deleted before policies

Makefile

@@ -2,11 +2,11 @@
 # Image URL to use all building/pushing image targets
 IMG ?= controller:latest
 # ENVTEST_K8S_VERSION refers to the version of kubebuilder assets to be downloaded by envtest binary.
-ENVTEST_K8S_VERSION = 1.23
+ENVTEST_K8S_VERSION = 1.29
 # K3S_TESTCONTAINER_VERSION refers to the version of k3s testcontainer to be used by envtest to run integration tests.
-K3S_TESTCONTAINER_VERSION = v1.23.2-k3s1
+K3S_TESTCONTAINER_VERSION = v1.29.1-k3s2
 # POLICY_SERVER_VERSION refers to the version of the policy server to be used by integration tests.
-POLICY_SERVER_VERSION = v1.9.0
+POLICY_SERVER_VERSION = v1.10.0
 # Binary directory
 ROOT_DIR:=$(shell dirname $(realpath $(firstword $(MAKEFILE_LIST))))
 BIN_DIR := $(abspath $(ROOT_DIR)/bin)
@@ -115,7 +115,7 @@ unit-tests: manifests generate fmt vet setup-envtest ## Run unit tests.
 
 .PHONY: setup-envtest integration-tests
 integration-tests: manifests generate fmt vet setup-envtest ## Run integration tests.
-	ACK_GINKGO_DEPRECATIONS=2.12.0 K3S_TESTCONTAINER_VERSION="$(K3S_TESTCONTAINER_VERSION)" POLICY_SERVER_VERSION="$(POLICY_SERVER_VERSION)" go test ./controllers/... -ginkgo.v -ginkgo.progress -race -test.v -coverprofile=coverage/integration-tests/coverage-controllers.txt -covermode=atomic
+	ACK_GINKGO_DEPRECATIONS=2.12.0 K3S_TESTCONTAINER_VERSION="$(K3S_TESTCONTAINER_VERSION)" POLICY_SERVER_VERSION="$(POLICY_SERVER_VERSION)" go test -v ./controllers/... -ginkgo.v -ginkgo.progress -race -test.v -coverprofile=coverage/integration-tests/coverage-controllers.txt -covermode=atomic
 
 .PHONY: generate-crds
 generate-crds: $(KUSTOMIZE) manifests kustomize ## generate final crds with kustomize. Normally shipped in Helm charts.

controllers/policyserver_controller.go

@@ -67,7 +67,7 @@ func (r *PolicyServerReconciler) Reconcile(ctx context.Context, req ctrl.Request
 		return ctrl.Result{}, fmt.Errorf("cannot retrieve policy server: %w", err)
 	}
 
-	policies, err := r.Reconciler.GetPolicies(ctx, &policyServer, admission.SkipDeleted)
+	policies, err := r.Reconciler.GetPolicies(ctx, &policyServer)
 	if err != nil {
 		return ctrl.Result{}, errors.Join(errors.New("could not get policies"), err)
 	}
@@ -100,29 +100,45 @@ func (r *PolicyServerReconciler) reconcile(ctx context.Context, policyServer *po
 }
 
 func (r *PolicyServerReconciler) reconcileDeletion(ctx context.Context, policyServer *policiesv1.PolicyServer, policies []policiesv1.Policy) (ctrl.Result, error) {
-	someDeletionFailed := false
+	if len(policies) != 0 {
+		// There are still policies scheduled on the PolicyServer, we have to
+		// wait for them to be completely removed before going further with the cleanup
+		return r.deletePoliciesAndRequeue(ctx, policyServer, policies)
+	}
+
+	if err := r.Reconciler.ReconcileDeletion(ctx, policyServer); err != nil {
+		return ctrl.Result{}, errors.Join(errors.New("could not reconcile policy server deletion"), err)
+	}
+
+	controllerutil.RemoveFinalizer(policyServer, constants.KubewardenFinalizer)
+	if err := r.Update(ctx, policyServer); err != nil {
+		// return if PolicyServer was previously deleted
+		if apierrors.IsConflict(err) {
+			return ctrl.Result{}, nil
+		}
+		return ctrl.Result{}, fmt.Errorf("cannot update policy server: %w", err)
+	}
+
+	return ctrl.Result{}, nil
+}
+
+func (r *PolicyServerReconciler) deletePoliciesAndRequeue(ctx context.Context, policyServer *policiesv1.PolicyServer, policies []policiesv1.Policy) (ctrl.Result, error) {
+	deleteError := make([]error, 0)
 	for _, policy := range policies {
+		if policy.GetDeletionTimestamp() != nil {
+			// the policy is already pending deletion
+			continue
+		}
 		if err := r.Delete(ctx, policy); err != nil && !apierrors.IsNotFound(err) {
-			someDeletionFailed = true
+			deleteError = append(deleteError, err)
 		}
 	}
-	if someDeletionFailed {
+
+	if len(deleteError) != 0 {
+		r.Log.Error(errors.Join(deleteError...), "could not remove all policies bound to policy server", "policy-server", policyServer.Name)
 		return ctrl.Result{}, fmt.Errorf("could not remove all policies bound to policy server %s", policyServer.Name)
 	}
-	if len(policies) == 0 {
-		if err := r.Reconciler.ReconcileDeletion(ctx, policyServer); err != nil {
-			return ctrl.Result{}, errors.Join(errors.New("could not reconcile policy server deletion"), err)
-		}
-		controllerutil.RemoveFinalizer(policyServer, constants.KubewardenFinalizer)
-		if err := r.Update(ctx, policyServer); err != nil {
-			// return if PolicyServer was previously deleted
-			if apierrors.IsConflict(err) {
-				return ctrl.Result{}, nil
-			}
-			return ctrl.Result{}, fmt.Errorf("cannot update policy server: %w", err)
-		}
-		return ctrl.Result{}, nil
-	}
+
 	return ctrl.Result{Requeue: true}, nil
 }
 

controllers/policyserver_controller_test.go

@@ -21,6 +21,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"
@@ -33,23 +34,45 @@ var _ = Describe("PolicyServer controller", func() {
 		Expect(
 			k8sClient.Create(ctx, policyServerFactory(policyServerName)),
 		).To(haveSucceededOrAlreadyExisted())
+		// Wait for the Service associated with the PolicyServer to be created
+		Eventually(func(g Gomega) error {
+			_, err := getTestPolicyServerService(policyServerName)
+			return err
+		}, timeout, pollInterval).Should(Succeed())
 	})
 
-	Context("it has no assigned policies", func() {
+	Context("with no assigned policies", func() {
 		It("should get its finalizer removed", func() {
 			By("deleting the policy server")
 			Expect(
 				k8sClient.Delete(ctx, policyServerFactory(policyServerName)),
 			).To(Succeed())
 
-			policyServer, err := getTestPolicyServer(policyServerName)
+			Eventually(func(g Gomega) (*policiesv1.PolicyServer, error) {
+				return getTestPolicyServer(policyServerName)
+			}, timeout, pollInterval).ShouldNot(
+				HaveField("Finalizers", ContainElement(constants.KubewardenFinalizer)),
+			)
+		})
 
+		AfterEach(func() {
+			// It's necessary remove the test finalizer to make the
+			// BeforeEach work as extected. Otherwise, the policy service
+			// creation will not work as expected
+			policyServer, err := getTestPolicyServer(policyServerName)
+			Expect(err).Should(Succeed())
+			controllerutil.RemoveFinalizer(policyServer, IntegrationTestsFinalizer)
+			err = reconciler.Client.Update(ctx, policyServer)
 			Expect(err).ToNot(HaveOccurred())
-			Expect(policyServer.Finalizers).ToNot(ContainElement(constants.KubewardenFinalizer))
+			Eventually(func(g Gomega) error {
+				_, err := getTestPolicyServer(policyServerName)
+				return err
+			}, timeout, pollInterval).ShouldNot(Succeed())
 		})
+
 	})
 
-	Context("it has assigned policies", func() {
+	Context("it has assigned policies", Serial, func() {
 		policyName := newName("policy")
 
 		It("should delete assigned policies", func() {
@@ -58,6 +81,12 @@ var _ = Describe("PolicyServer controller", func() {
 				k8sClient.Create(ctx, clusterAdmissionPolicyFactory(policyName, policyServerName, false)),
 			).To(haveSucceededOrAlreadyExisted())
 
+			Expect(
+				getTestPolicyServerService(policyServerName),
+			).To(
+				HaveField("DeletionTimestamp", BeNil()),
+			)
+
 			By("deleting the policy server")
 			Expect(
 				k8sClient.Delete(ctx, policyServerFactory(policyServerName)),
@@ -70,7 +99,44 @@ var _ = Describe("PolicyServer controller", func() {
 			)
 		})
 
+		It("should not delete its managed resources until all the scheduled policies are gone", func() {
+			By("having still policies pending deletion")
+			Expect(
+				getTestClusterAdmissionPolicy(policyName),
+			).To(
+				And(
+					HaveField("DeletionTimestamp", Not(BeNil())),
+					HaveField("Finalizers", Not(ContainElement(constants.KubewardenFinalizer))),
+					HaveField("Finalizers", ContainElement(IntegrationTestsFinalizer)),
+				),
+			)
+
+			Eventually(func(g Gomega) error {
+				_, err := getTestPolicyServerService(policyServerName)
+				return err
+			}).Should(Succeed())
+		})
+
 		It(fmt.Sprintf("should get its %q finalizer removed", constants.KubewardenFinalizer), func() {
+			By("not having policies assigned")
+			policy, err := getTestClusterAdmissionPolicy(policyName)
+			Expect(err).ToNot(HaveOccurred())
+
+			controllerutil.RemoveFinalizer(policy, IntegrationTestsFinalizer)
+			err = reconciler.Client.Update(ctx, policy)
+			Expect(err).ToNot(HaveOccurred())
+
+			// wait for the reconciliation loop of the ClusterAdmissionPolicy to remove the resource
+			Eventually(func(g Gomega) error {
+				_, err := getTestClusterAdmissionPolicy(policyName)
+				return err
+			}, timeout, pollInterval).ShouldNot(Succeed())
+
+			Eventually(func(g Gomega) error {
+				_, err := getTestPolicyServerService(policyServerName)
+				return err
+			}, timeout, pollInterval).ShouldNot(Succeed())
+
 			Eventually(func(g Gomega) (*policiesv1.PolicyServer, error) {
 				return getTestPolicyServer(policyServerName)
 			}, timeout, pollInterval).ShouldNot(

controllers/utils_test.go

@@ -32,12 +32,14 @@ import (
 	admissionregistrationv1 "k8s.io/api/admissionregistration/v1"
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
 const (
-	timeout      = 120 * time.Second
-	pollInterval = 250 * time.Millisecond
+	timeout                   = 120 * time.Second
+	pollInterval              = 250 * time.Millisecond
+	IntegrationTestsFinalizer = "integration-tests-safety-net-finalizer"
 )
 
 var (
@@ -77,11 +79,15 @@ func policyServerVersion() string {
 func policyServerFactory(name string) *policiesv1.PolicyServer {
 	policyServer := templatePolicyServer.DeepCopy()
 	policyServer.Name = name
-
-	// By adding this finalizer automatically, we ensure that when
-	// testing removal of finalizers on deleted objects, that they will
-	// exist at all times
-	policyServer.Finalizers = []string{"integration-tests-safety-net-finalizer"}
+	policyServer.Finalizers = []string{
+		// On a real cluster the Kubewarden finalizer is added by our mutating
+		// webhook. This is not running now, hence we have to manually add the finalizer
+		constants.KubewardenFinalizer,
+		// By adding this finalizer automatically, we ensure that when
+		// testing removal of finalizers on deleted objects, that they will
+		// exist at all times
+		IntegrationTestsFinalizer,
+	}
 	return policyServer
 }
 
@@ -91,10 +97,15 @@ func admissionPolicyFactory(name, policyNamespace, policyServerName string, muta
 	admissionPolicy.Namespace = policyNamespace
 	admissionPolicy.Spec.PolicyServer = policyServerName
 	admissionPolicy.Spec.PolicySpec.Mutating = mutating
-	// By adding this finalizer automatically, we ensure that when
-	// testing removal of finalizers on deleted objects, that they will
-	// exist at all times
-	admissionPolicy.Finalizers = []string{"integration-tests-safety-net-finalizer"}
+	admissionPolicy.Finalizers = []string{
+		// On a real cluster the Kubewarden finalizer is added by our mutating
+		// webhook. This is not running now, hence we have to manually add the finalizer
+		constants.KubewardenFinalizer,
+		// By adding this finalizer automatically, we ensure that when
+		// testing removal of finalizers on deleted objects, that they will
+		// exist at all times
+		IntegrationTestsFinalizer,
+	}
 	return admissionPolicy
 }
 
@@ -103,10 +114,15 @@ func clusterAdmissionPolicyFactory(name, policyServerName string, mutating bool)
 	clusterAdmissionPolicy.Name = name
 	clusterAdmissionPolicy.Spec.PolicyServer = policyServerName
 	clusterAdmissionPolicy.Spec.PolicySpec.Mutating = mutating
-	// By adding this finalizer automatically, we ensure that when
-	// testing removal of finalizers on deleted objects, that they will
-	// exist at all times
-	clusterAdmissionPolicy.Finalizers = []string{"integration-tests-safety-net-finalizer"}
+	clusterAdmissionPolicy.Finalizers = []string{
+		// On a real cluster the Kubewarden finalizer is added by our mutating
+		// webhook. This is not running now, hence we have to manually add the finalizer
+		constants.KubewardenFinalizer,
+		// By adding this finalizer automatically, we ensure that when
+		// testing removal of finalizers on deleted objects, that they will
+		// exist at all times
+		IntegrationTestsFinalizer,
+	}
 	return clusterAdmissionPolicy
 }
 
@@ -134,6 +150,21 @@ func getTestPolicyServer(name string) (*policiesv1.PolicyServer, error) {
 	return &policyServer, nil
 }
 
+func getTestPolicyServerService(policyServerName string) (*corev1.Service, error) {
+	policyServer := policiesv1.PolicyServer{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: policyServerName,
+		},
+	}
+	serviceName := policyServer.NameWithPrefix()
+
+	service := corev1.Service{}
+	if err := reconciler.APIReader.Get(ctx, client.ObjectKey{Name: serviceName, Namespace: DeploymentsNamespace}, &service); err != nil {
+		return nil, errors.Join(errors.New("could not find Service owned by PolicyServer"), err)
+	}
+	return &service, nil
+}
+
 func getTestValidatingWebhookConfiguration(name string) (*admissionregistrationv1.ValidatingWebhookConfiguration, error) {
 	validatingWebhookConfiguration := admissionregistrationv1.ValidatingWebhookConfiguration{}
 	if err := reconciler.APIReader.Get(ctx, client.ObjectKey{Name: name}, &validatingWebhookConfiguration); err != nil {

internal/pkg/admission/reconciler.go

@@ -248,16 +248,9 @@ func (r *Reconciler) Reconcile(
 	return nil
 }
 
-type GetPoliciesBehavior int
-
-const (
-	SkipDeleted GetPoliciesBehavior = iota
-	IncludeDeleted
-)
-
 // GetPolicies returns all admission policies and cluster admission
 // policies bound to the given policyServer
-func (r *Reconciler) GetPolicies(ctx context.Context, policyServer *policiesv1.PolicyServer, getPoliciesBehavior GetPoliciesBehavior) ([]policiesv1.Policy, error) {
+func (r *Reconciler) GetPolicies(ctx context.Context, policyServer *policiesv1.PolicyServer) ([]policiesv1.Policy, error) {
 	var clusterAdmissionPolicies policiesv1.ClusterAdmissionPolicyList
 	err := r.Client.List(ctx, &clusterAdmissionPolicies, client.MatchingFields{constants.PolicyServerIndexKey: policyServer.Name})
 	if err != nil && apierrors.IsNotFound(err) {
@@ -274,16 +267,10 @@ func (r *Reconciler) GetPolicies(ctx context.Context, policyServer *policiesv1.P
 	policies := make([]policiesv1.Policy, 0)
 	for _, clusterAdmissionPolicy := range clusterAdmissionPolicies.Items {
 		clusterAdmissionPolicy := clusterAdmissionPolicy
-		if getPoliciesBehavior == SkipDeleted && clusterAdmissionPolicy.DeletionTimestamp != nil {
-			continue
-		}
 		policies = append(policies, &clusterAdmissionPolicy)
 	}
 	for _, admissionPolicy := range admissionPolicies.Items {
 		admissionPolicy := admissionPolicy
-		if getPoliciesBehavior == SkipDeleted && admissionPolicy.DeletionTimestamp != nil {
-			continue
-		}
 		policies = append(policies, &admissionPolicy)
 	}
 

internal/pkg/admission/reconciler_test.go

@@ -78,7 +78,7 @@ func TestGetPolicies(t *testing.T) {
 			reconciler := newReconciler(ttest.policies, false)
 			policies, err := reconciler.GetPolicies(context.Background(), &policiesv1.PolicyServer{
 				ObjectMeta: metav1.ObjectMeta{Name: policyServer},
-			}, IncludeDeleted)
+			})
 			if err != nil {
 				t.Errorf("received unexpected error %s", err.Error())
 			}