Allow access log variables in {request,response}_headers_to_add

[htuch]

In the v2 API https://github.com/lyft/envoy-api/blob/master/api/rds.proto#L325 we would like to support access to the access log variable in the custom header values. This issue will track implementation work on this feature.

[htuch]

This will entail adding to https://github.com/lyft/envoy/blob/master/source/common/router/config_impl.cc#L184 (and lines below) as well as https://github.com/lyft/envoy/blob/master/source/common/http/conn_manager_utility.cc#L141 a call to https://github.com/lyft/envoy/blob/master/source/common/http/access_log/access_log_formatter.cc#L124.

The trick is that during request (and to some extent response), not all the information available to the access log is around. For example, requests won't have response headers or the fully populated RequestInfo object. Response headers may not be able to depend on later response headers.

Part of the implementation work will be to establish a specific ordering of the user added headers and to ensure that they can consume all preceding headers in a well defined way. There's probably also some additional plumbing to get the RequestInfo plumbed in

[mattklein123]

@khgandhi just looked at the code. To add onto what @htuch said, here is what I would recommend doing:

• I don't think it's worth trying to use RequestInfo in this context. There are too many things that don't make sense. I think I would define a new interface called RequestHeaderFillContext or something like that.
• We should define a new set of replacement variables. If any of them are the same as what is already supported in access log, we should use the same name.
• The fill code should be modeled exactly like the access log formatter code, in that we pre-check each header to add to see if it has replacement, then build a set of lambdas that are chained together to produce the final header, based on a passed RequestHeaderFillContext.
• I think it's very likely that you can share some of the RequestInfoFormatter code in both places. Perhaps some of the lambdas can be factored out into some common code where they are shared.
• The router can create a RequestHeaderFillContext based on info it knows and the provided RequestInfo, or perhaps the fill context should be provided by the connection manager.
• Obviously we will pass a RequestHeaderFillContext to finalizeRequestHeaders() so that the expansion can take place.

[khgandhi]

@mattklein123.. thanks for the input.. which replacement variables do you think we should support to begin with ? I can think of a few:

• CLIENT_IP
• %REQ(X?Y):Z (which covers all the headers in the incoming request for the clients)

[khgandhi]

@mattklein123 .. also request_headers_to_add are supported at three different levels:

• route_config level
• virtual_host level
• route level

Do we want to add support at all levels ?

[mattklein123]

%REQ(X?Y):Z (which covers all the headers in the incoming request for the clients)

Is this useful? The headers are about to be forwarded? Is the for composing?

also request_headers_to_add are supported at three different levels

I would use the same code/expansion for all levels.

[khgandhi]

%REQ(X?Y):Z is the way we define the format. What i meant to ask is do we want to support arbitrary request headers found on the client request to be forwarded to the cluster ?

CLIENT_IP is something that might not be in the client's request headers and we are already populating it in XFF header. I was wondering if we could do support the following use cases
{
"request_headers_to_add" : [
// a built-in header that we want to remap using a different key
{"key":"X-Client-IP", "value": "%REQ(%X-Forwarded-For%)"}},
// a arbitrary header that we see in the client request which we would like to forward.
{"key":"X-Foo-Header", "value": "%REQ(%Foo-Header%)"}},
// a custom header with static value that we would like envoy to add (already supported)
{"key":"X-Foo-Header-Static", "value": "Foo"}}
]
}`

[mattklein123]

Yes I understand the CLIENT_IP case, as well as other variables we might add in the future. I don't understand why you would allow other request headers to be inserted, as they are about to be forwarded to the upstream cluster. What's the point? The only thing I can think of is you want to compose multiple headers into a new header.

[khgandhi]

i see.. so all the request headers in the client that are custom are anyways forwarded to the upstream clusters.

So the use case is only for variables like client ip. Do we have any other headers that we can think of ?

[khgandhi]

@mattklein123 i have started working on this feature and i am trying to get the remote address (client ip address)
in router.cc where we are calling the finalizeHeaders method. ?

[mattklein123]

https://github.com/lyft/envoy/blob/master/include/envoy/http/filter.h#L125

[khgandhi]

I actually tried that and the value seems to be empty. do we know if the value is actually getting set ?

i try the following in router.cc:
log().debug("downstream address: '{}'\n", callbacks_->downstreamAddress());
route_entry_->finalizeRequestHeaders(headers, callbacks_->requestInfo());

i and see an empty string..

[2017-06-16 14:03:49.059][21041][debug][router] downstream address: ''

[jwfang]

@khgandhi can we have CLIENT_PORT support for this ? :)