[Nexthop][fboss2-dev] Support deleting an L3 interface with delete interface

cmake/CliFboss2.cmake

@@ -834,6 +834,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

fboss/cli/fboss2/BUCK

@@ -1079,6 +1079,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",
@@ -1232,6 +1233,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",

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 <folly/String.h>
+#include <algorithm>
+#include <cstdint>
+#include <set>
+#include <string>
+#include <vector>
+#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<std::string> tunnelsOnUnderlayIntf(
+    const cfg::SwitchConfig& swConfig,
+    int32_t intfId) {
+  std::vector<std::string> 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<std::string> enabledMemberPorts(
+    const cfg::SwitchConfig& swConfig,
+    int32_t vlanId,
+    const std::set<PortID>& portsBeingDeleted) {
+  std::set<int32_t> memberPorts;
+  for (const auto& vlanPort : *swConfig.vlanPorts()) {
+    if (*vlanPort.vlanID() == vlanId) {
+      memberPorts.insert(*vlanPort.logicalPort());
+    }
+  }
+
+  std::vector<std::string> 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<InterfaceID>& 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<InterfaceID>& goingAway,
+    const std::set<PortID>& 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()) : "<unset>",
+        ". 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<InterfaceID>& intfIds,
+    const std::set<PortID>& 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<int32_t>(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

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 <set>
+
+#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 <port-name>
+  //   - 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 <id>
+  //   - 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 <id> has no interface, even when corresp port <port> is
+  //     enabled"
+  //       -> disable or unbind the member ports, or drop the whole VLAN with
+  //          delete vlan <id>
+  //     Ports listed in portsBeingDeleted are excluded from this check: a
+  //     single 'delete interface <svi> <its-only-port>' 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<InterfaceID>& intfIds,
+      const std::set<PortID>& portsBeingDeleted = {});
+};
+
+} // namespace facebook::fboss

fboss/cli/fboss2/commands/delete/interface/CmdDeleteInterface.cpp

@@ -23,7 +23,9 @@
 #include <vector>
 
 #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<std::string>& 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<PortID> portsToDelete;
+    std::set<InterfaceID> interfacesToDelete;
     std::vector<std::string> 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 <port> 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
+    // <port> 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));