NAT RPW #3804

internet-diglett · 2023-07-31T22:58:32Z

Add db table for tracking nat entries
Add endpoint for retrieving changesets
Update instance sagas to update table and trigger RPW
Periodically cleanup soft-deleted entries that no longer
need to be sync'd by dendrite.

The other half of the RPW lives in Dendrite. It will periodically
check for a changeset, or check for a changeset when the trigger
endpoint is called by the relevant saga / nexus operation.

common/src/sql/dbinit.sql

smklein · 2023-09-28T21:30:14Z

nexus/src/internal_api/http_entrypoints.rs

+/// empty vec is returned.
+#[endpoint {
+   method = GET,
+    path = "/rpw/nat/ipv4/changeset/{from_gen}"


tiny nitpick -- I'd just prefix these paths with "/nat", the fact that they're called from Dendrite is an implementation detail

I think I still might remove the /rpw prefix, the fact that Nexus processes this as an RPW is an implementation detail.

internet-diglett · 2023-10-12T21:08:24Z

Testing resolution of oxidecomputer/buildomat#38

* Add db table for tracking nat entries * Add endpoint for retrieving changesets * Update instance sagas to update table and trigger RPW * Periodically cleanup soft-deleted entries that no longer need to be sync'd by dendrite. The other half of the RPW lives in Dendrite. It will periodically check for a changeset, or check for a changeset when the trigger endpoint is called by the relevant saga / nexus operation.

smklein

My first pass here is focused a bit more at the DB than the RPW itself

smklein · 2023-11-06T19:40:04Z

nexus/db-model/src/ipv4_nat_entry.rs

+// TODO correctness
+// If we're not going to store ipv4 and ipv6
+// NAT entries in the same table, and we don't
+// need any of the special properties of the IpNetwork
+// column type, does it make sense to use a different
+// column type?


Couple questions about this:

I don't see anything in this struct that appears IPv4 specific -- doesn't ipnetwork::IpNetwork encapsulate both v4 and v6 addresses? Could this entire struct just be IpNatValues (and same elsewhere in this file) or would that be a bad idea?

If that genuinely is a bad idea, https://docs.rs/ipnetwork/0.20.0/ipnetwork/struct.Ipv4Network.html exists, and we could use that instead?

Either way, we should resolve this comment before merging!

By keeping the tables for v4 and v6 separate, we maintain a 1:1 mapping with the tables in dendrite, which does make troubleshooting, observability, and state reconciliation a bit more straightforward, so my gut says "keep them separate"

I'll try using Ipv4Network unless someone has an objection :)

smklein · 2023-11-06T19:40:29Z

nexus/db-model/src/ipv4_nat_entry.rs

+// TODO correctness
+// If we're not going to store ipv4 and ipv6
+// NAT entries in the same table, we should probably
+// make the types more restrictive to prevent an
+// accidental ipv6 entry from being created.


Same as the previous comment

smklein · 2023-11-06T21:00:12Z

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

+        // different target sled / vpc / mac, we will violate a uniqueness
+        // constraint (which is desired in this case).
+        //
+        // It seems that this isn't straightforward to express in diesel


Diesel supports insert_into from SELECT statements: https://docs.diesel.rs/master/diesel/prelude/trait.Insertable.html#method.insert_into

Diesel supports NOT: https://docs.diesel.rs/master/diesel/dsl/fn.not.html

Diesel supports EXISTS: https://docs.diesel.rs/master/diesel/expression/exists/fn.exists.html

I know it's not trivial, but it's probably worth expressing this in Diesel -- if we added a necessary column here, this raw SQL code would still compile, but fail to actually insert a valid entry.

I do think you had the right idea -- get moving with sql_query -- but hopefully the following code lets us add a little more type-safety here.

I have not executed this code, but I have confirmed it compiles:

use db::schema::ipv4_nat_entry::dsl; use diesel::sql_types; // Look up any NAT entries that already have the exact parameters // we're trying to INSERT. let matching_entry_subquery = dsl::ipv4_nat_entry .filter(dsl::external_address.eq(nat_entry.external_address)) .filter(dsl::first_port.eq(nat_entry.first_port)) .filter(dsl::last_port.eq(nat_entry.last_port)) .filter(dsl::sled_address.eq(nat_entry.sled_address)) .filter(dsl::vni.eq(nat_entry.vni)) .filter(dsl::mac.eq(nat_entry.mac)) .select(( dsl::external_address, dsl::first_port, dsl::last_port, dsl::sled_address, dsl::vni, dsl::mac, )); // SELECT exactly the values we're trying to INSERT, but only // if it does not already exist. let new_entry_subquery = diesel::dsl::select(( nat_entry.external_address.into_sql::<sql_types::Inet>(), nat_entry.first_port.into_sql::<sql_types::Int4>(), nat_entry.last_port.into_sql::<sql_types::Int4>(), nat_entry.sled_address.into_sql::<sql_types::Inet>(), nat_entry.vni.into_sql::<sql_types::Int4>(), nat_entry.mac.into_sql::<sql_types::BigInt>(), )).filter(diesel::dsl::not(diesel::dsl::exists(matching_entry_subquery))); diesel::insert_into(dsl::ipv4_nat_entry) .values(new_entry_subquery) .into_columns(( dsl::external_address, dsl::first_port, dsl::last_port, dsl::sled_address, dsl::vni, dsl::mac, )) .execute_async(&*self.pool_connection_authorized(opctx).await?) .await .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?;

