Repository: vmware-tanzu/octant
Pull request #1796 (2021-01-07 16:56:21)

Fixed issues with incorrect urls on Windows
In some cases when doing url string manipulations, we were using `filepath` instead of `path` go package.  Using OS specific `filepath` package on Windows resulted in incorrect urls that contain backslashes, as noted in #1696.

The original problem from #1696 is not present any more because that code is eliminated with latest breadcrumbs improvements. However, I found another occurrence where `filepath` was used when generating navigation entries and fixed it in this PR.

Big thanks to @lgellrich for correctly identifying the problem.

Signed-off-by: Milan Klanjsek <mklanjsek@pivotal.io>

**Which issue(s) this PR fixes**
- Fixes #1996



Patch:
diff --git a/changelogs/unreleased/1696-mklanjsek b/changelogs/unreleased/1696-mklanjsek
new file mode 100644
index 00000000..e0b0e1d1
--- /dev/null
+++ b/changelogs/unreleased/1696-mklanjsek
@@ -0,0 +1,1 @@
+Fixed issues with incorrect paths on Windows
diff --git a/internal/api/breadcrumb.go b/internal/api/breadcrumb.go
index 31fd7090..3efcb003 100644
--- a/internal/api/breadcrumb.go
+++ b/internal/api/breadcrumb.go
@@ -14,6 +14,7 @@ import (
 
 	"github.com/vmware-tanzu/octant/internal/module"
 	"github.com/vmware-tanzu/octant/internal/octant"
+	"github.com/vmware-tanzu/octant/internal/util/path_util"
 	"github.com/vmware-tanzu/octant/pkg/store"
 	"github.com/vmware-tanzu/octant/pkg/view/component"
 
@@ -44,7 +45,7 @@ func GenerateBreadcrumb(cm *ContentManager, contentPath string, state octant.Sta
 		if path.Base(contentPath) == parent.Title || path.Base(parent.Url) == crPath {
 			title = append(title, component.NewText(parent.Title))
 		} else {
-			title = append(title, component.NewLink("", parent.Title, parent.Url), component.NewText(path.Base(contentPath)))
+			title = append(title, component.NewLink("", parent.Title, path_util.PrefixedPath(parent.Url)), component.NewText(path.Base(contentPath)))
 		}
 	} else {
 		gvk, err := cm.moduleManager.GvkFromPath(path.Dir(contentPath), state.GetNamespace())
diff --git a/internal/api/websocket_state.go b/internal/api/websocket_state.go
index 7db8037f..4e662824 100644
--- a/internal/api/websocket_state.go
+++ b/internal/api/websocket_state.go
@@ -14,6 +14,8 @@ import (
 	"strings"
 	"sync"
 
+	"github.com/vmware-tanzu/octant/internal/util/path_util"
+
 	"github.com/vmware-tanzu/octant/pkg/event"
 
 	"github.com/google/uuid"
@@ -188,6 +190,7 @@ func (c *WebsocketState) Dispatch(ctx context.Context, actionName string, payloa
 // SetContentPath sets the content path.
 func (c *WebsocketState) SetContentPath(contentPath string) {
 	if contentPath == "" {
+		contentPath = path_util.NamespacedPath("overview", c.namespace.get())
 		contentPath = path.Join("overview", "namespace", c.namespace.get())
 	} else if c.contentPath.get() == contentPath {
 		return
diff --git a/internal/modules/applications/application.go b/internal/modules/applications/application.go
index e504e09c..fbeccb53 100644
--- a/internal/modules/applications/application.go
+++ b/internal/modules/applications/application.go
@@ -8,9 +8,10 @@ package applications
 import (
 	"context"
 	"fmt"
-	"path"
 	"sort"
 
+	"github.com/vmware-tanzu/octant/internal/util/path_util"
+
 	"github.com/pkg/errors"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 
@@ -71,7 +72,7 @@ func (a *application) Title() string {
 }
 
 func (a *application) Path(prefix, namespace string) string {
-	return path.Join("/", prefix, "namespace", namespace, a.Name, a.Instance, a.Version)
+	return path_util.NamespacedPath(path_util.PrefixedPath(prefix), namespace, a.Name, a.Instance, a.Version)
 }
 
 func listApplications(ctx context.Context, objectStore store.Store, namespace string) ([]application, error) {
diff --git a/internal/modules/applications/module.go b/internal/modules/applications/module.go
index 3d5c3823..184568fa 100644
--- a/internal/modules/applications/module.go
+++ b/internal/modules/applications/module.go
@@ -8,7 +8,6 @@ package applications
 import (
 	"context"
 	"path"
-	"path/filepath"
 
 	"github.com/pkg/errors"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
@@ -19,6 +18,7 @@ import (
 	"github.com/vmware-tanzu/octant/internal/generator"
 	"github.com/vmware-tanzu/octant/internal/module"
 	"github.com/vmware-tanzu/octant/internal/octant"
+	"github.com/vmware-tanzu/octant/internal/util/path_util"
 	"github.com/vmware-tanzu/octant/pkg/navigation"
 	"github.com/vmware-tanzu/octant/pkg/view/component"
 )
@@ -86,7 +86,7 @@ func (m *Module) ContentPath() string {
 
 // Navigation generates navigation entries for the module.
 func (m *Module) Navigation(ctx context.Context, namespace, root string) ([]navigation.Navigation, error) {
-	rootPath := filepath.Join(m.ContentPath(), "namespace", namespace)
+	rootPath := path_util.NamespacedPath(m.ContentPath(), namespace)
 
 	applications, err := listApplications(ctx, m.DashConfig.ObjectStore(), namespace)
 	if err != nil {
diff --git a/internal/modules/overview/path.go b/internal/modules/overview/path.go
index 4b37aaec..1727c4f4 100644
--- a/internal/modules/overview/path.go
+++ b/internal/modules/overview/path.go
@@ -9,6 +9,8 @@ import (
 	"path"
 	"strings"
 
+	"github.com/vmware-tanzu/octant/internal/util/path_util"
+
 	"github.com/pkg/errors"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 
@@ -45,7 +47,7 @@ func crdPath(namespace, crdName, version, name string) (string, error) {
 		return "", errors.Errorf("unable to create CRD path for %s due to missing namespace", crdName)
 	}
 
-	return path.Join("/overview/namespace", namespace, "custom-resources", crdName, version, name), nil
+	return path_util.NamespacedPath("/overview", namespace, "custom-resources", crdName, version, name), nil
 }
 
 func gvkPath(namespace, apiVersion, kind, name string) (string, error) {
diff --git a/internal/modules/workloads/module.go b/internal/modules/workloads/module.go
index 417bb3a2..b9543f32 100644
--- a/internal/modules/workloads/module.go
+++ b/internal/modules/workloads/module.go
@@ -8,7 +8,8 @@ package workloads
 import (
 	"context"
 	"fmt"
-	"path"
+
+	"github.com/vmware-tanzu/octant/internal/util/path_util"
 
 	"github.com/pkg/errors"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
@@ -99,7 +100,7 @@ func (m *Module) ContentPath() string {
 
 // Navigation returns navigation entries for the module.
 func (m *Module) Navigation(ctx context.Context, namespace, root string) ([]navigation.Navigation, error) {
-	rootPath := path.Join(m.ContentPath(), "namespace", namespace)
+	rootPath := path_util.NamespacedPath(m.ContentPath(), namespace)
 
 	rootNav := navigation.Navigation{
 		Title:    "Applications",
diff --git a/internal/octant/factory.go b/internal/octant/factory.go
index fccdcd4c..c424cdc5 100644
--- a/internal/octant/factory.go
+++ b/internal/octant/factory.go
@@ -11,6 +11,8 @@ import (
 	"strings"
 	"sync"
 
+	"github.com/vmware-tanzu/octant/internal/util/path_util"
+
 	"github.com/pkg/errors"
 	"golang.org/x/sync/errgroup"
 
@@ -41,7 +43,7 @@ type NavigationFactory struct {
 func NewNavigationFactory(namespace string, root string, objectStore store.Store, entries NavigationEntries) *NavigationFactory {
 	var rootPath = root
 	if namespace != "" {
-		rootPath = path.Join(root, "namespace", namespace, "")
+		rootPath = path_util.NamespacedPath(root, namespace, "")
 	}
 	if !strings.HasSuffix(rootPath, "/") {
 		rootPath = rootPath + "/"
diff --git a/internal/octant/workload.go b/internal/octant/workload.go
index 2a019a0d..0e9d2c02 100644
--- a/internal/octant/workload.go
+++ b/internal/octant/workload.go
@@ -8,10 +8,11 @@ package octant
 import (
 	"context"
 	"fmt"
-	"path"
 	"sort"
 	"sync"
 
+	"github.com/vmware-tanzu/octant/internal/util/path_util"
+
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/resource"
@@ -301,7 +302,7 @@ func CreateCard(workload *Workload, namespace string) (*component.Card, bool, er
 		supportsMetrics = false
 	}
 
-	cardPath := path.Join("/workloads/namespace", namespace, "detail", workload.Name)
+	cardPath := path_util.NamespacedPath("/workloads", namespace, "detail", workload.Name)
 	cardTitle := component.NewLink("", workload.Name, cardPath)
 
 	card := component.NewCard([]component.TitleComponent{cardTitle})
diff --git a/internal/printer/path.go b/internal/printer/path.go
index fa63832b..378689f4 100644
--- a/internal/printer/path.go
+++ b/internal/printer/path.go
@@ -8,6 +8,8 @@ package printer
 import (
 	"path"
 
+	"github.com/vmware-tanzu/octant/internal/util/path_util"
+
 	corev1 "k8s.io/api/core/v1"
 )
 
@@ -53,7 +55,7 @@ func ObjectReferencePath(or corev1.ObjectReference) (string, error) {
 
 	var objectPath string
 	if or.Namespace != "" {
-		objectPath = path.Join("/overview/namespace", or.Namespace, section, or.Name)
+		objectPath = path_util.NamespacedPath("/overview", or.Namespace, section, or.Name)
 	} else {
 		objectPath = path.Join("/overview", section, or.Name)
 	}
diff --git a/internal/util/path_util/path.go b/internal/util/path_util/path.go
new file mode 100644
index 00000000..c3a90f2a
--- /dev/null
+++ b/internal/util/path_util/path.go
@@ -0,0 +1,27 @@
+/*
+Copyright (c) 2021 the Octant contributors. All Rights Reserved.
+SPDX-License-Identifier: Apache-2.0
+*/
+
+package path_util
+
+import (
+	"path"
+	"strings"
+)
+
+// NamespacedPath generates the URL for namespaced path
+// by joining base url, namespace and additional path segments.
+//
+func NamespacedPath(base, namespace string, paths ...string) string {
+	return path.Join(append([]string{base, "namespace", namespace}, paths...)...)
+}
+
+// PrefixedPath ensures that provided url starts with slash ("/")
+//
+func PrefixedPath(url string) string {
+	if !strings.HasPrefix(url, "/") {
+		url = path.Join("/", url)
+	}
+	return url
+}
diff --git a/pkg/dash/dash.go b/pkg/dash/dash.go
index 381f406a..1ec7b44c 100644
--- a/pkg/dash/dash.go
+++ b/pkg/dash/dash.go
@@ -16,6 +16,8 @@ import (
 	"strings"
 	"time"
 
+	"github.com/vmware-tanzu/octant/internal/util/path_util"
+
 	"contrib.go.opencensus.io/exporter/jaeger"
 	"github.com/skratchdot/open-golang/open"
 	"github.com/soheilhy/cmux"
@@ -508,10 +510,7 @@ func (d *dash) Run(ctx context.Context, startupCh chan bool) error {
 	if d.willOpenBrowser {
 		runURL := dashboardURL
 		if d.browserPath != "" {
-			if !strings.HasPrefix(d.browserPath, "/") {
-				d.browserPath = "/" + d.browserPath
-			}
-			runURL += d.browserPath
+			runURL += path_util.PrefixedPath(d.browserPath)
 		}
 		if err = open.Run(runURL); err != nil {
 			d.logger.Warnf("unable to open browser: %v", err)


Test patch:
diff --git a/internal/resourceviewer/handler_test.go b/internal/resourceviewer/handler_test.go
index 55c6b60b..2a92778c 100644
--- a/internal/resourceviewer/handler_test.go
+++ b/internal/resourceviewer/handler_test.go
@@ -3,9 +3,10 @@ package resourceviewer
 import (
 	"context"
 	"fmt"
-	"path"
 	"testing"
 
+	"github.com/vmware-tanzu/octant/internal/util/path_util"
+
 	"github.com/golang/mock/gomock"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -159,7 +160,7 @@ func TestHandler(t *testing.T) {
 		accessor, err := meta.Accessor(object)
 		require.NoError(t, err)
 		name := accessor.GetName()
-		return component.NewLink("", name, path.Join("/", name))
+		return component.NewLink("", name, path_util.PrefixedPath(name))
 	}
 
 	podStatus1 := component.NewPodStatus()
@@ -344,7 +345,7 @@ func mockLinkPath(t *testing.T, dashConfig *configFake.MockDash, object runtime.
 
 	apiVersion, kind := object.GetObjectKind().GroupVersionKind().ToAPIVersionAndKind()
 
-	label := path.Join("/", accessor.GetName())
+	label := path_util.PrefixedPath(accessor.GetName())
 
 	dashConfig.EXPECT().
 		ObjectPath(accessor.GetNamespace(), apiVersion, kind, accessor.GetName()).
diff --git a/internal/util/path_util/path_test.go b/internal/util/path_util/path_test.go
new file mode 100644
index 00000000..b9542e2d
--- /dev/null
+++ b/internal/util/path_util/path_test.go
@@ -0,0 +1,91 @@
+/*
+Copyright (c) 2021 the Octant contributors. All Rights Reserved.
+SPDX-License-Identifier: Apache-2.0
+*/
+
+package path_util
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func Test_NamespacedPath(t *testing.T) {
+	cases := []struct {
+		name      string
+		base      string
+		namespace string
+		args      []string
+		expected  string
+	}{
+		{
+			name:      "base path, no args",
+			base:      "overview",
+			namespace: "default",
+			args:      []string{},
+			expected:  "overview/namespace/default",
+		},
+		{
+			name:      "base path, single arg",
+			base:      "overview",
+			namespace: "default",
+			args:      []string{"arg"},
+			expected:  "overview/namespace/default/arg",
+		},
+		{
+			name:      "base path, two args",
+			base:      "overview",
+			namespace: "default",
+			args:      []string{"arg1", "arg2"},
+			expected:  "overview/namespace/default/arg1/arg2",
+		},
+		{
+			name:      "no base path, two args",
+			base:      "",
+			namespace: "default",
+			args:      []string{"arg1", "arg2"},
+			expected:  "namespace/default/arg1/arg2",
+		},
+	}
+
+	for i := range cases {
+		tc := cases[i]
+		t.Run(tc.name, func(t *testing.T) {
+			got := NamespacedPath(tc.base, tc.namespace, tc.args...)
+			assert.Equal(t, tc.expected, got)
+		})
+	}
+}
+
+func Test_PrefixedPath(t *testing.T) {
+	cases := []struct {
+		name     string
+		path     string
+		expected string
+	}{
+		{
+			name:     "base path no slash",
+			path:     "path",
+			expected: "/path",
+		},
+		{
+			name:     "base path with slash",
+			path:     "/path",
+			expected: "/path",
+		},
+		{
+			name:     "empty string",
+			path:     "",
+			expected: "/",
+		},
+	}
+
+	for i := range cases {
+		tc := cases[i]
+		t.Run(tc.name, func(t *testing.T) {
+			got := PrefixedPath(tc.path)
+			assert.Equal(t, tc.expected, got)
+		})
+	}
+}