Skip to content

[Nexthop][fboss2-dev] Add fboss2-dev config acl CLI subcommands#1370

Open
benoit-nexthop wants to merge 1 commit into
facebook:mainfrom
nexthop-ai:config-acltable-basic
Open

[Nexthop][fboss2-dev] Add fboss2-dev config acl CLI subcommands#1370
benoit-nexthop wants to merge 1 commit into
facebook:mainfrom
nexthop-ai:config-acltable-basic

Conversation

@benoit-nexthop

Copy link
Copy Markdown
Contributor

Summary

Adds two new subcommands under config acl for configuring ACL table groups and tables:

  • config acl table-group <group-name> stage <stage-value> — sets AclTableGroup.stage (enum: ingress, ingress-macsec, egress-macsec, ingress-post-lookup, or numeric 0–3)
  • config acl table <group-name> <table-name> priority <value> — sets AclTable.priority (non-negative i16)

The Thrift model has a two-level key structure (AclTableGroup → AclTable). Both commands target sw.aclTableGroups (field 56, the preferred form); the deprecated aclTableGroup (field 45) is not supported.

When FLAGS_enable_acl_table_group is set, both changes are applied at runtime via processAclTableGroupDelta in SaiSwitch with no warmboot-prohibited guard, so the CLI marks them HITLESS. When that flag is off (the default on some platforms today), aclTableGroups changes take a different code path in SaiSwitch; this PR only adds the CLI plumbing and was validated against a DUT with the flag off, so the flag-on runtime path is not covered by the included tests (see Test Plan).

Group/table lookup logic shared between the two subcommand handlers lives in AclConfigUtils.{h,cpp} rather than being duplicated between the table and table-group handlers.

Test Plan

Unit tests (19/19 passing)

The seed config includes two table groups (the first with two tables), so tests can assert that mutating the targeted group/table leaves its siblings untouched, and dedicated tests target the non-first group/table by name — with only one group/table, or only ever mutating index 0, a find_if bug that matched "first element" instead of "match by name" would pass undetected. A separate fixture with no aclTableGroups at all covers the "no aclTableGroups configured" error branch shared by both handlers.

./fboss/oss/scripts/bazel.sh test //fboss/cli/fboss2/test/config:cmd_config_test \
    --test_filter='CmdConfigAcl*' --test_output=all

[ RUN      ] CmdConfigAclTableGroupTestFixture.argValidation_valid                    OK
[ RUN      ] CmdConfigAclTableGroupTestFixture.argValidation_badArity                 OK
[ RUN      ] CmdConfigAclTableGroupTestFixture.argValidation_unknownAttr              OK
[ RUN      ] CmdConfigAclTableGroupTestFixture.argValidation_invalidStage             OK
[ RUN      ] CmdConfigAclTableGroupTestFixture.setStageIngressPostLookup              OK
[ RUN      ] CmdConfigAclTableGroupTestFixture.setStageNoopSameValue                  OK
[ RUN      ] CmdConfigAclTableGroupTestFixture.setStageOnNonFirstGroup                OK
[ RUN      ] CmdConfigAclTableGroupTestFixture.groupNotFound                          OK
[ RUN      ] CmdConfigAclNoGroupsTestFixture.tableGroupNoAclTableGroupsConfigured     OK
[ RUN      ] CmdConfigAclTableTestFixture.argValidation_valid                         OK
[ RUN      ] CmdConfigAclTableTestFixture.argValidation_badArity                      OK
[ RUN      ] CmdConfigAclTableTestFixture.argValidation_unknownAttr                   OK
[ RUN      ] CmdConfigAclTableTestFixture.argValidation_nonInteger                    OK
[ RUN      ] CmdConfigAclTableTestFixture.argValidation_negative                      OK
[ RUN      ] CmdConfigAclTableTestFixture.setPriority                                 OK
[ RUN      ] CmdConfigAclTableTestFixture.setPriorityOnNonFirstTable                  OK
[ RUN      ] CmdConfigAclTableTestFixture.tableNotFound                              OK
[ RUN      ] CmdConfigAclTableTestFixture.groupNotFound                               OK
[ RUN      ] CmdConfigAclNoGroupsTestFixture.tableNoAclTableGroupsConfigured          OK
[  PASSED  ] 19 tests.

Integration tests (NH-4010-F — skipped: DUT has no sw.aclTableGroups)

The NH-4010-F DUT used for testing uses a single flat ACL map without ACL table groups (FLAGS_enable_acl_table_group is off / field 56 absent). The integration tests detect this and skip gracefully:

[ SKIPPED ] ConfigAclTest.SetTableGroupStage — DUT has no sw.aclTableGroups (field 56)
[ SKIPPED ] ConfigAclTest.SetTablePriority   — DUT has no sw.aclTableGroups (field 56)

Sample CLI transcript (NH-4010-F)

$ fboss2-dev config acl --help
Configure ACL table group and table settings
Subcommands:
  table                 Configure an ACL table within a group (e.g. set priority)
  table-group           Configure an ACL table group (e.g. set stage)

$ fboss2-dev config acl table-group --help
Positionals:
  acl_table_group_config TEXT x 3 REQUIRED
    <group-name> stage <stage-value> where <stage-value> is one of:
    ingress, ingress-macsec, egress-macsec, ingress-post-lookup (or numeric 0-3)

$ fboss2-dev config acl table --help
Positionals:
  acl_table_config TEXT x 4 REQUIRED
    <group-name> <table-name> priority <value>

$ fboss2-dev config acl
localhost: Thrift call failed: 'Incomplete command, please use 'table-group' or 'table' subcommand'

Add two new subcommands under `config acl`:

  config acl table-group <group-name> stage <stage-value>
    Sets AclTableGroup.stage (AclStage enum: ingress, ingress-macsec,
    egress-macsec, ingress-post-lookup, or numeric 0-3).

  config acl table <group-name> <table-name> priority <value>
    Sets AclTable.priority (non-negative i16).

Both commands target sw.aclTableGroups (field 56, the preferred form);
the deprecated aclTableGroup (field 45) is not supported. When
FLAGS_enable_acl_table_group is set, both changes are applied at
runtime via processAclTableGroupDelta in SaiSwitch with no
warmboot-prohibited guard, so the CLI marks them HITLESS. When that
flag is off (the default on some platforms today), aclTableGroups
changes take a different code path in SaiSwitch; this PR only adds the
CLI plumbing and was validated against a DUT with the flag off, so the
flag-on runtime path is not covered by the included tests.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@benoit-nexthop benoit-nexthop requested review from a team as code owners July 8, 2026 23:32
@meta-cla meta-cla Bot added the CLA Signed label Jul 8, 2026

@joseph5wu joseph5wu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's support create if not exist for both acl table group and acl table

* 45, is not supported). Throws std::runtime_error if aclTableGroups is
* unset/empty, or if no group with that name exists.
*/
cfg::AclTableGroup& findAclTableGroupOrThrow(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we consider to create a new acl table group if it doesn't exist?

if (it != kAclStageByName.end()) {
return it->second;
}
// Numeric fallback.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't recommend to use Numeric fallback cause they're not straightforward.
Let's just enforce the string values as the only options

std::find_if(tables.begin(), tables.end(), [&](const cfg::AclTable& t) {
return *t.name() == args.getTableName();
});
if (tableIt == tables.end()) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's also support create an acl table if it doesn't exist

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants