Repository: metallb/metallb
Pull request #1361 (2022-05-10 08:44:49)

Add BFDProfile validation webhook
This PR is adding a validation webhook to the BFDProfile CRD.
This webhook only validates a delete action, asserting that the deleted
profile is not being used by a BGPPeer. 
For "create" and "update" validation, the CRD handles it itself with limits values.

fedepaol: The failure on the operator lane is expected as the operator doesn't deploy the webhook (yet).
Merging as it's a chicken - egg problem.

Patch:
diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml
index 66a48d36..cdc5b265 100644
--- a/.github/workflows/ci.yaml
+++ b/.github/workflows/ci.yaml
@@ -188,7 +188,12 @@ jobs:
         run: |
           inv build
           inv dev-env -b frr --enable-webhooks
-
+      
+      # Patch the old e2etest to cleanup the resources in the correct order.
+      - name: Apply patch
+        run: |
+          patch metallb-v0.12.1/e2etest/pkg/config/update.go < e2etest/backwardcompatible/patchfile
+          
       - name: E2E
         run: |
           cat <<EOF | kubectl apply -f -
diff --git a/api/v1beta1/bfdprofile_webhook.go b/api/v1beta1/bfdprofile_webhook.go
new file mode 100644
index 00000000..e737a5d7
--- /dev/null
+++ b/api/v1beta1/bfdprofile_webhook.go
@@ -0,0 +1,64 @@
+/*
+
+
+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 (
+	"fmt"
+
+	"github.com/go-kit/log/level"
+	"go.universe.tf/metallb/api/v1beta2"
+	"k8s.io/apimachinery/pkg/runtime"
+	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/webhook"
+)
+
+func (bfdProfile *BFDProfile) SetupWebhookWithManager(mgr ctrl.Manager) error {
+	return ctrl.NewWebhookManagedBy(mgr).
+		For(bfdProfile).
+		Complete()
+}
+
+//+kubebuilder:webhook:verbs=delete,path=/validate-metallb-io-v1beta1-bfdprofile,mutating=false,failurePolicy=fail,groups=metallb.io,resources=bfdprofiles,versions=v1beta1,name=bfdprofilevalidationwebhook.metallb.io,sideEffects=None,admissionReviewVersions=v1
+
+var _ webhook.Validator = &BFDProfile{}
+
+// ValidateCreate implements webhook.Validator so a webhook will be registered for BFDProfile.
+func (bfdProfile *BFDProfile) ValidateCreate() error {
+	return nil
+}
+
+// ValidateUpdate implements webhook.Validator so a webhook will be registered for BFDProfile.
+func (bfdProfile *BFDProfile) ValidateUpdate(old runtime.Object) error {
+	return nil
+}
+
+// ValidateDelete implements webhook.Validator so a webhook will be registered for BFDProfile.
+func (bfdProfile *BFDProfile) ValidateDelete() error {
+	level.Debug(Logger).Log("webhook", "bfdprofile", "action", "delete", "name", bfdProfile.Name, "namespace", bfdProfile.Namespace)
+
+	existingBGPPeers, err := v1beta2.GetExistingBGPPeers()
+	if err != nil {
+		return err
+	}
+
+	for _, peer := range existingBGPPeers.Items {
+		if bfdProfile.Name == peer.Spec.BFDProfile {
+			return fmt.Errorf("Failed to delete BFDProfile %s, used by BGPPeer %s", bfdProfile.Name, peer.Spec.BFDProfile)
+		}
+	}
+	return nil
+}
diff --git a/api/v1beta2/bgppeer_webhook.go b/api/v1beta2/bgppeer_webhook.go
index 6bda6469..a8cbcf3f 100644
--- a/api/v1beta2/bgppeer_webhook.go
+++ b/api/v1beta2/bgppeer_webhook.go
@@ -41,7 +41,7 @@ var _ webhook.Validator = &BGPPeer{}
 func (bgpPeer *BGPPeer) ValidateCreate() error {
 	level.Debug(Logger).Log("webhook", "bgppeer", "action", "create", "name", bgpPeer.Name, "namespace", bgpPeer.Namespace)
 
-	existingBGPPeers, err := getExistingBGPPeers()
+	existingBGPPeers, err := GetExistingBGPPeers()
 	if err != nil {
 		return err
 	}
@@ -59,7 +59,7 @@ func (bgpPeer *BGPPeer) ValidateCreate() error {
 func (bgpPeer *BGPPeer) ValidateUpdate(old runtime.Object) error {
 	level.Debug(Logger).Log("webhook", "bgppeer", "action", "update", "name", bgpPeer.Name, "namespace", bgpPeer.Namespace)
 
-	existingBGPPeers, err := getExistingBGPPeers()
+	existingBGPPeers, err := GetExistingBGPPeers()
 	if err != nil {
 		return err
 	}
@@ -78,7 +78,7 @@ func (bgpPeer *BGPPeer) ValidateDelete() error {
 	return nil
 }
 
-var getExistingBGPPeers = func() (*BGPPeerList, error) {
+var GetExistingBGPPeers = func() (*BGPPeerList, error) {
 	existingBGPPeerslList := &BGPPeerList{}
 	err := WebhookClient.List(context.Background(), existingBGPPeerslList, &client.ListOptions{Namespace: MetalLBNamespace})
 	if err != nil {
diff --git a/charts/metallb/templates/crds.yaml b/charts/metallb/templates/crds.yaml
index 913e2b06..8d58829a 100644
--- a/charts/metallb/templates/crds.yaml
+++ b/charts/metallb/templates/crds.yaml
@@ -224,6 +224,9 @@ kind: CustomResourceDefinition
 metadata:
   annotations:
     controller-gen.kubebuilder.io/version: v0.7.0
+  {{- if .Values.webhooks.enable }}
+    cert-manager.io/inject-ca-from: {{ .Release.Namespace }}/{{ template "metallb.fullname" . }}-serving-cert    
+  {{- end }}
   creationTimestamp: null
   name: bfdprofiles.metallb.io
 spec:
diff --git a/charts/metallb/templates/rbac.yaml b/charts/metallb/templates/rbac.yaml
index b8488ea3..94aca394 100644
--- a/charts/metallb/templates/rbac.yaml
+++ b/charts/metallb/templates/rbac.yaml
@@ -116,6 +116,9 @@ rules:
 - apiGroups: ["metallb.io"]
   resources: ["communities"]
   verbs: ["get", "list"]
+- apiGroups: ["metallb.io"]
+  resources: ["bfdprofiles"]
+  verbs: ["get", "list","watch"]
 ---
 apiVersion: rbac.authorization.k8s.io/v1
 kind: ClusterRoleBinding
diff --git a/charts/metallb/templates/webhooks.yaml b/charts/metallb/templates/webhooks.yaml
index 33ff747e..15f822c7 100644
--- a/charts/metallb/templates/webhooks.yaml
+++ b/charts/metallb/templates/webhooks.yaml
@@ -108,6 +108,25 @@ webhooks:
     resources:
     - communities
   sideEffects: None
+- admissionReviewVersions:
+  - v1
+  clientConfig:
+    service:
+      name: {{ template "metallb.fullname" . }}-webhook-service
+      namespace: {{ .Release.Namespace }}
+      path: /validate-metallb-io-v1beta1-bfdprofile
+  failurePolicy: Fail
+  name: bfdprofileyvalidationwebhook.metallb.io
+  rules:
+  - apiGroups:
+    - metallb.io
+    apiVersions:
+    - v1beta1
+    operations:
+    - DELETE
+    resources:
+    - bfdprofiles
+  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 5783740d..a78b6219 100644
--- a/config/manifests/metallb-frr-with-webhooks.yaml
+++ b/config/manifests/metallb-frr-with-webhooks.yaml
@@ -1236,6 +1236,14 @@ rules:
   - get
   - list
   - watch
+- apiGroups:
+  - metallb.io
+  resources:
+  - bfdprofiles
+  verbs:
+  - get
+  - list
+  - watch
 - apiGroups:
   - metallb.io
   resources:
@@ -1954,6 +1962,25 @@ webhooks:
     resources:
     - addresspools
   sideEffects: None
+- admissionReviewVersions:
+  - v1
+  clientConfig:
+    service:
+      name: webhook-service
+      namespace: metallb-system
+      path: /validate-metallb-io-v1beta1-bfdprofile
+  failurePolicy: Fail
+  name: bfdprofilevalidationwebhook.metallb.io
+  rules:
+  - apiGroups:
+    - metallb.io
+    apiVersions:
+    - v1beta1
+    operations:
+    - DELETE
+    resources:
+    - bfdprofiles
+  sideEffects: None
 - admissionReviewVersions:
   - v1
   clientConfig:
diff --git a/config/manifests/metallb-frr.yaml b/config/manifests/metallb-frr.yaml
index 1868f6b4..b35b2bd3 100644
--- a/config/manifests/metallb-frr.yaml
+++ b/config/manifests/metallb-frr.yaml
@@ -1212,6 +1212,14 @@ rules:
   - get
   - list
   - watch
+- apiGroups:
+  - metallb.io
+  resources:
+  - bfdprofiles
+  verbs:
+  - get
+  - list
+  - watch
 - apiGroups:
   - metallb.io
   resources:
diff --git a/config/manifests/metallb-native-with-webhooks.yaml b/config/manifests/metallb-native-with-webhooks.yaml
index 6108399e..9bb0cf52 100644
--- a/config/manifests/metallb-native-with-webhooks.yaml
+++ b/config/manifests/metallb-native-with-webhooks.yaml
@@ -1236,6 +1236,14 @@ rules:
   - get
   - list
   - watch
+- apiGroups:
+  - metallb.io
+  resources:
+  - bfdprofiles
+  verbs:
+  - get
+  - list
+  - watch
 - apiGroups:
   - metallb.io
   resources:
@@ -1744,6 +1752,25 @@ webhooks:
     resources:
     - addresspools
   sideEffects: None
+- admissionReviewVersions:
+  - v1
+  clientConfig:
+    service:
+      name: webhook-service
+      namespace: metallb-system
+      path: /validate-metallb-io-v1beta1-bfdprofile
+  failurePolicy: Fail
+  name: bfdprofilevalidationwebhook.metallb.io
+  rules:
+  - apiGroups:
+    - metallb.io
+    apiVersions:
+    - v1beta1
+    operations:
+    - DELETE
+    resources:
+    - bfdprofiles
+  sideEffects: None
 - admissionReviewVersions:
   - v1
   clientConfig:
diff --git a/config/manifests/metallb-native.yaml b/config/manifests/metallb-native.yaml
index 0a15a3ae..a09035d5 100644
--- a/config/manifests/metallb-native.yaml
+++ b/config/manifests/metallb-native.yaml
@@ -1212,6 +1212,14 @@ rules:
   - get
   - list
   - watch
+- apiGroups:
+  - metallb.io
+  resources:
+  - bfdprofiles
+  verbs:
+  - get
+  - list
+  - watch
 - apiGroups:
   - metallb.io
   resources:
diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml
index 057e6e5d..6d02a482 100644
--- a/config/rbac/role.yaml
+++ b/config/rbac/role.yaml
@@ -200,6 +200,14 @@ rules:
       - get
       - list
       - watch
+  - apiGroups:
+      - metallb.io
+    resources:
+      - bfdprofiles
+    verbs:
+      - get
+      - list
+      - watch
   - apiGroups:
       - metallb.io
     resources:
diff --git a/config/webhook/manifests.yaml b/config/webhook/manifests.yaml
index 10d0f0c6..494751ab 100644
--- a/config/webhook/manifests.yaml
+++ b/config/webhook/manifests.yaml
@@ -46,6 +46,25 @@ webhooks:
     resources:
     - addresspools
   sideEffects: None
+- admissionReviewVersions:
+  - v1
+  clientConfig:
+    service:
+      name: webhook-service
+      namespace: system
+      path: /validate-metallb-io-v1beta1-bfdprofile
+  failurePolicy: Fail
+  name: bfdprofilevalidationwebhook.metallb.io
+  rules:
+  - apiGroups:
+    - metallb.io
+    apiVersions:
+    - v1beta1
+    operations:
+    - DELETE
+    resources:
+    - bfdprofiles
+  sideEffects: None
 - admissionReviewVersions:
   - v1
   clientConfig:
diff --git a/internal/k8s/k8s.go b/internal/k8s/k8s.go
index d429a32e..c1d0c2db 100644
--- a/internal/k8s/k8s.go
+++ b/internal/k8s/k8s.go
@@ -310,6 +310,11 @@ func enableWebhook(mgr manager.Manager, validate config.Validate, namespace stri
 		return err
 	}
 
+	if err := (&metallbv1beta1.BFDProfile{}).SetupWebhookWithManager(mgr); err != nil {
+		level.Error(logger).Log("op", "startup", "error", err, "msg", "unable to create webhook", "webhook", "BFDProfile")
+		return err
+	}
+
 	return nil
 }
 


Test patch:
diff --git a/api/v1beta1/bfdprofile_webhook_test.go b/api/v1beta1/bfdprofile_webhook_test.go
new file mode 100644
index 00000000..df7b25e5
--- /dev/null
+++ b/api/v1beta1/bfdprofile_webhook_test.go
@@ -0,0 +1,47 @@
+// SPDX-License-Identifier:Apache-2.0
+
+package v1beta1
+
+import (
+	"testing"
+
+	"go.universe.tf/metallb/api/v1beta2"
+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+func TestValidateBFDProfile(t *testing.T) {
+	toRestoreBGPPeers := v1beta2.GetExistingBGPPeers
+	v1beta2.GetExistingBGPPeers = func() (*v1beta2.BGPPeerList, error) {
+		return &v1beta2.BGPPeerList{
+			Items: []v1beta2.BGPPeer{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "test-bgppeer",
+						Namespace: MetalLBTestNameSpace,
+					},
+					Spec: v1beta2.BGPPeerSpec{
+						BFDProfile: "bfdprofile",
+					},
+				},
+			},
+		}, nil
+	}
+	defer func() {
+		v1beta2.GetExistingBGPPeers = toRestoreBGPPeers
+	}()
+
+	desc := "Delete bfdprofile used by bgppeer"
+	bfdProfile := &BFDProfile{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "bfdprofile",
+			Namespace: MetalLBTestNameSpace,
+		},
+	}
+
+	err := bfdProfile.ValidateDelete()
+
+	if err == nil {
+		t.Fatalf("test %s failed, expecting error", desc)
+	}
+}
diff --git a/api/v1beta1/bgpadvertisement_webhook_test.go b/api/v1beta1/bgpadvertisement_webhook_test.go
index 232545d1..64a9b56b 100644
--- a/api/v1beta1/bgpadvertisement_webhook_test.go
+++ b/api/v1beta1/bgpadvertisement_webhook_test.go
@@ -97,7 +97,7 @@ func TestValidateBGPAdvertisement(t *testing.T) {
 			},
 		},
 		{
-			desc: "Same, update",
+			desc: "Same, new",
 			bgpAdv: &BGPAdvertisement{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:      "test-bgpadv",
diff --git a/api/v1beta2/bgppeer_webhook_test.go b/api/v1beta2/bgppeer_webhook_test.go
index 163454b0..9a6aabc0 100644
--- a/api/v1beta2/bgppeer_webhook_test.go
+++ b/api/v1beta2/bgppeer_webhook_test.go
@@ -22,8 +22,8 @@ func TestValidateBGPPeer(t *testing.T) {
 
 	Logger = log.NewNopLogger()
 
-	toRestore := getExistingBGPPeers
-	getExistingBGPPeers = func() (*BGPPeerList, error) {
+	toRestore := GetExistingBGPPeers
+	GetExistingBGPPeers = func() (*BGPPeerList, error) {
 		return &BGPPeerList{
 			Items: []BGPPeer{
 				bgpPeer,
@@ -32,7 +32,7 @@ func TestValidateBGPPeer(t *testing.T) {
 	}
 
 	defer func() {
-		getExistingBGPPeers = toRestore
+		GetExistingBGPPeers = toRestore
 	}()
 
 	tests := []struct {
diff --git a/e2etest/backwardcompatible/patchfile b/e2etest/backwardcompatible/patchfile
new file mode 100644
index 00000000..e6fb89cd
--- /dev/null
+++ b/e2etest/backwardcompatible/patchfile
@@ -0,0 +1,16 @@
+Switch the order of the resources deletion to avoid BFDProfile webhook error
+@@ -131,12 +131,12 @@
+ 		return err
+ 	}
+ 
+-	err = o.DeleteAllOf(context.Background(), &operatorv1beta1.BFDProfile{}, client.InNamespace(o.namespace))
++	err = o.DeleteAllOf(context.Background(), &operatorv1beta1.BGPPeer{}, client.InNamespace(o.namespace))
+ 	if err != nil {
+ 		return err
+ 	}
+ 
+-	err = o.DeleteAllOf(context.Background(), &operatorv1beta1.BGPPeer{}, client.InNamespace(o.namespace))
++	err = o.DeleteAllOf(context.Background(), &operatorv1beta1.BFDProfile{}, client.InNamespace(o.namespace))
+ 	if err != nil {
+ 		return err
+ 	}
diff --git a/e2etest/webhookstests/webhooks.go b/e2etest/webhookstests/webhooks.go
index 86705589..1767a623 100644
--- a/e2etest/webhookstests/webhooks.go
+++ b/e2etest/webhookstests/webhooks.go
@@ -19,8 +19,11 @@ limitations under the License.
 package webhookstests
 
 import (
+	"context"
+
 	"go.universe.tf/metallb/e2etest/pkg/config"
 	"go.universe.tf/metallb/e2etest/pkg/k8s"
+	"go.universe.tf/metallb/e2etest/pkg/metallb"
 
 	"github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
@@ -32,6 +35,7 @@ import (
 	"go.universe.tf/metallb/internal/pointer"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/kubernetes/test/e2e/framework"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
 var (
@@ -333,4 +337,47 @@ var _ = ginkgo.Describe("Webhooks", func() {
 			Expect(err.Error()).To(ContainSubstring("duplicate definition of community"))
 		})
 	})
+
+	ginkgo.Context("For BFDProfile", func() {
+		testBFDProfile := metallbv1beta1.BFDProfile{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "bfdprofile-webhooks-test",
+				Namespace: metallb.Namespace,
+			},
+		}
+		testPeer := metallbv1beta2.BGPPeer{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "bgppeer-webhooks-test",
+				Namespace: metallb.Namespace,
+			},
+			Spec: metallbv1beta2.BGPPeerSpec{
+				BFDProfile: testBFDProfile.Name,
+				ASN:        1234,
+				MyASN:      1234,
+				Address:    "1.2.3.4",
+			},
+		}
+		ginkgo.It("Should produce an error when deleting a profile used by a BGPPeer", func() {
+			ginkgo.By("Creating BFDProfile and BGPPeer")
+			resources := metallbconfig.ClusterResources{
+				BFDProfiles: []metallbv1beta1.BFDProfile{testBFDProfile},
+				Peers:       []metallbv1beta2.BGPPeer{testPeer},
+			}
+			err := ConfigUpdater.Update(resources)
+			framework.ExpectNoError(err)
+
+			ginkgo.By("Deleting the profile used by BGPPeer")
+			err = ConfigUpdater.Client().Delete(context.TODO(), &testBFDProfile, &client.DeleteOptions{})
+			framework.ExpectError(err)
+			Expect(err.Error()).To(ContainSubstring("Failed to delete BFDProfile"))
+
+			ginkgo.By("Deleting the BGPPeer")
+			err = ConfigUpdater.Client().Delete(context.TODO(), &testPeer, &client.DeleteOptions{})
+			framework.ExpectNoError(err)
+
+			ginkgo.By("Deleting the profile not used by BGPPeer")
+			err = ConfigUpdater.Client().Delete(context.TODO(), &testBFDProfile, &client.DeleteOptions{})
+			framework.ExpectNoError(err)
+		})
+	})
 })