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

fix(inputs.http_response): Fix for IPv4 and IPv6 addresses when interface is set #15496

Merged
merged 4 commits into from
Jun 13, 2024

Conversation

zak-pawel
Copy link
Collaborator

Summary

This PR solves the problem that existed in the scenario where the urls list contains mix of IPv4 and IPv6 addresses and the interface is config file set.
Until now, the plugin chooses the first available address for the given interface. In my case, it was IPv6 for Windows and IPv4 for Linux. Because of this, the plugin worked incorrectly with IPv4 addresses for Windows and IPv6 addresses for Linux.

Here is a list of the most important things done in this PR:

  • Each URL now has its own httpClient (until now, one httpClient was used to handle all URLs).

  • Each URL provided in the config and each address for the given interface in the config is parsed and checked to see if it is an IPv4 or IPv6 address.

    • I didn't find methods in the standard library (or in other dependencies) that would do this correctly and handle both url.URL and net.IPNet. That's why I added github.com/seancfoley/ipaddress-go lib. The topic of parsing addresses is complex enough that I didn't see the point in doing it myself. If any of the reviewers have an idea on how to do this better, I would be happy to hear it :)
  • Each URL has a httpClient assigned that matches the address format (IPv4 or IPv6).

  • Configuration checking and initializing defaults have been moved from Gather to Init.

    • This changes the behavior of the plugin slightly, which will now fail immediately if:
      • parsing the regular expression from h.ResponseStringMatch fails
      • at least one of the URLs provided in the config cannot be parsed
      • at least one of the URLs provided in the config is not of type http or https
      • another problem occurs while creating the httpClient for the URL

    (previously, these errors could occur in Gather, which did not cause the plugin to fail, but rather resulted in recurring errors in the logs)

Thanks to this PR, the http_response plugin tests for Windows are re-enabled.

Checklist

  • No AI generated code was used in this PR

Related issues

resolves #8451

@telegraf-tiger telegraf-tiger bot added fix pr to fix corresponding bug plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins labels Jun 12, 2024
Copy link
Contributor

@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.

Awesome work @zak-pawel! Just a few small comments...

plugins/inputs/http_response/http_response.go Outdated Show resolved Hide resolved
plugins/inputs/http_response/http_response.go Outdated Show resolved Hide resolved
plugins/inputs/http_response/http_response.go Outdated Show resolved Hide resolved
@srebhan srebhan self-assigned this Jun 12, 2024
@telegraf-tiger
Copy link
Contributor

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

⚠️ This pull request increases the Telegraf binary size by 1.62 % for linux amd64 (new size: 243.6 MB, nightly size 239.7 MB)

📦 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

Copy link
Contributor

@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 @zak-pawel!

@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 Jun 13, 2024
@srebhan srebhan assigned powersj and DStrand1 and unassigned srebhan Jun 13, 2024
@powersj powersj merged commit fc3cbb8 into influxdata:master Jun 13, 2024
26 of 27 checks passed
@github-actions github-actions bot added this to the v1.31.1 milestone Jun 13, 2024
srebhan pushed a commit that referenced this pull request Jul 1, 2024
}
}

return nil, fmt.Errorf("cannot create local address for interface %q and server address %q", interfaceName, address.String())
Copy link
Contributor

@Mic92 Mic92 Jul 6, 2024

Choose a reason for hiding this comment

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

This seems to break the case when the address is a hostname (in my case pointing to an ipv6 address) instead of an ip address:

Error running agent: could not initialize input inputs.http_response: cannot create local address for interface "tinc.retiolum" and server address "http://transmission.r/transmission/web/"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix pr to fix corresponding bug 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
Development

Successfully merging this pull request may close these issues.

http_response plugin have issues when urls list contains IPv4 and IPv6 addresses
5 participants