Repository: metallb/metallb
Pull request #1454 (2022-06-24 14:16:40)

Add validation webhook for L2Advertisement
The validation webook is currently missing for l2 adv
object, this commit ensures such validating webhook
implmented for it.

Signed-off-by: Periyasamy Palanisamy <pepalani@redhat.com>


fedepaol: LGTM

Patch:
diff --git a/api/v1beta1/l2advertisement_webhook.go b/api/v1beta1/l2advertisement_webhook.go
new file mode 100644
index 00000000..29e30296
--- /dev/null
+++ b/api/v1beta1/l2advertisement_webhook.go
@@ -0,0 +1,120 @@
+/*
+
+
+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/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 (l2Adv *L2Advertisement) SetupWebhookWithManager(mgr ctrl.Manager) error {
+	return ctrl.NewWebhookManagedBy(mgr).
+		For(l2Adv).
+		Complete()
+}
+
+//+kubebuilder:webhook:verbs=create;update,path=/validate-metallb-io-v1beta1-l2advertisement,mutating=false,failurePolicy=fail,groups=metallb.io,resources=l2advertisements,versions=v1beta1,name=l2advertisementvalidationwebhook.metallb.io,sideEffects=None,admissionReviewVersions=v1
+
+var _ webhook.Validator = &L2Advertisement{}
+
+// ValidateCreate implements webhook.Validator so a webhook will be registered for L2Advertisement.
+func (l2Adv *L2Advertisement) ValidateCreate() error {
+	level.Debug(Logger).Log("webhook", "l2advertisement", "action", "create", "name", l2Adv.Name, "namespace", l2Adv.Namespace)
+
+	existingL2AdvList, err := getExistingL2Advs()
+	if err != nil {
+		return err
+	}
+
+	addressPools, err := getExistingAddressPools()
+	if err != nil {
+		return err
+	}
+
+	ipAddressPools, err := getExistingIPAddressPools()
+	if err != nil {
+		return err
+	}
+
+	toValidate := l2AdvListWithUpdate(existingL2AdvList, l2Adv)
+	err = Validator.Validate(toValidate, addressPools, ipAddressPools)
+	if err != nil {
+		level.Error(Logger).Log("webhook", "l2advertisement", "action", "create", "name", l2Adv.Name, "namespace", l2Adv.Namespace, "error", err)
+		return err
+	}
+	return nil
+}
+
+// ValidateUpdate implements webhook.Validator so a webhook will be registered for L2Advertisement.
+func (l2Adv *L2Advertisement) ValidateUpdate(old runtime.Object) error {
+	level.Debug(Logger).Log("webhook", "l2advertisement", "action", "update", "name", l2Adv.Name, "namespace", l2Adv.Namespace)
+
+	l2Advs, err := getExistingL2Advs()
+	if err != nil {
+		return err
+	}
+
+	addressPools, err := getExistingAddressPools()
+	if err != nil {
+		return err
+	}
+
+	ipAddressPools, err := getExistingIPAddressPools()
+	if err != nil {
+		return err
+	}
+
+	toValidate := l2AdvListWithUpdate(l2Advs, l2Adv)
+	err = Validator.Validate(toValidate, addressPools, ipAddressPools)
+	if err != nil {
+		level.Error(Logger).Log("webhook", "l2advertisement", "action", "create", "name", l2Adv.Name, "namespace", l2Adv.Namespace, "error", err)
+		return err
+	}
+	return nil
+}
+
+// ValidateDelete implements webhook.Validator so a webhook will be registered for L2Advertisement.
+func (l2Adv *L2Advertisement) ValidateDelete() error {
+	return nil
+}
+
+var getExistingL2Advs = func() (*L2AdvertisementList, error) {
+	existingL2AdvList := &L2AdvertisementList{}
+	err := WebhookClient.List(context.Background(), existingL2AdvList, &client.ListOptions{Namespace: MetalLBNamespace})
+	if err != nil {
+		return nil, errors.Wrapf(err, "Failed to get existing L2Advertisement objects")
+	}
+	return existingL2AdvList, nil
+}
+
+func l2AdvListWithUpdate(existing *L2AdvertisementList, toAdd *L2Advertisement) *L2AdvertisementList {
+	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 49342eda..7918a663 100644
--- a/charts/metallb/templates/webhooks.yaml
+++ b/charts/metallb/templates/webhooks.yaml
@@ -124,6 +124,26 @@ webhooks:
     resources:
     - bfdprofiles
   sideEffects: None
+- admissionReviewVersions:
+  - v1
+  clientConfig:
+    service:
+      name: metallb-webhook-service
+      namespace: {{ .Release.Namespace }}
+      path: /validate-metallb-io-v1beta1-l2advertisement
+  failurePolicy: Fail
+  name: l2advertisementvalidationwebhook.metallb.io
+  rules:
+  - apiGroups:
+    - metallb.io
+    apiVersions:
+    - v1beta1
+    operations:
+    - CREATE
+    - UPDATE
+    resources:
+    - l2advertisements
+  sideEffects: None
 ---
 apiVersion: v1
 kind: Service
diff --git a/config/manifests/metallb-frr.yaml b/config/manifests/metallb-frr.yaml
index 7e43c579..47d55b66 100644
--- a/config/manifests/metallb-frr.yaml
+++ b/config/manifests/metallb-frr.yaml
@@ -2052,3 +2052,23 @@ webhooks:
     resources:
     - ipaddresspools
   sideEffects: None
+- admissionReviewVersions:
+  - v1
+  clientConfig:
+    service:
+      name: webhook-service
+      namespace: metallb-system
+      path: /validate-metallb-io-v1beta1-l2advertisement
+  failurePolicy: Fail
+  name: l2advertisementvalidationwebhook.metallb.io
+  rules:
+  - apiGroups:
+    - metallb.io
+    apiVersions:
+    - v1beta1
+    operations:
+    - CREATE
+    - UPDATE
+    resources:
+    - l2advertisements
+  sideEffects: None
diff --git a/config/manifests/metallb-native.yaml b/config/manifests/metallb-native.yaml
index 7f926ab4..f5e3036f 100644
--- a/config/manifests/metallb-native.yaml
+++ b/config/manifests/metallb-native.yaml
@@ -1842,3 +1842,23 @@ webhooks:
     resources:
     - ipaddresspools
   sideEffects: None
+- admissionReviewVersions:
+  - v1
+  clientConfig:
+    service:
+      name: webhook-service
+      namespace: metallb-system
+      path: /validate-metallb-io-v1beta1-l2advertisement
+  failurePolicy: Fail
+  name: l2advertisementvalidationwebhook.metallb.io
+  rules:
+  - apiGroups:
+    - metallb.io
+    apiVersions:
+    - v1beta1
+    operations:
+    - CREATE
+    - UPDATE
+    resources:
+    - l2advertisements
+  sideEffects: None
diff --git a/config/webhook/manifests.yaml b/config/webhook/manifests.yaml
index 494751ab..301a7d7e 100644
--- a/config/webhook/manifests.yaml
+++ b/config/webhook/manifests.yaml
@@ -125,3 +125,23 @@ webhooks:
     resources:
     - ipaddresspools
   sideEffects: None
+- admissionReviewVersions:
+  - v1
+  clientConfig:
+    service:
+      name: webhook-service
+      namespace: system
+      path: /validate-metallb-io-v1beta1-l2advertisement
+  failurePolicy: Fail
+  name: l2advertisementvalidationwebhook.metallb.io
+  rules:
+  - apiGroups:
+    - metallb.io
+    apiVersions:
+    - v1beta1
+    operations:
+    - CREATE
+    - UPDATE
+    resources:
+    - l2advertisements
+  sideEffects: None
diff --git a/internal/k8s/k8s.go b/internal/k8s/k8s.go
index 760ae38d..339d5e67 100644
--- a/internal/k8s/k8s.go
+++ b/internal/k8s/k8s.go
@@ -365,6 +365,11 @@ func enableWebhook(mgr manager.Manager, validate config.Validate, namespace stri
 		return err
 	}
 
+	if err := (&metallbv1beta1.L2Advertisement{}).SetupWebhookWithManager(mgr); err != nil {
+		level.Error(logger).Log("op", "startup", "error", err, "msg", "unable to create webhook", "webhook", "L2Advertisement")
+		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


Test patch:
diff --git a/api/v1beta1/l2advertisement_webhook_test.go b/api/v1beta1/l2advertisement_webhook_test.go
new file mode 100644
index 00000000..bdb5f1b4
--- /dev/null
+++ b/api/v1beta1/l2advertisement_webhook_test.go
@@ -0,0 +1,139 @@
+// 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 TestValidateL2Advertisement(t *testing.T) {
+	l2Adv := L2Advertisement{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test-l2adv",
+			Namespace: MetalLBTestNameSpace,
+		},
+	}
+
+	Logger = log.NewNopLogger()
+
+	toRestore := getExistingL2Advs
+	getExistingL2Advs = func() (*L2AdvertisementList, error) {
+		return &L2AdvertisementList{
+			Items: []L2Advertisement{
+				l2Adv,
+			},
+		}, nil
+	}
+	toRestoreAddresspools := getExistingAddressPools
+	getExistingAddressPools = func() (*AddressPoolList, error) {
+		return &AddressPoolList{}, nil
+	}
+	toRestoreIPAddressPools := getExistingIPAddressPools
+	getExistingIPAddressPools = func() (*IPAddressPoolList, error) {
+		return &IPAddressPoolList{}, nil
+	}
+
+	defer func() {
+		getExistingL2Advs = toRestore
+		getExistingAddressPools = toRestoreAddresspools
+		getExistingIPAddressPools = toRestoreIPAddressPools
+	}()
+
+	tests := []struct {
+		desc         string
+		l2Adv        *L2Advertisement
+		isNew        bool
+		failValidate bool
+		expected     *L2AdvertisementList
+	}{
+		{
+			desc: "Second Adv",
+			l2Adv: &L2Advertisement{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test",
+					Namespace: MetalLBTestNameSpace,
+				},
+			},
+			isNew: true,
+			expected: &L2AdvertisementList{
+				Items: []L2Advertisement{
+					{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "test-l2adv",
+							Namespace: MetalLBTestNameSpace,
+						},
+					},
+					{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "test",
+							Namespace: MetalLBTestNameSpace,
+						},
+					},
+				},
+			},
+		},
+		{
+			desc: "Same, update",
+			l2Adv: &L2Advertisement{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-l2adv",
+					Namespace: MetalLBTestNameSpace,
+				},
+			},
+			isNew: false,
+			expected: &L2AdvertisementList{
+				Items: []L2Advertisement{
+					{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "test-l2adv",
+							Namespace: MetalLBTestNameSpace,
+						},
+					},
+				},
+			},
+		},
+		{
+			desc: "Same, new",
+			l2Adv: &L2Advertisement{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-l2adv",
+					Namespace: MetalLBTestNameSpace,
+				},
+			},
+			isNew: true,
+			expected: &L2AdvertisementList{
+				Items: []L2Advertisement{
+					{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "test-l2adv",
+							Namespace: MetalLBTestNameSpace,
+						},
+					},
+				},
+			},
+			failValidate: true,
+		},
+	}
+	for _, test := range tests {
+		var err error
+		mock := &mockValidator{}
+		Validator = mock
+		mock.forceError = test.failValidate
+
+		if test.isNew {
+			err = test.l2Adv.ValidateCreate()
+		} else {
+			err = test.l2Adv.ValidateUpdate(nil)
+		}
+		if test.failValidate && err == nil {
+			t.Fatalf("test %s failed, expecting error", test.desc)
+		}
+		if !cmp.Equal(test.expected, mock.l2Advs) {
+			t.Fatalf("test %s failed, %s", test.desc, cmp.Diff(test.expected, mock.l2Advs))
+		}
+	}
+}
diff --git a/api/v1beta1/mock_validator_test.go b/api/v1beta1/mock_validator_test.go
index 5d173efe..0be0ecc9 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
+	l2Advs         *L2AdvertisementList
 	communities    *CommunityList
 	forceError     bool
 }
@@ -22,6 +23,8 @@ func (m *mockValidator) Validate(objects ...client.ObjectList) error {
 			m.pools = list
 		case *BGPAdvertisementList:
 			m.bgpAdvs = list
+		case *L2AdvertisementList:
+			m.l2Advs = list
 		case *IPAddressPoolList:
 			m.ipAddressPools = list
 		case *CommunityList: