From df23697ae7876703db8912812377377c99953151 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20Luk=C5=A1a?= Date: Tue, 28 May 2019 11:38:27 +0200 Subject: [PATCH 1/3] Better error message if panic occurs during iptables-save output parsing --- pkg/util/iptables/save_restore.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/pkg/util/iptables/save_restore.go b/pkg/util/iptables/save_restore.go index 172f07e7fa..7ec11e4f00 100644 --- a/pkg/util/iptables/save_restore.go +++ b/pkg/util/iptables/save_restore.go @@ -61,7 +61,11 @@ func GetChainLines(table Table, save []byte) map[Chain][]byte { } else if line[0] == ':' && len(line) > 1 { // We assume that the contains space - chain lines have 3 fields, // space delimited. If there is no space, this line will panic. - chain := Chain(line[1:bytes.Index(line, spaceBytes)]) + spaceIndex := bytes.Index(line, spaceBytes) + if spaceIndex == -1 { + panic(fmt.Sprintf("Unexpected chain line in iptables-save output: %v", string(line))) + } + chain := Chain(line[1:spaceIndex]) chainsMap[chain] = line } } From 00e750561870697a9a51c5653b7c48143f664401 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20Luk=C5=A1a?= Date: Tue, 28 May 2019 14:43:28 +0200 Subject: [PATCH 2/3] Discard stderr output when calling iptables-save --- pkg/util/iptables/iptables.go | 3 ++- pkg/util/iptables/iptables_test.go | 8 +++++--- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/pkg/util/iptables/iptables.go b/pkg/util/iptables/iptables.go index 24d2ef4ef3..40fde4a162 100644 --- a/pkg/util/iptables/iptables.go +++ b/pkg/util/iptables/iptables.go @@ -20,6 +20,7 @@ import ( "bytes" "context" "fmt" + "io/ioutil" "regexp" "strings" "sync" @@ -332,7 +333,7 @@ func (runner *runner) SaveInto(table Table, buffer *bytes.Buffer) error { // creates a new buffer, redirects stdout and stderr to it and also // calls Run()]. cmd.SetStdout(buffer) - cmd.SetStderr(buffer) + cmd.SetStderr(ioutil.Discard) return cmd.Run() } diff --git a/pkg/util/iptables/iptables_test.go b/pkg/util/iptables/iptables_test.go index e169468062..73240939ec 100644 --- a/pkg/util/iptables/iptables_test.go +++ b/pkg/util/iptables/iptables_test.go @@ -932,6 +932,8 @@ func testSaveInto(t *testing.T, protocol Protocol) { COMMIT # Completed on Thu Jan 19 11:38:09 2017`, iptablesSaveCmd+version) + stderrOutput := "#STDERR OUTPUT" // SaveInto() should should NOT capture stderr into the buffer + fcmd := fakeexec.FakeCmd{ CombinedOutputScript: []fakeexec.FakeCombinedOutputAction{ // iptables version check @@ -940,7 +942,7 @@ COMMIT func() ([]byte, error) { return []byte(iptablesRestoreCmd + version), nil }, }, RunScript: []fakeexec.FakeRunAction{ - func() ([]byte, []byte, error) { return []byte(output), nil, nil }, + func() ([]byte, []byte, error) { return []byte(output), []byte(stderrOutput), nil }, func() ([]byte, []byte, error) { return nil, nil, &fakeexec.FakeExitError{Status: 1} }, }, } @@ -962,8 +964,8 @@ COMMIT t.Fatalf("%s: Expected success, got %v", protoStr, err) } - if string(buffer.Bytes()[:len(output)]) != output { - t.Errorf("%s: Expected output '%s', got '%v'", protoStr, output, buffer.Bytes()) + if string(buffer.Bytes()) != output { + t.Errorf("%s: Expected output '%s', got '%v'", protoStr, output, string(buffer.Bytes())) } if fcmd.CombinedOutputCalls != 2 { From 93a549679fcdc949a0fcddb296a70b25197698ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20Luk=C5=A1a?= Date: Tue, 28 May 2019 17:09:29 +0200 Subject: [PATCH 3/3] Capture stderr output and write it to buffer on error --- pkg/util/iptables/iptables.go | 16 ++++++++-------- pkg/util/iptables/iptables_test.go | 5 ++++- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/pkg/util/iptables/iptables.go b/pkg/util/iptables/iptables.go index 40fde4a162..761e55c94a 100644 --- a/pkg/util/iptables/iptables.go +++ b/pkg/util/iptables/iptables.go @@ -20,7 +20,6 @@ import ( "bytes" "context" "fmt" - "io/ioutil" "regexp" "strings" "sync" @@ -327,14 +326,15 @@ func (runner *runner) SaveInto(table Table, buffer *bytes.Buffer) error { args := []string{"-t", string(table)} klog.V(4).Infof("running %s %v", iptablesSaveCmd, args) cmd := runner.exec.Command(iptablesSaveCmd, args...) - // Since CombinedOutput() doesn't support redirecting it to a buffer, - // we need to workaround it by redirecting stdout and stderr to buffer - // and explicitly calling Run() [CombinedOutput() underneath itself - // creates a new buffer, redirects stdout and stderr to it and also - // calls Run()]. cmd.SetStdout(buffer) - cmd.SetStderr(ioutil.Discard) - return cmd.Run() + stderrBuffer := bytes.NewBuffer(nil) + cmd.SetStderr(stderrBuffer) + + err := cmd.Run() + if err != nil { + stderrBuffer.WriteTo(buffer) // ignore error, since we need to return the original error + } + return err } // Restore is part of Interface. diff --git a/pkg/util/iptables/iptables_test.go b/pkg/util/iptables/iptables_test.go index 73240939ec..dfc5809538 100644 --- a/pkg/util/iptables/iptables_test.go +++ b/pkg/util/iptables/iptables_test.go @@ -943,7 +943,7 @@ COMMIT }, RunScript: []fakeexec.FakeRunAction{ func() ([]byte, []byte, error) { return []byte(output), []byte(stderrOutput), nil }, - func() ([]byte, []byte, error) { return nil, nil, &fakeexec.FakeExitError{Status: 1} }, + func() ([]byte, []byte, error) { return nil, []byte(stderrOutput), &fakeexec.FakeExitError{Status: 1} }, }, } fexec := fakeexec.FakeExec{ @@ -984,6 +984,9 @@ COMMIT if err == nil { t.Errorf("%s: Expected failure", protoStr) } + if string(buffer.Bytes()) != stderrOutput { + t.Errorf("%s: Expected output '%s', got '%v'", protoStr, stderrOutput, string(buffer.Bytes())) + } } func TestSaveIntoIPv4(t *testing.T) {