[Nexthop] [fboss2-dev] Add CLI commands for interface IP addresses and static routes#1118
[Nexthop] [fboss2-dev] Add CLI commands for interface IP addresses and static routes#1118manoharan-nexthop wants to merge 1 commit into
Conversation
joseph5wu
left a comment
joseph5wu
left a comment
There was a problem hiding this comment.
Unfortunately this PR slipped out of my radar, and it's been old now.
Can you rebase and address all my comments?
| for (const auto& itr : std::as_const(*this)) { | ||
| const auto& intf = itr.second; | ||
| if (intf->getVlanID() == vlan) { | ||
| if (intf->getVlanIDHelper() == vlan) { |
There was a problem hiding this comment.
@manoharan-nexthop What's the reason to change the vlan function here?
There was a problem hiding this comment.
Mano's recollection is that for normal interfaces, without intf->getVlanIDHelper(), it was crashing trying to fetch the VLAN ID. I've removed this still and will see if I can repro the crash.
| * For example (IPv4): | ||
| * 10.0.0.0/8 192.168.1.1 # Single nexthop | ||
| * 10.0.0.0/8 192.168.1.1 192.168.1.2 # ECMP with two nexthops | ||
| * 10.0.0.0/8 null0 # Null route (drop) |
There was a problem hiding this comment.
Can't we just use null? I don't think null is a keyword for our fboss2 or cli.
There was a problem hiding this comment.
null0 is the name of the interface, it's referenced in other places e.g.
configerator/structs/neteng/bgp_policy/thrift/routing_policy.thrift:165: // example: ipv6 route 2803:6082:f822:400::/54 Null0
| template <network::thrift::AddressType IPVersion> | ||
| class StaticRouteAddArgBase : public utils::BaseObjectArgType<std::string> { | ||
| public: | ||
| /* implicit */ StaticRouteAddArgBase( // NOLINT(google-explicit-constructor) |
There was a problem hiding this comment.
Please consider to move the implementation to .cpp and .h light-weighted
There was a problem hiding this comment.
This is template code, it needs to be in the .h.
There was a problem hiding this comment.
Why do you want to delete the CmdDeleteConfig.cpp/.h here?
| // Validate IP address version matches attribute | ||
| auto [ip, prefixLen] = folly::IPAddress::createNetwork(ipAddress); | ||
| bool expectV6 = (attribute == "ipv6-address"); | ||
|
|
||
| if (expectV6 && !ip.isV6()) { | ||
| throw std::invalid_argument( | ||
| fmt::format( | ||
| "Expected IPv6 address for 'ipv6-address', got IPv4: {}", | ||
| ipAddress)); | ||
| } | ||
| if (!expectV6 && ip.isV6()) { | ||
| throw std::invalid_argument( | ||
| fmt::format( | ||
| "Expected IPv4 address for 'ip-address', got IPv6: {}", ipAddress)); | ||
| } |
There was a problem hiding this comment.
Can we put this validation logic in InterfaceDeleteConfig::InterfaceDeleteConfig()?
So that we can always assume when InterfaceDeleteConfig is created, the user input should be valid.
There was a problem hiding this comment.
Our internal review had actually raised something similar and we factored it out in a new header.
| OBJECT_ARG_TYPE_STATIC_ROUTE_PREFIX, | ||
| OBJECT_ARG_TYPE_STATIC_ROUTE_V6, | ||
| OBJECT_ARG_TYPE_STATIC_ROUTE_PREFIX_V6, | ||
| }; |
There was a problem hiding this comment.
Please don't introduce new OBJECT_ARG_TYPE for config cli.
#1112 introduced per-Traits addCliArg method to replace the central ObjectArgTypeId enum. Please adjust this PR accordingly
| // since we do a lot of work of figuring out argument types at compile time, | ||
| // for now we need to know the the theoretical max depth of nested subcommands | ||
| static constexpr auto MAX_DEPTH = 5; | ||
| static constexpr auto MAX_DEPTH = 6; |
There was a problem hiding this comment.
Can you put a comment here to use the example of the max depth(6) here for better understanding?
There was a problem hiding this comment.
Mano told me that previously the code was using ip address in the CLI, which required an increase in depth, but after discussions we changed it to ip-address so this wasn't needed anymore, and he just forgot to reduce the max depth. I've removed this change from the PR.
| }}, | ||
| }, | ||
|
|
||
| {"delete", |
There was a problem hiding this comment.
Please don't delete this cli
| "<port-list> [<attr> <value> ...] where <attr> is one " | ||
| "of: description, mtu"); | ||
| break; | ||
| case utils::ObjectArgTypeId::OBJECT_ARG_TYPE_ID_INTERFACE_DELETE_CONFIG: |
There was a problem hiding this comment.
As comment above, please read #1112 to adjust your PR here
benoit-nexthop
force-pushed
the
manoharan.ip-route-cmds
branch
from
62409d1 to
15b3bea
Compare
|
@manoharan-nexthop has updated the pull request. You must reimport the pull request before landing. |
benoit-nexthop
force-pushed
the
manoharan.ip-route-cmds
branch
from
15b3bea to
0c3679d
Compare
|
@manoharan-nexthop has updated the pull request. You must reimport the pull request before landing. |
benoit-nexthop
force-pushed
the
manoharan.ip-route-cmds
branch
from
0c3679d to
bd18480
Compare
|
@manoharan-nexthop has updated the pull request. You must reimport the pull request before landing. |
benoit-nexthop
force-pushed
the
manoharan.ip-route-cmds
branch
from
bd18480 to
fd1393e
Compare
|
@manoharan-nexthop has updated the pull request. You must reimport the pull request before landing. |
|
@joseph5wu has imported this pull request. If you are a Meta employee, you can view this in D106142019. |
joseph5wu
left a comment
joseph5wu
left a comment
There was a problem hiding this comment.
The core "find a route by (routerID, prefix)" predicate is duplicated 12 times across CmdConfigProtocolStaticRouteAdd.cpp and CmdDeleteProtocolStaticRoute.cpp:
std::find_if(routes.begin(), routes.end(), [vrfId, &prefix](const auto& route) {
return *route.routerID() == vrfId && *route.prefix() == prefix;
});
And in addStaticRouteImpl, each of the three branches (null0 / cpu / nexthop) independently re-implements "remove this prefix from the other two route lists" — so the same erase-from-list block is copy-pasted across branches. This is the most
error-prone part of the change: the route-list juggling is exactly where a missed list or a wrong comparison would silently corrupt the staged config, and right now correctness depends on every copy staying in sync.
Could we pull this into a small shared header (mirroring the existing InterfaceIpUtils.h), e.g. StaticRouteUtils.h:
| auto it = std::find_if( | ||
| nullRoutes.begin(), | ||
| nullRoutes.end(), | ||
| [vrfId, &prefix](const auto& route) { | ||
| return *route.routerID() == vrfId && *route.prefix() == prefix; | ||
| }); |
There was a problem hiding this comment.
I noticed this find-route-by-vrfId+prefix lambda is repeated 12 times across the whole PR.
Can we define StaticRouteUtils.h to have this common function:
template <typename RouteList>
auto findStaticRoute(RouteList& routes, int32_t vrfId, const std::string& prefix) {
return std::find_if(routes.begin(), routes.end(), [&](const auto& r) {
return *r.routerID() == vrfId && *r.prefix() == prefix;
});
}
| auto& nhopRoutes = *swConfig.staticRoutesWithNhops(); | ||
| auto nhopIt = std::find_if( | ||
| nhopRoutes.begin(), | ||
| nhopRoutes.end(), | ||
| [vrfId, &prefix](const auto& route) { | ||
| return *route.routerID() == vrfId && *route.prefix() == prefix; | ||
| }); | ||
| if (nhopIt != nhopRoutes.end()) { | ||
| nhopRoutes.erase(nhopIt); | ||
| found = true; | ||
| } | ||
|
|
||
| auto& nullRoutes = *swConfig.staticRoutesToNull(); | ||
| auto nullIt = std::find_if( | ||
| nullRoutes.begin(), | ||
| nullRoutes.end(), | ||
| [vrfId, &prefix](const auto& route) { | ||
| return *route.routerID() == vrfId && *route.prefix() == prefix; | ||
| }); | ||
| if (nullIt != nullRoutes.end()) { | ||
| nullRoutes.erase(nullIt); | ||
| found = true; | ||
| } | ||
|
|
||
| auto& cpuRoutes = *swConfig.staticRoutesToCPU(); | ||
| auto cpuIt = std::find_if( | ||
| cpuRoutes.begin(), cpuRoutes.end(), [vrfId, &prefix](const auto& route) { | ||
| return *route.routerID() == vrfId && *route.prefix() == prefix; | ||
| }); | ||
| if (cpuIt != cpuRoutes.end()) { | ||
| cpuRoutes.erase(cpuIt); | ||
| found = true; | ||
| } |
There was a problem hiding this comment.
also seeing lots of erase Static Route logic is repeated across the whole PR too.
Maybe we can introduce eraseStaticRoute() in StaticRouteUtils.h
// Returns true if a matching route was present and erased.
template <typename RouteList>
bool eraseStaticRoute(RouteList& routes, int32_t vrfId, const std::string& prefix) {
auto it = findStaticRoute(routes, vrfId, prefix);
if (it == routes.end()) {
return false;
}
routes.erase(it);
return true;
}
Then each add branch reduces to its real intent — "remove this prefix everywhere else, then add it here":
// e.g. the null0 branch
auto& nullRoutes = *swConfig.staticRoutesToNull();
if (findStaticRoute(nullRoutes, vrfId, prefix) != nullRoutes.end()) {
return fmt::format("Static null0 route {} (VRF {}) already exists", prefix, vrfId);
}
eraseStaticRoute(*swConfig.staticRoutesWithNhops(), vrfId, prefix);
eraseStaticRoute(*swConfig.staticRoutesToCPU(), vrfId, prefix);
nullRoutes.push_back(newRoute);
fd1393e to
ea28d32
Compare
|
@manoharan-nexthop has updated the pull request. You must reimport the pull request before landing. |
|
@manoharan-nexthop In your last change, you replaced lots of places to use cc: @benoit-nexthop Will Nexthop internal CI block a commit with compiling error like this before publish in our github? |
benoit-nexthop
force-pushed
the
manoharan.ip-route-cmds
branch
from
ea28d32 to
cbe8461
Compare
|
@manoharan-nexthop has updated the pull request. You must reimport the pull request before landing. |
benoit-nexthop
force-pushed
the
manoharan.ip-route-cmds
branch
from
cbe8461 to
6acf2b4
Compare
|
@manoharan-nexthop has updated the pull request. You must reimport the pull request before landing. |
This is entirely my fault, I forgot to |
- Add config ip/ipv6 route add commands with nexthop support - Add delete config ip/ipv6 route commands (moved from config tree to delete tree) - Add delete config interface ip-address/ipv6-address commands - Add parent commands for config ip and delete config ip/ipv6 - Update integration tests to use new delete command structure - Add unit tests and integration tests for all new commands Address issues found during testing Fix crash in InterfaceMap::getInterfaceInVlanIf for PORT-type interfaces The getInterfaceInVlanIf() method was calling intf->getVlanID() which has a CHECK that fails when the interface type is PORT or SYSTEM_PORT. This was causing FBOSS agent crashes during IPv6 packet handling, specifically when resolveDestAndHandlePacket() tried to look up interfaces by VLAN ID. The fix is to use getVlanIDHelper() instead, which properly handles all interface types: - For VLAN type: returns the actual VLAN ID - For PORT/SYSTEM_PORT types: returns VlanID(0) This allows the code to work correctly with both traditional VLAN-based routing and modern PORT-based routing configurations. Root cause identified from core dump analysis showing crash at: InterfaceMap::getInterfaceInVlanIf -> Interface::getVlanID() CHECK failure when processing IPv6 packets on PORT-type interfaces. Tested on gold405 with PORT-type interface configuration (eth1/25/1). Refactor static route commands to use 'config protocol static' hierarchy - Move static route commands from 'config ip/ipv6 route' to 'config protocol static ip/ipv6 route' - New command syntax: * config protocol static ip route add <prefix> <nexthop|null0|cpu> * config protocol static ipv6 route add <prefix> <nexthop|null0|cpu> * delete config protocol static ip route <prefix> * delete config protocol static ipv6 route <prefix> - Remove --vrf option (defaults to VRF 0) - Remove interface parameter for nexthops (not supported in thrift structure) - Add support for null0 (drop) and cpu as nexthop destinations - Implement template-based code sharing between IPv4 and IPv6 commands - Add proper IPv4/IPv6 validation with helpful error messages - Create parent command classes for new hierarchy: * CmdConfigProtocolStatic * CmdDeleteConfigProtocol * CmdDeleteConfigProtocolStatic - Update build files (CliFboss2.cmake, BUCK) - Update command registration (CmdListConfig.cpp, CmdSubcommands.cpp) Tested on gold221 with successful route additions for IPv4 and IPv6. Fix delete command argument parsing by completing ParentCmd chain Key fixes: 1. Added ParentCmd = CmdDeleteConfig to CmdDeleteConfigProtocolTraits to complete the parent chain for argument resolution 2. Increased MAX_DEPTH from 5 to 6 in CmdArgsLists to support deeper command nesting 3. Created intermediate handler classes CmdDeleteConfigProtocolStaticIp/Ipv6 (though not strictly required, kept for consistency with old structure) 4. Flattened config command structure: removed 'add' level, making 'route' the direct leaf command The root cause was a mismatch between: - Command depth (where CLI11 registers arguments in CmdArgsLists array) - ParentCmd chain (how CmdHandler resolves arguments via template recursion) The 'delete' verb adds an extra 'config' level compared to other verbs, requiring the full parent chain to be explicitly defined. Tested on gold221: - config protocol static ip route <prefix> <nexthop|null0|cpu> ✓ - config protocol static ipv6 route <prefix> <nexthop|null0|cpu> ✓ - delete config protocol static ip route <prefix> ✓ - delete config protocol static ipv6 route <prefix> ✓ Remove 'config' keyword from delete commands Restructure delete commands to remove the redundant 'config' level: - delete config interface → delete interface - delete config protocol static → delete protocol static Changes: 1. Renamed all CmdDeleteConfig* classes to CmdDelete* 2. Moved directory structure from delete/config/* to delete/* 3. Updated ParentCmd chains to remove CmdDeleteConfig dependency 4. Updated all includes, cmake, and BUCK files 5. Updated CmdListConfig.cpp command tree structure Benefits: - Shorter, cleaner commands - Consistent with industry standard CLI patterns - Simpler depth management (one less nesting level) - Cleaner namespace without redundant 'config' in delete context Files affected: - Renamed/moved: 14 source/header files - Updated: cmake/CliFboss2.cmake, fboss/cli/fboss2/BUCK - Updated: CmdListConfig.cpp command tree registration Tested on gold221: - delete interface <interface> <ip-address|ipv6-address> <value> ✓ - delete protocol static ip route <prefix> ✓ - delete protocol static ipv6 route <prefix> ✓
benoit-nexthop
force-pushed
the
manoharan.ip-route-cmds
branch
from
6acf2b4 to
d3ddaeb
Compare
|
@manoharan-nexthop has updated the pull request. You must reimport the pull request before landing. |
|
@joseph5wu merged this pull request in 812c688. |
3 participants
Pre-submission checklist
pip install -r requirements-dev.txt && pre-commit installpre-commit runSummary
Add/remove IP addresses on interfaces:
Add/remove static routes:
Test Plan