On Fri, Jun 17, 2016 at 5:33 AM, Paul Jakma <p...@jakma.org> wrote:
> Oh,
>
> Just another thing, the bgp_route.c::bgp_peer_remove_private_as function
> wasn't completely clear ot me. I think the logic simplifies out to at least:
>
> /* If this is an EBGP peer with remove-private-AS */
> static void
> bgp_peer_remove_private_as (struct bgp *bgp, afi_t afi, safi_t safi,
> struct peer *peer, struct attr *attr)
> {
> if (peer->sort != BGP_PEER_EBGP)
> return;
> if (!peer_af_flag_check (peer, afi, safi, PEER_FLAG_REMOVE_PRIVATE_AS))
> return;
>
This check wouldn't work because PEER_FLAG_REMOVE_PRIVATE_AS is not set if
the user did "remove-private-AS all", PEER_FLAG_REMOVE_PRIVATE_AS_ALL would
be set in that scenario.
root@superm-redxp-05[bgpd]# grep PEER_FLAG_REMOVE_PRIVATE_AS bgpd.h
#define PEER_FLAG_REMOVE_PRIVATE_AS (1 << 10) /* remove-private-as
*/
#define PEER_FLAG_REMOVE_PRIVATE_AS_ALL (1 << 18) /* remove-private-as
all */
#define PEER_FLAG_REMOVE_PRIVATE_AS_REPLACE (1 << 19) /* remove-private-as
replace-as */
#define PEER_FLAG_REMOVE_PRIVATE_AS_ALL_REPLACE (1 << 21) /*
remove-private-as all replace-as */
root@superm-redxp-05[bgpd]#
>
> /* Take action on the entire aspath */
> if (peer_af_flag_check (peer, afi, safi,
> PEER_FLAG_REMOVE_PRIVATE_AS_ALL))
> {
> if (peer_af_flag_check (peer, afi, safi,
> PEER_FLAG_REMOVE_PRIVATE_AS_REPLA
> attr->aspath = aspath_replace_private_asns (attr->aspath, bgp->as);
> else
> attr->aspath = aspath_remove_private_asns (attr->aspath);
> }
>
> /* 'all' was not specified so the entire aspath must be private ASNs
> for us to do anything */
> else if (aspath_private_as_check (attr->aspath))
> {
> if (peer_af_flag_check (peer, afi, safi,
> PEER_FLAG_REMOVE_PRIVATE_AS_REPLA
> attr->aspath = aspath_replace_private_asns (attr->aspath, bgp->as);
> else
> attr->aspath = aspath_empty_get ();
> }
> }
>
> Which raises the question:
>
> If the user has not specified 'all', but has specified 'replace' wouldn't
> the action of least-surprise be to replace private-ASes regardless of
> whether the AS_PATH has public ones?
>
>
This is one of those things were other vendors only honored
remove-private-AS if the entire AS_PATH were made of private ASNs so quagga
did the same but then someone wanted to always be able to always remove the
private ASNs no matter what so the "all" keyword was added. It does make
me wonder how many of these remove-private permutations actually get used
in the real world.
I'm not opposed to simplifying the options here...the low hanging fruit to
me would be to drop "all" but always behave as if "all" were configured.
Then you would just have "remove-private-as" and "remove-private-as
replace-as".
Daniel
> I.e. could it be simplied further, and just have 'replace' have an
> unconditional meaning, so it's:
>
> ...
> if (peer_af_flag_check (peer, afi, safi,
> PEER_FLAG_REMOVE_PRIVATE_AS_REPLACE))
> {
> attr->aspath = aspath_replace_private_asns (attr->aspath, bgp->as);
> return;
> }
>
> if (peer_af_flag_check (peer, afi, safi,
> PEER_FLAG_REMOVE_PRIVATE_AS_ALL))
> attr->aspath = aspath_remove_private_asns (attr->aspath);
> else if (aspath_private_as_check (attr->aspath))
> attr->aspath = aspath_empty_get ();
>
> return;
> }
>
> Do we need all 2^2+1 different ways to twiddle with private-ASes? Is it
> useful to have a "only do something if the whole AS_PATH is private"
> variable for a new feature?
>
> I.e. 1 flag to select between new and old behavior for
> 'remove-private-as', and replace can just be 'replace any private ASes'?
>
> regards,
> --
> Paul Jakma | p...@jakma.org | @pjakma | Key ID: 0xD86BF79464A2FF6A
> Fortune:
> Isn't it conceivable to you that an intelligent person could harbor
> two opposing ideas in his mind?
> -- Adlai Stevenson, to reporters
>
_______________________________________________
Quagga-dev mailing list
Quagga-dev@lists.quagga.net
https://lists.quagga.net/mailman/listinfo/quagga-dev
