On Tue, Jan 5, 2016 at 7:59 AM, Vivek Venkatraman <[email protected] > wrote:
> > On Tue, Jan 5, 2016 at 5:56 AM, Lou Berger <[email protected]> wrote: > >> Vivek, >> >> On 1/5/2016 7:55 AM, Vivek Venkatraman wrote: >> > I think it'd be really good if the new functions introduced for VPN >> > static routes (which take SAFI as the parameter) subsume the non-VPN >> > (i.e., IP unicast) static routes too, so we don't have duplicate code. >> > >> >> The genesis of these changes were the *existing* commands: >> bgp_mplsvpn.c:DEFUN (vpnv4_network, >> bgp_mplsvpn.c:DEFUN (no_vpnv4_network, >> bgp_routemap.c:DEFUN (set_vpnv4_nexthop, >> bgp_routemap.c:DEFUN (no_set_vpnv4_nexthop, >> bgp_routemap.c:ALIAS (no_set_vpnv4_nexthop, >> bgp_vty.c:DEFUN (address_family_vpnv4, >> bgp_vty.c:ALIAS (address_family_vpnv4, >> >> > Understood. But I did see a comment in this patch itself about the > potential to combine these (i.e., a single bgp_static_set() or > bgp_static_update() handles all the different AFI/SAFI) and I was stating > my preference for this. Of course, this could be incorporated as a > subsequent patch. > > >> > bgp_static_update_safi() is handling change to attribute (of an >> > existing VPN route) but its calling function bgp_static_set_safi() >> >> right, which replaces the old bgp_static_set_vpnv4 function >> > doesn't seem to be; compare with bgp_static_set(). >> > >> >> Can you elaborate on what you mean. Our code was a modification of >> bgp_static_set_vpnv4 rather than trying to modify / extend bgp_static_set. >> > > I'll get back on this tomorrow as I'm on a different system right now. But > basically, I was saying that while the call chain for IPv4/IPv6 networks > (bgp_static_set() and bgp_static_update()) handles changes to attributes, > the similar chain for VPNv4 doesn't appear to be. > In bgp_static_set_safi(), the following block of code will return if the same network were to be modified. if (rn->info) { vty_out (vty, "%% Same network configuration exists%s", VTY_NEWLINE); bgp_unlock_node (rn); } The corresponding portion in bgp_static_set() is different. This is what I was referring to. It is not terribly important right now, but would need to be taken care when bgp_static_set() and bgp_static_set_safi() are combined (my earlier point). > > >> >> > Overall, I'm unsure about the use cases for injecting (or removing) >> > VPN static routes (networks) directly. The common thing on a PE would >> > be to inject networks into appropriate VRFs and these would then get >> > exported by BGP. The original quagga code says these functions are for >> > "test purposes". Possibly, there are scenarios where a controller >> > would directly inject VPN routes into BGP? >> >> Not sure, but we were aiming to generalize/fix what was there. I think >> it's at least useful for testing and perhaps other uses that may come up >> in actual operation. We also weren't comfortable suggesting removing >> existing function without a good reason. >> > > Sure. This question was just to know if this interface has other > well-known uses. > > >> >> Thanks again, >> Lou >> >> > >> > On Thu, Dec 24, 2015 at 10:10 AM, Lou Berger <[email protected] >> > <mailto:[email protected]>> wrote: >> > >> > This is part of the core VPN and Encap SAFI changes. >> > >> > This changes the existing _vpnv4 functions for MPLS-VPN into >> > SAFI-agnostic functions, renaming them from *_vpnv4 to *_safi. >> > >> > Also adds route-map support while at it. >> > >> > Signed-off-by: Lou Berger <[email protected] <mailto: >> [email protected]>> >> > Reviewed-by: David Lamparter <[email protected] >> > <mailto:[email protected]>> >> > --- >> > bgpd/bgp_mplsvpn.c | 20 ++++- >> > bgpd/bgp_route.c | 212 >> > ++++++++++++++++++++++++++++++++++++++++------------- >> > bgpd/bgp_route.h | 6 +- >> > 3 files changed, 184 insertions(+), 54 deletions(-) >> > >> > diff --git a/bgpd/bgp_mplsvpn.c b/bgpd/bgp_mplsvpn.c >> > index a72d5ed..18627ad 100644 >> > --- a/bgpd/bgp_mplsvpn.c >> > +++ b/bgpd/bgp_mplsvpn.c >> > @@ -294,7 +294,22 @@ DEFUN (vpnv4_network, >> > "BGP tag\n" >> > "tag value\n") >> > { >> > - return bgp_static_set_vpnv4 (vty, argv[0], argv[1], argv[2]); >> > + return bgp_static_set_safi (SAFI_MPLS_VPN, vty, argv[0], >> > argv[1], argv[2], NULL); >> > +} >> > + >> > +DEFUN (vpnv4_network_route_map, >> > + vpnv4_network_route_map_cmd, >> > + "network A.B.C.D/M rd ASN:nn_or_IP-address:nn tag WORD >> > route-map WORD", >> > + "Specify a network to announce via BGP\n" >> > + "IP prefix <network>/<length>, e.g., 35.0.0.0/8\n >> <http://35.0.0.0/8%5Cn> >> > <http://35.0.0.0/8%5Cn>" >> > + "Specify Route Distinguisher\n" >> > + "VPN Route Distinguisher\n" >> > + "BGP tag\n" >> > + "tag value\n" >> > + "route map\n" >> > + "route map name\n") >> > +{ >> > + return bgp_static_set_safi (SAFI_MPLS_VPN, vty, argv[0], >> > argv[1], argv[2], argv[3]); >> > } >> > >> > /* For testing purpose, static route of MPLS-VPN. */ >> > @@ -309,7 +324,7 @@ DEFUN (no_vpnv4_network, >> > "BGP tag\n" >> > "tag value\n") >> > { >> > - return bgp_static_unset_vpnv4 (vty, argv[0], argv[1], argv[2]); >> > + return bgp_static_unset_safi (SAFI_MPLS_VPN, vty, argv[0], >> > argv[1], argv[2]); >> > } >> > >> > static int >> > @@ -722,6 +737,7 @@ void >> > bgp_mplsvpn_init (void) >> > { >> > install_element (BGP_VPNV4_NODE, &vpnv4_network_cmd); >> > + install_element (BGP_VPNV4_NODE, &vpnv4_network_route_map_cmd); >> > install_element (BGP_VPNV4_NODE, &no_vpnv4_network_cmd); >> > >> > >> > diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c >> > index 316fa5a..9866e37 100644 >> > --- a/bgpd/bgp_route.c >> > +++ b/bgpd/bgp_route.c >> > @@ -3614,39 +3614,6 @@ bgp_static_update (struct bgp *bgp, struct >> > prefix *p, >> > } >> > } >> > >> > -static void >> > -bgp_static_update_vpnv4 (struct bgp *bgp, struct prefix *p, afi_t >> > afi, >> > - safi_t safi, struct prefix_rd *prd, >> > u_char *tag) >> > -{ >> > - struct bgp_node *rn; >> > - struct bgp_info *new; >> > - >> > - rn = bgp_afi_node_get (bgp->rib[afi][safi], afi, safi, p, prd); >> > - >> > - /* Make new BGP info. */ >> > - new = bgp_info_new (); >> > - new->type = ZEBRA_ROUTE_BGP; >> > - new->sub_type = BGP_ROUTE_STATIC; >> > - new->peer = bgp->peer_self; >> > - new->attr = bgp_attr_default_intern (BGP_ORIGIN_IGP); >> > - SET_FLAG (new->flags, BGP_INFO_VALID); >> > - new->uptime = bgp_clock (); >> > - new->extra = bgp_info_extra_new(); >> > - memcpy (new->extra->tag, tag, 3); >> > - >> > - /* Aggregate address increment. */ >> > - bgp_aggregate_increment (bgp, p, new, afi, safi); >> > - >> > - /* Register new BGP information. */ >> > - bgp_info_add (rn, new); >> > - >> > - /* route_node_get lock */ >> > - bgp_unlock_node (rn); >> > - >> > - /* Process change. */ >> > - bgp_process (bgp, rn, afi, safi); >> > -} >> > - >> > void >> > bgp_static_withdraw (struct bgp *bgp, struct prefix *p, afi_t afi, >> > safi_t safi) >> > @@ -3695,9 +3662,12 @@ bgp_check_local_routes_rsclient (struct >> > peer *rsclient, afi_t afi, safi_t safi) >> > } >> > } >> > >> > +/* >> > + * Used for SAFI_MPLS_VPN and SAFI_ENCAP >> > + */ >> > static void >> > -bgp_static_withdraw_vpnv4 (struct bgp *bgp, struct prefix *p, >> > afi_t afi, >> > - safi_t safi, struct prefix_rd *prd, >> > u_char *tag) >> > +bgp_static_withdraw_safi (struct bgp *bgp, struct prefix *p, >> > afi_t afi, >> > + safi_t safi, struct prefix_rd *prd, >> > u_char *tag) >> > { >> > struct bgp_node *rn; >> > struct bgp_info *ri; >> > @@ -3723,6 +3693,131 @@ bgp_static_withdraw_vpnv4 (struct bgp >> > *bgp, struct prefix *p, afi_t afi, >> > bgp_unlock_node (rn); >> > } >> > >> > +static void >> > +bgp_static_update_safi (struct bgp *bgp, struct prefix *p, >> > + struct bgp_static *bgp_static, afi_t afi, >> > safi_t safi) >> > +{ >> > + struct bgp_node *rn; >> > + struct bgp_info *new; >> > + struct attr *attr_new; >> > + struct attr attr = { 0 }; >> > + struct bgp_info *ri; >> > + >> > + assert (bgp_static); >> > + >> > + rn = bgp_afi_node_get (bgp->rib[afi][safi], afi, safi, p, >> > &bgp_static->prd); >> > + >> > + bgp_attr_default_set (&attr, BGP_ORIGIN_IGP); >> > + >> > + attr.nexthop = bgp_static->igpnexthop; >> > + attr.med = bgp_static->igpmetric; >> > + attr.flag |= ATTR_FLAG_BIT (BGP_ATTR_MULTI_EXIT_DISC); >> > + >> > + /* Apply route-map. */ >> > + if (bgp_static->rmap.name <http://rmap.name>) >> > + { >> > + struct attr attr_tmp = attr; >> > + struct bgp_info info; >> > + int ret; >> > + >> > + info.peer = bgp->peer_self; >> > + info.attr = &attr_tmp; >> > + >> > + SET_FLAG (bgp->peer_self->rmap_type, PEER_RMAP_TYPE_NETWORK); >> > + >> > + ret = route_map_apply (bgp_static->rmap.map, p, RMAP_BGP, >> > &info); >> > + >> > + bgp->peer_self->rmap_type = 0; >> > + >> > + if (ret == RMAP_DENYMATCH) >> > + { >> > + /* Free uninterned attribute. */ >> > + bgp_attr_flush (&attr_tmp); >> > + >> > + /* Unintern original. */ >> > + aspath_unintern (&attr.aspath); >> > + bgp_attr_extra_free (&attr); >> > + bgp_static_withdraw_safi (bgp, p, afi, safi, >> > &bgp_static->prd, >> > + bgp_static->tag); >> > + return; >> > + } >> > + >> > + attr_new = bgp_attr_intern (&attr_tmp); >> > + } >> > + else >> > + { >> > + attr_new = bgp_attr_intern (&attr); >> > + } >> > + >> > + for (ri = rn->info; ri; ri = ri->next) >> > + if (ri->peer == bgp->peer_self && ri->type == ZEBRA_ROUTE_BGP >> > + && ri->sub_type == BGP_ROUTE_STATIC) >> > + break; >> > + >> > + if (ri) >> > + { >> > + if (attrhash_cmp (ri->attr, attr_new) && >> > + !CHECK_FLAG(ri->flags, BGP_INFO_REMOVED)) >> > + { >> > + bgp_unlock_node (rn); >> > + bgp_attr_unintern (&attr_new); >> > + aspath_unintern (&attr.aspath); >> > + bgp_attr_extra_free (&attr); >> > + return; >> > + } >> > + else >> > + { >> > + /* The attribute is changed. */ >> > + bgp_info_set_flag (rn, ri, BGP_INFO_ATTR_CHANGED); >> > + >> > + /* Rewrite BGP route information. */ >> > + if (CHECK_FLAG(ri->flags, BGP_INFO_REMOVED)) >> > + bgp_info_restore(rn, ri); >> > + else >> > + bgp_aggregate_decrement (bgp, p, ri, afi, safi); >> > + bgp_attr_unintern (&ri->attr); >> > + ri->attr = attr_new; >> > + ri->uptime = bgp_clock (); >> > + >> > + /* Process change. */ >> > + bgp_aggregate_increment (bgp, p, ri, afi, safi); >> > + bgp_process (bgp, rn, afi, safi); >> > + bgp_unlock_node (rn); >> > + aspath_unintern (&attr.aspath); >> > + bgp_attr_extra_free (&attr); >> > + return; >> > + } >> > + } >> > + >> > + >> > + /* Make new BGP info. */ >> > + new = bgp_info_new (); >> > + new->type = ZEBRA_ROUTE_BGP; >> > + new->sub_type = BGP_ROUTE_STATIC; >> > + new->peer = bgp->peer_self; >> > + new->attr = attr_new; >> > + SET_FLAG (new->flags, BGP_INFO_VALID); >> > + new->uptime = bgp_clock (); >> > + new->extra = bgp_info_extra_new(); >> > + memcpy (new->extra->tag, bgp_static->tag, 3); >> > + >> > + /* Aggregate address increment. */ >> > + bgp_aggregate_increment (bgp, p, new, afi, safi); >> > + >> > + /* Register new BGP information. */ >> > + bgp_info_add (rn, new); >> > + >> > + /* route_node_get lock */ >> > + bgp_unlock_node (rn); >> > + >> > + /* Process change. */ >> > + bgp_process (bgp, rn, afi, safi); >> > + >> > + /* Unintern original. */ >> > + aspath_unintern (&attr.aspath); >> > + bgp_attr_extra_free (&attr); >> > +} >> > + >> > /* Configure static BGP network. When user don't run zebra, static >> > route should be installed as valid. */ >> > static int >> > @@ -3893,8 +3988,8 @@ bgp_static_delete (struct bgp *bgp) >> > for (rm = bgp_table_top (table); rm; rm = >> > bgp_route_next (rm)) >> > { >> > bgp_static = rn->info; >> > - bgp_static_withdraw_vpnv4 (bgp, &rm->p, >> > - AFI_IP, >> SAFI_MPLS_VPN, >> > + bgp_static_withdraw_safi (bgp, &rm->p, >> > + AFI_IP, safi, >> > (struct prefix_rd >> > *)&rn->p, >> > bgp_static->tag); >> > bgp_static_free (bgp_static); >> > @@ -3913,9 +4008,15 @@ bgp_static_delete (struct bgp *bgp) >> > } >> > } >> > >> > +/* >> > + * gpz 110624 >> > + * Currently this is used to set static routes for VPN and ENCAP. >> > + * I think it can probably be factored with bgp_static_set. >> > + */ >> > int >> > -bgp_static_set_vpnv4 (struct vty *vty, const char *ip_str, const >> > char *rd_str, >> > - const char *tag_str) >> > +bgp_static_set_safi (safi_t safi, struct vty *vty, const char >> > *ip_str, >> > + const char *rd_str, const char *tag_str, >> > + const char *rmap_str) >> > { >> > int ret; >> > struct prefix p; >> > @@ -3951,10 +4052,10 @@ bgp_static_set_vpnv4 (struct vty *vty, >> > const char *ip_str, const char *rd_str, >> > return CMD_WARNING; >> > } >> > >> > - prn = bgp_node_get (bgp->route[AFI_IP][SAFI_MPLS_VPN], >> > + prn = bgp_node_get (bgp->route[AFI_IP][safi], >> > (struct prefix *)&prd); >> > if (prn->info == NULL) >> > - prn->info = bgp_table_init (AFI_IP, SAFI_MPLS_VPN); >> > + prn->info = bgp_table_init (AFI_IP, safi); >> > else >> > bgp_unlock_node (prn); >> > table = prn->info; >> > @@ -3970,11 +4071,24 @@ bgp_static_set_vpnv4 (struct vty *vty, >> > const char *ip_str, const char *rd_str, >> > { >> > /* New configuration. */ >> > bgp_static = bgp_static_new (); >> > - bgp_static->valid = 1; >> > - memcpy (bgp_static->tag, tag, 3); >> > + bgp_static->backdoor = 0; >> > + bgp_static->valid = 0; >> > + bgp_static->igpmetric = 0; >> > + bgp_static->igpnexthop.s_addr = 0; >> > + memcpy(bgp_static->tag, tag, 3); >> > + bgp_static->prd = prd; >> > + >> > + if (rmap_str) >> > + { >> > + if (bgp_static->rmap.name <http://rmap.name>) >> > + free (bgp_static->rmap.name <http://rmap.name>); >> > + bgp_static->rmap.name <http://rmap.name> = strdup >> > (rmap_str); >> > + bgp_static->rmap.map = route_map_lookup_by_name >> (rmap_str); >> > + } >> > rn->info = bgp_static; >> > >> > - bgp_static_update_vpnv4 (bgp, &p, AFI_IP, SAFI_MPLS_VPN, >> > &prd, tag); >> > + bgp_static->valid = 1; >> > + bgp_static_update_safi (bgp, &p, bgp_static, AFI_IP, safi); >> > } >> > >> > return CMD_SUCCESS; >> > @@ -3982,8 +4096,8 @@ bgp_static_set_vpnv4 (struct vty *vty, const >> > char *ip_str, const char *rd_str, >> > >> > /* Configure static BGP network. */ >> > int >> > -bgp_static_unset_vpnv4 (struct vty *vty, const char *ip_str, >> > - const char *rd_str, const char *tag_str) >> > +bgp_static_unset_safi(safi_t safi, struct vty *vty, const char >> > *ip_str, >> > + const char *rd_str, const char *tag_str) >> > { >> > int ret; >> > struct bgp *bgp; >> > @@ -4020,10 +4134,10 @@ bgp_static_unset_vpnv4 (struct vty *vty, >> > const char *ip_str, >> > return CMD_WARNING; >> > } >> > >> > - prn = bgp_node_get (bgp->route[AFI_IP][SAFI_MPLS_VPN], >> > + prn = bgp_node_get (bgp->route[AFI_IP][safi], >> > (struct prefix *)&prd); >> > if (prn->info == NULL) >> > - prn->info = bgp_table_init (AFI_IP, SAFI_MPLS_VPN); >> > + prn->info = bgp_table_init (AFI_IP, safi); >> > else >> > bgp_unlock_node (prn); >> > table = prn->info; >> > @@ -4032,7 +4146,7 @@ bgp_static_unset_vpnv4 (struct vty *vty, >> > const char *ip_str, >> > >> > if (rn) >> > { >> > - bgp_static_withdraw_vpnv4 (bgp, &p, AFI_IP, SAFI_MPLS_VPN, >> > &prd, tag); >> > + bgp_static_withdraw_safi (bgp, &p, AFI_IP, safi, &prd, tag); >> > >> > bgp_static = rn->info; >> > bgp_static_free (bgp_static); >> > diff --git a/bgpd/bgp_route.h b/bgpd/bgp_route.h >> > index 3d2eea5..3e7f6f5 100644 >> > --- a/bgpd/bgp_route.h >> > +++ b/bgpd/bgp_route.h >> > @@ -209,10 +209,10 @@ extern void bgp_static_update (struct bgp *, >> > struct prefix *, struct bgp_static >> > afi_t, safi_t); >> > extern void bgp_static_withdraw (struct bgp *, struct prefix *, >> > afi_t, safi_t); >> > >> > -extern int bgp_static_set_vpnv4 (struct vty *vty, const char *, >> > - const char *, const char *); >> > +extern int bgp_static_set_safi (safi_t safi, struct vty *vty, >> > const char *, >> > + const char *, const char *, const char >> *); >> > >> > -extern int bgp_static_unset_vpnv4 (struct vty *, const char *, >> > +extern int bgp_static_unset_safi (safi_t safi, struct vty *, >> > const char *, >> > const char *, const char *); >> > >> > /* this is primarily for MPLS-VPN */ >> > -- >> > 2.1.3 >> > >> > >> > _______________________________________________ >> > Quagga-dev mailing list >> > [email protected] <mailto:[email protected]> >> > https://lists.quagga.net/mailman/listinfo/quagga-dev >> > >> > >> >> >> >
_______________________________________________ Quagga-dev mailing list [email protected] https://lists.quagga.net/mailman/listinfo/quagga-dev
