[review] move mvlan and switch port uplinks (for mcast egress) out of pools

Author: zeeshanlakhani

docs/control-plane-architecture.adoc

@@ -14,6 +14,8 @@ NOTE: Much of this material originally came from <<rfd48>> and <<rfd61>>.  This
 
 NOTE: The RFD references in this documentation may be Oxide-internal.  Where possible, we're trying to move relevant documentation from those RFDs into docs here.
 
+See also: link:../notes/multicast-architecture.adoc[Multicast Architecture: VLAN Scope]
+
 == What is the control plane
 
 In software systems the terms **data plane** and **control plane** are often used to refer to the parts of the system that directly provide resources to users (the data plane) and the parts that support the configuration, control, monitoring, and operation of the system (the control plane).  Within the Oxide system, we say that the data plane comprises those parts that provide CPU resources (including both the host CPU and hypervisor software), storage resources, and network resources.  The control plane provides the APIs through which users provision, configure, and monitor these resources and the mechanisms through which these APIs are implemented.  Also part of the control plane are the APIs and facilities through which operators manage the system itself, including fault management, alerting, software updates for various components of the system, and so on.

docs/networking.adoc

@@ -6,6 +6,8 @@
 
 This is a very rough introduction to how networking works within the Oxide system and particularly the control plane (Omicron).  Much more information is available in various RFDs, particularly <<rfd63>>.
 
+See also: link:../notes/multicast-architecture.adoc[Multicast Architecture: VLAN Scope]
+
 == IPv6: the least you need to know
 
 While IPv4 can be used for connectivity between Omicron and the outside world, everything else in the system uses IPv6.  This section provides a _very_ cursory introduction to IPv6 for people only familiar with IPv4.  You can skip this if you know IPv6.  If you want slightly more detail than what's here, see https://www.roesen.org/files/ipv6_cheat_sheet.pdf[this cheat sheet].

end-to-end-tests/src/bin/bootstrap.rs

@@ -53,9 +53,7 @@ async fn run_test() -> Result<()> {
             name: pool_name.parse().unwrap(),
             description: "Default IP pool".to_string(),
             ip_version,
-            mvlan: None,
             pool_type: IpPoolType::Unicast,
-            switch_port_uplinks: None,
         })
         .send()
         .await?;

end-to-end-tests/src/bin/commtest.rs

@@ -295,9 +295,7 @@ async fn rack_prepare(
                         name: pool_name.parse().unwrap(),
                         description: "Default IP pool".to_string(),
                         ip_version,
-                        mvlan: None,
                         pool_type: IpPoolType::Unicast,
-                        switch_port_uplinks: None,
                     })
                     .send()
                     .await?;

nexus/db-model/src/ip_pool.rs

@@ -5,7 +5,6 @@
 //! Model types for IP Pools and the CIDR blocks therein.
 
 use crate::Name;
-use crate::SqlU16;
 use crate::collection::DatastoreCollectionConfig;
 use crate::impl_enum_type;
 use chrono::DateTime;
@@ -21,7 +20,6 @@ use nexus_types::external_api::shared;
 use nexus_types::external_api::views;
 use nexus_types::identity::Resource;
 use omicron_common::api::external;
-use omicron_common::vlan::VlanID;
 use std::net::IpAddr;
 use uuid::Uuid;
 
@@ -105,12 +103,6 @@ pub struct IpPool {
     pub ip_version: IpVersion,
     /// Pool type for unicast (default) vs multicast pools.
     pub pool_type: IpPoolType,
-    /// Switch port uplinks for multicast pools (array of switch port UUIDs).
-    /// Only applies to multicast pools, None for unicast pools.
-    pub switch_port_uplinks: Option<Vec<Uuid>>,
-    /// MVLAN ID for multicast pools.
-    /// Only applies to multicast pools, None for unicast pools.
-    pub mvlan: Option<SqlU16>,
     /// Child resource generation number, for optimistic concurrency control of
     /// the contained ranges.
     pub rcgen: i64,
@@ -129,8 +121,6 @@ impl IpPool {
             ),
             ip_version,
             pool_type: IpPoolType::Unicast,
-            switch_port_uplinks: None,
-            mvlan: None,
             rcgen: 0,
         }
     }
