[OVN] Update the DHCP options when the metadata port is modified

Always update the DHCP options when the metadata port is created,
updated or deleted. If the metadata port IP addresses are updated,
the DHCP options register should be too, modifying the static routes
defined in "DHCP_Options.options.classless_static_route".

These static routes will be injected in the VM in the DHCP request.
The IP address of the metadata port should match with the static
route redirecting the traffic to the metadata IP address
"169.254.169.254/32":

  $ ip r
  default via 10.0.0.1 dev eth0
  10.0.0.0/28 dev eth0 scope link  src 10.0.0.7
  169.254.169.254 via 10.0.0.2 dev eth0  # 10.0.0.2 is the metadata
                                         # port IP address
Conflicts:
      neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py
      neutron/tests/unit/fake_resources.py

Closes-Bug: #1942794
Change-Id: Id5d4909caa521a899b97d83bdc1963b010e97dac
(cherry picked from commit bd0ded15ca)

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

@@ -547,7 +547,7 @@ class OvsdbNbOvnIdl(nb_impl_idl.OvnNbApiIdlImpl, Backend):
                 'external_ids': ext_ids, 'uuid': row.uuid}
 
     def get_subnet_dhcp_options(self, subnet_id, with_ports=False):
-        subnet = None
+        subnet = {}
         ports = []
         for row in self._tables['DHCP_Options'].rows.values():
             external_ids = getattr(row, 'external_ids', {})

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

@@ -496,6 +496,16 @@ class OVNClient(object):
             else:
                 dhcpv6_options = [port_info.dhcpv6_options['uuid']]
 
+            if self.is_metadata_port(port):
+                context = n_context.get_admin_context()
+                network = self._plugin.get_network(context, port['network_id'])
+                subnet_ids = set(_ip['subnet_id'] for _ip in port['fixed_ips'])
+                for subnet_id in subnet_ids:
+                    subnet = self._plugin.get_subnet(context, subnet_id)
+                    if not subnet['enable_dhcp']:
+                        continue
+                    self._update_subnet_dhcp_options(subnet, network, txn)
+
             # NOTE(mjozefcz): Do not set addresses if the port is not
             # bound, has no device_owner and it is OVN LB VIP port.
             # For more details check related bug #1789686.
@@ -1958,10 +1968,14 @@ class OVNClient(object):
 
     def create_subnet(self, context, subnet, network):
         if subnet['enable_dhcp']:
-            if subnet['ip_version'] == 4:
-                self.update_metadata_port(context, network['id'],
-                                          subnet_id=subnet['id'])
-            self._add_subnet_dhcp_options(subnet, network)
+            mport_updated = False
+            if subnet['ip_version'] == const.IP_VERSION_4:
+                mport_updated = self.update_metadata_port(
+                    context, network['id'], subnet=subnet)
+            if subnet['ip_version'] == const.IP_VERSION_6 or not mport_updated:
+                # NOTE(ralonsoh): if IPv4 but the metadata port has not been
+                # updated, the DHPC options register has not been created.
+                self._add_subnet_dhcp_options(subnet, network)
         db_rev.bump_revision(context, subnet, ovn_const.TYPE_SUBNETS)
 
     def _modify_subnet_dhcp_options(self, subnet, ovn_subnet, network, txn):
@@ -1977,8 +1991,7 @@ class OVNClient(object):
             subnet['id'])['subnet']
 
         if subnet['enable_dhcp'] or ovn_subnet:
-            self.update_metadata_port(context, network['id'],
-                                      subnet_id=subnet['id'])
+            self.update_metadata_port(context, network['id'], subnet=subnet)
 
         check_rev_cmd = self._nb_idl.check_revision_number(
             subnet['id'], subnet, ovn_const.TYPE_SUBNETS)
@@ -2043,6 +2056,15 @@ class OVNClient(object):
         db_rev.delete_revision(
             context, rule['id'], ovn_const.TYPE_SECURITY_GROUP_RULES)
 
