Implement virtualService delegate #22118
Conversation
googlebot
added
the
cla: yes
istio-testing
added
needs-rebase
hzxuzhonghu
force-pushed
the
vs-delegate
branch
from
ed30b7a to
c4bc553
Compare
istio-testing
removed
the
needs-rebase
|
@rshriram @howardjohn @nrjpoddar @idouba @dreadbird During the implement, I found that this is not so hard if every point is documented. |
|
ping @rshriram @howardjohn @costinm @linsun may be interested |
pilot/pkg/model/virtualservice.go
Outdated
| for _, subRoute := range delegate { | ||
| // suppose there are N1 match conditions in root, N2 match conditions in delegate | ||
| // if match condition of N2 is a subset of anyone in N1, this is a valid matching conditions | ||
| merged, conflict := mergeHTTPMatchRequests(root.Match, subRoute.Match) |
There was a problem hiding this comment.
We should have an istioctl analyzer for this?
There was a problem hiding this comment.
I would add a TODO. Once this API is approved, i will add it
pilot/pkg/model/virtualservice.go
Outdated
| if subRoute.MirrorPercentage == nil { | ||
| subRoute.MirrorPercentage = root.MirrorPercentage | ||
| } | ||
| if subRoute.CorsPolicy == nil { |
There was a problem hiding this comment.
any way we can ensure new fields are added here?
There was a problem hiding this comment.
If we can introduce fuzz test, then it can be used to intercept new fields unset here.
There was a problem hiding this comment.
Can we create a test and assert the current field count if possible using reflection? If a new field is added, that test will fail and we can fix it?
There was a problem hiding this comment.
added fuzz test, could help in some degree
istio-testing
added
the
needs-rebase
hzxuzhonghu
force-pushed
the
vs-delegate
branch
from
b49fd94 to
b02612b
Compare
istio-testing
added
needs-rebase
pilot/pkg/model/virtualservice.go
Outdated
| out := make([]*networking.HTTPRoute, 0, len(delegate)) | ||
| for _, subRoute := range delegate { | ||
| // suppose there are N1 match conditions in root, N2 match conditions in delegate | ||
| // if match condition of N2 is a subset of anyone in N1, this is a valid matching conditions |
There was a problem hiding this comment.
doesnt that beat the purpose of delegation? The owner of root VS simply says /api/products should be delegated to vs1. vs1 can have any number of rules like /api/products/foo + few header matches. Did you mean all conditions in N1 must be a subset of N2 ?
There was a problem hiding this comment.
It is that all conditions of the delegate must be a subset of any of root's. Because now all the conditions logic is or, if anyone of the condition matched, it is routable.
istio-testing
added
needs-rebase
hzxuzhonghu
force-pushed
the
vs-delegate
branch
2 times, most recently
from
0ea087c to
05c799b
Compare
istio-testing
added
the
needs-rebase
There was a problem hiding this comment.
LGTM. would prefer to have a simple integration test for this working end to end. Should be as simple as adding a case to
Since its a big PR and has had a lot of back and forth i'll give @ramaraochavali and @rshriram a chance to take another look. If not I can approve
| @@ -2044,7 +2050,7 @@ var ValidateVirtualService = registerValidateFunc("ValidateVirtualService", | |||
| errs = appendErrors(errs, errors.New("http, tcp or tls must be provided in virtual service")) | |||
| } | |||
| for _, httpRoute := range virtualService.Http { | |||
| errs = appendErrors(errs, validateHTTPRoute(httpRoute)) | |||
| errs = appendErrors(errs, validateHTTPRoute(httpRoute, isDelegate)) | |||
There was a problem hiding this comment.
You also need to run another check: ensure that the delegation is applied only when the virtual service is bound to gateways and not to the default mesh gateway (i.e. sidecars) - as we have not fully figured out the semantics of the delegation at sidecars.
There was a problem hiding this comment.
As you said #22118 (comment)
If we require the gateways empty, there is no way to figure out here
|
@rshriram I believe most comments are addressed |
|
@hzxuzhonghu can you demo this functionality in today’s Networking WG meeting? I would love to see this in action. Thanks! |
|
@nrjpoddar would be glad to do that, but I haven't prepared a demo. We have deployed it for a production user. |
|
I see, well if you can before 9 AM MST good, or else let’s try for next week. Thanks! |
|
Sounds good |
| delegate.Match = merged | ||
|
|
||
| if delegate.Name == "" { | ||
| delegate.Name = root.Name |
There was a problem hiding this comment.
sorry one last nit.. this will still result in duplicate route names right?
|
/retest |
|
integ-telemetry-k8s-tests_istio stuck /test all |
For more context: #6070
And proposal
The API: istio/api#1209
TODO: add istioctl analysis