From 6b3906b07eb47a8b3c840576a310662f43d6dd10 Mon Sep 17 00:00:00 2001 From: BenTheElder Date: Fri, 7 Aug 2015 16:54:48 -0400 Subject: [PATCH 1/2] Add Save, SaveAll, Restore, RestoreAll to pkg/util/iptables MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds utility wrappers for `iptables-save` and `iptables-restore` to the iptables Interface in pkg/util/iptables. Also const’s command strings. --- pkg/util/iptables/iptables.go | 144 ++++++++++++++++++++++++++++- pkg/util/iptables/iptables_test.go | 8 +- 2 files changed, 143 insertions(+), 9 deletions(-) diff --git a/pkg/util/iptables/iptables.go b/pkg/util/iptables/iptables.go index 1995fc19d3f..84a5f9fcd3e 100644 --- a/pkg/util/iptables/iptables.go +++ b/pkg/util/iptables/iptables.go @@ -18,6 +18,8 @@ package iptables import ( "fmt" + "io/ioutil" + "os" "regexp" "strconv" "strings" @@ -49,6 +51,18 @@ type Interface interface { DeleteRule(table Table, chain Chain, args ...string) error // IsIpv6 returns true if this is managing ipv6 tables IsIpv6() bool + // Save calls `iptables-save` for table. + Save(table Table) ([]byte, error) + // SaveAll calls `iptables-save`. + SaveAll() ([]byte, error) + // Restore runs `iptables-restore` passing data through a temporary file. + // table is the Table to restore + // data should be formatted like the output of Save() + // flush sets the presence of the "--noflush" flag. see: FlushFlag + // counters sets the "--counters" flag. see: RestoreCountersFlag + Restore(table Table, data []byte, flush FlushFlag, counters RestoreCountersFlag) error + // RestoreAll is the same as Restore except that no table is specified. + RestoreAll(data []byte, flush FlushFlag, counters RestoreCountersFlag) error } type Protocol byte @@ -72,6 +86,25 @@ const ( ChainOutput Chain = "OUTPUT" ) +const ( + cmdIptablesSave string = "iptables-save" + cmdIptablesRestore string = "iptables-restore" + cmdIptables string = "iptables" + cmdIp6tables string = "ip6tables" +) + +// Option flag for Restore +type RestoreCountersFlag bool + +const RestoreCounters RestoreCountersFlag = true +const NoRestoreCounters RestoreCountersFlag = false + +// Option flag for Restore +type FlushFlag bool + +const FlushTables FlushFlag = true +const NoFlushTables FlushFlag = false + // runner implements Interface in terms of exec("iptables"). type runner struct { mu sync.Mutex @@ -178,11 +211,112 @@ func (runner *runner) IsIpv6() bool { return runner.protocol == ProtocolIpv6 } +// Save is part of Interface. +func (runner *runner) Save(table Table) ([]byte, error) { + runner.mu.Lock() + defer runner.mu.Unlock() + + // run and return + args := []string{"-t", string(table)} + return runner.exec.Command(cmdIptablesSave, args...).CombinedOutput() +} + +// SaveAll is part of Interface. +func (runner *runner) SaveAll() ([]byte, error) { + runner.mu.Lock() + defer runner.mu.Unlock() + + // run and return + return runner.exec.Command(cmdIptablesSave, []string{}...).CombinedOutput() +} + +// Restore is part of Interface. +func (runner *runner) Restore(table Table, data []byte, flush FlushFlag, counters RestoreCountersFlag) error { + runner.mu.Lock() + defer runner.mu.Unlock() + + // setup args + args := []string{"-T", string(table)} + if !flush { + args = append(args, "--noflush") + } + if counters { + args = append(args, "--counters") + } + // create temp file through which to pass data + temp, err := ioutil.TempFile("", "kube-temp-iptables-restore-") + // make sure we delete the temp file + defer os.Remove(temp.Name()) + if err != nil { + return err + } + // Put the filename at the end of args. + // NOTE: the filename must be at the end. + // See: https://git.netfilter.org/iptables/commit/iptables-restore.c?id=e6869a8f59d779ff4d5a0984c86d80db70784962 + args = append(args, temp.Name()) + if err != nil { + return err + } + // write data to the file + _, err = temp.Write(data) + temp.Close() + if err != nil { + return err + } + // run the command and return the output or an error including the output and error + b, err := runner.exec.Command(cmdIptablesRestore, args...).CombinedOutput() + if err != nil { + return fmt.Errorf("%v (%s)", err, b) + } + return nil +} + +// RestoreAll is part of Interface. +func (runner *runner) RestoreAll(data []byte, flush FlushFlag, counters RestoreCountersFlag) error { + runner.mu.Lock() + defer runner.mu.Unlock() + + // setup args + args := make([]string, 0) + if !flush { + args = append(args, "--noflush") + } + if counters { + args = append(args, "--counters") + } + // create temp file through which to pass data + temp, err := ioutil.TempFile("", "kube-temp-iptables-restore-") + // make sure we delete the temp file + defer os.Remove(temp.Name()) + if err != nil { + return err + } + // Put the filename at the end of args. + // NOTE: the filename must be at the end. + // See: https://git.netfilter.org/iptables/commit/iptables-restore.c?id=e6869a8f59d779ff4d5a0984c86d80db70784962 + args = append(args, temp.Name()) + if err != nil { + return err + } + // write data to the file + _, err = temp.Write(data) + temp.Close() + if err != nil { + return err + } + // run the command and return the output or an error including the output and error + b, err := runner.exec.Command(cmdIptablesRestore, args...).CombinedOutput() + if err != nil { + return fmt.Errorf("%v (%s)", err, b) + } + return nil +} + func (runner *runner) iptablesCommand() string { if runner.IsIpv6() { - return "ip6tables" + return cmdIp6tables } else { - return "iptables" + return cmdIptables } } @@ -215,7 +349,7 @@ func (runner *runner) checkRule(table Table, chain Chain, args ...string) (bool, // of hack and half-measures. We should nix this ASAP. func (runner *runner) checkRuleWithoutCheck(table Table, chain Chain, args ...string) (bool, error) { glog.V(1).Infof("running iptables-save -t %s", string(table)) - out, err := runner.exec.Command("iptables-save", "-t", string(table)).CombinedOutput() + out, err := runner.exec.Command(cmdIptablesSave, "-t", string(table)).CombinedOutput() if err != nil { return false, fmt.Errorf("error checking rule: %v", err) } @@ -335,11 +469,11 @@ func extractIptablesVersion(str string) (int, int, int, error) { // Runs "iptables --version" to get the version string func getIptablesVersionString(exec utilexec.Interface) (string, error) { - bytes, err := exec.Command("iptables", "--version").CombinedOutput() + // this doesn't access mutable state so we don't need to use the interface / runner + bytes, err := exec.Command(cmdIptables, "--version").CombinedOutput() if err != nil { return "", err } - return string(bytes), nil } diff --git a/pkg/util/iptables/iptables_test.go b/pkg/util/iptables/iptables_test.go index b64f25e4761..844be9652b0 100644 --- a/pkg/util/iptables/iptables_test.go +++ b/pkg/util/iptables/iptables_test.go @@ -25,10 +25,10 @@ import ( func getIptablesCommand(protocol Protocol) string { if protocol == ProtocolIpv4 { - return "iptables" + return cmdIptables } if protocol == ProtocolIpv6 { - return "ip6tables" + return cmdIp6tables } panic("Unknown protocol") } @@ -503,7 +503,7 @@ func TestCheckRuleWithoutCheckPresent(t *testing.T) { :PREROUTING ACCEPT [2136997:197881818] :POSTROUTING ACCEPT [4284525:258542680] :OUTPUT ACCEPT [5901660:357267963] --A PREROUTING -m addrtype --dst-type LOCAL -j DOCKER +-A PREROUTING -m addrtype --dst-type LOCAL -j DOCKER COMMIT # Completed on Wed Oct 29 14:56:01 2014` @@ -541,7 +541,7 @@ func TestCheckRuleWithoutCheckAbsent(t *testing.T) { :PREROUTING ACCEPT [2136997:197881818] :POSTROUTING ACCEPT [4284525:258542680] :OUTPUT ACCEPT [5901660:357267963] --A PREROUTING -m addrtype --dst-type LOCAL -j DOCKER +-A PREROUTING -m addrtype --dst-type LOCAL -j DOCKER COMMIT # Completed on Wed Oct 29 14:56:01 2014` From 5867fca8bffcf8f0a4f34f7d30b4299aa8baab57 Mon Sep 17 00:00:00 2001 From: BenTheElder Date: Fri, 7 Aug 2015 19:06:54 -0400 Subject: [PATCH 2/2] Fix iptables Interface mocking, move Restore/RestoreAll to shared impl also put TODO for unit tests, move defer file deletion until after file creation error is checked. --- pkg/proxy/proxier_test.go | 16 +++++++++++ pkg/util/iptables/iptables.go | 50 ++++++++--------------------------- 2 files changed, 27 insertions(+), 39 deletions(-) diff --git a/pkg/proxy/proxier_test.go b/pkg/proxy/proxier_test.go index 8214de3cf4a..e5714eca7da 100644 --- a/pkg/proxy/proxier_test.go +++ b/pkg/proxy/proxier_test.go @@ -104,6 +104,22 @@ func (fake *fakeIptables) IsIpv6() bool { return false } +func (fake *fakeIptables) Save(table iptables.Table) ([]byte, error) { + return []byte{}, nil +} + +func (fake *fakeIptables) SaveAll() ([]byte, error) { + return []byte{}, nil +} + +func (fake *fakeIptables) Restore(table iptables.Table, data []byte, flush iptables.FlushFlag, counters iptables.RestoreCountersFlag) error { + return nil +} + +func (fake *fakeIptables) RestoreAll(data []byte, flush iptables.FlushFlag, counters iptables.RestoreCountersFlag) error { + return nil +} + var tcpServerPort int var udpServerPort int diff --git a/pkg/util/iptables/iptables.go b/pkg/util/iptables/iptables.go index 84a5f9fcd3e..0c54d57b7c1 100644 --- a/pkg/util/iptables/iptables.go +++ b/pkg/util/iptables/iptables.go @@ -51,6 +51,7 @@ type Interface interface { DeleteRule(table Table, chain Chain, args ...string) error // IsIpv6 returns true if this is managing ipv6 tables IsIpv6() bool + // TODO: (BenTheElder) Unit-Test Save/SaveAll, Restore/RestoreAll // Save calls `iptables-save` for table. Save(table Table) ([]byte, error) // SaveAll calls `iptables-save`. @@ -232,52 +233,23 @@ func (runner *runner) SaveAll() ([]byte, error) { // Restore is part of Interface. func (runner *runner) Restore(table Table, data []byte, flush FlushFlag, counters RestoreCountersFlag) error { - runner.mu.Lock() - defer runner.mu.Unlock() - // setup args args := []string{"-T", string(table)} - if !flush { - args = append(args, "--noflush") - } - if counters { - args = append(args, "--counters") - } - // create temp file through which to pass data - temp, err := ioutil.TempFile("", "kube-temp-iptables-restore-") - // make sure we delete the temp file - defer os.Remove(temp.Name()) - if err != nil { - return err - } - // Put the filename at the end of args. - // NOTE: the filename must be at the end. - // See: https://git.netfilter.org/iptables/commit/iptables-restore.c?id=e6869a8f59d779ff4d5a0984c86d80db70784962 - args = append(args, temp.Name()) - if err != nil { - return err - } - // write data to the file - _, err = temp.Write(data) - temp.Close() - if err != nil { - return err - } - // run the command and return the output or an error including the output and error - b, err := runner.exec.Command(cmdIptablesRestore, args...).CombinedOutput() - if err != nil { - return fmt.Errorf("%v (%s)", err, b) - } - return nil + return runner.restoreInternal(args, data, flush, counters) } // RestoreAll is part of Interface. func (runner *runner) RestoreAll(data []byte, flush FlushFlag, counters RestoreCountersFlag) error { + // setup args + args := make([]string, 0) + return runner.restoreInternal(args, data, flush, counters) +} + +// restoreInternal is the shared part of Restore/RestoreAll +func (runner *runner) restoreInternal(args []string, data []byte, flush FlushFlag, counters RestoreCountersFlag) error { runner.mu.Lock() defer runner.mu.Unlock() - // setup args - args := make([]string, 0) if !flush { args = append(args, "--noflush") } @@ -286,11 +258,11 @@ func (runner *runner) RestoreAll(data []byte, flush FlushFlag, counters RestoreC } // create temp file through which to pass data temp, err := ioutil.TempFile("", "kube-temp-iptables-restore-") - // make sure we delete the temp file - defer os.Remove(temp.Name()) if err != nil { return err } + // make sure we delete the temp file + defer os.Remove(temp.Name()) // Put the filename at the end of args. // NOTE: the filename must be at the end. // See: https://git.netfilter.org/iptables/commit/iptables-restore.c?id=e6869a8f59d779ff4d5a0984c86d80db70784962