More WIP after review feedback

- Remove default allowlist impl
- Fallible construction of allowlist from DB repr
- Remove generation from DB

bootstore/src/schemes/v0/peer.rs

@@ -52,8 +52,8 @@ pub enum NodeRequestError {
     Send,
 
     #[error(
-        "Network config update failed because it is out of date. Attempted
-        update generation: {attempted_update_generation}, current generation:
+        "Network config update failed because it is out of date. Attempted \
+        update generation: {attempted_update_generation}, current generation: \
         {current_generation}"
     )]
     StaleNetworkConfig {

clients/nexus-client/src/lib.rs

@@ -419,3 +419,57 @@ impl TryFrom<types::ProducerEndpoint>
         })
     }
 }
+
+impl TryFrom<&omicron_common::api::external::Ipv4Net> for types::Ipv4Net {
+    type Error = String;
+
+    fn try_from(
+        net: &omicron_common::api::external::Ipv4Net,
+    ) -> Result<Self, Self::Error> {
+        types::Ipv4Net::try_from(net.to_string()).map_err(|e| e.to_string())
+    }
+}
+
+impl TryFrom<&omicron_common::api::external::Ipv6Net> for types::Ipv6Net {
+    type Error = String;
+
+    fn try_from(
+        net: &omicron_common::api::external::Ipv6Net,
+    ) -> Result<Self, Self::Error> {
+        types::Ipv6Net::try_from(net.to_string()).map_err(|e| e.to_string())
+    }
+}
+
+impl TryFrom<&omicron_common::api::external::IpNet> for types::IpNet {
+    type Error = String;
+
+    fn try_from(
+        net: &omicron_common::api::external::IpNet,
+    ) -> Result<Self, Self::Error> {
+        use omicron_common::api::external::IpNet;
+        match net {
+            IpNet::V4(v4) => types::Ipv4Net::try_from(v4).map(types::IpNet::V4),
+            IpNet::V6(v6) => types::Ipv6Net::try_from(v6).map(types::IpNet::V6),
+        }
+    }
+}
+
+impl TryFrom<&omicron_common::api::external::AllowedSourceIps>
+    for types::AllowedSourceIps
+{
+    type Error = String;
+
+    fn try_from(
+        ips: &omicron_common::api::external::AllowedSourceIps,
+    ) -> Result<Self, Self::Error> {
+        use omicron_common::api::external::AllowedSourceIps;
+        match ips {
+            AllowedSourceIps::Any => Ok(types::AllowedSourceIps::Any),
+            AllowedSourceIps::List(list) => list
+                .iter()
+                .map(TryInto::try_into)
+                .collect::<Result<_, _>>()
+                .map(types::AllowedSourceIps::List),
+        }
+    }
+}

common/src/api/internal/shared.rs

@@ -424,13 +424,10 @@ impl fmt::Display for PortFec {
 }
 
 /// Description of source IPs allowed to reach rack services.
-#[derive(
-    Clone, Debug, Default, Deserialize, JsonSchema, PartialEq, Serialize,
-)]
+#[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)]
 #[serde(rename_all = "snake_case", tag = "allow", content = "ips")]
 pub enum AllowedSourceIps {
     /// Allow traffic from any external IP address.
-    #[default]
     Any,
     /// Restrict access to a specific set of source IP addresses or subnets.
     ///
@@ -450,7 +447,7 @@ mod tests {
     #[test]
     fn test_deserialize_allowed_source_ips() {
         let parsed: AllowedSourceIps = serde_json::from_str(
-            "{ \"allow\" : \"list\", \"ips\" : [\"127.0.0.1\", \"10.0.0.0/24\", \"fd00::1/64\"]}",
+            r#"{"allow":"list","ips":["127.0.0.1","10.0.0.0/24","fd00::1/64"]}"#,
         )
         .unwrap();
         assert_eq!(
@@ -473,7 +470,7 @@ mod tests {
 
     #[test]
     fn test_deserialize_unknown_string() {
-        serde_json::from_str::<AllowedSourceIps>("{\"allow\": \"wat\"}")
+        serde_json::from_str::<AllowedSourceIps>(r#"{"allow":"wat"}"#)
             .expect_err(
                 "Should not be able to deserialize from unknown variant name",
             );
@@ -483,7 +480,7 @@ mod tests {
     fn test_deserialize_any_into_allowed_external_ips() {
         assert_eq!(
             AllowedSourceIps::Any,
-            serde_json::from_str("{\"allow\": \"any\"}").unwrap(),
+            serde_json::from_str(r#"{"allow":"any"}"#).unwrap(),
         );
     }
 }

nexus/db-model/src/allowed_source_ip.rs

@@ -8,13 +8,13 @@
 //! IP allowlisting.
 
 use crate::schema::allowed_source_ip;
-use crate::Generation;
 use chrono::DateTime;
 use chrono::Utc;
 use ipnetwork::IpNetwork;
 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;
@@ -29,7 +29,6 @@ pub struct AllowedSourceIp {
     pub id: Uuid,
     pub time_created: DateTime<Utc>,
     pub time_modified: DateTime<Utc>,
-    pub generation: Generation,
     pub allowed_ips: Option<Vec<IpNetwork>>,
 }
 
@@ -43,12 +42,25 @@ impl AllowedSourceIp {
                 Some(list.into_iter().map(Into::into).collect())
             }
         };
-        Self {
-            id,
-            time_created: now,
-            time_modified: now,
-            generation: Generation::new(),
-            allowed_ips,
+        Self { id, time_created: now, time_modified: now, allowed_ips }
+    }
+
+    /// Create an `AllowedSourceIps` type from the contained address.
+    pub fn allowed_source_ips(&self) -> Result<external::AllowedSourceIps, Error> {
+        match &self.allowed_ips {
+            Some(list) => {
+                if list.is_empty() {
+                    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(),
+                    ))
+                }
+            }
+            None => Ok(external::AllowedSourceIps::Any),
         }
     }
 }
@@ -72,23 +84,18 @@ impl From<params::AllowedSourceIpsUpdate> for AllowedSourceIpUpdate {
     }
 }
 
-impl From<AllowedSourceIp> for views::AllowedSourceIps {
-    fn from(db: AllowedSourceIp) -> Self {
-        let allowed_ips = if let Some(list) = db.allowed_ips {
-            assert!(
-                !list.is_empty(),
-                "IP allowlist must not be empty, use NULL instead"
-            );
-            external::AllowedSourceIps::List(
-                list.into_iter().map(IpNet::from).collect(),
-            )
-        } else {
-            external::AllowedSourceIps::Any
-        };
-        Self {
-            time_created: db.time_created,
-            time_modified: db.time_modified,
-            allowed_ips,
-        }
+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,
+                }
+            })
     }
 }

nexus/db-model/src/schema.rs

@@ -1629,7 +1629,6 @@ table! {
         id -> Uuid,
         time_created -> Timestamptz,
         time_modified -> Timestamptz,
-        generation -> Int8,
         allowed_ips -> Nullable<Array<Inet>>,
     }
 }

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

