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

Fix duplicate entries in IP pools list #4808

Merged
merged 4 commits into from
Jan 16, 2024
Merged

Fix duplicate entries in IP pools list #4808

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
09f354f
fix list ip pools by deduping results of left outer join
david-crespo Jan 12, 2024
6c0518c
use service IP pool name to filter out internal pool from normal endp…
david-crespo Jan 12, 2024
d9d8701
use name lookup helper to look up thing by name!
david-crespo Jan 12, 2024
5896550
fix auth test, which I think is more correct now
david-crespo Jan 16, 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
59 changes: 7 additions & 52 deletions nexus/db-queries/src/db/datastore/ip_pool.rs
View file
Open in desktop
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))
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, so much better 🙌

.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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦

Copy link
Contributor

Choose a reason for hiding this comment

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

I count this as a win, haha.

}

/// 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
Open in desktop
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
Open in desktop
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
Loading