On Fri, Jun 17, 2016 at 9:28 AM, Paul Jakma <[email protected]> wrote:
> 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>
> }
>
?
ok I see where our delta is coming from....later on after this patch we
have another patch so that members of a peer-group can have different
outbound policies. The update-group code is smart enough to put two peers
in a peer-group into different update-groups of those peers have different
outbound policies. As part of that patch I had to rework these
REMOVE_PRIVATE_AS flags so that they are all treated uniquely...so in our
code we are doing:
if (peer->sort == BGP_PEER_EBGP &&
(peer_af_flag_check (peer, afi, safi,
PEER_FLAG_REMOVE_PRIVATE_AS_ALL_REPLACE) ||
peer_af_flag_check (peer, afi, safi,
PEER_FLAG_REMOVE_PRIVATE_AS_REPLACE) ||
peer_af_flag_check (peer, afi, safi,
PEER_FLAG_REMOVE_PRIVATE_AS_ALL) ||
peer_af_flag_check (peer, afi, safi, PEER_FLAG_REMOVE_PRIVATE_AS)))
but yeah for the patch you are looking at I think your change would be fine.
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?
So only offer "replace" if "all" was specified?
Daniel
_______________________________________________
Quagga-dev mailing list
[email protected]
https://lists.quagga.net/mailman/listinfo/quagga-dev
