Skip to content

<feature>[kvm]: support TPM revert without KMS#3696

Closed
MatheMatrix wants to merge 1 commit intofeature-zsv-5.0.0-vm-support-vtpm-and-secucebootfrom
sync/wenhao.zhang/zsv-ldap-9
Closed

<feature>[kvm]: support TPM revert without KMS#3696
MatheMatrix wants to merge 1 commit intofeature-zsv-5.0.0-vm-support-vtpm-and-secucebootfrom
sync/wenhao.zhang/zsv-ldap-9

Conversation

@MatheMatrix
Copy link
Copy Markdown
Owner

Support TPM snapshot group revert when ALLOWED_TPM_VM_WITHOUT_KMS
config is enabled. Skip key provider resolution to allow TPM
recovery without KMS dependency.

Changes:

  • Add ALLOWED_TPM_VM_WITHOUT_KMS config check
  • Skip key provider resolution when config is true

Resolves: ZSV-11489
Related: ZSV-11310

Change-Id: I6470766e66656f6464767a7776696a7a6979706b

sync from gitlab !9558

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 7, 2026

Walkthrough

在 TPM 快照还原助手中加入对全局配置 ALLOWED_TPM_VM_WITHOUT_KMS 的运行时检查;当该标志为真时,跳过密钥提供者名称/UUID 的解析、默认设置及相关警告逻辑;同时调整了在未解析到名称或 UUID 时的警告文本与默认行为。

Changes

Cohort / File(s) Summary
TPM 密钥提供者配置
plugin/kvm/src/main/java/org/zstack/kvm/tpm/SnapshotGroupRevertTpmHelper.java
新增静态导入并基于 ALLOWED_TPM_VM_WITHOUT_KMS 进行条件分支:当标志不为真时执行 key-provider 名称解析、通过 tpmKeyBackend 解析/设置 keyProviderUuid 并在缺失时使用默认 UUID;当标志为真时跳过上述流程。修改并简化了未找到名称/UUID 时的警告文本和默认设置行为。

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 分钟

Poem

🐰✨ 我是小兔守密林,
配置风轻过指尖,
若允许跳过钥匙声,
不问来处与归程,
仍留警示暖如春。

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed 标题清晰明确地总结了主要变更:为KVM模块的TPM快照恢复添加支持在无KMS情况下的功能,与代码变更直接相关。
Description check ✅ Passed 描述准确地说明了变更内容:在启用ALLOWED_TPM_VM_WITHOUT_KMS配置时支持TPM快照组恢复,包含具体改动说明和相关issue编号。

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch sync/wenhao.zhang/zsv-ldap-9

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
plugin/kvm/src/main/java/org/zstack/kvm/tpm/SnapshotGroupRevertTpmHelper.java (1)

103-124: 建议:当跳过 key provider 解析时添加日志。

skipKeyProvidertrue 时,整个 key provider 解析逻辑被跳过,但没有任何日志记录。参考 KvmTpmExtensions.java(上下文代码片段 3)中的实现,在跳过 create-dek 步骤时会输出日志:"skip create-dek: allowed.tpm.vm.without.kms is enabled and no KMS provider bound"

建议添加类似的日志以便于调试和问题排查:

♻️ 建议添加跳过日志
             final Boolean skipKeyProvider = ALLOWED_TPM_VM_WITHOUT_KMS.value(Boolean.class);
             if (skipKeyProvider != Boolean.TRUE) {
                 String keyProviderName = KVMSystemTags.TPM_KEY_PROVIDER_NAME
                         .getTokenByResourceUuid(tpmBackupFile.getUuid(), KVMSystemTags.TPM_KEY_PROVIDER_NAME_TOKEN);
                 // ... existing logic ...
+            } else {
+                logger.debug(String.format("skip key provider resolution for snapshotGroup[uuid:%s]: " +
+                        "allowed.tpm.vm.without.kms is enabled", snapshotGroupUuid));
             }
