bgpd: When static default route is present in RIB and advertised to p… #6184
Conversation
There was a problem hiding this comment.
Thanks for your contribution to FRR!
- One of your commits does not have a blank line between the summary and body; this will break
git log --oneline
Click for style suggestions
To apply these suggestions:
curl -s https://gist.githubusercontent.com/polychaeta/c74d5f87058e5224aa073bd3b79a91fb/raw/8809ae410a57ea0094e63c9a86bec1312758d3d8/cr_6184_1586349266.diff | git apply
diff --git a/bgpd/bgp_updgrp_adv.c b/bgpd/bgp_updgrp_adv.c
index 3063b0ff3..4dd118f67 100644
--- a/bgpd/bgp_updgrp_adv.c
+++ b/bgpd/bgp_updgrp_adv.c
@@ -536,9 +536,8 @@ void bgp_adj_out_unset_subgroup(struct bgp_node *rn,
/* If default originate is enabled and the route is default
* route, do not send withdraw
*/
- if (CHECK_FLAG(subgrp->sflags,
- SUBGRP_STATUS_DEFAULT_ORIGINATE) &&
- is_default_prefix(&rn->p))
+ if (CHECK_FLAG(subgrp->sflags, SUBGRP_STATUS_DEFAULT_ORIGINATE)
+ && is_default_prefix(&rn->p))
return;
if (adj->attr && withdraw) {
@@ -650,9 +649,9 @@ void subgroup_announce_table(struct update_subgroup *subgrp,
* withdraw
*/
if (CHECK_FLAG(
- peer->af_flags[afi][safi],
- PEER_FLAG_DEFAULT_ORIGINATE) &&
- is_default_prefix(&rn->p))
+ peer->af_flags[afi][safi],
+ PEER_FLAG_DEFAULT_ORIGINATE)
+ && is_default_prefix(&rn->p))
break;
bgp_adj_out_unset_subgroup(
@@ -825,7 +824,8 @@ void subgroup_default_originate(struct update_subgroup *subgrp, int withdraw)
/* Remove the adjacency for the previously advertised
* default route
*/
- adj = adj_lookup(rn, subgrp,
+ adj = adj_lookup(
+ rn, subgrp,
BGP_ADDPATH_TX_ID_FOR_DEFAULT_ORIGINATE);
if (adj != NULL) {
/* Clean up previous advertisement. */
If you are a new contributor to FRR, please see our contributing guidelines.
|
LOGS |
|
Let's fix the subject line to actually represent the problem instead of it being the start of a statement |
ok will change the subject like to the issue observed |
There was a problem hiding this comment.
Thanks for your contribution to FRR!
Click for style suggestions
To apply these suggestions:
curl -s https://gist.githubusercontent.com/polychaeta/2d89b3fe85329f0de21dca60b2a030bf/raw/8809ae410a57ea0094e63c9a86bec1312758d3d8/cr_6184_1586350509.diff | git apply
diff --git a/bgpd/bgp_updgrp_adv.c b/bgpd/bgp_updgrp_adv.c
index 3063b0ff3..4dd118f67 100644
--- a/bgpd/bgp_updgrp_adv.c
+++ b/bgpd/bgp_updgrp_adv.c
@@ -536,9 +536,8 @@ void bgp_adj_out_unset_subgroup(struct bgp_node *rn,
/* If default originate is enabled and the route is default
* route, do not send withdraw
*/
- if (CHECK_FLAG(subgrp->sflags,
- SUBGRP_STATUS_DEFAULT_ORIGINATE) &&
- is_default_prefix(&rn->p))
+ if (CHECK_FLAG(subgrp->sflags, SUBGRP_STATUS_DEFAULT_ORIGINATE)
+ && is_default_prefix(&rn->p))
return;
if (adj->attr && withdraw) {
@@ -650,9 +649,9 @@ void subgroup_announce_table(struct update_subgroup *subgrp,
* withdraw
*/
if (CHECK_FLAG(
- peer->af_flags[afi][safi],
- PEER_FLAG_DEFAULT_ORIGINATE) &&
- is_default_prefix(&rn->p))
+ peer->af_flags[afi][safi],
+ PEER_FLAG_DEFAULT_ORIGINATE)
+ && is_default_prefix(&rn->p))
break;
bgp_adj_out_unset_subgroup(
@@ -825,7 +824,8 @@ void subgroup_default_originate(struct update_subgroup *subgrp, int withdraw)
/* Remove the adjacency for the previously advertised
* default route
*/
- adj = adj_lookup(rn, subgrp,
+ adj = adj_lookup(
+ rn, subgrp,
BGP_ADDPATH_TX_ID_FOR_DEFAULT_ORIGINATE);
if (adj != NULL) {
/* Clean up previous advertisement. */
If you are a new contributor to FRR, please see our contributing guidelines.
will verify with routmap and update results |
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
Continuous Integration Result: SUCCESSFULContinuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-11720/ This is a comment from an automated CI system. Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: |
Continuous Integration Result: SUCCESSFULContinuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-11721/ This is a comment from an automated CI system. Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: |
Please find the results with routemap configuration |
| if (CHECK_FLAG(subgrp->sflags, | ||
| SUBGRP_STATUS_DEFAULT_ORIGINATE) && | ||
| is_default_prefix(&rn->p)) | ||
| return; |
There was a problem hiding this comment.
If there is a default route that needs to be withdrawn even when default originate config is present this check will avoid it and return from here. This may lead to default route with peer.
There was a problem hiding this comment.
Also if we have default-originate before and then default-originate with route-map option is given to match on some prefix which is not present then already advertised default route needs to be withdrawn and that will not happen due to above check.
There was a problem hiding this comment.
default route advertisement using default originate is handled seperately in subgroup_default_update_packet() and deletion in subgroup_default_withdraw_packet()
When default route is advertised previously using redistribute command and then default originate is added, it is sufficient to clear the adj out entry since the newly advertised copy of default route will serve as implicit withdraw to the peer, the receiver will replace old copy with the new copy
Will check on the scenario involving routemap change
|
Also it does look like default route origination has issue here. This is what documentation says but if there is a static default route present and if we do redistribute static then end up advertising this route to peer and that does not seem right. Your fix is above this assumption and I guess we need to fix default route advertisement first. [no] neighbor PEER default-originate |
There was a problem hiding this comment.
Thanks for your contribution to FRR!
Click for style suggestions
To apply these suggestions:
curl -s https://gist.githubusercontent.com/polychaeta/4b6b39127ccd6dead84c1221ae6d43ee/raw/6e8b7d9ec89e57b385e4c40da731dc0129cc3811/cr_6184_1586514725.diff | git apply
diff --git a/bgpd/bgp_updgrp_adv.c b/bgpd/bgp_updgrp_adv.c
index a9b8992f3..005efe163 100644
--- a/bgpd/bgp_updgrp_adv.c
+++ b/bgpd/bgp_updgrp_adv.c
@@ -537,9 +537,8 @@ void bgp_adj_out_unset_subgroup(struct bgp_node *rn,
* route, do not send withdraw. This will prevent deletion of
* the default route at the peer.
*/
- if (CHECK_FLAG(subgrp->sflags,
- SUBGRP_STATUS_DEFAULT_ORIGINATE) &&
- is_default_prefix(&rn->p))
+ if (CHECK_FLAG(subgrp->sflags, SUBGRP_STATUS_DEFAULT_ORIGINATE)
+ && is_default_prefix(&rn->p))
return;
if (adj->attr && withdraw) {
@@ -653,9 +652,9 @@ void subgroup_announce_table(struct update_subgroup *subgrp,
* default originate
*/
if (CHECK_FLAG(
- peer->af_flags[afi][safi],
- PEER_FLAG_DEFAULT_ORIGINATE) &&
- is_default_prefix(&rn->p))
+ peer->af_flags[afi][safi],
+ PEER_FLAG_DEFAULT_ORIGINATE)
+ && is_default_prefix(&rn->p))
break;
bgp_adj_out_unset_subgroup(
@@ -825,14 +824,11 @@ void subgroup_default_originate(struct update_subgroup *subgrp, int withdraw)
for (pi = bgp_node_get_bgp_path_info(rn); pi;
pi = pi->next) {
if (CHECK_FLAG(pi->flags, BGP_PATH_SELECTED))
- if (subgroup_announce_check(rn, pi,
- subgrp,
- &rn->p,
- &attr))
- bgp_adj_out_set_subgroup(rn,
- subgrp,
- &attr,
- pi);
+ if (subgroup_announce_check(
+ rn, pi, subgrp, &rn->p,
+ &attr))
+ bgp_adj_out_set_subgroup(
+ rn, subgrp, &attr, pi);
}
}
} else {
@@ -858,7 +854,8 @@ void subgroup_default_originate(struct update_subgroup *subgrp, int withdraw)
/* Remove the adjacency for the previously advertised
* default route
*/
- adj = adj_lookup(rn, subgrp,
+ adj = adj_lookup(
+ rn, subgrp,
BGP_ADDPATH_TX_ID_FOR_DEFAULT_ORIGINATE);
if (adj != NULL) {
/* Clean up previous advertisement. */
If you are a new contributor to FRR, please see our contributing guidelines.
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
Continuous Integration Result: SUCCESSFULContinuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-11777/ This is a comment from an automated CI system. Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: |
There was a problem hiding this comment.
Thanks for your contribution to FRR!
Click for style suggestions
To apply these suggestions:
curl -s https://gist.githubusercontent.com/polychaeta/341b6a8ef891948519d4bd84cacd3fb1/raw/24b17e8b7572c4de353a00e27e7c023a84ae29d2/cr_6184_1586591148.diff | git apply
diff --git a/bgpd/bgp_updgrp_adv.c b/bgpd/bgp_updgrp_adv.c
index 3a15b673f..18e01759f 100644
--- a/bgpd/bgp_updgrp_adv.c
+++ b/bgpd/bgp_updgrp_adv.c
@@ -537,9 +537,8 @@ void bgp_adj_out_unset_subgroup(struct bgp_node *rn,
* route, do not send withdraw. This will prevent deletion of
* the default route at the peer.
*/
- if (CHECK_FLAG(subgrp->sflags,
- SUBGRP_STATUS_DEFAULT_ORIGINATE) &&
- is_default_prefix(&rn->p))
+ if (CHECK_FLAG(subgrp->sflags, SUBGRP_STATUS_DEFAULT_ORIGINATE)
+ && is_default_prefix(&rn->p))
return;
if (adj->attr && withdraw) {
@@ -653,9 +652,9 @@ void subgroup_announce_table(struct update_subgroup *subgrp,
* default originate
*/
if (CHECK_FLAG(
- peer->af_flags[afi][safi],
- PEER_FLAG_DEFAULT_ORIGINATE) &&
- is_default_prefix(&rn->p))
+ peer->af_flags[afi][safi],
+ PEER_FLAG_DEFAULT_ORIGINATE)
+ && is_default_prefix(&rn->p))
break;
bgp_adj_out_unset_subgroup(
@@ -820,19 +819,17 @@ void subgroup_default_originate(struct update_subgroup *subgrp, int withdraw)
/* If default route is present in the local RIB, advertise the
* route
*/
- rn = bgp_afi_node_lookup(bgp->rib[afi][safi], afi, safi, &p, NULL);
+ rn = bgp_afi_node_lookup(bgp->rib[afi][safi], afi, safi, &p,
+ NULL);
if (rn != NULL) {
for (pi = bgp_node_get_bgp_path_info(rn); pi;
pi = pi->next) {
if (CHECK_FLAG(pi->flags, BGP_PATH_SELECTED))
- if (subgroup_announce_check(rn, pi,
- subgrp,
- &rn->p,
- &attr))
- bgp_adj_out_set_subgroup(rn,
- subgrp,
- &attr,
- pi);
+ if (subgroup_announce_check(
+ rn, pi, subgrp, &rn->p,
+ &attr))
+ bgp_adj_out_set_subgroup(
+ rn, subgrp, &attr, pi);
}
}
} else {
@@ -858,7 +855,8 @@ void subgroup_default_originate(struct update_subgroup *subgrp, int withdraw)
/* Remove the adjacency for the previously advertised
* default route
*/
- adj = adj_lookup(rn, subgrp,
+ adj = adj_lookup(
+ rn, subgrp,
BGP_ADDPATH_TX_ID_FOR_DEFAULT_ORIGINATE);
if (adj != NULL) {
/* Clean up previous advertisement. */
If you are a new contributor to FRR, please see our contributing guidelines.
|
The current implementation is sending explicit withdraw when default originate is configured, therefore the route gets deleted from the peer. Fix detailsWhen default route is redistributed to bgp and then default originate is configured for peer, the default route with origin BGP_ORIGIN_IGP is advertised, seperate withdraw message is not sent as the new copy (BGP_ORIGIN_IGP) will replace old copy (BGP_ORIGIN_INCOMPLETE) at the peer When default originate is configured with routemap that evaluates to false (deny), default route will not be advertised. When default originate is deleted and if there is redistributed default route in local RIB, this will be advertised to peer (based on routemap conditions if configured) |
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: FailedCentOS 7 amd64 build: Failed (click for details)CentOS 7 amd64 build: No useful log foundSuccessful on other platforms/tests
Warnings Generated during build:Checkout code: Successful with additional warningsCentOS 7 amd64 build: Failed (click for details)CentOS 7 amd64 build: No useful log foundWarnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: |
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
There was a problem hiding this comment.
Thanks for your contribution to FRR!
Click for style suggestions
To apply these suggestions:
curl -s https://gist.githubusercontent.com/polychaeta/96e9ff3728bd5c7357c68c8b14c77fc9/raw/0f40e9e58bd4e2c4d17cb43afa382768b04f3ca1/cr_6184_1586598755.diff | git apply
diff --git a/bgpd/bgp_updgrp_adv.c b/bgpd/bgp_updgrp_adv.c
index cbd2695f5..ff2395c24 100644
--- a/bgpd/bgp_updgrp_adv.c
+++ b/bgpd/bgp_updgrp_adv.c
@@ -652,8 +652,8 @@ void subgroup_announce_table(struct update_subgroup *subgrp,
* default originate
*/
if (CHECK_FLAG(
- peer->af_flags[afi][safi],
- PEER_FLAG_DEFAULT_ORIGINATE)
+ peer->af_flags[afi][safi],
+ PEER_FLAG_DEFAULT_ORIGINATE)
&& is_default_prefix(&rn->p))
break;
@@ -855,8 +855,9 @@ void subgroup_default_originate(struct update_subgroup *subgrp, int withdraw)
/* Remove the adjacency for the previously advertised
* default route
*/
- adj = adj_lookup(rn, subgrp,
- BGP_ADDPATH_TX_ID_FOR_DEFAULT_ORIGINATE);
+ adj = adj_lookup(
+ rn, subgrp,
+ BGP_ADDPATH_TX_ID_FOR_DEFAULT_ORIGINATE);
if (adj != NULL) {
/* Clean up previous advertisement. */
if (adj->adv)
If you are a new contributor to FRR, please see our contributing guidelines.
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopology tests on Ubuntu 16.04 amd64: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPOU1604-11789/test Topology Tests failed for Topology tests on Ubuntu 16.04 amd64: see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-11789/artifact/TOPOU1604/ErrorLog/log_topotests.txt CentOS 7 rpm pkg check: Failed (click for details)CentOS 7 rpm pkg check: No useful log foundTopotest tests on Ubuntu 16.04 i386: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPOI386-11789/test Topology Tests failed for Topotest tests on Ubuntu 16.04 i386: see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-11789/artifact/TOPOI386/ErrorLog/log_topotests.txt Topology tests on Ubuntu 18.04 amd64: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPOU1804-11789/test Topology Tests failed for Topology tests on Ubuntu 18.04 amd64: see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-11789/artifact/TOPOU1804/ErrorLog/log_topotests.txt Successful on other platforms/tests
Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: |
|
multipath test logs |
We should be consistent in our behavior -- the order in which things are configured should not matter to the final state (as much as possible, given BGP already has the idiosyncrasies)... Further, removing If I understand correctly, these are the two targets of this PR, and if so, I agree with the aim. :-) I would like to see testing showing this is what happens, though, if anyone has time to bring it up and show the changes from a peer's perspective. :-) I agree with @ton31337 that we probably need a test case for this. I've not looked at the documentation, but I don't think it mentions any of this stuff, so it's probably "okay." I'm okay with the code, but I see others have comments out, so ... |
Verified that default originate gets precedence over redistribute and peer starts advertising redistribute when default originate is deleted. Will add test case as suggested |
Saw the test results. I see that cases are tested as below.
The comment on stale default would remain with peer may be caused with below steps. Can you please test this?
So in this case we should withdraw default route advertised due to redistribution. |
The routemap deny case is verified as mentioned in the logs below
|
There was a problem hiding this comment.
Your commit short summary is still too long:
http://docs.frrouting.org/projects/dev-guide/en/latest/workflow.html#commit-messages
There was a problem hiding this comment.
Your commit short summary is still too long:
http://docs.frrouting.org/projects/dev-guide/en/latest/workflow.html#commit-messages
There was a problem hiding this comment.
Thanks for your contribution to FRR!
Click for style suggestions
To apply these suggestions:
curl -s https://gist.githubusercontent.com/polychaeta/54a089e33201c7c1307ac8224f1ec989/raw/9b91dbf6e289c45e8fad42326d1d24a122c8eb75/cr_6184_1587493933.diff | git apply
diff --git a/bgpd/bgp_updgrp_adv.c b/bgpd/bgp_updgrp_adv.c
index 2f220be51..e83eed6e3 100644
--- a/bgpd/bgp_updgrp_adv.c
+++ b/bgpd/bgp_updgrp_adv.c
@@ -842,13 +842,14 @@ void subgroup_default_originate(struct update_subgroup *subgrp, int withdraw)
/* Remove the adjacency for the previously
* advertised default route
*/
- adj = adj_lookup(rn, subgrp,
- BGP_ADDPATH_TX_ID_FOR_DEFAULT_ORIGINATE);
+ adj = adj_lookup(
+ rn, subgrp,
+ BGP_ADDPATH_TX_ID_FOR_DEFAULT_ORIGINATE);
if (adj != NULL) {
/* Clean up previous advertisement. */
if (adj->adv)
bgp_advertise_clean_subgroup(
- subgrp, adj);
+ subgrp, adj);
/* Remove from adjacency. */
RB_REMOVE(bgp_adj_out_rb, &rn->adj_out,
If you are a new contributor to FRR, please see our contributing guidelines.
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
Continuous Integration Result: SUCCESSFULContinuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-11977/ This is a comment from an automated CI system. Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: |
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-11983/ This is a comment from an automated CI system. Warnings Generated during build:Checkout code: Successful with additional warningsWarnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: |
Updated the commit short summary |
Issue: Configuring default-originate when static default route is previously advertised results in withdrawal of the route. Fix : Delete the adj-out entry for the previously advertised static default route without sending explicit withdraw message. Signed-off-by: kssoman <somanks@gmail.com>
There was a problem hiding this comment.
Thanks for your contribution to FRR!
Click for style suggestions
To apply these suggestions:
curl -s https://gist.githubusercontent.com/polychaeta/1dec4a31b28f28b0e91d009458ce3557/raw/83d3cd1f348d1234ce3e4126e5c61355025965fb/cr_6184_1587575098.diff | git apply
diff --git a/bgpd/bgp_updgrp_adv.c b/bgpd/bgp_updgrp_adv.c
index 6399bc93a..e83eed6e3 100644
--- a/bgpd/bgp_updgrp_adv.c
+++ b/bgpd/bgp_updgrp_adv.c
@@ -843,8 +843,8 @@ void subgroup_default_originate(struct update_subgroup *subgrp, int withdraw)
* advertised default route
*/
adj = adj_lookup(
- rn, subgrp,
- BGP_ADDPATH_TX_ID_FOR_DEFAULT_ORIGINATE);
+ rn, subgrp,
+ BGP_ADDPATH_TX_ID_FOR_DEFAULT_ORIGINATE);
if (adj != NULL) {
/* Clean up previous advertisement. */
if (adj->adv)
If you are a new contributor to FRR, please see our contributing guidelines.
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-12000/ This is a comment from an automated CI system. Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: |
lol now polchaeta is unhappy. for future help, you might want to use the clang format tool and then you wont have to worry about polychaeta again. Its pretty easy to set up with git. http://docs.frrouting.org/projects/dev-guide/en/latest/workflow.html#code-formatting |
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
Applying causes checkpatch.sh to give error (line > 80 characters) |
Tried with git clang-format line 847 : BGP_ADDPATH_TX_ID_FOR_DEFAULT_ORIGINATE); We can probably ignore clang format message |
…eer,
Signed-off-by: kssoman somanks@gmail.com