[Iceberg 1.11] feat: add RegisterTable overwrite support; preserve correct auth for overwrites - #3719
Conversation
a3bcdb3 to
3ccadc8
Compare
|
Honestly, I think this 'overwrite` flag can be dangerous, because it allows users to change an existing table in a breaking way. The ability to replace an existing table should be guarded by a separate privilege. We should also think about what happens to the existing, old (meta)data files and how changes to the base-location and write-(meta)data properties are handled. I do not mind adding this to the feature branch, but it would be really good to have a separate discussion about the behavior before this change goes into the The safest approach IMO would be to initially just error out when |
|
It's an operation users need to be careful with. Adding an addition privilege sounds a good solution to me. In which case, the operation is blocked by default. It is only possible when a special privilege is granted. |
|
Thanks for the feedback @snazy and @flyrain. I understand the concern about safety.
As a next step, would you prefer I update the PR to throw an error when overwrite=true (disabling it for now) so we can merge the API changes first, or should I implement the stricter auth checks in this PR? My preference would be implement the auth checks in this PR itself, but I am open to suggestions. |
We will need a new operation in enum |
|
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. |
|
The new operation( The location concern is also valid, I think it should be similar to location checking in |
Totally agree this deserves careful thought — a few notes on what this PR covers:
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:
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. |
|
@adutra There is a gradle check failing in this PR. I am not sure if that failure has anything do with the changes in this PR. I tried debugging it but I didn't quite get it. Could you help lead me in the right direction please. @snazy @flyrain Could you please take another look at this PR when you get a chance. |
d6be3e7 to
d389cbd
Compare
| return; | ||
| } | ||
|
|
||
| // Table doesn't exist, fall back to standard register-table authorization. |
There was a problem hiding this comment.
I'm wondering how important this fallback is. If the table already exists, then eventually the caller needs REGISTER_TABLE_OVERWRITE role, but it turns out only when they attempted the operation. I'd expect anyone, who calls this endpoint to have REGISTER_TABLE_OVERWRITE, regardless of the 'state of the world'. What do you think?
There was a problem hiding this comment.
Agreed with @nandorKollar, I think a more consistent behavior here is still to use operation REGISTER_TABLE_OVERWRITE.
There was a problem hiding this comment.
We can also consolidate the code by
var target = getResolvedPath()
if (target == null) {
target = resolutionManifest.getResolvedPath(identifier.namespace(), true);
}
authorizer()
.authorizeOrThrow(
polarisPrincipal(),
resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
PolarisAuthorizableOperation.REGISTER_TABLE_OVERWRITE,
target,
null);
There was a problem hiding this comment.
I agree with this. I simplified the logic so overwrite=true always requires REGISTER_TABLE_OVERWRITE; the fallback path was removed. That makes auth deterministic and independent of runtime table-existence state.
| * exists | ||
| * @return the registered table | ||
| */ | ||
| public Table registerTable( |
There was a problem hiding this comment.
This method is not part of Catalog interface in Iceberg, in fact, there's no such method which corresponds to the register table overwrite semantic. Would it make sense to extend Iceberg Catalog interface with a corresponding method (registerTable(boolean overwrite))?
There was a problem hiding this comment.
I agree, I will raise a separate PR in the Iceberg repo for this. As the support for overwrite is added in the rest interface https://github.com/apache/iceberg/pull/15248/changes it is probably a good idea to add the same in the core Iceberg as well.
There was a problem hiding this comment.
Published apache/iceberg#15525 in Iceberg repo.
There was a problem hiding this comment.
Addressed all outstanding review items from this round: removed fallback auth logic for overwrite, clarified federated-catalog limitation in code/docs, and kept unsupported paths fail-fast with a clear error. cc: @nandorKollar @flyrain @snazy
| * exists | ||
| * @return the registered table | ||
| */ | ||
| public Table registerTable( |
There was a problem hiding this comment.
I agree, I will raise a separate PR in the Iceberg repo for this. As the support for overwrite is added in the rest interface https://github.com/apache/iceberg/pull/15248/changes it is probably a good idea to add the same in the core Iceberg as well.
| "Overwriting registered table for identifier={}, metadataFileLocation={}", | ||
| identifier, | ||
| metadataFileLocation); | ||
| return overwriteRegisteredTable(identifier, metadataFileLocation, locationDir); |
There was a problem hiding this comment.
IIUC, this path comes from overwrite=true and tableExists=true, in such case, would additional authz be required? i.e., privileges on the existing table entity, not just the namespace.
There was a problem hiding this comment.
The authZ usually happens in the class IcebergCatalogHandler.
…LE_FULL_METADATA
Enforce stricter permissions for RegisterTable with overwrite=true by requiring
TABLE_FULL_METADATA privilege (or CATALOG_MANAGE_CONTENT) instead of the previously
insufficient TABLE_WRITE_PROPERTIES. This aligns with the security requirement that
overwriting a table's metadata pointer is a destructive operation requiring both
create and drop capabilities.
Changes:
1. PolarisAuthorizableOperation.java
- Added new enum value: REGISTER_TABLE_OVERWRITE(TABLE_FULL_METADATA)
- Added static import for TABLE_FULL_METADATA privilege
- Positioned after REGISTER_TABLE for semantic grouping
2. IcebergCatalogHandler.java
- Updated authorizeUpdateTableOverwriteOrThrow() to use REGISTER_TABLE_OVERWRITE
- Updated documentation explaining why TABLE_FULL_METADATA is required
- Preserved fallback to REGISTER_TABLE when table doesn't exist
3. AbstractIcebergCatalogHandlerAuthzTest.java
- Updated testRegisterTableOverwriteSufficientPrivileges():
* Removed TABLE_WRITE_PROPERTIES from sufficient privileges
* Now only requires TABLE_FULL_METADATA or CATALOG_MANAGE_CONTENT
* Added detailed documentation of the privilege requirement
- Updated testRegisterTableOverwriteInsufficientPermissions():
* Added TABLE_WRITE_PROPERTIES to insufficient privileges list
* Documented why TABLE_CREATE or TABLE_DROP alone are insufficient
* Clarified that TABLE_FULL_METADATA is the minimum required
Security Properties:
✓ TABLE_FULL_METADATA: succeeds (contains both CREATE and DROP)
✓ CATALOG_MANAGE_CONTENT: succeeds (super-privilege for content management)
✗ TABLE_WRITE_PROPERTIES: fails (insufficient, no create/drop authority)
✗ TABLE_CREATE: fails individually (can't drop existing table pointer)
✗ TABLE_DROP: fails individually (can't create new entry)
✗ All read-only privileges: fail as expected
Rationale:
Table overwrite is semantically: (1) invalidate old table reference + (2) create
new one. Both operations require TABLE_FULL_METADATA. This prevents privilege
escalation where a user with only write-properties could replace another
principal's table.
daf6274 to
c3e546f
Compare
flyrain
left a comment
There was a problem hiding this comment.
LGTM. Thanks @sririshindra ! Left a few minor comments.
| "Overwriting registered table for identifier={}, metadataFileLocation={}", | ||
| identifier, | ||
| metadataFileLocation); | ||
| return overwriteRegisteredTable(identifier, metadataFileLocation, locationDir); |
There was a problem hiding this comment.
The authZ usually happens in the class IcebergCatalogHandler.
| Set<String> tableLocations = | ||
| StorageUtil.getLocationsUsedByTable(metadata.location(), metadata.properties()); |
There was a problem hiding this comment.
Nit: fit into one line
var tableLocations = StorageUtil.getLocationsUsedByTable(metadata);
There was a problem hiding this comment.
@flyrain Addressed it in the latest commit.
…og/iceberg/IcebergCatalog.java Co-authored-by: Yufei Gu <yufei@apache.org>
Thanks @flyrain , I addressed your remaining comments as well in the latest commits. |
|
Thanks @sririshindra for the change. Thanks @snazy @flyingImer @nandorKollar for the review! |
apache#4506) --------- Co-authored-by: Rishi <sririshindra@gmail.com>
apache#4506) --------- Co-authored-by: Rishi <sririshindra@gmail.com>
Add support for the new overwrite boolean on RegisterTableRequest (default: false) so clients can register a metadata location that replaces an existing table pointer. Implement register-table overwrite semantics:
Ensure safety/backward-compatibility:
Details / rationale:
The change implements the REST Catalog spec extension that adds an overwrite flag to RegisterTableRequest. This enables clients to atomically point an existing table identifier at a new metadata file (useful for moving or restoring table metadata).
Tests added/updated:
overwrite=false→ success.overwrite=false→ conflict / exception as before.overwrite=true→ success and the table's metadata-location is updated atomically.Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)