Add proper platform handling. (#705)

2023-12-10 09:03:32 +01:00 · 2023-12-10 09:03:32 +01:00 · c4c347c626
parent b3ef5f5196
commit c4c347c626
7 changed files with 285 additions and 12 deletions
--- a/LICENSES/Apache-2.0.txt
+++ b/LICENSES/Apache-2.0.txt
@ -1,7 +1,7 @@

                                 Apache License
                           Version 2.0, January 2004
-                        http://www.apache.org/licenses/
+                        https://www.apache.org/licenses/

   TERMS AND CONDITIONS FOR USE, REPRODUCTION, AND DISTRIBUTION

@ -176,13 +176,13 @@

   END OF TERMS AND CONDITIONS

-   Copyright 2016 Docker, Inc.
+   Copyright [yyyy] [name of copyright owner]

   Licensed under the Apache License, Version 2.0 (the "License");
   you may not use this file except in compliance with the License.
   You may obtain a copy of the License at

-       http://www.apache.org/licenses/LICENSE-2.0
+       https://www.apache.org/licenses/LICENSE-2.0

   Unless required by applicable law or agreed to in writing, software
   distributed under the License is distributed on an "AS IS" BASIS,
--- a/changelogs/fragments/705-docker_container-platform.yml
+++ b/changelogs/fragments/705-docker_container-platform.yml
@ -0,0 +1,2 @@
+minor_changes:
+  - "docker_container - implement better ``platform`` string comparisons to improve idempotency (https://github.com/ansible-collections/community.docker/issues/654, https://github.com/ansible-collections/community.docker/pull/705)."
--- a/plugins/module_utils/_platform.py
+++ b/plugins/module_utils/_platform.py
@ -0,0 +1,179 @@
+# This code is part of the Ansible collection community.docker, but is an independent component.
+# This particular file, and this file only, is based on containerd's platforms Go module
+# (https://github.com/containerd/containerd/tree/main/platforms)
+#
+# Copyright (c) 2023 Felix Fontein <felix@fontein.de>
+# Copyright The containerd Authors
+#
+# It is licensed under the Apache 2.0 license (see LICENSES/Apache-2.0.txt in this collection)
+# SPDX-License-Identifier: Apache-2.0
+
+from __future__ import (absolute_import, division, print_function)
+__metaclass__ = type
+
+import re
+
+
+_VALID_STR = re.compile('^[A-Za-z0-9_-]+$')
+
+
+def _validate_part(string, part, part_name):
+    if not part:
+        raise ValueError('Invalid platform string "{string}": {part} is empty'.format(string=string, part=part_name))
+    if not _VALID_STR.match(part):
+        raise ValueError('Invalid platform string "{string}": {part} has invalid characters'.format(string=string, part=part_name))
+    return part
+
+
+# See https://github.com/containerd/containerd/blob/main/platforms/database.go#L32-L38
+_KNOWN_OS = (
+    "aix", "android", "darwin", "dragonfly", "freebsd", "hurd", "illumos", "ios", "js",
+    "linux", "nacl", "netbsd", "openbsd", "plan9", "solaris", "windows", "zos",
+)
+
+# See https://github.com/containerd/containerd/blob/main/platforms/database.go#L54-L60
+_KNOWN_ARCH = (
+    "386", "amd64", "amd64p32", "arm", "armbe", "arm64", "arm64be", "ppc64", "ppc64le",
+    "loong64", "mips", "mipsle", "mips64", "mips64le", "mips64p32", "mips64p32le",
+    "ppc", "riscv", "riscv64", "s390", "s390x", "sparc", "sparc64", "wasm",
+)
+
+
+def _normalize_os(os_str):
+    # See normalizeOS() in https://github.com/containerd/containerd/blob/main/platforms/database.go
+    os_str = os_str.lower()
+    if os_str == 'macos':
+        os_str = 'darwin'
+    return os_str
+
+
+_NORMALIZE_ARCH = {
+    ("i386", None): ("386", ""),
+    ("x86_64", "v1"): ("amd64", ""),
+    ("x86-64", "v1"): ("amd64", ""),
+    ("amd64", "v1"): ("amd64", ""),
+    ("x86_64", None): ("amd64", None),
+    ("x86-64", None): ("amd64", None),
+    ("amd64", None): ("amd64", None),
+    ("aarch64", "8"): ("arm64", ""),
+    ("arm64", "8"): ("arm64", ""),
+    ("aarch64", "v8"): ("arm64", ""),
+    ("arm64", "v8"): ("arm64", ""),
+    ("aarch64", None): ("arm64", None),
+    ("arm64", None): ("arm64", None),
+    ("armhf", None): ("arm", "v7"),
+    ("armel", None): ("arm", "v6"),
+    ("arm", ""): ("arm", "v7"),
+    ("arm", "5"): ("arm", "v5"),
+    ("arm", "6"): ("arm", "v6"),
+    ("arm", "7"): ("arm", "v7"),
+    ("arm", "8"): ("arm", "v8"),
+    ("arm", None): ("arm", None),
+}
+
+
+def _normalize_arch(arch_str, variant_str):
+    # See normalizeArch() in https://github.com/containerd/containerd/blob/main/platforms/database.go
+    arch_str = arch_str.lower()
+    variant_str = variant_str.lower()
+    res = _NORMALIZE_ARCH.get((arch_str, variant_str))
+    if res is None:
+        res = _NORMALIZE_ARCH.get((arch_str, None))
+    if res is None:
+        return arch_str, variant_str
+    if res is not None:
+        arch_str = res[0]
+        if res[1] is not None:
+            variant_str = res[1]
+        return arch_str, variant_str
+
+
+class _Platform(object):
+    def __init__(self, os=None, arch=None, variant=None):
+        self.os = os
+        self.arch = arch
+        self.variant = variant
+        if variant is not None:
+            if arch is None:
+                raise ValueError('If variant is given, architecture must be given too')
+            if os is None:
+                raise ValueError('If variant is given, os must be given too')
+
+    @classmethod
+    def parse_platform_string(cls, string, daemon_os=None, daemon_arch=None):
+        # See Parse() in https://github.com/containerd/containerd/blob/main/platforms/platforms.go
+        if string is None:
+            return cls()
+        if not string:
+            raise ValueError('Platform string must be non-empty')
+        parts = string.split('/', 2)
+        arch = None
+        variant = None
+        if len(parts) == 1:
+            _validate_part(string, string, 'OS/architecture')
+            # The part is either OS or architecture
+            os = _normalize_os(string)
+            if os in _KNOWN_OS:
+                if daemon_arch is not None:
+                    arch, variant = _normalize_arch(daemon_arch, '')
+                return cls(os=os, arch=arch, variant=variant)
+            arch, variant = _normalize_arch(os, '')
+            if arch in _KNOWN_ARCH:
+                return cls(
+                    os=_normalize_os(daemon_os) if daemon_os else None,
+                    arch=arch or None,
+                    variant=variant or None,
+                )
+            raise ValueError('Invalid platform string "{0}": unknown OS or architecture'.format(string))
+        os = _validate_part(string, parts[0], 'OS')
+        if not os:
+            raise ValueError('Invalid platform string "{0}": OS is empty'.format(string))
+        arch = _validate_part(string, parts[1], 'architecture') if len(parts) > 1 else None
+        if arch is not None and not arch:
+            raise ValueError('Invalid platform string "{0}": architecture is empty'.format(string))
+        variant = _validate_part(string, parts[2], 'variant') if len(parts) > 2 else None
+        if variant is not None and not variant:
+            raise ValueError('Invalid platform string "{0}": variant is empty'.format(string))
+        arch, variant = _normalize_arch(arch, variant or '')
+        if len(parts) == 2 and arch == 'arm' and variant == 'v7':
+            variant = None
+        if len(parts) == 3 and arch == 'arm64' and variant == '':
+            variant = 'v8'
+        return cls(os=_normalize_os(os), arch=arch, variant=variant or None)
+
+    def __str__(self):
+        if self.variant:
+            parts = [self.os, self.arch, self.variant]
+        elif self.os:
+            if self.arch:
+                parts = [self.os, self.arch]
+            else:
+                parts = [self.os]
+        elif self.arch is not None:
+            parts = [self.arch]
+        else:
+            parts = []
+        return '/'.join(parts)
+
+    def __repr__(self):
+        return '_Platform(os={os!r}, arch={arch!r}, variant={variant!r})'.format(os=self.os, arch=self.arch, variant=self.variant)
+
+    def __eq__(self, other):
+        return self.os == other.os and self.arch == other.arch and self.variant == other.variant
+
+
+def normalize_platform_string(string, daemon_os=None, daemon_arch=None):
+    return str(_Platform.parse_platform_string(string, daemon_os=daemon_os, daemon_arch=daemon_arch))
+
+
+def compose_platform_string(os=None, arch=None, variant=None, daemon_os=None, daemon_arch=None):
+    if os is None and daemon_os is not None:
+        os = _normalize_os(daemon_os)
+    if arch is None and daemon_arch is not None:
+        arch, variant = _normalize_arch(daemon_arch, variant or '')
+        variant = variant or None
+    return str(_Platform(os=os, arch=arch, variant=variant or None))
+
+
+def compare_platform_strings(string1, string2):
+    return _Platform.parse_platform_string(string1) == _Platform.parse_platform_string(string2)
--- a/plugins/module_utils/module_container/base.py
+++ b/plugins/module_utils/module_container/base.py
@ -24,6 +24,10 @@ from ansible_collections.community.docker.plugins.module_utils.util import (
    omit_none_from_dict,
 )

+from ansible_collections.community.docker.plugins.module_utils._platform import (
+    compare_platform_strings,
+)
+
 from ansible_collections.community.docker.plugins.module_utils._api.utils.utils import (
    parse_env_file,
 )
@ -755,6 +759,15 @@ def _preprocess_ports(module, values):
    return values


+def _compare_platform(option, param_value, container_value):
+    if option.comparison == 'ignore':
+        return True
+    try:
+        return compare_platform_strings(param_value, container_value)
+    except ValueError:
+        return param_value == container_value
+
+
 OPTION_AUTO_REMOVE = (
    OptionGroup()
    .add_option('auto_remove', type='bool')
@ -1031,7 +1044,7 @@ OPTION_PIDS_LIMIT = (

 OPTION_PLATFORM = (
    OptionGroup()
-    .add_option('platform', type='str')
+    .add_option('platform', type='str', compare=_compare_platform)
 )

 OPTION_PRIVILEGED = (
--- a/plugins/module_utils/module_container/docker_api.py
+++ b/plugins/module_utils/module_container/docker_api.py
@ -17,6 +17,11 @@ from ansible_collections.community.docker.plugins.module_utils.common_api import
    RequestException,
 )

+from ansible_collections.community.docker.plugins.module_utils._platform import (
+    compose_platform_string,
+    normalize_platform_string,
+)
+
 from ansible_collections.community.docker.plugins.module_utils.module_container.base import (
    OPTION_AUTO_REMOVE,
    OPTION_BLKIO_WEIGHT,
@ -1048,16 +1053,48 @@ def _set_values_log(module, data, api_version, options, values):


 def _get_values_platform(module, container, api_version, options, image, host_info):
+    if image and (image.get('Os') or image.get('Architecture') or image.get('Variant')):
+        return {
+            'platform': compose_platform_string(
+                os=image.get('Os'),
+                arch=image.get('Architecture'),
+                variant=image.get('Variant'),
+                daemon_os=host_info.get('OSType') if host_info else None,
+                daemon_arch=host_info.get('Architecture') if host_info else None,
+            )
+        }
    return {
        'platform': container.get('Platform'),
    }


+def _get_expected_values_platform(module, client, api_version, options, image, values, host_info):
+    expected_values = {}
+    if 'platform' in values:
+        try:
+            expected_values['platform'] = normalize_platform_string(
+                values['platform'],
+                daemon_os=host_info.get('OSType') if host_info else None,
+                daemon_arch=host_info.get('Architecture') if host_info else None,
+            )
+        except ValueError as exc:
+            module.fail_json(msg='Error while parsing platform parameer: %s' % (to_native(exc), ))
+    return expected_values
+
+
 def _set_values_platform(module, data, api_version, options, values):
    if 'platform' in values:
        data['platform'] = values['platform']


+def _needs_container_image_platform(values):
+    return 'platform' in values
+
+
+def _needs_host_info_platform(values):
+    return 'platform' in values
+
+
 def _get_values_restart(module, container, api_version, options, image, host_info):
    restart_policy = container['HostConfig'].get('RestartPolicy') or {}
    return {
@ -1306,6 +1343,9 @@ OPTION_PIDS_LIMIT.add_engine('docker_api', DockerAPIEngine.host_config_value('Pi
 OPTION_PLATFORM.add_engine('docker_api', DockerAPIEngine(
    get_value=_get_values_platform,
    set_value=_set_values_platform,
+    get_expected_values=_get_expected_values_platform,
+    needs_container_image=_needs_container_image_platform,
+    needs_host_info=_needs_host_info_platform,
    min_api_version='1.41',
 ))

--- a/plugins/modules/docker_container.py
+++ b/plugins/modules/docker_container.py
@ -733,9 +733,12 @@ options:
  platform:
    description:
      - Platform for the container in the format C(os[/arch[/variant]]).
-      - "Please note that inspecting the container does not always return the exact platform string used to
-         create the container. This can cause idempotency to break for this module. Use the O(comparisons) option
-         with C(platform: ignore) to prevent accidental recreation of the container due to this."
+      - "Note that since community.docker 3.5.0, the module uses both the image's metadata and the Docker
+         daemon's information to normalize platform strings similarly to how Docker itself is doing this.
+         If you notice idempotency problems, L(please create an issue in the community.docker GitHub repository,
+         https://github.com/ansible-collections/community.docker/issues/new?assignees=&labels=&projects=&template=bug_report.md).
+         For older community.docker versions, you can use the O(comparisons) option with C(platform: ignore)
+         to prevent accidental recreation of the container due to this."
    type: str
    version_added: 3.0.0
  privileged:
--- a/tests/integration/targets/docker_container/tasks/tests/options.yml
+++ b/tests/integration/targets/docker_container/tasks/tests/options.yml
@ -3564,17 +3564,38 @@ avoid such warnings, please quote the value.' in (log_options_2.warnings | defau
  register: platform_1
  ignore_errors: true

- name: platform (idempotency)
+- name: platform (idempotency with full name)
+  # Docker daemon only returns 'linux' as the platform for the container,
+  # so this has to be handled correctly by our additional code
  docker_container:
    image: hello-world:latest
    name: "{{ cname }}"
    state: present
-    # The container always reports 'linux' as platform instead of 'linux/amd64'...
-    platform: linux
+    platform: linux/amd64
    debug: true
  register: platform_2
  ignore_errors: true

+- name: platform (idempotency with shorter name)
+  docker_container:
+    image: hello-world:latest
+    name: "{{ cname }}"
+    state: present
+    platform: linux
+    debug: true
+  register: platform_3
+  ignore_errors: true
+
+- name: platform (idempotency with shorter name)
+  docker_container:
+    image: hello-world:latest
+    name: "{{ cname }}"
+    state: present
+    platform: amd64
+    debug: true
+  register: platform_4
+  ignore_errors: true
+
 - name: platform (changed)
  docker_container:
    image: hello-world:latest
@ -3587,7 +3608,19 @@ avoid such warnings, please quote the value.' in (log_options_2.warnings | defau
    comparisons:
      # Do not restart because of the changed image ID
      image: ignore
-  register: platform_3
+  register: platform_5
+  ignore_errors: true
+
+- name: platform (idempotency)
+  docker_container:
+    image: hello-world:latest
+    name: "{{ cname }}"
+    state: present
+    pull: true
+    platform: 386
+    force_kill: true
+    debug: true
+  register: platform_6
  ignore_errors: true

 - name: cleanup
@ -3601,7 +3634,10 @@ avoid such warnings, please quote the value.' in (log_options_2.warnings | defau
    that:
    - platform_1 is changed
    - platform_2 is not changed and platform_2 is not failed
-    - platform_3 is changed
+    - platform_3 is not changed and platform_3 is not failed
+    - platform_4 is not changed and platform_4 is not failed
+    - platform_5 is changed
+    - platform_6 is not changed and platform_6 is not failed
  when: docker_api_version is version('1.41', '>=')
 - assert:
    that: