Merge pull request #638 from maiqueb/fix-confusing-error-msg-invalid-cidr

Fix confusing error msg invalid cidr

plugins/ipam/static/main.go

@@ -161,7 +161,7 @@ func LoadIPAMConfig(bytes []byte, envArgs string) (*IPAMConfig, string, error) {
 
 				ip, subnet, err := net.ParseCIDR(ipstr)
 				if err != nil {
-					return nil, "", fmt.Errorf("invalid CIDR %s: %s", ipstr, err)
+					return nil, "", fmt.Errorf("the 'ip' field is expected to be in CIDR notation, got: '%s'", ipstr)
 				}
 
 				addr := Address{
@@ -192,8 +192,13 @@ func LoadIPAMConfig(bytes []byte, envArgs string) (*IPAMConfig, string, error) {
 	if n.Args != nil && n.Args.A != nil && len(n.Args.A.IPs) != 0 {
 		// args IP overwrites IP, so clear IPAM Config
 		n.IPAM.Addresses = make([]Address, 0, len(n.Args.A.IPs))
-		for _, addr := range n.Args.A.IPs {
+		for _, addrStr := range n.Args.A.IPs {
-			n.IPAM.Addresses = append(n.IPAM.Addresses, Address{AddressStr: addr})
+			ip, addr, err := net.ParseCIDR(addrStr)
+			if err != nil {
+				return nil, "", fmt.Errorf("an entry in the 'ips' field is NOT in CIDR notation, got: '%s'", addrStr)
+			}
+			addr.IP = ip
+			n.IPAM.Addresses = append(n.IPAM.Addresses, Address{AddressStr: addrStr, Address: *addr})
 		}
 	}
 
@@ -201,8 +206,13 @@ func LoadIPAMConfig(bytes []byte, envArgs string) (*IPAMConfig, string, error) {
 	if len(n.RuntimeConfig.IPs) != 0 {
 		// runtimeConfig IP overwrites IP, so clear IPAM Config
 		n.IPAM.Addresses = make([]Address, 0, len(n.RuntimeConfig.IPs))
-		for _, addr := range n.RuntimeConfig.IPs {
+		for _, addrStr := range n.RuntimeConfig.IPs {
-			n.IPAM.Addresses = append(n.IPAM.Addresses, Address{AddressStr: addr})
+			ip, addr, err := net.ParseCIDR(addrStr)
+			if err != nil {
+				return nil, "", fmt.Errorf("an entry in the 'ips' field is NOT in CIDR notation, got: '%s'", addrStr)
+			}
+			addr.IP = ip
+			n.IPAM.Addresses = append(n.IPAM.Addresses, Address{AddressStr: addrStr, Address: *addr})
 		}
 	}
 
@@ -211,12 +221,15 @@ func LoadIPAMConfig(bytes []byte, envArgs string) (*IPAMConfig, string, error) {
 	numV6 := 0
 
 	for i := range n.IPAM.Addresses {
-		ip, addr, err := net.ParseCIDR(n.IPAM.Addresses[i].AddressStr)
+		if n.IPAM.Addresses[i].Address.IP == nil {
-		if err != nil {
+			ip, addr, err := net.ParseCIDR(n.IPAM.Addresses[i].AddressStr)
-			return nil, "", fmt.Errorf("invalid CIDR %s: %s", n.IPAM.Addresses[i].AddressStr, err)
+			if err != nil {
+				return nil, "", fmt.Errorf(
+					"the 'address' field is expected to be in CIDR notation, got: '%s'", n.IPAM.Addresses[i].AddressStr)
+			}
+			n.IPAM.Addresses[i].Address = *addr
+			n.IPAM.Addresses[i].Address.IP = ip
 		}
-		n.IPAM.Addresses[i].Address = *addr
-		n.IPAM.Addresses[i].Address.IP = ip
 
 		if err := canonicalizeIP(&n.IPAM.Addresses[i].Address.IP); err != nil {
 			return nil, "", fmt.Errorf("invalid address %d: %s", i, err)

plugins/ipam/static/static_test.go

@@ -546,6 +546,137 @@ var _ = Describe("static Operations", func() {
 			})
 			Expect(err).Should(MatchError("IPAM config missing 'ipam' key"))
 		})
+
+		It(fmt.Sprintf("[%s] errors when passed an invalid CIDR via ipam config", ver), func() {
+			const ifname string = "eth0"
+			const nspath string = "/some/where"
+			const ipStr string = "10.10.0.1"
+
+			conf := fmt.Sprintf(`{
+				"cniVersion": "%s",
+				"name": "mynet",
+				"type": "bridge",
+				"ipam": {
+					"type": "static",
+					"addresses": [ {
+						"address": "%s"
+					}]
+				}
+			}`, ver, ipStr)
+
+			args := &skel.CmdArgs{
+				ContainerID: "dummy",
+				Netns:       nspath,
+				IfName:      ifname,
+				StdinData:   []byte(conf),
+			}
+
+			// Allocate the IP
+			_, _, err := testutils.CmdAddWithArgs(args, func() error {
+				return cmdAdd(args)
+			})
+			Expect(err).Should(MatchError(
+				fmt.Sprintf("the 'address' field is expected to be in CIDR notation, got: '%s'", ipStr)))
+		})
+
+		It(fmt.Sprintf("[%s] errors when passed an invalid CIDR via Args", ver), func() {
+			const ifname string = "eth0"
+			const nspath string = "/some/where"
+			const ipStr string = "10.10.0.1"
+
+			conf := fmt.Sprintf(`{
+				"cniVersion": "%s",
+				"name": "mynet",
+				"type": "bridge",
+				"ipam": {
+					"type": "static",
+					"routes": [{ "dst": "0.0.0.0/0" }]
+				}
+			}`, ver)
+
+			args := &skel.CmdArgs{
+				ContainerID: "dummy",
+				Netns:       nspath,
+				IfName:      ifname,
+				StdinData:   []byte(conf),
+				Args:        fmt.Sprintf("IP=%s", ipStr),
+			}
+
+			// Allocate the IP
+			_, _, err := testutils.CmdAddWithArgs(args, func() error {
+				return cmdAdd(args)
+			})
+			Expect(err).Should(MatchError(
+				fmt.Sprintf("the 'ip' field is expected to be in CIDR notation, got: '%s'", ipStr)))
+		})
+
+		It(fmt.Sprintf("[%s] errors when passed an invalid CIDR via CNI_ARGS", ver), func() {
+			const ifname string = "eth0"
+			const nspath string = "/some/where"
+			const ipStr string = "10.10.0.1"
+
+			conf := fmt.Sprintf(`{
+				"cniVersion": "%s",
+				"name": "mynet",
+				"type": "bridge",
+				"ipam": {
+					"type": "static",
+					"routes": [{ "dst": "0.0.0.0/0" }]
+				},
+				"args": {
+					"cni": {
+						"ips" : ["%s"]
+					}
+				}
+			}`, ver, ipStr)
+
+			args := &skel.CmdArgs{
+				ContainerID: "dummy",
+				Netns:       nspath,
+				IfName:      ifname,
+				StdinData:   []byte(conf),
+			}
+
+			// Allocate the IP
+			_, _, err := testutils.CmdAddWithArgs(args, func() error {
+				return cmdAdd(args)
+			})
+			Expect(err).Should(MatchError(
+				fmt.Sprintf("an entry in the 'ips' field is NOT in CIDR notation, got: '%s'", ipStr)))
+		})
+
+		It(fmt.Sprintf("[%s] errors when passed an invalid CIDR via RuntimeConfig", ver), func() {
+			const ifname string = "eth0"
+			const nspath string = "/some/where"
+			const ipStr string = "10.10.0.1"
+
+			conf := fmt.Sprintf(`{
+				"cniVersion": "%s",
+				"name": "mynet",
+				"type": "bridge",
+				"ipam": {
+					"type": "static",
+					"routes": [{ "dst": "0.0.0.0/0" }]
+				},
+				"RuntimeConfig": {
+					"ips" : ["%s"]
+				}
+			}`, ver, ipStr)
+
+			args := &skel.CmdArgs{
+				ContainerID: "dummy",
+				Netns:       nspath,
+				IfName:      ifname,
+				StdinData:   []byte(conf),
+			}
+
+			// Allocate the IP
+			_, _, err := testutils.CmdAddWithArgs(args, func() error {
+				return cmdAdd(args)
+			})
+			Expect(err).Should(MatchError(
+				fmt.Sprintf("an entry in the 'ips' field is NOT in CIDR notation, got: '%s'", ipStr)))
+		})
 	}
 })