docker connection: handle version and docker_args (ansible-collection…

…s#327)

* handle version and docker_args

* Remove breaking change.

* Add changelog fragment.

* Fix unit tests.

Co-authored-by: Felix Fontein <felix@fontein.de>
(cherry picked from commit 6679eee)

changelogs/fragments/327-connection-fix.yml

@@ -0,0 +1,4 @@
+bugfixes:
+  - "docker connection plugin - make sure that ``docker_extra_args`` is used for querying the Docker version.
+     Also ensures that the Docker version is only queried when needed. This is currently the case if a remote user is specified
+     (https://github.com/ansible-collections/community.docker/issues/325, https://github.com/ansible-collections/community.docker/pull/327)."

plugins/connection/docker.py

@@ -108,6 +108,7 @@ def __init__(self, play_context, new_stdin, *args, **kwargs):
 
         self._docker_args = []
         self._container_user_cache = {}
+        self._version = None
 
         # Windows uses Powershell modules
         if getattr(self._shell, "_IS_WINDOWS", False):
@@ -121,12 +122,6 @@ def __init__(self, play_context, new_stdin, *args, **kwargs):
             except ValueError:
                 raise AnsibleError("docker command not found in PATH")
 
-        self.docker_version = self._get_docker_version()
-        if self.docker_version == u'dev':
-            display.warning(u'Docker version number is "dev". Will assume latest version.')
-        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')
-
     @staticmethod
     def _sanitize_version(version):
         version = re.sub(u'[^0-9a-zA-Z.]', u'', version)
@@ -212,17 +207,20 @@ def _build_exec_cmd(self, 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 '''
-
+    def _set_docker_args(self):
         # 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
+        extra_args = self.get_option('docker_extra_args') or getattr(self._play_context, 'docker_extra_args', '')
         if extra_args:
             self._docker_args += extra_args.split(' ')
 
+    def _set_conn_data(self):
+
+        ''' initialize for the connection, cannot do only in init since all data is not ready at that point '''
+
+        self._set_docker_args()
+
         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
@@ -250,6 +248,19 @@ def _set_conn_data(self):
         if self.timeout == 10 and self.timeout != self._play_context.timeout:
             self.timeout = self._play_context.timeout
 
+    @property
+    def docker_version(self):
+
+        if not self._version:
+            self._set_docker_args()
+
+            self._version = self._get_docker_version()
+            if self._version == u'dev':
+                display.warning(u'Docker version number is "dev". Will assume latest version.')
+            if self._version != u'dev' and LooseVersion(self._version) < LooseVersion(u'1.3'):
+                raise AnsibleError('docker connection type requires docker 1.3 or higher')
+        return self._version
+
     def _connect(self, port=None):
         """ Connect to the container. Nothing to do """
         super(Connection, self)._connect()
@@ -365,8 +376,7 @@ def put_file(self, in_path, out_path):
             args = self._build_exec_cmd([self._play_context.executable, "-c", "dd of=%s bs=%s%s" % (out_path, BUFSIZE, count)])
             args = [to_bytes(i, errors='surrogate_or_strict') for i in args]
             try:
-                p = subprocess.Popen(args, stdin=in_file,
-                                     stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+                p = subprocess.Popen(args, stdin=in_file, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
             except OSError:
                 raise AnsibleError("docker connection requires dd command in the container to put files")
             stdout, stderr = p.communicate()

tests/unit/plugins/connection/test_docker.py

@@ -38,16 +38,8 @@ def setUp(self):
             '[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)
+        with mock.patch('ansible_collections.community.docker.plugins.connection.docker.get_bin_path', return_value='docker'):
+            self.dc = connection_loader.get('community.docker.docker', self.play_context, self.in_stream)
 
     def tearDown(self):
         pass
@@ -57,22 +49,24 @@ def tearDown(self):
     @mock.patch('ansible_collections.community.docker.plugins.connection.docker.Connection._new_docker_version',
                 return_value=('docker version', '1.2.3', '', 0))
     def test_docker_connection_module_too_old(self, mock_new_docker_verison, mock_old_docker_version):
-        self.assertRaisesRegexp(AnsibleError, '^docker connection type requires docker 1.3 or higher$',
-                                DockerConnection, self.play_context, self.in_stream, docker_command='/fake/docker')
+        self.dc._version = None
+        self.dc.remote_user = 'foo'
+        self.assertRaisesRegexp(AnsibleError, '^docker connection type requires docker 1.3 or higher$', self.dc._get_actual_user)
 
     @mock.patch('ansible_collections.community.docker.plugins.connection.docker.Connection._old_docker_version',
                 return_value=('false', 'garbage', '', 1))
     @mock.patch('ansible_collections.community.docker.plugins.connection.docker.Connection._new_docker_version',
-                return_value=('docker version', '1.3.4', '', 0))
+                return_value=('docker version', '1.7.0', '', 0))
     def test_docker_connection_module(self, mock_new_docker_verison, mock_old_docker_version):
-        self.assertIsInstance(DockerConnection(self.play_context, self.in_stream, docker_command='/fake/docker'),
-                              DockerConnection)
+        self.dc._version = None
+        version = self.dc.docker_version
 
     # old version and new version fail
     @mock.patch('ansible_collections.community.docker.plugins.connection.docker.Connection._old_docker_version',
                 return_value=('false', 'garbage', '', 1))
     @mock.patch('ansible_collections.community.docker.plugins.connection.docker.Connection._new_docker_version',
                 return_value=('false', 'garbage', '', 1))
     def test_docker_connection_module_wrong_cmd(self, mock_new_docker_version, mock_old_docker_version):
-        self.assertRaisesRegexp(AnsibleError, '^Docker version check (.*?) failed: ',
-                                DockerConnection, self.play_context, self.in_stream, docker_command='/fake/docker')
+        self.dc._version = None
+        self.dc.remote_user = 'foo'
+        self.assertRaisesRegexp(AnsibleError, '^Docker version check (.*?) failed: ', self.dc._get_actual_user)