Dogface2k commented on issue #13796: URL: https://github.com/apache/cloudstack/issues/13796#issuecomment-5205072886
Thanks @winterhazel putting the complete fix back into 4.23 RC3 is the cleanest point for the main release line. I have rechecked the current runtime paths, the database upgrade hierarchy and the project-membership lifecycle before replying. For supported released-version upgrades, 4.22.1.0 → 4.23.0.0 is the correct repair boundary. schema-42210to42300-cleanup.sql already exists, and the default DbUpgrade.getCleanupScripts() implementation loads that version-specific resource automatically. Every supported upgrade path into 4.23 from 4.22.1 or an earlier release eventually crosses that transition. Therefore, if both the runtime prevention and the idempotent data repair ship together in RC3, we do not need to copy the same repair into every historical 4.20 → 4.21 → 4.22 transition or add it again to 4.23 → 4.24 solely for this defect. That achieves the purpose of carrying the repair forward without repeatedly executing identical cleanup during one upgrade. A future 4.22 maintenance backport is separate. The current 4.22 branch is 4.22.2.0-SNAPSHOT, but its database hierarchy currently stops at 4.22.0.0 → 4.22.1.0; upgrading from 4.22.1 to 4.22.2 otherwise falls through to NoopDbUpgrade. A proper 4.22 backport would therefore require a real Upgrade42210to42220 node/class and its cleanup resource, as well as the runtime fix. The 4.20 branch only needs its own 42030→42040 repair if 4.20.4 actually obtains an RM and is released. I would not restore PR #10008 verbatim, though. Its transaction removed project memberships first and then returned the result of _userDao.remove(userId). Transaction.execute commits whenever the callback returns normally, including when it returns false. That means the old ordering could commit the project cleanup while leaving the user active if the user DAO returned false. The required invariant is: all existing user validation, self-deletion prevention, account access, role and privilege checks remain unchanged; the current 4.23 API-key cleanup remains intact; the user removal, API-key cleanup and project cleanup run in one database transaction; no related cleanup is committed if _userDao.remove(userId) returns false; any exception from API-key or project cleanup escapes the transaction so every earlier database change is rolled back. One safe implementation shape is to load the user and their project-membership data before entering the transaction, execute _userDao.remove(userId) inside it, return false immediately if that operation fails, and only then perform the API-key and project cleanup using the already loaded user/membership data. If any later operation fails, the exception rolls back the soft delete as well. This preserves the existing Boolean failure contract while preventing partial cleanup. The project cleanup should not be implemented as only: projectAccountDao.removeUserFromProjects(userId); CloudStack’s normal deleteUserFromProject flow performs additional business bookkeeping. For each individual project membership it: removes the project_account entry; decrements the account’s ResourceType.project count when the removed membership is an Admin membership; removes any corresponding user-scoped project invitation. The user-deletion path should reuse or centralise those project-layer semantics rather than bypass them through a bulk DAO delete. Otherwise the NPE disappears but project quota/resource-count state can remain inflated until a later recalculation, which would be another partial fix rather than a clean invariant restoration. The upgrade repair should therefore remove only individual project memberships referencing soft-deleted users: DELETE pa FROM `cloud`.`project_account` pa JOIN `cloud`.`user` u ON u.`id` = pa.`user_id` WHERE u.`removed` IS NOT NULL; This is narrowly scoped and naturally idempotent: active-user memberships are untouched; account-level project memberships where user_id IS NULL are untouched; a second execution finds no matching membership rows. The migration also needs to leave project resource counts consistent. Those counts should be recalculated from the surviving Admin memberships rather than blindly decremented, because recalculation remains correct and idempotent if the migration is retried. User-scoped project invitations should be included in the cleanup audit for the same reason: their foreign-key cascade is triggered only by a physical user deletion, whereas CloudStack removes users by setting removed. There is one additional runtime path that should be covered before we say this class of corruption is closed: moveUser. The current implementation copies the user, soft-deletes the original row and persists the copy under the destination account. The new user receives a new numeric database ID, while existing project_account.user_id values continue to reference the removed original ID and retain the old account_id. That can recreate the same stale user reference and listing failure. The safest default on a user move is to remove the user-specific project memberships and invitations as part of the same move transaction, with the same project resource-count bookkeeping. Silently transferring Project Admin membership to another account would change the security principal and resource ownership. Such a transfer should only happen if it is deliberately specified, with both user_id and account_id updated atomically, destination access and resource limits validated, and the resource counts transferred correctly. I would also be comfortable with a defensive null check in ProjectJoinDaoImpl so one manually corrupted row cannot make the complete listProjects response fail, but only as secondary hardening. It must log and skip the impossible stale membership; it must not use findByIdIncludingRemoved to expose a deleted user, and it must not replace either the runtime prevention or the database repair. The regression coverage should prove: Successful direct user deletion removes the user, API keys and all user-specific project state. Admin and Regular project memberships receive the correct bookkeeping. Multiple memberships across multiple projects are handled. _userDao.remove returning false causes no API-key, project-membership, invitation or resource-count cleanup. A failure during API-key or project cleanup rolls back the user removal. Existing self-deletion, access and caller-privilege protections remain unchanged. Moving a project member cannot leave a membership pointing at the removed original user ID. The migration preserves an active-user membership and an account-level membership, removes a removed-user membership, and leaves project resource counts correct. Executing the migration twice produces the same final state. The 4.22.1.0 → 4.23.0.0 upgrade path actually loads and runs the cleanup resource. A mocked executeUpdate() unit test can verify that a statement was invoked, but proving the row selection, resource-count result and second-run idempotency needs a real MySQL/MariaDB-backed upgrade test or an explicitly documented database-level reproduction. One RC-specific caveat: a database already upgraded by RC2 may already report schema version 4.23.0.0, so moving from RC2 to RC3 would not normally rerun 42210→42300. If RC-to-RC database upgrades are intended to be supported, that path needs explicit handling or a documented one-off repair. This does not affect the normal supported upgrade path from released 4.22.x versions into 4.23 GA. With those conditions, RC3 is the right main-line location and the result is a complete fix rather than a null-check workaround or a one-time SQL repair. -- 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]
