From 4e57b84b2701b8fd7a01200e434722c77de1054b Mon Sep 17 00:00:00 2001 From: chenjian2664 Date: Mon, 22 Sep 2025 14:14:28 +0800 Subject: [PATCH 1/2] 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` --- .../sphinx/object-storage/file-system-gcs.md | 12 +- .../filesystem/gcs/GcsFileSystemConfig.java | 41 ++++++- .../filesystem/gcs/GcsFileSystemModule.java | 12 +- .../gcs/TestGcsFileSystemConfig.java | 104 ++++++++++++++---- 4 files changed, 135 insertions(+), 34 deletions(-) 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..ac49d252746b 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 `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` 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..7e9a9c2a16eb 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 @@ -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 endpoint = Optional.empty(); - private boolean useGcsAccessToken; + private Optional useGcsAccessToken = Optional.empty(); + private AuthType authType; private String jsonKey; private String jsonKeyFilePath; private int maxRetries = 20; @@ -134,15 +142,38 @@ public GcsFileSystemConfig setEndpoint(Optional 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; } 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..9d665df83a15 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 DEFAULT -> _ -> binder.bind(GcsAuth.class).to(GcsDefaultAuth.class).in(Scopes.SINGLETON); + })); } } 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..24f65bad6b8f 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,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 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 +130,24 @@ 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 @@ -95,18 +155,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 +174,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( From ffa804624d31b52b62c050c263e0c51ae4efb10c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Osipiuk?= Date: Fri, 26 Sep 2025 18:33:28 +0200 Subject: [PATCH 2/2] blah --- .../filesystem/gcs/GcsFileSystemConfig.java | 33 +++++++----- .../gcs/TestGcsFileSystemConfig.java | 53 ++++++++++++------- 2 files changed, 53 insertions(+), 33 deletions(-) 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 7e9a9c2a16eb..10f15757854e 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 @@ -26,10 +26,10 @@ import jakarta.validation.constraints.NotNull; import jakarta.validation.constraints.Size; +import java.util.Arrays; 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 @@ -49,7 +49,7 @@ public enum AuthType private Optional endpoint = Optional.empty(); private Optional useGcsAccessToken = Optional.empty(); - private AuthType authType; + private Optional authType = Optional.empty(); private String jsonKey; private String jsonKeyFilePath; private int maxRetries = 20; @@ -145,38 +145,43 @@ public GcsFileSystemConfig setEndpoint(Optional endpoint) @NotNull public AuthType getAuthType() { - return Optional.ofNullable(authType).orElse(AuthType.DEFAULT); + if (useGcsAccessToken.isPresent() && useGcsAccessToken.get()) { + return AuthType.ACCESS_TOKEN; + } + return 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; + this.authType = Optional.of(authType); return this; } @Deprecated public boolean isUseGcsAccessToken() { - return useGcsAccessToken.orElse(false); + // should not be called; allow calling from TestGcsFileSystemConfig as it is needed by airlift config test framework + if (Arrays.stream(Thread.currentThread().getStackTrace()).anyMatch(element -> element.getClassName().equals("io.trino.filesystem.gcs.TestGcsFileSystemConfig"))) { + return useGcsAccessToken.orElse(false); + } + throw new RuntimeException("isUseGcsAccessToken must not be called; use getAuthType instead"); } @Deprecated @Config("gcs.use-access-token") public GcsFileSystemConfig setUseGcsAccessToken(boolean 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; } + @AssertTrue(message = "Cannot set both gcs.use-access-token and gcs.auth-type") + public boolean isAuthTypeAndUseGcsAccessTokenMutuallyExclusive() + { + return !(authType.isPresent() && useGcsAccessToken.isPresent()); + } + @Nullable public String getJsonKey() { @@ -302,7 +307,7 @@ public boolean isRetryDelayValid() @AssertTrue(message = "Either gcs.auth-type or gcs.json-key or gcs.json-key-file-path must be set") public boolean isAuthMethodValid() { - if (authType == AuthType.ACCESS_TOKEN) { + if (getAuthType() == AuthType.ACCESS_TOKEN) { return jsonKey == null && jsonKeyFilePath == null; } 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 24f65bad6b8f..b4befcf87776 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 @@ -46,7 +46,7 @@ void testDefaults() .setBatchSize(100) .setProjectId(null) .setEndpoint(Optional.empty()) - .setAuthType(null) + .setAuthType(AuthType.DEFAULT) .setJsonKey(null) .setJsonKeyFilePath(null) .setMaxRetries(20) @@ -56,7 +56,7 @@ void testDefaults() .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 + // use-access-token is now deprecated and isn't allowed to be set with auth-type .hasMessage("Untested attributes: [UseGcsAccessToken]"); } @@ -133,23 +133,6 @@ void testExplicitPropertyMappingsWithDeprecatedUseAccessToken() 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() { @@ -185,5 +168,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), + "authTypeAndUseGcsAccessTokenMutuallyExclusive", + "Cannot set both gcs.use-access-token and gcs.auth-type", + AssertTrue.class); + + assertFailsValidation( + new GcsFileSystemConfig() + .setAuthType(AuthType.ACCESS_TOKEN) + .setUseGcsAccessToken(false), + "authTypeAndUseGcsAccessTokenMutuallyExclusive", + "Cannot set both gcs.use-access-token and gcs.auth-type", + AssertTrue.class); + + assertFailsValidation( + new GcsFileSystemConfig() + .setAuthType(AuthType.DEFAULT) + .setUseGcsAccessToken(true), + "authTypeAndUseGcsAccessTokenMutuallyExclusive", + "Cannot set both gcs.use-access-token and gcs.auth-type", + AssertTrue.class); + + assertFailsValidation( + new GcsFileSystemConfig() + .setAuthType(AuthType.DEFAULT) + .setUseGcsAccessToken(false), + "authTypeAndUseGcsAccessTokenMutuallyExclusive", + "Cannot set both gcs.use-access-token and gcs.auth-type", + AssertTrue.class); } }