docker_network: fix multiple subnet (of same IP version) idempotence (a…

…nsible#65839)

* Fix multiple subnet (of same IP version) idempotence for docker_network.

* Add changelog.

* Unit tests no longer make sense, since the part of the code they test has been removed.

* Re-add CIDR validation. Move it to better position (module setup instead of idempotence check).

* Update changelog.

* Only run new tests on VM test images.

* Actually do what is documented. Especially since an empty object is a valid value for aux_addresses.

changelogs/fragments/65839-docker_network-idempotence.yml

@@ -0,0 +1,3 @@
+bugfixes:
+- "docker_network - fix idempotency for multiple IPAM configs of the same IP version (https://github.com/ansible/ansible/issues/65815)."
+- "docker_network - validate IPAM config subnet CIDR notation on module setup and not during idempotence checking."

lib/ansible/modules/cloud/docker/docker_network.py

@@ -347,8 +347,8 @@ def container_names_in_network(network):
 CIDR_IPV6 = re.compile(r'^[0-9a-fA-F:]+/([0-9]|[1-9][0-9]|1[0-2][0-9])$')
 
 
-def get_ip_version(cidr):
-    """Gets the IP version of a CIDR string
+def validate_cidr(cidr):
+    """Validate CIDR. Return IP version of a CIDR string on success.
 
     :param cidr: Valid CIDR
     :type cidr: str
@@ -364,7 +364,7 @@ def get_ip_version(cidr):
 
 
 def normalize_ipam_config_key(key):
-    """Normalizes IPAM config keys returned by Docker API to match Ansible keys
+    """Normalizes IPAM config keys returned by Docker API to match Ansible keys.
 
     :param key: Docker API key
     :type key: str
@@ -377,6 +377,16 @@ def normalize_ipam_config_key(key):
     return special_cases.get(key, key.lower())
 
 
+def dicts_are_essentially_equal(a, b):
+    """Make sure that a is a subset of b, where None entries of a are ignored."""
+    for k, v in a.items():
+        if v is None:
+            continue
+        if b.get(k) != v:
+            return False
+    return True
+
+
 class DockerNetworkManager(object):
 
     def __init__(self, client):
@@ -400,6 +410,13 @@ def __init__(self, client):
                 self.parameters.ipam_options['gateway'] or self.parameters.ipam_options['aux_addresses']):
             self.parameters.ipam_config = [self.parameters.ipam_options]
 
+        if self.parameters.ipam_config:
+            try:
+                for ipam_config in self.parameters.ipam_config:
+                    validate_cidr(ipam_config['subnet'])
+            except ValueError as e:
+                self.client.fail(str(e))
+
         if self.parameters.driver_options:
             self.parameters.driver_options = clean_dict_booleans_for_docker_api(self.parameters.driver_options)
 
@@ -461,30 +478,29 @@ def has_different_config(self, net):
                                 parameter=self.parameters.ipam_config,
                                 active=net.get('IPAM', {}).get('Config'))
             else:
+                # Put network's IPAM config into the same format as module's IPAM config
+                net_ipam_configs = []
+                for net_ipam_config in net['IPAM']['Config']:
+                    config = dict()
+                    for k, v in net_ipam_config.items():
+                        config[normalize_ipam_config_key(k)] = v
+                    net_ipam_configs.append(config)
+                # Compare lists of dicts as sets of dicts
                 for idx, ipam_config in enumerate(self.parameters.ipam_config):
                     net_config = dict()
-                    try:
-                        ip_version = get_ip_version(ipam_config['subnet'])
-                        for net_ipam_config in net['IPAM']['Config']:
-                            if ip_version == get_ip_version(net_ipam_config['Subnet']):
-                                net_config = net_ipam_config
-                    except ValueError as e:
-                        self.client.fail(str(e))
-
+                    for net_ipam_config in net_ipam_configs:
+                        if dicts_are_essentially_equal(ipam_config, net_ipam_config):
+                            net_config = net_ipam_config
+                            break
                     for key, value in ipam_config.items():
                         if value is None:
                             # due to recursive argument_spec, all keys are always present
                             # (but have default value None if not specified)
                             continue
