More WIP

- Add newtype for IP allowlist that checks for empty lists on
  construction. This statically guarantees we have a non-empty list, so
  we can remove some panics and centralize conversion / checking.
- Modify existing firewall rules for the internal services VPC, rather
  than adding new ones

common/src/api/internal/shared.rs

@@ -432,7 +432,77 @@ pub enum AllowedSourceIps {
     /// Restrict access to a specific set of source IP addresses or subnets.
     ///
     /// All others are prevented from reaching rack services.
-    List(Vec<IpNet>),
+    List(IpAllowList),
+}
+
+impl TryFrom<Vec<IpNet>> for AllowedSourceIps {
+    type Error = &'static str;
+    fn try_from(list: Vec<IpNet>) -> Result<Self, Self::Error> {
+        IpAllowList::try_from(list).map(Self::List)
+    }
+}
+
+impl TryFrom<&[IpNetwork]> for AllowedSourceIps {
+    type Error = &'static str;
+    fn try_from(list: &[IpNetwork]) -> Result<Self, Self::Error> {
+        IpAllowList::try_from(list).map(Self::List)
+    }
+}
+
+/// A non-empty allowlist of IP addresses or subnets
+#[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)]
+#[serde(try_from = "Vec<IpNet>", into = "Vec<IpNet>")]
+#[schemars(transparent)]
+pub struct IpAllowList(Vec<IpNet>);
+
+impl IpAllowList {
+    /// Return the entries of the list as a slice.
+    pub fn as_slice(&self) -> &[IpNet] {
+        &self.0
+    }
+
+    /// Return an iterator over the entries of the list.
+    pub fn iter(&self) -> impl Iterator<Item = &IpNet> {
+        self.0.iter()
+    }
+
+    /// Consume the list into an iterator.
+    pub fn into_iter(self) -> impl Iterator<Item = IpNet> {
+        self.0.into_iter()
+    }
+
+    /// Return the number of entries in the allowlist.
+    ///
+    /// Note that this is always >= 1, though we return a usize for simplicity.
+    pub fn len(&self) -> usize {
+        self.0.len()
+    }
+}
+
+impl From<IpAllowList> for Vec<IpNet> {
+    fn from(list: IpAllowList) -> Self {
+        list.0
+    }
+}
+
+impl TryFrom<Vec<IpNet>> for IpAllowList {
+    type Error = &'static str;
+    fn try_from(list: Vec<IpNet>) -> Result<Self, Self::Error> {
+        if list.is_empty() {
+            return Err("IP allowlist must not be empty");
+        }
+        Ok(Self(list))
+    }
+}
+
+impl TryFrom<&[IpNetwork]> for IpAllowList {
+    type Error = &'static str;
+    fn try_from(list: &[IpNetwork]) -> Result<Self, Self::Error> {
+        if list.is_empty() {
+            return Err("IP allowlist must not be empty");
+        }
+        Ok(Self(list.iter().copied().map(Into::into).collect()))
+    }
 }
 
 #[cfg(test)]
@@ -452,7 +522,7 @@ mod tests {
         .unwrap();
         assert_eq!(
             parsed,
-            AllowedSourceIps::List(vec![
+            AllowedSourceIps::try_from(vec![
                 IpNet::from(Ipv4Addr::LOCALHOST),
                 IpNet::V4(Ipv4Net(
                     Ipv4Network::new(Ipv4Addr::new(10, 0, 0, 0), 24).unwrap()
@@ -465,6 +535,7 @@ mod tests {
                     .unwrap()
                 )),
             ])
+            .unwrap()
         );
     }
 

nexus/db-model/src/allowed_source_ip.rs

@@ -15,7 +15,6 @@ use nexus_types::external_api::params;
 use nexus_types::external_api::views;
 use omicron_common::api::external;
 use omicron_common::api::external::Error;
-use omicron_common::api::external::IpNet;
 use serde::Deserialize;
 use serde::Serialize;
 use uuid::Uuid;
@@ -46,20 +45,17 @@ impl AllowedSourceIp {
     }
 
     /// Create an `AllowedSourceIps` type from the contained address.
-    pub fn allowed_source_ips(&self) -> Result<external::AllowedSourceIps, Error> {
+    pub fn allowed_source_ips(
+        &self,
+    ) -> Result<external::AllowedSourceIps, Error> {
         match &self.allowed_ips {
-            Some(list) => {
-                if list.is_empty() {
-                    Err(Error::internal_error(
+            Some(list) => external::AllowedSourceIps::try_from(list.as_slice())
+                .map_err(|_| {
+                    Error::internal_error(
                         "Allowlist from database is empty, but NULL \
-                        should be used to allow any source IP"
-                    ))
-                } else {
-                    Ok(external::AllowedSourceIps::List(
-                        list.iter().copied().map(IpNet::from).collect(),
-                    ))
-                }
-            }
+                        should be used to allow any source IP",
+                    )
+                }),
             None => Ok(external::AllowedSourceIps::Any),
         }
     }
@@ -88,14 +84,10 @@ impl TryFrom<AllowedSourceIp> for views::AllowedSourceIps {
     type Error = Error;
 
     fn try_from(db: AllowedSourceIp) -> Result<Self, Self::Error> {
-        db
-            .allowed_source_ips()
-            .map(|allowed_ips| {
-                Self {
-                    time_created: db.time_created,
-                    time_modified: db.time_modified,
-                    allowed_ips,
-                }
-            })
+        db.allowed_source_ips().map(|allowed_ips| Self {
+            time_created: db.time_created,
+            time_modified: db.time_modified,
+            allowed_ips,
+        })
     }
 }

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

@@ -87,10 +87,9 @@ mod tests {
         datastore::test_utils::datastore_test,
         fixed_data::allowed_source_ips::ALLOWED_SOURCE_IPS_ID,
     };
-    use nexus_db_model::AllowedSourceIp;
     use nexus_test_utils::db::test_setup_database;
     use omicron_common::api::external::{
-        self, Error, IpNet, LookupType, ResourceType,
+        self, Error, LookupType, ResourceType,
     };
     use omicron_test_utils::dev;
 
@@ -114,22 +113,11 @@ mod tests {
             "Expected an ObjectNotFound error when there is no IP allowlist"
         );
 
-        // Helper to get IP addresses from a record.
-        fn get_ips(record: &AllowedSourceIp) -> Vec<IpNet> {
-            record
-                .allowed_ips
-                .as_ref()
-                .expect("Record should have non-NULL allowlist")
-                .iter()
-                .copied()
-                .map(IpNet::from)
-                .collect()
-        }
-
         // Upsert an allowlist, with some specific IPs.
         let ips =
             vec!["10.0.0.0/8".parse().unwrap(), "::1/64".parse().unwrap()];
-        let allowed_ips = external::AllowedSourceIps::List(ips.clone());
+        let allowed_ips =
+            external::AllowedSourceIps::try_from(ips.as_slice()).unwrap();
         let record = datastore
             .allowed_source_ips_upsert(&opctx, allowed_ips)
             .await
@@ -140,14 +128,15 @@ mod tests {
         );
         assert_eq!(
             ips,
-            get_ips(&record),
+            record.allowed_ips.unwrap(),
             "Inserted and re-read incorrect allowed source ips"
         );
 
         // Upsert a new list, verify it's changed.
         let new_ips =
             vec!["10.0.0.0/4".parse().unwrap(), "fd00::10/32".parse().unwrap()];
-        let allowed_ips = external::AllowedSourceIps::List(new_ips.clone());
+        let allowed_ips =
+            external::AllowedSourceIps::try_from(new_ips.as_slice()).unwrap();
         let new_record = datastore
             .allowed_source_ips_upsert(&opctx, allowed_ips)
             .await
@@ -165,8 +154,8 @@ mod tests {
             "Time modified should have changed"
         );
         assert_eq!(
-            new_ips,
-            get_ips(&new_record),
+            &new_ips,
+            new_record.allowed_ips.as_ref().unwrap(),
             "Updated allowed IPs are incorrect"
         );
 
@@ -196,7 +185,8 @@ mod tests {
 
         // Lastly change it back to a real list again.
         let record = new_record;
-        let allowed_ips = external::AllowedSourceIps::List(new_ips.clone());
+        let allowed_ips =
+            external::AllowedSourceIps::try_from(new_ips.as_slice()).unwrap();
         let new_record = datastore
             .allowed_source_ips_upsert(&opctx, allowed_ips)
             .await
@@ -214,8 +204,8 @@ mod tests {
             "Time modified should have changed"
         );
         assert_eq!(
-            new_ips,
-            get_ips(&new_record),
+            &new_ips,
+            new_record.allowed_ips.as_ref().unwrap(),
             "Updated allowed IPs are incorrect"
         );