feat: Support Client IP Detection using Custom Header#2566
feat: Support Client IP Detection using Custom Header#2566arkodg merged 3 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: David Alger <davidmalger@gmail.com>
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2566 +/- ##
==========================================
- Coverage 64.49% 63.96% -0.53%
==========================================
Files 116 117 +1
Lines 17790 18098 +308
==========================================
+ Hits 11473 11576 +103
- Misses 5575 5769 +194
- Partials 742 753 +11 ☔ View full report in Codecov by Sentry. |
Signed-off-by: David Alger <davidmalger@gmail.com>
Signed-off-by: David Alger <davidmalger@gmail.com>
| var useRemoteAddress = true | ||
| var originalIPDetectionExtensions = originalIPDetectionExtensions(irListener.ClientIPDetection) | ||
| if originalIPDetectionExtensions != nil { | ||
| useRemoteAddress = false |
There was a problem hiding this comment.
does this have to be false ?
this knobs helps the next upstream with middleware client ip info, unsure how its tied to downstream client ip info which is being used to determine true client ip for logging, acl, ratelimiting
There was a problem hiding this comment.
does this have to be false? this knobs helps the next upstream with middleware client ip info
It has to be false. Both top-level xff_num_trusted_hops and use_remote_address are mutually exclusive with original_ip_detection_extensions. There's a note in the docs about it, and if you try setting it to true while also configuring original_ip_detection_extensions envoy will reject the config:
[2024-02-07 22:30:29.118][1][warning][config] [source/extensions/config_subscription/grpc/grpc_subscription_impl.cc:138] gRPC config for type.googleapis.com/envoy.config.listener.v3.Listener rejected: Error adding/updating listener(s) default/eg/http: Original IP detection extensions and use_remote_address may not be mixed
And yes, this does mean when customHeader is configured the X-Forwarded-For header will not have the envoy proxy IP appended to it since use_remote_address is false and it's assumed you aren't relying on the XFF headers when using alternate forms of client IP detection.
unsure how its tied to downstream client ip info which is being used to determine true client ip for logging, acl, ratelimiting
The value of the named header will be present in log entries as the downstream_remote_address, so I would assume that it would still be used for ACLs and rate limits.
AllowExtensionToSetAddressAsTrusted: true is set on the extension which at minimum allows the address to also be used to determine if the address is "internal" or not based on the CIDR range.
|
@arkodg anything required from me to get this one across the finish line? |
waiting for another review from a maintainer before merging this one, cc @zhaohuabing @zirain |
|
|
||
| // ClientIPDetectionSettings provides configuration for determining the original client IP address for requests. | ||
| // | ||
| // +kubebuilder:validation:XValidation:rule="!(has(self.xForwardedFor) && has(self.customHeader))",message="customHeader cannot be used in conjunction with xForwardedFor" |
There was a problem hiding this comment.
One of 'xForwordFor' and 'customHeader' must be specified? Or default to 'xForwadFor' ?
There was a problem hiding this comment.
That is correct. The top-level xff_num_trusted_hops knob in Envoy is mutually exclusive with original_ip_detection_extensions. Even if Envoy supported both simultaneously though, I'm not sure it would make sense to have two methods of downstream client IP detection configured at the same time as it would muddy where the logged downstream IP address came from.
When customHeader is specified, any XFF headers are essentially ignored.
There was a problem hiding this comment.
One of 'xForwordFor' and 'customHeader' must be specified?
Can we also add a CEL validation for that?
There was a problem hiding this comment.
@zhaohuabing I think I misunderstood your question at first. They are mutually exclusive, but neither one is required. When both are omitted, they are simply not configured and the default behavior is to use the remote address which is correct when Envoy is directly exposed to clients rather than residing behind a CDN or cloud-provider LB. Not sure any additional CEL validation is needed here.
|
/retest |
Between envoyproxy#2566 & envoyproxy#2585 Signed-off-by: Arko Dasgupta <arko@tetrate.io>
What type of PR is this?
feat: Support Client IP Detection using Custom Header
What this PR does / why we need it:
Similar to #2535 this extends ClientTrafficPolicy to support using a custom header rather than XFF headers to perform Client IP Detection. Where envoy proxies reside behind a CDN or a cloud providers global LBs, this is useful as often the client IP is provided to the backend by the downstream CDN/LB proxy when handling the request.
Two examples:
"customRequestHeaders":["x-client-ip-address:{client_ip_address}"]in which case thex-client-ip-addressmay be used to determine the address in a more favorable manner than counting number of hops in the XFF header.True-Client-IPheader enabled to include the end user’s IP address on all requests to envoy proxies.Proposed configuration support on
ClientTrafficPolicy: