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

Extending configuration nodes with additional data #87

Closed
samsp-msft opened this issue Apr 23, 2020 · 18 comments
Closed

Extending configuration nodes with additional data #87

samsp-msft opened this issue Apr 23, 2020 · 18 comments
Assignees
Labels
Type: Idea This issue is a high-level idea for discussion.
Milestone

Comments

@samsp-msft
Copy link
Contributor

In looking at the round robin implementation and the features / scenarios @Kahbazi mentioned, we are going to need to be able to stuff extra information into these nodes.
There are a couple of scenarios:

  • Data that internal components will want to store, and will always be needed - these should probably go as properties directly on the objects

  • Data that some optional components will want to store, such as load balancing counters - For these I would suggest we go with something like the Feature collection from HttpContext. The key complication here is that it needs to be thread safe as it can be accessed from many workers simultaneously. This mechanism could also be used for custom components for storing state that does not need to be set by configuration. A further complication is whether this state is persisted across configuration changes - I would suggest it should be, with some form of notification so that the owning component can decide what to do.

  • Data that needs/can be specified by the user as part of configuration. For example if a load balancing scheme is to direct a percentage of traffic to particular nodes, that needs to be specifiable. I think it may be better to implement a dictionary approach for specifying properties on these nodes so that we have flexibility to add what is needed.

I am trying to think of YARP as a framework for a proxy with extensibility as a core tenet - we aren't going to cover every scenario with built in components, so we need to be consider how extra components are going to hook in and store their state.

@samsp-msft samsp-msft added the Type: Discussion This issue is a discussion thread and doesn't currently represent actionable work. label Apr 23, 2020
@analogrelay
Copy link
Member

As this is a "Discussion" item that usually indicates we don't have specific action to take here yet. Should we plan to have a more concrete discussion on this and create some actionable work or are we not quite there yet?

@Tratcher
Copy link
Member

  • Data that needs/can be specified by the user as part of configuration. For example if a load balancing scheme is to direct a percentage of traffic to particular nodes, that needs to be specifiable. I think it may be better to implement a dictionary approach for specifying properties on these nodes so that we have flexibility to add what is needed.

We actually already have something for this. Backend, BackendEndpoint, and ProxyRoute all have IDictionary<string, string> Metadata collections that bind from config.

Read-only external config input should be stored separately from mutable runtime state due to concurrency issues. i.e. we shouldn't try to re-use this collection for the other two data types.

@analogrelay analogrelay added this to the Feedback milestone Apr 28, 2020
@Kahbazi
Copy link
Collaborator

Kahbazi commented Apr 28, 2020

I would like to see List<object> EndpointMetadata property on ProxyRoute which should be added to endpointBuilder

var endpointBuilder = new AspNetCore.Routing.RouteEndpointBuilder(
requestDelegate: _pipeline ?? Invoke,
routePattern: AspNetCore.Routing.Patterns.RoutePatternFactory.Parse(pathPattern),
order: 0);

@analogrelay analogrelay modified the milestones: Feedback, 1.0.0 Apr 28, 2020
@analogrelay analogrelay added Type: Idea This issue is a high-level idea for discussion. and removed Type: Discussion This issue is a discussion thread and doesn't currently represent actionable work. labels Apr 28, 2020
@Tratcher
Copy link
Member

Filed #121 to make Metadata more user friendly.

@Kahbazi List<object>? How would a component find the value it needs in the list?

@Kahbazi
Copy link
Collaborator

Kahbazi commented Apr 28, 2020

I would like to add IAuthorizeData, IAllowAnonymous or ICorsMetadata to Endpoint Metadata, so I could have different AuthN, AuthZ and CORS for different endpoints.

@Tratcher
Copy link
Member

Hmm, I can see why you'd want those, but those aren't objects you'll be able to load directly from a config source. It would take some intermediate translation in the endpoint builder.

