Repository: metallb/metallb
Pull request #1310 (2022-04-07 08:21:43)

Add Community Webhook
This PR adds the webhook for the Community CRD.
Based on the Add Community CRD PR #1253 

fedepaol: LGTM, thanks!

Patch:
diff --git a/api/v1beta1/community_webhook.go b/api/v1beta1/community_webhook.go
new file mode 100644
index 00000000..87dfb53f
--- /dev/null
+++ b/api/v1beta1/community_webhook.go
@@ -0,0 +1,100 @@
+/*
+
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package v1beta1
+
+import (
+	"context"
+
+	"github.com/go-kit/kit/log/level"
+	"github.com/pkg/errors"
+	"k8s.io/apimachinery/pkg/runtime"
+	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/webhook"
+)
+
+func (community *Community) SetupWebhookWithManager(mgr ctrl.Manager) error {
+	return ctrl.NewWebhookManagedBy(mgr).
+		For(community).
+		Complete()
+}
+
+//+kubebuilder:webhook:verbs=create;update,path=/validate-metallb-io-v1beta1-community,mutating=false,failurePolicy=fail,groups=metallb.io,resources=communities,versions=v1beta1,name=communityvalidationwebhook.metallb.io,sideEffects=None,admissionReviewVersions=v1
+
+var _ webhook.Validator = &Community{}
+
+// ValidateCreate implements webhook.Validator so a webhook will be registered for Community.
+func (community *Community) ValidateCreate() error {
+	level.Debug(Logger).Log("webhook", "community", "action", "create", "name", community.Name, "namespace", community.Namespace)
+
+	existingCommunityList, err := getExistingCommunities()
+	if err != nil {
+		return err
+	}
+
+	communityList := communitylistWithUpdate(existingCommunityList, community)
+	err = Validator.Validate(communityList)
+	if err != nil {
+		level.Error(Logger).Log("webhook", "community", "action", "create", "name", community.Name, "namespace", community.Namespace, "error", err)
+		return err
+	}
+	return nil
+}
+
+// ValidateUpdate implements webhook.Validator so a webhook will be registered for Community.
+func (community *Community) ValidateUpdate(old runtime.Object) error {
+	level.Debug(Logger).Log("webhook", "community", "action", "update", "name", community.Name, "namespace", community.Namespace)
+
+	existingCommunityList, err := getExistingCommunities()
+	if err != nil {
+		return err
+	}
+
+	communityList := communitylistWithUpdate(existingCommunityList, community)
+	err = Validator.Validate(communityList)
+	if err != nil {
+		level.Error(Logger).Log("webhook", "community", "action", "update", "name", community.Name, "namespace", community.Namespace, "error", err)
+		return err
+	}
+	return nil
+}
+
+// ValidateDelete implements webhook.Validator so a webhook will be registered for Community.
+func (community *Community) ValidateDelete() error {
+	return nil
+}
+
+var getExistingCommunities = func() (*CommunityList, error) {
+	existingCommunityList := &CommunityList{}
+	err := WebhookClient.List(context.Background(), existingCommunityList, &client.ListOptions{Namespace: MetalLBNamespace})
+	if err != nil {
+		return nil, errors.Wrapf(err, "Failed to get existing Community objects")
+	}
+	return existingCommunityList, nil
+}
+
+func communitylistWithUpdate(existing *CommunityList, toAdd *Community) *CommunityList {
+	res := existing.DeepCopy()
+	for i, item := range res.Items { // We override the element with the fresh copy
+		if item.Name == toAdd.Name {
+			res.Items[i] = *toAdd.DeepCopy()
+			return res
+		}
+	}
+	res.Items = append(res.Items, *toAdd.DeepCopy())
+	return res
+}
diff --git a/charts/metallb/templates/webhooks.yaml b/charts/metallb/templates/webhooks.yaml
index ea5d6907..33ff747e 100644
--- a/charts/metallb/templates/webhooks.yaml
+++ b/charts/metallb/templates/webhooks.yaml
@@ -88,6 +88,26 @@ webhooks:
     resources:
     - bgpadvertisements
   sideEffects: None
+- admissionReviewVersions:
+  - v1
+  clientConfig:
+    service:
+      name: {{ template "metallb.fullname" . }}-webhook-service
+      namespace: {{ .Release.Namespace }}
+      path: /validate-metallb-io-v1beta1-community
+  failurePolicy: Fail
+  name: communityvalidationwebhook.metallb.io
+  rules:
+  - apiGroups:
+    - metallb.io
+    apiVersions:
+    - v1beta1
+    operations:
+    - CREATE
+    - UPDATE
+    resources:
+    - communities
+  sideEffects: None
 ---
 apiVersion: v1
 kind: Service
diff --git a/config/manifests/metallb-frr-with-webhooks.yaml b/config/manifests/metallb-frr-with-webhooks.yaml
index 27012b81..5769d7f7 100644
--- a/config/manifests/metallb-frr-with-webhooks.yaml
+++ b/config/manifests/metallb-frr-with-webhooks.yaml
@@ -1840,6 +1840,26 @@ webhooks:
     resources:
     - bgpadvertisements
   sideEffects: None
+- admissionReviewVersions:
+  - v1
+  clientConfig:
+    service:
+      name: webhook-service
+      namespace: metallb-system
+      path: /validate-metallb-io-v1beta1-community
+  failurePolicy: Fail
+  name: communityvalidationwebhook.metallb.io
+  rules:
+  - apiGroups:
+    - metallb.io
+    apiVersions:
+    - v1beta1
+    operations:
+    - CREATE
+    - UPDATE
+    resources:
+    - communities
+  sideEffects: None
 - admissionReviewVersions:
   - v1
   clientConfig:
diff --git a/config/manifests/metallb-native-with-webhooks.yaml b/config/manifests/metallb-native-with-webhooks.yaml
index 2226f54b..9248b90a 100644
--- a/config/manifests/metallb-native-with-webhooks.yaml
+++ b/config/manifests/metallb-native-with-webhooks.yaml
@@ -1628,6 +1628,26 @@ webhooks:
     resources:
     - bgpadvertisements
   sideEffects: None
+- admissionReviewVersions:
+  - v1
+  clientConfig:
+    service:
+      name: webhook-service
+      namespace: metallb-system
+      path: /validate-metallb-io-v1beta1-community
+  failurePolicy: Fail
+  name: communityvalidationwebhook.metallb.io
+  rules:
+  - apiGroups:
+    - metallb.io
+    apiVersions:
+    - v1beta1
+    operations:
+    - CREATE
+    - UPDATE
+    resources:
+    - communities
+  sideEffects: None
 - admissionReviewVersions:
   - v1
   clientConfig:
diff --git a/config/webhook/manifests.yaml b/config/webhook/manifests.yaml
index 0b20212c..10d0f0c6 100644
--- a/config/webhook/manifests.yaml
+++ b/config/webhook/manifests.yaml
@@ -66,6 +66,26 @@ webhooks:
     resources:
     - bgpadvertisements
   sideEffects: None
+- admissionReviewVersions:
+  - v1
+  clientConfig:
+    service:
+      name: webhook-service
+      namespace: system
+      path: /validate-metallb-io-v1beta1-community
+  failurePolicy: Fail
+  name: communityvalidationwebhook.metallb.io
+  rules:
+  - apiGroups:
+    - metallb.io
+    apiVersions:
+    - v1beta1
+    operations:
+    - CREATE
+    - UPDATE
+    resources:
+    - communities
+  sideEffects: None
 - admissionReviewVersions:
   - v1
   clientConfig:
diff --git a/internal/config/validator.go b/internal/config/validator.go
index 1a1a5ad4..d9bb5e94 100644
--- a/internal/config/validator.go
+++ b/internal/config/validator.go
@@ -35,6 +35,7 @@ func (v *validator) Validate(resources ...client.ObjectList) error {
 		BGPAdvs:            make([]metallbv1beta1.BGPAdvertisement, 0),
 		L2Advs:             make([]metallbv1beta1.L2Advertisement, 0),
 		LegacyAddressPools: make([]metallbv1beta1.AddressPool, 0),
+		Communities:        make([]metallbv1beta1.Community, 0),
 	}
 	for _, list := range resources {
 		switch list := list.(type) {
@@ -50,6 +51,8 @@ func (v *validator) Validate(resources ...client.ObjectList) error {
 			clusterResources.L2Advs = append(clusterResources.L2Advs, list.Items...)
 		case *metallbv1beta1.AddressPoolList:
 			clusterResources.LegacyAddressPools = append(clusterResources.LegacyAddressPools, list.Items...)
+		case *metallbv1beta1.CommunityList:
+			clusterResources.Communities = append(clusterResources.Communities, list.Items...)
 		}
 	}
 	_, err := For(clusterResources, v.validate)
diff --git a/internal/k8s/k8s.go b/internal/k8s/k8s.go
index 27c8cd74..9b5e4af4 100644
--- a/internal/k8s/k8s.go
+++ b/internal/k8s/k8s.go
@@ -315,6 +315,11 @@ func enableWebhook(mgr manager.Manager, validate config.Validate, namespace stri
 		return err
 	}
 
+	if err := (&metallbv1beta1.Community{}).SetupWebhookWithManager(mgr); err != nil {
+		level.Error(logger).Log("op", "startup", "error", err, "msg", "unable to create webhook", "webhook", "Community")
+		return err
+	}
+
 	return nil
 }
 


Test patch:
diff --git a/api/v1beta1/community_webhook_test.go b/api/v1beta1/community_webhook_test.go
new file mode 100644
index 00000000..bcdd9fd1
--- /dev/null
+++ b/api/v1beta1/community_webhook_test.go
@@ -0,0 +1,126 @@
+// SPDX-License-Identifier:Apache-2.0
+
+package v1beta1
+
+import (
+	"testing"
+
+	"github.com/go-kit/log"
+	"github.com/google/go-cmp/cmp"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+func TestValidateCommunity(t *testing.T) {
+	Logger = log.NewNopLogger()
+
+	toRestoreCommunities := getExistingCommunities
+	getExistingCommunities = func() (*CommunityList, error) {
+		return &CommunityList{
+			Items: []Community{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "test-commuinty1",
+						Namespace: MetalLBTestNameSpace,
+					},
+				},
+			},
+		}, nil
+	}
+	defer func() {
+		getExistingCommunities = toRestoreCommunities
+	}()
+
+	tests := []struct {
+		desc           string
+		commuinty      *Community
+		isNewCommunity bool
+		failValidate   bool
+		expected       *CommunityList
+	}{
+		{
+			desc: "Second Community",
+			commuinty: &Community{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-community2",
+					Namespace: MetalLBTestNameSpace,
+				},
+			},
+			isNewCommunity: true,
+			expected: &CommunityList{
+				Items: []Community{
+					{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "test-commuinty1",
+							Namespace: MetalLBTestNameSpace,
+						},
+					},
+					{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "test-community2",
+							Namespace: MetalLBTestNameSpace,
+						},
+					},
+				},
+			},
+		},
+		{
+			desc: "Same Community, update",
+			commuinty: &Community{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-commuinty1",
+					Namespace: MetalLBTestNameSpace,
+				},
+			},
+			isNewCommunity: false,
+			expected: &CommunityList{
+				Items: []Community{
+					{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "test-commuinty1",
+							Namespace: MetalLBTestNameSpace,
+						},
+					},
+				},
+			},
+		},
+		{
+			desc: "Same community, new",
+			commuinty: &Community{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-commuinty1",
+					Namespace: MetalLBTestNameSpace,
+				},
+			},
+			isNewCommunity: true,
+			expected: &CommunityList{
+				Items: []Community{
+					{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "test-commuinty1",
+							Namespace: MetalLBTestNameSpace,
+						},
+					},
+				},
+			},
+			failValidate: true,
+		},
+	}
+	for _, test := range tests {
+		var err error
+		mock := &mockValidator{}
+		Validator = mock
+		mock.forceError = test.failValidate
+
+		if test.isNewCommunity {
+			err = test.commuinty.ValidateCreate()
+		} else {
+			err = test.commuinty.ValidateUpdate(nil)
+		}
+		if test.failValidate && err == nil {
+			t.Fatalf("test %s failed, expecting error", test.desc)
+		}
+		if !cmp.Equal(test.expected, mock.communities) {
+			t.Fatalf("test %s failed, %s", test.desc, cmp.Diff(test.expected, mock.pools))
+		}
+	}
+}
diff --git a/api/v1beta1/mock_validator_test.go b/api/v1beta1/mock_validator_test.go
index c9e7cb77..5d173efe 100644
--- a/api/v1beta1/mock_validator_test.go
+++ b/api/v1beta1/mock_validator_test.go
@@ -11,6 +11,7 @@ type mockValidator struct {
 	pools          *AddressPoolList
 	ipAddressPools *IPAddressPoolList
 	bgpAdvs        *BGPAdvertisementList
+	communities    *CommunityList
 	forceError     bool
 }
 
@@ -23,6 +24,8 @@ func (m *mockValidator) Validate(objects ...client.ObjectList) error {
 			m.bgpAdvs = list
 		case *IPAddressPoolList:
 			m.ipAddressPools = list
+		case *CommunityList:
+			m.communities = list
 		default:
 			panic("unexpected type")
 		}
diff --git a/e2etest/webhookstests/webhooks.go b/e2etest/webhookstests/webhooks.go
index e8b7e70a..07e63f91 100644
--- a/e2etest/webhookstests/webhooks.go
+++ b/e2etest/webhookstests/webhooks.go
@@ -41,13 +41,13 @@ var _ = ginkgo.Describe("Webhooks", func() {
 		framework.ExpectNoError(err)
 	})
 
-	ginkgo.Context("For IPAddressPool", func() {
-		ginkgo.AfterEach(func() {
-			// Clean previous configuration.
-			err := ConfigUpdater.Clean()
-			framework.ExpectNoError(err)
-		})
+	ginkgo.AfterEach(func() {
+		// Clean previous configuration.
+		err := ConfigUpdater.Clean()
+		framework.ExpectNoError(err)
+	})
 
+	ginkgo.Context("For IPAddressPool", func() {
 		ginkgo.It("Should recognize overlapping addresses in two AddressPools", func() {
 			ginkgo.By("Creating first IPAddressPool")
 			resources := metallbconfig.ClusterResources{
@@ -96,12 +96,6 @@ var _ = ginkgo.Describe("Webhooks", func() {
 	})
 
 	ginkgo.Context("for Legacy AddressPool", func() {
-		ginkgo.AfterEach(func() {
-			// Clean previous configuration.
-			err := ConfigUpdater.Clean()
-			framework.ExpectNoError(err)
-		})
-
 		ginkgo.It("Should recognize overlapping addresses in two AddressPools", func() {
 			ginkgo.By("Creating first AddrssPool")
 			resources := metallbconfig.ClusterResources{
@@ -154,13 +148,6 @@ var _ = ginkgo.Describe("Webhooks", func() {
 	})
 
 	ginkgo.Context("for BGPAdvertisement", func() {
-		ginkgo.AfterEach(func() {
-			ginkgo.By("Clearing any previous configuration")
-
-			err := ConfigUpdater.Clean()
-			framework.ExpectNoError(err)
-		})
-
 		ginkgo.It("Should recognize invalid AggregationLength", func() {
 			ginkgo.By("Creating AddressPool")
 			resources := metallbconfig.ClusterResources{
@@ -199,13 +186,6 @@ var _ = ginkgo.Describe("Webhooks", func() {
 	})
 
 	ginkgo.Context("For BGPPeer", func() {
-		ginkgo.AfterEach(func() {
-			ginkgo.By("Clearing any previous configuration")
-
-			err := ConfigUpdater.Clean()
-			framework.ExpectNoError(err)
-		})
-
 		ginkgo.It("Should reject invalid BGPPeer IP address", func() {
 			ginkgo.By("Creating BGPPeer")
 			resources := metallbconfig.ClusterResources{
@@ -227,4 +207,121 @@ var _ = ginkgo.Describe("Webhooks", func() {
 			Expect(err.Error()).To(ContainSubstring("invalid BGPPeer address"))
 		})
 	})
+
+	ginkgo.Context("For Community", func() {
+		ginkgo.It("Should reject a new invalid Community", func() {
+			ginkgo.By("Creating invalid Community")
+			resources := metallbconfig.ClusterResources{
+				Communities: []metallbv1beta1.Community{
+					{
+						ObjectMeta: metav1.ObjectMeta{
+							Name: "community-webhooks-test",
+						},
+						Spec: metallbv1beta1.CommunitySpec{
+							Communities: []metallbv1beta1.CommunityAlias{
+								{
+									Name:  "INVALID_COMMUNITY",
+									Value: "99999999:1",
+								},
+							},
+						},
+					},
+				},
+			}
+			err := ConfigUpdater.Update(resources)
+			framework.ExpectError(err)
+			Expect(err.Error()).To(ContainSubstring("invalid first section of community"))
+		})
+
+		ginkgo.It("Should reject an update to an invalid Community", func() {
+			ginkgo.By("Creating Community")
+			resources := metallbconfig.ClusterResources{
+				Communities: []metallbv1beta1.Community{
+					{
+						ObjectMeta: metav1.ObjectMeta{
+							Name: "community-webhooks-test",
+						},
+					},
+				},
+			}
+			err := ConfigUpdater.Update(resources)
+			framework.ExpectNoError(err)
+
+			ginkgo.By("Updating community")
+			resources.Communities[0].Spec = metallbv1beta1.CommunitySpec{
+				Communities: []metallbv1beta1.CommunityAlias{
+					{
+						Name:  "INVALID_COMMUNITY",
+						Value: "99999999:1",
+					},
+				},
+			}
+			err = ConfigUpdater.Update(resources)
+			framework.ExpectError(err)
+			Expect(err.Error()).To(ContainSubstring("invalid first section of community"))
+		})
+
+		ginkgo.It("Should reject Community duplications", func() {
+			ginkgo.By("Creating duplicates in the same Community")
+			resources := metallbconfig.ClusterResources{
+				Communities: []metallbv1beta1.Community{
+					{
+						ObjectMeta: metav1.ObjectMeta{
+							Name: "community-webhooks-test",
+						},
+						Spec: metallbv1beta1.CommunitySpec{
+							Communities: []metallbv1beta1.CommunityAlias{
+								{
+									Name:  "DUP_COMMUNITY",
+									Value: "1111:2222",
+								},
+								{
+									Name:  "DUP_COMMUNITY",
+									Value: "1111:2222",
+								},
+							},
+						},
+					},
+				},
+			}
+			err := ConfigUpdater.Update(resources)
+			framework.ExpectError(err)
+			Expect(err.Error()).To(ContainSubstring("duplicate definition of community"))
+
+			ginkgo.By("Creating duplicates across two different Communities")
+			resources = metallbconfig.ClusterResources{
+				Communities: []metallbv1beta1.Community{
+					{
+						ObjectMeta: metav1.ObjectMeta{
+							Name: "community-webhooks-test1",
+						},
+						Spec: metallbv1beta1.CommunitySpec{
+							Communities: []metallbv1beta1.CommunityAlias{
+								{
+									Name:  "DUP_COMMUNITY",
+									Value: "1111:2222",
+								},
+							},
+						},
+					},
+					{
+						ObjectMeta: metav1.ObjectMeta{
+							Name: "community-webhooks-test2",
+						},
+						Spec: metallbv1beta1.CommunitySpec{
+							Communities: []metallbv1beta1.CommunityAlias{
+								{
+									Name:  "DUP_COMMUNITY",
+									Value: "1111:2222",
+								},
+							},
+						},
+					},
+				},
+			}
+			err = ConfigUpdater.Update(resources)
+			framework.ExpectError(err)
+			Expect(err.Error()).To(ContainSubstring("duplicate definition of community"))
+		})
+	})
 })