<fix>[l2]: null-safe type check in ChangeL2NetworkVlanId#3722
Closed
zstack-robot-2 wants to merge 1 commit intofeature-5.5.12-znsfrom
Closed
<fix>[l2]: null-safe type check in ChangeL2NetworkVlanId#3722zstack-robot-2 wants to merge 1 commit intofeature-5.5.12-znsfrom
zstack-robot-2 wants to merge 1 commit intofeature-5.5.12-znsfrom
Conversation
Use CONSTANT.equals(msg.getType()) instead of msg.getType().equals(CONSTANT) to prevent NullPointerException when the optional 'type' parameter is null in APIChangeL2NetworkVlanIdMsg. This fixes SYS.1000 internal error when calling ChangeL2NetworkVlanId on L2GeneveNetwork without specifying type. Affected files: - L2NetworkApiInterceptor: lines 151, 195 - L2NoVlanNetwork: lines 425, 433 - VxlanNetworkCheckerImpl: line 41 Resolves: ZCF-2074 Change-Id: I1ae26712bf3355d9a949be819cd8d08a26acdfc1 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
功能概览三个L2网络验证模块的字符串比较方式从 变更清单
预估代码审查工作量🎯 2 (Simple) | ⏱️ ~10 minutes 庆祝诗句
🚥 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 docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
network/src/main/java/org/zstack/network/l2/L2NetworkApiInterceptor.java (1)
149-193:⚠️ Potential issue | 🟠 Major建议在拦截器先归一化
type，避免空值漏校验
Line 149和Line 193的空安全比较解决了 NPE，但当msg.getType() == null时，两条分支都不会命中，当前请求将绕过 VLAN/NO_VLAN 约束校验。建议在此方法开头先计算effectiveType（为空时回退到l2.getType()），后续统一按effectiveType校验。💡 建议修复
private void validate(APIChangeL2NetworkVlanIdMsg msg) { L2NetworkVO l2 = dbf.findByUuid(msg.getL2NetworkUuid(), L2NetworkVO.class); + String effectiveType = msg.getType() == null ? l2.getType() : msg.getType(); + l2.getAttachedClusterRefs().forEach(ref -> { ... }); @@ - if (L2NetworkConstant.L2_VLAN_NETWORK_TYPE.equals(msg.getType())) { + if (L2NetworkConstant.L2_VLAN_NETWORK_TYPE.equals(effectiveType)) { ... - } else if (L2NetworkConstant.L2_NO_VLAN_NETWORK_TYPE.equals(msg.getType())) { + } else if (L2NetworkConstant.L2_NO_VLAN_NETWORK_TYPE.equals(effectiveType)) { ... } }🤖 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/l2/L2NetworkApiInterceptor.java` around lines 149 - 193, The interceptor currently branches on msg.getType() and can skip VLAN checks if msg.getType() is null; update L2NetworkApiInterceptor to compute an effectiveType at the start (e.g. effectiveType = msg.getType() == null ? l2.getType() : msg.getType()) and then replace usages of msg.getType() in the VLAN/NO_VLAN conditionals with effectiveType so null msg types fall back to l2.getType() and are validated correctly (adjust any string comparisons like L2NetworkConstant.L2_VLAN_NETWORK_TYPE and L2NetworkConstant.L2_NO_VLAN_NETWORK_TYPE to use effectiveType).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@network/src/main/java/org/zstack/network/l2/L2NoVlanNetwork.java`:
- Around line 425-433: The null/empty msg.getType() can flow past the
early-return in L2NoVlanNetwork and later be persisted into L2NetworkVO.type,
causing invalid DB state; update the message handling in L2NoVlanNetwork (where
the code checks L2NetworkConstant.L2_NO_VLAN_NETWORK_TYPE and
L2NetworkConstant.L2_VLAN_NETWORK_TYPE) to explicitly guard msg.getType() for
null/empty (e.g., return/error) before any further logic, and ensure the code
path that writes back to L2NetworkVO.type (the block that uses msg.getType() as
targetType) only uses a non-null, validated type or substitutes a safe
default/throws a controlled error.
In
`@plugin/vxlan/src/main/java/org/zstack/network/l2/vxlan/vxlanNetworkPool/VxlanNetworkCheckerImpl.java`:
- Around line 41-43: The check in VxlanNetworkCheckerImpl currently returns when
msg.getType() is null, skipping VNI validation; instead compute an effectiveType
(use msg.getType() if non-null, otherwise load the L2 network by
msg.getL2NetworkUuid() and read its type) and compare that effectiveType to
VxlanNetworkConstant.VXLAN_NETWORK_TYPE before running the VNI
validity/duplication checks present in the VNI check block (the logic around the
current 44-60 region). Ensure the code uses the effectiveType variable in the
comparison and only returns early for non-VXLAN effective types so null type
messages still trigger VXLAN VNI checks.
---
Outside diff comments:
In `@network/src/main/java/org/zstack/network/l2/L2NetworkApiInterceptor.java`:
- Around line 149-193: The interceptor currently branches on msg.getType() and
can skip VLAN checks if msg.getType() is null; update L2NetworkApiInterceptor to
compute an effectiveType at the start (e.g. effectiveType = msg.getType() ==
null ? l2.getType() : msg.getType()) and then replace usages of msg.getType() in
the VLAN/NO_VLAN conditionals with effectiveType so null msg types fall back to
l2.getType() and are validated correctly (adjust any string comparisons like
L2NetworkConstant.L2_VLAN_NETWORK_TYPE and
L2NetworkConstant.L2_NO_VLAN_NETWORK_TYPE to use effectiveType).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
Run ID: 345aa2a3-768c-4825-9f3e-1ce845c9ff4a
📒 Files selected for processing (3)
network/src/main/java/org/zstack/network/l2/L2NetworkApiInterceptor.javanetwork/src/main/java/org/zstack/network/l2/L2NoVlanNetwork.javaplugin/vxlan/src/main/java/org/zstack/network/l2/vxlan/vxlanNetworkPool/VxlanNetworkCheckerImpl.java
Comment on lines
+425
to
433
| && L2NetworkConstant.L2_NO_VLAN_NETWORK_TYPE.equals(msg.getType()) | ||
| && self.getVirtualNetworkId().equals(0)) { | ||
| event.setInventory(getSelfInventory()); | ||
| bus.publish(event); | ||
| return; | ||
| } | ||
| if (msg.getVlan() != null | ||
| && msg.getType().equals(L2NetworkConstant.L2_VLAN_NETWORK_TYPE) | ||
| && L2NetworkConstant.L2_VLAN_NETWORK_TYPE.equals(msg.getType()) | ||
| && self.getVirtualNetworkId().equals(msg.getVlan())) { |
There was a problem hiding this comment.
type 为空时会落入后续写库路径，存在写入非法类型风险
Line 425-433 现在不会再因空值抛 NPE，但空 type 会继续执行到后续链路；同文件 Line 545-553 会把 msg.getType() 作为 targetType 写回 L2NetworkVO.type。当 type == null 时，这里会产生错误状态或数据库异常。
💡 建议修复
private void handle(APIChangeL2NetworkVlanIdMsg msg){
APIChangeL2NetworkVlanIdEvent event = new APIChangeL2NetworkVlanIdEvent(msg.getId());
+ String effectiveType = msg.getType() == null ? self.getType() : msg.getType();
+ if (msg.getType() == null) {
+ msg.setType(effectiveType);
+ }
+
if (msg.getVlan() == null
- && L2NetworkConstant.L2_NO_VLAN_NETWORK_TYPE.equals(msg.getType())
+ && L2NetworkConstant.L2_NO_VLAN_NETWORK_TYPE.equals(effectiveType)
&& self.getVirtualNetworkId().equals(0)) {
event.setInventory(getSelfInventory());
bus.publish(event);
return;
}
if (msg.getVlan() != null
- && L2NetworkConstant.L2_VLAN_NETWORK_TYPE.equals(msg.getType())
+ && L2NetworkConstant.L2_VLAN_NETWORK_TYPE.equals(effectiveType)
&& self.getVirtualNetworkId().equals(msg.getVlan())) {
event.setInventory(getSelfInventory());
bus.publish(event);
return;
}🤖 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/l2/L2NoVlanNetwork.java` around
lines 425 - 433, The null/empty msg.getType() can flow past the early-return in
L2NoVlanNetwork and later be persisted into L2NetworkVO.type, causing invalid DB
state; update the message handling in L2NoVlanNetwork (where the code checks
L2NetworkConstant.L2_NO_VLAN_NETWORK_TYPE and
L2NetworkConstant.L2_VLAN_NETWORK_TYPE) to explicitly guard msg.getType() for
null/empty (e.g., return/error) before any further logic, and ensure the code
path that writes back to L2NetworkVO.type (the block that uses msg.getType() as
targetType) only uses a non-null, validated type or substitutes a safe
default/throws a controlled error.
Comment on lines
+41
to
43
| if (!VxlanNetworkConstant.VXLAN_NETWORK_TYPE.equals(msg.getType())){ | ||
| return; | ||
| } |
There was a problem hiding this comment.
空 type 会直接绕过 VXLAN 的 VNI 校验路径
Line 41 在 type == null 时会直接 return，导致后续 Line 44-60 的 VNI 合法性/重复性检查被跳过。type 是可选字段，这里建议先推导 effectiveType（为空时按当前 L2 实际类型判断），再决定是否进入 VXLAN 校验。
💡 建议修复
private void validate(APIChangeL2NetworkVlanIdMsg msg) {
- if (!VxlanNetworkConstant.VXLAN_NETWORK_TYPE.equals(msg.getType())){
+ String effectiveType = msg.getType();
+ if (effectiveType == null) {
+ effectiveType = Q.New(L2NetworkVO.class)
+ .select(L2NetworkVO_.type)
+ .eq(L2NetworkVO_.uuid, msg.getL2NetworkUuid())
+ .findValue();
+ }
+ if (!VxlanNetworkConstant.VXLAN_NETWORK_TYPE.equals(effectiveType)) {
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (!VxlanNetworkConstant.VXLAN_NETWORK_TYPE.equals(msg.getType())){ | |
| return; | |
| } | |
| String effectiveType = msg.getType(); | |
| if (effectiveType == null) { | |
| effectiveType = Q.New(L2NetworkVO.class) | |
| .select(L2NetworkVO_.type) | |
| .eq(L2NetworkVO_.uuid, msg.getL2NetworkUuid()) | |
| .findValue(); | |
| } | |
| if (!VxlanNetworkConstant.VXLAN_NETWORK_TYPE.equals(effectiveType)) { | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@plugin/vxlan/src/main/java/org/zstack/network/l2/vxlan/vxlanNetworkPool/VxlanNetworkCheckerImpl.java`
around lines 41 - 43, The check in VxlanNetworkCheckerImpl currently returns
when msg.getType() is null, skipping VNI validation; instead compute an
effectiveType (use msg.getType() if non-null, otherwise load the L2 network by
msg.getL2NetworkUuid() and read its type) and compare that effectiveType to
VxlanNetworkConstant.VXLAN_NETWORK_TYPE before running the VNI
validity/duplication checks present in the VNI check block (the logic around the
current 44-60 region). Ensure the code uses the effectiveType variable in the
comparison and only returns early for non-VXLAN effective types so null type
messages still trigger VXLAN VNI checks.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Use CONSTANT.equals(msg.getType()) instead of msg.getType().equals(CONSTANT)
to prevent NullPointerException when the optional 'type' parameter is null
in APIChangeL2NetworkVlanIdMsg. This fixes SYS.1000 internal error when
calling ChangeL2NetworkVlanId on L2GeneveNetwork without specifying type.
Affected files:
Resolves: ZCF-2074
Change-Id: I1ae26712bf3355d9a949be819cd8d08a26acdfc1
Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com
sync from gitlab !9590