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

Modify util.GetAvailablePort to produce less confusing port allocations #5591

Closed
wants to merge 1 commit into from
Closed

Modify util.GetAvailablePort to produce less confusing port allocations #5591

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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 pkg/skaffold/kubernetes/portforward/kubectl_forwarder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ func (k *KubectlForwarder) forward(parentCtx context.Context, pfe *portForwardEn
}
pfe.terminationLock.Unlock()

if !isPortFree(util.Loopback, pfe.localPort) {
if !isPortFree(pfe.localPort) {
// Assuming that Skaffold brokered ports don't overlap, this has to be an external process that started
// since the dev loop kicked off. We are notifying the user in the hope that they can fix it
color.Red.Fprintf(k.out, "failed to port forward %v, port %d is taken, retrying...\n", pfe, pfe.localPort)
Expand Down
2 changes: 1 addition & 1 deletion pkg/skaffold/kubernetes/portforward/kubectl_forwarder_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ func TestUnavailablePort(t *testing.T) {
// has been called
var portFreeWG sync.WaitGroup
portFreeWG.Add(1)
t.Override(&isPortFree, func(string, int) bool {
t.Override(&isPortFree, func(int) bool {
portFreeWG.Done()
return false
})
Expand Down
2 changes: 1 addition & 1 deletion pkg/skaffold/kubernetes/portforward/pod_forwarder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ func (p *WatchingPodForwarder) podForwardingEntry(resourceVersion, containerName
}

// retrieve an open port on the host
entry.localPort = retrieveAvailablePort(resource.Address, resource.Port.IntVal, &p.entryManager.forwardedPorts)
entry.localPort = retrieveAvailablePort(resource.Port.IntVal, &p.entryManager.forwardedPorts)

return entry, nil
}
2 changes: 1 addition & 1 deletion pkg/skaffold/kubernetes/portforward/pod_forwarder_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,7 +403,7 @@ func TestAutomaticPortForwardPod(t *testing.T) {
testutil.Run(t, test.description, func(t *testutil.T) {
testEvent.InitializeState([]latest.Pipeline{{}})
taken := map[int]struct{}{}
t.Override(&retrieveAvailablePort, mockRetrieveAvailablePort("127.0.0.1", taken, test.availablePorts))
t.Override(&retrieveAvailablePort, mockRetrieveAvailablePort(taken, test.availablePorts))
t.Override(&topLevelOwnerKey, func(context.Context, metav1.Object, string) string { return "owner" })

if test.forwarder == nil {
Expand Down
4 changes: 2 additions & 2 deletions pkg/skaffold/kubernetes/portforward/port_forward_integration.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ func SimulateDevCycle(t *testing.T, kubectlCLI *kubectl.CLI, namespace string) {
defer func() { portForwardEvent = portForwardEventHandler }()
portForwardEvent = func(entry *portForwardEntry) {}
ctx := context.Background()
localPort := retrieveAvailablePort("127.0.0.1", 9000, &em.forwardedPorts)
localPort := retrieveAvailablePort(9000, &em.forwardedPorts)
pfe := newPortForwardEntry(0, latest.PortForwardResource{
Type: "deployment",
Name: "leeroy-web",
Expand All @@ -50,7 +50,7 @@ func SimulateDevCycle(t *testing.T, kubectlCLI *kubectl.CLI, namespace string) {

logrus.Info("waiting for the same port to become available...")
if err := wait.Poll(100*time.Millisecond, 5*time.Second, func() (done bool, err error) {
nextPort := retrieveAvailablePort("127.0.0.1", localPort, &em.forwardedPorts)
nextPort := retrieveAvailablePort(localPort, &em.forwardedPorts)

logrus.Infof("next port %d", nextPort)

Expand Down
2 changes: 1 addition & 1 deletion pkg/skaffold/kubernetes/portforward/resource_forwarder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ func (p *ResourceForwarder) getCurrentEntry(resource latest.PortForwardResource)
}

// retrieve an open port on the host
entry.localPort = retrieveAvailablePort(resource.Address, resource.LocalPort, &p.entryManager.forwardedPorts)
entry.localPort = retrieveAvailablePort(resource.LocalPort, &p.entryManager.forwardedPorts)
return entry
}

Expand Down
10 changes: 5 additions & 5 deletions pkg/skaffold/kubernetes/portforward/resource_forwarder_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,10 @@ func newTestForwarder() *testForwarder {
return &testForwarder{}
}

func mockRetrieveAvailablePort(_ string, taken map[int]struct{}, availablePorts []int) func(string, int, *util.PortSet) int {
func mockRetrieveAvailablePort(taken map[int]struct{}, availablePorts []int) func(int, *util.PortSet) int {
// Return first available port in ports that isn't taken
var lock sync.Mutex
return func(string, int, *util.PortSet) int {
return func(int, *util.PortSet) int {
for _, p := range availablePorts {
lock.Lock()
if _, ok := taken[p]; ok {
Expand Down Expand Up @@ -122,7 +122,7 @@ func TestStart(t *testing.T) {
for _, test := range tests {
testutil.Run(t, test.description, func(t *testutil.T) {
testEvent.InitializeState([]latest.Pipeline{{}})
t.Override(&retrieveAvailablePort, mockRetrieveAvailablePort("127.0.0.1", map[int]struct{}{}, test.availablePorts))
t.Override(&retrieveAvailablePort, mockRetrieveAvailablePort(map[int]struct{}{}, test.availablePorts))
t.Override(&retrieveServices, func(context.Context, string, []string) ([]*latest.PortForwardResource, error) {
return test.resources, nil
})
Expand Down Expand Up @@ -188,7 +188,7 @@ func TestGetCurrentEntryFunc(t *testing.T) {

for _, test := range tests {
testutil.Run(t, test.description, func(t *testutil.T) {
t.Override(&retrieveAvailablePort, mockRetrieveAvailablePort("127.0.0.1", map[int]struct{}{}, test.availablePorts))
t.Override(&retrieveAvailablePort, mockRetrieveAvailablePort(map[int]struct{}{}, test.availablePorts))

entryManager := NewEntryManager(ioutil.Discard, newTestForwarder())
entryManager.forwardedResources = forwardedResources{
Expand Down Expand Up @@ -251,7 +251,7 @@ func TestUserDefinedResources(t *testing.T) {
for _, test := range tests {
testutil.Run(t, test.description, func(t *testutil.T) {
testEvent.InitializeState([]latest.Pipeline{{}})
t.Override(&retrieveAvailablePort, mockRetrieveAvailablePort("127.0.0.1", map[int]struct{}{}, []int{8080, 9000}))
t.Override(&retrieveAvailablePort, mockRetrieveAvailablePort(map[int]struct{}{}, []int{8080, 9000}))
t.Override(&retrieveServices, func(context.Context, string, []string) ([]*latest.PortForwardResource, error) {
return []*latest.PortForwardResource{svc}, nil
})
Expand Down
2 changes: 1 addition & 1 deletion pkg/skaffold/server/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ func errorHandler(ctx context.Context, _ *runtime.ServeMux, marshaler runtime.Ma

func listenOnAvailablePort(preferredPort int, usedPorts *util.PortSet) (net.Listener, int, error) {
for try := 1; ; try++ {
port := util.GetAvailablePort(util.Loopback, preferredPort, usedPorts)
port := util.GetAvailablePort(preferredPort, usedPorts)

l, err := net.Listen("tcp", fmt.Sprintf("%s:%d", util.Loopback, port))
if err != nil {
Expand Down
39 changes: 21 additions & 18 deletions pkg/skaffold/util/port.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,34 +87,37 @@ func (f *PortSet) List() []int {
return list
}

// GetAvailablePort returns an available port that is near the requested port when possible.
// First, check if the provided port is available on the specified address. If so, use it.
// If not, check if any of the next 10 subsequent ports are available.
// If not, check if any of ports 4503-4533 are available.
// If not, return a random port, which hopefully won't collide with any future containers

// See https://www.iana.org/assignments/service-names-port-numbers/service-names-port-numbers.txt,
func GetAvailablePort(address string, port int, usedPorts *PortSet) int {
if getPortIfAvailable(address, port, usedPorts) {
return port
}

// try the next 10 ports after the provided one
for i := 0; i < 10; i++ {
port++
if getPortIfAvailable(address, port, usedPorts) {
logrus.Debugf("found open port: %d", port)
//
// See https://www.iana.org/assignments/service-names-port-numbers/service-names-port-numbers.txt
func GetAvailablePort(port int, usedPorts *PortSet) int {
if port > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry if this is a dumb question: Where are we preventing trying to allocate 1-1023?

Copy link
Member Author

Choose a reason for hiding this comment

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

See #5554 (comment) for the motivation. We had another group ask as they wanted to control which service is bound to port 443 for https.

if getPortIfAvailable(port, usedPorts) {
return port
}

// try the next 10 ports after the provided one
for i := 0; i < 10; i++ {
port++
if getPortIfAvailable(port, usedPorts) {
logrus.Debugf("found open port: %d", port)
return port
}
}
}

for port = 4503; port <= 4533; port++ {
if getPortIfAvailable(address, port, usedPorts) {
if getPortIfAvailable(port, usedPorts) {
logrus.Debugf("found open port: %d", port)
return port
}
}

l, err := net.Listen("tcp", fmt.Sprintf("%s:0", address))
l, err := net.Listen("tcp", ":0")
if err != nil {
return -1
}
Expand All @@ -126,16 +129,16 @@ func GetAvailablePort(address string, port int, usedPorts *PortSet) int {
return p
}

func getPortIfAvailable(address string, p int, usedPorts *PortSet) bool {
func getPortIfAvailable(p int, usedPorts *PortSet) bool {
if alreadySet := usedPorts.LoadOrSet(p); alreadySet {
return false
}

return IsPortFree(address, p)
return IsPortFree(p)
}

func IsPortFree(address string, p int) bool {
l, err := net.Listen("tcp", fmt.Sprintf("%s:%d", address, p))
func IsPortFree(p int) bool {
l, err := net.Listen("tcp", fmt.Sprintf(":%d", p))
if err != nil {
return false
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/skaffold/util/port_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ func TestGetAvailablePort(t *testing.T) {
wg.Add(N)
for i := 0; i < N; i++ {
go func() {
port := GetAvailablePort("127.0.0.1", 4503, &ports)
port := GetAvailablePort(4503, &ports)

l, err := net.Listen("tcp", fmt.Sprintf("%s:%d", Loopback, port))
if err != nil {
Expand Down