LTE-2824 : Avoid adding eth0 port to brlan0 bridge in warehouse mode#16
LTE-2824 : Avoid adding eth0 port to brlan0 bridge in warehouse mode#16SanthoshGujulvajagadeesh merged 2 commits intodevelopfrom
Conversation
Reason for change: Internet not reachable in warehouse mode since
OvsAgent adds eth0 (WAN) interface to brlan0 (LAN) bridge
Test Procedure:
1) In warehouse mode, after factory reset internet should be
reachable.
Risks: Low
Priority: P0
Signed-off-by: Santhosh_GujulvaJagadeesh@comcast.com
There was a problem hiding this comment.
Pull request overview
This PR prevents the eth0 (WAN) interface from being added to the brlan0 (LAN) bridge when the device is in warehouse mode, which was causing internet connectivity issues after factory reset. The fix adds a conditional check that skips the bridge port addition operation when warehouse mode is active.
Changes:
- Added warehouse mode detection logic using file existence check
- Restructured bridge port addition code with new conditional branching
- Skips eth0-to-brlan0 bridge operation in warehouse mode while logging the action
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
source/OvsAction/ovs_action.c
Outdated
| if (req && req->if_name && req->parent_bridge && | ||
| strcmp(req->if_name, "eth0") == 0 && | ||
| strcmp(req->parent_bridge, "brlan0") == 0 && | ||
| access("/tmp/warehouse_mode", F_OK) == 0) | ||
| { | ||
| v_secure_system( "ovs-vsctl add-port %s %s", req->parent_bridge,req->if_name); | ||
| OvsActionDebug("%s Cmd: ovs-vsctl add-port %s %s\n", __func__, req->parent_bridge,req->if_name); | ||
| OvsActionInfo("%s: In warehouse mode, skipping port %s addition to bridge %s\n", | ||
| __func__, req->if_name, req->parent_bridge); | ||
| } |
There was a problem hiding this comment.
The warehouse mode check that skips adding eth0 to brlan0 lacks test coverage. There are existing tests in OvsActionTest.cpp that verify bridge operations with brlan0, but none that cover the new warehouse mode behavior. Given that this is a P0 priority fix affecting network connectivity, adding test coverage for this specific scenario (verifying that eth0 is NOT added to brlan0 when warehouse_mode file exists) would help prevent regression.
source/OvsAction/ovs_action.c
Outdated
| strcmp(req->if_name, "eth0") == 0 && | ||
| strcmp(req->parent_bridge, "brlan0") == 0 && |
There was a problem hiding this comment.
The hardcoded string "eth0" should be replaced with a named constant. The codebase already defines constants for interface and bridge names (e.g., BRLAN0_ETH_NAME, LLAN0_ETH_NAME, PUMA7_ETH1_NAME) in ovs_action.h. For consistency and maintainability, "eth0" should also be defined as a constant such as ETH0_WAN_NAME and used here instead of the string literal.
| strcmp(req->if_name, "eth0") == 0 && | |
| strcmp(req->parent_bridge, "brlan0") == 0 && | |
| strcmp(req->if_name, ETH0_WAN_NAME) == 0 && | |
| strcmp(req->parent_bridge, BRLAN0_ETH_NAME) == 0 && |
| if (req && req->if_name && req->parent_bridge && | ||
| strcmp(req->if_name, "eth0") == 0 && | ||
| strcmp(req->parent_bridge, "brlan0") == 0 && | ||
| access("/tmp/warehouse_mode", F_OK) == 0) |
There was a problem hiding this comment.
The hardcoded path "/tmp/warehouse_mode" should be defined as a named constant. The codebase follows the convention of defining file paths as constants (e.g., OVS_PRIORITY_FILE, OVSAGENT_INIT_FILE, OVSAGENT_DEBUG_ENABLE). This path should be defined as a constant like WAREHOUSE_MODE_FILE at the top of the file or in a header file for consistency and maintainability.
| if (req && req->if_name && req->parent_bridge && | ||
| strcmp(req->if_name, "eth0") == 0 && | ||
| strcmp(req->parent_bridge, "brlan0") == 0 && | ||
| access("/tmp/warehouse_mode", F_OK) == 0) |
There was a problem hiding this comment.
Using a file in /tmp to detect warehouse mode creates a potential security issue through Time-of-Check-Time-of-Use (TOCTOU) race conditions. Between checking the file existence and executing the conditional logic, the file could be created or removed by another process. Additionally, /tmp is world-writable, allowing any user to create this file and potentially alter the network configuration behavior. Consider using a more robust mechanism such as reading from a system configuration file or using a dedicated API to determine the warehouse mode status.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
2 participants
Reason for change: Internet not reachable in warehouse mode since
OvsAgent adds eth0 (WAN) interface to brlan0 (LAN) bridge
Test Procedure:
reachable.
Risks: Low
Priority: P0
Signed-off-by: Santhosh_GujulvaJagadeesh@comcast.com