Merge pull request #78428 from luksa/iptables-save-panic

Fix panic in kube-proxy when iptables-save prints to stderr

pkg/util/iptables/iptables.go

@@ -326,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(buffer)
-	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.

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,8 +942,8 @@ 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 nil, nil, &fakeexec.FakeExitError{Status: 1} },
+			func() ([]byte, []byte, error) { return []byte(output), []byte(stderrOutput), nil },
+			func() ([]byte, []byte, error) { return nil, []byte(stderrOutput), &fakeexec.FakeExitError{Status: 1} },
 		},
 	}
 	fexec := fakeexec.FakeExec{
@@ -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 {
@@ -982,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) {

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 <line> 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
 		}
 	}