diff --git a/docs/src/main/sphinx/object-storage/file-system-gcs.md b/docs/src/main/sphinx/object-storage/file-system-gcs.md index 3bde8515b782..49df7ca4a86f 100644 --- a/docs/src/main/sphinx/object-storage/file-system-gcs.md +++ b/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 `SERVICE_ACCOUNT`. + Supported values are: + * `SERVICE_ACCOUNT`: 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` diff --git a/lib/trino-filesystem-gcs/src/main/java/io/trino/filesystem/gcs/GcsFileSystemConfig.java b/lib/trino-filesystem-gcs/src/main/java/io/trino/filesystem/gcs/GcsFileSystemConfig.java index 3e5020675655..1f94873a389e 100644 --- a/lib/trino-filesystem-gcs/src/main/java/io/trino/filesystem/gcs/GcsFileSystemConfig.java +++ b/lib/trino-filesystem-gcs/src/main/java/io/trino/filesystem/gcs/GcsFileSystemConfig.java @@ -21,6 +21,7 @@ import io.airlift.units.Duration; import io.airlift.units.MinDuration; import jakarta.annotation.Nullable; +import jakarta.validation.constraints.AssertFalse; import jakarta.validation.constraints.AssertTrue; import jakarta.validation.constraints.Min; import jakarta.validation.constraints.NotNull; @@ -33,6 +34,12 @@ public class GcsFileSystemConfig { + public enum AuthType + { + ACCESS_TOKEN, + SERVICE_ACCOUNT; + } + 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 endpoint = Optional.empty(); - private boolean useGcsAccessToken; + private Optional useGcsAccessToken = Optional.empty(); + private Optional authType = Optional.empty(); private String jsonKey; private String jsonKeyFilePath; private int maxRetries = 20; @@ -134,15 +142,33 @@ public GcsFileSystemConfig setEndpoint(Optional endpoint) return this; } + @NotNull + public AuthType getAuthType() + { + if (useGcsAccessToken.isPresent() && useGcsAccessToken.get()) { + return AuthType.ACCESS_TOKEN; + } + return authType.orElse(AuthType.SERVICE_ACCOUNT); + } + + @Config("gcs.auth-type") + public GcsFileSystemConfig setAuthType(AuthType authType) + { + this.authType = Optional.of(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; + this.useGcsAccessToken = Optional.of(useGcsAccessToken); return this; } @@ -268,13 +294,19 @@ 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 (getAuthType() == AuthType.ACCESS_TOKEN) { return jsonKey == null && jsonKeyFilePath == null; } return (jsonKey == null) ^ (jsonKeyFilePath == null); } + + @AssertFalse(message = "Cannot set both gcs.use-access-token and gcs.auth-type") + public boolean isAuthTypeAndGcsAccessTokenConfigured() + { + return authType.isPresent() && useGcsAccessToken.isPresent(); + } } diff --git a/lib/trino-filesystem-gcs/src/main/java/io/trino/filesystem/gcs/GcsFileSystemModule.java b/lib/trino-filesystem-gcs/src/main/java/io/trino/filesystem/gcs/GcsFileSystemModule.java index e3eb7a469595..9e32abb19eed 100644 --- a/lib/trino-filesystem-gcs/src/main/java/io/trino/filesystem/gcs/GcsFileSystemModule.java +++ b/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 SERVICE_ACCOUNT -> _ -> binder.bind(GcsAuth.class).to(GcsServiceAccountAuth.class).in(Scopes.SINGLETON); + })); } } diff --git a/lib/trino-filesystem-gcs/src/main/java/io/trino/filesystem/gcs/GcsDefaultAuth.java b/lib/trino-filesystem-gcs/src/main/java/io/trino/filesystem/gcs/GcsServiceAccountAuth.java similarity index 96% rename from lib/trino-filesystem-gcs/src/main/java/io/trino/filesystem/gcs/GcsDefaultAuth.java rename to lib/trino-filesystem-gcs/src/main/java/io/trino/filesystem/gcs/GcsServiceAccountAuth.java index d5935b437508..a30ddacf0942 100644 --- a/lib/trino-filesystem-gcs/src/main/java/io/trino/filesystem/gcs/GcsDefaultAuth.java +++ b/lib/trino-filesystem-gcs/src/main/java/io/trino/filesystem/gcs/GcsServiceAccountAuth.java @@ -26,13 +26,13 @@ import static java.nio.charset.StandardCharsets.UTF_8; -public class GcsDefaultAuth +public class GcsServiceAccountAuth implements GcsAuth { private final Optional jsonGoogleCredential; @Inject - public GcsDefaultAuth(GcsFileSystemConfig config) + public GcsServiceAccountAuth(GcsFileSystemConfig config) throws IOException { String jsonKey = config.getJsonKey(); diff --git a/lib/trino-filesystem-gcs/src/test/java/io/trino/filesystem/gcs/AbstractTestGcsFileSystem.java b/lib/trino-filesystem-gcs/src/test/java/io/trino/filesystem/gcs/AbstractTestGcsFileSystem.java index 6e54f1ed7c55..6c5c1d93baa8 100644 --- a/lib/trino-filesystem-gcs/src/test/java/io/trino/filesystem/gcs/AbstractTestGcsFileSystem.java +++ b/lib/trino-filesystem-gcs/src/test/java/io/trino/filesystem/gcs/AbstractTestGcsFileSystem.java @@ -56,7 +56,7 @@ protected void initialize(String gcpCredentialKey) // For gcp testing this corresponds to the Cluster Storage Admin and Cluster Storage Object Admin roles byte[] jsonKeyBytes = Base64.getDecoder().decode(gcpCredentialKey); GcsFileSystemConfig config = new GcsFileSystemConfig().setJsonKey(new String(jsonKeyBytes, UTF_8)); - GcsStorageFactory storageFactory = new GcsStorageFactory(config, new GcsDefaultAuth(config)); + GcsStorageFactory storageFactory = new GcsStorageFactory(config, new GcsServiceAccountAuth(config)); this.gcsFileSystemFactory = new GcsFileSystemFactory(config, storageFactory); this.storage = storageFactory.create(ConnectorIdentity.ofUser("test")); String bucket = RemoteStorageHelper.generateBucketName(); diff --git a/lib/trino-filesystem-gcs/src/test/java/io/trino/filesystem/gcs/TestGcsFileSystemConfig.java b/lib/trino-filesystem-gcs/src/test/java/io/trino/filesystem/gcs/TestGcsFileSystemConfig.java index 05a2259fb77e..179bce9cb78c 100644 --- a/lib/trino-filesystem-gcs/src/test/java/io/trino/filesystem/gcs/TestGcsFileSystemConfig.java +++ b/lib/trino-filesystem-gcs/src/test/java/io/trino/filesystem/gcs/TestGcsFileSystemConfig.java @@ -16,6 +16,8 @@ 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.AssertFalse; import jakarta.validation.constraints.AssertTrue; import org.junit.jupiter.api.Test; @@ -41,9 +43,10 @@ void testDefaults() .setWriteBlockSize(DataSize.of(16, MEGABYTE)) .setPageSize(100) .setBatchSize(100) + .setUseGcsAccessToken(false) .setProjectId(null) .setEndpoint(Optional.empty()) - .setUseGcsAccessToken(false) + .setAuthType(AuthType.SERVICE_ACCOUNT) .setJsonKey(null) .setJsonKeyFilePath(null) .setMaxRetries(20) @@ -56,6 +59,43 @@ void testDefaults() @Test void testExplicitPropertyMappings() + { + Map properties = ImmutableMap.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 properties = ImmutableMap.builder() .put("gcs.read-block-size", "51MB") @@ -87,7 +127,7 @@ 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 @@ -95,18 +135,18 @@ 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( @@ -114,7 +154,7 @@ public void testValidation() .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( @@ -125,5 +165,37 @@ public void testValidation() "retryDelayValid", "gcs.client.min-backoff-delay must be less than or equal to gcs.client.max-backoff-delay", AssertTrue.class); + + assertFailsValidation( + new GcsFileSystemConfig() + .setAuthType(AuthType.ACCESS_TOKEN) + .setUseGcsAccessToken(true), + "authTypeAndGcsAccessTokenConfigured", + "Cannot set both gcs.use-access-token and gcs.auth-type", + AssertFalse.class); + + assertFailsValidation( + new GcsFileSystemConfig() + .setAuthType(AuthType.ACCESS_TOKEN) + .setUseGcsAccessToken(false), + "authTypeAndGcsAccessTokenConfigured", + "Cannot set both gcs.use-access-token and gcs.auth-type", + AssertFalse.class); + + assertFailsValidation( + new GcsFileSystemConfig() + .setUseGcsAccessToken(true) + .setAuthType(AuthType.SERVICE_ACCOUNT), + "authTypeAndGcsAccessTokenConfigured", + "Cannot set both gcs.use-access-token and gcs.auth-type", + AssertFalse.class); + + assertFailsValidation( + new GcsFileSystemConfig() + .setUseGcsAccessToken(false) + .setAuthType(AuthType.SERVICE_ACCOUNT), + "authTypeAndGcsAccessTokenConfigured", + "Cannot set both gcs.use-access-token and gcs.auth-type", + AssertFalse.class); } } diff --git a/lib/trino-filesystem-gcs/src/test/java/io/trino/filesystem/gcs/TestGcsStorageFactory.java b/lib/trino-filesystem-gcs/src/test/java/io/trino/filesystem/gcs/TestGcsStorageFactory.java index 2edc89fb6b04..89247ee7334e 100644 --- a/lib/trino-filesystem-gcs/src/test/java/io/trino/filesystem/gcs/TestGcsStorageFactory.java +++ b/lib/trino-filesystem-gcs/src/test/java/io/trino/filesystem/gcs/TestGcsStorageFactory.java @@ -32,7 +32,7 @@ void testDefaultCredentials() // No credentials options are set GcsFileSystemConfig config = new GcsFileSystemConfig(); - GcsStorageFactory storageFactory = new GcsStorageFactory(config, new GcsDefaultAuth(config)); + GcsStorageFactory storageFactory = new GcsStorageFactory(config, new GcsServiceAccountAuth(config)); Credentials actualCredentials; try (Storage storage = storageFactory.create(ConnectorIdentity.ofUser("test"))) {