Make localPref uniform among updates #1820

mlguerrero12 · 2023-02-06T07:57:40Z

These changes ensure that the local preference is unique among BGP ADVs with the same type of BGP update. It is still possible to have BGP ADVs with different values of localPref if the set of BGP peers are different. BGP ADVs with common BGP peers must not have common nodes. Checks are performed for BGP ADVs that have the same aggregation length.

Fixed #1739

mlguerrero12 · 2023-02-06T11:48:09Z

/retest

fedepaol · 2023-02-06T14:14:14Z

internal/config/config.go

 	return nil
 }

+func uniqueConfig(newAdv, adv *BGPAdvertisement) bool {


let's call this "advertisementsAreCompatible"

internal/config/config.go

fedepaol · 2023-02-06T14:17:17Z

internal/config/config.go

+	}
+
+	// BGP ADVs with different set of nodes do not collide.
+	for node := range newAdv.Nodes {


let's handle the case when one of the two maps is empty. I don't remember if when we get here, the nodes are already unrolled in the map but I wouldn't rely on that.

This is totally sane and makes sense.
I am a bit afraid of a scenario where a new node is added (or a label is changed to a node), and that change makes the configuration invalid. That won't be caught by the webhook (because we don't have webhooks on nodes and we can't have them), so from a user's point of view the configuration will be invalid without an explicit reason.
I think that the most common use case is to have the localpref be differentiated on peer basis rather than on node basis, so I am tempted to say let's be more restrictive and not allow advertisements on different nodes to have different prefixes.

Thoughts? Also invoking @oribon

Nodes are already unrolled and it already covers the case when one of the maps is empty. It will simply not collide.

I'm with you in your concern about adding a node. We need to come up with a solution here.

Regarding the use of localPref, I think it is all the opposite. The most common use case is with nodes. What's the point of setting the same localPref for all speakers against a specific peer? We could choose any value for localPref since it won't add anything to the updates. It will be useless in my opinion.

The way I've used localPref before was with the aim of giving preference to some updates in iBGP. In this case, users might select a set of nodes with higher preference to receive traffic.

sorry for the late response
I'm a bit concerned about this approach, mainly because a change in the cluster which we can't listen for that suddenly makes the config bad feels wrong to me, especially when we (still) don't have an accessible way to expose if the running configuration is good or not.

there's a thing I'd like to get your opinion that I'm still not sure of - for all of the other fields that overlap between different resources targeting the same service (like communities) we union/add them together and suddenly with localpref we'd take an "intersection" approach, saying that they all have to be the same value for a given advertisement.
is it wrong to treat overlapping localprefs similarly, where "adding" them together means choosing the highest value for the advertisement?

Yes, we shouldn't go this way if we can't expose the state of the running configuration easily.

Indeed, community is another (and I think the only other) attribute that should ideally be unique. I think that our design was simply applying what it was present in the BGP Adv regardless of the attributes' values. It fit perfect for communities since it can only have 2 states (present or not), but not so well for localpref, as you saw it in the issue you reported. We could choose the highest value but I think we will need to implement some additional logic in the speaker (bgp controller). It might get complicated there. See the TODO in updateAds.

Another option is to act as a mutating webhook by simply overriding the value of localpref if there is another one present (or using the one with the highest value). This way we won't need changes in the speaker. We can return a warning saying that the localpref was changed so that users can be aware of it.

What do you think?

@fedepaol, any thoughts on this?

even if we had a mutating webhook, we wouldn't be able to be always on the safe side because we might have concurrent add operations that sneak in together.
We'd have to invalidate the config anyway in this scenario.

I am almost convinced that the "newly added node" invalidating the config is not that terrible.

If the current configuration is valid, it means that the user took care of labelling the nodes in such a way that they are partiotioned wrt localpref (or that they just don't care).
If a new node comes in and invalidates the configuration, it means it wasn't labeled properly,

What is going to happen is:

the existing speakers will keep the old configuration, without the extra node, but they will keep running

the speaker running on the new node won't advertise

also, any new config change won't have any effect.
Having the same node aimed by two localprefs is not right, the current behaviour is not predictable and resorting to priorities will just hide a misconfiguration under the carpet, because what a user really wants is to apply one single localpref from one node. They will find metallb is behaving, it's just that the overridden localpref might be more subtle to find, compared to metallb not working at all on one node (plus we have metrics for stale configs and we will land https://github.com/metallb/metallb/pull/1685/files soon or later)

ok, so we continue with this approach then. I fixed the unit tests.

fedepaol · 2023-02-06T14:18:03Z

internal/config/config.go

+	// Verify that BGP ADVs set a unique local preference value per BGP update.
+	for _, bgpAdv := range pool.BGPAdvertisements {
+		if adv.LocalPref != bgpAdv.LocalPref {
+			if adv.AggregationLength == bgpAdv.AggregationLength || adv.AggregationLengthV6 == bgpAdv.AggregationLengthV6 {


Let's embed this if in the "are compatible" function

fedepaol · 2023-02-06T14:43:48Z

config/rbac/role.yaml

      - get
      - list
      - watch
+  - apiGroups:


In case we decide we want to validate against nodes, we'll have to add this to the charts too.

yes, I saw the tests failing due to this

I will create a PR on the operator for the same

fedepaol · 2023-02-06T16:59:58Z

internal/config/config.go

+
+	// Verify that BGP ADVs set a unique local preference value per BGP update.
+	for _, bgpAdv := range pool.BGPAdvertisements {
+		if adv.LocalPref != bgpAdv.LocalPref {


we can embed this one in the function I guess

I think this is fine. No need to insert the attribute in the function. We might use it in the future for something else.

fedepaol · 2023-02-06T17:00:20Z

internal/config/config.go

 }

+func advertisementsAreCompatible(newAdv, adv *BGPAdvertisement) bool {
+	if adv.AggregationLength == newAdv.AggregationLength || adv.AggregationLengthV6 == newAdv.AggregationLengthV6 {


this can be flipped and return (I hope :D )

mlguerrero12 · 2023-03-15T09:57:43Z

@fedepaol, is there a way to link a pr from the operator to this pr so that the ci doesn't fail?

fedepaol · 2023-03-20T15:58:07Z

lgtm, thanks and sorry again for the delay!

mlguerrero12 · 2023-03-20T18:19:51Z

@fedepaol, e2e-use-operator will always fail

These changes ensure that the local preference is unique among BGP ADVs with the same type of BGP update. It is still possible to have BGP ADVs with different values of localPref if the set of BGP peers are different. BGP ADVs with common BGP peers must not have common nodes. Checks are performed for BGP ADVs that have the same aggregation length. Fixed metallb#1739 Signed-off-by: Marcelo Guerrero Viveros <marguerr@redhat.com>

Bump metallb to the latest version mainly due to changes in the cluster role of the controller. See metallb/metallb#1820. Signed-off-by: Marcelo Guerrero Viveros <marguerr@redhat.com>

mlguerrero12 requested review from fedepaol, gclawes, oribon, rata and russellb as code owners February 6, 2023 07:57

mlguerrero12 force-pushed the localprefwebhook branch from 53796c9 to 7e1aa14 Compare February 6, 2023 08:45

mlguerrero12 force-pushed the localprefwebhook branch 4 times, most recently from 9645e39 to 8459c3b Compare February 6, 2023 14:29

fedepaol reviewed Feb 6, 2023

View reviewed changes

mlguerrero12 force-pushed the localprefwebhook branch from 8459c3b to acc522d Compare February 6, 2023 16:48

fedepaol reviewed Feb 6, 2023

View reviewed changes

mlguerrero12 force-pushed the localprefwebhook branch 2 times, most recently from f6e0b7c to 2cef742 Compare February 7, 2023 07:59

mlguerrero12 force-pushed the localprefwebhook branch from 2cef742 to b0960b5 Compare February 14, 2023 20:38

mlguerrero12 force-pushed the localprefwebhook branch 2 times, most recently from 6d9cee5 to 1d02c50 Compare March 14, 2023 11:19

fedepaol added this pull request to the merge queue Mar 20, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 20, 2023

fedepaol added this pull request to the merge queue Mar 20, 2023

github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Mar 20, 2023

mlguerrero12 force-pushed the localprefwebhook branch from 1d02c50 to 6c464ff Compare March 21, 2023 08:50

Merge branch 'main' into localprefwebhook

2400fed

fedepaol enabled auto-merge March 21, 2023 16:00

fedepaol added this pull request to the merge queue Mar 21, 2023

fedepaol merged commit 7b6d91d into metallb:main Mar 21, 2023

fedepaol mentioned this pull request Mar 22, 2023

Bump the metallb version metallb/metallb-operator#331

Merged

busterswt mentioned this pull request Mar 27, 2024

Fix for #1739 breaks localPref for different prefix lengths #2345

Closed

8 tasks

Make localPref uniform among updates #1820

Make localPref uniform among updates #1820

Uh oh!

Conversation

mlguerrero12 commented Feb 6, 2023

Uh oh!

mlguerrero12 commented Feb 6, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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!

mlguerrero12 commented Mar 15, 2023

Uh oh!

fedepaol commented Mar 20, 2023

Uh oh!

Uh oh!

mlguerrero12 commented Mar 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants