reverse-proxy icon indicating copy to clipboard operation
reverse-proxy copied to clipboard

Missing DI possibilities in using HttpTransformer

Open PaulVrugt opened this issue 1 year ago • 15 comments

Some details

How many backends are in your application?

  • [ ] 1-2
  • [ ] 3-5
  • [ ] 6-10
  • [x] 10+

How do you host your application?

  • [ ] Kubernetes
  • [x] Azure App Service
  • [ ] Azure VMs
  • [ ] Other Cloud Provider (please include details below)
  • [ ] Other Hosting model (please include details below)

What did you think of YARP?

We are using the HttpForwarder functionality of YARP to dynamically reroute incoming requests to another server, and it seems to work really well. However, we are missing a functionality to use DI in the HttpTransformer class. Because we always have to pass an instance of a HttpTransformer to the MapForwarder method, there is no way to use scoped services within the HttpTransformer, other than passing the ServiceProvider itself to the HttpTransformer and manually creating a scope and resolving services in the HttpTransformer. We would really like it if we were able to register a scoped HttpTransformer to the ServiceCollection and have the HttpForwarder resolve the HttpTransformer from the ServiceCollection, so we can use scoped services in the HttpTransformer constructor

PaulVrugt avatar Feb 09 '23 09:02 PaulVrugt

@PaulVrugt You should be able to use HttpContext.RequestServices.GetService<T>() in HttpTransformer to get the service that you need.

Kahbazi avatar Feb 09 '23 10:02 Kahbazi

Thanks @Kahbazi

that makes by code a bit better, but I would still prefer that I could just add my dependencies to the constructor of my HttpTransformer class to be able to have a decent ioc pattern without relying directly on the ServiceProvider

I checked, and it would be a very small addition to the MapForwarder logic, to add an overload that registers the route and uses dependency injection to get the HttpTransformer:

public static IEndpointConventionBuilder MapForwarder<TRequestForwarderType>(the normal parameters)
{
  return endpoints.Map(pattern, async delegate (HttpContext httpContext)
  {
      await forwarder.SendAsync(httpContext, destinationPrefix2, httpClient2, requestConfig2, 
  httpContext.RequestServices.GetRequiredService<TRequestForwarderType>());
  });
}

and voila, we have a DI enabled requestforwarder

The problem is that I cannot make this overload myself, because it requires the DirectForwardingHttpClientProvider class, that is internal sealed, so I cannot access it

PaulVrugt avatar Feb 09 '23 10:02 PaulVrugt

The problem is that I cannot make this overload myself, because it requires the DirectForwardingHttpClientProvider class, that is internal sealed, so I cannot access it

DirectForwardingHttpClientProvider only caches the HttpClient instance, you can do that yourself.

Re-creating the transforms and transformer per request would be really bad for performance, you're better off accessing RequestServices inside your transform.

Tratcher avatar Feb 09 '23 22:02 Tratcher

I never said anything about recreating the transforms, only my custom transformer class. Having a class created has hardly any overhead, and avoids having to use an di antipattern of using the service collection directly.

Anyway, with the di way, the user implementing the transformer has full control over the scope.

PaulVrugt avatar Feb 09 '23 23:02 PaulVrugt

Can you give an example implementation of TRequestForwarderType?

Tratcher avatar Feb 09 '23 23:02 Tratcher

In our case, we have a multi tenanted web application (so a single instance of a web application, serving multiple customers). We use the forwarder to expose a backend 3rd party reporting service. We have multiple instances running multiple versions of the reporting service. We need to be able to dynamically determine which customer goes to what backend service. So we use an implementation of HttpTransformer and override the TransformRequestAsync and update the RequestUri of the proxyRequest object. To determine where the tenant needs to go, we need a service from the ServiceCollection, which is a scoped service. So currently we something like:

public class ReportingHttpTransformer : HttpTransformer
{
    async public override ValueTask TransformRequestAsync(HttpContext httpContext, HttpRequestMessage proxyRequest, string destinationPrefix)
    {
        //do default stuff like copy headers
        await base.TransformRequestAsync(httpContext, proxyRequest, destinationPrefix);

        //get the DI dependencies I need
        var context = httpContext.RequestServices.GetRequiredService<Context>();

        proxyRequest.RequestUri = RequestUtilities.MakeDestinationAddress(context.ReportingUrl, httpContext.Request.Path.Value!, httpContext.Request.QueryString);

        //clear host to make sure that the host of the new destination is used
        proxyRequest.Headers.Host = null;
    }
}

but what I'd like to be able to do is:

public class ReportingHttpTransformer : HttpTransformer
{
    public ReportingHttpTransformer(IContext context)
    {
        _context = context;
    }

    async public override ValueTask TransformRequestAsync(HttpContext httpContext, HttpRequestMessage proxyRequest, string destinationPrefix)
    {
        //do default stuff like copy headers
        await base.TransformRequestAsync(httpContext, proxyRequest, destinationPrefix);

        proxyRequest.RequestUri = RequestUtilities.MakeDestinationAddress(_context.ReportingUrl, httpContext.Request.Path.Value!, httpContext.Request.QueryString);

        //clear host to make sure that the host of the new destination is used
        proxyRequest.Headers.Host = null;
    }
}

this way I can remove the dependency on the ServiceCollection, and make it much easier to unit test my code and mocking dependencies.

The solution I proposed in an earlier comment doesn't require you to use scoped or transient services. If you don't need them, you can still register you HttpTransformer as a singleton. But this change will give the user full control of the scope.

PaulVrugt avatar Feb 10 '23 07:02 PaulVrugt

We need to be able to dynamically determine which customer goes to what backend service.

There are easier ways to do this, you don't need transforms. Try this:

using Yarp.ReverseProxy.Configuration;
using Yarp.ReverseProxy.Forwarder;

var builder = WebApplication.CreateBuilder(args);

builder.Services.AddHttpForwarder();
builder.Services.AddSingleton(new Context() { ReportingUrl = "http://localhost/backend" });

var app = builder.Build();

var pattern = "toProxy";
var client = new ForwarderHttpClientFactory().CreateClient(new ForwarderHttpClientContext
{
    NewConfig = HttpClientConfig.Empty
});

app.Map(pattern, (HttpContext httpContext, IHttpForwarder forwarder, Context context) =>
{
    return forwarder.SendAsync(httpContext, context.ReportingUrl, client);
});

app.Run();

public class Context
{
    public string ReportingUrl { get; init; }
}

Tratcher avatar Feb 10 '23 21:02 Tratcher

Thank you for the update!

I'm not entirely convinced what you are proposing is actually easier than the code I already have. And what I forgot to mention is that we also need to transform headers in the request to the backend. I'm not sure if this is possible in the method you are proposing. And lastly, our "context" class is scoped, not a singleton (not sure if this matters, haven't tested the code yet).

Could you explain to me to the urge not to use the HttpTransformer? Using it seems like the "clean" and testable way to implement the forwarding . I don't think it is because of performance, because if I don't pass a transformer, still a default transformer is used.

PaulVrugt avatar Feb 11 '23 06:02 PaulVrugt

This is really about the lifetime of the transformer. It's a singleton, and resolving scoped services needs to be done manually. A scoped alternative could be designed but I'm not convinced it would be very clean to let the lifetime of the existing component change based on registration.

davidfowl avatar Feb 11 '23 18:02 davidfowl

Well yes that's the whole point. I can only pass a singleton version of the transformer, while I want to have a scoped version. Having a DI version of the transformer would allow me to use a scoped version, and would still allow others to use the existing method to use it as a singleton (or use the di way and register your own transformer as a singleton)

PaulVrugt avatar Feb 12 '23 14:02 PaulVrugt

This seems like a reasonable API proposal.

davidfowl avatar Feb 12 '23 15:02 davidfowl

The transformer isn't even normally in the DI container, its lifetime is managed by the caller. E.g. when we load routes from config then there's a separate transformer built for each route. That doesn't map to DI lifetimes.

And lastly, our "context" class is scoped, not a singleton (not sure if this matters, haven't tested the code yet).

Scoped shouldn't change anything in this example.

we also need to transform headers in the request to the backend

Is that also unique to each tenant? Maybe you shouldn't be using direct forwarding, but the higher level config model. How do you decide which destination to send the requests to? I wonder if that could be captured as route constraints or in the A/B dynamic re-routing feature. https://microsoft.github.io/reverse-proxy/articles/config-files.html https://microsoft.github.io/reverse-proxy/articles/config-providers.html https://microsoft.github.io/reverse-proxy/articles/ab-testing.html

The main thing I want to emphasize at this point is that MapForwarder is a convenience method for a simple, common scenario, routing all traffic with a simple config. For more complex/custom scenarios we expect people to map an endpoint or controller like shown above. Since this is the first time anyone has asked to put the HttpTransformer into the DI container, we need to wait for more feedback to see if we get similar request or variations on it.

Tratcher avatar Feb 13 '23 20:02 Tratcher

Yes the headers are unique to each tenant. Even unique to specific users in each tenant. We are using the forwarder for its ability to dynamically determine the target of a request. Using config files or routing constraints is far too static for our need. The configuration of tenants (including the target of these requests to forward) is stored in a database and can change at any time, and our application has to react to these changes almost instantly.

Now let me stress that my current setup works like a charm. The only suggestion I have is to make using a transformer more di friendly, and making my code better testable in the process

I am still curious however about your tendency to point me away from using the forwarder with a transformer. Is there any specific downside to what I am doing here? It seems to me I have the simplest setup with the maximum flexibility of modifying the proxy request using this setup

PaulVrugt avatar Feb 13 '23 20:02 PaulVrugt

YARP is a pretty flexible system, I just wanted to make sure you're using the APIs that best suit your needs. Using the forwarder does give you the most flexibility. Using the transformer is certainly a reasonable approach, I'm more concerned about mitigating the costs. Scoped services introduce a lot of per request overhead in CPU, allocations, etc.. While I appreciate your config can change a lot, I would still hope you're able to cache some of the artifacts like the transformer locally between changes.

For now I want to collect more requirements from other customers to see what would be most generally useful.

Tratcher avatar Feb 14 '23 00:02 Tratcher

Triage: We would consider making a change if there is reasonable demand. Putting to Backlog. Please upvote top post if you land here and you also want it to be fixed. Thanks!

karelz avatar Feb 14 '23 18:02 karelz