yuqi1129 commented on code in PR #13957: URL: https://github.com/apache/gravitino/pull/13957#discussion_r4396050881
########## core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/PolicyRoleRelBaseSQLProvider.java: ########## @@ -0,0 +1,131 @@ +/* + * 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.gravitino.storage.relational.mapper.provider.base; + +import static org.apache.gravitino.storage.relational.mapper.PolicyRoleRelMapper.POLICY_ROLE_RELATION_TABLE_NAME; + +import org.apache.gravitino.storage.relational.mapper.MetalakeMetaMapper; +import org.apache.gravitino.storage.relational.mapper.PolicyMetaMapper; +import org.apache.gravitino.storage.relational.mapper.provider.DatabaseTimeSQL; +import org.apache.gravitino.storage.relational.po.PolicyRoleRelPO; +import org.apache.ibatis.annotations.Param; + +/** Base SQL provider for policy-to-role references. */ +public class PolicyRoleRelBaseSQLProvider { + + /** Returns SQL for listing the role references of one policy. */ + public String listByPolicyId(@Param("policyId") Long policyId) { + return selectColumns() + + joins() + + " WHERE prr.policy_id = #{policyId}" + + activePredicates() + + " ORDER BY prr.role_name"; + } + + /** Returns SQL for listing the policies that reference one role name. */ + public String listByRoleName( + @Param("metalakeName") String metalakeName, @Param("roleName") String roleName) { + return selectColumns() + + joins() + + " WHERE pm.metalake_id IN (SELECT mm.metalake_id FROM " + + MetalakeMetaMapper.TABLE_NAME + + " mm WHERE mm.metalake_name = #{metalakeName} AND mm.deleted_at = 0)" + + " AND prr.role_name = #{roleName}" + + activePredicates() + + " ORDER BY pm.policy_name"; + } + + /** Returns SQL for inserting one reference. */ + public String insert(@Param("relation") PolicyRoleRelPO relation) { + return "INSERT INTO " + + POLICY_ROLE_RELATION_TABLE_NAME + + " (policy_id, role_name, current_version, last_version, deleted_at)" + + " VALUES (#{relation.policyId}, #{relation.roleName}," + + " #{relation.currentVersion}, #{relation.lastVersion}, #{relation.deletedAt})"; + } + + /** Returns SQL for soft-deleting one reference with a version CAS. */ + public String softDeleteByIdAndVersion(@Param("relation") PolicyRoleRelPO relation) { + return "UPDATE " + + POLICY_ROLE_RELATION_TABLE_NAME + // Assign last_version first so MySQL computes both columns from the version observed + // before this update. + + " SET last_version = current_version + 1," + + " current_version = current_version + 1," + + " deleted_at = " + + deletedAtNowExpression() Review Comment: [Important] The diff-based update avoids rewriting retained references, but the deletion timestamp still cannot safely distinguish historical rows in the unique key `(policy_id, role_name, deleted_at)`. For example, remove `analyst`, add it back, then remove it again. If the two removals receive the same millisecond timestamp, the second soft delete collides with the first tombstone. Serializing writes on the policy row guarantees ordering, not distinct clock values. I reproduced this on H2 by fixing only `deletedAtNowExpression()` to one timestamp and keeping the real mapper, schema, transactions, and `backend.update` calls. The third update fails with SQLState 23505 in `softDeleteByIdAndVersion`. It becomes `EntityAlreadyExistsException`, and `PolicyManager.alterPolicy` then reports that the new policy name already exists, ultimately returning an internal error. Could we separate active-reference uniqueness from historical-row identity, or otherwise guarantee unique tombstones, and cover remove/re-add/remove (and policy deletion after re-add) with a fixed-clock test? The comment in `updateRoleRelationsWithoutCommit` should also avoid claiming that the diff alone rules out timestamp collisions. This verifies the boundary condition; I have not measured its frequency with real request timing or run the counterexample on MySQL/PostgreSQL. ########## core/src/main/java/org/apache/gravitino/storage/relational/service/MetalakeMetaService.java: ########## @@ -401,6 +406,9 @@ public boolean deleteMetalake(NameIdentifier ident, boolean cascade) { () -> SessionUtils.doWithoutCommit( PolicyTagRelMapper.class, mapper -> mapper.softDeleteByMetalakeId(metalakeId)), + () -> + SessionUtils.doWithoutCommit( + PolicyRoleRelMapper.class, mapper -> mapper.softDeleteByMetalakeId(metalakeId)), Review Comment: [Important] Could we also fence policy updates in the non-cascade path before cleaning these references? The cascade path deletes the policy rows before this cleanup, but this branch leaves the policy rows active. I reproduced this interleaving on H2 using the real backend: 1. A policy update reads the existing policy and pauses in its updater callback. 2. Another thread completes `backend.delete(metalakeIdent, METALAKE, false)` for a metalake with a policy but no catalogs. 3. The policy update resumes. Its policy-row CAS still succeeds, and it inserts a new active role reference after the metalake cleanup has committed. A direct count of `policy_role_relation_meta WHERE policy_id = ? AND deleted_at = 0` then returns 1 rather than 0. The role-anchored query hides this row through its metalake filter, and legacy cleanup cannot remove it because it only collects soft-deleted rows. The parent-lifecycle gap predates this PR, but the new table inherits it. Could we fence the policy row in both deletion branches, or lock and revalidate the parent on the policy update path with a consistent lock order? Please add a non-cascade regression test that explicitly arranges this interleaving. This counterexample was run on H2; I have not run it on MySQL/PostgreSQL. ########## scripts/mysql/schema-2.0.0-mysql.sql: ########## @@ -424,6 +424,18 @@ CREATE TABLE IF NOT EXISTS `policy_tag_relation_meta` ( KEY `policy_tag_relation_meta_idx_tag_id` (`tag_id`) ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin COMMENT 'policy tag relation'; +CREATE TABLE IF NOT EXISTS `policy_role_relation_meta` ( + `id` BIGINT(20) UNSIGNED NOT NULL AUTO_INCREMENT COMMENT 'auto increment id', + `policy_id` BIGINT(20) UNSIGNED NOT NULL COMMENT 'policy id', + `role_name` VARCHAR(128) NOT NULL COMMENT 'role name referenced by the policy content', + `current_version` INT UNSIGNED NOT NULL DEFAULT 1 COMMENT 'policy role relation current version', + `last_version` INT UNSIGNED NOT NULL DEFAULT 1 COMMENT 'policy role relation last version', + `deleted_at` BIGINT(20) UNSIGNED NOT NULL DEFAULT 0 COMMENT 'policy role relation deleted at', + PRIMARY KEY (`id`), + UNIQUE KEY `policy_role_relation_meta_uk_pid_rn_del` (`policy_id`, `role_name`, `deleted_at`), Review Comment: [Important] Could we align the role-name equality used by this unique key and lookup with the equality used by policy content? `AccessControlContent.validate()` accepts both `"analyst"` and `"analyst "`, and the `LinkedHashSet` in `initializePolicyRoleRelPOs` treats them as different names. However, this table inherits `utf8mb4_bin`, a PAD SPACE collation: MySQL ignores trailing spaces in these comparisons. Consequently: - A new policy containing both names attempts two inserts that collide on this key, and the create path misreports the failure as an already-existing policy. - A policy containing only `"analyst "` can be returned by `listByRoleName(..., "analyst")`, although the content strings differ. Please either define and consistently validate/canonicalize the role-name contract, or use storage/comparison semantics that preserve the intended exact equality. The schema and upgrade script need the same treatment, with backend tests for the boundary. Trimming only this derived index would leave it inconsistent with authoritative content. Evidence: the H2 boundary test passes public content validation and stores the two distinct references; the MySQL conclusion follows from the schema/query and [MySQL's documented PAD SPACE behavior](https://dev.mysql.com/doc/refman/8.0/en/charset-binary-collations.html). I could not run the new MySQL counterexample locally, so this is not a claimed MySQL runtime reproduction. The production role-deletion consumer is explicitly deferred to M6; this comment does not claim a current authorization bypass. -- 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]
