<fix>[crypto]: clean TPM key ref on VM destroy#3679
Open
zstack-robot-1 wants to merge 4 commits intofeature-zsv-5.0.0-vm-support-vtpm-and-secucebootfrom
Open
<fix>[crypto]: clean TPM key ref on VM destroy#3679zstack-robot-1 wants to merge 4 commits intofeature-zsv-5.0.0-vm-support-vtpm-and-secucebootfrom
zstack-robot-1 wants to merge 4 commits intofeature-zsv-5.0.0-vm-support-vtpm-and-secucebootfrom
Conversation
Walkthrough新增 Changes
Sequence Diagram(s)sequenceDiagram
participant VM as "VM (deletion trigger)"
participant VmExt as "VmTpmExtensions"
participant VmMgr as "VmTpmManager"
participant KeyMgr as "EncryptedResourceKeyManager"
participant KMS as "Key Provider / Backend"
rect rgba(200,220,255,0.5)
VM->>VmExt: vmJustBeforeDeleteFromDb(vmUuid)
VmExt->>VmMgr: deleteResourceKeyRefForVmTpmIfPresent(vmUuid)
VmMgr->>VmMgr: 查询 TpmVO 获取 tpmUuid
VmMgr->>KeyMgr: deleteRef("tpm", tpmUuid)
KeyMgr->>KMS: 移除或忽略持久化的引用
end
sequenceDiagram
participant KvmExt as "KvmTpmExtensions (DEK 流)"
participant KeyCtx as "GetOrCreateResourceKeyContext"
participant KeyMgr as "EncryptedResourceKeyManager"
participant Result as "ResourceKeyResult"
rect rgba(200,255,200,0.5)
KvmExt->>KeyCtx: 构建 context（设置 providerUuid/providerName/可能的 keyProviderName）
alt provider 信息至少有一项
KvmExt->>KeyMgr: getOrCreateResourceKey(KeyCtx)
KeyMgr-->>Result: 返回 keyProviderUuid, keyProviderName, dek
KvmExt->>KvmExt: 更新 context.providerUuid / context.providerName / DEK
else providerUuid 与 providerName 均为空
KvmExt->>KvmExt: 跳过或失败该步骤
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
compute/src/main/java/org/zstack/compute/vm/devices/VmTpmExtensions.java (1)
161-169: 建议：考虑deleteRef异常处理
forEach循环中调用deleteRef时，如果某次调用抛出异常，后续的 TPM UUID 将不会被处理，可能留下孤立的引用记录。建议捕获并记录异常，确保所有引用都尝试清理：
♻️ 建议的修改
`@Override` public void afterDestroyVm(VmInstanceInventory inv) { List<String> tpmUuids = pendingDestroyTpmUuids.remove(inv.getUuid()); if (tpmUuids == null || tpmUuids.isEmpty()) { return; } - tpmUuids.forEach(tpmUuid -> resourceKeyManager.deleteRef(TpmVO.class.getSimpleName(), tpmUuid)); + for (String tpmUuid : tpmUuids) { + try { + resourceKeyManager.deleteRef(TpmVO.class.getSimpleName(), tpmUuid); + } catch (Exception e) { + logger.warn(String.format("failed to delete key ref for tpm[uuid:%s] during VM destroy, ignoring: %s", + tpmUuid, e.getMessage())); + } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@compute/src/main/java/org/zstack/compute/vm/devices/VmTpmExtensions.java` around lines 161 - 169, The current afterDestroyVm method reads pendingDestroyTpmUuids and calls resourceKeyManager.deleteRef for each tpmUuid using forEach, but if deleteRef throws on one UUID the rest won't be processed; modify afterDestroyVm to iterate over tpmUuids (from pendingDestroyTpmUuids.remove(inv.getUuid())) with a try/catch around each resourceKeyManager.deleteRef(TpmVO.class.getSimpleName(), tpmUuid) call, log any exceptions (including the tpmUuid and inv.getUuid() for context) and continue so all UUIDs are attempted to be deleted; ensure null/empty checks remain and use the same identifiers: afterDestroyVm, pendingDestroyTpmUuids, resourceKeyManager.deleteRef, and TpmVO.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@compute/src/main/java/org/zstack/compute/vm/devices/VmTpmExtensions.java`:
- Around line 161-169: The current afterDestroyVm method reads
pendingDestroyTpmUuids and calls resourceKeyManager.deleteRef for each tpmUuid
using forEach, but if deleteRef throws on one UUID the rest won't be processed;
modify afterDestroyVm to iterate over tpmUuids (from
pendingDestroyTpmUuids.remove(inv.getUuid())) with a try/catch around each
resourceKeyManager.deleteRef(TpmVO.class.getSimpleName(), tpmUuid) call, log any
exceptions (including the tpmUuid and inv.getUuid() for context) and continue so
all UUIDs are attempted to be deleted; ensure null/empty checks remain and use
the same identifiers: afterDestroyVm, pendingDestroyTpmUuids,
resourceKeyManager.deleteRef, and TpmVO.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f202b3d3-b56d-49b8-b67a-8ca62b1e4053
⛔ Files ignored due to path filters (1)
conf/springConfigXml/VmInstanceManager.xmlis excluded by!**/*.xml
📒 Files selected for processing (5)
compute/src/main/java/org/zstack/compute/vm/devices/DummyEncryptedResourceKeyManager.javacompute/src/main/java/org/zstack/compute/vm/devices/VmTpmExtensions.javaheader/src/main/java/org/zstack/header/keyprovider/EncryptedResourceKeyManager.javaplugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmExtensions.javaplugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.java
Delay/trash deletion no longer drops EncryptedResourceKeyRef; cleanup runs in vmJustBeforeDeleteFromDb (expunge/direct delete). Centralize ref delete in VmTpmManager; wire RemoveTpm/add-tpm rollback and drop unused destroy extension from VmTpmExtensions. Resolves: ZSV-11721 Change-Id: I68676e706f6a786773627074636f6e6a617a6e4f
MatheMatrix
force-pushed
the
sync/zstackio/remove-resource-keyref@@2
branch
from
5c27d70 to
70f437f
Compare
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 `@compute/src/main/java/org/zstack/compute/vm/devices/VmTpmManager.java`:
- Around line 73-75: The current deleteTpmAndResourceKeyRef method calls
deleteResourceKeyRefForTpm(tpmUuid) then deleteTpmVO(tpmUuid) sequentially, but
if deleteResourceKeyRefForTpm throws the TPM record (TpmVO) may not be removed;
change deleteTpmAndResourceKeyRef to call deleteResourceKeyRefForTpm inside a
try block and always call deleteTpmVO in a finally block so the TPM row is
deleted regardless of exceptions; keep the same method names
(deleteTpmAndResourceKeyRef, deleteResourceKeyRefForTpm, deleteTpmVO) and
preserve existing exception propagation/logging behavior.
🪄 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: 50c7ae3a-87d8-4ddd-a783-5bc716608c19
⛔ Files ignored due to path filters (1)
conf/springConfigXml/VmInstanceManager.xmlis excluded by!**/*.xml
📒 Files selected for processing (6)
compute/src/main/java/org/zstack/compute/vm/devices/DummyEncryptedResourceKeyManager.javacompute/src/main/java/org/zstack/compute/vm/devices/VmTpmExtensions.javacompute/src/main/java/org/zstack/compute/vm/devices/VmTpmManager.javaheader/src/main/java/org/zstack/header/keyprovider/EncryptedResourceKeyManager.javaplugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmExtensions.javaplugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.java
🚧 Files skipped from review as they are similar to previous changes (4)
- compute/src/main/java/org/zstack/compute/vm/devices/DummyEncryptedResourceKeyManager.java
- plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.java
- plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmExtensions.java
- header/src/main/java/org/zstack/header/keyprovider/EncryptedResourceKeyManager.java
compute/src/main/java/org/zstack/compute/vm/devices/VmTpmManager.java
Outdated
Show resolved
Hide resolved
Collaborator
|
Comment from yaohua.wu: Review: MR !9540 — ZSV-11721 (zstack repo)
总体评价目标明确：将 TPM key ref 的清理从分散的 🔴 Critical1. [MR 合并状态] MR 存在合并冲突
🟡 Warning1. [VmTpmManager.java:73-75] 旧代码 // 旧代码（有 try-finally 保护）
try {
resourceKeyBackend.detachKeyProviderFromTpm(tpmUuid);
} finally {
vmTpmManager.deleteTpmVO(tpmUuid);
}新代码顺序调用，无保护： public void deleteTpmAndResourceKeyRef(String tpmUuid) {
deleteResourceKeyRefForTpm(tpmUuid); // 若此处抛异常...
deleteTpmVO(tpmUuid); // ...此行不会执行，TpmVO 泄漏
}影响范围：两处调用受影响：
建议恢复 try-finally 语义： public void deleteTpmAndResourceKeyRef(String tpmUuid) {
try {
deleteResourceKeyRefForTpm(tpmUuid);
} finally {
deleteTpmVO(tpmUuid);
}
}CodeRabbit 也标记了此问题，建议采纳。 2. [KvmTpmExtensions.java:173,182] // 旧：有任一为空就 skip/fail
(StringUtils.isBlank(context.providerUuid) || StringUtils.isBlank(context.providerName))
// 新：两者都为空才 skip/fail
(StringUtils.isBlank(context.providerUuid) && StringUtils.isBlank(context.providerName))变更后语义：只要有 UUID 或 name 中的任意一个，就会进入 语义本身是合理的（配合 name fallback），但需注意：如果 3. [VmTpmExtensions.java:127-141] 当用户先调用 RemoveTpm（删除 TPM 设备），然后 expunge VM 时：
此时 TpmVO 已在 RemoveTpm 流程中被删除，所以 🟢 Suggestion1. [VmTpmManager.java:49-51] 关键资源清理操作缺少日志输出，不利于问题排查： public void deleteResourceKeyRefForTpm(String tpmUuid) {
logger.debug(String.format("deleting encrypted resource key ref for TpmVO[uuid:%s]", tpmUuid));
resourceKeyManager.deleteRef(TpmVO.class.getSimpleName(), tpmUuid);
}2. [KvmTpmExtensions.java:199] context.providerUuid = result.getKeyProviderUuid();在 Verdict: REVISION_REQUIRED主要原因：合并冲突需解决； 🤖 Robot Reviewer |
added 2 commits
April 4, 2026 16:21
Delay/trash deletion no longer drops EncryptedResourceKeyRef; cleanup runs in vmJustBeforeDeleteFromDb (expunge/direct delete). Centralize ref delete in VmTpmManager; wire RemoveTpm/add-tpm rollback and drop unused destroy extension from VmTpmExtensions. Resolves: ZSV-11721 Change-Id: I68676e706f6a786773627074636f6e6a617a6e4f
…m-and-secuceboot' into remove-resource-keyref@@2
There was a problem hiding this comment.
🧹 Nitpick comments (2)
compute/src/main/java/org/zstack/compute/vm/devices/VmTpmManager.java (1)
52-54: 建议添加 debug 日志记录 ref 删除尝试。根据 PR 评审建议，在
deleteResourceKeyRefForTpm中添加 debug 日志有助于追踪 ref 删除操作，便于问题排查。♻️ 建议的修改
public void deleteResourceKeyRefForTpm(String tpmUuid) { + logger.debug(String.format("Deleting resource key ref for TPM[uuid:%s]", tpmUuid)); resourceKeyManager.deleteRef(TpmVO.class.getSimpleName(), tpmUuid); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@compute/src/main/java/org/zstack/compute/vm/devices/VmTpmManager.java` around lines 52 - 54, 在 VmTpmManager.deleteResourceKeyRefForTpm 中，在调用 resourceKeyManager.deleteRef(TpmVO.class.getSimpleName(), tpmUuid) 之前添加一条 debug 日志，记录尝试删除的资源类型和 tpmUuid（例如 logger.debug("Deleting resource key ref for {} id={}", TpmVO.class.getSimpleName(), tpmUuid)），以便追踪删除操作；如果 deleteRef 返回或抛出异常，可以在调用后补充相应的 debug/trace 日志或在 catch 块中记录错误细节以便排查。compute/src/main/java/org/zstack/compute/vm/devices/VmTpmExtensions.java (1)
72-77: SQLBatch 包装器可简化。
deleteTpmAndResourceKeyRef是单个方法调用，且内部已包含 try-finally 逻辑。外层的SQLBatch包装可能是为了与原有代码风格保持一致，但从功能上看并非必需。如果deleteTpmAndResourceKeyRef内部的数据库操作需要在同一事务中执行，可以考虑在该方法内部处理。♻️ 可选简化
if (tpmUuid == null) { return; } - new SQLBatch() { - `@Override` - protected void scripts() { - vmTpmManager.deleteTpmAndResourceKeyRef(tpmUuid); - } - }.execute(); + vmTpmManager.deleteTpmAndResourceKeyRef(tpmUuid); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@compute/src/main/java/org/zstack/compute/vm/devices/VmTpmExtensions.java` around lines 72 - 77, The SQLBatch wrapper around the single call to vmTpmManager.deleteTpmAndResourceKeyRef(tpmUuid) is unnecessary; remove the new SQLBatch() { ... }.execute() block in VmTpmExtensions and invoke vmTpmManager.deleteTpmAndResourceKeyRef(tpmUuid) directly (or, if transactional semantics are required, ensure deleteTpmAndResourceKeyRef handles its own transaction/try-finally inside vmTpmManager). This simplifies the code by eliminating the redundant SQLBatch wrapper while preserving existing error/transaction handling in deleteTpmAndResourceKeyRef.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@compute/src/main/java/org/zstack/compute/vm/devices/VmTpmExtensions.java`:
- Around line 72-77: The SQLBatch wrapper around the single call to
vmTpmManager.deleteTpmAndResourceKeyRef(tpmUuid) is unnecessary; remove the new
SQLBatch() { ... }.execute() block in VmTpmExtensions and invoke
vmTpmManager.deleteTpmAndResourceKeyRef(tpmUuid) directly (or, if transactional
semantics are required, ensure deleteTpmAndResourceKeyRef handles its own
transaction/try-finally inside vmTpmManager). This simplifies the code by
eliminating the redundant SQLBatch wrapper while preserving existing
error/transaction handling in deleteTpmAndResourceKeyRef.
In `@compute/src/main/java/org/zstack/compute/vm/devices/VmTpmManager.java`:
- Around line 52-54: 在 VmTpmManager.deleteResourceKeyRefForTpm 中，在调用
resourceKeyManager.deleteRef(TpmVO.class.getSimpleName(), tpmUuid) 之前添加一条 debug
日志，记录尝试删除的资源类型和 tpmUuid（例如 logger.debug("Deleting resource key ref for {}
id={}", TpmVO.class.getSimpleName(), tpmUuid)），以便追踪删除操作；如果 deleteRef
返回或抛出异常，可以在调用后补充相应的 debug/trace 日志或在 catch 块中记录错误细节以便排查。
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 572a4d89-341a-4e0e-bac4-d4e48dccaa89
⛔ Files ignored due to path filters (1)
conf/springConfigXml/VmInstanceManager.xmlis excluded by!**/*.xml
📒 Files selected for processing (3)
compute/src/main/java/org/zstack/compute/vm/devices/VmTpmExtensions.javacompute/src/main/java/org/zstack/compute/vm/devices/VmTpmManager.javaplugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.java
…onflict Integrate origin/remove-resource-keyref@@2 after divergent pushes; conflict in VmTpmManager/VmTpmExtensions resolved keeping try/finally deleteTpmAndResourceKeyRef and extended javadoc. Resolves: ZSV-11721 Change-Id: I00d3201a733026a6c82edc281dcb6fa760e6ddfd
MatheMatrix
force-pushed
the
sync/zstackio/remove-resource-keyref@@2
branch
from
771fefb to
0ed8cb6
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
2 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-11721
Change-Id: I68676e706f6a786773627074636f6e6a617a6e4f
sync from gitlab !9540