From 1850b091853e2933b1322980f08c4177f6711e30 Mon Sep 17 00:00:00 2001 From: Will Gorman Date: Wed, 19 Jun 2019 09:34:48 -0500 Subject: [PATCH] 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 | 29 +++++++++++++--- network/iptables_test.go | 73 ++++++++++++++++++++++++++++++++++++---- 2 files changed, 91 insertions(+), 11 deletions(-) diff --git a/network/iptables.go b/network/iptables.go index 6db8544e4c..bceb2fd0bb 100644 --- a/network/iptables.go +++ b/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 } diff --git a/network/iptables_test.go b/network/iptables_test.go index 8dbc986b2b..9aadd74c16 100644 --- a/network/iptables_test.go +++ b/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) } }