<feature>[storage]: register and take over sblk#3539
Closed
ZStack-Robot wants to merge 1 commit into
Closed
Conversation
Walkthrough新增主存储一致性检查（GET /primary-storage/{uuid}/consistency）和接管（PUT /primary-storage/{uuid}/takeover）两个 REST API，包括请求/响应/事件类、枚举、SDK 对应实现、文档与基础处理逻辑及测试辅助方法。 Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as 客户端
participant SDK as SDK/Action
participant API as API Server
participant PSBase as PrimaryStorageBase
participant Provider as PrimaryStorageProvider
Client->>SDK: 调用 Check/Takeover Action
SDK->>API: 发送 REST 请求 (GET/PUT)
API->>PSBase: 转发 APICheck... / APITakeover... 消息
alt 存储类型支持
PSBase->>Provider: 执行检查或接管操作
Provider-->>PSBase: 返回结果 (consistent/reconnectResult 等)
PSBase-->>API: 封装 Reply/Event 并返回
else 不支持
PSBase-->>API: 返回“不支持”错误的 Reply/Event
end
API-->>SDK: HTTP 响应
SDK-->>Client: Result/异常
代码审查工作量估计🎯 3 (中等) | ⏱️ ~25 分钟 诗歌
🚥 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)
📝 Coding Plan
Comment |
Collaborator
|
Comment from yaohua.wu: Review: MR !9399 — ZSV-10000 (zstackio/zstack)跨仓库联合 Review: zstack + premium !13239 + zstack-utility !6774🔴 Critical
🟡 Warning
🟢 Suggestion
Verdict: REVISION_REQUIRED主要阻塞项：Takeover API 缺少前置一致性检查的服务端校验（Critical #1），可导致用户误操作造成存储不可逆损坏。 🤖 Robot Reviewer |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@header/src/main/java/org/zstack/header/storage/primary/ConsistencyCheckReasonDoc_zh_cn.groovy`:
- Around line 13-19: Update the inaccurate "since" version tags in
ConsistencyCheckReasonDoc_zh_cn.groovy: change the two occurrences of since
"4.7.0" (the one before the field block and the one inside the field block for
name "ordinal") to since "5.0.0" so the enum/documentation reflects the newly
introduced capability; ensure both instances are updated to the same "5.0.0"
value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bf7db6e5-5b97-4bbc-8a9b-27c6ca288910
⛔ Files ignored due to path filters (1)
conf/serviceConfig/primaryStorage.xmlis excluded by!**/*.xml
📒 Files selected for processing (24)
header/src/main/java/org/zstack/header/storage/primary/APICheckPrimaryStorageConsistencyMsg.javaheader/src/main/java/org/zstack/header/storage/primary/APICheckPrimaryStorageConsistencyMsgDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/storage/primary/APICheckPrimaryStorageConsistencyReply.javaheader/src/main/java/org/zstack/header/storage/primary/APICheckPrimaryStorageConsistencyReplyDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/storage/primary/APITakeoverPrimaryStorageEvent.javaheader/src/main/java/org/zstack/header/storage/primary/APITakeoverPrimaryStorageEventDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/storage/primary/APITakeoverPrimaryStorageMsg.javaheader/src/main/java/org/zstack/header/storage/primary/APITakeoverPrimaryStorageMsgDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/storage/primary/ConsistencyCheckReason.javaheader/src/main/java/org/zstack/header/storage/primary/ConsistencyCheckReasonDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/storage/primary/ReconnectResult.javaheader/src/main/java/org/zstack/header/storage/primary/ReconnectResultDoc_zh_cn.groovysdk/src/main/java/SourceClassMap.javasdk/src/main/java/org/zstack/sdk/CheckPrimaryStorageConsistencyAction.javasdk/src/main/java/org/zstack/sdk/CheckPrimaryStorageConsistencyResult.javasdk/src/main/java/org/zstack/sdk/ConsistencyCheckReason.javasdk/src/main/java/org/zstack/sdk/DiscoverSharedBlockGroupVgsAction.javasdk/src/main/java/org/zstack/sdk/DiscoverSharedBlockGroupVgsResult.javasdk/src/main/java/org/zstack/sdk/ReconnectResult.javasdk/src/main/java/org/zstack/sdk/SharedBlockGroupVgInfo.javasdk/src/main/java/org/zstack/sdk/TakeoverPrimaryStorageAction.javasdk/src/main/java/org/zstack/sdk/TakeoverPrimaryStorageResult.javastorage/src/main/java/org/zstack/storage/primary/PrimaryStorageBase.javatestlib/src/main/java/org/zstack/testlib/ApiHelper.groovy
Comment on lines
+13
to
+19
| since "4.7.0" | ||
| } | ||
| field { | ||
| name "ordinal" | ||
| desc "" | ||
| type "int" | ||
| since "4.7.0" |
There was a problem hiding this comment.
since 版本号疑似不一致，建议改为 5.0.0。
Line 13 和 Line 19 当前是 4.7.0，但该枚举与相关 API 是本次新增能力，版本标注不一致会影响文档可信度。
建议修改
field {
name "name"
desc "资源名称"
type "String"
- since "4.7.0"
+ since "5.0.0"
}
field {
name "ordinal"
desc ""
type "int"
- since "4.7.0"
+ since "5.0.0"
}🤖 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/storage/primary/ConsistencyCheckReasonDoc_zh_cn.groovy`
around lines 13 - 19, Update the inaccurate "since" version tags in
ConsistencyCheckReasonDoc_zh_cn.groovy: change the two occurrences of since
"4.7.0" (the one before the field block and the one inside the field block for
name "ordinal") to since "5.0.0" so the enum/documentation reflects the newly
introduced capability; ensure both instances are updated to the same "5.0.0"
value.
MatheMatrix
force-pushed
the
sync/tao.gan/sblk-ZSV-10000@@3
branch
from
ab573e8 to
77533d3
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@header/src/main/java/org/zstack/header/storage/primary/APICheckPrimaryStorageConsistencyMsg.java`:
- Around line 8-13: The APICheckPrimaryStorageConsistencyMsg class lacks a
permission/authorization annotation, widening the auth boundary; update the
APICheckPrimaryStorageConsistencyMsg declaration to include the appropriate
admin-level permission annotation (e.g., add the project's admin-only annotation
or an `@Action`(...) specifying admin category/roles) so only privileged users can
call this endpoint; ensure the annotation is placed on the
APICheckPrimaryStorageConsistencyMsg class and matches other admin-protected API
message classes in the codebase.
In
`@header/src/main/java/org/zstack/header/storage/primary/APITakeoverPrimaryStorageMsgDoc_zh_cn.groovy`:
- Around line 28-32: The API doc for the parameter with name "uuid" incorrectly
sets enclosedIn to "takeoverPrimaryStorage" while location is "url"; update the
APITakeoverPrimaryStorageMsgDoc_zh_cn.groovy entry for the parameter named
"uuid" so that enclosedIn is an empty string (or removed) when location is "url"
to reflect that the UUID is a URL path parameter and ensure examples render
correctly.
In `@storage/src/main/java/org/zstack/storage/primary/PrimaryStorageBase.java`:
- Around line 938-951: The takeover path in PrimaryStorageBase
(handle(APITakeoverPrimaryStorageMsg)) must enforce a server-side pre-check that
APICheckPrimaryStorageConsistencyMsg was performed and returned
consistent==false before proceeding; modify handle(APITakeoverPrimaryStorageMsg)
to synchronously/transactionally invoke or dispatch the consistency check (reuse
the existing
APICheckPrimaryStorageConsistencyMsg/APICheckPrimaryStorageConsistencyEvent flow
handled by handle(APICheckPrimaryStorageConsistencyMsg)), examine the check
result and if the check is missing or returns consistent==true publish an
APITakeoverPrimaryStorageEvent with an error (operr) rejecting the takeover,
otherwise continue with the existing takeover logic—this centralizes the guard
in PrimaryStorageBase so implementations cannot bypass it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 71060f03-e21f-4e56-92a0-7ba6d6c4081c
⛔ Files ignored due to path filters (1)
conf/serviceConfig/primaryStorage.xmlis excluded by!**/*.xml
📒 Files selected for processing (22)
header/src/main/java/org/zstack/header/storage/primary/APICheckPrimaryStorageConsistencyMsg.javaheader/src/main/java/org/zstack/header/storage/primary/APICheckPrimaryStorageConsistencyMsgDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/storage/primary/APICheckPrimaryStorageConsistencyReply.javaheader/src/main/java/org/zstack/header/storage/primary/APICheckPrimaryStorageConsistencyReplyDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/storage/primary/APITakeoverPrimaryStorageEvent.javaheader/src/main/java/org/zstack/header/storage/primary/APITakeoverPrimaryStorageEventDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/storage/primary/APITakeoverPrimaryStorageMsg.javaheader/src/main/java/org/zstack/header/storage/primary/APITakeoverPrimaryStorageMsgDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/storage/primary/ConsistencyCheckReason.javaheader/src/main/java/org/zstack/header/storage/primary/ReconnectResult.javasdk/src/main/java/SourceClassMap.javasdk/src/main/java/org/zstack/sdk/CheckPrimaryStorageConsistencyAction.javasdk/src/main/java/org/zstack/sdk/CheckPrimaryStorageConsistencyResult.javasdk/src/main/java/org/zstack/sdk/ConsistencyCheckReason.javasdk/src/main/java/org/zstack/sdk/DiscoverSharedBlockGroupVgsAction.javasdk/src/main/java/org/zstack/sdk/DiscoverSharedBlockGroupVgsResult.javasdk/src/main/java/org/zstack/sdk/ReconnectResult.javasdk/src/main/java/org/zstack/sdk/SharedBlockGroupVgInfo.javasdk/src/main/java/org/zstack/sdk/TakeoverPrimaryStorageAction.javasdk/src/main/java/org/zstack/sdk/TakeoverPrimaryStorageResult.javastorage/src/main/java/org/zstack/storage/primary/PrimaryStorageBase.javatestlib/src/main/java/org/zstack/testlib/ApiHelper.groovy
✅ Files skipped from review due to trivial changes (11)
- header/src/main/java/org/zstack/header/storage/primary/ReconnectResult.java
- sdk/src/main/java/org/zstack/sdk/ReconnectResult.java
- sdk/src/main/java/org/zstack/sdk/TakeoverPrimaryStorageResult.java
- sdk/src/main/java/org/zstack/sdk/DiscoverSharedBlockGroupVgsResult.java
- header/src/main/java/org/zstack/header/storage/primary/ConsistencyCheckReason.java
- header/src/main/java/org/zstack/header/storage/primary/APITakeoverPrimaryStorageEventDoc_zh_cn.groovy
- sdk/src/main/java/org/zstack/sdk/CheckPrimaryStorageConsistencyResult.java
- sdk/src/main/java/org/zstack/sdk/ConsistencyCheckReason.java
- header/src/main/java/org/zstack/header/storage/primary/APICheckPrimaryStorageConsistencyReply.java
- sdk/src/main/java/org/zstack/sdk/SharedBlockGroupVgInfo.java
- header/src/main/java/org/zstack/header/storage/primary/APICheckPrimaryStorageConsistencyReplyDoc_zh_cn.groovy
🚧 Files skipped from review as they are similar to previous changes (5)
- header/src/main/java/org/zstack/header/storage/primary/APITakeoverPrimaryStorageMsg.java
- sdk/src/main/java/SourceClassMap.java
- header/src/main/java/org/zstack/header/storage/primary/APITakeoverPrimaryStorageEvent.java
- testlib/src/main/java/org/zstack/testlib/ApiHelper.groovy
- sdk/src/main/java/org/zstack/sdk/CheckPrimaryStorageConsistencyAction.java
Comment on lines
+8
to
+13
| @RestRequest( | ||
| path = "/primary-storage/{uuid}/consistency", | ||
| responseClass = APICheckPrimaryStorageConsistencyReply.class, | ||
| method = HttpMethod.GET | ||
| ) | ||
| public class APICheckPrimaryStorageConsistencyMsg extends APISyncCallMessage implements PrimaryStorageMessage { |
There was a problem hiding this comment.
缺少权限注解，存在授权边界过宽风险。
Line 13 的 API 消息未声明权限控制（如管理员级别）。该接口与接管流程直接相关，建议在消息类上显式增加权限注解，避免普通租户可调用。
🤖 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/storage/primary/APICheckPrimaryStorageConsistencyMsg.java`
around lines 8 - 13, The APICheckPrimaryStorageConsistencyMsg class lacks a
permission/authorization annotation, widening the auth boundary; update the
APICheckPrimaryStorageConsistencyMsg declaration to include the appropriate
admin-level permission annotation (e.g., add the project's admin-only annotation
or an `@Action`(...) specifying admin category/roles) so only privileged users can
call this endpoint; ensure the annotation is placed on the
APICheckPrimaryStorageConsistencyMsg class and matches other admin-protected API
message classes in the codebase.
Comment on lines
+28
to
+32
| name "uuid" | ||
| enclosedIn "takeoverPrimaryStorage" | ||
| desc "主存储的UUID" | ||
| location "url" | ||
| type "String" |
There was a problem hiding this comment.
URL 参数 uuid 的 enclosedIn 配置不一致。
Line 29 的 uuid 位于 URL 路径中，enclosedIn 建议为空字符串；当前值可能导致文档示例体裁不准确。
建议修复
column {
name "uuid"
- enclosedIn "takeoverPrimaryStorage"
+ enclosedIn ""
desc "主存储的UUID"
location "url"
type "String"🤖 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/storage/primary/APITakeoverPrimaryStorageMsgDoc_zh_cn.groovy`
around lines 28 - 32, The API doc for the parameter with name "uuid" incorrectly
sets enclosedIn to "takeoverPrimaryStorage" while location is "url"; update the
APITakeoverPrimaryStorageMsgDoc_zh_cn.groovy entry for the parameter named
"uuid" so that enclosedIn is an empty string (or removed) when location is "url"
to reflect that the UUID is a URL path parameter and ensure examples render
correctly.
Comment on lines
+938
to
+951
| } else if (msg instanceof APITakeoverPrimaryStorageMsg) { | ||
| handle((APITakeoverPrimaryStorageMsg) msg); | ||
| } else if (msg instanceof APICheckPrimaryStorageConsistencyMsg) { | ||
| handle((APICheckPrimaryStorageConsistencyMsg) msg); | ||
| } else { | ||
| bus.dealWithUnknownMessage(msg); | ||
| } | ||
| } | ||
|
|
||
| protected void handle(APITakeoverPrimaryStorageMsg msg) { | ||
| APITakeoverPrimaryStorageEvent event = new APITakeoverPrimaryStorageEvent(msg.getId()); | ||
| event.setError(operr("takeover not supported for primary storage type[%s]", self.getType())); | ||
| bus.publish(event); | ||
| } |
There was a problem hiding this comment.
接管流程缺少服务端强制前置校验入口。
Line 938 到 Line 951 对 APITakeoverPrimaryStorageMsg 仅做分发/处理，没有统一强制“先完成一致性检查且 consistent=false”的门禁。对不可逆操作，这会形成可绕过风险（实现类一旦漏校验即直接执行）。建议在服务端统一增加强制前置校验，而不是只依赖调用顺序。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@storage/src/main/java/org/zstack/storage/primary/PrimaryStorageBase.java`
around lines 938 - 951, The takeover path in PrimaryStorageBase
(handle(APITakeoverPrimaryStorageMsg)) must enforce a server-side pre-check that
APICheckPrimaryStorageConsistencyMsg was performed and returned
consistent==false before proceeding; modify handle(APITakeoverPrimaryStorageMsg)
to synchronously/transactionally invoke or dispatch the consistency check (reuse
the existing
APICheckPrimaryStorageConsistencyMsg/APICheckPrimaryStorageConsistencyEvent flow
handled by handle(APICheckPrimaryStorageConsistencyMsg)), examine the check
result and if the check is missing or returns consistent==true publish an
APITakeoverPrimaryStorageEvent with an error (operr) rejecting the takeover,
otherwise continue with the existing takeover logic—this centralizes the guard
in PrimaryStorageBase so implementations cannot bypass it.
APIImpact Resolves: ZSV-10000 Change-Id: I70637377776e777070676c6a6c616e74786b6667
MatheMatrix
force-pushed
the
sync/tao.gan/sblk-ZSV-10000@@3
branch
from
77533d3 to
dac6ffe
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
sdk/src/main/java/org/zstack/sdk/DiscoverSharedBlockGroupVgsAction.java (1)
31-35: 建议为systemTags和userTags添加泛型类型参数当前使用了原始类型
java.util.List，缺少泛型参数，这会降低类型安全性。PR 评审意见中也提到了相关类DiscoverSharedBlockGroupVgsResult存在类似的原始类型问题。如果这是自动生成的 SDK 代码且需要保持与其他 Action 类的一致性，可以忽略此建议。
♻️ 建议的修改
`@Param`(required = false) -public java.util.List systemTags; +public java.util.List<String> systemTags; `@Param`(required = false) -public java.util.List userTags; +public java.util.List<String> userTags;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/main/java/org/zstack/sdk/DiscoverSharedBlockGroupVgsAction.java` around lines 31 - 35, The fields systemTags and userTags in DiscoverSharedBlockGroupVgsAction are declared as raw java.util.List; update these to use a concrete generic type (e.g., List<String>) to restore type safety and mirror other Action classes; also update the corresponding raw-list fields in DiscoverSharedBlockGroupVgsResult (and any related getters/setters or usages) to the same generic type and add/import java.util.List if needed so the signatures and usages remain consistent.sdk/src/main/java/org/zstack/sdk/CheckPrimaryStorageConsistencyAction.java (2)
31-35: 建议为 List 字段添加泛型类型
systemTags和userTags使用了原始类型java.util.List，缺少泛型参数会导致类型安全警告，并可能在运行时引发ClassCastException。根据 PR 评审意见中提到的类似问题（DiscoverSharedBlockGroupVgsResult 中的 raw Map），建议统一添加泛型以提升类型安全性。
建议添加泛型类型
`@Param`(required = false) -public java.util.List systemTags; +public java.util.List<String> systemTags; `@Param`(required = false) -public java.util.List userTags; +public java.util.List<String> userTags;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/main/java/org/zstack/sdk/CheckPrimaryStorageConsistencyAction.java` around lines 31 - 35, The fields systemTags and userTags are declared as raw java.util.List which causes unchecked warnings and unsafe casts; update their declarations in CheckPrimaryStorageConsistencyAction to use a concrete generic type (e.g., List<String> or the appropriate tag type) instead of raw List, update any related getters/setters or uses to match the generic type, and ensure imports remain unchanged and compilation fixes any downstream unchecked warnings (target the fields named systemTags and userTags).
5-5: 冗余的通配符导入
import org.zstack.sdk.*;是多余的，因为当前类已经在org.zstack.sdk包中，同包内的类无需显式导入。建议移除冗余导入
package org.zstack.sdk; import java.util.HashMap; import java.util.Map; -import org.zstack.sdk.*; public class CheckPrimaryStorageConsistencyAction extends AbstractAction {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/main/java/org/zstack/sdk/CheckPrimaryStorageConsistencyAction.java` at line 5, 在 CheckPrimaryStorageConsistencyAction 类中移除冗余的通配符导入声明 `import org.zstack.sdk.*;`（类已位于 org.zstack.sdk 包内，同包类无需导入），确保只保留必要的显式导入或直接删除该导入行以消除警告并保持代码整洁。
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sdk/src/main/java/org/zstack/sdk/TakeoverPrimaryStorageAction.java`:
- Around line 28-35: 类 TakeoverPrimaryStorageAction 的请求参数缺少用于绑定“先执行一致性检查”的结果（例如
candidateVgUuid 或 checkToken），导致可绕过前置一致性约束；请在 TakeoverPrimaryStorageAction
中新增一个必选或条件必需的参数（例如 public java.lang.String candidateVgUuid 或 public
java.lang.String checkToken），并在服务端 takeover 处理逻辑（处理该 Action 的
API/服务方法）强制校验该参数与之前 check 操作的缓存/数据库记录一致，拒绝未匹配或缺失的 takeover 请求，以实现“先 check 再
takeover”绑定验证。
- Around line 91-99: PrimaryStorageBase.handle(APITakeoverPrimaryStorageMsg)
currently lacks the required precondition check that the target primary storage
was previously checked and found inconsistent; update the handler to enforce the
APICheckPrimaryStorageConsistencyMsg result before proceeding: either query the
stored consistency result for the given primary storage UUID or synchronously
send/handle an APICheckPrimaryStorageConsistencyMsg and inspect its response,
and if the response's consistent flag is true (or no failing check exists)
throw/reply with an error to abort takeover; ensure you reference
PrimaryStorageBase.handle, APITakeoverPrimaryStorageMsg,
APICheckPrimaryStorageConsistencyMsg and the consistency response's consistent
boolean when implementing this validation.
---
Nitpick comments:
In `@sdk/src/main/java/org/zstack/sdk/CheckPrimaryStorageConsistencyAction.java`:
- Around line 31-35: The fields systemTags and userTags are declared as raw
java.util.List which causes unchecked warnings and unsafe casts; update their
declarations in CheckPrimaryStorageConsistencyAction to use a concrete generic
type (e.g., List<String> or the appropriate tag type) instead of raw List,
update any related getters/setters or uses to match the generic type, and ensure
imports remain unchanged and compilation fixes any downstream unchecked warnings
(target the fields named systemTags and userTags).
- Line 5: 在 CheckPrimaryStorageConsistencyAction 类中移除冗余的通配符导入声明 `import
org.zstack.sdk.*;`（类已位于 org.zstack.sdk
包内，同包类无需导入），确保只保留必要的显式导入或直接删除该导入行以消除警告并保持代码整洁。
In `@sdk/src/main/java/org/zstack/sdk/DiscoverSharedBlockGroupVgsAction.java`:
- Around line 31-35: The fields systemTags and userTags in
DiscoverSharedBlockGroupVgsAction are declared as raw java.util.List; update
these to use a concrete generic type (e.g., List<String>) to restore type safety
and mirror other Action classes; also update the corresponding raw-list fields
in DiscoverSharedBlockGroupVgsResult (and any related getters/setters or usages)
to the same generic type and add/import java.util.List if needed so the
signatures and usages remain consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4ec48d10-bcaf-43ab-bce4-869cbdca9a79
⛔ Files ignored due to path filters (1)
conf/serviceConfig/primaryStorage.xmlis excluded by!**/*.xml
📒 Files selected for processing (22)
header/src/main/java/org/zstack/header/storage/primary/APICheckPrimaryStorageConsistencyMsg.javaheader/src/main/java/org/zstack/header/storage/primary/APICheckPrimaryStorageConsistencyMsgDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/storage/primary/APICheckPrimaryStorageConsistencyReply.javaheader/src/main/java/org/zstack/header/storage/primary/APICheckPrimaryStorageConsistencyReplyDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/storage/primary/APITakeoverPrimaryStorageEvent.javaheader/src/main/java/org/zstack/header/storage/primary/APITakeoverPrimaryStorageEventDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/storage/primary/APITakeoverPrimaryStorageMsg.javaheader/src/main/java/org/zstack/header/storage/primary/APITakeoverPrimaryStorageMsgDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/storage/primary/ConsistencyCheckReason.javaheader/src/main/java/org/zstack/header/storage/primary/ReconnectResult.javasdk/src/main/java/SourceClassMap.javasdk/src/main/java/org/zstack/sdk/CheckPrimaryStorageConsistencyAction.javasdk/src/main/java/org/zstack/sdk/CheckPrimaryStorageConsistencyResult.javasdk/src/main/java/org/zstack/sdk/ConsistencyCheckReason.javasdk/src/main/java/org/zstack/sdk/DiscoverSharedBlockGroupVgsAction.javasdk/src/main/java/org/zstack/sdk/DiscoverSharedBlockGroupVgsResult.javasdk/src/main/java/org/zstack/sdk/ReconnectResult.javasdk/src/main/java/org/zstack/sdk/SharedBlockGroupVgInfo.javasdk/src/main/java/org/zstack/sdk/TakeoverPrimaryStorageAction.javasdk/src/main/java/org/zstack/sdk/TakeoverPrimaryStorageResult.javastorage/src/main/java/org/zstack/storage/primary/PrimaryStorageBase.javatestlib/src/main/java/org/zstack/testlib/ApiHelper.groovy
✅ Files skipped from review due to trivial changes (11)
- header/src/main/java/org/zstack/header/storage/primary/ReconnectResult.java
- sdk/src/main/java/org/zstack/sdk/ConsistencyCheckReason.java
- header/src/main/java/org/zstack/header/storage/primary/ConsistencyCheckReason.java
- sdk/src/main/java/org/zstack/sdk/ReconnectResult.java
- sdk/src/main/java/org/zstack/sdk/DiscoverSharedBlockGroupVgsResult.java
- sdk/src/main/java/org/zstack/sdk/TakeoverPrimaryStorageResult.java
- sdk/src/main/java/org/zstack/sdk/CheckPrimaryStorageConsistencyResult.java
- header/src/main/java/org/zstack/header/storage/primary/APICheckPrimaryStorageConsistencyReplyDoc_zh_cn.groovy
- header/src/main/java/org/zstack/header/storage/primary/APITakeoverPrimaryStorageEventDoc_zh_cn.groovy
- header/src/main/java/org/zstack/header/storage/primary/APICheckPrimaryStorageConsistencyReply.java
- sdk/src/main/java/org/zstack/sdk/SharedBlockGroupVgInfo.java
🚧 Files skipped from review as they are similar to previous changes (6)
- header/src/main/java/org/zstack/header/storage/primary/APITakeoverPrimaryStorageMsgDoc_zh_cn.groovy
- header/src/main/java/org/zstack/header/storage/primary/APICheckPrimaryStorageConsistencyMsg.java
- sdk/src/main/java/SourceClassMap.java
- storage/src/main/java/org/zstack/storage/primary/PrimaryStorageBase.java
- testlib/src/main/java/org/zstack/testlib/ApiHelper.groovy
- header/src/main/java/org/zstack/header/storage/primary/APITakeoverPrimaryStorageMsg.java
Comment on lines
+28
to
+35
| @Param(required = true, nonempty = false, nullElements = false, emptyString = true, noTrim = false) | ||
| public java.lang.String uuid; | ||
|
|
||
| @Param(required = false) | ||
| public java.util.List systemTags; | ||
|
|
||
| @Param(required = false) | ||
| public java.util.List userTags; |
There was a problem hiding this comment.
缺少“检查结果绑定”参数，接管请求可绕过前置一致性约束。
当前请求参数只有 uuid（及 tags），无法把“先执行一致性检查得到的目标信息”与 takeover 调用绑定。这样会使“先 check 再 takeover”在请求层脱钩，存在误接管风险。建议增加 candidateVgUuid（或一次性 check token）并在服务端强制校验。
建议修改（示例）
`@Param`(required = true, nonempty = false, nullElements = false, emptyString = true, noTrim = false)
public java.lang.String uuid;
+ // 由一致性检查返回并在 takeover 时强校验，防止绕过前置检查
+ `@Param`(required = true)
+ public java.lang.String candidateVgUuid;
+
`@Param`(required = false)
public java.util.List systemTags;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk/src/main/java/org/zstack/sdk/TakeoverPrimaryStorageAction.java` around
lines 28 - 35, 类 TakeoverPrimaryStorageAction 的请求参数缺少用于绑定“先执行一致性检查”的结果（例如
candidateVgUuid 或 checkToken），导致可绕过前置一致性约束；请在 TakeoverPrimaryStorageAction
中新增一个必选或条件必需的参数（例如 public java.lang.String candidateVgUuid 或 public
java.lang.String checkToken），并在服务端 takeover 处理逻辑（处理该 Action 的
API/服务方法）强制校验该参数与之前 check 操作的缓存/数据库记录一致，拒绝未匹配或缺失的 takeover 请求，以实现“先 check 再
takeover”绑定验证。
Comment on lines
+91
to
+99
| protected RestInfo getRestInfo() { | ||
| RestInfo info = new RestInfo(); | ||
| info.httpMethod = "PUT"; | ||
| info.path = "/primary-storage/{uuid}/takeover"; | ||
| info.needSession = true; | ||
| info.needPoll = true; | ||
| info.parameterName = "takeoverPrimaryStorage"; | ||
| return info; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
msg_file="$(fd 'APITakeoverPrimaryStorageMsg.java' | head -n 1 || true)"
if [ -z "${msg_file}" ]; then
echo "未找到 APITakeoverPrimaryStorageMsg.java"
exit 0
fi
echo "== Inspect: ${msg_file} =="
sed -n '1,260p' "${msg_file}"
echo
echo "== 权限注解核验 =="
rg -n -C2 '@APIPermission|AdminOnly|Action\.Root' "${msg_file}" || true
echo
echo "== 一致性检查绑定字段/校验核验 =="
rg -n -C2 'candidateVgUuid|consistency|check|token|takeover' "${msg_file}" || trueRepository: MatheMatrix/zstack
Length of output: 1537
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== 检查权限注解的使用范例 =="
rg -n '@APIPermission' --max-count=10 | head -20
echo
echo "== 查找 APITakeoverPrimaryStorageMsg 的处理器 =="
rg -n 'APITakeoverPrimaryStorageMsg|TakeoverPrimaryStorage' --max-count=15
echo
echo "== 检查是否有通用的 API 权限拦截器 =="
fd 'APIInterceptor|HostApiInterceptor|PermissionInterceptor' -t f | head -5Repository: MatheMatrix/zstack
Length of output: 79
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== 查找 Takeover 相关处理类 =="
fd -e java | xargs rg -l 'takeover|Takeover' | head -20
echo
echo "== 查找主存储管理相关处理器 =="
fd 'PrimaryStorageManager|PrimaryStorageHandler' -e java | head -10
echo
echo "== 查找 API 拦截器 =="
fd 'Interceptor' -e java | grep -i 'api\|permission' | head -10Repository: MatheMatrix/zstack
Length of output: 670
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== 检查 PrimaryStorageBase.java 中的权限和一致性检查 =="
file_path="storage/src/main/java/org/zstack/storage/primary/PrimaryStorageBase.java"
if [ -f "${file_path}" ]; then
line_count=$(wc -l < "${file_path}")
echo "文件行数: ${line_count}"
# 搜索 takeover 相关处理
rg -n 'takeover|Takeover' "${file_path}" | head -20
echo
echo "== 查找权限相关检查 =="
rg -n '@APIPermission|checkPermission|permission|permission' "${file_path}" | head -10
fi
echo
echo "== 检查 ConsistencyCheckReason.java =="
cat -n header/src/main/java/org/zstack/header/storage/primary/ConsistencyCheckReason.java 2>/dev/null || echo "未找到"
echo
echo "== 在 APITakeoverPrimaryStorageMsg 周围查找其他信息 =="
fd 'APITakeover' -e javaRepository: MatheMatrix/zstack
Length of output: 534
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== 查看 PrimaryStorageBase 中的 takeover 处理器实现 =="
file_path="storage/src/main/java/org/zstack/storage/primary/PrimaryStorageBase.java"
sed -n '935,980p' "${file_path}"
echo
echo "== 查找 PrimaryStorage 的子类实现（可能覆盖 takeover） =="
rg -l 'extends PrimaryStorageBase|implements.*PrimaryStorage' --max-count=10
echo
echo "== 查找是否有 API 层面的权限拦截器 =="
fd 'Interceptor' -e java | xargs rg -l 'APIMessage|APIPermission' | head -5Repository: MatheMatrix/zstack
Length of output: 18142
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== 查看 PrimaryStorageApiInterceptor.java =="
file_path="storage/src/main/java/org/zstack/storage/primary/PrimaryStorageApiInterceptor.java"
wc -l "${file_path}"
echo
rg -n 'takeover|Takeover|permission|Permission' "${file_path}" | head -20
echo
echo "== 查找通用 API 权限拦截逻辑 =="
find . -name "*Interceptor.java" -type f | xargs grep -l 'before.*APIMessage\|APIPermission' | head -3 | xargs -I {} sh -c 'echo "=== {} ===" && rg -n "@APIPermission|permission" {} | head -5'
echo
echo "== 检查是否有 APICheckPrimaryStorageConsistencyMsg 相关的一致性检查概念 =="
rg -n 'consistency|Consistency' header/src/main/java/org/zstack/header/storage/primary/ | head -15Repository: MatheMatrix/zstack
Length of output: 192
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== 查看 PrimaryStorageApiInterceptor 的完整内容 =="
cat -n storage/src/main/java/org/zstack/storage/primary/PrimaryStorageApiInterceptor.java
echo
echo "== 查找 APICheckPrimaryStorageConsistencyMsg =="
fd 'APICheckPrimaryStorageConsistencyMsg' -e javaRepository: MatheMatrix/zstack
Length of output: 13345
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== 查看 APICheckPrimaryStorageConsistencyMsg =="
cat -n header/src/main/java/org/zstack/header/storage/primary/APICheckPrimaryStorageConsistencyMsg.java
echo
echo "== 检查是否有在 takeover 前调用一致性检查的代码 =="
rg -n 'APICheckPrimaryStorageConsistency|takeover.*consistency|consistency.*takeover' --max-count=10
echo
echo "== 查找其他 API 的权限注解模式 =="
rg -B3 '@APIPermission' header/src/main/java/org/zstack/header/ | head -30Repository: MatheMatrix/zstack
Length of output: 4399
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== 查看 APITakeoverPrimaryStorageMsgDoc_zh_cn.groovy 前置条件 =="
cat header/src/main/java/org/zstack/header/storage/primary/APITakeoverPrimaryStorageMsgDoc_zh_cn.groovy
echo
echo "== 搜索 `@APIPermission` 注解的实际使用 =="
find header -name "*.java" -exec grep -l '@APIPermission' {} \; | head -5
echo
echo "== 查看权限注解的实际定义和使用 =="
fd '@APIPermission' -e java | head -3 | xargs -I {} sh -c 'echo "=== {} ===" && head -20 {}'
echo
echo "== 搜索是否有通用的权限检查机制（如 `@Admin` 或 Root 权限） =="
rg -n '@Admin|@Root|Action\.Root' header/src/main/java/org/zstack/header/ | head -10Repository: MatheMatrix/zstack
Length of output: 1404
🏁 Script executed:
#!/bin/bash
set -euo pipefrepública
echo "== 查找权限相关的注解定义 =="
fd -e java | xargs grep -l 'APIPermission\|@Admin\|@Root' | head -10
echo
echo "== 检查 APIMessage 或基类中的权限处理 =="
fd 'APIMessage.java' -e java | head -1 | xargs cat -n | head -50
echo
echo "== 搜索权限检查的常见关键词 =="
rg -n 'checkPermission|permission.*check|admin.*check|isAdmin' header/src/main/java/org/zstack/header/ | head -10
echo
echo "== 查找 interceptor 中是否有权限检查的通用机制 =="
find . -path ./header -prune -o -name "*Interceptor.java" -type f -print | head -3 | xargs -I {} sh -c 'echo "=== {} ===" && rg -n "permission|admin|intercept.*before" {} | head -5'Repository: MatheMatrix/zstack
Length of output: 121
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== 查找权限相关的注解定义 =="
fd -e java | xargs grep -l 'APIPermission\|@Admin\|@Root' 2>/dev/null | head -10 || echo "未找到权限注解"
echo
echo "== 检查 APIMessage 基类 =="
fd 'APIMessage.java' -e java | head -1 | xargs cat -n | head -60
echo
echo "== 搜索权限检查的常见关键词 =="
rg -n 'checkPermission|permission.*check|admin.*check|isAdmin' header/ 2>/dev/null | head -10 || echo "未找到权限检查代码"
echo
echo "== 查看 API 拦截器的 before 方法实现 =="
rg -n 'ApiMessageInterceptor|beforeCall|intercept' storage/src/main/java/org/zstack/storage/primary/PrimaryStorageApiInterceptor.javaRepository: MatheMatrix/zstack
Length of output: 3515
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== 查看 RBAC 权限检查逻辑 =="
file_path="header/src/main/java/org/zstack/header/identity/rbac/RBAC.java"
wc -l "${file_path}"
echo
echo "== 查找 isAdminOnlyAPI 和 takeover 相关 =="
rg -n 'takeover|Takeover|AdminOnly' "${file_path}" | head -20
echo
echo "== 查看 RBAC 权限定义部分 =="
sed -n '700,750p' "${file_path}"
echo
echo "== 查找 Takeover 在权限列表中是否被定义 =="
rg -n 'Takeover' header/src/main/java/org/zstack/header/identity/ | head -10Repository: MatheMatrix/zstack
Length of output: 3060
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== 查找权限定义文件（通常是 Permission 类或配置） =="
fd 'Permission' -e java | grep -i 'identity\|rbac' | head -10
echo
echo "== 查找 xml 中的权限配置 =="
fd -e xml | xargs grep -l 'permission\|identity' | head -5
echo
echo "== 在权限定义中查找 TakeoverPrimaryStorage =="
rg -n 'TakeoverPrimaryStorage\|takeover' header/src/main/java/org/zstack/header/identity/ 2>/dev/null | head -10 || echo "未找到"
echo
echo "== 查找默认权限列表 =="
fd 'DefaultPermission' -e java
echo
echo "== 查看权限如何标记为 admin only =="
rg -n 'AdminOnly\|admin.*only\|adminOnlyAPIs' header/src/main/java/org/zstack/header/identity/rbac/ | head -15Repository: MatheMatrix/zstack
Length of output: 408
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== 查找 Action.Root 或类似的权限常量 =="
rg -n 'Action\.Root\|class Action\|enum Action' header/ | head -20
echo
echo "== 查找 Permission 类的定义 =="
fd 'Permission.java' -e java | head -3 | xargs cat -n | head -80
echo
echo "== 查找已有权限注解的 API 示例 =="
find . -name "API*StorageMsg.java" | head -3 | xargs grep -B2 -A2 '@APIPermission\|@RestRequest' | head -30Repository: MatheMatrix/zstack
Length of output: 91
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== 查找 Permission 类 =="
fd 'Permission' -e java | head -10
echo
echo "== 查找如何定义 Admin-only API =="
rg -n '@RestRequest' header/src/main/java/org/zstack/header/storage/primary/ -A5 | head -40
echo
echo "== 检查是否有 admin-only 的标记机制 =="
rg -n 'adminOnly|admin.*only' header/src/main/java/org/zstack/header/ | head -10
echo
echo "== 查看权限检查的拦截器实现 =="
rg -l 'RBACAPIRequestChecker\|checkPermission' compute/ storage/ | head -3 | xargs -I {} sh -c 'echo "=== {} ===" && rg -n "takeover|Takeover" {} || echo "不涉及takeover"'Repository: MatheMatrix/zstack
Length of output: 5680
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== 查看主存储 RBACInfo.java =="
cat -n header/src/main/java/org/zstack/header/storage/primary/RBACInfo.java
echo
echo "== 验证 adminOnlyForAll() 的含义 =="
rg -n 'adminOnlyForAll' header/ -B2 -A2 | head -30
echo
echo "== 查看 RBACInfo 类的定义 =="
fd 'RBACInfo.java' header/src/main/java/org/zstack/header/console/ | head -1 | xargs cat -n | head -40Repository: MatheMatrix/zstack
Length of output: 5254
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== 重新查看 PrimaryStorageBase 中的 takeover 处理器，确保没有遗漏一致性检查 =="
sed -n '945,960p' storage/src/main/java/org/zstack/storage/primary/PrimaryStorageBase.java
echo
echo "== 查找是否有具体实现覆盖 takeover 处理器 =="
rg -n 'handle.*APITakeoverPrimaryStorageMsg' plugin/ storage/ | head -10
echo
echo "== 验证权限框架是否正确应用 adminOnlyForAll =="
rg -n 'adminOnlyForAll' header/src/main/java/org/zstack/header/identity/rbac/ | head -5Repository: MatheMatrix/zstack
Length of output: 1343
验证权限控制和一致性检查的实现位置。
权限控制已通过 RBAC 框架实现：header/src/main/java/org/zstack/header/storage/primary/RBACInfo.java 中声明 .adminOnlyForAll()，将所有主存储 API（包括 takeover）默认限制为仅管理员可访问。
但一致性检查的强制校验缺失：文档要求"前置条件：主存储必须通过 APICheckPrimaryStorageConsistencyMsg 检查且 consistent=false"，然而 PrimaryStorageBase.handle(APITakeoverPrimaryStorageMsg) 的实现中未见该前置检查的验证逻辑。应在处理器中添加对一致性检查结果的强制校验。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk/src/main/java/org/zstack/sdk/TakeoverPrimaryStorageAction.java` around
lines 91 - 99, PrimaryStorageBase.handle(APITakeoverPrimaryStorageMsg) currently
lacks the required precondition check that the target primary storage was
previously checked and found inconsistent; update the handler to enforce the
APICheckPrimaryStorageConsistencyMsg result before proceeding: either query the
stored consistency result for the given primary storage UUID or synchronously
send/handle an APICheckPrimaryStorageConsistencyMsg and inspect its response,
and if the response's consistent flag is true (or no failing check exists)
throw/reply with an error to abort takeover; ensure you reference
PrimaryStorageBase.handle, APITakeoverPrimaryStorageMsg,
APICheckPrimaryStorageConsistencyMsg and the consistency response's consistent
boolean when implementing this validation.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
APIImpact
Resolves: ZSV-10000
Change-Id: I70637377776e777070676c6a6c616e74786b6667
sync from gitlab !9399