k8s: the default host (localhost) should get the default port-forward…

…ing behavior

The tiltfile evaluator always defaults the empty string to "localhost".
We want to make sure that this goes down the default kubernetes binding
codepath instead of our custom codepath to resolve hostnames.

Fixes tilt-dev#5981

Signed-off-by: Nick Santos <nick.santos@docker.com>

internal/k8s/portforward.go

@@ -9,9 +9,8 @@ import (
 	"strconv"
 	"strings"
 
-	v1 "k8s.io/client-go/kubernetes/typed/core/v1"
+	"k8s.io/apimachinery/pkg/util/httpstream"
 	_ "k8s.io/client-go/plugin/pkg/client/auth/gcp" // registers gcp auth provider
-	"k8s.io/client-go/rest"
 	"k8s.io/client-go/transport/spdy"
 
 	"github.com/tilt-dev/tilt/internal/k8s/portforward"
@@ -86,9 +85,10 @@ func (k *K8sClient) CreatePortForwarder(ctx context.Context, namespace Namespace
 	return k.portForwardClient.CreatePortForwarder(ctx, namespace, podID, localPort, remotePort, host)
 }
 
+type newPodDialerFn func(namespace Namespace, podID PodID) (httpstream.Dialer, error)
+
 type portForwardClient struct {
-	config *rest.Config
-	core   v1.CoreV1Interface
+	newPodDialer newPodDialerFn
 }
 
 func ProvidePortForwardClient(
@@ -100,32 +100,46 @@ func ProvidePortForwardClient(
 	if maybeClientset.Error != nil {
 		return explodingPortForwardClient{error: maybeClientset.Error}
 	}
+
+	config := maybeRESTConfig.Config
+	core := maybeClientset.Clientset.CoreV1()
+	newPodDialer := newPodDialerFn(func(namespace Namespace, podID PodID) (httpstream.Dialer, error) {
+		transport, upgrader, err := spdy.RoundTripperFor(config)
+		if err != nil {
+			return nil, errors.Wrap(err, "error getting roundtripper")
+		}
+
+		req := core.RESTClient().Post().
+			Resource("pods").
+			Namespace(namespace.String()).
+			Name(podID.String()).
+			SubResource("portforward")
+
+		dialer := spdy.NewDialer(upgrader, &http.Client{Transport: transport}, "POST", req.URL())
+		return dialer, nil
+	})
+
 	return portForwardClient{
-		maybeRESTConfig.Config,
-		maybeClientset.Clientset.CoreV1(),
+		newPodDialer: newPodDialer,
 	}
 }
 
 func (c portForwardClient) CreatePortForwarder(ctx context.Context, namespace Namespace, podID PodID, localPort int, remotePort int, host string) (PortForwarder, error) {
-	transport, upgrader, err := spdy.RoundTripperFor(c.config)
+	dialer, err := c.newPodDialer(namespace, podID)
 	if err != nil {
-		return nil, errors.Wrap(err, "error getting roundtripper")
+		return nil, err
 	}
-
-	req := c.core.RESTClient().Post().
-		Resource("pods").
-		Namespace(namespace.String()).
-		Name(podID.String()).
-		SubResource("portforward")
-
-	dialer := spdy.NewDialer(upgrader, &http.Client{Transport: transport}, "POST", req.URL())
-
 	readyChan := make(chan struct{}, 1)
 
 	ports := []string{fmt.Sprintf("%d:%d", localPort, remotePort)}
 
 	var pf *portforward.PortForwarder
-	if host == "" {
+
+	// The tiltfile evaluator always defaults the empty string.
+	//
+	// If it's defaulting to localhost, use the default kubernetse logic
+	// for binding the portforward.
+	if host == "" || host == "localhost" {
 		pf, err = portforward.New(
 			ctx,
 			dialer,

internal/k8s/portforward_test.go

@@ -0,0 +1,61 @@
+package k8s
+
+import (
+	"context"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"k8s.io/apimachinery/pkg/util/httpstream"
+)
+
+type fakeDialer struct {
+	dialed             bool
+	conn               httpstream.Connection
+	err                error
+	negotiatedProtocol string
+}
+
+func (d *fakeDialer) Dial(protocols ...string) (httpstream.Connection, string, error) {
+	d.dialed = true
+	return d.conn, d.negotiatedProtocol, d.err
+}
+
+var fakeNewPodDialer = newPodDialerFn(func(namespace Namespace, podID PodID) (httpstream.Dialer, error) {
+	return &fakeDialer{}, nil
+})
+
+func TestPortForwardEmptyHost(t *testing.T) {
+	client := portForwardClient{newPodDialer: fakeNewPodDialer}
+	pf, err := client.CreatePortForwarder(context.Background(), "default", "podid", 8080, 8080, "")
+	assert.NoError(t, err)
+	assert.Equal(t, []string{"127.0.0.1", "::1"}, pf.Addresses())
+}
+
+func TestPortForwardLocalhost(t *testing.T) {
+	client := portForwardClient{newPodDialer: fakeNewPodDialer}
+	pf, err := client.CreatePortForwarder(context.Background(), "default", "podid", 8080, 8080, "localhost")
+	assert.NoError(t, err)
+	assert.Equal(t, []string{"127.0.0.1", "::1"}, pf.Addresses())
+}
+
+func TestPortForwardInvalidDomain(t *testing.T) {
+	client := portForwardClient{newPodDialer: fakeNewPodDialer}
+	_, err := client.CreatePortForwarder(context.Background(), "default", "podid", 8080, 8080, "domain.invalid")
+	if assert.Error(t, err) {
+		assert.Contains(t, err.Error(), "failed to look up address for domain.invalid")
+	}
+}
+
+func TestPortForwardAllHosts(t *testing.T) {
+	client := portForwardClient{newPodDialer: fakeNewPodDialer}
+	pf, err := client.CreatePortForwarder(context.Background(), "default", "podid", 8080, 8080, "0.0.0.0")
+	assert.NoError(t, err)
+	assert.Equal(t, []string{"0.0.0.0"}, pf.Addresses())
+}
+
+func TestPortForwardIPv6Loopback(t *testing.T) {
+	client := portForwardClient{newPodDialer: fakeNewPodDialer}
+	pf, err := client.CreatePortForwarder(context.Background(), "default", "podid", 8080, 8080, "[::1]")
+	assert.NoError(t, err)
+	assert.Equal(t, []string{"::1"}, pf.Addresses())
+}

pkg/apis/core/v1alpha1/portforward_types.go

@@ -86,7 +86,9 @@ type Forward struct {
 	// The port on the Kubernetes pod to connect to. Required.
 	ContainerPort int32 `json:"containerPort" protobuf:"varint,3,opt,name=containerPort"`
 
-	// Optional host to bind to on the current machine (localhost by default)
+	// Optional host to bind to on the current machine.
+	//
+	// If not explicitly specified, uses the bind host of the tilt web UI (usually localhost).
 	//
 	// +optional
 	Host string `json:"host" protobuf:"bytes,5,opt,name=host"`