Fix crash when annotation's sourceValue is invalid (#967) (#1015)

A service can have a sliceOfMaps-based element annotation.  For example:

    service.binding/java-maven_port: path={.spec.ports},elementType=sliceOfMaps,sourceKey=name,sourceValue=asdf

When sourceValue is incorrect, the operator would panic, since it didn't
verify that the sourceValue was incorrect.  Now, it sets an error
condition in the reconciler (which propogates to the binding status).

Signed-off-by: Andy Sadler <[email protected]>

Co-authored-by: Igor Sutton <[email protected]>

Author: sadlerap

pkg/reconcile/pipeline/handler/collect/impl.go

@@ -18,13 +18,16 @@ import (
 )
 
 var DataNotMap = errors.New("Returned data are not a map, skip collecting")
+var ErrorValueNotFound = errors.New("Value not found in map")
 
 const (
 	ErrorReadingServicesReason   = "ErrorReadingServices"
 	ErrorReadingCRD              = "ErrorReadingCRD"
 	ErrorReadingDescriptorReason = "ErrorReadingDescriptor"
 	ErrorReadingBindingReason    = "ErrorReadingBinding"
 	ErrorReadingSecret           = "ErrorReadingSecret"
+
+	ValueNotFound = "ValueNotFound"
 )
 
 func PreFlight(ctx pipeline.Context) {
@@ -249,7 +252,17 @@ func collectItems(prefix string, ctx pipeline.Context, service pipeline.Service,
 			p = ""
 		}
 		for _, n := range v.MapKeys() {
-			collectItems(p, ctx, service, n, v.MapIndex(n).Interface())
+			if mapVal := v.MapIndex(n).Interface(); mapVal != nil {
+				collectItems(p, ctx, service, n, mapVal)
+			} else {
+				condition := apis.Conditions().NotCollectionReady().
+					Msg(fmt.Sprintf("Value for key %v_%v not found", prefix+k.String(), n.String())).
+					Reason(ValueNotFound).Build()
+				ctx.SetCondition(condition)
+				ctx.Error(ErrorValueNotFound)
+				ctx.StopProcessing()
+				return
+			}
 		}
 	case reflect.Slice:
 		for i := 0; i < v.Len(); i++ {

pkg/reconcile/pipeline/handler/collect/impl_test.go

@@ -388,6 +388,38 @@ var _ = Describe("Collect Binding Data", func() {
 			shouldRetry(pipeline.HandlerFunc(collect.BindingItems), collect.ErrorReadingBindingReason, err)
 		})
 
+		Context("on nil value in collected bindings", func() {
+			It("should set an error condition and stop processing the pipeline",
+				func() {
+					serviceResource := &unstructured.Unstructured{}
+
+					ctx := mocks.NewMockContext(mockCtrl)
+					service := mocks.NewMockService(mockCtrl)
+					definition := bindingmocks.NewMockDefinition(mockCtrl)
+					value := bindingmocks.NewMockValue(mockCtrl)
+
+					ctx.EXPECT().Services().Return([]pipeline.Service{service}, nil)
+
+					bindingDefs := []binding.Definition{definition}
+					service.EXPECT().BindingDefs().Return(bindingDefs)
+					service.EXPECT().Resource().Return(serviceResource)
+
+					definition.EXPECT().Apply(serviceResource).Return(value, nil)
+					value.EXPECT().Get().Return(map[string]map[string]interface{}{"java-maven_port": {"foo": nil}})
+
+					ctx.EXPECT().SetCondition(
+						apis.Conditions().NotCollectionReady().
+							Reason(collect.ValueNotFound).
+							Msg("Value for key java-maven_port_foo not found").
+							Build())
+					ctx.EXPECT().Error(collect.ErrorValueNotFound)
+					ctx.EXPECT().StopProcessing()
+
+					collect.BindingItems(ctx)
+				},
+			)
+		})
+
 		Context("on returning unexpected data", func() {
 			var (
 				service *mocks.MockService

test/acceptance/features/bindAppToServiceAnnotations.feature

@@ -542,3 +542,45 @@ Feature: Bind an application to a service using annotations
             """
         Then Service Binding "binding-app-via-gvk" is ready
         And The application env var "BACKEND_HOST_INTERNAL_DB" has value "internal.db.stable.example.com"
+
+    Scenario: Application cannot be bound to service containing annotation with an invalid sourceValue value
+        Given Generic test application is running
+        And CustomResourceDefinition backends.stable.example.com is available
+        And The Custom Resource is present
+            """
+            apiVersion: stable.example.com/v1
+            kind: Backend
+            metadata:
+                name: $scenario_id
+                annotations:
+                    service.binding/webarrows: path={.spec.connections},elementType=sliceOfMaps,sourceKey=type,sourceValue=asdf
+            spec:
+                connections:
+                  - type: primary
+                    url: primary.example.com
+            status:
+                somestatus: good
+            """
+        When Service Binding is applied
+            """
+            apiVersion: binding.operators.coreos.com/v1alpha1
+            kind: ServiceBinding
+            metadata:
+                name: $scenario_id
+            spec:
+                bindAsFiles: false
+                services:
+                  - group: stable.example.com
+                    version: v1
+                    kind: Backend
+                    name: $scenario_id
+                application:
+                    name: $scenario_id
+                    group: apps
+                    version: v1
+                    resource: deployments
+            """
+        Then Service Binding CollectionReady.status is "False"
+        And Service Binding CollectionReady.reason is "ValueNotFound"
+        And Service Binding CollectionReady.message is "Value for key webarrows_primary not found"
+

test/acceptance/resources/backend_crd.yaml

@@ -44,6 +44,15 @@ spec:
                       type: string
                     environment:
                       type: string
+                connections:
+                  type: array
+                  items:
+                    type: object
+                    properties:
+                      type:
+                        type: string
+                      url:
+                        type: string
             status:
               type: object
               properties: