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

ibuildthecloud · 2020-03-03T23:02:45Z

When proxying connection upgrade requests, like websockets, we dial
the target and then manually write the http.Request to the wire,
bypassing the http.Client. In this scenario we are by default using
HTTP/1.1 from both the client and to the target server we are proxying.
Because of this we must disable HTTP2 in the TLS handshake so that the
server does not think we are writing a HTTP2 request. We do this by
setting the TLSConfig.NextProtos field to "http/1.1".

fix #88782

NONE

k8s-ci-robot · 2020-03-03T23:03:54Z

Hi @ibuildthecloud. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

ibuildthecloud · 2020-03-03T23:09:59Z

Fixes #88782

liggitt · 2020-03-04T00:21:01Z

Thanks, can we exercise the bug and fix with a test?

mikedanese · 2020-03-04T03:41:40Z

Doesn't this diable http/2 for all proxied requests? Why not just use:

kubernetes/staging/src/k8s.io/apimachinery/pkg/util/proxy/upgradeaware.go

Lines 65 to 67 in aeb88b6

    
           // UpgradeTransport, if specified, will be used as the backend transport when upgrade requests are provided. 
        
           // This allows clients to disable HTTP/2. 
        
           UpgradeTransport UpgradeRequestRoundTripper

And pass in a transport with http/2 disabled?

ibuildthecloud · 2020-03-04T18:56:31Z

@mikedanese This disables http2 for only times that we use GetDialer which is only when we are handling a connection upgrade. Correct me if I'm wrong, but connection upgrade is a http/1.1 concept so it is required to disable http2. If the user passes in a transport to disable http2 that will disable http2 for all proxied request. We'd prefer to use http2 on http2 compatible requests. This approach disables http2 only when we know it is explicitly disallowed.

ibuildthecloud · 2020-03-04T19:02:54Z

@liggitt will add test.

ibuildthecloud · 2020-03-04T20:59:15Z

@liggitt Updated with tests.

mikedanese · 2020-03-05T01:25:26Z

Yes, we shouldn't be using http/2 for upgrade requests. I thought we hit it here:

kubernetes/staging/src/k8s.io/apimachinery/pkg/util/proxy/upgradeaware.go

Line 388 in e865c0b

return dial(req, h.Transport)

But that function is unused now. This is all pretty messy at this point and is due for some cleanup.

mikedanese · 2020-03-05T01:26:46Z

staging/src/k8s.io/apimachinery/pkg/util/proxy/dial.go

+			// 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.NextProtos = []string{"http/1.1"}


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

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

mikedanese · 2020-03-05T01:41:56Z

Can we either:

Document that DialURL won't negotiate http/2 with ALPN.
Make DialURL private since we don't use it anywhere else.

Alternatively, we could probably fix this outside of dial url by making sure that DialForUpgrade only uses a transport that has http/2 disabled.

ibuildthecloud · 2020-03-05T04:29:26Z

@mikedanese I added documentation to the DialURL method to describe it's behavior regarding http2 and http/1.1.

mikedanese · 2020-03-05T15:56:57Z

staging/src/k8s.io/apimachinery/pkg/util/proxy/upgradeaware_test.go

@@ -355,6 +355,25 @@ func TestProxyUpgrade(t *testing.T) {
 			ServerFunc:     httptest.NewServer,
 			ProxyTransport: nil,
 		},