-                        camelkey = None
-                        for net_key in net_config:
-                            if key == normalize_ipam_config_key(net_key):
-                                camelkey = net_key
-                                break
-                        if not camelkey or net_config.get(camelkey) != value:
+                        if value != net_config.get(key):
                             differences.add('ipam_config[%s].%s' % (idx, key),
                                             parameter=value,
-                                            active=net_config.get(camelkey) if camelkey else None)
+                                            active=net_config.get(key))
 
         if self.parameters.enable_ipv6 is not None and self.parameters.enable_ipv6 != net.get('EnableIPv6', False):
             differences.add('enable_ipv6',

test/integration/targets/docker_network/tasks/tests/ipam.yml

@@ -11,7 +11,7 @@
     dnetworks: "{{ dnetworks + [nname_ipam_0, nname_ipam_1, nname_ipam_2, nname_ipam_3] }}"
 
 
-#################### network-ipam-0 deprecated ####################
+#################### Deprecated ipam_config ####################
 
 - name: Create network with ipam_config and deprecated ipam_options (conflicting)
   docker_network:
@@ -98,7 +98,7 @@
     state: absent
 
 
-#################### network-ipam-1 ####################
+#################### IPv4 IPAM config ####################
 - name: Create network with custom IPAM config
   docker_network:
     name: "{{ nname_ipam_1 }}"
@@ -169,7 +169,7 @@
     state: absent
 
 
-#################### network-ipam-2 ####################
+#################### IPv6 IPAM config ####################
 
 - name: Create network with IPv6 IPAM config
   docker_network:
@@ -230,7 +230,7 @@
     state: absent
 
 
-#################### network-ipam-3 ####################
+#################### IPv4 and IPv6 network ####################
 
 - name: Create network with IPv6 and custom IPv4 IPAM config
   docker_network:
@@ -279,7 +279,79 @@
     state: absent
 
 
-#################### network-ipam-4 ####################
+#################### multiple IPv4 networks ####################
+
+- block:
+  - name: Create network with two IPv4 IPAM configs
+    docker_network:
+      name: "{{ nname_ipam_3 }}"
+      driver: "macvlan"
+      driver_options:
+        parent: "{{ ansible_default_ipv4.alias }}"
+      ipam_config:
+        - subnet: 172.4.27.0/24
+        - subnet: 172.4.28.0/24
+    register: network
+
+  - assert:
+      that:
+        - network is changed
+
+  - name: Create network with two IPv4 IPAM configs (idempotence)
+    docker_network:
+      name: "{{ nname_ipam_3 }}"
+      driver: "macvlan"
+      driver_options:
+        parent: "{{ ansible_default_ipv4.alias }}"
+      ipam_config:
+        - subnet: 172.4.28.0/24
+        - subnet: 172.4.27.0/24
+    register: network
+
+  - assert:
+      that:
+        - network is not changed
+
+  - name: Create network with two IPv4 IPAM configs (change)
+    docker_network:
+      name: "{{ nname_ipam_3 }}"
+      driver: "macvlan"
+      driver_options:
+        parent: "{{ ansible_default_ipv4.alias }}"
+      ipam_config:
+        - subnet: 172.4.27.0/24
+        - subnet: 172.4.29.0/24
+    register: network
+    diff: yes
+
+  - assert:
+      that:
+        - network is changed
+        - network.diff.differences | length == 1
+
+  - name: Create network with one IPv4 IPAM config (no change)
+    docker_network:
+      name: "{{ nname_ipam_3 }}"
+      driver: "macvlan"
+      driver_options:
+        parent: "{{ ansible_default_ipv4.alias }}"
+      ipam_config:
+        - subnet: 172.4.29.0/24
+    register: network
+
+  - assert:
+      that:
+        - network is not changed
+
+  - name: Cleanup network
+    docker_network:
+      name: "{{ nname_ipam_3 }}"
+      state: absent
+
+  when: ansible_facts.virtualization_type != 'docker'
+
+
+#################### IPAM driver options ####################
 
 - name: Create network with IPAM driver options
   docker_network:

test/units/modules/cloud/docker/test_docker_network.py

@@ -5,7 +5,7 @@
 
 import pytest
 
-from ansible.modules.cloud.docker.docker_network import get_ip_version
+from ansible.modules.cloud.docker.docker_network import validate_cidr
 
 
 @pytest.mark.parametrize("cidr,expected", [
@@ -15,8 +15,8 @@
     ('fdd1:ac8c:0557:7ce2::/64', 'ipv6'),
     ('fdd1:ac8c:0557:7ce2::/128', 'ipv6'),
 ])
-def test_get_ip_version_positives(cidr, expected):
-    assert get_ip_version(cidr) == expected
+def test_validate_cidr_positives(cidr, expected):
+    assert validate_cidr(cidr) == expected
 
 
 @pytest.mark.parametrize("cidr", [
@@ -25,7 +25,7 @@ def test_get_ip_version_positives(cidr, expected):
     '192.168.0.1/asd',
     'fdd1:ac8c:0557:7ce2::',
 ])
-def test_get_ip_version_negatives(cidr):
+def test_validate_cidr_negatives(cidr):
     with pytest.raises(ValueError) as e:
-        get_ip_version(cidr)
+        validate_cidr(cidr)
     assert '"{0}" is not a valid CIDR'.format(cidr) == str(e.value)