Not to add check attributes if check is disabled

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_);

src/istio/control/http/request_handler_impl.cc

@@ -29,26 +29,47 @@ namespace http {
 
 RequestHandlerImpl::RequestHandlerImpl(
     std::shared_ptr<ServiceContext> 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,

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<ServiceContext> service_context_;
+
+  // If true, request attributes are added
+  bool check_attributes_added_;
+  // If true, forward attributes are added
+  bool forward_attributes_added_;
 };
 
 }  // namespace http

src/istio/control/http/request_handler_impl_test.cc

@@ -218,7 +218,29 @@ TEST_F(RequestHandlerImplTest, TestHandlerDisabledCheckReport) {
 TEST_F(RequestHandlerImplTest, TestHandlerDisabledCheck) {
   ::testing::NiceMock<MockCheckData> mock_data;
   ::testing::NiceMock<MockHeaderUpdate> 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<MockCheckData> mock_data;
+  ::testing::NiceMock<MockHeaderUpdate> 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) {