From c33daf67063f5997f9e28aa1395c41f612c63eac Mon Sep 17 00:00:00 2001 From: Abhishek Chanda Date: Tue, 22 Mar 2016 17:16:59 -0700 Subject: [PATCH 1/9] pkg: add a function to generate chain names Adds a function to generate chain names for use in iptables and ports all drivers to use that function. Also adds tests for the said function. --- pkg/utils/utils.go | 20 ++++++++++++++++++++ pkg/utils/utils_suite_test.go | 13 +++++++++++++ pkg/utils/utils_test.go | 18 ++++++++++++++++++ plugins/main/bridge/bridge.go | 3 ++- plugins/main/ptp/ptp.go | 8 +++----- test | 2 +- 6 files changed, 57 insertions(+), 7 deletions(-) create mode 100644 pkg/utils/utils.go create mode 100644 pkg/utils/utils_suite_test.go create mode 100644 pkg/utils/utils_test.go diff --git a/pkg/utils/utils.go b/pkg/utils/utils.go new file mode 100644 index 00000000..eaf48d0b --- /dev/null +++ b/pkg/utils/utils.go @@ -0,0 +1,20 @@ +package utils + +import ( + "crypto/sha512" + "fmt" +) + +// 29 - len('CNI') - 2*len('-') +const maxNameLen = 16 + +// Generates a chain name to be used with iptables. +// Ensures that the generated name is less than +// 29 chars in length +func FormatChainName(name string, id string) string { + h := sha512.Sum512([]byte(id)) + if len(name) > maxNameLen { + return fmt.Sprintf("CNI-%s-%x", name[:len(name)-maxNameLen], h[:8]) + } + return fmt.Sprintf("CNI-%s-%x", name, h[:8]) +} diff --git a/pkg/utils/utils_suite_test.go b/pkg/utils/utils_suite_test.go new file mode 100644 index 00000000..f160db60 --- /dev/null +++ b/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") +} diff --git a/pkg/utils/utils_test.go b/pkg/utils/utils_test.go new file mode 100644 index 00000000..e9b9f9bf --- /dev/null +++ b/pkg/utils/utils_test.go @@ -0,0 +1,18 @@ +package utils + +import ( + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" +) + +var _ = Describe("Utils", func() { + It("should format a short name", func() { + chain := FormatChainName("test", "1234") + Expect(chain).To(Equal("CNI-test-d404559f602eab6f")) + }) + + It("should truncate a long name", func() { + chain := FormatChainName("testalongnamethatdoesnotmakesense", "1234") + Expect(chain).To(Equal("CNI-testalongnamethat-d404559f602eab6f")) + }) +}) diff --git a/plugins/main/bridge/bridge.go b/plugins/main/bridge/bridge.go index 49c0aa5d..d5581bf4 100644 --- a/plugins/main/bridge/bridge.go +++ b/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,7 +221,7 @@ 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 { return err } diff --git a/plugins/main/ptp/ptp.go b/plugins/main/ptp/ptp.go index 3cb8f643..b397b795 100644 --- a/plugins/main/ptp/ptp.go +++ b/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,8 +178,7 @@ func cmdAdd(args *skel.CmdArgs) error { } if conf.IPMasq { - h := sha512.Sum512([]byte(args.ContainerID)) - chain := fmt.Sprintf("CNI-%s-%x", conf.Name, h[:8]) + chain := utils.FormatChainName(conf.Name, args.ContainerID) if err = ip.SetupIPMasq(&result.IP4.IP, chain); err != nil { return err } @@ -206,8 +205,7 @@ func cmdDel(args *skel.CmdArgs) error { } if conf.IPMasq { - h := sha512.Sum512([]byte(args.ContainerID)) - chain := fmt.Sprintf("CNI-%s-%x", conf.Name, h[:8]) + chain := utils.FormatChainName(conf.Name, args.ContainerID) if err = ip.TeardownIPMasq(ipn, chain); err != nil { return err } diff --git a/test b/test index a333086f..a51a0e8f 100755 --- a/test +++ b/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 From 09248dfad9092a92370d8903d4161a850cb9dda3 Mon Sep 17 00:00:00 2001 From: Stefan Junker Date: Wed, 30 Mar 2016 19:17:37 +0200 Subject: [PATCH 2/9] pkg/utils: use name+id for hash and extend tests --- pkg/utils/utils.go | 11 +++++------ pkg/utils/utils_test.go | 29 +++++++++++++++++++++++++---- 2 files changed, 30 insertions(+), 10 deletions(-) diff --git a/pkg/utils/utils.go b/pkg/utils/utils.go index eaf48d0b..a07ccfab 100644 --- a/pkg/utils/utils.go +++ b/pkg/utils/utils.go @@ -5,16 +5,15 @@ import ( "fmt" ) -// 29 - len('CNI') - 2*len('-') -const maxNameLen = 16 +const MaxChainLength = 29 - len("CNI-") // Generates a chain name to be used with iptables. // Ensures that the generated name is less than // 29 chars in length func FormatChainName(name string, id string) string { - h := sha512.Sum512([]byte(id)) - if len(name) > maxNameLen { - return fmt.Sprintf("CNI-%s-%x", name[:len(name)-maxNameLen], h[:8]) + chain := fmt.Sprintf("%x", sha512.Sum512([]byte(name+id))) + if len(chain) > MaxChainLength { + chain = chain[:MaxChainLength] } - return fmt.Sprintf("CNI-%s-%x", name, h[:8]) + return fmt.Sprintf("CNI-%s", chain) } diff --git a/pkg/utils/utils_test.go b/pkg/utils/utils_test.go index e9b9f9bf..05fde8a9 100644 --- a/pkg/utils/utils_test.go +++ b/pkg/utils/utils_test.go @@ -6,13 +6,34 @@ import ( ) var _ = Describe("Utils", func() { - It("should format a short name", func() { + It("must format a short name", func() { chain := FormatChainName("test", "1234") - Expect(chain).To(Equal("CNI-test-d404559f602eab6f")) + Expect(len(chain) == 29).To(Equal(true)) + Expect(chain).To(Equal("CNI-2bbe0c48b91a7d1b8a6753a8b")) }) - It("should truncate a long name", func() { + It("must truncate a long name", func() { chain := FormatChainName("testalongnamethatdoesnotmakesense", "1234") - Expect(chain).To(Equal("CNI-testalongnamethat-d404559f602eab6f")) + Expect(len(chain) == 29).To(Equal(true)) + Expect(chain).To(Equal("CNI-374f33fe84ab0ed84dcdebe38")) }) + + It("must be predictable", func() { + chain1 := FormatChainName("testalongnamethatdoesnotmakesense", "1234") + chain2 := FormatChainName("testalongnamethatdoesnotmakesense", "1234") + Expect(len(chain1) == 29).To(Equal(true)) + Expect(len(chain2) == 29).To(Equal(true)) + Expect(chain1).To(Equal(chain2)) + }) + + It("must change when a character changes", func() { + chain1 := FormatChainName("testalongnamethatdoesnotmakesense", "1234") + chain2 := FormatChainName("testalongnamethatdoesnotmakesense", "1235") + Expect(len(chain1) == 29).To(Equal(true)) + Expect(len(chain2) == 29).To(Equal(true)) + Expect(chain1).To(Equal("CNI-374f33fe84ab0ed84dcdebe38")) + Expect(chain2).NotTo(Equal("CNI-374f33fe84ab0ed84dcdebe38")) + Expect(chain1).NotTo(Equal(chain2)) + }) + }) From 3e6069cab591b44348db715b3b99c4ba1c8e85af Mon Sep 17 00:00:00 2001 From: Stefan Junker Date: Wed, 30 Mar 2016 19:40:20 +0200 Subject: [PATCH 3/9] pkg/utils: use constant for chain prefix --- pkg/utils/utils.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pkg/utils/utils.go b/pkg/utils/utils.go index a07ccfab..e1468aaf 100644 --- a/pkg/utils/utils.go +++ b/pkg/utils/utils.go @@ -5,7 +5,8 @@ import ( "fmt" ) -const MaxChainLength = 29 - len("CNI-") +const ChainCNIPrefix = ("CNI-") +const MaxChainLength = 29 - len(ChainCNIPrefix) // Generates a chain name to be used with iptables. // Ensures that the generated name is less than @@ -15,5 +16,5 @@ func FormatChainName(name string, id string) string { if len(chain) > MaxChainLength { chain = chain[:MaxChainLength] } - return fmt.Sprintf("CNI-%s", chain) + return fmt.Sprintf("%s%s", ChainCNIPrefix, chain) } From bc44d1227d704e120a6e619c0adbe6298feda40b Mon Sep 17 00:00:00 2001 From: Stefan Junker Date: Wed, 30 Mar 2016 19:40:31 +0200 Subject: [PATCH 4/9] pkg/utils: fix docstring --- pkg/utils/utils.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/utils/utils.go b/pkg/utils/utils.go index e1468aaf..1566252d 100644 --- a/pkg/utils/utils.go +++ b/pkg/utils/utils.go @@ -9,7 +9,7 @@ const ChainCNIPrefix = ("CNI-") const MaxChainLength = 29 - len(ChainCNIPrefix) // Generates a chain name to be used with iptables. -// Ensures that the generated name is less than +// Ensures that the generated chain name is less than // 29 chars in length func FormatChainName(name string, id string) string { chain := fmt.Sprintf("%x", sha512.Sum512([]byte(name+id))) From 53d9cee00a16794bf750775597a147fc683a11cd Mon Sep 17 00:00:00 2001 From: Stefan Junker Date: Thu, 31 Mar 2016 10:08:52 +0200 Subject: [PATCH 5/9] pkg/utils: split and unexport constants --- pkg/utils/utils.go | 17 +++++++++-------- pkg/utils/utils_test.go | 13 ++++++------- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/pkg/utils/utils.go b/pkg/utils/utils.go index 1566252d..5877fb62 100644 --- a/pkg/utils/utils.go +++ b/pkg/utils/utils.go @@ -5,16 +5,17 @@ import ( "fmt" ) -const ChainCNIPrefix = ("CNI-") -const MaxChainLength = 29 - len(ChainCNIPrefix) +const ( + maxChainLength = 28 + chainPrefix = "CNI-" + prefixLength = len(chainPrefix) +) // Generates a chain name to be used with iptables. // Ensures that the generated chain name is less than -// 29 chars in length +// maxChainLength chars in length func FormatChainName(name string, id string) string { - chain := fmt.Sprintf("%x", sha512.Sum512([]byte(name+id))) - if len(chain) > MaxChainLength { - chain = chain[:MaxChainLength] - } - return fmt.Sprintf("%s%s", ChainCNIPrefix, chain) + chainBytes := sha512.Sum512([]byte(name + id)) + chain := fmt.Sprintf("%s%x", chainPrefix, chainBytes) + return chain[:maxChainLength] } diff --git a/pkg/utils/utils_test.go b/pkg/utils/utils_test.go index 05fde8a9..00e3b4f4 100644 --- a/pkg/utils/utils_test.go +++ b/pkg/utils/utils_test.go @@ -8,32 +8,31 @@ import ( var _ = Describe("Utils", func() { It("must format a short name", func() { chain := FormatChainName("test", "1234") - Expect(len(chain) == 29).To(Equal(true)) + Expect(len(chain)).To(Equal(maxChainLength)) Expect(chain).To(Equal("CNI-2bbe0c48b91a7d1b8a6753a8b")) }) It("must truncate a long name", func() { chain := FormatChainName("testalongnamethatdoesnotmakesense", "1234") - Expect(len(chain) == 29).To(Equal(true)) + Expect(len(chain)).To(Equal(maxChainLength)) Expect(chain).To(Equal("CNI-374f33fe84ab0ed84dcdebe38")) }) It("must be predictable", func() { chain1 := FormatChainName("testalongnamethatdoesnotmakesense", "1234") chain2 := FormatChainName("testalongnamethatdoesnotmakesense", "1234") - Expect(len(chain1) == 29).To(Equal(true)) - Expect(len(chain2) == 29).To(Equal(true)) + 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) == 29).To(Equal(true)) - Expect(len(chain2) == 29).To(Equal(true)) + Expect(len(chain1)).To(Equal(maxChainLength)) + Expect(len(chain2)).To(Equal(maxChainLength)) Expect(chain1).To(Equal("CNI-374f33fe84ab0ed84dcdebe38")) Expect(chain2).NotTo(Equal("CNI-374f33fe84ab0ed84dcdebe38")) Expect(chain1).NotTo(Equal(chain2)) }) - }) From 77759626a848d8a9fd3b4302fe97d043e31ef2e7 Mon Sep 17 00:00:00 2001 From: Stefan Junker Date: Thu, 31 Mar 2016 11:50:18 +0200 Subject: [PATCH 6/9] pkg/utils: fix docstring --- pkg/utils/utils.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/utils/utils.go b/pkg/utils/utils.go index 5877fb62..ea29c965 100644 --- a/pkg/utils/utils.go +++ b/pkg/utils/utils.go @@ -12,7 +12,7 @@ const ( ) // Generates a chain name to be used with iptables. -// Ensures that the generated chain name is less than +// 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)) From bcef17daac143f16a8a50c5ef07df224cc87d18f Mon Sep 17 00:00:00 2001 From: Stefan Junker Date: Thu, 31 Mar 2016 11:55:46 +0200 Subject: [PATCH 7/9] pkg/utils: remove unneeded condition in tests --- pkg/utils/utils_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/utils/utils_test.go b/pkg/utils/utils_test.go index 00e3b4f4..20a9fcd5 100644 --- a/pkg/utils/utils_test.go +++ b/pkg/utils/utils_test.go @@ -32,7 +32,6 @@ var _ = Describe("Utils", func() { Expect(len(chain1)).To(Equal(maxChainLength)) Expect(len(chain2)).To(Equal(maxChainLength)) Expect(chain1).To(Equal("CNI-374f33fe84ab0ed84dcdebe38")) - Expect(chain2).NotTo(Equal("CNI-374f33fe84ab0ed84dcdebe38")) Expect(chain1).NotTo(Equal(chain2)) }) }) From 6aad63055c0855d68d7f3785c0ac075024508457 Mon Sep 17 00:00:00 2001 From: Stefan Junker Date: Thu, 31 Mar 2016 15:44:54 +0200 Subject: [PATCH 8/9] *: add comment to iptables rules for ipmasq --- pkg/ip/ipmasq.go | 12 ++++++------ pkg/utils/utils.go | 6 ++++++ plugins/main/bridge/bridge.go | 3 ++- plugins/main/ptp/ptp.go | 6 ++++-- 4 files changed, 18 insertions(+), 9 deletions(-) diff --git a/pkg/ip/ipmasq.go b/pkg/ip/ipmasq.go index 6901f69e..8ee27971 100644 --- a/pkg/ip/ipmasq.go +++ b/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 } diff --git a/pkg/utils/utils.go b/pkg/utils/utils.go index ea29c965..7ec139fd 100644 --- a/pkg/utils/utils.go +++ b/pkg/utils/utils.go @@ -19,3 +19,9 @@ func FormatChainName(name string, id string) string { 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) +} diff --git a/plugins/main/bridge/bridge.go b/plugins/main/bridge/bridge.go index d5581bf4..e4bc106c 100644 --- a/plugins/main/bridge/bridge.go +++ b/plugins/main/bridge/bridge.go @@ -222,7 +222,8 @@ func cmdAdd(args *skel.CmdArgs) error { if n.IPMasq { 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 } } diff --git a/plugins/main/ptp/ptp.go b/plugins/main/ptp/ptp.go index b397b795..3035c643 100644 --- a/plugins/main/ptp/ptp.go +++ b/plugins/main/ptp/ptp.go @@ -179,7 +179,8 @@ func cmdAdd(args *skel.CmdArgs) error { if conf.IPMasq { chain := utils.FormatChainName(conf.Name, args.ContainerID) - if err = ip.SetupIPMasq(&result.IP4.IP, chain); err != nil { + comment := utils.FormatComment(conf.Name, args.ContainerID) + if err = ip.SetupIPMasq(&result.IP4.IP, chain, comment); err != nil { return err } } @@ -206,7 +207,8 @@ func cmdDel(args *skel.CmdArgs) error { if conf.IPMasq { chain := utils.FormatChainName(conf.Name, args.ContainerID) - if err = ip.TeardownIPMasq(ipn, chain); err != nil { + comment := utils.FormatComment(conf.Name, args.ContainerID) + if err = ip.TeardownIPMasq(ipn, chain, comment); err != nil { return err } } From 897766d74b1455b1f5ab99bd9c4688cfef0c3368 Mon Sep 17 00:00:00 2001 From: Stefan Junker Date: Thu, 31 Mar 2016 17:11:11 +0200 Subject: [PATCH 9/9] pkg/utils: correct the test's expected chain names --- pkg/utils/utils_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/utils/utils_test.go b/pkg/utils/utils_test.go index 20a9fcd5..91bb8654 100644 --- a/pkg/utils/utils_test.go +++ b/pkg/utils/utils_test.go @@ -9,13 +9,13 @@ 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-2bbe0c48b91a7d1b8a6753a8b")) + 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-374f33fe84ab0ed84dcdebe38")) + Expect(chain).To(Equal("CNI-374f33fe84ab0ed84dcdebe3")) }) It("must be predictable", func() { @@ -31,7 +31,7 @@ var _ = Describe("Utils", func() { chain2 := FormatChainName("testalongnamethatdoesnotmakesense", "1235") Expect(len(chain1)).To(Equal(maxChainLength)) Expect(len(chain2)).To(Equal(maxChainLength)) - Expect(chain1).To(Equal("CNI-374f33fe84ab0ed84dcdebe38")) + Expect(chain1).To(Equal("CNI-374f33fe84ab0ed84dcdebe3")) Expect(chain1).NotTo(Equal(chain2)) }) })