<fix>[kvm]: define secret on migrate destination#3727
<fix>[kvm]: define secret on migrate destination#3727ZStack-Robot 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:
走查概览该PR引入了新的资源密钥获取API、主机密钥消息字段扩展，以及vTPM迁移前处理机制。主要包括移除冗余状态跟踪、添加用法实例标记，以及重构vTPM密钥和密钥管理流程。 更改内容
序列图sequenceDiagram
participant VM as vTPM VM
participant Src as 源KVM主机
participant KMS as 密钥管理器
participant Secret as 密钥提供程序
participant Dst as 目标KVM主机
VM->>Src: 触发迁移前准备
Src->>Src: 解析TPM/提供程序/密钥版本
Src->>KMS: getKey(resourceKeyResult)
KMS-->>Src: 返回DEK base64 & 密钥版本
Src->>Src: 保存resourceKeyResult
Src->>Secret: SecretHostGetMsg(usageInstance=VTPM)
Secret-->>Src: 获取源主机密钥
Src->>Dst: SecretHostDefineMsg(secretUuid, usageInstance=VTPM)
Dst-->>Src: 在目标主机定义密钥
Src->>Src: 迁移前准备完成
代码审查工作量评估🎯 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 unit tests (beta)
Comment |
|
Comment from yaohua.wu: Review: MR !9596 — ZSV-11729
问题理解带 vTPM 的虚机在热迁移（或带存储迁移）时，目标主机上没有预先定义相同 UUID 的 libvirt secret，导致 swtpm 在目标侧启动时报 方案评价修复方案在
思路正确，将 secret 的"物化"操作移到迁移前完成，保证 libvirt 在目标侧能找到匹配的 secret。同时新增 具体发现🟡 Warning — Agent 侧是否已支持
|
| 级别 | 数量 |
|---|---|
| 🔴 Critical | 0 |
| 🟡 Warning | 4 |
| 🟢 Suggestion | 3 |
整体方案思路正确，通过在迁移前预定义 secret 解决了目标主机找不到 secret 的问题。最需要关注的是：Agent 侧是否已支持 secretUuid 字段，这是修复能否生效的关键。其次是 usageInstance 必填校验的向后兼容性，以及 skip 条件从精确判断改为全局配置开关带来的语义变化。
🤖 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: 3
🧹 Nitpick comments (1)
plugin/kvm/src/main/java/org/zstack/kvm/tpm/VtpmPreMigrateSecretCtx.java (1)
11-21: 可选改进：考虑添加toString()方法便于调试。在迁移流程出现问题时，
toString()方法可以帮助快速了解上下文状态。💡 可选的 toString() 实现
private String sourceSecretUuid; + + `@Override` + public String toString() { + return "VtpmPreMigrateSecretCtx{" + + "skipAll=" + skipAll + + ", tpmUuid='" + tpmUuid + '\'' + + ", providerUuid='" + providerUuid + '\'' + + ", keyVersion=" + keyVersion + + ", sourceSecretUuid='" + sourceSecretUuid + '\'' + + '}'; + }🤖 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/VtpmPreMigrateSecretCtx.java` around lines 11 - 21, Add a concise toString() implementation to VtpmPreMigrateSecretCtx that returns a string containing the class name and the key fields (skipAll, tpmUuid, providerUuid, providerName, keyVersion, resourceKeyResult, sourceSecretUuid) so debugging logs show context state; implement it as an override of toString() inside the VtpmPreMigrateSecretCtx class and format the output clearly (e.g., "VtpmPreMigrateSecretCtx{field=value,...}") without adding new dependencies.
🤖 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 3294-3331: The NoRollbackFlow
"vtpm-define-secret-on-dst-before-migrate" (inside KVMHost flow) currently
defines a secret on the destination via SecretHostDefineMsg but does not record
that it succeeded nor provide cleanup on failure; update the flow to set a flag
in the shared data map (e.g., data.put("vtpm.dstSecretDefined", true) or store
the dst secret uuid in VtpmPreMigrateSecretCtx) immediately after a successful
SecretHostDefineMsg reply, and add a compensating rollback/cleanup flow (or a
finally-style NoRollbackFlow executed on failure) that checks that flag and
sends a best-effort SecretHostDeleteMsg targeting dstHostUuid (using
ctx.getSourceSecretUuid() or stored secret id) to remove the materialized
secret; ensure the cleanup only logs/aggregates errors and does not replace the
original migration error returned to the caller.
- Around line 3197-3201: The current logic in KVMHost
(VmGlobalConfig.ALLOWED_TPM_VM_WITHOUT_KMS + ctx.setSkipAll(true) +
trigger.next()) unconditionally skips vTPM secret provisioning even for VMs that
already have a KMS/provider bound; change it so the skip only happens when the
VM's TPM has no provider binding (i.e., check the parsed provider/secret info
first) or move this ALLOWED_TPM_VM_WITHOUT_KMS check to after provider
resolution/parse failure; specifically, update the code around the provider
parsing logic in KVMHost to evaluate VmGlobalConfig.ALLOWED_TPM_VM_WITHOUT_KMS
only when provider == null/unbound, and only then call ctx.setSkipAll(true) and
trigger.next().
- Around line 5484-5486: Three places that construct SecretHostDeleteMsg in
KvmTpmExtensions and KvmTpmManager omit the now-required usageInstance, causing
runtime validation failures; update each construction to call
setUsageInstance(...) with the appropriate instance identifier (use the same VM
instance identifier already assigned to vmUuid where applicable), and mirror the
same fix for any SecretHostDefineMsg constructions; locate the builders that
currently set hostUuid, vmUuid, purpose, keyVersion and add setUsageInstance(<vm
instance id>) so the message passes the KVMHost validation.
---
Nitpick comments:
In `@plugin/kvm/src/main/java/org/zstack/kvm/tpm/VtpmPreMigrateSecretCtx.java`:
- Around line 11-21: Add a concise toString() implementation to
VtpmPreMigrateSecretCtx that returns a string containing the class name and the
key fields (skipAll, tpmUuid, providerUuid, providerName, keyVersion,
resourceKeyResult, sourceSecretUuid) so debugging logs show context state;
implement it as an override of toString() inside the VtpmPreMigrateSecretCtx
class and format the output clearly (e.g.,
"VtpmPreMigrateSecretCtx{field=value,...}") without adding new dependencies.
🪄 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: 0f3d65b3-5350-4838-967c-b2b2323b381b
📒 Files selected for processing (9)
compute/src/main/java/org/zstack/compute/vm/devices/DummyEncryptedResourceKeyManager.javaheader/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/KVMAgentCommands.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMHost.javaplugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmExtensions.javaplugin/kvm/src/main/java/org/zstack/kvm/tpm/VtpmPreMigrateSecretCtx.java
| if (VmGlobalConfig.ALLOWED_TPM_VM_WITHOUT_KMS.value(Boolean.class)) { | ||
| ctx.setSkipAll(true); | ||
| trigger.next(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
这里会把“允许无 KMS”误当成“永远不需要预置 secret”。
ALLOWED_TPM_VM_WITHOUT_KMS 为 true 时直接 skipAll，会把已经绑定 KMS/provider 的 vTPM VM 也一起跳过，导致目的端仍然不会定义 secret，混合场景下迁移会回到本 PR 修复前的失败路径。建议只在当前 TPM 确认没有 provider 绑定时才跳过，或者把这段判断放到 provider 解析失败之后。
🤖 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 3197 -
3201, The current logic in KVMHost (VmGlobalConfig.ALLOWED_TPM_VM_WITHOUT_KMS +
ctx.setSkipAll(true) + trigger.next()) unconditionally skips vTPM secret
provisioning even for VMs that already have a KMS/provider bound; change it so
the skip only happens when the VM's TPM has no provider binding (i.e., check the
parsed provider/secret info first) or move this ALLOWED_TPM_VM_WITHOUT_KMS check
to after provider resolution/parse failure; specifically, update the code around
the provider parsing logic in KVMHost to evaluate
VmGlobalConfig.ALLOWED_TPM_VM_WITHOUT_KMS only when provider == null/unbound,
and only then call ctx.setSkipAll(true) and trigger.next().
| flow(new NoRollbackFlow() { | ||
| String __name__ = "vtpm-define-secret-on-dst-before-migrate"; | ||
|
|
||
| @Override | ||
| public boolean skip(Map data) { | ||
| VtpmPreMigrateSecretCtx ctx = VtpmPreMigrateSecretCtx.from(data); | ||
| return ctx == null || ctx.isSkipAll(); | ||
| } | ||
|
|
||
| @Override | ||
| public void run(FlowTrigger trigger, Map data) { | ||
| VtpmPreMigrateSecretCtx ctx = VtpmPreMigrateSecretCtx.from(data); | ||
| ResourceKeyResult result = ctx.getResourceKeyResult(); | ||
| if (result == null || StringUtils.isBlank(result.getDekBase64())) { | ||
| trigger.fail(operr("missing DEK for tpm[uuid:%s] before ensure secret on destination", ctx.getTpmUuid())); | ||
| return; | ||
| } | ||
| SecretHostDefineMsg innerMsg = new SecretHostDefineMsg(); | ||
| innerMsg.setHostUuid(dstHostUuid); | ||
| innerMsg.setVmUuid(vmUuid); | ||
| innerMsg.setDekBase64(result.getDekBase64()); | ||
| innerMsg.setPurpose("vtpm"); | ||
| innerMsg.setKeyVersion(ctx.getKeyVersion()); | ||
| innerMsg.setUsageInstance(KVMConstant.HOST_SECRET_USAGE_INSTANCE_VTPM); | ||
| innerMsg.setSecretUuid(ctx.getSourceSecretUuid()); | ||
| innerMsg.setDescription(String.format("Define secret for VM %s before live migration", vmUuid)); | ||
| bus.makeTargetServiceIdByResourceUuid(innerMsg, HostConstant.SERVICE_ID, innerMsg.getHostUuid()); | ||
| bus.send(innerMsg, new CloudBusCallBack(trigger) { | ||
| @Override | ||
| public void run(MessageReply reply) { | ||
| if (reply.isSuccess()) { | ||
| trigger.next(); | ||
| } else { | ||
| trigger.fail(reply.getError()); | ||
| } | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
目的端 secret 定义成功后缺少失败补偿。
这一步成功后，后续任一 flow 失败都会直接结束整个迁移链路，但这里是 NoRollbackFlow，没有对应的 SecretHostDeleteMsg 清理。结果是迁移失败或重试后，目的端会残留 materialized secret 和脏状态。建议在 data 里记录 define 成功标记，并在链路失败时对 dstHostUuid 做 best-effort 删除，同时不要覆盖原始迁移错误。
🤖 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 3294 -
3331, The NoRollbackFlow "vtpm-define-secret-on-dst-before-migrate" (inside
KVMHost flow) currently defines a secret on the destination via
SecretHostDefineMsg but does not record that it succeeded nor provide cleanup on
failure; update the flow to set a flag in the shared data map (e.g.,
data.put("vtpm.dstSecretDefined", true) or store the dst secret uuid in
VtpmPreMigrateSecretCtx) immediately after a successful SecretHostDefineMsg
reply, and add a compensating rollback/cleanup flow (or a finally-style
NoRollbackFlow executed on failure) that checks that flag and sends a
best-effort SecretHostDeleteMsg targeting dstHostUuid (using
ctx.getSourceSecretUuid() or stored secret id) to remove the materialized
secret; ensure the cleanup only logs/aggregates errors and does not replace the
original migration error returned to the caller.
MatheMatrix
force-pushed
the
sync/zstackio/define-secret-mig@@2
branch
from
a4f9b7a to
93acfa0
Compare
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)
plugin/kvm/src/main/java/org/zstack/kvm/KVMAgentCommands.java (1)
415-472:⚠️ Potential issue | 🟠 Major确认 secretUuid 在 agent 端是否被实际应用到 libvirt secret 定义。
Java 管理侧已正确扩展和传递
secretUuid（KVMHost.java:5618），SecretHostDefineCmd和SecretHostDefineResponseDTO 也已完整定义此字段。然而，本仓库中未找到 agent 端的实现代码。agent 侧的 secret 定义逻辑（通常在单独部署的 KVM agent 模块中）需要确认是否真正读取并应用了secretUuid参数到 libvirt secret XML 定义中，否则迁移目标端生成的 secret UUID 仍会与源端不符。🤖 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/KVMAgentCommands.java` around lines 415 - 472, 确认 agent 端是否把传入的 secretUuid 应用到 libvirt secret 定义：检查 agent 中接收 SecretHostDefineCmd/SecretHostDefineResponse 的请求处理代码（agent 的 secret 定义/注册函数，例如用于生成 libvirt secret XML 的方法），确保它从 DTO 读取 secretUuid 字段并在生成的 libvirt secret XML 中设置 <uuid> 元素（而非让 libvirt 生成新 UUID）；如果当前未使用 secretUuid，则修改该处理函数以使用传入的 secretUuid，调用 libvirt API 时传入该 UUID，并加上单元/集成测试或运行时校验以确认创建后的 libvirt secret UUID 与传入的 secretUuid 一致。
🤖 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/tpm/KvmTpmExtensions.java`:
- Around line 266-267: The global flag check using
VmGlobalConfig.ALLOWED_TPM_VM_WITHOUT_KMS.value(Boolean.class) currently lets
code paths skip secret lookup/DEK/secret definition even when a vTPM already has
KMS binding; change those conditions in KvmTpmExtensions so they only skip when
both the global flag is true AND the TPM binding is truly "no KMS" (i.e. the
vTPM binding object has no providerUuid, no keyVersion and no secretUuid).
Update the three places that currently only check ALLOWED_TPM_VM_WITHOUT_KMS
(the checks around the secret query, DEK retrieval, and host secret definition)
to require both the global flag and an absent providerUuid/keyVersion/secretUuid
on the tpm binding before bypassing secret logic.
---
Outside diff comments:
In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMAgentCommands.java`:
- Around line 415-472: 确认 agent 端是否把传入的 secretUuid 应用到 libvirt secret 定义：检查
agent 中接收 SecretHostDefineCmd/SecretHostDefineResponse 的请求处理代码（agent 的 secret
定义/注册函数，例如用于生成 libvirt secret XML 的方法），确保它从 DTO 读取 secretUuid 字段并在生成的 libvirt
secret XML 中设置 <uuid> 元素（而非让 libvirt 生成新 UUID）；如果当前未使用 secretUuid，则修改该处理函数以使用传入的
secretUuid，调用 libvirt API 时传入该 UUID，并加上单元/集成测试或运行时校验以确认创建后的 libvirt secret UUID
与传入的 secretUuid 一致。
🪄 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: d80e533e-b2c8-4543-945d-b4b947f64aa2
📒 Files selected for processing (10)
compute/src/main/java/org/zstack/compute/vm/devices/DummyEncryptedResourceKeyManager.javaheader/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/KVMAgentCommands.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMHost.javaplugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmExtensions.javaplugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.javaplugin/kvm/src/main/java/org/zstack/kvm/tpm/VtpmPreMigrateSecretCtx.java
✅ Files skipped from review due to trivial changes (4)
- header/src/main/java/org/zstack/header/secret/SecretHostGetMsg.java
- header/src/main/java/org/zstack/header/secret/SecretHostDeleteMsg.java
- header/src/main/java/org/zstack/header/secret/SecretHostDefineMsg.java
- plugin/kvm/src/main/java/org/zstack/kvm/tpm/VtpmPreMigrateSecretCtx.java
🚧 Files skipped from review as they are similar to previous changes (3)
- compute/src/main/java/org/zstack/compute/vm/devices/DummyEncryptedResourceKeyManager.java
- header/src/main/java/org/zstack/header/keyprovider/EncryptedResourceKeyManager.java
- plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java
| return VmGlobalConfig.ALLOWED_TPM_VM_WITHOUT_KMS.value(Boolean.class); | ||
| } |
There was a problem hiding this comment.
不要让全局开关把已有 KMS 绑定的 vTPM 路径也一起跳过。
Line 266、Line 307 和 Line 343 现在只看 ALLOWED_TPM_VM_WITHOUT_KMS。这样一来，只要环境打开了“允许无 KMS”，即使某个 TPM 已经有 providerUuid/keyVersion/secretUuid，这里也会直接跳过 secret 查询、DEK 获取和目标 host 上的 secret 定义。后面的 beforeStartVmOnKvm() 仍然会把 secretUuid 传给 agent；如果该 host 上并没有对应 secret，这条启动/恢复链路就会直接退化成 Secret not found。更稳妥的条件应该是“仅在真正的无 KMS 场景下放宽”，而不是让全局开关覆盖掉已有 KMS 绑定的 VM。
Also applies to: 306-308, 342-344
🤖 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 266 - 267, The global flag check using
VmGlobalConfig.ALLOWED_TPM_VM_WITHOUT_KMS.value(Boolean.class) currently lets
code paths skip secret lookup/DEK/secret definition even when a vTPM already has
KMS binding; change those conditions in KvmTpmExtensions so they only skip when
both the global flag is true AND the TPM binding is truly "no KMS" (i.e. the
vTPM binding object has no providerUuid, no keyVersion and no secretUuid).
Update the three places that currently only check ALLOWED_TPM_VM_WITHOUT_KMS
(the checks around the secret query, DEK retrieval, and host secret definition)
to require both the global flag and an absent providerUuid/keyVersion/secretUuid
on the tpm binding before bypassing secret logic.
93acfa0 to
aca3c65
Compare
There was a problem hiding this comment.
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/KVMHost.java (1)
5633-5639:⚠️ Potential issue | 🟠 Major校验响应中的 secretUuid 与请求值是否一致。
在迁移场景中，源端获取的 secret UUID 被显式地传入
SecretHostDefineMsg.setSecretUuid(ctx.getSourceSecretUuid())（KVMHost.java:3318），但响应处理器（5635-5639）仅检查rsp.getSecretUuid() != null，不验证与请求值的一致性。Agent 模拟器实现（KVMSimulator.groovy:356-359）显示，缓存键仅由
(hostUuid, vmUuid, purpose, keyVersion, usageInstance)构成，不包含 secretUuid。这意味着即使请求中指定了 secretUuid，agent 也会忽略该字段并返回基于缓存的 UUID。若缓存已存在（例如来自不同的请求），会返回不匹配的 UUID。在迁移场景中，这会导致源端和目的端的 secret 不一致，后续在 libvirt/swtpm 阶段出现失败。建议：
- 若请求中指定了
msg.getSecretUuid()，响应成功时应验证rsp.getSecretUuid()与其一致；若不一致则返回错误。- 同时确认 agent 端 define-secret 确实消费了 secretUuid 字段（基于现有证据，agent 模拟器未使用该字段）。
🤖 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 5633 - 5639, The success handler for SecretHostDefine responses must validate that when the request specified a secret UUID the agent returned the same one; update the success(KVMAgentCommands.SecretHostDefineResponse rsp) logic to, if msg.getSecretUuid() is non-null, compare rsp.getSecretUuid() to msg.getSecretUuid() and treat a mismatch as an error by calling buildSecretAgentError (instead of accepting any non-null rsp.getSecretUuid()); locate this in KVMHost.success handling for SecretHostDefineResponse and also add a note to verify the agent-side define-secret consumer in KVMSimulator/KVM agent to ensure it honors msg.getSecretUuid().
♻️ Duplicate comments (2)
plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java (2)
3197-3206:⚠️ Potential issue | 🟠 Major不要在全局开关为
true时无条件跳过 vTPM secret 预置。这里会把“允许无 KMS 的 TPM VM”扩大成“所有 TPM VM 都跳过”，已绑定 provider 的场景也会被短路，目的端仍然不会定义 secret，迁移会退回到本次修复前的失败路径。建议先解析 provider，再只对“确实未绑定 provider”的 TPM VM 做 skip。
💡 建议修改
- if (VmGlobalConfig.ALLOWED_TPM_VM_WITHOUT_KMS.value(Boolean.class)) { - ctx.setSkipAll(true); - trigger.next(); - return; - } ctx.setTpmUuid(tpmUuid); ctx.setProviderUuid(tpmKeyBackend.findKeyProviderUuidByTpm(tpmUuid)); ctx.setProviderName(tpmKeyBackend.findKeyProviderNameByTpm(tpmUuid)); if (StringUtils.isBlank(ctx.getProviderUuid()) && StringUtils.isBlank(ctx.getProviderName())) { + if (VmGlobalConfig.ALLOWED_TPM_VM_WITHOUT_KMS.value(Boolean.class)) { + ctx.setSkipAll(true); + trigger.next(); + return; + } trigger.fail(operr("missing TPM resource key binding for tpm[uuid:%s] before migrate", tpmUuid)); return; }🤖 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 3197 - 3206, The current code checks VmGlobalConfig.ALLOWED_TPM_VM_WITHOUT_KMS and calls ctx.setSkipAll/trigger.next before resolving the TPM's provider, which causes vTPM secret pre-provisioning to be skipped for all TPM VMs; instead, first set ctx.setTpmUuid and resolve provider via tpmKeyBackend.findKeyProviderUuidByTpm and findKeyProviderNameByTpm (and set ctx.setProviderUuid and ctx.setProviderName), then only if both providerUuid and providerName are blank AND VmGlobalConfig.ALLOWED_TPM_VM_WITHOUT_KMS.value(Boolean.class) is true call ctx.setSkipAll and trigger.next; otherwise proceed to the existing trigger.fail(operr(...)) path when provider is missing. Ensure you update the branch ordering around VmGlobalConfig.ALLOWED_TPM_VM_WITHOUT_KMS, ctx.setTpmUuid, ctx.setProviderUuid, ctx.setProviderName, ctx.setSkipAll, trigger.next, and trigger.fail accordingly.
3294-3331:⚠️ Potential issue | 🟠 Major定义目的端 secret 后缺少失败补偿。
这一步成功后，后续任一 flow 失败都会直接结束链路，但这里仍是
NoRollbackFlow，迁移失败或重试会在目的端残留 secret。请记录 define 成功状态，并在链路失败时对dstHostUuid做 best-effortSecretHostDeleteMsg清理，且不要覆盖原始迁移错误。🤖 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 3294 - 3331, Record that the secret was successfully defined on destination by setting a flag on VtpmPreMigrateSecretCtx (e.g. ctx.setDstDefined(true)) immediately after a successful SecretHostDefineMsg reply inside the NoRollbackFlow; then add a best-effort cleanup in the flow-chain failure path that checks ctx.isDstDefined() and, without replacing the original migration error, sends a SecretHostDeleteMsg to dstHostUuid (usageInstance KVMConstant.HOST_SECRET_USAGE_INSTANCE_VTPM, secretUuid ctx.getSourceSecretUuid()) and ignores/logs any delete failures. Ensure the cleanup logic uses the same context (VtpmPreMigrateSecretCtx) and does not swallow or overwrite the original FlowTrigger error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java`:
- Around line 5633-5639: The success handler for SecretHostDefine responses must
validate that when the request specified a secret UUID the agent returned the
same one; update the success(KVMAgentCommands.SecretHostDefineResponse rsp)
logic to, if msg.getSecretUuid() is non-null, compare rsp.getSecretUuid() to
msg.getSecretUuid() and treat a mismatch as an error by calling
buildSecretAgentError (instead of accepting any non-null rsp.getSecretUuid());
locate this in KVMHost.success handling for SecretHostDefineResponse and also
add a note to verify the agent-side define-secret consumer in KVMSimulator/KVM
agent to ensure it honors msg.getSecretUuid().
---
Duplicate comments:
In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java`:
- Around line 3197-3206: The current code checks
VmGlobalConfig.ALLOWED_TPM_VM_WITHOUT_KMS and calls ctx.setSkipAll/trigger.next
before resolving the TPM's provider, which causes vTPM secret pre-provisioning
to be skipped for all TPM VMs; instead, first set ctx.setTpmUuid and resolve
provider via tpmKeyBackend.findKeyProviderUuidByTpm and findKeyProviderNameByTpm
(and set ctx.setProviderUuid and ctx.setProviderName), then only if both
providerUuid and providerName are blank AND
VmGlobalConfig.ALLOWED_TPM_VM_WITHOUT_KMS.value(Boolean.class) is true call
ctx.setSkipAll and trigger.next; otherwise proceed to the existing
trigger.fail(operr(...)) path when provider is missing. Ensure you update the
branch ordering around VmGlobalConfig.ALLOWED_TPM_VM_WITHOUT_KMS,
ctx.setTpmUuid, ctx.setProviderUuid, ctx.setProviderName, ctx.setSkipAll,
trigger.next, and trigger.fail accordingly.
- Around line 3294-3331: Record that the secret was successfully defined on
destination by setting a flag on VtpmPreMigrateSecretCtx (e.g.
ctx.setDstDefined(true)) immediately after a successful SecretHostDefineMsg
reply inside the NoRollbackFlow; then add a best-effort cleanup in the
flow-chain failure path that checks ctx.isDstDefined() and, without replacing
the original migration error, sends a SecretHostDeleteMsg to dstHostUuid
(usageInstance KVMConstant.HOST_SECRET_USAGE_INSTANCE_VTPM, secretUuid
ctx.getSourceSecretUuid()) and ignores/logs any delete failures. Ensure the
cleanup logic uses the same context (VtpmPreMigrateSecretCtx) and does not
swallow or overwrite the original FlowTrigger error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: edb2405d-2eb0-44b1-a665-9ee8ff2dfe72
📒 Files selected for processing (10)
compute/src/main/java/org/zstack/compute/vm/devices/DummyEncryptedResourceKeyManager.javaheader/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/KVMAgentCommands.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMHost.javaplugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmExtensions.javaplugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.javaplugin/kvm/src/main/java/org/zstack/kvm/tpm/VtpmPreMigrateSecretCtx.java
✅ Files skipped from review due to trivial changes (3)
- header/src/main/java/org/zstack/header/secret/SecretHostDeleteMsg.java
- header/src/main/java/org/zstack/header/secret/SecretHostDefineMsg.java
- plugin/kvm/src/main/java/org/zstack/kvm/tpm/VtpmPreMigrateSecretCtx.java
🚧 Files skipped from review as they are similar to previous changes (3)
- header/src/main/java/org/zstack/header/secret/SecretHostGetMsg.java
- compute/src/main/java/org/zstack/compute/vm/devices/DummyEncryptedResourceKeyManager.java
- plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmExtensions.java
MatheMatrix
force-pushed
the
sync/zstackio/define-secret-mig@@2
branch
from
aca3c65 to
9b984bf
Compare
| } | ||
|
|
||
| @Override | ||
| public void getKey(GetOrCreateResourceKeyContext ctx, |
There was a problem hiding this comment.
Comment from wenhao.zhang:
确认一下这个行为。
DummyXXX 的存在本意是，让跑 case 时就算没有对应的非 dummy 的实现类，也能正常跑完整个 case。
你直接报错是否会和这个本意冲突？如果没有 crypto 模块的 case 加了 TPM 会在这里报错吗？
|
|
||
| if (context.keyVersion != null) { | ||
| // If VM is first created, skip this flow. | ||
| if (context.keyVersion == null || StringUtils.isBlank(spec.getDevicesSpec().getTpm().getSecretUuid())) { |
There was a problem hiding this comment.
Comment from wenhao.zhang:
和以前一样，建议就可以看情况不修改。
一般推荐是，content 传递是否是 first created，然后在前面检查，first created 和 tpm == null/keyVersion == null 同时成立才跳过。
| } | ||
| }); | ||
|
|
||
| flow(new NoRollbackFlow() { |
There was a problem hiding this comment.
Comment from wenhao.zhang:
不能放 migrateVm 的 extension？放这里是没有对应 extension 才这样写的
MatheMatrix
force-pushed
the
sync/zstackio/define-secret-mig@@2
branch
8 times, most recently
from
9cf8637 to
345be8c
Compare
| public ResourceKeyResult getKey(GetOrCreateResourceKeyContext ctx) { | ||
| logger.warn(String.format("crypto module not installed, cannot get resource key for %s[uuid:%s]", | ||
| ctx.getResourceType(), ctx.getResourceUuid())); | ||
| completion.fail(operr("crypto module is not installed, cannot manage resource encryption keys")); |
There was a problem hiding this comment.
Comment from wenhao.zhang:
completion 不存在
also: - cleanup EncryptedResourceKeyRefVO when VM creation failed Resolves: ZSV-11729 Resolves: ZSV-11845 Change-Id: I616a776e78666179637976616c776162626c6533
MatheMatrix
force-pushed
the
sync/zstackio/define-secret-mig@@2
branch
from
345be8c to
8d32ee7
Compare
4 participants
Resolves: ZSV-11729
Change-Id: I616a776e78666179637976616c776162626c6533
sync from gitlab !9596