bgpd: fix various problems with hold/keepalive timers

Problem reported that we weren't adjusting the keepalive timer
correctly when we negotiated a lower hold time learned from a
peer.  While working on this, found we didn't do inheritance
correctly at all.  This fix solves the first problem and also
ensures that the timers are configured correctly based on this
priority order - peer defined > peer-group defined > global config.
This fix also displays the timers as "configured" regardless of
which of the three locations above is used.

Ticket: CM-18408
Signed-off-by: Don Slice <dslice@cumulusnetworks.com>
Reviewed-by: CCR-6807
Testing-performed:  Manual testing successful, fix tested by
submitter, bgp-smoke completed successfully

6 files changed, 76 insertions, 30 deletions

diff --git a/bgpd/bgp_fsm.c b/bgpd/bgp_fsm.c
index b609abac6..aa350d3dd 100644
--- a/bgpd/bgp_fsm.c
+++ b/bgpd/bgp_fsm.c
@@ -1096,7 +1096,7 @@ int bgp_stop(struct peer *peer)
 		}
 
 	/* Reset keepalive and holdtime */
-	if (CHECK_FLAG(peer->config, PEER_CONFIG_TIMER)) {
+	if (PEER_OR_GROUP_TIMER_SET(peer)) {
 		peer->v_keepalive = peer->keepalive;
 		peer->v_holdtime = peer->holdtime;
 	} else {
diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c
index 003fbaff6..a545162f8 100644
--- a/bgpd/bgp_packet.c
+++ b/bgpd/bgp_packet.c
@@ -529,7 +529,7 @@ void bgp_open_send(struct peer *peer)
 	u_int16_t send_holdtime;
 	as_t local_as;
 
-	if (CHECK_FLAG(peer->config, PEER_CONFIG_TIMER))
+	if (PEER_OR_GROUP_TIMER_SET(peer))
 		send_holdtime = peer->holdtime;
 	else
 		send_holdtime = peer->bgp->default_holdtime;
@@ -1087,7 +1087,7 @@ static int bgp_open_receive(struct peer *peer, bgp_size_t size)
 	   implementation MAY adjust the rate at which it sends KEEPALIVE
 	   messages as a function of the Hold Time interval. */
 
-	if (CHECK_FLAG(peer->config, PEER_CONFIG_TIMER))
+	if (PEER_OR_GROUP_TIMER_SET(peer))
 		send_holdtime = peer->holdtime;
 	else
 		send_holdtime = peer->bgp->default_holdtime;
@@ -1097,7 +1097,8 @@ static int bgp_open_receive(struct peer *peer, bgp_size_t size)
 	else
 		peer->v_holdtime = send_holdtime;
 
-	if (CHECK_FLAG(peer->config, PEER_CONFIG_TIMER))
+	if ((PEER_OR_GROUP_TIMER_SET(peer))
+	    && (peer->keepalive < peer->v_holdtime / 3))
 		peer->v_keepalive = peer->keepalive;
 	else
 		peer->v_keepalive = peer->v_holdtime / 3;
diff --git a/bgpd/bgp_snmp.c b/bgpd/bgp_snmp.c
index 8a9d61f30..484ea7c43 100644
--- a/bgpd/bgp_snmp.c
+++ b/bgpd/bgp_snmp.c
@@ -615,14 +615,14 @@ static u_char *bgpPeerTable(struct variable *v, oid name[], size_t *length,
 		break;
 	case BGPPEERHOLDTIMECONFIGURED:
 		*write_method = write_bgpPeerTable;
-		if (CHECK_FLAG(peer->config, PEER_CONFIG_TIMER))
+		if (PEER_OR_GROUP_TIMER_SET(peer))
 			return SNMP_INTEGER(peer->holdtime);
 		else
 			return SNMP_INTEGER(peer->v_holdtime);
 		break;
 	case BGPPEERKEEPALIVECONFIGURED:
 		*write_method = write_bgpPeerTable;
-		if (CHECK_FLAG(peer->config, PEER_CONFIG_TIMER))
+		if (PEER_OR_GROUP_TIMER_SET(peer))
 			return SNMP_INTEGER(peer->keepalive);
 		else
 			return SNMP_INTEGER(peer->v_keepalive);
diff --git a/bgpd/bgp_vty.c b/bgpd/bgp_vty.c
index 749c9d25d..84dae9b9b 100644
--- a/bgpd/bgp_vty.c
+++ b/bgpd/bgp_vty.c
@@ -8272,7 +8272,7 @@ static void bgp_show_peer(struct vty *vty, struct peer *p, u_char use_json,
 				    "bgpTimerKeepAliveIntervalMsecs",
 				    p->v_keepalive * 1000);
 
-		if (CHECK_FLAG(p->config, PEER_CONFIG_TIMER)) {
+		if (PEER_OR_GROUP_TIMER_SET(p)) {
 			json_object_int_add(json_neigh,
 					    "bgpTimerConfiguredHoldTimeMsecs",
 					    p->holdtime * 1000);
@@ -8280,6 +8280,16 @@ static void bgp_show_peer(struct vty *vty, struct peer *p, u_char use_json,
 				json_neigh,
 				"bgpTimerConfiguredKeepAliveIntervalMsecs",
 				p->keepalive * 1000);
+		} else if ((bgp->default_holdtime != BGP_DEFAULT_HOLDTIME)
+			   || (bgp->default_keepalive !=
+			       BGP_DEFAULT_KEEPALIVE)) {
+			json_object_int_add(json_neigh,
+					    "bgpTimerConfiguredHoldTimeMsecs",
+					    bgp->default_holdtime);
+			json_object_int_add(
+				json_neigh,
+				"bgpTimerConfiguredKeepAliveIntervalMsecs",
+				bgp->default_keepalive);
 		}
 	} else {
 		/* Administrative shutdown. */
@@ -8326,11 +8336,18 @@ static void bgp_show_peer(struct vty *vty, struct peer *p, u_char use_json,
 		vty_out(vty,
 			"  Hold time is %d, keepalive interval is %d seconds\n",
 			p->v_holdtime, p->v_keepalive);
-		if (CHECK_FLAG(p->config, PEER_CONFIG_TIMER)) {
+		if (PEER_OR_GROUP_TIMER_SET(p)) {
 			vty_out(vty, "  Configured hold time is %d",
 				p->holdtime);
 			vty_out(vty, ", keepalive interval is %d seconds\n",
 				p->keepalive);
+		} else if ((bgp->default_holdtime != BGP_DEFAULT_HOLDTIME)
+			   || (bgp->default_keepalive !=
+			       BGP_DEFAULT_KEEPALIVE)) {
+			vty_out(vty, "  Configured hold time is %d",
+				bgp->default_holdtime);
+			vty_out(vty, ", keepalive interval is %d seconds\n",
+				bgp->default_keepalive);
 		}
 	}
 	/* Capability. */
diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c
index a39ff3b27..345ceab9e 100644
--- a/bgpd/bgpd.c
+++ b/bgpd/bgpd.c
@@ -2301,6 +2301,7 @@ struct peer_group *peer_group_get(struct bgp *bgp, const char *name)
 	group->conf->gtsm_hops = 0;
 	group->conf->v_routeadv = BGP_DEFAULT_EBGP_ROUTEADV;
 	UNSET_FLAG(group->conf->config, PEER_CONFIG_TIMER);
+	UNSET_FLAG(group->conf->config, PEER_GROUP_CONFIG_TIMER);
 	UNSET_FLAG(group->conf->config, PEER_CONFIG_CONNECT);
 	group->conf->keepalive = 0;
 	group->conf->holdtime = 0;
@@ -4582,20 +4583,30 @@ int peer_timers_set(struct peer *peer, u_int32_t keepalive, u_int32_t holdtime)
 		return BGP_ERR_INVALID_VALUE;
 
 	/* Set value to the configuration. */
-	SET_FLAG(peer->config, PEER_CONFIG_TIMER);
 	peer->holdtime = holdtime;
 	peer->keepalive = (keepalive < holdtime / 3 ? keepalive : holdtime / 3);
 
-	if (!CHECK_FLAG(peer->sflags, PEER_STATUS_GROUP))
-		return 0;
-
-	/* peer-group member updates. */
-	group = peer->group;
-	for (ALL_LIST_ELEMENTS(group->peer, node, nnode, peer)) {
+	/* First work on real peers with timers */
+	if (!CHECK_FLAG(peer->sflags, PEER_STATUS_GROUP)) {
 		SET_FLAG(peer->config, PEER_CONFIG_TIMER);
-		peer->holdtime = group->conf->holdtime;
-		peer->keepalive = group->conf->keepalive;
+		UNSET_FLAG(peer->config, PEER_GROUP_CONFIG_TIMER);
+	} else {
+		/* Now work on the peer-group timers */
+		SET_FLAG(peer->config, PEER_GROUP_CONFIG_TIMER);
+
+		/* peer-group member updates. */
+		group = peer->group;
+		for (ALL_LIST_ELEMENTS(group->peer, node, nnode, peer)) {
+			/* Skip peers that have their own timers */
+			if (CHECK_FLAG(peer->config, PEER_CONFIG_TIMER))
+				continue;
+
+			SET_FLAG(peer->config, PEER_GROUP_CONFIG_TIMER);
+			peer->holdtime = group->conf->holdtime;
+			peer->keepalive = group->conf->keepalive;
+		}
 	}
+
 	return 0;
 }
 
@@ -4604,20 +4615,32 @@ int peer_timers_unset(struct peer *peer)
 	struct peer_group *group;
 	struct listnode *node, *nnode;
 
-	/* Clear configuration. */
-	UNSET_FLAG(peer->config, PEER_CONFIG_TIMER);
-	peer->keepalive = 0;
-	peer->holdtime = 0;
-
-	if (!CHECK_FLAG(peer->sflags, PEER_STATUS_GROUP))
-		return 0;
-
-	/* peer-group member updates. */
-	group = peer->group;
-	for (ALL_LIST_ELEMENTS(group->peer, node, nnode, peer)) {
+	/* First work on real peers vs the peer-group */
+	if (!CHECK_FLAG(peer->sflags, PEER_STATUS_GROUP)) {
 		UNSET_FLAG(peer->config, PEER_CONFIG_TIMER);
-		peer->holdtime = 0;
 		peer->keepalive = 0;
+		peer->holdtime = 0;
+
+		if (peer->group && peer->group->conf->holdtime) {
+			SET_FLAG(peer->config, PEER_GROUP_CONFIG_TIMER);
+			peer->keepalive = peer->group->conf->keepalive;
+			peer->holdtime = peer->group->conf->holdtime;
+		}
+	} else {
+		/* peer-group member updates. */
+		group = peer->group;
+		for (ALL_LIST_ELEMENTS(group->peer, node, nnode, peer)) {
+			if (!CHECK_FLAG(peer->config, PEER_CONFIG_TIMER)) {
+				UNSET_FLAG(peer->config,
+					   PEER_GROUP_CONFIG_TIMER);
+				peer->holdtime = 0;
+				peer->keepalive = 0;
+			}
+		}
+
+		UNSET_FLAG(group->conf->config, PEER_GROUP_CONFIG_TIMER);
+		group->conf->holdtime = 0;
+		group->conf->keepalive = 0;
 	}
 
 	return 0;
@@ -6535,7 +6558,7 @@ static void bgp_config_write_peer_global(struct vty *vty, struct bgp *bgp,
 	}
 
 	/* timers */
-	if (CHECK_FLAG(peer->config, PEER_CONFIG_TIMER)
+	if ((PEER_OR_GROUP_TIMER_SET(peer))
 	    && ((!peer_group_active(peer)
 		 && (peer->keepalive != BGP_DEFAULT_KEEPALIVE
 		     || peer->holdtime != BGP_DEFAULT_HOLDTIME))
diff --git a/bgpd/bgpd.h b/bgpd/bgpd.h
index 9c38a65b4..69ffe086d 100644
--- a/bgpd/bgpd.h
+++ b/bgpd/bgpd.h
@@ -771,6 +771,11 @@ struct peer {
 #define PEER_CONFIG_TIMER             (1 << 0) /* keepalive & holdtime */
 #define PEER_CONFIG_CONNECT           (1 << 1) /* connect */
 #define PEER_CONFIG_ROUTEADV          (1 << 2) /* route advertise */
+#define PEER_GROUP_CONFIG_TIMER       (1 << 3) /* timers from peer-group */
+
+#define PEER_OR_GROUP_TIMER_SET(peer)                                        \
+	(CHECK_FLAG(peer->config, PEER_CONFIG_TIMER)			     \
+	 || CHECK_FLAG(peer->config, PEER_GROUP_CONFIG_TIMER))
 
 	u_int32_t holdtime;
 	u_int32_t keepalive;