Merge pull request #979 from akrejcir/webhook-namespace-selector-0.18

[release-v0.18] fix: Remove namespaceSelector from webhook definition

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

@@ -146,6 +146,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,118 @@
+package controllers
+
+import (
+	"context"
+
+	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 containsString(rule.APIGroups, sspv1beta2.GroupVersion.Group) {
+				hasSspRule = true
+				break
+			}
+		}
+		if !hasSspRule {
+			continue
+		}
+
+		webhook.NamespaceSelector = nil
+		changed = true
+	}
+	return changed
+}
+
+func containsString(slice []string, value string) bool {
+	for _, s := range slice {
+		if s == value {
+			return true
+		}
+	}
+	return false
+}

controllers/wehook_test.go

@@ -0,0 +1,125 @@
+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/pointer"
+	"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() {
+		sideEffectsNone := admissionv1.SideEffectClassNone
+		webhookConfig = &admissionv1.ValidatingWebhookConfiguration{
+			TypeMeta: metav1.TypeMeta{
+				APIVersion: admissionv1.SchemeGroupVersion.String(),
+				Kind:       "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:      pointer.String("/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:             &sideEffectsNone,
+				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))
+	})
+})

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"
+	"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() {
+		sideEffectsNone := admissionv1.SideEffectClassNone
+		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:             &sideEffectsNone,
+				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())
+	})
+})