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

Design document for Node Interface Definition Language (IDL) #266

Open
wants to merge 22 commits into
base: gh-pages
Choose a base branch
from

Conversation

artivis
Copy link

@artivis artivis commented Nov 25, 2019

Design document for the 'Node Interface Definition Language (IDL)',

a simple and standardized manner to export the complete interface (action/message/parameter/service) of node(s) in a package.

Initially discussed within the Security WG, this proposal is not security-related only.
Security features and tooling building on top of this will be proposed in a subsequent document.

FYI @ruffsl @jacobperron @thomas-moulard @kyrofa

Signed-off-by: artivis <jeremie.deray@canonical.com>
Signed-off-by: artivis <jeremie.deray@canonical.com>
Signed-off-by: artivis <jeremie.deray@canonical.com>
Signed-off-by: artivis <jeremie.deray@canonical.com>
Signed-off-by: artivis <jeremie.deray@canonical.com>

Define the interface for a given parameter.

Attributes:
Copy link
Author

Choose a reason for hiding this comment

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

ROS 2 introduced read only parameters. Should the parameters attributes includes 'ro/rw' ?

Copy link
Member

Choose a reason for hiding this comment

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

IMO, it's okay to not include the read only status. We can already query this constraint on parameters (and other constraints) at runtime. Unless there's a strong case for static analysis, I feel like describing it in the node IDL just adds another point where information can go out-of-sync.

Copy link
Member

Choose a reason for hiding this comment

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

It might be good if the parameter description could be separated. I could see it being read independently by a node which doesn't have a "Node IDL" but wants to read the declared parameters from a file. That would imply that the parameter would need several additional arguments though (all which are possible to pass using the parameter declaration API: ranges, read-only, ...).

Copy link
Author

Choose a reason for hiding this comment

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

@jacobperron Not that one of the point of this proposal is to access the API info in a 'pre-runtime' fashion - e.g. at launch-parsing time. So querying for ro/rw and other constraints at runtime is kinda too late already.

@dirk-thomas I agree that it might be interesting, but it does feel a little out of scope here. Do you mean something a la ROS 1 rosparam_handler?


These examples are only a subset of use-cases made possible by such an interface.
It's clear that this is useful well beyond security.

Copy link
Author

@artivis artivis Nov 25, 2019

Choose a reason for hiding this comment

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

Suggested change
## Challenges to overcome
This proposal has a number of potential upsides, but it also has some downsides worthy of discussion.
### This is only really useful if it gains significant adoption in upstream packages
This is a valid criticism, particularly from a security standpoint.
However, its usefulness hopefully outweighs the work required to implement it upstream, and it's certainly something that can be contributed by community members given that the interface would be reviewed by the experts in the package.
### Declared and actual interface can get out of sync
This is certainly a concern: an out-of-date interface is debatably less useful than having no interface at all.
One potential solution for this is to more tightly couple the declared and actual interface by creating a library that consumes the declared interface and creates the corresponding ROS entities.
That has its own challenges, of course (e.g. how does it deal with wildcards?), and would need to be properly designed.
This is an area of future investigation beyond the scope of this article.

Copy link
Author

Choose a reason for hiding this comment

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

Not sure if those concerns belong to a design doc...
We therefore keep them as suggestion here until we figure that out.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's worth adding potential issues and how we plan/could address them.

I'm not convinced that adoption is a challenge. I'd argue a node IDL is still useful to individuals, or organizations using it internally.

Regarding sync issues, we could at the very least have a tool that launches each node and does a best-effort sanity check against the IDL (maybe this is what you are describing, I couldn't tell). It could report any discrepancies between the declared and actual interfaces. I bet this check could be added as a unit test in a lot of cases. Beyond that, I think we just have to rely on authors and community reports of inconsistencies.

Copy link
Author

Choose a reason for hiding this comment

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

Ok. I'll keep this as suggestion until updated with plan to address them.

The adoption challenge mentioned here is maybe a little too security (future plan/tools) centric and its tone could be voiced down a little.

As for your suggestion on the sync issue it will be added as potential plan to address it.

Copy link
Member

Choose a reason for hiding this comment

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

Have a verb under ros2 nodl such as verify. Make it really quick and people could potentially run it regularly at runtime, e.g. once a minute, as a sanity check.

Copy link
Member

Choose a reason for hiding this comment

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

Alright, updated suggestion with what was discussed here and pushed it.

Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

Thanks for working on this, I think it's a good initiative and could be quite valuable.

In addition to my comments below, it would also be nice to add an "alternatives" section where we document other options considered (if any) related to the node IDL.

articles/ros_node_idl.md Outdated Show resolved Hide resolved
articles/ros_node_idl.md Outdated Show resolved Hide resolved
articles/ros_node_idl.md Outdated Show resolved Hide resolved
articles/ros_node_idl.md Outdated Show resolved Hide resolved
articles/ros_node_idl.md Outdated Show resolved Hide resolved
articles/ros_node_idl.md Outdated Show resolved Hide resolved
articles/ros_node_idl.md Outdated Show resolved Hide resolved
articles/ros2_node_idl/interface_declaration.xml Outdated Show resolved Hide resolved
articles/ros2_node_idl/interface_declaration.xml Outdated Show resolved Hide resolved

These examples are only a subset of use-cases made possible by such an interface.
It's clear that this is useful well beyond security.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's worth adding potential issues and how we plan/could address them.

I'm not convinced that adoption is a challenge. I'd argue a node IDL is still useful to individuals, or organizations using it internally.

Regarding sync issues, we could at the very least have a tool that launches each node and does a best-effort sanity check against the IDL (maybe this is what you are describing, I couldn't tell). It could report any discrepancies between the declared and actual interfaces. I bet this check could be added as a unit test in a lot of cases. Beyond that, I think we just have to rely on authors and community reports of inconsistencies.

artivis and others added 5 commits November 25, 2019 13:56
Co-Authored-By: Jacob Perron <jacob@openrobotics.org>
Co-Authored-By: Jacob Perron <jacob@openrobotics.org>
Co-Authored-By: Jacob Perron <jacob@openrobotics.org>
Co-Authored-By: Jacob Perron <jacob@openrobotics.org>
Co-Authored-By: Jacob Perron <jacob@openrobotics.org>
@artivis
Copy link
Author

artivis commented Nov 25, 2019

@jacobperron Thanks for this initial review. I'll address it asap.

@artivis
Copy link
Author

artivis commented Nov 25, 2019

In addition to my comments below, it would also be nice to add an "alternatives" section where we document other options considered (if any) related to the node IDL.

I'm not sure of what alternatives we are talking about, alternative ideas to the concept presented here (explicitly exported node api) or alternative implementation design of the IDL or something else ?

Signed-off-by: artivis <jeremie.deray@canonical.com>
Signed-off-by: artivis <jeremie.deray@canonical.com>
Copy link
Member

@dirk-thomas dirk-thomas left a comment

Choose a reason for hiding this comment

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

Using IDL for this is imo a really confusing choice. ROS 2 already used the OMG IDL spec for defining message definitions. Therefore I strongly suggest a different abbreviation / terminology for this.

- Topics (publisher or subscriber)

The information contained within that interface is obviously very valuable on different levels and from different perspectives.
While it is usually readily available to a developer looking at the code, it cannot reliably be automatically extracted.
Copy link
Member

Choose a reason for hiding this comment

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

I assume you are implicitly assuming non-runtime here which would be good to mention. At runtime it should absolutely be possible to introspect these information from a node.

Copy link
Author

Choose a reason for hiding this comment

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

No we are not implicitly assuming non-runtime.
This is somewhat related to a point made in the Security motivation section were we consider the ros2 security generate_policy utility that only captures

snapshot of the live ROS 2 graph

Such snapshot(s) may very well miss some dynamic aspects of the overall graph.

On an other hand we considered code introspection tools as far from ideal - e.g. for cpp.

Copy link
Member

Choose a reason for hiding this comment

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

No we are not implicitly assuming non-runtime.

I would argue you do. All topics / service which are created during startup can perfectly be introspected at runtime. (Or do you see a case where it is not?) It would be technically possible to 1) start a (well written) component (which hopefully uses a lifecycle) and 2) query its "API" when it has finished initialization and using that information 3) generate the XML file representing the nodes interface.

Even with the "Node IDL" there is no limitation that a node can't change its interface later at runtime (add new topic, rename parameter, remove services). This kind of dynamic behavior can obviously not be captured by any static description.

Copy link
Author

@artivis artivis Nov 25, 2019

Choose a reason for hiding this comment

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

The "Node IDL" indeed does not enforce anything and could possibly go out-of-sync. It is one of the 'challenge' discussed in a comment/suggestion above.

The rational precisely covers the kind of dynamic behavior you are describing, assuming that the node author knows best the whole API and writes the (up to date) "Node IDL". Depending on the application, we foresee the development of some tools to catch the other dynamic behavior such as launch-time remapping.


Define the interface for a given topic.

Attributes:
Copy link
Member

Choose a reason for hiding this comment

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

Since the QoS settings are deciding of two endpoints on the same topic name are being matched wouldn't it make sense to also specify those? Otherwise any "connectivity analysis" on these metadata might be incorrect.

Copy link
Author

Choose a reason for hiding this comment

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

Fair point, going to update that.

Copy link
Author

Choose a reason for hiding this comment

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

I added the QoS settings for actions, services and topics (da63aa6). I'm not familiar with the later two so please let me know your take on this.
I also went ahead assuming we would like those to defaults to the same values they do in code, hopefully I'm correctly linking to the defaults.

Copy link
Author

Choose a reason for hiding this comment

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

Forgot to mention, I don't know what type should the qos attribute be. I'm not sure of to translate it to a xml tag attribute here...
If anyone has a suggestion?

Copy link
Member

Choose a reason for hiding this comment

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

Given that they are objects in code, it may not be suited to being an attribute at all, but an object nested under the respective tags.

Copy link
Author

Choose a reason for hiding this comment

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

You mean something similar to <rosparam> in roslaunch?
E.g.

<topic name="/foo/bar" type="std_msgs/msg/String" subscription="true">
  <qos>
    policy_history: best_effort
    policy_reliabiliity: reliable
    ...
  </qos>
</topic>

Copy link
Member

@kyrofa kyrofa Dec 10, 2019

Choose a reason for hiding this comment

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

That's the right idea, yeah, although I'd make those QoS attributes, maybe something like:

<topic name="/foo/bar" type="std_msgs/msg/String" subscription="true">
  <qos policy_history="keep_last" policy_reliabiliity="reliable" \>
</topic>

I also went ahead assuming we would like those to defaults to the same values they do in code

I think this makes sense. Given that the rationale for the defaults is "keep behavior like ROS 1" I doubt they will change and get out of sync.

Copy link
Author

Choose a reason for hiding this comment

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

Updated qos so that it is a nested object in action, service and topic.
Updated the example to highlight qos. The example is not fully finished.

articles/ros_node_idl.md Outdated Show resolved Hide resolved
Attributes:
- **version**: version of schema in use, allowing for future revisions.

#### `node`
Copy link
Member

Choose a reason for hiding this comment

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

Why should multiple independent nodes be described in the same file? I would think a one node-per-file mapping would be much more natural since they can evolve independently.

Copy link
Member

Choose a reason for hiding this comment

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

I agree that it's more natural to have one node per file. I think both option should be possible.

Copy link
Author

@artivis artivis Nov 25, 2019

Choose a reason for hiding this comment

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

In our mind this mechanism is very similar to that of plugins and therefore both options are possible. I'll make this explicit in the text.

Copy link
Member

Choose a reason for hiding this comment

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

I think that allowing multiple nodes in a file is something you would come to regret.

I also think that if you are allowing mutliple nodes in a file, then you are describing a Package IDL, not a Node IDL. Although this comment applies generally to the tone throughout this design document.

Copy link
Author

Choose a reason for hiding this comment

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

@gbiggs Do you have an example to illustrate this? As long as one can parse the info on a per-node basis I don't see any problem.

Copy link
Member

Choose a reason for hiding this comment

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

With XML, one could always xinclude from external paths, such that you could break your "package" IDL into separate node "IDL" files, if one wanted to.

Signed-off-by: artivis <jeremie.deray@canonical.com>
@artivis
Copy link
Author

artivis commented Nov 25, 2019

Using IDL for this is imo a really confusing choice. ROS 2 already used the OMG IDL spec for defining message definitions. Therefore I strongly suggest a different abbreviation / terminology for this.

We were expecting this particular concern to be raised as we share it too. However we kept this terminology - 'Node IDL' - as it best describes the proposal. We're open to suggestions tho.

Copy link
Member

@dirk-thomas dirk-thomas left a comment

Choose a reason for hiding this comment

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

We were expecting this particular concern to be raised as we share it too. However we kept this terminology - 'Node IDL' - as it best describes the proposal. We're open to suggestions tho.

Also the "definition language" part of IDL doesn't make sense since the information is described in XML and not like message interfaces in an actual "definition language" (specific syntax).

Maybe "Node API definition / descriptor"? (We can figure that out later though to not derail the discussion with a bike shed topic like this.)


Define the interface for a given parameter.

Attributes:
Copy link
Member

Choose a reason for hiding this comment

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

It might be good if the parameter description could be separated. I could see it being read independently by a node which doesn't have a "Node IDL" but wants to read the declared parameters from a file. That would imply that the parameter would need several additional arguments though (all which are possible to pass using the parameter declaration API: ranges, read-only, ...).

@artivis
Copy link
Author

artivis commented Nov 25, 2019

Also the "definition language" part of IDL doesn't make sense since the information is described in XML and not like message interfaces in an actual "definition language" (specific syntax).

