Merge pull request #924 from SirPhuttel/go-iptables_review

Review code using go-iptables module

pkg/utils/iptables.go

@@ -29,9 +29,9 @@ func EnsureChain(ipt *iptables.IPTables, table, chain string) error {
 	if ipt == nil {
 		return errors.New("failed to ensure iptable chain: IPTables was nil")
 	}
-	exists, err := ChainExists(ipt, table, chain)
+	exists, err := ipt.ChainExists(table, chain)
 	if err != nil {
-		return fmt.Errorf("failed to list iptables chains: %v", err)
+		return fmt.Errorf("failed to check iptables chain existence: %v", err)
 	}
 	if !exists {
 		err = ipt.NewChain(table, chain)
@@ -45,24 +45,6 @@ func EnsureChain(ipt *iptables.IPTables, table, chain string) error {
 	return nil
 }
 
-// ChainExists checks whether an iptables chain exists.
-func ChainExists(ipt *iptables.IPTables, table, chain string) (bool, error) {
-	if ipt == nil {
-		return false, errors.New("failed to check iptable chain: IPTables was nil")
-	}
-	chains, err := ipt.ListChains(table)
-	if err != nil {
-		return false, err
-	}
-
-	for _, ch := range chains {
-		if ch == chain {
-			return true, nil
-		}
-	}
-	return false, nil
-}
-
 // DeleteRule idempotently delete the iptables rule in the specified table/chain.
 // It does not return an error if the referring chain doesn't exist
 func DeleteRule(ipt *iptables.IPTables, table, chain string, rulespec ...string) error {

plugins/meta/portmap/chain.go

@@ -67,13 +67,27 @@ func (c *chain) setup(ipt *iptables.IPTables) error {
 // teardown idempotently deletes a chain. It will not error if the chain doesn't exist.
 // It will first delete all references to this chain in the entryChains.
 func (c *chain) teardown(ipt *iptables.IPTables) error {
-	// flush the chain
+	// nothing to do if the custom chain doesn't exist to begin with
-	// This will succeed *and create the chain* if it does not exist.
+	exists, err := ipt.ChainExists(c.table, c.name)
-	// If the chain doesn't exist, the next checks will fail.
+	if err == nil && !exists {
-	if err := utils.ClearChain(ipt, c.table, c.name); err != nil {
+		return nil
-		return err
+	}
+	// delete references created by setup()
+	for _, entryChain := range c.entryChains {
+		for _, rule := range c.entryRules {
+			r := []string{}
+			r = append(r, rule...)
+			r = append(r, "-j", c.name)
+
+			ipt.Delete(c.table, entryChain, r...)
+		}
+	}
+	// if chain deletion succeeds now, all references are gone
+	if err := ipt.ClearAndDeleteChain(c.table, c.name); err == nil {
+		return nil
 	}
 
+	// find references the hard way
 	for _, entryChain := range c.entryChains {
 		entryChainRules, err := ipt.List(c.table, entryChain)
 		if err != nil || len(entryChainRules) < 1 {
@@ -98,12 +112,12 @@ func (c *chain) teardown(ipt *iptables.IPTables) error {
 		}
 	}
 
-	return utils.DeleteChain(ipt, c.table, c.name)
+	return ipt.ClearAndDeleteChain(c.table, c.name)
 }
 
 // check the chain.
 func (c *chain) check(ipt *iptables.IPTables) error {
-	exists, err := utils.ChainExists(ipt, c.table, c.name)
+	exists, err := ipt.ChainExists(c.table, c.name)
 	if err != nil {
 		return err
 	}