Merge pull request #73 from camilamacedo86/AEROGEAR-9161

feat(finalizer) : Following the recommendations over how to work with finalizers

deploy/crds/examples/mobile-security-service_v1alpha1_mobilesecurityserviceapp_cr.yaml

@@ -2,8 +2,6 @@ apiVersion: mobile-security-service.aerogear.com/v1alpha1
 kind: MobileSecurityServiceApp
 metadata:
   name: mobile-security-service-app
-  finalizers:
-  - finalizer.mobile-security-service.aerogear.com
 spec:
   appName: "example-appname"
   # The appId spec defines the appId of the app used to bind the service

deploy/crds/mobile-security-service_v1alpha1_mobilesecurityservice_cr.yaml

@@ -31,4 +31,4 @@ spec:
   #    If the name be not specified then it will be the Mobile Security Service instance name.
   #    The DB will looking for this ConfigMap to get the Env Values variables
   configMapName: "mobile-security-service-config"
-  routeName: "route"
+  routeName: "route"

pkg/controller/mobilesecurityserviceapp/controller.go

@@ -144,11 +144,6 @@ func (r *ReconcileMobileSecurityServiceApp) Reconcile(request reconcile.Request)
 		return reconcile.Result{}, nil
 	}
 
-	//Check specs
-	if !hasMandatorySpecs(instance, serviceInstance, reqLogger) {
-		return reconcile.Result{Requeue: true}, nil
-	}
-
 	reqLogger.Info("Checking if the route already exists ...")
 	route := &routev1.Route{}
 	if err := r.client.Get(context.TODO(), types.NamespacedName{Name: utils.GetRouteName(serviceInstance), Namespace: operatorNamespace}, route); err != nil {
@@ -158,6 +153,43 @@ func (r *ReconcileMobileSecurityServiceApp) Reconcile(request reconcile.Request)
 	//Get the REST Service Endpoint
 	serviceAPI := service.GetServiceAPIURL(route, serviceInstance)
 
+	//Check if the APP CR was marked to be deleted
+	isAppMarkedToBeDeleted := instance.GetDeletionTimestamp() != nil
+	if isAppMarkedToBeDeleted {
+		//If the CR was marked to be deleted before it finalizes the app need to be deleted from the Service
+		//Do request to get the app.ID to delete app
+		app, err := fetchBindAppRestServiceByAppID(serviceAPI, instance, reqLogger);
+		if err != nil {
+			return reconcile.Result{}, err
+		}
+
+		// If the request works with success and the app was found then
+		// Do request to delete it from the service
+		if  app.ID != "" {
+			if err := service.DeleteAppFromServiceByRestAPI(serviceAPI,  app.ID, reqLogger); err != nil {
+				reqLogger.Error(err, "Unable to delete app from Service", "App.ID",  app.ID)
+				return reconcile.Result{}, err
+			}
+			reqLogger.Info("Successfully delete app ...")
+		}
+
+		// Check if the finalizer criteria is met and remove finalizer from the CR
+		if err := r.removeFinalizer(serviceAPI, reqLogger, request); err != nil {
+			return reconcile.Result{}, err
+		}
+		return reconcile.Result{}, nil
+	}
+
+	//Check specs
+	if !hasMandatorySpecs(instance, serviceInstance, reqLogger) {
+		return reconcile.Result{Requeue: true}, nil
+	}
+
+	//Add finalizer for this CR
+	if err := r.addFinalizer(reqLogger, instance, request); err != nil {
+		return reconcile.Result{}, err
+	}
+
 	//Check if ConfigMap for the app exist, if not create one.
 	if _, err := r.fetchSDKConfigMap(reqLogger, instance); err != nil {
 		return r.create(instance, CONFIGMAP, serviceAPI, reqLogger, request)
@@ -166,7 +198,7 @@ func (r *ReconcileMobileSecurityServiceApp) Reconcile(request reconcile.Request)
 	//Check if App is Bind in the REST Service, if not then bind it
 	if app, err := fetchBindAppRestServiceByAppID(serviceAPI, instance, reqLogger); err == nil {
 		//Update the name by the REST API
-		if hasApp(app) && app.AppName != instance.Spec.AppName {
+		if app.AppName != instance.Spec.AppName {
 			app.AppName = instance.Spec.AppName
 			//Check if App was update with success
 
@@ -176,7 +208,7 @@ func (r *ReconcileMobileSecurityServiceApp) Reconcile(request reconcile.Request)
 			return reconcile.Result{Requeue: true}, nil
 		}
 		// Bind App in the Service by the REST API
-		if !hasApp(app) {
+		if app.ID == "" {
 			newApp := models.NewApp(instance.Spec.AppName, instance.Spec.AppId)
 			if err := service.CreateAppByRestAPI(serviceAPI, newApp, reqLogger); err != nil {
 				return reconcile.Result{}, err
@@ -196,15 +228,5 @@ func (r *ReconcileMobileSecurityServiceApp) Reconcile(request reconcile.Request)
 		return reconcile.Result{}, err
 	}
 
-	//Handle the finalizer when the CR is deleted to unbind the app
-	if err := r.handleFinalizer(serviceAPI, reqLogger, request); err != nil {
-		return reconcile.Result{}, err
-	}
-
-	//Update finalizer timestamp to allow delete CR
-	if err := r.updateFinilizer(serviceAPI, reqLogger, request); err != nil {
-		return reconcile.Result{}, err
-	}
-
 	return reconcile.Result{}, nil
 }

pkg/controller/mobilesecurityserviceapp/finalizer.go

@@ -2,56 +2,61 @@ package mobilesecurityserviceapp
 
 import (
 	"context"
-	"fmt"
-	"github.com/aerogear/mobile-security-service-operator/pkg/service"
+	mobilesecurityservicev1alpha1 "github.com/aerogear/mobile-security-service-operator/pkg/apis/mobilesecurityservice/v1alpha1"
 	"github.com/go-logr/logr"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 )
-//updateFinilizer returns error when the app still not deleted in the REST Service
-func (r *ReconcileMobileSecurityServiceApp) updateFinilizer(serviceAPI string,reqLogger logr.Logger, request reconcile.Request) error {
-	instance, err := r.fetchInstance(reqLogger, request)
-	if err != nil {
-		return err
-	}
-	if len(instance.GetFinalizers()) > 0 && instance.GetDeletionTimestamp() != nil {
-		reqLogger.Info("Removing Finalizer for the MobileSecurityServiceApp")
-		if app, err := fetchBindAppRestServiceByAppID(serviceAPI, instance, reqLogger);  err != nil || hasApp(app){
-			if hasApp(app) {
-				err := fmt.Errorf("App was found in the REST Service API")
-				reqLogger.Error(err, "Unable to delete APP", "App.appId", instance.Spec.AppId)
-				return err
-			}
+
+//addFinalizer will add the Finalizer metadata in the Mobile Security Service App CR
+func (r *ReconcileMobileSecurityServiceApp) addFinalizer(reqLogger logr.Logger, instance *mobilesecurityservicev1alpha1.MobileSecurityServiceApp,  request reconcile.Request) error {
+	if len(instance.GetFinalizers()) < 1 && instance.GetDeletionTimestamp() == nil {
+		reqLogger.Info("Adding Finalizer for the MobileSecurityServiceApp")
+
+		// Get the latest version of CR
+		instance, err := r.fetchInstance(reqLogger, request)
+		if err != nil {
 			return err
 		}
-		instance.SetFinalizers(nil)
-		instance.SetDeletionTimestamp(nil)
-		err := r.client.Update(context.TODO(), instance)
+
+		//Set finalizer string/metadata
+		instance.SetFinalizers([]string{FINALIZER})
+
+		//Update CR
+		err = r.client.Update(context.TODO(), instance)
 		if err != nil {
-			reqLogger.Error(err, "Failed to update finalizer for the MobileSecurityService App")
+			reqLogger.Error(err, "Failed to update MobileSecurityService App CR with  finalizer")
 			return err
 		}
 	}
 	return nil
 }
 
-//handleFinalizer check if has the finalizer and delete the app
-func (r *ReconcileMobileSecurityServiceApp) handleFinalizer(serviceAPI string,reqLogger logr.Logger, request reconcile.Request) error {
+
+//removeFinalizer returns error when the app still not deleted in the REST Service
+func (r *ReconcileMobileSecurityServiceApp) removeFinalizer(serviceAPI string,reqLogger logr.Logger, request reconcile.Request) error {
+
+	// Get the latest version of CR
 	instance, err := r.fetchInstance(reqLogger, request)
 	if err != nil {
 		return err
 	}
-	// set up finalizers
+
 	if len(instance.GetFinalizers()) > 0 && instance.GetDeletionTimestamp() != nil {
-		//Check if App is delete into the REST Service
-		if app, err := fetchBindAppRestServiceByAppID(serviceAPI, instance, reqLogger); err == nil {
-			if hasApp(app) {
-				if err := service.DeleteAppFromServiceByRestAPI(serviceAPI,  app.ID, reqLogger); err != nil {
-					reqLogger.Error(err, "App was not delete on the Rest Service", "App.id",  app.ID)
-					return err
-				}
-				return nil
-			}
+		reqLogger.Info("Removing Finalizer for the MobileSecurityServiceApp")
+		if app, err := fetchBindAppRestServiceByAppID(serviceAPI, instance, reqLogger);  err != nil || app.ID != "" {
+			reqLogger.Error(err, "Unable to delete app", "App.appId", instance.Spec.AppId, "app.ID", app.ID)
+			return err
+		}
+
+		//Remove finalizer
+		instance.SetFinalizers(nil)
+
+		//Update CR
+		err := r.client.Update(context.TODO(), instance)
+		if err != nil {
+			reqLogger.Error(err, "Failed to update MobileSecurityService App CR with finalizer")
+			return err
 		}
 	}
 	return nil
-}
+}

pkg/controller/mobilesecurityserviceapp/helpers.go

@@ -9,6 +9,7 @@ import (
 )
 
 const SDK  = "-sdk"
+const FINALIZER = "finalizer.mobile-security-service.aerogear.com"
 
 // Returns an string map with the labels which wil be associated to the kubernetes/openshift objects
 // which will be created and managed by this operator
@@ -35,11 +36,6 @@ func getSDKConfigMapName(m *mobilesecurityservicev1alpha1.MobileSecurityServiceA
 	return m.Spec.AppName + SDK
 }
 
-//hasApp return true when APP has ID which is just created by the REST Service API
-func hasApp(app models.App) bool {
-	return len(app.ID) > 0
-}
-
 //Check if the mandatory specs are filled
 func hasMandatorySpecs(instance *mobilesecurityservicev1alpha1.MobileSecurityServiceApp, serviceInstance *mobilesecurityservicev1alpha1.MobileSecurityService, reqLogger logr.Logger) bool {
 	//Check if the appId was added in the CR

pkg/controller/mobilesecurityserviceapp/status.go

@@ -65,7 +65,7 @@ func (r *ReconcileMobileSecurityServiceApp) updateBindStatus(serviceURL string,r
 	}
 
 	// Check if the ConfigMap and the App is created in the Rest Service
-	if len(SDKConfigMapStatus.UID) < 1 && !hasApp(app) {
+	if len(SDKConfigMapStatus.UID) < 1 && app.ID == "" {
 		err := fmt.Errorf("Failed to get OK Status for MobileSecurityService Bind.")
 		reqLogger.Error(err, "One of the resources are not created", "MobileSecurityServiceApp.Namespace", instance.Namespace, "MobileSecurityServiceApp.Name", instance.Name)
 		return err

pkg/service/client.go

@@ -2,6 +2,7 @@ package service
 
 import (
 	"encoding/json"
+	"fmt"
 	"github.com/aerogear/mobile-security-service-operator/pkg/models"
 	"github.com/go-logr/logr"
 	"net/http"
@@ -78,8 +79,8 @@ func GetAppFromServiceByRestApi(serviceAPI string, appId string, reqLogger logr.
 	got := models.App{}
 
 	if appId == "" {
-		reqLogger.Info( "App without AppId", "App.AppId", appId)
-		return got, nil
+		err := fmt.Errorf( "App without AppId", "App.AppId", appId)
+		return got, err
 	}
 
 	//Create the GET request
@@ -102,8 +103,9 @@ func GetAppFromServiceByRestApi(serviceAPI string, appId string, reqLogger logr.
 	}
 
 	if 200 != response.StatusCode && 204 != response.StatusCode {
+		err := fmt.Errorf( "HTTP StatusCode not expected")
 		reqLogger.Error(err, "HTTP StatusCode not expected", "HTTPMethod", http.MethodGet, "url", url, "response.StatusCode", response.StatusCode)
-		return got, nil
+		return got, err
 	}
 
 	var obj []models.App