iptables: handle errors that prevent rule deletes

This prevents an error during a delete that leaves
a rule in place from causing the rules to be
incorrectly ordered after appending the rest of
the rules.

Fixes #1146

network/iptables.go

@@ -34,6 +34,11 @@ type IPTables interface {
 	Exists(table string, chain string, rulespec ...string) (bool, error)
 }
 
+type IPTablesError interface {
+	IsNotExist() bool
+	Error() string
+}
+
 type IPTablesRule struct {
 	table    string
 	chain    string
@@ -143,7 +148,9 @@ func ensureIPTables(ipt IPTables, rules []IPTablesRule) error {
 	// Otherwise, teardown all the rules and set them up again
 	// We do this because the order of the rules is important
 	log.Info("Some iptables rules are missing; deleting and recreating rules")
-	teardownIPTables(ipt, rules)
+	if err = teardownIPTables(ipt, rules); err != nil {
+		return fmt.Errorf("Error tearing down rules: %v", err)
+	}
 	if err = setupIPTables(ipt, rules); err != nil {
 		return fmt.Errorf("Error setting up rules: %v", err)
 	}
@@ -162,11 +169,23 @@ func setupIPTables(ipt IPTables, rules []IPTablesRule) error {
 	return nil
 }
 
-func teardownIPTables(ipt IPTables, rules []IPTablesRule) {
+func teardownIPTables(ipt IPTables, rules []IPTablesRule) error {
 	for _, rule := range rules {
 		log.Info("Deleting iptables rule: ", strings.Join(rule.rulespec, " "))
-		// We ignore errors here because if there's an error it's almost certainly because the rule
-		// doesn't exist, which is fine (we don't need to delete rules that don't exist)
-		ipt.Delete(rule.table, rule.chain, rule.rulespec...)
+		err := ipt.Delete(rule.table, rule.chain, rule.rulespec...)
+		if err != nil {
+			e := err.(IPTablesError)
+			// If this error is because the rule is already deleted, the message from iptables will be
+			// "Bad rule (does a matching rule exist in that chain?)". These are safe to ignore.
+			// However other errors (like EAGAIN caused by other things not respecting the xtables.lock)
+			// should halt the ensure process.  Otherwise rules can get out of order when a rule we think
+			// is deleted is actually still in the chain.
+			// This will leave the rules incomplete until the next successful reconciliation loop.
+			if !e.IsNotExist() {
+				return err
+			}
+		}
 	}
+
+	return nil
 }

network/iptables_test.go

@@ -16,8 +16,10 @@
 package network
 
 import (
+	"fmt"
 	"net"
 	"reflect"
+	"strings"
 	"testing"
 
 	"github.com/coreos/flannel/pkg/ip"
@@ -32,7 +34,32 @@ func lease() *subnet.Lease {
 }
 
 type MockIPTables struct {
-	rules []IPTablesRule
+	rules    []IPTablesRule
+	t        *testing.T
+	failures map[string]*MockIPTablesError
+}
+
+type MockIPTablesError struct {
+	notExist bool
+}
+
+func (mock *MockIPTablesError) IsNotExist() bool {
+	return mock.notExist
+}
+
+func (mock *MockIPTablesError) Error() string {
+	return fmt.Sprintf("IsNotExist: %v", !mock.notExist)
+}
+
+func (mock *MockIPTables) failDelete(table string, chain string, rulespec []string, notExist bool) {
+
+	if mock.failures == nil {
+		mock.failures = make(map[string]*MockIPTablesError)
+	}
+	key := table + chain + strings.Join(rulespec, "")
+	mock.failures[key] = &MockIPTablesError{
+		notExist: notExist,
+	}
 }
 
 func (mock *MockIPTables) ruleIndex(table string, chain string, rulespec []string) int {
@@ -46,6 +73,12 @@ func (mock *MockIPTables) ruleIndex(table string, chain string, rulespec []strin
 
 func (mock *MockIPTables) Delete(table string, chain string, rulespec ...string) error {
 	var ruleIndex = mock.ruleIndex(table, chain, rulespec)
+	key := table + chain + strings.Join(rulespec, "")
+	reason := mock.failures[key]
+	if reason != nil {
+		return reason
+	}
+
 	if ruleIndex != -1 {
 		mock.rules = append(mock.rules[:ruleIndex], mock.rules[ruleIndex+1:]...)
 	}
@@ -69,7 +102,7 @@ func (mock *MockIPTables) AppendUnique(table string, chain string, rulespec ...s
 }
 
 func TestDeleteRules(t *testing.T) {
-	ipt := &MockIPTables{}
+	ipt := &MockIPTables{t: t}
 	setupIPTables(ipt, MasqRules(ip.IP4Net{}, lease()))
 	if len(ipt.rules) != 4 {
 		t.Errorf("Should be 4 masqRules, there are actually %d: %#v", len(ipt.rules), ipt.rules)
@@ -80,16 +113,44 @@ func TestDeleteRules(t *testing.T) {
 	}
 }
 
+func TestEnsureRulesError(t *testing.T) {
+	// If an error prevents a rule from being deleted, ensureIPTables should leave the rules as is
+	// rather than potentially re-appending rules in an incorrect order
+	ipt_correct := &MockIPTables{t: t}
+	setupIPTables(ipt_correct, MasqRules(ip.IP4Net{}, lease()))
+	// setup a mock instance where we delete some masqRules and run `ensureIPTables`
+	ipt_recreate := &MockIPTables{t: t}
+	setupIPTables(ipt_recreate, MasqRules(ip.IP4Net{}, lease()))
+	ipt_recreate.rules = ipt_recreate.rules[0:2]
+
+	rule := ipt_recreate.rules[1]
+	ipt_recreate.failDelete(rule.table, rule.chain, rule.rulespec, false)
+	err := ensureIPTables(ipt_recreate, MasqRules(ip.IP4Net{}, lease()))
+	if err == nil {
+		t.Errorf("ensureIPTables should have failed but did not.")
+	}
+
+	if len(ipt_recreate.rules) == len(ipt_correct.rules) {
+		t.Errorf("ensureIPTables should not have completed.")
+	}
+}
+
 func TestEnsureRules(t *testing.T) {
 	// If any masqRules are missing, they should be all deleted and recreated in the correct order
-	ipt_correct := &MockIPTables{}
+	ipt_correct := &MockIPTables{t: t}
 	setupIPTables(ipt_correct, MasqRules(ip.IP4Net{}, lease()))
 	// setup a mock instance where we delete some masqRules and run `ensureIPTables`
-	ipt_recreate := &MockIPTables{}
+	ipt_recreate := &MockIPTables{t: t}
 	setupIPTables(ipt_recreate, MasqRules(ip.IP4Net{}, lease()))
 	ipt_recreate.rules = ipt_recreate.rules[0:2]
-	ensureIPTables(ipt_recreate, MasqRules(ip.IP4Net{}, lease()))
+	// set up a normal error that iptables returns when deleting a rule that is already gone
+	deletedRule := ipt_correct.rules[3]
+	ipt_recreate.failDelete(deletedRule.table, deletedRule.chain, deletedRule.rulespec, true)
+	err := ensureIPTables(ipt_recreate, MasqRules(ip.IP4Net{}, lease()))
+	if err != nil {
+		t.Errorf("ensureIPTables should have completed without errors")
+	}
 	if !reflect.DeepEqual(ipt_recreate.rules, ipt_correct.rules) {
-		t.Errorf("iptables masqRules after ensureIPTables are incorrected. Expected: %#v, Actual: %#v", ipt_recreate.rules, ipt_correct.rules)
+		t.Errorf("iptables masqRules after ensureIPTables are incorrect. Expected: %#v, Actual: %#v", ipt_recreate.rules, ipt_correct.rules)
 	}
 }