Skip to content

Add gcs.auth-type configuration #26681

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+135 −34
Open

Add gcs.auth-type configuration #26681

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions docs/src/main/sphinx/object-storage/file-system-gcs.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
Expand Down Expand Up @@ -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`
Expand Down
41 changes: 36 additions & 5 deletions lib/trino-filesystem-gcs/src/main/java/io/trino/filesystem/gcs/GcsFileSystemConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the authType is set after user-access-token? This will override the value without clear indication that this shouldn't be called. I think that this approach is not superior to the one I've already proposed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not how validation works.
It is run after all setters are run and we could model it in such way that you know that both setters were called - and that should result in failure.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the setUseGcsAccessToken should be optional in that case

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah - implementation wise changing useGcsAccessToken from boolean to Optional<Boolean> seems natural if we want to go this path.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In TestGcsFileSystemConfig#testDefaults we need use set method explicitly, do you have idea how to check those two property?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

testing defaults should be fine. you just assert on getters.
As for testing explicit setting is a bit more tricky. But recently we added assertFullMapping(Map<String, String> properties, T expected, Set<String> skipped) - so you can test scenarios when one setter is used, and then when the other is used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wendigo @losipiuk I'm not sure if this is okay, but please let me know if you have any advice on it

this.useGcsAccessToken = Optional.of(useGcsAccessToken);
if (useGcsAccessToken) {
this.authType = AuthType.ACCESS_TOKEN;
}
else {
this.authType = AuthType.DEFAULT;
}
return this;
}

Expand Down Expand Up @@ -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;
}

Expand Down
12 changes: 7 additions & 5 deletions lib/trino-filesystem-gcs/src/main/java/io/trino/filesystem/gcs/GcsFileSystemModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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);
}));
}
}
104 changes: 82 additions & 22 deletions lib/trino-filesystem-gcs/src/test/java/io/trino/filesystem/gcs/TestGcsFileSystemConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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")
Expand Down Expand Up @@ -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(
Expand Down
Loading