Skip to content

[mcast, vlan] match mcast NAT ingress on dst only (and not w/ the VLAN) #408

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
zeeshanlakhani wants to merge 3 commits into from
Open

[mcast, vlan] match mcast NAT ingress on dst only (and not w/ the VLAN) #408

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7ec1db6
[mcast, vlan] match mcast NAT ingress on dst only (and not w/ the VLAN)
zeeshanlakhani Oct 20, 2026
954ba49
[merge] merge fwd
zeeshanlakhani Oct 21, 2026
59d94d5
[merge] merge fwd
zeeshanlakhani Oct 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 20 additions & 77 deletions dpd-client/tests/integration_tests/mcast.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -808,8 +808,7 @@ async fn test_vlan_propagation_to_internal() -> TestResult {

// Verify IPv4 route table has one entry for the VLAN group.
// Route tables use simple destination address matching with
// forward_vlan(vlan_id) action. VLAN isolation (preventing translation) is
// handled by NAT ingress tables.
// forward_vlan(vlan_id) action.
let route_table = switch
.client
.table_dump("Ingress.l3_router.MulticastRouter4.tbl", false)
Expand All @@ -836,9 +835,8 @@ async fn test_vlan_propagation_to_internal() -> TestResult {
"Route entry should have forward_vlan(42) action"
);

// Verify NAT ingress table has a single entry with exact VLAN match.
// Decapsulated Geneve never reaches this table due to the
// !hdr.geneve.isValid() guard, so one entry per group suffices.
// Verify NAT ingress table has a single entry keyed on the destination
// address, regardless of the group's VLAN.
let nat_table = switch
.client
.table_dump("Ingress.nat_ingress.ingress_ipv4_mcast", false)
Expand All @@ -848,7 +846,7 @@ async fn test_vlan_propagation_to_internal() -> TestResult {
assert_eq!(
count_entries_for_ip(&nat_table.entries, "224.1.2.3"),
1,
"NAT table should have 1 entry with exact VLAN match"
"NAT table should have 1 entry per group (dst_addr only)"
);

// Verify the admin-scoped group now has access to the VLAN via NAT target
Expand Down Expand Up @@ -948,7 +946,7 @@ async fn test_group_api_lifecycle() {
"Route table should have 1 entry per group (dst_addr only)"
);

// NAT table: 1 entry per group with exact VLAN match
// NAT table: 1 entry per group (dst_addr only)
let nat_table = switch
.client
.table_dump("Ingress.nat_ingress.ingress_ipv4_mcast", false)
Expand All @@ -963,7 +961,7 @@ async fn test_group_api_lifecycle() {
assert_eq!(
nat_entries.len(),
1,
"NAT table should have 1 entry with exact VLAN match"
"NAT table should have 1 entry per group (dst_addr only)"
);

// Get all groups and verify our group is included
Expand Down Expand Up @@ -1986,13 +1984,19 @@ async fn test_ipv4_multicast_basic_replication_nat_ingress() -> TestResult {
let src_port = 3333;
let dst_port = 4444;

// The ingress VLAN (20) differs from the group's (10). NAT ingress
// matches on the destination IP, meaning that the packet is still
// admitted, and the ingress tag isn't preserved into the encapsulated
// frame.
//
// See oxidecomputer/sidecar-lite#152 (discussion_r4317211007).
let to_send = create_ipv4_multicast_packet(
multicast_ip,
src_mac,
src_ip,
src_port,
dst_port,
Some(10),
Some(20),
);

let to_recv1 = prepare_expected_pkt(
Expand Down Expand Up @@ -2745,13 +2749,19 @@ async fn test_ipv6_multicast_basic_replication_nat_ingress() -> TestResult {

let src_mac = switch.get_port_mac(ingress).unwrap();

// The ingress VLAN (20) differs from the group's (10). NAT ingress
// matches on the destination IP, meaning that the packet is still
// admitted, and the ingress tag isn't preserved into the encapsulated
// frame.
//
// See oxidecomputer/sidecar-lite#152 (discussion_r4317211007).
let to_send = create_ipv6_multicast_packet(
multicast_ip,
src_mac,
"2001:db8::1",
3333,
4444,
Some(10),
Some(20),
);

let to_recv1 = prepare_expected_pkt(
Expand Down Expand Up @@ -3880,73 +3890,6 @@ async fn test_multicast_reset_all_tables() -> TestResult {
Ok(())
}

#[tokio::test]
#[ignore]
async fn test_multicast_vlan_translation_not_possible() -> TestResult {
let switch = &*get_switch().await;

// Define test ports
let ingress = PhysPort(10);

// Create admin-scoped IPv6 underlay group that will handle actual replication
let egress1 = PhysPort(15);
let internal_multicast_ip = IpAddr::V6(MULTICAST_NAT_IP);
create_test_multicast_group(
switch,
internal_multicast_ip,
Some(TEST_TAG),
&[(egress1, types::Direction::External)],
types::InternalForwarding { nat_target: None },
types::ExternalForwarding { vlan_id: None }, // Admin-scoped groups don't need NAT targets
None,
)
.await;

// Create external group with VLAN
let multicast_ip = IpAddr::V4(MULTICAST_TEST_IPV4);
let output_vlan = Some(20);

let created_group = create_test_multicast_group(
switch,
multicast_ip,
Some(TEST_TAG),
&[], // External groups have no members
types::InternalForwarding {
nat_target: Some(create_nat_target_ipv4()),
}, // Create NAT target
types::ExternalForwarding { vlan_id: output_vlan },
None,
)
.await;

let src_mac = switch.get_port_mac(ingress).unwrap();

// Create test packet with input VLAN 10, which differs from the
// group's configured VLAN 20.
let input_vlan = 10;
let to_send = create_ipv4_multicast_packet(
multicast_ip,
src_mac,
"192.168.1.20",
4444,
5555,
Some(input_vlan),
);

let test_pkt = TestPacket { packet: Arc::new(to_send), port: ingress };

// Expect NO packets - this test demonstrates that VLAN translation
// is not possible for multicast packets
let expected_pkts = vec![];

switch.packet_test(vec![test_pkt], expected_pkts).unwrap();

cleanup_test_group(switch, get_group_ip(&created_group), TEST_TAG)
.await
.unwrap();
cleanup_test_group(switch, internal_multicast_ip, TEST_TAG).await
}

#[tokio::test]
#[ignore]
async fn test_multicast_multiple_packets() -> TestResult {
Expand Down
23 changes: 9 additions & 14 deletions dpd/p4/sidecar.p4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -631,16 +631,9 @@ control NatIngress (
}

// Separate table for IPv4 multicast packets that need to be encapsulated.
//
// Each group has a single entry keyed on (dst_addr, vlan_valid, vlan_id).
// Groups with a VLAN match on (addr, true, vlan_id). Groups without VLAN
// match on (addr, false, 0). Packets with the wrong VLAN miss and are
// not NAT encapsulated.
table ingress_ipv4_mcast {
key = {
hdr.ipv4.dst_addr : exact;
hdr.vlan.isValid() : exact;
hdr.vlan.vlan_id : exact;
}
actions = { mcast_forward_ipv4_to; }
const size = IPV4_MULTICAST_TABLE_SIZE;
Expand All @@ -658,12 +651,10 @@ control NatIngress (
}

// IPv6 counterpart of ingress_ipv4_mcast for IPv6 packets that need to be
// encapsulated. See ingress_ipv4_mcast for details on VLAN matching.
// encapsulated.
table ingress_ipv6_mcast {
key = {
hdr.ipv6.dst_addr : exact;
hdr.vlan.isValid() : exact;
hdr.vlan.vlan_id : exact;
}
actions = { mcast_forward_ipv6_to; }
const size = IPV6_MULTICAST_TABLE_SIZE;
Expand Down Expand Up @@ -1344,8 +1335,10 @@ control MulticastRouter4(
// If the packet came in with a VLAN tag, we need to invalidate
// the VLAN header before we do the lookup. The VLAN header
// will be re-attached if set in the forward_vlan action (or
// untagged for groups without VLAN). This prevents unintended
// VLAN translation.
// untagged for groups without VLAN).
//
// As a router, we don't preserve the ingress VLAN across a routed
// boundary, as there's no concept of VLAN isolation within the rack.
if (hdr.vlan.isValid()) {
hdr.ethernet.ether_type = hdr.vlan.ether_type;
hdr.vlan.setInvalid();
Expand Down Expand Up @@ -1478,8 +1471,10 @@ control MulticastRouter6 (
// If the packet came in with a VLAN tag, we need to invalidate
// the VLAN header before we do the lookup. The VLAN header
// will be re-attached if set in the forward_vlan action (or
// untagged for groups without VLAN). This prevents unintended
// VLAN translation.
// untagged for groups without VLAN).
//
// As a router, we don't preserve the ingress VLAN across a routed
// boundary, as there's no concept of VLAN isolation within the rack.
if (hdr.vlan.isValid()) {
hdr.ethernet.ether_type = hdr.vlan.ether_type;
hdr.vlan.setInvalid();
Expand Down
71 changes: 21 additions & 50 deletions dpd/src/mcast/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,6 @@ pub(crate) fn add_group_external(
scoped_underlay_id.id(),
nat_target,
group_info.sources.as_deref(),
group_info.external_forwarding.vlan_id,
);

// Configure external tables and handle VLAN propagation
Expand Down Expand Up @@ -663,6 +662,9 @@ pub(crate) fn modify_group_external(

validate_multicast_address(group_ip, new_group_info.sources.as_deref())?;
validate_nat_target(nat_target)?;
if let Some(vlan_id) = new_group_info.external_forwarding.vlan_id {
common::network::validate_vlan(vlan_id)?;
}

let group_entry = mcast.groups.remove(&group_ip).unwrap();
let old_nat_target = group_entry.int_fwding.nat_target;
Expand Down Expand Up @@ -1337,6 +1339,9 @@ fn validate_external_group_creation(
validate_group_exists(mcast, group_ip)?;
validate_multicast_address(group_ip, group_info.sources.as_deref())?;
validate_not_underlay_subnet(group_ip)?;
if let Some(vlan_id) = group_info.external_forwarding.vlan_id {
common::network::validate_vlan(vlan_id)?;
}
Ok(())
}

Expand Down Expand Up @@ -1369,17 +1374,12 @@ fn configure_external_tables(
add_source_filters(s, group_ip, sources_as_slice(&group_info.sources))?;

// Add NAT entry
let vlan_id = group_info.external_forwarding.vlan_id;
match group_ip {
IpAddr::V4(ipv4) => {
table::mcast::mcast_nat::add_ipv4_entry(
s, ipv4, nat_target, vlan_id,
)?;
table::mcast::mcast_nat::add_ipv4_entry(s, ipv4, nat_target)?;
}
IpAddr::V6(ipv6) => {
table::mcast::mcast_nat::add_ipv6_entry(
s, ipv6, nat_target, vlan_id,
)?;
table::mcast::mcast_nat::add_ipv6_entry(s, ipv6, nat_target)?;
}
}

Expand Down Expand Up @@ -1713,24 +1713,19 @@ fn update_external_tables(
let new_vlan_id = new_group_info.external_forwarding.vlan_id;

// Update NAT target - external groups always have NAT targets
// Also handles VLAN changes since VLAN is part of the NAT match key
let new_nat_target =
new_group_info.internal_forwarding.nat_target.ok_or_else(|| {
DpdError::Invalid(
"external groups must have NAT target".to_string(),
)
})?;

if Some(new_nat_target) != group_entry.int_fwding.nat_target
|| old_vlan_id != new_vlan_id
{
if Some(new_nat_target) != group_entry.int_fwding.nat_target {
update_nat_tables(
s,
group_ip,
Some(new_nat_target),
group_entry.int_fwding.nat_target,
old_vlan_id,
new_vlan_id,
)?;
}

Expand Down Expand Up @@ -1843,11 +1838,7 @@ fn delete_group_tables(
ipv4,
sources_as_slice(&group.sources),
)?;
table::mcast::mcast_nat::del_ipv4_entry(
s,
ipv4,
group.ext_fwding.vlan_id,
)?;
table::mcast::mcast_nat::del_ipv4_entry(s, ipv4)?;
}

delete_group_bitmap_entries(s, group)?;
Expand All @@ -1865,11 +1856,7 @@ fn delete_group_tables(
ipv6,
sources_as_slice(&group.sources),
)?;
table::mcast::mcast_nat::del_ipv6_entry(
s,
ipv6,
group.ext_fwding.vlan_id,
)?;
table::mcast::mcast_nat::del_ipv6_entry(s, ipv6)?;
}

delete_group_bitmap_entries(s, group)?;
Expand Down Expand Up @@ -1907,47 +1894,31 @@ fn update_nat_tables(
group_ip: IpAddr,
new_nat_target: Option<NatTarget>,
old_nat_target: Option<NatTarget>,
old_vlan_id: Option<u16>,
new_vlan_id: Option<u16>,
) -> DpdResult<()> {
match (group_ip, new_nat_target, old_nat_target) {
(IpAddr::V4(ipv4), Some(new_nat), Some(old_nat)) => {
// NAT to NAT -> update existing entry (handles VLAN changes internally)
table::mcast::mcast_nat::update_ipv4_entry(
s,
ipv4,
new_nat,
old_nat,
old_vlan_id,
new_vlan_id,
)
(IpAddr::V4(ipv4), Some(new_nat), Some(_)) => {
// NAT to NAT -> update existing entry
table::mcast::mcast_nat::update_ipv4_entry(s, ipv4, new_nat)
}
(IpAddr::V6(ipv6), Some(new_nat), Some(old_nat)) => {
// NAT to NAT -> update existing entry (handles VLAN changes internally)
table::mcast::mcast_nat::update_ipv6_entry(
s,
ipv6,
new_nat,
old_nat,
old_vlan_id,
new_vlan_id,
)
(IpAddr::V6(ipv6), Some(new_nat), Some(_)) => {
// NAT to NAT -> update existing entry
table::mcast::mcast_nat::update_ipv6_entry(s, ipv6, new_nat)
}
(IpAddr::V4(ipv4), Some(nat), None) => {
// No NAT to NAT - add new entry
table::mcast::mcast_nat::add_ipv4_entry(s, ipv4, nat, new_vlan_id)
table::mcast::mcast_nat::add_ipv4_entry(s, ipv4, nat)
}
(IpAddr::V6(ipv6), Some(nat), None) => {
// No NAT to NAT - add new entry
table::mcast::mcast_nat::add_ipv6_entry(s, ipv6, nat, new_vlan_id)
table::mcast::mcast_nat::add_ipv6_entry(s, ipv6, nat)
}
(IpAddr::V4(ipv4), None, Some(_)) => {
// NAT to no NAT - delete entry
table::mcast::mcast_nat::del_ipv4_entry(s, ipv4, old_vlan_id)
table::mcast::mcast_nat::del_ipv4_entry(s, ipv4)
}
(IpAddr::V6(ipv6), None, Some(_)) => {
// NAT to no NAT - delete entry
table::mcast::mcast_nat::del_ipv6_entry(s, ipv6, old_vlan_id)
table::mcast::mcast_nat::del_ipv6_entry(s, ipv6)
}
_ => Ok(()), // No change (None → None)
}
Expand Down
Loading
Loading