From cb3b7c01dc901b75a8f1419b59475e88ba07fb92 Mon Sep 17 00:00:00 2001 From: kaisecheng <69120390+kaisecheng@users.noreply.github.com> Date: Tue, 15 Oct 2024 22:27:36 +0100 Subject: [PATCH] Remove event_api.tags.illegal for v9 (#16461) This commit removed `--event_api.tags.illegal` option Fix: #16356 --- docker/data/logstash/env2yaml/env2yaml.go | 3 +- docs/static/settings-file.asciidoc | 6 --- logstash-core/lib/logstash/environment.rb | 1 - logstash-core/lib/logstash/runner.rb | 12 ----- logstash-core/locales/en.yml | 20 -------- logstash-core/spec/logstash/runner_spec.rb | 26 ---------- .../src/main/java/org/logstash/Event.java | 28 +++-------- .../src/test/java/org/logstash/EventTest.java | 47 +++---------------- .../specs/reserved_tags_field_spec.rb | 24 ++++------ 9 files changed, 23 insertions(+), 144 deletions(-) diff --git a/docker/data/logstash/env2yaml/env2yaml.go b/docker/data/logstash/env2yaml/env2yaml.go index 7bcaa33d17f..c7b79266815 100644 --- a/docker/data/logstash/env2yaml/env2yaml.go +++ b/docker/data/logstash/env2yaml/env2yaml.go @@ -16,12 +16,12 @@ package main import ( "errors" + "fmt" "gopkg.in/yaml.v2" "io/ioutil" "log" "os" "strings" - "fmt" ) var validSettings = []string{ @@ -49,7 +49,6 @@ var validSettings = []string{ "config.debug", "config.support_escapes", "config.field_reference.escape_style", - "event_api.tags.illegal", "queue.type", "path.queue", "queue.page_capacity", diff --git a/docs/static/settings-file.asciidoc b/docs/static/settings-file.asciidoc index d06327cce3d..b30207bd62e 100644 --- a/docs/static/settings-file.asciidoc +++ b/docs/static/settings-file.asciidoc @@ -361,12 +361,6 @@ separating each log lines per pipeline could be helpful in case you need to trou | Setting to `true` to allow or `false` to block running Logstash as a superuser. | `true` -| `event_api.tags.illegal` -| When set to `warn`, allow illegal value assignment to the reserved `tags` field. -When set to `rename`, Logstash events can't be created with an illegal value in `tags`. This value will be moved to `_tags` and a `_tagsparsefailure` tag is added to indicate the illegal operation. Doing `set` operation with illegal value will throw exception. -Setting this flag to `warn` is deprecated and will be removed in a future release. -| `rename` - | `pipeline.buffer.type` | Determine where to allocate memory buffers, for plugins that leverage them. Currently defaults to `direct` but can be switched to `heap` to select Java heap space, which will become the default in the future. diff --git a/logstash-core/lib/logstash/environment.rb b/logstash-core/lib/logstash/environment.rb index 164a190eb69..7b6f51ac68f 100644 --- a/logstash-core/lib/logstash/environment.rb +++ b/logstash-core/lib/logstash/environment.rb @@ -51,7 +51,6 @@ module Environment Setting::TimeValue.new("config.reload.interval", "3s"), # in seconds Setting::Boolean.new("config.support_escapes", false), Setting::String.new("config.field_reference.escape_style", "none", true, %w(none percent ampersand)), - Setting::String.new("event_api.tags.illegal", "rename", true, %w(rename warn)), Setting::Boolean.new("metric.collect", true), Setting::String.new("pipeline.id", "main"), Setting::Boolean.new("pipeline.system", false), diff --git a/logstash-core/lib/logstash/runner.rb b/logstash-core/lib/logstash/runner.rb index 20119d90467..842379364b3 100644 --- a/logstash-core/lib/logstash/runner.rb +++ b/logstash-core/lib/logstash/runner.rb @@ -263,12 +263,6 @@ class LogStash::Runner < Clamp::StrictCommand I18n.t("logstash.runner.flag.http_port"), :new_flag => "api.http.port", :passthrough => true # use settings to disambiguate - deprecated_option ["--event_api.tags.illegal"], "STRING", - I18n.t("logstash.runner.flag.event_api.tags.illegal"), - :default => LogStash::SETTINGS.get_default("event_api.tags.illegal"), - :attribute_name => "event_api.tags.illegal", :passthrough => true, - :obsoleted_version => "9" - # We configure the registry and load any plugin that can register hooks # with logstash, this needs to be done before any operation. SYSTEM_SETTINGS = LogStash::SETTINGS.clone @@ -356,12 +350,6 @@ def execute logger.debug("Setting global FieldReference escape style: #{field_reference_escape_style}") org.logstash.FieldReference::set_escape_style(field_reference_escape_style) - tags_illegal_setting = settings.get_setting('event_api.tags.illegal').value - if tags_illegal_setting == 'warn' - deprecation_logger.deprecated(I18n.t("logstash.runner.tags-illegal-warning")) - org.logstash.Event::set_illegal_tags_action(tags_illegal_setting) - end - return start_shell(setting("interactive"), binding) if setting("interactive") module_parser = LogStash::Modules::CLIParser.new(setting("modules_list"), setting("modules_variable_list")) diff --git a/logstash-core/locales/en.yml b/logstash-core/locales/en.yml index 2521a68255d..3176229e68b 100644 --- a/logstash-core/locales/en.yml +++ b/logstash-core/locales/en.yml @@ -139,9 +139,6 @@ en: configtest-flag-information: |- You may be interested in the '--configtest' flag which you can use to validate logstash's configuration before you choose to restart a running system. - tags-illegal-warning: >- - Setting `event_api.tags.illegal` to `warn` allows illegal values in the reserved `tags` field, which may crash pipeline unexpectedly. - This flag is deprecated and will be removed in version 9. # YAML named reference to the logstash.runner.configuration # so we can later alias it from logstash.agent.configuration configuration: &runner_configuration @@ -247,23 +244,6 @@ en: HTML-style ampersand-hash encoding notation representing decimal unicode codepoints (`[` is `[`; `]` is `]`). - event_api: - tags: - illegal: |+ - The top-level `tags` field is reserved, and may only contain a - single `string` or an array of `string`s -- other values will cause - subsequent access of the `tags` field to crash the pipeline. - This flag controls how the Event API handles a `tags` field that is - an illegal shape, such as a key-value map. - - Available options are: - - `rename`: illegal value in `tags` will be moved to `_tags`. - A tag `_tagsparsefailure` is added to `tags` field to - indicate the illegal assignment. Doing `set` operation with - illegal value will throw exception. This is the default option. - - `warn`: allow illegal value assignment and print warning - at startup. This option is deprecated and slated - for removal. modules: |+ Load Logstash modules. Modules can be defined using multiple instances diff --git a/logstash-core/spec/logstash/runner_spec.rb b/logstash-core/spec/logstash/runner_spec.rb index 0408e9de173..ffcb9ef9277 100644 --- a/logstash-core/spec/logstash/runner_spec.rb +++ b/logstash-core/spec/logstash/runner_spec.rb @@ -380,32 +380,6 @@ end end - context "event_api.tags.illegal" do - let(:runner_deprecation_logger_stub) { double("DeprecationLogger(Runner)").as_null_object } - let(:args) { ["--event_api.tags.illegal", "warn", "-e", pipeline_string] } - before(:each) { allow(runner).to receive(:deprecation_logger).and_return(runner_deprecation_logger_stub) } - DEPRECATED_MSG="The flag [\"--event_api.tags.illegal\"] has been deprecated and will be removed in version 9" - - it "gives deprecation message when setting to `warn`" do - expect(runner_deprecation_logger_stub).to receive(:deprecated) - .with(a_string_including "This flag is deprecated and will be removed in version 9") - .with(a_string_including DEPRECATED_MSG) - subject.run("bin/logstash", args) - end - - it "gives deprecation message when setting to `rename`" do - expect(runner_deprecation_logger_stub).to receive(:deprecated) - .with(a_string_including DEPRECATED_MSG) - subject.run("bin/logstash", args) - end - - it "does not give deprecation message when unset" do - expect(runner_deprecation_logger_stub).not_to receive(:deprecated) - .with(a_string_including DEPRECATED_MSG) - subject.run("bin/logstash", ["-e", pipeline_string]) - end - end - context "when :pipeline_workers is not defined by the user" do it "should not pass the value to the pipeline" do expect(LogStash::Agent).to receive(:new) do |settings| diff --git a/logstash-core/src/main/java/org/logstash/Event.java b/logstash-core/src/main/java/org/logstash/Event.java index 0621fb94c7a..e1e9f4db1f2 100644 --- a/logstash-core/src/main/java/org/logstash/Event.java +++ b/logstash-core/src/main/java/org/logstash/Event.java @@ -67,9 +67,6 @@ public final class Event implements Cloneable, Queueable, co.elastic.logstash.ap public static final String TAGS_FAILURE_TAG = "_tagsparsefailure"; public static final String TAGS_FAILURE = "_tags"; - enum IllegalTagsAction { RENAME, WARN } - private static IllegalTagsAction ILLEGAL_TAGS_ACTION = IllegalTagsAction.RENAME; - private static final FieldReference TAGS_FIELD = FieldReference.from(TAGS); private static final FieldReference TAGS_FAILURE_FIELD = FieldReference.from(TAGS_FAILURE); @@ -115,12 +112,10 @@ public Event(ConvertedMap data) { this.cancelled = false; // 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 (!isLegalTagValue(tags)) { - initFailTag(tags); - initTag(TAGS_FAILURE_TAG); - } + final Object tags = Accessors.get(data, TAGS_FIELD); + if (!isLegalTagValue(tags)) { + initFailTag(tags); + initTag(TAGS_FAILURE_TAG); } Object providedTimestamp = data.get(TIMESTAMP); @@ -217,11 +212,8 @@ public void setField(final String reference, final Object value) { @SuppressWarnings("unchecked") public void setField(final FieldReference field, final Object value) { - if (ILLEGAL_TAGS_ACTION == IllegalTagsAction.RENAME) { - if ((field.equals(TAGS_FIELD) && !isLegalTagValue(value)) || - isTagsWithMap(field)) { - throw new InvalidTagsTypeException(field, value); - } + if ((field.equals(TAGS_FIELD) && !isLegalTagValue(value)) || isTagsWithMap(field)) { + throw new InvalidTagsTypeException(field, value); } switch (field.type()) { @@ -544,14 +536,6 @@ private void scalarTagFallback(final String existing, final String tag) { appendTag(tags, tag); } - public static void setIllegalTagsAction(final String action) { - ILLEGAL_TAGS_ACTION = IllegalTagsAction.valueOf(action.toUpperCase()); - } - - public static IllegalTagsAction getIllegalTagsAction() { - return ILLEGAL_TAGS_ACTION; - } - @Override public byte[] serialize() throws JsonProcessingException { final Map> map = new HashMap<>(2, 1.0F); diff --git a/logstash-core/src/test/java/org/logstash/EventTest.java b/logstash-core/src/test/java/org/logstash/EventTest.java index d326e7a4432..e93088d7772 100644 --- a/logstash-core/src/test/java/org/logstash/EventTest.java +++ b/logstash-core/src/test/java/org/logstash/EventTest.java @@ -20,6 +20,12 @@ package org.logstash; +import org.jruby.RubyString; +import org.jruby.RubySymbol; +import org.jruby.RubyTime; +import org.jruby.java.proxies.ConcreteJavaProxy; +import org.junit.Test; + import java.io.IOException; import java.math.BigDecimal; import java.math.BigInteger; @@ -30,11 +36,6 @@ import java.util.HashMap; import java.util.List; import java.util.Map; -import org.jruby.RubyString; -import org.jruby.RubySymbol; -import org.jruby.RubyTime; -import org.jruby.java.proxies.ConcreteJavaProxy; -import org.junit.Test; import static net.javacrumbs.jsonunit.JsonAssert.assertJsonEquals; import static org.hamcrest.CoreMatchers.is; @@ -43,7 +44,6 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; -import static org.logstash.Event.getIllegalTagsAction; public final class EventTest extends RubyTestBase { @@ -565,39 +565,4 @@ public void allowTopLevelTagsListOfStrings() { assertNull(event.getField(Event.TAGS_FAILURE)); assertEquals(event.getField("[tags]"), List.of("foo", "bar")); } - - @Test - public void allowTopLevelTagsWithMap() { - withIllegalTagsAction(Event.IllegalTagsAction.WARN, () -> { - final Event event = new Event(); - event.setField("[tags][foo]", "bar"); - - assertNull(event.getField(Event.TAGS_FAILURE)); - assertEquals(event.getField("[tags][foo]"), "bar"); - }); - } - - @Test - public void allowCreatingEventWithTopLevelTagsWithMap() { - withIllegalTagsAction(Event.IllegalTagsAction.WARN, () -> { - Map inner = new HashMap<>(); - inner.put("poison", "true"); - Map data = new HashMap<>(); - data.put("tags", inner); - final Event event = new Event(data); - - assertNull(event.getField(Event.TAGS_FAILURE)); - assertEquals(event.getField("[tags][poison]"), "true"); - }); - } - - private void withIllegalTagsAction(final Event.IllegalTagsAction temporaryIllegalTagsAction, final Runnable runnable) { - final Event.IllegalTagsAction previous = getIllegalTagsAction(); - try { - Event.setIllegalTagsAction(temporaryIllegalTagsAction.toString()); - runnable.run(); - } finally { - Event.setIllegalTagsAction(previous.toString()); - } - } } diff --git a/qa/integration/specs/reserved_tags_field_spec.rb b/qa/integration/specs/reserved_tags_field_spec.rb index a9e99d95e39..00854eba422 100644 --- a/qa/integration/specs/reserved_tags_field_spec.rb +++ b/qa/integration/specs/reserved_tags_field_spec.rb @@ -21,7 +21,7 @@ require_relative '../framework/helpers' require "logstash/devutils/rspec/spec_helper" -# reserved tags should accept string and array of string only in rename mode +# reserved tags should accept string and array of string only describe "Guard reserved tags field against incorrect use" do before(:all) { @fixture = Fixture.new(__FILE__) @@ -48,11 +48,10 @@ } let(:settings_dir) { Stud::Temporary.directory } - shared_examples_for 'assign illegal value to tags' do |mode, pipeline_fixture, tags_match, fail_tags_match| - it "[#{mode}] update tags and _tags successfully" do + shared_examples_for 'assign illegal value to tags' do |pipeline_fixture, tags_match, fail_tags_match| + it "update tags and _tags successfully" do @logstash.env_variables = test_env @logstash.spawn_logstash("-f", config_to_temp_file(@fixture.config(pipeline_fixture)), - "--event_api.tags.illegal", "#{mode}", "--path.settings", settings_dir) Stud.try(num_retries.times, [StandardError, RSpec::Expectations::ExpectationNotMetError]) do @@ -65,18 +64,15 @@ end describe 'create event' do - it_behaves_like 'assign illegal value to tags', 'rename', 'create_tags_map', /"tags":\["_tagsparsefailure"\]/, /"_tags":\[{"poison":true}\]/ - it_behaves_like 'assign illegal value to tags', 'warn', 'create_tags_map', /"tags":{"poison":true}/, /(?!_tags)/ - it_behaves_like 'assign illegal value to tags', 'rename', 'create_tags_number', /"tags":\["_tagsparsefailure"\]/, /"_tags":\[\[1,2,3\]\]/ - it_behaves_like 'assign illegal value to tags', 'warn', 'create_tags_number', /"tags":\[1,2,3\]/, /(?!_tags)/ + it_behaves_like 'assign illegal value to tags', 'create_tags_map', /"tags":\["_tagsparsefailure"\]/, /"_tags":\[{"poison":true}\]/ + it_behaves_like 'assign illegal value to tags', 'create_tags_number', /"tags":\["_tagsparsefailure"\]/, /"_tags":\[\[1,2,3\]\]/ end it "should throw exception when assigning two illegal values" do - ['rename', 'warn'].each do |mode| - logstash = @logstash.run_cmd(["bin/logstash", "-e", @fixture.config('set_illegal_tags').gsub("\n", ""), - "--path.settings", settings_dir, "--event_api.tags.illegal", mode], - true, test_env) - expect(logstash.stderr_and_stdout).to match(/Ruby exception occurred/) - end + logstash = @logstash.run_cmd(["bin/logstash", "-e", @fixture.config('set_illegal_tags').gsub("\n", ""), + "--path.settings", settings_dir], + true, test_env) + expect(logstash.stderr_and_stdout).to match(/Ruby exception occurred/) end + end