Merge pull request #269 from squeed/portmap-prepend

Portmap: append, rather than prepend, entry rules

plugins/meta/portmap/chain.go

@@ -29,6 +29,8 @@ type chain struct {
 
 	entryRules [][]string // the rules that "point" to this chain
 	rules      [][]string // the rules this chain contains
+
+	prependEntry bool // whether or not the entry rules should be prepended
 }
 
 // setup idempotently creates the chain. It will not error if the chain exists.
@@ -45,19 +47,19 @@ func (c *chain) setup(ipt *iptables.IPTables) error {
 	}
 
 	// Add the rules to the chain
-	for i := len(c.rules) - 1; i >= 0; i-- {
-		if err := prependUnique(ipt, c.table, c.name, c.rules[i]); err != nil {
+	for _, rule := range c.rules {
+		if err := insertUnique(ipt, c.table, c.name, false, rule); err != nil {
 			return err
 		}
 	}
 
 	// Add the entry rules to the entry chains
 	for _, entryChain := range c.entryChains {
-		for i := len(c.entryRules) - 1; i >= 0; i-- {
+		for _, rule := range c.entryRules {
 			r := []string{}
-			r = append(r, c.entryRules[i]...)
+			r = append(r, rule...)
 			r = append(r, "-j", c.name)
-			if err := prependUnique(ipt, c.table, entryChain, r); err != nil {
+			if err := insertUnique(ipt, c.table, entryChain, c.prependEntry, r); err != nil {
 				return err
 			}
 		}
@@ -105,8 +107,9 @@ func (c *chain) teardown(ipt *iptables.IPTables) error {
 	return nil
 }
 
-// prependUnique will prepend a rule to a chain, if it does not already exist
-func prependUnique(ipt *iptables.IPTables, table, chain string, rule []string) error {
+// insertUnique will add a rule to a chain if it does not already exist.
+// By default the rule is appended, unless prepend is true.
+func insertUnique(ipt *iptables.IPTables, table, chain string, prepend bool, rule []string) error {
 	exists, err := ipt.Exists(table, chain, rule...)
 	if err != nil {
 		return err
@@ -115,7 +118,11 @@ func prependUnique(ipt *iptables.IPTables, table, chain string, rule []string) e
 		return nil
 	}
 
-	return ipt.Insert(table, chain, 1, rule...)
+	if prepend {
+		return ipt.Insert(table, chain, 1, rule...)
+	} else {
+		return ipt.Append(table, chain, rule...)
+	}
 }
 
 func chainExists(ipt *iptables.IPTables, tableName, chainName string) (bool, error) {

plugins/meta/portmap/chain_test.go

@@ -116,8 +116,8 @@ var _ = Describe("chain tests", func() {
 		Expect(err).NotTo(HaveOccurred())
 		Expect(haveRules).To(Equal([]string{
 			"-N " + tlChainName,
-			"-A " + tlChainName + " -d 203.0.113.1/32 -j " + testChain.name,
 			"-A " + tlChainName + ` -m comment --comment "canary value" -j ACCEPT`,
+			"-A " + tlChainName + " -d 203.0.113.1/32 -j " + testChain.name,
 		}))
 
 		// Check that the chain and rule was created

plugins/meta/portmap/portmap.go

@@ -255,6 +255,10 @@ func genMarkMasqChain(markBit int) chain {
 		table:       "nat",
 		name:        MarkMasqChainName,
 		entryChains: []string{"POSTROUTING"},
+		// Only this entry chain needs to be prepended, because otherwise it is
+		// stomped on by the masquerading rules created by the CNI ptp and bridge
+		// plugins.
+		prependEntry: true,
 		entryRules: [][]string{{
 			"-m", "comment",
 			"--comment", "CNI portfwd requiring masquerade",

plugins/meta/portmap/portmap_integ_test.go

@@ -163,6 +163,12 @@ var _ = Describe("portmap integration tests", func() {
 		fmt.Fprintf(GinkgoWriter, "hostIP: %s:%d, contIP: %s:%d\n",
 			hostIP, hostPort, contIP, containerPort)
 
+		// dump iptables-save output for debugging
+		cmd = exec.Command("iptables-save")
+		cmd.Stderr = GinkgoWriter
+		cmd.Stdout = GinkgoWriter
+		Expect(cmd.Run()).To(Succeed())
+
 		// Sanity check: verify that the container is reachable directly
 		contOK := testEchoServer(contIP.String(), containerPort, "")
 

plugins/meta/portmap/portmap_test.go

@@ -323,6 +323,7 @@ var _ = Describe("portmapping configuration", func() {
 						"--mark", "0x20/0x20",
 						"-j", "MASQUERADE",
 					}},
+					prependEntry: true,
 				}))
 			})
 		})