Library - Slim down the NodeManagerWorker for node / secure channel

[nazmulidris]

The purpose of the NodeManagerWorker is to be started as a Worker on a node and receive requests to create entities on that node: inlets, outlets, secure channels, etc...

This should be the only responsibility of the NodeManagerWorker:

• accept Requests, with a specific path
• based on the path, unpack the request arguments and call the relevant method on the NodeManager
• get a value back from the NodeManager and make a Response out of it

Example

For example if we look at this dispatched request:

(Get, ["node", "tcp", "connection", address]) => {
    encode_request_result(self.get_tcp_connection(req, address.to_string()).await)?
}

The implementation in the NodeManagerWorker is:

pub async fn get_tcp_connection(&self, req: &RequestHeader, address: String) 
    -> Result<Response<TransportStatus>, Response<Error>> {
    let tcp_transport = &self.node_manager.tcp_transport;
    let sender = match Self::find_connection(tcp_transport, address.to_string()) {
        None => {
            return Err(Response::not_found(
                req,
                &format!("Connection {address} was not found in the registry."),
            ));
        }
        Some(sender) => sender,
    };
    let status = TransportStatus::new(ApiTransport {
        tt: TransportType::Tcp,
        tm: (*sender.mode()).into(),
        socket_address: sender.socket_address(),
        worker_address: sender.address().to_string(),
        processor_address: sender.receiver_address().to_string(),
        flow_control_id: sender.flow_control_id().clone(),
    });
    Ok(Response::ok(req).body(status))
}

It should actually be something like:

pub async fn get_tcp_connection(&self, req: &RequestHeader, address: String) 
    -> Result<Response<TransportStatus>, Response<Error>> {
    match &self.node_manager.get_tcp_connection(address.to_string()).await? {
        Some(transport_status) => Ok(Response::ok(req).body(transport_status),
        None => {
            Err(Response::not_found(
                req,
                &format!("Connection {address} was not found in the registry."),
            ))
         }
    }
}

In the code above all the logic is handled by the NodeManager and the NodeManagerWorker only deals with Request/Responses.

Desired behavior

In this issue handle refactoring the following (each line corresponds to some paths in the handle_request method):

• node / secure channel
• node / secure channel listener

Additional notes

You can follow the way relays are handled in this PR (here is the section on relays).

You will notice that the NodeManagerWorker does not contain a direct reference to a NodeManager but to an InMemoryNode, which itself contains a NodeManager.

This intermediary layer is only required to monitor sessions for relays and portals. In most cases when you call self.node_manager in NodeManagerWorker the node_manager can be dereferenced to a NodeManager on which regular calls can be made: to list workers, get a credential etc....

Note

Related epic: #6237

---

We love helping new contributors! ❤️
If you have questions or need help as you explore, please join us on Discord. If you're looking for other issues to contribute to, please checkout our good first issues.

[Darioazzali]

May I work on this issue too please? :)

[mrinalwadhwa]

@Darioazzali of course, all yours.

[Darioazzali]

Have a couple of doubt:

1. Response from delete_secure_channel and show_secure_channel

In the actual implementation of delete_secure_channel here if there is an error deleting the secure channel the error is traced and transformed in a Option.
So if there is an Erris transformed in None and the response is sent back, but no error message is encoded in the response.
On the other hand, in the delete_secure_channel_listener here if no Address is encountered an error message is encoded in the response and sent back to the handler.
Should the delete_secure_channelbehave the same way as the delete_secure_channel_listener and notice the specific error as a Response<Error> to the handler?
This question apply to show_secure_channel too.

2. Wrappers

There are some methods on NodeManager that are called in distinct part of the code, so redefining with the same name but different arguments will definitely brake the code.
As a solution a wrapper method on NodeManagercan be created, like create_secure_channel_from_request that parse the arguments from the request header, prepare them and then pass that data to the original function that can remain the same. In this way, as an example, the create_secure_channel implementation on NodeManagerWorker has a similar structure:

    pub(super) async fn create_secure_channel(
        &mut self,
        req: &RequestHeader,
        dec: &mut Decoder<'_>,
        ctx: &Context,
    ) -> Result<Response<CreateSecureChannelResponse>, Response<Error>> {
        self.node_manager
            .create_secure_channel_from_request(req, dec, ctx)
            .await
            .map(|secure_channel| {
                Response::ok(req).body(CreateSecureChannelResponse::new(
                    secure_channel.encryptor_address(),
                    secure_channel.flow_control_id(),
                ))
            })
    }

Some other method, like show_secure_channel are called only from a part of code. For consistency does it sound ok to you create those wrapper for that function too?
E.g :

• delete_secure_channel_from_request
• show_secure_channel_from_request
  with that structure:

    pub(super) async fn xxxx_secure_channel(
        &mut self,
        req: &RequestHeader,
        dec: &mut Decoder<'_>,
        ctx: &Context,
    ) -> Result<Response<XxxxxSecureChannelResponse>, Response<Error>> {
        self.node_manager
            .xxxxx_secure_channel_from_request(req, dec, ctx)
            .await
            .map(|secure_channel| {
                Response::ok(req).body(XxxxxSecureChannelResponse::new(
                   .....
                ))
            })
    }

Thank you in advance!

[etorreborre]

Hi @Darioazzali.

1. Response from delete_secure_channel and show_secure_channel

delete_secure_channel should return:

• a NotFound response if the secure channel was not found
• an InternalError if there was an issue when deleting it
• an ok response if everything was successful. We can keep the DeleteSecureChannelResponse even if I don't really see the point in returning the same data that the user sent

