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

Add access to upstreamfilterstate in WasmStateWrapper #394

Merged
merged 2 commits into from
Feb 6, 2020
Merged

Add access to upstreamfilterstate in WasmStateWrapper #394

Changes from all commits
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
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_ &&
Copy link
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?

Copy link
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?

Copy link
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.

Copy link
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.

Copy link
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

Copy link
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