Skip to content

Commit b7beb65

Browse files
Rename params::AddressSelector to params::AddressAllocator in the external API (#9669)
From discussion in #9638 (comment), we decided that `AddressSelector` is a bit of a misnomer in our current scheme, where "selector" implies filtering/fetching from existing resources. We'll use `AddressAllocator` instead, which better describes the action of reserving/assigning a floating IP address from a pool. I've also done a bit of refactoring here to be more RFD 619 friendly. Before this PR, we had a series of `FloatingIpCreate` versions that each had `From` or `TryFrom` conversions directly to the latest API version in "big step" style. This becomes an _O(n)^2_ problem to maintain over time. Instead, in this PR, we convert to "small step" style, where each older version of `FloatingIpCreate` converts to the next newer version. In the terms of the RFD, we "[hop through intermediate versions](https://rfd.shared.oxide.computer/rfd/0619#determinations-one-prior-version)." This ensures that the next time we change `FloatingIpCreate`, we only need to introduce one new conversion, rather than update all older conversions. I've added proptests to make our expectations of the conversion behavior more explicit and prevent regressions.
1 parent c6e516c commit b7beb65

16 files changed

Lines changed: 30196 additions & 57 deletions

File tree

Cargo.lock

Lines changed: 2 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

nexus/external-api/Cargo.toml

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,3 +31,7 @@ scim2-rs.workspace = true
3131
serde.workspace = true
3232
tufaceous-artifact.workspace = true
3333
uuid.workspace = true
34+
35+
[dev-dependencies]
36+
proptest.workspace = true
37+
test-strategy.workspace = true

nexus/external-api/src/lib.rs

Lines changed: 39 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,10 @@ mod v2025122300;
4141
mod v2026010100;
4242
mod v2026010300;
4343
mod v2026010500;
44+
mod v2026011501;
45+
46+
#[cfg(test)]
47+
mod test_utils;
4448

4549
api_versions!([
4650
// API versions are in the format YYYYMMDDNN.0.0, defined below as
@@ -70,6 +74,7 @@ api_versions!([
7074
// | date-based version should be at the top of the list.
7175
// v
7276
// (next_yyyymmddnn, IDENT),
77+
(2026011600, RENAME_ADDRESS_SELECTOR_TO_ADDRESS_ALLOCATOR),
7378
(2026011501, AUDIT_LOG_CREDENTIAL_ID),
7479
(2026011500, AUDIT_LOG_AUTH_METHOD_ENUM),
7580
(2026011300, DOC_LINT_SUMMARY_TRAILING_PERIOD),
@@ -1298,10 +1303,12 @@ pub trait NexusExternalApi {
12981303
query_params: Query<params::ProjectSelector>,
12991304
floating_params: TypedBody<v2025121200::FloatingIpCreate>,
13001305
) -> Result<HttpResponseCreated<views::FloatingIp>, HttpError> {
1301-
Self::floating_ip_create(
1306+
let floating_params =
1307+
floating_params.map(v2026010300::FloatingIpCreate::from);
1308+
Self::v2026010300_floating_ip_create(
13021309
rqctx,
13031310
query_params,
1304-
floating_params.map(Into::into),
1311+
floating_params,
13051312
)
13061313
.await
13071314
}
@@ -1320,16 +1327,43 @@ pub trait NexusExternalApi {
13201327
query_params: Query<params::ProjectSelector>,
13211328
floating_params: TypedBody<v2026010300::FloatingIpCreate>,
13221329
) -> Result<HttpResponseCreated<views::FloatingIp>, HttpError> {
1323-
let floating_params = floating_params.try_map(TryInto::try_into)?;
1324-
Self::floating_ip_create(rqctx, query_params, floating_params).await
1330+
let floating_params =
1331+
floating_params.try_map(v2026011501::FloatingIpCreate::try_from)?;
1332+
Self::v2026011501_floating_ip_create(
1333+
rqctx,
1334+
query_params,
1335+
floating_params,
1336+
)
1337+
.await
13251338
}
13261339

13271340
/// Create floating IP
13281341
#[endpoint {
1342+
operation_id = "floating_ip_create",
13291343
method = POST,
13301344
path = "/v1/floating-ips",
13311345
tags = ["floating-ips"],
1332-
versions = VERSION_POOL_SELECTION_ENUMS..,
1346+
versions = VERSION_POOL_SELECTION_ENUMS..VERSION_RENAME_ADDRESS_SELECTOR_TO_ADDRESS_ALLOCATOR,
1347+
}]
1348+
async fn v2026011501_floating_ip_create(
1349+
rqctx: RequestContext<Self::Context>,
1350+
query_params: Query<params::ProjectSelector>,
1351+
floating_params: TypedBody<v2026011501::FloatingIpCreate>,
1352+
) -> Result<HttpResponseCreated<views::FloatingIp>, HttpError> {
1353+
Self::floating_ip_create(
1354+
rqctx,
1355+
query_params,
1356+
floating_params.map(Into::into),
1357+
)
1358+
.await
1359+
}
1360+
1361+
/// Create floating IP
1362+
#[endpoint {
1363+
method = POST,
1364+
path = "/v1/floating-ips",
1365+
tags = ["floating-ips"],
1366+
versions = VERSION_RENAME_ADDRESS_SELECTOR_TO_ADDRESS_ALLOCATOR..,
13331367
}]
13341368
async fn floating_ip_create(
13351369
rqctx: RequestContext<Self::Context>,
Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
// This Source Code Form is subject to the terms of the Mozilla Public
2+
// License, v. 2.0. If a copy of the MPL was not distributed with this
3+
// file, You can obtain one at https://mozilla.org/MPL/2.0/.
4+
5+
//! Common proptest strategies for versioned API type tests.
6+
//!
7+
//! This module provides reusable strategies for generating test data
8+
//! used in property-based tests across version modules.
9+
10+
use nexus_types::external_api::params;
11+
use omicron_common::api::external::{
12+
IdentityMetadataCreateParams, IpVersion, Name, NameOrId,
13+
};
14+
use proptest::prelude::*;
15+
use std::net::IpAddr;
16+
use uuid::Uuid;
17+
18+
/// Strategy for generating valid `Name` values.
19+
///
20+
/// Per RFD 4, names must be 1-63 characters and match the regex
21+
/// `[a-z]([-a-z0-9]*[a-z0-9])?`. We use `{0,61}` instead of `*` to
22+
/// bound generation length and avoid filtering out long strings.
23+
pub fn name_strategy() -> impl Strategy<Value = Name> {
24+
"[a-z]([-a-z0-9]{0,61}[a-z0-9])?"
25+
.prop_filter_map("valid name", |s| s.parse::<Name>().ok())
26+
}
27+
28+
/// Strategy for generating `NameOrId` values.
29+
pub fn name_or_id_strategy() -> impl Strategy<Value = NameOrId> {
30+
prop_oneof![
31+
name_strategy().prop_map(NameOrId::Name),
32+
any::<u128>().prop_map(|n| NameOrId::Id(Uuid::from_u128(n))),
33+
]
34+
}
35+
36+
/// Strategy for generating `IpVersion` values.
37+
pub fn ip_version_strategy() -> impl Strategy<Value = IpVersion> {
38+
prop_oneof![Just(IpVersion::V4), Just(IpVersion::V6)]
39+
}
40+
41+
/// Strategy for generating `PoolSelector` values.
42+
///
43+
/// Generates one of:
44+
/// - `Explicit { pool }`: Use a specific pool identified by name or ID
45+
/// - `Auto { ip_version }`: Use the silo's default pool, optionally filtered
46+
/// by IP version (IPv4/IPv6)
47+
pub fn pool_selector_strategy() -> impl Strategy<Value = params::PoolSelector> {
48+
prop_oneof![
49+
name_or_id_strategy()
50+
.prop_map(|pool| params::PoolSelector::Explicit { pool }),
51+
proptest::option::of(ip_version_strategy())
52+
.prop_map(|ip_version| params::PoolSelector::Auto { ip_version }),
53+
]
54+
}
55+
56+
/// Strategy for generating `IdentityMetadataCreateParams` values.
57+
///
58+
/// Description is limited to 512 characters in the database.
59+
pub fn identity_strategy() -> impl Strategy<Value = IdentityMetadataCreateParams>
60+
{
61+
(name_strategy(), ".{0,512}").prop_map(|(name, description)| {
62+
IdentityMetadataCreateParams { name, description }
63+
})
64+
}
65+
66+
/// Strategy for generating optional IP addresses.
67+
pub fn optional_ip_strategy() -> impl Strategy<Value = Option<IpAddr>> {
68+
proptest::option::of(any::<IpAddr>())
69+
}
70+
71+
/// Strategy for generating optional `NameOrId` values.
72+
pub fn optional_name_or_id_strategy() -> impl Strategy<Value = Option<NameOrId>>
73+
{
74+
proptest::option::of(name_or_id_strategy())
75+
}

nexus/external-api/src/v2025121200.rs

Lines changed: 52 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -120,22 +120,15 @@ pub struct FloatingIpCreate {
120120
pub pool: Option<external::NameOrId>,
121121
}
122122

123-
// Converts directly to params::FloatingIpCreate using AddressSelector
124-
impl From<FloatingIpCreate> for params::FloatingIpCreate {
125-
fn from(old: FloatingIpCreate) -> params::FloatingIpCreate {
126-
let address_selector = match (old.ip, old.pool) {
127-
// Explicit IP address provided
128-
(Some(ip), pool) => params::AddressSelector::Explicit { ip, pool },
129-
// Allocate from specified pool
130-
(None, Some(pool)) => params::AddressSelector::Auto {
131-
pool_selector: params::PoolSelector::Explicit { pool },
132-
},
133-
// Allocate from default pool
134-
(None, None) => params::AddressSelector::Auto {
135-
pool_selector: params::PoolSelector::Auto { ip_version: None },
136-
},
137-
};
138-
params::FloatingIpCreate { identity: old.identity, address_selector }
123+
// Converts to v2026010300::FloatingIpCreate (adds ip_version: None)
124+
impl From<FloatingIpCreate> for v2026010300::FloatingIpCreate {
125+
fn from(old: FloatingIpCreate) -> v2026010300::FloatingIpCreate {
126+
v2026010300::FloatingIpCreate {
127+
identity: old.identity,
128+
ip: old.ip,
129+
pool: old.pool,
130+
ip_version: None,
131+
}
139132
}
140133
}
141134

@@ -209,3 +202,46 @@ impl From<InstanceCreate> for v2026010100::InstanceCreate {
209202
}
210203
}
211204
}
205+
206+
#[cfg(test)]
207+
mod tests {
208+
use super::*;
209+
use crate::test_utils::{
210+
identity_strategy, optional_ip_strategy, optional_name_or_id_strategy,
211+
};
212+
use proptest::prelude::*;
213+
use test_strategy::proptest;
214+
215+
fn floating_ip_create_strategy() -> impl Strategy<Value = FloatingIpCreate>
216+
{
217+
(
218+
identity_strategy(),
219+
optional_ip_strategy(),
220+
optional_name_or_id_strategy(),
221+
)
222+
.prop_map(|(identity, ip, pool)| FloatingIpCreate {
223+
identity,
224+
ip,
225+
pool,
226+
})
227+
}
228+
229+
/// Verifies that the conversion from v2025121200::FloatingIpCreate to
230+
/// v2026010300::FloatingIpCreate preserves all existing fields, and that
231+
/// the ip_version field is set to None.
232+
#[proptest]
233+
fn floating_ip_create_converts_correctly(
234+
#[strategy(floating_ip_create_strategy())] input: FloatingIpCreate,
235+
) {
236+
let output: v2026010300::FloatingIpCreate = input.clone().into();
237+
238+
prop_assert_eq!(input.identity.name, output.identity.name);
239+
prop_assert_eq!(
240+
input.identity.description,
241+
output.identity.description
242+
);
243+
prop_assert_eq!(input.ip, output.ip);
244+
prop_assert_eq!(input.pool, output.pool);
245+
prop_assert_eq!(output.ip_version, None);
246+
}
247+
}

0 commit comments

Comments
 (0)