add local volume encryption (LUKS) creation path#3911
Open
MatheMatrix wants to merge 1 commit into
Open
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:
Walkthrough新增 VolumeLuksAgentSpec，向 InstantiateVolumeMsg/InstantiateVolumeOnPrimaryStorageMsg 传递该 spec，扩展本地 KVM 后端的代理命令以包含 LUKS 字段，并在相关空卷/从缓存创建/根卷创建与快照路径上应用该 spec；另增一脚本用于同时提交并推送多个仓库。 变更内容LUKS 加密参数数据模型与消息传播
后端接口与实现更新
工厂与流程集成
Sequence Diagram(s)sequenceDiagram
participant Client as API / 调用方
participant VolumeSvc as Volume 服务
participant Factory as LocalStorageFactory / KvmFactory
participant Backend as LocalStorageKvmBackend
participant KvmAgent as kvmagent
Client->>VolumeSvc: InstantiateVolumeMsg (含 volumeLuksAgentSpec)
VolumeSvc->>Factory: 构建 InstantiateVolumeOnPrimaryStorageMsg (转发 spec)
Factory->>Backend: 调用 createEmptyVolume(..., backingFile, volumeLuksAgentSpec, ...)
Backend->>KvmAgent: 发送 CreateEmptyVolumeCmd / CreateVolumeFromCacheCmd (包含 encrypt* 字段)
KvmAgent->>Host: 在主机上创建或初始化加密卷
估算代码审查工作量🎯 3 (Moderate) | ⏱️ ~25 分钟 诗歌
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 2
🧹 Nitpick comments (1)
plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmBackend.java (1)
1448-1453: ⚡ Quick win抽取统一的 LUKS 参数注入逻辑，避免多处重复漂移
同一段“读取 + 判空 + 赋值”已在 4 条创建路径重复，后续很容易出现某一路径漏改。建议抽成一个私有 helper（例如
applyLuksParams(volumeUuid, cmd)）统一处理。Also applies to: 1874-1879, 2350-2355, 2387-2392
🤖 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/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmBackend.java` around lines 1448 - 1453, Extract the repeated "read + null check + set" LUKS block into a private helper (e.g., applyLuksParams(String volumeUuid, /* appropriate cmd type */ cmd)) that calls LuksKvmVolumeAddon.readVolumeLuksAgentParams(volumeUuid) and, if non-null and complete, sets cmd.setEncryptFormat(...), cmd.setEncryptPassphraseBase64(...), and cmd.setEncryptSecretUuid(...); then replace the duplicated blocks (the ones using LuksKvmVolumeAddon.VolumeLuksAgentParams, readVolumeLuksAgentParams, volume.getUuid() and cmd setters) in all create/prepare paths in LocalStorageKvmBackend with a call to this helper to ensure a single source of truth.
🤖 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/kvm/src/main/java/org/zstack/kvm/KVMSystemTags.java`:
- Around line 65-71: The VOLUME_ENCRYPT_PASSPHRASE_BASE64 SystemTag in
KVMSystemTags stores reversible secret material
(VOLUME_ENCRYPT_PASSPHRASE_BASE64_TOKEN / VOLUME_ENCRYPT_PASSPHRASE_BASE64 tied
to VolumeVO); remove or stop using this tag to hold raw passphrases and instead
persist only the encryptSecretUuid (VOLUME_ENCRYPT_SECRET_UUID) on VolumeVO;
update callers to retrieve/decrypt keys at runtime via the
EncryptedResourceKeyManager / KMS when performing hypervisor operations, or if a
tag must be used ensure the platform encrypts the tag value server-side and
enforces short TTL and strict access controls.
In `@plugin/kvm/src/main/java/org/zstack/kvm/LuksKvmVolumeAddon.java`:
- Around line 24-26: isComplete() in LuksKvmVolumeAddon currently treats empty
strings as valid and returns false (allowing silent downgrade to unencrypted
volumes); change the validation to treat values as non-blank (not null, not
empty, not only whitespace) for encryptSecretUuid and encryptPassphraseBase64,
and instead of returning null or allowing creation when only one of the two is
present, throw an explicit exception to fail-fast; update the same logic used
around the other occurrence (lines noted 49-51) so that a single-present
secretUuid or passphraseBase64 causes an error, while both non-blank values
allow normal processing.
---
Nitpick comments:
In
`@plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmBackend.java`:
- Around line 1448-1453: Extract the repeated "read + null check + set" LUKS
block into a private helper (e.g., applyLuksParams(String volumeUuid, /*
appropriate cmd type */ cmd)) that calls
LuksKvmVolumeAddon.readVolumeLuksAgentParams(volumeUuid) and, if non-null and
complete, sets cmd.setEncryptFormat(...), cmd.setEncryptPassphraseBase64(...),
and cmd.setEncryptSecretUuid(...); then replace the duplicated blocks (the ones
using LuksKvmVolumeAddon.VolumeLuksAgentParams, readVolumeLuksAgentParams,
volume.getUuid() and cmd setters) in all create/prepare paths in
LocalStorageKvmBackend with a call to this helper to ensure a single source of
truth.
🪄 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: 1a8be443-f700-4f18-962a-cd32b302b649
📒 Files selected for processing (3)
plugin/kvm/src/main/java/org/zstack/kvm/KVMSystemTags.javaplugin/kvm/src/main/java/org/zstack/kvm/LuksKvmVolumeAddon.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmBackend.java
Comment on lines
+65
to
+71
| /** | ||
| * Base64-encoded LUKS passphrase bytes for {@link #VOLUME_ENCRYPT_SECRET_UUID} (same resource). | ||
| * Prefer KMS / {@link org.zstack.header.keyprovider.EncryptedResourceKeyManager} long-term; this tag wires the hypervisor. | ||
| */ | ||
| public static final String VOLUME_ENCRYPT_PASSPHRASE_BASE64_TOKEN = "passphraseBase64"; | ||
| public static PatternedSystemTag VOLUME_ENCRYPT_PASSPHRASE_BASE64 = new PatternedSystemTag( | ||
| String.format("kvm::volume::encryptPassphraseBase64::{%s}", VOLUME_ENCRYPT_PASSPHRASE_BASE64_TOKEN), VolumeVO.class); |
There was a problem hiding this comment.
避免将可逆口令内容长期存放在 SystemTag 中
passphraseBase64 作为可逆敏感信息直接写入 VolumeVO 标签，会显著扩大密钥材料暴露面（读取链路、导出链路、排障链路）。建议改为仅保存 encryptSecretUuid，由专门密钥管理路径在执行时解密/下发；至少应对 tag 中口令做平台侧加密并限制生命周期。
🤖 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/kvm/src/main/java/org/zstack/kvm/KVMSystemTags.java` around lines 65 -
71, The VOLUME_ENCRYPT_PASSPHRASE_BASE64 SystemTag in KVMSystemTags stores
reversible secret material (VOLUME_ENCRYPT_PASSPHRASE_BASE64_TOKEN /
VOLUME_ENCRYPT_PASSPHRASE_BASE64 tied to VolumeVO); remove or stop using this
tag to hold raw passphrases and instead persist only the encryptSecretUuid
(VOLUME_ENCRYPT_SECRET_UUID) on VolumeVO; update callers to retrieve/decrypt
keys at runtime via the EncryptedResourceKeyManager / KMS when performing
hypervisor operations, or if a tag must be used ensure the platform encrypts the
tag value server-side and enforces short TTL and strict access controls.
Comment on lines
+24
to
+26
| public boolean isComplete() { | ||
| return encryptSecretUuid != null && encryptPassphraseBase64 != null; | ||
| } |
There was a problem hiding this comment.
对加密参数应 fail-closed，禁止静默退化为非加密创建
这里在 tag 不完整时返回 null，调用方会继续创建普通卷；同时 isComplete() 把空字符串也当作有效值。建议：
- 完整性校验改为“非空白”；
secretUuid与passphraseBase64只出现一个时直接抛错，阻断创建。
🔧 建议修复
public static final class VolumeLuksAgentParams {
@@
public boolean isComplete() {
- return encryptSecretUuid != null && encryptPassphraseBase64 != null;
+ return isNotBlank(encryptSecretUuid)
+ && isNotBlank(encryptPassphraseBase64)
+ && isNotBlank(encryptFormat);
}
@@
}
+private static boolean isNotBlank(String s) {
+ return s != null && !s.trim().isEmpty();
+}
+
public static VolumeLuksAgentParams readVolumeLuksAgentParams(String volumeUuid) {
@@
- if (sec == null || b64 == null) {
- return null;
- }
+ boolean secPresent = isNotBlank(sec);
+ boolean b64Present = isNotBlank(b64);
+ if (secPresent ^ b64Present) {
+ throw new IllegalStateException(String.format(
+ "incomplete luks tags for volume[uuid:%s]: secretUuid/passphraseBase64 must appear together", volumeUuid));
+ }
+ if (!secPresent) {
+ return null;
+ }
return new VolumeLuksAgentParams(sec, b64, "luks");
}Also applies to: 49-51
🤖 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/kvm/src/main/java/org/zstack/kvm/LuksKvmVolumeAddon.java` around lines
24 - 26, isComplete() in LuksKvmVolumeAddon currently treats empty strings as
valid and returns false (allowing silent downgrade to unencrypted volumes);
change the validation to treat values as non-blank (not null, not empty, not
only whitespace) for encryptSecretUuid and encryptPassphraseBase64, and instead
of returning null or allowing creation when only one of the two is present,
throw an explicit exception to fail-fast; update the same logic used around the
other occurrence (lines noted 49-51) so that a single-present secretUuid or
passphraseBase64 causes an error, while both non-blank values allow normal
processing.
MatheMatrix
force-pushed
the
sync/zstackio/encrypt-volume
branch
from
5d6f385 to
b9d0a36
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmFactory.java (1)
182-198:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift在线快照预创建路径把新的 LUKS 参数硬编码成了
null。
createEmptyVolumeWithBackingFile(...)已经支持VolumeLuksAgentSpec，但这里固定传null，导致本地存储的在线快照预创建始终不会带加密信息。对已经加密的 qcow2 来说，这会把新 overlay 变成明文层，backing chain 也会失配。请把卷的 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/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmFactory.java` around lines 182 - 198, The beforeTakeSnapshot method currently hardcodes the LUKS spec to null when calling LocalStorageHypervisorBackend.createEmptyVolumeWithBackingFile, so online pre-created overlays lose encryption info; fix it by extracting the volume's LUKS spec from the incoming message/VolumeInventory (use msg.getVolume() / inv) and pass that VolumeLuksAgentSpec into createEmptyVolumeWithBackingFile instead of null (keep other args the same), ensuring LocalStorageHypervisorBackend.createEmptyVolumeWithBackingFile receives the original encryption spec when cmd.isOnline() is true.
🤖 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 `@header/src/main/java/org/zstack/header/volume/InstantiateVolumeMsg.java`:
- Around line 71-72: The setter setVolumeLuksAgentSpec in InstantiateVolumeMsg
currently accepts partially-populated VolumeLuksAgentSpec objects; change it to
enforce an "all-or-none" constraint by validating the incoming
VolumeLuksAgentSpec: if it's null allow null, otherwise verify that all required
fields on VolumeLuksAgentSpec (e.g., encryptSecretUuid and any other mandatory
properties on VolumeLuksAgentSpec) are non-null/non-empty and consistent, and
throw an IllegalArgumentException with a clear message when validation fails;
implement this validation inside InstantiateVolumeMsg.setVolumeLuksAgentSpec
before assigning to this.volumeLuksAgentSpec.
In `@header/src/main/java/org/zstack/header/volume/VolumeLuksAgentSpec.java`:
- Around line 16-18: isComplete() treats whitespace-only strings as valid;
update VolumeLuksAgentSpec.isComplete() to consider encryptSecretUuid and
encryptPassphraseBase64 invalid when they are null or contain only whitespace by
trimming (or using a blank-check utility) before testing emptiness. Locate the
isComplete() method and replace the plain isEmpty() checks on encryptSecretUuid
and encryptPassphraseBase64 with checks that first trim() (or use
StringUtils.isBlank) and then verify non-empty, so inputs like " " are
rejected as incomplete.
In
`@plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmBackend.java`:
- Around line 1425-1441: The current applyVolumeLuksAgentSpec methods (for
CreateEmptyVolumeCmd and CreateVolumeFromCacheCmd) silently return when spec !=
null && !spec.isComplete(), which lets the volume creation proceed unencrypted;
change both methods to fail-fast by throwing a clear exception (e.g.,
IllegalArgumentException or custom exception) when a non-null spec is
incomplete, and only populate cmd.setEncrypt* fields when spec.isComplete() is
true; this ensures callers of applyVolumeLuksAgentSpec (CreateEmptyVolumeCmd and
CreateVolumeFromCacheCmd flows) cannot proceed to create a plaintext volume when
encryption was requested but the LUKS spec is invalid.
- Around line 387-389: The new encrypt* fields (encryptFormat,
encryptPassphraseBase64, encryptSecretUuid) were wired only for
CreateEmptyVolumeCmd/CreateVolumeFromCacheCmd, leaving
CREATE_VOLUME_WITH_BACKING_PATH and MERGE_SNAPSHOT_PATH flows unencrypted;
update LocalStorageKvmBackend so the same auxiliary logic that copies encrypt*
into KVM requests is applied to CreateVolumeWithBackingCmd and MergeSnapshotCmd
as well (or extend the helper to accept a common interface/shared cmd type), and
ensure the builders that create requests for CREATE_VOLUME_WITH_BACKING_PATH and
MERGE_SNAPSHOT_PATH set the
encryptFormat/encryptPassphraseBase64/encryptSecretUuid fields on the outgoing
KVM commands so snapshot-derived and temporary volumes get LUKS wiring
consistently.
---
Outside diff comments:
In
`@plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmFactory.java`:
- Around line 182-198: The beforeTakeSnapshot method currently hardcodes the
LUKS spec to null when calling
LocalStorageHypervisorBackend.createEmptyVolumeWithBackingFile, so online
pre-created overlays lose encryption info; fix it by extracting the volume's
LUKS spec from the incoming message/VolumeInventory (use msg.getVolume() / inv)
and pass that VolumeLuksAgentSpec into createEmptyVolumeWithBackingFile instead
of null (keep other args the same), ensuring
LocalStorageHypervisorBackend.createEmptyVolumeWithBackingFile receives the
original encryption spec when cmd.isOnline() is true.
🪄 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: a7fa7f4a-610c-487c-bec1-48e1be10e487
📒 Files selected for processing (8)
header/src/main/java/org/zstack/header/storage/primary/InstantiateVolumeOnPrimaryStorageMsg.javaheader/src/main/java/org/zstack/header/volume/InstantiateVolumeMsg.javaheader/src/main/java/org/zstack/header/volume/VolumeLuksAgentSpec.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageFactory.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageHypervisorBackend.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmBackend.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmFactory.javastorage/src/main/java/org/zstack/storage/volume/VolumeBase.java
Comment on lines
+71
to
+72
| public void setVolumeLuksAgentSpec(VolumeLuksAgentSpec volumeLuksAgentSpec) { | ||
| this.volumeLuksAgentSpec = volumeLuksAgentSpec; |
There was a problem hiding this comment.
setVolumeLuksAgentSpec 缺少“要么完整、要么为空”的约束
当前 setter 会接受半配置对象（例如只带 encryptSecretUuid），容易把错误延后到后端执行阶段才暴露。
建议修复
public void setVolumeLuksAgentSpec(VolumeLuksAgentSpec volumeLuksAgentSpec) {
+ if (volumeLuksAgentSpec != null && !volumeLuksAgentSpec.isComplete()) {
+ throw new IllegalArgumentException("volumeLuksAgentSpec must include both encryptSecretUuid and encryptPassphraseBase64");
+ }
this.volumeLuksAgentSpec = volumeLuksAgentSpec;
}🤖 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/volume/InstantiateVolumeMsg.java`
around lines 71 - 72, The setter setVolumeLuksAgentSpec in InstantiateVolumeMsg
currently accepts partially-populated VolumeLuksAgentSpec objects; change it to
enforce an "all-or-none" constraint by validating the incoming
VolumeLuksAgentSpec: if it's null allow null, otherwise verify that all required
fields on VolumeLuksAgentSpec (e.g., encryptSecretUuid and any other mandatory
properties on VolumeLuksAgentSpec) are non-null/non-empty and consistent, and
throw an IllegalArgumentException with a clear message when validation fails;
implement this validation inside InstantiateVolumeMsg.setVolumeLuksAgentSpec
before assigning to this.volumeLuksAgentSpec.
Comment on lines
+16
to
+18
| public boolean isComplete() { | ||
| return encryptSecretUuid != null && !encryptSecretUuid.isEmpty() | ||
| && encryptPassphraseBase64 != null && !encryptPassphraseBase64.isEmpty(); |
There was a problem hiding this comment.
isComplete() 会把纯空白字符串当成有效值
这里仅用 isEmpty()，像 " " 这样的输入会被当成“完整配置”，后续才在更深层失败，定位成本高。
建议修复
public boolean isComplete() {
- return encryptSecretUuid != null && !encryptSecretUuid.isEmpty()
- && encryptPassphraseBase64 != null && !encryptPassphraseBase64.isEmpty();
+ return encryptSecretUuid != null && !encryptSecretUuid.trim().isEmpty()
+ && encryptPassphraseBase64 != null && !encryptPassphraseBase64.trim().isEmpty();
}🤖 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/volume/VolumeLuksAgentSpec.java`
around lines 16 - 18, isComplete() treats whitespace-only strings as valid;
update VolumeLuksAgentSpec.isComplete() to consider encryptSecretUuid and
encryptPassphraseBase64 invalid when they are null or contain only whitespace by
trimming (or using a blank-check utility) before testing emptiness. Locate the
isComplete() method and replace the plain isEmpty() checks on encryptSecretUuid
and encryptPassphraseBase64 with checks that first trim() (or use
StringUtils.isBlank) and then verify non-empty, so inputs like " " are
rejected as incomplete.
Comment on lines
1425
to
1441
| private static void applyVolumeLuksAgentSpec(CreateEmptyVolumeCmd cmd, VolumeLuksAgentSpec spec) { | ||
| if (spec == null || !spec.isComplete()) { | ||
| return; | ||
| } | ||
| cmd.setEncryptFormat(spec.getEncryptFormat()); | ||
| cmd.setEncryptPassphraseBase64(spec.getEncryptPassphraseBase64()); | ||
| cmd.setEncryptSecretUuid(spec.getEncryptSecretUuid()); | ||
| } | ||
|
|
||
| private static void applyVolumeLuksAgentSpec(CreateVolumeFromCacheCmd cmd, VolumeLuksAgentSpec spec) { | ||
| if (spec == null || !spec.isComplete()) { | ||
| return; | ||
| } | ||
| cmd.setEncryptFormat(spec.getEncryptFormat()); | ||
| cmd.setEncryptPassphraseBase64(spec.getEncryptPassphraseBase64()); | ||
| cmd.setEncryptSecretUuid(spec.getEncryptSecretUuid()); | ||
| } |
There was a problem hiding this comment.
不要在 LUKS 规格不完整时静默回退到明文卷。
这里 spec != null && !spec.isComplete() 会直接 return，但后续创建流程还会继续发送一个不带任何 encrypt* 字段的命令。结果就是“请求了加密卷，实际却创建成明文 qcow2”。这类安全配置应该显式失败，而不是 fail-open。
🤖 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/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmBackend.java`
around lines 1425 - 1441, The current applyVolumeLuksAgentSpec methods (for
CreateEmptyVolumeCmd and CreateVolumeFromCacheCmd) silently return when spec !=
null && !spec.isComplete(), which lets the volume creation proceed unencrypted;
change both methods to fail-fast by throwing a clear exception (e.g.,
IllegalArgumentException or custom exception) when a non-null spec is
incomplete, and only populate cmd.setEncrypt* fields when spec.isComplete() is
true; this ensures callers of applyVolumeLuksAgentSpec (CreateEmptyVolumeCmd and
CreateVolumeFromCacheCmd flows) cannot proceed to create a plaintext volume when
encryption was requested but the LUKS spec is invalid.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@commit-push-three-repos.sh`:
- Line 92: 当前脚本在执行 git -C "${dir}" push --force-with-lease origin "${branch}"
时对任意分支都允许强推，需增加受保护分支防护：在执行 push 前用变量 branch 检查是否属于受保护列表/模式（例如
main、master、zsv_5.1.0 或 ^v\d+\.\d+），若匹配且未设置显式开关（例如环境变量 FORCE_PUSH=true 或脚本参数
--allow-force）则中止并提示用户；仅当 override 开启时才允许继续执行 git -C "${dir}" push
--force-with-lease origin "${branch}"，否则使用常规非强推方式或退出。
- Around line 73-75: 当前脚本使用 branch="$(git -C "${dir}" rev-parse --abbrev-ref
HEAD)" 会在 detached HEAD 时返回 "HEAD" 并导致后续 git push --force-with-lease origin HEAD
推送到错误目标；将该命令改为使用 git -C "${dir}" symbolic-ref --quiet --short HEAD
来安全获取分支名，检测该命令的返回/退出状态并在无法解析分支（detached HEAD 或错误）时记录并跳过该仓库，确保变量 branch
只在有有效分支名时被使用以免推送到远程的 HEAD 符号引用。
🪄 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: df0c8e05-da21-496f-99a4-34a084cab799
📒 Files selected for processing (2)
commit-push-three-repos.shplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmBackend.java
Comment on lines
+73
to
+75
| local branch | ||
| branch="$(git -C "${dir}" rev-parse --abbrev-ref HEAD)" | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
repos=("." "../zstack-utility" "./premium")
for r in "${repos[@]}"; do
[[ -d "$r" ]] || continue
if git -C "$r" rev-parse --git-dir >/dev/null 2>&1; then
b="$(git -C "$r" symbolic-ref --quiet --short HEAD || true)"
if [[ -z "$b" ]]; then
echo "$r: DETACHED_HEAD"
else
echo "$r: BRANCH=$b"
fi
fi
doneRepository: MatheMatrix/zstack
Length of output: 79
🏁 Script executed:
# Find and read the commit-push-three-repos.sh file
find . -name "commit-push-three-repos.sh" -type f | head -20Repository: MatheMatrix/zstack
Length of output: 91
🏁 Script executed:
cat -n ./commit-push-three-repos.shRepository: MatheMatrix/zstack
Length of output: 3243
🏁 Script executed:
# Verify what rev-parse --abbrev-ref HEAD returns in detached HEAD state
# and what happens with symbolic-ref
cd /tmp
rm -rf test-detached-head 2>/dev/null || true
mkdir test-detached-head
cd test-detached-head
git init
echo "test" > file.txt
git add file.txt
git commit -m "initial"
# Create a branch and commit
git checkout -b branch1
echo "branch1" > file.txt
git commit -am "branch1 commit"
# Go back and create detached HEAD
git checkout HEAD~1
echo "=== Testing rev-parse behavior in detached HEAD ==="
echo "Current HEAD state:"
git status
echo ""
echo "rev-parse --abbrev-ref HEAD:"
git rev-parse --abbrev-ref HEAD
echo ""
echo "symbolic-ref --quiet --short HEAD (with ||true):"
git symbolic-ref --quiet --short HEAD || true
echo ""
echo "What happens if we try to push to 'HEAD':"
echo "Command would be: git push --force-with-lease origin HEAD"
echo "(Not actually running push)"Repository: MatheMatrix/zstack
Length of output: 2198
🏁 Script executed:
# Set up git config and properly test detached HEAD behavior
git config --global user.email "test@example.com"
git config --global user.name "Test User"
cd /tmp
rm -rf test-detached-head 2>/dev/null || true
mkdir test-detached-head
cd test-detached-head
git init
echo "test" > file.txt
git add file.txt
git commit -m "initial"
# Create another commit
echo "test2" > file.txt
git commit -am "second"
# Now checkout the first commit to create detached HEAD
FIRST_COMMIT=$(git rev-list --max-parents=0 HEAD)
git checkout "$FIRST_COMMIT"
echo "=== In Detached HEAD State ==="
echo ""
echo "rev-parse --abbrev-ref HEAD returns:"
git rev-parse --abbrev-ref HEAD
echo ""
echo "symbolic-ref --quiet --short HEAD returns (exit code shown):"
git symbolic-ref --quiet --short HEAD
EXIT_CODE=$?
echo "Exit code: $EXIT_CODE"
echo ""
echo "=== What would the script do? ==="
branch="$(git rev-parse --abbrev-ref HEAD)"
echo "branch variable = '$branch'"
echo "Command executed: git push --force-with-lease origin '$branch'"
echo "This would push to 'origin/HEAD' which is problematic"Repository: MatheMatrix/zstack
Length of output: 1738
显式处理 detached HEAD，避免推送到无效目标。
第 74 行使用 rev-parse --abbrev-ref HEAD 在 detached HEAD 状态时会返回字符串 "HEAD"，导致第 92 行执行 git push --force-with-lease origin HEAD，将内容推送到远程的 HEAD 符号引用（通常指向默认分支）而非实际分支，最终推送失败或产生非预期行为。建议改用 symbolic-ref --quiet --short HEAD 并在获取失败时跳过该仓库。
建议修改
local branch
- branch="$(git -C "${dir}" rev-parse --abbrev-ref HEAD)"
+ branch="$(git -C "${dir}" symbolic-ref --quiet --short HEAD || true)"
+ if [[ -z "${branch}" ]]; then
+ echo "skip ${name}: detached HEAD (${dir})"
+ return 0
+ fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| local branch | |
| branch="$(git -C "${dir}" rev-parse --abbrev-ref HEAD)" | |
| local branch | |
| branch="$(git -C "${dir}" symbolic-ref --quiet --short HEAD || true)" | |
| if [[ -z "${branch}" ]]; then | |
| echo "skip ${name}: detached HEAD (${dir})" | |
| return 0 | |
| fi | |
🤖 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 `@commit-push-three-repos.sh` around lines 73 - 75, 当前脚本使用 branch="$(git -C
"${dir}" rev-parse --abbrev-ref HEAD)" 会在 detached HEAD 时返回 "HEAD" 并导致后续 git
push --force-with-lease origin HEAD 推送到错误目标；将该命令改为使用 git -C "${dir}"
symbolic-ref --quiet --short HEAD 来安全获取分支名，检测该命令的返回/退出状态并在无法解析分支（detached HEAD
或错误）时记录并跳过该仓库，确保变量 branch 只在有有效分支名时被使用以免推送到远程的 HEAD 符号引用。
| git -C "${dir}" commit -m "${COMMIT_MSG}" | ||
| fi | ||
|
|
||
| git -C "${dir}" push --force-with-lease origin "${branch}" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
protected_re='^(main|master|zsv_5\.1\.0)$'
repos=("." "../zstack-utility" "./premium")
for r in "${repos[@]}"; do
[[ -d "$r" ]] || continue
if git -C "$r" rev-parse --git-dir >/dev/null 2>&1; then
b="$(git -C "$r" symbolic-ref --quiet --short HEAD || true)"
[[ -z "$b" ]] && { echo "$r: DETACHED_HEAD"; continue; }
if [[ "$b" =~ $protected_re ]]; then
echo "$r: PROTECTED_BRANCH=$b"
else
echo "$r: SAFE_BRANCH=$b"
fi
fi
doneRepository: MatheMatrix/zstack
Length of output: 79
🏁 Script executed:
find . -name "commit-push-three-repos.sh" -type fRepository: MatheMatrix/zstack
Length of output: 91
🏁 Script executed:
cat -n commit-push-three-repos.sh | head -100Repository: MatheMatrix/zstack
Length of output: 3069
对 --force-with-lease 增加受保护分支防护。
Line 92 当前会对任意当前分支执行强推；如果误在 main/master/zsv_5.1.0 这类分支运行，存在高风险历史改写。建议默认阻止，显式开关才允许。
💡 建议修改
+ local protected_re='^(main|master|zsv_5\.1\.0)$'
+ if [[ "${FORCE_PROTECTED_BRANCH:-0}" != "1" && "${branch}" =~ ${protected_re} ]]; then
+ echo "skip ${name}: protected branch '${branch}' (set FORCE_PROTECTED_BRANCH=1 to override)"
+ return 0
+ fi
+
git -C "${dir}" push --force-with-lease origin "${branch}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| git -C "${dir}" push --force-with-lease origin "${branch}" | |
| local protected_re='^(main|master|zsv_5\.1\.0)$' | |
| if [[ "${FORCE_PROTECTED_BRANCH:-0}" != "1" && "${branch}" =~ ${protected_re} ]]; then | |
| echo "skip ${name}: protected branch '${branch}' (set FORCE_PROTECTED_BRANCH=1 to override)" | |
| return 0 | |
| fi | |
| git -C "${dir}" push --force-with-lease origin "${branch}" |
🤖 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 `@commit-push-three-repos.sh` at line 92, 当前脚本在执行 git -C "${dir}" push
--force-with-lease origin "${branch}" 时对任意分支都允许强推，需增加受保护分支防护：在执行 push 前用变量
branch 检查是否属于受保护列表/模式（例如 main、master、zsv_5.1.0 或 ^v\d+\.\d+），若匹配且未设置显式开关（例如环境变量
FORCE_PUSH=true 或脚本参数 --allow-force）则中止并提示用户；仅当 override 开启时才允许继续执行 git -C
"${dir}" push --force-with-lease origin "${branch}"，否则使用常规非强推方式或退出。
cb25a92 to
15cdad0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@header/src/main/java/org/zstack/header/volume/VolumeLuksAgentSpec.java`:
- Around line 37-39: getEncryptFormat() currently only checks encryptFormat !=
null and !encryptFormat.isEmpty(), so pure-whitespace values slip through and
get sent as an invalid format; change VolumeLuksAgentSpec.getEncryptFormat to
treat blank (whitespace-only) strings as empty like isComplete() does — e.g.,
trim the value or use a blank-check (StringUtils.isBlank or
encryptFormat.trim().isEmpty()) and return the default "luks" when encryptFormat
is null or blank.
In
`@plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmFactory.java`:
- Line 198: The call to bkd.createEmptyVolume(..., null, ...) drops LUKS config;
change it to pass the current volume's VolumeLuksAgentSpec so snapshot
pre-creation inherits encryption settings (i.e., replace the null argument with
the VolumeLuksAgentSpec from the current volume/inv, e.g.
inv.getVolumeLuksAgentSpec() or the equivalent accessor) when invoking
createEmptyVolume in LocalStorageKvmFactory.
🪄 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: 4deac0cc-3ad4-44f7-a996-619882d29a0c
📒 Files selected for processing (9)
commit-push-three-repos.shheader/src/main/java/org/zstack/header/storage/primary/InstantiateVolumeOnPrimaryStorageMsg.javaheader/src/main/java/org/zstack/header/volume/InstantiateVolumeMsg.javaheader/src/main/java/org/zstack/header/volume/VolumeLuksAgentSpec.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageFactory.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageHypervisorBackend.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmBackend.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmFactory.javastorage/src/main/java/org/zstack/storage/volume/VolumeBase.java
🚧 Files skipped from review as they are similar to previous changes (7)
- header/src/main/java/org/zstack/header/storage/primary/InstantiateVolumeOnPrimaryStorageMsg.java
- storage/src/main/java/org/zstack/storage/volume/VolumeBase.java
- plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageFactory.java
- plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageHypervisorBackend.java
- header/src/main/java/org/zstack/header/volume/InstantiateVolumeMsg.java
- commit-push-three-repos.sh
- plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmBackend.java
Comment on lines
+37
to
+39
| public String getEncryptFormat() { | ||
| return encryptFormat != null && !encryptFormat.isEmpty() ? encryptFormat : "luks"; | ||
| } |
There was a problem hiding this comment.
getEncryptFormat() 需要对纯空白值回退默认值
Line 37-39 对 encryptFormat 仅检查 isEmpty()；当值为纯空白时会直接下发无效格式，可能导致后续创建链路失败。建议和 isComplete() 一样做 blank-check。
建议修复
public String getEncryptFormat() {
- return encryptFormat != null && !encryptFormat.isEmpty() ? encryptFormat : "luks";
+ return encryptFormat != null && !encryptFormat.trim().isEmpty() ? encryptFormat : "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 `@header/src/main/java/org/zstack/header/volume/VolumeLuksAgentSpec.java`
around lines 37 - 39, getEncryptFormat() currently only checks encryptFormat !=
null and !encryptFormat.isEmpty(), so pure-whitespace values slip through and
get sent as an invalid format; change VolumeLuksAgentSpec.getEncryptFormat to
treat blank (whitespace-only) strings as empty like isComplete() does — e.g.,
trim the value or use a blank-check (StringUtils.isBlank or
encryptFormat.trim().isEmpty()) and return the default "luks" when encryptFormat
is null or blank.
| LocalStorageHypervisorBackend bkd = getHypervisorBackend(primaryStorageVO); | ||
| String backingFile = cmd.isOnline() ? cmd.getVolumeInstallPath() : null; | ||
| bkd.createEmptyVolumeWithBackingFile(inv, msg.getHostUuid(), backingFile, new ReturnValueCompletion<VolumeStats>(completion) { | ||
| bkd.createEmptyVolume(inv, msg.getHostUuid(), backingFile, null, new ReturnValueCompletion<VolumeStats>(completion) { |
There was a problem hiding this comment.
LUKS 参数在快照预创建路径被丢弃（Line 198）
这里把新增的加密参数直接传 null，会让在线快照预创建卷这条链路无法继承 LUKS 配置；加密卷场景下可能生成未按预期加密的快照数据。请改为透传当前卷对应的 VolumeLuksAgentSpec。
建议修改
- bkd.createEmptyVolume(inv, msg.getHostUuid(), backingFile, null, new ReturnValueCompletion<VolumeStats>(completion) {
+ VolumeLuksAgentSpec volumeLuksAgentSpec = /* 从 msg/cmd/volume addon 中获取当前卷的 LUKS 规格 */;
+ bkd.createEmptyVolume(inv, msg.getHostUuid(), backingFile, volumeLuksAgentSpec, new ReturnValueCompletion<VolumeStats>(completion) {🤖 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/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmFactory.java`
at line 198, The call to bkd.createEmptyVolume(..., null, ...) drops LUKS
config; change it to pass the current volume's VolumeLuksAgentSpec so snapshot
pre-creation inherits encryption settings (i.e., replace the null argument with
the VolumeLuksAgentSpec from the current volume/inv, e.g.
inv.getVolumeLuksAgentSpec() or the equivalent accessor) when invoking
createEmptyVolume in LocalStorageKvmFactory.
138cb5b to
95f2e2e
Compare
Keep KVM LUKS agent params on InstantiateVolumeMsg / InstantiateVolumeOnPrimaryStorageMsg (VolumeLuksAgentSpec), premium pre-instantiate DEK path, and LocalStorageKvmBackend wiring. Shared MergeSnapshotCmd encrypt fields removed; helper script commit-push-three-repos.sh at repo root. Co-authored-by: Cursor <cursoragent@cursor.com>
MatheMatrix
force-pushed
the
sync/zstackio/encrypt-volume
branch
from
95f2e2e to
46438d5
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
1 participant
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.
Source: filtered from enc-test branch (commit "enc test"), excluding
migration/timeout-fix changes. Includes:
KVMAgentCommands, VolumeTO, LuksKvmVolumeAddon, LuksVolumeKvmWireExtension)
CephPrimaryStorageBase, KVMCephVolumeTO, SftpExportCephVolumeToKvmHost*,
DownloadBitsFromKVMHostToPrimaryStorageMsg)
sync from gitlab !9796