From a43eaae0e3e478d41831afc36a67ab82a52c95c7 Mon Sep 17 00:00:00 2001 From: Jakub Libosvar Date: Tue, 6 Aug 2024 20:26:51 +0000 Subject: [PATCH] Introduce LSP address column parsing functions There is a repeated code that can be replaced by function calls. Change-Id: Ia04e251a6014b10d0a3e1f75df71f1f4a00a1f8b Signed-off-by: Jakub Libosvar (cherry picked from commit e3a509cd14f8a715ccdcbc36e8af4749c6e1a5e6) --- .../drivers/openstack/nb_ovn_bgp_driver.py | 26 +++++-- ovn_bgp_agent/drivers/openstack/utils/port.py | 30 +++++++- .../openstack/watchers/nb_bgp_watcher.py | 32 ++++++--- ovn_bgp_agent/exceptions.py | 9 +++ .../unit/drivers/openstack/utils/test_port.py | 69 +++++++++++++++++++ 5 files changed, 149 insertions(+), 17 deletions(-) diff --git a/ovn_bgp_agent/drivers/openstack/nb_ovn_bgp_driver.py b/ovn_bgp_agent/drivers/openstack/nb_ovn_bgp_driver.py index 165a0603..c682d88e 100644 --- a/ovn_bgp_agent/drivers/openstack/nb_ovn_bgp_driver.py +++ b/ovn_bgp_agent/drivers/openstack/nb_ovn_bgp_driver.py @@ -26,6 +26,7 @@ from ovn_bgp_agent.drivers.openstack.utils import bgp as bgp_utils from ovn_bgp_agent.drivers.openstack.utils import driver_utils from ovn_bgp_agent.drivers.openstack.utils import ovn from ovn_bgp_agent.drivers.openstack.utils import ovs +from ovn_bgp_agent.drivers.openstack.utils import port as port_utils from ovn_bgp_agent.drivers.openstack.utils import wire as wire_utils from ovn_bgp_agent.drivers.openstack.watchers import nb_bgp_watcher as watcher from ovn_bgp_agent import exceptions @@ -244,8 +245,11 @@ class NBOVNBGPDriver(driver_api.AgentDriverBase): _, bridge_device, bridge_vlan = self._get_provider_ls_info( logical_switch) - ips = port.addresses[0].strip().split(' ')[1:] - mac = port.addresses[0].strip().split(' ')[0] + try: + ips = port_utils.get_ips_from_lsp(port) + except exceptions.IpAddressNotFound: + ips = [] + mac = port_utils.get_mac_from_lsp(port) self._expose_ip(ips, mac, logical_switch, bridge_device, bridge_vlan, port.type, port.external_ids.get( constants.OVN_CIDRS_EXT_ID_KEY, "").split()) @@ -777,13 +781,18 @@ class NBOVNBGPDriver(driver_api.AgentDriverBase): # Check if the ip's on this port match the address scope. As the # port can be dual-stack, it could be that v4 is not allowed, but # v6 is allowed, so then only v6 address should be exposed. - ips = self._ips_in_address_scope(port.addresses[0].split(' ')[1:], - subnet_info['address_scopes']) + try: + ips = self._ips_in_address_scope( + port_utils.get_ips_from_lsp(port), + subnet_info['address_scopes']) + except exceptions.IpAddressNotFound: + continue + if not ips: # All ip's have been removed due to address scope requirement continue - mac = port.addresses[0].strip().split(' ')[0] + mac = port_utils.get_mac_from_lsp(port) ips_info = { 'mac': mac, 'cidrs': port.external_ids.get(constants.OVN_CIDRS_EXT_ID_KEY, @@ -824,8 +833,11 @@ class NBOVNBGPDriver(driver_api.AgentDriverBase): ports = self.nb_idl.get_active_lsp(subnet_info['network']) for port in ports: - ips = port.addresses[0].split(' ')[1:] - mac = port.addresses[0].strip().split(' ')[0] + try: + ips = port_utils.get_ips_from_lsp(port) + except exceptions.IpAddressNotFound: + continue + mac = port_utils.get_mac_from_lsp(port) ips_info = { 'mac': mac, 'cidrs': port.external_ids.get(constants.OVN_CIDRS_EXT_ID_KEY, diff --git a/ovn_bgp_agent/drivers/openstack/utils/port.py b/ovn_bgp_agent/drivers/openstack/utils/port.py index 6b4f7bc1..b4485d1b 100644 --- a/ovn_bgp_agent/drivers/openstack/utils/port.py +++ b/ovn_bgp_agent/drivers/openstack/utils/port.py @@ -14,6 +14,7 @@ from ovn_bgp_agent import constants from ovn_bgp_agent.drivers.openstack.utils import common +from ovn_bgp_agent import exceptions def has_ip_address_defined(address): @@ -31,10 +32,37 @@ def has_additional_binding(row): constants.OVN_REQUESTED_CHASSIS, '') +def get_address_list(lsp): + try: + addrs = lsp.addresses[0].strip().split(' ') + # Check the first element for an empty string + if not addrs[0]: + return [] + except (AttributeError, IndexError): + return [] + + return addrs + + +def get_mac_from_lsp(lsp): + try: + return get_address_list(lsp)[0] + except IndexError: + raise exceptions.MacAddressNotFound(lsp=lsp) + + +def get_ips_from_lsp(lsp): + addresses = get_address_list(lsp)[1:] + if not addresses: + raise exceptions.IpAddressNotFound(lsp=lsp) + + return addresses + + def make_lsp_dict(row): # TODO(jlibosva): Stop passing around dynamic maps return { - 'mac': row.addresses[0].strip().split(' ')[0], + 'mac': get_mac_from_lsp(row), 'cidrs': row.external_ids.get(constants.OVN_CIDRS_EXT_ID_KEY, "").split(), 'type': row.type, diff --git a/ovn_bgp_agent/drivers/openstack/watchers/nb_bgp_watcher.py b/ovn_bgp_agent/drivers/openstack/watchers/nb_bgp_watcher.py index ae32a11f..cbb8e027 100644 --- a/ovn_bgp_agent/drivers/openstack/watchers/nb_bgp_watcher.py +++ b/ovn_bgp_agent/drivers/openstack/watchers/nb_bgp_watcher.py @@ -22,6 +22,7 @@ from ovn_bgp_agent.drivers.openstack.utils import loadbalancer as lb_utils from ovn_bgp_agent.drivers.openstack.utils import port as port_utils from ovn_bgp_agent.drivers.openstack.utils import router as router_utils from ovn_bgp_agent.drivers.openstack.watchers import base_watcher +from ovn_bgp_agent import exceptions LOG = logging.getLogger(__name__) @@ -74,7 +75,11 @@ class LogicalSwitchPortProviderCreateEvent(base_watcher.LSPChassisEvent): # At this point, the port is bound on this host, it is up and # the logical switch is exposable by the agent. # Only create the ips if not already exposed. - ips = row.addresses[0].split(' ')[1:] + try: + ips = port_utils.get_ips_from_lsp(row) + except exceptions.IpAddressNotFound: + return False + return not self.agent.is_ip_exposed(logical_switch, ips) except (IndexError, AttributeError): @@ -82,7 +87,7 @@ class LogicalSwitchPortProviderCreateEvent(base_watcher.LSPChassisEvent): def _run(self, event, row, old): with _SYNC_STATE_LOCK.read_lock(): - ips = row.addresses[0].split(' ')[1:] + ips = port_utils.get_ips_from_lsp(row) ips_info = port_utils.make_lsp_dict(row) self.agent.expose_ip(ips, ips_info) @@ -108,7 +113,11 @@ class LogicalSwitchPortProviderDeleteEvent(base_watcher.LSPChassisEvent): if not port_utils.has_ip_address_defined(row.addresses[0]): return False - ips = row.addresses[0].split(' ')[1:] + try: + ips = port_utils.get_ips_from_lsp(row) + except exceptions.IpAddressNotFound: + return False + logical_switch = common_utils.get_from_external_ids( row, constants.OVN_LS_NAME_EXT_ID_KEY) @@ -141,7 +150,7 @@ class LogicalSwitchPortProviderDeleteEvent(base_watcher.LSPChassisEvent): def _run(self, event, row, old): with _SYNC_STATE_LOCK.read_lock(): - ips = row.addresses[0].split(' ')[1:] + ips = port_utils.get_ips_from_lsp(row) ips_info = port_utils.make_lsp_dict(row) self.agent.withdraw_ip(ips, ips_info) @@ -591,7 +600,9 @@ class LogicalSwitchPortTenantCreateEvent(base_watcher.LSPChassisEvent): def match_fn(self, event, row, old): try: # single and dual-stack format - if not port_utils.has_ip_address_defined(row.addresses[0]): + try: + port_utils.get_ips_from_lsp(row) + except exceptions.IpAddressNotFound: return False if not bool(row.up[0]): @@ -620,8 +631,8 @@ class LogicalSwitchPortTenantCreateEvent(base_watcher.LSPChassisEvent): constants.OVN_VIRTUAL_VIF_PORT_TYPE]: return with _SYNC_STATE_LOCK.read_lock(): - ips = row.addresses[0].split(' ')[1:] - mac = row.addresses[0].strip().split(' ')[0] + ips = port_utils.get_ips_from_lsp(row) + mac = port_utils.get_mac_from_lsp(row) ips_info = { 'mac': mac, 'cidrs': row.external_ids.get(constants.OVN_CIDRS_EXT_ID_KEY, @@ -666,8 +677,11 @@ class LogicalSwitchPortTenantDeleteEvent(base_watcher.LSPChassisEvent): constants.OVN_VIRTUAL_VIF_PORT_TYPE]: return with _SYNC_STATE_LOCK.read_lock(): - ips = row.addresses[0].split(' ')[1:] - mac = row.addresses[0].strip().split(' ')[0] + # The address is present because of the + # has_ip_address_defined() check in match_fn(), therefore + # there is no need for the try-except block. + ips = port_utils.get_ips_from_lsp(row) + mac = port_utils.get_mac_from_lsp(row) ips_info = { 'mac': mac, 'cidrs': row.external_ids.get(constants.OVN_CIDRS_EXT_ID_KEY, diff --git a/ovn_bgp_agent/exceptions.py b/ovn_bgp_agent/exceptions.py index 3c95f9ea..97b7a94e 100644 --- a/ovn_bgp_agent/exceptions.py +++ b/ovn_bgp_agent/exceptions.py @@ -158,3 +158,12 @@ class InvalidArgument(RuntimeError): def __init__(self, message=None, device=None): message = message or self.message % {'device': device} super(InvalidArgument, self).__init__(message) + + +class MacAddressNotFound(OVNBGPAgentException): + message = _("Addresses column is empty or not set for port %(lsp)s") + + +class IpAddressNotFound(OVNBGPAgentException): + message = _("Addresses column has no IP addresses or is not set for port" + " %(lsp)s") diff --git a/ovn_bgp_agent/tests/unit/drivers/openstack/utils/test_port.py b/ovn_bgp_agent/tests/unit/drivers/openstack/utils/test_port.py index f9e3a347..2403dd57 100644 --- a/ovn_bgp_agent/tests/unit/drivers/openstack/utils/test_port.py +++ b/ovn_bgp_agent/tests/unit/drivers/openstack/utils/test_port.py @@ -15,6 +15,7 @@ from ovn_bgp_agent import constants from ovn_bgp_agent.drivers.openstack.utils import port +from ovn_bgp_agent import exceptions from ovn_bgp_agent.tests import base as test_base from ovn_bgp_agent.tests import utils as test_utils @@ -82,3 +83,71 @@ class TestHasAdditionalBinding(test_base.TestCase): options={constants.OVN_REQUESTED_CHASSIS: ""}) self.assertFalse(port.has_additional_binding(lsp)) + + +class TestGetAddressList(test_base.TestCase): + def test_get_list(self): + expected_list = ["mac", "ip1", "ip2"] + lsp = test_utils.create_row( + addresses=["mac ip1 ip2"]) + observed = port.get_address_list(lsp) + + self.assertListEqual(expected_list, observed) + + def test_get_list_strip(self): + expected_list = ["mac", "ip1", "ip2"] + lsp = test_utils.create_row( + addresses=[" mac ip1 ip2 "]) + observed = port.get_address_list(lsp) + + self.assertListEqual(expected_list, observed) + + def test_get_list_no_addresses(self): + lsp = test_utils.create_row() + observed = port.get_address_list(lsp) + + self.assertListEqual([], observed) + + def test_get_list_empty(self): + lsp = test_utils.create_row(addresses=[]) + observed = port.get_address_list(lsp) + + self.assertListEqual([], observed) + + def test_get_list_empty_string(self): + lsp = test_utils.create_row(addresses=[""]) + observed = port.get_address_list(lsp) + + self.assertListEqual([], observed) + + +class TestGetMacFromLsp(test_base.TestCase): + def test_get_mac(self): + mac = 'mac' + lsp = test_utils.create_row( + addresses=["%s ip1 ip2" % mac]) + observed_mac = port.get_mac_from_lsp(lsp) + + self.assertEqual(mac, observed_mac) + + def test_get_mac_empty_list(self): + lsp = test_utils.create_row( + addresses=[]) + self.assertRaises( + exceptions.MacAddressNotFound, port.get_mac_from_lsp, lsp) + + +class TestGetIpsFromLsp(test_base.TestCase): + def test_get_ips(self): + ips = ['ip1', 'ip2'] + lsp = test_utils.create_row( + addresses=["mac " + ' '.join(ips)]) + observed_ips = port.get_ips_from_lsp(lsp) + + self.assertEqual(ips, observed_ips) + + def test_get_ips_empty_list(self): + lsp = test_utils.create_row( + addresses=[]) + self.assertRaises( + exceptions.IpAddressNotFound, port.get_ips_from_lsp, lsp)