More bugfixes

The good:
* Added better comments and more thorough testing.

The bad:
The bugfix had bugs.
* We're supposed to SELECT FROM public.omicron.ipv4_nat_entry,
  but I wrote SELECT FROM ipv4_nat_entry. @dap explained a
  little while back that the fully qualified path needs to be
  present when the db is being initialized because we're
  instantiating the tables from a different namespace.
  This is what is breaking CI.

* In the view I wrote version_added AS version twice, when
  the second statement should be version_removed AS version.
  It actually works correctly during initial dendrite startup,
  but will probably have issues with subsequent NAT updates.
  The more thorough testing catches these cases now.

nexus/db-model/src/schema.rs

@@ -13,7 +13,7 @@ use omicron_common::api::external::SemverVersion;
 ///
 /// This should be updated whenever the schema is changed. For more details,
 /// refer to: schema/crdb/README.adoc
-pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(33, 0, 0);
+pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(33, 0, 1);
 
 table! {
     disk (id) {
@@ -546,7 +546,7 @@ table! {
     }
 }
 
-// View used for summarzing changes to ipv4_nat_entry
+// View used for summarizing changes to ipv4_nat_entry
 table! {
     ipv4_nat_changes (version) {
         external_address -> Inet,

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

@@ -872,17 +872,75 @@ mod test {
             datastore.ipv4_nat_delete(&opctx, entry).await.unwrap();
         }
 
-        // ensure that the changeset is ordered
+        // get the new state of all nat entries
+        // note that this is not the method under test
+        let db_records = datastore
+            .ipv4_nat_list_since_version(&opctx, 0, 300)
+            .await
+            .unwrap();
+
+        // ensure that the changeset is ordered, displaying the correct
+        // version numbers, and displaying the correct `deleted` status
         let mut version = 0;
         let limit = 100;
         let mut changes =
             datastore.ipv4_nat_changeset(&opctx, version, limit).await.unwrap();
 
         while !changes.is_empty() {
+            // check ordering
             assert!(changes
                 .windows(2)
                 .all(|entries| entries[0].gen < entries[1].gen));
 
+            // check deleted status and version numbers
+            changes.iter().for_each(|change| match change.deleted {
+                true => {
+                    // version should match a deleted entry
+                    let deleted_nat = db_records
+                        .iter()
+                        .find(|entry| entry.version_removed == Some(change.gen))
+                        .expect("did not find a deleted nat entry with a matching version number");
+
+                    assert_eq!(
+                        deleted_nat.external_address.ip(),
+                        change.external_address
+                    );
+                    assert_eq!(
+                        deleted_nat.first_port,
+                        change.first_port.into()
+                    );
+                    assert_eq!(deleted_nat.last_port, change.last_port.into());
+                    assert_eq!(
+                        deleted_nat.sled_address.ip(),
+                        change.sled_address
+                    );
+                    assert_eq!(*deleted_nat.mac, change.mac);
+                    assert_eq!(deleted_nat.vni.0, change.vni);
+                }
+                false => {
+                    // version should match an active nat entry
+                    let added_nat = db_records
+                        .iter()
+                        .find(|entry| entry.version_added == change.gen)
+                        .expect("did not find an active nat entry with a matching version number");
+
+                    assert!(added_nat.version_removed.is_none());
+
+                    assert_eq!(
+                        added_nat.external_address.ip(),
+                        change.external_address
+                    );
+                    assert_eq!(added_nat.first_port, change.first_port.into());
+                    assert_eq!(added_nat.last_port, change.last_port.into());
+                    assert_eq!(
+                        added_nat.sled_address.ip(),
+                        change.sled_address
+                    );
+                    assert_eq!(*added_nat.mac, change.mac);
+                    assert_eq!(added_nat.vni.0, change.vni);
+                }
+            });
+
             version = changes.last().unwrap().gen;
             changes = datastore
                 .ipv4_nat_changeset(&opctx, version, limit)

schema/crdb/33.0.1/up01.sql

@@ -0,0 +1 @@
+DROP VIEW IF EXISTS omicron.public.ipv4_nat_changes;

schema/crdb/33.0.1/up02.sql

@@ -0,0 +1,60 @@
+/*
+ * A view of the ipv4 nat change history
+ * used to summarize changes for external viewing
+ */
+CREATE VIEW IF NOT EXISTS omicron.public.ipv4_nat_changes
+AS
+-- Subquery:
+-- We need to be able to order partial changesets. ORDER BY on separate columns
+-- will not accomplish this, so we'll do this by interleaving version_added
+-- and version_removed (version_removed taking priority if NOT NULL) and then sorting
+-- on the appropriate version numbers at call time.
+WITH interleaved_versions AS (
+  -- fetch all active NAT entries (entries that have not been soft deleted)
+  SELECT
+    external_address,
+    first_port,
+    last_port,
+    sled_address,
+    vni,
+    mac,
+    -- rename version_added to version
+    version_added AS version,
+    -- create a new virtual column, boolean value representing whether or not
+    -- the record has been soft deleted
+    (version_removed IS NOT NULL) as deleted
+  FROM omicron.public.ipv4_nat_entry
+  WHERE version_removed IS NULL
+
+  -- combine the datasets, unifying the version_added and version_removed
+  -- columns to a single `version` column so we can interleave and sort the entries
+  UNION
+
+  -- fetch all inactive NAT entries (entries that have been soft deleted)
+  SELECT
+    external_address,
+    first_port,
+    last_port,
+    sled_address,
+    vni,
+    mac,
+    -- rename version_removed to version
+    version_removed AS version,
+    -- create a new virtual column, boolean value representing whether or not
+    -- the record has been soft deleted
+    (version_removed IS NOT NULL) as deleted
+  FROM omicron.public.ipv4_nat_entry
+  WHERE version_removed IS NOT NULL
+)
+-- this is our new "table"
+-- here we select the columns from the subquery defined above
+SELECT
+  external_address,
+  first_port,
+  last_port,
+  sled_address,
+  vni,
+  mac,
+  version,
+  deleted
+FROM interleaved_versions;

schema/crdb/dbinit.sql

@@ -3436,38 +3436,56 @@ STORING (
   time_deleted
 );
 
-/**
+/*
  * A view of the ipv4 nat change history
  * used to summarize changes for external viewing
  */
 CREATE VIEW IF NOT EXISTS omicron.public.ipv4_nat_changes
 AS
+-- Subquery:
+-- We need to be able to order partial changesets. ORDER BY on separate columns
+-- will not accomplish this, so we'll do this by interleaving version_added
+-- and version_removed (version_removed taking priority if NOT NULL) and then sorting
+-- on the appropriate version numbers at call time.
 WITH interleaved_versions AS (
+  -- fetch all active NAT entries (entries that have not been soft deleted)
   SELECT
     external_address,
     first_port,
     last_port,
     sled_address,
     vni,
     mac,
+    -- rename version_added to version
     version_added AS version,
+    -- create a new virtual column, boolean value representing whether or not
+    -- the record has been soft deleted
     (version_removed IS NOT NULL) as deleted
-  FROM ipv4_nat_entry
+  FROM omicron.public.ipv4_nat_entry
   WHERE version_removed IS NULL
 
+  -- combine the datasets, unifying the version_added and version_removed
+  -- columns to a single `version` column so we can interleave and sort the entries
   UNION
 
+  -- fetch all inactive NAT entries (entries that have been soft deleted)
   SELECT
     external_address,
     first_port,
     last_port,
     sled_address,
     vni,
     mac,
-    version_added AS version,
+    -- rename version_removed to version
+    version_removed AS version,
+    -- create a new virtual column, boolean value representing whether or not
+    -- the record has been soft deleted
     (version_removed IS NOT NULL) as deleted
-  FROM ipv4_nat_entry WHERE version_removed IS NOT NULL
+  FROM omicron.public.ipv4_nat_entry
+  WHERE version_removed IS NOT NULL
 )
+-- this is our new "table"
+-- here we select the columns from the subquery defined above
 SELECT
   external_address,
   first_port,
@@ -3486,7 +3504,7 @@ INSERT INTO omicron.public.db_metadata (
     version,
     target_version
 ) VALUES
-    ( TRUE, NOW(), NOW(), '33.0.0', NULL)
+    ( TRUE, NOW(), NOW(), '33.0.1', NULL)
 ON CONFLICT DO NOTHING;
 
 COMMIT;