Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reimplemented LogStash::String setting in Java #16576

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
40 changes: 20 additions & 20 deletions logstash-core/lib/logstash/environment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand All @@ -50,10 +50,10 @@ 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::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),
Expand All @@ -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::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::String.new("api.auth.type", "none", true, %w(none basic)),
Setting::String.new("api.auth.basic.username", nil, false).nullable,
Setting::StringSetting.new("api.environment", "production").with_deprecated_alias("http.environment"),
Setting::StringSetting.new("api.auth.type", "none", true, %w(none basic)),
Setting::StringSetting.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"),
Expand All @@ -102,16 +102,16 @@ 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"),
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::String.new("keystore.file", ::File.join(::File.join(LogStash::Environment::LOGSTASH_HOME, "config"), "logstash.keystore"), false), # will be populated on
Setting::StringSetting.new("keystore.classname", "org.logstash.secret.store.backend.JavaKeyStore"),
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::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) }

Expand Down
5 changes: 5 additions & 0 deletions logstash-core/lib/logstash/settings.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -824,6 +827,8 @@ def validate(value)
end
end

java_import org.logstash.settings.NullableSetting

# @see Setting#nullable
# @api internal
class Nullable < SimpleDelegator
Expand Down
4 changes: 2 additions & 2 deletions logstash-core/lib/logstash/util/settings_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions logstash-core/spec/logstash/queue_factory_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,15 @@
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),
LogStash::Setting::Numeric.new("queue.checkpoint.acks", 1024),
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)
]
Expand Down
37 changes: 36 additions & 1 deletion logstash-core/spec/logstash/runner_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -263,17 +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")
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

Expand Down
8 changes: 4 additions & 4 deletions logstash-core/spec/logstash/settings/nullable_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand All @@ -66,22 +77,23 @@

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
expect(settings.get(canonical_setting_name)).to eq(value)
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

Expand Down Expand Up @@ -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
Expand All @@ -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
12 changes: 6 additions & 6 deletions logstash-core/spec/logstash/settings_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ public void reset() {
}

public void validateValue() {
validate(this.value);
validate(this.value());
}

public T getDefault() {
Expand Down
Loading