Repository navigation
[Nexthop][fboss2-dev] fboss2 config/delete protocol bgp policy prefix-list commands - #1477
hillol-nexthop wants to merge 8 commits into
Conversation
|
@hillol-nexthop has updated the pull request. You must reimport the pull request before landing. |
|
This pull request has been imported. If you are a Meta employee, you can view this in D115057124. (Because this pull request was imported automatically, there will not be any future comments.) |
b8b80b3 to
7681ea9
Compare
|
@hillol-nexthop has updated the pull request. You must reimport the pull request before landing. |
7681ea9 to
851312f
Compare
|
@hillol-nexthop has updated the pull request. You must reimport the pull request before landing. |
851312f to
2525baf
Compare
|
@hillol-nexthop has updated the pull request. You must reimport the pull request before landing. |
2525baf to
633de9f
Compare
|
@hillol-nexthop has updated the pull request. You must reimport the pull request before landing. |
633de9f to
8f81d30
Compare
| constexpr std::string_view kCompareOperatorNe = "NE"; | ||
| constexpr std::string_view kCompareOperatorGt = "GT"; | ||
| constexpr std::string_view kCompareOperatorLt = "LT"; | ||
| constexpr std::string_view kCompareOperatorRg = "RG"; |
There was a problem hiding this comment.
This seems to be unused, could we clean it up
|
@hillol-nexthop has updated the pull request. You must reimport the pull request before landing. |
…y prefix-list Adds `config protocol bgp policy prefix-list <name> [entry <seq-num>] [<attribute> <value> ...]` and `delete protocol bgp policy prefix-list <name> [entry <seq-num>]` — the third BGP policy object-type command family, beneath the `policy` grouping node alongside as-path-list and community-list. - prefix-list dispatcher with a two-level key (list name + entry seq-num) writing bgp_policy.BgpPolicies.prefix_lists[] through the typed ConfigSession. List level: boolean-operator, compare-operator (EQ|GE|LE|NE|GT|LT), description, and ip-version <v4|v6> (stored as the numeric routing_policy.PrefixList.version, the field the sheet documents). Entry level (routing_policy.PrefixListEntry keyed by seq_num in prefixes[]): base-prefix (validated as <prefix/len>, an explicit /len is required), communities (accumulates into the optional set), description, match-logic, max-allowed-subnet-count (-> max_allowed_golden_prefix_subnet_count), regex, and the prefix-len-range compare-operator|value pair targeting the single CompareNumericValue the CLI maintains at prefix_len_ranges[0] (compare-operator additionally accepts RG; value is bounded 0-128). - delete mirrors community-list's two levels: the whole list by name, or a single entry by `entry <seq-num>`. - new intAttr factory in the shared BgpCliAttrHandlers.h (bounded int32, valueDesc taken by value like enumAttr's), used by the two numeric entry attributes. - unit tests (19) + commit-path integration tests mirroring ConfigBgpPolicyCommunityListTest, run on a DUT (all 4 pass). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Deleting a prefix-list that a routing-policy term still names in prefix_filters.prefix_list_names committed a dangling reference; bgpd fails the config load on it and crash-loops. The whole-list delete now refuses and names every referencing `policy <P> term <seq>`, the same rule the peer-group and as-path-list deletes apply. Deleting a single `entry <seq>` stays allowed: the name the terms resolve remains defined. The scanner is file-local: the term/match CLIs live above this change, so the tests seed the reference through the thrift structs directly. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Adds generate_prefix_list_commands (description, boolean-operator when not the OR default, compare-operator, ip-version from the `version` field) and hooks it into the policy block after the community-lists. RG at the list level, `prefix_list_names`, an unexpected `version` and the `ip_version` enum field have no CLI spelling and are emitted as `# WARNING:` lines. Entries follow in the entry subcommand change. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Delete mirrors add: deleting an absent list or an absent entry is a success with a `Warning:` and saves nothing, so a replayed script stays idempotent. The term-reference guard still errors. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
_boolean_operator_name() and _comparison_operator_name() call int() on the JSON value, so a null or a list in boolean_operator/compare_operator aborted the whole conversion with a Python traceback. j2c is deliberately not a validator: the fboss2 dispatchers are the single place that decides what is legal, and the script's warnings are reserved for valid thrift the CLI cannot express. So the helpers now catch TypeError and ValueError and return the raw value, which reaches the generated script quoted and is rejected by the CLI on replay with its normal message. Values int() accepts (floats, bools) are left alone for the same reason. _boolean_operator_name() predates the prefix-list work; the fix is here because this PR adds its sibling and the review landed on both. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
bgpd validates a PrefixList only when a routing-policy term references it (PrefixTreeMatch::validateAndCreatePrefixTree): any list-level compare_operator throws "Unsupported PrefixList configuration", and any boolean_operator other than OR throws "PrefixList BooleanOperator can only be OR". The CLI offered six compare operators and three boolean operators, so a committed list could be rejected wholesale the moment a policy used it, and the orphan-list integration test could not see that. Drop the compare-operator attribute and its unused RG constant, offer only boolean-operator OR, and make bgp_json_to_cli.py warn-and-skip instead of emitting the values bgpd rejects. The integration test stages OR and asserts the daemon reads back 2. Also drops a stale comment about composed prefix-len-range attribute names that this list-level file never had. Two more converter fixes from the same review: warning text now goes through _printable() inside _warning(), so a control character in a JSON name cannot end the # comment line and become shell input on replay; and generate_prefix_list_commands warns with the count when it drops prefixes[] (the as-path-list and community-list emitters already warn for theirs) until the entry subcommand adds real emission. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
bgpd rejects any PrefixListEntry that carries seq_num (PolicyMatch.cpp, "Unsupported Prefix configuration: seq_num"), yet the delete command's `entry <seq-num>` selector and the shared findOrCreatePrefixListEntry() helper used seq_num as the entry's stored identity. Rather than ship a delete path whose config bgpd refuses, this PR stops at the list level: `delete protocol bgp policy prefix-list <name>` only. The entry selector, the entry helpers and their tests move to the entry subcommand PR, which also decides the entry's identity (re-key by base_prefix, or relax bgpd). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
bgpd resolves a community-list reference by name at config load, from a
term's communities_filter.community_list_names (CommunityMatch) or its
community_action_list_names (CommunityAction), and fails the load on a
dangling one. The delete command erased a list without looking, and it
also reset communities to unset after removing the last value, which a
community action rejects ("Missing communities in CommunityList").
bgpd further requires a term's inline copy of the list to carry the same
boolean_operator as the named list ("Conflicting boolean_operator from the
reference") and reads exact_match only from that inline copy, so setting
either on the named list alone produced a config bgpd rejects or ignores.
Add findTermsReferencingCommunityList() beside the as-path-list one; refuse
a whole-list delete while referenced, refuse removing the last community
while a community action references the list, and write boolean_operator
and exact_match through to every referencing match filter. Mirrors what
the as-path-list command already does.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
65cad63 to
04be951
Compare
|
@hillol-nexthop has updated the pull request. You must reimport the pull request before landing. |
|
@induvsuresh merged this pull request in 7404660. |
Summary: > **Stacked on #1477 (#1477 is now rebased on `main`) (`config/delete protocol bgp policy prefix-list`). This PR's branch contains the whole stack below it, so the diff shows those commits too — please review only the `bgp policy prefix-list prefix` commit(s) here. Merge the lower PRs first. > > **Stack (bottom → top):** #1477 → **#1487 (this PR)** → #1488 → #1489 → #1490 → #1491 Adds the entry level of `config protocol bgp policy prefix-list` as its own CLI11 subcommand, `prefix-list <name> prefix <prefix/len> [<attribute> <value> ...]`, plus `delete ... prefix-list <name> prefix <prefix/len>`. **Entries are keyed by their prefix, not by a sequence number.** bgpd identifies a `PrefixListEntry` by `base_prefix`: it merges entries that share one, ignores order, and throws `"Unsupported Prefix configuration: seq_num"` on any entry carrying `seq_num`. A `seq-num` key therefore has nowhere to live that bgpd accepts, so the documented `entry <seq-num>` level is spelled `prefix <prefix/len>`: the prefix is the entry's identity and its `base_prefix`, the separate `base-prefix` attribute goes away, and nothing writes `seq_num`. One entry per prefix is the only shape the CLI needs; the documented grammar already exposes a single `prefix-len-range`, and bgpd merges the rest. Changing an entry's prefix is delete plus create, like every other keyed object. The list command owns only list attributes; this command owns the entry, including the composed `prefix-len-range` sub-attribute and the hand-written handlers whose value shapes have no factory (communities, prefix-len-range, regex). The list name reaches it through the ancestor-args tuple, and lookup/create is shared through `BgpPrefixListCliUtils.h`, which now keys entries by `base_prefix`. Only values bgpd accepts are offered: `match-logic EQUAL` only and the `EQ|GE|LE|NE|GT|LT` range operators (bgpd throws on `NOT_EQUAL` and `RG`); `max-allowed-subnet-count` is kept but documented as read only for golden-prefix policies. `bgp_json_to_cli.py` emits `prefix <base_prefix>` per entry, warns and skips the rejected values, drops a JSON `seq_num` with a warning, and skips an entry without `base_prefix`. A list attribute typed alongside `prefix` is rejected rather than silently dropped, because only the leaf command runs. ## Sample usage ``` $ fboss2-dev config protocol bgp policy prefix-list PL100 prefix 10.0.0.0/8 Successfully created BGP prefix-list PL100 prefix 10.0.0.0/8 Config saved to: ~/.fboss2/bgp_config.json $ fboss2-dev config protocol bgp policy prefix-list PL100 prefix 10.0.0.0/8 prefix-len-range compare-operator GE Successfully set prefix-len-range compare-operator to: GE for prefix-list PL100 prefix 10.0.0.0/8 $ fboss2-dev config protocol bgp policy prefix-list PL100 prefix 10.0.0.0/8 prefix-len-range value 24 Successfully set prefix-len-range value to: 24 for prefix-list PL100 prefix 10.0.0.0/8 $ fboss2-dev config protocol bgp policy prefix-list PL100 prefix 10.0.0.0/8 communities 65000:100 Successfully added 65000:100 to communities for prefix-list PL100 prefix 10.0.0.0/8 $ fboss2-dev config protocol bgp policy prefix-list PL100 prefix 10.0.0.0/8 max-allowed-subnet-count 64 Successfully set max-allowed-subnet-count to: 64 for prefix-list PL100 prefix 10.0.0.0/8 $ fboss2-dev config protocol bgp policy prefix-list PL100 prefix 10.0.0.0 Error: Invalid prefix '10.0.0.0'; expected <prefix/len> $ fboss2-dev config protocol bgp policy prefix-list PL100 prefix 10.0.0.0/8 match-logic NOT_EQUAL Error: Invalid match-logic value 'NOT_EQUAL'; expected EQUAL $ fboss2-dev delete protocol bgp policy prefix-list PL100 prefix 10.0.0.0/8 Successfully deleted BGP prefix-list PL100 prefix 10.0.0.0/8 ``` Pull Request resolved: #1487 Test Plan: Bazel (`bazel.sh test //fboss/cli/fboss2/test/config:cmd_config_test`): full suite 1282/1282 pass, including the 15 `CmdConfigBgpPolicyPrefixListEntryTestFixture` and 8 `CmdDeleteBgpPolicyPrefixListTestFixture` cases; the converter suite (`test_bgp_json_to_cli.py`) passes. clang-tidy over the changed files reports only `modernize-type-traits` inside gtest's `EXPECT_THROW` expansion. Integration, `run_test.py fboss2_integration --filter="ConfigBgpPolicy*"` on a gold DUT prepared with the latest stable image (build 238327): ``` [ OK ] ConfigBgpPolicyPrefixListEntryTest.SetAttributesAndCommit [ OK ] ConfigBgpPolicyPrefixListEntryTest.DeleteEntryAndCommit PASSED : 12 FAILED : 0 ``` Based-on: #1477 ## Review follow-ups carried from the list-level PR - The entry-level delete selector, `prefixListEntryExists()` / `findOrCreatePrefixListEntry()` and their tests live here, so the entry subcommand and its delete side change together. - Entry identity is settled by keying on the prefix (see Summary); the earlier `seq-num` key was rejected by bgpd. - `match-logic` accepts only `EQUAL` and `prefix-len-range compare-operator` no longer offers `RG`: bgpd throws on both. `max-allowed-subnet-count` is kept but documented as read only for golden-prefix policies. j2c warns and skips the rejected values. Reviewed By: srikrishnagopu Differential Revision: D126102729 Pulled By: induvsuresh fbshipit-source-id: 295eaa6e7ded050c8aa4b9736bb6c6e4b22e59de
Summary: > **Stacked on #1487 (`bgp policy prefix-list entry`). This PR's branch contains the whole stack below it, so the diff shows those commits too — please review only the `config/delete protocol bgp policy routing-policy` commit(s) here. Merge the lower PRs first. > > **Stack (bottom → top):** #1477 → #1487 → **#1488 (this PR)** → #1489 → #1490 → #1491 Adds `config protocol bgp policy routing-policy <name> [description <string>]` and `delete protocol bgp policy routing-policy <name>`, staging a `BgpPolicyStatement` in the BGP config session. A bare `routing-policy <name>` creates (or selects) the statement; `description` is its only documented policy-level attribute. The nested term level is a separate stacked PR. The dispatcher follows the shape the other BGP policy families use: one factory per value shape in `BgpCliAttrHandlers.h`, named setters that do nothing but assign the thrift field, and a registry that is one line per attribute. Lookup/create helpers live in `BgpRoutingPolicyCliUtils.h` so the term subcommand and the delete command share them. A rejected value leaves nothing staged — a policy implicitly created for the failed command is rolled back. **Delete is idempotent.** Deleting an absent target is a success with a `Warning:` (nothing is saved), so a replayed script never fails on something already gone; the reference guards still return `Error:`. ## Sample usage ``` $ fboss2-dev config protocol bgp policy routing-policy EXPORT-POLICY Successfully created BGP routing-policy EXPORT-POLICY Config saved to: ~/.fboss2/bgp_config.json $ fboss2-dev config protocol bgp policy routing-policy EXPORT-POLICY description Spine export policy Successfully set description to: Spine export policy for routing-policy EXPORT-POLICY Config saved to: ~/.fboss2/bgp_config.json $ fboss2-dev delete protocol bgp policy routing-policy EXPORT-POLICY Successfully deleted BGP routing-policy EXPORT-POLICY Config saved to: ~/.fboss2/bgp_config.json ``` Idempotent delete, recorded on a lab device: ``` $ fboss2-dev delete protocol bgp policy routing-policy NO-SUCH Warning: BGP routing-policy NO-SUCH does not exist; nothing to delete ``` Pull Request resolved: #1488 Test Plan: **Unit (delete)** — `fboss2_cmd_config_test`: ``` $ fboss2_cmd_config_test --gtest_filter=*DeleteBgpPolicyRoutingPolicy* [ OK ] CmdDeleteBgpPolicyRoutingPolicyTestFixture.argValidation (108 ms) [ OK ] CmdDeleteBgpPolicyRoutingPolicyTestFixture.deleteExistingPolicy (149 ms) [ OK ] CmdDeleteBgpPolicyRoutingPolicyTestFixture.deleteUnknownPolicyWarns (87 ms) [ OK ] CmdDeleteBgpPolicyRoutingPolicyTestFixture.deleteTwiceIsIdempotent (109 ms) [ OK ] CmdDeleteBgpPolicyRoutingPolicyTestFixture.deleteUnknownLeavesOthersIntact (78 ms) [ OK ] CmdDeleteBgpPolicyRoutingPolicyTestFixture.deleteReferencedPolicyRejected (165 ms) [ OK ] CmdDeleteBgpPolicyRoutingPolicyTestFixture.deleteReferencedPolicyNamesEveryReferrer (188 ms) [ OK ] CmdDeleteBgpPolicyRoutingPolicyTestFixture.referenceToOtherPolicyDoesNotBlockDelete (221 ms) [==========] 8 tests from 1 test suites ran. [ PASSED ] 8 tests. ``` **Unit** — `fboss2_cmd_config_test`: ``` $ fboss2_cmd_config_test --gtest_filter=*BgpPolicyRoutingPolicy* [ OK ] CmdConfigBgpPolicyRoutingPolicyTestFixture.argValidation (95 ms) [ OK ] CmdConfigBgpPolicyRoutingPolicyTestFixture.bareCreatePolicy (92 ms) [ OK ] CmdConfigBgpPolicyRoutingPolicyTestFixture.setDescription (89 ms) [ OK ] CmdConfigBgpPolicyRoutingPolicyTestFixture.namedPoliciesAreDistinct (208 ms) [ OK ] CmdConfigBgpPolicyRoutingPolicyTestFixture.reReferenceReportsExisting (85 ms) [ OK ] CmdConfigBgpPolicyRoutingPolicyTestFixture.emptyDescriptionRejected (54 ms) [ OK ] CmdConfigBgpPolicyRoutingPolicyTestFixture.rejectedValueOnExistingPolicyKeepsPolicy (73 ms) [ OK ] CmdDeleteBgpPolicyRoutingPolicyTestFixture.argValidation (57 ms) [ OK ] CmdDeleteBgpPolicyRoutingPolicyTestFixture.deleteExistingPolicy (112 ms) [ OK ] CmdDeleteBgpPolicyRoutingPolicyTestFixture.deleteUnknownPolicyRejected (54 ms) [ OK ] CmdDeleteBgpPolicyRoutingPolicyTestFixture.deleteUnknownLeavesOthersIntact (94 ms) [ OK ] CmdDeleteBgpPolicyRoutingPolicyTestFixture.deleteReferencedPolicyRejected (101 ms) [ OK ] CmdDeleteBgpPolicyRoutingPolicyTestFixture.deleteReferencedPolicyNamesEveryReferrer (151 ms) [ OK ] CmdDeleteBgpPolicyRoutingPolicyTestFixture.referenceToOtherPolicyDoesNotBlockDelete (257 ms) [==========] 14 tests from 2 test suites ran. (1530 ms total) [ PASSED ] 14 tests. ``` **Integration** — `fboss2_integration_test` on a lab device with `bgp_pp` active: ``` $ fboss2_integration_test --gtest_filter='ConfigBgpPolicyRoutingPolicyTest.*' [ OK ] ConfigBgpPolicyRoutingPolicyTest.SetDescriptionAndCommit (1277 ms) [ OK ] ConfigBgpPolicyRoutingPolicyTest.DeletePolicyAndCommit (3340 ms) [==========] 2 tests from 1 test suite ran. [ PASSED ] 2 tests. ``` Based-on: #1487 Reviewed By: joseph5wu Differential Revision: D126423390 Pulled By: induvsuresh fbshipit-source-id: 1f0100b5f6654d4656cbd485d616027a2c9f23ef
Summary
Adds the third BGP policy object-type command family to
fboss2-dev:config protocol bgp policy prefix-list <name> [<attribute> <value>]delete protocol bgp policy prefix-list <name>The entry level ships in #1487 as
prefix-list <name> prefix <prefix/len>for both config and delete: entries are keyed by their prefix, because bgpd rejects aPrefixListEntrycarryingseq_num. Nothing in this PR writes one.Commands
prefix-list <name>prefix_lists[](create/select)prefix-list <name> description <string>PrefixList.descriptionprefix-list <name> boolean-operator ORPrefixList.boolean_operator(bgpd accepts only OR)prefix-list <name> ip-version <v4|v6>PrefixList.version(numeric 4/6)delete ... prefix-list <name>prefix_lists[]entry (refused while a term references it)Design
BgpCliAttrHandlers.h(added in [Nexthop][fboss2-dev] fboss2 config/delete protocol bgp neighbor commands #1391), named setters, one registry line per attribute. Writesbgp_policy.BgpPolicies.prefix_lists[]through the typedConfigSession.PrefixTreeMatch::validateAndCreatePrefixTree(bgp++) throws on any list-levelcompare_operatorand on anyboolean_operatorother than OR, but only once a routing-policy term references the list — so a bad list commits cleanly and fails the whole config later. The CLI therefore has nocompare-operatorattribute and acceptsboolean-operator ORonly;bgp_json_to_cli.pywarns and skips instead of emitting the rejected values, warns with the count when it dropsprefixes[], and sanitizes every warning through_printable()so a control character in a JSON name cannot escape the#comment.BgpPrefixListCliUtils.h, shared with the delete command; the entry helpers arrive with [Nexthop][fboss2-dev] fboss2 bgp policy prefix-list entry subcommand #1487.Delete is idempotent. Deleting an absent target is a success with a
Warning:(nothing is saved), so a replayed script never fails on something already gone; the reference guards still returnError:.Sample usage
Idempotent delete, recorded on a lab device:
Test plan
Unit (delete) —
fboss2_cmd_config_test:Unit —
fboss2_cmd_config_test:Integration —
fboss2_integration_teston a lab device withbgp_ppactive:Community-list follow-ups (landed in #1475)
Review on this PR flagged two gaps in the community-list commands that landed with #1475; the last commit here fixes them the way the as-path-list family already works:
communities_filter.community_list_names(CommunityMatch) andcommunity_action_list_names(CommunityAction) by name at config load and fails the load on a dangling one, and a community action additionally rejects a referenced list with no values ("Missing communities in CommunityList").delete ... community-list <name>now refuses while any term references the list, anddelete ... community-list <name> community <value>refuses to remove the last value while a community action references it.boolean_operatoras the named list ("Conflicting boolean_operator from the reference") and bgpd readsexact_matchonly from that inline copy, so setting either on the named list now writes through to every referencing match filter.Note: bgp++'s
CommunityMatchconstructor currently throws unless the inlinecommunitiesis non-empty, so the CLI cannot yet create a by-name community-list match; the delete guard mainly protects action references and hand-written configs, and the write-through is forward-looking.Based-on: main