Add support for `http.client_ip` tag for Rack-based frameworks #2248

barweiss · 2022-08-30T12:44:09Z

What does this PR do?

Add support for the http.client_ip tag for Rack-based API frameworks.

Motivation

The http.client_ip tag will help distinguish whether the request came from a public or a private address, allowing Datadog to distinguish external from internal services and API endpoints.

How to test the change?

The change is validated by RSpec behavior tests.

To test integration:

Set up and start a demo Rails server and enable auto-instrumentation.
Then use the following commands:

curl localhost:3000 # direct IP
curl -H 'X-Forwarded-For: 1.1.1.1' localhost:3000 # forwarded IP
curl -H 'X-Forwarded-For: 1.1.1.1' -H 'X-Forwarded-For: 1.1.1.1' localhost:3000 # two values for the header = no client ip
curl -H 'X-Forwarded-For: 1.1.1.1' -H 'X-Real-Ip: 1.2.3.4' localhost:3000 # two "ip" headers set = no client ip

Review the logs from the server. The emitted traces should reflect the expected results.

lloeki

A few changes needed but overall LGTM.

It seems like CircleCI ought to catch some linting issues but it did not run, or Rubocop is set to ignore them.

I'd rather have a second review from the Tracing team as well :)

lloeki · 2022-08-30T13:52:28Z

lib/datadog/tracing/client_ip.rb

+        valid_ipv4?(ip) || valid_ipv6?(ip)
+      end
+
+      # --- Section vendored from the ipaddress gem --- #


Is there a rationale for not using IPAddr.new(addr).ipv4? and IPAddr.new(addr).ipv6??

If we decide to vendor third party code, we should find a way to slip the corresponding license, at least in LICENSE-3rd_party.csv.

I avoided IPAddr initially because it has the netmask handling which I didn't want but I guess it's solvable with some verification and it spares vendoring from ipaddress.
I'll give IPAddr a shot

lloeki · 2022-08-30T13:56:39Z

lib/datadog/tracing/client_ip.rb

+      end
+
+      def self.strip_zone_specifier(ipv6)
+        if /\A(.*?)%.*/ =~ ipv6


Good catch, I had to add this as well at Sqreen.

This could be achieved with ipv6.gsub(/%.*$/, '') instead of relying on intermediate match objects.

Nice! Fixed

lloeki · 2022-08-30T14:04:24Z

lib/datadog/core/configuration/settings.rb

+              o.default { env_to_bool(Tracing::Configuration::Ext::ClientIp::ENV_DISABLED, false) }
+              o.lazy
+            end
+            # An optional name of a custom header to resolve the client IP from.


Suggested change

# An optional name of a custom header to resolve the client IP from.

# An optional name of a custom header to resolve the client IP from.

Linter should have caught this?

Whoops.
Rubocop seems to pass but I shouldv'e considered the formatting in the other options.
Fixed

lloeki · 2022-08-30T14:10:51Z

lib/datadog/tracing/client_ip.rb

+      TAG_MULTIPLE_IP_HEADERS = '_dd.multiple-ip-headers'.freeze
+
+      # A header collection implementation that looks up headers in a Hash.
+      class HashHeaderCollection < HeaderCollection


I feel like this could go to the Core namespace.

Nice, fixed

lloeki · 2022-08-30T14:11:18Z

lib/datadog/tracing/client_ip.rb

+      ].freeze
+
+      # A collection of headers.
+      class HeaderCollection


I feel like this could go to the Core namespace.

lloeki · 2022-08-30T14:16:13Z

lib/datadog/tracing/client_ip.rb

+          return
+        end
+
+        unless valid_ip?(ip)


I feel this nesting of unless is confusing.

Is this necessary since extract_ip_from_full_address seems to return the address when its internal criteria don't match?

I completely refactored that part, let me know if it makes more sense now :)

lloeki · 2022-08-30T14:21:01Z

