remove support for v1beta1 webhooks

pkg/webhook/parser.go

@@ -36,7 +36,8 @@ import (
 
 // The default {Mutating,Validating}WebhookConfiguration version to generate.
 const (
-	defaultWebhookVersion = "v1"
+	v1                    = "v1"
+	defaultWebhookVersion = v1
 )
 
 var (
@@ -47,7 +48,7 @@ var (
 
 // supportedWebhookVersions returns currently supported API version of {Mutating,Validating}WebhookConfiguration.
 func supportedWebhookVersions() []string {
-	return []string{defaultWebhookVersion, "v1beta1"}
+	return []string{defaultWebhookVersion}
 }
 
 // +controllertools:marker:generateHelp:category=Webhook
@@ -104,14 +105,12 @@ type Config struct {
 	Path string
 
 	// WebhookVersions specifies the target API versions of the {Mutating,Validating}WebhookConfiguration objects
-	// itself to generate.  Defaults to v1.
+	// itself to generate. The only supported value is v1. Defaults to v1.
 	WebhookVersions []string `marker:"webhookVersions,optional"`
 
 	// AdmissionReviewVersions is an ordered list of preferred `AdmissionReview`
 	// versions the Webhook expects.
-	// For generating v1 {Mutating,Validating}WebhookConfiguration, this is mandatory.
-	// For generating v1beta1 {Mutating,Validating}WebhookConfiguration, this is optional, and default to v1beta1.
-	AdmissionReviewVersions []string `marker:"admissionReviewVersions,optional"`
+	AdmissionReviewVersions []string `marker:"admissionReviewVersions"`
 }
 
 // verbToAPIVariant converts a marker's verb to the proper value for the API.
@@ -331,9 +330,8 @@ func (Generator) Generate(ctx *genall.GenerationContext) error {
 	versionedWebhooks := make(map[string][]interface{}, len(supportedWebhookVersions))
 	for _, version := range supportedWebhookVersions {
 		if cfgs, ok := mutatingCfgs[version]; ok {
-			// All webhook config versions in supportedWebhookVersions have the same general form, with a few
-			// stricter requirements for v1. Since no conversion scheme exists for webhook configs, the v1
-			// type can be used for all versioned types in this context.
+			// The only possible version in supportedWebhookVersions is v1,
+			// so use it for all versioned types in this context.
 			objRaw := &admissionregv1.MutatingWebhookConfiguration{}
 			objRaw.SetGroupVersionKind(schema.GroupVersionKind{
 				Group:   admissionregv1.SchemeGroupVersion.Group,
@@ -342,28 +340,24 @@ func (Generator) Generate(ctx *genall.GenerationContext) error {
 			})
 			objRaw.SetName("mutating-webhook-configuration")
 			objRaw.Webhooks = cfgs
-			switch version {
-			case admissionregv1.SchemeGroupVersion.Version:
-				for i := range objRaw.Webhooks {
-					// SideEffects is required in admissionregistration/v1, if this is not set or set to `Some` or `Known`,
-					// return an error
-					if err := checkSideEffectsForV1(objRaw.Webhooks[i].SideEffects); err != nil {
-						return err
-					}
-					// AdmissionReviewVersions is required in admissionregistration/v1, if this is not set,
-					// return an error
-					if len(objRaw.Webhooks[i].AdmissionReviewVersions) == 0 {
-						return fmt.Errorf("AdmissionReviewVersions is mandatory for v1 {Mutating,Validating}WebhookConfiguration")
-					}
+			for i := range objRaw.Webhooks {
+				// SideEffects is required in admissionregistration/v1, if this is not set or set to `Some` or `Known`,
+				// return an error
+				if err := checkSideEffectsForV1(objRaw.Webhooks[i].SideEffects); err != nil {
+					return err
+				}
+				// AdmissionReviewVersions is required in admissionregistration/v1, if this is not set,
+				// return an error
+				if len(objRaw.Webhooks[i].AdmissionReviewVersions) == 0 {
+					return fmt.Errorf("AdmissionReviewVersions is mandatory for v1 {Mutating,Validating}WebhookConfiguration")
 				}
 			}
 			versionedWebhooks[version] = append(versionedWebhooks[version], objRaw)
 		}
 
 		if cfgs, ok := validatingCfgs[version]; ok {
-			// All webhook config versions in supportedWebhookVersions have the same general form, with a few
-			// stricter requirements for v1. Since no conversion scheme exists for webhook configs, the v1
-			// type can be used for all versioned types in this context.
+			// The only possible version in supportedWebhookVersions is v1,
+			// so use it for all versioned types in this context.
 			objRaw := &admissionregv1.ValidatingWebhookConfiguration{}
 			objRaw.SetGroupVersionKind(schema.GroupVersionKind{
 				Group:   admissionregv1.SchemeGroupVersion.Group,
@@ -372,19 +366,16 @@ func (Generator) Generate(ctx *genall.GenerationContext) error {
 			})
 			objRaw.SetName("validating-webhook-configuration")
 			objRaw.Webhooks = cfgs
-			switch version {
-			case admissionregv1.SchemeGroupVersion.Version:
-				for i := range objRaw.Webhooks {
-					// SideEffects is required in admissionregistration/v1, if this is not set or set to `Some` or `Known`,
-					// return an error
-					if err := checkSideEffectsForV1(objRaw.Webhooks[i].SideEffects); err != nil {
-						return err
-					}
-					// AdmissionReviewVersions is required in admissionregistration/v1, if this is not set,
-					// return an error
-					if len(objRaw.Webhooks[i].AdmissionReviewVersions) == 0 {
-						return fmt.Errorf("AdmissionReviewVersions is mandatory for v1 {Mutating,Validating}WebhookConfiguration")
-					}
+			for i := range objRaw.Webhooks {
+				// SideEffects is required in admissionregistration/v1, if this is not set or set to `Some` or `Known`,
+				// return an error
+				if err := checkSideEffectsForV1(objRaw.Webhooks[i].SideEffects); err != nil {
+					return err
+				}
+				// AdmissionReviewVersions is required in admissionregistration/v1, if this is not set,
+				// return an error
+				if len(objRaw.Webhooks[i].AdmissionReviewVersions) == 0 {
+					return fmt.Errorf("AdmissionReviewVersions is mandatory for v1 {Mutating,Validating}WebhookConfiguration")
 				}
 			}
 			versionedWebhooks[version] = append(versionedWebhooks[version], objRaw)

pkg/webhook/parser_integration_test.go

@@ -35,19 +35,108 @@ import (
 	"sigs.k8s.io/controller-tools/pkg/webhook"
 )
 
+// TODO(directxman12): test generation across multiple versions (right
+// now, we're trusting k/k's conversion code, though, which is probably
+// fine for the time being)
 var _ = Describe("Webhook Generation From Parsing to CustomResourceDefinition", func() {
 	assertSame := func(actual, expected interface{}) {
 		ExpectWithOffset(1, actual).To(Equal(expected), "type not as expected, check pkg/webhook/testdata/README.md for more details.\n\nDiff:\n\n%s", cmp.Diff(actual, expected))
 	}
 
+	It("should fail generating a v1beta1 webhook", func() {
+		By("switching into testdata to appease go modules")
+		cwd, err := os.Getwd()
+		Expect(err).NotTo(HaveOccurred())
+		Expect(os.Chdir("./testdata/invalid-v1beta1NotSupported")).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)
+		genCtx := &genall.GenerationContext{
+			Collector:  &markers.Collector{Registry: reg},
+			Roots:      pkgs,
+			OutputRule: genall.OutputToDirectory(outputDir),
+		}
+		err = webhook.Generator{}.Generate(genCtx)
+		Expect(err).To(MatchError("unsupported webhook version: v1beta1"))
+	})
+
+	It("should fail without admissionReviewVersions specified", func() {
+		By("switching into testdata to appease go modules")
+		cwd, err := os.Getwd()
+		Expect(err).NotTo(HaveOccurred())
+		Expect(os.Chdir("./testdata/invalid-admissionReviewVersionsRequired")).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)
+		genCtx := &genall.GenerationContext{
+			Collector:  &markers.Collector{Registry: reg},
+			Roots:      pkgs,
+			OutputRule: genall.OutputToDirectory(outputDir),
+		}
+		Expect(webhook.Generator{}.Generate(genCtx)).To(Succeed())
+		Expect(genCtx.Roots).To(HaveLen(1))
+		Expect(genCtx.Roots[0].Errors).To(HaveLen(1))
+		Expect(genCtx.Roots[0].Errors[0].Error()).To(ContainSubstring(`missing argument "admissionReviewVersions"`))
+	})
+
+	It("should fail with invalid side effects", func() {
+		By("switching into testdata to appease go modules")
+		cwd, err := os.Getwd()
+		Expect(err).NotTo(HaveOccurred())
+		Expect(os.Chdir("./testdata/invalid-sideEffects")).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)
+		genCtx := &genall.GenerationContext{
+			Collector:  &markers.Collector{Registry: reg},
+			Roots:      pkgs,
+			OutputRule: genall.OutputToDirectory(outputDir),
+		}
+		err = webhook.Generator{}.Generate(genCtx)
+		Expect(err).To(MatchError("SideEffects should not be set to `Some` or `Unknown` for v1 {Mutating,Validating}WebhookConfiguration"))
+	})
+
 	It("should properly generate the webhook definition", func() {
-		// TODO(directxman12): test generation across multiple versions (right
-		// now, we're trusting k/k's conversion code, though, which is probably
-		// fine for the time being)
 		By("switching into testdata to appease go modules")
 		cwd, err := os.Getwd()
 		Expect(err).NotTo(HaveOccurred())
-		Expect(os.Chdir("./testdata")).To(Succeed()) // go modules are directory-sensitive
+		Expect(os.Chdir("./testdata/valid")).To(Succeed()) // go modules are directory-sensitive
 		defer func() { Expect(os.Chdir(cwd)).To(Succeed()) }()
 
 		By("loading the roots")
@@ -63,11 +152,15 @@ var _ = Describe("Webhook Generation From Parsing to CustomResourceDefinition",
 		outputDir, err := ioutil.TempDir("", "webhook-integration-test")
 		Expect(err).NotTo(HaveOccurred())
 		defer os.RemoveAll(outputDir)
-		Expect(webhook.Generator{}.Generate(&genall.GenerationContext{
+		genCtx := &genall.GenerationContext{
 			Collector:  &markers.Collector{Registry: reg},
 			Roots:      pkgs,
 			OutputRule: genall.OutputToDirectory(outputDir),
-		})).To(Succeed())
+		}
+		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"))
@@ -82,20 +175,6 @@ var _ = Describe("Webhook Generation From Parsing to CustomResourceDefinition",
 		By("comparing the two")
 		assertSame(actualMutating, expectedMutating)
 		assertSame(actualValidating, expectedValidating)
-
-		By("loading the generated v1beta1 YAML")
-		actualFile, err = ioutil.ReadFile(path.Join(outputDir, "manifests.v1beta1.yaml"))
-		Expect(err).NotTo(HaveOccurred())
-		actualMutatingV1beta1, actualValidatingV1beta1 := unmarshalBothV1beta1(actualFile)
-
-		By("loading the desired v1beta1 YAML")
-		expectedFile, err = ioutil.ReadFile("manifests.v1beta1.yaml")
-		Expect(err).NotTo(HaveOccurred())
-		expectedMutatingV1beta1, expectedValidatingV1beta1 := unmarshalBothV1beta1(expectedFile)
-
-		By("comparing the two")
-		assertSame(actualMutatingV1beta1, expectedMutatingV1beta1)
-		assertSame(actualValidatingV1beta1, expectedValidatingV1beta1)
 	})
 })
 

pkg/webhook/testdata/invalid-admissionReviewVersionsRequired/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{})
+}

pkg/webhook/testdata/manifests.v1beta1.yaml → pkg/webhook/testdata/invalid-admissionReviewVersionsRequired/manifests.yaml

@@ -1,6 +1,6 @@
 
 ---
-apiVersion: admissionregistration.k8s.io/v1beta1
+apiVersion: admissionregistration.k8s.io/v1
 kind: MutatingWebhookConfiguration
 metadata:
   creationTimestamp: null
@@ -30,13 +30,37 @@ webhooks:
   sideEffects: None
 
 ---
-apiVersion: admissionregistration.k8s.io/v1beta1
+apiVersion: admissionregistration.k8s.io/v1
 kind: ValidatingWebhookConfiguration
 metadata:
   creationTimestamp: null
   name: validating-webhook-configuration
 webhooks:
-- admissionReviewVersions: null
+- 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.kubebuiler.io
+    apiVersions:
+    - v1
+    operations:
+    - CREATE
+    - UPDATE
+    resources:
+    - cronjobs
+  sideEffects: None
+- admissionReviewVersions:
+  - v1
+  - v1beta1
   clientConfig:
     service:
       name: webhook-service
@@ -55,4 +79,4 @@ webhooks:
     - UPDATE
     resources:
     - cronjobs
-  sideEffects: Some
+  sideEffects: NoneOnDryRun