webhook: sort on version if names are equal

Author: HaraldNordgren

pkg/webhook/parser.go

@@ -516,6 +516,9 @@ func (g Generator) Generate(ctx *genall.GenerationContext) error {
 	versionedWebhooks := make(map[string][]interface{}, len(supportedWebhookVersions))
 	for _, version := range supportedWebhookVersions {
 		if cfgs, ok := mutatingCfgs[version]; ok {
+			sort.SliceStable(cfgs, func(i, j int) bool {
+				return cfgs[i].Name < cfgs[j].Name
+			})
 			var objRaw *admissionregv1.MutatingWebhookConfiguration
 			if mutatingWebhookCfgs.Name != "" {
 				objRaw = &mutatingWebhookCfgs
@@ -553,6 +556,9 @@ func (g Generator) Generate(ctx *genall.GenerationContext) error {
 		}
 
 		if cfgs, ok := validatingCfgs[version]; ok {
+			sort.SliceStable(cfgs, func(i, j int) bool {
+				return cfgs[i].Name < cfgs[j].Name
+			})
 			var objRaw *admissionregv1.ValidatingWebhookConfiguration
 			if validatingWebhookCfgs.Name != "" {
 				objRaw = &validatingWebhookCfgs

pkg/webhook/parser_integration_test.go

@@ -426,6 +426,77 @@ var _ = Describe("Webhook Generation From Parsing to CustomResourceDefinition",
 		assertSame(actualValidating, expectedValidating)
 	})
 
+	It("should keep webhook order stable across package traversal orders", func() {
+		By("switching into testdata to appease go modules")
+		cwd, err := os.Getwd()
+		Expect(err).NotTo(HaveOccurred())
+		Expect(os.Chdir("./testdata/valid-crosspkg-stable")).To(Succeed()) // go modules are directory-sensitive
+		defer func() { Expect(os.Chdir(cwd)).To(Succeed()) }()
+
+		By("loading the roots in one order")
+		pkgsA, err := loader.LoadRoots("./v1", "./v1alpha1")
+		Expect(err).NotTo(HaveOccurred())
+		Expect(pkgsA).To(HaveLen(2))
+
+		By("setting up the parser")
+		reg := &markers.Registry{}
+		Expect(reg.Register(webhook.ConfigDefinition)).To(Succeed())
+		Expect(reg.Register(webhook.WebhookConfigDefinition)).To(Succeed())
+
+		By("generating manifests for order A")
+		outputDirA, err := os.MkdirTemp("", "webhook-integration-test-order-a")
+		Expect(err).NotTo(HaveOccurred())
+		defer os.RemoveAll(outputDirA)
+		genCtxA := &genall.GenerationContext{
+			Collector:  &markers.Collector{Registry: reg},
+			Roots:      pkgsA,
+			OutputRule: genall.OutputToDirectory(outputDirA),
+		}
+		Expect(webhook.Generator{}.Generate(genCtxA)).To(Succeed())
+		for _, r := range genCtxA.Roots {
+			Expect(r.Errors).To(HaveLen(0))
+		}
+
+		By("loading the generated v1 YAML for order A")
+		actualFileA, err := os.ReadFile(path.Join(outputDirA, "manifests.yaml"))
+		Expect(err).NotTo(HaveOccurred())
+		actualManifestA := &admissionregv1.ValidatingWebhookConfiguration{}
+		Expect(yaml.UnmarshalStrict(actualFileA, actualManifestA)).To(Succeed())
+
+		By("loading the roots in the reverse order")
+		pkgsB := []*loader.Package{pkgsA[1], pkgsA[0]}
+
+		By("generating manifests for order B")
+		outputDirB, err := os.MkdirTemp("", "webhook-integration-test-order-b")
+		Expect(err).NotTo(HaveOccurred())
+		defer os.RemoveAll(outputDirB)
+		genCtxB := &genall.GenerationContext{
+			Collector:  &markers.Collector{Registry: reg},
+			Roots:      pkgsB,
+			OutputRule: genall.OutputToDirectory(outputDirB),
+		}
+		Expect(webhook.Generator{}.Generate(genCtxB)).To(Succeed())
+		for _, r := range genCtxB.Roots {
+			Expect(r.Errors).To(HaveLen(0))
+		}
+
+		By("loading the generated v1 YAML for order B")
+		actualFileB, err := os.ReadFile(path.Join(outputDirB, "manifests.yaml"))
+		Expect(err).NotTo(HaveOccurred())
+		actualManifestB := &admissionregv1.ValidatingWebhookConfiguration{}
+		Expect(yaml.UnmarshalStrict(actualFileB, actualManifestB)).To(Succeed())
+
+		By("loading the desired v1 YAML")
+		expectedFile, err := os.ReadFile("manifests.yaml")
+		Expect(err).NotTo(HaveOccurred())
+		expectedManifest := &admissionregv1.ValidatingWebhookConfiguration{}
+		Expect(yaml.UnmarshalStrict(expectedFile, expectedManifest)).To(Succeed())
+
+		By("comparing manifests across orders and to expected")
+		assertSame(actualManifestA, actualManifestB)
+		assertSame(actualManifestA, expectedManifest)
+	})
+
 	It("should fail to generate when there are multiple `kubebuilder:webhookconfiguration` markers of the same mutation type", func() {
 		By("switching into testdata to appease go modules")
 		cwd, err := os.Getwd()

pkg/webhook/testdata/valid-crosspkg-stable/go.mod

@@ -0,0 +1,8 @@
+module sigs.k8s.io/controller-tools/pkg/webhook/testdata/valid-crosspkg-stable
+
+go 1.22
+
+require (
+	sigs.k8s.io/controller-runtime v0.18.5
+	k8s.io/api v0.34.1
+)

pkg/webhook/testdata/valid-crosspkg-stable/manifests.yaml

@@ -0,0 +1,50 @@
+---
+apiVersion: admissionregistration.k8s.io/v1
+kind: ValidatingWebhookConfiguration
+metadata:
+  name: validating-webhook-configuration
+webhooks:
+  - admissionReviewVersions:
+      - v1
+      - v1beta1
+    clientConfig:
+      service:
+        name: webhook-service
+        namespace: system
+        path: /validate-testdata-kubebuilder-io-v1alpha1-cronjob
+    failurePolicy: Fail
+    matchPolicy: Equivalent
+    name: a.cronjob.testdata.kubebuilder.io
+    rules:
+      - apiGroups:
+          - testdata.kubebuilder.io
+        apiVersions:
+          - v1alpha1
+        operations:
+          - CREATE
+          - UPDATE
+        resources:
+          - cronjobs
+    sideEffects: None
+  - admissionReviewVersions:
+      - v1
+      - v1beta1
+    clientConfig:
+      service:
+        name: webhook-service
+        namespace: system
+        path: /validate-testdata-kubebuilder-io-v1-cronjob
+    failurePolicy: Fail
+    matchPolicy: Equivalent
+    name: validation.cronjob.testdata.kubebuilder.io
+    rules:
+      - apiGroups:
+          - testdata.kubebuilder.io
+        apiVersions:
+          - v1
+        operations:
+          - CREATE
+          - UPDATE
+        resources:
+          - cronjobs
+    sideEffects: None

pkg/webhook/testdata/valid-crosspkg-stable/v1/cronjob_types.go

@@ -0,0 +1,3 @@
+package v1
+
+type CronJob struct{}

pkg/webhook/testdata/valid-crosspkg-stable/v1/webhook.go

@@ -0,0 +1,3 @@
+package v1
+
+// +kubebuilder:webhook:webhookVersions=v1,verbs=create;update,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=testdata.kubebuilder.io,resources=cronjobs,versions=v1,name=validation.cronjob.testdata.kubebuilder.io,sideEffects=None,admissionReviewVersions=v1;v1beta1,path=/validate-testdata-kubebuilder-io-v1-cronjob

pkg/webhook/testdata/valid-crosspkg-stable/v1alpha1/cronjob_types.go

@@ -0,0 +1,3 @@
+package v1alpha1
+
+type CronJob struct{}

pkg/webhook/testdata/valid-crosspkg-stable/v1alpha1/webhook.go

@@ -0,0 +1,3 @@
+package v1alpha1
+
+// +kubebuilder:webhook:webhookVersions=v1,verbs=create;update,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=testdata.kubebuilder.io,resources=cronjobs,versions=v1alpha1,name=a.cronjob.testdata.kubebuilder.io,sideEffects=None,admissionReviewVersions=v1;v1beta1,path=/validate-testdata-kubebuilder-io-v1alpha1-cronjob