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: Handle IPv6 addresses in OTLP HTTP connection #1747

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .rspec
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
--require spec_helper
Copy link
Contributor

Choose a reason for hiding this comment

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

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:

  1. Installing the bundle
$ bundle install
  1. Using Rake to run the tests
$ bundle exec rake tests

If this doesn't work, let us know and we can try to troubleshoot.

Copy link
Author

Choose a reason for hiding this comment

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

Alright

Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

Are you proposing i create some unit tests?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

http.use_ssl = uri.scheme == 'https'
http.verify_mode = ssl_verify_mode
http.ca_file = certificate_file unless certificate_file.nil?
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ module Util # rubocop:disable Metrics/ModuleLength
DEFAULT_USER_AGENT = "OTel-OTLP-MetricsExporter-Ruby/#{OpenTelemetry::Exporter::OTLP::Metrics::VERSION} Ruby/#{RUBY_VERSION} (#{RUBY_PLATFORM}; #{RUBY_ENGINE}/#{RUBY_ENGINE_VERSION})".freeze

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)
http.use_ssl = uri.scheme == 'https'
http.verify_mode = ssl_verify_mode
http.ca_file = certificate_file unless certificate_file.nil?
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ def shutdown(timeout: nil)
private

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)
http.use_ssl = uri.scheme == 'https'
http.verify_mode = ssl_verify_mode
http.ca_file = certificate_file unless certificate_file.nil?
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ def initialize(endpoint: OpenTelemetry::Common::Utilities.config_opt('OTEL_EXPOR
URI(endpoint)
end

@http = Net::HTTP.new(@uri.host, @uri.port)
@http = Net::HTTP.new(@uri.hostname, @uri.port)
@http.use_ssl = @uri.scheme == 'https'
@http.keep_alive_timeout = KEEP_ALIVE_TIMEOUT

Expand Down
98 changes: 98 additions & 0 deletions spec/spec_helper.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
# This file was generated by the `rspec --init` command. Conventionally, all
# specs live under a `spec` directory, which RSpec adds to the `$LOAD_PATH`.
# The generated `.rspec` file contains `--require spec_helper` which will cause
# this file to always be loaded, without a need to explicitly require it in any
# files.
#
# Given that it is always loaded, you are encouraged to keep this file as
# light-weight as possible. Requiring heavyweight dependencies from this file
# will add to the boot time of your test suite on EVERY test run, even for an
# individual file that may not need all of that loaded. Instead, consider making
# a separate helper file that requires the additional dependencies and performs
# the additional setup, and require it from the spec files that actually need
# it.
#
# See https://rubydoc.info/gems/rspec-core/RSpec/Core/Configuration
RSpec.configure do |config|
# rspec-expectations config goes here. You can use an alternate
# assertion/expectation library such as wrong or the stdlib/minitest
# assertions if you prefer.
config.expect_with :rspec do |expectations|
# This option will default to `true` in RSpec 4. It makes the `description`
# and `failure_message` of custom matchers include text for helper methods
# defined using `chain`, e.g.:
# be_bigger_than(2).and_smaller_than(4).description
# # => "be bigger than 2 and smaller than 4"
# ...rather than:
# # => "be bigger than 2"
expectations.include_chain_clauses_in_custom_matcher_descriptions = true
end

# rspec-mocks config goes here. You can use an alternate test double
# library (such as bogus or mocha) by changing the `mock_with` option here.
config.mock_with :rspec do |mocks|
# Prevents you from mocking or stubbing a method that does not exist on
# a real object. This is generally recommended, and will default to
# `true` in RSpec 4.
mocks.verify_partial_doubles = true
end

# This option will default to `:apply_to_host_groups` in RSpec 4 (and will
# have no way to turn it off -- the option exists only for backwards
# compatibility in RSpec 3). It causes shared context metadata to be
# inherited by the metadata hash of host groups and examples, rather than
# triggering implicit auto-inclusion in groups with matching metadata.
config.shared_context_metadata_behavior = :apply_to_host_groups

# The settings below are suggested to provide a good initial experience
# with RSpec, but feel free to customize to your heart's content.
=begin
# This allows you to limit a spec run to individual examples or groups
# you care about by tagging them with `:focus` metadata. When nothing
# is tagged with `:focus`, all examples get run. RSpec also provides
# aliases for `it`, `describe`, and `context` that include `:focus`
# metadata: `fit`, `fdescribe` and `fcontext`, respectively.
config.filter_run_when_matching :focus

# Allows RSpec to persist some state between runs in order to support
# the `--only-failures` and `--next-failure` CLI options. We recommend
# you configure your source control system to ignore this file.
config.example_status_persistence_file_path = "spec/examples.txt"

# Limits the available syntax to the non-monkey patched syntax that is
# recommended. For more details, see:
# https://rspec.info/features/3-12/rspec-core/configuration/zero-monkey-patching-mode/
config.disable_monkey_patching!

# This setting enables warnings. It's recommended, but in some cases may
# be too noisy due to issues in dependencies.
config.warnings = true

# Many RSpec users commonly either run the entire suite or an individual
# file, and it's useful to allow more verbose output when running an
# individual spec file.
if config.files_to_run.one?
# Use the documentation formatter for detailed output,
# unless a formatter has already been configured
# (e.g. via a command-line flag).
config.default_formatter = "doc"
end

# Print the 10 slowest examples and example groups at the
# end of the spec run, to help surface which specs are running
# particularly slow.
config.profile_examples = 10

# Run specs in random order to surface order dependencies. If you find an
# order dependency and want to debug it, you can fix the order by providing
# the seed, which is printed after each run.
# --seed 1234
config.order = :random

# Seed global randomization in this process using the `--seed` CLI option.
# Setting this allows you to use `--seed` to deterministically reproduce
# test failures related to randomization by passing the same `--seed` value
# as the one that triggered the failure.
Kernel.srand config.seed
=end
end
Loading