-
Notifications
You must be signed in to change notification settings - Fork 124
Matches
in TrafficSplit
is unclear
#218
Comments
I think this makes perfect sense expanding on the example in the doc that allows a canary to be created for firefox users. kind: TrafficSplit
metadata:
name: ab-test
spec:
service: website
matches:
- kind: HTTPRouteGroup
name: ab-test
matches:
- firefox-users
backends:
- service: website-v1
weight: 0
- service: website-v2
weight: 100
---
kind: HTTPRouteGroup
metadata:
name: ab-test
matches:
- name: firefox-users
headers:
- user-agent: ".*Firefox.*"
- name: safari-users
headers:
- user-agent: ".*Safari.*" I wonder should we refactor to be more in line with TrafficAccess? From kind: TrafficSplit
metadata:
name: ab-test
spec:
service: website
matches:
- kind: HTTPRouteGroup
name: ab-test
matches:
- firefox-users
backends:
- service: website-v1
weight: 0
- service: website-v2
weight: 100 To kind: TrafficSplit
metadata:
name: ab-test
spec:
service: website
rules:
- kind: HTTPRouteGroup
name: ab-test
matches:
- firefox-users
backends:
- service: website-v1
weight: 0
- service: website-v2
weight: 100 |
@michelleN @nicholasjackson I hope I can suggest some inputs here, I like the design proposed by Nick. I have one minor input on that, basically to add Nick's Version:
Suggested Change:
|
Thanks @nicholasjackson and @kumarabd. Thanks for the suggestions. The benefits I see of Nic's version is that there is greater clarity and less room for error when building and validating a traffic split. Having one traffic split per set of backends allows for the same pattern as TrafficTarget which is a set of sources that have a set of rules (with match conditions) for reaching a destination. The equivalent of that pattern in TrafficSplit would be having all traffic with a set of rules (with match conditions) reach a set of backends (destination). The tradeoff is the usability in @kumarabd's proposal which allows for one traffic split to contain essentially all the logic for splitting traffic to that service to any set of backends. If there is a single source of truth (one traffic split), there is only one split someone needs to modify. As a user it may be tedious to figure out which traffic splits to remove/update and in what order for the desired outcome. The set of backends under |
Yes, Absolutely. To your point,
I see some merit here in terms of predicting the behaviour. |
From an implementation perspective, I wonder if it would be better to have a default set of backends or remove the option of the default backends and only apply backends to a specific set so that there is no implicit behavior and everything is explicit? My gut says explicit is better since we have different proxies in our community and these subtleties may make a large impact when it comes to expected behavior, but I definitely understand and empathize with the issues @kumarabd pointed out. Inspired by both of your examples, what do ya'll think about something like this:
|
Looks good to me, would allow you to define all the properties of a test in a single CRD. 🚢 |
Looking at this. It shouldn't even break the sdk/Kubernetes API versioning requirements. Do you see any issues @nicholasjackson ? |
Perhaps we should also include backends for |
I think it would be fine @michelleN, moving from this version to the previous version would mean you have to drop anything other than the first rule but moving from a previous one to this would be unaffected. I think this is a happy compromise in the name of progress. |
I really like your examples and thought about them, but I having a key for explicit default backends would be less confusing. Right now, if a TrafficSplit does not match any of the match rules (say a non-Firefox/Chrome user like a Safari user), then when a request is made to Would love your thoughts on this @nicholasjackson @kumarabd Examples kind: TrafficSplit
metadata:
name: ab-test
spec:
service: website
splits:
- rules:
- kind: HTTPRouteGroup
name: ab-test
matches:
- firefox-users
backends:
- service: website-v1
weight: 50
- service: website-v2
weight: 50
- rules:
- kind: HTTPRouteGroup
name: ab-test
matches:
- chrome-users
backends:
- service: website-v3
weight: 20
- service: website-v4
weight: 80
- default:
backends:
- service: website
weight: 100 kind: TrafficSplit
metadata:
name: ab-test
spec:
service: website
splits:
- rules:
- kind: HTTPRouteGroup
name: ab-test
matches:
- firefox-users
backends:
- service: website-v1
weight: 50
- service: website-v2
weight: 50
- rules:
- kind: HTTPRouteGroup
name: ab-test
matches:
- chrome-users
backends:
- service: website-v3
weight: 20
- service: website-v4
weight: 80
- default:
backends:
- service: website-v1
weight: 20
- service: website-v2
weight: 80 |
@johnsonshi Your proposal makes sense to me.
On a different note, I've been thinking more about this. As I tried to go and just rename things, I came to the conclusion that if the
What do ya'll think? @nicholasjackson @kumarabd @johnsonshi PR for traffic specs changes here: #229 |
@michelleN I agree with the fact that the changes involved is quite big and that should be covered in the later versions. @johnsonshi Your proposal looks a lot cleaner. I was wondering if it will be a challenge to have |
In TrafficTarget, we refer to
Rules
(which "are traffic specs that define what traffic for specific protocols would look like. The kind can be different depending on what traffic a target is serving. In the following examples, HTTPRouteGroup is used for applications serving HTTP based traffic"). Arule
has an optionalmatches
field in case one wants to match on only one match condition defined in the TrafficSpec (HTTPRouteGroup) referenced in therule
. In TrafficSplit, we only have aMatches
field, noRules
andMatches
refers to a slice ofTypedLocalObjectReference
, so you can refer to HTTPRouteGroup in the same namespace but we cannot refer to a specific match condition defined in the HTTPRouteGroup. This is slightly confusing and inconsistent. Do we want to be able to specify a match condition in a Traffic Split as well?Related:
#190 (comment)
The text was updated successfully, but these errors were encountered: