sririshindra commented on PR #3719: URL: https://github.com/apache/polaris/pull/3719#issuecomment-3938033034
> I do not mind having this in the feature branch, but I do think the overwrite behavior deserves more thoughts wrt authZ and the behavior when table-attributes like the base-location or the write-(meta)data-locations are different. Totally agree this deserves careful thought — a few notes on what this PR covers: - AuthZ: We settled on REGISTER_TABLE_OVERWRITE mapped to TABLE_FULL_METADATA. The rationale is that replacing a table's metadata pointer is semantically equivalent to simultaneously dropping the old pointer and registering a new one, so it warrants the strictest table-level privilege rather than just TABLE_WRITE_PROPERTIES (which covers normal metadata updates). TABLE_FULL_METADATA also encompasses both TABLE_CREATE and TABLE_DROP individually. Happy to revisit if there's a more appropriate granularity in mind. - Location changes: The implementation mirrors what doCommit does when the table location changes. It calls StorageUtil.getLocationsUsedByTable(metadata.location(), metadata.properties()) to collect all locations referenced by the new metadata — including write.data.path and write.metadata.path if set — and then runs them through CatalogUtils.validateLocationsForTableLike (checks against the resolved storage config) and validateNoLocationOverlap (checks for conflicts with existing tables). So a metadata file pointing to a different base location or write paths will be validated and rejected if those fall outside the allowed storage configuration. > The new operation(`REGISTER_TABLE_OVERWRITE(TABLE_FULL_METADATA)`) looks good to me. > > The location concern is also valid, I think it should be similar to location checking in `updateTable`, more details could be find here > > https://github.com/apache/polaris/blob/24c9f238d5945d172321e6580730cef1eefb5f21/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java#L1541 > > . agreed it should mirror updateTable. This is addressed in the latest commit: overwriteRegisteredTable now performs the same three checks that doCommit does when the table location changes: 1. CatalogUtils.validateLocationsForTableLike — validates all data locations against the resolved storage configuration 2. validateNoLocationOverlap — ensures the new metadata location doesn't overlap with an existing table 3. validateMetadataFileInTableDir — ensures the metadata file is within the table's directory structure The resolved entity used for storage context is now obtained via getPassthroughResolvedPath on the table itself (rather than getResolvedPath on the namespace), which also aligns with how doCommit resolves storage credentials for an existing table. -- 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]
