adutra commented on code in PR #2280:
URL: https://github.com/apache/polaris/pull/2280#discussion_r2503670005
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/io/AccessConfigProvider.java:
##########
@@ -101,4 +120,58 @@ public AccessConfig getAccessConfig(
storageInfo.get(),
refreshCredentialsEndpoint);
}
+
+ /**
+ * Generates a remote signing configuration for accessing table storage at
explicit locations.
+ *
+ * @param callContext the call context containing realm, principal, and
security context
+ * @param catalogName the name of the catalog
+ * @param tableIdentifier the table identifier, used for logging and refresh
endpoint construction
+ * @param resolvedPath the entity hierarchy to search for storage
configuration
+ * @return {@link AccessConfig} with scoped credentials and metadata; empty
if no storage config
+ * found
+ */
+ public AccessConfig getAccessConfigForRemoteSigning(
+ @Nonnull CallContext callContext,
+ @Nonnull String catalogName,
+ @Nonnull TableIdentifier tableIdentifier,
+ @Nonnull PolarisResolvedPathWrapper resolvedPath) {
+ LOGGER
+ .atDebug()
+ .addKeyValue("tableIdentifier", tableIdentifier)
+ .log("Fetching remote signing config for table");
+
+ Optional<PolarisEntity> storageInfo =
FileIOUtil.findStorageInfoFromHierarchy(resolvedPath);
+ Optional<PolarisStorageConfigurationInfo> configurationInfo =
+ storageInfo
+ .map(PolarisEntity::getInternalPropertiesAsMap)
+ .map(info ->
info.get(PolarisEntityConstants.getStorageConfigInfoPropertyName()))
+ .map(PolarisStorageConfigurationInfo::deserialize);
+
+ if (configurationInfo.isEmpty()) {
+ LOGGER
+ .atWarn()
+ .addKeyValue("tableIdentifier", tableIdentifier)
+ .log("Table entity has no storage configuration in its hierarchy");
+ return AccessConfig.EMPTY;
+ }
+
+ PolarisStorageIntegration<AwsStorageConfigurationInfo> storageIntegration =
+
storageIntegrationProvider.getStorageIntegrationForConfig(configurationInfo.get());
+
+ if (!(storageIntegration
+ instanceof AwsCredentialsStorageIntegration
awsCredentialsStorageIntegration)) {
+ LOGGER
+ .atWarn()
+ .addKeyValue("tableIdentifier", tableIdentifier)
+ .log("Table entity storage integration is not an AWS credentials
storage integration");
+ return AccessConfig.EMPTY;
+ }
+
+ String prefix =
prefixParser.catalogNameToPrefix(callContext.getRealmContext(), catalogName);
+ URI signerUri = uriInfo.getBaseUri().resolve("api/");
Review Comment:
I agree, but I'm not sure a constant in `s3-sign-service` is the best
solution. The `api/` path segment doesn't seem to be a first-class citizen in
our APIs, rather a bi-product. It's only declared as a server variable in the
OpenAPI generator config, for example:
https://github.com/apache/polaris/blob/1f4e748a96b926cd1639d6ed463997d2f9c455c4/api/iceberg-service/build.gradle.kts#L86
https://github.com/apache/polaris/blob/b6e247deda8a618136ec16c75b86eb2a2d12642b/api/polaris-catalog-service/build.gradle.kts#L116
The management APi is actually a bit different, there is not `basePath`
variable in polaris-management-service.yml, so the base path is hard-coded:
https://github.com/apache/polaris/blob/04c8ee6972df948beb15401e8dc905a37b27c33d/spec/polaris-management-service.yml#L26
But the common characteristic is that `api/`is the first path segment of all
Polaris APIs, but that is not enforced by any constraint.
I would say the right place for this segment would be in
`PolarisResourcePaths`, but you will notice again that none of the paths
include the `api/` segment. Same for Iceberg's `ResourcePaths`. So it's really
implicit 😅
All of this to say, let's declare a constant in `PolarisResourcePaths` and
call it a day, wdyt?
--
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]
