Commands for common network operations #726

rcgoodfellow · 2024-07-04T05:44:13Z

We have a transactional API for port settings objects. The port settings object is somewhat complex. The port settings object returned by GET requests to view port settings is not exactly the same as the port settings object used to create/update port settings with PUT/POST requests. This makes it rather grueling to manage port settings.

This PR adds client-side read/modify/write routines for managing individual items within port settings such as addresses.

Also pulls in the observability stuff from #661.

An example of using these new commands is here, which demonstrates creating a new BGP connection to a cloud provider. An example of using the observability stuff pulled in from #661 is in this comment analyzing an Omicron bug.

karencfv · 2024-07-04T05:54:19Z

cli/src/cmd_net.rs

+enum Port {
+    Qsfp0,
+    Qsfp1,
+    Qsfp2,
+    Qsfp3,
+    Qsfp4,
+    Qsfp5,
+    Qsfp6,
+    Qsfp7,
+    Qsfp8,
+    Qsfp9,
+    Qsfp10,
+    Qsfp11,
+    Qsfp12,
+    Qsfp13,
+    Qsfp14,
+    Qsfp15,
+    Qsfp16,
+    Qsfp17,
+    Qsfp18,
+    Qsfp19,
+    Qsfp20,
+    Qsfp21,
+    Qsfp22,
+    Qsfp23,
+    Qsfp24,
+    Qsfp25,
+    Qsfp26,
+    Qsfp27,
+    Qsfp28,
+    Qsfp29,
+    Qsfp30,
+    Qsfp31,
+}


I wonder if this is something that could live in the API? Lists like this could be difficult to maintain the same in various clients 🤔

Additionally, if our customers were to create their own clients from our API, they might find it pretty hard to know that this enum is required

The network configuration API is going to be changing pretty significantly based on some work @internet-diglett is doing to eventually support things like terraform integration. A resource-based model is ultimately where we want to go. This PR is to make things reasonable for operators until that larger chunk of API work lands. It's my hope that at a CLI level, these commands will remain pretty much the same, while their internal implementation will benefit from broader API work, and things like this, which you rightfully note should be captured by the API will go away.

gotcha, thanks for the clarification!

karencfv · 2024-07-04T05:55:37Z

cli/src/cmd_net.rs

+        let links: HashMap<String, LinkConfigCreate> = current
+            .links
+            .iter()
+            .enumerate()
+            .map(|(i, x)| {
+                (
+                    format!("phy{}", i),
+                    LinkConfigCreate {
+                        autoneg: x.autoneg,
+                        fec: x.fec,
+                        lldp: LldpServiceConfigCreate {
+                            //TODO
+                            enabled: false,
+                            lldp_config: None,
+                        },
+                        mtu: x.mtu,
+                        speed: x.speed,
+                    },
+                )
+            })
+            .collect();


same as above

rcgoodfellow · 2024-07-09T06:54:16Z

Now depends on

add API endpoint for BGP announce set modification omicron#6024

ahl

this "net" vs "networking" subcommand seems confusing
I'd suggest we add some testing of these commands
positional arguments are (I think) new to the CLI; I think we should stick with - and -- prefixed arguments for clarity (but if that seems cumbersome for these, let me know--recall we do have completion that is inclusive of arguments)
consider docs on subcommands and internal nodes
I think there a few places we want to use .stream() rather than .send

cli/src/cmd_net.rs

ahl · 2024-07-09T17:26:14Z

cli/src/cmd_net.rs

+}
+
+#[async_trait]
+impl RunnableCmd for CmdNet {


this is the only thing that needs to impl RunnableCmd

All of the subcommands still need the Context object, so I found the interface convenient to reuse. Is there a different approach you would suggest?

cli/src/cmd_net.rs

ahl · 2024-07-09T17:34:29Z

cli/docs/cli.json

      ]
    },
