From 61fb32aa37fe463dc9727e1544abdffb7b82c9bd Mon Sep 17 00:00:00 2001 From: "leilei.gll" Date: Fri, 15 Feb 2019 13:30:03 +0800 Subject: [PATCH 01/23] Implement the routing of Dubbo requests Signed-off-by: leilei.gll --- .../filters/network/dubbo_proxy/BUILD | 20 +- .../network/dubbo_proxy/app_exception.cc | 72 +++ .../network/dubbo_proxy/app_exception.h | 47 ++ .../network/dubbo_proxy/deserializer.h | 12 + .../dubbo_proxy/dubbo_protocol_impl.cc | 98 +++- .../network/dubbo_proxy/dubbo_protocol_impl.h | 11 +- .../dubbo_proxy/hessian_deserializer_impl.cc | 19 +- .../dubbo_proxy/hessian_deserializer_impl.h | 2 + .../network/dubbo_proxy/hessian_utils.cc | 48 ++ .../network/dubbo_proxy/hessian_utils.h | 2 + .../filters/network/dubbo_proxy/message.h | 14 + .../filters/network/dubbo_proxy/metadata.h | 2 +- .../filters/network/dubbo_proxy/protocol.h | 45 +- .../filters/network/dubbo_proxy/router/BUILD | 37 ++ .../network/dubbo_proxy/router/config.cc | 34 ++ .../network/dubbo_proxy/router/config.h | 30 ++ .../network/dubbo_proxy/router/router_impl.cc | 324 ++++++++++++ .../network/dubbo_proxy/router/router_impl.h | 104 ++++ .../filters/network/dubbo_proxy/BUILD | 30 ++ .../network/dubbo_proxy/app_exception_test.cc | 84 ++++ .../dubbo_proxy/dubbo_protocol_impl_test.cc | 8 +- .../filters/network/dubbo_proxy/mocks.cc | 11 +- .../filters/network/dubbo_proxy/mocks.h | 21 + .../network/dubbo_proxy/router_test.cc | 462 ++++++++++++++++++ 24 files changed, 1515 insertions(+), 22 deletions(-) create mode 100644 source/extensions/filters/network/dubbo_proxy/app_exception.cc create mode 100644 source/extensions/filters/network/dubbo_proxy/app_exception.h create mode 100644 source/extensions/filters/network/dubbo_proxy/router/config.cc create mode 100644 source/extensions/filters/network/dubbo_proxy/router/config.h create mode 100644 source/extensions/filters/network/dubbo_proxy/router/router_impl.cc create mode 100644 source/extensions/filters/network/dubbo_proxy/router/router_impl.h create mode 100644 test/extensions/filters/network/dubbo_proxy/app_exception_test.cc create mode 100644 test/extensions/filters/network/dubbo_proxy/router_test.cc diff --git a/source/extensions/filters/network/dubbo_proxy/BUILD b/source/extensions/filters/network/dubbo_proxy/BUILD index b95792467befa..828e4b6f9deaa 100644 --- a/source/extensions/filters/network/dubbo_proxy/BUILD +++ b/source/extensions/filters/network/dubbo_proxy/BUILD @@ -35,6 +35,7 @@ envoy_cc_library( deps = [ ":buffer_helper_lib", ":message_lib", + ":metadata_lib", "//source/common/common:assert_lib", "//source/common/config:utility_lib", "//source/common/singleton:const_singleton", @@ -133,7 +134,6 @@ envoy_cc_library( external_deps = ["abseil_optional"], deps = [ ":message_lib", - ":protocol_interface", "//source/common/http:header_map_lib", ], ) @@ -161,3 +161,21 @@ envoy_cc_library( "//include/envoy/stats:stats_macros", ], ) + +envoy_cc_library( + name = "app_exception_lib", + srcs = ["app_exception.cc"], + hdrs = ["app_exception.h"], + deps = [ + ":deserializer_interface", + ":hessian_deserializer_impl_lib", + ":hessian_utils_lib", + ":message_lib", + ":metadata_lib", + ":protocol_interface", + "//include/envoy/buffer:buffer_interface", + "//source/common/buffer:buffer_lib", + "//source/common/common:byte_order_lib", + "//source/extensions/filters/network/dubbo_proxy/filters:filter_interface", + ], +) diff --git a/source/extensions/filters/network/dubbo_proxy/app_exception.cc b/source/extensions/filters/network/dubbo_proxy/app_exception.cc new file mode 100644 index 0000000000000..2b87a1a8aeea2 --- /dev/null +++ b/source/extensions/filters/network/dubbo_proxy/app_exception.cc @@ -0,0 +1,72 @@ +#include "extensions/filters/network/dubbo_proxy/app_exception.h" + +#include "common/buffer/buffer_impl.h" +#include "common/common/byte_order.h" +#include "common/common/hex.h" + +#include "extensions/filters/network/dubbo_proxy/message.h" + +namespace Envoy { +namespace Extensions { +namespace NetworkFilters { +namespace DubboProxy { + +AppException::AppException(AppExceptionType type, const std::string& what) + : EnvoyException(what), type_(type), response_type_(RpcResponseType::ResponseWithException) {} + +AppException::AppException(const AppException& ex) : EnvoyException(ex.what()), type_(ex.type_) {} + +AppException::ResponseType AppException::encode(MessageMetadata& metadata, + DubboProxy::Protocol& protocol, + Deserializer& deserializer, + Buffer::Instance& buffer) const { + ENVOY_LOG(debug, "err {}", what()); + + switch (type_) { + case AppExceptionType::ClientTimeout: + metadata.setResponseStatus(ResponseStatus::ClientTimeout); + break; + case AppExceptionType::ServerTimeout: + metadata.setResponseStatus(ResponseStatus::ServerTimeout); + break; + case AppExceptionType::BadRequest: + metadata.setResponseStatus(ResponseStatus::BadRequest); + break; + case AppExceptionType::BadResponse: + metadata.setResponseStatus(ResponseStatus::BadResponse); + break; + case AppExceptionType::ServiceNotFound: + metadata.setResponseStatus(ResponseStatus::ServiceNotFound); + break; + case AppExceptionType::ServiceError: + metadata.setResponseStatus(ResponseStatus::ServiceError); + break; + case AppExceptionType::ServerError: + metadata.setResponseStatus(ResponseStatus::ServerError); + break; + case AppExceptionType::ClientError: + metadata.setResponseStatus(ResponseStatus::ClientError); + break; + case AppExceptionType::ServerThreadpoolExhaustedError: + metadata.setResponseStatus(ResponseStatus::ServerThreadpoolExhaustedError); + break; + default: + NOT_REACHED_GCOVR_EXCL_LINE + } + + metadata.setMessageType(MessageType::Response); + + const std::string& response = what(); + if (!protocol.encode(buffer, response.size(), metadata)) { + throw EnvoyException("failed to encode local reply message"); + } + + deserializer.serializeRpcResult(buffer, response, response_type_); + + return DirectResponse::ResponseType::Exception; +} + +} // namespace DubboProxy +} // namespace NetworkFilters +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/filters/network/dubbo_proxy/app_exception.h b/source/extensions/filters/network/dubbo_proxy/app_exception.h new file mode 100644 index 0000000000000..d30c246edac42 --- /dev/null +++ b/source/extensions/filters/network/dubbo_proxy/app_exception.h @@ -0,0 +1,47 @@ +#pragma once + +#include "envoy/common/exception.h" + +#include "extensions/filters/network/dubbo_proxy/deserializer.h" +#include "extensions/filters/network/dubbo_proxy/filters/filter.h" +#include "extensions/filters/network/dubbo_proxy/metadata.h" +#include "extensions/filters/network/dubbo_proxy/protocol.h" + +namespace Envoy { +namespace Extensions { +namespace NetworkFilters { +namespace DubboProxy { + +/** + * Dubbo Application Exception types. + */ +enum class AppExceptionType { + ClientTimeout, + ServerTimeout, + BadRequest, + BadResponse, + ServiceNotFound, + ServiceError, + ServerError, + ClientError, + ServerThreadpoolExhaustedError, +}; + +struct AppException : public EnvoyException, + public DubboFilters::DirectResponse, + Logger::Loggable { + AppException(AppExceptionType type, const std::string& what); + AppException(const AppException& ex); + + using ResponseType = DubboFilters::DirectResponse::ResponseType; + ResponseType encode(MessageMetadata& metadata, Protocol& protocol, Deserializer& deserializer, + Buffer::Instance& buffer) const override; + + const AppExceptionType type_; + RpcResponseType response_type_; +}; + +} // namespace DubboProxy +} // namespace NetworkFilters +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/filters/network/dubbo_proxy/deserializer.h b/source/extensions/filters/network/dubbo_proxy/deserializer.h index f842fcb1df8c5..6dcc36b2944bb 100644 --- a/source/extensions/filters/network/dubbo_proxy/deserializer.h +++ b/source/extensions/filters/network/dubbo_proxy/deserializer.h @@ -107,6 +107,18 @@ class Deserializer { * @throws EnvoyException if the data is not valid for this serialization */ virtual RpcResultPtr deserializeRpcResult(Buffer::Instance& buffer, size_t body_size) PURE; + + /** + * serialize result of an rpc call + * If successful, the output_buffer is written to the serialized data + * + * @param output_buffer store the serialized data + * @param content the rpc response content + * @param type the rpc response type + * @throws EnvoyException if the data is not valid for this serialization + */ + virtual void serializeRpcResult(Buffer::Instance& output_buffer, const std::string& content, + RpcResponseType type) PURE; }; typedef std::unique_ptr DeserializerPtr; diff --git a/source/extensions/filters/network/dubbo_proxy/dubbo_protocol_impl.cc b/source/extensions/filters/network/dubbo_proxy/dubbo_protocol_impl.cc index 4682163ae01fd..a988c34b16c95 100644 --- a/source/extensions/filters/network/dubbo_proxy/dubbo_protocol_impl.cc +++ b/source/extensions/filters/network/dubbo_proxy/dubbo_protocol_impl.cc @@ -53,6 +53,36 @@ bool isValidResponseStatus(ResponseStatus status) { return true; } +void parseRequestInfoFromBuffer(Buffer::Instance& data, MessageMetadataSharedPtr metadata) { + ASSERT(data.length() >= DubboProtocolImpl::MessageSize); + uint8_t flag = data.peekInt(FlagOffset); + bool is_two_way = (flag & TwoWayMask) == TwoWayMask ? true : false; + SerializationType type = static_cast(flag & SerializationTypeMask); + if (!isValidSerializationType(type)) { + throw EnvoyException( + fmt::format("invalid dubbo message serialization type {}", + static_cast::type>(type))); + } + + if (!is_two_way) { + metadata->setMessageType(MessageType::Oneway); + } + + metadata->setSerializationType(type); +} + +void parseResponseInfoFromBuffer(Buffer::Instance& buffer, MessageMetadataSharedPtr metadata) { + ASSERT(buffer.length() >= DubboProtocolImpl::MessageSize); + ResponseStatus status = static_cast(buffer.peekInt(StatusOffset)); + if (!isValidResponseStatus(status)) { + throw EnvoyException( + fmt::format("invalid dubbo message response status {}", + static_cast::type>(status))); + } + + metadata->setResponseStatus(status); +} + void RequestMessageImpl::fromBuffer(Buffer::Instance& data) { ASSERT(data.length() >= DubboProtocolImpl::MessageSize); uint8_t flag = data.peekInt(FlagOffset); @@ -76,6 +106,8 @@ void ResponseMessageImpl::fromBuffer(Buffer::Instance& buffer) { } bool DubboProtocolImpl::decode(Buffer::Instance& buffer, Protocol::Context* context) { + ASSERT(callbacks_); + if (buffer.length() < DubboProtocolImpl::MessageSize) { return false; } @@ -103,18 +135,80 @@ bool DubboProtocolImpl::decode(Buffer::Instance& buffer, Protocol::Context* cont std::make_unique(request_id, body_size, is_event); req->fromBuffer(buffer); context->is_request_ = true; - callbacks_.onRequestMessage(std::move(req)); + callbacks_->onRequestMessage(std::move(req)); } else { ResponseMessageImplPtr res = std::make_unique(request_id, body_size, is_event); res->fromBuffer(buffer); - callbacks_.onResponseMessage(std::move(res)); + callbacks_->onResponseMessage(std::move(res)); } buffer.drain(MessageSize); return true; } +bool DubboProtocolImpl::decode(Buffer::Instance& buffer, Protocol::Context* context, + MessageMetadataSharedPtr metadata) { + if (!metadata) { + throw EnvoyException("invalid metadata parameter"); + } + + if (buffer.length() < DubboProtocolImpl::MessageSize) { + return false; + } + + uint16_t magic_number = buffer.peekBEInt(); + if (magic_number != MagicNumber) { + throw EnvoyException(fmt::format("invalid dubbo message magic number {}", magic_number)); + } + + uint8_t flag = buffer.peekInt(FlagOffset); + MessageType type = + (flag & MessageTypeMask) == MessageTypeMask ? MessageType::Request : MessageType::Response; + bool is_event = (flag & EventMask) == EventMask ? true : false; + int64_t request_id = buffer.peekBEInt(RequestIDOffset); + int32_t body_size = buffer.peekBEInt(BodySizeOffset); + + if (body_size > MaxBodySize || body_size <= 0) { + throw EnvoyException(fmt::format("invalid dubbo message size {}", body_size)); + } + + metadata->setMessageType(type); + metadata->setRequestId(request_id); + + if (type == MessageType::Request) { + parseRequestInfoFromBuffer(buffer, metadata); + } else { + parseResponseInfoFromBuffer(buffer, metadata); + } + + context->header_size_ = DubboProtocolImpl::MessageSize; + context->body_size_ = body_size; + context->is_heartbeat_ = is_event; + + return true; +} + +bool DubboProtocolImpl::encode(Buffer::Instance& buffer, int32_t body_size, + const MessageMetadata& metadata) { + switch (metadata.message_type()) { + case MessageType::Response: { + ASSERT(metadata.response_status().has_value()); + buffer.writeBEInt(MagicNumber); + buffer.writeByte(static_cast(metadata.serialization_type())); + buffer.writeByte(static_cast(metadata.response_status().value())); + buffer.writeBEInt(metadata.request_id()); + buffer.writeBEInt(body_size); + return true; + } + case MessageType::Request: { + NOT_IMPLEMENTED_GCOVR_EXCL_LINE; + } + default: + NOT_REACHED_GCOVR_EXCL_LINE; + } +} + class DubboProtocolConfigFactory : public ProtocolFactoryBase { public: DubboProtocolConfigFactory() : ProtocolFactoryBase(ProtocolType::Dubbo) {} diff --git a/source/extensions/filters/network/dubbo_proxy/dubbo_protocol_impl.h b/source/extensions/filters/network/dubbo_proxy/dubbo_protocol_impl.h index 34de7dfa33292..f5dca2d3c1dd7 100644 --- a/source/extensions/filters/network/dubbo_proxy/dubbo_protocol_impl.h +++ b/source/extensions/filters/network/dubbo_proxy/dubbo_protocol_impl.h @@ -68,16 +68,21 @@ typedef std::unique_ptr ResponseMessageImplPtr; class DubboProtocolImpl : public Protocol { public: - DubboProtocolImpl(ProtocolCallbacks& callbacks) : callbacks_(callbacks) {} + DubboProtocolImpl() {} + DubboProtocolImpl(ProtocolCallbacks* callbacks) : callbacks_(callbacks) {} const std::string& name() const override { return ProtocolNames::get().fromType(type()); } ProtocolType type() const override { return ProtocolType::Dubbo; } - virtual bool decode(Buffer::Instance& buffer, Protocol::Context* context) override; + bool decode(Buffer::Instance& buffer, Protocol::Context* context) override; + bool decode(Buffer::Instance& buffer, Protocol::Context* context, + MessageMetadataSharedPtr metadata) override; + bool encode(Buffer::Instance& buffer, int32_t body_size, + const MessageMetadata& metadata) override; static constexpr uint8_t MessageSize = 16; static constexpr int32_t MaxBodySize = 16 * 1024 * 1024; private: - ProtocolCallbacks& callbacks_; + ProtocolCallbacks* callbacks_; }; } // namespace DubboProxy diff --git a/source/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl.cc b/source/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl.cc index c86a297c48f78..b126e62d76a9a 100644 --- a/source/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl.cc +++ b/source/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl.cc @@ -14,15 +14,6 @@ namespace Extensions { namespace NetworkFilters { namespace DubboProxy { -enum class RpcResponseType : uint8_t { - ResponseWithException = 0, - ResponseWithValue = 1, - ResponseWithNullValue = 2, - ResponseWithExceptionWithAttachments = 3, - ResponseValueWithAttachments = 4, - ResponseNullValueWithAttachments = 5, -}; - RpcInvocationPtr HessianDeserializerImpl::deserializeRpcInvocation(Buffer::Instance& buffer, size_t body_size) { ASSERT(buffer.length() >= body_size); @@ -60,6 +51,8 @@ RpcResultPtr HessianDeserializerImpl::deserializeRpcResult(Buffer::Instance& buf result = std::make_unique(true); break; case RpcResponseType::ResponseWithValue: + result = std::make_unique(true); + break; case RpcResponseType::ResponseWithNullValue: has_value = false; FALLTHRU; @@ -85,6 +78,14 @@ RpcResultPtr HessianDeserializerImpl::deserializeRpcResult(Buffer::Instance& buf return result; } +void HessianDeserializerImpl::serializeRpcResult(Buffer::Instance& output_buffer, + const std::string& content, RpcResponseType type) { + uint8_t response_type_code = static_cast(type); + uint8_t base_code = 0x90; + output_buffer.writeByte(static_cast(base_code + response_type_code)); + HessianUtils::writeString(output_buffer, content); +} + class HessianDeserializerConfigFactory : public DeserializerFactoryBase { public: HessianDeserializerConfigFactory() : DeserializerFactoryBase(SerializationType::Hessian) {} diff --git a/source/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl.h b/source/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl.h index b56c44fbf7774..1f24c07c2fefd 100644 --- a/source/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl.h +++ b/source/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl.h @@ -17,6 +17,8 @@ class HessianDeserializerImpl : public Deserializer { virtual RpcInvocationPtr deserializeRpcInvocation(Buffer::Instance& buffer, size_t body_size) override; virtual RpcResultPtr deserializeRpcResult(Buffer::Instance& buffer, size_t body_size) override; + virtual void serializeRpcResult(Buffer::Instance& output_buffer, const std::string& content, + RpcResponseType type) override; }; } // namespace DubboProxy diff --git a/source/extensions/filters/network/dubbo_proxy/hessian_utils.cc b/source/extensions/filters/network/dubbo_proxy/hessian_utils.cc index 1dc6f55a2db9e..547f4711c256e 100644 --- a/source/extensions/filters/network/dubbo_proxy/hessian_utils.cc +++ b/source/extensions/filters/network/dubbo_proxy/hessian_utils.cc @@ -24,6 +24,48 @@ typename std::enable_if::value, T>::type leftShift(T left, uin return left << bit_number; } +inline void addByte(Buffer::Instance& buffer, const uint8_t value) { buffer.add(&value, 1); } + +void addSeq(Buffer::Instance& buffer, const std::initializer_list values) { + for (const int8_t& value : values) { + buffer.add(&value, 1); + } +} + +void doWriteString(Buffer::Instance& instance, absl::string_view str_view) { + const size_t length = str_view.length(); + constexpr size_t str_max_length = 0xffff; + constexpr size_t two_octet_max_lenth = 1024; + + if (length < 32) { + addByte(instance, static_cast(length)); + instance.add(str_view.data(), str_view.length()); + return; + } + + if (length < two_octet_max_lenth) { + uint8_t code = length / 256; // 0x30 + length / 0x100 must less than 0x34 + uint8_t remain = length % 256; + addSeq(instance, {static_cast(0x30 + code), remain}); + instance.add(str_view.data(), str_view.length()); + return; + } + + if (length <= str_max_length) { + uint8_t code = length / 256; + uint8_t remain = length % 256; + addSeq(instance, {'S', code, remain}); + instance.add(str_view.data(), str_view.length()); + return; + } + + addSeq(instance, {0x52, 0xff, 0xff}); + instance.add(str_view.data(), str_max_length); + doWriteString(instance, + absl::string_view(str_view.data() + str_max_length, length - str_max_length)); + return; +} + } // namespace /* @@ -516,6 +558,12 @@ std::string HessianUtils::readByte(Buffer::Instance& buffer) { return result; } +void HessianUtils::writeString(Buffer::Instance& buffer, const std::string& str) { + absl::string_view str_view(str); + doWriteString(buffer, str_view); + return; +} + } // namespace DubboProxy } // namespace NetworkFilters } // namespace Extensions diff --git a/source/extensions/filters/network/dubbo_proxy/hessian_utils.h b/source/extensions/filters/network/dubbo_proxy/hessian_utils.h index 39ca52cbbd701..d136019003b9f 100644 --- a/source/extensions/filters/network/dubbo_proxy/hessian_utils.h +++ b/source/extensions/filters/network/dubbo_proxy/hessian_utils.h @@ -35,6 +35,8 @@ class HessianUtils { static void readNull(Buffer::Instance& buffer); static std::chrono::milliseconds readDate(Buffer::Instance& buffer); static std::string readByte(Buffer::Instance& buffer); + + static void writeString(Buffer::Instance& buffer, const std::string& str); }; } // namespace DubboProxy diff --git a/source/extensions/filters/network/dubbo_proxy/message.h b/source/extensions/filters/network/dubbo_proxy/message.h index de2c1163f0489..81ecdae6b2f22 100644 --- a/source/extensions/filters/network/dubbo_proxy/message.h +++ b/source/extensions/filters/network/dubbo_proxy/message.h @@ -20,6 +20,11 @@ enum class SerializationType : uint8_t { enum class MessageType : uint8_t { Response = 0, Request = 1, + Oneway = 2, + Exception = 3, + + // ATTENTION: MAKE SURE THIS REMAINS EQUAL TO THE LAST MESSAGE TYPE + LastMessageType = Exception, }; /** @@ -39,6 +44,15 @@ enum class ResponseStatus : uint8_t { ServerThreadpoolExhaustedError = 100, }; +enum class RpcResponseType : uint8_t { + ResponseWithException = 0, + ResponseWithValue = 1, + ResponseWithNullValue = 2, + ResponseWithExceptionWithAttachments = 3, + ResponseValueWithAttachments = 4, + ResponseNullValueWithAttachments = 5, +}; + class Message { public: virtual ~Message() {} diff --git a/source/extensions/filters/network/dubbo_proxy/metadata.h b/source/extensions/filters/network/dubbo_proxy/metadata.h index 6fad8767fb41a..d67cd21a4a6fd 100644 --- a/source/extensions/filters/network/dubbo_proxy/metadata.h +++ b/source/extensions/filters/network/dubbo_proxy/metadata.h @@ -8,7 +8,7 @@ #include "common/common/empty_string.h" #include "common/http/header_map_impl.h" -#include "extensions/filters/network/dubbo_proxy/protocol.h" +#include "extensions/filters/network/dubbo_proxy/message.h" #include "absl/types/optional.h" diff --git a/source/extensions/filters/network/dubbo_proxy/protocol.h b/source/extensions/filters/network/dubbo_proxy/protocol.h index a22c82dfd695a..6fbcb7aca96e0 100644 --- a/source/extensions/filters/network/dubbo_proxy/protocol.h +++ b/source/extensions/filters/network/dubbo_proxy/protocol.h @@ -11,6 +11,7 @@ #include "common/singleton/const_singleton.h" #include "extensions/filters/network/dubbo_proxy/message.h" +#include "extensions/filters/network/dubbo_proxy/metadata.h" namespace Envoy { namespace Extensions { @@ -69,6 +70,8 @@ class Protocol { struct Context { bool is_request_ = false; size_t body_size_ = 0; + size_t header_size_ = 0; + bool is_heartbeat_ = false; }; virtual ~Protocol() {} Protocol() {} @@ -80,6 +83,10 @@ class Protocol { virtual ProtocolType type() const PURE; /* + * This interface will be deprecated, + * it is reserved for the purpose of compatibility with the existing Filter implementation, + * this interface will be deleted after the new Filter implementation code is submitted. + * * decodes the dubbo protocol message, potentially invoking callbacks. * If successful, the message is removed from the buffer. * @@ -90,6 +97,30 @@ class Protocol { * @throws EnvoyException if the data is not valid for this protocol. */ virtual bool decode(Buffer::Instance& buffer, Context* context) PURE; + + /* + * decodes the dubbo protocol message, potentially invoking callbacks. + * If successful, the message is removed from the buffer. + * + * @param buffer the currently buffered dubbo data. + * @param context save the meta data of current messages. + * @param metadata the meta data of current messages + * @return bool true if a complete message was successfully consumed, false if more data + * is required. + * @throws EnvoyException if the data is not valid for this protocol. + */ + virtual bool decode(Buffer::Instance& buffer, Context* context, + MessageMetadataSharedPtr metadata) PURE; + + /* + * encodes the dubbo protocol message. + * + * @param buffer save the currently buffered dubbo data. + * @param metadata the meta data of dubbo protocol + * @return bool true if the protocol coding succeeds. + */ + virtual bool encode(Buffer::Instance& buffer, int32_t body_size, + const MessageMetadata& metadata) PURE; }; typedef std::unique_ptr ProtocolPtr; @@ -103,12 +134,22 @@ class NamedProtocolConfigFactory { virtual ~NamedProtocolConfigFactory() {} /** + * This interface will be deprecated, + * it is reserved for the purpose of compatibility with the existing Filter implementation, + * this interface will be deleted after the new Filter implementation code is submitted. + * * Create a particular Dubbo protocol. * @param callbacks the callbacks to be notified of protocol decodes. * @return protocol instance pointer. */ virtual ProtocolPtr createProtocol(ProtocolCallbacks& callbacks) PURE; + /** + * Create a particular Dubbo protocol. + * @return protocol instance pointer. + */ + virtual ProtocolPtr createProtocol() PURE; + /** * @return std::string the identifying name for a particular implementation of Dubbo protocol * produced by the factory. @@ -131,9 +172,11 @@ class NamedProtocolConfigFactory { */ template class ProtocolFactoryBase : public NamedProtocolConfigFactory { ProtocolPtr createProtocol(ProtocolCallbacks& callbacks) override { - return std::make_unique(callbacks); + return std::make_unique(&callbacks); } + ProtocolPtr createProtocol() override { return std::make_unique(); } + std::string name() override { return name_; } protected: diff --git a/source/extensions/filters/network/dubbo_proxy/router/BUILD b/source/extensions/filters/network/dubbo_proxy/router/BUILD index 674713cb072e0..6f09745996167 100644 --- a/source/extensions/filters/network/dubbo_proxy/router/BUILD +++ b/source/extensions/filters/network/dubbo_proxy/router/BUILD @@ -32,3 +32,40 @@ envoy_cc_library( "@envoy_api//envoy/config/filter/network/dubbo_proxy/v2alpha1:dubbo_proxy_cc", ], ) + +envoy_cc_library( + name = "config", + srcs = ["config.cc"], + hdrs = ["config.h"], + deps = [ + ":router_lib", + ":router_matcher", + "//include/envoy/registry", + "//source/extensions/filters/network/dubbo_proxy/filters:factory_base_lib", + "//source/extensions/filters/network/dubbo_proxy/filters:filter_config_interface", + "//source/extensions/filters/network/dubbo_proxy/filters:well_known_names", + "@envoy_api//envoy/config/filter/dubbo/router/v2alpha1:router_cc", + ], +) + +envoy_cc_library( + name = "router_lib", + srcs = ["router_impl.cc"], + hdrs = ["router_impl.h"], + deps = [ + ":router_interface", + "//include/envoy/tcp:conn_pool_interface", + "//include/envoy/upstream:cluster_manager_interface", + "//include/envoy/upstream:load_balancer_interface", + "//include/envoy/upstream:thread_local_cluster_interface", + "//source/common/common:logger_lib", + "//source/common/http:header_utility_lib", + "//source/common/router:metadatamatchcriteria_lib", + "//source/common/upstream:load_balancer_lib", + "//source/extensions/filters/network/dubbo_proxy:app_exception_lib", + "//source/extensions/filters/network/dubbo_proxy:deserializer_interface", + "//source/extensions/filters/network/dubbo_proxy:protocol_interface", + "//source/extensions/filters/network/dubbo_proxy/filters:filter_interface", + "@envoy_api//envoy/config/filter/network/dubbo_proxy/v2alpha1:dubbo_proxy_cc", + ], +) diff --git a/source/extensions/filters/network/dubbo_proxy/router/config.cc b/source/extensions/filters/network/dubbo_proxy/router/config.cc new file mode 100644 index 0000000000000..fbb40dcb12b73 --- /dev/null +++ b/source/extensions/filters/network/dubbo_proxy/router/config.cc @@ -0,0 +1,34 @@ +#include "extensions/filters/network/dubbo_proxy/router/config.h" + +#include "envoy/registry/registry.h" + +#include "extensions/filters/network/dubbo_proxy/router/router_impl.h" + +namespace Envoy { +namespace Extensions { +namespace NetworkFilters { +namespace DubboProxy { +namespace Router { + +DubboFilters::FilterFactoryCb RouterFilterConfig::createFilterFactoryFromProtoTyped( + const envoy::config::filter::dubbo::router::v2alpha1::Router& proto_config, + const std::string& stat_prefix, Server::Configuration::FactoryContext& context) { + UNREFERENCED_PARAMETER(proto_config); + UNREFERENCED_PARAMETER(stat_prefix); + + return [&context](DubboFilters::FilterChainFactoryCallbacks& callbacks) -> void { + callbacks.addDecoderFilter(std::make_shared(context.clusterManager())); + }; +} + +/** + * Static registration for the router filter. @see RegisterFactory. + */ +static Registry::RegisterFactory + register_; + +} // namespace Router +} // namespace DubboProxy +} // namespace NetworkFilters +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/filters/network/dubbo_proxy/router/config.h b/source/extensions/filters/network/dubbo_proxy/router/config.h new file mode 100644 index 0000000000000..88145b4e21946 --- /dev/null +++ b/source/extensions/filters/network/dubbo_proxy/router/config.h @@ -0,0 +1,30 @@ +#pragma once + +#include "envoy/config/filter/dubbo/router/v2alpha1/router.pb.h" +#include "envoy/config/filter/dubbo/router/v2alpha1/router.pb.validate.h" + +#include "extensions/filters/network/dubbo_proxy/filters/factory_base.h" +#include "extensions/filters/network/dubbo_proxy/filters/well_known_names.h" + +namespace Envoy { +namespace Extensions { +namespace NetworkFilters { +namespace DubboProxy { +namespace Router { + +class RouterFilterConfig + : public DubboFilters::FactoryBase { +public: + RouterFilterConfig() : FactoryBase(DubboFilters::DubboFilterNames::get().ROUTER) {} + +private: + DubboFilters::FilterFactoryCb createFilterFactoryFromProtoTyped( + const envoy::config::filter::dubbo::router::v2alpha1::Router& proto_config, + const std::string& stat_prefix, Server::Configuration::FactoryContext& context) override; +}; + +} // namespace Router +} // namespace DubboProxy +} // namespace NetworkFilters +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc b/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc new file mode 100644 index 0000000000000..f2501dafdb479 --- /dev/null +++ b/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc @@ -0,0 +1,324 @@ +#include "extensions/filters/network/dubbo_proxy/router/router_impl.h" + +#include "envoy/config/filter/network/dubbo_proxy/v2alpha1/dubbo_proxy.pb.h" +#include "envoy/upstream/cluster_manager.h" +#include "envoy/upstream/thread_local_cluster.h" + +#include "extensions/filters/network/dubbo_proxy/app_exception.h" + +namespace Envoy { +namespace Extensions { +namespace NetworkFilters { +namespace DubboProxy { +namespace Router { + +void Router::onDestroy() { + if (upstream_request_) { + upstream_request_->resetStream(); + } + cleanup(); +} + +void Router::setDecoderFilterCallbacks(DubboFilters::DecoderFilterCallbacks& callbacks) { + callbacks_ = &callbacks; +} + +Network::FilterStatus Router::transportBegin() { + upstream_request_buffer_.drain(upstream_request_buffer_.length()); + ProtocolDataPassthroughConverter::initProtocolConverter(upstream_request_buffer_); + return Network::FilterStatus::Continue; +} + +Network::FilterStatus Router::transportEnd() { + ASSERT(upstream_request_); + + upstream_request_->encodeData(upstream_request_buffer_, true); + + if (upstream_request_->metadata_->message_type() == MessageType::Oneway) { + // No response expected + upstream_request_->onResponseComplete(); + cleanup(); + } + + return Network::FilterStatus::Continue; +} + +Network::FilterStatus Router::messageBegin(MessageType, int64_t, SerializationType) { + return Network::FilterStatus::Continue; +} + +Network::FilterStatus Router::messageEnd(MessageMetadataSharedPtr metadata) { + route_ = callbacks_->route(); + if (!route_) { + ENVOY_STREAM_LOG(debug, "dubbo router: no cluster match for interface '{}'", *callbacks_, + metadata->service_name()); + callbacks_->sendLocalReply(AppException(AppExceptionType::ServiceNotFound, + fmt::format("dubbo router: no route for interface '{}'", + metadata->service_name())), + true); + return Network::FilterStatus::StopIteration; + } + + route_entry_ = route_->routeEntry(); + + Upstream::ThreadLocalCluster* cluster = cluster_manager_.get(route_entry_->clusterName()); + if (!cluster) { + ENVOY_STREAM_LOG(debug, "dubbo router: unknown cluster '{}'", *callbacks_, + route_entry_->clusterName()); + callbacks_->sendLocalReply(AppException(AppExceptionType::ServerError, + fmt::format("dubbo router: unknown cluster '{}'", + route_entry_->clusterName())), + true); + return Network::FilterStatus::StopIteration; + } + + cluster_ = cluster->info(); + ENVOY_STREAM_LOG(debug, "dubbo router: cluster '{}' match for interface '{}'", *callbacks_, + route_entry_->clusterName(), metadata->service_name()); + + if (cluster_->maintenanceMode()) { + callbacks_->sendLocalReply( + AppException(AppExceptionType::ServerError, + fmt::format("dubbo router: maintenance mode for cluster '{}'", + route_entry_->clusterName())), + true); + return Network::FilterStatus::StopIteration; + } + + Tcp::ConnectionPool::Instance* conn_pool = cluster_manager_.tcpConnPoolForCluster( + route_entry_->clusterName(), Upstream::ResourcePriority::Default, this, nullptr); + if (!conn_pool) { + callbacks_->sendLocalReply( + AppException( + AppExceptionType::ServerError, + fmt::format("dubbo router: no healthy upstream for '{}'", route_entry_->clusterName())), + true); + return Network::FilterStatus::StopIteration; + } + + ENVOY_STREAM_LOG(debug, "dubbo router: decoding request", *callbacks_); + + upstream_request_.reset(new UpstreamRequest(*this, *conn_pool, metadata, + callbacks_->downstreamSerializationType(), + callbacks_->downstreamProtocolType())); + return upstream_request_->start(); +} + +void Router::onUpstreamData(Buffer::Instance& data, bool end_stream) { + ASSERT(!upstream_request_->response_complete_); + + ENVOY_STREAM_LOG(trace, "dubbo router: reading response: {} bytes", *callbacks_, data.length()); + + // Handle normal response. + if (!upstream_request_->response_started_) { + callbacks_->startUpstreamResponse(*upstream_request_->deserializer_.get(), + *upstream_request_->protocol_.get()); + upstream_request_->response_started_ = true; + } + + DubboFilters::UpstreamResponseStatus status = callbacks_->upstreamData(data); + if (status == DubboFilters::UpstreamResponseStatus::Complete) { + ENVOY_STREAM_LOG(debug, "dubbo router: response complete", *callbacks_); + upstream_request_->onResponseComplete(); + cleanup(); + return; + } else if (status == DubboFilters::UpstreamResponseStatus::Reset) { + ENVOY_STREAM_LOG(debug, "dubbo router: upstream reset", *callbacks_); + upstream_request_->resetStream(); + return; + } + + if (end_stream) { + // Response is incomplete, but no more data is coming. + ENVOY_STREAM_LOG(debug, "dubbo router: response underflow", *callbacks_); + upstream_request_->onResponseComplete(); + upstream_request_->onResetStream( + Tcp::ConnectionPool::PoolFailureReason::RemoteConnectionFailure); + cleanup(); + } +} + +void Router::onEvent(Network::ConnectionEvent event) { + if (!upstream_request_ || upstream_request_->response_complete_) { + // Client closed connection after completing response. + return; + } + + switch (event) { + case Network::ConnectionEvent::RemoteClose: + upstream_request_->onResetStream( + Tcp::ConnectionPool::PoolFailureReason::RemoteConnectionFailure); + break; + case Network::ConnectionEvent::LocalClose: + upstream_request_->onResetStream( + Tcp::ConnectionPool::PoolFailureReason::LocalConnectionFailure); + break; + default: + // Connected is consumed by the connection pool. + NOT_REACHED_GCOVR_EXCL_LINE; + } +} + +const Network::Connection* Router::downstreamConnection() const { + return callbacks_ != nullptr ? callbacks_->connection() : nullptr; +} + +void Router::cleanup() { + if (upstream_request_) { + upstream_request_.reset(); + } +} + +Router::UpstreamRequest::UpstreamRequest(Router& parent, Tcp::ConnectionPool::Instance& pool, + MessageMetadataSharedPtr& metadata, + SerializationType serialization_type, + ProtocolType protocol_type) + : parent_(parent), conn_pool_(pool), metadata_(metadata), + deserializer_( + NamedDeserializerConfigFactory::getFactory(serialization_type).createDeserializer()), + protocol_(NamedProtocolConfigFactory::getFactory(protocol_type).createProtocol()), + request_complete_(false), response_started_(false), response_complete_(false) {} + +Router::UpstreamRequest::~UpstreamRequest() {} + +Network::FilterStatus Router::UpstreamRequest::start() { + Tcp::ConnectionPool::Cancellable* handle = conn_pool_.newConnection(*this); + if (handle) { + // Pause while we wait for a connection. + conn_pool_handle_ = handle; + return Network::FilterStatus::StopIteration; + } + + return Network::FilterStatus::Continue; +} + +void Router::UpstreamRequest::resetStream() { + if (conn_pool_handle_) { + conn_pool_handle_->cancel(Tcp::ConnectionPool::CancelPolicy::Default); + } + + if (conn_data_ != nullptr) { + conn_data_->connection().close(Network::ConnectionCloseType::NoFlush); + conn_data_.reset(); + } +} + +void Router::UpstreamRequest::encodeData(Buffer::Instance& data, bool end_stream) { + if (!conn_data_) { + ENVOY_STREAM_LOG(trace, "buffering {} bytes", *parent_.callbacks_, data.length()); + if (!buffered_request_body_) { + buffered_request_body_ = std::make_unique(); + } + + buffered_request_body_->move(data); + } else { + ENVOY_STREAM_LOG(trace, "proxying {} bytes", *parent_.callbacks_, data.length()); + conn_data_->connection().write(data, false); + if (end_stream) { + parent_.callbacks_->streamInfo().onLastUpstreamTxByteSent(); + } + } +} + +void Router::UpstreamRequest::onPoolFailure(Tcp::ConnectionPool::PoolFailureReason reason, + Upstream::HostDescriptionConstSharedPtr host) { + ENVOY_LOG(warn, "dubbo upstream request: connection failure '{}'", host->address()->asString()); + + conn_pool_handle_ = nullptr; + + // Mimic an upstream reset. + onUpstreamHostSelected(host); + onResetStream(reason); + + conn_pool_handle_ = nullptr; + parent_.upstream_request_buffer_.drain(parent_.upstream_request_buffer_.length()); +} + +void Router::UpstreamRequest::onPoolReady(Tcp::ConnectionPool::ConnectionDataPtr&& conn_data, + Upstream::HostDescriptionConstSharedPtr host) { + ENVOY_LOG(debug, "dubbo upstream request: tcp connection has ready"); + + // Only invoke continueDecoding if we'd previously stopped the filter chain. + bool continue_decoding = conn_pool_handle_ != nullptr; + + onUpstreamHostSelected(host); + conn_data_ = std::move(conn_data); + conn_data_->addUpstreamCallbacks(parent_); + conn_pool_handle_ = nullptr; + + onRequestStart(continue_decoding); +} + +void Router::UpstreamRequest::onRequestStart(bool continue_decoding) { + ENVOY_LOG(debug, "dubbo upstream request: start sending data to the server {}", + upstream_host_->address()->asString()); + + if (buffered_request_body_) { + conn_data_->connection().write(*buffered_request_body_, false); + parent_.callbacks_->streamInfo().onLastUpstreamTxByteSent(); + } + + if (continue_decoding) { + parent_.callbacks_->continueDecoding(); + } + onRequestComplete(); +} + +void Router::UpstreamRequest::onRequestComplete() { request_complete_ = true; } + +void Router::UpstreamRequest::onResponseComplete() { + response_complete_ = true; + conn_data_.reset(); +} + +void Router::UpstreamRequest::onUpstreamHostSelected(Upstream::HostDescriptionConstSharedPtr host) { + ENVOY_LOG(debug, "dubbo upstream request: selected upstream {}", host->address()->asString()); + upstream_host_ = host; +} + +void Router::UpstreamRequest::onResetStream(Tcp::ConnectionPool::PoolFailureReason reason) { + if (metadata_->message_type() == MessageType::Oneway) { + // For oneway requests, we should not attempt a response. Reset the downstream to signal + // an error. + ENVOY_LOG(debug, "dubbo upstream request: the request is oneway, reset downstream connection"); + parent_.callbacks_->resetDownstreamConnection(); + return; + } + + switch (reason) { + case Tcp::ConnectionPool::PoolFailureReason::Overflow: + parent_.callbacks_->sendLocalReply( + AppException(AppExceptionType::ServerError, + fmt::format("dubbo upstream request: too many connections to '{}'", + upstream_host_->address()->asString())), + true); + break; + case Tcp::ConnectionPool::PoolFailureReason::LocalConnectionFailure: + // Should only happen if we closed the connection, due to an error condition, in which case + // we've already handled any possible downstream response. + parent_.callbacks_->resetDownstreamConnection(); + break; + case Tcp::ConnectionPool::PoolFailureReason::RemoteConnectionFailure: + case Tcp::ConnectionPool::PoolFailureReason::Timeout: + if (!response_started_) { + parent_.callbacks_->sendLocalReply( + AppException(AppExceptionType::ServerError, + fmt::format("dubbo upstream request: connection failure '{}'", + upstream_host_->address()->asString())), + true); + return; + } + + // Error occurred after a partial response, propagate the reset to the downstream. + parent_.callbacks_->resetDownstreamConnection(); + break; + default: + NOT_REACHED_GCOVR_EXCL_LINE; + } +} + +} // namespace Router +} // namespace DubboProxy +} // namespace NetworkFilters +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/filters/network/dubbo_proxy/router/router_impl.h b/source/extensions/filters/network/dubbo_proxy/router/router_impl.h new file mode 100644 index 0000000000000..069475ffce6ac --- /dev/null +++ b/source/extensions/filters/network/dubbo_proxy/router/router_impl.h @@ -0,0 +1,104 @@ +#pragma once + +#include +#include + +#include "envoy/buffer/buffer.h" +#include "envoy/tcp/conn_pool.h" + +#include "common/common/logger.h" +#include "common/upstream/load_balancer_impl.h" + +#include "extensions/filters/network/dubbo_proxy/filters/filter.h" +#include "extensions/filters/network/dubbo_proxy/router/router.h" + +namespace Envoy { +namespace Extensions { +namespace NetworkFilters { +namespace DubboProxy { +namespace Router { + +class Router : public Tcp::ConnectionPool::UpstreamCallbacks, + public Upstream::LoadBalancerContextBase, + public DubboFilters::DecoderFilter, + Logger::Loggable { +public: + Router(Upstream::ClusterManager& cluster_manager) : cluster_manager_(cluster_manager) {} + ~Router() {} + + // DubboFilters::DecoderFilter + void onDestroy() override; + void setDecoderFilterCallbacks(DubboFilters::DecoderFilterCallbacks& callbacks) override; + Network::FilterStatus transportBegin() override; + Network::FilterStatus transportEnd() override; + Network::FilterStatus messageBegin(MessageType type, int64_t message_id, + SerializationType serialization_type) override; + Network::FilterStatus messageEnd(MessageMetadataSharedPtr metadata) override; + + // Upstream::LoadBalancerContextBase + const Envoy::Router::MetadataMatchCriteria* metadataMatchCriteria() override { return nullptr; } + const Network::Connection* downstreamConnection() const override; + + // Tcp::ConnectionPool::UpstreamCallbacks + void onUpstreamData(Buffer::Instance& data, bool end_stream) override; + void onEvent(Network::ConnectionEvent event) override; + void onAboveWriteBufferHighWatermark() override {} + void onBelowWriteBufferLowWatermark() override {} + +private: + struct UpstreamRequest : public Tcp::ConnectionPool::Callbacks { + UpstreamRequest(Router& parent, Tcp::ConnectionPool::Instance& pool, + MessageMetadataSharedPtr& metadata, SerializationType serialization_type, + ProtocolType protocol_type); + ~UpstreamRequest(); + + Network::FilterStatus start(); + void resetStream(); + void encodeData(Buffer::Instance& data, bool end_stream); + + // Tcp::ConnectionPool::Callbacks + void onPoolFailure(Tcp::ConnectionPool::PoolFailureReason reason, + Upstream::HostDescriptionConstSharedPtr host) override; + void onPoolReady(Tcp::ConnectionPool::ConnectionDataPtr&& conn, + Upstream::HostDescriptionConstSharedPtr host) override; + + void onRequestStart(bool continue_decoding); + void onRequestComplete(); + void onResponseComplete(); + void onUpstreamHostSelected(Upstream::HostDescriptionConstSharedPtr host); + void onResetStream(Tcp::ConnectionPool::PoolFailureReason reason); + + Router& parent_; + Tcp::ConnectionPool::Instance& conn_pool_; + MessageMetadataSharedPtr metadata_; + + Tcp::ConnectionPool::Cancellable* conn_pool_handle_{}; + Tcp::ConnectionPool::ConnectionDataPtr conn_data_; + Upstream::HostDescriptionConstSharedPtr upstream_host_; + DeserializerPtr deserializer_; + ProtocolPtr protocol_; + Buffer::InstancePtr buffered_request_body_; + + bool request_complete_ : 1; + bool response_started_ : 1; + bool response_complete_ : 1; + }; + + void cleanup(); + + Upstream::ClusterManager& cluster_manager_; + + DubboFilters::DecoderFilterCallbacks* callbacks_{}; + RouteConstSharedPtr route_{}; + const RouteEntry* route_entry_{}; + Upstream::ClusterInfoConstSharedPtr cluster_; + + std::unique_ptr upstream_request_; + Envoy::Buffer::OwnedImpl upstream_request_buffer_; +}; + +} // namespace Router +} // namespace DubboProxy +} // namespace NetworkFilters +} // namespace Extensions +} // namespace Envoy diff --git a/test/extensions/filters/network/dubbo_proxy/BUILD b/test/extensions/filters/network/dubbo_proxy/BUILD index 0f77b98b12b9a..909ed48a64c34 100644 --- a/test/extensions/filters/network/dubbo_proxy/BUILD +++ b/test/extensions/filters/network/dubbo_proxy/BUILD @@ -118,3 +118,33 @@ envoy_extension_cc_test( "@envoy_api//envoy/config/filter/network/dubbo_proxy/v2alpha1:dubbo_proxy_cc", ], ) + +envoy_extension_cc_test( + name = "router_test", + srcs = ["router_test.cc"], + extension_name = "envoy.filters.network.dubbo_proxy", + deps = [ + ":mocks_lib", + "//source/extensions/filters/network/dubbo_proxy:app_exception_lib", + "//source/extensions/filters/network/dubbo_proxy:dubbo_protocol_impl_lib", + "//source/extensions/filters/network/dubbo_proxy:hessian_deserializer_impl_lib", + "//source/extensions/filters/network/dubbo_proxy:metadata_lib", + "//source/extensions/filters/network/dubbo_proxy/router:config", + "//test/mocks/server:server_mocks", + "//test/test_common:registry_lib", + ], +) + +envoy_extension_cc_test( + name = "app_exception_test", + srcs = ["app_exception_test.cc"], + extension_name = "envoy.filters.network.dubbo_proxy", + deps = [ + ":mocks_lib", + ":utility_lib", + "//source/extensions/filters/network/dubbo_proxy:app_exception_lib", + "//source/extensions/filters/network/dubbo_proxy:dubbo_protocol_impl_lib", + "//source/extensions/filters/network/dubbo_proxy:hessian_deserializer_impl_lib", + "//source/extensions/filters/network/dubbo_proxy:metadata_lib", + ], +) diff --git a/test/extensions/filters/network/dubbo_proxy/app_exception_test.cc b/test/extensions/filters/network/dubbo_proxy/app_exception_test.cc new file mode 100644 index 0000000000000..1987d2b20e088 --- /dev/null +++ b/test/extensions/filters/network/dubbo_proxy/app_exception_test.cc @@ -0,0 +1,84 @@ +#include "extensions/filters/network/dubbo_proxy/app_exception.h" +#include "extensions/filters/network/dubbo_proxy/deserializer_impl.h" +#include "extensions/filters/network/dubbo_proxy/dubbo_protocol_impl.h" +#include "extensions/filters/network/dubbo_proxy/filters/filter.h" +#include "extensions/filters/network/dubbo_proxy/hessian_deserializer_impl.h" +#include "extensions/filters/network/dubbo_proxy/metadata.h" + +#include "test/extensions/filters/network/dubbo_proxy/mocks.h" +#include "test/test_common/test_base.h" + +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +namespace Envoy { +namespace Extensions { +namespace NetworkFilters { +namespace DubboProxy { + +class AppExceptionTest : public TestBase { +public: + AppExceptionTest() : metadata_(std::make_shared()) {} + + HessianDeserializerImpl deserializer_; + DubboProtocolImpl protocol_; + MessageMetadataSharedPtr metadata_; + Protocol::Context context_; +}; + +TEST_F(AppExceptionTest, Encode) { + std::string mock_message("invalid method name 'Sub'"); + AppException app_exception(AppExceptionType::ServiceNotFound, mock_message); + + Buffer::OwnedImpl buffer; + metadata_->setSerializationType(SerializationType::Hessian); + metadata_->setRequestId(0); + + { + EXPECT_EQ(app_exception.encode(*(metadata_.get()), protocol_, deserializer_, buffer), + DubboFilters::DirectResponse::ResponseType::Exception); + + MessageMetadataSharedPtr metadata = std::make_shared(); + EXPECT_TRUE(protocol_.decode(buffer, &context_, metadata)); + EXPECT_EQ(metadata->message_type(), MessageType::Response); + buffer.drain(context_.header_size_); + + auto result = deserializer_.deserializeRpcResult(buffer, context_.body_size_); + EXPECT_TRUE(result->hasException()); + buffer.drain(buffer.length()); + } + + { + app_exception.response_type_ = RpcResponseType::ResponseValueWithAttachments; + EXPECT_EQ(app_exception.encode(*(metadata_.get()), protocol_, deserializer_, buffer), + DubboFilters::DirectResponse::ResponseType::Exception); + + MessageMetadataSharedPtr metadata = std::make_shared(); + EXPECT_TRUE(protocol_.decode(buffer, &context_, metadata)); + EXPECT_EQ(metadata->message_type(), MessageType::Response); + buffer.drain(context_.header_size_); + + auto result = deserializer_.deserializeRpcResult(buffer, context_.body_size_); + EXPECT_FALSE(result->hasException()); + buffer.drain(buffer.length()); + } + + { + app_exception.response_type_ = RpcResponseType::ResponseWithValue; + EXPECT_EQ(app_exception.encode(*(metadata_.get()), protocol_, deserializer_, buffer), + DubboFilters::DirectResponse::ResponseType::Exception); + + MessageMetadataSharedPtr metadata = std::make_shared(); + EXPECT_TRUE(protocol_.decode(buffer, &context_, metadata)); + EXPECT_EQ(metadata->message_type(), MessageType::Response); + buffer.drain(context_.header_size_); + + auto result = deserializer_.deserializeRpcResult(buffer, context_.body_size_); + EXPECT_TRUE(result->hasException()); + } +} + +} // namespace DubboProxy +} // namespace NetworkFilters +} // namespace Extensions +} // namespace Envoy diff --git a/test/extensions/filters/network/dubbo_proxy/dubbo_protocol_impl_test.cc b/test/extensions/filters/network/dubbo_proxy/dubbo_protocol_impl_test.cc index 44ba352e6e003..ded61a113f46e 100644 --- a/test/extensions/filters/network/dubbo_proxy/dubbo_protocol_impl_test.cc +++ b/test/extensions/filters/network/dubbo_proxy/dubbo_protocol_impl_test.cc @@ -19,7 +19,7 @@ using testing::StrictMock; TEST(DubboProtocolImplTest, NotEnoughData) { Buffer::OwnedImpl buffer; MockProtocolCallbacks cb; - DubboProtocolImpl dubbo_protocol(cb); + DubboProtocolImpl dubbo_protocol(&cb); Protocol::Context context; EXPECT_FALSE(dubbo_protocol.decode(buffer, &context)); buffer.add(std::string(15, 0x00)); @@ -28,13 +28,13 @@ TEST(DubboProtocolImplTest, NotEnoughData) { TEST(DubboProtocolImplTest, Name) { MockProtocolCallbacks cb; - DubboProtocolImpl dubbo_protocol(cb); + DubboProtocolImpl dubbo_protocol(&cb); EXPECT_EQ(dubbo_protocol.name(), "dubbo"); } TEST(DubboProtocolImplTest, Normal) { MockProtocolCallbacks cb; - DubboProtocolImpl dubbo_protocol(cb); + DubboProtocolImpl dubbo_protocol(&cb); // Normal dubbo request message { Buffer::OwnedImpl buffer; @@ -64,7 +64,7 @@ TEST(DubboProtocolImplTest, Normal) { TEST(DubboProtocolImplTest, InvalidProtocol) { MockProtocolCallbacks cb; - DubboProtocolImpl dubbo_protocol(cb); + DubboProtocolImpl dubbo_protocol(&cb); Protocol::Context context; // Invalid dubbo magic number diff --git a/test/extensions/filters/network/dubbo_proxy/mocks.cc b/test/extensions/filters/network/dubbo_proxy/mocks.cc index 2075536058f46..b12f2d6ec8e5b 100644 --- a/test/extensions/filters/network/dubbo_proxy/mocks.cc +++ b/test/extensions/filters/network/dubbo_proxy/mocks.cc @@ -28,9 +28,18 @@ MockDecoderEventHandler::MockDecoderEventHandler() { MockProtocolCallbacks::MockProtocolCallbacks() {} MockProtocolCallbacks::~MockProtocolCallbacks() {} -MockProtocol::MockProtocol() { ON_CALL(*this, name()).WillByDefault(ReturnRef(name_)); } +MockProtocol::MockProtocol() { + ON_CALL(*this, name()).WillByDefault(ReturnRef(name_)); + ON_CALL(*this, type()).WillByDefault(Return(type_)); +} MockProtocol::~MockProtocol() {} +MockDeserializer::MockDeserializer() { + ON_CALL(*this, name()).WillByDefault(ReturnRef(name_)); + ON_CALL(*this, type()).WillByDefault(Return(type_)); +} +MockDeserializer::~MockDeserializer() {} + namespace DubboFilters { MockFilterChainFactoryCallbacks::MockFilterChainFactoryCallbacks() {} diff --git a/test/extensions/filters/network/dubbo_proxy/mocks.h b/test/extensions/filters/network/dubbo_proxy/mocks.h index 3354a279cc6fd..bbcef29ad29e4 100644 --- a/test/extensions/filters/network/dubbo_proxy/mocks.h +++ b/test/extensions/filters/network/dubbo_proxy/mocks.h @@ -50,8 +50,29 @@ class MockProtocol : public Protocol { ~MockProtocol(); MOCK_CONST_METHOD0(name, const std::string&()); + MOCK_CONST_METHOD0(type, ProtocolType()); MOCK_METHOD2(decode, bool(Buffer::Instance&, Context*)); + MOCK_METHOD3(decode, bool(Buffer::Instance&, Protocol::Context*, MessageMetadataSharedPtr)); + MOCK_METHOD3(encode, bool(Buffer::Instance&, int32_t, const MessageMetadata&)); + std::string name_{"MockProtocol"}; + ProtocolType type_{ProtocolType::Dubbo}; +}; + +class MockDeserializer : public Deserializer { +public: + MockDeserializer(); + ~MockDeserializer(); + + // DubboProxy::Deserializer + MOCK_CONST_METHOD0(name, const std::string&()); + MOCK_CONST_METHOD0(type, SerializationType()); + MOCK_METHOD2(deserializeRpcInvocation, RpcInvocationPtr(Buffer::Instance&, size_t)); + MOCK_METHOD2(deserializeRpcResult, RpcResultPtr(Buffer::Instance&, size_t)); + MOCK_METHOD3(serializeRpcResult, void(Buffer::Instance&, const std::string&, RpcResponseType)); + + std::string name_{"mockDeserializer"}; + SerializationType type_{SerializationType::Hessian}; }; namespace Router { diff --git a/test/extensions/filters/network/dubbo_proxy/router_test.cc b/test/extensions/filters/network/dubbo_proxy/router_test.cc new file mode 100644 index 0000000000000..f6028a50e2452 --- /dev/null +++ b/test/extensions/filters/network/dubbo_proxy/router_test.cc @@ -0,0 +1,462 @@ +#include "extensions/filters/network/dubbo_proxy/app_exception.h" +#include "extensions/filters/network/dubbo_proxy/deserializer.h" +#include "extensions/filters/network/dubbo_proxy/protocol.h" +#include "extensions/filters/network/dubbo_proxy/router/router_impl.h" + +#include "test/extensions/filters/network/dubbo_proxy/mocks.h" +#include "test/mocks/network/mocks.h" +#include "test/mocks/server/mocks.h" +#include "test/test_common/printers.h" +#include "test/test_common/registry.h" +#include "test/test_common/test_base.h" + +#include "gtest/gtest.h" + +using testing::_; +using testing::ContainsRegex; +using testing::InSequence; +using testing::Invoke; +using testing::NiceMock; +using testing::Ref; +using testing::Return; +using testing::ReturnRef; +using testing::Test; +using testing::TestWithParam; +using testing::Values; + +namespace Envoy { +namespace Extensions { +namespace NetworkFilters { +namespace DubboProxy { +namespace Router { + +namespace { + +class TestNamedDeserializerConfigFactory : public NamedDeserializerConfigFactory { +public: + TestNamedDeserializerConfigFactory(std::function f) : f_(f) {} + + DeserializerPtr createDeserializer() override { return DeserializerPtr{f_()}; } + std::string name() override { + return DeserializerNames::get().fromType(SerializationType::Hessian); + } + + std::function f_; +}; + +class TestNamedProtocolConfigFactory : public NamedProtocolConfigFactory { +public: + TestNamedProtocolConfigFactory(std::function f) : f_(f) {} + + ProtocolPtr createProtocol(ProtocolCallbacks&) override { return ProtocolPtr{f_()}; } + ProtocolPtr createProtocol() override { return ProtocolPtr{f_()}; } + std::string name() override { return ProtocolNames::get().fromType(ProtocolType::Dubbo); } + + std::function f_; +}; + +} // namespace + +class RouterTestBase { +public: + RouterTestBase() + : deserializer_factory_([&]() -> MockDeserializer* { + ASSERT(deserializer_ == nullptr); + deserializer_ = new NiceMock(); + if (mock_deserializer_cb_) { + mock_deserializer_cb_(deserializer_); + } + return deserializer_; + }), + protocol_factory_([&]() -> MockProtocol* { + ASSERT(protocol_ == nullptr); + protocol_ = new NiceMock(); + if (mock_protocol_cb_) { + mock_protocol_cb_(protocol_); + } + return protocol_; + }), + deserializer_register_(deserializer_factory_), protocol_register_(protocol_factory_) {} + + void initializeRouter() { + route_ = new NiceMock(); + route_ptr_.reset(route_); + + router_ = std::make_unique(context_.clusterManager()); + + EXPECT_EQ(nullptr, router_->downstreamConnection()); + + router_->setDecoderFilterCallbacks(callbacks_); + } + + void initializeMetadata(MessageType msg_type) { + msg_type_ = msg_type; + + metadata_.reset(new MessageMetadata()); + metadata_->setServiceName("test"); + metadata_->setMessageType(msg_type_); + metadata_->setRequestId(1); + } + + void startRequest(MessageType msg_type) { + initializeMetadata(msg_type); + + EXPECT_CALL(callbacks_, route()).WillOnce(Return(route_ptr_)); + EXPECT_CALL(*route_, routeEntry()).WillOnce(Return(&route_entry_)); + EXPECT_CALL(route_entry_, clusterName()).WillRepeatedly(ReturnRef(cluster_name_)); + + EXPECT_CALL(callbacks_, downstreamSerializationType()) + .WillOnce(Return(SerializationType::Hessian)); + EXPECT_CALL(callbacks_, downstreamProtocolType()).WillOnce(Return(ProtocolType::Dubbo)); + + EXPECT_EQ(Network::FilterStatus::StopIteration, router_->messageEnd(metadata_)); + + EXPECT_CALL(callbacks_, connection()).WillRepeatedly(Return(&connection_)); + EXPECT_EQ(&connection_, router_->downstreamConnection()); + + // Not yet implemented: + EXPECT_EQ(absl::optional(), router_->computeHashKey()); + EXPECT_EQ(nullptr, router_->metadataMatchCriteria()); + EXPECT_EQ(nullptr, router_->downstreamHeaders()); + } + + void connectUpstream() { + EXPECT_CALL(*context_.cluster_manager_.tcp_conn_pool_.connection_data_, addUpstreamCallbacks(_)) + .WillOnce(Invoke([&](Tcp::ConnectionPool::UpstreamCallbacks& cb) -> void { + upstream_callbacks_ = &cb; + })); + + conn_state_.reset(); + EXPECT_CALL(*context_.cluster_manager_.tcp_conn_pool_.connection_data_, connectionState()) + .WillRepeatedly( + Invoke([&]() -> Tcp::ConnectionPool::ConnectionState* { return conn_state_.get(); })); + + EXPECT_CALL(callbacks_, continueDecoding()); + context_.cluster_manager_.tcp_conn_pool_.poolReady(upstream_connection_); + + EXPECT_NE(nullptr, upstream_callbacks_); + } + + void startRequestWithExistingConnection(MessageType msg_type) { + EXPECT_EQ(Network::FilterStatus::Continue, router_->transportBegin()); + + EXPECT_CALL(callbacks_, route()).WillOnce(Return(route_ptr_)); + EXPECT_CALL(*route_, routeEntry()).WillOnce(Return(&route_entry_)); + EXPECT_CALL(route_entry_, clusterName()).WillRepeatedly(ReturnRef(cluster_name_)); + + initializeMetadata(msg_type); + + EXPECT_CALL(*context_.cluster_manager_.tcp_conn_pool_.connection_data_, addUpstreamCallbacks(_)) + .WillOnce(Invoke([&](Tcp::ConnectionPool::UpstreamCallbacks& cb) -> void { + upstream_callbacks_ = &cb; + })); + + EXPECT_CALL(callbacks_, connection()).WillRepeatedly(Return(&connection_)); + EXPECT_EQ(&connection_, router_->downstreamConnection()); + + // Not yet implemented: + EXPECT_EQ(absl::optional(), router_->computeHashKey()); + EXPECT_EQ(nullptr, router_->metadataMatchCriteria()); + EXPECT_EQ(nullptr, router_->downstreamHeaders()); + + EXPECT_CALL(callbacks_, downstreamSerializationType()) + .WillOnce(Return(SerializationType::Hessian)); + EXPECT_CALL(callbacks_, downstreamProtocolType()).WillOnce(Return(ProtocolType::Dubbo)); + + EXPECT_CALL(callbacks_, continueDecoding()).Times(0); + EXPECT_CALL(context_.cluster_manager_.tcp_conn_pool_, newConnection(_)) + .WillOnce( + Invoke([&](Tcp::ConnectionPool::Callbacks& cb) -> Tcp::ConnectionPool::Cancellable* { + context_.cluster_manager_.tcp_conn_pool_.newConnectionImpl(cb); + context_.cluster_manager_.tcp_conn_pool_.poolReady(upstream_connection_); + return nullptr; + })); + + EXPECT_EQ(Network::FilterStatus::Continue, router_->transportBegin()); + EXPECT_NE(nullptr, upstream_callbacks_); + } + + void returnResponse() { + Buffer::OwnedImpl buffer; + + EXPECT_CALL(callbacks_, startUpstreamResponse(_, _)); + + EXPECT_CALL(callbacks_, upstreamData(Ref(buffer))) + .WillOnce(Return(DubboFilters::UpstreamResponseStatus::MoreData)); + upstream_callbacks_->onUpstreamData(buffer, false); + + EXPECT_CALL(callbacks_, upstreamData(Ref(buffer))) + .WillOnce(Return(DubboFilters::UpstreamResponseStatus::Complete)); + EXPECT_CALL(context_.cluster_manager_.tcp_conn_pool_, released(Ref(upstream_connection_))); + upstream_callbacks_->onUpstreamData(buffer, false); + } + + void destroyRouter() { + router_->onDestroy(); + router_.reset(); + } + + TestNamedDeserializerConfigFactory deserializer_factory_; + TestNamedProtocolConfigFactory protocol_factory_; + Registry::InjectFactory deserializer_register_; + Registry::InjectFactory protocol_register_; + + std::function mock_deserializer_cb_{}; + std::function mock_protocol_cb_{}; + + NiceMock context_; + NiceMock connection_; + NiceMock callbacks_; + NiceMock* deserializer_{}; + NiceMock* protocol_{}; + NiceMock* route_{}; + NiceMock route_entry_; + NiceMock* host_{}; + Tcp::ConnectionPool::ConnectionStatePtr conn_state_; + + RouteConstSharedPtr route_ptr_; + std::unique_ptr router_; + + std::string cluster_name_{"cluster"}; + + MessageType msg_type_{MessageType::Request}; + MessageMetadataSharedPtr metadata_; + + Tcp::ConnectionPool::UpstreamCallbacks* upstream_callbacks_{}; + NiceMock upstream_connection_; +}; + +class RouterTest : public RouterTestBase, public TestBase { +public: + RouterTest() {} +}; + +TEST_F(RouterTest, PoolRemoteConnectionFailure) { + initializeRouter(); + + EXPECT_CALL(callbacks_, sendLocalReply(_, _)) + .WillOnce(Invoke([&](const DubboFilters::DirectResponse& response, bool end_stream) -> void { + auto& app_ex = dynamic_cast(response); + EXPECT_EQ(AppExceptionType::ServerError, app_ex.type_); + EXPECT_THAT(app_ex.what(), ContainsRegex(".*connection failure.*")); + EXPECT_TRUE(end_stream); + })); + startRequest(MessageType::Request); + + context_.cluster_manager_.tcp_conn_pool_.poolFailure( + Tcp::ConnectionPool::PoolFailureReason::RemoteConnectionFailure); +} + +TEST_F(RouterTest, PoolTimeout) { + initializeRouter(); + + EXPECT_CALL(callbacks_, sendLocalReply(_, _)) + .WillOnce(Invoke([&](const DubboFilters::DirectResponse& response, bool end_stream) -> void { + auto& app_ex = dynamic_cast(response); + EXPECT_EQ(AppExceptionType::ServerError, app_ex.type_); + EXPECT_THAT(app_ex.what(), ContainsRegex(".*connection failure.*")); + EXPECT_TRUE(end_stream); + })); + startRequest(MessageType::Request); + + context_.cluster_manager_.tcp_conn_pool_.poolFailure( + Tcp::ConnectionPool::PoolFailureReason::Timeout); +} + +TEST_F(RouterTest, PoolOverflowFailure) { + initializeRouter(); + + EXPECT_CALL(callbacks_, sendLocalReply(_, _)) + .WillOnce(Invoke([&](const DubboFilters::DirectResponse& response, bool end_stream) -> void { + auto& app_ex = dynamic_cast(response); + EXPECT_EQ(AppExceptionType::ServerError, app_ex.type_); + EXPECT_THAT(app_ex.what(), ContainsRegex(".*too many connections.*")); + EXPECT_TRUE(end_stream); + })); + startRequest(MessageType::Request); + + context_.cluster_manager_.tcp_conn_pool_.poolFailure( + Tcp::ConnectionPool::PoolFailureReason::Overflow); +} + +TEST_F(RouterTest, ClusterMaintenanceMode) { + initializeRouter(); + initializeMetadata(MessageType::Request); + + EXPECT_CALL(callbacks_, route()).WillOnce(Return(route_ptr_)); + EXPECT_CALL(*route_, routeEntry()).WillOnce(Return(&route_entry_)); + EXPECT_CALL(route_entry_, clusterName()).WillRepeatedly(ReturnRef(cluster_name_)); + EXPECT_CALL(*context_.cluster_manager_.thread_local_cluster_.cluster_.info_, maintenanceMode()) + .WillOnce(Return(true)); + + EXPECT_CALL(callbacks_, sendLocalReply(_, _)) + .WillOnce(Invoke([&](const DubboFilters::DirectResponse& response, bool end_stream) -> void { + auto& app_ex = dynamic_cast(response); + EXPECT_EQ(AppExceptionType::ServerError, app_ex.type_); + EXPECT_THAT(app_ex.what(), ContainsRegex(".*maintenance mode.*")); + EXPECT_TRUE(end_stream); + })); + EXPECT_EQ(Network::FilterStatus::StopIteration, router_->messageEnd(metadata_)); +} + +TEST_F(RouterTest, NoHealthyHosts) { + initializeRouter(); + initializeMetadata(MessageType::Request); + + EXPECT_CALL(callbacks_, route()).WillOnce(Return(route_ptr_)); + EXPECT_CALL(*route_, routeEntry()).WillOnce(Return(&route_entry_)); + EXPECT_CALL(route_entry_, clusterName()).WillRepeatedly(ReturnRef(cluster_name_)); + EXPECT_CALL(context_.cluster_manager_, tcpConnPoolForCluster(cluster_name_, _, _, _)) + .WillOnce(Return(nullptr)); + + EXPECT_CALL(callbacks_, sendLocalReply(_, _)) + .WillOnce(Invoke([&](const DubboFilters::DirectResponse& response, bool end_stream) -> void { + auto& app_ex = dynamic_cast(response); + EXPECT_EQ(AppExceptionType::ServerError, app_ex.type_); + EXPECT_THAT(app_ex.what(), ContainsRegex(".*no healthy upstream.*")); + EXPECT_TRUE(end_stream); + })); + + EXPECT_EQ(Network::FilterStatus::StopIteration, router_->messageEnd(metadata_)); +} + +TEST_F(RouterTest, PoolConnectionFailureWithOnewayMessage) { + initializeRouter(); + initializeMetadata(MessageType::Oneway); + + EXPECT_CALL(callbacks_, downstreamSerializationType()) + .WillOnce(Return(SerializationType::Hessian)); + EXPECT_CALL(callbacks_, sendLocalReply(_, _)).Times(0); + EXPECT_CALL(callbacks_, resetDownstreamConnection()).Times(1); + EXPECT_EQ(Network::FilterStatus::StopIteration, router_->messageEnd(metadata_)); + + context_.cluster_manager_.tcp_conn_pool_.poolFailure( + Tcp::ConnectionPool::PoolFailureReason::RemoteConnectionFailure); + + destroyRouter(); +} + +TEST_F(RouterTest, NoRoute) { + initializeRouter(); + initializeMetadata(MessageType::Request); + + EXPECT_CALL(callbacks_, route()).WillOnce(Return(nullptr)); + EXPECT_CALL(callbacks_, sendLocalReply(_, _)) + .WillOnce(Invoke([&](const DubboFilters::DirectResponse& response, bool end_stream) -> void { + auto& app_ex = dynamic_cast(response); + EXPECT_EQ(AppExceptionType::ServiceNotFound, app_ex.type_); + EXPECT_THAT(app_ex.what(), ContainsRegex(".*no route.*")); + EXPECT_TRUE(end_stream); + })); + EXPECT_EQ(Network::FilterStatus::StopIteration, router_->messageEnd(metadata_)); +} + +TEST_F(RouterTest, NoCluster) { + initializeRouter(); + initializeMetadata(MessageType::Request); + + EXPECT_CALL(callbacks_, route()).WillOnce(Return(route_ptr_)); + EXPECT_CALL(*route_, routeEntry()).WillOnce(Return(&route_entry_)); + EXPECT_CALL(route_entry_, clusterName()).WillRepeatedly(ReturnRef(cluster_name_)); + EXPECT_CALL(context_.cluster_manager_, get(cluster_name_)).WillOnce(Return(nullptr)); + EXPECT_CALL(callbacks_, sendLocalReply(_, _)) + .WillOnce(Invoke([&](const DubboFilters::DirectResponse& response, bool end_stream) -> void { + auto& app_ex = dynamic_cast(response); + EXPECT_EQ(AppExceptionType::ServerError, app_ex.type_); + EXPECT_THAT(app_ex.what(), ContainsRegex(".*unknown cluster.*")); + EXPECT_TRUE(end_stream); + })); + EXPECT_EQ(Network::FilterStatus::StopIteration, router_->messageEnd(metadata_)); +} + +TEST_F(RouterTest, UnexpectedRouterDestroy) { + initializeRouter(); + initializeMetadata(MessageType::Request); + EXPECT_CALL(upstream_connection_, close(Network::ConnectionCloseType::NoFlush)); + startRequest(MessageType::Request); + + Buffer::OwnedImpl buffer; + buffer.add(std::string({'\xda', '\xbb', 0x42, 20})); + EXPECT_EQ(Network::FilterStatus::Continue, router_->transferHeaderTo(buffer, buffer.length())); + + connectUpstream(); + destroyRouter(); +} + +TEST_F(RouterTest, UpstreamRemoteCloseMidResponse) { + initializeRouter(); + + EXPECT_CALL(callbacks_, sendLocalReply(_, _)) + .WillOnce(Invoke([&](const DubboFilters::DirectResponse& response, bool end_stream) -> void { + auto& app_ex = dynamic_cast(response); + EXPECT_EQ(AppExceptionType::ServerError, app_ex.type_); + EXPECT_THAT(app_ex.what(), ContainsRegex(".*connection failure.*")); + EXPECT_TRUE(end_stream); + })); + startRequest(MessageType::Request); + connectUpstream(); + upstream_callbacks_->onEvent(Network::ConnectionEvent::RemoteClose); + destroyRouter(); +} + +TEST_F(RouterTest, UpstreamLocalCloseMidResponse) { + initializeRouter(); + startRequest(MessageType::Request); + connectUpstream(); + + upstream_callbacks_->onEvent(Network::ConnectionEvent::LocalClose); + destroyRouter(); +} + +TEST_F(RouterTest, OneWay) { + initializeRouter(); + initializeMetadata(MessageType::Oneway); + + EXPECT_CALL(context_.cluster_manager_.tcp_conn_pool_, released(Ref(upstream_connection_))); + + startRequest(MessageType::Oneway); + connectUpstream(); + + destroyRouter(); +} + +TEST_F(RouterTest, Call) { + initializeRouter(); + initializeMetadata(MessageType::Request); + + EXPECT_CALL(upstream_connection_, write(_, false)); + + startRequest(MessageType::Request); + connectUpstream(); + + EXPECT_EQ(Network::FilterStatus::Continue, router_->transportEnd()); + + returnResponse(); + destroyRouter(); +} + +TEST_F(RouterTest, DecoderFilterCallbacks) { + initializeRouter(); + initializeMetadata(MessageType::Request); + + EXPECT_CALL(upstream_connection_, write(_, false)); + EXPECT_CALL(callbacks_, startUpstreamResponse(_, _)).Times(1); + EXPECT_CALL(callbacks_, upstreamData(_)).Times(1); + + startRequest(MessageType::Request); + connectUpstream(); + + EXPECT_EQ(Network::FilterStatus::Continue, router_->transportEnd()); + + Buffer::OwnedImpl buffer; + buffer.add(std::string("This is the test data")); + router_->onUpstreamData(buffer, true); + + destroyRouter(); +} + +} // namespace Router +} // namespace DubboProxy +} // namespace NetworkFilters +} // namespace Extensions +} // namespace Envoy From 95dc2b58b967c2f35d3e30cad7cc0b0becc5ddd4 Mon Sep 17 00:00:00 2001 From: "leilei.gll" Date: Fri, 15 Feb 2019 15:08:12 +0800 Subject: [PATCH 02/23] Add missing files Signed-off-by: leilei.gll --- .../config/filter/dubbo/router/v2alpha1/BUILD | 8 ++++++++ .../filter/dubbo/router/v2alpha1/router.proto | 14 ++++++++++++++ 2 files changed, 22 insertions(+) create mode 100644 api/envoy/config/filter/dubbo/router/v2alpha1/BUILD create mode 100644 api/envoy/config/filter/dubbo/router/v2alpha1/router.proto diff --git a/api/envoy/config/filter/dubbo/router/v2alpha1/BUILD b/api/envoy/config/filter/dubbo/router/v2alpha1/BUILD new file mode 100644 index 0000000000000..ce0ad0e254f03 --- /dev/null +++ b/api/envoy/config/filter/dubbo/router/v2alpha1/BUILD @@ -0,0 +1,8 @@ +load("//bazel:api_build_system.bzl", "api_proto_library_internal") + +licenses(["notice"]) # Apache 2 + +api_proto_library_internal( + name = "router", + srcs = ["router.proto"], +) diff --git a/api/envoy/config/filter/dubbo/router/v2alpha1/router.proto b/api/envoy/config/filter/dubbo/router/v2alpha1/router.proto new file mode 100644 index 0000000000000..37a5542a17bbf --- /dev/null +++ b/api/envoy/config/filter/dubbo/router/v2alpha1/router.proto @@ -0,0 +1,14 @@ +syntax = "proto3"; + +package envoy.config.filter.dubbo.router.v2alpha1; + +option java_outer_classname = "RouterProto"; +option java_multiple_files = true; +option java_package = "io.envoyproxy.envoy.config.filter.dubbo.router.v2alpha1"; +option go_package = "v2alpha1"; + +// [#protodoc-title: Router] +// Dubbo router :ref:`configuration overview `. + +message Router { +} From 823230811056c2d1ea954ef39c11289fcafd5445 Mon Sep 17 00:00:00 2001 From: "leilei.gll" Date: Fri, 15 Feb 2019 15:21:23 +0800 Subject: [PATCH 03/23] Delete the testing::Test code Signed-off-by: leilei.gll --- test/extensions/filters/network/dubbo_proxy/router_test.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/extensions/filters/network/dubbo_proxy/router_test.cc b/test/extensions/filters/network/dubbo_proxy/router_test.cc index f6028a50e2452..7d209f0b83dd3 100644 --- a/test/extensions/filters/network/dubbo_proxy/router_test.cc +++ b/test/extensions/filters/network/dubbo_proxy/router_test.cc @@ -20,8 +20,6 @@ using testing::NiceMock; using testing::Ref; using testing::Return; using testing::ReturnRef; -using testing::Test; -using testing::TestWithParam; using testing::Values; namespace Envoy { From bd5a67dfa382f5e2eb49a22a8f81aac2af754123 Mon Sep 17 00:00:00 2001 From: "leilei.gll" Date: Fri, 15 Feb 2019 16:39:22 +0800 Subject: [PATCH 04/23] Fix formatting errors Signed-off-by: leilei.gll --- .../filters/network/dubbo_proxy/router/router_impl.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc b/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc index f2501dafdb479..59fe606ea89cb 100644 --- a/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc +++ b/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc @@ -98,9 +98,9 @@ Network::FilterStatus Router::messageEnd(MessageMetadataSharedPtr metadata) { ENVOY_STREAM_LOG(debug, "dubbo router: decoding request", *callbacks_); - upstream_request_.reset(new UpstreamRequest(*this, *conn_pool, metadata, - callbacks_->downstreamSerializationType(), - callbacks_->downstreamProtocolType())); + upstream_request_ = std::make_unique(*this, *conn_pool, metadata, + callbacks_->downstreamSerializationType(), + callbacks_->downstreamProtocolType()); return upstream_request_->start(); } From 48b834ed7ab53f82352fd5f6ef96f57f51cbfd98 Mon Sep 17 00:00:00 2001 From: "leilei.gll" Date: Fri, 15 Feb 2019 18:18:40 +0800 Subject: [PATCH 05/23] Remove the AppExceptionType definition and fixed a naming conflict for RouterTest Signed-off-by: leilei.gll --- .../filters/network/dubbo_proxy/BUILD | 2 - .../network/dubbo_proxy/app_exception.cc | 41 ++----------- .../network/dubbo_proxy/app_exception.h | 19 +----- .../network/dubbo_proxy/router/router_impl.cc | 22 +++---- .../network/dubbo_proxy/app_exception_test.cc | 2 +- .../network/dubbo_proxy/router_test.cc | 61 +++++++++---------- 6 files changed, 49 insertions(+), 98 deletions(-) diff --git a/source/extensions/filters/network/dubbo_proxy/BUILD b/source/extensions/filters/network/dubbo_proxy/BUILD index 828e4b6f9deaa..3fc0ab64ba6db 100644 --- a/source/extensions/filters/network/dubbo_proxy/BUILD +++ b/source/extensions/filters/network/dubbo_proxy/BUILD @@ -168,8 +168,6 @@ envoy_cc_library( hdrs = ["app_exception.h"], deps = [ ":deserializer_interface", - ":hessian_deserializer_impl_lib", - ":hessian_utils_lib", ":message_lib", ":metadata_lib", ":protocol_interface", diff --git a/source/extensions/filters/network/dubbo_proxy/app_exception.cc b/source/extensions/filters/network/dubbo_proxy/app_exception.cc index 2b87a1a8aeea2..bed47ba34cc63 100644 --- a/source/extensions/filters/network/dubbo_proxy/app_exception.cc +++ b/source/extensions/filters/network/dubbo_proxy/app_exception.cc @@ -11,10 +11,12 @@ namespace Extensions { namespace NetworkFilters { namespace DubboProxy { -AppException::AppException(AppExceptionType type, const std::string& what) - : EnvoyException(what), type_(type), response_type_(RpcResponseType::ResponseWithException) {} +AppException::AppException(ResponseStatus status, const std::string& what) + : EnvoyException(what), status_(status), + response_type_(RpcResponseType::ResponseWithException) {} -AppException::AppException(const AppException& ex) : EnvoyException(ex.what()), type_(ex.type_) {} +AppException::AppException(const AppException& ex) + : EnvoyException(ex.what()), status_(ex.status_) {} AppException::ResponseType AppException::encode(MessageMetadata& metadata, DubboProxy::Protocol& protocol, @@ -22,38 +24,7 @@ AppException::ResponseType AppException::encode(MessageMetadata& metadata, Buffer::Instance& buffer) const { ENVOY_LOG(debug, "err {}", what()); - switch (type_) { - case AppExceptionType::ClientTimeout: - metadata.setResponseStatus(ResponseStatus::ClientTimeout); - break; - case AppExceptionType::ServerTimeout: - metadata.setResponseStatus(ResponseStatus::ServerTimeout); - break; - case AppExceptionType::BadRequest: - metadata.setResponseStatus(ResponseStatus::BadRequest); - break; - case AppExceptionType::BadResponse: - metadata.setResponseStatus(ResponseStatus::BadResponse); - break; - case AppExceptionType::ServiceNotFound: - metadata.setResponseStatus(ResponseStatus::ServiceNotFound); - break; - case AppExceptionType::ServiceError: - metadata.setResponseStatus(ResponseStatus::ServiceError); - break; - case AppExceptionType::ServerError: - metadata.setResponseStatus(ResponseStatus::ServerError); - break; - case AppExceptionType::ClientError: - metadata.setResponseStatus(ResponseStatus::ClientError); - break; - case AppExceptionType::ServerThreadpoolExhaustedError: - metadata.setResponseStatus(ResponseStatus::ServerThreadpoolExhaustedError); - break; - default: - NOT_REACHED_GCOVR_EXCL_LINE - } - + metadata.setResponseStatus(status_); metadata.setMessageType(MessageType::Response); const std::string& response = what(); diff --git a/source/extensions/filters/network/dubbo_proxy/app_exception.h b/source/extensions/filters/network/dubbo_proxy/app_exception.h index d30c246edac42..977107069cd26 100644 --- a/source/extensions/filters/network/dubbo_proxy/app_exception.h +++ b/source/extensions/filters/network/dubbo_proxy/app_exception.h @@ -12,32 +12,17 @@ namespace Extensions { namespace NetworkFilters { namespace DubboProxy { -/** - * Dubbo Application Exception types. - */ -enum class AppExceptionType { - ClientTimeout, - ServerTimeout, - BadRequest, - BadResponse, - ServiceNotFound, - ServiceError, - ServerError, - ClientError, - ServerThreadpoolExhaustedError, -}; - struct AppException : public EnvoyException, public DubboFilters::DirectResponse, Logger::Loggable { - AppException(AppExceptionType type, const std::string& what); + AppException(ResponseStatus status, const std::string& what); AppException(const AppException& ex); using ResponseType = DubboFilters::DirectResponse::ResponseType; ResponseType encode(MessageMetadata& metadata, Protocol& protocol, Deserializer& deserializer, Buffer::Instance& buffer) const override; - const AppExceptionType type_; + const ResponseStatus status_; RpcResponseType response_type_; }; diff --git a/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc b/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc index 59fe606ea89cb..db3f1ad0fb010 100644 --- a/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc +++ b/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc @@ -52,7 +52,7 @@ Network::FilterStatus Router::messageEnd(MessageMetadataSharedPtr metadata) { if (!route_) { ENVOY_STREAM_LOG(debug, "dubbo router: no cluster match for interface '{}'", *callbacks_, metadata->service_name()); - callbacks_->sendLocalReply(AppException(AppExceptionType::ServiceNotFound, + callbacks_->sendLocalReply(AppException(ResponseStatus::ServiceNotFound, fmt::format("dubbo router: no route for interface '{}'", metadata->service_name())), true); @@ -65,10 +65,10 @@ Network::FilterStatus Router::messageEnd(MessageMetadataSharedPtr metadata) { if (!cluster) { ENVOY_STREAM_LOG(debug, "dubbo router: unknown cluster '{}'", *callbacks_, route_entry_->clusterName()); - callbacks_->sendLocalReply(AppException(AppExceptionType::ServerError, - fmt::format("dubbo router: unknown cluster '{}'", - route_entry_->clusterName())), - true); + callbacks_->sendLocalReply( + AppException(ResponseStatus::ServerError, fmt::format("dubbo router: unknown cluster '{}'", + route_entry_->clusterName())), + true); return Network::FilterStatus::StopIteration; } @@ -78,7 +78,7 @@ Network::FilterStatus Router::messageEnd(MessageMetadataSharedPtr metadata) { if (cluster_->maintenanceMode()) { callbacks_->sendLocalReply( - AppException(AppExceptionType::ServerError, + AppException(ResponseStatus::ServerError, fmt::format("dubbo router: maintenance mode for cluster '{}'", route_entry_->clusterName())), true); @@ -90,7 +90,7 @@ Network::FilterStatus Router::messageEnd(MessageMetadataSharedPtr metadata) { if (!conn_pool) { callbacks_->sendLocalReply( AppException( - AppExceptionType::ServerError, + ResponseStatus::ServerError, fmt::format("dubbo router: no healthy upstream for '{}'", route_entry_->clusterName())), true); return Network::FilterStatus::StopIteration; @@ -288,10 +288,10 @@ void Router::UpstreamRequest::onResetStream(Tcp::ConnectionPool::PoolFailureReas switch (reason) { case Tcp::ConnectionPool::PoolFailureReason::Overflow: parent_.callbacks_->sendLocalReply( - AppException(AppExceptionType::ServerError, + AppException(ResponseStatus::ServerError, fmt::format("dubbo upstream request: too many connections to '{}'", upstream_host_->address()->asString())), - true); + false); break; case Tcp::ConnectionPool::PoolFailureReason::LocalConnectionFailure: // Should only happen if we closed the connection, due to an error condition, in which case @@ -302,10 +302,10 @@ void Router::UpstreamRequest::onResetStream(Tcp::ConnectionPool::PoolFailureReas case Tcp::ConnectionPool::PoolFailureReason::Timeout: if (!response_started_) { parent_.callbacks_->sendLocalReply( - AppException(AppExceptionType::ServerError, + AppException(ResponseStatus::ServerError, fmt::format("dubbo upstream request: connection failure '{}'", upstream_host_->address()->asString())), - true); + false); return; } diff --git a/test/extensions/filters/network/dubbo_proxy/app_exception_test.cc b/test/extensions/filters/network/dubbo_proxy/app_exception_test.cc index 1987d2b20e088..16bf5834d4338 100644 --- a/test/extensions/filters/network/dubbo_proxy/app_exception_test.cc +++ b/test/extensions/filters/network/dubbo_proxy/app_exception_test.cc @@ -28,7 +28,7 @@ class AppExceptionTest : public TestBase { TEST_F(AppExceptionTest, Encode) { std::string mock_message("invalid method name 'Sub'"); - AppException app_exception(AppExceptionType::ServiceNotFound, mock_message); + AppException app_exception(ResponseStatus::ServiceNotFound, mock_message); Buffer::OwnedImpl buffer; metadata_->setSerializationType(SerializationType::Hessian); diff --git a/test/extensions/filters/network/dubbo_proxy/router_test.cc b/test/extensions/filters/network/dubbo_proxy/router_test.cc index 7d209f0b83dd3..1b87e487bf3d8 100644 --- a/test/extensions/filters/network/dubbo_proxy/router_test.cc +++ b/test/extensions/filters/network/dubbo_proxy/router_test.cc @@ -55,9 +55,9 @@ class TestNamedProtocolConfigFactory : public NamedProtocolConfigFactory { } // namespace -class RouterTestBase { +class DubboRouterTestBase { public: - RouterTestBase() + DubboRouterTestBase() : deserializer_factory_([&]() -> MockDeserializer* { ASSERT(deserializer_ == nullptr); deserializer_ = new NiceMock(); @@ -224,20 +224,17 @@ class RouterTestBase { NiceMock upstream_connection_; }; -class RouterTest : public RouterTestBase, public TestBase { -public: - RouterTest() {} -}; +class DubboRouterTest : public DubboRouterTestBase, public TestBase {}; -TEST_F(RouterTest, PoolRemoteConnectionFailure) { +TEST_F(DubboRouterTest, PoolRemoteConnectionFailure) { initializeRouter(); EXPECT_CALL(callbacks_, sendLocalReply(_, _)) .WillOnce(Invoke([&](const DubboFilters::DirectResponse& response, bool end_stream) -> void { auto& app_ex = dynamic_cast(response); - EXPECT_EQ(AppExceptionType::ServerError, app_ex.type_); + EXPECT_EQ(ResponseStatus::ServerError, app_ex.status_); EXPECT_THAT(app_ex.what(), ContainsRegex(".*connection failure.*")); - EXPECT_TRUE(end_stream); + EXPECT_FALSE(end_stream); })); startRequest(MessageType::Request); @@ -245,15 +242,15 @@ TEST_F(RouterTest, PoolRemoteConnectionFailure) { Tcp::ConnectionPool::PoolFailureReason::RemoteConnectionFailure); } -TEST_F(RouterTest, PoolTimeout) { +TEST_F(DubboRouterTest, PoolTimeout) { initializeRouter(); EXPECT_CALL(callbacks_, sendLocalReply(_, _)) .WillOnce(Invoke([&](const DubboFilters::DirectResponse& response, bool end_stream) -> void { auto& app_ex = dynamic_cast(response); - EXPECT_EQ(AppExceptionType::ServerError, app_ex.type_); + EXPECT_EQ(ResponseStatus::ServerError, app_ex.status_); EXPECT_THAT(app_ex.what(), ContainsRegex(".*connection failure.*")); - EXPECT_TRUE(end_stream); + EXPECT_FALSE(end_stream); })); startRequest(MessageType::Request); @@ -261,15 +258,15 @@ TEST_F(RouterTest, PoolTimeout) { Tcp::ConnectionPool::PoolFailureReason::Timeout); } -TEST_F(RouterTest, PoolOverflowFailure) { +TEST_F(DubboRouterTest, PoolOverflowFailure) { initializeRouter(); EXPECT_CALL(callbacks_, sendLocalReply(_, _)) .WillOnce(Invoke([&](const DubboFilters::DirectResponse& response, bool end_stream) -> void { auto& app_ex = dynamic_cast(response); - EXPECT_EQ(AppExceptionType::ServerError, app_ex.type_); + EXPECT_EQ(ResponseStatus::ServerError, app_ex.status_); EXPECT_THAT(app_ex.what(), ContainsRegex(".*too many connections.*")); - EXPECT_TRUE(end_stream); + EXPECT_FALSE(end_stream); })); startRequest(MessageType::Request); @@ -277,7 +274,7 @@ TEST_F(RouterTest, PoolOverflowFailure) { Tcp::ConnectionPool::PoolFailureReason::Overflow); } -TEST_F(RouterTest, ClusterMaintenanceMode) { +TEST_F(DubboRouterTest, ClusterMaintenanceMode) { initializeRouter(); initializeMetadata(MessageType::Request); @@ -290,14 +287,14 @@ TEST_F(RouterTest, ClusterMaintenanceMode) { EXPECT_CALL(callbacks_, sendLocalReply(_, _)) .WillOnce(Invoke([&](const DubboFilters::DirectResponse& response, bool end_stream) -> void { auto& app_ex = dynamic_cast(response); - EXPECT_EQ(AppExceptionType::ServerError, app_ex.type_); + EXPECT_EQ(ResponseStatus::ServerError, app_ex.status_); EXPECT_THAT(app_ex.what(), ContainsRegex(".*maintenance mode.*")); EXPECT_TRUE(end_stream); })); EXPECT_EQ(Network::FilterStatus::StopIteration, router_->messageEnd(metadata_)); } -TEST_F(RouterTest, NoHealthyHosts) { +TEST_F(DubboRouterTest, NoHealthyHosts) { initializeRouter(); initializeMetadata(MessageType::Request); @@ -310,7 +307,7 @@ TEST_F(RouterTest, NoHealthyHosts) { EXPECT_CALL(callbacks_, sendLocalReply(_, _)) .WillOnce(Invoke([&](const DubboFilters::DirectResponse& response, bool end_stream) -> void { auto& app_ex = dynamic_cast(response); - EXPECT_EQ(AppExceptionType::ServerError, app_ex.type_); + EXPECT_EQ(ResponseStatus::ServerError, app_ex.status_); EXPECT_THAT(app_ex.what(), ContainsRegex(".*no healthy upstream.*")); EXPECT_TRUE(end_stream); })); @@ -318,7 +315,7 @@ TEST_F(RouterTest, NoHealthyHosts) { EXPECT_EQ(Network::FilterStatus::StopIteration, router_->messageEnd(metadata_)); } -TEST_F(RouterTest, PoolConnectionFailureWithOnewayMessage) { +TEST_F(DubboRouterTest, PoolConnectionFailureWithOnewayMessage) { initializeRouter(); initializeMetadata(MessageType::Oneway); @@ -334,7 +331,7 @@ TEST_F(RouterTest, PoolConnectionFailureWithOnewayMessage) { destroyRouter(); } -TEST_F(RouterTest, NoRoute) { +TEST_F(DubboRouterTest, NoRoute) { initializeRouter(); initializeMetadata(MessageType::Request); @@ -342,14 +339,14 @@ TEST_F(RouterTest, NoRoute) { EXPECT_CALL(callbacks_, sendLocalReply(_, _)) .WillOnce(Invoke([&](const DubboFilters::DirectResponse& response, bool end_stream) -> void { auto& app_ex = dynamic_cast(response); - EXPECT_EQ(AppExceptionType::ServiceNotFound, app_ex.type_); + EXPECT_EQ(ResponseStatus::ServiceNotFound, app_ex.status_); EXPECT_THAT(app_ex.what(), ContainsRegex(".*no route.*")); EXPECT_TRUE(end_stream); })); EXPECT_EQ(Network::FilterStatus::StopIteration, router_->messageEnd(metadata_)); } -TEST_F(RouterTest, NoCluster) { +TEST_F(DubboRouterTest, NoCluster) { initializeRouter(); initializeMetadata(MessageType::Request); @@ -360,14 +357,14 @@ TEST_F(RouterTest, NoCluster) { EXPECT_CALL(callbacks_, sendLocalReply(_, _)) .WillOnce(Invoke([&](const DubboFilters::DirectResponse& response, bool end_stream) -> void { auto& app_ex = dynamic_cast(response); - EXPECT_EQ(AppExceptionType::ServerError, app_ex.type_); + EXPECT_EQ(ResponseStatus::ServerError, app_ex.status_); EXPECT_THAT(app_ex.what(), ContainsRegex(".*unknown cluster.*")); EXPECT_TRUE(end_stream); })); EXPECT_EQ(Network::FilterStatus::StopIteration, router_->messageEnd(metadata_)); } -TEST_F(RouterTest, UnexpectedRouterDestroy) { +TEST_F(DubboRouterTest, UnexpectedRouterDestroy) { initializeRouter(); initializeMetadata(MessageType::Request); EXPECT_CALL(upstream_connection_, close(Network::ConnectionCloseType::NoFlush)); @@ -381,15 +378,15 @@ TEST_F(RouterTest, UnexpectedRouterDestroy) { destroyRouter(); } -TEST_F(RouterTest, UpstreamRemoteCloseMidResponse) { +TEST_F(DubboRouterTest, UpstreamRemoteCloseMidResponse) { initializeRouter(); EXPECT_CALL(callbacks_, sendLocalReply(_, _)) .WillOnce(Invoke([&](const DubboFilters::DirectResponse& response, bool end_stream) -> void { auto& app_ex = dynamic_cast(response); - EXPECT_EQ(AppExceptionType::ServerError, app_ex.type_); + EXPECT_EQ(ResponseStatus::ServerError, app_ex.status_); EXPECT_THAT(app_ex.what(), ContainsRegex(".*connection failure.*")); - EXPECT_TRUE(end_stream); + EXPECT_FALSE(end_stream); })); startRequest(MessageType::Request); connectUpstream(); @@ -397,7 +394,7 @@ TEST_F(RouterTest, UpstreamRemoteCloseMidResponse) { destroyRouter(); } -TEST_F(RouterTest, UpstreamLocalCloseMidResponse) { +TEST_F(DubboRouterTest, UpstreamLocalCloseMidResponse) { initializeRouter(); startRequest(MessageType::Request); connectUpstream(); @@ -406,7 +403,7 @@ TEST_F(RouterTest, UpstreamLocalCloseMidResponse) { destroyRouter(); } -TEST_F(RouterTest, OneWay) { +TEST_F(DubboRouterTest, OneWay) { initializeRouter(); initializeMetadata(MessageType::Oneway); @@ -418,7 +415,7 @@ TEST_F(RouterTest, OneWay) { destroyRouter(); } -TEST_F(RouterTest, Call) { +TEST_F(DubboRouterTest, Call) { initializeRouter(); initializeMetadata(MessageType::Request); @@ -433,7 +430,7 @@ TEST_F(RouterTest, Call) { destroyRouter(); } -TEST_F(RouterTest, DecoderFilterCallbacks) { +TEST_F(DubboRouterTest, DecoderFilterCallbacks) { initializeRouter(); initializeMetadata(MessageType::Request); From cc34c53aaaf4a86d4c491d4bb8d3d58aca7358a1 Mon Sep 17 00:00:00 2001 From: "leilei.gll" Date: Mon, 18 Feb 2019 14:56:31 +0800 Subject: [PATCH 06/23] Add some comments to serializeRpcResult Signed-off-by: leilei.gll --- .../filters/network/dubbo_proxy/dubbo_protocol_impl.h | 2 +- .../filters/network/dubbo_proxy/hessian_deserializer_impl.cc | 3 +++ .../filters/network/dubbo_proxy/router/router_impl.cc | 1 + 3 files changed, 5 insertions(+), 1 deletion(-) diff --git a/source/extensions/filters/network/dubbo_proxy/dubbo_protocol_impl.h b/source/extensions/filters/network/dubbo_proxy/dubbo_protocol_impl.h index f5dca2d3c1dd7..24341df0530de 100644 --- a/source/extensions/filters/network/dubbo_proxy/dubbo_protocol_impl.h +++ b/source/extensions/filters/network/dubbo_proxy/dubbo_protocol_impl.h @@ -68,7 +68,7 @@ typedef std::unique_ptr ResponseMessageImplPtr; class DubboProtocolImpl : public Protocol { public: - DubboProtocolImpl() {} + DubboProtocolImpl() = default; DubboProtocolImpl(ProtocolCallbacks* callbacks) : callbacks_(callbacks) {} const std::string& name() const override { return ProtocolNames::get().fromType(type()); } ProtocolType type() const override { return ProtocolType::Dubbo; } diff --git a/source/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl.cc b/source/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl.cc index b126e62d76a9a..e077ed3b12ed2 100644 --- a/source/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl.cc +++ b/source/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl.cc @@ -80,9 +80,12 @@ RpcResultPtr HessianDeserializerImpl::deserializeRpcResult(Buffer::Instance& buf void HessianDeserializerImpl::serializeRpcResult(Buffer::Instance& output_buffer, const std::string& content, RpcResponseType type) { + // The serialized response type is compact int. uint8_t response_type_code = static_cast(type); uint8_t base_code = 0x90; output_buffer.writeByte(static_cast(base_code + response_type_code)); + + // Serialized response content. HessianUtils::writeString(output_buffer, content); } diff --git a/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc b/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc index db3f1ad0fb010..363f05b37fe73 100644 --- a/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc +++ b/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc @@ -195,6 +195,7 @@ Network::FilterStatus Router::UpstreamRequest::start() { void Router::UpstreamRequest::resetStream() { if (conn_pool_handle_) { conn_pool_handle_->cancel(Tcp::ConnectionPool::CancelPolicy::Default); + conn_pool_handle_ = nullptr; } if (conn_data_ != nullptr) { From 788706bae9ca03519f25c07032a4ea3effd295b8 Mon Sep 17 00:00:00 2001 From: "leilei.gll" Date: Mon, 18 Feb 2019 19:10:30 +0800 Subject: [PATCH 07/23] Modify the onResetStream function implementation Signed-off-by: leilei.gll --- .../dubbo_proxy/hessian_deserializer_impl.cc | 3 +- .../network/dubbo_proxy/hessian_utils.cc | 7 ++-- .../network/dubbo_proxy/hessian_utils.h | 2 +- .../network/dubbo_proxy/router/router_impl.cc | 32 +++++++++++-------- 4 files changed, 25 insertions(+), 19 deletions(-) diff --git a/source/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl.cc b/source/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl.cc index e077ed3b12ed2..6feac5541ff7b 100644 --- a/source/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl.cc +++ b/source/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl.cc @@ -86,7 +86,8 @@ void HessianDeserializerImpl::serializeRpcResult(Buffer::Instance& output_buffer output_buffer.writeByte(static_cast(base_code + response_type_code)); // Serialized response content. - HessianUtils::writeString(output_buffer, content); + absl::string_view str_view(content); + HessianUtils::writeString(output_buffer, str_view); } class HessianDeserializerConfigFactory : public DeserializerFactoryBase { diff --git a/source/extensions/filters/network/dubbo_proxy/hessian_utils.cc b/source/extensions/filters/network/dubbo_proxy/hessian_utils.cc index 547f4711c256e..90b44a38e75f9 100644 --- a/source/extensions/filters/network/dubbo_proxy/hessian_utils.cc +++ b/source/extensions/filters/network/dubbo_proxy/hessian_utils.cc @@ -32,7 +32,7 @@ void addSeq(Buffer::Instance& buffer, const std::initializer_list value } } -void doWriteString(Buffer::Instance& instance, absl::string_view str_view) { +void doWriteString(Buffer::Instance& instance, const absl::string_view& str_view) { const size_t length = str_view.length(); constexpr size_t str_max_length = 0xffff; constexpr size_t two_octet_max_lenth = 1024; @@ -558,9 +558,8 @@ std::string HessianUtils::readByte(Buffer::Instance& buffer) { return result; } -void HessianUtils::writeString(Buffer::Instance& buffer, const std::string& str) { - absl::string_view str_view(str); - doWriteString(buffer, str_view); +void HessianUtils::writeString(Buffer::Instance& buffer, const absl::string_view& str) { + doWriteString(buffer, str); return; } diff --git a/source/extensions/filters/network/dubbo_proxy/hessian_utils.h b/source/extensions/filters/network/dubbo_proxy/hessian_utils.h index d136019003b9f..5cf1fbc7819ca 100644 --- a/source/extensions/filters/network/dubbo_proxy/hessian_utils.h +++ b/source/extensions/filters/network/dubbo_proxy/hessian_utils.h @@ -36,7 +36,7 @@ class HessianUtils { static std::chrono::milliseconds readDate(Buffer::Instance& buffer); static std::string readByte(Buffer::Instance& buffer); - static void writeString(Buffer::Instance& buffer, const std::string& str); + static void writeString(Buffer::Instance& buffer, const absl::string_view& str); }; } // namespace DubboProxy diff --git a/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc b/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc index 363f05b37fe73..7e230fa9f8e42 100644 --- a/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc +++ b/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc @@ -194,11 +194,13 @@ Network::FilterStatus Router::UpstreamRequest::start() { void Router::UpstreamRequest::resetStream() { if (conn_pool_handle_) { + ASSERT(!conn_data_); conn_pool_handle_->cancel(Tcp::ConnectionPool::CancelPolicy::Default); conn_pool_handle_ = nullptr; } - if (conn_data_ != nullptr) { + if (conn_data_) { + ASSERT(!conn_pool_handle_); conn_data_->connection().close(Network::ConnectionCloseType::NoFlush); conn_data_.reset(); } @@ -297,21 +299,25 @@ void Router::UpstreamRequest::onResetStream(Tcp::ConnectionPool::PoolFailureReas case Tcp::ConnectionPool::PoolFailureReason::LocalConnectionFailure: // Should only happen if we closed the connection, due to an error condition, in which case // we've already handled any possible downstream response. - parent_.callbacks_->resetDownstreamConnection(); + parent_.callbacks_->sendLocalReply( + AppException(ResponseStatus::ServerError, + fmt::format("dubbo upstream request: local connection failure '{}'", + upstream_host_->address()->asString())), + false); break; case Tcp::ConnectionPool::PoolFailureReason::RemoteConnectionFailure: + parent_.callbacks_->sendLocalReply( + AppException(ResponseStatus::ServerError, + fmt::format("dubbo upstream request: remote connection failure '{}'", + upstream_host_->address()->asString())), + false); + break; case Tcp::ConnectionPool::PoolFailureReason::Timeout: - if (!response_started_) { - parent_.callbacks_->sendLocalReply( - AppException(ResponseStatus::ServerError, - fmt::format("dubbo upstream request: connection failure '{}'", - upstream_host_->address()->asString())), - false); - return; - } - - // Error occurred after a partial response, propagate the reset to the downstream. - parent_.callbacks_->resetDownstreamConnection(); + parent_.callbacks_->sendLocalReply( + AppException(ResponseStatus::ServerError, + fmt::format("dubbo upstream request: connection failure '{}' due to timeout", + upstream_host_->address()->asString())), + false); break; default: NOT_REACHED_GCOVR_EXCL_LINE; From 3c16f0fd06e0da9740cee175219e196ae59c3ed8 Mon Sep 17 00:00:00 2001 From: "leilei.gll" Date: Wed, 20 Feb 2019 20:23:06 +0800 Subject: [PATCH 08/23] Add the resetStream interface to DecoderFilterCallbacks Signed-off-by: leilei.gll --- .../network/dubbo_proxy/filters/filter.h | 5 ++++ .../dubbo_proxy/hessian_deserializer_impl.cc | 4 +-- .../network/dubbo_proxy/hessian_utils.cc | 5 ++++ .../network/dubbo_proxy/hessian_utils.h | 1 + .../network/dubbo_proxy/router/router_impl.cc | 26 ++++++++++++++----- .../network/dubbo_proxy/router/router_impl.h | 2 ++ .../filters/network/dubbo_proxy/mocks.h | 1 + .../network/dubbo_proxy/router_test.cc | 8 +++--- 8 files changed, 39 insertions(+), 13 deletions(-) diff --git a/source/extensions/filters/network/dubbo_proxy/filters/filter.h b/source/extensions/filters/network/dubbo_proxy/filters/filter.h index c6786b3d161b7..28e1fed8d37f6 100644 --- a/source/extensions/filters/network/dubbo_proxy/filters/filter.h +++ b/source/extensions/filters/network/dubbo_proxy/filters/filter.h @@ -132,6 +132,11 @@ class DecoderFilterCallbacks { * @return StreamInfo for logging purposes. */ virtual StreamInfo::StreamInfo& streamInfo() PURE; + + /** + * Reset the underlying stream. + */ + virtual void resetStream() PURE; }; /** diff --git a/source/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl.cc b/source/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl.cc index 6feac5541ff7b..baf83b1894b84 100644 --- a/source/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl.cc +++ b/source/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl.cc @@ -81,9 +81,7 @@ RpcResultPtr HessianDeserializerImpl::deserializeRpcResult(Buffer::Instance& buf void HessianDeserializerImpl::serializeRpcResult(Buffer::Instance& output_buffer, const std::string& content, RpcResponseType type) { // The serialized response type is compact int. - uint8_t response_type_code = static_cast(type); - uint8_t base_code = 0x90; - output_buffer.writeByte(static_cast(base_code + response_type_code)); + HessianUtils::writeInt(output_buffer, static_cast(type)); // Serialized response content. absl::string_view str_view(content); diff --git a/source/extensions/filters/network/dubbo_proxy/hessian_utils.cc b/source/extensions/filters/network/dubbo_proxy/hessian_utils.cc index 90b44a38e75f9..1a23ee923dafc 100644 --- a/source/extensions/filters/network/dubbo_proxy/hessian_utils.cc +++ b/source/extensions/filters/network/dubbo_proxy/hessian_utils.cc @@ -563,6 +563,11 @@ void HessianUtils::writeString(Buffer::Instance& buffer, const absl::string_view return; } +void HessianUtils::writeInt(Buffer::Instance& buffer, uint8_t value) { + // Compact int + buffer.writeByte(0x90 + value); +} + } // namespace DubboProxy } // namespace NetworkFilters } // namespace Extensions diff --git a/source/extensions/filters/network/dubbo_proxy/hessian_utils.h b/source/extensions/filters/network/dubbo_proxy/hessian_utils.h index 5cf1fbc7819ca..6942e3dc31b89 100644 --- a/source/extensions/filters/network/dubbo_proxy/hessian_utils.h +++ b/source/extensions/filters/network/dubbo_proxy/hessian_utils.h @@ -37,6 +37,7 @@ class HessianUtils { static std::string readByte(Buffer::Instance& buffer); static void writeString(Buffer::Instance& buffer, const absl::string_view& str); + static void writeInt(Buffer::Instance& buffer, uint8_t value); }; } // namespace DubboProxy diff --git a/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc b/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc index 7e230fa9f8e42..29476f20ef5e7 100644 --- a/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc +++ b/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc @@ -40,6 +40,8 @@ Network::FilterStatus Router::transportEnd() { cleanup(); } + filter_complete_ = true; + return Network::FilterStatus::Continue; } @@ -55,7 +57,7 @@ Network::FilterStatus Router::messageEnd(MessageMetadataSharedPtr metadata) { callbacks_->sendLocalReply(AppException(ResponseStatus::ServiceNotFound, fmt::format("dubbo router: no route for interface '{}'", metadata->service_name())), - true); + false); return Network::FilterStatus::StopIteration; } @@ -68,7 +70,7 @@ Network::FilterStatus Router::messageEnd(MessageMetadataSharedPtr metadata) { callbacks_->sendLocalReply( AppException(ResponseStatus::ServerError, fmt::format("dubbo router: unknown cluster '{}'", route_entry_->clusterName())), - true); + false); return Network::FilterStatus::StopIteration; } @@ -81,7 +83,7 @@ Network::FilterStatus Router::messageEnd(MessageMetadataSharedPtr metadata) { AppException(ResponseStatus::ServerError, fmt::format("dubbo router: maintenance mode for cluster '{}'", route_entry_->clusterName())), - true); + false); return Network::FilterStatus::StopIteration; } @@ -92,7 +94,7 @@ Network::FilterStatus Router::messageEnd(MessageMetadataSharedPtr metadata) { AppException( ResponseStatus::ServerError, fmt::format("dubbo router: no healthy upstream for '{}'", route_entry_->clusterName())), - true); + false); return Network::FilterStatus::StopIteration; } @@ -124,6 +126,10 @@ void Router::onUpstreamData(Buffer::Instance& data, bool end_stream) { return; } else if (status == DubboFilters::UpstreamResponseStatus::Reset) { ENVOY_STREAM_LOG(debug, "dubbo router: upstream reset", *callbacks_); + // When the upstreamData function returns Reset, + // the current stream is already released from the upper layer, + // so there is no need to call callbacks_->resetStream() to notify + // the upper layer to release the stream. upstream_request_->resetStream(); return; } @@ -131,9 +137,9 @@ void Router::onUpstreamData(Buffer::Instance& data, bool end_stream) { if (end_stream) { // Response is incomplete, but no more data is coming. ENVOY_STREAM_LOG(debug, "dubbo router: response underflow", *callbacks_); - upstream_request_->onResponseComplete(); upstream_request_->onResetStream( Tcp::ConnectionPool::PoolFailureReason::RemoteConnectionFailure); + upstream_request_->onResponseComplete(); cleanup(); } } @@ -233,7 +239,6 @@ void Router::UpstreamRequest::onPoolFailure(Tcp::ConnectionPool::PoolFailureReas onUpstreamHostSelected(host); onResetStream(reason); - conn_pool_handle_ = nullptr; parent_.upstream_request_buffer_.drain(parent_.upstream_request_buffer_.length()); } @@ -288,6 +293,8 @@ void Router::UpstreamRequest::onResetStream(Tcp::ConnectionPool::PoolFailureReas return; } + // When the filter's callback does not end, the sendLocalReply function call + // triggers the release of the current stream at the end of the filter's callback. switch (reason) { case Tcp::ConnectionPool::PoolFailureReason::Overflow: parent_.callbacks_->sendLocalReply( @@ -322,6 +329,13 @@ void Router::UpstreamRequest::onResetStream(Tcp::ConnectionPool::PoolFailureReas default: NOT_REACHED_GCOVR_EXCL_LINE; } + + if (parent_.filter_complete_ && !response_complete_) { + // When the filter's callback has ended and the reply message has not been processed, + // call resetStream to release the current stream. + // the resetStream eventually triggers the onDestroy function call. + parent_.callbacks_->resetStream(); + } } } // namespace Router diff --git a/source/extensions/filters/network/dubbo_proxy/router/router_impl.h b/source/extensions/filters/network/dubbo_proxy/router/router_impl.h index 069475ffce6ac..1b590d39baf28 100644 --- a/source/extensions/filters/network/dubbo_proxy/router/router_impl.h +++ b/source/extensions/filters/network/dubbo_proxy/router/router_impl.h @@ -95,6 +95,8 @@ class Router : public Tcp::ConnectionPool::UpstreamCallbacks, std::unique_ptr upstream_request_; Envoy::Buffer::OwnedImpl upstream_request_buffer_; + + bool filter_complete_{false}; }; } // namespace Router diff --git a/test/extensions/filters/network/dubbo_proxy/mocks.h b/test/extensions/filters/network/dubbo_proxy/mocks.h index bbcef29ad29e4..d6e178ee65347 100644 --- a/test/extensions/filters/network/dubbo_proxy/mocks.h +++ b/test/extensions/filters/network/dubbo_proxy/mocks.h @@ -125,6 +125,7 @@ class MockDecoderFilterCallbacks : public DecoderFilterCallbacks { MOCK_METHOD1(upstreamData, UpstreamResponseStatus(Buffer::Instance&)); MOCK_METHOD0(resetDownstreamConnection, void()); MOCK_METHOD0(streamInfo, StreamInfo::StreamInfo&()); + MOCK_METHOD0(resetStream, void()); uint64_t stream_id_{1}; NiceMock connection_; diff --git a/test/extensions/filters/network/dubbo_proxy/router_test.cc b/test/extensions/filters/network/dubbo_proxy/router_test.cc index 1b87e487bf3d8..9edbf0ca048a3 100644 --- a/test/extensions/filters/network/dubbo_proxy/router_test.cc +++ b/test/extensions/filters/network/dubbo_proxy/router_test.cc @@ -289,7 +289,7 @@ TEST_F(DubboRouterTest, ClusterMaintenanceMode) { auto& app_ex = dynamic_cast(response); EXPECT_EQ(ResponseStatus::ServerError, app_ex.status_); EXPECT_THAT(app_ex.what(), ContainsRegex(".*maintenance mode.*")); - EXPECT_TRUE(end_stream); + EXPECT_FALSE(end_stream); })); EXPECT_EQ(Network::FilterStatus::StopIteration, router_->messageEnd(metadata_)); } @@ -309,7 +309,7 @@ TEST_F(DubboRouterTest, NoHealthyHosts) { auto& app_ex = dynamic_cast(response); EXPECT_EQ(ResponseStatus::ServerError, app_ex.status_); EXPECT_THAT(app_ex.what(), ContainsRegex(".*no healthy upstream.*")); - EXPECT_TRUE(end_stream); + EXPECT_FALSE(end_stream); })); EXPECT_EQ(Network::FilterStatus::StopIteration, router_->messageEnd(metadata_)); @@ -341,7 +341,7 @@ TEST_F(DubboRouterTest, NoRoute) { auto& app_ex = dynamic_cast(response); EXPECT_EQ(ResponseStatus::ServiceNotFound, app_ex.status_); EXPECT_THAT(app_ex.what(), ContainsRegex(".*no route.*")); - EXPECT_TRUE(end_stream); + EXPECT_FALSE(end_stream); })); EXPECT_EQ(Network::FilterStatus::StopIteration, router_->messageEnd(metadata_)); } @@ -359,7 +359,7 @@ TEST_F(DubboRouterTest, NoCluster) { auto& app_ex = dynamic_cast(response); EXPECT_EQ(ResponseStatus::ServerError, app_ex.status_); EXPECT_THAT(app_ex.what(), ContainsRegex(".*unknown cluster.*")); - EXPECT_TRUE(end_stream); + EXPECT_FALSE(end_stream); })); EXPECT_EQ(Network::FilterStatus::StopIteration, router_->messageEnd(metadata_)); } From f15e8d8493205aa12ecf42da9e769e37537b16b9 Mon Sep 17 00:00:00 2001 From: "leilei.gll" Date: Wed, 20 Feb 2019 21:16:25 +0800 Subject: [PATCH 09/23] Remove useless code Signed-off-by: leilei.gll --- source/extensions/filters/network/dubbo_proxy/BUILD | 1 - .../filters/network/dubbo_proxy/app_exception.cc | 2 -- .../network/dubbo_proxy/hessian_deserializer_impl.cc | 3 +-- source/extensions/filters/network/dubbo_proxy/router/BUILD | 1 - .../filters/network/dubbo_proxy/router/router_impl.cc | 7 ++----- .../filters/network/dubbo_proxy/router/router_impl.h | 2 +- 6 files changed, 4 insertions(+), 12 deletions(-) diff --git a/source/extensions/filters/network/dubbo_proxy/BUILD b/source/extensions/filters/network/dubbo_proxy/BUILD index 3fc0ab64ba6db..250d8c15462bb 100644 --- a/source/extensions/filters/network/dubbo_proxy/BUILD +++ b/source/extensions/filters/network/dubbo_proxy/BUILD @@ -173,7 +173,6 @@ envoy_cc_library( ":protocol_interface", "//include/envoy/buffer:buffer_interface", "//source/common/buffer:buffer_lib", - "//source/common/common:byte_order_lib", "//source/extensions/filters/network/dubbo_proxy/filters:filter_interface", ], ) diff --git a/source/extensions/filters/network/dubbo_proxy/app_exception.cc b/source/extensions/filters/network/dubbo_proxy/app_exception.cc index bed47ba34cc63..e4bf681df499a 100644 --- a/source/extensions/filters/network/dubbo_proxy/app_exception.cc +++ b/source/extensions/filters/network/dubbo_proxy/app_exception.cc @@ -1,8 +1,6 @@ #include "extensions/filters/network/dubbo_proxy/app_exception.h" #include "common/buffer/buffer_impl.h" -#include "common/common/byte_order.h" -#include "common/common/hex.h" #include "extensions/filters/network/dubbo_proxy/message.h" diff --git a/source/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl.cc b/source/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl.cc index baf83b1894b84..3aa56ab6a32cf 100644 --- a/source/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl.cc +++ b/source/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl.cc @@ -84,8 +84,7 @@ void HessianDeserializerImpl::serializeRpcResult(Buffer::Instance& output_buffer HessianUtils::writeInt(output_buffer, static_cast(type)); // Serialized response content. - absl::string_view str_view(content); - HessianUtils::writeString(output_buffer, str_view); + HessianUtils::writeString(output_buffer, content); } class HessianDeserializerConfigFactory : public DeserializerFactoryBase { diff --git a/source/extensions/filters/network/dubbo_proxy/router/BUILD b/source/extensions/filters/network/dubbo_proxy/router/BUILD index 6f09745996167..9718704d26b3b 100644 --- a/source/extensions/filters/network/dubbo_proxy/router/BUILD +++ b/source/extensions/filters/network/dubbo_proxy/router/BUILD @@ -39,7 +39,6 @@ envoy_cc_library( hdrs = ["config.h"], deps = [ ":router_lib", - ":router_matcher", "//include/envoy/registry", "//source/extensions/filters/network/dubbo_proxy/filters:factory_base_lib", "//source/extensions/filters/network/dubbo_proxy/filters:filter_config_interface", diff --git a/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc b/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc index 29476f20ef5e7..1104dcfb5176d 100644 --- a/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc +++ b/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc @@ -32,7 +32,7 @@ Network::FilterStatus Router::transportBegin() { Network::FilterStatus Router::transportEnd() { ASSERT(upstream_request_); - upstream_request_->encodeData(upstream_request_buffer_, true); + upstream_request_->encodeData(upstream_request_buffer_); if (upstream_request_->metadata_->message_type() == MessageType::Oneway) { // No response expected @@ -212,7 +212,7 @@ void Router::UpstreamRequest::resetStream() { } } -void Router::UpstreamRequest::encodeData(Buffer::Instance& data, bool end_stream) { +void Router::UpstreamRequest::encodeData(Buffer::Instance& data) { if (!conn_data_) { ENVOY_STREAM_LOG(trace, "buffering {} bytes", *parent_.callbacks_, data.length()); if (!buffered_request_body_) { @@ -223,9 +223,6 @@ void Router::UpstreamRequest::encodeData(Buffer::Instance& data, bool end_stream } else { ENVOY_STREAM_LOG(trace, "proxying {} bytes", *parent_.callbacks_, data.length()); conn_data_->connection().write(data, false); - if (end_stream) { - parent_.callbacks_->streamInfo().onLastUpstreamTxByteSent(); - } } } diff --git a/source/extensions/filters/network/dubbo_proxy/router/router_impl.h b/source/extensions/filters/network/dubbo_proxy/router/router_impl.h index 1b590d39baf28..0daefaf1f2aa1 100644 --- a/source/extensions/filters/network/dubbo_proxy/router/router_impl.h +++ b/source/extensions/filters/network/dubbo_proxy/router/router_impl.h @@ -54,7 +54,7 @@ class Router : public Tcp::ConnectionPool::UpstreamCallbacks, Network::FilterStatus start(); void resetStream(); - void encodeData(Buffer::Instance& data, bool end_stream); + void encodeData(Buffer::Instance& data); // Tcp::ConnectionPool::Callbacks void onPoolFailure(Tcp::ConnectionPool::PoolFailureReason reason, From f3dfee084729f5a5afb363ee95d2fdd2ccdebfdc Mon Sep 17 00:00:00 2001 From: "leilei.gll" Date: Thu, 21 Feb 2019 10:31:52 +0800 Subject: [PATCH 10/23] Rename router_matcher to route_matcher Signed-off-by: leilei.gll --- source/extensions/filters/network/dubbo_proxy/router/BUILD | 2 +- test/extensions/filters/network/dubbo_proxy/BUILD | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/source/extensions/filters/network/dubbo_proxy/router/BUILD b/source/extensions/filters/network/dubbo_proxy/router/BUILD index 9718704d26b3b..790e94d00e3a9 100644 --- a/source/extensions/filters/network/dubbo_proxy/router/BUILD +++ b/source/extensions/filters/network/dubbo_proxy/router/BUILD @@ -18,7 +18,7 @@ envoy_cc_library( ) envoy_cc_library( - name = "router_matcher", + name = "route_matcher", srcs = ["route_matcher.cc"], hdrs = ["route_matcher.h"], deps = [ diff --git a/test/extensions/filters/network/dubbo_proxy/BUILD b/test/extensions/filters/network/dubbo_proxy/BUILD index 909ed48a64c34..08eb97527f276 100644 --- a/test/extensions/filters/network/dubbo_proxy/BUILD +++ b/test/extensions/filters/network/dubbo_proxy/BUILD @@ -113,7 +113,7 @@ envoy_extension_cc_test( extension_name = "envoy.filters.network.dubbo_proxy", deps = [ "//source/extensions/filters/network/dubbo_proxy:metadata_lib", - "//source/extensions/filters/network/dubbo_proxy/router:router_matcher", + "//source/extensions/filters/network/dubbo_proxy/router:route_matcher", "//test/mocks/server:server_mocks", "@envoy_api//envoy/config/filter/network/dubbo_proxy/v2alpha1:dubbo_proxy_cc", ], From 2e071ebaf4e6a1027d710842504a4377f241065f Mon Sep 17 00:00:00 2001 From: "leilei.gll" Date: Thu, 21 Feb 2019 14:49:34 +0800 Subject: [PATCH 11/23] Add the continue decoding logic when the connection fails Signed-off-by: leilei.gll --- .../network/dubbo_proxy/router/router_impl.cc | 14 ++++++++++++-- .../network/dubbo_proxy/app_exception_test.cc | 2 -- .../filters/network/dubbo_proxy/router_test.cc | 2 +- 3 files changed, 13 insertions(+), 5 deletions(-) diff --git a/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc b/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc index 1104dcfb5176d..fa0987691e023 100644 --- a/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc +++ b/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc @@ -237,6 +237,16 @@ void Router::UpstreamRequest::onPoolFailure(Tcp::ConnectionPool::PoolFailureReas onResetStream(reason); parent_.upstream_request_buffer_.drain(parent_.upstream_request_buffer_.length()); + + // If it is a connection error, it means that the connection pool returned + // the error asynchronously and the upper layer needs to be notified to continue decoding. + // If it is a non-connection error, it is returned synchronously from the connection pool + // and is still in the callback at the current Filter, nothing to do. + if (reason == Tcp::ConnectionPool::PoolFailureReason::Timeout || + reason == Tcp::ConnectionPool::PoolFailureReason::LocalConnectionFailure || + reason == Tcp::ConnectionPool::PoolFailureReason::RemoteConnectionFailure) { + parent_.callbacks_->continueDecoding(); + } } void Router::UpstreamRequest::onPoolReady(Tcp::ConnectionPool::ConnectionDataPtr&& conn_data, @@ -285,8 +295,8 @@ void Router::UpstreamRequest::onResetStream(Tcp::ConnectionPool::PoolFailureReas if (metadata_->message_type() == MessageType::Oneway) { // For oneway requests, we should not attempt a response. Reset the downstream to signal // an error. - ENVOY_LOG(debug, "dubbo upstream request: the request is oneway, reset downstream connection"); - parent_.callbacks_->resetDownstreamConnection(); + ENVOY_LOG(debug, "dubbo upstream request: the request is oneway, reset downstream stream"); + parent_.callbacks_->resetStream(); return; } diff --git a/test/extensions/filters/network/dubbo_proxy/app_exception_test.cc b/test/extensions/filters/network/dubbo_proxy/app_exception_test.cc index 16bf5834d4338..856007134d701 100644 --- a/test/extensions/filters/network/dubbo_proxy/app_exception_test.cc +++ b/test/extensions/filters/network/dubbo_proxy/app_exception_test.cc @@ -5,10 +5,8 @@ #include "extensions/filters/network/dubbo_proxy/hessian_deserializer_impl.h" #include "extensions/filters/network/dubbo_proxy/metadata.h" -#include "test/extensions/filters/network/dubbo_proxy/mocks.h" #include "test/test_common/test_base.h" -#include "gmock/gmock.h" #include "gtest/gtest.h" namespace Envoy { diff --git a/test/extensions/filters/network/dubbo_proxy/router_test.cc b/test/extensions/filters/network/dubbo_proxy/router_test.cc index 9edbf0ca048a3..c1c7e4be0b639 100644 --- a/test/extensions/filters/network/dubbo_proxy/router_test.cc +++ b/test/extensions/filters/network/dubbo_proxy/router_test.cc @@ -322,7 +322,7 @@ TEST_F(DubboRouterTest, PoolConnectionFailureWithOnewayMessage) { EXPECT_CALL(callbacks_, downstreamSerializationType()) .WillOnce(Return(SerializationType::Hessian)); EXPECT_CALL(callbacks_, sendLocalReply(_, _)).Times(0); - EXPECT_CALL(callbacks_, resetDownstreamConnection()).Times(1); + EXPECT_CALL(callbacks_, resetStream()).Times(1); EXPECT_EQ(Network::FilterStatus::StopIteration, router_->messageEnd(metadata_)); context_.cluster_manager_.tcp_conn_pool_.poolFailure( From a0c81f442560a35bb451f3db2a23c85c43d53acf Mon Sep 17 00:00:00 2001 From: "leilei.gll" Date: Thu, 21 Feb 2019 20:11:09 +0800 Subject: [PATCH 12/23] Remove the use of buffered_request_body_ Signed-off-by: leilei.gll --- .../network/dubbo_proxy/router/router_impl.cc | 25 +++++++------------ .../network/dubbo_proxy/router/router_impl.h | 1 - 2 files changed, 9 insertions(+), 17 deletions(-) diff --git a/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc b/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc index fa0987691e023..1b4cf8767e6a8 100644 --- a/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc +++ b/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc @@ -30,7 +30,11 @@ Network::FilterStatus Router::transportBegin() { } Network::FilterStatus Router::transportEnd() { + // If the connection fails, the callback of the filter will be suspended, + // so it is impossible to call the transportEnd interface. + // the encodeData function will be called only if the connection is successful. ASSERT(upstream_request_); + ASSERT(upstream_request_->conn_data_); upstream_request_->encodeData(upstream_request_buffer_); @@ -213,17 +217,11 @@ void Router::UpstreamRequest::resetStream() { } void Router::UpstreamRequest::encodeData(Buffer::Instance& data) { - if (!conn_data_) { - ENVOY_STREAM_LOG(trace, "buffering {} bytes", *parent_.callbacks_, data.length()); - if (!buffered_request_body_) { - buffered_request_body_ = std::make_unique(); - } - - buffered_request_body_->move(data); - } else { - ENVOY_STREAM_LOG(trace, "proxying {} bytes", *parent_.callbacks_, data.length()); - conn_data_->connection().write(data, false); - } + ASSERT(conn_data_); + ASSERT(!conn_pool_handle_); + + ENVOY_STREAM_LOG(trace, "proxying {} bytes", *parent_.callbacks_, data.length()); + conn_data_->connection().write(data, false); } void Router::UpstreamRequest::onPoolFailure(Tcp::ConnectionPool::PoolFailureReason reason, @@ -268,11 +266,6 @@ void Router::UpstreamRequest::onRequestStart(bool continue_decoding) { ENVOY_LOG(debug, "dubbo upstream request: start sending data to the server {}", upstream_host_->address()->asString()); - if (buffered_request_body_) { - conn_data_->connection().write(*buffered_request_body_, false); - parent_.callbacks_->streamInfo().onLastUpstreamTxByteSent(); - } - if (continue_decoding) { parent_.callbacks_->continueDecoding(); } diff --git a/source/extensions/filters/network/dubbo_proxy/router/router_impl.h b/source/extensions/filters/network/dubbo_proxy/router/router_impl.h index 0daefaf1f2aa1..d30a5989352b6 100644 --- a/source/extensions/filters/network/dubbo_proxy/router/router_impl.h +++ b/source/extensions/filters/network/dubbo_proxy/router/router_impl.h @@ -77,7 +77,6 @@ class Router : public Tcp::ConnectionPool::UpstreamCallbacks, Upstream::HostDescriptionConstSharedPtr upstream_host_; DeserializerPtr deserializer_; ProtocolPtr protocol_; - Buffer::InstancePtr buffered_request_body_; bool request_complete_ : 1; bool response_started_ : 1; From 0837aba4974895b0f35e052969197c5800a34d89 Mon Sep 17 00:00:00 2001 From: "leilei.gll" Date: Fri, 22 Feb 2019 14:07:52 +0800 Subject: [PATCH 13/23] Fixed body_size setting error in AppException encoding Signed-off-by: leilei.gll --- .../network/dubbo_proxy/app_exception.cc | 2 +- .../dubbo_proxy/hessian_deserializer_impl.cc | 3 +- .../filters/network/dubbo_proxy/BUILD | 2 + .../network/dubbo_proxy/app_exception_test.cc | 39 ++++++++++++++++++- .../dubbo_proxy/dubbo_protocol_impl_test.cc | 23 +++++++++++ .../hessian_deserializer_impl_test.cc | 24 ++++++++++++ 6 files changed, 90 insertions(+), 3 deletions(-) diff --git a/source/extensions/filters/network/dubbo_proxy/app_exception.cc b/source/extensions/filters/network/dubbo_proxy/app_exception.cc index e4bf681df499a..79aea955f6ea4 100644 --- a/source/extensions/filters/network/dubbo_proxy/app_exception.cc +++ b/source/extensions/filters/network/dubbo_proxy/app_exception.cc @@ -26,7 +26,7 @@ AppException::ResponseType AppException::encode(MessageMetadata& metadata, metadata.setMessageType(MessageType::Response); const std::string& response = what(); - if (!protocol.encode(buffer, response.size(), metadata)) { + if (!protocol.encode(buffer, response.size() + sizeof(response_type_), metadata)) { throw EnvoyException("failed to encode local reply message"); } diff --git a/source/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl.cc b/source/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl.cc index 3aa56ab6a32cf..f4b8b097c55d5 100644 --- a/source/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl.cc +++ b/source/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl.cc @@ -81,7 +81,8 @@ RpcResultPtr HessianDeserializerImpl::deserializeRpcResult(Buffer::Instance& buf void HessianDeserializerImpl::serializeRpcResult(Buffer::Instance& output_buffer, const std::string& content, RpcResponseType type) { // The serialized response type is compact int. - HessianUtils::writeInt(output_buffer, static_cast(type)); + HessianUtils::writeInt(output_buffer, + static_cast::type>(type)); // Serialized response content. HessianUtils::writeString(output_buffer, content); diff --git a/test/extensions/filters/network/dubbo_proxy/BUILD b/test/extensions/filters/network/dubbo_proxy/BUILD index 08eb97527f276..e537f7d3c9831 100644 --- a/test/extensions/filters/network/dubbo_proxy/BUILD +++ b/test/extensions/filters/network/dubbo_proxy/BUILD @@ -73,6 +73,7 @@ envoy_extension_cc_test( ":mocks_lib", ":utility_lib", "//source/extensions/filters/network/dubbo_proxy:hessian_deserializer_impl_lib", + "//source/extensions/filters/network/dubbo_proxy:hessian_utils_lib", "//test/mocks/server:server_mocks", ], ) @@ -145,6 +146,7 @@ envoy_extension_cc_test( "//source/extensions/filters/network/dubbo_proxy:app_exception_lib", "//source/extensions/filters/network/dubbo_proxy:dubbo_protocol_impl_lib", "//source/extensions/filters/network/dubbo_proxy:hessian_deserializer_impl_lib", + "//source/extensions/filters/network/dubbo_proxy:hessian_utils_lib", "//source/extensions/filters/network/dubbo_proxy:metadata_lib", ], ) diff --git a/test/extensions/filters/network/dubbo_proxy/app_exception_test.cc b/test/extensions/filters/network/dubbo_proxy/app_exception_test.cc index 856007134d701..251dc114da8c0 100644 --- a/test/extensions/filters/network/dubbo_proxy/app_exception_test.cc +++ b/test/extensions/filters/network/dubbo_proxy/app_exception_test.cc @@ -3,6 +3,7 @@ #include "extensions/filters/network/dubbo_proxy/dubbo_protocol_impl.h" #include "extensions/filters/network/dubbo_proxy/filters/filter.h" #include "extensions/filters/network/dubbo_proxy/hessian_deserializer_impl.h" +#include "extensions/filters/network/dubbo_proxy/hessian_utils.h" #include "extensions/filters/network/dubbo_proxy/metadata.h" #include "test/test_common/test_base.h" @@ -27,6 +28,7 @@ class AppExceptionTest : public TestBase { TEST_F(AppExceptionTest, Encode) { std::string mock_message("invalid method name 'Sub'"); AppException app_exception(ResponseStatus::ServiceNotFound, mock_message); + size_t expect_body_size = mock_message.size() + sizeof(app_exception.response_type_); Buffer::OwnedImpl buffer; metadata_->setSerializationType(SerializationType::Hessian); @@ -35,12 +37,23 @@ TEST_F(AppExceptionTest, Encode) { { EXPECT_EQ(app_exception.encode(*(metadata_.get()), protocol_, deserializer_, buffer), DubboFilters::DirectResponse::ResponseType::Exception); - MessageMetadataSharedPtr metadata = std::make_shared(); EXPECT_TRUE(protocol_.decode(buffer, &context_, metadata)); + EXPECT_EQ(context_.body_size_, expect_body_size); EXPECT_EQ(metadata->message_type(), MessageType::Response); buffer.drain(context_.header_size_); + // Verify the response type and content. + size_t hessian_int_size; + int type_value = HessianUtils::peekInt(buffer, &hessian_int_size); + EXPECT_EQ(static_cast(app_exception.response_type_), static_cast(type_value)); + + size_t hessian_string_size; + std::string message = HessianUtils::peekString(buffer, &hessian_string_size, sizeof(uint8_t)); + EXPECT_EQ(mock_message, message); + + EXPECT_EQ(buffer.length(), hessian_int_size + hessian_string_size); + auto result = deserializer_.deserializeRpcResult(buffer, context_.body_size_); EXPECT_TRUE(result->hasException()); buffer.drain(buffer.length()); @@ -53,9 +66,21 @@ TEST_F(AppExceptionTest, Encode) { MessageMetadataSharedPtr metadata = std::make_shared(); EXPECT_TRUE(protocol_.decode(buffer, &context_, metadata)); + EXPECT_EQ(context_.body_size_, expect_body_size); EXPECT_EQ(metadata->message_type(), MessageType::Response); buffer.drain(context_.header_size_); + // Verify the response type and content. + size_t hessian_int_size; + int type_value = HessianUtils::peekInt(buffer, &hessian_int_size); + EXPECT_EQ(static_cast(app_exception.response_type_), static_cast(type_value)); + + size_t hessian_string_size; + std::string message = HessianUtils::peekString(buffer, &hessian_string_size, sizeof(uint8_t)); + EXPECT_EQ(mock_message, message); + + EXPECT_EQ(buffer.length(), hessian_int_size + hessian_string_size); + auto result = deserializer_.deserializeRpcResult(buffer, context_.body_size_); EXPECT_FALSE(result->hasException()); buffer.drain(buffer.length()); @@ -68,9 +93,21 @@ TEST_F(AppExceptionTest, Encode) { MessageMetadataSharedPtr metadata = std::make_shared(); EXPECT_TRUE(protocol_.decode(buffer, &context_, metadata)); + EXPECT_EQ(context_.body_size_, expect_body_size); EXPECT_EQ(metadata->message_type(), MessageType::Response); buffer.drain(context_.header_size_); + // Verify the response type and content. + size_t hessian_int_size; + int type_value = HessianUtils::peekInt(buffer, &hessian_int_size); + EXPECT_EQ(static_cast(app_exception.response_type_), static_cast(type_value)); + + size_t hessian_string_size; + std::string message = HessianUtils::peekString(buffer, &hessian_string_size, sizeof(uint8_t)); + EXPECT_EQ(mock_message, message); + + EXPECT_EQ(buffer.length(), hessian_int_size + hessian_string_size); + auto result = deserializer_.deserializeRpcResult(buffer, context_.body_size_); EXPECT_TRUE(result->hasException()); } diff --git a/test/extensions/filters/network/dubbo_proxy/dubbo_protocol_impl_test.cc b/test/extensions/filters/network/dubbo_proxy/dubbo_protocol_impl_test.cc index ded61a113f46e..3407d4bb89880 100644 --- a/test/extensions/filters/network/dubbo_proxy/dubbo_protocol_impl_test.cc +++ b/test/extensions/filters/network/dubbo_proxy/dubbo_protocol_impl_test.cc @@ -116,6 +116,29 @@ TEST(DubboProtocolImplTest, DubboProtocolConfigFactory) { EXPECT_EQ(protocol->type(), ProtocolType::Dubbo); } +TEST(DubboProtocolImplTest, encode) { + MessageMetadata metadata; + metadata.setMessageType(MessageType::Response); + metadata.setResponseStatus(ResponseStatus::ServiceNotFound); + metadata.setSerializationType(SerializationType::Hessian); + metadata.setRequestId(100); + + Buffer::OwnedImpl buffer; + DubboProtocolImpl dubbo_protocol; + int32_t expect_body_size = 100; + EXPECT_TRUE(dubbo_protocol.encode(buffer, expect_body_size, metadata)); + + Protocol::Context context; + MessageMetadataSharedPtr output_metadata = std::make_shared(); + EXPECT_TRUE(dubbo_protocol.decode(buffer, &context, output_metadata)); + + EXPECT_EQ(metadata.message_type(), output_metadata->message_type()); + EXPECT_EQ(metadata.response_status().value(), output_metadata->response_status().value()); + EXPECT_EQ(metadata.serialization_type(), output_metadata->serialization_type()); + EXPECT_EQ(metadata.request_id(), output_metadata->request_id()); + EXPECT_EQ(context.body_size_, expect_body_size); +} + } // namespace DubboProxy } // namespace NetworkFilters } // namespace Extensions diff --git a/test/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl_test.cc b/test/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl_test.cc index a2c5b22a08c86..bff7788bd253a 100644 --- a/test/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl_test.cc +++ b/test/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl_test.cc @@ -1,4 +1,5 @@ #include "extensions/filters/network/dubbo_proxy/hessian_deserializer_impl.h" +#include "extensions/filters/network/dubbo_proxy/hessian_utils.h" #include "test/extensions/filters/network/dubbo_proxy/mocks.h" #include "test/extensions/filters/network/dubbo_proxy/utility.h" @@ -130,6 +131,29 @@ TEST(HessianProtocolTest, HessianDeserializerConfigFactory) { EXPECT_EQ(deserializer->type(), SerializationType::Hessian); } +TEST(HessianProtocolTest, serializeRpcResult) { + Buffer::OwnedImpl buffer; + std::string mock_response("invalid method name 'Add'"); + RpcResponseType mock_response_type = RpcResponseType::ResponseWithException; + HessianDeserializerImpl deserializer; + + deserializer.serializeRpcResult(buffer, mock_response, mock_response_type); + + size_t hessian_int_size; + int type_value = HessianUtils::peekInt(buffer, &hessian_int_size); + EXPECT_EQ(static_cast(mock_response_type), static_cast(type_value)); + + size_t hessian_string_size; + std::string content = HessianUtils::peekString(buffer, &hessian_string_size, sizeof(uint8_t)); + EXPECT_EQ(mock_response, content); + + EXPECT_EQ(buffer.length(), hessian_int_size + hessian_string_size); + + size_t body_size = mock_response.size() + sizeof(mock_response_type); + auto result = deserializer.deserializeRpcResult(buffer, body_size); + EXPECT_TRUE(result->hasException()); +} + } // namespace DubboProxy } // namespace NetworkFilters } // namespace Extensions From 2376ed9a8a1ae4d6b914df06aeae03902631d73c Mon Sep 17 00:00:00 2001 From: "leilei.gll" Date: Fri, 22 Feb 2019 16:15:27 +0800 Subject: [PATCH 14/23] Fix body_size calculation error in AppException encoding Signed-off-by: leilei.gll --- .../network/dubbo_proxy/app_exception.cc | 12 +++- .../network/dubbo_proxy/deserializer.h | 6 +- .../dubbo_proxy/hessian_deserializer_impl.cc | 17 ++++-- .../dubbo_proxy/hessian_deserializer_impl.h | 4 +- .../network/dubbo_proxy/hessian_utils.cc | 36 +++++++----- .../network/dubbo_proxy/hessian_utils.h | 4 +- .../network/dubbo_proxy/app_exception_test.cc | 12 ++-- .../network/dubbo_proxy/hessian_utils_test.cc | 58 +++++++++++++++++++ .../filters/network/dubbo_proxy/mocks.h | 2 +- 9 files changed, 116 insertions(+), 35 deletions(-) diff --git a/source/extensions/filters/network/dubbo_proxy/app_exception.cc b/source/extensions/filters/network/dubbo_proxy/app_exception.cc index 79aea955f6ea4..5cc9d4ff97687 100644 --- a/source/extensions/filters/network/dubbo_proxy/app_exception.cc +++ b/source/extensions/filters/network/dubbo_proxy/app_exception.cc @@ -20,17 +20,23 @@ AppException::ResponseType AppException::encode(MessageMetadata& metadata, DubboProxy::Protocol& protocol, Deserializer& deserializer, Buffer::Instance& buffer) const { + ASSERT(buffer.length() == 0); + ENVOY_LOG(debug, "err {}", what()); + // Serialize the response content to get the serialized response length. + const std::string& response = what(); + size_t serialized_body_size = deserializer.serializeRpcResult(buffer, response, response_type_); + metadata.setResponseStatus(status_); metadata.setMessageType(MessageType::Response); - const std::string& response = what(); - if (!protocol.encode(buffer, response.size() + sizeof(response_type_), metadata)) { + Buffer::OwnedImpl protocol_buffer; + if (!protocol.encode(protocol_buffer, serialized_body_size, metadata)) { throw EnvoyException("failed to encode local reply message"); } - deserializer.serializeRpcResult(buffer, response, response_type_); + buffer.prepend(protocol_buffer); return DirectResponse::ResponseType::Exception; } diff --git a/source/extensions/filters/network/dubbo_proxy/deserializer.h b/source/extensions/filters/network/dubbo_proxy/deserializer.h index 6dcc36b2944bb..bc67abcb3bc8a 100644 --- a/source/extensions/filters/network/dubbo_proxy/deserializer.h +++ b/source/extensions/filters/network/dubbo_proxy/deserializer.h @@ -115,10 +115,10 @@ class Deserializer { * @param output_buffer store the serialized data * @param content the rpc response content * @param type the rpc response type - * @throws EnvoyException if the data is not valid for this serialization + * @return size_t the length of the serialized content */ - virtual void serializeRpcResult(Buffer::Instance& output_buffer, const std::string& content, - RpcResponseType type) PURE; + virtual size_t serializeRpcResult(Buffer::Instance& output_buffer, const std::string& content, + RpcResponseType type) PURE; }; typedef std::unique_ptr DeserializerPtr; diff --git a/source/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl.cc b/source/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl.cc index f4b8b097c55d5..d6cc65bac5f1a 100644 --- a/source/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl.cc +++ b/source/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl.cc @@ -78,14 +78,21 @@ RpcResultPtr HessianDeserializerImpl::deserializeRpcResult(Buffer::Instance& buf return result; } -void HessianDeserializerImpl::serializeRpcResult(Buffer::Instance& output_buffer, - const std::string& content, RpcResponseType type) { +size_t HessianDeserializerImpl::serializeRpcResult(Buffer::Instance& output_buffer, + const std::string& content, + RpcResponseType type) { + size_t origin_length = output_buffer.length(); + // The serialized response type is compact int. - HessianUtils::writeInt(output_buffer, - static_cast::type>(type)); + size_t serialized_size = HessianUtils::writeInt( + output_buffer, static_cast::type>(type)); // Serialized response content. - HessianUtils::writeString(output_buffer, content); + serialized_size += HessianUtils::writeString(output_buffer, content); + + ASSERT(output_buffer.length() - origin_length == serialized_size); + + return serialized_size; } class HessianDeserializerConfigFactory : public DeserializerFactoryBase { diff --git a/source/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl.h b/source/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl.h index 1f24c07c2fefd..7544e362f3440 100644 --- a/source/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl.h +++ b/source/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl.h @@ -17,8 +17,8 @@ class HessianDeserializerImpl : public Deserializer { virtual RpcInvocationPtr deserializeRpcInvocation(Buffer::Instance& buffer, size_t body_size) override; virtual RpcResultPtr deserializeRpcResult(Buffer::Instance& buffer, size_t body_size) override; - virtual void serializeRpcResult(Buffer::Instance& output_buffer, const std::string& content, - RpcResponseType type) override; + virtual size_t serializeRpcResult(Buffer::Instance& output_buffer, const std::string& content, + RpcResponseType type) override; }; } // namespace DubboProxy diff --git a/source/extensions/filters/network/dubbo_proxy/hessian_utils.cc b/source/extensions/filters/network/dubbo_proxy/hessian_utils.cc index 1a23ee923dafc..f45ee26cc7b0c 100644 --- a/source/extensions/filters/network/dubbo_proxy/hessian_utils.cc +++ b/source/extensions/filters/network/dubbo_proxy/hessian_utils.cc @@ -26,13 +26,13 @@ typename std::enable_if::value, T>::type leftShift(T left, uin inline void addByte(Buffer::Instance& buffer, const uint8_t value) { buffer.add(&value, 1); } -void addSeq(Buffer::Instance& buffer, const std::initializer_list values) { +void addSeq(Buffer::Instance& buffer, const std::initializer_list& values) { for (const int8_t& value : values) { buffer.add(&value, 1); } } -void doWriteString(Buffer::Instance& instance, const absl::string_view& str_view) { +size_t doWriteString(Buffer::Instance& instance, const absl::string_view& str_view) { const size_t length = str_view.length(); constexpr size_t str_max_length = 0xffff; constexpr size_t two_octet_max_lenth = 1024; @@ -40,30 +40,36 @@ void doWriteString(Buffer::Instance& instance, const absl::string_view& str_view if (length < 32) { addByte(instance, static_cast(length)); instance.add(str_view.data(), str_view.length()); - return; + return length + sizeof(uint8_t); } if (length < two_octet_max_lenth) { uint8_t code = length / 256; // 0x30 + length / 0x100 must less than 0x34 uint8_t remain = length % 256; - addSeq(instance, {static_cast(0x30 + code), remain}); + std::initializer_list values{static_cast(0x30 + code), remain}; + addSeq(instance, values); instance.add(str_view.data(), str_view.length()); - return; + return length + values.size(); } if (length <= str_max_length) { uint8_t code = length / 256; uint8_t remain = length % 256; - addSeq(instance, {'S', code, remain}); + std::initializer_list values{'S', code, remain}; + addSeq(instance, values); instance.add(str_view.data(), str_view.length()); - return; + return length + values.size(); } - addSeq(instance, {0x52, 0xff, 0xff}); + std::initializer_list values{0x52, 0xff, 0xff}; + addSeq(instance, values); instance.add(str_view.data(), str_max_length); - doWriteString(instance, - absl::string_view(str_view.data() + str_max_length, length - str_max_length)); - return; + size_t size = str_max_length + values.size(); + ASSERT(size == (str_max_length + values.size())); + + size_t child_size = doWriteString( + instance, absl::string_view(str_view.data() + str_max_length, length - str_max_length)); + return child_size + size; } } // namespace @@ -558,14 +564,14 @@ std::string HessianUtils::readByte(Buffer::Instance& buffer) { return result; } -void HessianUtils::writeString(Buffer::Instance& buffer, const absl::string_view& str) { - doWriteString(buffer, str); - return; +size_t HessianUtils::writeString(Buffer::Instance& buffer, const absl::string_view& str) { + return doWriteString(buffer, str); } -void HessianUtils::writeInt(Buffer::Instance& buffer, uint8_t value) { +size_t HessianUtils::writeInt(Buffer::Instance& buffer, uint8_t value) { // Compact int buffer.writeByte(0x90 + value); + return sizeof(uint8_t); } } // namespace DubboProxy diff --git a/source/extensions/filters/network/dubbo_proxy/hessian_utils.h b/source/extensions/filters/network/dubbo_proxy/hessian_utils.h index 6942e3dc31b89..9280e216583e6 100644 --- a/source/extensions/filters/network/dubbo_proxy/hessian_utils.h +++ b/source/extensions/filters/network/dubbo_proxy/hessian_utils.h @@ -36,8 +36,8 @@ class HessianUtils { static std::chrono::milliseconds readDate(Buffer::Instance& buffer); static std::string readByte(Buffer::Instance& buffer); - static void writeString(Buffer::Instance& buffer, const absl::string_view& str); - static void writeInt(Buffer::Instance& buffer, uint8_t value); + static size_t writeString(Buffer::Instance& buffer, const absl::string_view& str); + static size_t writeInt(Buffer::Instance& buffer, uint8_t value); }; } // namespace DubboProxy diff --git a/test/extensions/filters/network/dubbo_proxy/app_exception_test.cc b/test/extensions/filters/network/dubbo_proxy/app_exception_test.cc index 251dc114da8c0..24502c7619900 100644 --- a/test/extensions/filters/network/dubbo_proxy/app_exception_test.cc +++ b/test/extensions/filters/network/dubbo_proxy/app_exception_test.cc @@ -28,9 +28,13 @@ class AppExceptionTest : public TestBase { TEST_F(AppExceptionTest, Encode) { std::string mock_message("invalid method name 'Sub'"); AppException app_exception(ResponseStatus::ServiceNotFound, mock_message); - size_t expect_body_size = mock_message.size() + sizeof(app_exception.response_type_); Buffer::OwnedImpl buffer; + size_t expect_body_size = + HessianUtils::writeString(buffer, mock_message) + + HessianUtils::writeInt(buffer, static_cast(app_exception.response_type_)); + buffer.drain(buffer.length()); + metadata_->setSerializationType(SerializationType::Hessian); metadata_->setRequestId(0); @@ -39,7 +43,7 @@ TEST_F(AppExceptionTest, Encode) { DubboFilters::DirectResponse::ResponseType::Exception); MessageMetadataSharedPtr metadata = std::make_shared(); EXPECT_TRUE(protocol_.decode(buffer, &context_, metadata)); - EXPECT_EQ(context_.body_size_, expect_body_size); + EXPECT_EQ(expect_body_size, context_.body_size_); EXPECT_EQ(metadata->message_type(), MessageType::Response); buffer.drain(context_.header_size_); @@ -66,7 +70,7 @@ TEST_F(AppExceptionTest, Encode) { MessageMetadataSharedPtr metadata = std::make_shared(); EXPECT_TRUE(protocol_.decode(buffer, &context_, metadata)); - EXPECT_EQ(context_.body_size_, expect_body_size); + EXPECT_EQ(expect_body_size, context_.body_size_); EXPECT_EQ(metadata->message_type(), MessageType::Response); buffer.drain(context_.header_size_); @@ -93,7 +97,7 @@ TEST_F(AppExceptionTest, Encode) { MessageMetadataSharedPtr metadata = std::make_shared(); EXPECT_TRUE(protocol_.decode(buffer, &context_, metadata)); - EXPECT_EQ(context_.body_size_, expect_body_size); + EXPECT_EQ(expect_body_size, context_.body_size_); EXPECT_EQ(metadata->message_type(), MessageType::Response); buffer.drain(context_.header_size_); diff --git a/test/extensions/filters/network/dubbo_proxy/hessian_utils_test.cc b/test/extensions/filters/network/dubbo_proxy/hessian_utils_test.cc index dfb721f30f4be..220c2d48bc3b4 100644 --- a/test/extensions/filters/network/dubbo_proxy/hessian_utils_test.cc +++ b/test/extensions/filters/network/dubbo_proxy/hessian_utils_test.cc @@ -775,6 +775,64 @@ TEST(HessianUtilsTest, peekByte) { } } +TEST(HessianUtilsTest, writeString) { + const size_t max = 65535; + const size_t segment_mark_length = 3; + + { + const std::string append_content("b"); + const size_t append_content_hessian_length = 1; + std::string message(max, 'a'); + message.append(append_content); + size_t expect_serialized_size = + max + segment_mark_length + append_content_hessian_length + append_content.size(); + + Buffer::OwnedImpl buffer; + size_t size = HessianUtils::writeString(buffer, message); + EXPECT_EQ(size, expect_serialized_size); + } + + { + const std::string append_content(33, 'b'); + const size_t append_content_hessian_length = 2; + std::string message(max, 'a'); + message.append(append_content); + size_t expect_serialized_size = + max + segment_mark_length + append_content_hessian_length + append_content.size(); + + Buffer::OwnedImpl buffer; + size_t size = HessianUtils::writeString(buffer, message); + EXPECT_EQ(size, expect_serialized_size); + } + + { + const std::string append_content(1025, 'b'); + const size_t append_content_hessian_length = 3; + std::string message(max, 'a'); + message.append(append_content); + size_t expect_serialized_size = + max + segment_mark_length + append_content_hessian_length + append_content.size(); + + Buffer::OwnedImpl buffer; + size_t size = HessianUtils::writeString(buffer, message); + EXPECT_EQ(size, expect_serialized_size); + } + + { + const std::string append_content(1025, 'b'); + const size_t append_content_hessian_length = 3; + const size_t max_size = 2 * max; + std::string message(max_size, 'a'); + message.append(append_content); + size_t expect_serialized_size = + max * 2 + segment_mark_length * 2 + append_content_hessian_length + append_content.size(); + + Buffer::OwnedImpl buffer; + size_t size = HessianUtils::writeString(buffer, message); + EXPECT_EQ(size, expect_serialized_size); + } +} + } // namespace DubboProxy } // namespace NetworkFilters } // namespace Extensions diff --git a/test/extensions/filters/network/dubbo_proxy/mocks.h b/test/extensions/filters/network/dubbo_proxy/mocks.h index d6e178ee65347..cbe6c1e13d365 100644 --- a/test/extensions/filters/network/dubbo_proxy/mocks.h +++ b/test/extensions/filters/network/dubbo_proxy/mocks.h @@ -69,7 +69,7 @@ class MockDeserializer : public Deserializer { MOCK_CONST_METHOD0(type, SerializationType()); MOCK_METHOD2(deserializeRpcInvocation, RpcInvocationPtr(Buffer::Instance&, size_t)); MOCK_METHOD2(deserializeRpcResult, RpcResultPtr(Buffer::Instance&, size_t)); - MOCK_METHOD3(serializeRpcResult, void(Buffer::Instance&, const std::string&, RpcResponseType)); + MOCK_METHOD3(serializeRpcResult, size_t(Buffer::Instance&, const std::string&, RpcResponseType)); std::string name_{"mockDeserializer"}; SerializationType type_{SerializationType::Hessian}; From 379aeba52eff99e5d8b06561084a51e64619712f Mon Sep 17 00:00:00 2001 From: "leilei.gll" Date: Fri, 22 Feb 2019 17:50:59 +0800 Subject: [PATCH 15/23] Fix the RpcResponseType spelling error and optimize the AppException implementation Signed-off-by: leilei.gll --- .../network/dubbo_proxy/app_exception.cc | 2 +- .../network/dubbo_proxy/app_exception.h | 2 +- .../dubbo_proxy/hessian_deserializer_impl.cc | 2 +- .../network/dubbo_proxy/app_exception_test.cc | 99 +++++-------------- 4 files changed, 25 insertions(+), 80 deletions(-) diff --git a/source/extensions/filters/network/dubbo_proxy/app_exception.cc b/source/extensions/filters/network/dubbo_proxy/app_exception.cc index 5cc9d4ff97687..806ee3e3015b8 100644 --- a/source/extensions/filters/network/dubbo_proxy/app_exception.cc +++ b/source/extensions/filters/network/dubbo_proxy/app_exception.cc @@ -14,7 +14,7 @@ AppException::AppException(ResponseStatus status, const std::string& what) response_type_(RpcResponseType::ResponseWithException) {} AppException::AppException(const AppException& ex) - : EnvoyException(ex.what()), status_(ex.status_) {} + : EnvoyException(ex.what()), status_(ex.status_), response_type_(ex.response_type_) {} AppException::ResponseType AppException::encode(MessageMetadata& metadata, DubboProxy::Protocol& protocol, diff --git a/source/extensions/filters/network/dubbo_proxy/app_exception.h b/source/extensions/filters/network/dubbo_proxy/app_exception.h index 977107069cd26..4917b30894651 100644 --- a/source/extensions/filters/network/dubbo_proxy/app_exception.h +++ b/source/extensions/filters/network/dubbo_proxy/app_exception.h @@ -23,7 +23,7 @@ struct AppException : public EnvoyException, Buffer::Instance& buffer) const override; const ResponseStatus status_; - RpcResponseType response_type_; + const RpcResponseType response_type_; }; } // namespace DubboProxy diff --git a/source/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl.cc b/source/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl.cc index d6cc65bac5f1a..d1b1eaf73d345 100644 --- a/source/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl.cc +++ b/source/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl.cc @@ -85,7 +85,7 @@ size_t HessianDeserializerImpl::serializeRpcResult(Buffer::Instance& output_buff // The serialized response type is compact int. size_t serialized_size = HessianUtils::writeInt( - output_buffer, static_cast::type>(type)); + output_buffer, static_cast::type>(type)); // Serialized response content. serialized_size += HessianUtils::writeString(output_buffer, content); diff --git a/test/extensions/filters/network/dubbo_proxy/app_exception_test.cc b/test/extensions/filters/network/dubbo_proxy/app_exception_test.cc index 24502c7619900..d2a4d85f61c60 100644 --- a/test/extensions/filters/network/dubbo_proxy/app_exception_test.cc +++ b/test/extensions/filters/network/dubbo_proxy/app_exception_test.cc @@ -38,83 +38,28 @@ TEST_F(AppExceptionTest, Encode) { metadata_->setSerializationType(SerializationType::Hessian); metadata_->setRequestId(0); - { - EXPECT_EQ(app_exception.encode(*(metadata_.get()), protocol_, deserializer_, buffer), - DubboFilters::DirectResponse::ResponseType::Exception); - MessageMetadataSharedPtr metadata = std::make_shared(); - EXPECT_TRUE(protocol_.decode(buffer, &context_, metadata)); - EXPECT_EQ(expect_body_size, context_.body_size_); - EXPECT_EQ(metadata->message_type(), MessageType::Response); - buffer.drain(context_.header_size_); - - // Verify the response type and content. - size_t hessian_int_size; - int type_value = HessianUtils::peekInt(buffer, &hessian_int_size); - EXPECT_EQ(static_cast(app_exception.response_type_), static_cast(type_value)); - - size_t hessian_string_size; - std::string message = HessianUtils::peekString(buffer, &hessian_string_size, sizeof(uint8_t)); - EXPECT_EQ(mock_message, message); - - EXPECT_EQ(buffer.length(), hessian_int_size + hessian_string_size); - - auto result = deserializer_.deserializeRpcResult(buffer, context_.body_size_); - EXPECT_TRUE(result->hasException()); - buffer.drain(buffer.length()); - } - - { - app_exception.response_type_ = RpcResponseType::ResponseValueWithAttachments; - EXPECT_EQ(app_exception.encode(*(metadata_.get()), protocol_, deserializer_, buffer), - DubboFilters::DirectResponse::ResponseType::Exception); - - MessageMetadataSharedPtr metadata = std::make_shared(); - EXPECT_TRUE(protocol_.decode(buffer, &context_, metadata)); - EXPECT_EQ(expect_body_size, context_.body_size_); - EXPECT_EQ(metadata->message_type(), MessageType::Response); - buffer.drain(context_.header_size_); - - // Verify the response type and content. - size_t hessian_int_size; - int type_value = HessianUtils::peekInt(buffer, &hessian_int_size); - EXPECT_EQ(static_cast(app_exception.response_type_), static_cast(type_value)); - - size_t hessian_string_size; - std::string message = HessianUtils::peekString(buffer, &hessian_string_size, sizeof(uint8_t)); - EXPECT_EQ(mock_message, message); - - EXPECT_EQ(buffer.length(), hessian_int_size + hessian_string_size); - - auto result = deserializer_.deserializeRpcResult(buffer, context_.body_size_); - EXPECT_FALSE(result->hasException()); - buffer.drain(buffer.length()); - } - - { - app_exception.response_type_ = RpcResponseType::ResponseWithValue; - EXPECT_EQ(app_exception.encode(*(metadata_.get()), protocol_, deserializer_, buffer), - DubboFilters::DirectResponse::ResponseType::Exception); - - MessageMetadataSharedPtr metadata = std::make_shared(); - EXPECT_TRUE(protocol_.decode(buffer, &context_, metadata)); - EXPECT_EQ(expect_body_size, context_.body_size_); - EXPECT_EQ(metadata->message_type(), MessageType::Response); - buffer.drain(context_.header_size_); - - // Verify the response type and content. - size_t hessian_int_size; - int type_value = HessianUtils::peekInt(buffer, &hessian_int_size); - EXPECT_EQ(static_cast(app_exception.response_type_), static_cast(type_value)); - - size_t hessian_string_size; - std::string message = HessianUtils::peekString(buffer, &hessian_string_size, sizeof(uint8_t)); - EXPECT_EQ(mock_message, message); - - EXPECT_EQ(buffer.length(), hessian_int_size + hessian_string_size); - - auto result = deserializer_.deserializeRpcResult(buffer, context_.body_size_); - EXPECT_TRUE(result->hasException()); - } + EXPECT_EQ(app_exception.encode(*(metadata_.get()), protocol_, deserializer_, buffer), + DubboFilters::DirectResponse::ResponseType::Exception); + MessageMetadataSharedPtr metadata = std::make_shared(); + EXPECT_TRUE(protocol_.decode(buffer, &context_, metadata)); + EXPECT_EQ(expect_body_size, context_.body_size_); + EXPECT_EQ(metadata->message_type(), MessageType::Response); + buffer.drain(context_.header_size_); + + // Verify the response type and content. + size_t hessian_int_size; + int type_value = HessianUtils::peekInt(buffer, &hessian_int_size); + EXPECT_EQ(static_cast(app_exception.response_type_), static_cast(type_value)); + + size_t hessian_string_size; + std::string message = HessianUtils::peekString(buffer, &hessian_string_size, sizeof(uint8_t)); + EXPECT_EQ(mock_message, message); + + EXPECT_EQ(buffer.length(), hessian_int_size + hessian_string_size); + + auto result = deserializer_.deserializeRpcResult(buffer, context_.body_size_); + EXPECT_TRUE(result->hasException()); + buffer.drain(buffer.length()); } } // namespace DubboProxy From a9fc68bff2feb775c44fa34422119f4b353252fd Mon Sep 17 00:00:00 2001 From: "leilei.gll" Date: Wed, 27 Feb 2019 15:17:04 +0800 Subject: [PATCH 16/23] Add some unit test cases Signed-off-by: leilei.gll --- .../network/dubbo_proxy/router/router_impl.cc | 3 + .../dubbo_proxy/dubbo_protocol_impl_test.cc | 86 +++++++++++++++++++ .../hessian_deserializer_impl_test.cc | 10 +++ .../network/dubbo_proxy/router_test.cc | 76 +++++++++++++++- 4 files changed, 172 insertions(+), 3 deletions(-) diff --git a/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc b/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc index 1b4cf8767e6a8..ff22478608c43 100644 --- a/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc +++ b/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc @@ -42,6 +42,7 @@ Network::FilterStatus Router::transportEnd() { // No response expected upstream_request_->onResponseComplete(); cleanup(); + ENVOY_LOG(debug, "dubbo upstream request: the message is one-way and no response is required"); } filter_complete_ = true; @@ -207,12 +208,14 @@ void Router::UpstreamRequest::resetStream() { ASSERT(!conn_data_); conn_pool_handle_->cancel(Tcp::ConnectionPool::CancelPolicy::Default); conn_pool_handle_ = nullptr; + ENVOY_LOG(debug, "dubbo upstream request: reset connection pool handler"); } if (conn_data_) { ASSERT(!conn_pool_handle_); conn_data_->connection().close(Network::ConnectionCloseType::NoFlush); conn_data_.reset(); + ENVOY_LOG(debug, "dubbo upstream request: reset connection data"); } } diff --git a/test/extensions/filters/network/dubbo_proxy/dubbo_protocol_impl_test.cc b/test/extensions/filters/network/dubbo_proxy/dubbo_protocol_impl_test.cc index 3407d4bb89880..550935b1d9e20 100644 --- a/test/extensions/filters/network/dubbo_proxy/dubbo_protocol_impl_test.cc +++ b/test/extensions/filters/network/dubbo_proxy/dubbo_protocol_impl_test.cc @@ -139,6 +139,92 @@ TEST(DubboProtocolImplTest, encode) { EXPECT_EQ(context.body_size_, expect_body_size); } +TEST(DubboProtocolImplTest, decode) { + Buffer::OwnedImpl buffer; + MessageMetadataSharedPtr metadata; + Protocol::Context context; + DubboProtocolImpl dubbo_protocol; + + // metadata is nullptr + EXPECT_THROW_WITH_MESSAGE(dubbo_protocol.decode(buffer, &context, metadata), EnvoyException, + "invalid metadata parameter"); + + metadata = std::make_shared(); + + // Invalid message header size + EXPECT_FALSE(dubbo_protocol.decode(buffer, &context, metadata)); + + // Invalid dubbo magic number + { + addInt64(buffer, 0); + addInt64(buffer, 0); + EXPECT_THROW_WITH_MESSAGE(dubbo_protocol.decode(buffer, &context), EnvoyException, + "invalid dubbo message magic number 0"); + buffer.drain(buffer.length()); + } + + // Invalid message body size + { + buffer.add(std::string({'\xda', '\xbb', '\xc2', 0x00})); + addInt64(buffer, 1); + addInt32(buffer, DubboProtocolImpl::MaxBodySize + 1); + std::string exception_string = + fmt::format("invalid dubbo message size {}", DubboProtocolImpl::MaxBodySize + 1); + EXPECT_THROW_WITH_MESSAGE(dubbo_protocol.decode(buffer, &context), EnvoyException, + exception_string); + buffer.drain(buffer.length()); + } + + // Invalid serialization type + { + buffer.add(std::string({'\xda', '\xbb', '\xc3', 0x00})); + addInt64(buffer, 1); + addInt32(buffer, 0xff); + EXPECT_THROW_WITH_MESSAGE(dubbo_protocol.decode(buffer, &context, metadata), EnvoyException, + "invalid dubbo message serialization type 3"); + buffer.drain(buffer.length()); + } + + // Invalid response status + { + buffer.add(std::string({'\xda', '\xbb', 0x42, 0x00})); + addInt64(buffer, 1); + addInt32(buffer, 0xff); + EXPECT_THROW_WITH_MESSAGE(dubbo_protocol.decode(buffer, &context, metadata), EnvoyException, + "invalid dubbo message response status 0"); + buffer.drain(buffer.length()); + } + + // The dubbo request message + { + Protocol::Context context; + buffer.add(std::string({'\xda', '\xbb', '\xc2', 0x00})); + addInt64(buffer, 1); + addInt32(buffer, 1); + EXPECT_TRUE(dubbo_protocol.decode(buffer, &context, metadata)); + EXPECT_EQ(1, context.body_size_); + EXPECT_FALSE(context.is_heartbeat_); + EXPECT_EQ(MessageType::Request, metadata->message_type()); + EXPECT_EQ(1, metadata->request_id()); + EXPECT_EQ(SerializationType::Hessian, metadata->serialization_type()); + buffer.drain(buffer.length()); + } + + // The One-way dubbo request message + { + Protocol::Context context; + buffer.add(std::string({'\xda', '\xbb', '\x82', 0x00})); + addInt64(buffer, 1); + addInt32(buffer, 1); + EXPECT_TRUE(dubbo_protocol.decode(buffer, &context, metadata)); + EXPECT_EQ(1, context.body_size_); + EXPECT_FALSE(context.is_heartbeat_); + EXPECT_EQ(MessageType::Oneway, metadata->message_type()); + EXPECT_EQ(1, metadata->request_id()); + EXPECT_EQ(SerializationType::Hessian, metadata->serialization_type()); + } +} + } // namespace DubboProxy } // namespace NetworkFilters } // namespace Extensions diff --git a/test/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl_test.cc b/test/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl_test.cc index bff7788bd253a..f9bb1b9337a45 100644 --- a/test/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl_test.cc +++ b/test/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl_test.cc @@ -87,6 +87,16 @@ TEST(HessianProtocolTest, deserializeRpcResult) { EXPECT_TRUE(result->hasException()); } + { + Buffer::OwnedImpl buffer; + buffer.add(std::string({ + '\x91', // return type + 0x04, 't', 'e', 's', 't', // return body + })); + auto result = deserializer.deserializeRpcResult(buffer, 4); + EXPECT_TRUE(result->hasException()); + } + // incorrect body size { Buffer::OwnedImpl buffer; diff --git a/test/extensions/filters/network/dubbo_proxy/router_test.cc b/test/extensions/filters/network/dubbo_proxy/router_test.cc index c1c7e4be0b639..f1b156e613c75 100644 --- a/test/extensions/filters/network/dubbo_proxy/router_test.cc +++ b/test/extensions/filters/network/dubbo_proxy/router_test.cc @@ -107,6 +107,8 @@ class DubboRouterTestBase { .WillOnce(Return(SerializationType::Hessian)); EXPECT_CALL(callbacks_, downstreamProtocolType()).WillOnce(Return(ProtocolType::Dubbo)); + EXPECT_EQ(Network::FilterStatus::Continue, + router_->messageBegin(msg_type, metadata_->request_id(), SerializationType::Hessian)); EXPECT_EQ(Network::FilterStatus::StopIteration, router_->messageEnd(metadata_)); EXPECT_CALL(callbacks_, connection()).WillRepeatedly(Return(&connection_)); @@ -169,9 +171,6 @@ class DubboRouterTestBase { context_.cluster_manager_.tcp_conn_pool_.poolReady(upstream_connection_); return nullptr; })); - - EXPECT_EQ(Network::FilterStatus::Continue, router_->transportBegin()); - EXPECT_NE(nullptr, upstream_callbacks_); } void returnResponse() { @@ -412,6 +411,8 @@ TEST_F(DubboRouterTest, OneWay) { startRequest(MessageType::Oneway); connectUpstream(); + EXPECT_EQ(Network::FilterStatus::Continue, router_->transportEnd()); + destroyRouter(); } @@ -424,6 +425,7 @@ TEST_F(DubboRouterTest, Call) { startRequest(MessageType::Request); connectUpstream(); + EXPECT_EQ(Network::FilterStatus::Continue, router_->transportBegin()); EXPECT_EQ(Network::FilterStatus::Continue, router_->transportEnd()); returnResponse(); @@ -441,6 +443,7 @@ TEST_F(DubboRouterTest, DecoderFilterCallbacks) { startRequest(MessageType::Request); connectUpstream(); + EXPECT_EQ(Network::FilterStatus::Continue, router_->transportBegin()); EXPECT_EQ(Network::FilterStatus::Continue, router_->transportEnd()); Buffer::OwnedImpl buffer; @@ -450,6 +453,73 @@ TEST_F(DubboRouterTest, DecoderFilterCallbacks) { destroyRouter(); } +TEST_F(DubboRouterTest, UpstreamDataReset) { + initializeRouter(); + initializeMetadata(MessageType::Request); + + EXPECT_CALL(callbacks_, startUpstreamResponse(_, _)).Times(1); + EXPECT_CALL(callbacks_, upstreamData(_)) + .WillOnce(Return(DubboFilters::UpstreamResponseStatus::Reset)); + EXPECT_CALL(upstream_connection_, close(Network::ConnectionCloseType::NoFlush)); + + startRequest(MessageType::Request); + connectUpstream(); + + Buffer::OwnedImpl buffer; + buffer.add(std::string("This is the test data")); + router_->onUpstreamData(buffer, false); + + destroyRouter(); +} + +TEST_F(DubboRouterTest, StartRequestWithExistingConnection) { + initializeRouter(); + startRequestWithExistingConnection(MessageType::Request); + + EXPECT_EQ(Network::FilterStatus::Continue, router_->messageEnd(metadata_)); + + destroyRouter(); +} + +TEST_F(DubboRouterTest, DestroyWhileConnecting) { + initializeRouter(); + initializeMetadata(MessageType::Request); + + NiceMock conn_pool_handle; + EXPECT_CALL(context_.cluster_manager_.tcp_conn_pool_, newConnection(_)) + .WillOnce(Invoke([&](Tcp::ConnectionPool::Callbacks&) -> Tcp::ConnectionPool::Cancellable* { + return &conn_pool_handle; + })); + + EXPECT_CALL(conn_pool_handle, cancel(Tcp::ConnectionPool::CancelPolicy::Default)); + + startRequest(MessageType::Request); + router_->onDestroy(); + + destroyRouter(); +} + +TEST_F(DubboRouterTest, LocalClosedWhileResponseComplete) { + initializeRouter(); + initializeMetadata(MessageType::Request); + + EXPECT_CALL(callbacks_, startUpstreamResponse(_, _)).Times(1); + EXPECT_CALL(callbacks_, upstreamData(_)) + .WillOnce(Return(DubboFilters::UpstreamResponseStatus::Complete)); + EXPECT_CALL(callbacks_, sendLocalReply(_, _)).Times(0); + + startRequest(MessageType::Request); + connectUpstream(); + + Buffer::OwnedImpl buffer; + buffer.add(std::string("This is the test data")); + router_->onUpstreamData(buffer, false); + + upstream_connection_.close(Network::ConnectionCloseType::NoFlush); + + destroyRouter(); +} + } // namespace Router } // namespace DubboProxy } // namespace NetworkFilters From d9eb2c17cffb24783c6696a81c5f6593f1147a51 Mon Sep 17 00:00:00 2001 From: "leilei.gll" Date: Thu, 28 Feb 2019 19:33:36 +0800 Subject: [PATCH 17/23] Revert "Add some unit test cases" This reverts commit a9fc68bff2feb775c44fa34422119f4b353252fd. Signed-off-by: leilei.gll --- .../network/dubbo_proxy/router/router_impl.cc | 3 - .../dubbo_proxy/dubbo_protocol_impl_test.cc | 86 ------------------- .../hessian_deserializer_impl_test.cc | 10 --- .../network/dubbo_proxy/router_test.cc | 76 +--------------- 4 files changed, 3 insertions(+), 172 deletions(-) diff --git a/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc b/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc index ff22478608c43..1b4cf8767e6a8 100644 --- a/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc +++ b/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc @@ -42,7 +42,6 @@ Network::FilterStatus Router::transportEnd() { // No response expected upstream_request_->onResponseComplete(); cleanup(); - ENVOY_LOG(debug, "dubbo upstream request: the message is one-way and no response is required"); } filter_complete_ = true; @@ -208,14 +207,12 @@ void Router::UpstreamRequest::resetStream() { ASSERT(!conn_data_); conn_pool_handle_->cancel(Tcp::ConnectionPool::CancelPolicy::Default); conn_pool_handle_ = nullptr; - ENVOY_LOG(debug, "dubbo upstream request: reset connection pool handler"); } if (conn_data_) { ASSERT(!conn_pool_handle_); conn_data_->connection().close(Network::ConnectionCloseType::NoFlush); conn_data_.reset(); - ENVOY_LOG(debug, "dubbo upstream request: reset connection data"); } } diff --git a/test/extensions/filters/network/dubbo_proxy/dubbo_protocol_impl_test.cc b/test/extensions/filters/network/dubbo_proxy/dubbo_protocol_impl_test.cc index 550935b1d9e20..3407d4bb89880 100644 --- a/test/extensions/filters/network/dubbo_proxy/dubbo_protocol_impl_test.cc +++ b/test/extensions/filters/network/dubbo_proxy/dubbo_protocol_impl_test.cc @@ -139,92 +139,6 @@ TEST(DubboProtocolImplTest, encode) { EXPECT_EQ(context.body_size_, expect_body_size); } -TEST(DubboProtocolImplTest, decode) { - Buffer::OwnedImpl buffer; - MessageMetadataSharedPtr metadata; - Protocol::Context context; - DubboProtocolImpl dubbo_protocol; - - // metadata is nullptr - EXPECT_THROW_WITH_MESSAGE(dubbo_protocol.decode(buffer, &context, metadata), EnvoyException, - "invalid metadata parameter"); - - metadata = std::make_shared(); - - // Invalid message header size - EXPECT_FALSE(dubbo_protocol.decode(buffer, &context, metadata)); - - // Invalid dubbo magic number - { - addInt64(buffer, 0); - addInt64(buffer, 0); - EXPECT_THROW_WITH_MESSAGE(dubbo_protocol.decode(buffer, &context), EnvoyException, - "invalid dubbo message magic number 0"); - buffer.drain(buffer.length()); - } - - // Invalid message body size - { - buffer.add(std::string({'\xda', '\xbb', '\xc2', 0x00})); - addInt64(buffer, 1); - addInt32(buffer, DubboProtocolImpl::MaxBodySize + 1); - std::string exception_string = - fmt::format("invalid dubbo message size {}", DubboProtocolImpl::MaxBodySize + 1); - EXPECT_THROW_WITH_MESSAGE(dubbo_protocol.decode(buffer, &context), EnvoyException, - exception_string); - buffer.drain(buffer.length()); - } - - // Invalid serialization type - { - buffer.add(std::string({'\xda', '\xbb', '\xc3', 0x00})); - addInt64(buffer, 1); - addInt32(buffer, 0xff); - EXPECT_THROW_WITH_MESSAGE(dubbo_protocol.decode(buffer, &context, metadata), EnvoyException, - "invalid dubbo message serialization type 3"); - buffer.drain(buffer.length()); - } - - // Invalid response status - { - buffer.add(std::string({'\xda', '\xbb', 0x42, 0x00})); - addInt64(buffer, 1); - addInt32(buffer, 0xff); - EXPECT_THROW_WITH_MESSAGE(dubbo_protocol.decode(buffer, &context, metadata), EnvoyException, - "invalid dubbo message response status 0"); - buffer.drain(buffer.length()); - } - - // The dubbo request message - { - Protocol::Context context; - buffer.add(std::string({'\xda', '\xbb', '\xc2', 0x00})); - addInt64(buffer, 1); - addInt32(buffer, 1); - EXPECT_TRUE(dubbo_protocol.decode(buffer, &context, metadata)); - EXPECT_EQ(1, context.body_size_); - EXPECT_FALSE(context.is_heartbeat_); - EXPECT_EQ(MessageType::Request, metadata->message_type()); - EXPECT_EQ(1, metadata->request_id()); - EXPECT_EQ(SerializationType::Hessian, metadata->serialization_type()); - buffer.drain(buffer.length()); - } - - // The One-way dubbo request message - { - Protocol::Context context; - buffer.add(std::string({'\xda', '\xbb', '\x82', 0x00})); - addInt64(buffer, 1); - addInt32(buffer, 1); - EXPECT_TRUE(dubbo_protocol.decode(buffer, &context, metadata)); - EXPECT_EQ(1, context.body_size_); - EXPECT_FALSE(context.is_heartbeat_); - EXPECT_EQ(MessageType::Oneway, metadata->message_type()); - EXPECT_EQ(1, metadata->request_id()); - EXPECT_EQ(SerializationType::Hessian, metadata->serialization_type()); - } -} - } // namespace DubboProxy } // namespace NetworkFilters } // namespace Extensions diff --git a/test/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl_test.cc b/test/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl_test.cc index f9bb1b9337a45..bff7788bd253a 100644 --- a/test/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl_test.cc +++ b/test/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl_test.cc @@ -87,16 +87,6 @@ TEST(HessianProtocolTest, deserializeRpcResult) { EXPECT_TRUE(result->hasException()); } - { - Buffer::OwnedImpl buffer; - buffer.add(std::string({ - '\x91', // return type - 0x04, 't', 'e', 's', 't', // return body - })); - auto result = deserializer.deserializeRpcResult(buffer, 4); - EXPECT_TRUE(result->hasException()); - } - // incorrect body size { Buffer::OwnedImpl buffer; diff --git a/test/extensions/filters/network/dubbo_proxy/router_test.cc b/test/extensions/filters/network/dubbo_proxy/router_test.cc index f1b156e613c75..c1c7e4be0b639 100644 --- a/test/extensions/filters/network/dubbo_proxy/router_test.cc +++ b/test/extensions/filters/network/dubbo_proxy/router_test.cc @@ -107,8 +107,6 @@ class DubboRouterTestBase { .WillOnce(Return(SerializationType::Hessian)); EXPECT_CALL(callbacks_, downstreamProtocolType()).WillOnce(Return(ProtocolType::Dubbo)); - EXPECT_EQ(Network::FilterStatus::Continue, - router_->messageBegin(msg_type, metadata_->request_id(), SerializationType::Hessian)); EXPECT_EQ(Network::FilterStatus::StopIteration, router_->messageEnd(metadata_)); EXPECT_CALL(callbacks_, connection()).WillRepeatedly(Return(&connection_)); @@ -171,6 +169,9 @@ class DubboRouterTestBase { context_.cluster_manager_.tcp_conn_pool_.poolReady(upstream_connection_); return nullptr; })); + + EXPECT_EQ(Network::FilterStatus::Continue, router_->transportBegin()); + EXPECT_NE(nullptr, upstream_callbacks_); } void returnResponse() { @@ -411,8 +412,6 @@ TEST_F(DubboRouterTest, OneWay) { startRequest(MessageType::Oneway); connectUpstream(); - EXPECT_EQ(Network::FilterStatus::Continue, router_->transportEnd()); - destroyRouter(); } @@ -425,7 +424,6 @@ TEST_F(DubboRouterTest, Call) { startRequest(MessageType::Request); connectUpstream(); - EXPECT_EQ(Network::FilterStatus::Continue, router_->transportBegin()); EXPECT_EQ(Network::FilterStatus::Continue, router_->transportEnd()); returnResponse(); @@ -443,7 +441,6 @@ TEST_F(DubboRouterTest, DecoderFilterCallbacks) { startRequest(MessageType::Request); connectUpstream(); - EXPECT_EQ(Network::FilterStatus::Continue, router_->transportBegin()); EXPECT_EQ(Network::FilterStatus::Continue, router_->transportEnd()); Buffer::OwnedImpl buffer; @@ -453,73 +450,6 @@ TEST_F(DubboRouterTest, DecoderFilterCallbacks) { destroyRouter(); } -TEST_F(DubboRouterTest, UpstreamDataReset) { - initializeRouter(); - initializeMetadata(MessageType::Request); - - EXPECT_CALL(callbacks_, startUpstreamResponse(_, _)).Times(1); - EXPECT_CALL(callbacks_, upstreamData(_)) - .WillOnce(Return(DubboFilters::UpstreamResponseStatus::Reset)); - EXPECT_CALL(upstream_connection_, close(Network::ConnectionCloseType::NoFlush)); - - startRequest(MessageType::Request); - connectUpstream(); - - Buffer::OwnedImpl buffer; - buffer.add(std::string("This is the test data")); - router_->onUpstreamData(buffer, false); - - destroyRouter(); -} - -TEST_F(DubboRouterTest, StartRequestWithExistingConnection) { - initializeRouter(); - startRequestWithExistingConnection(MessageType::Request); - - EXPECT_EQ(Network::FilterStatus::Continue, router_->messageEnd(metadata_)); - - destroyRouter(); -} - -TEST_F(DubboRouterTest, DestroyWhileConnecting) { - initializeRouter(); - initializeMetadata(MessageType::Request); - - NiceMock conn_pool_handle; - EXPECT_CALL(context_.cluster_manager_.tcp_conn_pool_, newConnection(_)) - .WillOnce(Invoke([&](Tcp::ConnectionPool::Callbacks&) -> Tcp::ConnectionPool::Cancellable* { - return &conn_pool_handle; - })); - - EXPECT_CALL(conn_pool_handle, cancel(Tcp::ConnectionPool::CancelPolicy::Default)); - - startRequest(MessageType::Request); - router_->onDestroy(); - - destroyRouter(); -} - -TEST_F(DubboRouterTest, LocalClosedWhileResponseComplete) { - initializeRouter(); - initializeMetadata(MessageType::Request); - - EXPECT_CALL(callbacks_, startUpstreamResponse(_, _)).Times(1); - EXPECT_CALL(callbacks_, upstreamData(_)) - .WillOnce(Return(DubboFilters::UpstreamResponseStatus::Complete)); - EXPECT_CALL(callbacks_, sendLocalReply(_, _)).Times(0); - - startRequest(MessageType::Request); - connectUpstream(); - - Buffer::OwnedImpl buffer; - buffer.add(std::string("This is the test data")); - router_->onUpstreamData(buffer, false); - - upstream_connection_.close(Network::ConnectionCloseType::NoFlush); - - destroyRouter(); -} - } // namespace Router } // namespace DubboProxy } // namespace NetworkFilters From dac7c18526b8aaa8059d9b8d2396b7a806f8bfea Mon Sep 17 00:00:00 2001 From: "leilei.gll" Date: Mon, 4 Mar 2019 21:57:55 +0800 Subject: [PATCH 18/23] Revert "Revert "Add some unit test cases"" This reverts commit d9eb2c17cffb24783c6696a81c5f6593f1147a51. Signed-off-by: leilei.gll --- .../network/dubbo_proxy/router/router_impl.cc | 3 + .../dubbo_proxy/dubbo_protocol_impl_test.cc | 86 +++++++++++++++++++ .../hessian_deserializer_impl_test.cc | 10 +++ .../network/dubbo_proxy/router_test.cc | 76 +++++++++++++++- 4 files changed, 172 insertions(+), 3 deletions(-) diff --git a/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc b/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc index 1b4cf8767e6a8..ff22478608c43 100644 --- a/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc +++ b/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc @@ -42,6 +42,7 @@ Network::FilterStatus Router::transportEnd() { // No response expected upstream_request_->onResponseComplete(); cleanup(); + ENVOY_LOG(debug, "dubbo upstream request: the message is one-way and no response is required"); } filter_complete_ = true; @@ -207,12 +208,14 @@ void Router::UpstreamRequest::resetStream() { ASSERT(!conn_data_); conn_pool_handle_->cancel(Tcp::ConnectionPool::CancelPolicy::Default); conn_pool_handle_ = nullptr; + ENVOY_LOG(debug, "dubbo upstream request: reset connection pool handler"); } if (conn_data_) { ASSERT(!conn_pool_handle_); conn_data_->connection().close(Network::ConnectionCloseType::NoFlush); conn_data_.reset(); + ENVOY_LOG(debug, "dubbo upstream request: reset connection data"); } } diff --git a/test/extensions/filters/network/dubbo_proxy/dubbo_protocol_impl_test.cc b/test/extensions/filters/network/dubbo_proxy/dubbo_protocol_impl_test.cc index 3407d4bb89880..550935b1d9e20 100644 --- a/test/extensions/filters/network/dubbo_proxy/dubbo_protocol_impl_test.cc +++ b/test/extensions/filters/network/dubbo_proxy/dubbo_protocol_impl_test.cc @@ -139,6 +139,92 @@ TEST(DubboProtocolImplTest, encode) { EXPECT_EQ(context.body_size_, expect_body_size); } +TEST(DubboProtocolImplTest, decode) { + Buffer::OwnedImpl buffer; + MessageMetadataSharedPtr metadata; + Protocol::Context context; + DubboProtocolImpl dubbo_protocol; + + // metadata is nullptr + EXPECT_THROW_WITH_MESSAGE(dubbo_protocol.decode(buffer, &context, metadata), EnvoyException, + "invalid metadata parameter"); + + metadata = std::make_shared(); + + // Invalid message header size + EXPECT_FALSE(dubbo_protocol.decode(buffer, &context, metadata)); + + // Invalid dubbo magic number + { + addInt64(buffer, 0); + addInt64(buffer, 0); + EXPECT_THROW_WITH_MESSAGE(dubbo_protocol.decode(buffer, &context), EnvoyException, + "invalid dubbo message magic number 0"); + buffer.drain(buffer.length()); + } + + // Invalid message body size + { + buffer.add(std::string({'\xda', '\xbb', '\xc2', 0x00})); + addInt64(buffer, 1); + addInt32(buffer, DubboProtocolImpl::MaxBodySize + 1); + std::string exception_string = + fmt::format("invalid dubbo message size {}", DubboProtocolImpl::MaxBodySize + 1); + EXPECT_THROW_WITH_MESSAGE(dubbo_protocol.decode(buffer, &context), EnvoyException, + exception_string); + buffer.drain(buffer.length()); + } + + // Invalid serialization type + { + buffer.add(std::string({'\xda', '\xbb', '\xc3', 0x00})); + addInt64(buffer, 1); + addInt32(buffer, 0xff); + EXPECT_THROW_WITH_MESSAGE(dubbo_protocol.decode(buffer, &context, metadata), EnvoyException, + "invalid dubbo message serialization type 3"); + buffer.drain(buffer.length()); + } + + // Invalid response status + { + buffer.add(std::string({'\xda', '\xbb', 0x42, 0x00})); + addInt64(buffer, 1); + addInt32(buffer, 0xff); + EXPECT_THROW_WITH_MESSAGE(dubbo_protocol.decode(buffer, &context, metadata), EnvoyException, + "invalid dubbo message response status 0"); + buffer.drain(buffer.length()); + } + + // The dubbo request message + { + Protocol::Context context; + buffer.add(std::string({'\xda', '\xbb', '\xc2', 0x00})); + addInt64(buffer, 1); + addInt32(buffer, 1); + EXPECT_TRUE(dubbo_protocol.decode(buffer, &context, metadata)); + EXPECT_EQ(1, context.body_size_); + EXPECT_FALSE(context.is_heartbeat_); + EXPECT_EQ(MessageType::Request, metadata->message_type()); + EXPECT_EQ(1, metadata->request_id()); + EXPECT_EQ(SerializationType::Hessian, metadata->serialization_type()); + buffer.drain(buffer.length()); + } + + // The One-way dubbo request message + { + Protocol::Context context; + buffer.add(std::string({'\xda', '\xbb', '\x82', 0x00})); + addInt64(buffer, 1); + addInt32(buffer, 1); + EXPECT_TRUE(dubbo_protocol.decode(buffer, &context, metadata)); + EXPECT_EQ(1, context.body_size_); + EXPECT_FALSE(context.is_heartbeat_); + EXPECT_EQ(MessageType::Oneway, metadata->message_type()); + EXPECT_EQ(1, metadata->request_id()); + EXPECT_EQ(SerializationType::Hessian, metadata->serialization_type()); + } +} + } // namespace DubboProxy } // namespace NetworkFilters } // namespace Extensions diff --git a/test/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl_test.cc b/test/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl_test.cc index bff7788bd253a..f9bb1b9337a45 100644 --- a/test/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl_test.cc +++ b/test/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl_test.cc @@ -87,6 +87,16 @@ TEST(HessianProtocolTest, deserializeRpcResult) { EXPECT_TRUE(result->hasException()); } + { + Buffer::OwnedImpl buffer; + buffer.add(std::string({ + '\x91', // return type + 0x04, 't', 'e', 's', 't', // return body + })); + auto result = deserializer.deserializeRpcResult(buffer, 4); + EXPECT_TRUE(result->hasException()); + } + // incorrect body size { Buffer::OwnedImpl buffer; diff --git a/test/extensions/filters/network/dubbo_proxy/router_test.cc b/test/extensions/filters/network/dubbo_proxy/router_test.cc index c1c7e4be0b639..f1b156e613c75 100644 --- a/test/extensions/filters/network/dubbo_proxy/router_test.cc +++ b/test/extensions/filters/network/dubbo_proxy/router_test.cc @@ -107,6 +107,8 @@ class DubboRouterTestBase { .WillOnce(Return(SerializationType::Hessian)); EXPECT_CALL(callbacks_, downstreamProtocolType()).WillOnce(Return(ProtocolType::Dubbo)); + EXPECT_EQ(Network::FilterStatus::Continue, + router_->messageBegin(msg_type, metadata_->request_id(), SerializationType::Hessian)); EXPECT_EQ(Network::FilterStatus::StopIteration, router_->messageEnd(metadata_)); EXPECT_CALL(callbacks_, connection()).WillRepeatedly(Return(&connection_)); @@ -169,9 +171,6 @@ class DubboRouterTestBase { context_.cluster_manager_.tcp_conn_pool_.poolReady(upstream_connection_); return nullptr; })); - - EXPECT_EQ(Network::FilterStatus::Continue, router_->transportBegin()); - EXPECT_NE(nullptr, upstream_callbacks_); } void returnResponse() { @@ -412,6 +411,8 @@ TEST_F(DubboRouterTest, OneWay) { startRequest(MessageType::Oneway); connectUpstream(); + EXPECT_EQ(Network::FilterStatus::Continue, router_->transportEnd()); + destroyRouter(); } @@ -424,6 +425,7 @@ TEST_F(DubboRouterTest, Call) { startRequest(MessageType::Request); connectUpstream(); + EXPECT_EQ(Network::FilterStatus::Continue, router_->transportBegin()); EXPECT_EQ(Network::FilterStatus::Continue, router_->transportEnd()); returnResponse(); @@ -441,6 +443,7 @@ TEST_F(DubboRouterTest, DecoderFilterCallbacks) { startRequest(MessageType::Request); connectUpstream(); + EXPECT_EQ(Network::FilterStatus::Continue, router_->transportBegin()); EXPECT_EQ(Network::FilterStatus::Continue, router_->transportEnd()); Buffer::OwnedImpl buffer; @@ -450,6 +453,73 @@ TEST_F(DubboRouterTest, DecoderFilterCallbacks) { destroyRouter(); } +TEST_F(DubboRouterTest, UpstreamDataReset) { + initializeRouter(); + initializeMetadata(MessageType::Request); + + EXPECT_CALL(callbacks_, startUpstreamResponse(_, _)).Times(1); + EXPECT_CALL(callbacks_, upstreamData(_)) + .WillOnce(Return(DubboFilters::UpstreamResponseStatus::Reset)); + EXPECT_CALL(upstream_connection_, close(Network::ConnectionCloseType::NoFlush)); + + startRequest(MessageType::Request); + connectUpstream(); + + Buffer::OwnedImpl buffer; + buffer.add(std::string("This is the test data")); + router_->onUpstreamData(buffer, false); + + destroyRouter(); +} + +TEST_F(DubboRouterTest, StartRequestWithExistingConnection) { + initializeRouter(); + startRequestWithExistingConnection(MessageType::Request); + + EXPECT_EQ(Network::FilterStatus::Continue, router_->messageEnd(metadata_)); + + destroyRouter(); +} + +TEST_F(DubboRouterTest, DestroyWhileConnecting) { + initializeRouter(); + initializeMetadata(MessageType::Request); + + NiceMock conn_pool_handle; + EXPECT_CALL(context_.cluster_manager_.tcp_conn_pool_, newConnection(_)) + .WillOnce(Invoke([&](Tcp::ConnectionPool::Callbacks&) -> Tcp::ConnectionPool::Cancellable* { + return &conn_pool_handle; + })); + + EXPECT_CALL(conn_pool_handle, cancel(Tcp::ConnectionPool::CancelPolicy::Default)); + + startRequest(MessageType::Request); + router_->onDestroy(); + + destroyRouter(); +} + +TEST_F(DubboRouterTest, LocalClosedWhileResponseComplete) { + initializeRouter(); + initializeMetadata(MessageType::Request); + + EXPECT_CALL(callbacks_, startUpstreamResponse(_, _)).Times(1); + EXPECT_CALL(callbacks_, upstreamData(_)) + .WillOnce(Return(DubboFilters::UpstreamResponseStatus::Complete)); + EXPECT_CALL(callbacks_, sendLocalReply(_, _)).Times(0); + + startRequest(MessageType::Request); + connectUpstream(); + + Buffer::OwnedImpl buffer; + buffer.add(std::string("This is the test data")); + router_->onUpstreamData(buffer, false); + + upstream_connection_.close(Network::ConnectionCloseType::NoFlush); + + destroyRouter(); +} + } // namespace Router } // namespace DubboProxy } // namespace NetworkFilters From 5352fa0dcbd1968e434fb74243340e7d1fb6958a Mon Sep 17 00:00:00 2001 From: "leilei.gll" Date: Tue, 5 Mar 2019 15:03:05 +0800 Subject: [PATCH 19/23] Add some unit test cases. Signed-off-by: leilei.gll --- .../filters/network/dubbo_proxy/BUILD | 12 ++ .../dubbo_proxy/dubbo_protocol_impl_test.cc | 4 +- .../network/dubbo_proxy/route_matcher_test.cc | 130 ++++++++++++++++++ .../dubbo_proxy/router_filter_config_test.cc | 54 ++++++++ 4 files changed, 198 insertions(+), 2 deletions(-) create mode 100644 test/extensions/filters/network/dubbo_proxy/router_filter_config_test.cc diff --git a/test/extensions/filters/network/dubbo_proxy/BUILD b/test/extensions/filters/network/dubbo_proxy/BUILD index e537f7d3c9831..39ff88513658f 100644 --- a/test/extensions/filters/network/dubbo_proxy/BUILD +++ b/test/extensions/filters/network/dubbo_proxy/BUILD @@ -150,3 +150,15 @@ envoy_extension_cc_test( "//source/extensions/filters/network/dubbo_proxy:metadata_lib", ], ) + +envoy_extension_cc_test( + name = "router_filter_config_test", + srcs = ["router_filter_config_test.cc"], + extension_name = "envoy.filters.network.dubbo_proxy", + deps = [ + ":mocks_lib", + "//source/extensions/filters/network/dubbo_proxy/filters:well_known_names", + "//source/extensions/filters/network/dubbo_proxy/router:config", + "//test/mocks/server:server_mocks", + ], +) diff --git a/test/extensions/filters/network/dubbo_proxy/dubbo_protocol_impl_test.cc b/test/extensions/filters/network/dubbo_proxy/dubbo_protocol_impl_test.cc index 550935b1d9e20..42e23efcdee31 100644 --- a/test/extensions/filters/network/dubbo_proxy/dubbo_protocol_impl_test.cc +++ b/test/extensions/filters/network/dubbo_proxy/dubbo_protocol_impl_test.cc @@ -158,7 +158,7 @@ TEST(DubboProtocolImplTest, decode) { { addInt64(buffer, 0); addInt64(buffer, 0); - EXPECT_THROW_WITH_MESSAGE(dubbo_protocol.decode(buffer, &context), EnvoyException, + EXPECT_THROW_WITH_MESSAGE(dubbo_protocol.decode(buffer, &context, metadata), EnvoyException, "invalid dubbo message magic number 0"); buffer.drain(buffer.length()); } @@ -170,7 +170,7 @@ TEST(DubboProtocolImplTest, decode) { addInt32(buffer, DubboProtocolImpl::MaxBodySize + 1); std::string exception_string = fmt::format("invalid dubbo message size {}", DubboProtocolImpl::MaxBodySize + 1); - EXPECT_THROW_WITH_MESSAGE(dubbo_protocol.decode(buffer, &context), EnvoyException, + EXPECT_THROW_WITH_MESSAGE(dubbo_protocol.decode(buffer, &context, metadata), EnvoyException, exception_string); buffer.drain(buffer.length()); } diff --git a/test/extensions/filters/network/dubbo_proxy/route_matcher_test.cc b/test/extensions/filters/network/dubbo_proxy/route_matcher_test.cc index ebf53c63914e9..e464cf99f35e6 100644 --- a/test/extensions/filters/network/dubbo_proxy/route_matcher_test.cc +++ b/test/extensions/filters/network/dubbo_proxy/route_matcher_test.cc @@ -432,6 +432,136 @@ serialization_type: Hessian2 MultiRouteMatcher matcher(config.route_config()); EXPECT_EQ("user_service_dubbo_server", matcher.route(metadata, 0)->routeEntry()->clusterName()); + + { + envoy::config::filter::network::dubbo_proxy::v2alpha1::DubboProxy invalid_config; + MultiRouteMatcher matcher(invalid_config.route_config()); + EXPECT_EQ(nullptr, matcher.route(metadata, 0)); + } +} + +TEST(DubboRouteMatcherTest, RouteByInvalidParameter) { + const std::string yaml = R"EOF( +name: local_route +interface: org.apache.dubbo.demo.DemoService +routes: + - match: + method: + name: + exact: "add" + params_match: + 1: + exact_match: "user_id:94562" + route: + cluster: user_service_dubbo_server +)EOF"; + + envoy::config::filter::network::dubbo_proxy::v2alpha1::RouteConfiguration config = + parseRouteConfigurationFromV2Yaml(yaml); + MessageMetadata metadata; + metadata.setServiceName("org.apache.dubbo.demo.DemoService"); + metadata.setMethodName("add"); + + // There is no parameter information in metadata. + RouteMatcher matcher(config); + EXPECT_EQ(nullptr, matcher.route(metadata, 0)); + + // The parameter is empty. + metadata.addParameterValue(1, ""); + EXPECT_EQ(nullptr, matcher.route(metadata, 0)); + + { + MessageMetadata metadata; + metadata.setServiceName("org.apache.dubbo.demo.DemoService"); + metadata.setMethodName("add"); + metadata.addParameterValue(1, "user_id:562"); + EXPECT_EQ(nullptr, matcher.route(metadata, 0)); + } +} + +TEST(DubboRouteMatcherTest, WeightedClusters) { + const std::string yaml = R"EOF( +name: local_route +interface: org.apache.dubbo.demo.DemoService +routes: + - match: + method: + name: + exact: "method1" + route: + weighted_clusters: + clusters: + - name: cluster1 + weight: 30 + - name: cluster2 + weight: 30 + - name: cluster3 + weight: 40 + - match: + method: + name: + exact: "method2" + route: + weighted_clusters: + clusters: + - name: cluster1 + weight: 2000 + - name: cluster2 + weight: 3000 + - name: cluster3 + weight: 5000 +)EOF"; + + envoy::config::filter::network::dubbo_proxy::v2alpha1::RouteConfiguration config = + parseRouteConfigurationFromV2Yaml(yaml); + RouteMatcher matcher(config); + MessageMetadata metadata; + metadata.setServiceName("org.apache.dubbo.demo.DemoService"); + + { + metadata.setMethodName("method1"); + EXPECT_EQ("cluster1", matcher.route(metadata, 0)->routeEntry()->clusterName()); + EXPECT_EQ("cluster1", matcher.route(metadata, 29)->routeEntry()->clusterName()); + EXPECT_EQ("cluster2", matcher.route(metadata, 30)->routeEntry()->clusterName()); + EXPECT_EQ("cluster2", matcher.route(metadata, 59)->routeEntry()->clusterName()); + EXPECT_EQ("cluster3", matcher.route(metadata, 60)->routeEntry()->clusterName()); + EXPECT_EQ("cluster3", matcher.route(metadata, 99)->routeEntry()->clusterName()); + EXPECT_EQ("cluster1", matcher.route(metadata, 100)->routeEntry()->clusterName()); + } + + { + metadata.setMethodName("method2"); + EXPECT_EQ("cluster1", matcher.route(metadata, 0)->routeEntry()->clusterName()); + EXPECT_EQ("cluster1", matcher.route(metadata, 1999)->routeEntry()->clusterName()); + EXPECT_EQ("cluster2", matcher.route(metadata, 2000)->routeEntry()->clusterName()); + EXPECT_EQ("cluster2", matcher.route(metadata, 4999)->routeEntry()->clusterName()); + EXPECT_EQ("cluster3", matcher.route(metadata, 5000)->routeEntry()->clusterName()); + EXPECT_EQ("cluster3", matcher.route(metadata, 9999)->routeEntry()->clusterName()); + EXPECT_EQ("cluster1", matcher.route(metadata, 10000)->routeEntry()->clusterName()); + } +} + +TEST(DubboRouteMatcherTest, WeightedClusterMissingWeight) { + const std::string yaml = R"EOF( +name: config +routes: + - match: + method: + name: + exact: "method1" + route: + weighted_clusters: + clusters: + - name: cluster1 + weight: 20000 + - name: cluster2 + - name: cluster3 + weight: 5000 +)EOF"; + + envoy::config::filter::network::dubbo_proxy::v2alpha1::RouteConfiguration config = + parseRouteConfigurationFromV2Yaml(yaml); + EXPECT_THROW(RouteMatcher m(config), EnvoyException); } } // namespace Router diff --git a/test/extensions/filters/network/dubbo_proxy/router_filter_config_test.cc b/test/extensions/filters/network/dubbo_proxy/router_filter_config_test.cc new file mode 100644 index 0000000000000..a46f43ddb10bb --- /dev/null +++ b/test/extensions/filters/network/dubbo_proxy/router_filter_config_test.cc @@ -0,0 +1,54 @@ +#include "envoy/config/filter/dubbo/router/v2alpha1/router.pb.validate.h" + +#include "extensions/filters/network/dubbo_proxy/filters/well_known_names.h" +#include "extensions/filters/network/dubbo_proxy/router/config.h" + +#include "test/extensions/filters/network/dubbo_proxy/mocks.h" +#include "test/mocks/server/mocks.h" + +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +using testing::_; + +namespace Envoy { +namespace Extensions { +namespace NetworkFilters { +namespace DubboProxy { +namespace Router { + +TEST(DubboProxyRouterFilterConfigTest, RouterV2Alpha1Filter) { + envoy::config::filter::dubbo::router::v2alpha1::Router router_config; + NiceMock context; + RouterFilterConfig factory; + DubboFilters::FilterFactoryCb cb = + factory.createFilterFactoryFromProto(router_config, "stats", context); + DubboFilters::MockFilterChainFactoryCallbacks filter_callback; + EXPECT_CALL(filter_callback, addDecoderFilter(_)); + cb(filter_callback); +} + +TEST(DubboProxyRouterFilterConfigTest, RouterFilterWithEmptyProtoConfig) { + NiceMock context; + RouterFilterConfig factory; + DubboFilters::FilterFactoryCb cb = + factory.createFilterFactoryFromProto(*factory.createEmptyConfigProto(), "stats", context); + DubboFilters::MockFilterChainFactoryCallbacks filter_callback; + EXPECT_CALL(filter_callback, addDecoderFilter(_)); + cb(filter_callback); +} + +TEST(DubboProxyRouterFilterConfigTest, DoubleRegistrationTest) { + EXPECT_THROW_WITH_MESSAGE( + (Registry::RegisterFactory()), + EnvoyException, + fmt::format("Double registration for name: '{}'", + DubboFilters::DubboFilterNames::get().ROUTER)); +} + +} // namespace Router +} // namespace DubboProxy +} // namespace NetworkFilters +} // namespace Extensions +} // namespace Envoy From c9f1567518dc47c68ba4db8d025388a42aba585b Mon Sep 17 00:00:00 2001 From: "leilei.gll" Date: Wed, 6 Mar 2019 11:32:58 +0800 Subject: [PATCH 20/23] Optimize the code Signed-off-by: leilei.gll --- .../filters/network/dubbo_proxy/hessian_deserializer_impl.cc | 2 +- .../filters/network/dubbo_proxy/router/router_impl.cc | 5 +---- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/source/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl.cc b/source/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl.cc index d1b1eaf73d345..8c433fd194dbe 100644 --- a/source/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl.cc +++ b/source/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl.cc @@ -90,7 +90,7 @@ size_t HessianDeserializerImpl::serializeRpcResult(Buffer::Instance& output_buff // Serialized response content. serialized_size += HessianUtils::writeString(output_buffer, content); - ASSERT(output_buffer.length() - origin_length == serialized_size); + ASSERT((output_buffer.length() - origin_length) == serialized_size); return serialized_size; } diff --git a/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc b/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc index ff22478608c43..8898b71168251 100644 --- a/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc +++ b/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc @@ -229,8 +229,6 @@ void Router::UpstreamRequest::encodeData(Buffer::Instance& data) { void Router::UpstreamRequest::onPoolFailure(Tcp::ConnectionPool::PoolFailureReason reason, Upstream::HostDescriptionConstSharedPtr host) { - ENVOY_LOG(warn, "dubbo upstream request: connection failure '{}'", host->address()->asString()); - conn_pool_handle_ = nullptr; // Mimic an upstream reset. @@ -302,8 +300,7 @@ void Router::UpstreamRequest::onResetStream(Tcp::ConnectionPool::PoolFailureReas case Tcp::ConnectionPool::PoolFailureReason::Overflow: parent_.callbacks_->sendLocalReply( AppException(ResponseStatus::ServerError, - fmt::format("dubbo upstream request: too many connections to '{}'", - upstream_host_->address()->asString())), + fmt::format("dubbo upstream request: too many connections")), false); break; case Tcp::ConnectionPool::PoolFailureReason::LocalConnectionFailure: From 4ecaa7a6c66c50469f4e307edfca006dd7b5d0b5 Mon Sep 17 00:00:00 2001 From: "leilei.gll" Date: Fri, 8 Mar 2019 19:27:22 +0800 Subject: [PATCH 21/23] Optimize some type definitions Signed-off-by: leilei.gll --- .../filters/network/dubbo_proxy/app_exception.h | 2 +- .../network/dubbo_proxy/dubbo_protocol_impl.h | 2 +- .../dubbo_proxy/hessian_deserializer_impl.cc | 2 -- .../filters/network/dubbo_proxy/hessian_utils.cc | 14 +++++++------- .../filters/network/dubbo_proxy/hessian_utils.h | 2 +- .../filters/network/dubbo_proxy/router/config.cc | 7 ++----- 6 files changed, 12 insertions(+), 17 deletions(-) diff --git a/source/extensions/filters/network/dubbo_proxy/app_exception.h b/source/extensions/filters/network/dubbo_proxy/app_exception.h index 4917b30894651..36b4295e4ebe0 100644 --- a/source/extensions/filters/network/dubbo_proxy/app_exception.h +++ b/source/extensions/filters/network/dubbo_proxy/app_exception.h @@ -16,7 +16,7 @@ struct AppException : public EnvoyException, public DubboFilters::DirectResponse, Logger::Loggable { AppException(ResponseStatus status, const std::string& what); - AppException(const AppException& ex); + explicit AppException(const AppException& ex); using ResponseType = DubboFilters::DirectResponse::ResponseType; ResponseType encode(MessageMetadata& metadata, Protocol& protocol, Deserializer& deserializer, diff --git a/source/extensions/filters/network/dubbo_proxy/dubbo_protocol_impl.h b/source/extensions/filters/network/dubbo_proxy/dubbo_protocol_impl.h index 24341df0530de..1f3804c0e6c90 100644 --- a/source/extensions/filters/network/dubbo_proxy/dubbo_protocol_impl.h +++ b/source/extensions/filters/network/dubbo_proxy/dubbo_protocol_impl.h @@ -69,7 +69,7 @@ typedef std::unique_ptr ResponseMessageImplPtr; class DubboProtocolImpl : public Protocol { public: DubboProtocolImpl() = default; - DubboProtocolImpl(ProtocolCallbacks* callbacks) : callbacks_(callbacks) {} + explicit DubboProtocolImpl(ProtocolCallbacks* callbacks) : callbacks_(callbacks) {} const std::string& name() const override { return ProtocolNames::get().fromType(type()); } ProtocolType type() const override { return ProtocolType::Dubbo; } bool decode(Buffer::Instance& buffer, Protocol::Context* context) override; diff --git a/source/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl.cc b/source/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl.cc index 8c433fd194dbe..d317022f9e861 100644 --- a/source/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl.cc +++ b/source/extensions/filters/network/dubbo_proxy/hessian_deserializer_impl.cc @@ -48,8 +48,6 @@ RpcResultPtr HessianDeserializerImpl::deserializeRpcResult(Buffer::Instance& buf switch (type) { case RpcResponseType::ResponseWithException: case RpcResponseType::ResponseWithExceptionWithAttachments: - result = std::make_unique(true); - break; case RpcResponseType::ResponseWithValue: result = std::make_unique(true); break; diff --git a/source/extensions/filters/network/dubbo_proxy/hessian_utils.cc b/source/extensions/filters/network/dubbo_proxy/hessian_utils.cc index f45ee26cc7b0c..6c064e5569dbe 100644 --- a/source/extensions/filters/network/dubbo_proxy/hessian_utils.cc +++ b/source/extensions/filters/network/dubbo_proxy/hessian_utils.cc @@ -44,8 +44,8 @@ size_t doWriteString(Buffer::Instance& instance, const absl::string_view& str_vi } if (length < two_octet_max_lenth) { - uint8_t code = length / 256; // 0x30 + length / 0x100 must less than 0x34 - uint8_t remain = length % 256; + uint8_t code = length >> 8; // 0x30 + length / 0x100 must less than 0x34 + uint8_t remain = length & 0xff; std::initializer_list values{static_cast(0x30 + code), remain}; addSeq(instance, values); instance.add(str_view.data(), str_view.length()); @@ -53,8 +53,8 @@ size_t doWriteString(Buffer::Instance& instance, const absl::string_view& str_vi } if (length <= str_max_length) { - uint8_t code = length / 256; - uint8_t remain = length % 256; + uint8_t code = length >> 8; + uint8_t remain = length & 0xff; std::initializer_list values{'S', code, remain}; addSeq(instance, values); instance.add(str_view.data(), str_view.length()); @@ -67,8 +67,8 @@ size_t doWriteString(Buffer::Instance& instance, const absl::string_view& str_vi size_t size = str_max_length + values.size(); ASSERT(size == (str_max_length + values.size())); - size_t child_size = doWriteString( - instance, absl::string_view(str_view.data() + str_max_length, length - str_max_length)); + size_t child_size = + doWriteString(instance, str_view.substr(str_max_length, length - str_max_length)); return child_size + size; } @@ -564,7 +564,7 @@ std::string HessianUtils::readByte(Buffer::Instance& buffer) { return result; } -size_t HessianUtils::writeString(Buffer::Instance& buffer, const absl::string_view& str) { +size_t HessianUtils::writeString(Buffer::Instance& buffer, const absl::string_view str) { return doWriteString(buffer, str); } diff --git a/source/extensions/filters/network/dubbo_proxy/hessian_utils.h b/source/extensions/filters/network/dubbo_proxy/hessian_utils.h index 9280e216583e6..335f4da239a5c 100644 --- a/source/extensions/filters/network/dubbo_proxy/hessian_utils.h +++ b/source/extensions/filters/network/dubbo_proxy/hessian_utils.h @@ -36,7 +36,7 @@ class HessianUtils { static std::chrono::milliseconds readDate(Buffer::Instance& buffer); static std::string readByte(Buffer::Instance& buffer); - static size_t writeString(Buffer::Instance& buffer, const absl::string_view& str); + static size_t writeString(Buffer::Instance& buffer, const absl::string_view str); static size_t writeInt(Buffer::Instance& buffer, uint8_t value); }; diff --git a/source/extensions/filters/network/dubbo_proxy/router/config.cc b/source/extensions/filters/network/dubbo_proxy/router/config.cc index fbb40dcb12b73..4e4382a61bc3f 100644 --- a/source/extensions/filters/network/dubbo_proxy/router/config.cc +++ b/source/extensions/filters/network/dubbo_proxy/router/config.cc @@ -11,11 +11,8 @@ namespace DubboProxy { namespace Router { DubboFilters::FilterFactoryCb RouterFilterConfig::createFilterFactoryFromProtoTyped( - const envoy::config::filter::dubbo::router::v2alpha1::Router& proto_config, - const std::string& stat_prefix, Server::Configuration::FactoryContext& context) { - UNREFERENCED_PARAMETER(proto_config); - UNREFERENCED_PARAMETER(stat_prefix); - + const envoy::config::filter::dubbo::router::v2alpha1::Router&, const std::string&, + Server::Configuration::FactoryContext& context) { return [&context](DubboFilters::FilterChainFactoryCallbacks& callbacks) -> void { callbacks.addDecoderFilter(std::make_shared(context.clusterManager())); }; From 8f66806bec70d40a3654e7f2f8a62729bcb6cc68 Mon Sep 17 00:00:00 2001 From: "leilei.gll" Date: Fri, 8 Mar 2019 21:23:54 +0800 Subject: [PATCH 22/23] Add some unit test cases. Signed-off-by: leilei.gll --- .../network/dubbo_proxy/router/route_matcher.h | 2 +- .../network/dubbo_proxy/app_exception_test.cc | 13 +++++++++++++ .../network/dubbo_proxy/dubbo_protocol_impl_test.cc | 13 +++++++++++-- .../network/dubbo_proxy/route_matcher_test.cc | 3 +++ .../filters/network/dubbo_proxy/router_test.cc | 9 +++++++++ 5 files changed, 37 insertions(+), 3 deletions(-) diff --git a/source/extensions/filters/network/dubbo_proxy/router/route_matcher.h b/source/extensions/filters/network/dubbo_proxy/router/route_matcher.h index 54c4b9b9ce3c6..287bbcb69f3d2 100644 --- a/source/extensions/filters/network/dubbo_proxy/router/route_matcher.h +++ b/source/extensions/filters/network/dubbo_proxy/router/route_matcher.h @@ -34,7 +34,7 @@ class RouteEntryImplBase : public RouteEntry, // Router::RouteEntry const std::string& clusterName() const override; const Envoy::Router::MetadataMatchCriteria* metadataMatchCriteria() const override { - NOT_IMPLEMENTED_GCOVR_EXCL_LINE; + return metadata_match_criteria_.get(); } // Router::Route diff --git a/test/extensions/filters/network/dubbo_proxy/app_exception_test.cc b/test/extensions/filters/network/dubbo_proxy/app_exception_test.cc index d2a4d85f61c60..7d4e7b1bf222f 100644 --- a/test/extensions/filters/network/dubbo_proxy/app_exception_test.cc +++ b/test/extensions/filters/network/dubbo_proxy/app_exception_test.cc @@ -6,10 +6,15 @@ #include "extensions/filters/network/dubbo_proxy/hessian_utils.h" #include "extensions/filters/network/dubbo_proxy/metadata.h" +#include "test/extensions/filters/network/dubbo_proxy/mocks.h" #include "test/test_common/test_base.h" +#include "gmock/gmock.h" #include "gtest/gtest.h" +using testing::_; +using testing::Return; + namespace Envoy { namespace Extensions { namespace NetworkFilters { @@ -60,6 +65,14 @@ TEST_F(AppExceptionTest, Encode) { auto result = deserializer_.deserializeRpcResult(buffer, context_.body_size_); EXPECT_TRUE(result->hasException()); buffer.drain(buffer.length()); + + AppException new_app_exception(app_exception); + EXPECT_EQ(new_app_exception.status_, ResponseStatus::ServiceNotFound); + + MockProtocol mock_protocol; + EXPECT_CALL(mock_protocol, encode(_, _, _)).WillOnce(Return(false)); + EXPECT_THROW(app_exception.encode(*(metadata_.get()), mock_protocol, deserializer_, buffer), + EnvoyException); } } // namespace DubboProxy diff --git a/test/extensions/filters/network/dubbo_proxy/dubbo_protocol_impl_test.cc b/test/extensions/filters/network/dubbo_proxy/dubbo_protocol_impl_test.cc index 42e23efcdee31..ec95ff72447d7 100644 --- a/test/extensions/filters/network/dubbo_proxy/dubbo_protocol_impl_test.cc +++ b/test/extensions/filters/network/dubbo_proxy/dubbo_protocol_impl_test.cc @@ -42,7 +42,12 @@ TEST(DubboProtocolImplTest, Normal) { buffer.add(std::string({'\xda', '\xbb', '\xc2', 0x00})); addInt64(buffer, 1); addInt32(buffer, 1); - EXPECT_CALL(cb, onRequestMessageRvr); + EXPECT_CALL(cb, onRequestMessageRvr).WillOnce(Invoke([&](RequestMessage* res) -> void { + EXPECT_EQ(MessageType::Request, res->messageType()); + EXPECT_EQ(SerializationType::Hessian, res->serializationType()); + EXPECT_EQ(1, res->bodySize()); + EXPECT_GE(res->toString().size(), 0); + })); EXPECT_TRUE(dubbo_protocol.decode(buffer, &context)); EXPECT_EQ(1, context.body_size_); EXPECT_TRUE(context.is_request_); @@ -55,7 +60,11 @@ TEST(DubboProtocolImplTest, Normal) { buffer.add(std::string({'\xda', '\xbb', 0x42, 20})); addInt64(buffer, 1); addInt32(buffer, 1); - EXPECT_CALL(cb, onResponseMessageRvr); + EXPECT_CALL(cb, onResponseMessageRvr).WillOnce(Invoke([](ResponseMessage* res) -> void { + EXPECT_EQ(ResponseStatus::Ok, res->responseStatus()); + EXPECT_EQ(MessageType::Response, res->messageType()); + EXPECT_GE(res->toString().size(), 0); + })); EXPECT_TRUE(dubbo_protocol.decode(buffer, &context)); EXPECT_EQ(1, context.body_size_); EXPECT_FALSE(context.is_request_); diff --git a/test/extensions/filters/network/dubbo_proxy/route_matcher_test.cc b/test/extensions/filters/network/dubbo_proxy/route_matcher_test.cc index e464cf99f35e6..21236fb2489c1 100644 --- a/test/extensions/filters/network/dubbo_proxy/route_matcher_test.cc +++ b/test/extensions/filters/network/dubbo_proxy/route_matcher_test.cc @@ -389,6 +389,7 @@ interface: org.apache.dubbo.demo.DemoService test_value = "456"; EXPECT_EQ("user_service_dubbo_server", matcher.route(metadata, 0)->routeEntry()->clusterName()); + EXPECT_EQ(nullptr, matcher.route(metadata, 0)->routeEntry()->metadataMatchCriteria()); test_value = "123"; EXPECT_EQ(nullptr, matcher.route(metadata, 0)); @@ -527,6 +528,7 @@ interface: org.apache.dubbo.demo.DemoService EXPECT_EQ("cluster3", matcher.route(metadata, 60)->routeEntry()->clusterName()); EXPECT_EQ("cluster3", matcher.route(metadata, 99)->routeEntry()->clusterName()); EXPECT_EQ("cluster1", matcher.route(metadata, 100)->routeEntry()->clusterName()); + EXPECT_EQ(nullptr, matcher.route(metadata, 100)->routeEntry()->metadataMatchCriteria()); } { @@ -538,6 +540,7 @@ interface: org.apache.dubbo.demo.DemoService EXPECT_EQ("cluster3", matcher.route(metadata, 5000)->routeEntry()->clusterName()); EXPECT_EQ("cluster3", matcher.route(metadata, 9999)->routeEntry()->clusterName()); EXPECT_EQ("cluster1", matcher.route(metadata, 10000)->routeEntry()->clusterName()); + EXPECT_EQ(nullptr, matcher.route(metadata, 10000)->routeEntry()->metadataMatchCriteria()); } } diff --git a/test/extensions/filters/network/dubbo_proxy/router_test.cc b/test/extensions/filters/network/dubbo_proxy/router_test.cc index f1b156e613c75..c3cb7f5e6196e 100644 --- a/test/extensions/filters/network/dubbo_proxy/router_test.cc +++ b/test/extensions/filters/network/dubbo_proxy/router_test.cc @@ -182,6 +182,10 @@ class DubboRouterTestBase { .WillOnce(Return(DubboFilters::UpstreamResponseStatus::MoreData)); upstream_callbacks_->onUpstreamData(buffer, false); + // Nothing to do. + upstream_callbacks_->onAboveWriteBufferHighWatermark(); + upstream_callbacks_->onBelowWriteBufferLowWatermark(); + EXPECT_CALL(callbacks_, upstreamData(Ref(buffer))) .WillOnce(Return(DubboFilters::UpstreamResponseStatus::Complete)); EXPECT_CALL(context_.cluster_manager_.tcp_conn_pool_, released(Ref(upstream_connection_))); @@ -369,9 +373,14 @@ TEST_F(DubboRouterTest, UnexpectedRouterDestroy) { EXPECT_CALL(upstream_connection_, close(Network::ConnectionCloseType::NoFlush)); startRequest(MessageType::Request); + EXPECT_EQ(Network::FilterStatus::Continue, router_->transportBegin()); + Buffer::OwnedImpl buffer; buffer.add(std::string({'\xda', '\xbb', 0x42, 20})); EXPECT_EQ(Network::FilterStatus::Continue, router_->transferHeaderTo(buffer, buffer.length())); + buffer.drain(buffer.length()); + buffer.add("test"); + EXPECT_EQ(Network::FilterStatus::Continue, router_->transferBodyTo(buffer, buffer.length())); connectUpstream(); destroyRouter(); From 1fe2cd6003662f794482bc204f5b2f44c0d58cdf Mon Sep 17 00:00:00 2001 From: "leilei.gll" Date: Wed, 13 Mar 2019 10:39:54 +0800 Subject: [PATCH 23/23] Change the use of string_view Signed-off-by: leilei.gll --- .../extensions/filters/network/dubbo_proxy/app_exception.cc | 3 --- source/extensions/filters/network/dubbo_proxy/app_exception.h | 2 +- .../extensions/filters/network/dubbo_proxy/hessian_utils.cc | 4 ++-- source/extensions/filters/network/dubbo_proxy/hessian_utils.h | 2 +- 4 files changed, 4 insertions(+), 7 deletions(-) diff --git a/source/extensions/filters/network/dubbo_proxy/app_exception.cc b/source/extensions/filters/network/dubbo_proxy/app_exception.cc index 806ee3e3015b8..d3dd4bbc77133 100644 --- a/source/extensions/filters/network/dubbo_proxy/app_exception.cc +++ b/source/extensions/filters/network/dubbo_proxy/app_exception.cc @@ -13,9 +13,6 @@ AppException::AppException(ResponseStatus status, const std::string& what) : EnvoyException(what), status_(status), response_type_(RpcResponseType::ResponseWithException) {} -AppException::AppException(const AppException& ex) - : EnvoyException(ex.what()), status_(ex.status_), response_type_(ex.response_type_) {} - AppException::ResponseType AppException::encode(MessageMetadata& metadata, DubboProxy::Protocol& protocol, Deserializer& deserializer, diff --git a/source/extensions/filters/network/dubbo_proxy/app_exception.h b/source/extensions/filters/network/dubbo_proxy/app_exception.h index 36b4295e4ebe0..31eba8b55a620 100644 --- a/source/extensions/filters/network/dubbo_proxy/app_exception.h +++ b/source/extensions/filters/network/dubbo_proxy/app_exception.h @@ -16,7 +16,7 @@ struct AppException : public EnvoyException, public DubboFilters::DirectResponse, Logger::Loggable { AppException(ResponseStatus status, const std::string& what); - explicit AppException(const AppException& ex); + AppException(const AppException& ex) = default; using ResponseType = DubboFilters::DirectResponse::ResponseType; ResponseType encode(MessageMetadata& metadata, Protocol& protocol, Deserializer& deserializer, diff --git a/source/extensions/filters/network/dubbo_proxy/hessian_utils.cc b/source/extensions/filters/network/dubbo_proxy/hessian_utils.cc index 6c064e5569dbe..7d37531d71494 100644 --- a/source/extensions/filters/network/dubbo_proxy/hessian_utils.cc +++ b/source/extensions/filters/network/dubbo_proxy/hessian_utils.cc @@ -32,7 +32,7 @@ void addSeq(Buffer::Instance& buffer, const std::initializer_list& valu } } -size_t doWriteString(Buffer::Instance& instance, const absl::string_view& str_view) { +size_t doWriteString(Buffer::Instance& instance, absl::string_view str_view) { const size_t length = str_view.length(); constexpr size_t str_max_length = 0xffff; constexpr size_t two_octet_max_lenth = 1024; @@ -564,7 +564,7 @@ std::string HessianUtils::readByte(Buffer::Instance& buffer) { return result; } -size_t HessianUtils::writeString(Buffer::Instance& buffer, const absl::string_view str) { +size_t HessianUtils::writeString(Buffer::Instance& buffer, absl::string_view str) { return doWriteString(buffer, str); } diff --git a/source/extensions/filters/network/dubbo_proxy/hessian_utils.h b/source/extensions/filters/network/dubbo_proxy/hessian_utils.h index 335f4da239a5c..88f250442cf20 100644 --- a/source/extensions/filters/network/dubbo_proxy/hessian_utils.h +++ b/source/extensions/filters/network/dubbo_proxy/hessian_utils.h @@ -36,7 +36,7 @@ class HessianUtils { static std::chrono::milliseconds readDate(Buffer::Instance& buffer); static std::string readByte(Buffer::Instance& buffer); - static size_t writeString(Buffer::Instance& buffer, const absl::string_view str); + static size_t writeString(Buffer::Instance& buffer, absl::string_view str); static size_t writeInt(Buffer::Instance& buffer, uint8_t value); };