-
Notifications
You must be signed in to change notification settings - Fork 4
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
Cli binary and protocol with dataplane #376
base: main
Are you sure you want to change the base?
Conversation
045f5d7
to
6e4823f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've glanced over the changes, although I haven't looked at each chunk in details. It looks good, no red flags, great work! It's self-contained in its own crate anyway so low risk to break something when we merge it.
Would be great to have more tests, but a number of them will likely require starting the datapath and run some sort of end-to-end test, which we don't really have any infra for, yet, so we can probably keep that for later.
I'll try to build and run this CLI tomorrow to get a better understanding on the features.
//if a node has no name, skip it and adopt | ||
//its children. This is just to ease the | ||
//creation of the tree by creating | ||
//unnamed commands that will not show up |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
//if a node has no name, skip it and adopt | |
//its children. This is just to ease the | |
//creation of the tree by creating | |
//unnamed commands that will not show up | |
// If a node has no name, skip it and adopt | |
// its children. This is just to ease the | |
// creation of the tree by creating | |
// unnamed commands that will not show up. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd leave it as it is. Note that this is not a doc, it's just an internal comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't have to be doc comments, we do usually leave a space after the //
marker - at least I can't find any other comment instance without that space 🤷. Your call, though.
cli/src/cliproto.rs
Outdated
enum_from_primitive! { | ||
#[allow(unused)] | ||
#[derive(Debug, Clone, Serialize, Deserialize)] | ||
|
||
pub enum CliAction { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing indent
@@ -1,6 +1,6 @@ | |||
[workspace] | |||
|
|||
members = [ | |||
members = [ "cli", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I note you added cli
to members
but not to default-members
, which means tests won't be exercised on a simple cargo test
from the root of the repository. Is this Probably something we should change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are not tests at the moment in the CLI
Commit suggestion: exiting on CTRL-D or CTRL-C: diff --git i/cli/src/bin/cli.rs w/cli/src/bin/cli.rs
index 4d471b51782a..ace08396beb6 100644
--- i/cli/src/bin/cli.rs
+++ w/cli/src/bin/cli.rs
@@ -207,7 +207,13 @@ fn main() {
/* infinite loop until user quits */
while terminal.runs() {
- let mut input = terminal.prompt();
+ let mut input = match terminal.prompt() {
+ Some(i) => i,
+ None => {
+ terminal.stop();
+ return;
+ }
+ };
if let Some(node) = cmds.find_best(input.get_tokens()) {
terminal.add_history_entry(input.get_line().to_owned());
if let Some(action) = &node.action {
diff --git i/cli/src/terminal.rs w/cli/src/terminal.rs
index dbc2a0d14a4c..2358b423c72e 100644
--- i/cli/src/terminal.rs
+++ w/cli/src/terminal.rs
@@ -5,6 +5,7 @@
use crate::cmdtree::Node;
use rustyline::config::{ColorMode, CompletionType, Config};
+use rustyline::error::ReadlineError;
use rustyline::{Cmd, Event, KeyCode, KeyEvent, Modifiers};
use smallvec::SmallVec;
use std::collections::VecDeque;
@@ -122,19 +123,34 @@ impl Terminal {
self.prompt = self.prompt_name.to_owned() + "(✖)# ";
}
}
- pub fn prompt(&mut self) -> TermInput {
+ pub fn prompt(&mut self) -> Option<TermInput> {
loop {
let input = self.editor.readline(&self.prompt);
- if let Ok(line) = input {
- let line = line.trim();
- if line.is_empty() {
- continue;
+ match (input) {
+ Ok(line) => {
+ let line = line.trim();
+ if line.is_empty() {
+ continue;
+ }
+ if let Some(c) = self.proc_line(line) {
+ return Some(c);
+ }
}
- if let Some(c) = self.proc_line(line) {
- return c;
+ Err(ReadlineError::Interrupted) => {
+ println!("CTRL-C");
+ break;
+ },
+ Err(ReadlineError::Eof) => {
+ println!("CTRL-D");
+ break;
+ },
+ Err(err) => {
+ println!("Error: {:?}", err);
+ break;
}
}
}
+ None
}
pub fn connected(&mut self, value: bool) {
self.connected = value; |
19bf93b
to
f8fbdd2
Compare
I wanted to highlight distinct facts in the output, but I believe I did not finish it. Let me redo. |
Sorry, I should have mentioned this. There are some "aliases" or shortcuts, that do not show up in the help. If you want to quit, you can type "quit" or "exit" (these are shown), but you can also use "q". |
6e4823f
to
c335c99
Compare
I found |
e264f42
to
c2979e1
Compare
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
The cmdtree (command tree) defines a tree of nodes (type Node) where each node can represent a command of the CLI. Every node has a name, optional description and can have an action associated to it. Some nodes exist simply to keep other nodes; its children. The action corresponding to a node is encoded as a u16 for generality. This module is independent of the types of commands that the cli will support, and includes tooling to build a specific tree of commands and specify their arguments. This tree representation is only useful for the cli itself, in order to implement command completion and help parsing user input. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Adds definitions for the protocol between the cli and the dataplane and defines action codes. The protocol consists of requests and responses, where a request consists of an action code plus arguments and the response piggybacks the request plus text. The latter is to be replaced by objects/collections in the dataplane state. Also, add log crate since tracing does not implement Serialize / Deserialize for Level, which is needed to dynamically set the log level in the dataplane. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
The completions use the cmdtree (Node) to offer candidates for completion and do not depend, therefore, on the specific protocol with the dataplane. Command completion leverages the rustyline crate. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
The feature is only needed by the cli binary. The only submodule from this crate needed by the dataplane is the cliproto. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Even if the cli may do little with such a result code, this may ease the implementation of cli request handlers since they may piggyback any error encountered when handling the request in the response. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
This allows setting the bind address to whatever is convenient, overriding the default. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
This patch: - removes the clap dependency, which was only meant to allow setting the address to locally bind to. With this code, the local address can optionally be set as argument to connect. - makes the terminal own the unix socket. - correctly implements connect and disconnect. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
In previous commits, RequestArgs included all args, whether the action they applied to was local or remote. This meant that all args (e.g. connect path) were included and sent in CliRequest to the remote. This patch defines CliArgs as the main struct were arguments are stored, which embeds a RequestArgs (for args for remote actions), which are the only ones that get sent to the remote. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
...so that, for example, we need only type show ip route instead of show routing ip route Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Internally, VRFs may be identified by a numerical vrf id. Therefore, using the id instead of the name shall make the search in the dataplane faster and easier. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
There's no reason to have two commands one for ipv4 and one for ipv6, as usually one wants to see all addresses on a particular interface. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
This illustrates the procedure to add a new command: 1) define a new kind in the CliAction enum, which is u16. 2) add nodes to the command tree, specifying the action value and optional argument. In this example, we use "address" which for which the argument parser already knows how to translate. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Adds option "protocol" to commands to display routes so that only certain routes are shown. This is extremely useful for troubleshooting large routing tables. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
The original completion code did not contemplate the fact that a node could have both children and args. This provides a quick fix but the result is not ideal. Will fix in separate PR. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
... or lines starting with #. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
fc63a7b
to
da545cc
Compare
Most are dependencies of rustyline. Even after updating to the latest version of rustyline, we get dups. Since these are only used for the cli binary, it is safe to except them. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
da545cc
to
6e506ef
Compare
NB: rides on #363
Build
Once in cli
... shows the available commands. Many are not implemented.
Give it a try!
The cli can be started to see the syntax of the commands, even if the dataplane does not handle them.
The command completion requires some re-work.