[JENKINS-38986] remove DOCKER_HOST

[ndeloof]

seems the initial intent was to let container access it's containing host, but actually this prevent a build to rely on /var/run/docker.sock bind mount. Also, there's no guarantee the hostname/ip as declared in jenkins for connectivity from master is relevant from container.

[ndeloof]

@reviewbybees

[ghost]

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

[jswager]

Oh - this would be PERFECT! I was just about to submit a similar PR. In our situation, we want to specify a different DOCKER_HOST via the environment vars of the container, but these lines of code override our setting. I had a clumsy approach to NOT run these lines of code if DOCKER_HOST was detected in the env list. This approach would work much better!

So, when will it get reviewed, merged, released?

[oleg-nenashev]

I had a clumsy approach to NOT run these lines of code if DOCKER_HOST was detected in the env list.

Maybe it's not that clumsy. I miss the context a bit, but one may end up with running tasks without DOCKER_HOST variable defined, right?

[ndeloof]

@oleg-nenashev I can't imagine how DOCKER_HOST could be useful as you won't have TLS keys inside container to authenticate. Also, if access to host docker daemon was the intent, then it's just partially implemented and can work just for simplest usages (unsecured daemon, simplest network topology)

[jswager]

@oleg-nenashev I would prefer that most of our containers not have the DOCKER_HOST variable set up at all. Only select ones would have it defined. And even then, preferably not in an automatic fashion.

We use a multi-host Docker Swarm for our containers. Plug-in works great, except for those containers that are running "docker" commands. The DOCKER_HOST value points back to the Swarm masters, so the command execute against the entire swarm. A Swarm re-acts slightly differently to some commands, so containers expecting a "normal" Docker host are in for a bit of a problem. For admin and isolation purposes, we would like to redirect those "docker" using containers to a specific host suited for their purpose, but can't easily override DOCKER_HOST at the slave level. Have to do it within the job, which is an admin pain.

Security isn't a problem in this case. We have two scenarios that we use: 1) docker hosts secured via IP restrictions, but no TLS and 2) authenticated docker hosts, using either TLS, AWS, Quay, etc. The unsecured ones, just need a proper DOCKER_HOST value. The other methods get their credentials from the job itself, via Credentials bindings.

The problem - for us - is that there is always a DOCKER_HOST variable defined, it's never undefined. And if we do want it defined, it has to be overridden at the job level, rather than the slave level.

[KostyaSha]

Breaks already working instances that using this variables.

[oleg-nenashev]

Also, if access to host docker daemon was the intent, then it's just partially implemented and can work just for simplest usages (unsecured daemon, simplest network topology)

Which is still a case. 🐛 for the current implementation.
The default behavior must be retained. If you want to drop it on your instance, use System Property or whatever to alter the behavior.

[ndeloof]

@oleg-nenashev if I understand your point of view, we need to keep this because it's there, even if ti's wrong ?
I like backward compatibility, but I dislike features that just break when you don't use the restricted expected setup. Shall we make this optional, enabled by default for legacy installation, disabled on new ones ?

[oleg-nenashev]

@ndeloof

Shall we make this optional

Would work for me if POM.xml (compatibleSince in maven HPI) and changelog methion this incompatible change.

Enabled by default for legacy installation, disabled on new ones ?

Works for me, especially if there is a global configuration setting

[KostyaSha]

As i already described multiple times DOCKER_HOST allowed to have easy side containers when there were no any CloudBeees and @ndeloof docker related plugins. This variable was never expected for usage from master.
All you need is simply run docker CLI with volumes-from=$CONTAINER_ID to get working side container.
And i know setups that using it for more than year.

Saying about networks is "wrong" (using termins introduced here) because that feature appeared before volumes and networks API.
Credentials binding is only question to credentials-binding plugin.

I would say vice versa that builds relying on docker.sock that can't survive variable existance are wrong.

So if you want to make that optional, write migration, write test case and that's all.