diff --git a/config/webhook/manifests.yaml b/config/webhook/manifests.yaml index bdf360c22..b2cb0193c 100644 --- a/config/webhook/manifests.yaml +++ b/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: diff --git a/config/webhook/webhook_patch.yaml b/config/webhook/webhook_patch.yaml index b7d9bcb81..f3c8c6b88 100644 --- a/config/webhook/webhook_patch.yaml +++ b/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" diff --git a/internal/hncconfig/reconciler.go b/internal/hncconfig/reconciler.go index 2d9807be6..4b06a9ee4 100644 --- a/internal/hncconfig/reconciler.go +++ b/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 diff --git a/internal/integtest/setup.go b/internal/integtest/setup.go index 071d88d67..302952ee0 100644 --- a/internal/integtest/setup.go +++ b/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() diff --git a/internal/objects/validator.go b/internal/objects/validator.go index 9d2407eb1..3820410a0 100644 --- a/internal/objects/validator.go +++ b/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 diff --git a/internal/setup/webhooks.go b/internal/setup/webhooks.go index 93451857c..ed2a13de6 100644 --- a/internal/setup/webhooks.go +++ b/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, }) diff --git a/internal/webhooks/webhooks.go b/internal/webhooks/webhooks.go index db02fcd15..32d703d07 100644 --- a/internal/webhooks/webhooks.go +++ b/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 {