There is no reason to overwrite attributes like AS path, next hop, etc. when the route source is not BGP. Preserving them has several advantages: * no attributes are lost by using opaque pipes * BGP attributes can by pre-populated in other protocols' import filters As a side effect of moving code from bgp_create_attrs() to bgp_update_attrs(), the BGP protocol now uses the same logic to determine if the "gw" attribute should be used as next hop for routes from BGP sources as for those from other sources. Previously, the "gw" attribute was never for routes from BGP. Limitations: * The handling of routes imported from/exported to route reflectors isn't preserved through opaque pipes --- Hi, this is a patch I just wrote up in need for this feature. I've tested it on a handful of systems and everything seems to be working correctly. Comments welcome :) Regards, Matthias proto/bgp/attrs.c | 101 ++++++++++++++++++++++++------------------------------ 1 file changed, 45 insertions(+), 56 deletions(-) diff --git a/proto/bgp/attrs.c b/proto/bgp/attrs.c index c27a498..6e6f784 100644 --- a/proto/bgp/attrs.c +++ b/proto/bgp/attrs.c @@ -903,47 +903,17 @@ bgp_rt_notify(struct proto *P, rtable *tbl UNUSED, net *n, rte *new, rte *old UN bgp_schedule_packet(p->conn, PKT_UPDATE); } -static int +static void bgp_create_attrs(struct bgp_proto *p, rte *e, ea_list **attrs, struct linpool *pool) { - ea_list *ea = lp_alloc(pool, sizeof(ea_list) + 4*sizeof(eattr)); rta *rta = e->attrs; - byte *z; - ea->next = *attrs; - *attrs = ea; - ea->flags = EALF_SORTED; - ea->count = 4; - - bgp_set_attr(ea->attrs, BA_ORIGIN, + if (!ea_find(*attrs, EA_CODE(EAP_BGP, BA_ORIGIN))) + bgp_attach_attr(attrs, pool, BA_ORIGIN, ((rta->source == RTS_OSPF_EXT1) || (rta->source == RTS_OSPF_EXT2)) ? ORIGIN_INCOMPLETE : ORIGIN_IGP); - if (p->is_internal) - bgp_set_attr_wa(ea->attrs+1, pool, BA_AS_PATH, 0); - else - { - z = bgp_set_attr_wa(ea->attrs+1, pool, BA_AS_PATH, 6); - z[0] = AS_PATH_SEQUENCE; - z[1] = 1; /* 1 AS */ - put_u32(z+2, p->local_as); - } - - /* iBGP -> use gw, eBGP multi-hop -> use source_addr, - eBGP single-hop -> use gw if on the same iface */ - z = bgp_set_attr_wa(ea->attrs+2, pool, BA_NEXT_HOP, NEXT_HOP_LENGTH); - if (p->cf->next_hop_self || - rta->dest != RTD_ROUTER || - ipa_equal(rta->gw, IPA_NONE) || - ipa_has_link_scope(rta->gw) || - (!p->is_internal && !p->cf->next_hop_keep && - (!p->neigh || (rta->iface != p->neigh->iface)))) - set_next_hop(z, p->source_addr); - else - set_next_hop(z, rta->gw); - - bgp_set_attr(ea->attrs+3, BA_LOCAL_PREF, p->cf->default_local_pref); - - return 0; /* Leave decision to the filters */ + if (!ea_find(*attrs, EA_CODE(EAP_BGP, BA_LOCAL_PREF))) + bgp_attach_attr(attrs, pool, BA_LOCAL_PREF, p->cf->default_local_pref); } @@ -973,7 +943,16 @@ static inline void bgp_path_prepend(rte *e, ea_list **attrs, struct linpool *pool, u32 as) { eattr *a = ea_find(e->attrs->eattrs, EA_CODE(EAP_BGP, BA_AS_PATH)); - bgp_attach_attr(attrs, pool, BA_AS_PATH, (uintptr_t) as_path_prepend(pool, a->u.ptr, as)); + if (a) + bgp_attach_attr(attrs, pool, BA_AS_PATH, (uintptr_t) as_path_prepend(pool, a->u.ptr, as)); + else + { + byte *z; + z = bgp_attach_attr_wa(attrs, pool, BA_AS_PATH, 6); + z[0] = AS_PATH_SEQUENCE; + z[1] = 1; /* 1 AS */ + put_u32(z+2, as); + } } static inline void @@ -986,6 +965,7 @@ bgp_cluster_list_prepend(rte *e, ea_list **attrs, struct linpool *pool, u32 cid) static int bgp_update_attrs(struct bgp_proto *p, rte *e, ea_list **attrs, struct linpool *pool, int rr) { + rta *rta = e->attrs; eattr *a; if (!p->is_internal && !p->rs_client) @@ -1000,6 +980,8 @@ bgp_update_attrs(struct bgp_proto *p, rte *e, ea_list **attrs, struct linpool *p if (a) bgp_attach_attr(attrs, pool, BA_MULTI_EXIT_DISC, 0); } + else if (!ea_find(e->attrs->eattrs, EA_CODE(EAP_BGP, BA_AS_PATH))) + bgp_attach_attr_wa(attrs, pool, BA_AS_PATH, 0); /* iBGP -> keep next_hop, eBGP multi-hop -> use source_addr, * eBGP single-hop -> keep next_hop if on the same iface. @@ -1019,7 +1001,15 @@ bgp_update_attrs(struct bgp_proto *p, rte *e, ea_list **attrs, struct linpool *p { /* Need to create new one */ byte *b = bgp_attach_attr_wa(attrs, pool, BA_NEXT_HOP, NEXT_HOP_LENGTH); - set_next_hop(b, p->source_addr); + if (p->cf->next_hop_self || + rta->dest != RTD_ROUTER || + ipa_equal(rta->gw, IPA_NONE) || + ipa_has_link_scope(rta->gw) || + (!p->is_internal && !p->cf->next_hop_keep && + (!p->neigh || (rta->iface != p->neigh->iface)))) + set_next_hop(b, p->source_addr); + else + set_next_hop(b, rta->gw); } if (rr) @@ -1079,30 +1069,29 @@ bgp_import_control(struct proto *P, rte **new, ea_list **attrs, struct linpool * if (p == new_bgp) /* Poison reverse updates */ return -1; - if (new_bgp) - { - /* We should check here for cluster list loop, because the receiving BGP instance - might have different cluster ID */ - if (bgp_cluster_list_loopy(p, e->attrs)) - return -1; - if (p->cf->interpret_communities && bgp_community_filter(p, e)) - return -1; + if (!new_bgp) + bgp_create_attrs(p, e, attrs, pool); - if (p->local_as == new_bgp->local_as && p->is_internal && new_bgp->is_internal) - { - /* Redistribution of internal routes with IBGP */ - if (p->rr_client || new_bgp->rr_client) - /* Route reflection, RFC 4456 */ - return bgp_update_attrs(p, e, attrs, pool, 1); - else - return -1; - } + /* We should check here for cluster list loop, because the receiving BGP instance + might have different cluster ID */ + if (bgp_cluster_list_loopy(p, e->attrs)) + return -1; + + if (p->cf->interpret_communities && bgp_community_filter(p, e)) + return -1; + + if (new_bgp && p->local_as == new_bgp->local_as && p->is_internal && new_bgp->is_internal) + { + /* Redistribution of internal routes with IBGP */ + if (p->rr_client || new_bgp->rr_client) + /* Route reflection, RFC 4456 */ + return bgp_update_attrs(p, e, attrs, pool, 1); else - return bgp_update_attrs(p, e, attrs, pool, 0); + return -1; } else - return bgp_create_attrs(p, e, attrs, pool); + return bgp_update_attrs(p, e, attrs, pool, 0); } static inline u32 -- 1.8.2.3