Merge pull request #46209 from wojtek-t/remove_iptables_save

Automatic merge from submit-queue (batch tested with PRs 46022, 46055, 45308, 46209, 43590)

Remove Save() from iptables interface

This is what @thockin requested in one of the reviews.

pkg/kubelet/network/hostport/fake_iptables.go

@@ -227,12 +227,6 @@ func saveChain(chain *fakeChain, data *bytes.Buffer) {
 	}
 }
 
-func (f *fakeIPTables) Save(tableName utiliptables.Table) ([]byte, error) {
-	data := bytes.NewBuffer(nil)
-	err := f.SaveInto(tableName, data)
-	return data.Bytes(), err
-}
-
 func (f *fakeIPTables) SaveInto(tableName utiliptables.Table, buffer *bytes.Buffer) error {
 	table, err := f.getTable(tableName)
 	if err != nil {

pkg/kubelet/network/hostport/hostport_manager.go

@@ -275,11 +275,12 @@ func gatherHostportMappings(podPortMapping *PodPortMapping) []*PortMapping {
 // getExistingHostportIPTablesRules retrieves raw data from iptables-save, parse it,
 // return all the hostport related chains and rules
 func getExistingHostportIPTablesRules(iptables utiliptables.Interface) (map[utiliptables.Chain]string, []string, error) {
-	iptablesSaveRaw, err := iptables.Save(utiliptables.TableNAT)
+	iptablesData := bytes.NewBuffer(nil)
+	err := iptables.SaveInto(utiliptables.TableNAT, iptablesData)
 	if err != nil { // if we failed to get any rules
 		return nil, nil, fmt.Errorf("failed to execute iptables-save: %v", err)
 	}
-	existingNATChains := utiliptables.GetChainLines(utiliptables.TableNAT, iptablesSaveRaw)
+	existingNATChains := utiliptables.GetChainLines(utiliptables.TableNAT, iptablesData.Bytes())
 
 	existingHostportChains := make(map[utiliptables.Chain]string)
 	existingHostportRules := []string{}
@@ -290,7 +291,7 @@ func getExistingHostportIPTablesRules(iptables utiliptables.Interface) (map[util
 		}
 	}
 
-	for _, line := range strings.Split(string(iptablesSaveRaw), "\n") {
+	for _, line := range strings.Split(string(iptablesData.Bytes()), "\n") {
 		if strings.HasPrefix(line, fmt.Sprintf("-A %s", kubeHostportChainPrefix)) ||
 			strings.HasPrefix(line, fmt.Sprintf("-A %s", string(kubeHostportsChain))) {
 			existingHostportRules = append(existingHostportRules, line)

pkg/kubelet/network/hostport/hostport_manager_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package hostport
 
 import (
+	"bytes"
 	"net"
 	"testing"
 
@@ -132,10 +133,11 @@ func TestHostportManager(t *testing.T) {
 	}
 
 	// Check Iptables-save result after adding hostports
-	raw, err := iptables.Save(utiliptables.TableNAT)
+	raw := bytes.NewBuffer(nil)
+	err := iptables.SaveInto(utiliptables.TableNAT, raw)
 	assert.NoError(t, err)
 
-	lines := strings.Split(string(raw), "\n")
+	lines := strings.Split(string(raw.Bytes()), "\n")
 	expectedLines := map[string]bool{
 		`*nat`: true,
 		`:KUBE-HOSTPORTS - [0:0]`:                                                                                                         true,
@@ -175,9 +177,10 @@ func TestHostportManager(t *testing.T) {
 	}
 
 	// Check Iptables-save result after deleting hostports
-	raw, err = iptables.Save(utiliptables.TableNAT)
+	raw.Reset()
+	err = iptables.SaveInto(utiliptables.TableNAT, raw)
 	assert.NoError(t, err)
-	lines = strings.Split(string(raw), "\n")
+	lines = strings.Split(string(raw.Bytes()), "\n")
 	remainingChains := make(map[string]bool)
 	for _, line := range lines {
 		if strings.HasPrefix(line, ":") {

pkg/kubelet/network/hostport/hostport_syncer.go

@@ -192,11 +192,12 @@ func (h *hostportSyncer) SyncHostports(natInterfaceName string, activePodPortMap
 	// Get iptables-save output so we can check for existing chains and rules.
 	// This will be a map of chain name to chain with rules as stored in iptables-save/iptables-restore
 	existingNATChains := make(map[utiliptables.Chain]string)
-	iptablesSaveRaw, err := h.iptables.Save(utiliptables.TableNAT)
+	iptablesData := bytes.NewBuffer(nil)
+	err = h.iptables.SaveInto(utiliptables.TableNAT, iptablesData)
 	if err != nil { // if we failed to get any rules
 		glog.Errorf("Failed to execute iptables-save, syncing all rules: %v", err)
 	} else { // otherwise parse the output
-		existingNATChains = utiliptables.GetChainLines(utiliptables.TableNAT, iptablesSaveRaw)
+		existingNATChains = utiliptables.GetChainLines(utiliptables.TableNAT, iptablesData.Bytes())
 	}
 
 	natChains := bytes.NewBuffer(nil)

pkg/proxy/iptables/proxier.go

@@ -527,11 +527,12 @@ func CleanupLeftovers(ipt utiliptables.Interface) (encounteredError bool) {
 	}
 
 	// Flush and remove all of our chains.
-	if iptablesSaveRaw, err := ipt.Save(utiliptables.TableNAT); err != nil {
+	iptablesData := bytes.NewBuffer(nil)
+	if err := ipt.SaveInto(utiliptables.TableNAT, iptablesData); err != nil {
 		glog.Errorf("Failed to execute iptables-save for %s: %v", utiliptables.TableNAT, err)
 		encounteredError = true
 	} else {
-		existingNATChains := utiliptables.GetChainLines(utiliptables.TableNAT, iptablesSaveRaw)
+		existingNATChains := utiliptables.GetChainLines(utiliptables.TableNAT, iptablesData.Bytes())
 		natChains := bytes.NewBuffer(nil)
 		natRules := bytes.NewBuffer(nil)
 		writeLine(natChains, "*nat")

pkg/util/exec/fake_exec.go

@@ -49,6 +49,9 @@ type FakeCmd struct {
 	CombinedOutputScript []FakeCombinedOutputAction
 	CombinedOutputCalls  int
 	CombinedOutputLog    [][]string
+	RunScript            []FakeRunAction
+	RunCalls             int
+	RunLog               [][]string
 	Dirs                 []string
 	Stdin                io.Reader
 	Stdout               io.Writer
@@ -61,6 +64,7 @@ func InitFakeCmd(fake *FakeCmd, cmd string, args ...string) Cmd {
 }
 
 type FakeCombinedOutputAction func() ([]byte, error)
+type FakeRunAction func() ([]byte, []byte, error)
 
 func (fake *FakeCmd) SetDir(dir string) {
 	fake.Dirs = append(fake.Dirs, dir)
@@ -79,7 +83,23 @@ func (fake *FakeCmd) SetStderr(out io.Writer) {
 }
 
 func (fake *FakeCmd) Run() error {
-	return fmt.Errorf("unimplemented")
+	if fake.RunCalls > len(fake.RunScript)-1 {
+		panic("ran out of Run() actions")
+	}
+	if fake.RunLog == nil {
+		fake.RunLog = [][]string{}
+	}
+	i := fake.RunCalls
+	fake.RunLog = append(fake.RunLog, append([]string{}, fake.Argv...))
+	fake.RunCalls++
+	stdout, stderr, err := fake.RunScript[i]()
+	if stdout != nil {
+		fake.Stdout.Write(stdout)
+	}
+	if stderr != nil {
+		fake.Stderr.Write(stderr)
+	}
+	return err
 }
 
 func (fake *FakeCmd) CombinedOutput() ([]byte, error) {

pkg/util/iptables/iptables.go

@@ -54,13 +54,11 @@ 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)
 	// SaveInto calls `iptables-save` for table and stores result in a given buffer.
 	SaveInto(table Table, buffer *bytes.Buffer) error
 	// Restore runs `iptables-restore` passing data through []byte.
 	// table is the Table to restore
-	// data should be formatted like the output of Save()
+	// data should be formatted like the output of SaveInto()
 	// 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
@@ -306,17 +304,6 @@ 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)}
-	glog.V(4).Infof("running iptables-save %v", args)
-	return runner.exec.Command(cmdIPTablesSave, args...).CombinedOutput()
-}
-
 // SaveInto is part of Interface.
 func (runner *runner) SaveInto(table Table, buffer *bytes.Buffer) error {
 	runner.mu.Lock()

pkg/util/iptables/iptables_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package iptables
 
 import (
+	"bytes"
 	"net"
 	"os"
 	"strings"
@@ -831,7 +832,7 @@ func TestReload(t *testing.T) {
 	}
 }
 
-func TestSave(t *testing.T) {
+func TestSaveInto(t *testing.T) {
 	output := `# Generated by iptables-save v1.6.0 on Thu Jan 19 11:38:09 2017
 *filter
 :INPUT ACCEPT [15079:38410730]
@@ -846,8 +847,10 @@ COMMIT
 			func() ([]byte, error) { return []byte("iptables v1.9.22"), nil },
 			// iptables-restore version check
 			func() ([]byte, error) { return []byte("iptables-restore v1.9.22"), nil },
-			func() ([]byte, error) { return []byte(output), nil },
+		},
-			func() ([]byte, error) { return nil, &exec.FakeExitError{Status: 1} },
+		RunScript: []exec.FakeRunAction{
+			func() ([]byte, []byte, error) { return []byte(output), nil, nil },
+			func() ([]byte, []byte, error) { return nil, nil, &exec.FakeExitError{Status: 1} },
 		},
 	}
 	fexec := exec.FakeExec{
@@ -860,25 +863,31 @@ COMMIT
 	}
 	runner := New(&fexec, dbus.NewFake(nil, nil), ProtocolIpv4)
 	defer runner.Destroy()
+	buffer := bytes.NewBuffer(nil)
+
 	// Success.
-	o, err := runner.Save(TableNAT)
+	err := runner.SaveInto(TableNAT, buffer)
 	if err != nil {
 		t.Fatalf("expected success, got %v", err)
 	}
 
-	if string(o[:len(output)]) != output {
+	if string(buffer.Bytes()[:len(output)]) != output {
-		t.Errorf("expected output to be equal to mocked one, got %v", o)
+		t.Errorf("expected output to be equal to mocked one, got %v", buffer.Bytes())
 	}
 
-	if fcmd.CombinedOutputCalls != 3 {
+	if fcmd.CombinedOutputCalls != 2 {
-		t.Errorf("expected 3 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls)
+		t.Errorf("expected 2 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls)
 	}
-	if !sets.NewString(fcmd.CombinedOutputLog[2]...).HasAll("iptables-save", "-t", "nat") {
+	if fcmd.RunCalls != 1 {
-		t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[2])
+		t.Errorf("expected 1 Run() call, got %d", fcmd.RunCalls)
+	}
+	if !sets.NewString(fcmd.RunLog[0]...).HasAll("iptables-save", "-t", "nat") {
+		t.Errorf("wrong Run() log, got %s", fcmd.RunLog[0])
 	}
 
 	// Failure.
-	_, err = runner.Save(TableNAT)
+	buffer.Reset()
+	err = runner.SaveInto(TableNAT, buffer)
 	if err == nil {
 		t.Errorf("expected failure")
 	}