+		"booth client and server support http2, but force to http/1.1 for upgrade": {


Suggested change

"booth client and server support http2, but force to http/1.1 for upgrade": {

"both client and server support http2, but force to http/1.1 for upgrade": {

mikedanese · 2020-03-05T15:57:32Z

staging/src/k8s.io/apimachinery/pkg/util/proxy/upgradeaware_test.go

@@ -355,6 +355,25 @@ func TestProxyUpgrade(t *testing.T) {
 			ServerFunc:     httptest.NewServer,
 			ProxyTransport: nil,
 		},
+		"booth client and server support http2, but force to http/1.1 for upgrade": {


Suggested change

"booth client and server support http2, but force to http/1.1 for upgrade": {

"both client and server support http2, but force to http/1.1 for upgrade": {

caesarxuchao · 2020-03-05T22:22:38Z

staging/src/k8s.io/apimachinery/pkg/util/proxy/dial.go

@@ -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


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

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)

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

caesarxuchao · 2020-03-05T22:23:21Z

staging/src/k8s.io/apimachinery/pkg/util/proxy/dial.go

+// 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) {


Let's make it private if it doesn't k/k.

caesarxuchao · 2020-03-05T22:25:09Z

In the PR description (the first comment), can you add "fix #88782"?

mikedanese · 2020-03-05T23:56:57Z

are you saying the UpgradeAwareHandler.Dial is not being used anymore?

Yup, I was kind of confused since it's the only thing that might dial for no upgrade requests so I deleted it and did a full compile. No usage apparently.

ibuildthecloud · 2020-03-06T00:30:06Z

I removed UpgradeAwareHandler.Dial and renamed DialURL to be private.

When proxying connection upgrade requests, like websockets, we dial the target and then manually write the http.Request to the wire, bypassing the http.Client. In this scenario we are by default using HTTP/1.1 from both the client and to the target server we are proxying. Because of this we must disable HTTP2 in the TLS handshake so that the server does not think we are writing a HTTP2 request. We do this by setting the TLSConfig.NextProtos field to "http/1.1". Signed-off-by: Darren Shepherd <darren@rancher.com>

mikedanese · 2020-03-06T00:34:07Z

/lgtm

mikedanese · 2020-03-06T00:46:21Z

Tentatively tagged for 1.18 since it fixes a bug and we haven't blown the final whistle yet.

mikedanese · 2020-03-06T02:24:59Z

/retest

mikedanese · 2020-03-06T04:01:47Z

/retest

caesarxuchao · 2020-03-06T04:16:58Z

This lgtm, too.

caesarxuchao · 2020-03-06T04:19:00Z

/test pull-kubernetes-e2e-gce-network-proxy

Run the tests since this is on the path of the apiserver network proxy. Though it won't pass until #88863 gets merged

mikedanese · 2020-03-06T04:34:49Z

@caesarxuchao still needs approval.

caesarxuchao · 2020-03-06T06:18:45Z

/test pull-kubernetes-e2e-gce-network-proxy

caesarxuchao · 2020-03-06T06:19:51Z

/approve
/hold

I want to make sure pull-kubernetes-e2e-gce-network-proxy passes before merging this.

This is a bug fix so I think it can merge after code freeze.

k8s-ci-robot · 2020-03-06T06:20:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: caesarxuchao, ibuildthecloud

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~staging/src/k8s.io/apimachinery/pkg/OWNERS~~ [caesarxuchao]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

caesarxuchao · 2020-03-06T06:20:38Z

@mikedanese @ibuildthecloud feel free to remove the hold if pull-kubernetes-e2e-gce-network-proxy passes.

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Mar 3, 2020

k8s-ci-robot requested review from mikedanese and smarterclayton March 3, 2020 23:06

ibuildthecloud mentioned this pull request Mar 3, 2020

Proxying websocket to http2 capable server failed with malformed response #88782

Closed

ibuildthecloud force-pushed the master branch from 7b6abe1 to 4884f5b Compare March 4, 2020 20:37

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 4, 2020

ibuildthecloud force-pushed the master branch from 4884f5b to 0d41308 Compare March 4, 2020 20:58

mikedanese reviewed Mar 5, 2020

View reviewed changes

ibuildthecloud force-pushed the master branch from 0d41308 to e5d35e8 Compare March 5, 2020 04:26

mikedanese reviewed Mar 5, 2020

View reviewed changes

caesarxuchao reviewed Mar 5, 2020

View reviewed changes

ibuildthecloud force-pushed the master branch 2 times, most recently from 27268da to 77d1c7a Compare March 6, 2020 00:29

ibuildthecloud force-pushed the master branch from 77d1c7a to eb9cf77 Compare March 6, 2020 00:31

k8s-ci-robot assigned mikedanese Mar 6, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 6, 2020

mikedanese added this to the v1.18 milestone Mar 6, 2020

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 6, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 6, 2020

mikedanese removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 6, 2020

k8s-ci-robot merged commit 7c7ce47 into kubernetes:master Mar 6, 2020

wansir mentioned this pull request May 7, 2020

fix list namespaces kubesphere/kubesphere#2051

Merged

zryfish mentioned this pull request May 12, 2020

fix websocket bug kubesphere/kubesphere#2058

Merged

liggitt mentioned this pull request Sep 11, 2023

Stream Translator Proxy and FallbackExecutor for WebSockets #119186

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ibuildthecloud commented Mar 3, 2020 •

edited

k8s-ci-robot commented Mar 3, 2020

ibuildthecloud commented Mar 3, 2020

liggitt commented Mar 4, 2020

mikedanese commented Mar 4, 2020

ibuildthecloud commented Mar 4, 2020

ibuildthecloud commented Mar 4, 2020

ibuildthecloud commented Mar 4, 2020

mikedanese commented Mar 5, 2020

mikedanese Mar 5, 2020

ibuildthecloud Mar 5, 2020

mikedanese commented Mar 5, 2020

ibuildthecloud commented Mar 5, 2020

mikedanese Mar 5, 2020

ibuildthecloud Mar 5, 2020

mikedanese Mar 5, 2020

ibuildthecloud Mar 5, 2020

caesarxuchao Mar 5, 2020

mikedanese Mar 6, 2020

caesarxuchao Mar 6, 2020

caesarxuchao Mar 5, 2020

caesarxuchao commented Mar 5, 2020

mikedanese commented Mar 5, 2020

ibuildthecloud commented Mar 6, 2020

mikedanese commented Mar 6, 2020

mikedanese commented Mar 6, 2020

mikedanese commented Mar 6, 2020

mikedanese commented Mar 6, 2020

caesarxuchao commented Mar 6, 2020

caesarxuchao commented Mar 6, 2020

mikedanese commented Mar 6, 2020

caesarxuchao commented Mar 6, 2020

caesarxuchao commented Mar 6, 2020

k8s-ci-robot commented Mar 6, 2020

caesarxuchao commented Mar 6, 2020

	"booth client and server support http2, but force to http/1.1 for upgrade": {
	"both client and server support http2, but force to http/1.1 for upgrade": {

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

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

Conversation

ibuildthecloud commented Mar 3, 2020 • edited

k8s-ci-robot commented Mar 3, 2020

ibuildthecloud commented Mar 3, 2020

liggitt commented Mar 4, 2020

mikedanese commented Mar 4, 2020

ibuildthecloud commented Mar 4, 2020

ibuildthecloud commented Mar 4, 2020

ibuildthecloud commented Mar 4, 2020

mikedanese commented Mar 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikedanese commented Mar 5, 2020

ibuildthecloud commented Mar 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caesarxuchao commented Mar 5, 2020

mikedanese commented Mar 5, 2020

ibuildthecloud commented Mar 6, 2020

mikedanese commented Mar 6, 2020

mikedanese commented Mar 6, 2020

mikedanese commented Mar 6, 2020

mikedanese commented Mar 6, 2020

caesarxuchao commented Mar 6, 2020

caesarxuchao commented Mar 6, 2020

mikedanese commented Mar 6, 2020

caesarxuchao commented Mar 6, 2020

caesarxuchao commented Mar 6, 2020

k8s-ci-robot commented Mar 6, 2020

caesarxuchao commented Mar 6, 2020

ibuildthecloud commented Mar 3, 2020 •

edited