We should be adding first class support for authz (filed #122). (And CORS?)

@Kahbazi
Copy link
Collaborator

Kahbazi commented Apr 28, 2020

I'm definitely in favor of adding first class support for authz (and CORS), but still I would like to see a
List<object> on ProxyRoute.
I believe each framework that maps endpoints to the IEndpointRouteBuilder, should also provide APIs for adding endpoint metadata to it. Who knows what middleware or components might comes in future that needs some metadata on an endpoints as a trigger, and since we can't know what they needs adding metadata from code is good enough for me.

(And CORS?)

I'm thinking of using YARP as a backend for multiple SPAs and each needs different CORS settings.

@samsp-msft
Copy link
Contributor Author

samsp-msft commented Apr 28, 2020

@Kahbazi List<object>? How would a component find the value it needs in the list?

I think part of what @Tratcher was asking is how would you want to retrieve the objects. HttpContext has a feature collection, which is essentially an IDictionary<System.Type, object>, so items can be retrieved based on the key rather than having to search through a list each time.

@Kahbazi
Copy link
Collaborator

Kahbazi commented Apr 28, 2020

no I don't want to add these items to each request of HttpContext. I want them to be in endpoint metadata and I could get them with
httpContext.GetEndpoint().Metadata.GetMetadata<SomeMetadata>()

@Tratcher
Copy link
Member

Oh, I see. Endpoint.Metadata is a custom collection with generic search functionality (via downcasting).
https://github.com/dotnet/aspnetcore/blob/600d5ff4978c84b42aa15a11a0fdb0dbd1cc8692/src/Http/Http.Abstractions/src/Routing/EndpointMetadataCollection.cs#L75

Ok, let me back up. Yes we need some way to add custom endpoint metadata. ProxyRoute is too low level for that, it's a raw config representation, you're not going to be able to represent complex objects like IAuthorizeData there. We likely need an event in RuntimeRouteBuilder that lets you customize the routes as we build them. You'd take some config metadata and translate it to complex objects like IAuthorizeData.

var endpointBuilder = new AspNetCore.Routing.RouteEndpointBuilder(
requestDelegate: _pipeline ?? Invoke,
routePattern: AspNetCore.Routing.Patterns.RoutePatternFactory.Parse(pathPattern),
order: 0);
endpointBuilder.DisplayName = source.RouteId;
endpointBuilder.Metadata.Add(newRouteConfig);
if (source.Host != null)
{
endpointBuilder.Metadata.Add(new AspNetCore.Routing.HostAttribute(source.Host));
}
if (source.Methods != null && source.Methods.Count > 0)
{
endpointBuilder.Metadata.Add(new AspNetCore.Routing.HttpMethodMetadata(source.Methods));
}

@Tratcher
Copy link
Member

Or maybe an event in DynamicConfigBuilder when we build the ParsedRoute object.

@Tratcher
Copy link
Member

I'm adding some config events earlier in the pipeline but I think they're still too low level for this.
#47.

@Kahbazi
Copy link
Collaborator

Kahbazi commented Jul 2, 2020

Can we have first class support for CORS the way we have for authz?

@Tratcher
Copy link
Member

Tratcher commented Jul 2, 2020

Can we have first class support for CORS the way we have for authz?

Can you open a new issue for that and outline what CORS support you're looking for? I'm not as familiar with that part of the stack. What would you normally configure per route?

@Tratcher
Copy link
Member

Tratcher commented Jul 2, 2020

FYI I've split out the endpoint metadata event from #87 (comment) to #286, it's independent of the original issue here around various data stores.

@karelz
Copy link
Member

karelz commented Mar 24, 2021

Triage: 1.0 has metadata, while not pretty, it is adequate. We can keep iterating on it post 1.0.

@karelz
Copy link
Member

karelz commented May 17, 2022

Triage: Merge it into #1709

@samsp-msft
Copy link
Contributor Author

Duplicate of #1709

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Idea This issue is a high-level idea for discussion.
Projects
None yet
Development

No branches or pull requests

5 participants