Unnumbered BGP Peering design document

[karampok]

/kind design

What this PR does / why we need it:

Special notes for your reviewer:

In part of this PR there is

1. A design document about unnumbered BGP peering, discussing implementation and API changes
2. A draft version of how an e2e test can run, more for discussion rather than actually merging that file in that PR.

My plan would be that first we discuss how the raw frr config would look like, then the API in frr-k8s, then in metallb.

Thanks

Release note:

None

[oribon]

thanks! great work, did a first pass

[oribon]

iiuc this will all be covered in the initial implementation?

[karampok]

Can you elaborate what you mean?

[oribon]

asking if these points will be covered or there's still a question about them, while the last point (about primary interfaces) still needs to be thought of

[oribon]

I wonder if there's some value in adding another worker+container that are peered using unnumbered (no p2p, but there will be no conflicts as they're the only routers in the network using it) to get some feedback on peering with the primary node interface? wdyt?

[karampok]

Do you mean to have a single worker as speaker, and have single external container frr connected on the same docker network bridge?

My concern is that locally the linux bridge does not forwarding RA when LLA address is the source (at least in my fedora laptop).

For sure we need to verifiy how that works on primary interface, and especially when the interface has also a non-LLA address configured.

[oribon]

My concern is that locally the linux bridge does not forwarding RA when LLA address is the source (at least in my fedora laptop).

oh, didn't you run into the "random" peering we talked about?

Do you mean to have a single worker as speaker, and have single external container frr connected on the same docker network bridge?

right (adding another worker / designating one), then the "randomness" will be okay - since there are only 2 peers on the same network using unnumbered they'll always peer with each other

[oribon]

not sure we need this :)

[karampok]

imo yes, maybe not our task, but unless I see that real config in real setup, I am not sure our use cases are valid.

[fedepaol]

let's move it to the bottom in an addendum. If we can make this work with frr, it's likely to work with other routers as its part of the spec. Here it's distracting

[karampok]

I am removing it as I was not able to find a real world example

[oribon]

I think this is the most valid one, and one that matches FRR the most in a sense (since it seems there's a distinction between interfaces and addresses) without locking us. Also, the docs don't need to be as complicated (stating all of the cases) as it's simply following the what frr is doing, that is we can just shortly say, if address is not specified the peering happens over the interface (unnumbered), if address is specified it is assumed the it is lla.

[fedepaol]

+1 on this

[karampok]

ACK then, will put that as but we need to decide if to further #2423
and have this

// 1. if address and no interface then as normal
// 2. if non valid address and no interface then fail
// 3. if address is LLA and interface not empty, then LLA support
// == extra FRR directive `neighbor fe80::a876:4dff:fe77:408 interface eth0`
// 4. If address is LLA and empty interface, then fail
// 5. If address is empty and interface has value then the BGP unnumber peering config will take place
// ==  modify FRR directive `neighbor PEER interface remote-as <ASN>`
// && replace any address directive to use interface e.g.  `neighbor net0 timers 180 540`
// && enable RAs on this interface
// 6. If address has value (non-LLA) and interface non empty, then fail
``` 
as the end API

[oribon]

not sure I understand - implementation wise the issue will be solved for free(?) and documentation wise it seems sufficient to document the Interface field something like "if specified without address then peering happens over the interface (unnumbered), if together with address then it is assumed to be LLA"?

[oribon]

the user will need to understand the usecases as it does for frr, is there extra complexity beyond that?

[karampok]

imo yes :)

[oribon]

so I think it should be more detailed, because it is not really clear atm

[oribon]

not sure I understand, you're proposing a separate test for the unnumbered case or expanding the existing containers setup?

[karampok]

I propose a separate isolated test for that use case.
(but no strong argument against expanding container setup).
Do you have a preference here?

[oribon]

I'd say if it's possible to have all the tests ran on unnumbered in a clean way it's preferable

[fedepaol]

Suggested change

	Unnumbered BGP utilizes IPv6 link local address to automatically decide who to
	Unnumbered BGP utilizes IPv6 link local address to automatically decide which peer to

[karampok]

done

[fedepaol]

Suggested change

	connect.
	connect to.

[karampok]

done

[fedepaol]

I'd avoid the last sentence, as we are talking about MetalLB in general and not about frr. We can talk about frr when we go in the implementation.

[karampok]

removed FRR reference.

[fedepaol]

Suggested change

	connected) between the interfaces two BGP daemons are using
	connected) between the interfaces two BGP speakers are using

Note that the other peer is normally a router

[karampok]

the other peer is a ToR which, which acts both as L2 switch, and L3 router. Do you see any confusion though here?

[fedepaol]

Suggested change

	### Testing
	### Test plan

[karampok]

done

[fedepaol]

Suggested change

	with a containers that runs FRR on each other side of the veth, and at the same
	with a container that runs FRR on each other side of the veth, and at the same

[karampok]

Done

[fedepaol]

Suggested change

	we test that scenario manual.
	we will test that scenario manually.

Manually how? What we can do manually that we can't automate?

[karampok]

As discussed is not worth the effort to refactor how we set up the infra.

[fedepaol]

Suggested change

	#### E2E test for [Unnumber on secondary interface](design/unnumbered-bgp.md#story-1-unnumbered-iebgp-on-secondary-node-interface)
	#### E2E test for [Unnumbered on secondary interface](design/unnumbered-bgp.md#story-1-unnumbered-iebgp-on-secondary-node-interface)

[karampok]

done

[fedepaol]

I'd start with this, the majority of the scenarios are functional and should (!!!) be orthogonal to how we establish the session

[karampok]

Sounds good, i keep the text as is, one we have option 1 I would say we can merge, but I am in favor to then try to implement also option

[karampok]

thanks for the feedback

[karampok]

removed FRR reference.

[karampok]

done

[karampok]

done

[karampok]

the other peer is a ToR which, which acts both as L2 switch, and L3 router. Do you see any confusion though here?

[karampok]

ok

[karampok]

Sounds good, i keep the text as is, one we have option 1 I would say we can merge, but I am in favor to then try to implement also option

[karampok]

done

[karampok]

Three options how the API could look like, but that was removed.

[karampok]

I was expected if you could explain me how the RouterID is important or not. Example when we have single IPv6 stack. But I have that for discussion, removed

[karampok]

TODO, explain better

[karampok]

@fedepaol @oribon I have updated the doc and I think I have addressed all your feedback. Please take a look, thanks

[oribon]

nit: remove the "not supported by MetalLB" (it might be in the future, and unrelated to this)

[karampok]

done

[oribon]

I'd also remove this (seems unrelated)

[karampok]

done

[oribon]

as the other comment, I think this could be a lot shorter (no need to specify everything and its consequence)

[karampok]

I agree, but I prefer to remove once @fedepaol take another feedback and he is ok with the consequence or not (we could decide no to support the LLA)

[fedepaol]

Let's focus on unnumbered and maybe implement LLA afterwards if we need / want to.
What's important here is that the API is compatible with that. If we want to mention it, let's add a subsection in the bottom

[karampok]

ok, removed the reference to LLA

[fedepaol]

Let's focus on unnumbered and maybe implement LLA afterwards if we need / want to.
What's important here is that the API is compatible with that. If we want to mention it, let's add a subsection in the bottom

[fedepaol]

Since this is the proposed change, let's reword with A new interface field will be introduced

[karampok]

done

[fedepaol]

This is not an option anymore, but the proposed change

[karampok]

done

[fedepaol]

LGTM, thanks for this!