Validate that every %{ in a grok processor is a grok token (SVR00011) - #1244
Validate that every %{ in a grok processor is a grok token (SVR00011)#1244ilyannn wants to merge 3 commits into
Conversation
Grok does not report text that fails to parse as a %{...} token. It
leaves it in the regular expression as-is, so the processor compiles
and runs and the capture the author wrote never exists. Because a
catch-all pattern or ignore_failure is usually nearby, nothing
downstream reports it either, and pipeline tests pass because their
expectations are generated from the broken output.
Six shipped packages in elastic/integrations have one: an alternation
inside a pattern name, a character class where a name goes, a missing
closing brace, a missing colon, a > for a }. Five of them silently lose
a field (elastic/integrations#21081). Run over all 485 packages, this
rule reports exactly those six and nothing else.
The token grammar is transcribed from GROK_PATTERN in Grok.java,
including its [A-z0-9] pattern-name class. Grok inside foreach and in
on_failure handlers is covered. Reported as a warning below 3.7.0 and
as an error from 3.7.0, following ValidateMinimumKibanaVersion.
| {fn: semantic.ValidateStaticHandlebarsFiles, types: []string{"integration", "input"}}, | ||
| {fn: semantic.ValidateKibanaTagDuplicates}, | ||
| {fn: semantic.ValidatePipelineOnFailure, types: []string{"integration"}, since: semver.MustParse("3.6.0")}, | ||
| {fn: warnOn(semantic.ValidatePipelineGrok), types: []string{"integration"}, until: semver.MustParse("3.7.0")}, |
There was a problem hiding this comment.
pinning this to 3.7 will just run for packages on this version; do we want to validate packages across all spec versions for this? is there a reason to pin it to 3.7?
There was a problem hiding this comment.
It does run across all spec versions — this line pairs with the one above it. Below 3.7.0 the same function is registered through warnOn, so every package gets the finding in lint output today as a warning; from 3.7.0 it becomes an error. I copied the shape from ValidateMinimumKibanaVersion (warning until 3.0.0, error since).
The reason for not making it an error everywhere: six shipped packages in elastic/integrations trip it right now (elastic/integrations#21081), and an unconditional error would fail their lint the moment elastic-package picks up this release. Three have fix PRs open (#21082, #21083, #21084); the other three (hid_bravura_monitor, nats, stan) belong to other teams and are only reported so far.
That said, every one of the six is a real bug that loses data, so an error for all versions is defensible. If you'd rather have that, I'll drop the warnOn line and the since, and the changelog entry already says breaking-change. Your call.
Note: drafted with 🤖 Cursor/Fable 5.1, under my supervision.
There was a problem hiding this comment.
(I really don't know what we usually do when we add a rule)
There was a problem hiding this comment.
we usually have a file under the packages to exclude rules (eg https://github.com/elastic/integrations/blob/831ce67b4dffcb722156640267a9e235bf35ebb0/packages/aws/validation.yml)
So i was thinking that this is something we want to align which is currently broken, we should enable it regardless the version. From my perspective, the spec establishes what should happen; if the package is wrong it can use the validation file to exclude until is fixed. When we release breaking changes on elastic-package we usually fix the packages alongside the bump.
Also, related to the version target; we can add this to the next patch as we don't have a 3.7 horizon right now.
cc @mrodm
There was a problem hiding this comment.
Also, related to the version target; we can add this to the next patch as we don't have a 3.7 horizon right now.
Yes, I think this could be released adding a new version entry as 3.6.7-next.
So i was thinking that this is something we want to align which is currently broken, we should enable it regardless the version. From my perspective, the spec establishes what should happen; if the package is wrong it can use the validation file to exclude until is fixed. When we release breaking changes on elastic-package we usually fix the packages alongside the bump.
I guess this was done to avoid breaking changes, that it is good (warning before a spec version and failing starting in that spec).
How could this issue affect to the packages that are not fixed @ilyannn ? Are there GitHub issues or pull requests created to fix those packages?
Options that I could think of:
- Keep the current until/since with the warnings
- Keep the current until/since but setting as spec threshold 3.0.0:
- old packages (
<3.0.0) would not be affected - tested locally and the packages that are failing right now are
stanandnats, that set spec 3.0.4
- old packages (
- Keep the current until/since but setting as spec threshold 3.1.0:
- old packages (
<3.1.0) would not be affected, but most of the new packages will apply this validation rule - no packages would fail
- lint in stan and nats would report warnings. Example:
2026/09/11 10:26:06 Warning: file "/home/mariorodriguez/Coding/work/integrations-upstream-main/packages/nats/data_stream/log/elasticsearch/ingest_pipeline/default.yml" is invalid: grok processor at line 31 has "%{GREEDYDATA:nats.log.msg.error\\]" in patterns[7], which is not a grok token and is matched as literal text (SVR00011)
- old packages (
- Apply everywhere this new validation.
- stan and nats packages will fail because of lint step
WDYT @teresaromero @ilyannn ?
| | [SVR00007] | Kibana tag is duplicate | | ||
| | [SVR00008] | Pipeline failure handler must set event.kind | | ||
| | [SVR00009] | Pipeline failure handler must set error.message | | ||
| | [SVR00011] | Grok pattern contains text that is not a token | |
💚 Build Succeeded
History
|
| {fn: semantic.ValidateStaticHandlebarsFiles, types: []string{"integration", "input"}}, | ||
| {fn: semantic.ValidateKibanaTagDuplicates}, | ||
| {fn: semantic.ValidatePipelineOnFailure, types: []string{"integration"}, since: semver.MustParse("3.6.0")}, | ||
| {fn: warnOn(semantic.ValidatePipelineGrok), types: []string{"integration"}, until: semver.MustParse("3.7.0")}, |
There was a problem hiding this comment.
Also, related to the version target; we can add this to the next patch as we don't have a 3.7 horizon right now.
Yes, I think this could be released adding a new version entry as 3.6.7-next.
So i was thinking that this is something we want to align which is currently broken, we should enable it regardless the version. From my perspective, the spec establishes what should happen; if the package is wrong it can use the validation file to exclude until is fixed. When we release breaking changes on elastic-package we usually fix the packages alongside the bump.
I guess this was done to avoid breaking changes, that it is good (warning before a spec version and failing starting in that spec).
How could this issue affect to the packages that are not fixed @ilyannn ? Are there GitHub issues or pull requests created to fix those packages?
Options that I could think of:
- Keep the current until/since with the warnings
- Keep the current until/since but setting as spec threshold 3.0.0:
- old packages (
<3.0.0) would not be affected - tested locally and the packages that are failing right now are
stanandnats, that set spec 3.0.4
- old packages (
- Keep the current until/since but setting as spec threshold 3.1.0:
- old packages (
<3.1.0) would not be affected, but most of the new packages will apply this validation rule - no packages would fail
- lint in stan and nats would report warnings. Example:
2026/09/11 10:26:06 Warning: file "/home/mariorodriguez/Coding/work/integrations-upstream-main/packages/nats/data_stream/log/elasticsearch/ingest_pipeline/default.yml" is invalid: grok processor at line 31 has "%{GREEDYDATA:nats.log.msg.error\\]" in patterns[7], which is not a grok token and is matched as literal text (SVR00011)
- old packages (
- Apply everywhere this new validation.
- stan and nats packages will fail because of lint step
WDYT @teresaromero @ilyannn ?
| fmt.Errorf("file %q is invalid: grok processor at line %d has %q in %s, which is not a grok token and is matched as literal text", | ||
| filename, proc.position.line, malformedTokenExcerpt(text[start:]), where), |
There was a problem hiding this comment.
How is this line obtained ? I see discrepancies between what it is reported (line number) and the actual line in the ingest pipeline.
Testing this branch with the packages in integrations fails for nats package for instance.
The error reported in
Error: linting package failed: found 1 validation error:
1. file "/home/mariorodriguez/Coding/work/integrations-upstream-main/packages/nats/data_stream/log/elasticsearch/ingest_pipeline/default.yml" is invalid: grok processor at line 31 has "%{GREEDYDATA:nats.log.msg.error\\]" in patterns[7], which is not a grok token and is matched as literal text (SVR00011)
But at that line there is no such string:
- line 31: https://github.com/elastic/integrations/blob/9bf0ca5794d3049667f858eebe8477f57c164176/packages/nats/data_stream/log/elasticsearch/ingest_pipeline/default.yml#L31
- it looks like it should be line 41: https://github.com/elastic/integrations/blob/9bf0ca5794d3049667f858eebe8477f57c164176/packages/nats/data_stream/log/elasticsearch/ingest_pipeline/default.yml#L41
3 participants
What does this PR do?
Adds semantic validation rule SVR00011: every
%{in a grok processor'spatternsandpattern_definitionsmust start a grok token.The token grammar is transcribed from
GROK_PATTERNinorg.elasticsearch.grok.Grok(including its[A-z0-9]pattern-name class, so nothing Elasticsearch accepts is rejected here). Grok processors insideforeachand inside processor-level and pipeline-levelon_failurehandlers are covered. The excerpt in the message runs from the%{to the first}after it, so the author sees exactly the text grok saw.Following
ValidateMinimumKibanaVersion, the rule is registered twice: as a warning for packages below3.7.0and as an error from3.7.0. Existing packages see the finding inelastic-package lintoutput today without anything breaking; the six affected packages are already fixed or tracked (below).Why is it important?
Grok does not report text that fails to parse as a
%{...}token. It leaves it in the regular expression as-is, so the processor compiles, runs, and simply never captures what the author wrote. There is no exception and no missing-pattern message; in practice a catch-all pattern orignore_failure: truesits nearby and absorbs the miss, and pipeline tests pass because their expected output was generated from the broken behaviour. The only way to find one is to look.Six shipped packages in
elastic/integrationshave one today, five of them silently losing a field: elastic/integrations#21081. The shapes are an alternation inside a pattern name (%{USERNAME|EMAILADDRESS:...}), a character class where a name goes (%{[A-Fa-f0-9]{32}:...}), a missing closing brace (%{GREEDYDATA:x.error\]), a missing colon (%{WORD_tmp.outcome}) and a>for a}in a definition. Each of them is one keystroke from correct, which is why they slipped past review.Run over all 485 packages in that repository, this rule reports exactly those six sites and nothing else.
What it looks like
elastic-package lintbuilt against this branch, onpulse_connect_securefromelastic/integrationsmain(the missing colon).As shipped today,
format_version: 3.0.3— a warning, exit 0:The same package on
format_version: 3.7.0— an error, exit 1. Lifting a 3.0-era package to 3.7 also trips the tag andon_failurerules, so the full list is 33 items; the last is this one:Line 55 is the grok processor's own line in the YAML,
patterns[4]picks the pattern within it, and the quoted text is the span grok saw, cut at the first}.Checklist
test/packagesthat prove my change is effective. (bad_pipeline_grok, asserted inTestValidateIngestPipelines; plus unit tests for each of the real-world shapes,foreachandon_failurenesting, and the token grammar's edge cases.)spec/changelog.yml.docs/validations.md.Related issues
%{…}in shipped grok patterns are read as literal text, so five fields are never captured integrations#21081 (the six findings; fixes for the three owned by@elastic/integration-experienceare [citrix_adc] Fix SSLVPN Message parsing and the Logout handler username integrations#21082, #21083, #21084)Note: drafted with 🤖 Cursor/Fable 5.1, under my supervision.