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

Enable scatter gather scenarios #2386

Closed
danielmarbach opened this issue Jan 25, 2024 · 4 comments
Closed

Enable scatter gather scenarios #2386

danielmarbach opened this issue Jan 25, 2024 · 4 comments
Assignees
Labels
Type: Idea This issue is a high-level idea for discussion.

Comments

@danielmarbach
Copy link

For context this is the scenario dotnet/aspnetcore#53565

@Tratcher pointed out we might only need the capability to create the HttpRequestMessage and run the transforms.

If we had this capability in place in Yarp we wouldn't have to copy the HttpContext for scatter gather scenarios.

Would be awesome to have some utility of some sort within the Yarp infrastructure.

@MihaZupan
Copy link
Member

For a scatter scenario, would you want to send anything other than GET requests?

If you want to create new scatter requests, all that is involved is along the lines of

async Task<HttpRequestMessage> CreateScatterRequestAsync(HttpContext context, string destinationPrefix, CancellationToken cancellationToken = default)
{
    if (context.Request.Method != HttpMethods.Get || context.WebSockets.IsWebSocketRequest)
    {
        throw new NotSupportedException("...");
    }

    var request = new HttpRequestMessage
    {
        Method = HttpMethod.Get,
        Version = HttpVersion.Version20,
        VersionPolicy = HttpVersionPolicy.RequestVersionOrLower
    };

    await ScatterTransformer.Instance.TransformRequestAsync(context, request, destinationPrefix, cancellationToken);

    // If you're not setting it in your transformer already
    request.RequestUri ??= RequestUtilities.MakeDestinationAddress(destinationPrefix, context.Request.Path, context.Request.QueryString);

    return request;
}

Most of the YARP logic around creating requests is around handling things like WebSocket version upgrades/downgrades and dealing with a custom HttpContent for POST requests.

In practice the above example is just setting the Method & Version, then calling the transformer. I'm not sure that is all that helpful / generic enough to have in YARP.

@danielmarbach
Copy link
Author

Thanks for following up! Yes it would be Get only.

Yeah this code would work. We could then simply pass the request message to the HttpMessageInvoker.

I figured I raise it here since you might be want to consider a scatter gather like abstraction similar to the forwarder interface

@MihaZupan
Copy link
Member

We have a general issue to track traffic mirroring (#105), but that is mainly about duplicating outgoing requests and potentially performing validation on different responses. The forwarded response still comes from a single destination.

Splitting requests and then merging responses like in your scenario is bound to involve custom logic to specify how requests are generated, how merging should work, what policies to use for some failed/delayed responses, etc. I'm not aware of an established practice that one would follow here.
Given the highly custom nature of such scenarios (and limited examples of different users doing so), I don't think we should be trying to design an abstraction here, as it's not clear what benefits it would provide.

Closing this issue as wont-fix, but we can of course revisit the decision in the future if there's more user demand and we believe we can provide value.

@danielmarbach
Copy link
Author

Absolutely agree with the reasoning. Thanks for taking the time to reply

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

2 participants