Merge pull request #290 from Uninett/bugfix/trap-varbind-resolve

Resolve incoming trap varbinds properly

changelog.d/290.fixed.md

@@ -0,0 +1 @@
+Resolve value types of incoming traps correctly

changelog.d/291.added.md

@@ -0,0 +1 @@
+Handle incoming Juniper BGP traps

src/zino/trapd.py

@@ -6,11 +6,13 @@
 from ipaddress import ip_address
 from typing import Any, Dict, List, NamedTuple, Optional, Set
 
+from pyasn1.type.base import SimpleAsn1Type
 from pysnmp.carrier.asyncio.dgram import udp
 from pysnmp.entity import config
 from pysnmp.entity.rfc3413 import ntfrcv
 from pysnmp.proto.rfc1902 import ObjectName
 from pysnmp.smi import view
+from pysnmp.smi.rfc1902 import ObjectIdentity, ObjectType
 
 import zino.state
 from zino.config.models import PollDevice
@@ -183,7 +185,7 @@ def trap_received(self, snmp_engine, state_reference, context_engine_id, context
         origin = TrapOriginator(address=sender_address, port=sender_port, device=router)
         trap = TrapMessage(agent=origin)
         for var, raw_value in var_binds:
-            mib, label, instance = self._resolve_object_name(var)
+            mib, label, instance, raw_value = self._resolve_varbind(var, raw_value)
             _logger.debug("(%r, %r, %s) = %s", mib, label, instance, raw_value.prettyPrint())
             try:
                 value = _mib_value_to_python(raw_value)
@@ -240,3 +242,18 @@ def _resolve_object_name(self, object_name: ObjectName) -> tuple[str, str, OID]:
             controller = view.MibViewController(engine.getMibBuilder())
         mib, label, instance = controller.getNodeLocation(object_name)
         return mib, label, OID(instance) if instance else None
+
+    def _resolve_varbind(self, name: ObjectName, value: SimpleAsn1Type) -> tuple[str, str, OID, SimpleAsn1Type]:
+        """Resolves a varbind name and value to a MIB, label, instance, and value.  The value will be interpreted
+        according to the resolved MIB object.
+
+        Raises MibNotFoundError if the object's MIB cannot be resolved.
+        """
+        engine = self.snmp_engine
+        controller = engine.getUserContext("mibViewController")
+        if not controller:
+            controller = view.MibViewController(engine.getMibBuilder())
+
+        name, value = ObjectType(ObjectIdentity(name), value).resolveWithMib(controller)
+        mib, label, instance = name.getMibSymbol()
+        return mib, label, instance, value

src/zino/trapobservers/bgp_traps.py

@@ -0,0 +1,91 @@
+"""This module implements BGP trap handling.
+
+Examples of how to send test traps:
+
+snmptrap -v 2c -c public localhost:1162 "" \
+    BGP4-V2-MIB-JUNIPER::jnxBgpM2BackwardTransition \
+    BGP4-V2-MIB-JUNIPER::jnxBgpM2PeerLocalAddrType i 1 \
+    BGP4-V2-MIB-JUNIPER::jnxBgpM2PeerLocalAddr x "0A000002" \
+    BGP4-V2-MIB-JUNIPER::jnxBgpM2PeerRemoteAddrType i 1 \
+    BGP4-V2-MIB-JUNIPER::jnxBgpM2PeerRemoteAddr x "0A000001" \
+    BGP4-V2-MIB-JUNIPER::jnxBgpM2PeerLastErrorReceived x "0102" \
+    BGP4-V2-MIB-JUNIPER::jnxBgpM2PeerState i 4
+
+This sends all the variables required by the MIB, but this trap observer only cares about the remote peer address and
+the peer state value.
+
+"""
+
+import logging
+from ipaddress import ip_address
+from typing import Optional, Tuple
+
+from zino.statemodels import BGPOperState, BGPPeerSession, IPAddress
+from zino.trapd import TrapMessage, TrapObserver
+
+_logger = logging.getLogger(__name__)
+
+
+class BgpTrapObserver(TrapObserver):
+    """Handles BGP peering session operational transition messages"""
+
+    WANTED_TRAPS = {
+        ("BGP4-V2-MIB-JUNIPER", "jnxBgpM2BackwardTransition"),
+        ("BGP4-V2-MIB-JUNIPER", "jnxBgpM2Established"),
+    }
+
+    def handle_trap(self, trap: TrapMessage) -> Optional[bool]:
+        try:
+            peer, state = self._pre_parse_trap(trap)
+        except MissingRequiredTrapVariables:
+            return
+        except ValueError as error:
+            _logger.warning(error)
+            return
+
+        if trap.name == "jnxBgpM2BackwardTransition":
+            self.handle_backward_transition(trap, peer, state)
+        elif trap.name == "jnxBgpM2Established":
+            self.handle_established(trap, peer, state)
+        else:
+            # Something weird happened, let someone else handle it
+            _logger.info("%s: Unknown trap received: %s", trap.agent.device.name, trap.name)
+            return True
+
+    def handle_backward_transition(self, trap: TrapMessage, peer: IPAddress, state: BGPOperState):
+        _logger.debug("BGP backward transition trap received: %r", trap)
+        bgp_peers = trap.agent.device.bgp_peers
+        prev_state = bgp_peers[peer].oper_state if peer in bgp_peers else "unknown"
+
+        if state != BGPOperState.ESTABLISHED and prev_state == BGPOperState.ESTABLISHED:
+            _logger.info("%s Lost BGP peer: %s state %s", trap.agent.device.name, peer, state)
+
+        bgp_peers.setdefault(peer, BGPPeerSession()).oper_state = state
+
+    def handle_established(self, trap: TrapMessage, peer: IPAddress, state: BGPOperState):
+        _logger.debug("BGP established trap received: %r", trap)
+        # TODO Zino 1 does not actually update the internal peering state here, we should verify that this is really
+        #  the desired behavior
+        _logger.info("%s BGP peer up: %s state %s", trap.agent.device.name, peer, state)
+
+    def _pre_parse_trap(self, trap: TrapMessage) -> Tuple[IPAddress, BGPOperState]:
+        if "jnxBgpM2PeerLocalAddrType" not in trap.variables:
+            raise MissingRequiredTrapVariables()
+
+        try:
+            remote_addr = bytes(trap.variables["jnxBgpM2PeerRemoteAddr"].raw_value)
+            peer = ip_address(remote_addr)
+        except ValueError:
+            raise ValueError(f"BGP transition trap received with invalid peer address: {remote_addr!r}")
+
+        try:
+            raw_state = trap.variables["jnxBgpM2PeerState"].value
+            state = BGPOperState(raw_state)
+        except ValueError:
+            raise ValueError(f"BGP transition trap received with invalid peer state: {raw_state}")
+
+        return peer, state
+
+
+class MissingRequiredTrapVariables(ValueError):
+    pass

src/zino/zino.py

@@ -21,7 +21,12 @@
 from zino.trapd import TrapReceiver
 
 # ensure all our desired trap observers are loaded.  They will not be explicitly referenced here, hence the noqa tag
-from zino.trapobservers import ignored_traps, link_traps, logged_traps  # noqa
+from zino.trapobservers import (  # noqa
+    bgp_traps,
+    ignored_traps,
+    link_traps,
+    logged_traps,
+)
 
 STATE_DUMP_JOB_ID = "zino.dump_state"
 # Never try to dump state more often than this:

tests/trapobservers/bgp_traps_test.py

@@ -0,0 +1,101 @@
+import ipaddress
+import logging
+from unittest.mock import Mock
+
+import pytest
+
+from zino.statemodels import BGPOperState, BGPPeerSession
+from zino.trapd import TrapMessage
+from zino.trapobservers.bgp_traps import BgpTrapObserver
+
+
+class TestBgpTrapObserver:
+    def test_when_backward_transition_trap_is_received_it_should_change_bgp_peer_state(self, backward_transition_trap):
+        device = backward_transition_trap.agent.device
+        peer = next(iter(device.bgp_peers.keys()))
+
+        observer = BgpTrapObserver(state=Mock())
+        observer.handle_trap(trap=backward_transition_trap)
+
+        assert len(device.bgp_peers) == 1
+        assert device.bgp_peers[peer].oper_state == BGPOperState.ACTIVE
+
+    def test_when_trap_is_missing_required_varbinds_it_should_do_nothing(self, backward_transition_trap):
+        """jnxBgpM2PeerLocalAddrType is required to be present, according to legacy Zino"""
+        device = backward_transition_trap.agent.device
+        peer = next(iter(device.bgp_peers.keys()))
+        backward_transition_trap.variables.pop("jnxBgpM2PeerLocalAddrType")
+
+        observer = BgpTrapObserver(state=Mock())
+        observer.handle_trap(trap=backward_transition_trap)
+
+        assert len(device.bgp_peers) == 1
+        assert device.bgp_peers[peer].oper_state == BGPOperState.ESTABLISHED
+
+    def test_when_trap_has_invalid_remote_addr_it_should_do_nothing(self, backward_transition_trap):
+        device = backward_transition_trap.agent.device
+        peer = next(iter(device.bgp_peers.keys()))
+        backward_transition_trap.variables["jnxBgpM2PeerRemoteAddr"] = Mock(
+            var="jnxBgpM2PeerLocalAddr", raw_value=b"INVALID"
+        )
+
+        observer = BgpTrapObserver(state=Mock())
+        observer.handle_trap(trap=backward_transition_trap)
+
+        assert len(device.bgp_peers) == 1
+        assert device.bgp_peers[peer].oper_state == BGPOperState.ESTABLISHED
+
+    def test_when_trap_has_invalid_oper_state_it_should_do_nothing(self, backward_transition_trap):
+        device = backward_transition_trap.agent.device
+        peer = next(iter(device.bgp_peers.keys()))
+        backward_transition_trap.variables["jnxBgpM2PeerState"] = Mock(var="jnxBgpM2PeerState", value="INVALIDFOOBAR")
+
+        observer = BgpTrapObserver(state=Mock())
+        observer.handle_trap(trap=backward_transition_trap)
+
+        assert len(device.bgp_peers) == 1
+        assert device.bgp_peers[peer].oper_state == BGPOperState.ESTABLISHED
+
+    def test_when_established_trap_is_received_it_should_just_log_it(self, established_trap, caplog):
+        """This requirement is disputed until Håvard E confirms it"""
+        observer = BgpTrapObserver(state=Mock())
+        with caplog.at_level(logging.INFO):
+            observer.handle_trap(trap=established_trap)
+            assert "BGP peer up" in caplog.text
+
+    def test_when_trap_is_unknown_it_should_pass_it_on(self, established_trap):
+        established_trap.name = "FOOBAR"
+        observer = BgpTrapObserver(state=Mock())
+        assert observer.handle_trap(trap=established_trap)
+
+
+@pytest.fixture
+def backward_transition_trap(localhost_trap_originator) -> TrapMessage:
+    """Returns a correct backward transition trap with internal state to match"""
+    peer = ipaddress.IPv4Address("10.0.0.1")
+    localhost_trap_originator.device.bgp_peers = {peer: BGPPeerSession(oper_state=BGPOperState.ESTABLISHED)}
+
+    trap = TrapMessage(agent=localhost_trap_originator, mib="BGP4-V2-MIB-JUNIPER", name="jnxBgpM2BackwardTransition")
+    trap.variables = {
+        "jnxBgpM2PeerLocalAddrType": Mock(var="jnxBgpM2PeerLocalAddrType", value=1),
+        "jnxBgpM2PeerRemoteAddrType": Mock(var="jnxBgpM2PeerLocalAddrType", value=1),
+        "jnxBgpM2PeerRemoteAddr": Mock(var="jnxBgpM2PeerLocalAddr", raw_value=peer.packed),
+        "jnxBgpM2PeerState": Mock(var="jnxBgpM2PeerState", value="active"),
+    }
+    return trap
+
+
+@pytest.fixture
+def established_trap(localhost_trap_originator) -> TrapMessage:
+    """Returns a correct established trap with internal state to match"""
+    peer = ipaddress.IPv4Address("10.0.0.1")
+    localhost_trap_originator.device.bgp_peers = {peer: BGPPeerSession(oper_state=BGPOperState.ACTIVE)}
+
+    trap = TrapMessage(agent=localhost_trap_originator, mib="BGP4-V2-MIB-JUNIPER", name="jnxBgpM2Established")
+    trap.variables = {
+        "jnxBgpM2PeerLocalAddrType": Mock(var="jnxBgpM2PeerLocalAddrType", value=1),
+        "jnxBgpM2PeerRemoteAddrType": Mock(var="jnxBgpM2PeerLocalAddrType", value=1),
+        "jnxBgpM2PeerRemoteAddr": Mock(var="jnxBgpM2PeerLocalAddr", raw_value=peer.packed),
+        "jnxBgpM2PeerState": Mock(var="jnxBgpM2PeerState", value="established"),
+    }
+    return trap

tests/trapobservers/conftest.py

@@ -0,0 +1,15 @@
+"""Common fixtures for trap tests"""
+
+import ipaddress
+
+import pytest
+
+from zino.statemodels import DeviceState
+from zino.trapd import TrapOriginator
+
+
+@pytest.fixture
+def localhost_trap_originator():
+    addr = ipaddress.IPv4Address("127.0.0.1")
+    device = DeviceState(name="localhost", addresses=set((addr,)))
+    return TrapOriginator(address=addr, port=162, device=device)

tests/trapobservers/logged_traps_test.py

@@ -1,11 +1,9 @@
-import ipaddress
 import logging
 from unittest.mock import Mock
 
 import pytest
 
-from zino.statemodels import DeviceState
-from zino.trapd import TrapMessage, TrapOriginator
+from zino.trapd import TrapMessage
 from zino.trapobservers.logged_traps import RestartTrapLogger
 
 
@@ -17,10 +15,3 @@ def test_when_handle_trap_is_called_it_should_log_trap_name(self, caplog, localh
         with caplog.at_level(logging.INFO):
             observer.handle_trap(trap=trap)
             assert f"localhost: {trap_name}" in caplog.text
-
-
-@pytest.fixture
-def localhost_trap_originator():
-    addr = ipaddress.IPv4Address("127.0.0.1")
-    device = DeviceState(name="localhost", addresses=set((addr,)))
-    return TrapOriginator(address=addr, port=162, device=device)