HCM: add support for IP detection extensions by rgs1 · Pull Request #14855 · envoyproxy/envoy

rgs1 · 2021-01-28T18:45:51Z

This is a follow-up to:

After that PR, it's no longer possible (unless you do a dynamic_cast)
to set the remote address from a filter. This is something that we
need to do because we have specialized logic for this (XFF doesn't
work for us).

So this adds an extension point which will allow us to push that logic
down to ConnectionManagerUtility::mutateRequestHeaders() where it
belongs.

Signed-off-by: Raul Gutierrez Segales rgs@pinterest.com

This is a follow-up to: envoyproxy#14432 (comment) After that PR, it's no longer possible (unless you do a dynamic_cast) to set the remote address from a filter. This is something that we need to do because we have specialized logic for this (XFF doesn't work for us). So this adds an extension point which will allow us to push that logic down to ConnectionManagerUtility::mutateRequestHeaders() where it belongs. Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

repokitteh-read-only · 2021-01-28T18:45:58Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
API shepherd assignee is @mattklein123
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #14855 was opened by rgs1.

see: more, trace.

alyssawilk

This is going to be awesome - thanks for adding it!

Two actual comments (ignoring the usual "docs, config examples, blah blah" which I'm sure you'll do as this goes out of WIP)

also cc @snowp in case he's interested and @antoniovicente @penguingao @AndresGuedez for visibility - we may want to move our own client IP calculations into an in-house extension.

api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto

include/envoy/http/ip_detection_extension.h

…tension-point

alyssawilk · 2021-01-28T22:07:55Z

I think you could get away with original_ip_detection as I don't know anyone who does this for upstream connections and it makes it a tiny bit less verbose :-P

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

…tension-point

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

Not sure where to put a full example of how to configure an extension. Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

htuch

Providing a review pass for API shepherds. @phlax are there any documentation requirements from your side?

api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto

phlax · 2021-01-31T10:01:06Z

@phlax are there any documentation requirements from your side?

im thinking that it would be useful to add a sandbox that demonstrated features/functonality of this filter

also, it doesnt seem like there any ref docs, so im not 100% about how it works without tracking through source and figuring out

im happy to add a sandbox, but it would be good if there was some explanation that i can work from

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

…tension-point

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

…tension-point

rgs1 · 2021-02-02T00:12:16Z

@phlax are there any documentation requirements from your side?

im thinking that it would be useful to add a sandbox that demonstrated features/functonality of this filter

also, it doesnt seem like there any ref docs, so im not 100% about how it works without tracking through source and figuring out

im happy to add a sandbox, but it would be good if there was some explanation that i can work from

Yup -- I'll do another pass now and add proper examples/docs.

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

htuch

Looks good, but a few API comments.
/wait

api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto

api/envoy/extensions/http/original_ip_detection/custom_header/v3/custom_header.proto

…tension-point

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

rgs1 · 2021-05-14T02:22:47Z

The gcc failure is our old friend:

fatal error: cannot execute ‘/usr/lib/gcc/x86_64-linux-gnu/9/cc1plus’: execv: Argument list too long

cc: @yanavlasov @phlax

rgs1 · 2021-05-14T02:24:39Z

@htuch API is ready for another pass, I think I addressed most of your concerns directly (or indirectly). Thanks!

htuch

LGTM modulo nits.

api/envoy/extensions/http/original_ip_detection/custom_header/v3/custom_header.proto

api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

…tension-point

htuch

/lgtm api

rgs1 · 2021-05-14T16:39:30Z

I don't think the x64 coverage failure is related:

FAIL: //test/integration:protocol_integration_test (shard 3 of 5) (see /build/tmp/_bazel_envoybuild/b570b5ccd0454dc9af9f65ab1833764d/execroot/envoy/bazel-out/k8-fastbuild/testlogs/test/integration/protocol_integration_test/shard_3_of_5/test.log)
stdout (/build/tmp/_bazel_envoybuild/b570b5ccd0454dc9af9f65ab1833764d/execroot/envoy/bazel-out/_tmp/actions/stdout-18662) exceeds maximum size of --experimental_ui_max_stdouterr_bytes=1048576 bytes; skipping

Looking into it...

rgs1 · 2021-05-14T16:41:34Z

I don't think the x64 coverage failure is related:

FAIL: //test/integration:protocol_integration_test (shard 3 of 5) (see /build/tmp/_bazel_envoybuild/b570b5ccd0454dc9af9f65ab1833764d/execroot/envoy/bazel-out/k8-fastbuild/testlogs/test/integration/protocol_integration_test/shard_3_of_5/test.log)
stdout (/build/tmp/_bazel_envoybuild/b570b5ccd0454dc9af9f65ab1833764d/execroot/envoy/bazel-out/_tmp/actions/stdout-18662) exceeds maximum size of --experimental_ui_max_stdouterr_bytes=1048576 bytes; skipping

Looking into it...

That's not real the error, trying to find the test log to see what happened.

rgs1 · 2021-05-14T16:44:12Z

/retest

repokitteh-read-only · 2021-05-14T16:44:15Z

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #14855 (comment) was created by @rgs1.

see: more, trace.

…tension-point

This is a follow-up to: After that PR, it's no longer possible (unless you do a dynamic_cast) to set the remote address from a filter. This is something that we need to do because we have specialized logic for this (XFF doesn't work for us). So this adds an extension point which will allow us to push that logic down to ConnectionManagerUtility::mutateRequestHeaders() where it belongs.

This is a follow-up to: After that PR, it's no longer possible (unless you do a dynamic_cast) to set the remote address from a filter. This is something that we need to do because we have specialized logic for this (XFF doesn't work for us). So this adds an extension point which will allow us to push that logic down to ConnectionManagerUtility::mutateRequestHeaders() where it belongs Signed-off-by: Raúl Gutiérrez Segalés <rgs@pinterest.com>

This is a follow-up to: envoyproxy#14432 (comment) After that PR, it's no longer possible (unless you do a dynamic_cast) to set the remote address from a filter. This is something that we need to do because we have specialized logic for this (XFF doesn't work for us). So this adds an extension point which will allow us to push that logic down to ConnectionManagerUtility::mutateRequestHeaders() where it belongs. Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com> Signed-off-by: Sixiang Gu <sgu@twitter.com>

KBaichoo · 2021-05-20T19:35:44Z

test/common/http/ip_detection_extensions.cc

+  return std::make_shared<Extensions::Http::OriginalIPDetection::Xff::XffIPDetection>(hops);
+}
+
+Http::OriginalIPDetectionSharedPtr getCustomHeaderExtension(const std::string& header_name) {


@rgs1 I think this file violates that extensions should be independent of each other, requiring both extensions to be enabled.

WDYT is the right way to get around this? Does this just null the point of this file?

This file is there to workaround the Envoy::Http vs Envoy::Extensions::Http namespaces pollution. How why are both extensions required?

I think from the bazel perspective the if you disable one of them here:

envoy/source/extensions/extensions_build_config.bzl

Line 274 in 19abe17

# Original IP detection

it'll only stop the factory registration. So if you want to test just one of them, and you use this file, both of these extensions end up being linked into the binary I believe with the other one (or N other IP detection extension if this pattern is repeated in the future) would just end up as dead code.

Seems like it's probably better to link the minimal set of what you'd use in a given situation.

possibly related - i was trying to update the envoy checkout on envoy-mobile (envoyproxy/envoy-mobile#1481)

i had to link in the xff code - its failing, possibly for some other reason - but something tells me that this will also cause an issue

not totally sure - as xff at least is listed in all_extensions.bzl which kinda overrides envoy_build_config.bzl

hmm it's also here:

# The map may be overridden by extensions specified in envoy_build_config. _required_extensions = { "envoy.common.crypto.utility_lib": "//source/extensions/common/crypto:utility_lib", + "envoy.http.original_ip_detection.xff": "//source/extensions/http/original_ip_detection/xff:config", "envoy.request_id.uuid": "//source/extensions/request_id/uuid:config", "envoy.transport_sockets.tls": "//source/extensions/transport_sockets/tls:config", }

…cab (#8346) * xds: sync envoy proto to commit 62ca8bd2b5960ed1c6ce2be97d3120cee719ecab * Suppress warnings for newly deprecated xDS proto fields Sync to the latest update to pick up envoyproxy/envoy#16942 for forward compatibility with upcoming xDS Rate Limiting features. Internal Envoy import CL for `62ca8bd2b5960ed1c6ce2be97d3120cee719ecab`: cl/381356375 Suppressed warnings for newly deprecated xDS proto fields: 1) `PerXdsConfig xds_config` to be replaced with `GenericXdsConfig generic_xds_configs`, but this work yet to be planned 2) `HttpConnectionManager`'s `uint32 setXffNumTrustedHops` to be replaced with `TypedExtensionConfig OriginalIpDetectionExtensions`: envoyproxy/envoy#14855

rgs1 requested review from alyssawilk and mattklein123 as code owners January 28, 2021 18:45

repokitteh-read-only bot added the api label Jan 28, 2021

repokitteh-read-only bot assigned mattklein123 Jan 28, 2021

alyssawilk self-assigned this Jan 28, 2021

snowp self-assigned this Jan 28, 2021

alyssawilk reviewed Jan 28, 2021

View reviewed changes

api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto Outdated Show resolved Hide resolved

include/envoy/http/ip_detection_extension.h Outdated Show resolved Hide resolved

Merge remote-tracking branch 'upstream/main' into add-ip-detection-ex…

e55d622

…tension-point

Raul Gutierrez Segales added 4 commits January 29, 2021 15:59

Rename --> original_ip_detection

eee3cd5

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

Merge remote-tracking branch 'upstream/main' into add-ip-detection-ex…

e8d890a

…tension-point

Update changelog

03819df

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

Some docs

701e979

Not sure where to put a full example of how to configure an extension. Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

mattklein123 added the waiting label Jan 30, 2021

htuch reviewed Jan 31, 2021

View reviewed changes

Raul Gutierrez Segales added 4 commits February 1, 2021 16:39

Add HCM config tests

68c9785

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

Merge remote-tracking branch 'upstream/main' into add-ip-detection-ex…

c525fa0

…tension-point

Add test for mutateRequestHeaders() using an extension

b42737b

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

Merge remote-tracking branch 'upstream/main' into add-ip-detection-ex…

9a4421d

…tension-point

repokitteh-read-only bot removed the waiting label Feb 1, 2021

Raul Gutierrez Segales added 2 commits February 1, 2021 20:28

Use TypedExtensionConfig

09a28e2

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

Extensive comments around the config message

2358acb

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

rgs1 changed the title ~~[WIP] HCM: add support for IP detection extensions~~ HCM: add support for IP detection extensions Feb 2, 2021

Raul Gutierrez Segales added 3 commits February 2, 2021 10:25

Fix test

445d0b2

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

Fix another test

c8b4115

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

clang-tidy fixes

0868c1e

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

htuch suggested changes Apr 30, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Apr 30, 2021

Raul Gutierrez Segales added 3 commits May 13, 2021 14:43

Merge remote-tracking branch 'upstream/main' into add-ip-detection-ex…

f9a372c

…tension-point

Fix merge

892a4a8

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

Trim down comment for original_ip_detection_extensions

fd10840

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

rgs1 dismissed alyssawilk’s stale review via fd10840 May 13, 2021 19:36

repokitteh-read-only bot removed the waiting label May 13, 2021

Raul Gutierrez Segales added 3 commits May 13, 2021 16:59

More fixes

3ef96b1

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

Field name update

f841b5f

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

Fix

c4a21e7

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

htuch reviewed May 14, 2021

View reviewed changes

api/envoy/extensions/http/original_ip_detection/custom_header/v3/custom_header.proto Outdated Show resolved Hide resolved

api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto Outdated Show resolved Hide resolved

Raul Gutierrez Segales added 2 commits May 14, 2021 10:07

htuch's review

166eddb

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

Merge remote-tracking branch 'upstream/main' into add-ip-detection-ex…

43ff219

…tension-point

htuch approved these changes May 14, 2021

View reviewed changes

repokitteh-read-only bot removed the api label May 14, 2021

Merge remote-tracking branch 'upstream/main' into add-ip-detection-ex…

d53e920

…tension-point

htuch merged commit beac1ec into envoyproxy:main May 16, 2021

KBaichoo reviewed May 20, 2021

View reviewed changes

sergiitk mentioned this pull request Jul 26, 2021

xds: sync envoy proto to commit 62ca8bd2b5960ed1c6ce2be97d3120cee719ecab grpc/grpc-java#8346

Merged

rgs1 mentioned this pull request Jul 30, 2021

IP detection regresion #17554

Closed

Conversation

rgs1 commented Jan 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Jan 28, 2021

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alyssawilk commented Jan 28, 2021

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

phlax commented Jan 31, 2021

Uh oh!

rgs1 commented Feb 2, 2021

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rgs1 commented May 14, 2021

Uh oh!

rgs1 commented May 14, 2021

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

rgs1 commented May 14, 2021

Uh oh!

rgs1 commented May 14, 2021

Uh oh!

rgs1 commented May 14, 2021

Uh oh!

repokitteh-read-only bot commented May 14, 2021

Uh oh!

KBaichoo May 20, 2021

Choose a reason for hiding this comment

Uh oh!

rgs1 May 20, 2021

Choose a reason for hiding this comment

Uh oh!

KBaichoo May 21, 2021

Choose a reason for hiding this comment

Uh oh!

phlax May 21, 2021

Choose a reason for hiding this comment

Uh oh!

rgs1 May 21, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

rgs1 commented Jan 28, 2021 •

edited

Loading