<fix>[zone]: validate management ip version (ZSTAC-85773)#4181
Open
zstack-robot-2 wants to merge 1 commit into
Open
<fix>[zone]: validate management ip version (ZSTAC-85773)#4181zstack-robot-2 wants to merge 1 commit into
zstack-robot-2 wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Warning
|
| Layer / File(s) | Summary |
|---|---|
Core interfaces and extension points header/src/main/java/org/zstack/header/zone/ManagementNetworkIpVersionManager.java, header/src/main/java/org/zstack/header/zone/ManagementNetworkIpVersionResourceExtensionPoint.java |
定义 ManagementNetworkIpVersionManager 接口与 ManagementNetworkIpVersionResourceExtensionPoint 扩展点。 |
IP version utilities utils/src/main/java/org/zstack/utils/network/ManagementNetworkIpVersionUtils.java |
提供 IP 版本归一化、端点 IPv4/IPv6 判断、IPv6 link-local 识别与端点主机提取等工具函数，兼容 URL/方括号 IPv6/NFS 风格等。 |
Zone system tags compute/src/main/java/org/zstack/compute/zone/ZoneSystemTags.java |
新增 MANAGEMENT_NETWORK_IP_VERSION 系统标签（managementNetwork::ipVersion::{ipVersion}），绑定到 ZoneVO。 |
Zone IP version manager implementation compute/src/main/java/org/zstack/compute/zone/ManagementNetworkIpVersionManagerImpl.java |
实现端点与 Zone ipVersion 匹配校验、Zone ipVersion 读取（缺失时默认 ipv4）、重复标签检测及既有资源兼容性校验，并提供系统标签事件的校验入口。 |
Zone manager integration compute/src/main/java/org/zstack/compute/zone/ZoneManagerImpl.java |
在 start() 中为 MANAGEMENT_NETWORK_IP_VERSION 注册 validator 与 judger，变更/删除操作触发兼容性校验。 |
Host and Storage Resource Validation
| Layer / File(s) | Summary |
|---|---|
Host management IP normalization and zone validation compute/src/main/java/org/zstack/compute/host/HostApiInterceptor.java |
在 APIAddHostMsg/APIUpdateHostMsg 校验中先规范化 managementIp（IPv6 canonical 返回），回写消息对象后再执行唯一性与 Zone 校验并调用管理器的 validateEndpointInZone。 |
Primary storage endpoint zone validation storage/src/main/java/org/zstack/storage/primary/PrimaryStorageApiInterceptor.java |
为 APIAddPrimaryStorageMsg、APIUpdatePrimaryStorageMsg、APIAttachPrimaryStorageToClusterMsg 添加 endpoint-in-zone 校验；本地类型跳过校验。 |
Backup storage endpoint zone validation storage/src/main/java/org/zstack/storage/backup/BackupStorageApiInterceptor.java |
在 APIAttachBackupStorageToZoneMsg 中解析备份存储 URL 的端点 IP 版本，若可解析则调用 validateEndpointInZone 进行 Zone 匹配校验。 |
SFTP backup storage endpoint and zone validation plugin/sftpBackupStorage/src/main/java/org/zstack/storage/backup/sftp/SftpBackupStorageApiInterceptor.java, plugin/sftpBackupStorage/src/main/java/org/zstack/storage/backup/sftp/SftpBackupStorageMetaDataMaker.java, plugin/sftpBackupStorage/src/main/java/org/zstack/storage/backup/sftp/SftpBackupStorage.java |
统一使用 IPv6NetworkUtils.isValidManagementEndpoint 校验 hostname；实现扩展点以校验已有 SFTP 备份存储；URL 构造改为 IPv6 兼容格式化。 |
Ceph mon endpoint zone IP version validation plugin/ceph/src/main/java/org/zstack/storage/ceph/CephApiInterceptor.java |
对 Ceph primary/backup mon 列表及 attach 场景增加 Zone IP 版本校验，替换/统一管理端点合法性校验并实现扩展点方法校验既有 mon。 |
Storage extension for zone resource validation storage/src/main/java/org/zstack/storage/ManagementNetworkIpVersionStorageExtension.java |
实现扩展点，按 Zone 遍历并校验已有 Primary/Backup 存储端点与目标 ipVersion 的匹配性。 |
Documentation and Constants
| Layer / File(s) | Summary |
|---|---|
API documentation IPv6 support header/src/main/java/org/zstack/header/host/APIAddHostMsg.java, header/src/main/java/org/zstack/header/host/APIUpdateHostMsgDoc_zh_cn.groovy, plugin/kvm/src/main/java/org/zstack/kvm/APIAddKVMHostMsgDoc_zh_cn.groovy, plugin/kvm/src/main/java/org/zstack/kvm/APIUpdateKVMHostMsgDoc_zh_cn.groovy |
更新 managementIp 字段描述以支持 IPv4/IPv6 或 DNS 名称。 |
Error codes and constant alignment utils/src/main/java/org/zstack/utils/clouderrorcode/CloudOperationsErrorCode.java, plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageConstants.java |
新增多组错误码常量以支持本次校验路径，且将本地存储类型常量对齐为 PrimaryStorageConstants.LOCAL_STORAGE_TYPE。 |
Integration and Unit Testing
| Layer / File(s) | Summary |
|---|---|
Zone IP version storage constraint integration test test/src/test/groovy/org/zstack/test/integration/storage/ManagementNetworkIpVersionStorageConstraintCase.groovy |
新增集成测试，覆盖默认 Zone ipVersion、系统标签写入/更新/重复校验、IPv6 link-local 拒绝、以及在存在 Host/Primary/Backup/SFTP/Ceph mon 等资源时阻止 Zone ipVersion 变更或删除的约束场景。 |
IP version utilities and Ceph mon URI unit tests test/src/test/groovy/org/zstack/test/unittest/utils/NetworkUtilsCase.groovy, test/src/test/groovy/org/zstack/compute/host/HostApiInterceptorIpv6Case.groovy, test/src/test/groovy/org/zstack/test/integration/kvm/host/KvmHostIpv6Case.groovy |
补充 ManagementNetworkIpVersionUtils 的归一化、端点解析、link-local 判定与 Ceph MonUri IPv6 解析测试；验证 Host API 的 IPv6 canonical 返回；为 KVM 测试环境新增 Zone ipVersion 系统标签。 |
🎯 4 (Complex) | ⏱️ ~60 minutes
"🐰 我在标签间跳跃，检查每个端点的模样，
IPv4 或 IPv6，规则都要量量当当。
区域标签立定序，已有资源一一问候，
管理网路齐一化，审查无一遗漏的周到。"
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 5.13% 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 | PR 标题准确总结了主要变更：通过添加区域级管理网络 IP 版本系统标签验证来解决相关问题。 |
| Description check | ✅ Passed | PR 描述详尽阐述了根本原因、修复方案、验证结果和关联的 Jira 问题，与变更集高度相关。 |
| 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 unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
sync/shixin.ruan/shixin-ZSTAC-85773@@2
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: 1
🧹 Nitpick comments (7)
test/src/test/groovy/org/zstack/test/integration/storage/ManagementNetworkIpVersionStorageConstraintCase.groovy (2)
141-153: 💤 Low value建议为混合存储测试场景添加说明注释。
该测试在第 149 行创建 IPv6 主存储时，第 143 行创建的 IPv4 主存储并未清理，导致 Zone 内同时存在两种 IP 版本的主存储。虽然这种设计是为了双向验证约束（IPv4→IPv6 和 IPv6→IPv4 均应失败），但测试流程可能不够直观。
建议在第 149 行前添加注释说明此处有意保留 IPv4 存储，以验证混合场景下的双向约束。
💡 建议的注释
} + // Keep IPv4 storage; create IPv6 storage to test bidirectional constraint createPrimaryStorage(zone.uuid, IPV6_NFS_URL) expect(ApiMessageInterceptionException.class) {🤖 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/groovy/org/zstack/test/integration/storage/ManagementNetworkIpVersionStorageConstraintCase.groovy` around lines 141 - 153, Add a clarifying comment inside the testExistingPrimaryStorageBlocksZoneIpVersionChange test before the second createPrimaryStorage(...) call (the one passing ManagementNetworkIpVersionUtils.IPV6) explaining that the earlier IPv4 primary storage created by createPrimaryStorage(...) is intentionally left in place to create a mixed-IP-version primary storage scenario so the subsequent manager.validateZoneCompatibleWithExistingResources(...) calls assert that both IPv4→IPv6 and IPv6→IPv4 transitions are blocked; reference the createPrimaryStorage and manager.validateZoneCompatibleWithExistingResources symbols in the comment for clarity.
274-312: 💤 Low value建议为 Ceph 存储的 "not used" url 字段添加说明。
第 280 行和 299 行将 Ceph 存储的
url字段设置为字符串字面值"not used"。虽然 Ceph 存储实际使用 Mon 主机名而非 url 字段作为端点，但该字面值可能引起困惑。建议添加注释说明 Ceph 存储的 url 字段仅用于满足 schema 要求，实际端点由 Mon VO 提供。
💡 建议的注释
ps.zoneUuid = zoneUuid ps.type = CephConstants.CEPH_PRIMARY_STORAGE_TYPE + // Ceph uses mon hostnames; url field is schema-required placeholder ps.url = "not used" ps.state = PrimaryStorageState.Enabledbs.type = CephConstants.CEPH_BACKUP_STORAGE_TYPE + // Ceph uses mon hostnames; url field is schema-required placeholder bs.url = "not used" bs.state = BackupStorageState.Enabled🤖 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/groovy/org/zstack/test/integration/storage/ManagementNetworkIpVersionStorageConstraintCase.groovy` around lines 274 - 312, In both createCephPrimaryStorage and createCephBackupStorage, the CephPrimaryStorageVO.url and CephBackupStorageVO.url are set to the literal "not used" which can confuse readers; update those methods to replace the literal with a short clarifying comment (next to the url assignment) that the url field is only present to satisfy the schema and is not used by Ceph (the actual endpoint comes from CephPrimaryStorageMonVO/CephBackupStorageMonVO via mon.hostname), and optionally consider using a named constant like UNUSED_URL to make intent explicit.utils/src/main/java/org/zstack/utils/network/ManagementNetworkIpVersionUtils.java (2)
13-13: ⚡ Quick win常量语义过载导致可读性下降
PORT_SEPARATOR常量在代码中承担了两种不同的语义角色：
- Line 50：用于判断字符串是否包含
:（检测 IPv6 地址格式，因为 IPv6 地址固有地包含:）- Line 93：用作实际的端口分隔符（区分主机与端口）
根据编码规范第 2 节"命名与格式规范"，常量命名要求"表达清楚，避免使用含糊或不准确的名称"。建议：
- 将 line 50 的判断改为直接调用
IPv6NetworkUtils.isIpv6Address(host)明确语义- 或引入单独的常量（如
IPV6_SEPARATOR）用于 IPv6 格式检测♻️ 建议重构方案
方案 1（推荐）：直接使用语义明确的判断
public static boolean isIpv6LinkLocalEndpoint(String endpoint) { String host = extractEndpointHost(endpoint); - if (StringUtils.isBlank(host) || !host.contains(PORT_SEPARATOR)) { + if (StringUtils.isBlank(host) || !IPv6NetworkUtils.isIpv6Address(host.split(IPV6_SCOPE_SEPARATOR)[0])) { return false; }方案 2：引入语义明确的常量
+ private static final String IPV6_ADDRESS_SEPARATOR = ":"; public static boolean isIpv6LinkLocalEndpoint(String endpoint) { String host = extractEndpointHost(endpoint); - if (StringUtils.isBlank(host) || !host.contains(PORT_SEPARATOR)) { + if (StringUtils.isBlank(host) || !host.contains(IPV6_ADDRESS_SEPARATOR)) { return false; }🤖 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 `@utils/src/main/java/org/zstack/utils/network/ManagementNetworkIpVersionUtils.java` at line 13, The PORT_SEPARATOR constant is being used for two different semantics (detecting IPv6 vs separating host:port) which harms readability; update ManagementNetworkIpVersionUtils to stop using PORT_SEPARATOR for IPv6 detection—either replace the check that tests for ':' with a clear semantic call IPv6NetworkUtils.isIpv6Address(host) (preferred) or introduce a new constant like IPV6_SEPARATOR for the IPv6-check and keep PORT_SEPARATOR for host:port splitting; locate usages in methods inside ManagementNetworkIpVersionUtils where PORT_SEPARATOR is used for the ':' presence test and where it is used to split host and port and adjust accordingly.Source: Coding guidelines
59-99: ⚡ Quick win复杂方法缺少注释说明
extractEndpointHost方法处理多种端点格式（URI scheme、user-info、IPv6 方括号、NFS:/、端口等），逻辑复杂度较高但缺少方法级注释和关键步骤说明。根据编码规范第 3 节"编写自解释代码"，对于较复杂的逻辑应配有注释以提升可维护性。建议补充：
- 方法级 Javadoc 说明支持的端点格式（如
http://[::1]:8080/path、user@host:port、host:/path）- 关键分支的注释（如为何先检查方括号、为何 NFS 格式优先于端口分隔）
📝 建议补充注释示例
+ /** + * 从端点字符串中提取主机部分 + * <p> + * 支持的端点格式包括： + * <ul> + * <li>带 scheme: {`@code` http://host:port/path}</li> + * <li>带 user-info: {`@code` user@host:port}</li> + * <li>IPv6 方括号: {`@code` [::1]:8080} 或 {`@code` http://[::1]/path}</li> + * <li>裸 IPv6: {`@code` 2001:db8::1}</li> + * <li>NFS 风格: {`@code` host:/path}</li> + * <li>带端口: {`@code` host:8080}</li> + * <li>单主机: {`@code` host} 或 {`@code` hostname.domain}</li> + * </ul> + * + * `@param` endpoint 端点字符串 + * `@return` 提取的主机部分，无法解析时返回 null + */ public static String extractEndpointHost(String endpoint) { if (StringUtils.isBlank(endpoint)) { return null; } String value = endpoint.trim(); + // 1. 移除 URI scheme (如 http://, https://) int schemeIndex = value.indexOf(URI_SCHEME_SEPARATOR); if (schemeIndex >= 0) { value = value.substring(schemeIndex + URI_SCHEME_SEPARATOR.length()); } + // 2. 移除 user-info (如 user@) if (value.contains(USER_INFO_SEPARATOR)) { value = value.substring(value.lastIndexOf(USER_INFO_SEPARATOR) + USER_INFO_SEPARATOR.length()); } + // 3. 处理方括号包裹的 IPv6 地址 if (value.startsWith(IPV6_HOST_PREFIX)) { return extractBracketIpv6Host(value); } String hostBeforePath = value; int pathIndex = hostBeforePath.indexOf(PATH_SEPARATOR); if (pathIndex >= 0) { hostBeforePath = hostBeforePath.substring(0, pathIndex); } + // 4. 检查是否为裸 IPv6 地址（无方括号） if (IPv6NetworkUtils.isIpv6Address(hostBeforePath)) { return hostBeforePath; } + // 5. 处理 NFS 风格的 host:/path 格式（优先级高于端口分隔） int nfsIndex = value.indexOf(NFS_URL_SEPARATOR); if (nfsIndex >= 0) { return value.substring(0, nfsIndex); } + // 6. 处理 host:port 格式（仅当恰有一个 ':' 时视为端口分隔符） int portIndex = hostBeforePath.indexOf(PORT_SEPARATOR); if (portIndex >= 0 && hostBeforePath.indexOf(PORT_SEPARATOR, portIndex + PORT_SEPARATOR.length()) < 0) { return hostBeforePath.substring(0, portIndex); } return hostBeforePath; }🤖 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 `@utils/src/main/java/org/zstack/utils/network/ManagementNetworkIpVersionUtils.java` around lines 59 - 99, The extractEndpointHost method mixes parsing for URI schemes, user-info, bracketed IPv6, NFS-style "host:/" and ports but has no documentation; add a method-level Javadoc on extractEndpointHost that enumerates supported endpoint patterns (e.g. "http://[::1]:8080/path", "user@host:port", "host:/path", plain host:port) and the expected return value, and add short inline comments at key steps (after trimming/scheme strip, after user-info removal, before the IPV6_HOST_PREFIX check referencing extractBracketIpv6Host, before the NFS_URL_SEPARATOR check explaining why NFS ':/'' is handled prior to port parsing, and before port splitting) to explain the rationale and assumptions and reference constants URI_SCHEME_SEPARATOR, USER_INFO_SEPARATOR, IPV6_HOST_PREFIX, NFS_URL_SEPARATOR and helper extractBracketIpv6Host/IPv6NetworkUtils.Source: Coding guidelines
compute/src/main/java/org/zstack/compute/zone/ManagementNetworkIpVersionManagerImpl.java (2)
69-77: 💤 Low valuegetZoneIpVersion 可能返回 null 的边界情况缺少说明
Lines 75-76 调用
ManagementNetworkIpVersionUtils.normalizeIpVersion(...)可能返回null（当系统标签值非法时）。虽然 Lines 104-112 的validateZoneIpVersionTag会在标签创建/更新时校验值的合法性，理论上不应存储非法值，但在以下边界情况下仍可能返回null：
- 数据库标签值被手动篡改
- 标签验证逻辑存在漏洞
当前调用方（Line 42、Line 137）通过 null 检查（Line 48）处理了该情况，但缺少文档说明。建议：
- 在方法上添加
@returnJavadoc 说明可能返回 null 的情况- 或在 Line 76 后添加防御性检查，当
normalizeIpVersion返回 null 时记录警告日志并返回默认值♻️ 建议增强方案（可选）
方案 1：添加 Javadoc 说明
+ /** + * 获取区域的管理网络 IP 版本 + * + * `@param` zoneUuid 区域 UUID + * `@return` 规范化的 IP 版本（"ipv4" 或 "ipv6"），未配置标签时返回默认值 "ipv4"； + * 若标签值非法（不应发生，除非数据被篡改）则返回 null + */ `@Override` public String getZoneIpVersion(String zoneUuid) {方案 2：增加防御性处理
`@Override` public String getZoneIpVersion(String zoneUuid) { Map<String, String> tokens = ZoneSystemTags.MANAGEMENT_NETWORK_IP_VERSION.getTokensByResourceUuid(zoneUuid); if (tokens == null) { return DEFAULT_ZONE_IP_VERSION; } - return ManagementNetworkIpVersionUtils.normalizeIpVersion( + String normalized = ManagementNetworkIpVersionUtils.normalizeIpVersion( tokens.get(ZoneSystemTags.MANAGEMENT_NETWORK_IP_VERSION_TOKEN)); + if (normalized == null) { + logger.warn(String.format("Zone[uuid:%s] has invalid ipVersion tag value, falling back to default %s", + zoneUuid, DEFAULT_ZONE_IP_VERSION)); + return DEFAULT_ZONE_IP_VERSION; + } + return normalized; }🤖 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 `@compute/src/main/java/org/zstack/compute/zone/ManagementNetworkIpVersionManagerImpl.java` around lines 69 - 77, getZoneIpVersion may return null because ManagementNetworkIpVersionUtils.normalizeIpVersion(...) can yield null for malformed tag values; update getZoneIpVersion to handle that by adding defensive logic: call normalizeIpVersion on tokens.get(...), if result is null log a warning (including zoneUuid and the raw tag value) and return DEFAULT_ZONE_IP_VERSION; alternatively also add a `@return` Javadoc to getZoneIpVersion documenting that it can fall back to DEFAULT_ZONE_IP_VERSION when the stored tag is invalid—refer to the getZoneIpVersion method and ManagementNetworkIpVersionUtils.normalizeIpVersion and the validateZoneIpVersionTag validator when making the change.
88-116: 💤 Low value公开方法 validateZoneIpVersionTag 的设计意图缺少说明
validateZoneIpVersionTag方法（Lines 88-116）声明为public，但未出现在ManagementNetworkIpVersionManager接口中。通过 Line 237、243 可知，该方法专门用于ZoneManagerImpl中集成系统标签验证框架。这种设计形成了双重职责：
- 接口方法（Lines 37-77）：供其他组件（如拦截器）调用的端点校验能力
- 标签框架方法（Lines 88-116）：供标签生命周期管理调用的校验入口
建议在类或方法上添加注释说明
validateZoneIpVersionTag的使用场景，避免其他开发者误用。📝 建议补充注释示例
+ /** + * 验证 Zone 管理网络 ipVersion 系统标签的合法性 + * <p> + * 该方法由标签框架（TagManager）在标签创建时调用，不应在业务代码中直接使用。 + * 若需验证端点与 Zone 的兼容性，请使用 {`@link` `#validateEndpointInZone`} 或 {`@link` `#validateEndpointMatchesIpVersion`}。 + * + * `@param` resourceUuid Zone UUID + * `@param` systemTag 系统标签字符串 + */ public void validateZoneIpVersionTag(String resourceUuid, String systemTag) { validateZoneIpVersionTag(resourceUuid, systemTag, null); } + /** + * 验证 Zone 管理网络 ipVersion 系统标签的合法性（支持排除指定标签 UUID） + * <p> + * 该方法由标签框架在标签更新时调用，用于排除旧标签自身的冲突检查。 + * + * `@param` resourceUuid Zone UUID + * `@param` systemTag 系统标签字符串 + * `@param` excludedTagUuid 需排除的标签 UUID（用于更新场景） + */ public void validateZoneIpVersionTag(String resourceUuid, String systemTag, String excludedTagUuid) {🤖 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 `@compute/src/main/java/org/zstack/compute/zone/ManagementNetworkIpVersionManagerImpl.java` around lines 88 - 116, The public validateZoneIpVersionTag method is intended only for the zone tag validation lifecycle invoked by the tag framework (used from ZoneManagerImpl) rather than as part of the ManagementNetworkIpVersionManager API; add a brief Javadoc on the validateZoneIpVersionTag method (or on the class) stating its intended use: that it is a tag-framework callback used by ZoneManagerImpl for system tag validation, not a general-purpose public API, and mention the expected parameters (resourceUuid, systemTag, optional excludedTagUuid) and why it is public (framework visibility). Reference the method name validateZoneIpVersionTag, the interface ManagementNetworkIpVersionManager, and the caller ZoneManagerImpl in the comment so future maintainers understand the intended usage and avoid misuse.storage/src/main/java/org/zstack/storage/backup/BackupStorageApiInterceptor.java (1)
136-143: ⚡ Quick win补充空值检查以提高防御性。
第 137 行使用
dbf.findByUuid加载BackupStorageVO，理论上由于框架的APIParam验证机制已确保该备份存储存在，但findByUuid本身可返回null。建议添加空值检查以提高代码防御性，避免后续 NPE 风险。🛡️ 建议的防御性检查
BackupStorageVO vo = dbf.findByUuid(msg.getBackupStorageUuid(), BackupStorageVO.class); +if (vo == null) { + throw new ApiMessageInterceptionException(operr(ORG_ZSTACK_STORAGE_BACKUP_10006, + "backup storage[uuid:%s] not found", msg.getBackupStorageUuid())); +} if (ManagementNetworkIpVersionUtils.getEndpointIpVersion(vo.getUrl()) == null) { return; }🤖 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 `@storage/src/main/java/org/zstack/storage/backup/BackupStorageApiInterceptor.java` around lines 136 - 143, After calling dbf.findByUuid to load BackupStorageVO (BackupStorageVO vo = dbf.findByUuid(...)), add a null check for vo before using it: if vo is null, return early (or handle appropriately) to avoid NPEs when calling ManagementNetworkIpVersionUtils.getEndpointIpVersion(vo.getUrl()) and managementNetworkIpVersionManager.validateEndpointInZone(...). This change affects the block that retrieves the VO and then calls getEndpointIpVersion and validateEndpointInZone (references: BackupStorageVO, dbf.findByUuid, ManagementNetworkIpVersionUtils.getEndpointIpVersion, managementNetworkIpVersionManager.validateEndpointInZone, msg.getBackupStorageUuid).
🤖 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
`@header/src/main/java/org/zstack/header/zone/ManagementNetworkIpVersionManager.java`:
- Around line 3-12: 为接口 ManagementNetworkIpVersionManager 的四个方法补全 Javadoc 注释：为
validateEndpointInZone、validateEndpointMatchesIpVersion、validateZoneCompatibleWithExistingResources
和 getZoneIpVersion 分别添加简要描述、每个参数（例如说明 endpoint 的格式/示例、resourceType 与
resourceIdentity 的区别、errorCode 的用途）的含义、返回值（若有）以及可能抛出的异常类型和触发条件；特别说明
validateEndpointInZone 与 validateEndpointMatchesIpVersion 的职责差异（前者检查 endpoint
是否属于指定 zone，后者检查 endpoint 是否与指定 ipVersion 匹配）并给出示例或约束格式以便自解释。
---
Nitpick comments:
In
`@compute/src/main/java/org/zstack/compute/zone/ManagementNetworkIpVersionManagerImpl.java`:
- Around line 69-77: getZoneIpVersion may return null because
ManagementNetworkIpVersionUtils.normalizeIpVersion(...) can yield null for
malformed tag values; update getZoneIpVersion to handle that by adding defensive
logic: call normalizeIpVersion on tokens.get(...), if result is null log a
warning (including zoneUuid and the raw tag value) and return
DEFAULT_ZONE_IP_VERSION; alternatively also add a `@return` Javadoc to
getZoneIpVersion documenting that it can fall back to DEFAULT_ZONE_IP_VERSION
when the stored tag is invalid—refer to the getZoneIpVersion method and
ManagementNetworkIpVersionUtils.normalizeIpVersion and the
validateZoneIpVersionTag validator when making the change.
- Around line 88-116: The public validateZoneIpVersionTag method is intended
only for the zone tag validation lifecycle invoked by the tag framework (used
from ZoneManagerImpl) rather than as part of the
ManagementNetworkIpVersionManager API; add a brief Javadoc on the
validateZoneIpVersionTag method (or on the class) stating its intended use: that
it is a tag-framework callback used by ZoneManagerImpl for system tag
validation, not a general-purpose public API, and mention the expected
parameters (resourceUuid, systemTag, optional excludedTagUuid) and why it is
public (framework visibility). Reference the method name
validateZoneIpVersionTag, the interface ManagementNetworkIpVersionManager, and
the caller ZoneManagerImpl in the comment so future maintainers understand the
intended usage and avoid misuse.
In
`@storage/src/main/java/org/zstack/storage/backup/BackupStorageApiInterceptor.java`:
- Around line 136-143: After calling dbf.findByUuid to load BackupStorageVO
(BackupStorageVO vo = dbf.findByUuid(...)), add a null check for vo before using
it: if vo is null, return early (or handle appropriately) to avoid NPEs when
calling ManagementNetworkIpVersionUtils.getEndpointIpVersion(vo.getUrl()) and
managementNetworkIpVersionManager.validateEndpointInZone(...). This change
affects the block that retrieves the VO and then calls getEndpointIpVersion and
validateEndpointInZone (references: BackupStorageVO, dbf.findByUuid,
ManagementNetworkIpVersionUtils.getEndpointIpVersion,
managementNetworkIpVersionManager.validateEndpointInZone,
msg.getBackupStorageUuid).
In
`@test/src/test/groovy/org/zstack/test/integration/storage/ManagementNetworkIpVersionStorageConstraintCase.groovy`:
- Around line 141-153: Add a clarifying comment inside the
testExistingPrimaryStorageBlocksZoneIpVersionChange test before the second
createPrimaryStorage(...) call (the one passing
ManagementNetworkIpVersionUtils.IPV6) explaining that the earlier IPv4 primary
storage created by createPrimaryStorage(...) is intentionally left in place to
create a mixed-IP-version primary storage scenario so the subsequent
manager.validateZoneCompatibleWithExistingResources(...) calls assert that both
IPv4→IPv6 and IPv6→IPv4 transitions are blocked; reference the
createPrimaryStorage and manager.validateZoneCompatibleWithExistingResources
symbols in the comment for clarity.
- Around line 274-312: In both createCephPrimaryStorage and
createCephBackupStorage, the CephPrimaryStorageVO.url and
CephBackupStorageVO.url are set to the literal "not used" which can confuse
readers; update those methods to replace the literal with a short clarifying
comment (next to the url assignment) that the url field is only present to
satisfy the schema and is not used by Ceph (the actual endpoint comes from
CephPrimaryStorageMonVO/CephBackupStorageMonVO via mon.hostname), and optionally
consider using a named constant like UNUSED_URL to make intent explicit.
In
`@utils/src/main/java/org/zstack/utils/network/ManagementNetworkIpVersionUtils.java`:
- Line 13: The PORT_SEPARATOR constant is being used for two different semantics
(detecting IPv6 vs separating host:port) which harms readability; update
ManagementNetworkIpVersionUtils to stop using PORT_SEPARATOR for IPv6
detection—either replace the check that tests for ':' with a clear semantic call
IPv6NetworkUtils.isIpv6Address(host) (preferred) or introduce a new constant
like IPV6_SEPARATOR for the IPv6-check and keep PORT_SEPARATOR for host:port
splitting; locate usages in methods inside ManagementNetworkIpVersionUtils where
PORT_SEPARATOR is used for the ':' presence test and where it is used to split
host and port and adjust accordingly.
- Around line 59-99: The extractEndpointHost method mixes parsing for URI
schemes, user-info, bracketed IPv6, NFS-style "host:/" and ports but has no
documentation; add a method-level Javadoc on extractEndpointHost that enumerates
supported endpoint patterns (e.g. "http://[::1]:8080/path", "user@host:port",
"host:/path", plain host:port) and the expected return value, and add short
inline comments at key steps (after trimming/scheme strip, after user-info
removal, before the IPV6_HOST_PREFIX check referencing extractBracketIpv6Host,
before the NFS_URL_SEPARATOR check explaining why NFS ':/'' is handled prior to
port parsing, and before port splitting) to explain the rationale and
assumptions and reference constants URI_SCHEME_SEPARATOR, USER_INFO_SEPARATOR,
IPV6_HOST_PREFIX, NFS_URL_SEPARATOR and helper
extractBracketIpv6Host/IPv6NetworkUtils.
🪄 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: dc966407-c819-4b94-a55c-90efb34f6b63
⛔ Files ignored due to path filters (5)
conf/springConfigXml/PrimaryStorageManager.xmlis excluded by!**/*.xmlconf/springConfigXml/SftpBackupStorage.xmlis excluded by!**/*.xmlconf/springConfigXml/ZoneManager.xmlis excluded by!**/*.xmlconf/springConfigXml/ceph.xmlis excluded by!**/*.xmltest/src/test/resources/unitTestSuiteXml/BackupStorageManager.xmlis excluded by!**/*.xml
📒 Files selected for processing (21)
compute/src/main/java/org/zstack/compute/host/HostApiInterceptor.javacompute/src/main/java/org/zstack/compute/zone/ManagementNetworkIpVersionManagerImpl.javacompute/src/main/java/org/zstack/compute/zone/ZoneManagerImpl.javacompute/src/main/java/org/zstack/compute/zone/ZoneSystemTags.javaheader/src/main/java/org/zstack/header/host/APIAddHostMsg.javaheader/src/main/java/org/zstack/header/host/APIUpdateHostMsgDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/zone/ManagementNetworkIpVersionManager.javaheader/src/main/java/org/zstack/header/zone/ManagementNetworkIpVersionResourceExtensionPoint.javaplugin/ceph/src/main/java/org/zstack/storage/ceph/CephApiInterceptor.javaplugin/kvm/src/main/java/org/zstack/kvm/APIAddKVMHostMsgDoc_zh_cn.groovyplugin/kvm/src/main/java/org/zstack/kvm/APIUpdateKVMHostMsgDoc_zh_cn.groovyplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageConstants.javaplugin/sftpBackupStorage/src/main/java/org/zstack/storage/backup/sftp/SftpBackupStorageApiInterceptor.javastorage/src/main/java/org/zstack/storage/ManagementNetworkIpVersionStorageExtension.javastorage/src/main/java/org/zstack/storage/backup/BackupStorageApiInterceptor.javastorage/src/main/java/org/zstack/storage/primary/PrimaryStorageApiInterceptor.javatest/src/test/groovy/org/zstack/compute/host/HostApiInterceptorIpv6Case.groovytest/src/test/groovy/org/zstack/test/integration/storage/ManagementNetworkIpVersionStorageConstraintCase.groovytest/src/test/groovy/org/zstack/test/unittest/utils/NetworkUtilsCase.groovyutils/src/main/java/org/zstack/utils/clouderrorcode/CloudOperationsErrorCode.javautils/src/main/java/org/zstack/utils/network/ManagementNetworkIpVersionUtils.java
Collaborator
|
Comment from gitlab: 自上次添加REVIEWED标签(2026-06-08 21:35:51.000Z)后, 有新的COMMIT更新(2026-06-09 14:35:07.703Z), 所以移除了REVIEWED标签 |
MatheMatrix
force-pushed
the
sync/shixin.ruan/shixin-ZSTAC-85773@@2
branch
3 times, most recently
from
53a14fa to
a2350c5
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/src/test/groovy/org/zstack/test/integration/kvm/host/KvmHostIpv6Case.groovy (1)
56-60: ⚡ Quick win建议避免硬编码 zone 管理网络标签字符串。
这里直接写死
managementNetwork::ipVersion::ipv6，与ZoneSystemTags契约存在重复；若上游 pattern 变更，测试会隐式漂移。建议改为复用ZoneSystemTags.MANAGEMENT_NETWORK_IP_VERSION相关常量/构造方式生成标签。🤖 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/groovy/org/zstack/test/integration/kvm/host/KvmHostIpv6Case.groovy` around lines 56 - 60, Replace the hard-coded management network tag string in the createSystemTag block with the canonical constant or builder from ZoneSystemTags (e.g. use ZoneSystemTags.MANAGEMENT_NETWORK_IP_VERSION or its helper method) so the test uses the same tag construction as production; update the createSystemTag call that sets resourceUuid = cluster.zoneUuid and resourceType = ZoneVO.simpleName to set tag using that ZoneSystemTags constant/formatter instead of the literal "managementNetwork::ipVersion::ipv6".
🤖 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.
Nitpick comments:
In
`@test/src/test/groovy/org/zstack/test/integration/kvm/host/KvmHostIpv6Case.groovy`:
- Around line 56-60: Replace the hard-coded management network tag string in the
createSystemTag block with the canonical constant or builder from ZoneSystemTags
(e.g. use ZoneSystemTags.MANAGEMENT_NETWORK_IP_VERSION or its helper method) so
the test uses the same tag construction as production; update the
createSystemTag call that sets resourceUuid = cluster.zoneUuid and resourceType
= ZoneVO.simpleName to set tag using that ZoneSystemTags constant/formatter
instead of the literal "managementNetwork::ipVersion::ipv6".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5b00fad0-6526-4b9e-81eb-c96941dd5bab
⛔ Files ignored due to path filters (5)
conf/springConfigXml/PrimaryStorageManager.xmlis excluded by!**/*.xmlconf/springConfigXml/SftpBackupStorage.xmlis excluded by!**/*.xmlconf/springConfigXml/ZoneManager.xmlis excluded by!**/*.xmlconf/springConfigXml/ceph.xmlis excluded by!**/*.xmltest/src/test/resources/unitTestSuiteXml/BackupStorageManager.xmlis excluded by!**/*.xml
📒 Files selected for processing (24)
compute/src/main/java/org/zstack/compute/host/HostApiInterceptor.javacompute/src/main/java/org/zstack/compute/zone/ManagementNetworkIpVersionManagerImpl.javacompute/src/main/java/org/zstack/compute/zone/ZoneManagerImpl.javacompute/src/main/java/org/zstack/compute/zone/ZoneSystemTags.javaheader/src/main/java/org/zstack/header/host/APIAddHostMsg.javaheader/src/main/java/org/zstack/header/host/APIUpdateHostMsgDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/zone/ManagementNetworkIpVersionManager.javaheader/src/main/java/org/zstack/header/zone/ManagementNetworkIpVersionResourceExtensionPoint.javaplugin/ceph/src/main/java/org/zstack/storage/ceph/CephApiInterceptor.javaplugin/kvm/src/main/java/org/zstack/kvm/APIAddKVMHostMsgDoc_zh_cn.groovyplugin/kvm/src/main/java/org/zstack/kvm/APIUpdateKVMHostMsgDoc_zh_cn.groovyplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageConstants.javaplugin/sftpBackupStorage/src/main/java/org/zstack/storage/backup/sftp/SftpBackupStorage.javaplugin/sftpBackupStorage/src/main/java/org/zstack/storage/backup/sftp/SftpBackupStorageApiInterceptor.javaplugin/sftpBackupStorage/src/main/java/org/zstack/storage/backup/sftp/SftpBackupStorageMetaDataMaker.javastorage/src/main/java/org/zstack/storage/ManagementNetworkIpVersionStorageExtension.javastorage/src/main/java/org/zstack/storage/backup/BackupStorageApiInterceptor.javastorage/src/main/java/org/zstack/storage/primary/PrimaryStorageApiInterceptor.javatest/src/test/groovy/org/zstack/compute/host/HostApiInterceptorIpv6Case.groovytest/src/test/groovy/org/zstack/test/integration/kvm/host/KvmHostIpv6Case.groovytest/src/test/groovy/org/zstack/test/integration/storage/ManagementNetworkIpVersionStorageConstraintCase.groovytest/src/test/groovy/org/zstack/test/unittest/utils/NetworkUtilsCase.groovyutils/src/main/java/org/zstack/utils/clouderrorcode/CloudOperationsErrorCode.javautils/src/main/java/org/zstack/utils/network/ManagementNetworkIpVersionUtils.java
✅ Files skipped from review due to trivial changes (7)
- header/src/main/java/org/zstack/header/host/APIAddHostMsg.java
- plugin/kvm/src/main/java/org/zstack/kvm/APIAddKVMHostMsgDoc_zh_cn.groovy
- plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageConstants.java
- plugin/sftpBackupStorage/src/main/java/org/zstack/storage/backup/sftp/SftpBackupStorageMetaDataMaker.java
- utils/src/main/java/org/zstack/utils/clouderrorcode/CloudOperationsErrorCode.java
- test/src/test/groovy/org/zstack/compute/host/HostApiInterceptorIpv6Case.groovy
- header/src/main/java/org/zstack/header/host/APIUpdateHostMsgDoc_zh_cn.groovy
🚧 Files skipped from review as they are similar to previous changes (16)
- plugin/kvm/src/main/java/org/zstack/kvm/APIUpdateKVMHostMsgDoc_zh_cn.groovy
- compute/src/main/java/org/zstack/compute/zone/ZoneSystemTags.java
- header/src/main/java/org/zstack/header/zone/ManagementNetworkIpVersionResourceExtensionPoint.java
- storage/src/main/java/org/zstack/storage/primary/PrimaryStorageApiInterceptor.java
- plugin/sftpBackupStorage/src/main/java/org/zstack/storage/backup/sftp/SftpBackupStorageApiInterceptor.java
- storage/src/main/java/org/zstack/storage/backup/BackupStorageApiInterceptor.java
- plugin/sftpBackupStorage/src/main/java/org/zstack/storage/backup/sftp/SftpBackupStorage.java
- compute/src/main/java/org/zstack/compute/host/HostApiInterceptor.java
- compute/src/main/java/org/zstack/compute/zone/ManagementNetworkIpVersionManagerImpl.java
- header/src/main/java/org/zstack/header/zone/ManagementNetworkIpVersionManager.java
- plugin/ceph/src/main/java/org/zstack/storage/ceph/CephApiInterceptor.java
- test/src/test/groovy/org/zstack/test/unittest/utils/NetworkUtilsCase.groovy
- compute/src/main/java/org/zstack/compute/zone/ZoneManagerImpl.java
- test/src/test/groovy/org/zstack/test/integration/storage/ManagementNetworkIpVersionStorageConstraintCase.groovy
- storage/src/main/java/org/zstack/storage/ManagementNetworkIpVersionStorageExtension.java
- utils/src/main/java/org/zstack/utils/network/ManagementNetworkIpVersionUtils.java
MatheMatrix
force-pushed
the
sync/shixin.ruan/shixin-ZSTAC-85773@@2
branch
11 times, most recently
from
93e1747 to
d73cf70
Compare
Validate zone management-network ipVersion system tag for deployment endpoints including host managementIp, storage URLs, SFTP backup storage hostname, and Ceph mon hostname. Allow updating the zone ipVersion tag, format IPv6 SFTP backup storage URLs, cover endpoint compatibility tests, and keep IPv6 host/storage cases aligned with the explicit zone ipVersion contract. Resolves: ZSTAC-85773 Change-Id: I2823e020f9b99afc66bc11c23474923e11eb471c
MatheMatrix
force-pushed
the
sync/shixin.ruan/shixin-ZSTAC-85773@@2
branch
from
d73cf70 to
515ea71
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
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.
Root cause:
Fix:
Verification:
Jira: http://jira.zstack.io/browse/ZSTAC-85773
sync from gitlab !10141