BGP Control Plane #18860

ldelossa · 2022-02-18T22:50:32Z

This PR implements a new BGP control plane.

The PR has self-contained documentation here located at /pkg/bgpv1/README.md and is worth reading through before an initial review.

The initial implementation moves over the existing PodCIDR advertisements feature with the feature-adds of dual stack and IPv6 single stack support.

This support encompasses both IPv6 peering and IPv6 MP BGP advertisements.

This PR also implements a new event handling pattern outlined here by this CFP

The large file count is due to vendoring new depdencies. The vendoring changes are isolated to their own commits and can be jumped over while reviewing.

Implementation of a GoBGP backed BGP control plane.

maintainer-s-little-helper · 2022-02-18T22:50:33Z

Commit bebe911c7693ad6d39f302c3f6d6eb4e8a3b7d74 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

ldelossa · 2022-04-06T17:35:55Z

@kaworu thanks - fixed and rebased.

ldelossa · 2022-04-06T17:58:29Z

/test

michi-covalent · 2022-04-06T21:02:48Z

@ldelossa travis ci failure => https://app.travis-ci.com/github/cilium/cilium/jobs/566155290#L9792

HINT: to fix this, run 'make -C Documentation update-helm-values'

ldelossa · 2022-04-06T21:50:18Z

Thanks! Do you run anything locally to check that this is required?

this commit wires up the bgp control plane by adding the "--enable-bgp-control-plane" flag and adds it to helm. this commit also adds a package level README which outlines details of the BGP control plane implementation. Signed-off-by: Louis DeLosSantos <louis.delos@isovalent.com>

michi-covalent

🚢

michi-covalent · 2022-04-07T00:34:56Z

/test

ldelossa · 2022-04-07T01:09:16Z

/test

michi-covalent · 2022-04-07T02:21:19Z

ci-l4lb => https://github.com/cilium/cilium/actions/runs/2106008869 looks like a cluster creation failure. re-triggering.

michi-covalent · 2022-04-07T02:21:28Z

/ci-l4lb

ldelossa · 2022-04-07T03:20:16Z

/ci-nat46x64

michi-covalent · 2022-04-07T03:43:47Z

https://jenkins.cilium.io/job/Cilium-PR-K8s-1.23-kernel-net-next/957/ failed to provision VMs. restarting...

michi-covalent · 2022-04-07T03:44:01Z

/test-1.23-net-next

michi-covalent · 2022-04-07T04:55:18Z

/test-1.23-net-next

ldelossa · 2022-04-07T11:34:57Z

@michi-covalent i think this is safe to mark as ready to merge. The final failing test seems to be unrelated to this change.

arianvp · 2022-04-11T10:44:37Z

Documentation/gettingstarted/bgp-control-plane.rst

+   ``routerID`` field in each defined ``CiliumBGPVirtualRouter`` object
+   within a ``CiliumBGPPeeringPolicy``
+
+   -  Cilium running on a IPv6 single stack cluster cannot reliably


Calico uses the hash of the ipv6 address to derive a router-id if it's not provided. Defining a router-id for each server is rather annoying especially in presence of auto-scaling and ephemeral nodes. Would it be possible to have some form of "IPAM" for router-IDs? Having to manually set them does not scale well :(

This is a good point.

We have discussed a bit about IPAM for router-IDs, and it definitely wasn't a no-go. More, a time constraint desicion not to add it for this initial merge.

The hashing of the IPv6 address is an interesting one for an "auto generated" way of going about it.

I think we had discussed with our teams how network admins may want to hard code router ids, as they pen-and-paper design their networks. Generation can causes surprises in some cases.

I'd be happy to cater to both use cases, ones where the net admins want to be completely explicit, and ones where generation is OK and helps lessen the administrative burden.

arianvp · 2022-04-11T10:47:58Z

Documentation/gettingstarted/bgp-control-plane.rst

+------------
+
+The ``BGP Control Plane`` is split into a ``Agent-Side Control Plane``
+and a ``Operator-Side`` control plane (not yet implemented).


What is the point of the Operator-Side ? not clear from me from the documentation but might be nice to know

Right now this is a bit in flux.

When the PR started we were looking to fork lift MetalLB code into our new native solution.
MetalLB couples the "LoadBalancer IP" advertisements with its internal "Bare Metal LoadBalancer" feature. They are one thing, and they act at the "cluster-wide" operator level. So when the initial CFP for this feature took place, we were planning on emulating that.

After a big of further discussion, I think the consensus is, a "Bare Metal LoadBalancer" feature is orthoganol in implementation to a "LoadBalancer IP" advertisement feature. One can exist without the other, but the latter can integrate with the former.

Moving forward, I think (don't quote me on this yet, we are still planning) we will create a "bare metal load balancer" feature on its own and potentially drop this "Operator-Side" architecture, or change its usages to suppor something like "router-id IPAM" as you mentioned earlier.

For now, we can probably omit it in the docs, but it slid in with the large change.

joestringer

I noticed a couple of anomalies in this PR while reading through this code, PTAL.

pkg/k8s/synced/crd.go

joestringer · 2022-05-18T17:54:38Z

pkg/k8s/watchers/watcher.go

+		synced.CRDResourceName(v2alpha1.BGPPName):    "SKIP", // Handled in BGP control plane
+		synced.CRDResourceName(v2alpha1.BGPPoolName): "SKIP", // Handled in BGP control plane


Does this mean that the k8s watcher initialization is done separately from this code? I would much prefer if we can avoid skipping here if possible, since this is the mechanism that we use to properly disable k8s resource sync unconditionally across the codebase, so any exceptions can end up having repercussions on builds where we explicitly intend to disable certain functionality (for instance, in EKSA we use this mechanism to remove features that downstream do not wish to support, which includes this BGP support for now).

Feel free to hit me up for more context on this, maybe that'll help to figure out a good way to make this more maintainable.

Might be worth cross-linking the remarks to #19376

(Adding this comment so that at least it will show up in the Github UI for that ticket :) )

@joestringer we skip the watchers registration here due to the BGP control plane managing its own informers and listers:

cilium/pkg/bgpv1/agent/controller.go

Lines 150 to 172 in 636edd0

clientset := k8s.CiliumClient()

factory := externalversions.NewSharedInformerFactory(clientset, 0)

selfTweakList := informers.WithTweakListOptions(func(lo *metav1.ListOptions) {

lo.FieldSelector = "metadata.name=" + nodetypes.GetName()

})

k8sfactory := informers.NewSharedInformerFactoryWithOptions(k8s.Client(), 0, selfTweakList)

nodeLister := k8sfactory.Core().V1().Nodes().Lister()

nodeInformer := k8sfactory.Core().V1().Nodes().Informer()

nodeInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{

AddFunc: sig.Event,

UpdateFunc: func(_ interface{}, _ interface{}) { sig.Event(struct{}{}) },

DeleteFunc: sig.Event,

})

policyLister := factory.Cilium().V2alpha1().CiliumBGPPeeringPolicies().Lister()

policyInformer := factory.Cilium().V2alpha1().CiliumBGPPeeringPolicies().Informer()

policyInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{

AddFunc: sig.Event,

UpdateFunc: func(_ interface{}, _ interface{}) { sig.Event(struct{}{}) },

DeleteFunc: sig.Event,

})

This was an initial attempt to avoid head of line blocking in the watcher code, and an implementation of: https://docs.google.com/document/d/1gybVpFOrnjJFLZEMwp_VQJZGQSZ30_pVo2VA2fylBFM/edit#

👍 okay cool, maybe I'll just hit you up then to see if there's a way to control the k8s registration all from one place then with this model then, because otherwise it'll be a pain to continue to support things like EKS-A going forwards if we have to track each feature individually registering CRD resources and also k8s watchers all over the codebase. Background is this PR: #17677

^ We discussed out of band, it's on me to come up with a concrete proposal of how we might improve this.

maintainer-s-little-helper bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Feb 18, 2022

ldelossa changed the title ~~wip - building and registering CRDs, generated, agent and op building~~ [WIP] BGP Controle Plane Feb 18, 2022

ldelossa changed the title ~~[WIP] BGP Controle Plane~~ [WIP] BGP Control Plane Feb 18, 2022

ldelossa force-pushed the ldelossa/feat/bgp-control-plane branch from bebe911 to c246ead Compare February 18, 2022 23:32

maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Feb 18, 2022

ldelossa force-pushed the ldelossa/feat/bgp-control-plane branch 2 times, most recently from 8ee4bec to ddba5a0 Compare February 21, 2022 00:10

ldelossa force-pushed the ldelossa/feat/bgp-control-plane branch 4 times, most recently from 5283cfb to 56122e9 Compare March 8, 2022 14:50

ldelossa force-pushed the ldelossa/feat/bgp-control-plane branch 2 times, most recently from de7c296 to 4b2154b Compare March 15, 2022 18:53

ldelossa marked this pull request as ready for review March 15, 2022 19:00

