Add integration test for sharder webhook (#489)

* Prefactor: make `WebhookConfigForControllerRing` reusable

* Simplify existing integration tests

* Add integration test for `sharder` webhook

Author: timebertt

.golangci.yaml

@@ -75,3 +75,7 @@ issues:
   - linters:
     - nolintlint
     text: "should be written without leading space"
+
+  exclude:
+  # ginkgolinter false positive: https://github.com/nunnatsa/ginkgolinter/issues/190
+  - the MatchError matcher used to assert a non error type \(ctx\)

pkg/controller/controllerring/reconciler.go

@@ -20,7 +20,6 @@ import (
 	"context"
 	"fmt"
 	"maps"
-	"path"
 	"strings"
 
 	"github.com/go-logr/logr"
@@ -150,7 +149,16 @@ func (r *Reconciler) reconcileWebhooks(ctx context.Context, controllerRing *shar
 }
 
 func (r *Reconciler) WebhookConfigForControllerRing(controllerRing *shardingv1alpha1.ControllerRing) (*admissionregistrationv1.MutatingWebhookConfiguration, error) {
-	webhookConfig := &admissionregistrationv1.MutatingWebhookConfiguration{
+	webhookConfig := WebhookConfigForControllerRing(controllerRing, r.Config.Webhook.Config)
+	if err := controllerutil.SetControllerReference(controllerRing, webhookConfig, r.Client.Scheme()); err != nil {
+		return nil, fmt.Errorf("error setting controller reference: %w", err)
+	}
+
+	return webhookConfig, nil
+}
+
+func WebhookConfigForControllerRing(controllerRing *shardingv1alpha1.ControllerRing, config *configv1alpha1.WebhookConfig) *admissionregistrationv1.MutatingWebhookConfiguration {
+	return &admissionregistrationv1.MutatingWebhookConfiguration{
 		TypeMeta: metav1.TypeMeta{
 			APIVersion: admissionregistrationv1.SchemeGroupVersion.String(),
 			Kind:       "MutatingWebhookConfiguration",
@@ -161,22 +169,17 @@ func (r *Reconciler) WebhookConfigForControllerRing(controllerRing *shardingv1al
 				"app.kubernetes.io/name":             shardingv1alpha1.AppControllerSharding,
 				shardingv1alpha1.LabelControllerRing: controllerRing.Name,
 			},
-			Annotations: maps.Clone(r.Config.Webhook.Config.Annotations),
+			Annotations: maps.Clone(config.Annotations),
 		},
-		Webhooks: []admissionregistrationv1.MutatingWebhook{r.WebhookForControllerRing(controllerRing)},
-	}
-	if err := controllerutil.SetControllerReference(controllerRing, webhookConfig, r.Client.Scheme()); err != nil {
-		return nil, fmt.Errorf("error setting controller reference: %w", err)
+		Webhooks: []admissionregistrationv1.MutatingWebhook{WebhookForControllerRing(controllerRing, config)},
 	}
-
-	return webhookConfig, nil
 }
 
-func (r *Reconciler) WebhookForControllerRing(controllerRing *shardingv1alpha1.ControllerRing) admissionregistrationv1.MutatingWebhook {
+func WebhookForControllerRing(controllerRing *shardingv1alpha1.ControllerRing, config *configv1alpha1.WebhookConfig) admissionregistrationv1.MutatingWebhook {
 	webhook := admissionregistrationv1.MutatingWebhook{
 		Name:              "sharder.sharding.timebertt.dev",
-		ClientConfig:      *r.Config.Webhook.Config.ClientConfig.DeepCopy(),
-		NamespaceSelector: r.Config.Webhook.Config.NamespaceSelector.DeepCopy(),
+		ClientConfig:      *config.ClientConfig.DeepCopy(),
+		NamespaceSelector: config.NamespaceSelector.DeepCopy(),
 
 		// only process unassigned objects
 		ObjectSelector: &metav1.LabelSelector{
@@ -203,7 +206,7 @@ func (r *Reconciler) WebhookForControllerRing(controllerRing *shardingv1alpha1.C
 	webhookPath := sharder.WebhookPathForControllerRing(controllerRing)
 
 	if service := webhook.ClientConfig.Service; service != nil {
-		service.Path = ptr.To(path.Join(ptr.Deref(service.Path, ""), webhookPath))
+		service.Path = ptr.To(webhookPath)
 	}
 	if url := webhook.ClientConfig.URL; url != nil {
 		// We can't use path.Join on URLs because it will drop one slash from the scheme.

pkg/controller/controllerring/reconciler_test.go

@@ -180,23 +180,23 @@ var _ = Describe("Reconciler", func() {
 
 	Describe("#WebhookForControllerRing", func() {
 		It("should have the correct settings", func() {
-			Expect(r.WebhookForControllerRing(ring)).To(MatchFields(IgnoreExtras, Fields{
+			Expect(WebhookForControllerRing(ring, config.Webhook.Config)).To(MatchFields(IgnoreExtras, Fields{
 				"Name":                    Equal("sharder.sharding.timebertt.dev"),
 				"SideEffects":             PointTo(Equal(admissionregistrationv1.SideEffectClassNone)),
 				"AdmissionReviewVersions": ConsistOf("v1"),
 			}))
 		})
 
 		It("should have non-problematic failure settings", func() {
-			Expect(r.WebhookForControllerRing(ring)).To(MatchFields(IgnoreExtras, Fields{
+			Expect(WebhookForControllerRing(ring, config.Webhook.Config)).To(MatchFields(IgnoreExtras, Fields{
 				"FailurePolicy":  PointTo(Equal(admissionregistrationv1.Ignore)),
 				"TimeoutSeconds": PointTo(BeEquivalentTo(5)),
 			}))
 		})
 
 		Context("client config", func() {
 			It("should use the config's default client config and add the path", func() {
-				Expect(r.WebhookForControllerRing(ring).ClientConfig).To(Equal(admissionregistrationv1.WebhookClientConfig{
+				Expect(WebhookForControllerRing(ring, config.Webhook.Config).ClientConfig).To(Equal(admissionregistrationv1.WebhookClientConfig{
 					Service: &admissionregistrationv1.ServiceReference{
 						Namespace: "sharding-system",
 						Name:      "sharder",
@@ -215,7 +215,7 @@ var _ = Describe("Reconciler", func() {
 				}
 				clientConfig := config.Webhook.Config.ClientConfig.DeepCopy()
 
-				Expect(r.WebhookForControllerRing(ring).ClientConfig).To(Equal(admissionregistrationv1.WebhookClientConfig{
+				Expect(WebhookForControllerRing(ring, config.Webhook.Config).ClientConfig).To(Equal(admissionregistrationv1.WebhookClientConfig{
 					Service: &admissionregistrationv1.ServiceReference{
 						Namespace: clientConfig.Service.Namespace,
 						Name:      clientConfig.Service.Name,
@@ -230,7 +230,7 @@ var _ = Describe("Reconciler", func() {
 					URL: ptr.To("https://example.com/webhook"),
 				}
 
-				Expect(r.WebhookForControllerRing(ring).ClientConfig).To(Equal(admissionregistrationv1.WebhookClientConfig{
+				Expect(WebhookForControllerRing(ring, config.Webhook.Config).ClientConfig).To(Equal(admissionregistrationv1.WebhookClientConfig{
 					URL: ptr.To("https://example.com/webhook/webhooks/sharder/controllerring/foo"),
 				}))
 			})
@@ -240,15 +240,15 @@ var _ = Describe("Reconciler", func() {
 					URL: ptr.To("https://example.com/"),
 				}
 
-				Expect(r.WebhookForControllerRing(ring).ClientConfig).To(Equal(admissionregistrationv1.WebhookClientConfig{
+				Expect(WebhookForControllerRing(ring, config.Webhook.Config).ClientConfig).To(Equal(admissionregistrationv1.WebhookClientConfig{
 					URL: ptr.To("https://example.com/webhooks/sharder/controllerring/foo"),
 				}))
 			})
 		})
 
 		Context("namespace selector", func() {
 			It("should use the config's default namespace selector", func() {
-				Expect(r.WebhookForControllerRing(ring).NamespaceSelector).To(Equal(&metav1.LabelSelector{
+				Expect(WebhookForControllerRing(ring, config.Webhook.Config).NamespaceSelector).To(Equal(&metav1.LabelSelector{
 					MatchExpressions: []metav1.LabelSelectorRequirement{{
 						Key:      corev1.LabelMetadataName,
 						Operator: metav1.LabelSelectorOpNotIn,
@@ -263,7 +263,7 @@ var _ = Describe("Reconciler", func() {
 				}
 				namespaceSelector := config.Webhook.Config.NamespaceSelector.DeepCopy()
 
-				Expect(r.WebhookForControllerRing(ring).NamespaceSelector).To(Equal(namespaceSelector))
+				Expect(WebhookForControllerRing(ring, config.Webhook.Config).NamespaceSelector).To(Equal(namespaceSelector))
 			})
 
 			It("should use the ControllerRing's namespace selector", func() {
@@ -272,12 +272,12 @@ var _ = Describe("Reconciler", func() {
 				}
 				namespaceSelector := ring.Spec.NamespaceSelector.DeepCopy()
 
-				Expect(r.WebhookForControllerRing(ring).NamespaceSelector).To(Equal(namespaceSelector))
+				Expect(WebhookForControllerRing(ring, config.Webhook.Config).NamespaceSelector).To(Equal(namespaceSelector))
 			})
 		})
 
 		It("should only select unassigned objects", func() {
-			selector, err := metav1.LabelSelectorAsSelector(r.WebhookForControllerRing(ring).ObjectSelector)
+			selector, err := metav1.LabelSelectorAsSelector(WebhookForControllerRing(ring, config.Webhook.Config).ObjectSelector)
 			Expect(err).NotTo(HaveOccurred())
 
 			Expect(selector.Matches(labels.Set{})).To(BeTrue())
@@ -295,7 +295,7 @@ var _ = Describe("Reconciler", func() {
 				},
 			}
 
-			Expect(r.WebhookForControllerRing(ring).Rules).To(ConsistOf(
+			Expect(WebhookForControllerRing(ring, config.Webhook.Config).Rules).To(ConsistOf(
 				RuleForResource(ring.Spec.Resources[0].GroupResource),
 				RuleForResource(ring.Spec.Resources[1].GroupResource),
 				RuleForResource(ring.Spec.Resources[1].ControlledResources[0]),

pkg/sharding/key/key.go

@@ -117,8 +117,6 @@ func ForController(obj client.Object) (string, error) {
 		return "", fmt.Errorf("invalid apiVersion of controller reference: %w", err)
 	}
 
-	// Namespace can be empty for cluster-scoped resources. Only check the other fields as an optimistic check for
-	// preventing wrong usage of the function.
 	return forMetadata(gv.Group, ref.Kind, obj.GetNamespace(), ref.Name), nil
 }
 

pkg/utils/test/envtest.go

@@ -0,0 +1,27 @@
+/*
+Copyright 2025 Tim Ebert.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package test
+
+import (
+	"os"
+	"strings"
+)
+
+// UseExistingCluster reads the `USE_EXISTING_CLUSTER` env var similar to what envtest does, though exported.
+func UseExistingCluster() bool {
+	return strings.ToLower(os.Getenv("USE_EXISTING_CLUSTER")) == "true"
+}

pkg/webhook/sharder/handler.go

@@ -72,7 +72,7 @@ func (h *Handler) Handle(ctx context.Context, req admission.Request) admission.R
 		Resource: req.Resource.Resource,
 	}, controllerRing)
 	if err != nil {
-		return admission.Errored(http.StatusBadRequest, fmt.Errorf("error deteriming hash key func for object: %w", err))
+		return admission.Errored(http.StatusBadRequest, fmt.Errorf("error determining hash key func for object: %w", err))
 	}
 
 	hashKey, err := keyFunc(obj)

test/integration/sharder/controller/controllerring/controllerring_test.go

@@ -51,7 +51,6 @@ var _ = Describe("ControllerRing controller", func() {
 						GroupResource: metav1.GroupResource{Group: "apps", Resource: "deployments"},
 					},
 				},
-				NamespaceSelector: nil,
 			},
 		}
 

test/integration/sharder/controller/shardlease/shardlease_suite_test.go

@@ -37,7 +37,6 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/manager"
 	metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server"
 
-	configv1alpha1 "github.com/timebertt/kubernetes-controller-sharding/pkg/apis/config/v1alpha1"
 	shardingv1alpha1 "github.com/timebertt/kubernetes-controller-sharding/pkg/apis/sharding/v1alpha1"
 	"github.com/timebertt/kubernetes-controller-sharding/pkg/controller/shardlease"
 	utilclient "github.com/timebertt/kubernetes-controller-sharding/pkg/utils/client"
@@ -127,9 +126,6 @@ var _ = BeforeSuite(func(ctx SpecContext) {
 	Expect(err).NotTo(HaveOccurred())
 
 	By("Register controller")
-	config := &configv1alpha1.SharderConfig{}
-	mgr.GetScheme().Default(config)
-
 	clock = testclock.NewFakeClock(time.Now())
 
 	Expect((&shardlease.Reconciler{

test/integration/sharder/controller/shardlease/shardlease_test.go

@@ -51,7 +51,6 @@ var _ = Describe("Shard Lease controller", func() {
 						GroupResource: metav1.GroupResource{Group: "apps", Resource: "deployments"},
 					},
 				},
-				NamespaceSelector: nil,
 			},
 		}
 
@@ -81,10 +80,6 @@ var _ = Describe("Shard Lease controller", func() {
 
 		Expect(testClient.Create(ctx, lease)).To(Succeed())
 		log.Info("Created Lease for test", "leaseName", lease.Name)
-
-		DeferCleanup(func(ctx SpecContext) {
-			Expect(testClient.Delete(ctx, lease)).To(Or(Succeed(), BeNotFoundError()))
-		}, NodeTimeout(time.Minute))
 	}, NodeTimeout(time.Minute), OncePerOrdered)
 
 	Describe("should reflect the shard state in the label", Ordered, func() {