Skip to content
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

[Improvement] generate credential according to the data path and metadata path #5648

Closed
FANNG1 opened this issue Nov 22, 2024 · 7 comments · Fixed by #5698
Closed

[Improvement] generate credential according to the data path and metadata path #5648

FANNG1 opened this issue Nov 22, 2024 · 7 comments · Fixed by #5698
Assignees
Labels
0.8.0 Release v0.8.0 improvement Improvements on everything

Comments

@FANNG1
Copy link
Contributor

FANNG1 commented Nov 22, 2024

What would you like to be improved?

Currently, Gravitino generates credential according to the table location, we should also consider write.data.path write.metadata.path in iceberg table properties. https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/TableProperties.java#L265-L273

How should we improve?

No response

@FANNG1 FANNG1 added the improvement Improvements on everything label Nov 22, 2024
@orenccl
Copy link
Collaborator

orenccl commented Nov 23, 2024

Hi, I’d like to work on this. Please let me know if it’s okay.

@FANNG1
Copy link
Contributor Author

FANNG1 commented Nov 24, 2024

@orenccl sure

@orenccl
Copy link
Collaborator

orenccl commented Nov 24, 2024

Currently, the only solution I can think of is to pass WRITE_DATA_LOCATION and WRITE_METADATA_LOCATION when injecting CredentialConfig.

CredentialUtils.vendCredential(
-             credentialProvider, loadTableResponse.tableMetadata().location());
+             credentialProvider,
+             new String[] {
+               loadTableResponse.tableMetadata().location(),
+               loadTableResponse.tableMetadata().property(TableProperties.WRITE_DATA_LOCATION, ""),
+               loadTableResponse.tableMetadata().property(TableProperties.WRITE_METADATA_LOCATION, "")
+             });
- public static Credential vendCredential(CredentialProvider credentialProvider, String path) {
+ public static Credential vendCredential(CredentialProvider credentialProvider, String[] path) {
    PathBasedCredentialContext pathBasedCredentialContext =
        new PathBasedCredentialContext(
-           PrincipalUtils.getCurrentUserName(), ImmutableSet.of(path), ImmutableSet.of());
+           PrincipalUtils.getCurrentUserName(), ImmutableSet.copyOf(path), ImmutableSet.of());
    return credentialProvider.getCredential(pathBasedCredentialContext);
}

However, I'm not sure what to do next—how to test it or what will be affected by these changes.

Could you give me some hints?

@FANNG1
Copy link
Contributor Author

FANNG1 commented Nov 25, 2024

Yes, it should be worked if you are passing the specific locations. Do you have a AWS or GCS account ? If yes, you could try to create a Iceberg table with specific metadata or data location, test whether could write read data by spark, please refer to https://gravitino.apache.org/docs/0.7.0-incubating/iceberg-rest-service#exploring-the-apache-gravitino-iceberg-rest-catalog-service-with-apache-spark. If no, I could do some test based your PR.

@orenccl
Copy link
Collaborator

orenccl commented Nov 25, 2024

I want to try testing it myself and will update you on the progress.

Thanks for your help!

@FANNG1
Copy link
Contributor Author

FANNG1 commented Nov 25, 2024

Please try to add a test in IcebergRESTS3IT to cover the feature, you could refer to testDML in IcebergRESTServiceIT

@FANNG1
Copy link
Contributor Author

FANNG1 commented Nov 25, 2024

I want to try testing it myself and will update you on the progress.

Thanks for your help!

please contact me if encounter any problems, credential vending is something complicated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.8.0 Release v0.8.0 improvement Improvements on everything
Projects
None yet
2 participants