Merge pull request #285 from erikgb/dynamic-objects-webhook

Only create per-object webhooks for configured types

Author: k8s-ci-robot

config/webhook/manifests.yaml

@@ -71,27 +71,6 @@ webhooks:
     resources:
     - hierarchyconfigurations
   sideEffects: None
-- admissionReviewVersions:
-  - v1
-  clientConfig:
-    service:
-      name: webhook-service
-      namespace: system
-      path: /validate-objects
-  failurePolicy: Fail
-  name: objects.hnc.x-k8s.io
-  rules:
-  - apiGroups:
-    - '*'
-    apiVersions:
-    - '*'
-    operations:
-    - CREATE
-    - UPDATE
-    - DELETE
-    resources:
-    - '*'
-  sideEffects: None
 - admissionReviewVersions:
   - v1
   clientConfig:

config/webhook/webhook_patch.yaml

@@ -1,10 +1,21 @@
 ---
+# Intentionally no rules, as these are maintained by the HNCConfiguration reconciler.
+# At present controller-gen is unable to generate a webhook without rules from kubebuilder markers.
 apiVersion: admissionregistration.k8s.io/v1
 kind: ValidatingWebhookConfiguration
 metadata:
   name: validating-webhook-configuration
 webhooks:
-- name: objects.hnc.x-k8s.io
+- admissionReviewVersions:
+    - v1
+  clientConfig:
+    service:
+      name: webhook-service
+      namespace: system
+      path: /validate-objects
+  failurePolicy: Fail
+  name: objects.hnc.x-k8s.io
+  sideEffects: None
   timeoutSeconds: 2
   # We only apply this object validator on non-excluded namespaces, which have
   # the "included-namespace" label set by the HC reconciler, so that when HNC
@@ -19,19 +30,3 @@ webhooks:
   namespaceSelector:
     matchLabels:
       hnc.x-k8s.io/included-namespace: "true"
-  rules:
-  # This overwrites the rules specified in the object validator to patch object
-  # scope of `namespaced` since there's no kubebuilder marker for `scope`.
-  # There's no way to just patch "scope: Namespaced" to the first rule, since
-  # `rules` takes a list of rules and we can only patch the entire `rules`.
-  - apiGroups:
-    - '*'
-    apiVersions:
-    - '*'
-    operations:
-    - CREATE
-    - UPDATE
-    - DELETE
-    resources:
-    - '*'
-    scope: "Namespaced"

internal/hncconfig/reconciler.go

@@ -8,6 +8,7 @@ import (
 	"time"
 
 	"github.com/go-logr/logr"
+	apiadmissionregistrationv1 "k8s.io/api/admissionregistration/v1"
 	apiextensions "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
@@ -27,6 +28,7 @@ import (
 	"sigs.k8s.io/hierarchical-namespaces/internal/forest"
 	"sigs.k8s.io/hierarchical-namespaces/internal/objects"
 	"sigs.k8s.io/hierarchical-namespaces/internal/stats"
+	"sigs.k8s.io/hierarchical-namespaces/internal/webhooks"
 )
 
 // Reconciler is responsible for determining the HNC configuration from the HNCConfiguration CR,
@@ -84,6 +86,10 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 		return ctrl.Result{}, err
 	}
 
+	if err := r.syncObjectWebhookConfigs(ctx); err != nil {
+		return ctrl.Result{}, err
+	}
+
 	// Create or sync corresponding ObjectReconcilers, if needed.
 	syncErr := r.syncObjectReconcilers(ctx, inst)
 
@@ -232,6 +238,41 @@ func (r *Reconciler) writeSingleton(ctx context.Context, inst *api.HNCConfigurat
 	return nil
 }
 
+func (r *Reconciler) syncObjectWebhookConfigs(ctx context.Context) error {
+	vwc := &apiadmissionregistrationv1.ValidatingWebhookConfiguration{}
+	if err := r.Get(ctx, client.ObjectKey{Name: webhooks.ValidatingWebhookConfigurationName}, vwc); err != nil {
+		return err
+	}
+	cleanVWC := vwc.DeepCopy()
+
+	for i, wh := range vwc.Webhooks {
+		if wh.Name == webhooks.ObjectsWebhookName {
+			vwc.Webhooks[i].Rules = objectWebhookRules(r.activeGVKMode)
+			return r.Patch(ctx, vwc, client.MergeFrom(cleanVWC))
+		}
+	}
+	return fmt.Errorf("webhook %q not found in ValidatingWebhookConfiguration %q", webhooks.ObjectsWebhookName, webhooks.ValidatingWebhookConfigurationName)
+}
+
+func objectWebhookRules(mode gr2gvkMode) []apiadmissionregistrationv1.RuleWithOperations {
+	// Group GR by group to make nicer rules
+	g2r := make(map[string][]string)
+	for gr := range mode {
+		g2r[gr.Group] = append(g2r[gr.Group], gr.Resource)
+	}
+
+	var rules []apiadmissionregistrationv1.RuleWithOperations
+	for g, r := range g2r {
+		rule := apiadmissionregistrationv1.RuleWithOperations{}
+		rule.APIGroups = []string{g}
+		rule.Resources = r
+		rule.APIVersions = []string{"*"}
+		rule.Operations = []apiadmissionregistrationv1.OperationType{apiadmissionregistrationv1.Create, apiadmissionregistrationv1.Update, apiadmissionregistrationv1.Delete}
+		rules = append(rules, rule)
+	}
+	return rules
+}
+
 // syncObjectReconcilers creates or syncs ObjectReconcilers.
 //
 // For newly added types in the HNC configuration, the method will create corresponding ObjectReconcilers and

internal/integtest/setup.go

@@ -23,14 +23,21 @@ import (
 
 	. "github.com/onsi/ginkgo/v2" //lint:ignore ST1001 Ignoring this for now
 	. "github.com/onsi/gomega"    //lint:ignore ST1001 Ignoring this for now
+	apiadmissionregistrationv1 "k8s.io/api/admissionregistration/v1"
 	corev1 "k8s.io/api/core/v1"
 	apiextensions "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/kubernetes/scheme"
+	"k8s.io/utils/pointer"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/envtest"
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/controller-runtime/pkg/log/zap"
+	"sigs.k8s.io/controller-runtime/pkg/webhook"
+	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
+	"sigs.k8s.io/hierarchical-namespaces/internal/objects"
+	"sigs.k8s.io/hierarchical-namespaces/internal/webhooks"
 
 	// +kubebuilder:scaffold:imports
 
@@ -69,8 +76,28 @@ func HNCBeforeSuite() {
 	SetDefaultEventuallyTimeout(time.Second * 4)
 
 	By("configuring test environment")
+	sideEffectClassNone := apiadmissionregistrationv1.SideEffectClassNone
 	testEnv = &envtest.Environment{
 		CRDDirectoryPaths: []string{filepath.Join("..", "..", "config", "crd", "bases")},
+		WebhookInstallOptions: envtest.WebhookInstallOptions{
+			ValidatingWebhooks: []*apiadmissionregistrationv1.ValidatingWebhookConfiguration{{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: webhooks.ValidatingWebhookConfigurationName,
+				},
+				Webhooks: []apiadmissionregistrationv1.ValidatingWebhook{{
+					Name:                    webhooks.ObjectsWebhookName,
+					AdmissionReviewVersions: []string{"v1"},
+					SideEffects:             &sideEffectClassNone,
+					ClientConfig: apiadmissionregistrationv1.WebhookClientConfig{
+						Service: &apiadmissionregistrationv1.ServiceReference{
+							Namespace: "system",
+							Name:      "webhook-service",
+							Path:      pointer.String(objects.ServingPath),
+						},
+					},
+				}},
+			}},
+		},
 	}
 
 	By("starting test environment")
@@ -94,13 +121,20 @@ func HNCBeforeSuite() {
 	// CF: https://github.com/microsoft/azure-databricks-operator/blob/0f722a710fea06b86ecdccd9455336ca712bf775/controllers/suite_test.go
 
 	By("creating manager")
+	webhookInstallOptions := &testEnv.WebhookInstallOptions
 	k8sManager, err := ctrl.NewManager(cfg, ctrl.Options{
 		NewClient:          config.NewClient(false),
 		MetricsBindAddress: "0", // disable metrics serving since 'go test' runs multiple suites in parallel processes
 		Scheme:             scheme.Scheme,
+		Host:               webhookInstallOptions.LocalServingHost,
+		Port:               webhookInstallOptions.LocalServingPort,
+		CertDir:            webhookInstallOptions.LocalServingCertDir,
 	})
 	Expect(err).ToNot(HaveOccurred())
 
+	// Register a dummy webhook since the test control plane is to test reconcilers
+	k8sManager.GetWebhookServer().Register(objects.ServingPath, &webhook.Admission{Handler: &allowAllHandler{}})
+
 	By("creating reconcilers")
 	opts := setup.Options{
 		MaxReconciles: 100,
@@ -125,6 +159,12 @@ func HNCBeforeSuite() {
 	}()
 }
 
+type allowAllHandler struct{}
+
+func (a allowAllHandler) Handle(_ context.Context, _ admission.Request) admission.Response {
+	return webhooks.Allow("All requests are allowed by allowAllHandler")
+}
+
 func HNCAfterSuite() {
 	if k8sManagerCancelFn != nil {
 		k8sManagerCancelFn()

internal/objects/validator.go

@@ -32,17 +32,6 @@ const (
 	ServingPath = "/validate-objects"
 )
 
-// Note: the validating webhook FAILS CLOSE. This means that if the webhook goes
-// down, all further changes are forbidden. In addition, the webhook `rules`
-// (groups, resources, versions, verbs) specified in the below kubebuilder marker
-// are overwritten by the `rules` configured in config/webhook/webhook_patch.yaml,
-// because there's no marker for `scope` and we only want this object webhook
-// to work on `namespaced` objects. Please make sure you edit the webhook_patch.yaml
-// file if you want to change the webhook `rules` and better make the rules
-// here the same as what's in the webhook_patch.yaml.
-//
-// +kubebuilder:webhook:admissionReviewVersions=v1,path=/validate-objects,mutating=false,failurePolicy=fail,groups="*",resources="*",sideEffects=None,verbs=create;update;delete,versions="*",name=objects.hnc.x-k8s.io
-
 type Validator struct {
 	Log     logr.Logger
 	Forest  *forest.Forest

internal/setup/webhooks.go

@@ -16,12 +16,11 @@ import (
 	"sigs.k8s.io/hierarchical-namespaces/internal/hrq"
 	ns "sigs.k8s.io/hierarchical-namespaces/internal/namespace" // for some reason, by default this gets imported as "namespace*s*"
 	"sigs.k8s.io/hierarchical-namespaces/internal/objects"
+	"sigs.k8s.io/hierarchical-namespaces/internal/webhooks"
 )
 
 const (
 	serviceName       = "hnc-webhook-service"
-	vwhName           = "hnc-validating-webhook-configuration"
-	mwhName           = "hnc-mutating-webhook-configuration"
 	caName            = "hnc-ca"
 	caOrganization    = "hnc"
 	secretName        = "hnc-webhook-server-cert"
@@ -50,10 +49,10 @@ func ManageCerts(mgr ctrl.Manager, setupFinished chan struct{}, restartOnSecretR
 		IsReady:        setupFinished,
 		Webhooks: []cert.WebhookInfo{{
 			Type: cert.Validating,
-			Name: vwhName,
+			Name: webhooks.ValidatingWebhookConfigurationName,
 		}, {
 			Type: cert.Mutating,
-			Name: mwhName,
+			Name: webhooks.MutatingWebhookConfigurationName,
 		}},
 		RestartOnSecretRefresh: restartOnSecretRefresh,
 	})

internal/webhooks/webhooks.go

@@ -14,6 +14,13 @@ import (
 	"sigs.k8s.io/hierarchical-namespaces/internal/config"
 )
 
+const (
+	ValidatingWebhookConfigurationName = "hnc-validating-webhook-configuration"
+	MutatingWebhookConfigurationName   = "hnc-mutating-webhook-configuration"
+
+	ObjectsWebhookName = "objects.hnc.x-k8s.io"
+)
+
 // IsHNCServiceAccount is inspired by isGKServiceAccount from open-policy-agent/gatekeeper.
 func IsHNCServiceAccount(user *authnv1.UserInfo) bool {
 	if user == nil {