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.

Sign up for GitHub

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

Jump to bottom

feat: Use HTTPClientConfig struct in elastic stack plugins #14207

Merged
merged 6 commits into from
Nov 13, 2023
Merged

feat: Use HTTPClientConfig struct in elastic stack plugins #14207

merged 6 commits into from
Nov 13, 2023

Conversation

oserde
Copy link
Contributor

@oserde oserde commented Oct 29, 2023 •
edited
Loading

Enable elasticsearch, elasticsearch_query, kibana, and logstash plugins to use environment variables HTTP_PROXY, HTTPS_PROXY and NO_PROXY (or the lowercase versions) in order to connect through a proxy

resolves #14206

Adds Proxy to Transport struct, as in many other plugins.

@oserde
feat: Support proxy env vars from elastic stack plugins
0e2cd28 
Enable elasticsearch, elasticsearch_query, kibana, and logstash plugins
to use environment variables HTTP_PROXY, HTTPS_PROXY and NO_PROXY (or the lowercase versions) in order to connect through a proxy
@telegraf-tiger telegraf-tiger bot added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Oct 29, 2023
srebhan
srebhan requested changes Oct 30, 2023
View reviewed changes
Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution @oserde! We cannot accept the PR in the current form as it may break users that have the environment variables set but are connecting to one of the inputs without proxy. That might e.g. be the case if the server is in a local network but the proxy is required for internet access.

In my view the best way would be to switch the plugins to use the HTTPClientConfig structure using the proxy.HTTPProxy implementation exposing a use_system_proxy setting.

Do you think you can modify the PR in this way?

@srebhan srebhan self-assigned this Oct 30, 2023
@srebhan srebhan added plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins area/elasticsearch labels Oct 30, 2023
@oserde
Copy link
Contributor Author

oserde commented Oct 30, 2023

Hi @srebhan ,

Thanks for the feedback! I think your suggestions make a lot of sense. I will implement those changes, test the plugins and get back to you.

@powersj powersj added the waiting for response waiting for response from contributor label Nov 3, 2023
@oserde
use HTTPClientConfig in elastic stack plugins
b860065 
HTTPClientConfig structure is a generic way of configuring HTTPconnections
and it is already used by other plugins. It supports many HTTP options,
including the ability to set HTTP proxies at input level. Config option
`http_timeout` has been deprecated for elasticsearch plugin, as `timeout`
and `response_timeout` options already cover its function.
@oserde oserde changed the title feat: Support proxy env vars from elastic stack plugins feat: Use HTTPClientConfig struct in elastic stack plugins Nov 8, 2023
srebhan
srebhan reviewed Nov 10, 2023
View reviewed changes
Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Thank you very much for the update @oserde! Just one question...

plugins/inputs/elasticsearch_query/elasticsearch_query.go Show resolved Hide resolved
@oserde
Reset default timeouts from elastic stack plugins
47b237e 
Timeout setting had been removed from plugins because default value in
httpconfig.HTTPClientConfig struct currently coincides with the timeout
that these plugins set. By setting default timeout at plugin level we avoid
depending on default values from httpconfig.HTTPClientConfig struct
Hipska
Hipska suggested changes Nov 13, 2023
View reviewed changes
plugins/inputs/elasticsearch/elasticsearch.go Show resolved Hide resolved
plugins/inputs/elasticsearch/elasticsearch.go Outdated Show resolved Hide resolved
plugins/inputs/elasticsearch_query/elasticsearch_query.go Outdated Show resolved Hide resolved
plugins/inputs/kibana/kibana.go Outdated Show resolved Hide resolved
plugins/inputs/logstash/logstash.go Outdated Show resolved Hide resolved
oserde and others added 2 commits November 13, 2023 18:24
@oserde
Add missing deprecated notice in elastic plugin
d63c37a 
As dev guidelines suggest, a deprecated message should also be included
in sample config
@oserde @Hipska
Apply suggestions from code review
be7e51a 
Co-authored-by: Thomas Casteleyn <thomas.casteleyn@me.com>
@oserde
Copy link
Contributor Author

oserde commented Nov 13, 2023

Thanks for your suggestions! @Hipska

@telegraf-tiger telegraf-tiger bot removed the waiting for response waiting for response from contributor label Nov 13, 2023
@srebhan
Copy link
Member

srebhan commented Nov 13, 2023

@oserde please check the test/compile errors...

@oserde
Copy link
Contributor Author

oserde commented Nov 13, 2023

Fixed!

@telegraf-tiger
Copy link
Contributor

Download PR build artifacts for linux_amd64.tar.gz, darwin_amd64.tar.gz, and windows_amd64.zip.
Downloads for additional architectures and packages are available below.

☺️ This pull request doesn't significantly change the Telegraf binary size (less than 1%)

📦 Click here to get additional PR build artifacts

Artifact URLs

DEB RPM TAR GZ ZIP
amd64.deb aarch64.rpm darwin_amd64.tar.gz windows_amd64.zip
arm64.deb armel.rpm darwin_arm64.tar.gz windows_arm64.zip
armel.deb armv6hl.rpm freebsd_amd64.tar.gz windows_i386.zip
armhf.deb i386.rpm freebsd_armv7.tar.gz
i386.deb ppc64le.rpm freebsd_i386.tar.gz
mips.deb riscv64.rpm linux_amd64.tar.gz
mipsel.deb s390x.rpm linux_arm64.tar.gz
ppc64el.deb x86_64.rpm linux_armel.tar.gz
riscv64.deb linux_armhf.tar.gz
s390x.deb linux_i386.tar.gz
linux_mips.tar.gz
linux_mipsel.tar.gz
linux_ppc64le.tar.gz
linux_riscv64.tar.gz
linux_s390x.tar.gz

srebhan
srebhan approved these changes Nov 13, 2023
View reviewed changes
Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Thanks for the update @oserde!

@srebhan srebhan added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Nov 13, 2023
@srebhan srebhan assigned powersj and unassigned srebhan Nov 13, 2023
powersj
powersj approved these changes Nov 13, 2023
View reviewed changes
Copy link
Contributor

@powersj powersj left a comment

Choose a reason for hiding this comment

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

Thanks for this!

@powersj powersj merged commit 59f53c0 into influxdata:master Nov 13, 2023
4 checks passed
@github-actions github-actions bot added this to the v1.29.0 milestone Nov 13, 2023
@Porkepix Porkepix mentioned this pull request Dec 11, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hipska Hipska Hipska requested changes

@srebhan srebhan srebhan approved these changes

@powersj powersj powersj approved these changes

Assignees

@powersj powersj

Labels
area/elasticsearch feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Milestone
v1.29.0
Development

Successfully merging this pull request may close these issues.

Elastic stack related plugins should take into account proxy environment variables
4 participants
@oserde @srebhan @Hipska @powersj