Merge pull request #474 from jiachengxu/admission-revirew-versions

⚠ Add AdmissionReviewVersions support for webhook
kubernetes-sigs · Aug 25, 2020 · 9a9db70 · 9a9db70
2 parents 04609bd + 1dc8cc4
commit 9a9db70
Show file tree

Hide file tree

Showing 4 changed files with 51 additions and 48 deletions.
diff --git a/pkg/webhook/parser.go b/pkg/webhook/parser.go
@@ -19,7 +19,7 @@ limitations under the License.
 //
 // The markers take the form:
 //
-//  +kubebuilder:webhook:webhookVersions=<[]string>,failurePolicy=<string>,matchPolicy=<string>,groups=<[]string>,resources=<[]string>,verbs=<[]string>,versions=<[]string>,name=<string>,path=<string>,mutating=<bool>,sideEffects=<string>
+//  +kubebuilder:webhook:webhookVersions=<[]string>,failurePolicy=<string>,matchPolicy=<string>,groups=<[]string>,resources=<[]string>,verbs=<[]string>,versions=<[]string>,name=<string>,path=<string>,mutating=<bool>,sideEffects=<string>,admissionReviewVersions=<[]string>
 package webhook
 
 import (
@@ -107,6 +107,12 @@ type Config struct {
 	// WebhookVersions specifies the target API versions of the {Mutating,Validating}WebhookConfiguration objects
 	// itself to generate.  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"`
 }
 
 // verbToAPIVariant converts a marker's verb to the proper value for the API.
@@ -140,15 +146,13 @@ func (c Config) ToMutatingWebhook() (admissionregv1.MutatingWebhook, error) {
 	}
 
 	return admissionregv1.MutatingWebhook{
-		Name:          c.Name,
-		Rules:         c.rules(),
-		FailurePolicy: c.failurePolicy(),
-		MatchPolicy:   matchPolicy,
-		ClientConfig:  c.clientConfig(),
-		SideEffects:   c.sideEffects(),
-		// TODO(jiachengxu): AdmissionReviewVersions becomes required in admissionregistration/v1, here we default it
-		// to `v1` and `v1beta1`, and we should support to config the `AdmissionReviewVersions` as a marker.
-		AdmissionReviewVersions: []string{defaultWebhookVersion, "v1beta1"},
+		Name:                    c.Name,
+		Rules:                   c.rules(),
+		FailurePolicy:           c.failurePolicy(),
+		MatchPolicy:             matchPolicy,
+		ClientConfig:            c.clientConfig(),
+		SideEffects:             c.sideEffects(),
+		AdmissionReviewVersions: c.AdmissionReviewVersions,
 	}, nil
 }
 
@@ -164,15 +168,13 @@ func (c Config) ToValidatingWebhook() (admissionregv1.ValidatingWebhook, error)
 	}
 
 	return admissionregv1.ValidatingWebhook{
-		Name:          c.Name,
-		Rules:         c.rules(),
-		FailurePolicy: c.failurePolicy(),
-		MatchPolicy:   matchPolicy,
-		ClientConfig:  c.clientConfig(),
-		SideEffects:   c.sideEffects(),
-		// TODO(jiachengxu): AdmissionReviewVersions becomes required in admissionregistration/v1, here we default it
-		// to `v1` and `v1beta1`, and we should support to config the `AdmissionReviewVersions` as a marker.
-		AdmissionReviewVersions: []string{defaultWebhookVersion, "v1beta1"},
+		Name:                    c.Name,
+		Rules:                   c.rules(),
+		FailurePolicy:           c.failurePolicy(),
+		MatchPolicy:             matchPolicy,
+		ClientConfig:            c.clientConfig(),
+		SideEffects:             c.sideEffects(),
+		AdmissionReviewVersions: c.AdmissionReviewVersions,
 	}, nil
 }
 
@@ -344,29 +346,26 @@ func (Generator) Generate(ctx *genall.GenerationContext) error {
 				},
 				Webhooks: cfgs,
 			}
