Repository: redhat-developer/service-binding-operator
Pull request #432 (2020-04-22 07:00:05)

fixes generic binding
### Motivation

- Fixes generic binding bug
- Refer -> https://github.com/redhat-developer/service-binding-operator/pull/414/files#r407707455

### Changes

- Reverts some changes introduced in this PR ->  https://github.com/redhat-developer/service-binding-operator/pull/414

### How to test

1. Follow the steps given in this [example](https://github.com/redhat-developer/service-binding-operator/tree/master/examples/nodejs_postgresql) to:
- install DB operator, Service Binding Operator
- Import an application
- Create a DB instance 

2. Create the following `ServiceBindingRequest`:

```
apiVersion: apps.openshift.io/v1alpha1
kind: ServiceBindingRequest
metadata:
  name: binding-request
  namespace: service-binding-demo
spec:
 backingServiceSelector:
    group: postgresql.baiju.dev
    version: v1alpha1
    kind: Database
    resourceRef: db-demo
```

3. Use the following command to check if the binding secret has been generated:
`oc get ServiceBindingRequest binding-request -o yaml`

The output should have:

```
  secret: binding-request
```

openshift-ci-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by:
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **dhritishikhar**
You can assign the PR to them by writing `/assign @dhritishikhar` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=redhat-developer%2Fservice-binding-operator).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/redhat-developer/service-binding-operator/blob/master/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["dhritishikhar"]} -->
codecov[bot]: # [Codecov](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/432?src=pr&el=h1) Report
> Merging [#432](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/432?src=pr&el=desc) into [master](https://codecov.io/gh/redhat-developer/service-binding-operator/commit/a60d54a2851383ab643b5098f9b08624136cdd30&el=desc) will **increase** coverage by `1.10%`.
> The diff coverage is `93.75%`.

[![Impacted file tree graph](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/432/graphs/tree.svg?width=650&height=150&src=pr&token=CICBeVWlrC)](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/432?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #432      +/-   ##
==========================================
+ Coverage   65.14%   66.25%   +1.10%     
==========================================
  Files          21       21              
  Lines        1423     1443      +20     
==========================================
+ Hits          927      956      +29     
+ Misses        373      366       -7     
+ Partials      123      121       -2     
```


| [Impacted Files](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/432?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/controller/servicebindingrequest/planner.go](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/432/diff?src=pr&el=tree#diff-cGtnL2NvbnRyb2xsZXIvc2VydmljZWJpbmRpbmdyZXF1ZXN0L3BsYW5uZXIuZ28=) | `93.75% <ø> (-0.28%)` | :arrow_down: |
| [pkg/controller/servicebindingrequest/binder.go](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/432/diff?src=pr&el=tree#diff-cGtnL2NvbnRyb2xsZXIvc2VydmljZWJpbmRpbmdyZXF1ZXN0L2JpbmRlci5nbw==) | `64.51% <91.66%> (+4.93%)` | :arrow_up: |
| [pkg/controller/servicebindingrequest/binding.go](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/432/diff?src=pr&el=tree#diff-cGtnL2NvbnRyb2xsZXIvc2VydmljZWJpbmRpbmdyZXF1ZXN0L2JpbmRpbmcuZ28=) | `71.42% <100.00%> (ø)` | |
| [pkg/controller/servicebindingrequest/common.go](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/432/diff?src=pr&el=tree#diff-cGtnL2NvbnRyb2xsZXIvc2VydmljZWJpbmRpbmdyZXF1ZXN0L2NvbW1vbi5nbw==) | `61.76% <0.00%> (+8.82%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/432?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/432?src=pr&el=footer). Last update [eca1065...3d3a00b](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/432?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

Avni-Sharma: Making me wonder if we really need the err EmptyApplicationSelectorErr, at all https://github.com/redhat-developer/service-binding-operator/blob/master/pkg/controller/servicebindingrequest/reconciler.go#L153 
The generic binding should not log this as an error
DhritiShikhar: We need `EmptyApplicationSelectorErr` to return proper error message in our `Status.Conditions`.
With this PR, generic binding updates the status with `EmptyApplicationSelectorErr` reason and keeps on reconciling until the user updates the CR.

However, the reconcile method needs a bit of cleanup as this function is never reached -> https://github.com/redhat-developer/service-binding-operator/blob/master/pkg/controller/servicebindingrequest/reconciler.go#L152

We could create another issue for this cleanup.

This PR is about reverting some changes made in #414  


Patch:
diff --git a/pkg/controller/servicebindingrequest/binder.go b/pkg/controller/servicebindingrequest/binder.go
index e733e4c2..9840bb6f 100644
--- a/pkg/controller/servicebindingrequest/binder.go
+++ b/pkg/controller/servicebindingrequest/binder.go
@@ -13,7 +13,6 @@ import (
 
 	"gotest.tools/assert/cmp"
 	corev1 "k8s.io/api/core/v1"
-	k8serror "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
@@ -65,6 +64,7 @@ func (f ExtraFieldsModifierFunc) ModifyExtraFields(u *unstructured.Unstructured)
 }
 
 var EmptyApplicationSelectorErr = errors.New("application ResourceRef or MatchLabel not found")
+var ApplicationNotFound = errors.New("Application is already deleted")
 
 // search objects based in Kind/APIVersion, which contain the labels defined in ApplicationSelector.
 func (b *Binder) search() (*unstructured.UnstructuredList, error) {
@@ -95,15 +95,13 @@ func (b *Binder) search() (*unstructured.UnstructuredList, error) {
 
 	objList, err := b.dynClient.Resource(gvr).Namespace(ns).List(opts)
 	if err != nil {
-		return nil, err
+		return nil, ApplicationNotFound
+
 	}
 
 	// Return fake NotFound error explicitly to ensure requeue when objList(^) is empty.
 	if len(objList.Items) == 0 {
-		return nil, k8serror.NewNotFound(
-			gvr.GroupResource(),
-			b.sbr.Spec.ApplicationSelector.GroupVersionResource.Resource,
-		)
+		return nil, ApplicationNotFound
 	}
 	return objList, err
 }
@@ -555,6 +553,9 @@ func (b *Binder) remove(objs *unstructured.UnstructuredList) error {
 func (b *Binder) Unbind() error {
 	objs, err := b.search()
 	if err != nil {
+		if errors.Is(err, ApplicationNotFound) {
+			return nil
+		}
 		return err
 	}
 	return b.remove(objs)
@@ -565,6 +566,9 @@ func (b *Binder) Unbind() error {
 func (b *Binder) Bind() ([]*unstructured.Unstructured, error) {
 	objs, err := b.search()
 	if err != nil {
+		if errors.Is(err, ApplicationNotFound) {
+			return nil, nil
+		}
 		return nil, err
 	}
 	return b.update(objs)
diff --git a/pkg/controller/servicebindingrequest/planner.go b/pkg/controller/servicebindingrequest/planner.go
index 44ffde51..642213a5 100644
--- a/pkg/controller/servicebindingrequest/planner.go
+++ b/pkg/controller/servicebindingrequest/planner.go
@@ -79,11 +79,6 @@ var EmptyBackingServiceSelectorsErr = errors.New("backing service selectors are
 func (p *Planner) Plan() (*Plan, error) {
 	ns := p.sbr.GetNamespace()
 
-	var emptyApplication v1alpha1.ApplicationSelector
-	if p.sbr.Spec.ApplicationSelector == emptyApplication {
-		return nil, EmptyApplicationSelectorErr
-	}
-
 	var selectors []v1alpha1.BackingServiceSelector
 	if p.sbr.Spec.BackingServiceSelector != nil {
 		selectors = append(selectors, *p.sbr.Spec.BackingServiceSelector)
diff --git a/pkg/controller/servicebindingrequest/reconciler.go b/pkg/controller/servicebindingrequest/reconciler.go
index c6f574dd..0c6df2a8 100644
--- a/pkg/controller/servicebindingrequest/reconciler.go
+++ b/pkg/controller/servicebindingrequest/reconciler.go
@@ -1,6 +1,8 @@
 package servicebindingrequest
 
 import (
+	"errors"
+
 	v1 "github.com/openshift/custom-resource-status/conditions/v1"
 	"github.com/redhat-developer/service-binding-operator/pkg/conditions"
 	corev1 "k8s.io/api/core/v1"
@@ -114,6 +116,10 @@ func (r *Reconciler) Reconcile(request reconcile.Request) (reconcile.Result, err
 	// fetch and validate namespaced ServiceBindingRequest instance
 	sbr, err := r.getServiceBindingRequest(request.NamespacedName)
 	if err != nil {
+		if errors.Is(err, ApplicationNotFound) {
+			logger.Info("SBR deleted after application deletion")
+			return Done()
+		}
 		logger.Error(err, "On retrieving service-binding-request instance.")
 		return DoneOnNotFound(err)
 	}
@@ -147,8 +153,10 @@ func (r *Reconciler) Reconcile(request reconcile.Request) (reconcile.Result, err
 		if err == EmptyBackingServiceSelectorsErr || err == EmptyApplicationSelectorErr {
 			// TODO: find or create an error type containing suitable information to be propagated
 			var reason string
-			if err == EmptyBackingServiceSelectorsErr {
+			if errors.Is(err, EmptyBackingServiceSelectorsErr) {
 				reason = "EmptyBackingServiceSelectors"
+			} else if errors.Is(err, ApplicationNotFound) {
+				reason = "ApplicationUnavailable"
 			} else {
 				reason = "EmptyApplicationSelector"
 			}
diff --git a/pkg/controller/servicebindingrequest/sbrcontroller.go b/pkg/controller/servicebindingrequest/sbrcontroller.go
index bc61e287..a6f82b4d 100644
--- a/pkg/controller/servicebindingrequest/sbrcontroller.go
+++ b/pkg/controller/servicebindingrequest/sbrcontroller.go
@@ -147,7 +147,7 @@ func (s *SBRController) AddWatchForGVK(gvk schema.GroupVersionKind) error {
 // addCSVWatch creates a watch on ClusterServiceVersion.
 func (s *SBRController) addCSVWatch() error {
 	log := s.logger
-	gvr := olmv1alpha1.SchemeGroupVersion.WithResource(ClusterServiceVersionKind)
+	gvr := olmv1alpha1.SchemeGroupVersion.WithResource(csvResource)
 	resourceClient := s.Client.Resource(gvr).Namespace(os.Getenv("WATCH_NAMESPACE"))
 	_, err := resourceClient.List(metav1.ListOptions{})
 	if err != nil && errors.IsNotFound(err) {


Test patch:
diff --git a/pkg/controller/servicebindingrequest/binding_test.go b/pkg/controller/servicebindingrequest/binding_test.go
index 7c27886d..45537697 100644
--- a/pkg/controller/servicebindingrequest/binding_test.go
+++ b/pkg/controller/servicebindingrequest/binding_test.go
@@ -545,7 +545,7 @@ func TestServiceBinder_Bind(t *testing.T) {
 			SBR:                    sbrEmptyAppSelector,
 			Client:                 f.FakeClient(),
 		},
-		wantBuildErr: EmptyApplicationSelectorErr,
+		wantErr: EmptyApplicationSelectorErr,
 		wantConditions: []wantedCondition{
 			{
 				Type:    conditions.BindingReady,
diff --git a/pkg/controller/servicebindingrequest/reconciler_test.go b/pkg/controller/servicebindingrequest/reconciler_test.go
index 525503ed..929dae0a 100644
--- a/pkg/controller/servicebindingrequest/reconciler_test.go
+++ b/pkg/controller/servicebindingrequest/reconciler_test.go
@@ -229,14 +229,14 @@ func TestReconcilerGenericBinding(t *testing.T) {
 	// Reconcile without deployment
 	res, err := reconciler.Reconcile(reconcileRequest())
 	require.NoError(t, err)
-	require.True(t, res.Requeue)
+	require.False(t, res.Requeue)
 
 	namespacedName := types.NamespacedName{Namespace: reconcilerNs, Name: reconcilerName}
 	sbrOutput, err := reconciler.getServiceBindingRequest(namespacedName)
 	require.NoError(t, err)
 
 	require.Equal(t, conditions.BindingReady, sbrOutput.Status.Conditions[0].Type)
-	require.Equal(t, corev1.ConditionFalse, sbrOutput.Status.Conditions[0].Status)
+	require.Equal(t, corev1.ConditionTrue, sbrOutput.Status.Conditions[0].Status)
 	require.Equal(t, 0, len(sbrOutput.Status.Applications))
 
 	// Reconcile with deployment
@@ -347,13 +347,15 @@ func TestEmptyApplicationSelector(t *testing.T) {
 
 	res, err := reconciler.Reconcile(reconcileRequest())
 	require.NoError(t, err)
-	require.True(t, res.Requeue)
+	require.False(t, res.Requeue)
 
 	namespacedName := types.NamespacedName{Namespace: reconcilerNs, Name: reconcilerName}
 	sbrOutput, err := reconciler.getServiceBindingRequest(namespacedName)
 	require.NoError(t, err)
 
 	require.Equal(t, conditions.BindingReady, sbrOutput.Status.Conditions[0].Type)
-	require.Equal(t, corev1.ConditionFalse, sbrOutput.Status.Conditions[0].Status)
+	// Currently the Conditions[0].Status would be true as application's absence won't cause error
+	// TODO New steps to conditions to be introduced - InjectionReady, CollectionReady
+	require.Equal(t, corev1.ConditionTrue, sbrOutput.Status.Conditions[0].Status)
 	require.Equal(t, 0, len(sbrOutput.Status.Applications))
 }