Add support for `http.client_ip` tag for Rack-based frameworks by barweiss · Pull Request #2248 · DataDog/dd-trace-rb

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

+        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

+      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

+              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

+      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

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


I feel like this could go to the Core namespace.

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

+          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

+        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

+        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


-          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

+        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

+      # @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

+            #
+            # @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

+      # @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

+
+  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

+      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

+    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

Bar Weiss 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

Bar Weiss 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

Bar Weiss 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

Bar Weiss 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

Uh oh!

Conversation

barweiss commented Aug 30, 2022

Uh oh!

lloeki left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

barweiss Aug 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

barweiss Aug 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

barweiss Aug 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TonyCTHsu Aug 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

barweiss Aug 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TonyCTHsu Aug 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

barweiss Aug 30, 2022 •

edited

Loading

barweiss Aug 30, 2022 •

edited

Loading

barweiss Aug 30, 2022 •

edited

Loading

TonyCTHsu Aug 31, 2022 •

edited

Loading

barweiss Aug 31, 2022 •

edited

Loading

TonyCTHsu Aug 31, 2022 •

edited

Loading