Remove DOCKER_HTTP_HOST_COMPAT env var

[ibuildthecloud]

The whole go 1.6 breaking clients issue is a bit near and dear to my heart. I've been playing whack-a-mole over the past month or so dealing with distros breaking old clients. I'm glad to see the hack got merged, but disabling it by default really doesn't help.

First, before if you built docker wrong it would not work with old clients. Now by default docker doesn't work with old clients unless you know the magic env var. This is a terrible user experience but the key thing is that docker is by default incompatible with old clients.

Second, for those crying "technical debt! technical debt!" Introducing a env var is far worse debt because this is a public interface. Distros will have to set this env var. I know Ubuntu already backported the hack, and other distros are doing the same. So supporting old clients is important enough to them so they will set the env var. This is the type of thing 5 years later when you don't need the env var anymore it will still be in scripts and documentation because nobody really knows what it does and is afraid to remove it.

Third, one code path is better than two. If there is ever a bug caused by the hack it is not an obvious thing to consider if this env var is set or not. This is just yet another variable to consider, another thing to ask a user if X is set or not, etc.

Finally, once 1.12 ships my whack-a-mole will change from "don't compile with 1.6" to "set DOCKER_HTTP_HOST_COMPAT=1." This does not make me happy.

I really think the key point is that we are consciously choosing the purity of our code over user experience. And that is bad.

[thaJeztah]

ping @docker/core-engine-maintainers

[cpuguy83]

In this case user experience is limited to a very specific case, and introducing this hack to the general public is far more likely to cause issues.

[ibuildthecloud]

@cpuguy83 you will support and fix the hack regardless. You're just making support harder for yourself. Make it the only option until older client are officially not supported.

[cpuguy83]

@ibuildthecloud Again, the number of users who need this hack is so small, it makes no sense to turn this on by default, especially with how much this potentially impacts performance (no connection re-use, extra allocation per request).

[mlaventure]

I don't think we should make it the default either, especially since as time goes by this issue will be less and less likely to present itself as people update docker.

[ibuildthecloud]

@cpuguy83 @mlaventure You guys are really missing the point. You have made the situation worse. Before you had to go out of your way to break old clients. Now with 1.12 you will break old clients by default. You're making your life more difficult, and mine. Until you declare old Dockers EOL, you should properly support them.

@cpuguy83 The extra allocation should make zero difference in the real world and no connection re-use only applies to old clients.

[crosbymichael]

I'm with @ibuildthecloud on this one. an env var is pretty bad. As long as we can verify that this is not giving us overhead and everything works we might as well just have some bad code but happy users.

Also, not bad code as in the author wrote bad code, bad code as in its a hack that we wish we didn't have.

[rhatdan]

I am with @ibuildthecloud on this also. I think the Variable is bad.

[cpuguy83]

@ibuildthecloud Ok, was thinking about this. I see your point.
The overhead is not so bad, and even people that don't need it won't be affected by it anyway.
It'll be good to have this tested before release anyway.

So, I'll go ahead and +1 this.

[runcom]

I'd love to remove that variable as well +1 hopefully we could battle test that code during RC

[ibuildthecloud]

@runcom Are there any tests that need to be reverted if the env var is removed? Or other code?

[runcom]

@ibuildthecloud based on https://github.com/docker/docker/pull/22000/files I don't think so

[LK4D4]

Ok, let's move it to code-review.
Also, it needs change in docs/breaking_changes.md

[LK4D4]

LGTM

[cpuguy83]

LGTM

[cpuguy83]

Hmm, actually do you think the CI failures could be from this?

[SvenDowideit]

docs LGTM

[LK4D4]

@cpuguy83 it might be indeed, but I wonder why only gccgo and userns.

[LK4D4]

It passed second time :)

[thaJeztah]

gccgo failed, related? https://jenkins.dockerproject.org/job/Docker-PRs-gccgo/5606/console

05:39:40 ----------------------------------------------------------------------
05:39:40 FAIL: docker_cli_import_test.go:88: TestImportFileWithMessage.pN52_github_com_docker_docker_integration_cli.DockerSuite
05:39:40 
05:39:40 /go/src/github.com/docker/docker/pkg/integration/dockerCmd_utils.go:42:
05:39:40     c.Assert(err, check.IsNil, check.Commentf(quote+"%v"+quote+" failed with errors: %s, %v", strings.Join(args, " "), out, err))
05:39:40 ... value *exec.ExitError = &exec.ExitError{ProcessState:(*os.ProcessState)(0xc2080aefa0)} ("exit status 1")
05:39:40 ... "import -m Testing commit message /tmp/exportImportTest948390921" failed with errors: An error occurred trying to connect: Post http:///go/src/github.com/docker/docker/bundles/1.12.0-dev/test-integration-cli/docker.sock/v1.24/images/create?fromSrc=-&message=Testing+commit+message&repo=&tag=: write unix /go/src/github.com/docker/docker/bundles/1.12.0-dev/test-integration-cli/docker.sock: broken pipe
05:39:40 , exit status 1
05:39:40

restarting

[thaJeztah]

LGTM (if green)

[LK4D4]

Merging

[runcom]

🎉 :)