From fe7703edc6be415d44aeb068247e746ec517a6ed Mon Sep 17 00:00:00 2001 From: Wayne Zhang Date: Fri, 21 Sep 2018 23:29:47 +0000 Subject: [PATCH 1/2] Not to add check attributes if check is disabled Signed-off-by: Wayne Zhang --- src/envoy/http/mixer/filter.cc | 11 ++++--- .../control/http/request_handler_impl.cc | 31 ++++++++++++++++--- src/istio/control/http/request_handler_impl.h | 10 ++++++ .../control/http/request_handler_impl_test.cc | 26 +++++++++++++++- 4 files changed, 69 insertions(+), 9 deletions(-) diff --git a/src/envoy/http/mixer/filter.cc b/src/envoy/http/mixer/filter.cc index 54bef4c4b28..3221ce22e34 100644 --- a/src/envoy/http/mixer/filter.cc +++ b/src/envoy/http/mixer/filter.cc @@ -282,11 +282,14 @@ void Filter::log(const HeaderMap* request_headers, ::istio::control::http::Controller::PerRouteConfig config; ReadPerRouteConfig(request_info.routeEntry(), &config); handler_ = control_.controller()->CreateRequestHandler(config); - - CheckData check_data(*request_headers, request_info.dynamicMetadata(), - nullptr); - handler_->ExtractRequestAttributes(&check_data); } + + // If check is NOT called, some request attributes are not extracted. + // If they are already extracted, handler will skip it. + CheckData check_data(*request_headers, request_info.dynamicMetadata(), + decoder_callbacks_->connection()); + handler_->ExtractRequestAttributes(&check_data); + // response trailer header is not counted to response total size. ReportData report_data(response_headers, response_trailers, request_info, request_total_size_); diff --git a/src/istio/control/http/request_handler_impl.cc b/src/istio/control/http/request_handler_impl.cc index 9b9ca09d924..b206c8f7fc2 100644 --- a/src/istio/control/http/request_handler_impl.cc +++ b/src/istio/control/http/request_handler_impl.cc @@ -29,26 +29,47 @@ namespace http { RequestHandlerImpl::RequestHandlerImpl( std::shared_ptr service_context) - : service_context_(service_context) {} + : service_context_(service_context), + check_attributes_added_(false), + forward_attributes_added_(false) {} + +void RequestHandlerImpl::AddForwardAttributes(CheckData* check_data) { + if (forward_attributes_added_) { + return; + } + forward_attributes_added_ = true; + + AttributesBuilder builder(&request_context_); + builder.ExtractForwardedAttributes(check_data); +} + +void RequestHandlerImpl::AddCheckAttributes(CheckData* check_data) { + if (check_attributes_added_) { + return; + } + check_attributes_added_ = true; -void RequestHandlerImpl::ExtractRequestAttributes(CheckData* check_data) { if (service_context_->enable_mixer_check() || service_context_->enable_mixer_report()) { service_context_->AddStaticAttributes(&request_context_); AttributesBuilder builder(&request_context_); - builder.ExtractForwardedAttributes(check_data); builder.ExtractCheckAttributes(check_data); service_context_->AddApiAttributes(check_data, &request_context_); } } +void RequestHandlerImpl::ExtractRequestAttributes(CheckData* check_data) { + AddForwardAttributes(check_data); + AddCheckAttributes(check_data); +} + CancelFunc RequestHandlerImpl::Check(CheckData* check_data, HeaderUpdate* header_update, TransportCheckFunc transport, CheckDoneFunc on_done) { - ExtractRequestAttributes(check_data); + AddForwardAttributes(check_data); header_update->RemoveIstioAttributes(); service_context_->InjectForwardedAttributes(header_update); @@ -59,6 +80,8 @@ CancelFunc RequestHandlerImpl::Check(CheckData* check_data, return nullptr; } + AddCheckAttributes(check_data); + service_context_->AddQuotas(&request_context_); return service_context_->client_context()->SendCheck(transport, on_done, diff --git a/src/istio/control/http/request_handler_impl.h b/src/istio/control/http/request_handler_impl.h index e858094a724..b244ffa1268 100644 --- a/src/istio/control/http/request_handler_impl.h +++ b/src/istio/control/http/request_handler_impl.h @@ -42,11 +42,21 @@ class RequestHandlerImpl : public RequestHandler { void ExtractRequestAttributes(CheckData* check_data) override; private: + // Add Forward attributes, allow re-entry + void AddForwardAttributes(CheckData* check_data); + // Add check attributes, allow re-entry + void AddCheckAttributes(CheckData* check_data); + // The request context object. RequestContext request_context_; // The service context. std::shared_ptr service_context_; + + // If true, request attributes are added + bool check_attributes_added_; + // If true, forward attributes are added + bool forward_attributes_added_; }; } // 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 45ad854981b..665136ae22f 100644 --- a/src/istio/control/http/request_handler_impl_test.cc +++ b/src/istio/control/http/request_handler_impl_test.cc @@ -218,7 +218,29 @@ TEST_F(RequestHandlerImplTest, TestHandlerDisabledCheckReport) { TEST_F(RequestHandlerImplTest, TestHandlerDisabledCheck) { ::testing::NiceMock mock_data; ::testing::NiceMock mock_header; - // Report is enabled so Attributes are extracted. + // Report is enabled so Check Attributes are not extracted. + EXPECT_CALL(mock_data, GetSourceIpPort(_, _)).Times(0); + EXPECT_CALL(mock_data, GetPrincipal(_, _)).Times(0); + + // Check should NOT be called. + EXPECT_CALL(*mock_client_, Check(_, _, _, _)).Times(0); + + ServiceConfig config; + config.set_disable_check_calls(true); + Controller::PerRouteConfig per_route; + ApplyPerRouteConfig(config, &per_route); + + auto handler = controller_->CreateRequestHandler(per_route); + handler->Check(&mock_data, &mock_header, nullptr, + [](const CheckResponseInfo& info) { + EXPECT_TRUE(info.response_status.ok()); + }); +} + +TEST_F(RequestHandlerImplTest, TestHandlerDisabledCheckWithExtract) { + ::testing::NiceMock mock_data; + ::testing::NiceMock mock_header; + // Report is enabled so Check Attributes are not extracted. EXPECT_CALL(mock_data, GetSourceIpPort(_, _)).Times(1); EXPECT_CALL(mock_data, GetPrincipal(_, _)).Times(2); @@ -235,6 +257,8 @@ TEST_F(RequestHandlerImplTest, TestHandlerDisabledCheck) { [](const CheckResponseInfo& info) { EXPECT_TRUE(info.response_status.ok()); }); + + handler->ExtractRequestAttributes(&mock_data); } TEST_F(RequestHandlerImplTest, TestPerRouteAttributes) { From 7b36290673a1bb2761bb9f4304bd9b69e36a37d3 Mon Sep 17 00:00:00 2001 From: Wayne Zhang Date: Sat, 22 Sep 2018 00:25:03 +0000 Subject: [PATCH 2/2] Remove ExtractRequestAttributes Signed-off-by: Wayne Zhang --- include/istio/control/http/request_handler.h | 18 +------- src/envoy/http/mixer/filter.cc | 7 +-- .../control/http/request_handler_impl.cc | 14 +++--- src/istio/control/http/request_handler_impl.h | 4 +- .../control/http/request_handler_impl_test.cc | 46 ++++++------------- 5 files changed, 26 insertions(+), 63 deletions(-) diff --git a/include/istio/control/http/request_handler.h b/include/istio/control/http/request_handler.h index ed863c7f468..916b6191dc0 100644 --- a/include/istio/control/http/request_handler.h +++ b/include/istio/control/http/request_handler.h @@ -42,24 +42,10 @@ class RequestHandler { // Make a Report call. It will: // * check service config to see if Report is required + // * extract check attributes if not done yet. // * extract more report attributes // * make a Report call. - virtual void Report(ReportData* report_data) = 0; - - // Extract the request attributes for Report() call. - // This is called at Report time when Check() is not called. - // Normally request attributes are extracted at Check() call. - // This is for cases the requests are rejected by http filters - // before mixer, such as fault injection, or auth. - // - // Usage: at Envoy filter::log() function - // if (!hander) { - // handle = control->CreateHandler(); - // handler->ExtractRequestAttributes(); - // } - // handler->Report(); - // - virtual void ExtractRequestAttributes(CheckData* check_data) = 0; + virtual void Report(CheckData* check_data, ReportData* report_data) = 0; }; } // namespace http diff --git a/src/envoy/http/mixer/filter.cc b/src/envoy/http/mixer/filter.cc index 3221ce22e34..40913a03f28 100644 --- a/src/envoy/http/mixer/filter.cc +++ b/src/envoy/http/mixer/filter.cc @@ -284,16 +284,13 @@ void Filter::log(const HeaderMap* request_headers, handler_ = control_.controller()->CreateRequestHandler(config); } - // If check is NOT called, some request attributes are not extracted. - // If they are already extracted, handler will skip it. + // If check is NOT called, check attributes are not extracted. CheckData check_data(*request_headers, request_info.dynamicMetadata(), decoder_callbacks_->connection()); - handler_->ExtractRequestAttributes(&check_data); - // response trailer header is not counted to response total size. ReportData report_data(response_headers, response_trailers, request_info, request_total_size_); - handler_->Report(&report_data); + handler_->Report(&check_data, &report_data); } } // namespace Mixer diff --git a/src/istio/control/http/request_handler_impl.cc b/src/istio/control/http/request_handler_impl.cc index b206c8f7fc2..87144b4a8ec 100644 --- a/src/istio/control/http/request_handler_impl.cc +++ b/src/istio/control/http/request_handler_impl.cc @@ -60,15 +60,12 @@ void RequestHandlerImpl::AddCheckAttributes(CheckData* check_data) { } } -void RequestHandlerImpl::ExtractRequestAttributes(CheckData* check_data) { - AddForwardAttributes(check_data); - AddCheckAttributes(check_data); -} - CancelFunc RequestHandlerImpl::Check(CheckData* check_data, HeaderUpdate* header_update, TransportCheckFunc transport, CheckDoneFunc on_done) { + // Forwarded attributes need to be stored regardless Check is needed + // or not since the header will be updated or removed. AddForwardAttributes(check_data); header_update->RemoveIstioAttributes(); service_context_->InjectForwardedAttributes(header_update); @@ -89,10 +86,15 @@ CancelFunc RequestHandlerImpl::Check(CheckData* check_data, } // Make remote report call. -void RequestHandlerImpl::Report(ReportData* report_data) { +void RequestHandlerImpl::Report(CheckData* check_data, + ReportData* report_data) { if (!service_context_->enable_mixer_report()) { return; } + + AddForwardAttributes(check_data); + AddCheckAttributes(check_data); + AttributesBuilder builder(&request_context_); builder.ExtractReportAttributes(report_data); diff --git a/src/istio/control/http/request_handler_impl.h b/src/istio/control/http/request_handler_impl.h index b244ffa1268..0e8c2b28072 100644 --- a/src/istio/control/http/request_handler_impl.h +++ b/src/istio/control/http/request_handler_impl.h @@ -37,9 +37,7 @@ class RequestHandlerImpl : public RequestHandler { ::istio::mixerclient::CheckDoneFunc on_done) override; // Make a Report call. - void Report(ReportData* report_data) override; - - void ExtractRequestAttributes(CheckData* check_data) override; + void Report(CheckData* check_data, ReportData* report_data) override; private: // Add Forward attributes, allow re-entry diff --git a/src/istio/control/http/request_handler_impl_test.cc b/src/istio/control/http/request_handler_impl_test.cc index 665136ae22f..eb5700f3dbe 100644 --- a/src/istio/control/http/request_handler_impl_test.cc +++ b/src/istio/control/http/request_handler_impl_test.cc @@ -237,30 +237,6 @@ TEST_F(RequestHandlerImplTest, TestHandlerDisabledCheck) { }); } -TEST_F(RequestHandlerImplTest, TestHandlerDisabledCheckWithExtract) { - ::testing::NiceMock mock_data; - ::testing::NiceMock mock_header; - // Report is enabled so Check Attributes are not extracted. - EXPECT_CALL(mock_data, GetSourceIpPort(_, _)).Times(1); - EXPECT_CALL(mock_data, GetPrincipal(_, _)).Times(2); - - // Check should NOT be called. - EXPECT_CALL(*mock_client_, Check(_, _, _, _)).Times(0); - - ServiceConfig config; - config.set_disable_check_calls(true); - Controller::PerRouteConfig per_route; - ApplyPerRouteConfig(config, &per_route); - - auto handler = controller_->CreateRequestHandler(per_route); - handler->Check(&mock_data, &mock_header, nullptr, - [](const CheckResponseInfo& info) { - EXPECT_TRUE(info.response_status.ok()); - }); - - handler->ExtractRequestAttributes(&mock_data); -} - TEST_F(RequestHandlerImplTest, TestPerRouteAttributes) { ::testing::NiceMock mock_data; ::testing::NiceMock mock_header; @@ -486,9 +462,11 @@ TEST_F(RequestHandlerImplTest, TestDefaultApiKey) { } TEST_F(RequestHandlerImplTest, TestHandlerReport) { - ::testing::NiceMock mock_data; - EXPECT_CALL(mock_data, GetResponseHeaders()).Times(1); - EXPECT_CALL(mock_data, GetReportInfo(_)).Times(1); + ::testing::NiceMock mock_check; + ::testing::NiceMock mock_report; + EXPECT_CALL(mock_check, GetSourceIpPort(_, _)).Times(1); + EXPECT_CALL(mock_report, GetResponseHeaders()).Times(1); + EXPECT_CALL(mock_report, GetReportInfo(_)).Times(1); // Report should be called. EXPECT_CALL(*mock_client_, Report(_)).Times(1); @@ -498,13 +476,15 @@ TEST_F(RequestHandlerImplTest, TestHandlerReport) { ApplyPerRouteConfig(config, &per_route); auto handler = controller_->CreateRequestHandler(per_route); - handler->Report(&mock_data); + handler->Report(&mock_check, &mock_report); } TEST_F(RequestHandlerImplTest, TestHandlerDisabledReport) { - ::testing::NiceMock mock_data; - EXPECT_CALL(mock_data, GetResponseHeaders()).Times(0); - EXPECT_CALL(mock_data, GetReportInfo(_)).Times(0); + ::testing::NiceMock mock_check; + ::testing::NiceMock mock_report; + EXPECT_CALL(mock_check, GetSourceIpPort(_, _)).Times(0); + EXPECT_CALL(mock_report, GetResponseHeaders()).Times(0); + EXPECT_CALL(mock_report, GetReportInfo(_)).Times(0); // Report should NOT be called. EXPECT_CALL(*mock_client_, Report(_)).Times(0); @@ -515,7 +495,7 @@ TEST_F(RequestHandlerImplTest, TestHandlerDisabledReport) { ApplyPerRouteConfig(config, &per_route); auto handler = controller_->CreateRequestHandler(per_route); - handler->Report(&mock_data); + handler->Report(&mock_check, &mock_report); } TEST_F(RequestHandlerImplTest, TestEmptyConfig) { @@ -552,7 +532,7 @@ TEST_F(RequestHandlerImplTest, TestEmptyConfig) { [](const CheckResponseInfo& info) { EXPECT_TRUE(info.response_status.ok()); }); - handler->Report(&mock_report); + handler->Report(&mock_check, &mock_report); } } // namespace http