stream info: propagate downstream filter state to upstream#19668
stream info: propagate downstream filter state to upstream#19668kyessenov wants to merge 3 commits intoenvoyproxy:mainfrom
Conversation
alyssawilk
left a comment
alyssawilk
left a comment
There was a problem hiding this comment.
Great to see more progress for avoiding the loopback hop. I have a couple of high level questions before digging in
/wait
| CodecType type() const { return type_; } | ||
|
|
||
| // Note this is the L4 stream info, not L7. | ||
| const StreamInfo::StreamInfo& streamInfo() { return connection_->streamInfo(); } |
There was a problem hiding this comment.
Your PR description says
" Set downstream filter state on upstream stream info."
but you're setting downstream filter state on the upstream connection stream info.
Matching downstream L7 streams with upstream L4 connections seems like it's going to be problematic. Is that what you're intending to do? I would think that we'd want to match stream to stream, in case multiple L7 streams went out over one virtual connection.
There was a problem hiding this comment.
Hmmm, this is a very good point. I think this PR needs more thought actually. L4 connections from downstreams are not strictly one-to-one with L4 upstream connections so it's not right to use either L4 or L7 filter state here. I tend to think that the import of filter state must also be explicit like in #19435. That way the hash policy determines the grouping of downstream L4/L7 stream filter states.
| } | ||
|
|
||
| /** | ||
| * Filter State object to be shared between upstream and downstream filters. |
There was a problem hiding this comment.
I think if we end up going this route we're going to want to be awfully clear that this is downstream L7 filters and upstream L4 filters.
I also wonder if instead of adding another set of accessors, if there would be a way to tag information that's added as "information that should be communicated to upstream stream info" WDYT?
There was a problem hiding this comment.
Yes, this is more confusing than helpful. I think an explicit export is probably better (either in the filter state itself or in the transport socket).
2 participants
Signed-off-by: Kuat Yessenov kuat@google.com
Commit Message: Set downstream filter state on upstream stream info.
Additional Description: This is useful for propagation of info from downstream to upstream filters. It avoids the lifetime issues with dynamic metadata and the rest of stream info because upstream and downstream infos lifetimes are decoupled. Once the internal listener lands, we will set the downstream filter state on the downstream info to the upstream filter state to enable propagation between listeners.
Risk Level: low
Testing: unit
Docs Changes: none
Release Notes: none