Adding PF-1.26: Double GUEv1 Decapsulation for Overlay Probing#5472
Adding PF-1.26: Double GUEv1 Decapsulation for Overlay Probing#5472ASHNA-AGGARWAL-KEYSIGHT wants to merge 3 commits into
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new test suite designed to verify Double GUEv1 Decapsulation, a key requirement for overlay probing. The changes include updates to configuration plugins and flow helper utilities to support the necessary packet encapsulation and device-specific CLI configurations, ensuring robust testing across the defined network topology. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
Pull Request Functional Test Report for #5472 / 5666acbVirtual Devices
Hardware Devices
|
![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 introduces a new test suite for double GUE decapsulation, updates the Arista policy forwarding configuration plugin to support optional interface IDs, and extends OTG flow helpers with custom UDP checksum capabilities. The code review identified several critical issues, including incorrect logic for verifying expected traffic loss in negative test cases, a destination IP mismatch in IPv6 validation, and a protocol inconsistency in IPv4 packet validation. Additionally, feedback was provided to optimize gNMI transactions through better batching, correct a typo in static route error messages, and address the use of identical source and destination IP addresses in test constants.
| func verifyTrafficFlow(t *testing.T, ate *ondatra.ATEDevice, flowName string, trafficloss bool) { | ||
| flowValidation.Flow.Name = flowName | ||
| if err := flowValidation.ValidateLossOnFlows(t, ate); err != nil { | ||
| if trafficloss { | ||
| t.Logf("traffic loss seen as expected") | ||
| } | ||
| t.Errorf("validation on flows failed: %q", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic for handling expected traffic loss in negative test cases is incorrect. If trafficloss is true and loss is detected (causing ValidateLossOnFlows to return an error), the test still calls t.Errorf, which will cause it to fail. Conversely, if no loss is detected when it was expected, the test currently passes. The logic should be updated to correctly handle both cases. Additionally, as a helper function that reports test errors, it should call t.Helper().
func verifyTrafficFlow(t *testing.T, ate *ondatra.ATEDevice, flowName string, trafficloss bool) {
t.Helper()
flowValidation.Flow.Name = flowName
err := flowValidation.ValidateLossOnFlows(t, ate)
if trafficloss {
if err == nil {
t.Errorf("Expected traffic loss on flow %q, but none was detected", flowName)
} else {
t.Logf("Traffic loss seen as expected: %v", err)
}
return
}
if err != nil {
t.Errorf("Validation on flow %q failed: %v", flowName, err)
}
}References
- Helper functions that report test errors should call t.Helper() as the first statement to ensure error logs point to the caller's location.
| NextHeader: 17, | ||
| HopLimit: 69, | ||
| TrafficClass: 32, | ||
| DstIP: srcHostv6, |
There was a problem hiding this comment.
The DstIP validation for the IPv6 inner payload is checking against the source host address (srcHostv6) instead of the destination host address (dstHostv6). This will cause the packet validation to fail.
| DstIP: srcHostv6, | |
| DstIP: dstHostv6, |
There was a problem hiding this comment.
Corrected code?:
decapValidation: &packetvalidationhelpers.PacketValidation{
PortName: "port2",
CaptureName: "decapCapture",
Validations: []packetvalidationhelpers.ValidationType{packetvalidationhelpers.ValidateIPv6Header},
IPv6Layer: &packetvalidationhelpers.IPv6Layer{
// NextHeader is omitted (defaults to 0) to skip the next-header check since H3 has no UDP
HopLimit: 49, // Inner Hop Limit (50) decremented by 1 by the DUT
TrafficClass: 20 << 2, // Inner Traffic Class (DSCP 20 << 2)
DstIP: dstHostv6, // BUG FIX: Must validate against dstHostv6, NOT srcHostv6
},
},
| srcHostv4 = "203.0.113.1" | ||
| srcHostv6 = "2001:db8:3::1" | ||
| dstHostv4 = "203.0.113.1" |
There was a problem hiding this comment.
srcHostv4 and dstHostv4 are defined with the same IP address. This is likely a mistake, as it prevents testing traffic between different hosts and differs from the pattern used for IPv6 addresses where source and destination are distinct.
| } | ||
|
|
||
| if _, err := cfgplugins.NewStaticRouteCfg(b, sV6, dut); err != nil { | ||
| t.Fatalf("Failed to configure IPv4 static route: %v", err) |
There was a problem hiding this comment.
Typo in the error message; it should refer to IPv6 static route configuration.
| t.Fatalf("Failed to configure IPv4 static route: %v", err) | |
| t.Fatalf("Failed to configure IPv6 static route: %v", err) |
References
- In tests, t.Fatalf is preferred over t.Errorf when a failure makes subsequent test steps meaningless, as this fails fast and reduces overall test execution time.
| for _, l := range dutLagData { | ||
| b := &gnmi.SetBatch{} | ||
| // Create LAG interface | ||
| l.LagName = netutil.NextAggregateInterface(t, dut) | ||
| cfgplugins.NewAggregateInterface(t, dut, b, l) | ||
| b.Set(t, dut) | ||
| } |
There was a problem hiding this comment.
The gnmi.SetBatch and b.Set(t, dut) calls are inside the loop. It is more efficient to create the batch outside the loop, add all configurations, and then call Set once to minimize gNMI transactions.
| CaptureName: "decapCapture", | ||
| Validations: []packetvalidationhelpers.ValidationType{packetvalidationhelpers.ValidateIPv4Header}, | ||
| IPv4Layer: &packetvalidationhelpers.IPv4Layer{ | ||
| Protocol: 17, |
There was a problem hiding this comment.
The validation expects Protocol: 17 (UDP), but the innerParams for the flow (lines 367-369) only define an IPv4 header without a UDP layer. If double decapsulation is successful, the received packet should be the inner-most IPv4 packet. Please verify if the inner packet is intended to be UDP or if the validation protocol is incorrect.
There was a problem hiding this comment.
Please fix the gemini recommendations before sending for review to avoid toil on Google's part
Corrected?:
decapValidation: &packetvalidationhelpers.PacketValidation{
PortName: "port2",
CaptureName: "decapCapture",
Validations: []packetvalidationhelpers.ValidationType{packetvalidationhelpers.ValidateIPv4Header},
IPv4Layer: &packetvalidationhelpers.IPv4Layer{
SkipProtocolCheck: true, // H3 has no UDP header, so skip protocol check
TTL: 49, // Inner TTL (50) decremented by 1 by the DUT
Tos: 20 << 2, // Inner DSCP (20) converted to 8-bit TOS (20 << 2)
DstIP: dstHostv4, // Validate against final destination IP
},
},
There was a problem hiding this comment.
This is also the case for line#407 "IPv6Layer: &packetvalidationhelpers.IPv6Layer{"
| Period: &period, | ||
| } | ||
|
|
||
| dutLagData = []*cfgplugins.DUTAggData{ |
There was a problem hiding this comment.
The README Requirement:
- Section: Topology & DUT Configuration
- "DUT Port 3: Configured in soft-loop mode. This port serves as the recirculation point for the double-decap process."
- "Soft-Loop: Enable software loopback on DUT Port 3."
- "Decap Aggregates: Configure DUT:Port3 with an aggregate IPv4 /28 and IPv6 /60 block address."
However, in "DUTAggData" there arent any physical ports tied to the LAG.
Therefore, should the corrected code instead be:
dutLagData = []*cfgplugins.DUTAggData{
{
Attributes: dutPort3Lag,
LacpParams: lacpParams,
AggType: oc.IfAggregate_AggregationType_STATIC,
OndatraPortsIdx: []int{2}, // Bind physical Port 3 to this LAG
},
}
| staticDstHostIp = "203.0.113.0/32" | ||
| staticDstHostIpv6 = "2001:db8:3::/128" |
There was a problem hiding this comment.
README Requirement:
Section: DUT Configuration
- "Configure the DUT to decapsulate Header 2 upon re-entry from the soft-loop on DUT Port 3 and then do a LPM (Longest Prefix Match) lookup on Header 3 that resolves to ATE:Port2 address."
To meet the above requirement, should these instead be the following?
Corrected:
staticDstHostIp = "203.0.113.1/32"
staticDstHostIpv6 = "2001:db8:3::100/128"
You mentioned the following IPs above,
dstHostv4 = "203.0.113.1"
dstHostv6 = "2001:db8:3::100"
How will the static routes match the flows?
| UDPFlow: &otgconfighelpers.UDPFlowParams{UDPDstPort: gueProtocolPort, SetUDPCheckSum: true, UDPCheckSum: 0}, | ||
| }, | ||
| middleParams: otgconfighelpers.Flow{ | ||
| IPv6Flow: &otgconfighelpers.IPv6FlowParams{IPv6Src: midSrcIPv6, IPv6Dst: decapInnerv6, TrafficClass: 32, HopLimit: 60}, |
There was a problem hiding this comment.
For IPv6, configuration in otgflowhelpers.go sets the raw 8-bit field directly. Because it writes to the raw TrafficClass field (which includes both DSCP and ECN), OTG does not perform any automatic shifting. You must manually shift the value left by 2 (e.g., 32 << 2) to ensure it lands in the upper 6 bits of the byte
Corrected?:
middleParams: otgconfighelpers.Flow{
IPv6Flow: &otgconfighelpers.IPv6FlowParams{
IPv6Src: midSrcIPv6,
IPv6Dst: decapInnerv6,
TrafficClass: 32 << 2, // Shift DSCP 32 to set the raw Traffic Class field (128)
HopLimit: 60,
},
UDPFlow: &otgconfighelpers.UDPFlowParams{
UDPDstPort: gueProtocolPort,
},
}
3 participants
Readme Location: https://github.com/openconfig/featureprofiles/blob/main/feature/policy_forwarding/otg_tests/double_gue_decap/README.md
Raised an issue for the test failing: https://partnerissuetracker.corp.google.com/issues/510599726