adnanhemani commented on code in PR #4023:
URL: https://github.com/apache/polaris/pull/4023#discussion_r2979657287
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -755,8 +806,19 @@ public boolean removeProperties(Namespace namespace,
Set<String> properties)
Map<String, String> updatedProperties = new
HashMap<>(entity.getPropertiesAsMap());
properties.forEach(updatedProperties::remove);
- PolarisEntity updatedEntity =
- new
PolarisEntity.Builder(entity).setProperties(updatedProperties).build();
+ PolarisEntity.Builder updatedEntityBuilder =
+ new PolarisEntity.Builder(entity).setProperties(updatedProperties);
+
+ // If polaris.storage.name is being removed, clear the storage config
override
+ if (properties.contains(POLARIS_STORAGE_NAME_PROPERTY)) {
+ LOGGER.debug("Clearing storage name override on namespace {}",
namespace);
+ Map<String, String> updatedInternalProperties =
+ new HashMap<>(entity.getInternalPropertiesAsMap());
+
updatedInternalProperties.remove(PolarisEntityConstants.getStorageConfigInfoPropertyName());
Review Comment:
could it be potentially easier to just transform
`POLARIS_STORAGE_NAME_PROPERTY` into
`PolarisEntityConstants.getStorageConfigInfoPropertyName()` prior to removing
all the properties the first time?
##########
polaris-core/src/test/java/org/apache/polaris/core/storage/WithStorageNameTest.java:
##########
@@ -0,0 +1,171 @@
+/*
+ * 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;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.List;
+import org.apache.polaris.core.storage.aws.AwsStorageConfigurationInfo;
+import org.apache.polaris.core.storage.azure.AzureStorageConfigurationInfo;
+import org.apache.polaris.core.storage.gcp.GcpStorageConfigurationInfo;
+import org.junit.jupiter.api.Test;
+
+class WithStorageNameTest {
+
+ @Test
+ void awsFieldsPreserved() {
+ AwsStorageConfigurationInfo base =
+ AwsStorageConfigurationInfo.builder()
+ .allowedLocations(List.of("s3://bucket/path"))
+ .storageName("original")
+ .roleARN("arn:aws:iam::123456789012:role/test-role")
+ .externalId("ext-id-123")
+ .region("us-west-2")
+ .endpoint("https://s3.example.com";)
+ .endpointInternal("https://s3-internal.example.com";)
+ .stsEndpoint("https://sts.example.com";)
+ .pathStyleAccess(true)
+ .stsUnavailable(true)
+ .kmsUnavailable(false)
+ .currentKmsKey("arn:aws:kms:us-west-2:123456789012:key/test")
+
.allowedKmsKeys(List.of("arn:aws:kms:us-west-2:123456789012:key/test"))
+ .build();
+
+ PolarisStorageConfigurationInfo result =
+ PolarisStorageConfigurationInfo.withStorageName(base, "new-storage");
+
+ assertThat(result).isInstanceOf(AwsStorageConfigurationInfo.class);
+ AwsStorageConfigurationInfo awsResult = (AwsStorageConfigurationInfo)
result;
+ assertThat(awsResult.getStorageName()).isEqualTo("new-storage");
+
assertThat(awsResult.getAllowedLocations()).containsExactly("s3://bucket/path");
+
assertThat(awsResult.getRoleARN()).isEqualTo("arn:aws:iam::123456789012:role/test-role");
+ assertThat(awsResult.getExternalId()).isEqualTo("ext-id-123");
+ assertThat(awsResult.getRegion()).isEqualTo("us-west-2");
+ assertThat(awsResult.getEndpoint()).isEqualTo("https://s3.example.com";);
+
assertThat(awsResult.getEndpointInternal()).isEqualTo("https://s3-internal.example.com";);
+
assertThat(awsResult.getStsEndpoint()).isEqualTo("https://sts.example.com";);
+ assertThat(awsResult.getPathStyleAccess()).isTrue();
+ assertThat(awsResult.getStsUnavailable()).isTrue();
+ assertThat(awsResult.getKmsUnavailable()).isFalse();
+ assertThat(awsResult.getCurrentKmsKey())
+ .isEqualTo("arn:aws:kms:us-west-2:123456789012:key/test");
+ assertThat(awsResult.getAllowedKmsKeys())
+ .containsExactly("arn:aws:kms:us-west-2:123456789012:key/test");
+ }
+
+ @Test
+ void awsNullStorageNameClearsOverride() {
+ AwsStorageConfigurationInfo base =
+ AwsStorageConfigurationInfo.builder()
+ .allowedLocations(List.of("s3://bucket/path"))
+ .storageName("original")
+ .roleARN("arn:aws:iam::123456789012:role/test-role")
+ .build();
+
+ PolarisStorageConfigurationInfo result =
+ PolarisStorageConfigurationInfo.withStorageName(base, null);
+
+ assertThat(result.getStorageName()).isNull();
+
assertThat(result.getAllowedLocations()).containsExactly("s3://bucket/path");
+ }
+
+ @Test
+ void azureFieldsPreserved() {
+ AzureStorageConfigurationInfo base =
+ AzureStorageConfigurationInfo.builder()
+
.allowedLocations(List.of("abfss://[email protected]/path"))
+ .storageName("original")
+ .tenantId("tenant-123")
+ .multiTenantAppName("my-app")
+ .consentUrl("https://consent.example.com";)
+ .hierarchical(true)
+ .build();
+
+ PolarisStorageConfigurationInfo result =
+ PolarisStorageConfigurationInfo.withStorageName(base, "azure-storage");
+
+ assertThat(result).isInstanceOf(AzureStorageConfigurationInfo.class);
+ AzureStorageConfigurationInfo azureResult =
(AzureStorageConfigurationInfo) result;
+ assertThat(azureResult.getStorageName()).isEqualTo("azure-storage");
+ assertThat(azureResult.getAllowedLocations())
+
.containsExactly("abfss://[email protected]/path");
+ assertThat(azureResult.getTenantId()).isEqualTo("tenant-123");
+ assertThat(azureResult.getMultiTenantAppName()).isEqualTo("my-app");
+
assertThat(azureResult.getConsentUrl()).isEqualTo("https://consent.example.com";);
+ assertThat(azureResult.isHierarchical()).isTrue();
+ }
+
+ @Test
+ void gcpFieldsPreserved() {
+ GcpStorageConfigurationInfo base =
+ GcpStorageConfigurationInfo.builder()
+ .allowedLocations(List.of("gs://bucket/path"))
+ .storageName("original")
+ .gcpServiceAccount("[email protected]")
+ .build();
+
+ PolarisStorageConfigurationInfo result =
+ PolarisStorageConfigurationInfo.withStorageName(base, "gcp-storage");
+
+ assertThat(result).isInstanceOf(GcpStorageConfigurationInfo.class);
+ GcpStorageConfigurationInfo gcpResult = (GcpStorageConfigurationInfo)
result;
+ assertThat(gcpResult.getStorageName()).isEqualTo("gcp-storage");
+
assertThat(gcpResult.getAllowedLocations()).containsExactly("gs://bucket/path");
+
assertThat(gcpResult.getGcpServiceAccount()).isEqualTo("[email protected]");
+ }
+
+ @Test
+ void fileFieldsPreserved() {
+ FileStorageConfigurationInfo base =
+ FileStorageConfigurationInfo.builder()
+ .allowedLocations(List.of("file:///tmp/warehouse"))
+ .storageName("original")
+ .build();
+
+ PolarisStorageConfigurationInfo result =
+ PolarisStorageConfigurationInfo.withStorageName(base, "file-storage");
+
+ assertThat(result).isInstanceOf(FileStorageConfigurationInfo.class);
+ assertThat(result.getStorageName()).isEqualTo("file-storage");
+
assertThat(result.getAllowedLocations()).containsExactly("file:///tmp/warehouse");
+ }
+
+ @Test
+ void serializationRoundTrip() {
Review Comment:
what is this test trying to test? That an overriden
`PolarisStorageConfigurationInfo` can be serialized/deserialized? Is there any
custom logic that puts this into doubt?
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/StorageNameValidator.java:
##########
@@ -0,0 +1,78 @@
+/*
+ * 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;
+
+import jakarta.annotation.Nullable;
+import java.util.regex.Pattern;
+
+/**
+ * Validates storage name references used in {@code polaris.storage.name}
property overrides.
+ *
+ * <p>Storage names are symbolic references to server-side credential
configurations (e.g. {@code
+ * polaris.storage.aws.<storageName>.access-key}). They must be valid Quarkus
config key segments.
+ */
+public final class StorageNameValidator {
+
+ /** Maximum length for a storage name. */
+ public static final int MAX_LENGTH = 128;
+
+ /** Alphanumeric characters, hyphens, and underscores only. */
+ private static final Pattern VALID_PATTERN =
Pattern.compile("^[a-zA-Z0-9_-]+$");
+
+ private StorageNameValidator() {}
+
+ /**
+ * Validates a storage name format.
+ *
+ * @param storageName the name to validate
+ * @throws IllegalArgumentException if the name is invalid
+ */
+ public static void validate(String storageName) {
+ if (storageName == null || storageName.isEmpty()) {
+ throw new IllegalArgumentException("Storage name must not be null or
empty");
+ }
+ if (storageName.length() > MAX_LENGTH) {
+ throw new IllegalArgumentException(
+ String.format(
+ "Storage name must not exceed %d characters, got %d",
+ MAX_LENGTH, storageName.length()));
+ }
+ if (!VALID_PATTERN.matcher(storageName).matches()) {
+ throw new IllegalArgumentException(
+ String.format(
+ "Storage name '%s' contains invalid characters. "
+ + "Only alphanumeric characters, hyphens, and underscores
are allowed.",
+ storageName));
+ }
+ }
+
+ /**
+ * Normalizes a blank or empty string to {@code null}. Non-blank values are
trimmed.
+ *
+ * @param value the input value
+ * @return {@code null} if the input is null, empty, or blank; otherwise the
trimmed value
+ */
+ public static @Nullable String normalizeBlankToNull(@Nullable String value) {
Review Comment:
Is there a reason this shouldn't be called from within `validate`?
##########
runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/StorageNameOverrideTest.java:
##########
@@ -0,0 +1,309 @@
+/*
+ * 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.service.catalog.iceberg;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import org.apache.iceberg.exceptions.BadRequestException;
+import org.apache.polaris.core.PolarisDiagnostics;
+import org.apache.polaris.core.auth.PolarisPrincipal;
+import org.apache.polaris.core.config.FeatureConfiguration;
+import org.apache.polaris.core.config.RealmConfig;
+import org.apache.polaris.core.context.CallContext;
+import org.apache.polaris.core.entity.CatalogEntity;
+import org.apache.polaris.core.entity.PolarisEntity;
+import org.apache.polaris.core.entity.PolarisEntityConstants;
+import org.apache.polaris.core.entity.PolarisEntitySubType;
+import org.apache.polaris.core.entity.PolarisEntityType;
+import org.apache.polaris.core.persistence.PolarisMetaStoreManager;
+import
org.apache.polaris.core.persistence.resolver.PolarisResolutionManifestCatalogView;
+import org.apache.polaris.core.persistence.resolver.ResolverFactory;
+import org.apache.polaris.core.storage.FileStorageConfigurationInfo;
+import org.apache.polaris.core.storage.PolarisStorageConfigurationInfo;
+import org.apache.polaris.core.storage.aws.AwsStorageConfigurationInfo;
+import org.apache.polaris.service.catalog.io.FileIOFactory;
+import org.apache.polaris.service.catalog.io.FileIOUtil;
+import org.apache.polaris.service.catalog.io.StorageAccessConfigProvider;
+import org.apache.polaris.service.events.PolarisEventMetadataFactory;
+import org.apache.polaris.service.events.listeners.PolarisEventListener;
+import org.apache.polaris.service.task.TaskExecutor;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Unit tests for the static helper methods in {@link IcebergCatalog} that
handle {@code
+ * polaris.storage.name} property overrides. These test the pure logic without
requiring a full
+ * Quarkus test context.
+ */
+class StorageNameOverrideTest {
+
+ private static final String STORAGE_CONFIG_KEY =
+ PolarisEntityConstants.getStorageConfigInfoPropertyName();
+
+ @Test
+ void storageConfigFromPropertyOverride_noProperty_returnsNull() throws
Exception {
+ // Use reflection since the method is package-private/static
+ var method =
Review Comment:
really not a fan of reflection...
would much rather prefer doing `@VisibleForTest` on the method.
##########
polaris-core/src/test/java/org/apache/polaris/core/storage/WithStorageNameTest.java:
##########
@@ -0,0 +1,171 @@
+/*
+ * 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;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.List;
+import org.apache.polaris.core.storage.aws.AwsStorageConfigurationInfo;
+import org.apache.polaris.core.storage.azure.AzureStorageConfigurationInfo;
+import org.apache.polaris.core.storage.gcp.GcpStorageConfigurationInfo;
+import org.junit.jupiter.api.Test;
+
+class WithStorageNameTest {
+
+ @Test
+ void awsFieldsPreserved() {
+ AwsStorageConfigurationInfo base =
+ AwsStorageConfigurationInfo.builder()
+ .allowedLocations(List.of("s3://bucket/path"))
Review Comment:
nit: would prefer if you pulled all the variables out and referred to them
twice. It would make the test more readable and maintainable :)
##########
runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/StorageNameOverrideTest.java:
##########
@@ -0,0 +1,309 @@
+/*
+ * 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.service.catalog.iceberg;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import org.apache.iceberg.exceptions.BadRequestException;
+import org.apache.polaris.core.PolarisDiagnostics;
+import org.apache.polaris.core.auth.PolarisPrincipal;
+import org.apache.polaris.core.config.FeatureConfiguration;
+import org.apache.polaris.core.config.RealmConfig;
+import org.apache.polaris.core.context.CallContext;
+import org.apache.polaris.core.entity.CatalogEntity;
+import org.apache.polaris.core.entity.PolarisEntity;
+import org.apache.polaris.core.entity.PolarisEntityConstants;
+import org.apache.polaris.core.entity.PolarisEntitySubType;
+import org.apache.polaris.core.entity.PolarisEntityType;
+import org.apache.polaris.core.persistence.PolarisMetaStoreManager;
+import
org.apache.polaris.core.persistence.resolver.PolarisResolutionManifestCatalogView;
+import org.apache.polaris.core.persistence.resolver.ResolverFactory;
+import org.apache.polaris.core.storage.FileStorageConfigurationInfo;
+import org.apache.polaris.core.storage.PolarisStorageConfigurationInfo;
+import org.apache.polaris.core.storage.aws.AwsStorageConfigurationInfo;
+import org.apache.polaris.service.catalog.io.FileIOFactory;
+import org.apache.polaris.service.catalog.io.FileIOUtil;
+import org.apache.polaris.service.catalog.io.StorageAccessConfigProvider;
+import org.apache.polaris.service.events.PolarisEventMetadataFactory;
+import org.apache.polaris.service.events.listeners.PolarisEventListener;
+import org.apache.polaris.service.task.TaskExecutor;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Unit tests for the static helper methods in {@link IcebergCatalog} that
handle {@code
+ * polaris.storage.name} property overrides. These test the pure logic without
requiring a full
+ * Quarkus test context.
+ */
+class StorageNameOverrideTest {
+
+ private static final String STORAGE_CONFIG_KEY =
+ PolarisEntityConstants.getStorageConfigInfoPropertyName();
+
+ @Test
+ void storageConfigFromPropertyOverride_noProperty_returnsNull() throws
Exception {
+ // Use reflection since the method is package-private/static
+ var method =
+ IcebergCatalog.class.getDeclaredMethod(
+ "storageConfigFromPropertyOverride", Map.class, List.class);
+ method.setAccessible(true);
+
+ PolarisStorageConfigurationInfo result =
+ (PolarisStorageConfigurationInfo) method.invoke(null, Map.of("other",
"value"), List.of());
+ assertThat(result).isNull();
+ }
+
+ @Test
+ void storageConfigFromPropertyOverride_withValidStorageName() throws
Exception {
+ var method =
+ IcebergCatalog.class.getDeclaredMethod(
+ "storageConfigFromPropertyOverride", Map.class, List.class);
+ method.setAccessible(true);
+
+ AwsStorageConfigurationInfo catalogConfig =
+ AwsStorageConfigurationInfo.builder()
+ .allowedLocations(List.of("s3://bucket/path"))
+ .roleARN("arn:aws:iam::123456789012:role/test-role")
+ .storageName("catalog-storage")
+ .build();
+
+ PolarisEntity catalogEntity = createEntityWithStorageConfig(catalogConfig);
+
+ Map<String, String> properties = new HashMap<>();
+ properties.put(IcebergCatalog.POLARIS_STORAGE_NAME_PROPERTY, "ns-storage");
+
+ PolarisStorageConfigurationInfo result =
+ (PolarisStorageConfigurationInfo) method.invoke(null, properties,
List.of(catalogEntity));
+
+ assertThat(result).isNotNull();
+ assertThat(result.getStorageName()).isEqualTo("ns-storage");
+ assertThat(result).isInstanceOf(AwsStorageConfigurationInfo.class);
+ assertThat(((AwsStorageConfigurationInfo) result).getRoleARN())
+ .isEqualTo("arn:aws:iam::123456789012:role/test-role");
+ }
+
+ @Test
+ void storageConfigFromPropertyOverride_blankValue_clearsStorageName() throws
Exception {
+ var method =
+ IcebergCatalog.class.getDeclaredMethod(
+ "storageConfigFromPropertyOverride", Map.class, List.class);
+ method.setAccessible(true);
+
+ AwsStorageConfigurationInfo catalogConfig =
+ AwsStorageConfigurationInfo.builder()
+ .allowedLocations(List.of("s3://bucket/path"))
+ .roleARN("arn:aws:iam::123456789012:role/test-role")
+ .storageName("catalog-storage")
+ .build();
+
+ PolarisEntity catalogEntity = createEntityWithStorageConfig(catalogConfig);
+
+ Map<String, String> properties = new HashMap<>();
+ properties.put(IcebergCatalog.POLARIS_STORAGE_NAME_PROPERTY, " ");
+
+ PolarisStorageConfigurationInfo result =
+ (PolarisStorageConfigurationInfo) method.invoke(null, properties,
List.of(catalogEntity));
+
+ assertThat(result).isNotNull();
+ assertThat(result.getStorageName()).isNull();
+ }
+
+ @Test
+ void storageConfigFromPropertyOverride_noParentConfig_throws() throws
Exception {
+ var method =
+ IcebergCatalog.class.getDeclaredMethod(
+ "storageConfigFromPropertyOverride", Map.class, List.class);
+ method.setAccessible(true);
+
+ Map<String, String> properties = new HashMap<>();
+ properties.put(IcebergCatalog.POLARIS_STORAGE_NAME_PROPERTY, "ns-storage");
+
+ assertThatThrownBy(() -> method.invoke(null, properties, List.of()))
+
.hasCauseInstanceOf(org.apache.iceberg.exceptions.BadRequestException.class)
Review Comment:
no need for the full class path, import instead.
--
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]
