Copilot commented on code in PR #9839:
URL: https://github.com/apache/gravitino/pull/9839#discussion_r2751268966
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/CatalogWrapperForREST.java:
##########
@@ -238,6 +239,36 @@ private Credential getCredential(
return credential;
}
+ private boolean shouldGenerateCredential(
+ LoadTableResponse loadTableResponse, boolean requestCredential) {
+ if (!requestCredential) {
+ return false;
+ }
+ return !isLocalOrHdfsTable(loadTableResponse.tableMetadata());
+ }
+
+ private boolean isLocalOrHdfsTable(TableMetadata tableMetadata) {
+ return isLocalOrHdfsLocation(tableMetadata.location());
Review Comment:
`shouldGenerateCredential` only checks `tableMetadata.location()` to decide
whether to skip credential vending, but `getCredential()` builds the credential
context from multiple locations (`location`,
`TableProperties.WRITE_DATA_LOCATION`,
`TableProperties.WRITE_METADATA_LOCATION`). If either write location is set to
a non-local scheme (e.g., S3) while `location` is local/HDFS, this method will
incorrectly skip vending and clients will miss required credentials. Consider
determining “local/HDFS table” by checking all relevant table locations (at
least the same ones used in `getCredential()`) and only skipping vending when
*all* of them are local/HDFS.
```suggestion
String[] locations =
Stream.of(
tableMetadata.location(),
tableMetadata.property(TableProperties.WRITE_DATA_LOCATION,
""),
tableMetadata.property(TableProperties.WRITE_METADATA_LOCATION, ""))
.filter(StringUtils::isNotBlank)
.toArray(String[]::new);
// If no non-blank locations are found, fall back to checking the base
location only.
if (locations.length == 0) {
return isLocalOrHdfsLocation(tableMetadata.location());
}
for (String location : locations) {
if (!isLocalOrHdfsLocation(location)) {
return false;
}
}
return true;
```
--
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]
