allowed_cidr validation for additional_vips

gaudenz · gaudenz · commit 60f579b64a8f · 2023-03-02T10:09:32.000+01:00
The validation for the allowed_cidr parameter did not take into account
the IP version of additional vIPs. The parameter was rejected if the IP
version did not match the IP version of the primary vIP. As additional
vIPs can have a different IP version from the primary vIP all vIPs must
be checked during validation.

Change-Id: I3706fd5e12e17be37edce974563c6806d4f09709
diff --git a/octavia/api/v2/controllers/listener.py b/octavia/api/v2/controllers/listener.py
@@ -156,14 +156,18 @@ def _validate_insert_headers(self, insert_header_list, listener_protocol):
                     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 @@ def _validate_create_listener(self, lock_session, listener_dict):
         # 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 @@ def _validate_listener_PUT(self, listener, db_listener):
 
         # 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
@@ -147,6 +147,10 @@ def _get_ethertype_for_ip(self, ip):
         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 @@ def _update_security_group_rules(self, load_balancer, sec_grp_id):
             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
@@ -1207,9 +1207,9 @@ def test_create_with_bad_ca_cert(self):
                       "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 @@ def _test_create_with_allowed_cidrs(self, allowed_cidrs):
 
     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']
@@ -1251,24 +1254,42 @@ def test_create_with_incompatible_allowed_cidrs_ipv6(self):
         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
@@ -1099,12 +1099,13 @@ def test_update_vip(self):
         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 @@ def test_update_vip(self):
                 '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 @@ def test_update_vip(self):
                 '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
@@ -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.
