diff --git a/cmake/CliFboss2.cmake b/cmake/CliFboss2.cmake index f081e383cbbda..629311b40f45b 100644 --- a/cmake/CliFboss2.cmake +++ b/cmake/CliFboss2.cmake @@ -836,6 +836,8 @@ add_library(fboss2_config_lib fboss/cli/fboss2/commands/config/interface/CmdConfigInterface.h fboss/cli/fboss2/commands/config/interface/InterfaceAttrArgsBase.h fboss/cli/fboss2/commands/config/interface/InterfaceIpUtils.h + fboss/cli/fboss2/commands/config/interface/InterfaceManager.cpp + fboss/cli/fboss2/commands/config/interface/InterfaceManager.h fboss/cli/fboss2/commands/config/interface/ProfileValidation.cpp fboss/cli/fboss2/commands/config/interface/ProfileValidation.h fboss/cli/fboss2/commands/config/interface/ipv6/CmdConfigInterfaceIpv6.cpp diff --git a/fboss/cli/fboss2/BUCK b/fboss/cli/fboss2/BUCK index 7d0a8d82a186a..4101d980625d6 100644 --- a/fboss/cli/fboss2/BUCK +++ b/fboss/cli/fboss2/BUCK @@ -1087,6 +1087,7 @@ cpp_library( "commands/config/dhcp/reply_source_override/CmdConfigDhcpReplySourceOverride.cpp", "commands/config/history/CmdConfigHistory.cpp", "commands/config/interface/CmdConfigInterface.cpp", + "commands/config/interface/InterfaceManager.cpp", "commands/config/interface/ProfileValidation.cpp", "commands/config/interface/ipv6/CmdConfigInterfaceIpv6.cpp", "commands/config/interface/ipv6/ndp/CmdConfigInterfaceIpv6Ndp.cpp", @@ -1244,6 +1245,7 @@ cpp_library( "commands/config/interface/CmdConfigInterface.h", "commands/config/interface/InterfaceAttrArgsBase.h", "commands/config/interface/InterfaceIpUtils.h", + "commands/config/interface/InterfaceManager.h", "commands/config/interface/ProfileValidation.h", "commands/config/interface/ipv6/CmdConfigInterfaceIpv6.h", "commands/config/interface/ipv6/ndp/CmdConfigInterfaceIpv6Ndp.h", diff --git a/fboss/cli/fboss2/commands/config/interface/InterfaceManager.cpp b/fboss/cli/fboss2/commands/config/interface/InterfaceManager.cpp new file mode 100644 index 0000000000000..e7e1359c91021 --- /dev/null +++ b/fboss/cli/fboss2/commands/config/interface/InterfaceManager.cpp @@ -0,0 +1,193 @@ +/* + * Copyright (c) 2004-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#include "fboss/cli/fboss2/commands/config/interface/InterfaceManager.h" + +#include +#include +#include +#include +#include +#include +#include "fboss/agent/FbossError.h" +#include "fboss/agent/gen-cpp2/switch_config_types.h" +#include "fboss/agent/types.h" + +namespace facebook::fboss { + +namespace { + +// Port name for error messages, falling back to the logical ID for the +// unnamed ports some configs carry. +std::string portLabel(const cfg::Port& port) { + return port.name().has_value() ? *port.name() + : std::to_string(*port.logicalID()); +} + +// Ids of the tunnels using `intfId` as their underlay interface. +std::vector tunnelsOnUnderlayIntf( + const cfg::SwitchConfig& swConfig, + int32_t intfId) { + std::vector ids; + if (swConfig.ipInIpTunnels().has_value()) { + for (const auto& tunnel : *swConfig.ipInIpTunnels()) { + if (*tunnel.underlayIntfID() == intfId) { + ids.push_back(*tunnel.ipInIpTunnelId()); + } + } + } + if (swConfig.srv6Tunnels().has_value()) { + for (const auto& tunnel : *swConfig.srv6Tunnels()) { + if (*tunnel.underlayIntfID() == intfId) { + ids.push_back(*tunnel.srv6TunnelId()); + } + } + } + return ids; +} + +// Names of the enabled ports that are members of `vlanId`. Membership comes +// from vlanPorts, matching how ThriftConfigApplier builds its port -> vlan map. +std::vector enabledMemberPorts( + const cfg::SwitchConfig& swConfig, + int32_t vlanId, + const std::set& portsBeingDeleted) { + std::set memberPorts; + for (const auto& vlanPort : *swConfig.vlanPorts()) { + if (*vlanPort.vlanID() == vlanId) { + memberPorts.insert(*vlanPort.logicalPort()); + } + } + + std::vector names; + for (const auto& port : *swConfig.ports()) { + // A port that is itself being deleted in the same command does not keep + // the VLAN alive, so it must not block the interface delete. + if (portsBeingDeleted.count(PortID(*port.logicalID())) > 0) { + continue; + } + if (memberPorts.count(*port.logicalID()) > 0 && + *port.state() == cfg::PortState::ENABLED) { + names.push_back(portLabel(port)); + } + } + return names; +} + +// True when a VLAN interface other than those in `goingAway` still covers +// `vlanId`, so the VLAN keeps an interface after the delete. +bool vlanKeepsAnInterface( + const cfg::SwitchConfig& swConfig, + int32_t vlanId, + const std::set& goingAway) { + return std::any_of( + swConfig.interfaces()->cbegin(), + swConfig.interfaces()->cend(), + [vlanId, &goingAway](const cfg::Interface& intf) { + return *intf.type() == cfg::InterfaceType::VLAN && + *intf.vlanID() == vlanId && + goingAway.count(InterfaceID(*intf.intfID())) == 0; + }); +} + +// Throws if removing `intf` — as part of removing all of `goingAway` — would +// dangle a reference or produce a config the agent cannot apply. +void checkDeletable( + const cfg::SwitchConfig& swConfig, + const cfg::Interface& intf, + const std::set& goingAway, + const std::set& portsBeingDeleted) { + const auto id = *intf.intfID(); + + if (*intf.type() == cfg::InterfaceType::PORT) { + throw FbossError( + "Cannot delete interface ", + id, + ": it is the port router interface for port ", + intf.portID().has_value() ? std::to_string(*intf.portID()) : "", + ". Deleting it would leave that port without an interface, which the " + "agent cannot run with. Delete the port itself instead."); + } + + auto tunnels = tunnelsOnUnderlayIntf(swConfig, id); + if (!tunnels.empty()) { + throw FbossError( + "Cannot delete interface ", + id, + ": it is the underlay interface for tunnel(s): ", + folly::join(", ", tunnels), + ". Delete the tunnel(s) first."); + } + + if (*intf.type() != cfg::InterfaceType::VLAN) { + return; + } + const auto vlanId = *intf.vlanID(); + if (vlanKeepsAnInterface(swConfig, vlanId, goingAway)) { + return; + } + auto enabledPorts = enabledMemberPorts(swConfig, vlanId, portsBeingDeleted); + if (!enabledPorts.empty()) { + throw FbossError( + "Cannot delete interface ", + id, + ": it is the only interface for VLAN ", + vlanId, + ", which still has enabled member port(s): ", + folly::join(", ", enabledPorts), + ". Disable or unbind those ports, or delete the whole VLAN with " + "'delete vlan ", + vlanId, + "'."); + } +} + +} // namespace + +void InterfaceManager::deleteInterfaces( + cfg::SwitchConfig& swConfig, + const std::set& intfIds, + const std::set& portsBeingDeleted) { + auto& interfaces = *swConfig.interfaces(); + + // Check everything before touching anything, so a refusal anywhere in the + // set leaves the config exactly as it was. + for (const auto& intfId : intfIds) { + const auto id = static_cast(intfId); + auto it = std::find_if( + interfaces.cbegin(), interfaces.cend(), [id](const cfg::Interface& i) { + return *i.intfID() == id; + }); + if (it == interfaces.cend()) { + throw FbossError("Interface ", id, " does not exist"); + } + checkDeletable(swConfig, *it, intfIds, portsBeingDeleted); + } + + // Safe to remove. A VLAN's intfID is a back-pointer carrying no + // configuration of its own, so it is cleared rather than refused. + for (auto& vlan : *swConfig.vlans()) { + if (vlan.intfID().has_value() && + intfIds.count(InterfaceID(*vlan.intfID())) > 0) { + vlan.intfID().reset(); + } + } + + interfaces.erase( + std::remove_if( + interfaces.begin(), + interfaces.end(), + [&intfIds](const cfg::Interface& intf) { + return intfIds.count(InterfaceID(*intf.intfID())) > 0; + }), + interfaces.end()); +} + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/interface/InterfaceManager.h b/fboss/cli/fboss2/commands/config/interface/InterfaceManager.h new file mode 100644 index 0000000000000..fb6cac67b85dd --- /dev/null +++ b/fboss/cli/fboss2/commands/config/interface/InterfaceManager.h @@ -0,0 +1,73 @@ +/* + * Copyright (c) 2004-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#pragma once + +#include + +#include "fboss/agent/gen-cpp2/switch_config_types.h" +#include "fboss/agent/types.h" + +namespace facebook::fboss { + +/** + * InterfaceManager provides utilities for managing L3 router interfaces + * (SwitchConfig.interfaces) that are not tied to a single port, such as VLAN + * SVIs and virtual/loopback interfaces. + * + * Port deletion prunes the interfaces it owns via + * utility::removePortsFromConfig; this class covers the interfaces that + * outlive any one port. + */ +class InterfaceManager { + public: + // Removes the interfaces with the given IDs from swConfig, along with the + // VLAN intfID back-pointers naming them. + // + // Every ID is checked before any of them is removed, so a refused delete + // leaves the config untouched rather than partially applied. Checking the + // set as a whole also means two interfaces sharing a VLAN can be deleted + // together: neither counts as the other's surviving cover. + // + // Refuses (throws FbossError) rather than leaving a dangling reference or a + // config the agent will reject — or crash on — at apply time: + // - the interface is a port router interface (InterfaceType::PORT). + // Deleting it leaves its port with an empty interface list, and + // Port::getInterfaceID() CHECK-fails on that, taking the agent down on + // the first packet routed via the port + // -> delete the port instead: delete interface + // - an ip-in-ip or SRv6 tunnel uses it as its underlay interface + // (Tunnel.underlayIntfID is a required field, so there is nothing to + // clear) + // -> delete the tunnel first: delete tunnel + // - it is the last VLAN-type interface for its VLAN and that VLAN still + // has an enabled member port; ThriftConfigApplier rejects such a config + // with "VLAN has no interface, even when corresp port is + // enabled" + // -> disable or unbind the member ports, or drop the whole VLAN with + // delete vlan + // Ports listed in portsBeingDeleted are excluded from this check: a + // single 'delete interface ' removes the port too, + // so the VLAN is not left with a live port and no interface. + // Throws FbossError if any of the given interfaces does not exist. + // + // An ACL redirect-nexthop naming one of these interfaces + // (RedirectNextHop.intfID) is deliberately not a refusal: the field is + // optional and the agent does not resolve it against the interface list, it + // just disables the ACL when no nexthop resolves. + // + // Does NOT call saveConfig() — callers save after this returns. + static void deleteInterfaces( + cfg::SwitchConfig& swConfig, + const std::set& intfIds, + const std::set& portsBeingDeleted = {}); +}; + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/delete/interface/CmdDeleteInterface.cpp b/fboss/cli/fboss2/commands/delete/interface/CmdDeleteInterface.cpp index 6e2e1d7391637..4880e807fb1c9 100644 --- a/fboss/cli/fboss2/commands/delete/interface/CmdDeleteInterface.cpp +++ b/fboss/cli/fboss2/commands/delete/interface/CmdDeleteInterface.cpp @@ -23,7 +23,9 @@ #include #include "fboss/agent/gen-cpp2/switch_config_types.h" +#include "fboss/agent/types.h" #include "fboss/cli/fboss2/commands/config/interface/InterfaceIpUtils.h" +#include "fboss/cli/fboss2/commands/config/interface/InterfaceManager.h" #include "fboss/cli/fboss2/session/ConfigSession.h" #include "fboss/cli/fboss2/utils/InterfaceList.h" #include "fboss/lib/config/AgentConfigUtils.h" @@ -89,7 +91,10 @@ InterfaceDeleteConfig::InterfaceDeleteConfig(const std::vector& v) } } - // Resolve port names to InterfaceList (throws if any port is not found). + // Resolve names to InterfaceList (throws if any name is not found). + // InterfaceList resolves a bare number as a port logical ID or an interface + // ID, so a whole-interface delete can name the interfaces that generated + // configs leave unnamed. interfaces_ = utils::InterfaceList(std::move(portNames)); } @@ -103,31 +108,50 @@ CmdDeleteInterfaceTraits::RetType CmdDeleteInterface::queryClient( throw std::invalid_argument("No interface name provided"); } - // No attributes => delete the whole port(s) from the config. + // No attributes => delete the whole port(s) / interface(s) from the config. if (attributes.empty()) { auto& swConfig = *ConfigSession::getInstance().getAgentConfig().sw(); std::set portsToDelete; + std::set interfacesToDelete; std::vector deletedNames; for (const utils::Intf& intf : interfaces) { - const cfg::Port* port = intf.getPort(); - if (!port) { + if (const cfg::Port* port = intf.getPort()) { + portsToDelete.insert(PortID(*port->logicalID())); + } else if (const cfg::Interface* iface = intf.getInterface()) { + // A name that resolves to an interface but no port is a portless L3 + // interface (VLAN SVI, loopback), so the interface itself is what gets + // removed. The interfaces a port owns are pruned by + // removePortsFromConfig below instead. + interfacesToDelete.insert(InterfaceID(*iface->intfID())); + } else { continue; } - portsToDelete.insert(PortID(*port->logicalID())); deletedNames.push_back(intf.name()); } - if (portsToDelete.empty()) { + if (portsToDelete.empty() && interfacesToDelete.empty()) { throw std::invalid_argument( - "No port found for the specified interface(s)"); + "No port or interface found for the specified name(s)"); + } + // Interfaces first: deleteInterfaces() refuses a delete that would dangle + // a reference or produce a config the agent rejects, and running those + // checks before removePortsFromConfig keeps a refusal from leaving the + // session half-mutated. portsToDelete is passed so a port removed in the + // same command does not count as keeping its VLAN's interface alive. + if (!interfacesToDelete.empty()) { + InterfaceManager::deleteInterfaces( + swConfig, interfacesToDelete, portsToDelete); + } + if (!portsToDelete.empty()) { + utility::removePortsFromConfig( + swConfig, + portsToDelete, + utility::PortRemovalMode::Erase, + /*pruneEmptyVlansAndInterfaces=*/true); } - utility::removePortsFromConfig( - swConfig, - portsToDelete, - utility::PortRemovalMode::Erase, - /*pruneEmptyVlansAndInterfaces=*/true); - // Removing a port is a HITLESS change: the agent's reloadConfig() applies - // the port-set delta live, matching how 'config interface profile' - // adds/removes ports. No agent warmboot is needed. + // Removing a port or an L3 interface is a HITLESS change: the agent's + // reloadConfig() applies the delta live, matching how 'config interface + // profile' adds/removes ports and how 'delete vlan' drops a VLAN's + // interfaces. No agent warmboot is needed. ConfigSession::getInstance().saveConfig(); return fmt::format( "Deleted interface(s): {}", folly::join(", ", deletedNames)); diff --git a/fboss/cli/fboss2/commands/delete/interface/CmdDeleteInterface.h b/fboss/cli/fboss2/commands/delete/interface/CmdDeleteInterface.h index 9ae405f94f762..e14c50805243f 100644 --- a/fboss/cli/fboss2/commands/delete/interface/CmdDeleteInterface.h +++ b/fboss/cli/fboss2/commands/delete/interface/CmdDeleteInterface.h @@ -22,7 +22,13 @@ namespace facebook::fboss { * delete (reset to default or remove by value) for the `delete interface` * command. * - * Usage: delete interface [ [] ...] + * Usage: delete interface [ [] ...] + * + * Each entry of is a port name (eth1/1/1), an interface name, or + * a bare interface ID — generated configs leave Interface.name unset, so the + * ID is the only handle for those. With no attribute the whole object is + * deleted: a port name deletes the port and its dependents, an interface + * deletes the L3 interface itself. * * Valueless attributes (reset to default): * loopback-mode, lookup-class, lldp-expected-value, lldp-expected-chassis, @@ -45,7 +51,7 @@ struct CmdDeleteInterfaceTraits : public WriteCommandTraits { cmd.add_option( "interface_delete_config", args, - " [loopback-mode|lookup-class|lldp-expected-*|ip-address |ipv6-address ]"); + "... [loopback-mode|lookup-class|lldp-expected-*|ip-address |ipv6-address ]"); } using ObjectArgType = InterfaceDeleteConfig; using RetType = std::string; diff --git a/fboss/cli/fboss2/test/InterfaceListTest.cpp b/fboss/cli/fboss2/test/InterfaceListTest.cpp index 8a20deb29cb9a..384666bfbc04b 100644 --- a/fboss/cli/fboss2/test/InterfaceListTest.cpp +++ b/fboss/cli/fboss2/test/InterfaceListTest.cpp @@ -53,6 +53,13 @@ class InterfaceListTest : public ::testing::Test { "state": 2, "speed": 100000 } + ], + "interfaces": [ + { + "intfID": 2001, + "name": "uplinks_1", + "vlanID": 0 + } ] } })"; @@ -102,4 +109,41 @@ TEST_F(InterfaceListTest, AllowMissingResolvesKnownPort) { EXPECT_NE(list[0].getPort(), nullptr); } +// A numeric name matching a port logical ID resolves to that port. +TEST_F(InterfaceListTest, ResolvesPortLogicalId) { + InterfaceList list({"1"}); + + ASSERT_EQ(list.size(), 1); + const auto& intf = list[0]; + ASSERT_NE(intf.getPort(), nullptr); + EXPECT_EQ(*intf.getPort()->name(), "eth1/1/1"); + EXPECT_EQ(intf.name(), "1"); +} + +// A numeric name matching an interface ID resolves to that interface. +TEST_F(InterfaceListTest, ResolvesInterfaceId) { + InterfaceList list({"2001"}); + + ASSERT_EQ(list.size(), 1); + const auto& intf = list[0]; + EXPECT_EQ(intf.getPort(), nullptr); + ASSERT_NE(intf.getInterface(), nullptr); + EXPECT_EQ(*intf.getInterface()->intfID(), 2001); +} + +// An interface is still resolvable by name. +TEST_F(InterfaceListTest, ResolvesInterfaceName) { + InterfaceList list({"uplinks_1"}); + + ASSERT_EQ(list.size(), 1); + ASSERT_NE(list[0].getInterface(), nullptr); + EXPECT_EQ(*list[0].getInterface()->intfID(), 2001); +} + +// A numeric name matching neither a port logical ID nor an interface ID +// throws. +TEST_F(InterfaceListTest, ThrowsForUnknownId) { + EXPECT_THROW(InterfaceList({"4242"}), std::invalid_argument); +} + } // namespace facebook::fboss::utils diff --git a/fboss/cli/fboss2/test/config/CmdDeleteConfigInterfaceTest.cpp b/fboss/cli/fboss2/test/config/CmdDeleteConfigInterfaceTest.cpp index 5f5d61639a009..aaa2b670058c2 100644 --- a/fboss/cli/fboss2/test/config/CmdDeleteConfigInterfaceTest.cpp +++ b/fboss/cli/fboss2/test/config/CmdDeleteConfigInterfaceTest.cpp @@ -3,7 +3,9 @@ #include #include #include +#include +#include "fboss/agent/FbossError.h" #include "fboss/cli/fboss2/commands/delete/interface/CmdDeleteInterface.h" #include "fboss/cli/fboss2/session/ConfigSession.h" #include "fboss/cli/fboss2/test/config/CmdConfigTestBase.h" @@ -363,4 +365,323 @@ TEST_F(DeleteQueueConfigAttrTestFixture, unboundPortIsNoOp) { EXPECT_FALSE(findPort("eth1/2/1")->portQueueConfigName().has_value()); } +// ============================================================================ +// Whole-interface delete (bare `delete interface `) +// ============================================================================ + +// Seed mirrors the shape a generated agent.conf has: interfaces carry no name +// (so the interface ID is the only handle), a virtual loopback interface sits +// on a member-less VLAN, and port router interfaces bind to a port by portID. +// Numeric enum values are cfg::PortState (1 DISABLED, 2 ENABLED) and +// cfg::InterfaceType (1 VLAN, 3 PORT). +// +// Interfaces, and what each is here to exercise: +// 10 virtual loopback, VLAN 10 has no member port -> deletable +// 100 VLAN 100 has an enabled member port -> refused (VLAN) +// 200 VLAN 200's only member port is disabled -> deletable +// 300 shares VLAN 300 with 301, which has an enabled -> deletable alone, +// 301 member port refused together +// 400 named by an ip-in-ip tunnel's underlayIntfID -> refused (tunnel) +// 500 named by an SRv6 tunnel AND on a VLAN with an -> refused (tunnel; +// enabled member port proves ordering) +// 2001 port router interface for port 1 -> refused (PORT) +// Shared helpers for the whole-object delete fixtures below. They differ only +// in their seed config, so the config-session accessor, existence checks, and +// the command runner live here rather than being repeated per fixture. +class DeleteInterfaceCmdTestBase : public CmdConfigTestBase { + public: + DeleteInterfaceCmdTestBase(const std::string& name, const std::string& config) + : CmdConfigTestBase(name, config) {} + + protected: + const std::string cmdPrefix_ = "delete interface"; + + static cfg::SwitchConfig& swConfig() { + return *ConfigSession::getInstance().getAgentConfig().sw(); + } + + static bool hasInterface(int32_t intfId) { + const auto& interfaces = *swConfig().interfaces(); + return std::any_of( + interfaces.begin(), interfaces.end(), [intfId](const auto& intf) { + return *intf.intfID() == intfId; + }); + } + + static bool hasPort(int32_t logicalId) { + const auto& ports = *swConfig().ports(); + return std::any_of(ports.begin(), ports.end(), [logicalId](const auto& p) { + return *p.logicalID() == logicalId; + }); + } + + // Runs `delete interface ` through the real arg type and handler. + std::string runDelete(const std::vector& names) { + auto cmd = CmdDeleteInterface(); + return cmd.queryClient(localhost(), InterfaceDeleteConfig(names)); + } +}; + +class CmdDeleteWholeL3InterfaceTestFixture : public DeleteInterfaceCmdTestBase { + public: + CmdDeleteWholeL3InterfaceTestFixture() + : DeleteInterfaceCmdTestBase( + "delete_l3_interface_test_%%%%-%%%%-%%%%", + R"({ + "sw": { + "ports": [ + {"logicalID": 1, "name": "eth1/1/1", "state": 2, "speed": 100000, "ingressVlan": 100}, + {"logicalID": 2, "name": "eth1/2/1", "state": 1, "speed": 100000, "ingressVlan": 200}, + {"logicalID": 3, "name": "eth1/3/1", "state": 2, "speed": 100000, "ingressVlan": 300}, + {"logicalID": 5, "name": "eth1/5/1", "state": 2, "speed": 100000, "ingressVlan": 500} + ], + "vlanPorts": [ + {"vlanID": 100, "logicalPort": 1, "spanningTreeState": 2, "emitTags": false}, + {"vlanID": 200, "logicalPort": 2, "spanningTreeState": 2, "emitTags": false}, + {"vlanID": 300, "logicalPort": 3, "spanningTreeState": 2, "emitTags": false}, + {"vlanID": 500, "logicalPort": 5, "spanningTreeState": 2, "emitTags": false} + ], + "defaultVlan": 4094, + "vlans": [ + {"id": 10, "name": "fbossLoopback0", "routable": true, "intfID": 10}, + {"id": 100, "name": "vlan100", "routable": true, "intfID": 100}, + {"id": 200, "name": "vlan200", "routable": true, "intfID": 200}, + {"id": 300, "name": "vlan300", "routable": true, "intfID": 300}, + {"id": 400, "name": "vlan400", "routable": true, "intfID": 400}, + {"id": 500, "name": "vlan500", "routable": true, "intfID": 500}, + {"id": 4094, "name": "default", "routable": false} + ], + "interfaces": [ + {"intfID": 10, "vlanID": 10, "routerID": 0, "type": 1, "isVirtual": true, + "ipAddresses": ["10.0.0.1/32"]}, + {"intfID": 100, "vlanID": 100, "routerID": 0, "type": 1, "mtu": 9412}, + {"intfID": 200, "vlanID": 200, "routerID": 0, "type": 1, "mtu": 9412}, + {"intfID": 300, "vlanID": 300, "routerID": 0, "type": 1, "mtu": 9412}, + {"intfID": 301, "vlanID": 300, "routerID": 0, "type": 1, "mtu": 9412}, + {"intfID": 400, "vlanID": 400, "routerID": 0, "type": 1, "mtu": 9412}, + {"intfID": 500, "vlanID": 500, "routerID": 0, "type": 1, "mtu": 9412}, + {"intfID": 2001, "vlanID": 0, "routerID": 0, "type": 3, "portID": 1} + ], + "ipInIpTunnels": [ + {"ipInIpTunnelId": "tunnel0", "underlayIntfID": 400, "dstIp": "2401::1"} + ], + "srv6Tunnels": [ + {"srv6TunnelId": "srv6tunnel0", "underlayIntfID": 500, "tunnelType": 3} + ] + } +})") {} +}; + +// A bare interface ID resolves even though the interface has no name, and the +// virtual loopback interface deletes: its VLAN has no member ports at all. +TEST_F(CmdDeleteWholeL3InterfaceTestFixture, deletesLoopbackInterfaceById) { + setupTestableConfigSession(cmdPrefix_, "10"); + + auto result = runDelete({"10"}); + + EXPECT_THAT(result, HasSubstr("Deleted interface(s)")); + EXPECT_THAT(result, HasSubstr("10")); + EXPECT_FALSE(hasInterface(10)); + + // The VLAN itself stays — dropping a VLAN is `delete vlan` — but its + // back-pointer to the deleted interface is cleared. + const auto& vlans = *swConfig().vlans(); + auto vlan = std::find_if( + vlans.begin(), vlans.end(), [](const auto& v) { return *v.id() == 10; }); + ASSERT_NE(vlan, vlans.end()); + EXPECT_FALSE(vlan->intfID().has_value()); +} + +// The only interface for a VLAN that still has an enabled member port is +// refused: ThriftConfigApplier would reject the resulting config. +TEST_F(CmdDeleteWholeL3InterfaceTestFixture, refusesWhenVlanHasEnabledPort) { + setupTestableConfigSession(cmdPrefix_, "100"); + + try { + runDelete({"100"}); + FAIL() << "expected the delete to be refused"; + } catch (const FbossError& e) { + EXPECT_THAT(std::string(e.what()), HasSubstr("enabled member port")); + EXPECT_THAT(std::string(e.what()), HasSubstr("eth1/1/1")); + } + EXPECT_TRUE(hasInterface(100)); +} + +// Same VLAN shape but the member port is disabled, so the agent tolerates the +// interface-less VLAN and the delete goes through. +TEST_F(CmdDeleteWholeL3InterfaceTestFixture, deletesWhenVlanPortIsDisabled) { + setupTestableConfigSession(cmdPrefix_, "200"); + + EXPECT_THAT(runDelete({"200"}), HasSubstr("Deleted interface(s)")); + EXPECT_FALSE(hasInterface(200)); +} + +// A sibling interface on the same VLAN keeps the VLAN covered, so removing +// just one of them is fine even though the VLAN has an enabled member port. +TEST_F(CmdDeleteWholeL3InterfaceTestFixture, deletesOneOfTwoInterfacesOnVlan) { + setupTestableConfigSession(cmdPrefix_, "300"); + + EXPECT_THAT(runDelete({"300"}), HasSubstr("Deleted interface(s)")); + EXPECT_FALSE(hasInterface(300)); + EXPECT_TRUE(hasInterface(301)); +} + +// Deleting both interfaces on that VLAN in one command is refused: neither +// counts as the other's surviving cover, so the VLAN would be left bare. +TEST_F( + CmdDeleteWholeL3InterfaceTestFixture, + refusesDeletingBothVlanInterfaces) { + setupTestableConfigSession(cmdPrefix_, "300 301"); + + EXPECT_THROW(runDelete({"300", "301"}), FbossError); + EXPECT_TRUE(hasInterface(300)); + EXPECT_TRUE(hasInterface(301)); +} + +// An ip-in-ip tunnel's underlayIntfID is a required field, so the interface it +// names cannot be deleted out from under it. +TEST_F(CmdDeleteWholeL3InterfaceTestFixture, refusesWhenIpInIpTunnelUsesIntf) { + setupTestableConfigSession(cmdPrefix_, "400"); + + try { + runDelete({"400"}); + FAIL() << "expected the delete to be refused"; + } catch (const FbossError& e) { + EXPECT_THAT(std::string(e.what()), HasSubstr("underlay interface")); + EXPECT_THAT(std::string(e.what()), HasSubstr("tunnel0")); + } + EXPECT_TRUE(hasInterface(400)); +} + +// Interface 500 trips both the tunnel check and the enabled-member-port check. +// The tunnel is reported, which pins the order the checks run in: reordering +// them fails this test. +TEST_F(CmdDeleteWholeL3InterfaceTestFixture, tunnelRefusalPreemptsVlanRefusal) { + setupTestableConfigSession(cmdPrefix_, "500"); + + try { + runDelete({"500"}); + FAIL() << "expected the delete to be refused"; + } catch (const FbossError& e) { + EXPECT_THAT(std::string(e.what()), HasSubstr("srv6tunnel0")); + EXPECT_THAT(std::string(e.what()), Not(HasSubstr("enabled member port"))); + } + EXPECT_TRUE(hasInterface(500)); +} + +// Deleting a port router interface would leave its port with an empty +// interface list, which CHECK-fails in Port::getInterfaceID() and takes the +// agent down, so it is refused. +TEST_F(CmdDeleteWholeL3InterfaceTestFixture, refusesPortRouterInterface) { + setupTestableConfigSession(cmdPrefix_, "2001"); + + try { + runDelete({"2001"}); + FAIL() << "expected the delete to be refused"; + } catch (const FbossError& e) { + EXPECT_THAT(std::string(e.what()), HasSubstr("port router interface")); + } + EXPECT_TRUE(hasInterface(2001)); +} + +// A refusal anywhere in a multi-interface delete leaves every interface in +// place, rather than applying the ones checked before the failure. +TEST_F(CmdDeleteWholeL3InterfaceTestFixture, refusalLeavesConfigUntouched) { + setupTestableConfigSession(cmdPrefix_, "10 100"); + + EXPECT_THROW(runDelete({"10", "100"}), FbossError); + EXPECT_TRUE(hasInterface(10)) << "deletable interface must not be applied"; + EXPECT_TRUE(hasInterface(100)); +} + +// An ID matching no interface is rejected at argument-resolution time, the +// same way an unknown port name is. +TEST_F(CmdDeleteWholeL3InterfaceTestFixture, unknownInterfaceIdThrows) { + setupTestableConfigSession(); + EXPECT_THROW(InterfaceDeleteConfig({"9999"}), std::invalid_argument); +} + +// A port name still resolves to its port, not to the port router interface +// bound to it, so the existing whole-port behaviour is unchanged. The port +// router interface is deliberately left in place by removePortsFromConfig. +TEST_F(CmdDeleteWholeL3InterfaceTestFixture, portNameStillDeletesThePort) { + setupTestableConfigSession(cmdPrefix_, "eth1/1/1"); + + EXPECT_THAT(runDelete({"eth1/1/1"}), HasSubstr("Deleted interface(s)")); + + const auto& ports = *swConfig().ports(); + EXPECT_TRUE(std::none_of(ports.begin(), ports.end(), [](const auto& p) { + return *p.logicalID() == 1; + })); + EXPECT_TRUE(hasInterface(2001)) + << "removePortsFromConfig leaves port router interfaces in place"; +} + +// Naming an SVI and its VLAN's only member port in one command deletes both. +// The port is removed in the same command, so the VLAN is not left with a live +// port and no interface, and the delete is accepted -- unlike +// refusesWhenVlanHasEnabledPort, which names the interface alone. Exercises the +// interfaces-before-ports ordering with both sets non-empty. +TEST_F( + CmdDeleteWholeL3InterfaceTestFixture, + deletesInterfaceAndItsPortTogether) { + setupTestableConfigSession(cmdPrefix_, "100 eth1/1/1"); + + EXPECT_THAT( + runDelete({"100", "eth1/1/1"}), HasSubstr("Deleted interface(s)")); + + EXPECT_FALSE(hasInterface(100)) << "the SVI is deleted"; + EXPECT_FALSE(hasPort(1)) << "its member port is deleted in the same command"; +} + +// A bare number that is simultaneously a port logical ID and an interface ID +// resolves to the port: InterfaceList tries the port logical ID before the +// interface ID. The resolver's own tests exercise each lookup in isolation but +// never the collision, so pin it here — it decides what `delete interface ` +// actually touches when both exist. +class CmdDeleteInterfaceIdCollisionTestFixture + : public DeleteInterfaceCmdTestBase { + public: + CmdDeleteInterfaceIdCollisionTestFixture() + : DeleteInterfaceCmdTestBase( + "delete_intf_id_collision_test_%%%%-%%%%-%%%%", + R"({ + "sw": { + "ports": [ + {"logicalID": 3, "name": "eth1/3/1", "state": 2, "speed": 100000, "ingressVlan": 300}, + {"logicalID": 300, "name": "eth1/9/1", "state": 2, "speed": 100000, "ingressVlan": 100} + ], + "vlanPorts": [ + {"vlanID": 300, "logicalPort": 3, "spanningTreeState": 2, "emitTags": false}, + {"vlanID": 100, "logicalPort": 300, "spanningTreeState": 2, "emitTags": false} + ], + "defaultVlan": 4094, + "vlans": [ + {"id": 100, "name": "vlan100", "routable": true, "intfID": 100}, + {"id": 300, "name": "vlan300", "routable": true, "intfID": 300}, + {"id": 4094, "name": "default", "routable": false} + ], + "interfaces": [ + {"intfID": 100, "vlanID": 100, "routerID": 0, "type": 1, "mtu": 9412}, + {"intfID": 300, "vlanID": 300, "routerID": 0, "type": 1, "mtu": 9412} + ] + } +})") {} +}; + +// intfID 300 and port logicalID 300 both exist. "300" resolves to the port, so +// the port is deleted and the same-numbered interface (on a VLAN that still has +// an enabled member port) is left untouched. +TEST_F( + CmdDeleteInterfaceIdCollisionTestFixture, + numericArgPrefersPortOverInterface) { + setupTestableConfigSession(cmdPrefix_, "300"); + + EXPECT_THAT(runDelete({"300"}), HasSubstr("Deleted interface(s)")); + + EXPECT_FALSE(hasPort(300)); + EXPECT_TRUE(hasInterface(300)) + << "the number resolved to the port, so interface 300 must survive"; +} + } // namespace facebook::fboss diff --git a/fboss/cli/fboss2/test/integration_test/DeleteInterfaceTest.cpp b/fboss/cli/fboss2/test/integration_test/DeleteInterfaceTest.cpp index 9e10d60f8713f..96730c02d519a 100644 --- a/fboss/cli/fboss2/test/integration_test/DeleteInterfaceTest.cpp +++ b/fboss/cli/fboss2/test/integration_test/DeleteInterfaceTest.cpp @@ -1,20 +1,25 @@ // (c) Meta Platforms, Inc. and affiliates. Confidential and proprietary. /** - * End-to-end tests for 'fboss2-dev delete interface [...]' + * End-to-end tests for 'fboss2-dev delete interface + * [ ...]' * - * These tests: + * The attribute tests: * 1. Pick an interface from the running system * 2. Set one or more attributes via the config CLI * 3. Delete (reset to defaults) via the delete CLI * 4. Verify the command succeeds (exit code 0) * + * The bare (no-attribute) tests cover whole-object deletes: a port by name, + * and an L3 interface by its interface ID. + * * Requirements: * - FBOSS agent must be running with a valid configuration * - The test must be run as root (or with appropriate permissions) */ #include +#include #include #include #include @@ -22,6 +27,7 @@ #include "fboss/cli/fboss2/test/integration_test/Fboss2IntegrationTest.h" using namespace facebook::fboss; +using ::testing::HasSubstr; class DeleteInterfaceTest : public Fboss2IntegrationTest { protected: @@ -260,3 +266,25 @@ TEST_F(DeleteInterfaceTest, DeleteWholePortRemovesCreatedSubport) { << "controlling port should be restored to its original profile"; XLOG(INFO) << "TEST PASSED"; } + +// Whole-interface deletes and their refusal paths are unit-tested in +// CmdDeleteWholeL3InterfaceTestFixture, not repeated here: the session's port +// map is not rebuilt for interfaces staged in the same session, and a +// PORT-type interface cannot be created via the CLI, so integration versions +// could only run conditionally on the DUT's existing config. Delete-by-ID +// resolution is still exercised end-to-end by DeleteUnknownInterfaceIdFails. + +// --------------------------------------------------------------------------- +// Test: an interface ID that matches nothing is rejected, and leaves no +// staged change behind. +// --------------------------------------------------------------------------- + +TEST_F(DeleteInterfaceTest, DeleteUnknownInterfaceIdFails) { + auto del = runCli({"delete", "interface", "65535"}); + discardSession(); + + EXPECT_NE(del.exitCode, 0) << "unknown interface ID should be rejected"; + EXPECT_THAT(del.stderr + del.stdout, HasSubstr("not found in configuration")) + << "should fail on resolution, not on some later error"; + XLOG(INFO) << "TEST PASSED"; +} diff --git a/fboss/cli/fboss2/utils/InterfaceList.cpp b/fboss/cli/fboss2/utils/InterfaceList.cpp index 5c8dbaddf13c3..1785ac8092f08 100644 --- a/fboss/cli/fboss2/utils/InterfaceList.cpp +++ b/fboss/cli/fboss2/utils/InterfaceList.cpp @@ -9,7 +9,9 @@ */ #include "fboss/cli/fboss2/utils/InterfaceList.h" +#include #include +#include #include #include #include @@ -20,6 +22,19 @@ namespace facebook::fboss::utils { +namespace { + +// Parse a purely-numeric name as an ID (port logical ID or interface ID). +std::optional parseId(const std::string& name) { + auto result = folly::tryTo(name); + if (result.hasValue() && *result >= 0) { + return *result; + } + return std::nullopt; +} + +} // namespace + InterfaceList::InterfaceList(std::vector names, bool allowMissing) : names_(std::move(names)) { // Get the PortMap from the session @@ -31,12 +46,24 @@ InterfaceList::InterfaceList(std::vector names, bool allowMissing) for (const auto& name : names_) { Intf intf(name); - // First try to look up as a port name - cfg::Port* port = portMap.getPort(name); + // First try to look up as a port name. A purely-numeric name may also + // be a port logical ID; name lookups take precedence over ID lookups. + std::string portName = name; + if (!portMap.hasPort(portName)) { + auto id = parseId(name); + if (id) { + auto resolvedPortName = portMap.getPortNameForLogicalId(PortID(*id)); + if (resolvedPortName) { + portName = *resolvedPortName; + } + } + } + + cfg::Port* port = portMap.getPort(portName); if (port) { intf.setPort(port); // Also try to get the associated interface - auto interfaceId = portMap.getInterfaceIdForPort(name); + auto interfaceId = portMap.getInterfaceIdForPort(portName); if (interfaceId) { cfg::Interface* interface = portMap.getInterface(*interfaceId); if (interface) { @@ -44,8 +71,15 @@ InterfaceList::InterfaceList(std::vector names, bool allowMissing) } } } else { - // If not found as a port name, try as an interface name + // If not found as a port, try as an interface name, then as an + // interface ID. cfg::Interface* interface = portMap.getInterfaceByName(name); + if (!interface) { + auto id = parseId(name); + if (id) { + interface = portMap.getInterface(InterfaceID(*id)); + } + } if (interface) { intf.setInterface(interface); } diff --git a/fboss/cli/fboss2/utils/PortMap.cpp b/fboss/cli/fboss2/utils/PortMap.cpp index 2f1e152902424..b9579e8c0727a 100644 --- a/fboss/cli/fboss2/utils/PortMap.cpp +++ b/fboss/cli/fboss2/utils/PortMap.cpp @@ -199,6 +199,15 @@ std::optional PortMap::getPortNameForInterface( return std::nullopt; } +std::optional PortMap::getPortNameForLogicalId( + PortID logicalId) const { + auto it = portLogicalIdToName_.find(logicalId); + if (it != portLogicalIdToName_.end()) { + return it->second; + } + return std::nullopt; +} + std::optional PortMap::getPortLogicalId( const std::string& portName) const { auto it = portNameToLogicalId_.find(portName); diff --git a/fboss/cli/fboss2/utils/PortMap.h b/fboss/cli/fboss2/utils/PortMap.h index f73b9943ca2e6..772db25b41941 100644 --- a/fboss/cli/fboss2/utils/PortMap.h +++ b/fboss/cli/fboss2/utils/PortMap.h @@ -64,6 +64,14 @@ class PortMap { std::optional getPortNameForInterface( InterfaceID interfaceId) const; + /** + * Get the port name for a given port logical ID. + * + * @param logicalId The port logical ID + * @return The port name if found, std::nullopt otherwise + */ + std::optional getPortNameForLogicalId(PortID logicalId) const; + /** * Get the port logical ID for a given port name. *