Codecov Report

Attention: 19 lines in your changes are missing coverage. Please review.

Comparison is base (1754679) 64.50% compared to head (e23da95) 64.46%.

@@            Coverage Diff             @@
##             main    #2535      +/-   ##
==========================================
- Coverage   64.50%   64.46%   -0.05%     
==========================================
  Files         116      116              
  Lines       17757    17790      +33     
==========================================
+ Hits        11455    11468      +13     
- Misses       5560     5579      +19     
- Partials      742      743       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

thanks for building this out @davidalger . Here's how I'm hoping we can make progress in this PR

• get some consensus that ClientTrafficPolicy is the right home for this field vs in EnvoyProxy
  Youve already highlighted this - since EnvoyProxy attaches to a GatewayClass, it can affect multiple Gateways and corresponding Envoy Proxy fleets, so introducing the field in ClientTrafficPolicy does make sense, im a +1 for this
• I'd prefer if we created a top level field called headers to hold all fields that affect proxy header transformations. Once we have consensus on this, we'd need to make that change first @liorokman
• I'd prefer if we skipped introducing the use_remote_address field in this PR

• I'd prefer if we created a top level field called headers to hold all fields that affect proxy header transformations. Once we have consensus on this, we'd need to make that change first @liorokman

I'm +1 for that. In fact, I've already proposed this in #2500 .

I'd prefer if we created a top level field called headers to hold all fields that affect proxy header transformations. Once we have consensus on this, we'd need to make that change first @liorokman

I like the idea of organizing things better, but not 100% sure headers is the right place to put it since XFF isn't just about header transformation and falls more into general IP detection territory.

@arkodg Since HCM supports either a combination of use_remote_address and xff_num_trusted_hops or using original_ip_detection_extensions for this, what are your thoughts on the following structure?

originalIpDetection:
  useRemoteAddress: true    # True by default, false if `extensions` is specified (not present in CRD, as suggested)
  xffNumTrustedHops: number # Mutually exclusive with `extensions` configures `xff_num_trusted_hops` HCM built-in
  extensions:
    # Configures type.googleapis.com/envoy.extensions.http.original_ip_detection.custom_header.v3.CustomHeaderConfig
    customHeader:
      headerName: string
      rejectWithStatus: enum
    # Configures type.googleapis.com/envoy.extensions.http.original_ip_detection.xff.v3.XffConfig
    xff:
      numTrustedHops: number

Supporting config for the custom headers IP detection extension will be of use to me in deployments of EG, so even if precluded from this MR for sake of keeping it single-purpose I would like to consider planning for it and can introduce it in a future change set.

so we have two options here, (please ignore the exact names of the fields)

1. add this config under a newly created headers field

headers:
  xForwardedFor:
    numTrustedHops:

2. add this config under a dedicated field around IP detection / network topology

ipDetection:
  numTrustedHops

cc @envoyproxy/gateway-maintainers
will bring this up in the community meeting this week

discussed this in the community meeting today and folks prefer 2 over 1 (generic headers section)

• brainstorming some names - what should the top level field be - remoteIP or originalIP or remoteIPDetection or originalIPDetection or originatingIP.... or clientIP..... Whichever term we decide will need to be used in other APIs (access control and RateLimit)

• sidenote (not related to this PR) - should proxy protocol config be moved under this field ? cc @envoyproxy/gateway-maintainers

brainstorming some names - what should the top level field be - remoteIP or originalIP or remoteIPDetection or originalIPDetection or originatingIP.... or clientIP..... Whichever term we decide will need to be used in other APIs (#2250 and RateLimit)

clientIPDetection makes sense to me given the term 'client IP' lines up with what's used used to document sourceCIDR on RateLimit already and also has widespread use across the broader industry (Google LBs, CloudFlare, Nginx, etc) and it's meaning should be widely understood

brainstorming some names - what should the top level field be - remoteIP or originalIP or remoteIPDetection or originalIPDetection or originatingIP.... or clientIP..... Whichever term we decide will need to be used in other APIs (#2250 and RateLimit)

clientIPDetection makes sense to me given the term 'client IP' lines up with what's used used to document sourceCIDR on RateLimit already and also has widespread use across the broader industry (Google LBs, CloudFlare, Nginx, etc) and it's meaning should be widely understood

sg @davidalger, lets go with clientIpDetection !

@arkodg Implemented first pass here with the following structure:

clientIPDetection:
  xffNumTrustedHops: number # Mutually exclusive with `extensions` configures `xff_num_trusted_hops` HCM built-in
  extensions:
    # Configures type.googleapis.com/envoy.extensions.http.original_ip_detection.custom_header.v3.CustomHeaderConfig
    customHeader:
      headerName: string
      rejectWithStatus: number
    # Configures type.googleapis.com/envoy.extensions.http.original_ip_detection.xff.v3.XffConfig
    xff:
      numTrustedHops: number

use_remote_address maintains it's default of true while being set to false if original IP detection extensions are used since the two are mutually exclusive

Once envoyproxy/envoy#31831 is merged, I would see there being an additional field to control the trusted CIDRs:

clientIPDetection:
  extensions:
    xff:
      trustedCIDRs:
      - "130.211.0.0/22"
      - "35.191.0.0/16"

Would it be better to keep things a little more flat given this won't support arbitrary extensions?

clientIPDetection:
    xffNumTrustedHops: 2
    customHeaderExtension:
      // ...
    xffExtension:
      // ...

@davidalger curious if you know the difference between the numTrustedHops field in the top level vs extension setting in envoy ?

curious if you know the difference between the numTrustedHops field in the top level vs extension setting in envoy?

At the moment the only difference I believe is the extensions were marked incompatible with use_remote_address, but otherwise the XFF extension is identical in function currently.

It's been asked by maintainers to implement additions to XFF handling in original_ip_detection_extensions and in the coming days it's looking like sourceCIDRs support will be added to the XFF extension (envoyproxy/envoy#31831) and notably it will evaluate the x-forwarded-for header either by a fixed number of trusted hops, or by evaluating the client IP address against a list of trusted addresses.

So something that could be considered is supporting numTrustedHops at the top-level and sourceCIDRs (potential future feature) would use the extension but without any plans to support the seemingly duplicate numTrustedHops field.

Something like this I imagine:

clientIPDetection:
    xForwardedFor:
      # Top-level xff_num_trusted_hops field with no support for same field in XFF extension
      numTrustedHops: 2

      # Future ability to skip appending to xff header
      skipAppend: true

      # Future ability to evaluate xff header based on trusted CIDRs rather than number of hops (uses extension)
      sourceCIDRs:
      - "130.211.0.0/22"
      - "35.191.0.0/16"

    # Configures type.googleapis.com/envoy.extensions.http.original_ip_detection.custom_header.v3.CustomHeaderConfig
    customHeader:
      headerName: x-client-ip-address
      rejectWithStatus: 403

Anything other than numTrustedHops would result in use_remote_address being set to false due to incompatibility with extensions, otherwise the current default of true would apply.

I'll update this MR to only add the ability to set numTrustedHops and tackle customHeader separately after this is complete.

curious if you know the difference between the numTrustedHops field in the top level vs extension setting in envoy?

At the moment the only difference I believe is the extensions were marked incompatible with use_remote_address, but otherwise the XFF extension is identical in function currently.

It's been asked by maintainers to implement additions to XFF handling in original_ip_detection_extensions and in the coming days it's looking like sourceCIDRs support will be added to the XFF extension (envoyproxy/envoy#31831) and notably it will evaluate the x-forwarded-for header either by a fixed number of trusted hops, or by evaluating the client IP address against a list of trusted addresses.

So something that could be considered is supporting numTrustedHops at the top-level and sourceCIDRs (potential future feature) would use the extension but without any plans to support the seemingly duplicate numTrustedHops field.

Something like this I imagine:

clientIPDetection:
    xForwardedFor:
      # Top-level xff_num_trusted_hops field with no support for same field in XFF extension
      numTrustedHops: 2

      # Future ability to skip appending to xff header
      skipAppend: true

      # Future ability to evaluate xff header based on trusted CIDRs rather than number of hops (uses extension)
      sourceCIDRs:
      - "130.211.0.0/22"
      - "35.191.0.0/16"

    # Configures type.googleapis.com/envoy.extensions.http.original_ip_detection.custom_header.v3.CustomHeaderConfig
    customHeader:
      headerName: x-client-ip-address
      rejectWithStatus: 403

Anything other than numTrustedHops would result in use_remote_address being set to false due to incompatibility with extensions, otherwise the current default of true would apply.

I'll update this MR to only add the ability to set numTrustedHops and tackle customHeader separately after this is complete.

sounds like a good plan ! this keeps any envoyisms out of the API increasing its ability to be moved into the upstream gateway api in the future

@arkodg updated and should be ready for review

PR looks good @davidalger, added a minor comment around optional fields

need a user doc for this?

need a user doc for this?

was part of this PR https://gateway.envoyproxy.io/latest/user/client-traffic-policy/#configure-client-ip-detection