Add NO_CREDENTIALS authentication type for GCS
#27106
Open
+23
−11
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideThis PR explicitly adds a NO_CREDENTIALS authentication mode for GCS by splitting service-account logic from unauthenticated access, refactors credential handling in the service account implementation, tightens configuration validation for different auth types, and updates tests and documentation accordingly. Class diagram for new and updated GCS authentication classesclassDiagram
class GcsAuth {
<<interface>>
+setAuth(StorageOptions.Builder builder, ConnectorIdentity identity)
}
class GcsServiceAccountAuth {
-GoogleCredentials jsonGoogleCredential
+GcsServiceAccountAuth(GcsFileSystemConfig config)
+setAuth(StorageOptions.Builder builder, ConnectorIdentity identity)
}
class NoCredentialsAuth {
+setAuth(StorageOptions.Builder builder, ConnectorIdentity identity)
}
GcsServiceAccountAuth --|> GcsAuth
NoCredentialsAuth --|> GcsAuth
Class diagram for updated GcsFileSystemConfig AuthType enum and validationclassDiagram
class GcsFileSystemConfig {
+AuthType : enum (ACCESS_TOKEN, SERVICE_ACCOUNT, NO_CREDENTIALS)
+isServiceAccountAuthValid() : boolean
+isNonServiceAccountAuthValid() : boolean
}
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
chenjian2664
force-pushed
the
gcs-no-credentials-auth
branch
from
f1cee33 to
3e7dc29
Compare
|
@chenjian2664 Is this a release blocker of 478? |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The documentation refers to
NO_CREDENTIALS_AUTHbut the enum value isNO_CREDENTIALS—please align the docs with the actual enum name. - Add a unit test for NoCredentialsAuth to verify that it falls back to NoCredentials when ADC lookup throws an IOException.
- The GcsFileSystemConfig validation tests have a lot of repetitive assertFailsValidation calls—consider parameterizing these cases to reduce duplication.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The documentation refers to `NO_CREDENTIALS_AUTH` but the enum value is `NO_CREDENTIALS`—please align the docs with the actual enum name.
- Add a unit test for NoCredentialsAuth to verify that it falls back to NoCredentials when ADC lookup throws an IOException.
- The GcsFileSystemConfig validation tests have a lot of repetitive assertFailsValidation calls—consider parameterizing these cases to reduce duplication.
## Individual Comments
### Comment 1
<location> `lib/trino-filesystem-gcs/src/test/java/io/trino/filesystem/gcs/TestGcsFileSystemConfig.java:136-142` </location>
<code_context>
assertFailsValidation(
new GcsFileSystemConfig()
.setAuthType(AuthType.ACCESS_TOKEN)
- .setJsonKey("{}}"),
- "authMethodValid",
- "Either gcs.auth-type or gcs.json-key or gcs.json-key-file-path must be set",
+ .setJsonKey("{}"),
+ "nonServiceAuthValid",
+ "Can't set gcs.json-key or gcs.json-key-file-path",
+ AssertTrue.class);
+
+ assertFailsValidation(
</code_context>
<issue_to_address>
**suggestion (testing):** Good coverage of validation for NO_CREDENTIALS, but missing positive test cases.
Please add tests that verify valid NO_CREDENTIALS configurations to ensure positive scenarios are covered.
</issue_to_address>
### Comment 2
<location> `lib/trino-filesystem-gcs/src/test/java/io/trino/filesystem/gcs/TestGcsStorageFactory.java:35-37` </location>
<code_context>
GcsFileSystemConfig config = new GcsFileSystemConfig();
- GcsStorageFactory storageFactory = new GcsStorageFactory(config, new GcsServiceAccountAuth(config));
+ GcsStorageFactory storageFactory = new GcsStorageFactory(config, new NoCredentialsAuth());
Credentials actualCredentials;
try (Storage storage = storageFactory.create(ConnectorIdentity.ofUser("test"))) {
</code_context>
<issue_to_address>
**suggestion (testing):** Test for NoCredentialsAuth only covers default config; edge cases are missing.
Add tests for cases where application default credentials are unavailable to confirm NoCredentialsAuth correctly acts as a fallback.
Suggested implementation:
```java
// No credentials options are set
GcsFileSystemConfig config = new GcsFileSystemConfig();
GcsStorageFactory storageFactory = new GcsStorageFactory(config, new NoCredentialsAuth());
Credentials actualCredentials;
try (Storage storage = storageFactory.create(ConnectorIdentity.ofUser("test"))) {
}
@Test
public void testNoCredentialsAuthFallbackWhenNoADC() throws Exception
{
// Simulate environment with no ADC
String originalGoogleApplicationCredentials = System.getenv("GOOGLE_APPLICATION_CREDENTIALS");
try {
// Unset the ADC environment variable
if (originalGoogleApplicationCredentials != null) {
System.clearProperty("GOOGLE_APPLICATION_CREDENTIALS");
}
GcsFileSystemConfig config = new GcsFileSystemConfig();
GcsStorageFactory storageFactory = new GcsStorageFactory(config, new NoCredentialsAuth());
try (Storage storage = storageFactory.create(ConnectorIdentity.ofUser("test"))) {
Credentials credentials = storage.getOptions().getCredentials();
// Should be null or a NoCredentials instance, depending on implementation
assertTrue(credentials == null || credentials.getClass().getSimpleName().equals("NoCredentials"), "NoCredentialsAuth should fallback to no credentials");
}
}
finally {
// Restore ADC environment variable
if (originalGoogleApplicationCredentials != null) {
System.setProperty("GOOGLE_APPLICATION_CREDENTIALS", originalGoogleApplicationCredentials);
}
}
}
```
- If your codebase uses a different way to unset or mock environment variables, adjust the test setup accordingly.
- If `NoCredentialsAuth` produces a specific credentials type, update the assertion to check for that type.
- Ensure the test method is annotated with `@Test` and imported from your test framework (e.g., JUnit).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
ebyhr
reviewed
Oct 27, 2025
chenjian2664
force-pushed
the
gcs-no-credentials-auth
branch
2 times, most recently
from
0e0a502 to
1c8d57a
Compare
|
CI failure is related to this change: |
chenjian2664
force-pushed
the
gcs-no-credentials-auth
branch
from
1c8d57a to
eeee061
Compare
Please replace NO_CREDENTIALS_AUTH with NO_CREDENTIALS in the commit title. |
chenjian2664
force-pushed
the
gcs-no-credentials-auth
branch
from
eeee061 to
f91c5bb
Compare
Praveen2112
reviewed
Oct 27, 2025
Comment on lines
81
to
83
| * `NO_CREDENTIALS`: allows accessing Google Cloud Storage without providing | ||
| explicit credentials. Attempts to acquire application default credentials from the | ||
| environment, and falls back to no-credentials access if none are available. |
There was a problem hiding this comment.
Shouldn't this be behaviour by default ? Why do we need to implement them as a dedicated enum or configure them out ?
There was a problem hiding this comment.
Previously, the NO_CREDENTIALS behavior was implicitly covered by the SERVICE_ACCOUNT auth type, but in practice it was never reachable because the SERVICE_ACCOUNT validator requires either a json-key or json-key-file-path to be configured. By introducing a dedicated NO_CREDENTIALS type, we can now support this scenario explicitly and ensure the validation logic stays consistent, context see #26984
There was a problem hiding this comment.
I meant if we didn't set the auth type to any value - it should exhibit this behaviour right ?
There was a problem hiding this comment.
Previously the default is SERVICE_ACCOUNT, so I don't want to change it in this pr - just want to introduce a new type.
WDYT @wendigo
chenjian2664
force-pushed
the
gcs-no-credentials-auth
branch
from
f91c5bb to
e72eae8
Compare
szisiu
reviewed
Oct 29, 2025
lib/trino-filesystem-gcs/src/main/java/io/trino/filesystem/gcs/NoCredentialsAuth.java
Outdated
Show resolved
Hide resolved
martint
reviewed
Oct 30, 2025
chenjian2664
force-pushed
the
gcs-no-credentials-auth
branch
from
e72eae8 to
5a088ac
Compare
chenjian2664
force-pushed
the
gcs-no-credentials-auth
branch
from
5a088ac to
afe4615
Compare
szisiu
reviewed
Oct 30, 2025
Comment on lines
-64
to
-65
| // This is consistent with the GCP SDK when no credentials are available in the environment | ||
| return null; |
chenjian2664
force-pushed
the
gcs-no-credentials-auth
branch
from
58dc340 to
390f406
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add a dedicated
NO_CREDENTIALStype to explicitly support unauthenticated access.Description
Fixes #26984
Additional context and related issues
Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(x) Release notes are required, with the following suggested text: