stream info: cleanup address handling

[mattklein123]

Consolidate all downstream address handling setting into a single
function. Also remove duplicate setting in the connection handler.
This should make this logic less error prone than it was previously.

Fixes #14133

Risk Level: Low
Testing: Existing tests
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A

[mattklein123]

/retest

[repokitteh-read-only]

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #14432 (comment) was created by @mattklein123.

see: more, trace.

[ggreenway]

I really like the approach. This cleans things up quite a bit.

[mattklein123]

@ggreenway I pushed a partial change which implements a variant of your suggestion which is to fundamentally fix the problem by having an address provider that we pass around via shared_ptr so it's not possible for anything to get out of date. I like this solution, but it's a lot of work so I don't want to spend more time on fixing all the tests and various pieces of code unless you agree with the direction. Also, there is a small downside of an extra shared_ptr allocation per connection, and there will be another one on top of that for HTTP connections that change remote address due to XFF (to have a provider that wraps the original). IMO this is worth it given the severity of possible bugs in this area, but let me know what you think!

[ggreenway]

I like that the connection and streaminfo addresses are combined.

I'm not sure I like that the http streaminfo and connection streaminfo sometimes share an address. If XFF has a different address than the connection remoteAddress, the http streaminfo will need to replace it's addressprovider.

Given that the http and connection addresses aren't necessarily the same, I think it may make sense for the http streaminfo to get a one-time copy of the addresses in the connection streaminfo when it is constructed, and from there are be separate/uncoupled.

[mattklein123]

We discussed ^ offline and my plan is to make a special version of StreamInfo, OverridableAddressStrreamInfo, or whatever, that implements SocketAddressProvider and also has a setDownstreamAddress function. Then, the overriding will happen in that stream info and pass through otherwise. I think it will be pretty clean. It can be self contained to the HCM code.

[mattklein123]

@ggreenway I need to take another pass over this but I think this should be ready for review. Sorry for the size of the change but I think it's the right thing to do. It should mostly be a mechanical review.

[mattklein123]

@ggreenway I went over the change again and I'm happy with it. I don't think tidy is doing to pass given the size of the change. No rush on reviewing.

[ggreenway]

This looks great overall! A couple minor nits, but the approach looks good, and this cleaned up the source of bugs.

[mattklein123]

@ggreenway updated PTAL

[ggreenway]

LGTM.

This will give your github stats a nice boost.

[mattklein123]

@ggreenway I had to fix a merge conflict so I will need another review. cc @KBaichoo see last commit.

[rgs1]

@mattklein123 so OverridableRemoteSocketAddressSetterStreamInfo isn't visible to filters? I have a filter where we do decoder_callbacks_->streamInfo().setDownstreamRemoteAddress() so that the ip tagging filter can then do the proper thing (XFF is broken for us)... Was the idea to lock things up so filters can't call setDownstreamoRemoteAddress() anymore?

[mattklein123]

@rgs1 and I just had a really long convo on Slack about ^. @rgs1 after you digest can you circle back with the proposed plan?

[rgs1]

Ok, so circling back.

Our options are:

1. we can add an extension system or hook for detecting and populating the downstream remote address (in the spirit of request_id_utils: add new extension system #10429). With this, we could push our logic into the early stage of the HCM so that it can happen before the other stuff that relies on IP detection (e.g.: header sanitation).
2. we can extend the ip tagging filter to have it lookup the remote ip from a header, instead of relying on stream info (ditto for our internal filters). This is somewhat elegant, given that upstream services rely on a header for this already. But requires some juggling to roll this out.
3. we can accrue some tech debt and reinsert back setDownstreamRemoteAddress() into StreamInfo, while we make 1) or 2) happen.

I really wanted to avoid 3), given the work that's gone into cleaning up StreamInfo but after some thought I think I'd prefer to send a PR to get that going. 1) is the most correct approach, but I don't have the immediate cycles for that. 2) is easier but requires some juggling in terms of rollout so the time doing that is probably better spent doing 1).

So how about:

• I send a PR for 3) with a big TODO comment that 1) needs to happen
• I start working on 1) in the background with a goal of getting land somewhere in Q1

Thoughts?

[rgs1]

Ok with #14633 we can do this from our filter:

+    //
+    // Hack: this is workaround to:
+    //
+    // https://github.com/envoyproxy/envoy/pull/14432#issuecomment-758004199
+    //
+    // We need the dynamic_cast to be able to set the downstream remote address _and_
+    // we need the catch std::bad_cast because the cast fails during tests because
+    // of the mocked decoder callbacks. Sigh.
+    try {
+      auto& stream_info = dynamic_cast<Http::OverridableRemoteSocketAddressSetterStreamInfo&>(
+          decoder_callbacks_->streamInfo());
+      stream_info.setDownstreamRemoteAddress(
+          Network::Utility::parseInternetAddress(trusted_client_ip_));
+    } catch (std::bad_cast&) {
+    }

I am not massively proud, but it probably works.

[mattklein123]

Yeah per offline convo I think that is your best bet until we get something better implemented for you.