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

Support UTF-8 label matchers: Log an error if the parsers produce different parsings #3570

Conversation

grobinson-grafana
Copy link
Contributor

@grobinson-grafana grobinson-grafana commented Oct 24, 2023

This commit updates the fallback parser to compare the parsings of both the UTF-8 matchers parser and the classic parser, and log an error if the parsers produce different parsings.

This can happen at present if escape sequences such as \n and \\ are used outside of double quotes, as these are not un-escaped in the UTF-8 matchers parser, leaving them as literal \n and \\.

The risk here is that without this change, a small number of Alertmanager user's routes might break on upgrade and without warning.

@grobinson-grafana grobinson-grafana changed the title Log an error if the parsers produce different parsings Support UTF-8 label matchers: Log an error if the parsers produce different parsings Oct 24, 2023
@grobinson-grafana
Copy link
Contributor Author

@gotjosh undecided about what to do here. Do we support escape sequences outside of double quotes (a further relaxation of the grammar?) or do we want to warn users to add double quotes around their matchers that have escape sequences?

@grobinson-grafana grobinson-grafana force-pushed the grobinson/compare-in-fallback-parser branch from 1dd37f2 to c9fa6a1 Compare October 24, 2023 21:39
@gotjosh
Copy link
Member

gotjosh commented Oct 25, 2023

I assume that this means that there is a bug with the current parser as one of the design goals of compat was to make sure we don't introduce any sort of breaking change. Or does the fallback mechanism we have in place takes care of that?

If it is a breaking change, at what point do we update https://github.com/prometheus/alertmanager/blob/main/matchers/compat/parse_test.go with the scenario we're trying to cover?

@grobinson-grafana
Copy link
Contributor Author

grobinson-grafana commented Oct 25, 2023

I assume that this means that there is a bug with the current parser

It's not that there is a bug as such, I had omitted openmetrics escape sequences outside double quotes on purpose. The issue is that when we relaxed the grammar to allow unquoted characters other than [a-zA-Z0-9]+, we also permitted escape sequences such as \n, \\, and \".

In the UTF-8 matchers parser, these are not un-escaped and kept as-is:

foo=bar\n

But in the classic parser, these are unescaped as it supports openmetrics:

foo=bar

That said, I'm not convinced we should support openmetrics escape sequences outside double quotes. One option is we make \ a reserved character which would mean foo=bar\n is invalid in the UTF-8 parser (see #3571).

as one of the design goals of compat was to make sure we don't introduce any sort of breaking change. Or does the fallback mechanism we have in place takes care of that?

The fallback mechanism we have ensures that old matchers that do not conform to the grammar still parse. It doesn't ensure there are no breaking changes if the input is valid in both parsers, but produces different parsings. That's what this change does though.

I think we can choose between adding openmetrics support and making \ a reserved character. I would like to keep this change though to detect possible other unforeseen edge cases.

@grobinson-grafana grobinson-grafana force-pushed the grobinson/compare-in-fallback-parser branch from c9fa6a1 to 73d0913 Compare October 25, 2023 09:57
This commit updates the fallback parser to compare the parsings
of both the UTF-8 matchers parser and the classic parser, and
log an error if the parsers produce different parsings.

This can happen at present if ecsape sequences such as \n and \t
are used outside of double quotes, as this is not supported in
the UTF-8 matchers parser.

Signed-off-by: George Robinson <[email protected]>
@grobinson-grafana grobinson-grafana force-pushed the grobinson/compare-in-fallback-parser branch from 73d0913 to 9955d49 Compare October 25, 2023 10:12
Signed-off-by: George Robinson <[email protected]>
@grobinson-grafana
Copy link
Contributor Author

I'm going to close this as I'm not 100% sure it's needed.

@grobinson-grafana grobinson-grafana deleted the grobinson/compare-in-fallback-parser branch April 16, 2024 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants