Refactor webhookclientConfig validation of admission and audit registration

pkg/apis/admissionregistration/validation/validation.go

@@ -18,7 +18,6 @@ package validation
 
 import (
 	"fmt"
-	"net/url"
 	"strings"
 
 	genericvalidation "k8s.io/apimachinery/pkg/api/validation"
@@ -26,6 +25,7 @@ import (
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apimachinery/pkg/util/validation"
 	"k8s.io/apimachinery/pkg/util/validation/field"
+	"k8s.io/apiserver/pkg/util/webhook"
 	"k8s.io/kubernetes/pkg/apis/admissionregistration"
 )
 
@@ -200,93 +200,15 @@ func validateWebhook(hook *admissionregistration.Webhook, fldPath *field.Path) f
 		allErrors = append(allErrors, metav1validation.ValidateLabelSelector(hook.NamespaceSelector, fldPath.Child("namespaceSelector"))...)
 	}
 
-	allErrors = append(allErrors, validateWebhookClientConfig(fldPath.Child("clientConfig"), &hook.ClientConfig)...)
+	cc := hook.ClientConfig
-
+	switch {
-	return allErrors
+	case (cc.URL == nil) == (cc.Service == nil):
-}
+		allErrors = append(allErrors, field.Required(fldPath.Child("clientConfig"), "exactly one of url or service is required"))
-
+	case cc.URL != nil:
-func validateWebhookClientConfig(fldPath *field.Path, cc *admissionregistration.WebhookClientConfig) field.ErrorList {
+		allErrors = append(allErrors, webhook.ValidateWebhookURL(fldPath.Child("clientConfig").Child("url"), *cc.URL, true)...)
-	var allErrors field.ErrorList
+	case cc.Service != nil:
-	if (cc.URL == nil) == (cc.Service == nil) {
+		allErrors = append(allErrors, webhook.ValidateWebhookService(fldPath.Child("clientConfig").Child("service"), cc.Service.Name, cc.Service.Namespace, cc.Service.Path)...)
-		allErrors = append(allErrors, field.Required(fldPath.Child("url"), "exactly one of url or service is required"))
 	}
-
-	if cc.URL != nil {
-		const form = "; desired format: https://host[/path]"
-		if u, err := url.Parse(*cc.URL); err != nil {
-			allErrors = append(allErrors, field.Required(fldPath.Child("url"), "url must be a valid URL: "+err.Error()+form))
-		} else {
-			if u.Scheme != "https" {
-				allErrors = append(allErrors, field.Invalid(fldPath.Child("url"), u.Scheme, "'https' is the only allowed URL scheme"+form))
-			}
-			if len(u.Host) == 0 {
-				allErrors = append(allErrors, field.Invalid(fldPath.Child("url"), u.Host, "host must be provided"+form))
-			}
-			if u.User != nil {
-				allErrors = append(allErrors, field.Invalid(fldPath.Child("url"), u.User.String(), "user information is not permitted in the URL"))
-			}
-			if len(u.Fragment) != 0 {
-				allErrors = append(allErrors, field.Invalid(fldPath.Child("url"), u.Fragment, "fragments are not permitted in the URL"))
-			}
-			if len(u.RawQuery) != 0 {
-				allErrors = append(allErrors, field.Invalid(fldPath.Child("url"), u.RawQuery, "query parameters are not permitted in the URL"))
-			}
-		}
-	}
-
-	if cc.Service != nil {
-		allErrors = append(allErrors, validateWebhookService(fldPath.Child("service"), cc.Service)...)
-	}
-	return allErrors
-}
-
-// note: this has copy/paste inheritance in auditregistration
-func validateWebhookService(fldPath *field.Path, svc *admissionregistration.ServiceReference) field.ErrorList {
-	var allErrors field.ErrorList
-
-	if len(svc.Name) == 0 {
-		allErrors = append(allErrors, field.Required(fldPath.Child("name"), "service name is required"))
-	}
-
-	if len(svc.Namespace) == 0 {
-		allErrors = append(allErrors, field.Required(fldPath.Child("namespace"), "service namespace is required"))
-	}
-
-	if svc.Path == nil {
-		return allErrors
-	}
-
-	// TODO: replace below with url.Parse + verifying that host is empty?
-
-	urlPath := *svc.Path
-	if urlPath == "/" || len(urlPath) == 0 {
-		return allErrors
-	}
-	if urlPath == "//" {
-		allErrors = append(allErrors, field.Invalid(fldPath.Child("path"), urlPath, "segment[0] may not be empty"))
-		return allErrors
-	}
-
-	if !strings.HasPrefix(urlPath, "/") {
-		allErrors = append(allErrors, field.Invalid(fldPath.Child("path"), urlPath, "must start with a '/'"))
-	}
-
-	urlPathToCheck := urlPath[1:]
-	if strings.HasSuffix(urlPathToCheck, "/") {
-		urlPathToCheck = urlPathToCheck[:len(urlPathToCheck)-1]
-	}
-	steps := strings.Split(urlPathToCheck, "/")
-	for i, step := range steps {
-		if len(step) == 0 {
-			allErrors = append(allErrors, field.Invalid(fldPath.Child("path"), urlPath, fmt.Sprintf("segment[%d] may not be empty", i)))
-			continue
-		}
-		failures := validation.IsDNS1123Subdomain(step)
-		for _, failure := range failures {
-			allErrors = append(allErrors, field.Invalid(fldPath.Child("path"), urlPath, fmt.Sprintf("segment[%d]: %v", i, failure)))
-		}
-	}
-
 	return allErrors
 }
 

