Add access to upstreamfilterstate in WasmStateWrapper #394
Conversation
gargnupur
requested review from
jplevyak,
lizan and
PiotrSikora
as code owners
| } | ||
|
|
||
| if (upstream_connection_filter_state_ && |
There was a problem hiding this comment.
What was the outcome of the discussion for the fallback of the filter state to upstream connection filter state?
There was a problem hiding this comment.
Don't think we discussed that...
https://github.com/envoyproxy/envoy/pull/9202/files PR added sustaining filterstate to downstream connection lifespan and then I added this envoyproxy/envoy#9896 to share filterstate between upstream and downstream filters.
I see your point that we will suffer from the problem, what if upstream connection closes before downstream connection, then in that case upstream filterstate will be invalid. Is it possible for that to happen?
There was a problem hiding this comment.
It's a shared_ptr so the state will outlive the upstream connection. But we should be careful to clear it in the wasm context eventually.
There was a problem hiding this comment.
My question was about filter_state itself falling through to upstream connection filter state. E.g. make it try HTTP, downstream TCP, then upstream TCP.
There was a problem hiding this comment.
No, didnt try to make filter_state itself fall through to upstream TCP
There was a problem hiding this comment.
Talked offline.. will simplify in a different pr..
|
/retest |
Signed-off-by: gargnupur <gargnupur@google.com>
gargnupur
force-pushed
the
nup_filter_state_shared
branch
from
916ae59
to
7f29426
Compare
source/common/tcp_proxy/tcp_proxy.cc
Outdated
| @@ -239,6 +239,12 @@ void Filter::initialize(Network::ReadFilterCallbacks& callbacks, bool set_connec | |||
| getStreamInfo().setDownstreamLocalAddress(read_callbacks_->connection().localAddress()); | |||
| getStreamInfo().setDownstreamRemoteAddress(read_callbacks_->connection().remoteAddress()); | |||
| getStreamInfo().setDownstreamSslConnection(read_callbacks_->connection().ssl()); | |||
| read_callbacks_->connection().streamInfo().setDownstreamLocalAddress( | |||
There was a problem hiding this comment.
I will after getting feedback from our team...
There was a problem hiding this comment.
@jplevyak : John removed tcp proxy change her and added to envoy only first, will merge here after that... envoyproxy/envoy#9949
Can you please help merge this PR?
gargnupur
changed the title
Signed-off-by: gargnupur <gargnupur@google.com>
gargnupur
force-pushed
the
nup_filter_state_shared
branch
from
9124020
to
b84ef7e
Compare
Add access to upstreamfilterstate in WasmStateWrapper
For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md
Add access to upstreamfilterstate in WasmStateWrapper
Description:
Risk Level:
Ref: istio/istio#20802
Testing: Tested E2E in istio/proxy
Docs Changes:
Release Notes:
[Optional Fixes #Issue]
[Optional Deprecated:]