in_udp: Add capability to insert source IP when format none is set #11160
Conversation
WalkthroughConditionally include the remote UDP source address in NONE-format log records when Changes
Sequence Diagram(s)sequenceDiagram
participant Sender as UDP Sender
participant Conn as udp_conn (plugin)
participant Encoder as NONE Encoder
participant Out as Output
Sender->>Conn: send UDP packet
Conn->>Conn: parse packet into log_value
alt source_address_key configured
Conn->>Conn: flb_connection_get_remote_address()
Conn-->>Encoder: encode(log_value, source_address_key, source_address)
else not configured
Conn-->>Encoder: encode(log_value)
end
Encoder->>Out: emit encoded payload (JSON or NONE-format record)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (1)plugins/in_udp/udp_conn.c (1)
🔇 Additional comments (3)
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: Joost van den Broek <jvandenbroek@gmail.com>
jvandenbroek
force-pushed
the
source-address-udp-format-none
branch
from
e6e89c4 to
c560873
Compare
leonardo-albertovich
left a comment
leonardo-albertovich
left a comment
There was a problem hiding this comment.
I've left a few notes related to the test code but those are just information, it's good as is.
| ssize_t w_size; | ||
|
|
||
| char *buf = "message\n"; | ||
| size_t size = strlen(buf); |
There was a problem hiding this comment.
Please separate the definition of this variable from the assignment of it.
| int num; | ||
| ssize_t w_size; | ||
|
|
||
| char *buf = "message\n"; |
There was a problem hiding this comment.
Please declare this variable as a const char *
| ctx = test_ctx_create(&cb_data); | ||
| if (!TEST_CHECK(ctx != NULL)) { | ||
| TEST_MSG("test_ctx_create failed"); | ||
| exit(EXIT_FAILURE); |
There was a problem hiding this comment.
I could be wrong but I think failed test cases shouldn't exit but instead just return
| } | ||
|
|
||
| /* waiting to flush */ | ||
| flb_time_msleep(1500); |
There was a problem hiding this comment.
As a general rule, basing non timing related tests on timing is problematic, particularly when CI runs on nodes that are somewhat loaded as it tends to create false positives.
| struct sockaddr_in addr; | ||
| flb_sockfd_t fd; | ||
| int ret; | ||
| int num; |
There was a problem hiding this comment.
Using meaningful variable names is preferred to these generic names because even though this is a simple test case neglecting to do so leads to using these types of names in more complex (and meaningful) pieces of code which means someone who reads the code later on might have a harder time understanding it or even miss the point and end up introducing a bug.
2 participants
This adds support for Source_Address_Key when Format is set to none.
Addresses my earlier comment in this PR: #7673 (comment)
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:
Unfortunately Valgrind fails on my Manjaro system, which seems to be expected because Valgrind uses Arch's glibc. But I doubt these simple changes, which were already implemented for the json part, would cause any issues. Maybe someone could test this?
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
Initially added documentation which corresponds to fluent/fluent-bit-docs#1165
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