Tie up the API with the FRR configuration mechanism #4

fedepaol · 2023-06-15T14:08:37Z

Here we recycle a good amount of what was done in MetalLB, wiring up the frr-k8s configuration with the intermediate one required to render the configuration.
We also add a basic command to spin up an external FRR and configure it.

oribon · 2023-06-19T10:18:34Z

internal/metrics/metrics.go

+}
+
+var (
+	Namespace = "metallb"


nit: frr-k8s?

oribon · 2023-06-19T10:23:39Z

internal/metrics/metrics.go

+	SessionUp = metric{
+		Name: "session_up",
+		Help: "BGP session state (1 is up, 0 is down)",
+	}
+
+	UpdatesSent = metric{
+		Name: "updates_total",
+		Help: "Number of BGP UPDATE messages sent",
+	}
+
+	Prefixes = metric{
+		Name: "announced_prefixes_total",
+		Help: "Number of prefixes currently being advertised on the BGP session",
+	}


should this be closer to the exporter or we're treating this package as the one containing all of the different metrics? if it's an option I think it makes more sense to separate the metrics the exporter is in charge of vs. the ones that actually relate to the "frr-k8s" controller (which we don't have any at this point?).

makes sense, I copied from metallb but here we don't have the legacy bgp, so these can be moved

internal/frr/api_to_config.go

internal/frr/config.go

cmd/main.go

Makefile

liornoy · 2023-06-21T09:13:44Z

internal/ipfamily/ipfamily.go

+		if ip.To4() != nil {
+			return IPv4, nil
+		}
+		return IPv6, nil


we can call ForAddress here.

liornoy · 2023-06-21T09:15:17Z

internal/ipfamily/ipfamily.go

+type Family string
+
+func (f Family) String() string {
+	return string(f)
+}


I think we can use type alias here instead (type Family = string)
and remove the String() func.

liornoy · 2023-06-21T09:52:20Z

frr-tools/metrics/collector/bgp.go

+
+var (
+	sessionUpDesc = prometheus.NewDesc(
+		prometheus.BuildFQName(Namespace, Subsystem, SessionUp.Name),


are we missing here the Namespace const definition and SessiopUp?

yeah I moved the file but forgot to commit it. Let me fix

liornoy · 2023-06-21T09:53:59Z

internal/metrics/metrics.go

-var (
-	Namespace = "frr-k8s"
-	Subsystem = "bgp"
-
-	SessionUp = metric{
-		Name: "session_up",
-		Help: "BGP session state (1 is up, 0 is down)",
-	}
-
-	UpdatesSent = metric{
-		Name: "updates_total",
-		Help: "Number of BGP UPDATE messages sent",
-	}
-
-	Prefixes = metric{
-		Name: "announced_prefixes_total",
-		Help: "Number of prefixes currently being advertised on the BGP session",
-	}
-)


we still use those variables

liornoy

found some nits in function comments, error logs, etc.
all the rest looks good

liornoy · 2023-06-27T07:36:29Z

frr-tools/metrics/liveness/liveness.go

+			delete(expected, d)
+		}
+		if len(expected) > 0 {
+			logger.Log("daemons not running. got: ", res, "missing: ", res)


in this log, we want to have the expected map's keys after the "missing"

liornoy · 2023-06-27T07:44:43Z

internal/controller/frrconfiguration_controller.go

+		empty := frrk8sv1beta1.FRRConfiguration{}
+		config, err := apiToFRR(empty)
+		if err != nil {
+			level.Error(r.Logger).Log("controller", "FRRConfigurationReconciler", "failed to translate the empty config", req.NamespacedName.String())


this log and the ones below are not printing the error

liornoy · 2023-06-27T07:46:22Z

internal/frr/frr.go

+
+	lastReloadStatus := strings.Fields(string(bytes))
+	if len(lastReloadStatus) != 2 {
+		level.Error(l).Log("op", "reload-validate", "error", err, "cause", "Fields", "bytes", string(bytes))


we need to re-write this one. err is nil here. it's more of an invalid format error.

internal/frr/parse.go

liornoy · 2023-06-27T07:49:25Z

internal/frr/parse.go

+// parseRoute takes the result of a show bgp neighbor
+// and parses the informations related to all the neighbours.


This is a copy-paste from the function above

liornoy · 2023-06-27T07:51:43Z

internal/ipfamily/ipfamily.go

+	return ForAddresses(ipsStrings...)
+}
+
+// ForCIDR returns the address family from a given CIDR.


Suggested change

// ForCIDR returns the address family from a given CIDR.

// ForCIDRString returns the address family from a given CIDR string.

liornoy · 2023-06-27T07:52:05Z

internal/ipfamily/ipfamily_test.go

+			family: IPv6,
+		},
+		{
+			desc:   "ipv4 and ipv6 addresse",


Suggested change

desc: "ipv4 and ipv6 addresse",

desc: "ipv4 and ipv6 addresses",

liornoy · 2023-06-27T07:52:35Z

internal/ipfamily/ipfamily_test.go

+			family: IPv6,
+		},
+		{
+			desc:   "ipv4 and ipv6 addresse",


Suggested change

desc: "ipv4 and ipv6 addresse",

desc: "ipv4 and ipv6 addresses",

liornoy · 2023-06-27T07:53:30Z

internal/controller/controller_test.go

+			))
+
+		})
+


extra newline

liornoy · 2023-06-27T07:53:36Z

internal/controller/controller_test.go

+					Routers: []*frr.RouterConfig{},
+				},
+			))
+


