add ability to include arbitrary BGP dump file as custom source#133
add ability to include arbitrary BGP dump file as custom source#133jorisstrakeljahn wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
I did some smoke testing so far, using some result files from full runs as input to the custom source and checking that the result is the same. I will think a bit more about testing options for this.
EDIT: I will also think a bit more about how we might want to support MRT files as an additional format option. That's definitely out of scope for this PR and I don't think we need to make changes here since we can detect what file type we are dealing with easily.
| parse_rpki(context) | ||
|
|
||
| rpki_file = Path(context.out_dir_rpki) / "rpki_final.txt" | ||
| shutil.copy2(rpki_file, context.final_result_file) |
There was a problem hiding this comment.
Please try to keep the map function clean of this lower level stuff, this should probably be done in parse_rpki. Our abstractions/layers aren't great but this is one thing I would like to keep going :)
There was a problem hiding this comment.
Done, moved into parse_rpki so the map function stays at the orchestration level
| ./run map -irr -rv | ||
| ``` | ||
|
|
||
| You can also include an arbitrary BGP dump file as a custom data source via `-cs` (`--custom-source`). The file should contain space-separated `prefix ASN` pairs, one per line, with an optional `AS` prefix on the ASN. The custom source is merged after RPKI but before IRR, so it takes precedence over IRR and Routeviews but defers to RPKI. The file is copied into the data directory for reproducibility. |
There was a problem hiding this comment.
It is probably easier if you refer to the map file format below here as the expected format, rather than describing the format in prose.
There was a problem hiding this comment.
Yes, replaced the prose with a link to the map file format section below
| # If the user-provided dump contains multiple entries for the same | ||
| # prefix, keep the first occurrence. This matches how the | ||
| # routeviews parser picks the first origin on multi-origin routes. | ||
| if prefix in seen_prefixes: | ||
| if context.debug_log: | ||
| with open(context.debug_log, 'a') as logs: | ||
| logs.write(f"Custom: parser encountered duplicate prefix: {prefix}\n") | ||
| continue | ||
| seen_prefixes.add(prefix) |
There was a problem hiding this comment.
Hm, I don't think this is really comparable. The multi-origin routes are ordered by occurence but we don't know what to expect from the order in the custom file. We could be strict and reject duplicates and ask the user to make a decision for us or we sort the file and initially (maybe at the copy step) and then take the first one. This means we take the lowest ASN which isn't great but at least that way a custom file with the same content but a different order doesn't lead to a different result.
There was a problem hiding this comment.
Right, the routeviews analogy doesn't really fit. Between the two options 'd lean towards strict reject.
Like you noted, sort-then-first effectively means "lowest ASN wins", which is just as arbitrary as the current first-occurrence behavior, only deterministic. Rejecting puts the call back on the user (whose dump it is) and surfaces what's most likely a defect in their export. We can always relax later behind a flag if a real use case shows up.
Happy to go either way though, let me know which one you'd prefer and I will push it.
|
|
||
| def test_parse_only_invalid_entries(tmp_path): | ||
| context = build_test_context(tmp_path, epoch="111111115") | ||
| invalid = "\n".join([ |
There was a problem hiding this comment.
These invalids as well as the valids below are all existing in the fixture file as well. Do those cases really need to be tested twice or is this getting us different coverage in a way I missed?
There was a problem hiding this comment.
You're right, totally missed that, removed the test.
|
Talking with someone who would like to use this feature I learned that their Bird router doesn't offer an export in the format that we need. It would be very nice if we could add some hints in documentation how a bird router can export their routes in a way that works with this function out of the box. GPT gives me some options using bird's own filters or awk. I guess these will work eventually but I couldn't really test them right now because I don't have an active BGP session and generating some dummy data turned out to be a bit annoying. Putting these here as hints anyway: With awk: or with filters: It needs to be the primary table because that's the actual best known route. |
aa580cd to
1c23250
Compare
|
@fjahr pushed fixes for the inline comments and replied to the duplicate-handling thread with a short argument for strict-reject. That one still needs your call on direction. And for the Bird router comment: Same situation here. I don't have a setup to test these against either. As an interim, what about a small section in the README with the filter variant (cleaner than the awk one) and an explicit "not verified" note? That way users have something concrete to start from and can report back if the format isn't right, and we can tighten or replace it once one validates against a live session. Something along these lines: #### Exporting from a Bird router
If you run a Bird BGP router, you can produce a custom source file from your route table using `birdc` filters against your primary tables, which represent the actual best known routes:
```
birdc 'show route primary table master4 filter { if source = RTS_BGP && defined(bgp_path) then { print net, " AS", bgp_path.last; } reject; }' > prefixes.txt
birdc 'show route primary table master6 filter { if source = RTS_BGP && defined(bgp_path) then { print net, " AS", bgp_path.last; } reject; }' >> prefixes.txt
```
These snippets have not been verified end-to-end against a live BGP session, so please confirm the output matches the [map file format](#map-files) before passing the file to kartograf. |
Adds the ability to include an arbitrary BGP dump file as a custom data source via
-cs/--custom-source, as discussed in #113.The input format is
prefix ASNpairs, one per line, with an optionalASprefix on the ASN. Entries are filtered for bogon prefixes, bogon ASNs, and ASNs outside the encoding range (same checks as the routeviews parser), malformed lines are skipped and duplicate prefixes are deduplicated on a first-occurrence basis. For reproducibility, the input file is copied into the run's data directory.The custom source is merged between RPKI and IRR, so it takes precedence over IRR and Routeviews but defers to RPKI.
Adding a new source would have required extending the conditional base_file selection in
merge_pfx2asand the if/elif chain insort.py. Instead, all merge functions now read from and write back tofinal_result_file, which is seeded with the RPKI result. This simplifies the existing code and makes future sources trivially addable.As a consequence of the refactor, the intermediate files
merged_file_rpki_irr.txtandmerged_file_rpki_rv.txtwere renamed tomerged_file_irr.txtandmerged_file_rv.txt, and the IRR merge step's section header was renamed from "Merging RPKI and IRR data" to "Merging IRR and base data".