dimas-b commented on code in PR #4422:
URL: https://github.com/apache/polaris/pull/4422#discussion_r3377454628
##########
runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractIcebergCatalogViewTest.java:
##########
@@ -193,7 +193,9 @@ public void before(TestInfo testInfo) {
.setStorageConfigurationInfo(
realmConfig,
new FileStorageConfigInfo(
- StorageConfigInfo.StorageTypeEnum.FILE,
List.of("file://", "/", "*"), null),
+ StorageConfigInfo.StorageTypeEnum.FILE,
+ List.of("file://tmp", "file://", "/", "*"),
Review Comment:
If `file://` is allowed, does it not subsume `file://tmp`?
##########
runtime/service/src/intTest/java/org/apache/polaris/service/it/RestCatalogMinIOSpecialIT.java:
##########
@@ -219,7 +219,7 @@ private RESTCatalog createCatalog(
.setStorageType(StorageConfigInfo.StorageTypeEnum.S3)
.setPathStyleAccess(pathStyleAccess)
.setStsUnavailable(!stsEnabled)
- .setAllowedLocations(List.of(storageBase.toString()));
+ .setAllowedLocations(List.of(storageBase.toASCIIString() + "/" +
catalogName));
Review Comment:
Why `toASCIIString()`?
##########
runtime/service/src/test/java/org/apache/polaris/service/admin/BaseLocationValidationTest.java:
##########
@@ -0,0 +1,96 @@
+/*
+ * 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.admin;
+
+import static org.assertj.core.api.Assertions.assertThatCode;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.util.List;
+import org.apache.iceberg.exceptions.BadRequestException;
+import org.apache.polaris.core.entity.CatalogEntity;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.CsvSource;
+
+/**
+ * Tests for {@link CatalogEntity#validateBaseLocationAgainstAllowedList}, the
helper used by {@code
+ * updateCatalog} (to validate the effective default-base-location against the
effective
+ * allowed-locations) and by {@code
CatalogEntity.Builder.setStorageConfigurationInfo} (to validate
+ * the user-supplied allowed-locations on create / storage-config replacement).
+ */
+public class BaseLocationValidationTest {
Review Comment:
Since these are unit tests for `CatalogEntity` code, why not add them to
`CatalogEntityTest`?
##########
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -911,11 +911,39 @@ private void validateUpdateCatalogDiffOrThrow(
// so we must restore it explicitly in the updateBuilder.
updateBuilder.setDefaultBaseLocation(defaultBaseLocation);
} else {
- // New base location is already in the updated properties; we'll also
potentially
- // plumb it into the logic for setting an updated
StorageConfigurationInfo.
defaultBaseLocation = newDefaultBaseLocation;
}
}
+
+ // Base-location validity is an invariant on every update. Update-side
semantics are strict:
+ // unlike CatalogEntity.Builder.setStorageConfigurationInfo (which has a
create-time
+ // convenience that auto-populates allowed-locations from
default-base-location when the
+ // caller supplied none), updates that touch storage_config must declare
allowed-locations
+ // explicitly. This prevents silent loss of an existing allowed-locations
list during a
+ // partial storage_config update (e.g. role rotation).
+ if (defaultBaseLocation != null) {
+ if (updateRequest.getStorageConfigInfo() != null) {
+ List<String> submittedAllowedLocations =
+ updateRequest.getStorageConfigInfo().getAllowedLocations();
+ if (submittedAllowedLocations == null ||
submittedAllowedLocations.isEmpty()) {
+ throw new BadRequestException(
+ "Cannot update Catalog %s: when updating storage_config,
allowed-locations must"
+ + " be specified explicitly (omitting it would silently lose
the catalog's"
+ + " existing allowed-locations list)",
+ name);
+ }
+ CatalogEntity.validateBaseLocationAgainstAllowedList(
Review Comment:
This will be called inside `setStorageConfigurationInfo()` on line 948, will
it not?
TBH, I find the multiple validate sites quite confusing. Would it be
possible to consolidate all validation inside the `CatalogEntity.Builder`? It
is supposed to process all data anyway.
If data is injected in pieces, perhaps we could have a dedicated
(non-static) validation method there... 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]
