Determine upgrade path for dynamic metadata

[curiouserrandy]

Title: Evaluate upgrade path from RequestInfo::dynamicMetadata to new DynamicMetadata class

Description:

As part of working on implementing a new DynamicMetadata class for arbitrarily typed communication between filters, I've been evaluating current uses of RequestInfo::dynamicMetadata(). After some discussion with @htuch, I wanted to poll the experts on the current uses of that method about whether they think an upgrade to the new method is feasible. Would each of you be willing to take a look at the design and comment as to whether you think there's a reasonable deprecation path from the old method to the new one, whether the two methods should be kept on in parallel, or if you have another preference? (The old method is an arbitrary proto indexed by RDNS filter name, the new method is an arbitrary C++ object indexed by RDNS organization+data name.)

Lua filter: @dio
RBAC filter: @yangminzhu
HeaderToMetadata: @rgs1
FYI: @mattklein123 @alyssawilk

Relevant Links
Design doc: https://docs.google.com/document/d/1GNawM59Pp09WZ34zqJYiO_qmERjuGbOUeAqF1GG6v9Q/edit#
(The key section for this issue is "Accessing data produced in one filter from another")

Implementation PR: #3918

[htuch]

@qiwzhang as well, plus @lizan and other Istio security folks. Can we rollover RBAC to Randy's new metadata approach? We don't want to maintain two metadata schemes and would like to turn down the old one unless it needs to be supported, e.g. due to semantic expresiveness missing in the new scheme.

[rgs1]

We use the HeaderToMetadata filter to dynamically match requests with endpoints in the subset LB, so migrating both in one go is probably easier (or, the subset LB would have to go first).

Here's how the subset LB matches a request to an endpoint based on metadata: https://github.com/envoyproxy/envoy/blob/master/source/common/upstream/subset_lb.cc#L286

Per the design doc, "... the producer and consumer must agree on the type of the data being accessed". I am guessing this shouldn't be a problem when matching an endpoint's metadata (deserialized from config/eds) and incoming request's metadata? That is, does the new Metadata class provides a way for comparing objects and introspecting types without a previous agreement (haven't looked at the implementation, so not sure)?

[lizan]

RBAC filters is heavily using the MetadataMatcher. It takes advantage of the metadata being a protobuf Struct so arbitrary data can be queried. From the design doc this seems hard since the accessor is not structured.

[yangminzhu]

As @lizan mentioned, the MetadataMatcher should be covered in the doc.

(Also recently we started to use RequestInfo.Metadata to pass data between different filters in Istios' proxy implementation. cc @diemtvu)

[qiwzhang]

Also dynamicData is used in AccessLog::Instance log() phase. Not sure if the design covers that phase. Can that phase access any dynamicData?

[htuch]

Yeah, I think I'm convinced that we're going to need to keep the dynamic metadata for config purposes. @curiouserrandy do you think we could come up with a name for the new dynamic metadata that doesn't confuse and conveys its intended use? Something like "inter-filter state", "request filter state", etc.

[curiouserrandy]

Agreed; the impedance mismatch is just too large :-{.

In terms of naming, I'm inclined to stay away from "metadata" since that has a well defined meaning inside of envoy. FilterState? GeneratedFilterState? I'd be good with either, but I'll go with the first in the name of terseness if no one else has an opinion.

[htuch]

+1 FilterState.

[qiwzhang]

how about SafelySharedFilterState to indicate the data is shared across filters, it is safe, non-intended filters could not access them.

[curiouserrandy]

I have a preference is against "SafelyShared" because a) I have more of a single producer/multiple consumer model for the state--any particular item isn't shared in that only one filter can source it, though multiple others can read it, and b) I'm not sure when the implementation of the dependency management portion of the design will be done (which I take as generating the "Safely" portion of your suggestion).

Having said that, my feelings on naming are weak; if there's a consensus around SafelySharedFilterState I'll happily accept that.

[lizan]

+1 FilterState.

Btw I think this proposal let the state holds arbitrary C++ object in it, so perhaps we can wrap the metadata proto and use the new mechanism. @htuch thoughts?

[htuch]

@lizan yeah, I had similar thoughts on this, @curiouserrandy have chatted IRL. There's a valid view that these are essentially orthogonal, and if you want to maintain the property that you set any given slot in only one filter, then it's probably too coarse grained to place the metadata proto in there, since multiple filters might want to manipulate it.

[mattklein123]

FWIW, I would vote for something along the lines of PerRequestState to indicate that this is state shared across the request and between filters. Similarly for network filters, perhaps PerConnectionState or similar. I don't feel very strongly about it though. I agree that wrapping the generic metadata in a C++ object seems like a fine way of merging both mechanisms.

[curiouserrandy]

+1 for Per{Request,Connection}State; that's something that hasn't historically been captured in the name.

I'll point out that how the mechanisms are implemented isn't visible at the API level, and I'd argue isn't very important for that reason. But (as @htuch says) I do think that the "single source" aspect of PerRequestState gets in the way of layering dynamicMetadata() on top of it--it's possible that current usage patterns wouldn't violate that requirement, but it's hard to tell since the lua filter exposes arbitrary modifications of the dynamic metadata to programs written in configurations, so we need to know the usage across the universe of configurations, not just what's in the source repository.

[htuch]

Also +1` on Per{Request,Connection}State.

[cmluciano]

Following up on the PR that @lizan mentioned above:

We agree that something like ConnectionState would be useful for network filters. My next question is what do we use to propagate the required metadata between filters on different Envoy hosts? For HTTP filters we convert whatever needs passed to headers, but I'm uncertain what method we would use for a connection oriented protocol.

cc @ggreenway @alyssawilk

[zuercher]

I added something similar to the TCP connection pool for upstream connections -- users of the conn pool can attach state to an upstream connection. I doubt that's of use here (where you wish to attach state to downstream connections), but if the new mechanism can be applied to either downstream or upstream connections, the TCP conn pool version could probably be replaced.

[hans-stripe]

This looks super helpful for us too! We'd like to build an extension that can convert the client cert subject DN into its component parts (which would be a vector of pairs of strings) and share it with some Lua that we run on every request (in addition to some static per-route metadata). Doing the conversion as a network filter makes the most sense, while the Lua itself is a request filter -- being able to pass this state around would be great.

[stale]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or other activity occurs. Thank you for your contributions.

[htuch]

@curiouserrandy is there something we should be doing with this issue? Looks like we had agreement earlier in the thread that we can't do the impedance match.

[curiouserrandy]

Yup. From my perspective, this issue was resolved with our agreement that the impedance match was too large (and in picking the name :-}). I didn't really track the discussion after that around ConnectionState, so I didn't respond to the stale marker figuring the other folks commenting on the issue could revive it if needed.

[htuch]

Closing out as resolved, thanks @curiouserrandy.