Standardised SSL settings

[edmocosta]

What this PR does?

• Standardized SSL settings
  • Deprecated cacert in favor of ssl_certificate_authorities
  • Deprecatedclient_cert in favor of ssl_certificate
  • Deprecated client_key in favor of ssl_key
  • Deprecated keystore in favor of ssl_keystore_path
  • Deprecated keystore_password in favor of ssl_keystore_password
  • Deprecated keystore_type in favor of ssl_keystore_type
  • Deprecated truststore in favor of ssl_truststore_path
  • Deprecated truststore_password in favor of ssl_truststore_password
  • Deprecated truststore_type in favor of ssl_truststore_type
• Added a module configuration to disable the deprecated SSL configs :with_deprecated
• Added the ssl_cipher_suites option

---

Closes elastic/logstash#15191

[yaauie]

I really appreciate how you provided an adapter for the backward-compatibility to reduce noise from the core options. That separation will make it easy for new plugins to rely on this mixin without accumulating already-deprecated options.

I like this approach, but I think that it may need to change slightly.

As of Logstash 8.9.0 we currently have three logstash-bundled plugins that depend on this mixin, and unfortunately two of them are defined in a way that does not prevent them from resolving to version 8.x of this plugin:

• logstash-filter-http (1.4.3) requires logstash-mixin-http_client (>= 7.2.0, < 9.0.0)
• logstash-input-http_poller (5.4.0) requires logstash-mixin-http_client (>= 7.2.0)
• logstash-output-http (5.5.0) requires logstash-mixin-http_client (>= 7.2.0, < 8.0.0)

This means that as soon as we release a logstash-output-http that is capable of using 8.x of this mixin, upgrading it on an existing Logstash installation would irreversibly break the others -- we don't have a way to "downgrade" non-plugin dependencies.

It also means that all three have to cross the 7.x-8.x boundary of this mixin together.

I think we can achieve the separation you were going for without breaking changes:

• Move bulk of functionality to new private module LogStash::PluginMixins::HttpClient::Implementation including standardized options, the client building, etc.
• Add new private module LogStash::PluginMixins::HttpClient::StandardizedOnly whose self.included(base) only includes the Implementation module into the provided base
• Add new private module LogStash::PluginMixins::HttpClient::StandardizedAndDeprecated whose self.included(base) includes the Implementation module and the deprecation support into the provided base, activating it if necessary.
• Add a new public method LogStash::PluginMixins::HttpClient(with_deprecated:) as the new public API for including this mixin that returns either StandardizedAndDeprecated or StandardizedOnly so that:
  • include LogStash::PluginMixins::HttpClient(with_deprecated: true) causes StandardizedAndDeprecated to be included, and
  • include LogStash::PluginMixins::HttpClient(with_deprecated: false) causes StandardizedOnly to be included
• Make the LogStash::PluginMixins::HttpClient#included(base) emit a deprecation warning before including StandardizedAndDeprecated into the provided base to ensure that currently-released plugins that include the module directly continue to support all of the options they already support.

This approach would ensure that:

• any released version of a plugin that does an include LogStash::PluginMixins::HttpClient will continue to get support for all of the now-deprecated options from previous releases of this mixin, plus the new standardized options and all of the deprecation handling
• new plugins can opt directly into the standardized options without bearing the burden of the deprecated options that they never needed
• at some point we can deprecate the legacy pre-standardized options by adding the deprecation log to StandardizedAndDeprecated#included, providing a path for its eventual removal.
• as a non-breaking change-set, we can keep the 7.x for this mixin ad eliminate the complexity of upgrading plugins that rely on it.

[edmocosta]

Hi @yaauie!

Thank you very much for your detailed review, I really appreciated it. I've pushed the changes I made based on your suggestions. I still testing it, but I think it's good enough to be reviewed.

The main differences are:

• I ended up not changing the LogStash::PluginMixins::HttpClient module, instead, I've moved only the SSL configs, splitting them into two different modules SslDeprecatedConfigs and SslStandardizedConfigs.

  The SslDeprecatedConfigs register all deprecated settings plus the normalize_http_client_deprecated_configs! method, it's included by default when the LogStash::PluginMixins::HttpClient is imported.

  The SslStandardizedConfigs register all standardized settings, and is not include by default. That way, the plugins already using this module won't include the standardized settings without normalizing them (normalize_http_client_deprecated_configs!). Having those new configs without the normalization would allow users to provide ambiguous configuration (e.g. ssl_certificate and client_cert), and the plugins wouldn't validate it.

