New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MetalLB incorrectly uses AS_SET to specify the originating ASN in path attributes #225

Closed
mhristache opened this Issue Apr 12, 2018 · 10 comments

Comments

Projects
2 participants
@mhristache
Copy link

mhristache commented Apr 12, 2018

Is this a bug report or a feature request?:

bug

What happened:

The BGP AS number can be either 2 bytes (when the value is from 1 to 65535) or 4 bytes (when the value is from 65536 to 4294967295) but Metal LB is always using 4 bytes (even for values lower than 65536).

This is causing compatibility issues with routers from e.g. Cisco, Ericsson etc as they have a strict check and reject AS numbers which are not compatible with the above stated rule.

See this article from Cisco with details about their implementation of 4 bytes AS numbers.

For example, Ericsson routers reject the BGP Update sent by Metal LB when the AS number is lower than 65536 with this error:

UPDATE problem  - aspath invalid - action to be taken: withdraw prefixes in update

The result is that the Services IPs are not propagated to the routers.

What you expected to happen:

AS numbers lower than 65536 should be encoded as 2 bytes

How to reproduce it (as minimally and precisely as possible):

Use the configuration in the tutorial with a BGP router from Ericsson.

Anything else we need to know?:

Environment:

  • MetalLB version: v0.5.0
  • Kubernetes version: 1.10
  • BGP router type/version: Ericsson R6672
  • OS (e.g. from /etc/os-release): Arch linux latest
  • Kernel (e.g. uname -a): 4.15.8-1-ARCH

@danderson danderson added the bug label Apr 12, 2018

@danderson danderson added this to To Do in BGP mode via automation Apr 12, 2018

@danderson danderson added this to the v0.6.0 milestone Apr 12, 2018

@danderson danderson self-assigned this Apr 12, 2018

@danderson

This comment has been minimized.

Copy link
Member

danderson commented Apr 12, 2018

Thank you for the detailed bug report! I'm surprised that cisco is that intolerant of using 4B ASN everywhere... But fortunately, this should be an easy fix. I'll prepare a patch tomorrow, after sleep.

@mhristache

This comment has been minimized.

Copy link

mhristache commented Apr 12, 2018

Hold on .. :)

After doing a bit more research, the bug might be on the Router side.

From RFC 6793:

   When a NEW BGP speaker processes an OPEN message from another NEW BGP
   speaker, it MUST use the AS number encoded in the Capability Value
   field of the "support for four-octet AS number capability" in lieu of
   the "My Autonomous System" field of the OPEN message.

   A BGP speaker that advertises such a capability to a particular peer,
   and receives from that peer the advertisement of such a capability,
   MUST encode AS numbers as four-octet entities in both the AS_PATH
   attribute and the AGGREGATOR attribute in the updates it sends to the
   peer and MUST assume that these attributes in the updates received
   from the peer encode AS numbers as four-octet entities

My understanding: the Metal LB behaviour is valid if the it negotiated the use of 4 bytes ASN with the router when BGP session was established.

So, it could also be a bug on the Cisco side (or the Cisco is using a version not compatible with RFC 6793).

I will do a trace to see what is negotiated exactly.

Thanks and please don't spend any time on this until it's more clear where the bug is. Feel free to close the issue if you want to and I can reopen it if I am sure the bug in on the Metal LB side.

//Max

@mhristache

This comment has been minimized.

Copy link

mhristache commented Apr 12, 2018

Somehow similar discussions here

@danderson

This comment has been minimized.

Copy link
Member

danderson commented Apr 12, 2018

Based on that discussion you linked, I think the bug could be in either. The RFC says if both routers agree to use 4B ASN, then the AS_PATH attribute can encode 4b ASNs. So, if that's not happening, 2 possibilities: either the peers are not configuring 4B ASN (but in that case metallb should be detecting that and rejecting the connection), or the router has a bug and rejects valid protocol messages.

I'm digging through metallb code now to check the former.

One useful thing, if you can, would be to get a pcap of the BGP session (from establishment all the way to the disconnection). That way I can dissect exactly what the packets are saying, and compare to the spec.

@danderson

This comment has been minimized.

Copy link
Member

danderson commented Apr 12, 2018

OK, I see that my BGP OPEN parsing code does not check that the peer advertised 4B ASN support. So, if your router is configured to not use 4B ASN, we could have this issue, where MetalLB behaves as if the session is full 4B ASN compliant, but the peer is not expecting this.

Can you check if there's an option on your router(s) to force 4B ASN on? And, if you can capture a pcap of the BGP exchange, that would allow us to confirm that this is happening.

If this is the problem, I think the fix is relatively simple, it just makes path attribute encoding a little bit more complicated.

@mhristache

This comment has been minimized.

Copy link

mhristache commented Apr 13, 2018

Hi again

I have done some more troubleshooting and the router supports 4bytes ASNs so that should not be the issue (see the attached traces).

I think the issue is actually related to the AS_PATH segment type.

I see that Metal LB is using AS_SET as AS path segment type and I think that is what the Router does not like.

According to BGP RFC 4271 the originating external router (MetalLB in this case) must use AS_SEQUENCE segment type in the UPDATE message:

   When a BGP speaker originates a route then:

      a) the originating speaker includes its own AS number in a path
         segment, of type AS_SEQUENCE, in the AS_PATH attribute of all
         UPDATE messages sent to an external peer.  In this case, the AS
         number of the originating speaker's autonomous system will be
         the only entry the path segment, and this path segment will be
         the only segment in the AS_PATH attribute.

      b) the originating speaker includes an empty AS_PATH attribute in
         all UPDATE messages sent to internal peers.  (An empty AS_PATH
         attribute is one whose length field contains the value zero).

So, I think the issue would be solved if Metal LB would use AS_SEQUENCE instead of AS_SET.

I tried to switch to iBGP between MetalLB and the router (I configured the same ASN in the LB and the router) and then the router is accepting the UPDATE messages and the routes get propagated.
When iBGP is used the AS_PATH is empty and I guess that is why it is working with iBGP.

I have attached traces with both iBGP and eBGP with connection setup and route updates.

//Max

traces.tar.gz

@mhristache

This comment has been minimized.

Copy link

mhristache commented Apr 13, 2018

Out of curiosity, how come you decided to implement your own BGP support instead of using e.g. gobgp?

@danderson

This comment has been minimized.

Copy link
Member

danderson commented Apr 14, 2018

Well, that's embarrassing. I'm surprised other BGP stacks let me get away with that mistake for all this time! Thank you for the very detailed research! Pushing a patch to fix this very soon.

As for "why not gobgp", good question! It was a deliberate choice between tradeoffs. I'll quote myself from a few months ago, when someone else asked the question:


Good question! The initial prototype used gobgp, but I kept having issues and headaches with it. The
API surface is completely undocumented (both the Go API and the gRPC API), and is very un-idiomatic
Go. So when I (frequently) encountered strange behaviors, it was a chore to debug. The last time I tried
using it, the library was also full of race conditions (causing go test -race failures), which did not inspire
confidence.

Aside from that, GoBGP also implements way more stuff than MetalLB needs. It wants to be a real
router, implementing the full BGP convergence algorithm and all that. All that behavior is actively harmful
to what MetalLB wants to do (which is just push a couple of routes and ignore everything the peer
sends). It's possible to make it work, by segmenting things into VRFs, adding policies to drop all inbound
routes, ... But at that point I'm writing significant amounts of code, against an undocumented library that
implements 10x what we need, to trick it into not doing 99% of what it tries to do by default. That's a lot
of additional complexity just to serialize and transmit an update message.

With that said, there's obviously downsides as well, and the big one is that OSRG has money to buy
vendor gear and do interop testing on their BGP stacks, and I don't.


danderson added a commit to danderson/metallb that referenced this issue Apr 14, 2018

Correctly use AS_SEQUENCE in path attributes.
The spec requires route originators to specify their ASN
in an AS_SEQUENCE path segment (which makes sense if you
think about it... AS_SET encodes a path that goes "via one
of these ASNs", which is impossible fo the *originating* ASN).

Fixes google#225.

BGP mode automation moved this from To Do to Done Apr 14, 2018

danderson added a commit that referenced this issue Apr 14, 2018

Correctly use AS_SEQUENCE in path attributes.
The spec requires route originators to specify their ASN
in an AS_SEQUENCE path segment (which makes sense if you
think about it... AS_SET encodes a path that goes "via one
of these ASNs", which is impossible fo the *originating* ASN).

Fixes #225.
@danderson

This comment has been minimized.

Copy link
Member

danderson commented Apr 14, 2018

Oops, merging marked the bug fixed.

If you can, could you test the fix by changing the speaker image to use metallb/speaker:master ? That will give you the latest dev code, which includes the fix for this.

@danderson danderson reopened this Apr 14, 2018

BGP mode automation moved this from Done to In progress Apr 14, 2018

danderson added a commit to danderson/metallb that referenced this issue Apr 14, 2018

danderson added a commit that referenced this issue Apr 14, 2018

@danderson danderson closed this Apr 15, 2018

BGP mode automation moved this from In progress to Done Apr 15, 2018

@mhristache

This comment has been minimized.

Copy link

mhristache commented Apr 16, 2018

I've tested the fix and it works. The router is now accepting the BGP updates from metallb.

Thanks a lot for your help!

//Max

@danderson danderson changed the title MetalLB BGP implementation is always using 4-byte ASNs causing compatibility issues MetalLB incorrectly uses AS_SET to specify the originating ASN in path attributes Apr 18, 2018

danderson added a commit to danderson/metallb that referenced this issue Apr 18, 2018

danderson added a commit that referenced this issue Apr 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment