Merge "allowed_cidr validation for additional_vips"

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
+            for vip in vips:
-            if common_utils.is_cidr_ipv6(cidr) != common_utils.is_ipv6(vip):
+                # Check if CIDR IP version matches VIP IP version
-                msg = _("CIDR %(cidr)s IP version incompatible with VIP "
+                if (common_utils.is_cidr_ipv6(cidr) ==
-                        "%(vip)s IP version.")
+                        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(
+        lb_db = self.repositories.load_balancer.get(
-            lock_session, load_balancer_id=lb_id)
+            lock_session, id=lb_id)
-        vip_address = vip_db.ip_address
+        vip_addresses = [lb_db.vip.ip_address]
-        self._validate_cidr_compatible_with_vip(vip_address, allowed_cidrs)
+        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:

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],
+                cidr = port_protocol[2]
-                                                 port_min=port_protocol[0],
+                if not cidr or self._get_ethertype_for_cidr(cidr) == ethertype:
-                                                 port_max=port_protocol[0],
+                    self._create_security_group_rule(
-                                                 ethertype=ethertype,
+                        sec_grp_id, port_protocol[1],
-                                                 cidr=port_protocol[2])
+                        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

octavia/tests/functional/api/v2/test_listener.py

@@ -1207,9 +1207,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'])
@@ -1218,14 +1218,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.'
+        self._test_create_with_allowed_cidrs(allowed_cidrs, lb_id)
-                        'get') as repo_mock:
-            repo_mock.return_value.ip_address = "2001:db9:a1b:13f0::1"
-            self._test_create_with_allowed_cidrs(allowed_cidrs)
 
     def test_create_with_bad_allowed_cidrs(self):
         allowed_cidrs = [u'10.0.1.0/33', u'172.16.55.1.0/25']
@@ -1251,24 +1254,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.'
+        body = self._build_body(lb_listener)
-                        'get') as repo_mock:
+        response = self.post(self.LISTENERS_PATH, body, status=400).json
-            repo_mock.return_value.ip_address = "2001:db9:a1b:13f0::1"
+        self.assertIn("Validation failure: CIDR 10.0.1.0/24 IP version "
-            body = self._build_body(lb_listener)
+                      "incompatible with all VIPs "
-            response = self.post(self.LISTENERS_PATH, body, status=400).json
+                      "['2001:db9:a1b:13f0::1'] IP version.",
-            self.assertIn("Validation failure: CIDR 10.0.1.0/24 IP version "
+                      response['faultstring'])
-                          "incompatible with VIP 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']

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):

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.