From 0ac62cc8ba29e2410ab49e822fff5b77d4902972 Mon Sep 17 00:00:00 2001 From: Armel Soro Date: Fri, 10 Feb 2023 14:42:20 +0100 Subject: [PATCH 01/15] Embed platform.Client interface in platform-specific interfaces This avoids repeating the same methods in both interfaces, and makes the intent clearer. --- pkg/kclient/interface.go | 12 ++---------- pkg/podman/interface.go | 28 +++++----------------------- 2 files changed, 7 insertions(+), 33 deletions(-) diff --git a/pkg/kclient/interface.go b/pkg/kclient/interface.go index b6f0f461e55..2fd205d72a4 100644 --- a/pkg/kclient/interface.go +++ b/pkg/kclient/interface.go @@ -22,16 +22,13 @@ import ( "k8s.io/client-go/rest" "k8s.io/client-go/tools/clientcmd" + "github.com/redhat-developer/odo/pkg/platform" bindingApi "github.com/redhat-developer/service-binding-operator/apis/binding/v1alpha1" specApi "github.com/redhat-developer/service-binding-operator/apis/spec/v1alpha3" ) type ClientInterface interface { - - // all.go - - // GetAllResourcesFromSelector returns all resources of any kind (including CRs) matching the given label selector - GetAllResourcesFromSelector(selector string, ns string) ([]unstructured.Unstructured, error) + platform.Client // binding.go IsServiceBindingSupported() (bool, error) @@ -107,12 +104,7 @@ type ClientInterface interface { TryWithBlockOwnerDeletion(ownerReference metav1.OwnerReference, exec func(ownerReference metav1.OwnerReference) error) error // pods.go - ExecCMDInContainer(containerName, podName string, cmd []string, stdout io.Writer, stderr io.Writer, stdin io.Reader, tty bool) error GetPodUsingComponentName(componentName string) (*corev1.Pod, error) - GetRunningPodFromSelector(selector string) (*corev1.Pod, error) - GetPodLogs(podName, containerName string, followLog bool) (io.ReadCloser, error) - GetAllPodsInNamespaceMatchingSelector(selector string, ns string) (*corev1.PodList, error) - GetPodsMatchingSelector(selector string) (*corev1.PodList, error) PodWatcher(ctx context.Context, selector string) (watch.Interface, error) IsPodNameMatchingSelector(ctx context.Context, podname string, selector string) (bool, error) diff --git a/pkg/podman/interface.go b/pkg/podman/interface.go index 1ed89b6de26..723fa77c238 100644 --- a/pkg/podman/interface.go +++ b/pkg/podman/interface.go @@ -1,14 +1,15 @@ package podman import ( - "io" + corev1 "k8s.io/api/core/v1" "github.com/redhat-developer/odo/pkg/api" - corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "github.com/redhat-developer/odo/pkg/platform" ) type Client interface { + platform.Client + // PlayKube creates the Pod with Podman PlayKube(pod *corev1.Pod) error @@ -30,28 +31,9 @@ type Client interface { // VolumeRm deletes the volume with given volumeName VolumeRm(volumeName string) error - // CleanupResources stops and removes a pod and its associated resources (volumes) + // CleanupPodResources stops and removes a pod and its associated resources (volumes) CleanupPodResources(pod *corev1.Pod) error - ExecCMDInContainer(containerName, podName string, cmd []string, stdout io.Writer, stderr io.Writer, stdin io.Reader, tty bool) error - - // GetPodLogs returns the logs of the specified pod container. - // All logs for all containers part of the pod are returned if an empty string is provided as container name. - GetPodLogs(podName, containerName string, followLog bool) (io.ReadCloser, error) - - // GetPodsMatchingSelector returns all pods matching the given label selector. - GetPodsMatchingSelector(selector string) (*corev1.PodList, error) - - // GetAllResourcesFromSelector returns all resources of any kind matching the given label selector. - GetAllResourcesFromSelector(selector string, ns string) ([]unstructured.Unstructured, error) - - // GetAllPodsInNamespaceMatchingSelector returns all pods matching the given label selector and in the specified namespace. - GetAllPodsInNamespaceMatchingSelector(selector string, ns string) (*corev1.PodList, error) - - // GetRunningPodFromSelector returns any pod matching the given label selector. - // If multiple pods are found, implementations might have different behavior, by either returning an error or returning any element. - GetRunningPodFromSelector(selector string) (*corev1.Pod, error) - ListAllComponents() ([]api.ComponentAbstract, error) Version() (SystemVersionReport, error) From 7419d14dbad1ce90d237e860e5a80d096e103f57 Mon Sep 17 00:00:00 2001 From: Armel Soro Date: Fri, 10 Feb 2023 14:45:33 +0100 Subject: [PATCH 02/15] Verify interface compliance of PodmanCli at compile time This is recommended in the Coding Conventions guidelines [1]. Specifically, what's important here is checking that it meets the 'platform.Client' contract. [1] https://github.com/redhat-developer/odo/wiki/Dev:-Coding-Conventions#verify-interface-compliance --- pkg/podman/podman.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pkg/podman/podman.go b/pkg/podman/podman.go index 75d48e6930c..c070722412c 100644 --- a/pkg/podman/podman.go +++ b/pkg/podman/podman.go @@ -8,6 +8,7 @@ import ( "strings" envcontext "github.com/redhat-developer/odo/pkg/config/context" + "github.com/redhat-developer/odo/pkg/platform" corev1 "k8s.io/api/core/v1" jsonserializer "k8s.io/apimachinery/pkg/runtime/serializer/json" @@ -19,6 +20,9 @@ type PodmanCli struct { podmanCmd string } +var _ Client = (*PodmanCli)(nil) +var _ platform.Client = (*PodmanCli)(nil) + // NewPodmanCli returns a new podman client, or nil if the podman command is not accessible in the system func NewPodmanCli(ctx context.Context) (*PodmanCli, error) { // Check if podman is available in the system From 25499f96fc5725def62cb1c7225ffc448a900104 Mon Sep 17 00:00:00 2001 From: Armel Soro Date: Fri, 10 Feb 2023 14:54:16 +0100 Subject: [PATCH 03/15] Move K8s-specific implementation of port-forwarding to a dedicated package This paves the way to providing a different implementation for Podman --- .../{ => kubeportforward}/portForward.go | 52 ++++++++----------- .../{ => kubeportforward}/writer.go | 2 +- .../{ => kubeportforward}/writer_test.go | 2 +- 3 files changed, 25 insertions(+), 31 deletions(-) rename pkg/portForward/{ => kubeportforward}/portForward.go (82%) rename pkg/portForward/{ => kubeportforward}/writer.go (99%) rename pkg/portForward/{ => kubeportforward}/writer_test.go (99%) diff --git a/pkg/portForward/portForward.go b/pkg/portForward/kubeportforward/portForward.go similarity index 82% rename from pkg/portForward/portForward.go rename to pkg/portForward/kubeportforward/portForward.go index 0408ae91f7d..6c4fa98fb13 100644 --- a/pkg/portForward/portForward.go +++ b/pkg/portForward/kubeportforward/portForward.go @@ -1,4 +1,4 @@ -package portForward +package kubeportforward import ( "errors" @@ -9,19 +9,20 @@ import ( "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" "github.com/devfile/library/v2/pkg/devfile/parser" - parsercommon "github.com/devfile/library/v2/pkg/devfile/parser/data/v2/common" "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/klog" + "github.com/redhat-developer/odo/pkg/api" "github.com/redhat-developer/odo/pkg/kclient" "github.com/redhat-developer/odo/pkg/libdevfile" "github.com/redhat-developer/odo/pkg/log" + "github.com/redhat-developer/odo/pkg/portForward" "github.com/redhat-developer/odo/pkg/state" "github.com/redhat-developer/odo/pkg/util" "github.com/redhat-developer/odo/pkg/watch" ) -var _ Client = (*PFClient)(nil) +var _ portForward.Client = (*PFClient)(nil) type PFClient struct { kubernetesClient kclient.ClientInterface @@ -47,15 +48,9 @@ func NewPFClient(kubernetesClient kclient.ClientInterface, stateClient state.Cli } } -func (o *PFClient) StartPortForwarding( - devFileObj parser.DevfileObj, - componentName string, - debug bool, - randomPorts bool, - errOut io.Writer, -) error { +func (o *PFClient) StartPortForwarding(devFileObj parser.DevfileObj, componentName string, debug bool, randomPorts bool, out io.Writer, errOut io.Writer, definedPorts []api.ForwardedPort) error { - ceMapping, err := o.GetPortsToForward(devFileObj, debug) + ceMapping, err := libdevfile.GetDevfileContainerEndpointMapping(devFileObj, debug) if err != nil { return err } @@ -66,7 +61,7 @@ func (o *PFClient) StartPortForwarding( o.appliedEndpoints = ceMapping - o.StopPortForwarding() + o.StopPortForwarding(componentName) if len(ceMapping) == 0 { klog.V(4).Infof("no endpoint declared in the component, no ports are forwarded") @@ -76,10 +71,22 @@ func (o *PFClient) StartPortForwarding( o.stopChan = make(chan struct{}, 1) var portPairs map[string][]string - if randomPorts { - portPairs = randomPortPairsFromContainerEndpoints(ceMapping) + if len(definedPorts) != 0 { + portPairs = make(map[string][]string) + for _, port := range definedPorts { + s := fmt.Sprintf("%d:%d", port.LocalPort, port.ContainerPort) + if port.LocalPort == 0 { + // random port + s = fmt.Sprintf(":%d", port.ContainerPort) + } + portPairs[port.ContainerName] = append(portPairs[port.ContainerName], s) + } } else { - portPairs = portPairsFromContainerEndpoints(ceMapping) + if randomPorts { + portPairs = randomPortPairsFromContainerEndpoints(ceMapping) + } else { + portPairs = portPairsFromContainerEndpoints(ceMapping) + } } var portPairsSlice []string for _, v1 := range portPairs { @@ -147,7 +154,7 @@ func (o *PFClient) StartPortForwarding( } } -func (o *PFClient) StopPortForwarding() { +func (o *PFClient) StopPortForwarding(componentName string) { if o.stopChan == nil { return } @@ -169,19 +176,6 @@ func (o *PFClient) GetForwardedPorts() map[string][]v1alpha2.Endpoint { return o.appliedEndpoints } -func (o *PFClient) GetPortsToForward(devFileObj parser.DevfileObj, includeDebug bool) (map[string][]v1alpha2.Endpoint, error) { - - // get the endpoint/port information for containers in devfile - containers, err := devFileObj.Data.GetComponents(parsercommon.DevfileOptions{ - ComponentOptions: parsercommon.ComponentOptions{ComponentType: v1alpha2.ContainerComponentType}, - }) - if err != nil { - return nil, err - } - ceMapping := libdevfile.GetContainerEndpointMapping(containers, includeDebug) - return ceMapping, nil -} - // randomPortPairsFromContainerEndpoints assigns a random (empty) port on localhost to each port in the provided containerEndpoints map // it returns a map of the format "":{":", ":"} // "container1": {":3000", ":3001"} diff --git a/pkg/portForward/writer.go b/pkg/portForward/kubeportforward/writer.go similarity index 99% rename from pkg/portForward/writer.go rename to pkg/portForward/kubeportforward/writer.go index 64fe97b9ea4..c4b3d847a06 100644 --- a/pkg/portForward/writer.go +++ b/pkg/portForward/kubeportforward/writer.go @@ -1,4 +1,4 @@ -package portForward +package kubeportforward import ( "errors" diff --git a/pkg/portForward/writer_test.go b/pkg/portForward/kubeportforward/writer_test.go similarity index 99% rename from pkg/portForward/writer_test.go rename to pkg/portForward/kubeportforward/writer_test.go index 00be2928955..3342ac52efe 100644 --- a/pkg/portForward/writer_test.go +++ b/pkg/portForward/kubeportforward/writer_test.go @@ -1,4 +1,4 @@ -package portForward +package kubeportforward import ( "testing" From 9817a9b842251b779b6af6a8c860a35398287463 Mon Sep 17 00:00:00 2001 From: Armel Soro Date: Fri, 10 Feb 2023 14:56:19 +0100 Subject: [PATCH 04/15] Remove GetPortsToForward method from the portForward.Client interface Current implementation relies on the Devfile object, so it makes more sense to be in the libdevfile package. --- .../adapters/kubernetes/component/adapter.go | 2 +- pkg/libdevfile/libdevfile.go | 14 ++++++++++++++ pkg/portForward/interface.go | 4 ---- 3 files changed, 15 insertions(+), 5 deletions(-) diff --git a/pkg/devfile/adapters/kubernetes/component/adapter.go b/pkg/devfile/adapters/kubernetes/component/adapter.go index 9138420b563..e8d0472bb7d 100644 --- a/pkg/devfile/adapters/kubernetes/component/adapter.go +++ b/pkg/devfile/adapters/kubernetes/component/adapter.go @@ -191,7 +191,7 @@ func (a Adapter) Push(ctx context.Context, parameters adapters.PushParameters, c } // Check if endpoints changed in Devfile - portsToForward, err := a.portForwardClient.GetPortsToForward(a.Devfile, parameters.Debug) + portsToForward, err := libdevfile.GetDevfileContainerEndpointMapping(a.Devfile, parameters.Debug) if err != nil { return err } diff --git a/pkg/libdevfile/libdevfile.go b/pkg/libdevfile/libdevfile.go index c43ae6adf13..61fca7f7e2c 100644 --- a/pkg/libdevfile/libdevfile.go +++ b/pkg/libdevfile/libdevfile.go @@ -277,6 +277,20 @@ func execDevfileEvent(devfileObj parser.DevfileObj, events []string, handler Han return nil } +// GetDevfileContainerEndpointMapping returns a map of container components names and slice of its endpoints, +// given a Devfile object in parameter. +// Debug endpoints will be included only if includeDebug is true. +func GetDevfileContainerEndpointMapping(devFileObj parser.DevfileObj, includeDebug bool) (map[string][]v1alpha2.Endpoint, error) { + // get the endpoint/port information for containers in devfile + containers, err := devFileObj.Data.GetComponents(common.DevfileOptions{ + ComponentOptions: common.ComponentOptions{ComponentType: v1alpha2.ContainerComponentType}, + }) + if err != nil { + return nil, err + } + return GetContainerEndpointMapping(containers, includeDebug), nil +} + // GetContainerEndpointMapping returns a map of container names and slice of its endpoints. // Debug endpoints will be included only if includeDebug is true. func GetContainerEndpointMapping(containers []v1alpha2.Component, includeDebug bool) map[string][]v1alpha2.Endpoint { diff --git a/pkg/portForward/interface.go b/pkg/portForward/interface.go index fbbd7719e3d..b26862c4b8a 100644 --- a/pkg/portForward/interface.go +++ b/pkg/portForward/interface.go @@ -25,8 +25,4 @@ type Client interface { // GetForwardedPorts returns the list of ports for each container currently forwarded. GetForwardedPorts() map[string][]v1alpha2.Endpoint - - // GetPortsToForward returns the endpoints to forward from the Devfile, by container name. - // Debug ports will be included only if includeDebug is true. - GetPortsToForward(devFileObj parser.DevfileObj, includeDebug bool) (map[string][]v1alpha2.Endpoint, error) } From 872156f8a4f597974d9c39657036dbf508674c7b Mon Sep 17 00:00:00 2001 From: Armel Soro Date: Fri, 10 Feb 2023 15:56:11 +0100 Subject: [PATCH 05/15] Monitor and send appropriate status events after starting a remote command process This allows callers to get more meaningful events about the process. --- pkg/remotecmd/kubeexec.go | 47 ++++++++++++++++++++++++++++------- pkg/remotecmd/kubexec_test.go | 6 ++--- pkg/remotecmd/types.go | 12 ++++----- 3 files changed, 47 insertions(+), 18 deletions(-) diff --git a/pkg/remotecmd/kubeexec.go b/pkg/remotecmd/kubeexec.go index 108d1069ad1..dc0a7afe719 100644 --- a/pkg/remotecmd/kubeexec.go +++ b/pkg/remotecmd/kubeexec.go @@ -95,24 +95,53 @@ func (k *kubeExecProcessHandler) StartProcessForCommand( fmt.Sprintf("echo $$ > %[1]s && %s %s (%s) 1>>/proc/1/fd/1 2>>/proc/1/fd/2; echo $? >> %[1]s", pidFile, cdCmd, setEnvCmd, cmdLine), } + //Monitoring go-routine + type event struct { + status RemoteProcessStatus + stdout []string + stderr []string + err error + } + eventsChan := make(chan event) + eventsReceived := make(map[RemoteProcessStatus]struct{}) go func() { - if outputHandler != nil { - outputHandler(Starting, nil, nil, nil) + for e := range eventsChan { + klog.V(5).Infof("event received for %q: %v, %v", def.Id, e.status, e.err) + if _, ok := eventsReceived[e.status]; ok { + continue + } + if outputHandler != nil { + outputHandler(e.status, e.stdout, e.stderr, e.err) + } + eventsReceived[e.status] = struct{}{} } + }() + + eventsChan <- event{status: Starting} + go func() { + eventsChan <- event{status: Running} stdout, stderr, err := k.execClient.ExecuteCommand(cmd, podName, containerName, false, nil, nil) if err != nil { klog.V(2).Infof("error while running background command: %v", err) } - if outputHandler != nil { - processInfo, infoErr := k.GetProcessInfoForCommand(def, podName, containerName) - if infoErr != nil { - outputHandler(Errored, stdout, stderr, err) - return - } - outputHandler(processInfo.Status, stdout, stderr, err) + processInfo, infoErr := k.GetProcessInfoForCommand(def, podName, containerName) + var status RemoteProcessStatus + if infoErr != nil { + status = Errored + } else { + status = processInfo.Status } + + eventsChan <- event{ + status: status, + stdout: stdout, + stderr: stderr, + err: err, + } + + close(eventsChan) }() return nil diff --git a/pkg/remotecmd/kubexec_test.go b/pkg/remotecmd/kubexec_test.go index a7f9780e0fb..dfc2ab918a8 100644 --- a/pkg/remotecmd/kubexec_test.go +++ b/pkg/remotecmd/kubexec_test.go @@ -223,7 +223,7 @@ func TestKubeExecProcessHandler_StartProcessForCommand(t *testing.T) { }) }, isCmdExpectedToRun: true, - expectedStatuses: []RemoteProcessStatus{Starting, Stopped}, + expectedStatuses: []RemoteProcessStatus{Starting, Running, Stopped}, }, { name: "command with all fields returned an error", @@ -250,7 +250,7 @@ func TestKubeExecProcessHandler_StartProcessForCommand(t *testing.T) { }) }, isCmdExpectedToRun: true, - expectedStatuses: []RemoteProcessStatus{Starting, Errored}, + expectedStatuses: []RemoteProcessStatus{Starting, Running, Errored}, }, } { t.Run(tt.name, func(t *testing.T) { @@ -264,7 +264,7 @@ func TestKubeExecProcessHandler_StartProcessForCommand(t *testing.T) { k := NewKubeExecProcessHandler(execClient) var wg sync.WaitGroup - wg.Add(2) //number of invocations of outputHandler + wg.Add(len(tt.expectedStatuses)) //number of invocations of outputHandler var statusesReported []RemoteProcessStatus err := k.StartProcessForCommand(tt.cmdDef, _podName, _containerName, func(status RemoteProcessStatus, stdout []string, stderr []string, err error) { diff --git a/pkg/remotecmd/types.go b/pkg/remotecmd/types.go index f3a4c3cc622..f05b1234c77 100644 --- a/pkg/remotecmd/types.go +++ b/pkg/remotecmd/types.go @@ -1,23 +1,23 @@ package remotecmd // RemoteProcessStatus is an enum type for representing process statuses. -type RemoteProcessStatus int +type RemoteProcessStatus string const ( // Unknown represents a process for which the status cannot be determined reliably or is not handled yet by us. - Unknown RemoteProcessStatus = iota + 1 + Unknown RemoteProcessStatus = "unknown" // Starting represents a process that is just about to start. - Starting + Starting = "starting" // Stopped represents a process stopped. - Stopped + Stopped = "stopped" // Errored represents a process that errored out, i.e. exited with a non-zero status code. - Errored + Errored = "errored" // Running represents a running process. - Running + Running = "running" ) const ( From 9f075734543eaf9f79297c5557c59b35600b1687 Mon Sep 17 00:00:00 2001 From: Armel Soro Date: Fri, 10 Feb 2023 15:38:48 +0100 Subject: [PATCH 06/15] Implement port-forwarding logic on Podman As explained in [1], this makes use of a helper sidecar container (aptly named "odo-helper-port-forwarding") to be added to the Pod Spec created by odo. In this scope, port-forwarding will be equivalent of executing a socat command in this helper container, like so: socat -d tcp-listen:20002,reuseaddr,fork tcp:localhost:5858 In the command above, this will open up port 20001 on the helper container, and forwarding requests to localhost:5858 (which would be in the application container, part of the same Pod) [1] https://github.com/redhat-developer/odo/issues/6510 --- .../podmanportforward/portForward.go | 130 ++++++++++++++++++ pkg/remotecmd/kubeexec.go | 6 +- pkg/remotecmd/types.go | 4 + 3 files changed, 139 insertions(+), 1 deletion(-) create mode 100644 pkg/portForward/podmanportforward/portForward.go diff --git a/pkg/portForward/podmanportforward/portForward.go b/pkg/portForward/podmanportforward/portForward.go new file mode 100644 index 00000000000..dbb1af6ea6f --- /dev/null +++ b/pkg/portForward/podmanportforward/portForward.go @@ -0,0 +1,130 @@ +package podmanportforward + +import ( + "fmt" + "io" + "reflect" + "strings" + "sync" + + "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" + "github.com/devfile/library/v2/pkg/devfile/parser" + "k8s.io/klog" + + "github.com/redhat-developer/odo/pkg/api" + "github.com/redhat-developer/odo/pkg/exec" + "github.com/redhat-developer/odo/pkg/portForward" + "github.com/redhat-developer/odo/pkg/remotecmd" +) + +const pfHelperContainer = "odo-helper-port-forwarding" + +type PFClient struct { + remoteProcessHandler remotecmd.RemoteProcessHandler + + appliedPorts map[api.ForwardedPort]struct{} +} + +var _ portForward.Client = (*PFClient)(nil) + +func NewPFClient(execClient exec.Client) *PFClient { + return &PFClient{ + remoteProcessHandler: remotecmd.NewKubeExecProcessHandler(execClient), + appliedPorts: make(map[api.ForwardedPort]struct{}), + } +} + +func (o *PFClient) StartPortForwarding( + devFileObj parser.DevfileObj, + componentName string, + debug bool, + randomPorts bool, + out io.Writer, + errOut io.Writer, + definedPorts []api.ForwardedPort, +) error { + var appliedPorts []api.ForwardedPort + for port := range o.appliedPorts { + appliedPorts = append(appliedPorts, port) + } + if reflect.DeepEqual(appliedPorts, definedPorts) { + klog.V(3).Infof("Port forwarding should already be running for defined ports: %v", definedPorts) + return nil + } + + o.StopPortForwarding(componentName) + + outputHandler := func(fwPort api.ForwardedPort) remotecmd.CommandOutputHandler { + return func(status remotecmd.RemoteProcessStatus, stdout []string, stderr []string, err error) { + klog.V(4).Infof("Status for port-forwarding (from %s:%d -> %d): %s", fwPort.LocalAddress, fwPort.LocalPort, fwPort.ContainerPort, status) + klog.V(4).Info(strings.Join(stdout, "\n")) + klog.V(4).Info(strings.Join(stderr, "\n")) + switch status { + case remotecmd.Running: + o.appliedPorts[fwPort] = struct{}{} + case remotecmd.Stopped, remotecmd.Errored: + delete(o.appliedPorts, fwPort) + if status == remotecmd.Stopped { + fmt.Fprintf(out, "Stopped port-forwarding from %s:%d -> %d", fwPort.LocalAddress, fwPort.LocalPort, fwPort.ContainerPort) + } + } + } + } + + for _, port := range definedPorts { + err := o.remoteProcessHandler.StartProcessForCommand(getCommandDefinition(port), getPodName(componentName), pfHelperContainer, outputHandler(port)) + if err != nil { + return fmt.Errorf("error while creating port-forwarding for container port %d: %w", port.ContainerPort, err) + } + o.appliedPorts[port] = struct{}{} + } + return nil +} + +func (o *PFClient) StopPortForwarding(componentName string) { + if len(o.appliedPorts) == 0 { + return + } + + var wg sync.WaitGroup + wg.Add(len(o.appliedPorts)) + for port := range o.appliedPorts { + port := port + go func() { + defer wg.Done() + err := o.remoteProcessHandler.StopProcessForCommand(getCommandDefinition(port), getPodName(componentName), pfHelperContainer) + if err != nil { + klog.V(4).Infof("error while stopping port-forwarding for container port %d: %v", port.ContainerPort, err) + } + }() + } + wg.Wait() + + o.appliedPorts = make(map[api.ForwardedPort]struct{}) +} + +func (o *PFClient) GetForwardedPorts() map[string][]v1alpha2.Endpoint { + result := make(map[string][]v1alpha2.Endpoint) + for port := range o.appliedPorts { + result[port.ContainerName] = append(result[port.ContainerName], v1alpha2.Endpoint{ + Name: port.PortName, + TargetPort: port.ContainerPort, + Exposure: v1alpha2.EndpointExposure(port.Exposure), + }) + } + return result +} + +func getPodName(componentName string) string { + return fmt.Sprintf("%s-app", componentName) +} + +func getCommandDefinition(port api.ForwardedPort) remotecmd.CommandDefinition { + return remotecmd.CommandDefinition{ + Id: fmt.Sprintf("pf-%s", port.PortName), + // PidDirectory needs to be writable + PidDirectory: "/projects/", + //TODO(rm3l) Use the right L4 protocol: tcp or udp? + CmdLine: fmt.Sprintf("socat -d tcp-listen:%d,reuseaddr,fork tcp:localhost:%d", port.LocalPort, port.ContainerPort), + } +} diff --git a/pkg/remotecmd/kubeexec.go b/pkg/remotecmd/kubeexec.go index dc0a7afe719..f62404f246f 100644 --- a/pkg/remotecmd/kubeexec.go +++ b/pkg/remotecmd/kubeexec.go @@ -358,5 +358,9 @@ func (k *kubeExecProcessHandler) getProcessChildren(pid int, podName string, con // The parent folder is supposed to be existing, because it should be mounted in the container using the mandatory // shared volume (more info in the AddOdoMandatoryVolume function from the utils package). func getPidFileForCommand(def CommandDefinition) string { - return fmt.Sprintf("%s/.odo_cmd_%s.pid", strings.TrimSuffix(storage.SharedDataMountPath, "/"), def.Id) + parentDir := def.PidDirectory + if parentDir == "" { + parentDir = storage.SharedDataMountPath + } + return fmt.Sprintf("%s/.odo_cmd_%s.pid", strings.TrimSuffix(parentDir, "/"), def.Id) } diff --git a/pkg/remotecmd/types.go b/pkg/remotecmd/types.go index f05b1234c77..44b36fb23be 100644 --- a/pkg/remotecmd/types.go +++ b/pkg/remotecmd/types.go @@ -39,6 +39,10 @@ type CommandDefinition struct { // Id is any unique (and short) identifier that helps manage the process associated to this command. Id string + // PidDirectory is the directory where the PID file for this process will be stored. + // The directory needs to be present in the remote container and be writable by the user (in the container) executing the command. + PidDirectory string + // WorkingDir is the working directory from which the command should get executed. WorkingDir string From 2802ed6af8344213791de8e34981b8b892db50e0 Mon Sep 17 00:00:00 2001 From: Armel Soro Date: Fri, 10 Feb 2023 15:41:04 +0100 Subject: [PATCH 07/15] Update portForward.Client interface methods Specifically, the 'StartPortForwarding' method can now accept an explicit list of ports that needs to be forwarded, if the caller can compute provide such information. This is currently useful on Podman where the ports (even the random ones) are known in advance. --- pkg/devfile/adapters/kubernetes/component/adapter.go | 4 ++-- pkg/portForward/interface.go | 9 +++++++-- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/pkg/devfile/adapters/kubernetes/component/adapter.go b/pkg/devfile/adapters/kubernetes/component/adapter.go index e8d0472bb7d..a7854be86bc 100644 --- a/pkg/devfile/adapters/kubernetes/component/adapter.go +++ b/pkg/devfile/adapters/kubernetes/component/adapter.go @@ -338,10 +338,10 @@ func (a Adapter) Push(ctx context.Context, parameters adapters.PushParameters, c } if podChanged || portsChanged { - a.portForwardClient.StopPortForwarding() + a.portForwardClient.StopPortForwarding(a.ComponentName) } - err = a.portForwardClient.StartPortForwarding(a.Devfile, a.ComponentName, parameters.Debug, parameters.RandomPorts, parameters.ErrOut) + err = a.portForwardClient.StartPortForwarding(a.Devfile, a.ComponentName, parameters.Debug, parameters.RandomPorts, log.GetStdout(), parameters.ErrOut, nil) if err != nil { return adapters.NewErrPortForward(err) } diff --git a/pkg/portForward/interface.go b/pkg/portForward/interface.go index b26862c4b8a..c8e0a386d37 100644 --- a/pkg/portForward/interface.go +++ b/pkg/portForward/interface.go @@ -5,6 +5,8 @@ import ( "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" "github.com/devfile/library/v2/pkg/devfile/parser" + + "github.com/redhat-developer/odo/pkg/api" ) type Client interface { @@ -12,16 +14,19 @@ type Client interface { // componentName indicates the name of component in the Devfile // randomPorts indicates to affect random ports, instead of stable ports starting at 20001 // output will be written to errOut writer + // definedPorts allows callers to explicitly define the mapping they want to set. StartPortForwarding( devFileObj parser.DevfileObj, componentName string, debug bool, randomPorts bool, + out io.Writer, errOut io.Writer, + definedPorts []api.ForwardedPort, ) error - // StopPortForwarding stops the port forwarding - StopPortForwarding() + // StopPortForwarding stops the port forwarding for the specified component. + StopPortForwarding(componentName string) // GetForwardedPorts returns the list of ports for each container currently forwarded. GetForwardedPorts() map[string][]v1alpha2.Endpoint From dc5fec02f8757967a4512ec668250b61e15fb373 Mon Sep 17 00:00:00 2001 From: Armel Soro Date: Fri, 10 Feb 2023 15:52:17 +0100 Subject: [PATCH 08/15] Add helper sidecar container to the Pod Spec generated on Podman As explained in [1], this helper sidecar container (aptly named "odo-helper-port-forwarding") will hold the actual container/host ports mapping in the spec (to overcome the limitation of using hostPort against a container app listening on the loopback interface); this running container will be used to execute the actual port-forwarding commands (based on socat), e.g: ``` apiVersion: v1 kind: Pod metadata: name: my-component-app labels: app: my-component-app spec: containers: - name: runtime command: [ 'tail', '-f', '/dev/null'] # Omitted for brevity, but imagine an application being executed by odo # and listening on both 0.0.0.0:3000 and 127.0.0.1:5858 - name: odo-helper-port-forwarding image: quay.io/devfile/base-developer-image:ubi8-latest command: [ 'tail', '-f', '/dev/null'] ports: - containerPort: 20001 # A command will be executed by odo, e.g.: 'socat -d -d tcp-listen:20001,reuseaddr,fork tcp:localhost:3000' hostPort: 20001 - containerPort: 20002 # A command will be executed by odo, e.g.: 'socat -d -d tcp-listen:20002,reuseaddr,fork tcp:localhost:5858' hostPort: 20002 # ... Omitted for brevity ``` In this scope, port-forwarding from 20002 to 5858 for example will be equivalent of executing a socat command in this helper container, like so: socat -d tcp-listen:20002,reuseaddr,fork tcp:localhost:5858 In the command above, this will open up port 20001 on the helper container, and forwarding requests to localhost:5858 (which would be in the 'runtime' container, part of the same Pod) [1] https://github.com/redhat-developer/odo/issues/6510 --- pkg/dev/podmandev/pod.go | 121 +++++++++++++++-------- pkg/dev/podmandev/pod_test.go | 172 ++++++++++++++++++++++++++++++--- pkg/dev/podmandev/reconcile.go | 2 +- 3 files changed, 242 insertions(+), 53 deletions(-) diff --git a/pkg/dev/podmandev/pod.go b/pkg/dev/podmandev/pod.go index 97e301f8af9..faa07ae04bf 100644 --- a/pkg/dev/podmandev/pod.go +++ b/pkg/dev/podmandev/pod.go @@ -23,6 +23,12 @@ import ( "k8s.io/klog" ) +// See https://github.com/devfile/developer-images and https://quay.io/repository/devfile/base-developer-image?tab=tags +const ( + portForwardingHelperContainerName = "odo-helper-port-forwarding" + portForwardingHelperImage = "quay.io/devfile/base-developer-image@sha256:27d5ce66a259decb84770ea0d1ce8058a806f39dfcfeed8387f9cf2f29e76480" +) + func createPodFromComponent( devfileObj parser.DevfileObj, componentName string, @@ -42,23 +48,14 @@ func createPodFromComponent( return nil, nil, fmt.Errorf("no valid components found in the devfile") } - containers, err = utils.UpdateContainersEntrypointsIfNeeded(devfileObj, containers, buildCommand, runCommand, debugCommand) + fwPorts, err := getPortMapping(devfileObj, debug, randomPorts, usedPorts) if err != nil { return nil, nil, err } + utils.AddOdoProjectVolume(&containers) utils.AddOdoMandatoryVolume(&containers) - // get the endpoint/port information for containers in devfile - containerComponents, err := devfileObj.Data.GetComponents(common.DevfileOptions{ - ComponentOptions: common.ComponentOptions{ComponentType: v1alpha2.ContainerComponentType}, - }) - if err != nil { - return nil, nil, err - } - ceMapping := libdevfile.GetContainerEndpointMapping(containerComponents, debug) - fwPorts := addHostPorts(containers, ceMapping, debug, randomPorts, usedPorts) - volumes := []corev1.Volume{ { Name: storage.OdoSourceVolume, @@ -98,6 +95,34 @@ func createPodFromComponent( } } + containers, err = utils.UpdateContainersEntrypointsIfNeeded(devfileObj, containers, buildCommand, runCommand, debugCommand) + if err != nil { + return nil, nil, err + } + + // Remove all containerPorts, as they will be set afterwards in the helper container + for i := range containers { + containers[i].Ports = nil + } + // Add helper container for port-forwarding + pfHelperContainer := corev1.Container{ + Name: portForwardingHelperContainerName, + Image: portForwardingHelperImage, + Command: []string{"tail"}, + Args: []string{"-f", "/dev/null"}, + } + for _, fwPort := range fwPorts { + pfHelperContainer.Ports = append(pfHelperContainer.Ports, corev1.ContainerPort{ + // It is intentional here to use the same port as ContainerPort and HostPort, for simplicity. + // In the helper container, a process will be run afterwards and will be listening on this port; + // this process will leverage socat to forward requests to the actual application port. + Name: fwPort.PortName, + ContainerPort: int32(fwPort.LocalPort), + HostPort: int32(fwPort.LocalPort), + }) + } + containers = append(containers, pfHelperContainer) + pod := corev1.Pod{ Spec: corev1.PodSpec{ Containers: containers, @@ -123,21 +148,44 @@ func getVolumeName(volume string, componentName string, appName string) string { return volume + "-" + componentName + "-" + appName } -func addHostPorts(containers []corev1.Container, ceMapping map[string][]v1alpha2.Endpoint, debug bool, randomPorts bool, usedPorts []int) []api.ForwardedPort { +func getPortMapping(devfileObj parser.DevfileObj, debug bool, randomPorts bool, usedPorts []int) ([]api.ForwardedPort, error) { + containerComponents, err := devfileObj.Data.GetComponents(common.DevfileOptions{ + ComponentOptions: common.ComponentOptions{ComponentType: v1alpha2.ContainerComponentType}, + }) + if err != nil { + return nil, err + } + ceMapping := libdevfile.GetContainerEndpointMapping(containerComponents, debug) + + var existingContainerPorts []int + for _, endpoints := range ceMapping { + for _, ep := range endpoints { + existingContainerPorts = append(existingContainerPorts, ep.TargetPort) + } + } + + isPortUsedInContainer := func(p int) bool { + for _, port := range existingContainerPorts { + if p == port { + return true + } + } + return false + } + var result []api.ForwardedPort startPort := 20001 endPort := startPort + 10000 usedPortsCopy := make([]int, len(usedPorts)) copy(usedPortsCopy, usedPorts) - for i := range containers { - var ports []corev1.ContainerPort - for _, port := range containers[i].Ports { - containerName := containers[i].Name - portName := port.Name + for containerName, endpoints := range ceMapping { + epLoop: + for _, ep := range endpoints { + portName := ep.Name isDebugPort := libdevfile.IsDebugPort(portName) if !debug && isDebugPort { - klog.V(4).Infof("not running in Debug mode, so skipping container Debug port: %v:%v:%v", - containerName, portName, port.ContainerPort) + klog.V(4).Infof("not running in Debug mode, so skipping Debug endpoint %s (%d) for container %q", + portName, ep.TargetPort, containerName) continue } var freePort int @@ -149,23 +197,27 @@ func addHostPorts(containers []corev1.Container, ceMapping map[string][]v1alpha2 rand.Seed(time.Now().UnixNano()) //#nosec for { freePort = rand.Intn(endPort-startPort+1) + startPort //#nosec - if util.IsPortFree(freePort) { + if !isPortUsedInContainer(freePort) && util.IsPortFree(freePort) { break } time.Sleep(100 * time.Millisecond) } } } else { - var err error - freePort, err = util.NextFreePort(startPort, endPort, usedPorts) - if err != nil { - klog.Infof("%s", err) - continue + for { + freePort, err = util.NextFreePort(startPort, endPort, usedPorts) + if err != nil { + klog.Infof("%s", err) + continue epLoop + } + if !isPortUsedInContainer(freePort) { + break + } + startPort = freePort + 1 + time.Sleep(100 * time.Millisecond) } startPort = freePort + 1 } - // Find the endpoint in the container-endpoint mapping - containerPort := int(port.ContainerPort) fp := api.ForwardedPort{ Platform: commonflags.PlatformPodman, PortName: portName, @@ -173,22 +225,13 @@ func addHostPorts(containers []corev1.Container, ceMapping map[string][]v1alpha2 ContainerName: containerName, LocalAddress: "127.0.0.1", LocalPort: freePort, - ContainerPort: containerPort, - } - - for _, ep := range ceMapping[containerName] { - if ep.TargetPort == containerPort { - fp.Exposure = string(ep.Exposure) - break - } + ContainerPort: ep.TargetPort, + Exposure: string(ep.Exposure), } result = append(result, fp) - port.HostPort = int32(freePort) - ports = append(ports, port) } - containers[i].Ports = ports } - return result + return result, nil } func addVolumeMountToContainer(containers []corev1.Container, devfileVolume storage.LocalStorage) error { diff --git a/pkg/dev/podmandev/pod_test.go b/pkg/dev/podmandev/pod_test.go index 390a65e90ab..9b690e18487 100644 --- a/pkg/dev/podmandev/pod_test.go +++ b/pkg/dev/podmandev/pod_test.go @@ -90,6 +90,12 @@ var ( }, }, }, + { + Args: []string{"-f", "/dev/null"}, + Command: []string{"tail"}, + Image: portForwardingHelperImage, + Name: portForwardingHelperContainerName, + }, }, Volumes: []corev1.Volume{ { @@ -219,10 +225,9 @@ func Test_createPodFromComponent(t *testing.T) { }, wantPod: func() *corev1.Pod { pod := basePod.DeepCopy() - pod.Spec.Containers[0].Ports = append(pod.Spec.Containers[0].Ports, corev1.ContainerPort{ + pod.Spec.Containers[1].Ports = append(pod.Spec.Containers[1].Ports, corev1.ContainerPort{ Name: "http", - ContainerPort: 8080, - Protocol: "TCP", + ContainerPort: 20001, HostPort: 20001, }) return pod @@ -264,10 +269,9 @@ func Test_createPodFromComponent(t *testing.T) { }, wantPod: func() *corev1.Pod { pod := basePod.DeepCopy() - pod.Spec.Containers[0].Ports = append(pod.Spec.Containers[0].Ports, corev1.ContainerPort{ + pod.Spec.Containers[1].Ports = append(pod.Spec.Containers[1].Ports, corev1.ContainerPort{ Name: "http", - ContainerPort: 8080, - Protocol: "TCP", + ContainerPort: 20001, HostPort: 20001, }) return pod @@ -310,16 +314,14 @@ func Test_createPodFromComponent(t *testing.T) { }, wantPod: func() *corev1.Pod { pod := basePod.DeepCopy() - pod.Spec.Containers[0].Ports = append(pod.Spec.Containers[0].Ports, corev1.ContainerPort{ + pod.Spec.Containers[1].Ports = append(pod.Spec.Containers[1].Ports, corev1.ContainerPort{ Name: "http", - ContainerPort: 8080, - Protocol: "TCP", + ContainerPort: 20001, HostPort: 20001, }) - pod.Spec.Containers[0].Ports = append(pod.Spec.Containers[0].Ports, corev1.ContainerPort{ + pod.Spec.Containers[1].Ports = append(pod.Spec.Containers[1].Ports, corev1.ContainerPort{ Name: "debug", - ContainerPort: 5858, - Protocol: "TCP", + ContainerPort: 20002, HostPort: 20002, }) return pod @@ -383,6 +385,150 @@ func Test_createPodFromComponent(t *testing.T) { return pod }, }, + { + name: "basic component + application endpoint + debug endpoint + container ports known - with debug", + args: args{ + devfileObj: func() parser.DevfileObj { + data, _ := data.NewDevfileData(string(data.APISchemaVersion200)) + _ = data.AddCommands([]v1alpha2.Command{command}) + cmp := baseComponent.DeepCopy() + cmp.Container.Endpoints = append(cmp.Container.Endpoints, v1alpha2.Endpoint{ + Name: "http", + TargetPort: 20001, + }) + cmp.Container.Endpoints = append(cmp.Container.Endpoints, v1alpha2.Endpoint{ + Name: "debug", + TargetPort: 20002, + }) + cmp.Container.Endpoints = append(cmp.Container.Endpoints, v1alpha2.Endpoint{ + Name: "debug-1", + TargetPort: 5858, + }) + _ = data.AddComponents([]v1alpha2.Component{*cmp}) + return parser.DevfileObj{ + Data: data, + } + }, + componentName: devfileName, + appName: appName, + debug: true, + }, + wantPod: func() *corev1.Pod { + pod := basePod.DeepCopy() + pod.Spec.Containers[1].Ports = append(pod.Spec.Containers[1].Ports, corev1.ContainerPort{ + Name: "http", + ContainerPort: 20003, + HostPort: 20003, + }) + pod.Spec.Containers[1].Ports = append(pod.Spec.Containers[1].Ports, corev1.ContainerPort{ + Name: "debug", + ContainerPort: 20004, + HostPort: 20004, + }) + pod.Spec.Containers[1].Ports = append(pod.Spec.Containers[1].Ports, corev1.ContainerPort{ + Name: "debug-1", + ContainerPort: 20005, + HostPort: 20005, + }) + return pod + }, + wantFwPorts: []api.ForwardedPort{ + { + Platform: "podman", + ContainerName: "mycomponent", + PortName: "http", + LocalAddress: "127.0.0.1", + LocalPort: 20003, + ContainerPort: 20001, + IsDebug: false, + }, + { + Platform: "podman", + ContainerName: "mycomponent", + PortName: "debug", + LocalAddress: "127.0.0.1", + LocalPort: 20004, + ContainerPort: 20002, + IsDebug: true, + }, + { + Platform: "podman", + ContainerName: "mycomponent", + PortName: "debug-1", + LocalAddress: "127.0.0.1", + LocalPort: 20005, + ContainerPort: 5858, + IsDebug: true, + }, + }, + }, + { + name: "basic component + application endpoint + debug endpoint + container ports known - without debug", + args: args{ + devfileObj: func() parser.DevfileObj { + data, _ := data.NewDevfileData(string(data.APISchemaVersion200)) + _ = data.AddCommands([]v1alpha2.Command{command}) + cmp := baseComponent.DeepCopy() + cmp.Container.Endpoints = append(cmp.Container.Endpoints, v1alpha2.Endpoint{ + Name: "http", + TargetPort: 20001, + }) + cmp.Container.Endpoints = append(cmp.Container.Endpoints, v1alpha2.Endpoint{ + Name: "debug", + TargetPort: 20002, + }) + cmp.Container.Endpoints = append(cmp.Container.Endpoints, v1alpha2.Endpoint{ + Name: "debug-1", + TargetPort: 5858, + }) + cmp.Container.Endpoints = append(cmp.Container.Endpoints, v1alpha2.Endpoint{ + Name: "http-1", + TargetPort: 8080, + }) + _ = data.AddComponents([]v1alpha2.Component{*cmp}) + return parser.DevfileObj{ + Data: data, + } + }, + componentName: devfileName, + appName: appName, + debug: false, + }, + wantPod: func() *corev1.Pod { + pod := basePod.DeepCopy() + pod.Spec.Containers[1].Ports = append(pod.Spec.Containers[1].Ports, corev1.ContainerPort{ + Name: "http", + ContainerPort: 20002, + HostPort: 20002, + }) + pod.Spec.Containers[1].Ports = append(pod.Spec.Containers[1].Ports, corev1.ContainerPort{ + Name: "http-1", + ContainerPort: 20003, + HostPort: 20003, + }) + return pod + }, + wantFwPorts: []api.ForwardedPort{ + { + Platform: "podman", + ContainerName: "mycomponent", + PortName: "http", + LocalAddress: "127.0.0.1", + LocalPort: 20002, + ContainerPort: 20001, + IsDebug: false, + }, + { + Platform: "podman", + ContainerName: "mycomponent", + PortName: "http-1", + LocalAddress: "127.0.0.1", + LocalPort: 20003, + ContainerPort: 8080, + IsDebug: false, + }, + }, + }, // TODO: Add test cases. } @@ -397,7 +543,7 @@ func Test_createPodFromComponent(t *testing.T) { tt.args.runCommand, tt.args.debugCommand, false, - []int{20001, 20002}, + []int{20001, 20002, 20003, 20004, 20005}, ) if (err != nil) != tt.wantErr { t.Errorf("createPodFromComponent() error = %v, wantErr %v", err, tt.wantErr) diff --git a/pkg/dev/podmandev/reconcile.go b/pkg/dev/podmandev/reconcile.go index ccfdf2e274f..0bc4654198a 100644 --- a/pkg/dev/podmandev/reconcile.go +++ b/pkg/dev/podmandev/reconcile.go @@ -171,7 +171,7 @@ func (o *DevClient) deployPod(ctx context.Context, options dev.StartOptions) (*c options.Debug, options.BuildCommand, options.RunCommand, - "", + options.DebugCommand, options.RandomPorts, o.usedPorts, ) From ea088f6ff77106e8499e5ee00528765bc216228f Mon Sep 17 00:00:00 2001 From: Armel Soro Date: Fri, 10 Feb 2023 15:53:46 +0100 Subject: [PATCH 09/15] Inject the right portForward client depending on the platform --- pkg/dev/podmandev/podmandev.go | 26 +++++++++++-------- .../genericclioptions/clientset/clientset.go | 10 ++++++- 2 files changed, 24 insertions(+), 12 deletions(-) diff --git a/pkg/dev/podmandev/podmandev.go b/pkg/dev/podmandev/podmandev.go index f38baf5daab..5b37b64e2fd 100644 --- a/pkg/dev/podmandev/podmandev.go +++ b/pkg/dev/podmandev/podmandev.go @@ -23,6 +23,7 @@ import ( "github.com/redhat-developer/odo/pkg/log" odocontext "github.com/redhat-developer/odo/pkg/odo/context" "github.com/redhat-developer/odo/pkg/podman" + "github.com/redhat-developer/odo/pkg/portForward" "github.com/redhat-developer/odo/pkg/state" "github.com/redhat-developer/odo/pkg/sync" "github.com/redhat-developer/odo/pkg/testingutil/filesystem" @@ -41,11 +42,12 @@ const ( type DevClient struct { fs filesystem.Filesystem - podmanClient podman.Client - syncClient sync.Client - execClient exec.Client - stateClient state.Client - watchClient watch.Client + podmanClient podman.Client + portForwardClient portForward.Client + syncClient sync.Client + execClient exec.Client + stateClient state.Client + watchClient watch.Client deployedPod *corev1.Pod usedPorts []int @@ -56,18 +58,20 @@ var _ dev.Client = (*DevClient)(nil) func NewDevClient( fs filesystem.Filesystem, podmanClient podman.Client, + portForwardClient portForward.Client, syncClient sync.Client, execClient exec.Client, stateClient state.Client, watchClient watch.Client, ) *DevClient { return &DevClient{ - fs: fs, - podmanClient: podmanClient, - syncClient: syncClient, - execClient: execClient, - stateClient: stateClient, - watchClient: watchClient, + fs: fs, + podmanClient: podmanClient, + portForwardClient: portForwardClient, + syncClient: syncClient, + execClient: execClient, + stateClient: stateClient, + watchClient: watchClient, } } diff --git a/pkg/odo/genericclioptions/clientset/clientset.go b/pkg/odo/genericclioptions/clientset/clientset.go index 160e2d2e5f0..3093d327b54 100644 --- a/pkg/odo/genericclioptions/clientset/clientset.go +++ b/pkg/odo/genericclioptions/clientset/clientset.go @@ -21,6 +21,8 @@ import ( "github.com/redhat-developer/odo/pkg/odo/commonflags" "github.com/redhat-developer/odo/pkg/podman" "github.com/redhat-developer/odo/pkg/portForward" + "github.com/redhat-developer/odo/pkg/portForward/kubeportforward" + "github.com/redhat-developer/odo/pkg/portForward/podmanportforward" "github.com/redhat-developer/odo/pkg/sync" "github.com/redhat-developer/odo/pkg/alizer" @@ -236,7 +238,12 @@ func Fetch(command *cobra.Command, platform string) (*Clientset, error) { dep.BindingClient = binding.NewBindingClient(dep.ProjectClient, dep.KubernetesClient) } if isDefined(command, PORT_FORWARD) { - dep.PortForwardClient = portForward.NewPFClient(dep.KubernetesClient, dep.StateClient) + switch platform { + case commonflags.PlatformPodman: + dep.PortForwardClient = podmanportforward.NewPFClient(dep.ExecClient) + default: + dep.PortForwardClient = kubeportforward.NewPFClient(dep.KubernetesClient, dep.StateClient) + } } if isDefined(command, DEV) { switch platform { @@ -244,6 +251,7 @@ func Fetch(command *cobra.Command, platform string) (*Clientset, error) { dep.DevClient = podmandev.NewDevClient( dep.FS, dep.PodmanClient, + dep.PortForwardClient, dep.SyncClient, dep.ExecClient, dep.StateClient, From c7e4b0ef576a392146b312982384ea3a33569329 Mon Sep 17 00:00:00 2001 From: Armel Soro Date: Fri, 10 Feb 2023 15:54:14 +0100 Subject: [PATCH 10/15] Delegate port-forwarding on Podman to the appropriate client --- pkg/dev/podmandev/reconcile.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/pkg/dev/podmandev/reconcile.go b/pkg/dev/podmandev/reconcile.go index 0bc4654198a..7bf916549e4 100644 --- a/pkg/dev/podmandev/reconcile.go +++ b/pkg/dev/podmandev/reconcile.go @@ -16,6 +16,7 @@ import ( "github.com/redhat-developer/odo/pkg/component" "github.com/redhat-developer/odo/pkg/dev" "github.com/redhat-developer/odo/pkg/devfile" + "github.com/redhat-developer/odo/pkg/devfile/adapters" "github.com/redhat-developer/odo/pkg/libdevfile" "github.com/redhat-developer/odo/pkg/log" odocontext "github.com/redhat-developer/odo/pkg/odo/context" @@ -123,6 +124,11 @@ func (o *DevClient) reconcile( return err } + err = o.portForwardClient.StartPortForwarding(*devfileObj, componentName, options.Debug, options.RandomPorts, out, errOut, fwPorts) + if err != nil { + return adapters.NewErrPortForward(err) + } + for _, fwPort := range fwPorts { s := fmt.Sprintf("Forwarding from %s:%d -> %d", fwPort.LocalAddress, fwPort.LocalPort, fwPort.ContainerPort) fmt.Fprintf(out, " - %s", log.SboldColor(color.FgGreen, s)) From 2c49fa89c6c95c14958e7452c97f868974ea9b40 Mon Sep 17 00:00:00 2001 From: Armel Soro Date: Tue, 28 Feb 2023 09:50:11 +0100 Subject: [PATCH 11/15] Implement --forward-localhost on Podman --- pkg/dev/interface.go | 3 ++ pkg/dev/podmandev/pod.go | 69 +++++++++++++++++++++++----------- pkg/dev/podmandev/pod_test.go | 16 ++++---- pkg/dev/podmandev/podmandev.go | 18 +++++---- pkg/dev/podmandev/reconcile.go | 21 +++++++---- pkg/odo/cli/dev/dev.go | 40 +++++++++++++------- pkg/watch/watch.go | 2 + 7 files changed, 110 insertions(+), 59 deletions(-) diff --git a/pkg/dev/interface.go b/pkg/dev/interface.go index eded4ce005e..3b21499c263 100644 --- a/pkg/dev/interface.go +++ b/pkg/dev/interface.go @@ -23,6 +23,9 @@ type StartOptions struct { // IgnoreLocalhost indicates whether to proceed with port-forwarding regardless of any container ports being bound to the container loopback interface. // Applicable to Podman only. IgnoreLocalhost bool + // ForwardLocalhost is a flag indicating if we inject a side container that will make port-forwarding work with container apps listening on the loopback interface. + // Applicable to Podman only. + ForwardLocalhost bool // Variables to override in the Devfile Variables map[string]string } diff --git a/pkg/dev/podmandev/pod.go b/pkg/dev/podmandev/pod.go index faa07ae04bf..7a981ae1250 100644 --- a/pkg/dev/podmandev/pod.go +++ b/pkg/dev/podmandev/pod.go @@ -37,6 +37,7 @@ func createPodFromComponent( buildCommand string, runCommand string, debugCommand string, + withHelperContainer bool, randomPorts bool, usedPorts []int, ) (*corev1.Pod, []api.ForwardedPort, error) { @@ -100,28 +101,7 @@ func createPodFromComponent( return nil, nil, err } - // Remove all containerPorts, as they will be set afterwards in the helper container - for i := range containers { - containers[i].Ports = nil - } - // Add helper container for port-forwarding - pfHelperContainer := corev1.Container{ - Name: portForwardingHelperContainerName, - Image: portForwardingHelperImage, - Command: []string{"tail"}, - Args: []string{"-f", "/dev/null"}, - } - for _, fwPort := range fwPorts { - pfHelperContainer.Ports = append(pfHelperContainer.Ports, corev1.ContainerPort{ - // It is intentional here to use the same port as ContainerPort and HostPort, for simplicity. - // In the helper container, a process will be run afterwards and will be listening on this port; - // this process will leverage socat to forward requests to the actual application port. - Name: fwPort.PortName, - ContainerPort: int32(fwPort.LocalPort), - HostPort: int32(fwPort.LocalPort), - }) - } - containers = append(containers, pfHelperContainer) + containers = addHostPorts(withHelperContainer, containers, fwPorts) pod := corev1.Pod{ Spec: corev1.PodSpec{ @@ -144,6 +124,51 @@ func createPodFromComponent( return &pod, fwPorts, nil } +func addHostPorts(withHelperContainer bool, containers []corev1.Container, fwPorts []api.ForwardedPort) []corev1.Container { + if withHelperContainer { + // A side helper container is added and will be responsible for redirecting the traffic, + // so it can work even if the application is listening on the container loopback interface. + for i := range containers { + containers[i].Ports = nil + } + // Add helper container for port-forwarding + pfHelperContainer := corev1.Container{ + Name: portForwardingHelperContainerName, + Image: portForwardingHelperImage, + Command: []string{"tail"}, + Args: []string{"-f", "/dev/null"}, + } + for _, fwPort := range fwPorts { + pfHelperContainer.Ports = append(pfHelperContainer.Ports, corev1.ContainerPort{ + // It is intentional here to use the same port as ContainerPort and HostPort, for simplicity. + // In the helper container, a process will be run afterwards and will be listening on this port; + // this process will leverage socat to forward requests to the actual application port. + Name: fwPort.PortName, + ContainerPort: int32(fwPort.LocalPort), + HostPort: int32(fwPort.LocalPort), + }) + } + containers = append(containers, pfHelperContainer) + } else { + // the original ports in container contains all Devfile endpoints that have been set by the Devfile library. + // We need to filter them out, to set only the ports that we need to port-forward. + for i := range containers { + var containerPorts []corev1.ContainerPort + for _, p := range containers[i].Ports { + for _, fwPort := range fwPorts { + if containers[i].Name == fwPort.ContainerName && int(p.ContainerPort) == fwPort.ContainerPort { + p.HostPort = int32(fwPort.LocalPort) + containerPorts = append(containerPorts, p) + break + } + } + } + containers[i].Ports = containerPorts + } + } + return containers +} + func getVolumeName(volume string, componentName string, appName string) string { return volume + "-" + componentName + "-" + appName } diff --git a/pkg/dev/podmandev/pod_test.go b/pkg/dev/podmandev/pod_test.go index 9b690e18487..4e51f19c822 100644 --- a/pkg/dev/podmandev/pod_test.go +++ b/pkg/dev/podmandev/pod_test.go @@ -122,13 +122,14 @@ var ( func Test_createPodFromComponent(t *testing.T) { type args struct { - devfileObj func() parser.DevfileObj - componentName string - appName string - debug bool - buildCommand string - runCommand string - debugCommand string + devfileObj func() parser.DevfileObj + componentName string + appName string + debug bool + buildCommand string + runCommand string + debugCommand string + forwardLocalhost bool } tests := []struct { name string @@ -543,6 +544,7 @@ func Test_createPodFromComponent(t *testing.T) { tt.args.runCommand, tt.args.debugCommand, false, + tt.args.forwardLocalhost, []int{20001, 20002, 20003, 20004, 20005}, ) if (err != nil) != tt.wantErr { diff --git a/pkg/dev/podmandev/podmandev.go b/pkg/dev/podmandev/podmandev.go index 5b37b64e2fd..cfce1ec8522 100644 --- a/pkg/dev/podmandev/podmandev.go +++ b/pkg/dev/podmandev/podmandev.go @@ -112,6 +112,7 @@ func (o *DevClient) Start( Variables: options.Variables, RandomPorts: options.RandomPorts, IgnoreLocalhost: options.IgnoreLocalhost, + ForwardLocalhost: options.ForwardLocalhost, WatchFiles: options.WatchFiles, WatchCluster: false, Out: out, @@ -193,14 +194,15 @@ func (o *DevClient) watchHandler(ctx context.Context, pushParams adapters.PushPa printWarningsOnDevfileChanges(ctx, watchParams) startOptions := dev.StartOptions{ - IgnorePaths: watchParams.FileIgnores, - Debug: watchParams.Debug, - BuildCommand: watchParams.DevfileBuildCmd, - RunCommand: watchParams.DevfileRunCmd, - RandomPorts: watchParams.RandomPorts, - IgnoreLocalhost: watchParams.IgnoreLocalhost, - WatchFiles: watchParams.WatchFiles, - Variables: watchParams.Variables, + IgnorePaths: watchParams.FileIgnores, + Debug: watchParams.Debug, + BuildCommand: watchParams.DevfileBuildCmd, + RunCommand: watchParams.DevfileRunCmd, + RandomPorts: watchParams.RandomPorts, + IgnoreLocalhost: watchParams.IgnoreLocalhost, + ForwardLocalhost: watchParams.ForwardLocalhost, + WatchFiles: watchParams.WatchFiles, + Variables: watchParams.Variables, } return o.reconcile(ctx, watchParams.Out, watchParams.ErrOut, startOptions, componentStatus) } diff --git a/pkg/dev/podmandev/reconcile.go b/pkg/dev/podmandev/reconcile.go index 7bf916549e4..21b96a644ba 100644 --- a/pkg/dev/podmandev/reconcile.go +++ b/pkg/dev/podmandev/reconcile.go @@ -124,10 +124,13 @@ func (o *DevClient) reconcile( return err } - err = o.portForwardClient.StartPortForwarding(*devfileObj, componentName, options.Debug, options.RandomPorts, out, errOut, fwPorts) - if err != nil { - return adapters.NewErrPortForward(err) - } + if options.ForwardLocalhost { + // Port-forwarding is enabled by executing dedicated socat commands + err = o.portForwardClient.StartPortForwarding(*devfileObj, componentName, options.Debug, options.RandomPorts, out, errOut, fwPorts) + if err != nil { + return adapters.NewErrPortForward(err) + } + } // else port-forwarding is done via the main container ports in the pod spec for _, fwPort := range fwPorts { s := fmt.Sprintf("Forwarding from %s:%d -> %d", fwPort.LocalAddress, fwPort.LocalPort, fwPort.ContainerPort) @@ -178,6 +181,7 @@ func (o *DevClient) deployPod(ctx context.Context, options dev.StartOptions) (*c options.BuildCommand, options.RunCommand, options.DebugCommand, + options.ForwardLocalhost, options.RandomPorts, o.usedPorts, ) @@ -244,12 +248,13 @@ Either change the application to make those port(s) reachable on all interfaces - --ignore-localhost: no error will be returned by odo, but forwarding to those ports might not work on Podman. - --forward-localhost: odo will inject a dedicated side container to redirect traffic to such ports.` } - if !options.IgnoreLocalhost { + if options.IgnoreLocalhost { + // ForwardLocalhost should not be true at this point. + log.Warningf(msg) + } else if !options.ForwardLocalhost { log.Errorf(msg) - return errors.New("cannot make port forwarding work with applications listening only on the loopback interface") + return errors.New("cannot make port forwarding work with ports bound to the loopback interface only") } - // No error, but only a warning if using --ignore-localhost - log.Warningf(msg) return nil } diff --git a/pkg/odo/cli/dev/dev.go b/pkg/odo/cli/dev/dev.go index bd0dc661453..b30c26ec299 100644 --- a/pkg/odo/cli/dev/dev.go +++ b/pkg/odo/cli/dev/dev.go @@ -51,12 +51,13 @@ type DevOptions struct { cancel context.CancelFunc // Flags - noWatchFlag bool - randomPortsFlag bool - debugFlag bool - buildCommandFlag string - runCommandFlag string - ignoreLocalhostFlag bool + noWatchFlag bool + randomPortsFlag bool + debugFlag bool + buildCommandFlag string + runCommandFlag string + ignoreLocalhostFlag bool + forwardLocalhostFlag bool } var _ genericclioptions.Runnable = (*DevOptions)(nil) @@ -110,11 +111,17 @@ func (o *DevOptions) Validate(ctx context.Context) error { if o.ignoreLocalhostFlag { return errors.New("--ignore-localhost cannot be used when running in cluster mode") } + if o.forwardLocalhostFlag { + return errors.New("--forward-localhost cannot be used when running in cluster mode") + } if o.clientset.KubernetesClient == nil { return kclient.NewNoConnectionError() } scontext.SetPlatform(ctx, o.clientset.KubernetesClient) case commonflags.PlatformPodman: + if o.ignoreLocalhostFlag && o.forwardLocalhostFlag { + return errors.New("--ignore-localhost and --forward-localhost cannot be used together") + } if o.clientset.PodmanClient == nil { return podman.NewPodmanNotFoundError(nil) } @@ -185,14 +192,15 @@ func (o *DevOptions) Run(ctx context.Context) (err error) { o.out, o.errOut, dev.StartOptions{ - IgnorePaths: o.ignorePaths, - Debug: o.debugFlag, - BuildCommand: o.buildCommandFlag, - RunCommand: o.runCommandFlag, - RandomPorts: o.randomPortsFlag, - WatchFiles: !o.noWatchFlag, - IgnoreLocalhost: o.ignoreLocalhostFlag, - Variables: variables, + IgnorePaths: o.ignorePaths, + Debug: o.debugFlag, + BuildCommand: o.buildCommandFlag, + RunCommand: o.runCommandFlag, + RandomPorts: o.randomPortsFlag, + WatchFiles: !o.noWatchFlag, + IgnoreLocalhost: o.ignoreLocalhostFlag, + ForwardLocalhost: o.forwardLocalhostFlag, + Variables: variables, }, ) } @@ -236,6 +244,10 @@ It forwards endpoints with any exposure values ('public', 'internal' or 'none') "Whether to ignore errors related to port-forwarding apps listening on the container loopback interface. Applicable only if platform is podman.") // TODO Unhide when moving Podman out of the experimental mode : https://github.com/redhat-developer/odo/issues/6592 _ = devCmd.Flags().MarkHidden("ignore-localhost") + devCmd.Flags().BoolVar(&o.forwardLocalhostFlag, "forward-localhost", false, + "Whether to enable port-forwarding if app is listening on the container loopback interface. Applicable only if platform is podman.") + // TODO Unhide when moving Podman out of the experimental mode : https://github.com/redhat-developer/odo/issues/6592 + _ = devCmd.Flags().MarkHidden("forward-localhost") clientset.Add(devCmd, clientset.BINDING, diff --git a/pkg/watch/watch.go b/pkg/watch/watch.go index 148579e5fb7..109f81629fe 100644 --- a/pkg/watch/watch.go +++ b/pkg/watch/watch.go @@ -95,6 +95,8 @@ type WatchParameters struct { IgnoreLocalhost bool // WatchFiles indicates to watch for file changes and sync changes to the container WatchFiles bool + // ForwardLocalhost indicates whether to try to make port-forwarding work with container apps listening on the loopback interface. + ForwardLocalhost bool // WatchCluster indicates to watch Cluster-related objects (Deployment, Pod, etc) WatchCluster bool // ErrOut is a Writer to output forwarded port information From 3829bcf7d4af1a6bdf8d3d7dbf36bc3718a44a61 Mon Sep 17 00:00:00 2001 From: Armel Soro Date: Wed, 1 Mar 2023 18:33:06 +0100 Subject: [PATCH 12/15] Add unit and integration test cases --- pkg/dev/podmandev/pod_test.go | 475 ++++++++++++++++++++++++++++-- tests/integration/cmd_dev_test.go | 102 +++++++ 2 files changed, 546 insertions(+), 31 deletions(-) diff --git a/pkg/dev/podmandev/pod_test.go b/pkg/dev/podmandev/pod_test.go index 4e51f19c822..fa6d48a92cc 100644 --- a/pkg/dev/podmandev/pod_test.go +++ b/pkg/dev/podmandev/pod_test.go @@ -42,8 +42,10 @@ var ( volume = generator.GetVolumeComponent(generator.VolumeComponentParams{ Name: "myvolume", }) +) - basePod = &corev1.Pod{ +func buildBasePod(withPfContainer bool) *corev1.Pod { + basePod := corev1.Pod{ TypeMeta: v1.TypeMeta{ APIVersion: "v1", Kind: "Pod", @@ -90,12 +92,6 @@ var ( }, }, }, - { - Args: []string{"-f", "/dev/null"}, - Command: []string{"tail"}, - Image: portForwardingHelperImage, - Name: portForwardingHelperContainerName, - }, }, Volumes: []corev1.Volume{ { @@ -117,7 +113,16 @@ var ( }, }, } -) + if withPfContainer { + basePod.Spec.Containers = append(basePod.Spec.Containers, corev1.Container{ + Args: []string{"-f", "/dev/null"}, + Command: []string{"tail"}, + Image: portForwardingHelperImage, + Name: portForwardingHelperContainerName, + }) + } + return &basePod +} func Test_createPodFromComponent(t *testing.T) { @@ -134,12 +139,12 @@ func Test_createPodFromComponent(t *testing.T) { tests := []struct { name string args args - wantPod func() *corev1.Pod + wantPod func(basePod *corev1.Pod) *corev1.Pod wantFwPorts []api.ForwardedPort wantErr bool }{ { - name: "basic component without command", + name: "basic component without command / forwardLocalhost=false", args: args{ devfileObj: func() parser.DevfileObj { data, _ := data.NewDevfileData(string(data.APISchemaVersion200)) @@ -152,13 +157,32 @@ func Test_createPodFromComponent(t *testing.T) { componentName: devfileName, appName: appName, }, - wantPod: func() *corev1.Pod { + wantPod: func(basePod *corev1.Pod) *corev1.Pod { + return basePod.DeepCopy() + }, + }, + { + name: "basic component without command / forwardLocalhost=true", + args: args{ + devfileObj: func() parser.DevfileObj { + data, _ := data.NewDevfileData(string(data.APISchemaVersion200)) + _ = data.AddCommands([]v1alpha2.Command{command}) + _ = data.AddComponents([]v1alpha2.Component{baseComponent}) + return parser.DevfileObj{ + Data: data, + } + }, + componentName: devfileName, + appName: appName, + forwardLocalhost: true, + }, + wantPod: func(basePod *corev1.Pod) *corev1.Pod { pod := basePod.DeepCopy() return pod }, }, { - name: "basic component with command", + name: "basic component with command / forwardLocalhost=false", args: args{ devfileObj: func() parser.DevfileObj { data, _ := data.NewDevfileData(string(data.APISchemaVersion200)) @@ -174,7 +198,7 @@ func Test_createPodFromComponent(t *testing.T) { componentName: devfileName, appName: appName, }, - wantPod: func() *corev1.Pod { + wantPod: func(basePod *corev1.Pod) *corev1.Pod { pod := basePod.DeepCopy() pod.Spec.Containers[0].Command = []string{"./cmd"} pod.Spec.Containers[0].Args = []string{"arg1", "arg2"} @@ -182,7 +206,32 @@ func Test_createPodFromComponent(t *testing.T) { }, }, { - name: "basic component + memory limit", + name: "basic component with command / forwardLocalhost=true", + args: args{ + devfileObj: func() parser.DevfileObj { + data, _ := data.NewDevfileData(string(data.APISchemaVersion200)) + _ = data.AddCommands([]v1alpha2.Command{command}) + cmp := baseComponent.DeepCopy() + cmp.Container.Command = []string{"./cmd"} + cmp.Container.Args = []string{"arg1", "arg2"} + _ = data.AddComponents([]v1alpha2.Component{*cmp}) + return parser.DevfileObj{ + Data: data, + } + }, + componentName: devfileName, + appName: appName, + forwardLocalhost: true, + }, + wantPod: func(basePod *corev1.Pod) *corev1.Pod { + pod := basePod.DeepCopy() + pod.Spec.Containers[0].Command = []string{"./cmd"} + pod.Spec.Containers[0].Args = []string{"arg1", "arg2"} + return pod + }, + }, + { + name: "basic component + memory limit / forwardLocalhost=false", args: args{ devfileObj: func() parser.DevfileObj { data, _ := data.NewDevfileData(string(data.APISchemaVersion200)) @@ -197,7 +246,7 @@ func Test_createPodFromComponent(t *testing.T) { componentName: devfileName, appName: appName, }, - wantPod: func() *corev1.Pod { + wantPod: func(basePod *corev1.Pod) *corev1.Pod { pod := basePod.DeepCopy() pod.Spec.Containers[0].Resources.Limits = corev1.ResourceList{ "memory": resource.MustParse("1Gi"), @@ -206,7 +255,32 @@ func Test_createPodFromComponent(t *testing.T) { }, }, { - name: "basic component + application endpoint", + name: "basic component + memory limit / forwardLocalhost=true", + args: args{ + devfileObj: func() parser.DevfileObj { + data, _ := data.NewDevfileData(string(data.APISchemaVersion200)) + _ = data.AddCommands([]v1alpha2.Command{command}) + cmp := baseComponent.DeepCopy() + cmp.Container.MemoryLimit = "1Gi" + _ = data.AddComponents([]v1alpha2.Component{*cmp}) + return parser.DevfileObj{ + Data: data, + } + }, + componentName: devfileName, + appName: appName, + forwardLocalhost: true, + }, + wantPod: func(basePod *corev1.Pod) *corev1.Pod { + pod := basePod.DeepCopy() + pod.Spec.Containers[0].Resources.Limits = corev1.ResourceList{ + "memory": resource.MustParse("1Gi"), + } + return pod + }, + }, + { + name: "basic component + application endpoint / forwardLocalhost=false", args: args{ devfileObj: func() parser.DevfileObj { data, _ := data.NewDevfileData(string(data.APISchemaVersion200)) @@ -224,7 +298,49 @@ func Test_createPodFromComponent(t *testing.T) { componentName: devfileName, appName: appName, }, - wantPod: func() *corev1.Pod { + wantPod: func(basePod *corev1.Pod) *corev1.Pod { + pod := basePod.DeepCopy() + pod.Spec.Containers[0].Ports = append(pod.Spec.Containers[0].Ports, corev1.ContainerPort{ + Name: "http", + ContainerPort: 8080, + HostPort: 20001, + Protocol: corev1.ProtocolTCP, + }) + return pod + }, + wantFwPorts: []api.ForwardedPort{ + { + Platform: "podman", + ContainerName: "mycomponent", + PortName: "http", + LocalAddress: "127.0.0.1", + LocalPort: 20001, + ContainerPort: 8080, + IsDebug: false, + }, + }, + }, + { + name: "basic component + application endpoint / forwardLocalhost=true", + args: args{ + devfileObj: func() parser.DevfileObj { + data, _ := data.NewDevfileData(string(data.APISchemaVersion200)) + _ = data.AddCommands([]v1alpha2.Command{command}) + cmp := baseComponent.DeepCopy() + cmp.Container.Endpoints = append(cmp.Container.Endpoints, v1alpha2.Endpoint{ + Name: "http", + TargetPort: 8080, + }) + _ = data.AddComponents([]v1alpha2.Component{*cmp}) + return parser.DevfileObj{ + Data: data, + } + }, + componentName: devfileName, + appName: appName, + forwardLocalhost: true, + }, + wantPod: func(basePod *corev1.Pod) *corev1.Pod { pod := basePod.DeepCopy() pod.Spec.Containers[1].Ports = append(pod.Spec.Containers[1].Ports, corev1.ContainerPort{ Name: "http", @@ -246,7 +362,7 @@ func Test_createPodFromComponent(t *testing.T) { }, }, { - name: "basic component + application endpoint + debug endpoint - without debug", + name: "basic component + application endpoint + debug endpoint - without debug / forwardLocalhost=false", args: args{ devfileObj: func() parser.DevfileObj { data, _ := data.NewDevfileData(string(data.APISchemaVersion200)) @@ -268,7 +384,53 @@ func Test_createPodFromComponent(t *testing.T) { componentName: devfileName, appName: appName, }, - wantPod: func() *corev1.Pod { + wantPod: func(basePod *corev1.Pod) *corev1.Pod { + pod := basePod.DeepCopy() + pod.Spec.Containers[0].Ports = append(pod.Spec.Containers[0].Ports, corev1.ContainerPort{ + Name: "http", + ContainerPort: 8080, + HostPort: 20001, + Protocol: corev1.ProtocolTCP, + }) + return pod + }, + wantFwPorts: []api.ForwardedPort{ + { + Platform: "podman", + ContainerName: "mycomponent", + PortName: "http", + LocalAddress: "127.0.0.1", + LocalPort: 20001, + ContainerPort: 8080, + IsDebug: false, + }, + }, + }, + { + name: "basic component + application endpoint + debug endpoint - without debug / forwardLocalhost=true", + args: args{ + devfileObj: func() parser.DevfileObj { + data, _ := data.NewDevfileData(string(data.APISchemaVersion200)) + _ = data.AddCommands([]v1alpha2.Command{command}) + cmp := baseComponent.DeepCopy() + cmp.Container.Endpoints = append(cmp.Container.Endpoints, v1alpha2.Endpoint{ + Name: "http", + TargetPort: 8080, + }) + cmp.Container.Endpoints = append(cmp.Container.Endpoints, v1alpha2.Endpoint{ + Name: "debug", + TargetPort: 5858, + }) + _ = data.AddComponents([]v1alpha2.Component{*cmp}) + return parser.DevfileObj{ + Data: data, + } + }, + componentName: devfileName, + appName: appName, + forwardLocalhost: true, + }, + wantPod: func(basePod *corev1.Pod) *corev1.Pod { pod := basePod.DeepCopy() pod.Spec.Containers[1].Ports = append(pod.Spec.Containers[1].Ports, corev1.ContainerPort{ Name: "http", @@ -290,7 +452,7 @@ func Test_createPodFromComponent(t *testing.T) { }, }, { - name: "basic component + application endpoint + debug endpoint - with debug", + name: "basic component + application endpoint + debug endpoint - with debug / forwardLocalhost=false", args: args{ devfileObj: func() parser.DevfileObj { data, _ := data.NewDevfileData(string(data.APISchemaVersion200)) @@ -313,7 +475,69 @@ func Test_createPodFromComponent(t *testing.T) { appName: appName, debug: true, }, - wantPod: func() *corev1.Pod { + wantPod: func(basePod *corev1.Pod) *corev1.Pod { + pod := basePod.DeepCopy() + pod.Spec.Containers[0].Ports = append(pod.Spec.Containers[0].Ports, corev1.ContainerPort{ + Name: "http", + ContainerPort: 8080, + HostPort: 20001, + Protocol: corev1.ProtocolTCP, + }) + pod.Spec.Containers[0].Ports = append(pod.Spec.Containers[0].Ports, corev1.ContainerPort{ + Name: "debug", + ContainerPort: 5858, + HostPort: 20002, + Protocol: corev1.ProtocolTCP, + }) + return pod + }, + wantFwPorts: []api.ForwardedPort{ + { + Platform: "podman", + ContainerName: "mycomponent", + PortName: "http", + LocalAddress: "127.0.0.1", + LocalPort: 20001, + ContainerPort: 8080, + IsDebug: false, + }, + { + Platform: "podman", + ContainerName: "mycomponent", + PortName: "debug", + LocalAddress: "127.0.0.1", + LocalPort: 20002, + ContainerPort: 5858, + IsDebug: true, + }, + }, + }, + { + name: "basic component + application endpoint + debug endpoint - with debug / forwardLocalhost=true", + args: args{ + devfileObj: func() parser.DevfileObj { + data, _ := data.NewDevfileData(string(data.APISchemaVersion200)) + _ = data.AddCommands([]v1alpha2.Command{command}) + cmp := baseComponent.DeepCopy() + cmp.Container.Endpoints = append(cmp.Container.Endpoints, v1alpha2.Endpoint{ + Name: "http", + TargetPort: 8080, + }) + cmp.Container.Endpoints = append(cmp.Container.Endpoints, v1alpha2.Endpoint{ + Name: "debug", + TargetPort: 5858, + }) + _ = data.AddComponents([]v1alpha2.Component{*cmp}) + return parser.DevfileObj{ + Data: data, + } + }, + componentName: devfileName, + appName: appName, + debug: true, + forwardLocalhost: true, + }, + wantPod: func(basePod *corev1.Pod) *corev1.Pod { pod := basePod.DeepCopy() pod.Spec.Containers[1].Ports = append(pod.Spec.Containers[1].Ports, corev1.ContainerPort{ Name: "http", @@ -349,7 +573,7 @@ func Test_createPodFromComponent(t *testing.T) { }, }, { - name: "basic component with volume mount", + name: "basic component with volume mount / forwardLocalhost=false", args: args{ devfileObj: func() parser.DevfileObj { data, _ := data.NewDevfileData(string(data.APISchemaVersion200)) @@ -369,7 +593,46 @@ func Test_createPodFromComponent(t *testing.T) { componentName: devfileName, appName: appName, }, - wantPod: func() *corev1.Pod { + wantPod: func(basePod *corev1.Pod) *corev1.Pod { + pod := basePod.DeepCopy() + pod.Spec.Volumes = append(pod.Spec.Volumes, corev1.Volume{ + Name: volume.Name, + VolumeSource: corev1.VolumeSource{ + PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{ + ClaimName: volume.Name + "-" + devfileName + "-" + appName, + }, + }, + }) + pod.Spec.Containers[0].VolumeMounts = append(pod.Spec.Containers[0].VolumeMounts, corev1.VolumeMount{ + Name: volume.Name, + MountPath: "/path/to/mount", + }) + return pod + }, + }, + { + name: "basic component with volume mount / forwardLocalhost=true", + args: args{ + devfileObj: func() parser.DevfileObj { + data, _ := data.NewDevfileData(string(data.APISchemaVersion200)) + _ = data.AddCommands([]v1alpha2.Command{command}) + _ = data.AddComponents([]v1alpha2.Component{baseComponent, volume}) + _ = data.AddVolumeMounts(baseComponent.Name, []v1alpha2.VolumeMount{ + { + Name: volume.Name, + Path: "/path/to/mount", + }, + }) + + return parser.DevfileObj{ + Data: data, + } + }, + componentName: devfileName, + appName: appName, + forwardLocalhost: true, + }, + wantPod: func(basePod *corev1.Pod) *corev1.Pod { pod := basePod.DeepCopy() pod.Spec.Volumes = append(pod.Spec.Volumes, corev1.Volume{ Name: volume.Name, @@ -387,7 +650,7 @@ func Test_createPodFromComponent(t *testing.T) { }, }, { - name: "basic component + application endpoint + debug endpoint + container ports known - with debug", + name: "basic component + application endpoint + debug endpoint + container ports known - with debug / forwardLocalhost=false", args: args{ devfileObj: func() parser.DevfileObj { data, _ := data.NewDevfileData(string(data.APISchemaVersion200)) @@ -414,7 +677,88 @@ func Test_createPodFromComponent(t *testing.T) { appName: appName, debug: true, }, - wantPod: func() *corev1.Pod { + wantPod: func(basePod *corev1.Pod) *corev1.Pod { + pod := basePod.DeepCopy() + pod.Spec.Containers[0].Ports = append(pod.Spec.Containers[0].Ports, corev1.ContainerPort{ + Name: "http", + ContainerPort: 20001, + HostPort: 20003, + Protocol: corev1.ProtocolTCP, + }) + pod.Spec.Containers[0].Ports = append(pod.Spec.Containers[0].Ports, corev1.ContainerPort{ + Name: "debug", + ContainerPort: 20002, + HostPort: 20004, + Protocol: corev1.ProtocolTCP, + }) + pod.Spec.Containers[0].Ports = append(pod.Spec.Containers[0].Ports, corev1.ContainerPort{ + Name: "debug-1", + ContainerPort: 5858, + HostPort: 20005, + Protocol: corev1.ProtocolTCP, + }) + return pod + }, + wantFwPorts: []api.ForwardedPort{ + { + Platform: "podman", + ContainerName: "mycomponent", + PortName: "http", + LocalAddress: "127.0.0.1", + LocalPort: 20003, + ContainerPort: 20001, + IsDebug: false, + }, + { + Platform: "podman", + ContainerName: "mycomponent", + PortName: "debug", + LocalAddress: "127.0.0.1", + LocalPort: 20004, + ContainerPort: 20002, + IsDebug: true, + }, + { + Platform: "podman", + ContainerName: "mycomponent", + PortName: "debug-1", + LocalAddress: "127.0.0.1", + LocalPort: 20005, + ContainerPort: 5858, + IsDebug: true, + }, + }, + }, + { + name: "basic component + application endpoint + debug endpoint + container ports known - with debug / forwardLocalhost=true", + args: args{ + devfileObj: func() parser.DevfileObj { + data, _ := data.NewDevfileData(string(data.APISchemaVersion200)) + _ = data.AddCommands([]v1alpha2.Command{command}) + cmp := baseComponent.DeepCopy() + cmp.Container.Endpoints = append(cmp.Container.Endpoints, v1alpha2.Endpoint{ + Name: "http", + TargetPort: 20001, + }) + cmp.Container.Endpoints = append(cmp.Container.Endpoints, v1alpha2.Endpoint{ + Name: "debug", + TargetPort: 20002, + }) + cmp.Container.Endpoints = append(cmp.Container.Endpoints, v1alpha2.Endpoint{ + Name: "debug-1", + TargetPort: 5858, + }) + _ = data.AddComponents([]v1alpha2.Component{*cmp}) + return parser.DevfileObj{ + Data: data, + } + }, + componentName: devfileName, + appName: appName, + debug: true, + forwardLocalhost: true, + }, + wantPod: func(basePod *corev1.Pod) *corev1.Pod { pod := basePod.DeepCopy() pod.Spec.Containers[1].Ports = append(pod.Spec.Containers[1].Ports, corev1.ContainerPort{ Name: "http", @@ -464,7 +808,7 @@ func Test_createPodFromComponent(t *testing.T) { }, }, { - name: "basic component + application endpoint + debug endpoint + container ports known - without debug", + name: "basic component + application endpoint + debug endpoint + container ports known - without debug / forwardLocalhost=false", args: args{ devfileObj: func() parser.DevfileObj { data, _ := data.NewDevfileData(string(data.APISchemaVersion200)) @@ -495,7 +839,77 @@ func Test_createPodFromComponent(t *testing.T) { appName: appName, debug: false, }, - wantPod: func() *corev1.Pod { + wantPod: func(basePod *corev1.Pod) *corev1.Pod { + pod := basePod.DeepCopy() + pod.Spec.Containers[0].Ports = append(pod.Spec.Containers[0].Ports, corev1.ContainerPort{ + Name: "http", + ContainerPort: 20001, + HostPort: 20002, + Protocol: corev1.ProtocolTCP, + }) + pod.Spec.Containers[0].Ports = append(pod.Spec.Containers[0].Ports, corev1.ContainerPort{ + Name: "http-1", + ContainerPort: 8080, + HostPort: 20003, + Protocol: corev1.ProtocolTCP, + }) + return pod + }, + wantFwPorts: []api.ForwardedPort{ + { + Platform: "podman", + ContainerName: "mycomponent", + PortName: "http", + LocalAddress: "127.0.0.1", + LocalPort: 20002, + ContainerPort: 20001, + IsDebug: false, + }, + { + Platform: "podman", + ContainerName: "mycomponent", + PortName: "http-1", + LocalAddress: "127.0.0.1", + LocalPort: 20003, + ContainerPort: 8080, + IsDebug: false, + }, + }, + }, + { + name: "basic component + application endpoint + debug endpoint + container ports known - without debug / forwardLocalhost=true", + args: args{ + devfileObj: func() parser.DevfileObj { + data, _ := data.NewDevfileData(string(data.APISchemaVersion200)) + _ = data.AddCommands([]v1alpha2.Command{command}) + cmp := baseComponent.DeepCopy() + cmp.Container.Endpoints = append(cmp.Container.Endpoints, v1alpha2.Endpoint{ + Name: "http", + TargetPort: 20001, + }) + cmp.Container.Endpoints = append(cmp.Container.Endpoints, v1alpha2.Endpoint{ + Name: "debug", + TargetPort: 20002, + }) + cmp.Container.Endpoints = append(cmp.Container.Endpoints, v1alpha2.Endpoint{ + Name: "debug-1", + TargetPort: 5858, + }) + cmp.Container.Endpoints = append(cmp.Container.Endpoints, v1alpha2.Endpoint{ + Name: "http-1", + TargetPort: 8080, + }) + _ = data.AddComponents([]v1alpha2.Component{*cmp}) + return parser.DevfileObj{ + Data: data, + } + }, + componentName: devfileName, + appName: appName, + debug: false, + forwardLocalhost: true, + }, + wantPod: func(basePod *corev1.Pod) *corev1.Pod { pod := basePod.DeepCopy() pod.Spec.Containers[1].Ports = append(pod.Spec.Containers[1].Ports, corev1.ContainerPort{ Name: "http", @@ -530,8 +944,6 @@ func Test_createPodFromComponent(t *testing.T) { }, }, }, - - // TODO: Add test cases. } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -543,8 +955,8 @@ func Test_createPodFromComponent(t *testing.T) { tt.args.buildCommand, tt.args.runCommand, tt.args.debugCommand, - false, tt.args.forwardLocalhost, + false, []int{20001, 20002, 20003, 20004, 20005}, ) if (err != nil) != tt.wantErr { @@ -552,7 +964,8 @@ func Test_createPodFromComponent(t *testing.T) { return } - if diff := cmp.Diff(tt.wantPod(), got, cmpopts.EquateEmpty()); diff != "" { + basePod := buildBasePod(tt.args.forwardLocalhost) + if diff := cmp.Diff(tt.wantPod(basePod), got, cmpopts.EquateEmpty()); diff != "" { t.Errorf("createPodFromComponent() pod mismatch (-want +got):\n%s", diff) } if diff := cmp.Diff(tt.wantFwPorts, gotFwPorts, cmpopts.EquateEmpty()); diff != "" { diff --git a/tests/integration/cmd_dev_test.go b/tests/integration/cmd_dev_test.go index f6916cc117f..4aad3c8493e 100644 --- a/tests/integration/cmd_dev_test.go +++ b/tests/integration/cmd_dev_test.go @@ -3372,6 +3372,17 @@ CMD ["npm", "start"] stderr := helper.Cmd("odo", args...).AddEnv(env...).ShouldFail().Err() Expect(stderr).Should(ContainSubstring("--ignore-localhost cannot be used when running in cluster mode")) }) + + It("should error out if using --forward-localhost on any platform other than Podman", func() { + args := []string{"dev", "--forward-localhost", "--random-ports"} + var env []string + if plt != "" { + args = append(args, "--platform", plt) + env = append(env, "ODO_EXPERIMENTAL_MODE=true") + } + stderr := helper.Cmd("odo", args...).AddEnv(env...).ShouldFail().Err() + Expect(stderr).Should(ContainSubstring("--forward-localhost cannot be used when running in cluster mode")) + }) } When("running on default cluster platform", func() { @@ -3421,6 +3432,14 @@ CMD ["npm", "start"] Context("running on Podman", Label(helper.LabelPodman), func() { + It("should error out if using both --ignore-localhost and --forward-localhost", func() { + stderr := helper.Cmd("odo", "dev", "--random-ports", "--platform", "podman", "--ignore-localhost", "--forward-localhost"). + AddEnv("ODO_EXPERIMENTAL_MODE=true"). + ShouldFail(). + Err() + Expect(stderr).Should(ContainSubstring("--ignore-localhost and --forward-localhost cannot be used together")) + }) + It("should error out if not ignoring localhost", func() { stderr := helper.Cmd("odo", "dev", "--random-ports", "--platform", "podman").AddEnv("ODO_EXPERIMENTAL_MODE=true").ShouldFail().Err() Expect(stderr).Should(ContainSubstring("Detected that the following port(s) can be reached only via the container loopback interface: admin (3001)")) @@ -3452,6 +3471,11 @@ CMD ["npm", "start"] By("displaying warning message for loopback port", func() { Expect(stderr).Should(ContainSubstring("Detected that the following port(s) can be reached only via the container loopback interface: admin (3001)")) }) + By("creating a pod with a single container pod because --forward-localhost is false", func() { + podDef := helper.NewComponent(cmpName, "app", labels.ComponentDevMode, commonVar.Project, commonVar.CliRunner).GetPodDef() + Expect(podDef.Spec.Containers).Should(HaveLen(1)) + Expect(podDef.Spec.Containers[0].Name).Should(Equal(fmt.Sprintf("%s-app-runtime", cmpName))) + }) By("reaching the local port for the non-loopback interface", func() { Eventually(func(g Gomega) { g.Expect(ports["3000"]).Should(haveHttpResponse(http.StatusOK, "Hello from Node.js Application!")) @@ -3492,6 +3516,84 @@ CMD ["npm", "start"] }) }) }) + + When("forwarding localhost", func() { + var devSession helper.DevSession + var stdout, stderr string + var ports map[string]string + + BeforeEach(func() { + var bOut, bErr []byte + var err error + devSession, bOut, bErr, ports, err = helper.StartDevMode(helper.DevSessionOpts{ + RunOnPodman: true, + CmdlineArgs: []string{"--forward-localhost"}, + }) + Expect(err).ShouldNot(HaveOccurred()) + stdout = string(bOut) + stderr = string(bErr) + }) + + AfterEach(func() { + devSession.Stop() + devSession.WaitEnd() + }) + + It("should port-forward successfully", func() { + By("not displaying warning message for loopback port", func() { + for _, output := range []string{stdout, stderr} { + Expect(output).ShouldNot(ContainSubstring("Detected that the following port(s) can be reached only via the container loopback interface")) + } + }) + By("creating a pod with a two-containers pod because --forward-localhost is true", func() { + podDef := helper.NewComponent(cmpName, "app", labels.ComponentDevMode, commonVar.Project, commonVar.CliRunner).GetPodDef() + Expect(podDef.Spec.Containers).Should(HaveLen(2)) + var found bool + var pfHelperContainer corev1.Container + for _, container := range podDef.Spec.Containers { + if container.Name == fmt.Sprintf("%s-app-odo-helper-port-forwarding", cmpName) { + pfHelperContainer = container + found = true + break + } + } + Expect(found).Should(BeTrue(), fmt.Sprintf("Could not find container 'odo-helper-port-forwarding' in pod spec: %v", podDef)) + Expect(pfHelperContainer.Image).Should(HavePrefix("quay.io/devfile/base-developer-image")) + }) + By("reaching the local port for the non-loopback interface", func() { + Eventually(func(g Gomega) { + g.Expect(ports["3000"]).Should(haveHttpResponse(http.StatusOK, "Hello from Node.js Application!")) + }).WithTimeout(60 * time.Second).WithPolling(3 * time.Second).Should(Succeed()) + }) + By("reaching the local port for the loopback interface", func() { + Eventually(func(g Gomega) { + g.Expect(ports["3001"]).Should(haveHttpResponse(http.StatusOK, "Hello from Node.js Admin Application!")) + }).WithTimeout(60 * time.Second).WithPolling(3 * time.Second).Should(Succeed()) + }) + }) + + When("making changes in the project source code during the dev session", func() { + BeforeEach(func() { + helper.ReplaceString(filepath.Join(commonVar.Context, "server.js"), "Hello from", "Hiya from the updated") + var err error + _, _, ports, err = devSession.WaitSync() + Expect(err).ShouldNot(HaveOccurred()) + }) + + It("should port-forward successfully", func() { + By("reaching the local port for the non-loopback interface", func() { + Eventually(func(g Gomega) { + g.Expect(ports["3000"]).Should(haveHttpResponse(http.StatusOK, "Hiya from the updated Node.js Application!")) + }).WithTimeout(60 * time.Second).WithPolling(3 * time.Second).Should(Succeed()) + }) + By("reaching the local port for the loopback interface", func() { + Eventually(func(g Gomega) { + g.Expect(ports["3001"]).Should(haveHttpResponse(http.StatusOK, "Hiya from the updated Node.js Admin Application!")) + }).WithTimeout(60 * time.Second).WithPolling(3 * time.Second).Should(Succeed()) + }) + }) + }) + }) }) }) From 4e0d071c73cf89de1e934bac8fbd9773c53c7740 Mon Sep 17 00:00:00 2001 From: Armel Soro Date: Fri, 10 Feb 2023 15:56:57 +0100 Subject: [PATCH 13/15] Cover more debug-related test cases on Podman --- tests/integration/cmd_dev_debug_test.go | 312 ++++++++++++------------ 1 file changed, 160 insertions(+), 152 deletions(-) diff --git a/tests/integration/cmd_dev_debug_test.go b/tests/integration/cmd_dev_debug_test.go index 449ce4feec8..f6f548339a8 100644 --- a/tests/integration/cmd_dev_debug_test.go +++ b/tests/integration/cmd_dev_debug_test.go @@ -50,8 +50,7 @@ var _ = Describe("odo dev debug command tests", func() { RunOnPodman: podman, } if podman { - // TODO(rm3l): use forward-localhost when it is implemented - opts.CmdlineArgs = append(opts.CmdlineArgs, "--ignore-localhost") + opts.CmdlineArgs = append(opts.CmdlineArgs, "--forward-localhost") } devSession, _, _, ports, err = helper.StartDevMode(opts) Expect(err).ToNot(HaveOccurred()) @@ -66,10 +65,6 @@ var _ = Describe("odo dev debug command tests", func() { By("connecting to the application port", func() { helper.HttpWaitForWithStatus("http://"+ports["3000"], "Hello from Node.js Starter Application!", 12, 5, 200) }) - if podman { - //TODO(rm3l): Remove this once https://github.com/redhat-developer/odo/issues/6510 is fixed and --forward-localhost is implemented - Skip("temporarily skipped on Podman because of https://github.com/redhat-developer/odo/issues/6510") - } By("expecting a ws connection when tried to connect on default debug port locally", func() { // 400 response expected because the endpoint expects a websocket request and we are doing a HTTP GET // We are just using this to validate if nodejs agent is listening on the other side @@ -146,81 +141,81 @@ var _ = Describe("odo dev debug command tests", func() { }, } { devfileHandlerCtx := devfileHandlerCtx - When("a composite command is used as debug command - "+devfileHandlerCtx.name, func() { - var devfileCmpName string - var session helper.DevSession - var stdout []byte - var stderr []byte - var ports map[string]string - BeforeEach(func() { - devfileCmpName = helper.RandString(6) - helper.CopyExampleDevFile( - filepath.Join("source", "devfiles", "nodejs", "devfileCompositeRunAndDebug.yaml"), - filepath.Join(commonVar.Context, "devfile.yaml"), - helper.DevfileMetadataNameSetter(devfileCmpName)) - helper.CopyExample(filepath.Join("source", "devfiles", "nodejs", "project"), commonVar.Context) - if devfileHandlerCtx.sourceHandler != nil { - devfileHandlerCtx.sourceHandler(commonVar.Context, devfileCmpName) - } - var err error - session, stdout, stderr, ports, err = helper.StartDevMode(helper.DevSessionOpts{ - CmdlineArgs: []string{"--debug"}, + for _, podman := range []bool{false, true} { + podman := podman + When("a composite command is used as debug command - "+devfileHandlerCtx.name, helper.LabelPodmanIf(podman, func() { + var devfileCmpName string + var session helper.DevSession + var stdout []byte + var stderr []byte + var ports map[string]string + BeforeEach(func() { + devfileCmpName = helper.RandString(6) + helper.CopyExampleDevFile( + filepath.Join("source", "devfiles", "nodejs", "devfileCompositeRunAndDebug.yaml"), + filepath.Join(commonVar.Context, "devfile.yaml"), + helper.DevfileMetadataNameSetter(devfileCmpName)) + helper.CopyExample(filepath.Join("source", "devfiles", "nodejs", "project"), commonVar.Context) + if devfileHandlerCtx.sourceHandler != nil { + devfileHandlerCtx.sourceHandler(commonVar.Context, devfileCmpName) + } + var err error + opts := helper.DevSessionOpts{ + RunOnPodman: podman, + CmdlineArgs: []string{"--debug"}, + } + if podman { + opts.CmdlineArgs = append(opts.CmdlineArgs, "--forward-localhost") + } + session, stdout, stderr, ports, err = helper.StartDevMode(opts) + Expect(err).ToNot(HaveOccurred()) }) - Expect(err).ToNot(HaveOccurred()) - }) - AfterEach(func() { - session.Stop() - session.WaitEnd() - }) + AfterEach(func() { + session.Stop() + session.WaitEnd() + }) - It("should run successfully", func() { - By("verifying from the output that all commands have been executed", func() { - helper.MatchAllInOutput(string(stdout), []string{ - "Building your application in container", - "Executing the application (command: mkdir)", - "Executing the application (command: echo)", - "Executing the application (command: install)", - "Executing the application (command: start-debug)", + It("should run successfully", func() { + By("verifying from the output that all commands have been executed", func() { + helper.MatchAllInOutput(string(stdout), []string{ + "Building your application in container", + "Executing the application (command: mkdir)", + "Executing the application (command: echo)", + "Executing the application (command: install)", + "Executing the application (command: start-debug)", + }) }) - }) - By("verifying that any command that did not succeed in the middle has logged such information correctly", func() { - helper.MatchAllInOutput(string(stderr), []string{ - "Devfile command \"echo\" exited with an error status", - "intentional-error-message", + By("verifying that any command that did not succeed in the middle has logged such information correctly", func() { + helper.MatchAllInOutput(string(stderr), []string{ + "Devfile command \"echo\" exited with an error status", + "intentional-error-message", + }) }) - }) - By("building the application only once", func() { - // Because of the Spinner, the "Building your application in container" is printed twice in the captured stdout. - // The bracket allows to match the last occurrence with the command execution timing information. - Expect(strings.Count(string(stdout), "Building your application in container (command: install) [")). - To(BeNumerically("==", 1), "\nOUTPUT: "+string(stdout)+"\n") - }) + By("building the application only once", func() { + // Because of the Spinner, the "Building your application in container" is printed twice in the captured stdout. + // The bracket allows to match the last occurrence with the command execution timing information. + Expect(strings.Count(string(stdout), "Building your application in container (command: install) [")). + To(BeNumerically("==", 1), "\nOUTPUT: "+string(stdout)+"\n") + }) - By("verifying that the command did run successfully", func() { - // Verify the command executed successfully - podName := commonVar.CliRunner.GetRunningPodNameByComponent(devfileCmpName, commonVar.Project) - res := commonVar.CliRunner.CheckCmdOpInRemoteDevfilePod( - podName, - "runtime", - commonVar.Project, - []string{"stat", "/projects/testfolder"}, - func(cmdOp string, err error) bool { - return err == nil - }, - ) - Expect(res).To(BeTrue()) - }) + By("verifying that the command did run successfully", func() { + // Verify the command executed successfully + cmp := helper.NewComponent(devfileCmpName, "app", labels.ComponentDevMode, commonVar.Project, commonVar.CliRunner) + out, _ := cmp.Exec("runtime", []string{"stat", "/projects/testfolder"}, pointer.Bool(true)) + Expect(out).To(ContainSubstring("/projects/testfolder")) + }) - By("expecting a ws connection when tried to connect on default debug port locally", func() { - // 400 response expected because the endpoint expects a websocket request and we are doing a HTTP GET - // We are just using this to validate if nodejs agent is listening on the other side - helper.HttpWaitForWithStatus("http://"+ports["5858"], "WebSockets request was expected", 12, 5, 400) + By("expecting a ws connection when tried to connect on default debug port locally", func() { + // 400 response expected because the endpoint expects a websocket request and we are doing a HTTP GET + // We are just using this to validate if nodejs agent is listening on the other side + helper.HttpWaitForWithStatus("http://"+ports["5858"], "WebSockets request was expected", 12, 5, 400) + }) }) - }) - }) + })) + } } When("a composite apply command is used as debug command", func() { @@ -308,98 +303,111 @@ var _ = Describe("odo dev debug command tests", func() { }, } { devfileHandlerCtx := devfileHandlerCtx - When("running build and debug commands as composite in different containers and a shared volume - "+devfileHandlerCtx.name, func() { - var devfileCmpName string - var session helper.DevSession - var stdout []byte - var stderr []byte - var ports map[string]string - BeforeEach(func() { - devfileCmpName = helper.RandString(6) - helper.CopyExampleDevFile( - filepath.Join("source", "devfiles", "nodejs", "devfileCompositeBuildRunDebugInMultiContainersAndSharedVolume.yaml"), - filepath.Join(commonVar.Context, "devfile.yaml"), - helper.DevfileMetadataNameSetter(devfileCmpName)) - helper.CopyExample(filepath.Join("source", "devfiles", "nodejs", "project"), commonVar.Context) - if devfileHandlerCtx.sourceHandler != nil { - devfileHandlerCtx.sourceHandler(commonVar.Context, devfileCmpName) - } - var err error - session, stdout, stderr, ports, err = helper.StartDevMode(helper.DevSessionOpts{ - CmdlineArgs: []string{"--debug"}, + for _, podman := range []bool{false, true} { + podman := podman + When("running build and debug commands as composite in different containers and a shared volume - "+devfileHandlerCtx.name, helper.LabelPodmanIf(podman, func() { + var devfileCmpName string + var session helper.DevSession + var stdout []byte + var stderr []byte + var ports map[string]string + BeforeEach(func() { + //TODO(rm3l): For some reason, this does not work on Podman + if podman { + Skip("Does not work on Podman due to permission issues related in the volume mount path: /bin/sh: /artifacts/build-result: Permission denied") + } + devfileCmpName = helper.RandString(6) + helper.CopyExampleDevFile( + filepath.Join("source", "devfiles", "nodejs", "devfileCompositeBuildRunDebugInMultiContainersAndSharedVolume.yaml"), + filepath.Join(commonVar.Context, "devfile.yaml"), + helper.DevfileMetadataNameSetter(devfileCmpName)) + helper.CopyExample(filepath.Join("source", "devfiles", "nodejs", "project"), commonVar.Context) + if devfileHandlerCtx.sourceHandler != nil { + devfileHandlerCtx.sourceHandler(commonVar.Context, devfileCmpName) + } + var err error + opts := helper.DevSessionOpts{ + RunOnPodman: podman, + CmdlineArgs: []string{"--debug"}, + } + if podman { + opts.CmdlineArgs = append(opts.CmdlineArgs, "--forward-localhost") + } + session, stdout, stderr, ports, err = helper.StartDevMode(opts) + Expect(err).ToNot(HaveOccurred()) }) - Expect(err).ToNot(HaveOccurred()) - }) - AfterEach(func() { - session.Stop() - session.WaitEnd() - }) + AfterEach(func() { + session.Stop() + session.WaitEnd() + }) - It("should run successfully", func() { - By("verifying from the output that all commands have been executed", func() { - helper.MatchAllInOutput(string(stdout), []string{ - "Building your application in container (command: mkdir)", - "Building your application in container (command: sleep-cmd-build)", - "Building your application in container (command: build-cmd)", - "Executing the application (command: sleep-cmd-run)", - "Executing the application (command: echo-with-error)", - "Executing the application (command: check-build-result)", - "Executing the application (command: start-debug)", + It("should run successfully", func() { + By("verifying from the output that all commands have been executed", func() { + helper.MatchAllInOutput(string(stdout), []string{ + "Building your application in container (command: mkdir)", + "Building your application in container (command: sleep-cmd-build)", + "Building your application in container (command: build-cmd)", + "Executing the application (command: sleep-cmd-run)", + "Executing the application (command: echo-with-error)", + "Executing the application (command: check-build-result)", + "Executing the application (command: start-debug)", + }) }) - }) - By("verifying that any command that did not succeed in the middle has logged such information correctly", func() { - helper.MatchAllInOutput(string(stderr), []string{ - "Devfile command \"echo-with-error\" exited with an error status", - "intentional-error-message", + By("verifying that any command that did not succeed in the middle has logged such information correctly", func() { + helper.MatchAllInOutput(string(stderr), []string{ + "Devfile command \"echo-with-error\" exited with an error status", + "intentional-error-message", + }) }) - }) - By("building the application only once per exec command in the build command", func() { - // Because of the Spinner, the "Building your application in container" is printed twice in the captured stdout. - // The bracket allows to match the last occurrence with the command execution timing information. - out := string(stdout) - for _, cmd := range []string{"mkdir", "sleep-cmd-build", "build-cmd"} { - Expect(strings.Count(out, fmt.Sprintf("Building your application in container (command: %s) [", cmd))). - To(BeNumerically("==", 1), "\nOUTPUT: "+string(stdout)+"\n") - } - }) + By("building the application only once per exec command in the build command", func() { + // Because of the Spinner, the "Building your application in container" is printed twice in the captured stdout. + // The bracket allows to match the last occurrence with the command execution timing information. + out := string(stdout) + for _, cmd := range []string{"mkdir", "sleep-cmd-build", "build-cmd"} { + Expect(strings.Count(out, fmt.Sprintf("Building your application in container (command: %s) [", cmd))). + To(BeNumerically("==", 1), "\nOUTPUT: "+string(stdout)+"\n") + } + }) - By("verifying that the command did run successfully", func() { - // Verify the command executed successfully - podName := commonVar.CliRunner.GetRunningPodNameByComponent(devfileCmpName, commonVar.Project) - res := commonVar.CliRunner.CheckCmdOpInRemoteDevfilePod( - podName, - "runtime", - commonVar.Project, - []string{"stat", "/projects/testfolder"}, - func(cmdOp string, err error) bool { - return err == nil - }, - ) - Expect(res).To(BeTrue()) - }) + By("verifying that the command did run successfully", func() { + // Verify the command executed successfully + cmp := helper.NewComponent(devfileCmpName, "app", labels.ComponentDevMode, commonVar.Project, commonVar.CliRunner) + out, _ := cmp.Exec("runtime", []string{"stat", "/projects/testfolder"}, pointer.Bool(true)) + Expect(out).To(ContainSubstring("/projects/testfolder")) + }) - By("expecting a ws connection when tried to connect on default debug port locally", func() { - // 400 response expected because the endpoint expects a websocket request and we are doing a HTTP GET - // We are just using this to validate if nodejs agent is listening on the other side - helper.HttpWaitForWithStatus("http://"+ports["5858"], "WebSockets request was expected", 12, 5, 400) + By("expecting a ws connection when tried to connect on default debug port locally", func() { + // 400 response expected because the endpoint expects a websocket request and we are doing a HTTP GET + // We are just using this to validate if nodejs agent is listening on the other side + helper.HttpWaitForWithStatus("http://"+ports["5858"], "WebSockets request was expected", 12, 5, 400) + }) }) - }) - }) + })) + } } - When("a component without debug command is bootstrapped", func() { - BeforeEach(func() { - helper.CopyExample(filepath.Join("source", "devfiles", "nodejs", "project"), commonVar.Context) - helper.Cmd("odo", "init", "--name", cmpName, "--devfile-path", helper.GetExamplePath("source", "devfiles", "nodejs", "devfile-without-debugrun.yaml")).ShouldPass() - Expect(helper.VerifyFileExists(".odo/env/env.yaml")).To(BeFalse()) - }) + for _, podman := range []bool{false, true} { + podman := podman + When("a component without debug command is bootstrapped", helper.LabelPodmanIf(podman, func() { + BeforeEach(func() { + helper.CopyExample(filepath.Join("source", "devfiles", "nodejs", "project"), commonVar.Context) + helper.Cmd("odo", "init", "--name", cmpName, "--devfile-path", helper.GetExamplePath("source", "devfiles", "nodejs", "devfile-without-debugrun.yaml")).ShouldPass() + Expect(helper.VerifyFileExists(".odo/env/env.yaml")).To(BeFalse()) + }) - It("should fail running odo dev --debug", func() { - output := helper.Cmd("odo", "dev", "--debug").ShouldFail().Err() - Expect(output).To(ContainSubstring("no command of kind \"debug\" found in the devfile")) - }) - }) + It("should fail running odo dev --debug", func() { + args := []string{"dev", "--debug"} + var env []string + if podman { + args = append(args, "--platform", "podman") + env = append(env, "ODO_EXPERIMENTAL_MODE=true") + } + output := helper.Cmd("odo", args...).AddEnv(env...).ShouldFail().Err() + Expect(output).To(ContainSubstring("no command of kind \"debug\" found in the devfile")) + }) + })) + } }) From ef3238a70d78b9e057693d359931a1b486e586f9 Mon Sep 17 00:00:00 2001 From: Armel Soro Date: Thu, 2 Mar 2023 10:00:46 +0100 Subject: [PATCH 14/15] Expose the endpoint protocol so as to instruct socat to listen and forward the right protocol --- pkg/api/component.go | 1 + pkg/dev/podmandev/pod.go | 1 + pkg/portForward/kubeportforward/writer.go | 1 + pkg/portForward/podmanportforward/portForward.go | 11 +++++++++-- 4 files changed, 12 insertions(+), 2 deletions(-) diff --git a/pkg/api/component.go b/pkg/api/component.go index 2266626067c..c97ae07b34c 100644 --- a/pkg/api/component.go +++ b/pkg/api/component.go @@ -26,6 +26,7 @@ type ForwardedPort struct { LocalPort int `json:"localPort"` ContainerPort int `json:"containerPort"` Exposure string `json:"exposure,omitempty"` + Protocol string `json:"protocol,omitempty"` } type ConnectionData struct { diff --git a/pkg/dev/podmandev/pod.go b/pkg/dev/podmandev/pod.go index 7a981ae1250..5b0e45e3ee9 100644 --- a/pkg/dev/podmandev/pod.go +++ b/pkg/dev/podmandev/pod.go @@ -252,6 +252,7 @@ func getPortMapping(devfileObj parser.DevfileObj, debug bool, randomPorts bool, LocalPort: freePort, ContainerPort: ep.TargetPort, Exposure: string(ep.Exposure), + Protocol: string(ep.Protocol), } result = append(result, fp) } diff --git a/pkg/portForward/kubeportforward/writer.go b/pkg/portForward/kubeportforward/writer.go index c4b3d847a06..938a658cb1d 100644 --- a/pkg/portForward/kubeportforward/writer.go +++ b/pkg/portForward/kubeportforward/writer.go @@ -95,6 +95,7 @@ containerLoop: fp.PortName = ep.Name fp.Exposure = string(ep.Exposure) fp.IsDebug = libdevfile.IsDebugPort(ep.Name) + fp.Protocol = string(ep.Protocol) break containerLoop } } diff --git a/pkg/portForward/podmanportforward/portForward.go b/pkg/portForward/podmanportforward/portForward.go index dbb1af6ea6f..0dbcf31c140 100644 --- a/pkg/portForward/podmanportforward/portForward.go +++ b/pkg/portForward/podmanportforward/portForward.go @@ -9,6 +9,7 @@ import ( "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" "github.com/devfile/library/v2/pkg/devfile/parser" + corev1 "k8s.io/api/core/v1" "k8s.io/klog" "github.com/redhat-developer/odo/pkg/api" @@ -120,11 +121,17 @@ func getPodName(componentName string) string { } func getCommandDefinition(port api.ForwardedPort) remotecmd.CommandDefinition { + proto := "tcp" + switch { + case strings.EqualFold(port.Protocol, string(corev1.ProtocolUDP)): + proto = "udp" + case strings.EqualFold(port.Protocol, string(corev1.ProtocolSCTP)): + proto = "sctp" + } return remotecmd.CommandDefinition{ Id: fmt.Sprintf("pf-%s", port.PortName), // PidDirectory needs to be writable PidDirectory: "/projects/", - //TODO(rm3l) Use the right L4 protocol: tcp or udp? - CmdLine: fmt.Sprintf("socat -d tcp-listen:%d,reuseaddr,fork tcp:localhost:%d", port.LocalPort, port.ContainerPort), + CmdLine: fmt.Sprintf("socat -d %[1]s-listen:%[2]d,reuseaddr,fork %[1]s:localhost:%[3]d", proto, port.LocalPort, port.ContainerPort), } } From 7b384d3aa85641ed81a15615ceea713df126ea04 Mon Sep 17 00:00:00 2001 From: Armel Soro Date: Fri, 3 Mar 2023 17:50:19 +0100 Subject: [PATCH 15/15] Fix sub-deps of EXEC and PORT_FORWARD, as suggested in review --- pkg/odo/genericclioptions/clientset/clientset.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/odo/genericclioptions/clientset/clientset.go b/pkg/odo/genericclioptions/clientset/clientset.go index 3093d327b54..10e5725d49f 100644 --- a/pkg/odo/genericclioptions/clientset/clientset.go +++ b/pkg/odo/genericclioptions/clientset/clientset.go @@ -92,10 +92,10 @@ var subdeps map[string][]string = map[string][]string{ DELETE_COMPONENT: {KUBERNETES_NULLABLE, PODMAN_NULLABLE, EXEC}, DEPLOY: {KUBERNETES, FILESYSTEM}, DEV: {BINDING, DELETE_COMPONENT, EXEC, FILESYSTEM, KUBERNETES_NULLABLE, PODMAN_NULLABLE, PORT_FORWARD, PREFERENCE, STATE, SYNC, WATCH}, - EXEC: {KUBERNETES_NULLABLE}, + EXEC: {KUBERNETES_NULLABLE, PODMAN_NULLABLE}, INIT: {ALIZER, FILESYSTEM, PREFERENCE, REGISTRY}, LOGS: {KUBERNETES_NULLABLE, PODMAN_NULLABLE}, - PORT_FORWARD: {KUBERNETES_NULLABLE, STATE}, + PORT_FORWARD: {KUBERNETES_NULLABLE, EXEC, STATE}, PROJECT: {KUBERNETES}, REGISTRY: {FILESYSTEM, PREFERENCE}, STATE: {FILESYSTEM},