lib/datadog/tracing/client_ip.rb

+        return {} unless headers
+
+        {}.tap do |result|
+          DEFAULT_IP_HEADERS_NAMES.each do |name|


Looks like a job for DEFAULT_IP_HEADERS_NAMES.each_with_object({}) do |name, result|

Done with reduce (each_with_object returns an array)

EDIT: No it does not, I misread the documentation. Changed

lloeki · 2022-08-30T14:23:00Z

lib/datadog/tracing/client_ip.rb

+        return headers.get(configuration.header_name) if configuration.header_name && headers
+
+        ip_values_from_headers = ip_headers(headers).values
+        case ip_values_from_headers.size


Suggested change

case ip_values_from_headers.size

case ip_values_from_headers.size

Method return value of this method comes from the case return value, so I'd suggest to space this to conform with the Ruby style guide where implicit returns live on their own "line".

Good point, done

lloeki · 2022-08-30T14:33:41Z

lib/datadog/tracing/contrib/rack/middlewares.rb

@@ -219,13 +226,13 @@ def configuration
            Datadog.configuration.tracing[:rack]
          end

-          def parse_request_headers(env)
+          def parse_request_headers(headers)
            {}.tap do |result|


Similarly to above, looks like a job for each_with_object

lloeki · 2022-08-30T14:39:55Z

lib/datadog/tracing/client_ip.rb

+        return if configuration.disabled
+
+        ip = client_address_from_request(headers, remote_ip)
+        if !configuration.header_name && ip.nil?


So this is only done when there's no IP reported? This feels off somehow (like, when would this happen), so I must be missing something.

Let me know if this is still confusing after the refactor

barweiss · 2022-08-31T07:50:28Z

lib/datadog/tracing/client_ip.rb

+      # @param [Span] span The span that's associated with the request.
+      # @param [HeaderCollection, #get, nil] headers A collection with the request headers.
+      # @param [String, nil] remote_ip The remote IP the request associated with the span is sent to.
+      def self.set_client_ip_tag(span, headers, remote_ip)


Noob question: should this have ! in the name?

! is a Ruby convention for hinting side-effect. For example, Array#map returns a new array while Array#map! returns a mutated self. Another common pattern is whether a method raise an exception, such as find_by vs find_by!.

I don't think this method deserve !, but this one seems to be a better candidate.

Since headers, remote_ip are optional. It is awkward to see

client_ip.set_client_ip_tag(span, nil, '1.1.1.1').

Consider making those keyword argument, so the caller makes the invokation a bit more explicit like,

client_ip.set_client_ip_tag(span, remote_ip: '1.1.1.1') or
client_ip.set_client_ip_tag(span, headers: headers).

Control flow using exception handling is slow in Ruby. This method need to be changed for

Reconsider whether if the scenario is truly an exception

Streamline the exceptional handling cases to avoid nested rescue/raise

Cool thanks!

As for using exceptions, I was indeed wondering whether this would have a performance impact. I'll change it so it doesn't use any exceptions.

TonyCTHsu · 2022-08-31T09:42:57Z

lib/datadog/core/configuration/settings.rb

+            #
+            # @default `DD_TRACE_CLIENT_IP_HEADER_DISABLED` environment variable, otherwise `false`.
+            # @return [Boolean]
+            option :disabled do |o|


Semantically, prefer enabled to disabled

I usually agree but since the environment variable is DISABLED I figured I'd keep it disabled.
Do you still think it's worth the setting name?

Can we also rename the environment variable to DD_TRACE_CLIENT_IP_HEADER_ENABLED to be consistent with other environment variable?

TonyCTHsu · 2022-08-31T09:57:49Z

lib/datadog/tracing/client_ip.rb

+      # @param [Span] span The span that's associated with the request.
+      # @param [HeaderCollection, #get, nil] headers A collection with the request headers.
+      # @param [String, nil] remote_ip The remote IP the request associated with the span is sent to.
+      def self.set_client_ip_tag(span, headers, remote_ip)


