cleanup(GCS+gRPC)!: simplify plugin configuration (#8488)

coryan · web-flow · commit 1b2e1fae0043 · 2022-03-02T21:14:39.000-05:00
With the new backend for Google Direct Access (aka Direct Path) it is
easier to configure gRPC.  I also made it possible to configure the
plugin using `google::cloud::Options`.  Some of these changes are not
backwards compatible, but this is an experimental component.
diff --git a/google/cloud/storage/grpc_plugin.cc b/google/cloud/storage/grpc_plugin.cc
@@ -15,27 +15,19 @@
 #include "google/cloud/storage/grpc_plugin.h"
 #include "google/cloud/storage/internal/grpc_client.h"
 #include "google/cloud/storage/internal/hybrid_client.h"
-#include "google/cloud/internal/getenv.h"
 
 namespace google {
 namespace cloud {
 namespace storage_experimental {
 GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN
 
-namespace {
-absl::optional<std::string> GrpcConfig() {
-  return google::cloud::internal::GetEnv(
-      "GOOGLE_CLOUD_CPP_STORAGE_GRPC_CONFIG");
-}
-}  // namespace
-
 google::cloud::storage::Client DefaultGrpcClient(Options opts) {
   opts = google::cloud::storage::internal::DefaultOptionsGrpc(std::move(opts));
-  auto config = GrpcConfig();
-  if (config.value_or("none") == "none") {
+  auto config = opts.get<GrpcPluginOption>();
+  if (config == "none" || config.empty()) {
     return google::cloud::storage::Client(std::move(opts));
   }
-  if (config.value_or("") == "metadata") {
+  if (config == "metadata") {
     return storage::internal::ClientImplDetails::CreateClient(
         storage::internal::GrpcClient::Create(opts));
   }
diff --git a/google/cloud/storage/internal/grpc_client.cc b/google/cloud/storage/internal/grpc_client.cc
@@ -51,21 +51,26 @@ int DefaultGrpcNumChannels() {
 }
 
 Options DefaultOptionsGrpc(Options options) {
+  using ::google::cloud::internal::GetEnv;
+  using ::google::cloud::storage_experimental::GrpcPluginOption;
+
   options = DefaultOptionsWithCredentials(std::move(options));
   if (!options.has<UnifiedCredentialsOption>() &&
       !options.has<GrpcCredentialOption>()) {
     options.set<UnifiedCredentialsOption>(
         google::cloud::MakeGoogleDefaultCredentials());
   }
   if (!options.has<EndpointOption>()) {
-    options.set<EndpointOption>("storage.googleapis.com");
+    options.set<EndpointOption>(GetEnv("CLOUD_STORAGE_GRPC_ENDPOINT")
+                                    .value_or("storage.googleapis.com"));
   }
   if (!options.has<AuthorityOption>()) {
     options.set<AuthorityOption>("storage.googleapis.com");
   }
-  using google::cloud::internal::GetEnv;
-  auto env = GetEnv("CLOUD_STORAGE_GRPC_ENDPOINT");
-  if (env.has_value()) options.set<EndpointOption>(*env);
+  if (!options.has<GrpcPluginOption>()) {
+    options.set<GrpcPluginOption>(
+        GetEnv("GOOGLE_CLOUD_CPP_STORAGE_GRPC_CONFIG").value_or("none"));
+  }
   if (GetEnv("CLOUD_STORAGE_EMULATOR_ENDPOINT")) {
     // The emulator does not support HTTPS or authentication, use insecure
     // (sometimes called "anonymous") credentials, which disable SSL.
diff --git a/google/cloud/storage/internal/grpc_client_test.cc b/google/cloud/storage/internal/grpc_client_test.cc
@@ -13,9 +13,11 @@
 // limitations under the License.
 
 #include "google/cloud/storage/internal/grpc_client.h"
+#include "google/cloud/storage/grpc_plugin.h"
 #include "google/cloud/storage/testing/mock_storage_stub.h"
 #include "google/cloud/credentials.h"
 #include "google/cloud/options.h"
+#include "google/cloud/testing_util/scoped_environment.h"
 #include "google/cloud/testing_util/status_matchers.h"
 #include "google/cloud/testing_util/validate_metadata.h"
 #include <gmock/gmock.h>
@@ -29,6 +31,8 @@ namespace {
 
 namespace v2 = ::google::storage::v2;
 using ::google::cloud::internal::CurrentOptions;
+using ::google::cloud::storage_experimental::GrpcPluginOption;
+using ::google::cloud::testing_util::ScopedEnvironment;
 using ::google::cloud::testing_util::StatusIs;
 using ::google::cloud::testing_util::ValidateMetadataFixture;
 using ::testing::Pair;
@@ -59,6 +63,24 @@ std::shared_ptr<GrpcClient> CreateTestClient(
       Options{}.set<UnifiedCredentialsOption>(MakeInsecureCredentials()));
 }
 
+TEST_F(GrpcClientTest, DefaultOptionsGrpcNoEnv) {
+  auto grpc_config =
+      ScopedEnvironment("GOOGLE_CLOUD_CPP_STORAGE_GRPC_CONFIG", absl::nullopt);
+  auto opts = DefaultOptionsGrpc();
+  EXPECT_EQ(opts.get<GrpcPluginOption>(), "none");
+  opts = DefaultOptionsGrpc(Options{}.set<GrpcPluginOption>("configured"));
+  EXPECT_EQ(opts.get<GrpcPluginOption>(), "configured");
+}
+
+TEST_F(GrpcClientTest, DefaultOptionsGrpcWithEnv) {
+  auto grpc_config =
+      ScopedEnvironment("GOOGLE_CLOUD_CPP_STORAGE_GRPC_CONFIG", "env");
+  auto opts = DefaultOptionsGrpc();
+  EXPECT_EQ(opts.get<GrpcPluginOption>(), "env");
+  opts = DefaultOptionsGrpc(Options{}.set<GrpcPluginOption>("configured"));
+  EXPECT_EQ(opts.get<GrpcPluginOption>(), "configured");
+}
+
 TEST_F(GrpcClientTest, QueryResumableUpload) {
   auto mock = std::make_shared<testing::MockStorageStub>();
   EXPECT_CALL(*mock, QueryWriteStatus)
diff --git a/google/cloud/storage/internal/storage_stub_factory.cc b/google/cloud/storage/internal/storage_stub_factory.cc
@@ -31,49 +31,12 @@ namespace storage_internal {
 GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN
 namespace {
 
-auto constexpr kDirectPathConfig = R"json({
-    "loadBalancingConfig": [{
-      "grpclb": {
-        "childPolicy": [{
-          "pick_first": {}
-        }]
-      }
-    }]
-  })json";
-
 std::shared_ptr<grpc::Channel> CreateGrpcChannel(
     google::cloud::internal::GrpcAuthenticationStrategy& auth,
     Options const& options, int channel_id) {
   grpc::ChannelArguments args;
-  auto const& config = options.get<storage_experimental::GrpcPluginOption>();
-  if (config.empty() || config == "default" || config == "none") {
-    // Just configure for the regular path.
-    args.SetInt("grpc.channel_id", channel_id);
-    return auth.CreateChannel(options.get<EndpointOption>(), std::move(args));
-  }
-  std::set<absl::string_view> settings = absl::StrSplit(config, ',');
-  auto const dp = settings.count("dp") != 0 || settings.count("alts") != 0;
-  if (dp || settings.count("pick-first-lb") != 0) {
-    args.SetServiceConfigJSON(kDirectPathConfig);
-  }
-  if (dp || settings.count("enable-dns-srv-queries") != 0) {
-    args.SetInt("grpc.dns_enable_srv_queries", 1);
-  }
-  if (settings.count("disable-dns-srv-queries") != 0) {
-    args.SetInt("grpc.dns_enable_srv_queries", 0);
-  }
-  if (settings.count("exclusive") != 0) {
-    args.SetInt("grpc.channel_id", channel_id);
-  }
-  if (settings.count("alts") != 0) {
-    grpc::experimental::AltsCredentialsOptions alts_opts;
-    return grpc::CreateCustomChannel(
-        options.get<EndpointOption>(),
-        grpc::CompositeChannelCredentials(
-            grpc::experimental::AltsCredentials(alts_opts),
-            grpc::GoogleComputeEngineCredentials()),
-        std::move(args));
-  }
+  // Just configure for the regular path.
+  args.SetInt("grpc.channel_id", channel_id);
   return auth.CreateChannel(options.get<EndpointOption>(), std::move(args));
 }
 
