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

Set DOCKER_HOST in the args to popen3() instead of using --env #2813

Merged
merged 1 commit into from
May 19, 2021

Conversation

mpkut
Copy link
Contributor

@mpkut mpkut commented Apr 30, 2021

We use service-url in inventory.yaml to specify an alternate Docker server address. For example:

   config:
     transport: docker
     docker:
       host: containername
       service-url: unix:///var/run/docker2.sock

This transport configuration has worked in the past with versions of Bolt 2. When we recently upgraded to Bolt 3, we found that the same configuration fails with a connection error. The error incorrectly refers to the default Docker socket, /var/run/docker.sock, instead of the Docker server specified by the inventory.

# bolt command run -t containername whoami
Started on containername...
Failed on containername:
  The command failed with exit code 1
  Cannot connect to the Docker daemon at unix:///var/run/docker.sock. Is the docker daemon running?
Failed on 1 target: containername
Ran on 1 target in 0.3 sec

Expected Behavior

The command run invocation should succeed:

# bolt command run -t containername whoami
Started on containername...
Finished on containername:
  root
Successful on 1 target: containername
Ran on 1 target in 0.62 sec

Steps to Reproduce

This procedure assumes that you have the Docker CLI client installed and are using an alternate socket or remote Docker host to access a container.

  • Confirm CLI access to the alternate Docker server
# DOCKER_HOST=unix:///var/run/docker2.sock docker exec containername whoami
root
  • Create a Bolt project with inventory.yaml as follows:
version: 2
groups:
 - name: example
   targets:
     - containername
   config:
     transport: docker
     docker:
       host: containername
       service-url: unix:///var/run/docker2.sock
  • Attempt bolt command run using the alternate Docker host:
# bolt command run -t containername whoami
  • Success == output is root as expected

  • Failure == Bolt issues the connection error referring to the default Docker socket

Environment

  • Version: 3.7.1
  • Platform: CentOS 7

Additional Context

The execute() method inside lib/bolt/transport/docker/connection.rb uses --env to pass the DOCKER_HOST variable to the docker exec command.

        args += %W[--env DOCKER_HOST=#{@docker_host}] if @docker_host

Although it's not clear in the Docker exec documentation, a direct test shows that --env sets variables inside the container. It does not change the process environment of the docker exec command itself. And that's where DOCKER_HOST is needed in this case.

The suggested patch sets DOCKER_HOST by passing env_hash to the optional env argument of popen3(). This seems consistent with the other methods in connection.rb.

With this change, the bolt command run invocation succeeds as expected with Bolt 3.7.1.

(which sets the variable inside the container).

!bug

* **Docker transport: fix DOCKER_HOST for `docker exec` commands**

Ensure that the `DOCKER_HOST` environment variable is supplied to `docker exec`
commands when using `service-url` in a Docker transport configuration.
@mpkut mpkut requested a review from a team as a code owner April 30, 2021 20:14
@CLAassistant
Copy link

CLAassistant commented Apr 30, 2021

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@lucywyman lucywyman left a comment

Choose a reason for hiding this comment

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

Thanks for catching this! Definitely a bug from when we refactored this transport. I verified the bug, and that the fix correctly connects to the specified Docker host.

Are you ok with signing our CLA before we merge?

@mpkut
Copy link
Contributor Author

mpkut commented May 11, 2021

Thank you for the review! I am currently working through our internal process to clear me to sign the CLA, and will update as soon as that is complete.

@lucywyman
Copy link
Contributor

Great, let me know if there's anything we can do help!

@mpkut
Copy link
Contributor Author

mpkut commented May 19, 2021

I have received approval and signed the CLA as an individual. Thanks for your patience!

@beechtom beechtom merged commit 66d2908 into puppetlabs:main May 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants