[RT-7.8] update BGP community and policy configuration - #6028
[RT-7.8] update BGP community and policy configuration#6028LiamWalsh98 wants to merge 9 commits into
Conversation
Ported-from: bgp-oc-yml-migration-infra@7df1f7d5f69bb258ef2a3b42f0e6220af1a8e690 Traceability-ID: INFRA-MAIN-001
Ported-from: 4541fc75d Traceability-ID: F23627-014
Ported-from: bf98d2562 Traceability-ID: F23627-014
Traceability-ID: REST-23627-DEFAULTS
Traceability-ID: REST-BGP-UPREV-DEVIATIONS
Ported-from: 6fd3e3c Traceability-ID: BGP-UPREV-DEFAULT-RT-7-8
Traceability-ID: BGP-UPREV-DEFAULT-RT-7-8
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 the BGP configuration logic to comply with recent changes in the OpenConfig model. The modifications primarily involve relocating community type settings and refactoring how routing policies are applied to neighbors and peer groups to ensure compatibility with the updated schema. 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
|
![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 updates the BGP community match action test to align with the new BGP OpenConfig model by removing the "skip_bgp_send_community_type" deviation and refactoring how apply policies are configured using "gnmi.BatchReplace". The review feedback recommends simplifying the conditional block in "bgpCreateNbr" by removing a redundant "else if" check that has identical logic to the "else" block, and refactoring the duplicated configuration logic for IPv4 and IPv6 AFI-SAFI apply policies into a single loop.
| } else if deviations.SkipBgpPeerGroupSendCommunityType(dut) { | ||
| as4.SetSendCommunityType([]oc.E_Bgp_CommunityType{oc.Bgp_CommunityType_STANDARD}) | ||
| as6.SetSendCommunityType([]oc.E_Bgp_CommunityType{oc.Bgp_CommunityType_STANDARD}) | ||
| } else { | ||
| if deviations.SkipBgpPeerGroupSendCommunityType(dut) { | ||
| as4.SetSendCommunityType([]oc.E_Bgp_CommunityType{oc.Bgp_CommunityType_STANDARD}) | ||
| as6.SetSendCommunityType([]oc.E_Bgp_CommunityType{oc.Bgp_CommunityType_STANDARD}) | ||
| } else { | ||
| pg.SetSendCommunityType([]oc.E_Bgp_CommunityType{oc.Bgp_CommunityType_STANDARD}) | ||
| } | ||
| as4.SetSendCommunityType([]oc.E_Bgp_CommunityType{oc.Bgp_CommunityType_STANDARD}) | ||
| as6.SetSendCommunityType([]oc.E_Bgp_CommunityType{oc.Bgp_CommunityType_STANDARD}) | ||
| } |
There was a problem hiding this comment.
The else if and else blocks are identical and perform the exact same configuration. Since the new BGP OpenConfig model always places send-community-type under AFI-SAFI, the deviation check SkipBgpPeerGroupSendCommunityType is no longer needed here. We can simplify this block by removing the redundant else if check.
} else {
as4.SetSendCommunityType([]oc.E_Bgp_CommunityType{oc.Bgp_CommunityType_STANDARD})
as6.SetSendCommunityType([]oc.E_Bgp_CommunityType{oc.Bgp_CommunityType_STANDARD})
}References
- Follow Go Code Review Comments, Effective Go, and Google Go Style Guide for writing readable Go code with a consistent look and feel. (link)
There was a problem hiding this comment.
This is intentional to keep any possible future changes to SkipBgpPeerGroupSendCommunityType separate from default behaviour
| applyPolicyV4 := &oc.NetworkInstance_Protocol_Bgp_PeerGroup_AfiSafi_ApplyPolicy{} | ||
| applyPolicyV4.SetImportPolicy([]string{policyName}) | ||
| applyPolicyV6 := &oc.NetworkInstance_Protocol_Bgp_PeerGroup_AfiSafi_ApplyPolicy{} | ||
| applyPolicyV6.SetImportPolicy([]string{policyName}) | ||
| gnmi.BatchReplace(batchConfig, bgpPath.PeerGroup(pgName).AfiSafi(oc.BgpTypes_AFI_SAFI_TYPE_IPV4_UNICAST).ApplyPolicy().Config(), applyPolicyV4) | ||
| gnmi.BatchReplace(batchConfig, bgpPath.PeerGroup(pgName).AfiSafi(oc.BgpTypes_AFI_SAFI_TYPE_IPV6_UNICAST).ApplyPolicy().Config(), applyPolicyV6) |
There was a problem hiding this comment.
The configuration logic for IPv4 and IPv6 AFI-SAFI apply policies is identical. Simplify this by using a single loop over the AFI-SAFI types to apply the policy, which improves maintainability and reduces code duplication.
| applyPolicyV4 := &oc.NetworkInstance_Protocol_Bgp_PeerGroup_AfiSafi_ApplyPolicy{} | |
| applyPolicyV4.SetImportPolicy([]string{policyName}) | |
| applyPolicyV6 := &oc.NetworkInstance_Protocol_Bgp_PeerGroup_AfiSafi_ApplyPolicy{} | |
| applyPolicyV6.SetImportPolicy([]string{policyName}) | |
| gnmi.BatchReplace(batchConfig, bgpPath.PeerGroup(pgName).AfiSafi(oc.BgpTypes_AFI_SAFI_TYPE_IPV4_UNICAST).ApplyPolicy().Config(), applyPolicyV4) | |
| gnmi.BatchReplace(batchConfig, bgpPath.PeerGroup(pgName).AfiSafi(oc.BgpTypes_AFI_SAFI_TYPE_IPV6_UNICAST).ApplyPolicy().Config(), applyPolicyV6) | |
| applyPolicy := &oc.NetworkInstance_Protocol_Bgp_PeerGroup_AfiSafi_ApplyPolicy{} | |
| applyPolicy.SetImportPolicy([]string{policyName}) | |
| for _, afiSafi := range []oc.E_BgpTypes_AFI_SAFI_TYPE{oc.BgpTypes_AFI_SAFI_TYPE_IPV4_UNICAST, oc.BgpTypes_AFI_SAFI_TYPE_IPV6_UNICAST} { | |
| gnmi.BatchReplace(batchConfig, bgpPath.PeerGroup(pgName).AfiSafi(afiSafi).ApplyPolicy().Config(), applyPolicy) | |
| } |
References
- Simplify identical logic for different entities (e.g., dut1 and dut2) by using a single loop over a combined data structure or a helper function to improve maintainability.
There was a problem hiding this comment.
matches the style of the remainder of the suite
|
superseded by #6031 |
3 participants
Summary
Update RT-7.8 for the current BGP OpenConfig model.
send-community configuration
Remove the RT-7.8 assignment:
skip_bgp_send_community_type: trueThis changes community configuration from:
/network-instances/network-instance/protocols/protocol/bgp/peer-groups/peer-group/config/send-communityto:
/network-instances/network-instance/protocols/protocol/bgp/peer-groups/peer-group/afi-safis/afi-safi/config/send-community-typeThe test configures this path for both IPv4 and IPv6.
import/export policy configuration
Replace complete
apply-policyparents instead of individualimport-policyandexport-policyleaf lists.Neighbor path:
/network-instances/network-instance/protocols/protocol/bgp/neighbors/neighbor/afi-safis/afi-safi/apply-policy/configPeer-group path:
/network-instances/network-instance/protocols/protocol/bgp/peer-groups/peer-group/afi-safis/afi-safi/apply-policy/configWhen the test moves the policy to a peer group, it deletes the complete former neighbor
apply-policyparent.BGP OpenConfig uprev
The updated model places peer-group
send-community-typeunder AFI-SAFI. It also requires policy changes through the complete containingapply-policyconfiguration.