diff --git a/include/istio/control/http/report_data.h b/include/istio/control/http/report_data.h index 7b4968200da..8cf7326fe0f 100644 --- a/include/istio/control/http/report_data.h +++ b/include/istio/control/http/report_data.h @@ -19,6 +19,8 @@ #include #include +#include "google/protobuf/struct.pb.h" + namespace istio { namespace control { namespace http { @@ -42,31 +44,32 @@ class ReportData { int response_code; std::string response_flags; }; - virtual void GetReportInfo(ReportInfo* info) const = 0; + virtual void GetReportInfo(ReportInfo *info) const = 0; // Get destination ip/port. - virtual bool GetDestinationIpPort(std::string* ip, int* port) const = 0; + virtual bool GetDestinationIpPort(std::string *ip, int *port) const = 0; // Get Rbac attributes. struct RbacReportInfo { std::string permissive_resp_code; std::string permissive_policy_id; }; - virtual bool GetRbacReportInfo(RbacReportInfo* report_info) const = 0; + virtual bool GetRbacReportInfo(RbacReportInfo *report_info) const = 0; // Get upstream host UID. This value overrides the value in the report bag. - virtual bool GetDestinationUID(std::string* uid) const = 0; + virtual bool GetDestinationUID(std::string *uid) const = 0; // gRPC status info struct GrpcStatus { std::string status; std::string message; }; - virtual bool GetGrpcStatus(GrpcStatus* status) const = 0; + virtual bool GetGrpcStatus(GrpcStatus *status) const = 0; // Get dynamic metadata generated by Envoy filters. // Useful for logging info generated by custom codecs. - virtual bool GetDynamicFilterState(std::string* filter_state) const = 0; + virtual const ::google::protobuf::Map + &GetDynamicFilterState() const = 0; }; } // namespace http diff --git a/include/istio/control/tcp/report_data.h b/include/istio/control/tcp/report_data.h index b8d2bc02e31..91a6b7241c0 100644 --- a/include/istio/control/tcp/report_data.h +++ b/include/istio/control/tcp/report_data.h @@ -19,6 +19,8 @@ #include #include +#include "google/protobuf/struct.pb.h" + namespace istio { namespace control { namespace tcp { @@ -30,7 +32,7 @@ class ReportData { virtual ~ReportData() {} // Get upstream tcp connection IP and port. IP is returned in format of bytes. - virtual bool GetDestinationIpPort(std::string* ip, int* port) const = 0; + virtual bool GetDestinationIpPort(std::string *ip, int *port) const = 0; // Get additional report data. struct ReportInfo { @@ -38,10 +40,10 @@ class ReportData { uint64_t received_bytes; std::chrono::nanoseconds duration; }; - virtual void GetReportInfo(ReportInfo* info) const = 0; + virtual void GetReportInfo(ReportInfo *info) const = 0; // Get upstream host UID. This value overrides the value in the report bag. - virtual bool GetDestinationUID(std::string* uid) const = 0; + virtual bool GetDestinationUID(std::string *uid) const = 0; // ConnectionEvent is used to indicates the tcp connection event in Report // call. @@ -53,7 +55,9 @@ class ReportData { // Get dynamic metadata generated by Envoy filters. // Useful for logging info generated by custom codecs. - virtual bool GetDynamicFilterState(std::string* filter_state) const = 0; + virtual const ::google::protobuf::Map<::std::string, + ::google::protobuf::Struct> + &GetDynamicFilterState() const = 0; }; } // namespace tcp diff --git a/include/istio/utils/attribute_names.h b/include/istio/utils/attribute_names.h index 4cc8b8cc336..c355e8d843e 100644 --- a/include/istio/utils/attribute_names.h +++ b/include/istio/utils/attribute_names.h @@ -47,9 +47,6 @@ struct AttributeName { static const char kRequestUserAgent[]; static const char kRequestApiKey[]; - // Dynamic state generated by various envoy http filters - static const char kRequestDynamicState[]; - static const char kResponseCode[]; static const char kResponseDuration[]; static const char kResponseHeaders[]; @@ -80,9 +77,6 @@ struct AttributeName { // Record TCP connection status: open, continue, close static const char kConnectionEvent[]; - // Dynamic state generated by various envoy network filters - static const char kConnectionDynamicState[]; - // Context attributes static const char kContextProtocol[]; static const char kContextReporterKind[]; diff --git a/include/istio/utils/attributes_builder.h b/include/istio/utils/attributes_builder.h index 82e71e89747..c48d872775c 100644 --- a/include/istio/utils/attributes_builder.h +++ b/include/istio/utils/attributes_builder.h @@ -32,32 +32,32 @@ namespace utils { // .Add("key2", value2); class AttributesBuilder { public: - AttributesBuilder(::istio::mixer::v1::Attributes* attributes) + AttributesBuilder(::istio::mixer::v1::Attributes *attributes) : attributes_(attributes) {} - void AddString(const std::string& key, const std::string& str) { + void AddString(const std::string &key, const std::string &str) { (*attributes_->mutable_attributes())[key].set_string_value(str); } - void AddBytes(const std::string& key, const std::string& bytes) { + void AddBytes(const std::string &key, const std::string &bytes) { (*attributes_->mutable_attributes())[key].set_bytes_value(bytes); } - void AddInt64(const std::string& key, int64_t value) { + void AddInt64(const std::string &key, int64_t value) { (*attributes_->mutable_attributes())[key].set_int64_value(value); } - void AddDouble(const std::string& key, double value) { + void AddDouble(const std::string &key, double value) { (*attributes_->mutable_attributes())[key].set_double_value(value); } - void AddBool(const std::string& key, bool value) { + void AddBool(const std::string &key, bool value) { (*attributes_->mutable_attributes())[key].set_bool_value(value); } void AddTimestamp( - const std::string& key, - const std::chrono::time_point& value) { + const std::string &key, + const std::chrono::time_point &value) { auto time_stamp = (*attributes_->mutable_attributes())[key].mutable_timestamp_value(); long long nanos = std::chrono::duration_cast( @@ -67,16 +67,16 @@ class AttributesBuilder { time_stamp->set_nanos(nanos % 1000000000); } - void AddDuration(const std::string& key, - const std::chrono::nanoseconds& value) { + void AddDuration(const std::string &key, + const std::chrono::nanoseconds &value) { auto duration = (*attributes_->mutable_attributes())[key].mutable_duration_value(); duration->set_seconds(value.count() / 1000000000); duration->set_nanos(value.count() % 1000000000); } - void AddStringMap(const std::string& key, - const std::map& string_map) { + void AddStringMap(const std::string &key, + const std::map &string_map) { if (string_map.size() == 0) { return; } @@ -84,13 +84,13 @@ class AttributesBuilder { .mutable_string_map_value() ->mutable_entries(); entries->clear(); - for (const auto& map_it : string_map) { + for (const auto &map_it : string_map) { (*entries)[map_it.first] = map_it.second; } } - void AddProtoStructStringMap(const std::string& key, - const google::protobuf::Struct& struct_map) { + void AddProtoStructStringMap(const std::string &key, + const google::protobuf::Struct &struct_map) { if (struct_map.fields().empty()) { return; } @@ -98,7 +98,7 @@ class AttributesBuilder { .mutable_string_map_value() ->mutable_entries(); entries->clear(); - for (const auto& field : struct_map.fields()) { + for (const auto &field : struct_map.fields()) { // Ignore all fields that are not string or string list. switch (field.second.kind_case()) { case google::protobuf::Value::kStringValue: @@ -122,15 +122,37 @@ class AttributesBuilder { break; } } + + if (entries->empty()) { + attributes_->mutable_attributes()->erase(key); + } + } + + // Serializes all the keys in a map and builds attributes. + // for example, foo.bar.com: struct {str:abc, list:[c,d,e]} will be emitted as + // foo.bar.com: string_map[str:abc, list: c,d,e] + // Only extracts strings and lists. + // TODO: add the ability to pack bools and nums as strings and recurse down + // the struct. + void FlattenMapOfStringToStruct( + const ::google::protobuf::Map<::std::string, ::google::protobuf::Struct> + &filter_state) { + if (filter_state.empty()) { + return; + } + + for (const auto &filter : filter_state) { + AddProtoStructStringMap(filter.first, filter.second); + } } - bool HasAttribute(const std::string& key) const { - const auto& attrs_map = attributes_->attributes(); + bool HasAttribute(const std::string &key) const { + const auto &attrs_map = attributes_->attributes(); return attrs_map.find(key) != attrs_map.end(); } private: - ::istio::mixer::v1::Attributes* attributes_; + ::istio::mixer::v1::Attributes *attributes_; }; } // namespace utils diff --git a/src/envoy/http/mixer/report_data.h b/src/envoy/http/mixer/report_data.h index 338e188fbfb..7d09992012d 100644 --- a/src/envoy/http/mixer/report_data.h +++ b/src/envoy/http/mixer/report_data.h @@ -20,6 +20,7 @@ #include "envoy/http/header_map.h" #include "envoy/stream_info/stream_info.h" #include "extensions/filters/http/well_known_names.h" +#include "google/protobuf/struct.pb.h" #include "include/istio/control/http/controller.h" #include "src/envoy/utils/utils.h" @@ -158,12 +159,9 @@ class ReportData : public ::istio::control::http::ReportData, } // Get attributes generated by http filters - bool GetDynamicFilterState(std::string *filter_state) const override { - if (info_.dynamicMetadata().filter_metadata_size() > 0) { - info_.dynamicMetadata().SerializeToString(filter_state); - return true; - } - return false; + const ::google::protobuf::Map + &GetDynamicFilterState() const override { + return info_.dynamicMetadata().filter_metadata(); } }; diff --git a/src/envoy/tcp/mixer/filter.cc b/src/envoy/tcp/mixer/filter.cc index dee47a00982..5043a44d7f2 100644 --- a/src/envoy/tcp/mixer/filter.cc +++ b/src/envoy/tcp/mixer/filter.cc @@ -177,19 +177,12 @@ bool Filter::GetDestinationUID(std::string *uid) const { } return false; } -bool Filter::GetDynamicFilterState(std::string *filter_state) const { - if (filter_callbacks_->connection() - .streamInfo() - .dynamicMetadata() - .filter_metadata_size() > 0) { - filter_callbacks_->connection() - .streamInfo() - .dynamicMetadata() - .SerializeToString(filter_state); - return true; - } - - return false; +const ::google::protobuf::Map + &Filter::GetDynamicFilterState() const { + return filter_callbacks_->connection() + .streamInfo() + .dynamicMetadata() + .filter_metadata(); } void Filter::GetReportInfo( ::istio::control::tcp::ReportData::ReportInfo *data) const { diff --git a/src/envoy/tcp/mixer/filter.h b/src/envoy/tcp/mixer/filter.h index c897e038d49..8bd13ca27d2 100644 --- a/src/envoy/tcp/mixer/filter.h +++ b/src/envoy/tcp/mixer/filter.h @@ -18,6 +18,7 @@ #include "common/common/logger.h" #include "envoy/network/connection.h" #include "envoy/network/filter.h" +#include "google/protobuf/struct.pb.h" #include "src/envoy/tcp/mixer/control.h" namespace Envoy { @@ -30,17 +31,17 @@ class Filter : public Network::Filter, public ::istio::control::tcp::ReportData, public Logger::Loggable { public: - Filter(Control& control); + Filter(Control &control); ~Filter(); void initializeReadFilterCallbacks( - Network::ReadFilterCallbacks& callbacks) override; + Network::ReadFilterCallbacks &callbacks) override; // Network::ReadFilter - Network::FilterStatus onData(Buffer::Instance& data, bool) override; + Network::FilterStatus onData(Buffer::Instance &data, bool) override; // Network::WriteFilter - Network::FilterStatus onWrite(Buffer::Instance& data, bool) override; + Network::FilterStatus onWrite(Buffer::Instance &data, bool) override; Network::FilterStatus onNewConnection() override; // Network::ConnectionCallbacks @@ -50,17 +51,18 @@ class Filter : public Network::Filter, void onBelowWriteBufferLowWatermark() override {} // CheckData virtual functions. - bool GetSourceIpPort(std::string* str_ip, int* port) const override; - bool GetPrincipal(bool peer, std::string* user) const override; + bool GetSourceIpPort(std::string *str_ip, int *port) const override; + bool GetPrincipal(bool peer, std::string *user) const override; bool IsMutualTLS() const override; - bool GetRequestedServerName(std::string* name) const override; + bool GetRequestedServerName(std::string *name) const override; // ReportData virtual functions. - bool GetDestinationIpPort(std::string* str_ip, int* port) const override; - bool GetDestinationUID(std::string* uid) const override; - bool GetDynamicFilterState(std::string* filter_state) const override; + bool GetDestinationIpPort(std::string *str_ip, int *port) const override; + bool GetDestinationUID(std::string *uid) const override; + const ::google::protobuf::Map + &GetDynamicFilterState() const override; void GetReportInfo( - ::istio::control::tcp::ReportData::ReportInfo* data) const override; + ::istio::control::tcp::ReportData::ReportInfo *data) const override; std::string GetConnectionId() const override; private: @@ -73,7 +75,7 @@ class Filter : public Network::Filter, void callCheck(); // Called when Check is done. - void completeCheck(const ::google::protobuf::util::Status& status); + void completeCheck(const ::google::protobuf::util::Status &status); // Cancel the pending Check call. void cancelCheck(); @@ -81,11 +83,11 @@ class Filter : public Network::Filter, // the cancel check istio::mixerclient::CancelFunc cancel_check_; // the control object. - Control& control_; + Control &control_; // pre-request handler std::unique_ptr<::istio::control::tcp::RequestHandler> handler_; // filter callback - Network::ReadFilterCallbacks* filter_callbacks_{}; + Network::ReadFilterCallbacks *filter_callbacks_{}; // state State state_{State::NotStarted}; // calling_check diff --git a/src/istio/control/http/attributes_builder.cc b/src/istio/control/http/attributes_builder.cc index fce41d3a480..5e4e69fdd68 100644 --- a/src/istio/control/http/attributes_builder.cc +++ b/src/istio/control/http/attributes_builder.cc @@ -253,10 +253,7 @@ void AttributesBuilder::ExtractReportAttributes(ReportData *report_data) { } } - std::string filter_state; - if (report_data->GetDynamicFilterState(&filter_state)) { - builder.AddBytes(utils::AttributeName::kRequestDynamicState, filter_state); - } + builder.FlattenMapOfStringToStruct(report_data->GetDynamicFilterState()); } } // namespace http diff --git a/src/istio/control/http/attributes_builder_test.cc b/src/istio/control/http/attributes_builder_test.cc index a0ceaa58f5b..31c14b947e9 100644 --- a/src/istio/control/http/attributes_builder_test.cc +++ b/src/istio/control/http/attributes_builder_test.cc @@ -31,6 +31,7 @@ using ::istio::mixer::v1::Attributes_StringMap; using ::testing::_; using ::testing::Invoke; +using ::testing::ReturnRef; namespace istio { namespace control { @@ -407,9 +408,18 @@ attributes { } } attributes { - key: "request.dynamic_state" + key: "foo.bar.com" value { - bytes_value: "aeiou" + string_map_value { + entries { + key: "str" + value: "abc" + } + entries { + key: "list" + value: "a,b,c" + } + } } } )"; @@ -716,6 +726,23 @@ TEST(AttributesBuilderTest, TestCheckAttributes) { TEST(AttributesBuilderTest, TestReportAttributes) { ::testing::StrictMock mock_data; + + ::google::protobuf::Map + filter_metadata; + ::google::protobuf::Struct struct_obj; + ::google::protobuf::Value strval, numval, boolval, listval; + strval.set_string_value("abc"); + (*struct_obj.mutable_fields())["str"] = strval; + numval.set_number_value(12.3); + (*struct_obj.mutable_fields())["num"] = numval; + boolval.set_bool_value(true); + (*struct_obj.mutable_fields())["bool"] = boolval; + listval.mutable_list_value()->add_values()->set_string_value("a"); + listval.mutable_list_value()->add_values()->set_string_value("b"); + listval.mutable_list_value()->add_values()->set_string_value("c"); + (*struct_obj.mutable_fields())["list"] = listval; + filter_metadata["foo.bar.com"] = struct_obj; + EXPECT_CALL(mock_data, GetDestinationIpPort(_, _)) .WillOnce(Invoke([](std::string *ip, int *port) -> bool { *ip = "1.2.3.4"; @@ -756,11 +783,8 @@ TEST(AttributesBuilderTest, TestReportAttributes) { report_info->permissive_policy_id = "policy-foo"; return true; })); - EXPECT_CALL(mock_data, GetDynamicFilterState(_)) - .WillOnce(Invoke([](std::string *dynamic_state) -> bool { - *dynamic_state = "aeiou"; - return true; - })); + EXPECT_CALL(mock_data, GetDynamicFilterState()) + .WillOnce(ReturnRef(filter_metadata)); RequestContext request; AttributesBuilder builder(&request); @@ -785,6 +809,23 @@ TEST(AttributesBuilderTest, TestReportAttributes) { TEST(AttributesBuilderTest, TestReportAttributesWithDestIP) { ::testing::StrictMock mock_data; + + ::google::protobuf::Map + filter_metadata; + ::google::protobuf::Struct struct_obj; + ::google::protobuf::Value strval, numval, boolval, listval; + strval.set_string_value("abc"); + (*struct_obj.mutable_fields())["str"] = strval; + numval.set_number_value(12.3); + (*struct_obj.mutable_fields())["num"] = numval; + boolval.set_bool_value(true); + (*struct_obj.mutable_fields())["bool"] = boolval; + listval.mutable_list_value()->add_values()->set_string_value("a"); + listval.mutable_list_value()->add_values()->set_string_value("b"); + listval.mutable_list_value()->add_values()->set_string_value("c"); + (*struct_obj.mutable_fields())["list"] = listval; + filter_metadata["foo.bar.com"] = struct_obj; + EXPECT_CALL(mock_data, GetDestinationIpPort(_, _)) .WillOnce(Invoke([](std::string *ip, int *port) -> bool { *ip = "2.3.4.5"; @@ -816,11 +857,8 @@ TEST(AttributesBuilderTest, TestReportAttributesWithDestIP) { report_info->permissive_policy_id = "policy-foo"; return true; })); - EXPECT_CALL(mock_data, GetDynamicFilterState(_)) - .WillOnce(Invoke([](std::string *dynamic_state) -> bool { - *dynamic_state = "aeiou"; - return true; - })); + EXPECT_CALL(mock_data, GetDynamicFilterState()) + .WillOnce(ReturnRef(filter_metadata)); RequestContext request; SetDestinationIp(&request, "1.2.3.4"); diff --git a/src/istio/control/http/mock_report_data.h b/src/istio/control/http/mock_report_data.h index 580a1c4d074..64c0402ef9b 100644 --- a/src/istio/control/http/mock_report_data.h +++ b/src/istio/control/http/mock_report_data.h @@ -27,12 +27,15 @@ namespace http { class MockReportData : public ReportData { public: MOCK_CONST_METHOD0(GetResponseHeaders, std::map()); - MOCK_CONST_METHOD1(GetReportInfo, void(ReportInfo* info)); - MOCK_CONST_METHOD2(GetDestinationIpPort, bool(std::string* ip, int* port)); - MOCK_CONST_METHOD1(GetDestinationUID, bool(std::string* ip)); - MOCK_CONST_METHOD1(GetGrpcStatus, bool(GrpcStatus* status)); - MOCK_CONST_METHOD1(GetRbacReportInfo, bool(RbacReportInfo* info)); - MOCK_CONST_METHOD1(GetDynamicFilterState, bool(std::string* filter_state)); + MOCK_CONST_METHOD1(GetReportInfo, void(ReportInfo *info)); + MOCK_CONST_METHOD2(GetDestinationIpPort, bool(std::string *ip, int *port)); + MOCK_CONST_METHOD1(GetDestinationUID, bool(std::string *ip)); + MOCK_CONST_METHOD1(GetGrpcStatus, bool(GrpcStatus *status)); + MOCK_CONST_METHOD1(GetRbacReportInfo, bool(RbacReportInfo *info)); + MOCK_CONST_METHOD0( + GetDynamicFilterState, + const ::google::protobuf::Map + &()); }; } // namespace http diff --git a/src/istio/control/http/request_handler_impl_test.cc b/src/istio/control/http/request_handler_impl_test.cc index eb5700f3dbe..4fc87aa12f0 100644 --- a/src/istio/control/http/request_handler_impl_test.cc +++ b/src/istio/control/http/request_handler_impl_test.cc @@ -38,6 +38,7 @@ using ::istio::utils::LocalAttributes; using ::testing::_; using ::testing::Invoke; +using ::testing::ReturnRef; namespace istio { namespace control { @@ -129,15 +130,15 @@ class RequestHandlerImplTest : public ::testing::Test { public: void SetUp() { SetUpMockController(kDefaultClientConfig); } - void SetUpMockController(const std::string& config_text) { + void SetUpMockController(const std::string &config_text) { SetUpMockController(config_text, kLocalInbound, kLocalOutbound, kLocalForward); } - void SetUpMockController(const std::string& config_text, - const std::string& local_inbound_attributes, - const std::string& local_outbound_attributes, - const std::string& local_forward_attributes) { + void SetUpMockController(const std::string &config_text, + const std::string &local_inbound_attributes, + const std::string &local_outbound_attributes, + const std::string &local_forward_attributes) { ASSERT_TRUE(TextFormat::ParseFromString(config_text, &client_config_)); LocalAttributes la; @@ -158,19 +159,19 @@ class RequestHandlerImplTest : public ::testing::Test { std::unique_ptr(new ControllerImpl(client_context_)); } - void SetServiceConfig(const std::string& name, const ServiceConfig& config) { + void SetServiceConfig(const std::string &name, const ServiceConfig &config) { (*client_config_.mutable_service_configs())[name] = config; } - void ApplyPerRouteConfig(const ServiceConfig& service_config, - Controller::PerRouteConfig* per_route) { + void ApplyPerRouteConfig(const ServiceConfig &service_config, + Controller::PerRouteConfig *per_route) { per_route->service_config_id = "1111"; controller_->AddServiceConfig(per_route->service_config_id, service_config); } std::shared_ptr client_context_; HttpClientConfig client_config_; - ::testing::NiceMock* mock_client_; + ::testing::NiceMock *mock_client_; std::unique_ptr controller_; }; @@ -210,7 +211,7 @@ TEST_F(RequestHandlerImplTest, TestHandlerDisabledCheckReport) { auto handler = controller_->CreateRequestHandler(per_route); handler->Check(&mock_data, &mock_header, nullptr, - [](const CheckResponseInfo& info) { + [](const CheckResponseInfo &info) { EXPECT_TRUE(info.response_status.ok()); }); } @@ -232,7 +233,7 @@ TEST_F(RequestHandlerImplTest, TestHandlerDisabledCheck) { auto handler = controller_->CreateRequestHandler(per_route); handler->Check(&mock_data, &mock_header, nullptr, - [](const CheckResponseInfo& info) { + [](const CheckResponseInfo &info) { EXPECT_TRUE(info.response_status.ok()); }); } @@ -245,8 +246,8 @@ TEST_F(RequestHandlerImplTest, TestPerRouteAttributes) { // Check should be called. EXPECT_CALL(*mock_client_, Check(_, _, _, _)) - .WillOnce(Invoke([](const Attributes& attributes, - const std::vector& quotas, + .WillOnce(Invoke([](const Attributes &attributes, + const std::vector "as, TransportCheckFunc transport, CheckDoneFunc on_done) -> CancelFunc { auto map = attributes.attributes(); @@ -273,8 +274,8 @@ TEST_F(RequestHandlerImplTest, TestDefaultRouteAttributes) { // Check should be called. EXPECT_CALL(*mock_client_, Check(_, _, _, _)) - .WillOnce(Invoke([](const Attributes& attributes, - const std::vector& quotas, + .WillOnce(Invoke([](const Attributes &attributes, + const std::vector "as, TransportCheckFunc transport, CheckDoneFunc on_done) -> CancelFunc { auto map = attributes.attributes(); @@ -285,7 +286,7 @@ TEST_F(RequestHandlerImplTest, TestDefaultRouteAttributes) { // Attribute is forwarded: route override EXPECT_CALL(mock_header, AddIstioAttributes(_)) - .WillOnce(Invoke([](const std::string& data) { + .WillOnce(Invoke([](const std::string &data) { Attributes forwarded_attr; EXPECT_TRUE(forwarded_attr.ParseFromString(data)); auto map = forwarded_attr.attributes(); @@ -312,8 +313,8 @@ TEST_F(RequestHandlerImplTest, TestRouteAttributes) { // Check should be called. EXPECT_CALL(*mock_client_, Check(_, _, _, _)) - .WillOnce(Invoke([](const Attributes& attributes, - const std::vector& quotas, + .WillOnce(Invoke([](const Attributes &attributes, + const std::vector "as, TransportCheckFunc transport, CheckDoneFunc on_done) -> CancelFunc { auto map = attributes.attributes(); @@ -324,7 +325,7 @@ TEST_F(RequestHandlerImplTest, TestRouteAttributes) { // Attribute is forwarded: global EXPECT_CALL(mock_header, AddIstioAttributes(_)) - .WillOnce(Invoke([](const std::string& data) { + .WillOnce(Invoke([](const std::string &data) { Attributes forwarded_attr; EXPECT_TRUE(forwarded_attr.ParseFromString(data)); auto map = forwarded_attr.attributes(); @@ -343,8 +344,8 @@ TEST_F(RequestHandlerImplTest, TestPerRouteQuota) { // Check should be called. EXPECT_CALL(*mock_client_, Check(_, _, _, _)) - .WillOnce(Invoke([](const Attributes& attributes, - const std::vector& quotas, + .WillOnce(Invoke([](const Attributes &attributes, + const std::vector "as, TransportCheckFunc transport, CheckDoneFunc on_done) -> CancelFunc { auto map = attributes.attributes(); @@ -371,7 +372,7 @@ TEST_F(RequestHandlerImplTest, TestPerRouteApiSpec) { ::testing::NiceMock mock_header; EXPECT_CALL(mock_data, FindHeaderByType(_, _)) .WillRepeatedly( - Invoke([](CheckData::HeaderType type, std::string* value) -> bool { + Invoke([](CheckData::HeaderType type, std::string *value) -> bool { if (type == CheckData::HEADER_PATH) { *value = "/books/120"; return true; @@ -385,8 +386,8 @@ TEST_F(RequestHandlerImplTest, TestPerRouteApiSpec) { // Check should be called. EXPECT_CALL(*mock_client_, Check(_, _, _, _)) - .WillOnce(Invoke([](const Attributes& attributes, - const std::vector& quotas, + .WillOnce(Invoke([](const Attributes &attributes, + const std::vector "as, TransportCheckFunc transport, CheckDoneFunc on_done) -> CancelFunc { auto map = attributes.attributes(); @@ -435,7 +436,7 @@ TEST_F(RequestHandlerImplTest, TestDefaultApiKey) { ::testing::NiceMock mock_header; EXPECT_CALL(mock_data, FindQueryParameter(_, _)) .WillRepeatedly( - Invoke([](const std::string& name, std::string* value) -> bool { + Invoke([](const std::string &name, std::string *value) -> bool { if (name == "key") { *value = "test-api-key"; return true; @@ -445,8 +446,8 @@ TEST_F(RequestHandlerImplTest, TestDefaultApiKey) { // Check should be called. EXPECT_CALL(*mock_client_, Check(_, _, _, _)) - .WillOnce(Invoke([](const Attributes& attributes, - const std::vector& quotas, + .WillOnce(Invoke([](const Attributes &attributes, + const std::vector "as, TransportCheckFunc transport, CheckDoneFunc on_done) -> CancelFunc { auto map = attributes.attributes(); @@ -464,9 +465,14 @@ TEST_F(RequestHandlerImplTest, TestDefaultApiKey) { TEST_F(RequestHandlerImplTest, TestHandlerReport) { ::testing::NiceMock mock_check; ::testing::NiceMock mock_report; + ::google::protobuf::Map + filter_metadata; EXPECT_CALL(mock_check, GetSourceIpPort(_, _)).Times(1); EXPECT_CALL(mock_report, GetResponseHeaders()).Times(1); EXPECT_CALL(mock_report, GetReportInfo(_)).Times(1); + EXPECT_CALL(mock_report, GetDynamicFilterState()) + .Times(1) + .WillOnce(ReturnRef(filter_metadata)); // Report should be called. EXPECT_CALL(*mock_client_, Report(_)).Times(1); @@ -485,6 +491,7 @@ TEST_F(RequestHandlerImplTest, TestHandlerDisabledReport) { EXPECT_CALL(mock_check, GetSourceIpPort(_, _)).Times(0); EXPECT_CALL(mock_report, GetResponseHeaders()).Times(0); EXPECT_CALL(mock_report, GetReportInfo(_)).Times(0); + EXPECT_CALL(mock_report, GetDynamicFilterState()).Times(0); // Report should NOT be called. EXPECT_CALL(*mock_client_, Report(_)).Times(0); @@ -509,7 +516,7 @@ TEST_F(RequestHandlerImplTest, TestEmptyConfig) { // Attributes is forwarded. EXPECT_CALL(mock_header, AddIstioAttributes(_)) - .WillOnce(Invoke([](const std::string& data) { + .WillOnce(Invoke([](const std::string &data) { Attributes forwarded_attr; EXPECT_TRUE(forwarded_attr.ParseFromString(data)); auto map = forwarded_attr.attributes(); @@ -522,6 +529,7 @@ TEST_F(RequestHandlerImplTest, TestEmptyConfig) { ::testing::NiceMock mock_report; EXPECT_CALL(mock_report, GetResponseHeaders()).Times(0); EXPECT_CALL(mock_report, GetReportInfo(_)).Times(0); + EXPECT_CALL(mock_report, GetDynamicFilterState()).Times(0); // Report should NOT be called. EXPECT_CALL(*mock_client_, Report(_)).Times(0); @@ -529,7 +537,7 @@ TEST_F(RequestHandlerImplTest, TestEmptyConfig) { Controller::PerRouteConfig config; auto handler = controller_->CreateRequestHandler(config); handler->Check(&mock_check, &mock_header, nullptr, - [](const CheckResponseInfo& info) { + [](const CheckResponseInfo &info) { EXPECT_TRUE(info.response_status.ok()); }); handler->Report(&mock_check, &mock_report); diff --git a/src/istio/control/tcp/attributes_builder.cc b/src/istio/control/tcp/attributes_builder.cc index d523e605f0c..0c8876e7923 100644 --- a/src/istio/control/tcp/attributes_builder.cc +++ b/src/istio/control/tcp/attributes_builder.cc @@ -132,11 +132,7 @@ void AttributesBuilder::ExtractReportAttributes( builder.AddString(utils::AttributeName::kDestinationUID, uid); } - std::string filter_state; - if (report_data->GetDynamicFilterState(&filter_state)) { - builder.AddBytes(utils::AttributeName::kConnectionDynamicState, - filter_state); - } + builder.FlattenMapOfStringToStruct(report_data->GetDynamicFilterState()); builder.AddTimestamp(utils::AttributeName::kContextTime, std::chrono::system_clock::now()); diff --git a/src/istio/control/tcp/attributes_builder_test.cc b/src/istio/control/tcp/attributes_builder_test.cc index 02c96306fb4..897b7652d0e 100644 --- a/src/istio/control/tcp/attributes_builder_test.cc +++ b/src/istio/control/tcp/attributes_builder_test.cc @@ -30,6 +30,7 @@ using ::google::protobuf::util::MessageDifferencer; using ::testing::_; using ::testing::Invoke; using ::testing::Return; +using ::testing::ReturnRef; namespace istio { namespace control { @@ -163,9 +164,18 @@ attributes { } } attributes { - key: "connection.dynamic_state" + key: "foo.bar.com" value { - bytes_value: "aeiou" + string_map_value { + entries { + key: "str" + value: "abc" + } + entries { + key: "list" + value: "a,b,c" + } + } } } )"; @@ -247,9 +257,18 @@ attributes { } } attributes { - key: "connection.dynamic_state" + key: "foo.bar.com" value { - bytes_value: "aeiou" + string_map_value { + entries { + key: "str" + value: "abc" + } + entries { + key: "list" + value: "a,b,c" + } + } } } )"; @@ -311,9 +330,18 @@ attributes { } } attributes { - key: "connection.dynamic_state" + key: "foo.bar.com" value { - bytes_value: "aeiou" + string_map_value { + entries { + key: "str" + value: "abc" + } + entries { + key: "list" + value: "a,b,c" + } + } } } )"; @@ -375,14 +403,23 @@ attributes { } } attributes { - key: "connection.dynamic_state" + key: "foo.bar.com" value { - bytes_value: "aeiou" + string_map_value { + entries { + key: "str" + value: "abc" + } + entries { + key: "list" + value: "a,b,c" + } + } } } )"; -void ClearContextTime(RequestContext* request) { +void ClearContextTime(RequestContext *request) { // Override timestamp with - utils::AttributesBuilder builder(request->attributes); std::chrono::time_point time0; @@ -392,7 +429,7 @@ void ClearContextTime(RequestContext* request) { TEST(AttributesBuilderTest, TestCheckAttributes) { ::testing::NiceMock mock_data; EXPECT_CALL(mock_data, GetSourceIpPort(_, _)) - .WillOnce(Invoke([](std::string* ip, int* port) -> bool { + .WillOnce(Invoke([](std::string *ip, int *port) -> bool { *ip = "1.2.3.4"; *port = 8080; return true; @@ -401,7 +438,7 @@ TEST(AttributesBuilderTest, TestCheckAttributes) { return true; })); EXPECT_CALL(mock_data, GetPrincipal(_, _)) - .WillRepeatedly(Invoke([](bool peer, std::string* user) -> bool { + .WillRepeatedly(Invoke([](bool peer, std::string *user) -> bool { if (peer) { *user = "cluster.local/sa/test_user/ns/ns_ns/"; } else { @@ -411,7 +448,7 @@ TEST(AttributesBuilderTest, TestCheckAttributes) { })); EXPECT_CALL(mock_data, GetConnectionId()).WillOnce(Return("1234-5")); EXPECT_CALL(mock_data, GetRequestedServerName(_)) - .WillOnce(Invoke([](std::string* name) -> bool { + .WillOnce(Invoke([](std::string *name) -> bool { *name = "www.google.com"; return true; })); @@ -434,43 +471,57 @@ TEST(AttributesBuilderTest, TestCheckAttributes) { TEST(AttributesBuilderTest, TestReportAttributes) { ::testing::NiceMock mock_data; + + ::google::protobuf::Map + filter_metadata; + ::google::protobuf::Struct struct_obj; + ::google::protobuf::Value strval, numval, boolval, listval; + strval.set_string_value("abc"); + (*struct_obj.mutable_fields())["str"] = strval; + numval.set_number_value(12.3); + (*struct_obj.mutable_fields())["num"] = numval; + boolval.set_bool_value(true); + (*struct_obj.mutable_fields())["bool"] = boolval; + listval.mutable_list_value()->add_values()->set_string_value("a"); + listval.mutable_list_value()->add_values()->set_string_value("b"); + listval.mutable_list_value()->add_values()->set_string_value("c"); + (*struct_obj.mutable_fields())["list"] = listval; + filter_metadata["foo.bar.com"] = struct_obj; + EXPECT_CALL(mock_data, GetDestinationIpPort(_, _)) .Times(4) - .WillRepeatedly(Invoke([](std::string* ip, int* port) -> bool { + .WillRepeatedly(Invoke([](std::string *ip, int *port) -> bool { *ip = "1.2.3.4"; *port = 8080; return true; })); EXPECT_CALL(mock_data, GetDestinationUID(_)) .Times(4) - .WillRepeatedly(Invoke([](std::string* uid) -> bool { + .WillRepeatedly(Invoke([](std::string *uid) -> bool { *uid = "pod1.ns2"; return true; })); - EXPECT_CALL(mock_data, GetDynamicFilterState(_)) + EXPECT_CALL(mock_data, GetDynamicFilterState()) .Times(4) - .WillRepeatedly(Invoke([](std::string* dynamic_state) -> bool { - *dynamic_state = "aeiou"; - return true; - })); + .WillRepeatedly(ReturnRef(filter_metadata)); EXPECT_CALL(mock_data, GetReportInfo(_)) .Times(4) - .WillOnce(Invoke([](ReportData::ReportInfo* info) { + .WillOnce(Invoke([](ReportData::ReportInfo *info) { info->received_bytes = 0; info->send_bytes = 0; info->duration = std::chrono::nanoseconds(1); })) - .WillOnce(Invoke([](ReportData::ReportInfo* info) { + .WillOnce(Invoke([](ReportData::ReportInfo *info) { info->received_bytes = 100; info->send_bytes = 200; info->duration = std::chrono::nanoseconds(2); })) - .WillOnce(Invoke([](ReportData::ReportInfo* info) { + .WillOnce(Invoke([](ReportData::ReportInfo *info) { info->received_bytes = 201; info->send_bytes = 404; info->duration = std::chrono::nanoseconds(3); })) - .WillOnce(Invoke([](ReportData::ReportInfo* info) { + .WillOnce(Invoke([](ReportData::ReportInfo *info) { info->received_bytes = 345; info->send_bytes = 678; info->duration = std::chrono::nanoseconds(4); diff --git a/src/istio/control/tcp/mock_report_data.h b/src/istio/control/tcp/mock_report_data.h index 1631cebfae6..85c749ff360 100644 --- a/src/istio/control/tcp/mock_report_data.h +++ b/src/istio/control/tcp/mock_report_data.h @@ -26,10 +26,13 @@ namespace tcp { // The mock object for ReportData interface. class MockReportData : public ReportData { public: - MOCK_CONST_METHOD2(GetDestinationIpPort, bool(std::string* ip, int* port)); - MOCK_CONST_METHOD1(GetDestinationUID, bool(std::string*)); - MOCK_CONST_METHOD1(GetDynamicFilterState, bool(std::string*)); - MOCK_CONST_METHOD1(GetReportInfo, void(ReportInfo* info)); + MOCK_CONST_METHOD2(GetDestinationIpPort, bool(std::string *ip, int *port)); + MOCK_CONST_METHOD1(GetDestinationUID, bool(std::string *)); + MOCK_CONST_METHOD0( + GetDynamicFilterState, + const ::google::protobuf::Map + &()); + MOCK_CONST_METHOD1(GetReportInfo, void(ReportInfo *info)); }; } // namespace tcp diff --git a/src/istio/control/tcp/request_handler_impl_test.cc b/src/istio/control/tcp/request_handler_impl_test.cc index eb8b9559c94..c27763b3bc4 100644 --- a/src/istio/control/tcp/request_handler_impl_test.cc +++ b/src/istio/control/tcp/request_handler_impl_test.cc @@ -36,6 +36,7 @@ using ::istio::utils::LocalAttributes; using ::testing::_; using ::testing::Invoke; +using ::testing::ReturnRef; namespace istio { namespace control { @@ -97,7 +98,7 @@ class RequestHandlerImplTest : public ::testing::Test { std::shared_ptr client_context_; TcpClientConfig client_config_; - ::testing::NiceMock* mock_client_; + ::testing::NiceMock *mock_client_; std::unique_ptr controller_; }; @@ -111,7 +112,7 @@ TEST_F(RequestHandlerImplTest, TestHandlerDisabledCheck) { client_config_.set_disable_check_calls(true); auto handler = controller_->CreateRequestHandler(); - handler->Check(&mock_data, [](const CheckResponseInfo& info) { + handler->Check(&mock_data, [](const CheckResponseInfo &info) { EXPECT_TRUE(info.response_status.ok()); }); } @@ -123,8 +124,8 @@ TEST_F(RequestHandlerImplTest, TestHandlerCheck) { // Check should be called. EXPECT_CALL(*mock_client_, Check(_, _, _, _)) - .WillOnce(Invoke([](const Attributes& attributes, - const std::vector& quotas, + .WillOnce(Invoke([](const Attributes &attributes, + const std::vector "as, TransportCheckFunc transport, CheckDoneFunc on_done) -> CancelFunc { auto map = attributes.attributes(); @@ -141,9 +142,14 @@ TEST_F(RequestHandlerImplTest, TestHandlerCheck) { TEST_F(RequestHandlerImplTest, TestHandlerReport) { ::testing::NiceMock mock_data; + ::google::protobuf::Map + filter_metadata; EXPECT_CALL(mock_data, GetDestinationIpPort(_, _)).Times(1); EXPECT_CALL(mock_data, GetDestinationUID(_)).Times(1); EXPECT_CALL(mock_data, GetReportInfo(_)).Times(1); + EXPECT_CALL(mock_data, GetDynamicFilterState()) + .Times(1) + .WillOnce(ReturnRef(filter_metadata)); // Report should be called. EXPECT_CALL(*mock_client_, Report(_)).Times(1); diff --git a/src/istio/utils/attribute_names.cc b/src/istio/utils/attribute_names.cc index 86c04960814..392bc1c45ef 100644 --- a/src/istio/utils/attribute_names.cc +++ b/src/istio/utils/attribute_names.cc @@ -39,9 +39,6 @@ const char AttributeName::kRequestTime[] = "request.time"; const char AttributeName::kRequestUserAgent[] = "request.useragent"; const char AttributeName::kRequestApiKey[] = "request.api_key"; -// Dynamic state generated by various envoy http filters -const char AttributeName::kRequestDynamicState[] = "request.dynamic_state"; - const char AttributeName::kResponseCode[] = "response.code"; const char AttributeName::kResponseDuration[] = "response.duration"; const char AttributeName::kResponseHeaders[] = "response.headers"; @@ -75,10 +72,6 @@ const char AttributeName::kConnectionRequestedServerName[] = const char AttributeName::kConnectionId[] = "connection.id"; const char AttributeName::kConnectionEvent[] = "connection.event"; -// Dynamic state generated by various envoy network filters -const char AttributeName::kConnectionDynamicState[] = - "connection.dynamic_state"; - // Context attributes const char AttributeName::kContextProtocol[] = "context.protocol"; const char AttributeName::kContextReporterKind[] = "context.reporter.kind"; diff --git a/src/istio/utils/utils.h b/src/istio/utils/utils.h index 063c18ca402..0e27aa82d27 100644 --- a/src/istio/utils/utils.h +++ b/src/istio/utils/utils.h @@ -16,6 +16,7 @@ #pragma once #include +#include "google/protobuf/struct.pb.h" namespace istio { namespace utils { @@ -23,6 +24,5 @@ namespace utils { // Get source.namespace attribute from principal. bool GetSourceNamespace(const std::string& principal, std::string* source_namespace); - } // namespace utils } // namespace istio