Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement virtualService delegate #22118

Merged
merged 15 commits into from Apr 10, 2020
Merged

Implement virtualService delegate #22118

merged 15 commits into from Apr 10, 2020

Conversation

hzxuzhonghu
Copy link
Member

@hzxuzhonghu hzxuzhonghu commented Mar 12, 2020 •
edited

For more context: #6070
And proposal

The API: istio/api#1209

TODO: add istioctl analysis

@hzxuzhonghu hzxuzhonghu requested review from a team as code owners March 12, 2020 07:43
@googlebot googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Mar 12, 2020
@istio-testing istio-testing added needs-rebase Indicates a PR needs to be rebased before being merged size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 12, 2020
@istio-testing istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Mar 12, 2020
@hzxuzhonghu
Copy link
Member Author

@rshriram @howardjohn @nrjpoddar @idouba @dreadbird During the implement, I found that this is not so hard if every point is documented.

@hzxuzhonghu
Copy link
Member Author

ping @rshriram @howardjohn @costinm @linsun may be interested

howardjohn
howardjohn reviewed Mar 19, 2020
View reviewed changes
pilot/pkg/model/virtualservice.go Outdated Show resolved Hide resolved
pilot/pkg/model/virtualservice.go Outdated Show resolved Hide resolved
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have an istioctl analyzer for this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make sense

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any way we can ensure new fields are added here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we can introduce fuzz test, then it can be used to intercept new fields unset here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added fuzz test, could help in some degree

pilot/pkg/model/virtualservice.go Show resolved Hide resolved
pilot/pkg/model/virtualservice.go Outdated Show resolved Hide resolved
pkg/config/validation/validation.go Outdated Show resolved Hide resolved
@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Mar 20, 2020
@istio-testing istio-testing added needs-rebase Indicates a PR needs to be rebased before being merged and removed needs-rebase Indicates a PR needs to be rebased before being merged labels Mar 20, 2020
rshriram
rshriram reviewed Mar 23, 2020
View reviewed changes
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 istio-testing added needs-rebase Indicates a PR needs to be rebased before being merged and removed needs-rebase Indicates a PR needs to be rebased before being merged labels Mar 23, 2020
@hzxuzhonghu hzxuzhonghu force-pushed the vs-delegate branch 2 times, most recently from 0ea087c to 05c799b Compare March 24, 2020 06:54
@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Mar 29, 2020
howardjohn
howardjohn reviewed Apr 9, 2020
View reviewed changes
Copy link
Member

@howardjohn howardjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. would prefer to have a simple integration test for this working end to end. Should be as simple as adding a case to

istio/tests/integration/pilot/routing_test.go

Line 31 in 3ad98d9

func TestTrafficRouting(t *testing.T) {

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

rshriram
rshriram reviewed Apr 9, 2020
View reviewed changes
pkg/config/validation/validation.go
@@ -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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you said #22118 (comment)

If we require the gateways empty, there is no way to figure out here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant for the root VS.

rshriram
rshriram reviewed Apr 9, 2020
View reviewed changes
Copy link
Member

@rshriram rshriram left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code is mostly LGTM. See few critical nits on route naming, labels merging (source labels). I am okay if they are tackled in a subsequent PR but these need to be tackled for sure.

@hzxuzhonghu
Copy link
Member Author

@rshriram I believe most comments are addressed

@nrjpoddar
Copy link
Member

@hzxuzhonghu can you demo this functionality in today’s Networking WG meeting? I would love to see this in action. Thanks!

@hzxuzhonghu
Copy link
Member Author

@nrjpoddar would be glad to do that, but I haven't prepared a demo. We have deployed it for a production user.

@nrjpoddar
Copy link
Member

I see, well if you can before 9 AM MST good, or else let’s try for next week. Thanks!

@hzxuzhonghu
Copy link
Member Author

Sounds good

rshriram
rshriram reviewed Apr 10, 2020
View reviewed changes
pilot/pkg/model/virtualservice.go
delegate.Match = merged

if delegate.Name == "" {
delegate.Name = root.Name
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry one last nit.. this will still result in duplicate route names right?

@hzxuzhonghu
Copy link
Member Author

/retest

@hzxuzhonghu
Copy link
Member Author

integ-telemetry-k8s-tests_istio stuck

/test all

@istio-testing istio-testing merged commit 8bbee5e into istio:master Apr 10, 2020
@hzxuzhonghu hzxuzhonghu deleted the vs-delegate branch April 10, 2020 03:57
@howardjohn howardjohn mentioned this pull request May 12, 2020
Closed
This was referenced Sep 28, 2020
Closed
Closed
@idouba idouba mentioned this pull request Jan 4, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@howardjohn howardjohn howardjohn left review comments

@nrjpoddar nrjpoddar nrjpoddar left review comments

@ramaraochavali ramaraochavali ramaraochavali left review comments

@rshriram rshriram rshriram approved these changes

@costinm costinm Awaiting requested review from costinm costinm is a code owner

@linsun linsun Awaiting requested review from linsun linsun is a code owner

Assignees
No one assigned
Labels
area/networking area/user experience cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@hzxuzhonghu @howardjohn @nrjpoddar @rshriram @ramaraochavali @googlebot @istio-testing @istio-policy-bot