Guard reserved tags field against incorrect use by kaisecheng · Pull Request #14822 · elastic/logstash

kaisecheng · 2022-12-22T19:35:11Z

Release notes

The reserved top-level tags field has required a certain shape since v7.9. It supposes to be a string or an array of strings. Assigning a map to tags field could crash unexpectedly. The pull request #14822 guards tags field against incorrect use. Assigning a map will result in having a _tagsparsefailure in tags field and the illegal value will be written to _tags field.

A new setting --event_api.tags.illegal is added for backward compatibility. Since 8.7, the default value is rename to move illegal value to _tags field. warn maintains the same behavior of allowing illegal value assignment to tags field.

What does this PR do?

This PR prevents the reserved tags field to be assigned with key/value map. Top-level tags should only accept string of list. When tags get a map value, LogStash::Event will rewrite the value to _tags and add _tagsparsefailure to tags.

After the change, tags does not hold map value, while it uses to be allowed with ruby { code => "event.set('[tags][k]' , 'v');" }. This means the unconsumed events in PQ and DLQ with a map value in tags will have a different event structure. _tags hold illegal value from tags.

To make this change backward compatible, this PR adds a flag --event_api.tags.illegal to allow fallback to old logic.
There are two options.
warn - the old flow that allows illegal value assignment to tags field.
rename - the new flow that assigns illegal value to _tags field and adds _tagsparsefailure to tags. This is the default value in 8.7

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

Prior to this change, when a json payload contains tags with map value, the value is set successfully but if there is any error from plugins that needs to add a failure tag to tags, such as _jsonparsefailure or _timestampparsefailure, the pipeline crashes as it cannot append a tag to a map value. This is bad because users lose visibility of what went wrong.

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

Author's Checklist

[ ]

How to test this PR locally

Follow the reproducer of #14711

Extra test case

input {
  generator {
    message => '{"super": "ball"}'
    codec => json
    count => 1
  }
}
filter {
  mutate { "add_field" => {"[tags][k]" => "v" } }
  ruby {  code => 'fail "intentional"' }
}
output {
 stdout {}
}

Expected output:
tags=> [_tagsparsefailure, _rubyexception]
_tags => { k => v}

Related issues

Use cases

Screenshots

Logs

…g. If the top-level `tags` is incorrect used, event will add tag `_tagsparsefailure` and rewrite the value to `_tags`.

…d_tags_field

…lue assignment to `tags` field `rename` - rename key/value pair assignment to `_tags` field, which is the default action `warn` - allow key/value pair assignment to `tags` field, which is the old logic

github-actions · 2023-01-03T23:16:39Z

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

github-actions · 2023-01-04T13:05:04Z

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

github-actions · 2023-01-04T22:05:02Z

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

github-actions · 2023-01-04T23:21:18Z

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

yaauie · 2023-01-05T01:04:07Z

+        // guard tags field from key/value map, only string or list is allowed
+        if (ILLEGAL_TAGS_ACTION == IllegalTagsAction.RENAME) {
+            final Object tags = Accessors.get(data, TAGS_FIELD);
+            if (tags instanceof ConvertedMap) {


instead of a block-list of one particular bad shape, can we have an allow-list of the supported shapes?

yaauie · 2023-01-05T01:15:18Z

+                final FieldReference renamedField = renameIllegalTags(field);
+                Accessors.set(data, renamedField, Valuefier.convert(value));


I'm not sure this does what we want, since it renames the field independently of the shape of its value.

It may be worth intercepting TAGS_FIELD to be handled by a helper method:

public void setField(final FieldReference field, final Object value) { if (field.equals(TAGS_FIELD)) { setTagsField(value); return; } switch (field.type()) { // ... } }

private void setTagsField(final Object value) if (isLegalTagValue(value)) { setField(TAGS_FIELD, value); } else if (ILLEGAL_TAGS_ACTION == IllegalTagsAction.RENAME) setField(FieldReference.from(TAGS_FAILURE_FIELD), value); } else { // log warning? avoid flooding though? setField(TAGS_FIELD, value); } } private boolean isLegalTagValue(final Object value) { if (value instanceof String) { return true; } if (value instanceof List) { return true; } // TODO: make sure it is a list of _strings_, which is difficult to do with generics and type erasure return false; }

Applied the suggestion. It now handles the illegal tags field first in Event.setField()

yaauie · 2023-01-05T01:25:27Z

+        if (ILLEGAL_TAGS_ACTION == IllegalTagsAction.RENAME &&
+                field.getPath() != null && field.getPath().length > 0 && field.getPath()[0].equals(TAGS)) {


I read this as:

IF we are supposed to rename illegal-shaped tags

AND the field reference has a non-empty path

AND the first-level is tags

In my mind, we need to handle two cases when ILLEGAL_TAGS_ACTION == IllegalTagsAction.RENAME:

(path == null || path.length == 0) && key.equals(TAGS), with a value that is neither a string nor an array of strings (setting top-level tags to be an illegal shape)

(path != null && path.length > 0 && path[0].equals(TAGS)) regardless of value (setting field nested inside the tags, which cannot have nested fields in it)

Examples that should trigger a rename using the Event.setField(String, Object):

event.setField("[tags]", Map.of("poison", "true"))

event.setField("[tags][poison]", "true")

Similarly, using the Ruby APIs Event#set:

event.set("[tags]", { "poison" => "true"})

event.set("[tags][poison]", "true")

Thanks for listing the cases. The PR is changed to handle other illegal types, not limited to map. And _tags field holds a list of illegal values.

yaauie · 2023-01-05T21:31:48Z

    private final int type;

-    private FieldReference(final String[] path, final String key, final int type) {
+    public FieldReference(final String[] path, final String key, final int type) {


I am wary of making this public, since it would allow plugins built with the Java plugin API to route around our field reference caching.

I revert it to private and build the FieldReference from string instead

Co-authored-by: Ry Biesemeyer <yaauie@users.noreply.github.com>

github-actions · 2023-01-10T15:40:57Z

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

change `_tags` to a list to store all invalid value build FieldReference from string instead of a public constructor

github-actions · 2023-01-11T17:11:49Z

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

…d_tags_field

github-actions · 2023-01-12T02:33:55Z

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

github-actions · 2023-01-23T11:35:31Z

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

github-actions · 2023-01-23T11:49:44Z

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

jsvd · 2023-01-23T17:35:37Z

A few thoughts on the behaviour we have on @timestamp vs what we're doing in this PR:

# creating new event with incompatible type succeeds with a warning
irb(main):026:0> e = LogStash::Event.from_json('{"@timestamp": ""}').first
[2023-01-23T17:19:12,701][WARN ][org.logstash.Event       ] Error parsing @timestamp string value=
=> #<LogStash::Event:0x8d65037>
# the event has a new tag and wrong data is placed in _@timestamp
irb(main):027:0> e.to_hash
=> {"@timestamp"=>2023-01-23T17:19:12.701840Z, "@version"=>"1", "tags"=>["_timestampparsefailure"], "_@timestamp"=>""}
# after creation, attempting to set anything not compatible with LogStash::Timestamp throws exception
irb(main):028:0> e.set("@timestamp", "")
TypeError (wrong argument type String (expected LogStash::Timestamp))
irb(main):029:0> e.set("[@timestamp][1]", "")
Java::OrgLogstash::Accessors::InvalidFieldSetException (Could not set field '1' on object '2023-01-23T17:19:12.701840Z' to value ''.This is probably due to trying to set a field like [foo][bar] = someValuewhen [foo] is not either a map or a string)

On the Java side, setField is (too?) lenient, and allows us to break the event:

irb(main):031:0> je = e.to_java
=> #<Java::OrgLogstash::Event:0x822e8a1>
irb(main):032:0> je.setField("@timestamp", "")
=> nil
irb(main):034:0> je.to_string
Java::JavaLang::ClassCastException (class org.jruby.RubyString cannot be cast to class org.logstash.ext.JrubyTimestampExtLibrary$RubyTimestamp (org.jruby.RubyString and org.logstash.ext.JrubyTimestampExtLibrary$RubyTimestamp are in unnamed module of loader 'app'))

From this I think we can create a similar experience for the tags field:

Creating a new event with a type that is not compatible with the reserved field should still create the event but store the value in an underscored prefixed field. This PR does that.
Any set operation on an existing Ruby Event object should throw an error. Here this PR is smarter and allows the operation to occur and stores the invalid data alongside any previous one in the _tags field.

The main reason not to blindly crash on operations in 2. is that we may breaking some EXTREMELY edge cases out there in the world where a pipeline takes a fully created event and in the filter section sets a value to "tags" that isn't a string or list of strings. Considering this PR helps the "new event with broken 'tags'" use case already, I am comfortable throwing exceptions on setField. This should simplify the logic we need to support, and make the experience similar for the reserved fields @timestamp and tags.

jsvd · 2023-01-23T19:24:28Z

For illustration, here is the tags behaviour in this PR when event_api.tags.illegal: rename:

irb(main):035:0> e = LogStash::Event.from_json('{"tags": {"hey": "you"} }').first
=> #<LogStash::Event:0x7edad7d2>
irb(main):036:0> e.to_hash
=> {"_tags"=>[{"hey"=>"you"}], "tags"=>["_tagsparsefailure"], "@version"=>"1", "@timestamp"=>2023-01-23T19:21:42.430284Z}
irb(main):037:0> e.set("tags", {"hi": "me"})
=> {:hi=>"me"}
irb(main):038:0> e.set("[tags][hello]", 0)
=> 0
irb(main):039:0> e.to_hash
=> {"_tags"=>[{"hey"=>"you"}, {"hi"=>"me"}, {"hello"=>0}], "tags"=>["_tagsparsefailure"], "@version"=>"1", "@timestamp"=>2023-01-23T19:21:42.430284Z}

…istory - any set operation on event throws exception

github-actions · 2023-01-24T00:38:17Z

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

jsvd

I believe we can now remove rebaseOnto and all its associated tests?

github-actions · 2023-01-24T14:09:10Z

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

kaisecheng · 2023-01-24T14:44:02Z

@jsvd I have changed to rename illegal value to _tags instead of making a list of illegal history and throw exception for any illegal set operation. Thanks for your review and please have a look again.

jsvd

I'm good with the behaviour, I just think we should find another name to describe the default behaviour, and then ensure the docs reflect that.
Some of the docs still describe the behaviour as if "rename" happens both on creation and set, and it's actually "rename on creation, reject on set"

Co-authored-by: João Duarte <jsvd@users.noreply.github.com>

github-actions · 2023-01-24T21:58:14Z

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

github-actions · 2023-01-24T22:26:46Z

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

jsvd

LGTM

kaisecheng · 2023-01-25T15:15:38Z

@logstashmachine backport 8.7

roaksoax · 2023-10-13T16:11:40Z

For future reference:

This change was release-noted in https://www.elastic.co/guide/en/logstash/current/logstash-8-7-0.html .
The setting available is event_api.tags.illegal as documented in https://www.elastic.co/guide/en/logstash/current/logstash-settings-file.html
Users can event_api.tags.illegal to warn to rollback the behavior

kaisecheng added 5 commits December 22, 2022 16:32

This commit guards reserved tags field from setting by event parsin…

58ac5c0

…g. If the top-level `tags` is incorrect used, event will add tag `_tagsparsefailure` and rewrite the value to `_tags`.

guard tags from setting a key/value map though add_field

410c8b9

Merge branch 'main' of github.com:elastic/logstash into guard_reserve…

461a34d

…d_tags_field

typo

97dce2e

fix unit test

a528472

kaisecheng changed the title ~~Guard reserved tags field~~ Guard reserved tags field against incorrect use Dec 22, 2022

kaisecheng marked this pull request as ready for review December 22, 2022 23:32

kaisecheng added 2 commits January 3, 2023 22:50

add flag --event_api.tags.illegal to control behavior of illegal va…

9cf92f2

…lue assignment to `tags` field `rename` - rename key/value pair assignment to `_tags` field, which is the default action `warn` - allow key/value pair assignment to `tags` field, which is the old logic

add doc

8348a04

fix tests

f89d765

kaisecheng requested a review from yaauie January 4, 2023 12:53

kaisecheng removed the request for review from yaauie January 4, 2023 17:43

fix incorrect use of FieldReference

fa1ee92

rename illegal FieldReference

d37d453

kaisecheng requested a review from yaauie January 4, 2023 23:11

yaauie reviewed Jan 5, 2023

View reviewed changes

kaisecheng and others added 4 commits January 10, 2023 15:26

Update logstash-core/locales/en.yml

1e1a951

Co-authored-by: Ry Biesemeyer <yaauie@users.noreply.github.com>

Update logstash-core/locales/en.yml

c356d9f

Co-authored-by: Ry Biesemeyer <yaauie@users.noreply.github.com>

Update logstash-core/src/test/java/org/logstash/EventTest.java

4d7c080

Co-authored-by: Ry Biesemeyer <yaauie@users.noreply.github.com>

Update logstash-core/src/test/java/org/logstash/EventTest.java

f3ab6ed

Co-authored-by: Ry Biesemeyer <yaauie@users.noreply.github.com>

support type checking to guard against string and array of string

9fa7993

change `_tags` to a list to store all invalid value build FieldReference from string instead of a public constructor

kaisecheng added 2 commits January 11, 2023 23:44

Merge branch 'main' of github.com:elastic/logstash into guard_reserve…

94607c0

…d_tags_field

add integration test

713923c

fix doclint

cd27ed8

- rename illegal value to _tags instead of making a list of illegal h…

b312284

…istory - any set operation on event throws exception

jsvd reviewed Jan 24, 2023

View reviewed changes

kaisecheng added 2 commits January 24, 2023 13:54

Rename and clean up

32a7d8b

clean up rebaseOnto

05ae484

kaisecheng requested a review from jsvd January 24, 2023 14:44

jsvd reviewed Jan 24, 2023

View reviewed changes

Comment thread docs/static/settings-file.asciidoc Outdated

Comment thread logstash-core/src/main/java/org/logstash/FieldReference.java Outdated

kaisecheng and others added 2 commits January 24, 2023 21:44

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

11bf4aa

Co-authored-by: João Duarte <jsvd@users.noreply.github.com>

Update docs/static/settings-file.asciidoc

ff21c0d

Co-authored-by: João Duarte <jsvd@users.noreply.github.com>

update docs

5df8219

kaisecheng requested a review from jsvd January 25, 2023 11:08

jsvd approved these changes Jan 25, 2023

View reviewed changes

roaksoax added status:approved and removed status:needs-review labels Jan 25, 2023

kaisecheng merged commit 46443e4 into elastic:main Jan 25, 2023

roaksoax removed the status:approved label Jan 25, 2023

robbavey mentioned this pull request Jul 25, 2024

Remove event_api.tags.illegal (default becomes locked to 'rename') #16356

Closed

This was referenced Sep 17, 2024

Remove event_api.tags.illegal option #16460

Closed

Remove event_api.tags.illegal for v9 #16461

Merged

		final FieldReference renamedField = renameIllegalTags(field);
		Accessors.set(data, renamedField, Valuefier.convert(value));

		if (ILLEGAL_TAGS_ACTION == IllegalTagsAction.RENAME &&
		field.getPath() != null && field.getPath().length > 0 && field.getPath()[0].equals(TAGS)) {

Conversation

kaisecheng commented Dec 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Release notes

What does this PR do?

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

Checklist

Author's Checklist

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

Uh oh!

github-actions bot commented Jan 3, 2023

Uh oh!

github-actions bot commented Jan 4, 2023

Uh oh!

github-actions bot commented Jan 4, 2023

Uh oh!

github-actions bot commented Jan 4, 2023

Uh oh!

Uh oh!

Uh oh!

yaauie Jan 5, 2023

Choose a reason for hiding this comment

Uh oh!

yaauie Jan 5, 2023

Choose a reason for hiding this comment

Uh oh!

kaisecheng Jan 11, 2023

Choose a reason for hiding this comment

Uh oh!

yaauie Jan 5, 2023

Choose a reason for hiding this comment

Uh oh!

kaisecheng Jan 11, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

yaauie Jan 5, 2023

Choose a reason for hiding this comment

Uh oh!

kaisecheng Jan 11, 2023

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 10, 2023

Uh oh!

github-actions bot commented Jan 11, 2023

Uh oh!

github-actions bot commented Jan 12, 2023

Uh oh!

github-actions bot commented Jan 23, 2023

Uh oh!

github-actions bot commented Jan 23, 2023

Uh oh!

jsvd commented Jan 23, 2023

Uh oh!

jsvd commented Jan 23, 2023

Uh oh!

github-actions bot commented Jan 24, 2023

Uh oh!

jsvd left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 24, 2023

Uh oh!

kaisecheng commented Jan 24, 2023

Uh oh!

jsvd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Jan 24, 2023

Uh oh!

github-actions bot commented Jan 24, 2023

Uh oh!

jsvd left a comment

kaisecheng commented Dec 22, 2022 •

edited

Loading