-			// SideEffects in required in admissionregistration/v1, if this is not set or set to `Some` or `Known`,
-			// we return an error
 			if version == defaultWebhookVersion {
 				for i := range objRaw.Webhooks {
+					// SideEffects is required in admissionregistration/v1, if this is not set or set to `Some` or `Known`,
+					// we 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,
+					// we return an error
+					if len(objRaw.Webhooks[i].AdmissionReviewVersions) == 0 {
+						return fmt.Errorf("AdmissionReviewVersions is mandatory for v1 {Mutating,Validating}WebhookConfiguration")
+					}
 				}
-			}
-			// AdmissionReviewVersions is optional in admissionregistration/v1beta1, so let kubernetes to default it.
-			if version == "v1beta1" {
-				for i := range objRaw.Webhooks {
-					objRaw.Webhooks[i].AdmissionReviewVersions = nil
-				}
-			}
-			if version != defaultWebhookVersion {
+				versionedWebhooks[version] = append(versionedWebhooks[version], objRaw)
+			} else {
 				conv, err := MutatingWebhookConfigurationAsVersion(objRaw, schema.GroupVersion{Group: admissionregv1.SchemeGroupVersion.Group, Version: version})
-				versionedWebhooks[version] = append(versionedWebhooks[version], conv)
 				if err != nil {
 					return err
 				}
-			} else {
-				versionedWebhooks[version] = append(versionedWebhooks[version], objRaw)
+				versionedWebhooks[version] = append(versionedWebhooks[version], conv)
 			}
 		}
 
@@ -381,29 +380,26 @@ func (Generator) Generate(ctx *genall.GenerationContext) error {
 				},
 				Webhooks: cfgs,
 			}
-			// SideEffects in required in admissionregistration/v1, if this is not set or set to `Some` or `Known`,
-			// we return an error
 			if version == defaultWebhookVersion {
 				for i := range objRaw.Webhooks {
+					// SideEffects is required in admissionregistration/v1, if this is not set or set to `Some` or `Known`,
+					// we 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,
+					// we return an error
+					if len(objRaw.Webhooks[i].AdmissionReviewVersions) == 0 {
+						return fmt.Errorf("AdmissionReviewVersions is mandatory for v1 {Mutating,Validating}WebhookConfiguration")
+					}
 				}
-			}
-			// AdmissionReviewVersions is optional in admissionregistration/v1beta1, so let kubernetes to default it.
-			if version == "v1beta1" {
-				for i := range objRaw.Webhooks {
-					objRaw.Webhooks[i].AdmissionReviewVersions = nil
-				}
-			}
-			if version != defaultWebhookVersion {
+				versionedWebhooks[version] = append(versionedWebhooks[version], objRaw)
+			} else {
 				conv, err := ValidatingWebhookConfigurationAsVersion(objRaw, schema.GroupVersion{Group: admissionregv1.SchemeGroupVersion.Group, Version: version})
-				versionedWebhooks[version] = append(versionedWebhooks[version], conv)
 				if err != nil {
 					return err
 				}
-			} else {
-				versionedWebhooks[version] = append(versionedWebhooks[version], objRaw)
+				versionedWebhooks[version] = append(versionedWebhooks[version], conv)
 			}
 		}
 	}

diff --git a/pkg/webhook/testdata/manifests.v1beta1.yaml b/pkg/webhook/testdata/manifests.v1beta1.yaml
@@ -6,7 +6,10 @@ metadata:
   creationTimestamp: null
   name: mutating-webhook-configuration
 webhooks:
-- clientConfig:
+- admissionReviewVersions:
+  - v1
+  - v1beta1
+  clientConfig:
     caBundle: Cg==
     service:
       name: webhook-service

diff --git a/pkg/webhook/testdata/webhook.go b/pkg/webhook/testdata/webhook.go
@@ -28,8 +28,8 @@ func (c *CronJob) SetupWebhookWithManager(mgr ctrl.Manager) error {
 }
 
 // +kubebuilder:webhook:webhookVersions=v1beta1,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=validation.cronjob.testdata.kubebuilder.io,sideEffects=Some
-// +kubebuilder:webhook: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=validation.cronjob.testdata.kubebuilder.io,sideEffects=NoneOnDryRun
-// +kubebuilder:webhook:webhookVersions=v1;v1beta1,verbs=create;update,path=/mutate-testdata-kubebuilder-io-v1-cronjob,mutating=true,failurePolicy=fail,matchPolicy=Equivalent,groups=testdata.kubebuiler.io,resources=cronjobs,versions=v1,name=default.cronjob.testdata.kubebuilder.io,sideEffects=None
+// +kubebuilder:webhook: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=validation.cronjob.testdata.kubebuilder.io,sideEffects=NoneOnDryRun,admissionReviewVersions=v1;v1beta1
+// +kubebuilder:webhook:webhookVersions=v1;v1beta1,verbs=create;update,path=/mutate-testdata-kubebuilder-io-v1-cronjob,mutating=true,failurePolicy=fail,matchPolicy=Equivalent,groups=testdata.kubebuiler.io,resources=cronjobs,versions=v1,name=default.cronjob.testdata.kubebuilder.io,sideEffects=None,admissionReviewVersions=v1;v1beta1
 
 var _ webhook.Defaulter = &CronJob{}
 var _ webhook.Validator = &CronJob{}

diff --git a/pkg/webhook/zz_generated.markerhelp.go b/pkg/webhook/zz_generated.markerhelp.go