pkg/apis/admissionregistration/validation/validation_test.go

@@ -540,7 +540,7 @@ func TestValidateValidatingWebhookConfiguration(t *testing.T) {
 						},
 					},
 				}),
-			expectedError: `[0].clientConfig.url: Required value: exactly one of url or service is required`,
+			expectedError: `[0].clientConfig: Required value: exactly one of url or service is required`,
 		},
 		{
 			name: "blank URL",

pkg/apis/auditregistration/validation/validation.go

@@ -17,14 +17,12 @@ limitations under the License.
 package validation
 
 import (
-	"fmt"
-	"net/url"
 	"strings"
 
 	genericvalidation "k8s.io/apimachinery/pkg/api/validation"
 	"k8s.io/apimachinery/pkg/util/sets"
-	"k8s.io/apimachinery/pkg/util/validation"
 	"k8s.io/apimachinery/pkg/util/validation/field"
+	"k8s.io/apiserver/pkg/util/webhook"
 	"k8s.io/kubernetes/pkg/apis/auditregistration"
 )
 
@@ -49,7 +47,16 @@ func ValidateWebhook(w auditregistration.Webhook, fldPath *field.Path) field.Err
 	if w.Throttle != nil {
 		allErrs = append(allErrs, ValidateWebhookThrottleConfig(w.Throttle, fldPath.Child("throttle"))...)
 	}
-	allErrs = append(allErrs, ValidateWebhookClientConfig(&w.ClientConfig, fldPath.Child("clientConfig"))...)
+
+	cc := w.ClientConfig
+	switch {
+	case (cc.URL == nil) == (cc.Service == nil):
+		allErrs = append(allErrs, field.Required(fldPath.Child("clientConfig"), "exactly one of url or service is required"))
+	case cc.URL != nil:
+		allErrs = append(allErrs, webhook.ValidateWebhookURL(fldPath.Child("clientConfig").Child("url"), *cc.URL, false)...)
+	case cc.Service != nil:
+		allErrs = append(allErrs, webhook.ValidateWebhookService(fldPath.Child("clientConfig").Child("service"), cc.Service.Name, cc.Service.Namespace, cc.Service.Path)...)
+	}
 	return allErrs
 }
 
@@ -65,90 +72,6 @@ func ValidateWebhookThrottleConfig(c *auditregistration.WebhookThrottleConfig, f
 	return allErrs
 }
 
