review

Author: adutra

CHANGELOG.md

@@ -41,8 +41,8 @@ request adding CHANGELOG notes for breaking (!) changes and possibly other secti
   production-ready. One new table privilege was introduced: `TABLE_REMOTE_SIGN`. To enable remote signing:
     1. Set the system-wide property `REMOTE_SIGNING_ENABLED` or the catalog-level `polaris.request-signing.enabled`
        property to `true`.
-    2. Grant the `TABLE_REMOTE_SIGN` privilege to a catalog role. The role must also be granted the `TABLE_READ_DATA`
-       and `TABLE_WRITE_DATA` privileges.
+    2. Grant the `TABLE_REMOTE_SIGN` privilege to a catalog role. The catalog role must also be granted the 
+       `TABLE_READ_DATA` and `TABLE_WRITE_DATA` privileges.
 
 ### Upgrade notes
 

runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java

@@ -867,6 +867,14 @@ ALLOW_FEDERATED_CATALOGS_CREDENTIAL_VENDING, getResolvedCatalogEntity())) {
     return responseBuilder;
   }
 
+  /**
+   * Selects the most appropriate access delegation mode from the set of modes requested by the
+   * client.
+   *
+   * <p>See <a
+   * href="https://github.com/apache/iceberg/blob/main/open-api/rest-catalog-open-api.yaml#L1858-L1859">Iceberg
+   * REST Catalog spec</a>.
+   */
   private AccessDelegationMode selectAccessDelegationMode(
       Set<AccessDelegationMode> delegationModes) {
 
@@ -875,6 +883,7 @@ private AccessDelegationMode selectAccessDelegationMode(
     }
 
     if (delegationModes.size() == 1) {
+      // No need to validate the mode here, it will be validated later.
       return delegationModes.iterator().next();
     }
 

runtime/service/src/main/java/org/apache/polaris/service/catalog/io/AccessConfigProvider.java

@@ -169,6 +169,7 @@ public AccessConfig getAccessConfigForRemoteSigning(
     }
 
     String prefix = prefixParser.catalogNameToPrefix(callContext.getRealmContext(), catalogName);
+    // TODO M2 handle cases where the catalog server is behind a proxy
     URI signerUri = uriInfo.getBaseUriBuilder().path(PolarisResourcePaths.API_PATH_SEGMENT).build();
     String signerEndpoint = new PolarisResourcePaths(prefix).s3RemoteSigning(tableIdentifier);
 

runtime/service/src/main/java/org/apache/polaris/service/config/ProductionReadinessChecks.java

@@ -383,4 +383,32 @@ public ProductionReadinessCheck checkConnectionCredentialVendors(
         ? ProductionReadinessCheck.OK
         : ProductionReadinessCheck.of(errors.toArray(new Error[0]));
   }
+
+  @Produces
+  public ProductionReadinessCheck checkRemoteSigning(FeaturesConfiguration featureConfiguration) {
+    var errors = new ArrayList<Error>();
+    var offending = FeatureConfiguration.REMOTE_SIGNING_ENABLED;
+    if (Boolean.parseBoolean(featureConfiguration.defaults().get(offending.key()))) {
+      errors.add(
+          Error.ofSevere(
+              "Remote signing is an experimental feature and should not be enabled in production.",
+              format("polaris.features.\"%s\"", offending.key())));
+    }
+    featureConfiguration
+        .realmOverrides()
+        .forEach(
+            (realmId, overrides) -> {
+              if (Boolean.parseBoolean(overrides.overrides().get(offending.key()))) {
+                errors.add(
+                    Error.ofSevere(
+                        "Remote signing is an experimental feature and should not be enabled in production.",
+                        format(
+                            "polaris.features.realm-overrides.\"%s\".overrides.\"%s\"",
+                            realmId, offending.key())));
+              }
+            });
+    return errors.isEmpty()
+        ? ProductionReadinessCheck.OK
+        : ProductionReadinessCheck.of(errors.toArray(new Error[0]));
+  }
 }

runtime/service/src/main/java/org/apache/polaris/service/storage/s3/sign/S3RemoteSigningCatalogHandler.java

@@ -128,6 +128,7 @@ public PolarisS3SignResponse signS3Request(
     return s3SignResponse;
   }
 
+  // TODO M2 computing allowed locations is expensive. We should cache it.
   private Collection<String> getAllowedLocations(TableIdentifier tableIdentifier) {
 
     if (baseCatalog.tableExists(tableIdentifier)) {
@@ -166,7 +167,12 @@ private Collection<String> getAllowedLocations(TableIdentifier tableIdentifier)
   }
 
   private Set<PolarisStorageActions> getStorageActions(PolarisS3SignRequest s3SignRequest) {
-    // TODO M2: better handling of DELETE and LIST
+    // TODO M2: better mapping of request URIs to storage actions.
+    // Disambiguate LIST vs READ or WRITE vs DELETE is not possible based on the HTTP method alone.
+    // Examples:
+    // - ListObjects is conceptually a LIST operation, and GetObject is conceptually a READ. But
+    // both requests use the GET method.
+    // - DeleteObject uses the DELETE method, but the DeleteObjects operation uses the POST method.
     return s3SignRequest.write()
         ? Set.of(PolarisStorageActions.WRITE)
         : Set.of(PolarisStorageActions.READ);

runtime/service/src/test/java/org/apache/polaris/service/it/S3RemoteSigningMinIOIntegrationTest.java

@@ -52,6 +52,7 @@ public Map<String, String> getConfigOverrides() {
       return ImmutableMap.<String, String>builder()
           .put("polaris.storage.aws.access-key", MINIO_ACCESS_KEY)
           .put("polaris.storage.aws.secret-key", MINIO_SECRET_KEY)
+          .put("polaris.readiness.ignore-severe-issues", "true")
           .put("polaris.features.\"REMOTE_SIGNING_ENABLED\"", "true")
           .put("polaris.features.\"SUPPORTED_CATALOG_STORAGE_TYPES\"", "[\"S3\"]")
           .put("polaris.features.\"SKIP_CREDENTIAL_SUBSCOPING_INDIRECTION\"", "false")