virtualservice delegation #1121
Conversation
|
Issues linked to changelog: |
| var ( | ||
| missingPrefixErr = errors.Errorf("invalid route: routes with delegate actions must specify a prefix matcher") | ||
| hasHeaderMatcherErr = errors.Errorf("invalid route: routes with delegate actions cannot use header matchers") | ||
| hasMethodMatcherErr = errors.Errorf("invalid route: routes with delegate actions cannot use header matchers") |
There was a problem hiding this comment.
| hasMethodMatcherErr = errors.Errorf("invalid route: routes with delegate actions cannot use header matchers") | |
| hasMethodMatcherErr = errors.Errorf("invalid route: routes with delegate actions cannot use method matchers") |
| missingPrefixErr = errors.Errorf("invalid route: routes with delegate actions must specify a prefix matcher") | ||
| hasHeaderMatcherErr = errors.Errorf("invalid route: routes with delegate actions cannot use header matchers") | ||
| hasMethodMatcherErr = errors.Errorf("invalid route: routes with delegate actions cannot use header matchers") | ||
| hasQueryMatcherErr = errors.Errorf("invalid route: routes with delegate actions cannot use header matchers") |
There was a problem hiding this comment.
| hasQueryMatcherErr = errors.Errorf("invalid route: routes with delegate actions cannot use header matchers") | |
| hasQueryMatcherErr = errors.Errorf("invalid route: routes with delegate actions cannot use query matchers") |
| func (rv *routeVisitor) convertDelegateAction(ours *v1.Route) ([]*gloov1.Route, error) { | ||
| action := ours.GetDelegateAction() | ||
| if action == nil { | ||
| return nil, errors.Errorf("internal error: convertDelegateAction() called on route without delegate action") |
There was a problem hiding this comment.
Add error to the error vars above.
| } | ||
| for _, visited := range rv.visited { | ||
| if routeTable == visited { | ||
| return nil, errors.Errorf("invalid route: delegation cycle detected") |
There was a problem hiding this comment.
Same as above. Maybe you could also log more information about which route tables caused the cycle.
| } | ||
| prefix = "/" + strings.Trim(prefix, "/") | ||
|
|
||
| if len(ours.GetMatcher().GetHeaders()) > 0 { |
There was a problem hiding this comment.
Nit: I think moving all ours.GetMatcher() the validation logic to a separate function would make this one more readable.
| @@ -1,17 +1,17 @@ | |||
| package translator_test | |||
| package translator | |||
There was a problem hiding this comment.
As a general principle I like to keep the isolation that separate packages provide. We test against an interface and don't have to update any test code when implementation details that should be transparent to clients change. It also surfaces limitations in the interface itself.
It looks like this change was done to get access to:
- the error variables
- the visitor
For the first, I don't see any harm in making them public. Regarding the second one, I see why you wouldn't want to expose the visitor to the outside. How about moving it to its own file and having a separate unit test for it, e.g. route_visitor.go + route_visitor_test.go?
| }, | ||
| }, | ||
| }, | ||
| expectedErr: missingPrefixErr, |
There was a problem hiding this comment.
I suspect there may be some copy/paste errors here and that if you fix the duplicate error messages this may fail. Worth to double-check.
There was a problem hiding this comment.
these are correct, same error for both the regex and exact path cases
|
/kick it appears is still a flake |
BOT NOTES:
resolves #1120
resolves #1122