Repository: kubernetes-sigs/controller-tools
Pull request #779 (2023-02-20 08:51:53)

🐛 Generate deterministic ordering for webhook manifests
### What
This PR modified to generate webhook manifests sorted by name and added tests.

### Why
Now we get differently ordered manifests every time.
It isn't easy to compare generated manifests.


Please see https://github.com/kubernetes-sigs/controller-tools/issues/730 for more information.


Patch:
diff --git a/pkg/webhook/parser.go b/pkg/webhook/parser.go
index a4aeb5f5..3a2abb87 100644
--- a/pkg/webhook/parser.go
+++ b/pkg/webhook/parser.go
@@ -24,6 +24,7 @@ package webhook
 
 import (
 	"fmt"
+	"sort"
 	"strings"
 
 	admissionregv1 "k8s.io/api/admissionregistration/v1"
@@ -330,7 +331,12 @@ func (g Generator) Generate(ctx *genall.GenerationContext) error {
 			root.AddError(err)
 		}
 
-		for _, cfg := range markerSet[ConfigDefinition.Name] {
+		cfgs := markerSet[ConfigDefinition.Name]
+		sort.SliceStable(cfgs, func(i, j int) bool {
+			return cfgs[i].(Config).Name < cfgs[j].(Config).Name
+		})
+
+		for _, cfg := range cfgs {
 			cfg := cfg.(Config)
 			webhookVersions, err := cfg.webhookVersions()
 			if err != nil {


Test patch:
diff --git a/pkg/webhook/parser_integration_test.go b/pkg/webhook/parser_integration_test.go
index f2f6c438..9c683f2a 100644
--- a/pkg/webhook/parser_integration_test.go
+++ b/pkg/webhook/parser_integration_test.go
@@ -175,6 +175,55 @@ var _ = Describe("Webhook Generation From Parsing to CustomResourceDefinition",
 		assertSame(actualMutating, expectedMutating)
 		assertSame(actualValidating, expectedValidating)
 	})
+
+	It("should generate the ordered webhook definitions", func() {
+		By("switching into testdata to appease go modules")
+		cwd, err := os.Getwd()
+		Expect(err).NotTo(HaveOccurred())
+		Expect(os.Chdir("./testdata/valid-ordered")).To(Succeed()) // go modules are directory-sensitive
+		defer func() { Expect(os.Chdir(cwd)).To(Succeed()) }()
+
+		By("loading the roots")
+		pkgs, err := loader.LoadRoots(".")
+		Expect(err).NotTo(HaveOccurred())
+		Expect(pkgs).To(HaveLen(1))
+
+		By("setting up the parser")
+		reg := &markers.Registry{}
+		Expect(reg.Register(webhook.ConfigDefinition)).To(Succeed())
+
+		By("requesting that the manifest be generated")
+		outputDir, err := ioutil.TempDir("", "webhook-integration-test")
+		Expect(err).NotTo(HaveOccurred())
+		defer os.RemoveAll(outputDir)
+
+		for i := 0; i < 10; i++ {
+			genCtx := &genall.GenerationContext{
+				Collector:  &markers.Collector{Registry: reg},
+				Roots:      pkgs,
+				OutputRule: genall.OutputToDirectory(outputDir),
+			}
+			Expect(webhook.Generator{}.Generate(genCtx)).To(Succeed())
+			for _, r := range genCtx.Roots {
+				Expect(r.Errors).To(HaveLen(0))
+			}
+
+			By("loading the generated v1 YAML")
+			actualFile, err := ioutil.ReadFile(path.Join(outputDir, "manifests.yaml"))
+			Expect(err).NotTo(HaveOccurred())
+			actualManifest := &admissionregv1.ValidatingWebhookConfiguration{}
+			Expect(yaml.UnmarshalStrict(actualFile, actualManifest)).To(Succeed())
+
+			By("loading the desired v1 YAML")
+			expectedFile, err := ioutil.ReadFile("manifests.yaml")
+			Expect(err).NotTo(HaveOccurred())
+			expectedManifest := &admissionregv1.ValidatingWebhookConfiguration{}
+			Expect(yaml.UnmarshalStrict(expectedFile, expectedManifest)).To(Succeed())
+
+			By("comparing the manifest")
+			assertSame(actualManifest, expectedManifest)
+		}
+	})
 })
 
 func unmarshalBothV1(in []byte) (mutating admissionregv1.MutatingWebhookConfiguration, validating admissionregv1.ValidatingWebhookConfiguration) {
diff --git a/pkg/webhook/testdata/valid-ordered/cronjob_types.go b/pkg/webhook/testdata/valid-ordered/cronjob_types.go
new file mode 100644
index 00000000..9144b496
--- /dev/null
+++ b/pkg/webhook/testdata/valid-ordered/cronjob_types.go
@@ -0,0 +1,71 @@
+/*
+
+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.
+*/
+
+//go:generate ../../../.run-controller-gen.sh webhook paths=. output:dir=.
+
+// +groupName=testdata.kubebuilder.io
+// +versionName=v1
+package cronjob
+
+import (
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+// EDIT THIS FILE!  THIS IS SCAFFOLDING FOR YOU TO OWN!
+// NOTE: json tags are required.  Any new fields you add must have json tags for the fields to be serialized.
+
+// CronJobSpec defines the desired state of CronJob
+type CronJobSpec struct {
+	// The schedule in Cron format, see https://en.wikipedia.org/wiki/Cron.
+	Schedule string `json:"schedule"`
+}
+
+// CronJobStatus defines the observed state of CronJob
+type CronJobStatus struct {
+	// INSERT ADDITIONAL STATUS FIELD - define observed state of cluster
+	// Important: Run "make" to regenerate code after modifying this file
+
+	// Information when was the last time the job was successfully scheduled.
+	// +optional
+	LastScheduleTime *metav1.Time `json:"lastScheduleTime,omitempty"`
+}
+
+// +kubebuilder:object:root=true
+// +kubebuilder:subresource:status
+// +kubebuilder:resource:singular=mycronjob
+
+// CronJob is the Schema for the cronjobs API
+type CronJob struct {
+	/*
+	 */
+	metav1.TypeMeta   `json:",inline"`
+	metav1.ObjectMeta `json:"metadata,omitempty"`
+
+	Spec   CronJobSpec   `json:"spec,omitempty"`
+	Status CronJobStatus `json:"status,omitempty"`
+}
+
+// +kubebuilder:object:root=true
+
+// CronJobList contains a list of CronJob
+type CronJobList struct {
+	metav1.TypeMeta `json:",inline"`
+	metav1.ListMeta `json:"metadata,omitempty"`
+	Items           []CronJob `json:"items"`
+}
+
+func init() {
+	SchemeBuilder.Register(&CronJob{}, &CronJobList{})
+}
diff --git a/pkg/webhook/testdata/valid-ordered/manifests.yaml b/pkg/webhook/testdata/valid-ordered/manifests.yaml
new file mode 100644
index 00000000..832f1fa0
--- /dev/null
+++ b/pkg/webhook/testdata/valid-ordered/manifests.yaml
@@ -0,0 +1,73 @@
+---
+apiVersion: admissionregistration.k8s.io/v1
+kind: ValidatingWebhookConfiguration
+metadata:
+  creationTimestamp: null
+  name: validating-webhook-configuration
+webhooks:
+- admissionReviewVersions:
+  - v1
+  - v1beta1
+  clientConfig:
+    service:
+      name: webhook-service
+      namespace: system
+      path: /validate-testdata-kubebuilder-io-v1-cronjob
+  failurePolicy: Fail
+  matchPolicy: Equivalent
+  name: cronjob.testdata.kubebuilder.io
+  rules:
+  - apiGroups:
+    - testdata.kubebuiler.io
+    apiVersions:
+    - v1
+    operations:
+    - CREATE
+    - UPDATE
+    resources:
+    - cronjobs
+  sideEffects: None
+- admissionReviewVersions:
+  - v1
+  - v1beta1
+  clientConfig:
+    service:
+      name: webhook-service
+      namespace: system
+      path: /validate-testdata-kubebuilder-io-v1-cronjoblist
+  failurePolicy: Fail
+  matchPolicy: Equivalent
+  name: cronjoblist.testdata.kubebuilder.io
+  rules:
+  - apiGroups:
+    - testdata.kubebuiler.io
+    apiVersions:
+    - v1
+    operations:
+    - CREATE
+    - UPDATE
+    resources:
+    - cronjoblist
+  sideEffects: None
+- admissionReviewVersions:
+  - v1
+  - v1beta1
+  clientConfig:
+    service:
+      name: webhook-service
+      namespace: system
+      path: /validate-testdata-kubebuilder-io-v1-deployments
+  failurePolicy: Fail
+  matchPolicy: Equivalent
+  name: deployment.testdata.kubebuilder.io
+  rules:
+  - apiGroups:
+    - testdata.kubebuiler.io
+    apiVersions:
+    - v1
+    operations:
+    - CREATE
+    - UPDATE
+    resources:
+    - deployments
+  sideEffects: None
diff --git a/pkg/webhook/testdata/valid-ordered/webhook.go b/pkg/webhook/testdata/valid-ordered/webhook.go
new file mode 100644
index 00000000..c23b7a37
--- /dev/null
+++ b/pkg/webhook/testdata/valid-ordered/webhook.go
@@ -0,0 +1,84 @@
+/*
+
+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 cronjob
+
+import (
+	"context"
+
+	"k8s.io/apimachinery/pkg/runtime"
+	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+)
+
+func (c *CronJob) SetupWebhookWithManager(mgr ctrl.Manager) error {
+	return ctrl.NewWebhookManagedBy(mgr).
+		For(c).
+		Complete()
+}
+
+// +kubebuilder:webhook:webhookVersions=v1,verbs=create;update,path=/validate-testdata-kubebuilder-io-v1-cronjob,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=testdata.kubebuiler.io,resources=cronjobs,versions=v1,name=cronjob.testdata.kubebuilder.io,sideEffects=None,admissionReviewVersions=v1;v1beta1,reinvocationPolicy=Never
+
+type cronJobValidator struct {
+	client client.Client
+}
+
+func (v cronJobValidator) ValidateCreate(ctx context.Context, obj runtime.Object) error {
+	return nil
+}
+
+func (v cronJobValidator) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) error {
+	return nil
+}
+
+func (v cronJobValidator) ValidateDelete(ctx context.Context, obj runtime.Object) error {
+	return nil
+}
+
+// +kubebuilder:webhook:webhookVersions=v1,verbs=create;update,path=/validate-testdata-kubebuilder-io-v1-cronjoblist,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=testdata.kubebuiler.io,resources=cronjoblist,versions=v1,name=cronjoblist.testdata.kubebuilder.io,sideEffects=None,admissionReviewVersions=v1;v1beta1,reinvocationPolicy=Never
+
+type cronjobListValidator struct {
+	client client.Client
+}
+
+func (v cronJobListValidator) ValidateCreate(ctx context.Context, obj runtime.Object) error {
+	return nil
+}
+
+func (v cronJobListValidator) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) error {
+	return nil
+}
+
+func (v cronJobListValidator) ValidateDelete(ctx context.Context, obj runtime.Object) error {
+	return nil
+}
+
+// +kubebuilder:webhook:webhookVersions=v1,verbs=create;update,path=/validate-testdata-kubebuilder-io-v1-deployments,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=testdata.kubebuiler.io,resources=deployments,versions=v1,name=deployment.testdata.kubebuilder.io,sideEffects=None,admissionReviewVersions=v1;v1beta1,reinvocationPolicy=Never
+
+type deploymentValidator struct {
+	client client.Client
+}
+
+func (v deploymentValidator) ValidateCreate(ctx context.Context, obj runtime.Object) error {
+	return nil
+}
+
+func (v deploymentValidator) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) error {
+	return nil
+}
+
+func (v deploymentValidator) ValidateDelete(ctx context.Context, obj runtime.Object) error {
+	return nil
+}