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 b8c98129..425b9525 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" @@ -47,19 +45,11 @@ 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) { - 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,14 +57,20 @@ func (r *LimitadorReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { } } - if err := r.ensureLimitadorServiceExists(); err != nil { - return ctrl.Result{}, err + if limitadorObj.GetDeletionTimestamp() != nil { // Marked to be deleted + reqLogger.V(1).Info("marked to be deleted") + return ctrl.Result{}, nil } - desiredDeployment := limitador.LimitadorDeployment(&limitadorObj) + err := r.ensureLimitadorServiceExists(limitadorObj) + reqLogger.V(1).Info("reconcile service", "error", err) + if err != nil { + return ctrl.Result{}, err + } - 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 } @@ -121,25 +117,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 +132,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..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" @@ -10,12 +9,16 @@ 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" + + limitadorv1alpha1 "github.com/3scale/limitador-operator/api/v1alpha1" + "github.com/3scale/limitador-operator/pkg/limitador" ) var _ = Describe("Limitador controller", func() { const ( - LimitadorName = "limitador-test" LimitadorNamespace = "default" LimitadorReplicas = 2 LimitadorImage = "quay.io/3scale/limitador" @@ -27,27 +30,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, deletePropagationPolicy) Expect(err == nil || errors.IsNotFound(err)) - Expect(k8sClient.Create(context.TODO(), limitador.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() { @@ -57,7 +70,7 @@ var _ = Describe("Limitador controller", func() { context.TODO(), types.NamespacedName{ Namespace: LimitadorNamespace, - Name: LimitadorName, + Name: limitadorObj.Name, }, &createdLimitadorDeployment) @@ -74,65 +87,29 @@ var _ = Describe("Limitador controller", func() { It("Should create a Limitador service", func() { createdLimitadorService := v1.Service{} - Eventually(func() bool { - err := k8sClient.Get( - context.TODO(), - types.NamespacedName{ - Namespace: "default", // Hardcoded for now - Name: "limitador", // Hardcoded for now - }, - &createdLimitadorService) - - return err == nil - }, timeout, interval).Should(BeTrue()) - }) - }) - - Context("Deleting a Limitador object", func() { - BeforeEach(func() { - err := k8sClient.Create(context.TODO(), limitador.DeepCopy()) - Expect(err == nil || errors.IsAlreadyExists(err)) - - Expect(k8sClient.Delete(context.TODO(), limitador.DeepCopy())).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: LimitadorName, - }, - &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: "default", // Hardcoded for now - Name: "limitador", // Hardcoded for now + Name: limitador.ServiceName, // Hardcoded for now }, &createdLimitadorService) - return errors.IsNotFound(err) + return err == nil }, timeout, interval).Should(BeTrue()) }) }) 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, deletePropagationPolicy) Expect(err == nil || errors.IsNotFound(err)) - Expect(k8sClient.Create(context.TODO(), limitador.DeepCopy())).Should(Succeed()) + Expect(k8sClient.Create(context.TODO(), limitadorObj)).Should(Succeed()) }) It("Should modify the limitador deployment", func() { @@ -142,7 +119,7 @@ var _ = Describe("Limitador controller", func() { context.TODO(), types.NamespacedName{ Namespace: LimitadorNamespace, - Name: LimitadorName, + Name: limitadorObj.Name, }, &updatedLimitador) @@ -161,7 +138,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, + } +}