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

dns: add ForceAttemptHTTP2 #7215

Merged
merged 1 commit into from Dec 14, 2023
Merged

dns: add ForceAttemptHTTP2 #7215

merged 1 commit into from Dec 14, 2023

Conversation

jsha
Copy link
Contributor

@jsha jsha commented Dec 13, 2023

Per https://pkg.go.dev/net/http#hdr-HTTP_2:

The http package's Transport and Server both automatically enable HTTP/2 support for simple configurations.

and https://pkg.go.dev/net/http#Transport:

// If non-nil, HTTP/2 support may not be enabled by default.
TLSClientConfig *tls.Config

Since we were setting a non-default TLSClientConfig to trust custom roots, we accidentally turned off HTTP/2 support. And Unbound requires HTTP/2 to serve DoH queries.

Also, clone the TLS config just to be safe against possible mutation in other packages.

Also, clone the TLS config just to be safe
@jsha jsha requested a review from a team as a code owner December 13, 2023 23:05
@jsha jsha requested a review from aarongable December 13, 2023 23:05
@pgporada
Copy link
Member

The default http.Transport used by a default http.Client sets ForceAttemptHTTP2: true. I don't believe we accidentally turned off http/2, I think that the field of rakes that determine when http/2 is disabled is at fault and we happened to inadvertently walk into said field. ForceAttemptHTTP2 itself seems poorly worded given that the doc states "Transport and Server both automatically enable HTTP/2 support..." which is great, why would you need to force it and dig into this boolean?

@pgporada pgporada merged commit 81e04ab into main Dec 14, 2023
12 checks passed
@pgporada pgporada deleted the forceattempthttp2 branch December 14, 2023 03:18
@jsha
Copy link
Contributor Author

jsha commented Dec 14, 2023

@pgporada We did accidentally turn off http/2, because we created a new Transport from scratch, so we did not inherit the ForceAttemptHTTP2: true setting from the default transport. Probably it would have been better to do http.DefaultTransport.Clone() and modify from there as desired. I'll send a followup PR.

@aarongable
Copy link
Contributor

(I think Phil was saying that it wasn't our mistake, not that it didn't happen.)

aarongable pushed a commit that referenced this pull request Dec 15, 2023
Besides inheriting the ForceAttemptHTTP2 setting, this inherits
reasonable defaults for MaxIdleConns, IdleConnTimeout, DialTimeout, and
so on.

Follow-up for #7215
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.

None yet

3 participants