<fix>[kvm]: define secret on migrate destination#3716
Closed
zstack-robot-2 wants to merge 1 commit intofeature-zsv-5.0.0-vm-support-vtpm-and-secucebootfrom
Closed
<fix>[kvm]: define secret on migrate destination#3716zstack-robot-2 wants to merge 1 commit intofeature-zsv-5.0.0-vm-support-vtpm-and-secucebootfrom
zstack-robot-2 wants to merge 1 commit intofeature-zsv-5.0.0-vm-support-vtpm-and-secucebootfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough为 vTPM 迁移新增在目标主机定义 secret 的预处理步骤；扩展 SecretHost 消息以携带 Changes
Sequence Diagram(s)sequenceDiagram
participant VM as VM 控制流
participant KeyMgr as EncryptedResourceKeyManager
participant SrcHost as 源主机
participant SecretSvc as Secret 服务
participant DestHost as 目标主机
participant Agent as KVM Agent
VM->>SrcHost: 触发迁移准备（检测 vTPM）
SrcHost->>KeyMgr: getOrCreateKey(purpose:"vtpm", resource:TpmVO)
KeyMgr-->>SrcHost: 返回 ResourceKeyResult (DEK, keyVersion)
SrcHost->>SecretSvc: SecretHostGetCmd(vmUuid, purpose, keyVersion=0, usageInstance)
SecretSvc-->>SrcHost: 返回 secretUuid
SrcHost->>DestHost: 发送 SecretHostDefineMsg(usageInstance:"tpm0", secretUuid, dek, keyVersion=0)
DestHost->>Agent: 执行 SecretHostDefineMsg
Agent-->>DestHost: 定义成功/失败
DestHost-->>VM: 报告目标就绪或失败
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 兔子的诗句
🚥 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 unit tests (beta)
Comment |
MatheMatrix
force-pushed
the
sync/zstackio/define-secret-mig
branch
from
c9f14b9 to
6013cb5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmExtensions.java (1)
224-235:⚠️ Potential issue | 🟠 Major前置校验仍强依赖
providerName，会导致合法请求被提前失败。Line 224-227 在发送
SecretHostDefineMsg前要求providerName非空，但当前下游校验（KVMHost.handle(SecretHostDefineMsg)）要求的是vmUuid/purpose/keyVersion/usageInstance。这会在providerName缺失但新字段完整时造成不必要失败。建议修复
`@Override` public void run(FlowTrigger trigger, Map data) { - if (StringUtils.isBlank(context.providerName)) { - trigger.fail(operr("missing effective key provider name for tpm[uuid:%s] before define-secret-on-host", context.tpmUuid)); - return; - } - SecretHostDefineMsg innerMsg = new SecretHostDefineMsg(); innerMsg.setHostUuid(spec.getDestHost().getUuid()); innerMsg.setVmUuid(spec.getVmInventory().getUuid()); innerMsg.setDekBase64(context.dekBase64); innerMsg.setPurpose("vtpm"); innerMsg.setKeyVersion(0); innerMsg.setUsageInstance("tpm0"); innerMsg.setDescription("Define secret for VM " + spec.getVmInventory().getUuid());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmExtensions.java` around lines 224 - 235, Current pre-check aborts when context.providerName is blank which is unnecessary because downstream KVMHost.handle(SecretHostDefineMsg) only requires vmUuid/purpose/keyVersion/usageInstance; remove the hard fail on context.providerName in KvmTpmExtensions (the if block that calls trigger.fail when StringUtils.isBlank(context.providerName)) and instead ensure SecretHostDefineMsg is populated with vmUuid, hostUuid, dekBase64, purpose ("vtpm"), keyVersion (0) and usageInstance ("tpm0"); optionally emit a debug/warn log if providerName is missing but do not fail the operation so valid requests are forwarded to KVMHost.handle for final validation.
🤖 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/secret/SecretHostDefineMsg.java`:
- Around line 20-22: Update the class Javadoc on SecretHostDefineMsg to reflect
the current message contract: remove or correct the outdated statement that
"providerName" is required, and explicitly document the new fields keyVersion,
usageInstance, and secretUuid (their purpose and whether they are
required/optional). Ensure the comment blocks for SecretHostDefineMsg and any
related constructors or validation logic (e.g., any checks referencing
providerName) are synchronized with the actual field requirements so callers
aren't misled.
In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java`:
- Around line 3221-3279: The code reads the real keyVersion from
EncryptedResourceKeyRefVO into keyVersionHolder[0] but then hardcodes 0 when
calling getCmd.setKeyVersion(...) and innerMsg.setKeyVersion(...); change both
callers (the KVMAgentCommands.SecretHostGetCmd construction where
getCmd.setKeyVersion(0) and the SecretHostDefineMsg construction where
innerMsg.setKeyVersion(0)) to use the retrieved keyVersionHolder[0] (or a
properly validated int extracted from keyVersionObj) so both the source secret
get (in the SecretHostGet flow) and the destination define (in the
ensure-secret-on-dst flow) use the actual key version for the TPM resource.
---
Outside diff comments:
In `@plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmExtensions.java`:
- Around line 224-235: Current pre-check aborts when context.providerName is
blank which is unnecessary because downstream
KVMHost.handle(SecretHostDefineMsg) only requires
vmUuid/purpose/keyVersion/usageInstance; remove the hard fail on
context.providerName in KvmTpmExtensions (the if block that calls trigger.fail
when StringUtils.isBlank(context.providerName)) and instead ensure
SecretHostDefineMsg is populated with vmUuid, hostUuid, dekBase64, purpose
("vtpm"), keyVersion (0) and usageInstance ("tpm0"); optionally emit a
debug/warn log if providerName is missing but do not fail the operation so valid
requests are forwarded to KVMHost.handle for final validation.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2ec5232c-1239-4832-b159-306ea88e0b00
📒 Files selected for processing (5)
header/src/main/java/org/zstack/header/secret/SecretHostDefineMsg.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMAgentCommands.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMConstant.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMHost.javaplugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmExtensions.java
MatheMatrix
force-pushed
the
sync/zstackio/define-secret-mig
branch
2 times, most recently
from
74de51b to
c6955ac
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java (1)
3244-3249:⚠️ Potential issue | 🔴 Critical不要把
keyVersion再次写死成0。Line 3244 已经拿到了真实版本，但 Line 3248 和 Line 3285 仍然固定传
0。一旦 TPM 密钥发生轮转，源端取 secret 和目标端 define secret 都会落到错误版本，迁移准备可能失败，或者把目标宿主机绑定到旧 secret。🐛 建议修改
- getCmd.setKeyVersion(0); + getCmd.setKeyVersion(keyVersionHolder[0]); ... - innerMsg.setKeyVersion(0); + innerMsg.setKeyVersion(keyVersionHolder[0]);Also applies to: 3280-3285
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java` around lines 3244 - 3249, The code is hardcoding keyVersion to 0 when building KVMAgentCommands.SecretHostGetCmd (getCmd.setKeyVersion(0)) and a similar second command around the 3280-3285 block; replace those hardcoded zeros with the actual version you obtained in keyVersionHolder[0] so both the secret fetch (SecretHostGetCmd) and the secret define/send command use keyVersionHolder[0] instead of 0 (locate setKeyVersion calls on KVMAgentCommands.SecretHostGetCmd and the other Secret* command and pass keyVersionHolder[0]).
🧹 Nitpick comments (1)
plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmExtensions.java (1)
246-248: 注释存在拼写问题，建议顺手修正。Line 248 的
shapshot应为snapshot，避免后续阅读歧义。✏️ 建议修改
- // definitely don't need to delete keytool secret on shapshot case. + // definitely don't need to delete keytool secret on snapshot case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmExtensions.java` around lines 246 - 248, Fix the typo in the inline comment inside class KvmTpmExtensions referring to TpmSpec.resourceKeyCreatedNew: change "shapshot" to "snapshot" so the comment reads correctly (the surrounding comment about using clone op and not setting rollback flag remains unchanged); update the comment near the flow rollback/preReleaseVmResource discussion to use "snapshot" for clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java`:
- Around line 5525-5527: The code in KVMHost (around the ensure-secret handling)
calls reply.setError twice, with the second call overwriting the first; remove
the duplicate/old setError so only the correct validation message ("vmUuid,
purpose, keyVersion and usageInstance are required for ensure secret") is set.
Update the validation block in class KVMHost (the method handling the
ensure-secret msg) to keep a single reply.setError(...) call with the accurate
combined message and delete the extra reply.setError(...) that references the
old "vmUuid, purpose and keyVersion" text.
In `@runMavenProfile`:
- Line 498: 将 runMavenProfile 中对构建并发的修改回退：在包含 premium 构建配置的 mvn 命令（目前包含 "-T 8"
的那一行）移除并行参数，恢复为仅使用原始 profile 形式（即保留 -P premium 但去掉 -T 8），将并行构建优化改为单独的
PR，以避免把构建策略调整与本次修复（KVM 迁移目的端 secret 定义）耦合在一起。
---
Duplicate comments:
In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java`:
- Around line 3244-3249: The code is hardcoding keyVersion to 0 when building
KVMAgentCommands.SecretHostGetCmd (getCmd.setKeyVersion(0)) and a similar second
command around the 3280-3285 block; replace those hardcoded zeros with the
actual version you obtained in keyVersionHolder[0] so both the secret fetch
(SecretHostGetCmd) and the secret define/send command use keyVersionHolder[0]
instead of 0 (locate setKeyVersion calls on KVMAgentCommands.SecretHostGetCmd
and the other Secret* command and pass keyVersionHolder[0]).
---
Nitpick comments:
In `@plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmExtensions.java`:
- Around line 246-248: Fix the typo in the inline comment inside class
KvmTpmExtensions referring to TpmSpec.resourceKeyCreatedNew: change "shapshot"
to "snapshot" so the comment reads correctly (the surrounding comment about
using clone op and not setting rollback flag remains unchanged); update the
comment near the flow rollback/preReleaseVmResource discussion to use "snapshot"
for clarity.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 435ef374-6e95-422b-875c-355f80dc5a1d
📒 Files selected for processing (5)
header/src/main/java/org/zstack/header/keyprovider/EncryptedResourceKeyManager.javaheader/src/main/java/org/zstack/header/secret/SecretHostDefineMsg.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMHost.javaplugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmExtensions.javarunMavenProfile
🚧 Files skipped from review as they are similar to previous changes (1)
- header/src/main/java/org/zstack/header/secret/SecretHostDefineMsg.java
runMavenProfile
Outdated
| run_profile() { | ||
| if test x$1 = x'premium'; then | ||
| mvn -Dmaven.test.skip=true -P premium clean install | ||
| mvn -Dmaven.test.skip=true -P premium -T 8 clean install |
There was a problem hiding this comment.
请移除与本次修复目标无关的构建并发参数变更。
Line 498 将 premium 构建改为 -T 8，这会引入额外的构建行为变化，但当前 PR 目标是修复 KVM 迁移目的端 secret 定义。建议回退该行并将并行构建优化单独提 PR，避免把功能修复与构建策略调整耦合在一起。
建议修改
- mvn -Dmaven.test.skip=true -P premium -T 8 clean install
+ mvn -Dmaven.test.skip=true -P premium clean installBased on learnings: ZStack项目在cherry-pick操作中，即使发现了性能优化机会，也严格遵循不做额外修改的政策，优先保证cherry-pick的完整性和一致性。
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| mvn -Dmaven.test.skip=true -P premium -T 8 clean install | |
| mvn -Dmaven.test.skip=true -P premium clean install |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@runMavenProfile` at line 498, 将 runMavenProfile 中对构建并发的修改回退：在包含 premium 构建配置的
mvn 命令（目前包含 "-T 8" 的那一行）移除并行参数，恢复为仅使用原始 profile 形式（即保留 -P premium 但去掉 -T
8），将并行构建优化改为单独的 PR，以避免把构建策略调整与本次修复（KVM 迁移目的端 secret 定义）耦合在一起。
c6955ac to
591d75b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java (2)
5525-5527:⚠️ Potential issue | 🟡 Minor删除重复的
setError，否则新校验文案会被覆盖。这里前一个
setError刚设置完，后一个旧文案就把它覆盖掉了。最终调用方拿到的仍然是旧的"vmUuid, purpose and keyVersion..."，和当前校验条件不一致。🧹 建议修改
- reply.setError(operr("vmUuid, purpose, keyVersion and usageInstance are required for ensure secret")); reply.setError(operr("vmUuid, purpose and keyVersion are required for ensure secret")); + reply.setError(operr("vmUuid, purpose, keyVersion and usageInstance are required for ensure secret"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java` around lines 5525 - 5527, Remove the duplicated reply.setError call so the correct validation message isn't overwritten: in the validation block that checks msg.getVmUuid(), msg.getPurpose(), msg.getKeyVersion(), and msg.getUsageInstance(), keep a single reply.setError(operr(...)) that accurately lists all required fields (vmUuid, purpose, keyVersion and usageInstance) and remove the older reply.setError(operr("vmUuid, purpose and keyVersion are required for ensure secret")); ensure the remaining operr message matches the checked conditions and references the same validation logic around msg.getVmUuid(), msg.getPurpose(), msg.getKeyVersion(), msg.getUsageInstance(), and reply.setError.
3244-3249:⚠️ Potential issue | 🔴 Critical不要在迁移取/定义 secret 时把
keyVersion写死成0。Line 3244 已经拿到了真实的
keyVersion，但 Line 3248 和 Line 3285 仍然固定传0。一旦 TPM 密钥发生轮转，源端查 secret 和目标端 define secret 都会命中错误版本，迁移前准备会直接失败，或者把目标宿主机绑定到旧版本的 secret。🐛 建议修改
- getCmd.setKeyVersion(0); + getCmd.setKeyVersion(keyVersionHolder[0]); ... - innerMsg.setKeyVersion(0); + innerMsg.setKeyVersion(keyVersionHolder[0]);Also applies to: 3280-3286
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java` around lines 3244 - 3249, The code is incorrectly hardcoding keyVersion to 0 when fetching/defining TPM secrets; replace the literal 0 with the actual version you already read into keyVersionHolder[0]. Specifically, update KVMAgentCommands.SecretHostGetCmd.getCmd.setKeyVersion(0) and the corresponding SecretVMDefineCmd/SecretHostDefineCmd calls (the places around the getCmd and the defineCmd in the same method, including lines referenced at 3280-3286) to call setKeyVersion(keyVersionHolder[0]) (or an int variable derived from it) so both secret retrieval and secret definition use the real key version.
🤖 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/keyprovider/EncryptedResourceKeyManager.java`:
- Around line 32-35: The rollback currently deletes EncryptedResourceKeyRef
whenever ResourceKeyResult.isCreatedNewKey() is true, which removes pre-existing
placeholder refs and breaks TPM bindings; change the logic to track whether the
ref existed before this create operation (use a flag like
ResourceKeyResult.setRefExistedBeforeCreate(boolean)) and only delete the
EncryptedResourceKeyRef when the ref was created by this flow (i.e.,
refExistedBeforeCreate == false && isCreatedNewKey() == true); preserve the
intentional KvmTpmExtensions.preReleaseVmResource behavior that sets
result.setRefExistedBeforeCreate(true) for TPM so rollback will only recycle
secrets/materialized keys and not remove the placeholder ref.
In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java`:
- Around line 3200-3206: 在 NoRollbackFlow 中调用
resourceKeyManager.getOrCreateKey(...) 后，如果 result.isCreatedNewKey() 为
true，则在后续任一步骤（例如 get-source-secret-uuid 或 ensure-secret-on-dst）失败时需要回滚新建的
key；为此在 getOrCreateKey 的 ReturnValueCompletion<ResourceKeyResult> 回调中捕获失败分支并在
trigger 的 fail/rollback 路径或异常处理处调用
resourceKeyManager.rollbackCreatedKey(...)（或现有的 rollbackCreatedKey 方法），确保在
get-source-secret-uuid、ensure-secret-on-dst 等任一失败时都检查 result.isCreatedNewKey()
并执行相应回滚；更新与之相同模式的另一处（行号约 3301-3318）做同样处理。
- Around line 5525-5527: The code in KVMHost validates usageInstance but then
discards it by hardcoding KVMConstant.HOST_SECRET_USAGE_INSTANCE_VTPM when
creating/sending the SecretHostDefineMsg; update all places (e.g., where
SecretHostDefineMsg is constructed/sent around send/receive blocks at the shown
ranges and the similar block at 5601-5608) to set the usage instance from
msg.getUsageInstance() instead of the constant, and keep the existing validation
(StringUtils.isBlank(msg.getUsageInstance())) as-is; also remove the duplicated
reply.setError line so only one clear error is set when required fields are
missing.
---
Duplicate comments:
In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java`:
- Around line 5525-5527: Remove the duplicated reply.setError call so the
correct validation message isn't overwritten: in the validation block that
checks msg.getVmUuid(), msg.getPurpose(), msg.getKeyVersion(), and
msg.getUsageInstance(), keep a single reply.setError(operr(...)) that accurately
lists all required fields (vmUuid, purpose, keyVersion and usageInstance) and
remove the older reply.setError(operr("vmUuid, purpose and keyVersion are
required for ensure secret")); ensure the remaining operr message matches the
checked conditions and references the same validation logic around
msg.getVmUuid(), msg.getPurpose(), msg.getKeyVersion(), msg.getUsageInstance(),
and reply.setError.
- Around line 3244-3249: The code is incorrectly hardcoding keyVersion to 0 when
fetching/defining TPM secrets; replace the literal 0 with the actual version you
already read into keyVersionHolder[0]. Specifically, update
KVMAgentCommands.SecretHostGetCmd.getCmd.setKeyVersion(0) and the corresponding
SecretVMDefineCmd/SecretHostDefineCmd calls (the places around the getCmd and
the defineCmd in the same method, including lines referenced at 3280-3286) to
call setKeyVersion(keyVersionHolder[0]) (or an int variable derived from it) so
both secret retrieval and secret definition use the real key version.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 592f2b8e-2f2e-401e-a1ff-294a765fb171
📒 Files selected for processing (5)
header/src/main/java/org/zstack/header/keyprovider/EncryptedResourceKeyManager.javaheader/src/main/java/org/zstack/header/secret/SecretHostDefineMsg.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMHost.javaplugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmExtensions.javarunMavenProfile
✅ Files skipped from review due to trivial changes (1)
- header/src/main/java/org/zstack/header/secret/SecretHostDefineMsg.java
🚧 Files skipped from review as they are similar to previous changes (2)
- runMavenProfile
- plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmExtensions.java
header/src/main/java/org/zstack/header/keyprovider/EncryptedResourceKeyManager.java
Show resolved
Hide resolved
Comment on lines
+3200
to
+3206
| GetOrCreateResourceKeyContext keyCtx = new GetOrCreateResourceKeyContext(); | ||
| keyCtx.setResourceUuid(tpmUuid); | ||
| keyCtx.setResourceType(TpmVO.class.getSimpleName()); | ||
| keyCtx.setKeyProviderUuid(providerUuid); | ||
| keyCtx.setKeyProviderName(providerName); | ||
| keyCtx.setPurpose("vtpm"); | ||
| resourceKeyManager.getOrCreateKey(keyCtx, new ReturnValueCompletion<ResourceKeyResult>(trigger) { |
There was a problem hiding this comment.
给 getOrCreateKey() 新建出来的 key 补上失败回滚。
这里在 NoRollbackFlow 里调用了 resourceKeyManager.getOrCreateKey()，但后面的 get-source-secret-uuid 或 ensure-secret-on-dst 任一步失败时，都不会在 result.isCreatedNewKey() 为 true 时执行 rollbackCreatedKey()。这样会把刚创建的 key/tool secret 和 EncryptedResourceKeyRef 残留在系统里。
🩹 建议方向
- flow(new NoRollbackFlow() {
+ flow(new Flow() {
+ ResourceKeyResult createdKey;
+
String __name__ = "ensure-secret-on-dst-before-migrate";
`@Override`
public void run(FlowTrigger trigger, Map data) {
...
resourceKeyManager.getOrCreateKey(keyCtx, new ReturnValueCompletion<ResourceKeyResult>(trigger) {
`@Override`
public void success(ResourceKeyResult result) {
+ createdKey = result;
...
}
});
}
+
+ `@Override`
+ public void rollback(FlowRollback trigger, Map data) {
+ if (createdKey != null && createdKey.isCreatedNewKey()) {
+ resourceKeyManager.rollbackCreatedKey(createdKey, new Completion(trigger) {
+ `@Override`
+ public void success() {
+ trigger.rollback();
+ }
+
+ `@Override`
+ public void fail(ErrorCode errorCode) {
+ trigger.rollback();
+ }
+ });
+ } else {
+ trigger.rollback();
+ }
+ }
});Also applies to: 3301-3318
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java` around lines 3200 -
3206, 在 NoRollbackFlow 中调用 resourceKeyManager.getOrCreateKey(...) 后，如果
result.isCreatedNewKey() 为 true，则在后续任一步骤（例如 get-source-secret-uuid 或
ensure-secret-on-dst）失败时需要回滚新建的 key；为此在 getOrCreateKey 的
ReturnValueCompletion<ResourceKeyResult> 回调中捕获失败分支并在 trigger 的 fail/rollback
路径或异常处理处调用 resourceKeyManager.rollbackCreatedKey(...)（或现有的 rollbackCreatedKey
方法），确保在 get-source-secret-uuid、ensure-secret-on-dst 等任一失败时都检查
result.isCreatedNewKey() 并执行相应回滚；更新与之相同模式的另一处（行号约 3301-3318）做同样处理。
Comment on lines
+5525
to
+5527
| if (StringUtils.isBlank(msg.getVmUuid()) || StringUtils.isBlank(msg.getPurpose()) | ||
| || msg.getKeyVersion() == null || StringUtils.isBlank(msg.getUsageInstance())) { | ||
| reply.setError(operr("vmUuid, purpose, keyVersion and usageInstance are required for ensure secret")); reply.setError(operr("vmUuid, purpose and keyVersion are required for ensure secret")); |
There was a problem hiding this comment.
不要把调用方传入的 usageInstance 丢掉。
这里已经把 usageInstance 设成必填，而且迁移流程上面也明确传了 tpm0，但下发给 agent 时仍然固定写死 KVMConstant.HOST_SECRET_USAGE_INSTANCE_VTPM。这样 SecretHostDefineMsg 新增的字段实际上不生效；只要调用方传入值和常量不一致，就会在目标端定义到错误的 usage slot。
🛠️ 建议修改
- cmd.setUsageInstance(KVMConstant.HOST_SECRET_USAGE_INSTANCE_VTPM);
+ cmd.setUsageInstance(msg.getUsageInstance());Also applies to: 5601-5608
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java` around lines 5525 -
5527, The code in KVMHost validates usageInstance but then discards it by
hardcoding KVMConstant.HOST_SECRET_USAGE_INSTANCE_VTPM when creating/sending the
SecretHostDefineMsg; update all places (e.g., where SecretHostDefineMsg is
constructed/sent around send/receive blocks at the shown ranges and the similar
block at 5601-5608) to set the usage instance from msg.getUsageInstance()
instead of the constant, and keep the existing validation
(StringUtils.isBlank(msg.getUsageInstance())) as-is; also remove the duplicated
reply.setError line so only one clear error is set when required fields are
missing.
bdebd90 to
8ca2091
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (4)
header/src/main/java/org/zstack/header/keyprovider/EncryptedResourceKeyManager.java (1)
32-35:⚠️ Potential issue | 🟠 Major回滚契约会误删 TPM 预置 Ref，导致状态漂移
Line 32-35 当前把
createdNewKey=true直接等价为“删除EncryptedResourceKeyRef”。这会把 TPM 场景中原本就存在的占位 Ref 一并删除，失败回滚后状态不再是“回到原状”。建议修复（恢复区分“回滚前是否已存在 Ref”）
/** * Roll back a newly created resource key during upper-layer workflow rollback. * <p> - * When {`@link` ResourceKeyResult#isCreatedNewKey()} is true, the implementation deletes the - * key-tool secret if one was materialized, then removes the {`@code` EncryptedResourceKeyRef} row - * for the resource (same storage effect as detaching the key provider from the resource). + * When {`@link` ResourceKeyResult#isCreatedNewKey()} is true, the implementation deletes the + * key-tool secret if one was materialized. It removes the {`@code` EncryptedResourceKeyRef} row + * only when that ref was created by this flow (i.e. ref did not exist before create). * When {`@code` createdNewKey} is false (existing secret was reused), this is a no-op. */ void rollbackCreatedKey(ResourceKeyResult result, Completion completion); class ResourceKeyResult { ... private boolean createdNewKey; + private boolean refExistedBeforeCreate; ... + public boolean isRefExistedBeforeCreate() { + return refExistedBeforeCreate; + } + + public void setRefExistedBeforeCreate(boolean refExistedBeforeCreate) { + this.refExistedBeforeCreate = refExistedBeforeCreate; + } }Based on learnings: In ZStack's
KvmTpmExtensions.preReleaseVmResource,result.setRefExistedBeforeCreate(true)is intentional because TPM already has a placeholderEncryptedResourceKeyRefVO, so rollback should not delete that placeholder ref.🤖 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/keyprovider/EncryptedResourceKeyManager.java` around lines 32 - 35, The rollback logic in EncryptedResourceKeyManager currently treats ResourceKeyResult#isCreatedNewKey()==true as the signal to delete the EncryptedResourceKeyRef, which erroneously removes TPM placeholder refs; change the deletion condition to check a new/existing flag on the result (use result.isRefExistedBeforeCreate() or the existing result.setRefExistedBeforeCreate(true) set by KvmTpmExtensions.preReleaseVmResource) so that you only delete the EncryptedResourceKeyRef if the ref did NOT exist before creation; update the code paths in EncryptedResourceKeyManager that remove the key-tool secret and delete the EncryptedResourceKeyRef to consult result.isRefExistedBeforeCreate() (or add that boolean to ResourceKeyResult) instead of relying solely on isCreatedNewKey().plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java (3)
3229-3286:⚠️ Potential issue | 🔴 Critical不要把
keyVersion写死成0。这里已经查出了真实的
keyVersion，但源端SecretHostGetCmd和目标端SecretHostDefineMsg仍然固定传0。一旦 TPM 资源密钥发生轮转，源端取 secret 和目标端 define secret 都会落到错误版本。🐛 建议修改
- getCmd.setKeyVersion(0); + getCmd.setKeyVersion(keyVersionHolder[0]); ... - innerMsg.setKeyVersion(0); + innerMsg.setKeyVersion(keyVersionHolder[0]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java` around lines 3229 - 3286, The code currently hardcodes keyVersion to 0 when calling KVMAgentCommands.SecretHostGetCmd.setKeyVersion(...) and SecretHostDefineMsg.setKeyVersion(...); use the actual value read into keyVersionHolder[0] instead (replace the literal 0 with keyVersionHolder[0]) so both the source get and destination define use the real key version found earlier (ensure keyVersionHolder is in scope where used).
3200-3318:⚠️ Potential issue | 🟠 Major给
getOrCreateKey()新建出来的密钥补上失败回滚。这里仍然把
resourceKeyManager.getOrCreateKey(...)放在NoRollbackFlow里。只要后面的get-source-secret-uuid或ensure-secret-on-dst失败，result.isCreatedNewKey()为true时创建出来的 key/tool secret 和EncryptedResourceKeyRef就会残留。EncryptedResourceKeyManager已经提供了rollbackCreatedKey(...)，这里需要接上。🛠️ 建议修改
- flow(new NoRollbackFlow() { + flow(new Flow() { + ResourceKeyResult createdKey; String __name__ = "ensure-secret-on-dst-before-migrate"; `@Override` public void run(FlowTrigger trigger, Map data) { ... resourceKeyManager.getOrCreateKey(keyCtx, new ReturnValueCompletion<ResourceKeyResult>(trigger) { `@Override` public void success(ResourceKeyResult result) { + createdKey = result; ... } }); } + + `@Override` + public void rollback(FlowRollback trigger, Map data) { + if (createdKey != null && createdKey.isCreatedNewKey()) { + resourceKeyManager.rollbackCreatedKey(createdKey, new Completion(trigger) { + `@Override` + public void success() { + trigger.rollback(); + } + + `@Override` + public void fail(ErrorCode errorCode) { + logger.warn(String.format("failed to rollback created key for tpm[uuid:%s]: %s", tpmUuid, errorCode)); + trigger.rollback(); + } + }); + } else { + trigger.rollback(); + } + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java` around lines 3200 - 3318, getOrCreateKey can create a new key but the subsequent flows ("get-source-secret-uuid"/"ensure-secret-on-dst") run in a NoRollbackFlow chain, so if they fail and result.isCreatedNewKey() is true the created key and EncryptedResourceKeyRef are left behind; call resourceKeyManager.rollbackCreatedKey(...) to undo the creation on failure. Concretely, inside the success(ResourceKeyResult result) callback (where GetOrCreateResourceKeyContext is used and the FlowChain is started) capture the result and in the chain.error handler and any other failure paths (including the getOrCreateKey.fail callback) check result.isCreatedNewKey() and invoke resourceKeyManager.rollbackCreatedKey(result) before propagating the original error to trigger.fail; ensure rollback completes (or is attempted) then rethrow/forward the original ErrorCode to maintain semantics.
5525-5608:⚠️ Potential issue | 🟠 Major不要校验了
usageInstance却又把它丢掉。这里已经把
usageInstance设成必填，但下发给 agent 时仍然写死KVMConstant.HOST_SECRET_USAGE_INSTANCE_VTPM，新增字段实际上没有生效。另外 Line 5527 连续两次reply.setError(...)，最终会把包含usageInstance的新报错文案覆盖掉。🛠️ 建议修改
if (StringUtils.isBlank(msg.getVmUuid()) || StringUtils.isBlank(msg.getPurpose()) || msg.getKeyVersion() == null || StringUtils.isBlank(msg.getUsageInstance())) { - reply.setError(operr("vmUuid, purpose, keyVersion and usageInstance are required for ensure secret")); reply.setError(operr("vmUuid, purpose and keyVersion are required for ensure secret")); + reply.setError(operr("vmUuid, purpose, keyVersion and usageInstance are required for ensure secret")); bus.reply(msg, reply); return; } ... - cmd.setUsageInstance(KVMConstant.HOST_SECRET_USAGE_INSTANCE_VTPM); + cmd.setUsageInstance(msg.getUsageInstance());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java` around lines 5525 - 5608, The code validates msg.getUsageInstance() but then ignores it and hardcodes KVMConstant.HOST_SECRET_USAGE_INSTANCE_VTPM; also the initial input-check block calls reply.setError(...) twice which overwrites the intended message. Fix by removing the duplicate reply.setError(...) in the validation branch (keep a single, clear error about vmUuid, purpose, keyVersion and usageInstance) and set the agent command's usage instance from the request (replace cmd.setUsageInstance(KVMConstant.HOST_SECRET_USAGE_INSTANCE_VTPM) with cmd.setUsageInstance(msg.getUsageInstance())). Ensure you reference msg.getUsageInstance(), cmd.setUsageInstance(...), and the duplicate reply.setError(...) to locate the changes.
🧹 Nitpick comments (1)
header/src/main/java/org/zstack/header/secret/SecretHostGetMsg.java (1)
14-55: SecretHostGetMsg 的 usageInstance 字段在处理端被忽略，建议明确设计意图消息体新增的
usageInstance字段在KVMHost.handle(SecretHostGetMsg)中被完全忽略——处理器硬编码KVMConstant.HOST_SECRET_USAGE_INSTANCE_VTPM而不读取msg.getUsageInstance()。这与SecretHostEnsureMsg的处理器形成对比（后者验证并使用了 usageInstance）。建议：
- 若此字段当前不被使用，应移除或在 Javadoc 中明确说明；
- 若此字段应被尊重，处理器应改为读取
msg.getUsageInstance()（并提供默认值回退）。🤖 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/secret/SecretHostGetMsg.java` around lines 14 - 55, The new usageInstance field on SecretHostGetMsg is ignored by the handler: update KVMHost.handle(SecretHostGetMsg) to read msg.getUsageInstance() (falling back to KVMConstant.HOST_SECRET_USAGE_INSTANCE_VTPM when null/empty) instead of hardcoding KVMConstant.HOST_SECRET_USAGE_INSTANCE_VTPM, mirroring how SecretHostEnsureMsg is validated/used; alternatively, if usageInstance is intentionally unused, remove the field from SecretHostGetMsg or add Javadoc on the field to state it is deprecated/unused.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@header/src/main/java/org/zstack/header/keyprovider/EncryptedResourceKeyManager.java`:
- Around line 32-35: The rollback logic in EncryptedResourceKeyManager currently
treats ResourceKeyResult#isCreatedNewKey()==true as the signal to delete the
EncryptedResourceKeyRef, which erroneously removes TPM placeholder refs; change
the deletion condition to check a new/existing flag on the result (use
result.isRefExistedBeforeCreate() or the existing
result.setRefExistedBeforeCreate(true) set by
KvmTpmExtensions.preReleaseVmResource) so that you only delete the
EncryptedResourceKeyRef if the ref did NOT exist before creation; update the
code paths in EncryptedResourceKeyManager that remove the key-tool secret and
delete the EncryptedResourceKeyRef to consult result.isRefExistedBeforeCreate()
(or add that boolean to ResourceKeyResult) instead of relying solely on
isCreatedNewKey().
In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java`:
- Around line 3229-3286: The code currently hardcodes keyVersion to 0 when
calling KVMAgentCommands.SecretHostGetCmd.setKeyVersion(...) and
SecretHostDefineMsg.setKeyVersion(...); use the actual value read into
keyVersionHolder[0] instead (replace the literal 0 with keyVersionHolder[0]) so
both the source get and destination define use the real key version found
earlier (ensure keyVersionHolder is in scope where used).
- Around line 3200-3318: getOrCreateKey can create a new key but the subsequent
flows ("get-source-secret-uuid"/"ensure-secret-on-dst") run in a NoRollbackFlow
chain, so if they fail and result.isCreatedNewKey() is true the created key and
EncryptedResourceKeyRef are left behind; call
resourceKeyManager.rollbackCreatedKey(...) to undo the creation on failure.
Concretely, inside the success(ResourceKeyResult result) callback (where
GetOrCreateResourceKeyContext is used and the FlowChain is started) capture the
result and in the chain.error handler and any other failure paths (including the
getOrCreateKey.fail callback) check result.isCreatedNewKey() and invoke
resourceKeyManager.rollbackCreatedKey(result) before propagating the original
error to trigger.fail; ensure rollback completes (or is attempted) then
rethrow/forward the original ErrorCode to maintain semantics.
- Around line 5525-5608: The code validates msg.getUsageInstance() but then
ignores it and hardcodes KVMConstant.HOST_SECRET_USAGE_INSTANCE_VTPM; also the
initial input-check block calls reply.setError(...) twice which overwrites the
intended message. Fix by removing the duplicate reply.setError(...) in the
validation branch (keep a single, clear error about vmUuid, purpose, keyVersion
and usageInstance) and set the agent command's usage instance from the request
(replace cmd.setUsageInstance(KVMConstant.HOST_SECRET_USAGE_INSTANCE_VTPM) with
cmd.setUsageInstance(msg.getUsageInstance())). Ensure you reference
msg.getUsageInstance(), cmd.setUsageInstance(...), and the duplicate
reply.setError(...) to locate the changes.
---
Nitpick comments:
In `@header/src/main/java/org/zstack/header/secret/SecretHostGetMsg.java`:
- Around line 14-55: The new usageInstance field on SecretHostGetMsg is ignored
by the handler: update KVMHost.handle(SecretHostGetMsg) to read
msg.getUsageInstance() (falling back to
KVMConstant.HOST_SECRET_USAGE_INSTANCE_VTPM when null/empty) instead of
hardcoding KVMConstant.HOST_SECRET_USAGE_INSTANCE_VTPM, mirroring how
SecretHostEnsureMsg is validated/used; alternatively, if usageInstance is
intentionally unused, remove the field from SecretHostGetMsg or add Javadoc on
the field to state it is deprecated/unused.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f06b733a-9d3e-4ace-a729-dae799f26e4c
📒 Files selected for processing (7)
header/src/main/java/org/zstack/header/keyprovider/EncryptedResourceKeyManager.javaheader/src/main/java/org/zstack/header/secret/SecretHostDefineMsg.javaheader/src/main/java/org/zstack/header/secret/SecretHostDeleteMsg.javaheader/src/main/java/org/zstack/header/secret/SecretHostGetMsg.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMHost.javaplugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmExtensions.javarunMavenProfile
✅ Files skipped from review due to trivial changes (1)
- header/src/main/java/org/zstack/header/secret/SecretHostDefineMsg.java
🚧 Files skipped from review as they are similar to previous changes (1)
- runMavenProfile
There was a problem hiding this comment.
♻️ Duplicate comments (4)
header/src/main/java/org/zstack/header/keyprovider/EncryptedResourceKeyManager.java (1)
32-35:⚠️ Potential issue | 🟠 Major回滚契约退化：仅靠
createdNewKey会误删既有 Ref。Line [32]-Line [35] 当前语义会在
createdNewKey=true时无条件删除EncryptedResourceKeyRef，无法区分“本次流程新建 Ref”与“回滚前已存在占位 Ref”。这会把 TPM 既有绑定一并删掉，导致失败后状态漂移。建议恢复“Ref 是否预存”的判定（例如恢复refExistedBeforeCreate），仅在本次新建 Ref 时才删除 Ref。建议修复（示意）
@@ - * When {`@link` ResourceKeyResult#isCreatedNewKey()} is true, the implementation deletes the - * key-tool secret if one was materialized, then removes the {`@code` EncryptedResourceKeyRef} row - * for the resource (same storage effect as detaching the key provider from the resource). + * When {`@link` ResourceKeyResult#isCreatedNewKey()} is true, the implementation recycles + * materialized key/secret created by this flow. + * The {`@code` EncryptedResourceKeyRef} row should be removed only if it was also created + * by this flow (i.e. ref did not exist before create). @@ class ResourceKeyResult { @@ private boolean createdNewKey; + private boolean refExistedBeforeCreate; @@ public void setCreatedNewKey(boolean createdNewKey) { this.createdNewKey = createdNewKey; } + + public boolean isRefExistedBeforeCreate() { + return refExistedBeforeCreate; + } + + public void setRefExistedBeforeCreate(boolean refExistedBeforeCreate) { + this.refExistedBeforeCreate = refExistedBeforeCreate; + } }Based on learnings: In ZStack's
KvmTpmExtensions.preReleaseVmResource,result.setRefExistedBeforeCreate(true)is intentional, and rollback should not remove pre-existing placeholder refs.🤖 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/keyprovider/EncryptedResourceKeyManager.java` around lines 32 - 35, The rollback logic in EncryptedResourceKeyManager currently deletes the EncryptedResourceKeyRef whenever ResourceKeyResult.isCreatedNewKey() is true, which can remove pre-existing placeholder refs; change the condition to also check a flag indicating the ref did not exist before creation (e.g., use ResourceKeyResult.getRefExistedBeforeCreate() or refExistedBeforeCreate) so that the code only deletes the EncryptedResourceKeyRef when the ref was created in this flow; locate the removal logic near ResourceKeyResult/EncryptedResourceKeyRef handling (and where createdNewKey is inspected) and update it to require createdNewKey == true AND refExistedBeforeCreate == false (mirror how KvmTpmExtensions.preReleaseVmResource sets result.setRefExistedBeforeCreate(true)).plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java (3)
3229-3248:⚠️ Potential issue | 🔴 Critical不要把
keyVersion继续写死成0。这里前面已经查到了真实的
keyVersion，但源端get secret和目标端define secret仍然都固定传0。只要 TPM 密钥轮转过，这两个调用就会命中错误版本，迁移前置准备会直接失效。🛠️ 建议修改
- getCmd.setKeyVersion(0); + getCmd.setKeyVersion(keyVersionHolder[0]); ... - innerMsg.setKeyVersion(0); + innerMsg.setKeyVersion(keyVersionHolder[0]);Also applies to: 3280-3285
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java` around lines 3229 - 3248, The code currently hardcodes keyVersion as 0 when calling KVMAgentCommands.SecretHostGetCmd (getCmd.setKeyVersion(0)) and the corresponding secret define call later, but you already retrieved the real version into keyVersionHolder[0]; update both places to pass keyVersionHolder[0] (i.e., use the retrieved keyVersion) instead of 0 so the source get and target define use the correct key version (check the spots around KVMAgentCommands.SecretHostGetCmd and the SecretHostDefine/SecretDefineCmd usage near lines ~3229 and ~3280).
5525-5527:⚠️ Potential issue | 🟠 Major
usageInstance校验了，但又被下发时写死覆盖了。现在
usageInstance已经是必填字段，但这里仍然固定下发HOST_SECRET_USAGE_INSTANCE_VTPM。同时 Line 5527 上第二个reply.setError(...)还会把包含usageInstance的新报错覆盖掉，导致字段实际上不生效、报错也不准确。🛠️ 建议修改
- reply.setError(operr("vmUuid, purpose, keyVersion and usageInstance are required for ensure secret")); reply.setError(operr("vmUuid, purpose and keyVersion are required for ensure secret")); + reply.setError(operr("vmUuid, purpose, keyVersion and usageInstance are required for ensure secret")); ... - cmd.setUsageInstance(KVMConstant.HOST_SECRET_USAGE_INSTANCE_VTPM); + cmd.setUsageInstance(msg.getUsageInstance());Also applies to: 5608-5608
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java` around lines 5525 - 5527, The code in KVMHost validates msg.getUsageInstance() but then overwrites it with the constant HOST_SECRET_USAGE_INSTANCE_VTPM and also sets reply.setError twice (the second call at the same spot overwrites the first), so usageInstance is effectively ignored and the error message is wrong; change the logic that currently forces HOST_SECRET_USAGE_INSTANCE_VTPM to instead use msg.getUsageInstance() when dispatching the secret, and remove or consolidate the duplicate reply.setError(...) so the error message correctly reflects missing vmUuid, purpose, keyVersion and usageInstance; apply the same fix for the analogous block referenced around the other occurrence (5608).
3200-3206:⚠️ Potential issue | 🟠 Major给
getOrCreateKey()新建出来的 key 补上失败回滚。这一段仍然放在
NoRollbackFlow里。getOrCreateKey()一旦新建了 key，后续get-source-secret-uuid或ensure-secret-on-dst任一步失败，都会把刚创建的 key/ref 留在系统里。🧹 建议方向
- flow(new NoRollbackFlow() { + flow(new Flow() { + ResourceKeyResult createdKey; String __name__ = "ensure-secret-on-dst-before-migrate"; `@Override` public void run(FlowTrigger trigger, Map data) { ... resourceKeyManager.getOrCreateKey(keyCtx, new ReturnValueCompletion<ResourceKeyResult>(trigger) { `@Override` public void success(ResourceKeyResult result) { + createdKey = result; ... } }); } + + `@Override` + public void rollback(FlowRollback trigger, Map data) { + if (createdKey != null && createdKey.isCreatedNewKey()) { + resourceKeyManager.rollbackCreatedKey(createdKey, new Completion(trigger) { + `@Override` + public void success() { + trigger.rollback(); + } + + `@Override` + public void fail(ErrorCode errorCode) { + trigger.rollback(); + } + }); + } else { + trigger.rollback(); + } + } });Also applies to: 3301-3317
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java` around lines 3200 - 3206, The call to resourceKeyManager.getOrCreateKey(...) inside NoRollbackFlow can create a new key that is never cleaned up if later steps (get-source-secret-uuid or ensure-secret-on-dst) fail; update the ReturnValueCompletion<ResourceKeyResult> handler in the getOrCreateKey invocations (the one using GetOrCreateResourceKeyContext and resourceKeyManager.getOrCreateKey) to record whether a new key/ref was created (from ResourceKeyResult), and on any downstream failure invoke the appropriate cleanup API (e.g. resourceKeyManager.deleteKey or remove the created ref using the returned key UUID) before signaling failure; apply the same fix to the other occurrence around lines 3301-3317 so both getOrCreateKey usages undo newly created keys when subsequent steps fail.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@header/src/main/java/org/zstack/header/keyprovider/EncryptedResourceKeyManager.java`:
- Around line 32-35: The rollback logic in EncryptedResourceKeyManager currently
deletes the EncryptedResourceKeyRef whenever ResourceKeyResult.isCreatedNewKey()
is true, which can remove pre-existing placeholder refs; change the condition to
also check a flag indicating the ref did not exist before creation (e.g., use
ResourceKeyResult.getRefExistedBeforeCreate() or refExistedBeforeCreate) so that
the code only deletes the EncryptedResourceKeyRef when the ref was created in
this flow; locate the removal logic near
ResourceKeyResult/EncryptedResourceKeyRef handling (and where createdNewKey is
inspected) and update it to require createdNewKey == true AND
refExistedBeforeCreate == false (mirror how
KvmTpmExtensions.preReleaseVmResource sets
result.setRefExistedBeforeCreate(true)).
In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java`:
- Around line 3229-3248: The code currently hardcodes keyVersion as 0 when
calling KVMAgentCommands.SecretHostGetCmd (getCmd.setKeyVersion(0)) and the
corresponding secret define call later, but you already retrieved the real
version into keyVersionHolder[0]; update both places to pass keyVersionHolder[0]
(i.e., use the retrieved keyVersion) instead of 0 so the source get and target
define use the correct key version (check the spots around
KVMAgentCommands.SecretHostGetCmd and the SecretHostDefine/SecretDefineCmd usage
near lines ~3229 and ~3280).
- Around line 5525-5527: The code in KVMHost validates msg.getUsageInstance()
but then overwrites it with the constant HOST_SECRET_USAGE_INSTANCE_VTPM and
also sets reply.setError twice (the second call at the same spot overwrites the
first), so usageInstance is effectively ignored and the error message is wrong;
change the logic that currently forces HOST_SECRET_USAGE_INSTANCE_VTPM to
instead use msg.getUsageInstance() when dispatching the secret, and remove or
consolidate the duplicate reply.setError(...) so the error message correctly
reflects missing vmUuid, purpose, keyVersion and usageInstance; apply the same
fix for the analogous block referenced around the other occurrence (5608).
- Around line 3200-3206: The call to resourceKeyManager.getOrCreateKey(...)
inside NoRollbackFlow can create a new key that is never cleaned up if later
steps (get-source-secret-uuid or ensure-secret-on-dst) fail; update the
ReturnValueCompletion<ResourceKeyResult> handler in the getOrCreateKey
invocations (the one using GetOrCreateResourceKeyContext and
resourceKeyManager.getOrCreateKey) to record whether a new key/ref was created
(from ResourceKeyResult), and on any downstream failure invoke the appropriate
cleanup API (e.g. resourceKeyManager.deleteKey or remove the created ref using
the returned key UUID) before signaling failure; apply the same fix to the other
occurrence around lines 3301-3317 so both getOrCreateKey usages undo newly
created keys when subsequent steps fail.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b615ab43-0f68-4882-b7d2-da11d64470d5
📒 Files selected for processing (7)
header/src/main/java/org/zstack/header/keyprovider/EncryptedResourceKeyManager.javaheader/src/main/java/org/zstack/header/secret/SecretHostDefineMsg.javaheader/src/main/java/org/zstack/header/secret/SecretHostDeleteMsg.javaheader/src/main/java/org/zstack/header/secret/SecretHostGetMsg.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMHost.javaplugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmExtensions.javarunMavenProfile
✅ Files skipped from review due to trivial changes (1)
- header/src/main/java/org/zstack/header/secret/SecretHostGetMsg.java
🚧 Files skipped from review as they are similar to previous changes (3)
- runMavenProfile
- header/src/main/java/org/zstack/header/secret/SecretHostDeleteMsg.java
- plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmExtensions.java
MatheMatrix
force-pushed
the
sync/zstackio/define-secret-mig
branch
12 times, most recently
from
691377b to
9e3049e
Compare
zstack-robot-2
commented
Apr 12, 2026
| keyCtx.setKeyProviderUuid(providerUuid); | ||
| keyCtx.setKeyProviderName(providerName); | ||
| keyCtx.setPurpose("vtpm"); | ||
| resourceKeyManager.getOrCreateKey(keyCtx, new ReturnValueCompletion<ResourceKeyResult>(trigger) { |
Collaborator
Author
There was a problem hiding this comment.
Comment from 周众:
过时了，重新review
MatheMatrix
reviewed
Apr 12, 2026
| reply.setError(operr("vmUuid, purpose and keyVersion are required for ensure secret")); | ||
| if (StringUtils.isBlank(msg.getVmUuid()) || StringUtils.isBlank(msg.getPurpose()) | ||
| || msg.getKeyVersion() == null || StringUtils.isBlank(msg.getUsageInstance())) { | ||
| reply.setError(operr("vmUuid, purpose, keyVersion and usageInstance are required for ensure secret")); reply.setError(operr("vmUuid, purpose and keyVersion are required for ensure secret")); |
MatheMatrix
force-pushed
the
sync/zstackio/define-secret-mig
branch
from
9e3049e to
786ef49
Compare
Collaborator
|
Comment from yaohua.wu: Review: MR !9579 — ZSV-11729问题背景Jira ZSV-11729: 带 vTPM 的虚机在热迁移时报错 修复方案概述本 MR 在迁移流程中新增了 4 个 flow（
方案正确解决了根因，整体架构清晰。 详细审查🟡 Warning: 迁移流程中获取源 secret UUID 使用 REST 直调 agent，与其他 flow 不一致文件: restf.asyncJsonPost(buildUrl(KVMConstant.KVM_GET_SECRET_PATH), getCmd, ...)此处通过
虽然在源主机上直调 agent 功能上可行（因为 🟡 Warning: 所有迁移 flow 均为
|
Resolves: ZSV-11729 Change-Id: I616a776e78666179637976616c776162626c6533
MatheMatrix
force-pushed
the
sync/zstackio/define-secret-mig
branch
from
786ef49 to
a4f9b7a
Compare
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
4 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.
Resolves: ZSV-11729
Change-Id: I616a776e78666179637976616c776162626c6533
sync from gitlab !9579