From 2c54173a3982928da5cc8ec13df71c82b6887a36 Mon Sep 17 00:00:00 2001 From: Morgan Martinet Date: Tue, 23 Nov 2021 18:01:30 -0500 Subject: [PATCH] apply code review recommandations use encodeURIComponent on dynamic parts ensure that we keep any dashboard path prefix when generating links move URL hash exception for SPAs into the standardFormatter Signed-off-by: Morgan Martinet --- .github/styles/vocab.txt | 2 +- .../clusterLinks/formatClusterLink.test.ts | 15 ------ .../utils/clusterLinks/formatClusterLink.ts | 5 +- .../clusterLinks/formatters/openshift.test.ts | 54 +++++++++++++------ .../clusterLinks/formatters/openshift.ts | 26 ++++++--- .../utils/clusterLinks/formatters/rancher.ts | 26 ++++++--- .../clusterLinks/formatters/standard.test.ts | 45 ++++++++++++++++ .../utils/clusterLinks/formatters/standard.ts | 14 +++-- 8 files changed, 130 insertions(+), 57 deletions(-) diff --git a/.github/styles/vocab.txt b/.github/styles/vocab.txt index 217342b448..4ec62923d8 100644 --- a/.github/styles/vocab.txt +++ b/.github/styles/vocab.txt @@ -189,7 +189,7 @@ oidc Okta onboarding Onboarding -Openshift +OpenShift orgs pagerduty pageview diff --git a/plugins/kubernetes/src/utils/clusterLinks/formatClusterLink.test.ts b/plugins/kubernetes/src/utils/clusterLinks/formatClusterLink.test.ts index afc53d9509..31e07a0f14 100644 --- a/plugins/kubernetes/src/utils/clusterLinks/formatClusterLink.test.ts +++ b/plugins/kubernetes/src/utils/clusterLinks/formatClusterLink.test.ts @@ -78,21 +78,6 @@ describe('clusterLinks', () => { 'https://k8s.foo.com/#/service/bar/foobar?namespace=bar', ); }); - it('should return an url on the deployment properly url encoded', () => { - const url = formatClusterLink({ - dashboardUrl: 'https://k8s.foo.com/', - object: { - metadata: { - name: 'foobar', - namespace: 'bar bar', - }, - }, - kind: 'Deployment', - }); - expect(url).toBe( - 'https://k8s.foo.com/#/deployment/bar%20bar/foobar?namespace=bar+bar', - ); - }); }); describe('standard app', () => { diff --git a/plugins/kubernetes/src/utils/clusterLinks/formatClusterLink.ts b/plugins/kubernetes/src/utils/clusterLinks/formatClusterLink.ts index 83e970248b..a8f83a9c76 100644 --- a/plugins/kubernetes/src/utils/clusterLinks/formatClusterLink.ts +++ b/plugins/kubernetes/src/utils/clusterLinks/formatClusterLink.ts @@ -40,8 +40,5 @@ export function formatClusterLink(options: FormatClusterLinkOptions) { object: options.object, kind: options.kind, }); - // Note that we can't rely on 'url.href' since it will put the search before the hash - // and this won't be properly recognized by SPAs such as Angular in the standard dashboard. - // Note also that pathname, hash and search will be properly url encoded. - return `${url.origin}${url.pathname}${url.hash}${url.search}`; + return url.toString(); } diff --git a/plugins/kubernetes/src/utils/clusterLinks/formatters/openshift.test.ts b/plugins/kubernetes/src/utils/clusterLinks/formatters/openshift.test.ts index b6cd60b85c..f0b85cad49 100644 --- a/plugins/kubernetes/src/utils/clusterLinks/formatters/openshift.test.ts +++ b/plugins/kubernetes/src/utils/clusterLinks/formatters/openshift.test.ts @@ -15,11 +15,7 @@ */ import { openshiftFormatter } from './openshift'; -function formatUrl(url: URL) { - return url.toString(); -} - -describe('clusterLinks - Openshift formatter', () => { +describe('clusterLinks - OpenShift formatter', () => { it('should return an url on the workloads when there is a namespace only', () => { const url = openshiftFormatter({ dashboardUrl: new URL('https://k8s.foo.com'), @@ -30,7 +26,7 @@ describe('clusterLinks - Openshift formatter', () => { }, kind: 'foo', }); - expect(formatUrl(url)).toBe('https://k8s.foo.com/k8s/cluster/projects/bar'); + expect(url.href).toBe('https://k8s.foo.com/k8s/cluster/projects/bar'); }); it('should return an url on the workloads when the kind is not recognizeed', () => { const url = openshiftFormatter({ @@ -43,7 +39,7 @@ describe('clusterLinks - Openshift formatter', () => { }, kind: 'UnknownKind', }); - expect(formatUrl(url)).toBe('https://k8s.foo.com/k8s/cluster/projects/bar'); + expect(url.href).toBe('https://k8s.foo.com/k8s/cluster/projects/bar'); }); it('should return an url on the deployment', () => { const url = openshiftFormatter({ @@ -56,8 +52,36 @@ describe('clusterLinks - Openshift formatter', () => { }, kind: 'Deployment', }); - expect(formatUrl(url)).toBe( - 'https://k8s.foo.com/k8s/ns/bar/deployments/foobar', + expect(url.href).toBe('https://k8s.foo.com/k8s/ns/bar/deployments/foobar'); + }); + it('should return an url on the deployment and keep the path prefix 1', () => { + const url = openshiftFormatter({ + dashboardUrl: new URL('https://k8s.foo.com/some/prefix/'), + object: { + metadata: { + name: 'foobar', + namespace: 'bar', + }, + }, + kind: 'Deployment', + }); + expect(url.href).toBe( + 'https://k8s.foo.com/some/prefix/k8s/ns/bar/deployments/foobar', + ); + }); + it('should return an url on the deployment and keep the path prefix 2', () => { + const url = openshiftFormatter({ + dashboardUrl: new URL('https://k8s.foo.com/some/prefix'), + object: { + metadata: { + name: 'foobar', + namespace: 'bar', + }, + }, + kind: 'Deployment', + }); + expect(url.href).toBe( + 'https://k8s.foo.com/some/prefix/k8s/ns/bar/deployments/foobar', ); }); it('should return an url on the service', () => { @@ -71,9 +95,7 @@ describe('clusterLinks - Openshift formatter', () => { }, kind: 'Service', }); - expect(formatUrl(url)).toBe( - 'https://k8s.foo.com/k8s/ns/bar/services/foobar', - ); + expect(url.href).toBe('https://k8s.foo.com/k8s/ns/bar/services/foobar'); }); it('should return an url on the ingress', () => { const url = openshiftFormatter({ @@ -86,9 +108,7 @@ describe('clusterLinks - Openshift formatter', () => { }, kind: 'Ingress', }); - expect(formatUrl(url)).toBe( - 'https://k8s.foo.com/k8s/ns/bar/ingresses/foobar', - ); + expect(url.href).toBe('https://k8s.foo.com/k8s/ns/bar/ingresses/foobar'); }); it('should return an url on the deployment for a hpa', () => { const url = openshiftFormatter({ @@ -101,7 +121,7 @@ describe('clusterLinks - Openshift formatter', () => { }, kind: 'HorizontalPodAutoscaler', }); - expect(formatUrl(url)).toBe( + expect(url.href).toBe( 'https://k8s.foo.com/k8s/ns/bar/horizontalpodautoscalers/foobar', ); }); @@ -115,7 +135,7 @@ describe('clusterLinks - Openshift formatter', () => { }, kind: 'PersistentVolume', }); - expect(formatUrl(url)).toBe( + expect(url.href).toBe( 'https://k8s.foo.com/k8s/cluster/persistentvolumes/foobar', ); }); diff --git a/plugins/kubernetes/src/utils/clusterLinks/formatters/openshift.ts b/plugins/kubernetes/src/utils/clusterLinks/formatters/openshift.ts index 4300212a03..6c20cd4720 100644 --- a/plugins/kubernetes/src/utils/clusterLinks/formatters/openshift.ts +++ b/plugins/kubernetes/src/utils/clusterLinks/formatters/openshift.ts @@ -24,21 +24,31 @@ const kindMappings: Record = { }; export function openshiftFormatter(options: ClusterLinksFormatterOptions): URL { - const result = new URL(options.dashboardUrl.href); - const name = options.object.metadata?.name; - const namespace = options.object.metadata?.namespace; + const basePath = new URL(options.dashboardUrl.href); + const name = encodeURIComponent(options.object.metadata?.name ?? ''); + const namespace = encodeURIComponent( + options.object.metadata?.namespace ?? '', + ); const validKind = kindMappings[options.kind.toLocaleLowerCase('en-US')]; + if (!basePath.pathname.endsWith('/')) { + // a dashboard url with a path should end with a slash otherwise + // the new combined URL will replace the last segment with the appended path! + // https://foobar.com/abc/def + k8s/cluster/projects/test --> https://foobar.com/abc/k8s/cluster/projects/test + // https://foobar.com/abc/def/ + k8s/cluster/projects/test --> https://foobar.com/abc/def/k8s/cluster/projects/test + basePath.pathname += '/'; + } + let path = ''; if (namespace) { if (name && validKind) { - result.pathname = `k8s/ns/${namespace}/${validKind}/${name}`; + path = `k8s/ns/${namespace}/${validKind}/${name}`; } else { - result.pathname = `k8s/cluster/projects/${namespace}`; + path = `k8s/cluster/projects/${namespace}`; } } else if (validKind) { - result.pathname = `k8s/cluster/${validKind}`; + path = `k8s/cluster/${validKind}`; if (name) { - result.pathname += `/${name}`; + path += `/${name}`; } } - return result; + return new URL(path, basePath); } diff --git a/plugins/kubernetes/src/utils/clusterLinks/formatters/rancher.ts b/plugins/kubernetes/src/utils/clusterLinks/formatters/rancher.ts index a3a274496e..1ace39ec59 100644 --- a/plugins/kubernetes/src/utils/clusterLinks/formatters/rancher.ts +++ b/plugins/kubernetes/src/utils/clusterLinks/formatters/rancher.ts @@ -23,14 +23,24 @@ const kindMappings: Record = { }; export function rancherFormatter(options: ClusterLinksFormatterOptions): URL { - const result = new URL(options.dashboardUrl.href); - const name = options.object.metadata?.name; - const namespace = options.object.metadata?.namespace; + const basePath = new URL(options.dashboardUrl.href); + const name = encodeURIComponent(options.object.metadata?.name ?? ''); + const namespace = encodeURIComponent( + options.object.metadata?.namespace ?? '', + ); const validKind = kindMappings[options.kind.toLocaleLowerCase('en-US')]; - if (validKind && name && namespace) { - result.pathname += `explorer/${validKind}/${namespace}/${name}`; - } else if (namespace) { - result.pathname += 'explorer/workload'; + if (!basePath.pathname.endsWith('/')) { + // a dashboard url with a path should end with a slash otherwise + // the new combined URL will replace the last segment with the appended path! + // https://foobar.com/abc/def + explorer/service/test --> https://foobar.com/abc/explorer/service/test + // https://foobar.com/abc/def/ + explorer/service/test --> https://foobar.com/abc/def/explorer/service/test + basePath.pathname += '/'; } - return result; + let path = ''; + if (validKind && name && namespace) { + path = `explorer/${validKind}/${namespace}/${name}`; + } else if (namespace) { + path = 'explorer/workload'; + } + return new URL(path, basePath); } diff --git a/plugins/kubernetes/src/utils/clusterLinks/formatters/standard.test.ts b/plugins/kubernetes/src/utils/clusterLinks/formatters/standard.test.ts index 76bbf5643d..0b3c21a627 100644 --- a/plugins/kubernetes/src/utils/clusterLinks/formatters/standard.test.ts +++ b/plugins/kubernetes/src/utils/clusterLinks/formatters/standard.test.ts @@ -67,6 +67,51 @@ describe('clusterLinks - standard formatter', () => { 'https://k8s.foo.com/#/deployment/bar/foobar?namespace=bar', ); }); + it('should return an url on the deployment with a prefix 1', () => { + const url = standardFormatter({ + dashboardUrl: new URL('https://k8s.foo.com/some/prefix'), + object: { + metadata: { + name: 'foobar', + namespace: 'bar', + }, + }, + kind: 'Deployment', + }); + expect(formatUrl(url)).toBe( + 'https://k8s.foo.com/some/prefix/#/deployment/bar/foobar?namespace=bar', + ); + }); + it('should return an url on the deployment with a prefix 2', () => { + const url = standardFormatter({ + dashboardUrl: new URL('https://k8s.foo.com/some/prefix/'), + object: { + metadata: { + name: 'foobar', + namespace: 'bar', + }, + }, + kind: 'Deployment', + }); + expect(formatUrl(url)).toBe( + 'https://k8s.foo.com/some/prefix/#/deployment/bar/foobar?namespace=bar', + ); + }); + it('should return an url on the deployment properly url encoded', () => { + const url = standardFormatter({ + dashboardUrl: new URL('https://k8s.foo.com/'), + object: { + metadata: { + name: 'foobar', + namespace: 'bar bar', + }, + }, + kind: 'Deployment', + }); + expect(formatUrl(url)).toBe( + 'https://k8s.foo.com/#/deployment/bar%20bar/foobar?namespace=bar%20bar', + ); + }); it('should return an url on the service', () => { const url = standardFormatter({ dashboardUrl: new URL('https://k8s.foo.com/'), diff --git a/plugins/kubernetes/src/utils/clusterLinks/formatters/standard.ts b/plugins/kubernetes/src/utils/clusterLinks/formatters/standard.ts index fb26cf220d..e28c9fae2b 100644 --- a/plugins/kubernetes/src/utils/clusterLinks/formatters/standard.ts +++ b/plugins/kubernetes/src/utils/clusterLinks/formatters/standard.ts @@ -24,16 +24,22 @@ const kindMappings: Record = { export function standardFormatter(options: ClusterLinksFormatterOptions) { const result = new URL(options.dashboardUrl.href); - const name = options.object.metadata?.name; - const namespace = options.object.metadata?.namespace; + const name = encodeURIComponent(options.object.metadata?.name ?? ''); + const namespace = encodeURIComponent( + options.object.metadata?.namespace ?? '', + ); const validKind = kindMappings[options.kind.toLocaleLowerCase('en-US')]; - if (namespace) { - result.searchParams.set('namespace', namespace); + if (!result.pathname.endsWith('/')) { + result.pathname += '/'; } if (validKind && name && namespace) { result.hash = `/${validKind}/${namespace}/${name}`; } else if (namespace) { result.hash = '/workloads'; } + if (namespace) { + // Note that Angular SPA requires a hash and the query parameter should be part of it + result.hash += `?namespace=${namespace}`; + } return result; }