fix: prevent AS loop

[maxime-peim]

Revert #2488
We should not allow a path with an AS loop to be advertised.
iBGP peers should not add locally originated routes with there own AS in their path.

[fujita]

I agree that this is the expected behavior. Hmm, I can't recall why I merged #2488

[fujita]

I thought that It would be better to allow whatever a user wants to do (even it's a mistake from the BGP perspective). No?

[maxime-peim]

Well, I don't think this is a good idea to break from BGP behavior to allow mistakes made by the user.

[fujita]

@floatingstatic Opinions?

[floatingstatic]

@fujita This will break an application I built around gobgp where I need to advertise some routes with an origin ASN that matches the ASN of an iBGP peer in some but not all cases. Can I suggest we make a configuration option to enable the behavior I need instead of reverting? We can make this disabled by default which should suffice for @maxime-peim 's needs. I'm happy to do the work for this and can open a PR later today to implement this if that works for you.

[maxime-peim]

Hi @floatingstatic,
I don't really understand what you are calling origin ASN. From your original post I am assuming you are talking about the AS path, right?
In that case wouldn't it be better in our case to have an export policy that does that instead?

[floatingstatic]

@maxime-peim Sorry for confusion due to my wording. For my use case the paths that are filtered without this check are routes i originate so I used the term "origin asn" but the as-path loop logic doesn't actually care where the asn is in the path generally. To your question about why I can't use an export policy is because the as-path loop check happens before any policy is applied (see prePolicyFilterpath() function in the BgpServer type).

I have a branch I'm testing right now that just adds a per-neighbor/peer option to allow me to get the behavior I need for my traffic engineering application to function and by default it would revert to behavior prior to #2448 so that should make all of us happy :)

[mpeim]

The check isASLoop that you were trying to avoid is only comparing AS from the AS path attribute.
Concerning the export policy, indeed the isASLoop check is perform in prePolicyFilterpath which is called before applying export policies (in BgpServer.filterpath). So you could have an export policy that add your ASN in the AS path without adding it beforehand in the path.

[floatingstatic]

@maxime-peim yep got it. i understand how the as-path check works. regarding the export policy thing, without explaining in great detail how my application works, no this is not possible as we are mirroring routes that already have as-path set.
PR is coming. I don't feel we need to debate this further.

[mpeim]

@floatingstatic perfect thanks!

[floatingstatic]

@fujita I'd like to propose we go with this instead of this revert PR:

#2883

Open to comments about the config option name or what struct you feel this should go under. I tested pulling my fork into my route injector application and setting this configuration option allows me to get what I want and not setting it (default) reverts to behavior @maxime-peim is expecting.