Merge pull request #406 from gunjan5/route-2

bugfix: CNI plugin throws file exists while programming container route

.gitignore

@@ -14,3 +14,4 @@ tmp-cni
 k8s-install/scripts/tmp/
 install_cni.test
 *.swp
+*.coverprofile

Makefile

@@ -140,7 +140,7 @@ test-containerized: run-etcd run-k8s-apiserver build-containerized dist/host-loc
 	-v $(CURDIR):/go/src/github.com/projectcalico/cni-plugin:rw \
 	$(CALICO_BUILD) sh -c '\
 			cd  /go/src/github.com/projectcalico/cni-plugin && \
-			ginkgo'
+			make run-tests'
 	make stop-etcd
 
 # This does not currently work, kubernetes needs additional configuration
@@ -179,11 +179,11 @@ run-test-containerized-without-building: run-etcd run-k8s-apiserver
 	-v $(CURDIR):/go/src/github.com/projectcalico/cni-plugin:rw \
 	$(CALICO_BUILD) sh -c '\
 			cd  /go/src/github.com/projectcalico/cni-plugin && \
-			ginkgo'
+			make run-tests'
 	make stop-etcd
 
 ## Run the tests in a container (as root) for different CNI spec versions
-## to make sure we don't break backwards compatiblity.
+## to make sure we don't break backwards compatibility.
 .PHONY: test-containerized-cni-versions
 test-containerized-cni-versions: build-containerized dist/host-local;
 	for cniversion in "0.2.0" "0.3.1" ; do \
@@ -204,6 +204,26 @@ build-containerized: vendor
 			cd /go/src/github.com/projectcalico/cni-plugin && \
 			make binary'
 
+.PHONY: run-tests
+## Run the tests locally, must have local etcd running
+run-tests:
+	# Run tests recursively (-r).
+	ginkgo -cover -r -skipPackage vendor -skipPackage k8s-install
+
+	@echo
+	@echo '+==============+'
+	@echo '| All coverage |'
+	@echo '+==============+'
+	@echo
+	@find . -iname '*.coverprofile' | xargs -I _ go tool cover -func=_
+
+	@echo
+	@echo '+==================+'
+	@echo '| Missing coverage |'
+	@echo '+==================+'
+	@echo
+	@find . -iname '*.coverprofile' | xargs -I _ go tool cover -func=_ | grep -v '100.0%'
+
 ## Etcd is used by the tests
 run-etcd: stop-etcd
 	docker run --detach \

calico_cni_test.go

@@ -505,4 +505,48 @@ var _ = Describe("CalicoCni", func() {
 			})
 		})
 	})
+
+	Describe("SetupRoutes works fine when the route is already programmed", func() {
+		Context("container route already exists on the host", func() {
+			netconf := fmt.Sprintf(`
+			{
+			  "cniVersion": "%s",
+			  "name": "net1",
+			  "type": "calico",
+			  "etcd_endpoints": "http://%s:2379",
+			  "datastore_type": "%s",
+			  "ipam": {
+			    "type": "host-local",
+			    "subnet": "10.0.0.0/8"
+			  },
+			  "log_level":"debug"
+			}`, cniVersion, os.Getenv("ETCD_IP"), os.Getenv("DATASTORE_TYPE"))
+
+			It("route setup should be resilient to existing route", func() {
+				By("creating a CNI networked container, which should also install the container route in the host namespace")
+				containerID, session, _, _, _, contNs, err := testutils.CreateContainerWithId(netconf, "", testutils.TEST_DEFAULT_NS, "", "meep1337")
+				Expect(err).ShouldNot(HaveOccurred())
+				Eventually(session).Should(gexec.Exit())
+
+				// CNI plugin generates host side vEth name from containerID if used for "cni" orchestrator.
+				hostVethName := "cali" + containerID[:utils.Min(11, len(containerID))] //"cali" + containerID
+				hostVeth, err := netlink.LinkByName(hostVethName)
+				Expect(err).ToNot(HaveOccurred())
+
+				result, err := testutils.GetResultForCurrent(session, cniVersion)
+				if err != nil {
+					log.Fatalf("Error getting result from the session: %v\n", err)
+				}
+
+				log.Printf("Unmarshalled result: %v\n", result)
+
+				By("setting up the same route CNI plugin installed in the initial run for the hostVeth")
+				err = utils.SetupRoutes(hostVeth, result)
+				Expect(err).NotTo(HaveOccurred())
+
+				_, err = testutils.DeleteContainerWithId(netconf, contNs.Path(), "", testutils.TEST_DEFAULT_NS, containerID)
+				Expect(err).ShouldNot(HaveOccurred())
+			})
+		})
+	})
 })

