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

briandealwis · 2021-03-22T15:29:16Z

Fixes: #5590
Related: #5554 (comment)

Description
This PR modifies our GetAvailablePort() function (and related functions) to:

Never attempt to allocate a system ports (port 1-1023) to avoid confusing results such as allocating ports 80 or 443.
Allocate ports using INADDR_ANY (0.0.0.0) as it is possible to listen on a port for a specific interface even when another process is listening on INADDR_ANY with the same port.

User facing changes (remove if N/A)

port-forwarding no longer allocates ports in the 1-1023 range

codecov · 2021-03-22T15:42:16Z

Codecov Report

Merging #5591 (c25d495) into master (5e2a4ec) will increase coverage by 0.03%.
The diff coverage is 86.66%.

❗ Current head c25d495 differs from pull request most recent head 5b2411a. Consider uploading reports for the commit 5b2411a to get more accurate results

@@            Coverage Diff             @@
##           master    #5591      +/-   ##
==========================================
+ Coverage   70.63%   70.67%   +0.03%     
==========================================
  Files         411      408       -3     
  Lines       15777    15580     -197     
==========================================
- Hits        11144    11011     -133     
+ Misses       3812     3758      -54     
+ Partials      821      811      -10

Impacted Files	Coverage Δ
...kubernetes/portforward/port_forward_integration.go	`0.00% <0.00%> (ø)`
pkg/skaffold/server/server.go	`41.96% <ø> (ø)`
...affold/kubernetes/portforward/kubectl_forwarder.go	`67.60% <100.00%> (ø)`
...g/skaffold/kubernetes/portforward/pod_forwarder.go	`81.63% <100.00%> (-0.73%)`	⬇️
...ffold/kubernetes/portforward/resource_forwarder.go	`85.41% <100.00%> (-1.86%)`	⬇️
pkg/skaffold/util/port.go	`92.15% <100.00%> (+0.15%)`	⬆️
pkg/skaffold/build/builder_mux.go	`34.37% <0.00%> (-10.37%)`	⬇️
pkg/skaffold/config/types.go	`91.66% <0.00%> (-8.34%)`	⬇️
...affold/kubernetes/portforward/forwarder_manager.go	`26.66% <0.00%> (-6.67%)`	⬇️
pkg/skaffold/event/v2/event.go	`61.17% <0.00%> (-6.39%)`	⬇️
... and 47 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e2a4ec...5b2411a. Read the comment docs.

gsquared94 · 2021-03-29T18:47:16Z

pkg/skaffold/util/port_test.go

