On Fri, 17 Jun 2016, Daniel Walton wrote:
/* 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.
Hmm, but in the version of the patch I have the whole block is
predicated on both the above being true:
+ if (peer->sort == BGP_PEER_EBGP &&
+ peer_af_flag_check (peer, afi, safi, PEER_FLAG_REMOVE_PRIVATE_AS))
+ {
<rest of the logic>
}
?
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 */
Hmm, reading the patch it looked like the logic expects
PEER_FLAG_REMOVE_PRIVATE_AS to be like an umbrella config option that is
always set if REPLACE or ALL_REPLACE are set, which are then
sub-options?
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
Yeah, sure. So we have to keep that behaviour on the base command of
course.
Just wondering if we need to offer both "all must be private first" and
"act on any" for the new 'replace' case?
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".
Not sure we could change 'remove-private-as'. We have freedom on
'replace-as' though I think, unless there's precedent there with others.
regards,
--
Paul Jakma | [email protected] | @pjakma | Key ID: 0xD86BF79464A2FF6A
Fortune:
Finagle's Creed:
Science is true. Don't be misled by facts.
_______________________________________________
Quagga-dev mailing list
[email protected]
https://lists.quagga.net/mailman/listinfo/quagga-dev
