From 77c1546511b7b5af1d6304dacdf5194acde104d1 Mon Sep 17 00:00:00 2001 From: "Grot (@grafanabot)" <43478413+grafanabot@users.noreply.github.com> Date: Fri, 7 Oct 2022 16:37:23 +0200 Subject: [PATCH] Loki: Fix redundant escaping in adhoc filter with regex match (#56447) (#56562) * Loki: Fix redundant escaping in adhoc filter with regex match * Update data.js * Simplify test * Simplify test * Update * Add more tests (cherry picked from commit a4c580144080625f7d8ec80c339bfa8ba6b15328) Co-authored-by: Ivana Huckova <30407135+ivanahuckova@users.noreply.github.com> --- devenv/docker/blocks/loki/data/data.js | 4 +- .../datasource/loki/datasource.test.ts | 57 ++++++++++++++----- .../app/plugins/datasource/loki/datasource.ts | 30 ++++++---- 3 files changed, 65 insertions(+), 26 deletions(-) diff --git a/devenv/docker/blocks/loki/data/data.js b/devenv/docker/blocks/loki/data/data.js index 07572a18efd..fed60c9e70b 100644 --- a/devenv/docker/blocks/loki/data/data.js +++ b/devenv/docker/blocks/loki/data/data.js @@ -125,8 +125,8 @@ async function main() { await sleep(getNextSineWaveSleepDuration()); const timestampMs = new Date().getTime(); const item = getRandomLogItem(step + 1) - lokiSendLogLine(timestampMs, JSON.stringify(item), {place:'moon', source: 'data'}); - lokiSendLogLine(timestampMs, logFmtLine(item), {place:'luna', source: 'data'}); + lokiSendLogLine(timestampMs, JSON.stringify(item), {place:'moon', source: 'data', instance: 'server\\1', job: '"grafana/data"'}); + lokiSendLogLine(timestampMs, logFmtLine(item), {place:'luna', source: 'data', instance: 'server\\2', job: '"grafana/data"'}); } } diff --git a/public/app/plugins/datasource/loki/datasource.test.ts b/public/app/plugins/datasource/loki/datasource.test.ts index eda90cbf0b6..8fc783a32cd 100644 --- a/public/app/plugins/datasource/loki/datasource.test.ts +++ b/public/app/plugins/datasource/loki/datasource.test.ts @@ -201,7 +201,7 @@ describe('LokiDatasource', () => { }, ]); expect(ds.applyTemplateVariables(query, {}).expr).toBe( - 'rate({bar="baz", job="foo", k1=~"v\\\\.\\\\*", k2=~"v\'\\\\.\\\\*"} |= "bar" [5m])' + 'rate({bar="baz", job="foo", k1=~"v.*", k2=~"v\\\\\'.*"} |= "bar" [5m])' ); }); }); @@ -665,10 +665,15 @@ describe('LokiDatasource', () => { describe('addAdHocFilters', () => { let ds: LokiDatasource; - let adHocFilters: AdHocFilter[]; + const createTemplateSrvMock = (options: { adHocFilters: AdHocFilter[] }) => { + return { + getAdhocFilters: (): AdHocFilter[] => options.adHocFilters, + replace: (a: string) => a, + } as unknown as TemplateSrv; + }; describe('when called with "=" operator', () => { beforeEach(() => { - adHocFilters = [ + const defaultAdHocFilters: AdHocFilter[] = [ { condition: '', key: 'job', @@ -676,11 +681,7 @@ describe('LokiDatasource', () => { value: 'grafana', }, ]; - const templateSrvMock = { - getAdhocFilters: (): AdHocFilter[] => adHocFilters, - replace: (a: string) => a, - } as unknown as TemplateSrv; - ds = createLokiDatasource(templateSrvMock); + ds = createLokiDatasource(createTemplateSrvMock({ adHocFilters: defaultAdHocFilters })); }); describe('and query has no parser', () => { it('then the correct label should be added for logs query', () => { @@ -706,6 +707,21 @@ describe('LokiDatasource', () => { it('then the correct label should be added for metrics query with empty selector and variable', () => { assertAdHocFilters('rate({}[$__interval])', 'rate({job="grafana"}[$__interval])', ds); }); + it('should correctly escape special characters in ad hoc filter', () => { + const ds = createLokiDatasource( + createTemplateSrvMock({ + adHocFilters: [ + { + condition: '', + key: 'instance', + operator: '=', + value: '"test"', + }, + ], + }) + ); + assertAdHocFilters('{job="grafana"}', '{job="grafana", instance="\\"test\\""}', ds); + }); }); describe('and query has parser', () => { it('then the correct label should be added for logs query', () => { @@ -719,7 +735,7 @@ describe('LokiDatasource', () => { describe('when called with "!=" operator', () => { beforeEach(() => { - adHocFilters = [ + const defaultAdHocFilters: AdHocFilter[] = [ { condition: '', key: 'job', @@ -727,11 +743,7 @@ describe('LokiDatasource', () => { value: 'grafana', }, ]; - const templateSrvMock = { - getAdhocFilters: (): AdHocFilter[] => adHocFilters, - replace: (a: string) => a, - } as unknown as TemplateSrv; - ds = createLokiDatasource(templateSrvMock); + ds = createLokiDatasource(createTemplateSrvMock({ adHocFilters: defaultAdHocFilters })); }); describe('and query has no parser', () => { it('then the correct label should be added for logs query', () => { @@ -751,6 +763,23 @@ describe('LokiDatasource', () => { }); }); }); + + describe('when called with regex operator', () => { + beforeEach(() => { + const defaultAdHocFilters: AdHocFilter[] = [ + { + condition: '', + key: 'instance', + operator: '=~', + value: '.*', + }, + ]; + ds = createLokiDatasource(createTemplateSrvMock({ adHocFilters: defaultAdHocFilters })); + }); + it('should not escape special characters in ad hoc filter', () => { + assertAdHocFilters('{job="grafana"}', '{job="grafana", instance=~".*"}', ds); + }); + }); }); describe('prepareLogRowContextQueryTarget', () => { diff --git a/public/app/plugins/datasource/loki/datasource.ts b/public/app/plugins/datasource/loki/datasource.ts index c1851b95255..5c3163277d2 100644 --- a/public/app/plugins/datasource/loki/datasource.ts +++ b/public/app/plugins/datasource/loki/datasource.ts @@ -49,7 +49,7 @@ import LanguageProvider from './LanguageProvider'; import { LiveStreams, LokiLiveTarget } from './LiveStreams'; import { transformBackendResult } from './backendResultTransformer'; import { LokiAnnotationsQueryEditor } from './components/AnnotationsQueryEditor'; -import { escapeLabelValueInSelector } from './languageUtils'; +import { escapeLabelValueInSelector, isRegexSelector } from './languageUtils'; import { labelNamesRegex, labelValuesRegex } from './migrations/variableQueryMigrations'; import { addLabelFormatToQuery, @@ -452,13 +452,15 @@ export class LokiDatasource switch (action.type) { case 'ADD_FILTER': { if (action.options?.key && action.options?.value) { - expression = this.addLabelToQuery(expression, action.options.key, '=', action.options.value); + const value = escapeLabelValueInSelector(action.options.value); + expression = addLabelToQuery(expression, action.options.key, '=', value); } break; } case 'ADD_FILTER_OUT': { if (action.options?.key && action.options?.value) { - expression = this.addLabelToQuery(expression, action.options.key, '!=', action.options.value); + const value = escapeLabelValueInSelector(action.options.value); + expression = addLabelToQuery(expression, action.options.key, '!=', value); } break; } @@ -743,18 +745,23 @@ export class LokiDatasource let expr = replaceVariables(queryExpr); expr = adhocFilters.reduce((acc: string, filter: { key: string; operator: string; value: string }) => { - const { key, operator, value } = filter; - return this.addLabelToQuery(acc, key, operator, value); + const { key, operator } = filter; + let { value } = filter; + if (isRegexSelector(operator)) { + // Adhoc filters don't support multiselect, therefore if user selects regex operator + // we are going to consider value to be regex filter and use lokiRegularEscape + // that does not escape regex special characters (e.g. .*test.* => .*test.*) + value = lokiRegularEscape(value); + } else { + // Otherwise, we want to escape special characters in value + value = escapeLabelValueInSelector(value, operator); + } + return addLabelToQuery(acc, key, operator, value); }, expr); return returnVariables(expr); } - addLabelToQuery(queryExpr: string, key: string, operator: string, value: string) { - const escapedValue = escapeLabelValueInSelector(value, operator); - return addLabelToQuery(queryExpr, key, operator, escapedValue); - } - // Used when running queries through backend filterQuery(query: LokiQuery): boolean { if (query.hide || query.expr === '') { @@ -790,6 +797,9 @@ export class LokiDatasource } } +// NOTE: these two functions are very similar to the escapeLabelValueIn* functions +// in language_utils.ts, but they are not exactly the same algorithm, and we found +// no way to reuse one in the another or vice versa. export function lokiRegularEscape(value: any) { if (typeof value === 'string') { return value.replace(/'/g, "\\\\'");