Merge pull request #353 from cf-container-networking/fix-hash-func

Increase IfbDeviceName size and refactor hashing functions, fix

pkg/utils/utils.go

@@ -22,16 +22,22 @@ import (
 const (
 	maxChainLength = 28
 	chainPrefix    = "CNI-"
-	prefixLength   = len(chainPrefix)
 )
 
-// Generates a chain name to be used with iptables.
+// FormatChainName generates a chain name to be used
-// Ensures that the generated chain name is exactly
+// with iptables. Ensures that the generated chain
-// maxChainLength chars in length
+// name is exactly maxChainLength chars in length.
 func FormatChainName(name string, id string) string {
-	chainBytes := sha512.Sum512([]byte(name + id))
+	return MustFormatChainNameWithPrefix(name, id, "")
-	chain := fmt.Sprintf("%s%x", chainPrefix, chainBytes)
+}
-	return chain[:maxChainLength]
+
+// MustFormatChainNameWithPrefix generates a chain name similar
+// to FormatChainName, but adds a custom prefix between
+// chainPrefix and unique identifier. Ensures that the
+// generated chain name is exactly maxChainLength chars in length.
+// Panics if the given prefix is too long.
+func MustFormatChainNameWithPrefix(name string, id string, prefix string) string {
+	return MustFormatHashWithPrefix(maxChainLength, chainPrefix+prefix, name+id)
 }
 
 // FormatComment returns a comment used for easier
@@ -39,3 +45,16 @@ func FormatChainName(name string, id string) string {
 func FormatComment(name string, id string) string {
 	return fmt.Sprintf("name: %q id: %q", name, id)
 }
+
+const MaxHashLen = sha512.Size * 2
+
+// MustFormatHashWithPrefix returns a string of given length that begins with the
+// given prefix. It is filled with entropy based on the given string toHash.
+func MustFormatHashWithPrefix(length int, prefix string, toHash string) string {
+	if len(prefix) >= length || length > MaxHashLen {
+		panic("invalid length")
+	}
+
+	output := sha512.Sum512([]byte(toHash))
+	return fmt.Sprintf("%s%x", prefix, output)[:length]
+}

pkg/utils/utils_test.go

@@ -15,37 +15,151 @@
 package utils
 
 import (
+	"fmt"
+	"strings"
+
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 )
 
 var _ = Describe("Utils", func() {
-	It("must format a short name", func() {
+	Describe("FormatChainName", func() {
-		chain := FormatChainName("test", "1234")
+		It("must format a short name", func() {
-		Expect(len(chain)).To(Equal(maxChainLength))
+			chain := FormatChainName("test", "1234")
-		Expect(chain).To(Equal("CNI-2bbe0c48b91a7d1b8a6753a8"))
+			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))
+		})
 	})
 
-	It("must truncate a long name", func() {
+	Describe("MustFormatChainNameWithPrefix", func() {
-		chain := FormatChainName("testalongnamethatdoesnotmakesense", "1234")
+		It("generates a chain name with a prefix", func() {
-		Expect(len(chain)).To(Equal(maxChainLength))
+			chain := MustFormatChainNameWithPrefix("test", "1234", "PREFIX-")
-		Expect(chain).To(Equal("CNI-374f33fe84ab0ed84dcdebe3"))
+			Expect(len(chain)).To(Equal(maxChainLength))
+			Expect(chain).To(Equal("CNI-PREFIX-2bbe0c48b91a7d1b8"))
+		})
+
+		It("must format a short name", func() {
+			chain := MustFormatChainNameWithPrefix("test", "1234", "PREFIX-")
+			Expect(len(chain)).To(Equal(maxChainLength))
+			Expect(chain).To(Equal("CNI-PREFIX-2bbe0c48b91a7d1b8"))
+		})
+
+		It("must truncate a long name", func() {
+			chain := MustFormatChainNameWithPrefix("testalongnamethatdoesnotmakesense", "1234", "PREFIX-")
+			Expect(len(chain)).To(Equal(maxChainLength))
+			Expect(chain).To(Equal("CNI-PREFIX-374f33fe84ab0ed84"))
+		})
+
+		It("must be predictable", func() {
+			chain1 := MustFormatChainNameWithPrefix("testalongnamethatdoesnotmakesense", "1234", "PREFIX-")
+			chain2 := MustFormatChainNameWithPrefix("testalongnamethatdoesnotmakesense", "1234", "PREFIX-")
+			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 := MustFormatChainNameWithPrefix("testalongnamethatdoesnotmakesense", "1234", "PREFIX-")
+			chain2 := MustFormatChainNameWithPrefix("testalongnamethatdoesnotmakesense", "1235", "PREFIX-")
+			Expect(len(chain1)).To(Equal(maxChainLength))
+			Expect(len(chain2)).To(Equal(maxChainLength))
+			Expect(chain1).To(Equal("CNI-PREFIX-374f33fe84ab0ed84"))
+			Expect(chain1).NotTo(Equal(chain2))
+		})
+
+		It("panics when prefix is too large", func() {
+			longPrefix := strings.Repeat("PREFIX-", 4)
+			Expect(func() {
+				MustFormatChainNameWithPrefix("test", "1234", longPrefix)
+			}).To(Panic())
+		})
 	})
 
-	It("must be predictable", func() {
+	Describe("MustFormatHashWithPrefix", func() {
-		chain1 := FormatChainName("testalongnamethatdoesnotmakesense", "1234")
+		It("always returns a string with the given prefix", func() {
-		chain2 := FormatChainName("testalongnamethatdoesnotmakesense", "1234")
+			Expect(MustFormatHashWithPrefix(10, "AAA", "some string")).To(HavePrefix("AAA"))
-		Expect(len(chain1)).To(Equal(maxChainLength))
+			Expect(MustFormatHashWithPrefix(10, "foo", "some string")).To(HavePrefix("foo"))
-		Expect(len(chain2)).To(Equal(maxChainLength))
+			Expect(MustFormatHashWithPrefix(10, "bar", "some string")).To(HavePrefix("bar"))
-		Expect(chain1).To(Equal(chain2))
+		})
+
+		It("always returns a string of the given length", func() {
+			Expect(MustFormatHashWithPrefix(10, "AAA", "some string")).To(HaveLen(10))
+			Expect(MustFormatHashWithPrefix(15, "AAA", "some string")).To(HaveLen(15))
+			Expect(MustFormatHashWithPrefix(5, "AAA", "some string")).To(HaveLen(5))
+		})
+
+		It("is deterministic", func() {
+			val1 := MustFormatHashWithPrefix(10, "AAA", "some string")
+			val2 := MustFormatHashWithPrefix(10, "AAA", "some string")
+			val3 := MustFormatHashWithPrefix(10, "AAA", "some string")
+			Expect(val1).To(Equal(val2))
+			Expect(val1).To(Equal(val3))
+		})
+
+		It("is (nearly) perfect (injective function)", func() {
+			hashes := map[string]int{}
+
+			for i := 0; i < 1000; i++ {
+				name := fmt.Sprintf("string %d", i)
+				hashes[MustFormatHashWithPrefix(8, "", name)]++
+			}
+
+			for key, count := range hashes {
+				Expect(count).To(Equal(1), "for key "+key+" got non-unique correspondence")
+			}
+		})
+
+		assertPanicWith := func(f func(), expectedErrorMessage string) {
+			defer func() {
+				Expect(recover()).To(Equal(expectedErrorMessage))
+			}()
+			f()
+			Fail("function should have panicked but did not")
+		}
+
+		It("panics when prefix is longer than the length", func() {
+			assertPanicWith(
+				func() { MustFormatHashWithPrefix(3, "AAA", "some string") },
+				"invalid length",
+			)
+		})
+
+		It("panics when length is not positive", func() {
+			assertPanicWith(
+				func() { MustFormatHashWithPrefix(0, "", "some string") },
+				"invalid length",
+			)
+		})
+
+		It("panics when length is larger than MaxLen", func() {
+			assertPanicWith(
+				func() { MustFormatHashWithPrefix(MaxHashLen+1, "", "some string") },
+				"invalid length",
+			)
+		})
 	})
 
-	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/meta/bandwidth/bandwidth_linux_test.go

@@ -105,7 +105,7 @@ var _ = Describe("bandwidth test", func() {
 		hostIP = net.IP{169, 254, 0, 1}
 		containerIP = net.IP{10, 254, 0, 1}
 		hostIfaceMTU = 1024
-		ifbDeviceName = "5b6c"
+		ifbDeviceName = "bwpa8eda89404b7"
 
 		createVeth(hostNs.Path(), hostIfname, containerNs.Path(), containerIfname, hostIP, containerIP, hostIfaceMTU)
 	})

plugins/meta/bandwidth/main.go

@@ -15,7 +15,6 @@
 package main
 
 import (
-	"crypto/sha1"
 	"encoding/json"
 	"fmt"
 
@@ -28,9 +27,13 @@ import (
 
 	"github.com/containernetworking/plugins/pkg/ip"
 	"github.com/containernetworking/plugins/pkg/ns"
+	"github.com/containernetworking/plugins/pkg/utils"
 	bv "github.com/containernetworking/plugins/pkg/utils/buildversion"
 )
 
+const maxIfbDeviceLength = 15
+const ifbDevicePrefix = "bwp"
+
 // BandwidthEntry corresponds to a single entry in the bandwidth argument,
 // see CONVENTIONS.md
 type BandwidthEntry struct {
@@ -111,14 +114,8 @@ func validateRateAndBurst(rate int, burst int) error {
 	return nil
 }
 
-func getIfbDeviceName(networkName string, containerId string) (string, error) {
+func getIfbDeviceName(networkName string, containerId string) string {
-	hash := sha1.New()
+	return utils.MustFormatHashWithPrefix(maxIfbDeviceLength, ifbDevicePrefix, networkName+containerId)
-	_, err := hash.Write([]byte(networkName + containerId))
-	if err != nil {
-		return "", err
-	}
-
-	return fmt.Sprintf("%x", hash.Sum(nil))[:4], nil
 }
 
 func getMTU(deviceName string) (int, error) {
@@ -205,10 +202,7 @@ func cmdAdd(args *skel.CmdArgs) error {
 			return err
 		}
 
-		ifbDeviceName, err := getIfbDeviceName(conf.Name, args.ContainerID)
+		ifbDeviceName := getIfbDeviceName(conf.Name, args.ContainerID)
-		if err != nil {
-			return err
-		}
 
 		err = CreateIfb(ifbDeviceName, mtu)
 		if err != nil {
@@ -239,10 +233,7 @@ func cmdDel(args *skel.CmdArgs) error {
 		return err
 	}
 
-	ifbDeviceName, err := getIfbDeviceName(conf.Name, args.ContainerID)
+	ifbDeviceName := getIfbDeviceName(conf.Name, args.ContainerID)
-	if err != nil {
-		return err
-	}
 
 	if err := TeardownIfb(ifbDeviceName); err != nil {
 		return err
@@ -343,10 +334,7 @@ func cmdCheck(args *skel.CmdArgs) error {
 		latency := latencyInUsec(latencyInMillis)
 		limitInBytes := limit(uint64(rateInBytes), latency, uint32(burstInBytes))
 
-		ifbDeviceName, err := getIfbDeviceName(bwConf.Name, args.ContainerID)
+		ifbDeviceName := getIfbDeviceName(bwConf.Name, args.ContainerID)
-		if err != nil {
-			return err
-		}
 
 		ifbDevice, err := netlink.LinkByName(ifbDeviceName)
 		if err != nil {

plugins/meta/portmap/portmap.go

@@ -20,6 +20,7 @@ import (
 	"sort"
 	"strconv"
 
+	"github.com/containernetworking/plugins/pkg/utils"
 	"github.com/containernetworking/plugins/pkg/utils/sysctl"
 	"github.com/coreos/go-iptables/iptables"
 )
@@ -172,7 +173,7 @@ func genToplevelDnatChain() chain {
 func genDnatChain(netName, containerID string) chain {
 	return chain{
 		table:       "nat",
-		name:        formatChainName("DN-", netName, containerID),
+		name:        utils.MustFormatChainNameWithPrefix(netName, containerID, "DN-"),
 		entryChains: []string{TopLevelDNATChainName},
 	}
 }
@@ -323,11 +324,9 @@ func enableLocalnetRouting(ifName string) error {
 // genOldSnatChain is no longer used, but used to be created. We'll try and
 // tear it down in case the plugin version changed between ADD and DEL
 func genOldSnatChain(netName, containerID string) chain {
-	name := formatChainName("SN-", netName, containerID)
-
 	return chain{
 		table:       "nat",
-		name:        name,
+		name:        utils.MustFormatChainNameWithPrefix(netName, containerID, "SN-"),
 		entryChains: []string{OldTopLevelSNATChainName},
 	}
 }

plugins/meta/portmap/utils.go

@@ -15,7 +15,6 @@
 package main
 
 import (
-	"crypto/sha512"
 	"fmt"
 	"net"
 	"strconv"
@@ -24,8 +23,6 @@ import (
 	"github.com/vishvananda/netlink"
 )
 
-const maxChainNameLength = 28
-
 // fmtIpPort correctly formats ip:port literals for iptables and ip6tables -
 // need to wrap v6 literals in a []
 func fmtIpPort(ip net.IP, port int) string {
@@ -62,12 +59,6 @@ func getRoutableHostIF(containerIP net.IP) string {
 	return ""
 }
 
-func formatChainName(prefix, name, id string) string {
-	chainBytes := sha512.Sum512([]byte(name + id))
-	chain := fmt.Sprintf("CNI-%s%x", prefix, chainBytes)
-	return chain[:maxChainNameLength]
-}
-
 // groupByProto groups port numbers by protocol
 func groupByProto(entries []PortMapEntry) map[string][]int {
 	if len(entries) == 0 {