[ML2/OVN] Validate allowed address pairs and distributed ports

In the ML2/OVN backend, if IP address of the unbound port is added to
the other port as `allowed_address_pair`, OVN treats this port as
`virtual`.
This could break connectivity to the metadata service as it uses
"special" port with device_owner set to `network:distributed` and this
port is `unbound`. So if someone would add IP address assigned to such
`network:distributed` port to the allowed_address_pair of the other
port, connectivity to the metadata will be broken.

This patch adds new validation of the allowed_address_pairs by the OVN
mech_driver. If IP address set as allowed_address_pair is used by the
`network:distributed` port, such API request will return BadRequest
error code and allowed_address_pair will not be set for the port.

Closes-Bug: #2116249

Depends-On: https://review.opendev.org/c/openstack/tempest/+/955569

Change-Id: I9b54e12fbd9b930a79660f2be195641107a5754e
Signed-off-by: Slawek Kaplonski <[email protected]>
(cherry picked from commit 79e9b02)

Author: slawqo

neutron/db/allowedaddresspairs_db.py

@@ -17,6 +17,8 @@
 from neutron_lib.api.definitions import allowedaddresspairs as addr_apidef
 from neutron_lib.api.definitions import port as port_def
 from neutron_lib.api import validators
+from neutron_lib.callbacks import events
+from neutron_lib.callbacks import registry
 from neutron_lib.db import api as db_api
 from neutron_lib.db import resource_extend
 from neutron_lib.db import utils as db_utils
@@ -36,6 +38,20 @@ def _process_create_allowed_address_pairs(self, context, port,
                                               allowed_address_pairs):
         if not validators.is_attr_set(allowed_address_pairs):
             return []
+
+        desired_state = {
+            'context': context,
+            'network_id': port['network_id'],
+            'allowed_address_pairs': allowed_address_pairs,
+        }
+        # TODO(slaweq): use constant from neutron_lib.callbacks.resources once
+        # it will be available and released
+        registry.publish(
+            'allowed_address_pair', events.BEFORE_CREATE, self,
+            payload=events.DBEventPayload(
+                context,
+                resource_id=port['id'],
+                desired_state=desired_state))
         try:
             with db_api.CONTEXT_WRITER.using(context):
                 for address_pair in allowed_address_pairs:

neutron/plugins/ml2/drivers/ovn/mech_driver/mech_driver.py

@@ -23,6 +23,7 @@
 import types
 import uuid
 
+import netaddr
 from neutron_lib.api.definitions import portbindings
 from neutron_lib.api.definitions import provider_net
 from neutron_lib.api.definitions import segment as segment_def
@@ -305,6 +306,11 @@ def subscribe(self):
         registry.subscribe(self.delete_segment_provnet_port,
                            resources.SEGMENT,
                            events.AFTER_DELETE)
+        # TODO(slaweq): use constant from neutron_lib.callbacks.resources once
+        # it will be available and released
+        registry.subscribe(self._validate_allowed_address_pairs,
+                           'allowed_address_pair',
+                           events.BEFORE_CREATE)
 
         # Handle security group/rule or address group notifications
         if self.sg_enabled:
@@ -623,6 +629,49 @@ def _validate_network_segments(self, network_segments):
                 )
                 raise n_exc.InvalidInput(error_message=m)
 
+    def _validate_allowed_address_pairs(self, resource, event, trigger,
+                                        payload):
+        context = payload.desired_state['context']
+        allowed_address_pairs = payload.desired_state['allowed_address_pairs']
+        network_id = payload.desired_state['network_id']
+        if not allowed_address_pairs:
+            return
+
+        port_allowed_address_pairs_ip_addresses = [
+            netaddr.IPNetwork(pair['ip_address'])
+            for pair in allowed_address_pairs]
+
+        distributed_ports = self._plugin.get_ports(
+            context.elevated(),
+            filters={'device_owner': [const.DEVICE_OWNER_DISTRIBUTED],
+                     'network_id': [network_id]})
+        if not distributed_ports:
+            return
+
+        def _get_common_ips(ip_addresses, ip_networks):
+            common_ips = set()
+            for ip_address in ip_addresses:
+                if any(ip_address in ip_net for ip_net in ip_networks):
+                    common_ips.add(str(ip_address))
+            return common_ips
+
+        for distributed_port in distributed_ports:
+            distributed_port_ip_addresses = [
+                netaddr.IPAddress(fixed_ip['ip_address']) for fixed_ip in
+                distributed_port.get('fixed_ips', [])]
+
+            common_ips = _get_common_ips(
+                distributed_port_ip_addresses,
+                port_allowed_address_pairs_ip_addresses)
+
+            if common_ips:
+                err_msg = (
+                    _("IP addresses '%s' already used by the '%s' port(s) in "
+                      "the same network" % (";".join(common_ips),
+                                            const.DEVICE_OWNER_DISTRIBUTED))
+                )
+                raise n_exc.InvalidInput(error_message=err_msg)
+
     def create_segment_provnet_port(self, resource, event, trigger,
                                     payload=None):
         segment = payload.latest_state