+
+func TestSystemPorts(t *testing.T) {
+	ps := PortSet{}
+	ps.lock.Lock() // cause deadlock: getPortIfAvailable should never check the PortSet


not sure why this is required. There's already an error if getPortIfAvailable returns true. Also if I deliberately introduce a bug then this deadlock causes the test to hang till timeout.

This is a whitebox test that verifies that Skaffold should never check the set-ports-list for system ports. The intent is that if this test hangs (e.g., someone violated this invariant), someone will quickly discover that this invariant has been violated.

Let me update this comment to be more clear.

but won't that get tested anyway in t.Errorf(...)? Also can't ps just be nil and you get the same thing?

I really do want to explicitly verify that GetPortIfAvailable() never considers ports 1–1023 separately from the provided PortSet. The PortSet uses locks to protect the data structure, and there's no reason to check it for this port range.

tejal29 · 2021-03-31T19:32:16Z

pkg/skaffold/util/port.go

 	for i := 0; i < 10; i++ {
 		port++
-		if getPortIfAvailable(address, port, usedPorts) {
+		if getPortIfAvailable(port, usedPorts) {


Shd we skip iterating over next 10 ports if port number is less than 1022?

briandealwis · 2021-04-06T03:34:37Z

Need to revisit this as it can be necessary to map to ports 80 or 443 to enable testing of OAuth. I think the way forward here is to allow explicit user-defined port-forwards to specific ports.

briandealwis · 2021-04-12T21:04:21Z

Verified that explicit local ports on user-define ports in skaffold.yaml do not use util.GetAvailablePort():

--- examples/microservices/skaffold.yaml
+++ examples/microservices/skaffold.yaml
@@ -23,7 +23,7 @@ portForward:
   - resourceType: deployment
     resourceName: leeroy-web
     port: 8080
-    localPort: 9000
+    localPort: 80
   - resourceType: deployment
     resourceName: leeroy-app
     port: http

And the result:

Deployments stabilized in 3.995 seconds
Press Ctrl+C to exit
Watching for changes...
[leeroy-app] 2021/04/12 20:54:05 leeroy app server ready
[leeroy-web] 2021/04/12 20:54:05 leeroy web server ready
port forwarding deployment-leeroy-web-default-8080 got terminated: output: Unable to listen on port 80: Listeners failed to create with the following errors: [unable to create listener: Error listen tcp4 127.0.0.1:80: bind: permission denied unable to create listener: Error listen tcp6 [::1]:80: bind: permission denied]
error: unable to listen on any of the requested ports: [{80 8080}]

Port forwarding deployment/leeroy-app in namespace default, remote port http -> 127.0.0.1:9001

But it would be nice if service ports did map to corresponding local ports if they were available:

$ skaffold dev --port-forward=services
...
Press Ctrl+C to exit
Watching for changes...
[leeroy-app] 2021/04/12 20:56:42 leeroy app server ready
[leeroy-web] 2021/04/12 20:56:42 leeroy web server ready
Port forwarding service/leeroy-app in namespace default, remote port 50051 -> 127.0.0.1:4503
Port forwarding service/leeroy-web in namespace default, remote port 8080 -> 127.0.0.1:4504

MarlonGamez · 2021-04-12T21:48:32Z

pkg/skaffold/util/port.go

+//
+// 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 {


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

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.

briandealwis · 2021-04-13T03:29:33Z

Closing this: the system ports was an unnecessary diversion

briandealwis requested a review from a team as a code owner March 22, 2021 15:29

pull-request-size bot added the size/M label Mar 22, 2021

google-cla bot added the cla: yes label Mar 22, 2021

gsquared94 reviewed Mar 29, 2021

View reviewed changes

tejal29 reviewed Mar 31, 2021

View reviewed changes

briandealwis added the !! do-not-merge !! label Apr 6, 2021

briandealwis added the kokoro:force-run forces a kokoro re-run on a PR label Apr 12, 2021

kokoro-team removed the kokoro:force-run forces a kokoro re-run on a PR label Apr 12, 2021

MarlonGamez reviewed Apr 12, 2021

View reviewed changes

Allocate ports against INADDR_ANY

5b2411a

briandealwis force-pushed the i5590 branch from c25d495 to 5b2411a Compare April 13, 2021 03:28

briandealwis closed this Apr 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

briandealwis commented Mar 22, 2021

codecov bot commented Mar 22, 2021 •

edited

Loading

gsquared94 Mar 29, 2021

briandealwis Mar 29, 2021

gsquared94 Mar 29, 2021 •

edited

Loading

briandealwis Mar 29, 2021

tejal29 Mar 31, 2021

briandealwis commented Apr 6, 2021

briandealwis commented Apr 12, 2021

MarlonGamez Apr 12, 2021

briandealwis Apr 21, 2021

briandealwis commented Apr 13, 2021

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

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

Conversation

briandealwis commented Mar 22, 2021

codecov bot commented Mar 22, 2021 • edited Loading

Codecov Report

gsquared94 Mar 29, 2021

Choose a reason for hiding this comment

briandealwis Mar 29, 2021

Choose a reason for hiding this comment

gsquared94 Mar 29, 2021 • edited Loading

Choose a reason for hiding this comment

briandealwis Mar 29, 2021

Choose a reason for hiding this comment

tejal29 Mar 31, 2021

Choose a reason for hiding this comment

briandealwis commented Apr 6, 2021

briandealwis commented Apr 12, 2021

MarlonGamez Apr 12, 2021

Choose a reason for hiding this comment

briandealwis Apr 21, 2021

Choose a reason for hiding this comment

briandealwis commented Apr 13, 2021

codecov bot commented Mar 22, 2021 •

edited

Loading

gsquared94 Mar 29, 2021 •

edited

Loading