<fix>[kvm]: update TLS certs via kvmagent on host reconnect#3706
Open
zstack-robot-2 wants to merge 1 commit into5.5.12from
Open
<fix>[kvm]: update TLS certs via kvmagent on host reconnect#3706zstack-robot-2 wants to merge 1 commit into5.5.12from
zstack-robot-2 wants to merge 1 commit into5.5.12from
Conversation
Walkthrough在 KVM 主机连接流程中新增了用于更新 libvirt TLS 证书的命令/响应 DTO、主机端 REST 路径常量，并在连接流程中加入一个无回滚步骤，按条件读取证书/私钥并调用主机代理更新证书；失败仅记录警告，不阻塞连接。 Changes
序列流程图sequenceDiagram
participant Host as KVMHost
participant Config as 配置检查
participant LabelDB as JsonLabel/DB
participant Agent as KVM代理
Host->>Config: 检查 UNIT_TEST_ON 与\nKVMGlobalConfig.LIBVIRT_TLS_ENABLED
Config-->>Host: 是否继续
alt TLS启用且非单元测试
Host->>LabelDB: 读取 libvirtTLSCA\n和 libvirtTLSPrivateKey
LabelDB-->>Host: 返回证书与私钥
Host->>Host: 构建去重 certIps（managementIp + EXTRA_IPS）
Host->>Agent: POST `KVM_UPDATE_TLS_CERT_PATH`\n发送 UpdateTlsCertCmd(caCert, caKey, certIps)
Agent-->>Host: UpdateTlsCertResponse / 错误
alt 调用成功
Host->>Host: 继续连接流程（trigger.next）
else 调用失败
Host->>Host: 记录警告并继续（trigger.next）
end
else 条件不满足
Host->>Host: 跳过证书更新步骤
end
代码审查工作量估算🎯 4 (Complex) | ⏱️ ~45 minutes 诗歌
🚥 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 docstrings
🧪 Generate unit tests (beta)
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.42.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/KVMAgentCommands.java`:
- Around line 459-466: The caKey field in KVMAgentCommands is sensitive and
missing the `@NoLogging` annotation; annotate the private String caKey field (and
optionally its getCaKey/setCaKey methods) with `@NoLogging` to prevent it from
appearing in serialized/debug logs, and add the required import for the
NoLogging annotation so compilation succeeds.
🪄 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: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
Run ID: 6f02ffd9-b89d-4cc8-bdbf-0149cd73c187
📒 Files selected for processing (3)
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/KVMHost.java
Resolves: ZSTAC-83696 Change-Id: I368cc5af8fb3d553bedc3be5d031015719e68ddc
MatheMatrix
force-pushed
the
sync/yingzhe.hu/5.5.12-ZSTAC-83696
branch
from
09d7ccb to
d2b5dc4
Compare
Collaborator
|
Comment from yaohua.wu: 🔍 Code Review — MR !9569 (zstack)关联 Jira: ZSTAC-83696 — 迁移网络非物理机管理网段时，存储迁移失败 (P1 Bug) ✅ 问题与方案一致性Java 管理侧正确实现了在主机 reconnect 流程中调用新的 kvmagent TLS 证书更新 API。流程位置合理（在收集 host facts 之后），跳过逻辑正确（单元测试或 TLS 未启用时跳过），错误处理得当（失败不阻断 reconnect）。 🟡 Warning (2)W1. String caCert = new JsonLabel().get("libvirtTLSCA", String.class);
String caKey = new JsonLabel().get("libvirtTLSPrivateKey", String.class);通过 建议确认现有代码库中是否有 W2.
🟢 Suggestion (2)S1. IP 列表构建可以考虑使用 List<String> allIps = new ArrayList<>();
allIps.add(managementIp);
if (extraIps != null && !extraIps.isEmpty()) {
for (String ip : extraIps.split(",")) {
String trimmed = ip.trim();
if (!trimmed.isEmpty() && !allIps.contains(trimmed)) {
allIps.add(trimmed);
}
}
}
Set<String> allIps = new LinkedHashSet<>();
allIps.add(managementIp);
if (extraIps != null && !extraIps.isEmpty()) {
for (String ip : extraIps.split(",")) {
String trimmed = ip.trim();
if (!trimmed.isEmpty()) allIps.add(trimmed);
}
}
String certIps = String.join(",", allIps);S2. 建议增加日志记录调用前的 IP 列表 logger.debug(String.format("Requesting TLS cert update on host[uuid:%s] for IPs: %s",
self.getUuid(), certIps));👍 优点
🤖 Robot Reviewer |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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`:
- Around line 6034-6096: The flow "update-tls-certs-if-needed" currently
unconditionally pushes libvirt TLS certs; change it to first read the host's
existing libvirt cert metadata and only proceed if the cert is already managed
by ZStack (e.g., compare a stored marker/fingerprint or host tag), otherwise
skip or require an explicit override flag (add a new config like
KVMGlobalConfig.LIBVIRT_TLS_FORCE_UPDATE). If proceeding, before calling
UpdateTlsCertCmd record the original cert/key/fingerprint (store under JsonLabel
keys such as "libvirtTLSOriginalCert"/"libvirtTLSOriginalKey" or a host tag) so
you can roll back, and modify the success/fail handlers to leave originals
intact and expose a rollback path; ensure checks live inside the run(...) of the
NoRollbackFlow and gate the new Http<>(updateTlsCertPath, ...) call by the
marker/fingerprint/or-force-flag logic.
- Around line 6062-6065: The current check only treats caCert/caKey as missing
when null, allowing empty or whitespace-only strings to be sent; update the
retrieval and check around JsonLabel.get("libvirtTLSCA", String.class) and
JsonLabel.get("libvirtTLSPrivateKey", String.class) (used in the block that logs
"TLS CA cert/key not found in database, skipping cert update") to treat blank
values as missing by applying a trimToNull-style normalization (or
StringUtils.isBlank) and then conditionally skipping and logging when the
normalized values are null/blank.
🪄 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: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
Run ID: 097fd425-a051-4c5f-bae8-0527680af8c2
📒 Files selected for processing (3)
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/KVMHost.java
✅ Files skipped from review due to trivial changes (1)
- plugin/kvm/src/main/java/org/zstack/kvm/KVMConstant.java
Comment on lines
+6034
to
+6096
| flow(new NoRollbackFlow() { | ||
| String __name__ = "update-tls-certs-if-needed"; | ||
|
|
||
| @Override | ||
| public boolean skip(Map data) { | ||
| return CoreGlobalProperty.UNIT_TEST_ON | ||
| || !KVMGlobalConfig.LIBVIRT_TLS_ENABLED.value(Boolean.class); | ||
| } | ||
|
|
||
| @Override | ||
| public void run(FlowTrigger trigger, Map data) { | ||
| String managementIp = getSelf().getManagementIp(); | ||
| String extraIps = HostSystemTags.EXTRA_IPS.getTokenByResourceUuid( | ||
| self.getUuid(), HostSystemTags.EXTRA_IPS_TOKEN); | ||
|
|
||
| List<String> allIps = new ArrayList<>(); | ||
| allIps.add(managementIp); | ||
| if (extraIps != null && !extraIps.isEmpty()) { | ||
| for (String ip : extraIps.split(",")) { | ||
| String trimmed = ip.trim(); | ||
| if (!trimmed.isEmpty() && !allIps.contains(trimmed)) { | ||
| allIps.add(trimmed); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| String certIps = String.join(",", allIps); | ||
|
|
||
| String caCert = new JsonLabel().get("libvirtTLSCA", String.class); | ||
| String caKey = new JsonLabel().get("libvirtTLSPrivateKey", String.class); | ||
| if (caCert == null || caKey == null) { | ||
| logger.warn("TLS CA cert/key not found in database, skipping cert update"); | ||
| trigger.next(); | ||
| return; | ||
| } | ||
|
|
||
| UpdateTlsCertCmd cmd = new UpdateTlsCertCmd(); | ||
| cmd.setCaCert(caCert); | ||
| cmd.setCaKey(caKey); | ||
| cmd.setCertIps(certIps); | ||
|
|
||
| new Http<>(updateTlsCertPath, cmd, UpdateTlsCertResponse.class) | ||
| .call(new ReturnValueCompletion<UpdateTlsCertResponse>(trigger) { | ||
| @Override | ||
| public void success(UpdateTlsCertResponse ret) { | ||
| if (!ret.isSuccess()) { | ||
| logger.warn(String.format("Failed to update TLS certs on host[uuid:%s]: %s", | ||
| self.getUuid(), ret.getError())); | ||
| } | ||
| // cert update failure should not block reconnect | ||
| trigger.next(); | ||
| } | ||
|
|
||
| @Override | ||
| public void fail(ErrorCode errorCode) { | ||
| logger.warn(String.format("Failed to update TLS certs on host[uuid:%s]: %s", | ||
| self.getUuid(), errorCode)); | ||
| // cert update failure should not block reconnect | ||
| trigger.next(); | ||
| } | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
避免在连接流程里无条件覆盖宿主机现有 TLS 证书
这里在 LIBVIRT_TLS_ENABLED 打开后会直接向 host 下发并更新 libvirt TLS 证书，但没有判断当前证书是否由平台托管，也没有记录原状态或提供回退路径。这样会把宿主机上可能由用户手工维护的证书一起覆盖掉，尤其这段逻辑还会跑在新增/重连流程里。建议至少加上“仅处理 ZStack 托管证书”的标记/指纹校验，或者放到显式开关后面。
As per coding guidelines “平台不应直接覆盖用户可能自定义修改过的东西……应当对原状态做好记录，做好二次确认，做好回退准备”.
🤖 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` around lines 6034 -
6096, The flow "update-tls-certs-if-needed" currently unconditionally pushes
libvirt TLS certs; change it to first read the host's existing libvirt cert
metadata and only proceed if the cert is already managed by ZStack (e.g.,
compare a stored marker/fingerprint or host tag), otherwise skip or require an
explicit override flag (add a new config like
KVMGlobalConfig.LIBVIRT_TLS_FORCE_UPDATE). If proceeding, before calling
UpdateTlsCertCmd record the original cert/key/fingerprint (store under JsonLabel
keys such as "libvirtTLSOriginalCert"/"libvirtTLSOriginalKey" or a host tag) so
you can roll back, and modify the success/fail handlers to leave originals
intact and expose a rollback path; ensure checks live inside the run(...) of the
NoRollbackFlow and gate the new Http<>(updateTlsCertPath, ...) call by the
marker/fingerprint/or-force-flag logic.
Comment on lines
+6062
to
+6065
| String caCert = new JsonLabel().get("libvirtTLSCA", String.class); | ||
| String caKey = new JsonLabel().get("libvirtTLSPrivateKey", String.class); | ||
| if (caCert == null || caKey == null) { | ||
| logger.warn("TLS CA cert/key not found in database, skipping cert update"); |
There was a problem hiding this comment.
把空白证书内容也当成缺失处理
这里只判断了 null，但 "" 或全空白字符串会继续下发到 agent，绕过“缺失就跳过”的保护分支。这里先做 trimToNull 更稳妥。
🩹 建议修改
- String caCert = new JsonLabel().get("libvirtTLSCA", String.class);
- String caKey = new JsonLabel().get("libvirtTLSPrivateKey", String.class);
+ String caCert = StringUtils.trimToNull(new JsonLabel().get("libvirtTLSCA", String.class));
+ String caKey = StringUtils.trimToNull(new JsonLabel().get("libvirtTLSPrivateKey", String.class));
if (caCert == null || caKey == null) {📝 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
| String caCert = new JsonLabel().get("libvirtTLSCA", String.class); | |
| String caKey = new JsonLabel().get("libvirtTLSPrivateKey", String.class); | |
| if (caCert == null || caKey == null) { | |
| logger.warn("TLS CA cert/key not found in database, skipping cert update"); | |
| String caCert = StringUtils.trimToNull(new JsonLabel().get("libvirtTLSCA", String.class)); | |
| String caKey = StringUtils.trimToNull(new JsonLabel().get("libvirtTLSPrivateKey", String.class)); | |
| if (caCert == null || caKey == null) { | |
| logger.warn("TLS CA cert/key not found in database, skipping cert update"); |
🤖 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` around lines 6062 -
6065, The current check only treats caCert/caKey as missing when null, allowing
empty or whitespace-only strings to be sent; update the retrieval and check
around JsonLabel.get("libvirtTLSCA", String.class) and
JsonLabel.get("libvirtTLSPrivateKey", String.class) (used in the block that logs
"TLS CA cert/key not found in database, skipping cert update") to treat blank
values as missing by applying a trimToNull-style normalization (or
StringUtils.isBlank) and then conditionally skipping and logging when the
normalized values are null/blank.
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.
Resolves: ZSTAC-83696
Change-Id: I368cc5af8fb3d553bedc3be5d031015719e68ddc
sync from gitlab !9569