diff --git a/dpd-client/tests/integration_tests/mcast.rs b/dpd-client/tests/integration_tests/mcast.rs index 2b56698..b3f25cb 100644 --- a/dpd-client/tests/integration_tests/mcast.rs +++ b/dpd-client/tests/integration_tests/mcast.rs @@ -50,6 +50,27 @@ impl ToIpAddr for types::UnderlayMulticastIpv6 { } } +/// Count table entries matching a specific IP address in any key field. +fn count_entries_for_ip(entries: &[types::TableEntry], ip: &str) -> usize { + entries.iter().filter(|e| e.keys.values().any(|v| v.contains(ip))).count() +} + +/// Check if any table entry for the given IP has a `forward_vlan` action with +/// the specified VLAN ID. +fn has_vlan_action_for_ip( + entries: &[types::TableEntry], + ip: &str, + vlan: u16, +) -> bool { + entries.iter().any(|e| { + e.keys.values().any(|v| v.contains(ip)) + && e.action_args + .get("vlan_id") + .map(|v| v == &vlan.to_string()) + .unwrap_or(false) + }) +} + async fn check_counter_incremented( switch: &Switch, counter_name: &str, @@ -299,6 +320,7 @@ fn create_ipv4_multicast_packet( src_ip: &str, src_port: u16, dst_port: u16, + vlan_id: Option, ) -> packet::Packet { let multicast_ip = match multicast_ip_addr { IpAddr::V4(addr) => addr, @@ -326,8 +348,14 @@ fn create_ipv4_multicast_packet( ) .unwrap(); - // Generate a UDP packet - common::gen_udp_packet(src_endpoint, dst_endpoint) + let mut pkt = common::gen_udp_packet(src_endpoint, dst_endpoint); + if let Some(vlan_id) = vlan_id { + eth::EthHdr::add_8021q( + &mut pkt, + eth::EthQHdr { eth_pcp: 0, eth_dei: 0, eth_vlan_tag: vlan_id }, + ); + } + pkt } fn create_ipv6_multicast_packet( @@ -336,6 +364,7 @@ fn create_ipv6_multicast_packet( src_ip: &str, src_port: u16, dst_port: u16, + vlan_id: Option, ) -> packet::Packet { let multicast_ip = match multicast_ip_addr { IpAddr::V6(addr) => addr, @@ -365,8 +394,14 @@ fn create_ipv6_multicast_packet( ) .unwrap(); - // Generate a UDP packet - common::gen_udp_packet(src_endpoint, dst_endpoint) + let mut pkt = common::gen_udp_packet(src_endpoint, dst_endpoint); + if let Some(vlan_id) = vlan_id { + eth::EthHdr::add_8021q( + &mut pkt, + eth::EthQHdr { eth_pcp: 0, eth_dei: 0, eth_vlan_tag: vlan_id }, + ); + } + pkt } /// Prepare the expected packet for multicast testing that either goes @@ -381,12 +416,14 @@ fn prepare_expected_pkt( match nat_target { Some(nat) => { // Deparse the incoming packet so we can copy it into the encapsulated - // packet + // packet. The P4 pipeline strips the VLAN tag before Geneve + // encapsulation (hdr.vlan.setInvalid()), so remove it here. let ingress_payload = { let mut encapped = send_pkt.clone(); let eth = encapped.hdrs.eth_hdr.as_mut().unwrap(); eth.eth_smac = MacAddr::new(0, 0, 0, 0, 0, 0); eth.eth_dmac = nat.inner_mac.clone().into(); + eth.eth_8021q = None; encapped.deparse().unwrap().to_vec() }; @@ -427,14 +464,15 @@ fn prepare_expected_pkt( ipv6::Ipv6Hdr::adjust_hlim(&mut recv_pkt, -1); } - // Add VLAN tag if required + // Set the egress VLAN tag to match the group config. The P4 + // pipeline applies the group's VLAN on egress via forward_vlan. if let Some(vlan_id) = vlan { - recv_pkt.hdrs.eth_hdr.as_mut().unwrap().eth_8021q = - Some(eth::EthQHdr { - eth_pcp: 0, - eth_dei: 0, - eth_vlan_tag: vlan_id, - }); + let eth = recv_pkt.hdrs.eth_hdr.as_mut().unwrap(); + eth.eth_8021q = Some(eth::EthQHdr { + eth_pcp: 0, + eth_dei: 0, + eth_vlan_tag: vlan_id, + }); } // Rewrite src mac @@ -536,6 +574,66 @@ async fn test_group_creation_with_validation() -> TestResult { _ => panic!("Expected ErrorResponse for invalid group ID"), } + // Test with reserved VLAN ID 0 (also invalid) + let external_vlan0 = types::MulticastGroupCreateExternalEntry { + group_ip: IpAddr::V4(MULTICAST_TEST_IPV4), + tag: Some(TEST_TAG.to_string()), + internal_forwarding: types::InternalForwarding { + nat_target: Some(nat_target.clone()), + }, + external_forwarding: types::ExternalForwarding { + vlan_id: Some(0), // Invalid: VLAN 0 is reserved + }, + sources: None, + }; + + let res = switch + .client + .multicast_group_create_external(&external_vlan0) + .await + .expect_err("Should fail with reserved VLAN ID 0"); + + match res { + Error::ErrorResponse(inner) => { + assert_eq!( + inner.status(), + 400, + "Expected 400 Bad Request status code for VLAN 0" + ); + } + _ => panic!("Expected ErrorResponse for reserved VLAN ID 0"), + } + + // Test with reserved VLAN ID 1 (also invalid) + let external_vlan1 = types::MulticastGroupCreateExternalEntry { + group_ip: IpAddr::V4(MULTICAST_TEST_IPV4), + tag: Some(TEST_TAG.to_string()), + internal_forwarding: types::InternalForwarding { + nat_target: Some(nat_target.clone()), + }, + external_forwarding: types::ExternalForwarding { + vlan_id: Some(1), // Invalid: VLAN 1 is reserved + }, + sources: None, + }; + + let res = switch + .client + .multicast_group_create_external(&external_vlan1) + .await + .expect_err("Should fail with reserved VLAN ID 1"); + + match res { + Error::ErrorResponse(inner) => { + assert_eq!( + inner.status(), + 400, + "Expected 400 Bad Request status code for VLAN 1" + ); + } + _ => panic!("Expected ErrorResponse for reserved VLAN ID 1"), + } + // Test with valid parameters // IPv4 groups are always external let external_valid = types::MulticastGroupCreateExternalEntry { @@ -704,8 +802,53 @@ async fn test_vlan_propagation_to_internal() -> TestResult { "ff04::200".parse::().unwrap() ); - // Verify the admin-scoped group now has access to the VLAN via NAT target reference - // Check the bitmap table to see if VLAN 42 is properly set (this is where VLAN matters for P4) + // 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. + let route_table = switch + .client + .table_dump("pipe.Ingress.l3_router.MulticastRouter4.tbl", false) + .await + .expect("Should dump IPv4 route table") + .into_inner(); + let group_entries: Vec<_> = route_table + .entries + .iter() + .filter(|e| e.keys.values().any(|v| v.contains("224.1.2.3"))) + .collect(); + assert_eq!( + group_entries.len(), + 1, + "Route table uses dst_addr only -> 1 entry per group" + ); + // Verify the action is forward_vlan with VLAN 42 + assert!( + group_entries[0] + .action_args + .get("vlan_id") + .map(|v| v == "42") + .unwrap_or(false), + "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. + let nat_table = switch + .client + .table_dump("pipe.Ingress.nat_ingress.ingress_ipv4_mcast", false) + .await + .expect("Should dump NAT ingress table") + .into_inner(); + assert_eq!( + count_entries_for_ip(&nat_table.entries, "224.1.2.3"), + 1, + "NAT table should have 1 entry with exact VLAN match" + ); + + // Verify the admin-scoped group now has access to the VLAN via NAT target + // reference. let bitmap_table = switch .client .table_dump("pipe.Egress.mcast_egress.tbl_decap_ports", false) @@ -713,7 +856,8 @@ async fn test_vlan_propagation_to_internal() -> TestResult { .expect("Should clean up internal group") .into_inner(); - // Verify the admin-scoped group's bitmap entry has VLAN 42 from external group propagation + // Verify the admin-scoped group's bitmap entry has VLAN 42 from external + // group propagation. assert!( bitmap_table .entries @@ -722,7 +866,8 @@ async fn test_vlan_propagation_to_internal() -> TestResult { "Admin-scoped group bitmap should have VLAN 42 from external group" ); - // Delete external group first since it references the internal group via NAT target + // Delete external group first since it references the internal group via + // NAT target cleanup_test_group(switch, created_external.group_ip, TEST_TAG) .await .unwrap(); @@ -781,6 +926,42 @@ async fn test_group_api_lifecycle() { ); assert_eq!(created.external_forwarding.vlan_id, Some(vlan_id)); + // Route table: 1 entry (dst_addr only, VLAN via action) + let route_table = switch + .client + .table_dump("pipe.Ingress.l3_router.MulticastRouter4.tbl", false) + .await + .expect("Should dump route table") + .into_inner(); + let route_entries: Vec<_> = route_table + .entries + .iter() + .filter(|e| e.keys.values().any(|v| v.contains("224.0.1.0"))) + .collect(); + assert_eq!( + route_entries.len(), + 1, + "Route table should have 1 entry per group (dst_addr only)" + ); + + // NAT table: 1 entry per group with exact VLAN match + let nat_table = switch + .client + .table_dump("pipe.Ingress.nat_ingress.ingress_ipv4_mcast", false) + .await + .expect("Should dump NAT table") + .into_inner(); + let nat_entries: Vec<_> = nat_table + .entries + .iter() + .filter(|e| e.keys.values().any(|v| v.contains("224.0.1.0"))) + .collect(); + assert_eq!( + nat_entries.len(), + 1, + "NAT table should have 1 entry with exact VLAN match" + ); + // Get all groups and verify our group is included let groups = switch .client @@ -1613,6 +1794,7 @@ async fn test_multicast_ttl_zero() -> TestResult { src_ip, src_port, dst_port, + Some(10), ); // Set TTL to 0 (should be dropped) @@ -1693,6 +1875,7 @@ async fn test_multicast_ttl_one() -> TestResult { src_ip, src_port, dst_port, + Some(10), ); // Set TTL to 1 - this should be dropped for multicast @@ -1805,6 +1988,7 @@ async fn test_ipv4_multicast_basic_replication_nat_ingress() -> TestResult { src_ip, src_port, dst_port, + Some(10), ); let to_recv1 = prepare_expected_pkt( @@ -1911,6 +2095,7 @@ async fn test_encapped_multicast_geneve_mcast_tag_to_external_members() src_ip, src_port, dst_port, + Some(10), ); // Modify the original packet to set the multicast tag, Vlan ID, @@ -1926,7 +2111,7 @@ async fn test_encapped_multicast_geneve_mcast_tag_to_external_members() let nat_target = create_nat_target_ipv4(); // Skip Ethernet header as it will be added by gen_geneve_packet - let eth_hdr_len = 14; // Standard Ethernet header length + let eth_hdr_len = og_pkt.hdrs.eth_hdr.as_ref().unwrap().hdr_len(); let payload = og_pkt.deparse().unwrap()[eth_hdr_len..].to_vec(); // Create the Geneve packet with mcast_tag = 0 @@ -2039,13 +2224,14 @@ async fn test_encapped_multicast_geneve_mcast_tag_to_underlay_members() src_ip, src_port, dst_port, + Some(10), ); // Emulate Nat Target from previous packet in the chain. let nat_target = create_nat_target_ipv6(); // Skip Ethernet header as it will be added by gen_geneve_packet - let eth_hdr_len = 14; // Standard Ethernet header length + let eth_hdr_len = og_pkt.hdrs.eth_hdr.as_ref().unwrap().hdr_len(); let payload = og_pkt.deparse().unwrap()[eth_hdr_len..].to_vec(); let geneve_src = Endpoint::parse( @@ -2174,13 +2360,14 @@ async fn test_encapped_multicast_geneve_mcast_tag_to_underlay_and_external_membe src_ip, src_port, dst_port, + Some(10), ); // Emulate Nat Target from previous packet in the chain. let nat_target = create_nat_target_ipv6(); // Skip Ethernet header as it will be added by gen_geneve_packet - let eth_hdr_len = 14; // Standard Ethernet header length + let eth_hdr_len = og_pkt.hdrs.eth_hdr.as_ref().unwrap().hdr_len(); let payload = og_pkt.deparse().unwrap()[eth_hdr_len..].to_vec(); // Create the Geneve packet with mcast_tag = 2 @@ -2303,6 +2490,7 @@ async fn test_ipv4_multicast_drops_ingress_is_egress_port() -> TestResult { src_ip, src_port, dst_port, + None, ); let test_pkt = TestPacket { packet: Arc::new(to_send), port: ingress }; @@ -2382,6 +2570,7 @@ async fn test_ipv6_multicast_hop_limit_zero() -> TestResult { "2001:db8::1", 3333, 4444, + Some(10), ); // Set Hop Limit to 0 (should be dropped) @@ -2466,6 +2655,7 @@ async fn test_ipv6_multicast_hop_limit_one() -> TestResult { src_ip, src_port, dst_port, + Some(10), ); // Set Hop Limit to 1 - this should be dropped for multicast @@ -2557,6 +2747,7 @@ async fn test_ipv6_multicast_basic_replication_nat_ingress() -> TestResult { "2001:db8::1", 3333, 4444, + Some(10), ); let to_recv1 = prepare_expected_pkt( @@ -2652,6 +2843,7 @@ async fn test_ipv4_multicast_source_filtering_exact_match() -> TestResult { &allowed_src_ip.to_string(), 3333, 4444, + Some(10), ); let filtered_pkt = create_ipv4_multicast_packet( @@ -2660,6 +2852,7 @@ async fn test_ipv4_multicast_source_filtering_exact_match() -> TestResult { &filtered_src_ip.to_string(), 3333, 4444, + Some(10), ); let to_recv11 = prepare_expected_pkt( @@ -2774,6 +2967,7 @@ async fn test_ipv4_multicast_source_filtering_multiple_exact() -> TestResult { &allowed_src_ip1.to_string(), 3333, 4444, + Some(10), ); let allowed_pkt2 = create_ipv4_multicast_packet( @@ -2782,6 +2976,7 @@ async fn test_ipv4_multicast_source_filtering_multiple_exact() -> TestResult { &allowed_src_ip2.to_string(), 3333, 4444, + Some(10), ); let filtered_pkt = create_ipv4_multicast_packet( @@ -2790,6 +2985,7 @@ async fn test_ipv4_multicast_source_filtering_multiple_exact() -> TestResult { &filtered_src_ip.to_string(), 3333, 4444, + Some(10), ); let to_recv11 = prepare_expected_pkt( @@ -2923,6 +3119,7 @@ async fn test_ipv6_multicast_multiple_source_filtering() -> TestResult { &allowed_src_ip1.to_string(), 3333, 4444, + Some(10), ); let allowed_pkt2 = create_ipv6_multicast_packet( @@ -2931,6 +3128,7 @@ async fn test_ipv6_multicast_multiple_source_filtering() -> TestResult { &allowed_src_ip2.to_string(), 3333, 4444, + Some(10), ); let filtered_pkt = create_ipv6_multicast_packet( @@ -2939,6 +3137,7 @@ async fn test_ipv6_multicast_multiple_source_filtering() -> TestResult { "2001:db8::3", // Not in the allowed sources list 3333, 4444, + Some(10), ); let to_recv11 = prepare_expected_pkt( @@ -3063,6 +3262,7 @@ async fn test_multicast_dynamic_membership() -> TestResult { "192.168.1.10", 3333, 4444, + Some(10), ); let to_recv1 = prepare_expected_pkt( switch, @@ -3151,14 +3351,14 @@ async fn test_multicast_dynamic_membership() -> TestResult { let to_recv1_new = prepare_expected_pkt( switch, &to_send, - None, + vlan, get_nat_target(&created_group), Some(egress2), ); let to_recv2_new = prepare_expected_pkt( switch, &to_send, - None, + vlan, get_nat_target(&created_group), Some(egress3), ); @@ -3250,6 +3450,7 @@ async fn test_multicast_multiple_groups() -> TestResult { "192.168.1.10", 3333, 4444, + Some(10), ); let to_send2 = create_ipv4_multicast_packet( @@ -3258,6 +3459,7 @@ async fn test_multicast_multiple_groups() -> TestResult { "192.168.1.10", 3333, 4444, + Some(20), ); let to_recv1_1 = prepare_expected_pkt( @@ -3686,10 +3888,6 @@ async fn test_multicast_reset_all_tables() -> TestResult { Ok(()) } -/* - * Commented out untl https://github.com/oxidecomputer/dendrite/issues/107 is - * fixed - * #[tokio::test] #[ignore] async fn test_multicast_vlan_translation_not_possible() -> TestResult { @@ -3724,36 +3922,26 @@ async fn test_multicast_vlan_translation_not_possible() -> TestResult { types::InternalForwarding { nat_target: Some(create_nat_target_ipv4()), }, // Create NAT target - types::ExternalForwarding { - vlan_id: output_vlan, - }, + types::ExternalForwarding { vlan_id: output_vlan }, None, ) .await; let src_mac = switch.get_port_mac(ingress).unwrap(); - // Create test packet with input VLAN + // Create test packet with input VLAN 10, which differs from the + // group's configured VLAN 20. let input_vlan = 10; - let mut to_send = create_ipv4_multicast_packet( + let to_send = create_ipv4_multicast_packet( multicast_ip, src_mac, "192.168.1.20", 4444, 5555, + Some(input_vlan), ); - // Add input VLAN tag - to_send.hdrs.eth_hdr.as_mut().unwrap().eth_8021q = Some(eth::EthQHdr { - eth_pcp: 0, - eth_dei: 0, - eth_vlan_tag: input_vlan, - }); - - let test_pkt = TestPacket { - packet: Arc::new(to_send), - port: ingress, - }; + 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 @@ -3766,7 +3954,6 @@ async fn test_multicast_vlan_translation_not_possible() -> TestResult { .unwrap(); cleanup_test_group(switch, internal_multicast_ip, TEST_TAG).await } -*/ #[tokio::test] #[ignore] @@ -3832,6 +4019,7 @@ async fn test_multicast_multiple_packets() -> TestResult { "192.168.1.10", src_port, dst_port, + Some(10), ); let to_recv1 = prepare_expected_pkt( @@ -3899,7 +4087,7 @@ async fn test_multicast_no_group_configured() -> TestResult { // Define test ports let ingress = PhysPort(10); - // Use unique multicast IP addresses that we will NOT configure any group for + // Use unique multicast IP addresses that we will not configure any group for let unconfigured_multicast_ipv4 = IpAddr::V4(Ipv4Addr::new(224, 1, 255, 1)); // Unique IPv4 multicast let unconfigured_multicast_ipv6 = IpAddr::V6(Ipv6Addr::new(0xff0e, 0, 0, 0, 0, 0, 255, 1)); // Unique IPv6 multicast @@ -3918,6 +4106,7 @@ async fn test_multicast_no_group_configured() -> TestResult { "192.168.1.10", 3333, 4444, + None, ); let test_pkt = TestPacket { packet: Arc::new(to_send), port: ingress }; @@ -3948,6 +4137,7 @@ async fn test_multicast_no_group_configured() -> TestResult { "2001:db8::1", 3333, 4444, + None, ); let test_pkt = TestPacket { packet: Arc::new(to_send), port: ingress }; @@ -4499,10 +4689,11 @@ async fn test_multicast_empty_then_add_members_ipv6() -> TestResult { src_ip, src_port, dst_port, + Some(10), ); // Create Geneve packet targeting the internal group - let eth_hdr_len = 14; + let eth_hdr_len = og_pkt.hdrs.eth_hdr.as_ref().unwrap().hdr_len(); let payload = og_pkt.deparse().unwrap()[eth_hdr_len..].to_vec(); let geneve_pkt = common::gen_geneve_packet_with_mcast_tag( Endpoint::parse( @@ -4616,6 +4807,7 @@ async fn test_multicast_empty_then_add_members_ipv6() -> TestResult { "2001:db8::2", // Different source IP to differentiate packets 3334, 4445, + Some(10), ); // Test packet for use in final assertion @@ -4853,10 +5045,11 @@ async fn test_multicast_empty_then_add_members_ipv4() -> TestResult { src_ip, src_port, dst_port, + Some(10), ); // Create Geneve packet targeting the internal group (like test_encapped_multicast_geneve_mcast_tag_to_underlay_members) - let eth_hdr_len = 14; + let eth_hdr_len = og_pkt.hdrs.eth_hdr.as_ref().unwrap().hdr_len(); let payload = og_pkt.deparse().unwrap()[eth_hdr_len..].to_vec(); let geneve_pkt = common::gen_geneve_packet_with_mcast_tag( Endpoint::parse( @@ -4969,6 +5162,7 @@ async fn test_multicast_empty_then_add_members_ipv4() -> TestResult { "10.1.1.2", 1235, 5679, + Some(10), ); // Test packet for use in final assertion @@ -6880,6 +7074,302 @@ async fn test_underlay_delete_recreate_recovery_flow() -> TestResult { Ok(()) } +/// Tests the VLAN lifecycle for multicast groups, verifying route action +/// transitions through None -> Some(10) -> Some(20) -> None. +#[tokio::test] +#[ignore] +async fn test_vlan_lifecycle_route_entries() -> TestResult { + let switch = &*get_switch().await; + let tag = "vlan_lifecycle_test"; + + // Setup: create internal admin-scoped group for NAT target + let internal_ip = IpAddr::V6(MULTICAST_NAT_IP); + let egress_port = PhysPort(28); + create_test_multicast_group( + switch, + internal_ip, + Some(tag), + &[(egress_port, types::Direction::Underlay)], + types::InternalForwarding { nat_target: None }, + types::ExternalForwarding { vlan_id: None }, + None, + ) + .await; + + let group_ip = IpAddr::V4(MULTICAST_TEST_IPV4); + let nat_target = create_nat_target_ipv4(); + let test_ip = "224.0.1.0"; + + // Create without VLAN + let create_no_vlan = types::MulticastGroupCreateExternalEntry { + group_ip, + tag: Some(tag.to_string()), + internal_forwarding: types::InternalForwarding { + nat_target: Some(nat_target.clone()), + }, + external_forwarding: types::ExternalForwarding { vlan_id: None }, + sources: None, + }; + + switch + .client + .multicast_group_create_external(&create_no_vlan) + .await + .expect("Should create group without VLAN"); + + // Update to add VLAN 10 + let update_add_vlan = types::MulticastGroupUpdateExternalEntry { + internal_forwarding: types::InternalForwarding { + nat_target: Some(nat_target.clone()), + }, + external_forwarding: types::ExternalForwarding { vlan_id: Some(10) }, + sources: None, + }; + + let updated = switch + .client + .multicast_group_update_external( + &group_ip, + &make_tag(tag), + &update_add_vlan, + ) + .await + .expect("Should update group to add VLAN"); + assert_eq!(updated.into_inner().external_forwarding.vlan_id, Some(10)); + + let table = switch + .client + .table_dump("pipe.Ingress.l3_router.MulticastRouter4.tbl", false) + .await? + .into_inner(); + assert!( + has_vlan_action_for_ip(&table.entries, test_ip, 10), + "Route entry should have forward_vlan(10) action" + ); + + // Update to change VLAN 10 -> 20, verify no stale entries + let update_change_vlan = types::MulticastGroupUpdateExternalEntry { + internal_forwarding: types::InternalForwarding { + nat_target: Some(nat_target.clone()), + }, + external_forwarding: types::ExternalForwarding { vlan_id: Some(20) }, + sources: None, + }; + + let updated = switch + .client + .multicast_group_update_external( + &group_ip, + &make_tag(tag), + &update_change_vlan, + ) + .await + .expect("Should update group to change VLAN"); + assert_eq!(updated.into_inner().external_forwarding.vlan_id, Some(20)); + + let table = switch + .client + .table_dump("pipe.Ingress.l3_router.MulticastRouter4.tbl", false) + .await? + .into_inner(); + assert!( + has_vlan_action_for_ip(&table.entries, test_ip, 20), + "Route entry should have forward_vlan(20) action" + ); + assert!( + !has_vlan_action_for_ip(&table.entries, test_ip, 10), + "Should not have stale forward_vlan(10) action" + ); + + // Update to remove VLAN + let update_remove_vlan = types::MulticastGroupUpdateExternalEntry { + internal_forwarding: types::InternalForwarding { + nat_target: Some(nat_target.clone()), + }, + external_forwarding: types::ExternalForwarding { vlan_id: None }, + sources: None, + }; + + let updated = switch + .client + .multicast_group_update_external( + &group_ip, + &make_tag(tag), + &update_remove_vlan, + ) + .await + .expect("Should update group to remove VLAN"); + assert_eq!(updated.into_inner().external_forwarding.vlan_id, None); + + let table = switch + .client + .table_dump("pipe.Ingress.l3_router.MulticastRouter4.tbl", false) + .await? + .into_inner(); + assert!( + !has_vlan_action_for_ip(&table.entries, test_ip, 20), + "Should not have forward_vlan action after VLAN removal" + ); + + // Cleanup + switch + .client + .multicast_reset_by_tag(&make_tag(tag)) + .await + .expect("Should cleanup by tag"); + + Ok(()) +} + +/// IPv6 version of the VLAN lifecycle test. Exercises the same +/// None -> Some(10) -> Some(20) -> None transitions on an IPv6 +/// external multicast group, verifying route and NAT table entries. +#[tokio::test] +#[ignore] +async fn test_vlan_lifecycle_route_entries_ipv6() -> TestResult { + let switch = &*get_switch().await; + let tag = "vlan_lifecycle_v6_test"; + + // Setup: create internal admin-scoped group for NAT target + let internal_ip = IpAddr::V6(MULTICAST_NAT_IP); + let egress_port = PhysPort(28); + create_test_multicast_group( + switch, + internal_ip, + Some(tag), + &[(egress_port, types::Direction::Underlay)], + types::InternalForwarding { nat_target: None }, + types::ExternalForwarding { vlan_id: None }, + None, + ) + .await; + + // Use a distinct IPv6 multicast address (global scope, external group) + let group_ip: IpAddr = "ff0e::1:2020".parse().unwrap(); + let nat_target = create_nat_target_ipv6(); + let test_ip = "ff0e::1:2020"; + + // Create without VLAN + let create_no_vlan = types::MulticastGroupCreateExternalEntry { + group_ip, + tag: Some(tag.to_string()), + internal_forwarding: types::InternalForwarding { + nat_target: Some(nat_target.clone()), + }, + external_forwarding: types::ExternalForwarding { vlan_id: None }, + sources: None, + }; + + switch + .client + .multicast_group_create_external(&create_no_vlan) + .await + .expect("Should create IPv6 group without VLAN"); + + // Update to add VLAN 10 + let update_add_vlan = types::MulticastGroupUpdateExternalEntry { + internal_forwarding: types::InternalForwarding { + nat_target: Some(nat_target.clone()), + }, + external_forwarding: types::ExternalForwarding { vlan_id: Some(10) }, + sources: None, + }; + + let updated = switch + .client + .multicast_group_update_external( + &group_ip, + &make_tag(tag), + &update_add_vlan, + ) + .await + .expect("Should update IPv6 group to add VLAN"); + assert_eq!(updated.into_inner().external_forwarding.vlan_id, Some(10)); + + let table = switch + .client + .table_dump("pipe.Ingress.l3_router.MulticastRouter6.tbl", false) + .await? + .into_inner(); + assert!( + has_vlan_action_for_ip(&table.entries, test_ip, 10), + "Route entry should have forward_vlan(10) action" + ); + + // Update to change VLAN 10 -> 20, verify no stale entries + let update_change_vlan = types::MulticastGroupUpdateExternalEntry { + internal_forwarding: types::InternalForwarding { + nat_target: Some(nat_target.clone()), + }, + external_forwarding: types::ExternalForwarding { vlan_id: Some(20) }, + sources: None, + }; + + let updated = switch + .client + .multicast_group_update_external( + &group_ip, + &make_tag(tag), + &update_change_vlan, + ) + .await + .expect("Should update IPv6 group to change VLAN"); + assert_eq!(updated.into_inner().external_forwarding.vlan_id, Some(20)); + + let table = switch + .client + .table_dump("pipe.Ingress.l3_router.MulticastRouter6.tbl", false) + .await? + .into_inner(); + assert!( + has_vlan_action_for_ip(&table.entries, test_ip, 20), + "Route entry should have forward_vlan(20) action" + ); + assert!( + !has_vlan_action_for_ip(&table.entries, test_ip, 10), + "Should not have stale forward_vlan(10) action" + ); + + // Update to remove VLAN + let update_remove_vlan = types::MulticastGroupUpdateExternalEntry { + internal_forwarding: types::InternalForwarding { + nat_target: Some(nat_target.clone()), + }, + external_forwarding: types::ExternalForwarding { vlan_id: None }, + sources: None, + }; + + let updated = switch + .client + .multicast_group_update_external( + &group_ip, + &make_tag(tag), + &update_remove_vlan, + ) + .await + .expect("Should update IPv6 group to remove VLAN"); + assert_eq!(updated.into_inner().external_forwarding.vlan_id, None); + + let table = switch + .client + .table_dump("pipe.Ingress.l3_router.MulticastRouter6.tbl", false) + .await? + .into_inner(); + assert!( + !has_vlan_action_for_ip(&table.entries, test_ip, 20), + "Should not have forward_vlan action after VLAN removal" + ); + + // Cleanup + switch + .client + .multicast_reset_by_tag(&make_tag(tag)) + .await + .expect("Should cleanup by tag"); + + Ok(()) +} + /// Tests that update operations validate tags. /// /// When updating a multicast group, the provided tag must match the existing diff --git a/dpd-client/tests/integration_tests/table_tests.rs b/dpd-client/tests/integration_tests/table_tests.rs index ee3a34e..327bf63 100644 --- a/dpd-client/tests/integration_tests/table_tests.rs +++ b/dpd-client/tests/integration_tests/table_tests.rs @@ -4,6 +4,7 @@ // // Copyright 2026 Oxide Computer Company +#[cfg(feature = "multicast")] use std::net::IpAddr; use std::net::Ipv4Addr; use std::net::Ipv6Addr; @@ -41,8 +42,11 @@ use crate::integration_tests::common::prelude::*; // investigating. If it only changes by an entry or two, it's fine to just // adjust the constant below to match the observed result. // +// TODO: Multicast drops IPv4 LPM capacity to 7164 (from 8187) due to +// ingress TCAM pressure. Investigate moving MulticastRouter4/6 into the +// egress pipeline to reclaim capacity. #[cfg(feature = "multicast")] -const IPV4_LPM_SIZE: usize = 8175; // ipv4 forwarding table +const IPV4_LPM_SIZE: usize = 7164; // ipv4 forwarding table #[cfg(not(feature = "multicast"))] const IPV4_LPM_SIZE: usize = 8187; // ipv4 forwarding table diff --git a/dpd/p4/sidecar.p4 b/dpd/p4/sidecar.p4 index 5e5ecbf..07a694a 100644 --- a/dpd/p4/sidecar.p4 +++ b/dpd/p4/sidecar.p4 @@ -608,8 +608,17 @@ 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; } + 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; counters = mcast_ipv4_ingress_ctr; @@ -625,9 +634,14 @@ control NatIngress ( mcast_ipv6_ingress_ctr.count(); } - // Separate table for IPv6 multicast packets that need to be encapsulated. + // IPv6 counterpart of ingress_ipv4_mcast for IPv6 packets that need to be + // encapsulated. See ingress_ipv4_mcast for details on VLAN matching. table ingress_ipv6_mcast { - key = { hdr.ipv6.dst_addr : exact; } + 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; counters = mcast_ipv6_ingress_ctr; @@ -1330,15 +1344,17 @@ control MulticastRouter4( apply { // 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. + // will be re-attached if set in the forward_vlan action (or + // untagged for groups without VLAN). This prevents unintended + // VLAN translation. if (hdr.vlan.isValid()) { hdr.ethernet.ether_type = hdr.vlan.ether_type; hdr.vlan.setInvalid(); } if (!tbl.apply().hit) { - icmp_error(ICMP6_DST_UNREACH, ICMP6_DST_UNREACH_NOROUTE); - meta.drop_reason = DROP_IPV6_UNROUTEABLE; + icmp_error(ICMP_DEST_UNREACH, ICMP_DST_UNREACH_NET); + meta.drop_reason = DROP_IPV4_UNROUTEABLE; // Dont set meta.dropped because we want an error packet // to go out. } else if (hdr.ipv4.ttl == 1 && !meta.service_routed) { @@ -1470,7 +1486,9 @@ control MulticastRouter6 ( apply { // 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. + // will be re-attached if set in the forward_vlan action (or + // untagged for groups without VLAN). This prevents unintended + // VLAN translation. if (hdr.vlan.isValid()) { hdr.ethernet.ether_type = hdr.vlan.ether_type; hdr.vlan.setInvalid(); @@ -1564,7 +1582,7 @@ control EgressFilter( egress_filter.apply(); } } - + control MacRewrite( inout sidecar_headers_t hdr, in PortId_t port diff --git a/dpd/src/mcast/mod.rs b/dpd/src/mcast/mod.rs index 0535983..da02516 100644 --- a/dpd/src/mcast/mod.rs +++ b/dpd/src/mcast/mod.rs @@ -363,6 +363,7 @@ 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 @@ -668,8 +669,12 @@ pub(crate) fn modify_group_external( // Create rollback context for external group update let group_entry_for_rollback = group_entry.clone(); - let rollback_ctx = - GroupUpdateRollbackContext::new(s, group_ip, &group_entry_for_rollback); + let rollback_ctx = GroupUpdateRollbackContext::new( + s, + group_ip, + &group_entry_for_rollback, + new_group_info.external_forwarding.vlan_id, + ); // Pre-compute normalized sources for rollback purposes let normalized_sources = @@ -715,10 +720,9 @@ pub(crate) fn modify_group_external( updated_group.int_fwding.nat_target = Some(nat_target); let old_vlan_id = updated_group.ext_fwding.vlan_id; - updated_group.ext_fwding.vlan_id = new_group_info - .external_forwarding - .vlan_id - .or(updated_group.ext_fwding.vlan_id); + // VLAN is assigned directly -> `Some(x)` sets VLAN, `None` removes VLAN + updated_group.ext_fwding.vlan_id = + new_group_info.external_forwarding.vlan_id; updated_group.sources = canonicalize_sources( new_group_info.sources.clone().or(updated_group.sources), ); @@ -811,9 +815,11 @@ pub(crate) fn modify_group_internal( s, group_ip.into(), &group_entry_for_rollback, + // Internal groups don't have VLANs, so `attempted_vlan_id` is None. + None, ); - // Internal groups don't update sources - always `None` + // Internal groups don't update sources, so always `None` debug_assert!( group_entry.sources.is_none(), "Internal groups should not have sources" @@ -1347,12 +1353,17 @@ 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)?; + table::mcast::mcast_nat::add_ipv4_entry( + s, ipv4, nat_target, vlan_id, + )?; } IpAddr::V6(ipv6) => { - table::mcast::mcast_nat::add_ipv6_entry(s, ipv6, nat_target)?; + table::mcast::mcast_nat::add_ipv6_entry( + s, ipv6, nat_target, vlan_id, + )?; } } @@ -1682,39 +1693,46 @@ fn update_external_tables( add_source_filters(s, group_ip, sources_as_slice(&new_sources))?; } + let old_vlan_id = group_entry.ext_fwding.vlan_id; + let new_vlan_id = new_group_info.external_forwarding.vlan_id; + // Update NAT target - external groups always have NAT targets - if Some(new_group_info.internal_forwarding.nat_target.ok_or_else(|| { - DpdError::Invalid("external groups must have NAT target".to_string()) - })?) != group_entry.int_fwding.nat_target + // 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 { update_nat_tables( s, group_ip, - Some(new_group_info.internal_forwarding.nat_target.ok_or_else( - || { - DpdError::Invalid( - "external groups must have NAT target".to_string(), - ) - }, - )?), + Some(new_nat_target), group_entry.int_fwding.nat_target, + old_vlan_id, + new_vlan_id, )?; } - // Update VLAN if it changed - if new_group_info.external_forwarding.vlan_id - != group_entry.ext_fwding.vlan_id - { + // Update route table VLAN if it changed + // Route tables use simple dst_addr matching but select forward vs forward_vlan action + if old_vlan_id != new_vlan_id { match group_ip { IpAddr::V4(ipv4) => table::mcast::mcast_route::update_ipv4_entry( s, ipv4, - new_group_info.external_forwarding.vlan_id, + old_vlan_id, + new_vlan_id, ), IpAddr::V6(ipv6) => table::mcast::mcast_route::update_ipv6_entry( s, ipv6, - new_group_info.external_forwarding.vlan_id, + old_vlan_id, + new_vlan_id, ), }?; } @@ -1809,7 +1827,11 @@ fn delete_group_tables( ipv4, sources_as_slice(&group.sources), )?; - table::mcast::mcast_nat::del_ipv4_entry(s, ipv4)?; + table::mcast::mcast_nat::del_ipv4_entry( + s, + ipv4, + group.ext_fwding.vlan_id, + )?; } delete_group_bitmap_entries(s, group)?; @@ -1827,7 +1849,11 @@ fn delete_group_tables( ipv6, sources_as_slice(&group.sources), )?; - table::mcast::mcast_nat::del_ipv6_entry(s, ipv6)?; + table::mcast::mcast_nat::del_ipv6_entry( + s, + ipv6, + group.ext_fwding.vlan_id, + )?; } delete_group_bitmap_entries(s, group)?; @@ -1865,31 +1891,47 @@ 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(nat), Some(_)) => { - // NAT to NAT - update existing entry - table::mcast::mcast_nat::update_ipv4_entry(s, ipv4, nat) + (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::V6(ipv6), Some(nat), Some(_)) => { - // NAT to NAT - update existing entry - table::mcast::mcast_nat::update_ipv6_entry(s, ipv6, 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::V4(ipv4), Some(nat), None) => { // No NAT to NAT - add new entry - table::mcast::mcast_nat::add_ipv4_entry(s, ipv4, nat) + table::mcast::mcast_nat::add_ipv4_entry(s, ipv4, nat, new_vlan_id) } (IpAddr::V6(ipv6), Some(nat), None) => { // No NAT to NAT - add new entry - table::mcast::mcast_nat::add_ipv6_entry(s, ipv6, nat) + table::mcast::mcast_nat::add_ipv6_entry(s, ipv6, nat, new_vlan_id) } (IpAddr::V4(ipv4), None, Some(_)) => { // NAT to no NAT - delete entry - table::mcast::mcast_nat::del_ipv4_entry(s, ipv4) + table::mcast::mcast_nat::del_ipv4_entry(s, ipv4, old_vlan_id) } (IpAddr::V6(ipv6), None, Some(_)) => { // NAT to no NAT - delete entry - table::mcast::mcast_nat::del_ipv6_entry(s, ipv6) + table::mcast::mcast_nat::del_ipv6_entry(s, ipv6, old_vlan_id) } _ => Ok(()), // No change (None → None) } @@ -1936,42 +1978,56 @@ fn update_internal_group_bitmap_tables( /// Only updates the external bitmap entry since that's the only bitmap /// entry created during group configuration. The underlay replication /// is handled separately via the ASIC's multicast group primitives. +/// +/// # Arguments +/// +/// * `s` - Switch instance for table operations. +/// * `group_ip` - IP address of the multicast group. +/// * `external_group_id` - ID of the external multicast group for bitmap updates. +/// * `members` - Group members used to recreate port bitmap. +/// * `current_vlan_id` - VLAN currently in the table (may be the attempted new VLAN). +/// * `target_vlan_id` - VLAN to restore to (the original VLAN). fn update_fwding_tables( s: &Switch, group_ip: IpAddr, external_group_id: MulticastGroupId, members: &[MulticastGroupMember], - vlan_id: Option, + current_vlan_id: Option, + target_vlan_id: Option, ) -> DpdResult<()> { match group_ip { - IpAddr::V4(ipv4) => { - table::mcast::mcast_route::update_ipv4_entry(s, ipv4, vlan_id) - } - IpAddr::V6(ipv6) => { - table::mcast::mcast_route::update_ipv6_entry(s, ipv6, vlan_id) - .and_then(|_| { - // Update external bitmap for external members - // (only external bitmap entries exist; underlay replication - // uses ASIC multicast groups directly). - // Skip if no external members to avoid spurious errors - // when called during rollback on empty groups. - if !members - .iter() - .any(|m| m.direction == Direction::External) - { - return Ok(()); - } + IpAddr::V4(ipv4) => table::mcast::mcast_route::update_ipv4_entry( + s, + ipv4, + current_vlan_id, + target_vlan_id, + ), + IpAddr::V6(ipv6) => table::mcast::mcast_route::update_ipv6_entry( + s, + ipv6, + current_vlan_id, + target_vlan_id, + ) + .and_then(|_| { + // Update external bitmap for external members (only external bitmap + // entries exist, whereas underlay replication uses ASIC multicast + // groups directly). + // + // Skip if no external members to avoid spurious errors when called + // during rollback on empty groups. + if !members.iter().any(|m| m.direction == Direction::External) { + return Ok(()); + } - let external_port_bitmap = - create_port_bitmap(members, Direction::External); - table::mcast::mcast_egress::update_bitmap_entry( - s, - external_group_id, - &external_port_bitmap, - vlan_id, - ) - }) - } + let external_port_bitmap = + create_port_bitmap(members, Direction::External); + table::mcast::mcast_egress::update_bitmap_entry( + s, + external_group_id, + &external_port_bitmap, + target_vlan_id, + ) + }), } } @@ -2321,17 +2377,14 @@ mod tests { None ); - // Vec with only Exact sources is deduplicated and sorted + // Vec with only Exact sources stays as-is let exact_sources = vec![ IpSrc::Exact(IpAddr::V4(Ipv4Addr::new(192, 168, 1, 1))), IpSrc::Exact(IpAddr::V4(Ipv4Addr::new(10, 0, 0, 1))), ]; assert_eq!( - canonicalize_sources(Some(exact_sources)), - Some(vec![ - IpSrc::Exact(IpAddr::V4(Ipv4Addr::new(10, 0, 0, 1))), - IpSrc::Exact(IpAddr::V4(Ipv4Addr::new(192, 168, 1, 1))), - ]) + canonicalize_sources(Some(exact_sources.clone())), + Some(exact_sources) ); // Single Exact source stays as-is diff --git a/dpd/src/mcast/rollback.rs b/dpd/src/mcast/rollback.rs index a6cbbc8..fb73cd7 100644 --- a/dpd/src/mcast/rollback.rs +++ b/dpd/src/mcast/rollback.rs @@ -136,6 +136,7 @@ pub(crate) struct GroupCreateRollbackContext<'a> { underlay_id: MulticastGroupId, nat_target: Option, sources: Option<&'a [IpSrc]>, + vlan_id: Option, } impl RollbackOps for GroupCreateRollbackContext<'_> { @@ -261,6 +262,7 @@ impl<'a> GroupCreateRollbackContext<'a> { underlay_id: MulticastGroupId, nat_target: NatTarget, sources: Option<&'a [IpSrc]>, + vlan_id: Option, ) -> Self { Self { switch, @@ -269,6 +271,7 @@ impl<'a> GroupCreateRollbackContext<'a> { underlay_id, nat_target: Some(nat_target), sources, + vlan_id, } } @@ -286,6 +289,7 @@ impl<'a> GroupCreateRollbackContext<'a> { underlay_id, nat_target: None, sources: None, + vlan_id: None, } } @@ -425,6 +429,7 @@ impl<'a> GroupCreateRollbackContext<'a> { table::mcast::mcast_nat::del_ipv4_entry( self.switch, ipv4, + self.vlan_id, ), ); } @@ -515,6 +520,7 @@ impl<'a> GroupCreateRollbackContext<'a> { table::mcast::mcast_nat::del_ipv6_entry( self.switch, ipv6, + self.vlan_id, ), ); } @@ -537,6 +543,10 @@ pub(crate) struct GroupUpdateRollbackContext<'a> { switch: &'a Switch, group_ip: IpAddr, original_group: &'a MulticastGroup, + /// The VLAN that the update is attempting to set. This may differ from + /// `original_group.ext_fwding.vlan_id` when a VLAN change is in progress. + /// Used during rollback to delete entries that may have been added. + attempted_vlan_id: Option, } impl RollbackOps for GroupUpdateRollbackContext<'_> { @@ -567,7 +577,7 @@ impl RollbackOps for GroupUpdateRollbackContext<'_> { return Ok(()); } - // Internal group - perform actual port rollback + // Internal group -> perform actual port rollback debug!( self.switch.log, "rolling back multicast group update"; @@ -715,12 +725,21 @@ impl RollbackOps for GroupUpdateRollbackContext<'_> { impl<'a> GroupUpdateRollbackContext<'a> { /// Create rollback context for group update operations. + /// + /// # Arguments + /// + /// * `switch` - Switch instance for table operations. + /// * `group_ip` - IP address of the multicast group being updated. + /// * `original_group` - Reference to the group's state before the update. + /// * `attempted_vlan_id` - The VLAN that the update is attempting to set. + /// Used during rollback to delete entries that may have been added. pub(crate) fn new( switch: &'a Switch, group_ip: IpAddr, original_group: &'a MulticastGroup, + attempted_vlan_id: Option, ) -> Self { - Self { switch, group_ip, original_group } + Self { switch, group_ip, original_group, attempted_vlan_id } } /// Restore tables and return error. @@ -759,43 +778,39 @@ impl<'a> GroupUpdateRollbackContext<'a> { ); } - // Restore NAT settings - match (self.group_ip, nat_target) { - (IpAddr::V4(ipv4), Some(nat)) => { - self.log_rollback_error( - "restore IPv4 NAT settings", - &format!("for group {}", self.group_ip), - table::mcast::mcast_nat::update_ipv4_entry( - self.switch, - ipv4, - nat, - ), - ); - } - (IpAddr::V6(ipv6), Some(nat)) => { - self.log_rollback_error( - "restore IPv6 NAT settings", - &format!("for group {}", self.group_ip), - table::mcast::mcast_nat::update_ipv6_entry( - self.switch, - ipv6, - nat, - ), - ); - } - (IpAddr::V4(ipv4), None) => { - self.log_rollback_error( - "remove IPv4 NAT settings", - &format!("for group {}", self.group_ip), - table::mcast::mcast_nat::del_ipv4_entry(self.switch, ipv4), - ); - } - (IpAddr::V6(ipv6), None) => { - self.log_rollback_error( - "remove IPv6 NAT settings", - &format!("for group {}", self.group_ip), - table::mcast::mcast_nat::del_ipv6_entry(self.switch, ipv6), - ); + // 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. + if let Some(nat) = nat_target { + match self.group_ip { + IpAddr::V4(ipv4) => { + self.log_rollback_error( + "restore IPv4 NAT settings", + &format!("for group {}", self.group_ip), + table::mcast::mcast_nat::update_ipv4_entry( + self.switch, + ipv4, + nat, + nat, + self.attempted_vlan_id, + vlan_id, + ), + ); + } + IpAddr::V6(ipv6) => { + self.log_rollback_error( + "restore IPv6 NAT settings", + &format!("for group {}", self.group_ip), + table::mcast::mcast_nat::update_ipv6_entry( + self.switch, + ipv6, + nat, + nat, + self.attempted_vlan_id, + vlan_id, + ), + ); + } } } @@ -807,6 +822,7 @@ impl<'a> GroupUpdateRollbackContext<'a> { self.group_ip, external_group_id, &prev_members, + self.attempted_vlan_id, vlan_id, ), ); diff --git a/dpd/src/mcast/validate.rs b/dpd/src/mcast/validate.rs index c1ebd93..9fab2d1 100644 --- a/dpd/src/mcast/validate.rs +++ b/dpd/src/mcast/validate.rs @@ -52,8 +52,8 @@ pub(crate) fn validate_multicast_address( pub(crate) fn validate_nat_target(nat_target: NatTarget) -> DpdResult<()> { if !nat_target.inner_mac.is_multicast() { return Err(DpdError::Invalid(format!( - "NAT target inner MAC address {} is not a multicast MAC address", - nat_target.inner_mac + "NAT target inner MAC address {inner_mac} is not a multicast MAC address", + inner_mac = nat_target.inner_mac ))); } diff --git a/dpd/src/table/mcast/mcast_egress.rs b/dpd/src/table/mcast/mcast_egress.rs index b156905..b9d1355 100644 --- a/dpd/src/table/mcast/mcast_egress.rs +++ b/dpd/src/table/mcast/mcast_egress.rs @@ -112,9 +112,7 @@ pub(crate) fn add_bitmap_entry( }; debug!( s.log, - "add multicast egress entry for decap {} -> {:?}", - match_key, - action_data + "add multicast egress entry for decap {match_key} -> {action_data:?}" ); s.table_entry_add( @@ -143,9 +141,7 @@ pub(crate) fn update_bitmap_entry( debug!( s.log, - "update multicast egress entry for decap {} -> {:?}", - match_key, - action_data + "update multicast egress entry for decap {match_key} -> {action_data:?}" ); s.table_entry_update( @@ -165,9 +161,7 @@ pub(crate) fn del_bitmap_entry( debug!( s.log, - "delete multicast egress entry for decap {} -> {}", - match_key, - mcast_external_grp + "delete multicast egress entry for decap {match_key} -> {mcast_external_grp}" ); s.table_entry_del(TableType::McastEgressDecapPorts, &match_key) @@ -212,7 +206,7 @@ pub(crate) fn add_port_mapping_entry( let action_data = PortIdAction::SetPortNumber { port_number: port.as_u8() }; - debug!(s.log, "add port id entry {} -> {:?}", match_key, action_data); + debug!(s.log, "add port id entry {match_key} -> {action_data:?}"); s.table_entry_add( TableType::McastEgressPortMapping, @@ -234,7 +228,7 @@ pub(crate) fn update_port_mapping_entry( let action_data = PortIdAction::SetPortNumber { port_number: port.as_u8() }; - debug!(s.log, "update port id entry {} -> {:?}", match_key, action_data); + debug!(s.log, "update port id entry {match_key} -> {action_data:?}"); s.table_entry_update( TableType::McastEgressPortMapping, @@ -251,7 +245,7 @@ pub(crate) fn del_port_mapping_entry( ) -> DpdResult<()> { let match_key = MatchKeyPortId::new(asic_port_id); - debug!(s.log, "delete port id entry {} -> {}", match_key, asic_port_id); + debug!(s.log, "delete port id entry {match_key} -> {asic_port_id}"); s.table_entry_del(TableType::McastEgressPortMapping, &match_key) } diff --git a/dpd/src/table/mcast/mcast_nat.rs b/dpd/src/table/mcast/mcast_nat.rs index 94ccfaa..0ccc999 100644 --- a/dpd/src/table/mcast/mcast_nat.rs +++ b/dpd/src/table/mcast/mcast_nat.rs @@ -5,13 +5,17 @@ // Copyright 2026 Oxide Computer Company //! Table operations for multicast NAT entries. +//! +//! 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. use std::net::{Ipv4Addr, Ipv6Addr}; use crate::{Switch, table::*}; -use super::{Ipv4MatchKey, Ipv6MatchKey}; - +use super::{Ipv4VlanMatchKey, Ipv6VlanMatchKey}; use aal::ActionParse; use aal_macros::*; use common::network::{MacAddr, NatTarget}; @@ -36,64 +40,118 @@ enum Ipv6Action { Forward { target: Ipv6Addr, inner_mac: MacAddr, vni: u32 }, } -/// Add a NAT entry for IPv4 multicast traffic, keyed on `ip`. +/// 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<()> { - let match_key = Ipv4MatchKey::new(ip); + if let Some(vlan_id) = vlan_id { + common::network::validate_vlan(vlan_id)?; + } + + let match_key = Ipv4VlanMatchKey::new(ip, vlan_id); let action_key = Ipv4Action::Forward { target: tgt.internal_ip, inner_mac: tgt.inner_mac, vni: tgt.vni.as_u32(), }; - debug!(s.log, "add ingress mcast entry {} -> {:?}", match_key, action_key); - + debug!(s.log, "add ingress mcast entry {match_key} -> {action_key:?}"); s.table_entry_add(TableType::NatIngressIpv4Mcast, &match_key, &action_key) } /// 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, - tgt: NatTarget, + new_tgt: NatTarget, + old_tgt: NatTarget, + old_vlan_id: Option, + new_vlan_id: Option, ) -> DpdResult<()> { - let match_key = Ipv4MatchKey::new(ip); - let action_key = Ipv4Action::Forward { - target: tgt.internal_ip, - inner_mac: tgt.inner_mac, - vni: tgt.vni.as_u32(), - }; - - debug!( - s.log, - "update ingress mcast entry {} -> {:?}", match_key, action_key - ); - - s.table_entry_update( - TableType::NatIngressIpv4Mcast, - &match_key, - &action_key, - ) + 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, + ); + } + + 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(()) } -/// Delete a NAT entry for IPv4 multicast traffic, keyed on `ip`. -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); - +/// 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); + 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, ) @@ -104,7 +162,10 @@ 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. @@ -112,64 +173,118 @@ pub(crate) fn reset_ipv4(s: &Switch) -> DpdResult<()> { s.table_clear(TableType::NatIngressIpv4Mcast) } -/// Add a NAT entry for IPv6 multicast traffic, keyed on `ip`. +/// 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<()> { - let match_key = Ipv6MatchKey::new(ip); + if let Some(vlan_id) = vlan_id { + common::network::validate_vlan(vlan_id)?; + } + + let match_key = Ipv6VlanMatchKey::new(ip, vlan_id); let action_key = Ipv6Action::Forward { target: tgt.internal_ip, inner_mac: tgt.inner_mac, vni: tgt.vni.as_u32(), }; - debug!(s.log, "add ingress mcast entry {} -> {:?}", match_key, action_key); - + debug!(s.log, "add ingress mcast entry {match_key} -> {action_key:?}"); s.table_entry_add(TableType::NatIngressIpv6Mcast, &match_key, &action_key) } /// 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, - tgt: NatTarget, + new_tgt: NatTarget, + old_tgt: NatTarget, + old_vlan_id: Option, + new_vlan_id: Option, ) -> DpdResult<()> { - let match_key = Ipv6MatchKey::new(ip); - let action_key = Ipv6Action::Forward { - target: tgt.internal_ip, - inner_mac: tgt.inner_mac, - vni: tgt.vni.as_u32(), - }; - - debug!( - s.log, - "update ingress mcast entry {} -> {:?}", match_key, action_key - ); - - s.table_entry_update( - TableType::NatIngressIpv6Mcast, - &match_key, - &action_key, - ) + 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, + ); + } + + 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(()) } -/// Delete a NAT entry for IPv6 multicast traffic, keyed on `ip`. -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); - +/// 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); + 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, ) @@ -180,7 +295,10 @@ 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_replication.rs b/dpd/src/table/mcast/mcast_replication.rs index 2a94c2f..44c97f8 100644 --- a/dpd/src/table/mcast/mcast_replication.rs +++ b/dpd/src/table/mcast/mcast_replication.rs @@ -69,7 +69,7 @@ pub(crate) fn add_ipv6_entry( level2_excl_id, }; - debug!(s.log, "add mcast_ipv6 entry {} -> {:?}", dst_addr, action_data); + debug!(s.log, "add mcast_ipv6 entry {dst_addr} -> {action_data:?}"); s.table_entry_add(TableType::McastIpv6, &match_key, &action_data) } @@ -104,7 +104,7 @@ pub(crate) fn update_ipv6_entry( level2_excl_id, }; - debug!(s.log, "update mcast_ipv6 entry {} -> {:?}", dst_addr, action_data); + debug!(s.log, "update mcast_ipv6 entry {dst_addr} -> {action_data:?}"); s.table_entry_update(TableType::McastIpv6, &match_key, &action_data) } @@ -114,7 +114,7 @@ pub(crate) fn update_ipv6_entry( pub(crate) fn del_ipv6_entry(s: &Switch, dst_addr: Ipv6Addr) -> DpdResult<()> { let match_key = Ipv6MatchKey::new(dst_addr); - debug!(s.log, "delete mcast_ipv6 entry {}", match_key); + debug!(s.log, "delete mcast_ipv6 entry {match_key}"); s.table_entry_del(TableType::McastIpv6, &match_key) } diff --git a/dpd/src/table/mcast/mcast_route.rs b/dpd/src/table/mcast/mcast_route.rs index ad2b993..5a8447e 100644 --- a/dpd/src/table/mcast/mcast_route.rs +++ b/dpd/src/table/mcast/mcast_route.rs @@ -5,18 +5,24 @@ // Copyright 2026 Oxide Computer Company //! Table operations for multicast routing entries (on Ingress to the switch). +//! +//! Route tables match only on destination address and select the egress action: +//! - `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. use std::net::{Ipv4Addr, Ipv6Addr}; -use crate::{Switch, table::*}; - -use super::{Ipv4MatchKey, Ipv6MatchKey}; - use aal::ActionParse; use aal_macros::*; use omicron_common::address::UNDERLAY_MULTICAST_SUBNET; use slog::debug; +use super::{Ipv4MatchKey, Ipv6MatchKey}; +use crate::{Switch, table::*}; + /// IPv4 Table for multicast routing entries. pub(crate) const IPV4_TABLE_NAME: &str = "pipe.Ingress.l3_router.MulticastRouter4.tbl"; @@ -40,16 +46,18 @@ enum Ipv6Action { ForwardVLAN { vlan_id: u16 }, } -/// Add an IPv4 multicast route entry to the routing table, keyed on -/// `route`, with an optional `vlan_id`. +/// Add an IPv4 multicast route entry. +/// +/// The action is selected based on VLAN configuration: +/// - No VLAN: `forward` (no VLAN modification on egress) +/// - With VLAN: `forward_vlan(vlan_id)` (add VLAN tag on egress) pub(crate) fn add_ipv4_entry( s: &Switch, route: Ipv4Addr, vlan_id: Option, ) -> DpdResult<()> { let match_key = Ipv4MatchKey::new(route); - - let action_data = match vlan_id { + let action = match vlan_id { None => Ipv4Action::Forward, Some(vlan_id) => { common::network::validate_vlan(vlan_id)?; @@ -57,19 +65,26 @@ pub(crate) fn add_ipv4_entry( } }; - debug!(s.log, "add multicast route entry {} -> {:?}", route, action_data); - - s.table_entry_add(TableType::RouteIpv4Mcast, &match_key, &action_data) + debug!(s.log, "add multicast route entry {match_key} -> {action:?}"); + s.table_entry_add(TableType::RouteIpv4Mcast, &match_key, &action) } -/// Update an IPv4 multicast route entry in the routing table. +/// Update an IPv4 multicast route entry. +/// +/// Updates the action when VLAN configuration changes. Since the match key +/// is just the destination address, this can be done in place. pub(crate) fn update_ipv4_entry( s: &Switch, route: Ipv4Addr, - vlan_id: Option, + old_vlan_id: Option, + new_vlan_id: Option, ) -> DpdResult<()> { + if old_vlan_id == new_vlan_id { + return Ok(()); + } + let match_key = Ipv4MatchKey::new(route); - let action_data = match vlan_id { + let action = match new_vlan_id { None => Ipv4Action::Forward, Some(vlan_id) => { common::network::validate_vlan(vlan_id)?; @@ -77,21 +92,14 @@ pub(crate) fn update_ipv4_entry( } }; - debug!( - s.log, - "update multicast route entry {} -> {:?}", route, action_data - ); - - s.table_entry_update(TableType::RouteIpv4Mcast, &match_key, &action_data) + debug!(s.log, "update multicast route entry {match_key} -> {action:?}"); + s.table_entry_update(TableType::RouteIpv4Mcast, &match_key, &action) } -/// Delete an IPv4 multicast route entry from table, keyed on -/// `route`. +/// Delete an IPv4 multicast route entry. pub(crate) fn del_ipv4_entry(s: &Switch, route: Ipv4Addr) -> DpdResult<()> { let match_key = Ipv4MatchKey::new(route); - - debug!(s.log, "delete multicast route entry {}", match_key); - + debug!(s.log, "delete multicast route entry {match_key}"); s.table_entry_del(TableType::RouteIpv4Mcast, &match_key) } @@ -119,8 +127,15 @@ pub(crate) fn reset_ipv4(s: &Switch) -> DpdResult<()> { s.table_clear(TableType::RouteIpv4Mcast) } -/// Add an IPv6 multicast route entry to the routing table, keyed on -/// `route`, with an optional `vlan_id`. +/// Add an IPv6 multicast route entry. +/// +/// The action is selected based on VLAN configuration: +/// - No VLAN: `forward` (no VLAN modification on egress) +/// - With VLAN: `forward_vlan(vlan_id)` (add VLAN tag on egress) +/// +/// Reserved underlay multicast subnet (ff04::/64) is internal to the rack +/// and always uses Forward without VLAN tagging, regardless of the vlan_id +/// parameter. pub(crate) fn add_ipv6_entry( s: &Switch, route: Ipv6Addr, @@ -129,10 +144,8 @@ pub(crate) fn add_ipv6_entry( let match_key = Ipv6MatchKey::new(route); // Reserved underlay multicast subnet (ff04::/64) is internal to the rack - // and doesn't require VLAN tagging. Other admin-local addresses - // (e.g., ff04:0:0:1::/64) may be used by customer external groups and - // can receive VLAN tagging. - let action_data: Ipv6Action = if UNDERLAY_MULTICAST_SUBNET.contains(route) { + // and doesn't require VLAN tagging. + let action: Ipv6Action = if UNDERLAY_MULTICAST_SUBNET.contains(route) { Ipv6Action::Forward } else { match vlan_id { @@ -144,27 +157,32 @@ pub(crate) fn add_ipv6_entry( } }; - debug!(s.log, "add multicast route entry {} -> {:?}", route, action_data); - - s.table_entry_add(TableType::RouteIpv6Mcast, &match_key, &action_data) + debug!(s.log, "add multicast route entry {match_key} -> {action:?}"); + s.table_entry_add(TableType::RouteIpv6Mcast, &match_key, &action) } -/// Update an IPv6 multicast route entry in the routing table. +/// Update an IPv6 multicast route entry. +/// +/// Updates the action when VLAN configuration changes. Since the match key +/// is just the destination address, this can be done in place. pub(crate) fn update_ipv6_entry( s: &Switch, route: Ipv6Addr, - vlan_id: Option, + old_vlan_id: Option, + new_vlan_id: Option, ) -> DpdResult<()> { + if old_vlan_id == new_vlan_id { + return Ok(()); + } + let match_key = Ipv6MatchKey::new(route); // Reserved underlay multicast subnet (ff04::/64) is internal to the rack - // and doesn't require VLAN tagging. Other admin-local addresses - // (e.g., ff04:0:0:1::/64) may be used by customer external groups and - // can receive VLAN tagging. - let action_data: Ipv6Action = if UNDERLAY_MULTICAST_SUBNET.contains(route) { + // and doesn't require VLAN tagging. + let action: Ipv6Action = if UNDERLAY_MULTICAST_SUBNET.contains(route) { Ipv6Action::Forward } else { - match vlan_id { + match new_vlan_id { None => Ipv6Action::Forward, Some(vlan_id) => { common::network::validate_vlan(vlan_id)?; @@ -173,21 +191,14 @@ pub(crate) fn update_ipv6_entry( } }; - debug!( - s.log, - "update multicast route entry {} -> {:?}", route, action_data - ); - - s.table_entry_update(TableType::RouteIpv6Mcast, &match_key, &action_data) + debug!(s.log, "update multicast route entry {match_key} -> {action:?}"); + s.table_entry_update(TableType::RouteIpv6Mcast, &match_key, &action) } -/// Delete an IPv6 multicast entry from routing table, keyed on -/// `route`. +/// Delete an IPv6 multicast route entry. pub(crate) fn del_ipv6_entry(s: &Switch, route: Ipv6Addr) -> DpdResult<()> { let match_key = Ipv6MatchKey::new(route); - - debug!(s.log, "delete multicast route entry {}", match_key); - + debug!(s.log, "delete multicast route entry {match_key}"); s.table_entry_del(TableType::RouteIpv6Mcast, &match_key) } diff --git a/dpd/src/table/mcast/mcast_src_filter.rs b/dpd/src/table/mcast/mcast_src_filter.rs index e79ac53..13f3bd5 100644 --- a/dpd/src/table/mcast/mcast_src_filter.rs +++ b/dpd/src/table/mcast/mcast_src_filter.rs @@ -85,7 +85,7 @@ pub(crate) fn add_ipv4_entry( let match_key = Ipv4MatchKey::new(src_addr, dst_addr); let action_data = Ipv4Action::AllowSrc; - debug!(s.log, "add source filter entry {} -> {:?}", src_addr, action_data); + debug!(s.log, "add source filter entry {src_addr} -> {action_data:?}"); s.table_entry_add(TableType::McastIpv4SrcFilter, &match_key, &action_data) } @@ -99,7 +99,7 @@ pub(crate) fn del_ipv4_entry( ) -> DpdResult<()> { let match_key = Ipv4MatchKey::new(src_addr, dst_addr); - debug!(s.log, "delete source filter entry {} -> {}", src_addr, dst_addr); + debug!(s.log, "delete source filter entry {src_addr} -> {dst_addr}"); s.table_entry_del(TableType::McastIpv4SrcFilter, &match_key) } @@ -138,7 +138,7 @@ pub(crate) fn add_ipv6_entry( let match_key = Ipv6MatchKey::new(src_addr, dst_addr); let action_data = Ipv6Action::AllowSrc; - debug!(s.log, "add source filter entry {} -> {:?}", src_addr, action_data); + debug!(s.log, "add source filter entry {src_addr} -> {action_data:?}"); s.table_entry_add(TableType::McastIpv6SrcFilter, &match_key, &action_data) } @@ -152,7 +152,7 @@ pub(crate) fn del_ipv6_entry( ) -> DpdResult<()> { let match_key = Ipv6MatchKey::new(src_addr, dst_addr); - debug!(s.log, "delete source filter entry {} -> {}", src_addr, dst_addr); + debug!(s.log, "delete source filter entry {src_addr} -> {dst_addr}"); s.table_entry_del(TableType::McastIpv6SrcFilter, &match_key) } diff --git a/dpd/src/table/mcast/mod.rs b/dpd/src/table/mcast/mod.rs index 600f7c7..0280262 100644 --- a/dpd/src/table/mcast/mod.rs +++ b/dpd/src/table/mcast/mod.rs @@ -55,3 +55,69 @@ 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) + } + } +} diff --git a/packet/src/eth.rs b/packet/src/eth.rs index 4e29834..0cff359 100644 --- a/packet/src/eth.rs +++ b/packet/src/eth.rs @@ -50,6 +50,17 @@ pub struct EthHdr { pub eth_size: u16, } +impl EthHdr { + /// Return the wire size of this L2 header, including any 802.1Q tag. + pub fn hdr_len(&self) -> usize { + if self.eth_8021q.is_some() { + ETH_HDR_SZ + ETH_8021Q_SZ + } else { + ETH_HDR_SZ + } + } +} + pub const ETHER_IPV4: u16 = 0x800; pub const ETHER_ARP: u16 = 0x806; pub const ETHER_VLAN: u16 = 0x8100;