Bugfixes for #4589 and #4611 (#4610)

Bugfix for issue #4589.

The root cause `ensure_ipv4_nat_entry` previously would match against
*any* existing table entries with the matching parameters. We need it to
only match against entries that are *active*, or in implementation
terms, entries whose `version_removed` column is `NULL`.

The events triggering the bug is as follows:
1. User creates a new instance, eventually triggering the creation of
new ipv4 nat entries, which are reconciled by the downstream dendrite
workflow.
2. User stops the instance. This triggers the soft-deletion of the ipv4
nat entries, which are again reconciled by the downstream dendrite
workflow.
3. The user restarts the instance. In the event that Nexus places the
instance back on the same sled as last time, the `external_ip` may have
the same parameters used by the soft-deleted nat records. Since we
previously were not filtering for `version_removed = NULL` in
`ensure_ipv4_nat_entry`, the soft-deleted records would still be treated
as "live" in our db query, causing Nexus to skip inserting new nat
records when the instance restarts.

This PR should resolve this unwanted behavior. However, a second issue
was noticed during verification of the bug fix. I noticed that when
running `swadm nat list`, the entries did not re-appear in the output
even though `dendrite` was indeed picking up the new additions and
configuring the softnpu asic accordingly. I believe this was also
something @askfongjojo reported in chat. This means that we could have
live entries on the switch and external traffic flowing to an instance,
even though the nat entry is not appearing in `swadm nat list`. This PR
also includes an upgraded dendrite that resolves that bug.

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"