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

Add dedicated NodeRole.ControlService to distinguish it from ordinary hosts #17

Closed
amdfxlucas opened this issue Jan 25, 2025 · 6 comments · Fixed by #22
Closed

Add dedicated NodeRole.ControlService to distinguish it from ordinary hosts #17

amdfxlucas opened this issue Jan 25, 2025 · 6 comments · Fixed by #22
Assignees

Comments

@amdfxlucas
Copy link

currently SCION ControlServices (CS) are treated like end hosts by SEED (they are registered under NodeRole.Host).

However this is sub optimal for at least the following reasons:

  • a SCION simulation always requires at least a CS and border router (BRD) node
    but can make do without a single host (i.e. transit AS)
  • a designated 'type' would ease retrieval/lookup of CS nodes from the registry and facilitate the special treatment it deserves (i.e. branching on node type to select SCION distributables that need to be installed )
  • would enable 'assertions' on node type

Accordingly this PR proposes to add a NodeType for CS'es

@amdfxlucas amdfxlucas self-assigned this Jan 25, 2025
@amdfxlucas amdfxlucas converted this from a draft issue Jan 25, 2025
@amdfxlucas amdfxlucas linked a pull request Jan 25, 2025 that will close this issue
@lschulz
Copy link
Member

lschulz commented Jan 27, 2025

Introducing a new node role might have a big impact on the entire codebase. In contrast to introducing BorderRouter in contrast to Router, the change would be specific to SCION. The CS has no equivalent in BGP.

The cases you quote can all be solved in different ways as far as I can see. I'm not convinced treating the CS extra special is the right way to go.

We should also not forget that the CS is actually multiple services, the certificate server, beacon server, path server, etc. are all distinct. The fact that they are implemented in the same process is just an implementation detail. By the same logic, we could consider introducing node roles for each of the services, but since each Node has only exactly one role that wouldn't work without further changes.

In summary, I would prefer a different solution like adding some kind of label or attribute to the host node.

@martenwallewein, @tjohn327, @LencigGaer What is your opinion?

@amdfxlucas
Copy link
Author

That's not the deciding reasoning behind NodeRoles anyway ( there's a RouteServer role which is very specific to BGP )
Ultimately they appear to exist for convenience and verifiability . I see the point that a node can provide multiple services, but i arose only resistance when i proposed to make NodeRole into an EnumFlag, so roles could be seamlessly combined (AND/OR -ed and tested so the branching simplifies ). They're not willing to reify this fact in their code.

@amdfxlucas
Copy link
Author

amdfxlucas commented Feb 7, 2025

here is a simple demonstrative example that would benefit greatly from this. ... If wanted to no longer install all distributables on all nodes, but only the required ones.

def ScionRouting:: _install_scion(self, node: Node):
        """
           @brief Install SCION packages on the node.        
        """
        node.addBuildCommand(
            'echo "deb [trusted=yes] https://packages.netsec.inf.ethz.ch/debian all main"'
            ' > /etc/apt/sources.list.d/scionlab.list')
        
        software = ''

        # SCION doesn't affect IntraDomain Routers     
        if NodeRole.Router != node.getRole():
            software += ' scion-daemon scion-dispatcher scion-tools '               

        if node.getRole() == NodeRole.Host:
            software += " scion-apps-bwtester"

        if NodeRole.BorderRouter == node.getRole():
            software += ' scion-border-router '

        if NodeRole.ControlService == node.getRole():   # <<<< CURRENTLY IMPOSSIBLE !!!!!
            software += 'scion-control-service'

        node.addBuildCommand(
            "apt-get update && apt-get install -y {soft}".format(soft=software)   )
        node.addSoftware("apt-transport-https")
        node.addSoftware("ca-certificates")

As of now, the only way to achieve this by rather awkwardly adding the node's AS as a function parameter so we can call 'getControlServices()->List[str]' to test if a given node is a Control Service.
The code is full such examples. .. .

An alternative would be to make ControlService a real 'Service' , which is installed on host nodes. Then we could call Node::getClasses()->List[str] and check if it contains 'SCION_ControlService' service-class-label.
Or one could check that: 'cs' in node.getName() which is also not nice.

@amdfxlucas
Copy link
Author

Introducing a new node role might have a big impact on the entire codebase. In contrast to introducing BorderRouter in contrast to Router, the change would be specific to SCION. The CS has no equivalent in BGP.

The cases you quote can all be solved in different ways as far as I can see. I'm not convinced treating the CS extra special is the right way to go.

We should also not forget that the CS is actually multiple services, the certificate server, beacon server, path server, etc. are all distinct. The fact that they are implemented in the same process is just an implementation detail. By the same logic, we could consider introducing node roles for each of the services, but since each Node has only exactly one role that wouldn't work without further changes.

In summary, I would prefer a different solution like adding some kind of label or attribute to the host node.

@martenwallewein, @tjohn327, @LencigGaer What is your opinion?

I think Labels are a bit hacky (they will show up in the docker-compose.yml file as

org.seedsecuritylabs.seedemu.meta.KEY: VALUE

in addition to the already existing 'role' label (which would also be 'SCION Control Service').

Moreover it would produce an ugly inconsistency/exception.. for all nodes the checks are like '.getRole()==xyz' but for ControlServices it would be ' if 'ControlService' in node.getLabels().values()' or sth.

@martenwallewein
Copy link

I thought quite a while about this and I think adding a ControlService role seems the most valuable approach for me. I know that in BGP we don't have that role, but I see the ControlService as more as a pure "service" in seed. It's more a required infrastructure element which needs to be known in the topology.

@lschulz
Copy link
Member

lschulz commented Feb 8, 2025

It seems adding a new node role is indeed the most viable option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging a pull request may close this issue.

3 participants