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.

Sign up for GitHub

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

Jump to bottom

Disable HTTP2 while proxying a "Connection: upgrade" request #88781

Merged
merged 1 commit into from Mar 6, 2020
Merged

Disable HTTP2 while proxying a "Connection: upgrade" request #88781

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
28 changes: 27 additions & 1 deletion staging/src/k8s.io/apimachinery/pkg/util/proxy/dial.go
View file
Open in desktop
Expand Up @@ -30,7 +30,12 @@ import (
"k8s.io/apimachinery/third_party/forked/golang/netutil"
)

func DialURL(ctx context.Context, url *url.URL, transport http.RoundTripper) (net.Conn, error) {
// dialURL will dial the specified URL using the underlying dialer held by the passed
// RoundTripper. The primary use of this method is to support proxying upgradable connections.
// For this reason this method will prefer to negotiate http/1.1 if the URL scheme is https.
// If you wish to ensure ALPN negotiates http2 then set NextProto=[]string{"http2"} in the
// TLSConfig of the http.Transport
func dialURL(ctx context.Context, url *url.URL, transport http.RoundTripper) (net.Conn, error) {
dialAddr := netutil.CanonicalAddr(url)

dialer, err := utilnet.DialerFor(transport)
Expand Down Expand Up @@ -81,6 +86,15 @@ func DialURL(ctx context.Context, url *url.URL, transport http.RoundTripper) (ne
tlsConfigCopy.ServerName = inferredHost
tlsConfig = tlsConfigCopy
}

// Since this method is primary used within a "Connection: Upgrade" call we assume the caller is
Copy link
Member

Choose a reason for hiding this comment

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

What does "Connection: Upgrade" mean? Is it a term?

Copy link
Member

Choose a reason for hiding this comment

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

https://tools.ietf.org/html/rfc7230#section-6.7

We upgrade http requests to websocket or spdy for streaming request like exec and logs. The headers communicates that you are switching protocols from http1/1 to X so when the upgrade is complete you can send whatever over it like it's just a tcp connection. Sending an upgrade request over http/2 is a connection error because underlying tcp connections are shared between requests. We can eventually migrate off of http1/1 altogether but we are waiting for go support to land. golang/go#27244 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. It's an http header.
nit: should be "Connection: upgrade" according to the RFC.

// going to write HTTP/1.1 request to the wire. http2 should not be allowed in the TLSConfig.NextProtos,
// so we explicitly set that here. We only do this check if the TLSConfig support http/1.1.
if supportsHTTP11(tlsConfig.NextProtos) {
tlsConfig = tlsConfig.Clone()
tlsConfig.NextProtos = []string{"http/1.1"}
Copy link
Member

Choose a reason for hiding this comment

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

The above block tries hard not to modify the tlsConfig that is passed in. Do we need to make a copy here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're correct, I thought it was already copied or new at this point but I see I'm wrong.

}

tlsConn = tls.Client(netConn, tlsConfig)
if err := tlsConn.Handshake(); err != nil {
netConn.Close()
Expand Down Expand Up @@ -115,3 +129,15 @@ func DialURL(ctx context.Context, url *url.URL, transport http.RoundTripper) (ne
return nil, fmt.Errorf("Unknown scheme: %s", url.Scheme)
}
}

func supportsHTTP11(nextProtos []string) bool {
if len(nextProtos) == 0 {
return true
}
for _, proto := range nextProtos {
if proto == "http/1.1" {
return true
}
}
return false
}
28 changes: 26 additions & 2 deletions staging/src/k8s.io/apimachinery/pkg/util/proxy/dial_test.go
View file
Open in desktop
Expand Up @@ -49,6 +49,7 @@ func TestDialURL(t *testing.T) {
TLSConfig *tls.Config
Dial utilnet.DialFunc
ExpectError string
ExpectProto string
}{
"insecure": {
TLSConfig: &tls.Config{InsecureSkipVerify: true},
Expand Down Expand Up @@ -90,13 +91,28 @@ func TestDialURL(t *testing.T) {
TLSConfig: &tls.Config{InsecureSkipVerify: false, RootCAs: roots, ServerName: "example.com"},
Dial: d.DialContext,
},
"ensure we use http2 if specified": {
TLSConfig: &tls.Config{InsecureSkipVerify: false, RootCAs: roots, ServerName: "example.com", NextProtos: []string{"http2"}},
Dial: d.DialContext,
ExpectProto: "http2",
},
"ensure we use http/1.1 if unspecified": {
TLSConfig: &tls.Config{InsecureSkipVerify: false, RootCAs: roots, ServerName: "example.com"},
Dial: d.DialContext,
ExpectProto: "http/1.1",
},
"ensure we use http/1.1 if available": {
TLSConfig: &tls.Config{InsecureSkipVerify: false, RootCAs: roots, ServerName: "example.com", NextProtos: []string{"http2", "http/1.1"}},
Dial: d.DialContext,
ExpectProto: "http/1.1",
},
}

for k, tc := range testcases {
func() {
ts := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {}))
defer ts.Close()
ts.TLS = &tls.Config{Certificates: []tls.Certificate{cert}}
ts.TLS = &tls.Config{Certificates: []tls.Certificate{cert}, NextProtos: []string{"http2", "http/1.1"}}
ts.StartTLS()

// Make a copy of the config
Expand Down Expand Up @@ -127,7 +143,7 @@ func TestDialURL(t *testing.T) {
u, _ := url.Parse(ts.URL)
_, p, _ := net.SplitHostPort(u.Host)
u.Host = net.JoinHostPort("127.0.0.1", p)
conn, err := DialURL(context.Background(), u, transport)
conn, err := dialURL(context.Background(), u, transport)

// Make sure dialing doesn't mutate the transport's TLSConfig
if !reflect.DeepEqual(tc.TLSConfig, tlsConfigCopy) {
Expand All @@ -143,6 +159,14 @@ func TestDialURL(t *testing.T) {
}
return
}

tlsConn := conn.(*tls.Conn)
if tc.ExpectProto != "" {
if tlsConn.ConnectionState().NegotiatedProtocol != tc.ExpectProto {
t.Errorf("%s: expected proto %s, got %s", k, tc.ExpectProto, tlsConn.ConnectionState().NegotiatedProtocol)
}
}

conn.Close()
if tc.ExpectError != "" {
t.Errorf("%s: expected error %q, got none", k, tc.ExpectError)
Expand Down
8 changes: 1 addition & 7 deletions staging/src/k8s.io/apimachinery/pkg/util/proxy/upgradeaware.go
View file
Open in desktop
Expand Up @@ -384,10 +384,6 @@ func (h *UpgradeAwareHandler) tryUpgrade(w http.ResponseWriter, req *http.Reques
return true
}

func (h *UpgradeAwareHandler) Dial(req *http.Request) (net.Conn, error) {
return dial(req, h.Transport)
}

func (h *UpgradeAwareHandler) DialForUpgrade(req *http.Request) (net.Conn, error) {
if h.UpgradeTransport == nil {
return dial(req, h.Transport)
Expand All @@ -414,7 +410,7 @@ func getResponse(r io.Reader) (*http.Response, []byte, error) {

// dial dials the backend at req.URL and writes req to it.
func dial(req *http.Request, transport http.RoundTripper) (net.Conn, error) {
conn, err := DialURL(req.Context(), req.URL, transport)
conn, err := dialURL(req.Context(), req.URL, transport)
if err != nil {
return nil, fmt.Errorf("error dialing backend: %v", err)
}
Expand All @@ -427,8 +423,6 @@ func dial(req *http.Request, transport http.RoundTripper) (net.Conn, error) {
return conn, err
}

var _ utilnet.Dialer = &UpgradeAwareHandler{}

func (h *UpgradeAwareHandler) defaultProxyTransport(url *url.URL, internalTransport http.RoundTripper) http.RoundTripper {
scheme := url.Scheme
host := url.Host
Expand Down
19 changes: 19 additions & 0 deletions staging/src/k8s.io/apimachinery/pkg/util/proxy/upgradeaware_test.go
View file
Open in desktop
Expand Up @@ -355,6 +355,25 @@ func TestProxyUpgrade(t *testing.T) {
ServerFunc: httptest.NewServer,
ProxyTransport: nil,
},
"both client and server support http2, but force to http/1.1 for upgrade": {
ServerFunc: func(h http.Handler) *httptest.Server {
cert, err := tls.X509KeyPair(exampleCert, exampleKey)
if err != nil {
t.Errorf("https (invalid hostname): proxy_test: %v", err)
}
ts := httptest.NewUnstartedServer(h)
ts.TLS = &tls.Config{
Certificates: []tls.Certificate{cert},
NextProtos: []string{"http2", "http/1.1"},
}
ts.StartTLS()
return ts
},
ProxyTransport: utilnet.SetTransportDefaults(&http.Transport{TLSClientConfig: &tls.Config{
NextProtos: []string{"http2", "http/1.1"},
InsecureSkipVerify: true,
}}),
},
"https (invalid hostname + InsecureSkipVerify)": {
ServerFunc: func(h http.Handler) *httptest.Server {
cert, err := tls.X509KeyPair(exampleCert, exampleKey)
Expand Down