From 139a4b8326505cc0b0802e7b7fb96653d3bf90e3 Mon Sep 17 00:00:00 2001 From: Igor Sutton Date: Wed, 21 Jul 2021 15:35:47 +0200 Subject: [PATCH] Fix crash when annotation's sourceValue is invalid (#967) 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 --- .../pipeline/handler/collect/impl.go | 15 ++++++- .../pipeline/handler/collect/impl_test.go | 32 ++++++++++++++ .../bindAppToServiceAnnotations.feature | 42 +++++++++++++++++++ test/acceptance/resources/backend_crd.yaml | 9 ++++ 4 files changed, 97 insertions(+), 1 deletion(-) diff --git a/pkg/reconcile/pipeline/handler/collect/impl.go b/pkg/reconcile/pipeline/handler/collect/impl.go index 5b34a5f761..587ea6b1b5 100644 --- a/pkg/reconcile/pipeline/handler/collect/impl.go +++ b/pkg/reconcile/pipeline/handler/collect/impl.go @@ -18,6 +18,7 @@ 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" @@ -25,6 +26,8 @@ const ( 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++ { diff --git a/pkg/reconcile/pipeline/handler/collect/impl_test.go b/pkg/reconcile/pipeline/handler/collect/impl_test.go index d56bbb9692..e85520d2f8 100644 --- a/pkg/reconcile/pipeline/handler/collect/impl_test.go +++ b/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 diff --git a/test/acceptance/features/bindAppToServiceAnnotations.feature b/test/acceptance/features/bindAppToServiceAnnotations.feature index a0c1bae659..cef30efcd6 100644 --- a/test/acceptance/features/bindAppToServiceAnnotations.feature +++ b/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" + diff --git a/test/acceptance/resources/backend_crd.yaml b/test/acceptance/resources/backend_crd.yaml index ff5ab970af..475494db5e 100644 --- a/test/acceptance/resources/backend_crd.yaml +++ b/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: