Repository: redhat-developer/service-binding-operator
Pull request #645 (2020-08-27 15:34:45)

[acceptance-tests] Fix multiservice step to work with both oc v4.5 and v4.6
### Motivation

Currently the `"{app_name}" deployment must contain SBR name "{sbr_name1}" and "{sbr_name2}" step assumes that the output from the `oc get ... -o jsonpath=...` would be in the form of a go structure:

`[map[secretRef:map[name:{binding-request-1}]] map[secretRef:map[name:{binding-request-2}]]]`

which is what the `oc` v4.5 produces. However when `oc` v4.6 is used, the output changes into a valid JSON su:

`[{{"secretRef":{{"name":"{binding-request-1}"}}}},{{"secretRef":{{"name":"{binding-request-2}"}}}}]`

So if one runs the acceptance tests with `oc` v4.6, the scenarios fails on that step, because the expected and actual values of the result do not match even if the operator's behavior is correct.

### Changes

This PR:
* Adds a support for both variants of the result in the `"{app_name}" deployment must contain SBR name "{sbr_name1}" and "{sbr_name2}"` step
* Replaces hard-coded SBR names with the respective step input parameters

### Testing

`make test-acceptance`

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 **pmacik**
You can assign the PR to them by writing `/assign @pmacik` 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:

- **[test/OWNERS](https://github.com/redhat-developer/service-binding-operator/blob/master/test/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":["pmacik"]} -->

Patch:
diff --git a/README.md b/README.md
index 310a8b16..9fed6a61 100644
--- a/README.md
+++ b/README.md
@@ -26,7 +26,7 @@ information that's “interesting” to applications.
 There are multiple methods for making backing services
 bindable, including the backing service provider providing metadata as
 annotations on the resource. Details on the methods for making backing services bindable
-are available in the [Backing Service Provider Best Practices Guide](docs/OperatorBestPractices.md)
+are available in the [Backing Service Provider Best Practices Guide](docs/BackingServiceBestPractices.md)
 
 To make an imported application (for example, a NodeJS application)
 connect to a backing service (for example, a database):
diff --git a/docs/OperatorBestPractices.md b/docs/BackingServiceBestPractices.md
similarity index 64%
rename from docs/OperatorBestPractices.md
rename to docs/BackingServiceBestPractices.md
index e297be88..7939c861 100644
--- a/docs/OperatorBestPractices.md
+++ b/docs/BackingServiceBestPractices.md
@@ -1,26 +1,59 @@
-# Operator Best Practices
+# Best Practices for Making a Kubernetes Resource Bindable
 
 ## Introduction
 
 The goals of the Service Binding Operator is to make it easier for
 applications developers to bind applications with needed backing
-services, without having to perform manual configuration of secrets,
-configmaps, etc. and to assist operator providers in promoting and
+services, without having to perform manual configuration of `secrets`,
+`configmaps`, etc. and to assist operator providers in promoting and
 expanding the adoption of their operators.
 
-When a ServiceBindingRequest is created the Service Binding Operator
+When a `ServiceBindingRequest` is created the Service Binding Operator
 collects binding information and shares it with application. The
 Binding Service Operator's controller injects the binding information
-into the application's "DeploymentConfig", "Deployment" or “Replicaset"
-as environment variables via an intermediate Secret called "binding-request".
-The binding also works with Knative services as it works with any "deployment"
-controller which has the podspec defined in the its jsonpath as
+into the application's `DeploymentConfig`, `Deployment` or `Replicaset`
+as environment variables via an intermediate Secret.
+The binding also works with Knative `Services` as it works with any API which has the podspec defined in the its jsonpath as
 "spec.template.spec.containers".
 
 This document provides "best practices" guidelines for the development of
 Operators that manage backing services to be bound together with applications
 by the Service Binding Operator.
 
+## Making a Kubernetes Resource Bindable
+
+In order to make backing service bindable, the Kubernetes resource representing the same needs to be annotated as a means to express what is "interesting" for applications.
+
+Example, in an `Ingress` or `Route` resource representing a backing service, the `host` and `port` among other things would be "interesting" for applications for connecting to the backing service.
+
+To do so, the Kubernetes resource may be annotated to meaningfully convey what is "interesting" for binding.
+
+``` yaml
+kind: Route
+apiVersion: route.openshift.io/v1
+metadata:
+  name: example
+  namespace: service-binding-demo
+  annotations:
+    openshift.io/host.generated: 'true'
+    servicebindingoperator.redhat.io/spec.host: 'binding:env:attribute' #annotate here.
+spec:
+  host: example-sbo.apps.ci-ln-smyggvb-d5d6b.origin-ci-int-aws.dev.rhcloud.com
+  path: /
+  to:
+    kind: Service
+    name: example
+    weight: 100
+  port:
+    targetPort: 80
+  wildcardPolicy: None
+```
+
+## Making a Helm-Chart managed Backing Service Bindable
+
+Resources created from a Helm Chart are treated the same way as any other Kubernetes resource. The Kubernetes resource may be annotated the way in the chart templates to denote what is "interesting" for binding.
+
+
 ## Making an Operator Managed Backing Service Bindable
 
 In order to make a service bindable, the operator provider needs to express
@@ -52,8 +85,9 @@ apiVersion: apiextensions.k8s.io/v1beta1
 metadata:
   name: databases.postgresql.baiju.dev
   annotations:
-    servicebindingoperator.redhat.io/status.dbConfigMap.password: 'binding:env:object:secret'
-    servicebindingoperator.redhat.io/status.dbConfigMap.username: 'binding:env:object:configmap'
+    servicebindingoperator.redhat.io/status.dbConfigMap-host: 'binding:env:object:configmap'
+    servicebindingoperator.redhat.io/status.dbCredentials-password: 'binding:env:object:secret'
+    servicebindingoperator.redhat.io/status.dbCredentials-username: 'binding:env:object:secret'
     servicebindingoperator.redhat.io/status.dbName: 'binding:env:attribute'
     servicebindingoperator.redhat.io/spec.Token.private: 'binding:volumemount:secret'
 spec:
@@ -61,6 +95,20 @@ spec:
   version: v1alpha1
 ```
 
+The following annotation indicates that the key `host` in the `configmap` referenced in `status.dbConfigMap`
+is "interesting" for binding.
+
+```
+servicebindingoperator.redhat.io/status.dbConfigMap-host: 'binding:env:object:configmap'
+```
+
+Similarly, the following annotation indicates that the key `password` in the `secret` referenced in `status.dbCredentials`
+is "interesting" for binding.
+
+```
+servicebindingoperator.redhat.io/status.dbCredentials-password: 'binding:env:object:secret'
+```
+
 ### Operator Providing Metadata in OLM
 
 This feature enables operator providers to specify binding information an
diff --git a/pkg/controller/servicebindingrequest/binder.go b/pkg/controller/servicebindingrequest/binder.go
index d9153640..f7b42e9e 100644
--- a/pkg/controller/servicebindingrequest/binder.go
+++ b/pkg/controller/servicebindingrequest/binder.go
@@ -347,7 +347,7 @@ func (b *binder) appendEnvVar(
 // part of the list or appended.
 func (b *binder) appendEnvFrom(envList []corev1.EnvFromSource, secret string) []corev1.EnvFromSource {
 	for _, env := range envList {
-		if env.SecretRef.Name == secret {
+		if env.SecretRef != nil && env.SecretRef.Name == secret {
 			b.logger.Debug("Directive 'envFrom' is already present!")
 			// secret name is already referenced
 			return envList
@@ -368,7 +368,7 @@ func (b *binder) appendEnvFrom(envList []corev1.EnvFromSource, secret string) []
 func (b *binder) removeEnvFrom(envList []corev1.EnvFromSource, secret string) []corev1.EnvFromSource {
 	var cleanEnvList []corev1.EnvFromSource
 	for _, env := range envList {
-		if env.SecretRef.Name != secret {
+		if env.SecretRef != nil && env.SecretRef.Name != secret {
 			cleanEnvList = append(cleanEnvList, env)
 		}
 	}


Test patch:
diff --git a/pkg/controller/servicebindingrequest/binder_test.go b/pkg/controller/servicebindingrequest/binder_test.go
index b20371da..66683865 100644
--- a/pkg/controller/servicebindingrequest/binder_test.go
+++ b/pkg/controller/servicebindingrequest/binder_test.go
@@ -149,7 +149,7 @@ func TestBinderNew(t *testing.T) {
 		require.Equal(t, 1, len(list.Items))
 	})
 
-	t.Run("appendEnvFrom-removeEnvFrom", func(t *testing.T) {
+	t.Run("appendEnvFrom-removeEnvFrom-with-empty-envFrom", func(t *testing.T) {
 		binder := newBinder(
 			context.TODO(),
 			f.FakeDynClient(),
@@ -171,6 +171,37 @@ func TestBinderNew(t *testing.T) {
 		require.Equal(t, 0, len(list))
 	})
 
+	t.Run("appendEnvFrom-removeEnvFrom-with-configMapRef", func(t *testing.T) {
+		binder := newBinder(
+			context.TODO(),
+			f.FakeDynClient(),
+			sbr,
+			[]string{},
+			testutils.BuildTestRESTMapper(),
+		)
+
+		require.NotNil(t, binder)
+		secretName := "secret"
+		configMapName := "configmap"
+		d := mocks.DeploymentMock("binder", "binder", map[string]string{})
+		envFrom := d.Spec.Template.Spec.Containers[0].EnvFrom
+		envFrom = append(envFrom, corev1.EnvFromSource{
+			ConfigMapRef: &corev1.ConfigMapEnvSource{
+				LocalObjectReference: corev1.LocalObjectReference{
+					Name: configMapName,
+				},
+			},
+		})
+
+		list := binder.appendEnvFrom(envFrom, secretName)
+		require.Equal(t, 2, len(list))
+		require.Equal(t, configMapName, list[0].ConfigMapRef.Name)
+		require.Equal(t, secretName, list[1].SecretRef.Name)
+
+		list = binder.removeEnvFrom(envFrom, secretName)
+		require.Equal(t, 0, len(list))
+	})
+
 	t.Run("appendEnv", func(t *testing.T) {
 
 		binder := newBinder(
diff --git a/test/acceptance/features/steps/quarkus_application.py b/test/acceptance/features/steps/quarkus_application.py
index c1d5df50..5a09f073 100644
--- a/test/acceptance/features/steps/quarkus_application.py
+++ b/test/acceptance/features/steps/quarkus_application.py
@@ -108,8 +108,12 @@ class QuarkusApplication(object):
         return self.openshift.search_resource_lst_in_namespace("deployment", self.format_pattern(self.deployment_name_pattern), self.namespace)
 
     def get_deployment_with_intermediate_secret(self, intermediate_secret_name, wait=False, interval=5, timeout=300):
-        expected_secretRef_1 = f'[{{"secretRef":{{"name":"{intermediate_secret_name}"}}}}]'
-        expected_secretRef_2 = f'[map[secretRef:map[name:{intermediate_secret_name}]]]'
+
+        # Expected result from 'oc' (openshift client) v4.6
+        expected_secretRef_oc_46 = f'[{{"secretRef":{{"name":"{intermediate_secret_name}"}}}}]'
+        # Expected result from 'oc' (openshift client) v4.5
+        expected_secretRef_oc_45 = f'[map[secretRef:map[name:{intermediate_secret_name}]]]'
+
         deployment_name_pattern = self.format_pattern(self.deployment_name_pattern)
         if wait:
             start = 0
@@ -118,10 +122,11 @@ class QuarkusApplication(object):
                 if deployment_list is not None:
                     for deployment in deployment_list:
                         result = self.openshift.get_deployment_envFrom_info(deployment, self.namespace)
-                        if result == expected_secretRef_1 or result == expected_secretRef_2:
+                        if result == expected_secretRef_oc_45 or result == expected_secretRef_oc_46:
                             return deployment
                         else:
-                            print(f"\nUnexpected deployment's envFrom info: \nExpected: {expected_secretRef_1} or {expected_secretRef_2} \nbut was: {result}\n")
+                            print("\nUnexpected deployment's envFrom info: \n" +
+                                  f"Expected: {expected_secretRef_oc_45} or {expected_secretRef_oc_46} \nbut was: {result}\n")
                 else:
                     print(f"No deployment that matches {deployment_name_pattern} found.\n")
                 time.sleep(interval)
@@ -131,10 +136,11 @@ class QuarkusApplication(object):
             if deployment_list is not None:
                 for deployment in deployment_list:
                     result = self.openshift.get_deployment_envFrom_info(deployment, self.namespace)
-                    if result == expected_secretRef_1 or result == expected_secretRef_2:
+                    if result == expected_secretRef_oc_45 or result == expected_secretRef_oc_46:
                         return deployment
                     else:
-                        print(f"\nUnexpected deployment's envFrom info: \nExpected: {expected_secretRef_1} or {expected_secretRef_2} \nbut was: {result}\n")
+                        print("\nUnexpected deployment's envFrom info: \n" +
+                              f"Expected: {expected_secretRef_oc_45} or {expected_secretRef_oc_46} \nbut was: {result}\n")
             else:
                 print(f"No deployment that matches {deployment_name_pattern} found.\n")
         return None
diff --git a/test/acceptance/features/steps/steps.py b/test/acceptance/features/steps/steps.py
index d5bde495..e4d09e7a 100644
--- a/test/acceptance/features/steps/steps.py
+++ b/test/acceptance/features/steps/steps.py
@@ -267,8 +267,14 @@ def then_envFrom_contains(context, app_name, sbr_name1, sbr_name2):
     time.sleep(60)
     openshift = Openshift()
     result = openshift.get_deployment_envFrom_info(app_name, context.namespace.name)
-    result | should.be_equal_to("[map[secretRef:map[name:binding-request-1]] map[secretRef:map[name:binding-request-2]]]")\
-        .desc(f'{app_name} deployment should contain secretRef: {sbr_name1} and {sbr_name2}')
+
+    # Expected result from 'oc' (openshift client) v4.5
+    expected_result_oc_45 = f'[map[secretRef:map[name:{sbr_name1}]] map[secretRef:map[name:{sbr_name2}]]]'
+    # Expected result from 'oc' (openshift client) v4.6+
+    expected_result_oc_46 = f'[{{"secretRef":{{"name":"{sbr_name1}"}}}},{{"secretRef":{{"name":"{sbr_name2}"}}}}]'
+
+    assert result == expected_result_oc_45 or result == expected_result_oc_46, \
+        f'\n{app_name} deployment should contain secretRef: {sbr_name1} and {sbr_name2}\nActual secretRef: {result}'
 
 
 # STEP