Plumb firewall rules #1636

plotnick · 2022-08-22T22:06:31Z

Plumb VPC firewall rules through Nexus → sled-agent → OPTE. Builds on @bnaecker's work in the propagate-firewall-rules-to-sled branch, which in turn was based on work started in PR #465.

Outstanding issues:

~~Target interfaces are matched by MAC address only right now. Is this ok?~~ Use (VNI, MAC) pair to match.
IP and network targets are not yet supported. ~~The console doesn't support these yet either, but the API does.~~
Send default rules to sled agent on instance creation. Right now rules are only sent on VPC creation and rule updates. Fixes Send current firewall rules applying to a guest in the sled agent instance-creation request #1017.
Missing integration tests (issue Need VPC firewall rule integration tests #1792). I have tested this manually with multiple instances and a small variety of targets and host filters, but we definitely need automated tests.
Fix existing tests, which are now failing with cross-VPC firewall target unsupported

Needed to use Ipv4Net for firewall rule targets.

zephraph · 2022-08-24T15:41:08Z

Feel free to create an issue in the console repo to capture whatever is missing.

bnaecker

This is a lot of great work, thanks for taking it on! I see your TODO-list on the PR comment itself. Is the plan to tackle those now as part of this PR? Or are those going to be separate issues?

bnaecker · 2022-09-06T17:36:06Z

nexus/src/app/vpc.rs

+                    {
+                        *name = params.identity.name.clone()
+                    }
+                    _ => (),


Just checking that the intent is to silently ignore targets other than VPCs, rather than, say, returning 400. Maybe when we add IP address and subnet targets, we should have a catchall that returns 400?

1dc02d3 now raises an error for these cases, which should never occur (if they do, it's because we made a mistake in specifying DEFAULT_FIREWALL_RULES).

bnaecker · 2022-09-06T17:38:30Z

nexus/src/app/vpc.rs

+                            ) if name.as_str() == "default" => {
+                                *name = params.identity.name.clone()
+                            }
+                            _ => (),


Same question here.

Fixed in 1dc02d3.

bnaecker · 2022-09-06T18:02:24Z

nexus/src/app/vpc.rs

+        let no_networks: Vec<IpNetwork> = Vec::new();
+        let no_interfaces: Vec<NetworkInterface> = Vec::new();
+
+        let mut instance_interfaces: NicMap = HashMap::new();


We're splitting up interfaces by instance, VPC, and VPC Subnet here. Below, in the compilation loop starting on L600, we then push these onto a single array, targets. What happens with duplicate interfaces? As long as they don't change between those calls to derive_{guests,vpc,subnet}_network_interface_info, is that just an optimization to avoid looking up or sending more information to the sled agents than necessary?

Excellent question; 1fa97ab does explicit de-duplication on the NICs. (If sled_agent_client::types::NetworkInterface were Hash, this could be simplified by accumulating directly into a HashSet; please LMK if you know how to do that.)

I think you can implement Hash manually, probably by hashing all the fields. Not sure if it's worth it, up to you at this point, since I don't want to further delay this PR unnecessarily.

bnaecker · 2022-09-06T18:03:05Z

nexus/src/db/datastore/network_interface.rs

+            )
+            .inner_join(vpc::table.on(vpc_subnet::vpc_id.eq(vpc::id)))
+            .order_by(network_interface::slot)
+            // TODO-cleanup: See DRY comment above.


I'd probably remove this, since it refers to a comment in the other guest-specific function now.

bnaecker · 2022-09-06T18:07:45Z

nexus/src/db/datastore/network_interface.rs

+
+        use db::schema::{network_interface, vpc, vpc_subnet};
+        let rows = network_interface::table
+            .filter(network_interface::subnet_id.eq(authz_subnet.id()))


This is the only line that differs between these three implementations, AFAICT. Could we either make this function generic, on the column we're filtering, or have a shared function that implements most of this and accepts a boxed query on the table? Let me know if you'd like to talk through the Diesel details here, it can get pretty hairy with generics.

Hairy indeed, but probably worth it. Refactoring of these three routines forthcoming.

Ok, factored using a boxed query in 902e6a8.

bnaecker · 2022-09-06T18:12:48Z

nexus/src/db/datastore/vpc.rs

+            entry.push(ipnetwork::IpNetwork::V4(subnet.ipv4_block.0 .0));
+            entry.push(ipnetwork::IpNetwork::V6(subnet.ipv6_block.0 .0));


Since we're just interested in the name and IP subnets, we could select exactly those from the table rather than the whole record. E.g., .select((vpc_subnet::name, vpc_subnet::ipv4_block, vpc_subnet::ipv6_block)), I think.

Done in 578a2d9, thanks.

bnaecker · 2022-09-06T18:34:47Z

sled-agent/src/opte/illumos/port_manager.rs