-// ValidateWebhookClientConfig validates the WebhookClientConfig
-// note: this is largely copy/paste inheritance from admissionregistration with subtle changes
-func ValidateWebhookClientConfig(cc *auditregistration.WebhookClientConfig, fldPath *field.Path) field.ErrorList {
-	var allErrors field.ErrorList
-	if (cc.URL == nil) == (cc.Service == nil) {
-		allErrors = append(allErrors, field.Required(fldPath.Child("url"), "exactly one of url or service is required"))
-	}
-
-	if cc.URL != nil {
-		const form = "; desired format: https://host[/path]"
-		if u, err := url.Parse(*cc.URL); err != nil {
-			allErrors = append(allErrors, field.Required(fldPath.Child("url"), "url must be a valid URL: "+err.Error()+form))
-		} else {
-			if len(u.Host) == 0 {
-				allErrors = append(allErrors, field.Invalid(fldPath.Child("url"), u.Host, "host must be provided"+form))
-			}
-			if u.User != nil {
-				allErrors = append(allErrors, field.Invalid(fldPath.Child("url"), u.User.String(), "user information is not permitted in the URL"))
-			}
-			if len(u.Fragment) != 0 {
-				allErrors = append(allErrors, field.Invalid(fldPath.Child("url"), u.Fragment, "fragments are not permitted in the URL"))
-			}
-			if len(u.RawQuery) != 0 {
-				allErrors = append(allErrors, field.Invalid(fldPath.Child("url"), u.RawQuery, "query parameters are not permitted in the URL"))
-			}
-		}
-	}
-
-	if cc.Service != nil {
-		allErrors = append(allErrors, validateWebhookService(cc.Service, fldPath.Child("service"))...)
-	}
-	return allErrors
-}
-
-// note: this is copy/paste inheritance from admissionregistration
-func validateWebhookService(svc *auditregistration.ServiceReference, fldPath *field.Path) field.ErrorList {
-	var allErrors field.ErrorList
-
-	if len(svc.Name) == 0 {
-		allErrors = append(allErrors, field.Required(fldPath.Child("name"), "service name is required"))
-	}
-
-	if len(svc.Namespace) == 0 {
-		allErrors = append(allErrors, field.Required(fldPath.Child("namespace"), "service namespace is required"))
-	}
-
-	if svc.Path == nil {
-		return allErrors
-	}
-
-	// TODO: replace below with url.Parse + verifying that host is empty?
-
-	urlPath := *svc.Path
-	if urlPath == "/" || len(urlPath) == 0 {
-		return allErrors
-	}
-	if urlPath == "//" {
-		allErrors = append(allErrors, field.Invalid(fldPath.Child("path"), urlPath, "segment[0] may not be empty"))
-		return allErrors
-	}
-
-	if !strings.HasPrefix(urlPath, "/") {
-		allErrors = append(allErrors, field.Invalid(fldPath.Child("path"), urlPath, "must start with a '/'"))
-	}
-
-	urlPathToCheck := urlPath[1:]
-	if strings.HasSuffix(urlPathToCheck, "/") {
-		urlPathToCheck = urlPathToCheck[:len(urlPathToCheck)-1]
-	}
-	steps := strings.Split(urlPathToCheck, "/")
-	for i, step := range steps {
-		if len(step) == 0 {
-			allErrors = append(allErrors, field.Invalid(fldPath.Child("path"), urlPath, fmt.Sprintf("segment[%d] may not be empty", i)))
-			continue
-		}
-		failures := validation.IsDNS1123Subdomain(step)
-		for _, failure := range failures {
-			allErrors = append(allErrors, field.Invalid(fldPath.Child("path"), urlPath, fmt.Sprintf("segment[%d]: %v", i, failure)))
-		}
-	}
-
-	return allErrors
-}
-
 // ValidatePolicy validates the audit policy
 func ValidatePolicy(policy auditregistration.Policy, fldPath *field.Path) field.ErrorList {
 	var allErrs field.ErrorList

pkg/apis/auditregistration/validation/validation_test.go

@@ -159,7 +159,7 @@ func TestValidateWebhookConfiguration(t *testing.T) {
 					URL: strPtr("example.com/k8s/webhook"),
 				},
 			},
