Skip to content

Commit

Permalink
Fix duplicate entries in IP pools list (#4808)
Browse files Browse the repository at this point in the history 
Bugfix for #4261. Using `distinct` to eliminate dupes from the left
outer join works, but it's better to just use the well-known name of the
service IP pool to exclude it from whatever operations it needs to be
excluded from.

Potential followup related to #4762: if the ID was well-known to, we
could do the `is_internal` check without having to hit the DB.
  • Loading branch information
@david-crespo
david-crespo authored Jan 16, 2024
1 parent 146aa5f commit 4796057
Show file tree
Hide file tree
Showing 3 changed files with 81 additions and 75 deletions.
59 changes: 7 additions & 52 deletions nexus/db-queries/src/db/datastore/ip_pool.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,12 @@ use crate::context::OpContext;
use crate::db;
use crate::db::collection_insert::AsyncInsertError;
use crate::db::collection_insert::DatastoreCollection;
use crate::db::datastore::SERVICE_IP_POOL_NAME;
use crate::db::error::public_error_from_diesel;
use crate::db::error::public_error_from_diesel_lookup;
use crate::db::error::ErrorHandler;
use crate::db::fixed_data::silo::INTERNAL_SILO_ID;
use crate::db::identity::Resource;
use crate::db::lookup::LookupPath;
use crate::db::model::ExternalIp;
use crate::db::model::IpKind;
use crate::db::model::IpPool;
Expand Down Expand Up @@ -56,7 +57,6 @@ impl DataStore {
pagparams: &PaginatedBy<'_>,
) -> ListResultVec<IpPool> {
use db::schema::ip_pool;
use db::schema::ip_pool_resource;

opctx
.authorize(authz::Action::ListChildren, &authz::IP_POOL_LIST)
Expand All @@ -71,14 +71,7 @@ impl DataStore {
&pagparams.map_name(|n| Name::ref_cast(n)),
),
}
.left_outer_join(ip_pool_resource::table)
.filter(
ip_pool_resource::resource_id
.ne(*INTERNAL_SILO_ID)
// resource_id is not nullable -- null here means the
// pool has no entry in the join table
.or(ip_pool_resource::resource_id.is_null()),
)
.filter(ip_pool::name.ne(SERVICE_IP_POOL_NAME))
.filter(ip_pool::time_deleted.is_null())
.select(IpPool::as_select())
.get_results_async(&*self.pool_connection_authorized(opctx).await?)
Expand Down Expand Up @@ -225,48 +218,15 @@ impl DataStore {
})
}

/// Looks up an IP pool intended for internal services.
/// Look up IP pool intended for internal services by its well-known name.
///
/// This method may require an index by Availability Zone in the future.
pub async fn ip_pools_service_lookup(
&self,
opctx: &OpContext,
) -> LookupResult<(authz::IpPool, IpPool)> {
use db::schema::ip_pool;
use db::schema::ip_pool_resource;

opctx
.authorize(authz::Action::ListChildren, &authz::IP_POOL_LIST)
.await?;

// Look up IP pool by its association with the internal silo.
// We assume there is only one pool for that silo, or at least,
// if there is more than one, it doesn't matter which one we pick.
let (authz_pool, pool) = ip_pool::table
.inner_join(ip_pool_resource::table)
.filter(ip_pool::time_deleted.is_null())
.filter(
ip_pool_resource::resource_type
.eq(IpPoolResourceType::Silo)
.and(ip_pool_resource::resource_id.eq(*INTERNAL_SILO_ID)),
)
.select(IpPool::as_select())
.get_result_async(&*self.pool_connection_authorized(opctx).await?)
.await
.map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))
.map(|ip_pool| {
(
authz::IpPool::new(
authz::FLEET,
ip_pool.id(),
LookupType::ByCompositeId(
"Service IP Pool".to_string(),
),
),
ip_pool,
)
})?;
Ok((authz_pool, pool))
let name = SERVICE_IP_POOL_NAME.parse().unwrap();
LookupPath::new(&opctx, self).ip_pool_name(&Name(name)).fetch().await
}

