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

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

Open
Petar-Vutov opened this issue Sep 19, 2024 · 3 comments
Labels
needs-author-action An issue or pull request that requires more info or actions from the author. Type: Idea This issue is a high-level idea for discussion.

Comments

@Petar-Vutov
Copy link

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.

@Petar-Vutov Petar-Vutov added the Type: Idea This issue is a high-level idea for discussion. label Sep 19, 2024
@MihaZupan
Copy link
Member

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.

@MihaZupan MihaZupan added the needs-author-action An issue or pull request that requires more info or actions from the author. label Sep 24, 2024
@Petar-Vutov
Copy link
Author

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
Copy link
Member

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) });
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-author-action An issue or pull request that requires more info or actions from the author. Type: Idea This issue is a high-level idea for discussion.
Projects
None yet
Development

No branches or pull requests

2 participants