That should be the same thing with show_secure_channel and instead of using an Option in the ShowSecureChannelResponse we should use a NotFound status.

2. Wrappers

The idea is to be eventually able to use the NodeManager directly with nice method names, without any consideration for request/responses.

So the format is more like

pub(super) async fn xxxx_secure_channel(
        &mut self,
        header: &RequestHeader,
        request: &XxxxSecureChannelRequest,
        ctx: &Context,
    ) -> Result<Response<XxxxxSecureChannelResponse>, Response<Error>> {
        self.node_manager
            .xxxxx_secure_channel(ctx, request.parameter1, request.parameter2)
            .await
            .map(|secure_channel| {
                Response::ok(req).body(XxxxxSecureChannelResponse::new(
                   .....
                ))
            })
    }

An example of this would be NodeManagerWorker::create_outlet for example (my fault for not giving this as an example in the original description)

[Darioazzali]

Thanks for the quick response @etorreborre.
Just another question about how would be the preferred way the refactoring should aim to.
There are a couple of function, for example create_secure_channel and delete_secure_channel on the NodeManager that are referenced from other part of code.
Does those functions has to have the same exact structure? Because i have a couple of doubt on that.
Let me please take both examples to show you my doubt:

1 delete_secure_channel

The delete_secure_channel on NodeManager has three different caller:

1. portal_replacer
2. the relay replacer
3. the NodeWorkerManager

Since the NodeWorkerManager receives decoded parameter from the minicbor conversion, and those are as not typed (I mean they are simply String and not Address), it is necessary for someone to convert them in order to call delete_secure_channel maintaining its signature.

This is the obvious reason why the actual implementation of NodeWorkerManager carry the burden of some operations that we want to be in other place in order to slim down him's responsibility.

Changing the signature of delete_secure_channel to accept a String instead of the typed argument Address will brake:

• relay_replacer
• portal_replacer

So in order to use the same signature we have to either prepare the arguments before the function call.

2.create_secure_channel

This method on NodeManager is called by the InMemoryNode too in the relay module.
The signature of the method requires that arguments to be:

• addr -> MultiAddr
• authorized_identifiers -> Identifiers

However, clearly, decoding the CreateSecureChannelRequest gives

• addr -> String
• authorized_identifiers -> Option<Vec<String>>

So far the conversion was done in the create_secure_channel method on NodeWorkerManager, but clearly the purpose of this issue is give this responsibility to NodeWorker instead.
In order to solve the only solutions that comes in my mind are:

1. The conversion could be done directly in the arguments given to the function
   This doesn't change so much the responsibility given to NodeWorkerManager that still carry the burden of convert from String to MultiAddr.
   Furthermore the error handling become pretty confused (as the error should be mapped to a bad request error).
   So i think this is not what we want.
2. Change the function signature to accept the decoded arguments(String instead of MultiAddr for example).
   This will change the implementation of create_secure_channel for InMemoryNode that now has to take the responsibility to convert the arguments before calling the method on the NodeManager instance.
   This simply flips the problem because now the SecureChannelCreation implementation for InMemoryNode needs to prepare the arguments. MultiAddr should be converted back to String and Identifier to a Vec<String>.
3. Use generic arguments with trait bounds like M: impl TryInto<MultiAddr> instead of MultiAddr .
   In this implementation error handling gets pretty confused to me because we have to require the trait bound

<A as TryInto<MultiAddr>>::Error: Into<ockam_core::Error>,

But this is clearly not satisfied by the infallible and no sense conversion when addr is actually a MultiAddr:

addr.try_into();

Basically I'm stuck for the function that has multiple callers.
I don't know If this makes sense, basically may I please ask help.

[etorreborre]

@Darioazzali you raise some excellent points. We currently have too many places where we handle untyped data (in general Strings). We definitely need to address that concern.

What I propose is to start by well-typing the NodeManager interface, with Address instead of String, then tackle the propagation of well-typed data in other parts of the system (for example the list of encryptors in the portal_replacer implementation).

This is necessarily going to be a bit awkward at first but let's work our way up and eventually create issues to better type the rest of the stack.

[Darioazzali]

Thnaks @etorreborre.
Sorry for the late response but I've been busy.
Ok, so taking your example
should be something like:

(Post, ["node", "secure_channel"]) => {
                encode_response(self.create_secure_channel(req, dec.decode()?, ctx).await)?
            }

    pub(super) async fn create_secure_channel(
        &mut self,
        req: &RequestHeader,
        create_secure_channel: CreateSecureChannel,
        ctx: &Context,
    ) -> Result<Response<CreateSecureChannelResponse>, Response<Error>> {
        let CreateSecureChannel {
            addr,
            authorized_identifiers,
            timeout,
            identity_name: identity,
            credential_name,
            ..
        } = create_secure_channel;

with a typed request like this:

#[derive(Debug, Clone, Decode, Encode)]
#[rustfmt::skip]
#[cbor(map)]
pub struct CreateSecureChannel {
    #[n(1)] pub addr: MultiAddr,
    #[n(2)] pub authorized_identifiers: Option<Vec<Identifier>>,
    #[n(4)] pub timeout: Option<Duration>,
    #[n(5)] pub identity_name: Option<String>,
    #[n(6)] pub credential_name: Option<String>,
}

Error handling is simply managed by the mini-cbor crate implementing the error conversion.
I have to dive deeper, need some time more but working on this!

[etorreborre]

@Darioazzali correct. Then, once we extract the request parameters, we call a well-typed implementation at the NodeManager level.