Bogon.py updates #45

jurraca · 2025-01-03T16:27:07Z

Refactors the bogon.py module with some performance and clarity improvements, and adds tests for the module.

fjahr

Looks great so far. But I don't think we should handle things gracefully in is_bogon_pfx, having an invalid network there is almost certainly an developer error and so it should fail loudly.

kartograf/bogon.py

jurraca · 2025-01-11T11:38:27Z

I added the bugfix commit here for ease but its the same as #37.
Regarding handling valid or invalid networks, we could be getting invalid networks from the RPKI files no? Currently we just cast it and hope it works, afaict. I went in circles with this for a while, and settled on 7ce9637, which:

renames format_pfx to parse_pfx for clarity
returns None if it fails casting to a network
handles that return in the parsing logic of rpki irr and collectors parsing
Maybe that last commit doesn't fit in this PR, but I couldn't figure out a clean way to do it without it. Let me know if I'm thinking about this wrong.

fjahr

I think the renaming and returning None makes sense, thanks! When you are adding the logging I will do some more testing and then it should be ready soon.

On the error handling and formatter/parser: First of all this is of course cleaning up stuff that I didn't do well initially. In the beginning I didn't really know what to expect from the data formats and so I was taking it step by step, looking at what I was getting and then adjusting the parsing. At the time I didn't think through where I wanted the decision to lie if a pfx is valid or not and I was probably a bit too permissive because I wanted to get something works. As I looked at it now a formatting function didn't seem to be the right place and I fear that it may drop some networks that are formatted in a way we did not anticipate but that may be valid if we bring it into the right format. Going forward, ideally we should anticipate in what ways the pfx can be invalid and document these cases, similar how this is done for the comments case. If we encounter something new that we didn't anticipate I guess it is debatable if we should fail or not. Probably logging to the debug file is a better option for now and as we get more knowledge what we might be missing and if we don't see debug logs there anymore maybe we can get more confident and fail more quickly.

What do you think? Does that make sense?

fjahr · 2025-01-12T16:20:42Z

kartograf/util.py

            formatted_pfx = str(ipaddress.ip_network(pfx))
            return f"{formatted_pfx}"
        return str(ipaddress.ip_address(pfx))
+    return None


Can you print the pfx to our debug log here?

Something like: "Parser encountered unknown error while parsing ...."

yep, but I added this in the {rpki, irr, collectors}/parse.py code instead so we don't have to pass the debug.log file around.

fjahr · 2025-01-12T16:43:09Z

kartograf/bogon.py

-            if network.subnet_of(ipaddress.ip_network(ipv6_range)):
-                return True
-
+    if prefix:


I think this isn't needed. There is a prefix and before the places where it is called and I think it's better to handle this outside the function rather than inside.

The is_bogon_asn also doesn't take such precautions and I would like it more if there were consistent on this.

Yes, thank you, I was trying to get to a place where we cast via ipaddress.ip_network(pfx) only once, and everything downstream in the parsing takes an IPvXNetwork() object -- thanks for reading thru my intent :)

fjahr · 2025-01-12T16:49:43Z

kartograf/irr/parse.py


                    last_modified = datetime.strptime(entry["last-modified"], '%Y-%m-%dT%H:%M:%SZ')
                    last_modified = last_modified.replace(tzinfo=timezone.utc)
                    last_modified = last_modified.timestamp()

                    # Bogon prefixes and ASNs are excluded since they can not
                    # be used for routing.
-                    if is_bogon_pfx(route) or is_bogon_asn(origin):
+                    if route and (is_bogon_pfx(route) or is_bogon_asn(origin)):


I think this would do the same and be a bit easier to reason about?

Suggested change

if route and (is_bogon_pfx(route) or is_bogon_asn(origin)):

if not route or is_bogon_pfx(route) or is_bogon_asn(origin):

nice yes, thank you, but this went in #47

fjahr · 2025-01-12T16:50:32Z

kartograf/rpki/parse.py


                # Bogon prefixes and ASNs are excluded since they can not
                # be used for routing.
-                if is_bogon_pfx(prefix) or is_bogon_asn(asn):
+                if prefix and (is_bogon_pfx(prefix) or is_bogon_asn(asn)):


Same here and in the irr parse file, I would prefer: if not prefix or

fjahr · 2025-01-12T16:50:41Z

kartograf/collectors/parse.py

            asn = asn.upper().rstrip('\n')

-            if is_bogon_pfx(prefix) or is_bogon_asn(asn):
+            if prefix and (is_bogon_pfx(prefix) or is_bogon_asn(asn)):


Same here and in the irr parse file, I would prefer: if not prefix or

kartograf/util.py

fjahr · 2025-01-12T21:49:47Z

I added the bugfix commit here for ease but its the same as #37.

Hm, this was a bit confusing to me because I wasn't sure where to add my review comments on this part. In the future, can you be explicit which PR you would me to review first if there is overlap? In Bitcoin Core we either say "This depends on XY, please review that first" or even put a PR into draft status.

jurraca · 2025-01-14T14:45:05Z

this was a bit confusing

OK, i've rebased this on #47 , and this PR depends on it.

Probably logging to the debug file is a better option for now

Yep, good point. When I update my daily run with this, I'll go through the debug logs and see what we get.

I didn't really know what to expect from the data formats

Totally! More than the existing code, it took me a while to think through how the error handling should work tbh. I'm actually surprised this hasn't blown up more often, so it's likely that the upstream sources are pretty good about IP formats.

fjahr · 2025-01-19T18:35:07Z

Looking good here too after another light pass, but also needs rebase first :)

jurraca · 2025-01-21T17:01:49Z

rebased and ran another reproduction run to test.

using sets instead of lists is a faster lookup. Also, compiling the set with networks at module load time will be faster than casting to an ip_network at every iteration.

we're really parsing here, not formatting: the network may be invalid from sources provided upstream. Thus we return None for an invalid network and handle accordingly.

fjahr · 2025-01-26T17:39:58Z

ACK, looks great now!

fjahr reviewed Jan 5, 2025

View reviewed changes

kartograf/bogon.py Outdated Show resolved Hide resolved

kartograf/bogon.py Outdated Show resolved Hide resolved

kartograf/bogon.py Outdated Show resolved Hide resolved

jurraca force-pushed the bogons-fixes branch 2 times, most recently from b6d8e81 to bb1ca3a Compare January 11, 2025 11:34

jurraca force-pushed the bogons-fixes branch from bb1ca3a to 7ce9637 Compare January 11, 2025 11:40

fjahr reviewed Jan 12, 2025

View reviewed changes

jurraca force-pushed the bogons-fixes branch from 7ce9637 to fc73b66 Compare January 14, 2025 14:38

jurraca force-pushed the bogons-fixes branch from fc73b66 to 265db55 Compare January 21, 2025 17:00

jurraca force-pushed the bogons-fixes branch from 265db55 to ab6ac27 Compare January 21, 2025 17:43

jurraca added 4 commits January 24, 2025 13:56

is_bogon_pfx: refactor lists to sets, handle errors

562fff8

using sets instead of lists is a faster lookup. Also, compiling the set with networks at module load time will be faster than casting to an ip_network at every iteration.

is_bogon_asn: slightly faster and more compact function body

c68dccf

Add bogon.py tests

d77df06

rename format_pfx to parse_pfx, adjust parse logic

8bfd8ad

we're really parsing here, not formatting: the network may be invalid from sources provided upstream. Thus we return None for an invalid network and handle accordingly.

jurraca force-pushed the bogons-fixes branch from ab6ac27 to 8bfd8ad Compare January 24, 2025 13:57

fjahr merged commit f9a6fb7 into asmap:master Jan 26, 2025
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bogon.py updates #45

Bogon.py updates #45

jurraca commented Jan 3, 2025

fjahr left a comment

jurraca commented Jan 11, 2025 •

edited

Loading

fjahr left a comment

fjahr Jan 12, 2025

fjahr Jan 12, 2025

jurraca Jan 14, 2025

fjahr Jan 12, 2025

fjahr Jan 12, 2025

jurraca Jan 14, 2025 •

edited

Loading

fjahr Jan 12, 2025

jurraca Jan 14, 2025

fjahr Jan 12, 2025

jurraca Jan 14, 2025

fjahr Jan 12, 2025

jurraca Jan 14, 2025

fjahr commented Jan 12, 2025

jurraca commented Jan 14, 2025 •

edited

Loading

fjahr commented Jan 19, 2025

jurraca commented Jan 21, 2025 •

edited

Loading

fjahr commented Jan 26, 2025

	if route and (is_bogon_pfx(route) or is_bogon_asn(origin)):
	if not route or is_bogon_pfx(route) or is_bogon_asn(origin):

Bogon.py updates #45

Bogon.py updates #45

Conversation

jurraca commented Jan 3, 2025

fjahr left a comment

Choose a reason for hiding this comment

jurraca commented Jan 11, 2025 • edited Loading

fjahr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jurraca Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjahr commented Jan 12, 2025

jurraca commented Jan 14, 2025 • edited Loading

fjahr commented Jan 19, 2025

jurraca commented Jan 21, 2025 • edited Loading

fjahr commented Jan 26, 2025

jurraca commented Jan 11, 2025 •

edited

Loading

jurraca Jan 14, 2025 •

edited

Loading

jurraca commented Jan 14, 2025 •

edited

Loading

jurraca commented Jan 21, 2025 •

edited

Loading