in_syslog: Add support for RFC 6587 octet-counting #11035
Conversation
cosmo0920
requested review from
edsiper,
fujimotos and
koleini
as code owners
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds configurable TCP framing to the syslog input via a new "format" option (string). Introduces framing constants, plugin fields (format_str, frame_type), per-connection framing state, dual parsing modes (newline or RFC 6587 octet-counting), and tests covering octet-counting scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant TCP as TCP Stream
participant Parser as syslog_prot_process
participant NL as Newline Path
participant OC as Octet-Counting Path
participant Output as Message Emitter
TCP->>Parser: Deliver data buffer
Parser->>Parser: Check flb_syslog.frame_type
alt frame_type == NEWLINE
Parser->>NL: Scan buffer for '\n' or '\0'
NL->>Output: Emit message (line)
NL->>Parser: Advance buffer by len + 1
else frame_type == OCTET_COUNTING
Parser->>OC: Parse leading digits until space -> len
OC->>OC: Set frame_expected_len & frame_have_len
OC->>OC: Ensure len bytes available
OC->>Output: Emit message (len bytes)
OC->>OC: Reset frame_expected_len & frame_have_len
OC->>OC: Optionally consume trailing '\n'
OC->>Parser: Advance buffer accordingly
end
Parser->>TCP: Await next data
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 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 |
Signed-off-by: Hiroshi Hatake <[email protected]>
Signed-off-by: Hiroshi Hatake <[email protected]>
cosmo0920
force-pushed
the
cosmo0920-handle-octet_counting
branch
from
0aa5256 to
cccd679
Compare
![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.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugins/in_syslog/syslog_prot.h(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (30)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 32bit, x86, x86-windows-static, 3.31.6)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit, x64, x64-windows-static, 3.31.6)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit (Arm64), amd64_arm64, -DCMAKE_SYSTEM_NAME=Windows -DCMA...
- GitHub Check: run-ubuntu-unit-tests (-DFLB_ARROW=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_COVERAGE=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, clang, clang++)
- GitHub Check: PR - fuzzing test
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-22.04, clang-12)
- GitHub Check: pr-compile-without-cxx (3.31.6)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-24.04, clang-14)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-22.04, clang-12)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-24.04, clang-14)
- GitHub Check: pr-compile-centos-7
Signed-off-by: Hiroshi Hatake <[email protected]>
cosmo0920
force-pushed
the
cosmo0920-handle-octet_counting
branch
from
d096aaa to
005d02d
Compare
Signed-off-by: Hiroshi Hatake <[email protected]>
The term of frame is a bit of low level concept for users. Signed-off-by: Hiroshi Hatake <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/runtime/in_syslog.c (1)
323-354: Consider simplifying the truncation logic.The defensive truncation code (lines 338-345) has a logic issue: if truncation occurs,
mlenisn't reduced and the header's length field isn't recalculated, which could lead to writingmlenbytes even when the buffer can only hold less. Since the comment acknowledges buffers should be adequate in tests, consider either removing the truncation entirely or adding an assertion instead.Example simplification:
- if (need + 1 > outsz) { - /* truncate conservatively if buffer too small (shouldn't happen in tests) */ - need = outsz - 1; - add_lf = 0; - if ((size_t)hlen > need) { - hlen = (int)need; - } - } + /* Buffers in tests should always be adequate */ + if (need + 1 > outsz) { + return 0; /* or assert */ + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
plugins/in_syslog/syslog.c(1 hunks)plugins/in_syslog/syslog.h(2 hunks)plugins/in_syslog/syslog_conf.c(1 hunks)tests/runtime/in_syslog.c(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/in_syslog/syslog.c
🧰 Additional context used
🧬 Code graph analysis (1)
tests/runtime/in_syslog.c (1)
src/flb_lib.c (2)
flb_input_set(300-330)flb_start(914-925)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (27)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 32bit, x86, x86-windows-static, 3.31.6)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit (Arm64), amd64_arm64, -DCMAKE_SYSTEM_NAME=Windows -DCMA...
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit, x64, x64-windows-static, 3.31.6)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_ARROW=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_COVERAGE=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, clang, clang++)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-24.04, clang-14)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, gcc, g++)
- GitHub Check: pr-compile-without-cxx (3.31.6)
- GitHub Check: pr-compile-centos-7
- GitHub Check: PR - fuzzing test
🔇 Additional comments (10)
plugins/in_syslog/syslog_conf.c (1)
110-121: LGTM! Frame type initialization is well-structured.The framing initialization correctly defaults to newline, handles both "octet_counting" and "octet" variants, and provides helpful feedback for invalid values.
plugins/in_syslog/syslog.h (2)
36-39: LGTM! Framing constants are well-defined.The constants use clear naming and the zero-value for NEWLINE provides a safe default.
74-76: LGTM! Struct fields are properly typed and positioned.tests/runtime/in_syslog.c (7)
308-321: LGTM! String helper is safe and correct.The function properly handles buffer bounds, null-termination, and edge cases.
356-365: LGTM! Frame concatenation is clean and correct.
1064-1124: LGTM! Basic octet counting test is properly structured.The test correctly configures the plugin with
format=octet_countingand validates the core framing functionality.
1127-1187: LGTM! Trailing LF test covers RFC 6587 optional trailer.This correctly validates that the implementation handles the optional line feed trailer permitted by RFC 6587 section 3.4.1.
1190-1266: LGTM! Fragmentation test validates crucial TCP stream handling.This test ensures the implementation correctly buffers partial frames and reassembles them, which is essential for reliable TCP-based syslog ingestion.
1269-1332: LGTM! Multi-frame test validates boundary detection.This test ensures back-to-back frames are correctly parsed and separated, which is critical for high-throughput scenarios.
1352-1355: LGTM! TEST_LIST properly updated.All four new tests are correctly registered and follow existing naming conventions.
2 participants
FIxes: #10910.
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:
[INPUT] Name syslog Mode tcp Listen 0.0.0.0 Port 5140 Frame octet_counting Parser syslog-rfc5424If 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
Tests