We did tripped a little over the IDL acronym. Depending on the source the D letter stands for different things and we started this document with Description in mind rather than Definition.

Maybe "Node API definition / descriptor"? (We can figure that out later though to not derail the discussion with a bike shed topic like this.)

Agreed.

Signed-off-by: artivis <jeremie.deray@canonical.com>
It therefore calls for the creation of a standardized way to explicitly define and export this information.

This article defines a high-level abstraction allowing upstream packages to specify the communication requirements of the nodes in the package, such that the final user, be it a developer or a static analysis tool, can benefit from it.
The Interface Definition Language (IDL) specified in the next section is meant to be distributed alongside its associated package, be it in the source code or a generated release packaging format (e.g. debian).
Copy link
Member

Choose a reason for hiding this comment

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

I suggest you find a catchy name. "IDL" is a known term in computer science, ROS 1 defines an IDL, and DDS uses "the" IDL (the one from OMG).

"NoDL"?

Copy link
Contributor

Choose a reason for hiding this comment

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

So I see the potential for confusion, but what is proposed does seem like it could be called an IDL. For nodes. So Node IDL seems appropriate.

Copy link
Author

Choose a reason for hiding this comment

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

I believe the terminology is appropriate, however I do agree that it could be confusing. As discussed in another thread with @dirk-thomas we leave the potential renaming to later on.

ps - I like 'NoDL', other acronymes include 'NAPI', 'NAPID' (Node API Description).

Copy link
Member

Choose a reason for hiding this comment

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

I didn't mean to imply the terminology is incorrect, it is perfectly fine. But it's an over-used term and a catchy name avoids that. :)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, no strong feelings on the name. It is indeed an IDL, but we can name it whatever we like.

This article defines a high-level abstraction allowing upstream packages to specify the communication requirements of the nodes in the package, such that the final user, be it a developer or a static analysis tool, can benefit from it.
The Interface Definition Language (IDL) specified in the next section is meant to be distributed alongside its associated package, be it in the source code or a generated release packaging format (e.g. debian).
Whether the interface is declared or not is up to the package author and should not prevent the correct execution of any system pre-existing the IDL.
Similarly, the declared interface may be only partial and allow for the full use of pre-existing systems and the use of dependent systems on the parts covered by the partial interface.
Copy link
Member

Choose a reason for hiding this comment

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

What will be the impact of not having a complete description of node interfaces? Will you provide a way for a user to know if parts of their system are missing node descriptions? If not, will tools still be useful if the user does not know if they are impacting the entire system? Even if you do, will tools still be useful if applied to just part of the system?

Copy link
Author

Choose a reason for hiding this comment

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

For clarification, just in case: partial here applies at the package level, not all node are required to have their interface described. However, if the interface of a given node is described, then it must be described fully.

The take here is that not every node in a package are meant to be part of a live graph, some may be meant for visualization, debugging and what not and therefore exporting their interface may not be useful.

What will be the impact of not having a complete description of node interfaces?

It is assumed that a tool cannot be used for a node with no exported interface, nor for a node partially described (the later may be subject to technical difficulties).

Will you provide a way for a user to know if parts of their system are missing node descriptions?

This is a bit of a chicken & egg problem. The node IDL proposal addresses the issue that a system cannot be fully described with automatic tools since such tool may not capture the entire dynamic aspect of the graph. There could be tools such as the ros nodl verify you mentioned but we should not sell it as an exhaustive one. Maybe ros nodl try_verify would then be more appropriate.
Related to this discussion.

will tools still be useful if the user does not know if they are impacting the entire system? Even if you do, will tools still be useful if applied to just part of the system?

From our perspective - automating security - the entire live system should be described.
But that may not be true for all tools, thus it would depend on the tool in use, assuming the aforementioned specifications on 'partial'.

Copy link
Member

@jacobperron jacobperron Nov 26, 2019

Choose a reason for hiding this comment

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

From our perspective - automating security - the entire live system should be described.
But that may not be true for all tools, thus it would depend on the tool in use, assuming the aforementioned specifications on 'partial'.

Makes me think of possible use-cases for partial NoDLs. For example, I write a LIDAR driver node that implements a common scan_nodl, but I also have an extra publisher. For security purposes, I want to describe all topics of my node, but for a system construction tool I only care that it implements scan_nodl. As the node author, it'd be nice to include scan_nodl into my custom NoDL and somehow communicate that my node implements scan_nodl.

On the other hand, maybe composable NoDLs are too fragile. If the upstream scan_nodl changes, my node may break my system unexpectedly at runtime. It might be easier to maintain a non-composed NoDL for my node, compared to relying on external descriptions.

