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

186-custom-headers #187

Open
wants to merge 16 commits into
base: 3.x
Choose a base branch
from
Open

186-custom-headers #187

wants to merge 16 commits into from

Conversation

flexitrev
Copy link

This adds custom header support.
custom_headers => {"Key" => "Value"} will be added to requests. Supports use of API keys and bearer tokens

docs/index.asciidoc Outdated Show resolved Hide resolved
@flexitrev flexitrev requested a review from strawgate January 14, 2025 21:49
Copy link
Contributor

@robbavey robbavey left a comment

Choose a reason for hiding this comment

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

Don't forget to bump version info, and add changelog entry.

spec/filters/elasticsearch_spec.rb Outdated Show resolved Hide resolved
lib/logstash/.DS_Store Outdated Show resolved Hide resolved
@mashhurs
Copy link

Changes look good to me.
However, with the current situation (9.0 compatible v4.0.0 already released), I think with this change, we need to

  • cut 3.x branch
  • update logstash-filter-elasticsearch.gemspec -> version to 3.17.0
  • change current PR base branch against 3.x
  • merge
  • and forward port to main

Let me know if you need any help.

@mashhurs mashhurs changed the base branch from main to 3.x January 16, 2025 01:55
@mashhurs mashhurs changed the base branch from 3.x to main January 16, 2025 02:01
@mashhurs mashhurs changed the base branch from main to 3.x January 16, 2025 02:06
@mashhurs
Copy link

I have cur 3.x correctly and changed this PR's base branch.
If you please update logstash-filter-elasticsearch.gemspec -> version to 3.17.0, we will be good to move forward.
Thank you again for the effort!

@flexitrev flexitrev requested a review from robbavey January 16, 2025 02:29
@kaisecheng
Copy link
Contributor

@mashhurs Can you tell me why this feature need to be on top of 3.x but not the usual workflow to 4.1.0?
Stack version 9 is not released yet and Logstash 7 & 8 can freely update to es-filter v4.0.0. I don't see why this plugin need to be 3.x. Am I missing something?

@flexitrev
Copy link
Author

I think it's because of the changes we're making to SSL settings. That we want to wait for a major version change for that, but this is a minor change that we want to release sooner.

@kaisecheng
Copy link
Contributor

@flexitrev es-filter 4.0.0 is already published. If you release 3.17.0 now, the custom headers feature will be available in 3.17.0 but not 4.0.0, which is confusing. This is not a normal workflow we do in plugin release

@robbavey
Copy link
Contributor

To @kaisecheng's point - this work needs to be in both 3.x and 4.x.

The usual workflow of committing into main and then backporting to 3.x should have been followed here.

If we proceed with targeting against 3.x, this will require a forward port to main, and a new release in the 4.x series

robbavey and others added 3 commits January 16, 2025 15:12
* Mark previously deprecated SSL settings as obsolete

- SSL settings that were marked deprecated in version `3.15.0` are now marked obsolete, and will prevent the plugin from starting.
- These settings are:
  - `ca_file`, which should be replaced by `ssl_certificate_authorities`
  - `keystore`, which should be replaced by `ssl_keystore_path`
  - `keystore_password`, which should be replaced by `ssl_keystore_password`
  - `keystore_type`, which should be replaced by `ssl_keystore_password`
  - `ssl`, which should be replaced by `ssl_enabled`
@flexitrev
Copy link
Author

Rebased from main. Let me know if there's any outstanding issues, and I'll merge into main

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.

5 participants