http3: set tls.Config.ServerName for outgoing requests, if unset

[Glonee]

fix #3865
bug caused by: d683b84#diff-4b667feae66c9d46b21b9ecc19e8958cf4472d162ce0a47ac3e8386af8bbd8cfL231-L239
Copied this logic into http3 package.

[codecov]

Codecov Report

Merging #3867 (739b92c) into master (fce0261) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3867      +/-   ##
==========================================
- Coverage   83.91%   83.88%   -0.03%     
==========================================
  Files         146      146              
  Lines       14383    14390       +7     
==========================================
+ Hits        12069    12070       +1     
- Misses       1873     1877       +4     
- Partials      441      443       +2     

Impacted Files	Coverage Δ
http3/client.go	85.80% <100.00%> (+0.32%)	⬆️

... and 3 files with indirect coverage changes

[marten-seemann]

@kgersen Could you check if this PR fixes the regression you reported?

[kgersen]

partially. it works for simple cases. but when the connection is reused, for instance when following a redirect, it panics:

go run example/client/main.go -q https://www.google.com/

ok

but

go run example/client/main.go -q https://google.com/

panic: connection already exists.

it's the default behavior of std lib http client to follow redirects so http3 client should too.

I haven't looked further why this panic occurs. if @Glonee can look at too ? otherwise I'll try to fix it.

v0.34.0 was fine and handled the redirect fine.

side note: the new 'Transport' (quic.Transport) introduced in v0.35.0 add a bit of confusion. In std lib, Transport is usually understood as http.Transport (or http2.Transport), ie the implementation of HTTP/x protocol over TCP. Here it's called http3.RoundTripper and quic.Transport is something not specific to HTTP/3.
It's fine, it is just names and different packages but it's not very consistent with the stdlib.

[marten-seemann]

@kgersen Thanks for testing.

panic: connection already exists.

The panic comes from here: https://github.com/quic-go/quic-go/blob/v0.35.0/multiplexer.go#L59. It shows that the http3 package is not using the quic.Transport correctly. That's a very annoying oversight, and shouldn't have made it into the final release. We definitely should have a test case for that (I'll create one right away).

side note: the new 'Transport' (quic.Transport) introduced in v0.35.0 add a bit of confusion. In std lib, Transport is usually understood as http.Transport (or http2.Transport), ie the implementation of HTTP/x protocol over TCP. Here it's called http3.RoundTripper and quic.Transport is something not specific to HTTP/3.

Noted. Improving documentation (both GoDoc and README) is on my TODO list.

[Glonee]

@kgersen Can you check if the problem still exist?

[kgersen]

seems ok. thanks.
Both the provided example and our product (once we adapted / changed from using net.UDPConn to using quic.Transport) worked fine.
We didn't launch extensive tests, just our basic ones. I'll conduct more tests next week.

[marten-seemann]

@Glonee Can you split the last 2 commits into a separate PR? It would also be great if we had a HTTP integration test that dials two QUIC connections. Any idea?

[Glonee]

@marten-seemann localhost and 127.0.0.1 are considered as different server names. Maybe we can dial to localhost and 127.0.0.1 to make two QUIC connections?

[marten-seemann]

Sounds like a good idea. You could change the self-signed certificate to be valid for both the IP and localhost:

quic-go/integrationtests/tools/crypto.go

Line 46 in fce0261

DNSNames: []string{"localhost"},

[marten-seemann]

Thank you @Glonee!