Add support for ICMPv6 protocol filter in firewall rules#10101
Add support for ICMPv6 protocol filter in firewall rules#10101
Conversation
bnaecker
force-pushed
the
ben/icmpv6-firewall-support
branch
2 times, most recently
from
b91f7f1 to
b574dcd
Compare
|
I'm opening this up for review, even though there are still conflicts to address with |
This splits out the pre-existing, shared firewall protocol enum from `omicron-common` into local versions for Nexus and the sled-agent. This reflects the fact that the type was present from the initial version anyway, and lets make normal updates to the API. This also adds new versions to the Nexus and sled-agent APIs with type conversions and endpoint handlers. Fixes #9908
bnaecker
force-pushed
the
ben/icmpv6-firewall-support
branch
from
b574dcd to
a541bfd
Compare
jgallagher
left a comment
jgallagher
left a comment
There was a problem hiding this comment.
Looks good, just a few nits / questions, almost entirely of the "didn't change in this PR but since you're here..." variety.
| .instrument_dropshot_handler(&rqctx, async { | ||
| sa.firewall_rules_ensure(body_args.vni, &body_args.rules[..]) | ||
| let rules: Vec<_> = | ||
| body_args.rules.into_iter().map(Into::into).collect(); |
There was a problem hiding this comment.
Should we bump a new version of VpcFirewallRulesEnsureBody too, to avoid needing to do this conversion inline?
| external_ips: local_config.external_ips, | ||
| multicast_groups: local_config.multicast_groups, | ||
| firewall_rules: local_config.firewall_rules, | ||
| firewall_rules: local_config |
There was a problem hiding this comment.
Similar question as above - can we change the type of whichever of these is old so we don't have to convert?
There was a problem hiding this comment.
Ok, I misread what line you were commenting on. We're not converting between versions of the type in the sled-agent API here. We are converting from the sled-agent API type to the type in omicron_common::api::internal::shared, which is what the sled-agent uses internally.
I wasn't entirely sure how to resolve this. This type really is shared between Nexus and the sled-agent (and a lot of other crates), but it also feels awkward to convert here.
There was a problem hiding this comment.
Hmm. This could certainly be done independently of this PR, but I think my going-in position would be:
- if a type needs to be versioned, we should move it out of common
- its destination should be whatever the "lowest level" types crate is that needs it - often that's
sled-agent-types-versionsif it ends up getting pushed down to sled-agent somehow - it's fine for
nexus-types-versionsto reexport types fromsled-agent-types-versionsif they're identical - it's also be fine for Nexus to define its own flavor of a type from
sled-agent-types-versionsif it needs to be different - lean on
replacedirectives in progenitor clients so we don't have to do manual conversions all over the place
| #[derive(Clone, Copy, Debug, PartialEq, Deserialize, Serialize, JsonSchema)] | ||
| #[serde(rename_all = "snake_case")] | ||
| #[serde(tag = "type", content = "value")] | ||
| pub enum VpcFirewallRuleProtocol { |
There was a problem hiding this comment.
Can we move this type out of common altogether, now that we need to rev its version? Looks like it would need to go into sled-agent-types-versions (which can be used / re-exported by nexus-types).
There was a problem hiding this comment.
I think we can do that, yes. It feels a little odd to me to have the One True Definition in the sled-agent, but I don't think it makes the situation worse than it does today.
There was a problem hiding this comment.
Ok, I don't think we can easily do this, and would like to defer it. The issue is the illumos-utils crate, really. That currently depends on omicron_common for this type (and many others). If we move this out to the sled-agent, we'd have a circular dependency. We could break that, but I think it requires a new crate because of how the the internals of mapping from the API types for firewall rules to OPTE's types happens inside the PortManager.
It's all doable, but a pretty big change that I'd like to separate out.
There was a problem hiding this comment.
Welp #10070 (comment) haha. +1 on separating it out, happy to chat offline about ideas for tackling this, but since you and @zeeshanlakhani are both hitting it it's probably worth sorting out pretty soon.
…or firewall protocols
|
All the outstanding issues are about converting between types in |
|
I filed #10139 to track adding this new crate and resolving the outstanding issues on this PR. |
- Moved v29 of sled-agent past others to v31 - Moved v2026_03_14_00 of nexus past others to v2026_03_24_00
3 participants
This splits out the pre-existing, shared firewall protocol enum from
omicron-commoninto local versions for Nexus and the sled-agent. This reflects the fact that the type was present from the initial version anyway, and lets make normal updates to the API. This also adds new versions to the Nexus and sled-agent APIs with type conversions and endpoint handlers.