Copy link
Author

Choose a reason for hiding this comment

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

Well this isn't really an example of a partial NoDL file but rather a case for template/inheritable/composable one. The final NoDL of your driver would still export the scan_nodl + extra publisher and thus be complete.

I do see the potential for composition for NoDL, but it requires some more thinking - how to distribute template NoDL files ? How to prevent breaking from upstream ? Etc.

Copy link
Member

Choose a reason for hiding this comment

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

Welcome to the wonderful world of interface design! ;) There's oodles of theory behind all this stuff.

Choose a reason for hiding this comment

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

I do see the potential for composition for NoDL, but it requires some more thinking - how to distribute template NoDL files ? How to prevent breaking from upstream ? Etc.

A possibility would be to specify that a node implements a version of a template (as if pointing to a git commit). If the upstream version changes, you are not obligated to implement newer versions. Of course, this might be too much clutter already.

Choose a reason for hiding this comment

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

Makes me think of possible use-cases for partial NoDLs.

I have a use case for partial descriptions in static analysis, but I am kind of in a rush right now. I will write an elaborate post later on.

Consider a complex and popular upstream component, perhaps parts of the navigation stack.
**Every end user** of this component must duplicate the effort of attempting to properly lock it down.

If ROS 2 provided a way for upstream package authors to specify the interface required by the nodes in their package, and if the tools to generate security policies from that interface existed, neither of these problems would exist.
Copy link
Member

Choose a reason for hiding this comment

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

Why is this problem not solved by upstream package authors providing template security policies?

Copy link
Author

Choose a reason for hiding this comment

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

I'd say that's a multi factor issue. I'll cite two at least that are related to one another,

  • Adoption, security is likely among the last of people's concerns.
  • Difficulty, it's not that simple to write policies xml doc(s).

This Node IDL proposal together with a following security-specific proposal aim at tackling both at the same time with a relatively simple solution (for the pkg developer).
Decoupling the Node IDL from security seemed like an opportunity to enable other possibilities and tools.

Copy link
Contributor

Choose a reason for hiding this comment

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

Decoupling the Node IDL from security seemed like an opportunity to enable other possibilities and tools.

Certainly.

I've posted a link to this PR in the ROS Driver Common Interface Standards discussion on ROS Discourse.

Copy link
Member

Choose a reason for hiding this comment

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

Decoupling the Node IDL from security seemed like an opportunity to enable other possibilities and tools.

I absolutely agree, but I think this needs to be done properly to ensure the full potential is realisable. I think you are currently too focused on the package concept and I'm not sure that is correct. I could be convinced otherwise, of course.

Another example of the usefulness of having a static interface is the ability to create graphical tools for putting a ROS system together.
Yet another example would be an additional feature in `ros2 pkg create` that would allow a developer to hand it an IDL and have it generate scaffolding for a node with that interface.

These examples are only a subset of use-cases made possible by such an interface.
Copy link
Member

Choose a reason for hiding this comment

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

It would be better if each of these use cases were written up in detail, at least as detailed as the security one. Otherwise you might miss details relevant to making the node IDL correct for those use cases.

Copy link
Author

Choose a reason for hiding this comment

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

Our primary concern is security, that's how this proposal born. These other use cases are only speculation at this point. We could develop them a little but likely not as much as the sec one.

Copy link
Member

Choose a reason for hiding this comment

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

I'm sure your primary concern is security, but if you are promoting this as something for use cases outside of security then I feel that those use cases should also be properly described and taken into account. Perhaps gather some feedback specifically on those use cases at Discourse?

Choose a reason for hiding this comment

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

As I mentioned in another comment, I believe I have a use case for this in static analysis. I am still reading all of this for the first time though. I will write a more detailed comment once I go through all of it.


These examples are only a subset of use-cases made possible by such an interface.
It's clear that this is useful well beyond security.

Copy link
Member

Choose a reason for hiding this comment

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

Have a verb under ros2 nodl such as verify. Make it really quick and people could potentially run it regularly at runtime, e.g. once a minute, as a sanity check.

How do upstream packages specify their interface requirements?
Through a high-level description of all the actions, parameters, services and topics, provided or required, by each node within the package.

