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, |
| 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.
2 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