utils/network.go

@@ -5,7 +5,7 @@ import (
 	"io"
 	"net"
 	"os"
-	"reflect"
+	"syscall"
 
 	"github.com/containernetworking/cni/pkg/skel"
 	"github.com/containernetworking/cni/pkg/types/current"
@@ -194,18 +194,16 @@ func DoNetworking(args *skel.CmdArgs, conf types.NetConf, result *current.Result
 	}
 
 	// Now that the host side of the veth is moved, state set to UP, and configured with sysctls, we can add the routes to it in the host namespace.
-	err = setupRoutes(hostVeth, result)
+	err = SetupRoutes(hostVeth, result)
 	if err != nil {
 		return "", "", fmt.Errorf("error adding host side routes for interface: %s, error: %s", hostVeth.Attrs().Name, err)
 	}
 
 	return hostVethName, contVethMAC, err
 }
 
-var errFileExists = fmt.Errorf("file exists")
-
-// setupRoutes sets up the routes for the host side of the veth pair.
-func setupRoutes(hostVeth netlink.Link, result *current.Result) error {
+// SetupRoutes sets up the routes for the host side of the veth pair.
+func SetupRoutes(hostVeth netlink.Link, result *current.Result) error {
 
 	// Go through all the IPs and add routes for each IP in the result.
 	for _, ipAddr := range result.IPs {
@@ -220,7 +218,7 @@ func setupRoutes(hostVeth netlink.Link, result *current.Result) error {
 			switch err {
 
 			// Route already exists, but not necessarily pointing to the same interface.
-			case errFileExists:
+			case syscall.EEXIST:
 				// List all the routes for the interface.
 				routes, err := netlink.RouteList(hostVeth, netlink.FAMILY_ALL)
 				if err != nil {
@@ -231,22 +229,24 @@ func setupRoutes(hostVeth netlink.Link, result *current.Result) error {
 				// exactly what we are intending to program.
 				// If the route we want is already there then most likely it's programmed by Felix, so we ignore it,
 				// and we return an error if none of the routes match the route we're trying to program.
+				logrus.WithFields(logrus.Fields{"route": route, "scope": route.Scope}).Debug("Constructed route")
 				for _, r := range routes {
-					if reflect.DeepEqual(r, route) {
+					logrus.WithFields(logrus.Fields{"interface": hostVeth.Attrs().Name, "route": r, "scope": r.Scope}).Debug("Routes for the interface")
+					if r.LinkIndex == route.LinkIndex && r.Dst.IP.Equal(route.Dst.IP) && r.Scope == route.Scope {
 						// Route was already present on the host.
-						logrus.Infof("CNI skipping add route. Route already exists for %s\n", hostVeth.Attrs().Name)
+						logrus.WithFields(logrus.Fields{"interface": hostVeth.Attrs().Name}).Infof("CNI skipping add route. Route already exists")
 						return nil
 					}
 				}
-				return fmt.Errorf("route (Dst: %s, Scope: %v) already exists for an interface other than '%s'",
-					route.Dst.String(), route.Scope, hostVeth.Attrs().Name)
+				return fmt.Errorf("route (Ifindex: %d, Dst: %s, Scope: %v) already exists for an interface other than '%s'",
+					route.LinkIndex, route.Dst.String(), route.Scope, hostVeth.Attrs().Name)
 			default:
-				return fmt.Errorf("failed to add route (Dst: %s, Scope: %v, Iface: %s): %v",
-					route.Dst.String(), route.Scope, hostVeth.Attrs().Name, err)
+				return fmt.Errorf("failed to add route (Ifindex: %d, Dst: %s, Scope: %v, Iface: %s): %v",
+					route.LinkIndex, route.Dst.String(), route.Scope, hostVeth.Attrs().Name, err)
 			}
 		}
 
-		logrus.Debugf("CNI adding route for interface: %v, IP: %s", hostVeth, ipAddr.Address)
+		logrus.WithFields(logrus.Fields{"interface": hostVeth, "IP": ipAddr.Address}).Debugf("CNI adding route")
 	}
 	return nil
 }