Add support for adding metadata to proxy routes

[Kahbazi]

Partially Fixes #286

I changed the way endpoints are being built. I tried to mirror MVC, and now endpoints are being built in ProxyDynamicEndpointDataSource instead of RuntimeRouteBuilder.

I think the main challenge is with ProxyConfigLoader which is a IHostedService and runs before MapReverseProxy has a chance to add metadata, so I add a bool firstTime parameter and pass in around , so I only update RouteConfig and not building the endpoints.

I want feedback for design before I continue with unit tests.

[Tratcher]

I hadn't realized this would be so complex a change. It's going to take me some time to go over this.

The firstTime parameter is concerning, it seems like a blunt way to address a race condition. The design changes proposed in #277 should remove this race because they reverse the model from push to pull. I think we'll want to do #227 before continuing work on this PR.

[Kahbazi]

I don't like the firstTime either, I just wanted to make it work.
Overall I didn't understand the need for the IHostedService for reloading the config. Can't it be done with just IOptionsMonitor.OnChange(...)?

[Tratcher]

I don't like the firstTime either, I just wanted to make it work.
Overall I didn't understand the need for the IHostedService for reloading the config. Can't it be done with just IOptionsMonitor.OnChange(...)?

But where would you call OnChange?

[Kahbazi]

But where would you call OnChange?

Maybe in LoadFromConfig() but then somehow it should also run for the first time.

[Tratcher]

But where would you call OnChange?

Maybe in LoadFromConfig() but then somehow it should also run for the first time.

That's the trick, LoadFromConfig doesn't have access to IOptionsMonitor, the DI container hasn't been built yet. Something has to resolve IOptionsMonitor from DI at startup in order to kick of the process. The IHostedService is one way of doing that, but #227 should be able to replace that with a direct config source resolved from DI.

[Tratcher]

@Kahbazi can you please rebase this?

[Kahbazi]

I did the rebase. I need some feedback and then I will fix and add tests.

[Tratcher]

There's another merge conflict?

[davidfowl]

I think we're going to need an explanation of the overall design that comes out of this change since it looks like it's evolved a bunch from the original purpose of the PR.

[Kahbazi]

The purpose of this PR is to enable users to add their own Metadata to Endpoints that are created by YARP and it can be done by ReverseProxyConventionBuilder .

- void MapReverseProxy(...)
+ ReverseProxyConventionBuilder MapReverseProxy(...)

The challenge was that when calling MapReverseProxy() it will eventually call IRuntimeRouteBuilder.Build() and it both creates the RouteConfig and Endpoint. So there's no chance for the user convention to be applied to the Endpoint.
So I introcude ProxyEndpointFactory and move the responsibility of creating Endpoint from IRuntimeRouteBuilder to it, so the Endpoint could be created at a later point in time.
For the same reason I change the ProxyConfigManager.Endpoints getter from simply returning the Endpoint to Initializing the Endpoint first and then return them.

This is the API that user can use to add Metadata.

public class ReverseProxyConventionBuilder
{
    // Configure all routes
    public void Add(Action<EndpointBuilder> convention); 
    // Configure all routes based on their Cluster
    public ReverseProxyConventionBuilder ConfigureClusters(Action<IEndpointConventionBuilder, Cluster> convention);
    // Configure all routes based on their Route
    public ReverseProxyConventionBuilder ConfigureRoutes(Action<IEndpointConventionBuilder, ProxyRoute> convention);
    // Configure all routes for a specific Cluster
    public ReverseProxyConventionBuilder ConfigureCluster(string clusterId, Action<IEndpointConventionBuilder> convention);
    // Configure a specific Route
    public ReverseProxyConventionBuilder ConfigureRoute(string routeId, Action<IEndpointConventionBuilder> convention);
}

[davidfowl]

I like the design. Do those configure callbacks get run every time the configuration changes?

[Kahbazi]

I like the design. Do those configure callbacks get run every time the configuration changes?

If the configuration changes causes a change on routes.

reverse-proxy/src/ReverseProxy/Service/Management/ProxyConfigManager.cs

Lines 167 to 171 in dc2334b

	var hasChanged = await ApplyConfigAsync(newConfig);
	if (hasChanged)
	{
	CreateEndpoints();
	}

reverse-proxy/src/ReverseProxy/Service/Management/ProxyConfigManager.cs

Lines 103 to 113 in dc2334b

	private void CreateEndpoints()
	{
	var endpoints = new List<Endpoint>();
	foreach (var existingRoute in _routeManager.GetItems())
	{
	var runtimeConfig = existingRoute.Config.Value;
	_proxyEndpointFactory.AddEndpoint(endpoints, runtimeConfig, _conventions);
	}
	UpdateEndpoints(endpoints);
	}

[Tratcher]

If an individual route or cluster changes we shouldn't have to re-create every endpoint, only those that changed. That's why the endpoints were stashed on the RouteConfig before. What gets recreated is the list of endpoints.

There are callbacks to configure an endpoint route based on cluster id or data, but this doesn't detect changes to clusters so those wouldn't re-run unless the route itself changed.

[Kahbazi]

Where do you suggest we should stash the endpoints now that RouteConfig is too early to create the endpoints?

Also do we need to re-create the endpoints of a cluster if its destinations changed?

[Tratcher]

I guess it's technically possible for a route not to be assigned to a cluster, but why would proxyRoute ever be null?

[Kahbazi]

I removed the null check for proxy route

[Tratcher]

Suggested change

	/// Configures the endpoint for all routes
	/// Configures the endpoints for all routes

[Tratcher]

The experience question we need to answer is how common you'll write code to configure a route or cluster by name vs inspecting all the endpoints and reacting to other fields like metadata. i.e. do we need all 9 of these overloads, or only the first three Configure ones? The cluster and route specific ones can be built on top of the first three Configure conventions if needed.

I'd suggest limiting it to just the Configure overloads for now. We can revisit as we see demand for common variations.

[Tratcher]

Design update:
MapReverseProxy now returns a ReverseProxyConventionBuilder : IEndpointConventionBuilder. This has a new Configure API with three overloads. The purpose of the API is to let you provide one or more conventions that examine all of the routes loaded from config and append any custom endpoint metadata those routes need.

E.g. A convention could check for all routes that use cluster1 and set AllowAnonymous auth for those endpoints, where auth is normally configured per route. A developer could also insert their own endpoint metadata based on route or cluster extensible metadata fields.

New API:

Configure(Action<IEndpointConventionBuilder> convention)
Configure(Action<IEndpointConventionBuilder, ProxyRoute> convention)
Configure(Action<IEndpointConventionBuilder, ProxyRoute, Cluster> convention)

Clarifications from the prior discussion:

• The ProxyRoute and Cluster types are given here for information purposes, they're not modifiable at this point the the lifecycle. Use freezable pattern for contract types instead of deep cloning #179 tracks clarifying that.
• We don't want to define new routes in Startup, we want routes to be sourced from an external store so they can always be updated without restarting.
• When routes or clusters are updated, the associated endpoints are recreated and the conventions re-run.
• The overloads that reference individual routes or clusters by name have been removed. While it's possible to make decisions based on names, I don't think that's the primary use case, but rather to use config metadata from the routes and clusters.

[JamesNK]

Minor comments

[ghost]

Hello @Tratcher!

Because this pull request has the Auto-Merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

[Tratcher]

@Kahbazi thanks for the hard work on this one, I'm glad we were able to work through it eventually.