From f0c7f02ced53f0ba49497618680cc94126e618fb Mon Sep 17 00:00:00 2001 From: Bryan Cox Date: Thu, 6 Aug 2026 07:33:55 -0400 Subject: [PATCH 1/2] refactor(hcco): extract webhook validation into a dedicated controller Move webhook validation logic from the monolithic HCCO resources controller into its own webhookvalidation package with dedicated watches, reconciler, and tests. Service events now enqueue sentinel requests so that webhook config listing happens inside Reconcile where errors are retried by the work queue, rather than silently discarding list failures in the event handler. Co-Authored-By: Claude Opus 4.6 --- .../hostedclusterconfigoperator/cmd.go | 22 +- .../controllers/resources/resources.go | 66 --- .../controllers/resources/resources_test.go | 265 ----------- .../controllers/webhookvalidation/setup.go | 80 ++++ .../webhookvalidation/webhookvalidation.go | 168 +++++++ .../webhookvalidation_test.go | 425 ++++++++++++++++++ 6 files changed, 685 insertions(+), 341 deletions(-) create mode 100644 control-plane-operator/hostedclusterconfigoperator/controllers/webhookvalidation/setup.go create mode 100644 control-plane-operator/hostedclusterconfigoperator/controllers/webhookvalidation/webhookvalidation.go create mode 100644 control-plane-operator/hostedclusterconfigoperator/controllers/webhookvalidation/webhookvalidation_test.go diff --git a/control-plane-operator/hostedclusterconfigoperator/cmd.go b/control-plane-operator/hostedclusterconfigoperator/cmd.go index 29d8189ffd1b..36ae940f219d 100644 --- a/control-plane-operator/hostedclusterconfigoperator/cmd.go +++ b/control-plane-operator/hostedclusterconfigoperator/cmd.go @@ -32,6 +32,7 @@ import ( "github.com/openshift/hypershift/control-plane-operator/hostedclusterconfigoperator/controllers/reencryption" "github.com/openshift/hypershift/control-plane-operator/hostedclusterconfigoperator/controllers/resources" "github.com/openshift/hypershift/control-plane-operator/hostedclusterconfigoperator/controllers/spotremediation" + "github.com/openshift/hypershift/control-plane-operator/hostedclusterconfigoperator/controllers/webhookvalidation" "github.com/openshift/hypershift/control-plane-operator/hostedclusterconfigoperator/operator" hyperapi "github.com/openshift/hypershift/support/api" "github.com/openshift/hypershift/support/capabilities" @@ -64,16 +65,17 @@ func NewCommand() *cobra.Command { } var controllerFuncs = map[string]operator.ControllerSetupFunc{ - "controller-manager-ca": cmca.Setup, - resources.ControllerName: resources.Setup, - "inplaceupgrader": inplaceupgrader.Setup, - "node": node.Setup, - nodecount.ControllerName: nodecount.Setup, - "machine": machine.Setup, - "drainer": drainer.Setup, - hcpstatus.ControllerName: hcpstatus.Setup, - spotremediation.ControllerName: spotremediation.Setup, - reencryption.ControllerName: reencryption.Setup, + "controller-manager-ca": cmca.Setup, + resources.ControllerName: resources.Setup, + "inplaceupgrader": inplaceupgrader.Setup, + "node": node.Setup, + nodecount.ControllerName: nodecount.Setup, + "machine": machine.Setup, + "drainer": drainer.Setup, + hcpstatus.ControllerName: hcpstatus.Setup, + spotremediation.ControllerName: spotremediation.Setup, + reencryption.ControllerName: reencryption.Setup, + webhookvalidation.ControllerName: webhookvalidation.Setup, } type HostedClusterConfigOperator struct { diff --git a/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go b/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go index 707fa65a7ba8..4ab4c6aeef88 100644 --- a/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go +++ b/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go @@ -492,7 +492,6 @@ func (r *reconciler) reconcileStorageAndMisc(ctx context.Context, log logr.Logge log.Info("reconciling observed configuration") errs = append(errs, r.reconcileObservedConfiguration(ctx, hcp)...) - errs = append(errs, r.ensureGuestAdmissionWebhooksAreValid(ctx)) return errs } @@ -2935,61 +2934,6 @@ func (r *reconciler) reconcileRestoredCluster(ctx context.Context, hcp *hyperv1. return false, nil } -func (r *reconciler) ensureGuestAdmissionWebhooksAreValid(ctx context.Context) error { - log := ctrl.LoggerFrom(ctx) - - cpServices := &corev1.ServiceList{} - if err := r.cpClient.List(ctx, cpServices, client.InNamespace(r.hcpNamespace)); err != nil { - return fmt.Errorf("failed to list control plane services: %w", err) - } - - // disallow all urls targeting services in the hcp namespace by default unless 'hypershift.openshift.io/allow-guest-webhooks' label is present. - disallowedUrls := make([]string, 0) - for _, svc := range cpServices.Items { - if _, exist := svc.Labels[hyperv1.AllowGuestWebhooksServiceLabel]; exist { - continue - } - - disallowedUrls = append(disallowedUrls, fmt.Sprintf("https://%s", svc.Name)) - disallowedUrls = append(disallowedUrls, fmt.Sprintf("https://%s.%s.svc", svc.Name, svc.Namespace)) - disallowedUrls = append(disallowedUrls, fmt.Sprintf("https://%s.%s.svc.cluster.local", svc.Name, svc.Namespace)) - } - - validatingWebhookConfigurations := &admissionregistrationv1.ValidatingWebhookConfigurationList{} - if err := r.client.List(ctx, validatingWebhookConfigurations); err != nil { - return fmt.Errorf("failed to list validatingWebhookConfigurations: %w", err) - } - - errs := make([]error, 0) - for _, configuration := range validatingWebhookConfigurations.Items { - for _, webhook := range configuration.Webhooks { - if webhook.ClientConfig.URL != nil && !isAllowedWebhookUrl(disallowedUrls, *webhook.ClientConfig.URL) { - log.Info("deleting validating webhook configuration with a disallowed url", "webhook_name", configuration.Name, "disallowed_url", *webhook.ClientConfig.URL) - errs = append(errs, r.client.Delete(ctx, &configuration)) - break - } - } - } - - mutatingWebhookConfigurations := &admissionregistrationv1.MutatingWebhookConfigurationList{} - if err := r.client.List(ctx, mutatingWebhookConfigurations); err != nil { - errs = append(errs, fmt.Errorf("failed to list mutatingWebhookConfigurations: %w", err)) - return utilerrors.NewAggregate(errs) - } - - for _, configuration := range mutatingWebhookConfigurations.Items { - for _, webhook := range configuration.Webhooks { - if webhook.ClientConfig.URL != nil && !isAllowedWebhookUrl(disallowedUrls, *webhook.ClientConfig.URL) { - log.Info("deleting mutating webhook configuration with a disallowed url", "webhook_name", configuration.Name, "disallowed_url", *webhook.ClientConfig.URL) - errs = append(errs, r.client.Delete(ctx, &configuration)) - break - } - } - } - - return utilerrors.NewAggregate(errs) -} - // reconcileKubeletConfig Lists the KubeletConfig ConfigMaps from the controlPlane cluster // and copies them to the hosted cluster. // In addition, it deletes KubeletConfig ConfigMaps from the hosted cluster which are no longer relevant. @@ -3118,16 +3062,6 @@ func mutateKubeletConfig(controlPlaneConfigMap, hostedClusterConfigMap *corev1.C return nil } -func isAllowedWebhookUrl(disallowedUrls []string, url string) bool { - for i := range disallowedUrls { - if strings.Contains(url, disallowedUrls[i]) { - return false - } - } - - return true -} - func (r *reconciler) ensureResourceCreationIsBlocked(ctx context.Context, hcp *hyperv1.HostedControlPlane) error { wh := manifests.ResourceCreationBlockerWebhook() if _, err := r.CreateOrUpdate(ctx, r.client, wh, func() error { diff --git a/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go b/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go index c7b0a296699c..a64d76f412c8 100644 --- a/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go +++ b/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go @@ -34,7 +34,6 @@ import ( openshiftcpv1 "github.com/openshift/api/openshiftcontrolplane/v1" operatorv1 "github.com/openshift/api/operator/v1" - admissionregistrationv1 "k8s.io/api/admissionregistration/v1" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" discoveryv1 "k8s.io/api/discovery/v1" @@ -4075,270 +4074,6 @@ func TestCleanupLegacyResources(t *testing.T) { } } -func TestIsAllowedWebhookUrl(t *testing.T) { - t.Parallel() - tests := []struct { - name string - disallowedUrls []string - url string - expected bool - }{ - { - name: "When URL contains a disallowed substring it should return false", - disallowedUrls: []string{"https://etcd-client"}, - url: "https://etcd-client:2379", - expected: false, - }, - { - name: "When URL matches a fully qualified disallowed URL it should return false", - disallowedUrls: []string{"https://etcd-client.ns.svc"}, - url: "https://etcd-client.ns.svc:2379/path", - expected: false, - }, - { - name: "When URL does not match any disallowed URL it should return true", - disallowedUrls: []string{"https://etcd-client"}, - url: "https://external.example.com", - expected: true, - }, - { - name: "When disallowed list is empty it should return true", - disallowedUrls: []string{}, - url: "https://anything", - expected: true, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - g := NewWithT(t) - result := isAllowedWebhookUrl(tt.disallowedUrls, tt.url) - g.Expect(result).To(Equal(tt.expected)) - }) - } -} - -func TestEnsureGuestAdmissionWebhooksAreValid(t *testing.T) { - t.Parallel() - const hcpNamespace = "test-hcp-namespace" - - tests := []struct { - name string - cpServices []corev1.Service - guestObjects []client.Object - expectWebhookGone string - expectWebhookAlive string - }{ - { - name: "When validating webhook targets a CP service it should delete the webhook", - cpServices: []corev1.Service{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "etcd-client", - Namespace: hcpNamespace, - }, - }, - }, - guestObjects: []client.Object{ - &admissionregistrationv1.ValidatingWebhookConfiguration{ - ObjectMeta: metav1.ObjectMeta{Name: "test-validating-webhook"}, - Webhooks: []admissionregistrationv1.ValidatingWebhook{ - { - Name: "test.webhook.io", - ClientConfig: admissionregistrationv1.WebhookClientConfig{URL: ptr.To("https://etcd-client:2379")}, - }, - }, - }, - }, - expectWebhookGone: "test-validating-webhook", - }, - { - name: "When validating webhook targets an allowed CP service it should preserve the webhook", - cpServices: []corev1.Service{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "allowed-service", - Namespace: hcpNamespace, - Labels: map[string]string{hyperv1.AllowGuestWebhooksServiceLabel: "true"}, - }, - }, - }, - guestObjects: []client.Object{ - &admissionregistrationv1.ValidatingWebhookConfiguration{ - ObjectMeta: metav1.ObjectMeta{Name: "preserved-validating-webhook"}, - Webhooks: []admissionregistrationv1.ValidatingWebhook{ - { - Name: "preserved.webhook.io", - ClientConfig: admissionregistrationv1.WebhookClientConfig{URL: ptr.To("https://allowed-service:8443")}, - }, - }, - }, - }, - expectWebhookAlive: "preserved-validating-webhook", - }, - { - name: "When mutating webhook targets a CP service it should delete the webhook", - cpServices: []corev1.Service{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "kube-apiserver", - Namespace: hcpNamespace, - }, - }, - }, - guestObjects: []client.Object{ - &admissionregistrationv1.MutatingWebhookConfiguration{ - ObjectMeta: metav1.ObjectMeta{Name: "test-mutating-webhook"}, - Webhooks: []admissionregistrationv1.MutatingWebhook{ - { - Name: "mutating.webhook.io", - ClientConfig: admissionregistrationv1.WebhookClientConfig{URL: ptr.To("https://kube-apiserver:6443")}, - }, - }, - }, - }, - expectWebhookGone: "test-mutating-webhook", - }, - { - name: "When webhook targets an external URL it should preserve the webhook", - cpServices: []corev1.Service{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "etcd-client", - Namespace: hcpNamespace, - }, - }, - }, - guestObjects: []client.Object{ - &admissionregistrationv1.ValidatingWebhookConfiguration{ - ObjectMeta: metav1.ObjectMeta{Name: "external-validating-webhook"}, - Webhooks: []admissionregistrationv1.ValidatingWebhook{ - { - Name: "external.webhook.io", - ClientConfig: admissionregistrationv1.WebhookClientConfig{URL: ptr.To("https://external.example.com")}, - }, - }, - }, - }, - expectWebhookAlive: "external-validating-webhook", - }, - { - name: "When webhook uses Service reference instead of URL it should preserve the webhook", - cpServices: []corev1.Service{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "etcd-client", - Namespace: hcpNamespace, - }, - }, - }, - guestObjects: []client.Object{ - &admissionregistrationv1.ValidatingWebhookConfiguration{ - ObjectMeta: metav1.ObjectMeta{Name: "service-ref-webhook"}, - Webhooks: []admissionregistrationv1.ValidatingWebhook{ - { - Name: "service.webhook.io", - ClientConfig: admissionregistrationv1.WebhookClientConfig{ - Service: &admissionregistrationv1.ServiceReference{ - Name: "my-webhook-service", - Namespace: "default", - }, - }, - }, - }, - }, - }, - expectWebhookAlive: "service-ref-webhook", - }, - { - name: "When validating webhook has mixed allowed and disallowed URLs it should delete the entire configuration", - cpServices: []corev1.Service{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "etcd-client", - Namespace: hcpNamespace, - }, - }, - }, - guestObjects: []client.Object{ - &admissionregistrationv1.ValidatingWebhookConfiguration{ - ObjectMeta: metav1.ObjectMeta{Name: "mixed-validating-webhook"}, - Webhooks: []admissionregistrationv1.ValidatingWebhook{ - { - Name: "allowed.webhook.io", - ClientConfig: admissionregistrationv1.WebhookClientConfig{URL: ptr.To("https://external.example.com")}, - }, - { - Name: "disallowed.webhook.io", - ClientConfig: admissionregistrationv1.WebhookClientConfig{URL: ptr.To("https://etcd-client:2379")}, - }, - }, - }, - }, - expectWebhookGone: "mixed-validating-webhook", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - g := NewWithT(t) - ctx := t.Context() - - cpObjects := make([]client.Object, 0, len(tt.cpServices)) - for i := range tt.cpServices { - cpObjects = append(cpObjects, &tt.cpServices[i]) - } - - cpClient := fake.NewClientBuilder().WithScheme(api.Scheme).WithObjects(cpObjects...).Build() - guestClient := fake.NewClientBuilder().WithScheme(api.Scheme).WithObjects(tt.guestObjects...).Build() - - r := &reconciler{ - client: guestClient, - uncachedClient: fake.NewClientBuilder().WithScheme(api.Scheme).Build(), - cpClient: cpClient, - hcpNamespace: hcpNamespace, - CreateOrUpdateProvider: &simpleCreateOrUpdater{}, - } - - err := r.ensureGuestAdmissionWebhooksAreValid(ctx) - g.Expect(err).ToNot(HaveOccurred()) - - if tt.expectWebhookGone != "" { - for _, obj := range tt.guestObjects { - key := client.ObjectKey{Name: tt.expectWebhookGone} - switch obj.(type) { - case *admissionregistrationv1.ValidatingWebhookConfiguration: - err := guestClient.Get(ctx, key, &admissionregistrationv1.ValidatingWebhookConfiguration{}) - g.Expect(apierrors.IsNotFound(err)).To(BeTrue(), - "ValidatingWebhookConfiguration %q should have been deleted", tt.expectWebhookGone) - case *admissionregistrationv1.MutatingWebhookConfiguration: - err := guestClient.Get(ctx, key, &admissionregistrationv1.MutatingWebhookConfiguration{}) - g.Expect(apierrors.IsNotFound(err)).To(BeTrue(), - "MutatingWebhookConfiguration %q should have been deleted", tt.expectWebhookGone) - default: - t.Fatalf("unexpected object type %T in guestObjects for expectWebhookGone check", obj) - } - } - } - - if tt.expectWebhookAlive != "" { - for _, obj := range tt.guestObjects { - key := client.ObjectKey{Name: tt.expectWebhookAlive} - switch obj.(type) { - case *admissionregistrationv1.ValidatingWebhookConfiguration: - g.Expect(guestClient.Get(ctx, key, &admissionregistrationv1.ValidatingWebhookConfiguration{})).To(Succeed(), - "ValidatingWebhookConfiguration %q should still exist", tt.expectWebhookAlive) - case *admissionregistrationv1.MutatingWebhookConfiguration: - g.Expect(guestClient.Get(ctx, key, &admissionregistrationv1.MutatingWebhookConfiguration{})).To(Succeed(), - "MutatingWebhookConfiguration %q should still exist", tt.expectWebhookAlive) - default: - t.Fatalf("unexpected object type %T in guestObjects for expectWebhookAlive check", obj) - } - } - } - }) - } -} - func TestIsServiceAccountPullSecretsControllerDisabled(t *testing.T) { t.Parallel() tests := []struct { diff --git a/control-plane-operator/hostedclusterconfigoperator/controllers/webhookvalidation/setup.go b/control-plane-operator/hostedclusterconfigoperator/controllers/webhookvalidation/setup.go new file mode 100644 index 000000000000..2c8d9fec1a71 --- /dev/null +++ b/control-plane-operator/hostedclusterconfigoperator/controllers/webhookvalidation/setup.go @@ -0,0 +1,80 @@ +package webhookvalidation + +import ( + "context" + "fmt" + + "github.com/openshift/hypershift/control-plane-operator/hostedclusterconfigoperator/operator" + + admissionregistrationv1 "k8s.io/api/admissionregistration/v1" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/types" + + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller" + "sigs.k8s.io/controller-runtime/pkg/handler" + "sigs.k8s.io/controller-runtime/pkg/reconcile" + "sigs.k8s.io/controller-runtime/pkg/source" +) + +const ControllerName = "webhook-validation" + +// serviceEventKey is a sentinel name used when a CP Service event triggers +// re-evaluation of all webhook configs of a given type. Reconcile detects this +// and lists all configs instead of fetching a single one. +const serviceEventKey = "*" + +func Setup(ctx context.Context, opts *operator.HostedClusterConfigOperatorConfig) error { + r := &reconciler{ + client: opts.Manager.GetClient(), + cpClient: opts.CPCluster.GetClient(), + hcpNamespace: opts.Namespace, + } + + c, err := controller.New(ControllerName, opts.Manager, controller.Options{Reconciler: r}) + if err != nil { + return fmt.Errorf("failed to construct controller: %w", err) + } + + if err := c.Watch(source.Kind[client.Object](opts.Manager.GetCache(), &admissionregistrationv1.ValidatingWebhookConfiguration{}, typedHandler(validatingType))); err != nil { + return fmt.Errorf("failed to watch ValidatingWebhookConfigurations: %w", err) + } + + if err := c.Watch(source.Kind[client.Object](opts.Manager.GetCache(), &admissionregistrationv1.MutatingWebhookConfiguration{}, typedHandler(mutatingType))); err != nil { + return fmt.Errorf("failed to watch MutatingWebhookConfigurations: %w", err) + } + + // Watch CP Services so that when the disallowed URL list changes (Service created/deleted/relabeled), + // all existing webhook configs are re-evaluated immediately rather than waiting for cache resync. + // Service events enqueue sentinel requests; Reconcile lists the configs so list errors are retried. + if err := c.Watch(source.Kind[client.Object](opts.CPCluster.GetCache(), &corev1.Service{}, serviceEventHandler())); err != nil { + return fmt.Errorf("failed to watch control plane Services: %w", err) + } + + return nil +} + +// Webhook configs are cluster-scoped so Namespace is normally empty; we repurpose it +// to carry the webhook kind ("validating"/"mutating") so Reconcile targets only the type that fired. +func typedHandler(wt webhookType) handler.EventHandler { + return handler.EnqueueRequestsFromMapFunc(func(_ context.Context, obj client.Object) []reconcile.Request { + return []reconcile.Request{{ + NamespacedName: types.NamespacedName{ + Namespace: wt.name, + Name: obj.GetName(), + }, + }} + }) +} + +// serviceEventHandler enqueues one sentinel request per webhook type when a CP +// Service changes. The listing of webhook configs happens inside Reconcile so +// that list errors are surfaced and retried by the controller work queue. +func serviceEventHandler() handler.EventHandler { + return handler.EnqueueRequestsFromMapFunc(func(_ context.Context, _ client.Object) []reconcile.Request { + return []reconcile.Request{ + {NamespacedName: types.NamespacedName{Namespace: validatingType.name, Name: serviceEventKey}}, + {NamespacedName: types.NamespacedName{Namespace: mutatingType.name, Name: serviceEventKey}}, + } + }) +} diff --git a/control-plane-operator/hostedclusterconfigoperator/controllers/webhookvalidation/webhookvalidation.go b/control-plane-operator/hostedclusterconfigoperator/controllers/webhookvalidation/webhookvalidation.go new file mode 100644 index 000000000000..50313bfa2a9e --- /dev/null +++ b/control-plane-operator/hostedclusterconfigoperator/controllers/webhookvalidation/webhookvalidation.go @@ -0,0 +1,168 @@ +package webhookvalidation + +import ( + "context" + "errors" + "fmt" + "strings" + + hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1" + + admissionregistrationv1 "k8s.io/api/admissionregistration/v1" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/reconcile" +) + +// webhookType encodes which kind of admission webhook triggered reconciliation. +// It carries the type-specific logic (object construction, URL extraction) so +// callers never need to switch on the kind. +type webhookType struct { + name string + newObj func() client.Object + newList func() client.ObjectList + getNames func(client.ObjectList) []string + getURLs func(client.Object) []*string +} + +var ( + validatingType = webhookType{ + name: "validating", + newObj: func() client.Object { return &admissionregistrationv1.ValidatingWebhookConfiguration{} }, + newList: func() client.ObjectList { return &admissionregistrationv1.ValidatingWebhookConfigurationList{} }, + getNames: func(list client.ObjectList) []string { + items := list.(*admissionregistrationv1.ValidatingWebhookConfigurationList).Items + names := make([]string, len(items)) + for i := range items { + names[i] = items[i].Name + } + return names + }, + getURLs: func(obj client.Object) []*string { + wh := obj.(*admissionregistrationv1.ValidatingWebhookConfiguration) + urls := make([]*string, 0, len(wh.Webhooks)) + for i := range wh.Webhooks { + urls = append(urls, wh.Webhooks[i].ClientConfig.URL) + } + return urls + }, + } + mutatingType = webhookType{ + name: "mutating", + newObj: func() client.Object { return &admissionregistrationv1.MutatingWebhookConfiguration{} }, + newList: func() client.ObjectList { return &admissionregistrationv1.MutatingWebhookConfigurationList{} }, + getNames: func(list client.ObjectList) []string { + items := list.(*admissionregistrationv1.MutatingWebhookConfigurationList).Items + names := make([]string, len(items)) + for i := range items { + names[i] = items[i].Name + } + return names + }, + getURLs: func(obj client.Object) []*string { + wh := obj.(*admissionregistrationv1.MutatingWebhookConfiguration) + urls := make([]*string, 0, len(wh.Webhooks)) + for i := range wh.Webhooks { + urls = append(urls, wh.Webhooks[i].ClientConfig.URL) + } + return urls + }, + } + webhookTypesByName = map[string]webhookType{ + validatingType.name: validatingType, + mutatingType.name: mutatingType, + } +) + +type reconciler struct { + client client.Client + cpClient client.Reader + hcpNamespace string +} + +func (r *reconciler) Reconcile(ctx context.Context, req reconcile.Request) (ctrl.Result, error) { + wt, ok := webhookTypesByName[req.Namespace] + if !ok { + return ctrl.Result{}, nil + } + + disallowedURLs, err := r.buildDisallowedURLs(ctx) + if err != nil { + return ctrl.Result{}, fmt.Errorf("failed to build disallowed URLs: %w", err) + } + + // Sentinel request from a CP Service event: list all configs of this type + // so that list errors are surfaced and retried by the work queue. + if req.Name == serviceEventKey { + return ctrl.Result{}, r.reconcileAllWebhooks(ctx, disallowedURLs, wt) + } + + return ctrl.Result{}, r.reconcileWebhook(ctx, req.Name, disallowedURLs, wt) +} + +func (r *reconciler) reconcileAllWebhooks(ctx context.Context, disallowedURLs []string, wt webhookType) error { + list := wt.newList() + if err := r.client.List(ctx, list); err != nil { + return fmt.Errorf("failed to list %s webhooks: %w", wt.name, err) + } + var errs []error + for _, name := range wt.getNames(list) { + if err := r.reconcileWebhook(ctx, name, disallowedURLs, wt); err != nil { + errs = append(errs, err) + } + } + return errors.Join(errs...) +} + +func (r *reconciler) reconcileWebhook(ctx context.Context, name string, disallowedURLs []string, wt webhookType) error { + log := ctrl.LoggerFrom(ctx) + obj := wt.newObj() + if err := r.client.Get(ctx, client.ObjectKey{Name: name}, obj); err != nil { + if apierrors.IsNotFound(err) { + return nil + } + return fmt.Errorf("failed to get %s webhook: %w", wt.name, err) + } + + for _, url := range wt.getURLs(obj) { + if url != nil && !isAllowedWebhookURL(disallowedURLs, *url) { + log.Info("deleting webhook configuration with a disallowed url", "type", wt.name, "webhook_name", name, "disallowed_url", *url) + if err := r.client.Delete(ctx, obj); err != nil && !apierrors.IsNotFound(err) { + return fmt.Errorf("failed to delete %s webhook %s: %w", wt.name, name, err) + } + return nil + } + } + return nil +} + +func (r *reconciler) buildDisallowedURLs(ctx context.Context) ([]string, error) { + cpServices := &corev1.ServiceList{} + if err := r.cpClient.List(ctx, cpServices, client.InNamespace(r.hcpNamespace)); err != nil { + return nil, fmt.Errorf("failed to list control plane services: %w", err) + } + + var disallowedURLs []string + for _, svc := range cpServices.Items { + if _, exist := svc.Labels[hyperv1.AllowGuestWebhooksServiceLabel]; exist { + continue + } + disallowedURLs = append(disallowedURLs, fmt.Sprintf("https://%s", svc.Name)) + disallowedURLs = append(disallowedURLs, fmt.Sprintf("https://%s.%s.svc", svc.Name, svc.Namespace)) + disallowedURLs = append(disallowedURLs, fmt.Sprintf("https://%s.%s.svc.cluster.local", svc.Name, svc.Namespace)) + } + + return disallowedURLs, nil +} + +func isAllowedWebhookURL(disallowedURLs []string, url string) bool { + for i := range disallowedURLs { + if strings.Contains(url, disallowedURLs[i]) { + return false + } + } + return true +} diff --git a/control-plane-operator/hostedclusterconfigoperator/controllers/webhookvalidation/webhookvalidation_test.go b/control-plane-operator/hostedclusterconfigoperator/controllers/webhookvalidation/webhookvalidation_test.go new file mode 100644 index 000000000000..b22538d30619 --- /dev/null +++ b/control-plane-operator/hostedclusterconfigoperator/controllers/webhookvalidation/webhookvalidation_test.go @@ -0,0 +1,425 @@ +package webhookvalidation + +import ( + "context" + "fmt" + "testing" + + . "github.com/onsi/gomega" + + hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1" + + admissionregistrationv1 "k8s.io/api/admissionregistration/v1" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "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/client/interceptor" + "sigs.k8s.io/controller-runtime/pkg/reconcile" +) + +func testScheme(t *testing.T) *runtime.Scheme { + t.Helper() + s := runtime.NewScheme() + for _, add := range []func(*runtime.Scheme) error{ + corev1.AddToScheme, + admissionregistrationv1.AddToScheme, + } { + if err := add(s); err != nil { + t.Fatalf("failed to add to scheme: %v", err) + } + } + return s +} + +func TestIsAllowedWebhookURL(t *testing.T) { + t.Parallel() + tests := []struct { + name string + disallowedURLs []string + url string + expected bool + }{ + { + name: "When URL contains a disallowed substring, it should return false", + disallowedURLs: []string{"https://etcd-client"}, + url: "https://etcd-client:2379", + expected: false, + }, + { + name: "When URL matches a fully qualified disallowed URL, it should return false", + disallowedURLs: []string{"https://etcd-client.ns.svc"}, + url: "https://etcd-client.ns.svc:2379/path", + expected: false, + }, + { + name: "When URL does not match any disallowed URL, it should return true", + disallowedURLs: []string{"https://etcd-client"}, + url: "https://external.example.com", + expected: true, + }, + { + name: "When disallowed list is empty, it should return true", + disallowedURLs: []string{}, + url: "https://anything", + expected: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + result := isAllowedWebhookURL(tt.disallowedURLs, tt.url) + g.Expect(result).To(Equal(tt.expected)) + }) + } +} + +func TestReconcile(t *testing.T) { + t.Parallel() + const hcpNamespace = "test-hcp-namespace" + + tests := []struct { + name string + webhookType string + cpServices []corev1.Service + guestObjects []client.Object + reconcileName string + cpInterceptor interceptor.Funcs + guestInterceptor interceptor.Funcs + expectWebhookGone bool + expectWebhookAlive bool + expectError string + }{ + { + name: "When validating webhook targets a CP service, it should delete the webhook", + webhookType: validatingType.name, + cpServices: []corev1.Service{ + {ObjectMeta: metav1.ObjectMeta{Name: "etcd-client", Namespace: hcpNamespace}}, + }, + guestObjects: []client.Object{ + &admissionregistrationv1.ValidatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "test-validating-webhook"}, + Webhooks: []admissionregistrationv1.ValidatingWebhook{ + {Name: "test.webhook.io", ClientConfig: admissionregistrationv1.WebhookClientConfig{URL: ptr.To("https://etcd-client:2379")}}, + }, + }, + }, + reconcileName: "test-validating-webhook", + expectWebhookGone: true, + }, + { + name: "When validating webhook targets an allowed CP service, it should preserve the webhook", + webhookType: validatingType.name, + cpServices: []corev1.Service{ + {ObjectMeta: metav1.ObjectMeta{Name: "allowed-service", Namespace: hcpNamespace, Labels: map[string]string{hyperv1.AllowGuestWebhooksServiceLabel: "true"}}}, + }, + guestObjects: []client.Object{ + &admissionregistrationv1.ValidatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "preserved-validating-webhook"}, + Webhooks: []admissionregistrationv1.ValidatingWebhook{ + {Name: "preserved.webhook.io", ClientConfig: admissionregistrationv1.WebhookClientConfig{URL: ptr.To("https://allowed-service:8443")}}, + }, + }, + }, + reconcileName: "preserved-validating-webhook", + expectWebhookAlive: true, + }, + { + name: "When mutating webhook targets a CP service, it should delete the webhook", + webhookType: mutatingType.name, + cpServices: []corev1.Service{ + {ObjectMeta: metav1.ObjectMeta{Name: "kube-apiserver", Namespace: hcpNamespace}}, + }, + guestObjects: []client.Object{ + &admissionregistrationv1.MutatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "test-mutating-webhook"}, + Webhooks: []admissionregistrationv1.MutatingWebhook{ + {Name: "mutating.webhook.io", ClientConfig: admissionregistrationv1.WebhookClientConfig{URL: ptr.To("https://kube-apiserver:6443")}}, + }, + }, + }, + reconcileName: "test-mutating-webhook", + expectWebhookGone: true, + }, + { + name: "When webhook targets an external URL, it should preserve the webhook", + webhookType: validatingType.name, + cpServices: []corev1.Service{ + {ObjectMeta: metav1.ObjectMeta{Name: "etcd-client", Namespace: hcpNamespace}}, + }, + guestObjects: []client.Object{ + &admissionregistrationv1.ValidatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "external-validating-webhook"}, + Webhooks: []admissionregistrationv1.ValidatingWebhook{ + {Name: "external.webhook.io", ClientConfig: admissionregistrationv1.WebhookClientConfig{URL: ptr.To("https://external.example.com")}}, + }, + }, + }, + reconcileName: "external-validating-webhook", + expectWebhookAlive: true, + }, + { + name: "When webhook uses Service reference instead of URL, it should preserve the webhook", + webhookType: validatingType.name, + cpServices: []corev1.Service{ + {ObjectMeta: metav1.ObjectMeta{Name: "etcd-client", Namespace: hcpNamespace}}, + }, + guestObjects: []client.Object{ + &admissionregistrationv1.ValidatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "service-ref-webhook"}, + Webhooks: []admissionregistrationv1.ValidatingWebhook{ + {Name: "service.webhook.io", ClientConfig: admissionregistrationv1.WebhookClientConfig{ + Service: &admissionregistrationv1.ServiceReference{Name: "my-webhook-service", Namespace: "default"}, + }}, + }, + }, + }, + reconcileName: "service-ref-webhook", + expectWebhookAlive: true, + }, + { + name: "When validating webhook has mixed allowed and disallowed URLs, it should delete the entire configuration", + webhookType: validatingType.name, + cpServices: []corev1.Service{ + {ObjectMeta: metav1.ObjectMeta{Name: "etcd-client", Namespace: hcpNamespace}}, + }, + guestObjects: []client.Object{ + &admissionregistrationv1.ValidatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "mixed-validating-webhook"}, + Webhooks: []admissionregistrationv1.ValidatingWebhook{ + {Name: "allowed.webhook.io", ClientConfig: admissionregistrationv1.WebhookClientConfig{URL: ptr.To("https://external.example.com")}}, + {Name: "disallowed.webhook.io", ClientConfig: admissionregistrationv1.WebhookClientConfig{URL: ptr.To("https://etcd-client:2379")}}, + }, + }, + }, + reconcileName: "mixed-validating-webhook", + expectWebhookGone: true, + }, + { + name: "When webhook config does not exist, it should return without error", + webhookType: validatingType.name, + cpServices: []corev1.Service{ + {ObjectMeta: metav1.ObjectMeta{Name: "etcd-client", Namespace: hcpNamespace}}, + }, + guestObjects: []client.Object{}, + reconcileName: "nonexistent-webhook", + }, + { + name: "When CP client list fails, it should return error", + webhookType: validatingType.name, + cpInterceptor: interceptor.Funcs{ + List: func(_ context.Context, _ client.WithWatch, _ client.ObjectList, _ ...client.ListOption) error { + return fmt.Errorf("simulated cp list error") + }, + }, + reconcileName: "any-webhook", + expectError: "simulated cp list error", + }, + { + name: "When guest client get fails with non-404 error, it should return error", + webhookType: validatingType.name, + guestInterceptor: interceptor.Funcs{ + Get: func(_ context.Context, _ client.WithWatch, _ client.ObjectKey, _ client.Object, _ ...client.GetOption) error { + return fmt.Errorf("simulated guest get error") + }, + }, + reconcileName: "any-webhook", + expectError: "simulated guest get error", + }, + { + name: "When guest client delete fails, it should return error", + webhookType: validatingType.name, + cpServices: []corev1.Service{ + {ObjectMeta: metav1.ObjectMeta{Name: "etcd-client", Namespace: hcpNamespace}}, + }, + guestObjects: []client.Object{ + &admissionregistrationv1.ValidatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "bad-webhook"}, + Webhooks: []admissionregistrationv1.ValidatingWebhook{ + {Name: "bad.webhook.io", ClientConfig: admissionregistrationv1.WebhookClientConfig{URL: ptr.To("https://etcd-client:2379")}}, + }, + }, + }, + guestInterceptor: interceptor.Funcs{ + Delete: func(_ context.Context, _ client.WithWatch, _ client.Object, _ ...client.DeleteOption) error { + return fmt.Errorf("simulated delete error") + }, + }, + reconcileName: "bad-webhook", + expectError: "simulated delete error", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + ctx := t.Context() + scheme := testScheme(t) + + cpObjects := make([]client.Object, 0, len(tt.cpServices)) + for i := range tt.cpServices { + cpObjects = append(cpObjects, &tt.cpServices[i]) + } + + cpClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(cpObjects...).WithInterceptorFuncs(tt.cpInterceptor).Build() + guestClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(tt.guestObjects...).WithInterceptorFuncs(tt.guestInterceptor).Build() + + r := &reconciler{ + client: guestClient, + cpClient: cpClient, + hcpNamespace: hcpNamespace, + } + + result, err := r.Reconcile(ctx, reconcile.Request{ + NamespacedName: client.ObjectKey{ + Namespace: tt.webhookType, + Name: tt.reconcileName, + }, + }) + + if tt.expectError != "" { + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring(tt.expectError)) + return + } + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(result).To(Equal(reconcile.Result{})) + + if tt.expectWebhookGone { + assertWebhookNotFound(g, ctx, guestClient, tt.webhookType, tt.reconcileName) + } + if tt.expectWebhookAlive { + assertWebhookExists(g, ctx, guestClient, tt.webhookType, tt.reconcileName) + } + }) + } +} + +func TestReconcileAllWebhooks(t *testing.T) { + t.Parallel() + const hcpNamespace = "test-hcp-namespace" + + tests := []struct { + name string + webhookType string + cpServices []corev1.Service + guestObjects []client.Object + guestInterceptor interceptor.Funcs + expectDeletedName string + expectKeptName string + expectError string + }{ + { + name: "When sentinel fires for validating type, it should list and delete disallowed webhooks", + webhookType: validatingType.name, + cpServices: []corev1.Service{ + {ObjectMeta: metav1.ObjectMeta{Name: "etcd-client", Namespace: hcpNamespace}}, + }, + guestObjects: []client.Object{ + &admissionregistrationv1.ValidatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "bad-webhook"}, + Webhooks: []admissionregistrationv1.ValidatingWebhook{ + {Name: "bad.webhook.io", ClientConfig: admissionregistrationv1.WebhookClientConfig{URL: ptr.To("https://etcd-client:2379")}}, + }, + }, + &admissionregistrationv1.ValidatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "good-webhook"}, + Webhooks: []admissionregistrationv1.ValidatingWebhook{ + {Name: "good.webhook.io", ClientConfig: admissionregistrationv1.WebhookClientConfig{URL: ptr.To("https://external.example.com")}}, + }, + }, + }, + expectDeletedName: "bad-webhook", + expectKeptName: "good-webhook", + }, + { + name: "When sentinel fires for mutating type, it should list and delete disallowed webhooks", + webhookType: mutatingType.name, + cpServices: []corev1.Service{ + {ObjectMeta: metav1.ObjectMeta{Name: "kube-apiserver", Namespace: hcpNamespace}}, + }, + guestObjects: []client.Object{ + &admissionregistrationv1.MutatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "bad-mutating"}, + Webhooks: []admissionregistrationv1.MutatingWebhook{ + {Name: "bad.webhook.io", ClientConfig: admissionregistrationv1.WebhookClientConfig{URL: ptr.To("https://kube-apiserver:6443")}}, + }, + }, + }, + expectDeletedName: "bad-mutating", + }, + { + name: "When guest client list fails, it should return error for retry", + webhookType: validatingType.name, + guestInterceptor: interceptor.Funcs{ + List: func(_ context.Context, _ client.WithWatch, _ client.ObjectList, _ ...client.ListOption) error { + return fmt.Errorf("simulated guest list error") + }, + }, + expectError: "simulated guest list error", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + ctx := t.Context() + scheme := testScheme(t) + + cpObjects := make([]client.Object, 0, len(tt.cpServices)) + for i := range tt.cpServices { + cpObjects = append(cpObjects, &tt.cpServices[i]) + } + + cpClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(cpObjects...).Build() + guestClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(tt.guestObjects...).WithInterceptorFuncs(tt.guestInterceptor).Build() + + r := &reconciler{ + client: guestClient, + cpClient: cpClient, + hcpNamespace: hcpNamespace, + } + + result, err := r.Reconcile(ctx, reconcile.Request{ + NamespacedName: client.ObjectKey{ + Namespace: tt.webhookType, + Name: serviceEventKey, + }, + }) + + if tt.expectError != "" { + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring(tt.expectError)) + return + } + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(result).To(Equal(reconcile.Result{})) + + if tt.expectDeletedName != "" { + assertWebhookNotFound(g, ctx, guestClient, tt.webhookType, tt.expectDeletedName) + } + if tt.expectKeptName != "" { + assertWebhookExists(g, ctx, guestClient, tt.webhookType, tt.expectKeptName) + } + }) + } +} + +func assertWebhookNotFound(g Gomega, ctx context.Context, c client.Client, wtName, name string) { + wt := webhookTypesByName[wtName] + err := c.Get(ctx, client.ObjectKey{Name: name}, wt.newObj()) + g.Expect(apierrors.IsNotFound(err)).To(BeTrue(), "%s webhook %q should have been deleted", wtName, name) +} + +func assertWebhookExists(g Gomega, ctx context.Context, c client.Client, wtName, name string) { + wt := webhookTypesByName[wtName] + g.Expect(c.Get(ctx, client.ObjectKey{Name: name}, wt.newObj())).To(Succeed(), + "%s webhook %q should still exist", wtName, name) +} From c9cb6d2bec59381dd475382c819c645cb3adad44 Mon Sep 17 00:00:00 2001 From: Bryan Cox Date: Thu, 6 Aug 2026 07:34:18 -0400 Subject: [PATCH 2/2] test: increase WebhookValidation e2e timeout to 3 minutes Co-Authored-By: Claude Opus 4.6 --- test/e2e/v2/tests/hosted_cluster_security_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/e2e/v2/tests/hosted_cluster_security_test.go b/test/e2e/v2/tests/hosted_cluster_security_test.go index 907ae656eb4c..4c6a0da873be 100644 --- a/test/e2e/v2/tests/hosted_cluster_security_test.go +++ b/test/e2e/v2/tests/hosted_cluster_security_test.go @@ -95,7 +95,7 @@ func EnsureHostedClusterWebhooksValidatedTest(getTestCtx internal.TestContextGet existing := &admissionregistrationv1.ValidatingWebhookConfiguration{} err := hcClient.Get(tc.Context, crclient.ObjectKeyFromObject(webhookConf), existing) g.Expect(apierrors.IsNotFound(err)).To(BeTrue(), "webhook should have been deleted by HCCO") - }, time.Minute, 5*time.Second).Should(Succeed()) + }, 3*time.Minute, 5*time.Second).Should(Succeed()) }) }) }