Skip to content
This repository was archived by the owner on Dec 16, 2020. It is now read-only.

Add access to upstreamfilterstate in WasmStateWrapper #394

Merged
jplevyak merged 2 commits into from
Feb 6, 2020
Merged

Add access to upstreamfilterstate in WasmStateWrapper #394

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7f29426
Fixed based on feedback
gargnupur Feb 5, 2020
b84ef7e
Remove TCP Proxy Changes
gargnupur Feb 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions source/common/tcp_proxy/tcp_proxy.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(

@jplevyak jplevyak Feb 6, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this being upstreamed?

@gargnupur gargnupur Feb 6, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will after getting feedback from our team...

@gargnupur gargnupur Feb 6, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

read_callbacks_->connection().localAddress());
read_callbacks_->connection().streamInfo().setDownstreamRemoteAddress(
read_callbacks_->connection().remoteAddress());
read_callbacks_->connection().streamInfo().setDownstreamSslConnection(
read_callbacks_->connection().ssl());

// Need to disable reads so that we don't write to an upstream that might fail
// in onData(). This will get re-enabled when the upstream connection is
Expand Down Expand Up @@ -469,6 +475,10 @@ void Filter::onPoolReady(Tcp::ConnectionPool::ConnectionDataPtr&& conn_data,
getStreamInfo().onUpstreamHostSelected(host);
getStreamInfo().setUpstreamLocalAddress(connection.localAddress());
getStreamInfo().setUpstreamSslConnection(connection.streamInfo().downstreamSslConnection());
read_callbacks_->connection().streamInfo().onUpstreamHostSelected(host);
read_callbacks_->connection().streamInfo().setUpstreamLocalAddress(connection.localAddress());
read_callbacks_->connection().streamInfo().setUpstreamSslConnection(
connection.streamInfo().downstreamSslConnection());
read_callbacks_->connection().streamInfo().setUpstreamFilterState(
connection.streamInfo().filterState());

Expand Down
46 changes: 21 additions & 25 deletions source/extensions/common/wasm/context.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,32 +348,29 @@ WasmResult serializeValue(Filters::Common::Expr::CelValue value, std::string* re
class WasmStateWrapper : public google::api::expr::runtime::CelMap {
public:
WasmStateWrapper(const StreamInfo::FilterState& filter_state,
const StreamInfo::FilterState* connection_filter_state)
: filter_state_(filter_state), connection_filter_state_(connection_filter_state) {}
WasmStateWrapper(const StreamInfo::FilterState& filter_state)
: filter_state_(filter_state), connection_filter_state_(nullptr) {}
const StreamInfo::FilterState* upstream_connection_filter_state)
: filter_state_(filter_state),
upstream_connection_filter_state_(upstream_connection_filter_state) {}
absl::optional<google::api::expr::runtime::CelValue>
operator[](google::api::expr::runtime::CelValue key) const override {
if (!key.IsString()) {
return {};
}
auto value = key.StringOrDie().value();
try {
if (filter_state_.hasData<WasmState>(value)) {
const WasmState& result = filter_state_.getDataReadOnly<WasmState>(value);
return google::api::expr::runtime::CelValue::CreateBytes(&result.value());
} catch (const EnvoyException& e) {
// If doesn't exist in request filter state, try looking up in connection filter state.
try {
if (connection_filter_state_) {
const WasmState& result = connection_filter_state_->getDataReadOnly<WasmState>(value);
return google::api::expr::runtime::CelValue::CreateBytes(&result.value());
}
} catch (const EnvoyException& e) {
return {};
}
return {};
}

if (upstream_connection_filter_state_ &&

@kyessenov kyessenov Feb 5, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was the outcome of the discussion for the fallback of the filter state to upstream connection filter state?

@gargnupur gargnupur Feb 5, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@kyessenov kyessenov Feb 5, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@kyessenov kyessenov Feb 5, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@gargnupur gargnupur Feb 5, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, didnt try to make filter_state itself fall through to upstream TCP

@gargnupur gargnupur Feb 5, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Talked offline.. will simplify in a different pr..

upstream_connection_filter_state_->hasData<WasmState>(value)) {
const WasmState& result =
upstream_connection_filter_state_->getDataReadOnly<WasmState>(value);
return google::api::expr::runtime::CelValue::CreateBytes(&result.value());
}
return {};
}

int size() const override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; }
bool empty() const override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; }
const google::api::expr::runtime::CelList* ListKeys() const override {
Expand All @@ -382,7 +379,7 @@ class WasmStateWrapper : public google::api::expr::runtime::CelMap {

private:
const StreamInfo::FilterState& filter_state_;
const StreamInfo::FilterState* connection_filter_state_;
const StreamInfo::FilterState* upstream_connection_filter_state_;
};

#define PROPERTY_TOKENS(_f) \
Expand Down Expand Up @@ -423,14 +420,9 @@ Context::FindValue(absl::string_view name, Protobuf::Arena* arena) const {
break;
case PropertyToken::FILTER_STATE:
if (info) {
const Envoy::Network::Connection* connection = getConnection();
if (connection) {
return CelValue::CreateMap(Protobuf::Arena::Create<WasmStateWrapper>(
arena, info->filterState(), &connection->streamInfo().filterState()));
} else {
return CelValue::CreateMap(
Protobuf::Arena::Create<WasmStateWrapper>(arena, info->filterState()));
}

return CelValue::CreateMap(Protobuf::Arena::Create<WasmStateWrapper>(
arena, info->filterState(), info->upstreamFilterState().get()));
}
break;
case PropertyToken::REQUEST:
Expand Down Expand Up @@ -1004,6 +996,10 @@ const Network::Connection* Context::getConnection() const {
return encoder_callbacks_->connection();
} else if (decoder_callbacks_) {
return decoder_callbacks_->connection();
} else if (network_read_filter_callbacks_) {
return &network_read_filter_callbacks_->connection();
} else if (network_write_filter_callbacks_) {
return &network_write_filter_callbacks_->connection();
}
return nullptr;
}
Expand Down