Skip to content

Commit

Permalink
[common] remove From<Ip*Addr> for IpNet impls (#5711)
Browse files Browse the repository at this point in the history
Closes #5687.
  • Loading branch information
sunshowers committed May 14, 2024
1 parent 0418e8a commit 4eacb92
Show file tree
Hide file tree
Showing 4 changed files with 36 additions and 28 deletions.
47 changes: 26 additions & 21 deletions common/src/api/external/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -1508,39 +1530,22 @@ impl From<ipnetwork::IpNetwork> for IpNet {
}
}

// NOTE: We deliberately do *NOT* implement `From<Ip{v4,v6,}Addr> 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<Ipv4Net> for IpNet {
fn from(n: Ipv4Net) -> IpNet {
IpNet::V4(n)
}
}

impl From<Ipv4Addr> for IpNet {
fn from(n: Ipv4Addr) -> IpNet {
IpNet::V4(Ipv4Net(ipnetwork::Ipv4Network::from(n)))
}
}

impl From<Ipv6Net> for IpNet {
fn from(n: Ipv6Net) -> IpNet {
IpNet::V6(n)
}
}

impl From<Ipv6Addr> for IpNet {
fn from(n: Ipv6Addr) -> IpNet {
IpNet::V6(Ipv6Net(ipnetwork::Ipv6Network::from(n)))
}
}

impl From<IpAddr> 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 {
Expand Down
2 changes: 1 addition & 1 deletion common/src/api/internal/shared.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()
)),
Expand Down
4 changes: 2 additions & 2 deletions nexus/networking/src/firewall_rules.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand All @@ -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) => {
Expand Down
11 changes: 7 additions & 4 deletions nexus/tests/integration_tests/allow_list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -74,16 +75,18 @@ 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;

// Add our IP in the front and end, and still make sure that works.
//
// 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;
Expand All @@ -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 };
Expand Down

0 comments on commit 4eacb92

Please sign in to comment.