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

Add access to streaminfo for tcp connections too #261

Merged
merged 3 commits into from
Nov 5, 2019

Conversation

gargnupur
Copy link
Contributor

Description: Add access to streaminfo for tcp connections too
Risk Level: Low

if (network_read_filter_callbacks_) {
return network_read_filter_callbacks_->connection().streamInfo().filterState();
}
return network_write_filter_callbacks_->connection().streamInfo().filterState();
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is correct, you have no guarantee that this isn't nullptr.

Also, does network_read_filter_callbacks_->connection() and network_write_filter_callbacks_->connection() point to the same connection or is one for downstream and one for upstream?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing..will fix that..

looks like they both point to the same connection.. we need different one for upstream and downstream right?

WasmResult Context::setProperty(absl::string_view key, absl::string_view serialized_value) {
ProtobufWkt::Value value_struct;
if (!value_struct.ParseFromArray(serialized_value.data(), serialized_value.size())) {
return WasmResult::ParseFailure;
}
if (decoder_callbacks_ == nullptr && encoder_callbacks_ == nullptr) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Everything below could be simplified to:

auto stream_info = getRequestStreamInfo();
if (!stream_info) {
  return WasmResult:: NotFound;
}

stream_info->filterState().setData(key, std::make_unique<WasmState>(value_struct),
                                   StreamInfo::FilterState::StateType::Mutable);
return WasmResult::Ok;

and then you don't even need getFilterState().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I totally agree with that, but I kept separate functions because getStreamInfo was looking at access_log_stream_info_ and getFilterstate was not.. Is it okay to look at access_log_stream_info_ for filter state?

Signed-off-by: gargnupur <gargnupur@google.com>
Signed-off-by: gargnupur <gargnupur@google.com>
Signed-off-by: gargnupur <gargnupur@google.com>
@gargnupur
Copy link
Contributor Author

@PiotrSikora : Can you please take a look at this?

@PiotrSikora PiotrSikora merged commit 13305fb into envoyproxy:master Nov 5, 2019
jplevyak pushed a commit to jplevyak/envoy-wasm that referenced this pull request Aug 25, 2020
… (#11831) (envoyproxy#261)

Currently Opencensus tracer uses grpcService proto to configure its tracing client stub. It uses GoogleGrpcUtil to construct a channel and create client stub with that. However, the channel created there does not take care of initial_metadata from grpcService configure. This change fills in OCprepare_client_context option in export to make it respect initial metadata.

Risk level: Low

Signed-off-by: Pengyuan Bian <bianpengyuan@google.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants