Add gcs.auth-type configuration

This makes it easier to add new GCS authentication types
in the future. The commit also deprecate `gcs.use-access-token`

Author: chenjian2664

docs/src/main/sphinx/object-storage/file-system-gcs.md

@@ -70,7 +70,15 @@ Cloud Storage:
   - Description
 * - `gcs.use-access-token`
   - Flag to set usage of a client-provided OAuth 2.0 token to access Google
-    Cloud Storage. Defaults to `false`.
+    Cloud Storage. Defaults to `false`, deprecated to use `gcs.auth-type` instead.
+* - `gcs.auth-type`
+  - Authentication type to use for Google Cloud Storage access. Default to `DEFAULT`.
+  Supported values are:
+    * `DEFAULT`: loads credentials from the environment. Either `gcs.json-key` or
+      `gcs.json-key-file-path` can be set in addition to override the default
+      credentials provider.
+    * `ACCESS_TOKEN`: usage of client-provided OAuth 2.0 token to access Google
+      Cloud Storage.
 * - `gcs.json-key`
   - Your Google Cloud service account key in JSON format. Not to be set together
     with `gcs.json-key-file-path`.
@@ -103,7 +111,7 @@ Cloud Storage, make the following edits to your catalog configuration:
      - Native property
      - Notes
    * - `hive.gcs.use-access-token`
-     - `gcs.use-access-token`
+     - `gcs.auth-type`
      -
    * - `hive.gcs.json-key-file-path`
      - `gcs.json-key-file-path`

lib/trino-filesystem-gcs/src/main/java/io/trino/filesystem/gcs/GcsFileSystemConfig.java

@@ -29,10 +29,17 @@
 import java.util.Optional;
 import java.util.concurrent.TimeUnit;
 
+import static com.google.common.base.Preconditions.checkArgument;
 import static io.airlift.units.DataSize.Unit.MEGABYTE;
 
 public class GcsFileSystemConfig
 {
+    public enum AuthType
+    {
+        ACCESS_TOKEN,
+        DEFAULT;
+    }
+
     private DataSize readBlockSize = DataSize.of(2, MEGABYTE);
     private DataSize writeBlockSize = DataSize.of(16, MEGABYTE);
     private int pageSize = 100;
@@ -41,7 +48,8 @@ public class GcsFileSystemConfig
     private String projectId;
     private Optional<String> endpoint = Optional.empty();
 
-    private boolean useGcsAccessToken;
+    private Optional<Boolean> useGcsAccessToken = Optional.empty();
+    private AuthType authType;
     private String jsonKey;
     private String jsonKeyFilePath;
     private int maxRetries = 20;
@@ -134,15 +142,38 @@ public GcsFileSystemConfig setEndpoint(Optional<String> endpoint)
         return this;
     }
 
+    @NotNull
+    public AuthType getAuthType()
+    {
+        return Optional.ofNullable(authType).orElse(AuthType.DEFAULT);
+    }
+
+    @Config("gcs.auth-type")
+    public GcsFileSystemConfig setAuthType(AuthType authType)
+    {
+        checkArgument(useGcsAccessToken.isEmpty(), "Cannot set both gcs.use-access-token and gcs.auth-type");
+        this.authType = authType;
+        return this;
+    }
+
+    @Deprecated
     public boolean isUseGcsAccessToken()
     {
-        return useGcsAccessToken;
+        return useGcsAccessToken.orElse(false);
     }
 
+    @Deprecated
     @Config("gcs.use-access-token")
     public GcsFileSystemConfig setUseGcsAccessToken(boolean useGcsAccessToken)
     {
-        this.useGcsAccessToken = useGcsAccessToken;
+        checkArgument(authType == null, "Cannot set both gcs.use-access-token and gcs.auth-type");
+        this.useGcsAccessToken = Optional.of(useGcsAccessToken);
+        if (useGcsAccessToken) {
+            this.authType = AuthType.ACCESS_TOKEN;
+        }
+        else {
+            this.authType = AuthType.DEFAULT;
+        }
         return this;
     }
 
@@ -268,10 +299,10 @@ public boolean isRetryDelayValid()
         return minBackoffDelay.compareTo(maxBackoffDelay) <= 0;
     }
 
-    @AssertTrue(message = "Either gcs.use-access-token or gcs.json-key or gcs.json-key-file-path must be set")
+    @AssertTrue(message = "Either gcs.auth-type or gcs.json-key or gcs.json-key-file-path must be set")
     public boolean isAuthMethodValid()
     {
-        if (useGcsAccessToken) {
+        if (authType == AuthType.ACCESS_TOKEN) {
             return jsonKey == null && jsonKeyFilePath == null;
         }
 

lib/trino-filesystem-gcs/src/main/java/io/trino/filesystem/gcs/GcsFileSystemModule.java

@@ -17,8 +17,8 @@
 import com.google.inject.Scopes;
 import io.airlift.configuration.AbstractConfigurationAwareModule;
 
-import static io.airlift.configuration.ConditionalModule.conditionalModule;
 import static io.airlift.configuration.ConfigBinder.configBinder;
+import static io.airlift.configuration.SwitchModule.switchModule;
 
 public class GcsFileSystemModule
         extends AbstractConfigurationAwareModule
@@ -30,10 +30,12 @@ protected void setup(Binder binder)
         binder.bind(GcsStorageFactory.class).in(Scopes.SINGLETON);
         binder.bind(GcsFileSystemFactory.class).in(Scopes.SINGLETON);
 
-        install(conditionalModule(
+        install(switchModule(
                 GcsFileSystemConfig.class,
-                GcsFileSystemConfig::isUseGcsAccessToken,
-                _ -> binder.bind(GcsAuth.class).to(GcsAccessTokenAuth.class).in(Scopes.SINGLETON),
-                _ -> binder.bind(GcsAuth.class).to(GcsDefaultAuth.class).in(Scopes.SINGLETON)));
+                GcsFileSystemConfig::getAuthType,
+                type -> switch (type) {
+                    case ACCESS_TOKEN -> _ -> binder.bind(GcsAuth.class).to(GcsAccessTokenAuth.class).in(Scopes.SINGLETON);
+                    case DEFAULT -> _ -> binder.bind(GcsAuth.class).to(GcsDefaultAuth.class).in(Scopes.SINGLETON);
+                }));
     }
 }

lib/trino-filesystem-gcs/src/test/java/io/trino/filesystem/gcs/TestGcsFileSystemConfig.java

@@ -16,6 +16,7 @@
 import com.google.common.collect.ImmutableMap;
 import io.airlift.units.DataSize;
 import io.airlift.units.Duration;
+import io.trino.filesystem.gcs.GcsFileSystemConfig.AuthType;
 import jakarta.validation.constraints.AssertTrue;
 import org.junit.jupiter.api.Test;
 