+            VpcFirewallRuleStatus::Enabled => true,
+        })
+        .filter(|rule| {
+            // TODO: no targets means apply everywhere, right?


Yep. We can generally think of each filter in a rule as restricting the set of objects to which the rule applies. Adding a protocol filter of TCP means it applies only TCP traffic, and no other protocol, for example.

Great, that's what I thought.

bnaecker · 2022-09-06T18:50:17Z

sled-agent/src/opte/illumos/port_manager.rs

+        .filter(|rule| {
+            // TODO: no targets means apply everywhere, right?
+            rule.targets.is_empty()
+                || rule.targets.iter().any(|nic| nic.mac.0 == *port.mac())


I'm not sure this is quite right. MAC addresses are unique within a VPC, not globally. Suppose we had two rules, each applying exclusively to two different VPCs, but otherwise identical. If those VPCs happened to each have a NIC with the same MAC, the rule would be applied to both of those.

If that sounds right to you as well, then I think we to check the identity of the VPC and the MAC of the Port. The vni field of the NetworkInterface sent in in the rules is bijective with the VPC identity, so that's the best thing to use at this point.

Fixed in 1994a43, thanks.

Also return an error on default rules we don't understand during the walk.

rzezeski · 2022-09-14T16:37:45Z

Heads up: connectivity inbound to a guest will fail when firewall rules are populated until oxidecomputer/opte#252 lands.

rzezeski · 2022-09-24T00:42:09Z

Okay, oxidecomputer/opte#252 is fixed. So inbound connectivity should work with latest xde. If it doesn't please let me know.

bnaecker

My apologies for the long delay. There's a lot of great work here, thank you! I've mostly got questions about improving names or comments, but the functionality itself looks great.

nexus/src/app/sagas/instance_migrate.rs

nexus/src/db/datastore/vpc.rs

sled-agent/src/opte/illumos/port_manager.rs

nexus/src/app/vpc.rs

bnaecker · 2022-10-04T18:38:34Z

nexus/src/app/vpc.rs

+        let no_networks: Vec<IpNetwork> = Vec::new();
+        let no_interfaces: Vec<NetworkInterface> = Vec::new();
+
+        let mut instance_interfaces: NicMap = HashMap::new();


I think you can implement Hash manually, probably by hashing all the fields. Not sure if it's worth it, up to you at this point, since I don't want to further delay this PR unnecessarily.

plotnick · 2022-10-05T19:35:02Z

@bnaecker: Thanks for the very helpful second review! The one remaining concern I have that I'd like to fix is that if you use a non-existent VPC name in a target or host filter, things currently fail in a rather opaque way. I'm going to try to fix that today (Wed), and then I think it'll be in ok shape (though see also issue #1756, which I think is serious but not blocking).

However, I'd like to maybe delay actually merging this until @smklein branches for the demo next week. Since this change involves touching many critical pieces (including instance creation), does not have integration tests, and is not necessary for that demo, I think it's a bit risky to land with so little time for testing. Other opinions welcome.

bnaecker · 2022-10-06T03:43:54Z

That's all excellent, thanks for chasing down those threads @plotnick! I think your plan of merging after the demo branch is created sounds very wise. I'm approving this now, feel free to merge whenever that happens. And again, nice work. It's really excellent to see such a core networking primitive start to take shape!

Crucible changes are: Make `ImpactedBlocks` an absolute range (#1685) update omicron (#1687) Update Rust crate chrono to v0.4.40 (#1682) Update Rust crate tempfile to v3.19.0 (#1676) Log loops during crutest replay (#1674) Refactor live-repair start and send (#1673) Update Rust crate libc to v0.2.171 (#1684) Update Rust crate clap to v4.5.32 (#1683) Update Rust crate async-trait to 0.1.88 (#1680) Update Rust crate bytes to v1.10.1 (#1681) Update Rust crate anyhow to v1.0.97 (#1679) Update Rust crate http to v1 (#1678) Update Rust crate tokio-util to v0.7.14 (#1675) Update Rust crate thiserror to v2 (#1657) Update Rust crate omicron-zone-package to 0.12.0 (#1647) Update Rust crate opentelemetry to 0.28.0 (#1543) Update Rust crate itertools to 0.14.0 (#1646) Update Rust crate clearscreen to v4 (#1656) nightly test polish (#1665) Update Rust crate strum_macros to 0.27 (#1654) Update Rust crate strum to 0.27 (#1653) Update Rust crate rusqlite to 0.34 (#1652) Better return semantics from should_send Trying to simplify enqueue Remove unnecessary argument Remove unused code from `ImpactedBlocks` (#1668) Log when pantry/volume layers activate things (#1670) Fix unused import (#1669) Use `RangeSet` instead of tracking every complete job (#1663) Update Rust crate dropshot to 0.16.0 (#1645) Simplify pending work tracking (#1662) Remove rusqlite dependency from crucible-common (#1659) Update Rust crate reedline to 0.38.0 (#1651) Update Rust crate proptest to 1.6.0 (#1648) Update Rust crate bytes to v1.10.0 (#1644) Update Rust crate tracing-subscriber to 0.3.19 (#1643) Update Rust crate tracing to v0.1.41 (#1642) Update Rust crate toml to v0.8.20 (#1641) Update Rust crate thiserror to v1.0.69 (#1640) Update Rust crate serde_json to v1.0.139 (#1639) Update Rust crate serde to v1.0.218 (#1638) Update Rust crate reqwest to v0.12.12 (#1637) Update Rust crate libc to v0.2.170 (#1636) Update Rust crate indicatif to 0.17.11 (#1635) Update Rust crate httptest to 0.16.3 (#1634) Update Rust crate clap to v4.5.31 (#1632) Update Rust crate chrono to v0.4.39 (#1631) Update Rust crate byte-unit to 5.1.6 (#1630) Update Rust crate async-trait to 0.1.86 (#1629) Update Rust crate csv to 1.3.1 (#1633) Update Rust crate anyhow to v1.0.96 (#1628) Fix a test flake (#1626) Bitpack per-block slot data in memory (#1625) Use `div_ceil` instead of `(x + 7) / 8` (#1624) Add repair server dynamometer (#1618) Propolis changes are: Paper over minor() differences Update crucible (#886) Test oximeter metrics end to end (#855) server: improve behavior of starting VMs that are waiting for Crucible activation (#873) server: extend API request queue's memory of prior requests (#880) Use production propolis-server flags in PHD CI builds (#878) Added a new field to the Board struct that propolis requires.

Crucible changes are: Make `ImpactedBlocks` an absolute range (#1685) update omicron (#1687) Update Rust crate chrono to v0.4.40 (#1682) Update Rust crate tempfile to v3.19.0 (#1676) Log loops during crutest replay (#1674) Refactor live-repair start and send (#1673) Update Rust crate libc to v0.2.171 (#1684) Update Rust crate clap to v4.5.32 (#1683) Update Rust crate async-trait to 0.1.88 (#1680) Update Rust crate bytes to v1.10.1 (#1681) Update Rust crate anyhow to v1.0.97 (#1679) Update Rust crate http to v1 (#1678) Update Rust crate tokio-util to v0.7.14 (#1675) Update Rust crate thiserror to v2 (#1657) Update Rust crate omicron-zone-package to 0.12.0 (#1647) Update Rust crate opentelemetry to 0.28.0 (#1543) Update Rust crate itertools to 0.14.0 (#1646) Update Rust crate clearscreen to v4 (#1656) nightly test polish (#1665) Update Rust crate strum_macros to 0.27 (#1654) Update Rust crate strum to 0.27 (#1653) Update Rust crate rusqlite to 0.34 (#1652) Better return semantics from should_send Trying to simplify enqueue Remove unnecessary argument Remove unused code from `ImpactedBlocks` (#1668) Log when pantry/volume layers activate things (#1670) Fix unused import (#1669) Use `RangeSet` instead of tracking every complete job (#1663) Update Rust crate dropshot to 0.16.0 (#1645) Simplify pending work tracking (#1662) Remove rusqlite dependency from crucible-common (#1659) Update Rust crate reedline to 0.38.0 (#1651) Update Rust crate proptest to 1.6.0 (#1648) Update Rust crate bytes to v1.10.0 (#1644) Update Rust crate tracing-subscriber to 0.3.19 (#1643) Update Rust crate tracing to v0.1.41 (#1642) Update Rust crate toml to v0.8.20 (#1641) Update Rust crate thiserror to v1.0.69 (#1640) Update Rust crate serde_json to v1.0.139 (#1639) Update Rust crate serde to v1.0.218 (#1638) Update Rust crate reqwest to v0.12.12 (#1637) Update Rust crate libc to v0.2.170 (#1636) Update Rust crate indicatif to 0.17.11 (#1635) Update Rust crate httptest to 0.16.3 (#1634) Update Rust crate clap to v4.5.31 (#1632) Update Rust crate chrono to v0.4.39 (#1631) Update Rust crate byte-unit to 5.1.6 (#1630) Update Rust crate async-trait to 0.1.86 (#1629) Update Rust crate csv to 1.3.1 (#1633) Update Rust crate anyhow to v1.0.96 (#1628) Fix a test flake (#1626) Bitpack per-block slot data in memory (#1625) Use `div_ceil` instead of `(x + 7) / 8` (#1624) Add repair server dynamometer (#1618) Propolis changes are: Paper over minor() differences Update crucible (#886) Test oximeter metrics end to end (#855) server: improve behavior of starting VMs that are waiting for Crucible activation (#873) server: extend API request queue's memory of prior requests (#880) Use production propolis-server flags in PHD CI builds (#878) --------- Co-authored-by: Alan Hanson <alan@oxide.computer>

plotnick added 8 commits August 22, 2022 14:20

Don't restrict IPv4 addresses to private subnets

93ba272

Needed to use Ipv4Net for firewall rule targets.

Start plumbing firewall rules through sled-agent

d0f2d78

Send firewall rules to sled-agent

2909d15

Plumb ports in firewall rules

8715ba8

Handle multiple hosts & protocols in firewall rules

1c4d983

Handle Ipv4Net host filters

8fe8dfe

Resolve firewall targets and hosts

b15b452

Set firewall rules only on matching targets

c5944c5

plotnick requested a review from bnaecker August 22, 2022 22:06

plotnick added 4 commits August 22, 2022 16:06

fmt

a50ddf0

clippy

0156492

Merge branch 'main' into firewall

ca6d01a

Customize default firewall rules for non-default VPCs

4241a7e

plotnick force-pushed the firewall branch from a3ae822 to 4241a7e Compare August 23, 2022 17:45

plotnick mentioned this pull request Aug 24, 2022

Firewall rule tweaks oxidecomputer/console#1127

Closed

4 tasks

bnaecker reviewed Sep 6, 2022

View reviewed changes

plotnick added 5 commits September 7, 2022 10:40

Factor out default firewall rule generation

1dc02d3

Also return an error on default rules we don't understand during the walk.

De-duplicate firewall rule target interfaces

1fa97ab

Match OPTE ports to NICs by (VNI, MAC) pair

1994a43

Select just the columns needed in resolve_subnets_to_ips

578a2d9

clippy

2c70fd8

plotnick mentioned this pull request Sep 9, 2022

Fix ownership bug in OPTE port management that can lead to deadlock #1695

Merged

Factor derive_*_network_interface_info routines

902e6a8

plotnick added 3 commits September 21, 2022 14:50

Merge branch 'main' into firewall

f08f06c

Send firewall rules to sled-agent on instance creation

a1f17cf

Setup firewall rules on OPTE port creation

41f61ca

plotnick force-pushed the firewall branch from ffe988a to 41f61ca Compare September 23, 2022 20:36

plotnick added 2 commits September 29, 2022 12:02

Merge branch 'main' into firewall

53d97f6

Merge branch 'main' into firewall

a8a6d3e

bnaecker reviewed Oct 4, 2022

View reviewed changes

plotnick mentioned this pull request Oct 4, 2022

Gather and send instance information on migration #1783

Closed

4 tasks

plotnick added 9 commits October 4, 2022 14:41

Link to tracking issue for missing instance migration info

f0dc4dd

Ungroup imports

f3b64ae

Rename resolution function for clarity

8ab92af

Use a B-tree instead of a hash map in resolution function

cbedb56

Remove superfluous scope

adc62cc

fmt

6048c14

Refactor firewall rule conversion routine

9869832

Elide needless clone

f7ba1eb

Clarify doc comment for default_firewall_rules_for_vpc

171a625

plotnick mentioned this pull request Oct 5, 2022

Handle multiple failures in sled-agent firewall rule requests #1791

Open

Link to multiple-error-handling issue

aae7d20

plotnick mentioned this pull request Oct 5, 2022

Need VPC firewall rule integration tests #1792

Open

Merge branch 'main' into firewall

cc424a2

bnaecker self-requested a review October 6, 2022 03:44

bnaecker approved these changes Oct 6, 2022

View reviewed changes

plotnick added 3 commits October 12, 2022 15:26

Merge branch 'main' into firewall

f204527

SELECT DISTINCT sleds for firewall rule updates

63e92bc

Don't fail on missing names in firewall rule resolution

fd5c3e4

plotnick merged commit 56a586d into main Oct 14, 2022

plotnick deleted the firewall branch October 14, 2022 02:27

plotnick mentioned this pull request Oct 21, 2022

Fix clippy warnings on illumos #1847

Merged

leftwo mentioned this pull request Mar 26, 2025

Update crucible and propolis #7877

Merged

		entry.push(ipnetwork::IpNetwork::V4(subnet.ipv4_block.0 .0));
		entry.push(ipnetwork::IpNetwork::V6(subnet.ipv6_block.0 .0));

Plumb firewall rules #1636

Plumb firewall rules #1636

Uh oh!

Conversation

plotnick commented Aug 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zephraph commented Aug 24, 2022

Uh oh!

bnaecker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rzezeski commented Sep 14, 2022

Uh oh!

rzezeski commented Sep 24, 2022

Uh oh!

bnaecker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plotnick commented Oct 5, 2022

Uh oh!

bnaecker commented Oct 6, 2022

Uh oh!

Uh oh!

plotnick commented Aug 22, 2022 •

edited

Loading