Better validate CIDRs - provide some hints on failure

With this:

`kops create cluster ... --admin-access 12.34.56.78`

gives

spec.sshAccess[0]: Invalid value: "12.34.56.78": Could not be parsed as
a CIDR (did you mean "12.34.56.78/32")

Fix #1595

pkg/apis/kops/validation/legacy.go

@@ -250,23 +250,11 @@ func ValidateCluster(c *kops.Cluster, strict bool) error {
 	if strict && len(c.Spec.SSHAccess) == 0 {
 		return fmt.Errorf("SSHAccess not configured")
 	}
-	for _, cidr := range c.Spec.SSHAccess {
-		_, _, err := net.ParseCIDR(cidr)
-		if err != nil {
-			return fmt.Errorf("SSHAccess rule %q could not be parsed (invalid CIDR)", cidr)
-		}
-	}
 
 	// AdminAccess
 	if strict && len(c.Spec.KubernetesAPIAccess) == 0 {
 		return fmt.Errorf("KubernetesAPIAccess not configured")
 	}
-	for _, cidr := range c.Spec.KubernetesAPIAccess {
-		_, _, err := net.ParseCIDR(cidr)
-		if err != nil {
-			return fmt.Errorf("KubernetesAPIAccess rule %q could not be parsed (invalid CIDR)", cidr)
-		}
-	}
 
 	// KubeProxy
 	if c.Spec.KubeProxy != nil {

pkg/apis/kops/validation/validation.go

@@ -22,6 +22,8 @@ import (
 	"k8s.io/kubernetes/pkg/api/validation"
 	"k8s.io/kubernetes/pkg/util/sets"
 	"k8s.io/kubernetes/pkg/util/validation/field"
+	"net"
+	"strings"
 )
 
 var validDockerConfigStorageValues = []string{"aufs", "btrfs", "devicemapper", "overlay", "overlay2", "zfs"}
@@ -43,6 +45,33 @@ func validateClusterSpec(spec *kops.ClusterSpec, fieldPath *field.Path) field.Er
 
 	allErrs = append(allErrs, validateSubnets(spec.Subnets, field.NewPath("spec"))...)
 
+	// SSHAccess
+	for i, cidr := range spec.SSHAccess {
+		allErrs = append(allErrs, validateCIDR(cidr, fieldPath.Child("sshAccess").Index(i))...)
+	}
+
+	// AdminAccess
+	for i, cidr := range spec.KubernetesAPIAccess {
+		allErrs = append(allErrs, validateCIDR(cidr, fieldPath.Child("kubernetesAPIAccess").Index(i))...)
+	}
+
+	return allErrs
+}
+
+func validateCIDR(cidr string, fieldPath *field.Path) field.ErrorList {
+	allErrs := field.ErrorList{}
+
+	_, _, err := net.ParseCIDR(cidr)
+	if err != nil {
+		detail := "Could not be parsed as a CIDR"
+		if !strings.Contains(cidr, "/") {
+			ip := net.ParseIP(cidr)
+			if ip != nil {
+				detail += fmt.Sprintf(" (did you mean \"%s/32\")", cidr)
+			}
+		}
+		allErrs = append(allErrs, field.Invalid(fieldPath, cidr, detail))
+	}
 	return allErrs
 }
 
@@ -65,7 +94,7 @@ func validateSubnets(subnets []kops.ClusterSubnetSpec, fieldPath *field.Path) fi
 		for i := range subnets {
 			name := subnets[i].Name
 			if names.Has(name) {
-				allErrs = append(allErrs, field.Invalid(fieldPath, subnets, fmt.Sprintf("Subnets with duplicate name %q found", name)))
+				allErrs = append(allErrs, field.Invalid(fieldPath, subnets, fmt.Sprintf("subnets with duplicate name %q found", name)))
 			}
 			names.Insert(name)
 		}

pkg/apis/kops/validation/validation_test.go

@@ -33,6 +33,72 @@ func Test_Validate_DNS(t *testing.T) {
 	}
 }
 
