diff --git a/dpd-client/tests/integration_tests/mcast.rs b/dpd-client/tests/integration_tests/mcast.rs index 6c5ac347..2eda88c0 100644 --- a/dpd-client/tests/integration_tests/mcast.rs +++ b/dpd-client/tests/integration_tests/mcast.rs @@ -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) @@ -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) @@ -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 @@ -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) @@ -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 @@ -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( @@ -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( @@ -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 { diff --git a/dpd/p4/sidecar.p4 b/dpd/p4/sidecar.p4 index ff976f70..b037f099 100644 --- a/dpd/p4/sidecar.p4 +++ b/dpd/p4/sidecar.p4 @@ -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; @@ -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; @@ -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(); @@ -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(); diff --git a/dpd/src/mcast/mod.rs b/dpd/src/mcast/mod.rs index 3ac32330..f67c429e 100644 --- a/dpd/src/mcast/mod.rs +++ b/dpd/src/mcast/mod.rs @@ -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 @@ -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; @@ -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(()) } @@ -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)?; } } @@ -1713,7 +1713,6 @@ 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( @@ -1721,16 +1720,12 @@ fn update_external_tables( ) })?; - 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, )?; } @@ -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)?; @@ -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)?; @@ -1907,47 +1894,31 @@ fn update_nat_tables( group_ip: IpAddr, new_nat_target: Option, old_nat_target: Option, - old_vlan_id: Option, - new_vlan_id: Option, ) -> 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) } diff --git a/dpd/src/mcast/rollback.rs b/dpd/src/mcast/rollback.rs index fb73cd78..7d7062f2 100644 --- a/dpd/src/mcast/rollback.rs +++ b/dpd/src/mcast/rollback.rs @@ -136,7 +136,6 @@ pub(crate) struct GroupCreateRollbackContext<'a> { underlay_id: MulticastGroupId, nat_target: Option, sources: Option<&'a [IpSrc]>, - vlan_id: Option, } impl RollbackOps for GroupCreateRollbackContext<'_> { @@ -262,7 +261,6 @@ impl<'a> GroupCreateRollbackContext<'a> { underlay_id: MulticastGroupId, nat_target: NatTarget, sources: Option<&'a [IpSrc]>, - vlan_id: Option, ) -> Self { Self { switch, @@ -271,7 +269,6 @@ impl<'a> GroupCreateRollbackContext<'a> { underlay_id, nat_target: Some(nat_target), sources, - vlan_id, } } @@ -289,7 +286,6 @@ impl<'a> GroupCreateRollbackContext<'a> { underlay_id, nat_target: None, sources: None, - vlan_id: None, } } @@ -429,7 +425,6 @@ impl<'a> GroupCreateRollbackContext<'a> { table::mcast::mcast_nat::del_ipv4_entry( self.switch, ipv4, - self.vlan_id, ), ); } @@ -520,7 +515,6 @@ impl<'a> GroupCreateRollbackContext<'a> { table::mcast::mcast_nat::del_ipv6_entry( self.switch, ipv6, - self.vlan_id, ), ); } @@ -779,8 +773,7 @@ impl<'a> GroupUpdateRollbackContext<'a> { } // Restore NAT settings for external groups (internal groups have no - // NAT entries). Both new_tgt and old_tgt are the same original NAT - // target since we're restoring to the original state. + // NAT entries) by rewriting the entry with the original NAT target. if let Some(nat) = nat_target { match self.group_ip { IpAddr::V4(ipv4) => { @@ -791,9 +784,6 @@ impl<'a> GroupUpdateRollbackContext<'a> { self.switch, ipv4, nat, - nat, - self.attempted_vlan_id, - vlan_id, ), ); } @@ -805,9 +795,6 @@ impl<'a> GroupUpdateRollbackContext<'a> { self.switch, ipv6, nat, - nat, - self.attempted_vlan_id, - vlan_id, ), ); } diff --git a/dpd/src/table/mcast/mcast_nat.rs b/dpd/src/table/mcast/mcast_nat.rs index 961f1644..43a716ef 100644 --- a/dpd/src/table/mcast/mcast_nat.rs +++ b/dpd/src/table/mcast/mcast_nat.rs @@ -8,15 +8,15 @@ //! //! The NatIngress tables (`ingress_ipv4_mcast`, `ingress_ipv6_mcast`) only //! see traffic from customer ports due to the outermost `!hdr.geneve.isValid()` -//! check in the P4 pipeline. Decapsulated Geneve packets never reach these -//! tables, so each group only needs a single entry with an exact VLAN match. +//! check in the P4 pipeline. Each group has a single entry keyed on its +//! destination address. use dpd_types::table; use std::net::{Ipv4Addr, Ipv6Addr}; use crate::{Switch, table::*}; -use super::{Ipv4VlanMatchKey, Ipv6VlanMatchKey}; +use super::{Ipv4MatchKey, Ipv6MatchKey}; use aal::ActionParse; use aal_macros::*; use common::network::{MacAddr, NatTarget}; @@ -35,20 +35,12 @@ enum Ipv6Action { } /// Add a NAT entry for IPv4 multicast traffic. -/// -/// A single entry is added with the exact VLAN match key. For groups without -/// a VLAN, the match key uses `None` for the VLAN field. pub(crate) fn add_ipv4_entry( s: &Switch, ip: Ipv4Addr, tgt: NatTarget, - vlan_id: Option, ) -> DpdResult<()> { - if let Some(vlan_id) = vlan_id { - common::network::validate_vlan(vlan_id)?; - } - - let match_key = Ipv4VlanMatchKey::new(ip, vlan_id); + let match_key = Ipv4MatchKey::new(ip); let action_key = Ipv4Action::Forward { target: tgt.internal_ip, inner_mac: tgt.inner_mac, @@ -60,92 +52,39 @@ pub(crate) fn add_ipv4_entry( } /// Update a NAT entry for IPv4 multicast traffic. -/// -/// When VLAN changes, old entry is deleted and a new one added because -/// the VLAN is part of the match key and cannot be updated in place. pub(crate) fn update_ipv4_entry( s: &Switch, ip: Ipv4Addr, new_tgt: NatTarget, - old_tgt: NatTarget, - old_vlan_id: Option, - new_vlan_id: Option, ) -> DpdResult<()> { - if old_vlan_id == new_vlan_id { - let match_key = Ipv4VlanMatchKey::new(ip, old_vlan_id); - let action_key = Ipv4Action::Forward { - target: new_tgt.internal_ip, - inner_mac: new_tgt.inner_mac, - vni: new_tgt.vni.as_u32(), - }; - - debug!( - s.log, - "update ingress mcast entry {match_key} -> {action_key:?}" - ); - return s.table_entry_update( - TableType::NatIngressIpv4Mcast, - &match_key, - &action_key, - ); - } + let match_key = Ipv4MatchKey::new(ip); + let action_key = Ipv4Action::Forward { + target: new_tgt.internal_ip, + inner_mac: new_tgt.inner_mac, + vni: new_tgt.vni.as_u32(), + }; - del_ipv4_entry_with_tgt(s, ip, old_tgt, old_vlan_id)?; - if let Err(e) = add_ipv4_entry(s, ip, new_tgt, new_vlan_id) { - debug!(s.log, "add failed, restoring old NAT entries for {ip}"); - let _ = add_ipv4_entry(s, ip, old_tgt, old_vlan_id); - return Err(e); - } - Ok(()) + debug!(s.log, "update ingress mcast entry {match_key} -> {action_key:?}"); + s.table_entry_update( + TableType::NatIngressIpv4Mcast, + &match_key, + &action_key, + ) } /// Delete a NAT entry for IPv4 multicast traffic. -pub(crate) fn del_ipv4_entry( - s: &Switch, - ip: Ipv4Addr, - vlan_id: Option, -) -> DpdResult<()> { - let match_key = Ipv4VlanMatchKey::new(ip, vlan_id); +pub(crate) fn del_ipv4_entry(s: &Switch, ip: Ipv4Addr) -> DpdResult<()> { + let match_key = Ipv4MatchKey::new(ip); debug!(s.log, "delete ingress mcast entry {match_key}"); s.table_entry_del(TableType::NatIngressIpv4Mcast, &match_key) } -/// Delete a NAT entry for IPv4 multicast traffic with rollback support. -/// -/// If deletion fails, restores the entry using the provided NAT target. -pub(crate) fn del_ipv4_entry_with_tgt( - s: &Switch, - ip: Ipv4Addr, - tgt: NatTarget, - vlan_id: Option, -) -> DpdResult<()> { - let match_key = Ipv4VlanMatchKey::new(ip, vlan_id); - debug!(s.log, "delete ingress mcast entry {match_key}"); - if let Err(e) = - s.table_entry_del(TableType::NatIngressIpv4Mcast, &match_key) - { - debug!(s.log, "delete failed, restoring entry for {ip}"); - let action_key = Ipv4Action::Forward { - target: tgt.internal_ip, - inner_mac: tgt.inner_mac, - vni: tgt.vni.as_u32(), - }; - let _ = s.table_entry_add( - TableType::NatIngressIpv4Mcast, - &match_key, - &action_key, - ); - return Err(e); - } - Ok(()) -} - /// Dump the IPv4 NAT table's contents. pub(crate) fn ipv4_table_dump( s: &Switch, from_hardware: bool, ) -> DpdResult { - s.table_dump::( + s.table_dump::( TableType::NatIngressIpv4Mcast, from_hardware, ) @@ -156,10 +95,7 @@ pub(crate) fn ipv4_counter_fetch( s: &Switch, force_sync: bool, ) -> DpdResult> { - s.counter_fetch::( - force_sync, - TableType::NatIngressIpv4Mcast, - ) + s.counter_fetch::(force_sync, TableType::NatIngressIpv4Mcast) } /// Reset the Ipv4 NAT table. @@ -168,20 +104,12 @@ pub(crate) fn reset_ipv4(s: &Switch) -> DpdResult<()> { } /// Add a NAT entry for IPv6 multicast traffic. -/// -/// A single entry is added with the exact VLAN match key. For groups without -/// a VLAN, the match key uses `None` for the VLAN field. pub(crate) fn add_ipv6_entry( s: &Switch, ip: Ipv6Addr, tgt: NatTarget, - vlan_id: Option, ) -> DpdResult<()> { - if let Some(vlan_id) = vlan_id { - common::network::validate_vlan(vlan_id)?; - } - - let match_key = Ipv6VlanMatchKey::new(ip, vlan_id); + let match_key = Ipv6MatchKey::new(ip); let action_key = Ipv6Action::Forward { target: tgt.internal_ip, inner_mac: tgt.inner_mac, @@ -193,92 +121,39 @@ pub(crate) fn add_ipv6_entry( } /// Update a NAT entry for IPv6 multicast traffic. -/// -/// When VLAN changes, old entry is deleted and a new one added because -/// the VLAN is part of the match key and cannot be updated in place. pub(crate) fn update_ipv6_entry( s: &Switch, ip: Ipv6Addr, new_tgt: NatTarget, - old_tgt: NatTarget, - old_vlan_id: Option, - new_vlan_id: Option, ) -> DpdResult<()> { - if old_vlan_id == new_vlan_id { - let match_key = Ipv6VlanMatchKey::new(ip, old_vlan_id); - let action_key = Ipv6Action::Forward { - target: new_tgt.internal_ip, - inner_mac: new_tgt.inner_mac, - vni: new_tgt.vni.as_u32(), - }; - - debug!( - s.log, - "update ingress mcast entry {match_key} -> {action_key:?}" - ); - return s.table_entry_update( - TableType::NatIngressIpv6Mcast, - &match_key, - &action_key, - ); - } + let match_key = Ipv6MatchKey::new(ip); + let action_key = Ipv6Action::Forward { + target: new_tgt.internal_ip, + inner_mac: new_tgt.inner_mac, + vni: new_tgt.vni.as_u32(), + }; - del_ipv6_entry_with_tgt(s, ip, old_tgt, old_vlan_id)?; - if let Err(e) = add_ipv6_entry(s, ip, new_tgt, new_vlan_id) { - debug!(s.log, "add failed, restoring old NAT entries for {ip}"); - let _ = add_ipv6_entry(s, ip, old_tgt, old_vlan_id); - return Err(e); - } - Ok(()) + debug!(s.log, "update ingress mcast entry {match_key} -> {action_key:?}"); + s.table_entry_update( + TableType::NatIngressIpv6Mcast, + &match_key, + &action_key, + ) } /// Delete a NAT entry for IPv6 multicast traffic. -pub(crate) fn del_ipv6_entry( - s: &Switch, - ip: Ipv6Addr, - vlan_id: Option, -) -> DpdResult<()> { - let match_key = Ipv6VlanMatchKey::new(ip, vlan_id); +pub(crate) fn del_ipv6_entry(s: &Switch, ip: Ipv6Addr) -> DpdResult<()> { + let match_key = Ipv6MatchKey::new(ip); debug!(s.log, "delete ingress mcast entry {match_key}"); s.table_entry_del(TableType::NatIngressIpv6Mcast, &match_key) } -/// Delete a NAT entry for IPv6 multicast traffic with rollback support. -/// -/// If deletion fails, restores the entry using the provided NAT target. -pub(crate) fn del_ipv6_entry_with_tgt( - s: &Switch, - ip: Ipv6Addr, - tgt: NatTarget, - vlan_id: Option, -) -> DpdResult<()> { - let match_key = Ipv6VlanMatchKey::new(ip, vlan_id); - debug!(s.log, "delete ingress mcast entry {match_key}"); - if let Err(e) = - s.table_entry_del(TableType::NatIngressIpv6Mcast, &match_key) - { - debug!(s.log, "delete failed, restoring entry for {ip}"); - let action_key = Ipv6Action::Forward { - target: tgt.internal_ip, - inner_mac: tgt.inner_mac, - vni: tgt.vni.as_u32(), - }; - let _ = s.table_entry_add( - TableType::NatIngressIpv6Mcast, - &match_key, - &action_key, - ); - return Err(e); - } - Ok(()) -} - /// Dump the IPv6 NAT table's contents. pub(crate) fn ipv6_table_dump( s: &Switch, from_hardware: bool, ) -> DpdResult { - s.table_dump::( + s.table_dump::( TableType::NatIngressIpv6Mcast, from_hardware, ) @@ -289,10 +164,7 @@ pub(crate) fn ipv6_counter_fetch( s: &Switch, force_sync: bool, ) -> DpdResult> { - s.counter_fetch::( - force_sync, - TableType::NatIngressIpv6Mcast, - ) + s.counter_fetch::(force_sync, TableType::NatIngressIpv6Mcast) } /// Reset the Ipv6 NAT table. diff --git a/dpd/src/table/mcast/mcast_route.rs b/dpd/src/table/mcast/mcast_route.rs index cfa925d3..e19e4b82 100644 --- a/dpd/src/table/mcast/mcast_route.rs +++ b/dpd/src/table/mcast/mcast_route.rs @@ -10,8 +10,8 @@ //! - `forward`: Forward without VLAN modification //! - `forward_vlan(vlan_id)`: Add VLAN tag on egress //! -//! VLAN-based access control (preventing VLAN translation) is handled by NAT -//! ingress tables before encapsulation, not by route tables. +//! NAT ingress also matches on destination address only. The group VLAN +//! gets applied on egress. use dpd_types::table; use std::net::{Ipv4Addr, Ipv6Addr}; diff --git a/dpd/src/table/mcast/mod.rs b/dpd/src/table/mcast/mod.rs index 5630d016..1f49cbab 100644 --- a/dpd/src/table/mcast/mod.rs +++ b/dpd/src/table/mcast/mod.rs @@ -54,69 +54,3 @@ impl fmt::Display for Ipv6MatchKey { write!(f, "{}", self.dst_addr) } } - -/// VLAN-aware match key for NAT ingress tables. -/// -/// Matches on destination address, VLAN header validity, and VLAN ID. A single -/// entry is installed per group with an exact VLAN match, so packets with a -/// mismatched VLAN miss and are dropped rather than being translated to another -/// customer's VLAN. -#[derive(MatchParse, Hash)] -pub(super) struct Ipv4VlanMatchKey { - dst_addr: Ipv4Addr, - #[match_xlate(name = "$valid")] - vlan_valid: bool, - vlan_id: u16, -} - -impl Ipv4VlanMatchKey { - pub(super) fn new(dst_addr: Ipv4Addr, vlan_id: Option) -> Self { - match vlan_id { - Some(id) => Self { dst_addr, vlan_valid: true, vlan_id: id }, - None => Self { dst_addr, vlan_valid: false, vlan_id: 0 }, - } - } -} - -impl fmt::Display for Ipv4VlanMatchKey { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - if self.vlan_valid { - write!(f, "{} vlan={}", self.dst_addr, self.vlan_id) - } else { - write!(f, "{} untagged", self.dst_addr) - } - } -} - -/// VLAN-aware match key for NAT ingress tables. -/// -/// Matches on destination address, VLAN header validity, and VLAN ID. A single -/// entry is installed per group with an exact VLAN match, so packets with a -/// mismatched VLAN miss and are dropped rather than being translated to another -/// customer's VLAN. -#[derive(MatchParse, Hash)] -pub(super) struct Ipv6VlanMatchKey { - dst_addr: Ipv6Addr, - #[match_xlate(name = "$valid")] - vlan_valid: bool, - vlan_id: u16, -} - -impl Ipv6VlanMatchKey { - pub(super) fn new(dst_addr: Ipv6Addr, vlan_id: Option) -> Self { - match vlan_id { - Some(id) => Self { dst_addr, vlan_valid: true, vlan_id: id }, - None => Self { dst_addr, vlan_valid: false, vlan_id: 0 }, - } - } -} - -impl fmt::Display for Ipv6VlanMatchKey { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - if self.vlan_valid { - write!(f, "{} vlan={}", self.dst_addr, self.vlan_id) - } else { - write!(f, "{} untagged", self.dst_addr) - } - } -}