Fix duplicate entries in IP pools list (#4808)

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.

Author: david-crespo

nexus/db-queries/src/db/datastore/ip_pool.rs

@@ -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;
@@ -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)
@@ -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?)
@@ -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.
@@ -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>(

nexus/tests/integration_tests/endpoints.rs

@@ -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
@@ -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
@@ -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(
@@ -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(

nexus/tests/integration_tests/ip_pools.rs

@@ -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;
@@ -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;
@@ -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
@@ -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]);
 
@@ -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.