[cacl][test] Add coverage for caclmgrd-owned dhcp_server syslog rule#26478
[cacl][test] Add coverage for caclmgrd-owned dhcp_server syslog rule#26478Xichen96 wants to merge 1 commit into
Conversation
|
/azp run |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Adds a new CACL behavioral test to validate that the dhcp_server docker0 syslog exception (RELP tcp/2514) is owned by caclmgrd, is ordered above the control-plane catch-all DROP, survives a control-plane ACL flush/rebuild, and follows FEATURE|dhcp_server enable/disable transitions.
Changes:
- Add
test_caclmgrd_dhcp_server_syslogto assert presence + ordering of thedocker0tcp/2514 ACCEPT rule while the feature is enabled. - Reuse
dummy_acl_rulesto force a control-plane ACL flush/rebuild and verify the rule is re-emitted. - Toggle
dhcp_serverfeature to validate rule removal and restoration, with cleanup restoring the original feature state.
16da940 to
643bf0c
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
tests/cacl/test_cacl_application.py:1450
- This new test hard-codes the tcp/2514
dhcp_server_syslogrule, but elsewhere in this same filegenerate_expected_rules()still describes/expects the older docker0 syslog exception as UDP/514 and is gated on container presence (see around lines 581-586). If that earlier expectation isn’t updated (or this PR isn’t rebased onto the change from #26477), the file will have conflicting assumptions about the dhcp_server syslog rule and the overall CACL suite can become inconsistent across branches/images.
dhcp_syslog_rule = ("-A INPUT -i docker0 -p tcp -m tcp --dport 2514"
" -m comment --comment dhcp_server_syslog -j ACCEPT")
catch_all_drop = "-A INPUT -j DROP"
orig_state = duthost.shell(
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
tests/cacl/test_cacl_application.py:1509
- The cleanup path waits for dhcp_server state/rule to converge back to the original value, but the return value of wait_until(...) is ignored. If the restore fails or never converges, the test can still pass and leave the DUT in a modified state, causing follow-on tests to be flaky. Please assert that the cleanup convergence actually succeeded.
wait_until(
|
This PR has backport request label(s) for branch(es): 202605, but is missing required test information. Please make sure you tick the tested branch(es) in the Tested branch section and provide test evidence (e.g., 202605: <test result>) in the Test result section as well in your PR description. ---Powered by SONiC BuildBot
|
643bf0c to
9d29613
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
tests/cacl/test_cacl_application.py:1456
- The teardown fixture currently only waits for CONFIG_DB FEATURE|dhcp_server 'state' to match the original value. That key reflects desired config and may flip immediately after
config feature state, so this doesn't guarantee the iptables rule/programming has converged; if the test fails mid-toggle, a transient rule/state can still leak into the next test despite the fixture's intent. Consider also waiting for the docker0 tcp/2514 rule presence/absence to matchorig_statewhen restoring.
cur_state = duthost.shell(
"sonic-db-cli CONFIG_DB HGET 'FEATURE|dhcp_server' state",
module_ignore_errors=True)["stdout"].strip()
if cur_state != orig_state:
duthost.shell("sudo config feature state dhcp_server {}".format(orig_state),
module_ignore_errors=True)
pytest_assert(
wait_until(
60, 5, 0,
lambda: duthost.shell(
"sonic-db-cli CONFIG_DB HGET 'FEATURE|dhcp_server' state",
module_ignore_errors=True)["stdout"].strip() == orig_state),
"dhcp_server feature state did not converge back to {} during teardown"
.format(orig_state))
Add test_caclmgrd_dhcp_server_syslog verifying the docker0 RELP tcp/2514 INPUT exception that caclmgrd now owns (sonic-net/sonic-buildimage#28328, sonic-net/sonic-host-services#412, sonic-net/sonic-buildimage#28580; fixes sonic-net/sonic-buildimage#27584): * present above the control-plane catch-all DROP while FEATURE|dhcp_server is enabled, * survives a control-plane ACL flush-and-rebuild (forced via the dummy_acl_rules fixture / acl-loader), and * removed on feature-disable and restored on feature-enable. The test skips when dhcp_server is not enabled on the DUT. Signed-off-by: Xichen Lin <lukelin0907@gmail.com>
9d29613 to
72f6145
Compare
Description of PR
Summary:
Add explicit CACL coverage for the caclmgrd-owned
dhcp_serverdocker0syslog (RELP tcp/2514)INPUTexception. Ownership of this rule moved from the container start/stop script intocaclmgrd, which re-emits it on every control-plane ACL flush-and-rebuild whileFEATURE|dhcp_serveris enabled, so it survives the rebuild (root cause of sonic-net/sonic-buildimage#27584).New test
test_caclmgrd_dhcp_server_syslogasserts:docker0tcp/2514ACCEPTrule is present above caclmgrd's control-plane catch-allDROPwhile the feature is enabled;dummy_acl_rulesfixture forces one viaacl-loader);The test skips when
dhcp_serveris not enabled on the DUT (bridge-mode, host-namespace only; enabled by default on e.g.mx).This is the optional coverage called out in #26470. It complements the expectation update in #26477.
Depends on #26477 (stacked). #26477 updates
generate_expected_rules(udp/514 -> tcp/2514); this PR only adds the new behavioral test. Merge order is #26477 first, then rebase this PR, so the two never present conflicting expectations on the target branch.Product change set this covers (all merged on master):
caclmgrdowns the rule.Fixes # (issue)
Related: #26470. ADO: 38902700 (parent PBI 38699922). Underlying bug: sonic-net/sonic-buildimage#27584.
Type of change
Back port request
Tracking issue/work item for backport/cherry-pick request (GitHub issue or Microsoft ADO): Microsoft ADO 38902700 / #26470
Failure type: day-one issue (the container-owned docker0 syslog rule never survived a caclmgrd control-plane ACL rebuild; this coverage follows the product change onto each branch it is backported to)
Tested branch
Test result
The behavior asserted here (caclmgrd owns the docker0 tcp/2514 rule above the catch-all DROP, survives an ACL rebuild, toggles with the feature) was confirmed during the 3-PR hardware validation on
bjw-can-720dt-2(720dt,mx, 202605 image 20260510.07) by hand-patching the caclmgrd + container changes onto a running DUT. A full pytest run of this new case against a live DUT requires a test image that already contains the three product PRs above (submodule-propagated); until then the CACL path on the old image still programs udp/514. This is a source/test/image ordering dependency — the coverage merges in lockstep with the product change reaching the image.Approach
What is the motivation for this PR?
test_cacl_application(updated in #26477) checks the static expected ruleset but does not exercise the ownership contract that #27584 is about: that caclmgrd re-emits the rule across a rebuild and tracks the feature toggle. This adds that behavioral coverage.How did you do it?
Added
test_caclmgrd_dhcp_server_syslog, reusing thedummy_acl_rulesfixture to apply control-plane ACLs (which forces a caclmgrd INPUT flush/rebuild and the catch-all DROP). The test readsFEATURE|dhcp_serverstate viasonic-db-cli, skips when disabled, and otherwise checks rule presence/ordering, survival across the rebuild, and the disable/enable transitions, restoring the original feature state in afinallyblock.How did you verify/test it?
python3 -m py_compile+ repo pre-commit (flake8) pass. Behavior matches the hand-patched DUT observations from the 3-PR HW validation. Full suite run is gated on image propagation as noted in Test result.Any platform specific information?
Host-namespace IPv4 INPUT rule; only runs where the
dhcp_serverfeature is enabled (skips otherwise).Supported testbed topology if it's a new test case?
Any topology with
dhcp_serverenabled (e.g.mx); skipped elsewhere. Module marker istopology('any', 'bmc').Documentation
N/A.