[202608] [redfish] Deliver bridged-container syslog to the host over docker0 by shreyansh-nexthop · Pull Request #3137 · Azure/sonic-buildimage-msft

shreyansh-nexthop · 2026-09-17T09:04:10Z

Manual cherry-pick of sonic-net/sonic-buildimage#29258 (master commit d5b23d5) 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 prerequisite dhcp_server docker0 syslog change from sonic-net/sonic-buildimage#28580 arrived through the 202605 code sync (#3112), so the commit applies cleanly and unmodified.

Why I did it

redfish runs bridge-networked, so its rsyslog cannot reach the host syslog listener over 127.0.0.1 and container logs, including startup failures, never reach host /var/log/syslog.

How I did it

git cherry-pick -x d5b23d5 on top of the 202608 head. No conflicts and no content changes relative to master.

How to verify it

Rendered the container control script for the redfish, dhcp_server, snmp and database containers; all pass bash -n and the bridged syslog wait logic renders only for the two bridged containers.

…#29258) What: Adds redfish's bridged-container syslog delivery over the docker0 gateway, touching docker_image_ctl.j2 and rsyslog-config.sh; renders bridged-container syslog handling from a single list so dhcp_server and redfish stay in sync. Why: redfish runs bridge-networked, so its 127.0.0.1 isn't the host's and its rsyslog couldn't reach the host over loopback — container logs (including startup FATALs) never reached host /var/log/syslog. How: Wires up the three pieces dhcp_server already has, refactoring the bridged-container syslog config into a single list to prevent drift between dhcp_server and redfish. Testing: CI green (all required checks); mergeStateStatus CLEAN, approved. Signed-off-by: shreyansh-nexthop <shreyansh@nexthop.ai> (cherry picked from commit d5b23d5)

azure-pipelines · 2026-09-17T09:04:19Z

Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run.

mssonicbld · 2026-09-17T09:04:29Z

/azp run

azure-pipelines · 2026-09-17T09:04:48Z

Azure Pipelines: Successfully started running 1 pipeline(s).

azure-pipelines · 2026-09-17T09:09:06Z

Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run.

Nikolay Mirin (nikamirrr)

Reviewed as a cherry-pick of sonic-net/sonic-buildimage#29258 into 202608.

Port fidelity: good

I rendered docker_image_ctl.j2 for redfish, dhcp_server and snmp; all three render and pass bash -n. The shebang survives the new {%- set %} on line 2, and the rewritten for (( attempt=0; attempt<=BRIDGE_SYSLOG_WAIT_SECS; attempt++ )) preserves upstream's 11-checks/10-sleeps semantics exactly. The dhcp_server -> bridged_containers generalisation is faithful to upstream.

One blocking issue, and it is about merge order

The redfish_syslog iptables rule that the new wait loop polls for does not exist in the pinned caclmgrd — at either the merge-base pin (409d3b2) or the current 202608 tip pin (dfa3cc3). The companion PR that adds it, Azure/sonic-host-services.msft#23, is still open. Nothing is wrong with this diff; it just cannot work until #23 lands. Details inline on line 48.

One correctness issue worth fixing in the port

dhcp_server.service.j2 also carries User={{ sonicadmin_user }}, so the new redfish -> sudo iptables / else -> iptables split leaves dhcp_server with a check that can never succeed (iptables -C as non-root exits 4). Always using sudo is both shorter and correct. Inline on line 47.

The rest are non-blocking: exit-status handling in the two wait loops, the -C check keying on a comment label rather than on whether traffic is permitted, an unvalidated docker network inspect feeding SYSLOG_TARGET_IP, the FEATURE-state gate, and two cleanups in rsyslog-config.sh.

One item I could not anchor inline (outside the diff hunks)

docker_image_ctl.j2:767 still reads {%- if docker_container_name != "dhcp_server" %} around --net=$NET. Every other == "dhcp_server" test in the file was converted to in bridged_containers, but this one was not, so the two bridged containers get their bridge networking by two different mechanisms: dhcp_server is created with no --net flag at all (relying on docker's implicit default bridge), while redfish gets an explicit --net=bridge from line 674. "Bridged container" now means two different things in the same template, and whoever adds a third entry has to know this line exists.

Nikolay Mirin (nikamirrr) · 2026-09-17T17:05:50Z

+{%- if docker_container_name in bridged_containers %}
+{#- redfish.service runs as the sonicadmin user, so its rule check needs sudo. #}
+{%- set ipt = "sudo iptables" if docker_container_name == "redfish" else "iptables" %}
+BRIDGE_SYSLOG_CHECK=({{ ipt }} -C INPUT -i docker0 -p tcp -m tcp --dport 2514 -m comment --comment {{ docker_container_name }}_syslog -j ACCEPT)


Blocking: the rule this polls for does not exist yet, so redfish syslog still does not work after this merges.

I checked scripts/caclmgrd at both relevant pins of src/sonic-host-services — 409d3b2 (this PR's merge-base) and dfa3cc3 (current 202608 tip, i.e. what the merge result will carry). redfish_syslog occurs 0 times in either. The only docker0 RELP exception is gated solely on dhcp_server:

# dhcp_server docker0 syslog (RELP tcp/2514) exception; host namespace / IPv4 only if namespace == DEFAULT_NAMESPACE and self.DhcpServerSyslogAllowed: iptables_cmds.append(... '--comment', 'dhcp_server_syslog')

allow_redfish()/block_redfish() only flip RedfishAllowed, which is consumed at one place (if acl_service == "REDFISH" and not self.RedfishAllowed) to gate the CONFIG_DB REDFISH ACL service — it never emits a tcp/2514 rule.

To be clear, this is not a missing submodule bump in this PR: the merge-base already pins 409d3b2, so the diff is correct as-is. It is a merge-order dependency on Azure/sonic-host-services.msft#23 ("[202608] [caclmgrd] Own the redfish docker0 syslog INPUT exception"), which is still OPEN.

Consequences if this lands first:

Every redfish.sh start runs the -C check 11 times, burns the full 10 s in preStartAction, and logs Warning: redfish syslog rule not present after 10 seconds.

On aspeed platforms where dhcp_server is absent (init_cfg.json.j2 only adds FEATURE|dhcp_server under include_dhcp_server == y, while FEATURE|redfish is added for sonic_asic_platform == 'aspeed'), there is no docker0:2514 ACCEPT rule at all, so redfish's RELP traffic hits caclmgrd's trailing -A INPUT -j DROP and the logs never reach host /var/log/syslog — which is the goal of this change.

Please land #23 first (and make sure the submodule pin in the merged tree includes it), or merge the two together.

Yes, it's understood that #23 will go before this PR, the same way it was done for master.

Nikolay Mirin (nikamirrr) · 2026-09-17T17:05:50Z

-DHCP_SERVER_SYSLOG_CHECK=(iptables -C INPUT -i docker0 -p tcp -m tcp --dport 2514 -m comment --comment dhcp_server_syslog -j ACCEPT)
+{%- if docker_container_name in bridged_containers %}
+{#- redfish.service runs as the sonicadmin user, so its rule check needs sudo. #}
+{%- set ipt = "sudo iptables" if docker_container_name == "redfish" else "iptables" %}


dhcp_server also runs as sonicadmin, so the non-sudo branch can never succeed.

The comment above states the real invariant — the unit runs as the sonicadmin user — but encodes it as a name test. That property lives in *.service.j2, and files/build_templates/dhcp_server.service.j2 has it too:

11: [Service] 12: User={{ sonicadmin_user }}

So the else "iptables" branch is taken for the one other member of bridged_containers, and it is broken. iptables -C as a non-root user exits 4 (Could not fetch rule set generation id: Permission denied (you must be root)) — never 0, never 1. Result: every dhcp_server start loops all 11 iterations, sleeps 10 s and prints ... not present after 10 seconds (iptables status 4); every disable-triggered stop does the same with still present (iptables status 4).

Note this is pre-existing on dhcp_server rather than introduced here, but the line is being rewritten anyway, and the name-keyed conditional also re-introduces exactly the per-container special case the rest of the PR removes — any future entry appended to bridged_containers silently inherits the wrong command.

Suggest dropping the conditional entirely and always using sudo. sonicadmin is in %sudo ALL=(ALL:ALL) NOPASSWD: ALL (files/image_config/sudoers/sudoers), and sudo as root is a no-op:

Suggested change

{%- set ipt = "sudo iptables" if docker_container_name == "redfish" else "iptables" %}

BRIDGE_SYSLOG_CHECK=(sudo iptables -C INPUT -i docker0 -p tcp -m tcp --dport 2514 -m comment --comment {{ docker_container_name }}_syslog -j ACCEPT)

(and delete the {%- set ipt = ... %} line above)

I am owning redfish here and not dhcp_server. If dhcp_server has the issue, it should considered separately, not via this PR.

Nikolay Mirin (nikamirrr) · 2026-09-17T17:05:50Z

-    if [ "$($SONIC_DB_CLI CONFIG_DB HGET 'FEATURE|dhcp_server' state 2>/dev/null)" = "enabled" ]; then
+{%- elif docker_container_name in bridged_containers %}
+    # Wait for caclmgrd to allow {{ docker_container_name }} syslog before startup.
+    if [ "$($SONIC_DB_CLI CONFIG_DB HGET 'FEATURE|{{ docker_container_name }}' state 2>/dev/null)" = "enabled" ]; then


Gating on the FEATURE state field re-opens the race this PR exists to close.

The wait only happens when FEATURE|<name> state already reads enabled. systemctl start redfish issued manually, or during a config reload window before hostcfgd has written that field, sees some other value, skips the wait entirely, and starts the container before caclmgrd has installed the ACCEPT rule — the early RELP connections are DROPped and the container's startup FATALs are lost. That is the original symptom.

The stop-side gate on = "disabled" (line 927) has the mirror problem: if hostcfgd's state write and the systemctl stop are not strictly ordered, the wait is skipped.

Gating on the actual condition (rule present / absent) with a bounded timeout would be race-free; the FEATURE row is a separately-written proxy for it.

Nikolay Mirin (nikamirrr) · 2026-09-17T17:05:50Z

-        for attempt in {0..10}; do
-            "${DHCP_SERVER_SYSLOG_CHECK[@]}" 2>/dev/null
+        for (( attempt=0; attempt<=BRIDGE_SYSLOG_WAIT_SECS; attempt++ )); do
+            "${BRIDGE_SYSLOG_CHECK[@]}" 2>/dev/null


The -C check tests for a comment label, not for whether this container's syslog is actually permitted.

caclmgrd's per-feature rules are byte-identical except for the --comment string, and any one of them already admits every docker0 container's RELP traffic (the match is -i docker0 -p tcp --dport 2514, with no per-container selector).

So with dhcp_server enabled and redfish just enabled, redfish's syslog is already flowing, yet iptables -C ... --comment redfish_syslog returns 1 until caclmgrd reconverges — a pointless 10 s stall plus a false "rule not present" warning. Conversely, a stale redfish_syslog rule can be present while the traffic path is broken.

In practice this is a proxy for "caclmgrd has reconverged", not for the invariant the comment claims. Worth at least saying so in the comment, since the next reader will assume the check means what it says.

Nikolay Mirin (nikamirrr) · 2026-09-17T17:05:50Z

    # Default rsyslog target IP for single ASIC platform
-{%- if docker_container_name == "dhcp_server" %}
+{%- if docker_container_name in bridged_containers %}
    SYSLOG_TARGET_IP=$(docker network inspect bridge --format={{ "'{{(index .IPAM.Config 0).Gateway}}'" }})


Unvalidated command substitution silently disables log forwarding — and it is run twice.

Previously redfish got the constant SYSLOG_TARGET_IP=127.0.0.1, which can never be empty. Now it is the unchecked stdout of docker network inspect. If that yields nothing — bridge network recreated/renamed via a custom daemon.json, .IPAM.Config empty so Go's index errors out, or a transient daemon error — the substitution returns "" and the container is launched with --env "SYSLOG_TARGET_IP"=. Inside, rsyslog-container.conf.j2's action(type="omrelp" target=...) gets an empty target: rsyslog logs a config complaint and carries on, so logs are dropped with no host-visible error and no non-zero exit from <container>.sh start. There is no || exit, no emptiness check and no fallback.

Separately (lines 456-457, just outside this hunk): for members of bridged_containers the if [[ ($NUM_ASIC -gt 1) ]] branch re-runs the byte-identical command, so it is dead code here and costs a second round-trip to the docker daemon on the container start path. The preceding comment (# Default rsyslog target IP for single ASIC platform) is also now inaccurate, since this line applies to both single- and multi-ASIC. Collapsing to one unconditional assignment for bridged containers would fix both.

Nikolay Mirin (nikamirrr) · 2026-09-17T17:05:50Z

+            "${BRIDGE_SYSLOG_CHECK[@]}" 2>/dev/null
            status=$?

            if [ "$status" -eq 1 ]; then


Raw exit status is being used as a rule-presence verdict; 1 is ambiguous.

stop() breaks when status -eq 1. But sudo also returns 1 when it cannot escalate (or on any sudo-level failure), which is indistinguishable from iptables' 1 = "rule does not exist" — so a broken sudo silently reports success here.

Symmetrically, preStartAction (line 146) treats every non-zero status as "not installed yet" and retries for 10 s, so permanent errors (2 = parameter problem, 4 = permission denied) are retried instead of failing fast. With 2>/dev/null swallowing the diagnostic text, the only clue left is the bare number in the warning.

Suggest distinguishing 1 (genuinely absent → the expected outcome) from 2/3/4 (the check itself is broken → log the stderr and break immediately) rather than treating the status as a boolean.

Nikolay Mirin (nikamirrr) · 2026-09-17T17:05:50Z

-if [ $contain_dhcp_server ]; then
-    docker0_ip=$(ip -o -4 addr list docker0 | awk '{print $4}' | cut -d/ -f1)
-fi
+bridged_syslog_features="dhcp_server redfish"


The commit message describes this set as "a single list", but it is now three independent copies across two repos.

bridged_syslog_features="dhcp_server redfish" (here)

{%- set bridged_containers = ["dhcp_server", "redfish"] %} (docker_image_ctl.j2:2)

if key not in ("redfish", "dhcp_server") / the DhcpServerSyslogAllowed rule block in sonic-host-services/scripts/caclmgrd

Adding a fourth bridged container means three edits in two repos with no build-time or test-time link between them, and omitting any one produces a silent failure (no docker0 RELP listener, or no ACCEPT rule, or no startup wait) rather than an error. That is the same drift the refactor is meant to prevent. Deriving this list from one place — or at minimum a cross-reference comment in all three — would help.

Nikolay Mirin (nikamirrr) · 2026-09-17T17:05:51Z

-fi
+bridged_syslog_features="dhcp_server redfish"
+for feature in $bridged_syslog_features; do
+    if [ -n "$(sonic-db-cli CONFIG_DB keys "FEATURE|$feature")" ]; then


One sonic-db-cli process per feature on the boot path, where a single query answers the question.

Each iteration is a fresh process start plus a redis connect, and this runs in rsyslog-config.service during boot. keys already takes a glob, so the whole loop collapses to one invocation:

if sonic-db-cli CONFIG_DB keys "FEATURE|*" | grep -qE 'FEATURE\|(dhcp_server|redfish)$'; then docker0_ip=$(ip -o -4 addr list docker0 | awk '{print $4}' | cut -d/ -f1) fi

That also gets closer to the "single list" intent, since the alternation could be built from one variable.

Nikolay Mirin (nikamirrr)

Please check the comments if anything is worth acting on

shreyansh-nexthop marked this pull request as ready for review September 17, 2026 09:08

This was referenced Sep 17, 2026

[process_monitoring] Re-evaluate the redfish exclusion from #27355 once bridged-container syslog delivery is fixed sonic-net/sonic-mgmt#27724

Open

[caclmgrd] Own the redfish docker0 syslog INPUT exception sonic-net/sonic-host-services#429

Merged

Nikolay Mirin (nikamirrr) suggested changes Sep 17, 2026

View reviewed changes

Nikolay Mirin (nikamirrr) reviewed Sep 17, 2026

View reviewed changes

	{%- set ipt = "sudo iptables" if docker_container_name == "redfish" else "iptables" %}
	BRIDGE_SYSLOG_CHECK=(sudo iptables -C INPUT -i docker0 -p tcp -m tcp --dport 2514 -m comment --comment {{ docker_container_name }}_syslog -j ACCEPT)

Conversation

shreyansh-nexthop commented Sep 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why I did it

How I did it

How to verify it

Uh oh!

azure-pipelines Bot commented Sep 17, 2026

Uh oh!

mssonicbld commented Sep 17, 2026

Uh oh!

azure-pipelines Bot commented Sep 17, 2026

Uh oh!

azure-pipelines Bot commented Sep 17, 2026

Uh oh!

Nikolay Mirin (nikamirrr) left a comment

Choose a reason for hiding this comment

Port fidelity: good

One blocking issue, and it is about merge order

One correctness issue worth fixing in the port

One item I could not anchor inline (outside the diff hunks)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nikolay Mirin (nikamirrr) left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

shreyansh-nexthop commented Sep 17, 2026 •

edited

Loading