fix: Remove namespaceSelector from webhook definition

The ValidatingWebhookConfiguration created by OLM can have
namespaceSelector field set. We don't want that, because
the webhook checks that there is only one SSP resource
in the cluster.

This commit adds a controller that watches
ValidatingWebhookConfigurations and removes the namespaceSelector,
if the object has a specific label.

Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>

config/rbac/role.yaml

@@ -32,6 +32,7 @@ rules:
   verbs:
   - create
   - delete
+  - get
   - list
   - update
   - watch

controllers/controllers_suite_test.go

@@ -0,0 +1,13 @@
+package controllers
+
+import (
+	"testing"
+
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+)
+
+func TestControllers(t *testing.T) {
+	RegisterFailHandler(Fail)
+	RunSpecs(t, "Controllers Suite")
+}

controllers/reconciler.go

@@ -2,10 +2,14 @@ package controllers
 
 import (
 	"context"
+
 	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 )
 
 type ControllerReconciler interface {
+	reconcile.Reconciler
+
 	Start(ctx context.Context, mgr ctrl.Manager) error
 	Name() string
 }

controllers/setup.go

@@ -148,6 +148,11 @@ func setupManager(ctx context.Context, cancel context.CancelFunc, mgr controller
 		return fmt.Errorf("error adding service controller: %w", err)
 	}
 