extra newline

Adding various utility packages stolen form metallb Signed-off-by: Federico Paolinelli <[email protected]>

The FRR module is implemented the same way (copied) as MetalLB. The only difference is the starting point, which is the frr-k8s api. We have a translation layer that takes the FRR-K8s config, then translates it, and then generate the FRR configuration and calls the reloader script. Signed-off-by: Federico Paolinelli <[email protected]>

Signed-off-by: Federico Paolinelli <[email protected]>

Here we add the same containers we had in MetalLB and we change the dockerfile accordingly. Signed-off-by: Federico Paolinelli <[email protected]>

Add a dev-env like script that configures both the external container and the frr-k8s configuration. Signed-off-by: Federico Paolinelli <[email protected]>

Signed-off-by: Federico Paolinelli <[email protected]>

This will make easier to test the controller by injecting a mock frr handler. Signed-off-by: Federico Paolinelli <[email protected]>

This is meant to test only the controller behaviour, to see if it reacts correctly to the events. Testing of the conversion of the api should not depend on the api server. Signed-off-by: Federico Paolinelli <[email protected]>

Here we unit test the conversion function. Signed-off-by: Federico Paolinelli <[email protected]>

The flag was not implemented, here we cover with a test and support it. Signed-off-by: Federico Paolinelli <[email protected]>

fedepaol · 2023-06-27T16:49:42Z

@liornoy I think I fixed all the suggestions

liornoy · 2023-06-28T08:21:05Z

/lgtm

oribon reviewed Jun 19, 2023

View reviewed changes

fedepaol force-pushed the withfrr branch from 784dd1a to 77e6079 Compare June 19, 2023 16:56

liornoy reviewed Jun 21, 2023

View reviewed changes

fedepaol force-pushed the withfrr branch 9 times, most recently from 6007f37 to 5f0394c Compare June 26, 2023 14:27

liornoy reviewed Jun 27, 2023

View reviewed changes

fedepaol added 4 commits June 27, 2023 18:10

Add utility packages

fa00d59

Adding various utility packages stolen form metallb Signed-off-by: Federico Paolinelli <[email protected]>

Wire up the FRR package and the controller

956ead3

Signed-off-by: Federico Paolinelli <[email protected]>

Makefile: restart the ds when running make deploy

35c082e

Signed-off-by: Federico Paolinelli <[email protected]>

fedepaol force-pushed the withfrr branch from 5f0394c to 4c5421b Compare June 27, 2023 16:11

fedepaol added 7 commits June 27, 2023 18:41

Add the extra frr containers and wire up the config

08ef923

Here we add the same containers we had in MetalLB and we change the dockerfile accordingly. Signed-off-by: Federico Paolinelli <[email protected]>

Add a script to configure the DS and create an external frr container

24a6962

Add a dev-env like script that configures both the external container and the frr-k8s configuration. Signed-off-by: Federico Paolinelli <[email protected]>

Run go mod tidy

474b031

Signed-off-by: Federico Paolinelli <[email protected]>

Move the frr interaction behind an interface

601fbba

This will make easier to test the controller by injecting a mock frr handler. Signed-off-by: Federico Paolinelli <[email protected]>

Add a controller based test suite

d553cee

This is meant to test only the controller behaviour, to see if it reacts correctly to the events. Testing of the conversion of the api should not depend on the api server. Signed-off-by: Federico Paolinelli <[email protected]>

Add unit tests for api to config

2415e57

Here we unit test the conversion function. Signed-off-by: Federico Paolinelli <[email protected]>

Support the "allowAll" flag

f7ba67a

The flag was not implemented, here we cover with a test and support it. Signed-off-by: Federico Paolinelli <[email protected]>

fedepaol force-pushed the withfrr branch from 4c5421b to f7ba67a Compare June 27, 2023 16:42

fedepaol merged commit 874ea25 into metallb:main Jun 28, 2023
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tie up the API with the FRR configuration mechanism #4

Tie up the API with the FRR configuration mechanism #4

fedepaol commented Jun 15, 2023

oribon Jun 19, 2023

fedepaol Jun 19, 2023

oribon Jun 19, 2023

fedepaol Jun 19, 2023

fedepaol Jun 19, 2023

liornoy Jun 21, 2023

liornoy Jun 21, 2023

liornoy Jun 21, 2023

fedepaol Jun 22, 2023

liornoy Jun 21, 2023

liornoy left a comment

liornoy Jun 27, 2023

liornoy Jun 27, 2023

liornoy Jun 27, 2023

liornoy Jun 27, 2023

liornoy Jun 27, 2023

liornoy Jun 27, 2023

liornoy Jun 27, 2023

liornoy Jun 27, 2023

liornoy Jun 27, 2023

fedepaol commented Jun 27, 2023

liornoy commented Jun 28, 2023

		// parseRoute takes the result of a show bgp neighbor
		// and parses the informations related to all the neighbours.

	// ForCIDR returns the address family from a given CIDR.
	// ForCIDRString returns the address family from a given CIDR string.

	desc: "ipv4 and ipv6 addresse",
	desc: "ipv4 and ipv6 addresses",

Tie up the API with the FRR configuration mechanism #4

Tie up the API with the FRR configuration mechanism #4

Conversation

fedepaol commented Jun 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liornoy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fedepaol commented Jun 27, 2023

liornoy commented Jun 28, 2023