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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: hosts => "es_host:port" regression (when ssl => true) #156

Merged
merged 23 commits into from
Feb 8, 2022

Conversation

kares
Copy link
Contributor

@kares kares commented Feb 7, 2022

The change here makes sure "es_host:port" is properly split into a { host: es_host, port: port } pair.
Elasticsearch gem not seeing a port: key will add the port: 9200 default and Manticore 0.8 appends the port part while building the URL.


The issue did reproduce with the added integration test (under SECURE_INTEGRATION) - we did not have testing setup with secured ES, the setup has been ~ copied from ES output plugin.

There's also some additional test targets to keep testing against previous ES and Manticore gem versions.


CI 馃敶 are caused by #157 (and logstash-plugins/logstash-input-elasticsearch#165)

@kares kares linked an issue Feb 7, 2022 that may be closed by this pull request
@kares kares changed the title Fix: split host:port pair to avoid regression Fix: hosts => "es_host:port" regression (when ssl => true) Feb 8, 2022
@kares kares marked this pull request as ready for review February 8, 2022 11:59
@jsvd jsvd self-requested a review February 8, 2022 12:12
@@ -3,5 +3,19 @@ FROM docker.elastic.co/elasticsearch/elasticsearch$distribution_suffix:$ELASTIC_

ARG es_path=/usr/share/elasticsearch
ARG es_yml=$es_path/config/elasticsearch.yml
ARG SECURE_INTEGRATION
ARG ES_SSL_SUPPORTED_PROTOCOLS
Copy link
Contributor Author

Choose a reason for hiding this comment

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

鉁傦笍 not used atm

spec/filters/integration/elasticsearch_spec.rb Outdated Show resolved Hide resolved
spec/filters/integration/elasticsearch_spec.rb Outdated Show resolved Hide resolved
lib/logstash/filters/elasticsearch/client.rb Show resolved Hide resolved
@@ -0,0 +1,20 @@
-----BEGIN CERTIFICATE-----
Copy link
Member

Choose a reason for hiding this comment

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

This cert expires in a couple of years, will this break tests or is curl -k the only relevant validation skip we need?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed it will break integration specs eventually, just like in ES output: https://github.com/logstash-plugins/logstash-output-elasticsearch/tree/v11.4.1/spec/fixtures/test_certs where it's copied from.

We do not have a base-line setup on generating TLS material in tests and there isn't a good enough excuse to start looking into that atm (simply due having more pressing stuff to tackle).
Hopefully the TLS setting unification effort will be a place to look into these...

Copy link
Member

Choose a reason for hiding this comment

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

agreed it doesn't need to be in scope, it's a good target for refactoring if we do an integration plugin. For reference, we have some plugins that generate certs during the tests, such as http input and tcp input. example here: https://github.com/logstash-plugins/logstash-input-tcp/blob/main/spec/spec_helper.rb#L37-L52

Copy link
Contributor Author

@kares kares Feb 8, 2022

Choose a reason for hiding this comment

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

yeah I know of these but here we need to have PEM certificates ahead of time during Docker build ...
(to setup a secured TLS configuration in ES)

kares and others added 2 commits February 8, 2022 14:56
Co-authored-by: Jo茫o Duarte <jsvd@users.noreply.github.com>
Co-authored-by: Jo茫o Duarte <jsvd@users.noreply.github.com>
@kares kares requested a review from jsvd February 8, 2022 14:40
Copy link
Member

@jsvd jsvd left a comment

Choose a reason for hiding this comment

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

LGTM.

The ES ruby client behaves very inconsistently wrt hosts input:

> begin; Elasticsearch::Client.new(hosts: "localhost:9000").transport.transport.connections.first.host; rescue => e; puts [e.class, e.message];end
=> {:host=>"localhost", :port=>9000, :protocol=>"http"}

> begin; Elasticsearch::Client.new(hosts: {host: "localhost:9000"}).transport.transport.connections.first.host; rescue => e; puts [e.class, e.message];end
URI::InvalidURIError
bad URI(is not URI?): http://localhost:9000:9200

> begin; Elasticsearch::Client.new(hosts: "http://localhost:9000").transport.transport.connections.first.host; rescue => e; puts [e.class, e.message];end
=> {:scheme=>"http", :user=>nil, :password=><REDACTED>, :host=>"localhost", :path=>"", :port=>9000, :protocol=>"http"}

> begin; Elasticsearch::Client.new(hosts: {host: "http://localhost:9000"}).transport.transport.connections.first.host; rescue => e; puts [e.class, e.message];end
=> {:host=>"http://localhost:9000", :protocol=>"http", :port=>9200}

I'll open an issue to track this on that side.

@kares kares merged commit 57482ef into logstash-plugins:main Feb 8, 2022
kares added a commit to kares/logstash-filter-elasticsearch that referenced this pull request Mar 14, 2022
* main:
  Fix: hosts => "es_host:port" regression (when ssl => true) (logstash-plugins#156)
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.

regression on Manticore 0.8.0 due port being part of host
3 participants