created configuration-driven proxy filter, which uses a handlebar-lik… #2166
Conversation
…e approach to wiring up configuration values - like routes and endpoints - based on values coming out of configuration.
src/ReverseProxy/Configuration/ConfigurationDrivenFilter/ConfigurationDrivenFilter.cs
Outdated
Show resolved
Hide resolved
| internal class ConfigurationDrivenFilter : IProxyConfigFilter | ||
| { | ||
| // Matches {{env_var_name}} or {{my-name}} or {{123name}} etc. | ||
| private readonly Regex _exp = new("\\{\\{(\\w+\\-?\\w+?)\\}\\}"); |
There was a problem hiding this comment.
is using {{ }} too similar to route paths?
There was a problem hiding this comment.
i don't think it's a bad thing; i think the handlebars are something most web folks are accustomed to using.
| foreach (var d in cluster.Destinations) | ||
| { | ||
| var origAddress = d.Value.Address; | ||
| if (_exp.IsMatch(origAddress)) |
There was a problem hiding this comment.
substitution here is only against full values, and for destinations. should this be more generic?
- should I be able to have a destination = "{{hostname}}:1234" where only hostname is replaced?
- should I be able to use this syntax in other places in configuration?
There was a problem hiding this comment.
We discussed this in a call; I think we landed on being okay with this current approach and it mainly being used for destinations. Correct me if I'm wrong. :)
There was a problem hiding this comment.
I don't think we want to do it this way. MapForwarder seems like a better fit.
#2165 (comment)
Also, config filters have a weakness where they're not able to respond to configuration change notifications. We'll want to solve that before building something like this on them.
| if (route.Order.HasValue && route.Order.Value < 1) | ||
| { | ||
| return new ValueTask<RouteConfig>(route with { Order = 1 }); | ||
| } |
There was a problem hiding this comment.
This is unrelated, it's copy-pasted from a different sample, right? Remove.
I agree on the problem of config filters adapting to changed, but I think the primary source for these values is environment variables that should be considered immutable during a process/container's lifetime. How smart is our change detection for configuration - does it know what has changed, or just something in config has changed? If its the latter, then this will be re-run when config has changed. I am not expecting this to be used much with alternate config sources - its really great to enable substitutions via env vars into a config so you don't have to re-build the container to pickup stuff when running in containers. |
Nope. We want full goodness of YARP which you don't get with the forwarder. This needs name substitution to be able to deal with running locally and then in the cloud - and wanting to map service names for each of them based on environment. |
|
I'll make the changes @samsp-msft suggested and hope it gets merged. :) |
…gurationDrivenFilter.cs Co-authored-by: Sam Spencer <54915162+samsp-msft@users.noreply.github.com>
|
ping to @Tratcher and @samsp-msft - all changes you both suggested have been made. Let me know if there's anything additional needed here, and appreciate the reviews! |
| public class ConfigSubstitutionFilter : IProxyConfigFilter | ||
| { | ||
| // Matches {{env_var_name}} or {{my-name}} or {{123name}} etc. | ||
| private readonly Regex _exp = new("\\{\\{(\\w+\\-?\\w+?)\\}\\}"); |
There was a problem hiding this comment.
Can we get a review on this regex? Also this should probably use the regex source generator.
cc @stephentoub
There was a problem hiding this comment.
It'd be easier to read if the expression had fewer slashes:
@"{{(\w+-?\w+?)}}"Is the intention that this can also match {{my-}} with no word after a dash? If not, it'd be better as:
@"{{(\w+(?:-\w+)?)}}"
|
|
||
| public static class ConfigSubstitutionFilterProxyBuilderExtensions | ||
| { | ||
| public static IReverseProxyBuilder AddConfigurationDrivenProxyFilter(this IReverseProxyBuilder builder) |
src/ReverseProxy/Configuration/Filters/ConfigSubstitutionFilter.cs
Outdated
Show resolved
Hide resolved
| if (_exp.IsMatch(origAddress)) | ||
| { | ||
| var lookup = _exp.Matches(origAddress)[0].Groups[1].Value; |
There was a problem hiding this comment.
Why IsMatch and then Matches(..)[0] rather than just using Match (and checking its Success property)?
There was a problem hiding this comment.
I presume they'd be equivalent, so that's a good call-out. @samsp-msft do you think this would enable consistent behavior?
There was a problem hiding this comment.
IsMatch followed by Matches will run the regex twice rather than once, and Matches will result in allocating an additional collection object. Just using Match, e.g.
Match m = _exp.Match(origAddress);
if (m.Success)
{
var lookup = match.Groups[1].Value;
...
}will have exactly the same functional semantics but be a heck of a lot cheaper.
There was a problem hiding this comment.
Thanks! I'll update the PR with that.
|
Triage: Looks like plans have changed, closing. |
created configuration-driven proxy filter, which uses a handlebar-like approach to wiring up configuration values - like routes and endpoints - based on values coming out of configuration.
resolves #2165