-			expectedError: `webhook.clientConfig.url: Required value: exactly one of url or service is required`,
+			expectedError: `webhook.clientConfig: Required value: exactly one of url or service is required`,
 		},
 		{
 			name: "blank URL",

staging/src/k8s.io/apiserver/pkg/util/webhook/validation.go

@@ -0,0 +1,101 @@
+/*
+Copyright 2018 The Kubernetes Authors.
+
+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 webhook
+
+import (
+	"fmt"
+	"net/url"
+	"strings"
+
+	"k8s.io/apimachinery/pkg/util/validation"
+	"k8s.io/apimachinery/pkg/util/validation/field"
+)
+
+// ValidateWebhookURL validates webhook's URL.
+func ValidateWebhookURL(fldPath *field.Path, URL string, forceHttps bool) field.ErrorList {
+	var allErrors field.ErrorList
+	const form = "; desired format: https://host[/path]"
+	if u, err := url.Parse(URL); err != nil {
+		allErrors = append(allErrors, field.Required(fldPath, "url must be a valid URL: "+err.Error()+form))
+	} else {
+		if forceHttps && u.Scheme != "https" {
+			allErrors = append(allErrors, field.Invalid(fldPath, u.Scheme, "'https' is the only allowed URL scheme"+form))
+		}
+		if len(u.Host) == 0 {
+			allErrors = append(allErrors, field.Invalid(fldPath, u.Host, "host must be provided"+form))
+		}
+		if u.User != nil {
+			allErrors = append(allErrors, field.Invalid(fldPath, u.User.String(), "user information is not permitted in the URL"))
+		}
+		if len(u.Fragment) != 0 {
+			allErrors = append(allErrors, field.Invalid(fldPath, u.Fragment, "fragments are not permitted in the URL"))
+		}
+		if len(u.RawQuery) != 0 {
+			allErrors = append(allErrors, field.Invalid(fldPath, u.RawQuery, "query parameters are not permitted in the URL"))
+		}
+	}
+	return allErrors
+}
+
+func ValidateWebhookService(fldPath *field.Path, namespace, name string, path *string) field.ErrorList {
+	var allErrors field.ErrorList
+
+	if len(name) == 0 {
+		allErrors = append(allErrors, field.Required(fldPath.Child("name"), "service name is required"))
+	}
+
+	if len(namespace) == 0 {
+		allErrors = append(allErrors, field.Required(fldPath.Child("namespace"), "service namespace is required"))
+	}
+
+	if path == nil {
+		return allErrors
+	}
+
+	// TODO: replace below with url.Parse + verifying that host is empty?
+
+	urlPath := *path
+	if urlPath == "/" || len(urlPath) == 0 {
+		return allErrors
+	}
+	if urlPath == "//" {
+		allErrors = append(allErrors, field.Invalid(fldPath.Child("path"), urlPath, "segment[0] may not be empty"))
+		return allErrors
+	}
+
+	if !strings.HasPrefix(urlPath, "/") {
+		allErrors = append(allErrors, field.Invalid(fldPath.Child("path"), urlPath, "must start with a '/'"))
+	}
+
+	urlPathToCheck := urlPath[1:]
+	if strings.HasSuffix(urlPathToCheck, "/") {
+		urlPathToCheck = urlPathToCheck[:len(urlPathToCheck)-1]
+	}
+	steps := strings.Split(urlPathToCheck, "/")
+	for i, step := range steps {
+		if len(step) == 0 {
+			allErrors = append(allErrors, field.Invalid(fldPath.Child("path"), urlPath, fmt.Sprintf("segment[%d] may not be empty", i)))
+			continue
+		}
+		failures := validation.IsDNS1123Subdomain(step)
+		for _, failure := range failures {
+			allErrors = append(allErrors, field.Invalid(fldPath.Child("path"), urlPath, fmt.Sprintf("segment[%d]: %v", i, failure)))
+		}
+	}
+
+	return allErrors
+}