-
Notifications
You must be signed in to change notification settings - Fork 91
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
acd21dc
to
ba78d29
Compare
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
ba78d29
to
ead28b4
Compare
serviceContent map[string]interface{} | ||
} | ||
|
||
It("should set an error condition", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
||
It("should set an error condition", | ||
func() { | ||
tc := testCase{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is any particular reason why use a new struct here, instead of just defining serviceContent
variable directly and using it throughout the test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
service := mocks.NewMockService(mockCtrl) | ||
serviceResource := &unstructured.Unstructured{Object: tc.serviceContent} | ||
|
||
ctx.EXPECT().Services().Return([]pipeline.Service{service}, nil).MinTimes(1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MinTimes(1)
is default if omitted, hence I would remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 6054ca0.
""" | ||
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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
acceptance tests are failing on CI on this assertion, please double check what is going on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
4d1d13f
to
6054ca0
Compare
/retest |
1 similar comment
/retest |
service := mocks.NewMockService(mockCtrl) | ||
serviceResource := &unstructured.Unstructured{Object: tc.serviceContent} | ||
|
||
ctx.EXPECT().Services().Return([]pipeline.Service{service}, nil).MinTimes(1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
service.EXPECT().BindingDefs().DoAndReturn(func() []binding.Definition { return bindingDefs }) | ||
service.EXPECT().Resource().Return(serviceResource) | ||
|
||
definition.EXPECT().Apply(serviceResource).Return(value, nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
given that we really mock this call, serviceResource
value could be further simplified and reduce even to something like unstructured.Unstructured{}
ctx.EXPECT().Services().Return([]pipeline.Service{service}, nil) | ||
|
||
bindingDefs := []binding.Definition{definition} | ||
service.EXPECT().BindingDefs().DoAndReturn(func() []binding.Definition { return bindingDefs }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these two lines could be collapsed in something like:
service.EXPECT().BindingDefs().Return(bindingDefs)
var ( | ||
mockCtrl *gomock.Controller | ||
) | ||
|
||
BeforeEach(func() { | ||
mockCtrl = gomock.NewController(GinkgoT()) | ||
}) | ||
|
||
AfterEach(func() { | ||
mockCtrl.Finish() | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
mockCtrl.Finish() | ||
}) | ||
|
||
It("should set an error condition", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
6054ca0
to
9219fbe
Compare
/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>
9219fbe
to
139a4b8
Compare
/lgtm |
[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:
Approvers can indicate their approval by writing |
/retest |
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.