From d25e4efc52eb5e4c71f43c02991e3c9bba2388c1 Mon Sep 17 00:00:00 2001 From: Don Slice Date: Thu, 19 Oct 2017 14:23:30 -0400 Subject: [PATCH] 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 Reviewed-by: CCR-6807 Testing-performed: Manual testing successful, fix tested by submitter, bgp-smoke completed successfully --- bgpd/bgp_fsm.c | 2 +- bgpd/bgp_packet.c | 7 ++--- bgpd/bgp_snmp.c | 4 +-- bgpd/bgp_vty.c | 21 +++++++++++++-- bgpd/bgpd.c | 67 +++++++++++++++++++++++++++++++---------------- bgpd/bgpd.h | 5 ++++ 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; -- 2.39.5