Repository: ansible/ansible
Pull request: docker: improve TLS config
##### SUMMARY
This PR does several things:
 - unify TLS setup for inventory plugin and docker modules (not done yet)
 - make `tls` and `tls_verify` options no longer mutually exclusive, and let `tls_verify` (host verification) take precedence over `tls` (no host verification) in case both are set to `yes`.

This also fixes #15614 and makes #51271 no longer necessary.

##### ISSUE TYPE
- Bugfix Pull Request
- Feature Pull Request

##### COMPONENT NAME
lib/ansible/module_utils/docker/common.py
lib/ansible/plugins/doc_fragments/docker.py
lib/ansible/plugins/inventory/docker_swarm.py


Problem statement:
docker_common TLS and TLS_Verify not mutually exclusive.
##### ISSUE TYPE
- Bug Report
##### ANSIBLE VERSION

```
ansible 2.1.0 (docker_common 8b50e93151) last updated 2016/04/25 15:04:12 (GMT -400)
  lib/ansible/modules/core: (devel 804a8e6378) last updated 2016/04/21 18:24:50 (GMT -400)
  lib/ansible/modules/extras: (devel 3031105e78) last updated 2016/04/21 18:24:27 (GMT -400)
  config file =
  configured module search path = Default w/o overrides
```
##### CONFIGURATION
##### OS / ENVIRONMENT
##### SUMMARY

This is actually wrong in docker_common:

```
DOCKER_MUTUALLY_EXCLUSIVE = [
    ['tls', 'tls_verify']
]
```

TLS and TLS_VERIFY are not mutually exclusive. One could set TLS and TLS_VERIFY to True, which would mean connect using TLS and verify the server using the default/public CA pool.

Need to review _get_connect_params() method again and make sure we're covering all the connection use cases.

Hints from issue discussion:
I think that this bug is still valid and there is a very easy way to verify it: testing docker cli behavior.
```bash
# success:
DOCKER_TLS=1 DOCKER_TLS_VERIFY=0 docker ps
# failure with 'parameters are mutually exclusive: tls, tls_verify'
DOCKER_TLS=1 DOCKER_TLS_VERIFY=0 ansible -m docker ....
```

There is no such thing as mutually exclusive on docker and for good reasons: one may have to use TLS on port 2376 but the SSL vertificates may be temporary broken like expired or not matching hostnames. 

This is someting that can happen in real life, even production and thus is why the DOCKER_TLS_VERIFY option was added in the first place, to allow bypassing verification. Making it mutually exclusive makes no sense and breaks it because without telling docker to use TLS via DOCKER_TLS=1, it will fail to run.



This bug is invalid and was closed rightly; I've explained why [here](https://github.com/ansible/ansible/pull/51271#issuecomment-459223312).

Commit message: PEP8.

Patch to review:
diff --git a/lib/ansible/module_utils/docker/common.py b/lib/ansible/module_utils/docker/common.py
index 3c5dacece9dd5d..2af7433b04522d 100644
--- a/lib/ansible/module_utils/docker/common.py
+++ b/lib/ansible/module_utils/docker/common.py
@@ -93,9 +93,7 @@
     debug=dict(type='bool', default=False)
 )
 
