BGP OC uprev test changes in RT-1.2, RT-1.27, RT-1.29, RT-1.30, RT-1.32, RT-7.8, RT-7.10, RT-7.11, and gNMI-1.3 - #6031
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 updates several functional network tests (FNTs) to align with the latest BGP OpenConfig model revisions. The changes primarily focus on standardizing how import/export policies are managed, updating configuration paths for send-community and MED actions, and cleaning up redundant platform deviation assignments. Additionally, the PR introduces helper functions to improve test reliability and adds compatibility support for IS-IS authentication on specific platforms. 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
|
LiamWalsh98
changed the title
![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 refactors and cleans up several BGP policy and static route redistribution tests, primarily by standardizing policy application/deletion, handling various platform deviations (such as FlattenPolicyWithMultipleStatements and SetISISAuthWithInterfaceAuthenticationContainer), and improving OTG telemetry validation. The review feedback highlights a copy-paste bug in import_export_multi_test.go where default policies are set on the wrong policy object, and recommends defensive checks in awaitBGPReady to prevent nil pointer dereferences. Additionally, the reviewer advises replacing direct gnmi.Get calls with gnmi.Lookup in static_route_bgp_redistribution_test.go and drained_configuration_convergence_time_bgp_test.go to prevent fatal test failures when configurations are missing.
| for _, device := range bs.ATETop.Devices().Items() { | ||
| bgp4Peer := device.Bgp().Ipv4Interfaces().Items()[0].Peers().Items()[0] | ||
| bgp6Peer := device.Bgp().Ipv6Interfaces().Items()[0].Peers().Items()[0] | ||
| gnmi.Await(t, bs.ATE.OTG(), gnmi.OTG().BgpPeer(bgp4Peer.Name()).SessionState().State(), 2*time.Minute, otgtelemetry.BgpPeer_SessionState_ESTABLISHED) | ||
| gnmi.Await(t, bs.ATE.OTG(), gnmi.OTG().BgpPeer(bgp6Peer.Name()).SessionState().State(), 2*time.Minute, otgtelemetry.BgpPeer_SessionState_ESTABLISHED) | ||
| } |
There was a problem hiding this comment.
To enforce defensive programming and prevent potential runtime panics (e.g., index out of bounds or nil pointer dereferences), we should explicitly check if device.Bgp() is non-nil and ensure that both Ipv4Interfaces and Ipv6Interfaces contain items before accessing their elements.
| for _, device := range bs.ATETop.Devices().Items() { | |
| bgp4Peer := device.Bgp().Ipv4Interfaces().Items()[0].Peers().Items()[0] | |
| bgp6Peer := device.Bgp().Ipv6Interfaces().Items()[0].Peers().Items()[0] | |
| gnmi.Await(t, bs.ATE.OTG(), gnmi.OTG().BgpPeer(bgp4Peer.Name()).SessionState().State(), 2*time.Minute, otgtelemetry.BgpPeer_SessionState_ESTABLISHED) | |
| gnmi.Await(t, bs.ATE.OTG(), gnmi.OTG().BgpPeer(bgp6Peer.Name()).SessionState().State(), 2*time.Minute, otgtelemetry.BgpPeer_SessionState_ESTABLISHED) | |
| } | |
| for _, device := range bs.ATETop.Devices().Items() { | |
| if device.Bgp() == nil { | |
| continue | |
| } | |
| if len(device.Bgp().Ipv4Interfaces().Items()) > 0 && len(device.Bgp().Ipv4Interfaces().Items()[0].Peers().Items()) > 0 { | |
| bgp4Peer := device.Bgp().Ipv4Interfaces().Items()[0].Peers().Items()[0] | |
| gnmi.Await(t, bs.ATE.OTG(), gnmi.OTG().BgpPeer(bgp4Peer.Name()).SessionState().State(), 2*time.Minute, otgtelemetry.BgpPeer_SessionState_ESTABLISHED) | |
| } | |
| if len(device.Bgp().Ipv6Interfaces().Items()) > 0 && len(device.Bgp().Ipv6Interfaces().Items()[0].Peers().Items()) > 0 { | |
| bgp6Peer := device.Bgp().Ipv6Interfaces().Items()[0].Peers().Items()[0] | |
| gnmi.Await(t, bs.ATE.OTG(), gnmi.OTG().BgpPeer(bgp6Peer.Name()).SessionState().State(), 2*time.Minute, otgtelemetry.BgpPeer_SessionState_ESTABLISHED) | |
| } | |
| } |
| tableConn := gnmi.Get[*oc.NetworkInstance_TableConnection](t, dut, tableConnPath.Config()) | ||
| tableConn.ImportPolicy = nil | ||
| gnmi.Replace(t, dut, tableConnPath.Config(), tableConn) |
There was a problem hiding this comment.
Using gnmi.Get directly on tableConnPath.Config() will cause the test to fail immediately if the TableConnection is not yet configured (for example, when running this subtest individually). To ensure test independence and robustness, use gnmi.Lookup to check if the table connection is present before attempting to modify it.
tableConn, present := gnmi.Lookup(t, dut, tableConnPath.Config()).Val()
if !present || tableConn == nil {
return
}
tableConn.ImportPolicy = nil
gnmi.Replace(t, dut, tableConnPath.Config(), tableConn)| func replacePeerGroupExportPolicy(t *testing.T, dut *ondatra.DUTDevice, policy string) { | ||
| bgpPath := gnmi.OC().NetworkInstance(deviations.DefaultNetworkInstance(dut)). | ||
| Protocol(oc.PolicyTypes_INSTALL_PROTOCOL_TYPE_BGP, "BGP").Bgp() | ||
| if deviations.RoutePolicyUnderAFIUnsupported(dut) { | ||
| policyPath := bgpPath.PeerGroup(setup.PeerGrpName).ApplyPolicy() | ||
| applyPolicy := gnmi.Get(t, dut, policyPath.Config()) | ||
| applyPolicy.SetExportPolicy([]string{policy}) | ||
| gnmi.Replace(t, dut, policyPath.Config(), applyPolicy) | ||
| return | ||
| } | ||
| policyPath := bgpPath.PeerGroup(setup.PeerGrpName).AfiSafi(oc.BgpTypes_AFI_SAFI_TYPE_IPV4_UNICAST).ApplyPolicy() | ||
| applyPolicy := gnmi.Get(t, dut, policyPath.Config()) | ||
| applyPolicy.SetExportPolicy([]string{policy}) | ||
| gnmi.Replace(t, dut, policyPath.Config(), applyPolicy) | ||
| } |
There was a problem hiding this comment.
Using gnmi.Get directly on policyPath.Config() assumes that the ApplyPolicy container is always present. If the subtests are run individually or if the configuration is not present, this will cause a fatal test failure. Using gnmi.Lookup with a fallback to a newly created object (similar to the pattern used in other tests) is much safer and more robust.
func replacePeerGroupExportPolicy(t *testing.T, dut *ondatra.DUTDevice, policy string) {
bgpPath := gnmi.OC().NetworkInstance(deviations.DefaultNetworkInstance(dut)).
Protocol(oc.PolicyTypes_INSTALL_PROTOCOL_TYPE_BGP, "BGP").Bgp()
if deviations.RoutePolicyUnderAFIUnsupported(dut) {
policyPath := bgpPath.PeerGroup(setup.PeerGrpName).ApplyPolicy()
applyPolicy, present := gnmi.Lookup(t, dut, policyPath.Config()).Val()
if !present {
applyPolicy = &oc.NetworkInstance_Protocol_Bgp_PeerGroup_ApplyPolicy{}
}
applyPolicy.SetExportPolicy([]string{policy})
gnmi.Replace(t, dut, policyPath.Config(), applyPolicy)
return
}
policyPath := bgpPath.PeerGroup(setup.PeerGrpName).AfiSafi(oc.BgpTypes_AFI_SAFI_TYPE_IPV4_UNICAST).ApplyPolicy()
applyPolicy, present := gnmi.Lookup(t, dut, policyPath.Config()).Val()
if !present {
applyPolicy = &oc.NetworkInstance_Protocol_Bgp_PeerGroup_AfiSafi_ApplyPolicy{}
}
applyPolicy.SetExportPolicy([]string{policy})
gnmi.Replace(t, dut, policyPath.Config(), applyPolicy)
}
3 participants
Summary
This PR updates nine FNTs for the current BGP OpenConfig model.
The changes update
send-communitypaths, import/export policy replacement, and MED actions. The PR also includes test-specific reliability changes.OpenConfig path changes
Peer-group
send-communityconfigurationPrevious root leaves:
/network-instances/network-instance/protocols/protocol/bgp/peer-groups/peer-group/config/send-community/network-instances/network-instance/protocols/protocol/bgp/peer-groups/peer-group/config/send-community-typeCurrent AFI-SAFI leaf:
/network-instances/network-instance/protocols/protocol/bgp/peer-groups/peer-group/afi-safis/afi-safi/config/send-community-typeNeighbor
send-communityconfigurationPrevious root leaf:
/network-instances/network-instance/protocols/protocol/bgp/neighbors/neighbor/config/send-community-typeCurrent AFI-SAFI leaf:
/network-instances/network-instance/protocols/protocol/bgp/neighbors/neighbor/afi-safis/afi-safi/config/send-community-typeImport/export policy root replacement
Previous operation:
.../apply-policy/config/import-policyor.../apply-policy/config/export-policy.Current operation:
.../apply-policy/configroot.import-policyorexport-policyin the returned configuration..../apply-policy/configroot.MED actions
The tests continue to set the MED value at:
/routing-policy/policy-definitions/policy-definition/statements/statement/actions/bgp-actions/config/set-medThe tests now set the related operation at:
/routing-policy/policy-definitions/policy-definition/statements/statement/actions/bgp-actions/config/set-med-actionThe tests use
SETfor assignment. RT-1.32 also usesADDfor its additive MED case.IS-IS authentication in gNMI-1.3
Previous level path:
/network-instances/network-instance/protocols/protocol/isis/interfaces/interface/levels/level/hello-authentication/configCurrent interface path for the applicable platform exception:
/network-instances/network-instance/protocols/protocol/isis/interfaces/interface/authentication/configThis compatibility change is not related to the BGP OpenConfig uprev.
Removed deviation assignments
The PR removes these suite metadata assignments:
bgp_set_med_action_unsupported: gNMI-1.3, RT-1.2, RT-1.27, RT-1.29, RT-1.30, RT-1.32, and RT-7.11.skip_bgp_send_community_type: RT-1.27, RT-7.8, and RT-7.11.skip_bgp_peer_group_send_community_type: RT-1.27.bgp_community_type_slice_input_unsupported: RT-7.10.Affected FNTs
RT-1.2
set-medwithset-med-action: SETand waits for IPv4/IPv6 neighbor resolution before BGP peer checks.RT-1.27
send-community-type: STANDARDfrom the global AFI-SAFI to the peer-group IPv4/IPv6 AFI-SAFIs, pairsset-medwithset-med-action: SET, and replaces the complete table-connection root when clearingimport-policy.RT-1.29
set-medwithset-med-action: SET.RT-1.30
set-medwithset-med-action: SET.RT-1.32
set-med-action: SETfor MED assignment andADDfor MED increments.RT-7.8
send-community-type: STANDARDand replaces complete neighbor or peer-group AFI-SAFI import/export policy roots.RT-7.10
send-community-type: STANDARDfrom the peer-group root to the IPv4 peer-group AFI-SAFI.RT-7.11
send-community-type: STANDARDandEXTENDED, and pairsset-medwithset-med-action: SET.gNMI-1.3
import-policywrite, and pairsset-medwithset-med-action: SET.set_isis_auth_with_interface_authentication_container; this is a compatibility fix from a different OC uprev, but gnmi-1.3 is affected by both.