[Nexthop][fboss2-dev] fboss2 bgp policy routing-policy term action commands

cmake/CliFboss2.cmake

@@ -986,6 +986,11 @@ add_library(fboss2_config_lib
   fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.h
   fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/CmdConfigProtocolBgpPolicyRoutingPolicy.cpp
   fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/CmdConfigProtocolBgpPolicyRoutingPolicy.h
+  fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/BgpRoutingPolicyCliUtils.h
+  fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.cpp
+  fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.h
+  fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/action/CmdConfigProtocolBgpPolicyRoutingPolicyTermAction.cpp
+  fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/action/CmdConfigProtocolBgpPolicyRoutingPolicyTermAction.h
   fboss/cli/fboss2/commands/config/ptp/CmdConfigPtp.cpp
   fboss/cli/fboss2/commands/config/ptp/CmdConfigPtp.h
   fboss/cli/fboss2/commands/config/ptp/transparent_clock/CmdConfigPtpTransparentClock.cpp
@@ -1132,6 +1137,8 @@ add_library(fboss2_config_lib
   fboss/cli/fboss2/commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.h
   fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/CmdDeleteProtocolBgpPolicyRoutingPolicy.cpp
   fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/CmdDeleteProtocolBgpPolicyRoutingPolicy.h
+  fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.cpp
+  fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.h
   fboss/cli/fboss2/commands/delete/protocol/CmdDeleteProtocol.h
   fboss/cli/fboss2/commands/delete/protocol/static/CmdDeleteProtocolStatic.cpp
   fboss/cli/fboss2/commands/delete/protocol/static/CmdDeleteProtocolStatic.h

cmake/CliFboss2TestConfig.cmake

@@ -19,6 +19,8 @@ add_executable(fboss2_cmd_config_test
   fboss/cli/fboss2/test/config/CmdConfigBgpPolicyPrefixListTest.cpp
   fboss/cli/fboss2/test/config/CmdConfigBgpPolicyPrefixListEntryTest.cpp
   fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTest.cpp
+  fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermTest.cpp
+  fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermActionTest.cpp
   fboss/cli/fboss2/test/config/CmdConfigCoppTest.cpp
   fboss/cli/fboss2/test/config/CmdConfigDataPlaneTrafficPolicyTest.cpp
   fboss/cli/fboss2/test/config/CmdConfigDhcpTest.cpp
@@ -60,6 +62,7 @@ add_executable(fboss2_cmd_config_test
   fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyCommunityListTest.cpp
   fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyPrefixListTest.cpp
   fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyRoutingPolicyTest.cpp
+  fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyRoutingPolicyTermTest.cpp
   fboss/cli/fboss2/test/config/CmdDeleteConfigInterfaceTest.cpp
   fboss/cli/fboss2/test/config/CmdDeleteCoppQueueTest.cpp
   fboss/cli/fboss2/test/config/CmdDeleteCoppReasonTest.cpp

cmake/CliFboss2TestIntegrationTest.cmake

@@ -23,6 +23,8 @@ add_executable(fboss2_integration_test
   fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyPrefixListTest.cpp
   fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyPrefixListEntryTest.cpp
   fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTest.cpp
+  fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermTest.cpp
+  fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermActionTest.cpp
   fboss/cli/fboss2/test/integration_test/ConfigBgpSessionTest.cpp
   fboss/cli/fboss2/test/integration_test/ConfigConcurrentSessionsTest.cpp
   fboss/cli/fboss2/test/integration_test/ConfigHostnameTest.cpp

fboss/cli/fboss2/BUCK

@@ -1220,6 +1220,8 @@ cpp_library(
         "commands/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.cpp",
         "commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.cpp",
         "commands/config/protocol/bgp/policy/routing-policy/CmdConfigProtocolBgpPolicyRoutingPolicy.cpp",
+        "commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.cpp",
+        "commands/config/protocol/bgp/policy/routing-policy/term/action/CmdConfigProtocolBgpPolicyRoutingPolicyTermAction.cpp",
         "commands/config/protocol/static/CmdConfigProtocolStatic.cpp",
         "commands/config/protocol/static/route/add/CmdConfigProtocolStaticRouteAdd.cpp",
         "commands/config/ptp/CmdConfigPtp.cpp",
@@ -1292,6 +1294,7 @@ cpp_library(
         "commands/delete/protocol/bgp/policy/community-list/CmdDeleteProtocolBgpPolicyCommunityList.cpp",
         "commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.cpp",
         "commands/delete/protocol/bgp/policy/routing-policy/CmdDeleteProtocolBgpPolicyRoutingPolicy.cpp",
+        "commands/delete/protocol/bgp/policy/routing-policy/term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.cpp",
         "commands/delete/protocol/static/CmdDeleteProtocolStatic.cpp",
         "commands/delete/protocol/static/route/CmdDeleteProtocolStaticRoute.cpp",
         "commands/delete/qos/CmdDeleteQos.cpp",
@@ -1374,6 +1377,9 @@ cpp_library(
         "commands/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.h",
         "commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.h",
         "commands/config/protocol/bgp/policy/routing-policy/CmdConfigProtocolBgpPolicyRoutingPolicy.h",
+        "commands/config/protocol/bgp/policy/routing-policy/BgpRoutingPolicyCliUtils.h",
+        "commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.h",
+        "commands/config/protocol/bgp/policy/routing-policy/term/action/CmdConfigProtocolBgpPolicyRoutingPolicyTermAction.h",
         "commands/config/protocol/static/CmdConfigProtocolStatic.h",
         "commands/config/protocol/static/route/StaticRouteUtils.h",
         "commands/config/protocol/static/route/add/CmdConfigProtocolStaticRouteAdd.h",
@@ -1447,6 +1453,7 @@ cpp_library(
         "commands/delete/protocol/bgp/policy/community-list/CmdDeleteProtocolBgpPolicyCommunityList.h",
         "commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.h",
         "commands/delete/protocol/bgp/policy/routing-policy/CmdDeleteProtocolBgpPolicyRoutingPolicy.h",
+        "commands/delete/protocol/bgp/policy/routing-policy/term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.h",
         "commands/delete/protocol/static/CmdDeleteProtocolStatic.h",
         "commands/delete/protocol/static/route/CmdDeleteProtocolStaticRoute.h",
         "commands/delete/qos/CmdDeleteQos.h",

fboss/cli/fboss2/CmdArgsLists.h

@@ -71,7 +71,7 @@ class CmdArgsLists {
  private:
   // 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;
 
   std::array<std::vector<std::string>, MAX_DEPTH> data_;
 };

fboss/cli/fboss2/CmdListConfig.cpp

@@ -48,6 +48,8 @@
 #include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.h"
 #include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.h"
 #include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/CmdConfigProtocolBgpPolicyRoutingPolicy.h"
+#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.h"
+#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/action/CmdConfigProtocolBgpPolicyRoutingPolicyTermAction.h"
 #include "fboss/cli/fboss2/commands/config/protocol/static/CmdConfigProtocolStatic.h"
 #include "fboss/cli/fboss2/commands/config/protocol/static/route/add/CmdConfigProtocolStaticRouteAdd.h"
 #include "fboss/cli/fboss2/commands/config/ptp/CmdConfigPtp.h"
@@ -117,6 +119,7 @@
 #include "fboss/cli/fboss2/commands/delete/protocol/bgp/policy/community-list/CmdDeleteProtocolBgpPolicyCommunityList.h"
 #include "fboss/cli/fboss2/commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.h"
 #include "fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/CmdDeleteProtocolBgpPolicyRoutingPolicy.h"
+#include "fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.h"
 #include "fboss/cli/fboss2/commands/delete/protocol/static/CmdDeleteProtocolStatic.h"
 #include "fboss/cli/fboss2/commands/delete/protocol/static/route/CmdDeleteProtocolStaticRoute.h"
 #include "fboss/cli/fboss2/commands/delete/qos/CmdDeleteQos.h"
@@ -482,6 +485,40 @@ const CommandTree& kConfigCommandTree() {
                                    CmdConfigProtocolBgpPolicyRoutingPolicy>,
                                argRegistrar<
                                    CmdConfigProtocolBgpPolicyRoutingPolicyTraits>,
+                               {{
+                                   "term",
+                                   "Configure a routing-policy term: "
+                                   "<seq-num> [<attribute> <value> ...] "
+                                   "(description)",
+                                   commandHandler<
+                                       CmdConfigProtocolBgpPolicyRoutingPolicyTerm>,
+                                   argRegistrar<
+                                       CmdConfigProtocolBgpPolicyRoutingPolicyTermTraits>,
+                                   {{
+                                       "action",
+                                       "Configure a term action",
+                                       {{
+                                            "result",
+                                            "Set the term result: "
+                                            "<ACCEPT|REJECT|CONTINUE>",
+                                            commandHandler<
+                                                CmdConfigProtocolBgpPolicyRoutingPolicyTermActionResult>,
+                                            argRegistrar<
+                                                CmdConfigProtocolBgpPolicyRoutingPolicyTermActionResultTraits>,
+                                        },
+                                        {
+                                            "set",
+                                            "Set a route attribute: "
+                                            "as-path prepend|community|"
+                                            "local-pref|med|next-hop|"
+                                            "origin|weight <value> ...",
+                                            commandHandler<
+                                                CmdConfigProtocolBgpPolicyRoutingPolicyTermActionSet>,
+                                            argRegistrar<
+                                                CmdConfigProtocolBgpPolicyRoutingPolicyTermActionSetTraits>,
+                                        }},
+                                   }},
+                               }},
                            }},
                       },
                   },
@@ -862,6 +899,14 @@ const CommandTree& kConfigCommandTree() {
                              CmdDeleteProtocolBgpPolicyRoutingPolicy>,
                          argRegistrar<
                              CmdDeleteProtocolBgpPolicyRoutingPolicyTraits>,
+                         {{
+                             "term",
+                             "Delete a routing-policy term: <seq-num>",
+                             commandHandler<
+                                 CmdDeleteProtocolBgpPolicyRoutingPolicyTerm>,
+                             argRegistrar<
+                                 CmdDeleteProtocolBgpPolicyRoutingPolicyTermTraits>,
+                         }},
                      }},
                 }},
            },

fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/BgpRoutingPolicyCliUtils.h

@@ -0,0 +1,108 @@
+/*
+ *  Copyright (c) Meta Platforms, Inc. and affiliates.
+ *  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 <algorithm>
+#include <cstdint>
+#include <string>
+
+#ifndef IS_OSS
+#include <configerator/structs/neteng/bgp_policy/thrift/gen-cpp2/bgp_policy_types.h>
+#include <configerator/structs/neteng/fboss/bgp/gen-cpp2/bgp_config_types.h>
+#else
+#include <neteng/fboss/bgp/public_tld/configerator/structs/neteng/fboss/bgp/gen-cpp2/bgp_config_types.h>
+#endif
+
+/**
+ * Lookup/create helpers for the routing-policy CLI family, shared between the
+ * policy-level dispatcher (CmdConfigProtocolBgpPolicyRoutingPolicy), the term
+ * subcommand (CmdConfigProtocolBgpPolicyRoutingPolicyTerm), and their delete
+ * counterparts. A routing-policy (BgpPolicyStatement) is keyed by name; a
+ * term (BgpPolicyTerm in policy_entries[]) is keyed by sequence_number.
+ */
+namespace facebook::fboss::bgpcli {
+
+inline bool routingPolicyExists(
+    const bgp::thrift::BgpConfig& cfg,
+    const std::string& name) {
+  if (!cfg.policies().has_value()) {
+    return false;
+  }
+  const auto& policies = *cfg.policies()->bgp_policy_statements();
+  return std::any_of(policies.begin(), policies.end(), [&](const auto& p) {
+    return *p.name() == name;
+  });
+}
+
+// The routing-policy statement keyed by name, or nullptr. Non-creating;
+// used by the delete commands so a typo'd delete can't stage a change.
+inline bgp::bgp_policy::BgpPolicyStatement* findRoutingPolicy(
+    bgp::thrift::BgpConfig& cfg,
+    const std::string& name) {
+  if (!cfg.policies().has_value()) {
+    return nullptr;
+  }
+  auto& policies = *cfg.policies()->bgp_policy_statements();
+  auto it = std::find_if(policies.begin(), policies.end(), [&](const auto& p) {
+    return *p.name() == name;
+  });
+  return it == policies.end() ? nullptr : &*it;
+}
+
+// Find the routing-policy statement keyed by name, creating it if absent.
+// Setting an attribute on a not-yet-created policy implicitly creates it, so
+// command ordering stays forgiving; a bare `routing-policy <name>` creates one
+// explicitly. BgpPolicyStatement's only key field is `name`.
+inline bgp::bgp_policy::BgpPolicyStatement& findOrCreateRoutingPolicy(
+    bgp::thrift::BgpConfig& cfg,
+    const std::string& name) {
+  auto& policies = *cfg.policies().ensure().bgp_policy_statements();
+  for (auto& policy : policies) {
+    if (*policy.name() == name) {
+      return policy;
+    }
+  }
+  policies.emplace_back();
+  auto& policy = policies.back();
+  policy.name() = name;
+  return policy;
+}
+
+inline bool routingPolicyTermExists(
+    const bgp::bgp_policy::BgpPolicyStatement& policy,
+    int64_t seqNum) {
+  const auto& terms = *policy.policy_entries();
+  return std::any_of(terms.begin(), terms.end(), [&](const auto& t) {
+    return t.sequence_number().has_value() && *t.sequence_number() == seqNum;
+  });
+}
+
+// Find the term keyed by sequence_number, creating it if absent. The name is
+// seeded from the seq-num so every term has a stable, unique identity —
+// BgpPolicyAction.next_term_id references terms by name.
+inline bgp::bgp_policy::BgpPolicyTerm& findOrCreateRoutingPolicyTerm(
+    bgp::bgp_policy::BgpPolicyStatement& policy,
+    int64_t seqNum) {
+  auto& terms = *policy.policy_entries();
+  for (auto& term : terms) {
+    if (term.sequence_number().has_value() &&
+        *term.sequence_number() == seqNum) {
+      return term;
+    }
+  }
+  terms.emplace_back();
+  auto& term = terms.back();
+  term.sequence_number() = seqNum;
+  term.name() = std::to_string(seqNum);
+  return term;
+}
+
+} // namespace facebook::fboss::bgpcli

fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/CmdConfigProtocolBgpPolicyRoutingPolicy.cpp

@@ -22,6 +22,7 @@
 #include <vector>
 #include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpCliAttrHandlers.h"
 #include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpCliValueParsers.h"
+#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/BgpRoutingPolicyCliUtils.h"
 #include "fboss/cli/fboss2/session/ConfigSession.h"
 #include "fboss/cli/fboss2/utils/CmdUtilsCommon.h"
 #include "fboss/cli/fboss2/utils/HostInfo.h"
@@ -85,37 +86,6 @@ std::string validAttrList() {
   return out;
 }
 
-// Find the routing-policy statement keyed by name, creating it if absent.
-// Setting an attribute on a not-yet-created policy implicitly creates it, so
-// command ordering stays forgiving; a bare `routing-policy <name>` creates one
-// explicitly. BgpPolicyStatement's only key field is `name`.
-BgpPolicyStatement& findOrCreatePolicy(
-    bgp::thrift::BgpConfig& cfg,
-    const std::string& name) {
-  auto& policies = *cfg.policies().ensure().bgp_policy_statements();
-  for (auto& policy : policies) {
-    if (*policy.name() == name) {
-      return policy;
-    }
-  }
-  policies.emplace_back();
-  auto& policy = policies.back();
-  policy.name() = name;
-  return policy;
-}
-
-bool policyExists(const bgp::thrift::BgpConfig& cfg, const std::string& name) {
-  if (!cfg.policies().has_value()) {
-    return false;
-  }
-  for (const auto& policy : *cfg.policies()->bgp_policy_statements()) {
-    if (*policy.name() == name) {
-      return true;
-    }
-  }
-  return false;
-}
-
 } // namespace
 
 // Parse + validate at construction so queryClient stays a thin dispatch.
@@ -156,8 +126,9 @@ CmdConfigProtocolBgpPolicyRoutingPolicy::queryClient(
     const ObjectArgType& args) {
   auto& session = ConfigSession::getInstance();
   auto& cfg = session.getBgpConfig();
-  const bool policyCreated = !policyExists(cfg, args.policyName());
-  auto& policy = findOrCreatePolicy(cfg, args.policyName());
+  const bool policyCreated =
+      !bgpcli::routingPolicyExists(cfg, args.policyName());
+  auto& policy = bgpcli::findOrCreateRoutingPolicy(cfg, args.policyName());
 
   Result result = args.attr().empty()
       ? ok(policyCreated

fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/CmdConfigProtocolBgpPolicyRoutingPolicy.h

@@ -58,9 +58,12 @@ struct CmdConfigProtocolBgpPolicyRoutingPolicyTraits
     : public WriteCommandTraits {
   using ParentCmd = CmdConfigProtocolBgpPolicy;
   static void addCliArg(CLI::App& cmd, std::vector<std::string>& args) {
-    // Stops CLI11 from classifying attribute tokens as subcommands once the
-    // policy name is consumed. See CmdConfigProtocolBgpNeighborTraits.
-    cmd.positionals_at_end();
+    // Unlike the sibling policy families, no positionals_at_end() here: the
+    // seq-num-keyed term level is a real CLI11 subcommand (`routing-policy
+    // <name> term <seq-num> ...`), and positionals_at_end() would stop CLI11
+    // from classifying `term` after the policy name. The trade-off is that a
+    // free-text attribute value equal to `term` is stolen by subcommand
+    // matching (same exposure as the `config interface` subcommand chain).
     cmd.add_option("args", args, "<name> [<attribute> <value> ...]");
   }
   using ObjectArgType = BgpRoutingPolicyConfig;