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 new file mode 100644 index 00000000..7ec139fd --- /dev/null +++ b/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) +} 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..91bb8654 --- /dev/null +++ b/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)) + }) +}) diff --git a/plugins/main/bridge/bridge.go b/plugins/main/bridge/bridge.go index 49c0aa5d..e4bc106c 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,8 +221,9 @@ func cmdAdd(args *skel.CmdArgs) error { } if n.IPMasq { - chain := "CNI-" + n.Name - if err = ip.SetupIPMasq(ip.Network(&result.IP4.IP), chain); err != nil { + chain := utils.FormatChainName(n.Name, args.ContainerID) + 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 3cb8f643..3035c643 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,9 +178,9 @@ 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]) - if err = ip.SetupIPMasq(&result.IP4.IP, chain); err != nil { + chain := utils.FormatChainName(conf.Name, args.ContainerID) + comment := utils.FormatComment(conf.Name, args.ContainerID) + 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 := fmt.Sprintf("CNI-%s-%x", conf.Name, h[:8]) - if err = ip.TeardownIPMasq(ipn, chain); err != nil { + chain := utils.FormatChainName(conf.Name, args.ContainerID) + comment := utils.FormatComment(conf.Name, args.ContainerID) + if err = ip.TeardownIPMasq(ipn, chain, comment); 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