The package interface is defined in a separate [XML][xml_wiki] file and exported from the `package.xml` using [REP 149's export mechanism][rep149_export], thereby avoiding pollution of the package manifest.
Copy link
Member

Choose a reason for hiding this comment

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

Why does it have to be tied to the package? Or the node, for that matter? I think it could be useful to distribute node interface descriptions separately and have several different nodes from different packages that support the same interface.

In computer science, this is one of the primary reasons for using interfaces: it allows substituteability. Particularly if you are going after static analysis tools and system constrution tools, such capability would be close to essential.

Copy link
Member

Choose a reason for hiding this comment

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

This is a nice idea. Then there's the question: how to communicate that your node implements a particular NoDL?

Copy link
Member

Choose a reason for hiding this comment

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

We could have common packages that just contain NoDL files, e.g. sensor_nodls, nav_nodls, etc.

Copy link
Author

@artivis artivis Nov 26, 2019

Choose a reason for hiding this comment

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

Yeah how one would tied node and NoDL back together if they are distributed separately. Also I believe that people are somewhat familiar with the plugin export scheme, so a look alike scheme would ease adoption.
Finally separating NoDL files in a separate pkg(s) just increase the chances that the files go out-of-sync.

I do see and appreciate @gbiggs' point on having some kind of template NoDL files tho.

Copy link
Member

@gbiggs gbiggs Nov 28, 2019

Choose a reason for hiding this comment

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

@artivis I think you are looking at this too much from the point of view of describing existing nodes, and not enough from the point of view of describing types. Describing types is ultimately what you are doing, and I think the correct approach is to find a way to state that a node implements a particular NoDL interface rather than saying that every node is a completely unique, unsubstitutable type that needs its own NoDL file.

In some ways this is a philosophical question about where the substitution level is in ROS. There are probably those who say it is at the level of topic, and those who say it is at the level of node. There are definite benefits to doing the later, but it does require more careful engineering and/or tools to support and ROS users may not have an appetite for that. I would argue, though, that if they are at the level of caring about proper security they are probably at the level of caring about good engineering practices and so wouldn't mind some extra complexity if it gives useful benefits, like powerful static analysis tools.

However I think the most important is that the reality is that people treat many nodes in this way already, and the state publisher node discussion on Discourse is a currently-being-discussed example of this.

Copy link
Member

Choose a reason for hiding this comment

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

In this context, I think that describing nodes versus types is a distinction without a difference. In the end, node A has interface X, regardless of where X comes from. As @ruffsl points out in another thread, we can xinclude files from other packages if we want to declare X in one place and have multiple packages/nodes use it. I don't see a solid use-case for designing much more than that at this time.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not asking for designing too much, but I think that we need to make sure what we design now does not close off future potential. Having a way to define node interfaces, parameters a node can take, etc. outside of the source code opens up huge possibilities for better engineering practices for ROS users, but it needs careful thought for what exactly is defined and how to ensure that we don't make it impossible to do, for example, easy substitution of equivalent nodes, or nodes with optional extensions to a defined interface, or verifying the compatibility of two nodes at launch time or in some pre-launch system check.


#### `interface`

This is how the package exports its defined IDL for other tools to consume.
Copy link
Member

Choose a reason for hiding this comment

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

I was not aware that ROS 2 had such a tight coupling between packages and nodes.

Attributes:
- **version**: version of schema in use, allowing for future revisions.

#### `node`
Copy link
Member

Choose a reason for hiding this comment

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

I think that allowing multiple nodes in a file is something you would come to regret.

I also think that if you are allowing mutliple nodes in a file, then you are describing a Package IDL, not a Node IDL. Although this comment applies generally to the tone throughout this design document.

@kyrofa kyrofa mentioned this pull request Dec 2, 2019
34 tasks
@tfoote tfoote self-requested a review December 5, 2019 00:39
Signed-off-by: Kyle Fazzari <kyle@canonical.com>

This is certainly a concern: an out-of-date interface is debatably less useful than having no interface at all.
There are a number of possibilities that will help with this issue.
One possibility is to more tightly couple the declared and actual interface by creating a library that consumes the declared interface and creates the corresponding ROS entities.
Copy link
Member

Choose a reason for hiding this comment

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

I discussed this with some people, but I don't recall who was there, perhaps @ruffsl and @thomas-moulard. We could create an extension to the Node concept (much like LifecycleNode) that consumes the NoDL file as part of the constructor (or early in execution) and would not allow topics/services/parameters/actions outside of that policy to be created either at all or past a certain point in execution (perhaps coupled with the configure state of LifeCycleNode.

Copy link
Member

Choose a reason for hiding this comment

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

That would be an excellent way to ensure these stayed in sync. It's essentially what I was thinking when I mentioned that this could live within RCL (the next sentence, I think). The only thing I dislike about the idea is that it still requires the developers to duplicate the information: once in the NoDL and again in code. It would really be ideal if they only had to write it once, but that's a harder problem to solve.

Copy link
Member

Choose a reason for hiding this comment

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

I think to solve that you would have to take the same approach as messages, and resort to generating base classes for node implementations to inherit from. At this point you are starting to get well into the realm of model-based tool chains and all the benefits and risks that entails. One big risk is the restrictions it places on developers in how they write and manage their code, which can become a barrier to adoption due to the increased learning required. Tools that have done this in the past have seen limited adoption while ROS has been happily adopted by people all over the place.

It's difficult to say which is the correct way to go because there are many facets to the decision.

@kyrofa kyrofa mentioned this pull request Dec 10, 2019
3 tasks
@ros-discourse
Copy link

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/guidelines-on-how-to-architect-ros-based-systems/12641/17

…based on implementation

Signed-off-by: Ted Kern <ted.kern@canonical.com>
@Arnatious
Copy link

Arnatious commented Jul 13, 2020

The proposal has been updated to match the implementation in https://github.com/ubuntu-robotics/nodl .

Changes of note:

  • Added "executable" as an attribute at the node level, specifying the executable containing the node (needed for launch integration)
  • Export mechanism specified as using the ament index plugin system
  • Added details of how to export NoDL files from a package using CMakeLists.txt or setup.py
  • Removed QoS related functionality, can be proposed later as an extension/future revision to NoDL
  • Added mention of a formal ".xsd" schema that is provided in the NoDL package
  • "Node IDL" changed to "NoDL" or "Node Definition Language" throughout

Comment on lines 195 to 198
- **server**: Whether or not the node provides an action server for the action.
Valid values are "true" or "false". Defaults to "false".
- **client**: Whether or not the node provides a client for the action.
Valid values are "true" or "false". Defaults to "false".

Choose a reason for hiding this comment

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

Currently, the two bools for server/client (or publisher/subscription in the topic case) have an OR requirement imposed on them. A node should never have both be false.

This adds a bit of logic that can't be expressed in the .xsd and requires a check in the parsing library and additional error handling (https://github.com/ubuntu-robotics/nodl/blob/f143f51174fb2b5954651534af89bdd165c82bc5/nodl_python/nodl/_parsing/_v1/_parsing.py#L29).

Proposal:
Create a required enum typed "role" tag for Action, Service, and Topic.

Enums would be ServerClientRole: server, client, both and PubSubRole: publisher, subscription, both.

This would allow the schema to enforce this rule and give Action, Service, and Topic an identical interface (name, type, role).

Copy link
Member

Choose a reason for hiding this comment

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

That seems okay to me, does anyone else have any thoughts?

@ros-discourse
Copy link

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2020-07-16/15468/1

@ros-discourse
Copy link

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/matching-the-dynamic-nature-of-ros-to-the-static-nature-of-dds-security/15653/1

@ros-discourse
Copy link

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/announcing-a-ros-package-generator/16317/3

@clalancette
Copy link
Contributor

@artivis What's the status of this design? I see there has been lots of discussion, though no approvals as of yet. Is there more work that needs to be done here?

@artivis
Copy link
Author

artivis commented Sep 24, 2020

@artivis What's the status of this design? I see there has been lots of discussion, though no approvals as of yet. Is there more work that needs to be done here?

Let me forward this to @Arnatious who has picked up this up.

@Arnatious
Copy link

@artivis What's the status of this design? I see there has been lots of discussion, though no approvals as of yet. Is there more work that needs to be done here?

There's been no comment since the last revision of the nodl schema, which added the role attribute. I'm taking that as a good sign.

We have a reference implementation in https://github.com/ubuntu-robotics/nodl, and are nearly done with a simple launch_ros integration pending some changes in SROS2. I think this proposal is ready to move ahead.

@clalancette
Copy link
Contributor

We have a reference implementation in https://github.com/ubuntu-robotics/nodl, and are nearly done with a simple launch_ros integration pending some changes in SROS2. I think this proposal is ready to move ahead.

All right, then we need some approvals. Once we have that, I'll go ahead and merge.

@clalancette clalancette self-assigned this Oct 8, 2020
@budrus
Copy link

budrus commented Oct 8, 2020

@Arnatious Have you thought of using yaml instead of xml? I just read some PRs and saw that for QoS settings first xml was discussed in #296 and then they decided to go the yaml way like for parameters in #300. Is it because of DDS or security interaction?

@kyrofa
Copy link
Member

kyrofa commented Oct 8, 2020

Have you thought of using yaml instead of xml?

We actually prefer YAML, but DDS and SROS2 use XML, and the package.xml (which the nodl would be alongside) is also XML. Using YAML just didn't seem consistent.


This article defines a high-level abstraction allowing upstream packages to specify the communication requirements of the nodes in the package, such that the final user, be it a developer or a static analysis tool, can benefit from it.
The Node Definition Language (NoDL) specified in the next section is meant to be distributed alongside its associated package, be it in the source code or a generated release packaging format (e.g. debian).
Whether the interface is declared or not is up to the package author and should not prevent the correct execution of any system pre-existing the NoDL.

Choose a reason for hiding this comment

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

Would predating be more appropriate than pre-existing?

Choose a reason for hiding this comment

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

I would say either 'correct execution of any pre-existing system' or 'correct execution of any system predating the NoDL'. The latter is probably better to portray the intent.

@ros-discourse
Copy link

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/eprosima-presents-visual-ros/17632/3

@ros-discourse
Copy link

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/eprosima-presents-visual-ros/17632/4

An `.xsd` xml schema is provided alongside the NoDL implementation.
This can be used to programmatically validate the NoDL's `.xml` document.
There are some semantics that cannot be expressed in this schema, so the the `.xsd` is not authoritative.
Rather, it is a heuristic, and the [NoDL reference implementation][nodl-reference] can reject a document that does not conform to other requirements.
Copy link
Member

Choose a reason for hiding this comment

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

I just noticed that the link in this sentence doesn't render correctly.

@aprotyas
Copy link
Member

aprotyas commented Jun 22, 2021

Hi @Arnatious, over the last few weeks - with the help of this design doc and the ubuntu-robotics/nodl API - some preliminary work has been done in writing a nodl_to_policy tool, that converts NoDL description files to a corresponding ROS 2 access control policy file. In doing so, we've come across some ambiguities with the NoDL description that I wanted to raise for discussion here:

  • The NoDL description seems to be lacking the concept of namespace. Every <profile> tag in an access control policy needs a node namespace, and insofar, the only option for namespace is to assume that all nodes are in the default "/" namespace, which is an invalid assumption for many reasons. Having said that, are there/should there be plans to incorporate the namespace concept into NoDL? Possibly as an additional (optional) attribute of the <node> tag?
  • Security enclaves are seen as collections of profiles, and a valid access control policy requires their fully qualified paths. Since the NoDL description does not specify enclave paths, an alternative is to qualify an enclave path using the namespace and node name, for example: "/[ns]/[node]". Having said that, should namespacing get formalized as a concept in the NoDL description, enclave paths are probably a non-issue. Else, we should consider adding the concept of a node's enclave path to the NoDL description too.
  • Thoughts on including default parameter services (describe_parameters, get_parameters, etc.), default topics (rosout, parameter_events), and default actions in a NoDL description? I'm assuming these are implicit with every node, and hence considered redundant from the description's POV. However, when converting from a NoDL description to an access control policy, certain assumptions have to be made about the presence/absence of said default services/topics/actions. This is not ideal from a security POV, even if this simply grants "empty" permissions. A fallout from this discussion is that a node's "type" (is it a lifecycle node?) is currently not expressed in the NoDL description. The "type" of node changes the set of "default" permission tags to consider for it.
  • Should there be a field to specify a node's "type"? I'm thinking of a regular node versus a lifecycle node. An alternate could be to capture the "extra" services/topics of lifecycle nodes with the regular service/topic tags, but that might go against omitting said implicit permission types (previous point).

FYI @iche033 @clalancette @wjwwood

@danthony06
Copy link

I was wondering if this is still under active development and will get merged in?

@artivis
Copy link
Author

artivis commented Dec 2, 2021

Hi @danthony06,
We do plan on picking up this work soon. Note that altho this PR is note yet merged, NoDL-related packages are already part of REP 2005 - ROS 2 Common Packages.

@vmayoral
Copy link
Member

vmayoral commented Dec 2, 2021 via email

@rkent
Copy link

rkent commented Dec 18, 2021

I'm a little late to this discussion, but I am looking at this from the perspective of generating package documentation with a tool like rosdoc2. When we document interfaces in Python or C++, classes and parameters typically have a short description. It would not be difficult to extend rosdoc2 to read this NoDL file and extract information for documentation purposes - but nodes and parameters would need descriptions. Could you add a description attribute (type string) to the node and parameter elements for use by documentation utilities?

[update] I see that rosindex has some recommendations for node documentation in the package.xml file. Coming from a documentation perspective, their examples show a lot of text for nodes descriptions.

What is the relationship of this PR to the rosindex proposal? I don't think you really want to ask package maintainers to document their packages in two places. If this PR becomes the definitive source, then you'll need the same info (that is a description attribute or element) that rosindex wanted.

Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

+1 for this. this is 4 years old design, but description and motivation looks good. this is useful for ROS 2 end users and security enclave configuration.

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.

None yet