quic-go / quic-go

A QUIC implementation in pure Go
https://quic-go.net
MIT License
10.07k stars 1.31k forks source link

Add http3.ConfigureTransport #2190

Open VoyTechnology opened 4 years ago

VoyTechnology commented 4 years ago

Upgrading the connection makes it hard to keep the same configuration as before. I believe a http3.ConfigureTransport method should be added, just like the one in http2.ConfigureTransport

This is based on the comment in https://github.com/tsenart/vegeta/pull/453/files#r339342957 by @tsenart

Any chance we can "translate" already set configuration in an existing Transport to this http3.RoundTripper? Akin to what http2.ConfigureTransport(tr) does. Example: Not loosing existing TLSClientConfig.

This shouldn't be too hard to implement unless I am missing something.

tmcnicol commented 4 years ago

I would be interested in helping out here. Is there anything I need to know before getting started?

marten-seemann commented 4 years ago

@tmcnicol Go for it, a PR would be appreciated! I haven't thought a lot about this issue yet, please let me know if you run into any problems.

tmcnicol commented 4 years ago

The solution I have come up with is a function which copies the TLS config from the original client. This is obviosuly quite a deviation from the UpgradeTransport function, but since we are completely replacing the transport maybe this is ok.

func UpgradeClient(client *http.Client) {
    var qconf quic.Config
    transport, ok := client.Transport.(*http.Transport)
    if !ok {
        log.Printf("could not make the type assertion")
    }

    tlsconfig := transport.TLSClientConfig

    roundtripper := &http3.RoundTripper{
        TLSClientConfig: tlsconfig,
        QuicConfig:      &qconf,
    }
    client.Transport = roundtripper
}

Figured I would grab some feedback to see this was inline with what you had in mind.

marten-seemann commented 4 years ago

Why do you need to copy the TLS config?

tmcnicol commented 4 years ago

In the linked issue this was described as the desired behaviour, to copy the TLS config across from an already configured client.

marten-seemann commented 4 years ago

Which linked issue? The reason I'd like to avoid copying the config is because then key updates (via tls.Config.SetSessionTicketKeys()) won't work any more. Of course, if there's a compelling reason why we have to copy, we could still do that, and add documentation that makes users aware of this limitation.