From 4d1d13f36ece523757dbb2ce0f001cd4c8cb0f2c 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 | 56 +++++++++++++++++++ .../bindAppToServiceAnnotations.feature | 42 ++++++++++++++ test/acceptance/resources/backend_crd.yaml | 9 +++ 4 files changed, 121 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..9a50f061c3 100644 --- a/pkg/reconcile/pipeline/handler/collect/impl_test.go +++ b/pkg/reconcile/pipeline/handler/collect/impl_test.go @@ -388,6 +388,62 @@ var _ = Describe("Collect Binding Data", func() { shouldRetry(pipeline.HandlerFunc(collect.BindingItems), collect.ErrorReadingBindingReason, err) }) + Context("on invalid sourceValue in sliceOfMap elements", func() { + var ( + mockCtrl *gomock.Controller + ) + + BeforeEach(func() { + mockCtrl = gomock.NewController(GinkgoT()) + }) + + AfterEach(func() { + mockCtrl.Finish() + }) + + It("should set an error condition", + func() { + serviceContent := map[string]interface{}{ + "metadata": map[string]interface{}{ + "annotations": map[string]interface{}{ + "service.binding/java-maven_port": "path={.status.ports},elementType=sliceOfMaps,sourceKey=name,sourceValue=asdf", + }, + }, + "status": map[string]interface{}{ + "ports": []interface{}{ + map[string]interface{}{"name": "foo", "value": "bar"}, + }, + }, + } + + ctx := mocks.NewMockContext(mockCtrl) + service := mocks.NewMockService(mockCtrl) + serviceResource := &unstructured.Unstructured{Object: serviceContent} + definition := bindingmocks.NewMockDefinition(mockCtrl) + value := bindingmocks.NewMockValue(mockCtrl) + + ctx.EXPECT().Services().Return([]pipeline.Service{service}, nil) + + bindingDefs := []binding.Definition{definition} + service.EXPECT().BindingDefs().DoAndReturn(func() []binding.Definition { 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..dd53417245 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: Bind application to service with invalid sourceValue annotations + 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: