address comments.

include/istio/control/tcp/check_data.h

@@ -34,10 +34,6 @@ class CheckData {
   // If SSL is used, get peer or local certificate SAN URI.
   virtual bool GetPrincipal(bool peer, std::string* user) const = 0;
 
-  // Get source namespace from the principal.
-  virtual bool GetSourceNamespace(const std::string& principal,
-                                  std::string* ns) const = 0;
-
   // Returns true if connection is mutual TLS enabled.
   virtual bool IsMutualTLS() const = 0;
 

src/envoy/http/authn/http_filter_test.cc

@@ -72,7 +72,8 @@ std::unique_ptr<AuthenticatorBase> createAlwaysPassAuthenticator(
     _local(FilterContext *filter_context) : AuthenticatorBase(filter_context) {}
     bool run(Payload *) override {
       // Set some data to verify authentication result later.
-      auto payload = TestUtilities::CreateX509Payload("sa/foo/ns/test_ns/");
+      auto payload = TestUtilities::CreateX509Payload(
+          "cluster.local/sa/test_user/ns/test_ns/");
       filter_context()->setPeerResult(&payload);
       return true;
     }
@@ -189,13 +190,13 @@ TEST_F(AuthenticationFilterTest, AllPass) {
        fields {
          key: "source.principal"
          value {
-           string_value: "sa/foo/ns/test_ns/"
+           string_value: "cluster.local/sa/test_user/ns/test_ns/"
          }
        }
        fields {
          key: "source.user"
          value {
-           string_value: "sa/foo/ns/test_ns/"
+           string_value: "cluster.local/sa/test_user/ns/test_ns/"
          }
        })",
                                                     &expected_data));

src/envoy/tcp/mixer/filter.cc

@@ -154,11 +154,6 @@ bool Filter::GetPrincipal(bool peer, std::string* user) const {
   return Utils::GetPrincipal(&filter_callbacks_->connection(), peer, user);
 }
 
-bool Filter::GetSourceNamespace(const std::string& principal,
-                                std::string* ns) const {
-  return Utils::GetSourceNamespace(principal, ns);
-}
-
 bool Filter::IsMutualTLS() const {
   return Utils::IsMutualTLS(&filter_callbacks_->connection());
 }

src/envoy/tcp/mixer/filter.h

@@ -52,8 +52,6 @@ class Filter : public Network::Filter,
   // CheckData virtual functions.
   bool GetSourceIpPort(std::string* str_ip, int* port) const override;
   bool GetPrincipal(bool peer, std::string* user) const override;
-  bool GetSourceNamespace(const std::string& principal,
-                          std::string* ns) const override;
   bool IsMutualTLS() const override;
   bool GetRequestedServerName(std::string* name) const override;
 

src/envoy/utils/BUILD

@@ -36,6 +36,7 @@ envoy_cc_library(
         "//include/istio/utils:attribute_names_header",
         "//src/istio/authn:context_proto",
         "//src/istio/utils:attribute_names_lib",
+        "//src/istio/utils:utils_lib",
         ":filter_names_lib",
         "@envoy//source/exe:envoy_common_lib",
     ],

src/envoy/utils/authn.cc

@@ -17,8 +17,8 @@
 #include "common/common/base64.h"
 #include "include/istio/utils/attribute_names.h"
 #include "src/envoy/utils/filter_names.h"
-#include "src/envoy/utils/utils.h"
 #include "src/istio/authn/context.pb.h"
+#include "src/istio/utils/utils.h"
 
 using istio::authn::Result;
 
@@ -49,7 +49,7 @@ void Authentication::SaveAuthAttributesToStruct(
     setKeyValue(data, istio::utils::AttributeName::kSourcePrincipal,
                 result.peer_user());
     std::string source_ns("");
-    if (GetSourceNamespace(result.peer_user(), &source_ns)) {
+    if (istio::utils::GetSourceNamespace(result.peer_user(), &source_ns)) {
       setKeyValue(data, istio::utils::AttributeName::kSourceNamespace,
                   source_ns);
     }

src/envoy/utils/authn_test.cc

@@ -41,7 +41,7 @@ TEST_F(AuthenticationTest, SaveAuthAttributesToStruct) {
   EXPECT_TRUE(data.mutable_fields()->empty());
 
   result.set_principal("principal");
-  result.set_peer_user("sa/peeruser/ns/abc/");
+  result.set_peer_user("cluster.local/sa/peeruser/ns/abc/");
   auto origin = result.mutable_origin();
   origin->add_audiences("audiences0");
   origin->add_audiences("audiences1");
@@ -62,11 +62,11 @@ TEST_F(AuthenticationTest, SaveAuthAttributesToStruct) {
             "principal");
   EXPECT_EQ(
       data.fields().at(istio::utils::AttributeName::kSourceUser).string_value(),
-      "sa/peeruser/ns/abc/");
+      "cluster.local/sa/peeruser/ns/abc/");
   EXPECT_EQ(data.fields()
                 .at(istio::utils::AttributeName::kSourcePrincipal)
                 .string_value(),
-            "sa/peeruser/ns/abc/");
+            "cluster.local/sa/peeruser/ns/abc/");
   EXPECT_EQ(data.fields()
                 .at(istio::utils::AttributeName::kSourceNamespace)
                 .string_value(),

src/envoy/utils/utils.cc

@@ -33,8 +33,6 @@ const std::string kPerHostMetadataKey("istio");
 // Attribute field for per-host data override
 const std::string kMetadataDestinationUID("uid");
 
-const std::string kNamespacePrefix("ns/");
-
 }  // namespace
 
 void ExtractHeaders(const Http::HeaderMap& header_map,
@@ -120,25 +118,6 @@ bool GetPrincipal(const Network::Connection* connection, bool peer,
   return false;
 }
 
-bool GetSourceNamespace(const std::string& principal,
-                        std::string* source_namespace) {
-  if (source_namespace) {
-    // The namespace is a substring in principal with format: "ns/<NAMESPACE>/".
-    size_t begin = principal.find(kNamespacePrefix);
-    if (begin == std::string::npos) {
-      return false;
-    }
-    begin += kNamespacePrefix.length();
-    size_t end = principal.find("/", begin);
-    if (end == std::string::npos) {
-      return false;
-    }
-    *source_namespace = principal.substr(begin, end - begin);
-    return true;
-  }
-  return false;
-}
-
 bool IsMutualTLS(const Network::Connection* connection) {
   return connection != nullptr && connection->ssl() != nullptr &&
          connection->ssl()->peerCertificatePresented();

src/envoy/utils/utils.h

@@ -41,10 +41,6 @@ bool GetDestinationUID(const envoy::api::v2::core::Metadata& metadata,
 bool GetPrincipal(const Network::Connection* connection, bool peer,
                   std::string* principal);
 
-// Get source.namespace attribute from principal.
-bool GetSourceNamespace(const std::string& principal,
-                        std::string* source_namespace);
-
 // Returns true if connection is mutual TLS enabled.
 bool IsMutualTLS(const Network::Connection* connection);
 

src/envoy/utils/utils_test.cc

@@ -44,31 +44,4 @@ TEST(UtilsTest, ParseMessageWithUnknownField) {
   EXPECT_EQ(http_config.default_destination_service(),
             "service.svc.cluster.local");
 }
-
-TEST(UtilsTest, GetSourceNamespace) {
-  std::string ns = "";
-  EXPECT_EQ(false, Envoy::Utils::GetSourceNamespace("", &ns));
-  EXPECT_EQ("", ns);
-
-  EXPECT_EQ(false, Envoy::Utils::GetSourceNamespace("ns/abc", &ns));
-  EXPECT_EQ("", ns);
-
-  EXPECT_EQ(false, Envoy::Utils::GetSourceNamespace("ns/", &ns));
-  EXPECT_EQ("", ns);
-
-  EXPECT_EQ(false, Envoy::Utils::GetSourceNamespace("s/abc/", &ns));
-  EXPECT_EQ("", ns);
-
-  EXPECT_EQ(false, Envoy::Utils::GetSourceNamespace("NS/abc/", &ns));
-  EXPECT_EQ("", ns);
-
-  EXPECT_EQ(false, Envoy::Utils::GetSourceNamespace("abc", &ns));
-  EXPECT_EQ("", ns);
-
-  EXPECT_EQ(true, Envoy::Utils::GetSourceNamespace("ns/abc/", &ns));
-  EXPECT_EQ("abc", ns);
-
-  EXPECT_EQ(true, Envoy::Utils::GetSourceNamespace("ns//", &ns));
-  EXPECT_EQ("", ns);
-}
 }  // namespace

src/istio/control/tcp/BUILD

@@ -31,6 +31,7 @@ cc_library(
         "//include/istio/utils:attribute_names_header",
         "//src/istio/control:common_lib",
         "//src/istio/utils:attribute_names_lib",
+        "//src/istio/utils:utils_lib",
     ],
 )
 
@@ -45,6 +46,7 @@ cc_test(
     linkstatic = 1,
     deps = [
         ":control_lib",
+        "//src/istio/utils:utils_lib",
         "//external:googletest_main",
     ],
 )

src/istio/control/tcp/attributes_builder.cc

@@ -14,6 +14,7 @@
  */
 
 #include "src/istio/control/tcp/attributes_builder.h"
+#include "src/istio/utils/utils.h"
 
 #include "include/istio/utils/attribute_names.h"
 #include "include/istio/utils/attributes_builder.h"
@@ -50,7 +51,7 @@ void AttributesBuilder::ExtractCheckAttributes(CheckData* check_data) {
     builder.AddString(utils::AttributeName::kSourceUser, source_user);
     builder.AddString(utils::AttributeName::kSourcePrincipal, source_user);
     std::string source_ns("");
-    if (check_data->GetSourceNamespace(source_user, &source_ns)) {
+    if (utils::GetSourceNamespace(source_user, &source_ns)) {
       builder.AddString(utils::AttributeName::kSourceNamespace, source_ns);
     }
   }

src/istio/control/tcp/attributes_builder_test.cc

@@ -22,6 +22,7 @@
 #include "include/istio/utils/attributes_builder.h"
 #include "src/istio/control/tcp/mock_check_data.h"
 #include "src/istio/control/tcp/mock_report_data.h"
+#include "src/istio/utils/utils.h"
 
 using ::google::protobuf::TextFormat;
 using ::google::protobuf::util::MessageDifferencer;
@@ -82,13 +83,13 @@ attributes {
 attributes {
   key: "source.principal"
   value {
-    string_value: "sa/test_user/ns/test_ns/"
+    string_value: "cluster.local/sa/test_user/ns/test_ns/"
   }
 }
 attributes {
   key: "source.user"
   value {
-    string_value: "sa/test_user/ns/test_ns/"
+    string_value: "cluster.local/sa/test_user/ns/test_ns/"
   }
 }
 attributes {
@@ -378,7 +379,7 @@ TEST(AttributesBuilderTest, TestCheckAttributes) {
   EXPECT_CALL(mock_data, GetPrincipal(_, _))
       .WillRepeatedly(Invoke([](bool peer, std::string* user) -> bool {
         if (peer) {
-          *user = "sa/test_user/ns/test_ns/";
+          *user = "cluster.local/sa/test_user/ns/test_ns/";
         } else {
           *user = "destination_user";
         }
@@ -387,7 +388,7 @@ TEST(AttributesBuilderTest, TestCheckAttributes) {
   EXPECT_CALL(mock_data, GetSourceNamespace(_, _))
       .WillRepeatedly(
           Invoke([](const std::string& principal, std::string* ns) -> bool {
-            if (principal == "sa/test_user/ns/test_ns/") {
+            if (principal == "cluster.local/sa/test_user/ns/test_ns/") {
               *ns = "test_ns";
               return true;
             }

src/istio/utils/BUILD

@@ -19,6 +19,10 @@ cc_library(
     srcs = [
         "protobuf.cc",
         "status.cc",
+        "utils.cc"
+    ],
+    hdrs = [
+        "utils.h",
     ],
     visibility = ["//visibility:public"],
     deps = [
@@ -27,6 +31,16 @@ cc_library(
     ],
 )
 
+cc_test(
+    name = "utils_test",
+    size = "small",
+    srcs = ["utils_test.cc"],
+    deps = [
+        ":utils_lib",
+        "//external:googletest_main",
+    ],
+)
+
 cc_library(
     name = "md5_lib",
     srcs = ["md5.cc"],

src/istio/utils/utils.cc

@@ -0,0 +1,61 @@
+/* Copyright 2018 Istio Authors. All Rights Reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "src/istio/utils/utils.h"
+
+#include <sstream>
+#include <vector>
+
+namespace istio {
+namespace utils {
+
+namespace {
+const std::string kNamespaceKey("ns");
+const char kDelimiter = '/';
+}  // namespace
+
+bool GetSourceNamespace(const std::string& principal,
+                        std::string* source_namespace) {
+  if (source_namespace) {
+    // The namespace is a substring in principal with format:
+    // "<DOMAIN>/ns/<NAMESPACE>/sa/<SERVICE-ACCOUNT>". '/' is not allowed to
+    // appear in actual content except as delimiter between tokens.
+    // The following algorithm extracts the namespace based on the above format
+    // but is a little more flexible. It assumes that the principal begins with
+    // a <DOMAIN> string followed by <key,value> pairs separated by '/'.
+
+    // Spilt the principal into tokens separated by kDelimiter.
+    std::stringstream ss(principal);
+    std::vector<std::string> tokens;
+    std::string token;
+    while (std::getline(ss, token, kDelimiter)) {
+      tokens.push_back(token);
+    }
+
+    // Skip the first <DOMAIN> string and check remaining <key, value> pairs.
+    for (int i = 1; i < tokens.size(); i += 2) {
+      if (tokens[i] == kNamespaceKey) {
+        // Found the namespace key, treat the following token as namespace.
+        int j = i + 1;
+        *source_namespace = (j < tokens.size() ? tokens[j] : "");
+        return true;
+      }
+    }
+  }
+  return false;
+}
+
+}  // namespace utils
+}  // namespace istio

src/istio/utils/utils.h

@@ -0,0 +1,28 @@
+/* Copyright 2018 Istio Authors. All Rights Reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#pragma once
+
+#include <string>
+
+namespace istio {
+namespace utils {
+
+// Get source.namespace attribute from principal.
+bool GetSourceNamespace(const std::string& principal,
+                        std::string* source_namespace);
+
+}  // namespace utils
+}  // namespace istio