Merge pull request #333 from gunjan5/ip-validation

validate if more than one IPv4/v6 are passed with ipAddrsNoIpam/ipAdd…
projectcalico · May 9, 2017 · 8eac9ac · 8eac9ac
2 parents 4cabccc + 74166de
commit 8eac9ac
Show file tree

Hide file tree

Showing 3 changed files with 123 additions and 40 deletions.
diff --git a/calico_cni_k8s_test.go b/calico_cni_k8s_test.go
@@ -884,6 +884,60 @@ var _ = Describe("CalicoCni", func() {
 					Expect(err).ShouldNot(HaveOccurred())
 				})
 			})
+
+			Context("ask for more than 1 IPv4 addrs using ipAddrsNoIpam annotation to assign IP address to a pod, bypassing IPAM", func() {
+				It("should return an error", func() {
+					netconfCalicoIPAM := fmt.Sprintf(`
+				{
+			      "cniVersion": "%s",
+				  "name": "net9",
+				  "type": "calico",
+				  "etcd_endpoints": "http://%s:2379",
+			 	  "ipam": {},
+					"kubernetes": {
+					  "k8s_api_root": "http://127.0.0.1:8080"
+					 },
+					"policy": {"type": "k8s"},
+					"log_level":"info"
+				}`, cniVersion, os.Getenv("ETCD_IP"))
+
+					config, err := clientcmd.DefaultClientConfig.ClientConfig()
+					Expect(err).NotTo(HaveOccurred())
+
+					clientset, err := kubernetes.NewForConfig(config)
+					Expect(err).NotTo(HaveOccurred())
+
+					// Now create a K8s pod passing in more than one IPv4 address.
+					name := fmt.Sprintf("run%d-ip", rand.Uint32())
+					pod, err := clientset.Pods(K8S_TEST_NS).Create(&v1.Pod{
+						ObjectMeta: v1.ObjectMeta{
+							Name: name,
+							Annotations: map[string]string{
+								"cni.projectcalico.org/ipAddrsNoIpam": "[\"10.0.0.1\", \"10.0.0.2\"]",
+							},
+						},
+						Spec: v1.PodSpec{Containers: []v1.Container{{
+							Name:  fmt.Sprintf("container-%s", name),
+							Image: "ignore",
+						}}},
+					})
+					Expect(err).NotTo(HaveOccurred())
+
+					logger.Infof("Created POD object: %v", pod)
+
+					_, netnspath, _, _, _, _, _, err := CreateContainer(netconfCalicoIPAM, name, "")
+					Expect(err).To(HaveOccurred())
+
+					// Make sure the WorkloadEndpoint is not created in the datastore.
+					endpoints, err := calicoClient.WorkloadEndpoints().List(api.WorkloadEndpointMetadata{})
+					Expect(err).ShouldNot(HaveOccurred())
+					Expect(endpoints.Items).Should(HaveLen(0))
+
+					// Delete the container.
+					_, err = DeleteContainer(netconfCalicoIPAM, netnspath, name)
+					Expect(err).ShouldNot(HaveOccurred())
+				})
+			})
 		})
 	})
 })
diff --git a/k8s/k8s.go b/k8s/k8s.go
@@ -104,7 +104,7 @@ func CmdAddK8s(args *skel.CmdArgs, conf utils.NetConf, nodename string, calicoCl
 			if err != nil {
 				return nil, err
 			}
-			logger.WithField("stdin", args.StdinData).Debug("Updated stdin data")
+			logger.WithField("stdin", string(args.StdinData)).Debug("Updated stdin data")
 		}
 
 		labels := make(map[string]string)
@@ -170,7 +170,7 @@ func CmdAddK8s(args *skel.CmdArgs, conf utils.NetConf, nodename string, calicoCl
 						return nil, err
 					}
 					args.StdinData = newData
-					logger.WithField("stdin", args.StdinData).Debug("Updated stdin data")
+					logger.WithField("stdin", string(args.StdinData)).Debug("Updated stdin data")
 				}
 			}
 		}
