Improve `ockam message send` command for graceful errors when sending to destinations that don't exist

[mrinalwadhwa]

Current Behaviour

I recorded a quick video to describe the current state:

https://user-images.githubusercontent.com/159583/184283962-7e22a8cc-fa99-44e3-b68b-d842b64bea9e.mp4

> ockam node create n1
> ockam message send --to /node/n2/service/uppercase
...
> ockam message send --to /node/n1/service/upper
...

Desired Behaviour

We want to improve ockam message send command for graceful errors when sending to destinations that don't exist

1. There should be a --timeout on ockam message send command with some default value for how long we'll wait to receive a reply.
2. When sending to a destination that includes a node that is not (see ockam node list command)

The command code for the send command is here https://github.com/build-trust/ockam/blob/develop/implementations/rust/ockam/ockam_command/src/message/send.rs

The service that it invokes when the --from option is used is here. https://github.com/build-trust/ockam/blob/develop/implementations/rust/ockam/ockam_api/src/nodes/service/message.rs#L53

[michealkeines]

Hi, @mrinalwadhwa

correct me if i am wrong...

ockam message send --to /node/n4/service/uppercase

for invalid node name (n4), we could check internally if the node exists before sending the message right?

ockam message send --to /node/n1/service/upper

for invalid service name(upper), your timeout option would be the solution

[mrinalwadhwa]

@michealkeines thank you for looking into this. Yes on both your questions.

[michealkeines]

Hi @mrinalwadhwa

Could you please review this, i have sample code to check whether the node exists before sending message

pub fn run(opts: CommandGlobalOpts, cmd: SendCommand) {
        // Check if the Node exist
        let config = &opts.config.clone();
        let mut node: bool = false;
        {
            let inner = config.get_inner();

            if !inner.nodes.is_empty() {
                // Iterate over all Nodes
                for (current_node, _) in &inner.nodes {
                    let first_node = &cmd.to.first();
                    // this unwrap won't panic, as we enter this block only if a node is there to check
                    let into_multi = first_node.as_ref().unwrap().data().0;
                    match std::str::from_utf8(&into_multi) {
                        Ok(value) if value == current_node => {
                            node = true;
                            break;
                        }
                        _ => continue,
                    };
                }
            }
        }
        if !node {
            eprintln!("Input Node doesn't exist, use `ockam node list` to list all Nodes");
            std::process::exit(exitcode::USAGE);
        }
      // Reducted
}

Output:

[mrinalwadhwa]

@michealkeines that looks great 👍

[michealkeines]

Hi, @mrinalwadhwa I have created PR(https://github.com/build-trust/ockam/pull/3278) for both the changes, please review it.

[mrinalwadhwa]

We merged https://github.com/build-trust/ockam/pull/3278 that adds timeouts and node existence check. Thank you @michealkeines 🥳

However in https://github.com/build-trust/ockam/pull/3289 I remove the node check since it made certain cases fail like this one ...

ockam node create n1
ockam message send hello --to /node/n1/service/uppercase

We'll have to find a better place of doing that check.

[michealkeines]

I tried that same scenario

ockam node create n1
ockam message send hello --to /node/n1/service/uppercase

It works in my feature branch

do you have any other suggestion on where to place it?, i can work on that

[mrinalwadhwa]

Hmm, let me run some more tests and report back.

[adrianbenavides]

We now have the --timeout argument and show a better error when the node doesn't exist:

➜ ock message send --to /node/n2/service/upperca hola  
    Finished dev [unoptimized + debuginfo] target(s) in 0.23s
     Running `target/debug/ockam message send --to /node/n2/service/upperca hola`
OCK500

  × Argument '--to' is invalid
  ╰─▶ Unable to find node named n2
  help: Please report this issue, with a copy of your logs, to https://github.com/build-trust/ockam/issues

  Version 0.91.0, hash: a4212bd9226ccdf464661c5408d021526af72812