Option to enable http2 on client connections. #25280

timothysc · 2016-05-06T21:04:16Z

Addresses #21081

Enables http2 connection by default.

before:

Active Internet connections (servers and established)
Proto Recv-Q Send-Q Local Address           Foreign Address         State       PID/Program name    
tcp        0      0 127.0.0.1:10248         0.0.0.0:*               LISTEN      21868/kubelet       
tcp        0      0 127.0.0.1:10249         0.0.0.0:*               LISTEN      21924/kube-proxy    
tcp        0      0 0.0.0.0:22              0.0.0.0:*               LISTEN      754/sshd            
tcp        0      0 192.2.5.12:47080        192.2.5.11:443          ESTABLISHED 21924/kube-proxy    
tcp        0      0 192.2.5.12:47082        192.2.5.11:443          ESTABLISHED 21924/kube-proxy    
tcp        0      0 192.2.5.12:55776        169.254.169.254:80      ESTABLISHED 21868/kubelet       
tcp        0    304 192.2.5.12:22           192.2.5.10:49978        ESTABLISHED 22287/sshd: root [p 
tcp        0      0 192.2.5.12:34568        192.2.5.11:2379         ESTABLISHED 3029/flanneld       
tcp        0      0 192.2.5.12:55774        169.254.169.254:80      ESTABLISHED 21868/kubelet       
tcp        0      0 192.2.5.12:47086        192.2.5.11:443          ESTABLISHED 21924/kube-proxy    
tcp        0      0 192.2.5.12:47102        192.2.5.11:443          ESTABLISHED 21868/kubelet       
tcp        0      0 192.2.5.12:47108        192.2.5.11:443          ESTABLISHED 21868/kubelet       
tcp        0      0 192.2.5.12:55772        169.254.169.254:80      ESTABLISHED 21868/kubelet       
tcp        0      0 192.2.5.12:47104        192.2.5.11:443          ESTABLISHED 21868/kubelet       
tcp        0      0 192.2.5.12:34718        192.2.5.11:2379         ESTABLISHED 3029/flanneld       
tcp        0      0 192.2.5.12:47110        192.2.5.11:443          ESTABLISHED 21868/kubelet       
tcp        0      0 192.2.5.12:47106        192.2.5.11:443          ESTABLISHED 21868/kubelet       
tcp        0      0 192.2.5.12:47084        192.2.5.11:443          ESTABLISHED 21924/kube-proxy    
tcp6       0      0 :::4194                 :::*                    LISTEN      21868/kubelet       
tcp6       0      0 :::10250                :::*                    LISTEN      21868/kubelet       
tcp6       0      0 :::10255                :::*                    LISTEN      21868/kubelet       
tcp6       0      0 :::22                   :::*                    LISTEN      754/sshd            
tcp6       0      0 192.2.5.12:10255        192.2.5.15:41570        ESTABLISHED 21868/kubelet       
udp        0      0 0.0.0.0:8472            0.0.0.0:*                           -                   
udp        0      0 0.0.0.0:22824           0.0.0.0:*                           600/dhclient        
udp        0      0 0.0.0.0:68              0.0.0.0:*                           600/dhclient        
udp6       0      0 :::4195                 :::*                                600/dhclient

after

Active Internet connections (servers and established)
Proto Recv-Q Send-Q Local Address           Foreign Address         State       PID/Program name    
tcp        0      0 127.0.0.1:10248         0.0.0.0:*               LISTEN      16962/kubelet       
tcp        0      0 127.0.0.1:10249         0.0.0.0:*               LISTEN      17024/kube-proxy    
tcp        0      0 0.0.0.0:22              0.0.0.0:*               LISTEN      754/sshd            
tcp        0      0 192.2.5.12:47046        192.2.5.11:443          ESTABLISHED 17024/kube-proxy    
tcp        0      0 192.2.5.12:47052        192.2.5.11:443          ESTABLISHED 16962/kubelet       
tcp        0      0 192.2.5.12:34568        192.2.5.11:2379         ESTABLISHED 3029/flanneld       
tcp        0      0 192.2.5.12:55720        169.254.169.254:80      ESTABLISHED 16962/kubelet       
tcp        0      0 192.2.5.12:55724        169.254.169.254:80      ESTABLISHED 16962/kubelet       
tcp        0      0 192.2.5.12:34718        192.2.5.11:2379         ESTABLISHED 3029/flanneld       
tcp        0      0 192.2.5.12:55726        169.254.169.254:80      ESTABLISHED 16962/kubelet       
tcp        0    304 192.2.5.12:22           192.2.5.10:49938        ESTABLISHED 17653/sshd: root [p 
tcp6       0      0 :::4194                 :::*                    LISTEN      16962/kubelet       
tcp6       0      0 :::10250                :::*                    LISTEN      16962/kubelet       
tcp6       0      0 :::10255                :::*                    LISTEN      16962/kubelet       
tcp6       0      0 :::22                   :::*                    LISTEN      754/sshd            
tcp6       0      0 192.2.5.12:10255        192.2.5.15:41472        ESTABLISHED 16962/kubelet       
udp        0      0 0.0.0.0:8472            0.0.0.0:*                           -                   
udp        0      0 0.0.0.0:22824           0.0.0.0:*                           600/dhclient        
udp        0      0 0.0.0.0:68              0.0.0.0:*                           600/dhclient        
udp6       0      0 :::4195                 :::*                                600/dhclient

/cc @jeremyeder

timothysc · 2016-05-06T21:36:58Z

Of note, our servers already have http2 enabled b/c of golang 1.6.

wojtek-t · 2016-05-07T11:38:48Z

@kubernetes/sig-api-machinery

wojtek-t · 2016-05-09T06:40:54Z

I'm probably fine with this PR as soon as tests are passing.

timothysc · 2016-05-09T14:55:19Z

Moved the godep change into this PR, b/c the vendoring change forced me to redo it anyway.

timothysc · 2016-05-09T16:37:23Z

@k8s-bot test this please issue: #24211

krousey · 2016-05-09T16:39:33Z

pkg/util/net/http.go

@@ -67,6 +70,9 @@ func SetTransportDefaults(t *http.Transport) *http.Transport {
 if t.TLSHandshakeTimeout == 0 {
 t.TLSHandshakeTimeout = defaultTransport.TLSHandshakeTimeout
 }
+ if err := http2.ConfigureTransport(t); err != nil {


I'm confused. Doesn't 1.6.2 support http2 seamlessly? Why do we have to do this?

The servers are enabled by default, but not the clients.

exact reference: golang/go#14950

Tests we're run against 1.6.2 and can be reproduced independently.

Yep. And if you check the duped one golang/go#14391, the fix made it in to 1.6.2. So do we have to do this even with 1.6.2?

Sorry. Didn't see your last comment. According to the CL, would it be simpler to default ExpectContinueTimeout to 0: https://go-review.googlesource.com/#/c/22035/2/src/net/http/transport.go ?

I don't understand how a custom CA affects or is affected by http2. What am I missing?

In almost every client config we create in cluster (anything in a pod) the
config will have a custom CA, which will trigger a custom TLSClientConfig,
which would prevent http2 from being enabled by default.

On Mon, May 9, 2016 at 1:23 PM, krousey notifications@github.com wrote:

In pkg/util/net/http.go
#25280 (comment)
:

@@ -67,6 +70,9 @@ func SetTransportDefaults(t *http.Transport) *http.Transport {
if t.TLSHandshakeTimeout == 0 {
t.TLSHandshakeTimeout = defaultTransport.TLSHandshakeTimeout
}

if err := http2.ConfigureTransport(t); err != nil {

I don't understand how a custom CA affects or is affected by http2. What
am I missing?

—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/25280/files/53ddc93c6e80e750d09fa72680c718754e0693bc#r62537120

Picking through the upstream code, I now see why this has to be done. Thanks!

I only noticed it because I saw the comment above the line you pasted and
it intrigued me :)

We have the TLS transport cache so in practice this won't cause a
performance issue. We just need to be sure we're aware of any wrinkles.

On Mon, May 9, 2016 at 1:35 PM, krousey notifications@github.com wrote:

In pkg/util/net/http.go
#25280 (comment)
:

@@ -67,6 +70,9 @@ func SetTransportDefaults(t *http.Transport) *http.Transport {
if t.TLSHandshakeTimeout == 0 {
t.TLSHandshakeTimeout = defaultTransport.TLSHandshakeTimeout
}

if err := http2.ConfigureTransport(t); err != nil {

Picking through the upstream code, I now see why this has to be done.
Thanks!

—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/25280/files/53ddc93c6e80e750d09fa72680c718754e0693bc#r62539066

Yea, that's what I went back and read more closely too. My concern wasn't performance, but maintainability. I dislike having to import non-standard library packages to have to enable features in the standard library. But in this case, that seems to be the intention.

wojtek-t · 2016-05-09T18:50:32Z

@krousey - I'm reasigning to you

krousey · 2016-05-10T15:49:15Z

@k8s-bot e2e test this issue: #24620

timothysc · 2016-05-10T17:54:07Z

something is wrong here that has nothing to do with this PR.

I'm seeing this on a number of other PRs as well.

timothysc · 2016-05-10T19:47:50Z

I rebased again just to verify, and check test results.

timothysc · 2016-05-25T17:00:23Z

I would like it in this release if possible, otherwise we will need to carry. Also, many of the hang-ups were around waiting on other tests to merge.

timothysc · 2016-05-25T18:41:59Z

Per discussion on @kubernetes/sig-api-machinery I'll add a disable knob.

@krousey given that is spans tools, I'm inclined to use environment vars. Thoughts?

port-forward which are customized

timothysc · 2016-05-27T20:12:04Z

@krousey I've updated to default off with an option to enable. I think this would make a 0 risk change for 1.3.

krousey · 2016-05-27T22:01:35Z

cc @lavalamp to make the milestone call

lavalamp · 2016-06-02T23:22:59Z

OK, this looks pretty harmless. Sorry for delay.

...tests currently red though.

timothysc · 2016-06-06T15:19:06Z

@k8s-bot unit test this issue #IGNORE

k8s-github-robot · 2016-06-06T16:25:28Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2016-06-06T17:05:47Z

GCE e2e build/test passed for commit 199e15a.

k8s-github-robot · 2016-06-06T17:22:48Z

Automatic merge from submit-queue

timothysc · 2016-06-08T16:55:11Z

pkg/util/net/http.go

+func SetTransportDefaults(t *http.Transport) *http.Transport {
+ t = SetOldTransportDefaults(t)
+ // Allow HTTP2 clients but default off for now
+ if s := os.Getenv("ENABLE_HTTP2"); len(s) > 0 {


/cc @jeremyeder @mffiedler FYI. New option to reduce connection counts, defaulted off.

@krousey

Automatic merge from submit-queue Enable HTTP2 by default Update to enable HTTP2 by default, with the option to disable. This is a continuation of #25280 for the 1.4 release. This should provide ample time for vetting. /cc @krousey

googlebot added the cla: yes label May 6, 2016

timothysc changed the title ~~[Do not merge] Enable Http2 on client connections.~~ [Do not merge] Enable http2 on client connections. May 6, 2016

k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note-label-needed labels May 6, 2016

timothysc added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels May 6, 2016

timothysc assigned wojtek-t May 6, 2016

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 9, 2016

timothysc force-pushed the http2_clients branch from 1ef6f9e to 53ddc93 Compare May 9, 2016 14:54

timothysc changed the title ~~[Do not merge] Enable http2 on client connections.~~ Enable http2 on client connections. May 9, 2016

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 9, 2016

timothysc mentioned this pull request May 9, 2016

Update golang/x/net dep to golang1.6 release, in prep for http2 changes #25076

Closed

krousey reviewed May 9, 2016
View reviewed changes

wojtek-t assigned krousey and unassigned wojtek-t May 9, 2016

timothysc force-pushed the http2_clients branch from 53ddc93 to 8e31cf5 Compare May 9, 2016 20:09

krousey added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 10, 2016

timothysc force-pushed the http2_clients branch from 8e31cf5 to ec3b8d1 Compare May 10, 2016 19:43

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 10, 2016

krousey removed this from the v1.3 milestone May 25, 2016

krousey added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 25, 2016

Timothy St. Clair added 2 commits May 27, 2016 14:25

Update golang.org/x/net for http2 enablement

786b798

Update client connections to try to use http2, except attach, exec, and

199e15a

port-forward which are customized

timothysc force-pushed the http2_clients branch from 61da76d to 199e15a Compare May 27, 2016 20:10

timothysc changed the title ~~Enable http2 on client connections.~~ Option to enable http2 on client connections. May 27, 2016

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 27, 2016

krousey added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 27, 2016

dims mentioned this pull request May 30, 2016

e2e flake: tests are failing with "net/http: TLS handshake timeout" in large clusters #26519

Closed

k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 2, 2016

lavalamp modified the milestones: 1.3, next-candidate Jun 2, 2016

goltermann modified the milestones: v1.3, 1.3 Jun 3, 2016

k8s-github-robot merged commit 2cc0f2f into kubernetes:master Jun 6, 2016

timothysc reviewed Jun 8, 2016
View reviewed changes

timothysc mentioned this pull request Jun 24, 2016

RFE 1.3 : Better client connection management from (X) clients each performing (Y) ops. #21081

Closed

timothysc deleted the http2_clients branch June 24, 2016 14:41

timothysc mentioned this pull request Jun 27, 2016

Enable HTTP2 by default #28114

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Option to enable http2 on client connections. #25280

Option to enable http2 on client connections. #25280

timothysc commented May 6, 2016 •

edited

Loading

timothysc commented May 6, 2016 •

edited

Loading

wojtek-t commented May 7, 2016

wojtek-t commented May 9, 2016

timothysc commented May 9, 2016

timothysc commented May 9, 2016

krousey May 9, 2016

timothysc May 9, 2016 •

edited

Loading

timothysc May 9, 2016

krousey May 9, 2016

krousey May 9, 2016

krousey May 9, 2016

smarterclayton May 9, 2016

krousey May 9, 2016

smarterclayton May 9, 2016

krousey May 9, 2016

wojtek-t commented May 9, 2016

krousey commented May 10, 2016

timothysc commented May 10, 2016 •

edited

Loading

timothysc commented May 10, 2016

timothysc commented May 25, 2016

timothysc commented May 25, 2016 •

edited

Loading

timothysc commented May 27, 2016

krousey commented May 27, 2016

lavalamp commented Jun 2, 2016

timothysc commented Jun 6, 2016

k8s-github-robot commented Jun 6, 2016

k8s-bot commented Jun 6, 2016

k8s-github-robot commented Jun 6, 2016

timothysc Jun 8, 2016

Option to enable http2 on client connections. #25280

Option to enable http2 on client connections. #25280

Conversation

timothysc commented May 6, 2016 • edited Loading

timothysc commented May 6, 2016 • edited Loading

wojtek-t commented May 7, 2016

wojtek-t commented May 9, 2016

timothysc commented May 9, 2016

timothysc commented May 9, 2016

Choose a reason for hiding this comment

timothysc May 9, 2016 • edited Loading

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

Choose a reason for hiding this comment

wojtek-t commented May 9, 2016

krousey commented May 10, 2016

timothysc commented May 10, 2016 • edited Loading

timothysc commented May 10, 2016

timothysc commented May 25, 2016

timothysc commented May 25, 2016 • edited Loading

timothysc commented May 27, 2016

krousey commented May 27, 2016

lavalamp commented Jun 2, 2016

timothysc commented Jun 6, 2016

k8s-github-robot commented Jun 6, 2016

k8s-bot commented Jun 6, 2016

k8s-github-robot commented Jun 6, 2016

Choose a reason for hiding this comment

timothysc commented May 6, 2016 •

edited

Loading

timothysc commented May 6, 2016 •

edited

Loading

timothysc May 9, 2016 •

edited

Loading

timothysc commented May 10, 2016 •

edited

Loading

timothysc commented May 25, 2016 •

edited

Loading