From a81b402a8ad944db0a20795a135392f4bfaa37ef Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Mon, 9 Mar 2020 09:46:33 -0700 Subject: [PATCH] Parse quagga output without knowledge about hostname, so robust against hostname changes or mismatch (#124) * Parse quagga output without knowledge about hostname, so robust against hostname changes or mismatch * Fix sock init after close * Fix bug * Fix mock socket * Reconnect less frequently, and make sock timeout 1s * Handle parsing protocol error with timeout recv * Fix regex bug * Fix typo and wording * Make regex more strict --- .gitignore | 2 -- src/sonic_ax_impl/lib/perseverantsocket.py | 15 +++++++-- src/sonic_ax_impl/lib/quaggaclient.py | 34 ++++++++++++++------- src/sonic_ax_impl/mibs/vendor/cisco/bgp4.py | 21 +++++++------ tests/mock_tables/socket.py | 33 ++++++++++++++------ tests/test_vtysh.py | 3 -- 6 files changed, 69 insertions(+), 39 deletions(-) diff --git a/.gitignore b/.gitignore index bc6faf5d636e..b9caeccda204 100644 --- a/.gitignore +++ b/.gitignore @@ -17,7 +17,6 @@ dist/ downloads/ eggs/ .eggs/ -lib/ lib64/ parts/ sdist/ @@ -135,6 +134,5 @@ crashlytics.properties crashlytics-build.properties fabric.properties -src/sonic_ax_impl/mibs/vendor/ gh-release.patch tests/test_cpuUtilizationHandler.py diff --git a/src/sonic_ax_impl/lib/perseverantsocket.py b/src/sonic_ax_impl/lib/perseverantsocket.py index 447bcfb4085c..64253320834b 100644 --- a/src/sonic_ax_impl/lib/perseverantsocket.py +++ b/src/sonic_ax_impl/lib/perseverantsocket.py @@ -6,8 +6,15 @@ def __init__(self, family=socket.AF_INET, type=socket.SOCK_STREAM, proto=0, file self.address_tuple = address_tuple self.family = family self.type = type - self.sock = socket.socket(family=self.family, type=self.type, proto=proto, fileno=fileno, *args, **kwargs) - self.sock.settimeout(10) + self.proto = proto + self.fileno = fileno + self.args = args + self.kwargs = kwargs + self._initsock() + + def _initsock(self): + self.sock = socket.socket(family=self.family, type=self.type, proto=self.proto, fileno=self.fileno, *self.args, **self.kwargs) + self.sock.settimeout(1) @property def connected(self): @@ -19,13 +26,15 @@ def connect(self, address_tuple): def reconnect(self): assert self.address_tuple is not None + if self._connected: + self.close() self.sock.connect(self.address_tuple) self._connected = True def close(self): self._connected = False self.sock.close() - self.sock = socket.socket(self.family, self.type) + self._initsock() ## TODO: override __getattr__ to implement auto function call forwarding if not implemented def send(self, *args, **kwargs): diff --git a/src/sonic_ax_impl/lib/quaggaclient.py b/src/sonic_ax_impl/lib/quaggaclient.py index 65fcb890b2a5..a19adc237dd5 100644 --- a/src/sonic_ax_impl/lib/quaggaclient.py +++ b/src/sonic_ax_impl/lib/quaggaclient.py @@ -1,5 +1,6 @@ import re import ipaddress +import socket STATE_CODE = { "Idle": 1, @@ -26,7 +27,7 @@ def parse_bgp_summary(summ): return bgpinfo if l.startswith('% No BGP neighbors found'): # in FRRouting (version 7.2) return bgpinfo - if l.endswith('> '): # directly hostname prompt, in FRRouting (version 4.0) + if (l.endswith('> ') or l.endswith('# ')) and li == n - 1: # empty output followed by prompt, in FRRouting (version 4.0) return bgpinfo li += 1 @@ -88,10 +89,9 @@ def bgp_peer_tuple(dic): class QuaggaClient: HOST = '127.0.0.1' PORT = 2605 - PROMPT_PASSWORD = b'Password: ' + PROMPT_PASSWORD = b'\x1fPassword: ' - def __init__(self, hostname, sock): - self.prompt_hostname = ('\r\n' + hostname + '> ').encode() + def __init__(self, sock): self.sock = sock self.bgp_provider = 'Quagga' @@ -110,10 +110,7 @@ def union_bgp_sessions(self): return neighbor_sessions def auth(self): - cmd = b"zebra\n" - self.sock.send(cmd) - - ## Nowadays we see 2 BGP stack + ## Nowadays we see 2 BGP stacks ## 1. Quagga (version 0.99.24.1) ## 2. FRRouting (version 7.2-sonic) banner = self.vtysh_recv() @@ -123,6 +120,10 @@ def auth(self): self.bgp_provider = 'FRRouting' else: raise ValueError('Unexpected data recv for banner: {0}'.format(banner)) + + ## Send default user credential and receive the prompt + passwd = "zebra" + self.vtysh_run(passwd) return banner def vtysh_run(self, command): @@ -133,11 +134,22 @@ def vtysh_run(self, command): def vtysh_recv(self): acc = b"" while True: - data = self.sock.recv(1024) + try: + data = self.sock.recv(1024) + except socket.timeout as e: + raise ValueError('Timeout recv acc=: {0}'.format(acc)) from e if not data: - raise ValueError('Unexpected data recv: {0}'.format(acc)) + raise ValueError('Unexpected data recv acc=: {0}'.format(acc)) acc += data - if acc.endswith(self.prompt_hostname): + ## 1. To match hostname + ## RFC 1123 Section 2.1 + ## First char of hostname must be a letter or a digit + ## Hostname length <= 255 + ## Hostname contains no whitespace characters + ## 2. To match the prompt line + ## The buffer may containers only prompt without return char + ## Or the buffer container some output followed by return char and prompt + if re.search(b'(^|\r\n)[a-zA-Z0-9][\\S]{0,254}[#>] $', acc): break if acc.endswith(QuaggaClient.PROMPT_PASSWORD): break diff --git a/src/sonic_ax_impl/mibs/vendor/cisco/bgp4.py b/src/sonic_ax_impl/mibs/vendor/cisco/bgp4.py index 25e450e39efe..d2aa5d3cad8f 100644 --- a/src/sonic_ax_impl/mibs/vendor/cisco/bgp4.py +++ b/src/sonic_ax_impl/mibs/vendor/cisco/bgp4.py @@ -11,13 +11,21 @@ def __init__(self): super().__init__() self.sock = PerseverantSocket(socket.AF_INET, socket.SOCK_STREAM , address_tuple=(QuaggaClient.HOST, QuaggaClient.PORT)) - self.QuaggaClient = QuaggaClient(socket.gethostname(), self.sock) + self.QuaggaClient = QuaggaClient(self.sock) self.session_status_map = {} self.session_status_list = [] def reinit_data(self): - pass + if not self.sock.connected: + try: + self.sock.reconnect() + mibs.logger.info('Connected quagga socket') + except (ConnectionRefusedError, socket.timeout) as e: + mibs.logger.debug('Failed to connect quagga socket. Retry later...: {}.'.format(e)) + return + self.QuaggaClient.auth() + mibs.logger.info('Authed quagga socket') def update_data(self): self.session_status_map = {} @@ -25,14 +33,7 @@ def update_data(self): try: if not self.sock.connected: - try: - self.sock.reconnect() - mibs.logger.info('Connected quagga socket') - except (ConnectionRefusedError, socket.timeout) as e: - mibs.logger.debug('Failed to connect quagga socket. Retry later...: {}.'.format(e)) - return - self.QuaggaClient.auth() - mibs.logger.info('Authed quagga socket') + return sessions = self.QuaggaClient.union_bgp_sessions() diff --git a/tests/mock_tables/socket.py b/tests/mock_tables/socket.py index 09fd4acd598c..3288fa4b3ded 100644 --- a/tests/mock_tables/socket.py +++ b/tests/mock_tables/socket.py @@ -4,6 +4,7 @@ import unittest from unittest import TestCase, mock from unittest.mock import patch, mock_open, MagicMock +from enum import Enum INPUT_DIR = os.path.dirname(os.path.abspath(__file__)) modules_path = os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) @@ -19,14 +20,20 @@ def MockGetHostname(): return 'str-msn2700-05' +class State(Enum): + CLOSED = 0 + BANNER = 1 + INTERACTIVE = 2 + class MockSocket(_socket_class): def __init__(self, *args, **kwargs): super(MockSocket, self).__init__(*args, **kwargs) - self._string_sent = b'' - self.prompt_hostname = MockGetHostname().encode() + self.prompt_hostname = (MockGetHostname() + '> ').encode() + self.state = State.CLOSED def connect(self, *args, **kwargs): - pass + self.state = State.BANNER + self._string_sent = b'' def send(self, *args, **kwargs): string = args[0] @@ -34,23 +41,29 @@ def send(self, *args, **kwargs): pass def recv(self, *args, **kwargs): + if self.state == State.CLOSED: + raise OSError("Transport endpoint is not connected") + + if self.state == State.BANNER: + self.state = State.INTERACTIVE + return b'\r\nHello, this is Quagga (version 0.99.24.1).\r\nCopyright 1996-2005 Kunihiro Ishiguro, et al.\r\n\r\n\r\nUser Access Verification\r\n\r\n\xff\xfb\x01\xff\xfb\x03\xff\xfe"\xff\xfd\x1fPassword: ' + + if not self._string_sent or b'\n' not in self._string_sent: + raise socket.timeout + try: - if self._string_sent == b'': - return b'\r\nHello, this is Quagga (version 0.99.24.1).\r\nCopyright 1996-2005 Kunihiro Ishiguro, et al.\r\n\r\n\r\nUser Access Verification\r\n\r\n\xff\xfb\x01\xff\xfb\x03\xff\xfe"\xff\xfd\x1fPassword: ' if self._string_sent == b'zebra\n': return self.prompt_hostname - if b'show ip bgp summary\n' in self._string_sent: + elif b'show ip bgp summary\n' in self._string_sent: filename = INPUT_DIR + '/bgpsummary_ipv4.txt' elif b'show ipv6 bgp summary\n' in self._string_sent: filename = INPUT_DIR + '/bgpsummary_ipv6.txt' - elif b'\n' in self._string_sent: - return self.prompt_hostname else: - return None + return self.prompt_hostname with open(filename, 'rb') as f: ret = f.read() - return ret + self.prompt_hostname + return ret + b'\r\n' + self.prompt_hostname finally: self._string_sent = b'' diff --git a/tests/test_vtysh.py b/tests/test_vtysh.py index 2c09448e3eed..995c9f97cb1e 100644 --- a/tests/test_vtysh.py +++ b/tests/test_vtysh.py @@ -28,9 +28,6 @@ def setUpClass(cls): for updater in cls.lut.updater_instances: updater.update_data() - def test_hostname(self): - self.assertEqual(socket.gethostname(), 'str-msn2700-05') - def test_getpdu_established(self): oid = ObjectIdentifier(20, 0, 0, 0, (1, 3, 6, 1, 4, 1, 9, 9, 187, 1, 2, 5, 1, 3, 1, 4, 10, 0, 0, 61)) get_pdu = GetPDU(