<feature>[sdnController]: enrich prepare-service#4179
Open
zstack-robot-1 wants to merge 5 commits into
Open
Conversation
added 2 commits
May 28, 2026 11:10
Add a Cloud-side handler to install and start zns-agent on hosts. JIRA: ZSTAC-3881 Related: ZCF-3881 Change-Id: Ie137516a69de9793f83c872d0f70e94f95f53350
Support richer prepare-service inputs for zns-agent.\n\nThe installer now accepts explicit controller addresses,\npackage metadata, and URL-based package resolution with\nsha256 validation. Tests cover address normalization and\npackage lookup/download handling.\n\nJIRA: ZSTAC-3881\nRelated: ZCF-3881\nChange-Id: Ied352168ff0e37ee911e505bc746d35750b5df0b
|
Warning
|
| Layer / File(s) | Summary |
|---|---|
配置与命令协议定义 plugin/sdnController/src/main/java/org/zstack/sdnController/ZnsProxyGlobalProperty.java, plugin/sdnController/src/main/java/org/zstack/sdnController/znsproxy/ZnsProxyPrepareServiceCmd.java |
ZnsProxyGlobalProperty 声明4个全局配置项（包路径、远端路径、仓库路径、包名）及默认值；ZnsProxyPrepareServiceCmd 定义REST命令协议与数据字段。 |
安装器核心实现 plugin/sdnController/src/main/java/org/zstack/sdnController/znsproxy/ZnsProxyInstaller.java |
ZnsProxyInstaller 提供完整的SSH远程安装流程：入口校验、包解析与下载、主机数据库查询、scp上传、远端命令执行（含权限设置、安装脚本、健康探测），以及包名合法性校验、SHA256验证、shell转义、端口规范化等支持方法。 |
控制器REST端点集成 plugin/sdnController/src/main/java/org/zstack/sdnController/SdnControllerManagerImpl.java |
SdnControllerManagerImpl 新增 RESTFacade 依赖注入和 ZnsProxyInstaller 成员，在 start() 方法中初始化安装器并注册 /zns/notify/prepare-service 同步HTTP处理器。 |
单元测试覆盖 test/src/test/java/org/zstack/test/unittest/sdnController/ZnsProxyInstallerTest.java |
ZnsProxyInstallerTest 覆盖安装命令生成、按包名解析、按版本解析、路径穿越防护、缺失包异常、URL包下载等6个测试场景。 |
Sequence Diagram(s)
sequenceDiagram
participant Client
participant SdnController
participant ZnsProxyInstaller
participant Database
participant RemoteHost
Client->>SdnController: POST /zns/notify/prepare-service
SdnController->>ZnsProxyInstaller: install(cmd)
ZnsProxyInstaller->>ZnsProxyInstaller: resolvePackage(cmd)
ZnsProxyInstaller->>Database: queryHost(hostUuid)
ZnsProxyInstaller->>RemoteHost: scp upload package
ZnsProxyInstaller->>RemoteHost: execute install command
ZnsProxyInstaller->>RemoteHost: health check
RemoteHost-->>ZnsProxyInstaller: success/error
ZnsProxyInstaller-->>SdnController: completion
SdnController-->>Client: response
🎯 3 (Moderate) | ⏱️ ~22 minutes
🐰 Proxy deployed through SDN,
SSH commands scripted and refined,
Sha256 hashes verified well—
REST handler binds the spell,
Tests ensure all paths align!
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | 标题 "[feature][sdnController]: enrich prepare-service" 直接且清晰地描述了此次变更的主要功能，与原始总结中 SdnControllerManagerImpl 新增 REST 同步 HTTP 回调注册、ZnsProxyInstaller 和 ZnsProxyPrepareServiceCmd 新增等核心改动保持高度一致。 |
| Description check | ✅ Passed | PR 描述清晰说明了该变更的目标与范围：为 zns-agent 增强 prepare-service 功能，包括支持更丰富的输入（显式控制器地址、包元数据、基于 URL 的包解析与 sha256 验证）并包含相关测试，与代码变更内容充分关联。 |
| 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. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ 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/dejing.liu/5.5.22
Comment @coderabbitai help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
test/src/test/java/org/zstack/test/unittest/sdnController/ZnsAgentInstallerTest.java (1)
105-119: ⚡ Quick win补充 URL 下载分支的 sha256 正反用例。
当前只验证了“能下载且非空”，未覆盖
sha256匹配与不匹配分支。建议增加两条断言，避免后续回归破坏完整性校验逻辑。🤖 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/unittest/sdnController/ZnsAgentInstallerTest.java` around lines 105 - 119, Add assertions and cases in testResolvePackageDownloadsUrl to cover the sha256 path: use ZnsAgentPrepareServiceCmd.sha256 to set the correct SHA256 of the temp file and assert resolvePackage returns the same file and passes integrity (non-empty); then add a negative case by setting cmd.sha256 to a wrong digest and assert resolvePackage throws or fails integrity (expect exception or verify behavior defined in ZnsAgentInstaller.resolvePackage). Locate changes around the test method testResolvePackageDownloadsUrl and references to ZnsAgentInstaller.resolvePackage and ZnsAgentPrepareServiceCmd to implement these two checks.
🤖 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/sdnController/src/main/java/org/zstack/sdnController/SdnControllerManagerImpl.java`:
- Around line 581-583: The log prints cmd.packageUrl un-sanitized in
SdnControllerManagerImpl (inside the logger.info call around prepare-service
handling); implement a sanitizer (e.g., sanitizeUrlForLog) that accepts a raw
URL, returns null or "<invalid-url>" for bad input, and constructs a string with
only scheme, host[:port] and path (strip userinfo, query, fragment), then call
that sanitizer and log the sanitized value instead of cmd.packageUrl in the
logger.info; ensure the sanitizer handles nulls and exceptions and reference it
from the SdnControllerManagerImpl logging site.
In
`@plugin/sdnController/src/main/java/org/zstack/sdnController/znsagent/ZnsAgentInstaller.java`:
- Around line 101-106: In ZnsAgentInstaller (where the installer builds remote
shell commands) avoid hardcoding mkdir paths; replace the fixed "mkdir -p " +
PACKAGE_REMOTE_PATH and "/etc/zstack-zns" with mkdir commands that create the
directories derived from PACKAGE_PATH and CONFIG_PATH respectively so the
created directories match the actual deploy targets (use PACKAGE_PATH's parent
directory and CONFIG_PATH's parent directory), and ensure subsequent mv/chmod
operations still reference PACKAGE_PATH and CONFIG_PATH.
- Line 111: The health check URL in ZnsAgentInstaller is hardcoded to
"127.0.0.1:8090" and will break when LISTEN_ADDRESS is changed; replace the
fixed URL with a builder that derives host and port from the LISTEN_ADDRESS
value (parse LISTEN_ADDRESS, split on the last ':' to get host and port, treat
"0.0.0.0" as "127.0.0.1", and fall back to defaults when LISTEN_ADDRESS is
null/invalid) and use that builder (e.g., buildHealthCheckUrl) wherever the
health check HTTP call is made so the agent checks the actual configured listen
address.
- Around line 162-174: In the ZnsAgentInstaller packageUrl branch, ensure URL
downloads always perform integrity checks: if cmd.packageUrl is present but
cmd.sha256 is null/empty, throw a CloudRuntimeException instead of skipping
verification; keep the download logic (packageInRepository,
FileUtils.copyURLToFile) and then call verifySha256(target, cmd.sha256) as
before, but validate cmd.sha256 before calling verifySha256 and produce a clear
error mentioning missing sha256 for this package URL.
---
Nitpick comments:
In
`@test/src/test/java/org/zstack/test/unittest/sdnController/ZnsAgentInstallerTest.java`:
- Around line 105-119: Add assertions and cases in
testResolvePackageDownloadsUrl to cover the sha256 path: use
ZnsAgentPrepareServiceCmd.sha256 to set the correct SHA256 of the temp file and
assert resolvePackage returns the same file and passes integrity (non-empty);
then add a negative case by setting cmd.sha256 to a wrong digest and assert
resolvePackage throws or fails integrity (expect exception or verify behavior
defined in ZnsAgentInstaller.resolvePackage). Locate changes around the test
method testResolvePackageDownloadsUrl and references to
ZnsAgentInstaller.resolvePackage and ZnsAgentPrepareServiceCmd to implement
these two checks.
🪄 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: e8d2f0e7-c43b-4d07-bb85-03288560b3f4
📒 Files selected for processing (5)
plugin/sdnController/src/main/java/org/zstack/sdnController/SdnControllerManagerImpl.javaplugin/sdnController/src/main/java/org/zstack/sdnController/ZnsAgentGlobalProperty.javaplugin/sdnController/src/main/java/org/zstack/sdnController/znsagent/ZnsAgentInstaller.javaplugin/sdnController/src/main/java/org/zstack/sdnController/znsagent/ZnsAgentPrepareServiceCmd.javatest/src/test/java/org/zstack/test/unittest/sdnController/ZnsAgentInstallerTest.java
Comment on lines
+581
to
+583
| logger.info(String.format("[ZnsAgent] prepare-service command received: cmUUID=%s hosts=%s controllers=%s packageName=%s agentVersion=%s packageUrl=%s", | ||
| cmd.computerManagerUuid, cmd.hostUuids, cmd.controllerAddresses, | ||
| cmd.packageName, cmd.agentVersion, cmd.packageUrl)); |
There was a problem hiding this comment.
避免在日志中输出未脱敏的 packageUrl。
Line 581-583 会把完整 packageUrl 打到 info 日志，若 URL 含签名参数或凭据会造成敏感信息泄露。建议只记录主机与路径，去掉 query/fragment 与 userinfo。
建议修复
- logger.info(String.format("[ZnsAgent] prepare-service command received: cmUUID=%s hosts=%s controllers=%s packageName=%s agentVersion=%s packageUrl=%s",
+ logger.info(String.format("[ZnsAgent] prepare-service command received: cmUUID=%s hosts=%s controllers=%s packageName=%s agentVersion=%s packageUrl=%s",
cmd.computerManagerUuid, cmd.hostUuids, cmd.controllerAddresses,
- cmd.packageName, cmd.agentVersion, cmd.packageUrl));
+ cmd.packageName, cmd.agentVersion, sanitizeUrlForLog(cmd.packageUrl)));private static String sanitizeUrlForLog(String rawUrl) {
if (rawUrl == null) {
return null;
}
try {
URL u = new URL(rawUrl);
int port = u.getPort();
String authority = port > 0 ? (u.getHost() + ":" + port) : u.getHost();
return u.getProtocol() + "://" + authority + u.getPath();
} catch (Exception ignored) {
return "<invalid-url>";
}
}🤖 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/sdnController/src/main/java/org/zstack/sdnController/SdnControllerManagerImpl.java`
around lines 581 - 583, The log prints cmd.packageUrl un-sanitized in
SdnControllerManagerImpl (inside the logger.info call around prepare-service
handling); implement a sanitizer (e.g., sanitizeUrlForLog) that accepts a raw
URL, returns null or "<invalid-url>" for bad input, and constructs a string with
only scheme, host[:port] and path (strip userinfo, query, fragment), then call
that sanitizer and log the sanitized value instead of cmd.packageUrl in the
logger.info; ensure the sanitizer handles nulls and exceptions and reference it
from the SdnControllerManagerImpl logging site.
Comment on lines
+101
to
+106
| "mkdir -p " + PACKAGE_REMOTE_PATH, | ||
| "mkdir -p /etc/zstack-zns", | ||
| "mv " + remotePackage + " " + PACKAGE_PATH, | ||
| "chmod 0755 " + PACKAGE_PATH, | ||
| "mv " + remoteConfig + " " + CONFIG_PATH, | ||
| "chmod 0644 " + CONFIG_PATH, |
There was a problem hiding this comment.
避免硬编码目录导致全局配置失效。
Line 101-106 当前创建目录与实际落盘路径不一致：命令使用 PACKAGE_PATH / CONFIG_PATH，但目录准备写死为 PACKAGE_REMOTE_PATH 和 /etc/zstack-zns。一旦全局属性改为其他目录，这里会直接失败。
建议修复
+ String packageDir = new File(PACKAGE_PATH).getParent();
+ String configDir = new File(CONFIG_PATH).getParent();
+
SshResult ret = ssh
.scpUpload(localPackage.getAbsolutePath(), remotePackage)
.scpUpload(configFile.getAbsolutePath(), remoteConfig)
.sudoCommand(
"systemctl stop " + SERVICE_NAME + " > /dev/null 2>&1 || true",
"pkill -x zns-agent > /dev/null 2>&1 || true",
- "mkdir -p " + PACKAGE_REMOTE_PATH,
- "mkdir -p /etc/zstack-zns",
+ "mkdir -p " + packageDir,
+ "mkdir -p " + configDir,
"mv " + remotePackage + " " + PACKAGE_PATH,🤖 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/sdnController/src/main/java/org/zstack/sdnController/znsagent/ZnsAgentInstaller.java`
around lines 101 - 106, In ZnsAgentInstaller (where the installer builds remote
shell commands) avoid hardcoding mkdir paths; replace the fixed "mkdir -p " +
PACKAGE_REMOTE_PATH and "/etc/zstack-zns" with mkdir commands that create the
directories derived from PACKAGE_PATH and CONFIG_PATH respectively so the
created directories match the actual deploy targets (use PACKAGE_PATH's parent
directory and CONFIG_PATH's parent directory), and ensure subsequent mv/chmod
operations still reference PACKAGE_PATH and CONFIG_PATH.
| "systemctl daemon-reload", | ||
| "systemctl enable " + SERVICE_NAME, | ||
| "systemctl restart " + SERVICE_NAME, | ||
| "curl -fsS http://127.0.0.1:8090/zns-agent/api/v1/health" |
There was a problem hiding this comment.
健康检查端口与配置项脱节。
Line 111 固定请求 127.0.0.1:8090，但配置文件实际监听地址来自 LISTEN_ADDRESS（Line 136）。监听端口调整后会出现“服务已启动但检查失败”的误报。
建议修复
- "curl -fsS http://127.0.0.1:8090/zns-agent/api/v1/health"
+ "curl -fsS " + buildHealthCheckUrl(LISTEN_ADDRESS)
)
.runAndClose();private static String buildHealthCheckUrl(String listenAddress) {
String addr = listenAddress == null ? "" : listenAddress.trim();
String host = "127.0.0.1";
String port = "8090";
int idx = addr.lastIndexOf(':');
if (idx > 0 && idx < addr.length() - 1) {
String cfgHost = addr.substring(0, idx);
host = "0.0.0.0".equals(cfgHost) ? "127.0.0.1" : cfgHost;
port = addr.substring(idx + 1);
}
return String.format("http://%s:%s/zns-agent/api/v1/health", host, port);
}📝 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
| "curl -fsS http://127.0.0.1:8090/zns-agent/api/v1/health" | |
| "curl -fsS " + buildHealthCheckUrl(LISTEN_ADDRESS) | |
| ) | |
| .runAndClose(); | |
| private static String buildHealthCheckUrl(String listenAddress) { | |
| String addr = listenAddress == null ? "" : listenAddress.trim(); | |
| String host = "127.0.0.1"; | |
| String port = "8090"; | |
| int idx = addr.lastIndexOf(':'); | |
| if (idx > 0 && idx < addr.length() - 1) { | |
| String cfgHost = addr.substring(0, idx); | |
| host = "0.0.0.0".equals(cfgHost) ? "127.0.0.1" : cfgHost; | |
| port = addr.substring(idx + 1); | |
| } | |
| return String.format("http://%s:%s/zns-agent/api/v1/health", host, port); | |
| } |
🤖 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/sdnController/src/main/java/org/zstack/sdnController/znsagent/ZnsAgentInstaller.java`
at line 111, The health check URL in ZnsAgentInstaller is hardcoded to
"127.0.0.1:8090" and will break when LISTEN_ADDRESS is changed; replace the
fixed URL with a builder that derives host and port from the LISTEN_ADDRESS
value (parse LISTEN_ADDRESS, split on the last ':' to get host and port, treat
"0.0.0.0" as "127.0.0.1", and fall back to defaults when LISTEN_ADDRESS is
null/invalid) and use that builder (e.g., buildHealthCheckUrl) wherever the
health check HTTP call is made so the agent checks the actual configured listen
address.
added 2 commits
June 8, 2026 14:33
Rename the prepare-service command, installer, global properties, and unit test from zns-agent to znsproxy. Align the command payload and host install config with the zns-proxy service. JIRA: ZCF-3882 Change-Id: Ia6f8ac77afb139ea2c45111cd65d0b8e53485108
Stop rendering zns-proxy TOML in Cloud. Upload the self-installing zns-proxy package, run its install command with the listen port, and only keep package resolution plus final health verification in Cloud. Test: - mvn -pl plugin/sdnController -am -DskipTests install - mvn test -Dtest=ZnsProxyInstallerTest -DskipJacoco=true JIRA: ZCF-3882 Change-Id: I43a5995e4e98731759ea487895dab949ebe9f49f
There was a problem hiding this comment.
♻️ Duplicate comments (3)
plugin/sdnController/src/main/java/org/zstack/sdnController/znsproxy/ZnsProxyInstaller.java (2)
70-79:⚠️ Potential issue | 🟠 Major | ⚡ Quick win避免硬编码目录，应动态从 PACKAGE_PATH 提取父目录。
当前 Line 73 使用
PACKAGE_REMOTE_PATH创建目录，但 Line 74-76 实际操作的是PACKAGE_PATH。若用户修改全局配置PACKAGE_PATH到其他位置，而PACKAGE_REMOTE_PATH未同步变更，会导致目录不存在而部署失败。建议改为动态提取
PACKAGE_PATH的父目录：String packageDir = new File(PACKAGE_PATH).getParent();然后在 Line 73 使用
"mkdir -p " + shellQuote(packageDir)。🤖 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/sdnController/src/main/java/org/zstack/sdnController/znsproxy/ZnsProxyInstaller.java` around lines 70 - 79, The upload/sudo block uses PACKAGE_REMOTE_PATH to create the directory but then moves files to PACKAGE_PATH, risking failure if PACKAGE_PATH differs; change the mkdir target to the parent directory of PACKAGE_PATH instead of PACKAGE_REMOTE_PATH: compute packageDir via new File(PACKAGE_PATH).getParent() and use "mkdir -p " + shellQuote(packageDir) in the SshResult/scpUpload().sudoCommand(...) sequence (symbols to update: PACKAGE_PATH, PACKAGE_REMOTE_PATH usage, shellQuote call within the SshResult ret block).
97-110:⚠️ Potential issue | 🟠 Major | ⚡ Quick winURL 下载分支应强制要求 sha256 校验。
当前实现中，若
cmd.sha256为空，Line 108 的verifySha256会直接返回（见 Line 177-179），导致从 URL 下载的包未经完整性校验即被安装，存在供应链攻击风险。建议在 Line 97 之后立即检查：
if (StringUtils.hasText(cmd.packageUrl)) { if (!StringUtils.hasText(cmd.sha256)) { throw new CloudRuntimeException("prepare zns-proxy service failed: sha256 is required when packageUrl is provided"); } // ... 继续下载逻辑 }🤖 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/sdnController/src/main/java/org/zstack/sdnController/znsproxy/ZnsProxyInstaller.java` around lines 97 - 110, The URL-download branch in ZnsProxyInstaller currently allows downloading when cmd.packageUrl is set but cmd.sha256 is empty, causing verifySha256(cmd.sha256) to be a no-op and skipping integrity checks; update the if (StringUtils.hasText(cmd.packageUrl)) branch to immediately validate that cmd.sha256 is present and non-empty and throw a CloudRuntimeException with a clear message if it's missing, before calling packageInRepository, FileUtils.copyURLToFile, or verifySha256; keep existing error handling around the download and still call verifySha256(target, cmd.sha256) afterwards.plugin/sdnController/src/main/java/org/zstack/sdnController/SdnControllerManagerImpl.java (1)
577-587:⚠️ Potential issue | 🟠 Major | ⚡ Quick win避免在日志中输出未脱敏的 packageUrl。
Line 581-583 将完整的
cmd.packageUrl打印到 info 日志中。若 URL 包含签名参数、访问令牌或凭据（例如预签名的对象存储 URL），会导致敏感信息泄露到日志系统。建议实现一个 URL 脱敏辅助方法，仅保留 scheme、host、port 和 path，移除 query、fragment 和 userinfo：
private static String sanitizeUrlForLog(String rawUrl) { if (rawUrl == null) { return null; } try { URL u = new URL(rawUrl); int port = u.getPort(); String authority = port > 0 ? (u.getHost() + ":" + port) : u.getHost(); return u.getProtocol() + "://" + authority + u.getPath(); } catch (Exception ignored) { return "<invalid-url>"; } }然后在 Line 583 使用
sanitizeUrlForLog(cmd.packageUrl)代替cmd.packageUrl。🤖 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/sdnController/src/main/java/org/zstack/sdnController/SdnControllerManagerImpl.java` around lines 577 - 587, The log currently prints the raw packageUrl from ZnsProxyPrepareServiceCmd in SdnControllerManagerImpl, which may leak credentials; add a helper method (e.g., sanitizeUrlForLog) that returns scheme://host[:port]/path and strips userinfo, query and fragment (returning null or "<invalid-url>" for null/parse errors), then replace cmd.packageUrl in the logger call inside the SyncHttpCallHandler with the sanitized result (sanitizeUrlForLog(cmd.packageUrl)) so logs never contain query strings or credentials.
🧹 Nitpick comments (2)
plugin/sdnController/src/main/java/org/zstack/sdnController/ZnsProxyGlobalProperty.java (1)
6-19: 💤 Low value可选：为配置类添加 Javadoc 注释。
建议在类上添加简要的 Javadoc 说明这些全局属性的用途（例如："ZNS Proxy 服务部署相关的全局配置项"），有助于其他开发者理解配置项的作用。
🤖 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/sdnController/src/main/java/org/zstack/sdnController/ZnsProxyGlobalProperty.java` around lines 6 - 19, Add a concise Javadoc block to the ZnsProxyGlobalProperty class explaining that it contains global configuration properties for ZNS Proxy service deployment (e.g., package path, remote/package repository path, and proxy package name) so other developers understand their purpose; place the Javadoc immediately above the class declaration for ZnsProxyGlobalProperty and mention the key fields PACKAGE_PATH, PACKAGE_REMOTE_PATH, PACKAGE_REPOSITORY_PATH, and PROXY_PACKAGE_NAME in one sentence.plugin/sdnController/src/main/java/org/zstack/sdnController/znsproxy/ZnsProxyPrepareServiceCmd.java (1)
3-15: 💤 Low value可选：为命令类添加 Javadoc 注释。
建议在类上添加简要说明，描述此命令的用途（例如："ZNS Proxy 服务远程部署准备命令的数据结构，通过 REST 同步调用接收"），有助于理解各字段的语义。
🤖 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/sdnController/src/main/java/org/zstack/sdnController/znsproxy/ZnsProxyPrepareServiceCmd.java` around lines 3 - 15, Add a concise Javadoc to the ZnsProxyPrepareServiceCmd class describing its purpose as the data transfer object for preparing ZNS Proxy service deployment (used by the REST endpoint at COMMAND_PATH = "/zns/notify/prepare-service"), and briefly document key fields such as computeManagerUuid, hostUuid, managementIp, sdnControllerUuid, proxyListenPort, proxyVersion, packageName, packageUrl and sha256 so callers understand their semantics and expected formats.
🤖 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.
Duplicate comments:
In
`@plugin/sdnController/src/main/java/org/zstack/sdnController/SdnControllerManagerImpl.java`:
- Around line 577-587: The log currently prints the raw packageUrl from
ZnsProxyPrepareServiceCmd in SdnControllerManagerImpl, which may leak
credentials; add a helper method (e.g., sanitizeUrlForLog) that returns
scheme://host[:port]/path and strips userinfo, query and fragment (returning
null or "<invalid-url>" for null/parse errors), then replace cmd.packageUrl in
the logger call inside the SyncHttpCallHandler with the sanitized result
(sanitizeUrlForLog(cmd.packageUrl)) so logs never contain query strings or
credentials.
In
`@plugin/sdnController/src/main/java/org/zstack/sdnController/znsproxy/ZnsProxyInstaller.java`:
- Around line 70-79: The upload/sudo block uses PACKAGE_REMOTE_PATH to create
the directory but then moves files to PACKAGE_PATH, risking failure if
PACKAGE_PATH differs; change the mkdir target to the parent directory of
PACKAGE_PATH instead of PACKAGE_REMOTE_PATH: compute packageDir via new
File(PACKAGE_PATH).getParent() and use "mkdir -p " + shellQuote(packageDir) in
the SshResult/scpUpload().sudoCommand(...) sequence (symbols to update:
PACKAGE_PATH, PACKAGE_REMOTE_PATH usage, shellQuote call within the SshResult
ret block).
- Around line 97-110: The URL-download branch in ZnsProxyInstaller currently
allows downloading when cmd.packageUrl is set but cmd.sha256 is empty, causing
verifySha256(cmd.sha256) to be a no-op and skipping integrity checks; update the
if (StringUtils.hasText(cmd.packageUrl)) branch to immediately validate that
cmd.sha256 is present and non-empty and throw a CloudRuntimeException with a
clear message if it's missing, before calling packageInRepository,
FileUtils.copyURLToFile, or verifySha256; keep existing error handling around
the download and still call verifySha256(target, cmd.sha256) afterwards.
---
Nitpick comments:
In
`@plugin/sdnController/src/main/java/org/zstack/sdnController/znsproxy/ZnsProxyPrepareServiceCmd.java`:
- Around line 3-15: Add a concise Javadoc to the ZnsProxyPrepareServiceCmd class
describing its purpose as the data transfer object for preparing ZNS Proxy
service deployment (used by the REST endpoint at COMMAND_PATH =
"/zns/notify/prepare-service"), and briefly document key fields such as
computeManagerUuid, hostUuid, managementIp, sdnControllerUuid, proxyListenPort,
proxyVersion, packageName, packageUrl and sha256 so callers understand their
semantics and expected formats.
In
`@plugin/sdnController/src/main/java/org/zstack/sdnController/ZnsProxyGlobalProperty.java`:
- Around line 6-19: Add a concise Javadoc block to the ZnsProxyGlobalProperty
class explaining that it contains global configuration properties for ZNS Proxy
service deployment (e.g., package path, remote/package repository path, and
proxy package name) so other developers understand their purpose; place the
Javadoc immediately above the class declaration for ZnsProxyGlobalProperty and
mention the key fields PACKAGE_PATH, PACKAGE_REMOTE_PATH,
PACKAGE_REPOSITORY_PATH, and PROXY_PACKAGE_NAME in one sentence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5549705a-0229-4b45-a807-dfbcf2832825
📒 Files selected for processing (5)
plugin/sdnController/src/main/java/org/zstack/sdnController/SdnControllerManagerImpl.javaplugin/sdnController/src/main/java/org/zstack/sdnController/ZnsProxyGlobalProperty.javaplugin/sdnController/src/main/java/org/zstack/sdnController/znsproxy/ZnsProxyInstaller.javaplugin/sdnController/src/main/java/org/zstack/sdnController/znsproxy/ZnsProxyPrepareServiceCmd.javatest/src/test/java/org/zstack/test/unittest/sdnController/ZnsProxyInstallerTest.java
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/src/test/java/org/zstack/test/unittest/sdnController/ZnsProxyInstallerTest.java (1)
22-24: ⚡ Quick win给
buildInstallCommand()再补一条带引号/空格路径的回归用例。当前只锁住了普通路径，没覆盖最容易回归的 shell 转义场景。这个方法最终直接拼进远端
sudoCommand，建议至少再加一个类似/tmp/zns proxy's.bin的 case，把单引号转义行为也固定下来。建议补测
public void testBuildInstallCommandUsesPackageDefaults() { String command = ZnsProxyInstaller.buildInstallCommand("/var/lib/zstack/zns-proxy/package/zns-proxy.bin"); assertEquals("'/var/lib/zstack/zns-proxy/package/zns-proxy.bin' install", command); + + String escaped = ZnsProxyInstaller.buildInstallCommand("/tmp/zns proxy's.bin"); + assertEquals("'/tmp/zns proxy'\"'\"'s.bin' install", escaped); }🤖 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/unittest/sdnController/ZnsProxyInstallerTest.java` around lines 22 - 24, Add a regression test to ZnsProxyInstallerTest that exercises ZnsProxyInstaller.buildInstallCommand with a path containing spaces and a single quote (e.g. "/tmp/zns proxy's.bin") to ensure shell-safe quoting/escaping; call buildInstallCommand with that path and assert the returned string matches the expected single-quoted/escaped command (so the produced token is safe to embed in the remote sudoCommand) to prevent regressions in shell-escaping behavior.
🤖 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/sdnController/src/main/java/org/zstack/sdnController/znsproxy/ZnsProxyInstaller.java`:
- Around line 39-44: The current install loop in ZnsProxyInstaller (where
normalizeHostUuids, resolvePackage and installOnHost are used) stops on the
first exception and leaves a partially applied state; change the for-loop around
installOnHost(hostUuid, localPackage) to catch exceptions per-host, record
failures (hostUuid -> exception or message) and continue processing remaining
hosts, then after the loop surface an aggregated failure/result (e.g., throw a
single exception listing failed host UUIDs and messages or return a composite
result) so callers get both successes and a summary of all failures; keep the
existing normalizeHostUuids, resolvePackage calls and ensure exception types and
logging use the same logger/exception class used elsewhere in this class.
---
Nitpick comments:
In
`@test/src/test/java/org/zstack/test/unittest/sdnController/ZnsProxyInstallerTest.java`:
- Around line 22-24: Add a regression test to ZnsProxyInstallerTest that
exercises ZnsProxyInstaller.buildInstallCommand with a path containing spaces
and a single quote (e.g. "/tmp/zns proxy's.bin") to ensure shell-safe
quoting/escaping; call buildInstallCommand with that path and assert the
returned string matches the expected single-quoted/escaped command (so the
produced token is safe to embed in the remote sudoCommand) to prevent
regressions in shell-escaping behavior.
🪄 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: 93d246d0-9259-42be-be85-c5ac6064e7a7
📒 Files selected for processing (4)
plugin/sdnController/src/main/java/org/zstack/sdnController/SdnControllerManagerImpl.javaplugin/sdnController/src/main/java/org/zstack/sdnController/znsproxy/ZnsProxyInstaller.javaplugin/sdnController/src/main/java/org/zstack/sdnController/znsproxy/ZnsProxyPrepareServiceCmd.javatest/src/test/java/org/zstack/test/unittest/sdnController/ZnsProxyInstallerTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- plugin/sdnController/src/main/java/org/zstack/sdnController/SdnControllerManagerImpl.java
Comment on lines
+39
to
+44
| List<String> hostUuids = normalizeHostUuids(cmd.hostUuids); | ||
|
|
||
| File localPackage = resolvePackage(cmd); | ||
| for (String hostUuid : hostUuids) { | ||
| installOnHost(hostUuid, localPackage); | ||
| } |
There was a problem hiding this comment.
批量安装在首个宿主失败时直接中断，会留下半成功状态。
这里改成 hostUuids 后，install() 仍然是顺序执行 + 首错即抛。结果就是：前面的宿主可能已经安装成功，后面的宿主却完全没尝试，而调用方只能拿到第一个失败信息。对于批量 prepare-service，至少应该继续处理剩余宿主并汇总失败结果，避免请求结果和实际部署状态严重背离。
建议修复
public void install(ZnsProxyPrepareServiceCmd cmd) {
if (cmd == null) {
throw new CloudRuntimeException("prepare zns-proxy service failed: command is empty");
}
List<String> hostUuids = normalizeHostUuids(cmd.hostUuids);
File localPackage = resolvePackage(cmd);
+ List<String> failures = new ArrayList<String>();
for (String hostUuid : hostUuids) {
- installOnHost(hostUuid, localPackage);
+ try {
+ installOnHost(hostUuid, localPackage);
+ } catch (CloudRuntimeException e) {
+ failures.add(String.format("%s: %s", hostUuid, e.getMessage()));
+ }
+ }
+ if (!failures.isEmpty()) {
+ throw new CloudRuntimeException("prepare zns-proxy service failed on hosts: " + String.join("; ", failures));
}
}🤖 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/sdnController/src/main/java/org/zstack/sdnController/znsproxy/ZnsProxyInstaller.java`
around lines 39 - 44, The current install loop in ZnsProxyInstaller (where
normalizeHostUuids, resolvePackage and installOnHost are used) stops on the
first exception and leaves a partially applied state; change the for-loop around
installOnHost(hostUuid, localPackage) to catch exceptions per-host, record
failures (hostUuid -> exception or message) and continue processing remaining
hosts, then after the loop surface an aggregated failure/result (e.g., throw a
single exception listing failed host UUIDs and messages or return a composite
result) so callers get both successes and a summary of all failures; keep the
existing normalizeHostUuids, resolvePackage calls and ensure exception types and
logging use the same logger/exception class used elsewhere in this class.
Accept hostUuids in prepare-service and install zns-proxy on each requested host. Drop unused management IP, controller UUID, and listen port fields so Cloud consumes only the package install request. Test: - mvn -pl plugin/sdnController -am -DskipTests install - mvn -f test/pom.xml -Dtest=ZnsProxyInstallerTest test JIRA: ZCF-3882 Change-Id: I3569ed797e21972fd91a4cad9dde532614b06e20
MatheMatrix
force-pushed
the
sync/dejing.liu/5.5.22
branch
from
60c6e46 to
b767d7f
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.
Support richer prepare-service inputs for zns-agent.\n\nThe installer now accepts explicit controller addresses,\npackage metadata, and URL-based package resolution with\nsha256 validation. Tests cover address normalization and\npackage lookup/download handling.\n\nJIRA: ZSTAC-3881\nRelated: ZCF-3881\nChange-Id: Ied352168ff0e37ee911e505bc746d35750b5df0b
sync from gitlab !10140