Merge pull request #1065 from abhinavdahiya/fix-1023

[FIX] Validate provided addresses are network addresses
openshift · Feb 2, 2019 · 0316257 · 0316257
2 parents 810b13a + 5c29e90
commit 0316257
Show file tree

Hide file tree

Showing 4 changed files with 26 additions and 22 deletions.
diff --git a/pkg/types/validation/installconfig.go b/pkg/types/validation/installconfig.go
@@ -61,10 +61,10 @@ func validateNetworking(n *types.Networking, fldPath *field.Path) field.ErrorLis
 		allErrs = append(allErrs, field.Required(fldPath.Child("type"), "network provider type required"))
 	}
 	if err := validate.SubnetCIDR(&n.MachineCIDR.IPNet); err != nil {
-		allErrs = append(allErrs, field.Invalid(fldPath.Child("machineCIDR"), n.MachineCIDR, err.Error()))
+		allErrs = append(allErrs, field.Invalid(fldPath.Child("machineCIDR"), n.MachineCIDR.String(), err.Error()))
 	}
 	if err := validate.SubnetCIDR(&n.ServiceCIDR.IPNet); err != nil {
-		allErrs = append(allErrs, field.Invalid(fldPath.Child("serviceCIDR"), n.ServiceCIDR, err.Error()))
+		allErrs = append(allErrs, field.Invalid(fldPath.Child("serviceCIDR"), n.ServiceCIDR.String(), err.Error()))
 	}
 	for i, cn := range n.ClusterNetworks {
 		allErrs = append(allErrs, validateClusterNetwork(&cn, fldPath.Child("clusterNetworks").Index(i), &n.ServiceCIDR.IPNet)...)

diff --git a/pkg/types/validation/installconfig_test.go b/pkg/types/validation/installconfig_test.go
@@ -128,7 +128,7 @@ func TestValidateInstallConfig(t *testing.T) {
 				c.Networking.ServiceCIDR = &ipnet.IPNet{}
 				return c
 			}(),
-			expectedError: `^networking\.serviceCIDR: Invalid value: ipnet\.IPNet{IPNet:net\.IPNet{IP:net\.IP\(nil\), Mask:net\.IPMask\(nil\)}}: must use IPv4$`,
+			expectedError: `^networking\.serviceCIDR: Invalid value: "<nil>": must use IPv4$`,
 		},
 		{
 			name: "overlapping cluster network cidr",

diff --git a/pkg/validate/validate.go b/pkg/validate/validate.go
@@ -70,14 +70,18 @@ func ClusterName(v string) error {
 	return validateSubdomain(v)
 }
 
-// SubnetCIDR checks if the given IP net is a valid CIDR for a master nodes or worker nodes subnet and returns an error if not.
+// SubnetCIDR checks if the given IP net is a valid CIDR.
 func SubnetCIDR(cidr *net.IPNet) error {
 	if cidr.IP.To4() == nil {
 		return errors.New("must use IPv4")
 	}
 	if cidr.IP.IsUnspecified() {
 		return errors.New("address must be specified")
 	}
+	nip := cidr.IP.Mask(cidr.Mask)
+	if nip.String() != cidr.IP.String() {
+		return fmt.Errorf("invalid network address. got %s, expecting %s", cidr.String(), (&net.IPNet{IP: nip, Mask: cidr.Mask}).String())
+	}
 	if DoCIDRsOverlap(cidr, dockerBridgeCIDR) {
 		return fmt.Errorf("overlaps with default Docker Bridge subnet (%v)", cidr.String())
 	}

diff --git a/pkg/validate/validate_test.go b/pkg/validate/validate_test.go
@@ -51,32 +51,32 @@ func TestClusterName(t *testing.T) {
 
 func TestSubnetCIDR(t *testing.T) {
 	cases := []struct {
-		cidr  string
-		valid bool
+		cidr   string
+		expErr string
 	}{
-		{"0.0.0.0/32", false},
-		{"1.2.3.4/0", false},
-		{"1.2.3.4/1", false},
-		{"1.2.3.4/31", true},
-		{"1.2.3.4/32", true},
-		{"0:0:0:0:0:1:102:304/116", false},
-		{"0:0:0:0:0:ffff:102:304/116", true},
-		{"172.17.1.2/20", false},
-		{"172.17.1.2/8", false},
-		{"255.255.255.255/1", false},
-		{"255.255.255.255/32", true},
+		{"0.0.0.0/32", "address must be specified"},
+		{"1.2.3.4/0", "invalid network address. got 1.2.3.4/0, expecting 0.0.0.0/0"},
+		{"1.2.3.4/1", "invalid network address. got 1.2.3.4/1, expecting 0.0.0.0/1"},
+		{"1.2.3.4/31", ""},
+		{"1.2.3.4/32", ""},
+		{"0:0:0:0:0:1:102:304/116", "must use IPv4"},
+		{"0:0:0:0:0:ffff:102:304/116", "invalid network address. got 1.2.3.4/20, expecting 1.2.0.0/20"},
+		{"172.17.0.0/20", "overlaps with default Docker Bridge subnet (172.17.0.0/20)"},
+		{"172.0.0.0/8", "overlaps with default Docker Bridge subnet (172.0.0.0/8)"},
+		{"255.255.255.255/1", "invalid network address. got 255.255.255.255/1, expecting 128.0.0.0/1"},
+		{"255.255.255.255/32", ""},
 	}
 	for _, tc := range cases {
 		t.Run(tc.cidr, func(t *testing.T) {
-			_, cidr, err := net.ParseCIDR(tc.cidr)
+			ip, cidr, err := net.ParseCIDR(tc.cidr)
 			if err != nil {
 				t.Fatalf("could not parse cidr: %v", err)
 			}
-			err = SubnetCIDR(cidr)
-			if tc.valid {
-				assert.NoError(t, err)
+			err = SubnetCIDR(&net.IPNet{IP: ip, Mask: cidr.Mask})
+			if tc.expErr != "" {
+				assert.EqualError(t, err, tc.expErr)
 			} else {
-				assert.Error(t, err)
+				assert.NoError(t, err)
 			}
 		})
 	}