Support specific dynamic request headers

[erwbgy]

Please describe the problem you have
From https://www.envoyproxy.io/docs/envoy/latest/configuration/http/http_conn_man/headers#custom-request-response-headers:

Envoy supports adding dynamic values to request and response headers. The percent symbol (%) is used to delimit variable names...
Supported variable names are:
%DOWNSTREAM_REMOTE_ADDRESS%
Remote address of the downstream connection. If the address is an IP address it includes both address and port.
...

I would like to use these dynamic values to set request headers - for example:

spec:
  routes:
  - services:
    - name: myservice
      port: 80
      requestHeadersPolicy:
        set:
        - name: X-Envoy-Hostname
          value: '%HOSTNAME%'

but currently the % is escaped so the result is a literal %HOSTNAME% rather than the expected dynamic value.

There have also been other requests like #2516 to set header values based on other header values.

There are legitimate concerns about sending invalid data to Envoy (see #1176 and #2569) as invalid custom headers cause route configuration failures - for example testing using a patched contour that allows %UNKNOWN% through:

      requestHeadersPolicy:
        set:
        - name: X-Envoy-unknown
          value: '%UNKNOWN%'  

results in:

envoy-wpvz4 envoy [2020-12-27 14:37:57.945][1][warning][config] [source/common/config/grpc_subscription_impl.cc:107] 
gRPC config for type.googleapis.com/envoy.config.route.v3.RouteConfiguration rejected: field 'UNKNOWN' not supported
 as custom header

The proposal is to only allow a specific set of known good Envoy dynamic headers to pass through unescaped and continue escaping the % for anything else:

• %DOWNSTREAM_REMOTE_ADDRESS%
• %DOWNSTREAM_REMOTE_ADDRESS_WITHOUT_PORT%
• %DOWNSTREAM_LOCAL_ADDRESS%
• %DOWNSTREAM_LOCAL_ADDRESS_WITHOUT_PORT%
• %DOWNSTREAM_LOCAL_PORT%
• %DOWNSTREAM_LOCAL_URI_SAN%
• %DOWNSTREAM_PEER_URI_SAN%
• %DOWNSTREAM_LOCAL_SUBJECT%
• %DOWNSTREAM_PEER_SUBJECT%
• %DOWNSTREAM_PEER_ISSUER%
• %DOWNSTREAM_TLS_SESSION_ID%
• %DOWNSTREAM_TLS_CIPHER%
• %DOWNSTREAM_TLS_VERSION%
• %DOWNSTREAM_PEER_FINGERPRINT_256%
• %DOWNSTREAM_PEER_FINGERPRINT_1%
• %DOWNSTREAM_PEER_SERIAL%
• %DOWNSTREAM_PEER_CERT%
• %DOWNSTREAM_PEER_CERT_V_START%
• %DOWNSTREAM_PEER_CERT_V_END%
• %HOSTNAME%
• %PROTOCOL%
• %REQ(header-name)%
• %START_TIME%
• %UPSTREAM_REMOTE_ADDRESS%
• %RESPONSE_FLAGS%
• %RESPONSE_CODE_DETAILS%

Notes:

• None of these variables cause route failures when passed through unescaped
• Envoy passes variables that can't be expanded through unchanged or skips them entirely - for example:
  • %UPSTREAM_REMOTE_ADDRESS% as a request header remains as %UPSTREAM_REMOTE_ADDRESS% because as noted in the Envoy docs: "The upstream remote address cannot be added to request headers as the upstream host has not been selected when custom request headers are generated."
  • %DOWNSTREAM_TLS_VERSION% is skipped if TLS is not in use
• Envoy ignores REQ headers that refer to an non-existent header - for example %REQ(Host)% works as expected but %REQ(Missing-Header)% is skipped
• The %DYNAMIC_METADATA([“namespace”, “key”, …])% and %PER_REQUEST_STATE(reverse.dns.data.name)% headers were explicitly excluded as this adds significant complexity and it is not clear these can be used safely

I think this provides useful functionality while still ensuring that bad config is not sent to Envoy.

Would you be open to a pull request that implements this?

[erwbgy]

See main...erwbgy:dynamic-headers for proposed changes.

[stevesloka]

Hey @erwbgy yup I think that makes sense to me and would work. Also, your design leaves the door open to allow full customization later on. Will be curious what others think as well about this.

[sunjayBhatia]

Agreed with @stevesloka, looks like your approach is simple enough, works, and provides value and as you note Envoy and the change to contour avoid some of the more convoluted scenarios. Might be worth examining if %START_TIME% needs to be special cased in the implementation or omitted (since it looks like it can take format parameters) and adding a test/end to end case around a desired header value with non-dynamic content/multiple dynamic fields (seems that is allowed since these are variable names), e.g. value: 'some content %HOSTNAME%' foo bar or value: '%HOSTNAME% - % PROTOCOL%'

[erwbgy]

Good catch about %START_TIME% @sunjayBhatia, I hadn't noticed that an optional format string could be passed. I think you're right that it should be omitted as special casing it would add unnecessary complication right now. The %REQ(header)% special case at least has a user request for this functionality.

Ack about the additional test cases required.

[youngnick]

I'll take a look at the PR now, but overall, I think this design is good. I really appreciate that you've been cautious about the NACK problem #1176 (which the Envoy message in your OP would indicate we would run into).

Thanks for the great design, and the PR!

[xaleeks]

Thanks for the awesome PR @erwbgy

Think we can get this in 1.12? @youngnick