Revert "Use pod ip instead of localhost in pod netns for portforward."

This reverts commit dd886bc281.

Signed-off-by: Lantao Liu <lantaol@google.com>

pkg/server/sandbox_portforward.go

@@ -23,6 +23,7 @@ import (
 	"os/exec"
 	"strings"
 
+	"github.com/containernetworking/plugins/pkg/ns"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 	"golang.org/x/net/context"
@@ -44,28 +45,31 @@ func (c *criService) PortForward(ctx context.Context, r *runtime.PortForwardRequ
 	return c.streamServer.GetPortForward(r)
 }
 
-// portForward requires `socat` on the node. It runs `socat` to forward stream for a
+// portForward requires `socat` on the node. It uses netns to enter the sandbox namespace,
-// specific port. The `socat` command keeps running until it exits or client disconnect.
+// and run `socat` insidethe namespace to forward stream for a specific port. The `socat`
+// command keeps running until it exits or client disconnect.
 func (c *criService) portForward(id string, port int32, stream io.ReadWriteCloser) error {
 	s, err := c.sandboxStore.Get(id)
 	if err != nil {
 		return errors.Wrapf(err, "failed to find sandbox %q in store", id)
 	}
-	if s.Status.Get().State != sandboxstore.StateReady {
+	var netNSDo func(func(ns.NetNS) error) error
-		return errors.Wrap(err, "sandbox container is not running")
+	// netNSPath is the network namespace path for logging.
-	}
+	var netNSPath string
-	// We use pod ip instead of netns.Do + localhost. One reason is that
-	// for sandbox container e.g. gvisor and kata, the network stack is
-	// inside sandbox, netns.Do + localhost will not work. However, accessing
-	// pod ip from host should always work, because it is an assumption of
-	// Kubernetes network model. (Related issue containerd/cri-containerd#524)
-	var addr string
 	securityContext := s.Config.GetLinux().GetSecurityContext()
 	hostNet := securityContext.GetNamespaceOptions().GetNetwork() == runtime.NamespaceMode_NODE
 	if !hostNet {
-		addr = s.IP
+		if s.NetNS == nil || s.NetNS.Closed() {
+			return errors.Errorf("network namespace for sandbox %q is closed", id)
+		}
+		netNSDo = s.NetNS.GetNs().Do
+		netNSPath = s.NetNS.GetPath()
 	} else {
-		addr = "localhost"
+		// Run the function directly for host network.
+		netNSDo = func(do func(_ ns.NetNS) error) error {
+			return do(nil)
+		}
+		netNSPath = "host"
 	}
 
 	socat, err := exec.LookPath("socat")
@@ -74,39 +78,46 @@ func (c *criService) portForward(id string, port int32, stream io.ReadWriteClose
 	}
 
 	// Check https://linux.die.net/man/1/socat for meaning of the options.
-	args := []string{socat, "-", fmt.Sprintf("TCP4:%s:%d", addr, port)}
+	args := []string{socat, "-", fmt.Sprintf("TCP4:localhost:%d", port)}
 
-	logrus.Infof("Executing port forwarding command %q", strings.Join(args, " "))
+	logrus.Infof("Executing port forwarding command %q in network namespace %q", strings.Join(args, " "), netNSPath)
-	cmd := exec.Command(args[0], args[1:]...)
+	err = netNSDo(func(_ ns.NetNS) error {
-	cmd.Stdout = stream
+		cmd := exec.Command(args[0], args[1:]...)
+		cmd.Stdout = stream
 
-	stderr := new(bytes.Buffer)
+		stderr := new(bytes.Buffer)
-	cmd.Stderr = stderr
+		cmd.Stderr = stderr
 
-	// If we use Stdin, command.Run() won't return until the goroutine that's copying
+		// If we use Stdin, command.Run() won't return until the goroutine that's copying
-	// from stream finishes. Unfortunately, if you have a client like telnet connected
+		// from stream finishes. Unfortunately, if you have a client like telnet connected
-	// via port forwarding, as long as the user's telnet client is connected to the user's
+		// via port forwarding, as long as the user's telnet client is connected to the user's
-	// local listener that port forwarding sets up, the telnet session never exits. This
+		// local listener that port forwarding sets up, the telnet session never exits. This
-	// means that even if socat has finished running, command.Run() won't ever return
+		// means that even if socat has finished running, command.Run() won't ever return
-	// (because the client still has the connection and stream open).
+		// (because the client still has the connection and stream open).
-	//
+		//
-	// The work around is to use StdinPipe(), as Wait() (called by Run()) closes the pipe
+		// The work around is to use StdinPipe(), as Wait() (called by Run()) closes the pipe
-	// when the command (socat) exits.
+		// when the command (socat) exits.
-	in, err := cmd.StdinPipe()
+		in, err := cmd.StdinPipe()
-	if err != nil {
+		if err != nil {
-		return errors.Wrap(err, "failed to create stdin pipe")
+			return errors.Wrap(err, "failed to create stdin pipe")
-	}
-	go func() {
-		if _, err := io.Copy(in, stream); err != nil {
-			logrus.WithError(err).Errorf("Failed to copy port forward input for %q port %d", id, port)
 		}
-		in.Close()
+		go func() {
-		logrus.Debugf("Finish copying port forward input for %q port %d", id, port)
+			if _, err := io.Copy(in, stream); err != nil {
-	}()
+				logrus.WithError(err).Errorf("Failed to copy port forward input for %q port %d", id, port)
+			}
+			in.Close()
+			logrus.Debugf("Finish copying port forward input for %q port %d", id, port)
+		}()
 
-	if err := cmd.Run(); err != nil {
+		if err := cmd.Run(); err != nil {
-		return errors.Errorf("socat command returns error: %v, stderr: %q", err, stderr.String())
+			return errors.Errorf("socat command returns error: %v, stderr: %q", err, stderr.String())
+		}
+		return nil
+	})
+	if err != nil {
+		return errors.Wrapf(err, "failed to execute portforward in network namespace %q", netNSPath)
 	}
 	logrus.Infof("Finish port forwarding for %q port %d", id, port)
+
 	return nil
 }