fix: Handle IPv6 addresses in OTLP HTTP connection #1747
Conversation
create2000
requested review from
fbogsany,
mwear,
robertlaurin,
dazuma,
ericmustin,
arielvalentin,
ahayworth,
plantfansam,
robbkidd and
kaylareopelle
as code owners
arielvalentin
changed the title
| @@ -109,7 +109,7 @@ def fetch_ssl_verify_mode | |||
| end | |||
|
|
|||
| def http_connection(uri, ssl_verify_mode, certificate_file, client_certificate_file, client_key_file) | |||
| http = Net::HTTP.new(uri.host, uri.port) | |||
| http = Net::HTTP.new(uri.hostname, uri.port) | |||
There was a problem hiding this comment.
Are there any unit tests that you may also include with this change would have exposed this bug?
e.g. setting the endpoint to "http://[::1]/v1/traces" and see the error goes away.
There was a problem hiding this comment.
Are you proposing i create some unit tests?
There was a problem hiding this comment.
@create2000 - It would be great to have unit tests for this change, especially if you can write some sort of regression-style test that demonstrates the bug was fixed due to your code updates.
There was a problem hiding this comment.
How would you suggest I go about the regression-style test? This is my first time contributing, and I am contributing as an Outreachy Intern applicant.
There was a problem hiding this comment.
Thanks for the context, @create2000!
What I mean is to write a test that would fail without your change. You could undo your change, write a test that uses an IPv6 address, and watch it fail. Then, add your change back and see it pass. This means your test will protect against regressions in the code that may re-introduce the bug.
There may be an existing test you can adjust from the OTLP exporter gem to make it easier.
The file you could look at is exporter/otlp/test/opentelemetry/exporter/otlp/exporter_test.rb
| @@ -0,0 +1 @@ | |||
| --require spec_helper | |||
There was a problem hiding this comment.
This repository uses minitest, rather than RSpec as the testing framework.
Would you mind deleting the .rspec file and the spec/spec_helper.rb file?
If you're having trouble running the tests locally, you can do so by:
- Installing the bundle
$ bundle install- Using Rake to run the tests
$ bundle exec rake testsIf this doesn't work, let us know and we can try to troubleshoot.
|
👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the |
Fixes #1721. This PR updates the Net::HTTP connection to handle IPv6 addresses correctly by using hostname instead of host.