[multicast] Narrow admin-scoped to admin-local only & match Omicron updates to mcast lifecycle #162
Conversation
This PR is a precursor to a follow-up PR that leverages updated code in `omicron-common`. This gets dendrite in line with Omicron `main`, capturing an upstream type change that we needed to accomodate.
Previously, internal multicast groups accepted admin-scoped addresses
including admin-local (ff04), site-local (ff05), and org-local (ff08).
This narrows the scope to only admin-local (ff04::/16), which is what
Omicron *now* dictates.
- [ ] This should be merged after
oxidecomputer/omicron#9450 is reviewed
and merged into Omicron. We now make Dendrite/Dpd match Omicron
consistently for validation.
Key changes:
- Remove IPV6_SITE_LOCAL_PATTERN and IPV6_ORG_SCOPE_PATTERN from P4
- Update P4 table entries to only match admin-local (size 4→2)
- Add ADMIN_LOCAL_PREFIX const to dpd-types with RFC doc links
- Update validation to use `is_admin_local_multicast()` from oxnet v0.1.4
- Bump to API version 2 for doc changes (only)
- Update README with OpenAPI generation instructions
- Use new multicast subnet constants from `omicron-common` for validation
zeeshanlakhani
force-pushed
the
zl/admin-scope-oxnet
branch
from
6f00b19 to
94e91ca
Compare
zeeshanlakhani
requested review from
FelixMcFelix,
Nieuwejaar and
rcgoodfellow
|
@rcgoodfellow updated for your review. |
FelixMcFelix
left a comment
FelixMcFelix
left a comment
There was a problem hiding this comment.
Thanks Zeeshan, the updates to the allowed IP families all make sense. Just some questions on the expanded comments on the decapsualtion bitmaps in the dataplane.
Yep. No questions, just my bad in context. Fixed. |
FelixMcFelix
left a comment
FelixMcFelix
left a comment
There was a problem hiding this comment.
Thanks for fixing up those comments!
|
@rcgoodfellow, let me know if you'd like another look at this. I'll be doing a follow-up PR as well on some ssm/asm updates. |
| // | ||
| // Copyright 2025 Oxide Computer Company | ||
|
|
||
| // Port Bitmap Check Table |
There was a problem hiding this comment.
Thanks for adding this.
This diffset handles our move toward ASM groups being able to have source IPs. Includes: - Replace IpSrc::Subnet with IpSrc::Any for any-source multicast filtering - Add source filter normalization: when Any is present, collapse to single /0 entry; empty sources treated as allow-any - API versioning - Add tag ownership validation for group updates and deletes (v4 API) - Add v2/v3 API version adapters for backward compatibility - Fix test_service_ipv4_unknown_address to set NAT-only on correct port - #172 - I had to fix this here, as it was failing consistently locally.
|
|
||
| # Workaround for omicron's bootstore transitive dependency on pq-sys | ||
| [env] | ||
| DEP_PQ_LIBDIRS = "1" |
There was a problem hiding this comment.
Sadly, this is now always necessary, having updated to a new Omicron commit. It's on maghemite too: https://github.com/oxidecomputer/maghemite/blob/main/.cargo/config.toml#L13.
| switch.packet_test(vec![send], Vec::new()) | ||
| // Mark the ingress port as NAT-only so packets to unknown addresses are | ||
| // dropped rather than triggering ICMP_UNREACHABLE (see issue #172). | ||
| let (ingress_port_id, ingress_link_id) = |
There was a problem hiding this comment.
I applied a fix for #172 logic issue here, as it was consistently failing for me (vs flaky on CI).
zeeshanlakhani
changed the title
Previously, internal/underlay multicast groups accepted admin-configured addresses including admin-local (ff04), site-local (ff05), and org-local (ff08). This narrows the scope to only admin-local (ff04::/16), which is what Omicron now dictates.
Related PRs:
mainrelated deps to 64b40cb #161, making clear the minor change from Omicronmainbefore these changes.Key changes:
IPV6_SITE_LOCAL_PATTERNandIPV6_ORG_SCOPE_PATTERNfrom P4ADMIN_LOCAL_PREFIXconst to dpd-types with RFC doc linksis_admin_local_multicast()from oxnet v0.1.4omicron-commonfor validation