+func TestValidateCIDR(t *testing.T) {
+	grid := []struct {
+		Input          string
+		ExpectedErrors []string
+		ExpectedDetail string
+	}{
+		{
+			Input: "192.168.0.1/32",
+		},
+		{
+			Input:          "192.168.0.1",
+			ExpectedErrors: []string{"Invalid value::CIDR"},
+			ExpectedDetail: "Could not be parsed as a CIDR (did you mean \"192.168.0.1/32\")",
+		},
+		{
+			Input:          "",
+			ExpectedErrors: []string{"Invalid value::CIDR"},
+		},
+		{
+			Input:          "invalid.example.com",
+			ExpectedErrors: []string{"Invalid value::CIDR"},
+			ExpectedDetail: "Could not be parsed as a CIDR",
+		},
+	}
+	for _, g := range grid {
+		errs := validateCIDR(g.Input, field.NewPath("CIDR"))
+
+		testErrors(t, g.Input, errs, g.ExpectedErrors)
+
+		if g.ExpectedDetail != "" {
+			found := false
+			for _, err := range errs {
+				if err.Detail == g.ExpectedDetail {
+					found = true
+				}
+			}
+			if !found {
+				for _, err := range errs {
+					t.Logf("found detail: %q", err.Detail)
+				}
+
+				t.Errorf("did not find expected error %q", g.ExpectedDetail)
+			}
+		}
+	}
+}
+
+func testErrors(t *testing.T, context interface{}, actual field.ErrorList, expectedErrors []string) {
+	if len(expectedErrors) == 0 {
+		if len(actual) != 0 {
+			t.Errorf("unexpected errors from %q: %v", context, actual)
+		}
+	} else {
+		errStrings := sets.NewString()
+		for _, err := range actual {
+			errStrings.Insert(err.Type.String() + "::" + err.Field)
+		}
+
+		for _, expected := range expectedErrors {
+			if !errStrings.Has(expected) {
+				t.Errorf("expected error %v from %q, was not found in %q", expected, context, errStrings.List())
+			}
+		}
+	}
+}
+
 func TestValidateSubnets(t *testing.T) {
 	grid := []struct {
 		Input          []kops.ClusterSubnetSpec
@@ -73,22 +139,7 @@ func TestValidateSubnets(t *testing.T) {
 	for _, g := range grid {
 		errs := validateSubnets(g.Input, field.NewPath("Subnets"))
 
-		if len(g.ExpectedErrors) == 0 {
+		testErrors(t, g.Input, errs, g.ExpectedErrors)
-			if len(errs) != 0 {
-				t.Errorf("unexpected errors from %q: %v", g.Input, errs)
-			}
-		} else {
-			errStrings := sets.NewString()
-			for _, err := range errs {
-				errStrings.Insert(err.Type.String() + "::" + err.Field)
-			}
-
-			for _, expected := range g.ExpectedErrors {
-				if !errStrings.Has(expected) {
-					t.Errorf("expected error %v from %q, was not found in %q", expected, g.Input, errStrings.List())
-				}
-			}
-		}
 	}
 }
 

upup/pkg/fi/cloudup/deepvalidate_test.go

@@ -107,7 +107,7 @@ func TestDeepValidate_DuplicateZones(t *testing.T) {
 	var groups []*api.InstanceGroup
 	groups = append(groups, buildMinimalMasterInstanceGroup("dup1"))
 	groups = append(groups, buildMinimalNodeInstanceGroup("dup1"))
-	expectErrorFromDeepValidate(t, c, groups, "Subnets with duplicate name \"dup1\" found")
+	expectErrorFromDeepValidate(t, c, groups, "subnets with duplicate name \"dup1\" found")
 }
 
 func TestDeepValidate_ExtraMasterZone(t *testing.T) {