Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Require both IPv4 and IPv6 blocks on VPC subnets #577

Closed
wants to merge 1 commit into from
Closed

Require both IPv4 and IPv6 blocks on VPC subnets #577

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions nexus/src/db/datastore.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1090,9 +1090,7 @@ impl DataStore {
}
// Insert and allocate an IP address
None => {
let block = interface.subnet.ipv4_block.ok_or_else(|| {
Error::internal_error("assuming subnets all have v4 block")
})?;
let block = interface.subnet.ipv4_block;
let allocation_query = AllocateIpQuery {
block: ipnetwork::IpNetwork::V4(block.0 .0),
interface,
Expand Down
12 changes: 6 additions & 6 deletions nexus/src/db/model.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1162,8 +1162,8 @@ pub struct VpcSubnet {
identity: VpcSubnetIdentity,

pub vpc_id: Uuid,
pub ipv4_block: Option<Ipv4Net>,
pub ipv6_block: Option<Ipv6Net>,
pub ipv4_block: Ipv4Net,
pub ipv6_block: Ipv6Net,
}

impl VpcSubnet {
Expand All @@ -1176,8 +1176,8 @@ impl VpcSubnet {
Self {
identity,
vpc_id,
ipv4_block: params.ipv4_block.map(Ipv4Net),
ipv6_block: params.ipv6_block.map(Ipv6Net),
ipv4_block: Ipv4Net(params.ipv4_block),
ipv6_block: Ipv6Net(params.ipv6_block),
}
}
}
Expand All @@ -1198,8 +1198,8 @@ impl From<params::VpcSubnetUpdate> for VpcSubnetUpdate {
name: params.identity.name.map(Name),
description: params.identity.description,
time_modified: Utc::now(),
ipv4_block: params.ipv4_block.map(Ipv4Net),
ipv6_block: params.ipv6_block.map(Ipv6Net),
ipv4_block: Some(Ipv4Net(params.ipv4_block)),
ipv6_block: Some(Ipv6Net(params.ipv6_block)),
Copy link
Contributor Author

@david-crespo david-crespo Jan 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact that the IP blocks use Some but name and description do not suggests something is off here, though it may be that the former are correct and the latter are wrong.

It's a bit odd that name and description are optional on IdentityMetadataUpdateParams given that I thought we decided to go with the more strictly correct PUT scheme in which all fields are expected to be present in the request body. We probably just haven't updated all the code in accordance with that determination.

omicron/common/src/api/external/mod.rs

Lines 635 to 640 in 939adec

#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)]
#[serde(rename_all = "camelCase")]
pub struct IdentityMetadataUpdateParams {
pub name: Option<Name>,
pub description: Option<String>,
}

If name and description are made optional in the params, I would expect the fields to remain optional on the diesel changeset because we can use that internally to make queries that only update some fields but not others.

#[derive(AsChangeset)]
#[table_name = "vpc_subnet"]
pub struct VpcSubnetUpdate {
    pub name: Option<Name>,
    pub description: Option<String>,
    pub time_modified: DateTime<Utc>,
    pub ipv4_block: Option<Ipv4Net>,
    pub ipv6_block: Option<Ipv6Net>,
}

In that case the resolution to the above asymmetry will be to wrap name and description in Some:

name: Some(Name(params.identity.name)),
description: Some(params.identity.description),

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed there's an issue for bringing our PUTs into line: #333

}
}
}
Expand Down
4 changes: 2 additions & 2 deletions nexus/src/db/schema.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,8 +233,8 @@ table! {
time_modified -> Timestamptz,
time_deleted -> Nullable<Timestamptz>,
vpc_id -> Uuid,
ipv4_block -> Nullable<Inet>,
ipv6_block -> Nullable<Inet>,
ipv4_block -> Inet,
ipv6_block -> Inet,
}
}