+    {
+      "name": "net",


so we're going to have "networking" and "net" as top-level subcommands? this seems confusing.

net has been removed, and the commands have been integrated into the existing command hierarchy.

ahl · 2024-07-09T17:39:26Z

cli/src/cmd_net.rs

+#[derive(Parser, Debug, Clone)]
+#[command(verbatim_doc_comment)]
+#[command(name = "net bgp status")]
+pub struct CmdBgpStatus {}


now does this differ from system networking bgp status?

The only difference is the output is formatted a bit nicer. Seems like this would fit in better as a flag to system networking bgp status? Something like --output table? This is likely true of several commands in this PR.

ry@masaka:~$ oxide net bgp status switch0 ======= Peer Address Local ASN Remote ASN Session State State Duration 169.254.30.1 65547 64502 Established 1h 5m 33s 942ms 169.254.10.1 65547 64500 Established 1h 5m 38s 910ms switch1 ======= Peer Address Local ASN Remote ASN Session State State Duration 169.254.20.1 65547 64500 Established 1h 5m 38s 855ms 169.254.40.1 65547 64502 Established 1h 5m 33s 888ms

ry@masaka:~$ oxide system networking bgp status [ { "addr": "169.254.30.1", "local_asn": 65547, "remote_asn": 64502, "state": "established", "state_duration_millis": 3943140, "switch": "switch0" }, { "addr": "169.254.10.1", "local_asn": 65547, "remote_asn": 64500, "state": "established", "state_duration_millis": 3948107, "switch": "switch0" }, { "addr": "169.254.20.1", "local_asn": 65547, "remote_asn": 64500, "state": "established", "state_duration_millis": 3948072, "switch": "switch1" }, { "addr": "169.254.40.1", "local_asn": 65547, "remote_asn": 64502, "state": "established", "state_duration_millis": 3943104, "switch": "switch1" } ]

For the moment, opting to have show commands that display information in a more human-oriented way than their status and view counterparts.

ahl · 2024-07-09T18:20:39Z

cli/src/cmd_net.rs

+            .body(BgpAnnounceSetCreate {
+                announcement: current,
+                name: self.announce_set.clone(),
+                description: self.announce_set.to_string(), //TODO?


is this the description we want?

I've made this configurable

cli/src/cmd_net.rs

ahl · 2024-07-09T18:44:07Z

cli/src/cmd_net.rs

+#[derive(Serialize, Deserialize, Clone, Debug)]
+pub struct MacAddr {
+    a: [u8; 6],
+}
+
+#[derive(Serialize, Deserialize, Clone, Debug)]
+struct LinkStatus {
+    address: MacAddr,
+    enabled: bool,
+    autoneg: bool,
+    fec: String,
+    link_state: String,
+    fsm_state: String,
+    media: String,
+    speed: String,
+}
+


how fragile is this?

Quite. The link status endpoint is experimental and opaque, passing just Object back. We could, in theory, import types from Dendrite, which is where these types ultimately come from, but that repo is not open, so there are challenges on that front.

cli/src/cmd_net.rs

rcgoodfellow · 2024-07-11T04:15:03Z

For testing, I've added an expectorate test for the one command that produces stable output: oxide system networking switch-port-settings show. The other commands are either strictly modifiers and don't return any output or are hooked up to an API endpoint that is not stable yet and returns an Object, making strongly typed testing a bit of a challenge and perhaps not worthwhile until the interface stabilizes in the API.

ahl

great stuff!

ahl · 2024-07-11T04:34:24Z

cli/src/cmd_net.rs

+        SwitchInterfaceConfigCreate, SwitchInterfaceKind, SwitchInterfaceKind2, SwitchLocation,
+        SwitchPort, SwitchPortConfigCreate, SwitchPortGeometry, SwitchPortGeometry2,


it's concerning to me that we've managed to have these types suffixed with 2 which indicates a name conflict, but that's a problem for another day. I should probably have some sort of lint check for that.

ahl · 2024-07-11T04:37:48Z

cli/src/cmd_net.rs

+#[derive(Parser, Debug, Clone)]
+#[command(verbatim_doc_comment)]
+#[command(name = "port")]
+pub struct CmdPort {
+    #[clap(subcommand)]
+    subcmd: PortSubCommand,
+}
+
+#[async_trait]
+impl RunnableCmd for CmdPort {
+    async fn run(&self, ctx: &Context) -> Result<()> {
+        match &self.subcmd {
+            PortSubCommand::Config(cmd) => cmd.run(ctx).await,
+            PortSubCommand::Status(cmd) => cmd.run(ctx).await,
+        }
+    }
+}
+
+#[derive(Parser, Debug, Clone)]
+enum PortSubCommand {
+    /// Manage port configuration.
+    Config(CmdPortConfig),
+
+    /// Observe port status.
+    Status(CmdPortStatus),
+}


can this all be deleted?

yep sure can! removed

ahl · 2024-07-11T04:43:36Z

cli/src/cmd_net.rs

+    Status(CmdPortStatus),
+}
+
+#[derive(Parser, Debug, Clone)]


I don't claim to really understand what this does, but perhaps a short doc comment explaining very briefly the command and (probably) that the subcommands are short-hand for common operations one could perform via system networking switch-port-settings create; if it's easy enough, I think a short doc comment on all the new commands and their subcommands would be great.

ahl · 2024-07-11T04:49:04Z

cli/src/cmd_net.rs

+    /// Prefixes to allow for the peer.
+    #[clap(long, conflicts_with = "no_filtering")]
+    allowed: Vec<oxnet::IpNet>,
+
+    /// Do not filter.
+    #[clap(long, conflicts_with = "allowed")]
+    no_filtering: bool,


rcgoodfellow mentioned this pull request Jul 4, 2024

A few custom commands for looking at the network #661

Closed

karencfv reviewed Jul 4, 2024

View reviewed changes

rcgoodfellow mentioned this pull request Jul 4, 2024

port settings update as transaction oxidecomputer/omicron#6001

Merged

rcgoodfellow marked this pull request as ready for review July 6, 2024 00:15

rcgoodfellow added 7 commits July 8, 2024 23:04

wip basic commands to do basic things

229ea6d

+ addres delete command, feed clippy, fix tests

fe1b6d1

bgp peer add/remove

7acf06a

link add/del commands

ed0bb5e

cleanup

21789dd

wip announce set

fc6b16c

announce set management commands

7f3fecb

rcgoodfellow force-pushed the port-settings-update branch from 61b3892 to 7f3fecb Compare July 9, 2024 06:53

add command for managing bgp peer filtering

236c2b1

ahl reviewed Jul 9, 2024

View reviewed changes

wip on feedback

c0ebc21

morlandi7 added this to the 9 milestone Jul 10, 2024

rcgoodfellow added 2 commits July 10, 2024 17:09

move commands from net into existing structure

e04e7cf

add expectorate test for switch-port-settings show

908c53b

use openapi spec from omicron main branch

1f56a7e

ahl approved these changes Jul 11, 2024

View reviewed changes

add doc comments, remove dead code

38e487d

rcgoodfellow merged commit 18d1196 into main Jul 11, 2024

rcgoodfellow deleted the port-settings-update branch July 11, 2024 05:54

		SwitchInterfaceConfigCreate, SwitchInterfaceKind, SwitchInterfaceKind2, SwitchLocation,
		SwitchPort, SwitchPortConfigCreate, SwitchPortGeometry, SwitchPortGeometry2,

Commands for common network operations #726

Commands for common network operations #726

Uh oh!

Conversation

rcgoodfellow commented Jul 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rcgoodfellow commented Jul 9, 2024

Uh oh!

ahl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rcgoodfellow commented Jul 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rcgoodfellow commented Jul 4, 2024 •

edited

Loading

rcgoodfellow commented Jul 11, 2024 •

edited

Loading