Supression of X-Envoy headers breaks expected output of default log format

[davidalger]

Description:

After the changes in #2500 and #2585 logs no longer include the proper request path where path rewrites are used.

Repro steps:

If you apply the quickstart demo and then add a rule to the HTTPRoute:

    - backendRefs:
        - group: ""
          kind: Service
          name: backend
          port: 3000
          weight: 1
      matches:
        - path:
            type: PathPrefix
            value: /rewrite
      filters:
        - type: URLRewrite
          urlRewrite:
            path:
              type: ReplacePrefixMatch
              replacePrefixMatch: /123

This request:

curl -sv -H "Host: www.example.com" 'http://172.18.255.200/rewrite/something?foo=bar'

Would normally (and previously did) result in the following log output):

{
  "start_time": "2024-03-13T02:18:21.648Z",
  "method": "GET",
  "x-envoy-origin-path": "/rewrite/something?foo=bar",
  "protocol": "HTTP/1.1",
  "response_code": "200",
...
  "duration": "1",
  "x-envoy-upstream-service-time": "0",
...
}

It is now incorrectly outputting the rewritten path instead and losing the original request path:

{
  "start_time": "2024-03-13T02:23:29.892Z",
  "method": "GET",
  "x-envoy-origin-path": "/123/something?foo=bar",
  "protocol": "HTTP/1.1",
  "response_code": "200",
...
  "duration": "1",
  "x-envoy-upstream-service-time": "-",
...
}

[davidalger]

The default logging format for Envoy is this:

[%START_TIME%] "%REQ(:METHOD)% %REQ(X-ENVOY-ORIGINAL-PATH?:PATH)% %PROTOCOL%"
%RESPONSE_CODE% %RESPONSE_FLAGS% %BYTES_RECEIVED% %BYTES_SENT% %DURATION%
%RESP(X-ENVOY-UPSTREAM-SERVICE-TIME)% "%REQ(X-FORWARDED-FOR)%" "%REQ(USER-AGENT)%"
"%REQ(X-REQUEST-ID)%" "%REQ(:AUTHORITY)%" "%UPSTREAM_HOST%"\n

The default logging format for Envoy Gateway is very similar but with JSON encoding in the string:

	EnvoyTextLogFormat = "{\"start_time\":\"%START_TIME%\",\"method\":\"%REQ(:METHOD)%\"," +
		"\"x-envoy-origin-path\":\"%REQ(X-ENVOY-ORIGINAL-PATH?:PATH)%\",\"protocol\":\"%PROTOCOL%\"," +
		"\"response_code\":\"%RESPONSE_CODE%\",\"response_flags\":\"%RESPONSE_FLAGS%\"," +
		"\"response_code_details\":\"%RESPONSE_CODE_DETAILS%\"," +
		"\"connection_termination_details\":\"%CONNECTION_TERMINATION_DETAILS%\"," +
		"\"upstream_transport_failure_reason\":\"%UPSTREAM_TRANSPORT_FAILURE_REASON%\"," +
		"\"bytes_received\":\"%BYTES_RECEIVED%\",\"bytes_sent\":\"%BYTES_SENT%\"," +
		"\"duration\":\"%DURATION%\",\"x-envoy-upstream-service-time\":\"%RESP(X-ENVOY-UPSTREAM-SERVICE-TIME)%\"," +
		"\"x-forwarded-for\":\"%REQ(X-FORWARDED-FOR)%\",\"user-agent\":\"%REQ(USER-AGENT)%\"," +
		"\"x-request-id\":\"%REQ(X-REQUEST-ID)%\",\":authority\":\"%REQ(:AUTHORITY)%\"," +
		"\"upstream_host\":\"%UPSTREAM_HOST%\",\"upstream_cluster\":\"%UPSTREAM_CLUSTER%\"," +
		"\"upstream_local_address\":\"%UPSTREAM_LOCAL_ADDRESS%\"," +
		"\"downstream_local_address\":\"%DOWNSTREAM_LOCAL_ADDRESS%\"," +
		"\"downstream_remote_address\":\"%DOWNSTREAM_REMOTE_ADDRESS%\"," +
		"\"requested_server_name\":\"%REQUESTED_SERVER_NAME%\",\"route_name\":\"%ROUTE_NAME%\"}\n"

Particularly since the logs use x-envoy-origin-path as the field name in the JSON format, one would naturally expect the default behavior to include the actual original path value the end user sent with the request.

This also affects gRPC ALS payloads which normally include the OriginalPath but don't when supress_envoy_headers is enabled in the XDS as it now is.

For myself, I'll be flipping this back to the default Envoy behavior, but I want to raise this as a bug as it seems the logic of suppressing these by default is flawed given it lowers the accuracy of request logs.

[arkodg]

looks like we need to tweak the default access log format based on the value from suppressEnvoyHeaders

[zirain]

looks like we need to tweak the default access log format based on the value from suppressEnvoyHeaders

I don't think so, we should just warning/highlight it.

[LeMyst]

I am writing this for reference, but if you want to re-enable the X-Envoy-* headers (at least some of them), you can create a ClientTrafficPolicy, as mentioned in this comment:
#7811 (comment)

[zirain]

it would be good to hightlight this at https://gateway.envoyproxy.io/docs/tasks/observability/proxy-accesslog/#default-access-log