<fix>[compute]: fix missing gateway validation for default/sole NIC static IP#3550
Open
MatheMatrix wants to merge 1 commit into5.5.12from
Open
<fix>[compute]: fix missing gateway validation for default/sole NIC static IP#3550MatheMatrix wants to merge 1 commit into5.5.12from
MatheMatrix wants to merge 1 commit into5.5.12from
Conversation
…tatic IP When user specifies netmask but not gateway for a default or sole NIC, the old code silently set gateway to empty string. This causes networking issues. Now it raises an error requiring the user to specify gateway explicitly when the custom netmask doesn't match the L3 CIDR netmask. Also fixes NicRoleContext to use srcL3Uuid (current NIC location) instead of destL3Uuid for determining default NIC role. Resolves: ZSTAC-83575 Change-Id: I07a7dbf4db404bdfa35faec77eaff1650692f529
Walkthrough通过引入 Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (1 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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.1)utils/src/main/java/org/zstack/utils/clouderrorcode/CloudOperationsErrorCode.javaComment |
Collaborator
|
Comment from zhongqi.liu: Code Review — MR !9410
亮点（不展开）
结论: BLOCK 🚫 回归风险: 高
必须修: #1 MR-Jira 关联错误 | #3 CA 私钥明文入库 | #4 shell 命令返回值未检查 🤖 Review by AI Reviewer | Findings: 🔴 2 | 🟡 4 | 🔵 2 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
compute/src/main/java/org/zstack/compute/vm/VmInstanceApiInterceptor.java (1)
1543-1543: 文件末尾缺少换行符。建议在文件末尾添加一个空行，这是大多数代码规范的常见做法。
🤖 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` at line 1543, The file VmInstanceApiInterceptor.java is missing a trailing newline at EOF; open the class file (VmInstanceApiInterceptor) and add a single blank newline character at the end of the file so the file ends with a newline, adhering to standard formatting rules.
🤖 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/StaticIpOperator.java`:
- Line 674: In StaticIpOperator.java there are inconsistent method calls to
IPv6NetworkUtils: two calls use the misspelled ipv6AddessToTagValue while a
later call uses the correct ipv6AddressToTagValue; update the two incorrect
usages (the calls currently referencing ipv6AddessToTagValue) to
ipv6AddressToTagValue so all invocations use the same correct method name
(ipv6AddressToTagValue) and recompile to ensure no references remain to the
misspelled symbol.
---
Nitpick comments:
In `@compute/src/main/java/org/zstack/compute/vm/VmInstanceApiInterceptor.java`:
- Line 1543: The file VmInstanceApiInterceptor.java is missing a trailing
newline at EOF; open the class file (VmInstanceApiInterceptor) and add a single
blank newline character at the end of the file so the file ends with a newline,
adhering to standard formatting rules.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
Run ID: b1384b18-dfb4-4d35-8e5a-595f2c92d7dd
📒 Files selected for processing (6)
compute/src/main/java/org/zstack/compute/vm/StaticIpOperator.javacompute/src/main/java/org/zstack/compute/vm/VmInstanceApiInterceptor.javadocs/modules/network/pages/networkResource/l3Ipam.adoctest/src/test/groovy/org/zstack/test/integration/networkservice/provider/flat/FlatChangeVmIpOutsideCidrCase.groovytest/src/test/groovy/org/zstack/test/integration/networkservice/provider/flat/PublicNetworkChangeVmIpOutsideCidrCase.groovyutils/src/main/java/org/zstack/utils/clouderrorcode/CloudOperationsErrorCode.java
💤 Files with no reviewable changes (1)
- test/src/test/groovy/org/zstack/test/integration/networkservice/provider/flat/PublicNetworkChangeVmIpOutsideCidrCase.groovy
| newSystags.add(VmSystemTags.IPV6_GATEWAY.instantiateTag( | ||
| map(e(VmSystemTags.IPV6_GATEWAY_L3_UUID_TOKEN, l3Uuid), | ||
| e(VmSystemTags.IPV6_GATEWAY_TOKEN, | ||
| IPv6NetworkUtils.ipv6AddressToTagValue(ipv6Result[1]))))); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 搜索 IPv6NetworkUtils 类中的方法定义
rg -n "ipv6Add.*ToTagValue" --type=javaRepository: MatheMatrix/zstack
Length of output: 1943
🏁 Script executed:
# Check file context to understand the scope and determine if methods should be standardized
head -n 10 compute/src/main/java/org/zstack/compute/vm/StaticIpOperator.java
wc -l compute/src/main/java/org/zstack/compute/vm/StaticIpOperator.javaRepository: MatheMatrix/zstack
Length of output: 559
🏁 Script executed:
# Check context around lines 201, 245, and 674 in StaticIpOperator.java
echo "=== Context around line 201 ==="
sed -n '195,210p' compute/src/main/java/org/zstack/compute/vm/StaticIpOperator.java
echo -e "\n=== Context around line 245 ==="
sed -n '240,250p' compute/src/main/java/org/zstack/compute/vm/StaticIpOperator.java
echo -e "\n=== Context around line 674 ==="
sed -n '668,680p' compute/src/main/java/org/zstack/compute/vm/StaticIpOperator.javaRepository: MatheMatrix/zstack
Length of output: 1892
统一使用正确的方法名 ipv6AddressToTagValue。
StaticIpOperator.java 中存在拼写不一致：
- 第 201、245 行使用
ipv6AddessToTagValue（缺少字母 'r'） - 第 674 行使用
ipv6AddressToTagValue（正确拼写）
IPv6NetworkUtils 中这两个方法都存在，都能编译通过。但同一文件内应保持一致。建议将第 201、245 行也更新为正确拼写的 ipv6AddressToTagValue 方法。
🤖 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` at line
674, In StaticIpOperator.java there are inconsistent method calls to
IPv6NetworkUtils: two calls use the misspelled ipv6AddessToTagValue while a
later call uses the correct ipv6AddressToTagValue; update the two incorrect
usages (the calls currently referencing ipv6AddessToTagValue) to
ipv6AddressToTagValue so all invocations use the same correct method name
(ipv6AddressToTagValue) and recompile to ensure no references remain to the
misspelled symbol.
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.
Summary
Fix missing gateway validation for default/sole NIC when user specifies custom netmask for static IP.
Changes
StaticIpOperator.java: Add error when custom netmask does not match L3 CIDR and NIC is default/soleVmInstanceApiInterceptor.java: Fix NicRoleContext to use srcL3Uuid instead of destL3UuidFlatChangeVmIpOutsideCidrCase.groovy: Add test coverageTesting
Resolves: ZSTAC-83575
sync from gitlab !9410