Make config values more strictly and enable to set nil or default #2685

ashie · 2019-11-06T08:11:24Z

Which issue(s) this PR fixes:
Fixes #2616

What this PR does / why we need it:
Add a new command line option --strict-config-value to parse integer, float and bool values in a config file more strictly. A system config item strict_config_value is also added.
Since non-numerical (or unrecognized values for bool) values for them in config values are implicitly parsed as 0 , sometimes it makes troubleshooting hard. This option will raise an error for such values to clarify such issues in a config file.

In addition, this PR also adds helper methods to set nil or default value to parameters in config files, since many users expect nil for "#{ENV['SOME_ENV_VAR']}" with empty SOME_ENV_VAR environment variable (but it's converted to an empty string in actual).

some_param "#{ENV['SOME_ENV_VAR'] || use_nil}"

some_param "#{ENV['SOME_ENV_VAR'] || use_default}"

Docs Changes:

Add --strict-config-value to Command Line Options in Deployment - System Configuration.
Add strict_config_value to system config in Configuration - Config File Syntax and Deployment - System Configuration.
Add note for use_nil and use_default to Embedding Ruby Expressions in Config File Syntax.

Release Note:
Same with the title.

ashie · 2019-11-06T08:11:43Z

This PR adds a new option :strict to config_param to confirm the
concept. Now I've confirmed that it can address referred issues such as
#2607, #2616 and #2444 when I merge this option to all plugins by
default (it's not commited yet). But I think it's not a thing you want.
A global option is the required one, isn't it?

My first question is "where should I add the global option"?

environment variable
command line option
system config

I think the command line option is the desired place.
What do you think about it?

The second question is: "The :strict option for config_param is usable?"
If it's not, I don't expose it to plugins and just switch the behavior
internally. If it's usable, I'll continue to implement this feature
(more tests are needed).

I have some more questions. I'll post it later.

cosmo0920 · 2019-11-06T08:40:27Z

My first question is "where should I add the global option"?

environment variable

command line option

system config

IMO, 3 would be ideal. 2 would be better.

The second question is: "The :strict option for config_param is usable?"

Looks workable in my environment:

diff --git a/lib/fluent/plugin/in_udp.rb b/lib/fluent/plugin/in_udp.rb
index c2d43611..ac3d0bc3 100644
--- a/lib/fluent/plugin/in_udp.rb
+++ b/lib/fluent/plugin/in_udp.rb
@@ -25,7 +25,7 @@ module Fluent::Plugin
     desc 'Tag of output events.'
     config_param :tag, :string
     desc 'The port to listen to.'
-    config_param :port, :integer, default: 5160
+    config_param :port, :integer, default: 5160, strict: true
     desc 'The bind address to listen to.'
     config_param :bind, :string, default: '0.0.0.0'

<source>
  @type udp
  format none
  bind 0.0.0.0
  port invalid
  body_size_limit 4KB
  source_host_key "host"
  tag test
</source>

<match test>
  @type stdout
</match>

2019-11-06 17:40:11 +0900 [info]: parsing config file is succeeded path="example/in_udp.conf"
2019-11-06 17:40:11 +0900 [info]: gem 'fluentd' version '1.8.0.rc2'
2019-11-06 17:40:11 +0900 [info]: gem 'fluent-plugin-elasticsearch' version '3.5.5'
2019-11-06 17:40:11 +0900 [info]: gem 'fluent-plugin-kinesis' version '3.1.0'
2019-11-06 17:40:11 +0900 [info]: gem 'fluent-plugin-prometheus' version '1.6.0'
bundler: failed to load command: fluentd (/Users/cosmo/GitHub/fluentd/vendor/bundle/ruby/2.6.0/bin/fluentd)
ArgumentError: invalid value for Integer(): "invalid"

And it would be preferable just adding strict checking.

lib/fluent/config/types.rb

test/config/test_types.rb

cosmo0920 · 2019-11-06T08:50:45Z

I've commented the my first impression.
Please take a look, @ashie.

repeatedly · 2019-11-07T08:10:10Z

I think :strict option in config_param is not good because it is unclear behaviour for users.
So command line option and system configuration are better.

lib/fluent/config/error.rb

lib/fluent/config/literal_parser.rb

lib/fluent/config/section.rb

test/config/test_types.rb

lib/fluent/config/types.rb

lib/fluent/config/literal_parser.rb

lib/fluent/config/types.rb

ashie · 2019-12-03T03:47:28Z

Oops, sorry I missed some comments.
I'll address them.

lib/fluent/config/element.rb

lib/fluent/config/section.rb

lib/fluent/config/element.rb

We should ensure to put only 1 assertion in 1 test because an failed assertion stops to continue the test, remaining assertions in it aren't evaluated. It's not convenient on modifying existing features. Signed-off-by: Takuro Ashie <ashie@clear-code.com>

Add a new config option :strict to Config::INTERGER_VALUE and Config::FLOAT_VALUE. Config.size_value and Config.time_value also support it. Signed-off-by: Takuro Ashie <ashie@clear-code.com>

Example config: <system> strict_config_value true </system> When this option is set to true, ambiguous values for config parameters aren't accepted. For example, an empty string or not a number like "abc" aren't accepted as an integer or a float, an exception will be raised instead. Signed-off-by: Takuro Ashie <ashie@clear-code.com>

Signed-off-by: Takuro Ashie <ashie@clear-code.com>

Should raise ConfigError on unrecognized strings or any other value types to notify mistakes in user's config as early as possible. Signed-off-by: Takuro Ashie <ashie@clear-code.com>

Example setting: username: "#{ENV['USERNAME'] || raise(SetNil)}" Although many users expect nil for "#{ENV{'FOOBAR']}" with empty FOOBAR environemnt variable, it's converted to an empty string "" since it's Ruby's specification. Instead we are going to add a helper method for it. The helper method will be added in a later commit. Signed-off-by: Takuro Ashie <ashie@clear-code.com>

Example setting: username: "#{ENV['USERNAME'] || raise(SetDefault)}" Signed-off-by: Takuro Ashie <ashie@clear-code.com>

Example settings: username: "#{ENV['USERNAME'] || use_nil}" or username: "#{ENV['USERNAME'] || use_default}" Signed-off-by: Takuro Ashie <ashie@clear-code.com>

Signed-off-by: Takuro Ashie <ashie@clear-code.com>

The new argument `strict_config_value` should be added to the last of arguments of the method. Signed-off-by: Takuro Ashie <ashie@clear-code.com>

Signed-off-by: Takuro Ashie <ashie@clear-code.com>

Default config values should be taken from ConfigProxy::defaults. Otherwise a default value overwritten by config_set_default isn't taken correctly. Signed-off-by: Takuro Ashie <ashie@clear-code.com>

Signed-off-by: Takuro Ashie <ashie@clear-code.com>

Since the first argument inherits Hash class, it's confusing. Signed-off-by: Takuro Ashie <ashie@clear-code.com>

repeatedly · 2019-12-11T07:06:43Z

Looks good for me. Does anyone have other concerns?

ashie · 2019-12-11T07:23:03Z

Thanks for your review!
It's completed, I have no additional patch for this PR.
I'll add description for this feature to fluent-docs if there is no objection to its specificaion.

cosmo0920 · 2019-12-11T07:39:35Z

Looks good for me, too. I have no concern for these patches.

repeatedly · 2019-12-12T01:28:30Z

Merged. Thanks!

cosmo0920 reviewed Nov 6, 2019

View reviewed changes

lib/fluent/config/types.rb Outdated Show resolved Hide resolved

lib/fluent/config/types.rb Outdated Show resolved Hide resolved

test/config/test_types.rb Show resolved Hide resolved

test/config/test_types.rb Show resolved Hide resolved

ashie force-pushed the strict-config-value branch 8 times, most recently from 684f9d3 to dfff109 Compare November 14, 2019 04:04

ashie mentioned this pull request Nov 14, 2019

Add option to make config value more stricitive #2616

Closed

ashie force-pushed the strict-config-value branch from dfff109 to 7b2e9ec Compare November 20, 2019 06:28

ashie changed the title ~~Make config values more strictive~~ Make config values more strictly Nov 25, 2019

ashie changed the title ~~Make config values more strictly~~ Make config values more strictly and enable to set nil or default Nov 25, 2019

ashie marked this pull request as ready for review November 25, 2019 08:59

ganmacs reviewed Nov 25, 2019

View reviewed changes

ashie force-pushed the strict-config-value branch 2 times, most recently from e07c550 to 23761f0 Compare December 3, 2019 02:37

ganmacs reviewed Dec 3, 2019

View reviewed changes

lib/fluent/config/element.rb Outdated Show resolved Hide resolved

lib/fluent/config/section.rb Outdated Show resolved Hide resolved

lib/fluent/config/element.rb Outdated Show resolved Hide resolved

ashie added 7 commits December 5, 2019 11:06

Add an ability to parse integer & float values more strictly

262d3b9

Add a new config option :strict to Config::INTERGER_VALUE and Config::FLOAT_VALUE. Config.size_value and Config.time_value also support it. Signed-off-by: Takuro Ashie <ashie@clear-code.com>

Raise more friendly error on strict_config_value = true

60f11b7

Signed-off-by: Takuro Ashie <ashie@clear-code.com>

Add a command line option "--strict-config-value"

2b55be3

Signed-off-by: Takuro Ashie <ashie@clear-code.com>

Parse config values strictly also for system config

4204e1c

Signed-off-by: Takuro Ashie <ashie@clear-code.com>

Also parse bool value strictly on strict_config_value = true

208f7ff

Should raise ConfigError on unrecognized strings or any other value types to notify mistakes in user's config as early as possible. Signed-off-by: Takuro Ashie <ashie@clear-code.com>

ashie added 14 commits December 5, 2019 11:24

Now embedded ruby code in config file can set default value

1fdb92d

Example setting: username: "#{ENV['USERNAME'] || raise(SetDefault)}" Signed-off-by: Takuro Ashie <ashie@clear-code.com>

Add helper methods to set nil or default in config file

06e55c5

Example settings: username: "#{ENV['USERNAME'] || use_nil}" or username: "#{ENV['USERNAME'] || use_default}" Signed-off-by: Takuro Ashie <ashie@clear-code.com>

Narrow the scope of begin - rescue to clarify what raises the errors

134990d

Signed-off-by: Takuro Ashie <ashie@clear-code.com>

SetNil and SetDefault should inherit Exception instead of ConfigError

56291e1

Signed-off-by: Takuro Ashie <ashie@clear-code.com>

Keep backward compatibility of SectionGenerator::generate

e0b641c

The new argument `strict_config_value` should be added to the last of arguments of the method. Signed-off-by: Takuro Ashie <ashie@clear-code.com>

Enable to dump a config which includes nil or :default

787d5fe

Signed-off-by: Takuro Ashie <ashie@clear-code.com>

Show more friendly message on ConfigError

64b4672

Signed-off-by: Takuro Ashie <ashie@clear-code.com>

Add more tests for setting nil or :default to Configurable

9238ffe

Signed-off-by: Takuro Ashie <ashie@clear-code.com>

Set correct default config value on the helper method "use_default"

d111922

Default config values should be taken from ConfigProxy::defaults. Otherwise a default value overwritten by config_set_default isn't taken correctly. Signed-off-by: Takuro Ashie <ashie@clear-code.com>

Enable use_nil and use_default also for config_argument

e35d4cc

Signed-off-by: Takuro Ashie <ashie@clear-code.com>

Use single quote

f1e1353

Signed-off-by: Takuro Ashie <ashie@clear-code.com>

strict_config_value should also affect to SystemConfig itself

45adea3

Signed-off-by: Takuro Ashie <ashie@clear-code.com>

Show correct default value on dumping a config element

f13d48b

Signed-off-by: Takuro Ashie <ashie@clear-code.com>

ashie force-pushed the strict-config-value branch from 0c4b8a9 to f13d48b Compare December 5, 2019 03:25

Don't use keyword argument for Configurable::configure

3ae212b

Since the first argument inherits Hash class, it's confusing. Signed-off-by: Takuro Ashie <ashie@clear-code.com>

ashie force-pushed the strict-config-value branch from 6e187f7 to 3ae212b Compare December 5, 2019 06:40

repeatedly mentioned this pull request Dec 11, 2019

Add option to pass index template for ES fluent/fluentd-kubernetes-daemonset#380

Merged

repeatedly merged commit 340d1aa into fluent:master Dec 12, 2019

ashie mentioned this pull request Dec 13, 2019

Add a new config option and helper methods fluent/fluentd-docs-gitbook#128

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make config values more strictly and enable to set nil or default #2685

Make config values more strictly and enable to set nil or default #2685

ashie commented Nov 6, 2019 •

edited

Loading

ashie commented Nov 6, 2019

cosmo0920 commented Nov 6, 2019 •

edited

Loading

cosmo0920 commented Nov 6, 2019

repeatedly commented Nov 7, 2019

ashie commented Dec 3, 2019

repeatedly commented Dec 11, 2019

ashie commented Dec 11, 2019

cosmo0920 commented Dec 11, 2019 •

edited

Loading

repeatedly commented Dec 12, 2019

Make config values more strictly and enable to set nil or default #2685

Make config values more strictly and enable to set nil or default #2685

Conversation

ashie commented Nov 6, 2019 • edited Loading

ashie commented Nov 6, 2019

cosmo0920 commented Nov 6, 2019 • edited Loading

cosmo0920 commented Nov 6, 2019

repeatedly commented Nov 7, 2019

ashie commented Dec 3, 2019

repeatedly commented Dec 11, 2019

ashie commented Dec 11, 2019

cosmo0920 commented Dec 11, 2019 • edited Loading

repeatedly commented Dec 12, 2019

ashie commented Nov 6, 2019 •

edited

Loading

cosmo0920 commented Nov 6, 2019 •

edited

Loading

cosmo0920 commented Dec 11, 2019 •

edited

Loading