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

Implement RFC 8050/7911 MRT Additional Path extension #203

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kenkeys
Copy link
Member

@kenkeys kenkeys commented Dec 14, 2020

Depends on CAIDA/libparsebgp#82
Fixes CAIDA/libparsebgp#76

There were several options for how to format the new addtional path id field in bgpreader -e output. Keep in mind, the idea behind the additional path feature is that multiple AS paths can coexist for a given prefix (from the same peer); i.e., a received path should not replace a path received earlier for the same prefix but with a different path id.

In bgpdump -m output, the new additional path id field appears as a new column in a record with a new TABLE_DUMP2_AP type. Here's the bgpdump output for a traditional record (for comparison) and a new AP record:

TABLE_DUMP2|1576051200|B|185.19.140.105|198349|1.0.0.0/24|198349 1267 13335|IGP|185.19.140.105|0|0|1267:180 1267:200|NAG|13335 172.68.196.1|
TABLE_DUMP2_AP|1576051200|B|185.36.140.18|47950|1.0.0.0/24|100|47950 1299 13335|IGP|185.36.140.18|0|0|1299:30000|NAG|13335 162.158.84.1|

They added the path_identifier field "100" after the prefix. That was easy enough to emulate in bgpreader -m.

Here are the same two records in bgpreader -e format, without the new path_identifier field:

# <rec-type>|<elem-type>|<rec-ts-sec>.<rec-ts-usec>|<project>|<collector>|<router>|<router-ip>|<peer-ASN>|<peer-IP>|<prefix>|<next-hop-IP>|<AS-path>|<origin-AS>|<communities>|<old-state>|<new-state>
#
# <rec-type>: R RIB, U Update
# <elem-type>: R RIB, A announcement, W withdrawal, S state message
#
R|R|1576051200.000000|singlefile|singlefile|||198349|185.19.140.105|1.0.0.0/24|185.19.140.105|198349 1267 13335|13335|1267:180 1267:200||
R|R|1576051200.000000|singlefile|singlefile|||47950|185.36.140.18|1.0.0.0/24|185.36.140.18|47950 1299 13335|13335|1299:30000||
  • Option 0: add a new path id column after prefix.
    Pro: this is the most obvious option. Authors of all existing code would be forced to consider if/how to deal with the new path id field.
    Con: it breaks all existing code that parses bgpreader output, even if that code did not need to break, i.e. the code did not care about prefixes.

  • Option 1: add a new column to the end of each elem line.
    Pro: Doesn't break code that doesn't care about prefix (e.g., just accumulating all possible paths or AS adjacencies) if it just ignores the extra column.
    Con: Silently produces bad results in code that does care about prefix if it just ignores extra column.

  • Option 2: similar to bgpdump: add a new elem-type, with a new column. E.g., appending "a" to the elem-type indicates that there's a new additional path id column after the prefix:

    R|Ra|1576051200.000000|singlefile|singlefile|||47950|185.36.140.18|1.0.0.0/24|100|185.36.140.18|47950 1299 13335|13335|1299:30000||
    

    Pro: wouldn't break existing code if it ignores elems with the unknown type. If that code cares about prefixes, it would ignore those elems, but at least it wouldn't produce invalid results.
    Con: would break existing code that rejects the unknown elem type, even if that code didn't care about prefixes.

  • Option 3: add path id as a sub-field to prefix, e.g. separated by "#":

    R|R|1576051200.000000|singlefile|singlefile|||47950|185.36.140.18|1.0.0.0/24#100|185.36.140.18|47950 1299 13335|13335|1299:30000||
    

    Pro: wouldn't break code that doesn't look at prefix. Code that does care about prefix will be forced to consider if/how to deal with the new path id field.
    Con: awkward format

In all cases, code that doesn't care about prefixes could be easily modified to ignore the new path id field, so breaking that code isn't really that big of a deal.

I implemented option 3, although now I'm thinking option 0 might be better. @digizeph, @alistairking, what's your opinion?

@dteach-rv
Copy link
Collaborator

My two cents, option 0 makes the most sense to me. Perhaps this could be move into a release candidate or a new minor revision. That would give us an opportunity to solicit some feedback from the bgpstream user community before having to make a breaking change.

@alistairking
Copy link
Member

Hmm. This is a tough one.
I think my biggest concern is that users who don't know the format has changed continue to parse the row as before but get garbage data. Depending on how they are parsing the prefix/IP address this could happen in options 0 and 3.

My preference would be a change that either doesn't change results for users (assuming they're using lazy parsing code like splitting on pipes), or one that breaks things in a such an obvious way that they'll notice.

Probably the most polite approach is to add a new elem subtype column, but that feels like it adds unnecessary complexity.

Hm, what about... we only output add-path elems in bgpreader if the user explicitly asks for them. If we did that then we could go with option 3 so the overall columns don't change, and then if/when we get to v3 one day we could consider changing the default to including add path elems.

@kenkeys
Copy link
Member Author

kenkeys commented Feb 19, 2021

I like the idea of making it opt-in. The user can turn it on when they're ready for it. It's compatible with both options 0 and 3, but I think 0 is easier for parsers to deal with.

The cmdline option could be something like {-E|--output-elems-option} addpath={0|1} (similar to the existing {-o|--data-interface-option}), flexible enough to add more formatting options later if needed. We make addpath=0 the default for now, but someday we could change it to addpath=1.

If the option is disabled, and there is a message with addpath, we could print a (one-time) warning something like "Skipping message(s) with RFC 7911 Additional Path extension. Use -E addpath=1 to enable."

@alistairking
Copy link
Member

Yep, that sounds good.

Maybe as a convenience -E addpath could be treated as -E addpath=1?
And/or use {enable|disable} (or off/on or something) in place of {0|1} so it reads a little bit nicer?

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.

RIB parsing error
3 participants