From e6a7f0253b084683f9a6ce8298f5b1d3c86ff35d Mon Sep 17 00:00:00 2001
From: Parthvi Vala <pvala@redhat.com>
Date: Wed, 12 Apr 2023 12:12:32 +0530
Subject: [PATCH] Fix: random port matches a custom local port (#6727)

Signed-off-by: Parthvi Vala <pvala@redhat.com>
---
 pkg/odo/cli/dev/dev_test.go                   |  2 +-
 .../kubeportforward/portForward.go            | 25 +++++++++++++------
 .../kubeportforward/portForward_test.go       | 20 ++++++++++++++-
 3 files changed, 38 insertions(+), 9 deletions(-)

diff --git a/pkg/odo/cli/dev/dev_test.go b/pkg/odo/cli/dev/dev_test.go
index 6a02f0c4304..f6c7e0d9352 100644
--- a/pkg/odo/cli/dev/dev_test.go
+++ b/pkg/odo/cli/dev/dev_test.go
@@ -280,7 +280,7 @@ func Test_parsePortForwardFlag(t *testing.T) {
 				return
 			}
 			if diff := cmp.Diff(gotForwardedPorts, tt.wantForwardedPorts); diff != "" {
-				t.Errorf("parsePortForwardFlag() gotForwardedPorts = %v, want %v", gotForwardedPorts, tt.wantForwardedPorts)
+				t.Errorf("parsePortForwardFlag() (got vs want) diff = %v", diff)
 			}
 		})
 	}
diff --git a/pkg/portForward/kubeportforward/portForward.go b/pkg/portForward/kubeportforward/portForward.go
index 91bb33b5f8e..0afc248a075 100644
--- a/pkg/portForward/kubeportforward/portForward.go
+++ b/pkg/portForward/kubeportforward/portForward.go
@@ -172,8 +172,11 @@ func (o *PFClient) GetForwardedPorts() map[string][]v1alpha2.Endpoint {
 // if not, it assigns a port starting from 20001 as done in portPairsFromContainerEndpoints
 func getCustomPortPairs(definedPorts []api.ForwardedPort, ceMapping map[string][]v1alpha2.Endpoint) map[string][]string {
 	portPairs := make(map[string][]string)
-
-	// getCustomLocalPort analyzes the definedPorts i.e. custom portforwarding to see if a containerPort has a custom localPort, if a container name is provided, it also takes that into account.
+	usedPorts := make(map[int]struct{})
+	for _, dPort := range definedPorts {
+		usedPorts[dPort.LocalPort] = struct{}{}
+	}
+	// getCustomLocalPort analyzes the definedPorts i.e. custom port forwarding to see if a containerPort has a custom localPort, if a container name is provided, it also takes that into account.
 	getCustomLocalPort := func(containerPort int, container string) int {
 		for _, dp := range definedPorts {
 			if dp.ContainerPort == containerPort {
@@ -194,11 +197,19 @@ func getCustomPortPairs(definedPorts []api.ForwardedPort, ceMapping map[string][
 		for _, p := range ports {
 			freePort := getCustomLocalPort(p.TargetPort, name)
 			if freePort == 0 {
-				var err error
-				freePort, err = util.NextFreePort(startPort, endPort, nil)
-				if err != nil {
-					klog.Infof("%s", err)
-					continue
+				for {
+					var err error
+					freePort, err = util.NextFreePort(startPort, endPort, nil)
+					if err != nil {
+						klog.Infof("%s", err)
+						continue
+					}
+					// if the free port matches any of the custom local port, try again
+					if _, isPortUsed := usedPorts[freePort]; isPortUsed {
+						startPort = freePort + 1
+						continue
+					}
+					break
 				}
 				startPort = freePort + 1
 			}
diff --git a/pkg/portForward/kubeportforward/portForward_test.go b/pkg/portForward/kubeportforward/portForward_test.go
index f31886c3718..bf4a4332f40 100644
--- a/pkg/portForward/kubeportforward/portForward_test.go
+++ b/pkg/portForward/kubeportforward/portForward_test.go
@@ -51,12 +51,30 @@ func Test_getCompleteCustomPortPairs(t *testing.T) {
 				"tools":   {"5000:5000"},
 			},
 		},
+		{
+			name: "local ports in range [20001-30001] are provided as custom forward ports",
+			args: args{
+				definedPorts: []api.ForwardedPort{
+					{LocalPort: 20001, ContainerPort: 8000},
+					{LocalPort: 20002, ContainerPort: 9000},
+					{LocalPort: 5000, ContainerPort: 5000},
+				},
+				ceMapping: map[string][]v1alpha2.Endpoint{
+					"runtime": {{TargetPort: 8000}, {TargetPort: 9000}},
+					"tools":   {{TargetPort: 5000}, {TargetPort: 8080}},
+				},
+			},
+			wantPortPairs: map[string][]string{
+				"runtime": {"20001:8000", "20002:9000"},
+				"tools":   {"5000:5000", "20003:8080"},
+			},
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			gotPortPairs := getCustomPortPairs(tt.args.definedPorts, tt.args.ceMapping)
 			if diff := cmp.Diff(gotPortPairs, tt.wantPortPairs); diff != "" {
-				t.Errorf("getCompleteCustomPortPairs() = %v, want %v", gotPortPairs, tt.wantPortPairs)
+				t.Errorf("getCompleteCustomPortPairs() (got vs want) diff = %v", diff)
 			}
 		})
 	}