• The modules' configuration are done on the LogStash::PluginMixins::HttpClient[...] inclusion, e.g:

   include LogStash::PluginMixins::HttpClient[:ssl_standardized_options => true] 
   include LogStash::PluginMixins::HttpClient[:ssl_standardized_options => true, ssl_deprecated_options => false]

  This allows plugins to enable all or disable at maximum one SSL options. When a plugin decides to remove the deprecated settings, it only needs to change the module parameters and remove the normalization method invocation.
  Please let me know if this approach is OK, I'd be glad to change it to any other suggestion you might have.

• Considering the default behaviour is using deprecated settings, the SslDeprecatedConfigs is not logging any deprecation warnings yet, so the existing plugins won't be affected by the deprecation logs.

[yaauie]

• Considering the default behaviour is using deprecated settings, the SslDeprecatedConfigs is not logging any deprecation warnings yet, so the existing plugins won't be affected by the deprecation logs.

The deprecated settings are all flagged with deprecated and relevant guidance about the standardized counterparts, which is not relevant to a plugin that does not have the other settings yet. This is likely because in order to use the normalization support adapter, they must be deprecated.

I don't think that there is harm in a plugin getting the standardized settings "for free" (along with the deprecation of the old bespoke ones), and there is a lot of overhead in implementing this in such a way that the availability of standardized settings is entirely opt-in by the developer. For example, making this implementation conditionally flag the bespoke options as deprecated based on the availability of the standardized counterparts would be complex.

The way I see API stability is that a plugin that is configured with any combination of the bespoke non-standardized settings should continue to operate exactly as it does at runtime. I don't see the logging of a deprecation warning or the acceptance of an alternate to be a breaking change. Therefore, from my perspective we should have:

• include HttpClient[with_deprecated: false]: explicitly include only standardized (for new plugins)
• include HttpClient[with_deprecated: true]: explicitly include standardized + deprecated mappings (for existing plugins)
• include HttpClient directly (deprecated path to standardized + deprecated), ensures no changes necessary in downstream code-bases.

Using your code, I spiked a reference implementation in #43 that does not port your tests or other validation, but largely follows the guidance from my first review borrowing heavily from your PR.

---

I would also like to consider pulling ssl_enabled from the requirements because its use is a consumer concern; there is no way for this plugin to use ssl_enabled => true as a safeguard as we do in other plugins.

[edmocosta]

I don't think that there is harm in a plugin getting the standardized settings "for free" (along with the deprecation of the old bespoke ones), and there is a lot of overhead in implementing this in such a way that the availability of standardized settings is entirely opt-in by the developer. For example, making this implementation conditionally flag the bespoke options as deprecated based on the availability of the standardized counterparts would be complex.

In my opinion, providing the standardized settings is just half of the standardization process. If we don't validate the settings, the plugin would accept configurations like this one, which IMO is ambiguous and confusing:

client_cert => "path/to/client_cert
ssl_certificate => "path/to/ssl_certificate"

Edit: Just saw how you implemented it on the spike PR. It looks good to me :)

[yaauie]

Largely on-track. Certainly the interface is stable enough now to be able to draft PR's for the plugins that rely on it.

I would prefer to not inject the normalized values into params unless we truly have a reason for doing so (since we already guarantee that we set the relevant ivars, which have accessors).

Other minor nitpicks and fixes in-line.

[yaauie]

Likely should file a separate issue, but :strict is deprecated and the poorly-named :default is the modern RFC-compliant strict verifier (see: logstash-plugins/logstash-output-elasticsearch#1139)

[yaauie]

LGTM 🎉

Thank you for working with my complicated and poorly-communicated ideas on this one. It took some iteration, but I'm confident we now have a non-breaking approach that the plugins that use this mixin can get "for free" once this mixin is upgraded. Now, if only the docs for all of these options could share a common source-of-truth 😂