Adding RT-3.52: Multidimensional test for Static GUE Encap/Decap based on BGP path selection and selective DSCP marking#4519
Conversation
Pull Request Test Coverage Report for Build 17237940094Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
| next_hop_group_config_unsupported: true | ||
| policy_forwarding_unsupported: true | ||
| interface_policy_forwarding_unsupported: true | ||
| ipv4_static_route_with_ipv6_nh_unsupported: true |
There was a problem hiding this comment.
Why is this deviation needed? Isnt the test requiring Static routes for IPv6 destination pointing at a IPv4 NH?
|
Need more clarification on the issue: https://partnerissuetracker.corp.google.com/issues/441232997 |
|
Device config provided in an internal bug |
…d on BGP path selection and selective DSCP marking
ASHNA-AGGARWAL-KEYSIGHT
force-pushed
the
fpt_otg_RT_3_52
branch
from
f34f106 to
2a4b999
Compare
| // IPv6Layer: innerGUEIPLayerIPv6, | ||
| } | ||
|
|
||
| decapValidation = &packetvalidationhelpers.PacketValidation{ |
There was a problem hiding this comment.
Thanks for the changes. This is very close to the expectations. However, one last question. Following is bullet#7 of the readme. How do you ensure that post decap, the inner ttl is reduced by 1 before forwarding the packet further?
7. Confirm that when the DUT handles GUEv1 traffic from the reverse path, it successfully performs decapsulation. During this decapsulation process, the system must not transfer the DSCP and TTL bits from the outer header to the inner header. Instead, following decapsulation, the DUT should decrement the inner header's TTL by 1 before forwarding the packet
There was a problem hiding this comment.
In innerGUEIPLayerIPv4, set the TTL to 63, which will be passed to decapValidation params.
innerGUEIPLayerIPv4 = &packetvalidationhelpers.IPv4Layer{
TTL: 63,
Protocol: udpEncapPort,
}
There was a problem hiding this comment.
Looks like we are expecting the ATE to set a TTL of 64 before forwarding the packet to the DUT? Rather, can we explicitly set ttl=64 on the OTG flow definition for Flow-Set#3 and Flow-Set#4? This imo will make the test more robust, no?
There was a problem hiding this comment.
Done
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a comprehensive test for Static GUE encapsulation and decapsulation, including BGP path selection and DSCP marking. The changes are extensive, introducing a new test file and modifying several helper packages to support the test logic, particularly for Arista device configurations via CLI. While the new test is thorough, I've identified several critical and high-severity issues related to test correctness, resource management, and potential race conditions. Key issues include a critical bug in IPv6 address generation, misuse of global variables leading to flaky tests, and a resource leak from unclosed files. I've also provided suggestions to improve maintainability by reducing code duplication and replacing fixed time.Sleep calls with more reliable polling mechanisms.
internal/iputil/iputil.go
Outdated
| func GenerateIPv6(startIP string, count uint64) []string { | ||
|
|
||
| // Maximum MAC value = 2^48 - 1 | ||
| maxMac := new(big.Int).Sub(new(big.Int).Lsh(big.NewInt(1), 48), big.NewInt(1)) | ||
|
|
||
| // Check final value does not overflow: base + step*(count-1) <= maxMac | ||
| mul := new(big.Int).Mul(stepInt, big.NewInt(int64(count-1))) | ||
| final := new(big.Int).Add(baseInt, mul) | ||
| if final.Cmp(maxMac) > 0 { | ||
| return []string{} // overflow → return empty | ||
| } | ||
|
|
||
| // Generate sequence | ||
| out := make([]string, 0, count) | ||
| for i := 0; i < count; i++ { | ||
| cur := new(big.Int).Add(baseInt, new(big.Int).Mul(stepInt, big.NewInt(int64(i)))) | ||
| buf := cur.FillBytes(make([]byte, 6)) // 6 bytes for MAC | ||
| hw := net.HardwareAddr(buf) | ||
| out = append(out, hw.String()) // canonical lower-case hex with colons | ||
| _, netCIDR, _ := net.ParseCIDR(startIP) | ||
| netMask := binary.BigEndian.Uint64(netCIDR.Mask) | ||
| maskSize, _ := netCIDR.Mask.Size() | ||
| firstIP := binary.BigEndian.Uint64(netCIDR.IP) | ||
| lastIP := (firstIP & netMask) | (netMask ^ 0xffffffff) | ||
| entries := []string{} | ||
|
|
||
| for i := firstIP; i <= lastIP; i++ { | ||
| ipv6 := make(net.IP, 16) | ||
| binary.BigEndian.PutUint64(ipv6, i) | ||
| // make last byte non-zero | ||
| p, _ := netip.ParsePrefix(fmt.Sprintf("%v/%d", ipv6, maskSize)) | ||
| entries = append(entries, p.Addr().Next().String()) | ||
| if uint64(len(entries)) >= count { | ||
| break | ||
| } | ||
| } | ||
|
|
||
| return out | ||
| return entries | ||
| } |
There was a problem hiding this comment.
The GenerateIPv6 function contains incorrect logic for generating IPv6 addresses. It appears to be using logic designed for 32-bit IPv4 addresses to manipulate 128-bit IPv6 addresses.
Specifically:
- It uses
binary.BigEndian.Uint64to convert a 16-bytenet.IPto auint64, which truncates the address to its first 64 bits. - The calculation for
lastIPuses0xffffffff, a 32-bit mask, which is incorrect for IPv6. - The loop iterates over a
uint64and usesbinary.BigEndian.PutUint64to write back to a 16-bytenet.IP, which only modifies the first 64 bits.
This will produce incorrect IPv6 addresses and will not work as expected for prefixes longer than /64 or for prefixes that cross a 64-bit boundary. The previous implementation using math/big was correct for handling 128-bit arithmetic. Please revert to a correct implementation for IPv6 address generation, for example by using math/big to handle 128-bit integers.
| PpsRate: trafficPps, | ||
| FlowName: "flowSet5-v4-1", | ||
| EthFlow: &otgconfighelpers.EthFlowParams{SrcMAC: otgBGPConfig[1].otgPortData[1].MAC, DstMAC: port2DstMac}, | ||
| VLANFlow: &otgconfighelpers.VLANFlowParams{VLANId: otgBGPConfig[1].otgPortData[0].Subinterface}, |
There was a problem hiding this comment.
In the traffic flow definitions for flowSet5, there seems to be an inconsistency. For example, in flowSet5-v4-1 (and others in this set), the SrcMAC is set to otgBGPConfig[1].otgPortData[1].MAC, which corresponds to the interface on VLAN 200 (R200). However, the VLANId is set to otgBGPConfig[1].otgPortData[0].Subinterface, which is VLAN 100. This means traffic is sourced with a MAC from the VLAN 200 interface but tagged with VLAN 100. If this is not intentional, it's likely a copy-paste error and could lead to incorrect test behavior. Please verify the intended configuration. The VLAN ID should probably correspond to the subinterface of the source MAC.
| outerGUEIPLayerIPv4.Tos = uint8(dscpValue["BE1"]) | ||
| innerGUEIPLayerIPv4.Tos = uint8(dscpValue["BE1"]) |
There was a problem hiding this comment.
The test functions like testBE1TrafficMigration, testAF1TrafficMigration, etc., modify global variables such as outerGUEIPLayerIPv4 and innerGUEIPLayerIPv4. While the test cases are currently run sequentially, this is a dangerous practice that can lead to race conditions and flaky tests if they are ever run in parallel. It also makes the test logic harder to follow and dependent on execution order. To make the tests more robust and maintainable, please create a local copy of the validation structs within each test function and modify the local copy instead.
| return fmt.Errorf("could not write bytes to pcap file: %v", err) | ||
| } | ||
| defer os.Remove(f.Name()) // Clean up the temporary file | ||
| // defer os.Remove(f.Name()) // Clean up the temporary file |
There was a problem hiding this comment.
The line defer os.Remove(f.Name()) is commented out. This will cause temporary pcap files to be left on the system after the test runs, leading to a resource leak. Please uncomment this line to ensure temporary files are cleaned up.
| // defer os.Remove(f.Name()) // Clean up the temporary file | |
| defer os.Remove(f.Name()) // Clean up the temporary file |
| // packetvalidationhelpers.ValidateIPv6Header, | ||
| } | ||
|
|
||
| encapValidationv6 = &packetvalidationhelpers.PacketValidation{ | ||
| PortName: "port2", | ||
| Validations: validationsV6, | ||
| IPv4Layer: outerGUEv6Encap, | ||
| UDPLayer: udpLayer, | ||
| // IPv6Layer: innerGUEIPLayerIPv6, | ||
| } |
There was a problem hiding this comment.
The validation for IPv6 GUE encapsulation (encapValidationv6) appears to be incomplete. The ValidateIPv6Header for the inner packet is commented out, and the IPv6Layer for the inner packet is also commented out in the PacketValidation struct. This means the test is not actually validating the contents of the encapsulated IPv6 packet. This is a significant gap in test coverage. Please enable and implement this validation to ensure the encapsulated IPv6 traffic is correct.
| t.Logf("Validate prefixes for %s. Expecting prefix received %v", neighborIP, PfxRcd) | ||
| bgpPath := gnmi.OC().NetworkInstance(deviations.DefaultNetworkInstance(dut)).Protocol(oc.PolicyTypes_INSTALL_PROTOCOL_TYPE_BGP, "BGP").Bgp() | ||
| if isV4 { | ||
| time.Sleep(10 * time.Second) |
There was a problem hiding this comment.
The test contains multiple uses of time.Sleep() with fixed durations to wait for network events like route propagation. This can lead to flaky tests if the event takes longer than the sleep duration, or slow tests if the event completes much faster. Please replace these fixed sleeps with polling mechanisms like gnmi.Watch to wait for the specific condition to be met. This will make the tests more reliable and efficient. For example, in validatePrefixes, instead of sleeping, you could use gnmi.Watch to wait for the prefix count to reach the expected value.
| func testBE1TrafficMigration(t *testing.T, dut *ondatra.DUTDevice, ate *ondatra.ATEDevice, otgConfig gosnappi.Config) { | ||
| advertiseRoutesWithiBGP([]string{ate2InternalPrefixesV4List[0]}, ate2ppnh1.IPv6, true, fmt.Sprintf("%s-CBGP-1", atePort2RoutesV4)) | ||
| advertiseRoutesWithiBGP([]string{ate2InternalPrefixesV6List[0]}, ate2ppnh1.IPv6, false, fmt.Sprintf("%s-CBGP-1", atePort2RoutesV6)) | ||
|
|
||
| flowsets := []string{"flowSet1", "flowSet5"} | ||
|
|
||
| flowNames := map[string][]string{ | ||
| "v4": {}, | ||
| "v6": {}, | ||
| } | ||
|
|
||
| otgConfig.Flows().Clear() | ||
|
|
||
| for _, flow := range flowsets { | ||
| otgConfig.Flows().Append(flowGroups[flow].Flows[0:2]...) | ||
|
|
||
| flowNames["v4"] = append(flowNames["v4"], flowGroups[flow].Flows[0].Name()) | ||
| flowNames["v6"] = append(flowNames["v6"], flowGroups[flow].Flows[1].Name()) | ||
| } | ||
|
|
||
| ate.OTG().PushConfig(t, otgConfig) | ||
| ate.OTG().StartProtocols(t) | ||
|
|
||
| t.Logf("Verify OTG BGP sessions up") | ||
| cfgplugins.VerifyOTGBGPEstablished(t, ate) | ||
|
|
||
| t.Logf("Verify DUT BGP sessions up") | ||
| cfgplugins.VerifyDUTBGPEstablished(t, dut) | ||
|
|
||
| time.Sleep(10 * time.Second) | ||
| // Validating routes to prefixes learnt from $ATE2_C.IBGP.v6/128 | ||
| validatePrefixes(t, dut, otgBGPConfig[1].otgPortData[1].IPv6, false, 1, 5) | ||
|
|
||
| t.Log("Validate IPv4 GUE encapsulation and decapsulation") | ||
| sendTrafficCapture(t, ate, otgConfig, flowNames["v4"]) | ||
| otgutils.LogFlowMetrics(t, ate.OTG(), otgConfig) | ||
|
|
||
| for _, flow := range flowNames["v4"] { | ||
| verifyTrafficFlow(t, ate, flow) | ||
| } | ||
|
|
||
| outerGUEIPLayerIPv4.Tos = uint8(dscpValue["BE1"]) | ||
| innerGUEIPLayerIPv4.Tos = uint8(dscpValue["BE1"]) | ||
| if err := validatePacket(t, ate, encapValidation); err != nil { | ||
| t.Errorf("capture and validatePackets failed (): %q", err) | ||
| } | ||
|
|
||
| t.Log("Validate GUE Decapsulation") | ||
| innerGUEIPLayerIPv4.SkipProtocolCheck = true | ||
| innerGUEIPLayerIPv4.Tos = uint8(dscpValue["BE1"]) | ||
| if err := validatePacket(t, ate, decapValidation); err != nil { | ||
| t.Errorf("capture and validatePackets failed (): %q", err) | ||
| } | ||
|
|
||
| t.Log("Validate IPv6 GUE encapsulation") | ||
| sendTrafficCapture(t, ate, otgConfig, flowNames["v6"]) | ||
| otgutils.LogFlowMetrics(t, ate.OTG(), otgConfig) | ||
|
|
||
| for _, flow := range flowNames["v6"] { | ||
| verifyTrafficFlow(t, ate, flow) | ||
| } | ||
|
|
||
| if err := validatePacket(t, ate, encapValidationv6); err != nil { | ||
| t.Errorf("capture and validatePackets failed (): %q", err) | ||
| } | ||
|
|
||
| // Validate the counters received on ATE and DUT are same | ||
| validateOutCounters(t, dut, ate.OTG()) | ||
| } | ||
|
|
||
| func testAF1TrafficMigration(t *testing.T, dut *ondatra.DUTDevice, ate *ondatra.ATEDevice, otgConfig gosnappi.Config) { | ||
| advertiseRoutesWithiBGP([]string{ate2InternalPrefixesV4List[1]}, ate2ppnh1.IPv6, true, fmt.Sprintf("%s-CBGP-2", atePort2RoutesV4)) | ||
| advertiseRoutesWithiBGP([]string{ate2InternalPrefixesV6List[1]}, ate2ppnh1.IPv6, false, fmt.Sprintf("%s-CBGP-2", atePort2RoutesV6)) | ||
| flowsets := []string{"flowSet1", "flowSet5"} | ||
| flowNames := map[string][]string{ | ||
| "v4": {}, | ||
| "v6": {}, | ||
| } | ||
|
|
||
| otgConfig.Flows().Clear() | ||
|
|
||
| for _, flow := range flowsets { | ||
| otgConfig.Flows().Append(flowGroups[flow].Flows[2:4]...) | ||
|
|
||
| flowNames["v4"] = append(flowNames["v4"], flowGroups[flow].Flows[2].Name()) | ||
| flowNames["v6"] = append(flowNames["v6"], flowGroups[flow].Flows[3].Name()) | ||
| } | ||
|
|
||
| ate.OTG().PushConfig(t, otgConfig) | ||
| ate.OTG().StartProtocols(t) | ||
|
|
||
| t.Logf("Verify OTG BGP sessions up") | ||
| cfgplugins.VerifyOTGBGPEstablished(t, ate) | ||
|
|
||
| t.Logf("Verify DUT BGP sessions up") | ||
| cfgplugins.VerifyDUTBGPEstablished(t, dut) | ||
|
|
||
| time.Sleep(10 * time.Second) | ||
| // Validating routes to prefixes learnt from $ATE2_C.IBGP.v6/128 | ||
| validatePrefixes(t, dut, otgBGPConfig[1].otgPortData[1].IPv6, false, 2, 5) | ||
|
|
||
| t.Log("Validate IPv4 GUE encapsulation and decapsulation") | ||
| sendTrafficCapture(t, ate, otgConfig, flowNames["v4"]) | ||
| otgutils.LogFlowMetrics(t, ate.OTG(), otgConfig) | ||
|
|
||
| for _, flow := range flowNames["v4"] { | ||
| verifyTrafficFlow(t, ate, flow) | ||
| } | ||
|
|
||
| outerGUEIPLayerIPv4.Tos = uint8(expectedDscpValue["AF1"]) | ||
| innerGUEIPLayerIPv4.Tos = uint8(expectedDscpValue["AF1"]) | ||
| innerGUEIPLayerIPv4.Protocol = udpEncapPort | ||
| if err := validatePacket(t, ate, encapValidation); err != nil { | ||
| t.Errorf("capture and validatePackets failed (): %q", err) | ||
| } | ||
|
|
||
| t.Log("Validate GUE Decapsulation") | ||
| innerGUEIPLayerIPv4.SkipProtocolCheck = true | ||
| innerGUEIPLayerIPv4.Tos = uint8(expectedDscpValue["AF1"]) | ||
| if err := validatePacket(t, ate, decapValidation); err != nil { | ||
| t.Errorf("capture and validatePackets failed (): %q", err) | ||
| } | ||
|
|
||
| t.Log("Validate IPv6 GUE encapsulation") | ||
| sendTrafficCapture(t, ate, otgConfig, flowNames["v6"]) | ||
| otgutils.LogFlowMetrics(t, ate.OTG(), otgConfig) | ||
|
|
||
| for _, flow := range flowNames["v6"] { | ||
| verifyTrafficFlow(t, ate, flow) | ||
| } | ||
|
|
||
| outerGUEv6Encap.Tos = uint8(dscpValue["AF1"]) | ||
| if err := validatePacket(t, ate, encapValidationv6); err != nil { | ||
| t.Errorf("capture and validatePackets failed (): %q", err) | ||
| } | ||
|
|
||
| // Validate the counters received on ATE and DUT are same | ||
| validateOutCounters(t, dut, ate.OTG()) | ||
| } | ||
|
|
||
| func testAF2TrafficMigration(t *testing.T, dut *ondatra.DUTDevice, ate *ondatra.ATEDevice, otgConfig gosnappi.Config) { | ||
| advertiseRoutesWithiBGP([]string{ate2InternalPrefixesV4List[2]}, ate2ppnh1.IPv6, true, fmt.Sprintf("%s-CBGP-3", atePort2RoutesV4)) | ||
| advertiseRoutesWithiBGP([]string{ate2InternalPrefixesV6List[2]}, ate2ppnh1.IPv6, false, fmt.Sprintf("%s-CBGP-3", atePort2RoutesV6)) | ||
|
|
||
| flowNames := map[string][]string{ | ||
| "v4": {}, | ||
| "v6": {}, | ||
| } | ||
|
|
||
| otgConfig.Flows().Clear() | ||
|
|
||
| otgConfig.Flows().Append(flowGroups["flowSet1"].Flows[4:]...) | ||
| flowNames["v4"] = append(flowNames["v4"], flowGroups["flowSet1"].Flows[4].Name()) | ||
| flowNames["v6"] = append(flowNames["v6"], flowGroups["flowSet1"].Flows[5].Name()) | ||
|
|
||
| otgConfig.Flows().Append(flowGroups["flowSet5"].Flows[4:6]...) | ||
| flowNames["v4"] = append(flowNames["v4"], flowGroups["flowSet5"].Flows[4].Name()) | ||
| flowNames["v6"] = append(flowNames["v6"], flowGroups["flowSet5"].Flows[5].Name()) | ||
|
|
||
| ate.OTG().PushConfig(t, otgConfig) | ||
| ate.OTG().StartProtocols(t) | ||
|
|
||
| t.Logf("Verify OTG BGP sessions up") | ||
| cfgplugins.VerifyOTGBGPEstablished(t, ate) | ||
|
|
||
| t.Logf("Verify DUT BGP sessions up") | ||
| cfgplugins.VerifyDUTBGPEstablished(t, dut) | ||
| time.Sleep(10 * time.Second) | ||
| // Validating routes to prefixes learnt from $ATE2_C.IBGP.v6/128 | ||
| validatePrefixes(t, dut, otgBGPConfig[1].otgPortData[1].IPv6, false, 3, 5) | ||
|
|
||
| t.Log("Validate IPv4 GUE encapsulation and decapsulation") | ||
| sendTrafficCapture(t, ate, otgConfig, flowNames["v4"]) | ||
| otgutils.LogFlowMetrics(t, ate.OTG(), otgConfig) | ||
|
|
||
| for _, flow := range flowNames["v4"] { | ||
| verifyTrafficFlow(t, ate, flow) | ||
| } | ||
|
|
||
| outerGUEIPLayerIPv4.Tos = uint8(expectedDscpValue["AF2"]) | ||
| innerGUEIPLayerIPv4.Tos = uint8(expectedDscpValue["AF2"]) | ||
| innerGUEIPLayerIPv4.Protocol = udpEncapPort | ||
| if err := validatePacket(t, ate, encapValidation); err != nil { | ||
| t.Errorf("capture and validatePackets failed (): %q", err) | ||
| } | ||
|
|
||
| t.Log("Validate GUE Decapsulation") | ||
| innerGUEIPLayerIPv4.SkipProtocolCheck = true | ||
| innerGUEIPLayerIPv4.Tos = uint8(expectedDscpValue["AF2"]) | ||
| if err := validatePacket(t, ate, decapValidation); err != nil { | ||
| t.Errorf("capture and validatePackets failed (): %q", err) | ||
| } | ||
|
|
||
| t.Log("Validate IPv6 GUE encapsulation") | ||
| sendTrafficCapture(t, ate, otgConfig, flowNames["v6"]) | ||
| otgutils.LogFlowMetrics(t, ate.OTG(), otgConfig) | ||
|
|
||
| for _, flow := range flowNames["v6"] { | ||
| verifyTrafficFlow(t, ate, flow) | ||
| } | ||
|
|
||
| outerGUEv6Encap.Tos = uint8(dscpValue["AF2"]) | ||
| if err := validatePacket(t, ate, encapValidationv6); err != nil { | ||
| t.Errorf("capture and validatePackets failed (): %q", err) | ||
| } | ||
|
|
||
| // Validate the counters received on ATE and DUT are same | ||
| validateOutCounters(t, dut, ate.OTG()) | ||
| } | ||
|
|
||
| func testAF3TrafficMigration(t *testing.T, dut *ondatra.DUTDevice, ate *ondatra.ATEDevice, otgConfig gosnappi.Config) { | ||
| advertiseRoutesWithiBGP([]string{ate2InternalPrefixesV4List[3]}, ate2ppnh2.IPv6, true, fmt.Sprintf("%s-CBGP-4", atePort2RoutesV4)) | ||
| advertiseRoutesWithiBGP([]string{ate2InternalPrefixesV6List[3]}, ate2ppnh2.IPv6, false, fmt.Sprintf("%s-CBGP-4", atePort2RoutesV6)) | ||
|
|
||
| flowNames := map[string][]string{ | ||
| "v4": {}, | ||
| "v6": {}, | ||
| } | ||
|
|
||
| otgConfig.Flows().Clear() | ||
|
|
||
| otgConfig.Flows().Append(flowGroups["flowSet2"].Flows[0:2]...) | ||
| flowNames["v4"] = append(flowNames["v4"], flowGroups["flowSet2"].Flows[0].Name()) | ||
| flowNames["v6"] = append(flowNames["v6"], flowGroups["flowSet2"].Flows[1].Name()) | ||
|
|
||
| otgConfig.Flows().Append(flowGroups["flowSet5"].Flows[6:8]...) | ||
| flowNames["v4"] = append(flowNames["v4"], flowGroups["flowSet5"].Flows[6].Name()) | ||
| flowNames["v6"] = append(flowNames["v6"], flowGroups["flowSet5"].Flows[7].Name()) | ||
|
|
||
| ate.OTG().PushConfig(t, otgConfig) | ||
| ate.OTG().StartProtocols(t) | ||
|
|
||
| t.Logf("Verify OTG BGP sessions up") | ||
| cfgplugins.VerifyOTGBGPEstablished(t, ate) | ||
|
|
||
| t.Logf("Verify DUT BGP sessions up") | ||
| cfgplugins.VerifyDUTBGPEstablished(t, dut) | ||
|
|
||
| time.Sleep(10 * time.Second) | ||
| // Validating routes to prefixes learnt from $ATE2_C.IBGP.v6/128 | ||
| validatePrefixes(t, dut, otgBGPConfig[1].otgPortData[1].IPv6, false, 4, 5) | ||
|
|
||
| t.Log("Validate IPv4 GUE encapsulation and decapsulation") | ||
| sendTrafficCapture(t, ate, otgConfig, flowNames["v4"]) | ||
| otgutils.LogFlowMetrics(t, ate.OTG(), otgConfig) | ||
|
|
||
| for _, flow := range flowNames["v4"] { | ||
| verifyTrafficFlow(t, ate, flow) | ||
| } | ||
|
|
||
| outerGUEIPLayerIPv4.Tos = uint8(expectedDscpValue["AF3"]) | ||
| innerGUEIPLayerIPv4.Tos = uint8(expectedDscpValue["AF3"]) | ||
| innerGUEIPLayerIPv4.Protocol = udpEncapPort | ||
| if err := validatePacket(t, ate, encapValidation); err != nil { | ||
| t.Errorf("capture and validatePackets failed (): %q", err) | ||
| } | ||
|
|
||
| t.Log("Validate GUE Decapsulation") | ||
| innerGUEIPLayerIPv4.SkipProtocolCheck = true | ||
| innerGUEIPLayerIPv4.Tos = uint8(expectedDscpValue["AF3"]) | ||
| if err := validatePacket(t, ate, decapValidation); err != nil { | ||
| t.Errorf("capture and validatePackets failed (): %q", err) | ||
| } | ||
|
|
||
| t.Log("Validate IPv6 GUE encapsulation") | ||
| sendTrafficCapture(t, ate, otgConfig, flowNames["v6"]) | ||
| otgutils.LogFlowMetrics(t, ate.OTG(), otgConfig) | ||
|
|
||
| for _, flow := range flowNames["v6"] { | ||
| verifyTrafficFlow(t, ate, flow) | ||
| } | ||
|
|
||
| outerGUEv6Encap.Tos = uint8(dscpValue["AF3"]) | ||
| if err := validatePacket(t, ate, encapValidationv6); err != nil { | ||
| t.Errorf("capture and validatePackets failed (): %q", err) | ||
| } | ||
|
|
||
| // Validate the counters received on ATE and DUT are same | ||
| validateOutCounters(t, dut, ate.OTG()) | ||
| } | ||
|
|
||
| func testAF4TrafficMigration(t *testing.T, dut *ondatra.DUTDevice, ate *ondatra.ATEDevice, otgConfig gosnappi.Config) { | ||
| advertiseRoutesWithiBGP([]string{ate2InternalPrefixesV4List[4]}, ate2ppnh2.IPv6, true, fmt.Sprintf("%s-CBGP-5", atePort2RoutesV4)) | ||
| advertiseRoutesWithiBGP([]string{ate2InternalPrefixesV6List[4]}, ate2ppnh2.IPv6, false, fmt.Sprintf("%s-CBGP-5", atePort2RoutesV6)) | ||
|
|
||
| flowNames := map[string][]string{ | ||
| "v4": {}, | ||
| "v6": {}, | ||
| } | ||
|
|
||
| otgConfig.Flows().Clear() | ||
|
|
||
| otgConfig.Flows().Append(flowGroups["flowSet2"].Flows[2:]...) | ||
| flowNames["v4"] = append(flowNames["v4"], flowGroups["flowSet2"].Flows[2].Name()) | ||
| flowNames["v6"] = append(flowNames["v6"], flowGroups["flowSet2"].Flows[3].Name()) | ||
|
|
||
| otgConfig.Flows().Append(flowGroups["flowSet5"].Flows[8:10]...) | ||
| flowNames["v4"] = append(flowNames["v4"], flowGroups["flowSet5"].Flows[8].Name()) | ||
| flowNames["v6"] = append(flowNames["v6"], flowGroups["flowSet5"].Flows[9].Name()) | ||
|
|
||
| ate.OTG().PushConfig(t, otgConfig) | ||
| ate.OTG().StartProtocols(t) | ||
|
|
||
| t.Logf("Verify OTG BGP sessions up") | ||
| cfgplugins.VerifyOTGBGPEstablished(t, ate) | ||
|
|
||
| t.Logf("Verify DUT BGP sessions up") | ||
| cfgplugins.VerifyDUTBGPEstablished(t, dut) | ||
|
|
||
| time.Sleep(10 * time.Second) | ||
| // Validating routes to prefixes learnt from $ATE2_C.IBGP.v6/128 | ||
| validatePrefixes(t, dut, otgBGPConfig[1].otgPortData[1].IPv6, false, 5, 5) | ||
|
|
||
| t.Log("Validate IPv4 GUE encapsulation and decapsulation") | ||
| sendTrafficCapture(t, ate, otgConfig, flowNames["v4"]) | ||
| otgutils.LogFlowMetrics(t, ate.OTG(), otgConfig) | ||
|
|
||
| for _, flow := range flowNames["v4"] { | ||
| verifyTrafficFlow(t, ate, flow) | ||
| } | ||
| outerGUEIPLayerIPv4.Tos = uint8(expectedDscpValue["AF4"]) | ||
| innerGUEIPLayerIPv4.Tos = uint8(expectedDscpValue["AF4"]) | ||
| innerGUEIPLayerIPv4.Protocol = udpEncapPort | ||
| if err := validatePacket(t, ate, encapValidation); err != nil { | ||
| t.Errorf("capture and validatePackets failed (): %q", err) | ||
| } | ||
|
|
||
| t.Log("Validate GUE Decapsulation") | ||
| innerGUEIPLayerIPv4.SkipProtocolCheck = true | ||
| innerGUEIPLayerIPv4.Tos = uint8(expectedDscpValue["AF4"]) | ||
| if err := validatePacket(t, ate, decapValidation); err != nil { | ||
| t.Errorf("capture and validatePackets failed (): %q", err) | ||
| } | ||
|
|
||
| t.Log("Validate IPv6 GUE encapsulation") | ||
| sendTrafficCapture(t, ate, otgConfig, flowNames["v6"]) | ||
| otgutils.LogFlowMetrics(t, ate.OTG(), otgConfig) | ||
|
|
||
| for _, flow := range flowNames["v6"] { | ||
| verifyTrafficFlow(t, ate, flow) | ||
| } | ||
|
|
||
| outerGUEv6Encap.Tos = uint8(dscpValue["AF4"]) | ||
| if err := validatePacket(t, ate, encapValidationv6); err != nil { | ||
| t.Errorf("capture and validatePackets failed (): %q", err) | ||
| } | ||
|
|
||
| // Validate the counters received on ATE and DUT are same | ||
| validateOutCounters(t, dut, ate.OTG()) | ||
| } |
There was a problem hiding this comment.
The functions testBE1TrafficMigration, testAF1TrafficMigration, testAF2TrafficMigration, testAF3TrafficMigration, and testAF4TrafficMigration are very similar and contain a lot of duplicated code. This makes the test suite harder to maintain. Consider refactoring these into a single, table-driven test or a helper function that accepts parameters for the specific traffic characteristics (like DSCP values, prefix lists, etc.) for each migration scenario. This would significantly reduce code duplication and improve maintainability.
| ipType := "" | ||
| switch params.IPType { | ||
| case "ipv4": | ||
| ipType = "ipv4-over-udp" | ||
| case "ipv6": | ||
| ipType = "ipv6-over-udp" | ||
| case "ip": | ||
| ipType = "ip" | ||
| } | ||
| // tunnel type %s-over-udp udp destination port %v | ||
| cliConfig := fmt.Sprintf(` | ||
| ip decap-group type udp destination port %v payload %s | ||
| tunnel type %s-over-udp udp destination port %v | ||
| ip decap-group %s | ||
| tunnel type UDP | ||
| tunnel decap-ip %s | ||
| tunnel decap-interface %s | ||
| `, params.GUEPort, params.IPType, params.IPType, params.GUEPort, params.AppliedPolicyName, params.TunnelIP, params.InterfaceID) | ||
| `, params.GUEPort, ipType, params.AppliedPolicyName, params.TunnelIP) | ||
| if params.InterfaceID != "" { | ||
| cliConfig += fmt.Sprintf("tunnel decap-interface %s", params.InterfaceID) | ||
| } | ||
| helpers.GnmiCLIConfig(t, dut, cliConfig) |
There was a problem hiding this comment.
The function aristaGueDecapCLIConfig sends multiple separate CLI configuration commands for different properties of the same decap group. While this might work, it is inefficient as it results in multiple gNMI.Set calls. It would be more efficient and atomic to construct a single multi-line CLI string with all the necessary configurations and send it in a single GnmiCLIConfig call. This would also make the configuration logic easier to read and maintain.
| if len(params.DstIp) > 0 { | ||
| tunnelDst := "" | ||
| for i, addr := range params.DstIp { | ||
| tunnelDst += fmt.Sprintf("entry %d tunnel-destination %s \n", i, addr) | ||
| } | ||
| cli = fmt.Sprintf(` | ||
| qos rewrite ipv4-over-udp inner dscp disabled | ||
| qos rewrite ipv6-over-udp inner dscp disabled | ||
| nexthop-group %s type %s | ||
| tunnel-source intf %s | ||
| fec hierarchical | ||
| %s | ||
| `, params.NexthopGrpName, groupType, params.SrcIp, tunnelDst) | ||
| helpers.GnmiCLIConfig(t, dut, cli) | ||
| } | ||
| if params.TTL != 0 { | ||
| cli = fmt.Sprintf(` | ||
| nexthop-group %s type %s | ||
| ttl %v | ||
| `, params.NexthopGrpName, groupType, params.TTL) | ||
| helpers.GnmiCLIConfig(t, dut, cli) | ||
| } | ||
|
|
||
| if params.DSCP != 0 { | ||
| cli = fmt.Sprintf(` | ||
| nexthop-group %s type %s | ||
| tos %v | ||
| `, params.NexthopGrpName, groupType, params.DSCP) | ||
| helpers.GnmiCLIConfig(t, dut, cli) | ||
| } | ||
|
|
||
| if params.DeleteTtl { | ||
| cli = fmt.Sprintf( | ||
| `nexthop-group %s type %s | ||
| no ttl %v | ||
| `, params.NexthopGrpName, groupType, params.TTL) | ||
| helpers.GnmiCLIConfig(t, dut, cli) | ||
| } | ||
|
|
||
| if params.DstUdpPort != 0 { | ||
| cli = fmt.Sprintf(`tunnel type %s udp destination port %v`, groupType, params.DstUdpPort) | ||
| helpers.GnmiCLIConfig(t, dut, cli) | ||
| } |
There was a problem hiding this comment.
The function NextHopGroupConfigForIpOverUdp sends multiple separate CLI configuration commands for different properties of the same next-hop group (e.g., one for destination IPs, one for TTL, one for DSCP). While this might work, it is inefficient as it results in multiple gNMI.Set calls. It would be more efficient and atomic to construct a single multi-line CLI string with all the necessary configurations for the next-hop group and send it in a single GnmiCLIConfig call. This would also make the configuration logic easier to read and maintain.
| if packetVal.InnerIPLayerIPv4.Protocol == packetVal.UDPLayer.DstPort { | ||
| udpLayer := packet.Layer(layers.LayerTypeUDP) | ||
| udp, _ := udpLayer.(*layers.UDP) | ||
| encapPacket = gopacket.NewPacket(udp.Payload, layers.LayerTypeIPv4, gopacket.Default) | ||
| packetVal.InnerIPLayerIPv4.Protocol = 0 |
There was a problem hiding this comment.
The function validateInnerIPv4Header modifies a field of its input parameter packetVal by setting packetVal.InnerIPLayerIPv4.Protocol = 0. This is a side effect that can make the code harder to understand and debug, as the state of the PacketValidation struct changes after being passed to the function. It's better to avoid modifying input parameters. Consider passing the necessary information through other means or creating a local copy to modify if needed.
|
|
||
| outerGUEIPLayerIPv4.Tos = uint8(dscpValue["BE1"]) | ||
| innerGUEIPLayerIPv4.Tos = uint8(dscpValue["BE1"]) | ||
| if err := validatePacket(t, ate, encapValidation); err != nil { |
There was a problem hiding this comment.
Wouldnt this use the default for encapValidation (similarly for "encapValidationv6")? & below are the defaults
encapValidation = &packetvalidationhelpers.PacketValidation{
PortName: "port2", <<------
Validations: validations,
IPv4Layer: outerGUEIPLayerIPv4,
UDPLayer: udpLayer,
InnerIPLayerIPv4: innerGUEIPLayerIPv4,
}
validationsV6 = []packetvalidationhelpers.ValidationType{
packetvalidationhelpers.ValidateIPv4Header,
packetvalidationhelpers.ValidateUDPHeader,
// packetvalidationhelpers.ValidateIPv6Header,
}
encapValidationv6 = &packetvalidationhelpers.PacketValidation{
PortName: "port2", <<------
Validations: validationsV6,
IPv4Layer: outerGUEv6Encap,
UDPLayer: udpLayer,
// IPv6Layer: innerGUEIPLayerIPv6,
}
We should have the Portname used as port3, for testBE1TrafficMigration, testAF1TrafficMigration etc?. Please check the readme for RT-3.52.2 thru RT-3.52.7, Traffic flows between ATE1:Port1 and ATE2:Port3 are successful.
There was a problem hiding this comment.
According to the README, ATE2_INTERNAL_TE11.v4/30 is advertised by ATE_Port3. I assumed that ATE2_INTERNAL_TE11.v4/32 is advertised by ATE_Port2, which is why we are receiving encapsulated packets on Port 2. I have therefore included the necessary validation.
To resolve this issue, I modified the configuration so that ATE_Port3 now advertises /32. This change should help us achieve the desired outcome, as you mentioned in your comment. Could you please confirm whether this trial configuration is correct? If it is, we will need to make a minor update to the README, and I will update the PR.
| outerGUEv6Encap = &packetvalidationhelpers.IPv4Layer{ | ||
| SkipProtocolCheck: true, | ||
| TTL: ttl, | ||
| DstIP: bgpInternalTE10.IPv4, |
There was a problem hiding this comment.
This one (i.e. bgpInternalTE10.IPv4) and bgpInternalTE11.IPv4, are these single /32 IP addresses or ranges like 198.18.10.0/30 as called out in the readme?
Similar comment for the struct OcPolicyForwardingParams
aristaGueDecapCLIConfig also must form the CLI command with prefix length.
There was a problem hiding this comment.
Done
| EthFlow: &otgconfighelpers.EthFlowParams{SrcMAC: otgBGPConfig[1].otgPortData[1].MAC, DstMAC: port2DstMac}, | ||
| VLANFlow: &otgconfighelpers.VLANFlowParams{VLANId: otgBGPConfig[1].otgPortData[0].Subinterface}, | ||
| IPv4Flow: &otgconfighelpers.IPv4FlowParams{IPv4Src: dutloopback0.IPv4, IPv4Dst: bgpInternalTE11.IPv4, TTL: ttl}, | ||
| UDPFlow: &otgconfighelpers.UDPFlowParams{UDPDstPort: udpEncapPort}, |
There was a problem hiding this comment.
As per the readme, flowset5 flows are unencaped. However, here we have these outer IPv4Flow and UDPFlow, plus innerParams. This defines GUE encapsulated flows, no? Shouldn't this be as follows, instead?
// Example for flowSet5-v4-1
{
flows: otgconfighelpers.Flow{
TxPort: otgBGPConfig[1].port, // ATE2 Port1
RxPorts: []string{otgBGPConfig[0].port}, // ATE1 Port1
IsTxRxPort: true,
PacketsToSend: totalPackets,
PpsRate: trafficPps,
FlowName: "flowSet5-v4-1",
EthFlow: &otgconfighelpers.EthFlowParams{SrcMAC: otgBGPConfig[1].otgPortData[0].MAC, DstMAC: port2DstMac},
VLANFlow: &otgconfighelpers.VLANFlowParams{VLANId: otgBGPConfig[1].otgPortData[0].Subinterface},
IPv4Flow: &otgconfighelpers.IPv4FlowParams{IPv4Src: ate2InternalPrefixesV4List[0], IPv4Dst: ate1UserPrefixesV4List[0], DSCP: dscpValue["BE1"], TTL: ttl},
},
},
// Similar for other flowSet5 flows
There was a problem hiding this comment.
On Port 1, we expect to receive decapsulated packets to verify whether the decapsulation process is working correctly. I will send an encapsulated packet from Port 2/Port 3 to Port 1. The DUT will then decapsulate the packet, allowing us to confirm the results on Port 1. The packet sent to Port 1 is similar to the packet we received after encapsulation on Port 2/Port 3.
Please let me know if my understanding is correct.
...tg_tests/staticgueencap_and_bgp_path_selection/staticgueencap_and_bgp_path_selection_test.go
Show resolved
Hide resolved
|
@ASHNA-AGGARWAL-KEYSIGHT - PR #4927 is merged. Keysight to replace v4 tunnels with v6 equivalents as detailed in the Description of the bug b/471388109 |
ASHNA-AGGARWAL-KEYSIGHT
force-pushed
the
fpt_otg_RT_3_52
branch
from
7ee7ed4 to
940eb00
Compare
Added the changes to v6 tunnel |
...tg_tests/staticgueencap_and_bgp_path_selection/staticgueencap_and_bgp_path_selection_test.go
Show resolved
Hide resolved
6 participants
Readme Location: https://github.com/openconfig/featureprofiles/blob/main/feature/policy_forwarding/encapsulation/otg_tests/staticgueencap_and_bgp_path_selection/README.md
Raised an issue for the test not passing completely: https://partnerissuetracker.corp.google.com/issues/441232997
Logs attached: https://partnerissuetracker.corp.google.com/issues/415458482#comment142