dimas-b commented on code in PR #2341:
URL: https://github.com/apache/polaris/pull/2341#discussion_r2292355303
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageIntegration.java:
##########
@@ -61,7 +62,8 @@ public abstract AccessConfig getSubscopedCreds(
@Nonnull RealmConfig realmConfig,
boolean allowListOperation,
@Nonnull Set<String> allowedReadLocations,
- @Nonnull Set<String> allowedWriteLocations);
+ @Nonnull Set<String> allowedWriteLocations,
+ Optional<String> refreshCredentialsEndpoint);
Review Comment:
Same here: please add javadoc
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/cache/StorageCredentialCache.java:
##########
@@ -130,7 +131,8 @@ public AccessConfig getOrGenerateSubScopeCreds(
polarisEntity.getType(),
k.allowedListAction(),
k.allowedReadLocations(),
- k.allowedWriteLocations());
+ k.allowedWriteLocations(),
+ refreshCredentialsEndpoint);
Review Comment:
Thx for adding it to the cache key! Would you mind accessing it here via
`k.` for clarity?
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogAdapter.java:
##########
@@ -430,16 +438,28 @@ public Response loadTable(
.loadTableIfStale(tableIdentifier, ifNoneMatch, snapshots)
.orElseThrow(() -> new
WebApplicationException(Response.Status.NOT_MODIFIED));
} else {
+ Optional<String> refreshCredentialsEndpoint =
+ getRefreshCredentialsEndpoint(delegationModes, prefix,
tableIdentifier);
response =
catalog
- .loadTableWithAccessDelegationIfStale(tableIdentifier,
ifNoneMatch, snapshots)
+ .loadTableWithAccessDelegationIfStale(
+ tableIdentifier, ifNoneMatch, snapshots,
refreshCredentialsEndpoint)
.orElseThrow(() -> new
WebApplicationException(Response.Status.NOT_MODIFIED));
}
return tryInsertETagHeader(Response.ok(response), response,
namespace, table).build();
});
}
+ private static Optional<String> getRefreshCredentialsEndpoint(
+ EnumSet<AccessDelegationMode> delegationModes,
+ String prefix,
+ TableIdentifier tableIdentifier) {
+ return delegationModes.contains(AccessDelegationMode.VENDED_CREDENTIALS)
Review Comment:
This logic is ok from my POV, but I wonder if other community members prefer
a feature flag to switch the new behaviour on/off.
Please send an email about this to the `dev` ML.
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisCredentialVendor.java:
##########
@@ -47,5 +48,6 @@ ScopedCredentialsResult getSubscopedCredsForEntity(
PolarisEntityType entityType,
boolean allowListOperation,
@Nonnull Set<String> allowedReadLocations,
- @Nonnull Set<String> allowedWriteLocations);
+ @Nonnull Set<String> allowedWriteLocations,
+ Optional<String> refreshCredentialsEndpoint);
Review Comment:
Please add javadoc for this property and mention that it may be a relative
URI.
--
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: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
