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

Feat: review and deprecate ssl protocol/cipher settings #450

Merged
merged 7 commits into from
Apr 28, 2022

Conversation

kares
Copy link
Contributor

@kares kares commented Apr 19, 2022

... same as logstash-plugins/logstash-input-http#151

(a follow-up on #447)

  • deprecating cipher_suites in favor of ssl_cipher_suites (with better validation of the supported cipher set)
  • adding ssl_supported_protocols as a replacement for tls_min_version / tls_max_version

@kares kares marked this pull request as ready for review April 27, 2022 09:51
@kares kares requested a review from karenzone April 27, 2022 09:51
@kares kares mentioned this pull request Apr 27, 2022
40 tasks
@kares kares requested a review from andsel April 27, 2022 11:29
Copy link
Contributor

@andsel andsel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

docs/index.asciidoc Outdated Show resolved Hide resolved
Co-authored-by: Karen Metts <35154725+karenzone@users.noreply.github.com>
Copy link
Contributor

@karenzone karenzone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments inline. Otherwise, LGTM!

Comment on lines +217 to +224
* Value type is <<string,string>>
* Supported values are:
** `disabled`: unstructured connection metadata added at root level
** `v1`: structured connection metadata added under ECS v1 compliant namespaces
** `v8`: structured connection metadata added under ECS v8 compliant namespaces
* Default value depends on which version of Logstash is running:
** When Logstash provides a `pipeline.ecs_compatibility` setting, its value is used as the default
** Otherwise, the default value is `disabled`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The asciidoc formatting is correct as it was. You don't even have to add the leading space to the first level bullets. Asciidoc automatically indents them for you.
The second level bullets are tagged ** and should be aligned with the left margin.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yy it just read weird - the rest of the documented options had the whitespaces to aid while reading the source

Copy link
Contributor

@karenzone karenzone Apr 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you like the leading spaces before the first level bullets, I don't have a problem with that. They won't hurt anything, and as you point out, it's consistent with other options.

Asciidoc standard is to left align all bullets. I like following the standards because otherwise, people see things done differently and a new "pseudo standard" is born. Ultimately, you can decide.

@@ -175,8 +175,9 @@ This plugin supports the following configuration options plus the <<plugins-{typ
| <<plugins-{type}s-{plugin}-ssl_key_passphrase>> |<<password,password>>|No
| <<plugins-{type}s-{plugin}-ssl_verify_mode>> |<<string,string>>, one of `["none", "peer", "force_peer"]`|No
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we're making things right, will you please move the ssl_verify_mode option to its proper place in alpha order--both here and in the description?

@kares kares requested a review from karenzone April 27, 2022 15:51
Copy link
Contributor

@karenzone karenzone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left comment for consideration. Otherwise, LGTM.

@kares kares merged commit d8b3ca5 into logstash-plugins:main Apr 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants