Forwarding resources should not allocate system ports

pkg/skaffold/kubernetes/portforward/kubectl_forwarder.go

@@ -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)

pkg/skaffold/kubernetes/portforward/kubectl_forwarder_test.go

@@ -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
 		})

pkg/skaffold/kubernetes/portforward/pod_forwarder.go

@@ -111,7 +111,6 @@ func (p *WatchingPodForwarder) portForwardPod(ctx context.Context, pod *v1.Pod)
 				Namespace: pod.Namespace,
 				Port:      schemautil.FromInt(int(port.ContainerPort)),
 				Address:   constants.DefaultPortForwardAddress,
-				LocalPort: int(port.ContainerPort),
 			}
 
 			entry, err := p.podForwardingEntry(pod.ResourceVersion, c.Name, port.Name, ownerReference, resource)
@@ -149,7 +148,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
 }

pkg/skaffold/kubernetes/portforward/pod_forwarder_test.go

@@ -60,7 +60,7 @@ func TestAutomaticPortForwardPod(t *testing.T) {
 						Namespace: "namespace",
 						Port:      schemautil.FromInt(8080),
 						Address:   "127.0.0.1",
-						LocalPort: 8080,
+						LocalPort: 0,
 					},
 					ownerReference:         "owner",
 					automaticPodForwarding: true,
@@ -105,7 +105,7 @@ func TestAutomaticPortForwardPod(t *testing.T) {
 						Namespace: "namespace",
 						Port:      schemautil.FromInt(8080),
 						Address:   "127.0.0.1",
-						LocalPort: 8080,
+						LocalPort: 0,
 					},
 					ownerReference:         "owner",
 					automaticPodForwarding: true,
@@ -181,7 +181,7 @@ func TestAutomaticPortForwardPod(t *testing.T) {
 						Namespace: "namespace",
 						Port:      schemautil.FromInt(8080),
 						Address:   "127.0.0.1",
-						LocalPort: 8080,
+						LocalPort: 0,
 					},
 					ownerReference:         "owner",
 					portName:               "portname",
@@ -198,7 +198,7 @@ func TestAutomaticPortForwardPod(t *testing.T) {
 						Namespace: "namespace2",
 						Port:      schemautil.FromInt(50051),
 						Address:   "127.0.0.1",
-						LocalPort: 50051,
+						LocalPort: 0,
 					},
 					ownerReference:         "owner",
 					portName:               "portname2",
@@ -265,7 +265,7 @@ func TestAutomaticPortForwardPod(t *testing.T) {
 						Namespace: "namespace",
 						Port:      schemautil.FromInt(8080),
 						Address:   "127.0.0.1",
-						LocalPort: 8080,
+						LocalPort: 0,
 					},
 					ownerReference:         "owner",
 					automaticPodForwarding: true,
@@ -282,7 +282,7 @@ func TestAutomaticPortForwardPod(t *testing.T) {
 						Namespace: "namespace2",
 						Port:      schemautil.FromInt(8080),
 						Address:   "127.0.0.1",
-						LocalPort: 8080,
+						LocalPort: 0,
 					},
 					ownerReference:         "owner",
 					automaticPodForwarding: true,
@@ -348,7 +348,7 @@ func TestAutomaticPortForwardPod(t *testing.T) {
 						Namespace: "namespace",
 						Port:      schemautil.FromInt(8080),
 						Address:   "127.0.0.1",
-						LocalPort: 8080,
+						LocalPort: 0,
 					},
 					ownerReference:         "owner",
 					automaticPodForwarding: true,
@@ -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 {

pkg/skaffold/kubernetes/portforward/port_forward_integration.go

@@ -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",
@@ -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)
 

pkg/skaffold/kubernetes/portforward/resource_forwarder.go

@@ -126,8 +126,13 @@ func (p *ResourceForwarder) getCurrentEntry(resource latest.PortForwardResource)
 		return entry
 	}
 
