From a2b76bd3740347bcb8800f28e28b622bdfcbce2e Mon Sep 17 00:00:00 2001 From: David Ortiz Date: Mon, 11 Jan 2021 17:56:12 +0100 Subject: [PATCH 1/2] controllers/limitador: use owner refs So the deployment and the service are deleted automatically when a Limitador object is deleted. --- controllers/limitador_controller.go | 51 +++-------------- controllers/limitador_controller_test.go | 71 ++++++++++++++---------- pkg/limitador/k8s_objects.go | 40 ++++++++----- 3 files changed, 77 insertions(+), 85 deletions(-) diff --git a/controllers/limitador_controller.go b/controllers/limitador_controller.go index b8c98129..f7209ac4 100644 --- a/controllers/limitador_controller.go +++ b/controllers/limitador_controller.go @@ -22,9 +22,7 @@ import ( "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" - "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -50,16 +48,8 @@ func (r *LimitadorReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { limitadorObj := limitadorv1alpha1.Limitador{} if err := r.Get(context.TODO(), req.NamespacedName, &limitadorObj); err != nil { if errors.IsNotFound(err) { - if err = r.ensureLimitadorDeploymentIsDeleted(req.NamespacedName); err != nil { - reqLogger.Error(err, "Failed to delete Limitador deployment.") - return ctrl.Result{}, err - } - - if err = r.ensureLimitadorServiceIsDeleted(); err != nil { - reqLogger.Error(err, "Failed to delete Limitador service.") - return ctrl.Result{}, err - } - + // The deployment and the service should be deleted automatically + // because they have an owner ref to Limitador return ctrl.Result{}, nil } else { reqLogger.Error(err, "Failed to get Limitador object.") @@ -67,7 +57,11 @@ func (r *LimitadorReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { } } - if err := r.ensureLimitadorServiceExists(); err != nil { + if limitadorObj.GetDeletionTimestamp() != nil { // Marked to be deleted + return ctrl.Result{}, nil + } + + if err := r.ensureLimitadorServiceExists(&limitadorObj); err != nil { return ctrl.Result{}, err } @@ -121,25 +115,8 @@ func (r *LimitadorReconciler) reconcileDeployment(desiredDeployment *v1.Deployme } } -func (r *LimitadorReconciler) ensureLimitadorDeploymentIsDeleted(name types.NamespacedName) error { - currentLimitadorDeployment := v1.Deployment{ - ObjectMeta: metav1.ObjectMeta{ - Name: name.Name, - Namespace: name.Namespace, - }, - } - - err := r.Delete(context.TODO(), ¤tLimitadorDeployment) - - if err != nil && !errors.IsNotFound(err) { - return err - } - - return nil -} - -func (r *LimitadorReconciler) ensureLimitadorServiceExists() error { - limitadorService := limitador.LimitadorService() +func (r *LimitadorReconciler) ensureLimitadorServiceExists(limitadorObj *limitadorv1alpha1.Limitador) error { + limitadorService := limitador.LimitadorService(limitadorObj) limitadorServiceKey, _ := client.ObjectKeyFromObject(limitadorService) err := r.Get(context.TODO(), limitadorServiceKey, limitadorService) @@ -153,13 +130,3 @@ func (r *LimitadorReconciler) ensureLimitadorServiceExists() error { return nil } - -func (r *LimitadorReconciler) ensureLimitadorServiceIsDeleted() error { - err := r.Delete(context.TODO(), limitador.LimitadorService()) - - if err != nil && !errors.IsNotFound(err) { - return err - } - - return nil -} diff --git a/controllers/limitador_controller_test.go b/controllers/limitador_controller_test.go index e67b5b1f..e1a42596 100644 --- a/controllers/limitador_controller_test.go +++ b/controllers/limitador_controller_test.go @@ -10,12 +10,13 @@ import ( "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/uuid" + "sigs.k8s.io/controller-runtime/pkg/client" "time" ) var _ = Describe("Limitador controller", func() { const ( - LimitadorName = "limitador-test" LimitadorNamespace = "default" LimitadorReplicas = 2 LimitadorImage = "quay.io/3scale/limitador" @@ -27,27 +28,37 @@ var _ = Describe("Limitador controller", func() { replicas := LimitadorReplicas version := LimitadorVersion - limitador := limitadorv1alpha1.Limitador{ - TypeMeta: metav1.TypeMeta{ - Kind: "Limitador", - APIVersion: "limitador.3scale.net/v1alpha1", - }, - ObjectMeta: metav1.ObjectMeta{ - Name: LimitadorName, - Namespace: LimitadorNamespace, - }, - Spec: limitadorv1alpha1.LimitadorSpec{ - Replicas: &replicas, - Version: &version, - }, + newLimitador := func() limitadorv1alpha1.Limitador { + // The name can't start with a number. + name := "a" + string(uuid.NewUUID()) + + return limitadorv1alpha1.Limitador{ + TypeMeta: metav1.TypeMeta{ + Kind: "Limitador", + APIVersion: "limitador.3scale.net/v1alpha1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: LimitadorNamespace, + }, + Spec: limitadorv1alpha1.LimitadorSpec{ + Replicas: &replicas, + Version: &version, + }, + } } + deletePropagationPolicy := client.PropagationPolicy(metav1.DeletePropagationForeground) + Context("Creating a new Limitador object", func() { + var limitadorObj limitadorv1alpha1.Limitador + BeforeEach(func() { - err := k8sClient.Delete(context.TODO(), limitador.DeepCopy()) + limitadorObj = newLimitador() + err := k8sClient.Delete(context.TODO(), limitadorObj.DeepCopy(), deletePropagationPolicy) Expect(err == nil || errors.IsNotFound(err)) - Expect(k8sClient.Create(context.TODO(), limitador.DeepCopy())).Should(Succeed()) + Expect(k8sClient.Create(context.TODO(), limitadorObj.DeepCopy())).Should(Succeed()) }) It("Should create a new deployment with the right number of replicas and version", func() { @@ -57,7 +68,7 @@ var _ = Describe("Limitador controller", func() { context.TODO(), types.NamespacedName{ Namespace: LimitadorNamespace, - Name: LimitadorName, + Name: limitadorObj.Name, }, &createdLimitadorDeployment) @@ -78,7 +89,7 @@ var _ = Describe("Limitador controller", func() { err := k8sClient.Get( context.TODO(), types.NamespacedName{ - Namespace: "default", // Hardcoded for now + Namespace: LimitadorNamespace, Name: "limitador", // Hardcoded for now }, &createdLimitadorService) @@ -89,11 +100,12 @@ var _ = Describe("Limitador controller", func() { }) Context("Deleting a Limitador object", func() { - BeforeEach(func() { - err := k8sClient.Create(context.TODO(), limitador.DeepCopy()) - Expect(err == nil || errors.IsAlreadyExists(err)) + var limitadorObj limitadorv1alpha1.Limitador - Expect(k8sClient.Delete(context.TODO(), limitador.DeepCopy())).Should(Succeed()) + BeforeEach(func() { + limitadorObj = newLimitador() + Expect(k8sClient.Create(context.TODO(), limitadorObj.DeepCopy())).Should(Succeed()) + Expect(k8sClient.Delete(context.TODO(), limitadorObj.DeepCopy(), deletePropagationPolicy)).Should(Succeed()) }) It("Should delete the limitador deployment", func() { @@ -103,7 +115,7 @@ var _ = Describe("Limitador controller", func() { context.TODO(), types.NamespacedName{ Namespace: LimitadorNamespace, - Name: LimitadorName, + Name: limitadorObj.Name, }, &createdLimitadorDeployment) @@ -117,7 +129,7 @@ var _ = Describe("Limitador controller", func() { err := k8sClient.Get( context.TODO(), types.NamespacedName{ - Namespace: "default", // Hardcoded for now + Namespace: LimitadorNamespace, Name: "limitador", // Hardcoded for now }, &createdLimitadorService) @@ -128,11 +140,14 @@ var _ = Describe("Limitador controller", func() { }) Context("Updating a limitador object", func() { + var limitadorObj limitadorv1alpha1.Limitador + BeforeEach(func() { - err := k8sClient.Delete(context.TODO(), limitador.DeepCopy()) + limitadorObj = newLimitador() + err := k8sClient.Delete(context.TODO(), limitadorObj.DeepCopy(), deletePropagationPolicy) Expect(err == nil || errors.IsNotFound(err)) - Expect(k8sClient.Create(context.TODO(), limitador.DeepCopy())).Should(Succeed()) + Expect(k8sClient.Create(context.TODO(), limitadorObj.DeepCopy())).Should(Succeed()) }) It("Should modify the limitador deployment", func() { @@ -142,7 +157,7 @@ var _ = Describe("Limitador controller", func() { context.TODO(), types.NamespacedName{ Namespace: LimitadorNamespace, - Name: LimitadorName, + Name: limitadorObj.Name, }, &updatedLimitador) @@ -161,7 +176,7 @@ var _ = Describe("Limitador controller", func() { context.TODO(), types.NamespacedName{ Namespace: LimitadorNamespace, - Name: LimitadorName, + Name: limitadorObj.Name, }, &updatedLimitadorDeployment) diff --git a/pkg/limitador/k8s_objects.go b/pkg/limitador/k8s_objects.go index 8544159d..985a1a9a 100644 --- a/pkg/limitador/k8s_objects.go +++ b/pkg/limitador/k8s_objects.go @@ -9,26 +9,26 @@ import ( ) const ( - DefaultVersion = "latest" - DefaultReplicas = 1 - ServiceName = "limitador" - ServiceNamespace = "default" - Image = "quay.io/3scale/limitador" - StatusEndpoint = "/status" - ServiceHTTPPort = 8080 - ServiceGRPCPort = 8081 + DefaultVersion = "latest" + DefaultReplicas = 1 + ServiceName = "limitador" + Image = "quay.io/3scale/limitador" + StatusEndpoint = "/status" + ServiceHTTPPort = 8080 + ServiceGRPCPort = 8081 ) -func LimitadorService() *v1.Service { +func LimitadorService(limitador *limitadorv1alpha1.Limitador) *v1.Service { return &v1.Service{ TypeMeta: metav1.TypeMeta{ Kind: "Service", APIVersion: "v1", }, ObjectMeta: metav1.ObjectMeta{ - Name: ServiceName, - Namespace: ServiceNamespace, - Labels: labels(), + Name: ServiceName, + Namespace: limitador.ObjectMeta.Namespace, // TODO: revisit later. For now assume same. + Labels: labels(), + OwnerReferences: []metav1.OwnerReference{ownerRefToLimitador(limitador)}, }, Spec: v1.ServiceSpec{ Ports: []v1.ServicePort{ @@ -69,9 +69,10 @@ func LimitadorDeployment(limitador *limitadorv1alpha1.Limitador) *appsv1.Deploym APIVersion: "apps/v1", }, ObjectMeta: metav1.ObjectMeta{ - Name: limitador.ObjectMeta.Name, // TODO: revisit later. For now assume same. - Namespace: limitador.ObjectMeta.Namespace, // TODO: revisit later. For now assume same. - Labels: labels(), + Name: limitador.ObjectMeta.Name, // TODO: revisit later. For now assume same. + Namespace: limitador.ObjectMeta.Namespace, // TODO: revisit later. For now assume same. + Labels: labels(), + OwnerReferences: []metav1.OwnerReference{ownerRefToLimitador(limitador)}, }, Spec: appsv1.DeploymentSpec{ Replicas: &replicas, @@ -145,3 +146,12 @@ func LimitadorDeployment(limitador *limitadorv1alpha1.Limitador) *appsv1.Deploym func labels() map[string]string { return map[string]string{"app": "limitador"} } + +func ownerRefToLimitador(limitador *limitadorv1alpha1.Limitador) metav1.OwnerReference { + return metav1.OwnerReference{ + APIVersion: limitador.APIVersion, + Kind: limitador.Kind, + Name: limitador.Name, + UID: limitador.UID, + } +} From 486880237311a64e04960e6c7a59e9b6b7fdae1a Mon Sep 17 00:00:00 2001 From: Eguzki Astiz Lezaun Date: Mon, 19 Jul 2021 14:13:02 +0200 Subject: [PATCH 2/2] fix tests --- Makefile | 2 +- controllers/limitador_controller.go | 16 +++--- controllers/limitador_controller_test.go | 62 +++++------------------- 3 files changed, 22 insertions(+), 58 deletions(-) diff --git a/Makefile b/Makefile index c3947002..bc8a11ed 100644 --- a/Makefile +++ b/Makefile @@ -33,7 +33,7 @@ ENVTEST_ASSETS_DIR = $(shell pwd)/testbin test: generate fmt vet manifests mkdir -p $(ENVTEST_ASSETS_DIR) test -f $(ENVTEST_ASSETS_DIR)/setup-envtest.sh || curl -sSLo $(ENVTEST_ASSETS_DIR)/setup-envtest.sh https://raw.githubusercontent.com/kubernetes-sigs/controller-runtime/v0.6.3/hack/setup-envtest.sh - source $(ENVTEST_ASSETS_DIR)/setup-envtest.sh; fetch_envtest_tools $(ENVTEST_ASSETS_DIR); setup_envtest_env $(ENVTEST_ASSETS_DIR); go test ./... -coverprofile cover.out + source $(ENVTEST_ASSETS_DIR)/setup-envtest.sh; fetch_envtest_tools $(ENVTEST_ASSETS_DIR); setup_envtest_env $(ENVTEST_ASSETS_DIR); go test ./... -coverprofile cover.out -v # Build manager binary manager: generate fmt vet diff --git a/controllers/limitador_controller.go b/controllers/limitador_controller.go index f7209ac4..425b9525 100644 --- a/controllers/limitador_controller.go +++ b/controllers/limitador_controller.go @@ -45,8 +45,8 @@ func (r *LimitadorReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { reqLogger := r.Log.WithValues("limitador", req.NamespacedName) // Delete Limitador deployment and service if needed - limitadorObj := limitadorv1alpha1.Limitador{} - if err := r.Get(context.TODO(), req.NamespacedName, &limitadorObj); err != nil { + limitadorObj := &limitadorv1alpha1.Limitador{} + if err := r.Get(context.TODO(), req.NamespacedName, limitadorObj); err != nil { if errors.IsNotFound(err) { // The deployment and the service should be deleted automatically // because they have an owner ref to Limitador @@ -58,17 +58,19 @@ func (r *LimitadorReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { } if limitadorObj.GetDeletionTimestamp() != nil { // Marked to be deleted + reqLogger.V(1).Info("marked to be deleted") return ctrl.Result{}, nil } - if err := r.ensureLimitadorServiceExists(&limitadorObj); err != nil { + err := r.ensureLimitadorServiceExists(limitadorObj) + reqLogger.V(1).Info("reconcile service", "error", err) + if err != nil { return ctrl.Result{}, err } - desiredDeployment := limitador.LimitadorDeployment(&limitadorObj) - - if err := r.reconcileDeployment(desiredDeployment); err != nil { - reqLogger.Error(err, "Failed to update Limitador deployment.") + err = r.reconcileDeployment(limitador.LimitadorDeployment(limitadorObj)) + reqLogger.V(1).Info("reconcile deployment", "error", err) + if err != nil { return ctrl.Result{}, err } diff --git a/controllers/limitador_controller_test.go b/controllers/limitador_controller_test.go index e1a42596..835ccf60 100644 --- a/controllers/limitador_controller_test.go +++ b/controllers/limitador_controller_test.go @@ -2,7 +2,6 @@ package controllers import ( "context" - limitadorv1alpha1 "github.com/3scale/limitador-operator/api/v1alpha1" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" appsv1 "k8s.io/api/apps/v1" @@ -13,6 +12,9 @@ import ( "k8s.io/apimachinery/pkg/util/uuid" "sigs.k8s.io/controller-runtime/pkg/client" "time" + + limitadorv1alpha1 "github.com/3scale/limitador-operator/api/v1alpha1" + "github.com/3scale/limitador-operator/pkg/limitador" ) var _ = Describe("Limitador controller", func() { @@ -28,11 +30,11 @@ var _ = Describe("Limitador controller", func() { replicas := LimitadorReplicas version := LimitadorVersion - newLimitador := func() limitadorv1alpha1.Limitador { + newLimitador := func() *limitadorv1alpha1.Limitador { // The name can't start with a number. name := "a" + string(uuid.NewUUID()) - return limitadorv1alpha1.Limitador{ + return &limitadorv1alpha1.Limitador{ TypeMeta: metav1.TypeMeta{ Kind: "Limitador", APIVersion: "limitador.3scale.net/v1alpha1", @@ -51,14 +53,14 @@ var _ = Describe("Limitador controller", func() { deletePropagationPolicy := client.PropagationPolicy(metav1.DeletePropagationForeground) Context("Creating a new Limitador object", func() { - var limitadorObj limitadorv1alpha1.Limitador + var limitadorObj *limitadorv1alpha1.Limitador BeforeEach(func() { limitadorObj = newLimitador() - err := k8sClient.Delete(context.TODO(), limitadorObj.DeepCopy(), deletePropagationPolicy) + err := k8sClient.Delete(context.TODO(), limitadorObj, deletePropagationPolicy) Expect(err == nil || errors.IsNotFound(err)) - Expect(k8sClient.Create(context.TODO(), limitadorObj.DeepCopy())).Should(Succeed()) + Expect(k8sClient.Create(context.TODO(), limitadorObj)).Should(Succeed()) }) It("Should create a new deployment with the right number of replicas and version", func() { @@ -90,7 +92,7 @@ var _ = Describe("Limitador controller", func() { context.TODO(), types.NamespacedName{ Namespace: LimitadorNamespace, - Name: "limitador", // Hardcoded for now + Name: limitador.ServiceName, // Hardcoded for now }, &createdLimitadorService) @@ -99,55 +101,15 @@ var _ = Describe("Limitador controller", func() { }) }) - Context("Deleting a Limitador object", func() { - var limitadorObj limitadorv1alpha1.Limitador - - BeforeEach(func() { - limitadorObj = newLimitador() - Expect(k8sClient.Create(context.TODO(), limitadorObj.DeepCopy())).Should(Succeed()) - Expect(k8sClient.Delete(context.TODO(), limitadorObj.DeepCopy(), deletePropagationPolicy)).Should(Succeed()) - }) - - It("Should delete the limitador deployment", func() { - createdLimitadorDeployment := appsv1.Deployment{} - Eventually(func() bool { - err := k8sClient.Get( - context.TODO(), - types.NamespacedName{ - Namespace: LimitadorNamespace, - Name: limitadorObj.Name, - }, - &createdLimitadorDeployment) - - return errors.IsNotFound(err) - }, timeout, interval).Should(BeTrue()) - }) - - It("Should delete the limitador service", func() { - createdLimitadorService := v1.Service{} - Eventually(func() bool { - err := k8sClient.Get( - context.TODO(), - types.NamespacedName{ - Namespace: LimitadorNamespace, - Name: "limitador", // Hardcoded for now - }, - &createdLimitadorService) - - return errors.IsNotFound(err) - }, timeout, interval).Should(BeTrue()) - }) - }) - Context("Updating a limitador object", func() { - var limitadorObj limitadorv1alpha1.Limitador + var limitadorObj *limitadorv1alpha1.Limitador BeforeEach(func() { limitadorObj = newLimitador() - err := k8sClient.Delete(context.TODO(), limitadorObj.DeepCopy(), deletePropagationPolicy) + err := k8sClient.Delete(context.TODO(), limitadorObj, deletePropagationPolicy) Expect(err == nil || errors.IsNotFound(err)) - Expect(k8sClient.Create(context.TODO(), limitadorObj.DeepCopy())).Should(Succeed()) + Expect(k8sClient.Create(context.TODO(), limitadorObj)).Should(Succeed()) }) It("Should modify the limitador deployment", func() {