Skip to content
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.

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

Asa bgp #772

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Asa bgp #772

wants to merge 6 commits into from

Conversation

thekyr
Copy link

@thekyr thekyr commented Jul 14, 2023

Description

The PR is adding on ASA two more parsers:

  • BGP

    • show bgp summary
    • show bgp { address_family } unicast summary
  • Route

    • show route bgp

Motivation and Context

ASA were missing a parser for the BGP and we needed to create one to be able to run some checks on our monitoring system

Impact (If any)

No negative effect. The code was tested and

Screenshots:

  • script to test the collection of show bgp ipv4 unicast summary
image
  • json output of shown bgp ipv4 unicast summary
image
  • json output of show route bgp
image

Checklist:

  • I have updated the changelog.
  • I have updated the documentation (If applicable).
  • I have added tests to cover my changes (If applicable).
  • All new and existing tests passed.
  • All new code passed compilation.

@thekyr thekyr requested a review from a team as a code owner July 14, 2023 10:32
@thekyr thekyr marked this pull request as draft October 6, 2023 12:43
@thekyr thekyr marked this pull request as ready for review October 6, 2023 12:44
@Taarini Taarini requested review from ThomasJRyan and removed request for Donglin-Wang-Cisco November 1, 2023 19:04
Copy link
Collaborator

@ThomasJRyan ThomasJRyan left a comment

Choose a reason for hiding this comment

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

Hey there! Thank you for the contribution and sorry it's taken so long to get someone on this.

I've left you a few comments to go over. Additionally, all of our parsers need accompanying unittests. You can find examples under the various tests folders in the repo.

You create a folder with the parser class name, create sub equal and empty folders, then add appropriate py and txt files

Comment on lines +6 to +11
* asa
* Added ShowBgpSummary
* show bgp summary
* show bgp {address_family} unicat summary
* Added ShowRouteBgp
* show route bgp
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is good, though in the wrong place. Create a file in https://github.com/CiscoTestAutomation/genieparser/tree/master/changelog/undistributed named asa_bgp_parser_<unix_timestamp>.rst and put this in there

--------------------------------------------------------------------------------
                                      New                                       
--------------------------------------------------------------------------------

* asa
    * Added ShowBgpSummary
      * show bgp summary
      * show bgp {address_family} unicat summary
    * Added ShowRouteBgp
      * show route bgp

Comment on lines +27 to +128
"properties": {
"bgp_id": { "type": "integer" },
"neighbor": {
"type": "object",
"patternProperties": {
"^.+$": {
"type": "object",
"properties": {
"address_family": {
"type": "object",
"patternProperties": {
"^.+$": {
"type": "object",
"properties": {
"version": { "type": "integer" },
"as": { "type": "integer" },
"msg_rcvd": { "type": "integer" },
"msg_sent": { "type": "integer" },
"tbl_ver": { "type": "integer" },
"input_queue": { "type": "integer" },
"output_queue": { "type": "integer" },
"up_down": { "type": "string" },
"state_pfxrcd": { "type": "string" },
"route_identifier": { "type": "string" },
"local_as": { "type": "integer" },
"bgp_table_version": { "type": "integer" },
"routing_table_version": { "type": "integer" },
"attribute_entries": { "type": "string" },
"prefixes": {
"type": "object",
"properties": {
"total_entries": { "type": "integer" },
"memory_usage": { "type": "integer" }
},
"required": ["total_entries", "memory_usage"]
},
"path": {
"type": "object",
"properties": {
"total_entries": { "type": "integer" },
"memory_usage": { "type": "integer" }
},
"required": ["total_entries", "memory_usage"]
},
"total_memory": { "type": "integer" },
"activity_prefixes": { "type": "string" },
"activity_paths": { "type": "string" },
"scan_interval": { "type": "integer" },
"cache_entries": {
"type": "object",
"patternProperties": {
"^.+$": {
"type": "object",
"properties": {
"total_entries": { "type": "integer" },
"memory_usage": { "type": "integer" }
},
"required": ["total_entries", "memory_usage"]
}
}
},
"filter-list": {
"type": "object",
"patternProperties": {
"^.+$": {
"type": "object",
"properties": {
"total_entries": { "type": "integer" },
"memory_usage": { "type": "integer" }
},
"required": ["total_entries", "memory_usage"]
}
}
},
"entries": {
"type": "object",
"patternProperties": {
"^.+$": {
"type": "object",
"properties": {
"total_entries": { "type": "integer" },
"memory_usage": { "type": "integer" }
},
"required": ["total_entries", "memory_usage"]
}
}
}
},
"required": ["version", "as", "msg_rcvd", "msg_sent", "tbl_ver", "input_queue", "output_queue", "up_down", "state_pfxrcd"]
}
}
}
},
"required": ["address_family"]
}
}
}
},
"required": ["bgp_id", "neighbor"]
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

So, while I definitely see what you're going for here, and it's fair to assume we would be using some sort of standardized schema engine, Genie actually uses its own in-house engine that looks nothing like this.

I would suggest reading over this documentation and revising your schema
https://pubhub.devnetcloud.com/media/pyats-development-guide/docs/writeparser/writeparser.html#writing-the-schema-class

