[RSS] Change uplink_ip to uplink_cidr in rack network config (#3755)

jgallagher · web-flow · commit bf69cf63f65a · 2023-07-25T09:49:43.000-04:00
This is necessary (but not sufficient) to address #3588 - tfport needs the netmask/prefix for each uplink, which means we need to collect it at RSS time. Most of this PR is various openapi plumbing changes. There are a couple spots that warrant a closer look (although they are small); I'll call them out in comments below.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/Cargo.toml b/Cargo.toml
@@ -206,7 +206,7 @@ installinator-artifact-client = { path = "installinator-artifact-client" }
 installinator-common = { path = "installinator-common" }
 internal-dns = { path = "internal-dns" }
 ipcc-key-value = { path = "ipcc-key-value" }
-ipnetwork = "0.20"
+ipnetwork = { version = "0.20", features = ["schemars"] }
 itertools = "0.10.5"
 key-manager = { path = "key-manager" }
 lazy_static = "1.4.0"
diff --git a/bootstrap-agent-client/src/lib.rs b/bootstrap-agent-client/src/lib.rs
@@ -18,6 +18,9 @@ progenitor::generate_api!(
         slog::debug!(log, "client response"; "result" => ?result);
     }),
     derives = [schemars::JsonSchema],
+    replace = {
+        Ipv4Network = ipnetwork::Ipv4Network,
+    }
 );
 
 impl omicron_common::api::external::ClientError for types::Error {
diff --git a/common/src/api/internal/shared.rs b/common/src/api/internal/shared.rs
@@ -5,6 +5,7 @@
 //! Types shared between Nexus and Sled Agent.
 
 use crate::api::external::{self, Name};
+use ipnetwork::Ipv4Network;
 use schemars::JsonSchema;
 use serde::{Deserialize, Serialize};
 use std::{
@@ -91,8 +92,9 @@ pub struct UplinkConfig {
     pub uplink_port_speed: PortSpeed,
     /// Forward Error Correction setting for the uplink port
     pub uplink_port_fec: PortFec,
-    /// IP Address to apply to switchport (must be in infra_ip pool)
-    pub uplink_ip: Ipv4Addr,
+    /// IP Address and prefix (e.g., `192.168.0.1/16`) to apply to switchport
+    /// (must be in infra_ip pool)
+    pub uplink_cidr: Ipv4Network,
     /// VLAN id to use for uplink
     pub uplink_vid: Option<u16>,
 }
diff --git a/nexus-client/Cargo.toml b/nexus-client/Cargo.toml
@@ -7,6 +7,7 @@ license = "MPL-2.0"
 [dependencies]
 chrono.workspace = true
 futures.workspace = true
+ipnetwork.workspace = true
 omicron-common.workspace = true
 omicron-passwords.workspace = true
 progenitor.workspace = true
diff --git a/nexus-client/src/lib.rs b/nexus-client/src/lib.rs
@@ -22,6 +22,7 @@ progenitor::generate_api!(
         slog::debug!(log, "client response"; "result" => ?result);
     }),
     replace = {
+        Ipv4Network = ipnetwork::Ipv4Network,
         MacAddr = omicron_common::api::external::MacAddr,
         Name = omicron_common::api::external::Name,
         NewPasswordHash = omicron_passwords::NewPasswordHash,
diff --git a/nexus/src/app/rack.rs b/nexus/src/app/rack.rs
@@ -34,6 +34,7 @@ use omicron_common::api::external::DataPageParams;
 use omicron_common::api::external::Error;
 use omicron_common::api::external::IdentityMetadataCreateParams;
 use omicron_common::api::external::IpNet;
+use omicron_common::api::external::Ipv4Net;
 use omicron_common::api::external::ListResultVec;
 use omicron_common::api::external::LookupResult;
 use omicron_common::api::external::Name;
@@ -458,11 +459,8 @@ impl super::Nexus {
                     addresses: HashMap::new(),
                 };
 
-                let uplink_address = IpNet::from_str(&format!("{}/32", uplink_config.uplink_ip))
-                    .map_err(|e| Error::internal_error(&format!(
-                    "failed to parse value provided for `rack_network_config.uplink_ip` as `IpNet`: {e}"
-                )))?;
-
+                let uplink_address =
+                    IpNet::V4(Ipv4Net(uplink_config.uplink_cidr));
                 let address = Address {
                     address_lot: NameOrId::Name(address_lot_name.clone()),
                     address: uplink_address,
diff --git a/openapi/bootstrap-agent.json b/openapi/bootstrap-agent.json
@@ -353,6 +353,10 @@
           }
         ]
       },
+      "Ipv4Network": {
+        "type": "string",
+        "pattern": "^((25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\\.){3}(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\\/(3[0-2]|[0-2]?[0-9])$"
+      },
       "Ipv4Range": {
         "description": "A non-decreasing IPv4 address range, inclusive of both ends.\n\nThe first address must be less than or equal to the last address.",
         "type": "object",
@@ -782,10 +786,13 @@
               }
             ]
           },
-          "uplink_ip": {
-            "description": "IP Address to apply to switchport (must be in infra_ip pool)",
-            "type": "string",
-            "format": "ipv4"
+          "uplink_cidr": {
+            "description": "IP Address and prefix (e.g., `192.168.0.1/16`) to apply to switchport (must be in infra_ip pool)",
+            "allOf": [
+              {
+                "$ref": "#/components/schemas/Ipv4Network"
+              }
+            ]
           },
           "uplink_port": {
             "description": "Switchport to use for external connectivity",
@@ -818,7 +825,7 @@
         "required": [
           "gateway_ip",
           "switch",
-          "uplink_ip",
+          "uplink_cidr",
           "uplink_port",
           "uplink_port_fec",
           "uplink_port_speed"
diff --git a/openapi/nexus-internal.json b/openapi/nexus-internal.json
@@ -2040,6 +2040,10 @@
           }
         ]
       },
+      "Ipv4Network": {
+        "type": "string",
+        "pattern": "^((25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\\.){3}(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\\/(3[0-2]|[0-2]?[0-9])$"
+      },
       "Ipv4Range": {
         "description": "A non-decreasing IPv4 address range, inclusive of both ends.\n\nThe first address must be less than or equal to the last address.",
         "type": "object",
@@ -3227,10 +3231,13 @@
               }
             ]
           },
-          "uplink_ip": {
-            "description": "IP Address to apply to switchport (must be in infra_ip pool)",
-            "type": "string",
-            "format": "ipv4"
+          "uplink_cidr": {
+            "description": "IP Address and prefix (e.g., `192.168.0.1/16`) to apply to switchport (must be in infra_ip pool)",
+            "allOf": [
+              {
+                "$ref": "#/components/schemas/Ipv4Network"
+              }
+            ]
           },
           "uplink_port": {
             "description": "Switchport to use for external connectivity",
@@ -3263,7 +3270,7 @@
         "required": [
           "gateway_ip",
           "switch",
-          "uplink_ip",
+          "uplink_cidr",
           "uplink_port",
           "uplink_port_fec",
           "uplink_port_speed"
diff --git a/openapi/wicketd.json b/openapi/wicketd.json
@@ -1167,6 +1167,10 @@
           }
         ]
       },
+      "Ipv4Network": {
+        "type": "string",
+        "pattern": "^((25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\\.){3}(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\\/(3[0-2]|[0-2]?[0-9])$"
+      },
       "Ipv4Range": {
         "description": "A non-decreasing IPv4 address range, inclusive of both ends.\n\nThe first address must be less than or equal to the last address.",
         "type": "object",
@@ -4213,10 +4217,13 @@
               }
             ]
           },
-          "uplink_ip": {
-            "description": "IP Address to apply to switchport (must be in infra_ip pool)",
-            "type": "string",
-            "format": "ipv4"
+          "uplink_cidr": {
+            "description": "IP Address and prefix (e.g., `192.168.0.1/16`) to apply to switchport (must be in infra_ip pool)",
+            "allOf": [
+              {
+                "$ref": "#/components/schemas/Ipv4Network"
+              }
+            ]
           },
           "uplink_port": {
             "description": "Switchport to use for external connectivity",
@@ -4249,7 +4256,7 @@
         "required": [
           "gateway_ip",
           "switch",
-          "uplink_ip",
+          "uplink_cidr",
           "uplink_port",
           "uplink_port_fec",
           "uplink_port_speed"
diff --git a/schema/rss-sled-plan.json b/schema/rss-sled-plan.json
@@ -169,6 +169,11 @@
         }
       ]
     },
