Repository: metallb/metallb
Pull request #2202 (2023-12-06 13:01:09)

Webhook does not process transient errors properly
Fixed  #2173 

There are TransientErrors that can occasionally result in overlooking genuine issues. The solution is to reset the fields that might be causing a TransientError.


liornoy: Sorry for not saying this before, but now that I think of this, we also don't need to export the other 2 functions - `RemoveElement` and `ExistCommunity`. 
And can you please run "go fmt" on it? this has some formatting issues.
shimritproj: > Sorry for not saying this before, but now that I think of this, we also don't need to export the other 2 functions - `RemoveElement` and `ExistCommunity`. And can you please run "go fmt" on it? this has some formatting issues.

Yes, sure! you can see now that the file is updated.
fedepaol: Do we need transient error after these changes?
fedepaol: Also, please change the commit message explaining the why we are doing this change.
shimritproj: > Do we need transient error after these changes?
> 
> And, can you add unit tests?

On the Webhooks side, resetting the fields that cause Transienterrors, and there will be no Transienterrors after the changes. Transienterrors prevent us from seeing real errors. However, on the Speaker side, the Transienterrors will remain as usual. This is because, in the Speaker, the Transienterrors help us document the code. 
As for unit tests - yes, I will do them.
@fedepaol 
fedepaol: > > Do we need transient error after these changes?
> > And, can you add unit tests?
> 
> On the Webhooks side, resetting the fields that cause Transienterrors, and there will be no Transienterrors after the changes. Transienterrors prevent us from seeing real errors. However, on the Speaker side, the Transienterrors will remain as usual. This is because, in the Speaker, the Transienterrors help us document the code.> Do we need transient error after these changes?
> And, can you add unit tests?

> However, on the Speaker side, the Transienterrors will remain as usual. This is because, in the Speaker, the 
> Transienterrors help us document the code. As for unit tests - yes, I will do them. @fedepaol

Not sure I understood this. Can you expand a bit?

> As for unit tests - yes, I will do them. @fedepaol


liornoy: > > > Do we need transient error after these changes?
> > > And, can you add unit tests?
> > 
> > 
> > On the Webhooks side, resetting the fields that cause Transienterrors, and there will be no Transienterrors after the changes. Transienterrors prevent us from seeing real errors. However, on the Speaker side, the Transienterrors will remain as usual. This is because, in the Speaker, the Transienterrors help us document the code.> Do we need transient error after these changes?
> > And, can you add unit tests?
> 
> > However, on the Speaker side, the Transienterrors will remain as usual. This is because, in the Speaker, the
> > Transienterrors help us document the code. As for unit tests - yes, I will do them. @fedepaol
> 
> Not sure I understood this. Can you expand a bit?
> 
> > As for unit tests - yes, I will do them. @fedepaol

Regarding transient errors, it might make sense to maintain the transient error as a custom error type. We can then return this error type in certain functions in config.go. This approach will make it clearer why we override some fields in the webhook.
liornoy: Can you please place "resetTransientErrorsFields" below "NewValidator" ?
it's a common practice to place the exported functions first and the private one after.
shimritproj: > > > > Do we need transient error after these changes?
> > > > And, can you add unit tests?
> > > 
> > > 
> > > On the Webhooks side, resetting the fields that cause Transienterrors, and there will be no Transienterrors after the changes. Transienterrors prevent us from seeing real errors. However, on the Speaker side, the Transienterrors will remain as usual. This is because, in the Speaker, the Transienterrors help us document the code.> Do we need transient error after these changes?
> > > And, can you add unit tests?
> > 
> > 
> > > However, on the Speaker side, the Transienterrors will remain as usual. This is because, in the Speaker, the
> > > Transienterrors help us document the code. As for unit tests - yes, I will do them. @fedepaol
> > 
> > 
> > Not sure I understood this. Can you expand a bit?
> > > As for unit tests - yes, I will do them. @fedepaol
> 
> Regarding transient errors, it might make sense to maintain the transient error as a custom error type. We can then return this error type in certain functions in config.go. This approach will make it clearer why we override some fields in the webhook.

A transient error was preventing me from identifying real errors, so we decided to remove them. If it is crucial for us to retain the information contained in errors, we can proceed with Lior's suggestion. 

