From 73468e14817f073d8feaa1312a22253d593faa55 Mon Sep 17 00:00:00 2001 From: Ivana Huckova <30407135+ivanahuckova@users.noreply.github.com> Date: Mon, 13 Jul 2020 10:54:01 +0200 Subject: [PATCH] Jaeger/Zipkin: URL-encode service names and trace ids for API calls (#26115) * Encode services and id * Encode URL for Zipkin API calls * Add test coverage --- .../datasource/jaeger/QueryField.test.tsx | 55 ++++++++++++++++++- .../plugins/datasource/jaeger/QueryField.tsx | 2 +- .../datasource/jaeger/datasource.test.ts | 26 ++++++++- .../plugins/datasource/jaeger/datasource.ts | 2 +- .../datasource/zipkin/datasource.test.ts | 6 ++ .../plugins/datasource/zipkin/datasource.ts | 4 +- 6 files changed, 89 insertions(+), 6 deletions(-) diff --git a/public/app/plugins/datasource/jaeger/QueryField.test.tsx b/public/app/plugins/datasource/jaeger/QueryField.test.tsx index 6d0e41985da..2aa8172725b 100644 --- a/public/app/plugins/datasource/jaeger/QueryField.test.tsx +++ b/public/app/plugins/datasource/jaeger/QueryField.test.tsx @@ -18,6 +18,55 @@ describe('JaegerQueryField', function() { expect(wrapper.find(ButtonCascader).props().options[0].label).toBe('No traces found'); }); + it('uses URL encoded service name in metadataRequest request', async function() { + const wrapper = mount( + {}} + onChange={() => {}} + /> + ); + + // Simulating selection options. We need this as the function depends on the intermediate state of the component + await wrapper + .find(ButtonCascader) + .props() + .loadData([{ value: 'service/test', label: 'service/test' }]); + + wrapper.update(); + expect(wrapper.find(ButtonCascader).props().options[0].label).toEqual('service/test'); + expect(wrapper.find(ButtonCascader).props().options[0].value).toEqual('service/test'); + expect(wrapper.find(ButtonCascader).props().options[0].children[1]).toEqual({ + isLeaf: false, + label: 'op1', + value: 'op1', + }); + }); + it('shows root span as 3rd level in cascader', async function() { const wrapper = mount( { findOperations = async (service: string) => { const { datasource } = this.props; - const url = `/api/services/${service}/operations`; + const url = `/api/services/${encodeURIComponent(service)}/operations`; try { return await datasource.metadataRequest(url); } catch (error) { diff --git a/public/app/plugins/datasource/jaeger/datasource.test.ts b/public/app/plugins/datasource/jaeger/datasource.test.ts index 666427f9b43..5ecdce4d36b 100644 --- a/public/app/plugins/datasource/jaeger/datasource.test.ts +++ b/public/app/plugins/datasource/jaeger/datasource.test.ts @@ -16,6 +16,28 @@ describe('JaegerDatasource', () => { }); }); + it('returns trace when traceId with special characters is queried', async () => { + await withMockedBackendSrv(makeBackendSrvMock('a/b'), async () => { + const ds = new JaegerDatasource(defaultSettings); + const query = { + ...defaultQuery, + targets: [ + { + query: 'a/b', + refId: '1', + }, + ], + }; + const response = await ds.query(query).toPromise(); + const field = response.data[0].fields[0]; + expect(field.name).toBe('trace'); + expect(field.type).toBe(FieldType.trace); + expect(field.values.get(0)).toEqual({ + traceId: 'a/b', + }); + }); + }); + it('returns empty response if trace id is not specified', async () => { const ds = new JaegerDatasource(defaultSettings); const response = await ds @@ -34,7 +56,9 @@ describe('JaegerDatasource', () => { function makeBackendSrvMock(traceId: string) { return { datasourceRequest(options: BackendSrvRequest): Promise { - expect(options.url.substr(options.url.length - 17, options.url.length)).toBe(`/api/traces/${traceId}`); + expect(options.url.substr(options.url.length - 17, options.url.length)).toBe( + `/api/traces/${encodeURIComponent(traceId)}` + ); return Promise.resolve({ data: { data: [ diff --git a/public/app/plugins/datasource/jaeger/datasource.ts b/public/app/plugins/datasource/jaeger/datasource.ts index 89f3cb9ef42..52cf1ced4cf 100644 --- a/public/app/plugins/datasource/jaeger/datasource.ts +++ b/public/app/plugins/datasource/jaeger/datasource.ts @@ -36,7 +36,7 @@ export class JaegerDatasource extends DataSourceApi { const id = options.targets[0]?.query; if (id) { // TODO: this api is internal, used in jaeger ui. Officially they have gRPC api that should be used. - return this._request(`/api/traces/${id}`).pipe( + return this._request(`/api/traces/${encodeURIComponent(id)}`).pipe( map(response => { return { data: [ diff --git a/public/app/plugins/datasource/zipkin/datasource.test.ts b/public/app/plugins/datasource/zipkin/datasource.test.ts index dffa9ace48f..bd0ccd9a493 100644 --- a/public/app/plugins/datasource/zipkin/datasource.test.ts +++ b/public/app/plugins/datasource/zipkin/datasource.test.ts @@ -11,6 +11,12 @@ describe('ZipkinDatasource', () => { const response = await ds.query({ targets: [{ query: '12345' }] } as DataQueryRequest).toPromise(); expect(response.data[0].fields[0].values.get(0)).toEqual(jaegerTrace); }); + it('runs query with traceId that includes special characters', async () => { + setupBackendSrv({ url: '/api/datasources/proxy/1/api/v2/trace/a%2Fb', response: zipkinResponse }); + const ds = new ZipkinDatasource(defaultSettings); + const response = await ds.query({ targets: [{ query: 'a/b' }] } as DataQueryRequest).toPromise(); + expect(response.data[0].fields[0].values.get(0)).toEqual(jaegerTrace); + }); }); describe('metadataRequest', () => { diff --git a/public/app/plugins/datasource/zipkin/datasource.ts b/public/app/plugins/datasource/zipkin/datasource.ts index b24638aaae5..cb019ae3855 100644 --- a/public/app/plugins/datasource/zipkin/datasource.ts +++ b/public/app/plugins/datasource/zipkin/datasource.ts @@ -27,7 +27,9 @@ export class ZipkinDatasource extends DataSourceApi { query(options: DataQueryRequest): Observable { const traceId = options.targets[0]?.query; if (traceId) { - return this.request(`${apiPrefix}/trace/${traceId}`).pipe(map(responseToDataQueryResponse)); + return this.request(`${apiPrefix}/trace/${encodeURIComponent(traceId)}`).pipe( + map(responseToDataQueryResponse) + ); } else { return of(emptyDataQueryResponse); }