Bugfixes for #4589 and #4611

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

@@ -36,6 +36,7 @@ impl DataStore {
             .filter(dsl::sled_address.eq(nat_entry.sled_address))
             .filter(dsl::vni.eq(nat_entry.vni))
             .filter(dsl::mac.eq(nat_entry.mac))
+            .filter(dsl::version_removed.is_null())
             .select((
                 dsl::external_address,
                 dsl::first_port,
@@ -275,7 +276,7 @@ mod test {
 
     use crate::db::datastore::datastore_test;
     use chrono::Utc;
-    use nexus_db_model::{Ipv4NatValues, MacAddr, Vni};
+    use nexus_db_model::{Ipv4NatEntry, Ipv4NatValues, MacAddr, Vni};
     use nexus_test_utils::db::test_setup_database;
     use omicron_common::api::external;
     use omicron_test_utils::dev;
@@ -427,7 +428,6 @@ mod test {
             datastore.ipv4_nat_list_since_version(&opctx, 0, 10).await.unwrap();
 
         assert_eq!(nat_entries.len(), 1);
-
         // version should be unchanged
         assert_eq!(
             datastore.ipv4_nat_current_version(&opctx).await.unwrap(),
@@ -437,4 +437,150 @@ mod test {
         db.cleanup().await.unwrap();
         logctx.cleanup_successful();
     }
+
+    #[tokio::test]
+    /// Table design and queries should only insert one active NAT entry for a given
+    /// set of properties, but allow multiple deleted nat entries for the same set
+    /// of properties.
+    async fn table_allows_unique_active_multiple_deleted() {
+        let logctx = dev::test_setup_log("test_nat_version_tracking");
+        let mut db = test_setup_database(&logctx.log).await;
+        let (opctx, datastore) = datastore_test(&logctx, &db).await;
+
+        // We should not have any NAT entries at this moment
+        let initial_state =
+            datastore.ipv4_nat_list_since_version(&opctx, 0, 10).await.unwrap();
+
+        assert!(initial_state.is_empty());
+        assert_eq!(
+            datastore.ipv4_nat_current_version(&opctx).await.unwrap(),
+            0
+        );
+
+        // Each change (creation / deletion) to the NAT table should increment the
+        // version number of the row in the NAT table
+        let external_address = external::Ipv4Net(
+            ipnetwork::Ipv4Network::try_from("10.0.0.100").unwrap(),
+        );
+
+        let sled_address = external::Ipv6Net(
+            ipnetwork::Ipv6Network::try_from("fd00:1122:3344:104::1").unwrap(),
+        );
+
+        // Add a nat entry.
+        let nat1 = Ipv4NatValues {
+            external_address: external_address.into(),
+            first_port: 0.into(),
+            last_port: 999.into(),
+            sled_address: sled_address.into(),
+            vni: Vni(external::Vni::random()),
+            mac: MacAddr(
+                external::MacAddr::from_str("A8:40:25:F5:EB:2A").unwrap(),
+            ),
+        };
+
+        datastore.ensure_ipv4_nat_entry(&opctx, nat1.clone()).await.unwrap();
+
+        // Try to add it again. It should still only result in a single entry.
+        datastore.ensure_ipv4_nat_entry(&opctx, nat1.clone()).await.unwrap();
+        let first_entry = datastore
+            .ipv4_nat_find_by_values(&opctx, nat1.clone())
+            .await
+            .unwrap();
+
+        let nat_entries =
+            datastore.ipv4_nat_list_since_version(&opctx, 0, 10).await.unwrap();
+
+        // The NAT table has undergone one change. One entry has been added,
+        // none deleted, so we should be at version 1.
+        assert_eq!(nat_entries.len(), 1);
+        assert_eq!(nat_entries.last().unwrap().version_added, 1);
+        assert_eq!(
+            datastore.ipv4_nat_current_version(&opctx).await.unwrap(),
+            1
+        );
+
+        datastore.ipv4_nat_delete(&opctx, &first_entry).await.unwrap();
+
+        // The NAT table has undergone two changes. One entry has been added,
+        // then deleted, so we should be at version 2.
+        let nat_entries = datastore
+            .ipv4_nat_list_since_version(&opctx, 0, 10)
+            .await
+            .unwrap()
+            .into_iter();
+
+        let active: Vec<Ipv4NatEntry> = nat_entries
+            .clone()
+            .filter(|entry| entry.version_removed.is_none())
+            .collect();
+
+        let inactive: Vec<Ipv4NatEntry> = nat_entries
+            .filter(|entry| entry.version_removed.is_some())
+            .collect();
+
+        assert!(active.is_empty());
+        assert_eq!(inactive.len(), 1);
+        assert_eq!(
+            datastore.ipv4_nat_current_version(&opctx).await.unwrap(),
+            2
+        );
+
+        // Add the same entry back. This simulates the behavior we will see
+        // when stopping and then restarting an instance.
+        datastore.ensure_ipv4_nat_entry(&opctx, nat1.clone()).await.unwrap();
+
+        // The NAT table has undergone three changes.
+        let nat_entries = datastore
+            .ipv4_nat_list_since_version(&opctx, 0, 10)
+            .await
+            .unwrap()
+            .into_iter();
+
+        let active: Vec<Ipv4NatEntry> = nat_entries
+            .clone()
+            .filter(|entry| entry.version_removed.is_none())
+            .collect();
+
+        let inactive: Vec<Ipv4NatEntry> = nat_entries
+            .filter(|entry| entry.version_removed.is_some())
+            .collect();
+
+        assert_eq!(active.len(), 1);
+        assert_eq!(inactive.len(), 1);
+        assert_eq!(
+            datastore.ipv4_nat_current_version(&opctx).await.unwrap(),
+            3
+        );
+
+        let second_entry =
+            datastore.ipv4_nat_find_by_values(&opctx, nat1).await.unwrap();
+        datastore.ipv4_nat_delete(&opctx, &second_entry).await.unwrap();
+
+        // The NAT table has undergone four changes
+        let nat_entries = datastore
+            .ipv4_nat_list_since_version(&opctx, 0, 10)
+            .await
+            .unwrap()
+            .into_iter();
+
+        let active: Vec<Ipv4NatEntry> = nat_entries
+            .clone()
+            .filter(|entry| entry.version_removed.is_none())
+            .collect();
+
+        let inactive: Vec<Ipv4NatEntry> = nat_entries
+            .filter(|entry| entry.version_removed.is_some())
+            .collect();
+
+        assert_eq!(active.len(), 0);
+        assert_eq!(inactive.len(), 2);
+        assert_eq!(
+            datastore.ipv4_nat_current_version(&opctx).await.unwrap(),
+            4
+        );
+
+        db.cleanup().await.unwrap();
+        logctx.cleanup_successful();
+    }
 }

nexus/src/app/sagas/instance_delete.rs

@@ -8,6 +8,7 @@ use super::ActionRegistry;
 use super::NexusActionContext;
 use super::NexusSaga;
 use crate::app::sagas::declare_saga_actions;
+use nexus_db_queries::db::lookup::LookupPath;
 use nexus_db_queries::{authn, authz, db};
 use omicron_common::api::external::{Error, ResourceType};
 use omicron_common::api::internal::shared::SwitchLocation;
@@ -39,6 +40,9 @@ declare_saga_actions! {
     DEALLOCATE_EXTERNAL_IP -> "no_result3" {
         + sid_deallocate_external_ip
     }
+    INSTANCE_DELETE_NAT -> "no_result4" {
+        + sid_delete_nat
+    }
 }
 
 // instance delete saga: definition
@@ -57,6 +61,7 @@ impl NexusSaga for SagaInstanceDelete {
         _params: &Self::Params,
         mut builder: steno::DagBuilder,
     ) -> Result<steno::Dag, super::SagaInitError> {
+        builder.append(instance_delete_nat_action());
         builder.append(instance_delete_record_action());
         builder.append(delete_network_interfaces_action());
         builder.append(deallocate_external_ip_action());
@@ -110,6 +115,32 @@ async fn sid_delete_network_interfaces(
     Ok(())
 }
 
+async fn sid_delete_nat(
+    sagactx: NexusActionContext,
+) -> Result<(), ActionError> {
+    let params = sagactx.saga_params::<Params>()?;
+    let instance_id = params.authz_instance.id();
+    let osagactx = sagactx.user_data();
+    let opctx = crate::context::op_context_for_saga_action(
+        &sagactx,
+        &params.serialized_authn,
+    );
+
+    let (.., authz_instance) = LookupPath::new(&opctx, &osagactx.datastore())
+        .instance_id(instance_id)
+        .lookup_for(authz::Action::Modify)
+        .await
+        .map_err(ActionError::action_failed)?;
+
+    osagactx
+        .nexus()
+        .instance_delete_dpd_config(&opctx, &authz_instance)
+        .await
+        .map_err(ActionError::action_failed)?;
+
+    Ok(())
+}
+
 async fn sid_deallocate_external_ip(
     sagactx: NexusActionContext,
 ) -> Result<(), ActionError> {

package-manifest.toml

@@ -476,8 +476,8 @@ only_for_targets.image = "standard"
 # 2. Copy dendrite.tar.gz from dendrite/out to omicron/out
 source.type = "prebuilt"
 source.repo = "dendrite"
-source.commit = "2af6adea85c62ac37e451148b84e5eb0ef005f36"
-source.sha256 = "dc93b671cce54e83ed55faaa267f81ba9e65abcd6714aa559d68a8783d73b1c1"
+source.commit = "1b15e62b04044ef2b15c82d8dcef03f6fc24b3d8"
+source.sha256 = "06b5eeedaebf30e96a5c5e932e08034c90947af7a54e9bc04d57d6807013ade9"
 output.type = "zone"
 output.intermediate_only = true
 
@@ -501,8 +501,8 @@ only_for_targets.image = "standard"
 # 2. Copy the output zone image from dendrite/out to omicron/out
 source.type = "prebuilt"
 source.repo = "dendrite"
-source.commit = "2af6adea85c62ac37e451148b84e5eb0ef005f36"
-source.sha256 = "c34b10d47fa3eb9f9f6b3655ea4ed8a726f93399ea177efea79f5c89f2ab5a1e"
+source.commit = "1b15e62b04044ef2b15c82d8dcef03f6fc24b3d8"
+source.sha256 = "51be0b0342bc7cdf927797af45af3bc82861bb8efb174d50958cb16b5620c51d"
 output.type = "zone"
 output.intermediate_only = true
 
@@ -519,8 +519,8 @@ only_for_targets.image = "standard"
 # 2. Copy dendrite.tar.gz from dendrite/out to omicron/out/dendrite-softnpu.tar.gz
 source.type = "prebuilt"
 source.repo = "dendrite"
-source.commit = "2af6adea85c62ac37e451148b84e5eb0ef005f36"
-source.sha256 = "ce7065227c092ee82704f39a966b7441e3ae82d75eedb6eb281bd8b3e5873e32"
+source.commit = "1b15e62b04044ef2b15c82d8dcef03f6fc24b3d8"
+source.sha256 = "9afb24cdae27755eaf86a856268686bb641048b5d450dae858cf47b9daaa46ed"
 output.type = "zone"
 output.intermediate_only = true
 

tools/dendrite_openapi_version

@@ -1,2 +1,2 @@
-COMMIT="2af6adea85c62ac37e451148b84e5eb0ef005f36"
+COMMIT="1b15e62b04044ef2b15c82d8dcef03f6fc24b3d8"
 SHA2="07d115bfa8498a8015ca2a8447efeeac32e24aeb25baf3d5e2313216e11293c0"

tools/dendrite_stub_checksums

@@ -1,3 +1,3 @@
-CIDL_SHA256_ILLUMOS="dc93b671cce54e83ed55faaa267f81ba9e65abcd6714aa559d68a8783d73b1c1"
-CIDL_SHA256_LINUX_DPD="b13b391a085ba6bf16fdd99774f64c9d53cd7220ad518d5839c8558fb925c40c"
-CIDL_SHA256_LINUX_SWADM="6bfa4e367eb2b0be89f1588ac458026a186314597a4feb9fee6cea60101c7ebe"
+CIDL_SHA256_ILLUMOS="06b5eeedaebf30e96a5c5e932e08034c90947af7a54e9bc04d57d6807013ade9"
+CIDL_SHA256_LINUX_DPD="99a800cbd5739245154831004892d47be5a871e37c536ec3009911ddb02fdb16"
+CIDL_SHA256_LINUX_SWADM="e92bfc071f3944523a2e69b13ee877a4fd87cb8a9a78011b4aa8f40218347e25"