es_out: support Upstream Servers with configuration overriding #7608
Conversation
mabrarov
requested review from
PettitWesley,
edsiper,
niedbalski,
leonardo-albertovich,
fujimotos and
koleini
as code owners
mabrarov
force-pushed
the
feature/out_es_upstream_support_extended
branch
from
812020d to
ba3382a
Compare
|
Hi reviewers, Is it possible to approve only workflow for this pull request, so that automated checks and build can start? Thank you. |
|
@mabrarov sure |
|
Hi @PettitWesley, It looks like all failed checks are around run-macos-unit-tests jobs and caused by the following failed unit tests:
I feel like other pull requests have the same issues, i.e. it doesn't seem that the failed checks are caused by this pull request changes. Help of maintainers is appreciated. Thank you. |
mabrarov
force-pushed
the
feature/out_es_upstream_support_extended
branch
from
ba3382a to
b7cd81b
Compare
|
Hi @PettitWesley, Is it possible to trigger automated workflow (build) for this pull request one more time? I found & fixed one issue and added tests for the new code since last build happened. Thank you. |
|
Hi dear reviewers, Is it possible to get this pull request reviewed / accepted sooner? Is there something pending / waiting from my side to start review? Thank you. |
mabrarov
force-pushed
the
feature/out_es_upstream_support_extended
branch
from
b7cd81b to
b81d3f7
Compare
|
Hi @PettitWesley and @edsiper, It feels like you are code owners for Elasticsearch output plugin. Is there something pending / waiting from my side to start review of this pull request? This new feature was requested 4 years ago and I feel it is something which multiple users of Fluent Bit (not just my team) would like to have. Thank you. |
mabrarov
force-pushed
the
feature/out_es_upstream_support_extended
branch
from
b81d3f7 to
f6431c2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
plugins/out_es/es.c (2)
242-279: Fix potential overflow and pointer arithmetic in compose_index_headersnprintf return handling is incorrect: truncation occurs when ret >= available. Using strlen(separator_str) to advance p/len is also unsafe on truncation (it advances past the written bytes). This can produce out-of-bounds pointer advances and corrupt the buffer.
Apply this diff to handle lengths correctly:
static int compose_index_header(struct flb_elasticsearch_config *ec, int es_index_custom_len, char *logstash_index, size_t logstash_index_size, char *separator_str, struct tm *tm) { int ret; int len; char *p; size_t s; /* Compose Index header */ if (es_index_custom_len > 0) { p = logstash_index + es_index_custom_len; } else { - p = logstash_index + flb_sds_len(ec->logstash_prefix); + p = logstash_index + flb_sds_len(ec->logstash_prefix); } len = p - logstash_index; - ret = snprintf(p, logstash_index_size - len, "%s", - separator_str); - if (ret > logstash_index_size - len) { + { + size_t avail = logstash_index_size - len; + ret = snprintf(p, avail, "%s", separator_str); + if ((size_t) ret >= avail) { /* exceed limit */ - return -1; + return -1; + } } - p += strlen(separator_str); - len += strlen(separator_str); + p += ret; + len += ret; s = strftime(p, logstash_index_size - len, ec->logstash_dateformat, tm); if (s==0) { /* exceed limit */ return -1; } p += s; *p++ = '\0'; return 0; }
1038-1049: Free AWS SigV4 signature on retry path to avoid leaksignature is freed on success but leaked on error paths that jump to retry.
retry: flb_http_client_destroy(c); flb_free(pack); if (out_buf != pack) { flb_free(out_buf); } + if (signature) { + flb_sds_destroy(signature); + signature = NULL; + } + flb_upstream_conn_release(u_conn); FLB_OUTPUT_RETURN(FLB_RETRY);
♻️ Duplicate comments (2)
plugins/out_es/es.c (1)
648-670: Target selection helper is clean and contract-drivenHA-aware target selection via flb_elasticsearch_target is straightforward and avoids extra branching in callers. Given the internal-function contract approach agreed earlier, omitting NULL checks here is fine.
plugins/out_es/es.h (1)
178-193: Good: contract documented for flb_elasticsearch_targetThe comment explicitly documents the non-NULL ctx/node contract and return semantics. This addresses previous debate and improves maintainability for internal callers.
🧹 Nitpick comments (6)
include/fluent-bit/flb_lib.h (2)
25-25: Prefer a forward declaration over including flb_input.h in a public headerPulling flb_input.h into flb_lib.h widens public header dependencies and can slow builds or leak internals unnecessarily. Since the prototype only needs a pointer to struct flb_input_instance, a forward declaration suffices.
Apply this diff within this line range:
-#include <fluent-bit/flb_input.h>And add this forward declaration near the existing forward decls (e.g., after Line 50 alongside struct flb_processor;):
/* Forward declarations for pointer-only API types used in public signatures */ struct flb_input_instance;
74-80: Introduce a typedef for the flush_ctx_callback signature to improve readability and reuseThe inline function-pointer type is verbose and error-prone to replicate. A typedef will improve readability and make future uses consistent.
Apply this diff to the prototype to use a typedef:
FLB_EXPORT int flb_output_set_test_flush_ctx_callback(flb_ctx_t *ctx, int ffd, char *test_name, - void *(*flush_ctx_callback)( - struct flb_config *, - struct flb_input_instance *, - void *, void *), + flb_test_flush_ctx_callback_t flush_ctx_callback, void *flush_ctx);Then add this typedef near other public typedefs (e.g., after Line 50):
typedef void *(*flb_test_flush_ctx_callback_t)(struct flb_config *, struct flb_input_instance *, void *, void *);plugins/out_es/es.c (3)
20-22: Remove duplicate msgpack includemsgpack.h is included twice in this file (Line 21 and Line 37). Drop one to avoid redundancy.
-#include <msgpack.h>
1065-1069: Improve log message when no target config is availableThe warning says "http_do=%i" which is misleading at this point and uses ret which hasn’t been set by an HTTP call here. Use a clearer message.
- flb_plg_warn(ctx->ins, "http_do=%i", ret); + flb_plg_warn(ctx->ins, "no Elasticsearch target available (HA=%s)", + ctx->ha_mode == FLB_TRUE ? "enabled" : "disabled");
462-475: Handle compose_index_header failure robustlyIf compose_index_header fails and the fallback attempt also fails, the function proceeds with a partially composed index, leading to malformed requests. Consider checking the fallback result and aborting record formatting if both attempts fail.
Would you like me to propose an error-handling diff that unwinds tmp_sbuf/j_index/bulk safely from here?
plugins/out_es/es.h (1)
48-162: Ownership flags scale concern: consider a small wrapper to reduce noiseThe own_* fields clearly solve double-free/ownership tracking across shared plugin/upstream-node configs. However, the proliferation harms readability and maintainability.
Consider introducing lightweight wrappers to pair pointer-like fields with an ownership bit, e.g.:
- For SDS: struct flb_owned_sds { flb_sds_t val; uint8_t own; };
- For raw pointers: struct flb_owned_ptr { void *ptr; uint8_t own; };
You can then define helper accessors/macros to keep flb_config_map offsetof compatibility (e.g., mapping to .val), while centralizing destroy/free logic. This reduces the number of standalone own_* members and lowers the risk of mismatched updates during future changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
include/fluent-bit/flb_lib.h(2 hunks)plugins/out_es/es.c(32 hunks)plugins/out_es/es.h(5 hunks)src/flb_lib.c(2 hunks)tests/runtime/out_elasticsearch.c(19 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/flb_lib.c
- tests/runtime/out_elasticsearch.c
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-20T02:04:27.220Z
Learnt from: mabrarov
PR: fluent/fluent-bit#7608
File: plugins/out_es/es.c:648-670
Timestamp: 2025-08-20T02:04:27.220Z
Learning: The author mabrarov prefers function contracts that assume valid parameters for internal functions in the Elasticsearch output plugin, rather than adding defensive null pointer checks, to maintain performance and code clarity.
Applied to files:
plugins/out_es/es.c
🧬 Code Graph Analysis (3)
plugins/out_es/es.c (5)
src/flb_record_accessor.c (2)
flb_ra_get_value_object(803-814)flb_ra_translate(628-633)plugins/out_es/es_conf.c (1)
flb_es_upstream_conf(932-946)src/flb_upstream_ha.c (1)
flb_upstream_ha_node_get(88-110)src/flb_upstream.c (1)
flb_upstream_conn_get(700-833)src/flb_http_client.c (7)
flb_http_client(800-845)flb_http_buffer_size(858-869)flb_http_add_header(949-981)flb_http_basic_auth(1297-1301)flb_http_dummy_client(780-798)flb_http_set_response_test(1149-1182)flb_http_do(1522-1571)
include/fluent-bit/flb_lib.h (1)
src/flb_lib.c (1)
flb_output_set_test_flush_ctx_callback(612-644)
plugins/out_es/es.h (1)
plugins/out_es/es.c (1)
flb_elasticsearch_target(648-670)
⏰ 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). (4)
- GitHub Check: run-windows-unit-tests / call-build-windows-unit-test (Windows 32bit, x86, x86-windows-static, 3.31.6)
- GitHub Check: PR - container builds / Windows container images (2025)
- GitHub Check: PR - container builds / Windows container images (2022)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, clang, clang++)
🔇 Additional comments (3)
include/fluent-bit/flb_lib.h (2)
69-74: Param rename to flush_ctx is clearer and ABI-safeRenaming the last parameter of flb_output_set_test to flush_ctx aligns its purpose with the new callback mechanism without changing the ABI. Looks good.
69-80: All API signatures are consistent across header, implementation, and tests
Verified thatflb_output_set_testandflb_output_set_test_flush_ctx_callbackdefinitions in src/flb_lib.c match their declarations in include/fluent-bit/flb_lib.h, and all test call sites use the updated prototypes correctly.plugins/out_es/es.h (1)
23-26: Includes align with new HA/multi-upstream designAdding mk_list and flb_upstream_node headers is appropriate for tracking per-node configs and HA nodes.
| /* Compose HTTP Client request (dummy client) */ | ||
| c = flb_http_dummy_client(u_conn, FLB_HTTP_POST, ctx->uri, | ||
| c = flb_http_dummy_client(u_conn, FLB_HTTP_POST, ec->uri, | ||
| NULL, 0, NULL, 0, NULL, 0); | ||
|
|
||
| flb_http_buffer_size(c, ctx->buffer_size); | ||
| flb_http_buffer_size(c, ec->buffer_size); | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Check dummy client creation in test path
flb_http_dummy_client may return NULL; add a guard before using c.
- c = flb_http_dummy_client(u_conn, FLB_HTTP_POST, ec->uri,
- NULL, 0, NULL, 0, NULL, 0);
-
- flb_http_buffer_size(c, ec->buffer_size);
+ c = flb_http_dummy_client(u_conn, FLB_HTTP_POST, ec->uri,
+ NULL, 0, NULL, 0, NULL, 0);
+ if (c == NULL) {
+ flb_plg_error(ctx->ins, "failed to create dummy HTTP client");
+ return -2;
+ }
+ flb_http_buffer_size(c, ec->buffer_size);Here’s a quick search to locate both the call site and the function prototype/definition for flb_http_dummy_client:
🏁 Script executed:
#!/bin/bash
set -e
# Find all occurrences (calls + definition) in .c files
rg -n "flb_http_dummy_client" -C3 --glob "*.c"
# Find its declaration in headers
rg -n "flb_http_dummy_client" -C3 --glob "*.h"Length of output: 1750
Here’s the flb_http_dummy_client implementation for inspection:
🏁 Script executed:
#!/bin/bash
set -e
# Extract dummy client function definition for review
sed -n '760,860p' src/flb_http_client.cLength of output: 3044
🏁 Script executed:
#!/bin/bash
set -e
# Show context around the dummy client usage in es.c
sed -n '1050,1150p' plugins/out_es/es.cLength of output: 3229
Here’s the implementation of create_http_client, which is invoked by flb_http_dummy_client. It confirms that flb_http_dummy_client can indeed return NULL, and there is no upstream‐null guard inside:
🏁 Script executed:
#!/bin/bash
set -e
# Show definition of create_http_client
rg -nP "^struct flb_http_client \\*create_http_client" -C10 src/flb_http_client.cLength of output: 862
Guard against NULL dummy client in elasticsearch_response_test
flb_http_dummy_client can return NULL; dereferencing c in flb_http_buffer_size will crash. Add a null check and error return before sizing the buffer.
• File: plugins/out_es/es.c, function elasticsearch_response_test (around line 1075)
• Ensure you log an error and return -2 on failure to create the dummy client
- /* Compose HTTP Client request (dummy client) */
- c = flb_http_dummy_client(u_conn, FLB_HTTP_POST, ec->uri,
- NULL, 0, NULL, 0, NULL, 0);
-
- flb_http_buffer_size(c, ec->buffer_size);
+ /* Compose HTTP Client request (dummy client) */
+ c = flb_http_dummy_client(u_conn, FLB_HTTP_POST, ec->uri,
+ NULL, 0, NULL, 0, NULL, 0);
+ if (!c) {
+ flb_plg_error(ctx->ins,
+ "failed to create dummy HTTP client for URI=%s",
+ ec->uri);
+ return -2;
+ }
+ flb_http_buffer_size(c, ec->buffer_size);📝 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.
| /* Compose HTTP Client request (dummy client) */ | |
| c = flb_http_dummy_client(u_conn, FLB_HTTP_POST, ctx->uri, | |
| c = flb_http_dummy_client(u_conn, FLB_HTTP_POST, ec->uri, | |
| NULL, 0, NULL, 0, NULL, 0); | |
| flb_http_buffer_size(c, ctx->buffer_size); | |
| flb_http_buffer_size(c, ec->buffer_size); | |
| /* Compose HTTP Client request (dummy client) */ | |
| c = flb_http_dummy_client(u_conn, FLB_HTTP_POST, ec->uri, | |
| NULL, 0, NULL, 0, NULL, 0); | |
| if (!c) { | |
| flb_plg_error(ctx->ins, | |
| "failed to create dummy HTTP client for URI=%s", | |
| ec->uri); | |
| return -2; | |
| } | |
| flb_http_buffer_size(c, ec->buffer_size); |
🤖 Prompt for AI Agents
In plugins/out_es/es.c around lines 1074 to 1079, flb_http_dummy_client can
return NULL so dereferencing `c` in the subsequent flb_http_buffer_size call can
crash; add a NULL check immediately after flb_http_dummy_client, log an error if
creation failed (using the plugin/logger in this file), and return -2 to abort
the elasticsearch_response_test before calling flb_http_buffer_size.
2d0f7ff to
91d9b05
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
plugins/out_es/es.c (4)
648-670: LGTM! The upstream targeting function is well-structuredThe function properly handles both HA and simple modes, returning the appropriate configuration context. The contract that parameters cannot be NULL is reasonable for internal functions where all call sites are controlled.
1074-1079: Add NULL check for dummy HTTP client creation/* Compose HTTP Client request (dummy client) */ c = flb_http_dummy_client(u_conn, FLB_HTTP_POST, ec->uri, NULL, 0, NULL, 0, NULL, 0); +if (!c) { + flb_plg_error(ctx->ins, "failed to create dummy HTTP client"); + return -2; +} flb_http_buffer_size(c, ec->buffer_size);
911-916: Guard against NULL HTTP client creationThe lack of NULL check after
flb_http_clientis a pre-existing issue that should be addressed in a separate PR to avoid expanding this PR's scope.
1065-1070: Guard against NULL ec in elasticsearch_response_testThe function dereferences
ecwithout checking ifflb_elasticsearch_targetreturned NULL. Whileflb_elasticsearch_targetreturns NULL when no node is available, the subsequent code at line 1075 would crash.ec = flb_elasticsearch_target(ctx, &node); if (!ec) { - flb_plg_warn(ctx->ins, "http_do=%i", ret); + flb_plg_error(ctx->ins, "no upstream target available"); return -2; }
🧹 Nitpick comments (6)
include/fluent-bit/flb_lib.h (3)
25-25: Use a forward declaration forflb_input_instanceinstead of including its headerThe
include/fluent-bit/flb_lib.hheader only usesstruct flb_input_instance*, so you can safely remove the public include and add a forward declaration to minimize coupling and prevent potential circular dependencies.• File:
include/fluent-bit/flb_lib.h
• Location: remove the include at line 25, add the forward declaration alongside existing onesSuggested diff:
- #include <fluent-bit/flb_input.h> /* Forward declarations */ struct flb_processor; +struct flb_input_instance;
74-81: Add documentation for flush_ctx ownership and introduce a typedef for the callback
- Verified that the new fields are correctly wired through:
• In src/flb_lib.c,o_ins->test_formatter.flush_ctxandflush_ctx_callbackare set at lines 636–637.
• In flb_engine_dispatch.c, the callback is invoked withflush_ctx(lines 117–118).- No header-cycle risk detected: include/fluent-bit/flb_input.h does not
#include <fluent-bit/flb_lib.h>.Please update as follows:
• Document ownership/lifetime in
include/fluent-bit/flb_lib.h, e.g.:/** * flb_output_set_test_flush_ctx_callback * * @ctx Fluent Bit context (owned by caller) * @ffd formatter ID * @test_name name of the test (caller‐owned string) * @flush_ctx_callback * optional callback that may produce a new context; * the returned pointer must be allocated/freed by the caller * on the same thread as invocation. * @flush_ctx seed pointer passed as-is to the callback * * Returns 0 on success, -1 on error. */• Optional: make
test_nameconst-correct:-FLB_EXPORT int flb_output_set_test_flush_ctx_callback(flb_ctx_t *ctx, int ffd, - char *test_name, +FLB_EXPORT int flb_output_set_test_flush_ctx_callback(flb_ctx_t *ctx, int ffd, + const char *test_name, void *(*flush_ctx_callback)( struct flb_config *, struct flb_input_instance *, void *, void *), void *flush_ctx);• Introduce a public typedef to simplify the callback signature:
/* In include/fluent-bit/flb_lib.h or a shared test-header */ typedef void *(*flb_test_flush_ctx_cb)( struct flb_config *, struct flb_input_instance *, void * /* out_callback_data */, void * /* flush_ctx seed */ );
69-74: Enhance const-correctness of flb_output_set_test signatureWe’ve confirmed that every call to flb_output_set_test passes a string literal for test_name and always supplies NULL for the new flush_ctx parameter. Updating the API to take const char * for test_name is safe and requires no call-site changes.
• include/fluent-bit/flb_lib.h (line 69): change char *test_name → const char *test_name
• src/flb_lib.c (line 579): change char *test_name → const char *test_nameProposed diff:
--- include/fluent-bit/flb_lib.h @@ -69,7 +69,7 @@ -FLB_EXPORT int flb_output_set_test(flb_ctx_t *ctx, int ffd, char *test_name, +FLB_EXPORT int flb_output_set_test(flb_ctx_t *ctx, int ffd, const char *test_name, void (*out_callback) (void *, int, int, void *, size_t, void *), void *out_callback_data, --- src/flb_lib.c @@ -579,7 +579,7 @@ -int flb_output_set_test(flb_ctx_t *ctx, int ffd, char *test_name, +int flb_output_set_test(flb_ctx_t *ctx, int ffd, const char *test_name, void (*out_callback) (void *, int, int, void *, size_t, void *), void *out_callback_data,No updates to existing test calls are needed.
plugins/out_es/es.c (1)
20-21: Remove duplicate msgpack.h includeThe header
msgpack.his included twice (lines 21 and 37).#include <time.h> -#include <msgpack.h> #include <fluent-bit/flb_output_plugin.h>plugins/out_es/es_conf.c (2)
34-39: Consider using#definemacros for string constantsUsing string literals directly could lead to typos and maintenance issues. Consider defining these as macros in a header file for consistency.
-static const char * const es_default_path = ""; -static const char * const es_write_op_index = FLB_ES_WRITE_OP_INDEX; -static const char * const es_write_op_create = FLB_ES_WRITE_OP_CREATE; -static const char * const es_write_op_update = FLB_ES_WRITE_OP_UPDATE; -static const char * const es_write_op_upsert = FLB_ES_WRITE_OP_UPSERT; +#define ES_DEFAULT_PATH "" +#define ES_WRITE_OP_INDEX FLB_ES_WRITE_OP_INDEX +#define ES_WRITE_OP_CREATE FLB_ES_WRITE_OP_CREATE +#define ES_WRITE_OP_UPDATE FLB_ES_WRITE_OP_UPDATE +#define ES_WRITE_OP_UPSERT FLB_ES_WRITE_OP_UPSERTThen use the macros:
- path = es_default_path; + path = ES_DEFAULT_PATH;
302-327: Document the ownership tracking patternThe ownership flags (
own_xxx) are a departure from typical Fluent Bit patterns but serve a valid purpose for preventing double-free issues when configuration data is shared between plugin-level and per-upstream-node contexts. Consider adding a comment block explaining this ownership model./* Copy base configuration */ *ec = *base; +/* + * Reset ownership flags to prevent double-free of shared pointers. + * When a per-node configuration shares a value with the base configuration, + * only the owner should free it. These flags track ownership to ensure + * proper cleanup without memory leaks or double-free errors. + */ ec->own_index = FLB_FALSE;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
include/fluent-bit/flb_lib.h(2 hunks)include/fluent-bit/flb_output.h(1 hunks)plugins/out_es/CMakeLists.txt(1 hunks)plugins/out_es/es.c(32 hunks)plugins/out_es/es.h(5 hunks)plugins/out_es/es_conf.c(1 hunks)plugins/out_es/es_conf.h(1 hunks)plugins/out_es/es_conf_parse.c(1 hunks)plugins/out_es/es_conf_parse.h(1 hunks)plugins/out_es/es_conf_prop.h(1 hunks)src/flb_engine_dispatch.c(2 hunks)src/flb_lib.c(2 hunks)tests/runtime/out_elasticsearch.c(19 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- plugins/out_es/es_conf_parse.h
- src/flb_lib.c
- plugins/out_es/es_conf_prop.h
- tests/runtime/out_elasticsearch.c
- include/fluent-bit/flb_output.h
- plugins/out_es/CMakeLists.txt
- src/flb_engine_dispatch.c
- plugins/out_es/es_conf.h
- plugins/out_es/es_conf_parse.c
- plugins/out_es/es.h
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-20T02:04:27.220Z
Learnt from: mabrarov
PR: fluent/fluent-bit#7608
File: plugins/out_es/es.c:648-670
Timestamp: 2025-08-20T02:04:27.220Z
Learning: The author mabrarov prefers function contracts that assume valid parameters for internal functions in the Elasticsearch output plugin, rather than adding defensive null pointer checks, to maintain performance and code clarity.
Applied to files:
plugins/out_es/es.c
🧬 Code graph analysis (3)
plugins/out_es/es_conf.c (10)
src/flb_record_accessor.c (2)
flb_ra_create(271-358)flb_ra_destroy(232-248)include/fluent-bit/flb_mem.h (2)
flb_free(126-128)flb_calloc(84-96)src/flb_uri.c (1)
flb_uri_get(72-85)plugins/out_es/es_conf_parse.c (4)
flb_es_conf_set_cloud_auth(134-184)flb_es_conf_set_cloud_credentials(36-65)flb_es_set_aws_unsigned_headers(188-207)flb_es_conf_set_aws_provider(271-336)src/flb_output.c (4)
flb_output_get_property(1096-1099)flb_output_upstream_ha_set(1649-1661)flb_output_net_default(1059-1069)flb_output_upstream_set(1556-1647)src/flb_utils.c (2)
flb_utils_bool(671-685)flb_utils_size_to_bytes(527-608)src/flb_sds.c (2)
flb_sds_create(78-90)flb_sds_destroy(389-399)src/flb_upstream_ha.c (2)
flb_upstream_ha_from_file(356-514)flb_upstream_ha_destroy(63-78)include/fluent-bit/flb_upstream_node.h (2)
flb_upstream_node_set_data(82-86)flb_upstream_node_get_data(88-91)src/flb_upstream.c (2)
flb_upstream_create(290-363)flb_upstream_destroy(649-687)
include/fluent-bit/flb_lib.h (2)
include/fluent-bit/flb_output.h (1)
int(579-582)src/flb_lib.c (1)
flb_output_set_test_flush_ctx_callback(612-644)
plugins/out_es/es.c (6)
src/flb_record_accessor.c (2)
flb_ra_get_value_object(803-814)flb_ra_translate(628-633)include/fluent-bit/flb_compat.h (1)
gmtime_r(75-81)plugins/out_es/es_conf.c (1)
flb_es_upstream_conf(932-946)src/flb_upstream_ha.c (1)
flb_upstream_ha_node_get(88-110)src/flb_upstream.c (1)
flb_upstream_conn_get(700-833)src/flb_http_client.c (7)
flb_http_client(800-845)flb_http_buffer_size(858-869)flb_http_add_header(949-981)flb_http_basic_auth(1297-1301)flb_http_dummy_client(780-798)flb_http_set_response_test(1149-1182)flb_http_do(1522-1571)
⏰ 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). (70)
- GitHub Check: PR - container builds / Windows container images (2025)
- GitHub Check: PR - container builds / Windows container images (2022)
- GitHub Check: PR - packages build Linux / raspbian/bookworm package build and stage to S3
- GitHub Check: PR - packages build Linux / debian/bullseye.arm64v8 package build and stage to S3
- GitHub Check: PR - packages build Linux / almalinux/9 package build and stage to S3
- GitHub Check: PR - packages build Linux / debian/bullseye package build and stage to S3
- GitHub Check: PR - packages build Linux / ubuntu/22.04.arm64v8 package build and stage to S3
- GitHub Check: PR - packages build Linux / debian/bookworm.arm64v8 package build and stage to S3
- GitHub Check: PR - packages build Linux / ubuntu/24.04.arm64v8 package build and stage to S3
- GitHub Check: PR - packages build Linux / ubuntu/24.04 package build and stage to S3
- GitHub Check: PR - packages build Linux / ubuntu/22.04 package build and stage to S3
- GitHub Check: PR - packages build Linux / debian/bookworm package build and stage to S3
- GitHub Check: PR - packages build Linux / almalinux/8.arm64v8 package build and stage to S3
- GitHub Check: PR - packages build Linux / rockylinux/9.arm64v8 package build and stage to S3
- GitHub Check: PR - packages build Linux / rockylinux/9 package build and stage to S3
- GitHub Check: PR - packages build Linux / debian/buster.arm64v8 package build and stage to S3
- GitHub Check: PR - packages build Linux / almalinux/9.arm64v8 package build and stage to S3
- GitHub Check: PR - packages build Linux / debian/buster package build and stage to S3
- GitHub Check: PR - packages build Linux / centos/7.arm64v8 package build and stage to S3
- GitHub Check: PR - packages build Linux / almalinux/8 package build and stage to S3
- GitHub Check: PR - packages build Linux / centos/9 package build and stage to S3
- GitHub Check: PR - packages build Linux / rockylinux/8 package build and stage to S3
- GitHub Check: PR - packages build Linux / centos/9.arm64v8 package build and stage to S3
- GitHub Check: PR - packages build Linux / centos/8.arm64v8 package build and stage to S3
- GitHub Check: PR - packages build Linux / centos/7 package build and stage to S3
- GitHub Check: PR - packages build Linux / rockylinux/8.arm64v8 package build and stage to S3
- GitHub Check: PR - packages build Linux / amazonlinux/2.arm64v8 package build and stage to S3
- GitHub Check: PR - packages build Linux / amazonlinux/2 package build and stage to S3
- GitHub Check: PR - packages build Linux / centos/8 package build and stage to S3
- GitHub Check: PR - packages build Linux / amazonlinux/2023.arm64v8 package build and stage to S3
- GitHub Check: PR - packages build Linux / amazonlinux/2023 package build and stage to S3
- GitHub Check: PR - container builds / amd64/debug container image build
- GitHub Check: PR - container builds / arm/v7/production container image build
- GitHub Check: PR - container builds / arm64/debug container image build
- GitHub Check: PR - container builds / arm/v7/debug container image build
- GitHub Check: PR - container builds / amd64/production container image build
- GitHub Check: PR - container builds / arm64/production container image build
- GitHub Check: PR - packages build MacOS / call-build-macos-package (Apple Silicon macOS runner, macos-14, 3.31.6)
- GitHub Check: PR - packages build MacOS / call-build-macos-package (Intel macOS runner, macos-14-large, 3.31.6)
- GitHub Check: PR - packages build Windows / call-build-windows-package (Windows 32bit, x86, x86-windows-static, 3.31.6)
- GitHub Check: PR - packages build Windows / call-build-windows-package (Windows 64bit (Arm64), amd64_arm64, -DCMAKE_SYSTEM_NAME=Windows -DCMA...
- GitHub Check: PR - packages build Windows / call-build-windows-package (Windows 64bit, x64, x64-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: 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_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_COVERAGE=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_MEMORY=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 (-DSANITIZE_ADDRESS=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_SANITIZE_MEMORY=On, 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 (-DFLB_JEMALLOC=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, 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: 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
- 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 - fuzzing test
🔇 Additional comments (6)
plugins/out_es/es.c (2)
851-872: LGTM! Proper HA-aware upstream connection handlingThe code correctly differentiates between HA mode and simple mode when obtaining upstream connections. The error messages clearly indicate which mode failed, making debugging easier.
1359-1363: LGTM! Upstream configuration property properly integratedThe new
upstreamconfiguration property is correctly placed and documented, following the existing pattern for configuration options.plugins/out_es/es_conf.c (4)
413-418: LGTM! Proper error handling for string allocationThe code correctly checks for allocation failure and returns an error immediately, preventing potential NULL pointer dereferences.
593-679: LGTM! Comprehensive cleanup function with ownership checksThe
elasticsearch_config_destroyfunction properly checks ownership flags before freeing resources, preventing double-free issues. The systematic cleanup of all allocated resources is well-structured.
681-790: LGTM! Well-structured HA configuration with proper error handlingThe
es_config_hafunction has excellent error handling with proper cleanup paths. The rollback logic when configuration fails for any node ensures no partial state is left behind.
932-946: LGTM! Clean API for upstream configuration retrievalThe
flb_es_upstream_conffunction provides a clean interface for retrieving the appropriate configuration based on whether HA mode is enabled and whether a specific node is targeted.
…wn to parser of Upstream node configuration section are implemented, e.g. "host" and "port" Signed-off-by: Marat Abrarov <[email protected]>
…o the test callback based on configuration of Fluent Bit and based on configuration of plugin Signed-off-by: Marat Abrarov <[email protected]>
…llback contract Signed-off-by: Marat Abrarov <[email protected]>
…with Upstream node configuration For Elastic cloud authentication these parameters are always taken from plugin configuration and never from Upstream node configuration: cloud_id. For AWS authentication these parameters are always taken from plugin configuration and never from Upstream node configuration: http_proxy, no_proxy, tls*. Signed-off-by: Marat Abrarov <[email protected]>
Signed-off-by: Marat Abrarov <[email protected]>
…o the test callback based on configuration of Fluent Bit and based on configuration of plugin Signed-off-by: Marat Abrarov <[email protected]>
Signed-off-by: Marat Abrarov <[email protected]>
mabrarov
force-pushed
the
feature/out_es_upstream_support_extended
branch
from
91d9b05 to
2ceb6d2
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/out_es/es.c (1)
1038-1049: Memory leak: signature not freed on retry path.signature (SigV4 header) is leaked if execution jumps to retry. Free it on both paths.
retry: - flb_http_client_destroy(c); + flb_http_client_destroy(c); + if (signature) { + flb_sds_destroy(signature); + signature = NULL; + } flb_free(pack);
♻️ Duplicate comments (5)
plugins/out_es/es_conf_parse.c (1)
124-132: Unsafe strcat/strcpy; possible buffer overflow when composing cloud host.Use bounded formatting or SDS to avoid overflow in cloud_host_buf.
- strcpy(cloud_host_buf, host); - strcat(cloud_host_buf, "."); - strcat(cloud_host_buf, region); - if (port != NULL) { - strcat(cloud_host_buf, ":"); - strcat(cloud_host_buf, port); - } - return flb_sds_create(cloud_host_buf); + if (port) { + if (snprintf(cloud_host_buf, sizeof(cloud_host_buf), + "%s.%s:%s", host, region, port) >= (int) sizeof(cloud_host_buf)) { + return NULL; + } + } + else { + if (snprintf(cloud_host_buf, sizeof(cloud_host_buf), + "%s.%s", host, region) >= (int) sizeof(cloud_host_buf)) { + return NULL; + } + } + return flb_sds_create(cloud_host_buf);plugins/out_es/es.c (2)
1074-1079: Guard against NULL dummy client in response test.flb_http_dummy_client can return NULL; add a check before flb_http_buffer_size.
- c = flb_http_dummy_client(u_conn, FLB_HTTP_POST, ec->uri, - NULL, 0, NULL, 0, NULL, 0); - - flb_http_buffer_size(c, ec->buffer_size); + c = flb_http_dummy_client(u_conn, FLB_HTTP_POST, ec->uri, + NULL, 0, NULL, 0, NULL, 0); + if (!c) { + flb_plg_error(ctx->ins, "failed to create dummy HTTP client"); + return -2; + } + flb_http_buffer_size(c, ec->buffer_size);
911-916: Guard against NULL HTTP client creation.flb_http_client can return NULL; following flb_http_buffer_size dereferences it.
- c = flb_http_client(u_conn, FLB_HTTP_POST, ec->uri, - pack, pack_size, NULL, 0, NULL, 0); - - flb_http_buffer_size(c, ec->buffer_size); + c = flb_http_client(u_conn, FLB_HTTP_POST, ec->uri, + pack, pack_size, NULL, 0, NULL, 0); + if (!c) { + flb_plg_error(ctx->ins, "failed to create HTTP client"); + goto retry; + } + flb_http_buffer_size(c, ec->buffer_size);tests/runtime/out_elasticsearch.c (1)
23-44: Avoid tmpnam(NULL); use mkstemp for safe temp files (tests).tmpnam is insecure and returns a static buffer. Use mkstemp and fdopen, return a heap-duplicated filename for later unlink.
- upstream_conf_filename = tmpnam(NULL); - if (upstream_conf_filename == NULL) { - return NULL; - } - upstream_conf_filename = flb_strdup(upstream_conf_filename); - if (upstream_conf_filename == NULL) { - return NULL; - } - - upstream_conf_file = fopen(upstream_conf_filename, "w"); + char tmpl[] = "/tmp/flb_es_upstream_XXXXXX"; + int fd = mkstemp(tmpl); + if (fd == -1) { + return NULL; + } + upstream_conf_filename = flb_strdup(tmpl); + if (!upstream_conf_filename) { + close(fd); + unlink(tmpl); + return NULL; + } + upstream_conf_file = fdopen(fd, "w"); if (upstream_conf_file == NULL) { - flb_free(upstream_conf_filename); + close(fd); + unlink(upstream_conf_filename); + flb_free(upstream_conf_filename); return NULL; }Add required headers at the top:
+#include <stdlib.h> +#include <unistd.h>plugins/out_es/es.h (1)
48-56: Ownership flags proliferation hurts maintainability; compress or centralize.The per-field
own_*booleans across the struct are noisy and error-prone. Consider either:
- A single packed bitfield struct (e.g.,
struct { unsigned index:1; ... } own;), or- A single bitmask (enum +
uint64_t owns) with helpers.This reduces footprint and the chance of mismatched frees while preserving your current semantics.
🧹 Nitpick comments (13)
plugins/out_es/es_conf.c (3)
582-591: Strengthen config validation message and branches (optional).The validation is correct. Consider clarifying the error to mention which option must be enabled or which field is missing to aid operators.
295-315: Ownership flags reset: OK, but guard boolean parsing failures.When overriding from node properties, flb_utils_bool may return -1 for invalid values. Today those are assigned directly. Consider treating -1 as FLB_FALSE or logging a warning to avoid propagating invalid states.
490-503: Record accessor creation for logstash_prefix_key: set ownership after success.own_ra_prefix_key is set before verifying flb_ra_create result. Move ownership flag assignment after successful creation to keep state consistent on failure paths.
- ec->ra_prefix_key = flb_ra_create(buf, FLB_TRUE); - ec->own_ra_prefix_key = FLB_TRUE; + ec->ra_prefix_key = flb_ra_create(buf, FLB_TRUE); flb_free(buf); + if (!ec->ra_prefix_key) { + flb_errno(); + return -1; + } + ec->own_ra_prefix_key = FLB_TRUE;plugins/out_es/es.c (2)
648-670: Target selection helper: contract is fine. Document non-NULL preconditions.Given prior feedback, add a brief comment documenting that ctx and node must be non-NULL for internal callers.
242-271: compose_index_header: fix snprintf bound and size calculation (optional).Use >= on the snprintf check to catch exact-bound truncation.
- if (ret > logstash_index_size - len) { + if (ret >= (int)(logstash_index_size - len)) { return -1; }plugins/out_es/es.h (8)
61-66: Document borrow/own semantics for cloud auth fields.Please add a brief comment stating whether
cloud_user/cloud_passwdmay alias plugin-level values and thatown_*guards deallocation. This prevents future double-free regressions.
109-114: SDS fields are shared; ensure they are treated read-only or dup on override.If per-node configs alias
logstash_prefix/logstash_prefix_separator, any in-place SDS mutation would bleed across nodes. Either guarantee these are immutable after parse or duplicate on override.
116-130: Same immutability concern for prefix_key/dateformat/time_key/time_key_format.Please confirm these SDS are not mutated post-parse. If they can change, dup-per-node to avoid cross-node side effects.
135-139: es_action lifetime: ensure it points to static storage or per-config owned memory.You switched to
const char *. Good. Verify it’s always set to string literals (static) or memory owned by the sameflb_elasticsearch_configlifetime. Add a short comment to codify this contract.- /* write operation elasticsearch operation */ - const char *es_action; + /* write operation elasticsearch action; points to static literal or memory + * owned for the lifetime of this config; never free. */ + const char *es_action;
141-156: Record accessor ownership flags: add explicit free contract.For
ra_id_keyandra_prefix_key, add a note that whenown_*is set these must be destroyed with the correct RA free routine and not via generic free.
164-169: HA members: tighten types and clarify ownership of ha_upstream.
- Consider
uint8_tforha_mode.- Document who frees
ha_upstream(path string) in the destroy path to avoid leaks.- int ha_mode; /* High Availability mode enabled ? */ + uint8_t ha_mode; /* High Availability mode enabled ? */
171-173: Initialize configs list in all modes.Ensure
mk_list_init(&ctx->configs)is called during init even in single-upstream mode to avoid undefined iteration in common paths.
178-193: Makenodeoptional in API or update docs.The doc says “Non-NULL output parameter.” Allowing callers to pass NULL when they don’t need the node simplifies usage. Either:
- Support NULL
nodein the implementation, or- Keep the requirement and add an assertion.
Also, consider returning a tri-state error (e.g., “no node available” vs “error”) instead of NULL for both.
- * @param node Non-NULL output parameter for selected upstream node. + * @param node Optional output parameter for selected upstream node (may be NULL).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
include/fluent-bit/flb_lib.h(2 hunks)include/fluent-bit/flb_output.h(1 hunks)plugins/out_es/CMakeLists.txt(1 hunks)plugins/out_es/es.c(32 hunks)plugins/out_es/es.h(5 hunks)plugins/out_es/es_conf.c(1 hunks)plugins/out_es/es_conf.h(1 hunks)plugins/out_es/es_conf_parse.c(1 hunks)plugins/out_es/es_conf_parse.h(1 hunks)plugins/out_es/es_conf_prop.h(1 hunks)src/flb_engine_dispatch.c(2 hunks)src/flb_lib.c(2 hunks)tests/runtime/out_elasticsearch.c(19 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- plugins/out_es/CMakeLists.txt
- src/flb_lib.c
- src/flb_engine_dispatch.c
- plugins/out_es/es_conf.h
- include/fluent-bit/flb_output.h
- plugins/out_es/es_conf_prop.h
- plugins/out_es/es_conf_parse.h
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-20T02:04:27.220Z
Learnt from: mabrarov
PR: fluent/fluent-bit#7608
File: plugins/out_es/es.c:648-670
Timestamp: 2025-08-20T02:04:27.220Z
Learning: The author mabrarov prefers function contracts that assume valid parameters for internal functions in the Elasticsearch output plugin, rather than adding defensive null pointer checks, to maintain performance and code clarity.
Applied to files:
plugins/out_es/es.c
🧬 Code graph analysis (3)
plugins/out_es/es_conf.c (12)
src/flb_record_accessor.c (2)
flb_ra_create(271-358)flb_ra_destroy(232-248)include/fluent-bit/flb_mem.h (2)
flb_free(126-128)flb_calloc(84-96)src/flb_uri.c (1)
flb_uri_get(72-85)plugins/out_es/es_conf_parse.c (4)
flb_es_conf_set_cloud_auth(134-184)flb_es_conf_set_cloud_credentials(36-65)flb_es_set_aws_unsigned_headers(188-207)flb_es_conf_set_aws_provider(271-336)src/flb_output.c (4)
flb_output_get_property(1096-1099)flb_output_upstream_ha_set(1649-1661)flb_output_net_default(1059-1069)flb_output_upstream_set(1556-1647)src/flb_upstream_node.c (1)
flb_upstream_node_get_property(183-199)src/flb_utils.c (2)
flb_utils_bool(671-685)flb_utils_size_to_bytes(527-608)src/flb_sds.c (2)
flb_sds_create(78-90)flb_sds_destroy(389-399)src/aws/flb_aws_credentials.c (1)
flb_aws_provider_destroy(769-791)src/tls/flb_tls.c (1)
flb_tls_destroy(258-277)src/flb_upstream_ha.c (2)
flb_upstream_ha_from_file(356-514)flb_upstream_ha_destroy(63-78)include/fluent-bit/flb_upstream_node.h (2)
flb_upstream_node_set_data(82-86)flb_upstream_node_get_data(88-91)
plugins/out_es/es_conf_parse.c (6)
src/flb_utils.c (2)
flb_utils_split(464-467)flb_utils_split_free(477-489)src/flb_sds.c (2)
flb_sds_create(78-90)flb_sds_destroy(389-399)src/tls/flb_tls.c (1)
flb_tls_create(183-232)include/fluent-bit/flb_mem.h (1)
flb_free(126-128)src/aws/flb_aws_util.c (1)
flb_aws_client_generator(280-283)src/aws/flb_aws_credentials.c (1)
flb_standard_chain_provider_create(269-327)
tests/runtime/out_elasticsearch.c (2)
include/fluent-bit/flb_mem.h (1)
flb_free(126-128)src/flb_lib.c (6)
flb_output_set_test_flush_ctx_callback(612-644)flb_lib_push(808-835)flb_input(261-271)flb_output(274-284)flb_output_set(515-546)flb_output_set_test(579-610)
⏰ 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). (71)
- GitHub Check: PR - container builds / Windows container images (2025)
- GitHub Check: PR - container builds / Windows container images (2022)
- GitHub Check: PR - packages build Linux / almalinux/9 package build and stage to S3
- GitHub Check: PR - packages build Linux / debian/bullseye.arm64v8 package build and stage to S3
- GitHub Check: PR - packages build Linux / raspbian/bookworm package build and stage to S3
- GitHub Check: PR - packages build Linux / ubuntu/22.04.arm64v8 package build and stage to S3
- GitHub Check: PR - packages build Linux / ubuntu/22.04 package build and stage to S3
- GitHub Check: PR - packages build Linux / ubuntu/24.04.arm64v8 package build and stage to S3
- GitHub Check: PR - packages build Linux / ubuntu/24.04 package build and stage to S3
- GitHub Check: PR - packages build Linux / centos/8.arm64v8 package build and stage to S3
- GitHub Check: PR - packages build Linux / debian/bullseye package build and stage to S3
- GitHub Check: PR - packages build Linux / rockylinux/9 package build and stage to S3
- GitHub Check: PR - packages build Linux / debian/bookworm.arm64v8 package build and stage to S3
- GitHub Check: PR - packages build Linux / almalinux/8.arm64v8 package build and stage to S3
- GitHub Check: PR - packages build Linux / centos/7 package build and stage to S3
- GitHub Check: PR - packages build Linux / almalinux/9.arm64v8 package build and stage to S3
- GitHub Check: PR - packages build Linux / debian/buster.arm64v8 package build and stage to S3
- GitHub Check: PR - packages build Linux / debian/bookworm package build and stage to S3
- GitHub Check: PR - packages build Linux / rockylinux/9.arm64v8 package build and stage to S3
- GitHub Check: PR - packages build Linux / debian/buster package build and stage to S3
- GitHub Check: PR - packages build Linux / rockylinux/8.arm64v8 package build and stage to S3
- GitHub Check: PR - packages build Linux / rockylinux/8 package build and stage to S3
- GitHub Check: PR - packages build Linux / amazonlinux/2.arm64v8 package build and stage to S3
- GitHub Check: PR - packages build Linux / centos/9.arm64v8 package build and stage to S3
- GitHub Check: PR - packages build Linux / centos/9 package build and stage to S3
- GitHub Check: PR - packages build Linux / almalinux/8 package build and stage to S3
- GitHub Check: PR - packages build Linux / centos/8 package build and stage to S3
- GitHub Check: PR - packages build Linux / amazonlinux/2023 package build and stage to S3
- GitHub Check: PR - packages build Linux / centos/7.arm64v8 package build and stage to S3
- GitHub Check: PR - packages build Linux / amazonlinux/2 package build and stage to S3
- GitHub Check: PR - packages build Linux / amazonlinux/2023.arm64v8 package build and stage to S3
- GitHub Check: PR - container builds / arm/v7/production container image build
- GitHub Check: PR - container builds / arm/v7/debug container image build
- GitHub Check: PR - container builds / amd64/debug container image build
- GitHub Check: PR - container builds / arm64/production container image build
- GitHub Check: PR - container builds / arm64/debug container image build
- GitHub Check: PR - container builds / amd64/production container image build
- GitHub Check: PR - packages build MacOS / call-build-macos-package (Apple Silicon macOS runner, macos-14, 3.31.6)
- GitHub Check: PR - packages build MacOS / call-build-macos-package (Intel macOS runner, macos-14-large, 3.31.6)
- GitHub Check: PR - packages build Windows / call-build-windows-package (Windows 32bit, x86, x86-windows-static, 3.31.6)
- GitHub Check: PR - packages build Windows / call-build-windows-package (Windows 64bit, x64, x64-windows-static, 3.31.6)
- GitHub Check: PR - packages build Windows / 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 (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-24.04, clang-14)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-22.04, clang-12)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-24.04, clang-14)
- GitHub Check: pr-compile-centos-7
- 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: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=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_JEMALLOC=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_SANITIZE_THREAD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_COVERAGE=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 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 (-DSANITIZE_ADDRESS=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_UNDEFINED=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=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_SANITIZE_MEMORY=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, clang, clang++)
🔇 Additional comments (8)
include/fluent-bit/flb_lib.h (1)
69-81: New test API: verify symbol export and documentation.Ensure flb_output_set_test_flush_ctx_callback is exported on all platforms (e.g., Windows .def/.map) and added to user-facing docs alongside flb_output_set_test.
Would you like a script to scan export lists and API docs for the new symbol?
plugins/out_es/es_conf.c (1)
133-137: snprintf truncation check is incorrect; fix boundary condition.ret should be compared against the buffer size used (sizeof(ec->uri) - 1), not sizeof(ec->uri). Current check may miss truncation.
- if (ret < 0 || ret >= sizeof(ec->uri)) { + if (ret < 0 || ret >= (int)(sizeof(ec->uri) - 1)) { return -1; }Likely an incorrect or invalid review comment.
plugins/out_es/es_conf_parse.c (1)
145-182: Transfer ownership of cloud_host: OK. Minor: prefer const-correct types.extract_cloud_host returns flb_sds_t; variable should be flb_sds_t for clarity.
[ suggest_nitpick ]
tests/runtime/out_elasticsearch.c (2)
335-335: Good addition: tests correctly register flush-ctx callback.The formatter tests now supply per-flush ec via the new callback; covers upstream override paths.
Also applies to: 382-382, 430-433, 479-483, 526-530, 573-576, 623-626, 671-676, 722-725, 770-773, 818-821, 1009-1013, 1061-1064, 1119-1123, 1135-1138, 1191-1193, 1250-1252, 1303-1306, 1365-1368, 1421-1424
1135-1138: Ensure temp files are freed in all error branches.You clean up at normal exit; double-check early-return paths also unlink/free to avoid littering /tmp on test failures.
[ suggest_nitpick ]
Also applies to: 1191-1193, 1250-1252, 1319-1321, 1381-1383, 1437-1439
plugins/out_es/es.h (3)
28-28: Default port corrected to 9200 — LGTM.Accurate and matches Elasticsearch defaults.
160-162: List hook for configs — LGTM.This cleanly enables per-config linkage under HA.
68-85: Ignore runtime-invariant helper for AWS providers
Providers and base-providers are always distinct pointers and are destroyed only when their respectiveown_*flag is set, so there’s no drift or double-free. Theaws_unsigned_headerslist is locally allocated and unconditionally destroyed on teardown (theown_aws_unsigned_headersflag is unused)—you may remove that flag if desired.Likely an incorrect or invalid review comment.
| @@ -22,6 +22,7 @@ | |||
|
|
|||
| #include <fluent-bit/flb_macros.h> | |||
| #include <fluent-bit/flb_config.h> | |||
| #include <fluent-bit/flb_input.h> | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid pulling internal headers into the public API; forward-declare instead.
Including flb_input.h in a public header expands API surface and coupling. Forward-declare struct flb_input_instance and remove the include.
-#include <fluent-bit/flb_input.h>
+/* forward declaration to avoid exposing internal header here */
+struct flb_input_instance;📝 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.
| #include <fluent-bit/flb_input.h> | |
| /* forward declaration to avoid exposing internal header here */ | |
| struct flb_input_instance; |
🤖 Prompt for AI Agents
In include/fluent-bit/flb_lib.h around line 25, remove the internal include
'#include <fluent-bit/flb_input.h>' and instead forward-declare 'struct
flb_input_instance;' at the top of the header; update any use sites in this
header to reference only pointers to that struct (e.g., struct
flb_input_instance *), ensuring no in-header dereferences or inline uses that
require the full definition, and keep the full include in source files or
private headers where the struct layout is needed.
| int flb_es_conf_set_cloud_credentials(const char *cloud_auth, | ||
| struct flb_elasticsearch_config *ec) | ||
| { | ||
| /* extract strings */ | ||
| int items = 0; | ||
| struct mk_list *toks; | ||
| struct mk_list *head; | ||
| struct flb_split_entry *entry; | ||
|
|
||
| if (!cloud_auth) { | ||
| return 0; | ||
| } | ||
|
|
||
| toks = flb_utils_split((const char *)cloud_auth, ':', -1); | ||
| mk_list_foreach(head, toks) { | ||
| items++; | ||
| entry = mk_list_entry(head, struct flb_split_entry, _head); | ||
| if (items == 1) { | ||
| ec->cloud_user = flb_strdup(entry->value); | ||
| ec->own_cloud_user = FLB_TRUE; | ||
| } | ||
| if (items == 2) { | ||
| ec->cloud_passwd = flb_strdup(entry->value); | ||
| ec->own_cloud_passwd = FLB_TRUE; | ||
| } | ||
| } | ||
| flb_utils_split_free(toks); | ||
|
|
||
| return 0; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle OOM when duplicating cloud credentials.
flb_strdup can return NULL; return -1 and cleanup on partial allocation to avoid inconsistent state.
- toks = flb_utils_split((const char *)cloud_auth, ':', -1);
+ toks = flb_utils_split((const char *)cloud_auth, ':', -1);
mk_list_foreach(head, toks) {
items++;
entry = mk_list_entry(head, struct flb_split_entry, _head);
if (items == 1) {
- ec->cloud_user = flb_strdup(entry->value);
- ec->own_cloud_user = FLB_TRUE;
+ ec->cloud_user = flb_strdup(entry->value);
+ if (!ec->cloud_user) { flb_utils_split_free(toks); return -1; }
+ ec->own_cloud_user = FLB_TRUE;
}
if (items == 2) {
- ec->cloud_passwd = flb_strdup(entry->value);
- ec->own_cloud_passwd = FLB_TRUE;
+ ec->cloud_passwd = flb_strdup(entry->value);
+ if (!ec->cloud_passwd) { flb_utils_split_free(toks); return -1; }
+ ec->own_cloud_passwd = FLB_TRUE;
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In plugins/out_es/es_conf_parse.c around lines 36 to 65, flb_strdup calls for
ec->cloud_user and ec->cloud_passwd are not checked for NULL; update the loop to
check each flb_strdup return, and on any NULL perform cleanup of any previously
allocated credential (free ec->cloud_user or ec->cloud_passwd if set and reset
its own_* flag), free toks via flb_utils_split_free(toks), and return -1 to
indicate OOM; ensure that on success both own_cloud_* flags remain true and the
function still returns 0.
| struct flb_elasticsearch_config *flb_es_upstream_conf(struct flb_elasticsearch *ctx, | ||
| struct flb_upstream_node *node) | ||
| { | ||
| if (!ctx) { | ||
| return NULL; | ||
| } | ||
| if (node) { | ||
| /* Get elasticsearch_config stored in node opaque data */ | ||
| return flb_upstream_node_get_data(node); | ||
| } | ||
| if (mk_list_is_empty(&ctx->configs) == 0) { | ||
| return NULL; | ||
| } | ||
| return mk_list_entry_last(&ctx->configs, struct flb_elasticsearch_config, _head); | ||
| } |
There was a problem hiding this comment.
Bug: wrong empty-list check causes NULL return in single-upstream mode.
mk_list_is_empty returns non-zero when empty. The current condition returns NULL when the list is non-empty, breaking single-upstream selection.
- if (mk_list_is_empty(&ctx->configs) == 0) {
- return NULL;
- }
- return mk_list_entry_last(&ctx->configs, struct flb_elasticsearch_config, _head);
+ if (mk_list_is_empty(&ctx->configs) != 0) {
+ return NULL;
+ }
+ return mk_list_entry_last(&ctx->configs,
+ struct flb_elasticsearch_config, _head);📝 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.
| struct flb_elasticsearch_config *flb_es_upstream_conf(struct flb_elasticsearch *ctx, | |
| struct flb_upstream_node *node) | |
| { | |
| if (!ctx) { | |
| return NULL; | |
| } | |
| if (node) { | |
| /* Get elasticsearch_config stored in node opaque data */ | |
| return flb_upstream_node_get_data(node); | |
| } | |
| if (mk_list_is_empty(&ctx->configs) == 0) { | |
| return NULL; | |
| } | |
| return mk_list_entry_last(&ctx->configs, struct flb_elasticsearch_config, _head); | |
| } | |
| struct flb_elasticsearch_config *flb_es_upstream_conf(struct flb_elasticsearch *ctx, | |
| struct flb_upstream_node *node) | |
| { | |
| if (!ctx) { | |
| return NULL; | |
| } | |
| if (node) { | |
| /* Get elasticsearch_config stored in node opaque data */ | |
| return flb_upstream_node_get_data(node); | |
| } | |
| - if (mk_list_is_empty(&ctx->configs) == 0) { | |
| - return NULL; | |
| - } | |
| if (mk_list_is_empty(&ctx->configs) != 0) { | |
| return NULL; | |
| } | |
| return mk_list_entry_last(&ctx->configs, | |
| struct flb_elasticsearch_config, _head); | |
| } |
🤖 Prompt for AI Agents
In plugins/out_es/es_conf.c around lines 932 to 946, the code uses "if
(mk_list_is_empty(&ctx->configs) == 0) return NULL;" which is inverted because
mk_list_is_empty returns non-zero when the list is empty; change the check to
return NULL when the list is empty by using "if
(mk_list_is_empty(&ctx->configs)) return NULL;" (i.e., remove the == 0
inversion) so single-upstream selection returns the last entry when the list is
non-empty.
| #include <monkey/mk_core/mk_list.h> | ||
| #include <fluent-bit/flb_sds.h> | ||
| #include <fluent-bit/flb_upstream_node.h> | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce public header coupling; prefer forward declarations.
This header only needs pointer types for upstreams, nodes, TLS, record accessor, and output instance. Including flb_upstream_node.h here forces transitive rebuilds and increases compile-time. Keep mk_list.h and flb_sds.h, but forward-declare the rest and move heavy includes to .c.
#include <monkey/mk_core/mk_list.h>
#include <fluent-bit/flb_sds.h>
-#include <fluent-bit/flb_upstream_node.h>
+
+/* Forward declarations to minimize header coupling */
+struct flb_upstream;
+struct flb_upstream_ha;
+struct flb_upstream_node;
+struct flb_output_instance;
+struct flb_record_accessor;
+#ifdef FLB_HAVE_AWS
+struct flb_aws_provider;
+struct flb_tls;
+#endif🤖 Prompt for AI Agents
In plugins/out_es/es.h around lines 23 to 26, avoid including
flb_upstream_node.h to reduce public header coupling: keep the existing includes
for mk_list.h and flb_sds.h, remove the flb_upstream_node.h include, and replace
it with forward declarations for the pointer-only types used in this header
(e.g., struct flb_upstream; struct flb_upstream_node; struct flb_tls; struct
flb_record_accessor; struct flb_output_instance;), then move the heavy
include(s) (flb_upstream_node.h and any other Fluent Bit headers needed for full
definitions) into the corresponding .c file where the concrete types are
used/defined.
There was a problem hiding this comment.
As this feature grows, having these definitions spread out could become challenging to manage. Perhaps we could unify them to make it more robust?
So, I thought that generalizing creations and destruction could be unified with C macro. Like:
#define OUT_ES_OWNED_MEMBER(type, name) \
type name; \
int own_##name;and
#define OUT_ES_DESTROY_OWNED_MEMBER(config_ptr, member_name, destroy_func) \
do { \
if ((config_ptr)->own_##member_name && (config_ptr)->member_name) { \
destroy_func((config_ptr)->member_name); \
} \
} while (0)These macros encapsulate the details of implementations that we frequently pointed out for fragility and maintainability.
If we use this type of macros, we could use like:
struct flb_elasticsearch_config {
/* Elasticsearch index (database) and type (table) */
OUT_ES_DECLARE_OWNED_MEMBER(char *, index);
OUT_ES_DECLARE_OWNED_MEMBER(char *, type);
int suppress_type_name;
/* Elastic Cloud Auth */
OUT_ES_DECLARE_OWNED_MEMBER(char *, cloud_user);
OUT_ES_DECLARE_OWNED_MEMBER(char *, cloud_passwd);
#ifdef FLB_HAVE_AWS
/* ... other AWS members ... */
OUT_ES_DECLARE_OWNED_MEMBER(struct flb_aws_provider *, aws_provider);
OUT_ES_DECLARE_OWNED_MEMBER(struct flb_aws_provider *, base_aws_provider);
#endif
/* prefix */
OUT_ES_DECLARE_OWNED_MEMBER(flb_sds_t, logstash_prefix);
OUT_ES_DECLARE_OWNED_MEMBER(flb_sds_t, logstash_prefix_separator);
/* time key */
OUT_ES_DECLARE_OWNED_MEMBER(flb_sds_t, time_key);
/* ... and so on for all other owned members ... */
};And destruction phase is written as:
void flb_elasticsearch_config_destroy(struct flb_elasticsearch_config *config)
{
if (!config) {
return;
}
/* Free all owned members using the helper macro */
OUT_ES_DESTROY_OWNED_MEMBER(config, index, flb_free);
OUT_ES_DESTROY_OWNED_MEMBER(config, type, flb_free);
OUT_ES_DESTROY_OWNED_MEMBER(config, cloud_user, flb_free);
OUT_ES_DESTROY_OWNED_MEMBER(config, cloud_passwd, flb_free);
#ifdef FLB_HAVE_AWS
OUT_ES_DESTROY_OWNED_MEMBER(config, aws_provider, flb_aws_provider_destroy);
OUT_ES_DESTROY_OWNED_MEMBER(config, base_aws_provider, flb_aws_provider_destroy);
#endif
OUT_ES_DESTROY_OWNED_MEMBER(config, logstash_prefix, flb_sds_destroy);
OUT_ES_DESTROY_OWNED_MEMBER(config, logstash_prefix_separator, flb_sds_destroy);
OUT_ES_DESTROY_OWNED_MEMBER(config, time_key, flb_sds_destroy);
/* ... destruction for all other owned members ... */
flb_free(config);
}This approach could be generalized type independent and more readable.
Implementation of Upstream feature for the Elasticsearch output plugin.
This pull request is based on pull request #1560 and Forward output plugin.
It was tested in a local setup with:
Fluent Bit without Upstream feature connected to a single node of Elasticsearch cluster consisting of 3 master-eligible/data and 1 coordinating nodes.
Refer to elastic-cluster directory of mabrarov/elastic-stack repository for Docker Compose project used to create target Elasticsearch cluster and Kibana.
fluent-bit.conf Fluent Bit configuration file used for the test - refer to fluent-bit-es/fluent-bit.conf and (same in YAML format) fluent-bit-es/fluent-bit.yaml in mabrarov/elastic-stack repository.
Debug log is available at flb_es.log.
Fluent Bit with Upstream feature connected to all Elasticsearch data nodes of Elasticsearch cluster consisting of 3 master-eligible/data and 1 coordinating nodes.
Refer to elastic-cluster directory of mabrarov/elastic-stack repository for Docker Compose project used to create target Elasticsearch cluster and Kibana.
fluent-bit.conf Fluent Bit configuration file used for the test - refer to fluent-bit-es-cluster/fluent-bit.conf and (same in YAML format) fluent-bit-es-cluster/fluent-bit.yaml in mabrarov/elastic-stack repository.
Debug log is available at flb_es_upstream.log.
Testing
TEST_PRESET=valgrind SKIP_TESTS='flb-rt-out_td flb-it-network' ./run_code_analysis.shok-package-testlabel to test for all targets (requires maintainer to do).Documentation
Backporting
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.
Summary by CodeRabbit
New Features
Bug Fixes
Build
Tests