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: add socks5 proxy support to kafka #8192

Merged
merged 1 commit into from
Feb 16, 2022
Merged

feat: add socks5 proxy support to kafka #8192

merged 1 commit into from
Feb 16, 2022

Conversation

alexole
Copy link
Contributor

@alexole alexole commented Sep 29, 2020 •
edited
Loading

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

@alexole alexole marked this pull request as draft September 29, 2020 14:13
@reimda
Copy link
Contributor

reimda commented Sep 29, 2020

Hi @alexole thanks for this work on socks5 proxy. I see this is still a draft PR but I wanted to share some thoughts.

Since proxy support is something many other plugins could benefit from, I'd like to see us add it in a way that lets other plugins reuse it easily. We have a similar situation with tls config. In that case we have separate config structs with toml tags, and tls related code factored out of plugins. This means every plugin that uses tls has settings with the same names and the plugin doesn't have to duplicate code to set up tls. (see plugins/common/tls/config.go)

I'd like to see us do something similar with proxy settings. I expect you only need it for kafka but it would be helpful if you'd add it in a generic way.

@alexole
Copy link
Contributor Author

alexole commented Sep 30, 2020

@reimda thanks for the comment, I'll try to find time to make it properly.

@alexole alexole force-pushed the add-kafka-plugin-socks5-support branch from e2be845 to 9e49963 Compare October 14, 2020 09:11
@sjwang90 sjwang90 added area/kafka feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin labels Nov 24, 2020
srebhan
srebhan reviewed Oct 28, 2021
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 @alexole for the PR. Looks good overall, but I want to suggest to remove the extra enable option. Can you please take a look?
Furthermore, please move the PR out of the Draft state once you are done.

plugins/outputs/kafka/kafka.go Outdated Show resolved Hide resolved
plugins/outputs/kafka/kafka.go Outdated Show resolved Hide resolved
plugins/outputs/kafka/kafka.go Outdated Show resolved Hide resolved
@srebhan srebhan self-assigned this Oct 28, 2021
@srebhan
Copy link
Member

srebhan commented Dec 20, 2021

@alexole any news here?

@srebhan
Copy link
Member

srebhan commented Feb 4, 2022

@alexole do you plan to continue on this PR?

@srebhan srebhan added the waiting for response waiting for response from contributor label Feb 4, 2022
@alexole
Copy link
Contributor Author

alexole commented Feb 7, 2022

Hi, sorry for not getting back to you. Yes, we will address the change requests soon.

@telegraf-tiger telegraf-tiger bot removed the waiting for response waiting for response from contributor label Feb 7, 2022
@mhoffm-aiven mhoffm-aiven force-pushed the add-kafka-plugin-socks5-support branch 2 times, most recently from 04d8814 to 333990c Compare February 7, 2022 15:25
@alexole alexole marked this pull request as ready for review February 7, 2022 15:27
@mhoffm-aiven mhoffm-aiven force-pushed the add-kafka-plugin-socks5-support branch 3 times, most recently from 5981590 to 1628daf Compare February 7, 2022 15:33
@alexole alexole changed the title kafka: add socks5 proxy support for kafka output plugin kafka: add socks5 proxy support Feb 7, 2022
@mhoffm-aiven mhoffm-aiven force-pushed the add-kafka-plugin-socks5-support branch 3 times, most recently from 739a02c to 9f4ad29 Compare February 11, 2022 12:42
@mhoffm-aiven
Copy link
Contributor

Hey @srebhan,

could you have another look?

srebhan
srebhan reviewed Feb 16, 2022
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.

Sorry @alexole for the late response. I have some smaller comments left, but nothing big. Can you please take a look?

plugins/outputs/kafka/kafka.go Outdated Show resolved Hide resolved
plugins/common/proxy/socks5_test.go Show resolved Hide resolved
plugins/common/proxy/socks5_test.go Outdated Show resolved Hide resolved
@mhoffm-aiven mhoffm-aiven force-pushed the add-kafka-plugin-socks5-support branch 2 times, most recently from ab2eb9e to 4c8cf67 Compare February 16, 2022 12:57
@srebhan
Copy link
Member

srebhan commented Feb 16, 2022

Sorry @mhoffm-aiven I messed up the formatting. Can you please run make fmt and push the change... :-(

@srebhan
Copy link
Member

srebhan commented Feb 16, 2022

Oh... too slow. ;-)

@mhoffm-aiven
Copy link
Contributor