! is a Ruby convention for hinting side-effect. For example, Array#map returns a new array while Array#map! returns a mutated self. Another common pattern is whether a method raise an exception, such as find_by vs find_by!.

I don't think this method deserve !, but this one seems to be a better candidate.

Since headers, remote_ip are optional. It is awkward to see

client_ip.set_client_ip_tag(span, nil, '1.1.1.1').

Consider making those keyword argument, so the caller makes the invokation a bit more explicit like,

client_ip.set_client_ip_tag(span, remote_ip: '1.1.1.1') or
client_ip.set_client_ip_tag(span, headers: headers).

Control flow using exception handling is slow in Ruby. This method need to be changed for

Reconsider whether if the scenario is truly an exception

Streamline the exceptional handling cases to avoid nested rescue/raise

TonyCTHsu · 2022-08-31T10:15:11Z

spec/datadog/tracing/client_ip_spec.rb

+
+  describe 'ip validation' do
+    context 'when given valid ip addresses' do
+      subject do


I think this could be simplified, without custom matcher

describe ".validate_ip" do ips = [ ... ] ips.each do |ip| context 'when given #{ip}' do let(:normalised) { described_class.strip_decorations(ip) } subject { described_class.validate_ip(normalised) } it { is_expected.to be true } end end end

TonyCTHsu · 2022-08-31T10:23:46Z

spec/datadog/tracing/client_ip_spec.rb

+      let(:tagging_enabled) { false }
+
+      it 'does nothing' do
+        expect(span).to_not receive(:set_tag).with(any_args)


with(any_args) is redundant and assertion expect(span).to_not receive(:set_tag) should be strong enough.

Awesome, fixed

barweiss · 2022-09-05T16:03:13Z

One of the remaining issues in this PR is choosing to use the builtin IPAddr class, or vendoring the ipaddress gem.

In order to check whether a string is a valid IP with IPAddr, we need to see that IPAddr.new does not raise an exception. However, IPAddress.valid? allows checking for invalid IP addresses without raising exceptions.

One of my concerns was whether raising an exceptions will have a significant performance impact, considering that in cases of misconfiguration, an invalid IP can be present in every traced request.

After a discussion with @TonyCTHsu, and following his advice, I ran some benchmarks that make it clear there's no significant performance hit:

Warming up --------------------------------------
              ipaddr    13.742k i/100ms
           ipaddress    14.983k i/100ms
Calculating -------------------------------------
              ipaddr    141.715k (± 1.5%) i/s -    714.584k in   5.043502s
           ipaddress    148.502k (± 1.6%) i/s -    749.150k in   5.046105s

Comparison:
           ipaddress:   148501.6 i/s
              ipaddr:   141714.9 i/s - 1.05x  (± 0.00) slower

…'ipaddr'

lloeki · 2022-09-07T19:53:04Z

Approved, provided the integration CI failures are investigated if they still fail after a re-run.

marcotc · 2022-09-08T00:27:32Z

lib/datadog/core/header_collection.rb

+    class HashHeaderCollection < HeaderCollection
+      def initialize(hash)
+        super()
+        @hash = hash.transform_keys(&:downcase)


Hash#transform_keys is only available in Ruby 2.5 or later:

puts({}.transform_keys(&:downcase)) # == 2.4.10 == # -e:1:in `<main>': undefined method `transform_keys' for {}:Hash (NoMethodError) # Did you mean? transform_values # # == 2.5.6 == # {}

Is this method being hit during our test runs? It should show up as a failure in 2.1-2.4 test cases, if so.

It's used in spec/datadog/tracing/client_ip_spec, odd. I'll try to see why it isn't called.

