Merge pull request #8494 from donaldsharp/wfi_failures

bgpd, tests: Add code to handle failed installations

bgpd/bgp_route.c

@@ -2627,9 +2627,14 @@ void subgroup_process_announce_selected(struct update_subgroup *subgrp,
 			/* Route is selected, if the route is already installed
 			 * in FIB, then it is advertised
 			 */
-			if (advertise)
+			if (advertise) {
-				bgp_adj_out_set_subgroup(dest, subgrp, &attr,
+				if (!bgp_check_withdrawal(bgp, dest))
-							 selected);
+					bgp_adj_out_set_subgroup(
+						dest, subgrp, &attr, selected);
+				else
+					bgp_adj_out_unset_subgroup(
+						dest, subgrp, 1, addpath_tx_id);
+			}
 		} else
 			bgp_adj_out_unset_subgroup(dest, subgrp, 1,
 						   addpath_tx_id);
@@ -2908,6 +2913,11 @@ static void bgp_process_main_one(struct bgp *bgp, struct bgp_dest *dest,
 			if (bgp_fibupd_safi(safi)
 			    && !bgp_option_check(BGP_OPT_NO_FIB)) {
 
+				if (BGP_SUPPRESS_FIB_ENABLED(bgp)
+				    && new_select->sub_type == BGP_ROUTE_NORMAL)
+					SET_FLAG(dest->flags,
+						 BGP_NODE_FIB_INSTALL_PENDING);
+
 				if (new_select->type == ZEBRA_ROUTE_BGP
 				    && (new_select->sub_type == BGP_ROUTE_NORMAL
 					|| new_select->sub_type
@@ -3007,11 +3017,16 @@ static void bgp_process_main_one(struct bgp *bgp, struct bgp_dest *dest,
 	/* FIB update. */
 	if (bgp_fibupd_safi(safi) && (bgp->inst_type != BGP_INSTANCE_TYPE_VIEW)
 	    && !bgp_option_check(BGP_OPT_NO_FIB)) {
+
 		if (new_select && new_select->type == ZEBRA_ROUTE_BGP
 		    && (new_select->sub_type == BGP_ROUTE_NORMAL
 			|| new_select->sub_type == BGP_ROUTE_AGGREGATE
 			|| new_select->sub_type == BGP_ROUTE_IMPORTED)) {
 
+			if (BGP_SUPPRESS_FIB_ENABLED(bgp))
+				SET_FLAG(dest->flags,
+					 BGP_NODE_FIB_INSTALL_PENDING);
+
 			/* if this is an evpn imported type-5 prefix,
 			 * we need to withdraw the route first to clear
 			 * the nh neigh and the RMAC entry.
@@ -8336,6 +8351,7 @@ void bgp_redistribute_add(struct bgp *bgp, struct prefix *p,
 		bgp_aggregate_increment(bgp, p, new, afi, SAFI_UNICAST);
 		bgp_path_info_add(bn, new);
 		bgp_dest_unlock_node(bn);
+		SET_FLAG(bn->flags, BGP_NODE_FIB_INSTALLED);
 		bgp_process(bgp, bn, afi, SAFI_UNICAST);
 
 		if ((bgp->inst_type == BGP_INSTANCE_TYPE_VRF)

bgpd/bgp_route.h

@@ -587,6 +587,33 @@ static inline bool bgp_check_advertise(struct bgp *bgp, struct bgp_dest *dest)
 		 (!bgp_option_check(BGP_OPT_NO_FIB))));
 }
 
+/*
+ * If we have a fib result and it failed to install( or was withdrawn due
+ * to better admin distance we need to send down the wire a withdrawal.
+ * This function assumes that bgp_check_advertise was already returned
+ * as good to go.
+ */
+static inline bool bgp_check_withdrawal(struct bgp *bgp, struct bgp_dest *dest)
+{
+	struct bgp_path_info *pi;
+
+	if (!BGP_SUPPRESS_FIB_ENABLED(bgp))
+		return false;
+
+	for (pi = bgp_dest_get_bgp_path_info(dest); pi; pi = pi->next) {
+		if (CHECK_FLAG(pi->flags, BGP_PATH_SELECTED))
+			continue;
+
+		if (pi->sub_type != BGP_ROUTE_NORMAL)
+			return true;
+	}
+
+	if (CHECK_FLAG(dest->flags, BGP_NODE_FIB_INSTALLED))
+		return false;
+
+	return true;
+}
+
 /* called before bgp_process() */
 DECLARE_HOOK(bgp_process,
 	     (struct bgp * bgp, afi_t afi, safi_t safi, struct bgp_dest *bn,

bgpd/bgp_updgrp_adv.c

@@ -686,11 +686,21 @@ void subgroup_announce_table(struct update_subgroup *subgrp,
 							    dest_p, &attr,
 							    false)) {
 					/* Check if route can be advertised */
-					if (advertise)
+					if (advertise) {
-						bgp_adj_out_set_subgroup(dest,
+						if (!bgp_check_withdrawal(bgp,
-									 subgrp,
+									  dest))
-									 &attr,
+							bgp_adj_out_set_subgroup(
-									 ri);
+								dest, subgrp,
+								&attr, ri);
+						else
+							bgp_adj_out_unset_subgroup(
+								dest, subgrp, 1,
+								bgp_addpath_id_for_peer(
+									peer,
+									afi,
+									safi,
+									&ri->tx_addpath));
+					}
 				} else {
 					/* If default originate is enabled for
 					 * the peer, do not send explicit

bgpd/bgp_zebra.c

@@ -2528,35 +2528,27 @@ static int bgp_zebra_route_notify_owner(int command, struct zclient *zclient,
 	case ZAPI_ROUTE_INSTALLED:
 		new_select = NULL;
 		/* Clear the flags so that route can be processed */
-		if (CHECK_FLAG(dest->flags,
+		UNSET_FLAG(dest->flags, BGP_NODE_FIB_INSTALL_PENDING);
-			       BGP_NODE_FIB_INSTALL_PENDING)) {
-			UNSET_FLAG(dest->flags,
-				   BGP_NODE_FIB_INSTALL_PENDING);
 		SET_FLAG(dest->flags, BGP_NODE_FIB_INSTALLED);
 		if (BGP_DEBUG(zebra, ZEBRA))
 			zlog_debug("route %pRN : INSTALLED", dest);
 		/* Find the best route */
 		for (pi = dest->info; pi; pi = pi->next) {
 			/* Process aggregate route */
-				bgp_aggregate_increment(bgp, &p, pi,
+			bgp_aggregate_increment(bgp, &p, pi, afi, safi);
-							afi, safi);
+			if (CHECK_FLAG(pi->flags, BGP_PATH_SELECTED))
-				if (CHECK_FLAG(pi->flags,
-					       BGP_PATH_SELECTED))
 				new_select = pi;
 		}
 		/* Advertise the route */
 		if (new_select)
-				group_announce_route(bgp, afi, safi,
+			group_announce_route(bgp, afi, safi, dest, new_select);
-						     dest, new_select);
 		else {
 			flog_err(EC_BGP_INVALID_ROUTE,
-					 "selected route %pRN not found",
+				 "selected route %pRN not found", dest);
-					 dest);
 
 			bgp_dest_unlock_node(dest);
 			return -1;
 		}
-		}
 		break;
 	case ZAPI_ROUTE_REMOVED:
 		/* Route deleted from dataplane, reset the installed flag
@@ -2568,15 +2560,34 @@ static int bgp_zebra_route_notify_owner(int command, struct zclient *zclient,
 			zlog_debug("route %pRN: Removed from Fib", dest);
 		break;
 	case ZAPI_ROUTE_FAIL_INSTALL:
+		new_select = NULL;
 		if (BGP_DEBUG(zebra, ZEBRA))
 			zlog_debug("route: %pRN Failed to Install into Fib",
 				   dest);
+		UNSET_FLAG(dest->flags, BGP_NODE_FIB_INSTALL_PENDING);
+		UNSET_FLAG(dest->flags, BGP_NODE_FIB_INSTALLED);
+		for (pi = bgp_dest_get_bgp_path_info(dest); pi; pi = pi->next) {
+			if (CHECK_FLAG(pi->flags, BGP_PATH_SELECTED))
+				new_select = pi;
+		}
+		if (new_select)
+			group_announce_route(bgp, afi, safi, dest, new_select);
 		/* Error will be logged by zebra module */
 		break;
 	case ZAPI_ROUTE_BETTER_ADMIN_WON:
 		if (BGP_DEBUG(zebra, ZEBRA))
 			zlog_debug("route: %pRN removed due to better admin won",
 				   dest);
+		new_select = NULL;
+		UNSET_FLAG(dest->flags, BGP_NODE_FIB_INSTALL_PENDING);
+		UNSET_FLAG(dest->flags, BGP_NODE_FIB_INSTALLED);
+		for (pi = bgp_dest_get_bgp_path_info(dest); pi; pi = pi->next) {
+			bgp_aggregate_decrement(bgp, &p, pi, afi, safi);
+			if (CHECK_FLAG(pi->flags, BGP_PATH_SELECTED))
+				new_select = pi;
+		}
+		if (new_select)
+			group_announce_route(bgp, afi, safi, dest, new_select);
 		/* No action required */
 		break;
 	case ZAPI_ROUTE_REMOVE_FAIL:

tests/topotests/bgp_suppress_fib/r1/bgp_ipv4_allowas.json

@@ -0,0 +1,40 @@
+{
+  "prefix":"192.168.1.1/32",
+  "paths":[
+    {
+      "aspath":{
+        "string":"2",
+        "segments":[
+          {
+            "type":"as-sequence",
+            "list":[
+              2
+            ]
+          }
+        ],
+        "length":1
+      },
+      "origin":"incomplete",
+      "metric":0,
+      "valid":true,
+      "bestpath":{
+        "overall":true,
+        "selectionReason":"First path received"
+      },
+      "nexthops":[
+        {
+          "ip":"10.0.0.2",
+          "afi":"ipv4",
+          "metric":0,
+          "accessible":true,
+          "used":true
+        }
+      ],
+      "peer":{
+        "peerId":"10.0.0.2",
+        "routerId":"10.0.0.9",
+        "type":"external"
+      }
+    }
+  ]
+}

tests/topotests/bgp_suppress_fib/r2/bgp_ipv4_allowas.json

@@ -0,0 +1,68 @@
+{
+  "prefix":"192.168.1.1/32",
+  "paths":[
+    {
+      "aspath":{
+        "string":"1 2",
+        "segments":[
+          {
+            "type":"as-sequence",
+            "list":[
+              1,
+              2
+            ]
+          }
+        ],
+        "length":2
+      },
+      "origin":"incomplete",
+      "valid":true,
+      "fibInstalled":true,
+      "nexthops":[
+        {
+          "ip":"10.0.0.1",
+          "afi":"ipv4",
+          "metric":0,
+          "accessible":true,
+          "used":true
+        }
+      ],
+      "peer":{
+        "peerId":"10.0.0.1",
+        "routerId":"10.0.0.1",
+        "type":"external"
+      }
+    },
+    {
+      "aspath":{
+        "string":"Local",
+        "segments":[
+        ],
+        "length":0
+      },
+      "origin":"incomplete",
+      "metric":0,
+	"weight":32768,
+	      "valid":true,
+      "sourced":true,
+      "bestpath":{
+        "overall":true,
+        "selectionReason":"Weight"
+      },
+      "fibInstalled":true,
+      "nexthops":[
+        {
+          "ip":"10.0.0.10",
+          "afi":"ipv4",
+          "metric":0,
+          "accessible":true,
+          "used":true
+        }
+      ],
+      "peer":{
+        "peerId":"0.0.0.0",
+        "routerId":"10.0.0.9"
+      }
+    }
+  ]
+}

tests/topotests/bgp_suppress_fib/r2/bgpd.allowas_in.conf

@@ -0,0 +1,18 @@
+access-list access seq 10 permit 192.168.1.1/32
+!
+ip route 192.168.1.1/32 10.0.0.10
+!
+debug bgp bestpath
+debug bgp nht
+debug bgp updates
+debug bgp update-groups
+debug bgp zebra
+debug zebra rib detail
+!
+router bgp 2
+  address-family ipv4 uni
+     redistribute static
+     neighbor 10.0.0.10 allowas-in 1
+     neighbor 10.0.0.1 allowas-in 1
+  !
+!

tests/topotests/bgp_suppress_fib/r2/bgpd.conf

@@ -1,3 +1,6 @@
+debug bgp updates
+debug bgp bestpath 40.0.0.0/8
+debug bgp zebra
 !
 router bgp 2
   no bgp ebgp-requires-policy

tests/topotests/bgp_suppress_fib/r2/no_bgp_ipv4_allowas.json

@@ -0,0 +1 @@
+{}

tests/topotests/bgp_suppress_fib/r2/v4_override.json

@@ -0,0 +1,20 @@
+{
+  "40.0.0.0\/8":[
+    {
+      "prefix":"40.0.0.0\/8",
+      "protocol":"static",
+      "vrfName":"default",
+      "selected":true,
+      "destSelected":true,
+      "installed":true,
+      "nexthops":[
+        {
+          "fib":true,
+          "ip":"10.0.0.10",
+          "afi":"ipv4",
+          "active":true
+        }
+      ]
+    }
+  ]
+}

tests/topotests/bgp_suppress_fib/r3/v4_override.json

@@ -0,0 +1,4 @@
+{
+  "0.0.0.0\/0":[
+  ]
+}

tests/topotests/bgp_suppress_fib/test_bgp_suppress_fib.py

@@ -29,6 +29,7 @@ import json
 import pytest
 from functools import partial
 from time import sleep
+from lib.topolog import logger
 
 CWD = os.path.dirname(os.path.realpath(__file__))
 sys.path.append(os.path.join(CWD, "../"))
@@ -112,6 +113,115 @@ def test_bgp_route():
     assert result is None, assertmsg
 
 
+def test_bgp_better_admin_won():
+    "A better Admin distance protocol may come along and knock us out"
+
+    tgen = get_topogen()
+
+    if tgen.routers_have_failure():
+        pytest.skip(tgen.errors)
+
+    r2 = tgen.gears["r2"]
+    r2.vtysh_cmd("conf\nip route 40.0.0.0/8 10.0.0.10")
+
+    json_file = "{}/r2/v4_override.json".format(CWD)
+    expected = json.loads(open(json_file).read())
+
+    logger.info(expected)
+    test_func = partial(
+        topotest.router_json_cmp, r2, "show ip route 40.0.0.0 json", expected
+    )
+
+    _, result = topotest.run_and_expect(test_func, None, count=10, wait=0.5)
+    assertmsg = '"r2" static route did not take over'
+    assert result is None, assertmsg
+
+    r3 = tgen.gears["r3"]
+
+    json_file = "{}/r3/v4_override.json".format(CWD)
+    expected = json.loads(open(json_file).read())
+
+    test_func = partial(
+        topotest.router_json_cmp, r3, "show ip route 40.0.0.0 json", expected
+    )
+
+    _, result = topotest.run_and_expect(test_func, None, count=10, wait=0.5)
+    assertmsg = '"r3" route to 40.0.0.0 should have been lost'
+    assert result is None, assertmsg
+
+    r2.vtysh_cmd("conf\nno ip route 40.0.0.0/8 10.0.0.10")
+
+    json_file = "{}/r3/v4_route.json".format(CWD)
+    expected = json.loads(open(json_file).read())
+
+    test_func = partial(
+        topotest.router_json_cmp,
+        r3,
+        "show ip route 40.0.0.0 json",
+        expected,
+    )
+    _, result = topotest.run_and_expect(test_func, None, count=10, wait=0.5)
+    assertmsg = '"r3" route to 40.0.0.0 did not come back'
+    assert result is None, assertmsg
+
+
+def test_bgp_allow_as_in():
+    tgen = get_topogen()
+
+    if tgen.routers_have_failure():
+        pytest.skip(tgen.errors)
+
+    r2 = tgen.gears["r2"]
+
+    config_file = "{}/r2/bgpd.allowas_in.conf".format(CWD)
+    r2.run("vtysh -f {}".format(config_file))
+
+    json_file = "{}/r2/bgp_ipv4_allowas.json".format(CWD)
+    expected = json.loads(open(json_file).read())
+
+    test_func = partial(
+        topotest.router_json_cmp,
+        r2,
+        "show bgp ipv4 uni 192.168.1.1/32 json",
+        expected,
+    )
+    _, result = topotest.run_and_expect(test_func, None, count=10, wait=0.5)
+    assertmsg = '"r2" static redistribution failed into bgp'
+    assert result is None, assertmsg
+
+    r1 = tgen.gears["r1"]
+
+    json_file = "{}/r1/bgp_ipv4_allowas.json".format(CWD)
+    expected = json.loads(open(json_file).read())
+
+    test_func = partial(
+        topotest.router_json_cmp,
+        r1,
+        "show bgp ipv4 uni 192.168.1.1/32 json",
+        expected,
+    )
+
+    _, result = topotest.run_and_expect(test_func, None, count=10, wait=0.5)
+    assertmsg = '"r1" 192.168.1.1/32 route should have arrived'
+    assert result is None, assertmsg
+
+    r2.vtysh_cmd("conf\nno ip route 192.168.1.1/32 10.0.0.10")
+
+    json_file = "{}/r2/no_bgp_ipv4_allowas.json".format(CWD)
+    expected = json.loads(open(json_file).read())
+
+    test_func = partial(
+        topotest.router_json_cmp,
+        r2,
+        "show bgp ipv4 uni 192.168.1.1/32 json",
+        expected,
+    )
+
+    _, result = topotest.run_and_expect(test_func, None, count=10, wait=0.5)
+    assertmsg = '"r2" 192.168.1.1/32 route should be gone'
+    assert result is None, assertmsg
+
+
 if __name__ == "__main__":
     args = ["-s"] + sys.argv[1:]
     sys.exit(pytest.main(args))