<fix>[kvm]: create key when attach tpm#3776
Closed
zstack-robot-2 wants to merge 1 commit intozsv_5.0.0from
Closed
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Walkthrough在 Changes
Sequence Diagram(s)sequenceDiagram
participant VMFlow as "VM Create Flow"
participant Kvm as "KvmTpmManager"
participant KeyMgr as "EncryptedResourceKeyManager"
participant KeyProv as "KeyProvider DB"
VMFlow->>Kvm: request addTpmToVm(context)
Kvm->>Kvm: check context.keyProviderUuid
alt keyProviderUuid == null
Kvm-->>VMFlow: trigger.fail(operr(...))
else keyProviderUuid != null
Kvm->>KeyProv: attach key provider to TPM (async)
KeyProv-->>Kvm: attach success
Kvm->>KeyMgr: getOrCreateKey(resourceUuid=TPM UUID, resourceType=TpmVO, keyProviderUuid, purpose="vtpm")
alt getOrCreateKey success
KeyMgr-->>Kvm: success
Kvm-->>VMFlow: trigger.next()
else getOrCreateKey fail
KeyMgr-->>Kvm: error
Kvm-->>VMFlow: trigger.fail(error)
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.
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/tpm/KvmTpmManager.java (1)
287-292:⚠️ Potential issue | 🟠 Major缺少对创建的资源密钥的回滚处理。
当前的回滚逻辑（lines 287-292）仅分离了密钥提供器，但没有处理在 lines 268-284 中可能创建的加密资源密钥。根据检索到的学习记录，
EncryptedResourceKeyManager.ResourceKeyResult提供了isCreatedNewKey()方法来判断是否创建了新密钥。如果创建了新密钥，回滚时应删除已具体化的 key-tool secret 并移除EncryptedResourceKeyRef行。建议在回滚逻辑中保存
ResourceKeyResult，并在回滚时检查isCreatedNewKey()以决定是否需要清理密钥。Based on learnings: "In ZStack's KvmTpmExtensions.preReleaseVmResource, as of PR
#3716, theResourceKeyResult.refExistedBeforeCreatefield has been removed fromEncryptedResourceKeyManager.ResourceKeyResult. Rollback behavior is now determined solely byResourceKeyResult.isCreatedNewKey(): when true, the implementation deletes the materialized key-tool secret and removes theEncryptedResourceKeyRefrow; when false, rollback is a no-op."🤖 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/KvmTpmManager.java` around lines 287 - 292, The rollback currently only detaches the key provider (tpmKeyBackend.detachKeyProviderFromTpm using context.createdTpmUuid) but misses cleanup for any newly created encrypted resource key; capture and store the EncryptedResourceKeyManager.ResourceKeyResult from the create path (e.g., in the KvmTpmManager context as context.resourceKeyResult) and in the rollback lambda check resourceKeyResult.isCreatedNewKey(), and if true delete the materialized key-tool secret and remove the EncryptedResourceKeyRef row (use your existing key-secret deletion and DB removal helpers or add methods like deleteKeyToolSecret(...) and removeEncryptedResourceKeyRef(...)); leave behavior as no-op when isCreatedNewKey() is false.
🧹 Nitpick comments (1)
plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.java (1)
268-273: 建议使用完整的变量名以提高可读性。变量名
keyCtx使用了缩写。根据编码规范，应避免不必要的缩写，建议使用完整的变量名keyContext以提升代码可读性。As per coding guidelines: "不允许使用不必要的缩写，如：
AbsSchedulerJob、condi、Fu等。应使用完整单词提升可读性。"♻️ 重命名建议
- EncryptedResourceKeyManager.GetOrCreateResourceKeyContext keyCtx = + EncryptedResourceKeyManager.GetOrCreateResourceKeyContext keyContext = new EncryptedResourceKeyManager.GetOrCreateResourceKeyContext(); - keyCtx.setResourceUuid(context.createdTpmUuid); - keyCtx.setResourceType(TpmVO.class.getSimpleName()); - keyCtx.setKeyProviderUuid(context.keyProviderUuid); - keyCtx.setPurpose("vtpm"); - resourceKeyManager.getOrCreateKey(keyCtx, new ReturnValueCompletion<EncryptedResourceKeyManager.ResourceKeyResult>(trigger) { + keyContext.setResourceUuid(context.createdTpmUuid); + keyContext.setResourceType(TpmVO.class.getSimpleName()); + keyContext.setKeyProviderUuid(context.keyProviderUuid); + keyContext.setPurpose("vtpm"); + resourceKeyManager.getOrCreateKey(keyContext, new ReturnValueCompletion<EncryptedResourceKeyManager.ResourceKeyResult>(trigger) {🤖 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/KvmTpmManager.java` around lines 268 - 273, Rename the local variable keyCtx to a full-word name (e.g., keyContext) where the EncryptedResourceKeyManager.GetOrCreateResourceKeyContext is instantiated and used in the KvmTpmManager class; update all subsequent references (setResourceUuid, setResourceType, setKeyProviderUuid, setPurpose) to use the new variable name to follow the coding guideline against unnecessary abbreviations.
🤖 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/KvmTpmManager.java`:
- Around line 259-286: The code calls resourceKeyManager.getOrCreateKey(...)
with a ReturnValueCompletion that calls trigger.next() on success, but then
immediately calls trigger.next() again right after starting the async call,
causing duplicate continuation; remove the extraneous trigger.next() that
follows resourceKeyManager.getOrCreateKey(...) so only the completion's
success() advances the flow (leave tpmKeyBackend.attachKeyProviderToTpm(...) and
context.keyProviderAttached = true as-is and rely on
EncryptedResourceKeyManager.GetOrCreateResourceKeyContext + the
ReturnValueCompletion to drive the next trigger).
---
Outside diff comments:
In `@plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.java`:
- Around line 287-292: The rollback currently only detaches the key provider
(tpmKeyBackend.detachKeyProviderFromTpm using context.createdTpmUuid) but misses
cleanup for any newly created encrypted resource key; capture and store the
EncryptedResourceKeyManager.ResourceKeyResult from the create path (e.g., in the
KvmTpmManager context as context.resourceKeyResult) and in the rollback lambda
check resourceKeyResult.isCreatedNewKey(), and if true delete the materialized
key-tool secret and remove the EncryptedResourceKeyRef row (use your existing
key-secret deletion and DB removal helpers or add methods like
deleteKeyToolSecret(...) and removeEncryptedResourceKeyRef(...)); leave behavior
as no-op when isCreatedNewKey() is false.
---
Nitpick comments:
In `@plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.java`:
- Around line 268-273: Rename the local variable keyCtx to a full-word name
(e.g., keyContext) where the
EncryptedResourceKeyManager.GetOrCreateResourceKeyContext is instantiated and
used in the KvmTpmManager class; update all subsequent references
(setResourceUuid, setResourceType, setKeyProviderUuid, setPurpose) to use the
new variable name to follow the coding guideline against unnecessary
abbreviations.
🪄 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: cd178880-46e3-4845-a27f-ac41d0d17d29
📒 Files selected for processing (1)
plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.java
For existing non-vTPM VMs, attach the key provider and materialize the actual key before preInstantiateVmResource, so libvirt secret cache lookup does not miss keyVersion. Resolves: ZSV-11729 Change-Id: I6e6d6c6c647122226769756b75756c6f72654477
MatheMatrix
force-pushed
the
sync/zstackio/ref-attach-tpm@@2
branch
from
f30ef86 to
59d0b78
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.
For existing non-vTPM VMs, attach the key provider and
materialize the actual key before preInstantiateVmResource,
so libvirt secret cache lookup does not miss keyVersion.
Resolves: ZSV-11729
Change-Id: I6e6d6c6c647122226769756b75756c6f72654477
sync from gitlab !9646