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.

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

Support Virtual Service route match percentage #3278

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sschepens
Copy link
Contributor

Fixes to istio/istio#39880

Add percentage support in Virtual Service route match, this will configure runtime_fraction match in Routes.

This is useful for a couple of usecases:

  • applying different timeout, retries configurations in a subset of requests to test changes
  • routing a subset of traffic to a different service when service migrations are happening. (route destinations are not usable because they are used by teams to perform deployments)

@sschepens sschepens requested a review from a team as a code owner July 23, 2024 16:06
@istio-policy-bot
Copy link

🤔 🐛 You appear to be fixing a bug in Go code, yet your PR doesn't include updates to any test files. Did you forget to add a test?

Courtesy of your friendly test nag.

@istio-policy-bot
Copy link

😊 Welcome @sschepens! This is either your first contribution to the Istio api repo, or it's been
a while since you've been here.

You can learn more about the Istio working groups, Code of Conduct, and contribution guidelines
by referring to Contributing to Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

@istio-testing istio-testing added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jul 23, 2024
@istio-testing istio-testing added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 23, 2024
@sschepens
Copy link
Contributor Author

sschepens commented Jul 23, 2024

@howardjohn what do you think? we're in need of this and is not easy to implement other way

@zirain
Copy link
Member

zirain commented Jul 24, 2024

Will Gateway API support this?

@sschepens
Copy link
Contributor Author

Will Gateway API support this?

I don't really know, I'm not really much involved into Gateway API. But I don't think Gateway API will ever cover all the features we have in Virtual Services, such as the stat_prefix option.

@zirain
Copy link
Member

zirain commented Jul 24, 2024

Will Gateway API support this?

I don't really know, I'm not really much involved into Gateway API. But I don't think Gateway API will ever cover all the features we have in Virtual Services, such as the stat_prefix option.

As we always said it's the future, should think about it before making decision.

@sschepens
Copy link
Contributor Author

sschepens commented Jul 24, 2024

As we always said it's the future, should think about it before making decision.

Sure, it might be the future, but in my opinion it's currently very limited and there a very small subset of features of VirtualServices that are currently covered, there's still a very long way to go and standards evolve pretty slow, I don't think it's best for Istio to stop inovating in the mean time.

I'll let others chime in though

@linsun
Copy link
Member

linsun commented Jul 26, 2024

Given VS API reaches GA, and Gateway API is future of our networking API, I would like to also understand if this is planned for GW API. @keithmattix @howardjohn @robscott may have some insights.

@keithmattix
Copy link
Contributor

keithmattix commented Jul 26, 2024

I'm fairly certain this won't be coming to Gateway API any time soon. I think the use-case is valid (if somewhat niche) and agree that Istio generally shouldn't stop innovating even though we want Gateway API to be the default experience. My main question is whether the use-case is worth the complexity of implementation and support

@hzxuzhonghu
Copy link
Member

https://www.envoyproxy.io/docs/envoy/latest/configuration/http/http_conn_man/traffic_splitting#traffic-shifting-between-two-upstreams

It doesnot mention traffic shifting across more than three clusters, not sure if it suites. And use case is mostly like weighted clusters

@sschepens
Copy link
Contributor Author

sschepens commented Jul 30, 2024

My main question is whether the use-case is worth the complexity of implementation and support

The implementation itslef is very simple, you can check the PR, it's really like 5 lines of code, the rest is mostly tests.

It doesnot mention traffic shifting across more than three clusters, not sure if it suites. And use case is mostly like weighted clusters

Most use-cases are probably OK with weighted clusters, but route percent match enables much more versatility and applying different route configurations for each given percentage. Also playing with percentages is slightly less troublesome than playing with weights (whenever a weight changes, you most probably have to adjust all other wieghts as well).

@zirain
Copy link
Member

zirain commented Jul 30, 2024

So that's what we should think about: should we make first class API to support a corner case, and at the same time it won't be part of Gateway API(It has been described as the future on many occasions)?

@sschepens
Copy link
Contributor Author

There's already a bunch of corner cases exposed in the API, so what's the logic to tell which ones are ok and which ones are not?

Direct Reponses, VS Delegates, sourceLabels, sourceNamespace, statPrefix, to name a few, are examples of niche or corner cases that are not supported by Gateway API and we had no trouble implementing.

@linsun
Copy link
Member

linsun commented Aug 9, 2024

There's already a bunch of corner cases exposed in the API, so what's the logic to tell which ones are ok and which ones are not?

Direct Reponses, VS Delegates, sourceLabels, sourceNamespace, statPrefix, to name a few, are examples of niche or corner cases that are not supported by Gateway API and we had no trouble implementing.

It is based on number of users' asks either in slack or github or in community meetings. So far, we haven't seen a lot of asks for istio/istio#39880 based on engagement from GitHub.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants