Field Reference: handle special characters #14044

yaauie · 2022-04-27T21:33:46Z

Release notes

Fixes a long-standing issue where setting a field whose name includes characters that are reserved in the FieldReference syntax ([ and ]) had undefined behaviour that resulted in pipeline crashes or the silent truncation of the field name.
Introduces two experimental opt-in methods for referencing fields whose names include characters that are reserved in the FieldReference syntax

What does this PR do?

This PR has three commits that tell a story:

Demonstrate previously-unvalidated edge-cases in which FieldReference-special characters in field names result in undesired behaviour:

Category	Behaviour	Example
Valid field reference	truncate path, leaving only key	`[deeply][nested][square-brackets][field]` -> `field`
Invalid field reference	rejected input, potentially crash	`log[WARN]` -> CRASH!

Fixes the Event API's ConvertedMap internals so that data structures with fields whose names include FieldReference-special characters (currently [ and ]) correctly behave as expected – that is, uses field names as-given. This has the down-side of allowing fields on an event that cannot be referenced using the FieldReference syntax.
Introduces two EXPERIMENTAL opt-in ways of being able to reference fields with special characters in their names.

Why is it important/What is the impact to the user?

Allows plugins that handle as-given data-structures (like the JSON codec) to correctly create events from the events as-given, even if those events contain FieldReference-special characters.
Provides an experimental path forward for referencing fields whose names contain FieldReference-special characters

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have made corresponding change to the default configuration files (and/or docker env variables)
I have added tests that prove my fix is effective or that my feature works

How to test this PR locally

With the following pipeline.conf, we create an event from JSON data that contains a field named [bracketed][field], and attempt to address that field in a variety of ways (field reference literals, sprintf):

input { generator { count => 1 codec => json message => '{"[bracketed][field]": "okay"}' } }
filter {
  if [bracketed][field] {
    mutate { add_tag => "MATCH-literal" }
  }
  if [%5Bbracketed%5D%5Bfield%5D] {
    mutate { add_tag => "MATCH-percent" }
  }
  if [&#91;bracketed&#93;&#91;field&#93;] {
    mutate { add_tag => "MATCH-ampersand" }
  }
  mutate {
    replace => {
      "sprintf" => "LITERAL:~%{[bracketed][field]}~ PERCENT:~%{%5Bbracketed%5D%5Bfield%5D}~ AMPERSAND:~%{&#91;bracketed&#93;&#91;field&#93;}~"
    }
  }
}
output { stdout { codec => rubydebug } }

Execute each of the following to observe behaviour:

bin/logstash -f pipeline.conf

bin/logstash -f pipeline.conf --field-reference-escape-style=PERCENT

bin/logstash -f pipeline.conf --field-reference-escape-style=AMPERSAND

Related issues

Resolves: #13606
Resolves: #11608
Supersedes: #13479

…ences

Keys passed to most methods of `ConvertedMap`, based on `IdentityHashMap` depend on identity and not equivalence, and therefore rely on the keys being _interned_ strings. In order to avoid hitting the JVM's global String intern pool (which can have performance problems), operations to normalize a string to its interned counterpart have traditionally relied on the behaviour of `FieldReference#from` returning a likely-cached `FieldReference`, that had an interned `key` and an empty `path`. This is problematic on two points. First, when `ConvertedMap` was given data with keys that _were_ valid string field references representing a nested field (such as `[host][geo][location]`), the implementation of `ConvertedMap#put` effectively silently discarded the path components because it assumed them to be empty, and only the key was kept (`location`). Second, when `ConvertedMap` was given a map whose keys contained what the field reference parser considered special characters but _were NOT_ valid field references, the resulting `FieldReference.IllegalSyntaxException` caused the operation to abort. Instead of using the `FieldReference` cache, which sits on top of objects whose `key` and `path`-components are known to have been interned, we introduce an internment helper on our `ConvertedMap` that is also backed by the global string intern pool, and ensure that our field references are primed through this pool. In addition to fixing the `ConvertedMap#newFromMap` functionality, this has three net effects: - Our ConvertedMap operations still use strings from the global intern pool - We have a new, smaller cache of individual field names, improving lookup performance - Our FieldReference cache no longer is flooded with fragments and therefore is more likely to remain performant NOTE: this does NOT create isolated intern pools, as doing so would require a careful audit of the possible code-paths to `ConvertedMap#putInterned`. The new cache is limited to 10k strings, and when more are used only the FIRST 10k strings will be primed into the cache, leaving the remainder to always hit the global String intern pool. NOTE: by fixing this bug, we alow events to be created whose fields _CANNOT_ be referenced with the existing FieldReference implementation. Resolves: elastic#13606 Resolves: elastic#11608

Adds a `config.field_reference.escape_style` option and a companion command-line flag `--field-reference-escape-style` allowing a user to opt into one of two proposed escape-sequence implementations for field reference parsing: - `PERCENT`: URI-style `%`+`HH` hexadecimal encoding of UTF-8 bytes - `AMPERSAND`: HTML-style `&#`+`DD`+`;` encoding of decimal Unicode code-points The default is `NONE`, which does _not_ proccess escape sequences. With this setting a user effectively cannot reference a field whose name contains FieldReference-reserved characters. | ESCAPE STYLE | `[` | `]` | | ------------ | ------- | ------- | | `NONE` | _N/A_ | _N/A_ | | `PERCENT` | `%5B` | `%5D` | | `AMPERSAND` | `&elastic#91;` | `&elastic#93;` |

yaauie · 2022-04-28T15:54:43Z

Docs link: https://logstash_14044.docs-preview.app.elstc.co/diff

docs/static/field-reference.asciidoc

docs/static/settings-file.asciidoc

kares

🥇 👏 👏 👏

from my testing this seems to be working fine - only have minor comments that aren't blockers in terms of shipping the feature ...

looked into a very naive performance test of various escape styles, given the regular expression use on the new ones (obviously only matters when the cache gets full) :

the PERCENT style (un-cached) degrades by ~ 30% compared to NONE
the AMPERSAND is on par with NONE (the escaped.contains("&") seems to help)

logstash-core/src/main/java/org/logstash/FieldReference.java

kares · 2022-05-09T15:57:54Z

logstash-core/lib/logstash/runner.rb

@@ -87,6 +87,11 @@ class LogStash::Runner < Clamp::StrictCommand
    :default => LogStash::SETTINGS.get_default("config.string"),
    :attribute_name => "config.string"

+  option ["--field-reference-escape-style"], "STYLE",


got confused that this does not take effect when using irb e.g.
bin/logstash --field-reference-escape-style PERCENT -i irb but that's for a separate issue

Resolved in d58447d by moving the setting's application from the agent (which isn't started for shell sessions) to the runner before shell sessions are invoked.

Co-authored-by: Karol Bucek <[email protected]>

github-actions · 2022-05-18T15:52:49Z

📃 DOCS PREVIEW ✨ https://logstash_14044.docs-preview.app.elstc.co/diff

logstash-core/src/main/java/org/logstash/FieldReference.java

yaauie · 2022-05-18T15:49:06Z

logstash-core/lib/logstash/runner.rb

@@ -87,6 +87,11 @@ class LogStash::Runner < Clamp::StrictCommand
    :default => LogStash::SETTINGS.get_default("config.string"),
    :attribute_name => "config.string"

+  option ["--field-reference-escape-style"], "STYLE",


Resolved in d58447d by moving the setting's application from the agent (which isn't started for shell sessions) to the runner before shell sessions are invoked.

docs/static/field-reference.asciidoc

github-actions · 2022-05-20T02:31:42Z

📃 DOCS PREVIEW ✨ https://logstash_14044.docs-preview.app.elstc.co/diff

karenzone

Left some comments inline, most notably about replacing "experimental" with "technical preview."

karenzone · 2022-05-20T19:34:32Z

docs/static/field-reference.asciidoc

+[[formal-grammar-escape-sequences]]
+=== Escape Sequences
+
+In order to reference a field whose name contains a character that has special meaning in the field reference grammar, it needs to be escaped.


Suggested change

In order to reference a field whose name contains a character that has special meaning in the field reference grammar, it needs to be escaped.

For {ls} to reference a field whose name contains a character that has special meaning in the field reference grammar, the character must be escaped.

karenzone · 2022-05-20T19:35:19Z

docs/static/field-reference.asciidoc

+
+ - `NONE` (default): no escape sequence processing is done. Fields containing literal square brackets cannot be referenced by the Event API.
+ - `PERCENT`: URI-style percent encoding of UTF-8 bytes. The left square bracket (`[`) is expressed as `%5B`, and the right square bracket (`]`) is expressed as `%5D`.
+// NOTE: the following is _also_ HTML-escaped in the asciidoc source document so that browsers rendering the HTML will unwrap one escape and leave the remaining.


Suggested change

// NOTE: the following is _also_ HTML-escaped in the asciidoc source document so that browsers rendering the HTML will unwrap one escape and leave the remaining.

// Note that the following is _also_ HTML-escaped in the asciidoc source document so that browsers rendering the HTML will unwrap one escape and leave the remaining.

Suggest using regular words rather than the asciidoc admonition format. The comment treatment would keep it from getting formatted, but it keeps catching my eye.

karenzone · 2022-05-20T20:29:52Z

docs/static/settings-file.asciidoc

+* `AMPERSAND`: HTML-style `&#`{plus}`DD`{plus}`;` encoding of decimal Unicode code-points (`[` -> `&#91;`; `]` -> `&#91;`)
+* `NONE`: field names containing special characters _cannot_ be referenced.
+
+| `NONE`


Is this entry case sensitive? In this topic, we already have two instances of None and one instance of none.

It is. And there really is no reason for it to be upcase, so I have changed the implementation to be downcase throughout to match the other none.

Both other instances of "None" in the file should actually be "N/A" since they represent an absence of a default value instead of a default value that is the literal N+o+n+e

karenzone · 2022-05-20T20:36:38Z

docs/static/settings-file.asciidoc

@@ -178,6 +178,17 @@ Values other than `disabled` are currently considered BETA, and may produce unin
 | When set to `true`, quoted strings will process the following escape sequences: `\n` becomes a literal newline (ASCII 10). `\r` becomes a literal carriage return (ASCII 13). `\t` becomes a literal tab (ASCII 9). `\\` becomes a literal backslash `\`. `\"` becomes a literal double quotation mark. `\'` becomes a literal quotation mark.
 | `false`

+| `config.field_reference.escape_style`
+a| _EXPERIMENTAL_ setting that provides a way to reference fields that contain <<formal-grammar-escape-sequences,field reference special characters>> `[` and `]`.


Should be "Technical preview" instead of "Experimental." Reference: https://github.com/elastic/docs#using-the-technical-preview-admonition

I tried adding/formatting this nugget in a variety of ways. So far, I don't really like any of them. Here are two of several things I tried:

Admonitions are supposed to handle formatting, but I haven't hit on a combination that looks good and provides adequate info for the user. Only tagging the option with "Preview" doesn't convey the risk that the option might change or go away. I can sync with @gtback next week for ideas and design intent.

I wish the admonitions contained a link to a better place where we define what we mean and add more detail. I've never seen these used in a table, but let me know what I can do to help.

kares

👍

…cial-characters

github-actions · 2022-05-23T16:48:13Z

📃 DOCS PREVIEW ✨ https://logstash_14044.docs-preview.app.elstc.co/diff

dlouzan · 2022-06-28T16:25:45Z

@yaauie Did this make it to v8.3.0? The PR labels say so, but the tag diffs and release notes say otherwise.

We are experiencing this very error and would be really happy to put a fix in place 🙇

/cc @fh1ch

yaauie · 2022-06-28T19:01:23Z

Yes, it is in 8.3.0.

included in the ancestry of the v8.3.0 tag
included in the 8.3.0 release notes

dlouzan · 2022-06-29T11:34:15Z

@yaauie I must be blind, I promise I checked the release notes but did not see it. Our initial tests show very promising results, thank you!

yaauie added 3 commits April 25, 2022 15:57

add failing tests for Event.new with field that look like field refer…

5d80ce0

…ences

yaauie added bug enhancement labels Apr 27, 2022

roaksoax added the status:needs-review label Apr 27, 2022

roaksoax assigned yaauie Apr 28, 2022

yaauie commented Apr 28, 2022

View reviewed changes

docs/static/field-reference.asciidoc Outdated Show resolved Hide resolved

docs/static/settings-file.asciidoc Outdated Show resolved Hide resolved

