Skip to content

[202608] [caclmgrd] Own the redfish docker0 syslog INPUT exception #23

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
shreyansh-nexthop wants to merge 1 commit into
base: 202608
Choose a base branch
from
+152 −3
Open

[202608] [caclmgrd] Own the redfish docker0 syslog INPUT exception #23

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions scripts/caclmgrd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -670,9 +670,11 @@ class ControlPlaneAclManager(logger.Logger):
iptables_cmds.append(self.iptables_cmd_ns_prefix[namespace] + ['iptables', '-A', 'INPUT', '-s', '127.0.0.1', '-i', 'lo', '-j', 'ACCEPT'])
iptables_cmds.append(self.iptables_cmd_ns_prefix[namespace] + ['ip6tables', '-A', 'INPUT', '-s', '::1', '-i', 'lo', '-j', 'ACCEPT'])

# 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

@nikamirrr Nikolay Mirin (nikamirrr) Sep 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

"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.

if namespace == DEFAULT_NAMESPACE:

@nikamirrr Nikolay Mirin (nikamirrr) Sep 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

for feature, allowed in (("redfish", self.RedfishAllowed), ("dhcp_server", self.DhcpServerSyslogAllowed)):

@nikamirrr Nikolay Mirin (nikamirrr) Sep 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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 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'])

@nikamirrr Nikolay Mirin (nikamirrr) Sep 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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_server enabled and redfish disabled, redfish's RELP traffic is still ACCEPTed by the dhcp_server_syslog rule. test_rule_absent_when_feature_disabled therefore asserts a gate that does not exist on the wire.
  • With both enabled, iptables -L INPUT carries two identical ACCEPT rules and the dhcp_server_syslog one is dead — the redfish_syslog one 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 self.bfdAllowed:
iptables_cmds += self.get_bfd_iptable_commands(namespace)
Expand Down
147 changes: 147 additions & 0 deletions tests/caclmgrd/caclmgrd_redfish_syslog_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,147 @@
import os

@nikamirrr Nikolay Mirin (nikamirrr) Sep 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

import sys

from swsscommon import swsscommon
from sonic_py_common.general import load_module_from_source
from unittest import TestCase, mock
from pyfakefs.fake_filesystem_unittest import patchfs

from tests.common.mock_configdb import MockConfigDb


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.

@nikamirrr Nikolay Mirin (nikamirrr) Sep 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

REDFISH_SYSLOG_RULE = (
'iptables', '-A', 'INPUT', '-i', 'docker0', '-p', 'tcp', '--dport', '2514',
'-j', 'ACCEPT', '-m', 'comment', '--comment', 'redfish_syslog',
)


class TestCaclmgrdRedfishSyslog(TestCase):
"""
Verifies caclmgrd owns the redfish docker0 syslog (RELP tcp/2514) INPUT
exception, gated on FEATURE.redfish, and re-emits it before the
catch-all DROP on every rebuild.

redfish is bridge-networked, so its rsyslog forwards over RELP to the
docker0 gateway instead of 127.0.0.1. That traffic arrives on docker0
(not lo), so it is not covered by the loopback ACCEPT and would be
swept into the control-plane catch-all DROP without this exception.
"""
def setUp(self):
swsscommon.ConfigDBConnector = MockConfigDb
test_path = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
modules_path = os.path.dirname(test_path)
scripts_path = os.path.join(modules_path, "scripts")
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

@nikamirrr Nikolay Mirin (nikamirrr) Sep 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.


def setup_daemon(self, config_db):
MockConfigDb.set_config_db(config_db)
self.caclmgrd.ControlPlaneAclManager.get_namespace_mgmt_ip = mock.MagicMock()

@nikamirrr Nikolay Mirin (nikamirrr) Sep 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

self.caclmgrd.ControlPlaneAclManager.get_namespace_mgmt_ipv6 = mock.MagicMock()
self.caclmgrd.ControlPlaneAclManager.generate_block_ip2me_traffic_iptables_commands = mock.MagicMock(return_value=[])
self.caclmgrd.ControlPlaneAclManager.generate_allow_internal_docker_ip_traffic_commands = mock.MagicMock(return_value=[])
self.caclmgrd.ControlPlaneAclManager.generate_allow_internal_chasis_midplane_traffic = mock.MagicMock(return_value=[])
self.caclmgrd.ControlPlaneAclManager.get_chain_list = mock.MagicMock(return_value=["INPUT", "FORWARD", "OUTPUT"])
self.caclmgrd.ControlPlaneAclManager.get_chassis_midplane_interface_ip = mock.MagicMock(return_value='')
return self.caclmgrd.ControlPlaneAclManager("caclmgrd")

@patchfs
def test_init_seeds_flag_from_feature_state(self, fs):
"""RedfishAllowed is seeded from the persisted FEATURE state at __init__."""
if not os.path.exists(DBCONFIG_PATH):
fs.create_file(DBCONFIG_PATH)

