dimas-b commented on code in PR #2802:
URL: https://github.com/apache/polaris/pull/2802#discussion_r2449148539
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/cache/StorageCredentialCache.java:
##########
@@ -136,7 +137,8 @@ public AccessConfig getOrGenerateSubScopeCreds(
k.allowedListAction(),
k.allowedReadLocations(),
k.allowedWriteLocations(),
- k.refreshCredentialsEndpoint());
+ k.refreshCredentialsEndpoint(),
+ props);
Review Comment:
If `prop` is an argument to generating `AccessConfig` is should be part of
the cache key.
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java:
##########
@@ -75,11 +80,15 @@ public AccessConfig getSubscopedCreds(
boolean allowListOperation,
@Nonnull Set<String> allowedReadLocations,
@Nonnull Set<String> allowedWriteLocations,
- Optional<String> refreshCredentialsEndpoint) {
+ Optional<String> refreshCredentialsEndpoint,
+ Map props) {
+ LOGGER.info("Getting subscoped creds props: {}", props);
+ String kmsKey = props.get("s3.sse.key") != null ?
props.get("s3.sse.key").toString() : null;
Review Comment:
I personally think that "storage integration" code should work independently
of table properties. In other words Polaris should own storage integration and
`AccessConfig`. Table properties are settable by the end users outside the
context of a catalog (e.g. via registerTable). Mixing both sources of
integration config is confusing IMHO.
Is the intention here to support different KMS config per table?
Polaris may need to be able to get `AccessConfig` in order to load table
metadata (which has table properties)... Does this not create a chicken-and-egg
problem?
That said, I'm open to a wider discussion about this on the `dev` ML.
--
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]
