Sys-2.1 configured acl and create deviation for /description leaf#5226
Sys-2.1 configured acl and create deviation for /description leaf#5226fmolinar wants to merge 1 commit intoopenconfig:mainfrom
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 significantly upgrades the control plane ACL testing framework by integrating robust packet capture verification, enhancing DUT configuration logic for loopback interfaces and static routes, and introducing a deviation mechanism for ACL description support. These changes aim to improve the accuracy and reliability of control plane ACL tests across various network device platforms. Highlights
Changelog
Activity
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 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 counter productive. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Pull Request Functional Test Report for #5226 / 30be7e1Virtual 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 deviation for ACL descriptions, adds static routes for return traffic, and significantly improves test verification by capturing and analyzing DUT responses. The changes are generally good, but there are several opportunities for improvement regarding test structure, code duplication, and adherence to contribution guidelines. I've identified some areas in the test logic and code style that could be refactored for better correctness, maintainability, and robustness.
| // set ports and device configuration | ||
| atePorts := sortPorts(ate.Ports()) | ||
| p0 := atePorts[0] | ||
| otgConfig.Ports().Add().SetName(p0.ID()) | ||
| srcDev := otgConfig.Devices().Add().SetName(atePort1.Name) | ||
| t.Logf("The name of the source device is %s", srcDev.Name()) | ||
| srcEth := srcDev.Ethernets().Add().SetName(atePort1.Name + ".Eth").SetMac(atePort1.MAC) | ||
| srcEth.Connection().SetPortName(p0.ID()) | ||
| srcEth.Ipv4Addresses().Add().SetName(atePort1.Name + ".IPv4").SetAddress(atePort1.IPv4).SetGateway(dutPort1.IPv4).SetPrefix(uint32(atePort1.IPv4Len)) | ||
| srcEth.Ipv6Addresses().Add().SetName(atePort1.Name + ".IPv6").SetAddress(atePort1.IPv6).SetGateway(dutPort1.IPv6).SetPrefix(uint32(atePort1.IPv6Len)) | ||
| otgConfig.Captures().Add().SetName("permitResponseCapture").SetPortNames([]string{p0.ID()}).SetFormat(gosnappi.CaptureFormat.PCAP) | ||
|
|
||
| ate.OTG().PushConfig(t, otgConfig) |
There was a problem hiding this comment.
The ATE configuration logic is confusing. configureATE is called at the start of TestControlPlaneACL (line 480), which pushes an initial configuration to the ATE. However, this subtest then creates a new, empty otgConfig, re-builds the entire ATE topology (duplicating code from configureATE), and pushes it again, completely overwriting the previous configuration. This makes the initial configureATE call redundant and the test structure hard to follow. Consider refactoring the ATE configuration to be done once, or in a more modular way within each subtest, to avoid this overwrite and code duplication.
| otgConfig := gosnappi.NewConfig() | ||
| otgConfig.Ports().Add().SetName(p0.ID()) |
There was a problem hiding this comment.
The OTG configuration in the SYS-2.1.2 subtest is incomplete. It only adds a port but does not configure the OTG device, ethernet interface, or IP addresses. This makes the test fragile as it implicitly depends on the configuration from the preceding SYS-2.1.1 subtest. Each subtest should be self-contained and set up its required ATE configuration explicitly to ensure it can run independently and reliably. The ATE setup logic from the SYS-2.1.1 subtest should be replicated here.
| lb0 := netutil.LoopbackInterface(t, dut, 0) | ||
| _, lb0Present := gnmi.Lookup(t, dut, gnmi.OC().Interface(lb0).Name().State()).Val() | ||
| lb := lb0 | ||
| if lb0Present { | ||
| lb = netutil.LoopbackInterface(t, dut, 1) | ||
| } |
There was a problem hiding this comment.
The logic to select a loopback interface is inefficient. It checks for the presence of Loopback0 and, if present, unconditionally switches to using Loopback1. If Loopback0 is already present and correctly configured with the required IP addresses, the test will unnecessarily proceed to check and configure Loopback1. The logic should first verify if Loopback0 is correctly configured and only use an alternative interface if it is not.
| if !foundICMPv4Reply { | ||
| t.Errorf("Did not find IPv4 ICMP echo reply from DUT in ATE capture") | ||
| } | ||
| if !foundTCPSynAckV4 { | ||
| t.Errorf("Did not find IPv4 TCP SYN-ACK from 198.51.100.1 to 192.0.2.100 in ATE capture") | ||
| } | ||
| if !foundICMPv6Reply { | ||
| t.Errorf("Did not find IPv6 ICMP echo reply from DUT in ATE capture") | ||
| } | ||
| if !foundTCPSynAckV6 { | ||
| t.Errorf("Did not find IPv6 TCP SYN-ACK from 2001:db8::1 to 2001:db8::100 in ATE capture") | ||
| } |
There was a problem hiding this comment.
The error messages in verifyDUTResponsesInCapture contain hardcoded IP addresses. For better maintainability and readability, these should be replaced with the constants defined for these addresses (e.g., dutLoopbackIPv4, mgmtSrcIPv4).
| if !foundICMPv4Reply { | |
| t.Errorf("Did not find IPv4 ICMP echo reply from DUT in ATE capture") | |
| } | |
| if !foundTCPSynAckV4 { | |
| t.Errorf("Did not find IPv4 TCP SYN-ACK from 198.51.100.1 to 192.0.2.100 in ATE capture") | |
| } | |
| if !foundICMPv6Reply { | |
| t.Errorf("Did not find IPv6 ICMP echo reply from DUT in ATE capture") | |
| } | |
| if !foundTCPSynAckV6 { | |
| t.Errorf("Did not find IPv6 TCP SYN-ACK from 2001:db8::1 to 2001:db8::100 in ATE capture") | |
| } | |
| if !foundICMPv4Reply { | |
| t.Errorf("Did not find IPv4 ICMP echo reply from DUT in ATE capture") | |
| } | |
| if !foundTCPSynAckV4 { | |
| t.Errorf("Did not find IPv4 TCP SYN-ACK from %s to %s in ATE capture", dutLoopbackIPv4, mgmtSrcIPv4) | |
| } | |
| if !foundICMPv6Reply { | |
| t.Errorf("Did not find IPv6 ICMP echo reply from DUT in ATE capture") | |
| } | |
| if !foundTCPSynAckV6 { | |
| t.Errorf("Did not find IPv6 TCP SYN-ACK from %s to %s in ATE capture", dutLoopbackIPv6, mgmtSrcIPv6) | |
| } |
| // ACLDescriptionUnsupported returns true if ACL description is unsupported | ||
| // | ||
| // /acl/acl-sets/acl-set/state/descriptionmake proto/metadata_go_proto/metadata.pb.go |
There was a problem hiding this comment.
The comment for the ACLDescriptionUnsupported deviation function has a typo and is missing required information.
- There is a typo:
descriptionmakeshould bedescription make. - According to the repository's deviation guidelines (rule 3.5), the comment must include a URL to an issue tracker. The URL
https://partnerissuetracker.corp.google.com/issues/492342925fromproto/metadata.protoshould be added here.
// ACLDescriptionUnsupported returns true if ACL description is unsupported.
//
// See: https://partnerissuetracker.corp.google.com/issues/492342925
//
// /acl/acl-sets/acl-set/state/descriptionReferences
- A comment should be added to accessor functions for deviations, containing a URL link to an issue tracker which tracks removal of the deviation. (link)
2 participants
— Major test updates:
New imports: bytes, fmt, net, gopacket, gopacket/layers, gopacket/pcapgo, and deviations.
configureDUTLoopback: Now checks if loopback 0 already has the expected IPs (both v4 and v6) before deciding to configure an alternate loopback index. Previously it always used loopback 0 unconditionally.
configureDUT: Adds static routes for mgmtSrcIPv4/32 and mgmtSrcIPv6/128 pointing to atePort1 IPs, ensuring the DUT can send responses back to the ATE.
configureACLs: All Description fields on ACL sets and terms are now guarded by !deviations.ACLDescriptionUnsupported(dut).
createFlow: Sets TCP SYN flag on TCP flows; uses the actual port ID (not atePort1.Name) for Tx/Rx.
New verifyDUTResponsesInCapture: Parses a PCAP capture from the ATE and asserts that ICMP echo replies and TCP SYN-ACK responses are received from the DUT's loopback address — confirming permitted traffic is actually processed.
TestControlPlaneACL / SYS-2.1.1: Adds OTG port/device config and packet capture (start/stop) within the test run; calls verifyDUTResponsesInCapture instead of the former TODO comment. SSH flow source port changed from 12345 → 12346.
SYS-2.1.2: Adds otgConfig.Ports().Add() that was previously missing.