Skip to content

<feature>[zbs]: ZSV-12579 support zbs volume encryption#4366

Open
ZStack-Robot wants to merge 1 commit into
feature-zsv-5.1.0-encryptionfrom
sync/zstackio/zbs-volume-encryption@@3
Open

<feature>[zbs]: ZSV-12579 support zbs volume encryption#4366
ZStack-Robot wants to merge 1 commit into
feature-zsv-5.1.0-encryptionfrom
sync/zstackio/zbs-volume-encryption@@3

Conversation

@ZStack-Robot

Copy link
Copy Markdown
Collaborator

Summary

Implement ZBS encrypted volume support aligned with LocalStorage, NFS, SharedBlock, and RBD behavior.

Changes

  • Carry encryption context through ExternalPrimaryStorage create, clone, snapshot, image-cache, resize, and conversion paths.
  • Add ZBS controller handling for LUKS-backed create/copy/convert/resize flows and actual-size refresh.
  • Add ZBS install path conversion handling and focused spec coverage.

Testing

  • cbok zsv compile deployed to 172.24.241.203
  • cbok zsv replace_agent deployed to all ZBS nodes
  • cbok zsv status shows MN/UI Running
  • git diff --check

Resolves: ZSV-12579

sync from gitlab !10327

@coderabbitai

coderabbitai Bot commented Jun 30, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 41 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 4ab9901e-9a08-4f43-a4d1-efc66092e5e8

📥 Commits

Reviewing files that changed from the base of the PR and between 9ddbfda and 001b74a.

