From 4eacb9288ac300bd2b9c5718eb5d85c8d2d4c081 Mon Sep 17 00:00:00 2001 From: Rain Date: Tue, 14 May 2024 02:27:35 -0700 Subject: [PATCH] [common] remove From for IpNet impls (#5711) Closes #5687. --- common/src/api/external/mod.rs | 47 ++++++++++++--------- common/src/api/internal/shared.rs | 2 +- nexus/networking/src/firewall_rules.rs | 4 +- nexus/tests/integration_tests/allow_list.rs | 11 +++-- 4 files changed, 36 insertions(+), 28 deletions(-) diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs index 9ce5e6ce46..1c01782cc6 100644 --- a/common/src/api/external/mod.rs +++ b/common/src/api/external/mod.rs @@ -1234,6 +1234,13 @@ impl DiskState { pub struct Ipv4Net(pub ipnetwork::Ipv4Network); impl Ipv4Net { + /// Constructs a new `Ipv4Net` representing a single IP. + pub fn single(ip: Ipv4Addr) -> Self { + Ipv4Net( + ipnetwork::Ipv4Network::new(ip, 32).expect("32 is within range"), + ) + } + /// Return `true` if this IPv4 subnetwork is from an RFC 1918 private /// address space. pub fn is_private(&self) -> bool { @@ -1301,6 +1308,13 @@ impl Ipv6Net { /// The prefix length for all VPC Sunets pub const VPC_SUBNET_IPV6_PREFIX_LENGTH: u8 = 64; + /// Constructs a new `Ipv6Net` representing a single IPv6 address. + pub fn single(ip: Ipv6Addr) -> Self { + Ipv6Net( + ipnetwork::Ipv6Network::new(ip, 128).expect("128 is within range"), + ) + } + /// Return `true` if this subnetwork is in the IPv6 Unique Local Address /// range defined in RFC 4193, e.g., `fd00:/8` pub fn is_unique_local(&self) -> bool { @@ -1436,6 +1450,14 @@ pub enum IpNet { } impl IpNet { + /// Constructs a new `IpNet` representing a single IP. + pub fn single(ip: IpAddr) -> Self { + match ip { + IpAddr::V4(ip) => IpNet::V4(Ipv4Net::single(ip)), + IpAddr::V6(ip) => IpNet::V6(Ipv6Net::single(ip)), + } + } + /// Return the underlying address. pub fn ip(&self) -> IpAddr { match self { @@ -1508,39 +1530,22 @@ impl From for IpNet { } } +// NOTE: We deliberately do *NOT* implement `From for IpNet`. +// This is because there are many ways to convert an address into a network. +// See https://github.com/oxidecomputer/omicron/issues/5687. + impl From for IpNet { fn from(n: Ipv4Net) -> IpNet { IpNet::V4(n) } } -impl From for IpNet { - fn from(n: Ipv4Addr) -> IpNet { - IpNet::V4(Ipv4Net(ipnetwork::Ipv4Network::from(n))) - } -} - impl From for IpNet { fn from(n: Ipv6Net) -> IpNet { IpNet::V6(n) } } -impl From for IpNet { - fn from(n: Ipv6Addr) -> IpNet { - IpNet::V6(Ipv6Net(ipnetwork::Ipv6Network::from(n))) - } -} - -impl From for IpNet { - fn from(n: IpAddr) -> IpNet { - match n { - IpAddr::V4(v4) => IpNet::from(v4), - IpAddr::V6(v6) => IpNet::from(v6), - } - } -} - impl std::fmt::Display for IpNet { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { diff --git a/common/src/api/internal/shared.rs b/common/src/api/internal/shared.rs index 6bd40d3ff0..9d9ff083e4 100644 --- a/common/src/api/internal/shared.rs +++ b/common/src/api/internal/shared.rs @@ -608,7 +608,7 @@ mod tests { assert_eq!( parsed, AllowedSourceIps::try_from(vec![ - IpNet::from(Ipv4Addr::LOCALHOST), + IpNet::V4(Ipv4Net::single(Ipv4Addr::LOCALHOST)), IpNet::V4(Ipv4Net( Ipv4Network::new(Ipv4Addr::new(10, 0, 0, 0), 24).unwrap() )), diff --git a/nexus/networking/src/firewall_rules.rs b/nexus/networking/src/firewall_rules.rs index dc67ce5937..623c545702 100644 --- a/nexus/networking/src/firewall_rules.rs +++ b/nexus/networking/src/firewall_rules.rs @@ -353,7 +353,7 @@ pub async fn resolve_firewall_rules_for_sled_agent( .unwrap_or(&no_interfaces) { host_addrs.push( - HostIdentifier::Ip(IpNet::from( + HostIdentifier::Ip(IpNet::single( interface.ip, )) .into(), @@ -373,7 +373,7 @@ pub async fn resolve_firewall_rules_for_sled_agent( } external::VpcFirewallRuleHostFilter::Ip(addr) => { host_addrs.push( - HostIdentifier::Ip(IpNet::from(*addr)).into(), + HostIdentifier::Ip(IpNet::single(*addr)).into(), ) } external::VpcFirewallRuleHostFilter::IpNet(net) => { diff --git a/nexus/tests/integration_tests/allow_list.rs b/nexus/tests/integration_tests/allow_list.rs index 319696b5f5..fde0fe5db7 100644 --- a/nexus/tests/integration_tests/allow_list.rs +++ b/nexus/tests/integration_tests/allow_list.rs @@ -9,6 +9,7 @@ use nexus_test_utils::http_testing::{AuthnMode, NexusRequest}; use nexus_test_utils_macros::nexus_test; use nexus_types::external_api::{params, views}; use omicron_common::api::external::AllowedSourceIps; +use omicron_common::api::external::IpNet; use std::net::IpAddr; use std::net::Ipv4Addr; @@ -74,7 +75,7 @@ async fn test_allow_list(cptestctx: &ControlPlaneTestContext) { } // Set the list with exactly one IP, make sure it's the same. - let allowed_ips = AllowedSourceIps::try_from(vec![our_addr.into()]) + let allowed_ips = AllowedSourceIps::try_from(vec![IpNet::single(our_addr)]) .expect("Expected a valid IP list"); update_list_and_compare(client, allowed_ips).await; @@ -82,8 +83,10 @@ async fn test_allow_list(cptestctx: &ControlPlaneTestContext) { // // This is a regression for // https://github.com/oxidecomputer/omicron/issues/5727. - let addrs = - vec![our_addr.into(), IpAddr::V4(Ipv4Addr::new(10, 0, 0, 1)).into()]; + let addrs = vec![ + IpNet::single(our_addr), + IpNet::single(IpAddr::V4(Ipv4Addr::new(10, 0, 0, 1))), + ]; let allowed_ips = AllowedSourceIps::try_from(addrs.clone()) .expect("Expected a valid IP list"); update_list_and_compare(client, allowed_ips).await; @@ -98,7 +101,7 @@ async fn test_allow_list(cptestctx: &ControlPlaneTestContext) { // Check that we cannot make the request with a list that doesn't include // us. - let addrs = vec![IpAddr::V4(Ipv4Addr::new(1, 1, 1, 1)).into()]; + let addrs = vec![IpNet::single(IpAddr::V4(Ipv4Addr::new(1, 1, 1, 1)))]; let allowed_ips = AllowedSourceIps::try_from(addrs.clone()) .expect("Expected a valid IP list"); let new_list = params::AllowListUpdate { allowed_ips };