<feature>[kvm]: add libvirt TLS config#3458
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
Walkthrough为 KVM 虚拟机迁移命令新增 TLS 支持与源主机管理 IP：添加全局配置 Changes
Sequence Diagram(s)sequenceDiagram
participant KVMHost as KVMHost
participant Config as KVMGlobalConfig
participant AgentCmd as KVMAgentCommands.MigrateVmCmd
participant Libvirt as Libvirt (remote)
KVMHost->>Config: read LIBVIRT_TLS_ENABLED
Config-->>KVMHost: boolean value
KVMHost->>AgentCmd: new MigrateVmCmd(...)\nsetUseTls(value)\nsetSrcHostManagementIp(srcHostMnIp)
KVMHost->>Libvirt: send migration request (includes useTls, srcHostManagementIp)
Libvirt-->>KVMHost: respond to migration request
预计代码审查工作量🎯 2 (简单) | ⏱️ ~12 分钟 诗
🚥 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 unit tests (beta)
📝 Coding Plan
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.41.1)plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.javaComment |
![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
🤖 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/KVMHost.java`:
- Line 3177: The call to
KVMGlobalConfig.LIBVIRT_TLS_ENABLED.value(Boolean.class) in KVMHost (affecting
cmd.setUseTls) bypasses cluster-scoped overrides; change it to fetch the
cluster-level value via the resource config facade (use
rcf.getResourceConfigValue) for the host's cluster (use the host/cluster UUID
available in KVMHost) and pass that Boolean into cmd.setUseTls, matching the
pattern used elsewhere in this class (e.g., where rcf.getResourceConfigValue is
used around lines ~3157, ~3931, ~1782).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
Run ID: 7bb792e6-a9b5-4822-b465-e9ee96620481
📒 Files selected for processing (3)
plugin/kvm/src/main/java/org/zstack/kvm/KVMAgentCommands.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMGlobalConfig.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java
| cmd.setDownTime(s.downTime); | ||
| cmd.setBandwidth(s.bandwidth); | ||
| cmd.setNics(nicTos); | ||
| cmd.setUseTls(KVMGlobalConfig.LIBVIRT_TLS_ENABLED.value(Boolean.class)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Inspect LIBVIRT_TLS_ENABLED declaration =="
fd 'KVMGlobalConfig.java$' | while read -r f; do
echo "-- $f --"
rg -n -C4 'LIBVIRT_TLS_ENABLED|ClusterVO|bind' "$f" || true
done
echo
echo "== Inspect current use in KVMHost =="
fd 'KVMHost.java$' | while read -r f; do
echo "-- $f --"
rg -n -C3 'setUseTls|LIBVIRT_TLS_ENABLED' "$f" || true
done
echo
echo "== Compare with resource-scoped KVM config reads in KVMHost =="
fd 'KVMHost.java$' | while read -r f; do
echo "-- $f --"
rg -n -C1 'rcf\.getResourceConfigValue\(KVMGlobalConfig\.' "$f" || true
doneRepository: MatheMatrix/zstack
Length of output: 8576
配置作用域错误，绕过了集群级别的 TLS 设置
LIBVIRT_TLS_ENABLED 在 KVMGlobalConfig.java 中被标注为 @BindResourceConfig({ClusterVO.class})，是集群级资源配置。但第 3177 行直接使用 .value(Boolean.class) 读取全局默认值，绕过了集群维度的覆盖值。这导致某集群即使显式配置禁用 TLS，迁移命令仍会沿用默认值，新增的集群级开关失效，也违反了向后兼容原则。
应按照文件中其他资源配置的读取方式（如第 3157 行、3931 行、1782 行等），通过 rcf.getResourceConfigValue() 按 cluster 读取资源配置。
🔧 建议修改
- cmd.setUseTls(KVMGlobalConfig.LIBVIRT_TLS_ENABLED.value(Boolean.class));
+ cmd.setUseTls(rcf.getResourceConfigValue(
+ KVMGlobalConfig.LIBVIRT_TLS_ENABLED,
+ self.getClusterUuid(),
+ Boolean.class
+ ));🤖 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/KVMHost.java` at line 3177, The call
to KVMGlobalConfig.LIBVIRT_TLS_ENABLED.value(Boolean.class) in KVMHost
(affecting cmd.setUseTls) bypasses cluster-scoped overrides; change it to fetch
the cluster-level value via the resource config facade (use
rcf.getResourceConfigValue) for the host's cluster (use the host/cluster UUID
available in KVMHost) and pass that Boolean into cmd.setUseTls, matching the
pattern used elsewhere in this class (e.g., where rcf.getResourceConfigValue is
used around lines ~3157, ~3931, ~1782).
Add libvirt.tls.enabled GlobalConfig and useTls field in MigrateVmCmd to support TLS-encrypted libvirt connections for migration and V2V. Resolves: ZSTAC-81343 Change-Id: I391fa36c0dd63c25c5d85d102bc3579c8eb3d685
MatheMatrix
force-pushed
the
sync/yingzhe.hu/ZSTAC-81343@@2
branch
from
69e99d0 to
5e7f24f
Compare
|
Comment from yaohua.wu: Review: MR !9317 — ZSTAC-81343 (libvirt TLS 配置 - Java 侧)关联 MR: zstack-utility !6727 | premium !13134 正面评价
Warning
Suggestion
Verdict: APPROVEDJava 侧改动简洁正确，架构决策合理（全局配置 vs 集群级配置的选择正确）。Warning #1 建议确认但不阻塞合并。 🤖 Robot Reviewer |
2 participants
Add libvirt.tls.enabled GlobalConfig and useTls field in MigrateVmCmd to support TLS-encrypted libvirt connections for migration and V2V.
Resolves: ZSTAC-81343
Change-Id: I391fa36c0dd63c25c5d85d102bc3579c8eb3d685
sync from gitlab !9317