Enable a frr-k8s based bgp backend #2162

fedepaol · 2023-11-13T17:33:29Z

Following the frr splitting design proposal, here we add a new BGP implementation, whcih is based on FRR-K8s. What this implemetation does is to generate the corresponding frr-k8s configuration so that the FRR-k8s daemon can configure FRR.

This will allow external actors to add additional FRR knobs that are not related to MetalLB.

Those might be required also by the frrk8s implementation. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

oribon · 2023-11-16T08:08:05Z

internal/bgp/frrk8s/frrk8s.go

+	metallbNamespace string
+	currentNode      string


nit: these could be just namespace, node

I'll change them to what they mean: targetNamespace and nodeToConfigure or something along that line
wdyt?

oribon · 2023-11-16T08:23:51Z

internal/bgp/frrk8s/frrk8s.go

+		toAdd := frrv1beta1.BFDProfile{
+			Name: bfd.Name,
+		}
+		if bfd.ReceiveInterval != nil {


do the ifs here do something?

they prevent the speaker from panicking if the value is nil

maybe I'm missing something but how when both are pointers (toAdd.x and bfd.x)?

ah dang you are right!

oribon · 2023-11-16T08:24:55Z

internal/bgp/frrk8s/frrk8s.go

+		newConfig.Spec.BGP.Routers = append(newConfig.Spec.BGP.Routers, toAdd)
+	}
+
+	for _, bfd := range sm.bfdProfiles {


nit: can you leave a short comment that sm.BfdProfiles is already sorted?

I'll move the sorting here, I think it makes more sense

oribon · 2023-11-16T08:30:00Z

internal/bgp/frrk8s/frrk8s.go

+		}
+		neighbor.ToAdvertise.PrefixesWithCommunity = toAdvertiseWithCommunity(prefixesForCommunity)
+		neighbor.ToAdvertise.PrefixesWithLocalPref = toAdvertiseWithLocalPref(prefixesForLocalPref)
+		sort.Slice(neighbor.ToAdvertise.Allowed.Prefixes, func(i, j int) bool {


nit: use sort.Strings

oribon · 2023-11-16T08:32:58Z

internal/bgp/frrk8s/frrk8s.go

+		res = append(res, frrv1beta1.CommunityPrefixes{Community: c, Prefixes: prefixes})
+	}
+	sort.Slice(res, func(i, j int) bool {
+		return res[i].Community > res[j].Community


nit: i < j for increasing order

oribon · 2023-11-16T10:23:59Z

internal/k8s/k8s.go

+		}
+		if err := frrk8sController.SetupWithManager(mgr); err != nil {
+			level.Error(c.logger).Log("error", err, "unable to create controller", "node")
+			fmt.Println("FEDE error")


oribon · 2023-11-16T10:24:27Z

internal/k8s/k8s.go

+			Reload:    make(chan event.GenericEvent),
+		}
+		if err := frrk8sController.SetupWithManager(mgr); err != nil {
+			level.Error(c.logger).Log("error", err, "unable to create controller", "node")


nit: "node" -> "frr-k8s"

oribon · 2023-11-16T10:26:43Z

config/frr-k8s/kustomization.yaml

+apiVersion: kustomize.config.k8s.io/v1beta1
+kind: Kustomization
+resources:
+- https://github.com/metallb/frr-k8s/config/default/?timeout=120&ref=main


we should remember setting this to != main before merging this

we'll have to do a tag first

oribon · 2023-11-16T10:27:15Z

config/frr-k8s/controller-patch.yaml

is this necessary?

yes, but adapted the type. We need it because the controller serves as webhook endpoint and it must know what needs to validate.

oribon · 2023-11-16T10:29:51Z

config/frr-k8s/frr-cm.yaml

oribon · 2023-11-19T09:12:30Z

internal/bgp/frrk8s/frrk8s.go

+	configChangedCallback func(interface{})
+}
+
+func (sm *sessionManager) SetEventListener(callback func(interface{})) {


I think you saw in the next commit how the session manager is buried into the speaker abstraction and it's not easy to extract (unless we want to have a singleton variable as part of the module, which I am not sure I like).

yep let's keep it that way

Here we set a listener that listens to event. The name does not seem terrible to me, but I can switch to callback if you want.

I think it makes a bit more sense since we don't listen here, I guess SetEventCallback?

oribon · 2023-11-19T09:36:11Z

internal/bgp/frrk8s/frrk8s.go

+
+func (sm *sessionManager) SyncExtraInfo(extraInfo string) error {
+	if extraInfo != "" {
+		return errors.New("extra info not supported in frr-k8s mode")


I'm fine with that but we should remember to document it (more like "highlighting" since this mode unlocks more frr capabilities) when we get to it

oribon · 2023-11-19T09:40:11Z

internal/bgp/frrk8s/frrk8s.go

+		toAdd := frrv1beta1.BFDProfile{
+			Name: bfd.Name,
+		}
+		if bfd.ReceiveInterval != nil {


maybe I'm missing something but how when both are pointers (toAdd.x and bfd.x)?

oribon · 2023-11-19T09:42:12Z

internal/bgp/frrk8s/frrk8s.go

+	toNotify := sm.desiredConfig.DeepCopy()
+	sm.configChangedCallback(*toNotify)


what I'm more worried about is us creating a ton of events that the controller will process (polluting the logs at a minimum) + it'd be nice to save a few api calls (since every speaker generates them).

oribon · 2023-11-19T09:52:48Z

internal/k8s/controllers/frrk8s_config_controller.go

+		return ctrl.Result{}, nil
+	}
+
+	toApply := &frrv1beta1.FRRConfiguration{ObjectMeta: metav1.ObjectMeta{Name: r.desiredConfiguration.Name, Namespace: r.desiredConfiguration.Namespace}}


nit: can you indent this?

oribon · 2023-11-19T09:59:48Z

internal/k8s/controllers/frrk8s_config_controller_test.go

+	g.Expect(err).ToNot(HaveOccurred())
+	m, err := manager.New(cfg, manager.Options{Metrics: metricsserver.Options{BindAddress: "0"}})
+	g.Expect(err).ToNot(HaveOccurred())
+


can we start the test with a "stale" config and see that the controller first deletes it (since desired == nil)?

oribon · 2023-11-19T10:02:15Z

config/frr-k8s/frr-k8s-image-patch.yaml

The real reason why we need this, is to be able to inject a listener that listens to generated frrk8s changes. It has to be a listener because it has to added AFTER the creation of the manager, that in turn is passed to the various controllers. In order to keep the event generic, we pass an empty interface to the callback, so the consumer can assert the type and handle the event accordingly to its type. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

Here we add another bgp session manager implementation where starting from the exposed interface we generate an FRR-K8s configuration tied to the running node. The consuming side is notified via a callback. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

We expose a method that gets short-circuited with the protocol implementation. This will allow us to set a listener to the frrk8s implementation. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

The controller is driven in two ways: - it listens for changes on the frrconfig related to the metallb namespace and the node the controller is running on - it listen for changes in the UpdateConfig method Given calling UpdateConfig is asynchronous, the passed data is stored and an event is sent to a channel so it can be processed inside the reconciliation loop. The notification callback has empty interface as a field so this callback an be used as a bgp event listener. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

We need to: - pass the implementation to the controller, so services and config can be translated - expose a way for the controller to set an event listener, so the implementation can notify the contorller of a new event to process Given this circular dependency, the controller sets the listener after its creation, to the already existing frrk8s implementation. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

We don't have password support yet, but we want to see a basic smoke test working. Here we run a service connectivity test with ebgp-single-hop which is the container configured without password. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

Adding a new lane that just deploys the machinery and runs the smoke test we added. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

oribon · 2023-11-27T09:06:33Z

lgtm 😄

FRR: export utility functions

da55277

Those might be required also by the frrk8s implementation. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

fedepaol requested review from gclawes, oribon and russellb as code owners November 13, 2023 17:33

fedepaol changed the title ~~Enable a frr-k8s based bgp backend~~ WIP: Enable a frr-k8s based bgp backend Nov 13, 2023

fedepaol force-pushed the withfrrk8s_topush branch 8 times, most recently from d5dc257 to 8b6d342 Compare November 14, 2023 20:08

fedepaol changed the title ~~WIP: Enable a frr-k8s based bgp backend~~ Enable a frr-k8s based bgp backend Nov 15, 2023

oribon reviewed Nov 16, 2023

View reviewed changes

fedepaol force-pushed the withfrrk8s_topush branch 2 times, most recently from 657d8af to 166dd6c Compare November 17, 2023 16:35

oribon reviewed Nov 19, 2023

View reviewed changes

fedepaol added 3 commits November 20, 2023 13:32

Speaker: expose a set event listener method from the protocol

9f98737

We expose a method that gets short-circuited with the protocol implementation. This will allow us to set a listener to the frrk8s implementation. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

fedepaol force-pushed the withfrrk8s_topush branch 3 times, most recently from 61c78f7 to 9b95287 Compare November 22, 2023 11:38

fedepaol added 2 commits November 22, 2023 12:44

fedepaol force-pushed the withfrrk8s_topush branch 2 times, most recently from 3251ca4 to abd1b2b Compare November 22, 2023 14:35

fedepaol added 2 commits November 23, 2023 17:41

Config: add an frr-k8s kustomize overlay

ed02249

Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

CI: add a new frr-k8s lane

f21f089

Adding a new lane that just deploys the machinery and runs the smoke test we added. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

fedepaol force-pushed the withfrrk8s_topush branch from abd1b2b to f21f089 Compare November 23, 2023 16:41

Merge branch 'main' into withfrrk8s_topush

246370b

fedepaol added this pull request to the merge queue Nov 27, 2023

Merged via the queue into metallb:main with commit 91a7d56 Nov 27, 2023

		toNotify := sm.desiredConfig.DeepCopy()
		sm.configChangedCallback(*toNotify)

Enable a frr-k8s based bgp backend #2162

Enable a frr-k8s based bgp backend #2162

Uh oh!

Conversation

fedepaol commented Nov 13, 2023

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!

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!

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!

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!

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!

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!

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!

oribon commented Nov 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants