Skip to content

<fix>[sdnController]: set lacp mode when change host #3149

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
MatheMatrix wants to merge 1 commit into from
+11 −3
Closed

<fix>[sdnController]: set lacp mode when change host #3149

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions plugin/sdnController/src/main/java/org/zstack/sdnController/SdnControllerApiInterceptor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,16 +200,24 @@ private void validate(APISdnControllerChangeHostMsg msg) {
msg.setNicNames(new ArrayList<>(nicNamePciAddressMap.keySet()));
}

if (msg.getNicNames().size() > 1 && msg.getBondMode() == null) {
if (msg.getBondMode() == null) {
msg.setBondMode(refVO.getBondMode());
}

if (msg.getLacpMode() == null) {
if (msg.getBondMode().equals(refVO.getBondMode())) {
msg.setLacpMode(refVO.getLacpMode());
}
Comment on lines +207 to 209
Copy link

@coderabbitai coderabbitai bot Dec 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

严重问题：存在 NullPointerException 风险且代码逻辑存在缺陷。

第 207 行存在两个问题：

  1. NPE 风险：如果 refVO.getBondMode() 返回 null，那么第 204 行会将 msg.setBondMode(null)，导致第 207 行 msg.getBondMode().equals() 抛出 NullPointerException。

  2. 无效代码：第 208 行设置的 lacpMode 会被第 211-221 行的逻辑无条件覆盖。无论第 208 行设置什么值，后续的 if-else 链都会重新设置 lacpMode。这使得第 207-209 行的代码实际上没有任何作用。

🔎 建议的修复方案

需要重新设计此逻辑。如果意图是在 bondMode 未改变时保留原有 lacpMode，则应：

 if (msg.getBondMode() == null) {
     msg.setBondMode(refVO.getBondMode());
 }

+if (msg.getBondMode() == null) {
+    throw new ApiMessageInterceptionException(argerr("could not change host[uuid:%s] of sdn controller[uuid:%s], " +
+            "because bondMode is not specified and reference bondMode is null", msg.getHostUuid(), msg.getSdnControllerUuid()));
+}
+
-if (msg.getBondMode().equals(refVO.getBondMode())) {
-    msg.setLacpMode(refVO.getLacpMode());
-}
-
-if (msg.getBondMode().equals(L2NetworkConstant.BONDING_MODE_TCP)) {
+if (msg.getBondMode().equals(refVO.getBondMode()) && msg.getLacpMode() == null) {
+    msg.setLacpMode(refVO.getLacpMode());
+} else if (msg.getLacpMode() == null && msg.getBondMode().equals(L2NetworkConstant.BONDING_MODE_TCP)) {
     msg.setLacpMode(L2NetworkConstant.LACP_MODE_ACTIVE);
 } else if (msg.getBondMode().equals(L2NetworkConstant.BONDING_MODE_SLB)) {
     if (msg.getLacpMode() == null) {
         msg.setLacpMode(L2NetworkConstant.LACP_MODE_OFF);
     } else if (!(msg.getLacpMode().equals(L2NetworkConstant.LACP_MODE_PASSIVE))) {
         msg.setLacpMode(L2NetworkConstant.LACP_MODE_OFF);
     }
-} else {
+} else if (msg.getLacpMode() == null) {
     msg.setLacpMode(L2NetworkConstant.LACP_MODE_OFF);
 }

注意：此修复假设只在用户未提供 lacpMode 时才进行默认赋值。请根据实际业务需求调整。

🤖 Prompt for AI Agents 
In
plugin/sdnController/src/main/java/org/zstack/sdnController/SdnControllerApiInterceptor.java
around lines 207-209, the current bond/lacp handling risks a
NullPointerException and is redundant because lacpMode set here is always
overwritten later; change the logic to safely compare bond modes (use null-safe
comparison or check for null before calling equals) and only assign lacpMode
here if the bondMode did not change AND the incoming msg has no lacpMode
provided (i.e., msg.getLacpMode() == null), or alternatively remove this early
assignment and adjust the later if/else chain to preserve an existing lacpMode
when bondMode is unchanged; ensure no code path leaves an NPE risk by guarding
against nulls in comparisons.


if (msg.getBondMode() != null && msg.getBondMode().equals(L2NetworkConstant.BONDING_MODE_TCP) && msg.getLacpMode() == null) {
if (msg.getBondMode().equals(L2NetworkConstant.BONDING_MODE_TCP)) {
msg.setLacpMode(L2NetworkConstant.LACP_MODE_ACTIVE);
Comment on lines +211 to 212
Copy link

@coderabbitai coderabbitai bot Dec 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

主要问题：用户提供的 lacpMode 被无条件覆盖。

当 bondMode 为 TCP 时，第 212 行无条件将 lacpMode 设置为 ACTIVE，即使用户在 API 请求中明确指定了其他 lacpMode 值。这可能违反用户意图，且与 API 契约不符（lacpMode 是可选参数，暗示用户可以自定义）。

建议仅在 msg.getLacpMode() == null 时设置默认值：

 if (msg.getBondMode().equals(L2NetworkConstant.BONDING_MODE_TCP)) {
-    msg.setLacpMode(L2NetworkConstant.LACP_MODE_ACTIVE);
+    if (msg.getLacpMode() == null) {
+        msg.setLacpMode(L2NetworkConstant.LACP_MODE_ACTIVE);
+    }
 }
🤖 Prompt for AI Agents 
In
plugin/sdnController/src/main/java/org/zstack/sdnController/SdnControllerApiInterceptor.java
around lines 211-212, the code unconditionally overwrites the user-supplied
lacpMode when bondMode is TCP; change the logic so you only set the default
LACP_MODE_ACTIVE if msg.getLacpMode() == null (i.e., preserve any non-null
user-provided value), keeping the bondMode check first and using a null-safe
check before calling msg.setLacpMode(...).

} else if (msg.getBondMode().equals(L2NetworkConstant.BONDING_MODE_SLB)) {
if (msg.getLacpMode() == null) {
msg.setLacpMode(L2NetworkConstant.LACP_MODE_OFF);
} else if (!(msg.getLacpMode().equals(L2NetworkConstant.LACP_MODE_PASSIVE))) {
msg.setLacpMode(L2NetworkConstant.LACP_MODE_OFF);
}
} else {
msg.setLacpMode(L2NetworkConstant.LACP_MODE_OFF);
}
}

Expand Down