From b4bc9a8ec22f178023bba160a8a5eb1d40d3cbce Mon Sep 17 00:00:00 2001 From: Armel Soro Date: Wed, 14 Sep 2022 08:54:17 +0200 Subject: [PATCH] Make sure to port-forward all endpoints, regardless of exposure (#6119) Defining endpoints for Debug ports is required for Devfiles to work properly with `odo dev --debug`. This addresses the question of exposure for such endpoints. Tools like DevSpaces currently set 'exposure: none' for debug endpoints. This makes sense from the developer perspective, as 'odo dev' aims at emulating a local developer machine. As a consequence, `odo dev` should no longer ignore endpoints with 'exposure: none'. They should be port-forwarded just like other endpoints. --- pkg/libdevfile/libdevfile.go | 6 +- pkg/libdevfile/libdevfile_test.go | 40 +++++++++-- pkg/odo/cli/dev/dev.go | 2 +- tests/integration/cmd_dev_test.go | 108 +++++++----------------------- 4 files changed, 64 insertions(+), 92 deletions(-) diff --git a/pkg/libdevfile/libdevfile.go b/pkg/libdevfile/libdevfile.go index 020909e653e..3183e2d5aff 100644 --- a/pkg/libdevfile/libdevfile.go +++ b/pkg/libdevfile/libdevfile.go @@ -269,7 +269,7 @@ func execDevfileEvent(devfileObj parser.DevfileObj, events []string, handler Han return nil } -// GetContainerEndpointMapping returns a map of container names and slice of its endpoints (in int) with exposure status other than none +// GetContainerEndpointMapping returns a map of container names and slice of its endpoints (in int) func GetContainerEndpointMapping(containers []v1alpha2.Component) map[string][]int { ceMapping := make(map[string][]int) for _, container := range containers { @@ -284,9 +284,7 @@ func GetContainerEndpointMapping(containers []v1alpha2.Component) map[string][]i endpoints := container.Container.Endpoints for _, e := range endpoints { - if e.Exposure != v1alpha2.NoneEndpointExposure { - ceMapping[k] = append(ceMapping[k], e.TargetPort) - } + ceMapping[k] = append(ceMapping[k], e.TargetPort) } } return ceMapping diff --git a/pkg/libdevfile/libdevfile_test.go b/pkg/libdevfile/libdevfile_test.go index 20fe833d2ec..eff8eda3c9e 100644 --- a/pkg/libdevfile/libdevfile_test.go +++ b/pkg/libdevfile/libdevfile_test.go @@ -530,6 +530,17 @@ func TestGetContainerEndpointMapping(t *testing.T) { }, }) + containerWithOneNoneInternalEndpoint := generator.GetContainerComponent(generator.ContainerComponentParams{ + Name: "container-none-endpoint", + Endpoints: []v1alpha2.Endpoint{ + { + Name: "debug", + TargetPort: 9099, + Exposure: v1alpha2.NoneEndpointExposure, + }, + }, + }) + tests := []struct { name string args args @@ -552,16 +563,37 @@ func TestGetContainerEndpointMapping(t *testing.T) { { name: "multiple containers with varying types of endpoints", args: args{ - containers: []v1alpha2.Component{containerWithNoEndpoints, containerWithOnePublicEndpoint, containerWithOneInternalEndpoint}, + containers: []v1alpha2.Component{ + containerWithNoEndpoints, + containerWithOnePublicEndpoint, + containerWithOneInternalEndpoint, + containerWithOneNoneInternalEndpoint, + }, + }, + want: map[string][]int{ + containerWithNoEndpoints.Name: {}, + containerWithOnePublicEndpoint.Name: {8080}, + containerWithOneInternalEndpoint.Name: {9090}, + containerWithOneNoneInternalEndpoint.Name: {9099}, }, - want: map[string][]int{containerWithNoEndpoints.Name: {}, containerWithOnePublicEndpoint.Name: {8080}, containerWithOneInternalEndpoint.Name: {9090}}, }, { name: "invalid input - one image component with rest being containers", args: args{ - containers: []v1alpha2.Component{containerWithNoEndpoints, containerWithOnePublicEndpoint, containerWithOneInternalEndpoint, imageComponent}, + containers: []v1alpha2.Component{ + containerWithNoEndpoints, + containerWithOnePublicEndpoint, + containerWithOneInternalEndpoint, + containerWithOneNoneInternalEndpoint, + imageComponent, + }, + }, + want: map[string][]int{ + containerWithNoEndpoints.Name: {}, + containerWithOnePublicEndpoint.Name: {8080}, + containerWithOneInternalEndpoint.Name: {9090}, + containerWithOneNoneInternalEndpoint.Name: {9099}, }, - want: map[string][]int{containerWithNoEndpoints.Name: {}, containerWithOnePublicEndpoint.Name: {8080}, containerWithOneInternalEndpoint.Name: {9090}}, }, } for _, tt := range tests { diff --git a/pkg/odo/cli/dev/dev.go b/pkg/odo/cli/dev/dev.go index 9cd05dbe59e..1f713da5db4 100644 --- a/pkg/odo/cli/dev/dev.go +++ b/pkg/odo/cli/dev/dev.go @@ -307,7 +307,7 @@ func NewCmdDev(name, fullName string) *cobra.Command { Use: name, Short: "Deploy component to development cluster", Long: `odo dev is a long running command that will automatically sync your source to the cluster. -It forwards endpoints with exposure values 'public' or 'internal' to a port on localhost.`, +It forwards endpoints with any exposure values ('public', 'internal' or 'none') to a port on localhost.`, Example: fmt.Sprintf(devExample, fullName), Args: cobra.MaximumNArgs(0), Run: func(cmd *cobra.Command, args []string) { diff --git a/tests/integration/cmd_dev_test.go b/tests/integration/cmd_dev_test.go index 9c31c5ea49d..b828909628e 100644 --- a/tests/integration/cmd_dev_test.go +++ b/tests/integration/cmd_dev_test.go @@ -10,6 +10,7 @@ import ( "path/filepath" "regexp" "sort" + "strconv" "strings" "time" @@ -568,23 +569,23 @@ ComponentSettings: devSession.WaitEnd() }) - It("should expose two endpoints on localhost", func() { - url1 := fmt.Sprintf("http://%s", ports["3000"]) - url2 := fmt.Sprintf("http://%s", ports["4567"]) - - resp1, err := http.Get(url1) - Expect(err).ToNot(HaveOccurred()) - defer resp1.Body.Close() - - resp2, err := http.Get(url2) - Expect(err).ToNot(HaveOccurred()) - defer resp2.Body.Close() + It("should expose all endpoints on localhost regardless of exposure", func() { + getServerResponse := func(p int) string { + resp, err := http.Get(fmt.Sprintf("http://%s", ports[strconv.Itoa(p)])) + Expect(err).ToNot(HaveOccurred()) + defer resp.Body.Close() - body1, _ := io.ReadAll(resp1.Body) - helper.MatchAllInOutput(string(body1), []string{"Hello from Node.js Starter Application!"}) + body, _ := io.ReadAll(resp.Body) + return string(body) + } + containerPorts := []int{3000, 4567, 7890} - body2, _ := io.ReadAll(resp2.Body) - helper.MatchAllInOutput(string(body2), []string{"Hello from Node.js Starter Application!"}) + for _, p := range containerPorts { + By(fmt.Sprintf("exposing a port targeting container port %d", p), func() { + r := getServerResponse(p) + helper.MatchAllInOutput(r, []string{"Hello from Node.js Starter Application!"}) + }) + } helper.ReplaceString("server.js", "Hello from Node.js", "H3110 from Node.js") @@ -596,76 +597,17 @@ ComponentSettings: devSession.PressKey('p') } - _, _, _, err = devSession.WaitSync() + _, _, _, err := devSession.WaitSync() Expect(err).Should(Succeed()) - Eventually(func() bool { - resp3, err := http.Get(url1) - if err != nil { - return false - } - defer resp3.Body.Close() - - resp4, err := http.Get(url2) - if err != nil { - return false - } - defer resp4.Body.Close() - - body3, _ := io.ReadAll(resp3.Body) - if string(body3) != "H3110 from Node.js Starter Application!" { - return false - } - - body4, _ := io.ReadAll(resp4.Body) - return string(body4) == "H3110 from Node.js Starter Application!" - }, 180, 10).Should(Equal(true)) - }) - - When("an endpoint is added after first run of odo dev", func() { - - BeforeEach(func() { - helper.ReplaceString("devfile.yaml", "exposure: none", "exposure: public") - - if manual { - if os.Getenv("SKIP_KEY_PRESS") == "true" { - Skip("This is a unix-terminal specific scenario, skipping") - } - - devSession.PressKey('p') - } - - var err error - _, _, ports, err = devSession.WaitSync() - Expect(err).Should(Succeed()) - - }) - It("should expose three endpoints on localhost", func() { - url1 := fmt.Sprintf("http://%s", ports["3000"]) - url2 := fmt.Sprintf("http://%s", ports["4567"]) - url3 := fmt.Sprintf("http://%s", ports["7890"]) - - resp1, err := http.Get(url1) - Expect(err).ToNot(HaveOccurred()) - defer resp1.Body.Close() - - resp2, err := http.Get(url2) - Expect(err).ToNot(HaveOccurred()) - defer resp2.Body.Close() - - resp3, err := http.Get(url3) - Expect(err).ToNot(HaveOccurred()) - defer resp3.Body.Close() - - body1, _ := io.ReadAll(resp1.Body) - helper.MatchAllInOutput(string(body1), []string{"Hello from Node.js Starter Application!"}) - - body2, _ := io.ReadAll(resp2.Body) - helper.MatchAllInOutput(string(body2), []string{"Hello from Node.js Starter Application!"}) - - body3, _ := io.ReadAll(resp3.Body) - helper.MatchAllInOutput(string(body3), []string{"Hello from Node.js Starter Application!"}) - }) + for _, p := range containerPorts { + By(fmt.Sprintf("returning the right response when querying port forwarded for container port %d", p), + func() { + Eventually(func() string { + return getServerResponse(p) + }, 180, 10).Should(Equal("H3110 from Node.js Starter Application!")) + }) + } }) })