-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[bgp-cfgd] BGP allow list enhancement #11586
[bgp-cfgd] BGP allow list enhancement #11586
Conversation
what is meant for different neighbor type ? Allow list is for T0 neighbor type |
""" | ||
Generate prefix-list names for a given peer_ip and community value | ||
:param deployment_id: deployment_id for which we're going to filter prefixes | ||
:param community_value: community, which we want to use to filter prefixes | ||
:return: a dictionary with names | ||
""" | ||
if community_value == BGPAllowListMgr.EMPTY_COMMUNITY: | ||
community_name = BGPAllowListMgr.EMPTY_COMMUNITY | ||
if neighbor_type == '': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just thinking, why not ignore neighbor_type
completely and use the existing flow? Will there be any conflicts?
For e.g:
ip prefix-list PL_ALLOW_LIST_DEPLOYMENT_ID_5_COMMUNITY_empty_V4 seq 10 deny 0.0.0.0/0 le 17
instead of
ip prefix-list PL_ALLOW_LIST_DEPLOYMENT_ID_5_NEIGHBOR_OpticalLonghaulTerminal_COMMUNITY_empty_V4 seq 10 deny 0.0.0.0/0 le 17
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@prsunny The neighbor of Mgmt ToR could be T1 or OLT, to add neighbor type info to tell the difference. Since it is possible to add different allow prefix list towards T1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, @abdosi to review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Why I did it 2 things are missing in current allow-prefix list implementation. In some usecase, need to tell the BGP neighbor and have different allow-prefix list for different neighbors, which is not supported. for the prefix list, can't support flexible le and ge. How I did it To enhance the bgp allow-prefix list feature to have: To include the neighbor type info for the allow-prefix list. To support flexible le and ge length for allow-prefix list. How to verify it 4 new unit test cases are added in this PR to cover changes.
Why I did it 2 things are missing in current allow-prefix list implementation. In some usecase, need to tell the BGP neighbor and have different allow-prefix list for different neighbors, which is not supported. for the prefix list, can't support flexible le and ge. How I did it To enhance the bgp allow-prefix list feature to have: To include the neighbor type info for the allow-prefix list. To support flexible le and ge length for allow-prefix list. How to verify it 4 new unit test cases are added in this PR to cover changes.
Why I did it 2 things are missing in current allow-prefix list implementation. In some usecase, need to tell the BGP neighbor and have different allow-prefix list for different neighbors, which is not supported. for the prefix list, can't support flexible le and ge. How I did it To enhance the bgp allow-prefix list feature to have: To include the neighbor type info for the allow-prefix list. To support flexible le and ge length for allow-prefix list. How to verify it 4 new unit test cases are added in this PR to cover changes.
Why I did it For mgmt ToR which is connected to OLT, need to have bgp allow-prefix list to control the routes advertisement between OLT and mgmt ToR. How I did it To add new template for this Scenario, which is based on common mgmt ToR BGP template. below is the doc for detailed design. https://microsoft.sharepoint.com/:w:/t/Aznet/EUXF5cffHVtDspT4W0o3ni8Bax5uGLwe5_hRGQnERDYrbg?e=rtmE9c The changes in managers_allow_list.py is a common one and PR in public repo. sonic-net#11586 How to verify it 1. Unit Test Add new unit test cases for new templates. 2. Integration Test To simulate the Scenario in physical TB, and verify the run bgp configuration. WIP, to verify with Optical team. Others Need to port back to 202012 and 202205 branch.
Why I did it For mgmt ToR which is connected to OLT, need to have bgp allow-prefix list to control the routes advertisement between OLT and mgmt ToR. How I did it To add new template for this Scenario, which is based on common mgmt ToR BGP template. below is the doc for detailed design. https://microsoft.sharepoint.com/:w:/t/Aznet/EUXF5cffHVtDspT4W0o3ni8Bax5uGLwe5_hRGQnERDYrbg?e=rtmE9c The changes in managers_allow_list.py is a common one and PR in public repo. sonic-net#11586 How to verify it 1. Unit Test Add new unit test cases for new templates. 2. Integration Test To simulate the Scenario in physical TB, and verify the run bgp configuration. WIP, to verify with Optical team. Others Need to port back to 202012 and 202205 branch. Cherry picked from !6446272
Why I did it
2 things are missing in current allow-prefix list implementation.
How I did it
To enhance the bgp allow-prefix list feature to have:
How to verify it
4 new unit test cases are added in this PR to cover changes.
Which release branch to backport (provide reason below if selected)
Description for the changelog
Link to config_db schema for YANG module changes
Yang model changes would be implemented by another PR.
A picture of a cute animal (not mandatory but encouraged)