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

http3: prefer IPv6 over IPv4 when dialing #3770

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

marten-seemann
Copy link
Member

Fixes #3755.

I'm not sure this is the correct fix. It seems like we should be able to handle multiple DNS responses, and fall back from IPv6 to IPv4 if IPv6 is not available (for example). That would be a bigger change, and would probably need to be done in the quic and not the http3 package.

I'm not sure this is the correct fix. It seems like we should be able to handle
multiple DNS responses, and fall back from IPv6 to IPv4 if IPv6 is not
available (for example). That would be a bigger change, and would probably need
to be done in the quic and not the http3 package.
@codecov
Copy link

codecov bot commented Apr 13, 2023

Codecov Report

Merging #3770 (53b9550) into master (af517bd) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #3770   +/-   ##
=======================================
  Coverage   84.49%   84.49%           
=======================================
  Files         142      142           
  Lines       14114    14118    +4     
=======================================
+ Hits        11925    11929    +4     
  Misses       1769     1769           
  Partials      420      420           
Impacted Files Coverage Δ
http3/roundtrip.go 89.44% <100.00%> (+0.31%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@kgersen
Copy link

kgersen commented Apr 14, 2023

this doesn't fix the issue in our app because we use this kind of code:

	rtTransport = &http3.RoundTripper{
		DisableCompression: true,
		TLSClientConfig:    netTransport.TLSClientConfig,
		QuicConfig:         quicConf,
		Dial: func(ctx context.Context, addr string, tlsCfg *tls.Config, cfg *quic.Config) (quic.EarlyConnection, error) {
			j.Logger().Debug().Msgf("HTTP3 dialing to: %s", addr)
			return quic.DialAddrEarlyContext(ctx, addr, tlsCfg, cfg)
		},
	}

cause => quic.DialAddrEarlyContext also uses net.ResolveUDPAddr thru dialAddrContext

@marten-seemann
Copy link
Member Author

If you're setting the Dial option yourself, you can implement a similar workaround as in this PR.

Issue for the fix of DialAddr here: #3772.

@kgersen
Copy link

kgersen commented Apr 14, 2023

Does this means quic.DialAddrEarlyContext and quic.DialAddrContext will always prefer IPv4 ?

I've updated h3dial with a use case of a custom http3.RoundTripper.Dial (which should be called DialContext ?)

@marten-seemann
Copy link
Member Author

Does this means quic.DialAddrEarlyContext and quic.DialAddrContext will always prefer IPv4 ?

It's also using ResolveUDPAddr, so I'd assume so. See #3772.

Copy link
Member

@lucas-clemente lucas-clemente left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels unfortunate to have a workaround here rather than fix this in the stdlib, but I agree with the agenda of pushing ipv6 ;)

@marten-seemann
Copy link
Member Author

The problem now is that if you do a Get("https://localhost:1234"), it only does IPv6, which doesn't work if you're just listening on IPv4.

@bt90
Copy link
Contributor

bt90 commented Jun 5, 2023

Isn't the fallback mechanism defined by the IETF?

https://en.m.wikipedia.org/wiki/Happy_Eyeballs

@bt90
Copy link
Contributor

bt90 commented Jun 5, 2023

@bt90
Copy link
Contributor

bt90 commented Jun 9, 2023

This is also what curl is doing as explained in @bagder blog post:

https://daniel.haxx.se/blog/2023/01/12/

@kgersen
Copy link

kgersen commented Jun 9, 2023

Fallback (happy eyeball) is another issue. see #3772

@bt90
Copy link
Contributor

bt90 commented Jun 9, 2023

Strictly speaking, the preference of IPv6 is part of happy eyeballs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

http3: client always prefers IPv4
4 participants