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

rework SCION #30

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

Conversation

amdfxlucas
Copy link
Collaborator

@amdfxlucas amdfxlucas commented Jan 29, 2025

This PR has the aim to reshape the SCION layer a bit, to be more suitable for automatic topology generation based on a DataProvider.

The changes include:

  • allow AS IFIDs in IXP's between pairs of peering ASes to be manually overridden/specified (i.e. if known from CAIDA dataset), rather than auto assigning them.
    The 'ScionAutonomousSystem::getNextIfid(self) -> int:' to access the auto increment interface counter is removed as it presents a stain on the design.
    Its not the responsibility of the AS to know about its interfaces, but this is what the Scion layer is for.

  • do the same for XC IFIDS

  • include IFIDS in graphviz output

  • if latency between IFs is not known, but geolocation -> estimate it to lightspeed propagation of geodesic distance

  • impede misconfiguration of SCION link type/relations (i.e. PARENT/CHILD, core link between non core ASes) with further checks

  • retire the deprecated 'underlay.public' key in topology.json in favor of 'underlay.local' (https://scion.docs.anapaya.net/en/latest/manuals/common.html#topology-json)

  • change the signature of some Node -configuration methods in ScionRouting to also accept the respective local AS of the node. It will provide additional configuration necessary for the set up of the node.

  • enable static routing within SCION ASes (no routing daemon will be installed on border routers) This reduces both runtime CPU load of simulations (no BIRD processes anymore) as well as build time and image size. Static intra-AS routing is an optional config parameter on the SCION layer to support larger topologies with more ASes.


This change is Reviewable

@amdfxlucas amdfxlucas self-assigned this Jan 29, 2025
@amdfxlucas amdfxlucas linked an issue Jan 29, 2025 that may be closed by this pull request
@amdfxlucas
Copy link
Collaborator Author

both SCION tests succeede with the changes in place

Copy link
Member

@lschulz lschulz left a comment

Choose a reason for hiding this comment

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

General remark: Please use whitespace consistently with the rest of the code (and yourself).

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @tjohn327)


seedemu/core/Node.py line 388 at r1 (raw file):

    def getLocalIPAddress(self) -> str:
        """!
        @brief Get the IP address of the local interface for this node.

Is it guaranteed that nodes are connected to only one local network?


seedemu/core/Node.py line 393 at r1 (raw file):

            if iface.getNet().getType() == NetworkType.Local:
                return iface.getAddress()
        return ''

Would it make more sense to raise an exception here? Or return None?


seedemu/layers/Scion.py line 46 at r1 (raw file):

        """
        if self.value == "Core":
            assert core_as, 'non core AS cannot have core link'

Not true, see scionproto/scion#4605
Maybe we should open a new Issue about supporting core peering properly in SEED.


seedemu/layers/Scion.py line 66 at r1 (raw file):

    """

    __links: Dict[Tuple[IA, IA,  str, str, LinkType], int]

Formatting: double space


seedemu/layers/Scion.py line 122 at r1 (raw file):

    @staticmethod
    def getNextIfId( ia: IA ) -> int:

There seems too be some code duplication between PeekNextIfIf and getNextIfId.


seedemu/layers/Scion.py line 187 at r1 (raw file):

        @param b Second AS (ISD and ASN).
        @param linkType Link type from a to b.
        In case of Transit: A is parent

Indent this line or merge with the one above.


seedemu/layers/Scion.py line 299 at r1 (raw file):

                                alabel=f'#{ids[0]}',blabel=f'#{ids[1]}')
            else:
                raise RuntimeError("invalid LinkType")

I'd use an assert in this case. RuntimeError doesn't fit IMO, and SEED uses asserts for general exception throwing a lot.


seedemu/layers/Scion.py line 464 at r1 (raw file):

        b_core = 'core' in b_as.getAsAttributes(b_ia.isd)

        if a_core and b_core: assert rel==LinkType.Core, f'Between Core ASes there can only be Core Links! {a_ia} -- {b_ia}'

Break the line, or write the assertion as one boolean expression.


seedemu/layers/Routing.py line 119 at r1 (raw file):

        rnode.appendStartCommand('[ ! -d /run/bird ] && mkdir /run/bird')
        rnode.appendStartCommand('bird -d', True)
        if has_localnet: rnode.addProtocol('direct', 'local_nets', RoutingFileTemplates['rnode_bird_direct'].format(interfaces = ifaces))

Please put a line break after the semicolon.


seedemu/core/ScionAutonomousSystem.py line 182 at r1 (raw file):

            rnode: ScionRouter = self.getRouter( router.getName() )

            listen_addr= rnode.getLocalIPAddress() if  list([ ifn.getNet().isDirect() for ifn in rnode.getInterfaces() ]).count(True) ==1 else rnode.getLoopbackAddress()

This expression is hard to read. You could factor out the predicate as a local function or lambda.

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.

Don't run BIRD on SCION nodes
2 participants