dimas-b commented on code in PR #2422:
URL: https://github.com/apache/polaris/pull/2422#discussion_r2292300611
##########
polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java:
##########
@@ -100,6 +100,16 @@ public static void enforceFeatureEnabledOrThrow(
.defaultValue(false)
.buildFeatureConfiguration();
+ public static final FeatureConfiguration<Boolean>
ALLOW_NAMESPACE_LOCATION_ESCAPE =
+ PolarisConfiguration.<Boolean>builder()
+ .key("ALLOW_NAMESPACE_LOCATION_ESCAPE")
Review Comment:
Feature flags
[constitute](https://polaris.apache.org/in-dev/unreleased/evolution/) our
SemVer "API" surface, so I believe we need to formally send a discussion email
to the `dev` ML about this flag.
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -1083,6 +1093,65 @@ private void validateNoLocationOverlap(
}
}
+ /** Checks whether the location of a namespace is valid given its parent */
+ private void validateNamespaceLocation(
+ NamespaceEntity namespace, PolarisResolvedPathWrapper resolvedParent) {
+ StorageLocation namespaceLocation =
+ StorageLocation.of(
+ resolveNamespaceLocation(namespace.asNamespace(),
namespace.getPropertiesAsMap()));
+ PolarisEntity parent = resolvedParent.getResolvedLeafEntity().getEntity();
+ if (parent.getType().equals(PolarisEntityType.CATALOG)) {
+ CatalogEntity parentEntity = CatalogEntity.of(parent);
+ LOGGER.debug(
+ "Validating namespace {} given parent catalog {}",
+ namespace.getName(),
+ parentEntity.getName());
+ var storageConfigInfo = parentEntity.getStorageConfigurationInfo();
+ if (storageConfigInfo == null) {
+ throw new IllegalArgumentException(
+ "Cannot create namespace without a parent storage configuration");
+ }
+ System.out.println("#### Validating " + namespace.getName());
Review Comment:
oops, `System.out`?
##########
polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java:
##########
@@ -100,6 +100,16 @@ public static void enforceFeatureEnabledOrThrow(
.defaultValue(false)
.buildFeatureConfiguration();
+ public static final FeatureConfiguration<Boolean>
ALLOW_NAMESPACE_LOCATION_ESCAPE =
+ PolarisConfiguration.<Boolean>builder()
+ .key("ALLOW_NAMESPACE_LOCATION_ESCAPE")
Review Comment:
Proactively +1 from my side :)
##########
integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisApplicationIntegrationTest.java:
##########
@@ -641,4 +666,53 @@ public void testRequestBodyTooLarge() throws Exception {
});
}
}
+
+ @ParameterizedTest
+ @ValueSource(booleans = {true, false})
+ public void testNamespaceOutsideCatalog(boolean
allowNamespaceLocationEscape) throws IOException {
+ String catalogName =
client.newEntityName("testNamespaceOutsideCatalog_specificLocation");
+ String catalogLocation = baseLocation.resolve(catalogName +
"/catalog").toString();
+ String badLocation = baseLocation.resolve(catalogName + "/ns").toString();
+ createCatalog(
+ catalogName,
+ Catalog.TypeEnum.INTERNAL,
+ principalRoleName,
+ FileStorageConfigInfo.builder(StorageConfigInfo.StorageTypeEnum.FILE)
+ .setAllowedLocations(List.of(catalogLocation))
+ .build(),
+ catalogLocation,
+ ImmutableMap.of(
+
FeatureConfiguration.ALLOW_NAMESPACE_LOCATION_ESCAPE.catalogConfig(),
+ String.valueOf(allowNamespaceLocationEscape)));
+ try (RESTSessionCatalog sessionCatalog = newSessionCatalog(catalogName)) {
+ SessionCatalog.SessionContext sessionContext =
SessionCatalog.SessionContext.createEmpty();
+ sessionCatalog.createNamespace(sessionContext,
Namespace.of("good_namespace"));
+ ThrowableAssert.ThrowingCallable createBadNamespace =
+ () ->
+ sessionCatalog.createNamespace(
+ sessionContext,
+ Namespace.of("bad_namespace"),
+ ImmutableMap.of("location", badLocation));
+ if (!allowNamespaceLocationEscape) {
+ assertThatThrownBy(createBadNamespace)
+ .isInstanceOf(BadRequestException.class)
+ .hasMessageContaining("location");
+ } else {
+ assertThatCode(createBadNamespace).doesNotThrowAnyException();
+ }
+ ThrowableAssert.ThrowingCallable createBadChildGoodParent =
+ () ->
+ sessionCatalog.createNamespace(
+ sessionContext,
+ Namespace.of("good_namespace", "bad_child"),
Review Comment:
Could you add a test where one of the parent namespaces has a location
property set too (not just at the catalog level)?
--
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: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