/// Creates a new IP pool.
Expand Down Expand Up @@ -374,15 +334,10 @@ impl DataStore {
authz_pool: &authz::IpPool,
) -> LookupResult<bool> {
use db::schema::ip_pool;
use db::schema::ip_pool_resource;

ip_pool::table
.inner_join(ip_pool_resource::table)
.filter(ip_pool::id.eq(authz_pool.id()))
.filter(
ip_pool_resource::resource_type.eq(IpPoolResourceType::Silo),
)
.filter(ip_pool_resource::resource_id.eq(*INTERNAL_SILO_ID))
.filter(ip_pool::name.eq(SERVICE_IP_POOL_NAME))
.filter(ip_pool::time_deleted.is_null())
.select(ip_pool::id)
.first_async::<Uuid>(
Expand Down
8 changes: 4 additions & 4 deletions nexus/tests/integration_tests/endpoints.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1048,7 +1048,7 @@ pub static VERIFY_ENDPOINTS: Lazy<Vec<VerifyEndpoint>> = Lazy::new(|| {
// IP Pool endpoint (Oxide services)
VerifyEndpoint {
url: &DEMO_IP_POOL_SERVICE_URL,
visibility: Visibility::Public,
visibility: Visibility::Protected,
unprivileged_access: UnprivilegedAccess::None,
allowed_methods: vec![
AllowedMethod::Get
Expand All @@ -1058,7 +1058,7 @@ pub static VERIFY_ENDPOINTS: Lazy<Vec<VerifyEndpoint>> = Lazy::new(|| {
// IP Pool ranges endpoint (Oxide services)
VerifyEndpoint {
url: &DEMO_IP_POOL_SERVICE_RANGES_URL,
visibility: Visibility::Public,
visibility: Visibility::Protected,
unprivileged_access: UnprivilegedAccess::None,
allowed_methods: vec![
AllowedMethod::Get
Expand All @@ -1068,7 +1068,7 @@ pub static VERIFY_ENDPOINTS: Lazy<Vec<VerifyEndpoint>> = Lazy::new(|| {
// IP Pool ranges/add endpoint (Oxide services)
VerifyEndpoint {
url: &DEMO_IP_POOL_SERVICE_RANGES_ADD_URL,
visibility: Visibility::Public,
visibility: Visibility::Protected,
unprivileged_access: UnprivilegedAccess::None,
allowed_methods: vec![
AllowedMethod::Post(
Expand All @@ -1080,7 +1080,7 @@ pub static VERIFY_ENDPOINTS: Lazy<Vec<VerifyEndpoint>> = Lazy::new(|| {
// IP Pool ranges/delete endpoint (Oxide services)
VerifyEndpoint {
url: &DEMO_IP_POOL_SERVICE_RANGES_DEL_URL,
visibility: Visibility::Public,
visibility: Visibility::Protected,
unprivileged_access: UnprivilegedAccess::None,
allowed_methods: vec![
AllowedMethod::Post(
Expand Down
89 changes: 70 additions & 19 deletions nexus/tests/integration_tests/ip_pools.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ use nexus_test_utils::http_testing::RequestBuilder;
use nexus_test_utils::resource_helpers::create_instance;
use nexus_test_utils::resource_helpers::create_ip_pool;
use nexus_test_utils::resource_helpers::create_project;
use nexus_test_utils::resource_helpers::create_silo;
use nexus_test_utils::resource_helpers::link_ip_pool;
use nexus_test_utils::resource_helpers::object_create;
use nexus_test_utils::resource_helpers::object_create_error;
Expand All @@ -36,13 +37,15 @@ use nexus_types::external_api::params::IpPoolUpdate;
use nexus_types::external_api::shared::IpRange;
use nexus_types::external_api::shared::Ipv4Range;
use nexus_types::external_api::shared::Ipv6Range;
use nexus_types::external_api::shared::SiloIdentityMode;
use nexus_types::external_api::views::IpPool;
use nexus_types::external_api::views::IpPoolRange;
use nexus_types::external_api::views::IpPoolSilo;
use nexus_types::external_api::views::Silo;
use nexus_types::identity::Resource;
use omicron_common::api::external::IdentityMetadataUpdateParams;
use omicron_common::api::external::NameOrId;
use omicron_common::api::external::SimpleIdentity;
use omicron_common::api::external::{IdentityMetadataCreateParams, Name};
use omicron_nexus::TestInterfaces;
use sled_agent_client::TestInterfaces as SledTestInterfaces;
Expand All @@ -62,16 +65,7 @@ async fn test_ip_pool_basic_crud(cptestctx: &ControlPlaneTestContext) {
let ip_pool_ranges_url = format!("{}/ranges", ip_pool_url);
let ip_pool_add_range_url = format!("{}/add", ip_pool_ranges_url);

// Verify the list of IP pools is empty
let ip_pools = NexusRequest::iter_collection_authn::<IpPool>(
client,
ip_pools_url,
"",
None,
)
.await
.expect("Failed to list IP Pools")
.all_items;
let ip_pools = get_ip_pools(&client).await;
assert_eq!(ip_pools.len(), 0, "Expected empty list of IP pools");

// Verify 404 if the pool doesn't exist yet, both for creating or deleting
Expand Down Expand Up @@ -102,15 +96,7 @@ async fn test_ip_pool_basic_crud(cptestctx: &ControlPlaneTestContext) {
assert_eq!(created_pool.identity.name, pool_name);
assert_eq!(created_pool.identity.description, description);

let list = NexusRequest::iter_collection_authn::<IpPool>(
client,
ip_pools_url,
"",
None,
)
.await
.expect("Failed to list IP Pools")
.all_items;
let list = get_ip_pools(client).await;
assert_eq!(list.len(), 1, "Expected exactly 1 IP pool");
assert_pools_eq(&created_pool, &list[0]);

Expand Down Expand Up @@ -212,6 +198,71 @@ async fn test_ip_pool_basic_crud(cptestctx: &ControlPlaneTestContext) {
.expect("Expected to be able to delete an empty IP Pool");
}

async fn get_ip_pools(client: &ClientTestContext) -> Vec<IpPool> {
NexusRequest::iter_collection_authn::<IpPool>(
client,
"/v1/system/ip-pools",
"",
None,
)
.await
.expect("Failed to list IP Pools")
.all_items
}

// this test exists primarily because of a bug in the initial implementation
// where we included a duplicate of each pool in the list response for every
// associated silo
#[nexus_test]
async fn test_ip_pool_list_dedupe(cptestctx: &ControlPlaneTestContext) {
let client = &cptestctx.external_client;

let ip_pools = get_ip_pools(&client).await;
assert_eq!(ip_pools.len(), 0);

let range1 = IpRange::V4(
Ipv4Range::new(
std::net::Ipv4Addr::new(10, 0, 0, 51),
std::net::Ipv4Addr::new(10, 0, 0, 52),
)
.unwrap(),
);
let (pool1, ..) = create_ip_pool(client, "pool1", Some(range1)).await;
let range2 = IpRange::V4(
Ipv4Range::new(
std::net::Ipv4Addr::new(10, 0, 0, 53),
std::net::Ipv4Addr::new(10, 0, 0, 54),
)
.unwrap(),
);
let (pool2, ..) = create_ip_pool(client, "pool2", Some(range2)).await;

let ip_pools = get_ip_pools(&client).await;
assert_eq!(ip_pools.len(), 2);
assert_eq!(ip_pools[0].identity.id, pool1.id());
assert_eq!(ip_pools[1].identity.id, pool2.id());

// create 3 silos and link
let silo1 =
create_silo(&client, "silo1", true, SiloIdentityMode::SamlJit).await;
link_ip_pool(client, "pool1", &silo1.id(), false).await;
// linking pool2 here only, just for variety
link_ip_pool(client, "pool2", &silo1.id(), false).await;

let silo2 =
create_silo(&client, "silo2", true, SiloIdentityMode::SamlJit).await;
link_ip_pool(client, "pool1", &silo2.id(), true).await;

let silo3 =
create_silo(&client, "silo3", true, SiloIdentityMode::SamlJit).await;
link_ip_pool(client, "pool1", &silo3.id(), true).await;

let ip_pools = get_ip_pools(&client).await;
assert_eq!(ip_pools.len(), 2);
assert_eq!(ip_pools[0].identity.id, pool1.id());
assert_eq!(ip_pools[1].identity.id, pool2.id());
}

/// The internal IP pool, defined by its association with the internal silo,
/// cannot be interacted with through the operator API. CRUD operations should
/// all 404 except fetch by name or ID.
Expand Down

0 comments on commit 4796057

Please sign in to comment.