Will give this a try! Thanks for giving this a look!

smklein · 2023-11-06T22:06:20Z

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

+            "
+            UPDATE omicron.public.ipv4_nat_entry
+            SET
+            version_removed = nextval('omicron.public.ipv4_nat_version'),
+            time_deleted = now()
+            WHERE time_deleted IS NULL AND version_removed IS NULL AND id = $1 AND version_added = $2
+            ",
+        )


I think we definitely need custom SQL to handle nextval, but the rest of this could be vanilla diesel.

If we add the following function:

fn ipv4_nat_next_version() -> diesel::expression::SqlLiteral<BigInt> { diesel::dsl::sql::<BigInt>("nextval('omicron.public.ipv4_nat_version')") }

Then this particular snippet of SQL could be replaced with the following:

use db::schema::ipv4_nat_entry::dsl; let updated_rows = diesel::update(dsl::ipv4_nat_entry) .set(( dsl::version_removed.eq(ipv4_nat_next_version().nullable()), dsl::time_deleted.eq(Utc::now()), )) .filter(dsl::time_deleted.is_null()) .filter(dsl::version_removed.is_null()) .filter(dsl::id.eq(nat_entry.id)) .filter(dsl::version_added.eq(nat_entry.version_added)) .execute_async(&*self.pool_connection_authorized(opctx).await?) .await .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?;

smklein · 2023-11-06T22:09:26Z

nexus/db-model/src/ipv4_nat_entry.rs

+    pub version_added: SqlU32,
+    pub version_removed: Option<SqlU32>,


Should we be storing these versions as BigInts, rather than SqlU32s?

If we overflow a u32 (very possible!) these values would start throwing deserialization errors.

smklein · 2023-11-06T22:26:42Z

nexus/src/internal_api/http_entrypoints.rs

+        changeset.sort_by_key(|e| e.gen);
+        Ok(HttpResponseOk(changeset))
+    };
+    apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await


Suggested change

apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await

apictx.internal_latencies.instrument_dropshot_handler(&rqctx, handler).await

smklein · 2023-11-06T22:26:52Z

nexus/src/internal_api/http_entrypoints.rs

+        let view = Ipv4NatGenView { gen };
+        Ok(HttpResponseOk(view))
+    };
+    apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await


Suggested change

apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await

apictx.internal_latencies.instrument_dropshot_handler(&rqctx, handler).await

smklein · 2023-11-06T22:36:53Z

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

