feat(rust): `ockam node show` to use dynamic data from node #3504

neil2468 · 2022-09-21T17:25:50Z

Current Behavior

ockam node show intends to show the status and details of a node (e.g. a list of its services). At the moment this feature uses static text and assumes things about the node (see below code).
This code is also used by ockam node list and ockam node create.
Related to Make services information displayed with ockam node create | show command more dynamic. #3177.

ockam/implementations/rust/ockam/ockam_command/src/node/show.rs

Lines 43 to 79 in d4dd352

    
           fn print_node_info(node_cfg: &NodeConfig, node_name: &str, status: &str, default_id: &str) { 
        
               println!( 
        
                   r#" 
        
           Node: 
        
             Name: {} 
        
             Status: {} 
        
             Services: 
        
               Service: 
        
                 Type: TCP Listener 
        
                 Address: /ip4/127.0.0.1/tcp/{} 
        
               Service: 
        
                 Type: Secure Channel Listener 
        
                 Address: /service/api 
        
                 Route: /ip4/127.0.0.1/tcp/{}/service/api 
        
                 Identity: {} 
        
                 Authorized Identities: 
        
                   - {} 
        
               Service: 
        
                 Type: Uppercase 
        
                 Address: /service/uppercase 
        
               Service: 
        
                 Type: Echo 
        
                 Address: /service/echo 
        
             Secure Channel Listener Address: /service/api 
        
           "#, 
        
                   node_name, 
        
                   match status { 
        
                       "UP" => status.light_green(), 
        
                       "DOWN" => status.light_red(), 
        
                       _ => status.white(), 
        
                   }, 
        
                   node_cfg.port, 
        
                   node_cfg.port, 
        
                   default_id, 
        
                   default_id, 
        
               ); 
        
           }

Proposed Changes

Modify ockam node show to ask the node for details about itself and display those details.
Add a request/response get /node/service to allow the CLI to get a list of services from a node.
Add a basic test to ockam_command/tests/commands/bats to test the new feature in CI.

Queries

Is the output formatting of ockam node show okay? Instead of a verbose tree, could we output some of the information in a tabular style like docker image ls?

> docker image ls
REPOSITORY                                     TAG       IMAGE ID       CREATED        SIZE
ghcr.io/build-trust/ockam-builder              latest    b10ae04c8b6f   2 weeks ago    4.62GB
ghcr.io/build-trust/ockam-builder              <none>    9cfba9ba6e22   2 months ago   4.58GB
ghcr.io/cross-rs/riscv64gc-unknown-linux-gnu   0.2.4     23b40ea6412a   3 months ago   1.03GB
hello-world                                    latest    46331d942d63   6 months ago   9.14kB

In the output, should the Transport field Payload be renamed?
The request/response get /node/service builds its response from ockam_api::nodes::service::NodeManager.registry. Is there a risk that this might leak private information from the registry to a user using the CLI?
Are there other details about a node that should be added to this feature (e.g. TCP inlets and outlets)?

Example Output

> ockam node create n1
> ockam node create n2
> ockam secure-channel-listener create test --at /node/n1
> ockam tcp-listener create 127.0.0.1:7002 --at /node/n1
> ockam node list

Node:
  Name: n1
  Status: UP
  Route to node:
    Short: /node/n1
    Verbose: /dnsaddr/localhost/tcp/55754
  Identity: Pcf94e075af7c5885ab0eb84abb9a76223f7359f6c64bf753a136ae1b2bb3f02f
  Transports:
    Transport:
      Type: TCP
      Mode: Listening
      Payload: 127.0.0.1:55754
    Transport:
      Type: TCP
      Mode: Listening
      Payload: 127.0.0.1:7002
  Secure Channel Listeners:
    Listener:
      Address: /service/api
    Listener:
      Address: /service/test
  Services:
    Service:
      Type: vault
      Address: /service/vault_service
    Service:
      Type: identity
      Address: /service/identity_service
    Service:
      Type: authenticated
      Address: /service/authenticated
    Service:
      Type: uppercase
      Address: /service/uppercase
    Service:
      Type: echoer
      Address: /service/echo
    Service:
      Type: credentials
      Address: /service/credentials

Node:
  Name: n2
  Status: UP
  Route to node:
    Short: /node/n2
    Verbose: /dnsaddr/localhost/tcp/55761
  Identity: Pcf94e075af7c5885ab0eb84abb9a76223f7359f6c64bf753a136ae1b2bb3f02f
  Transports:
    Transport:
      Type: TCP
      Mode: Listening
      Payload: 127.0.0.1:55761
  Secure Channel Listeners:
    Listener:
      Address: /service/api
  Services:
    Service:
      Type: vault
      Address: /service/vault_service
    Service:
      Type: identity
      Address: /service/identity_service
    Service:
      Type: authenticated
      Address: /service/authenticated
    Service:
      Type: uppercase
      Address: /service/uppercase
    Service:
      Type: echoer
      Address: /service/echo
    Service:
      Type: credentials
      Address: /service/credentials

Checks

All commits in this Pull Request are signed and Verified by Github.
All commits in this Pull Request follow the Ockam commit message convention.
I accept the Ockam Community Code of Conduct.
I have accepted the Ockam Contributor License Agreement by adding my Git/Github details in a row at the end of the CONTRIBUTORS.csv file in a separate pull request to the build-trust/ockam-contributors repository.

mrinalwadhwa

That looks great!
Thank you for another awesome contribution!

mrinalwadhwa · 2022-09-22T05:35:11Z

Is the output formatting of ockam node show okay? Instead of a verbose tree, could we output some of the information in a tabular style like docker image ls?

This is a good question to consider, let's do that in a separate issue. The main thing to discuss would be what information makes sense in the table like output.

In the output, should the Transport field Payload be renamed?

Yes it should be called Address.

The request/response get /node/service builds its response from ockam_api::nodes::service::NodeManager.registry. Is there a risk that this might leak private information from the registry to a user using the CLI?

No, I think.

Are there other details about a node that should be added to this feature (e.g. TCP inlets and outlets)?

Yes. Inlets & Outlets should be there. I'll think about what else.

neil2468 · 2022-09-23T08:08:23Z

Hi,

I'm looking at making some improvements and adding the inlets and outlets.
Sorry for not making the PR draft earlier 🙏

Thanks, Neil.

mrinalwadhwa · 2022-09-23T08:12:55Z

Great, glad you noticed I was about to merge 😬

neil2468 · 2022-09-23T14:38:32Z

Additional Proposed Changes

Inside a NodeManager's registry, InletInfo stores the onward route to the inlet's outlet.
ockam node show lists the details of a node's inlets and outlets.

Queries

I removed the comment // FIXME route.as_ref()... from ockam_api/src/nodes/service/portals.rs. Is this okay? @SanjoDeundiak
I added the function addr_to_multiaddr<T: Into<Address>>(a: T) -> Option<MultiAddr>. As it is public, is its API okay?

New Example Output

> ockam node create n1
> ockam node create n2
> ockam tcp-outlet create --at /node/n1 --from /service/outlet --to 127.0.0.1:5000
> ockam tcp-inlet create --at /node/n2 --from 127.0.0.1:6000 --to /node/n1/service/outlet
> ockam node list


Node:
  Name: n1
  Status: UP
  Route To Node:
    Short: /node/n1
    Verbose: /dnsaddr/localhost/tcp/58434
  Identity: P5f2178c1852c02f602ead6f261133f036b1e4910058cc9c6cdfcddd5605d87cd
  Transports:
    Transport:
      Type: TCP
      Mode: Listening
      Address: 127.0.0.1:58434
  Secure Channel Listeners:
    Listener:
      Address: /service/api
  Inlets:
  Outlets:
    Outlet:
      Forward Address: 127.0.0.1:5000
      Address: /service/outlet
  Services:
    Service:
      Type: vault
      Address: /service/vault_service
    Service:
      Type: identity
      Address: /service/identity_service
    Service:
      Type: authenticated
      Address: /service/authenticated
    Service:
      Type: uppercase
      Address: /service/uppercase
    Service:
      Type: echoer
      Address: /service/echo
    Service:
      Type: credentials
      Address: /service/credentials

