Merge pull request #168 from steveeJ/chain-len

attempt to ensure chain name is unique and at most 29 chars long

pkg/ip/ipmasq.go

@@ -23,7 +23,7 @@ import (
 
 // SetupIPMasq installs iptables rules to masquerade traffic
 // coming from ipn and going outside of it
-func SetupIPMasq(ipn *net.IPNet, chain string) error {
+func SetupIPMasq(ipn *net.IPNet, chain string, comment string) error {
 	ipt, err := iptables.New()
 	if err != nil {
 		return fmt.Errorf("failed to locate iptables: %v", err)
@@ -36,25 +36,25 @@ func SetupIPMasq(ipn *net.IPNet, chain string) error {
 		}
 	}
 
-	if err = ipt.AppendUnique("nat", chain, "-d", ipn.String(), "-j", "ACCEPT"); err != nil {
+	if err = ipt.AppendUnique("nat", chain, "-d", ipn.String(), "-j", "ACCEPT", "-m", "comment", "--comment", comment); err != nil {
 		return err
 	}
 
-	if err = ipt.AppendUnique("nat", chain, "!", "-d", "224.0.0.0/4", "-j", "MASQUERADE"); err != nil {
+	if err = ipt.AppendUnique("nat", chain, "!", "-d", "224.0.0.0/4", "-j", "MASQUERADE", "-m", "comment", "--comment", comment); err != nil {
 		return err
 	}
 
-	return ipt.AppendUnique("nat", "POSTROUTING", "-s", ipn.String(), "-j", chain)
+	return ipt.AppendUnique("nat", "POSTROUTING", "-s", ipn.String(), "-j", chain, "-m", "comment", "--comment", comment)
 }
 
 // TeardownIPMasq undoes the effects of SetupIPMasq
-func TeardownIPMasq(ipn *net.IPNet, chain string) error {
+func TeardownIPMasq(ipn *net.IPNet, chain string, comment string) error {
 	ipt, err := iptables.New()
 	if err != nil {
 		return fmt.Errorf("failed to locate iptables: %v", err)
 	}
 
-	if err = ipt.Delete("nat", "POSTROUTING", "-s", ipn.String(), "-j", chain); err != nil {
+	if err = ipt.Delete("nat", "POSTROUTING", "-s", ipn.String(), "-j", chain, "-m", "comment", "--comment", comment); err != nil {
 		return err
 	}
 

pkg/utils/utils.go

@@ -0,0 +1,27 @@
+package utils
+
+import (
+	"crypto/sha512"
+	"fmt"
+)
+
+const (
+	maxChainLength = 28
+	chainPrefix    = "CNI-"
+	prefixLength   = len(chainPrefix)
+)
+
+// Generates a chain name to be used with iptables.
+// Ensures that the generated chain name is exactly
+// maxChainLength chars in length
+func FormatChainName(name string, id string) string {
+	chainBytes := sha512.Sum512([]byte(name + id))
+	chain := fmt.Sprintf("%s%x", chainPrefix, chainBytes)
+	return chain[:maxChainLength]
+}
+
+// FormatComment returns a comment used for easier
+// rule identification within iptables.
+func FormatComment(name string, id string) string {
+	return fmt.Sprintf("name: %q id: %q", name, id)
+}

pkg/utils/utils_suite_test.go

@@ -0,0 +1,13 @@
+package utils_test
+
+import (
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+
+	"testing"
+)
+
+func TestUtils(t *testing.T) {
+	RegisterFailHandler(Fail)
+	RunSpecs(t, "Utils Suite")
+}

pkg/utils/utils_test.go

@@ -0,0 +1,37 @@
+package utils
+
+import (
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+)
+
+var _ = Describe("Utils", func() {
+	It("must format a short name", func() {
+		chain := FormatChainName("test", "1234")
+		Expect(len(chain)).To(Equal(maxChainLength))
+		Expect(chain).To(Equal("CNI-2bbe0c48b91a7d1b8a6753a8"))
+	})
+
+	It("must truncate a long name", func() {
+		chain := FormatChainName("testalongnamethatdoesnotmakesense", "1234")
+		Expect(len(chain)).To(Equal(maxChainLength))
+		Expect(chain).To(Equal("CNI-374f33fe84ab0ed84dcdebe3"))
+	})
+
+	It("must be predictable", func() {
+		chain1 := FormatChainName("testalongnamethatdoesnotmakesense", "1234")
+		chain2 := FormatChainName("testalongnamethatdoesnotmakesense", "1234")
+		Expect(len(chain1)).To(Equal(maxChainLength))
+		Expect(len(chain2)).To(Equal(maxChainLength))
+		Expect(chain1).To(Equal(chain2))
+	})
+
+	It("must change when a character changes", func() {
+		chain1 := FormatChainName("testalongnamethatdoesnotmakesense", "1234")
+		chain2 := FormatChainName("testalongnamethatdoesnotmakesense", "1235")
+		Expect(len(chain1)).To(Equal(maxChainLength))
+		Expect(len(chain2)).To(Equal(maxChainLength))
+		Expect(chain1).To(Equal("CNI-374f33fe84ab0ed84dcdebe3"))
+		Expect(chain1).NotTo(Equal(chain2))
+	})
+})

plugins/main/bridge/bridge.go

@@ -28,6 +28,7 @@ import (
 	"github.com/appc/cni/pkg/ns"
 	"github.com/appc/cni/pkg/skel"
 	"github.com/appc/cni/pkg/types"
+	"github.com/appc/cni/pkg/utils"
 	"github.com/vishvananda/netlink"
 )
 
@@ -220,8 +221,9 @@ func cmdAdd(args *skel.CmdArgs) error {
 	}
 
 	if n.IPMasq {
-		chain := "CNI-" + n.Name
+		chain := utils.FormatChainName(n.Name, args.ContainerID)
-		if err = ip.SetupIPMasq(ip.Network(&result.IP4.IP), chain); err != nil {
+		comment := utils.FormatComment(n.Name, args.ContainerID)
+		if err = ip.SetupIPMasq(ip.Network(&result.IP4.IP), chain, comment); err != nil {
 			return err
 		}
 	}

plugins/main/ptp/ptp.go

@@ -15,7 +15,6 @@
 package main
 
 import (
-	"crypto/sha512"
 	"encoding/json"
 	"errors"
 	"fmt"
@@ -30,6 +29,7 @@ import (
 	"github.com/appc/cni/pkg/ns"
 	"github.com/appc/cni/pkg/skel"
 	"github.com/appc/cni/pkg/types"
+	"github.com/appc/cni/pkg/utils"
 )
 
 func init() {
@@ -178,9 +178,9 @@ func cmdAdd(args *skel.CmdArgs) error {
 	}
 
 	if conf.IPMasq {
-		h := sha512.Sum512([]byte(args.ContainerID))
+		chain := utils.FormatChainName(conf.Name, args.ContainerID)
-		chain := fmt.Sprintf("CNI-%s-%x", conf.Name, h[:8])
+		comment := utils.FormatComment(conf.Name, args.ContainerID)
-		if err = ip.SetupIPMasq(&result.IP4.IP, chain); err != nil {
+		if err = ip.SetupIPMasq(&result.IP4.IP, chain, comment); err != nil {
 			return err
 		}
 	}
@@ -206,9 +206,9 @@ func cmdDel(args *skel.CmdArgs) error {
 	}
 
 	if conf.IPMasq {
-		h := sha512.Sum512([]byte(args.ContainerID))
+		chain := utils.FormatChainName(conf.Name, args.ContainerID)
-		chain := fmt.Sprintf("CNI-%s-%x", conf.Name, h[:8])
+		comment := utils.FormatComment(conf.Name, args.ContainerID)
-		if err = ip.TeardownIPMasq(ipn, chain); err != nil {
+		if err = ip.TeardownIPMasq(ipn, chain, comment); err != nil {
 			return err
 		}
 	}

test

@@ -11,7 +11,7 @@ set -e
 
 source ./build
 
-TESTABLE="plugins/ipam/dhcp plugins/main/loopback pkg/invoke pkg/ns pkg/skel pkg/types"
+TESTABLE="plugins/ipam/dhcp plugins/main/loopback pkg/invoke pkg/ns pkg/skel pkg/types pkg/utils"
 FORMATTABLE="$TESTABLE libcni pkg/ip pkg/ns pkg/types pkg/ipam plugins/ipam/host-local plugins/main/bridge plugins/meta/flannel plugins/meta/tuning"
 
 # user has not provided PKG override