Consistent node naming across ros2cli tools

[AAlon]

connects to ros2/ros2cli#158

Summary

This is part of the improvements suggested here mainly intended for CLI tools' usability with security, and depends on this change to ros2cli. It will make it easy to use ros2bag on security-enabled systems.

• Launch nodes using the common naming prefix provided by ros2cli.

Related changes

• Consistent node naming ros2cli#158
• Security directory lookup improvements rcl#332

Refer to

https://discourse.ros.org/t/ros2-security-cli-tools/6647/

[ruffsl]

Wouldn't this hardcoding interfere if a user wanted to have rosbag2 to either play or record traffic while advertising itself under a FQN of the user's choosing? Some example use cases:

• Multiple instances of rosbag2 running simultaneously
  • To avoid node namespace collisions a user would like to designate a unique node name to each
• Simulating or spoofing publishers or subscribers
  • This might be an anti pattern for against anonymous pubsub, but suppose I want to use rosbag2 playback or record to replicate a target ROS2 network as closely as possible, down to the FQN names of the nodes themselves for simulation or testing purposes.

I think it might be more useful to enable ros2 run like runtime arguments like __node:=<node_name> and __ns:<node_namespace>. This might be more flexible and would give a more consistent user experience with the rest of the ros2 CLI. I think it might be nice if most all ros2 CLI tools accepted ros2 run like overrides to control the underlying nodes they spaw to interface with the ROS2 graph.

[AAlon]

The current rosbag implementation already starts up the node with the hardcoded name "rosbag2", so none of those use cases are currently possible 😢
This PR simply adds a prefix and propagates it from the ros2cli layer; which means that when we'd want to add support for user-specified node/ns names, it will be really simple to set the prefix to be the provided namespace, since it's part of the CLI layer.

[Karsten1987]

I am not sure about the implications regarding ros2cli, but the hardcoded rosbag2 node name should definitely be changed. I feel it should adhere with the general way ros2 nodes get their node names such that remapping etc works.

[AAlon]

I am not sure about the implications regarding ros2cli, but the hardcoded rosbag2 node name should definitely be changed. I feel it should adhere with the general way ros2 nodes get their node names such that remapping etc works.

Makes sense, but I'll reemphasize the point that this change doesn't contradict any future name changes. We add a prefix so that naming would be consistent across ros2cli nodes by default, making it easier to integrate with security. A future PR could allow users to specify the node name and namespace.

[AAlon]

Any thoughts on this one?

[AAlon]

Rebased properly now.

[ruffsl]

I feel it should adhere with the general way ros2 nodes get their node names such that remapping etc works.

@Karsten1987 how would you suggest this be implemented? Having all unknown args parsed to the ros2bag cli's argparser be passed onto the transport.init(), and consecutively to rclcpp::init; Allowing folks to use the same remapping argument such as __ns:= and __name:=?

rosbag2/rosbag2_transport/src/rosbag2_transport/rosbag2_transport_python.cpp

Lines 82 to 85 in a963c68

	rosbag2_transport::Rosbag2Transport transport;
	transport.init();
	transport.record(storage_options, record_options);
	transport.shutdown();

[AAlon]

This needs rebasing, but in any case would be blocked on ros2/ros2cli#158 since it refers to CLI_NODE_NAME_PREFIX from that PR.

[AAlon]

Rebased. Build would fail because it depends on ros2/ros2cli#158 (which depends on ros2/rclpy#259).

[Karsten1987]

Sorry for being so silent on this, didn't know rosbag was a blocking factor on this.

Allowing folks to use the same remapping argument such as __ns:= and __name:=?

Eventually this rosbag transport interface has to be rewritten a bit more generic to also allow an easy python entry point, but yes. The command line argument should be used to allow remapping et al. That being said, for now I believe it's the correct way to forward all command line arguments to the init function. However, I am unsure if args received in python can just be forwarded to the init function in rclcpp as is.
In any case the implementation here has to be changed:

rosbag2/rosbag2_transport/src/rosbag2_transport/rosbag2_transport.cpp

Lines 53 to 56 in a963c68

	void Rosbag2Transport::init()
	{
	rclcpp::init(0, nullptr);
	}

And then further down the actual call to setup_node as well:

rosbag2/rosbag2_transport/src/rosbag2_transport/rosbag2_transport.cpp

Lines 79 to 85 in a963c68

	std::shared_ptr<Rosbag2Node> Rosbag2Transport::setup_node()
	{
	if (!transport_node_) {
	transport_node_ = std::make_shared<Rosbag2Node>("rosbag2");
	}
	return transport_node_;
	}

Looking at the two snippets, I don't think we can easily forward the arguments but rather pre-parse them and pass them to the individual functions.
All that being said, I am happy to adhere with whatever convention is applied in ros2/ros2cli#158

[AAlon]

Sorry for being so silent on this, didn't know rosbag was a blocking factor on this.

Allowing folks to use the same remapping argument such as __ns:= and __name:=?

Eventually this rosbag transport interface has to be rewritten a bit more generic to also allow an easy python entry point, but yes. The command line argument should be used to allow remapping et al. That being said, for now I believe it's the correct way to forward all command line arguments to the init function. However, I am unsure if args received in python can just be forwarded to the init function in rclcpp as is.
In any case the implementation here has to be changed:
rosbag2/rosbag2_transport/src/rosbag2_transport/rosbag2_transport.cpp

Lines 53 to 56 in a963c68

void Rosbag2Transport::init()
{
rclcpp::init(0, nullptr);
}
And then further down the actual call to setup_node as well:
rosbag2/rosbag2_transport/src/rosbag2_transport/rosbag2_transport.cpp

Lines 79 to 85 in a963c68

std::shared_ptr Rosbag2Transport::setup_node()
{
if (!transport_node_) {
transport_node_ = std::make_shared("rosbag2");
}
return transport_node_;
}
Looking at the two snippets, I don't think we can easily forward the arguments but rather pre-parse them and pass them to the individual functions.
All that being said, I am happy to adhere with whatever convention is applied in ros2/ros2cli#158

I'm a bit confused, does that mean you'd be fine with the current PR? do you have any comments we'd need to address?

Thanks.

[Karsten1987]

I think it might be more useful to enable ros2 run like runtime arguments like __node:=<node_name> and __ns:<node_namespace>

just to make sure, I am fully understanding what's happening here. The current PR as-is does not address this feature quoted above but rather introduce a node-prefix, is that correct?

if so, I believe this PR is consistent with the changes just recently merged to ros2cli and can be merged.
running CI for it:

• Linux
• Linux-aarch64
• macOS
• Windows

[Karsten1987]

it looks like we run into the same issue than here:
ros2/rcl#375

i have to see whether ros2/rcl#350 is related to it.

[rutvih]

@Karsten1987 fix for logging was pushed here: ros2/rcl#384

[Karsten1987]

new round of CI:

• Linux
• Linux-aarch64
• macOS
• Windows

[Karsten1987]

test failures are due to updated cppcheck. Unrelated to this change.