# * 'show bgp summary'
# * 'show bgp {address_family} unicast summary'
# =====================================================
class ShowBgpSummary(ShowBgpSummarySuperParser, ShowBgpSummarySchema):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't need to subclass from ShowBgpSummarySchema since ShowBgpSummarySuperParser has it already

Suggested change
class ShowBgpSummary(ShowBgpSummarySuperParser, ShowBgpSummarySchema):
class ShowBgpSummary(ShowBgpSummarySuperParser):

* 'show bgp {address_family} summary'
'''

def cli(self, address_family='', cmd='', output=None):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think cmd is actually used anywhere here. I'm thinking we can remove it

Copy link
Collaborator

Choose a reason for hiding this comment

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

And actually, it looks like address_family gets overwritten on line 237. Unless p1 is never matched when an address family is passed into the command, we can probably remove this argument from the super parser as well

Comment on lines +373 to +406
if attribute_entries:
nbr_af_dict['attribute_entries'] = attribute_entries
if num_prefix_entries:
nbr_af_dict['prefixes'] = {}
nbr_af_dict['prefixes']['total_entries'] = num_prefix_entries
nbr_af_dict['prefixes']['memory_usage'] = num_memory_usage

if path_total_entries:
nbr_af_dict['path'] = {}
nbr_af_dict['path']['total_entries'] = path_total_entries
nbr_af_dict['path']['memory_usage'] = path_memory_usage

if total_memory:
nbr_af_dict['total_memory'] = total_memory

if activity_prefixes:
nbr_af_dict['activity_prefixes'] = activity_prefixes

if activity_paths:
nbr_af_dict['activity_paths'] = activity_paths

if scan_interval:
nbr_af_dict['scan_interval'] = int(scan_interval)

if len(cache_dict):
nbr_af_dict['cache_entries'] = cache_dict

if len(entries_dict):
nbr_af_dict['entries'] = entries_dict

if num_community_entries:
nbr_af_dict['community_entries'] = {}
nbr_af_dict['community_entries']['total_entries'] = num_community_entries
nbr_af_dict['community_entries']['memory_usage'] = community_memory_usage
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is generally considered bad practice with our parsers. It's typically best to assign the value right as it's found

Comment on lines +347 to +359
ret_dict = {}
# strip this patter from the original text
# Codes: L - Local, C - connected, S - static, I - IGRP, R - RIP, M - mobile, B - BGP
# D - EIGRP, E - EGP, EX - EIGRP external, O - OSPF, I - IGRP, IA - OSPF inter area
# N1 - OSPF NSSA external type 1, N2 - OSPF NSSA external type 2
# E1 - OSPF external type 1, E2 - OSPF external type 2, E - EGP
# i - IS-IS, L1 - IS-IS level-1, L2 - IS-IS level-2, ia - IS-IS inter area
# * - candidate default, su - IS-IS summary, U - per-user static route, o - ODR
# P - periodic downloaded static route, + - replicated route
# SI - Static InterVRF
p1 = re.sub(r'(?ms)(Codes:.+?)replicated\sroute(\s*SI\s-\sStatic\sInterVRF)?', '', out)

lines = [x.strip() for x in p1.splitlines()]
Copy link
Collaborator

Choose a reason for hiding this comment

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

So actually, I get that you're stripping this from the output, but if you write regex that doesn't match it then this can be removed entirely. If we don't match it we don't have to worry about it

Comment on lines +364 to +377
for line in lines:
if re.match(r'(^[A-Z]{1,2})', line):
entries[line] = list()
last_entry = line
else:
if last_entry == '':
continue

entries[last_entry].append(line)

for k, v in entries.items():
clean_lines.append(' '.join(k.split()) + " " + " ".join(v))

# clean_lines now holds a list of entries in a single line (easier for parsing)
Copy link
Collaborator

Choose a reason for hiding this comment

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

While I don't necessarily disagree with cleaning up the output before parsing, I can't help but feel this makes it more difficult of a process for both yourself and anyone else who might need to make changes to this parser in the future. It also introduces extra failure points should the output change in any way

Comment on lines +393 to +396
# B X.Y.Z.253 255.255.255.255 [20/0] via X.Y.X.17, 1w0d
p2 = re.compile(r'(?P<code>\S+)\s(?P<network>\S+)\s(?P<subnet>\S+)\s\[(?P<route_preference>[\d\/]+)\]')
# [20/0] via 172.25.141.2, 7w0d
p3 = re.compile(r'\[(?P<route_preference>[\d\/]+)\]\svia\s+(?P<next_hop>\S+),\s(?P<date>\S+)')
Copy link
Collaborator

Choose a reason for hiding this comment

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

The regular expressions you have defined here kind of make the entire last 50-ish lines obsolete. They'll never match the other lines, so cleaning them out seems like a waste of effort :/



if not clean_lines:
return
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should still return ret_dict rather than None

# =============================================
# Parser for 'show route bgp'
# =============================================
class ShowRouteBgp(ShowRouteSchema):
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's something I probably should have asked myself before I reviewed the rest of this parser, but couldn't you have just added this to the ShowRoute class instead of creating a new one?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants