adutra commented on code in PR #8:
URL: https://github.com/apache/polaris-tools/pull/8#discussion_r2055573374
##########
polaris-synchronizer/api/src/main/java/org/apache/polaris/tools/sync/polaris/service/impl/PolarisIcebergCatalogService.java:
##########
@@ -160,4 +161,11 @@ public void dropTableWithoutPurge(TableIdentifier
tableIdentifier) {
this.catalog.dropTable(tableIdentifier, false /* purge */);
}
+ @Override
+ public void close() throws IOException {
+ if (this.catalog != null) {
+ this.catalog.close();
+ }
Review Comment:
```suggestion
this.catalog.close();
```
##########
polaris-synchronizer/cli/src/main/java/org/apache/polaris/tools/sync/polaris/CLIUtil.java:
##########
@@ -0,0 +1,48 @@
+package org.apache.polaris.tools.sync.polaris;
+
+import java.io.Closeable;
+import java.io.IOException;
+
+/**
+ * CLI specific utilities and constants.
+ */
+public class CLIUtil {
+
+ public static final String API_SERVICE_PROPERTIES_DESCRIPTION =
+ "\nProperties:" +
+ "\n\t- base-url: the base url of the Polaris instance (eg.
http://localhost:8181)" +
+ "\n\t- token: the bearer token to authenticate against the Polaris
instance with." +
+ "\n\t- oauth2-server-uri: the uri of the OAuth2 server to
authenticate to. (eg. http://localhost:8181/api/catalog/v1/oauth/tokens)" +
+ "\n\t- credential: the client credentials to use to authenticate
against the Polaris instance (eg. <client_id>:client_secret>)" +
+ "\n\t- scope: the scope to authenticate with for the service_admin
(eg. PRINCIPAL_ROLE:ALL)" +
+ "\n\t- <token_type>=<token>: for token exchange authentication,
the token type (eg. urn:ietf:params:oauth:token-type:access_token) with a
provided token";
+
+ public static final String OMNIPOTENT_PRINCIPAL_PROPERTIES_DESCRIPTION =
+ "\nOmnipotent Principal Properties:" +
+ "\n\t- omnipotent-principal-name: the name of the omnipotent
principal created using create-omnipotent-principal on the source Polaris" +
+ "\n\t- omnipotent-principal-client-id: the client id of the
omnipotent principal created using create-omnipotent-principal on the source
Polaris" +
+ "\n\t- omnipotent-principal-client-secret: the client secret of
the omnipotent principal created using create-omnipotent-principal on the
source Polaris" +
+ "\n\t- omnipotent-principal-oauth2-server-uri: (default:
/v1/oauth/tokens endpoint for provided Polaris base-url) "
+ + "the OAuth2 server to use to authenticate the
omnipotent-principal for Iceberg catalog access";
+
+ private CLIUtil() {}
+
+ /**
+ * While all resources should ideally be explicitly closed prior to
program termination,
+ * passing a closeable entity to this method adds a runtime shutdown hook
to close the
+ * provided resource on program termination, even if the entity was not
explicitly
+ * properly closed prior. This is useful in the event that some hard
failure occurs before
+ * the program reaches the call to {@link Closeable#close()}.
+ * @param closeable the resource to close
+ */
+ public static void closeResourceOnTermination(final Closeable closeable) {
Review Comment:
I don't think this is needed, it's possible to close all resources using
try-with-resources blocks.
##########
polaris-synchronizer/api/src/main/java/org/apache/polaris/tools/sync/polaris/auth/AuthenticationSessionWrapper.java:
##########
@@ -0,0 +1,88 @@
+package org.apache.polaris.tools.sync.polaris.auth;
+
+import org.apache.iceberg.rest.HTTPClient;
+import org.apache.iceberg.rest.RESTClient;
+import org.apache.iceberg.rest.auth.AuthConfig;
+import org.apache.iceberg.rest.auth.OAuth2Properties;
+import org.apache.iceberg.rest.auth.OAuth2Util;
+import org.apache.iceberg.util.ThreadPools;
+
+import java.io.Closeable;
+import java.io.IOException;
+import java.util.Map;
+import java.util.UUID;
+
+/**
+ * Wraps {@link OAuth2Util.AuthSession} to provide supported authentication
flows.
+ */
+public class AuthenticationSessionWrapper implements Closeable {
+
+ private final RESTClient restClient;
+
+ private final OAuth2Util.AuthSession authSession;
+
+ public AuthenticationSessionWrapper(Map<String, String> properties) {
+ this.restClient = HTTPClient.builder(Map.of())
+ .uri(properties.get(OAuth2Properties.OAUTH2_SERVER_URI))
+ .build();
+ this.authSession = this.newAuthSession(this.restClient, properties);
+ }
Review Comment:
While at it let's tackle the executor issue as well:
```suggestion
private final OAuth2Util.AuthSession authSession;
private final ScheduledExecutorService executor;
public AuthenticationSessionWrapper(Map<String, String> properties) {
this.restClient = HTTPClient.builder(Map.of())
.uri(properties.get(OAuth2Properties.OAUTH2_SERVER_URI))
.build();
this.authSession = this.newAuthSession(this.restClient, properties);
executor = ThreadPools.newScheduledPool(UUID.randomUUID() +
"-token-refresh", 1);
}
```
##########
polaris-synchronizer/cli/src/main/java/org/apache/polaris/tools/sync/polaris/SyncPolarisCommand.java:
##########
@@ -137,18 +109,12 @@ public Integer call() throws Exception {
Review Comment:
Use try-with-resources again:
```java
try (
PolarisService source =
PolarisServiceFactory.createPolarisService(PolarisServiceFactory.ServiceType.API,
sourceProperties);
PolarisService target = PolarisServiceFactory.createPolarisService(
PolarisServiceFactory.ServiceType.API, targetProperties)) {
```
##########
polaris-synchronizer/api/src/main/java/org/apache/polaris/tools/sync/polaris/catalog/PolarisCatalog.java:
##########
@@ -171,4 +158,17 @@ public Table loadTable(TableIdentifier ident, String etag)
{
return new BaseTable(ops, CatalogUtil.fullTableName(catalogName, ident));
}
+
+ @Override
+ public void close() throws IOException {
+ if (httpClient != null) {
+ httpClient.close();
+ }
+
+ if (this.authenticationSession != null) {
+ this.authenticationSession.close();
+ }
+
+ super.close();
Review Comment:
```suggestion
AuthenticationSessionWrapper session = authenticationSession;
HttpClient httpClient = this.httpClient;
try (session; httpClient) {
super.close();
} finally {
this.authenticationSession = null;
this.httpClient = null;
this.objectMapper = null;
this.resourcePaths = null;
}
```
##########
polaris-synchronizer/api/src/main/java/org/apache/polaris/tools/sync/polaris/auth/AuthenticationSessionWrapper.java:
##########
@@ -0,0 +1,88 @@
+package org.apache.polaris.tools.sync.polaris.auth;
+
+import org.apache.iceberg.rest.HTTPClient;
+import org.apache.iceberg.rest.RESTClient;
+import org.apache.iceberg.rest.auth.AuthConfig;
+import org.apache.iceberg.rest.auth.OAuth2Properties;
+import org.apache.iceberg.rest.auth.OAuth2Util;
+import org.apache.iceberg.util.ThreadPools;
+
+import java.io.Closeable;
+import java.io.IOException;
+import java.util.Map;
+import java.util.UUID;
+
+/**
+ * Wraps {@link OAuth2Util.AuthSession} to provide supported authentication
flows.
+ */
+public class AuthenticationSessionWrapper implements Closeable {
+
+ private final RESTClient restClient;
+
+ private final OAuth2Util.AuthSession authSession;
+
+ public AuthenticationSessionWrapper(Map<String, String> properties) {
+ this.restClient = HTTPClient.builder(Map.of())
+ .uri(properties.get(OAuth2Properties.OAUTH2_SERVER_URI))
+ .build();
+ this.authSession = this.newAuthSession(this.restClient, properties);
+ }
+
+ /**
+ * Initializes a new authentication session. Supports client_credentials
and bearer token flow.
+ * @param properties properties to initialize the session with
+ * @return an authentication session, with token refresh if applicable
+ */
+ private OAuth2Util.AuthSession newAuthSession(RESTClient restClient,
Map<String, String> properties) {
+ OAuth2Util.AuthSession parent = new OAuth2Util.AuthSession(
+ Map.of(),
+ AuthConfig.builder()
+ .scope(properties.get(OAuth2Properties.SCOPE))
+
.oauth2ServerUri(properties.get(OAuth2Properties.OAUTH2_SERVER_URI))
+
.optionalOAuthParams(OAuth2Util.buildOptionalParam(properties))
+ .build()
+ );
+
+ // This is for client_credentials flow
+ if (properties.containsKey(OAuth2Properties.CREDENTIAL)) {
+ return OAuth2Util.AuthSession.fromCredential(
+ restClient,
+ // threads created here will be daemon threads, so
termination of main program
+ // will terminate the token refresh thread automatically
+ ThreadPools.newScheduledPool(UUID.randomUUID() +
"-token-refresh", 1),
+ properties.get(OAuth2Properties.CREDENTIAL),
+ parent
+ );
+ }
+
+ // This is for regular bearer token flow
+ if (properties.containsKey(OAuth2Properties.TOKEN)) {
+ return OAuth2Util.AuthSession.fromAccessToken(
+ restClient,
+ // threads created here will be daemon threads, so
termination of main program
+ // will terminate the token refresh thread automatically
+ ThreadPools.newScheduledPool(UUID.randomUUID() +
"-access-token-refresh", 1),
+ properties.get(OAuth2Properties.TOKEN),
+ null, /* defaultExpiresAtMillis */
+ parent
+ );
+ }
+
+ throw new IllegalArgumentException("Unable to construct authenticated
session with the provided properties.");
+ }
+
+ /**
+ * Get refreshed authentication headers for session.
+ */
+ public Map<String, String> getSessionHeaders() {
+ return this.authSession.headers();
+ }
+
+ @Override
+ public void close() throws IOException {
+ if (this.restClient != null) {
+ this.restClient.close();
+ }
Review Comment:
```suggestion
try (restClient; executor){}
```
##########
polaris-synchronizer/cli/src/main/java/org/apache/polaris/tools/sync/polaris/CLIUtil.java:
##########
@@ -0,0 +1,48 @@
+package org.apache.polaris.tools.sync.polaris;
+
+import java.io.Closeable;
+import java.io.IOException;
+
+/**
+ * CLI specific utilities and constants.
+ */
+public class CLIUtil {
+
+ public static final String API_SERVICE_PROPERTIES_DESCRIPTION =
+ "\nProperties:" +
+ "\n\t- base-url: the base url of the Polaris instance (eg.
http://localhost:8181)" +
+ "\n\t- token: the bearer token to authenticate against the Polaris
instance with." +
+ "\n\t- oauth2-server-uri: the uri of the OAuth2 server to
authenticate to. (eg. http://localhost:8181/api/catalog/v1/oauth/tokens)" +
+ "\n\t- credential: the client credentials to use to authenticate
against the Polaris instance (eg. <client_id>:client_secret>)" +
+ "\n\t- scope: the scope to authenticate with for the service_admin
(eg. PRINCIPAL_ROLE:ALL)" +
+ "\n\t- <token_type>=<token>: for token exchange authentication,
the token type (eg. urn:ietf:params:oauth:token-type:access_token) with a
provided token";
Review Comment:
Does this still hold true?
##########
polaris-synchronizer/cli/src/main/java/org/apache/polaris/tools/sync/polaris/SyncPolarisCommand.java:
##########
@@ -137,18 +109,12 @@ public Integer call() throws Exception {
ETagManager etagService =
ETagManagerFactory.createETagManager(etagManagerType, etagManagerProperties);
- Runtime.getRuntime()
- .addShutdownHook(
- new Thread(
- () -> {
- if (etagService instanceof Closeable closableETagService) {
- try {
- closableETagService.close();
- } catch (IOException e) {
- throw new RuntimeException(e);
- }
- }
- }));
+ if (etagService instanceof Closeable closeableETagService) {
Review Comment:
Suggestion: make `ETagManager` extend `Autocloseable`:
```java
public interface ETagManager extends AutoCloseable {
...
@Override
default void close() throws Exception {
}
}
```
Then here:
```java
try (
PolarisService source =
PolarisServiceFactory.createPolarisService(PolarisServiceFactory.ServiceType.API,
sourceProperties);
PolarisService target = PolarisServiceFactory.createPolarisService(
PolarisServiceFactory.ServiceType.API, targetProperties);
ETagManager etagService =
ETagManagerFactory.createETagManager(etagManagerType,
etagManagerProperties)) {
PolarisSynchronizer synchronizer =
new PolarisSynchronizer(
consoleLog,
haltOnFailure,
accessControlAwarePlanner,
source,
target,
etagService);
synchronizer.syncPrincipalRoles();
if (shouldSyncPrincipals) {
consoleLog.warn(
"Principal migration will reset credentials on the target
Polaris instance. " +
"Principal migration will log the new target Principal
credentials to stdout.");
synchronizer.syncPrincipals();
}
synchronizer.syncCatalogs();
}
```
##########
polaris-synchronizer/cli/src/main/java/org/apache/polaris/tools/sync/polaris/CreateOmnipotentPrincipalCommand.java:
##########
@@ -99,6 +91,8 @@ public Integer call() throws Exception {
PolarisService polaris = PolarisServiceFactory.createPolarisService(
Review Comment:
Use try-with-resources instead, it's much more reliable:
```java
try (PolarisService polaris = PolarisServiceFactory.createPolarisService(
PolarisServiceFactory.ServiceType.API,
polarisApiConnectionProperties)) {
...
}
```
--
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]
