docker_network: fix multiple subnet (of same IP version) idempotence (#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.
pull/66123/head
Felix Fontein 5 years ago committed by GitHub
parent 30cfa92e90
commit 17ef253ad1
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23

@ -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."

@ -347,8 +347,8 @@ CIDR_IPV4 = re.compile(r'^([0-9]{1,3}\.){3}[0-9]{1,3}/([0-9]|[1-2][0-9]|3[0-2])$
CIDR_IPV6 = re.compile(r'^[0-9a-fA-F:]+/([0-9]|[1-9][0-9]|1[0-2][0-9])$') 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): def validate_cidr(cidr):
"""Gets the IP version of a CIDR string """Validate CIDR. Return IP version of a CIDR string on success.
:param cidr: Valid CIDR :param cidr: Valid CIDR
:type cidr: str :type cidr: str
@ -364,7 +364,7 @@ def get_ip_version(cidr):
def normalize_ipam_config_key(key): 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 :param key: Docker API key
:type key: str :type key: str
@ -377,6 +377,16 @@ def normalize_ipam_config_key(key):
return special_cases.get(key, key.lower()) 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): class DockerNetworkManager(object):
def __init__(self, client): def __init__(self, client):
@ -400,6 +410,13 @@ class DockerNetworkManager(object):
self.parameters.ipam_options['gateway'] or self.parameters.ipam_options['aux_addresses']): self.parameters.ipam_options['gateway'] or self.parameters.ipam_options['aux_addresses']):
self.parameters.ipam_config = [self.parameters.ipam_options] 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: if self.parameters.driver_options:
self.parameters.driver_options = clean_dict_booleans_for_docker_api(self.parameters.driver_options) self.parameters.driver_options = clean_dict_booleans_for_docker_api(self.parameters.driver_options)
@ -461,30 +478,29 @@ class DockerNetworkManager(object):
parameter=self.parameters.ipam_config, parameter=self.parameters.ipam_config,
active=net.get('IPAM', {}).get('Config')) active=net.get('IPAM', {}).get('Config'))
else: 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): for idx, ipam_config in enumerate(self.parameters.ipam_config):
net_config = dict() net_config = dict()
try: for net_ipam_config in net_ipam_configs:
ip_version = get_ip_version(ipam_config['subnet']) if dicts_are_essentially_equal(ipam_config, net_ipam_config):
for net_ipam_config in net['IPAM']['Config']: net_config = net_ipam_config
if ip_version == get_ip_version(net_ipam_config['Subnet']): break
net_config = net_ipam_config
except ValueError as e:
self.client.fail(str(e))
for key, value in ipam_config.items(): for key, value in ipam_config.items():
if value is None: if value is None:
# due to recursive argument_spec, all keys are always present # due to recursive argument_spec, all keys are always present
# (but have default value None if not specified) # (but have default value None if not specified)
continue continue
camelkey = None if value != net_config.get(key):
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:
differences.add('ipam_config[%s].%s' % (idx, key), differences.add('ipam_config[%s].%s' % (idx, key),
parameter=value, 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): if self.parameters.enable_ipv6 is not None and self.parameters.enable_ipv6 != net.get('EnableIPv6', False):
differences.add('enable_ipv6', differences.add('enable_ipv6',

@ -11,7 +11,7 @@
dnetworks: "{{ dnetworks + [nname_ipam_0, nname_ipam_1, nname_ipam_2, nname_ipam_3] }}" 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) - name: Create network with ipam_config and deprecated ipam_options (conflicting)
docker_network: docker_network:
@ -98,7 +98,7 @@
state: absent state: absent
#################### network-ipam-1 #################### #################### IPv4 IPAM config ####################
- name: Create network with custom IPAM config - name: Create network with custom IPAM config
docker_network: docker_network:
name: "{{ nname_ipam_1 }}" name: "{{ nname_ipam_1 }}"
@ -169,7 +169,7 @@
state: absent state: absent
#################### network-ipam-2 #################### #################### IPv6 IPAM config ####################
- name: Create network with IPv6 IPAM config - name: Create network with IPv6 IPAM config
docker_network: docker_network:
@ -230,7 +230,7 @@
state: absent state: absent
#################### network-ipam-3 #################### #################### IPv4 and IPv6 network ####################
- name: Create network with IPv6 and custom IPv4 IPAM config - name: Create network with IPv6 and custom IPv4 IPAM config
docker_network: docker_network:
@ -279,7 +279,79 @@
state: absent 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 - name: Create network with IPAM driver options
docker_network: docker_network:

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

Loading…
Cancel
Save