Skip to content

add refresh credentials property to loadTableResult #1164

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 25 commits into from
Closed

add refresh credentials property to loadTableResult #1164

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
63c7dc7
add refresh credentials property to loadTableResult
Mar 12, 2025
a885ffb
remove bad method and replace with localhost
Mar 12, 2025
7912eca
spotless
Mar 12, 2025
aa6f1b1
add request base uri to call context
Mar 14, 2025
9c1c283
fix tests
Mar 17, 2025
0241b80
add uri builder
Mar 19, 2025
c1f2cbe
clean up code
Mar 19, 2025
b0057d4
fix wrong url
Mar 19, 2025
97b8990
merge
Mar 19, 2025
d8e55c0
remove delegation modes
Mar 19, 2025
13d33a5
cleanup code
Mar 19, 2025
7961dc2
change based on iceberg latest updates
Mar 21, 2025
f2f3fd1
merge
Mar 21, 2025
348f1ee
spotlessApply
Mar 21, 2025
a2a4640
fix path
Mar 21, 2025
ab02a2d
Merge branch 'main' into loadTableResponse-refresh-credential-properties
Mar 28, 2025
87d8358
remove getBaseUri
Apr 10, 2025
07f9dc1
merge
Apr 10, 2025
7002409
refactor based on merge
Apr 10, 2025
aec31d7
move endpoint construction to IcebergCatalogAdapter
Apr 10, 2025
bcea6c6
pass down path
Apr 10, 2025
6ccc3e1
spotlessApply
Apr 11, 2025
54ef878
remove unnecessary method
Apr 11, 2025
315afda
inject refresh credential properties at proper place
Apr 15, 2025
d073982
Merge branch 'main' into loadTableResponse-refresh-credential-properties
Apr 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions ...e/src/test/java/org/apache/polaris/service/quarkus/auth/JWTSymmetricKeyGeneratorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import com.auth0.jwt.JWTVerifier;
import com.auth0.jwt.algorithms.Algorithm;
import com.auth0.jwt.interfaces.DecodedJWT;
import java.net.URI;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unnecessary import?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has been removed

import java.util.Map;
import org.apache.polaris.core.PolarisCallContext;
import org.apache.polaris.core.context.CallContext;
Expand Down Expand Up @@ -65,6 +66,11 @@ public PolarisCallContext getPolarisCallContext() {
public Map<String, Object> contextVariables() {
return Map.of();
}

@Override
public URI getBaseUri() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change still needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is removed

return null;
}
});
PolarisMetaStoreManager metastoreManager = Mockito.mock(PolarisMetaStoreManager.class);
String mainSecret = "test_secret";
Expand Down
22 changes: 18 additions & 4 deletions ...ava/org/apache/polaris/service/quarkus/catalog/IcebergCatalogHandlerWrapperAuthzTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import io.quarkus.test.junit.TestProfile;
import jakarta.ws.rs.core.SecurityContext;
import java.time.Instant;
import java.util.EnumSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
Expand Down Expand Up @@ -64,6 +65,7 @@
import org.apache.polaris.core.persistence.PolarisEntityManager;
import org.apache.polaris.core.persistence.dao.entity.CreatePrincipalResult;
import org.apache.polaris.core.persistence.resolver.PolarisResolutionManifest;
import org.apache.polaris.service.catalog.AccessDelegationMode;
import org.apache.polaris.service.catalog.iceberg.IcebergCatalogHandlerWrapper;
import org.apache.polaris.service.catalog.io.DefaultFileIOFactory;
import org.apache.polaris.service.config.RealmEntityManagerFactory;
Expand Down Expand Up @@ -872,7 +874,10 @@ public void testLoadTableWithReadAccessDelegationSufficientPrivileges() {
PolarisPrivilege.TABLE_READ_DATA,
PolarisPrivilege.TABLE_WRITE_DATA,
PolarisPrivilege.CATALOG_MANAGE_CONTENT),
() -> newWrapper().loadTableWithAccessDelegation(TABLE_NS1A_2, "all"),
() ->
newWrapper()
.loadTableWithAccessDelegation(
TABLE_NS1A_2, "all", EnumSet.noneOf(AccessDelegationMode.class)),
null /* cleanupAction */);
}

Expand All @@ -888,7 +893,10 @@ public void testLoadTableWithReadAccessDelegationInsufficientPermissions() {
PolarisPrivilege.TABLE_CREATE,
PolarisPrivilege.TABLE_LIST,
PolarisPrivilege.TABLE_DROP),
() -> newWrapper().loadTableWithAccessDelegation(TABLE_NS1A_2, "all"));
() ->
newWrapper()
.loadTableWithAccessDelegation(
TABLE_NS1A_2, "all", EnumSet.noneOf(AccessDelegationMode.class)));
}

@Test
Expand All @@ -901,7 +909,10 @@ public void testLoadTableWithWriteAccessDelegationSufficientPrivileges() {
PolarisPrivilege.TABLE_READ_DATA,
PolarisPrivilege.TABLE_WRITE_DATA,
PolarisPrivilege.CATALOG_MANAGE_CONTENT),
() -> newWrapper().loadTableWithAccessDelegation(TABLE_NS1A_2, "all"),
() ->
newWrapper()
.loadTableWithAccessDelegation(
TABLE_NS1A_2, "all", EnumSet.noneOf(AccessDelegationMode.class)),
null /* cleanupAction */);
}

Expand All @@ -917,7 +928,10 @@ public void testLoadTableWithWriteAccessDelegationInsufficientPermissions() {
PolarisPrivilege.TABLE_CREATE,
PolarisPrivilege.TABLE_LIST,
PolarisPrivilege.TABLE_DROP),
() -> newWrapper().loadTableWithAccessDelegation(TABLE_NS1A_2, "all"));
() ->
newWrapper()
.loadTableWithAccessDelegation(
TABLE_NS1A_2, "all", EnumSet.noneOf(AccessDelegationMode.class)));
}

@Test
Expand Down
14 changes: 14 additions & 0 deletions service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
import org.apache.iceberg.TableMetadata;
import org.apache.iceberg.TableMetadataParser;
import org.apache.iceberg.TableOperations;
import org.apache.iceberg.aws.AwsClientProperties;
import org.apache.iceberg.catalog.Namespace;
import org.apache.iceberg.catalog.SupportsNamespaces;
import org.apache.iceberg.catalog.TableIdentifier;
Expand Down Expand Up @@ -853,6 +854,19 @@ public Map<String, String> getCredentialConfig(
storageInfo.get());
}

@Override
public Map<String, String> getVendedCredentialConfig(TableIdentifier tableIdentifier) {
Map<String, String> vendedCredentialConfig = new HashMap<>();
String credentialsEndpoint =
String.format(
"/v1/%s/namespaces/%s/tables/%s/credentials",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IcebergCatalog does not deal with the REST API at all, but this config requires understanding namespace path encoding/decoding.

I believe it is preferable to inject this config in IcebergCatalogAdapter (where the decoding code exists).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved the string construction to IcebergCatalogAdapter where the decoding code exists, but I feel that's a little bit clumsy to pass it all the way down to SupportsCredentialDelegation. I wonder if you have a more elegant way of doing this

catalogName, tableIdentifier.namespace().toString(), tableIdentifier.name());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this do proper escaping of special chars in the URI path?

Cf. IcebergCatalogAdapter.decodeNamespace()

vendedCredentialConfig.put(AwsClientProperties.REFRESH_CREDENTIALS_ENABLED, "true");
vendedCredentialConfig.put(
AwsClientProperties.REFRESH_CREDENTIALS_ENDPOINT, credentialsEndpoint);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this property specific to the AWS client? I did not find any formal definition for it in Iceberg.

I believe it would be preferable to have a local constant for it in Polaris to avoid the impression that it is supposed to work only for AWS.

Given apache/iceberg#11281, why do we have to set this property at all? Why is it not discovered automatically?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was my question to the iceberg team as well, but they insist on having this property set so iceberg knows which endpoint to reach out to.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the AwsClientProperties, there's also discussion here

return vendedCredentialConfig;
}

/**
* Based on configuration settings, for callsites that need to handle potentially setting a new
* base location for a TableLike entity, produces the transformed location if applicable, or else
Expand Down
7 changes: 5 additions & 2 deletions ...ommon/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogAdapter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,7 +344,9 @@ public Response loadTable(
if (delegationModes.isEmpty()) {
return Response.ok(catalog.loadTable(tableIdentifier, snapshots)).build();
} else {
return Response.ok(catalog.loadTableWithAccessDelegation(tableIdentifier, snapshots))
return Response.ok(
catalog.loadTableWithAccessDelegation(
tableIdentifier, snapshots, delegationModes))
.build();
}
});
Expand Down Expand Up @@ -485,7 +487,8 @@ public Response loadCredentials(
prefix,
catalog -> {
LoadTableResponse loadTableResponse =
catalog.loadTableWithAccessDelegation(tableIdentifier, "all");
catalog.loadTableWithAccessDelegation(
tableIdentifier, "all", EnumSet.noneOf(AccessDelegationMode.class));
return Response.ok(
ImmutableLoadCredentialsResponse.builder()
.credentials(loadTableResponse.credentials())
Expand Down
10 changes: 9 additions & 1 deletion ...rc/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandlerWrapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import java.time.ZoneOffset;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.EnumSet;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -90,6 +91,7 @@
import org.apache.polaris.core.persistence.resolver.ResolverPath;
import org.apache.polaris.core.persistence.resolver.ResolverStatus;
import org.apache.polaris.core.storage.PolarisStorageActions;
import org.apache.polaris.service.catalog.AccessDelegationMode;
import org.apache.polaris.service.catalog.SupportsNotifications;
import org.apache.polaris.service.context.CallContextCatalogFactory;
import org.apache.polaris.service.types.NotificationRequest;
Expand Down Expand Up @@ -777,7 +779,9 @@ public LoadTableResponse loadTable(TableIdentifier tableIdentifier, String snaps
}

public LoadTableResponse loadTableWithAccessDelegation(
TableIdentifier tableIdentifier, String snapshots) {
TableIdentifier tableIdentifier,
String snapshots,
EnumSet<AccessDelegationMode> accessDelegationModes) {
// Here we have a single method that falls through multiple candidate
// PolarisAuthorizableOperations because instead of identifying the desired operation up-front
// and
Expand Down Expand Up @@ -839,6 +843,10 @@ public LoadTableResponse loadTableWithAccessDelegation(
responseBuilder.addAllConfig(
credentialDelegation.getCredentialConfig(
tableIdentifier, tableMetadata, actionsRequested));
if (accessDelegationModes.contains(AccessDelegationMode.VENDED_CREDENTIALS)) {
responseBuilder.addAllConfig(
credentialDelegation.getVendedCredentialConfig(tableIdentifier));
}
}
return responseBuilder.build();
} else if (table instanceof BaseMetadataTable) {
Expand Down
2 changes: 2 additions & 0 deletions ...rc/main/java/org/apache/polaris/service/catalog/iceberg/SupportsCredentialDelegation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,4 +36,6 @@ Map<String, String> getCredentialConfig(
TableIdentifier tableIdentifier,
TableMetadata tableMetadata,
Set<PolarisStorageActions> storageActions);

Map<String, String> getVendedCredentialConfig(TableIdentifier tableIdentifier);
}
6 changes: 6 additions & 0 deletions service/common/src/test/java/org/apache/polaris/service/catalog/io/FileIOFactoryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import com.google.common.collect.ImmutableMap;
import jakarta.annotation.Nonnull;
import java.lang.reflect.Method;
import java.net.URI;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unnecessary import?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is removed.

import java.time.Clock;
import java.util.HashMap;
import java.util.List;
Expand Down Expand Up @@ -149,6 +150,11 @@ public PolarisCallContext getPolarisCallContext() {
public Map<String, Object> contextVariables() {
return new HashMap<>();
}

@Override
public URI getBaseUri() {
return null;
}
};
}

Expand Down
6 changes: 6 additions & 0 deletions service/common/src/testFixtures/java/org/apache/polaris/service/TestServices.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import com.google.auth.oauth2.AccessToken;
import com.google.auth.oauth2.GoogleCredentials;
import jakarta.ws.rs.core.SecurityContext;
import java.net.URI;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, i forgot to do spotlessApply. these should all be fixed now.

import java.security.Principal;
import java.time.Clock;
import java.time.Instant;
Expand Down Expand Up @@ -158,6 +159,11 @@ public PolarisCallContext getPolarisCallContext() {
public Map<String, Object> contextVariables() {
return new HashMap<>();
}

@Override
public URI getBaseUri() {
return null;
}
};

FileIOFactory fileIOFactory =
Expand Down