Handling AGGREGATOR in v2.0.7

[Nasato Goto]

Hi Ondrej,

Thank you!
Could you tell me will it be merged on next release, v2.0.8?

> Just wondering whether you noticed this issue during testing or during
> operation (so there are still some 2B-AS-only implementations in the
> wild.
I found this through our test suite. We faced with this kind of issue in production few years ago.
Currently, we still have only few customers who don't support 4byte-AS.

Best regards,

-----
Nasato Goto
JPNAP / INTERNET MULTIFEED CO.
goto at mfeed.ad.jp
-----

2020/04/15 0:12 に、"Ondrej Zajicek" <santiago at crfreenet.org> を書き込みました:

    On Tue, Apr 14, 2020 at 02:45:29PM +0000, Nasato Goto wrote:
    > Hi all,
    > 
    > I found a bit strange behavior of handling AGGREGATOR attributes in BIRD v2.0.7 route server.
    > When BIRD receives a route containing AGGREGATOR attribute from 4byte-AS capable peer,
    > it sends the route with strange AGGREGATOR to 4byte-AS incapable peer.
    > On the other hand, BIRD can sends AS4_AGGREGATOR attribute correctly.
    > 
    > Here is a scenario what I did.
    > In the following simple topology, Peer1 and BIRD RS are 4byte-AS capable and Peer2 is 2byte-AS support only.
    > 
    > Peer1 --(r1)--> BIRD RS --(r1’)--> Peer2
    >
    > ...
    > 
    > Then I tried a small patch, this fixed my problem.
    > But I’m not sure if it’s appropriate.
    
    Hi
    
    Thanks for the bugreport and the patch. It is fully appropriate, the
    second part is a clear omission on my side. Will merge it.
    
    Just wondering whether you noticed this issue during testing or during
    operation (so there are still some 2B-AS-only implementations in the
    wild.
    
    
    > ------------------------------------------
    > diff --git a/nest/attrs.h b/nest/attrs.h
    > index 6fb0a8fa..e0399708 100644
    > --- a/nest/attrs.h
    > +++ b/nest/attrs.h
    > @@ -111,7 +111,7 @@ static inline struct adata *
    > aggregator_to_old(struct linpool *pool, const struct adata *a)
    > {
    >    struct adata *d = lp_alloc_adata(pool, 8);
    > -  put_u32(d->data, 0xFFFF);
    > +  put_u32(d->data, AS_TRANS);
    >    memcpy(d->data + 4, a->data + 4, 4);
    >    return d;
    > }
    > diff --git a/proto/bgp/attrs.c b/proto/bgp/attrs.c
    > index b8921363..f2a4d5b2 100644
    > --- a/proto/bgp/attrs.c
    > +++ b/proto/bgp/attrs.c
    > @@ -562,6 +562,7 @@ bgp_encode_aggregator(struct bgp_write_state *s, eattr *a, byte *buf, uint size)
    >      /* Prepare 16-bit AGGREGATOR (from 32-bit one) in a temporary buffer */
    >      byte *dst = alloca(6);
    >      len = aggregator_32to16(dst, data);
    > +    data = dst;
    >    }
    > 
    >    return bgp_put_attr(buf, size, BA_AGGREGATOR, a->flags, data, len);
    > ------------------------------------------
    > 
    > If further information is needed I will be happy to provide it.
    > 
    > 
    > Best regards,
    > 
    > -----
    > Nasato Goto
    > JPNAP / INTERNET MULTIFEED CO.
    > goto at mfeed.ad.jp
    > -----
    
    -- 
    Elen sila lumenn' omentielvo
    
    Ondrej 'Santiago' Zajicek (email: santiago at crfreenet.org)
    OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
    "To err is human -- to blame it on a computer is even more so."