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

Add support for interface field in http_response input plugin #6006

Merged
merged 4 commits into from
Jun 19, 2019
Merged

Add support for interface field in http_response input plugin #6006

merged 4 commits into from
Jun 19, 2019

Conversation

GeorgeMac
Copy link
Contributor

@GeorgeMac GeorgeMac commented Jun 18, 2019

This is an attempt to implement the interface field in the http_response input plugin.

The interface field can be populated with an interface name such as eth0. A local address associated with the network interface will be derived and used for dialing.

Resolves: #5856

Required for all PRs:

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


intf, err := findInterface()
if err != nil {
t.Fatal(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

I likely would have just t.Skip(err), but then again I believe httptest.NewServer requires at least the loopback interface, so this is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can change that if skip is preferred. I didn’t realize skip existed in that form.

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems fine, since we require the loopback in many tests, though in Telegraf we usually use testify: require.NoError(t, err).

@glinton glinton added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Jun 19, 2019
@glinton glinton added this to the 1.12.0 milestone Jun 19, 2019

defer ts.Close()

intf, err := findInterface()
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like the test would depend on finding an interface that can route to the test http server network, which would only be a loopback interface because this server listens on 127.0.0.1:0. Could we simplify this function to just grab any loopback interface?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely. That makes more sense. I was trying to test a non-loopback interface, which in hindsight makes no sense. I will make it get the name of the first loopback interface it finds.


intf, err := findInterface()
if err != nil {
t.Fatal(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems fine, since we require the loopback in many tests, though in Telegraf we usually use testify: require.NoError(t, err).

@danielnelson danielnelson merged commit 8d04cb7 into influxdata:master Jun 19, 2019
@GeorgeMac GeorgeMac deleted the gm/inputs-http-response-interface branch June 20, 2019 10:10
bitcharmer pushed a commit to bitcharmer/telegraf that referenced this pull request Oct 18, 2019
athoune pushed a commit to bearstech/telegraf that referenced this pull request Apr 17, 2020
idohalevi pushed a commit to idohalevi/telegraf that referenced this pull request Sep 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support binding to a specific local address for http_response
3 participants