Merge pull request #1017 from zeeke/ds/417/OCPBUGS-41897

OCPBUGS-43108: [release-4.17] Delete webhooks when SriovOperatorConfig is deleted

controllers/sriovoperatorconfig_controller.go

@@ -18,6 +18,7 @@ package controllers
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"os"
 	"sort"
@@ -28,6 +29,7 @@ import (
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	uns "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
 	kscheme "k8s.io/client-go/kubernetes/scheme"
 	ctrl "sigs.k8s.io/controller-runtime"
@@ -81,7 +83,9 @@ func (r *SriovOperatorConfigReconciler) Reconcile(ctx context.Context, req ctrl.
 	if err != nil {
 		if apierrors.IsNotFound(err) {
 			logger.Info("default SriovOperatorConfig object not found. waiting for creation.")
-			return reconcile.Result{}, nil
+
+			err := r.deleteAllWebhooks(ctx)
+			return reconcile.Result{}, err
 		}
 		// Error reading the object - requeue the request.
 		logger.Error(err, "Failed to get default SriovOperatorConfig object")
@@ -456,3 +460,29 @@ func (r SriovOperatorConfigReconciler) setLabelInsideObject(ctx context.Context,
 
 	return nil
 }
+
+func (r SriovOperatorConfigReconciler) deleteAllWebhooks(ctx context.Context) error {
+	var err error
+	obj := &uns.Unstructured{}
+	obj.SetGroupVersionKind(schema.GroupVersionKind{Group: "admissionregistration.k8s.io", Kind: "MutatingWebhookConfiguration", Version: "v1"})
+	obj.SetName(consts.OperatorWebHookName)
+	err = errors.Join(
+		err, r.deleteWebhookObject(ctx, obj),
+	)
+
+	obj = &uns.Unstructured{}
+	obj.SetGroupVersionKind(schema.GroupVersionKind{Group: "admissionregistration.k8s.io", Kind: "ValidatingWebhookConfiguration", Version: "v1"})
+	obj.SetName(consts.OperatorWebHookName)
+	err = errors.Join(
+		err, r.deleteWebhookObject(ctx, obj),
+	)
+
+	obj = &uns.Unstructured{}
+	obj.SetGroupVersionKind(schema.GroupVersionKind{Group: "admissionregistration.k8s.io", Kind: "MutatingWebhookConfiguration", Version: "v1"})
+	obj.SetName(consts.InjectorWebHookName)
+	err = errors.Join(
+		err, r.deleteWebhookObject(ctx, obj),
+	)
+
+	return err
+}

controllers/sriovoperatorconfig_controller_test.go

@@ -6,6 +6,7 @@ import (
 	"os"
 	"strings"
 	"sync"
+	"time"
 
 	admv1 "k8s.io/api/admissionregistration/v1"
 	appsv1 "k8s.io/api/apps/v1"
@@ -38,15 +39,7 @@ var _ = Describe("SriovOperatorConfig controller", Ordered, func() {
 
 	BeforeAll(func() {
 		By("Create SriovOperatorConfig controller k8s objs")
-		config := &sriovnetworkv1.SriovOperatorConfig{}
-		config.SetNamespace(testNamespace)
-		config.SetName(consts.DefaultConfigName)
-		config.Spec = sriovnetworkv1.SriovOperatorConfigSpec{
-			EnableInjector:           true,
-			EnableOperatorWebhook:    true,
-			ConfigDaemonNodeSelector: map[string]string{},
-			LogLevel:                 2,
-		}
+		config := makeDefaultSriovOpConfig()
 		Expect(k8sClient.Create(context.Background(), config)).Should(Succeed())
 		DeferCleanup(func() {
 			err := k8sClient.Delete(context.Background(), config)
@@ -224,6 +217,29 @@ var _ = Describe("SriovOperatorConfig controller", Ordered, func() {
 			Expect(err).NotTo(HaveOccurred())
 		})
 
+		// Namespaced resources are deleted via the `.ObjectMeta.OwnerReference` field. That logic can't be tested here because testenv doesn't have built-in controllers
+		// (See https://book.kubebuilder.io/reference/envtest#testing-considerations). Since Service and DaemonSet are deleted when default/SriovOperatorConfig is no longer
+		// present, it's important that webhook configurations are deleted as well.
+		It("should delete the webhooks when SriovOperatorConfig/default is deleted", func() {
+			DeferCleanup(k8sClient.Create, context.Background(), makeDefaultSriovOpConfig())
+
+			err := k8sClient.Delete(context.Background(), &sriovnetworkv1.SriovOperatorConfig{
+				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: "default"},
+			})
+			Expect(err).NotTo(HaveOccurred())
+
+			assertResourceDoesNotExist(
+				schema.GroupVersionKind{Group: "admissionregistration.k8s.io", Kind: "MutatingWebhookConfiguration", Version: "v1"},
+				client.ObjectKey{Name: "sriov-operator-webhook-config"})
+			assertResourceDoesNotExist(
+				schema.GroupVersionKind{Group: "admissionregistration.k8s.io", Kind: "ValidatingWebhookConfiguration", Version: "v1"},
+				client.ObjectKey{Name: "sriov-operator-webhook-config"})
+
+			assertResourceDoesNotExist(
+				schema.GroupVersionKind{Group: "admissionregistration.k8s.io", Kind: "MutatingWebhookConfiguration", Version: "v1"},
+				client.ObjectKey{Name: "network-resources-injector-config"})
+		})
+
 		It("should be able to update the node selector of sriov-network-config-daemon", func() {
 			By("specify the configDaemonNodeSelector")
 			config := &sriovnetworkv1.SriovOperatorConfig{}
@@ -495,9 +511,36 @@ var _ = Describe("SriovOperatorConfig controller", Ordered, func() {
 	})
 })
 
+func makeDefaultSriovOpConfig() *sriovnetworkv1.SriovOperatorConfig {
+	config := &sriovnetworkv1.SriovOperatorConfig{}
+	config.SetNamespace(testNamespace)
+	config.SetName(consts.DefaultConfigName)
+	config.Spec = sriovnetworkv1.SriovOperatorConfigSpec{
+		EnableInjector:           true,
+		EnableOperatorWebhook:    true,
+		ConfigDaemonNodeSelector: map[string]string{},
+		LogLevel:                 2,
+	}
+	return config
+}
+
 func assertResourceExists(gvk schema.GroupVersionKind, key client.ObjectKey) {
 	u := &unstructured.Unstructured{}
 	u.SetGroupVersionKind(gvk)
 	err := k8sClient.Get(context.Background(), key, u)
 	Expect(err).NotTo(HaveOccurred())
 }
+
+func assertResourceDoesNotExist(gvk schema.GroupVersionKind, key client.ObjectKey) {
+	Eventually(func(g Gomega) {
+		u := &unstructured.Unstructured{}
+		u.SetGroupVersionKind(gvk)
+		err := k8sClient.Get(context.Background(), key, u)
+		g.Expect(err).To(HaveOccurred())
+		g.Expect(errors.IsNotFound(err)).To(BeTrue())
+	}).
+		WithOffset(1).
+		WithPolling(100*time.Millisecond).
+		WithTimeout(2*time.Second).
+		Should(Succeed(), "Resource type[%s] name[%s] still present in the cluster", gvk.String(), key.String())
+}

pkg/host/internal/network/network.go

@@ -75,7 +75,7 @@ func (n *network) TryToGetVirtualInterfaceName(pciAddr string) string {
 func (n *network) TryGetInterfaceName(pciAddr string) string {
 	names, err := n.dputilsLib.GetNetNames(pciAddr)
 	if err != nil || len(names) < 1 {
-		log.Log.Error(err, "TryGetInterfaceName(): failed to get interface name")
+		log.Log.Error(err, "TryGetInterfaceName(): failed to get interface name", "pciAddress", pciAddr)
 		return ""
 	}
 	netDevName := names[0]

pkg/host/internal/vdpa/vdpa.go

@@ -94,11 +94,9 @@ func (v *vdpa) DeleteVDPADevice(pciAddr string) error {
 func (v *vdpa) DiscoverVDPAType(pciAddr string) string {
 	expectedVDPAName := generateVDPADevName(pciAddr)
 	funcLog := log.Log.WithValues("device", pciAddr, "name", expectedVDPAName)
-	funcLog.V(2).Info("DiscoverVDPAType() discover device type")
 	_, err := v.netlinkLib.VDPAGetDevByName(expectedVDPAName)
 	if err != nil {
 		if errors.Is(err, syscall.ENODEV) {
-			funcLog.V(2).Info("DiscoverVDPAType(): VDPA device for VF not found")
 			return ""
 		}
 		if errors.Is(err, syscall.ENOENT) {