From 6df5df065425564690c54c3ed18961cc70f9244f Mon Sep 17 00:00:00 2001 From: andsel Date: Thu, 17 Oct 2024 11:30:02 +0200 Subject: [PATCH 01/10] Reimplemented LogStash::String setting in Java --- logstash-core/lib/logstash/environment.rb | 26 ++++++++-------- logstash-core/lib/logstash/settings.rb | 3 ++ .../org/logstash/settings/BaseSetting.java | 2 +- .../org/logstash/settings/StringSetting.java | 30 +++++++++++++++++++ 4 files changed, 47 insertions(+), 14 deletions(-) create mode 100644 logstash-core/src/main/java/org/logstash/settings/StringSetting.java diff --git a/logstash-core/lib/logstash/environment.rb b/logstash-core/lib/logstash/environment.rb index 164a190eb69..ce7d8ad1785 100644 --- a/logstash-core/lib/logstash/environment.rb +++ b/logstash-core/lib/logstash/environment.rb @@ -50,8 +50,8 @@ module Environment Setting::Boolean.new("config.reload.automatic", false), 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::StringSetting.new("config.field_reference.escape_style", "none", true, %w(none percent ampersand)), + Setting::StringSetting.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), @@ -67,30 +67,30 @@ module Environment Setting.new("path.plugins", Array, []), Setting::NullableString.new("interactive", nil, false), Setting::Boolean.new("config.debug", false), - Setting::String.new("log.level", "info", true, ["fatal", "error", "warn", "debug", "info", "trace"]), + Setting::StringSetting.new("log.level", "info", true, ["fatal", "error", "warn", "debug", "info", "trace"]), Setting::Boolean.new("version", false), Setting::Boolean.new("help", false), Setting::Boolean.new("enable-local-plugin-development", false), - Setting::String.new("log.format", "plain", true, ["json", "plain"]), + Setting::StringSetting.new("log.format", "plain", true, ["json", "plain"]), Setting::Boolean.new("log.format.json.fix_duplicate_message_fields", false), Setting::Boolean.new("api.enabled", true).with_deprecated_alias("http.enabled"), Setting::String.new("api.http.host", "127.0.0.1").with_deprecated_alias("http.host"), Setting::PortRange.new("api.http.port", 9600..9700).with_deprecated_alias("http.port"), Setting::String.new("api.environment", "production").with_deprecated_alias("http.environment"), - Setting::String.new("api.auth.type", "none", true, %w(none basic)), + Setting::StringSetting.new("api.auth.type", "none", true, %w(none basic)), Setting::String.new("api.auth.basic.username", nil, false).nullable, Setting::Password.new("api.auth.basic.password", nil, false).nullable, - Setting::String.new("api.auth.basic.password_policy.mode", "WARN", true, %w[WARN ERROR]), + Setting::StringSetting.new("api.auth.basic.password_policy.mode", "WARN", true, %w[WARN ERROR]), Setting::Numeric.new("api.auth.basic.password_policy.length.minimum", 8), - Setting::String.new("api.auth.basic.password_policy.include.upper", "REQUIRED", true, %w[REQUIRED OPTIONAL]), - Setting::String.new("api.auth.basic.password_policy.include.lower", "REQUIRED", true, %w[REQUIRED OPTIONAL]), - Setting::String.new("api.auth.basic.password_policy.include.digit", "REQUIRED", true, %w[REQUIRED OPTIONAL]), - Setting::String.new("api.auth.basic.password_policy.include.symbol", "OPTIONAL", true, %w[REQUIRED OPTIONAL]), + Setting::StringSetting.new("api.auth.basic.password_policy.include.upper", "REQUIRED", true, %w[REQUIRED OPTIONAL]), + Setting::StringSetting.new("api.auth.basic.password_policy.include.lower", "REQUIRED", true, %w[REQUIRED OPTIONAL]), + Setting::StringSetting.new("api.auth.basic.password_policy.include.digit", "REQUIRED", true, %w[REQUIRED OPTIONAL]), + Setting::StringSetting.new("api.auth.basic.password_policy.include.symbol", "OPTIONAL", true, %w[REQUIRED OPTIONAL]), Setting::Boolean.new("api.ssl.enabled", false), Setting::ExistingFilePath.new("api.ssl.keystore.path", nil, false).nullable, Setting::Password.new("api.ssl.keystore.password", nil, false).nullable, Setting::StringArray.new("api.ssl.supported_protocols", nil, true, %w[TLSv1 TLSv1.1 TLSv1.2 TLSv1.3]), - Setting::String.new("queue.type", "memory", true, ["persisted", "memory"]), + Setting::StringSetting.new("queue.type", "memory", true, ["persisted", "memory"]), Setting::Boolean.new("queue.drain", false), Setting::Bytes.new("queue.page_capacity", "64mb"), Setting::Bytes.new("queue.max_bytes", "1024mb"), @@ -102,7 +102,7 @@ module Environment Setting::Boolean.new("dead_letter_queue.enable", false), Setting::Bytes.new("dead_letter_queue.max_bytes", "1024mb"), Setting::Numeric.new("dead_letter_queue.flush_interval", 5000), - Setting::String.new("dead_letter_queue.storage_policy", "drop_newer", true, ["drop_newer", "drop_older"]), + Setting::StringSetting.new("dead_letter_queue.storage_policy", "drop_newer", true, ["drop_newer", "drop_older"]), Setting::NullableString.new("dead_letter_queue.retain.age"), # example 5d Setting::TimeValue.new("slowlog.threshold.warn", "-1"), Setting::TimeValue.new("slowlog.threshold.info", "-1"), @@ -111,7 +111,7 @@ module Environment Setting::String.new("keystore.classname", "org.logstash.secret.store.backend.JavaKeyStore"), Setting::String.new("keystore.file", ::File.join(::File.join(LogStash::Environment::LOGSTASH_HOME, "config"), "logstash.keystore"), false), # will be populated on Setting::NullableString.new("monitoring.cluster_uuid"), - Setting::String.new("pipeline.buffer.type", "direct", true, ["direct", "heap"]) + Setting::StringSetting.new("pipeline.buffer.type", "direct", true, ["direct", "heap"]) # post_process ].each {|setting| SETTINGS.register(setting) } diff --git a/logstash-core/lib/logstash/settings.rb b/logstash-core/lib/logstash/settings.rb index 5d2fbd588df..9d285884b59 100644 --- a/logstash-core/lib/logstash/settings.rb +++ b/logstash-core/lib/logstash/settings.rb @@ -524,6 +524,9 @@ def validate(value) end end + + java_import org.logstash.settings.StringSetting + class String < Setting def initialize(name, default = nil, strict = true, possible_strings = []) @possible_strings = possible_strings diff --git a/logstash-core/src/main/java/org/logstash/settings/BaseSetting.java b/logstash-core/src/main/java/org/logstash/settings/BaseSetting.java index b08408393df..5174a984580 100644 --- a/logstash-core/src/main/java/org/logstash/settings/BaseSetting.java +++ b/logstash-core/src/main/java/org/logstash/settings/BaseSetting.java @@ -166,7 +166,7 @@ public void reset() { } public void validateValue() { - validate(this.value); + validate(this.value()); } public T getDefault() { diff --git a/logstash-core/src/main/java/org/logstash/settings/StringSetting.java b/logstash-core/src/main/java/org/logstash/settings/StringSetting.java new file mode 100644 index 00000000000..b70d8ea51cd --- /dev/null +++ b/logstash-core/src/main/java/org/logstash/settings/StringSetting.java @@ -0,0 +1,30 @@ +package org.logstash.settings; + +import java.util.Collections; +import java.util.List; + +public class StringSetting extends BaseSetting { + + private List possibleStrings = Collections.emptyList(); + + public StringSetting(String name, String defaultValue, boolean strict, List possibleStrings) { + super(name, strict, noValidator()); // this super doesn't call validate either if it's strict + this.possibleStrings = possibleStrings; + this.defaultValue = defaultValue; + + if (strict) { + staticValidate(defaultValue, possibleStrings, name); + } + } + + @Override + public void validate(String input) throws IllegalArgumentException { + staticValidate(input, possibleStrings, this.getName()); + } + + private static void staticValidate(String input, List possibleStrings, String name) { + if (!possibleStrings.isEmpty() && !possibleStrings.contains(input)) { + throw new IllegalArgumentException(String.format("Invalid value \"%s: %s\" . Options are: %s", name, input, possibleStrings)); + } + } +} From 01f74e7c297a25887ac897cc7dd90165cfcd9efa Mon Sep 17 00:00:00 2001 From: andsel Date: Fri, 18 Oct 2024 09:38:35 +0200 Subject: [PATCH 02/10] [WIP] Introduce StringSetting constructor that accepts 2 arguments --- logstash-core/lib/logstash/environment.rb | 10 +++++----- logstash-core/spec/logstash/runner_spec.rb | 1 + .../logstash/settings/SettingWithDeprecatedAlias.java | 2 +- .../main/java/org/logstash/settings/StringSetting.java | 4 ++++ 4 files changed, 11 insertions(+), 6 deletions(-) diff --git a/logstash-core/lib/logstash/environment.rb b/logstash-core/lib/logstash/environment.rb index ce7d8ad1785..df39f589b0d 100644 --- a/logstash-core/lib/logstash/environment.rb +++ b/logstash-core/lib/logstash/environment.rb @@ -35,7 +35,7 @@ module Environment [ Setting::Boolean.new("allow_superuser", true), - Setting::String.new("node.name", Socket.gethostname), + Setting::StringSetting.new("node.name", Socket.gethostname), Setting::NullableString.new("path.config", nil, false), Setting::WritableDirectory.new("path.data", ::File.join(LogStash::Environment::LOGSTASH_HOME, "data")), Setting::NullableString.new("config.string", nil, false), @@ -53,7 +53,7 @@ module Environment Setting::StringSetting.new("config.field_reference.escape_style", "none", true, %w(none percent ampersand)), Setting::StringSetting.new("event_api.tags.illegal", "rename", true, %w(rename warn)), Setting::Boolean.new("metric.collect", true), - Setting::String.new("pipeline.id", "main"), + Setting::StringSetting.new("pipeline.id", "main"), Setting::Boolean.new("pipeline.system", false), Setting::PositiveInteger.new("pipeline.workers", LogStash::Config::CpuCoreStrategy.maximum), Setting::PositiveInteger.new("pipeline.batch.size", 125), @@ -74,9 +74,9 @@ module Environment Setting::StringSetting.new("log.format", "plain", true, ["json", "plain"]), Setting::Boolean.new("log.format.json.fix_duplicate_message_fields", false), Setting::Boolean.new("api.enabled", true).with_deprecated_alias("http.enabled"), - Setting::String.new("api.http.host", "127.0.0.1").with_deprecated_alias("http.host"), + Setting::StringSetting.new("api.http.host", "127.0.0.1").with_deprecated_alias("http.host"), Setting::PortRange.new("api.http.port", 9600..9700).with_deprecated_alias("http.port"), - Setting::String.new("api.environment", "production").with_deprecated_alias("http.environment"), + Setting::StringSetting.new("api.environment", "production").with_deprecated_alias("http.environment"), Setting::StringSetting.new("api.auth.type", "none", true, %w(none basic)), Setting::String.new("api.auth.basic.username", nil, false).nullable, Setting::Password.new("api.auth.basic.password", nil, false).nullable, @@ -108,7 +108,7 @@ module Environment Setting::TimeValue.new("slowlog.threshold.info", "-1"), Setting::TimeValue.new("slowlog.threshold.debug", "-1"), Setting::TimeValue.new("slowlog.threshold.trace", "-1"), - Setting::String.new("keystore.classname", "org.logstash.secret.store.backend.JavaKeyStore"), + Setting::StringSetting.new("keystore.classname", "org.logstash.secret.store.backend.JavaKeyStore"), Setting::String.new("keystore.file", ::File.join(::File.join(LogStash::Environment::LOGSTASH_HOME, "config"), "logstash.keystore"), false), # will be populated on Setting::NullableString.new("monitoring.cluster_uuid"), Setting::StringSetting.new("pipeline.buffer.type", "direct", true, ["direct", "heap"]) diff --git a/logstash-core/spec/logstash/runner_spec.rb b/logstash-core/spec/logstash/runner_spec.rb index 0408e9de173..44466578ef7 100644 --- a/logstash-core/spec/logstash/runner_spec.rb +++ b/logstash-core/spec/logstash/runner_spec.rb @@ -271,6 +271,7 @@ expect(LogStash::Agent).to receive(:new) do |settings| expect(settings.set?("api.http.host")).to be(true) expect(settings.get("api.http.host")).to eq("localhost") + # settings.get_setting("api.http.host").observe_post_process end subject.run("bin/logstash", args) diff --git a/logstash-core/src/main/java/org/logstash/settings/SettingWithDeprecatedAlias.java b/logstash-core/src/main/java/org/logstash/settings/SettingWithDeprecatedAlias.java index 3b3b0678ed7..9ce6705303c 100644 --- a/logstash-core/src/main/java/org/logstash/settings/SettingWithDeprecatedAlias.java +++ b/logstash-core/src/main/java/org/logstash/settings/SettingWithDeprecatedAlias.java @@ -98,7 +98,7 @@ public void format(List output) { @Override public void validateValue() { if (deprecatedAlias.isSet() && getCanonicalSetting().isSet()) { - throw new IllegalStateException(String.format("Both `%s` and its deprecated alias `%s` have been set.\n" + + throw new IllegalStateException(String.format("Both `%s` and its deprecated alias `%s` have been set. " + "Please only set `%s`", getCanonicalSetting().getName(), deprecatedAlias.getName(), getCanonicalSetting().getName())); } super.validateValue(); diff --git a/logstash-core/src/main/java/org/logstash/settings/StringSetting.java b/logstash-core/src/main/java/org/logstash/settings/StringSetting.java index b70d8ea51cd..dc1dd1ce58a 100644 --- a/logstash-core/src/main/java/org/logstash/settings/StringSetting.java +++ b/logstash-core/src/main/java/org/logstash/settings/StringSetting.java @@ -17,6 +17,10 @@ public StringSetting(String name, String defaultValue, boolean strict, List Date: Mon, 28 Oct 2024 16:27:49 +0100 Subject: [PATCH 03/10] Fixed spec test that verified a log from depreacated alias to use the log4j real logger properly configured to spy the logged messages --- logstash-core/spec/logstash/runner_spec.rb | 38 ++++++++++++++++++++-- 1 file changed, 36 insertions(+), 2 deletions(-) diff --git a/logstash-core/spec/logstash/runner_spec.rb b/logstash-core/spec/logstash/runner_spec.rb index 44466578ef7..b0894edec5d 100644 --- a/logstash-core/spec/logstash/runner_spec.rb +++ b/logstash-core/spec/logstash/runner_spec.rb @@ -263,18 +263,52 @@ let(:runner_deprecation_logger_stub) { double("DeprecationLogger(Runner)").as_null_object } before(:each) { allow(runner).to receive(:deprecation_logger).and_return(runner_deprecation_logger_stub) } + let(:configuration_spy) { configure_log_spy } + + def configure_log_spy + java_import org.apache.logging.log4j.core.config.builder.api.ConfigurationBuilderFactory + java_import org.apache.logging.log4j.Level + config_builder = ConfigurationBuilderFactory.newConfigurationBuilder + return config_builder + .add( + config_builder + .newAppender("LOG_SPY", "List") + .add(config_builder.newLayout("PatternLayout").addAttribute("pattern", "%-5p [%t]: %m%n")) + ) + .add( + config_builder + .newRootLogger(Level::INFO) + .add(config_builder.newAppenderRef("LOG_SPY"))) + .build(false) + end + context "when deprecated :http.host is defined by the user" do let(:args) { ["--http.host", "localhost", "-e", pipeline_string]} it "creates an Agent whose `api.http.host` uses the provided value and provides helpful deprecation message" do - expect(deprecation_logger_stub).to receive(:deprecated).with(a_string_including "`http.host` is a deprecated alias for `api.http.host`") + java_import org.apache.logging.log4j.core.config.Configurator + java_import org.apache.logging.log4j.core.config.Configuration + + java_import org.apache.logging.log4j.LogManager + java_import org.apache.logging.log4j.core.impl.Log4jContextFactory + # This is done because the LoggerExt calls setFactory LogstashLoggerContextFactory implements + # org.apache.logging.log4j.spi.LoggerContextFactory and doesn't extend org.apache.logging.log4j.core.impl.Log4jContextFactory + # which is the class expected by the following Configurator to use the programmatic configuration. + LogManager.setFactory(Log4jContextFactory.new) + log_ctx = Configurator.java_send(:initialize, [Configuration], configure_log_spy) + expect(log_ctx).not_to be nil + log_ctx.reconfigure(configure_log_spy) # force the programmatic configuration, without this it's not used + + appender_spy = log_ctx.getConfiguration().getAppender("LOG_SPY") + expect(runner_deprecation_logger_stub).to receive(:deprecated).with(a_string_including 'The flag ["--http.host"] has been deprecated') expect(LogStash::Agent).to receive(:new) do |settings| expect(settings.set?("api.http.host")).to be(true) expect(settings.get("api.http.host")).to eq("localhost") - # settings.get_setting("api.http.host").observe_post_process end subject.run("bin/logstash", args) + + expect(appender_spy.messages[0]).to match(/`http.host` is a deprecated alias for `api.http.host`/) end end From 7a5d6bd9d3f0d848e25283e519b4aa3e93ff83f6 Mon Sep 17 00:00:00 2001 From: andsel Date: Mon, 28 Oct 2024 17:52:56 +0100 Subject: [PATCH 04/10] Added String constructor with 2 arguments --- logstash-core/lib/logstash/environment.rb | 4 ++-- .../src/main/java/org/logstash/settings/StringSetting.java | 4 ++++ 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/logstash-core/lib/logstash/environment.rb b/logstash-core/lib/logstash/environment.rb index df39f589b0d..f8a9d8d92b8 100644 --- a/logstash-core/lib/logstash/environment.rb +++ b/logstash-core/lib/logstash/environment.rb @@ -78,7 +78,7 @@ module Environment Setting::PortRange.new("api.http.port", 9600..9700).with_deprecated_alias("http.port"), Setting::StringSetting.new("api.environment", "production").with_deprecated_alias("http.environment"), Setting::StringSetting.new("api.auth.type", "none", true, %w(none basic)), - Setting::String.new("api.auth.basic.username", nil, false).nullable, + Setting::StringSetting.new("api.auth.basic.username", nil, false).nullable, Setting::Password.new("api.auth.basic.password", nil, false).nullable, Setting::StringSetting.new("api.auth.basic.password_policy.mode", "WARN", true, %w[WARN ERROR]), Setting::Numeric.new("api.auth.basic.password_policy.length.minimum", 8), @@ -109,7 +109,7 @@ module Environment Setting::TimeValue.new("slowlog.threshold.debug", "-1"), Setting::TimeValue.new("slowlog.threshold.trace", "-1"), Setting::StringSetting.new("keystore.classname", "org.logstash.secret.store.backend.JavaKeyStore"), - Setting::String.new("keystore.file", ::File.join(::File.join(LogStash::Environment::LOGSTASH_HOME, "config"), "logstash.keystore"), false), # will be populated on + Setting::StringSetting.new("keystore.file", ::File.join(::File.join(LogStash::Environment::LOGSTASH_HOME, "config"), "logstash.keystore"), false), # will be populated on Setting::NullableString.new("monitoring.cluster_uuid"), Setting::StringSetting.new("pipeline.buffer.type", "direct", true, ["direct", "heap"]) # post_process diff --git a/logstash-core/src/main/java/org/logstash/settings/StringSetting.java b/logstash-core/src/main/java/org/logstash/settings/StringSetting.java index dc1dd1ce58a..eafb1425030 100644 --- a/logstash-core/src/main/java/org/logstash/settings/StringSetting.java +++ b/logstash-core/src/main/java/org/logstash/settings/StringSetting.java @@ -21,6 +21,10 @@ public StringSetting(String name, String defaultValue) { this(name, defaultValue, true, Collections.emptyList()); } + public StringSetting(String name, String defaultValue, boolean strict) { + this(name, defaultValue, strict, Collections.emptyList()); + } + @Override public void validate(String input) throws IllegalArgumentException { staticValidate(input, possibleStrings, this.getName()); From 47a89398daf23bb80658d66b9ac2a36db7b1778c Mon Sep 17 00:00:00 2001 From: andsel Date: Tue, 29 Oct 2024 11:28:08 +0100 Subject: [PATCH 05/10] Fixed nullable specs after the usage of the new Java StringSetting instead of Ruby one --- logstash-core/lib/logstash/settings.rb | 2 ++ logstash-core/lib/logstash/util/settings_helper.rb | 4 ++-- logstash-core/spec/logstash/settings/nullable_spec.rb | 8 ++++---- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/logstash-core/lib/logstash/settings.rb b/logstash-core/lib/logstash/settings.rb index 9d285884b59..ee4158ac5c6 100644 --- a/logstash-core/lib/logstash/settings.rb +++ b/logstash-core/lib/logstash/settings.rb @@ -827,6 +827,8 @@ def validate(value) end end + java_import org.logstash.settings.NullableSetting + # @see Setting#nullable # @api internal class Nullable < SimpleDelegator diff --git a/logstash-core/lib/logstash/util/settings_helper.rb b/logstash-core/lib/logstash/util/settings_helper.rb index 6925a38f53f..e0f0d72ebf4 100644 --- a/logstash-core/lib/logstash/util/settings_helper.rb +++ b/logstash-core/lib/logstash/util/settings_helper.rb @@ -33,8 +33,8 @@ module LogStash::Util::SettingsHelper # The `path.settings` and `path.logs` can not be defined in "logstash/environment" since the `Environment::LOGSTASH_HOME` doesn't # exist unless launched via "bootstrap/environment" def self.pre_process - LogStash::SETTINGS.register(LogStash::Setting::String.new("path.settings", ::File.join(LogStash::Environment::LOGSTASH_HOME, "config"))) - LogStash::SETTINGS.register(LogStash::Setting::String.new("path.logs", ::File.join(LogStash::Environment::LOGSTASH_HOME, "logs"))) + LogStash::SETTINGS.register(LogStash::Setting::StringSetting.new("path.settings", ::File.join(LogStash::Environment::LOGSTASH_HOME, "config"))) + LogStash::SETTINGS.register(LogStash::Setting::StringSetting.new("path.logs", ::File.join(LogStash::Environment::LOGSTASH_HOME, "logs"))) end # Ensure that any settings are re-calculated after loading yaml diff --git a/logstash-core/spec/logstash/settings/nullable_spec.rb b/logstash-core/spec/logstash/settings/nullable_spec.rb index 130df2e150f..9c0f7c91f6d 100644 --- a/logstash-core/spec/logstash/settings/nullable_spec.rb +++ b/logstash-core/spec/logstash/settings/nullable_spec.rb @@ -20,13 +20,13 @@ describe LogStash::Setting::Nullable do let(:setting_name) { "this.that" } - let(:normal_setting) { LogStash::Setting::String.new(setting_name, nil, false, possible_strings) } + let(:normal_setting) { LogStash::Setting::StringSetting.new(setting_name, nil, false, possible_strings) } let(:possible_strings) { [] } # empty means any string passes subject(:nullable_setting) { normal_setting.nullable } it 'is a kind of Nullable' do - expect(nullable_setting).to be_a_kind_of(described_class) + expect(nullable_setting).to be_a_kind_of(LogStash::Setting::NullableSetting) end it "retains the wrapped setting's name" do @@ -56,14 +56,14 @@ context 'to an invalid wrong-type value' do let(:candidate_value) { 127 } # wrong type, expects String it 'is an invalid setting' do - expect { nullable_setting.validate_value }.to raise_error(ArgumentError, a_string_including("Setting \"#{setting_name}\" must be a ")) + expect { nullable_setting.validate_value }.to raise_error(java.lang.ClassCastException, a_string_including("class java.lang.Long cannot be cast to class java.lang.String")) end end context 'to an invalid value not in the allow-list' do let(:possible_strings) { %w(this that)} let(:candidate_value) { "another" } # wrong type, expects String it 'is an invalid setting' do - expect { nullable_setting.validate_value }.to raise_error(ArgumentError, a_string_including("Invalid value")) + expect { nullable_setting.validate_value }.to raise_error(java.lang.IllegalArgumentException, a_string_including("Invalid value")) end end context 'to a valid value' do From b1f07d2027908c9c26f8a881230f19e93e45d834 Mon Sep 17 00:00:00 2001 From: andsel Date: Tue, 29 Oct 2024 18:59:02 +0100 Subject: [PATCH 06/10] Moved from logger double to real log spy using log4j appender in SettingWithDeprecatedAlias spec test. --- .../setting_with_deprecated_alias_spec.rb | 27 ++++++++++---- spec/spec_helper.rb | 37 +++++++++++++++++++ 2 files changed, 57 insertions(+), 7 deletions(-) diff --git a/logstash-core/spec/logstash/settings/setting_with_deprecated_alias_spec.rb b/logstash-core/spec/logstash/settings/setting_with_deprecated_alias_spec.rb index 481e387ba03..3dc879824d7 100644 --- a/logstash-core/spec/logstash/settings/setting_with_deprecated_alias_spec.rb +++ b/logstash-core/spec/logstash/settings/setting_with_deprecated_alias_spec.rb @@ -25,7 +25,16 @@ let(:default_value) { "DeFaUlT" } let(:settings) { LogStash::Settings.new } - let(:canonical_setting) { LogStash::Setting::String.new(canonical_setting_name, default_value, true) } + let(:canonical_setting) { LogStash::Setting::StringSetting.new(canonical_setting_name, default_value, true) } + + let(:log_ctx) { setup_logger_spy } + let(:log_spy) { retrieve_logger_spy(log_ctx) } + + before(:each) do + # Initialization of appender and logger use to spy, need to be done before executing any code that logs, + # that's the reason wy to refer the spying logger context before any test. + log_ctx + end before(:each) do allow(LogStash::Settings).to receive(:logger).and_return(double("SettingsLogger").as_null_object) @@ -57,6 +66,8 @@ it 'does not emit a deprecation warning' do expect(LogStash::Settings.deprecation_logger).to_not receive(:deprecated).with(a_string_including(deprecated_setting_name)) settings.get_setting(deprecated_setting_name).observe_post_process + log_spy = retrieve_logger_spy(log_ctx) + expect(log_spy.messages).to be_empty end end end @@ -66,6 +77,7 @@ before(:each) do settings.set(deprecated_setting_name, value) + settings.get_setting(deprecated_setting_name).observe_post_process end it 'resolves to the value provided for the deprecated alias' do @@ -73,15 +85,15 @@ end it 'logs a deprecation warning' do - expect(LogStash::Settings.deprecation_logger).to have_received(:deprecated).with(a_string_including(deprecated_setting_name)) + expect(log_spy.messages[0]).to include(deprecated_setting_name) end include_examples '#validate_value success' context "#observe_post_process" do it 're-emits the deprecation warning' do - expect(LogStash::Settings.deprecation_logger).to receive(:deprecated).with(a_string_including(deprecated_setting_name)) settings.get_setting(deprecated_setting_name).observe_post_process + expect(log_spy.messages[0]).to include(deprecated_setting_name) end end @@ -117,15 +129,16 @@ end it 'does not produce a relevant deprecation warning' do - expect(LogStash::Settings.deprecation_logger).to_not have_received(:deprecated).with(a_string_including(deprecated_setting_name)) + settings.get_setting(deprecated_setting_name).observe_post_process + expect(log_spy.messages).to be_empty end include_examples '#validate_value success' context "#observe_post_process" do it 'does not emit a deprecation warning' do - expect(LogStash::Settings.deprecation_logger).to_not receive(:deprecated).with(a_string_including(deprecated_setting_name)) settings.get_setting(deprecated_setting_name).observe_post_process + expect(log_spy.messages).to be_empty end end end @@ -139,15 +152,15 @@ context '#validate_value' do it "raises helpful exception" do expect { settings.get_setting(canonical_setting_name).validate_value } - .to raise_exception(ArgumentError, a_string_including("Both `#{canonical_setting_name}` and its deprecated alias `#{deprecated_setting_name}` have been set. Please only set `#{canonical_setting_name}`")) + .to raise_exception(java.lang.IllegalStateException, a_string_including("Both `#{canonical_setting_name}` and its deprecated alias `#{deprecated_setting_name}` have been set. Please only set `#{canonical_setting_name}`")) end end end context 'Settings#get on deprecated alias' do it 'produces a WARN-level message to the logger' do - expect(LogStash::Settings.logger).to receive(:warn).with(a_string_including "setting `#{canonical_setting_name}` has been queried by its deprecated alias `#{deprecated_setting_name}`") settings.get(deprecated_setting_name) + expect(log_spy.messages[0]).to include("setting `#{canonical_setting_name}` has been queried by its deprecated alias `#{deprecated_setting_name}`") end end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index a6811fc90de..5403b41cf78 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -90,3 +90,40 @@ def puts(payload) def installed_plugins Gem::Specification.find_all.select { |spec| spec.metadata["logstash_plugin"] }.map { |plugin| plugin.name } end + + +def setup_logger_spy + java_import org.apache.logging.log4j.core.config.builder.api.ConfigurationBuilderFactory + java_import org.apache.logging.log4j.Level + config_builder = ConfigurationBuilderFactory.newConfigurationBuilder + configure_log_spy = config_builder + .add( + config_builder + .newAppender("LOG_SPY", "List") + .add(config_builder.newLayout("PatternLayout").addAttribute("pattern", "%-5p [%t]: %m%n")) + ) + .add( + config_builder + .newRootLogger(Level::INFO) + .add(config_builder.newAppenderRef("LOG_SPY"))) + .build(false) + + java_import org.apache.logging.log4j.core.config.Configurator + java_import org.apache.logging.log4j.core.config.Configuration + + java_import org.apache.logging.log4j.LogManager + java_import org.apache.logging.log4j.core.impl.Log4jContextFactory + # This is done because the LoggerExt calls setFactory LogstashLoggerContextFactory implements + # org.apache.logging.log4j.spi.LoggerContextFactory and doesn't extend org.apache.logging.log4j.core.impl.Log4jContextFactory + # which is the class expected by the following Configurator to use the programmatic configuration. + LogManager.setFactory(Log4jContextFactory.new) + log_ctx = Configurator.java_send(:initialize, [Configuration], configure_log_spy) + expect(log_ctx).not_to be nil + log_ctx.reconfigure(configure_log_spy) # force the programmatic configuration, without this it's not used + + return log_ctx +end + +def retrieve_logger_spy(log_ctx) + log_ctx.getConfiguration().getAppender("LOG_SPY") +end From 8ffcede0354f394c9cbb68849dc218f12258dc49 Mon Sep 17 00:00:00 2001 From: andsel Date: Tue, 29 Oct 2024 19:17:24 +0100 Subject: [PATCH 07/10] [test] Updated remaining Logstash core test to use StringSetting --- logstash-core/spec/logstash/queue_factory_spec.rb | 4 ++-- logstash-core/spec/logstash/settings_spec.rb | 12 ++++++------ 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/logstash-core/spec/logstash/queue_factory_spec.rb b/logstash-core/spec/logstash/queue_factory_spec.rb index b1d6547c54f..f4cc767edaf 100644 --- a/logstash-core/spec/logstash/queue_factory_spec.rb +++ b/logstash-core/spec/logstash/queue_factory_spec.rb @@ -23,7 +23,7 @@ let(:settings_array) do [ LogStash::Setting::WritableDirectory.new("path.queue", Stud::Temporary.pathname), - LogStash::Setting::String.new("queue.type", "memory", true, ["persisted", "memory"]), + LogStash::Setting::StringSetting.new("queue.type", "memory", true, ["persisted", "memory"]), LogStash::Setting::Bytes.new("queue.page_capacity", "8mb"), LogStash::Setting::Bytes.new("queue.max_bytes", "64mb"), LogStash::Setting::Numeric.new("queue.max_events", 0), @@ -31,7 +31,7 @@ LogStash::Setting::Numeric.new("queue.checkpoint.writes", 1024), LogStash::Setting::Numeric.new("queue.checkpoint.interval", 1000), LogStash::Setting::Boolean.new("queue.checkpoint.retry", false), - LogStash::Setting::String.new("pipeline.id", pipeline_id), + LogStash::Setting::StringSetting.new("pipeline.id", pipeline_id), LogStash::Setting::PositiveInteger.new("pipeline.batch.size", 125), LogStash::Setting::PositiveInteger.new("pipeline.workers", LogStash::Config::CpuCoreStrategy.maximum) ] diff --git a/logstash-core/spec/logstash/settings_spec.rb b/logstash-core/spec/logstash/settings_spec.rb index 31f67d38fcf..62c946ed132 100644 --- a/logstash-core/spec/logstash/settings_spec.rb +++ b/logstash-core/spec/logstash/settings_spec.rb @@ -154,8 +154,8 @@ settings.on_post_process do settings.set("baz", "bot") end - settings.register(LogStash::Setting::String.new("foo", "bar")) - settings.register(LogStash::Setting::String.new("baz", "somedefault")) + settings.register(LogStash::Setting::StringSetting.new("foo", "bar")) + settings.register(LogStash::Setting::StringSetting.new("baz", "somedefault")) settings.post_process end @@ -183,7 +183,7 @@ context "transient settings" do subject do settings = described_class.new - settings.register(LogStash::Setting::String.new("exist", "bonsoir")) + settings.register(LogStash::Setting::StringSetting.new("exist", "bonsoir")) settings end @@ -237,9 +237,9 @@ subject do settings = described_class.new - settings.register(LogStash::Setting::String.new("interpolated_env", "missing")) - settings.register(LogStash::Setting::String.new("with_dot_env", "missing")) - settings.register(LogStash::Setting::String.new("interpolated_store", "missing")) + settings.register(LogStash::Setting::StringSetting.new("interpolated_env", "missing")) + settings.register(LogStash::Setting::StringSetting.new("with_dot_env", "missing")) + settings.register(LogStash::Setting::StringSetting.new("interpolated_store", "missing")) settings end From 6f65bf0ecb6be450917bc9d01f8bfadf22f3d0d4 Mon Sep 17 00:00:00 2001 From: andsel Date: Tue, 5 Nov 2024 14:16:02 +0100 Subject: [PATCH 08/10] Force to use freach Log4j configuration on each test, in particaular when it closes the logger context --- logstash-core/spec/logstash/runner_spec.rb | 5 +-- .../setting_with_deprecated_alias_spec.rb | 24 +++++++++---- spec/spec_helper.rb | 35 ++++++++++--------- 3 files changed, 40 insertions(+), 24 deletions(-) diff --git a/logstash-core/spec/logstash/runner_spec.rb b/logstash-core/spec/logstash/runner_spec.rb index b0894edec5d..66e2652e247 100644 --- a/logstash-core/spec/logstash/runner_spec.rb +++ b/logstash-core/spec/logstash/runner_spec.rb @@ -263,8 +263,6 @@ let(:runner_deprecation_logger_stub) { double("DeprecationLogger(Runner)").as_null_object } before(:each) { allow(runner).to receive(:deprecation_logger).and_return(runner_deprecation_logger_stub) } - let(:configuration_spy) { configure_log_spy } - def configure_log_spy java_import org.apache.logging.log4j.core.config.builder.api.ConfigurationBuilderFactory java_import org.apache.logging.log4j.Level @@ -308,7 +306,10 @@ def configure_log_spy subject.run("bin/logstash", args) + expect(appender_spy.messages).not_to be_empty expect(appender_spy.messages[0]).to match(/`http.host` is a deprecated alias for `api.http.host`/) + + log_ctx.close end end diff --git a/logstash-core/spec/logstash/settings/setting_with_deprecated_alias_spec.rb b/logstash-core/spec/logstash/settings/setting_with_deprecated_alias_spec.rb index 3dc879824d7..168162ee088 100644 --- a/logstash-core/spec/logstash/settings/setting_with_deprecated_alias_spec.rb +++ b/logstash-core/spec/logstash/settings/setting_with_deprecated_alias_spec.rb @@ -27,22 +27,34 @@ let(:settings) { LogStash::Settings.new } let(:canonical_setting) { LogStash::Setting::StringSetting.new(canonical_setting_name, default_value, true) } - let(:log_ctx) { setup_logger_spy } - let(:log_spy) { retrieve_logger_spy(log_ctx) } + log_spy = nil + log_ctx = nil - before(:each) do - # Initialization of appender and logger use to spy, need to be done before executing any code that logs, - # that's the reason wy to refer the spying logger context before any test. - log_ctx + def log_ctx + @log_ctx + end + + def log_spy + @log_spy end before(:each) do + # Initialization of appender and logger use to spy, need to be freshly recreated on each test is context shutdown is used. + @log_ctx = setup_logger_spy + @log_spy = retrieve_logger_spy(@log_ctx) + allow(LogStash::Settings).to receive(:logger).and_return(double("SettingsLogger").as_null_object) allow(LogStash::Settings).to receive(:deprecation_logger).and_return(double("SettingsDeprecationLogger").as_null_object) settings.register(canonical_setting.with_deprecated_alias(deprecated_setting_name)) end + after(:each) do + @log_ctx.close + @log_spy = nil + @log_ctx = nil + end + shared_examples '#validate_value success' do context '#validate_value' do it "returns without raising" do diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 5403b41cf78..4e0800d3330 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -93,21 +93,6 @@ def installed_plugins def setup_logger_spy - java_import org.apache.logging.log4j.core.config.builder.api.ConfigurationBuilderFactory - java_import org.apache.logging.log4j.Level - config_builder = ConfigurationBuilderFactory.newConfigurationBuilder - configure_log_spy = config_builder - .add( - config_builder - .newAppender("LOG_SPY", "List") - .add(config_builder.newLayout("PatternLayout").addAttribute("pattern", "%-5p [%t]: %m%n")) - ) - .add( - config_builder - .newRootLogger(Level::INFO) - .add(config_builder.newAppenderRef("LOG_SPY"))) - .build(false) - java_import org.apache.logging.log4j.core.config.Configurator java_import org.apache.logging.log4j.core.config.Configuration @@ -121,7 +106,25 @@ def setup_logger_spy expect(log_ctx).not_to be nil log_ctx.reconfigure(configure_log_spy) # force the programmatic configuration, without this it's not used - return log_ctx + log_ctx +end + +def configure_log_spy + java_import org.apache.logging.log4j.core.config.builder.api.ConfigurationBuilderFactory + java_import org.apache.logging.log4j.Level + config_builder = ConfigurationBuilderFactory.newConfigurationBuilder + configuration = config_builder + .add( + config_builder + .newAppender("LOG_SPY", "List") + .add(config_builder.newLayout("PatternLayout").addAttribute("pattern", "%-5p [%t]: %m%n")) + ) + .add( + config_builder + .newRootLogger(Level::INFO) + .add(config_builder.newAppenderRef("LOG_SPY"))) + .build(false) + configuration end def retrieve_logger_spy(log_ctx) From d986c8161a8a4051a7734bb2c3c6159d716f5c46 Mon Sep 17 00:00:00 2001 From: andsel Date: Fri, 8 Nov 2024 12:18:07 +0100 Subject: [PATCH 09/10] Added debug logs to check why the spy doesn't work --- logstash-core/spec/logstash/runner_spec.rb | 20 +++++++++++ .../setting_with_deprecated_alias_spec.rb | 33 ++++++++++++++++--- .../logstash/settings/DeprecatedAlias.java | 11 +++++++ 3 files changed, 60 insertions(+), 4 deletions(-) diff --git a/logstash-core/spec/logstash/runner_spec.rb b/logstash-core/spec/logstash/runner_spec.rb index 66e2652e247..c9894746f05 100644 --- a/logstash-core/spec/logstash/runner_spec.rb +++ b/logstash-core/spec/logstash/runner_spec.rb @@ -263,6 +263,8 @@ let(:runner_deprecation_logger_stub) { double("DeprecationLogger(Runner)").as_null_object } before(:each) { allow(runner).to receive(:deprecation_logger).and_return(runner_deprecation_logger_stub) } + let(:configuration_spy) { configure_log_spy } + def configure_log_spy java_import org.apache.logging.log4j.core.config.builder.api.ConfigurationBuilderFactory java_import org.apache.logging.log4j.Level @@ -296,20 +298,38 @@ def configure_log_spy expect(log_ctx).not_to be nil log_ctx.reconfigure(configure_log_spy) # force the programmatic configuration, without this it's not used + # logger = log_ctx.getLogger("test_dna") + # Disabling the following the test doesn't receive any message, enabling it receives these and the expected log lines, but fails other tests with log spy + # logger.info("DNADBG%% Rspec test direct log") + appender_spy = log_ctx.getConfiguration().getAppender("LOG_SPY") + # logger = log_ctx.getLogger("test_dna") + # expect(logger.info_enabled?).to be_truthy + # logger.info("DNADBG>> Rspec test direct log") + # # puts "DNADBG>> log spy in before #{log_spy}" + # # puts "DNADBG>> all appenders #{@log_ctx.getConfiguration().getAppenders}" + # expect(appender_spy.messages[0]).to include("Rspec test direct log") + # appender_spy.clear + # expect(appender_spy.messages).to be_empty + expect(runner_deprecation_logger_stub).to receive(:deprecated).with(a_string_including 'The flag ["--http.host"] has been deprecated') expect(LogStash::Agent).to receive(:new) do |settings| expect(settings.set?("api.http.host")).to be(true) expect(settings.get("api.http.host")).to eq("localhost") + # settings.get_setting('http.host').observe_post_process end subject.run("bin/logstash", args) + #appender_spy.stop + # sleep 1 + # log_ctx.stop() expect(appender_spy.messages).not_to be_empty expect(appender_spy.messages[0]).to match(/`http.host` is a deprecated alias for `api.http.host`/) log_ctx.close + # LogManager.shutdown(log_ctx) end end diff --git a/logstash-core/spec/logstash/settings/setting_with_deprecated_alias_spec.rb b/logstash-core/spec/logstash/settings/setting_with_deprecated_alias_spec.rb index 168162ee088..5d28c59511b 100644 --- a/logstash-core/spec/logstash/settings/setting_with_deprecated_alias_spec.rb +++ b/logstash-core/spec/logstash/settings/setting_with_deprecated_alias_spec.rb @@ -27,8 +27,14 @@ let(:settings) { LogStash::Settings.new } let(:canonical_setting) { LogStash::Setting::StringSetting.new(canonical_setting_name, default_value, true) } - log_spy = nil - log_ctx = nil + # let(:log_ctx) { setup_logger_spy } + # let(:log_spy) { retrieve_logger_spy(log_ctx) } + # + # let!(:log_ctx) { setup_logger_spy } + # let!(:log_spy) { retrieve_logger_spy(log_ctx) } + + # log_spy = nil + # log_ctx = nil def log_ctx @log_ctx @@ -39,9 +45,26 @@ def log_spy end before(:each) do - # Initialization of appender and logger use to spy, need to be freshly recreated on each test is context shutdown is used. + puts "DNADBG>> before - outer" + + # Initialization of appender and logger use to spy, need to be done before executing any code that logs, + # that's the reason wy to refer the spying logger context before any test. + # log_ctx @log_ctx = setup_logger_spy @log_spy = retrieve_logger_spy(@log_ctx) + if @log_spy == nil || log_spy == nil + puts "DNADBG>> @log_spy: #{@log_spy}, log_spy: #{log_spy}" + end + expect(log_spy).not_to be nil + + # Verify the appender work properly + logger = log_ctx.getLogger("org.logstash.settings.DeprecatedAlias") + expect(logger.info_enabled?).to be_truthy + logger.info("DNADBG>> Rspec test direct log") + puts "DNADBG>> log spy in before #{log_spy}" + expect(log_spy.messages[0]).to include("Rspec test direct log") + log_spy.clear + expect(log_spy.messages).to be_empty allow(LogStash::Settings).to receive(:logger).and_return(double("SettingsLogger").as_null_object) allow(LogStash::Settings).to receive(:deprecation_logger).and_return(double("SettingsDeprecationLogger").as_null_object) @@ -50,7 +73,10 @@ def log_spy end after(:each) do + puts "DNADBG>> after - outer all appenders #{@log_ctx.getConfiguration().getAppenders}" @log_ctx.close + # java_import org.apache.logging.log4j.LogManager + # LogManager.shutdown() @log_spy = nil @log_ctx = nil end @@ -78,7 +104,6 @@ def log_spy it 'does not emit a deprecation warning' do expect(LogStash::Settings.deprecation_logger).to_not receive(:deprecated).with(a_string_including(deprecated_setting_name)) settings.get_setting(deprecated_setting_name).observe_post_process - log_spy = retrieve_logger_spy(log_ctx) expect(log_spy.messages).to be_empty end end diff --git a/logstash-core/src/main/java/org/logstash/settings/DeprecatedAlias.java b/logstash-core/src/main/java/org/logstash/settings/DeprecatedAlias.java index d2b8eac17e3..d85cc8ff090 100644 --- a/logstash-core/src/main/java/org/logstash/settings/DeprecatedAlias.java +++ b/logstash-core/src/main/java/org/logstash/settings/DeprecatedAlias.java @@ -22,6 +22,8 @@ import co.elastic.logstash.api.DeprecationLogger; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +import org.apache.logging.log4j.core.Appender; +import org.apache.logging.log4j.core.LoggerContext; import org.logstash.log.DefaultDeprecationLogger; /** @@ -44,8 +46,14 @@ public final class DeprecatedAlias extends SettingDelegator { // check https://github.com/elastic/logstash/pull/16339 public void observePostProcess() { if (isSet()) { + System.out.println("DNADBG>> observePostProcess invoked on DeprecatedAlias: " + getName()); DEPRECATION_LOGGER.deprecated("The setting `{}` is a deprecated alias for `{}` and will be removed in a " + "future release of Logstash. Please use `{}` instead", getName(), canonicalProxy.getName(), canonicalProxy.getName()); +// LOGGER.info("DNADBG DeprecatedAlias test from LOGGER"); +// LogManager.getLogger().info("DNADBG DeprecatedAlias test from direct logger"); + + Appender spy = (LoggerContext.getContext(false)).getConfiguration().getAppender("LOG_SPY"); + System.out.println("DNADBG>> spy from DeprecatedAlias.observePostProcess is: " + spy); } } @@ -53,6 +61,9 @@ public void observePostProcess() { public T value() { LOGGER.warn("The value of setting `{}` has been queried by its deprecated alias `{}`. " + "Code should be updated to query `{}` instead", canonicalProxy.getName(), getName(), canonicalProxy.getName()); + System.out.println("DNADBG>> spy from DeprecatedAlias.value() of " + getName()); + Appender spy = (LoggerContext.getContext(false)).getConfiguration().getAppender("LOG_SPY"); + System.out.println("DNADBG>> spy from DeprecatedAlias.value is: " + spy); return super.value(); } From 8a14684472aedba107e329786c1c7835e24284e3 Mon Sep 17 00:00:00 2001 From: andsel Date: Fri, 8 Nov 2024 12:49:53 +0100 Subject: [PATCH 10/10] Removed commented code --- .../settings/setting_with_deprecated_alias_spec.rb | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/logstash-core/spec/logstash/settings/setting_with_deprecated_alias_spec.rb b/logstash-core/spec/logstash/settings/setting_with_deprecated_alias_spec.rb index 5d28c59511b..1e6d829b743 100644 --- a/logstash-core/spec/logstash/settings/setting_with_deprecated_alias_spec.rb +++ b/logstash-core/spec/logstash/settings/setting_with_deprecated_alias_spec.rb @@ -27,15 +27,6 @@ let(:settings) { LogStash::Settings.new } let(:canonical_setting) { LogStash::Setting::StringSetting.new(canonical_setting_name, default_value, true) } - # let(:log_ctx) { setup_logger_spy } - # let(:log_spy) { retrieve_logger_spy(log_ctx) } - # - # let!(:log_ctx) { setup_logger_spy } - # let!(:log_spy) { retrieve_logger_spy(log_ctx) } - - # log_spy = nil - # log_ctx = nil - def log_ctx @log_ctx end @@ -49,7 +40,6 @@ def log_spy # Initialization of appender and logger use to spy, need to be done before executing any code that logs, # that's the reason wy to refer the spying logger context before any test. - # log_ctx @log_ctx = setup_logger_spy @log_spy = retrieve_logger_spy(@log_ctx) if @log_spy == nil || log_spy == nil