⛔ Files ignored due to path filters (1)
  • conf/springConfigXml/encrypt.xml is excluded by !**/*.xml
📒 Files selected for processing (26)
  • header/src/main/java/org/zstack/header/storage/addon/primary/CreateVolumeSpec.java
  • header/src/main/java/org/zstack/header/storage/addon/primary/PrimaryStorageControllerSvc.java
  • header/src/main/java/org/zstack/header/storage/addon/primary/ZbsVolumeEncryptionBackend.java
  • header/src/main/java/org/zstack/header/storage/addon/primary/ZbsVolumeEncryptionExtensionPoint.java
  • header/src/main/java/org/zstack/header/storage/primary/EncryptVolumeBitsOnPrimaryStorageMsg.java
  • header/src/main/java/org/zstack/header/storage/primary/EncryptVolumeBitsOnPrimaryStorageReply.java
  • plugin/expon/src/main/java/org/zstack/expon/ExponStorageController.java
  • plugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsStorageController.java
  • plugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsVolumeEncryptionBackendImpl.java
  • storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java
  • storage/src/main/java/org/zstack/storage/encrypt/ZbsEncryptedEmptyVolumeCreator.java
  • storage/src/main/java/org/zstack/storage/encrypt/ZbsEncryptedSnapshotVolumeCopier.java
  • storage/src/main/java/org/zstack/storage/encrypt/ZbsEncryptedVolumeCloner.java
  • storage/src/main/java/org/zstack/storage/encrypt/ZbsEncryptedVolumeResizer.java
  • storage/src/main/java/org/zstack/storage/encrypt/ZbsVolumeEncryptInPlaceHandler.java
  • storage/src/main/java/org/zstack/storage/encrypt/ZbsVolumeEncryptionCleanup.java
  • storage/src/main/java/org/zstack/storage/encrypt/ZbsVolumeEncryptionCommands.java
  • storage/src/main/java/org/zstack/storage/encrypt/ZbsVolumeEncryptionConstants.java
  • storage/src/main/java/org/zstack/storage/encrypt/ZbsVolumeEncryptionExtension.java
  • storage/src/main/java/org/zstack/storage/encrypt/ZbsVolumeEncryptionKvmCaller.java
  • storage/src/main/java/org/zstack/storage/encrypt/ZbsVolumeEncryptionMaterial.java
  • storage/src/main/java/org/zstack/storage/encrypt/ZbsVolumeEncryptionMaterialFactory.java
  • storage/src/main/java/org/zstack/storage/volume/VolumeInPlaceEncryptor.java
  • storage/src/main/java/org/zstack/storage/volume/VolumeManagerImpl.java
  • test/src/test/groovy/org/zstack/storage/volume/VolumeManagerImplHostSelectionTest.groovy
  • test/src/test/groovy/org/zstack/test/unittest/storage/encrypt/ZbsVolumeEncryptionExtensionTest.groovy

Walkthrough

本次改动扩展了加密卷的公共契约和扩展点,并让 ZBS 控制器、外部主存储与 Expon 扩容路径按卷的加密状态分派和填充相关字段。

Changes

ZBS 加密卷与镜像缓存支持

Layer / File(s) Summary
契约与扩展点
header/.../CreateVolumeSpec.java, header/.../PrimaryStorageControllerSvc.java, header/.../ZbsVolumeEncryptionExtensionPoint.java
CreateVolumeSpec 增加 encrypted、hostUuid、encryptedDek 字段及访问方法;PrimaryStorageControllerSvc 新增 encryptVolumeBits 默认失败实现并将 expandVolume 入参改为 VolumeInventory;新增 ZbsVolumeEncryptionExtensionPoint 公开接口。
控制器分派与适配
plugin/zbs/.../ZbsStorageController.java, plugin/expon/.../ExponStorageController.java
ZbsStorageController 通过插件注册表接入加密创建、克隆、拷贝、加密块处理和加密扩容分支;ExponStorageController 同步调整 expandVolume 签名并继续按卷路径扩容。
外部主存储加密准备与缓存处理
storage/.../ExternalPrimaryStorage.java, storage/.../ExternalPrimaryStorageFactory.java
外部主存储补充宿主筛选、卷/快照加密规格准备、加密块处理、镜像缓存加密字段填充与重建根卷加密准备;工厂侧将快照加密状态下发到模板创建消息。

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant ExternalPrimaryStorage
  participant ZbsStorageController
  participant ZbsVolumeEncryptionExtensionPoint

  Caller->>ExternalPrimaryStorage: 发送卷/快照相关主存储消息
  ExternalPrimaryStorage->>ExternalPrimaryStorage: prepareVolumeSpecEncryption / prepareSnapshotImageCacheEncryption
  ExternalPrimaryStorage->>ZbsStorageController: 提交带加密字段的处理请求
  ZbsStorageController->>ZbsVolumeEncryptionExtensionPoint: 按 encrypted 状态选择对应扩展实现
  ZbsVolumeEncryptionExtensionPoint-->>ZbsStorageController: 返回 VolumeStats 或加密结果
  ZbsStorageController-->>ExternalPrimaryStorage: 回传处理结果
  ExternalPrimaryStorage-->>Caller: 返回主存储回复
Loading

Poem

小兔揣着加密卷,🐇
宿主、快照都照看。
扩展点里轻轻转,
字段一填路就宽。
LUKS 轻跳到云端。

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.59% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed 标题简洁且准确概括了本次 ZBS 卷加密支持的主要变更。
Description check ✅ Passed 描述与变更内容一致,详细说明了加密上下文传递和 ZBS 控制器支持。
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch sync/zstackio/zbs-volume-encryption@@3

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🧹 Nitpick comments (1)
plugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsStorageController.java (1)

672-688: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

resolveLuksHostUuid 建议校验宿主可用性。

此处仅取任意一条 status=ConnectedPrimaryStorageHostRefVO,未校验宿主的 hypervisorType=KVMstate=Enabled/status=Connected(对比 ExternalPrimaryStorage.findConnectedKvmHostForEncryption 的更严格筛选)。可能选中一台已 Disabled 或主机侧失联的宿主,导致 LUKS 操作失败。建议与外部主存储侧保持一致的宿主筛选条件。

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@plugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsStorageController.java`
around lines 672 - 688, The resolveLuksHostUuid method currently picks any
connected PrimaryStorageHostRefVO without verifying the host is a usable KVM
host. Update the host lookup in ZbsStorageController.resolveLuksHostUuid to
apply the same stricter filtering as
ExternalPrimaryStorage.findConnectedKvmHostForEncryption, including
hypervisorType=KVM and host state/status checks such as Enabled and Connected.
Keep the preferredHostUuid shortcut intact, but make the fallback selection only
return a host that is actually eligible for the LUKS operation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@plugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsStorageController.java`:
- Around line 1202-1233: The replace-original-volume flow currently deletes the
original volume before the COPY step, creating an unrecoverable window if the
copy fails. Update the logic in the replace-original-volume NoRollbackFlow
within ZbsStorageController so the data is copied from tempPath to the target
volume first, and only after a successful httpCall(COPY_PATH, ...) should
doDeleteVolume(msg.getInstallPath(), true) run; alternatively make this step
rollback-safe by restoring from tempPath on failure. Keep the trigger/completion
handling in place, but ensure the original install path is not removed until the
replacement volume is fully written and verified.

In
`@storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java`:
- Around line 2244-2248: The GetVolumeSnapshotEncryptedOnPrimaryStorageMsg
handler is returning the primary storage install path in the encrypt field,
which is the wrong value. Update
ExternalPrimaryStorage.handle(GetVolumeSnapshotEncryptedOnPrimaryStorageMsg) so
reply.setEncrypt uses the actual encryption flag/status from the message or
computed result instead of msg.getPrimaryStorageInstallPath(), and keep the
snapshotUuid reply unchanged.
- Around line 176-198: The host selection logic in
ExternalPrimaryStorage.getConnectedHostUuid currently relaxes the filter when
connectedPsHostUuids is empty, which can allow picking a KVM host that is not
actually connected to the external primary storage. Update this method to
distinguish between “no connected PrimaryStorageHostRefVO exists” and “some refs
exist but none are currently connected,” and fail fast in the empty case instead
of removing the hostUuid constraint. Keep the existing query constraints on
clusterUuids, VmInstanceConstant.KVM_HYPERVISOR_TYPE, HostStatus.Connected, and
HostState.Enabled, but ensure the fallback does not broaden selection beyond
hosts truly attached to the storage.

---

Nitpick comments:
In `@plugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsStorageController.java`:
- Around line 672-688: The resolveLuksHostUuid method currently picks any
connected PrimaryStorageHostRefVO without verifying the host is a usable KVM
host. Update the host lookup in ZbsStorageController.resolveLuksHostUuid to
apply the same stricter filtering as
ExternalPrimaryStorage.findConnectedKvmHostForEncryption, including
hypervisorType=KVM and host state/status checks such as Enabled and Connected.
Keep the preferredHostUuid shortcut intact, but make the fallback selection only
return a host that is actually eligible for the LUKS operation.
🪄 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: c7290d64-3289-461e-9f24-d18323f993c4

📥 Commits

Reviewing files that changed from the base of the PR and between 45e799e and ef5e489.

📒 Files selected for processing (8)
  • header/src/main/java/org/zstack/header/storage/addon/primary/BaseVolumeInfo.java
  • header/src/main/java/org/zstack/header/storage/addon/primary/CreateVolumeSnapshotSpec.java
  • header/src/main/java/org/zstack/header/storage/addon/primary/CreateVolumeSpec.java
  • header/src/main/java/org/zstack/header/storage/addon/primary/PrimaryStorageControllerSvc.java
  • plugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsStorageController.java
  • storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java
  • storage/src/main/java/org/zstack/storage/volume/VolumeBase.java
  • test/src/test/java/org/zstack/test/unittest/storage/addon/primary/ExternalPrimaryStorageEncryptionSpecTest.java

Comment thread plugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsStorageController.java Outdated
Comment on lines +176 to +198
List<String> connectedPsHostUuids = Q.New(PrimaryStorageHostRefVO.class)
.select(PrimaryStorageHostRefVO_.hostUuid)
.eq(PrimaryStorageHostRefVO_.primaryStorageUuid, self.getUuid())
.eq(PrimaryStorageHostRefVO_.status, PrimaryStorageHostStatus.Connected)
.listValues();

org.zstack.core.db.SimpleQuery<HostVO> q = dbf.createQuery(HostVO.class);
q.add(HostVO_.clusterUuid, org.zstack.core.db.SimpleQuery.Op.IN, clusterUuids);
if (!connectedPsHostUuids.isEmpty()) {
q.add(HostVO_.uuid, org.zstack.core.db.SimpleQuery.Op.IN, connectedPsHostUuids);
}
q.add(HostVO_.hypervisorType, org.zstack.core.db.SimpleQuery.Op.EQ, VmInstanceConstant.KVM_HYPERVISOR_TYPE);
q.add(HostVO_.status, org.zstack.core.db.SimpleQuery.Op.EQ, HostStatus.Connected);
q.add(HostVO_.state, org.zstack.core.db.SimpleQuery.Op.EQ, HostState.Enabled);
q.orderBy(HostVO_.uuid, org.zstack.core.db.SimpleQuery.Od.ASC);
q.setLimit(1);
HostVO host = q.find();
if (host == null) {
throw new OperationFailureException(operr(
"cannot find a connected KVM host attached to external primary storage[uuid:%s] to %s",
self.getUuid(), operation));
}
return host.getUuid();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# 确认 PrimaryStorageHostRefVO 连接状态对外部主存储如何维护
rg -nP '\bupdatePrimaryStorageHostStatus\b' --type=java -C3 -g '!**/test/**'
echo "===== PrimaryStorageHostRefVO 写入点 ====="
rg -nP 'PrimaryStorageHostRefVO_\.status|set\(PrimaryStorageHostRefVO_\.status' --type=java -C2 -g '!**/*_.java'

Repository: MatheMatrix/zstack

Length of output: 156


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== File outline =="
ast-grep outline storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java --view expanded || true

echo
echo "== Relevant slice of ExternalPrimaryStorage.java =="
sed -n '150,230p' storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java | cat -n

echo
echo "== Search for ExternalPrimaryStorageHostRefVO usages =="
rg -n "ExternalPrimaryStorageHostRefVO|PrimaryStorageHostRefVO" storage/src/main/java -g '*.java' -C 2

echo
echo "== Search for KVM host selection in this addon =="
rg -n "connectedPsHostUuids|HostState.Enabled|VmInstanceConstant.KVM_HYPERVISOR_TYPE" storage/src/main/java/org/zstack/storage -g '*.java' -C 2

Repository: MatheMatrix/zstack

Length of output: 31282


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== UpdatePrimaryStorageHostStatusMsg handling in primary storage code =="
rg -n "UpdatePrimaryStorageHostStatusMsg|handle\\(UpdatePrimaryStorageHostStatusMsg" storage/src/main/java/org/zstack/storage -g '*.java' -C 4

echo
echo "== PrimaryStorageBase status update block =="
sed -n '540,600p' storage/src/main/java/org/zstack/storage/primary/PrimaryStorageBase.java | cat -n

echo
echo "== ExternalPrimaryStorage status update block =="
sed -n '330,360p' storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java | cat -n

echo
echo "== Find any explicit update of ExternalPrimaryStorageHostRefVO status =="
rg -n "ExternalPrimaryStorageHostRefVO_\\.status|set\\(ExternalPrimaryStorageHostRefVO_\\.status|PrimaryStorageHostStatus\\.Connected|PrimaryStorageHostStatus\\.Disconnected" storage/src/main/java/org/zstack/storage/addon/primary -g '*.java' -C 2

Repository: MatheMatrix/zstack

Length of output: 12133


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Encryption host selection call sites =="
rg -n "findConnectedKvmHostForEncryption|resolveEncryptionHostUuid|materializeAndSealVolumeDekForHost|prepareVolumeSpecEncryption|prepareImageCacheSpecEncryption" storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java -C 3

echo
echo "== External primary storage client / ZBS references =="
rg -n "ZBS|external primary storage client|client.*primary storage|encrypt.*host|sealVolumeDek|LUKS" storage/src/main/java/org/zstack/storage/addon/primary -g '*.java' -C 3

echo
echo "== PrimaryStorageHostRefVO queries in external storage code =="
rg -n "PrimaryStorageHostRefVO" storage/src/main/java/org/zstack/storage/addon/primary -g '*.java' -C 3

echo
echo "== ExternalPrimaryStorageHostRefVO purpose =="
sed -n '1,220p' storage/src/main/java/org/zstack/storage/addon/primary/ExternalHostIdGetter.java | cat -n

Repository: MatheMatrix/zstack

Length of output: 34918


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Volume encrypted secret helper references =="
rg -n "materializeAndSealVolumeDekForHost|volumeEncryptedSecretHelper|snapshotEncryptionHelper|prepareTemporarySnapshotImageEncryptedDek" storage/src/main/java -g '*.java' -C 3

echo
echo "== Host access path / encryption flow around external primary storage =="
sed -n '200,270p' storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java | cat -n

