Fix crash when service annotation with sliceOfMaps elements has invalid sourceValue #1015

sadlerap · 2021-09-07T16:15:15Z

Motivation

When using the sliceOfMaps elementType in a service binding annotation, the operator panics when the specified sourceValue does not exist in the underlying maps, although the expected behavior would be to inform the user the binding has an error through conditions.

Changes

This PR changes the current behavior by verifying that the extracted value is present, and reports an error in the service binding in the case that the sourceValue key isn't present.

Replaces #993 and fixes #967.

Testing

See #967 for reproducibility details.

codecov · 2021-09-07T19:46:37Z

Codecov Report

Merging #1015 (ba78d29) into master (b21b536) will increase coverage by 0.25%.
The diff coverage is 100.00%.

❗ Current head ba78d29 differs from pull request most recent head 139a4b8. Consider uploading reports for the commit 139a4b8 to get more accurate results

@@            Coverage Diff             @@
##           master    #1015      +/-   ##
==========================================
+ Coverage   56.62%   56.87%   +0.25%     
==========================================
  Files          28       28              
  Lines        1547     1556       +9     
==========================================
+ Hits          876      885       +9     
  Misses        555      555              
  Partials      116      116

Impacted Files	Coverage Δ
pkg/reconcile/pipeline/handler/collect/impl.go	`82.50% <100.00%> (+1.04%)`	⬆️
apis/binding/v1alpha1/servicebinding_types.go	`35.29% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b21b536...139a4b8. Read the comment docs.

pedjak · 2021-09-08T21:21:33Z

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

+		serviceContent map[string]interface{}
+	}
+
+	It("should set an error condition",


this should be actually added around this line https://github.com/redhat-developer/service-binding-operator/blob/master/pkg/reconcile/pipeline/handler/collect/impl_test.go#L390

The test name should describe the fixture and the expected outcome - "should set an error condition" does not say anything about under what condition and kind of error.

pedjak · 2021-09-08T21:23:12Z

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

+
+	It("should set an error condition",
+		func() {
+			tc := testCase{


is any particular reason why use a new struct here, instead of just defining serviceContent variable directly and using it throughout the test?

Not particularly; there used to be more in this struct, but most of it got removed as a result of refactoring, leaving only this. I'll take it out.

pedjak · 2021-09-08T21:24:15Z

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

+			service := mocks.NewMockService(mockCtrl)
+			serviceResource := &unstructured.Unstructured{Object: tc.serviceContent}
+
+			ctx.EXPECT().Services().Return([]pipeline.Service{service}, nil).MinTimes(1)


MinTimes(1) is default if omitted, hence I would remove it.

We need MinTimes(1) since we're asserting that we call Services() at least once, not exactly once. Default behavior appears to be asserting that it gets called exactly once.

well, I do think that we assert that we call it only once, and we will do that when we remove collect.BindingDefinitions(ctx) from the test.

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

pedjak · 2021-09-08T21:38:35Z

test/acceptance/features/bindAppToServiceAnnotations.feature

@@ -542,3 +542,49 @@ 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


the scenario description suggests that the binding is performed, but actually it is not. Hence, we should reflect that, e.g.

"Application cannot be bound to service containing annotation with an invalid sourceValue value"

Fixed in 6054ca0.

pedjak · 2021-09-08T21:42:10Z

test/acceptance/features/bindAppToServiceAnnotations.feature

+            """
+        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"


acceptance tests are failing on CI on this assertion, please double check what is going on.

I've looked into the artifacts of the failing without-olm job and found that this step is failing because the message is actually Value for key webarrows_secondary not found:

$ cat servicebindings.binding.operators.coreos.com.yaml| yq -r '.items[] | select(.metadata.name == "bindapptoserviceannotations-546").status.conditions[] | select(.type == "CollectionReady")' { "lastTransitionTime": "2021-09-08T17:15:27Z", "message": "Value for key webarrows_secondary not found", "reason": "ValueNotFound", "status": "False", "type": "CollectionReady" }

For comparison by checking the artifacts of the passed with-olm job the message is correct:

$ cat servicebindings.binding.operators.coreos.com.yaml| yq -r '.items[] | select(.metadata.name == "bindapptoserviceannotations-546").status.conditions[] | select(.type == "CollectionReady")' { "lastTransitionTime": "2021-09-08T17:15:50Z", "message": "Value for key webarrows_primary not found", "reason": "ValueNotFound", "status": "False", "type": "CollectionReady" }

It looks like whatever is going through .spec.connections map keys does so in a different (perhaps random) order for each of the above cases

sadlerap · 2021-09-09T20:47:16Z

/retest

pedjak · 2021-09-10T09:32:35Z

/retest

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

pedjak · 2021-09-09T16:57:04Z

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

+			service := mocks.NewMockService(mockCtrl)
+			serviceResource := &unstructured.Unstructured{Object: tc.serviceContent}
+
+			ctx.EXPECT().Services().Return([]pipeline.Service{service}, nil).MinTimes(1)


well, I do think that we assert that we call it only once, and we will do that when we remove collect.BindingDefinitions(ctx) from the test.

pedjak

looks good, apart from some comments on the introduced unit test. A few simplifications/test name improvements should be made to improve readability and maintainability. If this can be done quickly, I would suggest to do it then within this PR, otherwise, we should perform in the followup PR.

pedjak · 2021-09-10T10:01:33Z

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

+					service.EXPECT().BindingDefs().DoAndReturn(func() []binding.Definition { return bindingDefs })
+					service.EXPECT().Resource().Return(serviceResource)
+
+					definition.EXPECT().Apply(serviceResource).Return(value, nil)


given that we really mock this call, serviceResource value could be further simplified and reduce even to something like unstructured.Unstructured{}

pedjak · 2021-09-10T10:02:36Z

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

+					ctx.EXPECT().Services().Return([]pipeline.Service{service}, nil)
+
+					bindingDefs := []binding.Definition{definition}
+					service.EXPECT().BindingDefs().DoAndReturn(func() []binding.Definition { return bindingDefs })


these two lines could be collapsed in something like:

service.EXPECT().BindingDefs().Return(bindingDefs)

pedjak · 2021-09-10T10:06:34Z

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

+			var (
+				mockCtrl *gomock.Controller
+			)
+
+			BeforeEach(func() {
+				mockCtrl = gomock.NewController(GinkgoT())
+			})
+
+			AfterEach(func() {
+				mockCtrl.Finish()
+			})


this is not needed because they are already declared in the outer context. With that, we can even remove introduced Context, because it does not contain any additional customizations.

pedjak · 2021-09-10T10:18:44Z

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

+				mockCtrl.Finish()
+			})
+
+			It("should set an error condition",


so, at the end, this test is not about the invalid sourceValue - it is certainly triggered by such annotation from user side, but here we have an unit test, and the trigger is nil value of collected binding. We should reflect that in the test name. Also, the outcome of this tests is not only to set the right error condition, we also indicate that processing should be stopped and not retried at all.

sadlerap · 2021-09-10T15:49:52Z

/retest

) 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 <ansadler@redhat.com>

pedjak · 2021-09-10T16:06:06Z

/lgtm
/approve

openshift-ci · 2021-09-10T16:06:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pedjak

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [pedjak]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

sadlerap · 2021-09-10T18:09:21Z

/retest

openshift-ci bot requested review from isutton and pedjak September 7, 2021 16:15

sadlerap force-pushed the service-binding-operator-967 branch 2 times, most recently from acd21dc to ba78d29 Compare September 7, 2021 18:14

sadlerap changed the title ~~Service binding operator 967~~ Fix crash when sliceOfMaps has invalid value Sep 8, 2021

sadlerap changed the title ~~Fix crash when sliceOfMaps has invalid value~~ Fix crash when service annotation of sliceOfMaps has invalid sourceValue Sep 8, 2021

sadlerap changed the title ~~Fix crash when service annotation of sliceOfMaps has invalid sourceValue~~ Fix crash when service annotation with sliceOfMaps elements has invalid sourceValue Sep 8, 2021

sadlerap force-pushed the service-binding-operator-967 branch from ba78d29 to ead28b4 Compare September 8, 2021 16:58

pedjak suggested changes Sep 8, 2021

View reviewed changes

pedjak removed the request for review from isutton September 9, 2021 07:44

sadlerap force-pushed the service-binding-operator-967 branch 2 times, most recently from 4d1d13f to 6054ca0 Compare September 9, 2021 20:19

pedjak reviewed Sep 10, 2021

View reviewed changes

pedjak approved these changes Sep 10, 2021

View reviewed changes

sadlerap force-pushed the service-binding-operator-967 branch from 6054ca0 to 9219fbe Compare September 10, 2021 14:09

sadlerap force-pushed the service-binding-operator-967 branch from 9219fbe to 139a4b8 Compare September 10, 2021 16:00

openshift-ci bot assigned pedjak Sep 10, 2021

openshift-ci bot added the lgtm label Sep 10, 2021

openshift-ci bot added the approved label Sep 10, 2021

openshift-merge-robot merged commit 252987b into redhat-developer:master Sep 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix crash when service annotation with sliceOfMaps elements has invalid sourceValue #1015

Fix crash when service annotation with sliceOfMaps elements has invalid sourceValue #1015

sadlerap commented Sep 7, 2021 •

edited

Loading

codecov bot commented Sep 7, 2021 •

edited

Loading

pedjak Sep 8, 2021

pedjak Sep 8, 2021

sadlerap Sep 9, 2021

pedjak Sep 8, 2021

sadlerap Sep 9, 2021

pedjak Sep 9, 2021

pedjak Sep 8, 2021

sadlerap Sep 9, 2021

pedjak Sep 8, 2021

pmacik Sep 9, 2021

pmacik Sep 9, 2021 •

edited

Loading

sadlerap commented Sep 9, 2021

pedjak commented Sep 10, 2021

pedjak Sep 9, 2021

pedjak left a comment

pedjak Sep 10, 2021

pedjak Sep 10, 2021

pedjak Sep 10, 2021

pedjak Sep 10, 2021

sadlerap commented Sep 10, 2021

pedjak commented Sep 10, 2021

openshift-ci bot commented Sep 10, 2021

sadlerap commented Sep 10, 2021

Fix crash when service annotation with sliceOfMaps elements has invalid sourceValue #1015

Fix crash when service annotation with sliceOfMaps elements has invalid sourceValue #1015

Conversation

sadlerap commented Sep 7, 2021 • edited Loading

Motivation

Changes

Testing

codecov bot commented Sep 7, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmacik Sep 9, 2021 • edited Loading

Choose a reason for hiding this comment

sadlerap commented Sep 9, 2021

pedjak commented Sep 10, 2021

Choose a reason for hiding this comment

pedjak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sadlerap commented Sep 10, 2021

pedjak commented Sep 10, 2021

openshift-ci bot commented Sep 10, 2021

sadlerap commented Sep 10, 2021

sadlerap commented Sep 7, 2021 •

edited

Loading

codecov bot commented Sep 7, 2021 •

edited

Loading

pmacik Sep 9, 2021 •

edited

Loading