Node:
  Name: n2
  Status: UP
  Route To Node:
    Short: /node/n2
    Verbose: /dnsaddr/localhost/tcp/58441
  Identity: P5f2178c1852c02f602ead6f261133f036b1e4910058cc9c6cdfcddd5605d87cd
  Transports:
    Transport:
      Type: TCP
      Mode: Listening
      Address: 127.0.0.1:58441
  Secure Channel Listeners:
    Listener:
      Address: /service/api
  Inlets:
    Inlet:
      Listen Address: 127.0.0.1:6000
      Route To Outlet: /ip4/127.0.0.1/tcp/58434/service/outlet
  Outlets:
  Services:
    Service:
      Type: vault
      Address: /service/vault_service
    Service:
      Type: identity
      Address: /service/identity_service
    Service:
      Type: authenticated
      Address: /service/authenticated
    Service:
      Type: uppercase
      Address: /service/uppercase
    Service:
      Type: echoer
      Address: /service/echo
    Service:
      Type: credentials
      Address: /service/credentials

adrianbenavides

That node output looks pretty great! I left a couple of nits, @SanjoDeundiak feel free to merge it whenever you want!

implementations/rust/ockam/ockam_command/src/node/show.rs

adrianbenavides · 2022-09-30T11:58:29Z

@Mergifyio rebase

mergify · 2022-09-30T11:58:52Z

rebase

✅ Branch has been successfully rebased

adrianbenavides · 2022-10-03T10:13:30Z

@neil2468 looks like your commits are not signed. Could you please force-push your last commit with your signature? Here you can find more details on how to do it: https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits

neil2468 · 2022-10-03T15:19:21Z

@adrianbenavides Done! Thanks.
The Rust Nightly CI fails on my fork and I've noticed the same is happening in the Ockam repo.
Rust stable seems okay 😄.

adrianbenavides · 2022-10-05T05:58:59Z

The Rust Nightly CI fails on my fork and I've noticed the same is happening in the Ockam repo. Rust stable seems okay smile.

Yeah, don't worry about that. We looked into that and it seems a bug on the latest nightly version.

adrianbenavides

LGTM!! 🚀 Thanks again @neil2468, this PR is a great improvement 🙏

adrianbenavides · 2022-10-05T15:54:22Z

Here too @neil2468, although this time looks like it's a simple conflict. I'll merge it as soon as you push your changes!

mrinalwadhwa

Thank you for another awesome contribution! 🙏

neil2468 requested a review from a team as a code owner September 21, 2022 17:25

mrinalwadhwa previously approved these changes Sep 21, 2022

View reviewed changes

mrinalwadhwa added auto-merge and removed auto-merge labels Sep 22, 2022

neil2468 dismissed mrinalwadhwa’s stale review via 1ee02a6 September 22, 2022 07:50

mrinalwadhwa self-requested a review September 23, 2022 08:00

mrinalwadhwa previously approved these changes Sep 23, 2022

View reviewed changes

mrinalwadhwa added the auto-merge label Sep 23, 2022

neil2468 marked this pull request as draft September 23, 2022 08:06

mrinalwadhwa removed the auto-merge label Sep 23, 2022

neil2468 changed the title ~~feat(rust): ockam node show to use dynamic data from node~~ WIP - feat(rust): ockam node show to use dynamic data from node Sep 23, 2022

neil2468 dismissed mrinalwadhwa’s stale review via 0a683a9 September 23, 2022 14:09

neil2468 marked this pull request as ready for review September 23, 2022 14:38

neil2468 changed the title ~~WIP - feat(rust): ockam node show to use dynamic data from node~~ feat(rust): ockam node show to use dynamic data from node Sep 23, 2022

neil2468 requested a review from mrinalwadhwa September 28, 2022 17:46

adrianbenavides reviewed Sep 30, 2022

View reviewed changes

implementations/rust/ockam/ockam_command/src/node/show.rs Show resolved Hide resolved

implementations/rust/ockam/ockam_command/src/node/show.rs Outdated Show resolved Hide resolved

neil2468 requested review from adrianbenavides and removed request for mrinalwadhwa October 4, 2022 18:43

adrianbenavides previously approved these changes Oct 5, 2022

View reviewed changes

