Remove deprecated SSL settings and simplify SSL configuration #213
Conversation
|
Got a failure on 7.x https://app.travis-ci.com/github/logstash-plugins/logstash-input-elasticsearch/jobs/628037127 that looked unrelated and suspiciously like a race condition. I reloaded that cell. |
|
For reviewer: I found reviewing this work to be very helpful in making sense of the removals https://github.com/logstash-plugins/logstash-input-elasticsearch/pull/185/files |
lib/logstash/inputs/elasticsearch.rb
Outdated
| @@ -404,10 +393,9 @@ def validate_authentication | |||
| def setup_client_ssl | |||
| ssl_options = {} | |||
| ssl_options[:ssl] = true if @ssl_enabled | |||
| ssl_options[:trust_strategy] = trust_strategy_for_ca_trusted_fingerprint | |||
There was a problem hiding this comment.
oops, i should only set this if @ssl_enabled. I'll move that back to where it was before.
donoghuc
force-pushed
the
GH-210-remove-obsolet-ssl-settings
branch
from
3f3d572
to
cde2f75
Compare
| @@ -198,23 +198,12 @@ class LogStash::Inputs::Elasticsearch < LogStash::Inputs::Base | |||
| # Set the address of a forward HTTP proxy. | |||
| config :proxy, :validate => :uri_or_empty | |||
|
|
|||
| # SSL | |||
There was a problem hiding this comment.
Rather than remove, let's mark as obsolete. We typically go from deprecated -> obsolete -> final removal
Marking a setting as obsolete triggers a specific error message in Logstash that prevents Logstash from starting, but gives an informative error message to the user:
There was a problem hiding this comment.
ah, I totally missed that. Will get this fixed up!
|
TODO:
|
| @@ -52,6 +52,19 @@ | |||
| end | |||
| end | |||
|
|
|||
| describe 'handling obsolete settings' do | |||
There was a problem hiding this comment.
Inspired by tests in logstash-plugins/logstash-input-http#182 credit @robbavey
donoghuc
force-pushed
the
GH-210-remove-obsolet-ssl-settings
branch
3 times, most recently
from
c74627d
to
6caee6d
Compare
lib/logstash/inputs/elasticsearch.rb
Outdated
| @@ -264,6 +253,11 @@ class LogStash::Inputs::Elasticsearch < LogStash::Inputs::Base | |||
| # If set, the _source of each hit will be added nested under the target instead of at the top-level | |||
| config :target, :validate => :field_reference | |||
|
|
|||
| # Obsolete Settings | |||
| config :ssl, :validate => :boolean, :default => false, :obsolete => "Set 'ssl_enabled' instead." | |||
There was a problem hiding this comment.
Thinking about this, we can probably skip the :validate and :default parts, and stick with :obsolete to avoid users fixing up settings, only to find they were obsolete anyway. WDYT?
(My only caveat is to keep :password and :uri (not applicable here), as they are considered secret, and are obfuscated on discovery)
There was a problem hiding this comment.
Nice, yeah i got tripped up on this writing tests. I was surprised to find that :validate would trigger when the option is :obsolete. I may be misunderstanding the test scenario but it looks to me like as a user I may be frustrated with a case whereby i set the wrong thing, then get a validation error that its the wrong form, only to be rewarded with a "this is no longer a valid option" once I satisfy the input validation. Not a huge deal but certainly a surprise if that is the case.
donoghuc
force-pushed
the
GH-210-remove-obsolet-ssl-settings
branch
2 times, most recently
from
57654c7
to
9b1f74c
Compare
docs/index.asciidoc
Outdated
| @@ -608,12 +610,12 @@ option when authenticating to the Elasticsearch server. If set to an | |||
| empty string authentication will be disabled. | |||
|
|
|||
|
|
|||
| [id="plugins-{type}s-{plugin}-deprecated-options"] | |||
| ==== Elasticsearch Input deprecated configuration options | |||
| [id="plugins-{type}s-{plugin}-removed-options"] | |||
There was a problem hiding this comment.
For the docs update, let's follow the example in logstash-plugins/logstash-output-http@2fae93e#diff-cae5619b3d18ec99c5ccd0a9f6de0c6d3f53343c64692444551a7d29da6863e7
ie, remove all inline references to deprecated settings, and add a dedicated section at the bottom, above "common options", with a link to that section just before the config table.
cc @karenzone
This commit updates SSL settings to be marked as obsolete: - Replace `ssl` with `ssl_enabled` - Replace `ca_file` with `ssl_certificate_authorities` - Replace `ssl_certificate_verification` with `ssl_verification_mode` `setup_ssl_params!` has been updated to only handle SSL inference when not explicitly configured. All changes have been updated in tests and in docs. The preparation for releasing a new major version is also included.
This commit updates the documentation to follow the pattern established in logstash-plugins/logstash-output-http#147 for documenting obsolete options.
donoghuc
force-pushed
the
GH-210-remove-obsolet-ssl-settings
branch
from
9b1f74c
to
c2bd0e6
Compare
|
Rebased and updated documentation to match logstash-plugins/logstash-output-http#147 |
|
Corresponding docs PR elastic/logstash#16744 in LS |
Get rid of the deprecated parameters and document their removal. Ensure the integration and unit tests pass and that the removals are clearly documented.
Closes #210