validator: prevent multiple filters/predicates per yaml item #2691
Conversation
Signed-off-by: Mustafa Abdelrahman <[email protected]>
| filters, err := eskip.ParseFilters(f) | ||
| if len(filters) > 1 { | ||
| errs = append(errs, errMultipleFilters) | ||
| } |
There was a problem hiding this comment.
I think we can check len(filters) == 1 and make error message like "single filter expected".
This would cover both chain and empty string case (not sure if ParseFilters fails on empty string or not).
I also think we should not mention json/yaml in the error message as it is ambiguous and also routegroup could be created programmatically.
There was a problem hiding this comment.
for empty string we already validate with https://github.com/zalando/skipper/blob/master/dataclients/kubernetes/definitions/routegroupvalidator.go#L134-L140
There was a problem hiding this comment.
Since we check empty first > 1 makes more sense because if the filter was invalid, ParseFilters will return empty filter which will then push 2 errors (parsing & single)
There was a problem hiding this comment.
Then maybe we should move empty check here to have filter validation in one place.
Note that value might also contain only whitespaces " " which would produce zero filters but will not be empty - currently we do not catch this case. Here parse would correctly handle empty value like that
| @@ -56,7 +63,10 @@ func (rgv *RouteGroupValidator) filtersValidation(item *RouteGroupItem) error { | |||
| var errs []error | |||
| for _, r := range item.Spec.Routes { | |||
| for _, f := range r.Filters { | |||
| _, err := eskip.ParseFilters(f) | |||
| filters, err := eskip.ParseFilters(f) | |||
| if len(filters) > 1 { | |||
There was a problem hiding this comment.
I think we should check number of filters only when err is nil.
There was a problem hiding this comment.
Why? I think we should send both errors to decrease roundtrips of fixes
There was a problem hiding this comment.
Lets add a test case for empty value, maybe eskip.Parse already handles it
There was a problem hiding this comment.
there is one already
- https://github.com/zalando/skipper/blob/master/dataclients/kubernetes/definitions/testdata/validation/route-with-empty-filter.yaml
- https://github.com/zalando/skipper/blob/master/dataclients/kubernetes/definitions/testdata/validation/route-with-empty-predicate.yaml
- https://github.com/zalando/skipper/blob/master/eskip/eskip_test.go#L328-L407
There was a problem hiding this comment.
without validation ParseFilter/predicate return no error for empty string
Signed-off-by: Mustafa Abdelrahman <[email protected]>
Signed-off-by: Mustafa Abdelrahman <[email protected]>
| errMultipleFilters = errors.New("single filter expected") | ||
| errMultiplePredicates = errors.New("single predicate expected") |
There was a problem hiding this comment.
Since we use this for both no filter and multiple filters I think it should be named like errSingleFilterExpected
Signed-off-by: Mustafa Abdelrahman <[email protected]>
Signed-off-by: Mustafa Abdelrahman <[email protected]>
|
👍 |
1 similar comment
|
👍 |
* Don't allow more than one filter/predicate per yaml/json item Signed-off-by: Mustafa Abdelrahman <[email protected]>
* Don't allow more than one filter/predicate per yaml/json item Signed-off-by: Mustafa Saber <[email protected]>
* Don't allow more than one filter/predicate per yaml/json item Signed-off-by: Mustafa Abdelrahman <[email protected]>
MustafaSaber
changed the title
MustafaSaber
changed the title
* Don't allow more than one filter/predicate per yaml/json item Signed-off-by: Mustafa Abdelrahman <[email protected]>
* Don't allow more than one filter/predicate per yaml/json item Signed-off-by: Mustafa Abdelrahman <[email protected]>
* Don't allow more than one filter/predicate per yaml/json item Signed-off-by: Mustafa Abdelrahman <[email protected]>
Related to #1618.