neil2468 dismissed adrianbenavides’s stale review via 0a3bd8e October 5, 2022 17:49

feat(rust): ockam node show to use dynamic data from node

0c59b74

mrinalwadhwa requested review from mrinalwadhwa and adrianbenavides October 5, 2022 18:05

mrinalwadhwa approved these changes Oct 5, 2022

View reviewed changes

mrinalwadhwa added the auto-merge label Oct 5, 2022

mergify bot merged commit ea1bd79 into build-trust:develop Oct 5, 2022

adrianbenavides added the HACKTOBERFEST-ACCEPTED To be used when a PR is ready to merge or after it's merged label Oct 6, 2022

neil2468 deleted the neil2468/cli_list_services branch November 1, 2022 11:22

adrianbenavides mentioned this pull request Dec 8, 2022

feat(rust): add dyn_info for dynamic output with `ockam node create… #3223

Closed

mrinalwadhwa added HACKTOBERFEST-ACCEPTED-2022 and removed HACKTOBERFEST-ACCEPTED To be used when a PR is ready to merge or after it's merged labels Sep 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rust): `ockam node show` to use dynamic data from node #3504

feat(rust): `ockam node show` to use dynamic data from node #3504

neil2468 commented Sep 21, 2022 •

edited

Loading

mrinalwadhwa left a comment

mrinalwadhwa commented Sep 22, 2022

neil2468 commented Sep 23, 2022

mrinalwadhwa commented Sep 23, 2022

neil2468 commented Sep 23, 2022

adrianbenavides left a comment

adrianbenavides commented Sep 30, 2022

mergify bot commented Sep 30, 2022

adrianbenavides commented Oct 3, 2022

neil2468 commented Oct 3, 2022

adrianbenavides commented Oct 5, 2022

adrianbenavides left a comment

adrianbenavides commented Oct 5, 2022 •

edited

Loading

mrinalwadhwa left a comment

	fn print_node_info(node_cfg: &NodeConfig, node_name: &str, status: &str, default_id: &str) {
	println!(
	r#"
	Node:
	Name: {}
	Status: {}
	Services:
	Service:
	Type: TCP Listener
	Address: /ip4/127.0.0.1/tcp/{}
	Service:
	Type: Secure Channel Listener
	Address: /service/api
	Route: /ip4/127.0.0.1/tcp/{}/service/api
	Identity: {}
	Authorized Identities:
	- {}
	Service:
	Type: Uppercase
	Address: /service/uppercase
	Service:
	Type: Echo
	Address: /service/echo
	Secure Channel Listener Address: /service/api
	"#,
	node_name,
	match status {
	"UP" => status.light_green(),
	"DOWN" => status.light_red(),
	_ => status.white(),
	},
	node_cfg.port,
	node_cfg.port,
	default_id,
	default_id,
	);
	}

feat(rust): ockam node show to use dynamic data from node #3504

feat(rust): ockam node show to use dynamic data from node #3504

Conversation

neil2468 commented Sep 21, 2022 • edited Loading

Current Behavior

Proposed Changes

Queries

Example Output

Checks

mrinalwadhwa left a comment

Choose a reason for hiding this comment

mrinalwadhwa commented Sep 22, 2022

neil2468 commented Sep 23, 2022

mrinalwadhwa commented Sep 23, 2022

neil2468 commented Sep 23, 2022

Additional Proposed Changes

Queries

New Example Output

adrianbenavides left a comment

Choose a reason for hiding this comment

adrianbenavides commented Sep 30, 2022

mergify bot commented Sep 30, 2022

✅ Branch has been successfully rebased

adrianbenavides commented Oct 3, 2022

neil2468 commented Oct 3, 2022

adrianbenavides commented Oct 5, 2022

adrianbenavides left a comment

Choose a reason for hiding this comment

adrianbenavides commented Oct 5, 2022 • edited Loading

mrinalwadhwa left a comment

Choose a reason for hiding this comment

feat(rust): `ockam node show` to use dynamic data from node #3504

feat(rust): `ockam node show` to use dynamic data from node #3504

neil2468 commented Sep 21, 2022 •

edited

Loading

adrianbenavides commented Oct 5, 2022 •

edited

Loading