Check if unready binding can be removed (#959)

* added missing acceptance tests * in the reconcile loop, add finalizer only if deletion timestamp is unset * unbind handler: stop processing and do not retry in case of any error when reading the application resource Signed-off-by: Predrag Knezevic <pknezevi@redhat.com>
redhat-developer · May 3, 2021 · 4efb48c · 4efb48c
1 parent c256ac6
commit 4efb48c
Show file tree

Hide file tree

Showing 5 changed files with 55 additions and 16 deletions.
diff --git a/controllers/servicebinding_controller.go b/controllers/servicebinding_controller.go
@@ -79,12 +79,16 @@ func (r *ServiceBindingReconciler) Reconcile(req ctrl.Request) (ctrl.Result, err
 		log.Error(err, "Failed to get ServiceBinding", "name", req.NamespacedName, "err", err)
 		return ctrl.Result{}, err
 	}
-	if serviceBinding.MaybeAddFinalizer() {
+	if serviceBinding.DeletionTimestamp.IsZero() && serviceBinding.MaybeAddFinalizer() {
 		if err = r.Update(ctx, serviceBinding); err != nil {
 			return ctrl.Result{}, err
 		}
 	}
-	log.Info("Reconciling", "sb", serviceBinding)
+	if serviceBinding.DeletionTimestamp.IsZero() {
+		log.Info("Reconciling")
+	} else {
+		log.Info("Deleted, unbind the application")
+	}
 	retry, err := r.pipeline.Process(serviceBinding)
 	if !retry && err == nil {
 		if !serviceBinding.DeletionTimestamp.IsZero() {

diff --git a/pkg/reconcile/pipeline/handler/project/impl.go b/pkg/reconcile/pipeline/handler/project/impl.go
@@ -6,7 +6,6 @@ import (
 	"github.com/redhat-developer/service-binding-operator/api/v1alpha1"
 	"github.com/redhat-developer/service-binding-operator/pkg/reconcile/pipeline"
 	corev1 "k8s.io/api/core/v1"
-	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
 	"path"
@@ -224,15 +223,7 @@ func Unbind(ctx pipeline.Context) {
 		return
 	}
 	applications, err := ctx.Applications()
-	if err != nil {
-		if k8serrors.IsNotFound(err) {
-			ctx.StopProcessing()
-			return
-		}
-		ctx.RetryProcessing(err)
-		return
-	}
-	if len(applications) == 0 {
+	if err != nil || len(applications) == 0 {
 		ctx.StopProcessing()
 		return
 	}

diff --git a/pkg/reconcile/pipeline/handler/project/impl_test.go b/pkg/reconcile/pipeline/handler/project/impl_test.go
@@ -507,13 +507,20 @@ var _ = Describe("Unbind handler", func() {
 		ctx = mocks.NewMockContext(mockCtrl)
 		ctx.EXPECT().UnbindRequested().Return(true)
 		ctx.EXPECT().StopProcessing()
-
 	})
 
 	AfterEach(func() {
 		mockCtrl.Finish()
 	})
 
+	It("should stop processing when there is error getting applications", func() {
+		ctx.EXPECT().Applications().Return(nil, errors.New("foo"))
+		project.Unbind(ctx)
+	})
+	It("should stop processing when there are no applications", func() {
+		ctx.EXPECT().Applications().Return([]pipeline.Application{}, nil)
+		project.Unbind(ctx)
+	})
 	Context("successful processing", func() {
 		var (
 			deploymentsUnstructured    []*unstructured.Unstructured

diff --git a/test/acceptance/features/steps/steps.py b/test/acceptance/features/steps/steps.py
@@ -501,8 +501,8 @@ def validate_error(context, err_msg=None):
 @then(u'Service Binding "{sb_name}" is not persistent in the cluster')
 def validate_absent_sb(context, sb_name):
     openshift = Openshift()
-    output = openshift.search_resource_in_namespace("servicebindings", sb_name, context.namespace.name)
-    assert output is None, f"Service Binding {sb_name} is present in namespace '{context.namespace.name}'"
+    polling2.poll(lambda: openshift.search_resource_in_namespace("servicebindings", sb_name, context.namespace.name),
+                  step=5, timeout=400, check_success=lambda v: v is None)
 
 
 @then(u'Service Binding "{sb_name}" is not updated')

diff --git a/test/acceptance/features/unbindAppToService.feature b/test/acceptance/features/unbindAppToService.feature
@@ -154,4 +154,41 @@ Feature: Unbind an application from a service
         Then The application got redeployed 2 times so far
         * File "/bindings/remove-bindings-as-files-app-sb/host" is unavailable in application pod
         * File "/bindings/remove-bindings-as-files-app-sb/port" is unavailable in application pod
-        * Service Binding secret is not present
+        * Service Binding secret is not present
+
+    Scenario: Remove not ready binding
+        Given The Custom Resource is present
+            """
+            apiVersion: "stable.example.com/v1"
+            kind: Backend
+            metadata:
+                name: example-backend
+                annotations:
+                    service.binding/host: path={.spec.host}
+                    service.binding/username: path={.spec.username}
+            spec:
+                host: example.com
+                username: foo
+            """
+        * Service Binding is applied
+            """
+            apiVersion: binding.operators.coreos.com/v1alpha1
+            kind: ServiceBinding
+            metadata:
+                name: unready-binding
+            spec:
+                bindAsFiles: false
+                application:
+                    name: not-found-app
+                    group: apps
+                    version: v1
+                    resource: deployments
+                services:
+                -   group: stable.example.com
+                    version: v1
+                    kind: Backend
+                    name: example-backend
+            """
+        * jq ".status.conditions[] | select(.type=="Ready").status" of Service Binding "unready-binding" should be changed to "False"
+        When Service binding "unready-binding" is deleted
+        Then Service Binding "unready-binding" is not persistent in the cluster