From b588f9cc05b8eca9d345c56dc99059e616d835a0 Mon Sep 17 00:00:00 2001 From: Andrej Krejcir Date: Wed, 7 Feb 2024 15:07:42 +0100 Subject: [PATCH 1/2] 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. Cherry-pick fron: 948eeaaa2d816966e6d5340c051175f5b7deb59b Signed-off-by: Andrej Krejcir --- controllers/controllers_suite_test.go | 13 +++ controllers/reconciler.go | 4 + controllers/setup.go | 5 ++ controllers/webhook_controller.go | 110 +++++++++++++++++++++++ controllers/wehook_test.go | 120 ++++++++++++++++++++++++++ tests/webhook_controller_test.go | 95 ++++++++++++++++++++ 6 files changed, 347 insertions(+) create mode 100644 controllers/controllers_suite_test.go create mode 100644 controllers/webhook_controller.go create mode 100644 controllers/wehook_test.go create mode 100644 tests/webhook_controller_test.go diff --git a/controllers/controllers_suite_test.go b/controllers/controllers_suite_test.go new file mode 100644 index 000000000..df1b84055 --- /dev/null +++ b/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") +} diff --git a/controllers/reconciler.go b/controllers/reconciler.go index 06aa924bb..e5415f3f3 100644 --- a/controllers/reconciler.go +++ b/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 } diff --git a/controllers/setup.go b/controllers/setup.go index da6ff3ea2..a9866894f 100644 --- a/controllers/setup.go +++ b/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 { diff --git a/controllers/webhook_controller.go b/controllers/webhook_controller.go new file mode 100644 index 000000000..26fd14074 --- /dev/null +++ b/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 +} diff --git a/controllers/wehook_test.go b/controllers/wehook_test.go new file mode 100644 index 000000000..ab89e2b08 --- /dev/null +++ b/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)) + }) +}) diff --git a/tests/webhook_controller_test.go b/tests/webhook_controller_test.go new file mode 100644 index 000000000..53e4b305a --- /dev/null +++ b/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()) + }) +}) From 52995a8fa97e7f80939896d06d8055f9a24b06b7 Mon Sep 17 00:00:00 2001 From: Andrej Krejcir Date: Thu, 2 May 2024 13:37:41 +0200 Subject: [PATCH 2/2] fix: webhook-controller backport Some changes were needed from the PR in main branch. - "slices" package is not yet in go 1.19 - The "TypeMeta" needs to be defined in objects used in unit tests. - Do not use "k8s.io/utils/ptr" package. Signed-off-by: Andrej Krejcir --- controllers/webhook_controller.go | 12 ++++++++++-- controllers/wehook_test.go | 11 ++++++++--- tests/webhook_controller_test.go | 4 ++-- 3 files changed, 20 insertions(+), 7 deletions(-) diff --git a/controllers/webhook_controller.go b/controllers/webhook_controller.go index 26fd14074..377234ffc 100644 --- a/controllers/webhook_controller.go +++ b/controllers/webhook_controller.go @@ -2,7 +2,6 @@ package controllers import ( "context" - "slices" admissionv1 "k8s.io/api/admissionregistration/v1" "k8s.io/apimachinery/pkg/api/errors" @@ -94,7 +93,7 @@ func updateWebhookConfiguration(webhookConfig *admissionv1.ValidatingWebhookConf // Check if the webhook reacts to SSP resource. var hasSspRule bool for _, rule := range webhook.Rules { - if slices.Contains(rule.APIGroups, sspv1beta2.GroupVersion.Group) { + if containsString(rule.APIGroups, sspv1beta2.GroupVersion.Group) { hasSspRule = true break } @@ -108,3 +107,12 @@ func updateWebhookConfiguration(webhookConfig *admissionv1.ValidatingWebhookConf } return changed } + +func containsString(slice []string, value string) bool { + for _, s := range slice { + if s == value { + return true + } + } + return false +} diff --git a/controllers/wehook_test.go b/controllers/wehook_test.go index ab89e2b08..9b84648b2 100644 --- a/controllers/wehook_test.go +++ b/controllers/wehook_test.go @@ -9,7 +9,7 @@ import ( admissionv1 "k8s.io/api/admissionregistration/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" - "k8s.io/utils/ptr" + "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" @@ -28,7 +28,12 @@ var _ = Describe("Webhook controller", func() { ) 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{ @@ -41,7 +46,7 @@ var _ = Describe("Webhook controller", func() { Service: &admissionv1.ServiceReference{ Namespace: "test-namespace", Name: "test-name", - Path: ptr.To("/webhook"), + Path: pointer.String("/webhook"), }, }, Rules: []admissionv1.RuleWithOperations{{ @@ -59,7 +64,7 @@ var _ = Describe("Webhook controller", func() { "test-namespace-label": "some-value", }, }, - SideEffects: ptr.To(admissionv1.SideEffectClassNone), + SideEffects: &sideEffectsNone, AdmissionReviewVersions: []string{"v1"}, }}, } diff --git a/tests/webhook_controller_test.go b/tests/webhook_controller_test.go index 53e4b305a..5960e8449 100644 --- a/tests/webhook_controller_test.go +++ b/tests/webhook_controller_test.go @@ -8,7 +8,6 @@ import ( 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" @@ -20,6 +19,7 @@ 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-", @@ -49,7 +49,7 @@ var _ = Describe("Webhook controller", func() { "ssp-webhook-test-label": "ssp-webhook-test-label-vale", }, }, - SideEffects: ptr.To(admissionv1.SideEffectClassNone), + SideEffects: &sideEffectsNone, AdmissionReviewVersions: []string{"v1"}, }}, }