Re: [PR] feat(storage): inline polaris.storage.name overrides for namespace/table [polaris]
dimas-b commented on code in PR #4023:
URL: https://github.com/apache/polaris/pull/4023#discussion_r3397003140
##
polaris-core/src/main/java/org/apache/polaris/core/storage/StorageConfigOverrideResolver.java:
##
@@ -0,0 +1,83 @@
+/*
+ * 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 java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import org.apache.polaris.core.entity.PolarisEntity;
+import org.apache.polaris.core.entity.PolarisEntityConstants;
+import org.jspecify.annotations.NonNull;
+
+/**
+ * Resolves the effective {@link PolarisStorageConfigurationInfo} for an
entity hierarchy by
+ * applying any nearest-ancestor {@code storage_name_override} on top of the
catalog's base storage
+ * configuration.
+ *
+ * The input list is in root-to-leaf order (the catalog is at index 0). The
walk runs
+ * leaf-to-root and:
+ *
+ *
+ * Captures the first {@code storage_name_override} encountered (nearest
wins).
+ * Returns the first {@code storage_configuration_info} encountered —
the base config.
+ *
+ *
+ * If a base config exists and an override was captured, the result is
{@link
+ * PolarisStorageConfigurationInfo#withStorageName} applied to the base. If no
base config exists,
+ * the result is empty regardless of any override.
+ */
+public final class StorageConfigOverrideResolver {
+
+ private StorageConfigOverrideResolver() {}
+
+ /**
+ * Walk {@code entityPath} leaf-to-root and return the effective storage
configuration. The base
+ * config is sourced from the nearest entity that has {@code
storage_configuration_info} in its
+ * internal properties; if any closer ancestor (including the leaf itself)
carries {@code
+ * storage_name_override}, that name replaces the base config's {@code
storageName}.
+ *
+ * @param entityPath entities in root-to-leaf order; the catalog is at index 0
+ * @return effective config, or empty if no entity in the chain has a base
storage config
+ */
+ public static Optional
resolveEffectiveConfig(
+ @NonNull List entityPath) {
+String override = null;
+for (int i = entityPath.size() - 1; i >= 0; i--) {
+ Map internalProps =
entityPath.get(i).getInternalPropertiesAsMap();
+ if (override == null) {
+String candidate =
+
internalProps.get(PolarisEntityConstants.getStorageNameOverridePropertyName());
+if (candidate != null && !candidate.isEmpty()) {
+ override = candidate;
+}
+ }
+ String serializedBase =
+
internalProps.get(PolarisEntityConstants.getStorageConfigInfoPropertyName());
+ if (serializedBase != null) {
+PolarisStorageConfigurationInfo base =
+PolarisStorageConfigurationInfo.deserialize(serializedBase);
+if (override != null) {
+ return
Optional.of(PolarisStorageConfigurationInfo.withStorageName(base, override));
Review Comment:
Heads up: this PR is on the agenda for the Community Sync call today :wink:
--
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]
Re: [PR] feat(storage): inline polaris.storage.name overrides for namespace/table [polaris]
adutra commented on code in PR #4023:
URL: https://github.com/apache/polaris/pull/4023#discussion_r3396980960
##
polaris-core/src/main/java/org/apache/polaris/core/storage/StorageConfigOverrideResolver.java:
##
@@ -0,0 +1,83 @@
+/*
+ * 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 java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import org.apache.polaris.core.entity.PolarisEntity;
+import org.apache.polaris.core.entity.PolarisEntityConstants;
+import org.jspecify.annotations.NonNull;
+
+/**
+ * Resolves the effective {@link PolarisStorageConfigurationInfo} for an
entity hierarchy by
+ * applying any nearest-ancestor {@code storage_name_override} on top of the
catalog's base storage
+ * configuration.
+ *
+ * The input list is in root-to-leaf order (the catalog is at index 0). The
walk runs
+ * leaf-to-root and:
+ *
+ *
+ * Captures the first {@code storage_name_override} encountered (nearest
wins).
+ * Returns the first {@code storage_configuration_info} encountered —
the base config.
+ *
+ *
+ * If a base config exists and an override was captured, the result is
{@link
+ * PolarisStorageConfigurationInfo#withStorageName} applied to the base. If no
base config exists,
+ * the result is empty regardless of any override.
+ */
+public final class StorageConfigOverrideResolver {
+
+ private StorageConfigOverrideResolver() {}
+
+ /**
+ * Walk {@code entityPath} leaf-to-root and return the effective storage
configuration. The base
+ * config is sourced from the nearest entity that has {@code
storage_configuration_info} in its
+ * internal properties; if any closer ancestor (including the leaf itself)
carries {@code
+ * storage_name_override}, that name replaces the base config's {@code
storageName}.
+ *
+ * @param entityPath entities in root-to-leaf order; the catalog is at index 0
+ * @return effective config, or empty if no entity in the chain has a base
storage config
+ */
+ public static Optional
resolveEffectiveConfig(
+ @NonNull List entityPath) {
+String override = null;
+for (int i = entityPath.size() - 1; i >= 0; i--) {
+ Map internalProps =
entityPath.get(i).getInternalPropertiesAsMap();
+ if (override == null) {
+String candidate =
+
internalProps.get(PolarisEntityConstants.getStorageNameOverridePropertyName());
+if (candidate != null && !candidate.isEmpty()) {
+ override = candidate;
+}
+ }
+ String serializedBase =
+
internalProps.get(PolarisEntityConstants.getStorageConfigInfoPropertyName());
+ if (serializedBase != null) {
+PolarisStorageConfigurationInfo base =
+PolarisStorageConfigurationInfo.deserialize(serializedBase);
+if (override != null) {
+ return
Optional.of(PolarisStorageConfigurationInfo.withStorageName(base, override));
Review Comment:
Exactly, I don't think it's a good idea to let users define the storage
profile name on a namespace or table in a managed environment.
> I remember that introducing a separate crud resource for storages was
rejected
You mean for storage profile names or for `PolarisStorageConfigurationInfo`?
I think we need to settle on this topic; I am leaning more towards the idea of
leveraging the management API. That's the direction suggested in [this design
doc](https://docs.google.com/document/d/1hbDkE-w84Pn_112iW2vCnlDKPDtyg8flaYcFGjvD120/edit?tab=t.0)
by @sririshindra.
--
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]
Re: [PR] feat(storage): inline polaris.storage.name overrides for namespace/table [polaris]
dimas-b commented on code in PR #4023:
URL: https://github.com/apache/polaris/pull/4023#discussion_r3376611787
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/io/FileIOUtil.java:
##
@@ -18,35 +18,57 @@
*/
package org.apache.polaris.service.catalog.io;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
import java.util.Optional;
import org.apache.polaris.core.entity.PolarisEntity;
import org.apache.polaris.core.entity.PolarisEntityConstants;
import org.apache.polaris.core.persistence.PolarisResolvedPathWrapper;
+import org.apache.polaris.core.storage.PolarisStorageConfigurationInfo;
+import org.apache.polaris.core.storage.StorageConfigOverrideResolver;
public class FileIOUtil {
private FileIOUtil() {}
/**
- * Finds storage configuration information in the hierarchy of the resolved
storage entity.
+ * Finds storage configuration information in the hierarchy of the resolved
storage entity, with
+ * any nearest-ancestor {@code storage_name_override} applied to the
catalog's base config.
*
- * This method starts at the "leaf" level (e.g., table) and walks
"upwards" through namespaces
- * in the hierarchy to the "root." It searches for the first entity
containing storage config
- * properties, identified using a key from {@link
- * PolarisEntityConstants#getStorageConfigInfoPropertyName()}.
+ * This method walks the path leaf-to-root, locates the first entity
carrying {@code
+ * storage_configuration_info} (the catalog), and — if any closer ancestor
carries a {@code
+ * storage_name_override} — returns a synthetic copy of the base entity
whose serialized
+ * configuration has its {@code storageName} replaced. Callers that read the
resulting entity's
+ * internal properties (e.g., to stash on a purge task) therefore see the
effective config.
*
* @param resolvedStorageEntity the resolved entity wrapper containing the
hierarchical path
- * @return an {@link Optional} containing the entity with storage config, or
empty if not found
+ * @return an {@link Optional} containing the (possibly synthetic) entity
with the effective
+ * storage config, or empty if no entity in the chain has a base storage
config
Review Comment:
Callers of this method only need to know the presence of this entity and if
present, its properties... Why bother with synthetic entities? I'd propose to
refactor this code to return an `Optional`... 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]
Re: [PR] feat(storage): inline polaris.storage.name overrides for namespace/table [polaris]
dimas-b commented on code in PR #4023:
URL: https://github.com/apache/polaris/pull/4023#discussion_r3396932850
##
polaris-core/src/main/java/org/apache/polaris/core/storage/StorageConfigOverrideResolver.java:
##
@@ -0,0 +1,83 @@
+/*
+ * 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 java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import org.apache.polaris.core.entity.PolarisEntity;
+import org.apache.polaris.core.entity.PolarisEntityConstants;
+import org.jspecify.annotations.NonNull;
+
+/**
+ * Resolves the effective {@link PolarisStorageConfigurationInfo} for an
entity hierarchy by
+ * applying any nearest-ancestor {@code storage_name_override} on top of the
catalog's base storage
+ * configuration.
+ *
+ * The input list is in root-to-leaf order (the catalog is at index 0). The
walk runs
+ * leaf-to-root and:
+ *
+ *
+ * Captures the first {@code storage_name_override} encountered (nearest
wins).
+ * Returns the first {@code storage_configuration_info} encountered —
the base config.
+ *
+ *
+ * If a base config exists and an override was captured, the result is
{@link
+ * PolarisStorageConfigurationInfo#withStorageName} applied to the base. If no
base config exists,
+ * the result is empty regardless of any override.
+ */
+public final class StorageConfigOverrideResolver {
+
+ private StorageConfigOverrideResolver() {}
+
+ /**
+ * Walk {@code entityPath} leaf-to-root and return the effective storage
configuration. The base
+ * config is sourced from the nearest entity that has {@code
storage_configuration_info} in its
+ * internal properties; if any closer ancestor (including the leaf itself)
carries {@code
+ * storage_name_override}, that name replaces the base config's {@code
storageName}.
+ *
+ * @param entityPath entities in root-to-leaf order; the catalog is at index 0
+ * @return effective config, or empty if no entity in the chain has a base
storage config
+ */
+ public static Optional
resolveEffectiveConfig(
+ @NonNull List entityPath) {
+String override = null;
+for (int i = entityPath.size() - 1; i >= 0; i--) {
+ Map internalProps =
entityPath.get(i).getInternalPropertiesAsMap();
+ if (override == null) {
+String candidate =
+
internalProps.get(PolarisEntityConstants.getStorageNameOverridePropertyName());
+if (candidate != null && !candidate.isEmpty()) {
+ override = candidate;
+}
+ }
+ String serializedBase =
+
internalProps.get(PolarisEntityConstants.getStorageConfigInfoPropertyName());
+ if (serializedBase != null) {
+PolarisStorageConfigurationInfo base =
+PolarisStorageConfigurationInfo.deserialize(serializedBase);
+if (override != null) {
+ return
Optional.of(PolarisStorageConfigurationInfo.withStorageName(base, override));
Review Comment:
I guess the whole idea for multiple storage configs/names is applicable only
to self-managed deployments. So the admin should be able to sort out name
overlaps there.
I'm not sure how this idea can fit in SaaS deployments, TBH :thinking:
--
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]
Re: [PR] feat(storage): inline polaris.storage.name overrides for namespace/table [polaris]
tokoko commented on code in PR #4023:
URL: https://github.com/apache/polaris/pull/4023#discussion_r3396897728
##
polaris-core/src/main/java/org/apache/polaris/core/storage/StorageConfigOverrideResolver.java:
##
@@ -0,0 +1,83 @@
+/*
+ * 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 java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import org.apache.polaris.core.entity.PolarisEntity;
+import org.apache.polaris.core.entity.PolarisEntityConstants;
+import org.jspecify.annotations.NonNull;
+
+/**
+ * Resolves the effective {@link PolarisStorageConfigurationInfo} for an
entity hierarchy by
+ * applying any nearest-ancestor {@code storage_name_override} on top of the
catalog's base storage
+ * configuration.
+ *
+ * The input list is in root-to-leaf order (the catalog is at index 0). The
walk runs
+ * leaf-to-root and:
+ *
+ *
+ * Captures the first {@code storage_name_override} encountered (nearest
wins).
+ * Returns the first {@code storage_configuration_info} encountered —
the base config.
+ *
+ *
+ * If a base config exists and an override was captured, the result is
{@link
+ * PolarisStorageConfigurationInfo#withStorageName} applied to the base. If no
base config exists,
+ * the result is empty regardless of any override.
+ */
+public final class StorageConfigOverrideResolver {
+
+ private StorageConfigOverrideResolver() {}
+
+ /**
+ * Walk {@code entityPath} leaf-to-root and return the effective storage
configuration. The base
+ * config is sourced from the nearest entity that has {@code
storage_configuration_info} in its
+ * internal properties; if any closer ancestor (including the leaf itself)
carries {@code
+ * storage_name_override}, that name replaces the base config's {@code
storageName}.
+ *
+ * @param entityPath entities in root-to-leaf order; the catalog is at index 0
+ * @return effective config, or empty if no entity in the chain has a base
storage config
+ */
+ public static Optional
resolveEffectiveConfig(
+ @NonNull List entityPath) {
+String override = null;
+for (int i = entityPath.size() - 1; i >= 0; i--) {
+ Map internalProps =
entityPath.get(i).getInternalPropertiesAsMap();
+ if (override == null) {
+String candidate =
+
internalProps.get(PolarisEntityConstants.getStorageNameOverridePropertyName());
+if (candidate != null && !candidate.isEmpty()) {
+ override = candidate;
+}
+ }
+ String serializedBase =
+
internalProps.get(PolarisEntityConstants.getStorageConfigInfoPropertyName());
+ if (serializedBase != null) {
+PolarisStorageConfigurationInfo base =
+PolarisStorageConfigurationInfo.deserialize(serializedBase);
+if (override != null) {
+ return
Optional.of(PolarisStorageConfigurationInfo.withStorageName(base, override));
Review Comment:
I remember that introducing a separate crud resource for storages was
rejected, but iiuc what you're talking about is allowing multiple
`PolarisStorageConfigurationInfo` to be defined on catalog level and then
tables/namespaces would be able to choose between them, is that right? I
suppose that's a reasonably clean approach.
One thing to note... `storagaName` field from #3409 is more of a pointer atm
rather than a unique name designation for a specific
`PolarisStorageConfigurationInfo`. I suppose it can be both, but it might be a
little awkward: for example what if two catalogs define a storage config with
same names, but want to resolve different credentials from the server config. I
guess it shouldn't be a problem as long as it's well-documented.
--
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]
Re: [PR] feat(storage): inline polaris.storage.name overrides for namespace/table [polaris]
dimas-b commented on code in PR #4023:
URL: https://github.com/apache/polaris/pull/4023#discussion_r3396595808
##
polaris-core/src/main/java/org/apache/polaris/core/storage/StorageConfigOverrideResolver.java:
##
@@ -0,0 +1,83 @@
+/*
+ * 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 java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import org.apache.polaris.core.entity.PolarisEntity;
+import org.apache.polaris.core.entity.PolarisEntityConstants;
+import org.jspecify.annotations.NonNull;
+
+/**
+ * Resolves the effective {@link PolarisStorageConfigurationInfo} for an
entity hierarchy by
+ * applying any nearest-ancestor {@code storage_name_override} on top of the
catalog's base storage
+ * configuration.
+ *
+ * The input list is in root-to-leaf order (the catalog is at index 0). The
walk runs
+ * leaf-to-root and:
+ *
+ *
+ * Captures the first {@code storage_name_override} encountered (nearest
wins).
+ * Returns the first {@code storage_configuration_info} encountered —
the base config.
+ *
+ *
+ * If a base config exists and an override was captured, the result is
{@link
+ * PolarisStorageConfigurationInfo#withStorageName} applied to the base. If no
base config exists,
+ * the result is empty regardless of any override.
+ */
+public final class StorageConfigOverrideResolver {
+
+ private StorageConfigOverrideResolver() {}
+
+ /**
+ * Walk {@code entityPath} leaf-to-root and return the effective storage
configuration. The base
+ * config is sourced from the nearest entity that has {@code
storage_configuration_info} in its
+ * internal properties; if any closer ancestor (including the leaf itself)
carries {@code
+ * storage_name_override}, that name replaces the base config's {@code
storageName}.
+ *
+ * @param entityPath entities in root-to-leaf order; the catalog is at index 0
+ * @return effective config, or empty if no entity in the chain has a base
storage config
+ */
+ public static Optional
resolveEffectiveConfig(
+ @NonNull List entityPath) {
+String override = null;
+for (int i = entityPath.size() - 1; i >= 0; i--) {
+ Map internalProps =
entityPath.get(i).getInternalPropertiesAsMap();
+ if (override == null) {
+String candidate =
+
internalProps.get(PolarisEntityConstants.getStorageNameOverridePropertyName());
+if (candidate != null && !candidate.isEmpty()) {
+ override = candidate;
+}
+ }
+ String serializedBase =
+
internalProps.get(PolarisEntityConstants.getStorageConfigInfoPropertyName());
+ if (serializedBase != null) {
+PolarisStorageConfigurationInfo base =
+PolarisStorageConfigurationInfo.deserialize(serializedBase);
+if (override != null) {
+ return
Optional.of(PolarisStorageConfigurationInfo.withStorageName(base, override));
Review Comment:
I'd expect different storage names to be associated with different
`PolarisStorageConfigurationInfo` objects (semantically). The storage name
would act as a user-level identifier for a specific
`PolarisStorageConfigurationInfo` piece of data.
Simply updating the name in a `PolarisStorageConfigurationInfo` in runtime
looks odd to me :thinking:
--
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]
Re: [PR] feat(storage): inline polaris.storage.name overrides for namespace/table [polaris]
tokoko commented on code in PR #4023:
URL: https://github.com/apache/polaris/pull/4023#discussion_r3396079073
##
polaris-core/src/main/java/org/apache/polaris/core/storage/StorageConfigOverrideResolver.java:
##
@@ -0,0 +1,83 @@
+/*
+ * 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 java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import org.apache.polaris.core.entity.PolarisEntity;
+import org.apache.polaris.core.entity.PolarisEntityConstants;
+import org.jspecify.annotations.NonNull;
+
+/**
+ * Resolves the effective {@link PolarisStorageConfigurationInfo} for an
entity hierarchy by
+ * applying any nearest-ancestor {@code storage_name_override} on top of the
catalog's base storage
+ * configuration.
+ *
+ * The input list is in root-to-leaf order (the catalog is at index 0). The
walk runs
+ * leaf-to-root and:
+ *
+ *
+ * Captures the first {@code storage_name_override} encountered (nearest
wins).
+ * Returns the first {@code storage_configuration_info} encountered —
the base config.
+ *
+ *
+ * If a base config exists and an override was captured, the result is
{@link
+ * PolarisStorageConfigurationInfo#withStorageName} applied to the base. If no
base config exists,
+ * the result is empty regardless of any override.
+ */
+public final class StorageConfigOverrideResolver {
+
+ private StorageConfigOverrideResolver() {}
+
+ /**
+ * Walk {@code entityPath} leaf-to-root and return the effective storage
configuration. The base
+ * config is sourced from the nearest entity that has {@code
storage_configuration_info} in its
+ * internal properties; if any closer ancestor (including the leaf itself)
carries {@code
+ * storage_name_override}, that name replaces the base config's {@code
storageName}.
+ *
+ * @param entityPath entities in root-to-leaf order; the catalog is at index 0
+ * @return effective config, or empty if no entity in the chain has a base
storage config
+ */
+ public static Optional
resolveEffectiveConfig(
+ @NonNull List entityPath) {
+String override = null;
+for (int i = entityPath.size() - 1; i >= 0; i--) {
+ Map internalProps =
entityPath.get(i).getInternalPropertiesAsMap();
+ if (override == null) {
+String candidate =
+
internalProps.get(PolarisEntityConstants.getStorageNameOverridePropertyName());
+if (candidate != null && !candidate.isEmpty()) {
+ override = candidate;
+}
+ }
+ String serializedBase =
+
internalProps.get(PolarisEntityConstants.getStorageConfigInfoPropertyName());
+ if (serializedBase != null) {
+PolarisStorageConfigurationInfo base =
+PolarisStorageConfigurationInfo.deserialize(serializedBase);
+if (override != null) {
+ return
Optional.of(PolarisStorageConfigurationInfo.withStorageName(base, override));
Review Comment:
I think you might be misreading this (or maybe I am). storage names are set
effectively from server configuration similar to how they are handled after
#3409. changing storage name of a config from table/namespace metadata simply
changes the pointer to the predefined storages that are part of server config.
P.S. I also agree the entire approach isn't exactly ideal, but my
understanding of the devlist consensus was also how it's implemented in the PR.
--
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]
Re: [PR] feat(storage): inline polaris.storage.name overrides for namespace/table [polaris]
adutra commented on code in PR #4023:
URL: https://github.com/apache/polaris/pull/4023#discussion_r3383462916
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/io/FileIOUtil.java:
##
@@ -18,35 +18,57 @@
*/
package org.apache.polaris.service.catalog.io;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
import java.util.Optional;
import org.apache.polaris.core.entity.PolarisEntity;
import org.apache.polaris.core.entity.PolarisEntityConstants;
import org.apache.polaris.core.persistence.PolarisResolvedPathWrapper;
+import org.apache.polaris.core.storage.PolarisStorageConfigurationInfo;
+import org.apache.polaris.core.storage.StorageConfigOverrideResolver;
public class FileIOUtil {
private FileIOUtil() {}
/**
- * Finds storage configuration information in the hierarchy of the resolved
storage entity.
+ * Finds storage configuration information in the hierarchy of the resolved
storage entity, with
+ * any nearest-ancestor {@code storage_name_override} applied to the
catalog's base config.
*
- * This method starts at the "leaf" level (e.g., table) and walks
"upwards" through namespaces
- * in the hierarchy to the "root." It searches for the first entity
containing storage config
- * properties, identified using a key from {@link
- * PolarisEntityConstants#getStorageConfigInfoPropertyName()}.
+ * This method walks the path leaf-to-root, locates the first entity
carrying {@code
+ * storage_configuration_info} (the catalog), and — if any closer ancestor
carries a {@code
+ * storage_name_override} — returns a synthetic copy of the base entity
whose serialized
+ * configuration has its {@code storageName} replaced. Callers that read the
resulting entity's
+ * internal properties (e.g., to stash on a purge task) therefore see the
effective config.
*
* @param resolvedStorageEntity the resolved entity wrapper containing the
hierarchical path
- * @return an {@link Optional} containing the entity with storage config, or
empty if not found
+ * @return an {@link Optional} containing the (possibly synthetic) entity
with the effective
+ * storage config, or empty if no entity in the chain has a base storage
config
*/
public static Optional findStorageInfoFromHierarchy(
PolarisResolvedPathWrapper resolvedStorageEntity) {
-Optional storageInfoEntity =
-resolvedStorageEntity.getRawFullPath().reversed().stream()
+List path = resolvedStorageEntity.getRawFullPath();
+Optional effective =
+StorageConfigOverrideResolver.resolveEffectiveConfig(path);
+if (effective.isEmpty()) {
+ return Optional.empty();
+}
+Optional baseEntity =
+path.reversed().stream()
.filter(
e ->
e.getInternalPropertiesAsMap()
.containsKey(PolarisEntityConstants.getStorageConfigInfoPropertyName()))
.findFirst();
-return storageInfoEntity;
+if (baseEntity.isEmpty()) {
Review Comment:
+1, there is some overlap between this class and
`StorageConfigOverrideResolver`.
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##
@@ -157,6 +158,13 @@ public class IcebergCatalog extends
BaseMetastoreViewCatalog
private static final Joiner SLASH = Joiner.on("/");
+ /**
+ * User-facing property key on namespaces and tables that selects a
server-configured named
+ * storage profile to use for credential vending. Validated and translated
into the internal
+ * property {@link
PolarisEntityConstants#getStorageNameOverridePropertyName()}.
+ */
+ static final String POLARIS_STORAGE_NAME_PROPERTY = "polaris.storage.name";
Review Comment:
Just thinking out loud: if I get this correctly, we would be tying a
namespace or table property to a storage profile that must be declared in the
server configuration. IOW, the namespace or table in question becomes unusable
if the catalog configuration changes and does not contain the profile anymore.
Probably OK, but this requires some coordination between two personas: the user
that created the table, and the admin that manages the server configuration.
##
polaris-core/src/main/java/org/apache/polaris/core/storage/StorageNameValidator.java:
##
@@ -0,0 +1,64 @@
+/*
+ * 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 u
Re: [PR] feat(storage): inline polaris.storage.name overrides for namespace/table [polaris]
dimas-b commented on code in PR #4023:
URL: https://github.com/apache/polaris/pull/4023#discussion_r3376448866
##
polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java:
##
@@ -657,6 +657,18 @@ public static void enforceFeatureEnabledOrThrow(
.defaultValue(false)
.buildFeatureConfiguration();
+ public static final FeatureConfiguration
ALLOW_STORAGE_NAME_OVERRIDE =
+ PolarisConfiguration.builder()
+ .key("ALLOW_STORAGE_NAME_OVERRIDE")
+ .description(
+ "If set to true, allows namespaces and tables to set the
polaris.storage.name "
Review Comment:
What is the value of this flag? If a user points to a non-existent storage
name, the adverse effect is localized to that user's change.
Adding storage names is a Catalog Management action and should probably be
covered by AuthZ checks.
I'd think setting/changing `polaris.storage.name` probably needs an new
AuthZ operation rather than a global feature flag.
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]
Re: [PR] feat(storage): inline polaris.storage.name overrides for namespace/table [polaris]
dimas-b commented on code in PR #4023:
URL: https://github.com/apache/polaris/pull/4023#discussion_r3376500905
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##
@@ -1937,6 +2002,88 @@ private String newTableMetadataFilePath(TableMetadata
meta, int newVersion) {
}
}
+ /**
+ * Process the user-facing {@link #POLARIS_STORAGE_NAME_PROPERTY} on a write
request: validate the
+ * value, gate against {@link
FeatureConfiguration#ALLOW_STORAGE_NAME_OVERRIDE} when the value
+ * would change, write the resolved value into the internal-property map (or
remove it when
+ * cleared), and strip the user-facing key from the user property map.
+ *
+ * The flag is only enforced when the requested value would actually
mutate the persisted
+ * {@code storage_name_override}. An idempotent re-set with the prior value
is always allowed, so
+ * flipping the flag off does not break existing entities.
+ *
+ * Property semantics:
+ *
+ *
+ * {@code polaris.storage.name=foo} (and {@code foo} is non-blank,
valid) sets the override.
+ * {@code polaris.storage.name=""} (or whitespace) clears the override.
+ * Property absent from the user map: no change to the override.
+ *
+ *
+ * @param userProperties user-facing property map being persisted; the {@link
+ * #POLARIS_STORAGE_NAME_PROPERTY} key is removed in place if present.
+ * @param internalProperties internal-property map that will be persisted
onto the entity; the
+ * {@code storage_name_override} key is set or removed in place based on
the request.
+ * @param priorOverride the override value already persisted on the entity
(or {@code null} for a
+ * fresh create). Used to decide whether the request is an idempotent
re-set.
+ */
+ @VisibleForTesting
+ void processStorageNameOverrideOnWrite(
+ Map userProperties,
+ Map internalProperties,
+ @Nullable String priorOverride) {
Review Comment:
Is `priorOverride` actually an "override"? What does it override? 🤔
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##
@@ -1937,6 +2002,88 @@ private String newTableMetadataFilePath(TableMetadata
meta, int newVersion) {
}
}
+ /**
+ * Process the user-facing {@link #POLARIS_STORAGE_NAME_PROPERTY} on a write
request: validate the
+ * value, gate against {@link
FeatureConfiguration#ALLOW_STORAGE_NAME_OVERRIDE} when the value
+ * would change, write the resolved value into the internal-property map (or
remove it when
+ * cleared), and strip the user-facing key from the user property map.
+ *
+ * The flag is only enforced when the requested value would actually
mutate the persisted
+ * {@code storage_name_override}. An idempotent re-set with the prior value
is always allowed, so
+ * flipping the flag off does not break existing entities.
+ *
+ * Property semantics:
+ *
+ *
+ * {@code polaris.storage.name=foo} (and {@code foo} is non-blank,
valid) sets the override.
+ * {@code polaris.storage.name=""} (or whitespace) clears the override.
+ * Property absent from the user map: no change to the override.
+ *
+ *
+ * @param userProperties user-facing property map being persisted; the {@link
+ * #POLARIS_STORAGE_NAME_PROPERTY} key is removed in place if present.
+ * @param internalProperties internal-property map that will be persisted
onto the entity; the
+ * {@code storage_name_override} key is set or removed in place based on
the request.
+ * @param priorOverride the override value already persisted on the entity
(or {@code null} for a
+ * fresh create). Used to decide whether the request is an idempotent
re-set.
+ */
+ @VisibleForTesting
+ void processStorageNameOverrideOnWrite(
+ Map userProperties,
+ Map internalProperties,
+ @Nullable String priorOverride) {
+if (!userProperties.containsKey(POLARIS_STORAGE_NAME_PROPERTY)) {
+ return;
+}
+String rawValue = userProperties.remove(POLARIS_STORAGE_NAME_PROPERTY);
Review Comment:
Modifying parameters is not intuitive... Can we avoid this?
##
polaris-core/src/main/java/org/apache/polaris/core/storage/StorageConfigOverrideResolver.java:
##
@@ -0,0 +1,83 @@
+/*
+ * 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
Re: [PR] feat(storage): inline polaris.storage.name overrides for namespace/table [polaris]
tokoko commented on code in PR #4023:
URL: https://github.com/apache/polaris/pull/4023#discussion_r3364827472
##
runtime/service/src/main/java/org/apache/polaris/service/storage/PolarisStorageIntegrationProviderImpl.java:
##
@@ -129,8 +129,7 @@ public PolarisStorageIntegrationProviderImpl(
@Override
public @Nullable PolarisStorageIntegration getStorageIntegration(
@NonNull List resolvedEntityPath) {
-return
PolarisStorageConfigurationInfo.findStorageInfoFromHierarchy(resolvedEntityPath)
Review Comment:
can we get rid of
`PolarisStorageConfigurationInfo.findStorageInfoFromHierarchy`?
`StorageConfigOverrideResolver.resolveEffectiveConfig` seems like a more
correct replacement for it now.
--
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]
Re: [PR] feat(storage): inline polaris.storage.name overrides for namespace/table [polaris]
github-actions[bot] commented on PR #4023: URL: https://github.com/apache/polaris/pull/4023#issuecomment-4393694003 This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days. -- 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]
Re: [PR] feat(storage): inline polaris.storage.name overrides for namespace/table [polaris]
sririshindra commented on code in PR #4023:
URL: https://github.com/apache/polaris/pull/4023#discussion_r3042166529
##
runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/StorageNameOverrideTest.java:
##
@@ -0,0 +1,350 @@
+/*
+ * 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.PolarisEventDispatcher;
+import org.apache.polaris.service.events.PolarisEventMetadataFactory;
+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 {
Review Comment:
Added a test for flag disabled plus priorValue equals requestedValue, and it
asserts no exception is thrown. This protects existing tables when the feature
flag is later turned off.
--
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]
Re: [PR] feat(storage): inline polaris.storage.name overrides for namespace/table [polaris]
sririshindra commented on code in PR #4023:
URL: https://github.com/apache/polaris/pull/4023#discussion_r3042159500
##
polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManager.java:
##
@@ -538,4 +542,76 @@ default Optional
findPrincipalRoleByName(
}
return Optional.of(entityResult.getEntity()).map(PrincipalRoleEntity::of);
}
+
+ /**
+ * Resolves the effective storage configuration for an entity by
reconstructing the entity
+ * hierarchy via {@code parentId} links and delegating to {@link
StorageConfigResolver#resolve},
+ * which is the single source of truth for storage-name override resolution.
+ *
+ * This is the credential-vending counterpart of {@code
+ * FileIOUtil.resolveEffectiveStorageConfig}; both paths use {@link
StorageConfigResolver} to
+ * guarantee identical semantics.
+ *
+ * @param callCtx call context
+ * @param entity the entity whose storage config should be resolved
+ * @return the entity to use (either unchanged, or a synthetic copy with
resolved
+ * storageConfigInfo)
+ */
+ default PolarisBaseEntity resolveEntityStorageConfig(
+ @Nonnull PolarisCallContext callCtx, @Nonnull PolarisBaseEntity entity) {
+Map props = entity.getInternalPropertiesAsMap();
+
+// Fast path: entity already carries the full storageConfigInfo (e.g.
catalog entities).
+if
(props.containsKey(PolarisEntityConstants.getStorageConfigInfoPropertyName())) {
+ return entity;
+}
+
+// Build the entity chain (leaf-to-root) by walking parentId links.
+long catalogId = entity.getCatalogId();
+List chain = new ArrayList<>();
+chain.add(entity);
+
+long currentParentId = entity.getParentId();
+while (currentParentId != PolarisEntityConstants.getNullId() &&
currentParentId != catalogId) {
+ EntityResult parentResult =
+ loadEntity(callCtx, catalogId, currentParentId,
PolarisEntityType.NAMESPACE);
+ if (parentResult.getReturnStatus() != BaseResult.ReturnStatus.SUCCESS) {
+break;
+ }
+ chain.add(parentResult.getEntity());
+ currentParentId = parentResult.getEntity().getParentId();
+}
+
+// Append the catalog entity (root of the chain).
+EntityResult catalogResult =
+loadEntity(
+callCtx, PolarisEntityConstants.getNullId(), catalogId,
PolarisEntityType.CATALOG);
+if (catalogResult.getReturnStatus() != BaseResult.ReturnStatus.SUCCESS) {
+ return entity;
+}
+chain.add(catalogResult.getEntity());
+
+// Delegate to the shared resolver.
+Optional resolved =
StorageConfigResolver.resolve(chain);
+if (resolved.isEmpty()) {
+ return entity;
+}
+
+// Only create a synthetic entity if the resolved config differs from the
catalog's base
+// (i.e., an override was actually applied). If no override exists, the
resolved config is
+// the catalog's own config and should not be stamped onto a non-catalog
entity.
+String catalogConfigJson =
+catalogResult
+.getEntity()
+.getInternalPropertiesAsMap()
+.get(PolarisEntityConstants.getStorageConfigInfoPropertyName());
+String resolvedJson = resolved.get().serialize();
+if (resolvedJson.equals(catalogConfigJson)) {
Review Comment:
Great catch. I replaced string equality on serialized JSON with structural
comparison using ObjectMapper and JsonNode equality. This makes the comparison
robust to field ordering differences.
--
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]
Re: [PR] feat(storage): inline polaris.storage.name overrides for namespace/table [polaris]
sririshindra commented on code in PR #4023:
URL: https://github.com/apache/polaris/pull/4023#discussion_r3042162496
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/io/FileIOUtil.java:
##
@@ -18,35 +18,98 @@
*/
package org.apache.polaris.service.catalog.io;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
import java.util.Optional;
import org.apache.polaris.core.entity.PolarisEntity;
import org.apache.polaris.core.entity.PolarisEntityConstants;
import org.apache.polaris.core.persistence.PolarisResolvedPathWrapper;
+import org.apache.polaris.core.storage.PolarisStorageConfigurationInfo;
+import org.apache.polaris.core.storage.StorageConfigResolver;
public class FileIOUtil {
private FileIOUtil() {}
/**
- * Finds storage configuration information in the hierarchy of the resolved
storage entity.
+ * Finds the first entity in a hierarchy (leaf to root) that has storage
configuration info in its
+ * internal properties ({@code storageConfigInfo}).
*
- * This method starts at the "leaf" level (e.g., table) and walks
"upwards" through namespaces
- * in the hierarchy to the "root." It searches for the first entity
containing storage config
- * properties, identified using a key from {@link
- * PolarisEntityConstants#getStorageConfigInfoPropertyName()}.
+ * This returns the base config entity and does not apply any
{@code
+ * storageNameOverride} that may be present on descendant entities. Use
{@link
+ * #resolveEffectiveStorageConfig(List)} to get the fully-resolved
configuration.
*
- * @param resolvedStorageEntity the resolved entity wrapper containing the
hierarchical path
+ * @param entityPath a list of entities ordered root-to-leaf (catalog first,
leaf last)
* @return an {@link Optional} containing the entity with storage config, or
empty if not found
*/
+ public static Optional findEntityWithStorageConfigInHierarchy(
+ List entityPath) {
+return entityPath.reversed().stream()
+.filter(
+e ->
+e.getInternalPropertiesAsMap()
+
.containsKey(PolarisEntityConstants.getStorageConfigInfoPropertyName()))
+.findFirst();
+ }
+
+ /**
+ * Resolves the effective storage configuration for an entity hierarchy.
+ *
+ * Delegates to {@link StorageConfigResolver#resolve}, which is the
single source of truth for
+ * storage-name override resolution. See that class for the resolution
algorithm.
+ *
+ * @param entityPath a list of entities ordered root-to-leaf (catalog first,
leaf last)
+ * @return the effective {@link PolarisStorageConfigurationInfo}, or empty
if no base config is
+ * found in the hierarchy
+ */
+ public static Optional
resolveEffectiveStorageConfig(
+ List entityPath) {
+return StorageConfigResolver.resolve(entityPath.reversed());
+ }
+
+ /**
+ * Finds storage configuration information in the hierarchy of the resolved
storage entity,
+ * applying any {@code storageNameOverride} found on descendant entities.
+ *
+ * Returns a {@link PolarisEntity} whose {@code storageConfigInfo}
internal property reflects
+ * the effective (potentially name-overridden) storage configuration.
+ *
+ * @param resolvedStorageEntity the resolved entity wrapper containing the
hierarchical path
+ * @return an {@link Optional} containing an entity with the effective
storage config, or empty if
+ * not found
+ */
public static Optional findStorageInfoFromHierarchy(
PolarisResolvedPathWrapper resolvedStorageEntity) {
-Optional storageInfoEntity =
-resolvedStorageEntity.getRawFullPath().reversed().stream()
-.filter(
-e ->
-e.getInternalPropertiesAsMap()
-
.containsKey(PolarisEntityConstants.getStorageConfigInfoPropertyName()))
-.findFirst();
-return storageInfoEntity;
+List entityPath = resolvedStorageEntity.getRawFullPath();
+Optional effectiveConfig =
+resolveEffectiveStorageConfig(entityPath);
+if (effectiveConfig.isEmpty()) {
+ return Optional.empty();
+}
+
+// Return the base entity (catalog) with its storageConfigInfo replaced by
the effective config.
+// This preserves all other internal properties (e.g.
storageIntegrationIdentifier) while
+// ensuring callers read the correctly name-overridden config.
+PolarisEntity baseEntity =
findEntityWithStorageConfigInHierarchy(entityPath).orElseThrow();
+Map updatedInternalProps =
+new HashMap<>(baseEntity.getInternalPropertiesAsMap());
+updatedInternalProps.put(
+PolarisEntityConstants.getStorageConfigInfoPropertyName(),
+effectiveConfig.get().serialize());
+return Optional.of(
+new
PolarisEntity.Builder(baseEntity).setInternalProperties(updatedInternalProps).build());
+ }
+
+ /**
+ * Deserializes and returns the e
Re: [PR] feat(storage): inline polaris.storage.name overrides for namespace/table [polaris]
sririshindra commented on code in PR #4023:
URL: https://github.com/apache/polaris/pull/4023#discussion_r3042158733
##
polaris-core/src/test/java/org/apache/polaris/core/storage/StorageConfigResolverTest.java:
##
@@ -0,0 +1,181 @@
+/*
+ * 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 java.util.Map;
+import java.util.Optional;
+import org.apache.polaris.core.entity.PolarisBaseEntity;
+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.storage.aws.AwsStorageConfigurationInfo;
+import org.junit.jupiter.api.Test;
+
+class StorageConfigResolverTest {
+
+ @Test
+ void emptyChain_returnsEmpty() {
+assertThat(StorageConfigResolver.resolve(List.of())).isEmpty();
+ }
+
+ @Test
+ void catalogOnly_returnsBaseConfig() {
+PolarisBaseEntity catalog = catalogWithConfig("catalog-storage");
+
+Optional result =
+StorageConfigResolver.resolve(List.of(catalog));
+
+assertThat(result).isPresent();
+assertThat(result.get().getStorageName()).isEqualTo("catalog-storage");
+ }
+
+ @Test
+ void entityWithOverride_appliesOverrideToCatalogConfig() {
+PolarisBaseEntity table = entityWithOverride("table-storage", 3L, 2L);
+PolarisBaseEntity namespace = entityWithoutConfig(2L, 1L);
+PolarisBaseEntity catalog = catalogWithConfig("catalog-storage");
+
+// leaf-to-root: table -> namespace -> catalog
+Optional result =
+StorageConfigResolver.resolve(List.of(table, namespace, catalog));
+
+assertThat(result).isPresent();
+assertThat(result.get().getStorageName()).isEqualTo("table-storage");
+ }
+
+ @Test
+ void namespaceOverride_inheritedByTable() {
+PolarisBaseEntity table = entityWithoutConfig(3L, 2L);
+PolarisBaseEntity namespace = entityWithOverride("ns-storage", 2L, 1L);
+PolarisBaseEntity catalog = catalogWithConfig("catalog-storage");
+
+// Table has no override; namespace does — table should inherit
+Optional result =
+StorageConfigResolver.resolve(List.of(table, namespace, catalog));
+
+assertThat(result).isPresent();
+assertThat(result.get().getStorageName()).isEqualTo("ns-storage");
+ }
+
+ @Test
+ void tableOverride_winsOverNamespace() {
+PolarisBaseEntity table = entityWithOverride("table-storage", 3L, 2L);
+PolarisBaseEntity namespace = entityWithOverride("ns-storage", 2L, 1L);
+PolarisBaseEntity catalog = catalogWithConfig("catalog-storage");
+
+// Both table and namespace have overrides — table (leaf) should win
+Optional result =
+StorageConfigResolver.resolve(List.of(table, namespace, catalog));
+
+assertThat(result).isPresent();
+assertThat(result.get().getStorageName()).isEqualTo("table-storage");
+ }
+
+ @Test
+ void noOverride_returnsCatalogBaseConfig() {
+PolarisBaseEntity table = entityWithoutConfig(3L, 2L);
+PolarisBaseEntity namespace = entityWithoutConfig(2L, 1L);
+PolarisBaseEntity catalog = catalogWithConfig("catalog-storage");
+
+Optional result =
+StorageConfigResolver.resolve(List.of(table, namespace, catalog));
+
+assertThat(result).isPresent();
+assertThat(result.get().getStorageName()).isEqualTo("catalog-storage");
+ }
+
+ @Test
+ void noBaseConfig_returnsEmpty() {
+PolarisBaseEntity table = entityWithOverride("table-storage", 3L, 2L);
+PolarisBaseEntity namespace = entityWithoutConfig(2L, 1L);
+
+// No catalog with storageConfigInfo in the chain
+Optional result =
+StorageConfigResolver.resolve(List.of(table, namespace));
+
+assertThat(result).isEmpty();
+ }
+
+ @Test
+ void overridePreservesBaseConfigFields() {
+PolarisBaseEntity table = entityWithOverride("overridden-storage", 3L, 2L);
+String roleArn = "arn:aws:iam::123456789012:role/test-role";
Review Comment:
Done. I extracted repeated storage-name and ARN literals into top-level
constants.
--
This is an automated message from the Apache Git Service.
Re: [PR] feat(storage): inline polaris.storage.name overrides for namespace/table [polaris]
sririshindra commented on code in PR #4023:
URL: https://github.com/apache/polaris/pull/4023#discussion_r3042156883
##
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.
+ *
+ * Storage names are symbolic references to server-side credential
configurations (e.g. {@code
+ * polaris.storage.aws..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:
I combined normalization and validation into a single helper,
normalizeAndValidate, and updated all call sites to use it.
--
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]
Re: [PR] feat(storage): inline polaris.storage.name overrides for namespace/table [polaris]
sririshindra commented on code in PR #4023:
URL: https://github.com/apache/polaris/pull/4023#discussion_r3042154193
##
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
Re: [PR] feat(storage): inline polaris.storage.name overrides for namespace/table [polaris]
adnanhemani commented on code in PR #4023:
URL: https://github.com/apache/polaris/pull/4023#discussion_r3034643642
##
polaris-core/src/test/java/org/apache/polaris/core/storage/StorageConfigResolverTest.java:
##
@@ -0,0 +1,181 @@
+/*
+ * 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 java.util.Map;
+import java.util.Optional;
+import org.apache.polaris.core.entity.PolarisBaseEntity;
+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.storage.aws.AwsStorageConfigurationInfo;
+import org.junit.jupiter.api.Test;
+
+class StorageConfigResolverTest {
+
+ @Test
+ void emptyChain_returnsEmpty() {
+assertThat(StorageConfigResolver.resolve(List.of())).isEmpty();
+ }
+
+ @Test
+ void catalogOnly_returnsBaseConfig() {
+PolarisBaseEntity catalog = catalogWithConfig("catalog-storage");
+
+Optional result =
+StorageConfigResolver.resolve(List.of(catalog));
+
+assertThat(result).isPresent();
+assertThat(result.get().getStorageName()).isEqualTo("catalog-storage");
+ }
+
+ @Test
+ void entityWithOverride_appliesOverrideToCatalogConfig() {
+PolarisBaseEntity table = entityWithOverride("table-storage", 3L, 2L);
+PolarisBaseEntity namespace = entityWithoutConfig(2L, 1L);
+PolarisBaseEntity catalog = catalogWithConfig("catalog-storage");
+
+// leaf-to-root: table -> namespace -> catalog
+Optional result =
+StorageConfigResolver.resolve(List.of(table, namespace, catalog));
+
+assertThat(result).isPresent();
+assertThat(result.get().getStorageName()).isEqualTo("table-storage");
+ }
+
+ @Test
+ void namespaceOverride_inheritedByTable() {
+PolarisBaseEntity table = entityWithoutConfig(3L, 2L);
+PolarisBaseEntity namespace = entityWithOverride("ns-storage", 2L, 1L);
+PolarisBaseEntity catalog = catalogWithConfig("catalog-storage");
+
+// Table has no override; namespace does — table should inherit
+Optional result =
+StorageConfigResolver.resolve(List.of(table, namespace, catalog));
+
+assertThat(result).isPresent();
+assertThat(result.get().getStorageName()).isEqualTo("ns-storage");
+ }
+
+ @Test
+ void tableOverride_winsOverNamespace() {
+PolarisBaseEntity table = entityWithOverride("table-storage", 3L, 2L);
+PolarisBaseEntity namespace = entityWithOverride("ns-storage", 2L, 1L);
+PolarisBaseEntity catalog = catalogWithConfig("catalog-storage");
+
+// Both table and namespace have overrides — table (leaf) should win
+Optional result =
+StorageConfigResolver.resolve(List.of(table, namespace, catalog));
+
+assertThat(result).isPresent();
+assertThat(result.get().getStorageName()).isEqualTo("table-storage");
+ }
+
+ @Test
+ void noOverride_returnsCatalogBaseConfig() {
+PolarisBaseEntity table = entityWithoutConfig(3L, 2L);
+PolarisBaseEntity namespace = entityWithoutConfig(2L, 1L);
+PolarisBaseEntity catalog = catalogWithConfig("catalog-storage");
+
+Optional result =
+StorageConfigResolver.resolve(List.of(table, namespace, catalog));
+
+assertThat(result).isPresent();
+assertThat(result.get().getStorageName()).isEqualTo("catalog-storage");
+ }
+
+ @Test
+ void noBaseConfig_returnsEmpty() {
+PolarisBaseEntity table = entityWithOverride("table-storage", 3L, 2L);
+PolarisBaseEntity namespace = entityWithoutConfig(2L, 1L);
+
+// No catalog with storageConfigInfo in the chain
+Optional result =
+StorageConfigResolver.resolve(List.of(table, namespace));
+
+assertThat(result).isEmpty();
+ }
+
+ @Test
+ void overridePreservesBaseConfigFields() {
+PolarisBaseEntity table = entityWithOverride("overridden-storage", 3L, 2L);
+String roleArn = "arn:aws:iam::123456789012:role/test-role";
Review Comment:
Move magic variable to the top. I'd also suggest moving all the
`ns-storage`, `catalog-storage`, etc. to the top as well.
##
polaris-core
Re: [PR] feat(storage): inline polaris.storage.name overrides for namespace/table [polaris]
sririshindra commented on code in PR #4023:
URL: https://github.com/apache/polaris/pull/4023#discussion_r3016842818
##
polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManager.java:
##
@@ -538,4 +542,76 @@ default Optional
findPrincipalRoleByName(
}
return Optional.of(entityResult.getEntity()).map(PrincipalRoleEntity::of);
}
+
+ /**
+ * Resolves the effective storage configuration for an entity by
reconstructing the entity
+ * hierarchy via {@code parentId} links and delegating to {@link
StorageConfigResolver#resolve},
+ * which is the single source of truth for storage-name override resolution.
+ *
+ * This is the credential-vending counterpart of {@code
+ * FileIOUtil.resolveEffectiveStorageConfig}; both paths use {@link
StorageConfigResolver} to
+ * guarantee identical semantics.
+ *
+ * @param callCtx call context
+ * @param entity the entity whose storage config should be resolved
+ * @return the entity to use (either unchanged, or a synthetic copy with
resolved
+ * storageConfigInfo)
+ */
+ default PolarisBaseEntity resolveEntityStorageConfig(
+ @Nonnull PolarisCallContext callCtx, @Nonnull PolarisBaseEntity entity) {
+Map props = entity.getInternalPropertiesAsMap();
+
+// Fast path: entity already carries the full storageConfigInfo (e.g.
catalog entities).
+if
(props.containsKey(PolarisEntityConstants.getStorageConfigInfoPropertyName())) {
+ return entity;
+}
+
+// Build the entity chain (leaf-to-root) by walking parentId links.
+long catalogId = entity.getCatalogId();
+List chain = new ArrayList<>();
+chain.add(entity);
+
+long currentParentId = entity.getParentId();
+while (currentParentId != PolarisEntityConstants.getNullId() &&
currentParentId != catalogId) {
+ EntityResult parentResult =
+ loadEntity(callCtx, catalogId, currentParentId,
PolarisEntityType.NAMESPACE);
Review Comment:
Sounds good @dimas-b . I will wait for
https://github.com/apache/polaris/pull/3699 to be merged and rebase this PR. I
will review it as well.
--
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]
Re: [PR] feat(storage): inline polaris.storage.name overrides for namespace/table [polaris]
dimas-b commented on code in PR #4023:
URL: https://github.com/apache/polaris/pull/4023#discussion_r3012543880
##
polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManager.java:
##
@@ -538,4 +542,76 @@ default Optional
findPrincipalRoleByName(
}
return Optional.of(entityResult.getEntity()).map(PrincipalRoleEntity::of);
}
+
+ /**
+ * Resolves the effective storage configuration for an entity by
reconstructing the entity
+ * hierarchy via {@code parentId} links and delegating to {@link
StorageConfigResolver#resolve},
+ * which is the single source of truth for storage-name override resolution.
+ *
+ * This is the credential-vending counterpart of {@code
+ * FileIOUtil.resolveEffectiveStorageConfig}; both paths use {@link
StorageConfigResolver} to
+ * guarantee identical semantics.
+ *
+ * @param callCtx call context
+ * @param entity the entity whose storage config should be resolved
+ * @return the entity to use (either unchanged, or a synthetic copy with
resolved
+ * storageConfigInfo)
+ */
+ default PolarisBaseEntity resolveEntityStorageConfig(
+ @Nonnull PolarisCallContext callCtx, @Nonnull PolarisBaseEntity entity) {
+Map props = entity.getInternalPropertiesAsMap();
+
+// Fast path: entity already carries the full storageConfigInfo (e.g.
catalog entities).
+if
(props.containsKey(PolarisEntityConstants.getStorageConfigInfoPropertyName())) {
+ return entity;
+}
+
+// Build the entity chain (leaf-to-root) by walking parentId links.
+long catalogId = entity.getCatalogId();
+List chain = new ArrayList<>();
+chain.add(entity);
+
+long currentParentId = entity.getParentId();
+while (currentParentId != PolarisEntityConstants.getNullId() &&
currentParentId != catalogId) {
+ EntityResult parentResult =
+ loadEntity(callCtx, catalogId, currentParentId,
PolarisEntityType.NAMESPACE);
Review Comment:
From my POV, I'd prefer to merge #3699 first, then rebase this PR.
@sririshindra : 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]
Re: [PR] feat(storage): inline polaris.storage.name overrides for namespace/table [polaris]
dimas-b commented on code in PR #4023:
URL: https://github.com/apache/polaris/pull/4023#discussion_r3012543880
##
polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManager.java:
##
@@ -538,4 +542,76 @@ default Optional
findPrincipalRoleByName(
}
return Optional.of(entityResult.getEntity()).map(PrincipalRoleEntity::of);
}
+
+ /**
+ * Resolves the effective storage configuration for an entity by
reconstructing the entity
+ * hierarchy via {@code parentId} links and delegating to {@link
StorageConfigResolver#resolve},
+ * which is the single source of truth for storage-name override resolution.
+ *
+ * This is the credential-vending counterpart of {@code
+ * FileIOUtil.resolveEffectiveStorageConfig}; both paths use {@link
StorageConfigResolver} to
+ * guarantee identical semantics.
+ *
+ * @param callCtx call context
+ * @param entity the entity whose storage config should be resolved
+ * @return the entity to use (either unchanged, or a synthetic copy with
resolved
+ * storageConfigInfo)
+ */
+ default PolarisBaseEntity resolveEntityStorageConfig(
+ @Nonnull PolarisCallContext callCtx, @Nonnull PolarisBaseEntity entity) {
+Map props = entity.getInternalPropertiesAsMap();
+
+// Fast path: entity already carries the full storageConfigInfo (e.g.
catalog entities).
+if
(props.containsKey(PolarisEntityConstants.getStorageConfigInfoPropertyName())) {
+ return entity;
+}
+
+// Build the entity chain (leaf-to-root) by walking parentId links.
+long catalogId = entity.getCatalogId();
+List chain = new ArrayList<>();
+chain.add(entity);
+
+long currentParentId = entity.getParentId();
+while (currentParentId != PolarisEntityConstants.getNullId() &&
currentParentId != catalogId) {
+ EntityResult parentResult =
+ loadEntity(callCtx, catalogId, currentParentId,
PolarisEntityType.NAMESPACE);
Review Comment:
From my POV, I'd prefer to merge #3699 first, then rebase this PR. 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]
Re: [PR] feat(storage): inline polaris.storage.name overrides for namespace/table [polaris]
tokoko commented on code in PR #4023:
URL: https://github.com/apache/polaris/pull/4023#discussion_r3012471227
##
polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManager.java:
##
@@ -538,4 +542,76 @@ default Optional
findPrincipalRoleByName(
}
return Optional.of(entityResult.getEntity()).map(PrincipalRoleEntity::of);
}
+
+ /**
+ * Resolves the effective storage configuration for an entity by
reconstructing the entity
+ * hierarchy via {@code parentId} links and delegating to {@link
StorageConfigResolver#resolve},
+ * which is the single source of truth for storage-name override resolution.
+ *
+ * This is the credential-vending counterpart of {@code
+ * FileIOUtil.resolveEffectiveStorageConfig}; both paths use {@link
StorageConfigResolver} to
+ * guarantee identical semantics.
+ *
+ * @param callCtx call context
+ * @param entity the entity whose storage config should be resolved
+ * @return the entity to use (either unchanged, or a synthetic copy with
resolved
+ * storageConfigInfo)
+ */
+ default PolarisBaseEntity resolveEntityStorageConfig(
+ @Nonnull PolarisCallContext callCtx, @Nonnull PolarisBaseEntity entity) {
+Map props = entity.getInternalPropertiesAsMap();
+
+// Fast path: entity already carries the full storageConfigInfo (e.g.
catalog entities).
+if
(props.containsKey(PolarisEntityConstants.getStorageConfigInfoPropertyName())) {
+ return entity;
+}
+
+// Build the entity chain (leaf-to-root) by walking parentId links.
+long catalogId = entity.getCatalogId();
+List chain = new ArrayList<>();
+chain.add(entity);
+
+long currentParentId = entity.getParentId();
+while (currentParentId != PolarisEntityConstants.getNullId() &&
currentParentId != catalogId) {
+ EntityResult parentResult =
+ loadEntity(callCtx, catalogId, currentParentId,
PolarisEntityType.NAMESPACE);
Review Comment:
I think this PR would no longer have to solve this dual resolution issue
after #3699 since everything happens in `StorageAccessConfigProvider` there.
honestly this feels like we're putting bandaids on bandaids :smile: ideally we
should have a single place (StorageAccessConfigProvider most likely) that would
resolve storageInfo, resolve storage name, get additional properties from
config per storage name and return effective storage configuration. I don't
think that's possible to do now w/o first getting rid of all these unnecessary
layers.
--
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]
Re: [PR] feat(storage): inline polaris.storage.name overrides for namespace/table [polaris]
dimas-b commented on code in PR #4023:
URL: https://github.com/apache/polaris/pull/4023#discussion_r3012297416
##
polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManager.java:
##
@@ -538,4 +542,76 @@ default Optional
findPrincipalRoleByName(
}
return Optional.of(entityResult.getEntity()).map(PrincipalRoleEntity::of);
}
+
+ /**
+ * Resolves the effective storage configuration for an entity by
reconstructing the entity
+ * hierarchy via {@code parentId} links and delegating to {@link
StorageConfigResolver#resolve},
+ * which is the single source of truth for storage-name override resolution.
+ *
+ * This is the credential-vending counterpart of {@code
+ * FileIOUtil.resolveEffectiveStorageConfig}; both paths use {@link
StorageConfigResolver} to
+ * guarantee identical semantics.
+ *
+ * @param callCtx call context
+ * @param entity the entity whose storage config should be resolved
+ * @return the entity to use (either unchanged, or a synthetic copy with
resolved
+ * storageConfigInfo)
+ */
+ default PolarisBaseEntity resolveEntityStorageConfig(
+ @Nonnull PolarisCallContext callCtx, @Nonnull PolarisBaseEntity entity) {
+Map props = entity.getInternalPropertiesAsMap();
+
+// Fast path: entity already carries the full storageConfigInfo (e.g.
catalog entities).
+if
(props.containsKey(PolarisEntityConstants.getStorageConfigInfoPropertyName())) {
+ return entity;
+}
+
+// Build the entity chain (leaf-to-root) by walking parentId links.
+long catalogId = entity.getCatalogId();
+List chain = new ArrayList<>();
+chain.add(entity);
+
+long currentParentId = entity.getParentId();
+while (currentParentId != PolarisEntityConstants.getNullId() &&
currentParentId != catalogId) {
+ EntityResult parentResult =
+ loadEntity(callCtx, catalogId, currentParentId,
PolarisEntityType.NAMESPACE);
Review Comment:
I'm not sure about this loop, TBH... So far the usual workflow for requests
was to use the `Resolver` to find all relevant entities based on API parameters
upfront, then proceed with request execution.
So, normally, all the parents up to the catalog should already be loaded by
the time `StorageAccessConfig` is required. Could we reuse existing data from
the resolution manifest.
Note: this may overlap with #3699 CC: @tokoko
--
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]
Re: [PR] feat(storage): inline polaris.storage.name overrides for namespace/table [polaris]
dimas-b commented on code in PR #4023:
URL: https://github.com/apache/polaris/pull/4023#discussion_r2982695247
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##
@@ -520,15 +534,35 @@ private void createNamespaceInternal(
baseLocation += "/";
}
-NamespaceEntity entity =
+enforceStorageNameOverrideEnabledIfRequested(metadata);
+
+PolarisStorageConfigurationInfo namespaceStorageConfig =
+storageConfigFromPropertyOverride(metadata,
resolvedParent.getRawFullPath());
+
+NamespaceEntity.Builder nsBuilder =
new NamespaceEntity.Builder(namespace)
.setCatalogId(getCatalogId())
.setId(getMetaStoreManager().generateNewEntityId(getCurrentPolarisContext()).getId())
.setParentId(resolvedParent.getRawLeafEntity().getId())
.setProperties(metadata)
.setCreateTimestamp(System.currentTimeMillis())
-.setBaseLocation(baseLocation)
-.build();
+.setBaseLocation(baseLocation);
+NamespaceEntity entity;
+if (namespaceStorageConfig != null) {
+ LOGGER.debug(
+ "Applying storage name override '{}' on namespace {}",
+ namespaceStorageConfig.getStorageName(),
+ namespace);
+ entity =
+ new NamespaceEntity(
+ new PolarisEntity.Builder(nsBuilder.build())
+ .addInternalProperty(
+
PolarisEntityConstants.getStorageConfigInfoPropertyName(),
+ namespaceStorageConfig.serialize())
Review Comment:
Why embed a copy of the config into the entity here?
Based on previous `dev` discussion (link below) my impression is that we'd
want to keep (named) storage configs at the catalog level for ease of
management.
https://lists.apache.org/thread/nwcc8krhvm35olgc22676mm12cjkpgn5
I'd expect entities to only reference storage config by name, and the config
resolution to happen when requested 🤔
--
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]
Re: [PR] feat(storage): inline polaris.storage.name overrides for namespace/table [polaris]
sririshindra commented on code in PR #4023:
URL: https://github.com/apache/polaris/pull/4023#discussion_r3002981976
##
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.
+ *
+ * Storage names are symbolic references to server-side credential
configurations (e.g. {@code
+ * polaris.storage.aws..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:
They serve different purposes: validate() is a pure check, while
normalizeBlankToNull() is a transformation. Combining them would make
validate() impure — callers wouldn't expect a validation method to also
mutate/transform the input. That said, I could rename normalizeBlankToNull to
something clearer like toNullIfBlank or inline it at the call sites if you'd
prefer. Open to suggestions.
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##
@@ -755,8 +806,19 @@ public boolean removeProperties(Namespace namespace,
Set properties)
Map 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 updatedInternalProperties =
+ new HashMap<>(entity.getInternalPropertiesAsMap());
+
updatedInternalProperties.remove(PolarisEntityConstants.getStorageConfigInfoPropertyName());
Review Comment:
I am not sure I understood your point correctly. But from what I understood,
the challenge is that polaris.storage.name is a user-facing property (in
getPropertiesAsMap()) while storage_name_override is an internal property (in
getInternalPropertiesAsMap()). These are separate maps, so a key transformation
before the first removal pass wouldn't help.
We'd still need to reach into both maps. The special if-block is the way we
bridge the user-facing property
Re: [PR] feat(storage): inline polaris.storage.name overrides for namespace/table [polaris]
dimas-b commented on code in PR #4023:
URL: https://github.com/apache/polaris/pull/4023#discussion_r2982623324
##
polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageConfigurationInfo.java:
##
@@ -180,6 +180,63 @@ public static Optional forEntityPath(
return Optional.empty();
}
+ /**
+ * Creates a copy of the given storage configuration with only the {@code
storageName} field
+ * replaced. All other fields (allowed locations, cloud identity, etc.) are
preserved from the
+ * base config.
+ *
+ * @param baseConfig the storage configuration to copy
+ * @param storageName the new storage name (may be null to clear an override)
+ * @return a new configuration instance with the updated storage name
+ * @throws IllegalArgumentException if the base config type is not supported
+ */
+ public static PolarisStorageConfigurationInfo withStorageName(
+ PolarisStorageConfigurationInfo baseConfig, @Nullable String
storageName) {
+if (baseConfig instanceof AwsStorageConfigurationInfo awsConfig) {
+ return AwsStorageConfigurationInfo.builder()
Review Comment:
Maybe `.builder().from(awsConfig)`, etc... ?
##
polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java:
##
@@ -169,6 +169,22 @@ public static void enforceFeatureEnabledOrThrow(
.defaultValue(false)
.buildFeatureConfiguration();
+ public static final FeatureConfiguration
ALLOW_STORAGE_NAME_OVERRIDE =
+ PolarisConfiguration.builder()
+ .key("ALLOW_STORAGE_NAME_OVERRIDE")
Review Comment:
Please add a CHANGELOG entry for this new flag.
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##
@@ -520,15 +534,35 @@ private void createNamespaceInternal(
baseLocation += "/";
}
-NamespaceEntity entity =
+enforceStorageNameOverrideEnabledIfRequested(metadata);
+
+PolarisStorageConfigurationInfo namespaceStorageConfig =
+storageConfigFromPropertyOverride(metadata,
resolvedParent.getRawFullPath());
+
+NamespaceEntity.Builder nsBuilder =
new NamespaceEntity.Builder(namespace)
.setCatalogId(getCatalogId())
.setId(getMetaStoreManager().generateNewEntityId(getCurrentPolarisContext()).getId())
.setParentId(resolvedParent.getRawLeafEntity().getId())
.setProperties(metadata)
.setCreateTimestamp(System.currentTimeMillis())
-.setBaseLocation(baseLocation)
-.build();
+.setBaseLocation(baseLocation);
+NamespaceEntity entity;
+if (namespaceStorageConfig != null) {
+ LOGGER.debug(
+ "Applying storage name override '{}' on namespace {}",
+ namespaceStorageConfig.getStorageName(),
+ namespace);
+ entity =
+ new NamespaceEntity(
+ new PolarisEntity.Builder(nsBuilder.build())
+ .addInternalProperty(
+
PolarisEntityConstants.getStorageConfigInfoPropertyName(),
+ namespaceStorageConfig.serialize())
Review Comment:
Why embed a copy of the config into the entity here?
Based on previous `dev` discussion (link below) my impression is that we'd
want to keep (named) storage configs at the catalog level for east of
management.
https://lists.apache.org/thread/nwcc8krhvm35olgc22676mm12cjkpgn5
I'd expect entities to only reference storage config by name, and the config
resolution to happen when requested 🤔
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##
@@ -1745,7 +1811,78 @@ private String newTableMetadataFilePath(TableMetadata
meta, int newVersion) {
}
}
- private static Map
buildTableMetadataPropertiesMap(TableMetadata metadata) {
+ /**
+ * Resolves a storage configuration override from the {@code
polaris.storage.name} property. If
+ * the property is present, walks the parent entity path to find the nearest
ancestor with a
+ * storage configuration and returns a copy with the storageName overridden.
+ *
+ * @param properties the namespace or table properties to inspect
+ * @param parentPath the parent entity path (root-to-leaf) to search for
inherited storage config
+ * @return the overridden config, or null if no override is requested
+ */
+ private static @Nullable PolarisStorageConfigurationInfo
storageConfigFromPropertyOverride(
+ Map properties, List parentPath) {
+if (!properties.containsKey(POLARIS_STORAGE_NAME_PROPERTY)) {
+ return null;
+}
+
+String rawValue = properties.get(POLARIS_STORAGE_NAME_PROPERTY);
+String storageName = StorageNameValidator.normalizeBlankToNull(rawValue);
+if (storageName != null) {
+ StorageNameValidator.validate(storageName);
+}
+
+PolarisStorageConfigurationInfo baseConfig =
des
Re: [PR] feat(storage): inline polaris.storage.name overrides for namespace/table [polaris]
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 properties)
Map 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 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 =
Re: [PR] feat(storage): inline polaris.storage.name overrides for namespace/table [polaris]
sririshindra commented on PR #4023: URL: https://github.com/apache/polaris/pull/4023#issuecomment-4089010698 > This should definitely go behind a feature flag since there's currently no way to limit what a user can put in `polaris.storage.name` property. > > I hate to be adding to the feature flag proliferation in polaris, but maybe there should be a way to configure at what level override is allowed to happen. I can see an admin wanting to allow override on namespace level, but not on table level since `create table` permissions are generally more liberally granted than `create namespace` ones. A more proper fix is to integrate `polaris.storage.name` property in the authorization step, but I'm not sure that's practical for internal rbac. Thanks for the review, @tokoko! I'll go ahead and put this behind a feature flag. I intentionally kept authorization out of scope for this phase, but we can definitely address integrating `polaris.storage.name` into the authz step (and adding level-based configuration) in a follow-up PR. -- 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]
Re: [PR] feat(storage): inline polaris.storage.name overrides for namespace/table [polaris]
tokoko commented on PR #4023: URL: https://github.com/apache/polaris/pull/4023#issuecomment-4087812921 This should definitely go behind a feature flag since there's currently no way to limit what a user can put in `polaris.storage.name` property. I hate to be adding to the feature flag proliferation in polaris, but maybe there should be a way to configure at what level override is allowed to happen. I can see an admin wanting to allow override on namespace level, but not on table level since `create table` permissions are generally more liberally granted than `create namespace` ones. A more proper fix is to integrate `polaris.storage.name` property in the authorization step, but I'm not sure that's practical for internal rbac. -- 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]
