From 6679eee41e7283b4223993666c6a6ec31643b5dc Mon Sep 17 00:00:00 2001 From: Brian Coca Date: Mon, 25 Apr 2022 14:35:33 -0400 Subject: [PATCH] docker connection: handle version and docker_args (#327) * handle version and docker_args * Remove breaking change. * Add changelog fragment. * Fix unit tests. Co-authored-by: Felix Fontein --- changelogs/fragments/327-connection-fix.yml | 4 +++ plugins/connection/docker.py | 36 +++++++++++++------- tests/unit/plugins/connection/test_docker.py | 28 ++++++--------- 3 files changed, 38 insertions(+), 30 deletions(-) create mode 100644 changelogs/fragments/327-connection-fix.yml diff --git a/changelogs/fragments/327-connection-fix.yml b/changelogs/fragments/327-connection-fix.yml new file mode 100644 index 000000000..1611875ec --- /dev/null +++ b/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)." diff --git a/plugins/connection/docker.py b/plugins/connection/docker.py index f039b994b..6c8b84aff 100644 --- a/plugins/connection/docker.py +++ b/plugins/connection/docker.py @@ -116,6 +116,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): @@ -129,12 +130,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) @@ -220,17 +215,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 @@ -240,6 +238,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 _get_actual_user(self): if self.remote_user is not None: # An explicit user is provided @@ -377,8 +388,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() diff --git a/tests/unit/plugins/connection/test_docker.py b/tests/unit/plugins/connection/test_docker.py index e21872e53..e811c8243 100644 --- a/tests/unit/plugins/connection/test_docker.py +++ b/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,16 +49,17 @@ 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', @@ -74,5 +67,6 @@ def test_docker_connection_module(self, mock_new_docker_verison, mock_old_docker @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)