dimas-b commented on code in PR #389: URL: https://github.com/apache/polaris/pull/389#discussion_r1994092260
########## polaris-core/src/main/java/org/apache/polaris/core/storage/s3compatible/S3CompatibleCredentialsStorageIntegration.java: ########## @@ -0,0 +1,123 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.polaris.core.storage.s3compatible; + +import static org.apache.polaris.core.config.FeatureConfiguration.STORAGE_CREDENTIAL_DURATION_SECONDS; +import static org.apache.polaris.core.config.PolarisConfiguration.loadConfig; +import static org.apache.polaris.core.storage.PolarisCredentialProperty.AWS_KEY_ID; +import static org.apache.polaris.core.storage.PolarisCredentialProperty.AWS_SECRET_KEY; +import static org.apache.polaris.core.storage.PolarisCredentialProperty.AWS_TOKEN; + +import jakarta.annotation.Nonnull; +import jakarta.ws.rs.NotAuthorizedException; +import java.net.URI; +import java.util.EnumMap; +import java.util.Set; +import org.apache.polaris.core.PolarisDiagnostics; +import org.apache.polaris.core.storage.InMemoryStorageIntegration; +import org.apache.polaris.core.storage.PolarisCredentialProperty; +import org.apache.polaris.core.storage.StorageUtil; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import software.amazon.awssdk.auth.credentials.AwsBasicCredentials; +import software.amazon.awssdk.auth.credentials.ProfileCredentialsProvider; +import software.amazon.awssdk.auth.credentials.StaticCredentialsProvider; +import software.amazon.awssdk.profiles.ProfileFileSupplier; +import software.amazon.awssdk.services.sts.StsClient; +import software.amazon.awssdk.services.sts.StsClientBuilder; +import software.amazon.awssdk.services.sts.model.AssumeRoleRequest; +import software.amazon.awssdk.services.sts.model.AssumeRoleResponse; + +/** S3 compatible implementation of PolarisStorageIntegration */ +public class S3CompatibleCredentialsStorageIntegration + extends InMemoryStorageIntegration<S3CompatibleStorageConfigurationInfo> { + + private static final Logger LOGGER = + LoggerFactory.getLogger(S3CompatibleCredentialsStorageIntegration.class); + + public S3CompatibleCredentialsStorageIntegration() { + super(S3CompatibleCredentialsStorageIntegration.class.getName()); + } + + @Override + public EnumMap<PolarisCredentialProperty, String> getSubscopedCreds( + @Nonnull PolarisDiagnostics diagnostics, + @Nonnull S3CompatibleStorageConfigurationInfo storageConfig, + boolean allowListOperation, + @Nonnull Set<String> allowedReadLocations, + @Nonnull Set<String> allowedWriteLocations) { + + String caI = System.getenv(storageConfig.getS3CredentialsCatalogAccessKeyId()); Review Comment: I'm strongly opposed to storing credential env. variable name is the strage config (essentially in the catalog config). The environment and catalog config are different management domains with different concerns. Using a static env. variable name (e.g. `AWS_ACCESS_KEY`) is ok in the short term, IMHO. Is it strictly necessary to support different secrets for different catalogs in this case? ########## polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java: ########## @@ -141,6 +143,22 @@ private StorageConfigInfo getStorageInfo(Map<String, String> internalProperties) .setRegion(awsConfig.getRegion()) .build(); } + if (configInfo instanceof S3CompatibleStorageConfigurationInfo) { + S3CompatibleStorageConfigurationInfo s3Config = + (S3CompatibleStorageConfigurationInfo) configInfo; + return S3CompatibleStorageConfigInfo.builder() + .setStorageType(StorageConfigInfo.StorageTypeEnum.S3_COMPATIBLE) + .setS3Endpoint(s3Config.getS3Endpoint()) + .setS3ProfileName(s3Config.getS3ProfileName()) Review Comment: these kind of attribute copy blocks of code are easy to mess up by accidental typos... Could you add a unit test for this conversion? ########## polaris-core/src/main/java/org/apache/polaris/core/storage/s3compatible/S3CompatibleCredentialsStorageIntegration.java: ########## @@ -0,0 +1,123 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.polaris.core.storage.s3compatible; + +import static org.apache.polaris.core.config.FeatureConfiguration.STORAGE_CREDENTIAL_DURATION_SECONDS; +import static org.apache.polaris.core.config.PolarisConfiguration.loadConfig; +import static org.apache.polaris.core.storage.PolarisCredentialProperty.AWS_KEY_ID; +import static org.apache.polaris.core.storage.PolarisCredentialProperty.AWS_SECRET_KEY; +import static org.apache.polaris.core.storage.PolarisCredentialProperty.AWS_TOKEN; + +import jakarta.annotation.Nonnull; +import jakarta.ws.rs.NotAuthorizedException; +import java.net.URI; +import java.util.EnumMap; +import java.util.Set; +import org.apache.polaris.core.PolarisDiagnostics; +import org.apache.polaris.core.storage.InMemoryStorageIntegration; +import org.apache.polaris.core.storage.PolarisCredentialProperty; +import org.apache.polaris.core.storage.StorageUtil; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import software.amazon.awssdk.auth.credentials.AwsBasicCredentials; +import software.amazon.awssdk.auth.credentials.ProfileCredentialsProvider; +import software.amazon.awssdk.auth.credentials.StaticCredentialsProvider; +import software.amazon.awssdk.profiles.ProfileFileSupplier; +import software.amazon.awssdk.services.sts.StsClient; +import software.amazon.awssdk.services.sts.StsClientBuilder; +import software.amazon.awssdk.services.sts.model.AssumeRoleRequest; +import software.amazon.awssdk.services.sts.model.AssumeRoleResponse; + +/** S3 compatible implementation of PolarisStorageIntegration */ +public class S3CompatibleCredentialsStorageIntegration + extends InMemoryStorageIntegration<S3CompatibleStorageConfigurationInfo> { + + private static final Logger LOGGER = + LoggerFactory.getLogger(S3CompatibleCredentialsStorageIntegration.class); + + public S3CompatibleCredentialsStorageIntegration() { + super(S3CompatibleCredentialsStorageIntegration.class.getName()); + } + + @Override + public EnumMap<PolarisCredentialProperty, String> getSubscopedCreds( + @Nonnull PolarisDiagnostics diagnostics, + @Nonnull S3CompatibleStorageConfigurationInfo storageConfig, + boolean allowListOperation, + @Nonnull Set<String> allowedReadLocations, + @Nonnull Set<String> allowedWriteLocations) { + + String caI = System.getenv(storageConfig.getS3CredentialsCatalogAccessKeyId()); + String caS = System.getenv(storageConfig.getS3CredentialsCatalogSecretAccessKey()); + + EnumMap<PolarisCredentialProperty, String> propertiesMap = + new EnumMap<>(PolarisCredentialProperty.class); + propertiesMap.put(PolarisCredentialProperty.AWS_ENDPOINT, storageConfig.getS3Endpoint()); + propertiesMap.put( + PolarisCredentialProperty.AWS_PATH_STYLE_ACCESS, + storageConfig.getS3PathStyleAccess().toString()); + if (storageConfig.getS3Region() != null) { + propertiesMap.put(PolarisCredentialProperty.CLIENT_REGION, storageConfig.getS3Region()); + } + + LOGGER.debug("S3Compatible - createStsClient()"); + StsClientBuilder stsBuilder = StsClient.builder(); + stsBuilder.endpointOverride(URI.create(storageConfig.getS3Endpoint())); + if (storageConfig.getS3ProfileName() != null) { + stsBuilder.credentialsProvider( + ProfileCredentialsProvider.builder() + .profileFile(ProfileFileSupplier.defaultSupplier()) + .profileName(storageConfig.getS3ProfileName()) + .build()); + LOGGER.debug("S3Compatible - stsClient using profile from catalog settings"); + } else if (caI != null && caS != null) { + stsBuilder.credentialsProvider( + StaticCredentialsProvider.create(AwsBasicCredentials.create(caI, caS))); + LOGGER.debug("S3Compatible - stsClient using keys from catalog settings"); + } + try (StsClient stsClient = stsBuilder.build()) { Review Comment: Creating an StsClient for each request is an overkill, IMHO, but we can probably refactor that later. ########## polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java: ########## @@ -250,6 +268,21 @@ public Builder setStorageConfigurationInfo( awsConfig.validateArn(awsConfigModel.getRoleArn()); config = awsConfig; break; + + case S3_COMPATIBLE: + S3CompatibleStorageConfigInfo s3ConfigModel = + (S3CompatibleStorageConfigInfo) storageConfigModel; + config = + new S3CompatibleStorageConfigurationInfo( + s3ConfigModel.getS3Endpoint(), + s3ConfigModel.getS3ProfileName(), + s3ConfigModel.getS3CredentialsCatalogAccessKeyEnvVar(), + s3ConfigModel.getS3CredentialsCatalogSecretAccessKeyEnvVar(), + s3ConfigModel.getS3PathStyleAccess(), + s3ConfigModel.getS3Region(), + s3ConfigModel.getS3RoleArn(), + new ArrayList<>(allowedLocations)); Review Comment: nit: why `new` array? ########## regtests/README.md: ########## @@ -40,7 +40,7 @@ follows: ```shell ./gradlew clean :polaris-quarkus-server:assemble -Dquarkus.container-image.build=true --no-build-cache -docker compose -f ./regtests/docker-compose.yml up --build --exit-code-from regtest +docker-compose -f ./regtests/docker-compose.yml up --build --exit-code-from regtest Review Comment: This seems to be unrelated to the purpose of this PR. Please make this change in a separate PR. ########## polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageConfigurationInfo.java: ########## @@ -241,6 +243,7 @@ public void validateMaxAllowedLocations(int maxAllowedLocations) { /** Polaris' storage type, each has a fixed prefix for its location */ public enum StorageType { S3("s3://"), + S3_COMPATIBLE("s3://"), Review Comment: I believe `s3` is sufficient for this PR because AWS also maps to only `s3`. Other URIs schemes (like `s3a`) are applicable to AWS too, but I think it is best to handle that in a follow-up PR. ########## polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java: ########## @@ -62,4 +67,136 @@ public class StorageUtil { public static @Nonnull String getBucket(URI uri) { return uri.getAuthority(); } + + /** + * Given a path, return it without leading slash + * + * @param path A path to parse + * @return Same path without leading slash + */ + private static @Nonnull String trimLeadingSlash(String path) { + if (path.startsWith("/")) { + path = path.substring(1); + } + return path; + } + + /** + * Given an uri, and format an S3 path + * + * @param uri A path to parse + * @return A bucket and a path joined by slash + */ + private static @Nonnull String parseS3Path(URI uri) { + String bucket = getBucket(uri); + String path = trimLeadingSlash(uri.getPath()); + return String.join("/", bucket, path); + } + + /** + * Given a roleArn, return the prefix + * + * @param roleArn A roleArn to parse + * @return The prefix of the roleArn + */ + private static String getArnPrefixFor(String roleArn) { + if (roleArn.contains("aws-cn")) { + return "arn:aws-cn:s3:::"; + } else if (roleArn.contains("aws-us-gov")) { + return "arn:aws-us-gov:s3:::"; + } else { + return "arn:aws:s3:::"; + } + } + + /** + * generate an IamPolicy from the input readLocations and writeLocations, optionally with list + * support. Credentials will be scoped to exactly the resources provided. If read and write + * locations are empty, a non-empty policy will be generated that grants GetObject and optionally + * ListBucket privileges with no resources. This prevents us from sending an empty policy to AWS + * and just assuming the role with full privileges. + * + * @param roleArn A roleArn + * @param allowList Allow list or not + * @param readLocations A list of input read locations + * @param writeLocations A list of input write locations + * @return A policy limiting scope access + */ + // TODO - add KMS key access + public static IamPolicy policyString( Review Comment: Why not reuse the policy from the code the deals with AWS S3? ########## polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java: ########## @@ -62,4 +67,136 @@ public class StorageUtil { public static @Nonnull String getBucket(URI uri) { return uri.getAuthority(); } + + /** + * Given a path, return it without leading slash + * + * @param path A path to parse + * @return Same path without leading slash + */ + private static @Nonnull String trimLeadingSlash(String path) { + if (path.startsWith("/")) { + path = path.substring(1); + } + return path; + } + + /** + * Given an uri, and format an S3 path + * + * @param uri A path to parse + * @return A bucket and a path joined by slash + */ + private static @Nonnull String parseS3Path(URI uri) { + String bucket = getBucket(uri); + String path = trimLeadingSlash(uri.getPath()); + return String.join("/", bucket, path); + } + + /** + * Given a roleArn, return the prefix + * + * @param roleArn A roleArn to parse + * @return The prefix of the roleArn + */ + private static String getArnPrefixFor(String roleArn) { + if (roleArn.contains("aws-cn")) { + return "arn:aws-cn:s3:::"; + } else if (roleArn.contains("aws-us-gov")) { + return "arn:aws-us-gov:s3:::"; Review Comment: I believe this kind of special cases should not be in Polaris code, but it's ok for this PR as I understand it mimics previous AWS-specific logic. ########## regtests/minio/docker-compose.yml: ########## @@ -0,0 +1,66 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +# + +services: + polaris-minio: + image: minio/minio:latest + container_name: minio + environment: + - MINIO_ROOT_USER=admin + - MINIO_ROOT_PASSWORD=password + - MINIO_DOMAIN=minio + networks: + minio_net: + aliases: + - warehouse.minio + - warehouse2.minio + ports: + - 9001:9001 + - 9000:9000 + volumes: + - ./miniodata:/data + - ./certs:/root/.minio/certs/ Review Comment: Is this strictly necessary for tests? -- 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]
