Forwarded attributes override statically configured Local Attributes

Makefile

@@ -20,6 +20,7 @@ ARTIFACTS_DIR ?= $(LOCAL_ARTIFACTS_DIR)
 BAZEL_STARTUP_ARGS ?=
 BAZEL_BUILD_ARGS ?=
 BAZEL_TEST_ARGS ?=
+BAZEL_TARGETS ?= //...
 HUB ?=
 TAG ?=
 ifeq "$(origin CC)" "default"
@@ -30,7 +31,7 @@ CXX := clang++-6.0
 endif
 
 build:
-	CC=$(CC) CXX=$(CXX) bazel $(BAZEL_STARTUP_ARGS) build $(BAZEL_BUILD_ARGS) //...
+	CC=$(CC) CXX=$(CXX) bazel $(BAZEL_STARTUP_ARGS) build $(BAZEL_BUILD_ARGS) $(BAZEL_TARGETS)
 	@bazel shutdown
 
 # Build only envoy - fast
@@ -43,15 +44,15 @@ clean:
 	@bazel shutdown
 
 test:
-	CC=$(CC) CXX=$(CXX) bazel $(BAZEL_STARTUP_ARGS) test $(BAZEL_TEST_ARGS) //...
+	CC=$(CC) CXX=$(CXX) bazel $(BAZEL_STARTUP_ARGS) test $(BAZEL_TEST_ARGS) $(BAZEL_TARGETS)
 	@bazel shutdown
 
 test_asan:
-	CC=$(CC) CXX=$(CXX) bazel $(BAZEL_STARTUP_ARGS) test $(BAZEL_TEST_ARGS) --config=clang-asan //...
+	CC=$(CC) CXX=$(CXX) bazel $(BAZEL_STARTUP_ARGS) test $(BAZEL_TEST_ARGS) --config=clang-asan $(BAZEL_TARGETS)
 	@bazel shutdown
 
 test_tsan:
-	CC=$(CC) CXX=$(CXX) bazel $(BAZEL_STARTUP_ARGS) test $(BAZEL_TEST_ARGS) --config=clang-tsan //...
+	CC=$(CC) CXX=$(CXX) bazel $(BAZEL_STARTUP_ARGS) test $(BAZEL_TEST_ARGS) --config=clang-tsan $(BAZEL_TARGETS)
 	@bazel shutdown
 
 check:

src/istio/control/http/request_handler_impl.cc

@@ -66,6 +66,7 @@ CancelFunc RequestHandlerImpl::Check(CheckData* check_data,
                                      CheckDoneFunc on_done) {
   // Forwarded attributes need to be stored regardless Check is needed
   // or not since the header will be updated or removed.
+  AddCheckAttributes(check_data);
   AddForwardAttributes(check_data);
   header_update->RemoveIstioAttributes();
   service_context_->InjectForwardedAttributes(header_update);
@@ -77,8 +78,6 @@ 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_test.cc

@@ -49,7 +49,7 @@ const char kLocalInbound[] = R"(
 attributes {
   key: "destination.uid"
   value {
-    string_value: "kubernetes://client-84469dc8d7-jbbxt.default"
+    string_value: "kubernetes://dest-client-84469dc8d7-jbbxt.default"
   }
 }
 )";
@@ -58,7 +58,7 @@ const char kLocalOutbound[] = R"(
 attributes {
   key: "source.uid"
   value {
-    string_value: "kubernetes://client-84469dc8d7-jbbxt.default"
+    string_value: "kubernetes://src-client-84469dc8d7-jbbxt.default"
   }
 }
 )";
@@ -219,9 +219,9 @@ TEST_F(RequestHandlerImplTest, TestHandlerDisabledCheckReport) {
 TEST_F(RequestHandlerImplTest, TestHandlerDisabledCheck) {
   ::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(0);
-  EXPECT_CALL(mock_data, GetPrincipal(_, _)).Times(0);
+  // Report is enabled so Check Attributes are extracted but not sent.
+  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);
@@ -543,6 +543,59 @@ TEST_F(RequestHandlerImplTest, TestEmptyConfig) {
   handler->Report(&mock_check, &mock_report);
 }
 
+TEST_F(RequestHandlerImplTest, TestLocalAttributes) {
+  ::testing::NiceMock<MockCheckData> mock_data;
+  ::testing::NiceMock<MockHeaderUpdate> mock_header;
+  // Check should be called.
+  EXPECT_CALL(*mock_client_, Check(_, _, _, _))
+      .WillOnce(Invoke([](const Attributes &attributes,
+                          const std::vector<Requirement> &quotas,
+                          TransportCheckFunc transport,
+                          CheckDoneFunc on_done) -> CancelFunc {
+        auto map = attributes.attributes();
+        EXPECT_EQ(map["destination.uid"].string_value(),
+                  "kubernetes://dest-client-84469dc8d7-jbbxt.default");
+        return nullptr;
+      }));
+
+  ServiceConfig config;
+  Controller::PerRouteConfig per_route;
+  ApplyPerRouteConfig(config, &per_route);
+  auto handler = controller_->CreateRequestHandler(per_route);
+  handler->Check(&mock_data, &mock_header, nullptr, nullptr);
+}
+
+TEST_F(RequestHandlerImplTest, TestLocalAttributesOverride) {
+  ::testing::NiceMock<MockCheckData> mock_data;
+  ::testing::NiceMock<MockHeaderUpdate> mock_header;
+
+  EXPECT_CALL(mock_data, ExtractIstioAttributes(_))
+      .WillOnce(Invoke([](std::string *data) -> bool {
+        Attributes fwd_attr;
+        (*fwd_attr.mutable_attributes())["destination.uid"].set_string_value(
+            "fwded");
+        fwd_attr.SerializeToString(data);
+        return true;
+      }));
+
+  // Check should be called.
+  EXPECT_CALL(*mock_client_, Check(_, _, _, _))
+      .WillOnce(Invoke([](const Attributes &attributes,
+                          const std::vector<Requirement> &quotas,
+                          TransportCheckFunc transport,
+                          CheckDoneFunc on_done) -> CancelFunc {
+        auto map = attributes.attributes();
+        EXPECT_EQ(map["destination.uid"].string_value(), "fwded");
+        return nullptr;
+      }));
+
+  ServiceConfig config;
+  Controller::PerRouteConfig per_route;
+  ApplyPerRouteConfig(config, &per_route);
+  auto handler = controller_->CreateRequestHandler(per_route);
+  handler->Check(&mock_data, &mock_header, nullptr, nullptr);
+}
+
 }  // namespace http
 }  // namespace control
 }  // namespace istio