Repository: redhat-developer/service-binding-operator
Pull request #649 (2020-08-28 06:20:35)

fix issue #648 , service-binding-operator panics if there is a configMapRef in envFrom
To fix issue #648

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 @qibobo. 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>

DhritiShikhar: /ok-to-test
qibobo: @DhritiShikhar fixed.

DhritiShikhar: /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/649#issuecomment-682493715" title="Approved">DhritiShikhar</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)~~ [DhritiShikhar]

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/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(