Refactor and improve API validation

kubernetes · Jun 5, 2020 · f15480f · f15480f
1 parent 4095e4c
commit f15480f
Show file tree

Hide file tree

Showing 3 changed files with 29 additions and 27 deletions.
diff --git a/pkg/apis/kops/validation/legacy.go b/pkg/apis/kops/validation/legacy.go
@@ -19,9 +19,7 @@ package validation
 import (
 	"fmt"
 	"net"
-	"strings"
 
-	"k8s.io/apimachinery/pkg/util/validation"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	"k8s.io/kops/pkg/apis/kops"
 	"k8s.io/kops/pkg/apis/kops/util"
@@ -48,25 +46,6 @@ func ValidateCluster(c *kops.Cluster, strict bool) field.ErrorList {
 		return allErrs
 	}
 
-	if c.ObjectMeta.Name == "" {
-		allErrs = append(allErrs, field.Required(field.NewPath("objectMeta", "name"), "Cluster Name is required (e.g. --name=mycluster.myzone.com)"))
-	} else {
-		// Must be a dns name
-		errs := validation.IsDNS1123Subdomain(c.ObjectMeta.Name)
-		if len(errs) != 0 {
-			allErrs = append(allErrs, field.Invalid(field.NewPath("objectMeta", "name"), c.ObjectMeta.Name, fmt.Sprintf("Cluster Name must be a valid DNS name (e.g. --name=mycluster.myzone.com) errors: %s", strings.Join(errs, ", "))))
-		} else if !strings.Contains(c.ObjectMeta.Name, ".") {
-			// Tolerate if this is a cluster we are importing for upgrade
-			if c.ObjectMeta.Annotations[kops.AnnotationNameManagement] != kops.AnnotationValueManagementImported {
-				allErrs = append(allErrs, field.Invalid(field.NewPath("objectMeta", "name"), c.ObjectMeta.Name, "Cluster Name must be a fully-qualified DNS name (e.g. --name=mycluster.myzone.com)"))
-			}
-		}
-	}
-
-	if c.Spec.Assets != nil && c.Spec.Assets.ContainerProxy != nil && c.Spec.Assets.ContainerRegistry != nil {
-		allErrs = append(allErrs, field.Forbidden(fieldSpec.Child("Assets", "ContainerProxy"), "ContainerProxy cannot be used in conjunction with ContainerRegistry as represent mutually exclusive concepts. Please consult the documentation for details."))
-	}
-
 	requiresSubnets := true
 	requiresNetworkCIDR := true
 	requiresSubnetCIDR := true

diff --git a/pkg/apis/kops/validation/validation.go b/pkg/apis/kops/validation/validation.go
@@ -42,6 +42,23 @@ import (
 
 func newValidateCluster(cluster *kops.Cluster) field.ErrorList {
 	allErrs := validation.ValidateObjectMeta(&cluster.ObjectMeta, false, validation.NameIsDNSSubdomain, field.NewPath("metadata"))
+
+	clusterName := cluster.ObjectMeta.Name
+	if clusterName == "" {
+		allErrs = append(allErrs, field.Required(field.NewPath("objectMeta", "name"), "Cluster Name is required (e.g. --name=mycluster.myzone.com)"))
+	} else {
+		// Must be a dns name
+		errs := utilvalidation.IsDNS1123Subdomain(clusterName)
+		if len(errs) != 0 {
+			allErrs = append(allErrs, field.Invalid(field.NewPath("objectMeta", "name"), clusterName, fmt.Sprintf("Cluster Name must be a valid DNS name (e.g. --name=mycluster.myzone.com) errors: %s", strings.Join(errs, ", "))))
+		} else if !strings.Contains(clusterName, ".") {
+			// Tolerate if this is a cluster we are importing for upgrade
+			if cluster.ObjectMeta.Annotations[kops.AnnotationNameManagement] != kops.AnnotationValueManagementImported {
+				allErrs = append(allErrs, field.Invalid(field.NewPath("objectMeta", "name"), clusterName, "Cluster Name must be a fully-qualified DNS name (e.g. --name=mycluster.myzone.com)"))
+			}
+		}
+	}
+
 	allErrs = append(allErrs, validateClusterSpec(&cluster.Spec, cluster, field.NewPath("spec"))...)
 
 	// Additional cloud-specific validation rules
@@ -155,6 +172,12 @@ func validateClusterSpec(spec *kops.ClusterSpec, c *kops.Cluster, fieldPath *fie
 		allErrs = append(allErrs, validateDockerConfig(spec.Docker, fieldPath.Child("docker"))...)
 	}
 
+	if spec.Assets != nil {
+		if spec.Assets.ContainerProxy != nil && spec.Assets.ContainerRegistry != nil {
+			allErrs = append(allErrs, field.Forbidden(fieldPath.Child("assets", "containerProxy"), "containerProxy cannot be used in conjunction with containerRegistry"))
+		}
+	}
+
 	if spec.RollingUpdate != nil {
 		allErrs = append(allErrs, validateRollingUpdate(spec.RollingUpdate, fieldPath.Child("rollingUpdate"), false)...)
 	}
@@ -441,18 +464,18 @@ func validateNodeAuthorization(n *kops.NodeAuthorizationSpec, c *kops.Cluster, f
 		return field.ErrorList{field.Forbidden(fldPath, "node authorization is experimental feature; set `export KOPS_FEATURE_FLAGS=EnableNodeAuthorization`")}
 	}
 
+	authorizerPath := fldPath.Child("nodeAuthorizer")
 	if c.Spec.NodeAuthorization.NodeAuthorizer == nil {
-		allErrs = append(allErrs, field.Forbidden(fldPath, "no node authorization policy has been set"))
+		allErrs = append(allErrs, field.Required(authorizerPath, "no node authorization policy has been set"))
 	} else {
-		path := fldPath.Child("nodeAuthorizer")
 		if c.Spec.NodeAuthorization.NodeAuthorizer.Port < 0 || n.NodeAuthorizer.Port >= 65535 {
-			allErrs = append(allErrs, field.Invalid(path.Child("port"), n.NodeAuthorizer.Port, "invalid port"))
+			allErrs = append(allErrs, field.Invalid(authorizerPath.Child("port"), n.NodeAuthorizer.Port, "invalid port"))
 		}
 		if c.Spec.NodeAuthorization.NodeAuthorizer.Timeout != nil && n.NodeAuthorizer.Timeout.Duration <= 0 {
-			allErrs = append(allErrs, field.Invalid(path.Child("timeout"), n.NodeAuthorizer.Timeout, "must be greater than zero"))
+			allErrs = append(allErrs, field.Invalid(authorizerPath.Child("timeout"), n.NodeAuthorizer.Timeout, "must be greater than zero"))
 		}
 		if c.Spec.NodeAuthorization.NodeAuthorizer.TokenTTL != nil && n.NodeAuthorizer.TokenTTL.Duration < 0 {
-			allErrs = append(allErrs, field.Invalid(path.Child("tokenTTL"), n.NodeAuthorizer.TokenTTL, "must be greater than or equal to zero"))
+			allErrs = append(allErrs, field.Invalid(authorizerPath.Child("tokenTTL"), n.NodeAuthorizer.TokenTTL, "must be greater than or equal to zero"))
 		}
 
 		// @question: we could probably just default these settings in the model when the node-authorizer is enabled??

diff --git a/upup/pkg/fi/cloudup/validation_test.go b/upup/pkg/fi/cloudup/validation_test.go
@@ -174,7 +174,7 @@ func TestValidate_ContainerRegistry_and_ContainerProxy_exclusivity(t *testing.T)
 
 	proxy := "https://proxy.example.com/"
 	c.Spec.Assets.ContainerProxy = &proxy
-	expectErrorFromValidate(t, c, "ContainerProxy cannot be used in conjunction with ContainerRegistry")
+	expectErrorFromValidate(t, c, "containerProxy cannot be used in conjunction with containerRegistry")
 
 	c.Spec.Assets.ContainerRegistry = nil
 	expectNoErrorFromValidate(t, c)