Sorry @mhoffm-aiven I messed up the formatting. Can you please run make fmt and push the change... :-(

already done! no worries i did not catch it in the web view

srebhan
srebhan approved these changes Feb 16, 2022
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.

Looks good to me. Thanks for pushing this over the finishing line @mhoffm-aiven!

@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 Feb 16, 2022
@alexole @srebhan @mhoffm-aiven
feat: add socks5 proxy support for kafka output plugin
8cff4a6 
Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>
@mhoffm-aiven mhoffm-aiven force-pushed the add-kafka-plugin-socks5-support branch from 4c8cf67 to 8cff4a6 Compare February 16, 2022 14:51
@powersj powersj changed the title kafka: add socks5 proxy support feat: add socks5 proxy support to kafka Feb 16, 2022
@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_i386.zip
armel.deb armv6hl.rpm freebsd_amd64.tar.gz
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
static_linux_amd64.tar.gz

@MyaLongmire MyaLongmire merged commit 0d529d8 into influxdata:master Feb 16, 2022
phemmer added a commit to phemmer/telegraf that referenced this pull request Feb 18, 2022
@phemmer
Merge remote-tracking branch 'origin/master' into postgres
e87744d 
* origin/master: (211 commits)
  feat: update configs (influxdata#10676)
  feat[elastic output]: add elastic pipeline flags (influxdata#10505)
  Update changelog
  fix: ensure folders do not get loaded more than once (influxdata#10551)
  docs: update VMWare doc links (influxdata#10663)
  fix: prometheusremotewrite wrong timestamp unit (influxdata#10547)
  feat: update configs (influxdata#10662)
  fix: add graylog toml tags (influxdata#10660)
  feat: add socks5 proxy support for kafka output plugin (influxdata#8192)
  docs: override reported OpenSearch version (influxdata#10586)
  feat: update etc/telegraf.conf and etc/telegraf_windows.conf (influxdata#10659)
  fix: bump all go.opentelemetry.io dependencies (influxdata#10647)
  feat: collection offset implementation (influxdata#10545)
  chore: update go to 1.17.7 (influxdata#10658)
  fix: check for nil client before closing in amqp (influxdata#10635)
  fix: timestamp change during execution of json_v2 parser. (influxdata#10657)
  fix: bump github.com/signalfx/golib/v3 from 3.3.38 to 3.3.43 (influxdata#10652)
  fix: bump github.com/aliyun/alibaba-cloud-sdk-go (influxdata#10653)
  fix: incorrect handling of json_v2 timestamp_path (influxdata#10618)
  feat: gather additional stats from memcached (influxdata#10641)
  feat: update etc/telegraf.conf and etc/telegraf_windows.conf (influxdata#10649)
  fix: Revert deprecation of http_listener_v2 (influxdata#10648)
  fix: bump github.com/denisenkom/go-mssqldb from 0.10.0 to 0.12.0 (influxdata#10503)
  fix: bump github.com/gopcua/opcua from 0.2.3 to 0.3.1 (influxdata#10626)
  fix: use current time as ecs timestamp (influxdata#10636)
  fix: bump github.com/nats-io/nats-server/v2 from 2.6.5 to 2.7.2 (influxdata#10638)
  chore: add -race flag to go tests (influxdata#10629)
  feat: update etc/telegraf.conf and etc/telegraf_windows.conf (influxdata#10631)
  fix: license doc outdated causing CI failure (influxdata#10630)
  fix: bump k8s.io/client-go from 0.22.2 to 0.23.3 (influxdata#10589)
  feat: Implemented support for reading raw values, added tests and doc (influxdata#6501)
  fix: Improve parser tests by using go-cmp/cmp (influxdata#10497)
  feat(mongodb): add FsTotalSize and FsUsedSize informations (influxdata#10625)
  docs: update quay docs for auth (influxdata#10612)
  chore: allow downgrade of go version in windows script (influxdata#10614)
  chore: update CI go to 1.17.6 (influxdata#10611)
  feat: update etc/telegraf.conf and etc/telegraf_windows.conf (influxdata#10600)
  fix(inputs.opcua): add more data to error log (influxdata#10465)
  fix: bump github.com/aws/aws-sdk-go-v2/service/kinesis from 1.6.0 to 1.13.0 (influxdata#10601)
  refactor: use early return pattern (influxdata#10591)
  ...
MyaLongmire pushed a commit that referenced this pull request Jul 6, 2022
@alexole @MyaLongmire
feat: add socks5 proxy support for kafka output plugin (#8192)
b85cbba
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhoffm-aiven mhoffm-aiven mhoffm-aiven left review comments

@srebhan srebhan srebhan approved these changes

@MyaLongmire MyaLongmire MyaLongmire approved these changes

@powersj powersj powersj approved these changes

Assignees

@srebhan srebhan

Labels
area/kafka feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin 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
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@alexole @reimda @srebhan @mhoffm-aiven @powersj @MyaLongmire @sjwang90