echo
echo "== Search for any check requiring connectedPsHostUuids non-empty =="
rg -n "connectedPsHostUuids.isEmpty|cannot find a connected KVM host attached to external primary storage|prepare LUKS secret" storage/src/main/java -g '*.java' -C 3

Repository: MatheMatrix/zstack

Length of output: 41880


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== VolumeManagerImpl host ref logic around empty connected refs =="
sed -n '740,770p' storage/src/main/java/org/zstack/storage/volume/VolumeManagerImpl.java | cat -n

echo
echo "== PrimaryStorageMainAllocatorFlow host ref logic =="
sed -n '55,85p' storage/src/main/java/org/zstack/storage/primary/PrimaryStorageMainAllocatorFlow.java | cat -n

echo
echo "== External primary storage docs/comments mentioning host refs or connected status =="
rg -n "connectedHostUuids|PrimaryStorageHostStatus|ExternalPrimaryStorageHostRefVO|primary storage host ref|external primary storage" storage/src/main/java/org/zstack/storage -g '*.java' -C 2

Repository: MatheMatrix/zstack

Length of output: 34599


空的 connected host 列表不能直接放宽筛选。
这里会退化为仅按 cluster / KVM / Connected 选宿主;如果 connectedPsHostUuids 为空表示该外部主存储没有任何 Connected 的 host ref,就可能把 LUKS 相关操作派到未连通该主存储的宿主。建议这里直接返回失败,或区分“没有 ref”和“全离线”的情况。

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java`
around lines 176 - 198, The host selection logic in
ExternalPrimaryStorage.getConnectedHostUuid currently relaxes the filter when
connectedPsHostUuids is empty, which can allow picking a KVM host that is not
actually connected to the external primary storage. Update this method to
distinguish between “no connected PrimaryStorageHostRefVO exists” and “some refs
exist but none are currently connected,” and fail fast in the empty case instead
of removing the hostUuid constraint. Keep the existing query constraints on
clusterUuids, VmInstanceConstant.KVM_HYPERVISOR_TYPE, HostStatus.Connected, and
HostState.Enabled, but ensure the fallback does not broaden selection beyond
hosts truly attached to the storage.

@MatheMatrix MatheMatrix force-pushed the sync/zstackio/zbs-volume-encryption@@3 branch 12 times, most recently from d0db2d4 to 3085c2e Compare July 2, 2026 03:17

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (2)
plugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsStorageController.java (1)

662-694: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

清理失败被 NopeCompletion 静默吞掉,缺少日志。

失败分支中 doDeleteVolume(cmd.dstPath, true, new NopeCompletion()) 用于清理部分创建的加密卷,但若该清理本身失败,NopeCompletion 不会记录任何日志,可能导致 ZBS 上残留孤立的加密卷且无排查线索。建议至少在清理失败时打印警告日志。

♻️ 建议的修改(以第一处清理调用为例,其余两处同理)
                                     if (!reply.isSuccess()) {
-                                        doDeleteVolume(cmd.dstPath, true, new NopeCompletion());
+                                        doDeleteVolume(cmd.dstPath, true, new Completion(trigger) {
+                                            `@Override`
+                                            public void success() {}
+
+                                            `@Override`
+                                            public void fail(ErrorCode errorCode) {
+                                                logger.warn(String.format("failed to clean up dst volume[%s] after luks clone failure: %s",
+                                                        cmd.dstPath, errorCode));
+                                            }
+                                        });
                                         trigger.fail(reply.getError());
                                         return;
                                     }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@plugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsStorageController.java`
around lines 662 - 694, The cleanup path in ZbsStorageController’s KVM callback
is swallowing failures because doDeleteVolume(..., new NopeCompletion()) does
not emit any log if the delete itself fails. Update the failure branches in the
anonymous CloudBusCallBack.run handling KVMHostLuksRsp to use a completion that
logs a warning on cleanup failure (or wrap NopeCompletion with logging) before
calling trigger.fail, and apply the same treatment to each
doDeleteVolume(cmd.dstPath, true, ...) call in this block so partial encrypted
volumes are not left behind silently.
header/src/main/java/org/zstack/header/storage/addon/primary/CreateVolumeSpec.java (1)

10-12: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

建议为 encryptedDek 增加日志脱敏保护。

