out_syslog: tls feature glitches for selecting protocols - #12196
Conversation
📝 WalkthroughWalkthroughThe syslog output now enables TLS automatically for ChangesSyslog secure mode handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SyslogConfig
participant SyslogOutput
participant TLSContext
participant TLSReceiver
SyslogConfig->>SyslogOutput: configures mode=tls
SyslogOutput->>TLSContext: creates client TLS context
SyslogOutput->>TLSReceiver: sends TLS syslog payload
TLSReceiver-->>SyslogOutput: accepts and reads payload
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
cosmo0920
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/integration/scenarios/out_syslog/tests/test_out_syslog_001.py (2)
309-336: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winWrap
service.start()in the try/finally block for cleanup safety.In
test_out_syslog_tls,service.start()runs before thetryblock. Ifservice.start()raises an unexpected error,service.stop()never runs, and the mock TLS receiver thread or a partially started Fluent Bit process is not cleaned up.test_out_syslog_tls_mode_requires_tlsabove already wraps itsservice.start()call in try/finally.Move
service.start()inside thetryblock for symmetry and safer cleanup on unexpected failures.🧹 Proposed fix
def test_out_syslog_tls(): service = Service("out_syslog_tls.yaml", "tls") - service.start() try: + service.start() payload = service.receiver.wait_message(timeout=20) finally: service.stop()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/scenarios/out_syslog/tests/test_out_syslog_001.py` around lines 309 - 336, Move service.start() inside the existing try/finally block in test_out_syslog_tls, keeping service.stop() guaranteed to run if startup or message reception fails.
114-152: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSet a timeout on the raw socket before the TLS handshake.
TlsReceiver._runaccepts a connection and wraps it withtls_context.wrap_socket(conn, server_side=True)without first callingconn.settimeout(...). The parent classTcpReceiver._runsetsconn.settimeout(20)right afteraccept(), butTlsReceiveronly sets a timeout ontls_connafter the handshake completes. In blocking mode, the TLS handshake performed insidewrap_socketcan hang indefinitely if the client never completes it.Set the timeout on
connbefore callingwrap_socket, so the handshake itself is bounded.🔒️ Proposed fix
self._ready.set() conn, _ = server.accept() + conn.settimeout(20) with tls_context.wrap_socket(conn, server_side=True) as tls_conn: tls_conn.settimeout(20)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/scenarios/out_syslog/tests/test_out_syslog_001.py` around lines 114 - 152, Update TlsReceiver._run to set the accepted raw conn socket timeout before calling tls_context.wrap_socket, using the existing 20-second timeout. Keep the existing tls_conn timeout and message-reading behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/integration/scenarios/out_syslog/tests/test_out_syslog_001.py`:
- Around line 309-336: Move service.start() inside the existing try/finally
block in test_out_syslog_tls, keeping service.stop() guaranteed to run if
startup or message reception fails.
- Around line 114-152: Update TlsReceiver._run to set the accepted raw conn
socket timeout before calling tls_context.wrap_socket, using the existing
20-second timeout. Keep the existing tls_conn timeout and message-reading
behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 72cc858f-953e-48e8-bcb4-2e13ceb393d8
📒 Files selected for processing (6)
plugins/out_syslog/syslog.cplugins/out_syslog/syslog_conf.ctests/integration/scenarios/out_syslog/config/out_syslog_tls.yamltests/integration/scenarios/out_syslog/config/out_syslog_tls_without_tls.yamltests/integration/scenarios/out_syslog/tests/test_out_syslog_001.pytests/runtime/out_syslog.c
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c0d0f54fd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Would be possible to make it auto enable? If the user needs mode tls , tls gets on automatically? |
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
6c0d0f5 to
ee6b805
Compare
|
Got it. I changed to enable TLS/DTLS automatically. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/integration/scenarios/out_syslog/tests/test_out_syslog_001.py (1)
113-151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared read loop instead of duplicating it in
TlsReceiver.
TlsReceiver._runrepeats the accept-and-chunk-read loop fromTcpReceiver._runalmost verbatim, differing only in the TLS wrapping step. Extract the chunk-reading logic (lines 79-89 inTcpReceiver._run, lines 134-144 here) into a shared helper that both classes call with the connected socket object.♻️ Proposed refactor to share the read loop
class TcpReceiver: ... + def _read_message(self, conn, timeout=20): + conn.settimeout(timeout) + chunks = [] + while True: + chunk = conn.recv(4096) + if not chunk: + break + chunks.append(chunk) + if b"\n" in chunk: + break + return b"".join(chunks) + def _run(self): try: with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as server: server.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) server.bind((self.host, self.port)) server.listen(1) server.settimeout(120) self._ready.set() conn, _ = server.accept() with conn: - conn.settimeout(20) - chunks = [] - - while True: - chunk = conn.recv(4096) - if not chunk: - break - chunks.append(chunk) - if b"\n" in chunk: - break - - self.message = b"".join(chunks) + self.message = self._read_message(conn) self._done.set() except Exception as exc: self.error = exc self._ready.set() self._done.set() class TlsReceiver(TcpReceiver): ... def _run(self): try: tls_context = ssl.SSLContext(ssl.PROTOCOL_TLS_SERVER) tls_context.load_cert_chain(certfile=self.cert_file, keyfile=self.key_file) with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as server: server.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) server.bind((self.host, self.port)) server.listen(1) server.settimeout(120) self._ready.set() conn, _ = server.accept() with tls_context.wrap_socket(conn, server_side=True) as tls_conn: - tls_conn.settimeout(20) - chunks = [] - - while True: - chunk = tls_conn.recv(4096) - if not chunk: - break - chunks.append(chunk) - if b"\n" in chunk: - break - - self.message = b"".join(chunks) + self.message = self._read_message(tls_conn) self._done.set() except Exception as exc: self.error = exc self._ready.set() self._done.set()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/scenarios/out_syslog/tests/test_out_syslog_001.py` around lines 113 - 151, Extract the repeated chunk-reading and message assembly logic from TcpReceiver._run and TlsReceiver._run into a shared helper that accepts a connected socket, then have both _run methods call it after establishing their respective connections. Preserve the existing newline-terminated read behavior, message assignment, and completion signaling while keeping TLS wrapping specific to TlsReceiver.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/integration/scenarios/out_syslog/tests/test_out_syslog_001.py`:
- Around line 113-151: Extract the repeated chunk-reading and message assembly
logic from TcpReceiver._run and TlsReceiver._run into a shared helper that
accepts a connected socket, then have both _run methods call it after
establishing their respective connections. Preserve the existing
newline-terminated read behavior, message assignment, and completion signaling
while keeping TLS wrapping specific to TlsReceiver.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: db47608e-d135-4285-a724-c07ffa3ac878
📒 Files selected for processing (6)
plugins/out_syslog/syslog.cplugins/out_syslog/syslog_conf.ctests/integration/scenarios/out_syslog/config/out_syslog_dtls.yamltests/integration/scenarios/out_syslog/config/out_syslog_tls.yamltests/integration/scenarios/out_syslog/tests/test_out_syslog_001.pytests/runtime/out_syslog.c
💤 Files with no reviewable changes (2)
- plugins/out_syslog/syslog_conf.c
- tests/integration/scenarios/out_syslog/config/out_syslog_dtls.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/integration/scenarios/out_syslog/config/out_syslog_tls.yaml
2 participants
Implemented the syslog TLS validation and integration coverage.
mode=tlsautomatically enables TLS over TCP.mode=dtlsautomatically enables DTLS over UDP.tcp/udpremain unchanged.mode=udpwithtls onremains rejected.Coverage:
tls on.The review’s commit-splitting requirement is addressed:
f32b9e05f— implementation9b35b8365— runtime testsee6b8058e— integration testsNo
AGENTS.mdchange is needed.The remote still has the previous three commits. Updating the PR requires a
--force-with-leasepush because the local history was rewritten; I have not pushed because force-pushing requires explicit authorization.Closes #12193.
Enter
[N/A]in the box, if an item is not applicable to your change.Testing
Before we can approve your change; please submit the following in a comment:
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
ok-package-testlabel to test for all targets (requires maintainer to do).Documentation
Backporting
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.
Summary by CodeRabbit
New Features
Bug Fixes
Tests