ldelossa requested a review from a team as a code owner March 15, 2022 19:00

ldelossa requested a review from a team March 15, 2022 19:00

ldelossa requested review from a team as code owners March 15, 2022 19:00

ldelossa requested a review from a team March 15, 2022 19:00

ldelossa requested review from a team as code owners March 15, 2022 19:00

ldelossa requested review from rolinh, nathanjsweet, kaworu and qmonnet March 15, 2022 19:00

ldelossa added the release-note/major This PR introduces major new functionality to Cilium. label Mar 15, 2022

kaworu removed the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Apr 6, 2022

ldelossa force-pushed the ldelossa/feat/bgp-control-plane branch from eb962a3 to 3419144 Compare April 6, 2022 21:51

ldelossa force-pushed the ldelossa/feat/bgp-control-plane branch from 3419144 to e49763e Compare April 6, 2022 22:40

michi-covalent reviewed Apr 6, 2022

View reviewed changes

michi-covalent approved these changes Apr 6, 2022

View reviewed changes

ldelossa added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 7, 2022

kaworu approved these changes Apr 7, 2022

View reviewed changes

borkmann merged commit 353b7a8 into cilium:master Apr 7, 2022

borkmann mentioned this pull request Apr 7, 2022

Advertise PodCIDR doesn't work on Ipv6 #18989

Closed

2 tasks

ldelossa deleted the ldelossa/feat/bgp-control-plane branch April 7, 2022 16:27

ldelossa mentioned this pull request Apr 8, 2022

BGP Control Plane Follow Ups #19376

Open

10 tasks

arianvp reviewed Apr 11, 2022

View reviewed changes

ldelossa restored the ldelossa/feat/bgp-control-plane branch May 14, 2022 14:50

joestringer reviewed May 18, 2022

View reviewed changes

joestringer mentioned this pull request Nov 17, 2022

Deprecate MetalLB support #22246

Closed

		synced.CRDResourceName(v2alpha1.BGPPName): "SKIP", // Handled in BGP control plane
		synced.CRDResourceName(v2alpha1.BGPPoolName): "SKIP", // Handled in BGP control plane

	clientset := k8s.CiliumClient()
	factory := externalversions.NewSharedInformerFactory(clientset, 0)

	selfTweakList := informers.WithTweakListOptions(func(lo *metav1.ListOptions) {
	lo.FieldSelector = "metadata.name=" + nodetypes.GetName()
	})
	k8sfactory := informers.NewSharedInformerFactoryWithOptions(k8s.Client(), 0, selfTweakList)

	nodeLister := k8sfactory.Core().V1().Nodes().Lister()
	nodeInformer := k8sfactory.Core().V1().Nodes().Informer()
	nodeInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{
	AddFunc: sig.Event,
	UpdateFunc: func(_ interface{}, _ interface{}) { sig.Event(struct{}{}) },
	DeleteFunc: sig.Event,
	})

	policyLister := factory.Cilium().V2alpha1().CiliumBGPPeeringPolicies().Lister()
	policyInformer := factory.Cilium().V2alpha1().CiliumBGPPeeringPolicies().Informer()
	policyInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{
	AddFunc: sig.Event,
	UpdateFunc: func(_ interface{}, _ interface{}) { sig.Event(struct{}{}) },
	DeleteFunc: sig.Event,
	})

BGP Control Plane #18860

BGP Control Plane #18860

Uh oh!

Conversation

ldelossa commented Feb 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maintainer-s-little-helper bot commented Feb 18, 2022

Uh oh!

ldelossa commented Apr 6, 2022

Uh oh!

ldelossa commented Apr 6, 2022

Uh oh!

michi-covalent commented Apr 6, 2022

Uh oh!

ldelossa commented Apr 6, 2022

Uh oh!

michi-covalent left a comment

Choose a reason for hiding this comment

Uh oh!

michi-covalent commented Apr 7, 2022

Uh oh!

ldelossa commented Apr 7, 2022

Uh oh!

michi-covalent commented Apr 7, 2022

Uh oh!

michi-covalent commented Apr 7, 2022

Uh oh!

ldelossa commented Apr 7, 2022

Uh oh!

michi-covalent commented Apr 7, 2022

Uh oh!

michi-covalent commented Apr 7, 2022

Uh oh!

michi-covalent commented Apr 7, 2022

Uh oh!

ldelossa commented Apr 7, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joestringer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joestringer May 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ldelossa commented Feb 18, 2022 •

edited

Loading

joestringer May 19, 2022 •

edited

Loading