Skip to content

Fix Quagga/FRR parser on IPv6 BGP sessions #122

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
qiluo-msft merged 1 commit into from
Jan 7, 2020
Merged

Fix Quagga/FRR parser on IPv6 BGP sessions #122

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions src/sonic_ax_impl/lib/quaggaclient.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,15 +22,17 @@ def parse_bgp_summary(summ):
l = ls[li]
if l.startswith('Neighbor '):
break
if l.startswith('No IPv'): # eg. No IPv6 neighbor is configured
Copy link
Copy Markdown
Collaborator

@zhenggen-xu zhenggen-xu Jan 4, 2020 •
edited by qiluo-msft
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we define these strings as constant at beginning so it would be easier to maintain? #Resolved

Copy link
Copy Markdown
Contributor

@yxieca yxieca Jan 6, 2020 •
edited by qiluo-msft
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a great idea. We could define a list of criteria to skip (with comments) at one place and check them in a loop here. #Resolved

Copy link
Copy Markdown
Contributor Author

@qiluo-msft qiluo-msft Jan 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion, both the strings and the functions such as startswith or endswith are important. So keep them together help maintenance.

In reply to: 363017378 [](ancestors = 363017378)

Copy link
Copy Markdown
Contributor

@yxieca yxieca Jan 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could define 2 lists of strings. one for starts with and one for ends with. They are still defined together. And the code to make use of these 2 lists are also together. This way, the data and handling are separate. I think it is cleaner this way.

Copy link
Copy Markdown
Contributor Author

@qiluo-msft qiluo-msft Jan 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can improve this in future. This is a quick bug fix for 201811 FRR special use case required by @zhenggen-xu . Since it is tested by him, let's keep the code in this PR.

if l.startswith('No IPv'): # eg. No IPv6 neighbor is configured, in Quagga (version 0.99.24.1)
return bgpinfo
if l.endswith('> exit'): # last command in the lines
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)
return bgpinfo
li += 1

## Read and store the table header
if li >= n:
raise ValueError('No table header found')
raise ValueError('No table header found: ' + summ)
hl = ls[li]
li += 1
ht = re.split('\s+', hl.rstrip())
Expand Down
5 changes: 1 addition & 4 deletions tests/mock_tables/bgpsummary_ipv6_nobgp.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1 @@
Hello, this is Quagga (version 0.99.24.1). Copyright 1996-2005 Kunihiro Ishiguro, et al. User Access Verification
"Password:
str-msn2700-05> show ipv6 bgp summary
str-msn2700-05> exit
show ip bgp ipv6 summary
12 changes: 9 additions & 3 deletions tests/test_vtysh.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
import os
import sys

INPUT_DIR = os.path.dirname(os.path.abspath(__file__))
modules_path = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
sys.path.insert(0, os.path.join(modules_path, 'src'))
sys.path.insert(0, os.path.join(modules_path, 'tests'))

from unittest import TestCase
from unittest.mock import patch, mock_open
Expand All @@ -17,6 +19,7 @@
from sonic_ax_impl.mibs.ietf import rfc4363
from sonic_ax_impl.main import SonicMIB
from sonic_ax_impl.lib.quaggaclient import parse_bgp_summary
from mock_tables.socket import MockGetHostname

class TestSonicMIB(TestCase):
@classmethod
Expand Down Expand Up @@ -106,10 +109,13 @@ def test_getpdu_ipv4_overwite_ipv6(self):
self.assertEqual(str(value0.name), str(oid))
self.assertEqual(value0.data, 6)

def parse_no_bgp():
filename = 'bgpsummary_ipv6_nobgp.txt'
def test_parse_no_bgp(self):
filename = INPUT_DIR + '/mock_tables/bgpsummary_ipv6_nobgp.txt'
with open(filename, 'rb') as f:
bgpsu = f.read()
hostname = MockGetHostname()
prompt_hostname = ('\r\n' + hostname + '> ').encode()
bgpsu += prompt_hostname
bgpsu = bgpsu.decode('ascii', 'ignore')
bgpsumm_ipv6 = parse_bgp_summary(bgpsu)
self.assertEqual(bgpsumm_ipv6, [])