Make gcs.json-key-file-path optional for Iceberg REST catalog#29101
Open
laserninja wants to merge 1 commit intotrinodb:masterfrom
Open
Make gcs.json-key-file-path optional for Iceberg REST catalog#29101laserninja wants to merge 1 commit intotrinodb:masterfrom
laserninja wants to merge 1 commit intotrinodb:masterfrom
Conversation
When using iceberg.rest-catalog.security=GOOGLE, the gcs.json-key-file-path property was validated as mandatory, preventing use of GKE Workload Identity or Application Default Credentials (ADC). The underlying GoogleAuthManager already supports ADC: when the credentials path is null/absent, it calls GoogleCredentials.getApplicationDefault(). Replace @NotNull with @nullable on getJsonKeyFilePath() and conditionally add the credentials property to the REST catalog auth map only when a path is provided. Fixes trinodb#29084
ebyhr
reviewed
Apr 14, 2026
| } | ||
|
|
||
| @NotNull | ||
| @Nullable |
Member
There was a problem hiding this comment.
Revert and return Optional type instead.
| .buildOrThrow(); | ||
| .put("header.x-goog-user-project", config.getProjectId()); | ||
| if (config.getJsonKeyFilePath() != null) { | ||
| builder.put(GCP_CREDENTIALS_PATH_PROPERTY, config.getJsonKeyFilePath()); |
Member
There was a problem hiding this comment.
Please update metastores.md to mention that gcs.json-key-file-path is optional.
| { | ||
| GoogleSecurityConfig config = new GoogleSecurityConfig() | ||
| .setProjectId("my-project"); | ||
| // jsonKeyFilePath intentionally omitted to use Workload Identity / ADC |
Member
There was a problem hiding this comment.
Move this comment above GoogleSecurityConfig config ...
|
|
||
| assertThat(properties).containsEntry(AUTH_TYPE, AUTH_TYPE_GOOGLE); | ||
| assertThat(properties).containsEntry("header.x-goog-user-project", "my-project"); | ||
| assertThat(properties).doesNotContainKey(GCP_CREDENTIALS_PATH_PROPERTY); |
Member
There was a problem hiding this comment.
No need to repeat assertThat:
Suggested change
| assertThat(properties).doesNotContainKey(GCP_CREDENTIALS_PATH_PROPERTY); | |
| assertThat(new GoogleAuthProperties(config).get()) | |
| .containsEntry(AUTH_TYPE, AUTH_TYPE_GOOGLE) | |
| .containsEntry("header.x-goog-user-project", "my-project") | |
| .doesNotContainKey(GCP_CREDENTIALS_PATH_PROPERTY); |
Same for testServiceAccountJsonKeyFile.
ebyhr
reviewed
Apr 14, 2026
| assertThat(properties).containsEntry("header.x-goog-user-project", "my-project"); | ||
| assertThat(properties).containsEntry(GCP_CREDENTIALS_PATH_PROPERTY, keyFile.toString()); | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
This test class doesn't currently provide much value. You can remove and update TestIcebergPlugin instead.
|
This pull request has gone a while without any activity. Ask for help on #core-dev on Trino slack. |
Contributor
Author
|
Waiting for #29098 |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Description
When using
iceberg.rest-catalog.security=GOOGLE, thegcs.json-key-file-pathproperty was validated as mandatory byGoogleSecurityConfig, causing the server to fail at startup when the property was omitted. This prevented use of GKE Workload Identity or environment-based Application Default Credentials (ADC), forcing users to provision and mount physical Service Account JSON key files — contrary to GCP security best practices.The underlying Iceberg library (
GoogleAuthManager) already handles a null/absent credentials path by falling through toGoogleCredentials.getApplicationDefault(). The fix:@NotNullwith@NullableongetJsonKeyFilePath()inGoogleSecurityConfig(retaining@FileExistsso the path is still validated when provided — consistent with the@Nullable @FileExistspattern used inGcsServiceAccountAuthConfig)GCP_CREDENTIALS_PATH_PROPERTYto the REST catalog auth properties map only when a key file path is providedTestGoogleAuthPropertiescovering both the ADC path and the explicit key file pathAdditional 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: