Repository: redhat-developer/service-binding-operator
Pull request #371 (2020-03-09 14:45:39)

(bug) Fix normalization of annotations to CRDDescription
### Motivation

Parsing the annotations into the data structures which contained spec|status descriptors had a bug where only one spec descriptor and one status descriptor was being persisted irrespective of the total number of annotations present.

Example, the following 
```
"servicebindingoperator.redhat.io/status.dbCredentials-db.password": "binding:env:object:secret",
"servicebindingoperator.redhat.io/spec.dbName":                      "binding:env:attribute",
"servicebindingoperator.redhat.io/spec.image":                       "binding:env:attribute",
"servicebindingoperator.redhat.io/status.dbConfigMap-db.host":       "binding:env:object:configmap",
```
was being mapped to only one spec descriptor and one status descriptor, instead of two each.

### Changes

Fix the 

```
func buildDescriptorsFromAnnotations(annotations map[string]string) (
	[]olmv1alpha1.SpecDescriptor,
	[]olmv1alpha1.StatusDescriptor,
	error, 
``` 
function to append parsed annotations into a list. 

### Testing

Ideally, use of a backing service which has no CSV should validate this.

- [x] Added tests
- [x] The fix was put in https://github.com/redhat-developer/service-binding-operator/pull/367 so that we could test use of backing service objects which had no CSV/CRD. The change has been validated there.



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: @sbose78: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
ci/prow/4.4-unit | 26f16085377a17aea2720c43bc970addf294cd30 | [link](https://prow.svc.ci.openshift.org/view/gcs/origin-ci-test/pr-logs/pull/redhat-developer_service-binding-operator/371/pull-ci-redhat-developer-service-binding-operator-master-4.4-unit/81) | `/test 4.4-unit`

[Full PR test history](https://prow.svc.ci.openshift.org/pr-history?org=redhat-developer&repo=service-binding-operator&pr=371). [Your PR dashboard](https://prow.svc.ci.openshift.org/pr?query=is:pr+state:open+author:sbose78). Please help us cut down on flakes by [linking to](https://github.com/kubernetes/community/blob/master/contributors/devel/flaky-tests.md#filing-issues-for-flaky-tests) an [open issue](https://github.com/redhat-developer/service-binding-operator/issues?q=is:issue+is:open) when you hit one in your PR.

<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. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
sbose78: /retest
codecov[bot]: # [Codecov](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/371?src=pr&el=h1) Report
> :exclamation: No coverage uploaded for pull request base (`master@58ea511`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference#section-missing-base-commit).
> The diff coverage is `100%`.

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

```diff
@@            Coverage Diff            @@
##             master     #371   +/-   ##
=========================================
  Coverage          ?   63.93%           
=========================================
  Files             ?       21           
  Lines             ?     1389           
  Branches          ?        0           
=========================================
  Hits              ?      888           
  Misses            ?      374           
  Partials          ?      127
```


| [Impacted Files](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/371?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/controller/servicebindingrequest/olm.go](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/371/diff?src=pr&el=tree#diff-cGtnL2NvbnRyb2xsZXIvc2VydmljZWJpbmRpbmdyZXF1ZXN0L29sbS5nbw==) | `68.39% <100%> (ø)` | |

------

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

Avni-Sharma: /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/371#issuecomment-597197662" title="Approved">Avni-Sharma</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)~~ [Avni-Sharma]

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/olm.go b/pkg/controller/servicebindingrequest/olm.go
index c48ab3f2..460ed192 100644
--- a/pkg/controller/servicebindingrequest/olm.go
+++ b/pkg/controller/servicebindingrequest/olm.go
@@ -192,18 +192,12 @@ func buildCRDDescriptionFromCRD(crd *unstructured.Unstructured) (*olmv1alpha1.CR
 		return nil, err
 	}
 
-	specDescriptor, statusDescriptor, err := buildDescriptorsFromAnnotations(crd.GetAnnotations())
+	specDescriptors, statusDescriptors, err := buildDescriptorsFromAnnotations(crd.GetAnnotations())
 	if err != nil {
 		return nil, err
 	}
-
-	if specDescriptor != nil {
-		crdDescription.SpecDescriptors = append(crdDescription.SpecDescriptors, *specDescriptor)
-	}
-
-	if statusDescriptor != nil {
-		crdDescription.StatusDescriptors = append(crdDescription.StatusDescriptors, *statusDescriptor)
-	}
+	crdDescription.SpecDescriptors = append(crdDescription.SpecDescriptors, specDescriptors...)
+	crdDescription.StatusDescriptors = append(crdDescription.StatusDescriptors, statusDescriptors...)
 
 	return crdDescription, nil
 }
@@ -211,12 +205,12 @@ func buildCRDDescriptionFromCRD(crd *unstructured.Unstructured) (*olmv1alpha1.CR
 // buildDescriptorsFromAnnotations builds two descriptors collection, one for spec descriptors and
 // another for status descriptors.
 func buildDescriptorsFromAnnotations(annotations map[string]string) (
-	*olmv1alpha1.SpecDescriptor,
-	*olmv1alpha1.StatusDescriptor,
+	[]olmv1alpha1.SpecDescriptor,
+	[]olmv1alpha1.StatusDescriptor,
 	error,
 ) {
-	var currentSpecDescriptor *olmv1alpha1.SpecDescriptor
-	var currentStatusDescriptor *olmv1alpha1.StatusDescriptor
+	var specDescriptors []olmv1alpha1.SpecDescriptor
+	var statusDescriptors []olmv1alpha1.StatusDescriptor
 
 	acc := make(map[string][]string)
 
@@ -250,19 +244,19 @@ func buildDescriptorsFromAnnotations(annotations map[string]string) (
 		sort.Strings(descriptors)
 		path := strings.Split(fieldPath, ".")
 		if path[0] == "status" {
-			currentStatusDescriptor = &olmv1alpha1.StatusDescriptor{
+			statusDescriptors = append(statusDescriptors, olmv1alpha1.StatusDescriptor{
 				Path:         path[1],
 				XDescriptors: descriptors,
-			}
+			})
 		} else if path[0] == "spec" {
-			currentSpecDescriptor = &olmv1alpha1.SpecDescriptor{
+			specDescriptors = append(specDescriptors, olmv1alpha1.SpecDescriptor{
 				Path:         path[1],
 				XDescriptors: descriptors,
-			}
+			})
 		}
 	}
 
-	return currentSpecDescriptor, currentStatusDescriptor, nil
+	return specDescriptors, statusDescriptors, nil
 }
 
 // extractGVKs loop owned objects and extract the GVK information from them.


Test patch:
diff --git a/pkg/controller/servicebindingrequest/olm_test.go b/pkg/controller/servicebindingrequest/olm_test.go
index 8997454d..53c64398 100644
--- a/pkg/controller/servicebindingrequest/olm_test.go
+++ b/pkg/controller/servicebindingrequest/olm_test.go
@@ -6,6 +6,7 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/require"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
 	logf "sigs.k8s.io/controller-runtime/pkg/runtime/log"
@@ -75,3 +76,58 @@ func TestOLMNew(t *testing.T) {
 		assertGVKs(t, gvks)
 	})
 }
+
+func TestAnnotationParsing(t *testing.T) {
+	annotations := map[string]interface{}{
+		"servicebindingoperator.redhat.io/status.dbCredentials-db.password": "binding:env:object:secret",
+		"servicebindingoperator.redhat.io/spec.dbName":                      "binding:env:attribute",
+		"servicebindingoperator.redhat.io/spec.image":                       "binding:env:attribute",
+		"servicebindingoperator.redhat.io/status.dbConfigMap-db.host":       "binding:env:object:configmap",
+	}
+	crd := &unstructured.Unstructured{
+		Object: map[string]interface{}{
+			"apiVersion": "apiextensions.k8s.io/v1beta1",
+			"kind":       "CustomResourceDefinition",
+			"metadata":   map[string]interface{}{},
+			"spec": map[string]interface{}{
+				"names": map[string]interface{}{
+					"kind": "Carp",
+				},
+				"group":   "app.dev",
+				"version": "v1",
+			},
+			"status": map[string]interface{}{},
+		},
+	}
+	t.Run("Build CSV from CRD - no annotations", func(t *testing.T) {
+		crdDescription, err := buildCRDDescriptionFromCRD(crd)
+		require.NoError(t, err)
+		require.Len(t, crdDescription.SpecDescriptors, 0)
+		require.Len(t, crdDescription.StatusDescriptors, 0)
+	})
+
+	t.Run("Build CSV from CRD", func(t *testing.T) {
+		crd.Object["metadata"] = map[string]interface{}{
+			"annotations": annotations,
+		}
+		crdDescription, err := buildCRDDescriptionFromCRD(crd)
+		require.NoError(t, err)
+
+		require.Len(t, crdDescription.StatusDescriptors, 2)
+		require.Len(t, crdDescription.SpecDescriptors, 2)
+
+		expected := map[string]string{
+			"dbName":        "binding:env:attribute:spec.dbName",
+			"image":         "binding:env:attribute:spec.image",
+			"dbCredentials": "binding:env:object:secret:db.password",
+			"dbConfigMap":   "binding:env:object:configmap:db.host",
+		}
+		for _, value := range crdDescription.SpecDescriptors {
+			require.Equal(t, expected[value.Path], value.XDescriptors[0])
+		}
+
+		for _, value := range crdDescription.StatusDescriptors {
+			require.Equal(t, expected[value.Path], value.XDescriptors[0])
+		}
+	})
+}