sunyuhan1998 commented on code in PR #10895:
URL: https://github.com/apache/gravitino/pull/10895#discussion_r3256846236
##########
lance/lance-common/src/main/java/org/apache/gravitino/lance/common/ops/gravitino/GravitinoLanceTableOperations.java:
##########
@@ -131,10 +159,61 @@ public DescribeTableResponse describeTable(
Optional.ofNullable(table.properties().get(LANCE_TABLE_VERSION))
.map(Long::valueOf)
.orElse(null));
-
response.setStorageOptions(LancePropertiesUtils.getLanceStorageOptions(table.properties()));
+
+ if (credentialPrivilege != null) {
+ response.setStorageOptions(
+ buildVendedStorageOptions(catalogName, catalog, table,
credentialPrivilege));
+ } else {
+
response.setStorageOptions(LancePropertiesUtils.getLanceStorageOptions(table.properties()));
+ }
+
return response;
}
+ private Map<String, String> buildVendedStorageOptions(
+ String catalogName, Catalog catalog, Table table, CredentialPrivilege
credentialPrivilege) {
Review Comment:
Understood. We'll refactor to move credential generation to the Gravitino
server side. Here's the overall approach:
1. `GenericCatalogOperations` implements `SupportsPathBasedCredentials` to
provide storage paths for Lance tables, following the
`FilesetCatalogOperations` pattern.
2. `GenericCatalog` overrides `propertiesWithCredentialProviders()` to
auto-detect credential providers, following the `IcebergCatalog` pattern.
3. Lance server obtains credentials via the Gravitino credential API,
converts to Lance `storage_options`, and removes local credential generation.
4. `vendCredentials` defaults to `false`.
One question: our current implementation generates **table-level
(path-scoped)** credentials — each credential is scoped to the specific table's
storage path with READ/WRITE distinction via `PathBasedCredentialContext`. To
preserve this granularity on the server side, we'd need TABLE-level credential
REST API support (adding `TABLE` to the supported types). Alternatively, we
could use the existing **catalog-level** credential API
(`catalog.supportsCredentials().getCredentials()`) which is simpler but
generates broader credentials without per-table path scoping.
@roryqi @FANNG1 Which granularity do you prefer — catalog-level (simpler) or
table-level (more secure, requires extending the credential API)?
##########
lance/lance-common/src/main/java/org/apache/gravitino/lance/common/ops/gravitino/GravitinoLanceTableOperations.java:
##########
@@ -131,10 +159,61 @@ public DescribeTableResponse describeTable(
Optional.ofNullable(table.properties().get(LANCE_TABLE_VERSION))
.map(Long::valueOf)
.orElse(null));
-
response.setStorageOptions(LancePropertiesUtils.getLanceStorageOptions(table.properties()));
+
+ if (credentialPrivilege != null) {
+ response.setStorageOptions(
+ buildVendedStorageOptions(catalogName, catalog, table,
credentialPrivilege));
+ } else {
+
response.setStorageOptions(LancePropertiesUtils.getLanceStorageOptions(table.properties()));
+ }
+
return response;
}
+ private Map<String, String> buildVendedStorageOptions(
+ String catalogName, Catalog catalog, Table table, CredentialPrivilege
credentialPrivilege) {
Review Comment:
(See the detailed reply on @roryqi 's comment above for the full plan)
We agree with the direction. One open question is whether to use
catalog-level or table-level credential granularity — see above for details.
Let us know your thoughts.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
