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

Kubelet: Kubelet built with go(<= 1.4) won't work with docker built with go(>=1.6) #23809

Closed
Random-Liu opened this issue Apr 4, 2016 · 14 comments

Comments

Projects
None yet
6 participants
@Random-Liu
Copy link
Member

commented Apr 4, 2016

After #23506, Kubelet built with go(<= 1.4) won't work with docker built with go(>=1.6).

@andyzheng0831 reported that #23506 broke our Jenkins trusty built #23506 (comment). Thank @eparis for pointing me to moby/moby#20865.

After some digging, I found out why this happened:

  1. After golang/go@6e11f45, the http server in Go 1.6 will validate the HTTP Host header conforming to HTTP 1.1 (See golang/go#13624)
  2. On the http client side, Go writes the HTTP Host header with request.Host or request.URL.Host(if the former is empty) (See here)
  3. When using the default docker endpoint (unix:///var/run/docker.sock), engine-api will set request.Host="", request.URL.Host="/var/run/docker.sock" (See set request.URL.Host and set request.Host)
  4. When the engine-api is built with Go >= 1.5, everything goes well, because Go will clean up the Host making both request.Host and request.URL.Host to "", the HTTP Host header will be "". (See golang/go#11206))
  5. But when the engine-api is built with Go<=1.4, Go won't clean up the Host, so the HTTP Host header will be "/var/run/docker.sock". It will break Go 1.6 http server side validation in 1. because '/'is not a valid character, causing 400 Bad Request: malformed Host header.

Go-dockerclient doesn't have this problem, because it is always using a "fake" url for unix endpoint. (See here and here.

I've filed an issue to docker engine-api docker/engine-api#189. But before they fix that, we could not use Kubelet built with go(<=1.4) together with Docker built with go(>=1.6) unless we revert #23506.

Is this acceptable? @kubernetes/sig-node

@Random-Liu

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2016

Also ref #23563.

@errordeveloper

This comment has been minimized.

Copy link
Member

commented Apr 4, 2016

This is not so much of an issue as of 1.11-rc2 (see moby/moby#21489).

@eparis

This comment has been minimized.

Copy link
Member

commented Apr 4, 2016

It doesn't seem like moby/moby#21489 actually 'fixes' anything. This report is explicitly about 1.6 not working and all that does is not us 1.6. At best I would think we would call that a 'work around'.

@dchen1107

This comment has been minimized.

Copy link
Member

commented Apr 4, 2016

Let's fix docker/engine-api#189 or at least workaround it. IMO, before the issue is fixed, we should revert #23506.

@dchen1107

This comment has been minimized.

Copy link
Member

commented Apr 4, 2016

Chatted with @lavalamp offline, and he pointed me that #22149 might be in today. In that case, can we wait for a day or two? @andyzheng0831

@vishh

This comment has been minimized.

Copy link
Member

commented Apr 4, 2016

+1 for a revert!

On Mon, Apr 4, 2016 at 9:32 AM, Dawn Chen notifications@github.com wrote:

Let's fix docker/engine-api#189
docker/engine-api#189 or at least workaround
it. IMO, before the issue is fixed, we should revert #23506
#23506.


You are receiving this because you are on a team that was mentioned.
Reply to this email directly or view it on GitHub
#23809 (comment)

@Random-Liu

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2016

@dchen1107 I'll send a PR to engine-api to fix this. Before that if go1.4+go1.6 is really important, we could do a revert.

@andyzheng0831

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2016

@dchen1107 @Random-Liu and all other guys, I really appreciate that you are working on a solution promptly. For @dchen1107 's question, yes we can wait for a couple days for you guys to fix it. As long as #23506 will not be in next release before fixing it, we are totally fine. The only noise is the Jenkins tests result, but we already narrowed the email notification to limited people in our team to avoid bothering a broad range of people very 0.5 hour. Thanks again!

@Random-Liu

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2016

I've sent a PR docker/engine-api#190 to fix this. Hope it could get in.

@andyzheng0831

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2016

Thanks for making it! When your fix is merged, will it take effect immediately, or we need to wait for some cycles from docker side? If it will take effect immediately in k8s, our jenkins will verify it very quickly

@Random-Liu

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2016

@andyzheng0831 If that one is merged, on our side after bumping up the engine-api version it should work.

@andyzheng0831

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2016

@Random-Liu FYI, PR #22149 was just merged which upgrades k8s to golang 1.6. Accordingly, we are not blocked by the breakage due to incompatible docker daemon-client.

@Random-Liu

This comment has been minimized.

Copy link
Member Author

commented Apr 6, 2016

@andyzheng0831 That's great! :) I'll keep this issue open till docker/engine-api#190 get merged.

@Random-Liu

This comment has been minimized.

Copy link
Member Author

commented Apr 21, 2016

docker/engine-api#190 got merged, will close this issue after bumping up the engine-api version.

@Random-Liu Random-Liu self-assigned this Apr 21, 2016

k8s-github-robot added a commit that referenced this issue May 6, 2016

Merge pull request #24748 from Random-Liu/cleanup-with-new-engine-api
Automatic merge from submit-queue

Kubelet: Cleanup with new engine api

Finish step 2 of #23563

This PR:
1) Cleanup go-dockerclient reference in the code.
2) Bump up the engine-api version.
3) Cleanup the code with new engine-api.

Fixes #24076.
Fixes #23809.

/cc @yujuhong
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.