+    "Ipv4Network": {
+      "type": "string",
+      "pattern": "^((25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\\.){3}(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\\/(3[0-2]|[0-2]?[0-9])$",
+      "x-rust-type": "ipnetwork::Ipv4Network"
+    },
     "Ipv4Range": {
       "description": "A non-decreasing IPv4 address range, inclusive of both ends.\n\nThe first address must be less than or equal to the last address.",
       "type": "object",
@@ -484,7 +489,7 @@
       "required": [
         "gateway_ip",
         "switch",
-        "uplink_ip",
+        "uplink_cidr",
         "uplink_port",
         "uplink_port_fec",
         "uplink_port_speed"
@@ -503,10 +508,13 @@
             }
           ]
         },
-        "uplink_ip": {
-          "description": "IP Address to apply to switchport (must be in infra_ip pool)",
-          "type": "string",
-          "format": "ipv4"
+        "uplink_cidr": {
+          "description": "IP Address and prefix (e.g., `192.168.0.1/16`) to apply to switchport (must be in infra_ip pool)",
+          "allOf": [
+            {
+              "$ref": "#/definitions/Ipv4Network"
+            }
+          ]
         },
         "uplink_port": {
           "description": "Switchport to use for external connectivity",
diff --git a/sled-agent/src/rack_setup/service.rs b/sled-agent/src/rack_setup/service.rs
@@ -610,7 +610,7 @@ impl ServiceInner {
                         .map(|config| NexusTypes::UplinkConfig {
                             gateway_ip: config.gateway_ip,
                             switch: config.switch.clone().into(),
-                            uplink_ip: config.uplink_ip,
+                            uplink_cidr: config.uplink_cidr,
                             uplink_port: config.uplink_port.clone(),
                             uplink_port_speed: config
                                 .uplink_port_speed
@@ -1188,7 +1188,10 @@ impl ServiceInner {
             v6_routes: HashMap::new(),
         };
         let link_id = LinkId(0);
-        let addr = IpAddr::V4(uplink_config.uplink_ip);
+        // TODO We're discarding the `uplink_cidr.prefix()` here and only using
+        // the IP address; this needs to be fixed as part of
+        // https://github.com/oxidecomputer/omicron/issues/3588.
+        let addr = IpAddr::V4(uplink_config.uplink_cidr.ip());
         let link_settings = LinkSettings {
             // TODO Allow user to configure link properties
             // https://github.com/oxidecomputer/omicron/issues/3061
diff --git a/smf/sled-agent/gimlet-standalone/config-rss.toml b/smf/sled-agent/gimlet-standalone/config-rss.toml
@@ -104,7 +104,7 @@ uplink_port_speed = "40G"
 uplink_port_fec="none"
 # For softnpu, an address within the "infra" block above that will be used for
 # the softnpu uplink port.  You can just pick the first address in that pool.
-uplink_ip = "192.168.1.30"
+uplink_cidr = "192.168.1.30/32"
 # Switch to use for the uplink. For single-rack deployments this can be
 # "switch0" (upper slot) or "switch1" (lower slot). For single-node softnpu
 # and dendrite stub environments, use "switch0"
diff --git a/smf/sled-agent/non-gimlet/config-rss.toml b/smf/sled-agent/non-gimlet/config-rss.toml
@@ -104,7 +104,7 @@ uplink_port_speed = "40G"
 uplink_port_fec="none"
 # For softnpu, an address within the "infra" block above that will be used for
 # the softnpu uplink port.  You can just pick the first address in that pool.
-uplink_ip = "192.168.1.30"
+uplink_cidr = "192.168.1.30/32"
 # Switch to use for the uplink. For single-rack deployments this can be
 # "switch0" (upper slot) or "switch1" (lower slot). For single-node softnpu
 # and dendrite stub environments, use "switch0"
diff --git a/wicket/src/rack_setup/config_template.toml b/wicket/src/rack_setup/config_template.toml
@@ -44,10 +44,23 @@ infra_ip_first = ""
 infra_ip_last = ""
 
 [[rack_network_config.uplinks]]
+# Either `switch0` or `switch1`, matching the hardware.
 switch = ""
+
+# IP address this uplink should use as its gateway.
 gateway_ip = ""
+
+# qsfp0, qsfp1, ...
 uplink_port = ""
+
+# `speed40_g`, `speed100_g`, ...
 uplink_port_speed = ""
+
+# `none`, `firecode`, or `rs`
 uplink_port_fec = ""
-uplink_ip = ""
-uplink_vid = 1234 # optional - remove if no VLAN id is needed
+
+# IP address and prefix for this uplink; e.g., `192.168.100.100/16`
+uplink_cidr = ""
+
+# VLAN ID for this uplink; omit if no VLAN ID is needed
+uplink_vid = 1234
diff --git a/wicket/src/rack_setup/config_toml.rs b/wicket/src/rack_setup/config_toml.rs
@@ -224,7 +224,7 @@ fn populate_network_table(
                             "uplink_port_fec",
                             enum_to_toml_string(&cfg.uplink_port_fec),
                         ),
-                        ("uplink_ip", cfg.uplink_ip.to_string()),
+                        ("uplink_cidr", cfg.uplink_cidr.to_string()),
                     ] {
                         uplink.insert(
                             property,
@@ -265,7 +265,6 @@ fn populate_network_table(
 mod tests {
     use super::*;
     use omicron_common::api::internal::shared::RackNetworkConfig as InternalRackNetworkConfig;
-    use std::net::Ipv4Addr;
     use std::net::Ipv6Addr;
     use wicket_common::rack_setup::PutRssUserConfigInsensitive;
     use wicketd_client::types::Baseboard;
@@ -341,7 +340,7 @@ mod tests {
                             PortFec::None => InternalPortFec::None,
                             PortFec::Rs => InternalPortFec::Rs,
                         },
-                        uplink_ip: config.uplink_ip,
+                        uplink_cidr: config.uplink_cidr,
                         uplink_vid: config.uplink_vid,
                         switch: match config.switch {
                             SwitchLocation::Switch0 => {
@@ -394,11 +393,11 @@ mod tests {
             external_dns_ips: vec!["10.0.0.1".parse().unwrap()],
             ntp_servers: vec!["ntp1.com".into(), "ntp2.com".into()],
             rack_network_config: Some(RackNetworkConfig {
-                infra_ip_first: Ipv4Addr::new(2, 3, 4, 5),
-                infra_ip_last: Ipv4Addr::new(3, 4, 5, 6),
+                infra_ip_first: "172.30.0.1".parse().unwrap(),
+                infra_ip_last: "172.30.0.10".parse().unwrap(),
                 uplinks: vec![UplinkConfig {
-                    gateway_ip: Ipv4Addr::new(1, 2, 3, 4),
-                    uplink_ip: Ipv4Addr::new(4, 5, 6, 7),
+                    gateway_ip: "172.30.0.10".parse().unwrap(),
+                    uplink_cidr: "172.30.0.1/24".parse().unwrap(),
                     uplink_port_speed: PortSpeed::Speed400G,
                     uplink_port_fec: PortFec::Firecode,
                     uplink_port: "port0".into(),
diff --git a/wicket/src/ui/panes/rack_setup.rs b/wicket/src/ui/panes/rack_setup.rs
@@ -755,8 +755,8 @@ fn rss_config_text<'a>(
                     Span::styled(uplink.gateway_ip.to_string(), ok_style),
                 ],
                 vec![
-                    Span::styled("  • Uplink IP        : ", label_style),
-                    Span::styled(uplink.uplink_ip.to_string(), ok_style),
+                    Span::styled("  • Uplink CIDR        : ", label_style),
+                    Span::styled(uplink.uplink_cidr.to_string(), ok_style),
                 ],
                 vec![
                     Span::styled("  • Uplink port      : ", label_style),
diff --git a/wicketd-client/Cargo.toml b/wicketd-client/Cargo.toml
@@ -7,6 +7,7 @@ license = "MPL-2.0"
 [dependencies]
 chrono.workspace = true
 installinator-common.workspace = true
+ipnetwork.workspace = true
 progenitor.workspace = true
 regress.workspace = true
 reqwest = { workspace = true, features = ["rustls-tls", "stream"] }
diff --git a/wicketd-client/src/lib.rs b/wicketd-client/src/lib.rs
@@ -50,6 +50,7 @@ progenitor::generate_api!(
     },
     replace = {
         Duration = std::time::Duration,
+        Ipv4Network = ipnetwork::Ipv4Network,
         PutRssUserConfigInsensitive = wicket_common::rack_setup::PutRssUserConfigInsensitive,
         EventReportForWicketdEngineSpec = wicket_common::update_events::EventReport,
         StepEventForWicketdEngineSpec = wicket_common::update_events::StepEvent,
diff --git a/wicketd/src/rss_config.rs b/wicketd/src/rss_config.rs
