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

[API Proposal]: HTTP/3 Add QuicConnectCallback #64449

Open
ManickaP opened this issue Jan 28, 2022 · 7 comments
Open

[API Proposal]: HTTP/3 Add QuicConnectCallback #64449

ManickaP opened this issue Jan 28, 2022 · 7 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Net.Http
Milestone

Comments

@ManickaP
Copy link
Member

Background and motivation

Since 5.0 we provide ability to customize socket-based connections in HttpClient via ConnectCallback. The same functionality was asked for in #48617 for QUIC and HTTP/3. Due to differences between QUIC and sockets/Stream we cannot reuse the existing callback. So we decided to add a new callback specifically for QUIC connections.

ConnectCallback served 2 different types of requirements:

  • completely replacing the transport (UNIX domain sockets, in-memory transport etc.)
  • using the same transport, but customize socket creation (setting up TCP keep alive, doing manual DNS, etc.)

For 7.0 milestone, we decided to only allow the second requirement, customizing QuicConnection creation (see usages). For the transport replacement we would need to provide and commit to QuicConnection/QuicStream abstractions, which we don't want to do yet. But we do plan to design these classes with potential to become abstract in the future, so we're not closing the door on the transport replacement.

Also for the callback to make sense we should expand QuicClientConnectionOptions with some useful settings (this will be part of separate API discussion), e.g.:

More details in my notes: https://gist.github.com/ManickaP/7a96748c6789f008e4e0597b9b883487

