[Nexthop][fboss2-dev] add config interface <intf> name <value> attribute

fboss/cli/fboss2/commands/config/interface/CmdConfigInterface.cpp

@@ -50,6 +50,7 @@ namespace {
 const std::unordered_set<std::string> kKnownAttributes = [] {
   std::unordered_set<std::string> attrs = {
       "description",
+      "name",
       "mtu",
       "ip-address",
       "ipv6-address",
@@ -76,7 +77,7 @@ const std::unordered_set<std::string> kValuelessAttributes = {
 };
 
 constexpr auto kValidConfigAttrs =
-    "description, mtu, ip-address, ipv6-address, profile, loopback-mode, "
+    "description, name, mtu, ip-address, ipv6-address, profile, loopback-mode, "
     "flow-control-rx, flow-control-tx, lldp-expected-*, type, shutdown, "
     "no-shutdown, lookup-class, queue-config";
 
@@ -506,6 +507,61 @@ bool applyLookupClass(
   return changed;
 }
 
+// Set the name of the L3 interface(s) targeted by the command. Since an
+// interface name must be unique, only a single target interface is allowed,
+// and the new name must not collide with an existing port or interface name.
+// Purely-numeric names are rejected: they would shadow lookups by port
+// logical ID or interface ID.
+bool applyInterfaceName(
+    const std::string& value,
+    const utils::InterfaceList& interfaces) {
+  if (value.empty()) {
+    throw std::invalid_argument("Interface name cannot be empty");
+  }
+  if (std::all_of(value.begin(), value.end(), ::isdigit)) {
+    throw std::invalid_argument(
+        fmt::format(
+            "Invalid interface name '{}': a purely-numeric name would "
+            "conflict with lookups by port or interface ID",
+            value));
+  }
+
+  std::vector<cfg::Interface*> targets;
+  for (const utils::Intf& intf : interfaces) {
+    cfg::Interface* interface = intf.getInterface();
+    if (interface) {
+      targets.push_back(interface);
+    }
+  }
+  if (targets.size() > 1) {
+    throw std::invalid_argument(
+        "Cannot set the same name on multiple interfaces");
+  }
+
+  auto& portMap = ConfigSession::getInstance().getPortMap();
+  if (portMap.hasPort(value)) {
+    throw std::invalid_argument(
+        fmt::format("'{}' is already in use as a port name", value));
+  }
+  cfg::Interface* existing = portMap.getInterfaceByName(value);
+  if (existing && (targets.empty() || existing != targets[0])) {
+    throw std::invalid_argument(
+        fmt::format(
+            "'{}' is already in use by interface {}",
+            value,
+            *existing->intfID()));
+  }
+
+  if (targets.empty()) {
+    return false;
+  }
+  targets[0]->name() = value;
+  // Refresh the name-based lookup maps so the rest of the session sees the
+  // new name.
+  ConfigSession::getInstance().rebuildPortMap();
+  return true;
+}
+
 // Binds a named queue config to each port, or clears the binding for the
 // reserved `default`.
 //
@@ -601,6 +657,9 @@ CmdConfigInterfaceTraits::RetType CmdConfigInterface::queryClient(
         }
       }
       results.push_back(fmt::format("description=\"{}\"", value));
+    } else if (attr == "name") {
+      changed |= applyInterfaceName(value, effectiveInterfaces);
+      results.push_back(fmt::format("name=\"{}\"", value));
     } else if (attr == "ip-address" || attr == "ipv6-address") {
       validateInterfaceIpAttr(attr, value);
 

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

fboss/cli/fboss2/test/config/CmdConfigInterfaceTest.cpp

@@ -71,6 +71,13 @@ class CmdConfigInterfaceTestFixture : public CmdConfigTestBase {
         "vlanID": 2,
         "name": "eth1/2/1",
         "mtu": 1500
+      },
+      {
+        "intfID": 3001,
+        "routerID": 0,
+        "vlanID": 0,
+        "name": "",
+        "mtu": 1500
       }
     ]
   }
@@ -396,6 +403,97 @@ TEST_F(CmdConfigInterfaceTestFixture, queryClientSetsMtu) {
   }
 }
 
+// Test renaming an interface addressed by its port name
+TEST_F(CmdConfigInterfaceTestFixture, queryClientSetsInterfaceName) {
+  setupTestableConfigSession(cmdPrefix_, "eth1/1/1 name uplink_1");
+  auto cmd = CmdConfigInterface();
+  InterfacesConfig config({"eth1/1/1", "name", "uplink_1"});
+
+  auto result = cmd.queryClient(localhost(), config);
+
+  EXPECT_THAT(result, HasSubstr("Successfully configured"));
+  EXPECT_THAT(result, HasSubstr("name=\"uplink_1\""));
+
+  auto& intfs =
+      *ConfigSession::getInstance().getAgentConfig().sw()->interfaces();
+  for (const auto& intf : intfs) {
+    if (*intf.intfID() == 1) {
+      EXPECT_EQ(*intf.name(), "uplink_1");
+    } else if (*intf.intfID() == 2) {
+      EXPECT_EQ(*intf.name(), "eth1/2/1");
+    }
+  }
+}
+
+// A nameless interface (name set to the empty string) can still be addressed
+// by its interface ID to give it a name.
+TEST_F(CmdConfigInterfaceTestFixture, queryClientSetsNameOnNamelessInterface) {
+  setupTestableConfigSession(cmdPrefix_, "3001 name svi_mgmt");
+  auto cmd = CmdConfigInterface();
+  InterfacesConfig config({"3001", "name", "svi_mgmt"});
+
+  auto result = cmd.queryClient(localhost(), config);
+
+  EXPECT_THAT(result, HasSubstr("Successfully configured"));
+  EXPECT_THAT(result, HasSubstr("name=\"svi_mgmt\""));
+
+  auto& intfs =
+      *ConfigSession::getInstance().getAgentConfig().sw()->interfaces();
+  bool found = false;
+  for (const auto& intf : intfs) {
+    if (*intf.intfID() == 3001) {
+      found = true;
+      EXPECT_EQ(*intf.name(), "svi_mgmt");
+    }
+  }
+  EXPECT_TRUE(found);
+
+  // The interface is now also addressable by its new name.
+  utils::InterfaceList relist({"svi_mgmt"});
+  ASSERT_EQ(relist.size(), 1);
+  ASSERT_NE(relist[0].getInterface(), nullptr);
+  EXPECT_EQ(*relist[0].getInterface()->intfID(), 3001);
+}
+
+// A purely-numeric name is rejected: it would shadow ID-based lookups.
+TEST_F(CmdConfigInterfaceTestFixture, queryClientNumericNameThrows) {
+  setupTestableConfigSession(cmdPrefix_, "3001 name 1234");
+  auto cmd = CmdConfigInterface();
+  InterfacesConfig config({"3001", "name", "1234"});
+
+  EXPECT_THROW(cmd.queryClient(localhost(), config), std::invalid_argument);
+}
+
+// A name already used by another interface or by a port is rejected.
+TEST_F(CmdConfigInterfaceTestFixture, queryClientDuplicateNameThrows) {
+  setupTestableConfigSession(cmdPrefix_, "3001 name eth1/2/1");
+  auto cmd = CmdConfigInterface();
+
+  // eth1/2/1 is both a port name and another interface's name.
+  InterfacesConfig config({"3001", "name", "eth1/2/1"});
+  EXPECT_THROW(cmd.queryClient(localhost(), config), std::invalid_argument);
+}
+
+// Renaming an interface to the name it already has is idempotent.
+TEST_F(CmdConfigInterfaceTestFixture, queryClientRenameToSameName) {
+  setupTestableConfigSession(cmdPrefix_, "3001 name svi_mgmt");
+  auto cmd = CmdConfigInterface();
+
+  InterfacesConfig config({"3001", "name", "svi_mgmt"});
+  cmd.queryClient(localhost(), config);
+  auto result = cmd.queryClient(localhost(), config);
+  EXPECT_THAT(result, HasSubstr("Successfully configured"));
+}
+
+// Setting the same name on multiple interfaces is rejected.
+TEST_F(CmdConfigInterfaceTestFixture, queryClientNameOnMultipleThrows) {
+  setupTestableConfigSession(cmdPrefix_, "eth1/1/1 eth1/2/1 name uplink_1");
+  auto cmd = CmdConfigInterface();
+
+  InterfacesConfig config({"eth1/1/1", "eth1/2/1", "name", "uplink_1"});
+  EXPECT_THROW(cmd.queryClient(localhost(), config), std::invalid_argument);
+}
+
 // Regression test: ip-address/ipv6-address must persist the session config
 // to disk. A missing `changed = true` in the ip-address branch used to skip
 // saveConfig() while still reporting success (so `config session diff`

fboss/cli/fboss2/utils/InterfaceList.cpp

@@ -9,7 +9,9 @@
  */
 
 #include "fboss/cli/fboss2/utils/InterfaceList.h"
+#include <folly/Conv.h>
 #include <folly/String.h>
+#include <optional>
 #include <stdexcept>
 #include <string>
 #include <utility>
@@ -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<int32_t> parseId(const std::string& name) {
+  auto result = folly::tryTo<int32_t>(name);
+  if (result.hasValue() && *result >= 0) {
+    return *result;
+  }
+  return std::nullopt;
+}
+
+} // namespace
+
 InterfaceList::InterfaceList(std::vector<std::string> names, bool allowMissing)
     : names_(std::move(names)) {
   // Get the PortMap from the session
@@ -31,21 +46,40 @@ InterfaceList::InterfaceList(std::vector<std::string> 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) {
           intf.setInterface(interface);
         }
       }
     } 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);
       }

fboss/cli/fboss2/utils/PortMap.cpp

@@ -199,6 +199,16 @@ std::optional<std::string> PortMap::getPortNameForInterface(
   return std::nullopt;
 }
 
+std::optional<std::string> PortMap::getPortNameForLogicalId(
+    // @lint-ignore CLANGTIDY performance-unnecessary-value-param
+    PortID logicalId) const {
+  auto it = portLogicalIdToName_.find(logicalId);
+  if (it != portLogicalIdToName_.end()) {
+    return it->second;
+  }
+  return std::nullopt;
+}
+
 std::optional<PortID> PortMap::getPortLogicalId(
     const std::string& portName) const {
   auto it = portNameToLogicalId_.find(portName);

fboss/cli/fboss2/utils/PortMap.h

@@ -64,6 +64,16 @@ class PortMap {
   std::optional<std::string> 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<std::string> getPortNameForLogicalId(
+      // @lint-ignore CLANGTIDY performance-unnecessary-value-param
+      PortID logicalId) const;
+
   /**
    * Get the port logical ID for a given port name.
    *