docker_container: refactoring preparing better comparisons (#713)

* Always get the container's image as well to allow get_value() to use that one too.

* Allow options and engines to overwrite comparison functions.

* Do not fail if image (by ID) cannot be found.

* Allow to control when container image is needed.

* Pass option to compare function.

* Allow to pass the host info for retrieving a value.

* Add changelog fragment.

changelogs/fragments/713-docker_container-refactoring.yml

@@ -0,0 +1,2 @@
+minor_changes:
+  - "docker_container - internal refactorings which allow comparisons to use more information like details of the current image or the Docker host config (https://github.com/ansible-collections/community.docker/pull/713)."

plugins/module_utils/module_container/base.py

@@ -19,6 +19,7 @@ from ansible.module_utils.six import string_types
 
 from ansible_collections.community.docker.plugins.module_utils.util import (
     clean_dict_booleans_for_docker_api,
+    compare_generic,
     normalize_healthcheck,
     omit_none_from_dict,
 )
@@ -67,6 +68,7 @@ class Option(object):
         not_a_container_option=False,
         not_an_ansible_option=False,
         copy_comparison_from=None,
+        compare=None,
     ):
         self.name = name
         self.type = type
@@ -106,6 +108,11 @@ class Option(object):
         self.not_a_container_option = not_a_container_option
         self.not_an_ansible_option = not_an_ansible_option
         self.copy_comparison_from = copy_comparison_from
+        self.compare = (
+            lambda param_value, container_value: compare(self, param_value, container_value)
+        ) if compare else (
+            lambda param_value, container_value: compare_generic(param_value, container_value, self.comparison, self.comparison_type)
+        )
 
 
 class OptionGroup(object):
@@ -168,15 +175,18 @@ class Engine(object):
     min_api_version_obj = None  # LooseVersion object or None
 
     @abc.abstractmethod
-    def get_value(self, module, container, api_version, options):
+    def get_value(self, module, container, api_version, options, image, host_info):
         pass
 
+    def compare_value(self, option, param_value, container_value):
+        return option.compare(param_value, container_value)
+
     @abc.abstractmethod
     def set_value(self, module, data, api_version, options, values):
         pass
 
     @abc.abstractmethod
-    def get_expected_values(self, module, client, api_version, options, image, values):
+    def get_expected_values(self, module, client, api_version, options, image, values, host_info):
         pass
 
     @abc.abstractmethod
@@ -199,6 +209,14 @@ class Engine(object):
     def can_update_value(self, api_version):
         pass
 
+    @abc.abstractmethod
+    def needs_container_image(self, values):
+        pass
+
+    @abc.abstractmethod
+    def needs_host_info(self, values):
+        pass
+
 
 class EngineDriver(object):
     name = None  # string
@@ -208,6 +226,10 @@ class EngineDriver(object):
         # Return (module, active_options, client)
         pass
 
+    @abc.abstractmethod
+    def get_host_info(self, client):
+        pass
+
     @abc.abstractmethod
     def get_api_version(self, client):
         pass

plugins/module_utils/module_container/docker_api.py

@@ -181,6 +181,9 @@ class DockerAPIEngineDriver(EngineDriver):
 
         return client.module, active_options, client
 
+    def get_host_info(self, client):
+        return client.info()
+
     def get_api_version(self, client):
         return client.docker_api_version
 
@@ -216,7 +219,7 @@ class DockerAPIEngineDriver(EngineDriver):
         return client.get_container_by_id(container_id)
 
     def inspect_image_by_id(self, client, image_id):
-        return client.find_image_by_id(image_id)
+        return client.find_image_by_id(image_id, accept_missing_image=True)
 
     def inspect_image_by_name(self, client, repository, tag):
         return client.find_image(repository, tag)
@@ -387,18 +390,25 @@ class DockerAPIEngine(Engine):
         can_set_value=None,
         can_update_value=None,
         min_api_version=None,
+        compare_value=None,
+        needs_container_image=None,
+        needs_host_info=None,
     ):
         self.min_api_version = min_api_version
         self.min_api_version_obj = None if min_api_version is None else LooseVersion(min_api_version)
         self.get_value = get_value
         self.set_value = set_value
-        self.get_expected_values = get_expected_values or (lambda module, client, api_version, options, image, values: values)
+        self.get_expected_values = get_expected_values or (lambda module, client, api_version, options, image, values, host_info: values)
         self.ignore_mismatching_result = ignore_mismatching_result or \
             (lambda module, client, api_version, option, image, container_value, expected_value: False)
         self.preprocess_value = preprocess_value or (lambda module, client, api_version, options, values: values)
         self.update_value = update_value
         self.can_set_value = can_set_value or (lambda api_version: set_value is not None)
         self.can_update_value = can_update_value or (lambda api_version: update_value is not None)
+        self.needs_container_image = needs_container_image or (lambda values: False)
+        self.needs_host_info = needs_host_info or (lambda values: False)
+        if compare_value is not None:
+            self.compare_value = compare_value
 
     @classmethod
     def config_value(
@@ -423,7 +433,7 @@ class DockerAPIEngine(Engine):
                     values[options[0].name] = value
             return values
 
-        def get_value(module, container, api_version, options):
+        def get_value(module, container, api_version, options, image, host_info):
             if len(options) != 1:
                 raise AssertionError('config_value can only be used for a single option')
             value = container['Config'].get(config_name, _SENTRY)
@@ -435,7 +445,7 @@ class DockerAPIEngine(Engine):
 
         get_expected_values_ = None
         if get_expected_value:
-            def get_expected_values_(module, client, api_version, options, image, values):
+            def get_expected_values_(module, client, api_version, options, image, values, host_info):
                 if len(options) != 1:
                     raise AssertionError('host_config_value can only be used for a single option')
                 value = values.get(options[0].name, _SENTRY)
@@ -499,7 +509,7 @@ class DockerAPIEngine(Engine):
                     values[options[0].name] = value
             return values
 
-        def get_value(module, container, api_version, options):
+        def get_value(module, container, api_version, options, get_value, host_info):
             if len(options) != 1:
                 raise AssertionError('host_config_value can only be used for a single option')
             value = container['HostConfig'].get(host_config_name, _SENTRY)
@@ -511,7 +521,7 @@ class DockerAPIEngine(Engine):
 
         get_expected_values_ = None
         if get_expected_value:
-            def get_expected_values_(module, client, api_version, options, image, values):
+            def get_expected_values_(module, client, api_version, options, image, values, host_info):
                 if len(options) != 1:
                     raise AssertionError('host_config_value can only be used for a single option')
                 value = values.get(options[0].name, _SENTRY)
@@ -585,7 +595,7 @@ def _get_default_host_ip(module, client):
     return ip
 
 
-def _get_value_detach_interactive(module, container, api_version, options):
+def _get_value_detach_interactive(module, container, api_version, options, image, host_info):
     attach_stdin = container['Config'].get('OpenStdin')
     attach_stderr = container['Config'].get('AttachStderr')
     attach_stdout = container['Config'].get('AttachStdout')
@@ -836,7 +846,7 @@ def _get_network_id(module, client, network_name):
         client.fail("Error getting network id for %s - %s" % (network_name, to_native(exc)))
 
 
-def _get_values_network(module, container, api_version, options):
+def _get_values_network(module, container, api_version, options, image, host_info):
     value = container['HostConfig'].get('NetworkMode', _SENTRY)
     if value is _SENTRY:
         return {}
@@ -852,7 +862,7 @@ def _set_values_network(module, data, api_version, options, values):
     data['HostConfig']['NetworkMode'] = value
 
 
-def _get_values_mounts(module, container, api_version, options):
+def _get_values_mounts(module, container, api_version, options, image, host_info):
     volumes = container['Config'].get('Volumes')
     binds = container['HostConfig'].get('Binds')
     # According to https://github.com/moby/moby/, support for HostConfig.Mounts
@@ -916,7 +926,7 @@ def _get_image_binds(volumes):
     return results
 
 
-def _get_expected_values_mounts(module, client, api_version, options, image, values):
+def _get_expected_values_mounts(module, client, api_version, options, image, values, host_info):
     expected_values = {}
 
     # binds
@@ -1017,7 +1027,7 @@ def _set_values_mounts(module, data, api_version, options, values):
         data['HostConfig']['Binds'] = values['volume_binds']
 
 
-def _get_values_log(module, container, api_version, options):
+def _get_values_log(module, container, api_version, options, image, host_info):
     log_config = container['HostConfig'].get('LogConfig') or {}
     return {
         'log_driver': log_config.get('Type'),
@@ -1037,7 +1047,7 @@ def _set_values_log(module, data, api_version, options, values):
     data['HostConfig']['LogConfig'] = log_config
 
 
-def _get_values_platform(module, container, api_version, options):
+def _get_values_platform(module, container, api_version, options, image, host_info):
     return {
         'platform': container.get('Platform'),
     }
@@ -1048,7 +1058,7 @@ def _set_values_platform(module, data, api_version, options, values):
         data['platform'] = values['platform']
 
 
-def _get_values_restart(module, container, api_version, options):
+def _get_values_restart(module, container, api_version, options, image, host_info):
     restart_policy = container['HostConfig'].get('RestartPolicy') or {}
     return {
         'restart_policy': restart_policy.get('Name'),
@@ -1077,7 +1087,7 @@ def _update_value_restart(module, data, api_version, options, values):
     }
 
 
-def _get_values_ports(module, container, api_version, options):
+def _get_values_ports(module, container, api_version, options, image, host_info):
     host_config = container['HostConfig']
     config = container['Config']
 
@@ -1094,7 +1104,7 @@ def _get_values_ports(module, container, api_version, options):
     }
 
 
-def _get_expected_values_ports(module, client, api_version, options, image, values):
+def _get_expected_values_ports(module, client, api_version, options, image, values, host_info):
     expected_values = {}
 
     if 'published_ports' in values:

plugins/module_utils/module_container/module.py

@@ -268,6 +268,20 @@ class ContainerManager(DockerBaseClass):
             parameters.append((options, values))
         return parameters
 
+    def _needs_container_image(self):
+        for options, values in self.parameters:
+            engine = options.get_engine(self.engine_driver.name)
+            if engine.needs_container_image(values):
+                return True
+        return False
+
+    def _needs_host_info(self):
+        for options, values in self.parameters:
+            engine = options.get_engine(self.engine_driver.name)
+            if engine.needs_host_info(values):
+                return True
+        return False
+
     def present(self, state):
         self.parameters = self._collect_params(self.options)
         container = self._get_container(self.param_name)
@@ -280,8 +294,10 @@ class ContainerManager(DockerBaseClass):
         # the container already runs or not; in the former case, in case the
         # container needs to be restarted, we use the existing container's
         # image ID.
-        image, comparison_image = self._get_image(container)
+        image, container_image, comparison_image = self._get_image(
+            container, needs_container_image=self._needs_container_image())
         self.log(image, pretty_print=True)
+        host_info = self.engine_driver.get_host_info(self.client) if self._needs_host_info() else None
         if not container.exists or container.removing:
             # New container
             if container.removing:
@@ -301,7 +317,7 @@ class ContainerManager(DockerBaseClass):
             container_created = True
         else:
             # Existing container
-            different, differences = self.has_different_configuration(container, comparison_image)
+            different, differences = self.has_different_configuration(container, container_image, comparison_image, host_info)
             image_different = False
             if self.all_options['image'].comparison == 'strict':
                 image_different = self._image_is_different(image, container)
@@ -333,7 +349,7 @@ class ContainerManager(DockerBaseClass):
                 comparison_image = image
 
         if container and container.exists:
-            container = self.update_limits(container, comparison_image)
+            container = self.update_limits(container, container_image, comparison_image, host_info)
             container = self.update_networks(container, container_created)
 
             if state == 'started' and not container.running:
@@ -398,13 +414,20 @@ class ContainerManager(DockerBaseClass):
             image = self.engine_driver.inspect_image_by_name(self.client, repository, tag)
         return image or fallback
 
-    def _get_image(self, container):
+    def _get_image(self, container, needs_container_image=False):
         image_parameter = self.param_image
+        get_container_image = needs_container_image or not image_parameter
+        container_image = self._get_container_image(container) if get_container_image else None
+        if container_image:
+            self.log("current image")
+            self.log(container_image, pretty_print=True)
         if not image_parameter:
             self.log('No image specified')
-            return None, self._get_container_image(container)
+            return None, container_image, container_image
         if is_image_name_id(image_parameter):
             image = self.engine_driver.inspect_image_by_id(self.client, image_parameter)
+            if image is None:
+                self.client.fail("Cannot find image with ID %s" % (image_parameter, ))
         else:
             repository, tag = parse_repository_tag(image_parameter)
             if not tag:
@@ -431,12 +454,11 @@ class ContainerManager(DockerBaseClass):
 
         comparison_image = image
         if self.param_image_comparison == 'current-image':
-            comparison_image = self._get_container_image(container, image)
+            if not get_container_image:
-            if comparison_image != image:
+                container_image = self._get_container_image(container)
-                self.log("current image")
+            comparison_image = container_image
-                self.log(comparison_image, pretty_print=True)
 
-        return image, comparison_image
+        return image, container_image, comparison_image
 
     def _image_is_different(self, image, container):
         if image and image.get('Id'):
@@ -455,15 +477,16 @@ class ContainerManager(DockerBaseClass):
         params['Image'] = image
         return params
 
-    def _record_differences(self, differences, options, param_values, engine, container, image):
+    def _record_differences(self, differences, options, param_values, engine, container, container_image, image, host_info):
-        container_values = engine.get_value(self.module, container.raw, self.engine_driver.get_api_version(self.client), options.options)
+        container_values = engine.get_value(
+            self.module, container.raw, self.engine_driver.get_api_version(self.client), options.options, container_image, host_info)
         expected_values = engine.get_expected_values(
-            self.module, self.client, self.engine_driver.get_api_version(self.client), options.options, image, param_values.copy())
+            self.module, self.client, self.engine_driver.get_api_version(self.client), options.options, image, param_values.copy(), host_info)
         for option in options.options:
             if option.name in expected_values:
                 param_value = expected_values[option.name]
                 container_value = container_values.get(option.name)
-                match = compare_generic(param_value, container_value, option.comparison, option.comparison_type)
+                match = engine.compare_value(option, param_value, container_value)
 
                 if not match:
                     # No match.
@@ -497,28 +520,28 @@ class ContainerManager(DockerBaseClass):
                             c = sorted(c, key=sort_key_fn)
                     differences.add(option.name, parameter=p, active=c)
 
-    def has_different_configuration(self, container, image):
+    def has_different_configuration(self, container, container_image, image, host_info):
         differences = DifferenceTracker()
         update_differences = DifferenceTracker()
         for options, param_values in self.parameters:
             engine = options.get_engine(self.engine_driver.name)
             if engine.can_update_value(self.engine_driver.get_api_version(self.client)):
-                self._record_differences(update_differences, options, param_values, engine, container, image)
+                self._record_differences(update_differences, options, param_values, engine, container, container_image, image, host_info)
             else:
-                self._record_differences(differences, options, param_values, engine, container, image)
+                self._record_differences(differences, options, param_values, engine, container, container_image, image, host_info)
         has_differences = not differences.empty
         # Only consider differences of properties that can be updated when there are also other differences
         if has_differences:
             differences.merge(update_differences)
         return has_differences, differences
 
-    def has_different_resource_limits(self, container, image):
+    def has_different_resource_limits(self, container, container_image, image, host_info):
         differences = DifferenceTracker()
         for options, param_values in self.parameters:
             engine = options.get_engine(self.engine_driver.name)
             if not engine.can_update_value(self.engine_driver.get_api_version(self.client)):
                 continue
-            self._record_differences(differences, options, param_values, engine, container, image)
+            self._record_differences(differences, options, param_values, engine, container, container_image, image, host_info)
         has_differences = not differences.empty
         return has_differences, differences
 
@@ -531,8 +554,8 @@ class ContainerManager(DockerBaseClass):
             engine.update_value(self.module, result, self.engine_driver.get_api_version(self.client), options.options, values)
         return result
 
-    def update_limits(self, container, image):
+    def update_limits(self, container, container_image, image, host_info):
-        limits_differ, different_limits = self.has_different_resource_limits(container, image)
+        limits_differ, different_limits = self.has_different_resource_limits(container, container_image, image, host_info)
         if limits_differ:
             self.log("limit differences:")
             self.log(different_limits.get_legacy_docker_container_diffs(), pretty_print=True)