<fix>[network]: set nic ip out of l3 cidr scope#3301
Open
zstack-robot-2 wants to merge 3 commits into5.5.6from
Open
<fix>[network]: set nic ip out of l3 cidr scope#3301zstack-robot-2 wants to merge 3 commits into5.5.6from
zstack-robot-2 wants to merge 3 commits into5.5.6from
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:
Walkthrough该变更引入 VM 网卡静态 DNS 系统标签与持久化接口，扩展静态 IP 流程以携带/回滚 dns 信息；新增允许（可配置）在 L3 网络 IP 范围外创建静态 IP 的支持并伴随数据库、DHCP、验证拦截器、API/消息与 VO/元模型的变更。 Changes
Sequence Diagram(s)sequenceDiagram
participant API as API 请求
participant VmBase as VmInstanceBase
participant StaticIp as StaticIpOperator
participant DB as 数据库
participant NetSvc as NetworkService
API->>VmBase: APISetVmStaticIpMsg(ip, dnsAddresses)
VmBase->>StaticIp: SetVmStaticIpMsg(..., dnsAddresses)
VmBase->>StaticIp: setStaticDns(vmUuid, l3Uuid, dnsAddresses)
StaticIp->>StaticIp: 验证并处理 DNS（含 IPv6 表示转换）
StaticIp->>DB: 写入/更新 STATIC_DNS 系统标签
StaticIp-->>VmBase: 返回成功
VmBase->>DB: 保存/提交 IP 变更
VmBase-->>API: 返回完成
Note over NetSvc,DB: 查询顺序：NIC 系统标签 STATIC_DNS -> L3 网络 DNS
NetSvc->>DB: 查询 VM NIC STATIC_DNS 标签
alt NIC 有 STATIC_DNS
DB-->>NetSvc: 返回 NIC 级 DNS 列表
else
NetSvc->>DB: 查询 L3 网络 DNS
DB-->>NetSvc: 返回 L3 DNS 列表
end
sequenceDiagram
participant API as API 请求
participant Interceptor as 验证拦截器 (EIP/LB/PF)
participant DB as 数据库
participant Service as 后续服务
API->>Interceptor: 创建/绑定 请求(含 usedIpUuid)
Interceptor->>DB: 查询 UsedIpVO(by usedIpUuid)
Interceptor->>Interceptor: 检查 usedIpVO.ipRangeUuid 是否为 null
alt ipRangeUuid == null
Interceptor-->>API: 抛出 ApiMessageInterceptionException（拒绝绑定）
else
Interceptor->>Service: 继续后续处理
Service-->>API: 返回成功
end
Estimated code review effort🎯 4 (复杂) | ⏱️ ~60 分钟 Poem
🚥 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 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.41.0)compute/src/main/java/org/zstack/compute/vm/VmInstanceBase.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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
compute/src/main/java/org/zstack/compute/vm/StaticIpOperator.java (1)
298-311:⚠️ Potential issue | 🔴 Critical方法返回类型声明与实现不匹配，导致编译错误
方法签名声明返回
Map<String, NicIpAddressInfo>，但实现创建并返回的是Map<Integer, String>。经检查，所有调用此方法的地方（共5处）都期望接收Map<Integer, String>：
TfPortService.java:54FlatDhcpBackend.java:1753VmAllocateNicIpFlow.java:113VmInstanceBase.java:6640VmAllocateNicForStartingVmFlow.java:88方法体使用
Integer类型的键（IPv6Constants.IPv4、IPv6Constants.IPv6）存储 IP 字符串，与NicIpAddressInfo无关。此错误会导致代码无法编译。修复方案： 将方法签名改为
public Map<Integer, String> getNicStaticIpMap(List<String> nicStaticIpList)，与实现和所有调用方保持一致。
🤖 Fix all issues with AI agents
In `@compute/src/main/java/org/zstack/compute/vm/StaticIpOperator.java`:
- Around line 284-296: In getStaticDnsByVmUuidAndL3Uuid, make the l3Uuid
comparison null-safe and return an empty list instead of null: guard against
tokens.get(L3NetworkSystemTags.STATIC_DNS_L3_UUID_TOKEN) being null by using
l3Uuid.equals(uuid) or an explicit null check before comparing, and change the
method to return Collections.emptyList() (or new ArrayList<>()) when no DNS
entries are found to match the behavior of getStaticIpbyVmUuid; update
references to L3NetworkSystemTags.STATIC_DNS_TOKEN handling accordingly.
In
`@network/src/main/java/org/zstack/network/service/NetworkServiceManagerImpl.java`:
- Around line 491-499: The loop in NetworkServiceManagerImpl that compares
uuid.equals(l3NetworkUuid) risks NPE if
tokens.get(L3NetworkSystemTags.STATIC_DNS_L3_UUID_TOKEN) returns null; change
the check to use l3NetworkUuid.equals(uuid) or explicitly null-check uuid before
comparing, and ensure you still retrieve and validate the STATIC_DNS_TOKEN
string (dnsStr) before splitting to avoid downstream errors (refer to the
variables uuid, l3NetworkUuid and keys
L3NetworkSystemTags.STATIC_DNS_L3_UUID_TOKEN /
L3NetworkSystemTags.STATIC_DNS_TOKEN).
In
`@plugin/eip/src/main/java/org/zstack/network/service/eip/EipApiInterceptor.java`:
- Around line 205-212: In EipApiInterceptor update the error message string used
when throwing ApiMessageInterceptionException for a UsedIpVO with null
ipRangeUuid: locate the block that calls dbf.findByUuid(msg.getUsedIpUuid(),
UsedIpVO.class) and currently throws
ApiMessageInterceptionException(argerr("cannot bindBind EIP to IP address[%s]
...", usedIpVO.getIp())); and correct the typo to "cannot bind EIP to IP
address[%s] which is outside L3 network CIDR range" (retain the argerr usage and
the inserted usedIpVO.getIp() parameter).
In
`@plugin/loadBalancer/src/main/java/org/zstack/network/service/lb/LoadBalancerApiInterceptor.java`:
- Around line 645-657: Replace the per-NIC DB lookups and single-usedIp check
with batch queries and full-usedIp iteration: fetch all VmNicVOs for
msg.getVmNicUuids() in one dbf query, collect all UsedIpVO UUIDs from each
VmNicVO.getUsedIps() (or from getUsedIpUuids if available), batch-load those
UsedIpVOs in one dbf.findByUuids call, then for each VmNicVO iterate its
associated UsedIpVOs and throw ApiMessageInterceptionException using the
ORG_ZSTACK_NETWORK_SERVICE_LB_XXXXX error constant when any UsedIpVO has
ipRangeUuid == null; update the code in LoadBalancerApiInterceptor where the
loop over msg.getVmNicUuids() currently lives to use these batched collections
and iteration to avoid looped queries and to handle dual-stack IPs.
In
`@plugin/portForwarding/src/main/java/org/zstack/network/service/portforwarding/PortForwardingApiInterceptor.java`:
- Around line 152-160: The exception message in PortForwardingApiInterceptor
when checking nicVO/usedIpVO mistakenly contains "bindBind" and should be
reworded for clarity; update the ApiMessageInterceptionException thrown via
argerr to a clear single-voice message (e.g., "cannot bind port forwarding rule
to IP address [%s] because it is outside the L3 network CIDR range") referencing
usedIpVO.getIp(), keeping the same exception type and placement so nicVO,
usedIpVO, ApiMessageInterceptionException and argerr remain unchanged except for
the corrected message text.
In
`@plugin/securityGroup/src/main/java/org/zstack/network/securitygroup/SecurityGroupManagerImpl.java`:
- Around line 169-178: The query in getVmIpsBySecurityGroup unconditionally
excludes rows with ip.ipRangeUuid == null which drops NICs on L3s with
enableIPAM=false or IPs allowed out-of-range; change the WHERE so the
ipRangeUuid check only applies when the L3 has IPAM enabled and disallows
out-of-range addresses. Concretely, join UsedIpVO (alias ip) to L3NetworkVO and
replace the final clause "and ip.ipRangeUuid is not null" with a predicate like
"(l3.enableIpam = false OR (l3.enableIpam = true AND ip.ipRangeUuid is not
null))" (adjust field names to the actual enableIPAM/allowOver fields) so
getVmIpsBySecurityGroup returns IPs for L3s without IPAM and still filters only
when IPAM is enabled and range-enforced.
🧹 Nitpick comments (3)
conf/db/upgrade/V5.5.7__schema.sql (2)
5-8: 表名和列名建议使用反引号包裹根据编码规范，为避免 MySQL 8.0 / GreatSQL 保留关键字冲突，所有表名和列名应使用反引号包裹。
♻️ 建议修复
-UPDATE UsedIpVO u -INNER JOIN IpRangeVO r ON u.ipRangeUuid = r.uuid -SET u.prefixLen = r.prefixLen -WHERE u.ipVersion = 6 AND u.ipRangeUuid IS NOT NULL AND u.prefixLen IS NULL; +UPDATE `UsedIpVO` u +INNER JOIN `IpRangeVO` r ON u.`ipRangeUuid` = r.`uuid` +SET u.`prefixLen` = r.`prefixLen` +WHERE u.`ipVersion` = 6 AND u.`ipRangeUuid` IS NOT NULL AND u.`prefixLen` IS NULL;As per coding guidelines: "所有表名和列名必须使用反引号包裹（例如：WHERE `system` = 1），以避免 MySQL 8.0 / GreatSQL 保留关键字冲突导致的语法错误"
14-35: 存储过程逻辑正确，但建议增强幂等性存储过程正确检查了约束是否存在后再进行删除和重建。但如果约束已经是
SET NULL类型，重复执行会导致不必要的 DDL 操作。考虑在检查约束存在的同时，也检查当前的删除规则是否已经是
SET NULL，以实现完全幂等。整体迁移方案合理：将
ipRangeUuid的外键从CASCADE改为SET NULL，支持 UsedIpVO 在 IpRange 删除后仍能保留（用于 CIDR 范围外的 IP）。network/src/main/java/org/zstack/network/l3/CHANGES-VmNicIpOutsideCidr.md (1)
1-259: 文档应使用英文编写根据编码规范，所有文件内容（包括文档和注释）都应当使用英文编写。此 CHANGES 文件完全使用中文，违反了
**/*.*规则。建议将整个文档翻译为英文，包括：
- 标题和章节名
- 功能描述
- 代码注释
- 表格内容
As per coding guidelines: "代码里不应当有有中文，包括报错、注释等都应当使用正确的、无拼写错误的英文来写"
Comment on lines
284
to
296
| public List<String> getStaticDnsByVmUuidAndL3Uuid(String vmUuid, String l3Uuid) { | ||
| List<Map<String, String>> tokenList = L3NetworkSystemTags.STATIC_DNS.getTokensOfTagsByResourceUuid(vmUuid); | ||
| for (Map<String, String> tokens : tokenList) { | ||
| String uuid = tokens.get(L3NetworkSystemTags.STATIC_DNS_L3_UUID_TOKEN); | ||
| if (uuid.equals(l3Uuid)) { | ||
| String dnsStr = tokens.get(L3NetworkSystemTags.STATIC_DNS_TOKEN); | ||
| if (dnsStr != null && !dnsStr.isEmpty()) { | ||
| return Arrays.asList(dnsStr.split(",")); | ||
| } | ||
| } | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
潜在空指针异常
Line 288: uuid.equals(l3Uuid) 中 uuid 可能为 null（如果 token 中没有该 key），会导致 NPE。建议使用 l3Uuid.equals(uuid) 或进行 null 检查。
另外，Line 295 返回 null 而非空列表，与其他类似方法（如 getStaticIpbyVmUuid）返回空集合的模式不一致。
🛡️ 修复建议
public List<String> getStaticDnsByVmUuidAndL3Uuid(String vmUuid, String l3Uuid) {
List<Map<String, String>> tokenList = L3NetworkSystemTags.STATIC_DNS.getTokensOfTagsByResourceUuid(vmUuid);
for (Map<String, String> tokens : tokenList) {
String uuid = tokens.get(L3NetworkSystemTags.STATIC_DNS_L3_UUID_TOKEN);
- if (uuid.equals(l3Uuid)) {
+ if (l3Uuid.equals(uuid)) {
String dnsStr = tokens.get(L3NetworkSystemTags.STATIC_DNS_TOKEN);
if (dnsStr != null && !dnsStr.isEmpty()) {
return Arrays.asList(dnsStr.split(","));
}
}
}
- return null;
+ return new ArrayList<>();
}🤖 Prompt for AI Agents
In `@compute/src/main/java/org/zstack/compute/vm/StaticIpOperator.java` around
lines 284 - 296, In getStaticDnsByVmUuidAndL3Uuid, make the l3Uuid comparison
null-safe and return an empty list instead of null: guard against
tokens.get(L3NetworkSystemTags.STATIC_DNS_L3_UUID_TOKEN) being null by using
l3Uuid.equals(uuid) or an explicit null check before comparing, and change the
method to return Collections.emptyList() (or new ArrayList<>()) when no DNS
entries are found to match the behavior of getStaticIpbyVmUuid; update
references to L3NetworkSystemTags.STATIC_DNS_TOKEN handling accordingly.
Comment on lines
491
to
499
| for (Map<String, String> tokens : tokenList) { | ||
| String uuid = tokens.get(L3NetworkSystemTags.STATIC_DNS_L3_UUID_TOKEN); | ||
| if (uuid.equals(l3NetworkUuid)) { | ||
| String dnsStr = tokens.get(L3NetworkSystemTags.STATIC_DNS_TOKEN); | ||
| if (dnsStr != null && !dnsStr.isEmpty()) { | ||
| return Arrays.asList(dnsStr.split(",")); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
潜在空指针异常风险
当 tokens.get(L3NetworkSystemTags.STATIC_DNS_L3_UUID_TOKEN) 返回 null 时，第 493 行的 uuid.equals(l3NetworkUuid) 会抛出 NullPointerException。
建议使用 l3NetworkUuid.equals(uuid) 或添加空值检查：
🛠️ 建议修复
for (Map<String, String> tokens : tokenList) {
String uuid = tokens.get(L3NetworkSystemTags.STATIC_DNS_L3_UUID_TOKEN);
- if (uuid.equals(l3NetworkUuid)) {
+ if (l3NetworkUuid.equals(uuid)) {
String dnsStr = tokens.get(L3NetworkSystemTags.STATIC_DNS_TOKEN);
if (dnsStr != null && !dnsStr.isEmpty()) {
return Arrays.asList(dnsStr.split(","));
}
}
}🤖 Prompt for AI Agents
In
`@network/src/main/java/org/zstack/network/service/NetworkServiceManagerImpl.java`
around lines 491 - 499, The loop in NetworkServiceManagerImpl that compares
uuid.equals(l3NetworkUuid) risks NPE if
tokens.get(L3NetworkSystemTags.STATIC_DNS_L3_UUID_TOKEN) returns null; change
the check to use l3NetworkUuid.equals(uuid) or explicitly null-check uuid before
comparing, and ensure you still retrieve and validate the STATIC_DNS_TOKEN
string (dnsStr) before splitting to avoid downstream errors (refer to the
variables uuid, l3NetworkUuid and keys
L3NetworkSystemTags.STATIC_DNS_L3_UUID_TOKEN /
L3NetworkSystemTags.STATIC_DNS_TOKEN).
plugin/eip/src/main/java/org/zstack/network/service/eip/EipApiInterceptor.java
Show resolved
Hide resolved
Comment on lines
+645
to
+657
|
|
||
| // Check if any NIC's IP is outside L3 CIDR range (ipRangeUuid is null) | ||
| for (String nicUuid : msg.getVmNicUuids()) { | ||
| VmNicVO nicVO = dbf.findByUuid(nicUuid, VmNicVO.class); | ||
| if (nicVO != null && nicVO.getUsedIpUuid() != null) { | ||
| UsedIpVO usedIpVO = dbf.findByUuid(nicVO.getUsedIpUuid(), UsedIpVO.class); | ||
| if (usedIpVO != null && usedIpVO.getIpRangeUuid() == null) { | ||
| throw new ApiMessageInterceptionException(argerr( | ||
| "cannot add VM NIC[uuid:%s] with IP address[%s] which is outside L3 network CIDR range to load balancer", | ||
| nicUuid, usedIpVO.getIp())); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
循环内数据库查询存在性能问题，且可能遗漏双栈场景
存在以下问题：
-
性能问题：循环内执行了
dbf.findByUuid查询，违反了 "禁止循环里套查询" 的编码规范。应改为批量查询。 -
双栈场景：
nicVO.getUsedIpUuid()只返回单个 IP，但根据项目约束，同一 VmNic 可能同时存在两条 UsedIpVO（IPv4/IPv6 双栈）。应遍历nicVO.getUsedIps()检查所有关联 IP。 -
错误码缺失：错误信息应使用
ORG_ZSTACK_NETWORK_SERVICE_LB_XXXXX常量，与文件中其他错误信息保持一致。
建议的修复方案
- // Check if any NIC's IP is outside L3 CIDR range (ipRangeUuid is null)
- for (String nicUuid : msg.getVmNicUuids()) {
- VmNicVO nicVO = dbf.findByUuid(nicUuid, VmNicVO.class);
- if (nicVO != null && nicVO.getUsedIpUuid() != null) {
- UsedIpVO usedIpVO = dbf.findByUuid(nicVO.getUsedIpUuid(), UsedIpVO.class);
- if (usedIpVO != null && usedIpVO.getIpRangeUuid() == null) {
- throw new ApiMessageInterceptionException(argerr(
- "cannot add VM NIC[uuid:%s] with IP address[%s] which is outside L3 network CIDR range to load balancer",
- nicUuid, usedIpVO.getIp()));
- }
- }
- }
+ // Check if any NIC's IP is outside L3 CIDR range (ipRangeUuid is null)
+ List<UsedIpVO> usedIps = Q.New(UsedIpVO.class)
+ .in(UsedIpVO_.vmNicUuid, msg.getVmNicUuids())
+ .isNull(UsedIpVO_.ipRangeUuid)
+ .list();
+ if (!usedIps.isEmpty()) {
+ UsedIpVO usedIp = usedIps.get(0);
+ throw new ApiMessageInterceptionException(argerr(
+ ORG_ZSTACK_NETWORK_SERVICE_LB_XXXXX,
+ "cannot add VM NIC[uuid:%s] with IP address[%s] which is outside L3 network CIDR range to load balancer",
+ usedIp.getVmNicUuid(), usedIp.getIp()));
+ }Based on learnings: 同一 VmNic 可能同时存在两条 UsedIpVO，但它们只代表同一 L3 的 IPv4/IPv6 双栈。As per coding guidelines: 禁止循环里套查询，避免嵌套查询带来的性能问题。
🤖 Prompt for AI Agents
In
`@plugin/loadBalancer/src/main/java/org/zstack/network/service/lb/LoadBalancerApiInterceptor.java`
around lines 645 - 657, Replace the per-NIC DB lookups and single-usedIp check
with batch queries and full-usedIp iteration: fetch all VmNicVOs for
msg.getVmNicUuids() in one dbf query, collect all UsedIpVO UUIDs from each
VmNicVO.getUsedIps() (or from getUsedIpUuids if available), batch-load those
UsedIpVOs in one dbf.findByUuids call, then for each VmNicVO iterate its
associated UsedIpVOs and throw ApiMessageInterceptionException using the
ORG_ZSTACK_NETWORK_SERVICE_LB_XXXXX error constant when any UsedIpVO has
ipRangeUuid == null; update the code in LoadBalancerApiInterceptor where the
loop over msg.getVmNicUuids() currently lives to use these batched collections
and iteration to avoid looped queries and to handle dual-stack IPs.
...ng/src/main/java/org/zstack/network/service/portforwarding/PortForwardingApiInterceptor.java
Show resolved
Hide resolved
Comment on lines
169
to
178
| private List<String> getVmIpsBySecurityGroup(String sgUuid, int ipVersion){ | ||
| List<String> ret = new ArrayList<>(); | ||
| // Exclude IPs outside L3 CIDR range (ipRangeUuid is null) | ||
| String sql = "select ip.ip" + | ||
| " from VmNicVO nic, VmNicSecurityGroupRefVO ref, SecurityGroupVO sg, UsedIpVO ip" + | ||
| " where sg.uuid = ref.securityGroupUuid and ref.vmNicUuid = nic.uuid" + | ||
| " and ref.securityGroupUuid = :sgUuid" + | ||
| " and nic.uuid = ip.vmNicUuid and ip.ipVersion = :ipVersion"; | ||
| " and nic.uuid = ip.vmNicUuid and ip.ipVersion = :ipVersion" + | ||
| " and ip.ipRangeUuid is not null"; | ||
| TypedQuery<String> internalIpQuery = dbf.getEntityManager().createQuery(sql, String.class); |
There was a problem hiding this comment.
过滤 ipRangeUuid 可能导致 IPAM 关闭网络或允许越界 IP 的安全组成员缺失。
enableIPAM=false 的 L3 网络通常不会有 ipRangeUuid，这条过滤会把这些 NIC 的 IP 全部排除，远端安全组引用它们时会缺成员。若系统允许“IP 超出范围”，同样会被过滤掉。建议将过滤限定在 IPAM 开启且不允许越界 的场景，或显式允许 enableIPAM=false 的 L3。
💡 可选修正方向（请按实际字段名调整）
- String sql = "select ip.ip" +
- " from VmNicVO nic, VmNicSecurityGroupRefVO ref, SecurityGroupVO sg, UsedIpVO ip" +
+ String sql = "select ip.ip" +
+ " from VmNicVO nic, VmNicSecurityGroupRefVO ref, SecurityGroupVO sg, UsedIpVO ip, L3NetworkVO l3" +
" where sg.uuid = ref.securityGroupUuid and ref.vmNicUuid = nic.uuid" +
" and ref.securityGroupUuid = :sgUuid" +
- " and nic.uuid = ip.vmNicUuid and ip.ipVersion = :ipVersion" +
- " and ip.ipRangeUuid is not null";
+ " and nic.uuid = ip.vmNicUuid and ip.ipVersion = :ipVersion" +
+ " and nic.l3NetworkUuid = l3.uuid" +
+ " and (ip.ipRangeUuid is not null or l3.enableIPAM = false)";Based on learnings 在 L3NetworkApiInterceptor 中，enableIPAM=false 的 L3 网络不会有关联的 ipRangeUuid。
🤖 Prompt for AI Agents
In
`@plugin/securityGroup/src/main/java/org/zstack/network/securitygroup/SecurityGroupManagerImpl.java`
around lines 169 - 178, The query in getVmIpsBySecurityGroup unconditionally
excludes rows with ip.ipRangeUuid == null which drops NICs on L3s with
enableIPAM=false or IPs allowed out-of-range; change the WHERE so the
ipRangeUuid check only applies when the L3 has IPAM enabled and disallows
out-of-range addresses. Concretely, join UsedIpVO (alias ip) to L3NetworkVO and
replace the final clause "and ip.ipRangeUuid is not null" with a predicate like
"(l3.enableIpam = false OR (l3.enableIpam = true AND ip.ipRangeUuid is not
null))" (adjust field names to the actual enableIPAM/allowOver fields) so
getVmIpsBySecurityGroup returns IPs for L3s without IPAM and still filters only
when IPAM is enabled and range-enforced.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
compute/src/main/java/org/zstack/compute/vm/StaticIpOperator.java (1)
402-499:⚠️ Potential issue | 🟠 Major多 IP 段场景下的范围判断需覆盖全部段
当前仅用第一条 IPRange 计算 ipInRange；在允许范围外时，多段网络里“在其他段内”的 IP 会被误判为超范围并强制要求网关/掩码。建议遍历全部段确认是否在任一范围内。
🔧 建议修改
- NormalIpRangeVO ipRangeVO = Q.New(NormalIpRangeVO.class) - .eq(NormalIpRangeVO_.l3NetworkUuid, l3Uuid) - .eq(NormalIpRangeVO_.ipVersion, IPv6Constants.IPv4) - .limit(1).find(); - - // Check if IP is within the range - boolean ipInRange = ipRangeVO != null && - NetworkUtils.isInRange(nicIp.ipv4Address, ipRangeVO.getStartIp(), ipRangeVO.getEndIp()); + List<NormalIpRangeVO> ipRangeVOs = Q.New(NormalIpRangeVO.class) + .eq(NormalIpRangeVO_.l3NetworkUuid, l3Uuid) + .eq(NormalIpRangeVO_.ipVersion, IPv6Constants.IPv4) + .list(); + NormalIpRangeVO ipRangeVO = ipRangeVOs.isEmpty() ? null : ipRangeVOs.get(0); + + // Check if IP is within any range + boolean ipInRange = false; + for (NormalIpRangeVO range : ipRangeVOs) { + if (NetworkUtils.isInRange(nicIp.ipv4Address, range.getStartIp(), range.getEndIp())) { + ipInRange = true; + break; + } + }- NormalIpRangeVO ipRangeVO = Q.New(NormalIpRangeVO.class) - .eq(NormalIpRangeVO_.l3NetworkUuid, l3Uuid) - .eq(NormalIpRangeVO_.ipVersion, IPv6Constants.IPv6) - .limit(1).find(); - - // Check if IPv6 is within the range - boolean ipInRange = ipRangeVO != null && - IPv6NetworkUtils.isIpv6InRange(nicIp.ipv6Address, ipRangeVO.getStartIp(), ipRangeVO.getEndIp()); + List<NormalIpRangeVO> ipRangeVOs = Q.New(NormalIpRangeVO.class) + .eq(NormalIpRangeVO_.l3NetworkUuid, l3Uuid) + .eq(NormalIpRangeVO_.ipVersion, IPv6Constants.IPv6) + .list(); + NormalIpRangeVO ipRangeVO = ipRangeVOs.isEmpty() ? null : ipRangeVOs.get(0); + + // Check if IPv6 is within any range + boolean ipInRange = false; + for (NormalIpRangeVO range : ipRangeVOs) { + if (IPv6NetworkUtils.isIpv6InRange(nicIp.ipv6Address, range.getStartIp(), range.getEndIp())) { + ipInRange = true; + break; + } + }
🤖 Fix all issues with AI agents
In `@compute/src/main/java/org/zstack/compute/vm/StaticIpOperator.java`:
- Around line 139-148: In StaticIpOperator (the block handling
L3NetworkSystemTags.STATIC_DNS), trim and sanitize the parsed dns string before
splitting: after obtaining dnsStr from the tag, run a trim, remove any internal
newlines/extra whitespace, split on commas, then trim each token and filter out
empty strings before assigning to dnsAddresses; additionally, support IPv6-safe
storage by decoding the tag value when reading (and ensure callers encode IPv6
addresses when writing) using the project’s system-tag encode/decode utilities
so addresses containing "::" are preserved. Apply the same trimming/decoding
logic for the other similar parsing sites referenced around lines 244-275 and
289-292.
Comment on lines
139
to
148
| if(L3NetworkSystemTags.STATIC_DNS.isMatch(sysTag)) { | ||
| Map<String, String> token = TagUtils.parse(L3NetworkSystemTags.STATIC_DNS.getTagFormat(), sysTag); | ||
| String l3Uuid = token.get(L3NetworkSystemTags.STATIC_DNS_L3_UUID_TOKEN); | ||
| if (ret.get(l3Uuid) == null) { | ||
| continue; | ||
| } | ||
| String dnsStr = token.get(L3NetworkSystemTags.STATIC_DNS_TOKEN); | ||
| if (dnsStr != null && !dnsStr.isEmpty()) { | ||
| ret.get(l3Uuid).dnsAddresses = Arrays.asList(dnsStr.split(",")); | ||
| } |
There was a problem hiding this comment.
DNS 入参需 trim 且 IPv6 需做 tag 编码/解码
dnsAddresses 可能含空格/换行，直接校验会误判；另外 IPv6 若直接写入 system tag，含 “::” 的地址在解析时可能被截断，建议写入前编码、读取时解码。
🔧 建议修改
- // Validate DNS addresses
- for (String dns : dnsAddresses) {
- if (!NetworkUtils.isIpv4Address(dns) && !IPv6NetworkUtils.isIpv6Address(dns)) {
- throw new ApiMessageInterceptionException(argerr(
- "invalid DNS address[%s], must be a valid IPv4 or IPv6 address", dns));
- }
- }
-
- String dnsStr = String.join(",", dnsAddresses);
+ // Validate DNS addresses
+ List<String> normalizedDns = new ArrayList<>();
+ for (String dns : dnsAddresses) {
+ String trimmed = dns == null ? null : dns.trim();
+ if (StringUtils.isEmpty(trimmed)) {
+ continue;
+ }
+ if (!NetworkUtils.isIpv4Address(trimmed) && !IPv6NetworkUtils.isIpv6Address(trimmed)) {
+ throw new ApiMessageInterceptionException(argerr(
+ "invalid DNS address[%s], must be a valid IPv4 or IPv6 address", trimmed));
+ }
+ if (IPv6NetworkUtils.isIpv6Address(trimmed)) {
+ trimmed = IPv6NetworkUtils.ipv6AddessToTagValue(trimmed);
+ }
+ normalizedDns.add(trimmed);
+ }
+
+ String dnsStr = String.join(",", normalizedDns);- if (dnsStr != null && !dnsStr.isEmpty()) {
- ret.get(l3Uuid).dnsAddresses = Arrays.asList(dnsStr.split(","));
- }
+ if (dnsStr != null && !dnsStr.isEmpty()) {
+ List<String> dnsList = new ArrayList<>();
+ for (String dns : dnsStr.split(",")) {
+ dnsList.add(IPv6NetworkUtils.ipv6TagValueToAddress(dns));
+ }
+ ret.get(l3Uuid).dnsAddresses = dnsList;
+ }- return Arrays.asList(dnsStr.split(","));
+ List<String> dnsList = new ArrayList<>();
+ for (String dns : dnsStr.split(",")) {
+ dnsList.add(IPv6NetworkUtils.ipv6TagValueToAddress(dns));
+ }
+ return dnsList;As per coding guidelines: 注意检查来自 Message 的参数是否做过 trim，用户可能在浏览器上复制粘贴的数据带有空格、换行符等。
Also applies to: 244-275, 289-292
🤖 Prompt for AI Agents
In `@compute/src/main/java/org/zstack/compute/vm/StaticIpOperator.java` around
lines 139 - 148, In StaticIpOperator (the block handling
L3NetworkSystemTags.STATIC_DNS), trim and sanitize the parsed dns string before
splitting: after obtaining dnsStr from the tag, run a trim, remove any internal
newlines/extra whitespace, split on commas, then trim each token and filter out
empty strings before assigning to dnsAddresses; additionally, support IPv6-safe
storage by decoding the tag value when reading (and ensure callers encode IPv6
addresses when writing) using the project’s system-tag encode/decode utilities
so addresses containing "::" are preserved. Apply the same trimming/decoding
logic for the other similar parsing sites referenced around lines 244-275 and
289-292.
APIImpact DBImpact GlobalConfigImpact Resolves: ZSTAC-81969 Change-Id: I736e77646266696e646271766d7170627378796b
Resolves: ZSTAC-81969 Change-Id: I836f78747377706f6e647372776e8271737479786c
MatheMatrix
force-pushed
the
sync/shixin.ruan/shixin-ZSTAC-81969@@2
branch
from
465a69e to
962407e
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (4)
plugin/securityGroup/src/main/java/org/zstack/network/securitygroup/SecurityGroupManagerImpl.java (1)
171-177:⚠️ Potential issue | 🟠 Major
ipRangeUuid过滤条件过严，会误删enableIPAM=false网络成员。这里无条件使用
ip.ipRangeUuid is not null，会把enableIPAM=false的 L3 网络 NIC IP 全部排除（这类网络本身就没有ipRangeUuid），导致安全组成员不完整。💡 建议修正
- String sql = "select ip.ip" + - " from VmNicVO nic, VmNicSecurityGroupRefVO ref, SecurityGroupVO sg, UsedIpVO ip" + + String sql = "select ip.ip" + + " from VmNicVO nic, VmNicSecurityGroupRefVO ref, SecurityGroupVO sg, UsedIpVO ip, L3NetworkVO l3" + " where sg.uuid = ref.securityGroupUuid and ref.vmNicUuid = nic.uuid" + " and ref.securityGroupUuid = :sgUuid" + - " and nic.uuid = ip.vmNicUuid and ip.ipVersion = :ipVersion" + - " and ip.ipRangeUuid is not null"; + " and nic.uuid = ip.vmNicUuid and ip.ipVersion = :ipVersion" + + " and nic.l3NetworkUuid = l3.uuid" + + " and (l3.enableIPAM = false or ip.ipRangeUuid is not null)";Based on learnings 在 ZStack 的 L3NetworkApiInterceptor 中，enableIPAM=false 的 L3 网络不会有关联的 ipRangeUuid。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/securityGroup/src/main/java/org/zstack/network/securitygroup/SecurityGroupManagerImpl.java` around lines 171 - 177, The query in SecurityGroupManagerImpl building the sql string incorrectly filters out IPs with ip.ipRangeUuid == null, which excludes NICs on enableIPAM=false L3 networks; remove the "and ip.ipRangeUuid is not null" clause (or replace it with logic that includes IPs regardless of ipRangeUuid) in the sql construction so all UsedIpVO.ip rows for the VM NICs are considered when assembling security group members.compute/src/main/java/org/zstack/compute/vm/StaticIpOperator.java (2)
244-257:⚠️ Potential issue | 🟠 Major先标准化 DNS 输入，再做合法性校验。
Line [246] 直接校验原始
dns，null元素可能触发运行时异常，带空白字符的合法地址也会被误判。建议先 trim/判空，再进行地址校验与 tag 编码。🛠️ 建议修复
- // Validate DNS addresses - for (String dns : dnsAddresses) { - if (!NetworkUtils.isIpv4Address(dns) && !IPv6NetworkUtils.isIpv6Address(dns)) { + List<String> normalizedDns = new ArrayList<>(); + for (String dns : dnsAddresses) { + String trimmed = dns == null ? null : dns.trim(); + if (StringUtils.isEmpty(trimmed)) { + throw new ApiMessageInterceptionException(argerr( + "invalid DNS address[%s], must be a valid IPv4 or IPv6 address", dns)); + } + if (!NetworkUtils.isIpv4Address(trimmed) && !IPv6NetworkUtils.isIpv6Address(trimmed)) { throw new ApiMessageInterceptionException(argerr( - "invalid DNS address[%s], must be a valid IPv4 or IPv6 address", dns)); + "invalid DNS address[%s], must be a valid IPv4 or IPv6 address", trimmed)); } + normalizedDns.add(trimmed); } @@ - for (String dns : dnsAddresses) { + for (String dns : normalizedDns) { tagSafeDns.add(IPv6NetworkUtils.ipv6AddessToTagValue(dns)); }As per coding guidelines: 注意检查来自 Message 的参数是否做过 trim，用户可能在浏览器上复制粘贴的数据带有空格、换行符等。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@compute/src/main/java/org/zstack/compute/vm/StaticIpOperator.java` around lines 244 - 257, The DNS validation currently iterates raw dnsAddresses and can NPE or mis-validate values with surrounding whitespace; in StaticIpOperator you should first iterate dnsAddresses, trim each entry and skip or reject null/empty strings (throw ApiMessageInterceptionException via argerr for invalid/missing entries), then validate the trimmed value using NetworkUtils.isIpv4Address and IPv6NetworkUtils.isIpv6Address, and finally pass the trimmed/validated value into IPv6NetworkUtils.ipv6AddessToTagValue when building tagSafeDns and dnsStr (use the trimmed value everywhere instead of the original dns).
292-305:⚠️ Potential issue | 🟡 Minor修复 DNS 查询中的空指针与返回契约不一致。
Line [293] 的
uuid.equals(l3Uuid)有 NPE 风险；Line [305] 返回null会把空值处理压力转移给调用方。建议改为空安全比较并返回空集合。🛠️ 建议修复
for (Map<String, String> tokens : tokenList) { String uuid = tokens.get(VmSystemTags.STATIC_DNS_L3_UUID_TOKEN); - if (uuid.equals(l3Uuid)) { + if (Objects.equals(uuid, l3Uuid)) { String dnsStr = tokens.get(VmSystemTags.STATIC_DNS_TOKEN); if (dnsStr != null && !dnsStr.isEmpty()) { // Convert back from tag value: replace '--' with '::' for IPv6 addresses List<String> dnsList = new ArrayList<>(); for (String dns : dnsStr.split(",")) { dnsList.add(IPv6NetworkUtils.ipv6TagValueToAddress(dns)); } return dnsList; } } } - return null; + return Collections.emptyList();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@compute/src/main/java/org/zstack/compute/vm/StaticIpOperator.java` around lines 292 - 305, In StaticIpOperator, avoid NPE on comparing the L3 UUID and make the return contract non-null: null-check the tokens.get(VmSystemTags.STATIC_DNS_L3_UUID_TOKEN) (or use Objects.equals(l3Uuid, uuid)) instead of uuid.equals(l3Uuid) to prevent NPE, ensure dnsStr is checked for blank before splitting, convert entries with IPv6NetworkUtils.ipv6TagValueToAddress as before, and return an empty List (e.g. Collections.emptyList() or new ArrayList<>()) instead of null so callers always get a non-null list.network/src/main/java/org/zstack/network/service/NetworkServiceManagerImpl.java (1)
493-495:⚠️ Potential issue | 🟡 Minor修复 L3 UUID 比较的空指针风险。
Line [494] 使用
uuid.equals(l3NetworkUuid)，当uuid为null时会直接抛出 NPE，导致无法回退到 L3 DNS。建议改为空安全比较。🛠️ 建议修复
- if (uuid.equals(l3NetworkUuid)) { + if (Objects.equals(uuid, l3NetworkUuid)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@network/src/main/java/org/zstack/network/service/NetworkServiceManagerImpl.java` around lines 493 - 495, The code currently calls uuid.equals(l3NetworkUuid) which can NPE if uuid is null; change the comparison to a null-safe check (e.g. use Objects.equals(l3NetworkUuid, uuid) or compare l3NetworkUuid.equals(uuid) after ensuring l3NetworkUuid is non-null) in NetworkServiceManagerImpl where you read tokens.get(VmSystemTags.STATIC_DNS_L3_UUID_TOKEN) and then compare to l3NetworkUuid; keep the existing logic that then reads VmSystemTags.STATIC_DNS_TOKEN when they match.
🧹 Nitpick comments (2)
plugin/portForwarding/src/main/java/org/zstack/network/service/portforwarding/PortForwardingApiInterceptor.java (1)
152-155: 建议在 Interceptor 层先对vmNicUuid做 trim 再参与查询。Line 153 直接使用
msg.getVmNicUuid()；建议在进入validate(APIAttachPortForwardingRuleMsg)时统一 trim，避免复制粘贴带空白字符导致校验分支行为异常。🧹 建议修复
private void validate(final APIAttachPortForwardingRuleMsg msg) { + msg.setVmNicUuid(StringUtils.trim(msg.getVmNicUuid())); SimpleQuery<PortForwardingRuleVO> q = dbf.createQuery(PortForwardingRuleVO.class);As per coding guidelines: “注意检查来自 Message 的参数是否做过 trim，用户可能在浏览器上复制粘贴的数据带有空格、换行符等”。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/portForwarding/src/main/java/org/zstack/network/service/portforwarding/PortForwardingApiInterceptor.java` around lines 152 - 155, In PortForwardingApiInterceptor (validate(APIAttachPortForwardingRuleMsg)), ensure the vmNicUuid from the message is trimmed before any null/DB checks or queries: obtain msg.getVmNicUuid(), trim whitespace (and handle null safely), then use the trimmed value for dbf.findByUuid and any other logic (or set it back on the msg) so lookups like dbf.findByUuid(msg.getVmNicUuid(), VmNicVO.class) use a cleaned identifier and avoid failures from pasted whitespace.header/src/main/java/org/zstack/header/vm/APIChangeVmNicNetworkMsg.java (1)
36-37: 可选优化：在__example__()里补充dnsAddresses示例值。新入参已经暴露到 API，示例里带上该字段会让自动文档更完整。
Also applies to: 63-68, 87-93
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@header/src/main/java/org/zstack/header/vm/APIChangeVmNicNetworkMsg.java` around lines 36 - 37, The API now exposes the dnsAddresses field but the example payloads aren't showing it; update the __example__() implementations in APIChangeVmNicNetworkMsg (and the other two __example__ methods referenced) to include a sample dnsAddresses value (e.g., a List with one or two IP strings) so autogenerated docs show the new field; locate the __example__ methods in class APIChangeVmNicNetworkMsg and add dnsAddresses to the returned/example object initialization alongside the existing sample values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@compute/src/main/java/org/zstack/compute/vm/VmInstanceBase.java`:
- Around line 6282-6297: setDns() currently overwrites the target L3 DNS with
setStaticDns(...) but rollback() only deletes the DNS via
deleteStaticDnsByVmUuidAndL3Uuid(...), which can wipe an existing user
configuration; modify the logic in VmInstanceBase so setDns() first reads and
saves the original DNS list (e.g., oldDnsAddresses) before calling
StaticIpOperator.setStaticDns(...), set a flag like isDnsSet and store the
original value, and change rollback() to restore the original DNS (call
setStaticDns with oldDnsAddresses or delete only if there was none) instead of
unconditionally deleting; update related variables and the rollback invocation
to use those saved values.
In `@conf/db/upgrade/V5.5.7__schema.sql`:
- Around line 5-8: The SQL uses unquoted identifiers which violates the repo's
SQL style and can break on MySQL/GreateSQL; update the statements involving
UsedIpVO, IpRangeVO and the INFORMATION_SCHEMA query to wrap all table names,
column names and system schema identifiers in backticks (e.g., `UsedIpVO`,
`IpRangeVO`, `ipRangeUuid`, `prefixLen`, `ipVersion`, `INFORMATION_SCHEMA`,
`TABLE_CONSTRAINTS`, `TABLE_SCHEMA`, `TABLE_NAME`, `CONSTRAINT_NAME`) so both
the UPDATE ... INNER JOIN (the block using UsedIpVO u and IpRangeVO r) and the
SELECT ... INTO constraint_exists query (checking fkUsedIpVOIpRangeEO) use
backticked identifiers throughout.
- Around line 14-40: Add a pre-existing-procedure check before creating the
stored procedure ModifyUsedIpVOForeignKey: insert a "DROP PROCEDURE IF EXISTS
ModifyUsedIpVOForeignKey;" (and matching DELIMITER handling if needed)
immediately before the CREATE PROCEDURE ModifyUsedIpVOForeignKey() block so the
upgrade script is reentrant and won't fail if the procedure already exists; keep
the existing CALL ModifyUsedIpVOForeignKey() and DROP PROCEDURE IF EXISTS
ModifyUsedIpVOForeignKey; after execution as-is.
In `@network/src/main/java/org/zstack/network/l3/CHANGES-VmNicIpOutsideCidr.md`:
- Around line 1-259: The CHANGES-VmNicIpOutsideCidr.md document contains
extensive Chinese text; translate the entire file into clear, idiomatic English
(titles, descriptions, code comment examples, error message examples and the
summary table) and ensure any in-code comment samples and example error strings
shown (e.g., STATIC_DNS,
setStaticDns/deleteStaticDnsByVmUuidAndL3Uuid/getStaticDnsByVmUuidAndL3Uuid,
getVmNicDns, VmInstanceBase handlers, GuestToolsManagerImpl ipConfig.setDns,
L3NetworkManagerImpl SQL comments, NormalIpRangeFactory query,
L3NetworkApiInterceptor error messages, DhcpExtension debug log,
SecurityGroupManagerImpl/EipApiInterceptor/PortForwardingApiInterceptor/LoadBalancerApiInterceptor
exception messages, test case descriptions, and ALLOW_IP_OUTSIDE_RANGE
description) are presented in correct English with consistent terminology and no
Chinese; keep technical identifiers and code snippets unchanged except for
converting any Chinese comments or example strings to English.
In `@network/src/main/java/org/zstack/network/service/DhcpExtension.java`:
- Around line 214-231: The code incorrectly uses ip.getPrefixLen() != null to
decide "CIDR-outside" and thus misclassifies in-range IPv6 addresses; change the
branching to check ip.getIpRangeUuid() == null to detect outside-range
addresses, so in the block that currently tests ip.getPrefixLen() you should
instead test ip.getIpRangeUuid() == null and keep the assignments to struct
(setPrefixLength, setFirstIp, setEndIP, setRaMode) for true; the else branch
should remain the fallback that queries NormalIpRangeVO and sets
raMode/prefix/first/end from the range; apply the same replacement in the other
two analogous blocks that use the same prefixLen check.
In
`@plugin/applianceVm/src/main/java/org/zstack/appliancevm/ApplianceVmNicTO.java`:
- Around line 38-50: When ipRangeUuid is null the local variable addressMode can
remain null; update the ApplianceVmNicTO construction logic so addressMode
always has a deterministic fallback: after checking uip.getPrefixLen() and
attempting to load NormalIpRangeVO (via Q.New(NormalIpRangeVO.class) ...
find()), if addressMode is still null set a clear default (e.g.
"Stateless-DHCP") or validate/throw; alternatively add null-handling in
ApplianceVmNicBootstrapExtensionPoint implementations to handle null
addressMode. Locate the addressMode and prefixLength assignments in the block
that reads uip.getPrefixLen(), uip.getIpRangeUuid() and NormalIpRangeVO to apply
the default or validation change.
In
`@plugin/eip/src/main/java/org/zstack/network/service/eip/EipApiInterceptor.java`:
- Around line 206-212: The current outside-CIDR check (checking
UsedIpVO.getIpRangeUuid() == null and throwing ApiMessageInterceptionException)
exists only in validate(APIAttachEipMsg) in EipApiInterceptor, allowing
APICreateEipMsg that supplies usedIpUuid to bypass the restriction; add the same
validation to validate(APICreateEipMsg) or refactor the check into a shared
private helper (e.g., validateUsedIpOutsideCidr(UsedIpVO) or
ensureUsedIpInCidr(msg.getUsedIpUuid())) and call it from both
validate(APIAttachEipMsg) and validate(APICreateEipMsg) so attempts to bind an
EIP to an IP with ipRangeUuid == null are consistently rejected (use the same
error message and throw ApiMessageInterceptionException).
In
`@plugin/portForwarding/src/main/java/org/zstack/network/service/portforwarding/PortForwardingApiInterceptor.java`:
- Around line 152-161: The CIDR boundary check for the VM NIC (the block that
loads VmNicVO/UsedIpVO and throws the ApiMessageInterceptionException when
usedIpVO.getIpRangeUuid() == null) must be executed for all attach paths; move
this validation earlier in the same method so it runs before the early return
that checks peerL3NetworkUuids.contains(guestL3Uuid). Concretely, relocate the
VmNicVO/UsedIpVO null-check and exception logic (variables nicVO and usedIpVO)
to precede the branch that returns when peerL3NetworkUuids includes guestL3Uuid,
ensuring the CIDR check always runs regardless of that early-return condition.
---
Duplicate comments:
In `@compute/src/main/java/org/zstack/compute/vm/StaticIpOperator.java`:
- Around line 244-257: The DNS validation currently iterates raw dnsAddresses
and can NPE or mis-validate values with surrounding whitespace; in
StaticIpOperator you should first iterate dnsAddresses, trim each entry and skip
or reject null/empty strings (throw ApiMessageInterceptionException via argerr
for invalid/missing entries), then validate the trimmed value using
NetworkUtils.isIpv4Address and IPv6NetworkUtils.isIpv6Address, and finally pass
the trimmed/validated value into IPv6NetworkUtils.ipv6AddessToTagValue when
building tagSafeDns and dnsStr (use the trimmed value everywhere instead of the
original dns).
- Around line 292-305: In StaticIpOperator, avoid NPE on comparing the L3 UUID
and make the return contract non-null: null-check the
tokens.get(VmSystemTags.STATIC_DNS_L3_UUID_TOKEN) (or use Objects.equals(l3Uuid,
uuid)) instead of uuid.equals(l3Uuid) to prevent NPE, ensure dnsStr is checked
for blank before splitting, convert entries with
IPv6NetworkUtils.ipv6TagValueToAddress as before, and return an empty List (e.g.
Collections.emptyList() or new ArrayList<>()) instead of null so callers always
get a non-null list.
In
`@network/src/main/java/org/zstack/network/service/NetworkServiceManagerImpl.java`:
- Around line 493-495: The code currently calls uuid.equals(l3NetworkUuid) which
can NPE if uuid is null; change the comparison to a null-safe check (e.g. use
Objects.equals(l3NetworkUuid, uuid) or compare l3NetworkUuid.equals(uuid) after
ensuring l3NetworkUuid is non-null) in NetworkServiceManagerImpl where you read
tokens.get(VmSystemTags.STATIC_DNS_L3_UUID_TOKEN) and then compare to
l3NetworkUuid; keep the existing logic that then reads
VmSystemTags.STATIC_DNS_TOKEN when they match.
In
`@plugin/securityGroup/src/main/java/org/zstack/network/securitygroup/SecurityGroupManagerImpl.java`:
- Around line 171-177: The query in SecurityGroupManagerImpl building the sql
string incorrectly filters out IPs with ip.ipRangeUuid == null, which excludes
NICs on enableIPAM=false L3 networks; remove the "and ip.ipRangeUuid is not
null" clause (or replace it with logic that includes IPs regardless of
ipRangeUuid) in the sql construction so all UsedIpVO.ip rows for the VM NICs are
considered when assembling security group members.
---
Nitpick comments:
In `@header/src/main/java/org/zstack/header/vm/APIChangeVmNicNetworkMsg.java`:
- Around line 36-37: The API now exposes the dnsAddresses field but the example
payloads aren't showing it; update the __example__() implementations in
APIChangeVmNicNetworkMsg (and the other two __example__ methods referenced) to
include a sample dnsAddresses value (e.g., a List with one or two IP strings) so
autogenerated docs show the new field; locate the __example__ methods in class
APIChangeVmNicNetworkMsg and add dnsAddresses to the returned/example object
initialization alongside the existing sample values.
In
`@plugin/portForwarding/src/main/java/org/zstack/network/service/portforwarding/PortForwardingApiInterceptor.java`:
- Around line 152-155: In PortForwardingApiInterceptor
(validate(APIAttachPortForwardingRuleMsg)), ensure the vmNicUuid from the
message is trimmed before any null/DB checks or queries: obtain
msg.getVmNicUuid(), trim whitespace (and handle null safely), then use the
trimmed value for dbf.findByUuid and any other logic (or set it back on the msg)
so lookups like dbf.findByUuid(msg.getVmNicUuid(), VmNicVO.class) use a cleaned
identifier and avoid failures from pasted whitespace.
ℹ️ Review info
Configuration used: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
conf/globalConfig/vm.xmlis excluded by!**/*.xml
📒 Files selected for processing (32)
.gitignorecompute/src/main/java/org/zstack/compute/vm/StaticIpOperator.javacompute/src/main/java/org/zstack/compute/vm/VmCascadeExtension.javacompute/src/main/java/org/zstack/compute/vm/VmGlobalConfig.javacompute/src/main/java/org/zstack/compute/vm/VmInstanceApiInterceptor.javacompute/src/main/java/org/zstack/compute/vm/VmInstanceBase.javacompute/src/main/java/org/zstack/compute/vm/VmSystemTags.javaconf/db/upgrade/V5.5.7__schema.sqlheader/src/main/java/org/zstack/header/network/l3/UsedIpInventory.javaheader/src/main/java/org/zstack/header/network/l3/UsedIpVO.javaheader/src/main/java/org/zstack/header/network/l3/UsedIpVO_.javaheader/src/main/java/org/zstack/header/vm/APIChangeVmNicNetworkMsg.javaheader/src/main/java/org/zstack/header/vm/APISetVmStaticIpMsg.javaheader/src/main/java/org/zstack/header/vm/ChangeVmNicNetworkMsg.javaheader/src/main/java/org/zstack/header/vm/SetVmStaticIpMsg.javanetwork/src/main/java/org/zstack/network/l3/CHANGES-VmNicIpOutsideCidr.mdnetwork/src/main/java/org/zstack/network/l3/L3NetworkApiInterceptor.javanetwork/src/main/java/org/zstack/network/l3/L3NetworkManagerImpl.javanetwork/src/main/java/org/zstack/network/l3/L3NetworkSystemTags.javanetwork/src/main/java/org/zstack/network/l3/NormalIpRangeFactory.javanetwork/src/main/java/org/zstack/network/service/DhcpExtension.javanetwork/src/main/java/org/zstack/network/service/NetworkServiceManager.javanetwork/src/main/java/org/zstack/network/service/NetworkServiceManagerImpl.javaplugin/applianceVm/src/main/java/org/zstack/appliancevm/ApplianceVmNicTO.javaplugin/eip/src/main/java/org/zstack/network/service/eip/EipApiInterceptor.javaplugin/flatNetworkProvider/src/main/java/org/zstack/network/service/flat/FlatEipBackend.javaplugin/loadBalancer/src/main/java/org/zstack/network/service/lb/LoadBalancerApiInterceptor.javaplugin/portForwarding/src/main/java/org/zstack/network/service/portforwarding/PortForwardingApiInterceptor.javaplugin/securityGroup/src/main/java/org/zstack/network/securitygroup/SecurityGroupManagerImpl.javaplugin/virtualRouterProvider/src/main/java/org/zstack/network/service/virtualrouter/VirtualRouter.javaplugin/virtualRouterProvider/src/main/java/org/zstack/network/service/virtualrouter/VirtualRouterManagerImpl.javautils/src/main/java/org/zstack/utils/network/NicIpAddressInfo.java
💤 Files with no reviewable changes (1)
- network/src/main/java/org/zstack/network/l3/L3NetworkSystemTags.java
🚧 Files skipped from review as they are similar to previous changes (8)
- compute/src/main/java/org/zstack/compute/vm/VmGlobalConfig.java
- header/src/main/java/org/zstack/header/network/l3/UsedIpVO.java
- header/src/main/java/org/zstack/header/vm/SetVmStaticIpMsg.java
- network/src/main/java/org/zstack/network/l3/NormalIpRangeFactory.java
- .gitignore
- network/src/main/java/org/zstack/network/l3/L3NetworkApiInterceptor.java
- header/src/main/java/org/zstack/header/vm/APISetVmStaticIpMsg.java
- plugin/loadBalancer/src/main/java/org/zstack/network/service/lb/LoadBalancerApiInterceptor.java
Comment on lines
+6282
to
+6297
| void setDns() { | ||
| if (msg.getDnsAddresses() != null && !msg.getDnsAddresses().isEmpty()) { | ||
| new StaticIpOperator().setStaticDns(self.getUuid(), msg.getDestL3NetworkUuid(), msg.getDnsAddresses()); | ||
| isDnsSet = true; | ||
| } | ||
| } | ||
|
|
||
| void rollback() { | ||
| if (isSet) { | ||
| for (Map.Entry<String, List<String>> e : staticIpMap.entrySet()) { | ||
| new StaticIpOperator().deleteStaticIpByVmUuidAndL3Uuid(self.getUuid(), e.getKey()); | ||
| } | ||
| } | ||
| if (isDnsSet) { | ||
| new StaticIpOperator().deleteStaticDnsByVmUuidAndL3Uuid(self.getUuid(), msg.getDestL3NetworkUuid()); | ||
| } |
There was a problem hiding this comment.
回滚只删除 DNS 会丢失原配置
Line [6282]–Line [6297]（并在 Line [6303] 调用）里，setDns() 先写入目标 L3 DNS，但失败回滚仅执行 delete；如果该 VM/L3 之前已有 DNS 配置，失败后会被抹掉，属于可见配置回退不完整。
🔧 建议修复（记录旧值并在回滚时恢复）
class SetStaticIp {
private boolean isSet = false;
private boolean isDnsSet = false;
+ private List<String> oldDnsAddresses = null;
Map<String, List<String>> staticIpMap = null;
void set() {
staticIpMap = msg.getRequiredIpMap();
@@
}
void setDns() {
- if (msg.getDnsAddresses() != null && !msg.getDnsAddresses().isEmpty()) {
- new StaticIpOperator().setStaticDns(self.getUuid(), msg.getDestL3NetworkUuid(), msg.getDnsAddresses());
+ if (msg.getDnsAddresses() != null) {
+ StaticIpOperator ipOperator = new StaticIpOperator();
+ oldDnsAddresses = ipOperator.getStaticDnsByVmUuidAndL3Uuid(self.getUuid(), msg.getDestL3NetworkUuid());
+ ipOperator.setStaticDns(self.getUuid(), msg.getDestL3NetworkUuid(), msg.getDnsAddresses());
isDnsSet = true;
}
}
void rollback() {
if (isSet) {
for (Map.Entry<String, List<String>> e : staticIpMap.entrySet()) {
new StaticIpOperator().deleteStaticIpByVmUuidAndL3Uuid(self.getUuid(), e.getKey());
}
}
if (isDnsSet) {
- new StaticIpOperator().deleteStaticDnsByVmUuidAndL3Uuid(self.getUuid(), msg.getDestL3NetworkUuid());
+ StaticIpOperator ipOperator = new StaticIpOperator();
+ if (oldDnsAddresses == null || oldDnsAddresses.isEmpty()) {
+ ipOperator.deleteStaticDnsByVmUuidAndL3Uuid(self.getUuid(), msg.getDestL3NetworkUuid());
+ } else {
+ ipOperator.setStaticDns(self.getUuid(), msg.getDestL3NetworkUuid(), oldDnsAddresses);
+ }
}
}
}As per coding guidelines: “实在需要在覆盖用户的某些配置或者需要改动已有行为，应当对原状态做好记录，做好二次确认，做好回退准备”。
Also applies to: 6303-6303
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@compute/src/main/java/org/zstack/compute/vm/VmInstanceBase.java` around lines
6282 - 6297, setDns() currently overwrites the target L3 DNS with
setStaticDns(...) but rollback() only deletes the DNS via
deleteStaticDnsByVmUuidAndL3Uuid(...), which can wipe an existing user
configuration; modify the logic in VmInstanceBase so setDns() first reads and
saves the original DNS list (e.g., oldDnsAddresses) before calling
StaticIpOperator.setStaticDns(...), set a flag like isDnsSet and store the
original value, and change rollback() to restore the original DNS (call
setStaticDns with oldDnsAddresses or delete only if there was none) instead of
unconditionally deleting; update related variables and the rollback invocation
to use those saved values.
Comment on lines
+5
to
+8
| UPDATE UsedIpVO u | ||
| INNER JOIN IpRangeVO r ON u.ipRangeUuid = r.uuid | ||
| SET u.prefixLen = r.prefixLen | ||
| WHERE u.ipVersion = 6 AND u.ipRangeUuid IS NOT NULL AND u.prefixLen IS NULL; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n conf/db/upgrade/V5.5.7__schema.sqlRepository: MatheMatrix/zstack
Length of output: 1731
表名和列名需统一使用反引号包裹。
行 5-8 的 UPDATE 和行 19-24 的系统表查询中，表名、列名以及系统表标识符缺少反引号，不符合仓库 SQL 规范。根据编码指南，所有表名和列名必须使用反引号包裹，以避免 MySQL 8.0 / GreatSQL 保留关键字冲突。请修改为：
UPDATE `UsedIpVO` u
INNER JOIN `IpRangeVO` r ON u.`ipRangeUuid` = r.`uuid`
SET u.`prefixLen` = r.`prefixLen`
WHERE u.`ipVersion` = 6 AND u.`ipRangeUuid` IS NOT NULL AND u.`prefixLen` IS NULL;以及将行 19-24 修改为：
SELECT COUNT(*)
INTO constraint_exists
FROM `INFORMATION_SCHEMA`.`TABLE_CONSTRAINTS`
WHERE `TABLE_SCHEMA` = 'zstack'
AND `TABLE_NAME` = 'UsedIpVO'
AND `CONSTRAINT_NAME` = 'fkUsedIpVOIpRangeEO';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@conf/db/upgrade/V5.5.7__schema.sql` around lines 5 - 8, The SQL uses unquoted
identifiers which violates the repo's SQL style and can break on
MySQL/GreateSQL; update the statements involving UsedIpVO, IpRangeVO and the
INFORMATION_SCHEMA query to wrap all table names, column names and system schema
identifiers in backticks (e.g., `UsedIpVO`, `IpRangeVO`, `ipRangeUuid`,
`prefixLen`, `ipVersion`, `INFORMATION_SCHEMA`, `TABLE_CONSTRAINTS`,
`TABLE_SCHEMA`, `TABLE_NAME`, `CONSTRAINT_NAME`) so both the UPDATE ... INNER
JOIN (the block using UsedIpVO u and IpRangeVO r) and the SELECT ... INTO
constraint_exists query (checking fkUsedIpVOIpRangeEO) use backticked
identifiers throughout.
Comment on lines
+14
to
+40
| CREATE PROCEDURE ModifyUsedIpVOForeignKey() | ||
| BEGIN | ||
| DECLARE constraint_exists INT; | ||
|
|
||
| -- Check if the constraint exists | ||
| SELECT COUNT(*) | ||
| INTO constraint_exists | ||
| FROM INFORMATION_SCHEMA.TABLE_CONSTRAINTS | ||
| WHERE TABLE_SCHEMA = 'zstack' | ||
| AND TABLE_NAME = 'UsedIpVO' | ||
| AND CONSTRAINT_NAME = 'fkUsedIpVOIpRangeEO'; | ||
|
|
||
| IF constraint_exists > 0 THEN | ||
| -- Drop the existing constraint | ||
| ALTER TABLE `zstack`.`UsedIpVO` DROP FOREIGN KEY `fkUsedIpVOIpRangeEO`; | ||
|
|
||
| -- Re-create with SET NULL on delete | ||
| ALTER TABLE `zstack`.`UsedIpVO` | ||
| ADD CONSTRAINT `fkUsedIpVOIpRangeEO` | ||
| FOREIGN KEY (`ipRangeUuid`) REFERENCES `IpRangeEO`(`uuid`) ON DELETE SET NULL; | ||
| END IF; | ||
| END $$ | ||
|
|
||
| DELIMITER ; | ||
|
|
||
| CALL ModifyUsedIpVOForeignKey(); | ||
| DROP PROCEDURE IF EXISTS ModifyUsedIpVOForeignKey; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the specific upgrade script
find . -name "V5.5.7__schema.sql" -type fRepository: MatheMatrix/zstack
Length of output: 98
🏁 Script executed:
# Also check the general structure of database upgrade scripts
find conf/db/upgrade -name "*.sql" -type f | head -20Repository: MatheMatrix/zstack
Length of output: 733
🏁 Script executed:
# Read the V5.5.7__schema.sql file to verify the actual content
cat -n ./conf/db/upgrade/V5.5.7__schema.sqlRepository: MatheMatrix/zstack
Length of output: 1731
🏁 Script executed:
# Check a few recent upgrade scripts to see how they handle stored procedures
head -50 ./conf/db/upgrade/V4.10.16__schema.sqlRepository: MatheMatrix/zstack
Length of output: 159
🏁 Script executed:
# Find the most recent upgrade scripts to see patterns
ls -la conf/db/upgrade/*.sql | tail -10Repository: MatheMatrix/zstack
Length of output: 911
🏁 Script executed:
# Check if V4.10.16 exists with different naming
find conf/db/upgrade -name "*4.10*" -o -name "*4.9*" | sortRepository: MatheMatrix/zstack
Length of output: 96
🏁 Script executed:
# Look at a couple of recent scripts to see procedure handling patterns
cat -n conf/db/upgrade/V4.9.8__schema.sql 2>/dev/null | head -100 || echo "File not found"Repository: MatheMatrix/zstack
Length of output: 44
🏁 Script executed:
# Let's check several recent upgrade scripts for procedure handling patterns
for file in conf/db/upgrade/V5.5.6__schema.sql conf/db/upgrade/V5.5.0__schema.sql conf/db/upgrade/V5.4.6__schema.sql; do
echo "=== $file ==="
grep -A 5 -B 5 "CREATE PROCEDURE\|DROP PROCEDURE" "$file" || echo "No procedures found"
echo ""
doneRepository: MatheMatrix/zstack
Length of output: 5354
🏁 Script executed:
# Let's also check earlier versions to see the pattern
for file in conf/db/upgrade/V5.4.0__schema.sql conf/db/upgrade/V5.3.52__schema.sql; do
echo "=== $file ==="
grep -A 3 -B 3 "CREATE PROCEDURE\|DROP PROCEDURE" "$file" | head -40 || echo "No procedures found"
echo ""
doneRepository: MatheMatrix/zstack
Length of output: 481
🏁 Script executed:
# Let's verify the exact current content of V5.5.7 once more and check if there are any other procedures
wc -l conf/db/upgrade/V5.5.7__schema.sql
cat -n conf/db/upgrade/V5.5.7__schema.sqlRepository: MatheMatrix/zstack
Length of output: 1769
需要在 CREATE PROCEDURE 之前添加 DROP PROCEDURE IF EXISTS，确保升级脚本可重入。
当前脚本如果在 CREATE PROCEDURE 后但 CALL 前失败，重新执行会因存储过程已存在而中断。根据 ZStack 升级脚本规范（如 V5.4.6、V5.5.0 等），应在创建前先删除同名过程，以确保升级的幂等性。
建议修复
-- Modify ipRangeUuid foreign key constraint to SET NULL on delete (instead of CASCADE)
-- This allows UsedIpVO records to exist without an IpRange (for IPs outside range)
+DROP PROCEDURE IF EXISTS ModifyUsedIpVOForeignKey;
DELIMITER $$
CREATE PROCEDURE ModifyUsedIpVOForeignKey()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@conf/db/upgrade/V5.5.7__schema.sql` around lines 14 - 40, Add a
pre-existing-procedure check before creating the stored procedure
ModifyUsedIpVOForeignKey: insert a "DROP PROCEDURE IF EXISTS
ModifyUsedIpVOForeignKey;" (and matching DELIMITER handling if needed)
immediately before the CREATE PROCEDURE ModifyUsedIpVOForeignKey() block so the
upgrade script is reentrant and won't fail if the procedure already exists; keep
the existing CALL ModifyUsedIpVOForeignKey() and DROP PROCEDURE IF EXISTS
ModifyUsedIpVOForeignKey; after execution as-is.
Comment on lines
1
to
259
| # VM网卡IP地址支持不在L3 CIDR范围内 - 修改总结 | ||
|
|
||
| ## 功能概述 | ||
|
|
||
| 支持云主机网卡配置不在L3网络IP Range范围内的IP地址，同时支持通过QGA设置自定义DNS服务器。 | ||
|
|
||
| ## 一、DNS功能实现 | ||
|
|
||
| ### 1.1 系统标签定义 | ||
| **文件**: `compute/src/main/java/org/zstack/compute/vm/VmSystemTags.java` | ||
| ```java | ||
| // 新增DNS系统标签 | ||
| public static String STATIC_DNS_L3_UUID_TOKEN = "l3NetworkUuid"; | ||
| public static String STATIC_DNS_TOKEN = "staticDns"; | ||
| public static PatternedSystemTag STATIC_DNS = new PatternedSystemTag( | ||
| String.format("staticDns::{%s}::{%s}", STATIC_DNS_L3_UUID_TOKEN, STATIC_DNS_TOKEN), | ||
| VmInstanceVO.class); | ||
| ``` | ||
|
|
||
| ### 1.2 API消息修改 | ||
| **文件**: `header/src/main/java/org/zstack/header/vm/APISetVmStaticIpMsg.java` | ||
| ```java | ||
| @APIParam(required = false) | ||
| private List<String> dnsAddresses; | ||
| ``` | ||
|
|
||
| **文件**: `header/src/main/java/org/zstack/header/vm/SetVmStaticIpMsg.java` | ||
| ```java | ||
| private List<String> dnsAddresses; | ||
| ``` | ||
|
|
||
| **文件**: `header/src/main/java/org/zstack/header/vm/APIChangeVmNicNetworkMsg.java` | ||
| ```java | ||
| @APIParam(required = false) | ||
| private List<String> dnsAddresses; | ||
| ``` | ||
|
|
||
| **文件**: `header/src/main/java/org/zstack/header/vm/ChangeVmNicNetworkMsg.java` | ||
| ```java | ||
| private List<String> dnsAddresses; | ||
| ``` | ||
|
|
||
| ### 1.3 DNS操作方法 | ||
| **文件**: `compute/src/main/java/org/zstack/compute/vm/StaticIpOperator.java` | ||
| ```java | ||
| // 设置静态DNS | ||
| public void setStaticDns(String vmUuid, String l3Uuid, List<String> dnsAddresses) | ||
|
|
||
| // 删除静态DNS | ||
| public void deleteStaticDnsByVmUuidAndL3Uuid(String vmUuid, String l3Uuid) | ||
|
|
||
| // 获取静态DNS | ||
| public List<String> getStaticDnsByVmUuidAndL3Uuid(String vmUuid, String l3Uuid) | ||
| ``` | ||
|
|
||
| ### 1.4 DNS获取接口 | ||
| **文件**: `network/src/main/java/org/zstack/network/service/NetworkServiceManager.java` | ||
| ```java | ||
| // 新增接口方法 | ||
| List<String> getVmNicDns(String vmUuid, String l3NetworkUuid); | ||
| ``` | ||
|
|
||
| **文件**: `network/src/main/java/org/zstack/network/service/NetworkServiceManagerImpl.java` | ||
| ```java | ||
| @Override | ||
| public List<String> getVmNicDns(String vmUuid, String l3NetworkUuid) { | ||
| // 优先返回系统标签DNS，否则返回L3网络DNS | ||
| List<String> customDns = new StaticIpOperator().getStaticDnsByVmUuidAndL3Uuid(vmUuid, l3NetworkUuid); | ||
| if (customDns != null && !customDns.isEmpty()) { | ||
| return customDns; | ||
| } | ||
| return getL3NetworkDns(l3NetworkUuid); | ||
| } | ||
| ``` | ||
|
|
||
| ### 1.5 VM实例处理 | ||
| **文件**: `compute/src/main/java/org/zstack/compute/vm/VmInstanceBase.java` | ||
| - `handle(APISetVmStaticIpMsg)` - 传递dnsAddresses | ||
| - `setIpamStaticIp()` - 调用setStaticDns() | ||
| - `setNoIpamStaticIp()` - 调用setStaticDns() | ||
| - `handle(APIChangeVmNicNetworkMsg)` - 传递dnsAddresses | ||
| - `changeVmNicNetwork()` - SetStaticIp内部类处理DNS | ||
|
|
||
| ### 1.6 GuestTools集成 | ||
| **文件**: `premium/guesttools/src/main/java/org/zstack/guesttools/GuestToolsManagerImpl.java` | ||
| ```java | ||
| // 使用getVmNicDns()替代getL3NetworkDns() | ||
| ipConfig.setDns(nwServiceMgr.getVmNicDns(vmUuid, nic.getL3NetworkUuid())); | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ## 二、IP地址不在L3 CIDR范围内的约束实现 | ||
|
|
||
| ### 2.1 L3网络IP统计 | ||
| **文件**: `network/src/main/java/org/zstack/network/l3/L3NetworkManagerImpl.java` | ||
|
|
||
| **修改**: 按L3Network和Zone统计UsedIp时排除`ipRangeUuid=null`的记录 | ||
| ```sql | ||
| -- 修改前 | ||
| select count(distinct uip.ip), uip.l3NetworkUuid, uip.ipVersion | ||
| from UsedIpVO uip where uip.l3NetworkUuid in (:uuids) ... | ||
|
|
||
| -- 修改后 | ||
| select count(distinct uip.ip), uip.l3NetworkUuid, uip.ipVersion | ||
| from UsedIpVO uip where uip.l3NetworkUuid in (:uuids) | ||
| and uip.ipRangeUuid is not null ... | ||
| ``` | ||
|
|
||
| ### 2.2 添加IpRange时更新孤儿IP | ||
| **文件**: `network/src/main/java/org/zstack/network/l3/NormalIpRangeFactory.java` | ||
|
|
||
| **修改**: 只查询`ipRangeUuid=null`的UsedIpVO进行更新 | ||
| ```java | ||
| List<UsedIpVO> usedIpVos = Q.New(UsedIpVO.class) | ||
| .eq(UsedIpVO_.l3NetworkUuid, vo.getL3NetworkUuid()) | ||
| .eq(UsedIpVO_.ipVersion, vo.getIpVersion()) | ||
| .isNull(UsedIpVO_.ipRangeUuid) // 新增条件 | ||
| .list(); | ||
| ``` | ||
|
|
||
| ### 2.3 添加IpRange时校验特殊地址 | ||
| **文件**: `network/src/main/java/org/zstack/network/l3/L3NetworkApiInterceptor.java` | ||
|
|
||
| **新增**: 添加第一个IpRange时校验gateway/network/broadcast地址未被占用 | ||
| ```java | ||
| // 当添加第一个IpRange时，检查这些地址是否已被使用 | ||
| if (l3IpRanges.isEmpty()) { | ||
| // 检查gateway地址 | ||
| boolean gatewayUsed = Q.New(UsedIpVO.class) | ||
| .eq(UsedIpVO_.l3NetworkUuid, ipr.getL3NetworkUuid()) | ||
| .eq(UsedIpVO_.ip, ipr.getGateway()) | ||
| .isNull(UsedIpVO_.ipRangeUuid) | ||
| .isExists(); | ||
| if (gatewayUsed) { | ||
| throw new ApiMessageInterceptionException(...); | ||
| } | ||
| // 检查network地址和broadcast地址... | ||
| } | ||
| ``` | ||
|
|
||
| ### 2.4 DHCP配置排除CIDR外地址 | ||
| **文件**: `network/src/main/java/org/zstack/network/service/DhcpExtension.java` | ||
|
|
||
| **修改1**: `isDualStackNicInSingleL3Network()`方法过滤`ipRangeUuid=null` | ||
| ```java | ||
| List<UsedIpInventory> validIps = nic.getUsedIps().stream() | ||
| .filter(ip -> ip.getIpRangeUuid() != null) | ||
| .collect(Collectors.toList()); | ||
| ``` | ||
|
|
||
| **修改2**: `setDualStackNicOfSingleL3Network()`方法过滤`ipRangeUuid=null` | ||
|
|
||
| **修改3**: `makeDhcpStruct()`主循环跳过`ipRangeUuid=null` | ||
| ```java | ||
| for (UsedIpVO ip : nic.getUsedIps()) { | ||
| if (ip.getIpRangeUuid() == null) { | ||
| logger.debug("skip DHCP for vmnic[ip:%s] because it's outside L3 CIDR range"); | ||
| continue; | ||
| } | ||
| // ... | ||
| } | ||
| ``` | ||
|
|
||
| ### 2.5 安全组计算排除CIDR外地址 | ||
| **文件**: `plugin/securityGroup/src/main/java/org/zstack/network/securitygroup/SecurityGroupManagerImpl.java` | ||
|
|
||
| **修改**: `getVmIpsBySecurityGroup()`方法SQL添加`ip.ipRangeUuid is not null` | ||
| ```sql | ||
| select ip.ip from VmNicVO nic, VmNicSecurityGroupRefVO ref, SecurityGroupVO sg, UsedIpVO ip | ||
| where ... and ip.ipRangeUuid is not null | ||
| ``` | ||
|
|
||
| ### 2.6 EIP禁止绑定CIDR外地址 | ||
| **文件**: `plugin/eip/src/main/java/org/zstack/network/service/eip/EipApiInterceptor.java` | ||
|
|
||
| **新增**: `validate(APIAttachEipMsg)`添加检查 | ||
| ```java | ||
| UsedIpVO usedIpVO = dbf.findByUuid(msg.getUsedIpUuid(), UsedIpVO.class); | ||
| if (usedIpVO != null && usedIpVO.getIpRangeUuid() == null) { | ||
| throw new ApiMessageInterceptionException(argerr( | ||
| "cannot bindBind EIP to IP address[%s] which is outside L3 network CIDR range", | ||
| usedIpVO.getIp())); | ||
| } | ||
| ``` | ||
|
|
||
| ### 2.7 PF禁止绑定CIDR外地址 | ||
| **文件**: `plugin/portForwarding/src/main/java/org/zstack/network/service/portforwarding/PortForwardingApiInterceptor.java` | ||
|
|
||
| **新增**: `validate(APIAttachPortForwardingRuleMsg)`添加检查 | ||
| ```java | ||
| VmNicVO nicVO = dbf.findByUuid(msg.getVmNicUuid(), VmNicVO.class); | ||
| if (nicVO != null && nicVO.getUsedIpUuid() != null) { | ||
| UsedIpVO usedIpVO = dbf.findByUuid(nicVO.getUsedIpUuid(), UsedIpVO.class); | ||
| if (usedIpVO != null && usedIpVO.getIpRangeUuid() == null) { | ||
| throw new ApiMessageInterceptionException(...); | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| ### 2.8 LB禁止绑定CIDR外地址 | ||
| **文件**: `plugin/loadBalancer/src/main/java/org/zstack/network/service/lb/LoadBalancerApiInterceptor.java` | ||
|
|
||
| **新增**: `validate(APIAddVmNicToLoadBalancerMsg)`添加检查 | ||
| ```java | ||
| for (String nicUuid : msg.getVmNicUuids()) { | ||
| VmNicVO nicVO = dbf.findByUuid(nicUuid, VmNicVO.class); | ||
| if (nicVO != null && nicVO.getUsedIpUuid() != null) { | ||
| UsedIpVO usedIpVO = dbf.findByUuid(nicVO.getUsedIpUuid(), UsedIpVO.class); | ||
| if (usedIpVO != null && usedIpVO.getIpRangeUuid() == null) { | ||
| throw new ApiMessageInterceptionException(...); | ||
| } | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ## 三、测试用例 | ||
|
|
||
| **文件**: `premium/test-premium/src/test/groovy/org/zstack/test/integration/premium/vpc/networkService/VmNicIpOutsideCidrCase.groovy` | ||
|
|
||
| **测试场景**: | ||
| 1. `testSetVmNicIpOutsideCidr` - 验证设置CIDR外IP时`ipRangeUuid=null` | ||
| 2. `testEipCannotBindToIpOutsideCidr` - 验证EIP不能绑定 | ||
| 3. `testLbCannotAddNicWithIpOutsideCidr` - 验证LB不能添加 | ||
| 4. `testPfCannotBindToIpOutsideCidr` - 验证PF不能绑定 | ||
| 5. `testIpStatisticsExcludeIpOutsideCidr` - 验证IP统计排除 | ||
| 6. `testAddIpRangeAssociatesOrphanIps` - 验证添加IpRange后自动关联孤儿IP | ||
|
|
||
| --- | ||
|
|
||
| ## 四、逻辑总结表 | ||
|
|
||
| | 场景 | UsedIpVO.ipRangeUuid | 处理方式 | | ||
| |------|---------------------|---------| | ||
| | IP在CIDR范围内 | 有值 | 正常处理 | | ||
| | IP不在CIDR范围内 | null | 特殊处理 | | ||
| | L3网络IP统计 | - | 排除null记录 | | ||
| | DHCP下发 | - | 跳过null记录 | | ||
| | 安全组计算 | - | 排除null记录 | | ||
| | EIP/LB/PF绑定 | - | 禁止null记录 | | ||
| | 添加IpRange | - | 自动关联范围内的孤儿IP | | ||
|
|
||
| --- | ||
|
|
||
| ## 五、全局配置 | ||
|
|
||
| **文件**: `compute/src/main/java/org/zstack/compute/vm/VmGlobalConfig.java` | ||
| ```java | ||
| @GlobalConfigValidation(validValues = {"true", "false"}) | ||
| @GlobalConfigDef(defaultValue = "false", type = Boolean.class, | ||
| description = "Allow VM NIC to use IP address outside of L3 network IP ranges") | ||
| public static GlobalConfig ALLOW_IP_OUTSIDE_RANGE = new GlobalConfig(CATEGORY, "allow.ip.outside.range"); | ||
| ``` | ||
|
|
||
| **配置项**: `vm.allow.ip.outside.range` | ||
| - 默认值: `false` | ||
| - 设置为`true`时允许设置CIDR范围外的IP地址 |
There was a problem hiding this comment.
该文档包含大量中文内容，违反仓库统一语言规范。
建议将标题、正文说明、代码注释示例统一改为英文（含术语和错误文案示例）。
As per coding guidelines "**/*.*: 代码里不应当有有中文，包括报错、注释等都应当使用正确的、无拼写错误的英文来写"。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@network/src/main/java/org/zstack/network/l3/CHANGES-VmNicIpOutsideCidr.md`
around lines 1 - 259, The CHANGES-VmNicIpOutsideCidr.md document contains
extensive Chinese text; translate the entire file into clear, idiomatic English
(titles, descriptions, code comment examples, error message examples and the
summary table) and ensure any in-code comment samples and example error strings
shown (e.g., STATIC_DNS,
setStaticDns/deleteStaticDnsByVmUuidAndL3Uuid/getStaticDnsByVmUuidAndL3Uuid,
getVmNicDns, VmInstanceBase handlers, GuestToolsManagerImpl ipConfig.setDns,
L3NetworkManagerImpl SQL comments, NormalIpRangeFactory query,
L3NetworkApiInterceptor error messages, DhcpExtension debug log,
SecurityGroupManagerImpl/EipApiInterceptor/PortForwardingApiInterceptor/LoadBalancerApiInterceptor
exception messages, test case descriptions, and ALLOW_IP_OUTSIDE_RANGE
description) are presented in correct English with consistent terminology and no
Chinese; keep technical identifiers and code snippets unchanged except for
converting any Chinese comments or example strings to English.
Comment on lines
+214
to
231
| // First try to use prefixLen from UsedIpVO (for IP outside range) | ||
| if (ip.getPrefixLen() != null) { | ||
| struct.setPrefixLength(ip.getPrefixLen()); | ||
| struct.setFirstIp(ip.getIp()); | ||
| struct.setEndIP(ip.getIp()); | ||
| struct.setRaMode(IPv6Constants.Stateful_DHCP); | ||
| } else { | ||
| // Fallback to IpRangeVO for IP within range | ||
| List<NormalIpRangeVO> iprs = Q.New(NormalIpRangeVO.class).eq(NormalIpRangeVO_.l3NetworkUuid, ip.getL3NetworkUuid()) | ||
| .eq(NormalIpRangeVO_.ipVersion, ip.getIpVersion()).list(); | ||
| if (iprs.isEmpty() || iprs.get(0).getAddressMode().equals(IPv6Constants.SLAAC)) { | ||
| continue; | ||
| } | ||
| struct.setRaMode(iprs.get(0).getAddressMode()); | ||
| struct.setPrefixLength(iprs.get(0).getPrefixLen()); | ||
| struct.setFirstIp(NetworkUtils.getSmallestIp(iprs.stream().map(IpRangeVO::getStartIp).collect(Collectors.toList()))); | ||
| struct.setEndIP(NetworkUtils.getBiggesttIp(iprs.stream().map(IpRangeVO::getEndIp).collect(Collectors.toList()))); | ||
| } |
There was a problem hiding this comment.
prefixLen != null 不能作为“CIDR 外 IPv6”判定条件。
同一 PR 已把 in-range 的 IPv6 UsedIpVO.prefixLen 回填，当前分支会把 in-range IPv6 误判成 outside-range，导致 RA mode/firstIp/endIp 计算错误（包括 SLAAC 场景被误下发 DHCP）。
🔧 建议修复（用 ipRangeUuid == null 判定 outside-range）
- if (ip.getPrefixLen() != null) {
+ boolean outsideRangeIp = ip.getIpRangeUuid() == null;
+ if (outsideRangeIp && ip.getPrefixLen() != null) {
struct.setPrefixLength(ip.getPrefixLen());
struct.setFirstIp(ip.getIp());
struct.setEndIP(ip.getIp());
struct.setRaMode(IPv6Constants.Stateful_DHCP);
} else {
@@
- if (ip.getPrefixLen() != null) {
+ boolean outsideRangeIp = ip.getIpRangeUuid() == null;
+ if (outsideRangeIp && ip.getPrefixLen() != null) {
struct.setPrefixLength(ip.getPrefixLen());
struct.setFirstIp(ip.getIp());
struct.setEndIP(ip.getIp());
struct.setRaMode(IPv6Constants.Stateful_DHCP);
} else {
@@
- if (ip.getPrefixLen() == null) {
+ if (ip.getIpRangeUuid() != null) {
NormalIpRangeVO ipr = Q.New(NormalIpRangeVO.class)
.eq(NormalIpRangeVO_.l3NetworkUuid, ip.getL3NetworkUuid())
.eq(NormalIpRangeVO_.ipVersion, IPv6Constants.IPv6).limit(1).find();Also applies to: 249-267, 309-325
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@network/src/main/java/org/zstack/network/service/DhcpExtension.java` around
lines 214 - 231, The code incorrectly uses ip.getPrefixLen() != null to decide
"CIDR-outside" and thus misclassifies in-range IPv6 addresses; change the
branching to check ip.getIpRangeUuid() == null to detect outside-range
addresses, so in the block that currently tests ip.getPrefixLen() you should
instead test ip.getIpRangeUuid() == null and keep the assignments to struct
(setPrefixLength, setFirstIp, setEndIP, setRaMode) for true; the else branch
should remain the fallback that queries NormalIpRangeVO and sets
raMode/prefix/first/end from the range; apply the same replacement in the other
two analogous blocks that use the same prefixLen check.
Comment on lines
+38
to
+50
| // First try to use prefixLen from UsedIpInventory (for IP outside range) | ||
| if (uip.getPrefixLen() != null) { | ||
| prefixLength = uip.getPrefixLen(); | ||
| } | ||
| if (uip.getIpRangeUuid() != null) { | ||
| NormalIpRangeVO ipRangeVO = Q.New(NormalIpRangeVO.class).eq(NormalIpRangeVO_.uuid, uip.getIpRangeUuid()).find(); | ||
| if (ipRangeVO != null) { | ||
| if (prefixLength == null) { | ||
| prefixLength = ipRangeVO.getPrefixLen(); | ||
| } | ||
| addressMode = ipRangeVO.getAddressMode(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find all usages of ApplianceVmNicTO to understand downstream consumers
fd -t f "\.java$" -x grep -l "ApplianceVmNicTO" {} \;Repository: MatheMatrix/zstack
Length of output: 996
🏁 Script executed:
# Check how addressMode is used in the codebase
rg -n "\.getAddressMode\(\)" -t java -B 2 -A 2 | head -100Repository: MatheMatrix/zstack
Length of output: 12020
🏁 Script executed:
# Check NormalIpRangeVO and UsedIpInventory structure
fd -t f "NormalIpRangeVO\.java|UsedIpInventory\.java"Repository: MatheMatrix/zstack
Length of output: 324
🏁 Script executed:
# Look for patterns of addressMode assignment in similar code
rg -n "addressMode\s*=" -t java -B 3 -A 1 | head -100Repository: MatheMatrix/zstack
Length of output: 5053
🏁 Script executed:
# Check ApplianceVmNicBootstrapExtensionPoint to see how addressMode is used
cat -n plugin/applianceVm/src/main/java/org/zstack/appliancevm/ApplianceVmNicBootstrapExtensionPoint.java | grep -A 5 -B 5 "addressMode"Repository: MatheMatrix/zstack
Length of output: 44
🏁 Script executed:
# Check ApplianceVmFacadeImpl to see how ApplianceVmNicTO is used
cat -n plugin/applianceVm/src/main/java/org/zstack/appliancevm/ApplianceVmFacadeImpl.java | grep -A 5 -B 5 "addressMode\|ApplianceVmNicTO"Repository: MatheMatrix/zstack
Length of output: 2857
🏁 Script executed:
# Check the full context of ApplianceVmNicTO to see addressMode field declaration and usage
cat -n plugin/applianceVm/src/main/java/org/zstack/appliancevm/ApplianceVmNicTO.java | head -100Repository: MatheMatrix/zstack
Length of output: 3542
🏁 Script executed:
# Search for how ApplianceVmNicTO is used in VirtualRouter
rg -n -B 5 -A 5 "ApplianceVmNicTO" plugin/virtualRouterProvider/src/main/java/org/zstack/network/service/virtualrouter/VirtualRouter.javaRepository: MatheMatrix/zstack
Length of output: 588
🏁 Script executed:
# Check if ApplianceVmNicTO addressMode is actually used in serialization/deserialization
rg -n "getAddressMode\|setAddressMode" plugin/applianceVm/src/main/java/org/zstack/appliancevm/ApplianceVmNicTO.java
# Check if ApplianceVmNicTO is serialized to JSON/used in API responses
rg -n "ApplianceVmNicTO" plugin/applianceVm/src/main/java/org/zstack/appliancevm/ -A 2 -B 2 | grep -i "json\|serial\|to"Repository: MatheMatrix/zstack
Length of output: 3650
🏁 Script executed:
# Check where ApplianceVmNicBootstrapExtensionPoint is implemented to see if addressMode is used
fd -t f ".*ApplianceVmNicBootstrapExtensionPoint\.java"Repository: MatheMatrix/zstack
Length of output: 160
🏁 Script executed:
# Look at the whole context of how addressMode is used in ApplianceVmNicTO
cat -n plugin/applianceVm/src/main/java/org/zstack/appliancevm/ApplianceVmNicTO.java | tail -100Repository: MatheMatrix/zstack
Length of output: 2701
🏁 Script executed:
# Check if out-of-range IPs (IPs without ipRangeUuid) would typically be assigned to appliance VMs
rg -n "out.of.range|outside.*range" plugin/applianceVm/src/main/java/org/zstack/appliancevm/ -iRepository: MatheMatrix/zstack
Length of output: 232
🏁 Script executed:
# Check what ApplianceVmNicBootstrapExtensionPoint implementations exist
fd -t f ".*ExtensionPoint\.java|.*Extension\.java" plugin/applianceVm/ | xargs grep -l "ApplianceVmNicBootstrapExtensionPoint"Repository: MatheMatrix/zstack
Length of output: 160
🏁 Script executed:
# Check if ApplianceVmNicTO is serialized to JSON or used in any serialization
rg -n "ApplianceVmNicTO|addressMode" plugin/applianceVm/src/main/java/org/zstack/appliancevm/ApplianceVmFacadeImpl.java -A 10 | grep -E "(json|Json|serial|Serial|objectMapper|ObjectMapper|toJson|fromJson|Gson|fastjson)"Repository: MatheMatrix/zstack
Length of output: 44
🏁 Script executed:
# Check if appliance VMs have IPs outside ranges (out-of-range IPs are typically for non-appliance VMs)
rg -n "SetVmStaticIp|setNoIpam|ApplianceVm" compute/src/main/java/org/zstack/compute/vm/ -i | head -50Repository: MatheMatrix/zstack
Length of output: 2927
🏁 Script executed:
# Check the constructor of ApplianceVmNicTO more carefully - does it handle multiple UsedIpInventories?
cat -n plugin/applianceVm/src/main/java/org/zstack/appliancevm/ApplianceVmNicTO.java | sed -n '29,60p'Repository: MatheMatrix/zstack
Length of output: 1655
IPv6 地址配置支持范围外 IP，但 addressMode 缺少回退值
代码明确支持 IP 范围外的地址（通过注释"for IP outside range"），但当 ipRangeUuid 为 null 时，addressMode 将保持 null。对于 appliance VM 的引导信息，应确保 addressMode 有明确的回退值或文档说明该场景的处理方式。
建议：
- 为范围外的 IPv6 地址设置 addressMode 的默认值（如 Stateless-DHCP），或
- 在 ApplianceVmNicBootstrapExtensionPoint 的实现中显式处理 null addressMode，或
- 文档说明 appliance VM 不支持范围外的 IPv6 地址
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@plugin/applianceVm/src/main/java/org/zstack/appliancevm/ApplianceVmNicTO.java`
around lines 38 - 50, When ipRangeUuid is null the local variable addressMode
can remain null; update the ApplianceVmNicTO construction logic so addressMode
always has a deterministic fallback: after checking uip.getPrefixLen() and
attempting to load NormalIpRangeVO (via Q.New(NormalIpRangeVO.class) ...
find()), if addressMode is still null set a clear default (e.g.
"Stateless-DHCP") or validate/throw; alternatively add null-handling in
ApplianceVmNicBootstrapExtensionPoint implementations to handle null
addressMode. Locate the addressMode and prefixLength assignments in the block
that reads uip.getPrefixLen(), uip.getIpRangeUuid() and NormalIpRangeVO to apply
the default or validation change.
Comment on lines
+206
to
+212
| // Check if the IP is outside L3 CIDR range (ipRangeUuid is null) | ||
| UsedIpVO usedIpVO = dbf.findByUuid(msg.getUsedIpUuid(), UsedIpVO.class); | ||
| if (usedIpVO != null && usedIpVO.getIpRangeUuid() == null) { | ||
| throw new ApiMessageInterceptionException(argerr( | ||
| "cannot bind EIP to IP address[%s] which is outside L3 network CIDR range", | ||
| usedIpVO.getIp())); | ||
| } |
There was a problem hiding this comment.
新增校验只覆盖 Attach 路径，Create+Attach 仍可绕过 outside-CIDR 限制
Line 206-212 仅在 validate(APIAttachEipMsg) 中拦截 ipRangeUuid == null。但 validate(APICreateEipMsg) 也会设置/接收 usedIpUuid 并进入绑定流程，当前没有同等检查，存在策略不一致与绕过风险。
💡 建议修复（统一校验并在两个入口复用）
@@
private void validate(final APIAttachEipMsg msg) {
@@
- // Check if the IP is outside L3 CIDR range (ipRangeUuid is null)
- UsedIpVO usedIpVO = dbf.findByUuid(msg.getUsedIpUuid(), UsedIpVO.class);
- if (usedIpVO != null && usedIpVO.getIpRangeUuid() == null) {
- throw new ApiMessageInterceptionException(argerr(
- "cannot bind EIP to IP address[%s] which is outside L3 network CIDR range",
- usedIpVO.getIp()));
- }
+ validateUsedIpInL3CidrRange(msg.getUsedIpUuid());
}
+
+ private void validateUsedIpInL3CidrRange(String usedIpUuid) {
+ UsedIpVO usedIpVO = dbf.findByUuid(usedIpUuid, UsedIpVO.class);
+ if (usedIpVO != null && usedIpVO.getIpRangeUuid() == null) {
+ throw new ApiMessageInterceptionException(argerr(
+ "cannot bind EIP to IP address[%s] which is outside L3 network CIDR range",
+ usedIpVO.getIp()));
+ }
+ }
@@
private void validate(APICreateEipMsg msg) {
@@
- if (msg.getUsedIpUuid() != null) {
+ if (msg.getUsedIpUuid() != null) {
+ validateUsedIpInL3CidrRange(msg.getUsedIpUuid());
isVipInVmNicSubnet(msg.getVipUuid(), msg.getUsedIpUuid());
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@plugin/eip/src/main/java/org/zstack/network/service/eip/EipApiInterceptor.java`
around lines 206 - 212, The current outside-CIDR check (checking
UsedIpVO.getIpRangeUuid() == null and throwing ApiMessageInterceptionException)
exists only in validate(APIAttachEipMsg) in EipApiInterceptor, allowing
APICreateEipMsg that supplies usedIpUuid to bypass the restriction; add the same
validation to validate(APICreateEipMsg) or refactor the check into a shared
private helper (e.g., validateUsedIpOutsideCidr(UsedIpVO) or
ensureUsedIpInCidr(msg.getUsedIpUuid())) and call it from both
validate(APIAttachEipMsg) and validate(APICreateEipMsg) so attempts to bind an
EIP to an IP with ipRangeUuid == null are consistently rejected (use the same
error message and throw ApiMessageInterceptionException).
Comment on lines
+152
to
+161
| // Check if the NIC's IP is outside L3 CIDR range (ipRangeUuid is null) | ||
| VmNicVO nicVO = dbf.findByUuid(msg.getVmNicUuid(), VmNicVO.class); | ||
| if (nicVO != null && nicVO.getUsedIpUuid() != null) { | ||
| UsedIpVO usedIpVO = dbf.findByUuid(nicVO.getUsedIpUuid(), UsedIpVO.class); | ||
| if (usedIpVO != null && usedIpVO.getIpRangeUuid() == null) { | ||
| throw new ApiMessageInterceptionException(argerr( | ||
| "cannot bind port forwarding rule to IP address[%s] which is outside L3 network CIDR range", | ||
| usedIpVO.getIp())); | ||
| } | ||
| } |
There was a problem hiding this comment.
CIDR 越界校验存在可绕过路径。
当前校验放在 Line 140-142 的提前返回之后；当 peerL3NetworkUuids 已包含 guestL3Uuid 时会直接返回，导致 Line 152-161 不执行。建议把该校验前移到提前 return 之前，确保所有 attach 路径都执行。
🛠️ 建议修复
- if (vip.getPeerL3NetworkUuids() != null && vip.getPeerL3NetworkUuids().contains(guestL3Uuid)) {
- return;
- }
-
- VipBase vipBase = new VipBase(vip);
-
- try {
- vipBase.checkPeerL3Additive(guestL3Uuid);
- } catch (CloudRuntimeException e) {
- throw new ApiMessageInterceptionException(operr(ORG_ZSTACK_NETWORK_SERVICE_PORTFORWARDING_10011, e.getMessage()));
- }
-
- // Check if the NIC's IP is outside L3 CIDR range (ipRangeUuid is null)
+ // Check if the NIC's IP is outside L3 CIDR range (ipRangeUuid is null)
VmNicVO nicVO = dbf.findByUuid(msg.getVmNicUuid(), VmNicVO.class);
if (nicVO != null && nicVO.getUsedIpUuid() != null) {
UsedIpVO usedIpVO = dbf.findByUuid(nicVO.getUsedIpUuid(), UsedIpVO.class);
if (usedIpVO != null && usedIpVO.getIpRangeUuid() == null) {
throw new ApiMessageInterceptionException(argerr(
"cannot bind port forwarding rule to IP address[%s] which is outside L3 network CIDR range",
usedIpVO.getIp()));
}
}
+
+ if (vip.getPeerL3NetworkUuids() != null && vip.getPeerL3NetworkUuids().contains(guestL3Uuid)) {
+ return;
+ }
+
+ VipBase vipBase = new VipBase(vip);
+
+ try {
+ vipBase.checkPeerL3Additive(guestL3Uuid);
+ } catch (CloudRuntimeException e) {
+ throw new ApiMessageInterceptionException(operr(ORG_ZSTACK_NETWORK_SERVICE_PORTFORWARDING_10011, e.getMessage()));
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@plugin/portForwarding/src/main/java/org/zstack/network/service/portforwarding/PortForwardingApiInterceptor.java`
around lines 152 - 161, The CIDR boundary check for the VM NIC (the block that
loads VmNicVO/UsedIpVO and throws the ApiMessageInterceptionException when
usedIpVO.getIpRangeUuid() == null) must be executed for all attach paths; move
this validation earlier in the same method so it runs before the early return
that checks peerL3NetworkUuids.contains(guestL3Uuid). Concretely, relocate the
VmNicVO/UsedIpVO null-check and exception logic (variables nicVO and usedIpVO)
to precede the branch that returns when peerL3NetworkUuids includes guestL3Uuid,
ensuring the CIDR check always runs regardless of that early-return condition.
Resolves: ZSTAC-81969 Change-Id: I6c6d636275677870646270707976687a68666a6c
MatheMatrix
force-pushed
the
sync/shixin.ruan/shixin-ZSTAC-81969@@2
branch
from
962407e to
be38844
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
plugin/eip/src/main/java/org/zstack/network/service/eip/EipApiInterceptor.java (1)
206-212:⚠️ Potential issue | 🟠 Major应将 outside-CIDR 校验在 Create/Attach 两个入口统一复用，当前仍有绕过面。
Line 206-212 只在
validate(APIAttachEipMsg)做了ipRangeUuid == null拦截；validate(APICreateEipMsg)在usedIpUuid非空时仍可继续流程但未执行同等校验，策略不一致。建议提取私有方法并在两个入口统一调用。💡建议修复
@@ - // Check if the IP is outside L3 CIDR range (ipRangeUuid is null) - UsedIpVO usedIpVO = dbf.findByUuid(msg.getUsedIpUuid(), UsedIpVO.class); - if (usedIpVO != null && usedIpVO.getIpRangeUuid() == null) { - throw new ApiMessageInterceptionException(argerr( - "cannot bind EIP to IP address[%s] which is outside L3 network CIDR range", - usedIpVO.getIp())); - } + validateUsedIpInL3CidrRange(msg.getUsedIpUuid()); } + + private void validateUsedIpInL3CidrRange(String usedIpUuid) { + UsedIpVO usedIpVO = dbf.findByUuid(usedIpUuid, UsedIpVO.class); + if (usedIpVO != null && usedIpVO.getIpRangeUuid() == null) { + throw new ApiMessageInterceptionException(argerr( + "cannot bind EIP to IP address[%s] which is outside L3 network CIDR range", + usedIpVO.getIp())); + } + } @@ if (msg.getUsedIpUuid() != null) { + validateUsedIpInL3CidrRange(msg.getUsedIpUuid()); isVipInVmNicSubnet(msg.getVipUuid(), msg.getUsedIpUuid()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/eip/src/main/java/org/zstack/network/service/eip/EipApiInterceptor.java` around lines 206 - 212, Extract the "outside CIDR" validation into a private helper (e.g., checkIpOutsideCidr(UsedIpVO usedIpVO)) and call it from both validate(APIAttachEipMsg) and validate(APICreateEipMsg) when usedIpUuid is present; the helper should fetch the UsedIpVO if null, check usedIpVO.getIpRangeUuid() == null and throw the same ApiMessageInterceptionException(argerr(...)) with the existing message format, ensuring the ipRangeUuid null check is applied consistently for Create and Attach paths.compute/src/main/java/org/zstack/compute/vm/VmInstanceBase.java (1)
6288-6303:⚠️ Potential issue | 🟠 Major回滚会抹掉变更前 DNS，回退不完整。
Line [6288]-Line [6291] 先写入新 DNS，但 Line [6301]-Line [6303] 失败回滚时仅做删除，没有恢复旧值。若该 VM/L3 原本已有 DNS，失败后配置会丢失。
🔧 建议修复
class SetStaticIp { private boolean isSet = false; private boolean isDnsSet = false; + private List<String> oldDnsAddresses = null; Map<String, List<String>> staticIpMap = null; @@ void setDns() { if (msg.getDnsAddresses() != null && !msg.getDnsAddresses().isEmpty()) { - new StaticIpOperator().setStaticDns(self.getUuid(), msg.getDestL3NetworkUuid(), msg.getDnsAddresses()); + StaticIpOperator ipOperator = new StaticIpOperator(); + oldDnsAddresses = ipOperator.getStaticDnsByVmUuidAndL3Uuid(self.getUuid(), msg.getDestL3NetworkUuid()); + ipOperator.setStaticDns(self.getUuid(), msg.getDestL3NetworkUuid(), msg.getDnsAddresses()); isDnsSet = true; } } @@ if (isDnsSet) { - new StaticIpOperator().deleteStaticDnsByVmUuidAndL3Uuid(self.getUuid(), msg.getDestL3NetworkUuid()); + StaticIpOperator ipOperator = new StaticIpOperator(); + if (oldDnsAddresses == null || oldDnsAddresses.isEmpty()) { + ipOperator.deleteStaticDnsByVmUuidAndL3Uuid(self.getUuid(), msg.getDestL3NetworkUuid()); + } else { + ipOperator.setStaticDns(self.getUuid(), msg.getDestL3NetworkUuid(), oldDnsAddresses); + } } } }As per coding guidelines: “实在需要在覆盖用户的某些配置或者需要改动已有行为，应当对原状态做好记录，做好二次确认，做好回退准备，这个非常重要”。
Also applies to: 6309-6309
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@compute/src/main/java/org/zstack/compute/vm/VmInstanceBase.java` around lines 6288 - 6303, Before overwriting DNS in setDns(), capture and persist the previous DNS for this VM/L3 so rollback can restore it; change setDns() to read the current DNS (via StaticIpOperator for self.getUuid() and msg.getDestL3NetworkUuid()), store it in a new field (e.g., oldDnsMap keyed by L3 UUID) and then write the new DNS and set isDnsSet; update rollback() to check oldDnsMap: if an old DNS list exists restore it with StaticIpOperator.setStaticDns(self.getUuid(), l3Uuid, oldDns), otherwise delete the DNS (deleteStaticDnsByVmUuidAndL3Uuid) to preserve original state. Ensure you reference setDns(), rollback(), isDnsSet, staticIpMap and use msg.getDestL3NetworkUuid() and self.getUuid() when implementing the save/restore logic.network/src/main/java/org/zstack/network/l3/CHANGES-VmNicIpOutsideCidr.md (1)
1-359:⚠️ Potential issue | 🟠 Major该变更说明文档仍包含大段中文内容，需统一改为英文。
当前文件不是
_zh_cn自动本地化文件，按仓库规则应使用英文（标题、正文、注释示例、错误文案示例、表格说明都应统一转换）。As per coding guidelines "
**/*.*: 代码里不应当有有中文，包括报错、注释等都应当使用正确的、无拼写错误的英文来写"。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@network/src/main/java/org/zstack/network/l3/CHANGES-VmNicIpOutsideCidr.md` around lines 1 - 359, The CHANGES-VmNicIpOutsideCidr.md file contains extensive Chinese text (headings, section paragraphs, code comments, example messages, table entries) but must be fully converted to English; update all Chinese content to clear, idiomatic English while preserving the technical meaning and references (e.g., STATIC_DNS_L3_UUID_TOKEN, STATIC_DNS, APISetVmStaticIpMsg, ChangeVmNicNetworkMsg, StaticIpOperator, NetworkServiceManager.getVmNicDns, L3NetworkManagerImpl, NormalIpRangeFactory, L3NetworkApiInterceptor, DhcpExtension, SecurityGroupManagerImpl, EipApiInterceptor, PortForwardingApiInterceptor, LoadBalancerApiInterceptor, VmGlobalConfig, VmInstanceApiInterceptor, getStaticIp/getRequiredIpMap) so reviewers can locate the described changes, and ensure code comment examples, error message samples, and the summary table are all translated to English with consistent terminology.
🧹 Nitpick comments (3)
compute/src/main/java/org/zstack/compute/vm/VmInstanceApiInterceptor.java (1)
373-378: 建议提取长条件表达式以提高可读性。这两行代码超过 200 字符，建议提取查询逻辑为局部变量以提高可读性和可维护性。
♻️ 建议的重构方案
- if (msg.getIp() != null && !msg.getIp().isEmpty() && Q.New(UsedIpVO.class).eq(UsedIpVO_.ip, msg.getIp()).eq(UsedIpVO_.l3NetworkUuid, msg.getDestL3NetworkUuid()).isExists()) { - throw new ApiMessageInterceptionException(argerr(ORG_ZSTACK_COMPUTE_VM_10109, "the static IP[%s] has been occupied on the L3 network[uuid:%s]", msg.getIp(), msg.getDestL3NetworkUuid())); - } - if (msg.getIp6() != null && !msg.getIp6().isEmpty() && Q.New(UsedIpVO.class).eq(UsedIpVO_.ip, IPv6NetworkUtils.getIpv6AddressCanonicalString(msg.getIp6())).eq(UsedIpVO_.l3NetworkUuid, msg.getDestL3NetworkUuid()).isExists()) { - throw new ApiMessageInterceptionException(argerr(ORG_ZSTACK_COMPUTE_VM_10110, "the static IP[%s] has been occupied on the L3 network[uuid:%s]", msg.getIp6(), msg.getDestL3NetworkUuid())); - } + if (msg.getIp() != null && !msg.getIp().isEmpty()) { + boolean ipv4Occupied = Q.New(UsedIpVO.class) + .eq(UsedIpVO_.ip, msg.getIp()) + .eq(UsedIpVO_.l3NetworkUuid, msg.getDestL3NetworkUuid()) + .isExists(); + if (ipv4Occupied) { + throw new ApiMessageInterceptionException(argerr(ORG_ZSTACK_COMPUTE_VM_10109, + "the static IP[%s] has been occupied on the L3 network[uuid:%s]", + msg.getIp(), msg.getDestL3NetworkUuid())); + } + } + if (msg.getIp6() != null && !msg.getIp6().isEmpty()) { + String canonicalIp6 = IPv6NetworkUtils.getIpv6AddressCanonicalString(msg.getIp6()); + boolean ipv6Occupied = Q.New(UsedIpVO.class) + .eq(UsedIpVO_.ip, canonicalIp6) + .eq(UsedIpVO_.l3NetworkUuid, msg.getDestL3NetworkUuid()) + .isExists(); + if (ipv6Occupied) { + throw new ApiMessageInterceptionException(argerr(ORG_ZSTACK_COMPUTE_VM_10110, + "the static IP[%s] has been occupied on the L3 network[uuid:%s]", + msg.getIp6(), msg.getDestL3NetworkUuid())); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@compute/src/main/java/org/zstack/compute/vm/VmInstanceApiInterceptor.java` around lines 373 - 378, The long conditional checks in VmInstanceApiInterceptor (the two if blocks using Q.New(UsedIpVO.class)...isExists()) should be broken into local boolean variables to improve readability and maintainability; for example, compute a boolean like ipOccupied by evaluating Q.New(UsedIpVO.class).eq(UsedIpVO_.ip, msg.getIp()).eq(UsedIpVO_.l3NetworkUuid, msg.getDestL3NetworkUuid()).isExists() (and similarly ip6Occupied using IPv6NetworkUtils.getIpv6AddressCanonicalString(msg.getIp6()) for the ip field) and then use if (ipOccupied) { throw ... } and if (ip6Occupied) { throw ... } so the checks referencing msg.getIp(), msg.getIp6(), msg.getDestL3NetworkUuid(), UsedIpVO/UsedIpVO_ and IPv6NetworkUtils are extracted to clear local variables.compute/src/main/java/org/zstack/compute/vm/VmInstanceBase.java (1)
5441-5444: 同一 L3 可能重复删除 DNS，建议按 L3 去重后再删。这里按
UsedIp循环执行删除，双栈 NIC 会对同一个 L3 重复调用一次deleteStaticDnsByVmUuidAndL3Uuid。♻️ 建议改法
private void removeStaticIp() { - for (UsedIpInventory ip : nic.getUsedIps()) { - new StaticIpOperator().deleteStaticIpByVmUuidAndL3Uuid(self.getUuid(), ip.getL3NetworkUuid()); - new StaticIpOperator().deleteStaticDnsByVmUuidAndL3Uuid(self.getUuid(), ip.getL3NetworkUuid()); - } + Set<String> l3Uuids = nic.getUsedIps().stream() + .map(UsedIpInventory::getL3NetworkUuid) + .collect(Collectors.toSet()); + for (String l3Uuid : l3Uuids) { + new StaticIpOperator().deleteStaticIpByVmUuidAndL3Uuid(self.getUuid(), l3Uuid); + new StaticIpOperator().deleteStaticDnsByVmUuidAndL3Uuid(self.getUuid(), l3Uuid); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@compute/src/main/java/org/zstack/compute/vm/VmInstanceBase.java` around lines 5441 - 5444, The loop in VmInstanceBase iterates UsedIpInventory entries and may call StaticIpOperator.deleteStaticDnsByVmUuidAndL3Uuid multiple times for the same L3 (e.g., dual-stack NICs); fix it by collecting unique L3 UUIDs from nic.getUsedIps() (e.g., into a Set), then iterate that Set and call new StaticIpOperator().deleteStaticIpByVmUuidAndL3Uuid(self.getUuid(), l3Uuid) and new StaticIpOperator().deleteStaticDnsByVmUuidAndL3Uuid(self.getUuid(), l3Uuid) once per unique l3Uuid.header/src/main/java/org/zstack/header/vm/APIChangeVmNicNetworkMsg.java (1)
124-128: 可选优化：补充__example__的新字段示例值。
__example__已存在，但这次新增的ip/ip6/netmask/gateway/dnsAddresses没有示例，API 文档可读性会偏弱。As per coding guidelines "API 类需要实现 example 方法以便生成 API 文档，并确保生成对应的 Groovy API Template 与 API Markdown 文件"。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@header/src/main/java/org/zstack/header/vm/APIChangeVmNicNetworkMsg.java` around lines 124 - 128, The __example__ method for APIChangeVmNicNetworkMsg currently sets vmNicUuid and destL3NetworkUuid but omits the newly added fields (ip, ip6, netmask, gateway, dnsAddresses); update APIChangeVmNicNetworkMsg.__example__ to populate those fields with representative example values (e.g., ip="192.168.0.10", ip6="2001:db8::1", netmask="255.255.255.0", gateway="192.168.0.1", dnsAddresses=Collections.singletonList("8.8.8.8")) so the generated API docs/Groovy template include the new properties and improve readability and completeness.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@compute/src/main/java/org/zstack/compute/vm/VmInstanceBase.java`:
- Around line 6417-6431: The IPv6 branch mistakenly assigns IPv4 fields from msg
(nicVO.setIp(msg.getIp()), setGateway(msg.getGateway()),
setNetmask(msg.getNetmask())) which are empty in IPv6-only scenarios; update the
block handling msg.getIp6() so that nicVO.setIp(...) uses the canonical IPv6
string from vo (or
IPv6NetworkUtils.getIpv6AddressCanonicalString(msg.getIp6())),
nicVO.setGateway(...) uses the canonical IPv6 gateway (msg.getIpv6Gateway() via
IPv6NetworkUtils), and nicVO.setNetmask(...) uses the IPv6 netmask computed
earlier (or vo.getNetmask()); keep the rest (vo creation, vo.setIpVersion,
vo.setIpInBinary, nicVO.setUsedIpUuid, and ip range lookup) unchanged and ensure
you reference the same variables/methods (msg, nicVO, vo,
IPv6NetworkUtils.getIpv6AddressCanonicalString,
IPv6NetworkUtils.getFormalNetmaskOfNetworkCidr).
---
Duplicate comments:
In `@compute/src/main/java/org/zstack/compute/vm/VmInstanceBase.java`:
- Around line 6288-6303: Before overwriting DNS in setDns(), capture and persist
the previous DNS for this VM/L3 so rollback can restore it; change setDns() to
read the current DNS (via StaticIpOperator for self.getUuid() and
msg.getDestL3NetworkUuid()), store it in a new field (e.g., oldDnsMap keyed by
L3 UUID) and then write the new DNS and set isDnsSet; update rollback() to check
oldDnsMap: if an old DNS list exists restore it with
StaticIpOperator.setStaticDns(self.getUuid(), l3Uuid, oldDns), otherwise delete
the DNS (deleteStaticDnsByVmUuidAndL3Uuid) to preserve original state. Ensure
you reference setDns(), rollback(), isDnsSet, staticIpMap and use
msg.getDestL3NetworkUuid() and self.getUuid() when implementing the save/restore
logic.
In `@network/src/main/java/org/zstack/network/l3/CHANGES-VmNicIpOutsideCidr.md`:
- Around line 1-359: The CHANGES-VmNicIpOutsideCidr.md file contains extensive
Chinese text (headings, section paragraphs, code comments, example messages,
table entries) but must be fully converted to English; update all Chinese
content to clear, idiomatic English while preserving the technical meaning and
references (e.g., STATIC_DNS_L3_UUID_TOKEN, STATIC_DNS, APISetVmStaticIpMsg,
ChangeVmNicNetworkMsg, StaticIpOperator, NetworkServiceManager.getVmNicDns,
L3NetworkManagerImpl, NormalIpRangeFactory, L3NetworkApiInterceptor,
DhcpExtension, SecurityGroupManagerImpl, EipApiInterceptor,
PortForwardingApiInterceptor, LoadBalancerApiInterceptor, VmGlobalConfig,
VmInstanceApiInterceptor, getStaticIp/getRequiredIpMap) so reviewers can locate
the described changes, and ensure code comment examples, error message samples,
and the summary table are all translated to English with consistent terminology.
In
`@plugin/eip/src/main/java/org/zstack/network/service/eip/EipApiInterceptor.java`:
- Around line 206-212: Extract the "outside CIDR" validation into a private
helper (e.g., checkIpOutsideCidr(UsedIpVO usedIpVO)) and call it from both
validate(APIAttachEipMsg) and validate(APICreateEipMsg) when usedIpUuid is
present; the helper should fetch the UsedIpVO if null, check
usedIpVO.getIpRangeUuid() == null and throw the same
ApiMessageInterceptionException(argerr(...)) with the existing message format,
ensuring the ipRangeUuid null check is applied consistently for Create and
Attach paths.
---
Nitpick comments:
In `@compute/src/main/java/org/zstack/compute/vm/VmInstanceApiInterceptor.java`:
- Around line 373-378: The long conditional checks in VmInstanceApiInterceptor
(the two if blocks using Q.New(UsedIpVO.class)...isExists()) should be broken
into local boolean variables to improve readability and maintainability; for
example, compute a boolean like ipOccupied by evaluating
Q.New(UsedIpVO.class).eq(UsedIpVO_.ip, msg.getIp()).eq(UsedIpVO_.l3NetworkUuid,
msg.getDestL3NetworkUuid()).isExists() (and similarly ip6Occupied using
IPv6NetworkUtils.getIpv6AddressCanonicalString(msg.getIp6()) for the ip field)
and then use if (ipOccupied) { throw ... } and if (ip6Occupied) { throw ... } so
the checks referencing msg.getIp(), msg.getIp6(), msg.getDestL3NetworkUuid(),
UsedIpVO/UsedIpVO_ and IPv6NetworkUtils are extracted to clear local variables.
In `@compute/src/main/java/org/zstack/compute/vm/VmInstanceBase.java`:
- Around line 5441-5444: The loop in VmInstanceBase iterates UsedIpInventory
entries and may call StaticIpOperator.deleteStaticDnsByVmUuidAndL3Uuid multiple
times for the same L3 (e.g., dual-stack NICs); fix it by collecting unique L3
UUIDs from nic.getUsedIps() (e.g., into a Set), then iterate that Set and call
new StaticIpOperator().deleteStaticIpByVmUuidAndL3Uuid(self.getUuid(), l3Uuid)
and new StaticIpOperator().deleteStaticDnsByVmUuidAndL3Uuid(self.getUuid(),
l3Uuid) once per unique l3Uuid.
In `@header/src/main/java/org/zstack/header/vm/APIChangeVmNicNetworkMsg.java`:
- Around line 124-128: The __example__ method for APIChangeVmNicNetworkMsg
currently sets vmNicUuid and destL3NetworkUuid but omits the newly added fields
(ip, ip6, netmask, gateway, dnsAddresses); update
APIChangeVmNicNetworkMsg.__example__ to populate those fields with
representative example values (e.g., ip="192.168.0.10", ip6="2001:db8::1",
netmask="255.255.255.0", gateway="192.168.0.1",
dnsAddresses=Collections.singletonList("8.8.8.8")) so the generated API
docs/Groovy template include the new properties and improve readability and
completeness.
ℹ️ Review info
Configuration used: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
conf/globalConfig/vm.xmlis excluded by!**/*.xml
📒 Files selected for processing (8)
compute/src/main/java/org/zstack/compute/vm/VmGlobalConfig.javacompute/src/main/java/org/zstack/compute/vm/VmInstanceApiInterceptor.javacompute/src/main/java/org/zstack/compute/vm/VmInstanceBase.javaheader/src/main/java/org/zstack/header/vm/APIChangeVmNicNetworkMsg.javaheader/src/main/java/org/zstack/header/vm/ChangeVmNicNetworkMsg.javanetwork/src/main/java/org/zstack/network/l3/CHANGES-VmNicIpOutsideCidr.mdplugin/eip/src/main/java/org/zstack/network/service/eip/EipApiInterceptor.javaplugin/portForwarding/src/main/java/org/zstack/network/service/portforwarding/PortForwardingApiInterceptor.java
🚧 Files skipped from review as they are similar to previous changes (3)
- plugin/portForwarding/src/main/java/org/zstack/network/service/portforwarding/PortForwardingApiInterceptor.java
- header/src/main/java/org/zstack/header/vm/ChangeVmNicNetworkMsg.java
- compute/src/main/java/org/zstack/compute/vm/VmGlobalConfig.java
Comment on lines
+6417
to
+6431
| if (msg.getIp6() != null && !msg.getIp6().isEmpty()) { | ||
| UsedIpVO vo = new UsedIpVO(); | ||
| vo.setUuid(Platform.getUuid()); | ||
| vo.setIp(IPv6NetworkUtils.getIpv6AddressCanonicalString(nicIpAddressInfo.ipv6Address)); | ||
| vo.setNetmask(IPv6NetworkUtils.getFormalNetmaskOfNetworkCidr(nicIpAddressInfo.ipv6Address + "/" + nicIpAddressInfo.ipv6Prefix)); | ||
| vo.setGateway(nicIpAddressInfo.ipv6Gateway.isEmpty() ? "" : IPv6NetworkUtils.getIpv6AddressCanonicalString(nicIpAddressInfo.ipv6Gateway)); | ||
| vo.setIp(IPv6NetworkUtils.getIpv6AddressCanonicalString(msg.getIp6())); | ||
| vo.setNetmask(IPv6NetworkUtils.getFormalNetmaskOfNetworkCidr(msg.getIp6() + "/" + msg.getIpv6Prefix())); | ||
| vo.setGateway(msg.getIpv6Gateway() == null || msg.getIpv6Gateway().isEmpty() ? "" : IPv6NetworkUtils.getIpv6AddressCanonicalString(msg.getIpv6Gateway())); | ||
| vo.setIpVersion(IPv6Constants.IPv6); | ||
| vo.setVmNicUuid(msg.getVmNicUuid()); | ||
| vo.setL3NetworkUuid(msg.getDestL3NetworkUuid()); | ||
| vo.setIpRangeUuid(new StaticIpOperator().getIpRangeUuid(vo.getL3NetworkUuid(), vo.getIp())); | ||
| vo.setIpInBinary(NetworkUtils.ipStringToBytes(vo.getIp())); | ||
| nicVO.setUsedIpUuid(vo.getUuid()); | ||
| nicVO.setIp(nicIpAddressInfo.ipv4Address); | ||
| nicVO.setGateway(nicIpAddressInfo.ipv4Gateway); | ||
| nicVO.setNetmask(nicIpAddressInfo.ipv4Netmask); | ||
| nicVO.setIp(msg.getIp()); | ||
| nicVO.setGateway(msg.getGateway()); | ||
| nicVO.setNetmask(msg.getNetmask()); |
There was a problem hiding this comment.
IPv6 分支把 NIC 主地址字段写错了，IPv6-only 场景会落空值。
Line [6429]-Line [6431] 在 msg.getIp6() 分支里写入了 msg.getIp()/msg.getGateway()/msg.getNetmask()。当仅传 IPv6 时，这三项通常为空，nicVO 会被写成空主地址信息。
🔧 建议修复
- nicVO.setIp(msg.getIp());
- nicVO.setGateway(msg.getGateway());
- nicVO.setNetmask(msg.getNetmask());
+ nicVO.setIp(vo.getIp());
+ nicVO.setGateway(vo.getGateway());
+ nicVO.setNetmask(vo.getNetmask());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@compute/src/main/java/org/zstack/compute/vm/VmInstanceBase.java` around lines
6417 - 6431, The IPv6 branch mistakenly assigns IPv4 fields from msg
(nicVO.setIp(msg.getIp()), setGateway(msg.getGateway()),
setNetmask(msg.getNetmask())) which are empty in IPv6-only scenarios; update the
block handling msg.getIp6() so that nicVO.setIp(...) uses the canonical IPv6
string from vo (or
IPv6NetworkUtils.getIpv6AddressCanonicalString(msg.getIp6())),
nicVO.setGateway(...) uses the canonical IPv6 gateway (msg.getIpv6Gateway() via
IPv6NetworkUtils), and nicVO.setNetmask(...) uses the IPv6 netmask computed
earlier (or vo.getNetmask()); keep the rest (vo creation, vo.setIpVersion,
vo.setIpInBinary, nicVO.setUsedIpUuid, and ip range lookup) unchanged and ensure
you reference the same variables/methods (msg, nicVO, vo,
IPv6NetworkUtils.getIpv6AddressCanonicalString,
IPv6NetworkUtils.getFormalNetmaskOfNetworkCidr).
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.
APIImpact
DBImpact
GlobalConfigImpact
Resolves: ZSTAC-81969
Change-Id: I736e77646266696e646271766d7170627378796b
sync from gitlab !9122