Support custom JSON fields for Envoy access logs

[mike1808]

We want to be able to configure Contour to have custom JSON access log fields, such as custom_field: %REQ(OUR-CUSTOM-HEADER)%.

We are thinking that this can be achieved by having a new property with the JSON fields mapping like for DefaultField in the Contour config. For example:

accesslog-format: json
json-fields:
  - "@timestamp"
  - custom_field
extra-json-fields:
  custom_field:  "%REQ(X-CUSTOM-HEADER)%",

We @cloudfoundry/cf-for-k8s-networking are happy to contribute this feature to upstream.

[xcrezd]

+1

[jpeach]

Historically, new fields were added to the internal list, see #1734. I think that customizing the fields is a reasonable need, however IIUC there's always been a concern that directly exposing the envoy config is too risky because an invalid config would break the envoy config, and we don't have good ways to surface the error.

Maybe we can address this without directly exposing the envoy config, or maybe it's time to just accept the risk 🤷

[mike1808]

@jpeach do you think restricting fields to only take custom headers names will increase the chances of accepting this feature? For example:

json-fields:
  - "@timestamp"
  - custom_field
extra-json-fields-headers:
  custom_field:  "X-CUSTOM-HEADER"

So it won't be the exact Envoy config, and will help Contour to keep this API in future if Envoy decides to change it.

[xcrezd]

This is the only thing that stops us to move from nginx-controller to contour, the ability to add headers to log

[jpeach]

xref #1507, #2523

[jpeach]

When we send an unsupported log token, the listener resource is rejected:

ERRO[0004] Error adding/updating listener(s) ingress_http: Not supported field in StreamInfo: JPEACH  code=13 connection=4 context=xds node_id=envoy-f2lvb node_version=v1.16.0 response_nonce=1 version_info=1

Neither Contour nor go-control-plane recover gracefully from a rejected update, so we want to be careful to only push valid configuration.

envoyproxy/envoy#13620

[jpeach]

@jpeach do you think restricting fields to only take custom headers names will increase the chances of accepting this feature?

I'm not sure that we need to be as restrictive as that. What if we allow known-good envoy log tokens within the same json-fields syntax? Maybe:

json-fields:
- "@timestamp"
- "flags=%RESPONSE_FLAGS%"
- "other_field="some literal string"

We can validate the right hand-side of the key=value pair against the internal list of non-parameterized log tokens. For parameterized tokens, we ought to be able to validate %RESP()%, %REQ()% and %TRAILER()% with a regex. I'm not sure that the other parameterized tokens are all that useful in a Contour deployment.

If the field name conflicts with a built-in field name, we probably should treat this as an override (e.g. it would make sense for many organizations to override the request_id field to use a different header).

[youngnick]

Yes, to put this another way, the risk of accepting config directly from users is that the failure case is really bad. One typo, and your Envoys will stop accepting config from Contour, until they are restarted, even if you fix the issue. (!) This is not handled well in either the original Contour xDS implementation (#1176) or the go-control-plane one (envoyproxy/go-control-plane#46).

@jpeach's suggestion seems reasonable, I'd like to see a design proposal for something similar (That is, a PR to /design using https://github.com/projectcontour/contour/blob/main/design/design-document-tmpl.md as a base), or at least a detailed description in here of how we could handle the use cases @jpeach mentioned above.

[mike1808]

@youngnick I'll open a PR for a design. Also, just curious why the Envoy JSON logging proposal is still in a draft stage?

[youngnick]

It's probably still in draft because we forgot to update it to final, oops. Thanks for pointing that out.