-DOCKER_MUTUALLY_EXCLUSIVE = [
-    ['tls', 'tls_verify']
-]
+DOCKER_MUTUALLY_EXCLUSIVE = []
 
 DOCKER_REQUIRED_TOGETHER = [
     ['cert_path', 'key_path']
@@ -163,6 +161,99 @@ def log(self, msg, pretty_print=False):
         #         log_file.write(msg + u'\n')
 
 
+def update_tls_hostname(result):
+    if result['tls_hostname'] is None:
+        # get default machine name from the url
+        parsed_url = urlparse(result['docker_host'])
+        if ':' in parsed_url.netloc:
+            result['tls_hostname'] = parsed_url.netloc[:parsed_url.netloc.rindex(':')]
+        else:
+            result['tls_hostname'] = parsed_url
+
+
+def _get_tls_config(fail_function, **kwargs):
+    try:
+        tls_config = TLSConfig(**kwargs)
+        return tls_config
+    except TLSParameterError as exc:
+        fail_function("TLS config error: %s" % exc)
+
+
+def get_connect_params(auth, fail_function):
+    if auth['tls'] or auth['tls_verify']:
+        auth['docker_host'] = auth['docker_host'].replace('tcp://', 'https://')
+
+    if auth['tls_verify'] and auth['cert_path'] and auth['key_path']:
+        # TLS with certs and host verification
+        if auth['cacert_path']:
+            tls_config = _get_tls_config(client_cert=(auth['cert_path'], auth['key_path']),
+                                         ca_cert=auth['cacert_path'],
+                                         verify=True,
+                                         assert_hostname=auth['tls_hostname'],
+                                         ssl_version=auth['ssl_version'],
+                                         fail_function=fail_function)
+        else:
+            tls_config = _get_tls_config(client_cert=(auth['cert_path'], auth['key_path']),
+                                         verify=True,
+                                         assert_hostname=auth['tls_hostname'],
+                                         ssl_version=auth['ssl_version'],
+                                         fail_function=fail_function)
+
+        return dict(base_url=auth['docker_host'],
+                    tls=tls_config,
+                    version=auth['api_version'],
+                    timeout=auth['timeout'])
+
+    if auth['tls_verify'] and auth['cacert_path']:
+        # TLS with cacert only
+        tls_config = _get_tls_config(ca_cert=auth['cacert_path'],
+                                     assert_hostname=auth['tls_hostname'],
+                                     verify=True,
+                                     ssl_version=auth['ssl_version'],
+                                     fail_function=fail_function)
+        return dict(base_url=auth['docker_host'],
+                    tls=tls_config,
+                    version=auth['api_version'],
+                    timeout=auth['timeout'])
+
+    if auth['tls_verify']:
+        # TLS with verify and no certs
+        tls_config = _get_tls_config(verify=True,
+                                     assert_hostname=auth['tls_hostname'],
+                                     ssl_version=auth['ssl_version'],
+                                     fail_function=fail_function)
+        return dict(base_url=auth['docker_host'],
+                    tls=tls_config,
+                    version=auth['api_version'],
+                    timeout=auth['timeout'])
+
+    if auth['tls'] and auth['cert_path'] and auth['key_path']:
+        # TLS with certs and no host verification
+        tls_config = _get_tls_config(client_cert=(auth['cert_path'], auth['key_path']),
+                                     verify=False,
+                                     ssl_version=auth['ssl_version'],
+                                     fail_function=fail_function)
+        return dict(base_url=auth['docker_host'],
+                    tls=tls_config,
+                    version=auth['api_version'],
+                    timeout=auth['timeout'])
+
+    if auth['tls']:
+        # TLS with no certs and not host verification
+        tls_config = _get_tls_config(verify=False,
+                                     ssl_version=auth['ssl_version'],
+                                     fail_function=fail_function)
+        return dict(base_url=auth['docker_host'],
+                    tls=tls_config,
+                    version=auth['api_version'],
+                    timeout=auth['timeout'])
+
+    # No TLS
+    return dict(base_url=auth['docker_host'],
+                version=auth['api_version'],
+                timeout=auth['timeout'])
+
+
 class AnsibleDockerClient(Client):
 
     def __init__(self, argument_spec=None, supports_check_mode=False, mutually_exclusive=None,
@@ -229,7 +320,7 @@ def __init__(self, argument_spec=None, supports_check_mode=False, mutually_exclu
 
         self.debug = self.module.params.get('debug')
         self.check_mode = self.module.check_mode
-        self._connect_params = self._get_connect_params()
+        self._connect_params = get_connect_params(self.auth_params, fail_function=self.fail)
 
         try:
             super(AnsibleDockerClient, self).__init__(**self._connect_params)
@@ -327,99 +418,10 @@ def auth_params(self):
                                     DEFAULT_TIMEOUT_SECONDS),
         )
 
-        if result['tls_hostname'] is None:
-            # get default machine name from the url
-            parsed_url = urlparse(result['docker_host'])
-            if ':' in parsed_url.netloc:
-                result['tls_hostname'] = parsed_url.netloc[:parsed_url.netloc.rindex(':')]
-            else:
-                result['tls_hostname'] = parsed_url
+        update_tls_hostname(result)
 
         return result
 
-    def _get_tls_config(self, **kwargs):
-        self.log("get_tls_config:")
-        for key in kwargs:
-            self.log("  %s: %s" % (key, kwargs[key]))
-        try:
-            tls_config = TLSConfig(**kwargs)
-            return tls_config
-        except TLSParameterError as exc:
-            self.fail("TLS config error: %s" % exc)
-
-    def _get_connect_params(self):
-        auth = self.auth_params
-
-        self.log("connection params:")
-        for key in auth:
-            self.log("  %s: %s" % (key, auth[key]))
-
-        if auth['tls'] or auth['tls_verify']:
-            auth['docker_host'] = auth['docker_host'].replace('tcp://', 'https://')
-
-        if auth['tls'] and auth['cert_path'] and auth['key_path']:
-            # TLS with certs and no host verification
-            tls_config = self._get_tls_config(client_cert=(auth['cert_path'], auth['key_path']),
-                                              verify=False,
-                                              ssl_version=auth['ssl_version'])
-            return dict(base_url=auth['docker_host'],
-                        tls=tls_config,
-                        version=auth['api_version'],
-                        timeout=auth['timeout'])
-
-        if auth['tls']:
-            # TLS with no certs and not host verification
-            tls_config = self._get_tls_config(verify=False,
-                                              ssl_version=auth['ssl_version'])
-            return dict(base_url=auth['docker_host'],
-                        tls=tls_config,
-                        version=auth['api_version'],
-                        timeout=auth['timeout'])
-
-        if auth['tls_verify'] and auth['cert_path'] and auth['key_path']:
-            # TLS with certs and host verification
-            if auth['cacert_path']:
-                tls_config = self._get_tls_config(client_cert=(auth['cert_path'], auth['key_path']),
-                                                  ca_cert=auth['cacert_path'],
-                                                  verify=True,
-                                                  assert_hostname=auth['tls_hostname'],
-                                                  ssl_version=auth['ssl_version'])
-            else:
-                tls_config = self._get_tls_config(client_cert=(auth['cert_path'], auth['key_path']),
-                                                  verify=True,
-                                                  assert_hostname=auth['tls_hostname'],
-                                                  ssl_version=auth['ssl_version'])
-
-            return dict(base_url=auth['docker_host'],
-                        tls=tls_config,
-                        version=auth['api_version'],
-                        timeout=auth['timeout'])
-
-        if auth['tls_verify'] and auth['cacert_path']:
-            # TLS with cacert only
-            tls_config = self._get_tls_config(ca_cert=auth['cacert_path'],
-                                              assert_hostname=auth['tls_hostname'],
-                                              verify=True,
-                                              ssl_version=auth['ssl_version'])
-            return dict(base_url=auth['docker_host'],
-                        tls=tls_config,
-                        version=auth['api_version'],
-                        timeout=auth['timeout'])
-
-        if auth['tls_verify']:
-            # TLS with verify and no certs
-            tls_config = self._get_tls_config(verify=True,
-                                              assert_hostname=auth['tls_hostname'],
-                                              ssl_version=auth['ssl_version'])
-            return dict(base_url=auth['docker_host'],
-                        tls=tls_config,
-                        version=auth['api_version'],
-                        timeout=auth['timeout'])
-        # No TLS
-        return dict(base_url=auth['docker_host'],
-                    version=auth['api_version'],
-                    timeout=auth['timeout'])
-
     def _handle_ssl_error(self, error):
         match = re.match(r"hostname.*doesn\'t match (\'.*\')", str(error))
         if match:
diff --git a/lib/ansible/modules/cloud/docker/docker_container.py b/lib/ansible/modules/cloud/docker/docker_container.py
index ffd3fc0e94ee05..9801fff3a9d31b 100644
--- a/lib/ansible/modules/cloud/docker/docker_container.py
+++ b/lib/ansible/modules/cloud/docker/docker_container.py
@@ -931,7 +931,8 @@
     compare_generic,
     is_image_name_id,
     sanitize_result,
-    parse_healthcheck
+    parse_healthcheck,
+    DOCKER_COMMON_ARGS,
 )
 from ansible.module_utils.six import string_types
 
