Repository: redhat-developer/service-binding-operator
Pull request #426 (2020-04-16 03:13:53)

add extraFieldsModifier to binder
### Motivation
For some specific cases, binding to applications need extra fields to be modified other than env/volumes in container spec. For example we need to update revision name in knative service template if it is not empty. Detailed discussion please refer to https://github.com/redhat-developer/service-binding-operator/issues/418

### Changes
Add  `ExtraFieldsModifier` interface to binder and init it based on the GVR of ApplicationSelector.
If `ExtraFieldsModifier` is not nil and object is modified in `binder.update()`, the func `ModifyExtraFields` of interface`ExtraFieldsModifier`  is called.

### Testing
Unit test cases are 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 **sbose78**
You can assign the PR to them by writing `/assign @sbose78` 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":["sbose78"]} -->
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: /ok-to-test
codecov[bot]: # [Codecov](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/426?src=pr&el=h1) Report
> Merging [#426](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/426?src=pr&el=desc) into [master](https://codecov.io/gh/redhat-developer/service-binding-operator/commit/62ab03216c2a5ac2e46ca13a816d6c0511514a28&el=desc) will **increase** coverage by `0.84%`.
> The diff coverage is `91.66%`.

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

```diff
@@            Coverage Diff             @@
##           master     #426      +/-   ##
==========================================
+ Coverage   64.59%   65.44%   +0.84%     
==========================================
  Files          21       21              
  Lines        1418     1441      +23     
==========================================
+ Hits          916      943      +27     
+ Misses        376      373       -3     
+ Partials      126      125       -1     
```


| [Impacted Files](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/426?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/controller/servicebindingrequest/binding.go](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/426/diff?src=pr&el=tree#diff-cGtnL2NvbnRyb2xsZXIvc2VydmljZWJpbmRpbmdyZXF1ZXN0L2JpbmRpbmcuZ28=) | `69.12% <ø> (ø)` | |
| [pkg/controller/servicebindingrequest/binder.go](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/426/diff?src=pr&el=tree#diff-cGtnL2NvbnRyb2xsZXIvc2VydmljZWJpbmRpbmdyZXF1ZXN0L2JpbmRlci5nbw==) | `63.87% <91.66%> (+4.28%)` | :arrow_up: |

------

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


Patch:
diff --git a/pkg/controller/servicebindingrequest/binder.go b/pkg/controller/servicebindingrequest/binder.go
index 4811aafa..e733e4c2 100644
--- a/pkg/controller/servicebindingrequest/binder.go
+++ b/pkg/controller/servicebindingrequest/binder.go
@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"strings"
 	"time"
 
 	"k8s.io/apimachinery/pkg/fields"
@@ -14,6 +15,7 @@ import (
 	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"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
@@ -22,6 +24,7 @@ import (
 
 	"github.com/redhat-developer/service-binding-operator/pkg/apis/apps/v1alpha1"
 	"github.com/redhat-developer/service-binding-operator/pkg/log"
+	knativev1 "knative.dev/serving/pkg/apis/serving/v1"
 )
 
 var (
@@ -42,9 +45,25 @@ type Binder struct {
 	dynClient  dynamic.Interface               // kubernetes dynamic api client
 	sbr        *v1alpha1.ServiceBindingRequest // instantiated service binding request
 	volumeKeys []string                        // list of key names used in volume mounts
+	modifier   ExtraFieldsModifier             // extra modifier for CRDs before updating
 	logger     *log.Log                        // logger instance
 }
 
+// ExtraFieldsModifier is useful for updating backend service which requires additional changes besides
+// env/volumes updating. eg. for knative service we need to remove or update `spec.template.metadata.name`
+// from service template before updating otherwise it will be rejected.
+type ExtraFieldsModifier interface {
+	ModifyExtraFields(u *unstructured.Unstructured) error
+}
+
+// ExtraFieldsModifierFunc func receiver type for ExtraFieldsModifier
+type ExtraFieldsModifierFunc func(u *unstructured.Unstructured) error
+
+// ModifyExtraFields implements ExtraFieldsModifier interface
+func (f ExtraFieldsModifierFunc) ModifyExtraFields(u *unstructured.Unstructured) error {
+	return f(u)
+}
+
 var EmptyApplicationSelectorErr = errors.New("application ResourceRef or MatchLabel not found")
 
 // search objects based in Kind/APIVersion, which contain the labels defined in ApplicationSelector.
@@ -478,6 +497,13 @@ func (b *Binder) update(objs *unstructured.UnstructuredList) ([]*unstructured.Un
 			continue
 		}
 
+		if b.modifier != nil {
+			err = b.modifier.ModifyExtraFields(updatedObj)
+			if err != nil {
+				return nil, err
+			}
+		}
+
 		log.Debug("Updating object...")
 		if err := b.client.Update(b.ctx, updatedObj); err != nil {
 			return nil, err
@@ -552,12 +578,36 @@ func NewBinder(
 	sbr *v1alpha1.ServiceBindingRequest,
 	volumeKeys []string,
 ) *Binder {
+
+	logger := log.NewLog("binder")
+	modifier := extraFieldsModifier(logger, sbr)
+
 	return &Binder{
 		ctx:        ctx,
 		client:     client,
 		dynClient:  dynClient,
 		sbr:        sbr,
 		volumeKeys: volumeKeys,
-		logger:     log.NewLog("binder"),
+		modifier:   modifier,
+		logger:     logger,
+	}
+}
+
+func extraFieldsModifier(logger *log.Log, sbr *v1alpha1.ServiceBindingRequest) ExtraFieldsModifier {
+	gvr := sbr.Spec.ApplicationSelector.GroupVersionResource
+	ksvcgvr := knativev1.SchemeGroupVersion.WithResource("services")
+	switch gvr.String() {
+	case ksvcgvr.String():
+		pathToRevisionName := "spec.template.metadata.name"
+		return ExtraFieldsModifierFunc(func(u *unstructured.Unstructured) error {
+			revisionName, ok, err := unstructured.NestedString(u.Object, strings.Split(pathToRevisionName, ".")...)
+			if err == nil && ok {
+				logger.Info("remove revision in knative service template", "name", revisionName)
+				unstructured.RemoveNestedField(u.Object, strings.Split(pathToRevisionName, ".")...)
+			}
+			return nil
+		})
+	default:
+		return nil
 	}
 }
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/binder_test.go b/pkg/controller/servicebindingrequest/binder_test.go
index c6f683ed..4b0fa6d5 100644
--- a/pkg/controller/servicebindingrequest/binder_test.go
+++ b/pkg/controller/servicebindingrequest/binder_test.go
@@ -2,6 +2,7 @@ package servicebindingrequest
 
 import (
 	"context"
+	"fmt"
 	"testing"
 	"time"
 
@@ -13,6 +14,7 @@ import (
 	knativev1 "knative.dev/serving/pkg/apis/serving/v1"
 	logf "sigs.k8s.io/controller-runtime/pkg/runtime/log"
 
+	"github.com/redhat-developer/service-binding-operator/pkg/converter"
 	"github.com/redhat-developer/service-binding-operator/test/mocks"
 )
 
@@ -131,6 +133,28 @@ func TestBinderNew(t *testing.T) {
 		parsedTime, err := time.Parse(time.RFC3339, envVar.Value)
 		require.NoError(t, err)
 		require.True(t, parsedTime.Before(time.Now()))
+
+		// test binder with extra modifier present
+		ch := make(chan struct{})
+		binder.modifier = ExtraFieldsModifierFunc(func(u *unstructured.Unstructured) error {
+			close(ch)
+			return nil
+		})
+
+		list, err = binder.search()
+		require.NoError(t, err)
+		require.Equal(t, 1, len(list.Items))
+
+		updatedObjects, err = binder.update(list)
+		require.NoError(t, err)
+		require.Len(t, updatedObjects, 1)
+		<-ch
+
+		// call another update as object is already updated, modifier func should not be called
+		updatedObjects, err = binder.update(list)
+		require.NoError(t, err)
+		require.Len(t, updatedObjects, 0)
+		binder.modifier = nil
 	})
 
 	t.Run("remove", func(t *testing.T) {
@@ -300,6 +324,7 @@ func TestKnativeServicesContractWithBinder(t *testing.T) {
 	)
 
 	require.NotNil(t, binder)
+	require.NotNil(t, binder.modifier)
 
 	t.Run("Knative service contract with service binding operator", func(t *testing.T) {
 		list, err := binder.search()
@@ -308,4 +333,78 @@ func TestKnativeServicesContractWithBinder(t *testing.T) {
 
 	})
 
+	ksvc := mocks.KnativeServiceMock(ns, "knative-app-with-rev-name", matchLabels)
+	ksvc.Spec.Template.Name = "knative-app-with-rev-name-revision-1"
+
+}
+
+func Test_extraFieldsModifier(t *testing.T) {
+	ns := "binder"
+	name := "service-binding-request"
+	matchLabels := map[string]string{
+		"connects-to": "database",
+		"environment": "binder",
+	}
+
+	f := mocks.NewFake(t, ns)
+	deploy := mocks.DeploymentMock(ns, "deployment-fake", matchLabels)
+	sbr := mocks.ServiceBindingRequestMock(ns, name, nil, "", deploy.Name, deploymentsGVR, matchLabels)
+	binder := NewBinder(
+		context.TODO(),
+		f.FakeClient(),
+		f.FakeDynClient(),
+		sbr,
+		[]string{},
+	)
+
+	require.NotNil(t, binder)
+	require.Nil(t, binder.modifier)
+
+	gvr := knativev1.SchemeGroupVersion.WithResource("services")
+	ksvc := mocks.KnativeServiceMock(ns, "knative-app-with-rev-name", matchLabels)
+	sbr = mocks.ServiceBindingRequestMock(ns, name, nil, "", ksvc.Name, gvr, matchLabels)
+
+	binder = NewBinder(
+		context.TODO(),
+		f.FakeClient(),
+		f.FakeDynClient(),
+		sbr,
+		[]string{},
+	)
+
+	require.NotNil(t, binder)
+	require.NotNil(t, binder.modifier)
+
+	t.Run("ksvc revision name is empty", func(t *testing.T) {
+		u, err := converter.ToUnstructured(&ksvc)
+		require.NoError(t, err)
+
+		err = binder.modifier.ModifyExtraFields(u)
+		require.NoError(t, err)
+
+		var modified knativev1.Service
+		err = runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, &modified)
+		require.NoError(t, err)
+		assert.Equal(t, ksvc, modified)
+	})
+
+	t.Run("ksvc revision name is not empty", func(t *testing.T) {
+		ksvc.Spec.Template.Name = fmt.Sprintf("%s-%s", ksvc.Name, "rev-1")
+
+		u, err := converter.ToUnstructured(&ksvc)
+		require.NoError(t, err)
+
+		err = binder.modifier.ModifyExtraFields(u)
+		require.NoError(t, err)
+
+		var modified knativev1.Service
+		err = runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, &modified)
+		require.NoError(t, err)
+		assert.Equal(t, "", modified.Spec.Template.Name)
+
+		ksvc.Spec.Template.Name = ""
+		// the rest fields shoud not be modified
+		assert.Equal(t, ksvc, modified)
+	})
+
 }
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(),
+		},
+	}))
 }