@@ -363,39 +363,29 @@ func CmdDelK8s(c *calicoclient.Client, ep api.WorkloadEndpointMetadata, args *sk
 // each IP passed to it by setting the IP field in CNI_ARGS, and returns the result of calling the IPAM plugin.
 // Example annotation value string: "[\"10.0.0.1\", \"2001:db8::1\"]"
 func ipAddrsResult(ipAddrs string, conf utils.NetConf, args *skel.CmdArgs, logger *log.Entry) (*current.Result, error) {
-
 	logger.Infof("Parsing annotation \"cni.projectcalico.org/ipAddrs\":%s", ipAddrs)
-	ips, err := parseIPAddrs(ipAddrs, logger)
-	if err != nil {
-		return nil, fmt.Errorf("Failed to parse IPs %s for annotation \"cni.projectcalico.org/ipAddrs\": %s", ipAddrs, err)
-	}
 
-	// annotation value can't be empty.
-	if len(ips) == 0 {
-		return nil, fmt.Errorf("Annotation \"cni.projectcalico.org/ipAddrs\" specified but empty")
+	// We need to make sure there is only one IPv4 and/or one IPv6
+	// passed in, since CNI spec only supports one of each right now.
+	ipList, err := validateAndExtractIPs(ipAddrs, "cni.projectcalico.org/ipAddrs", logger)
+	if err != nil {
+		return nil, err
 	}
 
 	result := current.Result{}
 
 	// Go through all the IPs passed in as annotation value and call IPAM plugin
 	// for each, and populate the result variable with IP4 and/or IP6 IPs returned
-	// from the IPAM plugin. We also make sure there is only one IPv4 and/or one IPv6
-	// passed in, since CNI spec only supports one of each right now.
-	for _, ip := range ips {
-		ipAddr := net.ParseIP(ip)
-		if ipAddr == nil {
-			logger.WithField("IP", ip).Error("Invalid IP format")
-			return nil, fmt.Errorf("Invalid IP format: %s", ip)
-		}
-
+	// from the IPAM plugin.
+	for _, ip := range ipList {
 		// Call callIPAMWithIP with the ip address.
-		r, err := callIPAMWithIP(ipAddr, conf, args, logger)
+		r, err := callIPAMWithIP(ip, conf, args, logger)
 		if err != nil {
 			return nil, fmt.Errorf("Error getting IP from IPAM: %s", err)
 		}
 
 		result.IPs = append(result.IPs, r.IPs[0])
-		logger.Debugf("Adding IPv%s: %s to result", r.IPs[0].Version, ipAddr.String())
+		logger.Debugf("Adding IPv%s: %s to result", r.IPs[0].Version, ip.String())
 	}
 
 	return &result, nil
@@ -476,35 +466,27 @@ func callIPAMWithIP(ip net.IP, conf utils.NetConf, args *skel.CmdArgs, logger *l
 // but sets IP field manually since IPAM is bypassed with this annotation.
 // Example annotation value string: "[\"10.0.0.1\", \"2001:db8::1\"]"
 func overrideIPAMResult(ipAddrsNoIpam string, logger *log.Entry) (*current.Result, error) {
-
 	logger.Infof("Parsing annotation \"cni.projectcalico.org/ipAddrsNoIpam\":%s", ipAddrsNoIpam)
-	ips, err := parseIPAddrs(ipAddrsNoIpam, logger)
-	if err != nil {
-		return nil, fmt.Errorf("Failed to parse IPs %s for annotation \"cni.projectcalico.org/ipAddrsNoIpam\": %s", ipAddrsNoIpam, err)
-	}
 
-	// annotation value can't be empty.
-	if len(ips) == 0 {
-		return nil, fmt.Errorf("Annotation \"cni.projectcalico.org/ipAddrsNoIpam\" specified but empty")
+	// We need to make sure there is only one IPv4 and/or one IPv6
+	// passed in, since CNI spec only supports one of each right now.
+	ipList, err := validateAndExtractIPs(ipAddrsNoIpam, "cni.projectcalico.org/ipAddrsNoIpam", logger)
+	if err != nil {
+		return nil, err
 	}
 
 	result := current.Result{}
 
 	// Go through all the IPs passed in as annotation value and populate
 	// the result variable with IP4 and/or IP6 IPs.
-	for _, ip := range ips {
-		ipAddr := net.ParseIP(ip)
-		if ipAddr == nil {
-			logger.WithField("IP", ip).Error("Invalid IP format")
-			return nil, fmt.Errorf("Invalid IP format: %s", ip)
-		}
-
+	for _, ip := range ipList {
 		var version string
 		var mask net.IPMask
 
-		if ipAddr.To4() != nil {
+		if ip.To4() != nil {
 			version = "4"
 			mask = net.CIDRMask(32, 32)
+
 		} else {
 			version = "6"
 			mask = net.CIDRMask(128, 128)
@@ -513,17 +495,64 @@ func overrideIPAMResult(ipAddrsNoIpam string, logger *log.Entry) (*current.Resul
 		ipConf := &current.IPConfig{
 			Version: version,
 			Address: net.IPNet{
-				IP:   ipAddr,
+				IP:   ip,
 				Mask: mask,
 			},
 		}
 		result.IPs = append(result.IPs, ipConf)
-		logger.Debugf("Adding IPv%s: %s to result", ipConf.Version, ipAddr.String())
+		logger.Debugf("Adding IPv%s: %s to result", ipConf.Version, ip.String())
 	}
 
 	return &result, nil
 }
 
+// validateAndExtractIPs is a utility function that validates the passed IP list to make sure
+// there is one IPv4 and/or one IPv6 and then returns the slice of IPs.
+func validateAndExtractIPs(ipAddrs string, annotation string, logger *log.Entry) ([]net.IP, error) {
+	// Parse IPs from JSON.
+	ips, err := parseIPAddrs(ipAddrs, logger)
+	if err != nil {
+		return nil, fmt.Errorf("Failed to parse IPs %s for annotation \"%s\": %s", ipAddrs, annotation, err)
+	}
+
+	// annotation value can't be empty.
+	if len(ips) == 0 {
+		return nil, fmt.Errorf("Annotation \"%s\" specified but empty", annotation)
+	}
+
+	var hasIPv4, hasIPv6 bool
+	var ipList []net.IP
+
+	// We need to make sure there is only one IPv4 and/or one IPv6
+	// passed in, since CNI spec only supports one of each right now.
+	for _, ip := range ips {
+		ipAddr := net.ParseIP(ip)
+		if ipAddr == nil {
+			logger.WithField("IP", ip).Error("Invalid IP format")
+			return nil, fmt.Errorf("Invalid IP format: %s", ip)
+		}
+
+		if ipAddr.To4() != nil {
+			if hasIPv4 {
+				// Check if there is already has been an IPv4 in the list, as we only support one IPv4 and/or one IPv6 per interface for now.
+				return nil, fmt.Errorf("cannot have more than one IPv4 address for \"%s\" annotation", annotation)
+			}
+			hasIPv4 = true
+		} else {
+			if hasIPv6 {
+				// Check if there is already has been an IPv6 in the list, as we only support one IPv4 and/or one IPv6 per interface for now.
+				return nil, fmt.Errorf("cannot have more than one IPv6 address for \"%s\" annotation", annotation)
+			}
+			hasIPv6 = true
+		}
+
+		// Append the IP to ipList slice.
+		ipList = append(ipList, ipAddr)
+	}
+
+	return ipList, nil
+}
+
 // parseIPAddrs is a utility function that parses string of IPs in json format that are
 // passed in as a string and returns a slice of string with IPs.
 // It also makes sure the slice isn't empty.

diff --git a/utils/utils.go b/utils/utils.go
@@ -98,7 +98,7 @@ func CleanUpIPAM(conf NetConf, args *skel.CmdArgs, logger *log.Entry) error {
 		if err != nil {
 			return err
 		}
-		logger.WithField("stdin", args.StdinData).Debug("Updated stdin data for Delete Cmd")
+		logger.WithField("stdin", string(args.StdinData)).Debug("Updated stdin data for Delete Cmd")
 	}
 
 	err := ipam.ExecDel(conf.IPAM.Type, args.StdinData)