[202608] [caclmgrd] Own the redfish docker0 syslog INPUT exception - #23
shreyansh-nexthop wants to merge 1 commit into
Conversation
What: caclmgrd now owns and reprograms an INPUT chain exception for redfish's RELP syslog (tcp/2514) arriving on docker0, plus a matching test pinning the rule byte-for-byte. Why: caclmgrd flushes/rebuilds the INPUT chain on every control-plane ACL change and appends a catch-all DROP; redfish's bridge-networked rsyslog forwards to the docker0 gateway (not lo), so its logs were swept into the DROP and never reached host /var/log/syslog. How: Added a docker0:2514 ACCEPT exception modeled on the existing dhcp_server exception, reprogrammed via the FEATURE-table event handler and scoped by feature-enable. Testing: CI green (all required checks); mergeStateStatus CLEAN, approved by yxieca. Signed-off-by: Shreyansh Jain <shreyansh@nexthop.ai> (cherry picked from commit 0cc46258e1ab5f38eac5b5e4df44429b5d9c99d7)
|
@microsoft-github-policy-service agree company="Nexthop AI" |
Nikolay Mirin (nikamirrr)
left a comment
Nikolay Mirin (nikamirrr)
left a comment
There was a problem hiding this comment.
Reviewed against the upstream counterpart sonic-net/sonic-host-services#429.
The cherry-pick itself is clean: RedfishAllowed, handle_feature_state_events and the __init__ seeding all already exist on 202608, and nothing existing breaks — test_redfish_acl_vectors.py uses issubset, and no vector asserts full-list equality with a redfish/dhcp_server FEATURE enabled.
Detailed notes inline. The two I'd want resolved before merge:
always_enabledis not handled (scripts/caclmgrd:675) — the state check only accepts the literal"enabled", so a feature configuredalways_enablednever gets the rule. That is exactly the log-loss case this PR is meant to fix.- The two emitted rules are byte-identical apart from
--comment(scripts/caclmgrd:677) — so the per-feature gate the new tests assert does not exist on the wire.
One observation outside the diff, for context rather than as a change request:
handle_feature_state_events is called from the select loop (scripts/caclmgrd:1326) with whichever namespace woke it. On multi-ASIC, an ACL event on asic0 satisfies db_id == config_db_id, so the handler drains the host FEATURE subscriber, flips RedfishAllowed, and then queues ctrl_plane_acl_notification.add('asic0'). The host namespace — the only one that emits this rule — is never rebuilt, and the event has been consumed so it is not redelivered. This predates the PR and affects dhcp_server identically; flagging it because this change doubles the number of features that depend on that path.
| iptables_cmds.append(self.iptables_cmd_ns_prefix[namespace] + ['iptables', '-A', 'INPUT', '-i', 'docker0', '-p', 'tcp', '--dport', '2514', '-j', 'ACCEPT', '-m', 'comment', '--comment', 'dhcp_server_syslog']) | ||
| # bridged-container docker0 syslog (RELP tcp/2514) exceptions; host namespace / IPv4 only | ||
| if namespace == DEFAULT_NAMESPACE: | ||
| for feature, allowed in (("redfish", self.RedfishAllowed), ("dhcp_server", self.DhcpServerSyslogAllowed)): |
There was a problem hiding this comment.
always_enabled is a valid FEATURE state and is not handled here.
self.RedfishAllowed / self.DhcpServerSyslogAllowed are seeded in __init__ with ....get("state") == "enabled", and handle_feature_state_events uses the same literal comparison. featured treats always_enabled as running (feature.state in ("always_enabled", "enabled")), and init_cfg.json.j2 ships features that way, so on a box with FEATURE|redfish state=always_enabled the container is up, this loop emits nothing, and the rebuild appends the catch-all DROP — the RELP logs are silently dropped. That is the exact failure this PR sets out to fix.
Suggest widening the comparison in all three places:
state = feature_tb.get("redfish", {}).get("state")
self.RedfishAllowed = state in ("enabled", "always_enabled")Worth a test case too — every test in the new file uses "enabled"/"disabled", so this gap is uncovered.
| if namespace == DEFAULT_NAMESPACE: | ||
| for feature, allowed in (("redfish", self.RedfishAllowed), ("dhcp_server", self.DhcpServerSyslogAllowed)): | ||
| if allowed: | ||
| iptables_cmds.append(self.iptables_cmd_ns_prefix[namespace] + ['iptables', '-A', 'INPUT', '-i', 'docker0', '-p', 'tcp', '--dport', '2514', '-j', 'ACCEPT', '-m', 'comment', '--comment', feature + '_syslog']) |
There was a problem hiding this comment.
Both iterations emit a byte-identical match — -i docker0 -p tcp --dport 2514 -j ACCEPT — differing only in --comment. The rule body never references feature, so:
- With
dhcp_serverenabled andredfishdisabled, redfish's RELP traffic is still ACCEPTed by thedhcp_server_syslogrule.test_rule_absent_when_feature_disabledtherefore asserts a gate that does not exist on the wire. - With both enabled,
iptables -L INPUTcarries two identical ACCEPT rules and thedhcp_server_syslogone is dead — theredfish_syslogone precedes it and matches the same packets.
Either emit a single rule when either feature is on, or make the two rules actually distinguishable.
Separately: the match has no source restriction, so this opens the host rsyslog imrelp listener on tcp/2514 to every container on docker0, not just the redfish container. Scoping with -s <docker0 subnet> (or -d the docker0 IP that rsyslog.conf.j2 binds imrelp to) would keep the fix while preserving the intent of the catch-all DROP.
| if namespace == DEFAULT_NAMESPACE and self.DhcpServerSyslogAllowed: | ||
| iptables_cmds.append(self.iptables_cmd_ns_prefix[namespace] + ['iptables', '-A', 'INPUT', '-i', 'docker0', '-p', 'tcp', '--dport', '2514', '-j', 'ACCEPT', '-m', 'comment', '--comment', 'dhcp_server_syslog']) | ||
| # bridged-container docker0 syslog (RELP tcp/2514) exceptions; host namespace / IPv4 only | ||
| if namespace == DEFAULT_NAMESPACE: |
There was a problem hiding this comment.
The set of bridged-syslog features is now spelled out in four places: this tuple, the if key not in ("redfish", "dhcp_server") filter in handle_feature_state_events, the two __init__ seed lines, and two differently-named attributes (RedfishAllowed vs DhcpServerSyslogAllowed — and RedfishAllowed is additionally overloaded to gate the REDFISH CACL service further down).
Adding a third bridged container means four coordinated edits, and missing any one silently yields a feature whose flag flips but whose rule never appears. A single module-level BRIDGED_SYSLOG_FEATURES driving a self.syslog_allowed = {} dict that both the emitter and the event handler read would collapse all four into one.
| # dhcp_server docker0 syslog (RELP tcp/2514) exception; host namespace / IPv4 only | ||
| if namespace == DEFAULT_NAMESPACE and self.DhcpServerSyslogAllowed: | ||
| iptables_cmds.append(self.iptables_cmd_ns_prefix[namespace] + ['iptables', '-A', 'INPUT', '-i', 'docker0', '-p', 'tcp', '--dport', '2514', '-j', 'ACCEPT', '-m', 'comment', '--comment', 'dhcp_server_syslog']) | ||
| # bridged-container docker0 syslog (RELP tcp/2514) exceptions; host namespace / IPv4 only |
There was a problem hiding this comment.
"IPv4 only" is stated but not enforced. The rebuild appends an ip6tables -A INPUT -j DROP catch-all whenever any CTRLPLANE rule exists, and with "ipv6": true in /etc/docker/daemon.json docker0 gets an IPv6 gateway. If the container's SYSLOG_TARGET_IP resolves to that address, the RELP session arrives over IPv6 and hits that DROP. rsyslog.conf.j2 binds imrelp per-address with no v4 constraint, so this reads as a real (if currently unhit) gap rather than a design decision — worth either emitting the ip6tables counterpart or saying in the comment why v6 cannot happen.
| @@ -0,0 +1,147 @@ | |||
| import os | |||
There was a problem hiding this comment.
This file is a near-verbatim copy of tests/caclmgrd/caclmgrd_dhcp_server_syslog_test.py with s/dhcp_server/redfish/ — setUp, setup_daemon, DBCONFIG_PATH, the rule-tuple comment, and five of the six test bodies are identical to lines 13-133 there.
That file already imports parameterized and already parameterizes the two-feature case in test_handle_feature_state_events_mixed_batch, so the natural form here is one parameterized class over (feature_key, flag_attr, comment) rather than a second copy. The two have already drifted: the dhcp version asserts each seeded flag immediately, this one defers all three asserts to the end.
|
|
||
| DBCONFIG_PATH = '/var/run/redis/sonic-db/database_config.json' | ||
|
|
||
| # Must stay byte-identical to the container-side -C check in docker_image_ctl.j2. |
There was a problem hiding this comment.
There is no -C check in docker_image_ctl.j2 to stay byte-identical to. files/build_templates/docker_image_ctl.j2 in sonic-buildimage contains no iptables invocation at all, and no redfish_syslog/dhcp_server_syslog/2514 rule exists anywhere under files/ or dockers/ outside the rsyslog configs. The comment was carried over verbatim from caclmgrd_dhcp_server_syslog_test.py:15, and the tuple it guards starts with -A, not -C. Either drop it or point it at the real counterpart — as written it sends the next maintainer looking for a cross-repo contract that isn't there.
| self.assertFalse(absent.RedfishAllowed) | ||
|
|
||
| @patchfs | ||
| def test_rule_emitted_when_feature_enabled(self, fs): |
There was a problem hiding this comment.
Nothing here covers the case this diff actually exists for — both redfish and dhcp_server enabled at once. Every test sets FEATURE to exactly one of them.
Unasserted as a result: that both rules are emitted, the order between them, that the dhcp_server rule is unaffected when redfish is also on, and the duplicate-identical-rule outcome noted on scripts/caclmgrd:677. A regression that dropped the second loop iteration, or that made redfish shadow dhcp_server, would pass this entire file and caclmgrd_dhcp_server_syslog_test.py.
|
|
||
| @patchfs | ||
| def test_rule_reinserted_before_catch_all_drop(self, fs): | ||
| """With a CACL rule present (so the catch-all DROP exists), the exception appears |
There was a problem hiding this comment.
The docstring claims more than the assertion can show. assertLess(cmds.index(...), cmds.index(...)) checks ordering inside a generated Python list, and run_commands executes each command as a separate Popen — list order says nothing about the on-box window.
The real window is also the opposite of the one described: after iptables -F INPUT and before the rules are re-appended, INPUT policy is ACCEPT and no CACL rules exist at all. A change to iptables -I would still satisfy this assertion while breaking the stated property. Suggest narrowing the docstring to what is actually checked — emission order within the rebuild.
|
|
||
| def setup_daemon(self, config_db): | ||
| MockConfigDb.set_config_db(config_db) | ||
| self.caclmgrd.ControlPlaneAclManager.get_namespace_mgmt_ip = mock.MagicMock() |
There was a problem hiding this comment.
These six assignments replace methods on the ControlPlaneAclManager class, and swsscommon.ConfigDBConnector = MockConfigDb on line 33 mutates the imported module globally. Neither is undone, and there is no tearDown. sys.path.insert(0, modules_path) on line 37 also runs once per test method, so this file alone adds six duplicate sys.path entries in a full run.
Combined with load_module_from_source('caclmgrd', ...) rebinding sys.modules['caclmgrd'] on every setUp, a sibling such as caclmgrd_redfish_acl_test.py that does mock.patch("caclmgrd.ControlPlaneAclManager.run_commands_pipe") can end up patching a different module object than the one it instantiates — which makes failures order-dependent. mock.patch.object plus addCleanup would contain it.
| sys.path.insert(0, modules_path) | ||
| caclmgrd_path = os.path.join(scripts_path, 'caclmgrd') | ||
| self.caclmgrd = load_module_from_source('caclmgrd', caclmgrd_path) | ||
| self.maxDiff = None |
There was a problem hiding this comment.
Dead here — maxDiff only affects unittest's sequence/dict diff rendering, and every assertion in this file is assertIn/assertNotIn/assertTrue/assertFalse/assertLess on tuples. It is also absent from the file this was cloned from.
2 participants
Manual cherry-pick of sonic-net/sonic-host-services#429 (master commit 0cc46258e1ab) into 202608. The cherry-pick automation on the original PR hit a code conflict and asked for a manual cherry-pick PR.
The conflict no longer exists on the current 202608 head: the dhcp_server docker0 syslog exception from sonic-net/sonic-host-services#412 arrived through the 202605 code sync, so the commit applies cleanly and unmodified.
Why I did it
redfish is bridge-networked and its rsyslog forwards to the host over docker0 (RELP tcp/2514). Without a caclmgrd-owned INPUT exception, control plane ACL rebuilds sweep that traffic into the catch-all DROP and redfish container logs stop reaching the host.
How I did it
git cherry-pick -x 0cc46258e1ab on top of the 202608 head. No conflicts and no content changes relative to master. Includes the unit test from the original PR.
How to verify it
tests/caclmgrd/caclmgrd_redfish_syslog_test.py covers rule presence, feature gating, host-namespace-only emission and ordering before the catch-all DROP.