env: add new extended format to handle environment variables #10862
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]>
This patch introduces an extended version of environment variables. While
the normal definition sets in Yaml sets a map with key/value pairs, the
extended version allows to define them as a list of items with addition
properties, specifically when used with the new 'uri' component.
In the example below we have two env sections, both are valid:
env:
- name: SECRET
uri: 'file:///home/edsiper/c/fluent-bit/secret.txt'
refresh_interval: 4s
- name: FIXED
value: "fixed_value"
env:
backward_compatibility: "fixed_backward_compatibility"
pipeline:
inputs:
- name: dummy
dummy: '{"message": "Hello!", "secret": "${SECRET}", "fixed": "${FIXED}", "compat": "${backward_compatibility}"}'
outputs:
- name: stdout
match: '*'
note that simple and extended versions can be used together ONLY if they are
in different env sections. The extended version supports 'uri', 'value' and 'refresh_interval'
parameters, for now only 'file://...' uri prefix is supported and if refresh_interval
is set, the value is refreshed on timeout in the next read.
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]>
edsiper
requested review from
nokute78,
fujimotos and
koleini
as code owners
WalkthroughAdds an extended env-var model (name/value/uri/refresh_interval), YAML parsing for env lists, file-backed env values with on-demand refresh, dynamic env-template handling guarded by FLB_CONFIG_MAP_DYNAMIC_ENV, changes flb_config_map_set to accept Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant YAMLParser as YAML Parser
participant CF as Config Format
participant CFG as Config Map
participant ENV as Env Runtime
User->>YAMLParser: provide YAML (includes env: [ ... ])
YAMLParser->>CF: parse env list -> create flb_cf_env_var items
CF->>ENV: flb_cf_env_var_add / flb_env_set_extended(name,value,uri,refresh_interval)
CF->>CFG: build config-map entries (defaults/templates)
CFG->>CFG: treat string entries
alt FLB_CONFIG_MAP_DYNAMIC_ENV set
CFG-->>CFG: store raw template (defer translation)
else
CFG->>ENV: flb_env_var_translate(value) -> store translated value
end
sequenceDiagram
autonumber
participant Collector
participant Dummy as in_dummy
participant ENV as Env Runtime
participant FS as Filesystem
Collector->>Dummy: collect()
Dummy->>ENV: flb_env_var_translate(body_template)
alt var has uri and refresh due
ENV->>FS: read file://uri
FS-->>ENV: content
ENV->>ENV: update var.value and last_refresh
end
ENV-->>Dummy: resolved JSON string
Dummy->>Dummy: parse JSON -> msgpack -> emit events
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Suggested reviewers
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial 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). (29)
✨ Finishing Touches
🧪 Generate unit tests
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 |
| msg = DEFAULT_DUMMY_MESSAGE; | ||
| } | ||
| flb_plg_info(ctx->ins, "received dummy property: %s", msg); | ||
| ctx->body_template = flb_strdup(msg); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
plugins/processor_sampling/sampling_probabilistic.c (3)
43-59: Type mismatch: sampling_rule vs sampling_settings leads to UBsettings is declared as struct sampling_rule* but allocated as struct sampling_settings and used as such. Fix the type and add basic range validation.
Apply:
- struct sampling_rule *settings; + struct sampling_settings *settings; @@ - ret = flb_config_map_set(config, &ctx->plugin_settings_properties, ctx->plugin_config_map, (void *) settings); + ret = flb_config_map_set(config, &ctx->plugin_settings_properties, ctx->plugin_config_map, (void *) settings); if (ret == -1) { flb_free(settings); return -1; } + + /* validate percentage: 0..100 */ + if (settings->sampling_percentage < 0 || settings->sampling_percentage > 100) { + flb_plg_error(ctx->ins, "invalid sampling_percentage=%d (expected 0..100)", + settings->sampling_percentage); + flb_free(settings); + return -1; + }
64-76: Incorrect trace_id parsing (memcpy of ASCII); parse hex insteadmemcpy reads ASCII chars, not bytes. This skews sampling. Parse the first 16 hex digits into a uint64_t.
Apply:
-static uint64_t extract_trace_id(cfl_sds_t trace_id) { - uint64_t trace_number = 0; - - if (cfl_sds_len(trace_id) < 16) { - /* invalid trace_id */ - return 0; - } - - memcpy(&trace_number, trace_id, 8); - - /* convert to big-endian (if needed) */ - trace_number = flb_net_htonll(trace_number); - return trace_number; -} +static uint64_t extract_trace_id(cfl_sds_t trace_id) +{ + uint64_t v = 0; + int i; + + if (cfl_sds_len(trace_id) < 16) { + return 0; + } + + for (i = 0; i < 16; i++) { + char c = trace_id[i]; + uint64_t n = + (c >= '0' && c <= '9') ? (uint64_t)(c - '0') : + (c >= 'a' && c <= 'f') ? (uint64_t)(c - 'a' + 10) : + (c >= 'A' && c <= 'F') ? (uint64_t)(c - 'A' + 10) : 0xFFu; + if (n == 0xFFu) { + return 0; /* invalid hex */ + } + v = (v << 4) | n; + } + return v; +}
121-126: cb_exit uses wrong type; free the correct settings structEnsure symmetry with cb_init allocation.
Apply:
-static int cb_exit(struct flb_config *config, void *data) +static int cb_exit(struct flb_config *config, void *data) { - struct sampling_rule *rule = data; + struct sampling_settings *settings = data; - if (rule) { - flb_free(rule); + if (settings) { + flb_free(settings); } return 0; }src/flb_config_map.c (3)
669-673: Wrong condition: CLIST/SLIST range check uses OR instead of ANDThis should be a bounded-range check. Using || makes the branch match most types (e.g., VARIANT), causing incorrect list handling.
- else if (m->type >= FLB_CONFIG_MAP_CLIST || - m->type <= FLB_CONFIG_MAP_SLIST_4) { + else if (m->type >= FLB_CONFIG_MAP_CLIST && + m->type <= FLB_CONFIG_MAP_SLIST_4) {
760-786: Same OR/AND bug in MULT path for list parsingThis misclassifies non-list types as lists when populating entries.
- else if (m->type >= FLB_CONFIG_MAP_CLIST || - m->type <= FLB_CONFIG_MAP_SLIST_4) { + else if (m->type >= FLB_CONFIG_MAP_CLIST && + m->type <= FLB_CONFIG_MAP_SLIST_4) {
847-863: Same OR/AND bug in direct-write pathFix the condition here too.
- else if (m->type >= FLB_CONFIG_MAP_CLIST || - m->type <= FLB_CONFIG_MAP_SLIST_4) { + else if (m->type >= FLB_CONFIG_MAP_CLIST && + m->type <= FLB_CONFIG_MAP_SLIST_4) {plugins/in_dummy/in_dummy.c (1)
510-516: Do not dereference config in unused-parameter pattern
(void) *config;dereferences a possibly NULL pointer. Use(void) config;.static int in_dummy_exit(void *data, struct flb_config *config) { - (void) *config; + (void) config; struct flb_dummy *ctx = data;src/flb_env.c (1)
186-217: Hash table add error is ignored; potential lifetime bug with fs_buf.
If add fails, we still enqueue var; also passing fs_buf then freeing it can UAF if HT doesn’t copy. Create var first, store a stable SDS, then add HT; handle failures.- id = flb_hash_table_add(env->ht, key, klen, (void *) value, vlen); - - var = flb_calloc(1, sizeof(struct flb_env_var)); + /* Create tracking node first */ + var = flb_calloc(1, sizeof(struct flb_env_var)); if (!var) { flb_errno(); if (fs_buf) { flb_sds_destroy(fs_buf); } return -1; } - mk_list_add(&var->_head, &env->vars); - var->name = flb_sds_create(key); - if (vlen > 0) { - var->value = flb_sds_create_len(value, vlen); - } + var->name = flb_sds_create(key); + if (vlen > 0) { + var->value = flb_sds_create_len(value, vlen); /* stable copy */ + } if (orig_uri) { var->uri = flb_sds_create(orig_uri); } - var->refresh_interval = refresh_interval; + var->refresh_interval = refresh_interval; if (orig_uri) { var->last_refresh = time(NULL); } else { var->last_refresh = 0; } + /* Now publish into HT using the stable SDS */ + id = flb_hash_table_add(env->ht, key, klen, (void *) var->value, vlen); + if (id < 0) { + if (var->name) flb_sds_destroy(var->name); + if (var->value) flb_sds_destroy(var->value); + if (var->uri) flb_sds_destroy(var->uri); + flb_free(var); + if (fs_buf) { + flb_sds_destroy(fs_buf); + } + return -1; + } + mk_list_add(&var->_head, &env->vars); if (fs_buf) { flb_sds_destroy(fs_buf); } return id;
🧹 Nitpick comments (24)
plugins/processor_sampling/sampling_tail.c (1)
511-513: Avoid int overflow when converting seconds to millisecondsCast before multiply to keep wide arithmetic.
- settings->decision_wait_ms = settings->decision_wait * 1000; + settings->decision_wait_ms = (uint64_t) settings->decision_wait * 1000ULL;include/fluent-bit/flb_input.h (1)
718-741: Return 0 when no maps are present (avoid spurious -1)Current default ret is -1 if neither map exists. Initialize to success.
Apply:
- ret = -1; + ret = 0;src/flb_config.c (2)
935-949: Good switch to extended env handling; add light sanity checksIterating cf->env vars and calling flb_env_set_extended is correct. Consider guarding against missing names and improving diagnostics when both name or value are absent.
- mk_list_foreach(head, &cf->env) { - env_var = mk_list_entry(head, struct flb_cf_env_var, _head); + mk_list_foreach(head, &cf->env) { + env_var = mk_list_entry(head, struct flb_cf_env_var, _head); + if (!env_var->name) { + flb_error("skipping env var with null name"); + continue; + } ret = flb_env_set_extended(config->env, env_var->name, env_var->value, env_var->uri, env_var->refresh_interval);
950-950: Include URI in error to aid debuggingIf a URI was provided, include it in the error log.
- flb_error("could not set config environment variable '%s'", env_var->name); + if (env_var->uri) { + flb_error("could not set config environment variable '%s' (uri=%s)", + env_var->name, env_var->uri); + } + else { + flb_error("could not set config environment variable '%s'", env_var->name); + }include/fluent-bit/flb_config_map.h (1)
55-55: Flag addition looks fine; document intentDefine clarifies runtime env resolution. Add a brief comment to guide plugin authors.
-#define FLB_CONFIG_MAP_DYNAMIC_ENV 2 /* flag: resolve environment variables at runtime */ +#define FLB_CONFIG_MAP_DYNAMIC_ENV 2 /* flag: store raw template and resolve env at runtime */include/fluent-bit/flb_env.h (3)
25-26: Headers look right; ensure list is initialized.mk_list is introduced here; please verify flb_env_create() calls mk_list_init(&env->vars) to avoid undefined behavior when adding vars.
If needed, add in src/flb_env.c:
int flb_env_create() { struct flb_env *env = flb_calloc(1, sizeof(*env)); if (!env) { ... } + mk_list_init(&env->vars); ... }
30-37: Define invariants and lifecycle for extended env vars.
- Clarify whether name is mandatory and non-empty; code paths benefit from assuming non-null, non-zero length.
- Document refresh_interval semantics (e.g., 0 = no refresh; negative rejected).
- Confirm destruction path frees name/value/uri and removes from env->vars.
Would you like a small header doc-block added for struct flb_env_var spelling out these rules?
Also applies to: 42-42
53-55: API contract: precedence and ownership.
- flb_env_set_extended(): specify precedence between val and uri (current impl prefers uri and also handles value starting with "file://").
- Confirm that flb_hash_table_add copies the value buffer; src/flb_env.c frees fs_buf after insertion, which would be unsafe if the hash table stores pointers.
If needed, I can add a brief comment above the prototype summarizing precedence and expected ownership rules.
src/flb_reload.c (1)
320-326: Validate env entries before copying; improve error logging.
- Ensure ev->name is non-null and non-empty; skip or fail fast otherwise to avoid nameless env entries downstream.
- Log the failing env name on add failure to aid debugging.
Proposed tweak:
- if (!flb_cf_env_var_add(dest_cf, + if (ev->name == NULL || flb_sds_len(ev->name) == 0) { + flb_error("[reload] env entry without name encountered; skipping"); + continue; + } + if (!flb_cf_env_var_add(dest_cf, ev->name, ev->name ? flb_sds_len(ev->name) : 0, ev->value, ev->value ? flb_sds_len(ev->value) : 0, ev->uri, ev->uri ? flb_sds_len(ev->uri) : 0, ev->refresh_interval)) { - return -1; + flb_error("[reload] failed to copy env '%s'", ev->name); + return -1; }src/config_format/flb_config_format.c (3)
153-171: Unlink list nodes before free.mk_list_foreach_safe allows freeing, but explicitly unlinking avoids leaving stale pointers in the list until mk_list_init is called and is the common pattern elsewhere.
mk_list_foreach_safe(head, tmp, &cf->env) { ev = mk_list_entry(head, struct flb_cf_env_var, _head); + mk_list_del(&ev->_head); if (ev->name) { flb_sds_destroy(ev->name); } if (ev->value) { flb_sds_destroy(ev->value); } if (ev->uri) { flb_sds_destroy(ev->uri); } flb_free(ev); }
441-504: Enforce required name; tighten error paths.
- Reject entries without a name early to preserve invariants.
- Optionally clamp negative refresh_interval to 0 (no refresh), if supported.
- Minor: the extra conditional flb_sds_destroy(ev->uri) in the uri error path is redundant since allocation just failed.
struct flb_cf_env_var *flb_cf_env_var_add(struct flb_cf *cf, char *name, size_t name_len, char *value, size_t value_len, char *uri, size_t uri_len, int refresh_interval) { struct flb_cf_env_var *ev; + /* validate mandatory fields */ + if (name == NULL || (name_len == 0 && (name_len = strlen(name)) ) == 0) { + return NULL; + } + if (refresh_interval < 0) { + refresh_interval = 0; + } @@ - if (uri) { + if (uri) { ev->uri = flb_sds_create_len(uri, uri_len); if (!ev->uri) { if (ev->name) { flb_sds_destroy(ev->name); } if (ev->value) { flb_sds_destroy(ev->value); } - if (ev->uri) { - flb_sds_destroy(ev->uri); - } flb_free(ev); return NULL; } }
875-894: Include refresh_interval in dump for visibility.Printing the interval helps diagnose dynamic env behavior.
- if (ev->uri) { - printf(" - %-15s: %s\n", ev->name, ev->uri); + if (ev->uri) { + if (ev->refresh_interval > 0) { + printf(" - %-15s: %s (refresh=%ds)\n", ev->name, ev->uri, ev->refresh_interval); + } + else { + printf(" - %-15s: %s\n", ev->name, ev->uri); + } } else if (ev->value) { printf(" - %-15s: %s\n", ev->name, ev->value); }src/config_format/flb_cf_yaml.c (4)
969-975: Pop result unchecked; add NULL checkSTATE_ENV_LIST_VAL pops without verifying the returned state, risking NULL deref on subsequent transitions.
- state = state_pop(ctx); + state = state_pop(ctx); + if (state == NULL) { + flb_error("no state left"); + return YAML_FAILURE; + }
920-934: Avoid leaving dangling parent pointer to freed env_varAfter YAML_MAPPING_END_EVENT, when freeing an uncommitted env_var, the parent state's env_var remains pointing to freed memory.
Set the parent/active state's env_var to NULL after committing or freeing to prevent accidental reuse:
if (state->env_var && state->env_var->name) { mk_list_add(&state->env_var->_head, &conf->env); + state->env_var = NULL; } else if (state->env_var) { if (state->env_var->name) { flb_sds_destroy(state->env_var->name); } if (state->env_var->value) { flb_sds_destroy(state->env_var->value); } if (state->env_var->uri) { flb_sds_destroy(state->env_var->uri); } flb_free(state->env_var); + state->env_var = NULL; }
3055-3064: Memory hygiene on parser teardown for env_varOn error paths, state_pop_with_cleanup() frees keys/kvlists/variants but not any pending env_var allocations. That can leak if a YAML failure occurs while parsing an env mapping.
Option A: extend state_pop_with_cleanup to free s->env_var (name/value/uri) if set.
Option B: introduce a small helper (env_var_destroy) and call it from all early-return/error branches that allocate env_var but don’t commit it.
3119-3121: Typo: “poing” → “point”Minor doc fix.
- /* process the entry poing config file */ + /* process the entry point config file */src/flb_config_map.c (1)
294-321: Dynamic env default: OK, but consider lazy translation optionStoring raw def_value for FLB_CONFIG_MAP_DYNAMIC_ENV is correct. Optionally, add a helper to check “contains ${…}” and translate once if a template has no placeholders to save runtime cost.
plugins/in_dummy/in_dummy.c (4)
252-254: Typo in log message“genartion” → “generation”.
- flb_plg_error(ins, "log chunk genartion error (%d)", result); + flb_plg_error(ins, "log chunk generation error (%d)", result);
120-154: Leverage cached msgpack on per-collect parse failureYou already validate templates at configure() and keep ref_body_msgpack/ref_metadata_msgpack. If per-collect env resolution or JSON packing fails, fall back to the cached refs instead of failing the collect.
- if (result != 0) { - flb_plg_error(ctx->ins, "failed to parse JSON template"); - flb_sds_destroy(resolved_body); - flb_sds_destroy(resolved_metadata); - if (body_msgpack) { flb_free(body_msgpack); } - if (metadata_msgpack) { flb_free(metadata_msgpack); } - return -1; - } + if (result != 0) { + flb_plg_warn(ctx->ins, "failed to parse JSON template, using cached"); + if (body_msgpack) { flb_free(body_msgpack); } + if (metadata_msgpack) { flb_free(metadata_msgpack); } + body_msgpack = ctx->ref_body_msgpack; + body_msgpack_size = ctx->ref_body_msgpack_size; + metadata_msgpack = ctx->ref_metadata_msgpack; + metadata_msgpack_size = ctx->ref_metadata_msgpack_size; + /* continue with cached buffers */ + result = 0; + }
96-119: Avoid unnecessary translate+pack when template has no placeholdersIf the template lacks “${”, flb_env_var_translate() still allocates and flb_pack_json() runs every collect. Cache msgpack for placeholder-free templates to reduce CPU.
Example approach: detect ‘${’ in template and reuse ctx->ref_* when absent; only resolve+pack when present.
357-365: Info log may expose large payloads/secretsLogging full “dummy” property at INFO can be noisy or leak data. Consider DEBUG.
- flb_plg_info(ctx->ins, "received dummy property: %s", msg); + flb_plg_debug(ctx->ins, "received dummy property");include/fluent-bit/config_format/flb_cf.h (2)
144-151: Document refresh_interval semantics (and clamp negatives).
Please specify that 0 disables refresh and negatives are treated as 0. This avoids ambiguity for callers.Apply an inline comment:
- int refresh_interval; + int refresh_interval; /* seconds; 0 disables refresh; negatives treated as 0 */
152-157: Const-correctness on flb_cf_env_var_add parameters.
The function doesn’t mutate input buffers; use const to widen usability and avoid accidental writes.-struct flb_cf_env_var *flb_cf_env_var_add(struct flb_cf *cf, - char *name, size_t name_len, - char *value, size_t value_len, - char *uri, size_t uri_len, +struct flb_cf_env_var *flb_cf_env_var_add(struct flb_cf *cf, + const char *name, size_t name_len, + const char *value, size_t value_len, + const char *uri, size_t uri_len, int refresh_interval);src/flb_env.c (1)
121-142: Clamp invalid inputs early (refresh_interval, NULL key).
Avoid surprising behavior with negative intervals; guard NULL key.int flb_env_set_extended(struct flb_env *env, const char *key, const char *val, const char *uri, int refresh_interval) { + if (!env || !key) { + return -1; + } + if (refresh_interval < 0) { + refresh_interval = 0; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
include/fluent-bit/config_format/flb_cf.h(2 hunks)include/fluent-bit/flb_config_map.h(2 hunks)include/fluent-bit/flb_custom.h(1 hunks)include/fluent-bit/flb_env.h(2 hunks)include/fluent-bit/flb_filter.h(1 hunks)include/fluent-bit/flb_input.h(2 hunks)include/fluent-bit/flb_output.h(1 hunks)include/fluent-bit/flb_processor.h(1 hunks)plugins/in_dummy/in_dummy.c(12 hunks)plugins/in_dummy/in_dummy.h(1 hunks)plugins/processor_sampling/sampling_probabilistic.c(1 hunks)plugins/processor_sampling/sampling_tail.c(1 hunks)src/config_format/flb_cf_yaml.c(7 hunks)src/config_format/flb_config_format.c(4 hunks)src/flb_config.c(3 hunks)src/flb_config_map.c(5 hunks)src/flb_env.c(3 hunks)src/flb_input.c(1 hunks)src/flb_reload.c(2 hunks)tests/internal/config_map.c(5 hunks)tests/internal/env.c(2 hunks)tests/internal/fuzzers/config_map_fuzzer.c(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
PR: fluent/fluent-bit#9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit CMakeLists.txt, the system library preference flags are defined as FLB_PREFER_SYSTEM_LIB_ZSTD and FLB_PREFER_SYSTEM_LIB_KAFKA with the FLB_ prefix.
Applied to files:
src/config_format/flb_cf_yaml.c
📚 Learning: 2025-08-29T06:25:02.561Z
Learnt from: shadowshot-x
PR: fluent/fluent-bit#10794
File: tests/internal/aws_compress.c:7-7
Timestamp: 2025-08-29T06:25:02.561Z
Learning: In Fluent Bit, ZSTD (zstandard) compression library is bundled directly in the source tree at `lib/zstd-1.5.7` and is built unconditionally as a static library. Unlike optional external dependencies, ZSTD does not use conditional compilation guards like `FLB_HAVE_ZSTD` and is always available. Headers like `<fluent-bit/flb_zstd.h>` can be included directly without guards.
Applied to files:
src/config_format/flb_cf_yaml.c
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
PR: fluent/fluent-bit#9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit, the correct CMake flag for using system librdkafka is `FLB_PREFER_SYSTEM_LIB_KAFKA=ON`.
Applied to files:
src/config_format/flb_cf_yaml.c
🧬 Code graph analysis (21)
src/flb_input.c (1)
src/flb_sds.c (1)
flb_sds_create(78-90)
tests/internal/env.c (2)
src/flb_env.c (5)
flb_env_create(71-95)flb_env_set_extended(121-217)flb_env_destroy(97-119)flb_env_var_translate(315-428)flb_env_set(219-222)src/flb_sds.c (1)
flb_sds_destroy(389-399)
tests/internal/fuzzers/config_map_fuzzer.c (1)
src/flb_config_map.c (1)
flb_config_map_set(589-868)
plugins/processor_sampling/sampling_probabilistic.c (1)
src/flb_config_map.c (1)
flb_config_map_set(589-868)
include/fluent-bit/flb_config_map.h (1)
src/flb_config_map.c (1)
flb_config_map_set(589-868)
src/flb_config.c (1)
src/flb_env.c (1)
flb_env_set_extended(121-217)
include/fluent-bit/flb_custom.h (1)
src/flb_config_map.c (1)
flb_config_map_set(589-868)
plugins/processor_sampling/sampling_tail.c (1)
src/flb_config_map.c (1)
flb_config_map_set(589-868)
include/fluent-bit/flb_filter.h (1)
src/flb_config_map.c (1)
flb_config_map_set(589-868)
include/fluent-bit/flb_processor.h (1)
src/flb_config_map.c (1)
flb_config_map_set(589-868)
src/config_format/flb_config_format.c (2)
src/flb_sds.c (2)
flb_sds_destroy(389-399)flb_sds_create_len(58-76)include/fluent-bit/flb_mem.h (2)
flb_free(126-128)flb_calloc(84-96)
include/fluent-bit/flb_input.h (1)
src/flb_config_map.c (1)
flb_config_map_set(589-868)
src/flb_config_map.c (3)
src/flb_sds.c (2)
flb_sds_create(78-90)flb_sds_destroy(389-399)src/flb_env.c (1)
flb_env_var_translate(315-428)include/fluent-bit/flb_mem.h (1)
flb_free(126-128)
include/fluent-bit/config_format/flb_cf.h (1)
src/config_format/flb_config_format.c (1)
flb_cf_env_var_add(441-504)
include/fluent-bit/flb_output.h (1)
src/flb_config_map.c (1)
flb_config_map_set(589-868)
tests/internal/config_map.c (1)
src/flb_config_map.c (1)
flb_config_map_set(589-868)
src/flb_reload.c (1)
src/config_format/flb_config_format.c (1)
flb_cf_env_var_add(441-504)
include/fluent-bit/flb_env.h (1)
src/flb_env.c (1)
flb_env_set_extended(121-217)
plugins/in_dummy/in_dummy.c (5)
src/flb_input.c (1)
flb_input_get_property(777-781)src/flb_env.c (1)
flb_env_var_translate(315-428)src/flb_sds.c (2)
flb_sds_create(78-90)flb_sds_destroy(389-399)src/flb_pack.c (1)
flb_pack_json(335-341)include/fluent-bit/flb_mem.h (1)
flb_free(126-128)
src/flb_env.c (3)
src/flb_sds.c (3)
flb_sds_destroy(389-399)flb_sds_create(78-90)flb_sds_create_len(58-76)include/fluent-bit/flb_mem.h (2)
flb_free(126-128)flb_calloc(84-96)src/flb_hash_table.c (4)
flb_hash_table_destroy(197-215)flb_hash_table_get(496-522)flb_hash_table_del(605-674)flb_hash_table_add(401-494)
src/config_format/flb_cf_yaml.c (3)
include/fluent-bit/flb_mem.h (2)
flb_calloc(84-96)flb_free(126-128)src/flb_sds.c (2)
flb_sds_destroy(389-399)flb_sds_create(78-90)src/flb_utils.c (1)
flb_utils_time_to_seconds(646-669)
⏰ 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). (29)
- 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_SIMD=Off, 3.31.6, clang, clang++)
- 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_SIMD=On, 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, gcc, g++)
- 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, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=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_COVERAGE=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_SMALL=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=Off, 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 (-DFLB_JEMALLOC=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 (-DSANITIZE_ADDRESS=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, clang, clang++)
- 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-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-24.04, clang-14)
- 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-centos-7
🔇 Additional comments (21)
plugins/in_dummy/in_dummy.h (1)
59-62: Destroy path validated; optional SDS refactor only
The plugin correctly freesctx->body_templateandctx->metadata_templatein its destroy callback. Converting these fields toflb_sds_tis purely optional per SDS-string guidelines and not mandatory.tests/internal/fuzzers/config_map_fuzzer.c (1)
197-201: LGTM: updated to new flb_config_map_set signatureCall-site correctly passes config; rest of logic unchanged.
plugins/processor_sampling/sampling_tail.c (1)
505-509: LGTM: config map set call updated to new signatureChange aligns with API update and error paths remain correct.
tests/internal/config_map.c (1)
206-208: LGTM: tests updated for new flb_config_map_set signatureCoverage and assertions remain intact.
Also applies to: 258-260, 349-351, 411-413
include/fluent-bit/flb_processor.h (1)
281-282: LGTM: wrapper now forwards config to flb_config_map_setConsistent with API change; inlines remain minimal.
include/fluent-bit/flb_custom.h (1)
96-97: LGTM: custom wrapper updated to pass configMatches new signature and preserves behavior.
include/fluent-bit/flb_filter.h (1)
128-128: Correctly passes config into flb_config_map_setSignature update applied as expected.
include/fluent-bit/flb_output.h (1)
1305-1305: Updated flb_config_map_set calls look correct (normal + net props)Both paths now pass ins->config as first arg. Matches the new API.
Also applies to: 1313-1314
plugins/processor_sampling/sampling_probabilistic.c (2)
53-53: Correct API adaptation: pass config to flb_config_map_setGood update.
79-91: Confirm units: sampling_percentage is 0..100hash_value is [0, ~100). With the validation above, comparison is consistent. No change needed, just ensure constraints hold at init.
include/fluent-bit/flb_input.h (1)
724-725: Correctly updated to pass ins->config into flb_config_map_set (normal + net)Matches the new function signature.
Also applies to: 733-735
src/flb_config.c (1)
148-150: No-op change in service config tableEntry looks unchanged functionally. Nothing to do.
include/fluent-bit/flb_config_map.h (1)
114-118: Helper is OKInline checker is straightforward and useful.
tests/internal/env.c (2)
26-29: Includes are appropriate for file I/ONeeded headers added; OK.
418-426: Test list additions look goodNew cases are registered correctly.
src/flb_config_map.c (1)
589-590: All call sites updated to new flb_config_map_set signatureNo 3-argument calls remain; every caller now passes the
configpointer.include/fluent-bit/config_format/flb_cf.h (1)
93-95: LGTM: clearer env-section intent.
Comment and list type look good.src/flb_env.c (4)
29-29: LGTM: time.h include for refresh logic.
Needed for time() usage.
91-91: LGTM: initialize vars list.
Prevents destroy-time traversal issues.
99-116: LGTM: env->vars teardown covers all fields.
Safe iteration + sds destroys look correct.
219-223: LGTM: keep flb_env_set API as wrapper.
Backwards compatible behavior maintained.
| #define flb_cf_env_property_add(cf, name, name_len, value, value_len) \ | ||
| flb_cf_env_var_add(cf, name, name_len, value, value_len, NULL, 0, 0) |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Compatibility macro changes return type — likely API break.
Old flb_cf_env_property_add returned struct flb_kv*; the new macro returns struct flb_cf_env_var*. Call sites assigning to struct flb_kv* will fail.
Prefer a deprecated wrapper preserving the old signature to keep source/API compatibility:
-#define flb_cf_env_property_add(cf, name, name_len, value, value_len) \
- flb_cf_env_var_add(cf, name, name_len, value, value_len, NULL, 0, 0)
+/* Deprecated: kept for source/API compatibility. Returns NULL. */
+static inline struct flb_kv *flb_cf_env_property_add(struct flb_cf *cf,
+ char *name, size_t name_len,
+ char *value, size_t value_len)
+{
+ (void) flb_cf_env_var_add(cf, name, name_len, value, value_len, NULL, 0, 0);
+ return NULL;
+}If third parties may rely on the old exported symbol, also add a compat function in the .c to preserve ABI.
Preserve flb_cf_env_property_add API compatibility
flb_cf_env_property_add was changed from returning struct flb_kv* to invoking flb_cf_env_var_add (returning struct flb_cf_env_var*), which breaks third-party code expecting a flb_kv*. Supply a deprecated inline shim with the original signature:
-#define flb_cf_env_property_add(cf, name, name_len, value, value_len) \
- flb_cf_env_var_add(cf, name, name_len, value, value_len, NULL, 0, 0)
+/* Deprecated: kept for source/API compatibility (returns NULL). */
+static inline struct flb_kv *flb_cf_env_property_add(struct flb_cf *cf,
+ char *name, size_t name_len,
+ char *value, size_t value_len)
+{
+ (void) flb_cf_env_var_add(cf, name, name_len, value, value_len, NULL, 0, 0);
+ return NULL;
+}If ABI compatibility is required, also define and export a compat symbol in the .c implementation.
| int flb_config_map_set(struct flb_config *config, struct mk_list *properties, struct mk_list *map, void *context); | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
API break: flb_config_map_set now requires config; verify all call sites and out-of-tree impact
This is a source/ABI-breaking change for plugins/tests calling the old 3-arg form. Ensure all in-repo uses are updated and call out the break in release notes for external plugins.
🏁 Script executed:
#!/bin/bash
# List calls to flb_config_map_set with 3 vs 4 arguments.
# - Requires ast-grep in the runner.
echo "Calls with 3 args (old API):"
ast-grep --pattern $'flb_config_map_set($_, $_, $_)' || true
echo ""
echo "Calls with 4 args (new API):"
ast-grep --pattern $'flb_config_map_set($_, $_, $_, $_)' || trueLength of output: 58
Document API signature change for flb_config_map_set No in-repo calls remain using the old 3-arg form; update the project’s release notes to highlight this source/ABI break for external plugins.
| if (vlen > 7 && strncmp(value, "file://", 7) == 0) { | ||
| orig_uri = value; | ||
| vlen -= 7; | ||
| val += 7; | ||
| value += 7; | ||
|
|
||
| /* Check if the file exists */ | ||
| if (access(val, R_OK) == -1) { | ||
| flb_error("[env] file %s not found", val); | ||
| if (access(value, R_OK) == -1) { | ||
| flb_error("[env] file %s not found", value); | ||
| return -1; | ||
| } | ||
| /* Read the file content */ | ||
| fs_buf = flb_file_read(val); | ||
|
|
||
| fs_buf = flb_file_read(value); | ||
| if (!fs_buf) { | ||
| flb_error("[env] file %s could not be read", val); | ||
| flb_error("[env] file %s could not be read", value); | ||
| return -1; | ||
| } | ||
|
|
||
| /* Set the new value */ | ||
| val = fs_buf; | ||
| value = fs_buf; | ||
| vlen = flb_sds_len(fs_buf); | ||
|
|
||
| /* check if we have an ending \r or \n */ | ||
| if (vlen > 0 && (val[vlen - 1] == '\n' || val[vlen - 1] == '\r')) { | ||
| if (vlen > 0 && (value[vlen - 1] == '\n' || value[vlen - 1] == '\r')) { | ||
| vlen--; | ||
| flb_sds_len_set(fs_buf, vlen); | ||
| } | ||
|
|
||
| /* Check if the file content is empty */ | ||
| if (vlen == 0) { | ||
| flb_error("[env] file %s content is empty", val); | ||
| flb_error("[env] file %s content is empty", value); | ||
| flb_sds_destroy(fs_buf); | ||
| return -1; | ||
| } | ||
|
|
||
| flb_debug("[env] file %s content read propery, length= %d", val, vlen); | ||
| flb_debug("[env] file %s content read propery, length= %d", value, vlen); | ||
| } |
There was a problem hiding this comment.
Don’t reuse ‘value’ for both file path and file content; fix log placeholders and typo.
Using ‘value’ for content breaks error/debug logs and is fragile; also “propery” typo.
- if (vlen > 7 && strncmp(value, "file://", 7) == 0) {
- orig_uri = value;
- vlen -= 7;
- value += 7;
- if (access(value, R_OK) == -1) {
- flb_error("[env] file %s not found", value);
+ if (vlen > 7 && strncmp(value, "file://", 7) == 0) {
+ const char *file_path = value + 7;
+ orig_uri = value; /* keep full URI */
+ /* access() is TOCTOU-prone; rely on flb_file_read() below */
+ fs_buf = flb_file_read(file_path);
+ if (!fs_buf) {
+ flb_error("[env] file %s could not be read", file_path);
return -1;
}
-
- fs_buf = flb_file_read(value);
- if (!fs_buf) {
- flb_error("[env] file %s could not be read", value);
- return -1;
- }
-
- value = fs_buf;
+ value = fs_buf;
vlen = flb_sds_len(fs_buf);
- if (vlen > 0 && (value[vlen - 1] == '\n' || value[vlen - 1] == '\r')) {
+ if (vlen > 0 && (fs_buf[vlen - 1] == '\n' || fs_buf[vlen - 1] == '\r')) {
vlen--;
flb_sds_len_set(fs_buf, vlen);
}
if (vlen == 0) {
- flb_error("[env] file %s content is empty", value);
+ flb_error("[env] file %s content is empty", file_path);
flb_sds_destroy(fs_buf);
return -1;
}
- flb_debug("[env] file %s content read propery, length= %d", value, vlen);
+ flb_debug("[env] file %s content read properly, length=%d", file_path, vlen);
}📝 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.
| if (vlen > 7 && strncmp(value, "file://", 7) == 0) { | |
| orig_uri = value; | |
| vlen -= 7; | |
| val += 7; | |
| value += 7; | |
| /* Check if the file exists */ | |
| if (access(val, R_OK) == -1) { | |
| flb_error("[env] file %s not found", val); | |
| if (access(value, R_OK) == -1) { | |
| flb_error("[env] file %s not found", value); | |
| return -1; | |
| } | |
| /* Read the file content */ | |
| fs_buf = flb_file_read(val); | |
| fs_buf = flb_file_read(value); | |
| if (!fs_buf) { | |
| flb_error("[env] file %s could not be read", val); | |
| flb_error("[env] file %s could not be read", value); | |
| return -1; | |
| } | |
| /* Set the new value */ | |
| val = fs_buf; | |
| value = fs_buf; | |
| vlen = flb_sds_len(fs_buf); | |
| /* check if we have an ending \r or \n */ | |
| if (vlen > 0 && (val[vlen - 1] == '\n' || val[vlen - 1] == '\r')) { | |
| if (vlen > 0 && (value[vlen - 1] == '\n' || value[vlen - 1] == '\r')) { | |
| vlen--; | |
| flb_sds_len_set(fs_buf, vlen); | |
| } | |
| /* Check if the file content is empty */ | |
| if (vlen == 0) { | |
| flb_error("[env] file %s content is empty", val); | |
| flb_error("[env] file %s content is empty", value); | |
| flb_sds_destroy(fs_buf); | |
| return -1; | |
| } | |
| flb_debug("[env] file %s content read propery, length= %d", val, vlen); | |
| flb_debug("[env] file %s content read propery, length= %d", value, vlen); | |
| } | |
| if (vlen > 7 && strncmp(value, "file://", 7) == 0) { | |
| const char *file_path = value + 7; | |
| orig_uri = value; /* keep full URI */ | |
| /* access() is TOCTOU-prone; rely on flb_file_read() below */ | |
| fs_buf = flb_file_read(file_path); | |
| if (!fs_buf) { | |
| flb_error("[env] file %s could not be read", file_path); | |
| return -1; | |
| } | |
| value = fs_buf; | |
| vlen = flb_sds_len(fs_buf); | |
| if (vlen > 0 && (fs_buf[vlen - 1] == '\n' || fs_buf[vlen - 1] == '\r')) { | |
| vlen--; | |
| flb_sds_len_set(fs_buf, vlen); | |
| } | |
| if (vlen == 0) { | |
| flb_error("[env] file %s content is empty", file_path); | |
| flb_sds_destroy(fs_buf); | |
| return -1; | |
| } | |
| flb_debug("[env] file %s content read properly, length=%d", file_path, vlen); | |
| } |
| mk_list_foreach(head, &env->vars) { | ||
| var = mk_list_entry(head, struct flb_env_var, _head); | ||
| if (var->name && strcmp(var->name, key) == 0) { | ||
| if (var->uri && var->refresh_interval > 0) { | ||
| now = time(NULL); | ||
| if (var->last_refresh == 0 || | ||
| now - var->last_refresh >= var->refresh_interval) { | ||
| file = var->uri; | ||
| if (strncmp(file, "file://", 7) == 0) { | ||
| file += 7; | ||
| } | ||
|
|
||
| if (access(file, R_OK) == -1) { | ||
| flb_error("[env] file %s not found", file); | ||
| break; | ||
| } | ||
|
|
||
| fs_buf = flb_file_read(file); | ||
| if (!fs_buf) { | ||
| flb_error("[env] file %s could not be read", file); | ||
| break; | ||
| } | ||
|
|
||
| vlen = flb_sds_len(fs_buf); | ||
| if (vlen > 0 && (fs_buf[vlen - 1] == '\n' || fs_buf[vlen - 1] == '\r')) { | ||
| vlen--; | ||
| flb_sds_len_set(fs_buf, vlen); | ||
| } | ||
|
|
||
| if (vlen == 0) { | ||
| flb_error("[env] file %s content is empty", file); | ||
| flb_sds_destroy(fs_buf); | ||
| break; | ||
| } | ||
|
|
||
| flb_hash_table_del(env->ht, key); | ||
| if (var->value) { | ||
| flb_sds_destroy(var->value); | ||
| } | ||
| var->value = fs_buf; | ||
| ret = flb_hash_table_add(env->ht, key, len, fs_buf, vlen); | ||
| if (ret < 0) { | ||
| break; | ||
| } | ||
| var->last_refresh = now; | ||
| } | ||
| } | ||
| break; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unsynchronized HT writes during flb_env_get refresh — race risk.
flb_env_get mutates env->ht (del/add) without synchronization; concurrent readers/writers may race, corrupt HT, or observe partial state.
Guard refresh with a mutex in struct flb_env (init/destroy in create/destroy). Example critical section:
- if (var->uri && var->refresh_interval > 0) {
+ if (var->uri && var->refresh_interval > 0) {
now = time(NULL);
if (var->last_refresh == 0 ||
now - var->last_refresh >= var->refresh_interval) {
- file = var->uri;
+ file = var->uri;
if (strncmp(file, "file://", 7) == 0) {
file += 7;
}
- if (access(file, R_OK) == -1) {
- flb_error("[env] file %s not found", file);
- break;
- }
- fs_buf = flb_file_read(file);
+ fs_buf = flb_file_read(file);
if (!fs_buf) {
flb_error("[env] file %s could not be read", file);
break;
}
vlen = flb_sds_len(fs_buf);
if (vlen > 0 && (fs_buf[vlen - 1] == '\n' || fs_buf[vlen - 1] == '\r')) {
vlen--;
flb_sds_len_set(fs_buf, vlen);
}
if (vlen == 0) {
flb_error("[env] file %s content is empty", file);
flb_sds_destroy(fs_buf);
break;
}
- flb_hash_table_del(env->ht, key);
- if (var->value) {
- flb_sds_destroy(var->value);
- }
- var->value = fs_buf;
- ret = flb_hash_table_add(env->ht, key, len, fs_buf, vlen);
- if (ret < 0) {
- break;
- }
- var->last_refresh = now;
+ /* Begin critical section */
+ /* pthread_mutex_lock(&env->lock); */
+ do {
+ /* Prepare new SDS to keep HT independent if needed */
+ flb_sds_t newv = fs_buf;
+ fs_buf = NULL; /* ownership transferred */
+ flb_hash_table_del(env->ht, key);
+ if (var->value) {
+ flb_sds_destroy(var->value);
+ }
+ var->value = newv;
+ ret = flb_hash_table_add(env->ht, key, len, var->value, vlen);
+ if (ret < 0) {
+ /* restore previous state is out-of-scope; keep var->value to avoid leak */
+ break;
+ }
+ var->last_refresh = now;
+ } while (0);
+ /* pthread_mutex_unlock(&env->lock); */
}
}Also remove access() to avoid TOCTOU; rely on flb_file_read()’s failure.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/flb_env.c around lines 243 to 291, the code performs unsynchronized
modifications to env->ht (flb_hash_table_del / flb_hash_table_add) and uses
access() before reading the file, which risks races and TOCTOU; to fix it add a
mutex (e.g., pthread_mutex_t or existing flb_mutex_t) to struct flb_env,
initialize/destroy it in the env create/destroy paths, then wrap the entire
refresh critical section (from checking/reading/updating var->value and env->ht
and updating var->last_refresh) in mutex lock/unlock to serialize
readers/writers; also remove the access() check and instead rely on
flb_file_read() failure handling (log error and break) so the file is read
atomically without TOCTOU.
| /* Test file-based environment variable with refresh interval */ | ||
| void test_file_env_var_basic() | ||
| { | ||
| struct flb_env *env; | ||
| flb_sds_t buf = NULL; | ||
| char *test_file = "/tmp/flb_test_secret.txt"; | ||
| char *test_content = "secret_value_123"; | ||
| char *template = "${SECRET}"; | ||
| int fd; | ||
| int ret; | ||
|
|
||
| /* Create test file */ | ||
| fd = open(test_file, O_CREAT | O_WRONLY | O_TRUNC, 0644); | ||
| if (!TEST_CHECK(fd >= 0)) { | ||
| TEST_MSG("Failed to create test file"); | ||
| return; | ||
| } | ||
| ret = write(fd, test_content, strlen(test_content)); | ||
| close(fd); | ||
| if (!TEST_CHECK(ret == strlen(test_content))) { | ||
| TEST_MSG("Failed to write test content"); | ||
| unlink(test_file); | ||
| return; | ||
| } | ||
|
|
||
| /* Test environment variable loading */ | ||
| env = flb_env_create(); | ||
| if (!TEST_CHECK(env != NULL)) { | ||
| TEST_MSG("flb_env_create failed"); | ||
| unlink(test_file); | ||
| return; | ||
| } | ||
|
|
||
| /* Set file-based environment variable */ | ||
| ret = flb_env_set_extended(env, "SECRET", NULL, "file:///tmp/flb_test_secret.txt", 0); | ||
| if (!TEST_CHECK(ret == 0)) { | ||
| TEST_MSG("flb_env_set_extended failed"); | ||
| flb_env_destroy(env); | ||
| unlink(test_file); | ||
| return; | ||
| } | ||
|
|
||
| /* Test variable translation */ | ||
| buf = flb_env_var_translate(env, template); | ||
| if (!TEST_CHECK(buf != NULL)) { | ||
| TEST_MSG("flb_env_var_translate failed"); | ||
| flb_env_destroy(env); | ||
| unlink(test_file); | ||
| return; | ||
| } | ||
|
|
||
| if (!TEST_CHECK(strcmp(buf, test_content) == 0)) { | ||
| TEST_MSG("Content mismatch. Got=%s expect=%s", buf, test_content); | ||
| } | ||
|
|
||
| flb_sds_destroy(buf); | ||
| flb_env_destroy(env); | ||
| unlink(test_file); | ||
| } | ||
|
|
There was a problem hiding this comment.
Return code check too strict; allow non-negative id; consider unique temp files
flb_env_set_extended returns an id (>=0) or -1. Comparing to 0 can spuriously fail. Also, fixed /tmp path can cause collisions in parallel runs.
- ret = flb_env_set_extended(env, "SECRET", NULL, "file:///tmp/flb_test_secret.txt", 0);
- if (!TEST_CHECK(ret == 0)) {
+ ret = flb_env_set_extended(env, "SECRET", NULL, "file:///tmp/flb_test_secret.txt", 0);
+ if (!TEST_CHECK(ret >= 0)) {
TEST_MSG("flb_env_set_extended failed");
flb_env_destroy(env);
unlink(test_file);
return;
}📝 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.
| /* Test file-based environment variable with refresh interval */ | |
| void test_file_env_var_basic() | |
| { | |
| struct flb_env *env; | |
| flb_sds_t buf = NULL; | |
| char *test_file = "/tmp/flb_test_secret.txt"; | |
| char *test_content = "secret_value_123"; | |
| char *template = "${SECRET}"; | |
| int fd; | |
| int ret; | |
| /* Create test file */ | |
| fd = open(test_file, O_CREAT | O_WRONLY | O_TRUNC, 0644); | |
| if (!TEST_CHECK(fd >= 0)) { | |
| TEST_MSG("Failed to create test file"); | |
| return; | |
| } | |
| ret = write(fd, test_content, strlen(test_content)); | |
| close(fd); | |
| if (!TEST_CHECK(ret == strlen(test_content))) { | |
| TEST_MSG("Failed to write test content"); | |
| unlink(test_file); | |
| return; | |
| } | |
| /* Test environment variable loading */ | |
| env = flb_env_create(); | |
| if (!TEST_CHECK(env != NULL)) { | |
| TEST_MSG("flb_env_create failed"); | |
| unlink(test_file); | |
| return; | |
| } | |
| /* Set file-based environment variable */ | |
| ret = flb_env_set_extended(env, "SECRET", NULL, "file:///tmp/flb_test_secret.txt", 0); | |
| if (!TEST_CHECK(ret == 0)) { | |
| TEST_MSG("flb_env_set_extended failed"); | |
| flb_env_destroy(env); | |
| unlink(test_file); | |
| return; | |
| } | |
| /* Test variable translation */ | |
| buf = flb_env_var_translate(env, template); | |
| if (!TEST_CHECK(buf != NULL)) { | |
| TEST_MSG("flb_env_var_translate failed"); | |
| flb_env_destroy(env); | |
| unlink(test_file); | |
| return; | |
| } | |
| if (!TEST_CHECK(strcmp(buf, test_content) == 0)) { | |
| TEST_MSG("Content mismatch. Got=%s expect=%s", buf, test_content); | |
| } | |
| flb_sds_destroy(buf); | |
| flb_env_destroy(env); | |
| unlink(test_file); | |
| } | |
| /* Test file-based environment variable with refresh interval */ | |
| void test_file_env_var_basic() | |
| { | |
| struct flb_env *env; | |
| flb_sds_t buf = NULL; | |
| char *test_file = "/tmp/flb_test_secret.txt"; | |
| char *test_content = "secret_value_123"; | |
| char *template = "${SECRET}"; | |
| int fd; | |
| int ret; | |
| /* Create test file */ | |
| fd = open(test_file, O_CREAT | O_WRONLY | O_TRUNC, 0644); | |
| if (!TEST_CHECK(fd >= 0)) { | |
| TEST_MSG("Failed to create test file"); | |
| return; | |
| } | |
| ret = write(fd, test_content, strlen(test_content)); | |
| close(fd); | |
| if (!TEST_CHECK(ret == strlen(test_content))) { | |
| TEST_MSG("Failed to write test content"); | |
| unlink(test_file); | |
| return; | |
| } | |
| /* Test environment variable loading */ | |
| env = flb_env_create(); | |
| if (!TEST_CHECK(env != NULL)) { | |
| TEST_MSG("flb_env_create failed"); | |
| unlink(test_file); | |
| return; | |
| } | |
| /* Set file-based environment variable */ | |
| ret = flb_env_set_extended(env, "SECRET", NULL, | |
| "file:///tmp/flb_test_secret.txt", 0); | |
| if (!TEST_CHECK(ret >= 0)) { | |
| TEST_MSG("flb_env_set_extended failed"); | |
| flb_env_destroy(env); | |
| unlink(test_file); | |
| return; | |
| } | |
| /* Test variable translation */ | |
| buf = flb_env_var_translate(env, template); | |
| if (!TEST_CHECK(buf != NULL)) { | |
| TEST_MSG("flb_env_var_translate failed"); | |
| flb_env_destroy(env); | |
| unlink(test_file); | |
| return; | |
| } | |
| if (!TEST_CHECK(strcmp(buf, test_content) == 0)) { | |
| TEST_MSG("Content mismatch. Got=%s expect=%s", buf, test_content); | |
| } | |
| flb_sds_destroy(buf); | |
| flb_env_destroy(env); | |
| unlink(test_file); | |
| } |
🤖 Prompt for AI Agents
In tests/internal/env.c around lines 90 to 149, the test assumes
flb_env_set_extended returns 0 on success and uses a fixed /tmp filename; change
the success check to accept any non-negative id (ret >= 0) instead of ret == 0,
and replace the hard-coded "/tmp/flb_test_secret.txt" with a uniquely created
temporary file (e.g., use mkstemp to create a temp file, write the content to
the returned fd, close it, and pass the generated pathname to
flb_env_set_extended) ensuring you unlink the generated file and properly
destroy env on all error paths.
| /* Test file-based environment variable with file:// URI */ | ||
| void test_file_env_var_uri() | ||
| { | ||
| struct flb_env *env; | ||
| flb_sds_t buf = NULL; | ||
| char *test_file = "/tmp/flb_test_uri.txt"; | ||
| char *test_content = "uri_value_456"; | ||
| char *file_uri = "file:///tmp/flb_test_uri.txt"; | ||
| char *template = "${URI_VAR}"; | ||
| int fd; | ||
| int ret; | ||
|
|
||
| /* Create test file */ | ||
| fd = open(test_file, O_CREAT | O_WRONLY | O_TRUNC, 0644); | ||
| if (!TEST_CHECK(fd >= 0)) { | ||
| TEST_MSG("Failed to create test file"); | ||
| return; | ||
| } | ||
| ret = write(fd, test_content, strlen(test_content)); | ||
| close(fd); | ||
| if (!TEST_CHECK(ret == strlen(test_content))) { | ||
| TEST_MSG("Failed to write test content"); | ||
| unlink(test_file); | ||
| return; | ||
| } | ||
|
|
||
| /* Test environment variable loading with file:// URI */ | ||
| env = flb_env_create(); | ||
| if (!TEST_CHECK(env != NULL)) { | ||
| TEST_MSG("flb_env_create failed"); | ||
| unlink(test_file); | ||
| return; | ||
| } | ||
|
|
||
| /* Set file-based environment variable with file:// URI */ | ||
| ret = flb_env_set_extended(env, "URI_VAR", NULL, file_uri, 0); | ||
| if (!TEST_CHECK(ret == 0)) { | ||
| TEST_MSG("flb_env_set_extended failed"); | ||
| flb_env_destroy(env); | ||
| unlink(test_file); | ||
| return; | ||
| } | ||
|
|
||
| /* Test variable translation */ | ||
| buf = flb_env_var_translate(env, template); | ||
| if (!TEST_CHECK(buf != NULL)) { | ||
| TEST_MSG("flb_env_var_translate failed"); | ||
| flb_env_destroy(env); | ||
| unlink(test_file); | ||
| return; | ||
| } | ||
|
|
||
| if (!TEST_CHECK(strcmp(buf, test_content) == 0)) { | ||
| TEST_MSG("Content mismatch. Got=%s expect=%s", buf, test_content); | ||
| } | ||
|
|
||
| flb_sds_destroy(buf); | ||
| flb_env_destroy(env); | ||
| unlink(test_file); | ||
| } | ||
|
|
There was a problem hiding this comment.
Same retval semantics; use non-negative check
- ret = flb_env_set_extended(env, "URI_VAR", NULL, file_uri, 0);
- if (!TEST_CHECK(ret == 0)) {
+ ret = flb_env_set_extended(env, "URI_VAR", NULL, file_uri, 0);
+ if (!TEST_CHECK(ret >= 0)) {
TEST_MSG("flb_env_set_extended failed");
flb_env_destroy(env);
unlink(test_file);
return;
}📝 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.
| /* Test file-based environment variable with file:// URI */ | |
| void test_file_env_var_uri() | |
| { | |
| struct flb_env *env; | |
| flb_sds_t buf = NULL; | |
| char *test_file = "/tmp/flb_test_uri.txt"; | |
| char *test_content = "uri_value_456"; | |
| char *file_uri = "file:///tmp/flb_test_uri.txt"; | |
| char *template = "${URI_VAR}"; | |
| int fd; | |
| int ret; | |
| /* Create test file */ | |
| fd = open(test_file, O_CREAT | O_WRONLY | O_TRUNC, 0644); | |
| if (!TEST_CHECK(fd >= 0)) { | |
| TEST_MSG("Failed to create test file"); | |
| return; | |
| } | |
| ret = write(fd, test_content, strlen(test_content)); | |
| close(fd); | |
| if (!TEST_CHECK(ret == strlen(test_content))) { | |
| TEST_MSG("Failed to write test content"); | |
| unlink(test_file); | |
| return; | |
| } | |
| /* Test environment variable loading with file:// URI */ | |
| env = flb_env_create(); | |
| if (!TEST_CHECK(env != NULL)) { | |
| TEST_MSG("flb_env_create failed"); | |
| unlink(test_file); | |
| return; | |
| } | |
| /* Set file-based environment variable with file:// URI */ | |
| ret = flb_env_set_extended(env, "URI_VAR", NULL, file_uri, 0); | |
| if (!TEST_CHECK(ret == 0)) { | |
| TEST_MSG("flb_env_set_extended failed"); | |
| flb_env_destroy(env); | |
| unlink(test_file); | |
| return; | |
| } | |
| /* Test variable translation */ | |
| buf = flb_env_var_translate(env, template); | |
| if (!TEST_CHECK(buf != NULL)) { | |
| TEST_MSG("flb_env_var_translate failed"); | |
| flb_env_destroy(env); | |
| unlink(test_file); | |
| return; | |
| } | |
| if (!TEST_CHECK(strcmp(buf, test_content) == 0)) { | |
| TEST_MSG("Content mismatch. Got=%s expect=%s", buf, test_content); | |
| } | |
| flb_sds_destroy(buf); | |
| flb_env_destroy(env); | |
| unlink(test_file); | |
| } | |
| /* Set file-based environment variable with file:// URI */ | |
| ret = flb_env_set_extended(env, "URI_VAR", NULL, file_uri, 0); | |
| if (!TEST_CHECK(ret >= 0)) { | |
| TEST_MSG("flb_env_set_extended failed"); | |
| flb_env_destroy(env); | |
| unlink(test_file); | |
| return; | |
| } |
🤖 Prompt for AI Agents
In tests/internal/env.c around lines 245 to 305, the test uses strict equality
checks for functions that return non-negative success codes; change those
equality checks to non-negative checks. Specifically, replace the write() result
comparison from ret == strlen(test_content) to ret >= 0 (or ret >=
(ssize_t)strlen(test_content) if you want to ensure some bytes written) and
change the flb_env_set_extended() success check from ret == 0 to ret >= 0 so the
test accepts any non-negative success return value; keep the existing NULL and
>=0 check for open() as-is and leave other NULL checks unchanged.
| /* Test mixed static and dynamic environment variables */ | ||
| void test_mixed_env_vars() | ||
| { | ||
| struct flb_env *env; | ||
| flb_sds_t buf = NULL; | ||
| char *test_file = "/tmp/flb_test_mixed.txt"; | ||
| char *file_content = "dynamic_value"; | ||
| char *template = "Static: ${STATIC_VAR}, Dynamic: ${DYNAMIC_VAR}"; | ||
| char *expected = "Static: static_value, Dynamic: dynamic_value"; | ||
| int fd; | ||
| int ret; | ||
|
|
||
| /* Create test file */ | ||
| fd = open(test_file, O_CREAT | O_WRONLY | O_TRUNC, 0644); | ||
| if (!TEST_CHECK(fd >= 0)) { | ||
| TEST_MSG("Failed to create test file"); | ||
| return; | ||
| } | ||
| ret = write(fd, file_content, strlen(file_content)); | ||
| close(fd); | ||
| if (!TEST_CHECK(ret == strlen(file_content))) { | ||
| TEST_MSG("Failed to write test content"); | ||
| unlink(test_file); | ||
| return; | ||
| } | ||
|
|
||
| /* Test mixed environment variables */ | ||
| env = flb_env_create(); | ||
| if (!TEST_CHECK(env != NULL)) { | ||
| TEST_MSG("flb_env_create failed"); | ||
| unlink(test_file); | ||
| return; | ||
| } | ||
|
|
||
| /* Set static environment variable */ | ||
| ret = flb_env_set(env, "STATIC_VAR", "static_value"); | ||
| if (!TEST_CHECK(ret == 0)) { | ||
| TEST_MSG("flb_env_set failed for static variable"); | ||
| flb_env_destroy(env); | ||
| unlink(test_file); | ||
| return; | ||
| } | ||
|
|
||
| /* Set dynamic environment variable */ | ||
| ret = flb_env_set_extended(env, "DYNAMIC_VAR", NULL, "file:///tmp/flb_test_mixed.txt", 0); | ||
| if (!TEST_CHECK(ret == 0)) { | ||
| TEST_MSG("flb_env_set_extended failed for dynamic variable"); | ||
| flb_env_destroy(env); | ||
| unlink(test_file); | ||
| return; | ||
| } | ||
|
|
||
| /* Test mixed variable translation */ | ||
| buf = flb_env_var_translate(env, template); | ||
| if (!TEST_CHECK(buf != NULL)) { | ||
| TEST_MSG("flb_env_var_translate failed"); | ||
| flb_env_destroy(env); | ||
| unlink(test_file); | ||
| return; | ||
| } | ||
|
|
||
| if (!TEST_CHECK(strcmp(buf, expected) == 0)) { | ||
| TEST_MSG("Mixed content mismatch. Got=%s expect=%s", buf, expected); | ||
| } | ||
|
|
||
| flb_sds_destroy(buf); | ||
| flb_env_destroy(env); | ||
| unlink(test_file); | ||
| } | ||
|
|
There was a problem hiding this comment.
Same retval semantics for static and dynamic vars
- ret = flb_env_set(env, "STATIC_VAR", "static_value");
- if (!TEST_CHECK(ret == 0)) {
+ ret = flb_env_set(env, "STATIC_VAR", "static_value");
+ if (!TEST_CHECK(ret >= 0)) {
TEST_MSG("flb_env_set failed for static variable");
flb_env_destroy(env);
unlink(test_file);
return;
}
@@
- ret = flb_env_set_extended(env, "DYNAMIC_VAR", NULL, "file:///tmp/flb_test_mixed.txt", 0);
- if (!TEST_CHECK(ret == 0)) {
+ ret = flb_env_set_extended(env, "DYNAMIC_VAR", NULL, "file:///tmp/flb_test_mixed.txt", 0);
+ if (!TEST_CHECK(ret >= 0)) {
TEST_MSG("flb_env_set_extended failed for dynamic variable");
flb_env_destroy(env);
unlink(test_file);
return;
}📝 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.
| /* Test mixed static and dynamic environment variables */ | |
| void test_mixed_env_vars() | |
| { | |
| struct flb_env *env; | |
| flb_sds_t buf = NULL; | |
| char *test_file = "/tmp/flb_test_mixed.txt"; | |
| char *file_content = "dynamic_value"; | |
| char *template = "Static: ${STATIC_VAR}, Dynamic: ${DYNAMIC_VAR}"; | |
| char *expected = "Static: static_value, Dynamic: dynamic_value"; | |
| int fd; | |
| int ret; | |
| /* Create test file */ | |
| fd = open(test_file, O_CREAT | O_WRONLY | O_TRUNC, 0644); | |
| if (!TEST_CHECK(fd >= 0)) { | |
| TEST_MSG("Failed to create test file"); | |
| return; | |
| } | |
| ret = write(fd, file_content, strlen(file_content)); | |
| close(fd); | |
| if (!TEST_CHECK(ret == strlen(file_content))) { | |
| TEST_MSG("Failed to write test content"); | |
| unlink(test_file); | |
| return; | |
| } | |
| /* Test mixed environment variables */ | |
| env = flb_env_create(); | |
| if (!TEST_CHECK(env != NULL)) { | |
| TEST_MSG("flb_env_create failed"); | |
| unlink(test_file); | |
| return; | |
| } | |
| /* Set static environment variable */ | |
| ret = flb_env_set(env, "STATIC_VAR", "static_value"); | |
| if (!TEST_CHECK(ret == 0)) { | |
| TEST_MSG("flb_env_set failed for static variable"); | |
| flb_env_destroy(env); | |
| unlink(test_file); | |
| return; | |
| } | |
| /* Set dynamic environment variable */ | |
| ret = flb_env_set_extended(env, "DYNAMIC_VAR", NULL, "file:///tmp/flb_test_mixed.txt", 0); | |
| if (!TEST_CHECK(ret == 0)) { | |
| TEST_MSG("flb_env_set_extended failed for dynamic variable"); | |
| flb_env_destroy(env); | |
| unlink(test_file); | |
| return; | |
| } | |
| /* Test mixed variable translation */ | |
| buf = flb_env_var_translate(env, template); | |
| if (!TEST_CHECK(buf != NULL)) { | |
| TEST_MSG("flb_env_var_translate failed"); | |
| flb_env_destroy(env); | |
| unlink(test_file); | |
| return; | |
| } | |
| if (!TEST_CHECK(strcmp(buf, expected) == 0)) { | |
| TEST_MSG("Mixed content mismatch. Got=%s expect=%s", buf, expected); | |
| } | |
| flb_sds_destroy(buf); | |
| flb_env_destroy(env); | |
| unlink(test_file); | |
| } | |
| /* Set static environment variable */ | |
| ret = flb_env_set(env, "STATIC_VAR", "static_value"); | |
| if (!TEST_CHECK(ret >= 0)) { | |
| TEST_MSG("flb_env_set failed for static variable"); | |
| flb_env_destroy(env); | |
| unlink(test_file); | |
| return; | |
| } | |
| /* Set dynamic environment variable */ | |
| ret = flb_env_set_extended(env, "DYNAMIC_VAR", NULL, "file:///tmp/flb_test_mixed.txt", 0); | |
| if (!TEST_CHECK(ret >= 0)) { | |
| TEST_MSG("flb_env_set_extended failed for dynamic variable"); | |
| flb_env_destroy(env); | |
| unlink(test_file); | |
| return; | |
| } |
🤖 Prompt for AI Agents
In tests/internal/env.c around lines 306-375, the test assumes flb_env_set and
flb_env_set_extended share the same return-value semantics (0 on success,
non-zero on failure); update the implementation of flb_env_set_extended to match
flb_env_set by returning 0 on success and -1 (or a consistent non-zero error
code) on failure, and ensure any callers/tests expect that same convention.
Signed-off-by: Eduardo Silva <[email protected]>
Signed-off-by: Eduardo Silva <[email protected]>
edsiper
force-pushed
the
env-extended
branch
from
156425e to
ee16927
Compare
Signed-off-by: Eduardo Silva <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/in_dummy/in_dummy.c (1)
521-525: Fix compile error: unused-argument cast dereferences pointer
(void) *config;dereferences a pointer and will not compile. Use the standard unused-argument cast.static int in_dummy_exit(void *data, struct flb_config *config) { - (void) *config; + (void) config; struct flb_dummy *ctx = data;
🧹 Nitpick comments (8)
plugins/in_dummy/in_dummy.c (8)
120-141: Fallback to cached refs when env resolution yields empty templatesIf
${VAR}resolves to an empty string, parsing fails and event generation stops. Use the validated cached msgpack as a safe fallback instead of erroring.- if (resolved_body) { + if (resolved_body && flb_sds_len(resolved_body) > 0) { result = flb_pack_json(resolved_body, flb_sds_len(resolved_body), &body_msgpack, &body_msgpack_size, &root_type, NULL); - } - else { - result = 0; /* Using cached msgpack */ + } + else { + /* Using cached msgpack as fallback */ + body_msgpack = ctx->ref_body_msgpack; + body_msgpack_size = ctx->ref_body_msgpack_size; + result = 0; } - if (result == 0 && resolved_metadata) { + if (result == 0 && resolved_metadata && flb_sds_len(resolved_metadata) > 0) { result = flb_pack_json(resolved_metadata, flb_sds_len(resolved_metadata), &metadata_msgpack, &metadata_msgpack_size, &root_type, NULL); + } + else if (result == 0 && ctx->ref_metadata_msgpack) { + /* Using cached msgpack as fallback */ + metadata_msgpack = ctx->ref_metadata_msgpack; + metadata_msgpack_size = ctx->ref_metadata_msgpack_size; }
142-153: Improve failure log to aid diagnosisClarify which template failed. This makes field-side debugging faster.
- if (result != 0) { - flb_plg_error(ctx->ins, "failed to parse JSON template"); + if (result != 0) { + flb_plg_error(ctx->ins, "failed to parse JSON template (dummy and/or metadata)");
96-105: Avoid per-collect property lookups; use ctx->*_templateYou already stored
ctx->body_templateandctx->metadata_templateduring configure. Use them here to skip repeated property traversal.- /* Get the raw template from the property (should be raw when FLB_CONFIG_MAP_DYNAMIC_ENV is set) */ - body_template = flb_input_get_property("dummy", ctx->ins); - metadata_template = flb_input_get_property("metadata", ctx->ins); + /* Use templates captured at configure-time; env resolution remains dynamic */ + body_template = ctx->body_template; + metadata_template = ctx->metadata_template;
107-119: Skip env translation when no variables are presentFast-path: if a template has no “${…}”, avoid SDS alloc and translation. Requires <string.h>.
- resolved_body = flb_env_var_translate(ctx->ins->config->env, body_template); + resolved_body = (strstr(body_template, "${") != NULL) ? + flb_env_var_translate(ctx->ins->config->env, body_template) : + flb_sds_create(body_template); @@ - resolved_metadata = flb_env_var_translate(ctx->ins->config->env, metadata_template); + resolved_metadata = (strstr(metadata_template, "${") != NULL) ? + flb_env_var_translate(ctx->ins->config->env, metadata_template) : + flb_sds_create(metadata_template);Add header:
#include <stdlib.h> +#include <string.h>
90-98: Separate parse status from encoder status to avoid mixing conventions
flb_pack_jsonand the encoder both use 0 for success, but they’re different domains. Use distinct variables to prevent accidental logic bugs.- int result; + int result; /* encoder status */ + int pack_status; /* pack_json status */ @@ - while (result == FLB_EVENT_ENCODER_SUCCESS && + while (result == FLB_EVENT_ENCODER_SUCCESS && msgpack_unpack_next(&object, body_msgpack, body_msgpack_size, &chunk_offset) == MSGPACK_UNPACK_SUCCESS) {And apply locally in the pack paths:
- result = flb_pack_json(resolved_body, ... + pack_status = flb_pack_json(resolved_body, ... ... - if (result == 0 && resolved_metadata && flb_sds_len(resolved_metadata) > 0) { - result = flb_pack_json(resolved_metadata, ... + if (pack_status == 0 && resolved_metadata && flb_sds_len(resolved_metadata) > 0) { + pack_status = flb_pack_json(resolved_metadata, ...(Keep
resultstrictly for encoder return codes.)Also applies to: 155-161
252-254: Fix log typoMinor user-facing typo.
- flb_plg_error(ins, "log chunk genartion error (%d)", result); + flb_plg_error(ins, "log chunk generation error (%d)", result);
566-569: Fix description typo“collectd” → “collect”.
- "set the number of copies to generate per collectd." + "set the number of copies to generate per collect."
192-206: Cleanup simplification (optional)Since per-collect you always allocate new msgpack (and now also fallback to cached explicitly), the pointer inequality checks aren’t needed; free only when you allocated, else skip. Current code is correct; this is just a readability tweak.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugins/in_dummy/in_dummy.c(12 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
plugins/in_dummy/in_dummy.c (5)
src/flb_input.c (1)
flb_input_get_property(777-781)src/flb_env.c (1)
flb_env_var_translate(315-428)src/flb_sds.c (2)
flb_sds_create(78-90)flb_sds_destroy(389-399)src/flb_pack.c (1)
flb_pack_json(335-341)include/fluent-bit/flb_mem.h (1)
flb_free(126-128)
⏰ 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). (29)
- 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_SANITIZE_MEMORY=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, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=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=Off, 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 (-DSANITIZE_UNDEFINED=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_COVERAGE=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_SMALL=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, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, gcc, g++)
- GitHub Check: pr-compile-centos-7
- 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-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-22.04, clang-12)
- 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)
🔇 Additional comments (1)
plugins/in_dummy/in_dummy.c (1)
107-116: No NULL-guard needed — config->env is initialized in flb_config_init().
config->env is set via config->env = flb_env_create() (src/flb_config.c:375) and torn down in flb_config_exit; only add a guard if flb_config instances may be created without flb_config_init().
Signed-off-by: Eduardo Silva <[email protected]>
Signed-off-by: Eduardo Silva <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
include/fluent-bit/config_format/flb_cf.h (1)
152-157: Public API removal breaks source/ABI; add a deprecated compat shim for flb_cf_env_property_addflb_cf_env_property_add disappeared/changed shape. External code that previously returned/used struct flb_kv* will break at compile time (source) and possibly at link/runtime (ABI) if the symbol existed. Please add a deprecated wrapper preserving the old signature and, if ABI matters, a real symbol in the .c file too.
Header-level shim (source-compat):
+/* Deprecated: kept for source compatibility. Prefer flb_cf_env_var_add(). + * Returns NULL; callers should migrate to the new API. */ +static inline struct flb_kv *flb_cf_env_property_add(struct flb_cf *cf, + char *k_buf, size_t k_len, + char *v_buf, size_t v_len) +{ + (void) flb_cf_env_var_add(cf, k_buf, k_len, v_buf, v_len, NULL, 0, 0); + return NULL; +}For ABI-compatibility, also export a non-inline function in src/config_format/flb_config_format.c with the same body/signature so the old symbol remains resolvable.
To gauge blast radius in-tree:
#!/bin/bash # Find call sites relying on the old API rg -nP '\bflb_cf_env_property_add\s*\(' -C2
🧹 Nitpick comments (4)
include/fluent-bit/config_format/flb_cf.h (4)
93-95: Nit: tighten the comment wording and intentSuggest minor wording cleanup and make it explicit this list holds env-var entries parsed from YAML.
Apply:
- /* config environment variables (env section, available on YAML) */ - struct mk_list env; /* list of struct flb_cf_env_var */ + /* environment variables (env section, available in YAML) */ + struct mk_list env; /* list of struct flb_cf_env_var entries */
144-151: Document semantics and consider types for future-proofing of env-var model
- Please document ownership/lifetime of name/value/uri (who frees) and units/semantics of refresh_interval (e.g., seconds; 0 = disabled).
- Consider using uint32_t for refresh_interval and noting its unit inline; it avoids negative values and clarifies ABI.
- Optional: If you anticipate evolving this struct, consider making it opaque (forward-declare and expose accessors) to avoid ABI churn later.
Proposed tweaks:
-struct flb_cf_env_var { +struct flb_cf_env_var { flb_sds_t name; flb_sds_t value; flb_sds_t uri; - int refresh_interval; + /* refresh cadence in seconds; 0 = disabled */ + uint32_t refresh_interval; struct mk_list _head; };If you keep int, adding the “seconds; 0 = disabled” note is still valuable.
152-157: Const-correct the new API and document 0-length “auto” behaviorname/value/uri input buffers are not mutated; mark them const. Also, the implementation auto-derives lengths when len == 0—document that in the header so callers don’t guess.
Apply:
-struct flb_cf_env_var *flb_cf_env_var_add(struct flb_cf *cf, - char *name, size_t name_len, - char *value, size_t value_len, - char *uri, size_t uri_len, - int refresh_interval); +/* Note: if *_len == 0 and pointer != NULL, length is derived via strlen(). */ +struct flb_cf_env_var *flb_cf_env_var_add(struct flb_cf *cf, + const char *name, size_t name_len, + const char *value, size_t value_len, + const char *uri, size_t uri_len, + int refresh_interval);
93-95: Add destroy helpers to avoid leaks and clarify ownershipThere’s a public “add” API but no corresponding destroy/destroy_all in the header. Ensure flb_cf_destroy frees cf->env list, and consider exposing env-var destroy helpers for symmetry and plugin authors.
Propose adding:
+/* env-var lifecycle */ +void flb_cf_env_var_destroy(struct flb_cf_env_var *ev); +void flb_cf_env_destroy_all(struct flb_cf *cf);And document that flb_cf_destroy calls flb_cf_env_destroy_all(cf).
Also applies to: 144-157
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
include/fluent-bit/config_format/flb_cf.h(2 hunks)plugins/in_dummy/in_dummy.c(12 hunks)src/config_format/flb_cf_yaml.c(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/config_format/flb_cf_yaml.c
- plugins/in_dummy/in_dummy.c
🧰 Additional context used
🧬 Code graph analysis (1)
include/fluent-bit/config_format/flb_cf.h (1)
src/config_format/flb_config_format.c (1)
flb_cf_env_var_add(441-504)
⏰ 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). (29)
- 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: 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, clang, clang++, ubuntu-22.04, clang-12)
- 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, gcc, g++, ubuntu-24.04, clang-14)
- GitHub Check: pr-compile-centos-7
- 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 (-DFLB_SIMD=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_SANITIZE_THREAD=On, 3.31.6, clang, clang++)
- 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, 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 (-DSANITIZE_ADDRESS=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_JEMALLOC=Off, 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_SMALL=On, 3.31.6, clang, clang++)
- 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, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=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_JEMALLOC=On, 3.31.6, gcc, g++)
- GitHub Check: PR - fuzzing test
Signed-off-by: Eduardo Silva <[email protected]>
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
Chores