[bgp] Add 'allow list' manager feature #5309
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
test failure, can you also add description for this pr? |
|
retest vsimage please |
jleveque
changed the title
lguohan
reviewed
Sep 21, 2020
lguohan
reviewed
Sep 22, 2020
lguohan
reviewed
Sep 22, 2020
lguohan
reviewed
Sep 22, 2020
lguohan
reviewed
Sep 22, 2020
src/sonic-bgpcfgd/app/allow_list.py
Outdated
| :param deployment_id: deployment_id number | ||
| """ | ||
| log_info("BGPAllowListMgr::Restart peers with deployment_id=%d" % deployment_id) | ||
| no_error, _, _ = run_command(["vtysh", "-c", "clear bgp * soft in"]) |
There was a problem hiding this comment.
are we clearing all peers? including the upstream peers?
There was a problem hiding this comment.
it does not see you only clear the neighbor with deployment_id. impl and description does not match.
There was a problem hiding this comment.
Yes. Currently I clear all peers. But in this case I use soft in which will not reset any established bgp session, it will just update it.
It is not easy to find a neighbor with the deployment_id. For that I need to make a map between peer-group and neighbors and between peer-group and deployment id.
I'll look at it today.
There was a problem hiding this comment.
I am not sure I understand the impact here. does this soft in clear withdrawn and then readvertise all the routes?
There was a problem hiding this comment.
I have added implementation of reboot only required peer-groups.
lguohan
reviewed
Sep 22, 2020
lguohan
reviewed
Sep 22, 2020
lguohan
reviewed
Sep 22, 2020
added 7 commits
September 23, 2020 09:53
|
app/directory.py 63 44 30% both seems low, can you improve it? |
|
Yes, I can. |
|
both directory and manager are added in this pr. |
|
@lguohan: The objects Directory and Manager were moved from bgpcfgd to their own modules to make it possible to use them from modules. Both of them were written before this PR |
lguohan
approved these changes
Sep 25, 2020
|
retest vsimage please |
|
retest mellanox please |
abdosi
pushed a commit
that referenced
this pull request
Sep 28, 2020
implements a new feature: "BGP Allow list." This feature allows us to control which IP prefixes are going to be advertised via ebgp from the routes received from EBGP neighbors.
3 tasks
3 tasks
wangxin
added a commit
to sonic-net/sonic-mgmt
that referenced
this pull request
Nov 25, 2020
What is the motivation for this PR? The BGP allow list feature was introduced in SONiC. This PR is to add a script for testing the BGP allow list feature. How did you do it? Add a new script for testing BGP allow list. Covered scenarios: * Ensure that constants.bgp.allow_list.default_action is "permit". No BGP allow list is configured. * Announce routes with and without test community '1010:1010' to the first T0 VM by exabgp. * Check routes on the first T0 VM. All the routes should be successfully injected. * Check routes on DUT. All the routes should be accepted by DUT. * Check routes on other T0 and T2 VMs. All the routes should be advertised by DUT. The drop_community defined in * /etc/sonic/constants.yml should be added to all routes. The original community of routes should be kept. * Ensure that constants.bgp.allow_list.default_action is "permit". BGP allow list is configured. * Announce routes with and without test community '1010:1010' to the first T0 VM by exabgp. * Check routes on the first T0 VM. All the routes should be successfully injected. * Check routes on DUT. All the routes should be accepted by DUT. * Check routes on other T0 and T2 VMs. All the routes should be advertised by DUT. The drop_community should only be * added to routes not on allow list. The original community of routes should be kept. * Ensure that constants.bgp.allow_list.default_action is "deny". No BGP allow list is configured. * Announce routes with and without test community '1010:1010' to the first T0 VM by exabgp. * Check routes on the first T0 VM. All the routes should be successfully injected. * Check routes on DUT. All the routes should be accepted by DUT. * Check routes on other T0 and T2 VMs. No routes should be advertised by DUT. * Ensure that constants.bgp.allow_list.default_action is "deny". BGP allow list is configured. * Announce routes with and without test community '1010:1010' to the first T0 VM by exabgp. * Check routes on the first T0 VM. All the routes should be successfully injected. * Check routes on DUT. All the routes should be accepted by DUT. * Check routes on other T0 and T2 VMs. Only the routes on allow list should be advertised by DUT. No drop_community should be added to advertised routes. The original community of routes should be kept. Relevant change: sonic-net/sonic-buildimage#5309 How did you verify/test it? Run the test script using latest master image. Currently some test cases can't pass because of issue: sonic-net/sonic-buildimage#6001 If add `on-match next` to /usr/share/sonic/templates/bgpd/templates/general/policies.conf.j2 and restart bgp service, then all the cases can pass: ``` route-map FROM_BGP_PEER_V6 permit 1 on-match next set ipv6 next-hop prefer-global ``` Any platform specific information? No Supported testbed topology if it's a new test case? This test only supports topology type t1. Signed-off-by: Xin Wang <xiwang5@microsoft.com>
santhosh-kt
pushed a commit
to santhosh-kt/sonic-buildimage
that referenced
this pull request
Feb 25, 2021
implements a new feature: "BGP Allow list." This feature allows us to control which IP prefixes are going to be advertised via ebgp from the routes received from EBGP neighbors.
santhosh-kt
pushed a commit
to santhosh-kt/sonic-buildimage
that referenced
this pull request
Feb 25, 2021
This reverts commit 6eed082.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR implements a new feature: "BGP Allow list."
This feature allows us to control which IP prefixes are going to be advertised via ebgp from the routes received from EBGP neighbors.
The feature can be enabled or disabled using "allow_list_enabled" flag in the /etc/sonic/deployment_id_asn_map.yml`
There is another flag for the feature: default_action. When this flag is equal "permit", this feature will not drop any BGP prefixes. The prefixes will be marked with drop_community. It is useful for debugging. But when the flag is equal to "deny", all prefixes, which are not explicitly listed as "Allowed" will be marked with no-export community which prevent them from being exported to ebgp neighbors.
We use prefix-lists for each list of filtered prefixes. You can add default entries which are going to be inserted to the prefix-list to default_pl_rules
We can control the feature by using the following schema:
The schema above means the following:
a. Import IPv4 prefixes "10.20.0.0/16" and "10.50.1.0/29" only if they have associated BGP community "1010:1010".
b. Import IPv6 prefixes "fc01:10::/64" and "fc02:20::/64" only if they have associated BGP community "1010:1010".
c. Import IPv4 prefixes "10.20.0.0/16" and "10.50.1.0/29". The community is not required.
d. Import IPv6 prefixes "fc01:10::/64" and "fc02:20::/64". The community is not required.
All other prefixes will be either marked or dropped (depends on the "default_action" flag. see above for "allow_list_default_action").
When BGP starts and this feature is enabled, all prefixes are going to be either marked with the community or marked with "no-export" community. That depends on "default_action" flag.
The following BGP configuration is generated by default:
When we apply the configuration you can find on the top of this PR description, the following BGP configuration will be generated.
As you can see from the output above the feature is isolated inside of "ALLOW_LIST_DEPLOYMENT_ID_0_V4" and "ALLOW_LIST_DEPLOYMENT_ID_0_V6" route-maps. We call the route-maps on top of each "FROM" route-map for each deployment_id. When the "ALLOW_LIST_DEPLOYMENT_ID_0_V*" route-map evaluation returns "deny", the "FROM" route-map will return deny. But when "ALLOW_LIST_DEPLOYMENT_ID_0_V*" route-map evaluation returns "permit", the next "FROM" route-map entry will be evaluated.
After each change of configuration the feature "restart" all bgp neighbors. Both IPv4 and IPv6 neighbors restart in the soft mode.
To change a list of prefixes inside of each record, you can apply the new list of prefixes with the same key. bgpcfgd will update corresponding prefix-lists automatically.
- Why I did it
I implemented the new feature. See the description above.
- How I did it
I introduced a new functionality into bgpcfgd. All code mostly isolated inside of BGPAllowListMgr class. Also I made some changes inside of bgp jinja2 templates.
- How to verify it
sonic-cfggen -j test_schema.conf --write-to-db. After that you can useshow runningconfiguration bgpto check the changed configuration. Also take a look into /var/log/syslog. There you can find the following output:After that you can use
show runningconfiguration bgpto check the changed configuration.Test coverage data
- Which release branch to backport (provide reason below if selected)
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)