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. Signed-off-by: Andrej Krejcir <[email protected]>
kubevirt · Feb 9, 2024 · d1f74be · d1f74be
1 parent 4b42532
commit d1f74be
Show file tree

Hide file tree

Showing 4 changed files with 191 additions and 3 deletions.
diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml
@@ -33,6 +33,7 @@ rules:
   - create
   - delete
   - list
+  - patch
   - update
   - watch
 - apiGroups:

diff --git a/data/olm-catalog/ssp-operator.clusterserviceversion.yaml b/data/olm-catalog/ssp-operator.clusterserviceversion.yaml
@@ -91,6 +91,7 @@ spec:
           - create
           - delete
           - list
+          - patch
           - update
           - watch
         - apiGroups:

diff --git a/tests/webhook_test.go b/tests/webhook_test.go
@@ -6,15 +6,20 @@ import (
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
+
+	admissionv1 "k8s.io/api/admissionregistration/v1"
+	apps "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	sspv1beta1 "kubevirt.io/ssp-operator/api/v1beta1"
+	"k8s.io/utils/ptr"
+	"kubevirt.io/controller-lifecycle-operator-sdk/api"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
-	"kubevirt.io/controller-lifecycle-operator-sdk/api"
+	sspv1beta1 "kubevirt.io/ssp-operator/api/v1beta1"
 	sspv1beta2 "kubevirt.io/ssp-operator/api/v1beta2"
+	"kubevirt.io/ssp-operator/tests/env"
 )
 
 // Placement API tests variables
@@ -206,4 +211,113 @@ var _ = Describe("Validation webhook", func() {
 			}, 20*time.Second, time.Second).Should(MatchError(ContainSubstring("missing name in DataImportCronTemplate")))
 		})
 	})
+
+	Context("ValidatingWebhookConfiguration change", func() {
+		var testWebhookConfigName string
+
+		BeforeEach(func() {
+			strategy.SkipSspUpdateTestsIfNeeded()
+
+			webhookConfig := &admissionv1.ValidatingWebhookConfiguration{
+				ObjectMeta: v1.ObjectMeta{
+					GenerateName: "test-ssp-webhook-config-",
+				},
+				Webhooks: []admissionv1.ValidatingWebhook{{
+					Name: "ssp.validation.test",
+					Rules: []admissionv1.RuleWithOperations{{
+						Rule: admissionv1.Rule{
+							APIGroups: []string{sspv1beta2.GroupVersion.Group},
+							APIVersions: []string{
+								sspv1beta1.GroupVersion.Version,
+								sspv1beta2.GroupVersion.Version,
+							},
+							Resources: []string{"ssps"},
+						},
+						Operations: []admissionv1.OperationType{
+							admissionv1.Create, admissionv1.Update,
+						},
+					}},
+				}},
+			}
+
+			Expect(apiClient.Create(ctx, webhookConfig)).To(Succeed())
+			testWebhookConfigName = webhookConfig.Name
+
+			DeferCleanup(func() {
+				err := apiClient.Delete(ctx, webhookConfig)
+				if !errors.IsNotFound(err) {
+					Expect(err).ToNot(HaveOccurred())
+				}
+				testWebhookConfigName = ""
+			})
+		})
+
+		It("[test_id:TODO] should remove namespaceSelector from webhook on restart", func() {
+			webhookConfig := &admissionv1.ValidatingWebhookConfiguration{}
+			Expect(apiClient.Get(ctx, client.ObjectKey{Name: testWebhookConfigName}, webhookConfig)).To(Succeed())
+
+			webhookConfig.Webhooks[0].NamespaceSelector = &v1.LabelSelector{
+				MatchLabels: map[string]string{"test-label": "test-label-value"},
+			}
+
+			Expect(apiClient.Update(ctx, webhookConfig)).To(Succeed())
+
+			sspDeploymentKey := client.ObjectKey{
+				Name:      strategy.GetSSPDeploymentName(),
+				Namespace: strategy.GetSSPDeploymentNameSpace(),
+			}
+
+			// Scale down SSP deployment
+			var originalReplicas *int32
+			Eventually(func(g Gomega) {
+				deployment := &apps.Deployment{}
+				g.Expect(apiClient.Get(ctx, sspDeploymentKey, deployment)).To(Succeed())
+				originalReplicas = deployment.Spec.Replicas
+				deployment.Spec.Replicas = ptr.To[int32](0)
+				g.Expect(apiClient.Update(ctx, deployment)).To(Succeed())
+			}, env.ShortTimeout(), time.Second).Should(Succeed())
+
+			DeferCleanup(func() {
+				// Restore deployment also if test fails
+				Eventually(func(g Gomega) {
+					deployment := &apps.Deployment{}
+					g.Expect(apiClient.Get(ctx, sspDeploymentKey, deployment)).To(Succeed())
+					if *deployment.Spec.Replicas == *originalReplicas {
+						return
+					}
+					deployment.Spec.Replicas = originalReplicas
+					g.Expect(apiClient.Update(ctx, deployment)).To(Succeed())
+				}, env.ShortTimeout(), time.Second).Should(Succeed())
+			})
+
+			// Wait until no replicas are running
+			Eventually(func(g Gomega) {
+				deployment := &apps.Deployment{}
+				g.Expect(apiClient.Get(ctx, sspDeploymentKey, deployment)).To(Succeed())
+				g.Expect(deployment.Status.Replicas).To(BeZero())
+			}, env.ShortTimeout(), time.Second).Should(Succeed())
+
+			// Scale up SSP deployment
+			Eventually(func(g Gomega) {
+				deployment := &apps.Deployment{}
+				g.Expect(apiClient.Get(ctx, sspDeploymentKey, deployment)).To(Succeed())
+				deployment.Spec.Replicas = originalReplicas
+				g.Expect(apiClient.Update(ctx, deployment)).To(Succeed())
+			}, env.ShortTimeout(), time.Second).Should(Succeed())
+
+			// Wait until deployment is ready
+			Eventually(func(g Gomega) {
+				deployment := &apps.Deployment{}
+				g.Expect(apiClient.Get(ctx, sspDeploymentKey, deployment)).To(Succeed())
+				g.Expect(deployment.Status.ReadyReplicas).To(Equal(*deployment.Spec.Replicas))
+			}, env.ShortTimeout(), time.Second).Should(Succeed())
+
+			// Check that namespaceSelector is not set
+			Eventually(func(g Gomega) {
+				webhookConfig := &admissionv1.ValidatingWebhookConfiguration{}
+				g.Expect(apiClient.Get(ctx, client.ObjectKey{Name: testWebhookConfigName}, webhookConfig)).To(Succeed())
+				g.Expect(webhookConfig.Webhooks[0].NamespaceSelector).To(BeNil())
+			}, env.ShortTimeout(), 1*time.Second).Should(Succeed()) // TODO -- are these timeouts correct?
+		})
+	})
 })
diff --git a/webhooks/ssp_webhook.go b/webhooks/ssp_webhook.go
@@ -19,13 +19,18 @@ package webhooks
 import (
 	"context"
 	"fmt"
+	"slices"
 	"strings"
 
+	"gomodules.xyz/jsonpatch/v2"
+	admissionv1 "k8s.io/api/admissionregistration/v1"
 	apps "k8s.io/api/apps/v1"
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/apimachinery/pkg/util/json"
 	"k8s.io/utils/ptr"
 	"kubevirt.io/controller-lifecycle-operator-sdk/api"
 	ctrl "sigs.k8s.io/controller-runtime"
@@ -37,9 +42,17 @@ import (
 	sspv1beta2 "kubevirt.io/ssp-operator/api/v1beta2"
 )
 
+// RBAC Rule needed by webhook
+// +kubebuilder:rbac:groups=admissionregistration.k8s.io,resources=validatingwebhookconfigurations,verbs=list;patch
+
 var ssplog = logf.Log.WithName("ssp-resource")
 
-func Setup(mgr ctrl.Manager) error {
+func Setup(ctx context.Context, mgr ctrl.Manager) error {
+	err := patchValidatingWebhookConfiguration(ctx, mgr)
+	if err != nil {
+		return fmt.Errorf("failed patching webhook configuration: %w", err)
+	}
+
 	// This is a hack. Using Unstructured allows the webhook to correctly decode different versions of objects.
 	// Controller-runtime currently does not support a single webhook for multiple versions.
 
@@ -53,6 +66,65 @@ func Setup(mgr ctrl.Manager) error {
 		Complete()
 }
 
+// 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. Luckily the OLM does not watch and reconcile the ValidatingWebhookConfiguration,
+// so we can simply reset the namespaceSelector
+func patchValidatingWebhookConfiguration(ctx context.Context, mgr ctrl.Manager) error {
+	configList := &admissionv1.ValidatingWebhookConfigurationList{}
+	err := mgr.GetAPIReader().List(ctx, configList,
+		client.MatchingLabels{"olm.webhook-description-generate-name": "validation.ssp.kubevirt.io"},
+	)
+	if err != nil {
+		return fmt.Errorf("failed to list ValidatingWebhookConfigurations: %w", err)
+	}
+
+	for i := range configList.Items {
+		webhookConfig := &configList.Items[i]
+		var patchOperations []jsonpatch.Operation
+
+		for j := range webhookConfig.Webhooks {
+			webhook := &webhookConfig.Webhooks[j]
+			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
+			}
+
+			patchOperations = append(patchOperations, jsonpatch.NewOperation(
+				"remove",
+				fmt.Sprintf("/webhooks/%d/namespaceSelector", j),
+				nil,
+			))
+		}
+		if len(patchOperations) == 0 {
+			continue
+		}
+
+		patchBytes, err := json.Marshal(patchOperations)
+		if err != nil {
+			return fmt.Errorf("faied to encode json patch for ValidatingWebhookConfiguration: %w", err)
+		}
+
+		patch := client.RawPatch(types.JSONPatchType, patchBytes)
+		err = mgr.GetClient().Patch(ctx, webhookConfig, patch)
+		if err != nil {
+			return fmt.Errorf("failed to patch ValidatingWebhookConfiguration: %w", err)
+		}
+	}
+	return nil
+}
+
 // +kubebuilder:webhook:verbs=create;update,path=/validate-ssp-kubevirt-io-v1beta2-ssp,mutating=false,failurePolicy=fail,groups=ssp.kubevirt.io,resources=ssps,versions=v1beta1;v1beta2,name=validation.ssp.kubevirt.io,admissionReviewVersions=v1,sideEffects=None
 
 type sspValidator struct {
-Original file line number
+Diff line change
@@ Expand Up / @@ -33,6 +33,7 @@ rules: @@
       - create
       - delete
       - list
+      - patch
       - update
       - watch
     - apiGroups:
@@ Expand Down @@