Repository: redhat-developer/service-binding-operator
Pull request #430 (2020-04-19 15:53:14)

check SBR deletion before calling retriever.Get()
### Motivation

If a sbr is created with a bad customEnvVar template, the sbr cannot be deleted. Issue https://github.com/redhat-developer/service-binding-operator/issues/429

### Changes

Check the SBR's `DeletionTimestamp` before calling `Retriever.Get()` as the data is not used in deletion path.

### Testing
Unit test cases added


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"]} -->
openshift-ci-robot: Hi @int8dev. Thanks for your PR.

I'm waiting for a [redhat-developer](https://github.com/orgs/redhat-developer/people) member to verify that this patch is reasonable to test. If it is, they should reply with `/ok-to-test` on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should [join the org](https://github.com/orgs/redhat-developer/people) to skip this step.

Once the patch is verified, the new status will be reflected by the `ok-to-test` label.

I understand the commands that are listed [here](https://go.k8s.io/bot-commands?repo=redhat-developer%2Fservice-binding-operator).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>

sbose78: Thank you very much!
sbose78: /ok-to-test
codecov[bot]: # [Codecov](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/430?src=pr&el=h1) Report
> Merging [#430](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/430?src=pr&el=desc) into [master](https://codecov.io/gh/redhat-developer/service-binding-operator/commit/62ab03216c2a5ac2e46ca13a816d6c0511514a28&el=desc) will **increase** coverage by `0.54%`.
> The diff coverage is `100.00%`.

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

```diff
@@            Coverage Diff             @@
##           master     #430      +/-   ##
==========================================
+ Coverage   64.59%   65.14%   +0.54%     
==========================================
  Files          21       21              
  Lines        1418     1423       +5     
==========================================
+ Hits          916      927      +11     
+ Misses        376      373       -3     
+ Partials      126      123       -3     
```


| [Impacted Files](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/430?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/controller/servicebindingrequest/binding.go](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/430/diff?src=pr&el=tree#diff-cGtnL2NvbnRyb2xsZXIvc2VydmljZWJpbmRpbmdyZXF1ZXN0L2JpbmRpbmcuZ28=) | `71.42% <100.00%> (+2.30%)` | :arrow_up: |
| [...roller/servicebindingrequest/retrieve\_decoupled.go](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/430/diff?src=pr&el=tree#diff-cGtnL2NvbnRyb2xsZXIvc2VydmljZWJpbmRpbmdyZXF1ZXN0L3JldHJpZXZlX2RlY291cGxlZC5nbw==) | `80.48% <0.00%> (+4.87%)` | :arrow_up: |
| [...troller/servicebindingrequest/custom\_env\_parser.go](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/430/diff?src=pr&el=tree#diff-cGtnL2NvbnRyb2xsZXIvc2VydmljZWJpbmRpbmdyZXF1ZXN0L2N1c3RvbV9lbnZfcGFyc2VyLmdv) | `80.95% <0.00%> (+9.52%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/430?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/430?src=pr&el=footer). Last update [0ef6872...a60d54a](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/430?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

sbose78: /lgtm
/approve
openshift-ci-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/redhat-developer/service-binding-operator/pull/430#issuecomment-616183114" title="Approved">sbose78</a>*

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).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

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

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

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":[]} -->

Patch:
diff --git a/pkg/controller/servicebindingrequest/binding.go b/pkg/controller/servicebindingrequest/binding.go
index d7ddb218..8e35e8f2 100644
--- a/pkg/controller/servicebindingrequest/binding.go
+++ b/pkg/controller/servicebindingrequest/binding.go
@@ -299,6 +299,12 @@ var InvalidOptionsErr = errors.New("invalid options")
 
 // BuildServiceBinder creates a new binding manager according to options.
 func BuildServiceBinder(options *ServiceBinderOptions) (*ServiceBinder, error) {
+
+	var isSBRDeleting bool
+	if options.SBR != nil && options.SBR.GetDeletionTimestamp() != nil {
+		isSBRDeleting = true
+	}
+
 	if !options.Valid() {
 		return nil, InvalidOptionsErr
 	}
@@ -336,11 +342,15 @@ func BuildServiceBinder(options *ServiceBinderOptions) (*ServiceBinder, error) {
 		}
 	}
 
-	// gather retriever's read data
-	// TODO: do not return error
-	retrievedData, err := retriever.Get()
-	if err != nil {
-		return nil, err
+	var retrievedData map[string][]byte
+
+	if !isSBRDeleting {
+		// gather retriever's read data
+		// TODO: do not return error
+		retrievedData, err = retriever.Get()
+		if err != nil {
+			return nil, err
+		}
 	}
 
 	// gather related secret, again only appending it if there's a value.


Test patch:
diff --git a/pkg/controller/servicebindingrequest/binding_test.go b/pkg/controller/servicebindingrequest/binding_test.go
index c896dc59..7c27886d 100644
--- a/pkg/controller/servicebindingrequest/binding_test.go
+++ b/pkg/controller/servicebindingrequest/binding_test.go
@@ -2,8 +2,10 @@ package servicebindingrequest
 
 import (
 	"encoding/base64"
+	"errors"
 	"strings"
 	"testing"
+	"time"
 
 	conditionsv1 "github.com/openshift/custom-resource-status/conditions/v1"
 	"github.com/redhat-developer/service-binding-operator/pkg/conditions"
@@ -87,7 +89,7 @@ func TestServiceBinder_Bind(t *testing.T) {
 		return func(t *testing.T) {
 			sb, err := BuildServiceBinder(args.options)
 			if args.wantBuildErr != nil {
-				require.Error(t, err)
+				require.EqualError(t, err, args.wantBuildErr.Error())
 				return
 			} else {
 				require.NoError(t, err)
@@ -96,8 +98,7 @@ func TestServiceBinder_Bind(t *testing.T) {
 			res, err := sb.Bind()
 
 			if args.wantErr != nil {
-				require.Error(t, err)
-				require.Equal(t, args.wantErr, err)
+				require.EqualError(t, err, args.wantErr.Error())
 			} else {
 				require.NoError(t, err)
 			}
@@ -310,6 +311,46 @@ func TestServiceBinder_Bind(t *testing.T) {
 	}
 	f.AddMockResource(sbrSingleService)
 
+	sbrWithBadCustomEnvVarTemplate := &v1alpha1.ServiceBindingRequest{
+		TypeMeta: metav1.TypeMeta{
+			APIVersion: "apps.openshift.io/v1alpha1",
+			Kind:       "ServiceBindingRequest",
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "single-sbr-with-bad-customenvvar-template",
+		},
+		Spec: v1alpha1.ServiceBindingRequestSpec{
+			ApplicationSelector: v1alpha1.ApplicationSelector{
+				LabelSelector: &metav1.LabelSelector{
+					MatchLabels: matchLabels,
+				},
+				GroupVersionResource: metav1.GroupVersionResource{
+					Group:    d.GetObjectKind().GroupVersionKind().Group,
+					Version:  d.GetObjectKind().GroupVersionKind().Version,
+					Resource: "deployments",
+				},
+				ResourceRef: d.GetName(),
+			},
+			BackingServiceSelectors: &[]v1alpha1.BackingServiceSelector{
+				{
+					GroupVersionKind: metav1.GroupVersionKind{
+						Group:   db1.GetObjectKind().GroupVersionKind().Group,
+						Version: db1.GetObjectKind().GroupVersionKind().Version,
+						Kind:    db1.GetObjectKind().GroupVersionKind().Kind,
+					},
+					ResourceRef: db1.GetName(),
+				},
+			},
+			CustomEnvVar: []corev1.EnvVar{
+				{
+					Name:  "MY_DB_NAME",
+					Value: `{{ .status.dbName `,
+				},
+			},
+		},
+		Status: v1alpha1.ServiceBindingRequestStatus{},
+	}
+
 	// create the ServiceBindingRequest
 	sbrMultipleServices := &v1alpha1.ServiceBindingRequest{
 		TypeMeta: metav1.TypeMeta{
@@ -586,4 +627,28 @@ func TestServiceBinder_Bind(t *testing.T) {
 			},
 		},
 	}))
+
+	t.Run("bind SBR with bad custom env var template", assertBind(args{
+		options: &ServiceBinderOptions{
+			Logger:                 logger,
+			DynClient:              f.FakeDynClient(),
+			DetectBindingResources: false,
+			EnvVarPrefix:           "",
+			SBR:                    sbrWithBadCustomEnvVarTemplate,
+			Client:                 f.FakeClient(),
+		},
+		wantBuildErr: errors.New("template: set:1: unclosed action"),
+	}))
+
+	sbrWithBadCustomEnvVarTemplate.DeletionTimestamp = &metav1.Time{Time: time.Now()}
+	t.Run("bind SBR with bad custom env var template and deletion timestamp", assertBind(args{
+		options: &ServiceBinderOptions{
+			Logger:                 logger,
+			DynClient:              f.FakeDynClient(),
+			DetectBindingResources: false,
+			EnvVarPrefix:           "",
+			SBR:                    sbrWithBadCustomEnvVarTemplate,
+			Client:                 f.FakeClient(),
+		},
+	}))
 }