Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Loki: Fix wrongly escaped label values when using LabelFilter #59812

Merged
merged 4 commits into from Dec 6, 2022

Conversation

svennergr
Copy link
Contributor

What is this feature?

As reported in #59333 using the filter-in and out buttons in Loki is currently broken when the label value contains special characters. This PR fixes that by not using backticks but quotes as identifier for a value.

Which issue(s) does this PR fix?:

Fixes #59333

Special notes for your reviewer:

  1. With a Loki devenv open Explore
  2. Run a {place="luna"} | logfmt query
  3. Click the Filter-In button on an instance label containing server\2 or a job label containing "grafana/data".
  4. Without the fix notice the bug.

@svennergr svennergr requested a review from matyax December 5, 2022 16:50
Comment on lines +215 to +217
let value = getString(expr, node.getChild(String));
// `value` is wrapped in double quotes, so we need to remove them. As a value can contain double quotes, we can't use RegEx here.
value = value.substring(1, value.length - 1);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@matyax @ivanahuckova that was the issue which caused wrong visuals in the query builder when switching from code. Since it also messed up my testing I fixed it here directly.

@@ -35,6 +35,10 @@ export function escapeLabelValueInExactSelector(labelValue: string): string {
return labelValue.replace(/\\/g, '\\\\').replace(/\n/g, '\\n').replace(/"/g, '\\"');
}

export function unescapeLabelValue(labelValue: string): string {
return labelValue.replace(/\\n/g, '\n').replace(/\\"/g, '"').replace(/\\\\/g, '\\');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we care about \n here?

Copy link
Contributor Author

@svennergr svennergr Dec 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically I am just blindly doing the opposite as

export function escapeLabelValueInExactSelector(labelValue: string): string {
return labelValue.replace(/\\/g, '\\\\').replace(/\n/g, '\\n').replace(/"/g, '\\"');
}
, which is escaping newlines.

I am also unsure if a label value will ever contain newlines. Guess that's a good question which can be answered in #59824.

Copy link
Contributor

@matyax matyax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified that it fixes the bug 👍

@svennergr svennergr merged commit 932349b into main Dec 6, 2022
@svennergr svennergr deleted the svennergr/59333-fix-label-value-escape branch December 6, 2022 11:30
@grafanabot
Copy link
Contributor

The backport to v9.3.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new branch
git switch --create backport-59812-to-v9.3.x origin/v9.3.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x 932349b5ab4c91df3b78f99d78d20e7c5b851032
# Push it to GitHub
git push --set-upstream origin backport-59812-to-v9.3.x
git switch main
# Remove the local backport branch
git branch -D backport-59812-to-v9.3.x

Then, create a pull request where the base branch is v9.3.x and the compare/head branch is backport-59812-to-v9.3.x.

@grafanabot grafanabot added the backport-failed Failed to generate backport PR. Please resolve conflicts and create one manually. label Dec 6, 2022
svennergr added a commit that referenced this pull request Dec 6, 2022
* change backticks to quotes

* add unescaping to `addFilterAsLabelFilter`

* fix removal of wrong quotes

* change unescape order

(cherry picked from commit 932349b)
svennergr added a commit that referenced this pull request Dec 6, 2022
#59876)

Loki: Fix wrongly escaped label values when using LabelFilter (#59812)

* change backticks to quotes

* add unescaping to `addFilterAsLabelFilter`

* fix removal of wrong quotes

* change unescape order

(cherry picked from commit 932349b)
GuYounes pushed a commit to paul-wurth/BIXpert that referenced this pull request Feb 8, 2023
grafana#59876)

Loki: Fix wrongly escaped label values when using LabelFilter (grafana#59812)

* change backticks to quotes

* add unescaping to `addFilterAsLabelFilter`

* fix removal of wrong quotes

* change unescape order

(cherry picked from commit 932349b)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add to changelog area/frontend backport v9.3.x backport-failed Failed to generate backport PR. Please resolve conflicts and create one manually. datasource/Loki enterprise-ok type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Explore Loki query builder uses escape on backtick strings
3 participants