From 5d0f77367df2a6f160e6c2819c1bf812f6bf8a29 Mon Sep 17 00:00:00 2001
From: Erik Nelson <ernelson@redhat.com>
Date: Thu, 22 Mar 2018 15:35:28 -0700
Subject: [PATCH] Refactor common broker validations (#1865)

---
 pkg/apis/servicecatalog/validation/broker.go | 60 ++++++++++++--------
 1 file changed, 37 insertions(+), 23 deletions(-)

diff --git a/pkg/apis/servicecatalog/validation/broker.go b/pkg/apis/servicecatalog/validation/broker.go
index a2e7482357e6..617e802f1b15 100644
--- a/pkg/apis/servicecatalog/validation/broker.go
+++ b/pkg/apis/servicecatalog/validation/broker.go
@@ -24,17 +24,19 @@ import (
 	sc "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog"
 )
 
-// ValidateClusterServiceBrokerName is the validation function for Broker names.
-var ValidateClusterServiceBrokerName = apivalidation.NameIsDNSSubdomain
+// validateCommonServiceBrokerName is the validation function for common
+// broker names.
+var validateCommonServiceBrokerName = apivalidation.NameIsDNSSubdomain
 
-// ValidateClusterServiceBroker implements the validation rules for a BrokerResource.
+// ValidateClusterServiceBroker implements the validation rules for a
+// ClusterServiceBroker.
 func ValidateClusterServiceBroker(broker *sc.ClusterServiceBroker) field.ErrorList {
 	allErrs := field.ErrorList{}
 
 	allErrs = append(allErrs,
 		apivalidation.ValidateObjectMeta(&broker.ObjectMeta,
 			false, /* namespace required */
-			ValidateClusterServiceBrokerName,
+			validateCommonServiceBrokerName,
 			field.NewPath("metadata"))...)
 
 	allErrs = append(allErrs, validateClusterServiceBrokerSpec(&broker.Spec, field.NewPath("spec"))...)
@@ -44,12 +46,6 @@ func ValidateClusterServiceBroker(broker *sc.ClusterServiceBroker) field.ErrorLi
 func validateClusterServiceBrokerSpec(spec *sc.ClusterServiceBrokerSpec, fldPath *field.Path) field.ErrorList {
 	allErrs := field.ErrorList{}
 
-	if "" == spec.URL {
-		allErrs = append(allErrs,
-			field.Required(fldPath.Child("url"),
-				"brokers must have a remote url to contact"))
-	}
-
 	// if there is auth information, check it to make sure that it's properly formatted
 	if spec.AuthInfo != nil {
 		if spec.AuthInfo.Basic != nil {
@@ -91,12 +87,30 @@ func validateClusterServiceBrokerSpec(spec *sc.ClusterServiceBrokerSpec, fldPath
 		}
 	}
 
+	commonErrs := validateCommonServiceBrokerSpec(&spec.CommonServiceBrokerSpec, fldPath)
+
+	if len(commonErrs) != 0 {
+		allErrs = append(commonErrs)
+	}
+
+	return allErrs
+}
+
+func validateCommonServiceBrokerSpec(spec *sc.CommonServiceBrokerSpec, fldPath *field.Path) field.ErrorList {
+	commonErrs := field.ErrorList{}
+
+	if "" == spec.URL {
+		commonErrs = append(commonErrs,
+			field.Required(fldPath.Child("url"),
+				"brokers must have a remote url to contact"))
+	}
+
 	if spec.InsecureSkipTLSVerify && len(spec.CABundle) > 0 {
-		allErrs = append(allErrs, field.Invalid(fldPath.Child("caBundle"), spec.CABundle, "caBundle cannot be used when insecureSkipTLSVerify is true"))
+		commonErrs = append(commonErrs, field.Invalid(fldPath.Child("caBundle"), spec.CABundle, "caBundle cannot be used when insecureSkipTLSVerify is true"))
 	}
 
 	if "" == spec.RelistBehavior {
-		allErrs = append(allErrs,
+		commonErrs = append(commonErrs,
 			field.Required(fldPath.Child("relistBehavior"),
 				"relist behavior is required"))
 	}
@@ -105,29 +119,29 @@ func validateClusterServiceBrokerSpec(spec *sc.ClusterServiceBrokerSpec, fldPath
 		spec.RelistBehavior == sc.ServiceBrokerRelistBehaviorManual
 	if !isValidRelistBehavior {
 		errMsg := "relist behavior must be \"Manual\" or \"Duration\""
-		allErrs = append(
-			allErrs,
+		commonErrs = append(
+			commonErrs,
 			field.Required(fldPath.Child("relistBehavior"), errMsg),
 		)
 	}
 
 	if spec.RelistBehavior == sc.ServiceBrokerRelistBehaviorDuration && spec.RelistDuration == nil {
-		allErrs = append(
-			allErrs,
+		commonErrs = append(
+			commonErrs,
 			field.Required(fldPath.Child("relistDuration"), "relistDuration must be set if relistBehavior is set to Duration"),
 		)
 	}
 
 	if spec.RelistBehavior == sc.ServiceBrokerRelistBehaviorManual && spec.RelistDuration != nil {
-		allErrs = append(
-			allErrs,
+		commonErrs = append(
+			commonErrs,
 			field.Required(fldPath.Child("relistDuration"), "relistDuration must not be set if relistBehavior is set to Manual"),
 		)
 	}
 
 	if spec.RelistRequests < 0 {
-		allErrs = append(
-			allErrs,
+		commonErrs = append(
+			commonErrs,
 			field.Required(fldPath.Child("relistRequests"), "relistRequests must be greater than zero"),
 		)
 	}
@@ -135,14 +149,14 @@ func validateClusterServiceBrokerSpec(spec *sc.ClusterServiceBrokerSpec, fldPath
 	if spec.RelistDuration != nil {
 		zeroDuration := metav1.Duration{Duration: 0}
 		if spec.RelistDuration.Duration <= zeroDuration.Duration {
-			allErrs = append(
-				allErrs,
+			commonErrs = append(
+				commonErrs,
 				field.Required(fldPath.Child("relistDuration"), "relistDuration must be greater than zero"),
 			)
 		}
 	}
 
-	return allErrs
+	return commonErrs
 }
 
 // ValidateClusterServiceBrokerUpdate checks that when changing from an older broker to a newer broker is okay ?