-
Notifications
You must be signed in to change notification settings - Fork 842
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
Implement header based routing #448
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall besides some comments on test coverage below.
src/ReverseProxy/Abstractions/RouteDiscovery/Contract/ProxyMatch.cs
Outdated
Show resolved
Hide resolved
src/ReverseProxy/Abstractions/RouteDiscovery/Contract/ProxyMatch.cs
Outdated
Show resolved
Hide resolved
The main benefit is performance. It moves logic from being executed per-request to at startup when the DFA is built. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
src/ReverseProxy/Abstractions/RouteDiscovery/Contract/HeaderMatchMode.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
Is there a way to add a manual priority/weight to routes to bypass the default ordering. For example if I want all requests with a debug header to go to a specific destination, could I flag that rule in some way to push it to the top of the stack? |
If you are able to specify the route order explicitly that takes precedence over anything else. Routing supports that, I'm not sure if YARP exposes it, but it would be something good to expose. |
Yes, YARP exposes the Order option on routes. |
|
||
### IsCaseSensitive | ||
|
||
Indicates if the value match should be performed as case sensitive or insensitive. The default is `false`, insensitive. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we demonstrate this in one of the examples?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know. It's more of an escape hatch for people, I'm not sure how much it will get used.
// TODO: Matches individual values from multi-value headers (split by coma, or semicolon for cookies). | ||
// Also supports multiple headers of the same name. | ||
// ExactValue, | ||
// ValuePrefix, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we decide to do this, I like ContainsExactValue
and ContainsValuePrefix
names, but I'm not sure how common of a requirement matching individual values from multi-value headers will be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're not doing these right now, we'll wait for feedback first. I left them here as examples of possible future values.
I don't know about Contains, it has mixed connotations from string.Contains and ICollection.Contains, we'd want the latter.
}; | ||
} | ||
|
||
internal static bool Equals(RouteHeader header1, RouteHeader header2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why use a internal static Equals instead of overriding object.Equals?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To simplify the calling code when either value might be null.
|
||
### Precedence | ||
|
||
Route match precedence order is 1) path, 2) method, 3) host, 4) headers. That means a route that specifies methods and no headers will match before a route which specifies headers and no methods. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How did we come up with this precedence order? I feel like host should be first or last overall even if it's technically just another header.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first three are already built into AspNetCore routing with that precedence. Our only choice is where to rank headers. Per some other comments I'll add a note here that this can be overridden per route by setting the Order property.
/azp run |
No pipelines are associated with this pull request. |
7b2bcca
to
4ad649e
Compare
Fixes #405
Design points:
Other oddities: