dimas-b commented on code in PR #3224:
URL: https://github.com/apache/polaris/pull/3224#discussion_r2594521615
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/io/StorageAccessConfigProvider.java:
##########
@@ -49,13 +50,16 @@ public class StorageAccessConfigProvider {
private final StorageCredentialCache storageCredentialCache;
private final StorageCredentialsVendor storageCredentialsVendor;
+ private final PolarisPrincipal polarisPrincipal;
@Inject
public StorageAccessConfigProvider(
StorageCredentialCache storageCredentialCache,
- StorageCredentialsVendor storageCredentialsVendor) {
+ StorageCredentialsVendor storageCredentialsVendor,
+ PolarisPrincipal polarisPrincipal) {
Review Comment:
I wonder how it works for async tasks 🤔 Currently `PolarisPrincipal` is
[derived](https://github.com/apache/polaris/blob/fa472299fdf86736822b5fb60c0cfe22633603ac/runtime/service/src/main/java/org/apache/polaris/service/config/ServiceProducers.java#L203)
from `SecurityContext`. The latter is available only in the context on HTTP
requests, but tasks run in independent contexts now (#3210).
I imagine `TaskExecutorImpl` may have to propagate the `PolarisPrincipal` in
a similar way it propagates the Realm ID 🤔
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/cache/StorageCredentialCache.java:
##########
@@ -123,6 +125,7 @@ public StorageAccessConfig getOrGenerateSubScopeCreds(
allowListOperation,
allowedReadLocations,
allowedWriteLocations,
+ polarisPrincipal,
Review Comment:
This naturally broadens the range of key values for _all_ storage provides,
but only STS use case benefit from the value. Other storage backends will see a
reduction in cache efficiency without any change in credential vending
behaviour.
If you agree to my proposal of a feature flag, then even for STS the cache
efficiency may be reduced (when the flag is off) without any benefit.
WDYT?
I do not known of an easy solution for this, though. In my mind, ideally,
storage integrations implementation should provide a converter method,
condensing all available parameters to a sub-set of used parameters (based on
type and config), which would later be used for the cache key... however, this
will probably require a lot of refactoring.
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java:
##########
@@ -81,20 +82,24 @@ public StorageAccessConfig getSubscopedCreds(
boolean allowListOperation,
@Nonnull Set<String> allowedReadLocations,
@Nonnull Set<String> allowedWriteLocations,
+ @Nonnull PolarisPrincipal polarisPrincipal,
Optional<String> refreshCredentialsEndpoint) {
int storageCredentialDurationSeconds =
realmConfig.getConfig(STORAGE_CREDENTIAL_DURATION_SECONDS);
AwsStorageConfigurationInfo storageConfig = config();
String region = storageConfig.getRegion();
String accountId = storageConfig.getAwsAccountId();
StorageAccessConfig.Builder accessConfig = StorageAccessConfig.builder();
+ String roleSessionName = "polaris-" + polarisPrincipal.getName();
Review Comment:
I'd propose to cover this with a feature flag (defaulting to enabled +
CHANGELOG entry). I imagine in some cases Polaris administrators may not want
to expose principal names to external systems, which STS may be.
--
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]