+    @staticmethod
+    def is_metadata_port(port):
+        # TODO(ralonsoh): This method is implemented in order to be backported
+        # to stable releases; this is why a "const.DEVICE_OWNER_DHCP" port
+        # could be a metadata port.
+        return (port['device_owner'] == const.DEVICE_OWNER_DISTRIBUTED or
+                (port['device_owner'] == const.DEVICE_OWNER_DHCP and
+                 not utils.is_neutron_dhcp_agent_port(port)))
+
     def _find_metadata_port(self, context, network_id):
         if not ovn_conf.is_ovn_metadata_enabled():
             return
@@ -2086,11 +2108,13 @@ class OVNClient(object):
                 # TODO(boden): rehome create_port into neutron-lib
                 p_utils.create_port(self._plugin, context, port)
 
-    def update_metadata_port(self, context, network_id, subnet_id=None):
+    def update_metadata_port(self, context, network_id, subnet=None):
         """Update metadata port.
 
         This function will allocate an IP address for the metadata port of
-        the given network in all its IPv4 subnets or the given subnet.
+        the given network in all its IPv4 subnets or the given subnet. Returns
+        "True" if the metadata port has been updated and "False" if OVN
+        metadata is disabled or the metadata port does not exist.
         """
         def update_metadata_port_fixed_ips(metadata_port, add_subnet_ids,
                                            del_subnet_ids):
@@ -2108,14 +2132,14 @@ class OVNClient(object):
                                      metadata_port['id'], port)
 
         if not ovn_conf.is_ovn_metadata_enabled():
-            return
+            return False
 
         # Retrieve the metadata port of this network
         metadata_port = self._find_metadata_port(context, network_id)
         if not metadata_port:
             LOG.error("Metadata port couldn't be found for network %s",
                       network_id)
-            return
+            return False
 
         port_subnet_ids = set(ip['subnet_id'] for ip in
                               metadata_port['fixed_ips'])
@@ -2123,10 +2147,14 @@ class OVNClient(object):
         # If this method is called from "create_subnet" or "update_subnet",
         # only the fixed IP address from this subnet should be updated in the
         # metadata port.
-        if subnet_id:
-            if subnet_id not in port_subnet_ids:
-                update_metadata_port_fixed_ips(metadata_port, [subnet_id], [])
-            return
+        if subnet and subnet['id']:
+            if subnet['enable_dhcp'] and subnet['id'] not in port_subnet_ids:
+                update_metadata_port_fixed_ips(metadata_port,
+                                               [subnet['id']], [])
+            elif not subnet['enable_dhcp'] and subnet['id'] in port_subnet_ids:
+                update_metadata_port_fixed_ips(metadata_port,
+                                               [], [subnet['id']])
+            return True
 
         # Retrieve all subnets in this network
         subnets = self._plugin.get_subnets(context, filters=dict(
@@ -2142,6 +2170,8 @@ class OVNClient(object):
                                            subnet_ids - port_subnet_ids,
                                            port_subnet_ids - subnet_ids)
 
+        return True
+
     def get_parent_port(self, port_id):
         return self._nb_idl.get_parent_port(port_id)
 

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

@@ -13,8 +13,11 @@
 #    under the License.
 
 import functools
+import re
 from unittest import mock
 
+import netaddr
+
 from neutron_lib.api.definitions import portbindings
 from neutron_lib import constants
 from neutron_lib.exceptions import agent as agent_exc
@@ -31,6 +34,7 @@ from neutron.conf.plugins.ml2.drivers.ovn import ovn_conf
 from neutron.db import ovn_revision_numbers_db as db_rev
 from neutron.plugins.ml2.drivers.ovn.mech_driver import mech_driver
 from neutron.plugins.ml2.drivers.ovn.mech_driver.ovsdb import impl_idl_ovn
+from neutron.plugins.ml2.drivers.ovn.mech_driver.ovsdb import ovn_client
 from neutron.tests import base as tests_base
 from neutron.tests.functional import base
 
@@ -746,6 +750,92 @@ class TestProvnetPorts(base.TestOVNFunctionalBase):
         self.assertIsNone(ovn_localnetport)
 
 
+class TestMetadataPorts(base.TestOVNFunctionalBase):
+
+    def setUp(self, *args, **kwargs):
+        super().setUp(*args, **kwargs)
+        self._ovn_client = self.mech_driver._ovn_client
+        self.meta_regex = re.compile(r'%s,(\d+\.\d+\.\d+\.\d+)' %
+                                     constants.METADATA_V4_CIDR)
+
+    def _create_network_ovn(self, metadata_enabled=True):
+        self.mock_is_ovn_metadata_enabled = mock.patch.object(
+            ovn_conf, 'is_ovn_metadata_enabled').start()
+        self.mock_is_ovn_metadata_enabled.return_value = metadata_enabled
+        self.n1 = self._make_network(self.fmt, 'n1', True)
+        self.n1_id = self.n1['network']['id']
+
+    def _create_subnet_ovn(self, cidr, enable_dhcp=True):
+        _cidr = netaddr.IPNetwork(cidr)
+        res = self._create_subnet(self.fmt, self.n1_id, cidr,
+                                  enable_dhcp=enable_dhcp,
+                                  ip_version=_cidr.version)
+        return self.deserialize(self.fmt, res)['subnet']
+
+    def _list_ports_ovn(self, net_id=None):
+        res = self._list_ports(self.fmt, net_id=net_id)
+        return self.deserialize(self.fmt, res)['ports']
+
+    def _check_metadata_port(self, net_id, fixed_ip):
+        for port in self._list_ports_ovn(net_id=net_id):
+            if ovn_client.OVNClient.is_metadata_port(port):
+                self.assertEqual(net_id, port['network_id'])
+                if fixed_ip:
+                    self.assertIn(fixed_ip, port['fixed_ips'])
+                else:
+                    self.assertEqual([], port['fixed_ips'])
+                return port['id']
+
+        self.fail('Metadata port is not present in network %s or data is not '
+                  'correct' % self.n1_id)
+
+    def _check_subnet_dhcp_options(self, subnet_id, cidr):
+        # This method checks the DHCP options CIDR and returns, if exits, the
+        # metadata port IP address, included in the classless static routes.
+        dhcp_opts = self._ovn_client._nb_idl.get_subnet_dhcp_options(subnet_id)
+        self.assertEqual(cidr, dhcp_opts['subnet']['cidr'])
+        routes = dhcp_opts['subnet']['options'].get('classless_static_route')
+        if not routes:
+            return
+
+        match = self.meta_regex.search(routes)
+        if match:
+            return match.group(1)
+
+    def test_subnet_ipv4(self):
+        self._create_network_ovn(metadata_enabled=True)
+        subnet = self._create_subnet_ovn('10.0.0.0/24')
+        metatada_ip = self._check_subnet_dhcp_options(subnet['id'],
+                                                      '10.0.0.0/24')
+        fixed_ip = {'subnet_id': subnet['id'], 'ip_address': metatada_ip}
+        port_id = self._check_metadata_port(self.n1_id, fixed_ip)
+
+        # Update metatada port IP address to 10.0.0.5
+        data = {'port': {'fixed_ips': [{'subnet_id': subnet['id'],
+                                        'ip_address': '10.0.0.5'}]}}
+        req = self.new_update_request('ports', data, port_id)
+        req.get_response(self.api)
+        metatada_ip = self._check_subnet_dhcp_options(subnet['id'],
+                                                      '10.0.0.0/24')
+        self.assertEqual('10.0.0.5', metatada_ip)
+        fixed_ip = {'subnet_id': subnet['id'], 'ip_address': metatada_ip}
+        self._check_metadata_port(self.n1_id, fixed_ip)
+
+    def test_subnet_ipv4_no_metadata(self):
+        self._create_network_ovn(metadata_enabled=False)
+        subnet = self._create_subnet_ovn('10.0.0.0/24')
+        self.assertIsNone(self._check_subnet_dhcp_options(subnet['id'],
+                                                          '10.0.0.0/24'))
+        self.assertEqual([], self._list_ports_ovn(self.n1_id))
+
+    def test_subnet_ipv6(self):
+        self._create_network_ovn(metadata_enabled=True)
+        subnet = self._create_subnet_ovn('2001:db8::/64')
+        self.assertIsNone(self._check_subnet_dhcp_options(subnet['id'],
+                                                          '2001:db8::/64'))
+        self._check_metadata_port(self.n1_id, [])
+
+
 class AgentWaitEvent(event.WaitEvent):
     """Wait for a list of Chassis to be created"""
 

neutron/tests/unit/fake_resources.py

@@ -152,6 +152,10 @@ class FakeOvsdbNbOvnIdl(object):
         self.qos_del_ext_ids = mock.Mock()
         self.meter_add = mock.Mock()
         self.meter_del = mock.Mock()
+        self.ha_chassis_group_add = mock.Mock()
+        self.ha_chassis_group_del = mock.Mock()
+        self.ha_chassis_group_add_chassis = mock.Mock()
+        self.ha_chassis_group_del_chassis = mock.Mock()
 
 
 class FakeOvsdbSbOvnIdl(object):

neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_impl_idl_ovn.py

@@ -693,10 +693,10 @@ class TestNBImplIdlOvn(TestDBImplIdlOvn):
             'ports': []}, subnet_options)
         subnet_options = self.nb_ovn_idl.get_subnet_dhcp_options(
             'subnet-id-11-0-2-0')['subnet']
-        self.assertIsNone(subnet_options)
+        self.assertEqual({}, subnet_options)
         subnet_options = self.nb_ovn_idl.get_subnet_dhcp_options(
             'port-id-30-0-1-0')['subnet']
-        self.assertIsNone(subnet_options)
+        self.assertEqual({}, subnet_options)
 
     def test_get_subnet_dhcp_options_with_ports(self):
         # Test empty

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

@@ -1593,10 +1593,9 @@ class TestOVNMechanismDriver(TestOVNMechanismDriverBase):
             umd.assert_not_called()
 
     def test_update_subnet_postcommit_enable_dhcp(self):
-        context = fakes.FakeSubnetContext(
-            subnet={'enable_dhcp': True, 'ip_version': 4, 'network_id': 'id',
-                    'id': 'subnet_id'},
-            network={'id': 'id'})
+        subnet = {'enable_dhcp': True, 'ip_version': 4, 'network_id': 'id',
+                  'id': 'subnet_id'}
+        context = fakes.FakeSubnetContext(subnet=subnet, network={'id': 'id'})
         with mock.patch.object(
                 self.mech_driver._ovn_client,
                 '_enable_subnet_dhcp_options') as esd,\
@@ -1606,15 +1605,14 @@ class TestOVNMechanismDriver(TestOVNMechanismDriverBase):
             self.mech_driver.update_subnet_postcommit(context)
             esd.assert_called_once_with(
                 context.current, context.network.current, mock.ANY)
-            umd.assert_called_once_with(mock.ANY, 'id', subnet_id='subnet_id')
+            umd.assert_called_once_with(mock.ANY, 'id', subnet=subnet)
 
     def test_update_subnet_postcommit_disable_dhcp(self):
         self.mech_driver._nb_ovn.get_subnet_dhcp_options.return_value = {
             'subnet': mock.sentinel.subnet, 'ports': []}
-        context = fakes.FakeSubnetContext(
-            subnet={'enable_dhcp': False, 'id': 'subnet_id', 'ip_version': 4,
-                    'network_id': 'id'},
-            network={'id': 'id'})
+        subnet = {'enable_dhcp': False, 'id': 'subnet_id', 'ip_version': 4,
+                  'network_id': 'id'}
+        context = fakes.FakeSubnetContext(subnet=subnet, network={'id': 'id'})
         with mock.patch.object(
                 self.mech_driver._ovn_client,
                 '_remove_subnet_dhcp_options') as dsd,\
@@ -1623,15 +1621,14 @@ class TestOVNMechanismDriver(TestOVNMechanismDriverBase):
                 'update_metadata_port') as umd:
             self.mech_driver.update_subnet_postcommit(context)
             dsd.assert_called_once_with(context.current['id'], mock.ANY)
-            umd.assert_called_once_with(mock.ANY, 'id', subnet_id='subnet_id')
+            umd.assert_called_once_with(mock.ANY, 'id', subnet=subnet)
 
     def test_update_subnet_postcommit_update_dhcp(self):
         self.mech_driver._nb_ovn.get_subnet_dhcp_options.return_value = {
             'subnet': mock.sentinel.subnet, 'ports': []}
-        context = fakes.FakeSubnetContext(
-            subnet={'enable_dhcp': True, 'ip_version': 4, 'network_id': 'id',
-                    'id': 'subnet_id'},
-            network={'id': 'id'})
+        subnet = {'enable_dhcp': True, 'id': 'subnet_id', 'ip_version': 4,
+                  'network_id': 'id'}
+        context = fakes.FakeSubnetContext(subnet=subnet, network={'id': 'id'})
         with mock.patch.object(
                 self.mech_driver._ovn_client,
                 '_update_subnet_dhcp_options') as usd,\
@@ -1641,43 +1638,65 @@ class TestOVNMechanismDriver(TestOVNMechanismDriverBase):
             self.mech_driver.update_subnet_postcommit(context)
             usd.assert_called_once_with(
                 context.current, context.network.current, mock.ANY)
-            umd.assert_called_once_with(mock.ANY, 'id', subnet_id='subnet_id')
+            umd.assert_called_once_with(mock.ANY, 'id', subnet=subnet)
 
-    def test_update_metadata_port_with_subnet_present_in_port(self):
+    def test_update_metadata_port_with_subnet(self):
         ovn_conf.cfg.CONF.set_override('ovn_metadata_enabled', True,
                                        group='ovn')
-        fixed_ips = [{'subnet_id': 'subnet1', 'ip_address': 'ip_add1'}]
+
         with mock.patch.object(
-                self.mech_driver._ovn_client, '_find_metadata_port',
-                return_value={'fixed_ips': fixed_ips, 'id': 'metadata_id'}), \
-                mock.patch.object(self.mech_driver._plugin, 'get_subnets',
-                                  return_value=[{'id': 'subnet1'},
-                                                {'id': 'subnet2'}]), \
+                self.mech_driver._ovn_client, '_find_metadata_port') as \
+                mock_metaport, \
+                mock.patch.object(self.mech_driver._plugin, 'get_subnets') as \
+                mock_get_subnets, \
                 mock.patch.object(self.mech_driver._plugin, 'update_port') as \
                 mock_update_port:
+            # Subnet with DHCP, present in port.
+            fixed_ips = [{'subnet_id': 'subnet1', 'ip_address': 'ip_add1'}]
+            mock_metaport.return_value = {'fixed_ips': fixed_ips,
+                                          'id': 'metadata_id'}
+            mock_get_subnets.return_value = [{'id': 'subnet1'}]
+            subnet = {'id': 'subnet1', 'enable_dhcp': True}
             self.mech_driver._ovn_client.update_metadata_port(
-                self.context, 'net_id', subnet_id='subnet1')
+                self.context, 'net_id', subnet=subnet)
             mock_update_port.assert_not_called()
 
