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 54bef4c4b28..40913a03f28 100644 --- a/src/envoy/http/mixer/filter.cc +++ b/src/envoy/http/mixer/filter.cc @@ -282,15 +282,15 @@ 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, check attributes are not extracted. + CheckData check_data(*request_headers, request_info.dynamicMetadata(), + decoder_callbacks_->connection()); // 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 9b9ca09d924..87144b4a8ec 100644 --- a/src/istio/control/http/request_handler_impl.cc +++ b/src/istio/control/http/request_handler_impl.cc @@ -29,15 +29,31 @@ 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_); @@ -48,7 +64,9 @@ CancelFunc RequestHandlerImpl::Check(CheckData* check_data, HeaderUpdate* header_update, TransportCheckFunc transport, CheckDoneFunc on_done) { - ExtractRequestAttributes(check_data); + // 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); @@ -59,6 +77,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, @@ -66,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 e858094a724..0e8c2b28072 100644 --- a/src/istio/control/http/request_handler_impl.h +++ b/src/istio/control/http/request_handler_impl.h @@ -37,16 +37,24 @@ 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 + 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..eb5700f3dbe 100644 --- a/src/istio/control/http/request_handler_impl_test.cc +++ b/src/istio/control/http/request_handler_impl_test.cc @@ -218,9 +218,9 @@ TEST_F(RequestHandlerImplTest, TestHandlerDisabledCheckReport) { TEST_F(RequestHandlerImplTest, TestHandlerDisabledCheck) { ::testing::NiceMock mock_data; ::testing::NiceMock mock_header; - // Report is enabled so Attributes are extracted. - EXPECT_CALL(mock_data, GetSourceIpPort(_, _)).Times(1); - EXPECT_CALL(mock_data, GetPrincipal(_, _)).Times(2); + // 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); @@ -462,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); @@ -474,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); @@ -491,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) { @@ -528,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