+	webhookConfigController := NewWebhookConfigurationController(mgr.GetClient())
+	if err = mgr.Add(getRunnable(mgr, webhookConfigController)); err != nil {
+		return fmt.Errorf("error adding webhook configuration controller: %w", err)
+	}
+
 	if crdWatch.CrdExists(vmCRD) {
 		vmController, cErr := CreateVmController(mgr)
 		if cErr != nil {

controllers/webhook_controller.go

@@ -0,0 +1,110 @@
+package controllers
+
+import (
+	"context"
+	"slices"
+
+	admissionv1 "k8s.io/api/admissionregistration/v1"
+	"k8s.io/apimachinery/pkg/api/errors"
+	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/builder"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/predicate"
+	"sigs.k8s.io/controller-runtime/pkg/reconcile"
+
+	sspv1beta2 "kubevirt.io/ssp-operator/api/v1beta2"
+)
+
+const (
+	OlmNameLabel      = "olm.webhook-description-generate-name"
+	OlmNameLabelValue = "validation.ssp.kubevirt.io"
+)
+
+// +kubebuilder:rbac:groups=admissionregistration.k8s.io,resources=validatingwebhookconfigurations,verbs=get;update
+
+// CreateWebhookConfigurationController creates a controller
+// that watches ValidatingWebhookConfiguration created by OLM,
+// and removes any namespaceSelector defined in it.
+//
+// The OLM limits the webhook scope to the namespaces that are defined in the OperatorGroup
+// by setting namespaceSelector in the ValidatingWebhookConfiguration.
+// We would like our webhook to intercept requests from all namespaces.
+//
+// The SSP operator already watches all ValidatingWebhookConfigurations, because
+// of template validator operand, so this controller is not a performance issue.
+func NewWebhookConfigurationController(apiClient client.Client) ControllerReconciler {
+	return &webhookCtrl{
+		apiClient: apiClient,
+	}
+}
+
+type webhookCtrl struct {
+	apiClient client.Client
+}
+
+var _ ControllerReconciler = &webhookCtrl{}
+
+var _ reconcile.Reconciler = &webhookCtrl{}
+
+func (w *webhookCtrl) Start(_ context.Context, mgr ctrl.Manager) error {
+	return ctrl.NewControllerManagedBy(mgr).
+		Named(w.Name()).
+		For(&admissionv1.ValidatingWebhookConfiguration{}, builder.WithPredicates(
+			predicate.NewPredicateFuncs(hasExpectedLabel),
+		)).
+		Complete(w)
+}
+
+func (w *webhookCtrl) Name() string {
+	return "validating-webhook-controller"
+}
+
+func (w *webhookCtrl) Reconcile(ctx context.Context, request reconcile.Request) (reconcile.Result, error) {
+	webhookConfig := &admissionv1.ValidatingWebhookConfiguration{}
+	if err := w.apiClient.Get(ctx, request.NamespacedName, webhookConfig); err != nil {
+		if errors.IsNotFound(err) {
+			return reconcile.Result{}, nil
+		}
+		return reconcile.Result{}, err
+	}
+
+	if !hasExpectedLabel(webhookConfig) {
+		return reconcile.Result{}, nil
+	}
+
+	if changed := updateWebhookConfiguration(webhookConfig); !changed {
+		return reconcile.Result{}, nil
+	}
+
+	err := w.apiClient.Update(ctx, webhookConfig)
+	return reconcile.Result{}, err
+}
+
+func hasExpectedLabel(obj client.Object) bool {
+	return obj.GetLabels()[OlmNameLabel] == OlmNameLabelValue
+}
+
+func updateWebhookConfiguration(webhookConfig *admissionv1.ValidatingWebhookConfiguration) bool {
+	var changed bool
+	for i := range webhookConfig.Webhooks {
+		webhook := &webhookConfig.Webhooks[i]
+		if webhook.NamespaceSelector == nil {
+			continue
+		}
+		// Check if the webhook reacts to SSP resource.
+		var hasSspRule bool
+		for _, rule := range webhook.Rules {
+			if slices.Contains(rule.APIGroups, sspv1beta2.GroupVersion.Group) {
+				hasSspRule = true
+				break
+			}
+		}
+		if !hasSspRule {
+			continue
+		}
+
+		webhook.NamespaceSelector = nil
+		changed = true
+	}
+	return changed
+}

controllers/wehook_test.go

@@ -0,0 +1,120 @@
+package controllers
+
+import (
+	"context"
+
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+
+	admissionv1 "k8s.io/api/admissionregistration/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/utils/ptr"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+	"sigs.k8s.io/controller-runtime/pkg/reconcile"
+
+	sspv1beta2 "kubevirt.io/ssp-operator/api/v1beta2"
+	"kubevirt.io/ssp-operator/internal/common"
+)
+
+var _ = Describe("Webhook controller", func() {
+
+	var (
+		webhookConfig  *admissionv1.ValidatingWebhookConfiguration
+		fakeClient     client.Client
+		testController ControllerReconciler
+		testRequest    reconcile.Request
+	)
+
+	BeforeEach(func() {
+		webhookConfig = &admissionv1.ValidatingWebhookConfiguration{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: "test-webhook",
+				Labels: map[string]string{
+					OlmNameLabel: OlmNameLabelValue,
+				},
+			},
+			Webhooks: []admissionv1.ValidatingWebhook{{
+				Name: "test-ssp-webhook",
+				ClientConfig: admissionv1.WebhookClientConfig{
+					Service: &admissionv1.ServiceReference{
+						Namespace: "test-namespace",
+						Name:      "test-name",
+						Path:      ptr.To("/webhook"),
+					},
+				},
+				Rules: []admissionv1.RuleWithOperations{{
+					Rule: admissionv1.Rule{
+						APIGroups:   []string{sspv1beta2.GroupVersion.Group},
+						APIVersions: []string{sspv1beta2.GroupVersion.Version},
+						Resources:   []string{"ssps"},
+					},
+					Operations: []admissionv1.OperationType{
+						admissionv1.Create, admissionv1.Update,
+					},
+				}},
+				NamespaceSelector: &metav1.LabelSelector{
+					MatchLabels: map[string]string{
+						"test-namespace-label": "some-value",
+					},
+				},
+				SideEffects:             ptr.To(admissionv1.SideEffectClassNone),
+				AdmissionReviewVersions: []string{"v1"},
+			}},
+		}
+
+		fakeClient = fake.NewClientBuilder().WithScheme(common.Scheme).Build()
+
+		testController = NewWebhookConfigurationController(fakeClient)
+
+		testRequest = reconcile.Request{
+			NamespacedName: types.NamespacedName{
+				Name: webhookConfig.Name,
+			},
+		}
+	})
+
+	It("should remove namespaceSelector from webhook", func() {
+		Expect(fakeClient.Create(context.Background(), webhookConfig)).To(Succeed())
+
+		_, err := testController.Reconcile(context.Background(), testRequest)
+		Expect(err).ToNot(HaveOccurred())
+
+		updatedConfig := &admissionv1.ValidatingWebhookConfiguration{}
+		Expect(fakeClient.Get(context.Background(), client.ObjectKeyFromObject(webhookConfig), updatedConfig)).To(Succeed())
+
+		Expect(updatedConfig.Webhooks).ToNot(BeEmpty())
+		for _, webhook := range updatedConfig.Webhooks {
+			Expect(webhook.NamespaceSelector).To(BeNil())
+		}
+	})
+
+	It("should not remove namespaceSelector from non SSP webhook", func() {
+		webhookConfig.Webhooks[0].Rules[0].APIGroups = []string{"non-ssp-api-group"}
+
+		Expect(fakeClient.Create(context.Background(), webhookConfig)).To(Succeed())
+
+		_, err := testController.Reconcile(context.Background(), testRequest)
+		Expect(err).ToNot(HaveOccurred())
+
+		updatedConfig := &admissionv1.ValidatingWebhookConfiguration{}
+		Expect(fakeClient.Get(context.Background(), client.ObjectKeyFromObject(webhookConfig), updatedConfig)).To(Succeed())
+
+		Expect(updatedConfig).To(Equal(webhookConfig))
+	})
+
+	It("should not remove namespaceSelector from webhook without labels", func() {
+		webhookConfig.Labels = nil
+
+		Expect(fakeClient.Create(context.Background(), webhookConfig)).To(Succeed())
+
+		_, err := testController.Reconcile(context.Background(), testRequest)
+		Expect(err).ToNot(HaveOccurred())
+
+		updatedConfig := &admissionv1.ValidatingWebhookConfiguration{}
+		Expect(fakeClient.Get(context.Background(), client.ObjectKeyFromObject(webhookConfig), updatedConfig)).To(Succeed())
+
+		Expect(updatedConfig).To(Equal(webhookConfig))
+	})
+})

data/olm-catalog/ssp-operator.clusterserviceversion.yaml

@@ -90,6 +90,7 @@ spec:
           verbs:
           - create
           - delete
+          - get
           - list
           - update
           - watch

tests/webhook_controller_test.go

@@ -0,0 +1,95 @@
+package tests
+
+import (
+	"time"
+
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+
+	admissionv1 "k8s.io/api/admissionregistration/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/utils/ptr"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+
+	sspv1beta2 "kubevirt.io/ssp-operator/api/v1beta2"
+	"kubevirt.io/ssp-operator/controllers"
+	"kubevirt.io/ssp-operator/tests/env"
+)
+
+var _ = Describe("Webhook controller", func() {
+	var webhook *admissionv1.ValidatingWebhookConfiguration
+
+	BeforeEach(func() {
+		webhook = &admissionv1.ValidatingWebhookConfiguration{
+			ObjectMeta: metav1.ObjectMeta{
+				GenerateName: "ssp.kubevirt.io-test-webhook-",
+				Labels: map[string]string{
+					controllers.OlmNameLabel: controllers.OlmNameLabelValue,
+				},
+			},
+			Webhooks: []admissionv1.ValidatingWebhook{{
+				Name: "test.webhook.ssp.kubevirt.io",
+				ClientConfig: admissionv1.WebhookClientConfig{
+					Service: &admissionv1.ServiceReference{
+						Name:      "non-existing-service",
+						Namespace: "non-existing-namespace",
+					},
+				},
+				Rules: []admissionv1.RuleWithOperations{{
+					// Using "Delete" so it does not conflict with existing SSP webhook
+					Operations: []admissionv1.OperationType{admissionv1.Delete},
+					Rule: admissionv1.Rule{
+						APIGroups:   []string{sspv1beta2.GroupVersion.Group},
+						APIVersions: []string{sspv1beta2.GroupVersion.Version},
+						Resources:   []string{"ssps"},
+					},
+				}},
+				NamespaceSelector: &metav1.LabelSelector{
+					MatchLabels: map[string]string{
+						"ssp-webhook-test-label": "ssp-webhook-test-label-vale",
+					},
+				},
+				SideEffects:             ptr.To(admissionv1.SideEffectClassNone),
+				AdmissionReviewVersions: []string{"v1"},
+			}},
+		}
+	})
+
+	It("[test_id:TODO] should delete namespaceSelector from webhook configuration with OLM label", func() {
+		Expect(apiClient.Create(ctx, webhook)).To(Succeed())
+		DeferCleanup(func() {
+			Expect(apiClient.Delete(ctx, webhook)).To(Succeed())
+		})
+
+		Eventually(func(g Gomega) {
+			updatedWebhook := &admissionv1.ValidatingWebhookConfiguration{}
+			g.Expect(apiClient.Get(ctx, client.ObjectKeyFromObject(webhook), updatedWebhook)).To(Succeed())
+			g.Expect(updatedWebhook.Webhooks).To(HaveLen(1))
+
+			namespaceSelector := updatedWebhook.Webhooks[0].NamespaceSelector
+			if namespaceSelector != nil {
+				g.Expect(namespaceSelector.MatchLabels).To(BeEmpty())
+				g.Expect(namespaceSelector.MatchExpressions).To(BeEmpty())
+			}
+		}, env.ShortTimeout(), time.Second).Should(Succeed())
+	})
+
+	It("[test_id:TODO] should not delete namespaceSelector from webhook configuration without OLM label", func() {
+		webhook.Labels = nil
+
+		Expect(apiClient.Create(ctx, webhook)).To(Succeed())
+		DeferCleanup(func() {
+			Expect(apiClient.Delete(ctx, webhook)).To(Succeed())
+		})
+
+		Consistently(func(g Gomega) {
+			updatedWebhook := &admissionv1.ValidatingWebhookConfiguration{}
+			g.Expect(apiClient.Get(ctx, client.ObjectKeyFromObject(webhook), updatedWebhook)).To(Succeed())
+			g.Expect(updatedWebhook.Webhooks).To(HaveLen(1))
+
+			namespaceSelector := updatedWebhook.Webhooks[0].NamespaceSelector
+			g.Expect(namespaceSelector).ToNot(BeNil())
+			g.Expect(namespaceSelector.MatchLabels).ToNot(BeEmpty())
+		}, 10*time.Second, time.Second).Should(Succeed())
+	})
+})