in_http: add fluentd like add_remote_addr functionality #11080
Conversation
aletourneau
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 two new in_http configuration options and struct fields to capture client IP; extracts X-Forwarded-For from requests and injects a REMOTE_ADDR field into MsgPack records for both legacy and NG (HTTP/2) flows; adds unit tests for map/array and collision cases. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant HTTPServer
participant Handler
participant HeaderHelper
participant PayloadProcessor
participant Output
Client->>HTTPServer: POST (body + optional X-Forwarded-For)
HTTPServer->>Handler: deliver request (ctx, version, request)
Handler->>HeaderHelper: get_remote_addr(request)
HeaderHelper-->>Handler: remote_addr (trimmed/sanitized) or NULL
Handler->>PayloadProcessor: parse_payload_*(payload, size, request)
PayloadProcessor->>PayloadProcessor: process_pack(..., request)
alt add_remote_addr enabled and remote_addr present
PayloadProcessor->>PayloadProcessor: append_remote_addr(record, remote_addr)
end
PayloadProcessor->>Output: emit MsgPack record(s) (may include REMOTE_ADDR)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/in_http/http_prot.c (1)
413-562: Critical: HTTP version mismatch causes undefined behavior—confirmed.Line 432 hardcodes
HTTP_PROTOCOL_VERSION_11when callingget_remote_addr, but HTTP/2.0 requests using urlencoded payloads reach this code. The HTTP/2.0 code path at line 1406 callsparse_payload_urlencoded, which then invokesprocess_pack(line 697) with the hardcoded HTTP/1.1 version. Therequestobject is astruct flb_http_request*(HTTP/2.0), butget_remote_addris called with version 11, causinghttp_header_lookupto incorrectly cast it asstruct mk_http_request*(HTTP/1.1).For comparison, JSON payloads have a separate
parse_payload_json_ngpath that correctly callsprocess_pack_ng(line 1191), which usesHTTP_PROTOCOL_VERSION_20. Urlencoded payloads lack this alternative, creating an asymmetry.Solution: Create
parse_payload_urlencoded_ngthat callsprocess_pack_ngwith the correct HTTP/2.0 version, or pass the HTTP version as a parameter toparse_payload_urlencoded.
🧹 Nitpick comments (1)
plugins/in_http/http.c (1)
221-221: Remove trailing whitespace.Line 221 contains only whitespace.
Apply this diff:
- +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
plugins/in_http/http.c(1 hunks)plugins/in_http/http.h(2 hunks)plugins/in_http/http_prot.c(14 hunks)tests/runtime/in_http.c(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/runtime/in_http.c (3)
src/flb_lib.c (3)
flb_input_set(300-330)flb_output_set(515-546)flb_start(914-925)src/flb_http_client.c (4)
flb_http_client(814-859)flb_http_add_header(963-995)flb_http_do(1572-1632)flb_http_client_destroy(1688-1695)src/flb_upstream.c (1)
flb_upstream_conn_release(862-947)
plugins/in_http/http_prot.c (4)
src/flb_http_common.c (1)
flb_http_request_get_header(230-257)src/flb_sds.c (3)
flb_sds_create_len(58-76)flb_sds_trim(148-202)flb_sds_destroy(389-399)include/fluent-bit/flb_mem.h (1)
flb_free(126-128)lib/cfl/src/cfl_sds.c (1)
cfl_sds_len(88-91)
🔇 Additional comments (13)
plugins/in_http/http.c (1)
210-220: LGTM! Configuration options are well-defined.The new configuration options for
add_remote_addrandremote_addr_keyare properly structured with appropriate types, defaults, and descriptions. The default oftrueforadd_remote_addrmakes the feature opt-out, which aligns with the PR objectives.plugins/in_http/http_prot.c (7)
218-283: Well-structured version-aware header lookup.The
http_header_lookupfunction properly abstracts header retrieval across HTTP/1.1 (Monkey parser) and HTTP/2.0 (NextGen server) backends. The case-insensitive comparison for HTTP/1.1 headers and the parameter validation are appropriate.
285-319: Remote address extraction logic is sound.The
get_remote_addrfunction correctly:
- Extracts the X-Forwarded-For header using the version-aware lookup
- Handles comma-separated proxy chains by taking only the first IP (the client)
- Trims whitespace from the result
- Properly manages memory by destroying on error paths
The empty-string check on line 313 is appropriate, as
flb_sds_trimreturns the final length (0 for empty, -1 for error).
321-361: MessagePack augmentation is correctly implemented.The
append_remote_addrfunction properly:
- Repacks the existing map with an additional entry
- Copies all existing key-value pairs
- Appends the REMOTE_ADDR field with the configurable key name
- Unpacks the result for the caller to use
484-492: Memory management for array elements is correct.The code properly handles the
appended_resultlifecycle:
- Skips destruction on first iteration (i=0) since nothing was allocated yet
- Destroys the previous iteration's result before creating a new one (i>0)
- The final iteration's result is cleaned up at lines 540-543
564-599: JSON payload parsing correctly propagates the request.The function signature now includes the
requestparameter, which is properly passed through toprocess_packfor remote address extraction.
1172-1322: NextGen processing uses the correct HTTP version.Unlike
process_pack, theprocess_pack_ngfunction correctly callsget_remote_addrwithHTTP_PROTOCOL_VERSION_20(line 1191), ensuring proper header parsing for HTTP/2.0 requests. The memory management and record augmentation logic mirrors the non-ng variant.
1324-1365: NextGen JSON parsing correctly uses process_pack_ng.The
parse_payload_json_ngfunction properly callsprocess_pack_ng(line 1361), maintaining separation between HTTP/1.1 and HTTP/2.0 processing paths.plugins/in_http/http.h (2)
35-35: LGTM! Macro definition for default remote address key.The
REMOTE_ADDR_KEYmacro provides a clean default value that can be referenced consistently across the codebase.
51-52: LGTM! Struct fields properly support the new feature.The
add_remote_addrandremote_addr_keyfields are correctly typed and positioned within theflb_httpstructure, matching the configuration options defined in http.c.tests/runtime/in_http.c (3)
674-764: Well-structured test helper for remote address validation.The
test_http_add_remote_addrfunction provides comprehensive validation:
- Configures add_remote_addr feature and HTTP version
- Sends HTTP POST with X-Forwarded-For header
- Validates that REMOTE_ADDR appears in the output with the expected IP
- Properly handles both NextGen (HTTP/2.0) and legacy (HTTP/1.1) server variants
797-800: Test list properly updated.The four new test cases are correctly registered in the TEST_LIST for execution.
767-786: Remove unsubstantiated "critical bug" claim; urlencoded test suggestion is valid.The four test cases shown correctly cover JSON payloads across both HTTP/2.0 and HTTP/1.1 paths with proper remote address extraction validation. However, the reference to "the critical bug identified in http_prot.c" cannot be substantiated—no bugs were found in the HTTP/2.0 remote address handling, which implements identical logic to HTTP/1.1. The suggestion to add urlencoded tests is valid: the HTTP input plugin supports urlencoded payloads (confirmed in http_prot.c lines 897-898 for HTTP/1.1 and 1384-1386 for HTTP/2.0), but no such tests currently exist in the suite.
aletourneau
force-pushed
the
in_http/add_remote_addr
branch
2 times, most recently
from
9924cf3 to
71c1a00
Compare
Also: Thank you for looking at this PR, and for the comments, it is appreciated. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
plugins/in_http/http_prot.c (2)
437-473: Fix memory leak: destroyappended_resultbetween outer loop iterations in MAP case.When processing multiple maps in the outer while loop (line 436),
appended_resultis reinitialized inappend_remote_addrwithout destroying the previous iteration's data. The ARRAY case correctly handles this on lines 485-487.Apply this diff to destroy the previous result before reuse:
if (result.data.type == MSGPACK_OBJECT_MAP) { obj = &result.data; if (remote_addr != NULL && strlen(remote_addr) > 0) { if (!appended_initialized) { /* doing this only once, since it can be cleared and reused */ msgpack_sbuffer_init(&appended_sbuf); appended_initialized = 1; } + else { + /* destroy previous unpacked result before reuse */ + msgpack_unpacked_destroy(&appended_result); + } /* if we fail to append, we just continue with the original object */ if (append_remote_addr(obj, &appended_result, &appended_sbuf, ctx, remote_addr) == 0) {
1196-1230: Fix memory leak: destroyappended_resultbetween outer loop iterations in MAP case.Same issue as in
process_pack: when processing multiple maps in the outer while loop (line 1195),appended_resultis reinitialized without destroying the previous iteration's data.Apply this diff:
if (result.data.type == MSGPACK_OBJECT_MAP) { obj = &result.data; if (remote_addr != NULL && strlen(remote_addr) > 0) { if (!appended_initialized) { /* doing this only once, since it can be cleared and reused */ msgpack_sbuffer_init(&appended_sbuf); appended_initialized = 1; } + else { + /* destroy previous unpacked result before reuse */ + msgpack_unpacked_destroy(&appended_result); + } if (append_remote_addr(obj, &appended_result, &appended_sbuf, ctx, remote_addr) == 0) {
🧹 Nitpick comments (3)
plugins/in_http/http_prot.c (3)
439-439: Consider usingflb_sds_lenfor consistency.Since
remote_addris anflb_sds_ttype, preferflb_sds_len(remote_addr)overstrlen(remote_addr)for consistency with the codebase and potential performance benefits (O(1) vs O(n)).Apply this diff:
- if (remote_addr != NULL && strlen(remote_addr) > 0) { + if (remote_addr != NULL && flb_sds_len(remote_addr) > 0) {And similarly on line 478:
- if (remote_addr != NULL && strlen(remote_addr) > 0) { + if (remote_addr != NULL && flb_sds_len(remote_addr) > 0) {Also applies to: 478-478
1198-1198: Consider usingflb_sds_lenfor consistency.Same as in
process_pack: preferflb_sds_len(remote_addr)overstrlen(remote_addr)for consistency and efficiency.Apply this diff on line 1198:
- if (remote_addr != NULL && strlen(remote_addr) > 0) { + if (remote_addr != NULL && flb_sds_len(remote_addr) > 0) {And on line 1237:
- if (remote_addr != NULL && strlen(remote_addr) > 0) { + if (remote_addr != NULL && flb_sds_len(remote_addr) > 0) {Also applies to: 1237-1237
1320-1320: Optional: Remove trailing whitespace.Line 1320 appears to have trailing whitespace. Consider removing it for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
plugins/in_http/http.c(1 hunks)plugins/in_http/http.h(2 hunks)plugins/in_http/http_prot.c(14 hunks)tests/runtime/in_http.c(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/runtime/in_http.c
🧰 Additional context used
🧬 Code graph analysis (1)
plugins/in_http/http_prot.c (4)
src/flb_http_common.c (1)
flb_http_request_get_header(230-257)src/flb_sds.c (3)
flb_sds_create_len(58-76)flb_sds_trim(148-202)flb_sds_destroy(389-399)include/fluent-bit/flb_mem.h (1)
flb_free(126-128)lib/cfl/src/cfl_sds.c (1)
cfl_sds_len(88-91)
🔇 Additional comments (7)
plugins/in_http/http.h (1)
35-35: LGTM! Clean additions for remote address configuration.The macro and struct fields are well-defined and properly positioned within the existing structure.
Also applies to: 51-52
plugins/in_http/http.c (1)
210-220: LGTM! Configuration entries are well-structured.The new options are properly defined with appropriate defaults and clear descriptions.
plugins/in_http/http_prot.c (5)
218-283: LGTM! Well-structured header lookup supporting both HTTP versions.The function correctly handles both HTTP/1.x (using mk_http_session parser) and HTTP/2.0 (using flb_http_request_get_header) with appropriate null checks.
476-522: LGTM! Array case correctly manages msgpack unpacked results.The cleanup logic properly destroys previous results when iterating (line 486) and the final result after the loop (line 541).
531-559: LGTM! Comprehensive resource cleanup on all exit paths.Both normal and error paths properly clean up
remote_addr,appended_result, andappended_sbuf.
564-599: LGTM! Request parameter correctly threaded through payload parsing.The request parameter is properly added to both parsing functions and passed to
process_pack.Also applies to: 601-716
1324-1365: LGTM! NextGen payload processing correctly integrates request parameter.The request parameter is properly threaded through the NextGen HTTP server path.
Also applies to: 1406-1406
aletourneau
force-pushed
the
in_http/add_remote_addr
branch
from
71c1a00 to
2227f64
Compare
|
aletourneau
force-pushed
the
in_http/add_remote_addr
branch
from
2227f64 to
e8d9673
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/runtime/in_http.c (1)
674-764: Clean test implementation with minor whitespace nit.The test helper function is well-structured and covers the essential scenarios. It properly validates that REMOTE_ADDR is injected with the expected IP from the X-Forwarded-For header.
Line 697 appears to have trailing whitespace after the semicolon. Consider removing it for consistency.
plugins/in_http/http_prot.c (2)
413-562: Address code style inconsistencies.The remote address augmentation logic is correct and memory management is solid. However, there are minor code style issues:
Apply this diff to fix spacing:
- + flb_sds_t remote_addr = NULL;- if(i > 0) { + if (i > 0) { msgpack_unpacked_destroy(&appended_result); } - if(append_remote_addr(&record, &appended_result, &appended_sbuf, ctx, remote_addr) == 0) { + if (append_remote_addr(&record, &appended_result, &appended_sbuf, ctx, remote_addr) == 0) { record = appended_result.data; }
1172-1322: Address code style inconsistencies.The NextGen remote address augmentation mirrors the legacy path correctly. However, the same code style issues appear:
Apply this diff to fix spacing:
- if (i > 0) { + if (i > 0) { msgpack_unpacked_destroy(&appended_result); } - if(append_remote_addr(&record, &appended_result, &appended_sbuf, ctx, remote_addr) == 0) { + if (append_remote_addr(&record, &appended_result, &appended_sbuf, ctx, remote_addr) == 0) { record = appended_result.data; }And remove trailing whitespace at line 1320:
- + return -1;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
plugins/in_http/http.c(1 hunks)plugins/in_http/http.h(2 hunks)plugins/in_http/http_prot.c(14 hunks)tests/runtime/in_http.c(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/in_http/http.c
🧰 Additional context used
🧬 Code graph analysis (2)
tests/runtime/in_http.c (3)
src/flb_lib.c (3)
flb_input_set(300-330)flb_output_set(515-546)flb_start(914-925)src/flb_http_client.c (4)
flb_http_client(814-859)flb_http_add_header(963-995)flb_http_do(1572-1632)flb_http_client_destroy(1688-1695)src/flb_upstream.c (1)
flb_upstream_conn_release(862-947)
plugins/in_http/http_prot.c (4)
src/flb_http_common.c (1)
flb_http_request_get_header(230-257)src/flb_sds.c (3)
flb_sds_create_len(58-76)flb_sds_trim(148-202)flb_sds_destroy(389-399)include/fluent-bit/flb_mem.h (1)
flb_free(126-128)lib/cfl/src/cfl_sds.c (1)
cfl_sds_len(88-91)
🔇 Additional comments (10)
plugins/in_http/http.h (2)
35-35: LGTM!The macro definition follows the existing pattern and provides a clear constant for the remote address field name.
51-52: LGTM!The struct fields are well-typed and integrate cleanly with the existing structure. The use of
const char *forremote_addr_keyis appropriate and matches the actual usage withstrlenin the implementation.tests/runtime/in_http.c (2)
767-786: LGTM!Excellent test coverage across all combinations: NextGen/Legacy HTTP server paths and Map/Array payload formats. The test wrappers properly validate that the first IP address in the X-Forwarded-For header list is extracted.
797-800: LGTM!The new tests are properly registered in the TEST_LIST with clear, descriptive names.
plugins/in_http/http_prot.c (6)
218-283: LGTM!The version-aware header lookup function correctly handles both HTTP/1.1 (Monkey parser) and HTTP/2.0 (flb_http_request) backends. Case-insensitive comparison is appropriate for HTTP headers, and error handling is sound.
285-319: LGTM!The function correctly extracts the first IP address from the X-Forwarded-For header. The trim validation check at line 313 has been properly fixed to handle both error cases (-1) and empty strings (0).
321-361: LGTM!The function properly augments msgpack map objects with the REMOTE_ADDR field. Line 347 now correctly uses
strlenfor theconst char *field, fixing the previous type cast issue.
564-599: LGTM!The payload parsing functions correctly thread the request context through to
process_pack, enabling remote address extraction without altering the existing payload processing logic.Also applies to: 601-716
947-947: LGTM!The process_payload function correctly passes the request context to the payload parsing functions.
Also applies to: 950-950
1406-1406: LGTM!The NextGen process_payload function correctly passes the request context to parse_payload_urlencoded.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/runtime/in_http.c (2)
691-695: Remove redundant TEST_CHECK call.Line 691 calls
TEST_CHECK(ret > 0)standalone, then line 692 checks the same condition in an if statement. The standalone call is redundant.Apply this diff:
- TEST_CHECK(ret > 0); if(!TEST_CHECK(ret > 0)) { TEST_MSG("snprintf failed"); exit(EXIT_FAILURE); }
697-697: Remove trailing whitespace.Apply this diff:
- cb_data.data = expected_buffer; - + cb_data.data = expected_buffer;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
plugins/in_http/http.c(1 hunks)plugins/in_http/http.h(2 hunks)plugins/in_http/http_prot.c(14 hunks)tests/runtime/in_http.c(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- plugins/in_http/http.h
- plugins/in_http/http.c
🧰 Additional context used
🧬 Code graph analysis (2)
tests/runtime/in_http.c (3)
src/flb_lib.c (3)
flb_input_set(300-330)flb_output_set(515-546)flb_start(914-925)src/flb_http_client.c (4)
flb_http_client(814-859)flb_http_add_header(963-995)flb_http_do(1572-1632)flb_http_client_destroy(1688-1695)src/flb_upstream.c (1)
flb_upstream_conn_release(862-947)
plugins/in_http/http_prot.c (4)
src/flb_http_common.c (1)
flb_http_request_get_header(230-257)src/flb_sds.c (3)
flb_sds_create_len(58-76)flb_sds_trim(148-202)flb_sds_destroy(389-399)include/fluent-bit/flb_mem.h (1)
flb_free(126-128)lib/cfl/src/cfl_sds.c (1)
cfl_sds_len(88-91)
🔇 Additional comments (6)
tests/runtime/in_http.c (1)
674-764: Test implementation looks solid.The test function properly validates the add_remote_addr functionality by:
- Configuring the input with
add_remote_addrenabled- Adding an X-Forwarded-For header with comma-separated IPs
- Verifying REMOTE_ADDR is correctly extracted and appended to records
- Testing both NextGen and legacy server paths with map and array inputs
plugins/in_http/http_prot.c (5)
218-283: LGTM: Header lookup implementation is correct.The function properly handles both HTTP/1.1 (using Monkey parser) and HTTP/2.0 (using flb_http_request_get_header) with appropriate null checks and case-insensitive comparisons.
321-361: LGTM: Remote address appending logic is correct.The function properly:
- Creates a new map with increased size
- Copies existing key-value pairs
- Appends the REMOTE_ADDR field using the configured key
- Correctly uses
strlenfor the const char* key
413-562: LGTM: Memory management is correct.The function properly handles resource cleanup:
- Initializes
appended_sbufferonly once when needed- For arrays, destroys the previous
appended_resultbefore creating a new one (line 485-487)- Cleans up all resources on both success and error paths
- The
appended_initializedflag ensures proper cleanup
1172-1322: LGTM: NextGen variant follows the same correct patterns.The NextGen implementation mirrors the legacy variant with proper:
- HTTP/2.0 protocol version for header lookup
- Memory management for appended buffers
- Resource cleanup on all exit paths
564-565: LGTM: Function signatures updated consistently.The request parameter is properly threaded through all payload parsing functions, enabling remote address extraction at the right points in the call chain.
Also applies to: 601-602
aletourneau
force-pushed
the
in_http/add_remote_addr
branch
from
e8d9673 to
5fa7f6d
Compare
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/runtime/in_http.c (1)
765-784: LGTM! Comprehensive test coverage for both server paths.The four test wrappers provide good coverage by exercising:
- NextGen HTTP/2 server vs. legacy Monkey server
- Map input (
{}) vs. array input ([{}])All tests consistently validate extraction of the first IP from the
X-Forwarded-Forheader.Consider adding optional edge-case tests for:
- Single IP in XFF (e.g.,
"1.2.3.4")- Missing or empty XFF header
- Malformed XFF values
These would strengthen robustness validation but are not essential for the initial implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
plugins/in_http/http.c(1 hunks)plugins/in_http/http.h(2 hunks)plugins/in_http/http_prot.c(14 hunks)tests/runtime/in_http.c(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/in_http/http_prot.c
🔇 Additional comments (4)
plugins/in_http/http.h (1)
35-35: LGTM! Clean implementation.The macro definition and struct field additions are well-designed and follow the existing code patterns. The use of a macro for the default key name ensures consistency across the codebase.
Also applies to: 51-52
plugins/in_http/http.c (1)
210-220: LGTM! Configuration options properly defined.The new configuration options follow the established pattern and integrate smoothly into the existing config_map. The default value of
falseforadd_remote_addrcorrectly matches Fluentd's behavior (opt-in), as noted in the PR objectives.tests/runtime/in_http.c (2)
674-762: LGTM! Well-structured test implementation.The test helper function properly validates the add_remote_addr feature by:
- Configuring the plugin with
add_remote_addr=true- Injecting an
X-Forwarded-Forheader- Verifying the expected IP appears in the
REMOTE_ADDRfieldThe implementation follows the existing test patterns and includes appropriate error checking.
795-798: LGTM! Test registration is correct.The four new test cases are properly registered in the TEST_LIST with descriptive names that clearly indicate what each test validates.
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 (4)
plugins/in_http/http.c(1 hunks)plugins/in_http/http.h(2 hunks)plugins/in_http/http_prot.c(15 hunks)tests/runtime/in_http.c(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- plugins/in_http/http.h
- plugins/in_http/http.c
- tests/runtime/in_http.c
🧰 Additional context used
🧬 Code graph analysis (1)
plugins/in_http/http_prot.c (3)
src/flb_sds.c (3)
flb_sds_create_len(58-76)flb_sds_trim(148-202)flb_sds_destroy(389-399)include/fluent-bit/flb_mem.h (1)
flb_free(126-128)lib/cfl/src/cfl_sds.c (1)
cfl_sds_len(88-91)
🔇 Additional comments (8)
plugins/in_http/http_prot.c (8)
42-45: LGTM!Forward declarations are correctly placed to allow functions to reference each other before their definitions.
223-258: LGTM!The X-Forwarded-For extraction logic correctly handles:
- Header lookup and SDS creation
- Comma-separated list truncation with proper SDS length update (line 248)
- Whitespace trimming with appropriate error and empty-string handling (line 252)
Based on learnings from past reviews, this addresses previously identified issues.
260-300: LGTM!The msgpack augmentation logic correctly:
- Allocates a new map with space for the additional REMOTE_ADDR field
- Uses
strlenfor theconst char *key (line 286), properly addressing the type safety issue from past reviews- Repacks and validates the augmented record
352-501: LGTM!The HTTP/1.x processing path correctly:
- Extracts remote_addr when the feature is enabled (lines 370-372)
- Destroys previous
appended_resultbefore repacking subsequent maps (lines 383-385), preventing the memory leak identified in past reviews- Type-guards array elements (line 419) to only augment map records, addressing previous safety concerns
- Cleans up all allocated resources on both success and error paths
503-538: LGTM!Parameter threading to propagate the request context is straightforward and correct.
540-660: LGTM!The urlencoded parser now correctly routes to the protocol-appropriate packer:
process_pack_ngfor HTTP/2 (line 637)process_packfor HTTP/1.x (line 640)This addresses the protocol mismatch issue identified in past reviews.
958-961: LGTM!Request context is correctly propagated to the JSON and urlencoded parsers.
1372-1417: LGTM!Request context is correctly propagated through the HTTP/2 processing path.
aletourneau
force-pushed
the
in_http/add_remote_addr
branch
2 times, most recently
from
6e19317 to
273a860
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/runtime/in_http.c (1)
689-694: Strengthen the snprintf truncation check.The current check only verifies
ret > 0, but doesn't detect truncation. Whensnprintftruncates, it returns a value ≥ the buffer size.Apply this diff to detect truncation:
- if(!TEST_CHECK(ret > 0)) { + if(!TEST_CHECK(ret > 0 && ret < sizeof(expected_buffer))) { TEST_MSG("snprintf failed"); exit(EXIT_FAILURE); }plugins/in_http/http_prot.c (1)
42-43: Fix forward declaration to match actual definition.The forward declaration on line 42 is missing the
statickeyword, but the actual function definition at line 668 is declaredstatic.Apply this diff:
-static int http_header_lookup(int version, void *ptr, char *key, +static int http_header_lookup(int version, void *ptr, char *key, char **val, size_t *val_len);Wait, looking again, line 42 already has
static. Let me re-check... Actually, looking at the annotated code, line 42 shows:static int http_header_lookup(int version, void *ptr, char *key,So it does have
static. My mistake - this is fine.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
plugins/in_http/http.c(1 hunks)plugins/in_http/http.h(2 hunks)plugins/in_http/http_prot.c(15 hunks)tests/runtime/in_http.c(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
plugins/in_http/http_prot.c (3)
src/flb_sds.c (3)
flb_sds_create_len(58-76)flb_sds_trim(148-202)flb_sds_destroy(389-399)include/fluent-bit/flb_mem.h (1)
flb_free(126-128)lib/cfl/src/cfl_sds.c (1)
cfl_sds_len(88-91)
tests/runtime/in_http.c (3)
src/flb_lib.c (3)
flb_input_set(300-330)flb_output_set(515-546)flb_start(914-925)src/flb_http_client.c (4)
flb_http_client(814-859)flb_http_add_header(963-995)flb_http_do(1572-1632)flb_http_client_destroy(1688-1695)src/flb_upstream.c (1)
flb_upstream_conn_release(862-947)
🔇 Additional comments (8)
plugins/in_http/http.h (1)
35-35: LGTM! Clean API additions for remote address feature.The new
REMOTE_ADDR_KEYmacro and struct fields (add_remote_addr,remote_addr_key) are well-named and align with the PR objectives.Also applies to: 51-52
plugins/in_http/http.c (1)
210-220: LGTM! Configuration options properly defined.The new configuration entries follow the existing pattern and match the PR objectives. The default of
falseforadd_remote_addraligns with Fluentd's behavior as noted in the PR summary.tests/runtime/in_http.c (1)
765-798: Excellent test coverage for the new feature.The tests cover all combinations of payload format (map/array) and server type (ng/legacy), and properly validate that
REMOTE_ADDRis extracted from theX-Forwarded-Forheader.plugins/in_http/http_prot.c (5)
223-258: LGTM! X-Forwarded-For extraction is robust.The function correctly extracts the client IP from the X-Forwarded-For header, handles comma-separated lists (taking the first IP), trims whitespace, and properly manages the SDS lifecycle. Past review concerns have been addressed.
260-300: LGTM! Remote address injection is correctly implemented.The function properly augments msgpack map objects with the REMOTE_ADDR field. All callers verify the object type is a map before calling, and the use of
strlen()for the const char* key is correct (addressing past review feedback).
352-501: LGTM! HTTP/1.x processing with remote address augmentation is solid.The updated
process_packfunction correctly:
- Extracts the remote address when enabled
- Guards remote address injection with map type checks (line 376 for top-level, line 419 for array elements)
- Properly destroys the previous msgpack zone between iterations (lines 383-385, 424-426) to prevent memory leaks
- Cleans up resources on all exit paths
All concerns from past reviews have been addressed.
636-641: LGTM! URL-encoded payload routing is now correct.The conditional routing to
process_pack_ngvsprocess_packbased on theenable_http2flag addresses the past review concern about HTTP/2 requests with URL-encoded payloads being sent to the wrong handler.
1183-1334: LGTM! HTTP/2 processing mirrors HTTP/1.x with proper version handling.The
process_pack_ngupdates correctly:
- Use
HTTP_PROTOCOL_VERSION_20when extracting the remote address (line 1202)- Apply the same type guards and zone cleanup pattern as
process_pack- Clean up resources on all exit paths
The parallel structure between HTTP/1.x and HTTP/2 paths makes the code maintainable.
aletourneau
force-pushed
the
in_http/add_remote_addr
branch
from
273a860 to
70ed660
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
plugins/in_http/http_prot.c (1)
352-501: Excellent memory management and type safety.The implementation correctly addresses the critical issues from previous reviews:
- Zone memory is properly destroyed between map iterations (lines 383-385, 424-426), preventing the leak
- Array elements are type-checked (
record.type == MSGPACK_OBJECT_MAP) before attempting augmentation (line 419), preventing crashes on scalar arrays- Cleanup paths comprehensively handle all allocated resources
Optional style fix: Line 484 is missing a space after
if:- if(remote_addr) { + if (remote_addr) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
plugins/in_http/http.c(1 hunks)plugins/in_http/http.h(2 hunks)plugins/in_http/http_prot.c(15 hunks)tests/runtime/in_http.c(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- plugins/in_http/http.h
- tests/runtime/in_http.c
- plugins/in_http/http.c
🧰 Additional context used
🧬 Code graph analysis (1)
plugins/in_http/http_prot.c (2)
src/flb_sds.c (3)
flb_sds_create_len(58-76)flb_sds_trim(148-202)flb_sds_destroy(389-399)include/fluent-bit/flb_mem.h (1)
flb_free(126-128)
🔇 Additional comments (6)
plugins/in_http/http_prot.c (6)
42-45: LGTM - Forward declarations enable proper call ordering.The forward declarations are necessary and well-placed, allowing
get_remote_addrandparse_payload_urlencodedto reference these functions before their definitions appear later in the file.
223-258: Well-implemented client IP extraction.The function correctly handles X-Forwarded-For parsing with proper SDS length management after truncation (line 248) and appropriate error handling. The trim validation at line 252 properly rejects both errors (-1) and empty strings (0).
636-641: Critical fix: HTTP/2 urlencoded payloads now use correct packer.This properly routes HTTP/2 urlencoded requests through
process_pack_nginstead of incorrectly using the HTTP/1.1 packer, which would have caused struct misinterpretation and potential crashes whenadd_remote_addris enabled. This addresses a critical issue flagged in previous reviews.
1183-1335: HTTP/2 path correctly mirrors HTTP/1.1 implementation.The
process_pack_ngfunction applies the same memory management and type-safety patterns asprocess_pack:
- Proper zone destruction before reuse (lines 1214-1215, 1258-1259)
- Type guards for array elements (line 1252)
- Comprehensive cleanup in both success and error paths
The HTTP_PROTOCOL_VERSION_20 constant at line 1202 correctly identifies this as the HTTP/2 path for header extraction.
503-538: Request context properly threaded through JSON parsing paths.The signature changes to
parse_payload_jsonandparse_payload_json_ngcorrectly forward the request parameter to the processing functions, enabling remote address extraction for JSON payloads in both HTTP/1.1 and HTTP/2 paths.Also applies to: 1337-1378
958-962: Complete integration of request context.All payload processing entry points (
process_payloadfor HTTP/1.1,process_payload_ngfor HTTP/2) correctly pass the request parameter through to the parsing functions, ensuring the remote address feature works consistently across content types and protocol versions.Also applies to: 1419-1419
aletourneau
force-pushed
the
in_http/add_remote_addr
branch
from
70ed660 to
86d032b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
plugins/in_http/http_prot.c (2)
388-402: Consider removing redundant msgpack_unpacked_init.Line 398 calls
msgpack_unpacked_init(&appended_result), butappend_remote_addralready initializesappended_resultinternally at line 303. While harmless (msgpack_unpacked_init is idempotent), removing the redundant call would make the intent clearer.Apply this diff to remove the redundant initialization:
} else if (appended_result.zone != NULL) { msgpack_unpacked_destroy(&appended_result); } - /* if we fail to append, we just continue with the original object */ - msgpack_unpacked_init(&appended_result); + /* if we fail to append, we just continue with the original object */ if (append_remote_addr(obj, &appended_result, &appended_sbuf, ctx, remote_addr) == 0) { obj = &appended_result.data; }
1223-1237: Consider removing redundant msgpack_unpacked_init in HTTP/2 path.Similar to the HTTP/1.1 path, line 1233 redundantly initializes
appended_resultbefore callingappend_remote_addr, which performs its own initialization (line 303). While safe, removing this improves clarity.Apply this diff:
} else if (appended_result.zone != NULL) { msgpack_unpacked_destroy(&appended_result); } - /* if we fail to append, we just continue with the original object */ - msgpack_unpacked_init(&appended_result); + /* if we fail to append, we just continue with the original object */ if (append_remote_addr(obj, &appended_result, &appended_sbuf, ctx, remote_addr) == 0) { obj = &appended_result.data; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
plugins/in_http/http.c(1 hunks)plugins/in_http/http.h(2 hunks)plugins/in_http/http_prot.c(15 hunks)tests/runtime/in_http.c(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/in_http/http.h
🧰 Additional context used
🧬 Code graph analysis (2)
plugins/in_http/http_prot.c (3)
src/flb_sds.c (3)
flb_sds_create_len(58-76)flb_sds_trim(148-202)flb_sds_destroy(389-399)include/fluent-bit/flb_mem.h (1)
flb_free(126-128)lib/cfl/src/cfl_sds.c (1)
cfl_sds_len(88-91)
tests/runtime/in_http.c (3)
src/flb_lib.c (3)
flb_input_set(300-330)flb_output_set(515-546)flb_start(914-925)src/flb_http_client.c (4)
flb_http_client(814-859)flb_http_add_header(963-995)flb_http_do(1572-1632)flb_http_client_destroy(1688-1695)src/flb_upstream.c (1)
flb_upstream_conn_release(862-947)
🔇 Additional comments (9)
plugins/in_http/http_prot.c (6)
223-258: LGTM: Robust X-Forwarded-For extraction.The implementation correctly extracts the first IP from the X-Forwarded-For header, handles comma-separated lists, updates SDS length after truncation, trims whitespace, and cleans up on error paths.
260-310: LGTM: Thoughtful deduplication prevents key collisions.The function correctly checks for pre-existing REMOTE_ADDR keys (lines 272-282) before injection, preventing duplicate keys in the msgpack map. The msgpack repacking logic properly creates a new map with size+1 and copies all existing entries.
430-445: LGTM: Proper type guard for array elements.Line 430 correctly verifies
record.type == MSGPACK_OBJECT_MAPbefore attempting remote_addr augmentation, preventing invalid memory access when arrays contain non-map elements (e.g., scalars or nested arrays).
484-512: LGTM: Consistent cleanup in all exit paths.Remote_addr and appended buffers are properly destroyed in both normal (lines 493-500) and error (lines 506-512) paths, preventing resource leaks.
650-655: LGTM: Correct routing for URL-encoded payloads.The HTTP/2 path now correctly routes to
process_pack_ng(line 651) instead ofprocess_pack, ensuring the request structure is interpreted with the correct protocol version.
1267-1282: LGTM: HTTP/2 array handling mirrors HTTP/1.1.The type guard (line 1267) and memory management pattern match the HTTP/1.1 implementation, ensuring consistent behavior across protocol versions.
plugins/in_http/http.c (1)
210-220: LGTM: Well-documented configuration options.The two new config options follow existing patterns and provide clear descriptions. The conservative default of
falseforadd_remote_addrensures backward compatibility, and the configurableremote_addr_keyallows customization when needed.tests/runtime/in_http.c (2)
674-762: LGTM: Comprehensive test coverage for remote_addr feature.The test helper correctly:
- Configures
add_remote_addr=true(line 704)- Injects X-Forwarded-For header with comma-separated IPs (lines 733-734)
- Validates that the first IP is extracted and added to the record (line 689)
- Supports both HTTP/2 ("true") and legacy ("false") server modes (line 705)
The test structure follows existing patterns and properly cleans up resources.
765-790: LGTM: Test suite exercises key scenarios.The five test cases provide solid coverage:
- skip_colliding_ng: Verifies existing REMOTE_ADDR keys are preserved (line 767)
- map_ng / array_ng: Validates HTTP/2 path for both input types
- map / array: Validates legacy HTTP/1.1 path for both input types
This ensures the feature works across protocol versions and payload structures.
aletourneau
force-pushed
the
in_http/add_remote_addr
branch
from
86d032b to
a39f543
Compare
aletourneau
force-pushed
the
in_http/add_remote_addr
branch
from
a39f543 to
1ccd9ff
Compare
Signed-off-by: Alexandre Létourneau <[email protected]>
aletourneau
force-pushed
the
in_http/add_remote_addr
branch
from
1ccd9ff to
8ac7270
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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, like "@codex address that feedback". |
|
3 participants
Fixes #10392
This patch is adding a new option
add_remote_addrwhich appends a REMOTE_ADDR field to the record. The value of REMOTE_ADDR field is the client's IP address. The client's IP address is extracted from the X-Forwarded-For request header (as it was in fluentd)For example:
The value of
REMOTE_ADDRwould behost1Known Issues
If multiple X-Forwarded-For headers are specified in the request, which is used (last, or first) will depend on which http server is used (mk or ng):
http2: false: The first XFF header will be usedhttp2: true: The last XFF header will be usedEnter
[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
Doc PR: fluent/fluent-bit-docs#2130
Backporting
Configuration
Debug / Valgrind logs
curl command
"REMOTE_ADDR"=>"1.2.3.4"is appended[0] http.0: [[1761687337.855967665, {}], {"k"=>"v", "REMOTE_ADDR"=>"1.2.3.4"}]Valgrind output for tests
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