Tunneled routing support for boundary services. #381

rcgoodfellow · 2023-06-27T04:33:47Z

Introduces tunnel routing as described in RFD 404. The primary changes here are the following.

Removes the assumption of a single boundary services endpoint address.
Adds a virt-to-boundary table (V2B) that maps destination prefixes on a VPC to a boundary services address on the physical/underlay network.
Implements a longest-prefix-match routing table for V2B using a poptrie

Fixes

Multiple boundary services targets per VPC #374

FelixMcFelix

Looks good! A handful of nits/potential oversights, otherwise I think it's clear and looks to be a reasonable approach to store and use V2B mappings.

FelixMcFelix · 2023-08-29T18:06:58Z

crates/illumos-sys-hdrs/src/lib.rs

+#[repr(C)]
+pub enum krw_type_t {
+    RW_DRIVER = 2,
+    RW_DEFAULT = 4,
+}


I agree that this is a better construction than a newtype struct as before.

So, this probably wouldn't be an issue for this specific type but the newtype construction is to avoid an unfortunate footgun with matching on repr C enums, see #322

crates/opte-api/src/cmd.rs

crates/opte-api/src/ip.rs

bin/opteadm/src/bin/opteadm.rs

FelixMcFelix · 2023-08-29T19:22:23Z

lib/oxide-vpc/src/engine/overlay.rs

