-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Iceberg Azure credential vending #26921
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.
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
base: master
Are you sure you want to change the base?
Iceberg Azure credential vending #26921
Conversation
|
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Sreesh Maheshwar.
|
Reviewer's GuideThis PR extends Iceberg REST catalog support to vend Azure SAS token credentials alongside existing S3 credential vending by unifying the credential selection logic, propagating vended credentials through the Azure filesystem, and introducing a new AzureVendedAuth implementation. Class diagram for updated Azure authentication classesclassDiagram
class AzureAuth {
<<interface>>
+void setAuth(String storageAccount, BlobContainerClientBuilder builder)
+void setAuth(String storageAccount, DataLakeServiceClientBuilder builder)
}
class AzureAuthAccessKey
class AzureAuthDefault
class AzureAuthOauth
class AzureVendedAuth {
+Map<String, String> accountSasTokens
+AzureAuth fallbackAuth
+AzureVendedAuth(Map<String, String> accountSasTokens, AzureAuth fallbackAuth)
+void setAuth(String storageAccount, BlobContainerClientBuilder builder)
+void setAuth(String storageAccount, DataLakeServiceClientBuilder builder)
}
AzureAuth <|.. AzureAuthAccessKey
AzureAuth <|.. AzureAuthDefault
AzureAuth <|.. AzureAuthOauth
AzureAuth <|.. AzureVendedAuth
AzureVendedAuth --> AzureAuth : fallbackAuth
Class diagram for AzureFileSystemFactory authentication selectionclassDiagram
class AzureFileSystemFactory {
+TrinoFileSystem create(ConnectorIdentity identity)
-static AzureAuth withVendedAuth(ConnectorIdentity identity, AzureAuth defaultAuth)
}
class AzureAuth
class AzureVendedAuth
AzureFileSystemFactory --> AzureAuth
AzureFileSystemFactory --> AzureVendedAuth
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Sreesh Maheshwar.
|
|
|
||
| public sealed interface AzureAuth | ||
| permits AzureAuthAccessKey, AzureAuthDefault, AzureAuthOauth | ||
| permits AzureAuthAccessKey, AzureAuthDefault, AzureAuthOauth, AzureVendedAuth |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Introducing this new auth means that with this PR,
trino/lib/trino-filesystem-azure/src/main/java/io/trino/filesystem/azure/AzureFileSystem.java
Lines 482 to 486 in d20c663
| return switch (azureAuth) { | |
| case AzureAuthOauth _ -> oauth2PresignedUri(location, ttl, Optional.empty()); | |
| case AzureAuthAccessKey _ -> accessKeyPresignedUri(location, ttl, Optional.empty()); | |
| default -> throw new UnsupportedOperationException("Unsupported azure auth: " + azureAuth); | |
| }; |
throws.
Maybe this is fine for now, as I see it's not supported even for default auth?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also highlighting that I'm not adding this type to
trino/lib/trino-filesystem-azure/src/main/java/io/trino/filesystem/azure/AzureFileSystemConfig.java
Lines 27 to 32 in d20c663
| public enum AuthType | |
| { | |
| ACCESS_KEY, | |
| OAUTH, | |
| DEFAULT, | |
| } |
I think that this should be an internal auth type that cannot enabled by config. Though I appreciate this design of an internal, IRC-centred auth may be controversial.
| @Override | ||
| public void setAuth(String storageAccount, BlobContainerClientBuilder builder) | ||
| { | ||
| getSasToken(storageAccount) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting sasToken with fallback in these flows aims to mirror https://github.com/apache/iceberg/blob/acd3e4657d2f4e6bfe4d092fa88dd362bb50fedc/azure/src/main/java/org/apache/iceberg/azure/AzureProperties.java#L160-L181
ebyhr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a test.
|
@ebyhr yes, sounds good! (Currently, still a WIP and completely untested) While I have you here, may you elaborate on what sort of tests you'd like for this PR? I see |
|
Ping on the above @ebyhr 🙏 |
|
This can be very useful for us. How can I help pushing this? |
|
@smaheshwar-pltr Hi, since you’re adding credential vending support for the Iceberg REST catalog, the test can refer to the |
44d5a9b to
8c08a0d
Compare
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
1 similar comment
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
74f3e34 to
f56f64c
Compare
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
|
Thanks, @chenjian2664. I feel like we should be able to wire up the REST fixture with the Azure storage used in CI to get something similar to public IcebergAzureRestCatalogBackendContainer(
Optional<Network> network,
String warehouseLocation,
String accountName,
String domainName,
String sasToken)
{
super(
"tabulario/iceberg-rest:1.6.0",
"iceberg-rest",
ImmutableSet.of(8181),
ImmutableMap.of(),
toCatalogEnvVars(ImmutableMap.of(
"include-credentials", "true",
"warehouse", warehouseLocation,
"io-impl", "org.apache.iceberg.azure.adlsv2.ADLSFileIO",
"adls.sas-token." + accountName + "." + domainName, sasToken)),
network,
5);
}Can dig into this, though may be slow due to maybe having to iterate with CI. |
(Update from me is that I did get around to smoke testing this PR with a remote catalog and it works) Edit: I only tested the read path, from trying to wire up integration tests in #27427, I realised that the write path doesn't work because of the HNS check done with a SAS token. |
Description
// Work in progress
Support Azure credential vending with Iceberg REST catalog.
Additional context and related issues
Closes #23238
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.
( ) Release notes are required, with the following suggested text:
Summary by Sourcery
Enable Azure credential vending by extracting SAS tokens from file IO properties for Iceberg REST catalog and injecting them into the Azure filesystem via a new AzureVendedAuth wrapper.
New Features: