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) +} 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()) }) }) }