@@ -2642,13 +2643,11 @@ def detect_ipvX_address_usage(client):
 
 class AnsibleDockerClientContainer(AnsibleDockerClient):
     # A list of module options which are not docker container properties
-    __NON_CONTAINER_PROPERTY_OPTIONS = (
-        'docker_host', 'tls_hostname', 'api_version', 'timeout', 'cacert_path', 'cert_path',
-        'key_path', 'ssl_version', 'tls', 'tls_verify', 'debug', 'env_file', 'force_kill',
-        'keep_volumes', 'ignore_image', 'name', 'pull', 'purge_networks', 'recreate',
-        'restart', 'state', 'trust_image_content', 'networks', 'cleanup', 'kill_signal',
+    __NON_CONTAINER_PROPERTY_OPTIONS = tuple([
+        'env_file', 'force_kill', 'keep_volumes', 'ignore_image', 'name', 'pull', 'purge_networks',
+        'recreate', 'restart', 'state', 'trust_image_content', 'networks', 'cleanup', 'kill_signal',
         'output_logs', 'paused'
-    )
+    ] + list(DOCKER_COMMON_ARGS.keys()))
 
     def _parse_comparisons(self):
         comparisons = {}
diff --git a/lib/ansible/plugins/doc_fragments/docker.py b/lib/ansible/plugins/doc_fragments/docker.py
index 4eca66a016ee07..4e199145d301f6 100644
--- a/lib/ansible/plugins/doc_fragments/docker.py
+++ b/lib/ansible/plugins/doc_fragments/docker.py
@@ -71,8 +71,8 @@ class ModuleDocFragment(object):
         type: str
     tls:
         description:
-            -  Secure the connection to the API by using TLS without verifying the authenticity of the Docker host
-               server.
+            - Secure the connection to the API by using TLS without verifying the authenticity of the Docker host
+              server. Note that if C(tls_verify) is set to C(yes) as well, it will take precedence.
             - If the value is not specified in the task, the value of environment variable C(DOCKER_TLS) will be used
               instead. If the environment variable is not set, the default value will be used.
         type: bool
diff --git a/lib/ansible/plugins/inventory/docker_swarm.py b/lib/ansible/plugins/inventory/docker_swarm.py
index f078ed080c3688..8c2e5877ad0695 100644
--- a/lib/ansible/plugins/inventory/docker_swarm.py
+++ b/lib/ansible/plugins/inventory/docker_swarm.py
@@ -29,7 +29,9 @@
             required: true
             choices: docker_swarm
         host:
-            description: Socket of a Docker swarm manager node (tcp,unix).
+            description:
+                - Socket of a Docker swarm manager node (tcp,unix).
+                - "Use C(unix://var/run/docker.sock) to connect via local socket."
             type: str
             required: true
         verbose_output:
@@ -56,6 +58,21 @@
         tls_hostname:
             description: When verifying the authenticity of the Docker Host server, provide the expected name of the server.
             type: str
+        ssl_version:
+            description: Provide a valid SSL version number. Default value determined by ssl.py module.
+            type: str
+        api_version:
+            description:
+                - The version of the Docker API running on the Docker Host.
+                - Defaults to the latest version of the API supported by docker-py.
+            type: str
+        timeout:
+            description:
+                - The maximum amount of time in seconds to wait on a response from the API.
+                - If the value is not specified in the task, the value of environment variable C(DOCKER_TIMEOUT) will be used
+                  instead. If the environment variable is not set, the default value will be used.
+            type: int
+            default: 60
         include_host_uri:
             description: Toggle to return the additional attribute I(ansible_host_uri) which contains the URI of the
                          swarm leader in format of M(tcp://172.16.0.1:2376). This value may be used without additional
@@ -110,6 +127,7 @@
 
 from ansible.errors import AnsibleError
 from ansible.module_utils._text import to_native
+from ansible.module_utils.docker.common import update_tls_hostname, get_connect_params
 from ansible.plugins.inventory import BaseInventoryPlugin, Constructable
 from ansible.parsing.utils.addresses import parse_address
 
@@ -125,61 +143,31 @@ class InventoryModule(BaseInventoryPlugin, Constructable):
 
     NAME = 'docker_swarm'
 
-    def _get_tls_config(self, **kwargs):
-        try:
-            tls_config = docker.tls.TLSConfig(**kwargs)
-            return tls_config
-        except Exception as e:
-            raise AnsibleError('Unable to setup TLS, this was the original exception: %s' % to_native(e))
-
-    def _get_tls_connect_params(self):
-        if self.get_option('tls') and self.get_option('cert_path') and self.get_option('key_path'):
-            # TLS with certs and no host verification
-            tls_config = self._get_tls_config(client_cert=(self.get_option('cert_path'),
-                                                           self.get_option('key_path')),
-                                              verify=False)
-            return tls_config
-
-        if self.get_option('tls'):
-            # TLS with no certs and not host verification
-            tls_config = self._get_tls_config(verify=False)
-            return tls_config
-
-        if self.get_option('tls_verify') and self.get_option('cert_path') and self.get_option('key_path'):
-            # TLS with certs and host verification
-            if self.get_option('cacert_path'):
-                tls_config = self._get_tls_config(client_cert=(self.get_option('cert_path'),
-                                                               self.get_option('key_path')),
-                                                  ca_cert=self.get_option('cacert_path'),
-                                                  verify=True,
-                                                  assert_hostname=self.get_option('tls_hostname'))
-            else:
-                tls_config = self._get_tls_config(client_cert=(self.get_option('cert_path'),
-                                                               self.get_option('key_path')),
-                                                  verify=True,
-                                                  assert_hostname=self.get_option('tls_hostname'))
-
-            return tls_config
-
-        if self.get_option('tls_verify') and self.get_option('cacert_path'):
-            # TLS with cacert only
-            tls_config = self._get_tls_config(ca_cert=self.get_option('cacert_path'),
-                                              assert_hostname=self.get_option('tls_hostname'),
-                                              verify=True)
-            return tls_config
-
-        if self.get_option('tls_verify'):
-            # TLS with verify and no certs
-            tls_config = self._get_tls_config(verify=True,
-                                              assert_hostname=self.get_option('tls_hostname'))
-            return tls_config
-
-        # No TLS
-        return None
+    def _fail(self, msg):
+        raise AnsibleError(msg)
 
     def _populate(self):
-        self.client = docker.DockerClient(base_url=self.get_option('host'),
-                                          tls=self._get_tls_connect_params())
+        raw_params = dict(
+            docker_host=self.get_option('docker_host'),
+            tls=self.get_option('tls'),
+            tls_verify=self.get_option('tls_verify'),
+            key_path=self.get_option('key_path'),
+            cacert_path=self.get_option('cacert_path'),
+            cert_path=self.get_option('cert_path'),
+            tls_hostname=self.get_option('tls_hostname'),
+            api_version=self.get_option('api_version'),
+            timeout=self.get_option('timeout') or 60,
+            ssl_version=self.get_option('ssl_version'),
+            debug=None,
+        )
+        if raw_params['timeout'] is not None:
+            try:
+                raw_params['timeout'] = int(raw_params['timeout'])
+            except Exception as dummy:
+                raise AnsibleError('Argument to timeout function must be an integer')
+        update_tls_hostname(raw_params)
+        connect_params = get_connect_params(raw_params, fail_function=self._fail)
+        self.client = docker.DockerClient(**get_connect_params)
         self.inventory.add_group('all')
         self.inventory.add_group('manager')
         self.inventory.add_group('worker')


Code review comments:
diff_hunk: @@ -29,7 +29,9 @@
             required: true
             choices: docker_swarm
         host:
path: lib/ansible/plugins/inventory/docker_swarm.py
text: @user1:
needs to be renamed to `docker_host` or we'll get this warning:

```
 [WARNING]:  * Failed to parse /home/morph/ansible/docker_swarm.yml with auto plugin: 'Requested entry (plugin_type:
inventory plugin: docker_swarm setting: docker_host ) was not defined in configuration.'
```

Applies to examples too

@author:
Done. It would also be enough to fix line 151, but I think `docker_host` is somewhat better than `host` since it's the same name for the `docker_*` modules.

diff_hunk: @@ -125,61 +143,31 @@ class InventoryModule(BaseInventoryPlugin, Constructable):
 
     NAME = 'docker_swarm'
 
-    def _get_tls_config(self, **kwargs):
-        try:
-            tls_config = docker.tls.TLSConfig(**kwargs)
-            return tls_config
-        except Exception as e:
-            raise AnsibleError('Unable to setup TLS, this was the original exception: %s' % to_native(e))
-
-    def _get_tls_connect_params(self):
-        if self.get_option('tls') and self.get_option('cert_path') and self.get_option('key_path'):
-            # TLS with certs and no host verification
-            tls_config = self._get_tls_config(client_cert=(self.get_option('cert_path'),
-                                                           self.get_option('key_path')),
-                                              verify=False)
-            return tls_config
-
-        if self.get_option('tls'):
-            # TLS with no certs and not host verification
-            tls_config = self._get_tls_config(verify=False)
-            return tls_config
-
-        if self.get_option('tls_verify') and self.get_option('cert_path') and self.get_option('key_path'):
-            # TLS with certs and host verification
-            if self.get_option('cacert_path'):
-                tls_config = self._get_tls_config(client_cert=(self.get_option('cert_path'),
-                                                               self.get_option('key_path')),
-                                                  ca_cert=self.get_option('cacert_path'),
-                                                  verify=True,
-                                                  assert_hostname=self.get_option('tls_hostname'))
-            else:
-                tls_config = self._get_tls_config(client_cert=(self.get_option('cert_path'),
-                                                               self.get_option('key_path')),
-                                                  verify=True,
-                                                  assert_hostname=self.get_option('tls_hostname'))
-
-            return tls_config
-
-        if self.get_option('tls_verify') and self.get_option('cacert_path'):
-            # TLS with cacert only
-            tls_config = self._get_tls_config(ca_cert=self.get_option('cacert_path'),
-                                              assert_hostname=self.get_option('tls_hostname'),
-                                              verify=True)
-            return tls_config
-
-        if self.get_option('tls_verify'):
-            # TLS with verify and no certs
-            tls_config = self._get_tls_config(verify=True,
-                                              assert_hostname=self.get_option('tls_hostname'))
-            return tls_config
-
-        # No TLS
-        return None
+    def _fail(self, msg):
+        raise AnsibleError(msg)
 
     def _populate(self):
-        self.client = docker.DockerClient(base_url=self.get_option('host'),
-                                          tls=self._get_tls_connect_params())
+        raw_params = dict(
+            docker_host=self.get_option('docker_host'),
+            tls=self.get_option('tls'),
+            tls_verify=self.get_option('tls_verify'),
+            key_path=self.get_option('key_path'),
+            cacert_path=self.get_option('cacert_path'),
+            cert_path=self.get_option('cert_path'),
+            tls_hostname=self.get_option('tls_hostname'),
+            api_version=self.get_option('api_version'),
+            timeout=self.get_option('timeout') or 60,
+            ssl_version=self.get_option('ssl_version'),
+            debug=None,
+        )
+        if raw_params['timeout'] is not None:
+            try:
+                raw_params['timeout'] = int(raw_params['timeout'])
+            except Exception as dummy:
+                raise AnsibleError('Argument to timeout function must be an integer')
+        update_tls_hostname(raw_params)
+        connect_params = get_connect_params(raw_params, fail_function=self._fail)
+        self.client = docker.DockerClient(**get_connect_params)
path: lib/ansible/plugins/inventory/docker_swarm.py
text: @user1:
Not `self.client = docker.DockerClient(**connect_params)` ?

@author:
Yeah, good spot! Fixed.