From c89f58d2608a691f3980f804d9a8f7d746670f64 Mon Sep 17 00:00:00 2001 From: Justin Santa Barbara Date: Tue, 24 Jan 2017 22:53:59 -0500 Subject: [PATCH] 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 | 12 --- pkg/apis/kops/validation/validation.go | 31 +++++++- pkg/apis/kops/validation/validation_test.go | 83 +++++++++++++++++---- upup/pkg/fi/cloudup/deepvalidate_test.go | 2 +- 4 files changed, 98 insertions(+), 30 deletions(-) diff --git a/pkg/apis/kops/validation/legacy.go b/pkg/apis/kops/validation/legacy.go index 44a67b5d53..b710b3a11f 100644 --- a/pkg/apis/kops/validation/legacy.go +++ b/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 { diff --git a/pkg/apis/kops/validation/validation.go b/pkg/apis/kops/validation/validation.go index e2177069b6..7b2271b727 100644 --- a/pkg/apis/kops/validation/validation.go +++ b/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) } diff --git a/pkg/apis/kops/validation/validation_test.go b/pkg/apis/kops/validation/validation_test.go index 9c32405a84..92fa642b91 100644 --- a/pkg/apis/kops/validation/validation_test.go +++ b/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 { - 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()) - } - } - } + testErrors(t, g.Input, errs, g.ExpectedErrors) } } diff --git a/upup/pkg/fi/cloudup/deepvalidate_test.go b/upup/pkg/fi/cloudup/deepvalidate_test.go index 34bf0dec17..4253a7fa96 100644 --- a/upup/pkg/fi/cloudup/deepvalidate_test.go +++ b/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) {