@@ -54,12 +54,8 @@ impl super::DataStore {
         allowed_ips: AllowedSourceIps,
     ) -> Result<AllowedSourceIp, Error> {
         let conn = self.pool_connection_authorized(opctx).await?;
-        Self::allowed_source_ips_upsert_on_connection(
-            opctx,
-            &conn,
-            allowed_ips,
-        )
-        .await
+        Self::allowed_source_ips_upsert_on_connection(opctx, &conn, allowed_ips)
+            .await
     }
 
     pub(crate) async fn allowed_source_ips_upsert_on_connection(
@@ -78,7 +74,6 @@ impl super::DataStore {
             .set((
                 dsl::allowed_ips.eq(record.allowed_ips),
                 dsl::time_modified.eq(record.time_modified),
-                dsl::generation.eq(dsl::generation + 1),
             ))
             .get_result_async(conn)
             .await
@@ -143,11 +138,6 @@ mod tests {
             record.id, ALLOWED_SOURCE_IPS_ID,
             "Record should have hard-coded allowlist ID"
         );
-        assert_eq!(
-            u64::from(record.generation.0),
-            1,
-            "Generation should start at 1"
-        );
         assert_eq!(
             ips,
             get_ips(&record),
@@ -166,11 +156,6 @@ mod tests {
             new_record.id, ALLOWED_SOURCE_IPS_ID,
             "Record should have hard-coded allowlist ID"
         );
-        assert_eq!(
-            u64::from(new_record.generation.0),
-            2,
-            "Generation should now be 2"
-        );
         assert_eq!(
             record.time_created, new_record.time_created,
             "Time created should not have changed"
@@ -196,11 +181,6 @@ mod tests {
             new_record.id, ALLOWED_SOURCE_IPS_ID,
             "Record should have hard-coded allowlist ID"
         );
-        assert_eq!(
-            u64::from(new_record.generation.0),
-            3,
-            "Generation should now be 3"
-        );
         assert_eq!(
             record.time_created, new_record.time_created,
             "Time created should not have changed"
@@ -225,11 +205,6 @@ mod tests {
             new_record.id, ALLOWED_SOURCE_IPS_ID,
             "Record should have hard-coded allowlist ID"
         );
-        assert_eq!(
-            u64::from(new_record.generation.0),
-            4,
-            "Generation should now be 4"
-        );
         assert_eq!(
             record.time_created, new_record.time_created,
             "Time created should not have changed"

nexus/src/app/allowed_source_ips.rs

@@ -7,7 +7,6 @@
 //! Nexus methods for operating on source IP allowlists.
 
 use std::net::IpAddr;
-
 use nexus_db_queries::context::OpContext;
 use nexus_types::external_api::params;
 use nexus_types::external_api::views::AllowedSourceIps;
@@ -23,7 +22,7 @@ impl super::Nexus {
         self.db_datastore
             .allowed_source_ips_view(opctx)
             .await
-            .map(AllowedSourceIps::from)
+            .and_then(AllowedSourceIps::try_from)
     }
 
     /// Upsert the allowlist of source IPs that can reach user-facing services.
@@ -56,15 +55,9 @@ impl super::Nexus {
             // the request came from is on the allowlist. This is our only real
             // guardrail to prevent accidentally preventing any future access to
             // the rack!
+            let mut contains_remote = false;
             for entry in list.iter() {
-                if !entry.contains(remote_addr) {
-                    return Err(Error::invalid_request(
-                        "The source IP allow list would prevent access \
-                        from the current client! Ensure that the allowlist \
-                        contains an entry that continues to allow access \
-                        from this peer.",
-                    ));
-                }
+                contains_remote = entry.contains(remote_addr);
                 if entry.ip().is_unspecified() {
                     return Err(Error::invalid_request(
                         "Source IP allowlist may not contain the \
@@ -79,6 +72,14 @@ impl super::Nexus {
                     ));
                 }
             }
+            if !contains_remote {
+                return Err(Error::invalid_request(
+                    "The source IP allow list would prevent access \
+                    from the current client! Ensure that the allowlist \
+                    contains an entry that continues to allow access \
+                    from this peer.",
+                ));
+            }
         };
 
         // Actually insert the new allowlist.
@@ -107,7 +108,10 @@ impl super::Nexus {
                     "failed to update sled-agents with new allowlist";
                     "error" => ?e
                 );
-                Err(e)
+                let message = "Failed to plumb allowlist as firewall rules \
+                to relevant sled agents. The request must be retried for them \
+                to take effect.";
+                Err(Error::unavail(message))
             }
         }
     }

nexus/src/lib.rs

@@ -40,7 +40,7 @@ use omicron_common::address::IpRange;
 use omicron_common::api::external::Error;
 use omicron_common::api::internal::nexus::{ProducerEndpoint, ProducerKind};
 use omicron_common::api::internal::shared::{
-    ExternalPortDiscovery, RackNetworkConfig, SwitchLocation,
+    AllowedSourceIps, ExternalPortDiscovery, RackNetworkConfig, SwitchLocation,
 };
 use omicron_common::FileKv;
 use oximeter::types::ProducerRegistry;
@@ -322,7 +322,7 @@ impl nexus_test_interface::NexusServer for Server {
                         bgp: Vec::new(),
                         bfd: Vec::new(),
                     },
-                    allowed_source_ips: Default::default(),
+                    allowed_source_ips: AllowedSourceIps::Any,
                 },
             )
             .await

schema/crdb/add-allowed-source-ips/up.sql

@@ -2,6 +2,5 @@ CREATE TABLE IF NOT EXISTS omicron.public.allowed_source_ip (
     id UUID PRIMARY KEY,
     time_created TIMESTAMPTZ NOT NULL,
     time_modified TIMESTAMPTZ NOT NULL,
-    generation INT8 NOT NULL,
     allowed_ips INET[]
 );

schema/crdb/dbinit.sql

@@ -3765,7 +3765,6 @@ CREATE TABLE IF NOT EXISTS omicron.public.allowed_source_ip (
     id UUID PRIMARY KEY,
     time_created TIMESTAMPTZ NOT NULL,
     time_modified TIMESTAMPTZ NOT NULL,
-    generation INT8 NOT NULL,
     -- A nullable list of allowed source IPs.
     --
     -- NULL is used to indicate _any_ source IP is allowed. A _non-empty_ list