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

Option to enable http2 on client connections. #25280

Merged
merged 2 commits into from
Jun 6, 2016

Conversation

timothysc
Copy link
Member

@timothysc timothysc commented May 6, 2016

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 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 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 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
Copy link
Member Author

timothysc commented May 6, 2016

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

@wojtek-t
Copy link
Member

wojtek-t commented May 7, 2016

@kubernetes/sig-api-machinery

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

wojtek-t commented May 9, 2016

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

@timothysc timothysc changed the title [Do not merge] Enable http2 on client connections. Enable http2 on client connections. May 9, 2016
@k8s-github-robot 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
Copy link
Member Author

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

@timothysc
Copy link
Member Author

@k8s-bot test this please issue: #24211

@@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@timothysc timothysc May 9, 2016

Choose a reason for hiding this comment

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

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

exact reference: golang/go#14950

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Member

wojtek-t commented May 9, 2016

@krousey - I'm reasigning to you

@wojtek-t wojtek-t assigned krousey and unassigned wojtek-t May 9, 2016
@krousey krousey added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 10, 2016
@krousey
Copy link
Contributor

krousey commented May 10, 2016

@k8s-bot e2e test this issue: #24620

@timothysc
Copy link
Member Author

timothysc commented May 10, 2016

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
Copy link
Member Author

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

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 10, 2016
@krousey krousey removed this from the v1.3 milestone May 25, 2016
@krousey krousey added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 25, 2016
@timothysc
Copy link
Member Author

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
Copy link
Member Author

timothysc commented May 25, 2016

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?

@timothysc
Copy link
Member Author

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

@timothysc timothysc changed the title Enable http2 on client connections. Option to enable http2 on client connections. May 27, 2016
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 27, 2016
@krousey krousey added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 27, 2016
@krousey
Copy link
Contributor

krousey commented May 27, 2016

cc @lavalamp to make the milestone call

@k8s-github-robot 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 lavalamp modified the milestones: 1.3, next-candidate Jun 2, 2016
@lavalamp
Copy link
Member

lavalamp commented Jun 2, 2016

OK, this looks pretty harmless. Sorry for delay.

...tests currently red though.

@goltermann goltermann modified the milestones: v1.3, 1.3 Jun 3, 2016
@timothysc
Copy link
Member Author

@k8s-bot unit test this issue #IGNORE

@k8s-github-robot
Copy link

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

@k8s-bot
Copy link

k8s-bot commented Jun 6, 2016

GCE e2e build/test passed for commit 199e15a.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 2cc0f2f into kubernetes:master Jun 6, 2016
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 {
Copy link
Member Author

Choose a reason for hiding this comment

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

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

@timothysc timothysc deleted the http2_clients branch June 24, 2016 14:41
k8s-github-robot pushed a commit that referenced this pull request Jun 29, 2016
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.