From b7fc22be8a52b63bb74fa50ff80e6177c2ae42f2 Mon Sep 17 00:00:00 2001 From: Akram Ben Aissi Date: Tue, 21 Apr 2015 20:15:54 +0200 Subject: [PATCH] Fixes an issue with hosts having an IPv6 address on localhost - When 'getent hosts localhost' returns '::1' the creation of the listener fails because of the port parsing which uses ":" as a separator - Use of net.SplitHostPort() to do the job - Adding unit tests to ensure that the creation succeeds - On docker.go: adds a test on the presence the socat command which was failing silenty if not installed - Code Review 1 - Fixed typo on Expected - The UT now fails if the PortForwarder could not be created - Code Review 2 - Simplify socat error message - Changing t.Fatal to to.Error on unit tests - Code Review 3 - Removing useless uses cases in unit tests - Code Review 4 - Removing useless initiliasiation of PortForwarder - Changing error message - Code Review 5 - Simplifying TestCast struct - Adding addition test in one test case - Closing the listener - Code Review 6 - Improving unit test --- pkg/client/portforward/portforward.go | 33 +++++++--- pkg/client/portforward/portforward_test.go | 77 ++++++++++++++++++++++ pkg/kubelet/dockertools/docker.go | 6 +- 3 files changed, 105 insertions(+), 11 deletions(-) diff --git a/pkg/client/portforward/portforward.go b/pkg/client/portforward/portforward.go index 6cf92c54173..73d39dfcfaa 100644 --- a/pkg/client/portforward/portforward.go +++ b/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) { diff --git a/pkg/client/portforward/portforward_test.go b/pkg/client/portforward/portforward_test.go index 99d4ee43078..00132db46d8 100644 --- a/pkg/client/portforward/portforward_test.go +++ b/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) } } + } diff --git a/pkg/kubelet/dockertools/docker.go b/pkg/kubelet/dockertools/docker.go index 2f0a7d1c655..384ed0739c6 100644 --- a/pkg/kubelet/dockertools/docker.go +++ b/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...)