docker connection plugin: fix config docs and update to use config system (#297)

* fix config docs and update to use config system

 wean off play_context which did not have the correct data in all cases

* moar fixes

* Update plugins/connection/docker.py

Co-authored-by: Felix Fontein <felix@fontein.de>

* updated for backwards compat

* badmergeresolution

* makeitworks

* attempt to fix unit test

* mocking it# No more than 50 chars. #### 50 chars is here: #

* Update plugins/connection/docker.py

Co-authored-by: Felix Fontein <felix@fontein.de>

* Update plugins/connection/docker.py

Co-authored-by: Felix Fontein <felix@fontein.de>

* Update plugins/connection/docker.py

Co-authored-by: Felix Fontein <felix@fontein.de>

* Update plugins/connection/docker.py

Co-authored-by: Felix Fontein <felix@fontein.de>

* Update plugins/connection/docker.py

Co-authored-by: Felix Fontein <felix@fontein.de>

* Update plugins/connection/docker.py

Co-authored-by: Felix Fontein <felix@fontein.de>

* nomock

* remove bad paste

* properly load connection to initialize config

* initizlie docker args

* Fix bugs.

* Call _set_conn_data() when needed.

* Cache result of _get_docker_remote_user() now that it is called multiple times per task.

* Fix unit tests.

* list.clear() is Python 3...

* Add changelog.

* Call _set_conn_data() also in _connect().

Co-authored-by: Felix Fontein <felix@fontein.de>

changelogs/fragments/297-docker-connection-config.yml

@@ -0,0 +1,4 @@
+bugfixes:
+  - "docker connection plugin - fix option handling to be compatible with ansible-core 2.13 (https://github.com/ansible-collections/community.docker/pull/297, https://github.com/ansible-collections/community.docker/issues/307)."
+minor_changes:
+  - "docker connection plugin - the plugin supports new ways to define the timeout. These are the ``ANSIBLE_DOCKER_TIMEOUT`` environment variable, the ``timeout`` setting in the ``docker_connection`` section of ``ansible.cfg``, and the ``ansible_docker_timeout`` variable (https://github.com/ansible-collections/community.docker/pull/297)."

plugins/connection/docker.py

@@ -22,23 +22,59 @@ DOCUMENTATION = '''
           R(community.docker.docker_api,ansible_collections.community.docker.docker_api_connection)
           connection plugin.
     options:
-      remote_user:
-        description:
-            - The user to execute as inside the container
-        vars:
-            - name: ansible_user
-            - name: ansible_docker_user
-      docker_extra_args:
-        description:
-            - Extra arguments to pass to the docker command line
-        default: ''
       remote_addr:
         description:
             - The name of the container you want to access.
         default: inventory_hostname
         vars:
+            - name: inventory_hostname
             - name: ansible_host
             - name: ansible_docker_host
+      remote_user:
+        description:
+            - The user to execute as inside the container.
+            - If Docker is too old to allow this (< 1.7), the one set by Docker itself will be used.
+        vars:
+            - name: ansible_user
+            - name: ansible_docker_user
+        ini:
+            - section: defaults
+              key: remote_user
+        env:
+            - name: ANSIBLE_REMOTE_USER
+        cli:
+            - name: user
+        keyword:
+            - name: remote_user
+      docker_extra_args:
+        description:
+            - Extra arguments to pass to the docker command line.
+        default: ''
+        vars:
+            - name: ansible_docker_extra_args
+        ini:
+            - section: docker_connection
+              key: extra_cli_args
+      container_timeout:
+        default: 10
+        description:
+            - Controls how long we can wait to access reading output from the container once execution started.
+        env:
+            - name: ANSIBLE_TIMEOUT
+            - name: ANSIBLE_DOCKER_TIMEOUT
+              version_added: 2.2.0
+        ini:
+            - key: timeout
+              section: defaults
+            - key: timeout
+              section: docker_connection
+              version_added: 2.2.0
+        vars:
+          - name: ansible_docker_timeout
+            version_added: 2.2.0
+        cli:
+          - name: timeout
+        type: integer
 '''
 
 import fcntl
@@ -47,7 +83,6 @@ import os.path
 import subprocess
 import re
 
-import ansible.constants as C
 from ansible.compat import selectors
 from ansible.errors import AnsibleError, AnsibleFileNotFound
 from ansible.module_utils.six.moves import shlex_quote
@@ -79,6 +114,9 @@ class Connection(ConnectionBase):
         # configured to be connected to by root and they are not running as
         # root.
 
+        self._docker_args = []
+        self._container_user_cache = {}
+
         # Windows uses Powershell modules
         if getattr(self._shell, "_IS_WINDOWS", False):
             self.module_implementation_preferences = ('.ps1', '.exe', '')
@@ -91,34 +129,12 @@ class Connection(ConnectionBase):
             except ValueError:
                 raise AnsibleError("docker command not found in PATH")
 
-        docker_version = self._get_docker_version()
+        self.docker_version = self._get_docker_version()
-        if docker_version == u'dev':
+        if self.docker_version == u'dev':
             display.warning(u'Docker version number is "dev". Will assume latest version.')
-        if docker_version != u'dev' and LooseVersion(docker_version) < LooseVersion(u'1.3'):
+        if self.docker_version != u'dev' and LooseVersion(self.docker_version) < LooseVersion(u'1.3'):
             raise AnsibleError('docker connection type requires docker 1.3 or higher')
 
-        # The remote user we will request from docker (if supported)
-        self.remote_user = None
-        # The actual user which will execute commands in docker (if known)
-        self.actual_user = None
-
-        if self._play_context.remote_user is not None:
-            if docker_version == u'dev' or LooseVersion(docker_version) >= LooseVersion(u'1.7'):
-                # Support for specifying the exec user was added in docker 1.7
-                self.remote_user = self._play_context.remote_user
-                self.actual_user = self.remote_user
-            else:
-                self.actual_user = self._get_docker_remote_user()
-
-                if self.actual_user != self._play_context.remote_user:
-                    display.warning(u'docker {0} does not support remote_user, using container default: {1}'
-                                    .format(docker_version, self.actual_user or u'?'))
-        elif self._display.verbosity > 2:
-            # Since we're not setting the actual_user, look it up so we have it for logging later
-            # Only do this if display verbosity is high enough that we'll need the value
-            # This saves overhead from calling into docker when we don't need to
-            self.actual_user = self._get_docker_remote_user()
-
     @staticmethod
     def _sanitize_version(version):
         version = re.sub(u'[^0-9a-zA-Z.]', u'', version)
@@ -126,9 +142,7 @@ class Connection(ConnectionBase):
         return version
 
     def _old_docker_version(self):
-        cmd_args = []
+        cmd_args = self._docker_args
-        if self._play_context.docker_extra_args:
-            cmd_args += self._play_context.docker_extra_args.split(' ')
 
         old_version_subcommand = ['version']
 
@@ -140,9 +154,7 @@ class Connection(ConnectionBase):
 
     def _new_docker_version(self):
         # no result yet, must be newer Docker version
-        cmd_args = []
+        cmd_args = self._docker_args
-        if self._play_context.docker_extra_args:
-            cmd_args += self._play_context.docker_extra_args.split(' ')
 
         new_version_subcommand = ['version', '--format', "'{{.Server.Version}}'"]
 
@@ -167,7 +179,10 @@ class Connection(ConnectionBase):
 
     def _get_docker_remote_user(self):
         """ Get the default user configured in the docker container """
-        p = subprocess.Popen([self.docker_cmd, 'inspect', '--format', '{{.Config.User}}', self._play_context.remote_addr],
+        container = self.get_option('remote_addr')
+        if container in self._container_user_cache:
+            return self._container_user_cache[container]
+        p = subprocess.Popen([self.docker_cmd, 'inspect', '--format', '{{.Config.User}}', container],
                              stdout=subprocess.PIPE, stderr=subprocess.PIPE)
 
         out, err = p.communicate()
@@ -175,10 +190,13 @@ class Connection(ConnectionBase):
 
         if p.returncode != 0:
             display.warning(u'unable to retrieve default user from docker container: %s %s' % (out, to_text(err)))
+            self._container_user_cache[container] = None
             return None
 
         # The default exec user is root, unless it was changed in the Dockerfile with USER
-        return out.strip() or u'root'
+        user = out.strip() or u'root'
+        self._container_user_cache[container] = user
+        return user
 
     def _build_exec_cmd(self, cmd):
         """ Build the local docker exec command to run cmd on remote_host
@@ -189,8 +207,8 @@ class Connection(ConnectionBase):
 
         local_cmd = [self.docker_cmd]
 
-        if self._play_context.docker_extra_args:
+        if self._docker_args:
-            local_cmd += self._play_context.docker_extra_args.split(' ')
+            local_cmd += self._docker_args
 
         local_cmd += [b'exec']
 
@@ -198,26 +216,68 @@ class Connection(ConnectionBase):
             local_cmd += [b'-u', self.remote_user]
 
         # -i is needed to keep stdin open which allows pipelining to work
-        local_cmd += [b'-i', self._play_context.remote_addr] + cmd
+        local_cmd += [b'-i', self.get_option('remote_addr')] + cmd
 
         return local_cmd
 
+    def _set_conn_data(self):
+
+        ''' initialize for the connection, cannot do only in init since all data is not ready at that point '''
+
+        # TODO: this is mostly for backwards compatibility, play_context is used as fallback for older versions
+        # docker arguments
+        del self._docker_args[:]
+        extra_args = self.get_option('docker_extra_args') or self._play_context.docker_extra_args
+        if extra_args:
+            self._docker_args += extra_args.split(' ')
+
+        self.remote_user = self.get_option('remote_user')
+        if self.remote_user is None and self._play_context.remote_user is not None:
+            self.remote_user = self._play_context.remote_user
+        # The actual user which will execute commands in docker (if known)
+        self.actual_user = None
+
+        if self.remote_user is not None:
+            if self.docker_version == u'dev' or LooseVersion(self.docker_version) >= LooseVersion(u'1.7'):
+                # Support for specifying the exec user was added in docker 1.7
+                self.actual_user = self.remote_user
+            else:
+                self.remote_user = None
+                self.actual_user = self._get_docker_remote_user()
+                if self.actual_user != self.get_option('remote_user'):
+                    display.warning(u'docker {0} does not support remote_user, using container default: {1}'
+                                    .format(self.docker_version, self.actual_user or u'?'))
+        elif self._display.verbosity > 2:
+            # Since we're not setting the actual_user, look it up so we have it for logging later
+            # Only do this if display verbosity is high enough that we'll need the value
+            # This saves overhead from calling into docker when we don't need to
+            self.actual_user = self._get_docker_remote_user()
+
+        # timeout, use unless default and pc is different, backwards compat
+        self.timeout = self.get_option('container_timeout')
+        if self.timeout == 10 and self.timeout != self._play_context.timeout:
+            self.timeout = self._play_context.timeout
+
     def _connect(self, port=None):
         """ Connect to the container. Nothing to do """
         super(Connection, self)._connect()
         if not self._connected:
+            self._set_conn_data()
             display.vvv(u"ESTABLISH DOCKER CONNECTION FOR USER: {0}".format(
-                self.actual_user or u'?'), host=self._play_context.remote_addr
+                self.actual_user or u'?'), host=self.get_option('remote_addr')
             )
             self._connected = True
 
     def exec_command(self, cmd, in_data=None, sudoable=False):
         """ Run a command on the docker host """
+
+        self._set_conn_data()
+
         super(Connection, self).exec_command(cmd, in_data=in_data, sudoable=sudoable)
 
         local_cmd = self._build_exec_cmd([self._play_context.executable, '-c', cmd])
 
-        display.vvv(u"EXEC {0}".format(to_text(local_cmd)), host=self._play_context.remote_addr)
+        display.vvv(u"EXEC {0}".format(to_text(local_cmd)), host=self.get_option('remote_addr'))
         display.debug("opening command with Popen()")
 
         local_cmd = [to_bytes(i, errors='surrogate_or_strict') for i in local_cmd]
@@ -240,7 +300,7 @@ class Connection(ConnectionBase):
             become_output = b''
             try:
                 while not self.become.check_success(become_output) and not self.become.check_password_prompt(become_output):
-                    events = selector.select(self._play_context.timeout)
+                    events = selector.select(self.timeout)
                     if not events:
                         stdout, stderr = p.communicate()
                         raise AnsibleError('timeout waiting for privilege escalation password prompt:\n' + to_native(become_output))
@@ -291,8 +351,9 @@ class Connection(ConnectionBase):
 
     def put_file(self, in_path, out_path):
         """ Transfer a file from local to docker container """
+        self._set_conn_data()
         super(Connection, self).put_file(in_path, out_path)
-        display.vvv("PUT %s TO %s" % (in_path, out_path), host=self._play_context.remote_addr)
+        display.vvv("PUT %s TO %s" % (in_path, out_path), host=self.get_option('remote_addr'))
 
         out_path = self._prefix_login_path(out_path)
         if not os.path.exists(to_bytes(in_path, errors='surrogate_or_strict')):
@@ -324,15 +385,16 @@ class Connection(ConnectionBase):
 
     def fetch_file(self, in_path, out_path):
         """ Fetch a file from container to local. """
+        self._set_conn_data()
         super(Connection, self).fetch_file(in_path, out_path)
-        display.vvv("FETCH %s TO %s" % (in_path, out_path), host=self._play_context.remote_addr)
+        display.vvv("FETCH %s TO %s" % (in_path, out_path), host=self.get_option('remote_addr'))
 
         in_path = self._prefix_login_path(in_path)
         # out_path is the final file path, but docker takes a directory, not a
         # file path
         out_dir = os.path.dirname(out_path)
 
-        args = [self.docker_cmd, "cp", "%s:%s" % (self._play_context.remote_addr, in_path), out_dir]
+        args = [self.docker_cmd, "cp", "%s:%s" % (self.get_option('remote_addr'), in_path), out_dir]
         args = [to_bytes(i, errors='surrogate_or_strict') for i in args]
 
         p = subprocess.Popen(args, stdin=subprocess.PIPE,

tests/unit/plugins/connection/test_docker.py

@@ -27,6 +27,7 @@ from ansible_collections.community.docker.tests.unit.compat import unittest
 from ansible.errors import AnsibleError
 from ansible.playbook.play_context import PlayContext
 from ansible_collections.community.docker.plugins.connection.docker import Connection as DockerConnection
+from ansible.plugins.loader import connection_loader
 
 
 class TestDockerConnectionClass(unittest.TestCase):
@@ -37,6 +38,16 @@ class TestDockerConnectionClass(unittest.TestCase):
             '[sudo via ansible, key=ouzmdnewuhucvuaabtjmweasarviygqq] password: '
         )
         self.in_stream = StringIO()
+        self.mock_get_bin_path = mock.patch(
+            'ansible_collections.community.docker.plugins.connection.docker.get_bin_path', return_value='docker')
+        self.mock_get_bin_path.start()
+        with mock.patch(
+                'ansible_collections.community.docker.plugins.connection.docker.Connection._old_docker_version',
+                return_value=('false', 'garbage', '', 1)):
+            with mock.patch(
+                    'ansible_collections.community.docker.plugins.connection.docker.Connection._new_docker_version',
+                    return_value=(['docker', 'version'], '20.10.0', '', 0)):
+                dc = connection_loader.get('community.docker.docker', self.play_context, self.in_stream)
 
     def tearDown(self):
         pass