<fix>[kvm]: decouple TLS cert detection from libvirtd restart toggle#3866
Open
zstack-robot-2 wants to merge 1 commit into5.5.16from
Open
<fix>[kvm]: decouple TLS cert detection from libvirtd restart toggle#3866zstack-robot-2 wants to merge 1 commit into5.5.16from
zstack-robot-2 wants to merge 1 commit into5.5.16from
Conversation
演练在启用 变更
评估代码审核工作量🎯 3 (中等) | ⏱️ ~20 分钟 诗
🚥 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 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.
🧹 Nitpick comments (1)
plugin/kvm/src/main/java/org/zstack/kvm/KVMHostUtils.java (1)
143-145: 建议把多布尔入参改成语义化类型，降低调用歧义Line [143-145] 的方法签名使用了 3 个布尔参数，调用点会出现
true/false/true这类“位置语义”问题，后续维护和扩展风险较高。建议改成枚举（或拆分成场景化方法）。♻️ 示例改法
+ public enum HostConnectScene { + NEW_ADD, + RECONNECT + } + public static boolean shouldForceTlsRedeploy(boolean needDeployTlsCert, boolean allowRestartLibvirtd, - boolean isNewAdded) { + HostConnectScene scene) { if (!needDeployTlsCert) { return false; } - return allowRestartLibvirtd || isNewAdded; + return allowRestartLibvirtd || scene == HostConnectScene.NEW_ADD; }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/KVMHostUtils.java` around lines 143 - 145, The method KVMHostUtils.shouldForceTlsRedeploy currently takes three boolean flags (needDeployTlsCert, allowRestartLibvirtd, isNewAdded) which causes call-site ambiguity; replace the boolean parameters with a semantic type (e.g., an enum like TlsRedeployReason or a small value-object) or split into explicit methods (e.g., shouldForceTlsRedeployForNewHost, shouldForceTlsRedeployWithRestartPermission) and update all callers to pass the enum/value-object or call the new methods; ensure the new enum/value-object exposes clear names for each condition and update the method signature and internal logic in shouldForceTlsRedeploy accordingly so callers no longer pass positional true/false values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMHostUtils.java`:
- Around line 143-145: The method KVMHostUtils.shouldForceTlsRedeploy currently
takes three boolean flags (needDeployTlsCert, allowRestartLibvirtd, isNewAdded)
which causes call-site ambiguity; replace the boolean parameters with a semantic
type (e.g., an enum like TlsRedeployReason or a small value-object) or split
into explicit methods (e.g., shouldForceTlsRedeployForNewHost,
shouldForceTlsRedeployWithRestartPermission) and update all callers to pass the
enum/value-object or call the new methods; ensure the new enum/value-object
exposes clear names for each condition and update the method signature and
internal logic in shouldForceTlsRedeploy accordingly so callers no longer pass
positional true/false values.
ℹ️ 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: 392c51ce-a26b-47d8-80bb-87568a29f7d7
📒 Files selected for processing (3)
plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMHostUtils.javatest/src/test/java/org/zstack/test/kvm/KVMHostUtilsTest.java
MatheMatrix
force-pushed
the
sync/yingzhe.hu/fix/ZSTAC-84446-reopen
branch
from
1b6b10c to
3309397
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/src/test/java/org/zstack/test/kvm/KVMHostUtilsTest.java (1)
177-199: 测试方法名建议改为 lowerCamelCase 以符合 Java 规范。
shouldForceTlsRedeploy_noNeedNeverForces等方法名含下划线，建议统一改成shouldForceTlsRedeployNoNeedNeverForces这类 lowerCamelCase。✏️ 建议修改
- public void shouldForceTlsRedeploy_noNeedNeverForces() { + public void shouldForceTlsRedeployNoNeedNeverForces() { @@ - public void shouldForceTlsRedeploy_allowRestartForces() { + public void shouldForceTlsRedeployAllowRestartForces() { @@ - public void shouldForceTlsRedeploy_newAddedForces() { + public void shouldForceTlsRedeployNewAddedForces() { @@ - public void shouldForceTlsRedeploy_reconnectWithoutRestartSkips() { + public void shouldForceTlsRedeployReconnectWithoutRestartSkips() {As per coding guidelines, “方法名、参数名、成员变量和局部变量：使用 lowerCamelCase 风格。”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/src/test/java/org/zstack/test/kvm/KVMHostUtilsTest.java` around lines 177 - 199, The test method names use underscores and should be converted to lowerCamelCase to match Java conventions: rename shouldForceTlsRedeploy_noNeedNeverForces -> shouldForceTlsRedeployNoNeedNeverForces, shouldForceTlsRedeploy_allowRestartForces -> shouldForceTlsRedeployAllowRestartForces, shouldForceTlsRedeploy_newAddedForces -> shouldForceTlsRedeployNewAddedForces, and shouldForceTlsRedeploy_reconnectWithoutRestartSkips -> shouldForceTlsRedeployReconnectWithoutRestartSkips (update the method declarations and any references or test runners accordingly) in the KVMHostUtilsTest class so all test method names follow lowerCamelCase.
🤖 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 5719-5722: 当前 TLS 证书探测在 connectHook 中直接调用 collectHostIps(...)
并可能抛出异常或返回异常值，导致整个重连流程失败；请将整段探测逻辑按
KvmSecureBootExtensions.syncVmHostFilesFromHost 的模式用 try/catch 包裹，捕获所有异常并在 catch
中记录告警日志（说明是 TLS 探测失败及相关主机信息），然后无条件调用 trigger.next()，保持 NEED_DEPLOY_TLS_CERT
为最佳努力语义且不阻塞 connectHook 的继续执行；确保引用方法名 collectHostIps、标志 NEED_DEPLOY_TLS_CERT
和触发器 trigger.next() 以便准确定位修改位置。
---
Nitpick comments:
In `@test/src/test/java/org/zstack/test/kvm/KVMHostUtilsTest.java`:
- Around line 177-199: The test method names use underscores and should be
converted to lowerCamelCase to match Java conventions: rename
shouldForceTlsRedeploy_noNeedNeverForces ->
shouldForceTlsRedeployNoNeedNeverForces,
shouldForceTlsRedeploy_allowRestartForces ->
shouldForceTlsRedeployAllowRestartForces, shouldForceTlsRedeploy_newAddedForces
-> shouldForceTlsRedeployNewAddedForces, and
shouldForceTlsRedeploy_reconnectWithoutRestartSkips ->
shouldForceTlsRedeployReconnectWithoutRestartSkips (update the method
declarations and any references or test runners accordingly) in the
KVMHostUtilsTest class so all test method names follow lowerCamelCase.
🪄 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: 25d2616a-9a0c-48f1-9283-628d0de8cd7e
📒 Files selected for processing (3)
plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMHostUtils.javatest/src/test/java/org/zstack/test/kvm/KVMHostUtilsTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- plugin/kvm/src/main/java/org/zstack/kvm/KVMHostUtils.java
Resolves: ZSTAC-84446 Change-Id: I9bed31c0cefddd6ed11f59cd13e36eb1c2abc029
MatheMatrix
force-pushed
the
sync/yingzhe.hu/fix/ZSTAC-84446-reopen
branch
from
3309397 to
ad8a4c5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/src/test/java/org/zstack/test/kvm/KVMHostUtilsTest.java (1)
177-199: 测试方法名应遵循 lowerCamelCase 规范。当前四个测试方法名包含下划线：
shouldForceTlsRedeploy_noNeedNeverForcesshouldForceTlsRedeploy_allowRestartForcesshouldForceTlsRedeploy_newAddedForcesshouldForceTlsRedeploy_reconnectWithoutRestartSkips建议改为
shouldForceTlsRedeployNoNeedNeverForces、shouldForceTlsRedeployAllowRestartForces等纯 lowerCamelCase 形式，以符合编码指南中对方法命名的要求。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/src/test/java/org/zstack/test/kvm/KVMHostUtilsTest.java` around lines 177 - 199, The test method names use underscores and must be converted to lowerCamelCase; rename the four methods shouldForceTlsRedeploy_noNeedNeverForces, shouldForceTlsRedeploy_allowRestartForces, shouldForceTlsRedeploy_newAddedForces, and shouldForceTlsRedeploy_reconnectWithoutRestartSkips to shouldForceTlsRedeployNoNeedNeverForces, shouldForceTlsRedeployAllowRestartForces, shouldForceTlsRedeployNewAddedForces, and shouldForceTlsRedeployReconnectWithoutRestartSkips respectively, updating their declarations and any references (e.g., in test runners or IDE configurations) so the `@Test` methods in KVMHostUtilsTest compile and run under the project’s lowerCamelCase naming convention.
🤖 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 5741-5759: The certIpList returned by KVMHostUtils.collectHostIps
is used raw (split(",")) which can produce empty or whitespace entries and cause
false positives in the SAN comparison; normalize and filter it before use: split
certIpList, trim each entry, remove empty strings, rebuild certIpList (or a
canonicalList) and set data.put("TLS_DETECTED_IPS", canonicalList) and use that
canonicalList to populate allIps; keep using parseSanIps(...) and the existing
needDeploy logic but compare against the cleaned allIps to avoid unnecessary
redeploys.
---
Nitpick comments:
In `@test/src/test/java/org/zstack/test/kvm/KVMHostUtilsTest.java`:
- Around line 177-199: The test method names use underscores and must be
converted to lowerCamelCase; rename the four methods
shouldForceTlsRedeploy_noNeedNeverForces,
shouldForceTlsRedeploy_allowRestartForces,
shouldForceTlsRedeploy_newAddedForces, and
shouldForceTlsRedeploy_reconnectWithoutRestartSkips to
shouldForceTlsRedeployNoNeedNeverForces,
shouldForceTlsRedeployAllowRestartForces, shouldForceTlsRedeployNewAddedForces,
and shouldForceTlsRedeployReconnectWithoutRestartSkips respectively, updating
their declarations and any references (e.g., in test runners or IDE
configurations) so the `@Test` methods in KVMHostUtilsTest compile and run under
the project’s lowerCamelCase naming convention.
🪄 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: 5c0be66c-e4d3-4e34-9989-f986ca61035a
📒 Files selected for processing (3)
plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMHostUtils.javatest/src/test/java/org/zstack/test/kvm/KVMHostUtilsTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- plugin/kvm/src/main/java/org/zstack/kvm/KVMHostUtils.java
Comment on lines
+5741
to
+5759
| String certIpList = KVMHostUtils.collectHostIps( | ||
| sshShell, self.getUuid(), managementIp); | ||
| List<String> allIps = new ArrayList<>(Arrays.asList(certIpList.split(","))); | ||
| // Save detected IPs so apply-ansible-playbook can union with | ||
| // EXTRA_IPS without running a second SSH. | ||
| data.put("TLS_DETECTED_IPS", certIpList); | ||
|
|
||
| SshResult sanResult = sshShell.runCommand( | ||
| "openssl x509 -in /etc/pki/libvirt/servercert.pem -noout -ext subjectAltName 2>/dev/null"); | ||
|
|
||
| boolean needDeploy = false; | ||
| if (sanResult.isSshFailure() || sanResult.getReturnCode() != 0 | ||
| || sanResult.getStdout() == null || sanResult.getStdout().trim().isEmpty()) { | ||
| logger.info(String.format("TLS cert not found or unreadable on host[uuid:%s], need deploy", self.getUuid())); | ||
| needDeploy = true; | ||
| } else { | ||
| Set<String> sanIps = parseSanIps(sanResult.getStdout()); | ||
| for (String ip : allIps) { | ||
| if (!sanIps.contains(ip)) { |
There was a problem hiding this comment.
先规范化 certIpList，否则这里会误判需要重部署。
Line 5743 直接 split(",") 后没有 trim/过滤空值。只要 collectHostIps(...) 的返回里带空格、尾逗号，或者是空串，后面的 SAN 对比就可能把 NEED_DEPLOY_TLS_CERT 错误置为 true。
🛠️ 建议修改
- List<String> allIps = new ArrayList<>(Arrays.asList(certIpList.split(",")));
+ List<String> allIps = Arrays.stream(certIpList.split(","))
+ .map(String::trim)
+ .filter(StringUtils::isNotBlank)
+ .collect(Collectors.toList());🤖 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 5741 -
5759, The certIpList returned by KVMHostUtils.collectHostIps is used raw
(split(",")) which can produce empty or whitespace entries and cause false
positives in the SAN comparison; normalize and filter it before use: split
certIpList, trim each entry, remove empty strings, rebuild certIpList (or a
canonicalList) and set data.put("TLS_DETECTED_IPS", canonicalList) and use that
canonicalList to populate allIps; keep using parseSanIps(...) and the existing
needDeploy logic but compare against the cleaned allIps to avoid unnecessary
redeploys.
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
2 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-84446
Change-Id: I9bed31c0cefddd6ed11f59cd13e36eb1c2abc029
sync from gitlab !9741