routing: add new routing mechanism for input plugins #11041
Conversation
Signed-off-by: Eduardo Silva <[email protected]>
Signed-off-by: Eduardo Silva <[email protected]>
Signed-off-by: Eduardo Silva <[email protected]>
Signed-off-by: Eduardo Silva <[email protected]>
Signed-off-by: Eduardo Silva <[email protected]>
Signed-off-by: Eduardo Silva <[email protected]>
Signed-off-by: Eduardo Silva <[email protected]>
Signed-off-by: Eduardo Silva <[email protected]>
Signed-off-by: Eduardo Silva <[email protected]>
|
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 a routing subsystem: new router public headers and structs, runtime router parsing/apply/cleanup, condition-evaluation scaffolding, config-format changes to accept CFL variants for section properties, integration into config load/exit, unit tests and YAML fixtures. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CF as Config Format (CF)
participant Loader as flb_config
participant Parser as flb_router_config
participant Router as flb_router_apply_config
participant Runtime as Runtime I/O (inputs/outputs)
CF->>Loader: produce parsed CF (routes/processors as variants)
Loader->>Parser: flb_router_config_parse(cf, &input_routes, config)
Parser->>Parser: build input_routes -> routes -> processors/outputs
Parser-->>Loader: populate config->input_routes
Loader->>Runtime: perform normal plugin IO setup
alt config->input_routes non-empty
Loader->>Router: flb_router_apply_config(config)
Router->>Runtime: locate inputs/outputs, validate signals, attach processors, create connections
Router-->>Loader: success / failure
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–75 minutes Suggested reviewers
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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 |
![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
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".
| if (cfl_list_is_empty(&route->condition->rules) == 0) { | ||
| return FLB_TRUE; |
There was a problem hiding this comment.
When a route carries any rules, flb_route_condition_eval immediately returns FLB_TRUE because cfl_list_is_empty(&route->condition->rules) == 0 triggers the early return before any rule evaluation occurs. This means every route that defines conditions will match all chunks regardless of the configured predicates, defeating the purpose of conditional routing and potentially forwarding data to unintended outputs. The check should call the signal-specific evaluators rather than skipping them.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
src/config_format/flb_config_format.c (1)
328-364: Clarify ownership contract for variant insertionBehavior: on success, kvlist owns variant; on error, caller retains it. Add a brief comment/doc to prevent double-free or leaks by callers.
struct cfl_variant *flb_cf_section_property_add_variant(struct flb_cf *cf, @@ - rc = cfl_kvlist_insert(kv_list, key, variant); + /* On success, ownership of 'variant' is transferred to 'kv_list'. + * On failure, the caller retains ownership. */ + rc = cfl_kvlist_insert(kv_list, key, variant);src/flb_config.c (1)
1073-1079: Promote router parse failure to errorFailure aborts config load; log as error for visibility.
- if (ret == -1) { - flb_debug("[router] router configuration parsing failed"); - return -1; - } + if (ret == -1) { + flb_error("[router] router configuration parsing failed"); + return -1; + }tests/internal/router_config.c (1)
199-202: Optional: add a test for “processors” property handlingAdd a YAML or in-memory CF with an input ‘processors’ block to ensure configure_plugins_type doesn’t fail and processors load as expected (group or variant path).
Also applies to: 241-251
src/flb_router_config.c (3)
236-247: Normalize “any” to avoid ambiguous masks.If “any” appears with other tokens (e.g., “any,logs”), parse_signal_key returns a mixed mask. After the previous fix, “any” semantics remain unclear. Normalize by short-circuiting to ANY.
Suggested change:
@@ - else if (len == 3 && strncasecmp(start, "any", len) == 0) { - mask |= FLB_ROUTER_SIGNAL_ANY; - } + else if (len == 3 && strncasecmp(start, "any", len) == 0) { + mask = FLB_ROUTER_SIGNAL_ANY; + /* ignore any further tokens */ + break; + }
39-46: Defensive: guard size_t→int truncation in copy_from_cfl_sds().cfl_sds_len() returns size_t; flb_sds_create_len() takes int. Add bounds check to avoid truncation on very large inputs.
@@ static flb_sds_t copy_from_cfl_sds(cfl_sds_t value) { + size_t slen; if (!value) { return NULL; } - - return flb_sds_create_len(value, cfl_sds_len(value)); + slen = cfl_sds_len(value); + if (slen > INT_MAX) { + return NULL; + } + return flb_sds_create_len(value, (int) slen); }
759-766: Improve diagnostics for parse failures.These branches return -1 without context. Emitting flb_error/flb_warn helps users pinpoint bad config (section, key, reason).
Examples:
- When signals == 0: log the offending route name and signal key.
- When missing/invalid “name” fields: log the section path and expected type.
Also applies to: 920-924
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
include/fluent-bit/config_format/flb_cf.h(1 hunks)include/fluent-bit/flb_config.h(2 hunks)include/fluent-bit/flb_router.h(4 hunks)src/CMakeLists.txt(1 hunks)src/config_format/flb_cf_yaml.c(4 hunks)src/config_format/flb_config_format.c(1 hunks)src/flb_config.c(6 hunks)src/flb_router.c(1 hunks)src/flb_router_condition.c(1 hunks)src/flb_router_config.c(1 hunks)tests/internal/CMakeLists.txt(1 hunks)tests/internal/data/config_format/yaml/routing/basic.yaml(1 hunks)tests/internal/data/config_format/yaml/routing/metrics.yaml(1 hunks)tests/internal/data/config_format/yaml/routing/multi_signal.yaml(1 hunks)tests/internal/data/config_format/yaml/routing/precedence.yaml(1 hunks)tests/internal/router_config.c(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
include/fluent-bit/config_format/flb_cf.h (1)
src/config_format/flb_config_format.c (1)
flb_cf_section_property_add_variant(328-364)
src/flb_router_config.c (8)
src/flb_sds.c (3)
flb_sds_create_len(58-76)flb_sds_create(78-90)flb_sds_destroy(389-399)lib/cfl/src/cfl_sds.c (1)
cfl_sds_len(88-91)include/fluent-bit/flb_mem.h (2)
flb_free(126-128)flb_calloc(84-96)lib/cfl/include/cfl/cfl_array.h (2)
cfl_array_size(46-49)cfl_array_fetch_by_index(36-44)lib/cfl/src/cfl_kvlist.c (1)
cfl_kvlist_fetch(418-421)src/flb_output.c (1)
flb_output_name(1087-1094)src/flb_router.c (1)
flb_router_connect_direct(163-178)src/flb_input.c (1)
flb_input_name(790-797)
src/config_format/flb_cf_yaml.c (4)
lib/cfl/include/cfl/cfl_array.h (1)
cfl_array_fetch_by_index(36-44)src/config_format/flb_config_format.c (2)
flb_cf_section_property_add_list(366-411)flb_cf_section_property_add_variant(328-364)lib/cfl/src/cfl_sds.c (1)
cfl_sds_len(88-91)lib/cfl/src/cfl_array.c (1)
cfl_array_append_string(167-185)
src/flb_config.c (1)
src/flb_router_config.c (2)
flb_router_routes_destroy(379-394)flb_router_config_parse(992-1021)
src/config_format/flb_config_format.c (2)
src/flb_sds.c (2)
flb_sds_trim(148-202)flb_sds_destroy(389-399)lib/cfl/src/cfl_kvlist.c (1)
cfl_kvlist_insert(412-416)
tests/internal/router_config.c (11)
lib/cfl/src/cfl_array.c (3)
cfl_array_create(24-49)cfl_array_destroy(51-69)cfl_array_append(117-165)lib/cfl/include/cfl/cfl_array.h (2)
cfl_array_size(46-49)cfl_array_fetch_by_index(36-44)lib/cfl/src/cfl_variant.c (6)
cfl_variant_destroy(265-285)cfl_variant_create_from_string_s(90-115)cfl_variant_create_from_bytes(122-146)cfl_variant_size_get(292-295)cfl_variant_create_from_array(212-223)cfl_variant_create_from_kvlist(225-236)lib/cfl/src/cfl_kvlist.c (7)
cfl_kvlist_create(26-38)cfl_kvlist_destroy(40-61)cfl_kvlist_insert_s(291-318)cfl_kvlist_insert_string(341-355)cfl_kvlist_insert_array(394-398)cfl_kvlist_insert_kvlist(406-410)cfl_kvlist_insert_bool(370-374)lib/cfl/src/cfl_sds.c (1)
cfl_sds_len(88-91)src/config_format/flb_config_format.c (3)
flb_cf_create(104-149)flb_cf_destroy(151-157)flb_cf_section_create(640-722)src/config_format/flb_cf_yaml.c (1)
flb_cf_yaml_create(2993-3029)src/flb_router_config.c (3)
flb_router_config_parse(992-1021)flb_router_routes_destroy(379-394)flb_router_apply_config(1135-1224)src/flb_sds.c (2)
flb_sds_create(78-90)flb_sds_destroy(389-399)src/flb_router.c (2)
flb_router_exit(271-298)flb_router_match(139-143)src/flb_router_condition.c (1)
flb_route_condition_eval(79-122)
src/flb_router.c (1)
src/flb_router_config.c (1)
flb_router_apply_config(1135-1224)
include/fluent-bit/flb_router.h (2)
src/flb_router_condition.c (5)
flb_router_signal_from_chunk(24-42)flb_route_condition_eval(79-122)flb_condition_eval_logs(44-59)flb_condition_eval_metrics(61-68)flb_condition_eval_traces(70-77)src/flb_router_config.c (3)
flb_router_config_parse(992-1021)flb_router_routes_destroy(379-394)flb_router_apply_config(1135-1224)
⏰ 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 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: pr-windows-build / call-build-windows-package (Windows 32bit, x86, x86-windows-static, 3.31.6)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_ARROW=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_SANITIZE_THREAD=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_SIMD=On, 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, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 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_SANITIZE_MEMORY=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=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_SMALL=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_JEMALLOC=Off, 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_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, 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-without-cxx (3.31.6)
- 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-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-24.04, clang-14)
- GitHub Check: pr-compile-centos-7
🔇 Additional comments (22)
include/fluent-bit/config_format/flb_cf.h (1)
177-180: LGTM! Clean API addition for variant-based properties.The new
flb_cf_section_property_add_variantfunction complements the existing property-add API by accepting pre-constructed variants, enabling the router configuration flow to leverage variant-based routing semantics.tests/internal/CMakeLists.txt (1)
16-16: LGTM! Test wiring for router configuration.Adding
router_config.cto the internal test suite enables testing of the new router configuration parsing and application logic.src/CMakeLists.txt (1)
57-58: LGTM! Core router modules added to build.The new router condition and configuration modules are properly integrated into the core source compilation.
tests/internal/data/config_format/yaml/routing/precedence.yaml (1)
1-15: LGTM! Well-structured routing precedence test data.The YAML configuration provides test coverage for default route behavior in the routing system.
src/flb_router.c (1)
259-266: LGTM! Router configuration application integrated correctly.The post-routing configuration flow is properly integrated with appropriate error handling. The check for non-empty
input_routesensures the new router config is only applied when needed, and errors are properly logged and propagated.tests/internal/data/config_format/yaml/routing/multi_signal.yaml (1)
1-27: LGTM! Comprehensive multi-signal routing test data.This YAML configuration provides excellent test coverage for:
- Multi-signal routing (logs and traces combined)
- Conditional rules with field expressions and operators
- Fallback output configuration
The test data will help validate the router's ability to handle complex routing scenarios.
include/fluent-bit/flb_config.h (2)
30-30: LGTM! CFL include added for list support.The CFL library include is necessary for the new
input_routeslist field.
329-331: LGTM! Router configuration field added.The new
input_routesfield provides storage for parsed routing configurations. The field is appropriately placed at the end of the struct to minimize potential ABI impact.tests/internal/data/config_format/yaml/routing/metrics.yaml (1)
1-19: LGTM! Metrics routing test data is well-structured.This YAML configuration provides test coverage for metrics-specific routing with conditional rules and fallback output configuration, complementing the broader routing test suite.
tests/internal/data/config_format/yaml/routing/basic.yaml (1)
1-38: YAML fixture looks solidCovers logs/metrics, rule ops, and fallback reference; fits the intended parser paths.
src/config_format/flb_cf_yaml.c (1)
757-818: Array-of-strings detection is correct and safeGood: pre-count, type-check each entry, and only build a string list when all elements are strings; otherwise transfer the variant.
src/flb_config.c (4)
371-373: Good: initialize input_routes listEnsures lifetime-managed via flb_router_routes_destroy().
618-620: Good: destroy input_routes on config exitPrevents leaks of routes built during parse.
970-972: Nice: early debug for load startHelpful trace point when diagnosing config issues.
922-941: Verify flb_router_apply_config is called at the right timeI don’t see an invocation here; ensure it’s executed after plugin instances are created and before routing starts.
Would you like a grep script to locate actual call sites and validate ordering?
src/flb_router_condition.c (2)
24-42: Signal mapping is correctCovers logs/metrics/traces and returns 0 for unknown chunks.
79-122: Reasonable defaults for nowShort-circuit default/empty rules; per-signal evaluators return false pending resolvers.
When resolvers are added, ensure unit tests flip to assert true/false on concrete payloads for each op.
tests/internal/router_config.c (2)
711-757: Apply-config test is solidValidates direct route connection and cleanup.
487-553: Good end-to-end YAML parse testCatches routes on logs/metrics and default route wiring.
include/fluent-bit/flb_router.h (3)
64-71: Signal enum is sensibleBitmask with ANY aggregate enables multi-signal routes cleanly.
91-117: Struct definitions look goodSDS usage + mk_list for collections match core patterns.
129-147: Public prototypes align with implInterfaces match src/flb_router_condition.c and src/flb_router_config.c.
| if (strcasecmp(kv->key, "routes") == 0) { | ||
| continue; | ||
| } | ||
|
|
There was a problem hiding this comment.
Do not process “processors” as a regular property
Guard against variant “processors” breaking plugin setup; skip it like “routes”, or handle the variant explicitly.
if (strcasecmp(kv->key, "name") == 0) {
continue;
}
if (strcasecmp(kv->key, "routes") == 0) {
continue;
}
+ if (strcasecmp(kv->key, "processors") == 0) {
+ /* handled via group loader or (future) variant loader */
+ continue;
+ }🤖 Prompt for AI Agents
In src/flb_config.c around lines 864 to 867, the parsing currently skips the
"routes" key but not the "processors" variant which can disrupt plugin setup;
update the conditional to also skip keys equal to "processors" (e.g., treat
strcasecmp(kv->key, "processors") == 0 the same as "routes") or add an explicit
branch to handle "processors" correctly so it is not processed as a regular
property. Ensure the check is case-insensitive and placed alongside the existing
"routes" guard so "processors" will be bypassed or handled safely during
configuration parsing.
| val = var->data.as_string; | ||
|
|
||
| if (strcasecmp(val, "true") == 0) { | ||
| *out = FLB_TRUE; | ||
| return 0; | ||
| } | ||
| else if (strcasecmp(val, "false") == 0) { | ||
| *out = FLB_FALSE; | ||
| return 0; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Windows portability: strcasecmp/strncasecmp usage.
strcasecmp/strncasecmp are POSIX; this file includes <strings.h> only on non-Windows. Confirm a global portability shim maps these on Windows (e.g., to _stricmp/_strnicmp). If not, builds will fail.
Run to verify the presence of a mapping and current usage:
🏁 Script executed:
#!/bin/bash
# Find any central mapping for strcasecmp/strncasecmp on Windows
rg -nP 'define\s+strcasecmp|define\s+strncasecmp|_stricmp|_strnicmp|flb_compat' -C2
# Check other core files for pattern of including a compat header
rg -nP --type=c 'strcasecmp\(' -C2 src include lib | head -n 50Length of output: 106176
---
🏁 Script executed:
```shell
#!/bin/bash
# Check the full content of flb_compat.h to see all Windows mappings
cat include/fluent-bit/flb_compat.h | grep -A 10 -B 10 "strncasecmp\|strcasecmp"
echo "---"
echo "Checking src/flb_router_config.c for includes and usage:"
head -30 src/flb_router_config.c | grep -i "include"
grep -n "strcasecmp\|strncasecmp" src/flb_router_config.c | head -20
Length of output: 1159
---
🏁 Script executed:
```shell
#!/bin/bash
# View the full flb_compat.h file to see all mappings
echo "=== Full flb_compat.h content ==="
cat include/fluent-bit/flb_compat.h
echo ""
echo "=== All includes in src/flb_router_config.c ==="
grep "^#include" src/flb_router_config.c
echo ""
echo "=== Lines 100-110 of src/flb_router_config.c (context for strcasecmp usage) ==="
sed -n '100,110p' src/flb_router_config.c
Length of output: 6965
Add strcasecmp mapping to flb_compat.h and include it in src/flb_router_config.c.
The review comment is correct. Two critical issues prevent Windows compilation:
-
Missing mapping in flb_compat.h: The file defines
strncasecmp → _strnicmpbut lacksstrcasecmp → _stricmp. Comparison libraries (cfl, librdkafka, monkey, wasm-micro-runtime) in the codebase show both mappings are needed. -
Missing include in src/flb_router_config.c: The file uses
strcasecmp(lines 102, 106) andstrncasecmp(lines 236, 239, 242, 245) but does not include<fluent-bit/flb_compat.h>.
On Windows, <strings.h> does not provide these functions, so the build will fail. Fix by:
- Adding
#define strcasecmp _stricmpto flb_compat.h (matching the pattern forstrncasecmp) - Adding
#include <fluent-bit/flb_compat.h>to src/flb_router_config.c
🤖 Prompt for AI Agents
In src/flb_router_config.c around lines 100-110 and flb_compat.h, the code uses
strcasecmp but flb_compat.h only maps strncasecmp → _strnicmp and
src/flb_router_config.c does not include the compatibility header; add a mapping
in flb_compat.h: define strcasecmp as _stricmp (matching the existing
strncasecmp pattern) and then add #include <fluent-bit/flb_compat.h> at the top
of src/flb_router_config.c so both strcasecmp and strncasecmp resolve on Windows
builds.
| if (add_processor_properties(processor, kvlist) != 0) { | ||
| route_processors_destroy(&processor->properties); | ||
| if (processor->name) { | ||
| flb_sds_destroy(processor->name); | ||
| } | ||
| flb_free(processor); | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
Bug: Using processor-list destructor on a property list (type confusion) causes memory corruption on error path.
On add_processor_properties() failure, parse_processors() calls route_processors_destroy(&processor->properties), but that list holds flb_route_processor_property nodes, not flb_route_processor. This will cast/free the wrong type.
Apply this fix: introduce a dedicated properties destructor and use it in both places.
@@
static void route_processors_destroy(struct cfl_list *processors)
{
struct cfl_list *tmp;
struct cfl_list *head;
- struct cfl_list *p_head;
- struct cfl_list *p_tmp;
struct flb_route_processor *processor;
- struct flb_route_processor_property *prop;
@@
- cfl_list_foreach_safe(head, tmp, processors) {
- processor = cfl_list_entry(head, struct flb_route_processor, _head);
- cfl_list_del(&processor->_head);
-
- cfl_list_foreach_safe(p_head, p_tmp, &processor->properties) {
- prop = cfl_list_entry(p_head, struct flb_route_processor_property, _head);
- cfl_list_del(&prop->_head);
-
- if (prop->key) {
- flb_sds_destroy(prop->key);
- }
- if (prop->value) {
- flb_sds_destroy(prop->value);
- }
- flb_free(prop);
- }
-
- if (processor->name) {
- flb_sds_destroy(processor->name);
- }
- flb_free(processor);
- }
+ cfl_list_foreach_safe(head, tmp, processors) {
+ processor = cfl_list_entry(head, struct flb_route_processor, _head);
+ cfl_list_del(&processor->_head);
+ /* destroy properties */
+ {
+ struct cfl_list *p_head;
+ struct cfl_list *p_tmp;
+ struct flb_route_processor_property *prop;
+
+ cfl_list_foreach_safe(p_head, p_tmp, &processor->properties) {
+ prop = cfl_list_entry(p_head, struct flb_route_processor_property, _head);
+ cfl_list_del(&prop->_head);
+ if (prop->key) { flb_sds_destroy(prop->key); }
+ if (prop->value) { flb_sds_destroy(prop->value); }
+ flb_free(prop);
+ }
+ }
+ if (processor->name) {
+ flb_sds_destroy(processor->name);
+ }
+ flb_free(processor);
+ }
}
@@
- if (add_processor_properties(processor, kvlist) != 0) {
- route_processors_destroy(&processor->properties);
+ if (add_processor_properties(processor, kvlist) != 0) {
+ /* destroy only the properties accumulated so far */
+ {
+ struct cfl_list *p_head;
+ struct cfl_list *p_tmp;
+ struct flb_route_processor_property *prop;
+ cfl_list_foreach_safe(p_head, p_tmp, &processor->properties) {
+ prop = cfl_list_entry(p_head, struct flb_route_processor_property, _head);
+ cfl_list_del(&prop->_head);
+ if (prop->key) { flb_sds_destroy(prop->key); }
+ if (prop->value) { flb_sds_destroy(prop->value); }
+ flb_free(prop);
+ }
+ }
if (processor->name) {
flb_sds_destroy(processor->name);
}
flb_free(processor);
return -1;
}Committable suggestion skipped: line range outside the PR's diff.
| rules_array = rules_var->data.as_array; | ||
| for (idx = 0; idx < cfl_array_size(rules_array); idx++) { | ||
| entry = cfl_array_fetch_by_index(rules_array, idx); | ||
| rule = parse_condition_rule(entry); | ||
| if (!rule) { | ||
| route_condition_destroy(condition); | ||
| return NULL; | ||
| } | ||
|
|
||
| if (!validate_rule_field(rule->field, signals)) { | ||
| route_condition_destroy(condition); | ||
| return NULL; | ||
| } | ||
|
|
||
| cfl_list_add(&rule->_head, &condition->rules); |
There was a problem hiding this comment.
Leak: rule is allocated but not freed when validate_rule_field() fails.
If validation fails, condition is destroyed but the not-yet-linked rule leaks.
Apply this fix:
@@
rule = parse_condition_rule(entry);
if (!rule) {
route_condition_destroy(condition);
return NULL;
}
if (!validate_rule_field(rule->field, signals)) {
- route_condition_destroy(condition);
+ /* free the rule before destroying condition */
+ if (rule->value) { flb_sds_destroy(rule->value); }
+ if (rule->op) { flb_sds_destroy(rule->op); }
+ if (rule->field) { flb_sds_destroy(rule->field); }
+ flb_free(rule);
+ route_condition_destroy(condition);
return NULL;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rules_array = rules_var->data.as_array; | |
| for (idx = 0; idx < cfl_array_size(rules_array); idx++) { | |
| entry = cfl_array_fetch_by_index(rules_array, idx); | |
| rule = parse_condition_rule(entry); | |
| if (!rule) { | |
| route_condition_destroy(condition); | |
| return NULL; | |
| } | |
| if (!validate_rule_field(rule->field, signals)) { | |
| route_condition_destroy(condition); | |
| return NULL; | |
| } | |
| cfl_list_add(&rule->_head, &condition->rules); | |
| rules_array = rules_var->data.as_array; | |
| for (idx = 0; idx < cfl_array_size(rules_array); idx++) { | |
| entry = cfl_array_fetch_by_index(rules_array, idx); | |
| rule = parse_condition_rule(entry); | |
| if (!rule) { | |
| route_condition_destroy(condition); | |
| return NULL; | |
| } | |
| if (!validate_rule_field(rule->field, signals)) { | |
| /* free the rule before destroying condition */ | |
| if (rule->value) { flb_sds_destroy(rule->value); } | |
| if (rule->op) { flb_sds_destroy(rule->op); } | |
| if (rule->field) { flb_sds_destroy(rule->field); } | |
| flb_free(rule); | |
| route_condition_destroy(condition); | |
| return NULL; | |
| } | |
| cfl_list_add(&rule->_head, &condition->rules); |
🤖 Prompt for AI Agents
In src/flb_router_config.c around lines 600-614, the code leaks rule when
validate_rule_field() fails because the newly parsed rule is not freed before
returning; call the appropriate destructor for the rule (e.g.
route_rule_destroy(rule) or the module's free function) immediately before
route_condition_destroy(condition) and returning NULL so the rule is freed when
validation fails.
Signed-off-by: Eduardo Silva <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/config_format/flb_cf_yaml.c (1)
2069-2078: Dead code:strcmpforprocessorsis unreachable here.The conditional at line 2069 checks for both
"routes"and"processors", but the"processors"case is already handled at lines 2034–2055 with abreakat line 2054. This meansstrcmp(state->key, "processors") == 0will never evaluate to true here, making it dead code.Apply this diff to remove the unreachable condition:
- if (strcmp(state->key, "routes") == 0 || - strcmp(state->key, "processors") == 0) { + if (strcmp(state->key, "routes") == 0) { state = state_push_variant(ctx, state, 1);This clarifies that only
routesmappings are treated as variants here, aligning with the specialprocessorsgroup handling above.
🧹 Nitpick comments (1)
src/config_format/flb_cf_yaml.c (1)
2034-2055: Special-caseprocessorshandling adds complexity.This block creates a dedicated
processorsgroup for the mapping format, which appears to preserve backward compatibility. However, it introduces a bifurcated code path:
processors: {...}→ dedicated group (lines 2034–2055)processors: [...]→ variant (lines 2019–2022)routes: [...]orroutes: {...}→ always variant (lines 2019–2022, 2069–2078)Recommendation: Document why
processorsrequires special handling whileroutesdoes not, and confirm that allprocessorsconsumers gracefully handle both formats.Consider adding a comment explaining the dual-path processors handling:
+ /* Special handling for processors mapping format to preserve + * backward compatibility with existing plugin loaders that expect + * a group structure. Sequence format is handled as variant. */ if (strcmp(state->key, "processors") == 0) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/config_format/flb_cf_yaml.c(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/config_format/flb_cf_yaml.c (4)
lib/cfl/include/cfl/cfl_array.h (1)
cfl_array_fetch_by_index(36-44)src/config_format/flb_config_format.c (3)
flb_cf_section_property_add_list(366-411)flb_cf_section_property_add_variant(328-364)flb_cf_group_create(567-607)lib/cfl/src/cfl_sds.c (1)
cfl_sds_len(88-91)lib/cfl/src/cfl_array.c (1)
cfl_array_append_string(167-185)
⏰ 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 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: pr-windows-build / call-build-windows-package (Windows 32bit, x86, x86-windows-static, 3.31.6)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_ARROW=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=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_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_SANITIZE_THREAD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=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_ADDRESS=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_COVERAGE=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 (-DSANITIZE_UNDEFINED=On, 3.31.6, gcc, g++)
- 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_JEMALLOC=Off, 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_JEMALLOC=On, 3.31.6, clang, clang++)
- 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 - fuzzing test
- 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-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-22.04, clang-12)
- GitHub Check: pr-compile-centos-7
🔇 Additional comments (2)
src/config_format/flb_cf_yaml.c (2)
2019-2022: Verify consistency between SEQUENCE and MAPPINGprocessorshandling in the PR changes.The PR introduces significant changes to processor parsing: SEQUENCE format
processors: [...]now callsstate_push_variant(ctx, state, 0)(line 2019-2022, new), while MAPPING formatprocessors: {...}callsflb_cf_group_create()(line 2034-2046, new). This creates an asymmetry: MAPPING creates both a group and variant structure, but SEQUENCE creates only a variant with no group.Downstream code at
src/flb_config.c:923callsflb_cf_group_get(cf, s, "processors"), which expects a group structure. SEQUENCE-formatted processors will not produce a group, causing this call to return NULL. Confirm:
- Whether existing or planned configs use SEQUENCE format for processors and require group access, OR
- That SEQUENCE format is intentionally excluded from group creation (e.g., intended for direct variant consumption), with documentation or comments clarifying this design decision.
757-829: No issues found. The mixed array-handling strategy is correct and intentional.The dual-path array handling you flagged—homogeneous string arrays via
flb_cf_section_property_add_listand heterogeneous arrays viaflb_cf_section_property_add_variant—is both intentional and type-safe. Both paths wrap their data ascfl_variantin the kvlist (viacfl_kvlist_insert_arrayandcfl_kvlist_insert_variantrespectively), and downstream consumers (e.g.,flb_processor.c:load_from_config_format_group) correctly validate variant types before use. This change is part of the processors config feature expansion (git commit 65fc716), and it maintains backward compatibility while adding support for structured nested configs.
Signed-off-by: Eduardo Silva <[email protected]>
Fix 'any bit matches' logic to require all requested signals and ensure multi-signal routes only connect to outputs supporting all signals Signed-off-by: Eduardo Silva <[email protected]>
1 participant
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