Added support for SCION ASNs > 255.

[LencigGaer]

fixes #6

---

This change is 

[martenwallewein]

Nice to see that things are proceeding here! 🔥

I have two questions for this one:

1. Does it support pure numerical ASNs for SCION, e.g. as we have in SCIERA 71-20965?
2. How is subnetting within the AS solved for ASNs > 255? looking at this one it seems quite hacky https://github.com/netsys-lab/seed-emulator/blob/master/seedemu/core/AutonomousSystem.py#L148

[LencigGaer]

Regarding 1: You were right, I ignored the SCION spec in that case, I fixed that...

Regarding 2: I didn't modify subnetting and all these related functions so far. Currently all IP addresses have to be defined manually for ASNs>255 when creating the topology (e.g. as150.createNetwork('net0', "10.150.1.0/24")), as originally designed in SEED.
Reason: SEED currently takes the ASN directly as subnet (10.<asn>.0.0/16), this is not possible for ASNs>255. Any type of this sort of fixed mapping between ASN and subnet leads to double assigning some subnets to several possible ASNs, which I would see critically. Everything else would require keeping a sort of subnet-pool and assigning subnets just one after another without any correlation between ASN and subnet. I once thought about that, but it would also require several changes to the original SEED codebase if I remember correctly (or -- if we don't want to support mixed SCION-BGP-ASes -- a huge number of stuff we have to reimplement for SCION) and it means that we change the way SEED assigns IPs which seems to me like a bigger conceptual change.

[amdfxlucas]

can't we refactor the code duplication into a free function 'def ASN2str(asn: int)->str' and have ScionAutonomousSystem::getAsnStr(self) and IA::_ str _() call it (the former simply prepending the 'ISD-' to the result) ?!

Also move the computation of asn2 &3 beyond the quick-out wich only depends on asn1 .. .

[lschulz]

I think all functionality related to parsing and printing SCION ASNs (or ISD-ASN) pairs should be bundeled in one or two classes. I propose adding a method getScionAsn() -> IA to ScionAutonomousSystem that returns a smart ASN type that handles being turned into a string correctly. This new method should be called in ScionIsd and other SCION-specific layers instead of getAsn().

A Python IA, ISD-ASN class has been implemented many times.

• Here is one for just the ASN from may Scapy layer: https://github.com/lschulz/scapy-scion-int/blob/main/scapy_scion/scion_addr.py
• The current ISD-AS for Python from scionproto: https://github.com/scionproto/scion/blob/master/tools/topology/scion_addr.py
• There was also an older implementation that I used some years ago: https://github.com/lschulz/scion-ixp-testbed/blob/master/ixp_testbed/address.py
• Probably some more that I forgot

Reviewed 1 of 4 files at r1, 1 of 2 files at r2, all commit messages.
Reviewable status: 2 of 4 files reviewed, 3 unresolved discussions (waiting on @LencigGaer)

---

seedemu/core/ScionAutonomousSystem.py line 21 at r2 (raw file):

    def __str__(self):
        asn1 = f"{self.asn:012x}"[:3].lstrip('0') + f"{self.asn:012x}"[3]

This way of converting to a string seems a bit wasteful, it did it like this:
https://github.com/lschulz/scapy-scion-int/blob/61fd17457c07c8b61287e5e180112386af3e6139/scapy_scion/scion_addr.py#L47

---

seedemu/core/ScionAutonomousSystem.py line 276 at r2 (raw file):

        return self.__generateStaticInfoConfig

    def getAsnStr(self):

I agree with @amdfxlucas, the code from above shouldn't be duplicated here.

---

seedemu/layers/ScionIsd.py line 204 at r2 (raw file):

        return path

    def __asn_to_name(self, asn: int) -> str:

Same as in the other file, call the same code.

[LencigGaer]

Agreed, I also wasn't happy with the code duplication. I now created a new class ScionASN and refactored the IA class, this allowed me to remove the duplicated string conversions and replace the string conversion with the function you suggested, @lschulz. I replaced calls to the old getAsn() with a new getScionAsn() where it made sense.

---

MORE DETAILS:

The SCION SEED code used the IA class (originally inherited from NamedTuple) rather frequently in order to manage ISD and ASN in a combined way and for having a key to access a registry. That's why I wanted to keep the IA class.

The ScionAutonomousSystem class however is not aware of the ISD concept (all ASes are created from within an ISD instance, passing the ISD object to the AS would break the logic). So I additionally created the ScionASN class that does only ASN handling and its string conversions. It is used by both, ScionAutonomousSystem class and IA class, to avoid code duplication.

I added the function getScionAsn() to the ScionAutonomousSystem to get the ScionASN instance associated with an AS. Since the old getAsn() function is used in many places in the SEED code which aren't reimplemented for SCION, I didn't want to replace it. Both, ASN and SCION ASN concept coexist with each other. I updated the getAsn() calls to getScionAsn() where it made sense to me (string conversions, access to SCION specific stuff).

In order to allow the IA class making use of the ScionASN class, I removed the inheritance from NamedTuple to build a new constructor that also initialized a ScionASN instance to be able to handle it, I then reimplemented all operators used in our code to work the same way as the NamedTuple did + string handling for ISD-ASN combinations.

[LencigGaer]

Reviewable status: 1 of 4 files reviewed, 3 unresolved discussions (waiting on @amdfxlucas and @lschulz)

---

seedemu/core/ScionAutonomousSystem.py line 21 at r2 (raw file):

Previously, lschulz (Lars-Christian Schulz) wrote…

This way of converting to a string seems a bit wasteful, it did it like this:
https://github.com/lschulz/scapy-scion-int/blob/61fd17457c07c8b61287e5e180112386af3e6139/scapy_scion/scion_addr.py#L47

Done.

---

seedemu/core/ScionAutonomousSystem.py line 276 at r2 (raw file):

Previously, lschulz (Lars-Christian Schulz) wrote…

I agree with @amdfxlucas, the code from above shouldn't be duplicated here.

Done.

---

seedemu/layers/ScionIsd.py line 204 at r2 (raw file):

Previously, lschulz (Lars-Christian Schulz) wrote…

Same as in the other file, call the same code.

Done.

[lschulz]

It might be a good idea to add a SCION test case with long ASNs to avoid regressions in the future.

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @LencigGaer)

---

seedemu/layers/ScionIsd.py line 143 at r3 (raw file):

                    as_: ScionAutonomousSystem = base_layer.getAutonomousSystem(asn)
                    isds = self.getAsIsds(asn)
                    

Avoid trailing whitespace (PEP8). You can just set your editor to trim trailing whitespace or use a Python code formatter like black.

[LencigGaer]

Added a testcase for this.
Also noticed that I wasn't handling large ASNs for the Graphable class correctly - I fixed that.

Reviewable status: 2 of 9 files reviewed, 1 unresolved discussion (waiting on @lschulz)

---

seedemu/layers/ScionIsd.py line 143 at r3 (raw file):

Previously, lschulz (Lars-Christian Schulz) wrote…

Avoid trailing whitespace (PEP8). You can just set your editor to trim trailing whitespace or use a Python code formatter like black.

Done.

[lschulz]

Reviewable status: 0 of 12 files reviewed, 1 unresolved discussion

[lschulz]

Reviewed 8 of 10 files at r4, 4 of 4 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @LencigGaer)