Implement richer extensibility mechanisms for routes and clusters in …

[lqlive]

This PR is based on the Route Extensibility Design Document and primarily addresses the extensibility issues mentioned in the related issue: #1709.

I have carefully reviewed and studied the above document and related links, and I believe this functionality is highly beneficial for the extensibility of YARP, especially in supporting custom route and cluster configurations. After extensive preparation and development, I have successfully completed the implementation of this PR.

If you are already working on this feature or have a better implementation plan, I would be more than happy to close this PR and look forward to contributing ideas or providing assistance for the final implementation.

Thank you for your time and review, and I look forward to your feedback!

[lqlive]

@microsoft-github-policy-service agree

[lqlive]

Any news?

[MihaZupan]

@samsp-msft did you get a chance to look at this one?

[lqlive]

Hi @MihaZupan
Given that Sam has not yet responded, I am hesitant to close this PR. I have taken note of the warning flagged in your latest design document, and I’d like to discuss the design plan with you via this PR to move forward with the feature.
Thank you so much

[samsp-msft]

Sorry, been busy with another project. I trust Miha's judgement on this issue.

[lqlive]

@dotnet-policy-service agree

[lqlive]

@MihaZupan Hi
Could you help move this forward when you have time?

[lqlive]

I'm not entirely happy with this implementation. I'd like to take some time to redo this PR – I'd really value your input

[MihaZupan]

Sorry about the delay here.
I should be able to get to it next week, I want to talk about it with the team first.

[lqlive]

This is my refactored code. I’m quite happy with the current structure, but I’d love to hear any suggestions or feedback you might have. Thank you so much for taking the time to review it!

  /// <summary>
   /// Factory delegate for creating route extensions.
   /// </summary>
   /// <typeparam name="T">The type of extension to create.</typeparam>
   /// <param name="section">The configuration section for this extension.</param>
   /// <param name="routeConfig">The route configuration being extended.</param>
   /// <param name="existingExtension">The existing extension instance if available (during updates).</param>
   /// <returns>The created or updated extension instance.</returns>
   public delegate T RouteExtensionFactory<T>(IConfigurationSection section, RouteConfig routeConfig, T existingExtension);

   /// <summary>
   /// Interface for providers that can create route extensions from configuration.
   /// </summary>
   public interface IRouteExtensionProvider
   {
       /// <summary>
       /// Gets the configuration section name this provider handles.
       /// </summary>
       string SectionName { get; }

       /// <summary>
       /// Creates an extension instance from the given configuration section.
       /// </summary>
       /// <param name="section">The configuration section.</param>
       /// <param name="routeConfig">The route configuration.</param>
       /// <param name="extensions">Existing extensions dictionary.</param>
       /// <returns>True if an extension was created, false otherwise.</returns>
       bool CreateExtension(IConfigurationSection section, RouteConfig routeConfig, IDictionary<Type, object> extensions);
   }


   /// <summary>
   /// A provider that creates route extensions of a specific type.
   /// </summary>
   /// <typeparam name="T">The type of extension to create.</typeparam>
   internal sealed class RouteExtensionProvider<T> : IRouteExtensionProvider
   {
       private readonly RouteExtensionFactory<T> _factory;

       public RouteExtensionProvider(string sectionName, RouteExtensionFactory<T> factory)
       {
           SectionName = sectionName ?? throw new ArgumentNullException(nameof(sectionName));
           _factory = factory ?? throw new ArgumentNullException(nameof(factory));
       }

       /// <inheritdoc/>
       public string SectionName { get; }

       /// <inheritdoc/>
       public bool CreateExtension(IConfigurationSection section, RouteConfig routeConfig, IDictionary<Type, object> extensions)
       {
           if (section is null || routeConfig is null || extensions is null)
           {
               return false;
           }

           var extensionSection = section.GetSection(SectionName);
           if (!extensionSection.Exists())
           {
               return false;
           }

           T existingExtension = default;
           extensions.TryGetValue(typeof(T), out var existingObject);
           if (existingObject is T existing)
           {
               existingExtension = existing;
           }

           var extension = _factory(extensionSection, routeConfig, existingExtension);
           if (extension != null)
           {
               extensions[typeof(T)] = extension;
               return true;
           }

           return false;
       }
   }

  public static class ReverseProxyBuilderExtensions
   {
       /// <summary>
       /// Adds a route extension factory that will be used to create extension instances from configuration.
       /// </summary>
       /// <typeparam name="T">The type of extension to create.</typeparam>
       /// <param name="builder">The <see cref="IReverseProxyBuilder"/>.</param>
       /// <param name="sectionName">The configuration section name for this extension.</param>
       /// <param name="factory">The factory function to create the extension.</param>
       /// <returns>The <see cref="IReverseProxyBuilder"/>.</returns>
       public static IReverseProxyBuilder AddRouteExtension<T>(
           this IReverseProxyBuilder builder,
           string sectionName,
           RouteExtensionFactory<T> factory)
       {
           if (builder == null)
           {
               throw new ArgumentNullException(nameof(builder));
           }

           if (string.IsNullOrEmpty(sectionName))
           {
               throw new ArgumentException("Section name cannot be null or empty", nameof(sectionName));
           }

           if (factory == null)
           {
               throw new ArgumentNullException(nameof(factory));
           }

           builder.Services.AddSingleton<IRouteExtensionProvider>(
               new RouteExtensionProvider<T>(sectionName, factory));

           return builder;
       }
   }

internal class ConfigurationConfigProvider : IProxyConfigProvider
   {
       private readonly IEnumerable<IRouteExtensionProvider> _routeExtensionProviders;

       // ... other members ...

       private void ProcessRouteExtensions(IConfigurationSection section, RouteConfig route)
       {
           var extensions = new Dictionary<Type, object>();
           var extensionsSection = section.GetSection("Extensions");

           if (extensionsSection.Exists())
           {
               // Apply all registered route extension providers
               foreach (var provider in _routeExtensionProviders)
               {
                   provider.CreateExtension(extensionsSection, route, extensions);
               }
           }

           route.Extensions = extensions;
       }

       // ... other methods ...
   }

[MihaZupan]

First of all thank you for looking at improving things here and appologies that it's taking us this long to get to it.

Is this functionality that you're looking to use yourself / within your company? Or are you mainly interested at contributing and just happened to see this issue?
I ask since we haven't heard much interest in a feature like this yet. For example the tracking issue is 3 years old with 0 upvotes, and the initial design draft PR sat idle for 6 months before we merged it without much discussion and a note that we should revisit (the design was not finalized).

At this time we are hesitant to design a feature like this without at least a few clear real-world use cases, and indication that it would meaningfully improve things for more users.

---

As for your proposal #2682 (comment), a couple questions come to mind:

• What would the public API on Route/Cluster types look like? Some sort of IFeatureCollection property?
• Who is responsible for diffing config changes? You might want that to be the shared config provider instead of delegating the responsibility to every extension author.
  • You're passing around the dictionary itself, with a comment The existing extension instance if available (during updates). Was the intention here for that to be persisted between separate updates? Or were you envisioning multiple deserializer callbacks for the same type potentially interacting with one another?
