From ddad1dc4acc5776bcff1883685e4d06b26c47526 Mon Sep 17 00:00:00 2001 From: Parthvi Vala Date: Thu, 13 Apr 2023 22:36:10 +0530 Subject: [PATCH 1/4] Fix flaky unit Test_getCompleteCustomPortPairs Signed-off-by: Parthvi Vala --- pkg/portForward/kubeportforward/portForward_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/portForward/kubeportforward/portForward_test.go b/pkg/portForward/kubeportforward/portForward_test.go index bf4a4332f40..5588be1b304 100644 --- a/pkg/portForward/kubeportforward/portForward_test.go +++ b/pkg/portForward/kubeportforward/portForward_test.go @@ -23,6 +23,7 @@ func Test_getCompleteCustomPortPairs(t *testing.T) { args: args{ definedPorts: []api.ForwardedPort{ {ContainerName: "runtime", LocalPort: 8080, ContainerPort: 8000}, + {ContainerName: "tools", LocalPort: 5000, ContainerPort: 5000}, }, ceMapping: map[string][]v1alpha2.Endpoint{ "runtime": {{TargetPort: 8000}, {TargetPort: 9000}}, @@ -31,7 +32,7 @@ func Test_getCompleteCustomPortPairs(t *testing.T) { }, wantPortPairs: map[string][]string{ "runtime": {"8080:8000", "20001:9000"}, - "tools": {"20002:5000"}, + "tools": {"5000:5000"}, }, }, { From 1bcbacf18414320b470f594e9ec7717563d7da82 Mon Sep 17 00:00:00 2001 From: Parthvi Vala Date: Mon, 17 Apr 2023 16:15:07 +0530 Subject: [PATCH 2/4] Use range check Signed-off-by: Parthvi Vala --- .../kubeportforward/portForward_test.go | 64 ++++++++++++++----- 1 file changed, 47 insertions(+), 17 deletions(-) diff --git a/pkg/portForward/kubeportforward/portForward_test.go b/pkg/portForward/kubeportforward/portForward_test.go index 5588be1b304..e65d9731c7d 100644 --- a/pkg/portForward/kubeportforward/portForward_test.go +++ b/pkg/portForward/kubeportforward/portForward_test.go @@ -1,21 +1,29 @@ package kubeportforward import ( + "fmt" "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" - "github.com/google/go-cmp/cmp" "github.com/redhat-developer/odo/pkg/api" + "strconv" + "strings" "testing" ) func Test_getCompleteCustomPortPairs(t *testing.T) { + const ( + acceptablePortRange = "[20001-30001]" + acceptableMinPort = 20001 + acceptableMaxPort = 30001 + ) type args struct { definedPorts []api.ForwardedPort ceMapping map[string][]v1alpha2.Endpoint } tests := []struct { - name string - args args - wantPortPairs map[string][]string + name string + args args + // wantPortPairs format: {containerName: {containerPort: localPort}} + wantPortPairs map[string]map[string]string }{ // TODO: Add test cases. { @@ -23,16 +31,15 @@ func Test_getCompleteCustomPortPairs(t *testing.T) { args: args{ definedPorts: []api.ForwardedPort{ {ContainerName: "runtime", LocalPort: 8080, ContainerPort: 8000}, - {ContainerName: "tools", LocalPort: 5000, ContainerPort: 5000}, }, ceMapping: map[string][]v1alpha2.Endpoint{ "runtime": {{TargetPort: 8000}, {TargetPort: 9000}}, "tools": {{TargetPort: 5000}}, }, }, - wantPortPairs: map[string][]string{ - "runtime": {"8080:8000", "20001:9000"}, - "tools": {"5000:5000"}, + wantPortPairs: map[string]map[string]string{ + "runtime": {"8000": "8080", "9000": acceptablePortRange}, + "tools": {"5000": acceptablePortRange}, }, }, { @@ -47,17 +54,17 @@ func Test_getCompleteCustomPortPairs(t *testing.T) { "tools": {{TargetPort: 5000}}, }, }, - wantPortPairs: map[string][]string{ - "runtime": {"8080:8000", "20001:9000"}, - "tools": {"5000:5000"}, + wantPortPairs: map[string]map[string]string{ + "runtime": {"8000": "8080", "9000": acceptablePortRange}, + "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: 25001, ContainerPort: 8000}, + {LocalPort: 25002, ContainerPort: 9000}, {LocalPort: 5000, ContainerPort: 5000}, }, ceMapping: map[string][]v1alpha2.Endpoint{ @@ -65,16 +72,39 @@ func Test_getCompleteCustomPortPairs(t *testing.T) { "tools": {{TargetPort: 5000}, {TargetPort: 8080}}, }, }, - wantPortPairs: map[string][]string{ - "runtime": {"20001:8000", "20002:9000"}, - "tools": {"5000:5000", "20003:8080"}, + wantPortPairs: map[string]map[string]string{ + "runtime": {"8000": "25001", "9000": "25002"}, + "tools": {"5000": "5000", "8080": acceptablePortRange}, }, }, } 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 != "" { + + validatePortPairs := func(gotPortPairs map[string][]string, wantPortPairs map[string]map[string]string) (diff string) { + for container, portPairs := range gotPortPairs { + wantPortPair := wantPortPairs[container] + for _, portPair := range portPairs { + portMap := strings.Split(portPair, ":") + lPort, cPort := portMap[0], portMap[1] + wantLPort := wantPortPair[cPort] + if wantLPort == acceptablePortRange { + if intLPort, _ := strconv.Atoi(lPort); intLPort >= acceptableMinPort && intLPort <= acceptableMaxPort { + continue + } else { + diff += fmt.Sprintf("[container %q] %s:%s is not in range %s\n", container, cPort, lPort, acceptablePortRange) + } + } else if wantLPort == lPort { + continue + } else { + diff += fmt.Sprintf("[container %q] %s:%s does not match %s:%s\n", container, cPort, lPort, cPort, wantLPort) + } + } + } + return diff + } + if diff := validatePortPairs(gotPortPairs, tt.wantPortPairs); diff != "" { t.Errorf("getCompleteCustomPortPairs() (got vs want) diff = %v", diff) } }) From fb31c33af01308ce14df8a6d1d2768b20c6117ca Mon Sep 17 00:00:00 2001 From: Parthvi Vala Date: Mon, 17 Apr 2023 17:25:40 +0530 Subject: [PATCH 3/4] Revert "Use range check" This reverts commit 1bcbacf18414320b470f594e9ec7717563d7da82. --- .../kubeportforward/portForward_test.go | 64 +++++-------------- 1 file changed, 17 insertions(+), 47 deletions(-) diff --git a/pkg/portForward/kubeportforward/portForward_test.go b/pkg/portForward/kubeportforward/portForward_test.go index e65d9731c7d..5588be1b304 100644 --- a/pkg/portForward/kubeportforward/portForward_test.go +++ b/pkg/portForward/kubeportforward/portForward_test.go @@ -1,29 +1,21 @@ package kubeportforward import ( - "fmt" "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" + "github.com/google/go-cmp/cmp" "github.com/redhat-developer/odo/pkg/api" - "strconv" - "strings" "testing" ) func Test_getCompleteCustomPortPairs(t *testing.T) { - const ( - acceptablePortRange = "[20001-30001]" - acceptableMinPort = 20001 - acceptableMaxPort = 30001 - ) type args struct { definedPorts []api.ForwardedPort ceMapping map[string][]v1alpha2.Endpoint } tests := []struct { - name string - args args - // wantPortPairs format: {containerName: {containerPort: localPort}} - wantPortPairs map[string]map[string]string + name string + args args + wantPortPairs map[string][]string }{ // TODO: Add test cases. { @@ -31,15 +23,16 @@ func Test_getCompleteCustomPortPairs(t *testing.T) { args: args{ definedPorts: []api.ForwardedPort{ {ContainerName: "runtime", LocalPort: 8080, ContainerPort: 8000}, + {ContainerName: "tools", LocalPort: 5000, ContainerPort: 5000}, }, ceMapping: map[string][]v1alpha2.Endpoint{ "runtime": {{TargetPort: 8000}, {TargetPort: 9000}}, "tools": {{TargetPort: 5000}}, }, }, - wantPortPairs: map[string]map[string]string{ - "runtime": {"8000": "8080", "9000": acceptablePortRange}, - "tools": {"5000": acceptablePortRange}, + wantPortPairs: map[string][]string{ + "runtime": {"8080:8000", "20001:9000"}, + "tools": {"5000:5000"}, }, }, { @@ -54,17 +47,17 @@ func Test_getCompleteCustomPortPairs(t *testing.T) { "tools": {{TargetPort: 5000}}, }, }, - wantPortPairs: map[string]map[string]string{ - "runtime": {"8000": "8080", "9000": acceptablePortRange}, - "tools": {"5000": "5000"}, + wantPortPairs: map[string][]string{ + "runtime": {"8080:8000", "20001:9000"}, + "tools": {"5000:5000"}, }, }, { name: "local ports in range [20001-30001] are provided as custom forward ports", args: args{ definedPorts: []api.ForwardedPort{ - {LocalPort: 25001, ContainerPort: 8000}, - {LocalPort: 25002, ContainerPort: 9000}, + {LocalPort: 20001, ContainerPort: 8000}, + {LocalPort: 20002, ContainerPort: 9000}, {LocalPort: 5000, ContainerPort: 5000}, }, ceMapping: map[string][]v1alpha2.Endpoint{ @@ -72,39 +65,16 @@ func Test_getCompleteCustomPortPairs(t *testing.T) { "tools": {{TargetPort: 5000}, {TargetPort: 8080}}, }, }, - wantPortPairs: map[string]map[string]string{ - "runtime": {"8000": "25001", "9000": "25002"}, - "tools": {"5000": "5000", "8080": acceptablePortRange}, + 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) - - validatePortPairs := func(gotPortPairs map[string][]string, wantPortPairs map[string]map[string]string) (diff string) { - for container, portPairs := range gotPortPairs { - wantPortPair := wantPortPairs[container] - for _, portPair := range portPairs { - portMap := strings.Split(portPair, ":") - lPort, cPort := portMap[0], portMap[1] - wantLPort := wantPortPair[cPort] - if wantLPort == acceptablePortRange { - if intLPort, _ := strconv.Atoi(lPort); intLPort >= acceptableMinPort && intLPort <= acceptableMaxPort { - continue - } else { - diff += fmt.Sprintf("[container %q] %s:%s is not in range %s\n", container, cPort, lPort, acceptablePortRange) - } - } else if wantLPort == lPort { - continue - } else { - diff += fmt.Sprintf("[container %q] %s:%s does not match %s:%s\n", container, cPort, lPort, cPort, wantLPort) - } - } - } - return diff - } - if diff := validatePortPairs(gotPortPairs, tt.wantPortPairs); diff != "" { + if diff := cmp.Diff(gotPortPairs, tt.wantPortPairs); diff != "" { t.Errorf("getCompleteCustomPortPairs() (got vs want) diff = %v", diff) } }) From 5afb86b7b1ed245d59abbed3c4febea5f68a8698 Mon Sep 17 00:00:00 2001 From: Parthvi Vala Date: Mon, 17 Apr 2023 20:40:33 +0530 Subject: [PATCH 4/4] Use order to iterate over ceMapping Signed-off-by: Parthvi Vala --- pkg/portForward/kubeportforward/portForward.go | 14 ++++++++++++-- .../kubeportforward/portForward_test.go | 3 +-- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/pkg/portForward/kubeportforward/portForward.go b/pkg/portForward/kubeportforward/portForward.go index 0afc248a075..653447cf0f8 100644 --- a/pkg/portForward/kubeportforward/portForward.go +++ b/pkg/portForward/kubeportforward/portForward.go @@ -5,6 +5,7 @@ import ( "fmt" "io" "reflect" + "sort" "time" "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" @@ -69,7 +70,6 @@ func (o *PFClient) StartPortForwarding(devFileObj parser.DevfileObj, componentNa klog.V(4).Infof("no endpoint declared in the component, no ports are forwarded") return nil } - o.stopChan = make(chan struct{}, 1) var portPairs map[string][]string @@ -193,7 +193,17 @@ func getCustomPortPairs(definedPorts []api.ForwardedPort, ceMapping map[string][ } startPort := 20001 endPort := startPort + 10000 - for name, ports := range ceMapping { + + // Prepare to iterate over containers so that we can iterate in an orderly manner + // This is better to ensure same result every time + var containers []string + for container := range ceMapping { + containers = append(containers, container) + } + sort.Strings(containers) + + for _, name := range containers { + ports := ceMapping[name] for _, p := range ports { freePort := getCustomLocalPort(p.TargetPort, name) if freePort == 0 { diff --git a/pkg/portForward/kubeportforward/portForward_test.go b/pkg/portForward/kubeportforward/portForward_test.go index 5588be1b304..bf4a4332f40 100644 --- a/pkg/portForward/kubeportforward/portForward_test.go +++ b/pkg/portForward/kubeportforward/portForward_test.go @@ -23,7 +23,6 @@ func Test_getCompleteCustomPortPairs(t *testing.T) { args: args{ definedPorts: []api.ForwardedPort{ {ContainerName: "runtime", LocalPort: 8080, ContainerPort: 8000}, - {ContainerName: "tools", LocalPort: 5000, ContainerPort: 5000}, }, ceMapping: map[string][]v1alpha2.Endpoint{ "runtime": {{TargetPort: 8000}, {TargetPort: 9000}}, @@ -32,7 +31,7 @@ func Test_getCompleteCustomPortPairs(t *testing.T) { }, wantPortPairs: map[string][]string{ "runtime": {"8080:8000", "20001:9000"}, - "tools": {"5000:5000"}, + "tools": {"20002:5000"}, }, }, {