From 00749e404db27c76858740a11e9d6ae1e477de25 Mon Sep 17 00:00:00 2001 From: Luis Tomas Bolivar Date: Fri, 10 Mar 2023 15:37:03 +0100 Subject: [PATCH] Ensure proper order during expose/withdraw When exposing IPs, there is a need to first wire it, i.e., ensure connectivity to/from OVN overlay, and then expose the IP through BGP. For withdrawn the process is the opposite, first withdraw the IP through BGP, and then remove the wiring Change-Id: I3d41380829e2aa5dc3109a37f8879246a718c8d7 --- .../drivers/openstack/ovn_bgp_driver.py | 282 +++++++++++------- .../drivers/openstack/test_ovn_bgp_driver.py | 16 +- .../tests/unit/utils/test_linux_net.py | 3 +- ovn_bgp_agent/utils/linux_net.py | 3 +- 4 files changed, 182 insertions(+), 122 deletions(-) diff --git a/ovn_bgp_agent/drivers/openstack/ovn_bgp_driver.py b/ovn_bgp_agent/drivers/openstack/ovn_bgp_driver.py index d7c8dfda..e791d969 100644 --- a/ovn_bgp_agent/drivers/openstack/ovn_bgp_driver.py +++ b/ovn_bgp_agent/drivers/openstack/ovn_bgp_driver.py @@ -310,14 +310,14 @@ class OVNBGPDriver(driver_api.AgentDriverBase): ip_dst = "{}/32".format(ip_address) ovn_ip_rules.pop(ip_dst, None) - def _expose_provider_port(self, port_ips, provider_datapath, - bridge_device=None, bridge_vlan=None, - lladdr=None): + def _bgp_announce_ips(self, port_ips): linux_net.add_ips_to_dev(CONF.bgp_nic, port_ips) - if not bridge_device and not bridge_vlan: - bridge_device, bridge_vlan = self._get_bridge_for_datapath( - provider_datapath) + def _bgp_withdraw_ips(self, port_ips): + linux_net.del_ips_from_dev(CONF.bgp_nic, port_ips) + + def _wire_provider_port(self, port_ips, bridge_device, bridge_vlan, + lladdr, proxy_cidrs): for ip in port_ips: try: if lladdr: @@ -333,11 +333,129 @@ class OVNBGPDriver(driver_api.AgentDriverBase): except agent_exc.InvalidPortIP: LOG.exception("Invalid IP to create a rule for port" " on the provider network: %s", ip) - return [] + return False linux_net.add_ip_route( self.ovn_routing_tables_routes, ip, self.ovn_routing_tables[bridge_device], bridge_device, vlan=bridge_vlan) + if proxy_cidrs: + # add proxy ndp config for ipv6 + for n_cidr in proxy_cidrs: + if linux_net.get_ip_version(n_cidr) == constants.IP_VERSION_6: + linux_net.add_ndp_proxy(n_cidr, bridge_device, bridge_vlan) + return True + + def _unwire_provider_port(self, port_ips, bridge_device, bridge_vlan, + lladdr, proxy_cidrs): + for ip in port_ips: + if lladdr: + if linux_net.get_ip_version(ip) == constants.IP_VERSION_6: + cr_lrp_ip = '{}/128'.format(ip) + else: + cr_lrp_ip = '{}/32'.format(ip) + try: + linux_net.del_ip_rule( + cr_lrp_ip, self.ovn_routing_tables[bridge_device], + bridge_device, lladdr=lladdr) + except agent_exc.InvalidPortIP: + LOG.exception("Invalid IP to delete a rule for the " + "provider port: %s", cr_lrp_ip) + return False + else: + try: + linux_net.del_ip_rule( + ip, self.ovn_routing_tables[bridge_device], + bridge_device) + except agent_exc.InvalidPortIP: + LOG.exception("Invalid IP to delete a rule for the " + "provider port: %s", ip) + return False + linux_net.del_ip_route( + self.ovn_routing_tables_routes, ip, + self.ovn_routing_tables[bridge_device], bridge_device, + vlan=bridge_vlan) + if proxy_cidrs: + for n_cidr in proxy_cidrs: + if linux_net.get_ip_version(n_cidr) == constants.IP_VERSION_6: + linux_net.del_ndp_proxy(n_cidr, bridge_device, bridge_vlan) + return True + + def _wire_lrp_port(self, ip, bridge_device, bridge_vlan, cr_lrp_ips): + LOG.debug("Adding IP Rules for network %s on chassis %s", ip, + self.chassis) + try: + linux_net.add_ip_rule( + ip, self.ovn_routing_tables[bridge_device]) + except agent_exc.InvalidPortIP: + LOG.exception("Invalid IP to create a rule for the " + "lrp (network router interface) port: %s", ip) + return False + LOG.debug("Added IP Rules for network %s on chassis %s", ip, + self.chassis) + + LOG.debug("Adding IP Routes for network %s on chassis %s", ip, + self.chassis) + # NOTE(ltomasbo): This assumes the provider network can only have + # (at most) 2 subnets, one for IPv4, one for IPv6 + ip_version = linux_net.get_ip_version(ip) + for cr_lrp_ip in cr_lrp_ips: + if linux_net.get_ip_version(cr_lrp_ip) == ip_version: + linux_net.add_ip_route( + self.ovn_routing_tables_routes, + ip.split("/")[0], + self.ovn_routing_tables[bridge_device], + bridge_device, + vlan=bridge_vlan, + mask=ip.split("/")[1], + via=cr_lrp_ip) + break + LOG.debug("Added IP Routes for network %s on chassis %s", ip, + self.chassis) + return True + + def _unwire_lrp_port(self, ip, bridge_device, bridge_vlan, cr_lrp_ips): + LOG.debug("Deleting IP Rules for network %s on chassis %s", ip, + self.chassis) + try: + linux_net.del_ip_rule(ip, self.ovn_routing_tables[bridge_device], + bridge_device) + except agent_exc.InvalidPortIP: + LOG.exception("Invalid IP to delete a rule for the " + "lrp (network router interface) port: %s", ip) + return False + + LOG.debug("Deleted IP Rules for network %s on chassis %s", ip, + self.chassis) + + LOG.debug("Deleting IP Routes for network %s on chassis %s", ip, + self.chassis) + ip_version = linux_net.get_ip_version(ip) + for cr_lrp_ip in cr_lrp_ips: + if linux_net.get_ip_version(cr_lrp_ip) == ip_version: + linux_net.del_ip_route( + self.ovn_routing_tables_routes, + ip.split("/")[0], + self.ovn_routing_tables[bridge_device], + bridge_device, + vlan=bridge_vlan, + mask=ip.split("/")[1], + via=cr_lrp_ip) + LOG.debug("Deleted IP Routes for network %s on chassis %s", ip, + self.chassis) + return True + + def _expose_provider_port(self, port_ips, provider_datapath, + bridge_device=None, bridge_vlan=None, + lladdr=None, proxy_cidrs=None): + if not bridge_device and not bridge_vlan: + bridge_device, bridge_vlan = self._get_bridge_for_datapath( + provider_datapath) + + # Connect to OVN + if self._wire_provider_port(port_ips, bridge_device, bridge_vlan, + lladdr, proxy_cidrs): + # Expose the IP now that it is connected + self._bgp_announce_ips(port_ips) def _expose_tenant_port(self, port, ip_version, exposed_ips=None, ovn_ip_rules=None): @@ -347,8 +465,7 @@ class OVNBGPDriver(driver_api.AgentDriverBase): constants.OVN_CIDRS_EXT_ID_KEY) if ext_n_cidr: ovn_lb_ip = ext_n_cidr.split(" ")[0].split("/")[0] - linux_net.add_ips_to_dev( - CONF.bgp_nic, [ovn_lb_ip]) + self._bgp_announce_ips([ovn_lb_ip]) if exposed_ips and ovn_lb_ip in exposed_ips: exposed_ips.remove(ovn_lb_ip) if ovn_ip_rules: @@ -378,8 +495,7 @@ class OVNBGPDriver(driver_api.AgentDriverBase): # IP version port_ip_version = linux_net.get_ip_version(port_ip) if port_ip_version == ip_version: - linux_net.add_ips_to_dev( - CONF.bgp_nic, [port_ip]) + self._bgp_announce_ips([port_ip]) if exposed_ips and port_ip in exposed_ips: exposed_ips.remove(port_ip) if ovn_ip_rules: @@ -391,29 +507,17 @@ class OVNBGPDriver(driver_api.AgentDriverBase): def _withdraw_provider_port(self, port_ips, provider_datapath, bridge_device=None, bridge_vlan=None, - lladdr=None): - linux_net.del_ips_from_dev(CONF.bgp_nic, port_ips) + lladdr=None, proxy_cidrs=None): + # Withdraw IP before disconnecting it + self._bgp_withdraw_ips(port_ips) + # Disconnect IP from OVN # assuming either you pass both or none if not bridge_device and not bridge_vlan: bridge_device, bridge_vlan = self._get_bridge_for_datapath( provider_datapath) - for ip in port_ips: - if lladdr: - if linux_net.get_ip_version(ip) == constants.IP_VERSION_6: - cr_lrp_ip = '{}/128'.format(ip) - else: - cr_lrp_ip = '{}/32'.format(ip) - linux_net.del_ip_rule( - cr_lrp_ip, self.ovn_routing_tables[bridge_device], - bridge_device, lladdr=lladdr) - else: - linux_net.del_ip_rule( - ip, self.ovn_routing_tables[bridge_device], bridge_device) - linux_net.del_ip_route( - self.ovn_routing_tables_routes, ip, - self.ovn_routing_tables[bridge_device], bridge_device, - vlan=bridge_vlan) + self._unwire_provider_port(port_ips, bridge_device, bridge_vlan, + lladdr, proxy_cidrs) def _get_bridge_for_datapath(self, datapath): network_name, network_tag = self.sb_idl.get_network_name_and_tag( @@ -530,21 +634,21 @@ class OVNBGPDriver(driver_api.AgentDriverBase): # VM on provider Network if provider_network: LOG.debug("Adding BGP route for logical port with ip %s", ips) - self._expose_provider_port(ips, row.datapath) - # NOTE: For Amphora Load Balancer with IPv6 VIP on the provider - # network, we need a NDP Proxy so that the traffic from the - # amphora can properly be redirected back if row.type == constants.OVN_VIRTUAL_VIF_PORT_TYPE: + # NOTE: For Amphora Load Balancer with IPv6 VIP on the + # provider network, we need a NDP Proxy so that the + # traffic from the amphora can properly be redirected back bridge_device, bridge_vlan = self._get_bridge_for_datapath( row.datapath) # NOTE: This is neutron specific as we need the provider # prefix to add the ndp proxy n_cidr = row.external_ids.get( constants.OVN_CIDRS_EXT_ID_KEY) - if n_cidr and (linux_net.get_ip_version(n_cidr) == - constants.IP_VERSION_6): - linux_net.add_ndp_proxy(n_cidr, bridge_device, - bridge_vlan) + self._expose_provider_port(ips, row.datapath, + bridge_device, bridge_vlan, + None, [n_cidr]) + else: + self._expose_provider_port(ips, row.datapath) LOG.debug("Added BGP route for logical port with ip %s", ips) return ips # VM with FIP @@ -641,7 +745,7 @@ class OVNBGPDriver(driver_api.AgentDriverBase): if provider_network: LOG.debug("Deleting BGP route for logical port with ip %s", ips) - self._withdraw_provider_port(ips, row.datapath) + n_cidr = None if row.type == constants.OVN_VIRTUAL_VIF_PORT_TYPE: virtual_provider_ports = ( self.sb_idl.get_virtual_ports_on_datapath_by_chassis( @@ -657,10 +761,12 @@ class OVNBGPDriver(driver_api.AgentDriverBase): # provider prefix to add the ndp proxy n_cidr = row.external_ids.get( constants.OVN_CIDRS_EXT_ID_KEY) - if n_cidr and (linux_net.get_ip_version(n_cidr) == - constants.IP_VERSION_6): - linux_net.del_ndp_proxy(n_cidr, bridge_device, - bridge_vlan) + if n_cidr: + self._withdraw_provider_port(ips, row.datapath, + bridge_device, bridge_vlan, + None, [n_cidr]) + else: + self._withdraw_provider_port(ips, row.datapath) LOG.debug("Deleted BGP route for logical port with ip %s", ips) # VM with FIP else: @@ -738,7 +844,7 @@ class OVNBGPDriver(driver_api.AgentDriverBase): if port_lrp in self.ovn_local_lrps.keys(): LOG.debug("Adding BGP route for tenant IP %s on chassis %s", ips_to_expose, self.chassis) - linux_net.add_ips_to_dev(CONF.bgp_nic, ips_to_expose) + self._bgp_announce_ips(ips_to_expose) LOG.debug("Added BGP route for tenant IP %s on chassis %s", ips_to_expose, self.chassis) break @@ -781,7 +887,7 @@ class OVNBGPDriver(driver_api.AgentDriverBase): if port_lrp in self.ovn_local_lrps.keys(): LOG.debug("Deleting BGP route for tenant IP %s on chassis %s", ips_to_withdraw, self.chassis) - linux_net.del_ips_from_dev(CONF.bgp_nic, ips_to_withdraw) + self._bgp_withdraw_ips(ips_to_withdraw) LOG.debug("Deleted BGP route for tenant IP %s on chassis %s", ips_to_withdraw, self.chassis) break @@ -821,11 +927,7 @@ class OVNBGPDriver(driver_api.AgentDriverBase): ips_without_mask = [ip.split("/")[0] for ip in ips] self._expose_provider_port(ips_without_mask, provider_datapath, bridge_device, bridge_vlan, - lladdr=mac) - # add proxy ndp config for ipv6 - for ip in ips: - if linux_net.get_ip_version(ip) == constants.IP_VERSION_6: - linux_net.add_ndp_proxy(ip, bridge_device, bridge_vlan) + lladdr=mac, proxy_cidrs=ips) LOG.debug("Added BGP route for CR-LRP Port %s", ips) # Check if there are networks attached to the router, @@ -851,11 +953,8 @@ class OVNBGPDriver(driver_api.AgentDriverBase): # Removing information about the associated network for # tenant network advertisement ips_without_mask = [ip.split("/")[0] for ip in ips] - self._withdraw_provider_port(ips_without_mask, provider_datapath, - bridge_device=bridge_device, - bridge_vlan=bridge_vlan, - lladdr=mac) # del proxy ndp config for ipv6 + proxy_cidrs = [] for ip in ips_without_mask: if linux_net.get_ip_version(ip) == constants.IP_VERSION_6: cr_lrps_on_same_provider = [ @@ -864,7 +963,13 @@ class OVNBGPDriver(driver_api.AgentDriverBase): # if no other cr-lrp port on the same provider # delete the ndp proxy if (len(cr_lrps_on_same_provider) <= 1): - linux_net.del_ndp_proxy(ip, bridge_device, bridge_vlan) + proxy_cidrs.append(ip) + + self._withdraw_provider_port(ips_without_mask, provider_datapath, + bridge_device=bridge_device, + bridge_vlan=bridge_vlan, + lladdr=mac, proxy_cidrs=proxy_cidrs) + LOG.debug("Deleted BGP route for CR-LRP Port %s", ips) # Check if there are networks attached to the router, @@ -915,42 +1020,16 @@ class OVNBGPDriver(driver_api.AgentDriverBase): cr_lrp_info['subnets_cidr'].append(ip) self.ovn_local_lrps.update({lrp: associated_cr_lrp}) - LOG.debug("Adding IP Rules for network %s on chassis %s", ip, - self.chassis) - try: - linux_net.add_ip_rule( - ip, self.ovn_routing_tables[bridge_device]) - except agent_exc.InvalidPortIP: - LOG.exception("Invalid IP to create a rule for the " - "lrp (network router interface) port: %s", ip) + if not self._wire_lrp_port(ip, bridge_device, bridge_vlan, cr_lrp_ips): + LOG.warning("Not able to expose subnet with IP %s", ip) return - LOG.debug("Added IP Rules for network %s on chassis %s", ip, - self.chassis) if ovn_ip_rules: ovn_ip_rules.pop(ip, None) - LOG.debug("Adding IP Routes for network %s on chassis %s", ip, - self.chassis) - # NOTE(ltomasbo): This assumes the provider network can only have - # (at most) 2 subnets, one for IPv4, one for IPv6 - ip_version = linux_net.get_ip_version(ip) - for cr_lrp_ip in cr_lrp_ips: - if linux_net.get_ip_version(cr_lrp_ip) == ip_version: - linux_net.add_ip_route( - self.ovn_routing_tables_routes, - ip.split("/")[0], - self.ovn_routing_tables[bridge_device], - bridge_device, - vlan=bridge_vlan, - mask=ip.split("/")[1], - via=cr_lrp_ip) - break - LOG.debug("Added IP Routes for network %s on chassis %s", ip, - self.chassis) - # Check if there are VMs on the network # and if so expose the route ports = self.sb_idl.get_ports_on_datapath(subnet_datapath) + ip_version = linux_net.get_ip_version(ip) for port in ports: self._expose_tenant_port(port, ip_version=ip_version, exposed_ips=exposed_ips, @@ -965,8 +1044,6 @@ class OVNBGPDriver(driver_api.AgentDriverBase): return cr_lrp_info = self.ovn_local_cr_lrps.get(associated_cr_lrp, {}) - LOG.debug("Deleting IP Rules for network %s on chassis %s", ip, - self.chassis) exposed_lrp = False if lrp: if lrp in self.ovn_local_lrps.keys(): @@ -987,32 +1064,16 @@ class OVNBGPDriver(driver_api.AgentDriverBase): bridge_device = cr_lrp_info.get('bridge_device') bridge_vlan = cr_lrp_info.get('bridge_vlan') - linux_net.del_ip_rule(ip, self.ovn_routing_tables[bridge_device], - bridge_device) - LOG.debug("Deleted IP Rules for network %s on chassis %s", ip, - self.chassis) - - LOG.debug("Deleting IP Routes for network %s on chassis %s", ip, - self.chassis) ip_version = linux_net.get_ip_version(ip) for cr_lrp_ip in cr_lrp_ips: - if linux_net.get_ip_version(cr_lrp_ip) == ip_version: - linux_net.del_ip_route( - self.ovn_routing_tables_routes, - ip.split("/")[0], - self.ovn_routing_tables[bridge_device], - bridge_device, - vlan=bridge_vlan, - mask=ip.split("/")[1], - via=cr_lrp_ip) - if (linux_net.get_ip_version(cr_lrp_ip) == - constants.IP_VERSION_6): - net = ipaddress.IPv6Network(ip, strict=False) - else: - net = ipaddress.IPv4Network(ip, strict=False) - break - LOG.debug("Deleted IP Routes for network %s on chassis %s", ip, - self.chassis) + cr_lrp_ip_version = linux_net.get_ip_version(cr_lrp_ip) + if cr_lrp_ip_version != ip_version: + continue + if cr_lrp_ip_version == constants.IP_VERSION_6: + net = ipaddress.IPv6Network(ip, strict=False) + else: + net = ipaddress.IPv4Network(ip, strict=False) + break # Check if there are VMs on the network # and if so withdraw the routes @@ -1021,6 +1082,9 @@ class OVNBGPDriver(driver_api.AgentDriverBase): CONF.bgp_nic, net) linux_net.delete_exposed_ips(vms_on_net, CONF.bgp_nic) + # Disconnect the network to OVN + self._unwire_lrp_port(ip, bridge_device, bridge_vlan, cr_lrp_ips) + @lockutils.synchronized('bgp') def expose_subnet(self, ip, row): try: diff --git a/ovn_bgp_agent/tests/unit/drivers/openstack/test_ovn_bgp_driver.py b/ovn_bgp_agent/tests/unit/drivers/openstack/test_ovn_bgp_driver.py index 6a405d46..777eb7a0 100644 --- a/ovn_bgp_agent/tests/unit/drivers/openstack/test_ovn_bgp_driver.py +++ b/ovn_bgp_agent/tests/unit/drivers/openstack/test_ovn_bgp_driver.py @@ -300,8 +300,7 @@ class TestOVNBGPDriver(test_base.TestCase): mock_add_rule.side_effect = agent_exc.InvalidPortIP(ip=self.ipv4) self.bgp_driver._expose_provider_port(port_ips, provider_datapath) - mock_add_ips_dev.assert_called_once_with( - CONF.bgp_nic, [self.ipv4]) + mock_add_ips_dev.assert_not_called() mock_add_rule.assert_called_once_with( self.ipv4, 'fake-table') mock_add_route.assert_not_called() @@ -1305,6 +1304,7 @@ class TestOVNBGPDriver(test_base.TestCase): mock_ip_version.side_effect = (constants.IP_VERSION_4, constants.IP_VERSION_6, constants.IP_VERSION_4, + constants.IP_VERSION_6, constants.IP_VERSION_6) row = fakes.create_object({ 'name': 'fake-row', @@ -1531,9 +1531,8 @@ class TestOVNBGPDriver(test_base.TestCase): mock_del_ip_dev.assert_called_once_with(CONF.bgp_nic, [self.ipv6]) - @mock.patch.object(linux_net, 'add_ndp_proxy') @mock.patch.object(linux_net, 'get_ip_version') - def test__expose_cr_lrp_port(self, mock_ip_version, mock_ndp_proxy): + def test__expose_cr_lrp_port(self, mock_ip_version): mock_expose_provider_port = mock.patch.object( self.bgp_driver, '_expose_provider_port').start() mock_process_lrp_port = mock.patch.object( @@ -1557,15 +1556,13 @@ class TestOVNBGPDriver(test_base.TestCase): ips_without_mask = [ip.split("/")[0] for ip in ips] mock_expose_provider_port.assert_called_once_with( ips_without_mask, 'fake-provider-dp', self.bridge, None, - lladdr=self.mac) - mock_ndp_proxy.assert_called_once_with(self.ipv6, self.bridge, None) + lladdr=self.mac, proxy_cidrs=ips) mock_process_lrp_port.assert_called_once_with(dp_port0, self.cr_lrp0) mock_expose_ovn_lb.assert_called_once_with( 'fake-vip-ip', 'fake-vip-port', self.cr_lrp0) - @mock.patch.object(linux_net, 'del_ndp_proxy') @mock.patch.object(linux_net, 'get_ip_version') - def test__withdraw_cr_lrp_port(self, mock_ip_version, mock_ndp_proxy): + def test__withdraw_cr_lrp_port(self, mock_ip_version): mock_withdraw_provider_port = mock.patch.object( self.bgp_driver, '_withdraw_provider_port').start() mock_withdraw_lrp_port = mock.patch.object( @@ -1594,8 +1591,7 @@ class TestOVNBGPDriver(test_base.TestCase): ips_without_mask = [ip.split("/")[0] for ip in ips] mock_withdraw_provider_port.assert_called_once_with( ips_without_mask, 'fake-provider-dp', bridge_device=self.bridge, - bridge_vlan=10, lladdr=self.mac) - mock_ndp_proxy.assert_called_once_with(self.ipv6, self.bridge, 10) + bridge_vlan=10, lladdr=self.mac, proxy_cidrs=[self.ipv6]) mock_withdraw_lrp_port.assert_called_once_with('192.168.1.1/24', None, 'gateway_port') mock_withdraw_ovn_lb_on_provider.assert_called_once_with( diff --git a/ovn_bgp_agent/tests/unit/utils/test_linux_net.py b/ovn_bgp_agent/tests/unit/utils/test_linux_net.py index 9fa7aae0..a978e405 100644 --- a/ovn_bgp_agent/tests/unit/utils/test_linux_net.py +++ b/ovn_bgp_agent/tests/unit/utils/test_linux_net.py @@ -524,7 +524,8 @@ class TestLinuxNet(test_base.TestCase): rule = mock.MagicMock() self.fake_ndb.rules.__getitem__.return_value = rule - self.assertIsNone(linux_net.del_ip_rule('10.10.1.6/30/128', 7)) + self.assertRaises(agent_exc.InvalidPortIP, + linux_net.del_ip_rule, '10.10.1.6/30/128', 7) mock_rule_delete.assert_not_called() mock_del_ip_nei.assert_not_called() diff --git a/ovn_bgp_agent/utils/linux_net.py b/ovn_bgp_agent/utils/linux_net.py index 3a942d2a..dfe3e208 100644 --- a/ovn_bgp_agent/utils/linux_net.py +++ b/ovn_bgp_agent/utils/linux_net.py @@ -537,8 +537,7 @@ def del_ip_rule(ip, table, dev=None, lladdr=None): if ip_version == constants.IP_VERSION_6: rule['family'] = AF_INET6 else: - LOG.error("Invalid ip: {}".format(ip)) - return + raise agent_exc.InvalidPortIP(ip=ip) ovn_bgp_agent.privileged.linux_net.rule_delete(rule)