-    def test_update_metadata_port_with_subnet_not_present_in_port(self):
-        ovn_conf.cfg.CONF.set_override('ovn_metadata_enabled', True,
-                                       group='ovn')
-        fixed_ips = [{'subnet_id': 'subnet1', 'ip_address': 'ip_add1'}]
-        with mock.patch.object(
-                self.mech_driver._ovn_client, '_find_metadata_port',
-                return_value={'fixed_ips': fixed_ips, 'id': 'metadata_id'}), \
-                mock.patch.object(self.mech_driver._plugin, 'get_subnets',
-                                  return_value=[{'id': 'subnet1'},
-                                                {'id': 'subnet2'}]), \
-                mock.patch.object(self.mech_driver._plugin, 'update_port') as \
-                mock_update_port:
+            # Subnet without DHCP, present in port.
+            fixed_ips = [{'subnet_id': 'subnet1', 'ip_address': 'ip_add1'}]
+            mock_metaport.return_value = {'fixed_ips': fixed_ips,
+                                          'id': 'metadata_id'}
+            mock_get_subnets.return_value = [{'id': 'subnet1'}]
+            subnet = {'id': 'subnet1', 'enable_dhcp': False}
             self.mech_driver._ovn_client.update_metadata_port(
-                self.context, 'net_id', subnet_id='subnet3')
-            fixed_ips.append({'subnet_id': 'subnet3'})
-            port = {'id': 'metadata_id', 'port': {
-                'network_id': 'net_id', 'fixed_ips': fixed_ips}}
-            mock_update_port.assert_called_once_with(
-                mock.ANY, 'metadata_id', port)
+                self.context, 'net_id', subnet=subnet)
+            port = {'id': 'metadata_id',
+                    'port': {'network_id': 'net_id', 'fixed_ips': []}}
+            mock_update_port.assert_called_once_with(mock.ANY, 'metadata_id',
+                                                     port)
+            mock_update_port.reset_mock()
+
+            # Subnet with DHCP, not present in port.
+            mock_metaport.return_value = {'fixed_ips': [],
+                                          'id': 'metadata_id'}
+            mock_get_subnets.return_value = []
+            subnet = {'id': 'subnet1', 'enable_dhcp': True}
+            self.mech_driver._ovn_client.update_metadata_port(
+                self.context, 'net_id', subnet=subnet)
+            fixed_ips = [{'subnet_id': 'subnet1'}]
+            port = {'id': 'metadata_id',
+                    'port': {'network_id': 'net_id', 'fixed_ips': fixed_ips}}
+            mock_update_port.assert_called_once_with(mock.ANY, 'metadata_id',
+                                                     port)
+            mock_update_port.reset_mock()
+
+            # Subnet without DHCP, not present in port.
+            mock_metaport.return_value = {'fixed_ips': [],
+                                          'id': 'metadata_id'}
+            mock_get_subnets.return_value = []
+            subnet = {'id': 'subnet1', 'enable_dhcp': False}
+            self.mech_driver._ovn_client.update_metadata_port(
+                self.context, 'net_id', subnet=subnet)
+            mock_update_port.assert_not_called()
 
     def test_update_metadata_port_no_subnet(self):
         ovn_conf.cfg.CONF.set_override('ovn_metadata_enabled', True,
@@ -2334,6 +2353,11 @@ class TestOVNMechanismDriverSegment(test_segment.HostSegmentMappingTestCase):
             lswitch_name=ovn_utils.ovn_name(net['id']))
 
     def _test_segments_helper(self):
+        self.mech_driver.nb_ovn.get_subnet_dhcp_options.return_value = {
+            'subnet': {'uuid': 'foo-uuid',
+                       'options': {'server_mac': 'ca:fe:ca:fe:ca:fe'},
+                       'cidr': '1.2.3.4/5'},
+            'ports': {}}
         ovn_conf.cfg.CONF.set_override('ovn_metadata_enabled', True,
                                        group='ovn')
 
@@ -3370,6 +3394,8 @@ class TestOVNMechanismDriverMetadataPort(test_plugin.Ml2PluginV2TestCase):
         Check that the metadata port is updated with a new IP address when a
         subnet is created.
         """
+        self.mech_driver.nb_ovn.get_subnet_dhcp_options.return_value = {
+            'subnet': {}, 'ports': {}}
         with self.network(set_context=True, tenant_id='test') as net1:
             with self.subnet(network=net1, cidr='10.0.0.0/24') as subnet1:
                 # Create a network:dhcp owner port just as how Neutron DHCP