From b137f32db203244d9eee938f88635ee9ec2c2669 Mon Sep 17 00:00:00 2001 From: Feng Li Date: Wed, 12 Apr 2017 14:28:43 -0700 Subject: [PATCH 01/28] Fix heap-use-after-free error when shutting down envoy. The HotRestart holds a socket event for the domain socket used for admin purpose, it need to be deleted before the envoy server gets deleted, as the envoy server own the event_base and which already deleted the registered events with the event_base_free(). --- include/envoy/server/hot_restart.h | 5 +++++ source/exe/hot_restart.cc | 2 ++ source/exe/hot_restart.h | 1 + source/server/server.cc | 2 ++ source/server/server.h | 2 ++ test/integration/server.cc | 1 + test/mocks/server/mocks.h | 1 + 7 files changed, 14 insertions(+) diff --git a/include/envoy/server/hot_restart.h b/include/envoy/server/hot_restart.h index 69e29971e08fa..a86c1afc22cac 100644 --- a/include/envoy/server/hot_restart.h +++ b/include/envoy/server/hot_restart.h @@ -64,6 +64,11 @@ class HotRestart { */ virtual void terminateParent() PURE; + /** + * Shutdown admin processing in the current process. + */ + virtual void shutdownAdmin() PURE; + /** * Return the hot restart compatability version so that operations code can decide whether to * perform a full or hot restart. diff --git a/source/exe/hot_restart.cc b/source/exe/hot_restart.cc index 19f7ab2bf86de..c132ace4bb360 100644 --- a/source/exe/hot_restart.cc +++ b/source/exe/hot_restart.cc @@ -407,6 +407,8 @@ void HotRestartImpl::terminateParent() { parent_terminated_ = true; } +void HotRestartImpl::shutdownAdmin() { socket_event_.reset(); } + std::string HotRestartImpl::version() { return SharedMemory::version(); } } // Server diff --git a/source/exe/hot_restart.h b/source/exe/hot_restart.h index 170f1225e1e9e..e83dca07fd8ed 100644 --- a/source/exe/hot_restart.h +++ b/source/exe/hot_restart.h @@ -112,6 +112,7 @@ class HotRestartImpl : public HotRestart, void initialize(Event::Dispatcher& dispatcher, Server::Instance& server) override; void shutdownParentAdmin(ShutdownParentAdminInfo& info) override; void terminateParent() override; + void shutdownAdmin() override; std::string version() override; // RawStatDataAllocator diff --git a/source/server/server.cc b/source/server/server.cc index 731df52757641..d234e251cfdf7 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -85,6 +85,8 @@ InstanceImpl::InstanceImpl(Options& options, TestHooks& hooks, HotRestart& resta } } +InstanceImpl::~InstanceImpl() { restarter_.shutdownAdmin(); } + Upstream::ClusterManager& InstanceImpl::clusterManager() { return config_->clusterManager(); } Tracing::HttpTracer& InstanceImpl::httpTracer() { return config_->httpTracer(); } diff --git a/source/server/server.h b/source/server/server.h index 956e7c5e14ab7..7c3a0a0dc2ec2 100644 --- a/source/server/server.h +++ b/source/server/server.h @@ -94,6 +94,8 @@ class InstanceImpl : Logger::Loggable, public Instance { Thread::BasicLockable& access_log_lock, ComponentFactory& component_factory, const LocalInfo::LocalInfo& local_info); + ~InstanceImpl() override; + void run(); // Server::Instance diff --git a/test/integration/server.cc b/test/integration/server.cc index d0d8dee7d9db8..1fcecd8054630 100644 --- a/test/integration/server.cc +++ b/test/integration/server.cc @@ -21,6 +21,7 @@ class TestHotRestart : public HotRestart { void initialize(Event::Dispatcher&, Server::Instance&) override {} void shutdownParentAdmin(ShutdownParentAdminInfo&) override {} void terminateParent() override {} + void shutdownAdmin() override {} std::string version() override { return "1"; } }; diff --git a/test/mocks/server/mocks.h b/test/mocks/server/mocks.h index 05cb8d1e7839e..927561b9654bd 100644 --- a/test/mocks/server/mocks.h +++ b/test/mocks/server/mocks.h @@ -80,6 +80,7 @@ class MockHotRestart : public HotRestart { MOCK_METHOD2(initialize, void(Event::Dispatcher& dispatcher, Server::Instance& server)); MOCK_METHOD1(shutdownParentAdmin, void(ShutdownParentAdminInfo& info)); MOCK_METHOD0(terminateParent, void()); + MOCK_METHOD0(shutdownAdmin, void()); MOCK_METHOD0(version, std::string()); }; From d0cec57f55965fb1014337d22ddea34270864af7 Mon Sep 17 00:00:00 2001 From: Feng Li Date: Thu, 13 Apr 2017 14:20:29 -0700 Subject: [PATCH 02/28] Change HotRestart::shutdownAdmin() to HotRestart::shutdown(). --- include/envoy/server/hot_restart.h | 4 ++-- source/exe/hot_restart.cc | 2 +- source/exe/hot_restart.h | 2 +- source/server/server.cc | 2 +- test/integration/server.cc | 2 +- test/mocks/server/mocks.h | 2 +- 6 files changed, 7 insertions(+), 7 deletions(-) diff --git a/include/envoy/server/hot_restart.h b/include/envoy/server/hot_restart.h index a86c1afc22cac..a1d80b2967279 100644 --- a/include/envoy/server/hot_restart.h +++ b/include/envoy/server/hot_restart.h @@ -65,9 +65,9 @@ class HotRestart { virtual void terminateParent() PURE; /** - * Shutdown admin processing in the current process. + * Shutdown the hot restarter. */ - virtual void shutdownAdmin() PURE; + virtual void shutdown() PURE; /** * Return the hot restart compatability version so that operations code can decide whether to diff --git a/source/exe/hot_restart.cc b/source/exe/hot_restart.cc index c132ace4bb360..ebbf21d1f341f 100644 --- a/source/exe/hot_restart.cc +++ b/source/exe/hot_restart.cc @@ -407,7 +407,7 @@ void HotRestartImpl::terminateParent() { parent_terminated_ = true; } -void HotRestartImpl::shutdownAdmin() { socket_event_.reset(); } +void HotRestartImpl::shutdown() { socket_event_.reset(); } std::string HotRestartImpl::version() { return SharedMemory::version(); } diff --git a/source/exe/hot_restart.h b/source/exe/hot_restart.h index e83dca07fd8ed..418195cf0dd5a 100644 --- a/source/exe/hot_restart.h +++ b/source/exe/hot_restart.h @@ -112,7 +112,7 @@ class HotRestartImpl : public HotRestart, void initialize(Event::Dispatcher& dispatcher, Server::Instance& server) override; void shutdownParentAdmin(ShutdownParentAdminInfo& info) override; void terminateParent() override; - void shutdownAdmin() override; + void shutdown() override; std::string version() override; // RawStatDataAllocator diff --git a/source/server/server.cc b/source/server/server.cc index d234e251cfdf7..a6e54fef64f17 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -85,7 +85,7 @@ InstanceImpl::InstanceImpl(Options& options, TestHooks& hooks, HotRestart& resta } } -InstanceImpl::~InstanceImpl() { restarter_.shutdownAdmin(); } +InstanceImpl::~InstanceImpl() { restarter_.shutdown(); } Upstream::ClusterManager& InstanceImpl::clusterManager() { return config_->clusterManager(); } diff --git a/test/integration/server.cc b/test/integration/server.cc index 1fcecd8054630..469652f8e0501 100644 --- a/test/integration/server.cc +++ b/test/integration/server.cc @@ -21,7 +21,7 @@ class TestHotRestart : public HotRestart { void initialize(Event::Dispatcher&, Server::Instance&) override {} void shutdownParentAdmin(ShutdownParentAdminInfo&) override {} void terminateParent() override {} - void shutdownAdmin() override {} + void shutdown() override {} std::string version() override { return "1"; } }; diff --git a/test/mocks/server/mocks.h b/test/mocks/server/mocks.h index 927561b9654bd..1d2840b30a4f0 100644 --- a/test/mocks/server/mocks.h +++ b/test/mocks/server/mocks.h @@ -80,7 +80,7 @@ class MockHotRestart : public HotRestart { MOCK_METHOD2(initialize, void(Event::Dispatcher& dispatcher, Server::Instance& server)); MOCK_METHOD1(shutdownParentAdmin, void(ShutdownParentAdminInfo& info)); MOCK_METHOD0(terminateParent, void()); - MOCK_METHOD0(shutdownAdmin, void()); + MOCK_METHOD0(shutdown, void()); MOCK_METHOD0(version, std::string()); }; From ab3855f520ab57dee2cde60414c3fba9a084a632 Mon Sep 17 00:00:00 2001 From: Feng Li Date: Tue, 25 Apr 2017 15:09:18 -0700 Subject: [PATCH 03/28] Local change for thirdparty.cmake, DO NOT commit to upstream! --- thirdparty.cmake | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/thirdparty.cmake b/thirdparty.cmake index 18a8695e63282..6816260349838 100644 --- a/thirdparty.cmake +++ b/thirdparty.cmake @@ -4,55 +4,55 @@ # https://github.com/sakra/cotire # Last tested with 1.7.8 -set(ENVOY_COTIRE_MODULE_DIR "" CACHE FILEPATH "location of cotire cmake module") +set(ENVOY_COTIRE_MODULE_DIR "/usr/local/google/home/fengli/fengli79/cotire/CMake" CACHE FILEPATH "location of cotire cmake module") # https://github.com/gabime/spdlog # Last tested with 0.11.0 -set(ENVOY_SPDLOG_INCLUDE_DIR "" CACHE FILEPATH "location of spdlog includes") +set(ENVOY_SPDLOG_INCLUDE_DIR "/usr/local/google/home/fengli/fengli79/spdlog/include" CACHE FILEPATH "location of spdlog includes") # https://github.com/nodejs/http-parser # Last tested with 2.7.0 -set(ENVOY_HTTP_PARSER_INCLUDE_DIR "" CACHE FILEPATH "location of http-parser includes") +set(ENVOY_HTTP_PARSER_INCLUDE_DIR "/usr/local/google/home/fengli/fengli79/http-parser" CACHE FILEPATH "location of http-parser includes") # https://github.com/nghttp2/nghttp2 # Last tested with 1.20.0 -set(ENVOY_NGHTTP2_INCLUDE_DIR "" CACHE FILEPATH "location of nghttp2 includes") +set(ENVOY_NGHTTP2_INCLUDE_DIR "/usr/local/google/home/fengli/fengli79/nghttp2/lib/includes" CACHE FILEPATH "location of nghttp2 includes") # http://libevent.org/ # Last tested with 2.1.8 -set(ENVOY_LIBEVENT_INCLUDE_DIR "" CACHE FILEPATH "location of libevent includes") +set(ENVOY_LIBEVENT_INCLUDE_DIR "/usr/local/google/home/fengli/fengli79/libevent/include" CACHE FILEPATH "location of libevent includes") # http://tclap.sourceforge.net/ # Last tested with 1.2.1 -set(ENVOY_TCLAP_INCLUDE_DIR "" CACHE FILEPATH "location of tclap includes") +set(ENVOY_TCLAP_INCLUDE_DIR "/usr/local/google/home/fengli/fengli79/tclap-code/include" CACHE FILEPATH "location of tclap includes") # https://github.com/gperftools/gperftools # Last tested with 2.5.0 -set(ENVOY_GPERFTOOLS_INCLUDE_DIR "" CACHE FILEPATH "location of gperftools includes") +set(ENVOY_GPERFTOOLS_INCLUDE_DIR "/usr/local/google/home/fengli/fengli79/gperftools/src" CACHE FILEPATH "location of gperftools includes") # https://boringssl.googlesource.com/boringssl/+/chromium-stable # Last tested with sha be873e9f48b2a07269300282b69bb17d496c69ee -set(ENVOY_OPENSSL_INCLUDE_DIR "" CACHE FILEPATH "location of openssl includes") +set(ENVOY_OPENSSL_INCLUDE_DIR "/usr/local/google/home/fengli/fengli79/boringssl/include" CACHE FILEPATH "location of openssl includes") # https://github.com/c-ares/c-ares # Last tested with 1.12.0 -set(ENVOY_CARES_INCLUDE_DIR "" CACHE FILEPATH "location of c-ares includes") +set(ENVOY_CARES_INCLUDE_DIR "/usr/local/google/home/fengli/fengli79/c-ares" CACHE FILEPATH "location of c-ares includes") # https://github.com/google/protobuf # Last tested with 3.0.0 -set(ENVOY_PROTOBUF_INCLUDE_DIR "" CACHE FILEPATH "location of protobuf includes") -set(ENVOY_PROTOBUF_PROTOC "" CACHE FILEPATH "location of protoc") +set(ENVOY_PROTOBUF_INCLUDE_DIR "/usr/local/google/home/fengli/fengli79/protobuf/src" CACHE FILEPATH "location of protobuf includes") +set(ENVOY_PROTOBUF_PROTOC "/usr/local/google/home/fengli/fengli79/protobuf/src/protoc" CACHE FILEPATH "location of protoc") # http://lightstep.com/ # Last tested with lightstep-tracer-cpp-0.36 -set(ENVOY_LIGHTSTEP_TRACER_INCLUDE_DIR "" CACHE FILEPATH "location of lighstep tracer includes") +set(ENVOY_LIGHTSTEP_TRACER_INCLUDE_DIR "/usr/local/google/home/fengli/fengli79/lightstep-tracer-cpp/src/c++11" CACHE FILEPATH "location of lighstep tracer includes") # https://github.com/miloyip/rapidjson # Last tested with 1.1.0 -set(ENVOY_RAPIDJSON_INCLUDE_DIR "" CACHE FILEPATH "location of rapidjson includes") +set(ENVOY_RAPIDJSON_INCLUDE_DIR "/usr/local/google/home/fengli/fengli79/rapidjson/include" CACHE FILEPATH "location of rapidjson includes") # Extra linker flags required to properly link envoy with all of the above libraries. -set(ENVOY_EXE_EXTRA_LINKER_FLAGS "" CACHE STRING "envoy extra linker flags") +set(ENVOY_EXE_EXTRA_LINKER_FLAGS "-L/usr/grte/v4/lib64 -L/usr/local/lib -L/usr/local/google/home/fengli/fengli79/gpertools -L/usr/local/google/home/fengli/fengli79/boringssl/build/ssl -L/usr/local/google/home/fengli/fengli79/boringssl/build/crypto -L/usr/local/google/home/fengli/fengli79/nghttp2/lib/.libs -L/usr/local/google/home/fengli/fengli79/lightstep-tracer-cpp/src/c++11/.libs" CACHE STRING "envoy extra linker flags") # # Test Requirements @@ -60,12 +60,12 @@ set(ENVOY_EXE_EXTRA_LINKER_FLAGS "" CACHE STRING "envoy extra linker flags") # https://github.com/google/googletest # Last tested with 1.8.0 -set(ENVOY_GTEST_INCLUDE_DIR "" CACHE FILEPATH "location of gtest includes") -set(ENVOY_GMOCK_INCLUDE_DIR "" CACHE FILEPATH "location of gmock includes") +set(ENVOY_GTEST_INCLUDE_DIR "/usr/local/google/home/fengli/fengli79/googletest/googletest/include" CACHE FILEPATH "location of gtest includes") +set(ENVOY_GMOCK_INCLUDE_DIR "/usr/local/google/home/fengli/fengli79/googletest/googlemock/include" CACHE FILEPATH "location of gmock includes") # http://gcovr.com/ # Last tested with 3.3 -set(ENVOY_GCOVR "" CACHE FILEPATH "location of gcovr") +set(ENVOY_GCOVR "/usr/local/google/home/fengli/fengli79/gcovr" CACHE FILEPATH "location of gcovr") set(ENVOY_GCOVR_EXTRA_ARGS "" CACHE STRING "extra arguments to pass to gcovr") # Extra linker flags required to properly link envoy-test with all of the above libraries. From 8ecec66015b0ba3eb2d5f0072a02dea4261e543f Mon Sep 17 00:00:00 2001 From: fengli Date: Wed, 10 May 2017 13:21:21 -0700 Subject: [PATCH 04/28] Add grpc-web support for envoy. --- source/common/CMakeLists.txt | 1 + source/common/grpc/grpc_web_filter.cc | 104 +++++++++++++++++++++ source/common/grpc/grpc_web_filter.h | 59 ++++++++++++ source/common/http/conn_manager_impl.cc | 3 + source/server/CMakeLists.txt | 1 + source/server/config/http/grpc_web.cc | 28 ++++++ source/server/config/http/grpc_web.h | 16 ++++ test/common/http/conn_manager_impl_test.cc | 3 +- 8 files changed, 214 insertions(+), 1 deletion(-) create mode 100644 source/common/grpc/grpc_web_filter.cc create mode 100644 source/common/grpc/grpc_web_filter.h create mode 100644 source/server/config/http/grpc_web.cc create mode 100644 source/server/config/http/grpc_web.h diff --git a/source/common/CMakeLists.txt b/source/common/CMakeLists.txt index 89dbe630f0eb7..09c0ba4cb815a 100644 --- a/source/common/CMakeLists.txt +++ b/source/common/CMakeLists.txt @@ -47,6 +47,7 @@ add_library( generated/ratelimit.pb.cc grpc/codec.cc grpc/common.cc + grpc/grpc_web_filter.cc grpc/http1_bridge_filter.cc grpc/rpc_channel_impl.cc http/access_log/access_log_formatter.cc diff --git a/source/common/grpc/grpc_web_filter.cc b/source/common/grpc/grpc_web_filter.cc new file mode 100644 index 0000000000000..5b114d586d0db --- /dev/null +++ b/source/common/grpc/grpc_web_filter.cc @@ -0,0 +1,104 @@ +#include "common/grpc/grpc_web_filter.h" + +#include "common/common/base64.h" + +namespace Grpc { + +const std::string GrpcWebFilter::GRPC_WEB_CONTENT_TYPE{"application/grpc-web"}; +const std::string GrpcWebFilter::GRPC_WEB_TEXT_CONTENT_TYPE{"application/grpc-web-text"}; +const std::string GrpcWebFilter::GRPC_CONTENT_TYPE{"application/grpc"}; +const uint8_t GrpcWebFilter::GRPC_WEB_TRAILER = 0b10000000; +const Http::LowerCaseString GrpcWebFilter::HTTP_TE_KEY{"te"}; +const std::string GrpcWebFilter::HTTP_TE_VALUE{"trailers"}; +const Http::LowerCaseString GrpcWebFilter::GRPC_ACCEPT_ENCODING_KEY{"grpc-accept-encoding"}; +const std::string GrpcWebFilter::GRPC_ACCEPT_ENCODING_VALUE{"identity,deflate,gzip"}; + +GrpcWebFilter::GrpcWebFilter() : is_text_request_(false), is_text_response_(false) {} + +GrpcWebFilter::~GrpcWebFilter() {} + +// Implements StreamDecoderFilter. +Http::FilterHeadersStatus GrpcWebFilter::decodeHeaders(Http::HeaderMap& headers, bool) { + const Http::HeaderEntry* content_type = headers.ContentType(); + if (content_type != nullptr && GRPC_WEB_TEXT_CONTENT_TYPE == content_type->value().c_str()) { + is_text_request_ = true; + } + headers.removeContentType(); + headers.insertContentType().value(GRPC_CONTENT_TYPE); + + const Http::HeaderEntry* accept = headers.get(Http::LowerCaseString("accept")); + if (accept != nullptr && GRPC_WEB_TEXT_CONTENT_TYPE == accept->value().c_str()) { + is_text_response_ = true; + } + headers.addStatic(HTTP_TE_KEY, HTTP_TE_VALUE); + headers.addStatic(GRPC_ACCEPT_ENCODING_KEY, GRPC_ACCEPT_ENCODING_VALUE); + return Http::FilterHeadersStatus::Continue; +} + +Http::FilterDataStatus GrpcWebFilter::decodeData(Buffer::Instance& data, bool) { + if (!is_text_request_) { + return Http::FilterDataStatus::Continue; + } + + // Parse application/grpc-web-text format. + if (data.length() + decoding_buffer_.length() < 4) { + decoding_buffer_.move(data); + data.drain(data.length()); + return Http::FilterDataStatus::Continue; + } + + uint64_t needed = (data.length() + decoding_buffer_.length()) / 4 * 4 - decoding_buffer_.length(); + decoding_buffer_.move(data, needed); + std::string decoded = Base64::decode( + std::string(static_cast(decoding_buffer_.linearize(decoding_buffer_.length())), + decoding_buffer_.length())); + decoding_buffer_.drain(decoding_buffer_.length()); + decoding_buffer_.move(data); + data.add(decoded); + return Http::FilterDataStatus::Continue; +} + +// Implements StreamEncoderFilter. +Http::FilterHeadersStatus GrpcWebFilter::encodeHeaders(Http::HeaderMap& headers, bool) { + if (is_text_response_) { + headers.ContentType()->value(GRPC_WEB_TEXT_CONTENT_TYPE); + } else { + headers.ContentType()->value(GRPC_WEB_CONTENT_TYPE); + } + return Http::FilterHeadersStatus::Continue; +} + +Http::FilterDataStatus GrpcWebFilter::encodeData(Buffer::Instance& data, bool) { + if (!is_text_response_) { + return Http::FilterDataStatus::Continue; + } + + // Encodes the response as base64. + std::vector frames; + decoder_.decode(data, frames); + for (auto& frame : frames) { + Buffer::OwnedImpl temp; + temp.add(&frame.flags_, 1); + temp.add(&frame.length_, 4); + temp.add(*frame.data_); + data.add(temp); + } + return Http::FilterDataStatus::Continue; +} + +Http::FilterTrailersStatus GrpcWebFilter::encodeTrailers(Http::HeaderMap& trailers) { + if (!encoder_callbacks_->encodingBuffer()) { + encoder_callbacks_->encodingBuffer().reset(new Buffer::OwnedImpl()); + } + encoder_callbacks_->encodingBuffer()->add(&GrpcWebFilter::GRPC_WEB_TRAILER, 1); + trailers.iterate([](const Http::HeaderEntry& header, void* context) -> void { + Buffer::InstancePtr& buffer = + static_cast(context)->encoder_callbacks_->encodingBuffer(); + buffer->add(header.key().c_str(), header.key().size()); + buffer->add(":"); + buffer->add(header.value().c_str(), header.value().size()); + buffer->add("\r\n"); + }, this); + return Http::FilterTrailersStatus::Continue; +} +} // namespace Grpc diff --git a/source/common/grpc/grpc_web_filter.h b/source/common/grpc/grpc_web_filter.h new file mode 100644 index 0000000000000..3f3e352c5a310 --- /dev/null +++ b/source/common/grpc/grpc_web_filter.h @@ -0,0 +1,59 @@ +#ifndef SOURCE_COMMON_GRPC_GRPC_WEB_FILTER_H_ +#define SOURCE_COMMON_GRPC_GRPC_WEB_FILTER_H_ + +#include "envoy/http/filter.h" +#include "envoy/upstream/cluster_manager.h" + +#include "common/buffer/buffer_impl.h" +#include "common/grpc/codec.h" + +namespace Grpc { + +class GrpcWebFilter : public Http::StreamFilter { +public: + GrpcWebFilter(); + virtual ~GrpcWebFilter(); + + // GrpcWebFilter is neither copyable nor movable. + GrpcWebFilter(const GrpcWebFilter&) = delete; + GrpcWebFilter& operator=(const GrpcWebFilter&) = delete; + + // Implements StreamDecoderFilter. + Http::FilterHeadersStatus decodeHeaders(Http::HeaderMap&, bool) override; + Http::FilterDataStatus decodeData(Buffer::Instance&, bool) override; + + Http::FilterTrailersStatus decodeTrailers(Http::HeaderMap&) { + return Http::FilterTrailersStatus::Continue; + } + void setDecoderFilterCallbacks(Http::StreamDecoderFilterCallbacks& callbacks) { + decoder_callbacks_ = &callbacks; + } + + // Implements StreamEncoderFilter. + Http::FilterHeadersStatus encodeHeaders(Http::HeaderMap&, bool) override; + Http::FilterDataStatus encodeData(Buffer::Instance&, bool) override; + Http::FilterTrailersStatus encodeTrailers(Http::HeaderMap& trailers) override; + void setEncoderFilterCallbacks(Http::StreamEncoderFilterCallbacks& callbacks) { + encoder_callbacks_ = &callbacks; + } + + static const std::string GRPC_WEB_CONTENT_TYPE; + static const std::string GRPC_WEB_TEXT_CONTENT_TYPE; + static const std::string GRPC_CONTENT_TYPE; + static const uint8_t GRPC_WEB_TRAILER; + static const Http::LowerCaseString HTTP_TE_KEY; + static const std::string HTTP_TE_VALUE; + static const Http::LowerCaseString GRPC_ACCEPT_ENCODING_KEY; + static const std::string GRPC_ACCEPT_ENCODING_VALUE; + +private: + Http::StreamDecoderFilterCallbacks* decoder_callbacks_{}; + Http::StreamEncoderFilterCallbacks* encoder_callbacks_{}; + bool is_text_request_; + bool is_text_response_; + Buffer::OwnedImpl decoding_buffer_; + Decoder decoder_; +}; +} // namespace Grpc + +#endif // SOURCE_COMMON_GRPC_GRPC_WEB_FILTER_H_ diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index f4d08c8e8d94d..8b70725697fad 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -726,6 +726,9 @@ void ConnectionManagerImpl::ActiveStream::encodeTrailers(ActiveStreamEncoderFilt }, this); #endif + if (buffered_response_data_ && buffered_response_data_->length() > 0) { + response_encoder_->encodeData(*buffered_response_data_, false); + } response_encoder_->encodeTrailers(trailers); maybeEndEncode(true); } diff --git a/source/server/CMakeLists.txt b/source/server/CMakeLists.txt index 968306274f547..d50ff17ea0f9b 100644 --- a/source/server/CMakeLists.txt +++ b/source/server/CMakeLists.txt @@ -2,6 +2,7 @@ add_library(envoy-server OBJECT config/http/buffer.cc config/http/dynamo.cc config/http/fault.cc + config/http/grpc_web.cc config/http/grpc_http1_bridge.cc config/http/ratelimit.cc config/http/router.cc diff --git a/source/server/config/http/grpc_web.cc b/source/server/config/http/grpc_web.cc new file mode 100644 index 0000000000000..9392402162f47 --- /dev/null +++ b/source/server/config/http/grpc_web.cc @@ -0,0 +1,28 @@ +#include "server/config/http/grpc_web.h" + +#include "common/grpc/grpc_web_filter.h" + +namespace Server { +namespace Configuration { + +HttpFilterFactoryCb GrpcWebFilterConfig::tryCreateFilterFactory(HttpFilterType type, + const std::string& name, + const Json::Object&, + const std::string&, + Server::Instance& server) { + if (type != HttpFilterType::Both || name != "grpc_web") { + return nullptr; + } + + return [&server](Http::FilterChainFactoryCallbacks& callbacks) -> void { + callbacks.addStreamFilter(Http::StreamFilterSharedPtr{new Grpc::GrpcWebFilter()}); + }; +} + +/** + * Static registration for the gRpc-Web filter. @see RegisterHttpFilterConfigFactory. + */ +static RegisterHttpFilterConfigFactory register_; + +} // namespace Configuration +} // namespace Server diff --git a/source/server/config/http/grpc_web.h b/source/server/config/http/grpc_web.h new file mode 100644 index 0000000000000..6639aadc199f6 --- /dev/null +++ b/source/server/config/http/grpc_web.h @@ -0,0 +1,16 @@ +#pragma once + +#include "server/config/network/http_connection_manager.h" + +namespace Server { +namespace Configuration { + +class GrpcWebFilterConfig : public HttpFilterConfigFactory { +public: + HttpFilterFactoryCb tryCreateFilterFactory(HttpFilterType type, const std::string& name, + const Json::Object&, const std::string&, + Server::Instance& server) override; +}; + +} // namespace Configuration +} // namespace Server diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 587e627c866ae..258d939928f4e 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -1043,7 +1043,8 @@ TEST_F(HttpConnectionManagerImplTest, MultipleFilters) { EXPECT_CALL(encoder, encodeHeaders(_, false)); EXPECT_CALL(*encoder_filter2, encodeData(_, false)) .WillOnce(Return(Http::FilterDataStatus::Continue)); - EXPECT_CALL(encoder, encodeData(_, false)); + EXPECT_CALL(encoder, encodeData(_, false)) + .WillOnce(Invoke([](Buffer::Instance& buffer, bool) { buffer.drain(buffer.length()); })); EXPECT_CALL(*encoder_filter2, encodeTrailers(_)) .WillOnce(Return(Http::FilterTrailersStatus::Continue)); EXPECT_CALL(encoder, encodeTrailers(_)); From 4c9a291fe96f968772387ae47d8c760e9468e7c3 Mon Sep 17 00:00:00 2001 From: Feng Li Date: Wed, 10 May 2017 14:22:41 -0700 Subject: [PATCH 05/28] Sync with upstream. --- source/common/CMakeLists.txt | 137 ----------------------------------- source/server/CMakeLists.txt | 36 --------- thirdparty.cmake | 71 ------------------ 3 files changed, 244 deletions(-) delete mode 100644 source/common/CMakeLists.txt delete mode 100644 source/server/CMakeLists.txt delete mode 100644 thirdparty.cmake diff --git a/source/common/CMakeLists.txt b/source/common/CMakeLists.txt deleted file mode 100644 index 09c0ba4cb815a..0000000000000 --- a/source/common/CMakeLists.txt +++ /dev/null @@ -1,137 +0,0 @@ -set(gen_git_sha_target ${CMAKE_CURRENT_BINARY_DIR}/version_generated.cc) -add_custom_target( - gen_git_sha ALL - COMMAND ${PROJECT_SOURCE_DIR}/tools/gen_git_sha.sh ${CMAKE_SOURCE_DIR} ${gen_git_sha_target}) - -add_custom_command( - OUTPUT ${gen_git_sha_target} - DEPENDS gen_git_sha) - -file(MAKE_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/generated) -add_custom_command( - OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/generated/ratelimit.pb.h ${CMAKE_CURRENT_BINARY_DIR}/generated/ratelimit.pb.cc - COMMAND ${ENVOY_PROTOBUF_PROTOC} -I=${CMAKE_CURRENT_SOURCE_DIR}/ratelimit/ - --cpp_out=${CMAKE_CURRENT_BINARY_DIR}/generated - ${CMAKE_CURRENT_SOURCE_DIR}/ratelimit/ratelimit.proto - DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/ratelimit/ratelimit.proto -) - -set_source_files_properties(generated/ratelimit.pb.cc PROPERTIES COMPILE_FLAGS -Wno-unused-parameter) - -add_library( - envoy-common OBJECT - api/api_impl.cc - access_log/access_log_manager_impl.cc - buffer/buffer_impl.cc - common/base64.cc - common/hex.cc - common/logger.cc - common/thread.cc - common/utility.cc - common/version.cc - dynamo/dynamo_filter.cc - dynamo/dynamo_request_parser.cc - dynamo/dynamo_utility.cc - event/dispatcher_impl.cc - event/event_impl_base.cc - event/file_event_impl.cc - event/libevent.cc - event/signal_impl.cc - event/timer_impl.cc - filesystem/filesystem_impl.cc - filesystem/watcher_impl.cc - filter/auth/client_ssl.cc - filter/echo.cc - filter/ratelimit.cc - filter/tcp_proxy.cc - generated/ratelimit.pb.cc - grpc/codec.cc - grpc/common.cc - grpc/grpc_web_filter.cc - grpc/http1_bridge_filter.cc - grpc/rpc_channel_impl.cc - http/access_log/access_log_formatter.cc - http/access_log/access_log_impl.cc - http/async_client_impl.cc - http/codec_client.cc - http/codes.cc - http/conn_manager_impl.cc - http/conn_manager_utility.cc - http/date_provider_impl.cc - http/header_map_impl.cc - http/message_impl.cc - http/http1/codec_impl.cc - http/http1/conn_pool.cc - http/http2/codec_impl.cc - http/http2/conn_pool.cc - http/filter/buffer_filter.cc - http/filter/fault_filter.cc - http/filter/ratelimit.cc - http/rest_api_fetcher.cc - http/user_agent.cc - http/utility.cc - json/config_schemas.cc - json/json_loader.cc - memory/stats.cc - mongo/bson_impl.cc - mongo/codec_impl.cc - mongo/proxy.cc - mongo/utility.cc - network/address_impl.cc - network/cidr_range.cc - network/connection_impl.cc - network/dns_impl.cc - network/filter_manager_impl.cc - network/listener_impl.cc - network/listen_socket_impl.cc - network/proxy_protocol.cc - network/utility.cc - profiler/profiler.cc - ratelimit/ratelimit_impl.cc - redis/codec_impl.cc - redis/command_splitter_impl.cc - redis/conn_pool_impl.cc - redis/proxy_filter.cc - router/config_impl.cc - router/config_utility.cc - router/rds_impl.cc - router/retry_state_impl.cc - router/router.cc - router/router_ratelimit.cc - router/shadow_writer_impl.cc - runtime/runtime_impl.cc - runtime/uuid_util.cc - ssl/connection_impl.cc - ssl/context_config_impl.cc - ssl/context_impl.cc - ssl/context_manager_impl.cc - stats/stats_impl.cc - stats/statsd.cc - stats/thread_local_store.cc - thread_local/thread_local_impl.cc - tracing/http_tracer_impl.cc - tracing/lightstep_tracer_impl.cc - upstream/cds_api_impl.cc - upstream/cluster_manager_impl.cc - upstream/health_checker_impl.cc - upstream/host_utility.cc - upstream/load_balancer_impl.cc - upstream/logical_dns_cluster.cc - upstream/outlier_detection_impl.cc - upstream/ring_hash_lb.cc - upstream/sds.cc - upstream/upstream_impl.cc - ${gen_git_sha_target}) - -include_directories(SYSTEM ${ENVOY_HTTP_PARSER_INCLUDE_DIR}) -include_directories(${ENVOY_RAPIDJSON_INCLUDE_DIR}) - -if (NOT ENVOY_SANITIZE) - include_directories(${ENVOY_GPERFTOOLS_INCLUDE_DIR}) -endif() - -include_directories(${ENVOY_CARES_INCLUDE_DIR}) -include_directories(${ENVOY_LIBEVENT_INCLUDE_DIR}) -include_directories(${ENVOY_NGHTTP2_INCLUDE_DIR}) -include_directories(SYSTEM ${ENVOY_OPENSSL_INCLUDE_DIR}) -include_directories(SYSTEM ${ENVOY_LIGHTSTEP_TRACER_INCLUDE_DIR}) diff --git a/source/server/CMakeLists.txt b/source/server/CMakeLists.txt deleted file mode 100644 index d50ff17ea0f9b..0000000000000 --- a/source/server/CMakeLists.txt +++ /dev/null @@ -1,36 +0,0 @@ -add_library(envoy-server OBJECT - config/http/buffer.cc - config/http/dynamo.cc - config/http/fault.cc - config/http/grpc_web.cc - config/http/grpc_http1_bridge.cc - config/http/ratelimit.cc - config/http/router.cc - config/network/client_ssl_auth.cc - config/network/echo.cc - config/network/http_connection_manager.cc - config/network/mongo_proxy.cc - config/network/ratelimit.cc - config/network/redis_proxy.cc - config/network/tcp_proxy.cc - configuration_impl.cc - connection_handler_impl.cc - drain_manager_impl.cc - guarddog_impl.cc - http/admin.cc - http/health_check.cc - options_impl.cc - server.cc - watchdog_impl.cc - worker.cc) - -include_directories(SYSTEM ${ENVOY_TCLAP_INCLUDE_DIR}) -include_directories(SYSTEM ${ENVOY_HTTP_PARSER_INCLUDE_DIR}) -include_directories(${ENVOY_NGHTTP2_INCLUDE_DIR}) -include_directories(SYSTEM ${ENVOY_OPENSSL_INCLUDE_DIR}) -include_directories(SYSTEM ${ENVOY_LIGHTSTEP_TRACER_INCLUDE_DIR}) -include_directories(${ENVOY_LIBEVENT_INCLUDE_DIR}) - -# Needed due to generated proto headers. There is probably a way to have this only depend on the -# generation of the header but I don't feel like figuring that out right now. -add_dependencies(envoy-server envoy-common) diff --git a/thirdparty.cmake b/thirdparty.cmake deleted file mode 100644 index a0fe3f867851f..0000000000000 --- a/thirdparty.cmake +++ /dev/null @@ -1,71 +0,0 @@ -# NOTE: These are all of the third party requirements required to build Envoy. We realize this is -# not the cleanest cmake way of doing things and we welcome patches from cmake experts to -# make it better. - -# https://github.com/sakra/cotire -# Last tested with 1.7.8 -set(ENVOY_COTIRE_MODULE_DIR "/usr/local/google/home/fengli/fengli79/cotire/CMake" CACHE FILEPATH "location of cotire cmake module") - -# https://github.com/gabime/spdlog -# Last tested with 0.11.0 -set(ENVOY_SPDLOG_INCLUDE_DIR "/usr/local/google/home/fengli/fengli79/spdlog/include" CACHE FILEPATH "location of spdlog includes") - -# https://github.com/nodejs/http-parser -# Last tested with 2.7.0 -set(ENVOY_HTTP_PARSER_INCLUDE_DIR "/usr/local/google/home/fengli/fengli79/http-parser" CACHE FILEPATH "location of http-parser includes") - -# https://github.com/nghttp2/nghttp2 -# Last tested with 1.20.0 -set(ENVOY_NGHTTP2_INCLUDE_DIR "/usr/local/google/home/fengli/fengli79/nghttp2/lib/includes" CACHE FILEPATH "location of nghttp2 includes") - -# http://libevent.org/ -# Last tested with 2.1.8 -set(ENVOY_LIBEVENT_INCLUDE_DIR "/usr/local/google/home/fengli/fengli79/libevent/include" CACHE FILEPATH "location of libevent includes") - -# http://tclap.sourceforge.net/ -# Last tested with 1.2.1 -set(ENVOY_TCLAP_INCLUDE_DIR "/usr/local/google/home/fengli/fengli79/tclap-code/include" CACHE FILEPATH "location of tclap includes") - -# https://github.com/gperftools/gperftools -# Last tested with 2.5.0 -set(ENVOY_GPERFTOOLS_INCLUDE_DIR "/usr/local/google/home/fengli/fengli79/gperftools/src" CACHE FILEPATH "location of gperftools includes") - -# https://boringssl.googlesource.com/boringssl/+/chromium-stable -set(ENVOY_OPENSSL_INCLUDE_DIR "/usr/local/google/home/fengli/fengli79/boringssl/include" CACHE FILEPATH "location of openssl includes") - -# https://github.com/c-ares/c-ares -# Last tested with 1.12.0 -set(ENVOY_CARES_INCLUDE_DIR "/usr/local/google/home/fengli/fengli79/c-ares" CACHE FILEPATH "location of c-ares includes") - -# https://github.com/google/protobuf -# Last tested with 3.0.0 -set(ENVOY_PROTOBUF_INCLUDE_DIR "/usr/local/google/home/fengli/fengli79/protobuf/src" CACHE FILEPATH "location of protobuf includes") -set(ENVOY_PROTOBUF_PROTOC "/usr/local/google/home/fengli/fengli79/protobuf/src/protoc" CACHE FILEPATH "location of protoc") - -# http://lightstep.com/ -# Last tested with lightstep-tracer-cpp-0.36 -set(ENVOY_LIGHTSTEP_TRACER_INCLUDE_DIR "/usr/local/google/home/fengli/fengli79/lightstep-tracer-cpp/src/c++11" CACHE FILEPATH "location of lighstep tracer includes") - -# https://github.com/miloyip/rapidjson -# Last tested with 1.1.0 -set(ENVOY_RAPIDJSON_INCLUDE_DIR "/usr/local/google/home/fengli/fengli79/rapidjson/include" CACHE FILEPATH "location of rapidjson includes") - -# Extra linker flags required to properly link envoy with all of the above libraries. -set(ENVOY_EXE_EXTRA_LINKER_FLAGS "-L/usr/grte/v4/lib64 -L/usr/local/lib -L/usr/local/google/home/fengli/fengli79/gpertools -L/usr/local/google/home/fengli/fengli79/boringssl/build/ssl -L/usr/local/google/home/fengli/fengli79/boringssl/build/crypto -L/usr/local/google/home/fengli/fengli79/nghttp2/lib/.libs -L/usr/local/google/home/fengli/fengli79/lightstep-tracer-cpp/src/c++11/.libs" CACHE STRING "envoy extra linker flags") - -# -# Test Requirements -# - -# https://github.com/google/googletest -# Last tested with 1.8.0 -set(ENVOY_GTEST_INCLUDE_DIR "/usr/local/google/home/fengli/fengli79/googletest/googletest/include" CACHE FILEPATH "location of gtest includes") -set(ENVOY_GMOCK_INCLUDE_DIR "/usr/local/google/home/fengli/fengli79/googletest/googlemock/include" CACHE FILEPATH "location of gmock includes") - -# http://gcovr.com/ -# Last tested with 3.3 -set(ENVOY_GCOVR "/usr/local/google/home/fengli/fengli79/gcovr" CACHE FILEPATH "location of gcovr") -set(ENVOY_GCOVR_EXTRA_ARGS "" CACHE STRING "extra arguments to pass to gcovr") - -# Extra linker flags required to properly link envoy-test with all of the above libraries. -set(ENVOY_TEST_EXTRA_LINKER_FLAGS "" CACHE STRING "envoy-test extra linker flags") From 43297c31485ba9ad728d4cf1b5e7d1414b260055 Mon Sep 17 00:00:00 2001 From: Feng Li Date: Wed, 10 May 2017 16:27:52 -0700 Subject: [PATCH 06/28] Fix the trailer encoding of grpc-web. --- source/common/grpc/grpc_web_filter.cc | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/source/common/grpc/grpc_web_filter.cc b/source/common/grpc/grpc_web_filter.cc index 5b114d586d0db..740a30bbb7b8f 100644 --- a/source/common/grpc/grpc_web_filter.cc +++ b/source/common/grpc/grpc_web_filter.cc @@ -92,13 +92,15 @@ Http::FilterTrailersStatus GrpcWebFilter::encodeTrailers(Http::HeaderMap& traile } encoder_callbacks_->encodingBuffer()->add(&GrpcWebFilter::GRPC_WEB_TRAILER, 1); trailers.iterate([](const Http::HeaderEntry& header, void* context) -> void { - Buffer::InstancePtr& buffer = - static_cast(context)->encoder_callbacks_->encodingBuffer(); - buffer->add(header.key().c_str(), header.key().size()); - buffer->add(":"); - buffer->add(header.value().c_str(), header.value().size()); - buffer->add("\r\n"); + Buffer::Instance& temp = static_cast(context)->encoding_buffer_trailers_; + temp.add(header.key().c_str(), header.key().size()); + temp.add(":"); + temp.add(header.value().c_str(), header.value().size()); + temp.add("\r\n"); }, this); + uint64_t length = htonl(encoding_buffer_trailers_.length()); + encoder_callbacks_->encodingBuffer()->add(&length, 4); + encoder_callbacks_->encodingBuffer()->move(encoding_buffer_trailers_); return Http::FilterTrailersStatus::Continue; } } // namespace Grpc From 2f20d0d1b850915833553c2447d45932d32411a0 Mon Sep 17 00:00:00 2001 From: Feng Li Date: Thu, 11 May 2017 16:02:55 -0700 Subject: [PATCH 07/28] Modify bazel BUILD files to add grpc-web module. --- source/common/grpc/BUILD | 18 +++++++ source/common/grpc/grpc_web_filter.cc | 30 +++++------ source/common/grpc/grpc_web_filter.h | 59 +++++++++++++++++----- source/exe/BUILD | 1 + source/server/config/http/BUILD | 11 ++++ source/server/config/http/grpc_web.cc | 4 +- test/common/http/conn_manager_impl_test.cc | 3 +- 7 files changed, 94 insertions(+), 32 deletions(-) diff --git a/source/common/grpc/BUILD b/source/common/grpc/BUILD index f4f1b6c13e119..5ee87f7fa7cfe 100644 --- a/source/common/grpc/BUILD +++ b/source/common/grpc/BUILD @@ -56,6 +56,24 @@ envoy_cc_library( ], ) +envoy_cc_library( + name = "grpc_web_filter_lib", + srcs = ["grpc_web_filter.cc"], + hdrs = ["grpc_web_filter.h"], + deps = [ + ":codec_lib", + ":common_lib", + "//include/envoy/http:codes_interface", + "//include/envoy/http:filter_interface", + "//include/envoy/upstream:cluster_manager_interface", + "//source/common/common:base64_lib", + "//source/common/common:enum_to_int", + "//source/common/common:utility_lib", + "//source/common/http:headers_lib", + "//source/common/http/http1:codec_lib", + ], +) + envoy_cc_library( name = "rpc_channel_lib", srcs = ["rpc_channel_impl.cc"], diff --git a/source/common/grpc/grpc_web_filter.cc b/source/common/grpc/grpc_web_filter.cc index 740a30bbb7b8f..60543452e4daf 100644 --- a/source/common/grpc/grpc_web_filter.cc +++ b/source/common/grpc/grpc_web_filter.cc @@ -1,18 +1,11 @@ #include "common/grpc/grpc_web_filter.h" +#include + #include "common/common/base64.h" namespace Grpc { -const std::string GrpcWebFilter::GRPC_WEB_CONTENT_TYPE{"application/grpc-web"}; -const std::string GrpcWebFilter::GRPC_WEB_TEXT_CONTENT_TYPE{"application/grpc-web-text"}; -const std::string GrpcWebFilter::GRPC_CONTENT_TYPE{"application/grpc"}; -const uint8_t GrpcWebFilter::GRPC_WEB_TRAILER = 0b10000000; -const Http::LowerCaseString GrpcWebFilter::HTTP_TE_KEY{"te"}; -const std::string GrpcWebFilter::HTTP_TE_VALUE{"trailers"}; -const Http::LowerCaseString GrpcWebFilter::GRPC_ACCEPT_ENCODING_KEY{"grpc-accept-encoding"}; -const std::string GrpcWebFilter::GRPC_ACCEPT_ENCODING_VALUE{"identity,deflate,gzip"}; - GrpcWebFilter::GrpcWebFilter() : is_text_request_(false), is_text_response_(false) {} GrpcWebFilter::~GrpcWebFilter() {} @@ -20,18 +13,21 @@ GrpcWebFilter::~GrpcWebFilter() {} // Implements StreamDecoderFilter. Http::FilterHeadersStatus GrpcWebFilter::decodeHeaders(Http::HeaderMap& headers, bool) { const Http::HeaderEntry* content_type = headers.ContentType(); - if (content_type != nullptr && GRPC_WEB_TEXT_CONTENT_TYPE == content_type->value().c_str()) { + if (content_type != nullptr && + Constants::get().CONTENT_TYPE_GRPC_WEB_TEXT() == content_type->value().c_str()) { is_text_request_ = true; } headers.removeContentType(); - headers.insertContentType().value(GRPC_CONTENT_TYPE); + headers.insertContentType().value(Constants::get().CONTENT_TYPE_GRPC()); const Http::HeaderEntry* accept = headers.get(Http::LowerCaseString("accept")); - if (accept != nullptr && GRPC_WEB_TEXT_CONTENT_TYPE == accept->value().c_str()) { + if (accept != nullptr && + Constants::get().CONTENT_TYPE_GRPC_WEB_TEXT() == accept->value().c_str()) { is_text_response_ = true; } - headers.addStatic(HTTP_TE_KEY, HTTP_TE_VALUE); - headers.addStatic(GRPC_ACCEPT_ENCODING_KEY, GRPC_ACCEPT_ENCODING_VALUE); + headers.addStatic(Constants::get().HTTP_KEY_TE(), Constants::get().HTTP_KEY_TE_VALUE()); + headers.addStatic(Constants::get().HTTP_KEY_GRPC_ACCEPT_ENCODING(), + Constants::get().HTTP_KEY_GRPC_ACCEPT_ENCODING_VALUE()); return Http::FilterHeadersStatus::Continue; } @@ -61,9 +57,9 @@ Http::FilterDataStatus GrpcWebFilter::decodeData(Buffer::Instance& data, bool) { // Implements StreamEncoderFilter. Http::FilterHeadersStatus GrpcWebFilter::encodeHeaders(Http::HeaderMap& headers, bool) { if (is_text_response_) { - headers.ContentType()->value(GRPC_WEB_TEXT_CONTENT_TYPE); + headers.ContentType()->value(Constants::get().CONTENT_TYPE_GRPC_WEB_TEXT()); } else { - headers.ContentType()->value(GRPC_WEB_CONTENT_TYPE); + headers.ContentType()->value(Constants::get().CONTENT_TYPE_GRPC_WEB()); } return Http::FilterHeadersStatus::Continue; } @@ -90,7 +86,7 @@ Http::FilterTrailersStatus GrpcWebFilter::encodeTrailers(Http::HeaderMap& traile if (!encoder_callbacks_->encodingBuffer()) { encoder_callbacks_->encodingBuffer().reset(new Buffer::OwnedImpl()); } - encoder_callbacks_->encodingBuffer()->add(&GrpcWebFilter::GRPC_WEB_TRAILER, 1); + encoder_callbacks_->encodingBuffer()->add(&Constants::get().GRPC_WEB_TRAILER, 1); trailers.iterate([](const Http::HeaderEntry& header, void* context) -> void { Buffer::Instance& temp = static_cast(context)->encoding_buffer_trailers_; temp.add(header.key().c_str(), header.key().size()); diff --git a/source/common/grpc/grpc_web_filter.h b/source/common/grpc/grpc_web_filter.h index 3f3e352c5a310..dbd0ca35708dc 100644 --- a/source/common/grpc/grpc_web_filter.h +++ b/source/common/grpc/grpc_web_filter.h @@ -11,6 +11,49 @@ namespace Grpc { class GrpcWebFilter : public Http::StreamFilter { public: + class Constants { + public: + static Constants& get() { + static Constants instance; + return instance; + } + + // Constants is neither copyable nor movable. + Constants(const Constants&) = delete; + Constants& operator=(const Constants&) = delete; + + const std::string& CONTENT_TYPE_GRPC_WEB() { return content_type_grpc_web_; } + + const std::string& CONTENT_TYPE_GRPC_WEB_TEXT() { return content_type_grpc_web_text_; } + + const std::string& CONTENT_TYPE_GRPC() { return content_type_grpc_; } + + const Http::LowerCaseString& HTTP_KEY_TE() { return http_key_te_; } + + const std::string& HTTP_KEY_TE_VALUE() { return http_key_te_value_; } + + const Http::LowerCaseString& HTTP_KEY_GRPC_ACCEPT_ENCODING() { + return http_key_grpc_accept_encoding; + } + + const std::string& HTTP_KEY_GRPC_ACCEPT_ENCODING_VALUE() { + return http_key_grpc_accept_encoding_value_; + } + + const uint8_t GRPC_WEB_TRAILER = 0b10000000; + + private: + Constants() {} + + const std::string content_type_grpc_web_ = "application/grpc-web"; + const std::string content_type_grpc_web_text_ = "application/grpc-web-text"; + const std::string content_type_grpc_ = "application/grpc"; + const Http::LowerCaseString http_key_te_{"te"}; + const std::string http_key_te_value_ = "trailers"; + const Http::LowerCaseString http_key_grpc_accept_encoding{"grpc-accept-encoding"}; + const std::string http_key_grpc_accept_encoding_value_ = "identity,deflate,gzip"; + }; + GrpcWebFilter(); virtual ~GrpcWebFilter(); @@ -22,10 +65,10 @@ class GrpcWebFilter : public Http::StreamFilter { Http::FilterHeadersStatus decodeHeaders(Http::HeaderMap&, bool) override; Http::FilterDataStatus decodeData(Buffer::Instance&, bool) override; - Http::FilterTrailersStatus decodeTrailers(Http::HeaderMap&) { + Http::FilterTrailersStatus decodeTrailers(Http::HeaderMap&) override { return Http::FilterTrailersStatus::Continue; } - void setDecoderFilterCallbacks(Http::StreamDecoderFilterCallbacks& callbacks) { + void setDecoderFilterCallbacks(Http::StreamDecoderFilterCallbacks& callbacks) override { decoder_callbacks_ = &callbacks; } @@ -33,25 +76,17 @@ class GrpcWebFilter : public Http::StreamFilter { Http::FilterHeadersStatus encodeHeaders(Http::HeaderMap&, bool) override; Http::FilterDataStatus encodeData(Buffer::Instance&, bool) override; Http::FilterTrailersStatus encodeTrailers(Http::HeaderMap& trailers) override; - void setEncoderFilterCallbacks(Http::StreamEncoderFilterCallbacks& callbacks) { + void setEncoderFilterCallbacks(Http::StreamEncoderFilterCallbacks& callbacks) override { encoder_callbacks_ = &callbacks; } - static const std::string GRPC_WEB_CONTENT_TYPE; - static const std::string GRPC_WEB_TEXT_CONTENT_TYPE; - static const std::string GRPC_CONTENT_TYPE; - static const uint8_t GRPC_WEB_TRAILER; - static const Http::LowerCaseString HTTP_TE_KEY; - static const std::string HTTP_TE_VALUE; - static const Http::LowerCaseString GRPC_ACCEPT_ENCODING_KEY; - static const std::string GRPC_ACCEPT_ENCODING_VALUE; - private: Http::StreamDecoderFilterCallbacks* decoder_callbacks_{}; Http::StreamEncoderFilterCallbacks* encoder_callbacks_{}; bool is_text_request_; bool is_text_response_; Buffer::OwnedImpl decoding_buffer_; + Buffer::OwnedImpl encoding_buffer_trailers_; Decoder decoder_; }; } // namespace Grpc diff --git a/source/exe/BUILD b/source/exe/BUILD index 192c7ca8fd581..aab282df72ff5 100644 --- a/source/exe/BUILD +++ b/source/exe/BUILD @@ -35,6 +35,7 @@ envoy_cc_library( "//source/server/config/http:dynamo_lib", "//source/server/config/http:fault_lib", "//source/server/config/http:grpc_http1_bridge_lib", + "//source/server/config/http:grpc_web_lib", "//source/server/config/http:ratelimit_lib", "//source/server/config/http:router_lib", "//source/server/config/network:client_ssl_auth_lib", diff --git a/source/server/config/http/BUILD b/source/server/config/http/BUILD index 49800b12c3492..551a500ec2b15 100644 --- a/source/server/config/http/BUILD +++ b/source/server/config/http/BUILD @@ -53,6 +53,17 @@ envoy_cc_library( ], ) +envoy_cc_library( + name = "grpc_web_lib", + srcs = ["grpc_web.cc"], + hdrs = ["grpc_web.h"], + deps = [ + "//include/envoy/server:instance_interface", + "//source/common/grpc:grpc_web_filter_lib", + "//source/server/config/network:http_connection_manager_lib", + ], +) + envoy_cc_library( name = "ratelimit_lib", srcs = ["ratelimit.cc"], diff --git a/source/server/config/http/grpc_web.cc b/source/server/config/http/grpc_web.cc index 9392402162f47..ad03b2f96b52a 100644 --- a/source/server/config/http/grpc_web.cc +++ b/source/server/config/http/grpc_web.cc @@ -9,12 +9,12 @@ HttpFilterFactoryCb GrpcWebFilterConfig::tryCreateFilterFactory(HttpFilterType t const std::string& name, const Json::Object&, const std::string&, - Server::Instance& server) { + Server::Instance&) { if (type != HttpFilterType::Both || name != "grpc_web") { return nullptr; } - return [&server](Http::FilterChainFactoryCallbacks& callbacks) -> void { + return [](Http::FilterChainFactoryCallbacks& callbacks) -> void { callbacks.addStreamFilter(Http::StreamFilterSharedPtr{new Grpc::GrpcWebFilter()}); }; } diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 6f81d5ba4ee11..918c978f14c3c 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -1049,7 +1049,8 @@ TEST_F(HttpConnectionManagerImplTest, MultipleFilters) { EXPECT_CALL(*encoder_filter2, encodeData(_, false)) .WillOnce(Return(Http::FilterDataStatus::Continue)); EXPECT_CALL(encoder, encodeData(_, false)) - .WillOnce(Invoke([](Buffer::Instance& buffer, bool) { buffer.drain(buffer.length()); })); + .WillRepeatedly( + Invoke([](Buffer::Instance& buffer, bool) { buffer.drain(buffer.length()); })); EXPECT_CALL(*encoder_filter2, encodeTrailers(_)) .WillOnce(Return(Http::FilterTrailersStatus::Continue)); EXPECT_CALL(encoder, encodeTrailers(_)); From 7a205c9a5dd16c8f68026d95a788a9022bb6450d Mon Sep 17 00:00:00 2001 From: Feng Li Date: Thu, 11 May 2017 16:45:32 -0700 Subject: [PATCH 08/28] Change to use the Http::Headers singleton for gRPC-Web relative headers. --- source/common/grpc/BUILD | 5 --- source/common/grpc/grpc_web_filter.cc | 21 +++++++------ source/common/grpc/grpc_web_filter.h | 45 +-------------------------- source/common/http/headers.h | 13 ++++++++ 4 files changed, 26 insertions(+), 58 deletions(-) diff --git a/source/common/grpc/BUILD b/source/common/grpc/BUILD index 5ee87f7fa7cfe..0f235ef49def4 100644 --- a/source/common/grpc/BUILD +++ b/source/common/grpc/BUILD @@ -63,14 +63,9 @@ envoy_cc_library( deps = [ ":codec_lib", ":common_lib", - "//include/envoy/http:codes_interface", "//include/envoy/http:filter_interface", - "//include/envoy/upstream:cluster_manager_interface", "//source/common/common:base64_lib", - "//source/common/common:enum_to_int", - "//source/common/common:utility_lib", "//source/common/http:headers_lib", - "//source/common/http/http1:codec_lib", ], ) diff --git a/source/common/grpc/grpc_web_filter.cc b/source/common/grpc/grpc_web_filter.cc index 60543452e4daf..b11644154b0b5 100644 --- a/source/common/grpc/grpc_web_filter.cc +++ b/source/common/grpc/grpc_web_filter.cc @@ -3,9 +3,12 @@ #include #include "common/common/base64.h" +#include "common/http/headers.h" namespace Grpc { +const uint8_t GrpcWebFilter::GRPC_WEB_TRAILER = 0b10000000; + GrpcWebFilter::GrpcWebFilter() : is_text_request_(false), is_text_response_(false) {} GrpcWebFilter::~GrpcWebFilter() {} @@ -14,20 +17,20 @@ GrpcWebFilter::~GrpcWebFilter() {} Http::FilterHeadersStatus GrpcWebFilter::decodeHeaders(Http::HeaderMap& headers, bool) { const Http::HeaderEntry* content_type = headers.ContentType(); if (content_type != nullptr && - Constants::get().CONTENT_TYPE_GRPC_WEB_TEXT() == content_type->value().c_str()) { + Http::Headers::get().ContentTypeValues.GrpcWebText == content_type->value().c_str()) { is_text_request_ = true; } headers.removeContentType(); - headers.insertContentType().value(Constants::get().CONTENT_TYPE_GRPC()); + headers.insertContentType().value(Http::Headers::get().ContentTypeValues.Grpc); const Http::HeaderEntry* accept = headers.get(Http::LowerCaseString("accept")); if (accept != nullptr && - Constants::get().CONTENT_TYPE_GRPC_WEB_TEXT() == accept->value().c_str()) { + Http::Headers::get().ContentTypeValues.GrpcWebText == accept->value().c_str()) { is_text_response_ = true; } - headers.addStatic(Constants::get().HTTP_KEY_TE(), Constants::get().HTTP_KEY_TE_VALUE()); - headers.addStatic(Constants::get().HTTP_KEY_GRPC_ACCEPT_ENCODING(), - Constants::get().HTTP_KEY_GRPC_ACCEPT_ENCODING_VALUE()); + headers.addStatic(Http::Headers::get().TE, Http::Headers::get().TEValues.Trailers); + headers.addStatic(Http::Headers::get().GrpcAcceptEncoding, + Http::Headers::get().GrpcAcceptEncodingValues.Default); return Http::FilterHeadersStatus::Continue; } @@ -57,9 +60,9 @@ Http::FilterDataStatus GrpcWebFilter::decodeData(Buffer::Instance& data, bool) { // Implements StreamEncoderFilter. Http::FilterHeadersStatus GrpcWebFilter::encodeHeaders(Http::HeaderMap& headers, bool) { if (is_text_response_) { - headers.ContentType()->value(Constants::get().CONTENT_TYPE_GRPC_WEB_TEXT()); + headers.ContentType()->value(Http::Headers::get().ContentTypeValues.GrpcWebText); } else { - headers.ContentType()->value(Constants::get().CONTENT_TYPE_GRPC_WEB()); + headers.ContentType()->value(Http::Headers::get().ContentTypeValues.GrpcWeb); } return Http::FilterHeadersStatus::Continue; } @@ -86,7 +89,7 @@ Http::FilterTrailersStatus GrpcWebFilter::encodeTrailers(Http::HeaderMap& traile if (!encoder_callbacks_->encodingBuffer()) { encoder_callbacks_->encodingBuffer().reset(new Buffer::OwnedImpl()); } - encoder_callbacks_->encodingBuffer()->add(&Constants::get().GRPC_WEB_TRAILER, 1); + encoder_callbacks_->encodingBuffer()->add(&GRPC_WEB_TRAILER, 1); trailers.iterate([](const Http::HeaderEntry& header, void* context) -> void { Buffer::Instance& temp = static_cast(context)->encoding_buffer_trailers_; temp.add(header.key().c_str(), header.key().size()); diff --git a/source/common/grpc/grpc_web_filter.h b/source/common/grpc/grpc_web_filter.h index dbd0ca35708dc..db845af77d463 100644 --- a/source/common/grpc/grpc_web_filter.h +++ b/source/common/grpc/grpc_web_filter.h @@ -2,7 +2,6 @@ #define SOURCE_COMMON_GRPC_GRPC_WEB_FILTER_H_ #include "envoy/http/filter.h" -#include "envoy/upstream/cluster_manager.h" #include "common/buffer/buffer_impl.h" #include "common/grpc/codec.h" @@ -11,49 +10,6 @@ namespace Grpc { class GrpcWebFilter : public Http::StreamFilter { public: - class Constants { - public: - static Constants& get() { - static Constants instance; - return instance; - } - - // Constants is neither copyable nor movable. - Constants(const Constants&) = delete; - Constants& operator=(const Constants&) = delete; - - const std::string& CONTENT_TYPE_GRPC_WEB() { return content_type_grpc_web_; } - - const std::string& CONTENT_TYPE_GRPC_WEB_TEXT() { return content_type_grpc_web_text_; } - - const std::string& CONTENT_TYPE_GRPC() { return content_type_grpc_; } - - const Http::LowerCaseString& HTTP_KEY_TE() { return http_key_te_; } - - const std::string& HTTP_KEY_TE_VALUE() { return http_key_te_value_; } - - const Http::LowerCaseString& HTTP_KEY_GRPC_ACCEPT_ENCODING() { - return http_key_grpc_accept_encoding; - } - - const std::string& HTTP_KEY_GRPC_ACCEPT_ENCODING_VALUE() { - return http_key_grpc_accept_encoding_value_; - } - - const uint8_t GRPC_WEB_TRAILER = 0b10000000; - - private: - Constants() {} - - const std::string content_type_grpc_web_ = "application/grpc-web"; - const std::string content_type_grpc_web_text_ = "application/grpc-web-text"; - const std::string content_type_grpc_ = "application/grpc"; - const Http::LowerCaseString http_key_te_{"te"}; - const std::string http_key_te_value_ = "trailers"; - const Http::LowerCaseString http_key_grpc_accept_encoding{"grpc-accept-encoding"}; - const std::string http_key_grpc_accept_encoding_value_ = "identity,deflate,gzip"; - }; - GrpcWebFilter(); virtual ~GrpcWebFilter(); @@ -81,6 +37,7 @@ class GrpcWebFilter : public Http::StreamFilter { } private: + static const uint8_t GRPC_WEB_TRAILER; Http::StreamDecoderFilterCallbacks* decoder_callbacks_{}; Http::StreamEncoderFilterCallbacks* encoder_callbacks_{}; bool is_text_request_; diff --git a/source/common/http/headers.h b/source/common/http/headers.h index f0d76745cb89d..24b6e20080142 100644 --- a/source/common/http/headers.h +++ b/source/common/http/headers.h @@ -43,6 +43,7 @@ class HeaderValues { const LowerCaseString ForwardedProto{"x-forwarded-proto"}; const LowerCaseString GrpcMessage{"grpc-message"}; const LowerCaseString GrpcStatus{"grpc-status"}; + const LowerCaseString GrpcAcceptEncoding{"grpc-accept-encoding"}; const LowerCaseString Host{":authority"}; const LowerCaseString HostLegacy{"host"}; const LowerCaseString KeepAlive{"keep-alive"}; @@ -56,6 +57,7 @@ class HeaderValues { const LowerCaseString Server{"server"}; const LowerCaseString Status{":status"}; const LowerCaseString TransferEncoding{"transfer-encoding"}; + const LowerCaseString TE{"te"}; const LowerCaseString Upgrade{"upgrade"}; const LowerCaseString UserAgent{"user-agent"}; @@ -65,6 +67,9 @@ class HeaderValues { struct { const std::string Text{"text/plain"}; + const std::string Grpc{"application/grpc"}; + const std::string GrpcWeb{"application/grpc-web"}; + const std::string GrpcWebText{"application/grpc-web-text"}; } ContentTypeValues; struct { @@ -100,6 +105,14 @@ class HeaderValues { struct { const std::string EnvoyHealthChecker{"Envoy/HC"}; } UserAgentValues; + + struct { + const std::string Default{"identity,deflate,gzip"}; + } GrpcAcceptEncodingValues; + + struct { + const std::string Trailers{"trailers"}; + } TEValues; }; typedef ConstSingleton Headers; From 1394d32929989c8da0e55548e17b861ccdbeef5e Mon Sep 17 00:00:00 2001 From: Feng Li Date: Wed, 17 May 2017 11:56:19 -0700 Subject: [PATCH 09/28] Revert source/common/http/conn_manager_impl.cc. --- source/common/http/conn_manager_impl.cc | 3 --- 1 file changed, 3 deletions(-) diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 4712ce9dc00da..259e18e599a0d 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -797,9 +797,6 @@ void ConnectionManagerImpl::ActiveStream::encodeTrailers(ActiveStreamEncoderFilt }, this); #endif - if (buffered_response_data_ && buffered_response_data_->length() > 0) { - response_encoder_->encodeData(*buffered_response_data_, false); - } response_encoder_->encodeTrailers(trailers); maybeEndEncode(true); } From ffee1d4b51a4b1d3609d155ff0f262f86edbea56 Mon Sep 17 00:00:00 2001 From: Feng Li Date: Wed, 17 May 2017 12:55:07 -0700 Subject: [PATCH 10/28] Change to use addDecodedData to encode trailers to response body. --- source/common/grpc/grpc_web_filter.cc | 25 +++++++++++++------------ source/common/grpc/grpc_web_filter.h | 4 ++++ source/server/config/http/grpc_web.cc | 2 ++ source/server/config/http/grpc_web.h | 2 ++ 4 files changed, 21 insertions(+), 12 deletions(-) diff --git a/source/common/grpc/grpc_web_filter.cc b/source/common/grpc/grpc_web_filter.cc index b11644154b0b5..2f62e506a51df 100644 --- a/source/common/grpc/grpc_web_filter.cc +++ b/source/common/grpc/grpc_web_filter.cc @@ -5,6 +5,7 @@ #include "common/common/base64.h" #include "common/http/headers.h" +namespace Envoy { namespace Grpc { const uint8_t GrpcWebFilter::GRPC_WEB_TRAILER = 0b10000000; @@ -86,20 +87,20 @@ Http::FilterDataStatus GrpcWebFilter::encodeData(Buffer::Instance& data, bool) { } Http::FilterTrailersStatus GrpcWebFilter::encodeTrailers(Http::HeaderMap& trailers) { - if (!encoder_callbacks_->encodingBuffer()) { - encoder_callbacks_->encodingBuffer().reset(new Buffer::OwnedImpl()); - } - encoder_callbacks_->encodingBuffer()->add(&GRPC_WEB_TRAILER, 1); + Buffer::OwnedImpl buffer; + buffer.add(&GRPC_WEB_TRAILER, 1); trailers.iterate([](const Http::HeaderEntry& header, void* context) -> void { - Buffer::Instance& temp = static_cast(context)->encoding_buffer_trailers_; - temp.add(header.key().c_str(), header.key().size()); - temp.add(":"); - temp.add(header.value().c_str(), header.value().size()); - temp.add("\r\n"); - }, this); + Buffer::Instance* buffer = static_cast(context); + buffer->add(header.key().c_str(), header.key().size()); + buffer->add(":"); + buffer->add(header.value().c_str(), header.value().size()); + buffer->add("\r\n"); + }, &buffer); uint64_t length = htonl(encoding_buffer_trailers_.length()); - encoder_callbacks_->encodingBuffer()->add(&length, 4); - encoder_callbacks_->encodingBuffer()->move(encoding_buffer_trailers_); + buffer.add(&length, 4); + buffer.move(encoding_buffer_trailers_); + encoder_callbacks_->addEncodedData(buffer); return Http::FilterTrailersStatus::Continue; } } // namespace Grpc +} // namespace Envoy diff --git a/source/common/grpc/grpc_web_filter.h b/source/common/grpc/grpc_web_filter.h index db845af77d463..8dd284a393a87 100644 --- a/source/common/grpc/grpc_web_filter.h +++ b/source/common/grpc/grpc_web_filter.h @@ -6,6 +6,7 @@ #include "common/buffer/buffer_impl.h" #include "common/grpc/codec.h" +namespace Envoy { namespace Grpc { class GrpcWebFilter : public Http::StreamFilter { @@ -17,6 +18,8 @@ class GrpcWebFilter : public Http::StreamFilter { GrpcWebFilter(const GrpcWebFilter&) = delete; GrpcWebFilter& operator=(const GrpcWebFilter&) = delete; + void onDestroy() override{}; + // Implements StreamDecoderFilter. Http::FilterHeadersStatus decodeHeaders(Http::HeaderMap&, bool) override; Http::FilterDataStatus decodeData(Buffer::Instance&, bool) override; @@ -47,5 +50,6 @@ class GrpcWebFilter : public Http::StreamFilter { Decoder decoder_; }; } // namespace Grpc +} // namespace Envoy #endif // SOURCE_COMMON_GRPC_GRPC_WEB_FILTER_H_ diff --git a/source/server/config/http/grpc_web.cc b/source/server/config/http/grpc_web.cc index ad03b2f96b52a..806c469a31bea 100644 --- a/source/server/config/http/grpc_web.cc +++ b/source/server/config/http/grpc_web.cc @@ -2,6 +2,7 @@ #include "common/grpc/grpc_web_filter.h" +namespace Envoy { namespace Server { namespace Configuration { @@ -26,3 +27,4 @@ static RegisterHttpFilterConfigFactory register_; } // namespace Configuration } // namespace Server +} // namespace Envoy diff --git a/source/server/config/http/grpc_web.h b/source/server/config/http/grpc_web.h index 6639aadc199f6..04d66a0a8d225 100644 --- a/source/server/config/http/grpc_web.h +++ b/source/server/config/http/grpc_web.h @@ -2,6 +2,7 @@ #include "server/config/network/http_connection_manager.h" +namespace Envoy { namespace Server { namespace Configuration { @@ -14,3 +15,4 @@ class GrpcWebFilterConfig : public HttpFilterConfigFactory { } // namespace Configuration } // namespace Server +} // namespace Envoy From 2d195c354c5c2b7ee22db164ef9e468127704b60 Mon Sep 17 00:00:00 2001 From: Feng Li Date: Wed, 17 May 2017 17:24:06 -0700 Subject: [PATCH 11/28] Symbolize stack trace for tests. --- tools/BUILD | 1 + tools/bazel.rc | 3 +++ tools/stack_decode.py | 4 ++-- 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/tools/BUILD b/tools/BUILD index fa3e910f5fb8a..0a1372878f61d 100644 --- a/tools/BUILD +++ b/tools/BUILD @@ -11,6 +11,7 @@ envoy_package() exports_files([ "gen_git_sha.sh", "git_sha_rewriter.py", + "stack_decode.py", ]) envoy_py_test_binary( diff --git a/tools/bazel.rc b/tools/bazel.rc index 26953d1cec166..b88680b6fab1f 100644 --- a/tools/bazel.rc +++ b/tools/bazel.rc @@ -35,3 +35,6 @@ build:clang-msan --copt -fsanitize=memory build:clang-msan --linkopt -fsanitize=memory build:clang-msan --define tcmalloc=disabled build:clang-msan --copt -fsanitize-memory-track-origins=2 + +# Symbolize stack trace for tests. +test --run_under=//tools:stack_decode.py diff --git a/tools/stack_decode.py b/tools/stack_decode.py index e95af62c14192..1cbebc7450900 100755 --- a/tools/stack_decode.py +++ b/tools/stack_decode.py @@ -100,8 +100,8 @@ def output_stacktrace(thread_id, traceinfo): if __name__ == "__main__": if len(sys.argv) > 1: rununder = subprocess.Popen( - sys.argv[1:], stdout=subprocess.PIPE, stderr=subprocess.PIPE) - decode_stacktrace_log(rununder.stderr) + sys.argv[1:], stdout=subprocess.PIPE, stderr=subprocess.STDOUT) + decode_stacktrace_log(rununder.stdout) rununder.wait() sys.exit(rununder.returncode) # Pass back test pass/fail result else: From 539dcf5c20d0d80bc6a65de40681b13797ef49b2 Mon Sep 17 00:00:00 2001 From: Feng Li Date: Wed, 17 May 2017 17:30:12 -0700 Subject: [PATCH 12/28] Add unit tests for grpc-web filter. --- test/common/grpc/BUILD | 11 +++ test/common/grpc/grpc_web_filter_test.cc | 119 +++++++++++++++++++++++ 2 files changed, 130 insertions(+) create mode 100644 test/common/grpc/grpc_web_filter_test.cc diff --git a/test/common/grpc/BUILD b/test/common/grpc/BUILD index 37b39fb54d7a7..fcd6116a58a82 100644 --- a/test/common/grpc/BUILD +++ b/test/common/grpc/BUILD @@ -42,6 +42,17 @@ envoy_cc_test( ], ) +envoy_cc_test( + name = "grpc_web_filter_test", + srcs = ["grpc_web_filter_test.cc"], + deps = [ + "//source/common/grpc:grpc_web_filter_lib", + "//test/mocks/http:http_mocks", + "//test/mocks/upstream:upstream_mocks", + "//test/test_common:utility_lib", + ], +) + envoy_cc_test( name = "rpc_channel_impl_test", srcs = ["rpc_channel_impl_test.cc"], diff --git a/test/common/grpc/grpc_web_filter_test.cc b/test/common/grpc/grpc_web_filter_test.cc new file mode 100644 index 0000000000000..816cef41d0ec5 --- /dev/null +++ b/test/common/grpc/grpc_web_filter_test.cc @@ -0,0 +1,119 @@ +#include "common/buffer/buffer_impl.h" +#include "common/common/base64.h" +#include "common/grpc/grpc_web_filter.h" +#include "common/http/header_map_impl.h" +#include "common/http/headers.h" + +#include "test/mocks/http/mocks.h" +#include "test/mocks/upstream/mocks.h" +#include "test/test_common/printers.h" +#include "test/test_common/utility.h" + +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +using testing::_; +using testing::NiceMock; +using testing::Return; +using testing::ReturnPointee; +using testing::ReturnRef; + +namespace Envoy { +namespace Grpc { + +class GrpcWebFilterTest : public testing::Test { +public: + GrpcWebFilterTest() : filter_() { filter_.setEncoderFilterCallbacks(encoder_callbacks_); } + + ~GrpcWebFilterTest() override {} + + GrpcWebFilter filter_; + NiceMock encoder_callbacks_; +}; + +TEST_F(GrpcWebFilterTest, BinaryUnary) { + // Tests request headers. + Http::TestHeaderMapImpl request_headers; + request_headers.addViaCopy(Http::Headers::get().ContentType, + Http::Headers::get().ContentTypeValues.GrpcWeb); + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.decodeHeaders(request_headers, false)); + EXPECT_EQ(Http::Headers::get().ContentTypeValues.Grpc, + request_headers.ContentType()->value().c_str()); + + // Tests request data. + Buffer::OwnedImpl request_buffer("grpc-web-bin-data"); + EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.decodeData(request_buffer, true)); + EXPECT_EQ("grpc-web-bin-data", TestUtility::bufferToString(request_buffer)); + + // Tests response headers. + Http::TestHeaderMapImpl response_headers; + response_headers.addViaCopy(Http::Headers::get().Status, "200"); + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.encodeHeaders(response_headers, false)); + EXPECT_EQ("200", response_headers.get_(Http::Headers::get().Status.get())); + EXPECT_EQ(Http::Headers::get().ContentTypeValues.GrpcWeb, + response_headers.ContentType()->value().c_str()); + + // Tests response data. + Buffer::OwnedImpl response_buffer("grpc-web-bin-data"); + EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.encodeData(response_buffer, false)); + EXPECT_EQ("grpc-web-bin-data", TestUtility::bufferToString(response_buffer)); + response_buffer.drain(response_buffer.length()); + + // Tests response trailers. + Http::TestHeaderMapImpl response_trailers; + response_trailers.addViaCopy(Http::Headers::get().GrpcStatus, "0"); + EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_.encodeTrailers(response_trailers)); + EXPECT_EQ( + "\x80\0x00\0x00\0x00\0x23grpc-status:0\r\n", + std::string(reinterpret_cast(response_buffer.linearize(response_buffer.length())), + response_buffer.length())); +} + +TEST_F(GrpcWebFilterTest, TextUnary) { + // Tests request headers. + Http::TestHeaderMapImpl request_headers; + request_headers.addViaCopy(Http::Headers::get().ContentType, + Http::Headers::get().ContentTypeValues.GrpcWebText); + request_headers.addViaCopy(Http::Headers::get().Accept, + Http::Headers::get().ContentTypeValues.GrpcWebText); + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.decodeHeaders(request_headers, false)); + EXPECT_EQ(Http::Headers::get().ContentTypeValues.Grpc, + request_headers.ContentType()->value().c_str()); + + // Tests request data. + std::string data("\x00\x00\x00\x00\x12grpc-web-text-data", 23); + std::string b64_data("AAAAABJncnBjLXdlYi10ZXh0LWRhdGE="); + Buffer::OwnedImpl request_buffer(b64_data); + EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.decodeData(request_buffer, true)); + EXPECT_EQ(data, TestUtility::bufferToString(request_buffer)); + + // Tests response headers. + Http::TestHeaderMapImpl response_headers; + response_headers.addViaCopy(Http::Headers::get().Status, "200"); + response_headers.addViaCopy(Http::Headers::get().ContentType, + Http::Headers::get().ContentTypeValues.Grpc); + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.encodeHeaders(response_headers, false)); + EXPECT_EQ("200", response_headers.get_(Http::Headers::get().Status.get())); + EXPECT_EQ(Http::Headers::get().ContentTypeValues.GrpcWebText, + response_headers.ContentType()->value().c_str()); + + // Tests response data. + Buffer::OwnedImpl response_buffer(data); + EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.encodeData(response_buffer, false)); + EXPECT_EQ(b64_data, TestUtility::bufferToString(response_buffer)); + response_buffer.drain(response_buffer.length()); + + // Tests response trailers. + Http::TestHeaderMapImpl response_trailers; + response_trailers.addViaCopy(Http::Headers::get().GrpcStatus, "0"); + EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_.encodeTrailers(response_trailers)); + EXPECT_EQ( + "\x80\0x00\0x00\0x00\0x23grpc-status:0\r\n", + std::string(reinterpret_cast(response_buffer.linearize(response_buffer.length())), + response_buffer.length())); +} + +TEST_F(GrpcWebFilterTest, BinaryBidi) {} + +} // namespace Grpc +} // namespace Envoy From 6bf1e12116d91dcd6ab9d5181bc827dadfdc42da Mon Sep 17 00:00:00 2001 From: Feng Li Date: Wed, 17 May 2017 18:10:40 -0700 Subject: [PATCH 13/28] Add unit tests for grpc-web filter. --- source/common/grpc/grpc_web_filter.cc | 10 ++++++---- test/common/grpc/grpc_web_filter_test.cc | 10 ++++------ 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/source/common/grpc/grpc_web_filter.cc b/source/common/grpc/grpc_web_filter.cc index 2f62e506a51df..258269934d069 100644 --- a/source/common/grpc/grpc_web_filter.cc +++ b/source/common/grpc/grpc_web_filter.cc @@ -60,10 +60,11 @@ Http::FilterDataStatus GrpcWebFilter::decodeData(Buffer::Instance& data, bool) { // Implements StreamEncoderFilter. Http::FilterHeadersStatus GrpcWebFilter::encodeHeaders(Http::HeaderMap& headers, bool) { + headers.remove(Http::Headers::get().ContentType); if (is_text_response_) { - headers.ContentType()->value(Http::Headers::get().ContentTypeValues.GrpcWebText); + headers.addStatic(Http::Headers::get().ContentType, Http::Headers::get().ContentTypeValues.GrpcWebText); } else { - headers.ContentType()->value(Http::Headers::get().ContentTypeValues.GrpcWeb); + headers.addStatic(Http::Headers::get().ContentType, Http::Headers::get().ContentTypeValues.GrpcWeb); } return Http::FilterHeadersStatus::Continue; } @@ -79,9 +80,10 @@ Http::FilterDataStatus GrpcWebFilter::encodeData(Buffer::Instance& data, bool) { for (auto& frame : frames) { Buffer::OwnedImpl temp; temp.add(&frame.flags_, 1); - temp.add(&frame.length_, 4); + uint32_t length = htonl(frame.length_); + temp.add(&length, 4); temp.add(*frame.data_); - data.add(temp); + data.add(Base64::encode(temp, temp.length())); } return Http::FilterDataStatus::Continue; } diff --git a/test/common/grpc/grpc_web_filter_test.cc b/test/common/grpc/grpc_web_filter_test.cc index 816cef41d0ec5..59df3c5ae41ca 100644 --- a/test/common/grpc/grpc_web_filter_test.cc +++ b/test/common/grpc/grpc_web_filter_test.cc @@ -63,9 +63,8 @@ TEST_F(GrpcWebFilterTest, BinaryUnary) { Http::TestHeaderMapImpl response_trailers; response_trailers.addViaCopy(Http::Headers::get().GrpcStatus, "0"); EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_.encodeTrailers(response_trailers)); - EXPECT_EQ( - "\x80\0x00\0x00\0x00\0x23grpc-status:0\r\n", - std::string(reinterpret_cast(response_buffer.linearize(response_buffer.length())), + EXPECT_EQ(0, strncmp("\x80\0x00\0x00\0x00\0x23grpc-status:0\r\n", + reinterpret_cast(response_buffer.linearize(response_buffer.length())), response_buffer.length())); } @@ -107,9 +106,8 @@ TEST_F(GrpcWebFilterTest, TextUnary) { Http::TestHeaderMapImpl response_trailers; response_trailers.addViaCopy(Http::Headers::get().GrpcStatus, "0"); EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_.encodeTrailers(response_trailers)); - EXPECT_EQ( - "\x80\0x00\0x00\0x00\0x23grpc-status:0\r\n", - std::string(reinterpret_cast(response_buffer.linearize(response_buffer.length())), + EXPECT_EQ(0, strncmp("\x80\0x00\0x00\0x00\0x23grpc-status:0\r\n", + reinterpret_cast(response_buffer.linearize(response_buffer.length())), response_buffer.length())); } From 25b9c37fca422a9baba33dca9758d13063e4b5a5 Mon Sep 17 00:00:00 2001 From: Feng Li Date: Thu, 18 May 2017 14:02:09 -0700 Subject: [PATCH 14/28] fix format. --- source/common/grpc/grpc_web_filter.cc | 10 +++++++--- test/common/grpc/grpc_web_filter_test.cc | 8 ++++---- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/source/common/grpc/grpc_web_filter.cc b/source/common/grpc/grpc_web_filter.cc index 258269934d069..221854f23c039 100644 --- a/source/common/grpc/grpc_web_filter.cc +++ b/source/common/grpc/grpc_web_filter.cc @@ -62,9 +62,11 @@ Http::FilterDataStatus GrpcWebFilter::decodeData(Buffer::Instance& data, bool) { Http::FilterHeadersStatus GrpcWebFilter::encodeHeaders(Http::HeaderMap& headers, bool) { headers.remove(Http::Headers::get().ContentType); if (is_text_response_) { - headers.addStatic(Http::Headers::get().ContentType, Http::Headers::get().ContentTypeValues.GrpcWebText); + headers.addStatic(Http::Headers::get().ContentType, + Http::Headers::get().ContentTypeValues.GrpcWebText); } else { - headers.addStatic(Http::Headers::get().ContentType, Http::Headers::get().ContentTypeValues.GrpcWeb); + headers.addStatic(Http::Headers::get().ContentType, + Http::Headers::get().ContentTypeValues.GrpcWeb); } return Http::FilterHeadersStatus::Continue; } @@ -82,7 +84,9 @@ Http::FilterDataStatus GrpcWebFilter::encodeData(Buffer::Instance& data, bool) { temp.add(&frame.flags_, 1); uint32_t length = htonl(frame.length_); temp.add(&length, 4); - temp.add(*frame.data_); + if (frame.length_ > 0) { + temp.add(*frame.data_); + } data.add(Base64::encode(temp, temp.length())); } return Http::FilterDataStatus::Continue; diff --git a/test/common/grpc/grpc_web_filter_test.cc b/test/common/grpc/grpc_web_filter_test.cc index 59df3c5ae41ca..28fe021249d78 100644 --- a/test/common/grpc/grpc_web_filter_test.cc +++ b/test/common/grpc/grpc_web_filter_test.cc @@ -64,8 +64,8 @@ TEST_F(GrpcWebFilterTest, BinaryUnary) { response_trailers.addViaCopy(Http::Headers::get().GrpcStatus, "0"); EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_.encodeTrailers(response_trailers)); EXPECT_EQ(0, strncmp("\x80\0x00\0x00\0x00\0x23grpc-status:0\r\n", - reinterpret_cast(response_buffer.linearize(response_buffer.length())), - response_buffer.length())); + reinterpret_cast(response_buffer.linearize(response_buffer.length())), + response_buffer.length())); } TEST_F(GrpcWebFilterTest, TextUnary) { @@ -107,8 +107,8 @@ TEST_F(GrpcWebFilterTest, TextUnary) { response_trailers.addViaCopy(Http::Headers::get().GrpcStatus, "0"); EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_.encodeTrailers(response_trailers)); EXPECT_EQ(0, strncmp("\x80\0x00\0x00\0x00\0x23grpc-status:0\r\n", - reinterpret_cast(response_buffer.linearize(response_buffer.length())), - response_buffer.length())); + reinterpret_cast(response_buffer.linearize(response_buffer.length())), + response_buffer.length())); } TEST_F(GrpcWebFilterTest, BinaryBidi) {} From f6bca5984be4344c0baf8f86e46eb1d70f4061c7 Mon Sep 17 00:00:00 2001 From: Feng Li Date: Thu, 18 May 2017 14:28:13 -0700 Subject: [PATCH 15/28] Update unit tests. --- test/common/grpc/grpc_web_filter_test.cc | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/test/common/grpc/grpc_web_filter_test.cc b/test/common/grpc/grpc_web_filter_test.cc index 28fe021249d78..6c6fb1d92caa3 100644 --- a/test/common/grpc/grpc_web_filter_test.cc +++ b/test/common/grpc/grpc_web_filter_test.cc @@ -41,9 +41,10 @@ TEST_F(GrpcWebFilterTest, BinaryUnary) { request_headers.ContentType()->value().c_str()); // Tests request data. - Buffer::OwnedImpl request_buffer("grpc-web-bin-data"); + Buffer::OwnedImpl request_buffer("\x00\x00\x00\x00\x11grpc-web-bin-data"); EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.decodeData(request_buffer, true)); - EXPECT_EQ("grpc-web-bin-data", TestUtility::bufferToString(request_buffer)); + EXPECT_EQ(0, strncmp("\x00\x00\x00\x00\x11grpc-web-bin-data", + TestUtility::bufferToString(request_buffer).c_str(), 22)); // Tests response headers. Http::TestHeaderMapImpl response_headers; @@ -54,9 +55,9 @@ TEST_F(GrpcWebFilterTest, BinaryUnary) { response_headers.ContentType()->value().c_str()); // Tests response data. - Buffer::OwnedImpl response_buffer("grpc-web-bin-data"); + Buffer::OwnedImpl response_buffer("\x00\x00\x00\x00\x11grpc-web-bin-data"); EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.encodeData(response_buffer, false)); - EXPECT_EQ("grpc-web-bin-data", TestUtility::bufferToString(response_buffer)); + EXPECT_EQ("\x00\x00\x00\x00\x11grpc-web-bin-data", TestUtility::bufferToString(response_buffer)); response_buffer.drain(response_buffer.length()); // Tests response trailers. @@ -110,8 +111,5 @@ TEST_F(GrpcWebFilterTest, TextUnary) { reinterpret_cast(response_buffer.linearize(response_buffer.length())), response_buffer.length())); } - -TEST_F(GrpcWebFilterTest, BinaryBidi) {} - } // namespace Grpc } // namespace Envoy From 475a35cbbd87a39cf9fa621cfc9297f63f439b90 Mon Sep 17 00:00:00 2001 From: Feng Li Date: Thu, 18 May 2017 18:03:30 -0700 Subject: [PATCH 16/28] Update unit tests. --- source/common/grpc/grpc_web_filter.cc | 19 ++++----- source/common/grpc/grpc_web_filter.h | 1 - test/common/grpc/grpc_web_filter_test.cc | 51 ++++++++++++++---------- 3 files changed, 41 insertions(+), 30 deletions(-) diff --git a/source/common/grpc/grpc_web_filter.cc b/source/common/grpc/grpc_web_filter.cc index 221854f23c039..59320ce5c7bc1 100644 --- a/source/common/grpc/grpc_web_filter.cc +++ b/source/common/grpc/grpc_web_filter.cc @@ -93,18 +93,19 @@ Http::FilterDataStatus GrpcWebFilter::encodeData(Buffer::Instance& data, bool) { } Http::FilterTrailersStatus GrpcWebFilter::encodeTrailers(Http::HeaderMap& trailers) { + Buffer::OwnedImpl temp; + trailers.iterate([](const Http::HeaderEntry& header, void* context) -> void { + Buffer::Instance* temp = static_cast(context); + temp->add(header.key().c_str(), header.key().size()); + temp->add(":"); + temp->add(header.value().c_str(), header.value().size()); + temp->add("\r\n"); + }, &temp); Buffer::OwnedImpl buffer; buffer.add(&GRPC_WEB_TRAILER, 1); - trailers.iterate([](const Http::HeaderEntry& header, void* context) -> void { - Buffer::Instance* buffer = static_cast(context); - buffer->add(header.key().c_str(), header.key().size()); - buffer->add(":"); - buffer->add(header.value().c_str(), header.value().size()); - buffer->add("\r\n"); - }, &buffer); - uint64_t length = htonl(encoding_buffer_trailers_.length()); + uint64_t length = htonl(temp.length()); buffer.add(&length, 4); - buffer.move(encoding_buffer_trailers_); + buffer.move(temp); encoder_callbacks_->addEncodedData(buffer); return Http::FilterTrailersStatus::Continue; } diff --git a/source/common/grpc/grpc_web_filter.h b/source/common/grpc/grpc_web_filter.h index 8dd284a393a87..6bc03fc3fa25f 100644 --- a/source/common/grpc/grpc_web_filter.h +++ b/source/common/grpc/grpc_web_filter.h @@ -46,7 +46,6 @@ class GrpcWebFilter : public Http::StreamFilter { bool is_text_request_; bool is_text_response_; Buffer::OwnedImpl decoding_buffer_; - Buffer::OwnedImpl encoding_buffer_trailers_; Decoder decoder_; }; } // namespace Grpc diff --git a/test/common/grpc/grpc_web_filter_test.cc b/test/common/grpc/grpc_web_filter_test.cc index 6c6fb1d92caa3..760ded08d099e 100644 --- a/test/common/grpc/grpc_web_filter_test.cc +++ b/test/common/grpc/grpc_web_filter_test.cc @@ -14,12 +14,20 @@ using testing::_; using testing::NiceMock; -using testing::Return; -using testing::ReturnPointee; -using testing::ReturnRef; +using testing::Invoke; namespace Envoy { namespace Grpc { +namespace { +const char MESSAGE[] = "\x00\x00\x00\x00\x11grpc-web-bin-data"; +const size_t MESSAGE_SIZE = 22; +const char TEXT_MESSAGE[] = "\x00\x00\x00\x00\x12grpc-web-text-data"; +const size_t TEXT_MESSAGE_SIZE = 23; +const char B64_MESSAGE[] = "AAAAABJncnBjLXdlYi10ZXh0LWRhdGE="; +const size_t B64_MESSAGE_SIZE = 32; +const char TRAILERS[] = "\x80\x00\x00\x00\x0fgrpc-status:0\r\n"; +const size_t TRAILERS_SIZE = 20; +} // namespace class GrpcWebFilterTest : public testing::Test { public: @@ -41,10 +49,9 @@ TEST_F(GrpcWebFilterTest, BinaryUnary) { request_headers.ContentType()->value().c_str()); // Tests request data. - Buffer::OwnedImpl request_buffer("\x00\x00\x00\x00\x11grpc-web-bin-data"); + Buffer::OwnedImpl request_buffer(MESSAGE, MESSAGE_SIZE); EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.decodeData(request_buffer, true)); - EXPECT_EQ(0, strncmp("\x00\x00\x00\x00\x11grpc-web-bin-data", - TestUtility::bufferToString(request_buffer).c_str(), 22)); + EXPECT_EQ(0, memcmp(MESSAGE, TestUtility::bufferToString(request_buffer).c_str(), MESSAGE_SIZE)); // Tests response headers. Http::TestHeaderMapImpl response_headers; @@ -55,18 +62,20 @@ TEST_F(GrpcWebFilterTest, BinaryUnary) { response_headers.ContentType()->value().c_str()); // Tests response data. - Buffer::OwnedImpl response_buffer("\x00\x00\x00\x00\x11grpc-web-bin-data"); + Buffer::OwnedImpl response_buffer(MESSAGE, MESSAGE_SIZE); EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.encodeData(response_buffer, false)); - EXPECT_EQ("\x00\x00\x00\x00\x11grpc-web-bin-data", TestUtility::bufferToString(response_buffer)); + EXPECT_EQ(0, memcmp(MESSAGE, TestUtility::bufferToString(response_buffer).c_str(), MESSAGE_SIZE)); response_buffer.drain(response_buffer.length()); // Tests response trailers. + Buffer::OwnedImpl trailers_buffer; + EXPECT_CALL(encoder_callbacks_, addEncodedData(_)) + .WillOnce(Invoke([&](Buffer::Instance& data) { trailers_buffer.move(data); })); Http::TestHeaderMapImpl response_trailers; response_trailers.addViaCopy(Http::Headers::get().GrpcStatus, "0"); EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_.encodeTrailers(response_trailers)); - EXPECT_EQ(0, strncmp("\x80\0x00\0x00\0x00\0x23grpc-status:0\r\n", - reinterpret_cast(response_buffer.linearize(response_buffer.length())), - response_buffer.length())); + EXPECT_EQ(0, + memcmp(TRAILERS, TestUtility::bufferToString(trailers_buffer).c_str(), TRAILERS_SIZE)); } TEST_F(GrpcWebFilterTest, TextUnary) { @@ -81,11 +90,10 @@ TEST_F(GrpcWebFilterTest, TextUnary) { request_headers.ContentType()->value().c_str()); // Tests request data. - std::string data("\x00\x00\x00\x00\x12grpc-web-text-data", 23); - std::string b64_data("AAAAABJncnBjLXdlYi10ZXh0LWRhdGE="); - Buffer::OwnedImpl request_buffer(b64_data); + Buffer::OwnedImpl request_buffer(B64_MESSAGE, B64_MESSAGE_SIZE); EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.decodeData(request_buffer, true)); - EXPECT_EQ(data, TestUtility::bufferToString(request_buffer)); + EXPECT_EQ(0, memcmp(TEXT_MESSAGE, TestUtility::bufferToString(request_buffer).c_str(), + TEXT_MESSAGE_SIZE)); // Tests response headers. Http::TestHeaderMapImpl response_headers; @@ -98,18 +106,21 @@ TEST_F(GrpcWebFilterTest, TextUnary) { response_headers.ContentType()->value().c_str()); // Tests response data. - Buffer::OwnedImpl response_buffer(data); + Buffer::OwnedImpl response_buffer(TEXT_MESSAGE, TEXT_MESSAGE_SIZE); EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.encodeData(response_buffer, false)); - EXPECT_EQ(b64_data, TestUtility::bufferToString(response_buffer)); + EXPECT_EQ(0, memcmp(B64_MESSAGE, TestUtility::bufferToString(response_buffer).c_str(), + B64_MESSAGE_SIZE)); response_buffer.drain(response_buffer.length()); // Tests response trailers. + Buffer::OwnedImpl trailers_buffer; + EXPECT_CALL(encoder_callbacks_, addEncodedData(_)) + .WillOnce(Invoke([&](Buffer::Instance& data) { trailers_buffer.move(data); })); Http::TestHeaderMapImpl response_trailers; response_trailers.addViaCopy(Http::Headers::get().GrpcStatus, "0"); EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_.encodeTrailers(response_trailers)); - EXPECT_EQ(0, strncmp("\x80\0x00\0x00\0x00\0x23grpc-status:0\r\n", - reinterpret_cast(response_buffer.linearize(response_buffer.length())), - response_buffer.length())); + EXPECT_EQ(0, + memcmp(TRAILERS, TestUtility::bufferToString(trailers_buffer).c_str(), TRAILERS_SIZE)); } } // namespace Grpc } // namespace Envoy From 8e848f61ad900d5154fc10cef244968f3e66555d Mon Sep 17 00:00:00 2001 From: Feng Li Date: Thu, 18 May 2017 18:45:58 -0700 Subject: [PATCH 17/28] Address review comments. --- source/common/grpc/grpc_web_filter.cc | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/source/common/grpc/grpc_web_filter.cc b/source/common/grpc/grpc_web_filter.cc index 59320ce5c7bc1..f4529a2dbedf8 100644 --- a/source/common/grpc/grpc_web_filter.cc +++ b/source/common/grpc/grpc_web_filter.cc @@ -21,10 +21,9 @@ Http::FilterHeadersStatus GrpcWebFilter::decodeHeaders(Http::HeaderMap& headers, Http::Headers::get().ContentTypeValues.GrpcWebText == content_type->value().c_str()) { is_text_request_ = true; } - headers.removeContentType(); headers.insertContentType().value(Http::Headers::get().ContentTypeValues.Grpc); - const Http::HeaderEntry* accept = headers.get(Http::LowerCaseString("accept")); + const Http::HeaderEntry* accept = headers.get(Http::Headers::get().Accept); if (accept != nullptr && Http::Headers::get().ContentTypeValues.GrpcWebText == accept->value().c_str()) { is_text_response_ = true; @@ -43,7 +42,6 @@ Http::FilterDataStatus GrpcWebFilter::decodeData(Buffer::Instance& data, bool) { // Parse application/grpc-web-text format. if (data.length() + decoding_buffer_.length() < 4) { decoding_buffer_.move(data); - data.drain(data.length()); return Http::FilterDataStatus::Continue; } @@ -60,13 +58,10 @@ Http::FilterDataStatus GrpcWebFilter::decodeData(Buffer::Instance& data, bool) { // Implements StreamEncoderFilter. Http::FilterHeadersStatus GrpcWebFilter::encodeHeaders(Http::HeaderMap& headers, bool) { - headers.remove(Http::Headers::get().ContentType); if (is_text_response_) { - headers.addStatic(Http::Headers::get().ContentType, - Http::Headers::get().ContentTypeValues.GrpcWebText); + headers.insertContentType().value(Http::Headers::get().ContentTypeValues.GrpcWebText); } else { - headers.addStatic(Http::Headers::get().ContentType, - Http::Headers::get().ContentTypeValues.GrpcWeb); + headers.insertContentType().value(Http::Headers::get().ContentTypeValues.GrpcWeb); } return Http::FilterHeadersStatus::Continue; } From 4950429f622619e2e92f80f9f66b79eb30652b96 Mon Sep 17 00:00:00 2001 From: Feng Li Date: Fri, 19 May 2017 09:46:57 -0700 Subject: [PATCH 18/28] Address review comments. --- .../http_filters/grpc_web_filter.rst | 15 +++++++++++++++ source/common/grpc/grpc_web_filter.cc | 9 +++++---- source/common/grpc/grpc_web_filter.h | 15 ++++++--------- 3 files changed, 26 insertions(+), 13 deletions(-) create mode 100644 docs/configuration/http_filters/grpc_web_filter.rst diff --git a/docs/configuration/http_filters/grpc_web_filter.rst b/docs/configuration/http_filters/grpc_web_filter.rst new file mode 100644 index 0000000000000..4392da1e93160 --- /dev/null +++ b/docs/configuration/http_filters/grpc_web_filter.rst @@ -0,0 +1,15 @@ +.. _config_http_filters_grpc_web: + +gRPC-Web filter +==================== + +This is a filter which enables the bridging of a gRPC-Web client to a compliant gRPC server by +following https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-WEB.md. + +.. code-block:: json + + { + "type": "both", + "name": "grpc_web", + "config": {} + } diff --git a/source/common/grpc/grpc_web_filter.cc b/source/common/grpc/grpc_web_filter.cc index f4529a2dbedf8..f59599c89b91a 100644 --- a/source/common/grpc/grpc_web_filter.cc +++ b/source/common/grpc/grpc_web_filter.cc @@ -45,9 +45,10 @@ Http::FilterDataStatus GrpcWebFilter::decodeData(Buffer::Instance& data, bool) { return Http::FilterDataStatus::Continue; } - uint64_t needed = (data.length() + decoding_buffer_.length()) / 4 * 4 - decoding_buffer_.length(); + const uint64_t needed = + (data.length() + decoding_buffer_.length()) / 4 * 4 - decoding_buffer_.length(); decoding_buffer_.move(data, needed); - std::string decoded = Base64::decode( + const std::string decoded = Base64::decode( std::string(static_cast(decoding_buffer_.linearize(decoding_buffer_.length())), decoding_buffer_.length())); decoding_buffer_.drain(decoding_buffer_.length()); @@ -77,7 +78,7 @@ Http::FilterDataStatus GrpcWebFilter::encodeData(Buffer::Instance& data, bool) { for (auto& frame : frames) { Buffer::OwnedImpl temp; temp.add(&frame.flags_, 1); - uint32_t length = htonl(frame.length_); + const uint32_t length = htonl(frame.length_); temp.add(&length, 4); if (frame.length_ > 0) { temp.add(*frame.data_); @@ -98,7 +99,7 @@ Http::FilterTrailersStatus GrpcWebFilter::encodeTrailers(Http::HeaderMap& traile }, &temp); Buffer::OwnedImpl buffer; buffer.add(&GRPC_WEB_TRAILER, 1); - uint64_t length = htonl(temp.length()); + const uint32_t length = htonl(temp.length()); buffer.add(&length, 4); buffer.move(temp); encoder_callbacks_->addEncodedData(buffer); diff --git a/source/common/grpc/grpc_web_filter.h b/source/common/grpc/grpc_web_filter.h index 6bc03fc3fa25f..87b8d78dd8658 100644 --- a/source/common/grpc/grpc_web_filter.h +++ b/source/common/grpc/grpc_web_filter.h @@ -1,23 +1,22 @@ -#ifndef SOURCE_COMMON_GRPC_GRPC_WEB_FILTER_H_ -#define SOURCE_COMMON_GRPC_GRPC_WEB_FILTER_H_ +#pragma once #include "envoy/http/filter.h" #include "common/buffer/buffer_impl.h" +#include "common/common/non_copyable.h" #include "common/grpc/codec.h" namespace Envoy { namespace Grpc { -class GrpcWebFilter : public Http::StreamFilter { +/** + * See docs/configuration/http_filters/grpc_web_filter.rst + */ +class GrpcWebFilter : public Http::StreamFilter, NonCopyable { public: GrpcWebFilter(); virtual ~GrpcWebFilter(); - // GrpcWebFilter is neither copyable nor movable. - GrpcWebFilter(const GrpcWebFilter&) = delete; - GrpcWebFilter& operator=(const GrpcWebFilter&) = delete; - void onDestroy() override{}; // Implements StreamDecoderFilter. @@ -50,5 +49,3 @@ class GrpcWebFilter : public Http::StreamFilter { }; } // namespace Grpc } // namespace Envoy - -#endif // SOURCE_COMMON_GRPC_GRPC_WEB_FILTER_H_ From 92b511da311678d69f090416bdd64bce05b67eed Mon Sep 17 00:00:00 2001 From: Feng Li Date: Fri, 19 May 2017 10:32:02 -0700 Subject: [PATCH 19/28] Add document for grpc-web. --- docs/configuration/http_filters/grpc_web_filter.rst | 2 ++ docs/configuration/http_filters/http_filters.rst | 1 + 2 files changed, 3 insertions(+) diff --git a/docs/configuration/http_filters/grpc_web_filter.rst b/docs/configuration/http_filters/grpc_web_filter.rst index 4392da1e93160..aa871344b191f 100644 --- a/docs/configuration/http_filters/grpc_web_filter.rst +++ b/docs/configuration/http_filters/grpc_web_filter.rst @@ -3,6 +3,8 @@ gRPC-Web filter ==================== +gRPC :ref:`architecture overview `. + This is a filter which enables the bridging of a gRPC-Web client to a compliant gRPC server by following https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-WEB.md. diff --git a/docs/configuration/http_filters/http_filters.rst b/docs/configuration/http_filters/http_filters.rst index 2e579eb503fc1..7efb0bfc73aa8 100644 --- a/docs/configuration/http_filters/http_filters.rst +++ b/docs/configuration/http_filters/http_filters.rst @@ -10,6 +10,7 @@ HTTP filters fault_filter dynamodb_filter grpc_http1_bridge_filter + grpc_web_filter health_check_filter rate_limit_filter router_filter From 69b34b0c247bc4ef75d0abdf37988b7ad9d8eb52 Mon Sep 17 00:00:00 2001 From: Feng Li Date: Fri, 19 May 2017 15:29:13 -0700 Subject: [PATCH 20/28] Add gRPC-Web in Envoy intro document. --- docs/intro/arch_overview/grpc.rst | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/intro/arch_overview/grpc.rst b/docs/intro/arch_overview/grpc.rst index 386ab8d13ecbe..3e41fbd69c1f1 100644 --- a/docs/intro/arch_overview/grpc.rst +++ b/docs/intro/arch_overview/grpc.rst @@ -17,3 +17,7 @@ application layer: The response is translated back to HTTP/1.1. * When installed, the bridge filter gathers per RPC statistics in addition to the standard array of global HTTP statistics. +* gRPC-Web is supported by a :ref:`filter ` that allows a gRPC-Web + client sends requests to Envoy over HTTP/1.1 and gets proxied to a gRPC server. It's under + active development and is expected to be the successor of the gRPC :ref:`bridge filter + `. From 02fb75a029ac06d007953b10d570fa637a5ec52e Mon Sep 17 00:00:00 2001 From: Matt Klein Date: Fri, 19 May 2017 16:13:59 -0700 Subject: [PATCH 21/28] Update grpc.rst --- docs/intro/arch_overview/grpc.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/intro/arch_overview/grpc.rst b/docs/intro/arch_overview/grpc.rst index 3e41fbd69c1f1..4ef1dc600a474 100644 --- a/docs/intro/arch_overview/grpc.rst +++ b/docs/intro/arch_overview/grpc.rst @@ -18,6 +18,6 @@ application layer: * When installed, the bridge filter gathers per RPC statistics in addition to the standard array of global HTTP statistics. * gRPC-Web is supported by a :ref:`filter ` that allows a gRPC-Web - client sends requests to Envoy over HTTP/1.1 and gets proxied to a gRPC server. It's under - active development and is expected to be the successor of the gRPC :ref:`bridge filter + client to send requests to Envoy over HTTP/1.1 and get proxied to a gRPC server. It's under + active development and is expected to be the successor to the gRPC :ref:`bridge filter `. From 7251c10790da8cf50e494bce5365ddd2e5f60c30 Mon Sep 17 00:00:00 2001 From: Feng Li Date: Mon, 22 May 2017 14:34:40 -0700 Subject: [PATCH 22/28] Address comments. --- source/common/grpc/grpc_web_filter.cc | 24 ++++++++++++++++++++---- source/common/grpc/grpc_web_filter.h | 1 + 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/source/common/grpc/grpc_web_filter.cc b/source/common/grpc/grpc_web_filter.cc index f59599c89b91a..fed35a5a105bc 100644 --- a/source/common/grpc/grpc_web_filter.cc +++ b/source/common/grpc/grpc_web_filter.cc @@ -19,6 +19,7 @@ Http::FilterHeadersStatus GrpcWebFilter::decodeHeaders(Http::HeaderMap& headers, const Http::HeaderEntry* content_type = headers.ContentType(); if (content_type != nullptr && Http::Headers::get().ContentTypeValues.GrpcWebText == content_type->value().c_str()) { + // Checks whether gRPC-Web client is sending b64 ncoded request. is_text_request_ = true; } headers.insertContentType().value(Http::Headers::get().ContentTypeValues.Grpc); @@ -26,9 +27,13 @@ Http::FilterHeadersStatus GrpcWebFilter::decodeHeaders(Http::HeaderMap& headers, const Http::HeaderEntry* accept = headers.get(Http::Headers::get().Accept); if (accept != nullptr && Http::Headers::get().ContentTypeValues.GrpcWebText == accept->value().c_str()) { + // Checks whether gRPC-Web client is asking for b64 encoded response. is_text_response_ = true; } + + // Adds te:trailers to upstream HTTP2 request. It's required for gRPC. headers.addStatic(Http::Headers::get().TE, Http::Headers::get().TEValues.Trailers); + // Adds grpc-accept-encoding:identity,deflate,gzip. It's required for gRPC. headers.addStatic(Http::Headers::get().GrpcAcceptEncoding, Http::Headers::get().GrpcAcceptEncodingValues.Default); return Http::FilterHeadersStatus::Continue; @@ -36,6 +41,7 @@ Http::FilterHeadersStatus GrpcWebFilter::decodeHeaders(Http::HeaderMap& headers, Http::FilterDataStatus GrpcWebFilter::decodeData(Buffer::Instance& data, bool) { if (!is_text_request_) { + // No additional transcoding required if gRPC client is sending binary request. return Http::FilterDataStatus::Continue; } @@ -45,8 +51,7 @@ Http::FilterDataStatus GrpcWebFilter::decodeData(Buffer::Instance& data, bool) { return Http::FilterDataStatus::Continue; } - const uint64_t needed = - (data.length() + decoding_buffer_.length()) / 4 * 4 - decoding_buffer_.length(); + const uint64_t needed = (data.length() + decoding_buffer_.length()) / 4 * 4 - decoding_buffer_.length(); decoding_buffer_.move(data, needed); const std::string decoded = Base64::decode( std::string(static_cast(decoding_buffer_.linearize(decoding_buffer_.length())), @@ -69,18 +74,25 @@ Http::FilterHeadersStatus GrpcWebFilter::encodeHeaders(Http::HeaderMap& headers, Http::FilterDataStatus GrpcWebFilter::encodeData(Buffer::Instance& data, bool) { if (!is_text_response_) { + // No additional transcoding required if gRPC-Web client asked for binary response. return Http::FilterDataStatus::Continue; } - // Encodes the response as base64. + // The decoder always consumes and drains the given buffer. Incomplete data frame is buffered inside the decoder. std::vector frames; decoder_.decode(data, frames); + if(frames.empty()) { + // We don't have enough data to decode for one single frame, stop iteration until more data comes in. + return Http::FilterDataStatus::StopIterationNoBuffer; + } + + // Encodes the decoded frames with base64. for (auto& frame : frames) { Buffer::OwnedImpl temp; temp.add(&frame.flags_, 1); const uint32_t length = htonl(frame.length_); temp.add(&length, 4); - if (frame.length_ > 0) { + if(frame.length_>0) { temp.add(*frame.data_); } data.add(Base64::encode(temp, temp.length())); @@ -89,6 +101,8 @@ Http::FilterDataStatus GrpcWebFilter::encodeData(Buffer::Instance& data, bool) { } Http::FilterTrailersStatus GrpcWebFilter::encodeTrailers(Http::HeaderMap& trailers) { + // Trailers are expected to come all in once, and will be encoded into one single trailers frame. + // Trailers in the trailers frame are separated by CRLFs. Buffer::OwnedImpl temp; trailers.iterate([](const Http::HeaderEntry& header, void* context) -> void { Buffer::Instance* temp = static_cast(context); @@ -98,7 +112,9 @@ Http::FilterTrailersStatus GrpcWebFilter::encodeTrailers(Http::HeaderMap& traile temp->add("\r\n"); }, &temp); Buffer::OwnedImpl buffer; + // Adds the trailers frame head. buffer.add(&GRPC_WEB_TRAILER, 1); + // Adds the trailers frame length. const uint32_t length = htonl(temp.length()); buffer.add(&length, 4); buffer.move(temp); diff --git a/source/common/grpc/grpc_web_filter.h b/source/common/grpc/grpc_web_filter.h index 87b8d78dd8658..012d1c6f386d2 100644 --- a/source/common/grpc/grpc_web_filter.h +++ b/source/common/grpc/grpc_web_filter.h @@ -6,6 +6,7 @@ #include "common/common/non_copyable.h" #include "common/grpc/codec.h" + namespace Envoy { namespace Grpc { From a1b6643eae5798fdfde6c62fe34d564caaef9bff Mon Sep 17 00:00:00 2001 From: Feng Li Date: Mon, 22 May 2017 15:42:21 -0700 Subject: [PATCH 23/28] Fix format. --- source/common/grpc/grpc_web_filter.cc | 13 ++++++++----- source/common/grpc/grpc_web_filter.h | 1 - 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/source/common/grpc/grpc_web_filter.cc b/source/common/grpc/grpc_web_filter.cc index fed35a5a105bc..3e1ae18a94ee2 100644 --- a/source/common/grpc/grpc_web_filter.cc +++ b/source/common/grpc/grpc_web_filter.cc @@ -51,7 +51,8 @@ Http::FilterDataStatus GrpcWebFilter::decodeData(Buffer::Instance& data, bool) { return Http::FilterDataStatus::Continue; } - const uint64_t needed = (data.length() + decoding_buffer_.length()) / 4 * 4 - decoding_buffer_.length(); + const uint64_t needed = + (data.length() + decoding_buffer_.length()) / 4 * 4 - decoding_buffer_.length(); decoding_buffer_.move(data, needed); const std::string decoded = Base64::decode( std::string(static_cast(decoding_buffer_.linearize(decoding_buffer_.length())), @@ -78,11 +79,13 @@ Http::FilterDataStatus GrpcWebFilter::encodeData(Buffer::Instance& data, bool) { return Http::FilterDataStatus::Continue; } - // The decoder always consumes and drains the given buffer. Incomplete data frame is buffered inside the decoder. + // The decoder always consumes and drains the given buffer. Incomplete data frame is buffered + // inside the decoder. std::vector frames; decoder_.decode(data, frames); - if(frames.empty()) { - // We don't have enough data to decode for one single frame, stop iteration until more data comes in. + if (frames.empty()) { + // We don't have enough data to decode for one single frame, stop iteration until more data + // comes in. return Http::FilterDataStatus::StopIterationNoBuffer; } @@ -92,7 +95,7 @@ Http::FilterDataStatus GrpcWebFilter::encodeData(Buffer::Instance& data, bool) { temp.add(&frame.flags_, 1); const uint32_t length = htonl(frame.length_); temp.add(&length, 4); - if(frame.length_>0) { + if (frame.length_ > 0) { temp.add(*frame.data_); } data.add(Base64::encode(temp, temp.length())); diff --git a/source/common/grpc/grpc_web_filter.h b/source/common/grpc/grpc_web_filter.h index 012d1c6f386d2..87b8d78dd8658 100644 --- a/source/common/grpc/grpc_web_filter.h +++ b/source/common/grpc/grpc_web_filter.h @@ -6,7 +6,6 @@ #include "common/common/non_copyable.h" #include "common/grpc/codec.h" - namespace Envoy { namespace Grpc { From 15244b41c70e269a5528f569cb1f21217fe8c45d Mon Sep 17 00:00:00 2001 From: Feng Li Date: Tue, 23 May 2017 11:57:32 -0700 Subject: [PATCH 24/28] Address comments. --- include/envoy/http/header_map.h | 2 + source/common/grpc/grpc_web_filter.cc | 6 ++- test/common/grpc/grpc_web_filter_test.cc | 52 +++++++++++++++++------- 3 files changed, 43 insertions(+), 17 deletions(-) diff --git a/include/envoy/http/header_map.h b/include/envoy/http/header_map.h index a016f5533788f..9fe948d479e45 100644 --- a/include/envoy/http/header_map.h +++ b/include/envoy/http/header_map.h @@ -214,6 +214,7 @@ class HeaderEntry { HEADER_FUNC(Expect) \ HEADER_FUNC(ForwardedFor) \ HEADER_FUNC(ForwardedProto) \ + HEADER_FUNC(GrpcAcceptEncoding) \ HEADER_FUNC(GrpcMessage) \ HEADER_FUNC(GrpcStatus) \ HEADER_FUNC(Host) \ @@ -226,6 +227,7 @@ class HeaderEntry { HEADER_FUNC(Scheme) \ HEADER_FUNC(Server) \ HEADER_FUNC(Status) \ + HEADER_FUNC(TE) \ HEADER_FUNC(TransferEncoding) \ HEADER_FUNC(Upgrade) \ HEADER_FUNC(UserAgent) \ diff --git a/source/common/grpc/grpc_web_filter.cc b/source/common/grpc/grpc_web_filter.cc index 3e1ae18a94ee2..0300827b403fe 100644 --- a/source/common/grpc/grpc_web_filter.cc +++ b/source/common/grpc/grpc_web_filter.cc @@ -8,6 +8,7 @@ namespace Envoy { namespace Grpc { +// Bit mask denotes a trailers frame of gRPC-Web. const uint8_t GrpcWebFilter::GRPC_WEB_TRAILER = 0b10000000; GrpcWebFilter::GrpcWebFilter() : is_text_request_(false), is_text_response_(false) {} @@ -15,6 +16,7 @@ GrpcWebFilter::GrpcWebFilter() : is_text_request_(false), is_text_response_(fals GrpcWebFilter::~GrpcWebFilter() {} // Implements StreamDecoderFilter. +// TODO(fengli): Implements the subtypes of gRPC-Web content-type, like +proto, etc. Http::FilterHeadersStatus GrpcWebFilter::decodeHeaders(Http::HeaderMap& headers, bool) { const Http::HeaderEntry* content_type = headers.ContentType(); if (content_type != nullptr && @@ -48,7 +50,7 @@ Http::FilterDataStatus GrpcWebFilter::decodeData(Buffer::Instance& data, bool) { // Parse application/grpc-web-text format. if (data.length() + decoding_buffer_.length() < 4) { decoding_buffer_.move(data); - return Http::FilterDataStatus::Continue; + return Http::FilterDataStatus::StopIterationNoBuffer; } const uint64_t needed = @@ -89,7 +91,7 @@ Http::FilterDataStatus GrpcWebFilter::encodeData(Buffer::Instance& data, bool) { return Http::FilterDataStatus::StopIterationNoBuffer; } - // Encodes the decoded frames with base64. + // Encodes the decoded gRPC frames with base64. for (auto& frame : frames) { Buffer::OwnedImpl temp; temp.add(&frame.flags_, 1); diff --git a/test/common/grpc/grpc_web_filter_test.cc b/test/common/grpc/grpc_web_filter_test.cc index 760ded08d099e..9e49b766f3c18 100644 --- a/test/common/grpc/grpc_web_filter_test.cc +++ b/test/common/grpc/grpc_web_filter_test.cc @@ -20,13 +20,13 @@ namespace Envoy { namespace Grpc { namespace { const char MESSAGE[] = "\x00\x00\x00\x00\x11grpc-web-bin-data"; -const size_t MESSAGE_SIZE = 22; +const size_t MESSAGE_SIZE = sizeof(MESSAGE) - 1; const char TEXT_MESSAGE[] = "\x00\x00\x00\x00\x12grpc-web-text-data"; -const size_t TEXT_MESSAGE_SIZE = 23; +const size_t TEXT_MESSAGE_SIZE = sizeof(TEXT_MESSAGE) - 1; const char B64_MESSAGE[] = "AAAAABJncnBjLXdlYi10ZXh0LWRhdGE="; -const size_t B64_MESSAGE_SIZE = 32; -const char TRAILERS[] = "\x80\x00\x00\x00\x0fgrpc-status:0\r\n"; -const size_t TRAILERS_SIZE = 20; +const size_t B64_MESSAGE_SIZE = sizeof(B64_MESSAGE) - 1; +const char TRAILERS[] = "\x80\x00\x00\x00\x20grpc-status:0\r\ngrpc-message:ok\r\n"; +const size_t TRAILERS_SIZE = sizeof(TRAILERS) - 1; } // namespace class GrpcWebFilterTest : public testing::Test { @@ -47,11 +47,22 @@ TEST_F(GrpcWebFilterTest, BinaryUnary) { EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.decodeHeaders(request_headers, false)); EXPECT_EQ(Http::Headers::get().ContentTypeValues.Grpc, request_headers.ContentType()->value().c_str()); + EXPECT_EQ(Http::Headers::get().TEValues.Trailers, request_headers.TE()->value().c_str()); + EXPECT_EQ(Http::Headers::get().GrpcAcceptEncodingValues.Default, + request_headers.GrpcAcceptEncoding()->value().c_str()); // Tests request data. Buffer::OwnedImpl request_buffer(MESSAGE, MESSAGE_SIZE); EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.decodeData(request_buffer, true)); - EXPECT_EQ(0, memcmp(MESSAGE, TestUtility::bufferToString(request_buffer).c_str(), MESSAGE_SIZE)); + EXPECT_EQ(std::string(MESSAGE, MESSAGE_SIZE), TestUtility::bufferToString(request_buffer)); + + // Tests request trailers, they are passed through. + Http::TestHeaderMapImpl request_trailers; + request_trailers.addViaCopy(Http::Headers::get().GrpcStatus, "0"); + request_trailers.addViaCopy(Http::Headers::get().GrpcMessage, "ok"); + EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_.decodeTrailers(request_trailers)); + EXPECT_STREQ("0", request_trailers.GrpcStatus()->value().c_str()); + EXPECT_STREQ("ok", request_trailers.GrpcMessage()->value().c_str()); // Tests response headers. Http::TestHeaderMapImpl response_headers; @@ -64,7 +75,7 @@ TEST_F(GrpcWebFilterTest, BinaryUnary) { // Tests response data. Buffer::OwnedImpl response_buffer(MESSAGE, MESSAGE_SIZE); EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.encodeData(response_buffer, false)); - EXPECT_EQ(0, memcmp(MESSAGE, TestUtility::bufferToString(response_buffer).c_str(), MESSAGE_SIZE)); + EXPECT_EQ(std::string(MESSAGE, MESSAGE_SIZE), TestUtility::bufferToString(response_buffer)); response_buffer.drain(response_buffer.length()); // Tests response trailers. @@ -73,9 +84,9 @@ TEST_F(GrpcWebFilterTest, BinaryUnary) { .WillOnce(Invoke([&](Buffer::Instance& data) { trailers_buffer.move(data); })); Http::TestHeaderMapImpl response_trailers; response_trailers.addViaCopy(Http::Headers::get().GrpcStatus, "0"); + response_trailers.addViaCopy(Http::Headers::get().GrpcMessage, "ok"); EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_.encodeTrailers(response_trailers)); - EXPECT_EQ(0, - memcmp(TRAILERS, TestUtility::bufferToString(trailers_buffer).c_str(), TRAILERS_SIZE)); + EXPECT_EQ(std::string(TRAILERS, TRAILERS_SIZE), TestUtility::bufferToString(trailers_buffer)); } TEST_F(GrpcWebFilterTest, TextUnary) { @@ -88,12 +99,23 @@ TEST_F(GrpcWebFilterTest, TextUnary) { EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.decodeHeaders(request_headers, false)); EXPECT_EQ(Http::Headers::get().ContentTypeValues.Grpc, request_headers.ContentType()->value().c_str()); + EXPECT_EQ(Http::Headers::get().TEValues.Trailers, request_headers.TE()->value().c_str()); + EXPECT_EQ(Http::Headers::get().GrpcAcceptEncodingValues.Default, + request_headers.GrpcAcceptEncoding()->value().c_str()); // Tests request data. Buffer::OwnedImpl request_buffer(B64_MESSAGE, B64_MESSAGE_SIZE); EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.decodeData(request_buffer, true)); - EXPECT_EQ(0, memcmp(TEXT_MESSAGE, TestUtility::bufferToString(request_buffer).c_str(), - TEXT_MESSAGE_SIZE)); + EXPECT_EQ(std::string(TEXT_MESSAGE, TEXT_MESSAGE_SIZE), + TestUtility::bufferToString(request_buffer)); + + // Tests request trailers, they are passed through. + Http::TestHeaderMapImpl request_trailers; + request_trailers.addViaCopy(Http::Headers::get().GrpcStatus, "0"); + request_trailers.addViaCopy(Http::Headers::get().GrpcMessage, "ok"); + EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_.decodeTrailers(request_trailers)); + EXPECT_STREQ("0", request_trailers.GrpcStatus()->value().c_str()); + EXPECT_STREQ("ok", request_trailers.GrpcMessage()->value().c_str()); // Tests response headers. Http::TestHeaderMapImpl response_headers; @@ -108,8 +130,8 @@ TEST_F(GrpcWebFilterTest, TextUnary) { // Tests response data. Buffer::OwnedImpl response_buffer(TEXT_MESSAGE, TEXT_MESSAGE_SIZE); EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.encodeData(response_buffer, false)); - EXPECT_EQ(0, memcmp(B64_MESSAGE, TestUtility::bufferToString(response_buffer).c_str(), - B64_MESSAGE_SIZE)); + EXPECT_EQ(std::string(B64_MESSAGE, B64_MESSAGE_SIZE), + TestUtility::bufferToString(response_buffer)); response_buffer.drain(response_buffer.length()); // Tests response trailers. @@ -118,9 +140,9 @@ TEST_F(GrpcWebFilterTest, TextUnary) { .WillOnce(Invoke([&](Buffer::Instance& data) { trailers_buffer.move(data); })); Http::TestHeaderMapImpl response_trailers; response_trailers.addViaCopy(Http::Headers::get().GrpcStatus, "0"); + response_trailers.addViaCopy(Http::Headers::get().GrpcMessage, "ok"); EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_.encodeTrailers(response_trailers)); - EXPECT_EQ(0, - memcmp(TRAILERS, TestUtility::bufferToString(trailers_buffer).c_str(), TRAILERS_SIZE)); + EXPECT_EQ(std::string(TRAILERS, TRAILERS_SIZE), TestUtility::bufferToString(trailers_buffer)); } } // namespace Grpc } // namespace Envoy From 88e5c630c6ab8cd5ec7e51bc017563019fdb7f06 Mon Sep 17 00:00:00 2001 From: Feng Li Date: Tue, 23 May 2017 13:03:44 -0700 Subject: [PATCH 25/28] Address comments. --- source/common/grpc/grpc_web_filter.cc | 5 +++-- source/common/grpc/grpc_web_filter.h | 2 +- test/server/config/http/BUILD | 1 + test/server/config/http/config_test.cc | 17 +++++++++++++++++ 4 files changed, 22 insertions(+), 3 deletions(-) diff --git a/source/common/grpc/grpc_web_filter.cc b/source/common/grpc/grpc_web_filter.cc index 0300827b403fe..8d8a606218df9 100644 --- a/source/common/grpc/grpc_web_filter.cc +++ b/source/common/grpc/grpc_web_filter.cc @@ -21,7 +21,7 @@ Http::FilterHeadersStatus GrpcWebFilter::decodeHeaders(Http::HeaderMap& headers, const Http::HeaderEntry* content_type = headers.ContentType(); if (content_type != nullptr && Http::Headers::get().ContentTypeValues.GrpcWebText == content_type->value().c_str()) { - // Checks whether gRPC-Web client is sending b64 ncoded request. + // Checks whether gRPC-Web client is sending base64 encoded request. is_text_request_ = true; } headers.insertContentType().value(Http::Headers::get().ContentTypeValues.Grpc); @@ -29,7 +29,7 @@ Http::FilterHeadersStatus GrpcWebFilter::decodeHeaders(Http::HeaderMap& headers, const Http::HeaderEntry* accept = headers.get(Http::Headers::get().Accept); if (accept != nullptr && Http::Headers::get().ContentTypeValues.GrpcWebText == accept->value().c_str()) { - // Checks whether gRPC-Web client is asking for b64 encoded response. + // Checks whether gRPC-Web client is asking for base64 encoded response. is_text_response_ = true; } @@ -126,5 +126,6 @@ Http::FilterTrailersStatus GrpcWebFilter::encodeTrailers(Http::HeaderMap& traile encoder_callbacks_->addEncodedData(buffer); return Http::FilterTrailersStatus::Continue; } + } // namespace Grpc } // namespace Envoy diff --git a/source/common/grpc/grpc_web_filter.h b/source/common/grpc/grpc_web_filter.h index 87b8d78dd8658..f1199f26d8d03 100644 --- a/source/common/grpc/grpc_web_filter.h +++ b/source/common/grpc/grpc_web_filter.h @@ -22,7 +22,6 @@ class GrpcWebFilter : public Http::StreamFilter, NonCopyable { // Implements StreamDecoderFilter. Http::FilterHeadersStatus decodeHeaders(Http::HeaderMap&, bool) override; Http::FilterDataStatus decodeData(Buffer::Instance&, bool) override; - Http::FilterTrailersStatus decodeTrailers(Http::HeaderMap&) override { return Http::FilterTrailersStatus::Continue; } @@ -47,5 +46,6 @@ class GrpcWebFilter : public Http::StreamFilter, NonCopyable { Buffer::OwnedImpl decoding_buffer_; Decoder decoder_; }; + } // namespace Grpc } // namespace Envoy diff --git a/test/server/config/http/BUILD b/test/server/config/http/BUILD index 373c8530f4379..e1495cc1e26e5 100644 --- a/test/server/config/http/BUILD +++ b/test/server/config/http/BUILD @@ -16,6 +16,7 @@ envoy_cc_test( "//source/server/config/http:dynamo_lib", "//source/server/config/http:fault_lib", "//source/server/config/http:grpc_http1_bridge_lib", + "//source/server/config/http:grpc_web_lib", "//source/server/config/http:ratelimit_lib", "//source/server/config/http:router_lib", "//source/server/http:health_check_lib", diff --git a/test/server/config/http/config_test.cc b/test/server/config/http/config_test.cc index 42f8e3bb39b04..8352eebd8692e 100644 --- a/test/server/config/http/config_test.cc +++ b/test/server/config/http/config_test.cc @@ -4,6 +4,7 @@ #include "server/config/http/dynamo.h" #include "server/config/http/fault.h" #include "server/config/http/grpc_http1_bridge.h" +#include "server/config/http/grpc_web.h" #include "server/config/http/ratelimit.h" #include "server/config/http/router.h" #include "server/http/health_check.h" @@ -107,6 +108,22 @@ TEST(HttpFilterConfigTest, GrpcHttp1BridgeFilter) { cb(filter_callback); } +TEST(HttpFilterConfigTest, GrpcWebFilter) { + std::string json_string = R"EOF( + { + } + )EOF"; + + Json::ObjectSharedPtr json_config = Json::Factory::loadFromString(json_string); + NiceMock server; + GrpcWebFilterConfig factory; + HttpFilterFactoryCb cb = factory.tryCreateFilterFactory(HttpFilterType::Both, "grpc_web", + *json_config, "stats", server); + Http::MockFilterChainFactoryCallbacks filter_callback; + EXPECT_CALL(filter_callback, addStreamFilter(_)); + cb(filter_callback); +} + TEST(HttpFilterConfigTest, HealthCheckFilter) { std::string json_string = R"EOF( { From 0f93298d30fa47f12724f30eb5c10e1e84398f07 Mon Sep 17 00:00:00 2001 From: Feng Li Date: Tue, 23 May 2017 13:23:44 -0700 Subject: [PATCH 26/28] Address comments. --- source/common/grpc/grpc_web_filter.cc | 4 ---- source/common/grpc/grpc_web_filter.h | 13 +++++-------- 2 files changed, 5 insertions(+), 12 deletions(-) diff --git a/source/common/grpc/grpc_web_filter.cc b/source/common/grpc/grpc_web_filter.cc index 8d8a606218df9..db1271820eb5a 100644 --- a/source/common/grpc/grpc_web_filter.cc +++ b/source/common/grpc/grpc_web_filter.cc @@ -11,10 +11,6 @@ namespace Grpc { // Bit mask denotes a trailers frame of gRPC-Web. const uint8_t GrpcWebFilter::GRPC_WEB_TRAILER = 0b10000000; -GrpcWebFilter::GrpcWebFilter() : is_text_request_(false), is_text_response_(false) {} - -GrpcWebFilter::~GrpcWebFilter() {} - // Implements StreamDecoderFilter. // TODO(fengli): Implements the subtypes of gRPC-Web content-type, like +proto, etc. Http::FilterHeadersStatus GrpcWebFilter::decodeHeaders(Http::HeaderMap& headers, bool) { diff --git a/source/common/grpc/grpc_web_filter.h b/source/common/grpc/grpc_web_filter.h index f1199f26d8d03..e68d47cb86956 100644 --- a/source/common/grpc/grpc_web_filter.h +++ b/source/common/grpc/grpc_web_filter.h @@ -14,8 +14,8 @@ namespace Grpc { */ class GrpcWebFilter : public Http::StreamFilter, NonCopyable { public: - GrpcWebFilter(); - virtual ~GrpcWebFilter(); + GrpcWebFilter(){}; + virtual ~GrpcWebFilter(){}; void onDestroy() override{}; @@ -25,9 +25,7 @@ class GrpcWebFilter : public Http::StreamFilter, NonCopyable { Http::FilterTrailersStatus decodeTrailers(Http::HeaderMap&) override { return Http::FilterTrailersStatus::Continue; } - void setDecoderFilterCallbacks(Http::StreamDecoderFilterCallbacks& callbacks) override { - decoder_callbacks_ = &callbacks; - } + void setDecoderFilterCallbacks(Http::StreamDecoderFilterCallbacks&) override {} // Implements StreamEncoderFilter. Http::FilterHeadersStatus encodeHeaders(Http::HeaderMap&, bool) override; @@ -39,10 +37,9 @@ class GrpcWebFilter : public Http::StreamFilter, NonCopyable { private: static const uint8_t GRPC_WEB_TRAILER; - Http::StreamDecoderFilterCallbacks* decoder_callbacks_{}; Http::StreamEncoderFilterCallbacks* encoder_callbacks_{}; - bool is_text_request_; - bool is_text_response_; + bool is_text_request_{}; + bool is_text_response_{}; Buffer::OwnedImpl decoding_buffer_; Decoder decoder_; }; From 6dc30082250b42d0e13707a7c72209abfd406fa2 Mon Sep 17 00:00:00 2001 From: Feng Li Date: Wed, 24 May 2017 10:51:23 -0700 Subject: [PATCH 27/28] Change to use NamedHttpFilterConfigFactory to register gRPC-Web filter. --- source/server/config/http/grpc_web.cc | 22 ++++++++++++---------- source/server/config/http/grpc_web.h | 9 +++++---- test/server/config/http/config_test.cc | 4 ++-- 3 files changed, 19 insertions(+), 16 deletions(-) diff --git a/source/server/config/http/grpc_web.cc b/source/server/config/http/grpc_web.cc index 806c469a31bea..ef21e2049748f 100644 --- a/source/server/config/http/grpc_web.cc +++ b/source/server/config/http/grpc_web.cc @@ -6,24 +6,26 @@ namespace Envoy { namespace Server { namespace Configuration { -HttpFilterFactoryCb GrpcWebFilterConfig::tryCreateFilterFactory(HttpFilterType type, - const std::string& name, - const Json::Object&, - const std::string&, - Server::Instance&) { - if (type != HttpFilterType::Both || name != "grpc_web") { - return nullptr; +HttpFilterFactoryCb GrpcWebFilterConfig::createFilterFactory(HttpFilterType type, + const Json::Object&, + const std::string&, + Server::Instance& server) { + if (type != HttpFilterType::Both) { + throw EnvoyException(fmt::format( + "{} gRPC-Web filter must be configured as both a decoder and encoder filter.", name())); } - return [](Http::FilterChainFactoryCallbacks& callbacks) -> void { + return [&server](Http::FilterChainFactoryCallbacks& callbacks) -> void { callbacks.addStreamFilter(Http::StreamFilterSharedPtr{new Grpc::GrpcWebFilter()}); }; } +std::string GrpcWebFilterConfig::name() { return "grpc_web"; } + /** - * Static registration for the gRpc-Web filter. @see RegisterHttpFilterConfigFactory. + * Static registration for the gRPC-Web filter. @see RegisterNamedHttpFilterConfigFactory. */ -static RegisterHttpFilterConfigFactory register_; +static RegisterNamedHttpFilterConfigFactory register_; } // namespace Configuration } // namespace Server diff --git a/source/server/config/http/grpc_web.h b/source/server/config/http/grpc_web.h index 04d66a0a8d225..858a2a073114c 100644 --- a/source/server/config/http/grpc_web.h +++ b/source/server/config/http/grpc_web.h @@ -6,11 +6,12 @@ namespace Envoy { namespace Server { namespace Configuration { -class GrpcWebFilterConfig : public HttpFilterConfigFactory { +class GrpcWebFilterConfig : public NamedHttpFilterConfigFactory { public: - HttpFilterFactoryCb tryCreateFilterFactory(HttpFilterType type, const std::string& name, - const Json::Object&, const std::string&, - Server::Instance& server) override; + HttpFilterFactoryCb createFilterFactory(HttpFilterType type, const Json::Object&, + const std::string&, Server::Instance& server) override; + + std::string name() override; }; } // namespace Configuration diff --git a/test/server/config/http/config_test.cc b/test/server/config/http/config_test.cc index 3687e99c1e612..66ad037da01a3 100644 --- a/test/server/config/http/config_test.cc +++ b/test/server/config/http/config_test.cc @@ -120,8 +120,8 @@ TEST(HttpFilterConfigTest, GrpcWebFilter) { Json::ObjectSharedPtr json_config = Json::Factory::loadFromString(json_string); NiceMock server; GrpcWebFilterConfig factory; - HttpFilterFactoryCb cb = factory.tryCreateFilterFactory(HttpFilterType::Both, "grpc_web", - *json_config, "stats", server); + HttpFilterFactoryCb cb = + factory.createFilterFactory(HttpFilterType::Both, *json_config, "stats", server); Http::MockFilterChainFactoryCallbacks filter_callback; EXPECT_CALL(filter_callback, addStreamFilter(_)); cb(filter_callback); From ddb90fdc98bc7a6161b13040e9c401e95f6e0da5 Mon Sep 17 00:00:00 2001 From: Feng Li Date: Wed, 24 May 2017 10:59:03 -0700 Subject: [PATCH 28/28] Remove the unused capture of server variable. --- source/server/config/http/grpc_web.cc | 4 ++-- source/server/config/http/grpc_web.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/source/server/config/http/grpc_web.cc b/source/server/config/http/grpc_web.cc index ef21e2049748f..b35ff2fd7ee5b 100644 --- a/source/server/config/http/grpc_web.cc +++ b/source/server/config/http/grpc_web.cc @@ -9,13 +9,13 @@ namespace Configuration { HttpFilterFactoryCb GrpcWebFilterConfig::createFilterFactory(HttpFilterType type, const Json::Object&, const std::string&, - Server::Instance& server) { + Server::Instance&) { if (type != HttpFilterType::Both) { throw EnvoyException(fmt::format( "{} gRPC-Web filter must be configured as both a decoder and encoder filter.", name())); } - return [&server](Http::FilterChainFactoryCallbacks& callbacks) -> void { + return [](Http::FilterChainFactoryCallbacks& callbacks) -> void { callbacks.addStreamFilter(Http::StreamFilterSharedPtr{new Grpc::GrpcWebFilter()}); }; } diff --git a/source/server/config/http/grpc_web.h b/source/server/config/http/grpc_web.h index 858a2a073114c..9bf131c4fba8c 100644 --- a/source/server/config/http/grpc_web.h +++ b/source/server/config/http/grpc_web.h @@ -9,7 +9,7 @@ namespace Configuration { class GrpcWebFilterConfig : public NamedHttpFilterConfigFactory { public: HttpFilterFactoryCb createFilterFactory(HttpFilterType type, const Json::Object&, - const std::string&, Server::Instance& server) override; + const std::string&, Server::Instance&) override; std::string name() override; };