diff --git a/library/common/extensions/filters/http/platform_bridge/BUILD b/library/common/extensions/filters/http/platform_bridge/BUILD index 93892c6802..f0fac266c3 100644 --- a/library/common/extensions/filters/http/platform_bridge/BUILD +++ b/library/common/extensions/filters/http/platform_bridge/BUILD @@ -10,7 +10,7 @@ api_proto_package() envoy_cc_library( name = "platform_bridge_filter_lib", srcs = [ - "c_types.cc", + "c_type_definitions.h", "filter.cc", ], hdrs = [ diff --git a/library/common/extensions/filters/http/platform_bridge/c_types.cc b/library/common/extensions/filters/http/platform_bridge/c_type_definitions.h similarity index 93% rename from library/common/extensions/filters/http/platform_bridge/c_types.cc rename to library/common/extensions/filters/http/platform_bridge/c_type_definitions.h index b06216bce1..065c2f2b7f 100644 --- a/library/common/extensions/filters/http/platform_bridge/c_types.cc +++ b/library/common/extensions/filters/http/platform_bridge/c_type_definitions.h @@ -3,10 +3,6 @@ #include "envoy/http/filter.h" -const envoy_data envoy_unaltered_data = {0}; - -const envoy_headers envoy_unaltered_headers = {0}; - const envoy_filter_headers_status_t kEnvoyFilterHeadersStatusContinue = static_cast(Envoy::Http::FilterHeadersStatus::Continue); const envoy_filter_headers_status_t kEnvoyFilterHeadersStatusStopIteration = @@ -46,5 +42,5 @@ const envoy_filter_trailers_status_t kEnvoyFilterTrailersStatusContinue = const envoy_filter_trailers_status_t kEnvoyFilterTrailersStatusStopIteration = static_cast(Envoy::Http::FilterTrailersStatus::StopIteration); // See comment above. -extern const envoy_filter_trailers_status_t kEnvoyFilterTrailersStatusResumeIteration = +const envoy_filter_trailers_status_t kEnvoyFilterTrailersStatusResumeIteration = kEnvoyFilterTrailersStatusContinue - 1; diff --git a/library/common/extensions/filters/http/platform_bridge/filter.cc b/library/common/extensions/filters/http/platform_bridge/filter.cc index 96edc15755..31353530e3 100644 --- a/library/common/extensions/filters/http/platform_bridge/filter.cc +++ b/library/common/extensions/filters/http/platform_bridge/filter.cc @@ -1,4 +1,5 @@ #include "library/common/extensions/filters/http/platform_bridge/filter.h" +#include "library/common/extensions/filters/http/platform_bridge/c_type_definitions.h" #include "envoy/server/filter_config.h" @@ -56,6 +57,17 @@ void PlatformBridgeFilter::onDestroy() { platform_filter_.instance_context = nullptr; } +void PlatformBridgeFilter::replaceHeaders(Http::HeaderMap& headers, envoy_headers c_headers) { + headers.clear(); + for (envoy_header_size_t i = 0; i < c_headers.length; i++) { + headers.addCopy( + Http::LowerCaseString(Http::Utility::convertToString(c_headers.headers[i].key)), + Http::Utility::convertToString(c_headers.headers[i].value)); + } + // The C envoy_headers struct can be released now because the headers have been copied. + release_envoy_headers(c_headers); +} + Http::FilterHeadersStatus PlatformBridgeFilter::onHeaders(Http::HeaderMap& headers, bool end_stream, envoy_filter_on_headers_f on_headers) { // Allow nullptr to act as no-op. @@ -66,23 +78,26 @@ Http::FilterHeadersStatus PlatformBridgeFilter::onHeaders(Http::HeaderMap& heade envoy_headers in_headers = Http::Utility::toBridgeHeaders(headers); envoy_filter_headers_status result = on_headers(in_headers, end_stream, platform_filter_.instance_context); - Http::FilterHeadersStatus status = static_cast(result.status); - // TODO(goaway): Current platform implementations expose immutable headers, thus any modification - // necessitates a full copy. Add 'modified' bit to determine when we can elide the copy. See also - // https://github.com/lyft/envoy-mobile/issues/949 for potential future optimization. - headers.clear(); - for (envoy_header_size_t i = 0; i < result.headers.length; i++) { - headers.addCopy( - Http::LowerCaseString(Http::Utility::convertToString(result.headers.headers[i].key)), - Http::Utility::convertToString(result.headers.headers[i].value)); + + switch (result.status) { + case kEnvoyFilterHeadersStatusContinue: + PlatformBridgeFilter::replaceHeaders(headers, result.headers); + return Http::FilterHeadersStatus::Continue; + + case kEnvoyFilterHeadersStatusStopIteration: + iteration_state_ = IterationState::Stopped; + return Http::FilterHeadersStatus::StopIteration; + + default: + PANIC("invalid filter state: unsupported status for platform filters"); } - // The C envoy_headers struct can be released now because the headers have been copied. - release_envoy_headers(result.headers); - return status; + + NOT_REACHED_GCOVR_EXCL_LINE; } Http::FilterDataStatus PlatformBridgeFilter::onData(Buffer::Instance& data, bool end_stream, Buffer::Instance* internal_buffer, + Http::HeaderMap** pending_headers, envoy_filter_on_data_f on_data) { // Allow nullptr to act as no-op. if (on_data == nullptr) { @@ -90,7 +105,6 @@ Http::FilterDataStatus PlatformBridgeFilter::onData(Buffer::Instance& data, bool } envoy_data in_data; - if (iteration_state_ == IterationState::Stopped && internal_buffer && internal_buffer->length() > 0) { // Pre-emptively buffer data to present aggregate to platform. @@ -101,27 +115,26 @@ Http::FilterDataStatus PlatformBridgeFilter::onData(Buffer::Instance& data, bool } envoy_filter_data_status result = on_data(in_data, end_stream, platform_filter_.instance_context); - Http::FilterDataStatus status = static_cast(result.status); - switch (status) { - case Http::FilterDataStatus::Continue: + + switch (result.status) { + case kEnvoyFilterDataStatusContinue: if (iteration_state_ == IterationState::Stopped) { - // When platform filter iteration is Stopped, Resume must be used to start iterating again. - // TODO(goaway): decide on the means to surface/handle errors here. Options include: - // - crashing - // - creating an error response for this stream - // - letting Envoy handle any invalid resulting state via its own guards + PANIC("invalid filter state: filter iteration must be resumed with ResumeIteration"); } - break; - case Http::FilterDataStatus::StopIterationAndBuffer: + data.drain(data.length()); + data.addBufferFragment(*Buffer::BridgeFragment::createBridgeFragment(result.data)); + return Http::FilterDataStatus::Continue; + + case kEnvoyFilterDataStatusStopIterationAndBuffer: if (iteration_state_ == IterationState::Stopped) { // Data will already have been buffered (above). - status = Http::FilterDataStatus::StopIterationNoBuffer; - } else { - // Data will be buffered on return. - iteration_state_ = IterationState::Stopped; + return Http::FilterDataStatus::StopIterationNoBuffer; } - break; - case Http::FilterDataStatus::StopIterationNoBuffer: + // Data will be buffered on return. + iteration_state_ = IterationState::Stopped; + return Http::FilterDataStatus::StopIterationAndBuffer; + + case kEnvoyFilterDataStatusStopIterationNoBuffer: // In this context all previously buffered data can/should be dropped. If no data has been // buffered, this is a no-op. If data was previously buffered, the most likely case is // that a filter has decided to handle generating a response itself and no longer needs it. @@ -133,24 +146,32 @@ Http::FilterDataStatus PlatformBridgeFilter::onData(Buffer::Instance& data, bool internal_buffer->drain(internal_buffer->length()); } iteration_state_ = IterationState::Stopped; - break; - default: - PANIC("unsupported status for platform filters"); - } + return Http::FilterDataStatus::StopIterationNoBuffer; - // TODO(goaway): Current platform implementations expose immutable data, thus any modification - // necessitates a full copy. Add 'modified' bit to determine when we can elide the copy. See also - // https://github.com/lyft/envoy-mobile/issues/949 for potential future optimization. - if (iteration_state_ == IterationState::Ongoing) { - data.drain(data.length()); - data.addBufferFragment(*Buffer::BridgeFragment::createBridgeFragment(result.data)); + case kEnvoyFilterDataStatusResumeIteration: + if (iteration_state_ != IterationState::Stopped) { + PANIC("invalid filter state: ResumeIteration may only be used when filter iteration is stopped"); + } + if (result.extra_headers) { + PlatformBridgeFilter::replaceHeaders(**pending_headers, *result.extra_headers); + *pending_headers = nullptr; + free(result.extra_headers); + } + internal_buffer->drain(internal_buffer->length()); + internal_buffer->addBufferFragment(*Buffer::BridgeFragment::createBridgeFragment(result.data)); + return Http::FilterDataStatus::Continue; + + default: + PANIC("invalid filter state: unsupported status for platform filters"); } - return status; + NOT_REACHED_GCOVR_EXCL_LINE; } Http::FilterTrailersStatus PlatformBridgeFilter::onTrailers(Http::HeaderMap& trailers, + Buffer::Instance* internal_buffer, + Http::HeaderMap** pending_headers, envoy_filter_on_trailers_f on_trailers) { // Allow nullptr to act as no-op. if (on_trailers == nullptr) { @@ -159,19 +180,41 @@ PlatformBridgeFilter::onTrailers(Http::HeaderMap& trailers, envoy_headers in_trailers = Http::Utility::toBridgeHeaders(trailers); envoy_filter_trailers_status result = on_trailers(in_trailers, platform_filter_.instance_context); - Http::FilterTrailersStatus status = static_cast(result.status); - // TODO(goaway): Current platform implementations expose immutable trailers, thus any modification - // necessitates a full copy. Add 'modified' bit to determine when we can elide the copy. See also - // https://github.com/lyft/envoy-mobile/issues/949 for potential future optimization. - trailers.clear(); - for (envoy_header_size_t i = 0; i < result.trailers.length; i++) { - trailers.addCopy( - Http::LowerCaseString(Http::Utility::convertToString(result.trailers.headers[i].key)), - Http::Utility::convertToString(result.trailers.headers[i].value)); - } - // The C envoy_trailers struct can be released now because the trailers have been copied. - release_envoy_headers(result.trailers); - return status; + + switch (result.status) { + case kEnvoyFilterTrailersStatusContinue: + if (iteration_state_ == IterationState::Stopped) { + PANIC("invalid filter state: ResumeIteration may only be used when filter iteration is stopped"); + } + PlatformBridgeFilter::replaceHeaders(trailers, result.trailers); + return Http::FilterTrailersStatus::Continue; + + case kEnvoyFilterTrailersStatusStopIteration: + iteration_state_ = IterationState::Stopped; + return Http::FilterTrailersStatus::StopIteration; + + case kEnvoyFilterTrailersStatusResumeIteration: + if (iteration_state_ != IterationState::Stopped) { + PANIC("invalid filter state: ResumeIteration may only be used when filter iteration is stopped"); + } + if (result.extra_headers) { + PlatformBridgeFilter::replaceHeaders(**pending_headers, *result.extra_headers); + *pending_headers = nullptr; + free(result.extra_headers); + } + if (result.extra_data) { + internal_buffer->drain(internal_buffer->length()); + internal_buffer->addBufferFragment(*Buffer::BridgeFragment::createBridgeFragment(*result.extra_data)); + free(result.extra_data); + } + PlatformBridgeFilter::replaceHeaders(trailers, result.trailers); + return Http::FilterTrailersStatus::Continue; + + default: + PANIC("invalid filter state: unsupported status for platform filters"); + } + + NOT_REACHED_GCOVR_EXCL_LINE; } Http::FilterHeadersStatus PlatformBridgeFilter::decodeHeaders(Http::RequestHeaderMap& headers, @@ -203,7 +246,7 @@ Http::FilterDataStatus PlatformBridgeFilter::decodeData(Buffer::Instance& data, }); } - return onData(data, end_stream, internal_buffer, platform_filter_.on_request_data); + return onData(data, end_stream, internal_buffer, &pending_request_headers_, platform_filter_.on_request_data); } Http::FilterDataStatus PlatformBridgeFilter::encodeData(Buffer::Instance& data, bool end_stream) { @@ -215,12 +258,19 @@ Http::FilterDataStatus PlatformBridgeFilter::encodeData(Buffer::Instance& data, }); } - return onData(data, end_stream, internal_buffer, platform_filter_.on_response_data); + return onData(data, end_stream, internal_buffer, &pending_response_headers_, platform_filter_.on_response_data); } Http::FilterTrailersStatus PlatformBridgeFilter::decodeTrailers(Http::RequestTrailerMap& trailers) { // Delegate to shared implementation for request and response path. - auto status = onTrailers(trailers, platform_filter_.on_request_trailers); + Buffer::Instance* internal_buffer = nullptr; + if (decoder_callbacks_->decodingBuffer()) { + decoder_callbacks_->modifyDecodingBuffer([&internal_buffer](Buffer::Instance& mutable_buffer) { + internal_buffer = &mutable_buffer; + }); + } + + auto status = onTrailers(trailers, internal_buffer, &pending_request_headers_, platform_filter_.on_request_trailers); if (status == Http::FilterTrailersStatus::StopIteration) { pending_request_trailers_ = &trailers; } @@ -230,7 +280,14 @@ Http::FilterTrailersStatus PlatformBridgeFilter::decodeTrailers(Http::RequestTra Http::FilterTrailersStatus PlatformBridgeFilter::encodeTrailers(Http::ResponseTrailerMap& trailers) { // Delegate to shared implementation for request and response path. - auto status = onTrailers(trailers, platform_filter_.on_response_trailers); + Buffer::Instance* internal_buffer = nullptr; + if (encoder_callbacks_->encodingBuffer()) { + encoder_callbacks_->modifyEncodingBuffer([&internal_buffer](Buffer::Instance& mutable_buffer) { + internal_buffer = &mutable_buffer; + }); + } + + auto status = onTrailers(trailers, internal_buffer, &pending_response_headers_, platform_filter_.on_response_trailers); if (status == Http::FilterTrailersStatus::StopIteration) { pending_response_trailers_ = &trailers; } diff --git a/library/common/extensions/filters/http/platform_bridge/filter.h b/library/common/extensions/filters/http/platform_bridge/filter.h index c6a0a4910b..92726d254e 100644 --- a/library/common/extensions/filters/http/platform_bridge/filter.h +++ b/library/common/extensions/filters/http/platform_bridge/filter.h @@ -55,19 +55,24 @@ class PlatformBridgeFilter final : public Http::PassThroughFilter, Http::FilterTrailersStatus encodeTrailers(Http::ResponseTrailerMap& trailers) override; private: + static void replaceHeaders(Http::HeaderMap& headers, envoy_headers c_headers); Http::FilterHeadersStatus onHeaders(Http::HeaderMap& headers, bool end_stream, envoy_filter_on_headers_f on_headers); Http::FilterDataStatus onData(Buffer::Instance& data, bool end_stream, - Buffer::Instance* internal_buffer, envoy_filter_on_data_f on_data); + Buffer::Instance* internal_buffer, + Http::HeaderMap** pending_headers, + envoy_filter_on_data_f on_data); Http::FilterTrailersStatus onTrailers(Http::HeaderMap& trailers, + Buffer::Instance* internal_buffer, + Http::HeaderMap** pending_headers, envoy_filter_on_trailers_f on_trailers); const std::string filter_name_; IterationState iteration_state_; envoy_http_filter platform_filter_; - Http::RequestHeaderMap* pending_request_headers_{}; - Http::ResponseHeaderMap* pending_response_headers_{}; - Http::RequestTrailerMap* pending_request_trailers_{}; - Http::ResponseTrialerMap* pending_response_trailers_{}; + Http::HeaderMap* pending_request_headers_{}; + Http::HeaderMap* pending_response_headers_{}; + Http::HeaderMap* pending_request_trailers_{}; + Http::HeaderMap* pending_response_trailers_{}; }; } // namespace PlatformBridge diff --git a/test/common/extensions/filters/http/platform_bridge/platform_bridge_filter_test.cc b/test/common/extensions/filters/http/platform_bridge/platform_bridge_filter_test.cc index 6df17f453a..e68adf7715 100644 --- a/test/common/extensions/filters/http/platform_bridge/platform_bridge_filter_test.cc +++ b/test/common/extensions/filters/http/platform_bridge/platform_bridge_filter_test.cc @@ -165,7 +165,7 @@ TEST_F(PlatformBridgeFilterTest, BasicContinueOnRequestData) { EXPECT_EQ(to_string(c_data), "request body"); EXPECT_TRUE(end_stream); invocations->on_request_data_calls++; - return {kEnvoyFilterDataStatusContinue, c_data}; + return {kEnvoyFilterDataStatusContinue, c_data, nullptr}; }; setUpFilter(R"EOF( @@ -196,7 +196,7 @@ TEST_F(PlatformBridgeFilterTest, StopAndBufferOnRequestData) { EXPECT_EQ(to_string(c_data), expected_data[invocations->on_request_data_calls++]); EXPECT_FALSE(end_stream); c_data.release(c_data.context); - return {kEnvoyFilterDataStatusStopIterationAndBuffer, envoy_nodata}; + return {kEnvoyFilterDataStatusStopIterationAndBuffer, envoy_nodata, nullptr}; }; Buffer::OwnedImpl decoding_buffer; @@ -250,7 +250,7 @@ TEST_F(PlatformBridgeFilterTest, StopNoBufferOnRequestData) { EXPECT_EQ(to_string(c_data), expected_data[invocations->on_request_data_calls++]); EXPECT_FALSE(end_stream); c_data.release(c_data.context); - return {kEnvoyFilterDataStatusStopIterationNoBuffer, envoy_nodata}; + return {kEnvoyFilterDataStatusStopIterationNoBuffer, envoy_nodata, nullptr}; }; setUpFilter(R"EOF( @@ -289,7 +289,7 @@ TEST_F(PlatformBridgeFilterTest, BasicContinueOnRequestTrailers) { EXPECT_EQ(to_string(c_trailers.headers[0].key), "x-test-trailer"); EXPECT_EQ(to_string(c_trailers.headers[0].value), "test trailer"); invocations->on_request_trailers_calls++; - return {kEnvoyFilterTrailersStatusContinue, c_trailers}; + return {kEnvoyFilterTrailersStatusContinue, c_trailers, nullptr, nullptr}; }; setUpFilter(R"EOF( @@ -353,7 +353,7 @@ TEST_F(PlatformBridgeFilterTest, BasicContinueOnResponseData) { EXPECT_EQ(to_string(c_data), "response body"); EXPECT_TRUE(end_stream); invocations->on_response_data_calls++; - return {kEnvoyFilterDataStatusContinue, c_data}; + return {kEnvoyFilterDataStatusContinue, c_data, nullptr}; }; setUpFilter(R"EOF( @@ -384,7 +384,7 @@ TEST_F(PlatformBridgeFilterTest, StopAndBufferOnResponseData) { EXPECT_EQ(to_string(c_data), expected_data[invocations->on_response_data_calls++]); EXPECT_FALSE(end_stream); c_data.release(c_data.context); - return {kEnvoyFilterDataStatusStopIterationAndBuffer, envoy_nodata}; + return {kEnvoyFilterDataStatusStopIterationAndBuffer, envoy_nodata, nullptr}; }; Buffer::OwnedImpl encoding_buffer; @@ -438,7 +438,7 @@ TEST_F(PlatformBridgeFilterTest, StopNoBufferOnResponseData) { EXPECT_EQ(to_string(c_data), expected_data[invocations->on_response_data_calls++]); EXPECT_FALSE(end_stream); c_data.release(c_data.context); - return {kEnvoyFilterDataStatusStopIterationNoBuffer, envoy_nodata}; + return {kEnvoyFilterDataStatusStopIterationNoBuffer, envoy_nodata, nullptr}; }; setUpFilter(R"EOF( @@ -477,7 +477,7 @@ TEST_F(PlatformBridgeFilterTest, BasicContinueOnResponseTrailers) { EXPECT_EQ(to_string(c_trailers.headers[0].key), "x-test-trailer"); EXPECT_EQ(to_string(c_trailers.headers[0].value), "test trailer"); invocations->on_response_trailers_calls++; - return {kEnvoyFilterTrailersStatusContinue, c_trailers}; + return {kEnvoyFilterTrailersStatusContinue, c_trailers, nullptr, nullptr}; }; setUpFilter(R"EOF(