<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
Walkthrough本次变更在主机分配流程中引入了基于会话账户 UUID 的管理员权限短路路径，并将配额检查从直接抛异常改为返回 Changes
Sequence DiagramsequenceDiagram
participant QAF as QuotaAllocatorFlow
participant VMQ as VmQuotaOperator
participant QU as QuotaUtil
QAF->>QAF: throwExceptionIfIAmTheFirstFlow()
QAF->>QAF: spec.getAccountUuid() 并判断 isAdminPermission
alt 管理员
QAF->>QAF: next(candidates)
else 非管理员
QAF->>VMQ: checkVmCupAndMemoryCapacityWithResult(...)
VMQ->>QU: checkQuotaAndReturn(cpu/memory quota)
QU-->>VMQ: ErrorCode|null
VMQ-->>QAF: ErrorCode|null
alt 返回 ErrorCode
QAF->>QAF: throw OperationFailureException(error)
else 无错误
QAF->>VMQ: checkVmInstanceQuotaWithResult(...)
VMQ->>QU: checkQuotaAndReturn(instance quotas)
QU-->>VMQ: ErrorCode|null
VMQ-->>QAF: ErrorCode|null
alt 返回 ErrorCode
QAF->>QAF: throw OperationFailureException(error)
else 无错误
QAF->>QAF: next(candidates)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
compute/src/main/java/org/zstack/compute/allocator/QuotaAllocatorFlow.java
Show resolved
Hide resolved
|
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
MatheMatrix
force-pushed
the
sync/tian.huang/huangtian-ZSTAC-83646@@2
branch
from
c0bceed to
ee4b6be
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
compute/src/main/java/org/zstack/compute/allocator/QuotaAllocatorFlow.java (1)
56-61:⚠️ Potential issue | 🟠 Major管理员绕过条件仍然过窄，而且对漏传账号没有兜底。
这里仍然只看
AccountConstant.isAdminPermission(...)，它只覆盖INITIAL_SYSTEM_ADMIN_UUID，不能覆盖其他SystemAdmin；同时这段逻辑完全依赖spec.getAccountUuid()被上游显式透传，而HostAllocatorSpec.fromAllocationMsg(...)只是被动复制该字段，漏传时会静默退化回租户配额校验。建议和QuotaUtil.checkQuota(...)保持一致，按管理员账户类型判断，并补一层对操作账号缺失的兜底/校验。还需要顺手核查所有AllocateHostMsg/DesignatedAllocateHostMsg构造路径是否都会设置这个字段。🤖 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-bypass check in QuotaAllocatorFlow currently only calls AccountConstant.isAdminPermission(spec.getAccountUuid()) and silently skips when spec.getAccountUuid() is missing; change it to mirror QuotaUtil.checkQuota(...) by resolving the operator account type (e.g., check AccountType/SystemAdmin semantics rather than only INITIAL_SYSTEM_ADMIN_UUID) and treat null spec.getAccountUuid() as a missing operator—try to derive the account UUID from the original allocation message pathway (use HostAllocatorSpec.fromAllocationMsg(...) input or the AllocateHostMsg/DesignatedAllocateHostMsg constructors) and if still absent fail fast or log+reject instead of defaulting to tenant checks; update the logic in QuotaAllocatorFlow (and add unit checks) and verify all AllocateHostMsg / DesignatedAllocateHostMsg construction paths set the account UUID.
🧹 Nitpick comments (1)
compute/src/main/java/org/zstack/compute/vm/VmQuotaOperator.java (1)
237-270: 把checkVmCupAndMemoryCapacity*更正为Cpu。这两个是这次新增/扩展的公开入口，
Cup是拼写错误；等调用点扩散后再改名，兼容成本会更高。As per coding guidelines, “命名应尽量用完整的单词组合表达意图，并在名称中体现数据类型或用途”。
🤖 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/vm/VmQuotaOperator.java` around lines 237 - 270, The two public methods checkVmCupAndMemoryCapacityWithResult and checkVmCupAndMemoryCapacity contain a spelling mistake ("Cup")—rename them to checkVmCpuAndMemoryCapacityWithResult and checkVmCpuAndMemoryCapacity respectively, update all internal references/call sites to the new names, and adjust any related comments/Javadoc to use "Cpu" consistently; ensure compilation by refactoring callers in the same module and run tests to catch missed references.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@compute/src/main/java/org/zstack/compute/allocator/QuotaAllocatorFlow.java`:
- Around line 56-61: The admin-bypass check in QuotaAllocatorFlow currently only
calls AccountConstant.isAdminPermission(spec.getAccountUuid()) and silently
skips when spec.getAccountUuid() is missing; change it to mirror
QuotaUtil.checkQuota(...) by resolving the operator account type (e.g., check
AccountType/SystemAdmin semantics rather than only INITIAL_SYSTEM_ADMIN_UUID)
and treat null spec.getAccountUuid() as a missing operator—try to derive the
account UUID from the original allocation message pathway (use
HostAllocatorSpec.fromAllocationMsg(...) input or the
AllocateHostMsg/DesignatedAllocateHostMsg constructors) and if still absent fail
fast or log+reject instead of defaulting to tenant checks; update the logic in
QuotaAllocatorFlow (and add unit checks) and verify all AllocateHostMsg /
DesignatedAllocateHostMsg construction paths set the account UUID.
---
Nitpick comments:
In `@compute/src/main/java/org/zstack/compute/vm/VmQuotaOperator.java`:
- Around line 237-270: The two public methods
checkVmCupAndMemoryCapacityWithResult and checkVmCupAndMemoryCapacity contain a
spelling mistake ("Cup")—rename them to checkVmCpuAndMemoryCapacityWithResult
and checkVmCpuAndMemoryCapacity respectively, update all internal
references/call sites to the new names, and adjust any related comments/Javadoc
to use "Cpu" consistently; ensure compilation by refactoring callers in the same
module and run tests to catch missed references.
ℹ️ 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: cdbb1e58-8db8-4c58-b154-aba681aa2921
📒 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
🚧 Files skipped from review as they are similar to previous changes (2)
- header/src/main/java/org/zstack/header/allocator/AllocateHostMsg.java
- header/src/main/java/org/zstack/header/allocator/HostAllocatorSpec.java
2 participants
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