@@ -30,32 +31,74 @@
 import static io.airlift.units.DataSize.Unit.MEGABYTE;
 import static java.util.concurrent.TimeUnit.MILLISECONDS;
 import static java.util.concurrent.TimeUnit.SECONDS;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
 public class TestGcsFileSystemConfig
 {
     @Test
     void testDefaults()
     {
-        assertRecordedDefaults(recordDefaults(GcsFileSystemConfig.class)
-                .setReadBlockSize(DataSize.of(2, MEGABYTE))
-                .setWriteBlockSize(DataSize.of(16, MEGABYTE))
-                .setPageSize(100)
-                .setBatchSize(100)
-                .setProjectId(null)
-                .setEndpoint(Optional.empty())
-                .setUseGcsAccessToken(false)
-                .setJsonKey(null)
-                .setJsonKeyFilePath(null)
-                .setMaxRetries(20)
-                .setBackoffScaleFactor(3.0)
-                .setMaxRetryTime(new Duration(25, SECONDS))
-                .setMinBackoffDelay(new Duration(10, MILLISECONDS))
-                .setMaxBackoffDelay(new Duration(2000, MILLISECONDS))
-                .setApplicationId("Trino"));
+        assertThatThrownBy(
+                () -> assertRecordedDefaults(recordDefaults(GcsFileSystemConfig.class)
+                                .setReadBlockSize(DataSize.of(2, MEGABYTE))
+                                .setWriteBlockSize(DataSize.of(16, MEGABYTE))
+                                .setPageSize(100)
+                                .setBatchSize(100)
+                                .setProjectId(null)
+                                .setEndpoint(Optional.empty())
+                                .setAuthType(null)
+                                .setJsonKey(null)
+                                .setJsonKeyFilePath(null)
+                                .setMaxRetries(20)
+                                .setBackoffScaleFactor(3.0)
+                                .setMaxRetryTime(new Duration(25, SECONDS))
+                                .setMinBackoffDelay(new Duration(10, MILLISECONDS))
+                                .setMaxBackoffDelay(new Duration(2000, MILLISECONDS))
+                                .setApplicationId("Trino")))
+                .isInstanceOf(AssertionError.class)
+                // use-access-token is not deprecated and isn't allowed to be set with auth-type
+                .hasMessage("Untested attributes: [UseGcsAccessToken]");
     }
 
     @Test
     void testExplicitPropertyMappings()
+    {
+        Map<String, String> properties = ImmutableMap.<String, String>builder()
+                .put("gcs.read-block-size", "51MB")
+                .put("gcs.write-block-size", "52MB")
+                .put("gcs.page-size", "10")
+                .put("gcs.batch-size", "11")
+                .put("gcs.project-id", "project")
+                .put("gcs.endpoint", "http://custom.dns.org:8000")
+                .put("gcs.auth-type", "access_token")
+                .put("gcs.client.max-retries", "10")
+                .put("gcs.client.backoff-scale-factor", "4.0")
+                .put("gcs.client.max-retry-time", "10s")
+                .put("gcs.client.min-backoff-delay", "20ms")
+                .put("gcs.client.max-backoff-delay", "20ms")
+                .put("gcs.application-id", "application id")
+                .buildOrThrow();
+
+        GcsFileSystemConfig expected = new GcsFileSystemConfig()
+                .setReadBlockSize(DataSize.of(51, MEGABYTE))
+                .setWriteBlockSize(DataSize.of(52, MEGABYTE))
+                .setPageSize(10)
+                .setBatchSize(11)
+                .setProjectId("project")
+                .setEndpoint(Optional.of("http://custom.dns.org:8000"))
+                .setAuthType(AuthType.ACCESS_TOKEN)
+                .setMaxRetries(10)
+                .setBackoffScaleFactor(4.0)
+                .setMaxRetryTime(new Duration(10, SECONDS))
+                .setMinBackoffDelay(new Duration(20, MILLISECONDS))
+                .setMaxBackoffDelay(new Duration(20, MILLISECONDS))
+                .setApplicationId("application id");
+        assertFullMapping(properties, expected, Set.of("gcs.json-key", "gcs.json-key-file-path", "gcs.use-access-token"));
+    }
+
+    // backwards compatibility test, remove if use-access-token is removed
+    @Test
+    void testExplicitPropertyMappingsWithDeprecatedUseAccessToken()
     {
         Map<String, String> properties = ImmutableMap.<String, String>builder()
                 .put("gcs.read-block-size", "51MB")
@@ -87,34 +130,51 @@ void testExplicitPropertyMappings()
                 .setMinBackoffDelay(new Duration(20, MILLISECONDS))
                 .setMaxBackoffDelay(new Duration(20, MILLISECONDS))
                 .setApplicationId("application id");
-        assertFullMapping(properties, expected, Set.of("gcs.json-key", "gcs.json-key-file-path"));
+        assertFullMapping(properties, expected, Set.of("gcs.json-key", "gcs.json-key-file-path", "gcs.auth-type"));
+    }
+
+    @Test
+    void testSetUseGcsAccessTokenWithAuthType()
+    {
+        assertThatThrownBy(() -> new GcsFileSystemConfig().setAuthType(AuthType.ACCESS_TOKEN).setUseGcsAccessToken(true))
+                .isInstanceOf(IllegalArgumentException.class)
+                .hasMessage("Cannot set both gcs.use-access-token and gcs.auth-type");
+        assertThatThrownBy(() -> new GcsFileSystemConfig().setAuthType(AuthType.DEFAULT).setUseGcsAccessToken(false))
+                .isInstanceOf(IllegalArgumentException.class)
+                .hasMessage("Cannot set both gcs.use-access-token and gcs.auth-type");
+        assertThatThrownBy(() -> new GcsFileSystemConfig().setUseGcsAccessToken(true).setAuthType(AuthType.ACCESS_TOKEN))
+                .isInstanceOf(IllegalArgumentException.class)
+                .hasMessage("Cannot set both gcs.use-access-token and gcs.auth-type");
+        assertThatThrownBy(() -> new GcsFileSystemConfig().setUseGcsAccessToken(true).setAuthType(AuthType.DEFAULT))
+                .isInstanceOf(IllegalArgumentException.class)
+                .hasMessage("Cannot set both gcs.use-access-token and gcs.auth-type");
     }
 
     @Test
     public void testValidation()
     {
         assertFailsValidation(
                 new GcsFileSystemConfig()
-                        .setUseGcsAccessToken(true)
+                        .setAuthType(AuthType.ACCESS_TOKEN)
                         .setJsonKey("{}}"),
                 "authMethodValid",
-                "Either gcs.use-access-token or gcs.json-key or gcs.json-key-file-path must be set",
+                "Either gcs.auth-type or gcs.json-key or gcs.json-key-file-path must be set",
                 AssertTrue.class);
 
         assertFailsValidation(
                 new GcsFileSystemConfig()
-                        .setUseGcsAccessToken(true)
+                        .setAuthType(AuthType.ACCESS_TOKEN)
                         .setJsonKeyFilePath("/dev/null"),
                 "authMethodValid",
-                "Either gcs.use-access-token or gcs.json-key or gcs.json-key-file-path must be set",
+                "Either gcs.auth-type or gcs.json-key or gcs.json-key-file-path must be set",
                 AssertTrue.class);
 
         assertFailsValidation(
                 new GcsFileSystemConfig()
                         .setJsonKey("{}")
                         .setJsonKeyFilePath("/dev/null"),
                 "authMethodValid",
-                "Either gcs.use-access-token or gcs.json-key or gcs.json-key-file-path must be set",
+                "Either gcs.auth-type or gcs.json-key or gcs.json-key-file-path must be set",
                 AssertTrue.class);
 
         assertFailsValidation(