@@ -139,8 +129,6 @@ impl IpPool {
     pub fn new_multicast(
         pool_identity: &external::IdentityMetadataCreateParams,
         ip_version: IpVersion,
-        switch_port_uplinks: Option<Vec<Uuid>>,
-        mvlan: Option<VlanID>,
     ) -> Self {
         Self {
             identity: IpPoolIdentity::new(
@@ -149,8 +137,6 @@ impl IpPool {
             ),
             ip_version,
             pool_type: IpPoolType::Multicast,
-            switch_port_uplinks,
-            mvlan: mvlan.map(|vid| u16::from(vid).into()),
             rcgen: 0,
         }
     }
@@ -173,23 +159,10 @@ impl From<IpPool> for views::IpPool {
         let identity = pool.identity();
         let pool_type = pool.pool_type;
 
-        // Note: UUIDs expected to be converted to "switch.port" format in app
-        // layer, upon retrieval.
-        let switch_port_uplinks = match pool.switch_port_uplinks {
-            Some(uuid_list) => Some(
-                uuid_list.into_iter().map(|uuid| uuid.to_string()).collect(),
-            ),
-            None => None,
-        };
-
-        let mvlan = pool.mvlan.map(|vlan| vlan.into());
-
         Self {
             identity,
             pool_type: pool_type.into(),
             ip_version: pool.ip_version.into(),
-            switch_port_uplinks,
-            mvlan,
         }
     }
 }
@@ -203,22 +176,14 @@ impl From<IpPool> for views::IpPool {
 pub struct IpPoolUpdate {
     pub name: Option<Name>,
     pub description: Option<String>,
-    /// Switch port uplinks for multicast pools (array of switch port UUIDs),
-    /// used for multicast traffic outbound from the rack to external networks.
-    pub switch_port_uplinks: Option<Vec<Uuid>>,
-    /// MVLAN ID for multicast pools.
-    pub mvlan: Option<SqlU16>,
     pub time_modified: DateTime<Utc>,
 }
 
-// Used for unicast updates.
 impl From<params::IpPoolUpdate> for IpPoolUpdate {
     fn from(params: params::IpPoolUpdate) -> Self {
         Self {
             name: params.identity.name.map(|n| n.into()),
             description: params.identity.description,
-            switch_port_uplinks: None, // no change
-            mvlan: None,               // no change
             time_modified: Utc::now(),
         }
     }

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

@@ -1818,8 +1818,6 @@ mod test {
                 ip_version,
                 rcgen: 0,
                 pool_type: IpPoolType::Unicast,
-                mvlan: None,
-                switch_port_uplinks: None,
             };
             let pool = datastore
                 .ip_pool_create(&opctx, params)
@@ -2187,7 +2185,7 @@ mod test {
         let pool = datastore
             .ip_pool_create(
                 &opctx,
-                IpPool::new_multicast(&identity, IpVersion::V4, None, None),
+                IpPool::new_multicast(&identity, IpVersion::V4),
             )
             .await
             .expect("Failed to create multicast IP pool");
@@ -2257,7 +2255,7 @@ mod test {
         let pool = datastore
             .ip_pool_create(
                 &opctx,
-                IpPool::new_multicast(&identity, IpVersion::V4, None, None),
+                IpPool::new_multicast(&identity, IpVersion::V4),
             )
             .await
             .expect("Failed to create multicast IP pool");
@@ -2306,8 +2304,6 @@ mod test {
                 IpPool::new_multicast(
                     &ipv4_identity,
                     IpVersion::V4,
-                    None,
-                    None,
                 ),
             )
             .await
@@ -2348,8 +2344,6 @@ mod test {
                 IpPool::new_multicast(
                     &ipv6_identity,
                     IpVersion::V6,
-                    None,
-                    None,
                 ),
             )
             .await

nexus/db-schema/src/schema.rs

@@ -630,8 +630,6 @@ table! {
         time_deleted -> Nullable<Timestamptz>,
         ip_version -> crate::enums::IpVersionEnum,
         pool_type -> crate::enums::IpPoolTypeEnum,
-        switch_port_uplinks -> Nullable<Array<Uuid>>,
-        mvlan -> Nullable<Int4>,
         rcgen -> Int8,
     }
 }

nexus/src/app/ip_pool.rs

@@ -6,8 +6,6 @@
 
 use crate::external_api::params;
 use crate::external_api::shared;
-use crate::external_api::views;
-use chrono::Utc;
 use ipnetwork::IpNetwork;
 use nexus_db_lookup::LookupPath;
 use nexus_db_lookup::lookup;
@@ -80,38 +78,12 @@ impl super::Nexus {
         // https://github.com/oxidecomputer/omicron/issues/8881
         let ip_version = pool_params.ip_version.into();
 
-        let pool = match (
-            pool_params.pool_type.clone(),
-            pool_params.switch_port_uplinks.is_some(),
-        ) {
-            (shared::IpPoolType::Unicast, true) => {
-                return Err(Error::invalid_request(
-                    "switch_port_uplinks are only allowed for multicast IP pools",
-                ));
-            }
-            (shared::IpPoolType::Unicast, false) => {
-                if pool_params.mvlan.is_some() {
-                    return Err(Error::invalid_request(
-                        "mvlan is only allowed for multicast IP pools",
-                    ));
-                }
+        let pool = match pool_params.pool_type.clone() {
+            shared::IpPoolType::Unicast => {
                 IpPool::new(&pool_params.identity, ip_version)
             }
-            (shared::IpPoolType::Multicast, _) => {
-                let switch_port_ids = self
-                    .resolve_switch_port_ids(
-                        opctx,
-                        self.rack_id(),
-                        &pool_params.switch_port_uplinks,
-                    )
-                    .await?;
-
-                IpPool::new_multicast(
-                    &pool_params.identity,
-                    ip_version,
-                    switch_port_ids,
-                    pool_params.mvlan,
-                )
+            shared::IpPoolType::Multicast => {
+                IpPool::new_multicast(&pool_params.identity, ip_version)
             }
         };
 
@@ -316,21 +288,7 @@ impl super::Nexus {
             return Err(not_found_from_lookup(pool_lookup));
         }
 
-        let switch_port_ids = self
-            .resolve_switch_port_ids(
-                opctx,
-                self.rack_id(),
-                &updates.switch_port_uplinks,
-            )
-            .await?;
-
-        let updates_db = IpPoolUpdate {
-            name: updates.identity.name.clone().map(Into::into),
-            description: updates.identity.description.clone(),
-            switch_port_uplinks: switch_port_ids,
-            mvlan: updates.mvlan.map(|vid| u16::from(vid).into()),
-            time_modified: Utc::now(),
-        };
+        let updates_db = IpPoolUpdate::from(updates.clone());
 
         self.db_datastore.ip_pool_update(opctx, &authz_pool, updates_db).await
     }
@@ -544,99 +502,4 @@ impl super::Nexus {
         opctx.authorize(authz::Action::Modify, &authz_pool).await?;
         self.db_datastore.ip_pool_delete_range(opctx, &authz_pool, range).await
     }
-
-    async fn resolve_switch_port_ids(
-        &self,
-        opctx: &OpContext,
-        rack_id: Uuid,
-        uplinks: &Option<Vec<params::SwitchPortUplink>>,
-    ) -> Result<Option<Vec<Uuid>>, Error> {
-        match uplinks {
-            None => Ok(None),
-            Some(list) => {
-                let mut ids = Vec::with_capacity(list.len());
-
-                for uplink in list {
-                    let switch_location =
-                        Name::from(uplink.switch_location.clone());
-                    let port_name = Name::from(uplink.port_name.clone());
-                    let id = self
-                        .db_datastore
-                        .switch_port_get_id(
-                            opctx,
-                            rack_id,
-                            switch_location,
-                            port_name,
-                        )
-                        .await
-                        .map_err(|_| {
-                            Error::invalid_value(
-                                "switch_port_uplinks",
-                                format!("Switch port '{}' not found", uplink),
-                            )
-                        })?;
-                    ids.push(id);
-                }
-                Ok(Some(ids))
-            }
-        }
-    }
-
-    /// Convert IP pool with proper switch port name resolution in an async
-    /// context.
-    pub(crate) async fn ip_pool_to_view(
-        &self,
-        opctx: &OpContext,
-        pool: db::model::IpPool,
-    ) -> Result<views::IpPool, Error> {
-        let identity = pool.identity();
-        let pool_type = pool.pool_type;
-
-        // Convert switch port UUIDs to "switch.port" format
-        let switch_port_uplinks = self
-            .resolve_switch_port_names(opctx, &pool.switch_port_uplinks)
-            .await?;
-
-        let mvlan = pool.mvlan.map(|vlan| vlan.into());
-
-        Ok(views::IpPool {
-            identity,
-            ip_version: pool.ip_version.into(),
-            pool_type: pool_type.into(),
-            switch_port_uplinks,
-            mvlan,
-        })
-    }
-
-    // Convert switch port UUIDs to "switch.port" format for views
-    async fn resolve_switch_port_names(
-        &self,
-        opctx: &OpContext,
-        switch_port_ids: &Option<Vec<Uuid>>,
-    ) -> Result<Option<Vec<String>>, Error> {
-        match switch_port_ids {
-            None => Ok(None),
-            Some(ids) => {
-                let mut names = Vec::with_capacity(ids.len());
-                for &id in ids {
-                    let switch_port = self
-                        .db_datastore
-                        .switch_port_get(opctx, id)
-                        .await
-                        .map_err(|_| {
-                            Error::internal_error(&format!(
-                                "Switch port with ID {} not found",
-                                id
-                            ))
-                        })?;
-                    let name = format!(
-                        "{}.{}",
-                        switch_port.switch_location, switch_port.port_name
-                    );
-                    names.push(name);
-                }
-                Ok(Some(names))
-            }
-        }
-    }
 }

nexus/src/external_api/http_entrypoints.rs

@@ -1775,8 +1775,7 @@ impl NexusExternalApi for NexusExternalApiImpl {
             let opctx =
                 crate::context::op_context_for_external_api(&rqctx).await?;
             let pool = nexus.ip_pool_create(&opctx, &pool_params).await?;
-            let pool_view = nexus.ip_pool_to_view(&opctx, pool).await?;
-            Ok(HttpResponseCreated(pool_view))
+            Ok(HttpResponseCreated(pool.into()))
         };
         apictx
             .context

nexus/tests/integration_tests/endpoints.rs

@@ -953,8 +953,6 @@ pub static DEMO_IP_POOL_UPDATE: LazyLock<params::IpPoolUpdate> =
             name: None,
             description: Some(String::from("a new IP pool")),
         },
-        mvlan: None,
-        switch_port_uplinks: None,
     });
 pub static DEMO_IP_POOL_SILOS_URL: LazyLock<String> =
     LazyLock::new(|| format!("{}/silos", *DEMO_IP_POOL_URL));