Merge pull request #24635 from markdroth/xds_channel_creds_registry

Add XdsChannelCredsRegistry

src/core/ext/xds/xds_bootstrap.cc

@@ -38,10 +38,48 @@
 
 namespace grpc_core {
 
+//
+// XdsChannelCredsRegistry
+//
+
+bool XdsChannelCredsRegistry::IsSupported(const std::string& creds_type) {
+  return creds_type == "google_default" || creds_type == "insecure" ||
+         creds_type == "fake";
+}
+
+bool XdsChannelCredsRegistry::IsValidConfig(const std::string& creds_type,
+                                            const Json& config) {
+  // Currently, none of the creds types actually take a config, but we
+  // ignore whatever might be specified in the bootstrap file for
+  // forward compatibility reasons.
+  return true;
+}
+
+RefCountedPtr<grpc_channel_credentials>
+XdsChannelCredsRegistry::MakeChannelCreds(const std::string& creds_type,
+                                          const Json& config) {
+  if (creds_type == "google_default") {
+    return grpc_google_default_credentials_create(nullptr);
+  } else if (creds_type == "insecure") {
+    return grpc_insecure_credentials_create();
+  } else if (creds_type == "fake") {
+    return grpc_fake_transport_security_credentials_create();
+  }
+  return nullptr;
+}
+
+//
+// XdsBootstrap::XdsServer
+//
+
 bool XdsBootstrap::XdsServer::ShouldUseV3() const {
   return server_features.find("xds_v3") != server_features.end();
 }
 
+//
+// XdsBootstrap
+//
+
 namespace {
 
 std::string BootstrapString(const XdsBootstrap& bootstrap) {
@@ -66,8 +104,8 @@ std::string BootstrapString(const XdsBootstrap& bootstrap) {
       "servers=[\n"
       "  {\n"
       "    uri=\"%s\",\n"
-      "    creds=<%s>,\n",
-      bootstrap.server().server_uri, bootstrap.server().channel_creds->type()));
+      "    creds_type=%s,\n",
+      bootstrap.server().server_uri, bootstrap.server().channel_creds_type));
   if (bootstrap.server().channel_creds_config.type() != Json::Type::JSON_NULL) {
     parts.push_back(
         absl::StrFormat("    creds_config=%s,",
@@ -244,7 +282,7 @@ grpc_error* XdsBootstrap::ParseChannelCredsArray(Json* json,
       if (parse_error != GRPC_ERROR_NONE) error_list.push_back(parse_error);
     }
   }
-  if (server->channel_creds == nullptr) {
+  if (server->channel_creds_type.empty()) {
     error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING(
         "no known creds type found in \"channel_creds\""));
   }
@@ -277,20 +315,15 @@ grpc_error* XdsBootstrap::ParseChannelCreds(Json* json, size_t idx,
     }
   }
   // Select the first channel creds type that we support.
-  if (server->channel_creds == nullptr) {
-    if (type == "google_default") {
-      server->channel_creds.reset(
-          grpc_google_default_credentials_create(nullptr));
-    } else if (type == "insecure") {
-      server->channel_creds.reset(grpc_insecure_credentials_create());
-    } else if (type == "fake") {
-      server->channel_creds.reset(
-          grpc_fake_transport_security_credentials_create());
-    }
-    if (server->channel_creds != nullptr) {
-      server->channel_creds_type = std::move(type);
-      server->channel_creds_config = std::move(config);
+  if (server->channel_creds_type.empty() &&
+      XdsChannelCredsRegistry::IsSupported(type)) {
+    if (!XdsChannelCredsRegistry::IsValidConfig(type, config)) {
+      error_list.push_back(GRPC_ERROR_CREATE_FROM_COPIED_STRING(
+          absl::StrCat("invalid config for channel creds type \"", type, "\"")
+              .c_str()));
     }
+    server->channel_creds_type = std::move(type);
+    server->channel_creds_config = std::move(config);
   }
   // Can't use GRPC_ERROR_CREATE_FROM_VECTOR() here, because the error
   // string is not static in this case.

src/core/ext/xds/xds_bootstrap.h

@@ -40,6 +40,14 @@ namespace grpc_core {
 
 class XdsClient;
 
+class XdsChannelCredsRegistry {
+ public:
+  static bool IsSupported(const std::string& creds_type);
+  static bool IsValidConfig(const std::string& creds_type, const Json& config);
+  static RefCountedPtr<grpc_channel_credentials> MakeChannelCreds(
+      const std::string& creds_type, const Json& config);
+};
+
 class XdsBootstrap {
  public:
   struct Node {
@@ -55,7 +63,6 @@ class XdsBootstrap {
     std::string server_uri;
     std::string channel_creds_type;
     Json channel_creds_config;
-    RefCountedPtr<grpc_channel_credentials> channel_creds;
     std::set<std::string> server_features;
 
     bool ShouldUseV3() const;

src/core/ext/xds/xds_client.cc

@@ -444,9 +444,13 @@ grpc_channel* CreateXdsChannel(const XdsBootstrap::XdsServer& server) {
   };
   grpc_channel_args* new_args = grpc_channel_args_copy_and_add(
       g_channel_args, args_to_add.data(), args_to_add.size());
+  // Create channel creds.
+  RefCountedPtr<grpc_channel_credentials> channel_creds =
+      XdsChannelCredsRegistry::MakeChannelCreds(server.channel_creds_type,
+                                                server.channel_creds_config);
   // Create channel.
   grpc_channel* channel = grpc_secure_channel_create(
-      server.channel_creds.get(), server.server_uri.c_str(), new_args, nullptr);
+      channel_creds.get(), server.server_uri.c_str(), new_args, nullptr);
   grpc_channel_args_destroy(new_args);
   return channel;
 }

src/core/lib/gprpp/ref_counted_ptr.h

@@ -39,9 +39,7 @@ class RefCountedPtr {
 
   // If value is non-null, we take ownership of a ref to it.
   template <typename Y>
-  explicit RefCountedPtr(Y* value) {
-    value_ = value;
-  }
+  RefCountedPtr(Y* value) : value_(value) {}
 
   // Move ctors.
   RefCountedPtr(RefCountedPtr&& other) noexcept {

test/core/xds/xds_bootstrap_test.cc

@@ -94,9 +94,6 @@ TEST_F(XdsBootstrapTest, Basic) {
   EXPECT_EQ(bootstrap.server().channel_creds_type, "fake");
   EXPECT_EQ(bootstrap.server().channel_creds_config.type(),
             Json::Type::JSON_NULL);
-  ASSERT_NE(bootstrap.server().channel_creds, nullptr);
-  EXPECT_STREQ(bootstrap.server().channel_creds->type(),
-               "FakeTransportSecurity");
   ASSERT_NE(bootstrap.node(), nullptr);
   EXPECT_EQ(bootstrap.node()->id, "foo");
   EXPECT_EQ(bootstrap.node()->cluster, "bar");
@@ -155,8 +152,6 @@ TEST_F(XdsBootstrapTest, InsecureCreds) {
   EXPECT_EQ(error, GRPC_ERROR_NONE) << grpc_error_string(error);
   EXPECT_EQ(bootstrap.server().server_uri, "fake:///lb");
   EXPECT_EQ(bootstrap.server().channel_creds_type, "insecure");
-  ASSERT_NE(bootstrap.server().channel_creds, nullptr);
-  EXPECT_STREQ(bootstrap.server().channel_creds->type(), "insecure");
   EXPECT_EQ(bootstrap.node(), nullptr);
 }
 
@@ -193,8 +188,6 @@ TEST_F(XdsBootstrapTest, GoogleDefaultCreds) {
   EXPECT_EQ(error, GRPC_ERROR_NONE) << grpc_error_string(error);
   EXPECT_EQ(bootstrap.server().server_uri, "fake:///lb");
   EXPECT_EQ(bootstrap.server().channel_creds_type, "google_default");
-  ASSERT_NE(bootstrap.server().channel_creds, nullptr);
-  EXPECT_STREQ(bootstrap.server().channel_creds->type(), "GoogleDefault");
   EXPECT_EQ(bootstrap.node(), nullptr);
 }