-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
L3VPN exportation by using 'route-map vpn export' command instead of 'rt vpn export' command #14512
L3VPN exportation by using 'route-map vpn export' command instead of 'rt vpn export' command #14512
Conversation
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.
qq: How would the exportation be accomplished if let's say neither rt vpn export ...
is configured, nor route-map vpn export ...
matches any criteria? Both commands are mutually exclusive or how it works?
bgpd/bgp_routemap.c
Outdated
for (index = map->head; index; index = index->next) { | ||
for (set = index->set_list.head; set; set = set->next) { | ||
if (set->cmd && set->cmd->str && | ||
strmatch(set->cmd->str, "extcommunity rt")) |
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.
Not sure I understand this logic here. Don't say it's wrong, but in the long-term, we should have something like a custom enum for distinguishing a specific command for struct routemap_hook_context
.
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.
Today, the framework is only available for delete.
And if it had to be done, today, the framework does not distinguish between create and modify events.
that is why I chose this method.
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.
I thought this would allow the operator to change the RT, not just delete it, right?
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopotests Ubuntu 18.04 i386 part 9: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO9U18I386-14408/test Topology Tests failed for Topotests Ubuntu 18.04 i386 part 9 Successful on other platforms/tests
|
25a6cf0
to
131d8c3
Compare
ci:rerun |
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.
How would the exportation be accomplished if let's say neither rt vpn export ...
is configured, nor route-map vpn export ...
matches any criteria? Both commands are mutually exclusive or how it works?
In general, I don't understand the whole idea of this (what is the real gain). In both ways, you have to configure the RT list. Or do you mean that on specific criteria, you can set different RT export lists?
|
Could it also be documented? |
131d8c3
to
38d55bb
Compare
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
38d55bb
to
77b0534
Compare
77b0534
to
ef49b01
Compare
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.
a couple of minor comments ... I'm still a little confused by the use case here :-)
bgpd/bgp_routemap.c
Outdated
for (index = map->head; index; index = index->next) { | ||
for (set = index->set_list.head; set; set = set->next) { | ||
if (set->cmd && set->cmd->str && | ||
strmatch(set->cmd->str, "extcommunity rt")) |
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.
I thought this would allow the operator to change the RT, not just delete it, right?
ef49b01
to
14868b6
Compare
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.
looks good
The following route-map set rules events are destroyed with the 'match_destroy' API whereas there is a 'set_destroy' API available. Fix this for the following set commands: > set distance > set extcommunity rt > set extcommunity nt > set extcommunity color > set extcommunity soo Fixes: 48cb7ea ("bgpd: North-bound implementation for bgp rmaps") Fixes: c9a2561 ("bgpd: Implement Node Target Extended Communities") Fixes: b80ebc2 ("bgpd: add colored extended communities support") Signed-off-by: Philippe Guibert <[email protected]>
When exporting BGP prefixes, it is necessary to configure the route target extended communities with the following command: > rt vpn export <RouteTarget> But the customer may need to configure the route-target to apply to bgp updates, solely based on a route-map criterium. by using the below route-map configured like that: > route-map vpn export <routemapname> Fix this by allowing to export bgp updates based on the presence of route-targets on either route-map or vpn configured rt. the exportation process is stopped if no route target is available in the ecommunity list. Fixes: ddb5b48 ("bgpd: vpn-vrf route leaking") Signed-off-by: Philippe Guibert <[email protected]>
The prefixes unexportation triggers an attempt to create the VPN prefix node if that prefix was not already present. For instance, if a given prefix is not exported because of a route-map filtering, the withdraw process will try to create the node with the 'bgp_afi_node_get()' command. Fix this by replacing this call by the 'bgp_safi_node_lookup()' function. Fixes: ddb5b48 ("bgpd: vpn-vrf route leaking") Signed-off-by: Philippe Guibert <[email protected]>
Add a test to check that the presence of a route-map at exportation with a 'set extcommunity rt' is enough to allow the prefix to be exported. Signed-off-by: Philippe Guibert <[email protected]>
Explain that an export route target list can be configured alternatively by using route-maps. Signed-off-by: Philippe Guibert <[email protected]>
A route-map can be programmed to remove the route-target which has been set with 'rt vpn export' command, but fails to remove it. Fix this by applying the route-map, then considering the resulting extended community-list. Add some tests to catch this issue. Signed-off-by: Philippe Guibert <[email protected]>
14868b6
to
fc1177f
Compare
This pull request introduces a few fixes related to route-map and l3vpn usage.
This pull request also proposes to facilitate the L3VPN prefixes exportation by not solely relying on the 'rt vpn export ' command. That is to say that a route-map usage can also be used.