+        Ok(())
+    }
+
+    pub async fn ipv4_nat_cancel_delete(


Could this be marked #[cfg(test)]? The act of cancelling a delete seems somewhat unsafe to me outside test contexts.

This was originally intended for usage in saga unwinds, but I think we've since decided that we shouldn't try to unwind deletion of network configuration and instead consider it the "point of no return"

smklein · 2023-11-06T22:37:58Z

nexus/src/internal_api/http_entrypoints.rs

+struct RpwNatPathParam {
+    /// which change number to start generating
+    /// the change set from
+    from_gen: u32,


Same as other comment, about u32 vs u64 representation of generation numbers

Looks like this still needs updating

smklein · 2023-11-06T22:38:40Z

nexus/src/internal_api/http_entrypoints.rs

+// TODO: Remove (maybe)
+// This endpoint may be more trouble than it's worth. The reason this endpoint
+// may be a headache is because the underlying SEQUENCE in the DB will advance,
+// even on failed transactions. This is not a big deal, but it will lead to
+// callers trying to catch up to entries that don't exist if they check this
+// endpoint first. As an alternative, we could just page through the
+// changesets until we get nothing back, then revert to periodic polling
+// of the changeset endpoint.


Where'd we land on this?

I haven't seen a place where it has merit, so I'll remove it.

smklein · 2023-11-06T23:10:05Z

nexus/src/app/background/nat_cleanup.rs

+        async {
+            let log = &opctx.log;
+
+            let result = self.datastore.ipv4_nat_current_version(opctx).await;


To verify my understanding:

This task periodically reads the latest generation number from the DB

It sends that generation number to Dendrite instances (client.ipv4_nat_generation())

It cleans old records

So the "dendrite-playing-catch-up" entirely relies on Dendrite reaching back to the internal API within Nexus, correct?

That's pretty close to how it works:

Omicron periodically reads the latest generation in DB

Omicron checks to see what generation numbers the various dendrite instances have (we are querying dendrite, not triggering any work in dendrite)

Omicron only cleans up records older than the earliest found dendrite generation

The idea is that we don't clean up changes that a dendrite instance hasn't picked up yet. We also shouldn't have a detrimental TOCTOU race because the only way a dendrite instance moves backwards is if it crashes, and in that case it's okay that we're cleaning up old entries because we don't want that dendrite instance to re-apply them anyway.

Dendrite only catches up when:

An event in the control plane calls the trigger endpoint on the dendrite API (a "doorbell" of sorts)

Dendrite's internal RPW interval timer expires

In these events, yes, Dendrite will reach back via the internal API to catch up on the changes that have occurred.

rcgoodfellow · 2023-11-08T06:41:20Z

When you get a chance, could you merge in main so this can be tested in the a4x2 testbed topology?

internet-diglett · 2023-11-14T20:17:06Z

Note that since we are changing some of the types used by dendrite via the internal API, the deploy task will probably fail since we must first push our changes here, update dendrite, then reference the updated dendrite in the package manifest here.

smklein

Last few comments are mostly nitpicks, but this looks good! Thanks for the query cleanup!

smklein · 2023-11-14T20:21:16Z

nexus/src/internal_api/http_entrypoints.rs

+struct RpwNatPathParam {
+    /// which change number to start generating
+    /// the change set from
+    from_gen: u32,


Looks like this still needs updating

smklein · 2023-11-14T20:21:50Z

nexus/src/internal_api/http_entrypoints.rs

+/// empty vec is returned.
+#[endpoint {
+   method = GET,
+    path = "/rpw/nat/ipv4/changeset/{from_gen}"


I think I still might remove the /rpw prefix, the fact that Nexus processes this as an RPW is an implementation detail.

smklein · 2023-11-14T20:22:49Z

nexus/src/app/instance_network.rs

+                    .await?;
+            }
+            IpNetwork::V6(_v6net) => {
+                // TODO: implement handling of v6 nat.


If this is unimplemented, we should throw an error rather than silently doing nothing

smklein · 2023-11-14T20:28:50Z

nexus/db-model/src/ipv4_nat_entry.rs

+    }
+}
+
+/// Database representation of an Ipv4 NAT Generation.


Hey, it took me a while to figure this out -- this is a view of a sequence, not a table, and the log_cnt + is_called values are synthetically provided from CockroachDB.

That's worth documenting! I do not find that behavior obvious - I was trying to find out where these were used.

Related: Do we actually need to include log_cnt and is_called here? When we query for the Ipv4NatGen, we only read last_value, right?

Yes, thus far we only care about last_value, I can delete the other two columns from the struct

Actually, it seems that since we're not using this struct anymore thanks to some of the other changes that have been made :D

smklein · 2023-11-16T14:38:11Z

schema/crdb/dbinit.sql

@@ -2899,14 +2847,67 @@ CREATE TABLE IF NOT EXISTS omicron.public.db_metadata (
    CHECK (singleton = true)
 );

+
+CREATE SEQUENCE IF NOT EXISTS omicron.public.ipv4_nat_version START 1 INCREMENT 1;


This is a super minor nitpick, but we did some work to ensure that the order of declarations in dbinit.sql should mostly not matter.

Can we move this stuff up earlier? I find it a little odd to have the ordering of dbinit be:

Most of the structures we use

The db_metadata table declaration

The tables that were added for NAT

INSERT into db_metadata

(I kinda think db_metadata's declaration + INSERT should be right next to each other, at the end of the file, if opssible)

smklein

One minor ordering comment aside, this looks good to merge to me!

smklein · 2023-11-17T00:54:21Z

schema/crdb/10.0.0/up1.sql

@@ -0,0 +1 @@
+CREATE SEQUENCE IF NOT EXISTS omicron.public.ipv4_nat_version START 1 INCREMENT 1;


Before you merge this -- looks like 10.0.0 is already used on main?

https://github.com/oxidecomputer/omicron/tree/main/schema/crdb/10.0.0

Cool, I'll need to bump dendrite before merging anyway so it will give me one another chance to get my version ahead of main 😂

Awaiting merge of https://github.com/oxidecomputer/dendrite/pull/627

Dendrite PR merged, bumping Omicron dependency version and Schema version

This was accidentally merged in with 11.0.0 in oxidecomputer#3804 Fixes oxidecomputer#4530 (but see oxidecomputer#4531 for followup work)

internet-diglett force-pushed the nat_rpw branch from a9c6ae0 to 8e1ca66 Compare July 31, 2023 23:04

smklein reviewed Jul 31, 2023

View reviewed changes

common/src/sql/dbinit.sql Outdated Show resolved Hide resolved

internet-diglett changed the title ~~WIP: Initial DB Model for NAT RPW~~ NAT RPW Aug 1, 2023

internet-diglett force-pushed the nat_rpw branch from e800d5b to d990790 Compare August 9, 2023 18:21

smklein mentioned this pull request Aug 12, 2023

Define a saga for instance start #3873

Merged

internet-diglett force-pushed the nat_rpw branch from d990790 to f23e2a2 Compare September 14, 2023 16:00

internet-diglett force-pushed the nat_rpw branch from 6648ec7 to 5f61f27 Compare September 27, 2023 19:18

smklein reviewed Sep 28, 2023

View reviewed changes

internet-diglett force-pushed the nat_rpw branch from b3154cb to 32e53be Compare October 16, 2023 15:21

internet-diglett requested a review from gjcolombo October 16, 2023 19:49

internet-diglett force-pushed the nat_rpw branch from 32e53be to 315ff35 Compare October 17, 2023 19:13

internet-diglett added 3 commits October 17, 2023 20:07

undo change to nextest config

e29a612

update omdb background tasks

eda98c5

convert db tables to follow similar column names as dns rpws

8af0927

internet-diglett marked this pull request as ready for review October 19, 2023 21:52

internet-diglett added 4 commits October 20, 2023 13:42

add missing migration file

8fa914e

ensure test doesn't rely on table order

452e4d1

Merge branch 'main' into nat_rpw

ed63b1c

Update deleted_by_external_ip to use new table strategy

7fea2ac

internet-diglett requested a review from smklein November 1, 2023 18:07

smklein self-assigned this Nov 6, 2023

smklein reviewed Nov 6, 2023

View reviewed changes

smklein assigned internet-diglett and unassigned smklein Nov 6, 2023

smklein reviewed Nov 6, 2023

View reviewed changes

internet-diglett added 2 commits November 9, 2023 22:22

Merge branch 'main' into nat_rpw

38d6c5a

enforce correct types for Ipv4NatEntry

a591710

internet-diglett added 7 commits November 13, 2023 22:36

refactor ensure_ipv4_nat_entry to use diesel query

9858397

refactor sql queries use diesel dsl

92df045

remove unnecessary internal endpoint

151516f

update endpoint to use internal_latencies instrumentation

7f0c0a7

revert accidental change to non-gimlet/config-rss.toml

18164ee

remove unnecessary view struct

351a0a8

convert to SqlU32 to i64

d3e9c60

smklein approved these changes Nov 14, 2023

View reviewed changes

internet-diglett added 6 commits November 14, 2023 20:47

a wild u32 escaped

b38ebe5

error on attempt of ipv6 nat configuration

0ffee3f

remove /rpw prefix from internal endpoint

3ce9d98

remote unnecessary version tracking struct

e8d4768

bump schema version

049f324

bump dendrite version

f547f8a

smklein reviewed Nov 16, 2023

View reviewed changes

smklein approved these changes Nov 16, 2023

View reviewed changes

internet-diglett added 2 commits November 16, 2023 15:17

fixup dbinit statement ordering for clarity

7bf9e93

Merge branch 'main' into nat_rpw

45cea66

smklein reviewed Nov 17, 2023

View reviewed changes

bump dendrite and schema

eff1828

internet-diglett merged commit 14f8f31 into main Nov 18, 2023
21 checks passed

internet-diglett deleted the nat_rpw branch November 18, 2023 04:22

jgallagher mentioned this pull request Nov 20, 2023

Restore schema 10.0.0 #4529

Merged

smklein mentioned this pull request Nov 20, 2023

v10 schema changes removed, renamed into v11 #4530

Closed

karencfv pushed a commit to karencfv/omicron that referenced this pull request Nov 21, 2023

Restore schema 10.0.0 (oxidecomputer#4529)

74d3bf9

This was accidentally merged in with 11.0.0 in oxidecomputer#3804 Fixes oxidecomputer#4530 (but see oxidecomputer#4531 for followup work)

		pub version_added: SqlU32,
		pub version_removed: Option<SqlU32>,

	apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await
	apictx.internal_latencies.instrument_dropshot_handler(&rqctx, handler).await

		@@ -0,0 +1 @@
		CREATE SEQUENCE IF NOT EXISTS omicron.public.ipv4_nat_version START 1 INCREMENT 1;

NAT RPW #3804

NAT RPW #3804

Conversation

internet-diglett commented Jul 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

internet-diglett commented Oct 12, 2023

smklein left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rcgoodfellow commented Nov 8, 2023

internet-diglett commented Nov 14, 2023

smklein left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smklein left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

internet-diglett commented Jul 31, 2023 •

edited

Loading