From dfdc12e1647341bd2402986c5753102ee2bbe4ab Mon Sep 17 00:00:00 2001 From: Rafael Zalamena Date: Thu, 11 Jan 2024 17:12:44 -0300 Subject: [PATCH 1/5] lib: abstract instance redistribution management Use the linked list `del` callback to free memory instead of manually calling. Signed-off-by: Rafael Zalamena --- lib/zclient.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/lib/zclient.c b/lib/zclient.c index 063944fd3b..6aebe4377e 100644 --- a/lib/zclient.c +++ b/lib/zclient.c @@ -104,6 +104,11 @@ void zclient_free(struct zclient *zclient) XFREE(MTYPE_ZCLIENT, zclient); } +static void redist_free_instance(void *data) +{ + XFREE(MTYPE_REDIST_INST, data); +} + unsigned short *redist_check_instance(struct redist_proto *red, unsigned short instance) { @@ -126,8 +131,10 @@ void redist_add_instance(struct redist_proto *red, unsigned short instance) red->enabled = 1; - if (!red->instances) + if (!red->instances) { red->instances = list_new(); + red->instances->del = redist_free_instance; + } in = XMALLOC(MTYPE_REDIST_INST, sizeof(unsigned short)); *in = instance; @@ -143,7 +150,7 @@ void redist_del_instance(struct redist_proto *red, unsigned short instance) return; listnode_delete(red->instances, id); - XFREE(MTYPE_REDIST_INST, id); + red->instances->del(id); if (!red->instances->count) { red->enabled = 0; list_delete(&red->instances); @@ -152,14 +159,10 @@ void redist_del_instance(struct redist_proto *red, unsigned short instance) void redist_del_all_instances(struct redist_proto *red) { - struct listnode *ln, *nn; - unsigned short *id; - if (!red->instances) return; - for (ALL_LIST_ELEMENTS(red->instances, ln, nn, id)) - redist_del_instance(red, *id); + list_delete(&red->instances); } /* Stop zebra client services. */ From 28a9ca3405a6f7923b5a51b0e2db6844928676db Mon Sep 17 00:00:00 2001 From: Rafael Zalamena Date: Thu, 11 Jan 2024 17:23:28 -0300 Subject: [PATCH 2/5] lib,zebra: VRF table-direct support Implement the necessary data structures and code changes to support sending table-direct routes to protocols running in different VRFs. Signed-off-by: Rafael Zalamena --- lib/zclient.c | 138 +++++++++++++++++++++++++++++++++++++++++++ lib/zclient.h | 18 ++++++ zebra/redistribute.c | 115 +++++++++++++++++++++++------------- zebra/zapi_msg.c | 10 ++-- zebra/zapi_msg.h | 6 +- 5 files changed, 238 insertions(+), 49 deletions(-) diff --git a/lib/zclient.c b/lib/zclient.c index 6aebe4377e..8fc9addf2f 100644 --- a/lib/zclient.c +++ b/lib/zclient.c @@ -31,6 +31,7 @@ DEFINE_MTYPE_STATIC(LIB, ZCLIENT, "Zclient"); DEFINE_MTYPE_STATIC(LIB, REDIST_INST, "Redistribution instance IDs"); +DEFINE_MTYPE_STATIC(LIB, REDIST_TABLE_DIRECT, "Redistribution table direct"); /* Zebra client events. */ enum zclient_event { ZCLIENT_SCHEDULE, ZCLIENT_READ, ZCLIENT_CONNECT }; @@ -157,6 +158,87 @@ void redist_del_instance(struct redist_proto *red, unsigned short instance) } } +static void redist_free_table_direct(void *data) +{ + XFREE(MTYPE_REDIST_TABLE_DIRECT, data); +} + +struct redist_table_direct *redist_lookup_table_direct(const struct redist_proto *red, + const struct redist_table_direct *table) +{ + struct redist_table_direct *ntable; + struct listnode *node; + + if (red->instances == NULL) + return NULL; + + for (ALL_LIST_ELEMENTS_RO(red->instances, node, ntable)) { + if (table->vrf_id != ntable->vrf_id) + continue; + if (table->table_id != ntable->table_id) + continue; + + return ntable; + } + + return NULL; +} + +bool redist_table_direct_has_id(const struct redist_proto *red, int table_id) +{ + struct redist_table_direct *table; + struct listnode *node; + + if (red->instances == NULL) + return false; + + for (ALL_LIST_ELEMENTS_RO(red->instances, node, table)) { + if (table->table_id != table_id) + continue; + + return true; + } + + return false; +} + +void redist_add_table_direct(struct redist_proto *red, const struct redist_table_direct *table) +{ + struct redist_table_direct *ntable; + + ntable = redist_lookup_table_direct(red, table); + if (ntable != NULL) + return; + + if (red->instances == NULL) { + red->instances = list_new(); + red->instances->del = redist_free_table_direct; + } + + red->enabled = 1; + + ntable = XCALLOC(MTYPE_REDIST_TABLE_DIRECT, sizeof(*ntable)); + ntable->vrf_id = table->vrf_id; + ntable->table_id = table->table_id; + listnode_add(red->instances, ntable); +} + +void redist_del_table_direct(struct redist_proto *red, const struct redist_table_direct *table) +{ + struct redist_table_direct *ntable; + + ntable = redist_lookup_table_direct(red, table); + if (ntable == NULL) + return; + + listnode_delete(red->instances, ntable); + red->instances->del(ntable); + if (red->instances->count == 0) { + red->enabled = 0; + list_delete(&red->instances); + } +} + void redist_del_all_instances(struct redist_proto *red) { if (!red->instances) @@ -483,6 +565,17 @@ enum zclient_send_status zclient_send_localsid(struct zclient *zclient, return zclient_route_send(ZEBRA_ROUTE_ADD, zclient, &api); } +static void zclient_send_table_direct(struct zclient *zclient, afi_t afi, int type) +{ + struct redist_table_direct *table; + struct redist_proto *red = &zclient->mi_redist[afi][ZEBRA_ROUTE_TABLE_DIRECT]; + struct listnode *node; + + for (ALL_LIST_ELEMENTS_RO(red->instances, node, table)) + zebra_redistribute_send(type, zclient, afi, ZEBRA_ROUTE_TABLE_DIRECT, + table->table_id, table->vrf_id); +} + /* Send register requests to zebra daemon for the information in a VRF. */ void zclient_send_reg_requests(struct zclient *zclient, vrf_id_t vrf_id) { @@ -516,6 +609,12 @@ void zclient_send_reg_requests(struct zclient *zclient, vrf_id_t vrf_id) if (!zclient->mi_redist[afi][i].enabled) continue; + if (i == ZEBRA_ROUTE_TABLE_DIRECT) { + zclient_send_table_direct(zclient, afi, + ZEBRA_REDISTRIBUTE_ADD); + continue; + } + struct listnode *node; unsigned short *id; @@ -583,6 +682,12 @@ void zclient_send_dereg_requests(struct zclient *zclient, vrf_id_t vrf_id) if (!zclient->mi_redist[afi][i].enabled) continue; + if (i == ZEBRA_ROUTE_TABLE_DIRECT) { + zclient_send_table_direct(zclient, afi, + ZEBRA_REDISTRIBUTE_DELETE); + continue; + } + struct listnode *node; unsigned short *id; @@ -4637,9 +4742,42 @@ static void zclient_read(struct event *thread) zclient_event(ZCLIENT_READ, zclient); } +static void zclient_redistribute_table_direct(struct zclient *zclient, vrf_id_t vrf_id, afi_t afi, + int instance, int command) +{ + struct redist_proto *red = &zclient->mi_redist[afi][ZEBRA_ROUTE_TABLE_DIRECT]; + bool has_table; + struct redist_table_direct table = { + .vrf_id = vrf_id, + .table_id = instance, + }; + + has_table = redist_lookup_table_direct(red, &table); + + if (command == ZEBRA_REDISTRIBUTE_ADD) { + if (has_table) + return; + + redist_add_table_direct(red, &table); + } else { + if (!has_table) + return; + + redist_del_table_direct(red, &table); + } + + if (zclient->sock > 0) + zebra_redistribute_send(command, zclient, afi, ZEBRA_ROUTE_TABLE_DIRECT, instance, + vrf_id); +} + void zclient_redistribute(int command, struct zclient *zclient, afi_t afi, int type, unsigned short instance, vrf_id_t vrf_id) { + if (type == ZEBRA_ROUTE_TABLE_DIRECT) { + zclient_redistribute_table_direct(zclient, vrf_id, afi, instance, command); + return; + } if (instance) { if (command == ZEBRA_REDISTRIBUTE_ADD) { diff --git a/lib/zclient.h b/lib/zclient.h index 2385a8a219..35e23ddf63 100644 --- a/lib/zclient.h +++ b/lib/zclient.h @@ -268,6 +268,15 @@ struct redist_proto { struct list *instances; }; +/** + * Redistribute table direct instance data structure: keeps the VRF + * that subscribed to the table ID. + */ +struct redist_table_direct { + vrf_id_t vrf_id; + int table_id; +}; + struct zclient_capabilities { uint32_t ecmp; bool mpls_enabled; @@ -924,6 +933,15 @@ extern void redist_add_instance(struct redist_proto *, unsigned short); extern void redist_del_instance(struct redist_proto *, unsigned short); extern void redist_del_all_instances(struct redist_proto *red); +extern struct redist_table_direct * +redist_lookup_table_direct(const struct redist_proto *red, const struct redist_table_direct *table); +extern bool redist_table_direct_has_id(const struct redist_proto *red, int table_id); +extern void redist_add_table_direct(struct redist_proto *red, + const struct redist_table_direct *table); +extern void redist_del_table_direct(struct redist_proto *red, + const struct redist_table_direct *table); + + /* * Send to zebra that the specified vrf is using label to resolve * itself for L3VPN's. Repeated calls of this function with diff --git a/zebra/redistribute.c b/zebra/redistribute.c index 66dc5b4b5f..9bf7e2cbb5 100644 --- a/zebra/redistribute.c +++ b/zebra/redistribute.c @@ -82,8 +82,8 @@ static void zebra_redistribute_default(struct zserv *client, vrf_id_t vrf_id) RNODE_FOREACH_RE (rn, newre) { if (CHECK_FLAG(newre->flags, ZEBRA_FLAG_SELECTED)) - zsend_redistribute_route(ZEBRA_REDISTRIBUTE_ROUTE_ADD, - client, rn, newre, false); + zsend_redistribute_route(ZEBRA_REDISTRIBUTE_ROUTE_ADD, client, rn, + newre, NULL); } route_unlock_node(rn); @@ -91,6 +91,24 @@ static void zebra_redistribute_default(struct zserv *client, vrf_id_t vrf_id) } /* Redistribute routes. */ +static void redistribute_table_direct(struct zserv *client, int type, const struct route_node *rn, + const struct route_entry *re) +{ + struct redist_table_direct *table; + struct redist_proto *red; + struct listnode *node; + afi_t afi = family2afi(rn->p.family); + + red = &client->mi_redist[afi][ZEBRA_ROUTE_TABLE_DIRECT]; + + for (ALL_LIST_ELEMENTS_RO(red->instances, node, table)) { + if (table->table_id != (int)re->table) + continue; + + zsend_redistribute_route(type, client, rn, re, &table->vrf_id); + } +} + static void zebra_redistribute(struct zserv *client, int type, unsigned short instance, struct zebra_vrf *zvrf, int afi) @@ -102,13 +120,9 @@ static void zebra_redistribute(struct zserv *client, int type, vrf_id_t vrf_id = zvrf_id(zvrf); if (type == ZEBRA_ROUTE_TABLE_DIRECT) { - if (vrf_id == VRF_DEFAULT) { - table = zebra_router_find_table(zvrf, instance, afi, - SAFI_UNICAST); - type = ZEBRA_ROUTE_ALL; - is_table_direct = true; - } else - return; + table = zebra_router_find_table(zvrf, instance, afi, SAFI_UNICAST); + type = ZEBRA_ROUTE_ALL; + is_table_direct = true; } else table = zebra_vrf_table(afi, SAFI_UNICAST, vrf_id); @@ -140,15 +154,20 @@ static void zebra_redistribute(struct zserv *client, int type, if (!zebra_check_addr(&rn->p)) continue; - zsend_redistribute_route(ZEBRA_REDISTRIBUTE_ROUTE_ADD, - client, rn, newre, is_table_direct); + if (is_table_direct) + redistribute_table_direct(client, ZEBRA_REDISTRIBUTE_ROUTE_ADD, rn, + newre); + else + zsend_redistribute_route(ZEBRA_REDISTRIBUTE_ROUTE_ADD, client, rn, + newre, NULL); } } /* - * Function to return a valid table id value if table-direct is used - * return 0 otherwise - * This function can be called only if zebra_redistribute_check returns TRUE + * Checks if the route entry can be used as table-direct or not. + * `table-direct` routes always belong to `VRF_DEFAULT` and has an table + * ID different than the VRF it belongs (example main VRF table is 254, + * so in order to be `table-direct` the route's table ID must be != 254). */ static bool zebra_redistribute_is_table_direct(const struct route_entry *re) { @@ -177,15 +196,14 @@ static bool zebra_redistribute_check(const struct route_node *rn, afi = family2afi(rn->p.family); zvrf = zebra_vrf_lookup_by_id(re->vrf_id); - if (re->vrf_id == VRF_DEFAULT && zvrf->table_id != re->table) { + if (zvrf->table_id != re->table) { + /* + * Routes with table ID different from VRFs can be used as + * `table-direct` if enabled. + */ if (re->table && - redist_check_instance(&client->mi_redist - [afi][ZEBRA_ROUTE_TABLE_DIRECT], - re->table)) { - /* table-direct redistribution only for route entries which - * are on the default vrf, and that have table id different - * from the default table. - */ + redist_table_direct_has_id(&client->mi_redist[afi][ZEBRA_ROUTE_TABLE_DIRECT], + re->table)) { return true; } return false; @@ -227,7 +245,6 @@ void redistribute_update(const struct route_node *rn, { struct listnode *node, *nnode; struct zserv *client; - bool is_table_direct; if (IS_ZEBRA_DEBUG_RIB) zlog_debug( @@ -242,7 +259,6 @@ void redistribute_update(const struct route_node *rn, return; } - for (ALL_LIST_ELEMENTS(zrouter.client_list, node, nnode, client)) { if (zebra_redistribute_check(rn, re, client)) { if (IS_ZEBRA_DEBUG_RIB) { @@ -253,15 +269,19 @@ void redistribute_update(const struct route_node *rn, re->vrf_id, re->table, re->type, re->distance, re->metric); } - is_table_direct = zebra_redistribute_is_table_direct(re); - zsend_redistribute_route(ZEBRA_REDISTRIBUTE_ROUTE_ADD, - client, rn, re, - is_table_direct); + if (zebra_redistribute_is_table_direct(re)) + redistribute_table_direct(client, ZEBRA_REDISTRIBUTE_ROUTE_ADD, rn, + re); + else + zsend_redistribute_route(ZEBRA_REDISTRIBUTE_ROUTE_ADD, client, rn, + re, NULL); } else if (zebra_redistribute_check(rn, prev_re, client)) { - is_table_direct = zebra_redistribute_is_table_direct(prev_re); - zsend_redistribute_route(ZEBRA_REDISTRIBUTE_ROUTE_DEL, - client, rn, prev_re, - is_table_direct); + if (zebra_redistribute_is_table_direct(prev_re)) + redistribute_table_direct(client, ZEBRA_REDISTRIBUTE_ROUTE_DEL, rn, + prev_re); + else + zsend_redistribute_route(ZEBRA_REDISTRIBUTE_ROUTE_DEL, client, rn, + prev_re, NULL); } } } @@ -281,7 +301,6 @@ void redistribute_delete(const struct route_node *rn, struct listnode *node, *nnode; struct zserv *client; vrf_id_t vrfid; - bool is_table_direct; if (old_re) vrfid = old_re->vrf_id; @@ -344,10 +363,12 @@ void redistribute_delete(const struct route_node *rn, * happy. */ assert(old_re); - is_table_direct = zebra_redistribute_is_table_direct(old_re); - zsend_redistribute_route(ZEBRA_REDISTRIBUTE_ROUTE_DEL, - client, rn, old_re, - is_table_direct); + if (zebra_redistribute_is_table_direct(old_re)) + redistribute_table_direct(client, ZEBRA_REDISTRIBUTE_ROUTE_DEL, rn, + old_re); + else + zsend_redistribute_route(ZEBRA_REDISTRIBUTE_ROUTE_DEL, client, rn, + old_re, NULL); } } } @@ -383,8 +404,16 @@ void zebra_redistribute_add(ZAPI_HANDLER_ARGS) } if (instance) { - if (!redist_check_instance(&client->mi_redist[afi][type], - instance)) { + if (type == ZEBRA_ROUTE_TABLE_DIRECT) { + struct redist_table_direct table = { + .vrf_id = zvrf->vrf->vrf_id, + .table_id = instance, + }; + if (!redist_lookup_table_direct(&client->mi_redist[afi][type], &table)) { + redist_add_table_direct(&client->mi_redist[afi][type], &table); + zebra_redistribute(client, type, instance, zvrf, afi); + } + } else if (!redist_check_instance(&client->mi_redist[afi][type], instance)) { redist_add_instance(&client->mi_redist[afi][type], instance); zebra_redistribute(client, type, instance, zvrf, afi); @@ -443,7 +472,13 @@ void zebra_redistribute_delete(ZAPI_HANDLER_ARGS) * themselves should keep track of the received routes from zebra and * withdraw them when necessary. */ - if (instance) + if (type == ZEBRA_ROUTE_TABLE_DIRECT) { + struct redist_table_direct table = { + .vrf_id = zvrf->vrf->vrf_id, + .table_id = instance, + }; + redist_del_table_direct(&client->mi_redist[afi][type], &table); + } else if (instance) redist_del_instance(&client->mi_redist[afi][type], instance); else vrf_bitmap_unset(&client->redist[afi][type], zvrf_id(zvrf)); diff --git a/zebra/zapi_msg.c b/zebra/zapi_msg.c index ab55998af0..f32d8ea6c6 100644 --- a/zebra/zapi_msg.c +++ b/zebra/zapi_msg.c @@ -509,9 +509,8 @@ int zsend_interface_update(int cmd, struct zserv *client, struct interface *ifp) return zserv_send_message(client, s); } -int zsend_redistribute_route(int cmd, struct zserv *client, - const struct route_node *rn, - const struct route_entry *re, bool is_table_direct) +int zsend_redistribute_route(int cmd, struct zserv *client, const struct route_node *rn, + const struct route_entry *re, vrf_id_t *to_vrf) { struct zapi_route api; struct zapi_nexthop *api_nh; @@ -527,9 +526,10 @@ int zsend_redistribute_route(int cmd, struct zserv *client, api.vrf_id = re->vrf_id; api.type = re->type; api.safi = SAFI_UNICAST; - if (is_table_direct) { + if (to_vrf != NULL) { api.instance = re->table; api.type = ZEBRA_ROUTE_TABLE_DIRECT; + api.vrf_id = *to_vrf; } else api.instance = re->instance; api.flags = re->flags; @@ -598,7 +598,7 @@ int zsend_redistribute_route(int cmd, struct zserv *client, /* Attributes. */ SET_FLAG(api.message, ZAPI_MESSAGE_DISTANCE); - if (is_table_direct) + if (to_vrf != NULL) api.distance = ZEBRA_TABLEDIRECT_DISTANCE_DEFAULT; else api.distance = re->distance; diff --git a/zebra/zapi_msg.h b/zebra/zapi_msg.h index a59ccc838b..29a5b69f18 100644 --- a/zebra/zapi_msg.h +++ b/zebra/zapi_msg.h @@ -51,10 +51,8 @@ extern void nbr_connected_delete_ipv6(struct interface *ifp, struct in6_addr *address); extern int zsend_interface_update(int cmd, struct zserv *client, struct interface *ifp); -extern int zsend_redistribute_route(int cmd, struct zserv *zclient, - const struct route_node *rn, - const struct route_entry *re, - bool is_table_direct); +extern int zsend_redistribute_route(int cmd, struct zserv *zclient, const struct route_node *rn, + const struct route_entry *re, vrf_id_t *to_vrf); extern int zsend_router_id_update(struct zserv *zclient, afi_t afi, struct prefix *p, vrf_id_t vrf_id); From 7bcb2f5193daa0bf19fec880da37d42300feee96 Mon Sep 17 00:00:00 2001 From: Rafael Zalamena Date: Mon, 30 Dec 2024 16:55:12 -0300 Subject: [PATCH 3/5] bgpd: allow table-direct on different VRFs Allow table-direct to be configured in different VRFs. Signed-off-by: Rafael Zalamena --- bgpd/bgp_vty.c | 38 +------------------------------------- bgpd/bgp_zebra.c | 39 +++++++++++++++++++++++++++++++-------- 2 files changed, 32 insertions(+), 45 deletions(-) diff --git a/bgpd/bgp_vty.c b/bgpd/bgp_vty.c index 2b3e11929b..5b8858b133 100644 --- a/bgpd/bgp_vty.c +++ b/bgpd/bgp_vty.c @@ -17598,12 +17598,6 @@ DEFUN (bgp_redistribute_ipv4_ospf, if (strncmp(argv[idx_ospf_table]->arg, "o", 1) == 0) protocol = ZEBRA_ROUTE_OSPF; else { - if (bgp->vrf_id != VRF_DEFAULT) { - vty_out(vty, - "%% Only default BGP instance can use '%s'\n", - argv[idx_ospf_table]->arg); - return CMD_WARNING_CONFIG_FAILED; - } if (strncmp(argv[idx_ospf_table]->arg, "table-direct", strlen("table-direct")) == 0) { protocol = ZEBRA_ROUTE_TABLE_DIRECT; @@ -17657,12 +17651,6 @@ DEFUN (bgp_redistribute_ipv4_ospf_rmap, if (strncmp(argv[idx_ospf_table]->arg, "o", 1) == 0) protocol = ZEBRA_ROUTE_OSPF; else { - if (bgp->vrf_id != VRF_DEFAULT) { - vty_out(vty, - "%% Only default BGP instance can use '%s'\n", - argv[idx_ospf_table]->arg); - return CMD_WARNING_CONFIG_FAILED; - } if (strncmp(argv[idx_ospf_table]->arg, "table-direct", strlen("table-direct")) == 0) { protocol = ZEBRA_ROUTE_TABLE_DIRECT; @@ -17720,12 +17708,6 @@ DEFUN (bgp_redistribute_ipv4_ospf_metric, if (strncmp(argv[idx_ospf_table]->arg, "o", 1) == 0) protocol = ZEBRA_ROUTE_OSPF; else { - if (bgp->vrf_id != VRF_DEFAULT) { - vty_out(vty, - "%% Only default BGP instance can use '%s'\n", - argv[idx_ospf_table]->arg); - return CMD_WARNING_CONFIG_FAILED; - } if (strncmp(argv[idx_ospf_table]->arg, "table-direct", strlen("table-direct")) == 0) { protocol = ZEBRA_ROUTE_TABLE_DIRECT; @@ -17790,12 +17772,6 @@ DEFUN (bgp_redistribute_ipv4_ospf_rmap_metric, if (strncmp(argv[idx_ospf_table]->arg, "o", 1) == 0) protocol = ZEBRA_ROUTE_OSPF; else { - if (bgp->vrf_id != VRF_DEFAULT) { - vty_out(vty, - "%% Only default BGP instance can use '%s'\n", - argv[idx_ospf_table]->arg); - return CMD_WARNING_CONFIG_FAILED; - } if (strncmp(argv[idx_ospf_table]->arg, "table-direct", strlen("table-direct")) == 0) { protocol = ZEBRA_ROUTE_TABLE_DIRECT; @@ -17865,13 +17841,7 @@ DEFUN (bgp_redistribute_ipv4_ospf_metric_rmap, if (strncmp(argv[idx_ospf_table]->arg, "o", 1) == 0) protocol = ZEBRA_ROUTE_OSPF; else { - if (bgp->vrf_id != VRF_DEFAULT) { - vty_out(vty, - "%% Only default BGP instance can use '%s'\n", - argv[idx_ospf_table]->arg); - return CMD_WARNING_CONFIG_FAILED; - } else if (strncmp(argv[idx_ospf_table]->arg, "table-direct", - strlen("table-direct")) == 0) { + if (strncmp(argv[idx_ospf_table]->arg, "table-direct", strlen("table-direct")) == 0) { protocol = ZEBRA_ROUTE_TABLE_DIRECT; if (instance == RT_TABLE_MAIN || instance == RT_TABLE_LOCAL) { @@ -17934,12 +17904,6 @@ DEFUN (no_bgp_redistribute_ipv4_ospf, if (strncmp(argv[idx_ospf_table]->arg, "o", 1) == 0) protocol = ZEBRA_ROUTE_OSPF; else { - if (bgp->vrf_id != VRF_DEFAULT) { - vty_out(vty, - "%% Only default BGP instance can use '%s'\n", - argv[idx_ospf_table]->arg); - return CMD_WARNING_CONFIG_FAILED; - } if (strncmp(argv[idx_ospf_table]->arg, "table-direct", strlen("table-direct")) == 0) { protocol = ZEBRA_ROUTE_TABLE_DIRECT; diff --git a/bgpd/bgp_zebra.c b/bgpd/bgp_zebra.c index 8e8616c155..3d7481f630 100644 --- a/bgpd/bgp_zebra.c +++ b/bgpd/bgp_zebra.c @@ -2042,11 +2042,22 @@ int bgp_redistribute_set(struct bgp *bgp, afi_t afi, int type, /* Return if already redistribute flag is set. */ if (instance) { - if (redist_check_instance(&zclient->mi_redist[afi][type], - instance)) - return CMD_WARNING; + if (type == ZEBRA_ROUTE_TABLE_DIRECT) { + struct redist_table_direct table = { + .table_id = instance, + .vrf_id = bgp->vrf_id, + }; + if (redist_lookup_table_direct(&zclient->mi_redist[afi][type], &table) != + NULL) + return CMD_WARNING; - redist_add_instance(&zclient->mi_redist[afi][type], instance); + redist_add_table_direct(&zclient->mi_redist[afi][type], &table); + } else { + if (redist_check_instance(&zclient->mi_redist[afi][type], instance)) + return CMD_WARNING; + + redist_add_instance(&zclient->mi_redist[afi][type], instance); + } } else { if (vrf_bitmap_check(&zclient->redist[afi][type], bgp->vrf_id)) return CMD_WARNING; @@ -2174,10 +2185,22 @@ int bgp_redistribute_unreg(struct bgp *bgp, afi_t afi, int type, /* Return if zebra connection is disabled. */ if (instance) { - if (!redist_check_instance(&zclient->mi_redist[afi][type], - instance)) - return CMD_WARNING; - redist_del_instance(&zclient->mi_redist[afi][type], instance); + if (type == ZEBRA_ROUTE_TABLE_DIRECT) { + struct redist_table_direct table = { + .table_id = instance, + .vrf_id = bgp->vrf_id, + }; + if (redist_lookup_table_direct(&zclient->mi_redist[afi][type], &table) == + NULL) + return CMD_WARNING; + + redist_del_table_direct(&zclient->mi_redist[afi][type], &table); + } else { + if (!redist_check_instance(&zclient->mi_redist[afi][type], instance)) + return CMD_WARNING; + + redist_del_instance(&zclient->mi_redist[afi][type], instance); + } } else { if (!vrf_bitmap_check(&zclient->redist[afi][type], bgp->vrf_id)) return CMD_WARNING; From 36b94dcc7bbfd0735966820bcb8036aecbe34584 Mon Sep 17 00:00:00 2001 From: Rafael Zalamena Date: Mon, 30 Dec 2024 16:55:49 -0300 Subject: [PATCH 4/5] topotests: test direct-table on different VRFs Test new zebra feature that allows table-direct to work on any VRF with BGP. Signed-off-by: Rafael Zalamena --- .../bgp_table_direct_topo1/__init__.py | 0 .../bgp_table_direct_topo1/r1/frr.conf | 31 +++ .../bgp_table_direct_topo1/r2/frr.conf | 10 + .../bgp_table_direct_topo1/r3/frr.conf | 10 + .../test_bgp_table_direct_topo1.py | 201 ++++++++++++++++++ 5 files changed, 252 insertions(+) create mode 100644 tests/topotests/bgp_table_direct_topo1/__init__.py create mode 100644 tests/topotests/bgp_table_direct_topo1/r1/frr.conf create mode 100644 tests/topotests/bgp_table_direct_topo1/r2/frr.conf create mode 100644 tests/topotests/bgp_table_direct_topo1/r3/frr.conf create mode 100644 tests/topotests/bgp_table_direct_topo1/test_bgp_table_direct_topo1.py diff --git a/tests/topotests/bgp_table_direct_topo1/__init__.py b/tests/topotests/bgp_table_direct_topo1/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/topotests/bgp_table_direct_topo1/r1/frr.conf b/tests/topotests/bgp_table_direct_topo1/r1/frr.conf new file mode 100644 index 0000000000..c45e3456a4 --- /dev/null +++ b/tests/topotests/bgp_table_direct_topo1/r1/frr.conf @@ -0,0 +1,31 @@ +log commands +! +debug bgp zebra +debug zebra events +! +ip route 10.254.254.1/32 lo table 2000 +ip route 10.254.254.2/32 lo table 2000 +ip route 10.254.254.3/32 lo table 2000 +! +interface r1-eth0 + ip address 192.168.10.1/24 +! +interface r1-eth1 vrf blue + ip address 192.168.20.1/24 +! +router bgp 65001 + no bgp ebgp-requires-policy + no bgp network import-check + neighbor 192.168.10.2 remote-as external + address-family ipv4 unicast + redistribute table-direct 2000 + exit-address-family +! +router bgp 65001 vrf blue + no bgp ebgp-requires-policy + no bgp network import-check + neighbor 192.168.20.2 remote-as external + address-family ipv4 unicast + redistribute table-direct 2000 + exit-address-family +! \ No newline at end of file diff --git a/tests/topotests/bgp_table_direct_topo1/r2/frr.conf b/tests/topotests/bgp_table_direct_topo1/r2/frr.conf new file mode 100644 index 0000000000..04787be0b3 --- /dev/null +++ b/tests/topotests/bgp_table_direct_topo1/r2/frr.conf @@ -0,0 +1,10 @@ +log commands +! +interface r2-eth0 + ip address 192.168.10.2/24 +! +router bgp 65002 + no bgp ebgp-requires-policy + no bgp network import-check + neighbor 192.168.10.1 remote-as external +! \ No newline at end of file diff --git a/tests/topotests/bgp_table_direct_topo1/r3/frr.conf b/tests/topotests/bgp_table_direct_topo1/r3/frr.conf new file mode 100644 index 0000000000..2530b28bfd --- /dev/null +++ b/tests/topotests/bgp_table_direct_topo1/r3/frr.conf @@ -0,0 +1,10 @@ +log commands +! +interface r3-eth0 + ip address 192.168.20.2/24 +! +router bgp 65003 + no bgp ebgp-requires-policy + no bgp network import-check + neighbor 192.168.20.1 remote-as external +! \ No newline at end of file diff --git a/tests/topotests/bgp_table_direct_topo1/test_bgp_table_direct_topo1.py b/tests/topotests/bgp_table_direct_topo1/test_bgp_table_direct_topo1.py new file mode 100644 index 0000000000..70257be3e7 --- /dev/null +++ b/tests/topotests/bgp_table_direct_topo1/test_bgp_table_direct_topo1.py @@ -0,0 +1,201 @@ +#!/usr/bin/env python +# SPDX-License-Identifier: ISC + +# +# test_bgp_table_direct_topo1.py +# Part of NetDEF Topology Tests +# +# Copyright (c) 2025 by +# Network Device Education Foundation, Inc. ("NetDEF") +# + +""" +test_bgp_table_direct_topo1.py: Test the FRR PIM MSDP peer. +""" + +import os +import sys +import json +from functools import partial +import re +import pytest + +# Save the Current Working Directory to find configuration files. +CWD = os.path.dirname(os.path.realpath(__file__)) +sys.path.append(os.path.join(CWD, "../")) + +# pylint: disable=C0413 +# Import topogen and topotest helpers +from lib import topotest + +# Required to instantiate the topology builder class. +from lib.topogen import Topogen, TopoRouter, get_topogen +from lib.topolog import logger + +from lib.pim import McastTesterHelper + +pytestmark = [pytest.mark.bgpd, pytest.mark.pimd] + +app_helper = McastTesterHelper() + + +def build_topo(tgen): + """ + +----+ +----+ + | r1 | <-> | r2 | + +----+ +----+ + | + | +----+ + --------| r3 | + +----+ + """ + + # Create 3 routers + for routern in range(1, 4): + tgen.add_router(f"r{routern}") + + switch = tgen.add_switch("s1") + switch.add_link(tgen.gears["r1"]) + switch.add_link(tgen.gears["r2"]) + + switch = tgen.add_switch("s2") + switch.add_link(tgen.gears["r1"]) + switch.add_link(tgen.gears["r3"]) + + +def setup_module(mod): + "Sets up the pytest environment" + tgen = Topogen(build_topo, mod.__name__) + tgen.start_topology() + + router_list = tgen.routers() + for _, router in router_list.items(): + router.load_frr_config(os.path.join(CWD, f"{router.name}/frr.conf")) + + tgen.gears["r1"].run("ip link add blue type vrf table 10") + tgen.gears["r1"].run("ip link set blue up") + tgen.gears["r1"].run("ip link set r1-eth1 master blue") + + # Initialize all routers. + tgen.start_router() + + app_helper.init(tgen) + + +def teardown_module(): + "Teardown the pytest environment" + tgen = get_topogen() + app_helper.cleanup() + tgen.stop_topology() + + +def expect_bgp_route(router, iptype, route, missing=False): + "Wait until route is present on RIB for protocol." + if missing: + logger.info("waiting route {} go missing in {}".format(route, router)) + else: + logger.info("waiting route {} in {}".format(route, router)) + + tgen = get_topogen() + expected_output = {route: [{"protocol": "bgp"}]} + wait_time = 130 + if missing: + expected_output = {route: None} + wait_time = 5 + + test_func = partial( + topotest.router_json_cmp, + tgen.gears[router], + "show {} route json".format(iptype), + expected_output + ) + + _, result = topotest.run_and_expect(test_func, None, count=130, wait=1) + assertmsg = f'"{router}" convergence failure' + assert result is None, assertmsg + + +def test_bgp_convergence(): + "Wait for BGP protocol convergence" + tgen = get_topogen() + if tgen.routers_have_failure(): + pytest.skip(tgen.errors) + + logger.info("waiting for protocols to converge") + + # Wait for R2 + expect_bgp_route("r2", "ip", "10.254.254.1/32") + expect_bgp_route("r2", "ip", "10.254.254.2/32") + expect_bgp_route("r2", "ip", "10.254.254.3/32") + + # Wait for R3 + expect_bgp_route("r3", "ip", "10.254.254.1/32") + expect_bgp_route("r3", "ip", "10.254.254.2/32") + expect_bgp_route("r3", "ip", "10.254.254.3/32") + + +def test_route_change_convergence(): + "Change routes in table 2000 to test zebra redistribution." + tgen = get_topogen() + if tgen.routers_have_failure(): + pytest.skip(tgen.errors) + + tgen.gears["r1"].vtysh_cmd(""" + configure terminal + no ip route 10.254.254.2/32 lo table 2000 + ip route 10.254.254.10/32 lo table 2000 + """) + + # Check R2 + expect_bgp_route("r2", "ip", "10.254.254.2/32", missing=True) + expect_bgp_route("r2", "ip", "10.254.254.10/32") + + # Check R3 + expect_bgp_route("r3", "ip", "10.254.254.2/32", missing=True) + expect_bgp_route("r3", "ip", "10.254.254.10/32") + + +def test_configuration_removal_convergence(): + "Remove table direct configuration and check if routes went missing." + tgen = get_topogen() + if tgen.routers_have_failure(): + pytest.skip(tgen.errors) + + tgen.gears["r1"].vtysh_cmd(""" + configure terminal + router bgp 65001 + address-family ipv4 unicast + no redistribute table-direct 2000 + exit-address-family + exit + + router bgp 65001 vrf blue + address-family ipv4 unicast + no redistribute table-direct 2000 + exit-address-family + exit + """) + + # Check R2 + expect_bgp_route("r2", "ip", "10.254.254.1/32", missing=True) + expect_bgp_route("r2", "ip", "10.254.254.3/32", missing=True) + expect_bgp_route("r2", "ip", "10.254.254.10/32", missing=True) + + # Check R3 + expect_bgp_route("r3", "ip", "10.254.254.1/32", missing=True) + expect_bgp_route("r3", "ip", "10.254.254.3/32", missing=True) + expect_bgp_route("r3", "ip", "10.254.254.10/32", missing=True) + + +def test_memory_leak(): + "Run the memory leak test and report results." + tgen = get_topogen() + if not tgen.is_memleak_enabled(): + pytest.skip("Memory leak test/report is disabled") + + tgen.report_memory_leaks() + + +if __name__ == "__main__": + args = ["-s"] + sys.argv[1:] + sys.exit(pytest.main(args)) From 5846339eae91b83cf2c78cfab2e5c1a1013f456e Mon Sep 17 00:00:00 2001 From: Rafael Zalamena Date: Thu, 23 Jan 2025 15:13:01 -0300 Subject: [PATCH 5/5] bgpd,lib: document the table id / instance usage Document where relevant about the instance overload to table ID so users know what to expect. Signed-off-by: Rafael Zalamena --- bgpd/bgp_zebra.c | 12 ++++++++++++ lib/zclient.c | 10 ++++++++++ lib/zclient.h | 6 ++++++ 3 files changed, 28 insertions(+) diff --git a/bgpd/bgp_zebra.c b/bgpd/bgp_zebra.c index 3d7481f630..179404a2ce 100644 --- a/bgpd/bgp_zebra.c +++ b/bgpd/bgp_zebra.c @@ -2043,6 +2043,18 @@ int bgp_redistribute_set(struct bgp *bgp, afi_t afi, int type, /* Return if already redistribute flag is set. */ if (instance) { if (type == ZEBRA_ROUTE_TABLE_DIRECT) { + /* + * When redistribution type is `table-direct` the + * instance means `table identification`. + * + * `table_id` support 32bit integers, however since + * `instance` is being overloaded to `table_id` it + * will only be possible to use the first 65535 + * entries. + * + * Also the ZAPI must also support `int` + * (see `zebra_redistribute_add`). + */ struct redist_table_direct table = { .table_id = instance, .vrf_id = bgp->vrf_id, diff --git a/lib/zclient.c b/lib/zclient.c index 8fc9addf2f..9f6542eb31 100644 --- a/lib/zclient.c +++ b/lib/zclient.c @@ -4774,6 +4774,16 @@ static void zclient_redistribute_table_direct(struct zclient *zclient, vrf_id_t void zclient_redistribute(int command, struct zclient *zclient, afi_t afi, int type, unsigned short instance, vrf_id_t vrf_id) { + /* + * When asking for table-direct redistribution the parameter + * `instance` has a different meaning: it means table + * identification. + * + * The table identification information is stored in + * `zclient->mi_redist` along with the VRF identification + * information in a pair (different from the usual single protocol + * instance value). + */ if (type == ZEBRA_ROUTE_TABLE_DIRECT) { zclient_redistribute_table_direct(zclient, vrf_id, afi, instance, command); return; diff --git a/lib/zclient.h b/lib/zclient.h index 35e23ddf63..f3657822b8 100644 --- a/lib/zclient.h +++ b/lib/zclient.h @@ -271,6 +271,12 @@ struct redist_proto { /** * Redistribute table direct instance data structure: keeps the VRF * that subscribed to the table ID. + * + * **NOTE** + * `table_id` is an integer because that is what the netlink interface + * uses for route attribute RTA_TABLE (32bit int), however the whole + * zclient API uses `unsigned short` (and CLI commands) so it will be + * limited to the range 1 to 65535. */ struct redist_table_direct { vrf_id_t vrf_id;