From 6f52747693b72806cf50b29e485d7e2e0b6709bd Mon Sep 17 00:00:00 2001 From: Felix Fontein Date: Mon, 2 Aug 2021 19:31:46 +0200 Subject: [PATCH] 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> --- ...86-docker_container-command-entrypoint.yml | 4 + plugins/modules/docker_container.py | 87 ++++++- .../docker_container/tasks/tests/options.yml | 230 +++++++++++++++++- 3 files changed, 299 insertions(+), 22 deletions(-) create mode 100644 changelogs/fragments/186-docker_container-command-entrypoint.yml diff --git a/changelogs/fragments/186-docker_container-command-entrypoint.yml b/changelogs/fragments/186-docker_container-command-entrypoint.yml new file mode 100644 index 00000000..2537caa6 --- /dev/null +++ b/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)." diff --git a/plugins/modules/docker_container.py b/plugins/modules/docker_container.py index 5ae05e22..82b8b429 100644 --- a/plugins/modules/docker_container.py +++ b/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: - # convert from list to str. - self.entrypoint = ' '.join([to_text(x, errors='surrogate_or_strict') for x in self.entrypoint]) + if client.module.params['command_handling'] == 'correct': + if self.entrypoint is not None: + self.entrypoint = [to_text(x, errors='surrogate_or_strict') for x in self.entrypoint] - if self.command: - # convert from list to str - if isinstance(self.command, list): - self.command = ' '.join([to_text(x, errors='surrogate_or_strict') for x in self.command]) + if self.command is not None: + if not isinstance(self.command, list): + # convert from str to list + 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'), diff --git a/tests/integration/targets/docker_container/tasks/tests/options.yml b/tests/integration/targets/docker_container/tasks/tests/options.yml index 5f9a7667..170eeb3f 100644 --- a/tests/integration/targets/docker_container/tasks/tests/options.yml +++ b/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 #############################################################