dimas-b commented on code in PR #3170:
URL: https://github.com/apache/polaris/pull/3170#discussion_r2566609565
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/PolarisCredential.java:
##########
@@ -31,14 +31,22 @@
public interface PolarisCredential extends Credential {
static PolarisCredential of(
- @Nullable Long principalId, @Nullable String principalName, Set<String>
principalRoles) {
Review Comment:
Generally speaking, it is preferable to add new builder methods for new
optional parameters (and chain old methods into them). This way existing code
that does not use new optional parameter won't have to change.
##########
spec/polaris-management-service.yml:
##########
@@ -1136,6 +1136,11 @@ components:
for accessing data and metadata files within the related
catalog. Setting this property to `true`
effectively disables vending storage credentials to clients.
This setting is intended for configuring
catalogs with S3-compatible storage implementations that do
not support STS.
+ userTokenSts:
Review Comment:
suggestion: `propagateApiUserIdentity` - it is more directly connected to
actual code behaviour... at least for me 😅 I'd welcome other naming suggestions
too. Better have this discussion now than after the stuff is released :)
##########
client/python/apache_polaris/cli/command/catalogs.py:
##########
@@ -76,6 +76,7 @@ class CatalogsCommand(Command):
sts_endpoint: str
sts_unavailable: bool
path_style_access: bool
+ user_token_sts: bool
Review Comment:
nit: Combining CLI changes and backend changes in the same PR is ok, but
these areas usually attract different sets of reviewers. As for me, I usually
do CLI changes in a follow-up PR. I also usually do not usually have material
opinions about CLI code changes :) all in all, it might be easier for reviewers
to have CLI and java changes in different PRs... I guess.
##########
integration-tests/src/main/java/org/apache/polaris/service/it/test/CatalogFederationIntegrationTest.java:
##########
@@ -159,6 +159,7 @@ private void setupCatalogs() {
AwsStorageConfigInfo.builder()
.setStorageType(StorageConfigInfo.StorageTypeEnum.S3)
.setPathStyleAccess(true)
+ .setUserTokenSts(false)
Review Comment:
Is this required? Could we handle defaults transparently to code areas not
affected by the new feature?
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisCredential.java:
##########
@@ -16,7 +16,7 @@
* specific language governing permissions and limitations
* under the License.
*/
-package org.apache.polaris.service.auth;
+package org.apache.polaris.core.auth;
Review Comment:
I'm not sure about moving this class to core 🤔
Why not add the token as a well-known property to `PolarisPrincipal`?
##########
client/python/apache_polaris/cli/command/__init__.py:
##########
@@ -54,6 +54,7 @@ def options_get(key, f=lambda x: x):
allowed_locations=options_get(Arguments.ALLOWED_LOCATION),
role_arn=options_get(Arguments.ROLE_ARN),
external_id=options_get(Arguments.EXTERNAL_ID),
+ user_token_sts=options_get(Arguments.USER_TOKEN_STS),
Review Comment:
I made a comment about this name on the API yaml. Let's discuss actual name
there.
Once that is decided, my personal opinion is that using the same name in
Python CLI is quite appropriate.
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java:
##########
@@ -44,13 +46,15 @@
import software.amazon.awssdk.policybuilder.iam.IamStatement;
import software.amazon.awssdk.services.sts.StsClient;
import software.amazon.awssdk.services.sts.model.AssumeRoleRequest;
-import software.amazon.awssdk.services.sts.model.AssumeRoleResponse;
+import
software.amazon.awssdk.services.sts.model.AssumeRoleWithWebIdentityRequest;
+import software.amazon.awssdk.services.sts.model.Credentials;
/** Credential vendor that supports generating */
public class AwsCredentialsStorageIntegration
extends InMemoryStorageIntegration<AwsStorageConfigurationInfo> {
private final StsClientProvider stsClientProvider;
private final Optional<AwsCredentialsProvider> credentialsProvider;
+ private final SecurityIdentity securityIdentity;
Review Comment:
Depending on Quarkus classes in core is really not desirable... AFAIK, sorry.
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/PolarisCredential.java:
##########
@@ -31,14 +31,22 @@
public interface PolarisCredential extends Credential {
static PolarisCredential of(
- @Nullable Long principalId, @Nullable String principalName, Set<String>
principalRoles) {
Review Comment:
Please add a new `of()` method for the new parameter (since it's optional).
This way existing code that does not use the token, does not have to change.
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java:
##########
@@ -44,13 +46,15 @@
import software.amazon.awssdk.policybuilder.iam.IamStatement;
import software.amazon.awssdk.services.sts.StsClient;
import software.amazon.awssdk.services.sts.model.AssumeRoleRequest;
-import software.amazon.awssdk.services.sts.model.AssumeRoleResponse;
+import
software.amazon.awssdk.services.sts.model.AssumeRoleWithWebIdentityRequest;
+import software.amazon.awssdk.services.sts.model.Credentials;
/** Credential vendor that supports generating */
public class AwsCredentialsStorageIntegration
extends InMemoryStorageIntegration<AwsStorageConfigurationInfo> {
private final StsClientProvider stsClientProvider;
private final Optional<AwsCredentialsProvider> credentialsProvider;
+ private final SecurityIdentity securityIdentity;
Review Comment:
`PolarisPrincipal` should be injectable into `StorageAccessConfigProvider`
and then it should be possible to propagate it to this class as method
parameters... WDYT?
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisCredential.java:
##########
@@ -31,14 +31,22 @@
public interface PolarisCredential extends Credential {
static PolarisCredential of(
- @Nullable Long principalId, @Nullable String principalName, Set<String>
principalRoles) {
+ @Nullable String token,
+ @Nullable Long principalId,
+ @Nullable String principalName,
+ Set<String> principalRoles) {
Review Comment:
Please add a new `of()` method for the new parameter (since it's optional).
This way existing code that does not use the token, does not have to change.
--
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]
