adutra commented on code in PR #282:
URL: https://github.com/apache/polaris/pull/282#discussion_r1760684103
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java:
##########
@@ -18,104 +18,98 @@
*/
package org.apache.polaris.core.storage.aws;
-import com.fasterxml.jackson.annotation.JsonCreator;
-import com.fasterxml.jackson.annotation.JsonIgnore;
-import com.fasterxml.jackson.annotation.JsonProperty;
-import com.google.common.base.MoreObjects;
+import com.fasterxml.jackson.annotation.JsonTypeName;
+import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
+import com.fasterxml.jackson.databind.annotation.JsonSerialize;
import java.util.List;
+import java.util.OptionalInt;
+import java.util.Set;
import java.util.regex.Pattern;
import org.apache.polaris.core.storage.PolarisStorageConfigurationInfo;
-import org.jetbrains.annotations.NotNull;
+import org.apache.polaris.immutables.PolarisImmutable;
+import org.immutables.value.Value;
+import org.immutables.value.Value.Check;
import org.jetbrains.annotations.Nullable;
/** Aws Polaris Storage Configuration information */
-public class AwsStorageConfigurationInfo extends
PolarisStorageConfigurationInfo {
+@PolarisImmutable
+@JsonSerialize(as = ImmutableAwsStorageConfigurationInfo.class)
+@JsonDeserialize(as = ImmutableAwsStorageConfigurationInfo.class)
+@JsonTypeName("AwsStorageConfigurationInfo")
+public abstract class AwsStorageConfigurationInfo extends
PolarisStorageConfigurationInfo {
// 5 is the approximate max allowed locations for the size of AccessPolicy
when LIST is required
// for allowed read and write locations for subscoping creds.
- @JsonIgnore private static final int MAX_ALLOWED_LOCATIONS = 5;
+ private static final int MAX_ALLOWED_LOCATIONS = 5;
- // Technically, it should be
^arn:(aws|aws-cn|aws-us-gov):iam::\d{12}:role/.+$,
- @JsonIgnore public static final String ROLE_ARN_PATTERN =
"^arn:aws:iam::\\d{12}:role/.+$";
+ private static final String ROLE_ARN_PATTERN =
+ "^arn:(aws|aws-cn|aws-us-gov):iam::\\d{12}:role/.+$";
- // AWS role to be assumed
- private final @NotNull String roleARN;
-
- // AWS external ID, optional
- @JsonProperty(value = "externalId", required = false)
- private @Nullable String externalId = null;
-
- /** User ARN for the service principal */
- @JsonProperty(value = "userARN", required = false)
- private @Nullable String userARN = null;
+ public static AwsStorageConfigurationInfo of(Iterable<String>
allowedLocations, String roleARN) {
+ return of(allowedLocations, roleARN, null);
+ }
- @JsonCreator
- public AwsStorageConfigurationInfo(
- @JsonProperty(value = "storageType", required = true) @NotNull
StorageType storageType,
- @JsonProperty(value = "allowedLocations", required = true) @NotNull
- List<String> allowedLocations,
- @JsonProperty(value = "roleARN", required = true) @NotNull String
roleARN) {
- this(storageType, allowedLocations, roleARN, null);
+ public static AwsStorageConfigurationInfo of(
+ Iterable<String> allowedLocations, String roleARN, @Nullable String
externalId) {
+ return ImmutableAwsStorageConfigurationInfo.builder()
+ .allowedLocations(allowedLocations)
+ .roleARN(roleARN)
+ .externalId(externalId)
+ .build();
}
- public AwsStorageConfigurationInfo(
- @NotNull StorageType storageType,
- @NotNull List<String> allowedLocations,
- @NotNull String roleARN,
- @Nullable String externalId) {
- super(storageType, allowedLocations);
- this.roleARN = roleARN;
- this.externalId = externalId;
- validateMaxAllowedLocations(MAX_ALLOWED_LOCATIONS);
+ @Override
+ public abstract List<String> getAllowedLocations();
+
+ @Value.Default
+ @Override
+ public StorageType getStorageType() {
+ return StorageType.S3;
}
+ @Value.Default
@Override
public String getFileIoImplClassName() {
return "org.apache.iceberg.aws.s3.S3FileIO";
}
- public void validateArn(String arn) {
- if (arn == null || arn.isEmpty()) {
- throw new IllegalArgumentException("ARN cannot be null or empty");
- }
- // specifically throw errors for China and Gov
- if (arn.contains("aws-cn") || arn.contains("aws-us-gov")) {
- throw new IllegalArgumentException("AWS China or Gov Cloud are
temporarily not supported");
- }
- if (!Pattern.matches(ROLE_ARN_PATTERN, arn)) {
- throw new IllegalArgumentException("Invalid role ARN format");
- }
- }
+ /** AWS role to be assumed. */
+ public abstract String getRoleARN();
- public @NotNull String getRoleARN() {
- return roleARN;
- }
+ /** AWS external ID, optional. */
+ @Nullable
+ public abstract String getExternalId();
- public @Nullable String getExternalId() {
- return externalId;
- }
+ @Nullable
+ public abstract String getUserARN();
- public void setExternalId(String externalId) {
- this.externalId = externalId;
+ @Value.Default
+ protected Set<String> getAllowedArnDomains() {
Review Comment:
I think I need to revert the `ConfigurationInfo` changes then. I don't see
any other way to keep the current behavior of rejecting China/USGov while still
allowing it in 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]
