Update frr to 8.4.2 #1829

fedepaol · 2023-02-13T09:44:29Z

Bumping the frr version to 8.4.2
Also, changing the image revealed a misconfiguration we had in propagating the fixed ips to frr, so with this change we get rid of those fixed ips.

Fixes #1753

oribon · 2023-02-22T09:33:29Z

charts/metallb/values.yaml

@@ -318,7 +318,7 @@ speaker:
    enabled: false
    image:
      repository: quay.io/frrouting/frr
-      tag: 7.5.1


I see 7.5.1 is still the default under frr.conf (in frr-cm.yaml, metallb-frr.yaml for example)
mind changing it there also? I assume it doesn't change anything but wouldn't hurt

this is required because we want to keep the 7.5 format of the configuration.
The frr versoin statement declares the version of the configuration we want to apply

oribon · 2023-02-22T09:40:25Z

internal/bgp/frr/frr.go

@@ -286,8 +286,10 @@ func (sm *sessionManager) createConfig() (*frrConfig, error) {
 			switch family {
 			case ipfamily.IPv4:
 				rout.ipV4Prefixes[prefix] = prefix
+				neighbor.HasV4Advertisements = true


I might be misreading, but shouldn't we first set it to false somewhere before this? asking because if for example a neighbor is already cached with trues but in a later iteration doesn't have a v4 adv the cached true will not be flipped to false (or I'm describing a scenario that can't happen)?

It's the same as appending to advertisements. This flag is a shortcut to avoid looping over advertisements in order to understand if we need to add the deny any section or not, but it is not adding any information per se.
The map is being built every time we call this function, and what we care about is whether the peer has at least one v4 (and v6) advertisement in order to do the proper rendering

oribon · 2023-02-22T10:36:04Z

internal/config/validation.go

@@ -96,6 +97,53 @@ func DiscardNativeOnly(c ClusterResources) error {
 	return nil
 }

+// validateConfig is meant to validate all the inter-dependencies of a parsed configuration.
+// In this case, we ensure that bfd echo is not enabled on a pool.


nit: on a v6 pool?
also, this being called "validateConfig" but being dedicated to a specific dependency feels a bit odd, wdyt about adding another function specifically for the bfd echo and calling it from validateConfig?
it would make validateConfig seem silly (just calling another function) but for long term when we find more inter-dependencies it makes sense to keep this function clean

it's dedicated to a single dependency because so far that's the only one we have. That doesn't change the function semantic. Also, we might find that we can perform more checks in the same loop (or not, we don't know yet), but so far this function is doing what it's stating. I'd split into subfunctions whenever we will find ourselves in the need of it.

oribon · 2023-02-22T10:39:58Z

internal/config/validation.go

+				containsV6 = true
+				break
+			}
+			if !containsV6 { // we only care about v6 advertisements


not sure I understand, if the first cidr is ipv4 and the next is ipv6 we continue to the next pool?
I thought any pool that has an ipv6 cidr should be processed - meaning removing the outer loop, having this if outside of the cidrs loop? can you leave a comment there what exactly is a config we're trying to avoid?

no this is wrong, let me fix it

oribon · 2023-02-22T10:46:07Z

internal/config/validation.go

+			}
+		}
+		for _, a := range p.BGPAdvertisements {
+			if len(a.Peers) == 0 {


can you add a comment here please (why do we care about an advertisement that will not be advertised)?

it will be advertised. len(peers) == 0 means all peers. Commenting

oribon · 2023-02-22T10:47:29Z

internal/config/validation.go

+	if profile.EchoMode {
+		return true
+	}
+	return false


super nit: return profile.EchoMode instead?

not sure I like it more, but I can change it

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

Passing the ips from outside never worked, because the ips were passed after the image's name and so were injected as parameters of the entry point. Moreover, even if that was fixed, that won't work with some docker version resulting in a "user specified IP address is supported only when connecting to networks with user configured subnets" error. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

The frr doc says "If no ip prefix-list is specified, it acts as permit. If ip prefix-list is defined, and no match is found, default deny is applied" Here we add a deny any only if there are no advertisements, to handle special scenarios where we'd have a prefix but not advertised to a specific peer because of the peer selector. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

With the new frr version we move from echo interval to tx and rx interval, and so changes the json. Here we adjust the validation part to the change. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

This to make the test backward compatible. We assert that either echo interval or echo receive interval are rendered in the configuration, to make the test compatible with frr 7.5 Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

The events record the sum of the session oscillation, so if there is a session bounce then the value is not 0. Here we check for the metric to be present 0 or above. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

Giving it a proper name that reflects its behaviour. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

Because of the json change, the test will fail. Instead, we run a bfd test to ensure bfd is still working. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

This was needed to make bfd work across vrfs, but due to FRRouting/frr@edc3f63 this is not required anymore (and will prevent bfd to work with vrfs on ipv6). Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

BFD echo mode works only with another instance of frr (ref https://github.com/FRRouting/frr/blob/6664d745052afcb0f03d6e9ac01bbda44ad1810f/bfdd/bfdd_cli.c#L432) so we are going to disable it. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

The combination is now being rejected due to https://github.com/FRRouting/frr/blob/6664d745052afcb0f03d6e9ac01bbda44ad1810f/bfdd/bfdd_cli.c#L432 so we stop testing it. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

oribon

lgtm

fedepaol requested review from rata, russellb, gclawes and oribon as code owners February 13, 2023 09:44

fedepaol force-pushed the bumpfrr branch 7 times, most recently from d4ea1bb to 52f8b70 Compare February 15, 2023 16:17

fedepaol force-pushed the bumpfrr branch 4 times, most recently from 64e033b to 2f6217b Compare February 21, 2023 09:35

oribon reviewed Feb 22, 2023

View reviewed changes

fedepaol added 12 commits February 27, 2023 22:10

Bump the frr image to 8.4.2

a7eb0f4

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

Charts readme: update the frr version to 8.4.2

66f940b

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

Use remote-echo-receive-interval when validating bfd

2260098

With the new frr version we move from echo interval to tx and rx interval, and so changes the json. Here we adjust the validation part to the change. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

Change metrics.GreaterThan to metrics.GreaterOrEqualThan

1d0c70a

Giving it a proper name that reflects its behaviour. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

Backward compat CI: don't use the validate FRR config but run a bfd test

e9d25fe

Because of the json change, the test will fail. Instead, we run a bfd test to ensure bfd is still working. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

CI: Do not set udp_l3mdev_accept=1 anymore

6595bdc

This was needed to make bfd work across vrfs, but due to FRRouting/frr@edc3f63 this is not required anymore (and will prevent bfd to work with vrfs on ipv6). Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

Refuse BFD + ipv6 + echo mode

74a13d3

BFD echo mode works only with another instance of frr (ref https://github.com/FRRouting/frr/blob/6664d745052afcb0f03d6e9ac01bbda44ad1810f/bfdd/bfdd_cli.c#L432) so we are going to disable it. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

Do not test BFD + Echo + IPv6

598bd00

The combination is now being rejected due to https://github.com/FRRouting/frr/blob/6664d745052afcb0f03d6e9ac01bbda44ad1810f/bfdd/bfdd_cli.c#L432 so we stop testing it. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

fedepaol force-pushed the bumpfrr branch from fead1ca to 598bd00 Compare February 27, 2023 21:10

oribon approved these changes Mar 5, 2023

View reviewed changes

fedepaol merged commit 6a5ad67 into metallb:main Mar 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update frr to 8.4.2 #1829

Update frr to 8.4.2 #1829

fedepaol commented Feb 13, 2023 •

edited

oribon Feb 22, 2023

fedepaol Feb 27, 2023

oribon Feb 22, 2023

fedepaol Feb 27, 2023

oribon Feb 22, 2023

fedepaol Feb 27, 2023

oribon Feb 22, 2023

fedepaol Feb 27, 2023

oribon Feb 22, 2023

fedepaol Feb 27, 2023

oribon Feb 22, 2023

fedepaol Feb 27, 2023

oribon left a comment

Update frr to 8.4.2 #1829

Update frr to 8.4.2 #1829

Conversation

fedepaol commented Feb 13, 2023 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oribon left a comment

Choose a reason for hiding this comment

fedepaol commented Feb 13, 2023 •

edited