<fix>[kvm]: backport TLS reconnect PID fix#4465
Closed
MatheMatrix wants to merge 8 commits into
Closed
Conversation
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 (cherry picked from commit 3feb9e9)
Resolves: ZSTAC-83696 Change-Id: I368cc5af8fb3d553bedc3be5d031015719e68ddc (cherry picked from commit c85fab3)
…TTP for TLS cert update Resolves: ZSTAC-83696 Change-Id: I4a5f404e51f12487b61bdf8f990bbc490cafeeee (cherry picked from commit 659c296)
… redeploy Resolves: ZSTAC-84446 Change-Id: I8af5e3887a5bad286b43dda00c874c9de999e1cb (cherry picked from commit 2697ef2)
Resolves: ZSTAC-84446 Change-Id: I9bed31c0cefddd6ed11f59cd13e36eb1c2abc029 (cherry picked from commit ad8a4c5)
br_conn_all_ns (169.254.64.1, mevoco.py CONNECT_ALL_NETNS_BR_OUTER_IP) is created on the host only when the first flat L3 network lands, AFTER the add-host ansible playbook has generated the TLS cert. The cert SAN therefore lacks this IP, and on every subsequent reconnect the check-tls-certs flow detects the gap and triggers an ansible redeploy plus libvirtd/kvmagent restart, breaking PID stability. This IP is host-internal (used only for VM <-> root-netns userdata / pushgateway / kvmagent HTTP) and has no business in a libvirtd TLS cert. Add EXCLUDED_INTERNAL_IPS as a single source of truth used by both the check flow (buildIpList) and the deploy flow (unionTlsCertIps) so the two flows can never disagree, regardless of when the bridge appears. host_plugin.fact() on the agent side is unchanged. Resolves: ZSTAC-84446 Change-Id: I10821d8c68190f2cbc8a0679d19ed053916d9184 (cherry picked from commit a544065)
Adjust the TLS migration command gray version for the 5.4.10 backport so the new fields are sent to 5.4.x agents. Resolves: ZSTAC-84446 Change-Id: I0ca3210bd735649bf9d6b8e38e5ddee4f4cd63cb
Normalize the cherry-picked Groovy test to LF so git diff --check passes before creating the 4.8.38 backport MR. Resolves: ZSTAC-81344 Change-Id: I2ba5c110e21cf613c209a9ac49883b843fc26514
|
Warning
|
| Layer / File(s) | Summary |
|---|---|
迁移命令与部署参数TLS字段 KVMAgentCommands.java, KVMGlobalConfig.java, KVMHostDeployArguments.java, KVMConstant.java |
新增MigrateVmCmd的useTls/srcHostManagementIp字段及访问器，新增LIBVIRT_TLS_ENABLED全局配置，新增KVMHostDeployArguments的tlsCertIps字段及访问器。 |
主机IP采集与合并工具方法 KVMHostUtils.java |
新增EXCLUDED_INTERNAL_IPS常量、collectHostIps、unionTlsCertIps、unionIps、filterIps、buildIpList、shouldForceTlsRedeploy及newSsh方法。 |
连接流程TLS证书检测与部署装配 KVMHost.java |
连接流程新增SAN证书检测步骤，ansible部署阶段设置tlsCertIps与强制重跑标记，迁移命令设置useTls与源主机管理IP，新增parseSanIps工具方法。 |
Libvirt TLS CA初始化 KVMHostFactory.java |
start()流程新增initLibvirtTlsCA()，生成/读取本地CA证书与私钥并通过JsonLabel持久化到数据库。 |
单元测试与集成测试 LibvirtTlsMigrateCase.groovy, KVMHostUtilsTest.java |
新增集成测试验证TLS开关与迁移命令传播、全局配置校验、SAN解析；新增单元测试覆盖buildIpList、unionIps与shouldForceTlsRedeploy的多种场景。 |
Estimated code review effort: 4 (Complex) | ~60 minutes
Sequence Diagram(s)
sequenceDiagram
participant KVMHost
participant SrcHost as 源宿主机(SSH)
participant KVMHostUtils
participant DeployArguments as KVMHostDeployArguments
participant MigrateVmCmd
KVMHost->>SrcHost: openssl x509 读取servercert.pem SAN
SrcHost-->>KVMHost: subjectAltName输出
KVMHost->>KVMHost: parseSanIps(输出)
KVMHost->>KVMHostUtils: collectHostIps(hostUuid, managementIp)
KVMHostUtils-->>KVMHost: 宿主IP列表
KVMHost->>KVMHost: 比对SAN与宿主IP,标记NEED_DEPLOY_TLS_CERT/TLS_DETECTED_IPS
KVMHost->>KVMHostUtils: unionTlsCertIps(检测IP,管理IP)
KVMHostUtils-->>KVMHost: tlsCertIps
KVMHost->>DeployArguments: setTlsCertIps(tlsCertIps)
KVMHost->>KVMHostUtils: shouldForceTlsRedeploy(...)
KVMHostUtils-->>KVMHost: 是否强制重跑
KVMHost->>MigrateVmCmd: setUseTls(...), setSrcHostManagementIp(...)
Poem
兔子敲键盘，证书亮闪闪，
TLS护迁移，IP排排站，
SAN比对不慌乱，CA落库真周全，
测试用例齐上阵，代码稳稳向前跳～ 🐇🔐
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | 标题准确概括了本次 KVM TLS reconnect PID 修复回移的核心变更。 |
| Description check | ✅ Passed | 描述与变更集一致，明确说明了回移目标、TLS 证书处理和验证步骤。 |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
| 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 docstrings
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
sync/yingzhe.hu/cherry-pick/ZSTAC-84446-5.4.10-full
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.44.1)
plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java
ast-grep timed out on this file
plugin/kvm/src/main/java/org/zstack/kvm/KVMHostDeployArguments.java
ast-grep retry budget exhausted before isolating this batch
plugin/kvm/src/main/java/org/zstack/kvm/KVMHostFactory.java
ast-grep retry budget exhausted before isolating this batch
- 2 others
Comment @coderabbitai help to get the list of available commands.
![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
🧹 Nitpick comments (3)
test/src/test/java/org/zstack/test/kvm/KVMHostUtilsTest.java (2)
89-99: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
zstac84446_checkAndFirstDeployUseSameSource未真正验证 check/deploy 一致性。该测试两次调用同一个
buildIpList方法且参数完全相同，属于同义反复（tautology），必然相等，并未验证 PR 目标中 "check-flow 与 deploy-flow IP 列表保持一致" 的真实场景（即buildIpList的检测结果与unionIps的部署结果是否一致）。建议改为分别调用buildIpList（模拟 check 流程）与unionIps（模拟 deploy 流程，将buildIpList结果作为detectedIpsCsv传入）并断言两者等价，才能覆盖这条关键的一致性契约。♻️ 建议的修复方向
- String firstDeploy = KVMHostUtils.buildIpList("172.24.250.175", out, null); - String onCheck = KVMHostUtils.buildIpList("172.24.250.175", out, null); - Assert.assertEquals(firstDeploy, onCheck); + String onCheck = KVMHostUtils.buildIpList("172.24.250.175", out, null); + String onDeploy = KVMHostUtils.unionIps(onCheck, "172.24.250.175", null, null, + KVMHostUtils.EXCLUDED_INTERNAL_IPS); + Assert.assertEquals(onCheck, onDeploy);🤖 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 `@test/src/test/java/org/zstack/test/kvm/KVMHostUtilsTest.java` around lines 89 - 99, zstac84446_checkAndFirstDeployUseSameSource is currently a tautology because it calls buildIpList twice with identical inputs, so it does not verify check-flow versus deploy-flow consistency. Update the test to compare the check result from KVMHostUtils.buildIpList against the deploy result produced by KVMHostUtils.unionIps, passing the detected IP list from buildIpList into unionIps as the detectedIpsCsv input, and assert the two outputs are equivalent. Keep the existing assertions that filter expected and excluded IPs, but make sure the test exercises both buildIpList and unionIps to validate the real contract.
28-206: 📐 Maintainability & Code Quality | 🔵 Trivial测试方法命名未遵循 lowerCamelCase 规范。
新增的多个测试方法（如
filtersZsSuffixIface_consistentWithHostFact、zstac84446_checkAndFirstDeployUseSameSource、unionIps_detectedIsBaseAndExtraAppended、shouldForceTlsRedeploy_noNeedNeverForces等）使用下划线分隔而非纯 lowerCamelCase。该文件中已有的旧方法（如zstac86349_...）也是类似风格，属于沿用既有约定，非本次新引入的孤立问题，故仅作为可选建议。As per path instructions, "方法名、参数名、成员变量和局部变量：使用 lowerCamelCase 风格。"
🤖 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 `@test/src/test/java/org/zstack/test/kvm/KVMHostUtilsTest.java` around lines 28 - 206, The newly added test methods in KVMHostUtilsTest use underscore-separated names instead of lowerCamelCase. Rename the affected tests to follow the project naming convention, updating the method identifiers such as filtersZsSuffixIface_consistentWithHostFact, zstac84446_checkAndFirstDeployUseSameSource, unionIps_detectedIsBaseAndExtraAppended, and shouldForceTlsRedeploy_noNeedNeverForces to pure lowerCamelCase while keeping the test behavior unchanged.Source: Path instructions
plugin/kvm/src/main/java/org/zstack/kvm/KVMHostFactory.java (1)
497-506: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value使用已弃用的
FileUtils无 charset 重载。
FileUtils.readFileToString(File)与FileUtils.writeStringToFile(File, String)在 commons-io 中均已弃用，且依赖平台默认字符集，跨环境可能产生编码不确定性。建议显式传入StandardCharsets.UTF_8。♻️ 建议修改
- String ca = FileUtils.readFileToString(caFile).trim(); - String key = FileUtils.readFileToString(keyFile).trim(); + String ca = FileUtils.readFileToString(caFile, StandardCharsets.UTF_8).trim(); + String key = FileUtils.readFileToString(keyFile, StandardCharsets.UTF_8).trim(); // createIfAbsent: DB has no record -> write; DB has record -> return DB value JsonLabelInventory caInv = new JsonLabel().createIfAbsent(LIBVIRT_TLS_CA_KEY, ca); JsonLabelInventory keyInv = new JsonLabel().createIfAbsent(LIBVIRT_TLS_PRIVATE_KEY, key); // Use DB as source of truth - overwrite local files (HA: MN2 uses MN1's CA from DB) - FileUtils.writeStringToFile(caFile, caInv.getLabelValue()); - FileUtils.writeStringToFile(keyFile, keyInv.getLabelValue()); + FileUtils.writeStringToFile(caFile, caInv.getLabelValue(), StandardCharsets.UTF_8); + FileUtils.writeStringToFile(keyFile, keyInv.getLabelValue(), StandardCharsets.UTF_8);🤖 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/KVMHostFactory.java` around lines 497 - 506, The KVMHostFactory TLS file read/write block uses deprecated FileUtils overloads that rely on the platform default charset, so update the calls around readFileToString and writeStringToFile to use an explicit UTF-8 charset. Keep the existing createIfAbsent flow and DB-as-source-of-truth logic intact, but ensure the caFile and keyFile handling is done through the charset-aware FileUtils APIs so encoding is deterministic across environments.
🤖 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/KVMHostFactory.java`:
- Around line 497-506: The KVMHostFactory TLS sync logic is persisting the CA
private key in plain text through JsonLabel.createIfAbsent() and
JsonLabelVO.labelValue; update this flow to avoid storing the raw key directly.
In the block that reads cakey.pem and writes via LIBVIRT_TLS_PRIVATE_KEY, switch
to encrypted storage or a controlled secret-distribution mechanism, and only
write/decrypt the key when restoring local files. Ensure the handling around
createIfAbsent(), caInv, and keyInv no longer exposes the private key in the
database.
---
Nitpick comments:
In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMHostFactory.java`:
- Around line 497-506: The KVMHostFactory TLS file read/write block uses
deprecated FileUtils overloads that rely on the platform default charset, so
update the calls around readFileToString and writeStringToFile to use an
explicit UTF-8 charset. Keep the existing createIfAbsent flow and
DB-as-source-of-truth logic intact, but ensure the caFile and keyFile handling
is done through the charset-aware FileUtils APIs so encoding is deterministic
across environments.
In `@test/src/test/java/org/zstack/test/kvm/KVMHostUtilsTest.java`:
- Around line 89-99: zstac84446_checkAndFirstDeployUseSameSource is currently a
tautology because it calls buildIpList twice with identical inputs, so it does
not verify check-flow versus deploy-flow consistency. Update the test to compare
the check result from KVMHostUtils.buildIpList against the deploy result
produced by KVMHostUtils.unionIps, passing the detected IP list from buildIpList
into unionIps as the detectedIpsCsv input, and assert the two outputs are
equivalent. Keep the existing assertions that filter expected and excluded IPs,
but make sure the test exercises both buildIpList and unionIps to validate the
real contract.
- Around line 28-206: The newly added test methods in KVMHostUtilsTest use
underscore-separated names instead of lowerCamelCase. Rename the affected tests
to follow the project naming convention, updating the method identifiers such as
filtersZsSuffixIface_consistentWithHostFact,
zstac84446_checkAndFirstDeployUseSameSource,
unionIps_detectedIsBaseAndExtraAppended, and
shouldForceTlsRedeploy_noNeedNeverForces to pure lowerCamelCase while keeping
the test behavior unchanged.
🪄 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: d8853faf-bea5-4e1a-93ad-592b64d1489f
📒 Files selected for processing (9)
plugin/kvm/src/main/java/org/zstack/kvm/KVMAgentCommands.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMConstant.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMGlobalConfig.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMHost.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMHostDeployArguments.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMHostFactory.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMHostUtils.javatest/src/test/groovy/org/zstack/test/integration/kvm/vm/migrate/LibvirtTlsMigrateCase.groovytest/src/test/java/org/zstack/test/kvm/KVMHostUtilsTest.java
Comment on lines
+497
to
+506
| String ca = FileUtils.readFileToString(caFile).trim(); | ||
| String key = FileUtils.readFileToString(keyFile).trim(); | ||
|
|
||
| // createIfAbsent: DB has no record -> write; DB has record -> return DB value | ||
| JsonLabelInventory caInv = new JsonLabel().createIfAbsent(LIBVIRT_TLS_CA_KEY, ca); | ||
| JsonLabelInventory keyInv = new JsonLabel().createIfAbsent(LIBVIRT_TLS_PRIVATE_KEY, key); | ||
|
|
||
| // Use DB as source of truth - overwrite local files (HA: MN2 uses MN1's CA from DB) | ||
| FileUtils.writeStringToFile(caFile, caInv.getLabelValue()); | ||
| FileUtils.writeStringToFile(keyFile, keyInv.getLabelValue()); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant factory code and nearby context
sed -n '470,540p' plugin/kvm/src/main/java/org/zstack/kvm/KVMHostFactory.java
# Find JsonLabel usage and implementation
rg -n "class JsonLabel|createIfAbsent\(|LIBVIRT_TLS_PRIVATE_KEY|LIBVIRT_TLS_CA_KEY|JsonLabel" -S .
# Inspect the JsonLabel class and related storage paths if present
files=$(git ls-files | rg 'JsonLabel|jsonlabel|Label')
printf '%s\n' "$files"Repository: MatheMatrix/zstack
Length of output: 20622
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect JsonLabel storage semantics
sed -n '1,180p' core/src/main/java/org/zstack/core/jsonlabel/JsonLabel.java
# Inspect JsonLabelVO persistence fields
sed -n '1,140p' core/src/main/java/org/zstack/core/jsonlabel/JsonLabelVO.java
# Inspect the schema definition for JsonLabelVO
sed -n '120,170p' conf/db/upgrade/V1.7__schema.sql
# Inspect any later schema changes affecting labelValue
sed -n '1,40p' conf/db/upgrade/V3.9.0__schema.sqlRepository: MatheMatrix/zstack
Length of output: 9348
CA 私钥不要明文落库
cakey.pem 通过 JsonLabel.createIfAbsent() 直接写入 JsonLabelVO.labelValue，该字段只是普通 text/MEDIUMTEXT，没有加密或脱敏；数据库访问面一旦暴露，CA 私钥就会泄露。若需要跨管理节点共享，改为加密存储或受控密钥分发。
🤖 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/KVMHostFactory.java` around lines 497
- 506, The KVMHostFactory TLS sync logic is persisting the CA private key in
plain text through JsonLabel.createIfAbsent() and JsonLabelVO.labelValue; update
this flow to avoid storing the raw key directly. In the block that reads
cakey.pem and writes via LIBVIRT_TLS_PRIVATE_KEY, switch to encrypted storage or
a controlled secret-distribution mechanism, and only write/decrypt the key when
restoring local files. Ensure the handling around createIfAbsent(), caInv, and
keyInv no longer exposes the private key in the database.
zstack-robot-2
deleted the
sync/yingzhe.hu/cherry-pick/ZSTAC-84446-5.4.10-full
branch
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
3 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.
Summary
Backport the ZSTAC-84446 TLS reconnect PID fix chain to
5.4.10.br_conn_all_nshost-internal IP from TLS SAN expectations.5.4.10does not yet have the TLS config/deploy/migration test foundation used by this fix.Validation
git diff --check upstream/5.4.10..HEADmvn compile -pl plugin/kvm -am -Dmaven.test.skipmvn install -pl plugin/kvm -am -Dmaven.test.skipmvn -f test/pom.xml test -Dtest=org.zstack.test.kvm.KVMHostUtilsTest -DfailIfNoTests=false -DskipJacoco=trueResolves: ZSTAC-84446
sync from gitlab !10427