From 094dcc3cdac19d3da65b38effc45aa88d960909f Mon Sep 17 00:00:00 2001 From: Francois Dumontet Date: Tue, 23 Apr 2024 11:16:24 +0200 Subject: [PATCH 1/2] bgpd: fix "bgp as-pah access-list" with "set aspath exclude" set/unset issues whith the following config router bgp 65001 no bgp ebgp-requires-policy neighbor 192.168.1.2 remote-as external neighbor 192.168.1.2 timers 3 10 ! address-family ipv4 unicast neighbor 192.168.1.2 route-map r2 in exit-address-family exit ! bgp as-path access-list FIRST seq 5 permit ^65 bgp as-path access-list SECOND seq 5 permit 2$ ! route-map r2 permit 6 match ip address prefix-list p2 set as-path exclude as-path-access-list SECOND exit ! route-map r2 permit 10 match ip address prefix-list p1 set as-path exclude 65003 exit ! route-map r2 permit 20 match ip address prefix-list p3 set as-path exclude all exit making some no bgp as-path access-list SECOND permit 2$ bgp as-path access-list SECOND permit 3$ clear bgp * no bgp as-path access-list SECOND permit 3$ bgp as-path access-list SECOND permit 2$ clear bgp * will induce some crashes thus we rework the links between aslists and aspath_exclude Signed-off-by: Francois Dumontet --- bgpd/bgp_aspath.c | 47 ++++++++++++++++++++++++++++++++++++ bgpd/bgp_aspath.h | 7 ++++++ bgpd/bgp_filter.c | 41 +++++++++++++++++++------------ bgpd/bgp_filter.h | 13 +++++----- bgpd/bgp_routemap.c | 59 ++++++++++++++++++--------------------------- 5 files changed, 108 insertions(+), 59 deletions(-) diff --git a/bgpd/bgp_aspath.c b/bgpd/bgp_aspath.c index 9dcd0ad1d621..4c1615a5c625 100644 --- a/bgpd/bgp_aspath.c +++ b/bgpd/bgp_aspath.c @@ -77,6 +77,9 @@ static struct hash *ashash; /* Stream for SNMP. See aspath_snmp_pathseg */ static struct stream *snmp_stream; +/* as-path orphan exclude list */ +static struct as_list_list_head as_exclude_list_orphan; + /* Callers are required to initialize the memory */ static as_t *assegment_data_new(int num) { @@ -1558,6 +1561,38 @@ struct aspath *aspath_prepend(struct aspath *as1, struct aspath *as2) /* Not reached */ } +/* insert aspath exclude in head of orphan exclude list*/ +void as_exclude_set_orphan(struct aspath_exclude *ase) +{ + ase->exclude_aspath_acl = NULL; + as_list_list_add_head(&as_exclude_list_orphan, ase); +} + +void as_exclude_remove_orphan(struct aspath_exclude *ase) +{ + if (as_list_list_count(&as_exclude_list_orphan)) + as_list_list_del(&as_exclude_list_orphan, ase); +} + +/* currently provide only one exclude, not a list */ +struct aspath_exclude *as_exclude_lookup_orphan(const char *acl_name) +{ + struct aspath_exclude *ase = NULL; + char *name = NULL; + + frr_each (as_list_list, &as_exclude_list_orphan, ase) { + if (ase->exclude_aspath_acl_name) { + name = ase->exclude_aspath_acl_name; + if (!strcmp(name, acl_name)) + break; + } + } + if (ase) + as_exclude_remove_orphan(ase); + + return ase; +} + /* Iterate over AS_PATH segments and wipe all occurrences of the * listed AS numbers. Hence some segments may lose some or even * all data on the way, the operation is implemented as a smarter @@ -2236,14 +2271,26 @@ void aspath_init(void) { ashash = hash_create_size(32768, aspath_key_make, aspath_cmp, "BGP AS Path"); + + as_list_list_init(&as_exclude_list_orphan); } void aspath_finish(void) { + struct aspath_exclude *ase; + hash_clean_and_free(&ashash, (void (*)(void *))aspath_free); if (snmp_stream) stream_free(snmp_stream); + + while ((ase = as_list_list_pop(&as_exclude_list_orphan))) { + aspath_free(ase->aspath); + if (ase->exclude_aspath_acl_name) + XFREE(MTYPE_TMP, ase->exclude_aspath_acl_name); + XFREE(MTYPE_ROUTE_MAP_COMPILED, ase); + } + as_list_list_fini(&as_exclude_list_orphan); } /* return and as path value */ diff --git a/bgpd/bgp_aspath.h b/bgpd/bgp_aspath.h index 2a831c3a5510..f7e57fd66dda 100644 --- a/bgpd/bgp_aspath.h +++ b/bgpd/bgp_aspath.h @@ -9,6 +9,7 @@ #include "lib/json.h" #include "bgpd/bgp_route.h" #include "bgpd/bgp_filter.h" +#include /* AS path segment type. */ #define AS_SET 1 @@ -67,11 +68,14 @@ struct aspath { /* `set as-path exclude ASn' */ struct aspath_exclude { + struct as_list_list_item exclude_list; struct aspath *aspath; bool exclude_all; char *exclude_aspath_acl_name; struct as_list *exclude_aspath_acl; }; +DECLARE_DLIST(as_list_list, struct aspath_exclude, exclude_list); + /* Prototypes. */ extern void aspath_init(void); @@ -83,6 +87,9 @@ extern struct aspath *aspath_parse(struct stream *s, size_t length, extern struct aspath *aspath_dup(struct aspath *aspath); extern struct aspath *aspath_aggregate(struct aspath *as1, struct aspath *as2); extern struct aspath *aspath_prepend(struct aspath *as1, struct aspath *as2); +extern void as_exclude_set_orphan(struct aspath_exclude *ase); +extern void as_exclude_remove_orphan(struct aspath_exclude *ase); +extern struct aspath_exclude *as_exclude_lookup_orphan(const char *acl_name); extern struct aspath *aspath_filter_exclude(struct aspath *source, struct aspath *exclude_list); extern struct aspath *aspath_filter_exclude_all(struct aspath *source); diff --git a/bgpd/bgp_filter.c b/bgpd/bgp_filter.c index a85117965aaa..002f054f5e41 100644 --- a/bgpd/bgp_filter.c +++ b/bgpd/bgp_filter.c @@ -16,7 +16,7 @@ #include "bgpd/bgp_aspath.h" #include "bgpd/bgp_regex.h" -/* List of AS filter list. */ +/* List of AS list. */ struct as_list_list { struct as_list *head; struct as_list *tail; @@ -205,14 +205,6 @@ static struct as_list *as_list_new(void) static void as_list_free(struct as_list *aslist) { - struct aspath_exclude_list *cur_bp = aslist->exclude_list; - struct aspath_exclude_list *next_bp = NULL; - - while (cur_bp) { - next_bp = cur_bp->next; - XFREE(MTYPE_ROUTE_MAP_COMPILED, cur_bp); - cur_bp = next_bp; - } XFREE (MTYPE_AS_STR, aslist->name); XFREE (MTYPE_AS_LIST, aslist); @@ -299,7 +291,6 @@ static void as_list_delete(struct as_list *aslist) { struct as_list_list *list; struct as_filter *filter, *next; - struct aspath_exclude_list *cur_bp; for (filter = aslist->head; filter; filter = next) { next = filter->next; @@ -318,12 +309,6 @@ static void as_list_delete(struct as_list *aslist) else list->head = aslist->next; - cur_bp = aslist->exclude_list; - while (cur_bp) { - cur_bp->bp_as_excl->exclude_aspath_acl = NULL; - cur_bp = cur_bp->next; - } - as_list_free(aslist); } @@ -431,6 +416,7 @@ DEFUN(as_path, bgp_as_path_cmd, enum as_filter_type type; struct as_filter *asfilter; struct as_list *aslist; + struct aspath_exclude *ase; regex_t *regex; char *regstr; int64_t seqnum = ASPATH_SEQ_NUMBER_AUTO; @@ -482,6 +468,22 @@ DEFUN(as_path, bgp_as_path_cmd, else as_list_filter_add(aslist, asfilter); + /* init the exclude rule list*/ + as_list_list_init(&aslist->exclude_rule); + + /* get aspath orphan exclude that are using this acl */ + ase = as_exclude_lookup_orphan(alname); + if (ase) { + as_list_list_add_head(&aslist->exclude_rule, ase); + /* set reverse pointer */ + ase->exclude_aspath_acl = aslist; + /* set list of aspath excludes using that acl */ + while ((ase = as_exclude_lookup_orphan(alname))) { + as_list_list_add_head(&aslist->exclude_rule, ase); + ase->exclude_aspath_acl = aslist; + } + } + return CMD_SUCCESS; } @@ -502,6 +504,7 @@ DEFUN(no_as_path, no_bgp_as_path_cmd, enum as_filter_type type; struct as_filter *asfilter; struct as_list *aslist; + struct aspath_exclude *ase; char *regstr; regex_t *regex; @@ -556,6 +559,12 @@ DEFUN(no_as_path, no_bgp_as_path_cmd, XFREE(MTYPE_TMP, regstr); + /* put aspath exclude list into orphan */ + if (as_list_list_count(&aslist->exclude_rule)) + while ((ase = as_list_list_pop(&aslist->exclude_rule))) + as_exclude_set_orphan(ase); + + as_list_list_fini(&aslist->exclude_rule); as_list_filter_delete(aslist, asfilter); return CMD_SUCCESS; diff --git a/bgpd/bgp_filter.h b/bgpd/bgp_filter.h index 2d9f07ce84a3..77a3f3c2f727 100644 --- a/bgpd/bgp_filter.h +++ b/bgpd/bgp_filter.h @@ -6,11 +6,12 @@ #ifndef _QUAGGA_BGP_FILTER_H #define _QUAGGA_BGP_FILTER_H +#include + #define ASPATH_SEQ_NUMBER_AUTO -1 enum as_filter_type { AS_FILTER_DENY, AS_FILTER_PERMIT }; - /* Element of AS path filter. */ struct as_filter { struct as_filter *next; @@ -25,11 +26,7 @@ struct as_filter { int64_t seq; }; -struct aspath_exclude_list { - struct aspath_exclude_list *next; - struct aspath_exclude *bp_as_excl; -}; - +PREDECL_DLIST(as_list_list); /* AS path filter list. */ struct as_list { char *name; @@ -39,7 +36,9 @@ struct as_list { struct as_filter *head; struct as_filter *tail; - struct aspath_exclude_list *exclude_list; + + /* Changes in AS path */ + struct as_list_list_head exclude_rule; }; diff --git a/bgpd/bgp_routemap.c b/bgpd/bgp_routemap.c index df5c2557bd26..f938869b1c48 100644 --- a/bgpd/bgp_routemap.c +++ b/bgpd/bgp_routemap.c @@ -2325,7 +2325,7 @@ static const struct route_map_rule_cmd route_set_aspath_prepend_cmd = { static void *route_aspath_exclude_compile(const char *arg) { struct aspath_exclude *ase; - struct aspath_exclude_list *ael; + struct as_list *aux_aslist; const char *str = arg; static const char asp_acl[] = "as-path-access-list"; @@ -2337,44 +2337,37 @@ static void *route_aspath_exclude_compile(const char *arg) while (*str == ' ') str++; ase->exclude_aspath_acl_name = XSTRDUP(MTYPE_TMP, str); - ase->exclude_aspath_acl = as_list_lookup(str); + aux_aslist = as_list_lookup(str); + if (!aux_aslist) + /* new orphan filter */ + as_exclude_set_orphan(ase); + else + as_list_list_add_head(&aux_aslist->exclude_rule, ase); + + ase->exclude_aspath_acl = aux_aslist; } else ase->aspath = aspath_str2aspath(str, bgp_get_asnotation(NULL)); - if (ase->exclude_aspath_acl) { - ael = XCALLOC(MTYPE_ROUTE_MAP_COMPILED, - sizeof(struct aspath_exclude_list)); - ael->bp_as_excl = ase; - ael->next = ase->exclude_aspath_acl->exclude_list; - ase->exclude_aspath_acl->exclude_list = ael; - } - return ase; } static void route_aspath_exclude_free(void *rule) { struct aspath_exclude *ase = rule; - struct aspath_exclude_list *cur_ael = NULL; - struct aspath_exclude_list *prev_ael = NULL; + struct as_list *acl; + + /* manage references to that rule*/ + if (ase->exclude_aspath_acl) { + acl = ase->exclude_aspath_acl; + as_list_list_del(&acl->exclude_rule, ase); + } else { + /* no ref to acl, this aspath exclude is orphan */ + as_exclude_remove_orphan(ase); + } aspath_free(ase->aspath); if (ase->exclude_aspath_acl_name) XFREE(MTYPE_TMP, ase->exclude_aspath_acl_name); - if (ase->exclude_aspath_acl) - cur_ael = ase->exclude_aspath_acl->exclude_list; - while (cur_ael) { - if (cur_ael->bp_as_excl == ase) { - if (prev_ael) - prev_ael->next = cur_ael->next; - else - ase->exclude_aspath_acl->exclude_list = NULL; - XFREE(MTYPE_ROUTE_MAP_COMPILED, cur_ael); - break; - } - prev_ael = cur_ael; - cur_ael = cur_ael->next; - } XFREE(MTYPE_ROUTE_MAP_COMPILED, ase); } @@ -2409,16 +2402,10 @@ route_set_aspath_exclude(void *rule, const struct prefix *dummy, void *object) else if (ase->exclude_all) path->attr->aspath = aspath_filter_exclude_all(new_path); - else if (ase->exclude_aspath_acl_name) { - if (!ase->exclude_aspath_acl) - ase->exclude_aspath_acl = - as_list_lookup(ase->exclude_aspath_acl_name); - if (ase->exclude_aspath_acl) - path->attr->aspath = - aspath_filter_exclude_acl(new_path, - ase->exclude_aspath_acl); - } - + else if (ase->exclude_aspath_acl) + path->attr->aspath = + aspath_filter_exclude_acl(new_path, + ase->exclude_aspath_acl); return RMAP_OKAY; } From 0df2e149970beff39915d0095614d56d5859f3ff Mon Sep 17 00:00:00 2001 From: Francois Dumontet Date: Wed, 24 Apr 2024 14:34:48 +0200 Subject: [PATCH 2/2] tests: improve tests for aspath exclude and bgp access list add some match in route map rules add some set unset bgp access path list add another prefix for better tests discrimination update expected results Signed-off-by: Francois Dumontet --- .../bgp_set_aspath_exclude/r1/bgpd.conf | 9 +++ .../bgp_set_aspath_exclude/r3/zebra.conf | 1 + .../test_bgp_set_aspath_exclude.py | 70 +++++++++++++++---- 3 files changed, 67 insertions(+), 13 deletions(-) diff --git a/tests/topotests/bgp_set_aspath_exclude/r1/bgpd.conf b/tests/topotests/bgp_set_aspath_exclude/r1/bgpd.conf index 9bef24f93154..c70b4934a079 100644 --- a/tests/topotests/bgp_set_aspath_exclude/r1/bgpd.conf +++ b/tests/topotests/bgp_set_aspath_exclude/r1/bgpd.conf @@ -8,10 +8,19 @@ router bgp 65001 exit-address-family ! ip prefix-list p1 seq 5 permit 172.16.255.31/32 +ip prefix-list p2 seq 5 permit 172.16.255.32/32 +ip prefix-list p3 seq 5 permit 172.16.255.30/32 ! +bgp as-path access-list FIRST permit ^65 +bgp as-path access-list SECOND permit 2$ + +route-map r2 permit 6 + match ip address prefix-list p2 + set as-path exclude as-path-access-list SECOND route-map r2 permit 10 match ip address prefix-list p1 set as-path exclude 65003 route-map r2 permit 20 + match ip address prefix-list p3 set as-path exclude all ! diff --git a/tests/topotests/bgp_set_aspath_exclude/r3/zebra.conf b/tests/topotests/bgp_set_aspath_exclude/r3/zebra.conf index 3fa6c644844c..56893158a403 100644 --- a/tests/topotests/bgp_set_aspath_exclude/r3/zebra.conf +++ b/tests/topotests/bgp_set_aspath_exclude/r3/zebra.conf @@ -1,5 +1,6 @@ ! int lo + ip address 172.16.255.30/32 ip address 172.16.255.31/32 ip address 172.16.255.32/32 ! diff --git a/tests/topotests/bgp_set_aspath_exclude/test_bgp_set_aspath_exclude.py b/tests/topotests/bgp_set_aspath_exclude/test_bgp_set_aspath_exclude.py index d373a749fe87..85e7b9676d0e 100644 --- a/tests/topotests/bgp_set_aspath_exclude/test_bgp_set_aspath_exclude.py +++ b/tests/topotests/bgp_set_aspath_exclude/test_bgp_set_aspath_exclude.py @@ -64,29 +64,33 @@ def teardown_module(mod): expected_1 = { "routes": { + "172.16.255.30/32": [{"path": ""}], "172.16.255.31/32": [{"path": "65002"}], - "172.16.255.32/32": [{"path": ""}], + "172.16.255.32/32": [{"path": "65003"}], } } expected_2 = { "routes": { - "172.16.255.31/32": [{"path": ""}], + "172.16.255.30/32": [{"path": ""}], + "172.16.255.31/32": [{"path": "65002"}], "172.16.255.32/32": [{"path": ""}], } } expected_3 = { "routes": { - "172.16.255.31/32": [{"path": "65003"}], - "172.16.255.32/32": [{"path": "65003"}], + "172.16.255.30/32": [{"path": ""}], + "172.16.255.31/32": [{"path": "65002"}], + "172.16.255.32/32": [{"path": "65002 65003"}], } } expected_4 = { "routes": { - "172.16.255.31/32": [{"path": "65002 65003"}], - "172.16.255.32/32": [{"path": "65002 65003"}], + "172.16.255.30/32": [{"path": ""}], + "172.16.255.31/32": [{"path": "65002"}], + "172.16.255.32/32": [{"path": "65002"}], } } @@ -117,34 +121,42 @@ def test_bgp_set_aspath_exclude_access_list(): rname = "r1" r1 = tgen.gears[rname] + # tgen.mininet_cli() r1.vtysh_cmd( """ conf bgp as-path access-list FIRST permit ^65 route-map r2 permit 6 + no set as-path exclude as-path-access-list SECOND set as-path exclude as-path-access-list FIRST """ ) + # tgen.mininet_cli() + r1.vtysh_cmd( + """ +clear bgp * + """ + ) test_func = functools.partial(bgp_converge, tgen.gears["r1"], expected_2) _, result = topotest.run_and_expect(test_func, None, count=30, wait=0.5) - assert result is None, "Failed overriding incoming AS-PATH with regex 1 route-map" + assert result is None, "Failed change of exclude rule in route map" r1.vtysh_cmd( """ conf - bgp as-path access-list SECOND permit 2 route-map r2 permit 6 + no set as-path exclude as-path-access-list FIRST set as-path exclude as-path-access-list SECOND """ ) # tgen.mininet_cli() - test_func = functools.partial(bgp_converge, tgen.gears["r1"], expected_3) + test_func = functools.partial(bgp_converge, tgen.gears["r1"], expected_1) _, result = topotest.run_and_expect(test_func, None, count=30, wait=0.5) - assert result is None, "Failed overriding incoming AS-PATH with regex 2 route-map" + assert result is None, "Failed reverting exclude rule in route map" def test_no_bgp_set_aspath_exclude_access_list(): @@ -159,15 +171,28 @@ def test_no_bgp_set_aspath_exclude_access_list(): r1.vtysh_cmd( """ conf - no bgp as-path access-list SECOND permit 2 + no bgp as-path access-list SECOND permit 2$ + """ + ) + + r1.vtysh_cmd( + """ +clear bgp * """ ) test_func = functools.partial(bgp_converge, tgen.gears["r1"], expected_3) _, result = topotest.run_and_expect(test_func, None, count=30, wait=0.5) - assert result is None, "Failed removing bgp as-path access-list" + assert result is None, "Failed to removing current accesslist" + # tgen.mininet_cli() + r1.vtysh_cmd( + """ +conf + bgp as-path access-list SECOND permit 3$ + """ + ) r1.vtysh_cmd( """ clear bgp * @@ -177,7 +202,26 @@ def test_no_bgp_set_aspath_exclude_access_list(): test_func = functools.partial(bgp_converge, tgen.gears["r1"], expected_4) _, result = topotest.run_and_expect(test_func, None, count=30, wait=0.5) - assert result is None, "Failed to renegotiate with peers" + assert result is None, "Failed to renegotiate with peers 2" + + r1.vtysh_cmd( + """ +conf + route-map r2 permit 6 + no set as-path exclude as-path-access-list SECOND + """ + ) + + r1.vtysh_cmd( + """ +clear bgp * + """ + ) + + test_func = functools.partial(bgp_converge, tgen.gears["r1"], expected_3) + _, result = topotest.run_and_expect(test_func, None, count=30, wait=0.5) + + assert result is None, "Failed to renegotiate with peers 2" if __name__ == "__main__":