• This seems very skewed towards config file-based configuration approaches (IConfigurationSection).
  • If I'm doing code-based config, is the only functionality I'm getting the feature collection property on route/cluster models? (not saying that's necessarily a bad thing)
  • Or is the expectation that you're still populating a more weakly typed extensions object and also registering deserializers, where YARP then translates the extensions config into IConfigurationSections?
• Is the gist of the functionality that we're providing an opinionated wrapper on how metadata is deserialized? Do we envision use cases over e.g. simple json deserialization?
  • If not, could users solve this today with an extension method that deserialized from metadata? would the main concern there be that metadata is typed as Dictionary<string, string> and not Dictionary<string, object>, so the json would be ugly?

[lqlive]

Thank you very much for your reply.

Regarding my motivation for submitting this PR: while using YARP, I needed to extend its functionality but could only utilize the metadata approach. Since my ConfigMetadate requires strong typing and serialization, I initially planned to implement this through an extension method. However, I recognized that this functionality would be valuable as a built-in feature of YARP.

During my research, I discovered relevant proposals in the issue tracker:

• Route Extensibility Design Document
• Discussion in Issue #1709

This inspired me to dedicate time to implementing the feature. I'm truly happy to contribute this to the community.

---

What would the public API on Route/Cluster types look like? Some sort of IFeatureCollection property?

Regarding this question, I saw IReadOnlyDictionary<Type, object> defined in the documentation. At the time, I didn't give it much thought and hastily submitted the PR. Later when I saw IFeatureCollection in ASP.NET Core, I intended to discuss redesigning this part with you, but too much time passed and I forgot to raise this issue.

Who is responsible for diffing config changes? You might want that to be the shared config provider instead of delegating the responsibility to every extension author.

Initially, I wanted the ConfigProvider to handle this configuration. For example:

new ClusterConfig()

{

ClusterId = "cluster1",

Extensions = new Dictionary<Type, IConfigExtension>

{

{ typeof(UserModel), new UserModel { UserName = "admin" } }

}

};

But I realized this wasn't extensible-friendly. Later I redesigned it to use IRouteExtensionProvider to manage configurations and listen for changes. However, this interface only works well with IConfigurationSection. If the configuration doesn't come from IConfigurationSection (like in (https://github.com/dotnet/yarp/blob/1319bedfa0a2d3d4c45f6c83069497a773315aa9/src/Kubernetes.Controller/Converters/YarpParser.cs)), it becomes useless.

You're passing around the dictionary itself, with a comment "The existing extension instance if available (during updates)". Was the intention here for that to be persisted between updates? Or were you envisioning multiple deserializer callbacks for the same type potentially interacting?

My initial idea was simply to initialize objects as described in the documentation. Thanks for pointing out that multiple deserializers for the same type could indeed interfere with each other. Here I should probably apply the last-one-wins principle, similar to IConfigurationSection:

services.AddReverseProxy()

.LoadFromConfig(Configuration.GetSection("ReverseProxy"))

.AddRouteExtension("A-B", (section, _, _) => new ABState(section));

This seems very skewed towards config file-based configuration approaches (IConfigurationSection)

Yes, the current implementation is very friendly to IConfigurationSection. If configurations come from other sources, we may need to extend IRouteExtensionProvider further.

If I'm doing code-based config, is the only functionality I'm getting the feature collection property on route/cluster models?

Yes. Even if multiple instances of the same type are injected, it will still be treated as one type.

Or is the expectation that you're still populating a more weakly typed extensions object and also registering deserializers, where YARP then translates the extensions config into IConfigurationSections?

Sorry, I haven't considered this scenario yet. Could you show me a simple example?

Is the gist of the functionality that we're providing an opinionated wrapper on how metadata is deserialized? Do we envision use cases over e.g. simple json deserialization?

Absolutely. We could provide a GetMetadata method like (https://github.com/ThreeMammals/Ocelot/blob/0b794b39e26d8bb538006eb5834b841c893c6611/src/Ocelot/Metadata/DownstreamRouteExtensions.cs#L66). This aligns with my original need and would be minimally disruptive to existing code.

If not, could users solve this today with an extension method that deserialized from metadata? would the main concern there be that metadata is typed as Dictionary<string, string> and not Dictionary<string, object>, so the json would be ugly?

Metadata can cover nearly all scenarios. I don't think the JSON is particularly ugly – I was just eager to implement this design document at the time.

---

Through our discussion above, I've also realized that implementing this feature is quite difficult and we currently have some unclear points. Also, it's almost certain that users would not think to use this feature. Going back to my original need, perhaps users really just want the GetMetadata method.

If you agree, I'd like to spend some time implementing this GetMetadata method instead. I'd be truly happy to contribute to the community.

Please forgive me for rushing through the PR submission. I really appreciate your review.

[MihaZupan]

If you agree, I'd like to spend some time implementing this GetMetadata method instead. I'd be truly happy to contribute to the community.

I think the actual implementation should be rather trivial, e.g.

internal static class ClusterConfigExtensions
{
    public static T GetMetadata<T>(this ClusterState cluster, string name)
    {
        ArgumentNullException.ThrowIfNull(cluster);

        var metadata = cluster.Model.Config.Metadata;

        if (metadata is null || !metadata.TryGetValue(name, out var metadataString))
        {
            throw new KeyNotFoundException($"Metadata '{name}' not found in cluster '{cluster.ClusterId}'.");
        }

        return JsonSerializer.Deserialize<T>(metadataString)
            ?? throw new InvalidOperationException($"Metadata '{name}' in cluster '{cluster.ClusterId}' is not of type '{typeof(T).Name}'.");
    }
}

It's more of a question if that's the right thing to expose that'd be useful for more users.

[lqlive]

Based on our previous discussion, I propose to implement the GetMetadata method. I believe this will be valuable to developers who want to extend YARP's functionality. If the team finds it valuable, I will be honored to resubmit a PR to implement it.