From 9fb964de21c740a2e253a5527a9a037eed8c8378 Mon Sep 17 00:00:00 2001 From: Pascal Mathis Date: Mon, 11 Jun 2018 19:49:20 +0200 Subject: [PATCH] bgpd: Implement group-overrides for peer flags The current implementation of peer flags (e.g. shutdown, passive, ...) only has partial support for overriding flags of a peer-group when the peer is a member. Often settings might get lost if the user toys around with the peer-group configuration, which can lead to disaster. This commit introduces the same override implementation which was previously integrated to support proper peer flag/attribute override on the address-family level. The code is very similar and the global attributes now use their separate state-arrays *flags_invert* and *flags_override*. The test suite for BGP peer attributes was extended to also check peer global attributes, so that the newly introduced changes are covered. An additional feature was added which allows to test an attribute with an *interface-peer*, which can be configured by running `neighbor IF-TEST interface`. This was introduced so that the dynamic runtime inversion of the `extended-nexthop` flag, which is only enabled by default for interface peers, can also be tested. Last but not least, two small changes have been made to the current bgpd implementation: - The command `strict-capability-match` can now also be set on a peer-group, it seems like this command slipped through while implementing peer-groups in the very past. - The macro `COND_FLAG` was introduced inside lib/zebra.h, which now allows to either set or unset a flag based on a condition. The syntax for using this macro is: `COND_FLAG(flag_variable, flag, condition)` Signed-off-by: Pascal Mathis --- bgpd/bgp_vty.c | 25 +-- bgpd/bgpd.c | 235 ++++++++++++++-------------- bgpd/bgpd.h | 88 +++++++---- lib/zebra.h | 1 + tests/bgpd/test_peer_attr.c | 295 ++++++++++++++++++++++++----------- tests/bgpd/test_peer_attr.py | 10 ++ 6 files changed, 406 insertions(+), 248 deletions(-) diff --git a/bgpd/bgp_vty.c b/bgpd/bgp_vty.c index c1851a3e02..046f6f6de7 100644 --- a/bgpd/bgp_vty.c +++ b/bgpd/bgp_vty.c @@ -2862,8 +2862,11 @@ static int peer_conf_interface_get(struct vty *vty, const char *conf_if, bgp_session_reset(peer); } - if (!CHECK_FLAG(peer->flags, PEER_FLAG_CAPABILITY_ENHE)) - peer_flag_set(peer, PEER_FLAG_CAPABILITY_ENHE); + if (!CHECK_FLAG(peer->flags_invert, PEER_FLAG_CAPABILITY_ENHE)) { + SET_FLAG(peer->flags, PEER_FLAG_CAPABILITY_ENHE); + SET_FLAG(peer->flags_invert, PEER_FLAG_CAPABILITY_ENHE); + UNSET_FLAG(peer->flags_override, PEER_FLAG_CAPABILITY_ENHE); + } if (peer_group_name) { group = peer_group_lookup(bgp, peer_group_name); @@ -4980,26 +4983,28 @@ DEFUN (no_neighbor_override_capability, DEFUN (neighbor_strict_capability, neighbor_strict_capability_cmd, - "neighbor strict-capability-match", + "neighbor strict-capability-match", NEIGHBOR_STR - NEIGHBOR_ADDR_STR + NEIGHBOR_ADDR_STR2 "Strict capability negotiation match\n") { - int idx_ip = 1; - return peer_flag_set_vty(vty, argv[idx_ip]->arg, + int idx_peer = 1; + + return peer_flag_set_vty(vty, argv[idx_peer]->arg, PEER_FLAG_STRICT_CAP_MATCH); } DEFUN (no_neighbor_strict_capability, no_neighbor_strict_capability_cmd, - "no neighbor strict-capability-match", + "no neighbor strict-capability-match", NO_STR NEIGHBOR_STR - NEIGHBOR_ADDR_STR + NEIGHBOR_ADDR_STR2 "Strict capability negotiation match\n") { - int idx_ip = 2; - return peer_flag_unset_vty(vty, argv[idx_ip]->arg, + int idx_peer = 2; + + return peer_flag_unset_vty(vty, argv[idx_peer]->arg, PEER_FLAG_STRICT_CAP_MATCH); } diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c index 5bae542f89..9c84b3042f 100644 --- a/bgpd/bgpd.c +++ b/bgpd/bgpd.c @@ -783,6 +783,30 @@ static int peer_hash_same(const void *p1, const void *p2) == CHECK_FLAG(peer2->flags, PEER_FLAG_CONFIG_NODE)); } +void peer_flag_inherit(struct peer *peer, uint32_t flag) +{ + bool group_val; + + /* Skip if peer is not a peer-group member. */ + if (!peer_group_active(peer)) + return; + + /* Unset override flag to signal inheritance from peer-group. */ + UNSET_FLAG(peer->flags_override, flag); + + /* + * Inherit flag state from peer-group. If the flag of the peer-group is + * not being inverted, the peer must inherit the inverse of the current + * peer-group flag state. + */ + group_val = CHECK_FLAG(peer->group->conf->flags, flag); + if (!CHECK_FLAG(peer->group->conf->flags_invert, flag) + && CHECK_FLAG(peer->flags_invert, flag)) + COND_FLAG(peer->flags, flag, !group_val); + else + COND_FLAG(peer->flags, flag, group_val); +} + int peer_af_flag_check(struct peer *peer, afi_t afi, safi_t safi, uint32_t flag) { return CHECK_FLAG(peer->af_flags[afi][safi], flag); @@ -805,6 +829,18 @@ void peer_af_flag_inherit(struct peer *peer, afi_t afi, safi_t safi, UNSET_FLAG(peer->af_flags[afi][safi], flag); } +static bool peergroup_flag_check(struct peer *peer, uint32_t flag) +{ + if (!peer_group_active(peer)) { + if (CHECK_FLAG(peer->flags_invert, flag)) + return !CHECK_FLAG(peer->flags, flag); + else + return !!CHECK_FLAG(peer->flags, flag); + } + + return !!CHECK_FLAG(peer->flags_override, flag); +} + static bool peergroup_af_flag_check(struct peer *peer, afi_t afi, safi_t safi, uint32_t flag) { @@ -1264,6 +1300,7 @@ void peer_xfer_config(struct peer *peer_dst, struct peer *peer_src) /* peer flags apply */ peer_dst->flags = peer_src->flags; + peer_dst->flags_invert = peer_src->flags_invert; peer_dst->cap = peer_src->cap; peer_dst->config = peer_src->config; @@ -2443,9 +2480,11 @@ static void peer_group2peer_config_copy(struct peer_group *group, /* These are per-peer specific flags and so we must preserve them */ saved_flags |= CHECK_FLAG(peer->flags, PEER_FLAG_IFPEER_V6ONLY); - saved_flags |= CHECK_FLAG(peer->flags, PEER_FLAG_SHUTDOWN); - peer->flags = conf->flags; - SET_FLAG(peer->flags, saved_flags); + saved_flags |= peer->flags & peer->flags_override; + + peer->flags = conf->flags & ~peer->flags_override; + peer->flags |= saved_flags; + peer->flags_invert |= conf->flags_invert; /* peer config apply */ peer->config = conf->config; @@ -3993,72 +4032,85 @@ static int peer_flag_modify(struct peer *peer, uint32_t flag, int set) { int found; int size; - struct peer_group *group; - struct peer *tmp_peer; + bool invert, member_invert; + struct peer *member; struct listnode *node, *nnode; struct peer_flag_action action; memset(&action, 0, sizeof(struct peer_flag_action)); size = sizeof peer_flag_action_list / sizeof(struct peer_flag_action); + invert = CHECK_FLAG(peer->flags_invert, flag); found = peer_flag_action_set(peer_flag_action_list, size, &action, flag); - /* No flag action is found. */ + /* Abort if no flag action exists. */ if (!found) return BGP_ERR_INVALID_FLAG; - /* When unset the peer-group member's flag we have to check - peer-group configuration. */ - if (!set && peer_group_active(peer)) - if (CHECK_FLAG(peer->group->conf->flags, flag)) { - if (flag == PEER_FLAG_SHUTDOWN) - return BGP_ERR_PEER_GROUP_SHUTDOWN; - } - - /* Flag conflict check. */ + /* Check for flag conflict: STRICT_CAP_MATCH && OVERRIDE_CAPABILITY */ if (set && CHECK_FLAG(peer->flags | flag, PEER_FLAG_STRICT_CAP_MATCH) && CHECK_FLAG(peer->flags | flag, PEER_FLAG_OVERRIDE_CAPABILITY)) return BGP_ERR_PEER_FLAG_CONFLICT; + /* Handle flag updates where desired state matches current state. */ if (!CHECK_FLAG(peer->sflags, PEER_STATUS_GROUP)) { - if (set && CHECK_FLAG(peer->flags, flag) == flag) + if (set && CHECK_FLAG(peer->flags, flag)) { + COND_FLAG(peer->flags_override, flag, !invert); return 0; - if (!set && !CHECK_FLAG(peer->flags, flag)) + } + + if (!set && !CHECK_FLAG(peer->flags, flag)) { + COND_FLAG(peer->flags_override, flag, invert); return 0; + } } - if (set) - SET_FLAG(peer->flags, flag); + /* Inherit from peer-group or set/unset flags accordingly. */ + if (peer_group_active(peer) && set == invert) + peer_flag_inherit(peer, flag); else - UNSET_FLAG(peer->flags, flag); + COND_FLAG(peer->flags, flag, set); + /* Check if handling a regular peer. */ if (!CHECK_FLAG(peer->sflags, PEER_STATUS_GROUP)) { + /* Update flag override state accordingly. */ + COND_FLAG(peer->flags_override, flag, set != invert); + + /* Execute flag action on peer. */ if (action.type == peer_change_reset) peer_flag_modify_action(peer, flag); + /* Skip peer-group mechanics for regular peers. */ return 0; } - /* peer-group member updates. */ - group = peer->group; + /* + * Update peer-group members, unless they are explicitely overriding + * peer-group configuration. + */ + for (ALL_LIST_ELEMENTS(peer->group->peer, node, nnode, member)) { + /* Skip peers with overridden configuration. */ + if (CHECK_FLAG(member->flags_override, flag)) + continue; - for (ALL_LIST_ELEMENTS(group->peer, node, nnode, tmp_peer)) { + member_invert = CHECK_FLAG(member->flags_invert, flag); - if (set && CHECK_FLAG(tmp_peer->flags, flag) == flag) + /* Skip peers with equivalent configuration. */ + if (set != member_invert && CHECK_FLAG(member->flags, flag)) continue; - if (!set && !CHECK_FLAG(tmp_peer->flags, flag)) + if (set == member_invert && !CHECK_FLAG(member->flags, flag)) continue; - if (set) - SET_FLAG(tmp_peer->flags, flag); - else - UNSET_FLAG(tmp_peer->flags, flag); + /* Update flag on peer-group member. */ + COND_FLAG(member->flags, flag, set != member_invert); + /* Execute flag action on peer-group member. */ if (action.type == peer_change_reset) - peer_flag_modify_action(tmp_peer, flag); + peer_flag_modify_action(member, flag); } + return 0; } @@ -6862,22 +6914,20 @@ static void bgp_config_write_peer_global(struct vty *vty, struct bgp *bgp, if (peer->change_local_as) { if (!peer_group_active(peer) || peer->change_local_as != g_peer->change_local_as - || (CHECK_FLAG(peer->flags, PEER_FLAG_LOCAL_AS_NO_PREPEND) - != CHECK_FLAG(g_peer->flags, - PEER_FLAG_LOCAL_AS_NO_PREPEND)) - || (CHECK_FLAG(peer->flags, PEER_FLAG_LOCAL_AS_REPLACE_AS) - != CHECK_FLAG(g_peer->flags, - PEER_FLAG_LOCAL_AS_REPLACE_AS))) { - vty_out(vty, " neighbor %s local-as %u%s%s\n", addr, - peer->change_local_as, - CHECK_FLAG(peer->flags, - PEER_FLAG_LOCAL_AS_NO_PREPEND) - ? " no-prepend" - : "", - CHECK_FLAG(peer->flags, - PEER_FLAG_LOCAL_AS_REPLACE_AS) - ? " replace-as" - : ""); + || peergroup_flag_check(peer, PEER_FLAG_LOCAL_AS_NO_PREPEND) + || peergroup_flag_check(peer, + PEER_FLAG_LOCAL_AS_REPLACE_AS)) { + vty_out(vty, " neighbor %s local-as %u", addr, + peer->change_local_as); + + if (peergroup_flag_check(peer, + PEER_FLAG_LOCAL_AS_NO_PREPEND)) + vty_out(vty, " no-prepend"); + if (peergroup_flag_check(peer, + PEER_FLAG_LOCAL_AS_REPLACE_AS)) + vty_out(vty, " replace-as"); + + vty_out(vty, "\n"); } } @@ -6887,17 +6937,12 @@ static void bgp_config_write_peer_global(struct vty *vty, struct bgp *bgp, } /* shutdown */ - if (CHECK_FLAG(peer->flags, PEER_FLAG_SHUTDOWN)) { - if (!peer_group_active(peer) - || !CHECK_FLAG(g_peer->flags, PEER_FLAG_SHUTDOWN) - || peer->tx_shutdown_message) { - if (peer->tx_shutdown_message) - vty_out(vty, - " neighbor %s shutdown message %s\n", - addr, peer->tx_shutdown_message); - else - vty_out(vty, " neighbor %s shutdown\n", addr); - } + if (peergroup_flag_check(peer, PEER_FLAG_SHUTDOWN)) { + if (peer->tx_shutdown_message) + vty_out(vty, " neighbor %s shutdown message %s\n", addr, + peer->tx_shutdown_message); + else + vty_out(vty, " neighbor %s shutdown\n", addr); } /* bfd */ @@ -6934,12 +6979,8 @@ static void bgp_config_write_peer_global(struct vty *vty, struct bgp *bgp, } /* passive */ - if (CHECK_FLAG(peer->flags, PEER_FLAG_PASSIVE)) { - if (!peer_group_active(peer) - || !CHECK_FLAG(g_peer->flags, PEER_FLAG_PASSIVE)) { - vty_out(vty, " neighbor %s passive\n", addr); - } - } + if (peergroup_flag_check(peer, PEER_FLAG_PASSIVE)) + vty_out(vty, " neighbor %s passive\n", addr); /* ebgp-multihop */ if (peer->sort != BGP_PEER_IBGP && peer->ttl != 1 @@ -6960,22 +7001,12 @@ static void bgp_config_write_peer_global(struct vty *vty, struct bgp *bgp, } /* disable-connected-check */ - if (CHECK_FLAG(peer->flags, PEER_FLAG_DISABLE_CONNECTED_CHECK)) { - if (!peer_group_active(peer) - || !CHECK_FLAG(g_peer->flags, - PEER_FLAG_DISABLE_CONNECTED_CHECK)) { - vty_out(vty, " neighbor %s disable-connected-check\n", - addr); - } - } + if (peergroup_flag_check(peer, PEER_FLAG_DISABLE_CONNECTED_CHECK)) + vty_out(vty, " neighbor %s disable-connected-check\n", addr); /* enforce-first-as */ - if (CHECK_FLAG(peer->flags, PEER_FLAG_ENFORCE_FIRST_AS)) { - if (!peer_group_active(peer) - || !CHECK_FLAG(g_peer->flags, PEER_FLAG_ENFORCE_FIRST_AS)) { - vty_out(vty, " neighbor %s enforce-first-as\n", addr); - } - } + if (peergroup_flag_check(peer, PEER_FLAG_ENFORCE_FIRST_AS)) + vty_out(vty, " neighbor %s enforce-first-as\n", addr); /* update-source */ if (peer->update_if) { @@ -7029,60 +7060,32 @@ static void bgp_config_write_peer_global(struct vty *vty, struct bgp *bgp, } /* capability dynamic */ - if (CHECK_FLAG(peer->flags, PEER_FLAG_DYNAMIC_CAPABILITY)) { - if (!peer_group_active(peer) - || !CHECK_FLAG(g_peer->flags, - PEER_FLAG_DYNAMIC_CAPABILITY)) { - vty_out(vty, " neighbor %s capability dynamic\n", addr); - } - } + if (peergroup_flag_check(peer, PEER_FLAG_DYNAMIC_CAPABILITY)) + vty_out(vty, " neighbor %s capability dynamic\n", addr); /* capability extended-nexthop */ - if (peer->ifp && !CHECK_FLAG(peer->flags, PEER_FLAG_CAPABILITY_ENHE)) { - if (!peer_group_active(peer) - || !CHECK_FLAG(g_peer->flags, PEER_FLAG_CAPABILITY_ENHE)) { + if (peergroup_flag_check(peer, PEER_FLAG_CAPABILITY_ENHE)) { + if (CHECK_FLAG(peer->flags_invert, PEER_FLAG_CAPABILITY_ENHE)) vty_out(vty, " no neighbor %s capability extended-nexthop\n", addr); - } - } - - if (!peer->ifp && CHECK_FLAG(peer->flags, PEER_FLAG_CAPABILITY_ENHE)) { - if (!peer_group_active(peer) - || !CHECK_FLAG(g_peer->flags, PEER_FLAG_CAPABILITY_ENHE)) { + else vty_out(vty, " neighbor %s capability extended-nexthop\n", addr); - } } /* dont-capability-negotiation */ - if (CHECK_FLAG(peer->flags, PEER_FLAG_DONT_CAPABILITY)) { - if (!peer_group_active(peer) - || !CHECK_FLAG(g_peer->flags, PEER_FLAG_DONT_CAPABILITY)) { - vty_out(vty, " neighbor %s dont-capability-negotiate\n", - addr); - } - } + if (peergroup_flag_check(peer, PEER_FLAG_DONT_CAPABILITY)) + vty_out(vty, " neighbor %s dont-capability-negotiate\n", addr); /* override-capability */ - if (CHECK_FLAG(peer->flags, PEER_FLAG_OVERRIDE_CAPABILITY)) { - if (!peer_group_active(peer) - || !CHECK_FLAG(g_peer->flags, - PEER_FLAG_OVERRIDE_CAPABILITY)) { - vty_out(vty, " neighbor %s override-capability\n", - addr); - } - } + if (peergroup_flag_check(peer, PEER_FLAG_OVERRIDE_CAPABILITY)) + vty_out(vty, " neighbor %s override-capability\n", addr); /* strict-capability-match */ - if (CHECK_FLAG(peer->flags, PEER_FLAG_STRICT_CAP_MATCH)) { - if (!peer_group_active(peer) - || !CHECK_FLAG(g_peer->flags, PEER_FLAG_STRICT_CAP_MATCH)) { - vty_out(vty, " neighbor %s strict-capability-match\n", - addr); - } - } + if (peergroup_flag_check(peer, PEER_FLAG_STRICT_CAP_MATCH)) + vty_out(vty, " neighbor %s strict-capability-match\n", addr); } /* BGP peer configuration display function. */ diff --git a/bgpd/bgpd.h b/bgpd/bgpd.h index 33d65bcb0f..4048e8a8d9 100644 --- a/bgpd/bgpd.h +++ b/bgpd/bgpd.h @@ -822,6 +822,61 @@ struct peer { #define PEER_CAP_ENHE_AF_NEGO (1 << 14) /* Extended nexthop afi/safi negotiated */ /* Global configuration flags. */ + /* + * Parallel array to flags that indicates whether each flag originates + * from a peer-group or if it is config that is specific to this + * individual peer. If a flag is set independent of the peer-group, the + * same bit should be set here. If this peer is a peer-group, this + * memory region should be all zeros. + * + * The assumption is that the default state for all flags is unset, + * so if a flag is unset, the corresponding override flag is unset too. + * However if a flag is set, the corresponding override flag is set. + */ + uint32_t flags_override; + /* + * Parallel array to flags that indicates whether the default behavior + * of *flags_override* should be inverted. If a flag is unset and the + * corresponding invert flag is set, the corresponding override flag + * would be set. However if a flag is set and the corresponding invert + * flag is unset, the corresponding override flag would be unset. + * + * This can be used for attributes like *send-community*, which are + * implicitely enabled and have to be disabled explicitely, compared to + * 'normal' attributes like *next-hop-self* which are implicitely set. + * + * All operations dealing with flags should apply the following boolean + * logic to keep the internal flag system in a sane state: + * + * value=0 invert=0 Inherit flag if member, otherwise unset flag + * value=0 invert=1 Unset flag unconditionally + * value=1 invert=0 Set flag unconditionally + * value=1 invert=1 Inherit flag if member, otherwise set flag + * + * Contrary to the implementation of *flags_override*, the flag + * inversion state can be set either on the peer OR the peer *and* the + * peer-group. This was done on purpose, as the inversion state of a + * flag can be determined on either the peer or the peer-group. + * + * Example: Enabling the cisco configuration mode inverts all flags + * related to *send-community* unconditionally for both peer-groups and + * peers. + * + * This behavior is different for interface peers though, which enable + * the *extended-nexthop* flag by default, which regular peers do not. + * As the peer-group can contain both regular and interface peers, the + * flag inversion state must be set on the peer only. + * + * When a peer inherits the configuration from a peer-group and the + * inversion state of the flag differs between peer and peer-group, the + * newly set value must equal to the inverted state of the peer-group. + */ + uint32_t flags_invert; + /* + * Effective array for storing the peer/peer-group flags. In case of a + * peer-group, the peer-specific overrides (see flags_override and + * flags_invert) must be respected. + */ uint32_t flags; #define PEER_FLAG_PASSIVE (1 << 0) /* passive mode */ #define PEER_FLAG_SHUTDOWN (1 << 1) /* shutdown */ @@ -849,37 +904,13 @@ struct peer { /* Peer Per AF flags */ /* - * Parallel array to af_flags that indicates whether each flag - * originates from a peer-group or if it is config that is specific to - * this individual peer. If a flag is set independent of the - * peer-group the same bit should be set here. If this peer is a - * peer-group, this memory region should be all zeros. The assumption - * is that the default state for all flags is unset. - * - * Notes: - * - if a flag for an individual peer is unset, the corresponding - * override flag is unset and the peer is considered to be back in - * sync with the peer-group. - * - This does *not* contain the flag values, rather it contains - * whether the flag at the same position in af_flags is - * *peer-specific*. + * Please consult the comments for *flags_override*, *flags_invert* and + * *flags* to understand what these three arrays do. The address-family + * specific attributes are being treated the exact same way as global + * peer attributes. */ uint32_t af_flags_override[AFI_MAX][SAFI_MAX]; - /* - * Parallel array to af_flags that indicates whether each flag should - * be treated as regular (defaults to 0) or inverted (defaults to 1). - * If a flag is set to 1 by default, the same bit should be set here. - * - * Notes: - * - This does *not* contain the flag values, rather it contains - * whether the flag at the same position in af_flags is *regular* or - * *inverted*. - */ uint32_t af_flags_invert[AFI_MAX][SAFI_MAX]; - /* - * Effective flags, computed by applying peer-group flags and then - * overriding with individual flags - */ uint32_t af_flags[AFI_MAX][SAFI_MAX]; #define PEER_FLAG_SEND_COMMUNITY (1 << 0) /* send-community */ #define PEER_FLAG_SEND_EXT_COMMUNITY (1 << 1) /* send-community ext. */ @@ -1557,6 +1588,7 @@ extern int peer_group_unbind(struct bgp *, struct peer *, struct peer_group *); extern int peer_flag_set(struct peer *, uint32_t); extern int peer_flag_unset(struct peer *, uint32_t); +extern void peer_flag_inherit(struct peer *peer, uint32_t flag); extern int peer_af_flag_set(struct peer *, afi_t, safi_t, uint32_t); extern int peer_af_flag_unset(struct peer *, afi_t, safi_t, uint32_t); diff --git a/lib/zebra.h b/lib/zebra.h index 255e247221..98428eaab2 100644 --- a/lib/zebra.h +++ b/lib/zebra.h @@ -486,6 +486,7 @@ typedef enum { #define SET_FLAG(V,F) (V) |= (F) #define UNSET_FLAG(V,F) (V) &= ~(F) #define RESET_FLAG(V) (V) = 0 +#define COND_FLAG(V, F, C) ((C) ? (SET_FLAG(V, F)) : (UNSET_FLAG(V, F))) /* Atomic flag manipulation macros. */ #define CHECK_FLAG_ATOMIC(PV, F) \ diff --git a/tests/bgpd/test_peer_attr.c b/tests/bgpd/test_peer_attr.c index 67a6db849f..e4a3c6bb8a 100644 --- a/tests/bgpd/test_peer_attr.c +++ b/tests/bgpd/test_peer_attr.c @@ -62,6 +62,7 @@ struct test_config { int local_asn; int peer_asn; const char *peer_address; + const char *peer_interface; const char *peer_group; }; @@ -86,8 +87,10 @@ struct test_peer_attr { } filter; } u; struct { - bool invert; + bool invert_peer; + bool invert_group; bool use_ibgp; + bool use_iface_peer; } o; afi_t afi; @@ -113,6 +116,7 @@ static struct test_config cfg = { .local_asn = 100, .peer_asn = 200, .peer_address = "1.1.1.1", + .peer_interface = "IP-TEST", .peer_group = "PG-TEST", }; @@ -125,6 +129,61 @@ static struct test_peer_family test_default_families[] = { /* clang-format off */ static struct test_peer_attr test_peer_attrs[] = { + /* Peer Attributes */ + { + .cmd = "capability dynamic", + .u.flag = PEER_FLAG_DYNAMIC_CAPABILITY, + .type = PEER_AT_GLOBAL_FLAG, + }, + { + .cmd = "capability extended-nexthop", + .u.flag = PEER_FLAG_CAPABILITY_ENHE, + .type = PEER_AT_GLOBAL_FLAG, + }, + { + .cmd = "capability extended-nexthop", + .u.flag = PEER_FLAG_CAPABILITY_ENHE, + .type = PEER_AT_GLOBAL_FLAG, + .o.invert_peer = true, + .o.use_iface_peer = true, + }, + { + .cmd = "disable-connected-check", + .u.flag = PEER_FLAG_DISABLE_CONNECTED_CHECK, + .type = PEER_AT_GLOBAL_FLAG, + }, + { + .cmd = "dont-capability-negotiate", + .u.flag = PEER_FLAG_DONT_CAPABILITY, + .type = PEER_AT_GLOBAL_FLAG, + }, + { + .cmd = "enforce-first-as", + .u.flag = PEER_FLAG_ENFORCE_FIRST_AS, + .type = PEER_AT_GLOBAL_FLAG, + }, + { + .cmd = "override-capability", + .u.flag = PEER_FLAG_OVERRIDE_CAPABILITY, + .type = PEER_AT_GLOBAL_FLAG, + }, + { + .cmd = "passive", + .u.flag = PEER_FLAG_PASSIVE, + .type = PEER_AT_GLOBAL_FLAG, + }, + { + .cmd = "shutdown", + .u.flag = PEER_FLAG_SHUTDOWN, + .type = PEER_AT_GLOBAL_FLAG, + }, + { + .cmd = "strict-capability-match", + .u.flag = PEER_FLAG_STRICT_CAP_MATCH, + .type = PEER_AT_GLOBAL_FLAG, + }, + + /* Address Family Attributes */ { .cmd = "addpath-tx-all-paths", .u.flag = PEER_FLAG_ADDPATH_TX_ALL_PATHS, @@ -313,17 +372,20 @@ static struct test_peer_attr test_peer_attrs[] = { { .cmd = "send-community", .u.flag = PEER_FLAG_SEND_COMMUNITY, - .o.invert = true, + .o.invert_peer = true, + .o.invert_group = true, }, { .cmd = "send-community extended", .u.flag = PEER_FLAG_SEND_EXT_COMMUNITY, - .o.invert = true, + .o.invert_peer = true, + .o.invert_group = true, }, { .cmd = "send-community large", .u.flag = PEER_FLAG_SEND_LARGE_COMMUNITY, - .o.invert = true, + .o.invert_peer = true, + .o.invert_group = true, }, { .cmd = "soft-reconfiguration inbound", @@ -409,6 +471,20 @@ static const char *str_from_safi(safi_t safi) } } +static void test_log(struct test *test, const char *fmt, ...) +{ + va_list ap; + + /* Skip logging if test instance has previously failed. */ + if (test->state != TEST_SUCCESS) + return; + + /* Store formatted log message. */ + va_start(ap, fmt); + listnode_add(test->log, str_vprintf(fmt, ap)); + va_end(ap); +} + static void test_execute(struct test *test, const char *fmt, ...) { int ret; @@ -520,7 +596,8 @@ static void test_config_absent(struct test *test, const char *fmt, ...) va_end(ap); } -static struct test *test_new(const char *desc, bool use_ibgp) +static struct test *test_new(const char *desc, bool use_ibgp, + bool use_iface_peer) { struct test *test; union sockunion su; @@ -542,8 +619,16 @@ static struct test *test_new(const char *desc, bool use_ibgp) test_execute(test, "router bgp %d", cfg.local_asn); test_execute(test, "no bgp default ipv4-unicast"); test_execute(test, "neighbor %s peer-group", cfg.peer_group); - test_execute(test, "neighbor %s remote-as %d", cfg.peer_address, - use_ibgp ? cfg.local_asn : cfg.peer_asn); + if (use_iface_peer) { + test_execute(test, "neighbor %s interface", cfg.peer_interface); + test_execute(test, "neighbor %s remote-as %d", + cfg.peer_interface, + use_ibgp ? cfg.local_asn : cfg.peer_asn); + } else { + test_execute(test, "neighbor %s remote-as %d", cfg.peer_address, + use_ibgp ? cfg.local_asn : cfg.peer_asn); + } + if (test->state != TEST_SUCCESS) return test; @@ -557,8 +642,13 @@ static struct test *test_new(const char *desc, bool use_ibgp) } /* Fetch peer instance. */ - str2sockunion(cfg.peer_address, &su); - test->peer = peer_lookup(test->bgp, &su); + if (use_iface_peer) { + test->peer = + peer_lookup_by_conf_if(test->bgp, cfg.peer_interface); + } else { + str2sockunion(cfg.peer_address, &su); + test->peer = peer_lookup(test->bgp, &su); + } if (!test->peer) { test->state = TEST_INTERNAL_ERROR; test->error = str_printf( @@ -580,20 +670,6 @@ static struct test *test_new(const char *desc, bool use_ibgp) return test; }; -static void test_log(struct test *test, const char *fmt, ...) -{ - va_list ap; - - /* Skip logging if test instance has previously failed. */ - if (test->state != TEST_SUCCESS) - return; - - /* Store formatted log message. */ - va_start(ap, fmt); - listnode_add(test->log, str_vprintf(fmt, ap)); - va_end(ap); -} - static void test_finish(struct test *test) { char *msg; @@ -631,23 +707,40 @@ static void test_finish(struct test *test) XFREE(MTYPE_TMP, test); } -static void test_af_flags(struct test *test, struct peer *peer, - struct test_peer_attr *attr, bool exp_val, - bool exp_ovrd) +static void test_peer_flags(struct test *test, struct peer *peer, + struct test_peer_attr *attr, bool exp_val, + bool exp_ovrd) { bool exp_inv, cur_val, cur_ovrd, cur_inv; - /* Flip expected values for inverted flags. */ - exp_inv = attr->o.invert; + /* Skip execution if test instance has previously failed. */ + if (test->state != TEST_SUCCESS) + return; + + /* Detect if flag is meant to be inverted. */ + if (CHECK_FLAG(peer->sflags, PEER_STATUS_GROUP)) + exp_inv = attr->o.invert_group; + else + exp_inv = attr->o.invert_peer; + + /* Flip expected value if flag is inverted. */ exp_val ^= exp_inv; /* Fetch current state of value, override and invert flags. */ - cur_val = !!CHECK_FLAG(peer->af_flags[attr->afi][attr->safi], - attr->u.flag); - cur_ovrd = !!CHECK_FLAG(peer->af_flags_override[attr->afi][attr->safi], - attr->u.flag); - cur_inv = !!CHECK_FLAG(peer->af_flags_invert[attr->afi][attr->safi], - attr->u.flag); + if (attr->type == PEER_AT_GLOBAL_FLAG) { + cur_val = !!CHECK_FLAG(peer->flags, attr->u.flag); + cur_ovrd = !!CHECK_FLAG(peer->flags_override, attr->u.flag); + cur_inv = !!CHECK_FLAG(peer->flags_invert, attr->u.flag); + } else /* if (attr->type == PEER_AT_AF_FLAG) */ { + cur_val = !!CHECK_FLAG(peer->af_flags[attr->afi][attr->safi], + attr->u.flag); + cur_ovrd = !!CHECK_FLAG( + peer->af_flags_override[attr->afi][attr->safi], + attr->u.flag); + cur_inv = !!CHECK_FLAG( + peer->af_flags_invert[attr->afi][attr->safi], + attr->u.flag); + } /* Assert expected flag states. */ TEST_ASSERT_EQ(test, cur_val, exp_val); @@ -662,6 +755,10 @@ static void test_af_filter(struct test *test, struct peer *peer, bool cur_ovrd; struct bgp_filter *filter; + /* Skip execution if test instance has previously failed. */ + if (test->state != TEST_SUCCESS) + return; + /* Fetch and assert current state of override flag. */ cur_ovrd = !!CHECK_FLAG(peer->filter_override[attr->afi][attr->safi] [attr->u.filter.direct], @@ -703,19 +800,29 @@ static void test_peer_attr(struct test *test, struct test_peer_attr *pa) { int tc = 1; const char *type; - const char *ec = pa->o.invert ? "no " : ""; - const char *dc = pa->o.invert ? "" : "no "; + const char *ecp = pa->o.invert_peer ? "no " : ""; + const char *dcp = pa->o.invert_peer ? "" : "no "; + const char *ecg = pa->o.invert_group ? "no " : ""; + const char *dcg = pa->o.invert_group ? "" : "no "; const char *peer_cmd = pa->peer_cmd ?: pa->cmd; const char *group_cmd = pa->group_cmd ?: pa->cmd; struct peer *p = test->peer; struct peer_group *g = test->group; - if (pa->type == PEER_AT_AF_FLAG) + if (pa->type == PEER_AT_GLOBAL_FLAG) { + type = "peer-flag"; + } else if (pa->type == PEER_AT_AF_FLAG) { type = "af-flag"; - else /* if (pa->type == PEER_AT_AF_FILTER) */ + } else if (pa->type == PEER_AT_AF_FILTER) { type = "af-filter"; + } else { + test->state = TEST_INTERNAL_ERROR; + test->error = + str_printf("invalid attribute type: %d", pa->type); + return; + } - /* Test Case: Switch active address-family. */ + /* Test Preparation: Switch active address-family. */ if (pa->type == PEER_AT_AF_FLAG || pa->type == PEER_AT_AF_FILTER) { test_log(test, "prepare: switch address-family to [%s]", afi_safi_print(pa->afi, pa->safi)); @@ -726,12 +833,12 @@ static void test_peer_attr(struct test *test, struct test_peer_attr *pa) /* Test Case: Set flag on BGP peer. */ test_log(test, "case %02d: set %s [%s] on [%s]", tc++, type, peer_cmd, p->host); - test_execute(test, "%sneighbor %s %s", ec, p->host, peer_cmd); - test_config_present(test, "%sneighbor %s %s", ec, p->host, peer_cmd); + test_execute(test, "%sneighbor %s %s", ecp, p->host, peer_cmd); + test_config_present(test, "%sneighbor %s %s", ecp, p->host, peer_cmd); test_config_absent(test, "neighbor %s %s", g->name, pa->cmd); - if (pa->type == PEER_AT_AF_FLAG) { - test_af_flags(test, p, pa, true, true); - test_af_flags(test, g->conf, pa, false, false); + if (pa->type == PEER_AT_GLOBAL_FLAG || pa->type == PEER_AT_AF_FLAG) { + test_peer_flags(test, p, pa, true, true); + test_peer_flags(test, g->conf, pa, false, false); } else if (pa->type == PEER_AT_AF_FILTER) { test_af_filter(test, p, pa, true, true); test_af_filter(test, g->conf, pa, false, false); @@ -741,13 +848,13 @@ static void test_peer_attr(struct test *test, struct test_peer_attr *pa) test_log(test, "case %02d: add peer [%s] to group [%s]", tc++, p->host, g->name); test_execute(test, "neighbor %s peer-group %s", p->host, g->name); - test_config_present(test, "neighbor %s peer-group %s", p->host, - g->name); - test_config_present(test, "%sneighbor %s %s", ec, p->host, peer_cmd); + test_config_present(test, "neighbor %s %speer-group %s", p->host, + p->conf_if ? "interface " : "", g->name); + test_config_present(test, "%sneighbor %s %s", ecp, p->host, peer_cmd); test_config_absent(test, "neighbor %s %s", g->name, pa->cmd); - if (pa->type == PEER_AT_AF_FLAG) { - test_af_flags(test, p, pa, true, true); - test_af_flags(test, g->conf, pa, false, false); + if (pa->type == PEER_AT_GLOBAL_FLAG || pa->type == PEER_AT_AF_FLAG) { + test_peer_flags(test, p, pa, true, true); + test_peer_flags(test, g->conf, pa, false, false); } else if (pa->type == PEER_AT_AF_FILTER) { test_af_filter(test, p, pa, true, true); test_af_filter(test, g->conf, pa, false, false); @@ -757,13 +864,13 @@ static void test_peer_attr(struct test *test, struct test_peer_attr *pa) test_log(test, "case %02d: re-add peer [%s] to group [%s]", tc++, p->host, g->name); test_execute(test, "neighbor %s peer-group %s", p->host, g->name); - test_config_present(test, "neighbor %s peer-group %s", p->host, - g->name); - test_config_present(test, "%sneighbor %s %s", ec, p->host, peer_cmd); + test_config_present(test, "neighbor %s %speer-group %s", p->host, + p->conf_if ? "interface " : "", g->name); + test_config_present(test, "%sneighbor %s %s", ecp, p->host, peer_cmd); test_config_absent(test, "neighbor %s %s", g->name, pa->cmd); - if (pa->type == PEER_AT_AF_FLAG) { - test_af_flags(test, p, pa, true, true); - test_af_flags(test, g->conf, pa, false, false); + if (pa->type == PEER_AT_GLOBAL_FLAG || pa->type == PEER_AT_AF_FLAG) { + test_peer_flags(test, p, pa, true, true); + test_peer_flags(test, g->conf, pa, false, false); } else if (pa->type == PEER_AT_AF_FILTER) { test_af_filter(test, p, pa, true, true); test_af_filter(test, g->conf, pa, false, false); @@ -772,12 +879,12 @@ static void test_peer_attr(struct test *test, struct test_peer_attr *pa) /* Test Case: Set flag on BGP peer-group. */ test_log(test, "case %02d: set %s [%s] on [%s]", tc++, type, group_cmd, g->name); - test_execute(test, "%sneighbor %s %s", ec, g->name, group_cmd); - test_config_present(test, "%sneighbor %s %s", ec, p->host, peer_cmd); - test_config_present(test, "%sneighbor %s %s", ec, g->name, group_cmd); - if (pa->type == PEER_AT_AF_FLAG) { - test_af_flags(test, p, pa, true, true); - test_af_flags(test, g->conf, pa, true, false); + test_execute(test, "%sneighbor %s %s", ecg, g->name, group_cmd); + test_config_present(test, "%sneighbor %s %s", ecp, p->host, peer_cmd); + test_config_present(test, "%sneighbor %s %s", ecg, g->name, group_cmd); + if (pa->type == PEER_AT_GLOBAL_FLAG || pa->type == PEER_AT_AF_FLAG) { + test_peer_flags(test, p, pa, true, true); + test_peer_flags(test, g->conf, pa, true, false); } else if (pa->type == PEER_AT_AF_FILTER) { test_af_filter(test, p, pa, true, true); test_af_filter(test, g->conf, pa, true, false); @@ -786,12 +893,12 @@ static void test_peer_attr(struct test *test, struct test_peer_attr *pa) /* Test Case: Unset flag on BGP peer-group. */ test_log(test, "case %02d: unset %s [%s] on [%s]", tc++, type, group_cmd, g->name); - test_execute(test, "%sneighbor %s %s", dc, g->name, group_cmd); - test_config_present(test, "%sneighbor %s %s", ec, p->host, peer_cmd); + test_execute(test, "%sneighbor %s %s", dcg, g->name, group_cmd); + test_config_present(test, "%sneighbor %s %s", ecp, p->host, peer_cmd); test_config_absent(test, "neighbor %s %s", g->name, pa->cmd); - if (pa->type == PEER_AT_AF_FLAG) { - test_af_flags(test, p, pa, true, true); - test_af_flags(test, g->conf, pa, false, false); + if (pa->type == PEER_AT_GLOBAL_FLAG || pa->type == PEER_AT_AF_FLAG) { + test_peer_flags(test, p, pa, true, true); + test_peer_flags(test, g->conf, pa, false, false); } else if (pa->type == PEER_AT_AF_FILTER) { test_af_filter(test, p, pa, true, true); test_af_filter(test, g->conf, pa, false, false); @@ -800,12 +907,12 @@ static void test_peer_attr(struct test *test, struct test_peer_attr *pa) /* Test Case: Set flag on BGP peer-group. */ test_log(test, "case %02d: set %s [%s] on [%s]", tc++, type, group_cmd, g->name); - test_execute(test, "%sneighbor %s %s", ec, g->name, group_cmd); - test_config_present(test, "%sneighbor %s %s", ec, p->host, peer_cmd); - test_config_present(test, "%sneighbor %s %s", ec, g->name, group_cmd); - if (pa->type == PEER_AT_AF_FLAG) { - test_af_flags(test, p, pa, true, true); - test_af_flags(test, g->conf, pa, true, false); + test_execute(test, "%sneighbor %s %s", ecg, g->name, group_cmd); + test_config_present(test, "%sneighbor %s %s", ecp, p->host, peer_cmd); + test_config_present(test, "%sneighbor %s %s", ecg, g->name, group_cmd); + if (pa->type == PEER_AT_GLOBAL_FLAG || pa->type == PEER_AT_AF_FLAG) { + test_peer_flags(test, p, pa, true, true); + test_peer_flags(test, g->conf, pa, true, false); } else if (pa->type == PEER_AT_AF_FILTER) { test_af_filter(test, p, pa, true, true); test_af_filter(test, g->conf, pa, true, false); @@ -814,12 +921,12 @@ static void test_peer_attr(struct test *test, struct test_peer_attr *pa) /* Test Case: Re-set flag on BGP peer. */ test_log(test, "case %02d: re-set %s [%s] on [%s]", tc++, type, peer_cmd, p->host); - test_execute(test, "%sneighbor %s %s", ec, p->host, peer_cmd); - test_config_present(test, "%sneighbor %s %s", ec, p->host, peer_cmd); - test_config_present(test, "%sneighbor %s %s", ec, g->name, group_cmd); - if (pa->type == PEER_AT_AF_FLAG) { - test_af_flags(test, p, pa, true, true); - test_af_flags(test, g->conf, pa, true, false); + test_execute(test, "%sneighbor %s %s", ecp, p->host, peer_cmd); + test_config_present(test, "%sneighbor %s %s", ecp, p->host, peer_cmd); + test_config_present(test, "%sneighbor %s %s", ecg, g->name, group_cmd); + if (pa->type == PEER_AT_GLOBAL_FLAG || pa->type == PEER_AT_AF_FLAG) { + test_peer_flags(test, p, pa, true, true); + test_peer_flags(test, g->conf, pa, true, false); } else if (pa->type == PEER_AT_AF_FILTER) { test_af_filter(test, p, pa, true, true); test_af_filter(test, g->conf, pa, true, false); @@ -828,12 +935,12 @@ static void test_peer_attr(struct test *test, struct test_peer_attr *pa) /* Test Case: Unset flag on BGP peer. */ test_log(test, "case %02d: unset %s [%s] on [%s]", tc++, type, peer_cmd, p->host); - test_execute(test, "%sneighbor %s %s", dc, p->host, peer_cmd); + test_execute(test, "%sneighbor %s %s", dcp, p->host, peer_cmd); test_config_absent(test, "neighbor %s %s", p->host, pa->cmd); - test_config_present(test, "%sneighbor %s %s", ec, g->name, group_cmd); - if (pa->type == PEER_AT_AF_FLAG) { - test_af_flags(test, p, pa, true, false); - test_af_flags(test, g->conf, pa, true, false); + test_config_present(test, "%sneighbor %s %s", ecg, g->name, group_cmd); + if (pa->type == PEER_AT_GLOBAL_FLAG || pa->type == PEER_AT_AF_FLAG) { + test_peer_flags(test, p, pa, true, false); + test_peer_flags(test, g->conf, pa, true, false); } else if (pa->type == PEER_AT_AF_FILTER) { test_af_filter(test, p, pa, true, false); test_af_filter(test, g->conf, pa, true, false); @@ -842,12 +949,12 @@ static void test_peer_attr(struct test *test, struct test_peer_attr *pa) /* Test Case: Unset flag on BGP peer-group. */ test_log(test, "case %02d: unset %s [%s] on [%s]", tc++, type, group_cmd, g->name); - test_execute(test, "%sneighbor %s %s", dc, g->name, group_cmd); + test_execute(test, "%sneighbor %s %s", dcg, g->name, group_cmd); test_config_absent(test, "neighbor %s %s", p->host, pa->cmd); test_config_absent(test, "neighbor %s %s", g->name, pa->cmd); - if (pa->type == PEER_AT_AF_FLAG) { - test_af_flags(test, p, pa, false, false); - test_af_flags(test, g->conf, pa, false, false); + if (pa->type == PEER_AT_GLOBAL_FLAG || pa->type == PEER_AT_AF_FLAG) { + test_peer_flags(test, p, pa, false, false); + test_peer_flags(test, g->conf, pa, false, false); } else if (pa->type == PEER_AT_AF_FILTER) { test_af_filter(test, p, pa, false, false); test_af_filter(test, g->conf, pa, false, false); @@ -856,12 +963,12 @@ static void test_peer_attr(struct test *test, struct test_peer_attr *pa) /* Test Case: Set flag on BGP peer. */ test_log(test, "case %02d: set %s [%s] on [%s]", tc++, type, peer_cmd, p->host); - test_execute(test, "%sneighbor %s %s", ec, p->host, peer_cmd); - test_config_present(test, "%sneighbor %s %s", ec, p->host, peer_cmd); + test_execute(test, "%sneighbor %s %s", ecp, p->host, peer_cmd); + test_config_present(test, "%sneighbor %s %s", ecp, p->host, peer_cmd); test_config_absent(test, "neighbor %s %s", g->name, pa->cmd); - if (pa->type == PEER_AT_AF_FLAG) { - test_af_flags(test, p, pa, true, true); - test_af_flags(test, g->conf, pa, false, false); + if (pa->type == PEER_AT_GLOBAL_FLAG || pa->type == PEER_AT_AF_FLAG) { + test_peer_flags(test, p, pa, true, true); + test_peer_flags(test, g->conf, pa, false, false); } else if (pa->type == PEER_AT_AF_FILTER) { test_af_filter(test, p, pa, true, true); test_af_filter(test, g->conf, pa, false, false); @@ -980,7 +1087,7 @@ int main(void) desc = str_printf("peer\\%s", pa->cmd); /* Initialize new test instance. */ - test = test_new(desc, pa->o.use_ibgp); + test = test_new(desc, pa->o.use_ibgp, pa->o.use_iface_peer); XFREE(MTYPE_TMP, desc); /* Execute tests and finish test instance. */ diff --git a/tests/bgpd/test_peer_attr.py b/tests/bgpd/test_peer_attr.py index d93dfc0050..2c1623dea2 100644 --- a/tests/bgpd/test_peer_attr.py +++ b/tests/bgpd/test_peer_attr.py @@ -6,6 +6,16 @@ class TestFlag(frrtest.TestMultiOut): # List of tests can be generated by executing: # $> ./test_peer_attr 2>&1 | sed -n 's/\\/\\\\/g; s/\S\+ \[test\] \(.\+\)/TestFlag.okfail(\x27\1\x27)/pg' # +TestFlag.okfail('peer\\capability dynamic') +TestFlag.okfail('peer\\capability extended-nexthop') +TestFlag.okfail('peer\\capability extended-nexthop') +TestFlag.okfail('peer\\disable-connected-check') +TestFlag.okfail('peer\\dont-capability-negotiate') +TestFlag.okfail('peer\\enforce-first-as') +TestFlag.okfail('peer\\override-capability') +TestFlag.okfail('peer\\passive') +TestFlag.okfail('peer\\shutdown') +TestFlag.okfail('peer\\strict-capability-match') TestFlag.okfail('peer\\ipv4-unicast\\addpath-tx-all-paths') TestFlag.okfail('peer\\ipv4-multicast\\addpath-tx-all-paths') TestFlag.okfail('peer\\ipv6-unicast\\addpath-tx-all-paths') -- 2.39.5