Expand Down
11 changes: 6 additions & 5 deletions nexus/src/db/subnet_allocation.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ mod test {
use diesel::pg::Pg;
use diesel::prelude::*;
use omicron_common::api::external::{
IdentityMetadataCreateParams, Ipv4Net, MacAddr,
IdentityMetadataCreateParams, Ipv4Net, Ipv6Net, MacAddr,
};
use std::convert::TryInto;

Expand All @@ -249,7 +249,8 @@ mod test {
let subnet_id =
uuid::Uuid::parse_str("223cb7f7-0d3a-4a4e-a5e1-ad38ecb785d3")
.unwrap();
let block: ipnetwork::Ipv4Network = "192.168.1.0/24".parse().unwrap();
let ipv4_block = "192.168.1.0/24".parse().unwrap();
let ipv6_block = "fd12:3456::/64".parse().unwrap();
let subnet = VpcSubnet::new(
subnet_id,
vpc_id,
Expand All @@ -258,8 +259,8 @@ mod test {
name: "test-subnet".to_string().try_into().unwrap(),
description: "subnet description".to_string(),
},
ipv4_block: Some(Ipv4Net(block.clone()).into()),
ipv6_block: None,
ipv4_block: Ipv4Net(ipv4_block),
ipv6_block: Ipv6Net(ipv6_block),
},
);
let mac =
Expand All @@ -281,7 +282,7 @@ mod test {
);
let select = AllocateIpQuery {
interface,
block: block.into(),
block: ipv4_block.into(),
now: DateTime::<Utc>::from_utc(
NaiveDateTime::from_timestamp(0, 0),
Utc,
Expand Down
8 changes: 4 additions & 4 deletions nexus/src/external_api/params.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,8 +127,8 @@ pub struct VpcUpdate {
pub struct VpcSubnetCreate {
#[serde(flatten)]
pub identity: IdentityMetadataCreateParams,
pub ipv4_block: Option<Ipv4Net>,
pub ipv6_block: Option<Ipv6Net>,
pub ipv4_block: Ipv4Net,
pub ipv6_block: Ipv6Net,
}

/**
Expand All @@ -139,8 +139,8 @@ pub struct VpcSubnetCreate {
pub struct VpcSubnetUpdate {
#[serde(flatten)]
pub identity: IdentityMetadataUpdateParams,
pub ipv4_block: Option<Ipv4Net>,
pub ipv6_block: Option<Ipv6Net>,
pub ipv4_block: Ipv4Net,
pub ipv6_block: Ipv6Net,
}

/*
Expand Down
8 changes: 4 additions & 4 deletions nexus/src/external_api/views.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,19 +119,19 @@ pub struct VpcSubnet {
// how to do the validation of user-specified CIDR blocks, or how to create a block if one is
// not given.
/** The IPv4 subnet CIDR block. */
pub ipv4_block: Option<Ipv4Net>,
pub ipv4_block: Ipv4Net,

/** The IPv6 subnet CIDR block. */
pub ipv6_block: Option<Ipv6Net>,
pub ipv6_block: Ipv6Net,
}

impl Into<VpcSubnet> for model::VpcSubnet {
fn into(self) -> VpcSubnet {
VpcSubnet {
identity: self.identity(),
vpc_id: self.vpc_id,
ipv4_block: self.ipv4_block.map(|ip| ip.into()),
ipv6_block: self.ipv6_block.map(|ip| ip.into()),
ipv4_block: self.ipv4_block.into(),
ipv6_block: self.ipv6_block.into(),
}
}
}
Expand Down
8 changes: 4 additions & 4 deletions nexus/src/nexus.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1592,14 +1592,14 @@ impl Nexus {
params.identity.name
),
},
ipv4_block: Some(Ipv4Net(
ipv4_block: Ipv4Net(
// TODO: This value should be replaced with the correct ipv4 range for a default subnet
"10.1.9.32/16".parse::<Ipv4Network>().unwrap(),
)),
ipv6_block: Some(Ipv6Net(
),
ipv6_block: Ipv6Net(
// TODO: This value should be replaced w/ the first `/64` ipv6 from the address block
"2001:db8::0/64".parse::<Ipv6Network>().unwrap(),
)),
),
},
);
self.db_datastore.vpc_create_subnet(subnet).await?;
Expand Down
9 changes: 5 additions & 4 deletions nexus/tests/integration_tests/subnet_allocation.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use http::method::Method;
use http::StatusCode;
use omicron_common::api::external::{
ByteCount, IdentityMetadataCreateParams, IdentityMetadataUpdateParams,
Instance, InstanceCpuCount, Ipv4Net, NetworkInterface,
Instance, InstanceCpuCount, Ipv4Net, Ipv6Net, NetworkInterface,
};
use omicron_nexus::external_api::params;
use std::net::IpAddr;
Expand Down Expand Up @@ -88,14 +88,15 @@ async fn test_subnet_allocation(cptestctx: &ControlPlaneTestContext) {
"/organizations/{}/projects/{}/vpcs/default/subnets/default",
organization_name, project_name
);
let subnet = "192.168.42.0/29".parse().unwrap();
let ipv4_block = "192.168.42.0/29".parse().unwrap();
let ipv6_block = "fd12:3456::/64".parse().unwrap();
let subnet_update = params::VpcSubnetUpdate {
identity: IdentityMetadataUpdateParams {
name: Some("default".parse().unwrap()),
description: None,
},
ipv4_block: Some(Ipv4Net(subnet)),
ipv6_block: None,
ipv4_block: Ipv4Net(ipv4_block),
ipv6_block: Ipv6Net(ipv6_block),
};
client
.make_request(
Expand Down
26 changes: 14 additions & 12 deletions nexus/tests/integration_tests/vpc_subnets.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,10 +72,8 @@ async fn test_vpc_subnets(cptestctx: &ControlPlaneTestContext) {
assert_eq!(error.message, "not found: vpc-subnet with name \"subnet1\"");

/* Create a VPC Subnet. */
let ipv4_block =
Some(Ipv4Net("10.1.9.32/16".parse::<Ipv4Network>().unwrap()));
let ipv6_block =
Some(Ipv6Net("2001:db8::0/96".parse::<Ipv6Network>().unwrap()));
let ipv4_block = Ipv4Net("10.1.9.32/16".parse::<Ipv4Network>().unwrap());
let ipv6_block = Ipv6Net("2001:db8::0/96".parse::<Ipv6Network>().unwrap());
let new_subnet = params::VpcSubnetCreate {
identity: IdentityMetadataCreateParams {
name: subnet_name.parse().unwrap(),
Expand Down Expand Up @@ -146,16 +144,16 @@ async fn test_vpc_subnets(cptestctx: &ControlPlaneTestContext) {
name: subnet2_name.parse().unwrap(),
description: "it's also below the net".to_string(),
},
ipv4_block: None,
ipv6_block: None,
ipv4_block,
ipv6_block,
};
let subnet2: VpcSubnet =
objects_post(&client, &subnets_url, new_subnet.clone()).await;
assert_eq!(subnet2.identity.name, subnet2_name);
assert_eq!(subnet2.identity.description, "it's also below the net");
assert_eq!(subnet2.vpc_id, vpc.identity.id);
assert_eq!(subnet2.ipv4_block, None);
assert_eq!(subnet2.ipv6_block, None);
assert_eq!(subnet2.ipv4_block, ipv4_block);
assert_eq!(subnet2.ipv6_block, ipv6_block);

// subnets list should now have two in it
let subnets =
Expand All @@ -165,13 +163,17 @@ async fn test_vpc_subnets(cptestctx: &ControlPlaneTestContext) {
subnets_eq(&subnets[1], &subnet2);

// update first subnet
let new_ipv4_block =
Ipv4Net("10.1.9.33/16".parse::<Ipv4Network>().unwrap());
let new_ipv6_block =
Ipv6Net("2001:db9::0/96".parse::<Ipv6Network>().unwrap());
let update_params = params::VpcSubnetUpdate {
identity: IdentityMetadataUpdateParams {
name: Some("new-name".parse().unwrap()),
description: Some("another description".to_string()),
},
ipv4_block: None,
ipv6_block: None,
ipv4_block: new_ipv4_block,
ipv6_block: new_ipv6_block,
};
client
.make_request(
Expand Down Expand Up @@ -245,8 +247,8 @@ async fn test_vpc_subnets(cptestctx: &ControlPlaneTestContext) {
"it's also below the net"
);
assert_eq!(subnet_same_name.vpc_id, vpc2.identity.id);
assert_eq!(subnet_same_name.ipv4_block, None);
assert_eq!(subnet_same_name.ipv6_block, None);
assert_eq!(subnet_same_name.ipv4_block, ipv4_block);
assert_eq!(subnet_same_name.ipv6_block, ipv6_block);
}

fn subnets_eq(sn1: &VpcSubnet, sn2: &VpcSubnet) {
Expand Down