For some reason these tests don't fail in the pipeline:
https://app.circleci.com/pipelines/github/DataDog/dd-trace-rb/7179/workflows/73531d46-88fe-4910-af07-bb3d97451c23/jobs/265659/parallel-runs/1?filterBy=ALL
https://app.circleci.com/pipelines/github/DataDog/dd-trace-rb/7179/workflows/73531d46-88fe-4910-af07-bb3d97451c23/jobs/265659/parallel-runs/2?filterBy=ALL

Add support for http.client_ip tag for Rack-based frameworks

70440ae

barweiss requested a review from a team August 30, 2022 12:44

lloeki requested changes Aug 30, 2022

View reviewed changes

Address @lloeki's great review comments

cbfca12

barweiss requested a review from lloeki August 31, 2022 07:05

barweiss added 2 commits August 31, 2022 10:41

Fix linter issues

010066e

Revert accidental changes to http instrumentation

0c9eecb

barweiss commented Aug 31, 2022

View reviewed changes

Fix require line

ee61750

TonyCTHsu reviewed Aug 31, 2022

View reviewed changes

barweiss added 4 commits August 31, 2022 17:31

Fix comments from @TonyCTHsu's awesome review

ca4c899

Fix bad 'set_client_ip' call in Rack middleware

d9532ca

Fix Ruby 2.2 compatibilty issue

5d22113

Fix rubocop stuff that only appears in circleci

ef6f427

barweiss requested a review from TonyCTHsu September 1, 2022 07:55

barweiss added 3 commits September 5, 2022 19:09

Rename configuration option 'disabled' -> 'enabled', revert to using …

00f3c7d

…'ipaddr'

Remove 'ipaddress' from LICENSE-3rdparty

f49d03e

Update some documentation

2dae9e6

TonyCTHsu approved these changes Sep 6, 2022

View reviewed changes

Fix IPAddr usage for Ruby <= 2.3

76e6e0a

lloeki approved these changes Sep 7, 2022

View reviewed changes

marcotc reviewed Sep 8, 2022

View reviewed changes

barweiss and others added 2 commits September 8, 2022 14:07

Fix Ruby 2.1-2.4 compatibility issue

ea3b064

Merge branch 'master' into bar.weiss/client-ip-rack-support

7bbb322

marcotc approved these changes Sep 8, 2022

View reviewed changes

marcotc merged commit d2ba9c9 into master Sep 8, 2022

marcotc deleted the bar.weiss/client-ip-rack-support branch September 8, 2022 18:58

marcotc added the integrations Involves tracing integrations label Sep 8, 2022

github-actions bot added this to the 1.5.0 milestone Sep 8, 2022

lloeki mentioned this pull request Oct 24, 2022

Default to disable http.client_ip tag collection #2321

Merged

ivoanjo mentioned this pull request Mar 6, 2023

[APPSEC-7946] Update the client IP extraction resolution base on the latest RFC. #2665

Merged

	# An optional name of a custom header to resolve the client IP from.

	# An optional name of a custom header to resolve the client IP from.

	case ip_values_from_headers.size

	case ip_values_from_headers.size

Add support for http.client_ip tag for Rack-based frameworks #2248

Add support for http.client_ip tag for Rack-based frameworks #2248

Conversation

barweiss commented Aug 30, 2022

lloeki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barweiss Aug 30, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barweiss Aug 30, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barweiss Aug 30, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TonyCTHsu Aug 31, 2022 • edited

Choose a reason for hiding this comment

barweiss Aug 31, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TonyCTHsu Aug 31, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barweiss commented Sep 5, 2022

lloeki commented Sep 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add support for `http.client_ip` tag for Rack-based frameworks #2248

Add support for `http.client_ip` tag for Rack-based frameworks #2248

barweiss Aug 30, 2022 •

edited

barweiss Aug 30, 2022 •

edited

barweiss Aug 30, 2022 •

edited

TonyCTHsu Aug 31, 2022 •

edited

barweiss Aug 31, 2022 •

edited

TonyCTHsu Aug 31, 2022 •

edited