fixup: no need to double-escape HTML-ish escape sequences in docs

4089067

kares self-requested a review May 4, 2022 16:02

yaauie commented May 4, 2022

View reviewed changes

docs/static/settings-file.asciidoc Outdated Show resolved Hide resolved

kares reviewed May 9, 2022

View reviewed changes

yaauie and others added 2 commits May 18, 2022 08:42

Apply suggestions from code review

f7c2999

Co-authored-by: Karol Bucek <[email protected]>

field-reference: load escape style in runner

d58447d

yaauie requested a review from karenzone May 18, 2022 18:04

yaauie commented May 20, 2022

View reviewed changes

docs: sentences over semiciolons

86168c1

karenzone reviewed May 20, 2022

View reviewed changes

kares approved these changes May 23, 2022

View reviewed changes

roaksoax added status:approved and removed status:needs-review labels May 23, 2022

yaauie added 5 commits May 23, 2022 15:31

Merge remote-tracking branch 'upstream/main' into field-reference-spe…

88af969

…cial-characters

field-reference: faster shortcut for PERCENT escape mode

62be405

field-reference: escape mode control downcase

96e8868

field_reference: more s/experimental/technical preview/

cc31ac9

field_reference: still more s/experimental/technical preview/

bb9d939

yaauie merged commit d845411 into elastic:main May 24, 2022

roaksoax removed the status:approved label May 24, 2022

roaksoax added the v8.3.0 label Jun 14, 2022

yaauie mentioned this pull request Jun 22, 2022

The input beats plugin crashes if the document field has a special character #14291

Open

yaauie deleted the field-reference-special-characters branch June 22, 2022 02:41

yaauie mentioned this pull request Oct 19, 2022

"[" key in an event crashes logstash #12123

Open

robbavey mentioned this pull request Nov 23, 2022

Invalid FieldReferences still occur when [,] are embedded in field names #14777

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Field Reference: handle special characters #14044

Field Reference: handle special characters #14044

yaauie commented Apr 27, 2022 •

edited

Loading

yaauie commented Apr 28, 2022

kares left a comment

kares May 9, 2022

yaauie May 18, 2022

github-actions bot commented May 18, 2022

yaauie May 18, 2022

github-actions bot commented May 20, 2022

karenzone left a comment

karenzone May 20, 2022

karenzone May 20, 2022

karenzone May 20, 2022

karenzone May 20, 2022

yaauie May 23, 2022 •

edited

Loading

karenzone May 20, 2022 •

edited

Loading

gtback May 23, 2022

kares left a comment

github-actions bot commented May 23, 2022

dlouzan commented Jun 28, 2022 •

edited

Loading

yaauie commented Jun 28, 2022

dlouzan commented Jun 29, 2022

	In order to reference a field whose name contains a character that has special meaning in the field reference grammar, it needs to be escaped.
	For {ls} to reference a field whose name contains a character that has special meaning in the field reference grammar, the character must be escaped.

	// NOTE: the following is _also_ HTML-escaped in the asciidoc source document so that browsers rendering the HTML will unwrap one escape and leave the remaining.
	// Note that the following is _also_ HTML-escaped in the asciidoc source document so that browsers rendering the HTML will unwrap one escape and leave the remaining.

Field Reference: handle special characters #14044

Field Reference: handle special characters #14044

Conversation

yaauie commented Apr 27, 2022 • edited Loading

Release notes

What does this PR do?

Why is it important/What is the impact to the user?

Checklist

How to test this PR locally

Related issues

yaauie commented Apr 28, 2022

kares left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 18, 2022

Choose a reason for hiding this comment

github-actions bot commented May 20, 2022

karenzone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaauie May 23, 2022 • edited Loading

Choose a reason for hiding this comment

karenzone May 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kares left a comment

Choose a reason for hiding this comment

github-actions bot commented May 23, 2022

dlouzan commented Jun 28, 2022 • edited Loading

yaauie commented Jun 28, 2022

dlouzan commented Jun 29, 2022

yaauie commented Apr 27, 2022 •

edited

Loading

yaauie May 23, 2022 •

edited

Loading

karenzone May 20, 2022 •

edited

Loading

dlouzan commented Jun 28, 2022 •

edited

Loading