http: Change sendLocalReply to send percent-encoded GrpcMessage

[dio]

Description: This patch changed sendLocalReply to send GrpcMessage in percent-encoded
instead of plain string following: https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md#responses.

Risk Level: Low
Testing: Unit tests
Docs Changes: N/A
Release Notes: Added
Fixes #6902

[snowp]

Nice, looks pretty good.

Does this seem like something that needs to be config guarded in some way? While I understand this to be the correct behavior I wonder if there are people relying on this/don't want to spend the CPU on encoding every response.

[dio]

@snowp thanks for the review. Will address it soon. re: config, yeah probably that's a good point. Will try to introduce one for this.

[dio]

@snowp for configuration, I think I'll add it after #6261 is merged.

[thorkildcognite]

Note that if this feature is not enabled, then it will take down envoy if the external auth service responds with a multi-line message. It is a point that someone could be parsing and using that header already, though, of course, but it sounds like something that should be default on (with a note in the release log)

[snowp]

Yeah it's a bit tricky due to this being used by out of repo filters, so we don't really know how it is being used. There seems to be 3 options: 1) config guard it 2) make the encoding behavior optional through a parameter to sendLocalReply and set it to true for all in-repo usages or 3) just update sendLocalyReply to always encode

I'm not sure how careful we have to be here wrt to the breaking change policy. @alyssawilk @mattklein123 Do you have any thoughts on how to best handle this?

[alyssawilk]

I'd like @lizan 's take but if this is a correctness issue I'd be inclined to say we should always do it, but we could runtime guard the change to make sure if it causes problems it's easy for folks to roll back while we address.

[qiwzhang]

Per https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md

The value portion of Status-Message is conceptually a Unicode string description of the error, physically encoded as UTF-8 followed by percent-encoding. Percent-encoding is specified in RFC 3986 §2.1, although the form used here has different restricted characters. When decoding invalid values, implementations MUST NOT error or throw away the message. At worst, the implementation can abort decoding the status message altogether such that the user would received the raw percent-encoded form. Alternatively, the implementation can decode valid portions while leaving broken %-encodings as-is or replacing them with a replacement character (e.g., '?' or the Unicode replacement character).

Grpc-Message should be percent-encoded.

[lizan]

We should just update sendLocalyReply, per gRPC spec it should be always precent-encoded.

[snowp]

Seems like the consensus is that we don't need to gate this on anything, which seems fine to me. My initial concern was about out of tree uses that might already be percent encoding the value given to sendLocalReply, but that might not be very common

[dio]

/retest

[repokitteh-read-only]

🔨 rebuilding ci/circleci: tsan (failed build)

🐱

Caused by: a #6913 (comment) was created by @dio.

see: more, trace.

[snowp]

Sorry for the delay, this LGTM. @lizan Do you wanna do the senior review of this one?

[mattklein123]

LGTM w/ small comment request. Thank you!

/wait

[mattklein123]

This first check is a performance optimization, right? It took me a little while to figure out what was going on here. If so, can you add an explanatory comment about why we are doing it this way?

[dio]

Sure. Added some comments.

[mattklein123]

Awesome, thanks.