🤖 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/SnapshotGroupRevertTpmHelper.java`
around lines 103 - 124, When ALLOWED_TPM_VM_WITHOUT_KMS causes skipKeyProvider
to be true the method in SnapshotGroupRevertTpmHelper currently does nothing and
emits no trace; add a log entry (e.g. logger.info or logger.warn) in the branch
where skipKeyProvider == Boolean.TRUE to record that key-provider resolution was
skipped because allowed.tpm.vm.without.kms is enabled and no KMS provider is
bound, referencing the snapshotGroupUuid and tpmBackupFile.getUuid() for context
so operators can correlate events.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In
`@plugin/kvm/src/main/java/org/zstack/kvm/tpm/SnapshotGroupRevertTpmHelper.java`:
- Around line 103-124: When ALLOWED_TPM_VM_WITHOUT_KMS causes skipKeyProvider to
be true the method in SnapshotGroupRevertTpmHelper currently does nothing and
emits no trace; add a log entry (e.g. logger.info or logger.warn) in the branch
where skipKeyProvider == Boolean.TRUE to record that key-provider resolution was
skipped because allowed.tpm.vm.without.kms is enabled and no KMS provider is
bound, referencing the snapshotGroupUuid and tpmBackupFile.getUuid() for context
so operators can correlate events.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: d1be1bc3-83f0-4b56-bd7a-f8120a254ddc

📥 Commits

Reviewing files that changed from the base of the PR and between 912b02f and e7edbfb.

📒 Files selected for processing (1)
  • plugin/kvm/src/main/java/org/zstack/kvm/tpm/SnapshotGroupRevertTpmHelper.java

Support TPM snapshot group revert when ALLOWED_TPM_VM_WITHOUT_KMS
config is enabled. Skip key provider resolution to allow TPM
recovery without KMS dependency.

Changes:
- Add ALLOWED_TPM_VM_WITHOUT_KMS config check
- Skip key provider resolution when config is true

Resolves: ZSV-11489
Related: ZSV-11310

Change-Id: I6470766e66656f6464767a7776696a7a6979706b
@MatheMatrix MatheMatrix force-pushed the sync/wenhao.zhang/zsv-ldap-9 branch from e7edbfb to 866fc0f Compare April 7, 2026 10:57
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
plugin/kvm/src/main/java/org/zstack/kvm/tpm/SnapshotGroupRevertTpmHelper.java (1)

103-123: 建议把 provider 解析/兜底提取成私有方法。

新增这层配置门控后,setupFromApi() 在 TPM 还原分支里的 if/else 层级又加深了一层。把 keyProviderName 查询、keyProviderUuid 解析和默认值兜底拆出去,会让 resetTpmbackupFileUuid 和“允许无 KMS”这三条主线更清楚。
As per coding guidelines, “应尽量减少 if...else 结构的使用,建议:限制嵌套层级最多为两层,且内层不应再出现 else 分支。尽早返回(Early 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/tpm/SnapshotGroupRevertTpmHelper.java`
around lines 103 - 123, Extract the key-provider lookup/resolve logic into a
private helper in SnapshotGroupRevertTpmHelper (called from setupFromApi) to
reduce nesting: create a method like resolveAndSetKeyProvider(TpmSpec tpmSpec,
BackupFile tpmBackupFile, String snapshotGroupUuid) that encapsulates reading
KVMSystemTags.TPM_KEY_PROVIDER_NAME token, calling
tpmKeyBackend.findKeyProviderUuidByName(...), setting
tpmSpec.setKeyProviderUuid(...) or falling back to
tpmKeyBackend.defaultKeyProviderUuid() when appropriate and logging warnings;
then replace the current nested if/else block (which references
ALLOWED_TPM_VM_WITHOUT_KMS, tpmBackupFile.getUuid(), keyProviderName,
keyProviderUuid) with a simple guarded call to this helper to keep
setupFromApi’s control flow shallow and follow early-return/early-exit style.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In
`@plugin/kvm/src/main/java/org/zstack/kvm/tpm/SnapshotGroupRevertTpmHelper.java`:
- Around line 103-123: Extract the key-provider lookup/resolve logic into a
private helper in SnapshotGroupRevertTpmHelper (called from setupFromApi) to
reduce nesting: create a method like resolveAndSetKeyProvider(TpmSpec tpmSpec,
BackupFile tpmBackupFile, String snapshotGroupUuid) that encapsulates reading
KVMSystemTags.TPM_KEY_PROVIDER_NAME token, calling
tpmKeyBackend.findKeyProviderUuidByName(...), setting
tpmSpec.setKeyProviderUuid(...) or falling back to
tpmKeyBackend.defaultKeyProviderUuid() when appropriate and logging warnings;
then replace the current nested if/else block (which references
ALLOWED_TPM_VM_WITHOUT_KMS, tpmBackupFile.getUuid(), keyProviderName,
keyProviderUuid) with a simple guarded call to this helper to keep
setupFromApi’s control flow shallow and follow early-return/early-exit style.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 9b454d11-e761-4792-999b-d28da9b0c111

📥 Commits

Reviewing files that changed from the base of the PR and between e7edbfb and 866fc0f.

📒 Files selected for processing (1)
  • plugin/kvm/src/main/java/org/zstack/kvm/tpm/SnapshotGroupRevertTpmHelper.java

@ZStack-Robot ZStack-Robot deleted the sync/wenhao.zhang/zsv-ldap-9 branch April 7, 2026 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants