Repository: redhat-developer/service-binding-operator
Pull request #1032 (2021-09-27 09:05:36)

handle panics occurring in reconsile pipeline gracefuly
If a panic happens while reconciling service binding, put the binding back in the queue and retry again

Fixes crashing loop reported in #1024 

openshift-ci[bot]: [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 ask for approval from **pedjak** after the PR has been reviewed.

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

Patch:
diff --git a/pkg/reconcile/pipeline/builder/pipeline.go b/pkg/reconcile/pipeline/builder/pipeline.go
index 6828b83b..0ac6fcc7 100644
--- a/pkg/reconcile/pipeline/builder/pipeline.go
+++ b/pkg/reconcile/pipeline/builder/pipeline.go
@@ -1,6 +1,7 @@
 package builder
 
 import (
+	"fmt"
 	"github.com/redhat-developer/service-binding-operator/pkg/reconcile/pipeline"
 	"github.com/redhat-developer/service-binding-operator/pkg/reconcile/pipeline/handler/collect"
 	"github.com/redhat-developer/service-binding-operator/pkg/reconcile/pipeline/handler/mapping"
@@ -15,14 +16,20 @@ type impl struct {
 	handlers    []pipeline.Handler
 }
 
-func (i *impl) Process(binding interface{}) (bool, error) {
+func (i *impl) Process(binding interface{}) (retry bool, err error) {
+	defer func() {
+		if perr := recover(); perr != nil {
+			retry = true
+			err = fmt.Errorf("panic occurred: %v", perr)
+		}
+	}()
 	ctx, err := i.ctxProvider.Get(binding)
 	if err != nil {
 		return false, err
 	}
 	var status pipeline.FlowStatus
 	for _, h := range i.handlers {
-		h.Handle(ctx)
+		invokeHandler(h, ctx)
 		status = ctx.FlowStatus()
 		if status.Stop {
 			break
@@ -61,6 +68,15 @@ func Builder() *builder {
 	return &builder{}
 }
 
+func invokeHandler(h pipeline.Handler, ctx pipeline.Context) {
+	defer func() {
+		if err := recover(); err != nil {
+			ctx.RetryProcessing(fmt.Errorf("panic occurred: %v", err))
+		}
+	}()
+	h.Handle(ctx)
+}
+
 var defaultFlow = []pipeline.Handler{
 	pipeline.HandlerFunc(project.Unbind),
 	pipeline.HandlerFunc(collect.PreFlight),
diff --git a/pkg/reconcile/pipeline/handler/project/impl.go b/pkg/reconcile/pipeline/handler/project/impl.go
index bacdea5b..b25f3e52 100644
--- a/pkg/reconcile/pipeline/handler/project/impl.go
+++ b/pkg/reconcile/pipeline/handler/project/impl.go
@@ -389,7 +389,7 @@ func mountPath(container map[string]interface{}, ctx pipeline.Context) (string,
 	if err != nil {
 		return "", err
 	}
-	envs = append(envs, u)
+
 	if found {
 		container["env"] = append(envs, u)
 	} else {


Test patch:
diff --git a/pkg/reconcile/pipeline/builder/pipeline_test.go b/pkg/reconcile/pipeline/builder/pipeline_test.go
index 530991e4..ecd6b0e1 100644
--- a/pkg/reconcile/pipeline/builder/pipeline_test.go
+++ b/pkg/reconcile/pipeline/builder/pipeline_test.go
@@ -1,6 +1,7 @@
 package builder_test
 
 import (
+	"fmt"
 	"github.com/golang/mock/gomock"
 	"github.com/pkg/errors"
 	"github.com/redhat-developer/service-binding-operator/apis/binding/v1alpha1"
@@ -82,6 +83,27 @@ var _ = Describe("Pipeline", func() {
 		Expect(retry).To(BeTrue())
 	})
 
+	It("should retry processing if panic occurred in a handler", func() {
+		err := fmt.Errorf("panic occurred: %v", "foo")
+
+		h1 := defHandler()
+		h1.EXPECT().Handle(ctx)
+		h2 := func(c pipeline.Context) {
+			panic("foo")
+		}
+		h3 := defHandler()
+		p := builder.Builder().WithContextProvider(&ctxProvider{ctx: ctx}).WithHandlers(h1, pipeline.HandlerFunc(h2), h3).Build()
+
+		ctx.EXPECT().RetryProcessing(err)
+		ctx.EXPECT().Close().Return(nil)
+		ctx.EXPECT().FlowStatus().Return(pipeline.FlowStatus{})
+		ctx.EXPECT().FlowStatus().Return(pipeline.FlowStatus{Retry: true, Stop: true, Err: err})
+
+		retry, rerr := p.Process(&v1alpha1.ServiceBinding{})
+		Expect(rerr).To(Equal(err))
+		Expect(retry).To(BeTrue())
+	})
+
 	It("should stop without retry and error and propagate that back to caller", func() {
 		h1 := defHandler()
 		h1.EXPECT().Handle(ctx)
@@ -116,6 +138,32 @@ var _ = Describe("Pipeline", func() {
 		Expect(err).To(Equal(err))
 		Expect(retry).To(BeTrue())
 	})
+
+	It("should retry processing if panic occurs when closing context", func() {
+		var err = fmt.Errorf("panic occurred: %v", "foo")
+		h1 := defHandler()
+		h1.EXPECT().Handle(ctx)
+		p := builder.Builder().WithContextProvider(&ctxProvider{ctx: ctx}).WithHandlers(h1).Build()
+
+		ctx.EXPECT().Close().DoAndReturn(func() { panic("foo") })
+		ctx.EXPECT().FlowStatus().Return(pipeline.FlowStatus{}).Times(1)
+
+		retry, perr := p.Process(&v1alpha1.ServiceBinding{})
+		Expect(perr).To(Equal(err))
+		Expect(retry).To(BeTrue())
+	})
+
+	It("should retry processing if panic occurs when calling context provider", func() {
+		var err = fmt.Errorf("panic occurred: %v", "foo")
+		h1 := defHandler()
+		provider := mocks.NewMockContextProvider(mockCtrl)
+		provider.EXPECT().Get(&v1alpha1.ServiceBinding{}).DoAndReturn(func(b interface{}) { panic("foo") })
+		p := builder.Builder().WithContextProvider(provider).WithHandlers(h1).Build()
+
+		retry, perr := p.Process(&v1alpha1.ServiceBinding{})
+		Expect(perr).To(Equal(err))
+		Expect(retry).To(BeTrue())
+	})
 })
 
 type ctxProvider struct {
diff --git a/test/acceptance/features/injectBindingsAsFiles.feature b/test/acceptance/features/injectBindingsAsFiles.feature
index ffb29b92..1c4cd1f2 100644
--- a/test/acceptance/features/injectBindingsAsFiles.feature
+++ b/test/acceptance/features/injectBindingsAsFiles.feature
@@ -115,6 +115,7 @@ Feature: Bindings get injected as files in application
             """
             8080
             """
+        And The container declared in application resource contains env "SERVICE_BINDING_ROOT" set only once
 
     Scenario: Binding is injected as file into application at the location specified through mountPath
         Given Generic test application "generic-app-a-d-u-3" is running
@@ -363,3 +364,64 @@ Feature: Bindings get injected as files in application
             """
             mysql
             """
+
+    Scenario: SERVICE_BIDNING_ROOT is not defined twice in the deployment after binding two services
+        Given Generic test application is running
+        * The env var "SERVICE_BINDING_ROOT" is not available to the application
+        * The Service is present
+            """
+            apiVersion: v1
+            kind: Service
+            metadata:
+                name: $scenario_id-2
+                annotations:
+                    service.binding/service2: path={.metadata.name}
+            spec:
+                selector:
+                    name: $scenario_id
+                ports:
+                  - port: 80
+                    targetPort: 8080
+            """
+        * The Service is present
+            """
+            apiVersion: v1
+            kind: Service
+            metadata:
+                name: $scenario_id-3
+                annotations:
+                    service.binding/service3: path={.metadata.name}
+            spec:
+                selector:
+                    name: $scenario_id
+                ports:
+                  - port: 80
+                    targetPort: 8080
+            """
+        When Service Binding is applied
+            """
+            apiVersion: binding.operators.coreos.com/v1alpha1
+            kind: ServiceBinding
+            metadata:
+                name: $scenario_id
+            spec:
+                application:
+                    group: apps
+                    version: v1
+                    resource: deployments
+                    name: $scenario_id
+                bindAsFiles: true
+                detectBindingResources: true
+                services:
+                  - group: ""
+                    version: v1
+                    kind: Service
+                    name: $scenario_id-2
+                  - group: ""
+                    version: v1
+                    kind: Service
+                    name: $scenario_id-3
+            """
+        Then Service Binding is ready
+        * Service Binding has the binding secret name set in the status
+        * The container declared in application resource contains env "SERVICE_BINDING_ROOT" set only once
diff --git a/test/acceptance/features/steps/generic_testapp.py b/test/acceptance/features/steps/generic_testapp.py
index 8cc25cbc..456dfd6c 100644
--- a/test/acceptance/features/steps/generic_testapp.py
+++ b/test/acceptance/features/steps/generic_testapp.py
@@ -3,6 +3,7 @@ import requests
 import json
 import polling2
 from behave import step
+from openshift import Openshift
 from util import scenario_id
 from string import Template
 
@@ -113,3 +114,14 @@ def set_common_label(context, label):
 def check_env_var_value_in_both_apps(context, name, value):
     polling2.poll(lambda: context.application1.get_env_var_value(name) == value, step=5, timeout=400)
     polling2.poll(lambda: context.application2.get_env_var_value(name) == value, step=5, timeout=400)
+
+
+@step(u'The container declared in application resource contains env "{envVar}" set only once')
+@step(u'The container declared in application "{app_name}" resource contains env "{envVar}" set only once')
+def check_env_var_count_set_on_container(context, envVar, app_name=None):
+    openshift = Openshift()
+    if app_name is None:
+        app_name = context.application.name
+    app_name = Template(app_name).substitute(scenario_id=scenario_id(context))
+    env = openshift.get_deployment_env_info(app_name, context.namespace.name)
+    assert str(env).count(envVar) == 1
diff --git a/test/acceptance/features/steps/service_binding.py b/test/acceptance/features/steps/service_binding.py
index d3bb0a91..5e3beb1b 100644
--- a/test/acceptance/features/steps/service_binding.py
+++ b/test/acceptance/features/steps/service_binding.py
@@ -42,7 +42,7 @@ class ServiceBinding(object):
     def get_secret_name(self):
         output = self.get_info_by_jsonpath(self.secretPath)
         assert output is not None, "Failed to fetch secret name from ServiceBinding"
-        return output
+        return output.strip().strip('"')
 
     def delete(self):
         self.openshift.delete(self.yamlContent, self.namespace)
@@ -121,8 +121,13 @@ def validate_persistent_sb(context, sb_name):
 
 
 @step(u'Service Binding "{sbr_name}" has the binding secret name set in the status')
-def sbo_secret_name_has_been_set(context, sbr_name):
-    polling2.poll(lambda: context.bindings[sbr_name].get_info_by_jsonpath(".status.secret") != "",
+@step(u'Service Binding has the binding secret name set in the status')
+def sbo_secret_name_has_been_set(context, sbr_name=None):
+    if sbr_name is None:
+        sbr_name = list(context.bindings.values())[0].name
+    else:
+        sbr_name = Template(sbr_name).substitute(scenario_id=scenario_id(context))
+    polling2.poll(lambda: context.bindings[sbr_name].get_secret_name() != "",
                   step=5, timeout=800,  ignore_exceptions=(json.JSONDecodeError,))
 
 
diff --git a/test/acceptance/openshift-setup.sh b/test/acceptance/openshift-setup.sh
index 3f4326a9..6cc3fad1 100755
--- a/test/acceptance/openshift-setup.sh
+++ b/test/acceptance/openshift-setup.sh
@@ -10,22 +10,7 @@ USER_TOKEN=acceptance-tests-dev
 oc get secret $HTPASSWD_SECRET -n openshift-config >/dev/null 2>&1 \
   || oc create secret generic $HTPASSWD_SECRET --from-file=htpasswd=$HTPASSWD_FILE -n openshift-config
 
-oc apply -f - <<EOF
-apiVersion: config.openshift.io/v1
-kind: OAuth
-metadata:
-  name: cluster
-spec:
-  identityProviders:
-  - name: acceptance-tests
-    challenge: true
-    login: true
-    mappingMethod: claim
-    type: HTPasswd
-    htpasswd:
-      fileData:
-        name: $HTPASSWD_SECRET
-EOF
+oc patch oauth cluster --type merge -p '{"spec":{"identityProviders":[{"htpasswd":{"fileData":{"name":"'$HTPASSWD_SECRET'"}},"mappingMethod":"claim","name":"acceptance-tests","challenge":"true","login":true,"type":"HTPasswd"}]}}'
 
 CURRENT_CTX=$(oc config current-context)