docker_container: fix handling of command and entrypoint in a backwards-compatible way (#186)

* Fix handling of command and entrypoint in a backwards-compatible way.

* Fix copy'n'paste error.

* Fix some more.

* Improve documentation.

* Keep command and entrypoint as lists and not as strings.

* Simplify code, since we're already emitting the deprecation warning in this case during parameter processing.

* Change default only in community.docker 3.0.0.

* Update tests/integration/targets/docker_container/tasks/tests/options.yml

Co-authored-by: Brian Scholer <1260690+briantist@users.noreply.github.com>

* Apply suggestion to more places.

Co-authored-by: Brian Scholer <1260690+briantist@users.noreply.github.com>

changelogs/fragments/186-docker_container-command-entrypoint.yml

@@ -0,0 +1,4 @@
+minor_changes:
+  - "docker_container - added new ``command_handling`` option with current deprecated default value ``compatibility`` which allows to control how the module handles shell quoting when interpreting lists, and how the module handles empty lists/strings. The default will switch to ``correct`` in community.docker 3.0.0 (https://github.com/ansible-collections/community.docker/pull/186)."
+deprecated_features:
+  - "docker_container - the new ``command_handling``'s default value, ``compatibility``, is deprecated and will change to ``correct`` in community.docker 3.0.0. A deprecation warning is emitted by the module in cases where the behavior will change. Please note that ansible-core will output a deprecation warning only once, so if it is shown for an earlier task, there could be more tasks with this warning where it is not shown (https://github.com/ansible-collections/community.docker/pull/186)."

plugins/modules/docker_container.py

@@ -66,6 +66,7 @@ options:
     description:
       - Command to execute when the container starts. A command may be either a string or a list.
       - Prior to version 2.4, strings were split on commas.
+      - See I(command_handling) for differences in how strings and lists are handled.
     type: raw
   comparisons:
     description:
@@ -103,6 +104,24 @@ options:
     choices:
       - compatibility
       - no_defaults
+  command_handling:
+    description:
+      - The default behavior for I(command) (when provided as a list) and I(entrypoint) is to
+        convert them to strings without considering shell quoting rules. (For comparing idempotency,
+        the resulting string is split considering shell quoting rules.)
+      - Also, setting I(command) to an empty list of string, and setting I(entrypoint) to an empty
+        list will be handled as if these options are not specified. This is different from idempotency
+        handling for other container-config related options.
+      - When this is set to C(compatiblity), which is the default until community.docker 3.0.0, the
+        current behavior will be kept.
+      - When this is set to C(correct), these options are kept as lists, and an empty value or empty
+        list will be handled correctly for idempotency checks.
+      - In community.docker 3.0.0, the default will change to C(correct).
+    type: str
+    choices:
+      - compatibility
+      - correct
+    version_added: 1.9.0
   cpu_period:
     description:
       - Limit CPU CFS (Completely Fair Scheduler) period.
@@ -296,6 +315,7 @@ options:
   entrypoint:
     description:
       - Command that overwrites the default C(ENTRYPOINT) of the image.
+      - See I(command_handling) for differences in how strings and lists are handled.
     type: list
     elements: str
   etc_hosts:
@@ -1186,6 +1206,7 @@ status:
 '''
 
 import os
+import pipes
 import re
 import shlex
 import traceback
@@ -1224,6 +1245,14 @@ except Exception:
     pass
 
 
+def shell_join(parts):
+    if getattr(shlex, 'quote', None):
+        quote = shlex.quote
+    else:
+        quote = pipes.quote
+    return ' '.join([quote(part) for part in parts])
+
+
 REQUIRES_CONVERSION_TO_BYTES = [
     'kernel_memory',
     'memory',
@@ -1480,14 +1509,47 @@ class TaskParameters(DockerBaseClass):
             # Ensure the MAC address uses colons instead of hyphens for later comparison
             self.mac_address = self.mac_address.replace('-', ':')
 
-        if self.entrypoint:
+        if client.module.params['command_handling'] == 'correct':
-            # convert from list to str.
+            if self.entrypoint is not None:
-            self.entrypoint = ' '.join([to_text(x, errors='surrogate_or_strict') for x in self.entrypoint])
+                self.entrypoint = [to_text(x, errors='surrogate_or_strict') for x in self.entrypoint]
 
-        if self.command:
+            if self.command is not None:
-            # convert from list to str
+                if not isinstance(self.command, list):
-            if isinstance(self.command, list):
+                    # convert from str to list
-                self.command = ' '.join([to_text(x, errors='surrogate_or_strict') for x in self.command])
+                    self.command = shlex.split(to_text(self.command, errors='surrogate_or_strict'))
+                self.command = [to_text(x, errors='surrogate_or_strict') for x in self.command]
+        else:
+            will_change = False
+            if self.entrypoint:
+                # convert from list to str.
+                correct_entrypoint = [to_text(x, errors='surrogate_or_strict') for x in self.entrypoint]
+                self.entrypoint = shlex.split(' '.join([to_text(x, errors='surrogate_or_strict') for x in self.entrypoint]))
+                self.entrypoint = [to_text(x, errors='surrogate_or_strict') for x in self.entrypoint]
+                if correct_entrypoint != self.entrypoint:
+                    will_change = True
+            elif self.entrypoint is not None:
+                will_change = True
+
+            if self.command:
+                # convert from list to str
+                if isinstance(self.command, list):
+                    correct_command = [to_text(x, errors='surrogate_or_strict') for x in self.command]
+                    self.command = shlex.split(' '.join([to_text(x, errors='surrogate_or_strict') for x in self.command]))
+                    self.command = [to_text(x, errors='surrogate_or_strict') for x in self.command]
+                    if correct_command != self.command:
+                        will_change = True
+                else:
+                    self.command = shlex.split(to_text(self.command, errors='surrogate_or_strict'))
+                    self.command = [to_text(x, errors='surrogate_or_strict') for x in self.command]
+            elif self.command is not None:
+                will_change = True
+
+            if will_change and client.module.params['command_handling'] is None:
+                client.module.deprecate(
+                    'The command_handling option will change its default value from "compatibility" to '
+                    '"correct" in community.docker 3.0.0. To remove this warning, please specify an explicit value for it now',
+                    version='3.0.0', collection_name='community.docker'
+                )
 
         self.mounts_opt, self.expected_mounts = self._process_mounts()
 
@@ -2545,9 +2607,9 @@ class Container(DockerBaseClass):
         return expected_devices
 
     def _get_expected_entrypoint(self):
-        if not self.parameters.entrypoint:
+        if self.parameters.client.module.params['command_handling'] != 'correct' and not self.parameters.entrypoint:
             return None
-        return shlex.split(self.parameters.entrypoint)
+        return self.parameters.entrypoint
 
     def _get_expected_ports(self):
         if self.parameters.published_ports is None:
@@ -2719,9 +2781,9 @@ class Container(DockerBaseClass):
 
     def _get_expected_cmd(self):
         self.log('_get_expected_cmd')
-        if not self.parameters.command:
+        if self.parameters.client.module.params['command_handling'] != 'correct' and not self.parameters.command:
             return None
-        return shlex.split(self.parameters.command)
+        return self.parameters.command
 
     def _convert_simple_dict_to_list(self, param_name, join_with=':'):
         if getattr(self.parameters, param_name, None) is None:
@@ -3247,7 +3309,7 @@ class AnsibleDockerClientContainer(AnsibleDockerClient):
     __NON_CONTAINER_PROPERTY_OPTIONS = tuple([
         'env_file', 'force_kill', 'keep_volumes', 'ignore_image', 'name', 'pull', 'purge_networks',
         'recreate', 'restart', 'state', 'networks', 'cleanup', 'kill_signal',
-        'output_logs', 'paused', 'removal_wait_timeout', 'default_host_ip',
+        'output_logs', 'paused', 'removal_wait_timeout', 'default_host_ip', 'command_handling',
     ] + list(DOCKER_COMMON_ARGS.keys()))
 
     def _parse_comparisons(self):
@@ -3468,6 +3530,7 @@ def main():
         command=dict(type='raw'),
         comparisons=dict(type='dict'),
         container_default_behavior=dict(type='str', choices=['compatibility', 'no_defaults']),
+        command_handling=dict(type='str', choices=['compatibility', 'correct']),
         cpu_period=dict(type='int'),
         cpu_quota=dict(type='int'),
         cpus=dict(type='float'),

tests/integration/targets/docker_container/tasks/tests/options.yml

@@ -195,30 +195,58 @@
 ## command #########################################################
 ####################################################################
 
-- name: command
+# old
+
+- name: command (compatibility)
   docker_container:
     image: "{{ docker_test_image_alpine }}"
+    command_handling: compatibility
     command: '/bin/sh -v -c "sleep 10m"'
     name: "{{ cname }}"
     state: started
   register: command_1
 
-- name: command (idempotency)
+- name: command (compatibility, idempotency)
   docker_container:
     image: "{{ docker_test_image_alpine }}"
+    command_handling: compatibility
     command: '/bin/sh -v -c "sleep 10m"'
     name: "{{ cname }}"
     state: started
   register: command_2
 
-- name: command (less parameters)
+- name: command (compatibility, idempotency, list)
   docker_container:
     image: "{{ docker_test_image_alpine }}"
+    command_handling: compatibility
+    command:
+      - /bin/sh
+      - '-v'
+      - '-c'
+      - '"sleep 10m"'
+    name: "{{ cname }}"
+    state: started
+  register: command_3
+
+- name: command (compatibility, fewer parameters)
+  docker_container:
+    image: "{{ docker_test_image_alpine }}"
+    command_handling: compatibility
     command: '/bin/sh -c "sleep 10m"'
     name: "{{ cname }}"
     state: started
     force_kill: yes
-  register: command_3
+  register: command_4
+
+- name: command (compatibility, empty list)
+  docker_container:
+    image: "{{ docker_test_image_alpine }}"
+    command_handling: compatibility
+    command: []
+    name: "{{ cname }}"
+    state: started
+    force_kill: yes
+  register: command_5
 
 - name: cleanup
   docker_container:
@@ -231,7 +259,77 @@
     that:
     - command_1 is changed
     - command_2 is not changed
-    - command_3 is changed
+    - command_3 is not changed
+    - command_4 is changed
+    - command_5 is not changed
+
+# new
+
+- name: command (correct)
+  docker_container:
+    image: "{{ docker_test_image_alpine }}"
+    command_handling: correct
+    command: '/bin/sh -v -c "sleep 10m"'
+    name: "{{ cname }}"
+    state: started
+  register: command_1
+
+- name: command (correct, idempotency)
+  docker_container:
+    image: "{{ docker_test_image_alpine }}"
+    command_handling: correct
+    command: '/bin/sh -v -c "sleep 10m"'
+    name: "{{ cname }}"
+    state: started
+  register: command_2
+
+- name: command (correct, idempotency, list)
+  docker_container:
+    image: "{{ docker_test_image_alpine }}"
+    command_handling: correct
+    command:
+      - /bin/sh
+      - '-v'
+      - '-c'
+      - sleep 10m
+    name: "{{ cname }}"
+    state: started
+  register: command_3
+
+- name: command (correct, fewer parameters)
+  docker_container:
+    image: "{{ docker_test_image_alpine }}"
+    command_handling: correct
+    command: '/bin/sh -c "sleep 10m"'
+    name: "{{ cname }}"
+    state: started
+    force_kill: yes
+  register: command_4
+
+- name: command (correct, empty list)
+  docker_container:
+    image: "{{ docker_test_image_alpine }}"
+    command_handling: correct
+    command: []
+    name: "{{ cname }}"
+    state: started
+    force_kill: yes
+  register: command_5
+
+- name: cleanup
+  docker_container:
+    name: "{{ cname }}"
+    state: absent
+    force_kill: yes
+  diff: no
+
+- assert:
+    that:
+    - command_1 is changed
+    - command_2 is not changed
+    - command_3 is not changed
+    - command_4 is changed
+    - command_5 is changed
 
 ####################################################################
 ## cpu_period ######################################################
@@ -1235,9 +1333,12 @@
 ## entrypoint ######################################################
 ####################################################################
 
-- name: entrypoint
+# Old
+
+- name: entrypoint (compatibility)
   docker_container:
     image: "{{ docker_test_image_alpine }}"
+    command_handling: compatibility
     entrypoint:
     - /bin/sh
     - "-v"
@@ -1247,9 +1348,10 @@
     state: started
   register: entrypoint_1
 
-- name: entrypoint (idempotency)
+- name: entrypoint (compatibility, idempotency)
   docker_container:
     image: "{{ docker_test_image_alpine }}"
+    command_handling: compatibility
     entrypoint:
     - /bin/sh
     - "-v"
@@ -1259,9 +1361,10 @@
     state: started
   register: entrypoint_2
 
-- name: entrypoint (change order, should not be idempotent)
+- name: entrypoint (compatibility, change order, should not be idempotent)
   docker_container:
     image: "{{ docker_test_image_alpine }}"
+    command_handling: compatibility
     entrypoint:
     - /bin/sh
     - "-c"
@@ -1272,9 +1375,10 @@
     force_kill: yes
   register: entrypoint_3
 
-- name: entrypoint (less parameters)
+- name: entrypoint (compatibility, fewer parameters)
   docker_container:
     image: "{{ docker_test_image_alpine }}"
+    command_handling: compatibility
     entrypoint:
     - /bin/sh
     - "-c"
@@ -1284,9 +1388,10 @@
     force_kill: yes
   register: entrypoint_4
 
-- name: entrypoint (other parameters)
+- name: entrypoint (compatibility, other parameters)
   docker_container:
     image: "{{ docker_test_image_alpine }}"
+    command_handling: compatibility
     entrypoint:
     - /bin/sh
     - "-c"
@@ -1296,6 +1401,16 @@
     force_kill: yes
   register: entrypoint_5
 
+- name: entrypoint (compatibility, force empty)
+  docker_container:
+    image: "{{ docker_test_image_alpine }}"
+    command_handling: compatibility
+    entrypoint: []
+    name: "{{ cname }}"
+    state: started
+    force_kill: yes
+  register: entrypoint_6
+
 - name: cleanup
   docker_container:
     name: "{{ cname }}"
@@ -1310,6 +1425,101 @@
     - entrypoint_3 is changed
     - entrypoint_4 is changed
     - entrypoint_5 is changed
+    - entrypoint_6 is not changed
+
+# New
+
+- name: entrypoint (correct)
+  docker_container:
+    image: "{{ docker_test_image_alpine }}"
+    command_handling: correct
+    entrypoint:
+    - /bin/sh
+    - "-v"
+    - "-c"
+    - "sleep 10m"
+    name: "{{ cname }}"
+    state: started
+  register: entrypoint_1
+
+- name: entrypoint (correct, idempotency)
+  docker_container:
+    image: "{{ docker_test_image_alpine }}"
+    command_handling: correct
+    entrypoint:
+    - /bin/sh
+    - "-v"
+    - "-c"
+    - "sleep 10m"
+    name: "{{ cname }}"
+    state: started
+  register: entrypoint_2
+
+- name: entrypoint (correct, change order, should not be idempotent)
+  docker_container:
+    image: "{{ docker_test_image_alpine }}"
+    command_handling: correct
+    entrypoint:
+    - /bin/sh
+    - "-c"
+    - "sleep 10m"
+    - "-v"
+    name: "{{ cname }}"
+    state: started
+    force_kill: yes
+  register: entrypoint_3
+
+- name: entrypoint (correct, fewer parameters)
+  docker_container:
+    image: "{{ docker_test_image_alpine }}"
+    command_handling: correct
+    entrypoint:
+    - /bin/sh
+    - "-c"
+    - "sleep 10m"
+    name: "{{ cname }}"
+    state: started
+    force_kill: yes
+  register: entrypoint_4
+
+- name: entrypoint (correct, other parameters)
+  docker_container:
+    image: "{{ docker_test_image_alpine }}"
+    command_handling: correct
+    entrypoint:
+    - /bin/sh
+    - "-c"
+    - "sleep 5m"
+    name: "{{ cname }}"
+    state: started
+    force_kill: yes
+  register: entrypoint_5
+
+- name: entrypoint (correct, force empty)
+  docker_container:
+    image: "{{ docker_test_image_alpine }}"
+    command_handling: correct
+    entrypoint: []
+    name: "{{ cname }}"
+    state: started
+    force_kill: yes
+  register: entrypoint_6
+
+- name: cleanup
+  docker_container:
+    name: "{{ cname }}"
+    state: absent
+    force_kill: yes
+  diff: no
+
+- assert:
+    that:
+    - entrypoint_1 is changed
+    - entrypoint_2 is not changed
+    - entrypoint_3 is changed
+    - entrypoint_4 is changed
+    - entrypoint_5 is changed
+    - entrypoint_6 is changed
 
 ####################################################################
 ## env #############################################################