Repository navigation
Conversation
With no HTTPClient set, Connect shared http.DefaultTransport's tls.Config and set InsecureSkipVerify on it, so an Insecure client turned off certificate checks for later requests through the default transport, and parallel connects raced on the shared config (stmcginnis#567). Clone the config as stmcginnis#568 does, and set ForceAttemptHTTP2 as stmcginnis#518 does so the transport still negotiates HTTP/2.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Port of the fix for stmcginnis#567. With
ClientConfig.HTTPClientnil, Connect built a transport that sharedhttp.DefaultTransport'stls.Configpointer, then setInsecureSkipVerifyon it whenInsecurewas true. net/http gives the default transport a non-nil config on its first HTTPS request, so after that, an Insecure connect turned off certificate checks for every laterhttp.Getorhttp.DefaultClientcall in the program. Parallel Connects also wrote the shared config at the same time, whichgo test -racereports.Changes
tls.Configinstead of sharing it. This ports upstream stmcginnis/gofish#568.ForceAttemptHTTP2on the built transport and in the modify path. This ports upstream stmcginnis/gofish#518. net/http turns HTTP/2 off on a transport with its ownTLSClientConfigorDialContext. Without the clone, this change would also make net/http writeNextProtosinto the shared config, so the two land together.http.DefaultClientstill rejects a self-signed cert. Parallel Insecure connects pass-race. The client negotiates HTTP/2 with a server that offers it. Each test fails on main.Both are hand ports into the fork's client.go. Nothing was cherry-picked from upstream.
Compatibility
No API change. Transports that gofish builds, and caller transports it modifies (
NoModifyTransportfalse), now negotiate HTTP/2 when the server offers it. Callers that pass their own client withNoModifyTransport: truesee no change. A program that needs HTTP/1.1 against a server with a broken HTTP/2 stack can setGODEBUG=http2client=0.