From 0dbdf1723a859cadd993aa61bd3b422646e1404a Mon Sep 17 00:00:00 2001 From: Flavio Castelli Date: Wed, 28 Feb 2024 18:41:35 +0100 Subject: [PATCH 1/3] chore(deps): integration tests Upgrade version of k3s used inside of the integration tests Signed-off-by: Flavio Castelli --- Makefile | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Makefile b/Makefile index 2e9b9e29..4ce8a589 100644 --- a/Makefile +++ b/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. From 905be8c12dcf7dfe9bda8b959bf3583d4dab540d Mon Sep 17 00:00:00 2001 From: Flavio Castelli Date: Wed, 28 Feb 2024 18:43:12 +0100 Subject: [PATCH 2/3] test: handle finalizers inside of integration tests Ensure the `kubewarden` finalizer is always added to the relevant resources when running the integration tests Signed-off-by: Flavio Castelli --- controllers/utils_test.go | 45 ++++++++++++++++++++++++++------------- 1 file changed, 30 insertions(+), 15 deletions(-) diff --git a/controllers/utils_test.go b/controllers/utils_test.go index 544ded76..4b5d1e40 100644 --- a/controllers/utils_test.go +++ b/controllers/utils_test.go @@ -36,8 +36,9 @@ import ( ) 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 +78,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 +96,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 +113,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 } From d2637e1bcc924ded645ebe37b92b74a26fd088af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Guilherme=20Vanz?= Date: Tue, 27 Feb 2024 22:24:34 -0300 Subject: [PATCH 3/3] fix: policy server resource deleted before policies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There is an issue in the policy server controller which is causing inconsistencies when the policy server is deleted. It's possible, due the concurrent nature of events and controllers loops, where the policies ended in a inconsistent state. Where the policy and its webhooks are register in the cluster. But the policy server which should run it is gone. Causing an issue when the control plane tries to contact the policy server. This is caused because when the policy server controller deletes all the policy server policies, it filters out policies which have the deletion timestamp field. Once no policies are found, in other words, when no policies without deletion timestamp are found, the policy server resources are deleted. However, the deletion timestamp does not ensure that the resource is not register in the cluster. It tells that the resource is marked to be deleted by the control plane. Which means that we can reach a state where the policy server is deleted before the policies. Causing issue when the control plane tries to send request to the webhooks.This commit fixes that by removing the filter used to get the policies in the deletion reconciliation loop. Signed-off-by: José Guilherme Vanz --- controllers/policyserver_controller.go | 52 ++++++++++----- controllers/policyserver_controller_test.go | 74 +++++++++++++++++++-- controllers/utils_test.go | 16 +++++ internal/pkg/admission/reconciler.go | 15 +---- internal/pkg/admission/reconciler_test.go | 2 +- 5 files changed, 122 insertions(+), 37 deletions(-) diff --git a/controllers/policyserver_controller.go b/controllers/policyserver_controller.go index a0ed8949..1a76783b 100644 --- a/controllers/policyserver_controller.go +++ b/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 } diff --git a/controllers/policyserver_controller_test.go b/controllers/policyserver_controller_test.go index debb3a87..72db663c 100644 --- a/controllers/policyserver_controller_test.go +++ b/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( diff --git a/controllers/utils_test.go b/controllers/utils_test.go index 4b5d1e40..53bb0a88 100644 --- a/controllers/utils_test.go +++ b/controllers/utils_test.go @@ -32,6 +32,7 @@ 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" ) @@ -149,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 { diff --git a/internal/pkg/admission/reconciler.go b/internal/pkg/admission/reconciler.go index af163433..8f75f82a 100644 --- a/internal/pkg/admission/reconciler.go +++ b/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) } diff --git a/internal/pkg/admission/reconciler_test.go b/internal/pkg/admission/reconciler_test.go index 753d8e46..0a69e2a2 100644 --- a/internal/pkg/admission/reconciler_test.go +++ b/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()) }