Allow limited inbound ICMP to Nexus, add ICMP type/code filters to firewall rules #8194

FelixMcFelix · 2025-05-21T12:14:37Z

This PR adds more detail to firewall protocol filters. Rather than being specified purely as a string->enum mapping, each protocol filter is now a standard tagged enum like many other types in the Nexus API. Here, this allows us to express more control over ICMP traffic. For instance:

all ICMP ([{"type": "icmp"}]),
all ICMP echo ([{"type": "icmp", "value": { icmp_type: 0 }}, {"type": "icmp", "value": { icmp_type: 8 }}])
a subset of ICMP destination unreachable ({"type": "icmp", "value": { icmp_type: 0, code: "0-4" }}).

Building on this, Nexus now has a firewall entry to permit the receipt of e.g., Destination Unreachable messages. I've added a new system endpoint to control whether this is enabled or disabled.

As part of this, I've converted the CRDB representation from a fixed ENUM[] to a STRING(32)[]. There are a couple of reasons for this:

ICMPv6 will need a similar level of specificity,
Users will need to specify arbitrary (ranges) of L4 protocols, once OPTE is more permissive (Allow/support arbitrary L4 protocols in guest traffic opte#609).
Future protocols we give first-class support to might also require/want in-depth filters.

Should close #7998.

That seems to be be the primary reason for the entire test suite having a Very Bad Time.

Thanks, CRDB.

FelixMcFelix · 2025-05-23T11:08:44Z

I've setup these bits on dublin this morning. After performing the schema update, everything seems happily functional, and posting true/false to the new system endpoint correctly enables/disables the ICMP allowance as expected. Console is broken when we go to the firewall rules tab, but that is to be expected given the OpenAPI changes.

On the actual MTU dscovery front, it looks as though both this PR and main are doing the correct thing?

If from the web console I perform a 'Reload (Override Cache)' we can kick off large enough TCP transfers to run into LSO. When we trace this through:

Dublin

BRM23230018 # dtrace -n 'xde_mc_tx:entry{ this->m = (mblk_t *)arg1; this->mss = this->m->b_datap->db_struioun.cksum.pad } xde_mc_tx:entry/this->m->b_datap->db_struioun.cksum.flags && this-
>mss != 0xBADD && this->mss/{printf("packet with MSS %d", this->m->b_datap->db_struioun.cksum.pad);}'
dtrace: description 'xde_mc_tx:entry' matched 2 probes
CPU     ID                    FUNCTION:NAME
111   5301                  xde_mc_tx:entry packet with MSS 1368
 69   5301                  xde_mc_tx:entry packet with MSS 1368
 69   5301                  xde_mc_tx:entry packet with MSS 1368
 69   5301                  xde_mc_tx:entry packet with MSS 1368
 69   5301                  xde_mc_tx:entry packet with MSS 1368
 69   5301                  xde_mc_tx:entry packet with MSS 1368
 69   5301                  xde_mc_tx:entry packet with MSS 1368
 69   5301                  xde_mc_tx:entry packet with MSS 1368
 69   5301                  xde_mc_tx:entry packet with MSS 1368
 69   5301                  xde_mc_tx:entry packet with MSS 1368
 69   5301                  xde_mc_tx:entry packet with MSS 1368
 69   5301                  xde_mc_tx:entry packet with MSS 1368
 69   5301                  xde_mc_tx:entry packet with MSS 1368
 69   5301                  xde_mc_tx:entry packet with MSS 1368
 69   5301                  xde_mc_tx:entry packet with MSS 1368
  1   5301                  xde_mc_tx:entry packet with MSS 1368
 69   5301                  xde_mc_tx:entry packet with MSS 1368
 69   5301                  xde_mc_tx:entry packet with MSS 1368
^C

BRM23230018 # dtrace -n 't4_eth_tx:entry{ this->m = (mblk_t *)arg1; this->mss = this->m->b_datap->db_struioun.cksum.pad } t4_eth_tx:entry/this->m->b_datap->db_struioun.cksum.flags && this-
>mss != 0xBADD && this->mss/{printf("packet with MSS %d", this->m->b_datap->db_struioun.cksum.pad);}'
dtrace: description 't4_eth_tx:entry' matched 2 probes
CPU     ID                    FUNCTION:NAME
  9   6360                  t4_eth_tx:entry packet with MSS 8928
 34   6360                  t4_eth_tx:entry packet with MSS 8928
117   6360                  t4_eth_tx:entry packet with MSS 8928
120   6360                  t4_eth_tx:entry packet with MSS 8928
110   6360                  t4_eth_tx:entry packet with MSS 8928
120   6360                  t4_eth_tx:entry packet with MSS 8928
 86   6360                  t4_eth_tx:entry packet with MSS 8928
 86   6360                  t4_eth_tx:entry packet with MSS 8928
 77   6360                  t4_eth_tx:entry packet with MSS 8928
 78   6360                  t4_eth_tx:entry packet with MSS 8928
 87   6360                  t4_eth_tx:entry packet with MSS 1368
  8   6360                  t4_eth_tx:entry packet with MSS 8928
 69   6360                  t4_eth_tx:entry packet with MSS 1368
 69   6360                  t4_eth_tx:entry packet with MSS 1368
 69   6360                  t4_eth_tx:entry packet with MSS 1368
 78   6360                  t4_eth_tx:entry packet with MSS 8928
 69   6360                  t4_eth_tx:entry packet with MSS 1368
 69   6360                  t4_eth_tx:entry packet with MSS 1368
 69   6360                  t4_eth_tx:entry packet with MSS 1368
 69   6360                  t4_eth_tx:entry packet with MSS 1368
 43   6360                  t4_eth_tx:entry packet with MSS 1368
 69   6360                  t4_eth_tx:entry packet with MSS 1368
 69   6360                  t4_eth_tx:entry packet with MSS 1368
 69   6360                  t4_eth_tx:entry packet with MSS 1368
 69   6360                  t4_eth_tx:entry packet with MSS 1368
 69   6360                  t4_eth_tx:entry packet with MSS 1368
 43   6360                  t4_eth_tx:entry packet with MSS 1368
 43   6360                  t4_eth_tx:entry packet with MSS 1368
 69   6360                  t4_eth_tx:entry packet with MSS 1368
 69   6360                  t4_eth_tx:entry packet with MSS 1368
 69   6360                  t4_eth_tx:entry packet with MSS 1368
 78   6360                  t4_eth_tx:entry packet with MSS 8928
 69   6360                  t4_eth_tx:entry packet with MSS 1368
 69   6360                  t4_eth_tx:entry packet with MSS 1368
 43   6360                  t4_eth_tx:entry packet with MSS 1368
 43   6360                  t4_eth_tx:entry packet with MSS 1368
^C

The latter query is also catching underlay traffic, hence the 8928B entries. An MSS of 1368 matches what I'd maybe expect for a reduced MTU of 1402 bytes (resp. 1448 for a 1500B MTU). Do we have any hits on the new rules?

BRM23230018 # opteadm dump-layer -p opte0 firewall
Port opte0 - Layer firewall
======================================================================
Inbound Flows
----------------------------------------------------------------------
PROTO  SRC IP        SPORT  DST IP      DPORT  HITS  ACTION
TCP    172.20.17.94  50056  172.30.2.6  443    0     no-op
TCP    172.20.17.94  50062  172.30.2.6  443    0     no-op

Outbound Flows
----------------------------------------------------------------------
PROTO  SRC IP      SPORT  DST IP        DPORT  HITS  ACTION
TCP    172.30.2.6  443    172.20.17.94  50056  1     no-op
TCP    172.30.2.6  443    172.20.17.94  50062  1     no-op

Inbound Rules
----------------------------------------------------------------------
ID   PRI    HITS  PREDICATES                               ACTION
126  65534  2     inner.ip.proto=TCP                       "Stateful Allow"
                  inner.ulp.dst=80,=443

125  65534  0     inner.icmp.type=time exceeded            "Stateful Allow"
124  65534  0     inner.icmp.type=destination unreachable  "Stateful Allow"
                  inner.icmp.code=4

DEF  --     427   --                                       "deny"

Outbound Rules
----------------------------------------------------------------------
ID   PRI  HITS  PREDICATES  ACTION
DEF  --   2     --          "stateful allow"

Apparently not, although it's hard to tell given that the reconcile task is completely reinstating the rules periodically so we're losing those counters. Note that all the deny entries here are from my own ping https://recovery.sys.dublin.eng.oxide.computer command.

Dogfood

BRM44220011 # dtrace -n 'xde_mc_tx:entry{ this->m = (mblk_t *)arg1; this->mss = this->m->b_datap->db_struioun.cksum.pad } xde_mc_tx:entry/this->m->b_datap->db_struioun.cksum.flags && this-
>mss != 0xBADD && this->mss/{printf("packet with MSS %d", this->m->b_datap->db_struioun.cksum.pad);}'
dtrace: description 'xde_mc_tx:entry' matched 2 probes
CPU     ID                    FUNCTION:NAME
 41   4868                  xde_mc_tx:entry packet with MSS 1448
  9   4868                  xde_mc_tx:entry packet with MSS 1368
 68   4868                  xde_mc_tx:entry packet with MSS 1368
 68   4868                  xde_mc_tx:entry packet with MSS 1368
 68   4868                  xde_mc_tx:entry packet with MSS 1368
 68   4868                  xde_mc_tx:entry packet with MSS 1368
 68   4868                  xde_mc_tx:entry packet with MSS 1368
 68   4868                  xde_mc_tx:entry packet with MSS 1368
 68   4868                  xde_mc_tx:entry packet with MSS 1368
 68   4868                  xde_mc_tx:entry packet with MSS 1368
127   4868                  xde_mc_tx:entry packet with MSS 1368
 68   4868                  xde_mc_tx:entry packet with MSS 1368
 68   4868                  xde_mc_tx:entry packet with MSS 1368
 68   4868                  xde_mc_tx:entry packet with MSS 1368
 68   4868                  xde_mc_tx:entry packet with MSS 1368
 68   4868                  xde_mc_tx:entry packet with MSS 1368
 68   4868                  xde_mc_tx:entry packet with MSS 1368
 68   4868                  xde_mc_tx:entry packet with MSS 1368
 68   4868                  xde_mc_tx:entry packet with MSS 1368
 68   4868                  xde_mc_tx:entry packet with MSS 1368
 68   4868                  xde_mc_tx:entry packet with MSS 1368
 83   4868                  xde_mc_tx:entry packet with MSS 1368
 83   4868                  xde_mc_tx:entry packet with MSS 1368
 83   4868                  xde_mc_tx:entry packet with MSS 1368
 83   4868                  xde_mc_tx:entry packet with MSS 1368
^C

BRM44220011 # dtrace -n 't4_eth_tx:entry{ this->m = (mblk_t *)arg1; this->mss = this->m->b_datap->db_struioun.cksum.pad } t4_eth_tx:entry/this->m->b_datap->db_struioun.cksum.flags && this->mss != 0xBADD && this->mss/{printf("packet with MSS %d", this->m->b_datap->db_struioun.cksum.pad);}'
dtrace: description 't4_eth_tx:entry' matched 2 probes
CPU     ID                    FUNCTION:NAME
 18   5927                  t4_eth_tx:entry packet with MSS 8928
 52   5927                  t4_eth_tx:entry packet with MSS 8928
 52   5927                  t4_eth_tx:entry packet with MSS 1448
113   5927                  t4_eth_tx:entry packet with MSS 1368
 68   5927                  t4_eth_tx:entry packet with MSS 1368
 68   5927                  t4_eth_tx:entry packet with MSS 1368
 68   5927                  t4_eth_tx:entry packet with MSS 1368
 76   5927                  t4_eth_tx:entry packet with MSS 8928
 68   5927                  t4_eth_tx:entry packet with MSS 1368
 68   5927                  t4_eth_tx:entry packet with MSS 1368
 68   5927                  t4_eth_tx:entry packet with MSS 1368
 68   5927                  t4_eth_tx:entry packet with MSS 1368
 68   5927                  t4_eth_tx:entry packet with MSS 1368
 68   5927                  t4_eth_tx:entry packet with MSS 1368
 68   5927                  t4_eth_tx:entry packet with MSS 1368
 44   5927                  t4_eth_tx:entry packet with MSS 1448
 68   5927                  t4_eth_tx:entry packet with MSS 1448
 68   5927                  t4_eth_tx:entry packet with MSS 1448
 68   5927                  t4_eth_tx:entry packet with MSS 1448
 68   5927                  t4_eth_tx:entry packet with MSS 1448
 44   5927                  t4_eth_tx:entry packet with MSS 1448
 68   5927                  t4_eth_tx:entry packet with MSS 1448
 68   5927                  t4_eth_tx:entry packet with MSS 1448
 68   5927                  t4_eth_tx:entry packet with MSS 1448
 44   5927                  t4_eth_tx:entry packet with MSS 1448
 68   5927                  t4_eth_tx:entry packet with MSS 1448
 68   5927                  t4_eth_tx:entry packet with MSS 1448
 68   5927                  t4_eth_tx:entry packet with MSS 1448
 44   5927                  t4_eth_tx:entry packet with MSS 1448
 68   5927                  t4_eth_tx:entry packet with MSS 1448
 68   5927                  t4_eth_tx:entry packet with MSS 1448
 68   5927                  t4_eth_tx:entry packet with MSS 1448
 68   5927                  t4_eth_tx:entry packet with MSS 1448
 68   5927                  t4_eth_tx:entry packet with MSS 1448
 68   5927                  t4_eth_tx:entry packet with MSS 1448
 68   5927                  t4_eth_tx:entry packet with MSS 1448
 68   5927                  t4_eth_tx:entry packet with MSS 1448
 68   5927                  t4_eth_tx:entry packet with MSS 1448
 44   5927                  t4_eth_tx:entry packet with MSS 1448
 68   5927                  t4_eth_tx:entry packet with MSS 1448
 44   5927                  t4_eth_tx:entry packet with MSS 1448
 68   5927                  t4_eth_tx:entry packet with MSS 1448
 68   5927                  t4_eth_tx:entry packet with MSS 1448
 68   5927                  t4_eth_tx:entry packet with MSS 1448
 44   5927                  t4_eth_tx:entry packet with MSS 1448
 68   5927                  t4_eth_tx:entry packet with MSS 1448
 68   5927                  t4_eth_tx:entry packet with MSS 1448
 68   5927                  t4_eth_tx:entry packet with MSS 1448
 68   5927                  t4_eth_tx:entry packet with MSS 1448
 44   5927                  t4_eth_tx:entry packet with MSS 1448
 68   5927                  t4_eth_tx:entry packet with MSS 1448
 68   5927                  t4_eth_tx:entry packet with MSS 1448
 44   5927                  t4_eth_tx:entry packet with MSS 1448
 68   5927                  t4_eth_tx:entry packet with MSS 1448
 68   5927                  t4_eth_tx:entry packet with MSS 1448
 68   5927                  t4_eth_tx:entry packet with MSS 1448
 68   5927                  t4_eth_tx:entry packet with MSS 1448
 68   5927                  t4_eth_tx:entry packet with MSS 1448
 68   5927                  t4_eth_tx:entry packet with MSS 1448
 68   5927                  t4_eth_tx:entry packet with MSS 1448
 68   5927                  t4_eth_tx:entry packet with MSS 1448
 44   5927                  t4_eth_tx:entry packet with MSS 1448
 68   5927                  t4_eth_tx:entry packet with MSS 1448
 44   5927                  t4_eth_tx:entry packet with MSS 1448
 68   5927                  t4_eth_tx:entry packet with MSS 1448
 68   5927                  t4_eth_tx:entry packet with MSS 1448
 68   5927                  t4_eth_tx:entry packet with MSS 1448
 68   5927                  t4_eth_tx:entry packet with MSS 1448
 44   5927                  t4_eth_tx:entry packet with MSS 1448
 68   5927                  t4_eth_tx:entry packet with MSS 1448
 68   5927                  t4_eth_tx:entry packet with MSS 1448
 44   5927                  t4_eth_tx:entry packet with MSS 1448
 68   5927                  t4_eth_tx:entry packet with MSS 1448
 68   5927                  t4_eth_tx:entry packet with MSS 1448
 68   5927                  t4_eth_tx:entry packet with MSS 1448
 44   5927                  t4_eth_tx:entry packet with MSS 1448
 68   5927                  t4_eth_tx:entry packet with MSS 1448
 44   5927                  t4_eth_tx:entry packet with MSS 1448
 68   5927                  t4_eth_tx:entry packet with MSS 1368
 68   5927                  t4_eth_tx:entry packet with MSS 1368
 68   5927                  t4_eth_tx:entry packet with MSS 1368
 68   5927                  t4_eth_tx:entry packet with MSS 1368
 18   5927                  t4_eth_tx:entry packet with MSS 1368
 18   5927                  t4_eth_tx:entry packet with MSS 1368
 68   5927                  t4_eth_tx:entry packet with MSS 1368
 68   5927                  t4_eth_tx:entry packet with MSS 1368
 68   5927                  t4_eth_tx:entry packet with MSS 1368
 68   5927                  t4_eth_tx:entry packet with MSS 1368
120   5927                  t4_eth_tx:entry packet with MSS 1368
 68   5927                  t4_eth_tx:entry packet with MSS 1368
 68   5927                  t4_eth_tx:entry packet with MSS 1368
 68   5927                  t4_eth_tx:entry packet with MSS 1368
 68   5927                  t4_eth_tx:entry packet with MSS 1368
 68   5927                  t4_eth_tx:entry packet with MSS 1368
^C
 76   5927                  t4_eth_tx:entry packet with MSS 8928
 95   5927                  t4_eth_tx:entry packet with MSS 1368
 78   5927                  t4_eth_tx:entry packet with MSS 1368
 43   5927                  t4_eth_tx:entry packet with MSS 1368
 68   5927                  t4_eth_tx:entry packet with MSS 1368
 43   5927                  t4_eth_tx:entry packet with MSS 1368
 43   5927                  t4_eth_tx:entry packet with MSS 1368
 60   5927                  t4_eth_tx:entry packet with MSS 1368
 43   5927                  t4_eth_tx:entry packet with MSS 1368
 60   5927                  t4_eth_tx:entry packet with MSS 1368
 60   5927                  t4_eth_tx:entry packet with MSS 1368
  1   5927                  t4_eth_tx:entry packet with MSS 1368
113   5927                  t4_eth_tx:entry packet with MSS 1368
  1   5927                  t4_eth_tx:entry packet with MSS 1368
 69   5927                  t4_eth_tx:entry packet with MSS 1368
 86   5927                  t4_eth_tx:entry packet with MSS 1368
 52   5927                  t4_eth_tx:entry packet with MSS 1368
 68   5927                  t4_eth_tx:entry packet with MSS 1368
 42   5927                  t4_eth_tx:entry packet with MSS 1368
 42   5927                  t4_eth_tx:entry packet with MSS 1368
 42   5927                  t4_eth_tx:entry packet with MSS 1368
103   5927                  t4_eth_tx:entry packet with MSS 1368
 19   5927                  t4_eth_tx:entry packet with MSS 1368
 19   5927                  t4_eth_tx:entry packet with MSS 1368
 68   5927                  t4_eth_tx:entry packet with MSS 1368
 68   5927                  t4_eth_tx:entry packet with MSS 1368

There's a mix in there, but I assume we have something on the lab-side hitting the API periodically, since I see those 1448 entries passively. But dogfood appears to be discovering and using the reduced MTU just fine.

Fixing oxidecomputer/opte#748 may have been sufficient to get this working, and that PR has been pulled into main already. As a fun bonus, due to this work I can now run traceroute from the Nexus zone, so we might want to expand the scope of the allow rule across external DNS too. The NTP zones can't really benefit since they're behind a SNAT.

FelixMcFelix

I think this is good for review, I've pointed out some open design points I'm unsure on.

FelixMcFelix · 2025-05-27T10:04:02Z

.github/buildomat/jobs/deploy.sh

@@ -136,7 +136,7 @@ z_swadm () {

 # only set this if you want to override the version of opte/xde installed by the
 # install_opte.sh script
-OPTE_COMMIT=""
+OPTE_COMMIT="792ec8a3816ba8c7c4268f65cd19dbd946a9027d"


Noting that this needs to be removed as part of merging.

FelixMcFelix · 2025-05-27T10:13:48Z

illumos-utils/src/opte/firewall_rules.rs

+                                // Port assignments are incompatible with non
+                                // TCP/UDP protocols.
+                                if matches!(
+                                    proto,
+                                    ProtoFilter::Tcp | ProtoFilter::Udp
+                                ) {
+                                    filters.set_ports(ports.clone());
+                                }


I'm surprised we haven't run into this one yet -- if someone were to install a rule on [TCP, ICMP] x [port 80, port 443] this would previously be installed as the two rules:

proto = TCP && (port = 80 || port = 443), [valid]

proto = ICMP && (port = 80 || port = 443). [invalid]

In OPTE, a port match will always return false unless it is applied to TCP/UDP traffic.

FelixMcFelix · 2025-05-27T10:17:53Z

nexus/db-fixed-data/src/vpc_firewall_rule.rs

+        targets: vec![VpcFirewallRuleTarget::Subnet(
+            super::vpc_subnet::NEXUS_VPC_SUBNET.name().clone(),
+        )],


Do we want to include any more services here?

FelixMcFelix · 2025-05-27T10:19:17Z

nexus/db-fixed-data/src/vpc_firewall_rule.rs

+            protocols: Some(vec![
+                VpcFirewallRuleProtocol::Icmp(Some(VpcFirewallIcmpFilter {
+                    // Type 3 -- Destination Unreachable
+                    icmp_type: 3,
+                    // Code 4 -- Fragmentation needed
+                    code: Some(4.into()),
+                })),
+                VpcFirewallRuleProtocol::Icmp(Some(VpcFirewallIcmpFilter {
+                    // Type 11 -- Time Exceeded
+                    icmp_type: 11,
+                    code: None,
+                })),
+            ]),


This gives us PTMUD (although it looks as though the TCP stack can figure it out in the absence of Fragmentation Needed messages?), and the ability to use traceroute from Nexus/DNS. Is the latter needed? Are there any other ICMP families which would be crucial?

@jclulow Do you have any thoughts here on any missing types/codes which might be needed (or on whether to extend the scope to, e.g., external DNS)?

FelixMcFelix · 2025-05-27T10:28:36Z

nexus/external-api/src/lib.rs

+    async fn networking_inbound_icmp_view(
+        rqctx: RequestContext<Self::Context>,
+    ) -> Result<HttpResponseOk<bool>, HttpError>;


This works on a bool today to have a simple toggle. Would we rather have a Vec<VpcFirewallIcmpFilter> (or similar) here?

In general, I think we prefer to avoid JSON bodies that are just a single primitive type not wrapped in a response object as it's caused issues with client coe generation the past; see the final paragraph in this comment from @david-crespo:

A related but distinct issue is that we generally want API request and response bodies to be JSON objects rather than strings. We've run into issues with client generation when that has not been the case. A string is valid JSON, but I think this would be the only endpoint that returns a string. One reason to prefer objects is that objects can be extended by adding another key without changing the basic shape of the thing. If these unlikely to change their shape, consistency with other endpoints is probably a stronger argument.

Yeah, I think { "enabled": boolean } would be better. In addition to the above excellent points, it's a nice extra bit of unavoidable documentation about what the boolean represents.

That sounds good to me, it should be a simple enough change. I wasn't aware of the impact on clients here.

Should be done in 5c7caed.

nexus/external-api/src/lib.rs

nexus/src/app/vpc.rs

common/src/api/external/mod.rs

nexus/db-model/src/vpc_firewall_rule.rs

hawkw · 2025-05-28T18:45:51Z

Looks like you need to re-run integration_tests::unauthorized::test_unauthorized with EXPECTORATE=overwrite https://buildomat.eng.oxide.computer/wg/0/details/01JWBXVQWGZD1RRK5D3D1QEHT3/4jM5GiYbhdgOFpRJWfRHNBFflAiqzBgnrIo7ndUvW5ZbrRsJ/01JWBXW3KJD328SS2FYWZGXMK2#S7450

common/src/api/external/mod.rs

hawkw · 2025-05-29T18:02:45Z

I'm assuming the check-opte-ver CI failure is due to the patched OPTE commit you mentioned would be removed, but I'm not immediately sure why helios/build TUF repo has failed ...

hawkw

Some more kind of annoying nit-picking (sorry).

Other than that, this looks good to me, but I don't know if I'm the best person to answer the questions about whether there are other ICMP messages we might want to allow, so I'd hold off on merging until that's nailed down.

hawkw · 2025-05-29T18:07:33Z

common/src/api/external/mod.rs

+                Ok(Self::Icmp(Some(rhs.parse()?)))
+            }
+            (lhs, None) => {
+                Err(Error::invalid_value(lhs, "unrecognized protocol"))


I think elsewhere, when we use invalid_value, the idea is that the first part (the "label") is generally intended to be the name or type of the field that was invalid, rather than the value. for example:

omicron/nexus/src/app/switch_port.rs

Lines 66 to 72 in 80e9922

return Err(Error::invalid_value(

"md5_auth_key",

format!(

"md5 auth key for {} is longer than 80 characters",

p.addr

),

));

so, I think a more consistent use of invalid_value would be:

Suggested change

Err(Error::invalid_value(lhs, "unrecognized protocol"))

Err(Error::invalid_value(

"vpc_firewall_rule_protocol",

format!("unrecognized protocol: {lhs:?}"),

))

(and of course, this applies to other uses of invalid_value as well)

hawkw · 2025-05-29T18:14:28Z

common/src/api/external/mod.rs

+                        Error::invalid_value(
+                            range,
+                            format!("{INVALID_PORT_NUMBER_MSG}: {e}"),


similarly, I think this would maybe be more "proper" use of invalid_value as

Suggested change

Error::invalid_value(

range,

format!("{INVALID_PORT_NUMBER_MSG}: {e}"),

Error::invalid_value(

"port_number",

format!("{range}: {e}"),

)

or something?

In particular, it's worth noting that the fmt::Display implementation for the InvalidValue variant is:

"Invalid Value: {label}, {message}"

so we probably don't need to repeat the words "invalid $THING" in the error message, since it's already there.

The use of Error::invalid_value should now be following that pattern. I'd originally misinterpreted it as "value"->"err_msg", my bad.

hawkw · 2025-05-29T18:19:01Z

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

+            .await
+            .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?;


IIUC this will return a 500 error if there is no non-deleted firewall rule with the UUID SERVICES_VPC_ID and name NEXUS_ICMP_FW_RULE_NAME. I realize that because these are not user-created rules, we expect them to always exist...but I wonder if it would still be more correct to return NotFound here if they are not found?

Or, perhaps we should still return a 500 error in that case, but log a specific warning if the firewall rule isn't in the database, since that would suggest it's been deleted or something. We could use Diesel's Optional to handle the "not found" case specially and construct our own error with a more specific log message, or something?

My feeling here when choosing ErrorHandler::Server was that 500 is the right status code; the absence of this rule means we've either messed up when initially populating the database with the services VPC or we've messed the migration. I agree that throwing a more pointed log message is the right thing to do here, since these rules should be effectively immortal (short of any future work making the services firewall more customisable).

We should now be throwing a 500 with better context. Thanks for the suggestions there.

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

hawkw · 2025-05-29T18:22:11Z

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

+            .returning(VpcFirewallRule::as_returning())
+            .get_result_async(&*conn)
+            .await
+            .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?;


My comment above about what we ought to do if the firewall rule doesn't exist might apply here as well.

FelixMcFelix · 2025-05-29T22:16:54Z

I'm assuming the check-opte-ver CI failure is due to the patched OPTE commit you mentioned would be removed, but I'm not immediately sure why helios/build TUF repo has failed ...

In this case it's always going to fail because the OPTE version we're currently asking for (tools/opte_version) does not exist as a package in the helios pkg repo -- at least one of the checks on the way to deploy requires that the version specified there matches that of the commit used for opte-ioct/oxide-vpc. This is one of those things where a few jobs are doomed to fail until we merge the dataplane side, too (and create OS/falcon images); the set I'm choosing to fail is enough to get us up to the deploy check, at least.

FelixMcFelix added 3 commits May 21, 2025 13:09

Initial plumbing.

f315451

More OpteHdl fixups.

8a33170

Parity.

38bd2ff

FelixMcFelix force-pushed the felixmcfelix/icmp-for-nexus branch from 5594fc0 to 38bd2ff Compare May 21, 2025 12:46

FelixMcFelix added 13 commits May 21, 2025 13:59

Merge branch 'main' into felixmcfelix/icmp-for-nexus

c5aec33

CRDB didn't like *that* migration.

283fc56

TODO: migration test.

3d13775

Merge branch 'main' into felixmcfelix/icmp-for-nexus

da4c157

Iterating on launch failures.

29db3dd

serde_json::from_str 🫠

01f0f4c

That seems to be be the primary reason for the entire test suite having a Very Bad Time.

Merge branch 'main' into felixmcfelix/icmp-for-nexus

3f8d07e

Simple migration query.

14804cb

An idempotent column rename

983e6b7

Thanks, CRDB.

Some control over ICMP allowance.

3d1a58e

Help the deploy job along, a little.

62a8b3d

Last test fixup.

564424f

fmt.

0c19f5f

FelixMcFelix mentioned this pull request May 23, 2025

Nexus ICMP paranoia breaks Path MTU Discovery #7998

Open

FelixMcFelix added 2 commits May 23, 2025 20:20

Renaming, and some serde/integration tests.

32a4914

Self-review

6b9fa7a

FelixMcFelix commented May 27, 2025

View reviewed changes

FelixMcFelix marked this pull request as ready for review May 27, 2025 10:39

FelixMcFelix added this to the 16 milestone May 27, 2025

hawkw reviewed May 27, 2025

View reviewed changes

nexus/external-api/src/lib.rs Outdated Show resolved Hide resolved

nexus/src/app/vpc.rs Outdated Show resolved Hide resolved

common/src/api/external/mod.rs Outdated Show resolved Hide resolved

nexus/db-model/src/vpc_firewall_rule.rs Show resolved Hide resolved

Review feedback

5c7caed

hawkw reviewed May 28, 2025

View reviewed changes

common/src/api/external/mod.rs Outdated Show resolved Hide resolved

common/src/api/external/mod.rs Outdated Show resolved Hide resolved

common/src/api/external/mod.rs Outdated Show resolved Hide resolved

Review feedback, missed test fixup.

1fc7b2b

FelixMcFelix mentioned this pull request May 29, 2025

External API FromStrs could have api::external::Error as their error type #8238

Open

hawkw reviewed May 29, 2025

View reviewed changes

Further review feedback.

3d9011b

	return Err(Error::invalid_value(
	"md5_auth_key",
	format!(
	"md5 auth key for {} is longer than 80 characters",
	p.addr
	),
	));

		.await
		.map_err(\|e\| public_error_from_diesel(e, ErrorHandler::Server))?;

Allow limited inbound ICMP to Nexus, add ICMP type/code filters to firewall rules #8194

Are you sure you want to change the base?

Allow limited inbound ICMP to Nexus, add ICMP type/code filters to firewall rules #8194

Uh oh!

Conversation

FelixMcFelix commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FelixMcFelix commented May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Dublin

Dogfood

Uh oh!

FelixMcFelix 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!

david-crespo May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FelixMcFelix May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hawkw commented May 28, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hawkw commented May 29, 2025

Uh oh!

hawkw 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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FelixMcFelix commented May 29, 2025

Uh oh!

Uh oh!

FelixMcFelix commented May 21, 2025 •

edited

Loading

FelixMcFelix commented May 23, 2025 •

edited

Loading

david-crespo May 27, 2025 •

edited

Loading

FelixMcFelix May 28, 2025 •

edited

Loading