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:

include/istio/utils/attribute_names.h

@@ -31,6 +31,11 @@ struct AttributeName {
   static const char kSourceUID[];
   static const char kDestinationPrincipal[];
 
+  static const char kDestinationServiceName[];
+  static const char kDestinationServiceUID[];
+  static const char kDestinationServiceHost[];
+  static const char kDestinationServiceNamespace[];
+
   static const char kRequestHeaders[];
   static const char kRequestHost[];
   static const char kRequestMethod[];

src/istio/control/http/attributes_builder.cc

@@ -130,11 +130,30 @@ void AttributesBuilder::ExtractForwardedAttributes(CheckData *check_data) {
   if (!check_data->ExtractIstioAttributes(&forwarded_data)) {
     return;
   }
+
   Attributes v2_format;
-  if (v2_format.ParseFromString(forwarded_data)) {
-    request_->attributes->MergeFrom(v2_format);
+  if (!v2_format.ParseFromString(forwarded_data)) {
     return;
   }
+
+  static const std::set<std::string> kForwardWhitelist = {
+      utils::AttributeName::kSourceUID,
+      utils::AttributeName::kDestinationServiceName,
+      utils::AttributeName::kDestinationServiceUID,
+      utils::AttributeName::kDestinationServiceHost,
+      utils::AttributeName::kDestinationServiceNamespace,
+  };
+
+  auto fwd = v2_format.attributes();
+  utils::AttributesBuilder builder(request_->attributes);
+  for (const auto &attribute : kForwardWhitelist) {
+    const auto &iter = fwd.find(attribute);
+    if (iter != fwd.end() && !iter->second.string_value().empty()) {
+      builder.AddString(attribute, iter->second.string_value());
+    }
+  }
+
+  return;
 }
 
 void AttributesBuilder::ExtractCheckAttributes(CheckData *check_data) {

src/istio/control/http/attributes_builder_test.cc

@@ -545,7 +545,7 @@ void SetDestinationIp(RequestContext *request, const std::string &ip) {
 
 TEST(AttributesBuilderTest, TestExtractForwardedAttributes) {
   Attributes attr;
-  (*attr.mutable_attributes())["test_key"].set_string_value("test_value");
+  (*attr.mutable_attributes())["source.uid"].set_string_value("test_value");
 
   ::testing::StrictMock<MockCheckData> mock_data;
   EXPECT_CALL(mock_data, ExtractIstioAttributes(_))

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"
   }
 }
 )";
@@ -128,6 +128,7 @@ forward_attributes {
 
 class RequestHandlerImplTest : public ::testing::Test {
  public:
+  RequestHandlerImplTest(bool outbound = false) : outbound_(outbound) {}
   void SetUp() { SetUpMockController(kDefaultClientConfig); }
 
   void SetUpMockController(const std::string &config_text) {
@@ -154,7 +155,7 @@ class RequestHandlerImplTest : public ::testing::Test {
 
     client_context_ = std::make_shared<ClientContext>(
         std::unique_ptr<MixerClient>(mock_client_), client_config_, 3, la,
-        false);
+        outbound_);
     controller_ =
         std::unique_ptr<Controller>(new ControllerImpl(client_context_));
   }
@@ -173,6 +174,14 @@ class RequestHandlerImplTest : public ::testing::Test {
   HttpClientConfig client_config_;
   ::testing::NiceMock<MockMixerClient> *mock_client_;
   std::unique_ptr<Controller> controller_;
+
+ private:
+  bool outbound_;
+};
+
+class OutboundRequestHandlerImplTest : public RequestHandlerImplTest {
+ public:
+  OutboundRequestHandlerImplTest() : RequestHandlerImplTest(true) {}
 };
 
 TEST_F(RequestHandlerImplTest, TestServiceConfigManage) {
@@ -219,9 +228,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 +552,62 @@ TEST_F(RequestHandlerImplTest, TestEmptyConfig) {
   handler->Report(&mock_check, &mock_report);
 }
 
+TEST_F(OutboundRequestHandlerImplTest, 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["source.uid"].string_value(),
+                  "kubernetes://src-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(OutboundRequestHandlerImplTest, 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())["source.uid"].set_string_value(
+            "fwded");
+        (*fwd_attr.mutable_attributes())["destination.uid"].set_string_value(
+            "ignored");
+        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["source.uid"].string_value(), "fwded");
+        EXPECT_NE(map["destination.uid"].string_value(), "ignored");
+        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

src/istio/utils/attribute_names.cc

@@ -25,6 +25,14 @@ const char AttributeName::kSourceNamespace[] = "source.namespace";
 const char AttributeName::kSourceUID[] = "source.uid";
 const char AttributeName::kDestinationPrincipal[] = "destination.principal";
 
+const char AttributeName::kDestinationServiceName[] =
+    "destination.service.name";
+const char AttributeName::kDestinationServiceUID[] = "destination.service.uid";
+const char AttributeName::kDestinationServiceHost[] =
+    "destination.service.host";
+const char AttributeName::kDestinationServiceNamespace[] =
+    "destination.service.namespace";
+
 const char AttributeName::kRequestHeaders[] = "request.headers";
 const char AttributeName::kRequestHost[] = "request.host";
 const char AttributeName::kRequestMethod[] = "request.method";