+            RouterTargetInternal::InternetGateway => {
+                match self.v2b.get(&flow_id.dst_ip) {
+                    Some(phys) => PhysNet {
+                        ether: MacAddr::ZERO,


For my own understanding/confirmation, this is because of the move to an anycast gateway IP?

This has been reworked and this code no longer exists. Instead of doing anycast at the gateways, we're doing unicast and handling multipath decisions in OPTE based on flow hashing.

lib/oxide-vpc/src/engine/print.rs

lib/oxide-vpc/tests/common/mod.rs

luqmana · 2023-08-29T19:59:11Z

crates/illumos-sys-hdrs/src/lib.rs

+#[repr(C)]
+pub enum krw_type_t {
+    RW_DRIVER = 2,
+    RW_DEFAULT = 4,
+}


So, this probably wouldn't be an issue for this specific type but the newtype construction is to avoid an unfortunate footgun with matching on repr C enums, see #322

lib/opte/src/ddi/sync.rs

lib/oxide-vpc/src/engine/overlay.rs

luqmana · 2023-08-29T20:22:09Z

lib/oxide-vpc/src/engine/overlay.rs

+#[repr(C)]
+#[derive(Debug, Deserialize, Serialize)]
+pub struct DumpVirt2BoundaryReq {
+    pub unused: u64,


did the ioctl machinery not like this zero-sized?

I believe this is the case. I'm following suit with what's in DumpVirt2PhysReq.

luqmana · 2023-08-29T20:27:28Z

lib/oxide-vpc/src/engine/overlay.rs

+                    Some(phys) => PhysNet {
+                        ether: MacAddr::ZERO,
+                        ip: phys.ip,
+                        vni: Vni::new(BOUNDARY_SERVICES_VNI).unwrap(),


Would phys.vni be different than BOUNDARY_SERVICES_VNI? And speaking of which, do we even need a fixed BOUNDARY_SERVICES_VNI in opte anymore with the v2b map (besides tests)? I imagine the contract is now between the p4 and sled-agent who'll be setting these mappings.

On the latter part, we're still using 99 for boundary services egress traffic - even though the tunnel endpoint addresses are now dynamic and determined through advertisements.

luqmana · 2023-08-29T20:38:23Z

lib/oxide-vpc/src/engine/overlay.rs

+                let e = self.ip4.lock().insert(ip4, tep);
+                self.update_poptrie_v4();
+                e


The self.ip4 lock afaik will get dropped at the end of that first statement and so there's a chance of a race before update_poptrie_v4 tries to grab the lock again. In fact, by the time we're returning the inserted TunnelEndpoint e, it might've already been removed.

Perhaps just hold both structures (ip4,pt4/ip6,pt6) behind the same RwLock. Or at the very least perhaps make update_poptrie_v{4,6} take a MutexGuard

Good catch. Done.

luqmana · 2023-08-29T21:22:45Z

lib/oxide-vpc/tests/common/mod.rs

@@ -95,6 +97,12 @@ pub const VPC_LAYERS: [&str; 5] =
 pub const GW_MAC_ADDR: MacAddr =
    MacAddr::from_const([0xA8, 0x40, 0x25, 0xFF, 0x77, 0x77]);

+pub const BS_MAC_ADDR: MacAddr =
+    MacAddr::from_const([0xA8, 0x40, 0x25, 0x77, 0x77, 0x77]);


This falls into the physical devices space of our MAC address space. We also have some further constraints on the software-defined/virtual space noted in omicron.

This is just for tests but FF:00:00-FF:7F:FF would be where we can draw from for fixed addresses (i.e. GW_MAC_ADDR).

I also realize now that the boundary services mac addr we're using also conflicts :/ It falls into the guest addresses range (F0:00:00-FE:FF:FF) so an instance could potentially get assigned the same address. Opened issue for that oxidecomputer/omicron#3984

Thinking on it a lil more, can we move GW_MAC_ADDR, BS_MAC_ADDR, BS_IP_ADDR, BOUNDARY_SERVICES_VNI to be exported from oxide-vpc to omicron can also use them leaving us with less places to have to keep in sync.

I've moved GW_MAC_ADDR to oxide_vpc::api

Redefined BS_MAC_ADDR in terms of oxide_vpc::engine::overlay::INTERNET_GATEWAY_MAC

BS_IP_ADDR is staying put, as that's no longer a static entity, but we need to choose something for the purpose of this test.

lib/oxide-vpc/tests/common/mod.rs

xde/src/xde.rs

.github/buildomat/jobs/test.sh

FelixMcFelix

Thanks, Ry! I'm happy on the whole with this, just some nits.

FelixMcFelix · 2024-01-15T10:58:34Z

lib/oxide-vpc/src/api.rs

+
+impl PartialOrd for TunnelEndpoint {
+    fn partial_cmp(&self, other: &Self) -> Option<core::cmp::Ordering> {
+        Some(self.ip.cmp(&other.ip))


Since you impl Ord, you can rely on that instead:

Suggested change

Some(self.ip.cmp(&other.ip))

Some(self.cmp(other))

FelixMcFelix · 2024-01-15T11:33:50Z

lib/oxide-vpc/src/engine/overlay.rs

+}
+
+pub const BOUNDARY_SERVICES_VNI: u32 = 99u32;
+pub const INTERNET_GATEWAY_MAC: [u8; 6] = [0xA8, 0x40, 0x25, 0x77, 0x77, 0x77];


This seems identical to the new oxide_vpc::api::GW_MAC_ADDR.

These are slightly different.

pub const INTERNET_GATEWAY_MAC: [u8; 6] = [0xA8, 0x40, 0x25, 0x77, 0x77, 0x77]; pub const GW_MAC_ADDR: MacAddr = MacAddr::from_const([0xA8, 0x40, 0x25, 0xFF, 0x77, 0x77]);

The former is meant to represent the MAC of a physical gateway / tunnel endpoint, e.g. a sidecar. INTERNET_GATEWAY_MAC is not a great name for that, so I've renamed to TUNNEL_ENDPOINT_MAC.

Ah, I'd missed that distinction. Thanks for clarifying!

FelixMcFelix · 2024-01-15T11:53:38Z

lib/oxide-vpc/src/engine/print.rs

 use opte::engine::print::*;

+cfg_if::cfg_if! {
+    if #[cfg(all(not(feature = "std"), not(test)))] {
+        use alloc::string::ToString;


You can always just use alloc::string::ToString, it doesn't need a cfg_if!{ ... } after #391. std re-exports the exact same types as alloc.

FelixMcFelix · 2024-01-15T12:05:48Z

xde/src/xde.rs

-// TODO
-// - ddm integration to choose correct underlay device (currently just using
-//   first device)
+#![allow(clippy::arc_with_non_send_sync)]


What type is tripping this up? Do we have the wrong bounds on KRwLock/KMutex?

There are several of these in xde

warning: usage of an `Arc` that is not `Send` or `Sync` --> xde/src/xde.rs:244:20 | 244 | let ectx = Arc::new(ExecCtx { log: Box::new(opte::KernelLog {}) }); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: the trait `Send` is not implemented for `ExecCtx` = note: the trait `Sync` is not implemented for `ExecCtx` = note: required for `Arc<ExecCtx>` to implement `Send` and `Sync` = help: consider using an `Rc` instead or wrapping the inner type with a `Mutex` = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#arc_with_non_send_sync = note: `#[warn(clippy::arc_with_non_send_sync)]` on by default warning: usage of an `Arc` that is not `Send` or `Sync` --> xde/src/xde.rs:991:14 | 991 | let u1 = Arc::new(create_underlay_port(u1_name, "xdeu0")?); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: the trait `Send` is not implemented for `xde_underlay_port` = note: the trait `Sync` is not implemented for `xde_underlay_port` = note: required for `Arc<xde_underlay_port>` to implement `Send` and `Sync` = help: consider using an `Rc` instead or wrapping the inner type with a `Mutex` = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#arc_with_non_send_sync warning: usage of an `Arc` that is not `Send` or `Sync` --> xde/src/xde.rs:992:14 | 992 | let u2 = Arc::new(create_underlay_port(u2_name, "xdeu1")?); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: the trait `Send` is not implemented for `xde_underlay_port` = note: the trait `Sync` is not implemented for `xde_underlay_port` = note: required for `Arc<xde_underlay_port>` to implement `Send` and `Sync` = help: consider using an `Rc` instead or wrapping the inner type with a `Mutex` = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#arc_with_non_send_sync warning: usage of an `Arc` that is not `Send` or `Sync` --> xde/src/xde.rs:2024:8 | 2024 | Ok(Arc::new(pb.create(net, limit, limit)?)) | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: the trait `Send` is not implemented for `Port<VpcNetwork>` = note: the trait `Sync` is not implemented for `Port<VpcNetwork>` = note: required for `Arc<Port<VpcNetwork>>` to implement `Send` and `Sync` = help: consider using an `Rc` instead or wrapping the inner type with a `Mutex` = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#arc_with_non_send_sync warning: `xde` (lib) generated 5 warnings (1 duplicate) Finished dev [unoptimized + debuginfo] target(s) in 25.69s

rcgoodfellow force-pushed the boundary-services-overlay-flow-pinning branch from d0a9cf6 to 9c83e09 Compare June 30, 2023 07:56

rcgoodfellow changed the title ~~Pin boundary services overlay flows to underlay path~~ Tunneled routing support for boundary services. Jun 30, 2023

rcgoodfellow force-pushed the boundary-services-overlay-flow-pinning branch 3 times, most recently from 45e9db7 to e0cf827 Compare August 4, 2023 17:23

This was referenced Aug 7, 2023

ddm tunnel routing oxidecomputer/maghemite#77

Merged

updates for tunnel routing oxidecomputer/omicron#3859

Merged

rcgoodfellow force-pushed the boundary-services-overlay-flow-pinning branch 2 times, most recently from 05a0042 to fe70f18 Compare August 9, 2023 22:20

rcgoodfellow marked this pull request as ready for review August 9, 2023 22:52

rcgoodfellow requested a review from FelixMcFelix August 28, 2023 14:53

FelixMcFelix reviewed Aug 29, 2023

View reviewed changes

luqmana reviewed Aug 29, 2023

View reviewed changes

FelixMcFelix added a commit that referenced this pull request Sep 22, 2023

Port Mac-friendly KRwLock (#381), fix tests

5ff40c3

FelixMcFelix added a commit that referenced this pull request Sep 26, 2023

Port Mac-friendly KRwLock (#381), fix tests

38a7f8c

rcgoodfellow force-pushed the boundary-services-overlay-flow-pinning branch 3 times, most recently from 216453b to 82ea7ca Compare January 3, 2024 08:37

rcgoodfellow marked this pull request as draft January 4, 2024 20:36

rcgoodfellow force-pushed the boundary-services-overlay-flow-pinning branch from 8c6326c to 760aaee Compare January 4, 2024 20:38

rcgoodfellow self-assigned this Jan 8, 2024

rcgoodfellow commented Jan 12, 2024

View reviewed changes

.github/buildomat/jobs/test.sh Show resolved Hide resolved

rcgoodfellow marked this pull request as ready for review January 13, 2024 00:23

rcgoodfellow requested a review from FelixMcFelix January 13, 2024 01:46

rcgoodfellow force-pushed the boundary-services-overlay-flow-pinning branch from 71eb2d3 to 73d4669 Compare January 13, 2024 02:17

rcgoodfellow requested a review from luqmana January 13, 2024 17:00

FelixMcFelix approved these changes Jan 15, 2024

View reviewed changes

rcgoodfellow linked an issue Jan 18, 2024 that may be closed by this pull request

Multiple boundary services targets per VPC #374

Closed

support for tunneled routing

464938d

rcgoodfellow force-pushed the boundary-services-overlay-flow-pinning branch from 3739ce6 to 464938d Compare January 23, 2024 20:10

rcgoodfellow merged commit 1d29ef6 into master Jan 23, 2024
9 checks passed

rcgoodfellow deleted the boundary-services-overlay-flow-pinning branch January 23, 2024 20:39

FelixMcFelix mentioned this pull request Feb 14, 2024

Benchmarks and instrumentation #395

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tunneled routing support for boundary services. #381

Tunneled routing support for boundary services. #381

rcgoodfellow commented Jun 27, 2023 •

edited

Loading

FelixMcFelix left a comment

FelixMcFelix Aug 29, 2023

luqmana Aug 29, 2023

FelixMcFelix Aug 29, 2023

rcgoodfellow Jan 13, 2024

luqmana Aug 29, 2023

luqmana Aug 29, 2023

rcgoodfellow Jan 12, 2024

luqmana Aug 29, 2023

rcgoodfellow Jan 13, 2024

luqmana Aug 29, 2023

rcgoodfellow Jan 13, 2024

luqmana Aug 29, 2023

luqmana Aug 29, 2023

rcgoodfellow Jan 13, 2024

FelixMcFelix left a comment

FelixMcFelix Jan 15, 2024

rcgoodfellow Jan 15, 2024

FelixMcFelix Jan 15, 2024

rcgoodfellow Jan 15, 2024

FelixMcFelix Jan 15, 2024

FelixMcFelix Jan 15, 2024

rcgoodfellow Jan 15, 2024

FelixMcFelix Jan 15, 2024

rcgoodfellow Jan 15, 2024

Tunneled routing support for boundary services. #381

Tunneled routing support for boundary services. #381

Conversation

rcgoodfellow commented Jun 27, 2023 • edited Loading

FelixMcFelix 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

FelixMcFelix 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

rcgoodfellow commented Jun 27, 2023 •

edited

Loading