下游 ZbsStorageController.KVMHostLuksCloneCmd.encryptedDek 已用 @NoLogging 标注防止敏感信息写入日志,但此处 CreateVolumeSpec.encryptedDek 作为同一份密封 DEK 的载体,在被转发给 KVMHostLuksCloneCmd 之前若被框架的消息总线调试日志或 toString/JSON 序列化打印,仍存在泄露风险。建议保持两端一致性,为该字段加上等效的日志脱敏标注。

🔒 建议的修改
+import org.zstack.header.rest.NoLogging;
+
 public class CreateVolumeSpec {
     private String name;
     private String uuid;
     private long size;
     private VolumeQos qos;
     private boolean encrypted;
     private String hostUuid;
+    `@NoLogging`
     private String encryptedDek;

Also applies to: 81-87

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@header/src/main/java/org/zstack/header/storage/addon/primary/CreateVolumeSpec.java`
around lines 10 - 12, `CreateVolumeSpec.encryptedDek` currently carries
sensitive DEK data without any logging protection, unlike
`ZbsStorageController.KVMHostLuksCloneCmd.encryptedDek` which already uses
`@NoLogging`. Add the same log-redaction annotation to
`CreateVolumeSpec.encryptedDek` so framework debug logs, `toString`, or JSON
serialization do not expose the sealed key material before it is forwarded, and
keep the handling consistent with the downstream command field.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@plugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsStorageController.java`:
- Around line 643-654: The encrypted clone path in ZbsStorageController around
the KVMHostLuksCloneCmd setup only checks hostUuid and should also validate
dst.getEncryptedDek() before sending the request. Add a blank/null check for
encryptedDek in the same dst.isEncrypted() branch, fail early with a clear operr
message from the controller, and only populate cmd.encryptedDek after the value
is confirmed present.

---

Nitpick comments:
In
`@header/src/main/java/org/zstack/header/storage/addon/primary/CreateVolumeSpec.java`:
- Around line 10-12: `CreateVolumeSpec.encryptedDek` currently carries sensitive
DEK data without any logging protection, unlike
`ZbsStorageController.KVMHostLuksCloneCmd.encryptedDek` which already uses
`@NoLogging`. Add the same log-redaction annotation to
`CreateVolumeSpec.encryptedDek` so framework debug logs, `toString`, or JSON
serialization do not expose the sealed key material before it is forwarded, and
keep the handling consistent with the downstream command field.

In `@plugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsStorageController.java`:
- Around line 662-694: The cleanup path in ZbsStorageController’s KVM callback
is swallowing failures because doDeleteVolume(..., new NopeCompletion()) does
not emit any log if the delete itself fails. Update the failure branches in the
anonymous CloudBusCallBack.run handling KVMHostLuksRsp to use a completion that
logs a warning on cleanup failure (or wrap NopeCompletion with logging) before
calling trigger.fail, and apply the same treatment to each
doDeleteVolume(cmd.dstPath, true, ...) call in this block so partial encrypted
volumes are not left behind silently.
🪄 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: e11ca870-feb4-4fb6-afd0-52ed479f7a3d

📥 Commits

Reviewing files that changed from the base of the PR and between ebe439b and d0db2d4.

📒 Files selected for processing (3)
  • header/src/main/java/org/zstack/header/storage/addon/primary/CreateVolumeSpec.java
  • plugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsStorageController.java
  • storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java

Comment thread plugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsStorageController.java Outdated
@MatheMatrix MatheMatrix force-pushed the sync/zstackio/zbs-volume-encryption@@3 branch 15 times, most recently from d9afe36 to cb12c2a Compare July 2, 2026 09:20
@MatheMatrix MatheMatrix force-pushed the sync/zstackio/zbs-volume-encryption@@3 branch 9 times, most recently from 21be543 to 3903193 Compare July 3, 2026 03:10
@MatheMatrix

Copy link
Copy Markdown
Owner

Comment from yaohua.wu:

Review: MR !10327 — ZSV-12579

Background

  • Jira: ZSV-12579 — 【后端】【磁盘加密】ZBS适配
  • Bug summary: 该组 MR 为 ZBS 加密卷打通控制面与 agent 侧 LUKS 操作,覆盖 create/clone/copy snapshot/resize/in-place encrypt 与 VM attach 的 secret 注入。
  • Intent & scope: zstack 侧新增 ZBS encryption extension 和 ExternalPrimaryStorage/ZbsStorageController 分派;utility 侧实现对应 KVM agent endpoints。两个 MR 必须一起合入。
  • Round 1 initial findings: 1 × Critical, 1 × Warning,均在关联 utility MR;本 MR Java 控制面未发现 path/字段契约错配。
  • Suggested fix direction: 先修 utility 侧 CBD disk XML 的重复 encryption 注入,并在已有 LUKS bits 的幂等路径验证 sealed DEK。

关联 MR

MR 关系
zstackio/zstack-utility !7377 提供本 MR 调用的 KVM agent LUKS endpoints 和 VM XML CBD LUKS secret 注入;当前阻塞问题在该 MR。

Cross-Repo Findings

Severity File:Line Issue Fix
🔴 Critical zstack-utility!7377 kvmagent/kvmagent/plugins/vm_plugin.py:8910 zstack 侧会为 encrypted ZBS volume 提供 luksSecretUuid,utility 侧 _get_new_disk() 的 CBD 分支在 8856 已写一次 <encryption>,函数尾部又在 8910 无条件再写一次。迁移/块拷贝/磁盘 XML 重建路径会生成重复 LUKS 节点,导致关联功能在运行时失败。 在 utility MR 中跳过 CBD 的尾部公共 _add_luks_encryption,或改为单一入口,确保 CBD 只写一次且保持 allow_legacy_secret=False
🟡 Warning zstack-utility!7377 kvmagent/kvmagent/plugins/zbs_storage_plugin.py:284 utility 侧 _encrypt_luks_in_place() 遇到已有 LUKS header 时只看 qemu-img infofile format: luks,没有用本次 zstack 下发的 encryptedDek 验证密钥可打开;重试或已有 LUKS bits 但 key binding 不一致时,zstack 侧可能把 volume 标记为 encrypted,后续 VM attach 才失败。 已是 LUKS 的幂等分支也应使用本次 sealed DEK 执行 qemu-img info --image-opts driver=luks... 验证;验证失败时返回 error。

zstack-side Review

Area Result
Controller/agent contract ZbsVolumeEncryptionConstants 的 path 与 utility ZbsStoragePlugin 注册路径一致;command/reply 字段名对齐。
ExternalPrimaryStorage routing create/clone/copy/resize/in-place encrypt 均能分派到 ZbsVolumeEncryptionExtensionPoint;Expon 已补默认 unsupported 实现。
Remaining risk 当前 MR 没有 Java focused test 文件出现在最终 diff;建议补充 controller 层单测覆盖 encrypted create/clone/copy/resize/in-place 分派和 utility endpoint 字段序列化。

Coverage

  • Diff source: local git diff 45e799eca5f9ee49ee064ba01dce35d613bff549..39031938b685fe8827a41ff342d818833f139fe5,22 files / 1219 diff lines;GitLab changed-files 列表一致。
  • Upstream freshness: HEAD..upstream/feature-zsv-5.1.0-encryption 无新增提交;GitLab merge_status=can_be_merged
  • Verification: zstack 与 utility git diff --check 均通过;utility 新增 Python 测试直接执行通过。未运行 zstack Java 编译/单测(当前 diff 中未找到 MR 描述所说的 ExternalPrimaryStorageEncryptionSpecTest 文件)。

Verdict: REVISION_REQUIRED

zstack 侧控制面可以和 utility endpoint 对齐,但该组 MR 仍需先修复 utility 侧的 CBD XML 重复 encryption 注入后再合入。


🤖 Robot Reviewer

@MatheMatrix MatheMatrix force-pushed the sync/zstackio/zbs-volume-encryption@@3 branch 18 times, most recently from 6e6eb2e to af93180 Compare July 7, 2026 08:00
Add ZBS encrypted volume handling for create, clone, snapshot
restore, template consumption, resize, lifecycle, trash restore,
image change, reimage, and VM clone flows.

Move ZBS-specific orchestration into storage encrypt extensions
while keeping the ZBS controller as a dispatcher.

Tests: git diff --check.

Resolves: ZSV-12579

Change-Id: I2676f28a78f00a85c85847494703ba0eb4708547
@MatheMatrix MatheMatrix force-pushed the sync/zstackio/zbs-volume-encryption@@3 branch from af93180 to 001b74a Compare July 7, 2026 08:55
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