header to metadata: add base64 encode and protobuf value type

[yangminzhu]

Signed-off-by: Yangmin Zhu [email protected]

For the first point in #7771 for converting arbitrary protobuf value from header to metadata.

cc @rgs1 @kyessenov

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Description:
Risk Level: Low
Testing: Unit tests
Docs Changes: Updated version_history.rst
Release Notes: Updated version_history.rst
[Optional Fixes #Issue]
[Optional Deprecated:]

[yangminzhu]

For the second point, just a quick question, do we prefer to add a new metadata_to_header filter or expand the header_to_metadata API to support it?

[kyessenov]

Can something be done to avoid serializing the metadata on every request, and do it only once during filter construction? That's the critical part in the existing istio implementation.

[kyessenov]

cc @mandarjog

[lizan]

This is counter intuitive from API stand point without reading the comment above, from reading the implementation, I'd say make this VALUE or PROTOBUF_VALUE, and have another enum for the encoding to express it, since which base64 or not is orthogonal to value type here, WDYT?

[yangminzhu]

That looks a cleaner API though I imagine the base64 option is likely to be always used only with PROTOBUF_VALUE?

I'm fine with the suggestion, @rgs1 WDYT?

[lizan]

It is possible for someone to use STRING with BASE64 with some non header safe character sets (UTF-8), and when you add more binary types here it will help anyway.

[rgs1]

yeah i like Lizan's suggestion (PROTOBUF_VALUE).

[yangminzhu]

Thanks, updated.

[yangminzhu]

@kyessenov Hmm, not sure I understand your question. Could you clarify a little bit? Thanks. I thought the header value is only known on request so how can we do the serialization only during filter construction?

One thing I can think of right now is to use a cache to avoid the serialization on every request. Is this what you're suggesting?

[kyessenov]

@yangminzhu The filter I referenced sends node metadata, which is always the same across all requests. For example, pod labels.

[yangminzhu]

@kyessenov I see, Are you referring to the client side sending the metadata via header? I think that is not covered in this PR and will be solved with the new metadata_to_header filter. Thanks for raising the concern, I think the API of the metadata_to_header filter should be able to specify it's from the node metadata, and in this case we can only serialize it once and use it everytime.

This PR is about the server side decoding the header, Is there anything else we should take care of here? thanks!

[lizan]

@yangminzhu @kyessenov take a look on #2394 while it is not completely standardized though it is partially implemented. cc @soya3129 for current status.

[lizan]

IIUC it allows per connection metadata exchange as well as per stream metadata exchange.

[kyessenov]

@lizan Yes, we're are aware of H2 metadata. It's very cool, but unfortunately the world has not even moved to H2 yet, so it'll cause issues with inter-operability with other proxies (or even gRPC).

[kyessenov]

@yangminzhu This is fine for extracting a request header to metadata. I'm still not entirely sure how this works for extracting a response headers from an upstream host, to be usable as a replacement for the istio filter.

[rgs1]

@yangminzhu This is fine for extracting a request header to metadata. I'm still not entirely sure how this works for extracting a response headers from an upstream host, to be usable as a replacement for the istio filter.

The h-t-m filter works for both request/response headers, fwiw.

Also, do we need a separate metadata-to-header filter or could we have that functionality in this filter as well?

[kyessenov]

@rgs1 Gotcha, I didn't see response rules at first.

[repokitteh-read-only]

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #7796 was synchronize by yangminzhu.

see: more, trace.

[yangminzhu]

@lizan @rgs1 @kyessenov PTAL, the coverage seems failing for some network error, thank you.

[zuercher]

This seems very useful. Thanks for looking into it. To start with, I have a question about the API, once we get that nailed down I'll look at the rest more closely.

[repokitteh-read-only]

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #7796 was synchronize by yangminzhu.

see: more, trace.

[repokitteh-read-only]

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #7796 was synchronize by yangminzhu.

see: more, trace.

[repokitteh-read-only]

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #7796 was synchronize by yangminzhu.

see: more, trace.

[htuch]

@envoyproxy/api-shepherds API LGTM

[repokitteh-read-only]

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #7796 was synchronize by yangminzhu.

see: more, trace.

[lizan]

great, thanks. just one nit.

[repokitteh-read-only]

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #7796 was synchronize by yangminzhu.

see: more, trace.

[yangminzhu]

@htuch Would you mind to approve for the API again? I pushed a new commit to resolve some other comments, no new changes in API. Thank you.

[lizan]

No API changes since last API review, just merged.