Merge pull request #6278 from akram/fix_port_forward_listner_parsing_ipv4_ipv6

Fixes an issue with hosts having an IPv6 address on localhost

pkg/client/portforward/portforward.go

@@ -183,21 +183,13 @@ func (pf *PortForwarder) forward() error {
 	return nil
 }
 
-// listenOnPort creates a new listener on port and waits for new connections
+// listenOnPort delegates listener creation and waits for new connections
 // in the background.
 func (pf *PortForwarder) listenOnPort(port *ForwardedPort) error {
-	listener, err := net.Listen("tcp", fmt.Sprintf("localhost:%d", port.Local))
+	listener, err := pf.getListener("tcp", "localhost", port)
 	if err != nil {
 		return err
 	}
-	parts := strings.Split(listener.Addr().String(), ":")
-	localPort, err := strconv.ParseUint(parts[1], 10, 16)
-	if err != nil {
-		return fmt.Errorf("Error parsing local part: %s", err)
-	}
-	port.Local = uint16(localPort)
-	glog.Infof("Forwarding from %d -> %d", localPort, port.Remote)
-
 	pf.listeners = append(pf.listeners, listener)
 
 	go pf.waitForConnection(listener, *port)
@@ -205,6 +197,27 @@ func (pf *PortForwarder) listenOnPort(port *ForwardedPort) error {
 	return nil
 }
 
+// getListener creates a listener on the interface targeted by the given hostname on the given port with
+// the given protocol. protocol is in net.Listen style which basically admits values like tcp, tcp4, tcp6
+func (pf *PortForwarder) getListener(protocol string, hostname string, port *ForwardedPort) (net.Listener, error) {
+	listener, err := net.Listen(protocol, fmt.Sprintf("%s:%d", hostname, port.Local))
+	if err != nil {
+		glog.Errorf("Unable to create listener: Error %s", err)
+		return nil, err
+	}
+	listenerAddress := listener.Addr().String()
+	host, localPort, _ := net.SplitHostPort(listenerAddress)
+	localPortUInt, err := strconv.ParseUint(localPort, 10, 16)
+
+	if err != nil {
+		return nil, fmt.Errorf("Error parsing local port: %s from %s (%s)", err, listenerAddress, host)
+	}
+	port.Local = uint16(localPortUInt)
+	glog.Infof("Forwarding from %d -> %d", localPortUInt, port.Remote)
+
+	return listener, nil
+}
+
 // waitForConnection waits for new connections to listener and handles them in
 // the background.
 func (pf *PortForwarder) waitForConnection(listener net.Listener, port ForwardedPort) {

pkg/client/portforward/portforward_test.go

@@ -209,6 +209,82 @@ func (s *fakeUpgradeStream) Headers() http.Header {
 	return http.Header{}
 }
 
+func TestGetListener(t *testing.T) {
+	var pf PortForwarder
+	testCases := []struct {
+		Hostname                string
+		Protocol                string
+		ShouldRaiseError        bool
+		ExpectedListenerAddress string
+	}{
+		{
+			Hostname:                "localhost",
+			Protocol:                "tcp4",
+			ShouldRaiseError:        false,
+			ExpectedListenerAddress: "127.0.0.1",
+		},
+		{
+			Hostname:                "127.0.0.1",
+			Protocol:                "tcp4",
+			ShouldRaiseError:        false,
+			ExpectedListenerAddress: "127.0.0.1",
+		},
+		{
+			Hostname:                "[::1]",
+			Protocol:                "tcp6",
+			ShouldRaiseError:        false,
+			ExpectedListenerAddress: "::1",
+		},
+		{
+			Hostname:                "localhost",
+			Protocol:                "tcp6",
+			ShouldRaiseError:        false,
+			ExpectedListenerAddress: "::1",
+		},
+		{
+			Hostname:         "[::1]",
+			Protocol:         "tcp4",
+			ShouldRaiseError: true,
+		},
+		{
+			Hostname:         "127.0.0.1",
+			Protocol:         "tcp6",
+			ShouldRaiseError: true,
+		},
+	}
+
+	for i, testCase := range testCases {
+		expectedListenerPort := "12345"
+		listener, err := pf.getListener(testCase.Protocol, testCase.Hostname, &ForwardedPort{12345, 12345})
+		errorRaised := err != nil
+
+		if testCase.ShouldRaiseError != errorRaised {
+			t.Errorf("Test case #%d failed: Data %v an error has been raised(%t) where it should not (or reciprocally): %v", i, testCase, testCase.ShouldRaiseError, err)
+			continue
+		}
+		if errorRaised {
+			continue
+		}
+
+		if listener == nil {
+			t.Errorf("Test case #%d did not raised an error (%t) but failed in initializing listener", i, err)
+			continue
+		}
+
+		host, port, _ := net.SplitHostPort(listener.Addr().String())
+		t.Logf("Asked a %s forward for: %s:%v, got listener %s:%s, expected: %s", testCase.Protocol, testCase.Hostname, 12345, host, port, expectedListenerPort)
+		if host != testCase.ExpectedListenerAddress {
+			t.Errorf("Test case #%d failed: Listener does not listen on exepected address: asked %v got %v", i, testCase.ExpectedListenerAddress, host)
+		}
+		if port != expectedListenerPort {
+			t.Errorf("Test case #%d failed: Listener does not listen on exepected port: asked %v got %v", i, expectedListenerPort, port)
+
+		}
+		listener.Close()
+
+	}
+}
+
 func TestForwardPorts(t *testing.T) {
 	testCases := []struct {
 		Upgrader *fakeUpgrader
@@ -313,4 +389,5 @@ func TestForwardPorts(t *testing.T) {
 			t.Fatalf("%d: expected conn closure", i)
 		}
 	}
+
 }

pkg/kubelet/dockertools/docker.go

@@ -295,7 +295,11 @@ func (d *dockerContainerCommandRunner) PortForward(pod *kubecontainer.Pod, port
 	}
 
 	containerPid := container.State.Pid
-	// TODO use exec.LookPath for socat / what if the host doesn't have it???
+	// TODO what if the host doesn't have it???
+	_, lookupErr := exec.LookPath("socat")
+	if lookupErr != nil {
+		return fmt.Errorf("Unable to do port forwarding: socat not found.")
+	}
 	args := []string{"-t", fmt.Sprintf("%d", containerPid), "-n", "socat", "-", fmt.Sprintf("TCP4:localhost:%d", port)}
 	// TODO use exec.LookPath
 	command := exec.Command("nsenter", args...)