From 5e3062c17a9312fb0af8fda6c50ece2dafd8e56e Mon Sep 17 00:00:00 2001 From: Morten Brekkevold Date: Fri, 29 Nov 2024 10:08:35 +0000 Subject: [PATCH 1/5] Test handler selection for Cisco SMB switches Existing netbox mock fixtures needed to be updated with proper sysObjectID value mocking as well, since the selection logic for handlers will become more complex. --- tests/unittests/portadmin/conftest.py | 8 ++++++++ tests/unittests/portadmin/portadmin_test.py | 8 ++++++++ 2 files changed, 16 insertions(+) diff --git a/tests/unittests/portadmin/conftest.py b/tests/unittests/portadmin/conftest.py index 66102a03eb..4060e43d86 100644 --- a/tests/unittests/portadmin/conftest.py +++ b/tests/unittests/portadmin/conftest.py @@ -23,6 +23,7 @@ def netbox_hp(profile): netbox_type = Mock() netbox_type.vendor = vendor + netbox_type.sysobjectid = '1.3.6.1.4.1.11.2.3.7.11.45' netbox_type.get_enterprise_id.return_value = VENDOR_ID_HEWLETT_PACKARD netbox = Mock() @@ -40,6 +41,7 @@ def netbox_cisco(profile): netbox_type = Mock() netbox_type.vendor = vendor + netbox_type.sysobjectid = '1.3.6.1.4.1.9.1.278' netbox_type.get_enterprise_id.return_value = VENDOR_ID_CISCOSYSTEMS netbox = Mock() @@ -50,6 +52,12 @@ def netbox_cisco(profile): return netbox +@pytest.fixture +def netbox_cisco_smb(netbox_cisco): + netbox_cisco.type.sysobjectid = '1.3.6.1.4.1.9.6.1.1004.10.1' + return netbox_cisco + + @pytest.fixture def handler_hp(netbox_hp): return ManagementFactory.get_instance(netbox_hp) diff --git a/tests/unittests/portadmin/portadmin_test.py b/tests/unittests/portadmin/portadmin_test.py index d32a03ba9b..4c739cf303 100644 --- a/tests/unittests/portadmin/portadmin_test.py +++ b/tests/unittests/portadmin/portadmin_test.py @@ -2,6 +2,7 @@ from nav.oids import OID from nav.portadmin.management import ManagementFactory +from nav.portadmin.snmp.base import SNMPHandler from nav.portadmin.snmp.hp import HP from nav.portadmin.snmp.cisco import Cisco @@ -17,6 +18,13 @@ def test_get_cisco(self, netbox_cisco): assert handler is not None, "Could not get handler-object" assert isinstance(handler, Cisco), "Wrong handler-type" + def test_when_device_is_cisco_smb_switch_it_should_return_generic_snmp_handler( + self, netbox_cisco_smb + ): + handler = ManagementFactory.get_instance(netbox_cisco_smb) + assert handler is not None, "Could not get handler-object" + assert type(handler) is SNMPHandler, "Wrong handler-type" + class TestPortadminResponseHP: def test_get_vlan_hp(self, handler_hp): From c7b0ab69410f149a9306e7b9d8593eac304397ec Mon Sep 17 00:00:00 2001 From: Morten Brekkevold Date: Fri, 29 Nov 2024 10:10:02 +0000 Subject: [PATCH 2/5] Select generic SNMPHandler for Cisco SMB switches Cisco small business switches do not actually work like other Cisco products SNMP-wise, and we need to revert to the generic handler to make them work with PortAdmin --- python/nav/portadmin/management.py | 7 +++++++ python/nav/portadmin/snmp/cisco.py | 4 ++++ 2 files changed, 11 insertions(+) diff --git a/python/nav/portadmin/management.py b/python/nav/portadmin/management.py index 6018ac0742..66097c6505 100644 --- a/python/nav/portadmin/management.py +++ b/python/nav/portadmin/management.py @@ -14,6 +14,7 @@ # along with NAV. If not, see . # """This is a utility library made especially for PortAdmin.""" +from nav.enterprise.ids import VENDOR_ID_CISCOSYSTEMS from nav.errors import NoNetboxTypeError from nav.models import manage from nav.portadmin.handlers import ManagementHandler @@ -38,6 +39,12 @@ def get_instance(cls, netbox: manage.Netbox, **kwargs) -> ManagementHandler: raise NoNetboxTypeError() vendor_id = netbox.type.get_enterprise_id() + if ( + vendor_id == VENDOR_ID_CISCOSYSTEMS + and Cisco.OTHER_ENTERPRISES.is_a_prefix_of(netbox.type.sysobjectid) + ): + # Cisco SM and SMB products are not supported by the Cisco handler: + vendor_id = None handler = VENDOR_MAP.get(vendor_id, SNMPHandler) return handler(netbox, **kwargs) diff --git a/python/nav/portadmin/snmp/cisco.py b/python/nav/portadmin/snmp/cisco.py index 7d0b330e2a..1a10f6f85b 100644 --- a/python/nav/portadmin/snmp/cisco.py +++ b/python/nav/portadmin/snmp/cisco.py @@ -38,6 +38,10 @@ class Cisco(SNMPHandler): """A specialized class for handling ports in CISCO switches.""" + # Cisco sysObjectIDs under this tree are not normal Cisco products and should + # probably not be handled by this handler + OTHER_ENTERPRISES = OID('.1.3.6.1.4.1.9.6') + VENDOR = VENDOR_ID_CISCOSYSTEMS VTPNODES = get_mib('CISCO-VTP-MIB')['nodes'] From 483c9fb37c057b432edff0f1edda54d0f61f1859 Mon Sep 17 00:00:00 2001 From: Morten Brekkevold Date: Fri, 29 Nov 2024 10:32:14 +0000 Subject: [PATCH 3/5] Refactor management handler selection This pushes the logic of PortAdmin management handler selection to the handlers themselves, making it easier to implement more complex selection logic if needed. It seems more correct to let the handler classes decide for themselves whether a given Netbox is compatible. A `can_handle()` class method is added to `ManagementHandler`, whose default implementation is based on the existing "selection by `VENDOR` id" scheme. The extra logic for Cisco-but-not-quite-Cisco can therefore be pushed into a specific `can_handle()` implementation for the Cisco handler. --- python/nav/portadmin/handlers.py | 7 +++++++ python/nav/portadmin/management.py | 17 ++++++----------- python/nav/portadmin/snmp/base.py | 2 -- python/nav/portadmin/snmp/cisco.py | 9 +++++++++ 4 files changed, 22 insertions(+), 13 deletions(-) diff --git a/python/nav/portadmin/handlers.py b/python/nav/portadmin/handlers.py index f7f7fd2042..13d0c7f7b3 100644 --- a/python/nav/portadmin/handlers.py +++ b/python/nav/portadmin/handlers.py @@ -45,9 +45,16 @@ class ManagementHandler: a class. """ + VENDOR = None + def __init__(self, netbox: manage.Netbox, **kwargs): self.netbox = netbox + @classmethod + def can_handle(cls, netbox: manage.Netbox) -> bool: + """Returns True if this handler can handle the given netbox""" + return netbox.type and netbox.type.get_enterprise_id() == cls.VENDOR + def set_interface_description(self, interface: manage.Interface, description: str): """Configures a single interface's description, AKA the ifalias value""" raise NotImplementedError diff --git a/python/nav/portadmin/management.py b/python/nav/portadmin/management.py index 66097c6505..f78cbdade5 100644 --- a/python/nav/portadmin/management.py +++ b/python/nav/portadmin/management.py @@ -14,7 +14,6 @@ # along with NAV. If not, see . # """This is a utility library made especially for PortAdmin.""" -from nav.enterprise.ids import VENDOR_ID_CISCOSYSTEMS from nav.errors import NoNetboxTypeError from nav.models import manage from nav.portadmin.handlers import ManagementHandler @@ -25,7 +24,7 @@ from nav.portadmin.snmp.hp import HP from nav.portadmin.napalm.juniper import Juniper -VENDOR_MAP = {cls.VENDOR: cls for cls in (Cisco, Dell, H3C, HP, Juniper)} +SUPPORTED_HANDLERS = (Cisco, Dell, H3C, HP, Juniper, SNMPHandler) class ManagementFactory(object): @@ -38,15 +37,11 @@ def get_instance(cls, netbox: manage.Netbox, **kwargs) -> ManagementHandler: if not netbox.type: raise NoNetboxTypeError() - vendor_id = netbox.type.get_enterprise_id() - if ( - vendor_id == VENDOR_ID_CISCOSYSTEMS - and Cisco.OTHER_ENTERPRISES.is_a_prefix_of(netbox.type.sysobjectid) - ): - # Cisco SM and SMB products are not supported by the Cisco handler: - vendor_id = None - handler = VENDOR_MAP.get(vendor_id, SNMPHandler) - return handler(netbox, **kwargs) + for handler_class in SUPPORTED_HANDLERS: + if handler_class.can_handle(netbox): + break + + return handler_class(netbox, **kwargs) def __init__(self): pass diff --git a/python/nav/portadmin/snmp/base.py b/python/nav/portadmin/snmp/base.py index 70c65475fb..27dcee2979 100644 --- a/python/nav/portadmin/snmp/base.py +++ b/python/nav/portadmin/snmp/base.py @@ -88,8 +88,6 @@ class InvalidManagementProfileError(ManagementError): class SNMPHandler(ManagementHandler): """Implements PortAdmin management functions for SNMP-enabled switches""" - VENDOR = None - QBRIDGENODES = get_mib('Q-BRIDGE-MIB')['nodes'] SYSOBJECTID = '.1.3.6.1.2.1.1.2.0' diff --git a/python/nav/portadmin/snmp/cisco.py b/python/nav/portadmin/snmp/cisco.py index 1a10f6f85b..e77d1356b2 100644 --- a/python/nav/portadmin/snmp/cisco.py +++ b/python/nav/portadmin/snmp/cisco.py @@ -89,6 +89,15 @@ def __init__(self, netbox, **kwargs): self.voice_vlan_oid = '1.3.6.1.4.1.9.9.68.1.5.1.1.1' self.cdp_oid = '1.3.6.1.4.1.9.9.23.1.1.1.1.2' + @classmethod + def can_handle(cls, netbox: manage.Netbox) -> bool: + """Returns True if this handler can handle this netbox""" + if netbox.type and cls.OTHER_ENTERPRISES.is_a_prefix_of( + netbox.type.sysobjectid + ): + return False + return super().can_handle(netbox) + @translate_protocol_errors def get_interface_native_vlan(self, interface): return self._query_netbox(self.vlan_oid, interface.ifindex) From f7e798aac5b10b1ee808280a27c2fd4d2b603a74 Mon Sep 17 00:00:00 2001 From: Morten Brekkevold Date: Fri, 29 Nov 2024 10:41:35 +0000 Subject: [PATCH 4/5] Added news fragment --- changelog.d/3249.added.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/3249.added.md diff --git a/changelog.d/3249.added.md b/changelog.d/3249.added.md new file mode 100644 index 0000000000..7ce8ecc417 --- /dev/null +++ b/changelog.d/3249.added.md @@ -0,0 +1 @@ +Added support for setting access port VLANs for Cisco Small Business switches in PortAdmin (by reverting to non-Cisco-specific handler routines for this subgroup of products) From b6c327d8ec26cbbe258a0875ee862c29b5ddecde Mon Sep 17 00:00:00 2001 From: Morten Brekkevold Date: Wed, 4 Dec 2024 09:18:13 +0000 Subject: [PATCH 5/5] Refactor `ManagementFactory.get_instance()` Make the fallback algorithm more explicit, as per code review comments. --- python/nav/portadmin/management.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/python/nav/portadmin/management.py b/python/nav/portadmin/management.py index f78cbdade5..d207492535 100644 --- a/python/nav/portadmin/management.py +++ b/python/nav/portadmin/management.py @@ -24,7 +24,8 @@ from nav.portadmin.snmp.hp import HP from nav.portadmin.napalm.juniper import Juniper -SUPPORTED_HANDLERS = (Cisco, Dell, H3C, HP, Juniper, SNMPHandler) +SUPPORTED_HANDLERS = (Cisco, Dell, H3C, HP, Juniper) +FALLBACK_HANDLER = SNMPHandler class ManagementFactory(object): @@ -37,11 +38,9 @@ def get_instance(cls, netbox: manage.Netbox, **kwargs) -> ManagementHandler: if not netbox.type: raise NoNetboxTypeError() - for handler_class in SUPPORTED_HANDLERS: - if handler_class.can_handle(netbox): - break - - return handler_class(netbox, **kwargs) + matched_handlers = (h for h in SUPPORTED_HANDLERS if h.can_handle(netbox)) + chosen_handler = next(matched_handlers, FALLBACK_HANDLER) + return chosen_handler(netbox, **kwargs) def __init__(self): pass