From b650613ec8d019000f0df1b291c5e61c3339224d Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Mon, 5 Aug 2024 15:12:07 -0700 Subject: [PATCH 1/7] wip --- .../io/polaris/core/PolarisConfiguration.java | 159 +++++++++++++++--- .../core/PolarisConfigurationStore.java | 16 ++ .../service/catalog/BasePolarisCatalog.java | 12 ++ .../config/DefaultConfigurationStore.java | 4 + .../service/admin/PolarisAuthzTestBase.java | 2 +- 5 files changed, 172 insertions(+), 21 deletions(-) diff --git a/polaris-core/src/main/java/io/polaris/core/PolarisConfiguration.java b/polaris-core/src/main/java/io/polaris/core/PolarisConfiguration.java index ceefb83ff9..bbbe3e7082 100644 --- a/polaris-core/src/main/java/io/polaris/core/PolarisConfiguration.java +++ b/polaris-core/src/main/java/io/polaris/core/PolarisConfiguration.java @@ -15,30 +15,149 @@ */ package io.polaris.core; -public class PolarisConfiguration { +public class PolarisConfiguration { - public static final String ENFORCE_PRINCIPAL_CREDENTIAL_ROTATION_REQUIRED_CHECKING = - "ENFORCE_PRINCIPAL_CREDENTIAL_ROTATION_REQUIRED_CHECKING"; - public static final String ALLOW_TABLE_LOCATION_OVERLAP = "ALLOW_TABLE_LOCATION_OVERLAP"; - public static final String ALLOW_NAMESPACE_LOCATION_OVERLAP = "ALLOW_NAMESPACE_LOCATION_OVERLAP"; - public static final String ALLOW_EXTERNAL_METADATA_FILE_LOCATION = - "ALLOW_EXTERNAL_METADATA_FILE_LOCATION"; + public final String key; + public final String description; + public final T defaultValue; + public final boolean catalogConfig; - public static final String ALLOW_OVERLAPPING_CATALOG_URLS = "ALLOW_OVERLAPPING_CATALOG_URLS"; + public PolarisConfiguration(String key, String description, T defaultValue, boolean catalogConfig) { + this.key = key; + this.description = description; + this.defaultValue = defaultValue; + this.catalogConfig = catalogConfig; + } - public static final String CATALOG_ALLOW_UNSTRUCTURED_TABLE_LOCATION = - "allow.unstructured.table.location"; - public static final String CATALOG_ALLOW_EXTERNAL_TABLE_LOCATION = - "allow.external.table.location"; + public static class Builder { + private String key; + private String description; + private T defaultValue; + private boolean catalogConfig = false; - /* - * Default values for the configuration properties - */ + public Builder key(String key) { + this.key = key; + return this; + } - public static final boolean DEFAULT_ALLOW_OVERLAPPING_CATALOG_URLS = false; - public static final boolean DEFAULT_ALLOW_TABLE_LOCATION_OVERLAP = false; - public static final boolean DEFAULT_ALLOW_EXTERNAL_METADATA_FILE_LOCATION = false; - public static final boolean DEFAULT_ALLOW_NAMESPACE_LOCATION_OVERLAP = false; + public Builder description(String description) { + this.description = description; + return this; + } - private PolarisConfiguration() {} + public Builder defaultValue(T defaultValue) { + this.defaultValue = defaultValue; + return this; + } + + public Builder catalogConfig(boolean catalogConfig) { + this.catalogConfig = catalogConfig; + return this; + } + + public PolarisConfiguration build() { + if (key == null || description == null || defaultValue == null) { + throw new IllegalArgumentException("key, description, and defaultValue are required"); + } + return new PolarisConfiguration<>(key, description, defaultValue, catalogConfig); + } + } + + public static Builder builder() { + return new Builder<>(); + } + + public static final PolarisConfiguration + ENFORCE_PRINCIPAL_CREDENTIAL_ROTATION_REQUIRED_CHECKING = + PolarisConfiguration.builder() + .key("ENFORCE_PRINCIPAL_CREDENTIAL_ROTATION_REQUIRED_CHECKING") + .description( + "If set to true, require that principals must rotate their credentials before being used " + + "for anything else." + ) + .defaultValue(false) + .build(); + + public static final PolarisConfiguration ALLOW_TABLE_LOCATION_OVERLAP = + PolarisConfiguration.builder() + .key("ALLOW_TABLE_LOCATION_OVERLAP") + .description( + "If set to true, allow one table's location to reside within another table's location. " + + "This is only enforced within a given namespace." + ) + .defaultValue(false) + .build(); + + public static final PolarisConfiguration ALLOW_NAMESPACE_LOCATION_OVERLAP = + PolarisConfiguration.builder() + .key("ALLOW_NAMESPACE_LOCATION_OVERLAP") + .description( + "If set to true, allow one table's location to reside within another table's location. " + + "This is only enforced within a parent catalog or namespace." + ) + .defaultValue(false) + .build(); + + public static final PolarisConfiguration ALLOW_EXTERNAL_METADATA_FILE_LOCATION = + PolarisConfiguration.builder() + .key("ALLOW_EXTERNAL_METADATA_FILE_LOCATION") + .description( + "If set to true, allows metadata files to be located outside the default metadata directory." + ) + .defaultValue(false) + .build(); + + public static final PolarisConfiguration ENFORCE_GLOBALLY_UNIQUE_TABLE_LOCATIONS = + PolarisConfiguration.builder() + .key("ENFORCE_GLOBALLY_UNIQUE_TABLE_LOCATIONS") + .description( + "If set to true, enforces that all table locations must be globally unique. " + + "This is enforced across all namespaces and catalogs." + ) + .defaultValue(false) + .build(); + + public static final PolarisConfiguration + ENFORCE_TABLE_LOCATIONS_INSIDE_NAMESPACE_LOCATIONS = + PolarisConfiguration.builder() + .key("ENFORCE_TABLE_LOCATIONS_INSIDE_NAMESPACE_LOCATIONS") + .description( + "If set to true, enforces that table locations must reside within their immediate parent " + + "namespace's locations." + ) + .defaultValue(true) + .build(); + + public static final PolarisConfiguration ALLOW_OVERLAPPING_CATALOG_URLS = + PolarisConfiguration.builder() + .key("ALLOW_OVERLAPPING_CATALOG_URLS") + .description( + """ + If set to true, allows catalog URLs to overlap. + """.trim()) + .defaultValue(false) + .build(); + + public static final PolarisConfiguration CATALOG_ALLOW_UNSTRUCTURED_TABLE_LOCATION = + PolarisConfiguration.builder() + .catalogConfig(true) + .key("allow.unstructured.table.location") + .description( + """ + If set to true, allows unstructured table locations. + """.trim()) + .defaultValue(false) + .build(); + + public static final PolarisConfiguration CATALOG_ALLOW_EXTERNAL_TABLE_LOCATION = + PolarisConfiguration.builder() + .catalogConfig(true) + .key("allow.external.table.location") + .description( + """ + If set to true, allows tables to have external locations outside the default structure. + """ + .trim()) + .defaultValue(false) + .build(); } diff --git a/polaris-core/src/main/java/io/polaris/core/PolarisConfigurationStore.java b/polaris-core/src/main/java/io/polaris/core/PolarisConfigurationStore.java index f2e38c2ddd..9c424aad43 100644 --- a/polaris-core/src/main/java/io/polaris/core/PolarisConfigurationStore.java +++ b/polaris-core/src/main/java/io/polaris/core/PolarisConfigurationStore.java @@ -53,4 +53,20 @@ public interface PolarisConfigurationStore { T configValue = getConfiguration(ctx, configName); return configValue != null ? configValue : defaultValue; } + + /** + * Retrieve the current value for a configuration. + * + * @param ctx the current call context + * @param config the configuration to load + * @return the current value set for the configuration key or null if not set + * @param the type of the configuration value + */ + default @Nullable T getConfiguration(PolarisCallContext ctx, PolarisConfiguration config) { + if (config.catalogConfig) { + throw new IllegalArgumentException( + String.format("Attempted to read catalog configuration `%s` as a global config", config.key)); + } + return getConfiguration(ctx, config.key, config.defaultValue); + } } diff --git a/polaris-service/src/main/java/io/polaris/service/catalog/BasePolarisCatalog.java b/polaris-service/src/main/java/io/polaris/service/catalog/BasePolarisCatalog.java index 982ecf6c6e..f33c1a2d32 100644 --- a/polaris-service/src/main/java/io/polaris/service/catalog/BasePolarisCatalog.java +++ b/polaris-service/src/main/java/io/polaris/service/catalog/BasePolarisCatalog.java @@ -919,6 +919,7 @@ private void validateLocationForTableLike( */ private void validateNoLocationOverlap( TableIdentifier identifier, List resolvedNamespace, String location) { +<<<<<<< HEAD if (callContext .getPolarisCallContext() .getConfigurationStore() @@ -926,6 +927,17 @@ private void validateNoLocationOverlap( callContext.getPolarisCallContext(), PolarisConfiguration.ALLOW_TABLE_LOCATION_OVERLAP, PolarisConfiguration.DEFAULT_ALLOW_TABLE_LOCATION_OVERLAP)) { +======= + boolean allowLocalTableLocationOverlap = + Boolean.parseBoolean( + String.valueOf( + getCurrentPolarisContext() + .getConfigurationStore() + .getConfiguration( + getCurrentPolarisContext(), + PolarisConfiguration.ALLOW_TABLE_LOCATION_OVERLAP))); + if (allowLocalTableLocationOverlap) { +>>>>>>> 6b4ec75 (wip) LOG.debug("Skipping location overlap validation for identifier '{}'", identifier); } else { // if (entity.getSubType().equals(PolarisEntitySubType.TABLE)) { // TODO - is this necessary for views? overlapping views do not expose subdirectories via the diff --git a/polaris-service/src/main/java/io/polaris/service/config/DefaultConfigurationStore.java b/polaris-service/src/main/java/io/polaris/service/config/DefaultConfigurationStore.java index bf5a3f91f9..7d6d3a4d4f 100644 --- a/polaris-service/src/main/java/io/polaris/service/config/DefaultConfigurationStore.java +++ b/polaris-service/src/main/java/io/polaris/service/config/DefaultConfigurationStore.java @@ -16,7 +16,11 @@ package io.polaris.service.config; import io.polaris.core.PolarisCallContext; +import io.polaris.core.PolarisConfiguration; import io.polaris.core.PolarisConfigurationStore; + +import java.util.HashMap; +import java.util.List; import java.util.Map; import org.jetbrains.annotations.Nullable; diff --git a/polaris-service/src/test/java/io/polaris/service/admin/PolarisAuthzTestBase.java b/polaris-service/src/test/java/io/polaris/service/admin/PolarisAuthzTestBase.java index 5f1420cb39..f172dd7146 100644 --- a/polaris-service/src/test/java/io/polaris/service/admin/PolarisAuthzTestBase.java +++ b/polaris-service/src/test/java/io/polaris/service/admin/PolarisAuthzTestBase.java @@ -127,7 +127,7 @@ public abstract class PolarisAuthzTestBase { new PolarisAuthorizer( new DefaultConfigurationStore( Map.of( - PolarisConfiguration.ENFORCE_PRINCIPAL_CREDENTIAL_ROTATION_REQUIRED_CHECKING, + PolarisConfiguration.ENFORCE_PRINCIPAL_CREDENTIAL_ROTATION_REQUIRED_CHECKING.key, true))); protected BasePolarisCatalog baseCatalog; From e12e4dd53be77cad444c2622f12db52837dd9a2e Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Mon, 5 Aug 2024 20:54:20 -0700 Subject: [PATCH 2/7] resolve conflicts --- .../io/polaris/core/PolarisConfiguration.java | 45 ++++---- .../core/PolarisConfigurationStore.java | 44 +++++++- .../polaris/core/auth/PolarisAuthorizer.java | 3 +- .../service/catalog/BasePolarisCatalog.java | 103 ++++++++++++------ .../catalog/BasePolarisCatalogTest.java | 4 +- 5 files changed, 138 insertions(+), 61 deletions(-) diff --git a/polaris-core/src/main/java/io/polaris/core/PolarisConfiguration.java b/polaris-core/src/main/java/io/polaris/core/PolarisConfiguration.java index bbbe3e7082..9a16607402 100644 --- a/polaris-core/src/main/java/io/polaris/core/PolarisConfiguration.java +++ b/polaris-core/src/main/java/io/polaris/core/PolarisConfiguration.java @@ -15,25 +15,34 @@ */ package io.polaris.core; +import java.util.Optional; + public class PolarisConfiguration { public final String key; public final String description; public final T defaultValue; - public final boolean catalogConfig; + public final Optional catalogConfigImpl; - public PolarisConfiguration(String key, String description, T defaultValue, boolean catalogConfig) { + public PolarisConfiguration(String key, String description, T defaultValue, Optional catalogConfig) { this.key = key; this.description = description; this.defaultValue = defaultValue; - this.catalogConfig = catalogConfig; + this.catalogConfigImpl = catalogConfig; + } + + public String catalogConfig() { + return catalogConfigImpl.orElseThrow(() -> + new IllegalStateException( + "Attempted to read a catalog config key from a configuration that doesn't have one.") + ); } public static class Builder { private String key; private String description; private T defaultValue; - private boolean catalogConfig = false; + private Optional catalogConfig = Optional.empty(); public Builder key(String key) { this.key = key; @@ -50,8 +59,8 @@ public Builder defaultValue(T defaultValue) { return this; } - public Builder catalogConfig(boolean catalogConfig) { - this.catalogConfig = catalogConfig; + public Builder catalogConfig(String catalogConfig) { + this.catalogConfig = Optional.of(catalogConfig); return this; } @@ -132,32 +141,28 @@ public static Builder builder() { PolarisConfiguration.builder() .key("ALLOW_OVERLAPPING_CATALOG_URLS") .description( - """ - If set to true, allows catalog URLs to overlap. - """.trim()) + "If set to true, allows catalog URLs to overlap." + ) .defaultValue(false) .build(); public static final PolarisConfiguration CATALOG_ALLOW_UNSTRUCTURED_TABLE_LOCATION = PolarisConfiguration.builder() - .catalogConfig(true) - .key("allow.unstructured.table.location") + .key("ALLOW_UNSTRUCTURED_TABLE_LOCATION") + .catalogConfig("allow.unstructured.table.location") .description( - """ - If set to true, allows unstructured table locations. - """.trim()) + "If set to true, allows unstructured table locations." + ) .defaultValue(false) .build(); public static final PolarisConfiguration CATALOG_ALLOW_EXTERNAL_TABLE_LOCATION = PolarisConfiguration.builder() - .catalogConfig(true) - .key("allow.external.table.location") + .key("ALLOW_EXTERNAL_TABLE_LOCATION") + .catalogConfig("allow.external.table.location") .description( - """ - If set to true, allows tables to have external locations outside the default structure. - """ - .trim()) + "If set to true, allows tables to have external locations outside the default structure." + ) .defaultValue(false) .build(); } diff --git a/polaris-core/src/main/java/io/polaris/core/PolarisConfigurationStore.java b/polaris-core/src/main/java/io/polaris/core/PolarisConfigurationStore.java index 9c424aad43..c28c66065d 100644 --- a/polaris-core/src/main/java/io/polaris/core/PolarisConfigurationStore.java +++ b/polaris-core/src/main/java/io/polaris/core/PolarisConfigurationStore.java @@ -16,8 +16,10 @@ package io.polaris.core; import com.google.common.base.Preconditions; +import io.polaris.core.entity.CatalogEntity; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; +import software.amazon.awssdk.services.s3.endpoints.internal.Value; /** * Dynamic configuration store used to retrieve runtime parameters, which may vary by realm or by @@ -54,6 +56,22 @@ public interface PolarisConfigurationStore { return configValue != null ? configValue : defaultValue; } + /** + * In some cases, we may extract a value that doesn't match the expected type for a config. + * This method can be used to attempt to force-cast it using `String.valueOf` + */ + private @NotNull T tryCast(PolarisConfiguration config, Object value) { + if (value == null) { + return config.defaultValue; + } + + if (config.defaultValue instanceof Boolean) { + return (T) config.defaultValue.getClass().cast(Boolean.parseBoolean(String.valueOf(value))); + } else { + return (T) value; + } + } + /** * Retrieve the current value for a configuration. * @@ -62,11 +80,27 @@ public interface PolarisConfigurationStore { * @return the current value set for the configuration key or null if not set * @param the type of the configuration value */ - default @Nullable T getConfiguration(PolarisCallContext ctx, PolarisConfiguration config) { - if (config.catalogConfig) { - throw new IllegalArgumentException( - String.format("Attempted to read catalog configuration `%s` as a global config", config.key)); + default @NotNull T getConfiguration( + PolarisCallContext ctx, PolarisConfiguration config) { + T result = getConfiguration(ctx, config.key, config.defaultValue); + return tryCast(config, result); + } + + /** + * Retrieve the current value for a configuration, overriding with a catalog config if it is present. + * + * @param ctx the current call context + * @param catalogEntity the catalog to check for an override + * @param config the configuration to load + * @return the current value set for the configuration key or null if not set + * @param the type of the configuration value + */ + default @NotNull T getConfiguration( + PolarisCallContext ctx, @NotNull CatalogEntity catalogEntity, PolarisConfiguration config) { + if (catalogEntity.getPropertiesAsMap().containsKey(config.catalogConfig())) { + return tryCast(config, catalogEntity.getPropertiesAsMap().get(config.catalogConfig())); + } else { + return getConfiguration(ctx, config); } - return getConfiguration(ctx, config.key, config.defaultValue); } } diff --git a/polaris-core/src/main/java/io/polaris/core/auth/PolarisAuthorizer.java b/polaris-core/src/main/java/io/polaris/core/auth/PolarisAuthorizer.java index 29f40daaba..0e64348138 100644 --- a/polaris-core/src/main/java/io/polaris/core/auth/PolarisAuthorizer.java +++ b/polaris-core/src/main/java/io/polaris/core/auth/PolarisAuthorizer.java @@ -500,8 +500,7 @@ public void authorizeOrThrow( boolean enforceCredentialRotationRequiredState = featureConfig.getConfiguration( CallContext.getCurrentContext().getPolarisCallContext(), - PolarisConfiguration.ENFORCE_PRINCIPAL_CREDENTIAL_ROTATION_REQUIRED_CHECKING, - false); + PolarisConfiguration.ENFORCE_PRINCIPAL_CREDENTIAL_ROTATION_REQUIRED_CHECKING); if (enforceCredentialRotationRequiredState && authenticatedPrincipal .getPrincipalEntity() diff --git a/polaris-service/src/main/java/io/polaris/service/catalog/BasePolarisCatalog.java b/polaris-service/src/main/java/io/polaris/service/catalog/BasePolarisCatalog.java index f33c1a2d32..1e90020b11 100644 --- a/polaris-service/src/main/java/io/polaris/service/catalog/BasePolarisCatalog.java +++ b/polaris-service/src/main/java/io/polaris/service/catalog/BasePolarisCatalog.java @@ -430,13 +430,14 @@ private void createNamespaceInternal( .setCreateTimestamp(System.currentTimeMillis()) .setBaseLocation(baseLocation) .build(); - if (!callContext - .getPolarisCallContext() - .getConfigurationStore() - .getConfiguration( - callContext.getPolarisCallContext(), - PolarisConfiguration.ALLOW_NAMESPACE_LOCATION_OVERLAP, - PolarisConfiguration.DEFAULT_ALLOW_NAMESPACE_LOCATION_OVERLAP)) { + boolean allowNamespaceLocationOverlap = + callContext + .getPolarisCallContext() + .getConfigurationStore() + .getConfiguration( + callContext.getPolarisCallContext(), + PolarisConfiguration.ALLOW_NAMESPACE_LOCATION_OVERLAP); + if (!allowNamespaceLocationOverlap) { LOG.debug("Validating no overlap for {} with sibling tables or namespaces", namespace); validateNoLocationOverlap( entity.getBaseLocation(), resolvedParent.getRawFullPath(), entity.getName()); @@ -585,8 +586,7 @@ public boolean setProperties(Namespace namespace, Map properties .getConfigurationStore() .getConfiguration( callContext.getPolarisCallContext(), - PolarisConfiguration.ALLOW_NAMESPACE_LOCATION_OVERLAP, - PolarisConfiguration.DEFAULT_ALLOW_NAMESPACE_LOCATION_OVERLAP)) { + PolarisConfiguration.ALLOW_NAMESPACE_LOCATION_OVERLAP)) { LOG.debug("Validating no overlap with sibling tables or namespaces"); validateNoLocationOverlap( NamespaceEntity.of(updatedEntity).getBaseLocation(), @@ -854,10 +854,27 @@ private void validateLocationForTableLike( TableIdentifier identifier, String location, PolarisResolvedPathWrapper resolvedStorageEntity) { - Optional optStorageConfiguration = - PolarisStorageConfigurationInfo.forEntityPath( - callContext.getPolarisCallContext().getDiagServices(), - resolvedStorageEntity.getRawFullPath()); + boolean enforceTableLocationsInsideNamespaceLocations = + getCurrentPolarisContext() + .getConfigurationStore() + .getConfiguration( + getCurrentPolarisContext(), + PolarisConfiguration.ENFORCE_TABLE_LOCATIONS_INSIDE_NAMESPACE_LOCATIONS); + + Optional optStorageConfiguration = Optional.empty(); + if (enforceTableLocationsInsideNamespaceLocations) { + optStorageConfiguration = + PolarisStorageConfigurationInfo.forEntityPath( + callContext.getPolarisCallContext().getDiagServices(), + resolvedStorageEntity.getRawFullPath()); + } else { + optStorageConfiguration = + findStorageInfoFromHierarchy(resolvedStorageEntity) + .map( + storageInfoHolderEntity -> { + return new CatalogEntity(storageInfoHolderEntity).getStorageConfigurationInfo(); + }); + } optStorageConfiguration.ifPresentOrElse( storageConfigInfo -> { @@ -919,15 +936,6 @@ private void validateLocationForTableLike( */ private void validateNoLocationOverlap( TableIdentifier identifier, List resolvedNamespace, String location) { -<<<<<<< HEAD - if (callContext - .getPolarisCallContext() - .getConfigurationStore() - .getConfiguration( - callContext.getPolarisCallContext(), - PolarisConfiguration.ALLOW_TABLE_LOCATION_OVERLAP, - PolarisConfiguration.DEFAULT_ALLOW_TABLE_LOCATION_OVERLAP)) { -======= boolean allowLocalTableLocationOverlap = Boolean.parseBoolean( String.valueOf( @@ -937,7 +945,6 @@ private void validateNoLocationOverlap( getCurrentPolarisContext(), PolarisConfiguration.ALLOW_TABLE_LOCATION_OVERLAP))); if (allowLocalTableLocationOverlap) { ->>>>>>> 6b4ec75 (wip) LOG.debug("Skipping location overlap validation for identifier '{}'", identifier); } else { // if (entity.getSubType().equals(PolarisEntitySubType.TABLE)) { // TODO - is this necessary for views? overlapping views do not expose subdirectories via the @@ -1127,7 +1134,7 @@ public void doRefresh() { LOG.atError() .addKeyValue("entity.getTableIdentifier()", entity.getTableIdentifier()) .addKeyValue("tableIdentifier", tableIdentifier) - .log("Stored table identifier mismatches requested identifier"); + .log("Stored entity identifier mismatches requested identifier"); } } @@ -1142,13 +1149,18 @@ public void doRefresh() { MAX_RETRIES, metadataLocation -> { FileIO fileIO = this.tableFileIO; + boolean closeFileIO = false; + PolarisResolvedPathWrapper resolvedStorageEntity = + resolvedEntities == null + ? resolvedEntityView.getResolvedPath(tableIdentifier.namespace()) + : resolvedEntities; String latestLocationDir = latestLocation.substring(0, latestLocation.lastIndexOf('/')); fileIO = refreshIOWithCredentials( tableIdentifier, Set.of(latestLocationDir), - resolvedEntities, + resolvedStorageEntity, new HashMap<>(), fileIO); return TableMetadataParser.read(fileIO, metadataLocation); @@ -1158,7 +1170,7 @@ public void doRefresh() { @Override public void doCommit(TableMetadata base, TableMetadata metadata) { - LOG.debug("doCommit for table {} with base {}, metadata {}", tableIdentifier, base, metadata); + LOG.debug("doCommit for {} with base {}, metadata {}", tableIdentifier, base, metadata); // TODO: Maybe avoid writing metadata if there's definitely a transaction conflict if (null == base && !namespaceExists(tableIdentifier.namespace())) { throw new NoSuchNamespaceException( @@ -1291,8 +1303,7 @@ private void validateMetadataFileInTableDir( .getConfigurationStore() .getConfiguration( polarisCallContext, - PolarisConfiguration.ALLOW_EXTERNAL_METADATA_FILE_LOCATION, - PolarisConfiguration.DEFAULT_ALLOW_EXTERNAL_METADATA_FILE_LOCATION)) { + PolarisConfiguration.ALLOW_EXTERNAL_METADATA_FILE_LOCATION)) { LOG.debug( "Validating base location {} for table {} in metadata file {}", metadata.location(), @@ -1343,7 +1354,7 @@ public void doRefresh() { LOG.atError() .addKeyValue("entity.getTableIdentifier()", entity.getTableIdentifier()) .addKeyValue("identifier", identifier) - .log("Stored view identifier mismatches requested identifier"); + .log("Stored entity identifier mismatches requested identifier"); } } @@ -1359,10 +1370,14 @@ public void doRefresh() { metadataLocation -> { FileIO fileIO = this.viewFileIO; boolean closeFileIO = false; + PolarisResolvedPathWrapper resolvedStorageEntity = + resolvedEntities == null + ? resolvedEntityView.getResolvedPath(identifier.namespace()) + : resolvedEntities; String latestLocationDir = latestLocation.substring(0, latestLocation.lastIndexOf('/')); Optional storageInfoEntity = - findStorageInfoFromHierarchy(resolvedEntities); + findStorageInfoFromHierarchy(resolvedStorageEntity); Map credentialsMap = storageInfoEntity .map( @@ -1392,7 +1407,7 @@ public void doRefresh() { @Override public void doCommit(ViewMetadata base, ViewMetadata metadata) { // TODO: Maybe avoid writing metadata if there's definitely a transaction conflict - LOG.debug("doCommit for view {} with base {}, metadata {}", identifier, base, metadata); + LOG.debug("doCommit for {} with base {}, metadata {}", identifier, base, metadata); if (null == base && !namespaceExists(identifier.namespace())) { throw new NoSuchNamespaceException( "Cannot create view %s. Namespace does not exist: %s", @@ -1440,6 +1455,12 @@ public void doCommit(ViewMetadata base, ViewMetadata metadata) { String newLocation = writeNewMetadataIfRequired(metadata); String oldLocation = base == null ? null : currentMetadataLocation(); + if (null == base && !namespaceExists(identifier.namespace())) { + throw new NoSuchNamespaceException( + "Cannot create view %s. Namespace does not exist: %s", + identifier, identifier.namespace()); + } + TableLikeEntity entity = TableLikeEntity.of(resolvedEntities == null ? null : resolvedEntities.getRawLeafEntity()); String existingLocation; @@ -1649,7 +1670,7 @@ private void renameTableLike( /** * Caller must fill in all entity fields except parentId, since the caller may not want to - * duplicate the logic to try to resolve parentIds before constructing the proposed entity. This + * duplicate the logic to try to reolve parentIds before constructing the proposed entity. This * method will fill in the parentId if needed upon resolution. */ private void createTableLike(long catalogId, TableIdentifier identifier, PolarisEntity entity) { @@ -1675,6 +1696,24 @@ private void createTableLike( List catalogPath = resolvedParent.getRawFullPath(); + boolean enforceGloballyUniqueTableLocation = + getCurrentPolarisContext() + .getConfigurationStore() + .getConfiguration( + getCurrentPolarisContext(), + PolarisConfiguration.ENFORCE_GLOBALLY_UNIQUE_TABLE_LOCATIONS); + + if (enforceGloballyUniqueTableLocation) { + if (entityManager + .getMetaStoreManager() + .locationOverlapsWithExistingTableLike( + callContext.getPolarisCallContext(), entity.getLocation())) { + throw new org.apache.iceberg.exceptions.BadRequestException( + "Unable to create table at location '%s' because it conflicts with the location of an existing entity", + entity.getLocation()); + } + } + if (entity.getParentId() <= 0) { // TODO: Validate catalogPath size is at least 1 for catalog entity? entity = diff --git a/polaris-service/src/test/java/io/polaris/service/catalog/BasePolarisCatalogTest.java b/polaris-service/src/test/java/io/polaris/service/catalog/BasePolarisCatalogTest.java index 6ca1957230..3e5adeb737 100644 --- a/polaris-service/src/test/java/io/polaris/service/catalog/BasePolarisCatalogTest.java +++ b/polaris-service/src/test/java/io/polaris/service/catalog/BasePolarisCatalogTest.java @@ -187,8 +187,8 @@ public void before() { .setName(CATALOG_NAME) .setDefaultBaseLocation(storageLocation) .setReplaceNewLocationPrefixWithCatalogDefault("file:") - .addProperty(PolarisConfiguration.CATALOG_ALLOW_EXTERNAL_TABLE_LOCATION, "true") - .addProperty(PolarisConfiguration.CATALOG_ALLOW_UNSTRUCTURED_TABLE_LOCATION, "true") + .addProperty(PolarisConfiguration.CATALOG_ALLOW_EXTERNAL_TABLE_LOCATION.catalogConfig.get(), "true") + .addProperty(PolarisConfiguration.CATALOG_ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig.get(), "true") .setStorageConfigurationInfo(storageConfigModel, storageLocation) .build()); From 7554e33759cc0dab846134b68eca25377c4f8598 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Mon, 5 Aug 2024 21:18:01 -0700 Subject: [PATCH 3/7] reverts --- .../service/catalog/BasePolarisCatalog.java | 109 +++++------------- .../config/DefaultConfigurationStore.java | 4 - .../catalog/BasePolarisCatalogTest.java | 4 +- 3 files changed, 31 insertions(+), 86 deletions(-) diff --git a/polaris-service/src/main/java/io/polaris/service/catalog/BasePolarisCatalog.java b/polaris-service/src/main/java/io/polaris/service/catalog/BasePolarisCatalog.java index 1e90020b11..982ecf6c6e 100644 --- a/polaris-service/src/main/java/io/polaris/service/catalog/BasePolarisCatalog.java +++ b/polaris-service/src/main/java/io/polaris/service/catalog/BasePolarisCatalog.java @@ -430,14 +430,13 @@ private void createNamespaceInternal( .setCreateTimestamp(System.currentTimeMillis()) .setBaseLocation(baseLocation) .build(); - boolean allowNamespaceLocationOverlap = - callContext - .getPolarisCallContext() - .getConfigurationStore() - .getConfiguration( - callContext.getPolarisCallContext(), - PolarisConfiguration.ALLOW_NAMESPACE_LOCATION_OVERLAP); - if (!allowNamespaceLocationOverlap) { + if (!callContext + .getPolarisCallContext() + .getConfigurationStore() + .getConfiguration( + callContext.getPolarisCallContext(), + PolarisConfiguration.ALLOW_NAMESPACE_LOCATION_OVERLAP, + PolarisConfiguration.DEFAULT_ALLOW_NAMESPACE_LOCATION_OVERLAP)) { LOG.debug("Validating no overlap for {} with sibling tables or namespaces", namespace); validateNoLocationOverlap( entity.getBaseLocation(), resolvedParent.getRawFullPath(), entity.getName()); @@ -586,7 +585,8 @@ public boolean setProperties(Namespace namespace, Map properties .getConfigurationStore() .getConfiguration( callContext.getPolarisCallContext(), - PolarisConfiguration.ALLOW_NAMESPACE_LOCATION_OVERLAP)) { + PolarisConfiguration.ALLOW_NAMESPACE_LOCATION_OVERLAP, + PolarisConfiguration.DEFAULT_ALLOW_NAMESPACE_LOCATION_OVERLAP)) { LOG.debug("Validating no overlap with sibling tables or namespaces"); validateNoLocationOverlap( NamespaceEntity.of(updatedEntity).getBaseLocation(), @@ -854,27 +854,10 @@ private void validateLocationForTableLike( TableIdentifier identifier, String location, PolarisResolvedPathWrapper resolvedStorageEntity) { - boolean enforceTableLocationsInsideNamespaceLocations = - getCurrentPolarisContext() - .getConfigurationStore() - .getConfiguration( - getCurrentPolarisContext(), - PolarisConfiguration.ENFORCE_TABLE_LOCATIONS_INSIDE_NAMESPACE_LOCATIONS); - - Optional optStorageConfiguration = Optional.empty(); - if (enforceTableLocationsInsideNamespaceLocations) { - optStorageConfiguration = - PolarisStorageConfigurationInfo.forEntityPath( - callContext.getPolarisCallContext().getDiagServices(), - resolvedStorageEntity.getRawFullPath()); - } else { - optStorageConfiguration = - findStorageInfoFromHierarchy(resolvedStorageEntity) - .map( - storageInfoHolderEntity -> { - return new CatalogEntity(storageInfoHolderEntity).getStorageConfigurationInfo(); - }); - } + Optional optStorageConfiguration = + PolarisStorageConfigurationInfo.forEntityPath( + callContext.getPolarisCallContext().getDiagServices(), + resolvedStorageEntity.getRawFullPath()); optStorageConfiguration.ifPresentOrElse( storageConfigInfo -> { @@ -936,15 +919,13 @@ private void validateLocationForTableLike( */ private void validateNoLocationOverlap( TableIdentifier identifier, List resolvedNamespace, String location) { - boolean allowLocalTableLocationOverlap = - Boolean.parseBoolean( - String.valueOf( - getCurrentPolarisContext() - .getConfigurationStore() - .getConfiguration( - getCurrentPolarisContext(), - PolarisConfiguration.ALLOW_TABLE_LOCATION_OVERLAP))); - if (allowLocalTableLocationOverlap) { + if (callContext + .getPolarisCallContext() + .getConfigurationStore() + .getConfiguration( + callContext.getPolarisCallContext(), + PolarisConfiguration.ALLOW_TABLE_LOCATION_OVERLAP, + PolarisConfiguration.DEFAULT_ALLOW_TABLE_LOCATION_OVERLAP)) { LOG.debug("Skipping location overlap validation for identifier '{}'", identifier); } else { // if (entity.getSubType().equals(PolarisEntitySubType.TABLE)) { // TODO - is this necessary for views? overlapping views do not expose subdirectories via the @@ -1134,7 +1115,7 @@ public void doRefresh() { LOG.atError() .addKeyValue("entity.getTableIdentifier()", entity.getTableIdentifier()) .addKeyValue("tableIdentifier", tableIdentifier) - .log("Stored entity identifier mismatches requested identifier"); + .log("Stored table identifier mismatches requested identifier"); } } @@ -1149,18 +1130,13 @@ public void doRefresh() { MAX_RETRIES, metadataLocation -> { FileIO fileIO = this.tableFileIO; - boolean closeFileIO = false; - PolarisResolvedPathWrapper resolvedStorageEntity = - resolvedEntities == null - ? resolvedEntityView.getResolvedPath(tableIdentifier.namespace()) - : resolvedEntities; String latestLocationDir = latestLocation.substring(0, latestLocation.lastIndexOf('/')); fileIO = refreshIOWithCredentials( tableIdentifier, Set.of(latestLocationDir), - resolvedStorageEntity, + resolvedEntities, new HashMap<>(), fileIO); return TableMetadataParser.read(fileIO, metadataLocation); @@ -1170,7 +1146,7 @@ public void doRefresh() { @Override public void doCommit(TableMetadata base, TableMetadata metadata) { - LOG.debug("doCommit for {} with base {}, metadata {}", tableIdentifier, base, metadata); + LOG.debug("doCommit for table {} with base {}, metadata {}", tableIdentifier, base, metadata); // TODO: Maybe avoid writing metadata if there's definitely a transaction conflict if (null == base && !namespaceExists(tableIdentifier.namespace())) { throw new NoSuchNamespaceException( @@ -1303,7 +1279,8 @@ private void validateMetadataFileInTableDir( .getConfigurationStore() .getConfiguration( polarisCallContext, - PolarisConfiguration.ALLOW_EXTERNAL_METADATA_FILE_LOCATION)) { + PolarisConfiguration.ALLOW_EXTERNAL_METADATA_FILE_LOCATION, + PolarisConfiguration.DEFAULT_ALLOW_EXTERNAL_METADATA_FILE_LOCATION)) { LOG.debug( "Validating base location {} for table {} in metadata file {}", metadata.location(), @@ -1354,7 +1331,7 @@ public void doRefresh() { LOG.atError() .addKeyValue("entity.getTableIdentifier()", entity.getTableIdentifier()) .addKeyValue("identifier", identifier) - .log("Stored entity identifier mismatches requested identifier"); + .log("Stored view identifier mismatches requested identifier"); } } @@ -1370,14 +1347,10 @@ public void doRefresh() { metadataLocation -> { FileIO fileIO = this.viewFileIO; boolean closeFileIO = false; - PolarisResolvedPathWrapper resolvedStorageEntity = - resolvedEntities == null - ? resolvedEntityView.getResolvedPath(identifier.namespace()) - : resolvedEntities; String latestLocationDir = latestLocation.substring(0, latestLocation.lastIndexOf('/')); Optional storageInfoEntity = - findStorageInfoFromHierarchy(resolvedStorageEntity); + findStorageInfoFromHierarchy(resolvedEntities); Map credentialsMap = storageInfoEntity .map( @@ -1407,7 +1380,7 @@ public void doRefresh() { @Override public void doCommit(ViewMetadata base, ViewMetadata metadata) { // TODO: Maybe avoid writing metadata if there's definitely a transaction conflict - LOG.debug("doCommit for {} with base {}, metadata {}", identifier, base, metadata); + LOG.debug("doCommit for view {} with base {}, metadata {}", identifier, base, metadata); if (null == base && !namespaceExists(identifier.namespace())) { throw new NoSuchNamespaceException( "Cannot create view %s. Namespace does not exist: %s", @@ -1455,12 +1428,6 @@ public void doCommit(ViewMetadata base, ViewMetadata metadata) { String newLocation = writeNewMetadataIfRequired(metadata); String oldLocation = base == null ? null : currentMetadataLocation(); - if (null == base && !namespaceExists(identifier.namespace())) { - throw new NoSuchNamespaceException( - "Cannot create view %s. Namespace does not exist: %s", - identifier, identifier.namespace()); - } - TableLikeEntity entity = TableLikeEntity.of(resolvedEntities == null ? null : resolvedEntities.getRawLeafEntity()); String existingLocation; @@ -1670,7 +1637,7 @@ private void renameTableLike( /** * Caller must fill in all entity fields except parentId, since the caller may not want to - * duplicate the logic to try to reolve parentIds before constructing the proposed entity. This + * duplicate the logic to try to resolve parentIds before constructing the proposed entity. This * method will fill in the parentId if needed upon resolution. */ private void createTableLike(long catalogId, TableIdentifier identifier, PolarisEntity entity) { @@ -1696,24 +1663,6 @@ private void createTableLike( List catalogPath = resolvedParent.getRawFullPath(); - boolean enforceGloballyUniqueTableLocation = - getCurrentPolarisContext() - .getConfigurationStore() - .getConfiguration( - getCurrentPolarisContext(), - PolarisConfiguration.ENFORCE_GLOBALLY_UNIQUE_TABLE_LOCATIONS); - - if (enforceGloballyUniqueTableLocation) { - if (entityManager - .getMetaStoreManager() - .locationOverlapsWithExistingTableLike( - callContext.getPolarisCallContext(), entity.getLocation())) { - throw new org.apache.iceberg.exceptions.BadRequestException( - "Unable to create table at location '%s' because it conflicts with the location of an existing entity", - entity.getLocation()); - } - } - if (entity.getParentId() <= 0) { // TODO: Validate catalogPath size is at least 1 for catalog entity? entity = diff --git a/polaris-service/src/main/java/io/polaris/service/config/DefaultConfigurationStore.java b/polaris-service/src/main/java/io/polaris/service/config/DefaultConfigurationStore.java index 7d6d3a4d4f..bf5a3f91f9 100644 --- a/polaris-service/src/main/java/io/polaris/service/config/DefaultConfigurationStore.java +++ b/polaris-service/src/main/java/io/polaris/service/config/DefaultConfigurationStore.java @@ -16,11 +16,7 @@ package io.polaris.service.config; import io.polaris.core.PolarisCallContext; -import io.polaris.core.PolarisConfiguration; import io.polaris.core.PolarisConfigurationStore; - -import java.util.HashMap; -import java.util.List; import java.util.Map; import org.jetbrains.annotations.Nullable; diff --git a/polaris-service/src/test/java/io/polaris/service/catalog/BasePolarisCatalogTest.java b/polaris-service/src/test/java/io/polaris/service/catalog/BasePolarisCatalogTest.java index 3e5adeb737..6ca1957230 100644 --- a/polaris-service/src/test/java/io/polaris/service/catalog/BasePolarisCatalogTest.java +++ b/polaris-service/src/test/java/io/polaris/service/catalog/BasePolarisCatalogTest.java @@ -187,8 +187,8 @@ public void before() { .setName(CATALOG_NAME) .setDefaultBaseLocation(storageLocation) .setReplaceNewLocationPrefixWithCatalogDefault("file:") - .addProperty(PolarisConfiguration.CATALOG_ALLOW_EXTERNAL_TABLE_LOCATION.catalogConfig.get(), "true") - .addProperty(PolarisConfiguration.CATALOG_ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig.get(), "true") + .addProperty(PolarisConfiguration.CATALOG_ALLOW_EXTERNAL_TABLE_LOCATION, "true") + .addProperty(PolarisConfiguration.CATALOG_ALLOW_UNSTRUCTURED_TABLE_LOCATION, "true") .setStorageConfigurationInfo(storageConfigModel, storageLocation) .build()); From 9c19a376a20d3a0e40b37ebd2a428d6d1538c2c1 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Mon, 5 Aug 2024 21:41:38 -0700 Subject: [PATCH 4/7] fix some tests --- .../io/polaris/core/PolarisConfiguration.java | 38 +++++++------------ .../core/PolarisConfigurationStore.java | 13 +++++-- .../PolarisStorageConfigurationInfo.java | 24 +++++------- .../service/admin/PolarisAdminService.java | 13 +++---- .../service/catalog/BasePolarisCatalog.java | 23 +++++------ .../catalog/BasePolarisCatalogTest.java | 16 ++++---- .../catalog/BasePolarisCatalogViewTest.java | 4 +- .../PolarisRestCatalogIntegrationTest.java | 10 ++--- ...PolarisRestCatalogViewIntegrationTest.java | 4 +- 9 files changed, 66 insertions(+), 79 deletions(-) diff --git a/polaris-core/src/main/java/io/polaris/core/PolarisConfiguration.java b/polaris-core/src/main/java/io/polaris/core/PolarisConfiguration.java index 9a16607402..d04d63817e 100644 --- a/polaris-core/src/main/java/io/polaris/core/PolarisConfiguration.java +++ b/polaris-core/src/main/java/io/polaris/core/PolarisConfiguration.java @@ -22,13 +22,20 @@ public class PolarisConfiguration { public final String key; public final String description; public final T defaultValue; - public final Optional catalogConfigImpl; + private final Optional catalogConfigImpl; + private final Class typ; + @SuppressWarnings("unchecked") public PolarisConfiguration(String key, String description, T defaultValue, Optional catalogConfig) { this.key = key; this.description = description; this.defaultValue = defaultValue; this.catalogConfigImpl = catalogConfig; + this.typ = (Class) defaultValue.getClass(); + } + + public boolean hasCatalogConfig() { + return catalogConfigImpl.isPresent(); } public String catalogConfig() { @@ -38,6 +45,10 @@ public String catalogConfig() { ); } + T cast(Object value) { + return this.typ.cast(value); + } + public static class Builder { private String key; private String description; @@ -116,27 +127,6 @@ public static Builder builder() { .defaultValue(false) .build(); - public static final PolarisConfiguration ENFORCE_GLOBALLY_UNIQUE_TABLE_LOCATIONS = - PolarisConfiguration.builder() - .key("ENFORCE_GLOBALLY_UNIQUE_TABLE_LOCATIONS") - .description( - "If set to true, enforces that all table locations must be globally unique. " + - "This is enforced across all namespaces and catalogs." - ) - .defaultValue(false) - .build(); - - public static final PolarisConfiguration - ENFORCE_TABLE_LOCATIONS_INSIDE_NAMESPACE_LOCATIONS = - PolarisConfiguration.builder() - .key("ENFORCE_TABLE_LOCATIONS_INSIDE_NAMESPACE_LOCATIONS") - .description( - "If set to true, enforces that table locations must reside within their immediate parent " + - "namespace's locations." - ) - .defaultValue(true) - .build(); - public static final PolarisConfiguration ALLOW_OVERLAPPING_CATALOG_URLS = PolarisConfiguration.builder() .key("ALLOW_OVERLAPPING_CATALOG_URLS") @@ -146,7 +136,7 @@ public static Builder builder() { .defaultValue(false) .build(); - public static final PolarisConfiguration CATALOG_ALLOW_UNSTRUCTURED_TABLE_LOCATION = + public static final PolarisConfiguration ALLOW_UNSTRUCTURED_TABLE_LOCATION = PolarisConfiguration.builder() .key("ALLOW_UNSTRUCTURED_TABLE_LOCATION") .catalogConfig("allow.unstructured.table.location") @@ -156,7 +146,7 @@ public static Builder builder() { .defaultValue(false) .build(); - public static final PolarisConfiguration CATALOG_ALLOW_EXTERNAL_TABLE_LOCATION = + public static final PolarisConfiguration ALLOW_EXTERNAL_TABLE_LOCATION = PolarisConfiguration.builder() .key("ALLOW_EXTERNAL_TABLE_LOCATION") .catalogConfig("allow.external.table.location") diff --git a/polaris-core/src/main/java/io/polaris/core/PolarisConfigurationStore.java b/polaris-core/src/main/java/io/polaris/core/PolarisConfigurationStore.java index c28c66065d..63fb384149 100644 --- a/polaris-core/src/main/java/io/polaris/core/PolarisConfigurationStore.java +++ b/polaris-core/src/main/java/io/polaris/core/PolarisConfigurationStore.java @@ -17,8 +17,11 @@ import com.google.common.base.Preconditions; import io.polaris.core.entity.CatalogEntity; +import io.polaris.core.storage.PolarisStorageConfigurationInfo; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import software.amazon.awssdk.services.s3.endpoints.internal.Value; /** @@ -26,6 +29,7 @@ * request. */ public interface PolarisConfigurationStore { + final Logger LOGGER = LoggerFactory.getLogger(PolarisConfigurationStore.class); /** * Retrieve the current value for a configuration key. May be null if not set. @@ -66,9 +70,9 @@ public interface PolarisConfigurationStore { } if (config.defaultValue instanceof Boolean) { - return (T) config.defaultValue.getClass().cast(Boolean.parseBoolean(String.valueOf(value))); + return config.cast(Boolean.valueOf(String.valueOf(value))); } else { - return (T) value; + return config.cast(value); } } @@ -97,7 +101,10 @@ public interface PolarisConfigurationStore { */ default @NotNull T getConfiguration( PolarisCallContext ctx, @NotNull CatalogEntity catalogEntity, PolarisConfiguration config) { - if (catalogEntity.getPropertiesAsMap().containsKey(config.catalogConfig())) { + if (config.hasCatalogConfig() && catalogEntity.getPropertiesAsMap().containsKey(config.catalogConfig())) { + LOGGER.debug( + "Loaded config from catalog: {}", + config.catalogConfig()); return tryCast(config, catalogEntity.getPropertiesAsMap().get(config.catalogConfig())); } else { return getConfiguration(ctx, config); diff --git a/polaris-core/src/main/java/io/polaris/core/storage/PolarisStorageConfigurationInfo.java b/polaris-core/src/main/java/io/polaris/core/storage/PolarisStorageConfigurationInfo.java index 6150ef7ade..d8fc50aaf2 100644 --- a/polaris-core/src/main/java/io/polaris/core/storage/PolarisStorageConfigurationInfo.java +++ b/polaris-core/src/main/java/io/polaris/core/storage/PolarisStorageConfigurationInfo.java @@ -25,6 +25,7 @@ import io.polaris.core.PolarisConfiguration; import io.polaris.core.PolarisDiagnostics; import io.polaris.core.admin.model.Catalog; +import io.polaris.core.context.CallContext; import io.polaris.core.entity.CatalogEntity; import io.polaris.core.entity.PolarisEntity; import io.polaris.core.entity.PolarisEntityConstants; @@ -146,20 +147,15 @@ public static Optional forEntityPath( .findFirst() .orElse(null); CatalogEntity catalog = CatalogEntity.of(entityPath.get(0)); - boolean allowEscape = - Optional.ofNullable( - catalog - .getPropertiesAsMap() - .get(PolarisConfiguration.CATALOG_ALLOW_UNSTRUCTURED_TABLE_LOCATION)) - .map( - val -> { - LOGGER.debug( - "Found catalog level property to allow unstructured table location: {}", - val); - return Boolean.parseBoolean(val); - }) - .orElseGet(() -> Catalog.TypeEnum.EXTERNAL.equals(catalog.getCatalogType())); - if (!allowEscape && baseLocation != null) { + boolean allowEscape = CallContext + .getCurrentContext() + .getPolarisCallContext() + .getConfigurationStore() + .getConfiguration( + CallContext.getCurrentContext().getPolarisCallContext(), + catalog, + PolarisConfiguration.ALLOW_EXTERNAL_METADATA_FILE_LOCATION); + if (!allowEscape && catalog.getCatalogType() != Catalog.TypeEnum.EXTERNAL && baseLocation != null) { LOGGER.debug( "Not allowing unstructured table location for entity: {}", entityPath.getLast().getName()); diff --git a/polaris-service/src/main/java/io/polaris/service/admin/PolarisAdminService.java b/polaris-service/src/main/java/io/polaris/service/admin/PolarisAdminService.java index 119caad618..55c3768725 100644 --- a/polaris-service/src/main/java/io/polaris/service/admin/PolarisAdminService.java +++ b/polaris-service/src/main/java/io/polaris/service/admin/PolarisAdminService.java @@ -496,14 +496,11 @@ private String terminateWithSlash(String path) { */ private boolean catalogOverlapsWithExistingCatalog(CatalogEntity catalogEntity) { boolean allowOverlappingCatalogUrls = - Boolean.parseBoolean( - String.valueOf( - getCurrentPolarisContext() - .getConfigurationStore() - .getConfiguration( - getCurrentPolarisContext(), - PolarisConfiguration.ALLOW_OVERLAPPING_CATALOG_URLS, - PolarisConfiguration.DEFAULT_ALLOW_OVERLAPPING_CATALOG_URLS))); + getCurrentPolarisContext() + .getConfigurationStore() + .getConfiguration( + getCurrentPolarisContext(), + PolarisConfiguration.ALLOW_OVERLAPPING_CATALOG_URLS); if (allowOverlappingCatalogUrls) { return false; diff --git a/polaris-service/src/main/java/io/polaris/service/catalog/BasePolarisCatalog.java b/polaris-service/src/main/java/io/polaris/service/catalog/BasePolarisCatalog.java index 982ecf6c6e..6556fcad81 100644 --- a/polaris-service/src/main/java/io/polaris/service/catalog/BasePolarisCatalog.java +++ b/polaris-service/src/main/java/io/polaris/service/catalog/BasePolarisCatalog.java @@ -435,8 +435,7 @@ private void createNamespaceInternal( .getConfigurationStore() .getConfiguration( callContext.getPolarisCallContext(), - PolarisConfiguration.ALLOW_NAMESPACE_LOCATION_OVERLAP, - PolarisConfiguration.DEFAULT_ALLOW_NAMESPACE_LOCATION_OVERLAP)) { + PolarisConfiguration.ALLOW_NAMESPACE_LOCATION_OVERLAP)) { LOG.debug("Validating no overlap for {} with sibling tables or namespaces", namespace); validateNoLocationOverlap( entity.getBaseLocation(), resolvedParent.getRawFullPath(), entity.getName()); @@ -585,8 +584,7 @@ public boolean setProperties(Namespace namespace, Map properties .getConfigurationStore() .getConfiguration( callContext.getPolarisCallContext(), - PolarisConfiguration.ALLOW_NAMESPACE_LOCATION_OVERLAP, - PolarisConfiguration.DEFAULT_ALLOW_NAMESPACE_LOCATION_OVERLAP)) { + PolarisConfiguration.ALLOW_NAMESPACE_LOCATION_OVERLAP)) { LOG.debug("Validating no overlap with sibling tables or namespaces"); validateNoLocationOverlap( NamespaceEntity.of(updatedEntity).getBaseLocation(), @@ -924,8 +922,7 @@ private void validateNoLocationOverlap( .getConfigurationStore() .getConfiguration( callContext.getPolarisCallContext(), - PolarisConfiguration.ALLOW_TABLE_LOCATION_OVERLAP, - PolarisConfiguration.DEFAULT_ALLOW_TABLE_LOCATION_OVERLAP)) { + PolarisConfiguration.ALLOW_TABLE_LOCATION_OVERLAP)) { LOG.debug("Skipping location overlap validation for identifier '{}'", identifier); } else { // if (entity.getSubType().equals(PolarisEntitySubType.TABLE)) { // TODO - is this necessary for views? overlapping views do not expose subdirectories via the @@ -1270,17 +1267,17 @@ protected String tableName() { private void validateMetadataFileInTableDir( TableIdentifier identifier, TableMetadata metadata, CatalogEntity catalog) { PolarisCallContext polarisCallContext = callContext.getPolarisCallContext(); - String allowEscape = - catalog - .getPropertiesAsMap() - .get(PolarisConfiguration.CATALOG_ALLOW_EXTERNAL_TABLE_LOCATION); - if (!Boolean.parseBoolean(allowEscape) + boolean allowEscape = polarisCallContext + .getConfigurationStore() + .getConfiguration( + polarisCallContext, + PolarisConfiguration.ALLOW_EXTERNAL_TABLE_LOCATION); + if (!allowEscape && !polarisCallContext .getConfigurationStore() .getConfiguration( polarisCallContext, - PolarisConfiguration.ALLOW_EXTERNAL_METADATA_FILE_LOCATION, - PolarisConfiguration.DEFAULT_ALLOW_EXTERNAL_METADATA_FILE_LOCATION)) { + PolarisConfiguration.ALLOW_EXTERNAL_METADATA_FILE_LOCATION)) { LOG.debug( "Validating base location {} for table {} in metadata file {}", metadata.location(), diff --git a/polaris-service/src/test/java/io/polaris/service/catalog/BasePolarisCatalogTest.java b/polaris-service/src/test/java/io/polaris/service/catalog/BasePolarisCatalogTest.java index 6ca1957230..5cf7b94aec 100644 --- a/polaris-service/src/test/java/io/polaris/service/catalog/BasePolarisCatalogTest.java +++ b/polaris-service/src/test/java/io/polaris/service/catalog/BasePolarisCatalogTest.java @@ -187,8 +187,8 @@ public void before() { .setName(CATALOG_NAME) .setDefaultBaseLocation(storageLocation) .setReplaceNewLocationPrefixWithCatalogDefault("file:") - .addProperty(PolarisConfiguration.CATALOG_ALLOW_EXTERNAL_TABLE_LOCATION, "true") - .addProperty(PolarisConfiguration.CATALOG_ALLOW_UNSTRUCTURED_TABLE_LOCATION, "true") + .addProperty(PolarisConfiguration.ALLOW_EXTERNAL_TABLE_LOCATION.catalogConfig(), "true") + .addProperty(PolarisConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), "true") .setStorageConfigurationInfo(storageConfigModel, storageLocation) .build()); @@ -413,8 +413,8 @@ public void testCreateNotificationCreateTableInExternalLocation() { polarisContext, List.of(PolarisEntity.toCore(catalogEntity)), new CatalogEntity.Builder(CatalogEntity.of(catalogEntity)) - .addProperty(PolarisConfiguration.CATALOG_ALLOW_EXTERNAL_TABLE_LOCATION, "false") - .addProperty(PolarisConfiguration.CATALOG_ALLOW_UNSTRUCTURED_TABLE_LOCATION, "true") + .addProperty(PolarisConfiguration.ALLOW_EXTERNAL_TABLE_LOCATION.catalogConfig(), "false") + .addProperty(PolarisConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), "true") .build()); BasePolarisCatalog catalog = catalog(); TableMetadata tableMetadata = @@ -470,8 +470,8 @@ public void testCreateNotificationCreateTableOutsideOfMetadataLocation() { polarisContext, List.of(PolarisEntity.toCore(catalogEntity)), new CatalogEntity.Builder(CatalogEntity.of(catalogEntity)) - .addProperty(PolarisConfiguration.CATALOG_ALLOW_EXTERNAL_TABLE_LOCATION, "false") - .addProperty(PolarisConfiguration.CATALOG_ALLOW_UNSTRUCTURED_TABLE_LOCATION, "true") + .addProperty(PolarisConfiguration.ALLOW_EXTERNAL_TABLE_LOCATION.catalogConfig(), "false") + .addProperty(PolarisConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), "true") .build()); BasePolarisCatalog catalog = catalog(); TableMetadata tableMetadata = @@ -524,8 +524,8 @@ public void testUpdateNotificationCreateTableInExternalLocation() { polarisContext, List.of(PolarisEntity.toCore(catalogEntity)), new CatalogEntity.Builder(CatalogEntity.of(catalogEntity)) - .addProperty(PolarisConfiguration.CATALOG_ALLOW_EXTERNAL_TABLE_LOCATION, "false") - .addProperty(PolarisConfiguration.CATALOG_ALLOW_UNSTRUCTURED_TABLE_LOCATION, "true") + .addProperty(PolarisConfiguration.ALLOW_EXTERNAL_TABLE_LOCATION.catalogConfig(), "false") + .addProperty(PolarisConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), "true") .build()); BasePolarisCatalog catalog = catalog(); InMemoryFileIO fileIO = (InMemoryFileIO) catalog.getIo(); diff --git a/polaris-service/src/test/java/io/polaris/service/catalog/BasePolarisCatalogViewTest.java b/polaris-service/src/test/java/io/polaris/service/catalog/BasePolarisCatalogViewTest.java index 4d7ba998e8..8c175a33c9 100644 --- a/polaris-service/src/test/java/io/polaris/service/catalog/BasePolarisCatalogViewTest.java +++ b/polaris-service/src/test/java/io/polaris/service/catalog/BasePolarisCatalogViewTest.java @@ -112,8 +112,8 @@ public void before() { adminService.createCatalog( new CatalogEntity.Builder() .setName(CATALOG_NAME) - .addProperty(PolarisConfiguration.CATALOG_ALLOW_EXTERNAL_TABLE_LOCATION, "true") - .addProperty(PolarisConfiguration.CATALOG_ALLOW_UNSTRUCTURED_TABLE_LOCATION, "true") + .addProperty(PolarisConfiguration.ALLOW_EXTERNAL_TABLE_LOCATION.catalogConfig(), "true") + .addProperty(PolarisConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), "true") .setDefaultBaseLocation("file://tmp") .setStorageConfigurationInfo( new FileStorageConfigInfo( diff --git a/polaris-service/src/test/java/io/polaris/service/catalog/PolarisRestCatalogIntegrationTest.java b/polaris-service/src/test/java/io/polaris/service/catalog/PolarisRestCatalogIntegrationTest.java index a60d9f404b..9e8272d3ee 100644 --- a/polaris-service/src/test/java/io/polaris/service/catalog/PolarisRestCatalogIntegrationTest.java +++ b/polaris-service/src/test/java/io/polaris/service/catalog/PolarisRestCatalogIntegrationTest.java @@ -178,9 +178,9 @@ public void before( io.polaris.core.admin.model.CatalogProperties.Builder catalogPropsBuilder = io.polaris.core.admin.model.CatalogProperties.builder(catalogBaseLocation) .addProperty( - PolarisConfiguration.CATALOG_ALLOW_UNSTRUCTURED_TABLE_LOCATION, "true") + PolarisConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), "true") .addProperty( - PolarisConfiguration.CATALOG_ALLOW_EXTERNAL_TABLE_LOCATION, "true"); + PolarisConfiguration.ALLOW_EXTERNAL_TABLE_LOCATION.catalogConfig(), "true"); if (!S3_BUCKET_BASE.startsWith("file:/")) { catalogPropsBuilder.addProperty( CatalogEntity.REPLACE_NEW_LOCATION_PREFIX_WITH_CATALOG_DEFAULT_KEY, "file:"); @@ -593,7 +593,7 @@ public void testCreateTableWithOverriddenBaseLocation(PolarisToken adminToken) { assertThat(response).returns(Response.Status.OK.getStatusCode(), Response::getStatus); Catalog catalog = response.readEntity(Catalog.class); Map catalogProps = new HashMap<>(catalog.getProperties().toMap()); - catalogProps.put(PolarisConfiguration.CATALOG_ALLOW_UNSTRUCTURED_TABLE_LOCATION, "false"); + catalogProps.put(PolarisConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), "false"); try (Response updateResponse = EXT.client() .target( @@ -649,7 +649,7 @@ public void testCreateTableWithOverriddenBaseLocationCannotOverlapSibling( assertThat(response).returns(Response.Status.OK.getStatusCode(), Response::getStatus); Catalog catalog = response.readEntity(Catalog.class); Map catalogProps = new HashMap<>(catalog.getProperties().toMap()); - catalogProps.put(PolarisConfiguration.CATALOG_ALLOW_UNSTRUCTURED_TABLE_LOCATION, "false"); + catalogProps.put(PolarisConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), "false"); try (Response updateResponse = EXT.client() .target( @@ -714,7 +714,7 @@ public void testCreateTableWithOverriddenBaseLocationMustResideInNsDirectory( assertThat(response).returns(Response.Status.OK.getStatusCode(), Response::getStatus); Catalog catalog = response.readEntity(Catalog.class); Map catalogProps = new HashMap<>(catalog.getProperties().toMap()); - catalogProps.put(PolarisConfiguration.CATALOG_ALLOW_UNSTRUCTURED_TABLE_LOCATION, "false"); + catalogProps.put(PolarisConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), "false"); try (Response updateResponse = EXT.client() .target( diff --git a/polaris-service/src/test/java/io/polaris/service/catalog/PolarisRestCatalogViewIntegrationTest.java b/polaris-service/src/test/java/io/polaris/service/catalog/PolarisRestCatalogViewIntegrationTest.java index f99cbb32b1..cd8a9d42a3 100644 --- a/polaris-service/src/test/java/io/polaris/service/catalog/PolarisRestCatalogViewIntegrationTest.java +++ b/polaris-service/src/test/java/io/polaris/service/catalog/PolarisRestCatalogViewIntegrationTest.java @@ -156,9 +156,9 @@ public void before( CatalogEntity.REPLACE_NEW_LOCATION_PREFIX_WITH_CATALOG_DEFAULT_KEY, "file:") .addProperty( - PolarisConfiguration.CATALOG_ALLOW_EXTERNAL_TABLE_LOCATION, "true") + PolarisConfiguration.ALLOW_EXTERNAL_TABLE_LOCATION.catalogConfig(), "true") .addProperty( - PolarisConfiguration.CATALOG_ALLOW_UNSTRUCTURED_TABLE_LOCATION, "true") + PolarisConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), "true") .build(); Catalog catalog = PolarisCatalog.builder() From 675e852ba24fdde53dd56c1610875e859d8384f9 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Mon, 5 Aug 2024 21:44:58 -0700 Subject: [PATCH 5/7] stabilize tests --- .../polaris/core/storage/PolarisStorageConfigurationInfo.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/polaris-core/src/main/java/io/polaris/core/storage/PolarisStorageConfigurationInfo.java b/polaris-core/src/main/java/io/polaris/core/storage/PolarisStorageConfigurationInfo.java index d8fc50aaf2..74f80681c0 100644 --- a/polaris-core/src/main/java/io/polaris/core/storage/PolarisStorageConfigurationInfo.java +++ b/polaris-core/src/main/java/io/polaris/core/storage/PolarisStorageConfigurationInfo.java @@ -154,7 +154,7 @@ public static Optional forEntityPath( .getConfiguration( CallContext.getCurrentContext().getPolarisCallContext(), catalog, - PolarisConfiguration.ALLOW_EXTERNAL_METADATA_FILE_LOCATION); + PolarisConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION); if (!allowEscape && catalog.getCatalogType() != Catalog.TypeEnum.EXTERNAL && baseLocation != null) { LOGGER.debug( "Not allowing unstructured table location for entity: {}", From 0747ee8d10b09174e431a7aed262c034c53636d8 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Tue, 6 Aug 2024 09:43:43 -0700 Subject: [PATCH 6/7] nit --- .../main/java/io/polaris/core/PolarisConfigurationStore.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/polaris-core/src/main/java/io/polaris/core/PolarisConfigurationStore.java b/polaris-core/src/main/java/io/polaris/core/PolarisConfigurationStore.java index 63fb384149..009ff36a58 100644 --- a/polaris-core/src/main/java/io/polaris/core/PolarisConfigurationStore.java +++ b/polaris-core/src/main/java/io/polaris/core/PolarisConfigurationStore.java @@ -29,7 +29,7 @@ * request. */ public interface PolarisConfigurationStore { - final Logger LOGGER = LoggerFactory.getLogger(PolarisConfigurationStore.class); + Logger LOGGER = LoggerFactory.getLogger(PolarisConfigurationStore.class); /** * Retrieve the current value for a configuration key. May be null if not set. From e389095a32f4b7d4b30e4c1a54acca38904308b3 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Tue, 6 Aug 2024 09:47:42 -0700 Subject: [PATCH 7/7] lint --- .../io/polaris/core/PolarisConfiguration.java | 40 ++++++++----------- .../core/PolarisConfigurationStore.java | 23 +++++------ .../PolarisStorageConfigurationInfo.java | 20 +++++----- .../service/admin/PolarisAdminService.java | 3 +- .../service/catalog/BasePolarisCatalog.java | 13 +++--- .../catalog/BasePolarisCatalogTest.java | 24 +++++++---- .../catalog/BasePolarisCatalogViewTest.java | 3 +- .../PolarisRestCatalogIntegrationTest.java | 15 ++++--- ...PolarisRestCatalogViewIntegrationTest.java | 6 ++- 9 files changed, 77 insertions(+), 70 deletions(-) diff --git a/polaris-core/src/main/java/io/polaris/core/PolarisConfiguration.java b/polaris-core/src/main/java/io/polaris/core/PolarisConfiguration.java index d04d63817e..ed8a4221d6 100644 --- a/polaris-core/src/main/java/io/polaris/core/PolarisConfiguration.java +++ b/polaris-core/src/main/java/io/polaris/core/PolarisConfiguration.java @@ -26,7 +26,8 @@ public class PolarisConfiguration { private final Class typ; @SuppressWarnings("unchecked") - public PolarisConfiguration(String key, String description, T defaultValue, Optional catalogConfig) { + public PolarisConfiguration( + String key, String description, T defaultValue, Optional catalogConfig) { this.key = key; this.description = description; this.defaultValue = defaultValue; @@ -39,10 +40,10 @@ public boolean hasCatalogConfig() { } public String catalogConfig() { - return catalogConfigImpl.orElseThrow(() -> - new IllegalStateException( - "Attempted to read a catalog config key from a configuration that doesn't have one.") - ); + return catalogConfigImpl.orElseThrow( + () -> + new IllegalStateException( + "Attempted to read a catalog config key from a configuration that doesn't have one.")); } T cast(Object value) { @@ -92,9 +93,8 @@ public static Builder builder() { PolarisConfiguration.builder() .key("ENFORCE_PRINCIPAL_CREDENTIAL_ROTATION_REQUIRED_CHECKING") .description( - "If set to true, require that principals must rotate their credentials before being used " + - "for anything else." - ) + "If set to true, require that principals must rotate their credentials before being used " + + "for anything else.") .defaultValue(false) .build(); @@ -102,9 +102,8 @@ public static Builder builder() { PolarisConfiguration.builder() .key("ALLOW_TABLE_LOCATION_OVERLAP") .description( - "If set to true, allow one table's location to reside within another table's location. " + - "This is only enforced within a given namespace." - ) + "If set to true, allow one table's location to reside within another table's location. " + + "This is only enforced within a given namespace.") .defaultValue(false) .build(); @@ -112,9 +111,8 @@ public static Builder builder() { PolarisConfiguration.builder() .key("ALLOW_NAMESPACE_LOCATION_OVERLAP") .description( - "If set to true, allow one table's location to reside within another table's location. " + - "This is only enforced within a parent catalog or namespace." - ) + "If set to true, allow one table's location to reside within another table's location. " + + "This is only enforced within a parent catalog or namespace.") .defaultValue(false) .build(); @@ -122,17 +120,14 @@ public static Builder builder() { PolarisConfiguration.builder() .key("ALLOW_EXTERNAL_METADATA_FILE_LOCATION") .description( - "If set to true, allows metadata files to be located outside the default metadata directory." - ) + "If set to true, allows metadata files to be located outside the default metadata directory.") .defaultValue(false) .build(); public static final PolarisConfiguration ALLOW_OVERLAPPING_CATALOG_URLS = PolarisConfiguration.builder() .key("ALLOW_OVERLAPPING_CATALOG_URLS") - .description( - "If set to true, allows catalog URLs to overlap." - ) + .description("If set to true, allows catalog URLs to overlap.") .defaultValue(false) .build(); @@ -140,9 +135,7 @@ public static Builder builder() { PolarisConfiguration.builder() .key("ALLOW_UNSTRUCTURED_TABLE_LOCATION") .catalogConfig("allow.unstructured.table.location") - .description( - "If set to true, allows unstructured table locations." - ) + .description("If set to true, allows unstructured table locations.") .defaultValue(false) .build(); @@ -151,8 +144,7 @@ public static Builder builder() { .key("ALLOW_EXTERNAL_TABLE_LOCATION") .catalogConfig("allow.external.table.location") .description( - "If set to true, allows tables to have external locations outside the default structure." - ) + "If set to true, allows tables to have external locations outside the default structure.") .defaultValue(false) .build(); } diff --git a/polaris-core/src/main/java/io/polaris/core/PolarisConfigurationStore.java b/polaris-core/src/main/java/io/polaris/core/PolarisConfigurationStore.java index 009ff36a58..3d697c7ec7 100644 --- a/polaris-core/src/main/java/io/polaris/core/PolarisConfigurationStore.java +++ b/polaris-core/src/main/java/io/polaris/core/PolarisConfigurationStore.java @@ -17,12 +17,10 @@ import com.google.common.base.Preconditions; import io.polaris.core.entity.CatalogEntity; -import io.polaris.core.storage.PolarisStorageConfigurationInfo; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import software.amazon.awssdk.services.s3.endpoints.internal.Value; /** * Dynamic configuration store used to retrieve runtime parameters, which may vary by realm or by @@ -61,8 +59,8 @@ public interface PolarisConfigurationStore { } /** - * In some cases, we may extract a value that doesn't match the expected type for a config. - * This method can be used to attempt to force-cast it using `String.valueOf` + * In some cases, we may extract a value that doesn't match the expected type for a config. This + * method can be used to attempt to force-cast it using `String.valueOf` */ private @NotNull T tryCast(PolarisConfiguration config, Object value) { if (value == null) { @@ -84,14 +82,14 @@ public interface PolarisConfigurationStore { * @return the current value set for the configuration key or null if not set * @param the type of the configuration value */ - default @NotNull T getConfiguration( - PolarisCallContext ctx, PolarisConfiguration config) { + default @NotNull T getConfiguration(PolarisCallContext ctx, PolarisConfiguration config) { T result = getConfiguration(ctx, config.key, config.defaultValue); return tryCast(config, result); } /** - * Retrieve the current value for a configuration, overriding with a catalog config if it is present. + * Retrieve the current value for a configuration, overriding with a catalog config if it is + * present. * * @param ctx the current call context * @param catalogEntity the catalog to check for an override @@ -100,11 +98,12 @@ public interface PolarisConfigurationStore { * @param the type of the configuration value */ default @NotNull T getConfiguration( - PolarisCallContext ctx, @NotNull CatalogEntity catalogEntity, PolarisConfiguration config) { - if (config.hasCatalogConfig() && catalogEntity.getPropertiesAsMap().containsKey(config.catalogConfig())) { - LOGGER.debug( - "Loaded config from catalog: {}", - config.catalogConfig()); + PolarisCallContext ctx, + @NotNull CatalogEntity catalogEntity, + PolarisConfiguration config) { + if (config.hasCatalogConfig() + && catalogEntity.getPropertiesAsMap().containsKey(config.catalogConfig())) { + LOGGER.debug("Loaded config from catalog: {}", config.catalogConfig()); return tryCast(config, catalogEntity.getPropertiesAsMap().get(config.catalogConfig())); } else { return getConfiguration(ctx, config); diff --git a/polaris-core/src/main/java/io/polaris/core/storage/PolarisStorageConfigurationInfo.java b/polaris-core/src/main/java/io/polaris/core/storage/PolarisStorageConfigurationInfo.java index 74f80681c0..c10c8e6a68 100644 --- a/polaris-core/src/main/java/io/polaris/core/storage/PolarisStorageConfigurationInfo.java +++ b/polaris-core/src/main/java/io/polaris/core/storage/PolarisStorageConfigurationInfo.java @@ -147,15 +147,17 @@ public static Optional forEntityPath( .findFirst() .orElse(null); CatalogEntity catalog = CatalogEntity.of(entityPath.get(0)); - boolean allowEscape = CallContext - .getCurrentContext() - .getPolarisCallContext() - .getConfigurationStore() - .getConfiguration( - CallContext.getCurrentContext().getPolarisCallContext(), - catalog, - PolarisConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION); - if (!allowEscape && catalog.getCatalogType() != Catalog.TypeEnum.EXTERNAL && baseLocation != null) { + boolean allowEscape = + CallContext.getCurrentContext() + .getPolarisCallContext() + .getConfigurationStore() + .getConfiguration( + CallContext.getCurrentContext().getPolarisCallContext(), + catalog, + PolarisConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION); + if (!allowEscape + && catalog.getCatalogType() != Catalog.TypeEnum.EXTERNAL + && baseLocation != null) { LOGGER.debug( "Not allowing unstructured table location for entity: {}", entityPath.getLast().getName()); diff --git a/polaris-service/src/main/java/io/polaris/service/admin/PolarisAdminService.java b/polaris-service/src/main/java/io/polaris/service/admin/PolarisAdminService.java index f853d069d4..609e86eeaf 100644 --- a/polaris-service/src/main/java/io/polaris/service/admin/PolarisAdminService.java +++ b/polaris-service/src/main/java/io/polaris/service/admin/PolarisAdminService.java @@ -499,8 +499,7 @@ private boolean catalogOverlapsWithExistingCatalog(CatalogEntity catalogEntity) getCurrentPolarisContext() .getConfigurationStore() .getConfiguration( - getCurrentPolarisContext(), - PolarisConfiguration.ALLOW_OVERLAPPING_CATALOG_URLS); + getCurrentPolarisContext(), PolarisConfiguration.ALLOW_OVERLAPPING_CATALOG_URLS); if (allowOverlappingCatalogUrls) { return false; diff --git a/polaris-service/src/main/java/io/polaris/service/catalog/BasePolarisCatalog.java b/polaris-service/src/main/java/io/polaris/service/catalog/BasePolarisCatalog.java index 2e9af6b38f..ff3a4ac175 100644 --- a/polaris-service/src/main/java/io/polaris/service/catalog/BasePolarisCatalog.java +++ b/polaris-service/src/main/java/io/polaris/service/catalog/BasePolarisCatalog.java @@ -1253,17 +1253,16 @@ protected String tableName() { private void validateMetadataFileInTableDir( TableIdentifier identifier, TableMetadata metadata, CatalogEntity catalog) { PolarisCallContext polarisCallContext = callContext.getPolarisCallContext(); - boolean allowEscape = polarisCallContext - .getConfigurationStore() - .getConfiguration( - polarisCallContext, - PolarisConfiguration.ALLOW_EXTERNAL_TABLE_LOCATION); + boolean allowEscape = + polarisCallContext + .getConfigurationStore() + .getConfiguration( + polarisCallContext, PolarisConfiguration.ALLOW_EXTERNAL_TABLE_LOCATION); if (!allowEscape && !polarisCallContext .getConfigurationStore() .getConfiguration( - polarisCallContext, - PolarisConfiguration.ALLOW_EXTERNAL_METADATA_FILE_LOCATION)) { + polarisCallContext, PolarisConfiguration.ALLOW_EXTERNAL_METADATA_FILE_LOCATION)) { LOG.debug( "Validating base location {} for table {} in metadata file {}", metadata.location(), diff --git a/polaris-service/src/test/java/io/polaris/service/catalog/BasePolarisCatalogTest.java b/polaris-service/src/test/java/io/polaris/service/catalog/BasePolarisCatalogTest.java index 7d8f570565..a410d81eae 100644 --- a/polaris-service/src/test/java/io/polaris/service/catalog/BasePolarisCatalogTest.java +++ b/polaris-service/src/test/java/io/polaris/service/catalog/BasePolarisCatalogTest.java @@ -187,8 +187,10 @@ public void before() { .setName(CATALOG_NAME) .setDefaultBaseLocation(storageLocation) .setReplaceNewLocationPrefixWithCatalogDefault("file:") - .addProperty(PolarisConfiguration.ALLOW_EXTERNAL_TABLE_LOCATION.catalogConfig(), "true") - .addProperty(PolarisConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), "true") + .addProperty( + PolarisConfiguration.ALLOW_EXTERNAL_TABLE_LOCATION.catalogConfig(), "true") + .addProperty( + PolarisConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), "true") .setStorageConfigurationInfo(storageConfigModel, storageLocation) .build()); @@ -413,8 +415,10 @@ public void testCreateNotificationCreateTableInExternalLocation() { polarisContext, List.of(PolarisEntity.toCore(catalogEntity)), new CatalogEntity.Builder(CatalogEntity.of(catalogEntity)) - .addProperty(PolarisConfiguration.ALLOW_EXTERNAL_TABLE_LOCATION.catalogConfig(), "false") - .addProperty(PolarisConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), "true") + .addProperty( + PolarisConfiguration.ALLOW_EXTERNAL_TABLE_LOCATION.catalogConfig(), "false") + .addProperty( + PolarisConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), "true") .build()); BasePolarisCatalog catalog = catalog(); TableMetadata tableMetadata = @@ -470,8 +474,10 @@ public void testCreateNotificationCreateTableOutsideOfMetadataLocation() { polarisContext, List.of(PolarisEntity.toCore(catalogEntity)), new CatalogEntity.Builder(CatalogEntity.of(catalogEntity)) - .addProperty(PolarisConfiguration.ALLOW_EXTERNAL_TABLE_LOCATION.catalogConfig(), "false") - .addProperty(PolarisConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), "true") + .addProperty( + PolarisConfiguration.ALLOW_EXTERNAL_TABLE_LOCATION.catalogConfig(), "false") + .addProperty( + PolarisConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), "true") .build()); BasePolarisCatalog catalog = catalog(); TableMetadata tableMetadata = @@ -524,8 +530,10 @@ public void testUpdateNotificationCreateTableInExternalLocation() { polarisContext, List.of(PolarisEntity.toCore(catalogEntity)), new CatalogEntity.Builder(CatalogEntity.of(catalogEntity)) - .addProperty(PolarisConfiguration.ALLOW_EXTERNAL_TABLE_LOCATION.catalogConfig(), "false") - .addProperty(PolarisConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), "true") + .addProperty( + PolarisConfiguration.ALLOW_EXTERNAL_TABLE_LOCATION.catalogConfig(), "false") + .addProperty( + PolarisConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), "true") .build()); BasePolarisCatalog catalog = catalog(); InMemoryFileIO fileIO = (InMemoryFileIO) catalog.getIo(); diff --git a/polaris-service/src/test/java/io/polaris/service/catalog/BasePolarisCatalogViewTest.java b/polaris-service/src/test/java/io/polaris/service/catalog/BasePolarisCatalogViewTest.java index e6689fca43..7bc9531433 100644 --- a/polaris-service/src/test/java/io/polaris/service/catalog/BasePolarisCatalogViewTest.java +++ b/polaris-service/src/test/java/io/polaris/service/catalog/BasePolarisCatalogViewTest.java @@ -112,7 +112,8 @@ public void before() { new CatalogEntity.Builder() .setName(CATALOG_NAME) .addProperty(PolarisConfiguration.ALLOW_EXTERNAL_TABLE_LOCATION.catalogConfig(), "true") - .addProperty(PolarisConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), "true") + .addProperty( + PolarisConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), "true") .setDefaultBaseLocation("file://tmp") .setStorageConfigurationInfo( new FileStorageConfigInfo( diff --git a/polaris-service/src/test/java/io/polaris/service/catalog/PolarisRestCatalogIntegrationTest.java b/polaris-service/src/test/java/io/polaris/service/catalog/PolarisRestCatalogIntegrationTest.java index 9e8272d3ee..ac07812bd8 100644 --- a/polaris-service/src/test/java/io/polaris/service/catalog/PolarisRestCatalogIntegrationTest.java +++ b/polaris-service/src/test/java/io/polaris/service/catalog/PolarisRestCatalogIntegrationTest.java @@ -178,9 +178,11 @@ public void before( io.polaris.core.admin.model.CatalogProperties.Builder catalogPropsBuilder = io.polaris.core.admin.model.CatalogProperties.builder(catalogBaseLocation) .addProperty( - PolarisConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), "true") + PolarisConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), + "true") .addProperty( - PolarisConfiguration.ALLOW_EXTERNAL_TABLE_LOCATION.catalogConfig(), "true"); + PolarisConfiguration.ALLOW_EXTERNAL_TABLE_LOCATION.catalogConfig(), + "true"); if (!S3_BUCKET_BASE.startsWith("file:/")) { catalogPropsBuilder.addProperty( CatalogEntity.REPLACE_NEW_LOCATION_PREFIX_WITH_CATALOG_DEFAULT_KEY, "file:"); @@ -593,7 +595,8 @@ public void testCreateTableWithOverriddenBaseLocation(PolarisToken adminToken) { assertThat(response).returns(Response.Status.OK.getStatusCode(), Response::getStatus); Catalog catalog = response.readEntity(Catalog.class); Map catalogProps = new HashMap<>(catalog.getProperties().toMap()); - catalogProps.put(PolarisConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), "false"); + catalogProps.put( + PolarisConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), "false"); try (Response updateResponse = EXT.client() .target( @@ -649,7 +652,8 @@ public void testCreateTableWithOverriddenBaseLocationCannotOverlapSibling( assertThat(response).returns(Response.Status.OK.getStatusCode(), Response::getStatus); Catalog catalog = response.readEntity(Catalog.class); Map catalogProps = new HashMap<>(catalog.getProperties().toMap()); - catalogProps.put(PolarisConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), "false"); + catalogProps.put( + PolarisConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), "false"); try (Response updateResponse = EXT.client() .target( @@ -714,7 +718,8 @@ public void testCreateTableWithOverriddenBaseLocationMustResideInNsDirectory( assertThat(response).returns(Response.Status.OK.getStatusCode(), Response::getStatus); Catalog catalog = response.readEntity(Catalog.class); Map catalogProps = new HashMap<>(catalog.getProperties().toMap()); - catalogProps.put(PolarisConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), "false"); + catalogProps.put( + PolarisConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), "false"); try (Response updateResponse = EXT.client() .target( diff --git a/polaris-service/src/test/java/io/polaris/service/catalog/PolarisRestCatalogViewIntegrationTest.java b/polaris-service/src/test/java/io/polaris/service/catalog/PolarisRestCatalogViewIntegrationTest.java index cd8a9d42a3..1d7ebd8e1d 100644 --- a/polaris-service/src/test/java/io/polaris/service/catalog/PolarisRestCatalogViewIntegrationTest.java +++ b/polaris-service/src/test/java/io/polaris/service/catalog/PolarisRestCatalogViewIntegrationTest.java @@ -156,9 +156,11 @@ public void before( CatalogEntity.REPLACE_NEW_LOCATION_PREFIX_WITH_CATALOG_DEFAULT_KEY, "file:") .addProperty( - PolarisConfiguration.ALLOW_EXTERNAL_TABLE_LOCATION.catalogConfig(), "true") + PolarisConfiguration.ALLOW_EXTERNAL_TABLE_LOCATION.catalogConfig(), + "true") .addProperty( - PolarisConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), "true") + PolarisConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), + "true") .build(); Catalog catalog = PolarisCatalog.builder()