neutron/tests/common/test_db_base_plugin_v2.py

@@ -660,7 +660,7 @@ def _make_port(self, fmt, net_id, expected_res_status=None,
                    as_admin=False, **kwargs):
         res = self._create_port(fmt, net_id, expected_res_status,
                                 is_admin=as_admin, **kwargs)
-        self._check_http_response(res)
+        self._check_http_response(res, expected_res_status)
         return self.deserialize(fmt, res)
 
     def _make_security_group(self, fmt, name=None, expected_res_status=None,

neutron/tests/unit/db/test_l3_db.py

@@ -1081,6 +1081,11 @@ def test_update_router_gw_notify(self):
                           self.mixin, payload=mock.ANY),
                 mock.call(resources.PORT, events.BEFORE_CREATE,
                           mock.ANY, payload=mock.ANY),
+                # TODO(slaweq): use constant from
+                # neutron_lib.callbacks.resources once it will be available
+                # and released
+                mock.call('allowed_address_pair', events.BEFORE_CREATE,
+                          mock.ANY, payload=mock.ANY),
                 mock.call(resources.PORT, events.PRECOMMIT_CREATE,
                           mock.ANY, payload=mock.ANY),
                 mock.call(resources.PORT, events.AFTER_CREATE,

neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py

@@ -3183,6 +3183,47 @@ def test_node_uuid_worker_id(self, *args):
     def test_node_uuid_no_worker_id(self, *args):
         self.assertEqual(123456789, self.mech_driver.node_uuid)
 
+    def test_create_port_with_allowed_address_pairs(self):
+        with self.network() as network:
+            with self.subnet(network, cidr='10.0.0.0/24'):
+                self._make_port(
+                    self.fmt, network['network']['id'],
+                    device_owner=const.DEVICE_OWNER_DISTRIBUTED,
+                    fixed_ips=[{'ip_address': '10.0.0.2'}],
+                    as_admin=True,
+                    arg_list=('device_owner', 'fixed_ips'))
+                port1 = self._make_port(
+                    self.fmt, network['network']['id'],
+                    allowed_address_pairs=[{'ip_address': '10.0.0.3'}],
+                    as_admin=True,
+                    arg_list=('allowed_address_pairs',))['port']
+                self.assertEqual(
+                    [{'ip_address': '10.0.0.3',
+                      'mac_address': port1['mac_address']}],
+                    port1['allowed_address_pairs'])
+                self._make_port(
+                    self.fmt, network['network']['id'],
+                    allowed_address_pairs=[{'ip_address': '10.0.0.2'}],
+                    expected_res_status=exc.HTTPBadRequest.code,
+                    arg_list=('allowed_address_pairs',))
+                port2 = self._show('ports', port1['id'])['port']
+                self.assertEqual(
+                    [{'ip_address': '10.0.0.3',
+                      'mac_address': port2['mac_address']}],
+                    port2['allowed_address_pairs'])
+
+                # Now test the same but giving a subnet as allowed address pair
+                self._make_port(
+                    self.fmt, network['network']['id'],
+                    allowed_address_pairs=[{'ip_address': '10.0.0.2/26'}],
+                    expected_res_status=exc.HTTPBadRequest.code,
+                    arg_list=('allowed_address_pairs',))
+                port3 = self._show('ports', port1['id'])['port']
+                self.assertEqual(
+                    [{'ip_address': '10.0.0.3',
+                      'mac_address': port3['mac_address']}],
+                    port3['allowed_address_pairs'])
+
 
 class OVNMechanismDriverTestCase(MechDriverSetupBase,
                                  test_plugin.Ml2PluginV2TestCase):

releasenotes/notes/block-metadata-port-IP-address-to-be-used-as-virtual-ip-by-ovn-driver-0d46fed7652fea7a.yaml

@@ -0,0 +1,8 @@
+---
+fixes:
+  - |
+    When ML2/OVN backend is used, usage of the metadata port IP address as a
+    virtual IP address is blocked. That means that setting such IP address as
+    allowed_address_pair for other port is not allowed and API will return 400
+    error in such case. For more information, see bug
+    `2116249 <https://bugs.launchpad.net/neutron/+bug/2116249>`_.