@liornoy  @fedepaol 

Patch:
diff --git a/internal/config/validator.go b/internal/config/validator.go
index 2a215e67..a2895495 100644
--- a/internal/config/validator.go
+++ b/internal/config/validator.go
@@ -3,7 +3,7 @@
 package config
 
 import (
-	"errors"
+	"strings"
 
 	metallbv1beta1 "go.universe.tf/metallb/api/v1beta1"
 	metallbv1beta2 "go.universe.tf/metallb/api/v1beta2"
@@ -58,13 +58,43 @@ func (v *validator) Validate(resources ...client.ObjectList) error {
 			clusterResources.Nodes = append(clusterResources.Nodes, list.Items...)
 		}
 	}
+	clusterResources = resetTransientErrorsFields(clusterResources)
 	_, err := For(clusterResources, v.validate)
-	if errors.As(err, &TransientError{}) { // we do not want to make assumption on ordering in webhooks.
-		return nil
-	}
 	return err
 }
 
 func NewValidator(validate Validate) apivalidate.ClusterObjects {
 	return &validator{validate: validate}
 }
+
+// Returns the given ClusterResources without the fields that can cause a TransientError.
+// We use this so the webhooks do not make assumptions based on the ordering of objects.
+func resetTransientErrorsFields(clusterResources ClusterResources) ClusterResources {
+	for i := range clusterResources.Peers {
+		clusterResources.Peers[i].Spec.BFDProfile = ""
+		clusterResources.Peers[i].Spec.PasswordSecret = v1.SecretReference{}
+	}
+	for i, bgpAdv := range clusterResources.BGPAdvs {
+		var communities []string
+		for _, community := range bgpAdv.Spec.Communities {
+			// We want to pass only communities that are potentially explicit values.
+			if strings.Contains(community, ":") {
+				communities = append(communities, community)
+			}
+		}
+		clusterResources.BGPAdvs[i].Spec.Communities = communities
+	}
+	for i, legacyAddrPool := range clusterResources.LegacyAddressPools {
+		for j, bgpAdv := range legacyAddrPool.Spec.BGPAdvertisements {
+			var communities []string
+			for k := range bgpAdv.Communities {
+				// We want to pass only communities that are potentially explicit values.
+				if strings.Contains(bgpAdv.Communities[k], ":") {
+					communities = append(communities, bgpAdv.Communities[k])
+				}
+			}
+			clusterResources.LegacyAddressPools[i].Spec.BGPAdvertisements[j].Communities = communities
+		}
+	}
+	return clusterResources
+}
diff --git a/website/content/release-notes/_index.md b/website/content/release-notes/_index.md
index ead88371..cba55bbd 100644
--- a/website/content/release-notes/_index.md
+++ b/website/content/release-notes/_index.md
@@ -17,6 +17,7 @@ Chores:
 - Images updated to Go 1.20.12 ([PR 2213](https://github.com/metallb/metallb/pull/2213))
 - CI/E2E: Relabel the frr metrics from frr-k8s to show as MetalLB's ([PR 2210](https://github.com/metallb/metallb/pull/2210))
 - Dev-env: change the default BGP mode to FRR ([PR 2196](https://github.com/metallb/metallb/pull/2196))
+- Webhooks: avoid transient errors ([PR 2202](https://github.com/metallb/metallb/pull/2202)), [ISSUE 2173](https://github.com/metallb/metallb/issues/2173))
 
 ## Version 0.13.12
 


Test patch:
diff --git a/internal/config/validator_test.go b/internal/config/validator_test.go
index 4b618714..50e142c0 100644
--- a/internal/config/validator_test.go
+++ b/internal/config/validator_test.go
@@ -5,16 +5,19 @@ package config
 import (
 	"testing"
 
-	"go.universe.tf/metallb/api/v1beta2"
+	"github.com/google/go-cmp/cmp"
+	metallbv1beta1 "go.universe.tf/metallb/api/v1beta1"
+	metallbv1beta2 "go.universe.tf/metallb/api/v1beta2"
+	v1 "k8s.io/api/core/v1"
 )
 
 func TestValidator(t *testing.T) {
 	v := validator{DontValidate}
 
-	bgpPeerList := v1beta2.BGPPeerList{
-		Items: []v1beta2.BGPPeer{
+	bgpPeerList := metallbv1beta2.BGPPeerList{
+		Items: []metallbv1beta2.BGPPeer{
 			{
-				Spec: v1beta2.BGPPeerSpec{
+				Spec: metallbv1beta2.BGPPeerSpec{
 					MyASN:      42,
 					ASN:        42,
 					Address:    "1.2.3.4",
@@ -28,3 +31,131 @@ func TestValidator(t *testing.T) {
 		t.Error("The validator should not fail for non existing bfd profile")
 	}
 }
+
+func TestResetTransientErrorsFields(t *testing.T) {
+	tests := []struct {
+		desc             string
+		clusterResources ClusterResources
+		expected         ClusterResources
+	}{
+		{
+			desc:             "empty resource",
+			clusterResources: ClusterResources{},
+			expected:         ClusterResources{},
+		},
+		{
+			desc: "BFDProfiles, PasswordSecrets and Community references are reset",
+			clusterResources: ClusterResources{
+				Peers: []metallbv1beta2.BGPPeer{
+					{
+						Spec: metallbv1beta2.BGPPeerSpec{
+							BFDProfile: "myBFDProfile",
+							MyASN:      64512,
+							ASN:        64512,
+							Address:    "172.30.0.3",
+							PasswordSecret: v1.SecretReference{
+								Name:      "myName",
+								Namespace: "myNamespace",
+							},
+						},
+					},
+				},
+				Communities: []metallbv1beta1.Community{
+					{
+						Spec: metallbv1beta1.CommunitySpec{
+							Communities: []metallbv1beta1.CommunityAlias{
+								{
+									Name:  "myCommunityAlias",
+									Value: "65000:100",
+								},
+							},
+						},
+					},
+				},
+				BGPAdvs: []metallbv1beta1.BGPAdvertisement{
+					{
+						Spec: metallbv1beta1.BGPAdvertisementSpec{
+							Communities: []string{
+								"11111:11aaaa",
+								"larg:12345:12345:12345",
+								"alias",
+								"myCommunityAlias",
+							},
+						},
+					},
+				},
+				LegacyAddressPools: []metallbv1beta1.AddressPool{
+					{
+						Spec: metallbv1beta1.AddressPoolSpec{
+							BGPAdvertisements: []metallbv1beta1.LegacyBgpAdvertisement{
+								{
+									Communities: []string{
+										"11111:11aaaa",
+										"larg:12345:12345:12345",
+										"alias",
+										"myCommunityAlias",
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+			expected: ClusterResources{
+				Peers: []metallbv1beta2.BGPPeer{
+					{
+						Spec: metallbv1beta2.BGPPeerSpec{
+							BFDProfile:     "",
+							MyASN:          64512,
+							ASN:            64512,
+							Address:        "172.30.0.3",
+							PasswordSecret: v1.SecretReference{},
+						},
+					},
+				},
+				Communities: []metallbv1beta1.Community{
+					{
+						Spec: metallbv1beta1.CommunitySpec{
+							Communities: []metallbv1beta1.CommunityAlias{
+								{
+									Name:  "myCommunityAlias",
+									Value: "65000:100",
+								},
+							},
+						},
+					},
+				},
+				BGPAdvs: []metallbv1beta1.BGPAdvertisement{
+					{
+						Spec: metallbv1beta1.BGPAdvertisementSpec{
+							Communities: []string{
+								"11111:11aaaa",
+								"larg:12345:12345:12345",
+							},
+						},
+					},
+				},
+				LegacyAddressPools: []metallbv1beta1.AddressPool{
+					{
+						Spec: metallbv1beta1.AddressPoolSpec{
+							BGPAdvertisements: []metallbv1beta1.LegacyBgpAdvertisement{
+								{
+									Communities: []string{
+										"11111:11aaaa",
+										"larg:12345:12345:12345",
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+		},
+	}
+	for _, test := range tests {
+		result := resetTransientErrorsFields(test.clusterResources)
+		if !cmp.Equal(result, test.expected) {
+			t.Fatalf("test %s failed, unexpected clusterResources (-want +got)\n%s", test.desc, cmp.Diff(result, test.expected))
+		}
+	}
+}