dimas-b commented on code in PR #3224:
URL: https://github.com/apache/polaris/pull/3224#discussion_r2599298835
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/cache/StorageCredentialCacheKey.java:
##########
@@ -49,6 +50,9 @@ public interface StorageCredentialCacheKey {
Set<String> allowedWriteLocations();
@Value.Parameter(order = 7)
+ Optional<String> principalName();
Review Comment:
Since this change is in `polaris-core`, would you mind keeping old indexes
for old values. This is to simplify adjusting to the changed code in downstream
projects.
##########
runtime/service/src/test/java/org/apache/polaris/service/catalog/generic/PolarisGenericTableCatalogRelationalTest.java:
##########
@@ -23,6 +23,6 @@
import org.apache.polaris.service.catalog.Profiles;
@QuarkusTest
-@TestProfile(Profiles.DefaultProfile.class)
+@TestProfile(Profiles.AuthProfile.class)
Review Comment:
Is this change necessary? What is broken without it (just trying to
understand).
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java:
##########
@@ -89,12 +92,22 @@ public StorageAccessConfig getSubscopedCreds(
String accountId = storageConfig.getAwsAccountId();
StorageAccessConfig.Builder accessConfig = StorageAccessConfig.builder();
+ boolean includePrincipalNameInSubscopedCredential =
+
realmConfig.getConfig(FeatureConfiguration.INCLUDE_PRINCIPAL_NAME_IN_SUBSCOPED_CREDENTIAL);
+
+ String roleSessionName =
+ includePrincipalNameInSubscopedCredential
+ ? "polaris-" + polarisPrincipal.getName()
+ : "polaris";
Review Comment:
Please keep `PolarisAwsCredentialsStorageIntegration` as default for
backward compatibility. I know, it's not a huge change, but we'd better not
change old behaviours unless we have to.
##########
runtime/service/src/main/java/org/apache/polaris/service/task/TaskExecutorImpl.java:
##########
@@ -160,9 +165,17 @@ public void addTaskHandlerContext(long taskEntityId,
CallContext callContext) {
return CompletableFuture.failedFuture(e);
}
String realmId = callContext.getRealmContext().getRealmIdentifier();
+
+ PolarisPrincipal principalClone =
+ PolarisPrincipal.of(
Review Comment:
Going through the builder would more future-proof:
`ImmutablePolarisPrincipal.builder().from(polarisPrincipal)`... The `from`
method could be promoted to the `PolarisPrincipal` interface too.
##########
polaris-core/src/test/java/org/apache/polaris/service/storage/aws/AwsCredentialsStorageIntegrationTest.java:
##########
@@ -97,6 +123,54 @@ public void testGetSubscopedCreds(String scheme) {
true,
Set.of(warehouseDir + "/namespace/table"),
Set.of(warehouseDir + "/namespace/table"),
+ POLARIS_PRINCIPAL,
+ Optional.of("/namespace/table/credentials"));
+ assertThat(storageAccessConfig.credentials())
+ .isNotEmpty()
+ .containsEntry(StorageAccessProperty.AWS_TOKEN.getPropertyName(),
"sess")
+ .containsEntry(StorageAccessProperty.AWS_KEY_ID.getPropertyName(),
"accessKey")
+ .containsEntry(StorageAccessProperty.AWS_SECRET_KEY.getPropertyName(),
"secretKey")
+ .containsEntry(
+
StorageAccessProperty.AWS_SESSION_TOKEN_EXPIRES_AT_MS.getPropertyName(),
+ String.valueOf(EXPIRE_TIME.toEpochMilli()));
+ assertThat(storageAccessConfig.extraProperties())
+ .containsEntry(
+
StorageAccessProperty.AWS_REFRESH_CREDENTIALS_ENDPOINT.getPropertyName(),
+ "/namespace/table/credentials");
+ }
+
+ @Test
+ public void testGetSubscopedCredsWithNameInclude() {
+ StsClient stsClient = Mockito.mock(StsClient.class);
+ String roleARN = "arn:aws:iam::012345678901:role/jdoe";
+ String externalId = "externalId";
+
+ Mockito.when(stsClient.assumeRole(Mockito.isA(AssumeRoleRequest.class)))
+ .thenAnswer(
+ invocation -> {
+ assertThat(invocation.getArguments()[0])
+ .isInstanceOf(AssumeRoleRequest.class)
+
.asInstanceOf(InstanceOfAssertFactories.type(AssumeRoleRequest.class))
+ .returns(externalId, AssumeRoleRequest::externalId)
+ .returns(roleARN, AssumeRoleRequest::roleArn)
+ .returns("polaris-principal",
AssumeRoleRequest::roleSessionName);
Review Comment:
nit using `polaris-test-principal` might be easier to follow. In current
form is it not very obvious (without browsing through code quite a few lines
away from here) whether the `principal` part is some sort of a code constant of
comes from the test parameters
##########
runtime/service/src/main/java/org/apache/polaris/service/task/TaskExecutorImpl.java:
##########
@@ -70,6 +72,8 @@ public class TaskExecutorImpl implements TaskExecutor {
private final MetaStoreManagerFactory metaStoreManagerFactory;
private final TaskFileIOSupplier fileIOSupplier;
private final RealmContextHolder realmContextHolder;
+ @Inject PolarisPrincipalHolder polarisPrincipalHolder;
+ @Inject PolarisPrincipal polarisPrincipal;
Review Comment:
I suppose it works in practice, but how it works might be unclear to some
readers 😅 Would you mind adding comments to describe how we propagate the
principal into a the holder in a different CDI context?
nit: I'd prefer to stick to constructor-based injection for all args. I
believe it will not have any functional effect, but will maintain code
uniformity (and allow fields to be `final`, which simplifies reasoning about
code behaviour).
--
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]
