On Tue, 2019-04-30 at 13:24 +0200, Ondrej Zajicek wrote: > On Mon, Apr 29, 2019 at 03:46:09PM +0000, Kenth Eriksson wrote: > > On Mon, 2019-04-29 at 15:42 +0200, Ondrej Zajicek wrote: > > > CAUTION: This email originated from outside of the organization. > > > Do > > > not click links or open attachments unless you recognize the > > > sender > > > and know the content is safe. > > > > > > > > > On Mon, Apr 29, 2019 at 01:01:13PM +0000, Kenth Eriksson wrote: > > > > > 'ip' tool, perhaps harder in other cases). It would be great > > > > > if > > > > > there > > > > > existed sysctl option for default IPv4 route metric. > > > > > > > > > > > > > There is no overwrite involed here. The default route in the > > > > kernel > > > > here has metric 100. As you said, bird pushes with metric 32 so > > > > it > > > > should push again. > > > > > > Well, i misread your mail. If i understand it correctly, the > > > issue is > > > not that routes are not pushed into kernel, but that they are not > > > pushed into master table. I guess you have two routes for the > > > same > > > network in one static protocol. That is not valid case, although > > > we are missing error checks for that configuration. > > > > > > You could either use two separate static protocol instances, or > > > you > > > could use this patch: > > > > > > https://nam03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.labs.nic.cz%2Flabs%2Fbird%2Fcommit%2F9861dba5230da539e6ce7d2b6baa4f2631556d09&data=02%7C01%7CKenth.Eriksson%40infinera.com%7Cc239548bfa554327784c08d6cd5e7822%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C0%7C636922202971698401&sdata=Wv%2Be2YRd%2Fo2efHVjKT8wPlnjC3KUF8jGVTFsGwKsQpk%3D&reserved=0 > > > > > > > That patch did not work for me, still same issue. I had to add > > DISTANCE > > to the CF_KEYWORDS to make it compile. But is it anything else that > > is > > required? > > Hi > > The patch should be more-or-less all that is required, but it is not > fully > ready for master branch, so it has a bit awkward behavior. Namely it > requires > that preference is specified without '=', like in example i provided: > > > > route 0.0.0.0/0 via 10.210.137.1 { preference 220; }; > > > route 0.0.0.0/0 via 192.168.120.1 { preference 230; }; > > If you used the same config like without patch (i.e. with 'preference > = XXX'), > it would not work. >
I have attached three patches can you review them? It is your first commit, then a patch to make it compile and a patch to fix the syntax to be compliant with the filter syntax. Can this be upstreamed so you will support this use-case in upcoming releases? > > > I thought two prefixes differing only on metric should be perfectly > > valid? The kernel accepts that, so why would bird not? > > Mainly implementation reasons. In BIRD, preference is generally not > part > of key, therefore route update for one network could replace a route > for > route with different preference (which makes sense with filter-based > dynamic preference). > > Traditionally BIRD supported in one routing table one route per > network > and protocol instance. With BGP ADD_PATH extension the key was > extended > with arbitrary u32 distinguisher, which in BGP case is the path ID > value > from BGP ADD_PATH, but most other protocols does not use that. The > patch > above uses preference as distinguisher in case of static routes. >
From a978abe2380f3aa4c71f2af0a1001cbfa4b4a5b1 Mon Sep 17 00:00:00 2001 From: "Ondrej Zajicek (work)" <[email protected]> Date: Fri, 23 Nov 2018 00:18:11 +0100 Subject: [PATCH 1/3] Static: Allow multiple routes to the same network with different preference Signed-off-by: Kenth Eriksson <[email protected]> --- proto/static/config.Y | 2 ++ proto/static/static.c | 27 ++++++++++++++++++++------- proto/static/static.h | 1 + 3 files changed, 23 insertions(+), 7 deletions(-) diff --git a/proto/static/config.Y b/proto/static/config.Y index d7a02961..32eba0fe 100644 --- a/proto/static/config.Y +++ b/proto/static/config.Y @@ -135,6 +135,8 @@ stat_route: stat_route_item: cmd { *this_srt_last_cmd = $1; this_srt_last_cmd = &($1->next); } + | PREFERENCE expr ';' { this_srt->preference = $2; check_u16($2); } + | DISTANCE expr ';' { this_srt->preference = $2; check_u16($2); } ; stat_route_opts: diff --git a/proto/static/static.c b/proto/static/static.c index 75a74ad0..feb8e427 100644 --- a/proto/static/static.c +++ b/proto/static/static.c @@ -53,7 +53,7 @@ static void static_announce_rte(struct static_proto *p, struct static_route *r) { rta *a = allocz(RTA_MAX_SIZE); - a->src = p->p.main_source; + a->src = rt_get_source(&p->p, r->preference); a->source = RTS_STATIC; a->scope = SCOPE_UNIVERSE; a->dest = r->dest; @@ -101,11 +101,13 @@ static_announce_rte(struct static_proto *p, struct static_route *r) /* We skip rta_lookup() here */ rte *e = rte_get_temp(a); e->pflags = 0; + e->pref = r->preference; if (r->cmds) f_eval_rte(r->cmds, &e, static_lp); - rte_update(&p->p, r->net, e); + e->pref = r->preference; /* Avoid preference from filter */ + rte_update2(p->p.main_channel, r->net, e, a->src); r->state = SRS_CLEAN; if (r->cmds) @@ -117,7 +119,7 @@ withdraw: if (r->state == SRS_DOWN) return; - rte_update(&p->p, r->net, NULL); + rte_update2(p->p.main_channel, r->net, NULL, a->src); r->state = SRS_DOWN; } @@ -249,7 +251,7 @@ static void static_remove_rte(struct static_proto *p, struct static_route *r) { if (r->state) - rte_update(&p->p, r->net, NULL); + rte_update2(p->p.main_channel, r->net, NULL, rt_get_source(&p->p, r->preference)); static_reset_rte(p, r); } @@ -379,8 +381,13 @@ static_postconfig(struct proto_config *CF) cc->table : cf->c.global->def_tables[NET_IP6]; WALK_LIST(r, cf->routes) + { if (r->net && (r->net->type != CF->net_type)) cf_error("Route %N incompatible with channel type", r->net); + + if (!r->preference) + r->preference = cc->preference; + } } static struct proto * @@ -485,11 +492,17 @@ static_dump(struct proto *P) #define IGP_TABLE(cf, sym) ((cf)->igp_table_##sym ? (cf)->igp_table_##sym ->table : NULL ) +static inline int srt_equal(struct static_route *a, struct static_route *b) +{ return net_equal(a->net, b->net) && (a->preference == b->preference); } + +static inline int srt_compare(struct static_route *a, struct static_route *b) +{ return net_compare(a->net, b->net) ?: uint_cmp(a->preference, b->preference); } + static inline int static_cmp_rte(const void *X, const void *Y) { struct static_route *x = *(void **)X, *y = *(void **)Y; - return net_compare(x->net, y->net); + return srt_compare(x, y); } static int @@ -518,7 +531,7 @@ static_reconfigure(struct proto *P, struct proto_config *CF) /* Reconfigure initial matching sequence */ for (or = HEAD(o->routes), nr = HEAD(n->routes); - NODE_VALID(or) && NODE_VALID(nr) && net_equal(or->net, nr->net); + NODE_VALID(or) && NODE_VALID(nr) && srt_equal(or, nr); or = NODE_NEXT(or), nr = NODE_NEXT(nr)) static_reconfigure_rte(p, or, nr); @@ -549,7 +562,7 @@ static_reconfigure(struct proto *P, struct proto_config *CF) while ((orpos < ornum) && (nrpos < nrnum)) { - int x = net_compare(orbuf[orpos]->net, nrbuf[nrpos]->net); + int x = srt_compare(orbuf[orpos], nrbuf[nrpos]); if (x < 0) static_remove_rte(p, orbuf[orpos++]); else if (x > 0) diff --git a/proto/static/static.h b/proto/static/static.h index a3c30b87..ec9dffb3 100644 --- a/proto/static/static.h +++ b/proto/static/static.h @@ -40,6 +40,7 @@ struct static_route { struct static_route *mp_head; /* First nexthop of this route */ struct static_route *mp_next; /* Nexthops for multipath routes */ struct f_inst *cmds; /* List of commands for setting attributes */ + u16 preference; /* Route preference */ byte dest; /* Destination type (RTD_*) */ byte state; /* State of route announcement (SRS_*) */ byte active; /* Next hop is active (nbr/iface/BFD available) */ -- 2.21.0
From d7a2ce57a282d4eb3d2d4b83c4f4ea537a958506 Mon Sep 17 00:00:00 2001 From: Kenth Eriksson <[email protected]> Date: Mon, 29 Apr 2019 17:06:39 +0200 Subject: [PATCH 2/3] Add missing keyword DISTANCE Signed-off-by: Kenth Eriksson <[email protected]> --- nest/config.Y | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nest/config.Y b/nest/config.Y index aef5ed46..aee52b97 100644 --- a/nest/config.Y +++ b/nest/config.Y @@ -64,7 +64,7 @@ proto_postconfig(void) CF_DECLS -CF_KEYWORDS(ROUTER, ID, PROTOCOL, TEMPLATE, PREFERENCE, DISABLED, DEBUG, ALL, OFF, DIRECT) +CF_KEYWORDS(ROUTER, ID, PROTOCOL, TEMPLATE, PREFERENCE, DISTANCE, DISABLED, DEBUG, ALL, OFF, DIRECT) CF_KEYWORDS(INTERFACE, IMPORT, EXPORT, FILTER, NONE, VRF, TABLE, STATES, ROUTES, FILTERS) CF_KEYWORDS(IPV4, IPV6, VPN4, VPN6, ROA4, ROA6, FLOW4, FLOW6, SADR, MPLS) CF_KEYWORDS(RECEIVE, LIMIT, ACTION, WARN, BLOCK, RESTART, DISABLE, KEEP, FILTERED) -- 2.21.0
From a2aedcebf3aa2f924999baff133d2e87d7353661 Mon Sep 17 00:00:00 2001 From: Kenth Eriksson <[email protected]> Date: Thu, 2 May 2019 09:12:36 +0200 Subject: [PATCH 3/3] Align preference syntax for static routes and filters. Setting preference value for filters and static routes is done using the following syntax: preference = value. Signed-off-by: Kenth Eriksson <[email protected]> --- proto/static/config.Y | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/proto/static/config.Y b/proto/static/config.Y index 32eba0fe..0526a8cd 100644 --- a/proto/static/config.Y +++ b/proto/static/config.Y @@ -135,8 +135,8 @@ stat_route: stat_route_item: cmd { *this_srt_last_cmd = $1; this_srt_last_cmd = &($1->next); } - | PREFERENCE expr ';' { this_srt->preference = $2; check_u16($2); } - | DISTANCE expr ';' { this_srt->preference = $2; check_u16($2); } + | PREFERENCE '=' expr ';' { this_srt->preference = $3; check_u16($3); } + | DISTANCE '=' expr ';' { this_srt->preference = $3; check_u16($3); } ; stat_route_opts: -- 2.21.0
