Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tiltfile: custom host in port forwards #2382

Merged
merged 2 commits into from
Oct 21, 2019
Merged

tiltfile: custom host in port forwards #2382

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5199546
tiltfile: add test cases for host in port forwards
dolsem Oct 19, 2019
a62c1fe
tiltfile: custom host in port forwards
dolsem Oct 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion internal/containerupdate/synclet_manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ func newSyncletClient(ctx context.Context, kCli k8s.Client, syncletImageRef side

// TODO(nick): We need a better way to kill the client when the pod dies.
ctx, cancel := context.WithCancel(ctx)
pf, err := kCli.CreatePortForwarder(ctx, ns, podID, 0, synclet.Port)
pf, err := kCli.CreatePortForwarder(ctx, ns, podID, 0, synclet.Port, nil)
if err != nil {
cancel()
return nil, errors.Wrapf(err, "failed opening tunnel to synclet pod '%s'", podID)
Expand Down
2 changes: 1 addition & 1 deletion internal/engine/portforwardcontroller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ func (m *PortForwardController) onePortForward(ctx context.Context, entry portFo
ns := entry.namespace
podID := entry.podID

pf, err := m.kClient.CreatePortForwarder(ctx, ns, podID, forward.LocalPort, forward.ContainerPort)
pf, err := m.kClient.CreatePortForwarder(ctx, ns, podID, forward.LocalPort, forward.ContainerPort, forward.Host)
if err != nil {
return err
}
Expand Down
2 changes: 1 addition & 1 deletion internal/k8s/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ type Client interface {
ContainerLogs(ctx context.Context, podID PodID, cName container.Name, n Namespace, startTime time.Time) (io.ReadCloser, error)

// Opens a tunnel to the specified pod+port. Returns the tunnel's local port and a function that closes the tunnel
CreatePortForwarder(ctx context.Context, namespace Namespace, podID PodID, optionalLocalPort, remotePort int) (PortForwarder, error)
CreatePortForwarder(ctx context.Context, namespace Namespace, podID PodID, optionalLocalPort, remotePort int, host *string) (PortForwarder, error)

WatchPods(ctx context.Context, lps labels.Selector) (<-chan *v1.Pod, error)

Expand Down
2 changes: 1 addition & 1 deletion internal/k8s/exploding_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ func (ec *explodingClient) ContainerLogs(ctx context.Context, podID PodID, cName
return nil, errors.Wrap(ec.err, "could not set up k8s client")
}

func (ec *explodingClient) CreatePortForwarder(ctx context.Context, namespace Namespace, podID PodID, optionalLocalPort, remotePort int) (PortForwarder, error) {
func (ec *explodingClient) CreatePortForwarder(ctx context.Context, namespace Namespace, podID PodID, optionalLocalPort, remotePort int, host *string) (PortForwarder, error) {
return nil, errors.Wrap(ec.err, "could not set up k8s client")
}

Expand Down
8 changes: 5 additions & 3 deletions internal/k8s/fake_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,9 +323,9 @@ func (c *FakeK8sClient) applyWasCalled() bool {
return c.Yaml != ""
}

func (c *FakeK8sClient) CreatePortForwarder(ctx context.Context, namespace Namespace, podID PodID, optionalLocalPort, remotePort int) (PortForwarder, error) {
func (c *FakeK8sClient) CreatePortForwarder(ctx context.Context, namespace Namespace, podID PodID, optionalLocalPort, remotePort int, host *string) (PortForwarder, error) {
pfc := &(c.FakePortForwardClient)
return pfc.CreatePortForwarder(ctx, namespace, podID, optionalLocalPort, remotePort)
return pfc.CreatePortForwarder(ctx, namespace, podID, optionalLocalPort, remotePort, host)
}

func (c *FakeK8sClient) ContainerRuntime(ctx context.Context) container.Runtime {
Expand Down Expand Up @@ -398,15 +398,17 @@ type FakePortForwardClient struct {
CreatePortForwardCallCount int
LastForwardPortPodID PodID
LastForwardPortRemotePort int
LastForwardPortHost *string
LastForwarder FakePortForwarder
LastForwardContext context.Context
}

func (c *FakePortForwardClient) CreatePortForwarder(ctx context.Context, namespace Namespace, podID PodID, optionalLocalPort, remotePort int) (PortForwarder, error) {
func (c *FakePortForwardClient) CreatePortForwarder(ctx context.Context, namespace Namespace, podID PodID, optionalLocalPort, remotePort int, host *string) (PortForwarder, error) {
c.CreatePortForwardCallCount++
c.LastForwardContext = ctx
c.LastForwardPortPodID = podID
c.LastForwardPortRemotePort = remotePort
c.LastForwardPortHost = host

result := FakePortForwarder{
localPort: optionalLocalPort,
Expand Down
38 changes: 26 additions & 12 deletions internal/k8s/portforward.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import (
type PortForwardClient interface {
// Creates a new port-forwarder that's bound to the given context's lifecycle.
// When the context is canceled, the port-forwarder will close.
CreatePortForwarder(ctx context.Context, namespace Namespace, podID PodID, localPort int, remotePort int) (PortForwarder, error)
CreatePortForwarder(ctx context.Context, namespace Namespace, podID PodID, localPort int, remotePort int, host *string) (PortForwarder, error)
}

type PortForwarder interface {
Expand All @@ -43,7 +43,7 @@ func (pf portForwarder) LocalPort() int {
return pf.localPort
}

func (k K8sClient) CreatePortForwarder(ctx context.Context, namespace Namespace, podID PodID, optionalLocalPort, remotePort int) (PortForwarder, error) {
func (k K8sClient) CreatePortForwarder(ctx context.Context, namespace Namespace, podID PodID, optionalLocalPort, remotePort int, host *string) (PortForwarder, error) {
localPort := optionalLocalPort
if localPort == 0 {
// preferably, we'd set the localport to 0, and let the underlying function pick a port for us,
Expand All @@ -59,7 +59,7 @@ func (k K8sClient) CreatePortForwarder(ctx context.Context, namespace Namespace,
}
}

return k.portForwardClient.CreatePortForwarder(ctx, namespace, podID, localPort, remotePort)
return k.portForwardClient.CreatePortForwarder(ctx, namespace, podID, localPort, remotePort, host)
}

type portForwardClient struct {
Expand All @@ -82,7 +82,7 @@ func ProvidePortForwardClient(
}
}

func (c portForwardClient) CreatePortForwarder(ctx context.Context, namespace Namespace, podID PodID, localPort int, remotePort int) (PortForwarder, error) {
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)
if err != nil {
return nil, errors.Wrap(err, "error getting roundtripper")
Expand All @@ -103,13 +103,27 @@ func (c portForwardClient) CreatePortForwarder(ctx context.Context, namespace Na
readyChan := make(chan struct{}, 1)

ports := []string{fmt.Sprintf("%d:%d", localPort, remotePort)}
pf, err := portforward.New(
dialer,
ports,
stopChan,
readyChan,
logger.Get(ctx).Writer(logger.DebugLvl),
logger.Get(ctx).Writer(logger.DebugLvl))

var pf *portforward.PortForwarder
if host == nil {
pf, err = portforward.New(
dialer,
ports,
stopChan,
readyChan,
logger.Get(ctx).Writer(logger.DebugLvl),
logger.Get(ctx).Writer(logger.DebugLvl))
} else {
addresses := []string{*host}
pf, err = portforward.NewOnAddresses(
dialer,
addresses,
ports,
stopChan,
readyChan,
logger.Get(ctx).Writer(logger.DebugLvl),
logger.Get(ctx).Writer(logger.DebugLvl))
}
if err != nil {
return nil, errors.Wrap(err, "error forwarding port")
}
Expand Down Expand Up @@ -151,6 +165,6 @@ type explodingPortForwardClient struct {
error error
}

func (c explodingPortForwardClient) CreatePortForwarder(ctx context.Context, namespace Namespace, podID PodID, localPort int, remotePort int) (PortForwarder, error) {
func (c explodingPortForwardClient) CreatePortForwarder(ctx context.Context, namespace Namespace, podID PodID, localPort int, remotePort int, host *string) (PortForwarder, error) {
return nil, c.error
}
41 changes: 31 additions & 10 deletions internal/tiltfile/k8s.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package tiltfile

import (
"fmt"
"regexp"
"sort"
"strconv"
"strings"
Expand Down Expand Up @@ -775,6 +776,7 @@ func (s *tiltfileState) portForward(thread *starlark.Thread, fn *starlark.Builti
type portForward struct {
local int
container int
host *string
}

var _ starlark.Value = portForward{}
Expand All @@ -800,35 +802,54 @@ func (f portForward) Hash() (uint32, error) {
func intToPortForward(i starlark.Int) (portForward, error) {
n, ok := i.Int64()
if !ok {
return portForward{}, fmt.Errorf("portForward value %v is not representable as an int64", i)
return portForward{}, fmt.Errorf("portForward port value %v is not representable as an int64", i)
}
if n < 0 || n > 65535 {
return portForward{}, fmt.Errorf("portForward value %v is not in the range for a port [0-65535]", n)
return portForward{}, fmt.Errorf("portForward port value %v is not in the valid range [0-65535]", n)
}
return portForward{local: int(n)}, nil
}

const ipReStr = `^(([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])\.){3}([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])$`
const hostnameReStr = `^(([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\-]*[a-zA-Z0-9])\.)*([A-Za-z0-9]|[A-Za-z0-9][A-Za-z0-9\-]*[A-Za-z0-9])$`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where did these regexps come from?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what exactly you're asking. These are pretty standard regexps for IPs and hostnames, not sure if it's possible to pinpoint their exact source, cause they are pretty standard, based on relevant RFCs. Here are examples of where they can be encountered online:
IP: http://ipregex.com
Hostname: https://www.regextester.com/23

I've been using them for a while and I don't remember where I took them from, but again, they are pretty standard, so probably shouldn't matter.


var validHost = regexp.MustCompile(ipReStr + "|" + hostnameReStr)

func stringToPortForward(s starlark.String) (portForward, error) {
parts := strings.SplitN(string(s), ":", 2)
local, err := strconv.Atoi(parts[0])
parts := strings.SplitN(string(s), ":", 3)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where did this syntax come from?

it's probably ok, but seems different from the kubectl port-forward syntax (which has a separate --address flag). https://github.com/kubernetes/kubectl/blob/master/pkg/cmd/portforward/portforward.go#L87

i guess the advantage of the separate address flag is that you can specify multiple

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just as with the previous discussion, not sure what the exact origin of this syntax is, but it seems to be the most commonly used way to specify hostname for port forwarding, one example is OpenSSH.
You're right, it doesn't allow for multiple addresses, so if we need to support that, we should probably change syntax to something else. That being said, in my experience there's barely ever a need to bind to multiple addresses, you usually want it to either be accessible on a specific network, or accessible on every network (which can be done by specifying 0.0.0.0 as the address).


var host *string
var localString string
if len(parts) == 3 {
localString = parts[1]
host = &parts[0]
if !validHost.MatchString(*host) {
return portForward{}, fmt.Errorf("portForward host value %q is not a valid hostname or IP address", localString)
}
} else {
localString = parts[0]
}

local, err := strconv.Atoi(localString)
if err != nil || local < 0 || local > 65535 {
return portForward{}, fmt.Errorf("portForward value %q is not in the range for a port [0-65535]", parts[0])
return portForward{}, fmt.Errorf("portForward port value %q is not in the valid range [0-65535]", localString)
}

var container int
if len(parts) == 2 {
container, err = strconv.Atoi(parts[1])
if len(parts) > 1 {
last := parts[len(parts)-1]
container, err = strconv.Atoi(last)
if err != nil || container < 0 || container > 65535 {
return portForward{}, fmt.Errorf("portForward value %q is not in the range for a port [0-65535]", parts[1])
return portForward{}, fmt.Errorf("portForward port value %q is not in the valid range [0-65535]", last)
}
}
return portForward{local: local, container: container}, nil
return portForward{local: local, container: container, host: host}, nil
}

func (s *tiltfileState) portForwardsToDomain(r *k8sResource) []model.PortForward {
var result []model.PortForward
for _, pf := range r.portForwards {
result = append(result, model.PortForward{LocalPort: pf.local, ContainerPort: pf.container})
result = append(result, model.PortForward{LocalPort: pf.local, ContainerPort: pf.container, Host: pf.host})
}
return result
}
Expand Down
12 changes: 7 additions & 5 deletions internal/tiltfile/tiltfile_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -443,18 +443,20 @@ type portForwardCase struct {
}

func TestPortForward(t *testing.T) {
var goodHost = "0.0.0.0"
portForwardCases := []portForwardCase{
{"value_local", "8000", []model.PortForward{{LocalPort: 8000}}, ""},
{"value_local_negative", "-1", nil, "not in the range for a port"},
{"value_local_large", "8000000", nil, "not in the range for a port"},
{"value_local_negative", "-1", nil, "not in the valid range"},
{"value_local_large", "8000000", nil, "not in the valid range"},
{"value_string_local", "'10000'", []model.PortForward{{LocalPort: 10000}}, ""},
{"value_string_both", "'10000:8000'", []model.PortForward{{LocalPort: 10000, ContainerPort: 8000}}, ""},
{"value_string_garbage", "'garbage'", nil, "not in the range for a port"},
{"value_string_3x80", "'80:80:80'", nil, "not in the range for a port"},
{"value_string_empty", "''", nil, "not in the range for a port"},
{"value_string_garbage", "'garbage'", nil, "not in the valid range"},
{"value_string_empty", "''", nil, "not in the valid range"},
{"value_both", "port_forward(8001, 443)", []model.PortForward{{LocalPort: 8001, ContainerPort: 443}}, ""},
{"list", "[8000, port_forward(8001, 443)]", []model.PortForward{{LocalPort: 8000}, {LocalPort: 8001, ContainerPort: 443}}, ""},
{"list_string", "['8000', '8001:443']", []model.PortForward{{LocalPort: 8000}, {LocalPort: 8001, ContainerPort: 443}}, ""},
{"value_host_bad", "'bad+host:10000:8000'", nil, "not a valid hostname or IP address"},
{"value_host_good", "'" + goodHost + ":10000:8000'", []model.PortForward{{LocalPort: 10000, ContainerPort: 8000, Host: &goodHost}}, ""},
}

for _, c := range portForwardCases {
Expand Down
9 changes: 6 additions & 3 deletions pkg/model/manifest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,12 +368,15 @@ func ToRuns(cmds []Cmd) []Run {
}

type PortForward struct {
// The port to expose on localhost of the current machine.
LocalPort int

// The port to connect to inside the deployed container.
// If 0, we will connect to the first containerPort.
ContainerPort int

// The port to expose on the current machine.
LocalPort int

// Optional host to bind to on the current machine (localhost by default)
Host *string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the purpose of making it a pointer? would we want to treat the nil value differently than the empty string?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two thoughts:
A) Even if a piece of information is not currently used, but might be used in the future, and capturing it doesn't require any overhead, imo it's better to capture it now than to refactor later.
B) Right now if the value is nil we do port forwarding without addresses, and if the value is empty string we simply pass it to NewOnAddress, which I'm assuming explicitly fails with some error (tho I haven't tested it tbh). So this signature makes it slightly easier to catch bugs in case hostname is meant to be supplied, but ended up being empty string. This is probably rather opinionated.

}

var imageTargetAllowUnexported = cmp.AllowUnexported(ImageTarget{})
Expand Down