Use Ipv4Addr instead of String in RackNetworkConfig IP address fields (#3399)

I believe based on current usage that all of these are required to be
`Ipv4Addr`s specifically, and not `IpAddr`/`Ipv6Addr`:

* `infra_ip_first` and `infra_ip_last` are stored in a variable named
`ipv4_block`, and a later `ipv6_block` (that does not use those config
values) is also created
* `uplink_ip`'s doc comment says it should be in the
`infra_ip_first..last` range, which means it would also have to be ipv4
* `gateway_ip` was being parsed into `nexthop: Option<Ipv4Addr>`.

but I don't have a lot of context here so would appreciate confirmation.

Fixes #3397.

Author: jgallagher

common/src/api/internal/shared.rs

@@ -7,7 +7,7 @@
 use crate::api::external;
 use schemars::JsonSchema;
 use serde::{Deserialize, Serialize};
-use std::net::IpAddr;
+use std::net::{IpAddr, Ipv4Addr};
 use uuid::Uuid;
 
 /// The type of network interface
@@ -66,19 +66,19 @@ pub struct SourceNatConfig {
 #[derive(Clone, Debug, Deserialize, Serialize, PartialEq, JsonSchema)]
 pub struct RackNetworkConfig {
     /// Gateway address
-    pub gateway_ip: String,
+    pub gateway_ip: Ipv4Addr,
     /// First ip address to be used for configuring network infrastructure
-    pub infra_ip_first: String,
+    pub infra_ip_first: Ipv4Addr,
     /// Last ip address to be used for configuring network infrastructure
-    pub infra_ip_last: String,
+    pub infra_ip_last: Ipv4Addr,
     /// Switchport to use for external connectivity
     pub uplink_port: String,
     /// Speed for the Switchport
     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: String,
+    pub uplink_ip: Ipv4Addr,
     /// VLAN id to use for uplink
     pub uplink_vid: Option<u16>,
 }

nexus/src/app/rack.rs

@@ -263,24 +263,8 @@ impl super::Nexus {
 
             let kind = AddressLotKind::Infra;
 
-            let first_address = IpAddr::from_str(
-                &rack_network_config.infra_ip_first,
-            )
-            .map_err(|e| {
-                Error::internal_error(&format!(
-                    "encountered error while parsing `infra_ip_first`: {e}"
-                ))
-            })?;
-
-            let last_address = IpAddr::from_str(
-                &rack_network_config.infra_ip_last,
-            )
-            .map_err(|e| {
-                Error::internal_error(&format!(
-                    "encountered error while parsing `infra_ip_last`: {e}"
-                ))
-            })?;
-
+            let first_address = IpAddr::V4(rack_network_config.infra_ip_first);
+            let last_address = IpAddr::V4(rack_network_config.infra_ip_last);
             let ipv4_block =
                 AddressLotBlockCreate { first_address, last_address };
 
@@ -431,15 +415,8 @@ impl super::Nexus {
                 ))
             })?;
 
-            let gw = IpAddr::from_str(&rack_network_config.gateway_ip)
-                .map_err(|e| {
-                    Error::internal_error(&format!(
-                        "failed to parse provided default gateway address: {e}"
-                    ))
-                })?;
-
+            let gw = IpAddr::V4(rack_network_config.gateway_ip);
             let vid = rack_network_config.uplink_vid;
