diff --git a/changelogs/fragments/65839-docker_network-idempotence.yml b/changelogs/fragments/65839-docker_network-idempotence.yml new file mode 100644 index 00000000000000..5dd200520bd804 --- /dev/null +++ b/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." diff --git a/lib/ansible/modules/cloud/docker/docker_network.py b/lib/ansible/modules/cloud/docker/docker_network.py index ee12e19ffea399..eea827b36db79c 100644 --- a/lib/ansible/modules/cloud/docker/docker_network.py +++ b/lib/ansible/modules/cloud/docker/docker_network.py @@ -335,8 +335,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 @@ -352,7 +352,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 @@ -365,6 +365,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): @@ -388,6 +398,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) @@ -449,30 +466,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', diff --git a/test/integration/targets/docker_network/tasks/tests/ipam.yml b/test/integration/targets/docker_network/tasks/tests/ipam.yml index cd448fac42013f..7405c3b1949ea0 100644 --- a/test/integration/targets/docker_network/tasks/tests/ipam.yml +++ b/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: diff --git a/test/units/modules/cloud/docker/test_docker_network.py b/test/units/modules/cloud/docker/test_docker_network.py index 4d67565ea6a67d..5306506c1110c7 100644 --- a/test/units/modules/cloud/docker/test_docker_network.py +++ b/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)