As a side note, related to this API is PlaintextStreamFilter callback which operates on streams. Once again, we would need QuicStream abstraction or enhancement to Stream itself to make it work. So we decided to ignore this callback for now (as we've done so far) and revisit it in later releases.

API Proposal

namespace System.Net.Http
{
    public sealed class SocketsHttpHandler : HttpMessageHandler
    {
        public Func<SocketsHttpConnectionContext, CancellationToken, ValueTask<Stream>>? ConnectCallback { get; set; }
+       public Func<SocketsHttpConnectionContext, CancellationToken, ValueTask<QuicConnection>>? QuicConnectCallback { get; set; }
    }
}

API Usage

socketsHandler.QuicConnectCallback = async (context, token) =>
{
   // Do custom DNS query: https://gist.github.com/ManickaP/7a96748c6789f008e4e0597b9b883487#custom-name-resolution
   // Set up connection differently: https://gist.github.com/ManickaP/7a96748c6789f008e4e0597b9b883487#connection-settings

   // Most simple connection creation bellow:
   var quicConnection = new QuicConnection(new QuicClientConnectionOptions() {
         RemoteEndPoint = context.DnsEndPoint,
         ClientAuthenticationOptions = new SslClientAuthenticationOptions() {
            ApplicationProtocols = new List<SslApplicationProtocol>() {
               SslApplicationProtocol.Http3
            }
         }
   });
   await quicConnection.ConnectAsync(token).ConfigureAwait(false);
   return quicConnection;
};

Alternative Designs

Not doing anything at the moment.

Risks

Due to limited usages of HTTP/3 and QUIC, newness of these protocols in our stack, we don't have much feedback yet. The design is based on requirements for previous HTTP versions and sockets. As a result, the need for this callback might be non-existent and we're adding new API with no usefulness.

On the other hand, if we do nothing we might never get any feedback and/or adoption and thus will never know how to improve this. Moreover, the API surface is small and the design is conservative with future in mind.

Mitigation might be hiding the API behind RequiresPreviewFeaturesAttribute which would let us change the API shape in the next release.

@ManickaP ManickaP added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jan 28, 2022
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Net.Http untriaged New issue has not been triaged by the area owner labels Jan 28, 2022
@ghost
Copy link

ghost commented Jan 28, 2022

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

Since 5.0 we provide ability to customize socket-based connections in HttpClient via ConnectCallback. The same functionality was asked for in #48617 for QUIC and HTTP/3. Due to differences between QUIC and sockets/Stream we cannot reuse the existing callback. So we decided to add a new callback specifically for QUIC connections.

ConnectCallback served 2 different types of requirements:

  • completely replacing the transport (UNIX domain sockets, in-memory transport etc.)
  • using the same transport, but customize socket creation (setting up TCP keep alive, doing manual DNS, etc.)

For 7.0 milestone, we decided to only allow the second requirement, customizing QuicConnection creation (see usages). For the transport replacement we would need to provide and commit to QuicConnection/QuicStream abstractions, which we don't want to do yet. But we do plan to design these classes with potential to become abstract in the future, so we're not closing the door on the transport replacement.

Also for the callback to make sense we should expand QuicClientConnectionOptions with some useful settings (this will be part of separate API discussion), e.g.:

More details in my notes: https://gist.github.com/ManickaP/7a96748c6789f008e4e0597b9b883487

As a side note, related to this API is PlaintextStreamFilter callback which operates on streams. Once again, we would need QuicStream abstraction or enhancement to Stream itself to make it work. So we decided to ignore this callback for now (as we've done so far) and revisit it in later releases.

API Proposal

namespace System.Net.Http
{
    public sealed class SocketsHttpHandler : HttpMessageHandler
    {
        public Func<SocketsHttpConnectionContext, CancellationToken, ValueTask<Stream>>? ConnectCallback { get; set; }
+       public Func<SocketsHttpConnectionContext, CancellationToken, ValueTask<QuicConnection>>? QuicConnectCallback { get; set; }
    }
}

API Usage

socketsHandler.QuicConnectCallback = async (context, token) =>
{
   // Do custom DNS query: https://gist.github.com/ManickaP/7a96748c6789f008e4e0597b9b883487#custom-name-resolution
   // Set up connection differently: https://gist.github.com/ManickaP/7a96748c6789f008e4e0597b9b883487#connection-settings

   // Most simple connection creation bellow:
   var quicConnection = new QuicConnection(new QuicClientConnectionOptions() {
         RemoteEndPoint = context.DnsEndPoint,
         ClientAuthenticationOptions = new SslClientAuthenticationOptions() {
            ApplicationProtocols = new List<SslApplicationProtocol>() {
               SslApplicationProtocol.Http3
            }
         }
   });
   await quicConnection.ConnectAsync(token).ConfigureAwait(false);
   return quicConnection;
};

Alternative Designs

Not doing anything at the moment.

Risks

Due to limited usages of HTTP/3 and QUIC, newness of these protocols in our stack, we don't have much feedback yet. The design is based on requirements for previous HTTP versions and sockets. As a result, the need for this callback might be non-existent and we're adding new API with no usefulness.

On the other hand, if we do nothing we might never get any feedback and/or adoption and thus will never know how to improve this. Moreover, the API surface is small and the design is conservative with future in mind.

Mitigation might be hiding the API behind RequiresPreviewFeaturesAttribute which would let us change the API shape in the next release.

Author: ManickaP
Assignees: -
Labels:

api-suggestion, area-System.Net.Http, untriaged

Milestone: -

@ManickaP ManickaP self-assigned this Jan 28, 2022
@ManickaP ManickaP added this to the 7.0.0 milestone Jan 28, 2022
@ManickaP ManickaP removed the untriaged New issue has not been triaged by the area owner label Jan 28, 2022
@JamesNK
Copy link
Member

JamesNK commented Jan 31, 2022

gRPC uses ConnectCallback for two things:

  1. Change the transport. Mostly this is about enabling IPC over UDS/named-pipes. I'm not sure if QUIC IPC makes as much sense. Not being able to change the QUIC transport is fine for now. People who want IPC can continue to use HTTP/2 over UDS.
  2. Track active connections. I did this by creating a NetworkStream and wrapping it in a new stream that would intercept Stream.Dispose. When SocketsHttpHandler closed a connection it disposed the stream. If QuicConnection is not abstract then that won't be possible. However, a subscribable event on QuicConnection that gets raised on connection closed would be a workable alternative.

I believe there are plans for connection management API on SocketsHttpHandler in .NET 7. Tracking active connections might get replaced by that in .NET 7.

@ManickaP
Copy link
Member Author

ManickaP commented Feb 2, 2022

Track active connections.

Do I get it right that this is for the load-balancer?
https://github.com/grpc/grpc-dotnet/blob/306abc297963f910888f9f4dd0abf654a2ad2305/src/Grpc.Net.Client/Balancer/Internal/BalancerHttpHandler.cs#L49

How critical the connection counting is for gRPC over HTTP/3?
Hypothetically, if we plan to eventually add abstractions in the future .NET release, would is still be critical to add at least some sort of callback in this one?

I'd love to understand the use case and necessity for this before we start adding other features on QuicConnection.

cc: @JamesNK

@ManickaP
Copy link
Member Author

ManickaP commented Feb 8, 2022

Even though it does not have a direct impact on this API, getting notified about QuicConnection close is not critical for gRPC in 7.0 time frame. So we'll keep this as-is without any additional callbacks for now.

@wegylexy
Copy link
Contributor

wegylexy commented Feb 9, 2022

Without the ability to replace the transport, does it mean impossible to inject a QuicConnection implementation that supports HTTP Datagram or WebTransport?

@ManickaP
Copy link
Member Author

ManickaP commented Feb 9, 2022

Yes, for now, that's the plan for 7.0. For future releases, we'll revisit the abstractions. We want some usage and feedback before going all in.

@ManickaP ManickaP removed their assignment Jul 5, 2022
@ManickaP ManickaP modified the milestones: 7.0.0, Future Jul 14, 2022
@WGroenestein
Copy link

I'm not too familiar with quic (and it's SNI), but a scenario I'm planning to use ConnectCallback for is to be able to manually resolve the DnsEnpoint to an IpAddress and put it through a blocklist (make sure it does not target an internal ip address) and then use the validated ip address to setup the connection. This is part of a SSRF prevention mechanism when the application need to be able to connect to addresses provided by an end user. Since this is not possible to be setup for quic, http3 should be disabled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Net.Http
Projects
None yet
Development

No branches or pull requests

4 participants