-	// retrieve an open port on the host
-	entry.localPort = retrieveAvailablePort(resource.Address, resource.LocalPort, &p.entryManager.forwardedPorts)
+	// Try to request matching local port *providing* that it is not a system port.
+	// https://github.com/GoogleContainerTools/skaffold/pull/5554#issuecomment-803270340
+	requestPort := resource.LocalPort
+	if requestPort == 0 && resource.Port.IntVal >= 1024 {
+		requestPort = resource.Port.IntVal
+	}
+	entry.localPort = retrieveAvailablePort(requestPort, &p.entryManager.forwardedPorts)
 	return entry
 }
 
@@ -155,7 +160,6 @@ func retrieveServiceResources(ctx context.Context, label string, namespaces []st
 					Namespace: s.Namespace,
 					Port:      schemautil.FromInt(int(p.Port)),
 					Address:   constants.DefaultPortForwardAddress,
-					LocalPort: int(p.Port),
 				})
 			}
 		}

pkg/skaffold/kubernetes/portforward/resource_forwarder_test.go

@@ -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 {
@@ -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
 			})
@@ -152,6 +152,7 @@ func TestGetCurrentEntryFunc(t *testing.T) {
 		forwardedResources map[string]*portForwardEntry
 		availablePorts     []int
 		resource           latest.PortForwardResource
+		expectedReq        int
 		expected           *portForwardEntry
 	}{
 		{
@@ -162,6 +163,17 @@ func TestGetCurrentEntryFunc(t *testing.T) {
 				Port: schemautil.FromInt(8080),
 			},
 			availablePorts: []int{8080},
+			expectedReq:    8080,
+			expected:       newPortForwardEntry(0, latest.PortForwardResource{}, "", "", "", "", 8080, false),
+		}, {
+			description: "should not request system ports (1-1023)",
+			resource: latest.PortForwardResource{
+				Type: "service",
+				Name: "serviceName",
+				Port: schemautil.FromInt(80),
+			},
+			availablePorts: []int{8080},
+			expectedReq:    0, // no local port requested as port 80 is a system port
 			expected:       newPortForwardEntry(0, latest.PortForwardResource{}, "", "", "", "", 8080, false),
 		}, {
 			description: "port forward existing deployment",
@@ -182,13 +194,17 @@ func TestGetCurrentEntryFunc(t *testing.T) {
 					localPort: 9000,
 				},
 			},
-			expected: newPortForwardEntry(0, latest.PortForwardResource{}, "", "", "", "", 9000, false),
+			expectedReq: -1, // retrieveAvailablePort should not be called as there is an assigned localPort
+			expected:    newPortForwardEntry(0, latest.PortForwardResource{}, "", "", "", "", 9000, false),
 		},
 	}
 
 	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, func(req int, ps *util.PortSet) int {
+				t.CheckDeepEqual(test.expectedReq, req)
+				return mockRetrieveAvailablePort(map[int]struct{}{}, test.availablePorts)(req, ps)
+			})
 
 			entryManager := NewEntryManager(ioutil.Discard, newTestForwarder())
 			entryManager.forwardedResources = forwardedResources{
@@ -251,7 +267,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
 			})
@@ -321,14 +337,14 @@ func TestRetrieveServices(t *testing.T) {
 				Namespace: "test",
 				Port:      schemautil.FromInt(8080),
 				Address:   "127.0.0.1",
-				LocalPort: 8080,
+				LocalPort: 0,
 			}, {
 				Type:      constants.Service,
 				Name:      "svc2",
 				Namespace: "test1",
 				Port:      schemautil.FromInt(8081),
 				Address:   "127.0.0.1",
-				LocalPort: 8081,
+				LocalPort: 0,
 			}},
 		}, {
 			description: "no services in given namespace",

pkg/skaffold/server/server.go

@@ -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 {

pkg/skaffold/util/port.go

@@ -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 {
+		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
 	}
@@ -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
 	}

pkg/skaffold/util/port_test.go

@@ -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 {