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

[nexus] SNAT IP comes from same pool as ephemeral IP #5678

Closed
wants to merge 2 commits into from
Closed

[nexus] SNAT IP comes from same pool as ephemeral IP #5678

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
48dbd3a
snat IP comes from same pool as ephemeral IP if specified
david-crespo May 1, 2024
acf5759
update tests about which IPs go to which pools
david-crespo May 1, 2024
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
5 changes: 3 additions & 2 deletions nexus/db-queries/src/db/datastore/external_ip.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,12 +67,13 @@ impl DataStore {
opctx: &OpContext,
ip_id: Uuid,
instance_id: Uuid,
pool_id: Uuid,
pool: Option<authz::IpPool>,
) -> CreateResult<ExternalIp> {
let authz_pool = self.resolve_pool_for_allocation(&opctx, pool).await?;
let data = IncompleteExternalIp::for_instance_source_nat(
ip_id,
instance_id,
pool_id,
authz_pool.id(),
);
self.allocate_external_ip(opctx, data).await
}
Expand Down
35 changes: 8 additions & 27 deletions nexus/db-queries/src/db/queries/external_ip.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1038,15 +1038,6 @@ mod tests {
instance_id
}

async fn default_pool_id(&self) -> Uuid {
let (.., pool) = self
.db_datastore
.ip_pools_fetch_default(&self.opctx)
.await
.expect("Failed to lookup default ip pool");
pool.identity.id
}

async fn success(mut self) {
self.db.cleanup().await.unwrap();
self.logctx.cleanup_successful();
Expand Down Expand Up @@ -1075,7 +1066,7 @@ mod tests {
&context.opctx,
id,
instance_id,
context.default_pool_id().await,
None,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of these work as expected when given None to make it keep choosing the default pool, but there should probably now be a test for a non-None value.

)
.await
.expect("Failed to allocate instance external IP address");
Expand All @@ -1092,7 +1083,7 @@ mod tests {
&context.opctx,
Uuid::new_v4(),
instance_id,
context.default_pool_id().await,
None,
)
.await
.expect_err(
Expand Down Expand Up @@ -1148,7 +1139,7 @@ mod tests {
&context.opctx,
Uuid::new_v4(),
instance_id,
context.default_pool_id().await,
None,
)
.await;
assert!(
Expand Down Expand Up @@ -1225,7 +1216,7 @@ mod tests {
&context.opctx,
Uuid::new_v4(),
instance_id,
context.default_pool_id().await,
None,
)
.await
.expect("Failed to allocate instance external IP address");
Expand Down Expand Up @@ -1253,7 +1244,7 @@ mod tests {
&context.opctx,
Uuid::new_v4(),
instance_id,
context.default_pool_id().await,
None,
)
.await
.expect("Failed to allocate instance external IP address");
Expand All @@ -1280,7 +1271,7 @@ mod tests {
&context.opctx,
Uuid::new_v4(),
instance_id,
context.default_pool_id().await,
None,
)
.await
.expect("Failed to allocate instance external IP address");
Expand Down Expand Up @@ -1589,12 +1580,7 @@ mod tests {
let id = Uuid::new_v4();
let ip = context
.db_datastore
.allocate_instance_snat_ip(
&context.opctx,
id,
instance_id,
context.default_pool_id().await,
)
.allocate_instance_snat_ip(&context.opctx, id, instance_id, None)
.await
.expect("Failed to allocate instance SNAT IP address");
assert_eq!(ip.kind, IpKind::SNat);
Expand All @@ -1606,12 +1592,7 @@ mod tests {
// value.
let new_ip = context
.db_datastore
.allocate_instance_snat_ip(
&context.opctx,
id,
instance_id,
context.default_pool_id().await,
)
.allocate_instance_snat_ip(&context.opctx, id, instance_id, None)
.await
.expect("Failed to allocate instance SNAT IP address");

Expand Down
33 changes: 27 additions & 6 deletions nexus/src/app/sagas/instance_create.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -647,14 +647,35 @@ async fn sic_allocate_instance_snat_ip(
let instance_id = sagactx.lookup::<Uuid>("instance_id")?;
let ip_id = sagactx.lookup::<Uuid>("snat_ip_id")?;

let (.., pool) = datastore
.ip_pools_fetch_default(&opctx)
.await
.map_err(ActionError::action_failed)?;
let pool_id = pool.identity.id;
// If an ephemeral IP was asked for and it specified a pool, we
// want to use that pool for the SNAT IP. Otherwise we will use
// the default pool.
let maybe_pool = saga_params.create_params.external_ips.iter().find_map(
|eip| match eip {
params::ExternalIpCreate::Ephemeral { pool: Some(pool) } => {
Some(pool)
}
params::ExternalIpCreate::Ephemeral { pool: None } => None,
params::ExternalIpCreate::Floating { .. } => None,
},
);

let maybe_pool = match maybe_pool {
Some(pool) => Some(
osagactx
.nexus()
.ip_pool_lookup(&opctx, pool)
.map_err(ActionError::action_failed)?
.lookup_for(authz::Action::CreateChild)
.await
.map_err(ActionError::action_failed)?
.0,
),
None => None,
};

datastore
.allocate_instance_snat_ip(&opctx, ip_id, instance_id, pool_id)
.allocate_instance_snat_ip(&opctx, ip_id, instance_id, maybe_pool)
.await
.map_err(ActionError::action_failed)?;
Ok(())
Expand Down
22 changes: 11 additions & 11 deletions nexus/tests/integration_tests/instances.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3958,13 +3958,10 @@ async fn test_instance_ephemeral_ip_from_correct_pool(
"Expected ephemeral IP to come from pool2"
);

// SNAT comes from default pool, but count does not change because
// SNAT IPs can be shared. https://github.com/oxidecomputer/omicron/issues/5043
// is about getting SNAT IP from specified pool instead of default.
assert_ip_pool_utilization(client, "pool1", 2, 5, 0, 0).await;

// ephemeral IP comes from specified pool
assert_ip_pool_utilization(client, "pool2", 1, 5, 0, 0).await;
// ephemeral IP and SNAT IP come from specified pool, so +2
assert_ip_pool_utilization(client, "pool2", 2, 5, 0, 0).await;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what is supposed to happen, so that's good.


// make pool2 default and create instance with default pool. check that it now it comes from pool2
let _: views::IpPoolSiloLink = object_put(
Expand All @@ -3983,7 +3980,8 @@ async fn test_instance_ephemeral_ip_from_correct_pool(

// pool1 unchanged
assert_ip_pool_utilization(client, "pool1", 2, 5, 0, 0).await;
// +1 snat (now that pool2 is default) and +1 ephemeral
// +1 ephemeral makes 3. SNAT comes from pool2 again, but count does not
// change because SNAT IPs can be shared.
assert_ip_pool_utilization(client, "pool2", 3, 5, 0, 0).await;

// try to delete association with pool1, but it fails because there is an
Expand All @@ -4005,9 +4003,9 @@ async fn test_instance_ephemeral_ip_from_correct_pool(
stop_and_delete_instance(&cptestctx, "pool2-inst").await;

// pool1 is down to 0 because it had 1 snat + 1 ephemeral from pool1-inst
// and 1 snat from pool2-inst
assert_ip_pool_utilization(client, "pool1", 0, 5, 0, 0).await;
// pool2 drops one because it had 1 ephemeral from pool2-inst
// pool2 drops one because it had 1 snat + 1 ephemeral from pool2-inst, but
// the snat is still in use by pool2-inst2
assert_ip_pool_utilization(client, "pool2", 2, 5, 0, 0).await;

// now unlink works
Expand Down Expand Up @@ -4163,8 +4161,7 @@ async fn test_instance_ephemeral_ip_no_default_pool_error(
let url = format!("/v1/instances?project={}", PROJECT_NAME);
let error =
object_create_error(client, &url, &body, StatusCode::NOT_FOUND).await;
let msg = "not found: default IP pool for current silo".to_string();
assert_eq!(error.message, msg);
assert_eq!(error.message, "not found: default IP pool for current silo");

// same deal if you specify a pool that doesn't exist
let body = params::InstanceCreate {
Expand All @@ -4175,7 +4172,10 @@ async fn test_instance_ephemeral_ip_no_default_pool_error(
};
let error =
object_create_error(client, &url, &body, StatusCode::NOT_FOUND).await;
assert_eq!(error.message, msg);
assert_eq!(
error.message,
"not found: ip-pool with name \"nonexistent-pool\""
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This used to complain about the default pool despite another pool being specified because the SNAT IP came from the default pool, and that failed before any ephemeral IP stuff came up.

}

#[nexus_test]
Expand Down
Loading