enabled = self.setup_daemon({"DEVICE_METADATA": {"localhost": {}},
"FEATURE": {"redfish": {"state": "enabled"}}})
disabled = self.setup_daemon({"DEVICE_METADATA": {"localhost": {}},
"FEATURE": {"redfish": {"state": "disabled"}}})
absent = self.setup_daemon({"DEVICE_METADATA": {"localhost": {}},
"FEATURE": {}})
self.assertTrue(enabled.RedfishAllowed)
self.assertFalse(disabled.RedfishAllowed)
self.assertFalse(absent.RedfishAllowed)

@patchfs
def test_rule_emitted_when_feature_enabled(self, fs):

@nikamirrr Nikolay Mirin (nikamirrr) Sep 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

"""When enabled, the docker0/2514 ACCEPT rule is present in the host rebuild."""
if not os.path.exists(DBCONFIG_PATH):
fs.create_file(DBCONFIG_PATH)

daemon = self.setup_daemon({"DEVICE_METADATA": {"localhost": {}},
"FEATURE": {"redfish": {"state": "enabled"}}})
self.assertTrue(daemon.RedfishAllowed)

cmds, _ = daemon.get_acl_rules_and_translate_to_iptables_commands('', MockConfigDb())
self.assertIn(REDFISH_SYSLOG_RULE, [tuple(c) for c in cmds])

@patchfs
def test_rule_absent_when_feature_disabled(self, fs):
"""When disabled, no exception is programmed."""
if not os.path.exists(DBCONFIG_PATH):
fs.create_file(DBCONFIG_PATH)

daemon = self.setup_daemon({"DEVICE_METADATA": {"localhost": {}},
"FEATURE": {"redfish": {"state": "disabled"}}})
self.assertFalse(daemon.RedfishAllowed)

cmds, _ = daemon.get_acl_rules_and_translate_to_iptables_commands('', MockConfigDb())
self.assertNotIn(REDFISH_SYSLOG_RULE, [tuple(c) for c in cmds])

@patchfs
def test_rule_absent_when_feature_entry_missing(self, fs):
"""Images built without redfish have no FEATURE entry at all -- no exception."""
if not os.path.exists(DBCONFIG_PATH):
fs.create_file(DBCONFIG_PATH)

daemon = self.setup_daemon({"DEVICE_METADATA": {"localhost": {}},
"FEATURE": {}})
self.assertFalse(daemon.RedfishAllowed)

cmds, _ = daemon.get_acl_rules_and_translate_to_iptables_commands('', MockConfigDb())
self.assertNotIn(REDFISH_SYSLOG_RULE, [tuple(c) for c in cmds])

@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

@nikamirrr Nikolay Mirin (nikamirrr) Sep 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

AND strictly before the DROP -- there is never a rebuild window where a DROP
exists without it."""
if not os.path.exists(DBCONFIG_PATH):
fs.create_file(DBCONFIG_PATH)

daemon = self.setup_daemon({
"ACL_TABLE": {
"SSH_ONLY": {"stage": "INGRESS", "type": "CTRLPLANE", "services": ["SSH"]},
},
"ACL_RULE": {
"SSH_ONLY|RULE_1": {"PACKET_ACTION": "ACCEPT", "PRIORITY": "9999", "SRC_IP": "10.0.0.0/8"},
},
"DEVICE_METADATA": {"localhost": {}},
"FEATURE": {"redfish": {"state": "enabled"}},
})

cmds, _ = daemon.get_acl_rules_and_translate_to_iptables_commands('', MockConfigDb())
cmds = [tuple(c) for c in cmds]
catch_all_drop = ('iptables', '-A', 'INPUT', '-j', 'DROP')
self.assertIn(REDFISH_SYSLOG_RULE, cmds, "exception must be present in rebuild")
self.assertIn(catch_all_drop, cmds, "test setup should produce a catch-all DROP")
self.assertLess(cmds.index(REDFISH_SYSLOG_RULE), cmds.index(catch_all_drop),
"exception must come before the catch-all DROP")

@patchfs
def test_rule_host_namespace_only(self, fs):
"""docker0 lives in the host namespace, so asic namespaces get no exception."""
if not os.path.exists(DBCONFIG_PATH):
fs.create_file(DBCONFIG_PATH)

daemon = self.setup_daemon({"DEVICE_METADATA": {"localhost": {}},
"FEATURE": {"redfish": {"state": "enabled"}}})
self.assertTrue(daemon.RedfishAllowed)

daemon.iptables_cmd_ns_prefix['asic0'] = []
cmds, _ = daemon.get_acl_rules_and_translate_to_iptables_commands('asic0', MockConfigDb())
self.assertNotIn(REDFISH_SYSLOG_RULE, [tuple(c) for c in cmds])