From 8b689bf1da7c9943dae779139494dec6662254be Mon Sep 17 00:00:00 2001 From: Louis Tournayre Date: Thu, 13 Jan 2022 16:21:39 +0100 Subject: [PATCH] Changes from review : - refactor protocol to proto - fix check version for wait command available on iptables-restore - apply "Error strings should not be capitalized" - lower case for iptables-restore - refactor function ApplyPartial to ApplyWithoutFlush --- network/iptables.go | 24 ++++++++++++------------ network/iptables_restore.go | 28 ++++++++++++++-------------- network/iptables_restore_test.go | 1 + network/iptables_test.go | 2 +- 4 files changed, 28 insertions(+), 27 deletions(-) diff --git a/network/iptables.go b/network/iptables.go index 574e617cb..51aacf10a 100644 --- a/network/iptables.go +++ b/network/iptables.go @@ -159,13 +159,13 @@ func ipTablesCleanAndBuild(ipt IPTables, rules []IPTablesRule) (IPTablesRestoreR func ipTablesBootstrap(ipt IPTables, iptRestore IPTablesRestore, rules []IPTablesRule) error { tablesRules, err := ipTablesCleanAndBuild(ipt, rules) if err != nil { - // if we can't find iptables, give up and return - return fmt.Errorf("Failed to setup IPTables-restore payload: %v", err) + // if we can't find iptables or if we can check existing rules, give up and return + return fmt.Errorf("failed to setup iptables-restore payload: %v", err) } - log.V(6).Infof("trying to run iptable-restore < %+v", tablesRules) + log.V(6).Infof("trying to run iptables-restore < %+v", tablesRules) - err = iptRestore.ApplyPartial(tablesRules) + err = iptRestore.ApplyWithoutFlush(tablesRules) if err != nil { return fmt.Errorf("failed to apply partial iptables-restore %v", err) } @@ -184,8 +184,8 @@ func SetupAndEnsureIPTables(rules []IPTablesRule, resyncPeriod int) { } iptRestore, err := NewIPTablesRestore() if err != nil { - // if we can't find iptables, give up and return - log.Errorf("Failed to setup IPTables. iptables binary was not found: %v", err) + // if we can't find iptables-restore, give up and return + log.Errorf("Failed to setup IPTables. iptables-restore binary was not found: %v", err) return } @@ -219,7 +219,7 @@ func SetupAndEnsureIP6Tables(rules []IPTablesRule, resyncPeriod int) { iptRestore, err := NewIPTablesRestore() if err != nil { // if we can't find iptables, give up and return - log.Errorf("Failed to setup IPTables-restore: %v", err) + log.Errorf("Failed to setup iptables-restore: %v", err) return } @@ -254,7 +254,7 @@ func DeleteIPTables(rules []IPTablesRule) error { iptRestore, err := NewIPTablesRestore() if err != nil { // if we can't find iptables, give up and return - log.Errorf("Failed to setup IPTables-restore: %v", err) + log.Errorf("Failed to setup iptables-restore: %v", err) return err } err = teardownIPTables(ipt, iptRestore, rules) @@ -277,7 +277,7 @@ func DeleteIP6Tables(rules []IPTablesRule) error { iptRestore, err := NewIPTablesRestoreWithProtocol(iptables.ProtocolIPv6) if err != nil { // if we can't find iptables, give up and return - log.Errorf("Failed to setup IPTables-restore: %v", err) + log.Errorf("Failed to setup iptables-restore: %v", err) return err } err = teardownIPTables(ipt, iptRestore, rules) @@ -291,7 +291,7 @@ func DeleteIP6Tables(rules []IPTablesRule) error { func ensureIPTables(ipt IPTables, iptRestore IPTablesRestore, rules []IPTablesRule) error { exists, err := ipTablesRulesExist(ipt, rules) if err != nil { - return fmt.Errorf("Error checking rule existence: %v", err) + return fmt.Errorf("error checking rule existence: %v", err) } if exists { // if all the rules already exist, no need to do anything @@ -303,7 +303,7 @@ func ensureIPTables(ipt IPTables, iptRestore IPTablesRestore, rules []IPTablesRu err = ipTablesBootstrap(ipt, iptRestore, rules) if err != nil { // if we can't find iptables, give up and return - return fmt.Errorf("Error setting up rules: %v", err) + return fmt.Errorf("error setting up rules: %v", err) } return nil } @@ -325,7 +325,7 @@ func teardownIPTables(ipt IPTables, iptr IPTablesRestore, rules []IPTablesRule) tablesRules[rule.table] = append(tablesRules[rule.table], append(IPTablesRestoreRuleSpec{"-D", rule.chain}, rule.rulespec...)) } } - err := iptr.ApplyPartial(tablesRules) // ApplyPartial make a diff, Apply make a replace (desired state) + err := iptr.ApplyWithoutFlush(tablesRules) // ApplyWithoutFlush make a diff, Apply make a replace (desired state) if err != nil { return fmt.Errorf("unable to teardown iptables: %v", err) } diff --git a/network/iptables_restore.go b/network/iptables_restore.go index e016a4b4a..3091c2733 100644 --- a/network/iptables_restore.go +++ b/network/iptables_restore.go @@ -38,15 +38,15 @@ const ( type IPTablesRestore interface { // ApplyFully apply all rules and flush chains ApplyFully(rules IPTablesRestoreRules) error - // ApplyPartial apply without flush chains - ApplyPartial(rules IPTablesRestoreRules) error + // ApplyWithoutFlush apply without flush chains + ApplyWithoutFlush(rules IPTablesRestoreRules) error } // ipTablesRestore internal type type ipTablesRestore struct { - path string - protocol iptables.Protocol - hasWait bool + path string + proto iptables.Protocol + hasWait bool } // IPTablesRestoreRules represents iptables-restore table block @@ -60,7 +60,7 @@ func NewIPTablesRestore() (IPTablesRestore, error) { return NewIPTablesRestoreWithProtocol(iptables.ProtocolIPv4) } -// NewIPTablesRestoreWithProtocol build new IPTablesRestore for supplied protocol +// NewIPTablesRestoreWithProtocol build new IPTablesRestore for supplied proto func NewIPTablesRestoreWithProtocol(protocol iptables.Protocol) (IPTablesRestore, error) { cmd := getIptablesRestoreCommand(protocol) path, err := exec.LookPath(cmd) @@ -73,9 +73,9 @@ func NewIPTablesRestoreWithProtocol(protocol iptables.Protocol) (IPTablesRestore } ipt := ipTablesRestore{ - path: path, - protocol: protocol, - hasWait: hasWait, + path: path, + proto: protocol, + hasWait: hasWait, } return &ipt, nil } @@ -92,8 +92,8 @@ func (iptr *ipTablesRestore) ApplyFully(rules IPTablesRestoreRules) error { return nil } -// ApplyPartial apply without flush chains -func (iptr *ipTablesRestore) ApplyPartial(rules IPTablesRestoreRules) error { +// ApplyWithoutFlush apply without flush chains +func (iptr *ipTablesRestore) ApplyWithoutFlush(rules IPTablesRestoreRules) error { payload := buildIPTablesRestorePayload(rules) log.V(6).Infof("trying to run with payload %s", payload) @@ -158,10 +158,10 @@ func ipTablesHasWaitSupport(v1, v2, v3 int) bool { if v1 > 1 { return true } - if v1 == 1 && v2 > 4 { + if v1 == 1 && v2 > 6 { return true } - if v1 == 1 && v2 == 4 && v3 >= 20 { + if v1 == 1 && v2 == 6 && v3 >= 2 { return true } return false @@ -205,7 +205,7 @@ func getIptablesRestoreVersionString(path string) (string, error) { return out.String(), nil } -// getIptablesRestoreCommand returns the correct command for the given protocol, either "iptables-restore" or "ip6tables-restore". +// getIptablesRestoreCommand returns the correct command for the given proto, either "iptables-restore" or "ip6tables-restore". func getIptablesRestoreCommand(proto iptables.Protocol) string { if proto == iptables.ProtocolIPv6 { return ip6TablesRestoreCmd diff --git a/network/iptables_restore_test.go b/network/iptables_restore_test.go index fc8114f80..18552536d 100644 --- a/network/iptables_restore_test.go +++ b/network/iptables_restore_test.go @@ -11,6 +11,7 @@ // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. // See the License for the specific language governing permissions and // limitations under the License. +//go:build !windows // +build !windows package network diff --git a/network/iptables_test.go b/network/iptables_test.go index 66b5e2cae..f3346c18c 100644 --- a/network/iptables_test.go +++ b/network/iptables_test.go @@ -75,7 +75,7 @@ func (mock *MockIPTablesRestore) ApplyFully(rules IPTablesRestoreRules) error { return nil } -func (mock *MockIPTablesRestore) ApplyPartial(rules IPTablesRestoreRules) error { +func (mock *MockIPTablesRestore) ApplyWithoutFlush(rules IPTablesRestoreRules) error { mock.rules = append(mock.rules, rules) return nil }