-
             let route = Route { dst, gw, vid };
 
             port_settings_params.routes.insert(

openapi/bootstrap-agent.json

@@ -520,19 +520,23 @@
         "properties": {
           "gateway_ip": {
             "description": "Gateway address",
-            "type": "string"
+            "type": "string",
+            "format": "ipv4"
           },
           "infra_ip_first": {
             "description": "First ip address to be used for configuring network infrastructure",
-            "type": "string"
+            "type": "string",
+            "format": "ipv4"
           },
           "infra_ip_last": {
             "description": "Last ip address to be used for configuring network infrastructure",
-            "type": "string"
+            "type": "string",
+            "format": "ipv4"
           },
           "uplink_ip": {
             "description": "IP Address to apply to switchport (must be in infra_ip pool)",
-            "type": "string"
+            "type": "string",
+            "format": "ipv4"
           },
           "uplink_port": {
             "description": "Switchport to use for external connectivity",

openapi/nexus-internal.json

@@ -2455,19 +2455,23 @@
         "properties": {
           "gateway_ip": {
             "description": "Gateway address",
-            "type": "string"
+            "type": "string",
+            "format": "ipv4"
           },
           "infra_ip_first": {
             "description": "First ip address to be used for configuring network infrastructure",
-            "type": "string"
+            "type": "string",
+            "format": "ipv4"
           },
           "infra_ip_last": {
             "description": "Last ip address to be used for configuring network infrastructure",
-            "type": "string"
+            "type": "string",
+            "format": "ipv4"
           },
           "uplink_ip": {
             "description": "IP Address to apply to switchport (must be in infra_ip pool)",
-            "type": "string"
+            "type": "string",
+            "format": "ipv4"
           },
           "uplink_port": {
             "description": "Switchport to use for external connectivity",

openapi/wicketd.json

@@ -1771,19 +1771,23 @@
         "properties": {
           "gateway_ip": {
             "description": "Gateway address",
-            "type": "string"
+            "type": "string",
+            "format": "ipv4"
           },
           "infra_ip_first": {
             "description": "First ip address to be used for configuring network infrastructure",
-            "type": "string"
+            "type": "string",
+            "format": "ipv4"
           },
           "infra_ip_last": {
             "description": "Last ip address to be used for configuring network infrastructure",
-            "type": "string"
+            "type": "string",
+            "format": "ipv4"
           },
           "uplink_ip": {
             "description": "IP Address to apply to switchport (must be in infra_ip pool)",
-            "type": "string"
+            "type": "string",
+            "format": "ipv4"
           },
           "uplink_port": {
             "description": "Switchport to use for external connectivity",

sled-agent/src/rack_setup/service.rs

@@ -102,7 +102,6 @@ use slog::Logger;
 use std::collections::{HashMap, HashSet};
 use std::iter;
 use std::net::IpAddr;
-use std::net::Ipv4Addr;
 use std::net::{Ipv6Addr, SocketAddr, SocketAddrV6};
 use thiserror::Error;
 
@@ -814,10 +813,10 @@ impl ServiceInner {
         let rack_network_config = match &config.rack_network_config {
             Some(config) => {
                 let value = NexusTypes::RackNetworkConfig {
-                    gateway_ip: config.gateway_ip.clone(),
-                    infra_ip_first: config.infra_ip_first.clone(),
-                    infra_ip_last: config.infra_ip_last.clone(),
-                    uplink_ip: config.uplink_ip.clone(),
+                    gateway_ip: config.gateway_ip,
+                    infra_ip_first: config.infra_ip_first,
+                    infra_ip_last: config.infra_ip_last,
+                    uplink_ip: config.uplink_ip,
                     uplink_port: config.uplink_port.clone(),
                     uplink_port_speed: config.uplink_port_speed.clone().into(),
                     uplink_port_fec: config.uplink_port_fec.clone().into(),
@@ -1098,11 +1097,7 @@ impl ServiceInner {
             // TODO handle breakouts
             // https://github.com/oxidecomputer/omicron/issues/3062
             let link_id = LinkId(0);
-            let addr: IpAddr = rack_network_config.uplink_ip.parse()
-                .map_err(|e| {
-                    SetupServiceError::BadConfig(format!(
-                    "unable to parse rack_network_config.uplink_up as IpAddr: {e}"))
-                })?;
+            let addr = IpAddr::V4(rack_network_config.uplink_ip);
 
             let link_settings = LinkSettings {
                 // TODO Allow user to configure link properties
@@ -1125,10 +1120,7 @@ impl ServiceInner {
                         format!("could not use value provided to rack_network_config.uplink_port as PortID: {e}")
                 ))?;
 
-            let nexthop: Option<Ipv4Addr> = Some(rack_network_config.gateway_ip.parse()
-                .map_err(|e| SetupServiceError::BadConfig(
-                        format!("unable to parse rack_network_config.gateway_ip as Ipv4Addr: {e}")
-                ))?);
+            let nexthop = Some(rack_network_config.gateway_ip);
 
             dpd_port_settings.v4_routes.insert(
                 Ipv4Cidr { prefix: "0.0.0.0".parse().unwrap(), prefix_len: 0 }

wicket/src/rack_setup/config_toml.rs

@@ -169,10 +169,10 @@ fn populate_network_table(
 ) {
     // Helper function to serialize enums into their appropriate string
     // representations.
-    fn enum_to_toml_string<T: Serialize>(value: &T) -> Cow<'static, str> {
+    fn enum_to_toml_string<T: Serialize>(value: &T) -> String {
         let value = toml::Value::try_from(value).unwrap();
         match value {
-            toml::Value::String(s) => Cow::from(s),
+            toml::Value::String(s) => s,
             other => {
                 panic!("improper use of enum_to_toml_string: got {other:?}");
             }
@@ -184,23 +184,24 @@ fn populate_network_table(
     };
 
     for (property, value) in [
-        ("gateway_ip", Cow::from(&config.gateway_ip)),
-        ("infra_ip_first", Cow::from(&config.infra_ip_first)),
-        ("infra_ip_last", Cow::from(&config.infra_ip_last)),
-        ("uplink_port", Cow::from(&config.uplink_port)),
+        ("gateway_ip", config.gateway_ip.to_string()),
+        ("infra_ip_first", config.infra_ip_first.to_string()),
+        ("infra_ip_last", config.infra_ip_last.to_string()),
+        ("uplink_port", config.uplink_port.to_string()),
         ("uplink_port_speed", enum_to_toml_string(&config.uplink_port_speed)),
         ("uplink_port_fec", enum_to_toml_string(&config.uplink_port_fec)),
-        ("uplink_ip", Cow::from(&config.uplink_ip)),
+        ("uplink_ip", config.uplink_ip.to_string()),
     ] {
         *table.get_mut(property).unwrap().as_value_mut().unwrap() =
-            Value::String(Formatted::new(value.into_owned()));
+            Value::String(Formatted::new(value));
     }
 }
 
 #[cfg(test)]
 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;
@@ -300,10 +301,10 @@ mod tests {
             )],
             ntp_servers: vec!["ntp1.com".into(), "ntp2.com".into()],
             rack_network_config: Some(RackNetworkConfig {
-                gateway_ip: "1.2.3.4".into(),
-                infra_ip_first: "2.3.4.5".into(),
-                infra_ip_last: "3.4.5.6".into(),
-                uplink_ip: "4.5.6.7".into(),
+                gateway_ip: Ipv4Addr::new(1, 2, 3, 4),
+                infra_ip_first: Ipv4Addr::new(2, 3, 4, 5),
+                infra_ip_last: Ipv4Addr::new(3, 4, 5, 6),
+                uplink_ip: Ipv4Addr::new(4, 5, 6, 7),
                 uplink_port_speed: PortSpeed::Speed400G,
                 uplink_port_fec: PortFec::Firecode,
                 uplink_port: "port0".into(),

wicket/src/ui/panes/rack_setup.rs

@@ -709,19 +709,27 @@ fn rss_config_text<'a>(
             "External DNS zone name: ",
             Cow::from(insensitive.external_dns_zone_name.as_str()),
         ),
-        ("Gateway IP: ", net_config.map_or("", |c| &c.gateway_ip).into()),
+        (
+            "Gateway IP: ",
+            net_config.map_or("".into(), |c| c.gateway_ip.to_string().into()),
+        ),
         (
             "Infrastructure first IP: ",
-            net_config.map_or("", |c| &c.infra_ip_first).into(),
+            net_config
+                .map_or("".into(), |c| c.infra_ip_first.to_string().into()),
         ),
         (
             "Infrastructure last IP: ",
-            net_config.map_or("", |c| &c.infra_ip_last).into(),
+            net_config
+                .map_or("".into(), |c| c.infra_ip_last.to_string().into()),
         ),
         ("Uplink port: ", net_config.map_or("", |c| &c.uplink_port).into()),
         ("Uplink port speed: ", uplink_port_speed),
         ("Uplink port FEC: ", uplink_port_fec),
-        ("Uplink IP: ", net_config.map_or("", |c| &c.uplink_ip).into()),
+        (
+            "Uplink IP: ",
+            net_config.map_or("".into(), |c| c.uplink_ip.to_string().into()),
+        ),
     ] {
         spans.push(Spans::from(vec![
             Span::styled(label, label_style),

wicketd/src/rss_config.rs

@@ -11,6 +11,7 @@ use crate::http_entrypoints::CurrentRssUserConfig;
 use crate::http_entrypoints::CurrentRssUserConfigInsensitive;
 use crate::http_entrypoints::CurrentRssUserConfigSensitive;
 use crate::RackV1Inventory;
+use anyhow::anyhow;
 use anyhow::bail;
 use anyhow::Result;
 use bootstrap_agent_client::types::BootstrapAddressDiscovery;
@@ -22,6 +23,7 @@ use bootstrap_agent_client::types::UserId;
 use gateway_client::types::SpType;
 use omicron_certificates::CertificateValidator;
 use omicron_common::address;
+use omicron_common::address::Ipv4Range;
 use omicron_common::api::internal::shared::RackNetworkConfig;
 use sled_hardware::Baseboard;
 use std::collections::BTreeSet;
@@ -128,7 +130,7 @@ impl CurrentRssConfig {
             bail!("rack network config not set (have you uploaded a config?)");
         };
         let rack_network_config =
-            validate_rack_network_config(rack_network_config);
+            validate_rack_network_config(rack_network_config)?;
 
         let known_bootstrap_sleds = bootstrap_peers.sleds();
         let mut bootstrap_ips = Vec::new();
@@ -357,18 +359,36 @@ impl From<&'_ CurrentRssConfig> for CurrentRssUserConfig {
 
 fn validate_rack_network_config(
     config: &RackNetworkConfig,
-) -> bootstrap_agent_client::types::RackNetworkConfig {
+) -> Result<bootstrap_agent_client::types::RackNetworkConfig> {
     use bootstrap_agent_client::types::PortFec as BaPortFec;
     use bootstrap_agent_client::types::PortSpeed as BaPortSpeed;
     use omicron_common::api::internal::shared::PortFec;
     use omicron_common::api::internal::shared::PortSpeed;
 
-    // TODO Add client side checks on `rack_network_config` contents.
+    // Make sure `infra_ip_first`..`infra_ip_last` is a well-defined range...
+    let infra_ip_range =
+        Ipv4Range::new(config.infra_ip_first, config.infra_ip_last).map_err(
+            |s: String| {
+                anyhow!("invalid `infra_ip_first`, `infra_ip_last` range: {s}")
+            },
+        )?;
+
+    // ... and that it contains `uplink_ip`.
+    if config.uplink_ip < infra_ip_range.first
+        || config.uplink_ip > infra_ip_range.last
+    {
+        bail!(
+            "`uplink_ip` must be in the range defined by `infra_ip_first` \
+             and `infra_ip_last`"
+        );
+    }
 
-    bootstrap_agent_client::types::RackNetworkConfig {
-        gateway_ip: config.gateway_ip.clone(),
-        infra_ip_first: config.infra_ip_first.clone(),
-        infra_ip_last: config.infra_ip_last.clone(),
+    // TODO Add more client side checks on `rack_network_config` contents?
+
+    Ok(bootstrap_agent_client::types::RackNetworkConfig {
+        gateway_ip: config.gateway_ip,
+        infra_ip_first: config.infra_ip_first,
+        infra_ip_last: config.infra_ip_last,
         uplink_port: config.uplink_port.clone(),
         uplink_port_speed: match config.uplink_port_speed {
             PortSpeed::Speed0G => BaPortSpeed::Speed0G,
@@ -386,7 +406,7 @@ fn validate_rack_network_config(
             PortFec::None => BaPortFec::None,
             PortFec::Rs => BaPortFec::Rs,
         },
-        uplink_ip: config.uplink_ip.clone(),
+        uplink_ip: config.uplink_ip,
         uplink_vid: config.uplink_vid,
-    }
+    })
 }