diff --git a/octavia/api/v2/controllers/listener.py b/octavia/api/v2/controllers/listener.py index 0bca3b2c6e..bda4c3eff5 100644 --- a/octavia/api/v2/controllers/listener.py +++ b/octavia/api/v2/controllers/listener.py @@ -156,14 +156,18 @@ class ListenersController(base.BaseController): value=headers, option=('%s protocol listener.' % listener_protocol)) - def _validate_cidr_compatible_with_vip(self, vip, allowed_cidrs): + def _validate_cidr_compatible_with_vip(self, vips, allowed_cidrs): for cidr in allowed_cidrs: - # Check if CIDR IP version matches VIP IP version - if common_utils.is_cidr_ipv6(cidr) != common_utils.is_ipv6(vip): - msg = _("CIDR %(cidr)s IP version incompatible with VIP " - "%(vip)s IP version.") + for vip in vips: + # Check if CIDR IP version matches VIP IP version + if (common_utils.is_cidr_ipv6(cidr) == + common_utils.is_ipv6(vip)): + break + else: + msg = _("CIDR %(cidr)s IP version incompatible with all VIPs " + "%(vips)s IP version.") raise exceptions.ValidationException( - detail=msg % {'cidr': cidr, 'vip': vip}) + detail=msg % {'cidr': cidr, 'vips': vips}) def _validate_create_listener(self, lock_session, listener_dict): """Validate listener for wrong protocol or duplicate listeners @@ -306,10 +310,11 @@ class ListenersController(base.BaseController): # Validate allowed CIDRs allowed_cidrs = listener_dict.get('allowed_cidrs', []) or [] lb_id = listener_dict.get('load_balancer_id') - vip_db = self.repositories.vip.get( - lock_session, load_balancer_id=lb_id) - vip_address = vip_db.ip_address - self._validate_cidr_compatible_with_vip(vip_address, allowed_cidrs) + lb_db = self.repositories.load_balancer.get( + lock_session, id=lb_id) + vip_addresses = [lb_db.vip.ip_address] + vip_addresses.extend([vip.ip_address for vip in lb_db.additional_vips]) + self._validate_cidr_compatible_with_vip(vip_addresses, allowed_cidrs) if _can_tls_offload: # Validate TLS version list @@ -525,9 +530,13 @@ class ListenersController(base.BaseController): # Validate allowed CIDRs if (listener.allowed_cidrs and listener.allowed_cidrs != wtypes.Unset): - vip_address = db_listener.load_balancer.vip.ip_address + vip_addresses = [db_listener.load_balancer.vip.ip_address] + vip_addresses.extend( + [vip.ip_address + for vip in db_listener.load_balancer.additional_vips] + ) self._validate_cidr_compatible_with_vip( - vip_address, listener.allowed_cidrs) + vip_addresses, listener.allowed_cidrs) # Check TLS cipher prohibit list if listener.tls_ciphers: diff --git a/octavia/network/drivers/neutron/allowed_address_pairs.py b/octavia/network/drivers/neutron/allowed_address_pairs.py index 61618b82be..fbaacd3c55 100644 --- a/octavia/network/drivers/neutron/allowed_address_pairs.py +++ b/octavia/network/drivers/neutron/allowed_address_pairs.py @@ -147,6 +147,10 @@ class AllowedAddressPairsDriver(neutron_base.BaseNeutronDriver): address = ipaddress.ip_address(ip) return 'IPv6' if address.version == 6 else 'IPv4' + def _get_ethertype_for_cidr(self, cidr): + net = ipaddress.ip_network(cidr) + return 'IPv6' if net.version == 6 else 'IPv4' + def _update_security_group_rules(self, load_balancer, sec_grp_id): rules = self.neutron_client.list_security_group_rules( security_group_id=sec_grp_id) @@ -224,11 +228,15 @@ class AllowedAddressPairsDriver(neutron_base.BaseNeutronDriver): ethertypes.add(self._get_ethertype_for_ip(add_vip.ip_address)) for port_protocol in add_ports: for ethertype in ethertypes: - self._create_security_group_rule(sec_grp_id, port_protocol[1], - port_min=port_protocol[0], - port_max=port_protocol[0], - ethertype=ethertype, - cidr=port_protocol[2]) + cidr = port_protocol[2] + if not cidr or self._get_ethertype_for_cidr(cidr) == ethertype: + self._create_security_group_rule( + sec_grp_id, port_protocol[1], + port_min=port_protocol[0], + port_max=port_protocol[0], + ethertype=ethertype, + cidr=cidr, + ) # Currently we are using the VIP network for VRRP # so we need to open up the protocols for it diff --git a/octavia/tests/functional/api/v2/test_listener.py b/octavia/tests/functional/api/v2/test_listener.py index 1a18715be3..434210d640 100644 --- a/octavia/tests/functional/api/v2/test_listener.py +++ b/octavia/tests/functional/api/v2/test_listener.py @@ -1244,9 +1244,9 @@ class TestListener(base.BaseAPITest): "It must be a valid x509 PEM format certificate.", response['faultstring']) - def _test_create_with_allowed_cidrs(self, allowed_cidrs): + def _test_create_with_allowed_cidrs(self, allowed_cidrs, lb_id): listener = self.create_listener(constants.PROTOCOL_TCP, - 80, self.lb_id, + 80, lb_id, allowed_cidrs=allowed_cidrs) listener_path = self.LISTENER_PATH.format( listener_id=listener['listener']['id']) @@ -1255,14 +1255,17 @@ class TestListener(base.BaseAPITest): def test_create_with_allowed_cidrs_ipv4(self): allowed_cidrs = ['10.0.1.0/24', '172.16.55.0/25'] - self._test_create_with_allowed_cidrs(allowed_cidrs) + self._test_create_with_allowed_cidrs(allowed_cidrs, self.lb_id) def test_create_with_allowed_cidrs_ipv6(self): + lb_ipv6 = self.create_load_balancer( + uuidutils.generate_uuid(), + vip_address='2001:db9:a1b:13f0::1', + ) + lb_id = lb_ipv6.get('loadbalancer').get('id') + self.set_lb_status(lb_id) allowed_cidrs = ['2001:db8:a0b:12f0::/64', '2a02:8071:69e::/64'] - with mock.patch('octavia.db.repositories.VipRepository.' - 'get') as repo_mock: - repo_mock.return_value.ip_address = "2001:db9:a1b:13f0::1" - self._test_create_with_allowed_cidrs(allowed_cidrs) + self._test_create_with_allowed_cidrs(allowed_cidrs, lb_id) def test_create_with_bad_allowed_cidrs(self): allowed_cidrs = [u'10.0.1.0/33', u'172.16.55.1.0/25'] @@ -1288,24 +1291,42 @@ class TestListener(base.BaseAPITest): body = self._build_body(lb_listener) response = self.post(self.LISTENERS_PATH, body, status=400).json self.assertIn("Validation failure: CIDR 2001:db8:a0b:12f0::/64 IP " - "version incompatible with VIP 198.0.2.5 IP version.", + "version incompatible with all VIPs ['198.0.2.5'] IP " + "version.", response['faultstring']) def test_create_with_incompatible_allowed_cidrs_ipv4(self): + lb_ipv6 = self.create_load_balancer( + uuidutils.generate_uuid(), + vip_address='2001:db9:a1b:13f0::1', + ) + lb_id = lb_ipv6.get('loadbalancer').get('id') + self.set_lb_status(lb_id) lb_listener = { 'protocol': constants.PROTOCOL_TCP, 'protocol_port': 80, 'project_id': self.project_id, - 'loadbalancer_id': self.lb_id, + 'loadbalancer_id': lb_id, 'allowed_cidrs': ['10.0.1.0/24']} - with mock.patch('octavia.db.repositories.VipRepository.' - 'get') as repo_mock: - repo_mock.return_value.ip_address = "2001:db9:a1b:13f0::1" - body = self._build_body(lb_listener) - response = self.post(self.LISTENERS_PATH, body, status=400).json - self.assertIn("Validation failure: CIDR 10.0.1.0/24 IP version " - "incompatible with VIP 2001:db9:a1b:13f0::1 IP " - "version.", response['faultstring']) + body = self._build_body(lb_listener) + response = self.post(self.LISTENERS_PATH, body, status=400).json + self.assertIn("Validation failure: CIDR 10.0.1.0/24 IP version " + "incompatible with all VIPs " + "['2001:db9:a1b:13f0::1'] IP version.", + response['faultstring']) + + def test_create_with_mixed_version_allowed_cidrs(self): + lb_dualstack = self.create_load_balancer( + uuidutils.generate_uuid(), + additional_vips=[{'subnet_id': uuidutils.generate_uuid(), + 'ip_address': '2001:db9:a1b:13f0::1', + }], + ) + lb_id = lb_dualstack.get('loadbalancer').get('id') + self.set_lb_status(lb_id) + self._test_create_with_allowed_cidrs(['10.0.1.0/24', + '2001:db9:a1b:13f0::/64'], + lb_id) def test_create_with_duplicated_allowed_cidrs(self): allowed_cidrs = ['10.0.1.0/24', '10.0.2.0/24', '10.0.2.0/24'] diff --git a/octavia/tests/unit/network/drivers/neutron/test_allowed_address_pairs.py b/octavia/tests/unit/network/drivers/neutron/test_allowed_address_pairs.py index 49994a99ac..c5dc513f3a 100644 --- a/octavia/tests/unit/network/drivers/neutron/test_allowed_address_pairs.py +++ b/octavia/tests/unit/network/drivers/neutron/test_allowed_address_pairs.py @@ -1099,12 +1099,13 @@ class TestAllowedAddressPairsDriver(base.TestCase): lc_1 = data_models.ListenerCidr('l1', '10.0.101.0/24') lc_2 = data_models.ListenerCidr('l2', '10.0.102.0/24') lc_3 = data_models.ListenerCidr('l2', '10.0.103.0/24') + lc_4 = data_models.ListenerCidr('l2', '2001:0DB8::/32') listeners = [data_models.Listener(protocol_port=80, peer_port=1024, protocol=constants.PROTOCOL_TCP, allowed_cidrs=[lc_1]), data_models.Listener(protocol_port=443, peer_port=1025, protocol=constants.PROTOCOL_TCP, - allowed_cidrs=[lc_2, lc_3]), + allowed_cidrs=[lc_2, lc_3, lc_4]), data_models.Listener(protocol_port=50, peer_port=1026, protocol=constants.PROTOCOL_UDP)] vip = data_models.Vip(ip_address='10.0.0.2') @@ -1183,7 +1184,18 @@ class TestAllowedAddressPairsDriver(base.TestCase): 'remote_ip_prefix': '10.0.103.0/24' } } - expected_create_rule_udp = { + expected_create_rule_5 = { + 'security_group_rule': { + 'security_group_id': 'secgrp-1', + 'direction': 'ingress', + 'protocol': 'tcp', + 'port_range_min': 443, + 'port_range_max': 443, + 'ethertype': 'IPv6', + 'remote_ip_prefix': '2001:0DB8::/32' + } + } + expected_create_rule_udp_1 = { 'security_group_rule': { 'security_group_id': 'secgrp-1', 'direction': 'ingress', @@ -1194,13 +1206,26 @@ class TestAllowedAddressPairsDriver(base.TestCase): 'remote_ip_prefix': None } } + expected_create_rule_udp_2 = { + 'security_group_rule': { + 'security_group_id': 'secgrp-1', + 'direction': 'ingress', + 'protocol': 'udp', + 'port_range_min': 50, + 'port_range_max': 50, + 'ethertype': 'IPv6', + 'remote_ip_prefix': None + } + } create_rule.assert_has_calls([mock.call(expected_create_rule_1), mock.call(expected_create_rule_udp_peer), mock.call(expected_create_rule_2), mock.call(expected_create_rule_3), mock.call(expected_create_rule_4), - mock.call(expected_create_rule_udp)], + mock.call(expected_create_rule_5), + mock.call(expected_create_rule_udp_1), + mock.call(expected_create_rule_udp_2)], any_order=True) def test_update_vip_when_protocol_and_peer_ports_overlap(self): diff --git a/releasenotes/notes/allowed_cidr-validation-for-additional_vips-175c32824cc7ee95.yaml b/releasenotes/notes/allowed_cidr-validation-for-additional_vips-175c32824cc7ee95.yaml new file mode 100644 index 0000000000..456d9d1806 --- /dev/null +++ b/releasenotes/notes/allowed_cidr-validation-for-additional_vips-175c32824cc7ee95.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + The validation for the allowed_cidr parameter only took into account the + IP version of the primary VIP. CIDRs which only matched the version of an + additonal VIP were rejected. This if fixed and CIDRs are now matched + against the IP version of all VIPs.