dimas-b commented on code in PR #3327:
URL: https://github.com/apache/polaris/pull/3327#discussion_r2651836748
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisCredentialVendor.java:
##########
@@ -35,17 +35,64 @@ public interface PolarisCredentialVendor {
* @param callCtx the polaris call context
* @param catalogId the catalog id
* @param entityId the entity id
+ * @param entityType the type of entity
* @param allowListOperation whether to allow LIST operation on the
allowedReadLocations and
* allowedWriteLocations
* @param allowedReadLocations a set of allowed to read locations
* @param allowedWriteLocations a set of allowed to write locations
+ * @param polarisPrincipal the principal requesting credentials
* @param refreshCredentialsEndpoint an optional endpoint to use for
refreshing credentials. If
* supported by the storage type it will be returned to the client in
the appropriate
* properties. The endpoint may be relative to the base URI and the
client is responsible for
* handling the relative path
* @return an enum map containing the scoped credentials
*/
@Nonnull
+ default ScopedCredentialsResult getSubscopedCredsForEntity(
Review Comment:
This method is not called in Polaris. Let's deprecate it for removal (in a
later release).
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java:
##########
@@ -366,4 +388,67 @@ private static String arnPrefixForPartition(String
awsPartition) {
}
return path;
}
+
+ /**
+ * Builds a list of AWS STS session tags from the credential vending context
and principal name.
+ * These tags will appear in CloudTrail events for correlation purposes.
+ *
+ * @param principalName the name of the principal requesting credentials
+ * @param context the credential vending context containing catalog,
namespace, table, and roles
+ * @return a list of STS Tags to attach to the AssumeRole request
+ */
+ private List<Tag> buildSessionTags(String principalName,
CredentialVendingContext context) {
+ List<Tag> tags = new ArrayList<>();
+
+ // Always include all tags with "unknown" placeholder for missing values
+ // This ensures consistent tag presence in CloudTrail for correlation
+ tags.add(
+ Tag.builder()
+ .key(CredentialVendingContext.TAG_KEY_PRINCIPAL)
+ .value(truncateTagValue(principalName))
+ .build());
+ tags.add(
+ Tag.builder()
+ .key(CredentialVendingContext.TAG_KEY_CATALOG)
+ .value(truncateTagValue(context.catalogName().orElse(null)))
Review Comment:
Is having a null tag preferable to not having the tag at all? (just
wondering)
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/cache/StorageCredentialCache.java:
##########
@@ -130,21 +146,26 @@ public StorageAccessConfig getOrGenerateSubScopeCreds(
allowedReadLocations,
allowedWriteLocations,
refreshCredentialsEndpoint,
- includePrincipalNameInSubscopedCredential
- ? Optional.of(polarisPrincipal)
- : Optional.empty());
- LOGGER.atDebug().addKeyValue("key", key).log("subscopedCredsCache");
+ includePrincipalInCacheKey ? Optional.of(polarisPrincipal) :
Optional.empty(),
+ includeSessionTags ? Optional.of(credentialVendingContext) :
Optional.empty());
Function<StorageCredentialCacheKey, StorageCredentialCacheEntry> loader =
k -> {
LOGGER.atDebug().log("StorageCredentialCache::load");
+ // Use credentialVendingContext from the cache key for correctness.
+ // When session tags are disabled, the key stores Optional.empty(),
and we should
+ // use an empty context to ensure consistent behavior regardless of
what the
+ // local variable contains.
+ CredentialVendingContext contextFromKey =
+
k.credentialVendingContext().orElse(CredentialVendingContext.empty());
Review Comment:
I believe this is still not ideal. The key might have an `Optional.empty()`
but the storage integration code will use `CredentialVendingContext.empty()`...
How can we be sure they signify the same thing? Why not just put
`CredentialVendingContext.empty()` in the key and make this value non-optional
in the key?
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/StorageCredentialsVendor.java:
##########
Review Comment:
same here - this method in not used in Polaris. Let's deprecate.
--
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]
