microsoft / reverse-proxy

A toolkit for developing high-performance HTTP reverse proxy applications.
https://microsoft.github.io/reverse-proxy
MIT License
8.61k stars 844 forks source link

Provide option to configure HttpClient settings per Route, but not only per cluster. #2609

Closed Petar-Vutov closed 1 month ago

Petar-Vutov commented 2 months ago

What should we add or change to make your life better?

We are using YARP in order to proxy REST and WebSocket (WS) request to the backend. And one and the same service (server, container, cluster) exposes both REST and WS APIs (for one and the same domain and data). Something like following structure (simplified): YARP issue

WebSocket routes however have different requirements for the HttpClient compared to the REST ones. Manly in terms of "ActivityTimeout", as the default one (100 seconds) is too less for a WebSocket channel. Because sometimes we have idle periods with no messages.

Why is this important to you?

In order to achieve difference in the idle timeout, we need to define different clusters (situation right now). Which makes configuration looking more complex, harder to read and redundant (actually having 2 cluster definitions for one and the same background service - for every background service).

Some thoughts about the restrictions.

Someone could argue that we need to separate implementations of REST and WS channels to different service anyway. But actually those are APIs on one and the same data and domain. You could imagine them like REST version for data pulling and WS for data push (evening). And we have both for historical reasons. The are all public and some of the integrators prefer one and some the other.

As per the idle timeout - most of the WS APIs do have ping-pong logic (client sending "I'm alive"). But time interval for sending those is configurable (server side). And all this is deployed on customer side (mainly on-prem). Some customers want to decrease the (empty) chatting for those APIs. Thus we have the setting and it could be extended more that the 100 seconds default timeout.

MihaZupan commented 2 months ago

Instead of maintaining separate configurations for WS requests, have you considered using the request timeouts feature? This is distinct from ActivityTimeout in that it is a flat duration, and it does not affect WebSocket connections. That means you could e.g. set a Timeout sufficient for regular requests, and then increase the ActivityTimeout to a level you're comfortable with for WebSoceket requests.

Petar-Vutov commented 2 months ago

Thank you, @MihaZupan ! This sounds interesting (request timeouts) and gives some additional flexibility. However, I'm not sure we would give it a try. We do have much more http routes than ws ones. So would need to repeat the timeout configuration on many of them (even if it is implemented with a policy.

MihaZupan commented 2 months ago

Note that modifying existing clusters/routes is easier than creating new ones. For example, you can programatically add timeouts to all routes with a config filter like so:

builder.Services.AddReverseProxy()
    .LoadFromConfig(builder.Configuration.GetSection("ReverseProxy"))
    .AddConfigFilter<TimeoutsConfigFilter>();

internal sealed class TimeoutsConfigFilter : IProxyConfigFilter
{
    public ValueTask<ClusterConfig> ConfigureClusterAsync(ClusterConfig cluster, CancellationToken cancel) =>
        new(cluster);

    public ValueTask<RouteConfig> ConfigureRouteAsync(RouteConfig route, ClusterConfig cluster, CancellationToken cancel) =>
        new(route with { Timeout = TimeSpan.FromSeconds(10) });
}
adityamandaleeka commented 1 month ago

Seems like this has been answered. Closing.

Petar-Vutov commented 1 month ago

I still disagree with @MihaZupan. Explicit configuration in JSON file is much more easier to handle and understand (for support etc) instead of code provided one. Not to mention that last proposal was to modify the default settings and set later via code the ones for REST APIs. And if you have in your configuration 2 routes for WebSockets and 20 for REST APIs, such set-up is mildly set contra-intuitive. What we've made with duplicating clusters is much more clear. Anyways ...