<fix>[compute]: fix VM clone quota check fail#3624
<fix>[compute]: fix VM clone quota check fail#3624zstack-robot-1 wants to merge 1 commit into5.5.12from
Conversation
1.Clone a tenant VM using admin account, quota check didn't base on current role. 2.quota error is overwritten during exception handling. http://jira.zstack.io/browse/ZSTAC-83646 Resolves: ZSTAC-83646 Change-Id: I776c6b7a786d79746f616b716f736f646e686868
Walkthrough本次变更引入了基于会话账户UUID的管理员权限检查机制,并重构了配额验证流程,使其支持返回错误代码而非直接抛出异常。同时增强了主机分配消息和规范,使会话账户信息能够在分配流程中流转。 Changes
Sequence DiagramsequenceDiagram
participant QAF as QuotaAllocatorFlow
participant VMQ as VmQuotaOperator
participant QU as QuotaUtil
alt 管理员权限
QAF->>QAF: 检查sessionAccountUuid和管理员权限
QAF->>QAF: 直接跳过配额检查
QAF->>QAF: next(candidates)<br/>继续流程
else 非管理员权限
QAF->>VMQ: checkVmCupAndMemoryCapacityWithResult()
VMQ->>QU: checkQuotaAndReturn()
QU-->>VMQ: 返回 ErrorCode|null
VMQ-->>QAF: 返回 ErrorCode|null
alt 配额检查失败
QAF->>QAF: throw OperationFailureException(error)
else 配额检查成功
QAF->>VMQ: checkVmInstanceQuotaWithResult()
VMQ->>QU: checkQuotaAndReturn()
QU-->>VMQ: 返回 ErrorCode|null
VMQ-->>QAF: 返回 ErrorCode|null
alt 实例配额检查失败
QAF->>QAF: throw OperationFailureException(error)
else 实例配额检查成功
QAF->>QAF: next(candidates)<br/>继续流程
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
identity/src/main/java/org/zstack/identity/QuotaUtil.java (1)
35-35:⚠️ Potential issue | 🟡 Minor导入语句格式异常。
这一行的格式看起来有问题,
/**紧跟在分号后面,可能是合并冲突或编辑错误导致的。🐛 建议修复
-import static org.zstack.utils.clouderrorcode.CloudOperationsErrorCode.*;/** +import static org.zstack.utils.clouderrorcode.CloudOperationsErrorCode.*; + +/**🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@identity/src/main/java/org/zstack/identity/QuotaUtil.java` at line 35, The import line in QuotaUtil.java is malformed because a stray "/**" is appended to the end of the import statement; edit the import statement that references org.zstack.utils.clouderrorcode.CloudOperationsErrorCode (the static import line) to remove the trailing "/**" and ensure the line ends with a semicolon and nothing else so the file compiles and imports correctly.
🧹 Nitpick comments (2)
header/src/main/java/org/zstack/header/allocator/AllocateHostMsg.java (1)
34-34: 新增字段及其访问器的实现是正确的。该字段用于在主机分配流程中传递会话账户 UUID,以便在配额检查时区分操作者身份。
一个小建议:getter 和 setter 的格式(单行书写)与文件中其他方法风格不一致,建议调整为多行格式以保持一致性。
♻️ 可选的格式调整
- public String getSessionAccountUuid() { return sessionAccountUuid; } - - public void setSessionAccountUuid(String sessionAccountUuid) { this.sessionAccountUuid = sessionAccountUuid; } + public String getSessionAccountUuid() { + return sessionAccountUuid; + } + + public void setSessionAccountUuid(String sessionAccountUuid) { + this.sessionAccountUuid = sessionAccountUuid; + }Also applies to: 215-218
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@header/src/main/java/org/zstack/header/allocator/AllocateHostMsg.java` at line 34, 新增的字段 sessionAccountUuid 与其访问器 getSessionAccountUuid 和 setSessionAccountUuid 已实现正确,但访问器采用单行写法与文件其余方法风格不一致;请将 getSessionAccountUuid 和 setSessionAccountUuid 改为标准多行 getter/setter 格式(每个方法各占多行,带花括号和 return/this.assignment 行),并同时修正文件中另一处相同问题的访问器(提到的 215-218 位置的同名或类似方法)以保持风格一致性。header/src/main/java/org/zstack/header/allocator/HostAllocatorSpec.java (1)
165-171: getter/setter 的位置破坏了相关方法的分组。
getSessionAccountUuid()和setSessionAccountUuid()被放置在getL3NetworkUuids()和setL3NetworkUuids()之间,导致 L3NetworkUuids 的 getter/setter 被分隔开。建议将 sessionAccountUuid 的访问器移至字段声明附近的其他 getter/setter 区域,以保持代码组织的一致性。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@header/src/main/java/org/zstack/header/allocator/HostAllocatorSpec.java` around lines 165 - 171, The two accessors getSessionAccountUuid() and setSessionAccountUuid() are misplaced between getL3NetworkUuids() and setL3NetworkUuids(), splitting that pair; move getSessionAccountUuid() and setSessionAccountUuid() out of the middle and place them adjacent to the other sessionAccountUuid accessors (near the field declaration or the other related getters/setters) so that getL3NetworkUuids() and setL3NetworkUuids() remain contiguous in class HostAllocatorSpec.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@compute/src/main/java/org/zstack/compute/allocator/QuotaAllocatorFlow.java`:
- Around line 56-61: The admin-check currently in QuotaAllocatorFlow uses
AccountConstant.isAdminPermission(sessionAccountUuid) to skip quota checks;
change it to use QuotaUtil's account-type check so it matches other quota logic:
replace the AccountConstant.isAdminPermission(...) checks around
spec.getSessionAccountUuid() in QuotaAllocatorFlow with a call to new
QuotaUtil().isAdminAccount(...) (or the equivalent QuotaUtil.isAdminAccount
usage) so the flow treats all SystemAdmin accounts as admins consistent with
QuotaChecker and VmQuotaOperator.
---
Outside diff comments:
In `@identity/src/main/java/org/zstack/identity/QuotaUtil.java`:
- Line 35: The import line in QuotaUtil.java is malformed because a stray "/**"
is appended to the end of the import statement; edit the import statement that
references org.zstack.utils.clouderrorcode.CloudOperationsErrorCode (the static
import line) to remove the trailing "/**" and ensure the line ends with a
semicolon and nothing else so the file compiles and imports correctly.
---
Nitpick comments:
In `@header/src/main/java/org/zstack/header/allocator/AllocateHostMsg.java`:
- Line 34: 新增的字段 sessionAccountUuid 与其访问器 getSessionAccountUuid 和
setSessionAccountUuid 已实现正确,但访问器采用单行写法与文件其余方法风格不一致;请将 getSessionAccountUuid 和
setSessionAccountUuid 改为标准多行 getter/setter 格式(每个方法各占多行,带花括号和
return/this.assignment 行),并同时修正文件中另一处相同问题的访问器(提到的 215-218 位置的同名或类似方法)以保持风格一致性。
In `@header/src/main/java/org/zstack/header/allocator/HostAllocatorSpec.java`:
- Around line 165-171: The two accessors getSessionAccountUuid() and
setSessionAccountUuid() are misplaced between getL3NetworkUuids() and
setL3NetworkUuids(), splitting that pair; move getSessionAccountUuid() and
setSessionAccountUuid() out of the middle and place them adjacent to the other
sessionAccountUuid accessors (near the field declaration or the other related
getters/setters) so that getL3NetworkUuids() and setL3NetworkUuids() remain
contiguous in class HostAllocatorSpec.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
Run ID: 488d71f8-befd-414f-99bc-e337a0fb6044
📒 Files selected for processing (5)
compute/src/main/java/org/zstack/compute/allocator/QuotaAllocatorFlow.javacompute/src/main/java/org/zstack/compute/vm/VmQuotaOperator.javaheader/src/main/java/org/zstack/header/allocator/AllocateHostMsg.javaheader/src/main/java/org/zstack/header/allocator/HostAllocatorSpec.javaidentity/src/main/java/org/zstack/identity/QuotaUtil.java
| // skip checkquota if the operator is admin | ||
| String sessionAccountUuid = spec.getSessionAccountUuid(); | ||
| if (sessionAccountUuid != null && AccountConstant.isAdminPermission(sessionAccountUuid)) { | ||
| next(candidates); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 检查 AccountConstant.isAdminPermission 的所有使用场景,以及是否有其他管理员检查方式
echo "=== AccountConstant.isAdminPermission 的实现 ==="
ast-grep --pattern 'static boolean isAdminPermission($$$) {
$$$
}'
echo ""
echo "=== QuotaUtil.isAdminAccount 的使用 ==="
rg -n "isAdminAccount" --type java -C 2Repository: MatheMatrix/zstack
Length of output: 8656
🏁 Script executed:
# View the relevant lines in QuotaAllocatorFlow.java to confirm the usage
echo "=== QuotaAllocatorFlow.java lines 56-65 ==="
sed -n '56,65p' compute/src/main/java/org/zstack/compute/allocator/QuotaAllocatorFlow.java
echo ""
echo "=== Search for AccountConstant.isAdminPermission usage in quota-related code ==="
rg -n "isAdminPermission.*skip|skip.*isAdminPermission" --type java
echo ""
echo "=== Check other quota flow or allocator files for comparison ==="
fd ".*Allocator.*\.java$|.*Quota.*\.java$" compute/src/main/java --exec grep -l "isAdminPermission\|isAdminAccount" {} \;Repository: MatheMatrix/zstack
Length of output: 911
🏁 Script executed:
# Check how QuotaChecker.java and VmQuotaOperator.java handle the resource owner admin check
echo "=== QuotaChecker.java quota skip logic ==="
sed -n '35,50p' identity/src/main/java/org/zstack/identity/QuotaChecker.java
echo ""
echo "=== VmQuotaOperator.java quota check for resource owner ==="
sed -n '400,415p' compute/src/main/java/org/zstack/compute/vm/VmQuotaOperator.java
echo ""
echo "=== Check if there are existing quota checks in compute module ==="
rg -A 5 -B 5 "skip.*checkquota\|checkquota.*skip" compute/ --type javaRepository: MatheMatrix/zstack
Length of output: 1623
🏁 Script executed:
# Get the complete method context to understand the full quota check flow
echo "=== Full QuotaAllocatorFlow.java method context ==="
sed -n '50,100p' compute/src/main/java/org/zstack/compute/allocator/QuotaAllocatorFlow.java
echo ""
echo "=== Check QuotaUtil to understand when isAdminPermission vs isAdminAccount should be used ==="
sed -n '155,175p' identity/src/main/java/org/zstack/identity/QuotaUtil.javaRepository: MatheMatrix/zstack
Length of output: 2828
建议将管理员权限检查改为与其他配额检查保持一致。
在 QuotaAllocatorFlow.java 中,第 58 行和第 65 行使用 AccountConstant.isAdminPermission() 来跳过配额检查,但该方法仅检查账户 UUID 是否等于 INITIAL_SYSTEM_ADMIN_UUID(初始系统管理员)。
根据代码库中其他配额检查的实现模式,应该使用 QuotaUtil.isAdminAccount() 来检查,该方法会判断账户类型是否为 SystemAdmin(覆盖所有系统管理员账户)。具体体现在:
QuotaChecker.java第 41 行使用isAdminAccount()VmQuotaOperator.java第 408 行使用isAdminAccount()QuotaUtil.java第 166 行采用组合检查:(isAdminPermission() || isAdminAccount())
建议将第 58 行和第 65 行改为使用 new QuotaUtil().isAdminAccount() 以保持配额检查逻辑的一致性。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@compute/src/main/java/org/zstack/compute/allocator/QuotaAllocatorFlow.java`
around lines 56 - 61, The admin-check currently in QuotaAllocatorFlow uses
AccountConstant.isAdminPermission(sessionAccountUuid) to skip quota checks;
change it to use QuotaUtil's account-type check so it matches other quota logic:
replace the AccountConstant.isAdminPermission(...) checks around
spec.getSessionAccountUuid() in QuotaAllocatorFlow with a call to new
QuotaUtil().isAdminAccount(...) (or the equivalent QuotaUtil.isAdminAccount
usage) so the flow treats all SystemAdmin accounts as admins consistent with
QuotaChecker and VmQuotaOperator.
|
Comment from yaohua.wu: Review: MR !9480 — ZSTAC-83646 fix VM clone quota check fail关联 MR: premium !13346 变更概述本 MR 修复两个问题:
架构上设计合理: Warning
Suggestion
Verdict: ✅ APPROVED修复正确解决了 Jira 中描述的两个问题:admin clone 租户 VM 的 quota 绕过 + 错误信息保留。 🤖 Robot Reviewer |
1.Clone a tenant VM using admin account, quota check
didn't base on current role.
2.quota error is overwritten during exception handling.
http://jira.zstack.io/browse/ZSTAC-83646
Resolves: ZSTAC-83646
Change-Id: I776c6b7a786d79746f616b716f736f646e686868
sync from gitlab !9480