Add more validation of IPPool CRD (#3570)

Validate IP version and prefix length fields.

Signed-off-by: Jianjun Shen <shenj@vmware.com>

pkg/controller/ipam/validate.go

@@ -195,6 +195,17 @@ func ipVersion(ip net.IP) int {
 }
 
 func validateIPRange(r crdv1alpha2.SubnetIPRange, poolIPVersion int) (bool, string) {
+	if poolIPVersion == 4 {
+		if r.PrefixLength <= 0 || r.PrefixLength >= 32 {
+			return false, fmt.Sprintf("Invalid prefix length %d", r.PrefixLength)
+		}
+	} else if poolIPVersion == 6 {
+		if r.PrefixLength <= 0 || r.PrefixLength >= 128 {
+			return false, fmt.Sprintf("Invalid prefix length %d", r.PrefixLength)
+		}
+	} else {
+		return false, fmt.Sprintf("Invalid IP version %d", poolIPVersion)
+	}
 	// Validate the integrity the IP range:
 	//  Verify that all the IP ranges have the same IP family as the IP pool
 	//  Verify that the gateway IP is reachable from the IP range
@@ -209,28 +220,27 @@ func validateIPRange(r crdv1alpha2.SubnetIPRange, poolIPVersion int) (bool, stri
 
 	if r.CIDR != "" {
 		_, cidr, _ := net.ParseCIDR(r.CIDR)
-
+		if ipVersion(cidr.IP) != poolIPVersion {
+			return false, fmt.Sprintf(
+				"Range is invalid. IP version of range %s differs from Pool IP version", r.CIDR)
+		}
 		if !netCIDR.Contains(cidr.IP) {
 			return false, fmt.Sprintf(
 				"Range is invalid. CIDR %s is not contained within subnet %s/%d",
 				r.CIDR, netCIDR.IP.String(), r.PrefixLength)
 		}
-		if ipVersion(cidr.IP) != poolIPVersion {
-			return false, fmt.Sprintf(
-				"Range is invalid. IP version of range %s differs from Pool IP version", r.CIDR)
-		}
 	} else {
 		rStart := net.ParseIP(r.Start)
 		rEnd := net.ParseIP(r.End)
+		if ipVersion(rStart) != poolIPVersion || ipVersion(rEnd) != poolIPVersion {
+			return false, fmt.Sprintf(
+				"Range is invalid. IP version of range %s-%s differs from Pool IP version", r.Start, r.End)
+		}
 		if !netCIDR.Contains(rStart) || !netCIDR.Contains(rEnd) {
 			return false, fmt.Sprintf(
 				"Range is invalid. range %s-%s is not contained within subnet %s/%d",
 				r.Start, r.End, netCIDR.IP.String(), r.PrefixLength)
 		}
-		if ipVersion(rStart) != poolIPVersion || ipVersion(rEnd) != poolIPVersion {
-			return false, fmt.Sprintf(
-				"Range is invalid. IP version of range %s-%s differs from Pool IP version", r.Start, r.End)
-		}
 	}
 	return true, ""
 }

pkg/controller/ipam/validate_test.go

@@ -92,6 +92,30 @@ func TestEgressControllerValidateExternalIPPool(t *testing.T) {
 			},
 			expectedResponse: &admv1.AdmissionResponse{Allowed: true},
 		},
+		{
+			name: "CREATE operation with invalid prefix length should not be allowed",
+			request: &admv1.AdmissionRequest{
+				Name:      "foo",
+				Operation: "CREATE",
+				Object: runtime.RawExtension{Raw: marshal(copyAndMutateIPPool(testIPPool, func(pool *crdv1alpha2.IPPool) {
+					pool.Spec.IPRanges = append(pool.Spec.IPRanges, crdv1alpha2.SubnetIPRange{
+						IPRange: crdv1alpha2.IPRange{
+							CIDR: "192.168.3.0/26",
+						},
+						SubnetInfo: crdv1alpha2.SubnetInfo{
+							Gateway:      "192.168.3.1",
+							PrefixLength: 32,
+						},
+					})
+				}))},
+			},
+			expectedResponse: &admv1.AdmissionResponse{
+				Allowed: false,
+				Result: &metav1.Status{
+					Message: "Invalid prefix length 32",
+				},
+			},
+		},
 		{
 			name: "CREATE operation with CIDR partially overlap with IP range should not be allowed",
 			request: &admv1.AdmissionRequest{
@@ -152,7 +176,7 @@ func TestEgressControllerValidateExternalIPPool(t *testing.T) {
 						},
 						SubnetInfo: crdv1alpha2.SubnetInfo{
 							Gateway:      "10:2400::01",
-							PrefixLength: 64,
+							PrefixLength: 24,
 						},
 					})
 				}))},