es_out: support Upstream Servers with configuration overriding#7608
es_out: support Upstream Servers with configuration overriding#7608mabrarov wants to merge 12 commits intofluent:masterfrom
Conversation
mabrarov
requested review from
PettitWesley,
edsiper,
fujimotos,
koleini,
leonardo-albertovich and
niedbalski
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
🤖 Fix all issues with AI agents
In `@plugins/out_es/es.c`:
- Around line 1074-1078: The warning is misleading because
flb_elasticsearch_target(ctx, &node) failed before any HTTP action; update the
flb_plg_warn call in the failure branch of flb_elasticsearch_target to log a
clear target-resolution error (e.g., "failed to resolve Elasticsearch target" or
similar) and include relevant context such as the node variable or ec/ret values
for diagnostics; remove or replace the "http_do=%i" message so it no longer
implies an HTTP operation occurred.
♻️ Duplicate comments (1)
tests/runtime/out_elasticsearch.c (1)
31-44: Usetmpnamwith caller-provided buffer as previously agreed.The current code uses
tmpnam(NULL)which returns a pointer to an internal static buffer, then copies it withflb_strdup. While this addresses the buffer reuse concern, it's cleaner to provide your own buffer as discussed in previous review comments:- 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; - } + char tmpname_buf[L_tmpnam]; + if (tmpnam(tmpname_buf) == NULL) { + return NULL; + } + upstream_conf_filename = flb_strdup(tmpname_buf); + if (upstream_conf_filename == NULL) { + return NULL; + }This avoids relying on the static internal buffer.
🧹 Nitpick comments (3)
plugins/out_es/es_conf_prop.h (1)
1-65: LGTM! Clean centralization of ES configuration property names.Defining property names as macros reduces the risk of typos and makes refactoring easier. The AWS-specific properties are correctly guarded by
FLB_HAVE_AWS.Minor nit: The copyright year (2024) differs from other files in this PR (2026). Consider updating for consistency.
plugins/out_es/es_conf.c (2)
100-107: Potential type confusion withsize_tcomparison to-1.
buffer_sizeis of typesize_t(unsigned), so comparing it to-1compares againstSIZE_MAX. While this works because the config map likely sets the sentinel value as-1which wraps toSIZE_MAX, it's somewhat confusing. Consider using an explicit constant or adding a comment clarifying this is intentional./* buffer_size == (size_t)-1 indicates unlimited, convert to 0 for HTTP client */ if (buffer_size == (size_t)-1) { return 0; }
369-388: Boolean parsing does not validate return value.
flb_utils_bool()returns-1for invalid boolean strings (e.g., "maybe"), but this value is directly assigned to config fields without validation. While this follows existing Fluent Bit patterns, invalid boolean values in upstream node configs will silently become-1rather than generating a configuration error.If strict validation is desired, consider checking the return value:
tmp = flb_upstream_node_get_property(FLB_ES_CONFIG_PROPERTY_SUPPRESS_TYPE_NAME, node); if (tmp) { int bool_val = flb_utils_bool(tmp); if (bool_val == -1) { flb_plg_error(ctx->ins, "invalid boolean value for suppress_type_name: %s", tmp); return -1; } ec->suppress_type_name = bool_val; }This applies to multiple properties in this function.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
include/fluent-bit/flb_lib.hinclude/fluent-bit/flb_output.hplugins/out_es/CMakeLists.txtplugins/out_es/es.cplugins/out_es/es.hplugins/out_es/es_conf.cplugins/out_es/es_conf.hplugins/out_es/es_conf_parse.cplugins/out_es/es_conf_parse.hplugins/out_es/es_conf_prop.hplugins/out_es/es_type.cplugins/out_es/es_type.hsrc/flb_engine_dispatch.csrc/flb_lib.ctests/runtime/out_elasticsearch.c
🚧 Files skipped from review as they are similar to previous changes (4)
- plugins/out_es/es_conf_parse.h
- src/flb_engine_dispatch.c
- include/fluent-bit/flb_output.h
- plugins/out_es/CMakeLists.txt
🧰 Additional context used
🧠 Learnings (13)
📚 Learning: 2025-11-21T06:23:29.770Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 11171
File: include/fluent-bit/flb_lib.h:52-53
Timestamp: 2025-11-21T06:23:29.770Z
Learning: In Fluent Bit core (fluent/fluent-bit repository), function descriptions/documentation are not required for newly added functions in header files.
Applied to files:
include/fluent-bit/flb_lib.h
📚 Learning: 2025-08-29T06:25:27.250Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:93-107
Timestamp: 2025-08-29T06:25:27.250Z
Learning: In Fluent Bit, ZSTD compression is enabled by default and is treated as a core dependency, not requiring conditional compilation guards like `#ifdef FLB_HAVE_ZSTD`. Unlike some other optional components such as ARROW/PARQUET (which use `#ifdef FLB_HAVE_ARROW` guards), ZSTD support is always available and doesn't need build-time conditionals. ZSTD headers are included directly without guards across multiple plugins and core components.
Applied to files:
include/fluent-bit/flb_lib.h
📚 Learning: 2025-08-29T06:24:26.170Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:39-42
Timestamp: 2025-08-29T06:24:26.170Z
Learning: In Fluent Bit, ZSTD compression support is enabled by default and does not require conditional compilation guards (like `#ifdef` FLB_HAVE_ZSTD) around ZSTD-related code declarations and implementations.
Applied to files:
include/fluent-bit/flb_lib.h
📚 Learning: 2025-08-29T06:24:55.855Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: src/aws/flb_aws_compress.c:52-56
Timestamp: 2025-08-29T06:24:55.855Z
Learning: ZSTD compression is always available in Fluent Bit and does not require conditional compilation guards. Unlike Arrow/Parquet which use `#ifdef` FLB_HAVE_ARROW guards, ZSTD is built unconditionally with flb_zstd.c included directly in src/CMakeLists.txt and a bundled ZSTD library at lib/zstd-1.5.7/.
Applied to files:
include/fluent-bit/flb_lib.h
📚 Learning: 2025-08-29T06:25:27.250Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:93-107
Timestamp: 2025-08-29T06:25:27.250Z
Learning: In Fluent Bit, ZSTD compression is enabled by default and is treated as a core dependency, not requiring conditional compilation guards like `#ifdef FLB_HAVE_ZSTD`. Unlike some other optional components, ZSTD support is always available and doesn't need build-time conditionals.
Applied to files:
include/fluent-bit/flb_lib.h
📚 Learning: 2025-08-29T06:25:02.561Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 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:
include/fluent-bit/flb_lib.h
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 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:
include/fluent-bit/flb_lib.hplugins/out_es/es.h
📚 Learning: 2025-08-20T02:04:27.220Z
Learnt from: mabrarov
Repo: fluent/fluent-bit PR: 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.cplugins/out_es/es_conf_parse.c
📚 Learning: 2025-09-22T15:59:55.794Z
Learnt from: nicknezis
Repo: fluent/fluent-bit PR: 10882
File: plugins/out_http/http.c:112-116
Timestamp: 2025-09-22T15:59:55.794Z
Learning: When users consider bug fixes out of scope for their focused PRs, it's appropriate to create separate GitHub issues to track those concerns rather than expanding the current PR scope.
Applied to files:
plugins/out_es/es.c
📚 Learning: 2025-10-06T17:39:21.606Z
Learnt from: edsiper
Repo: fluent/fluent-bit PR: 10967
File: plugins/in_elasticsearch/in_elasticsearch_bulk_prot.c:266-272
Timestamp: 2025-10-06T17:39:21.606Z
Learning: In the in_elasticsearch plugin, payloads are JSON-based, so tag_key values will always be strings. There's no need to handle binary (FLB_RA_BINARY) values when extracting tags from records in this plugin.
Applied to files:
plugins/out_es/es.c
📚 Learning: 2025-11-11T20:33:42.883Z
Learnt from: mabrarov
Repo: fluent/fluent-bit PR: 7608
File: plugins/out_es/es_conf.c:932-946
Timestamp: 2025-11-11T20:33:42.883Z
Learning: The mk_list_is_empty function in Fluent Bit returns 0 when a list IS empty and -1 when a list is NOT empty. Use `mk_list_is_empty(list) == 0` to check if empty, and `mk_list_is_empty(list) != 0` to check if not empty.
Applied to files:
plugins/out_es/es_conf.c
📚 Learning: 2025-09-08T11:21:33.975Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 10851
File: include/fluent-bit/flb_simd.h:60-66
Timestamp: 2025-09-08T11:21:33.975Z
Learning: Fluent Bit currently only supports MSVC compiler on Windows, so additional compiler compatibility guards may be unnecessary for Windows-specific code paths.
Applied to files:
plugins/out_es/es_conf_parse.c
📚 Learning: 2025-12-17T18:45:02.283Z
Learnt from: edsiper
Repo: fluent/fluent-bit PR: 11286
File: src/flb_http_client.c:1657-1715
Timestamp: 2025-12-17T18:45:02.283Z
Learning: In the Fluent Bit OAuth2 implementation (src/flb_oauth2.c), the function flb_oauth2_get_access_token returns a pointer to the cached token stored in the OAuth2 context (ctx->access_token), not a new allocation. The OAuth2 context manages the token lifecycle, so callers must not free the returned token pointer.
Applied to files:
plugins/out_es/es_conf_parse.c
🧬 Code graph analysis (8)
include/fluent-bit/flb_lib.h (2)
include/fluent-bit/flb_output.h (1)
int(583-586)src/flb_lib.c (1)
flb_output_set_test_flush_ctx_callback(617-649)
plugins/out_es/es.c (2)
plugins/out_es/es_conf.c (1)
flb_es_upstream_conf(887-901)src/flb_upstream_ha.c (1)
flb_upstream_ha_node_get(88-110)
tests/runtime/out_elasticsearch.c (2)
plugins/out_es/es.c (1)
flb_elasticsearch_target(652-674)src/flb_lib.c (6)
flb_output_set_test_flush_ctx_callback(617-649)flb_start(1017-1028)flb_input(266-276)flb_output(279-289)flb_output_set(520-551)flb_output_set_test(584-615)
plugins/out_es/es_conf.c (9)
plugins/out_es/es_type.c (10)
flb_es_ra_move_ra(182-190)flb_es_str_copy_str(48-63)flb_es_str_set_str(41-46)flb_es_aws_provider_set(204-215)flb_es_ra_set_ra(172-180)flb_es_sds_copy_str(85-100)flb_es_sds_destroy(65-74)flb_es_ra_destroy(161-170)flb_es_list_destroy(102-112)flb_es_str_destroy(30-39)src/flb_record_accessor.c (1)
flb_ra_create(271-358)include/fluent-bit/flb_mem.h (2)
flb_free(126-128)flb_calloc(84-96)plugins/out_es/es_conf_parse.c (3)
flb_es_conf_set_cloud_auth(142-192)flb_es_conf_set_cloud_credentials(38-73)flb_es_conf_set_aws_provider(279-345)include/fluent-bit/flb_output.h (1)
flb_output_config_map_set(1321-1348)src/flb_upstream_node.c (1)
flb_upstream_node_get_property(183-199)src/flb_utils.c (1)
flb_utils_bool(757-771)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 (1)
flb_upstream_create(290-363)
plugins/out_es/es_conf.h (1)
plugins/out_es/es_conf.c (3)
flb_es_conf_create(819-852)flb_es_conf_destroy(854-885)flb_es_upstream_conf(887-901)
plugins/out_es/es_conf_parse.c (6)
src/flb_utils.c (2)
flb_utils_split(464-467)flb_utils_split_free(477-489)plugins/out_es/es_type.c (2)
flb_es_str_copy_str(48-63)flb_es_list_move_list(123-130)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_credentials.c (1)
flb_standard_chain_provider_create(269-327)
plugins/out_es/es.h (1)
plugins/out_es/es.c (1)
flb_elasticsearch_target(652-674)
plugins/out_es/es_type.h (1)
plugins/out_es/es_type.c (19)
flb_es_str_destroy(30-39)flb_es_str_set_str(41-46)flb_es_str_copy_str(48-63)flb_es_sds_destroy(65-74)flb_es_sds_set_sds(76-83)flb_es_sds_copy_str(85-100)flb_es_list_destroy(102-112)flb_es_list_set_list(114-121)flb_es_list_move_list(123-130)flb_es_tls_destroy(132-141)flb_es_tls_set_tls(143-150)flb_es_tls_move_tls(152-159)flb_es_ra_destroy(161-170)flb_es_ra_set_ra(172-180)flb_es_ra_move_ra(182-190)flb_es_aws_provider_destroy(193-202)flb_es_aws_provider_set(204-215)flb_es_aws_provider_move(217-228)flb_es_aws_provider_move_provider(230-238)
⏰ 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). (81)
- GitHub Check: PR - container builds / Windows container images (2025)
- GitHub Check: PR - container builds / Windows container images (2022)
- 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 Linux / debian/bookworm.arm64v8 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 / almalinux/10 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 / rockylinux/10 package build and stage to S3
- GitHub Check: PR - packages build Linux / debian/trixie 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 / debian/trixie.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 / debian/bullseye 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 / ubuntu/22.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 / almalinux/10.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 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 / raspbian/bookworm 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/10.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 / rockylinux/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 / almalinux/9 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/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 / ubuntu/22.04 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 / almalinux/9.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 / centos/7 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/10.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/10 package build and stage to S3
- GitHub Check: PR - packages build Linux / centos/7.arm64v8 package build and stage to S3
- GitHub Check: PR - container builds / arm64/production 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 / 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 - 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 - packages build Windows / 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 32bit, x86, x86-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_SANITIZE_THREAD=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_COMPILER_STRICT_POINTER_TYPES=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 (-DSANITIZE_UNDEFINED=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_SIMD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=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, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=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_SMALL=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, gcc, g++)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-22.04, clang-12)
- GitHub Check: PR - fuzzing test
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-24.04, clang-14)
- GitHub Check: pr-compile-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-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-24.04, clang-14)
- GitHub Check: pr-compile-without-cxx (3.31.6)
🔇 Additional comments (32)
src/flb_lib.c (2)
584-615: LGTM! Clean separation of test setup functions.The parameter rename from
test_ctxtoflush_ctxbetter reflects the purpose, and the implementation correctly stores the context without taking ownership.
617-649: LGTM! New flush context callback API follows existing patterns.The new
flb_output_set_test_flush_ctx_callbackfunction properly mirrors the structure offlb_output_set_testand addresses the prior review feedback about separating concerns. This enables tests to dynamically compute flush context via callback rather than using a static value.plugins/out_es/es_conf.h (1)
23-40: LGTM! Well-structured header with appropriate forward declarations.The header cleanly exposes the new upstream configuration API and write operation macros. The
voidreturn type forflb_es_conf_destroyaligns with typical destroy function patterns in the codebase.include/fluent-bit/flb_lib.h (2)
26-27: LGTM! Forward declaration addresses previous review feedback.Using a forward declaration instead of including
flb_input.hreduces coupling and keeps the public API surface clean.
70-81: LGTM! Public API declarations are consistent with implementation.The updated
flb_output_set_testsignature withflush_ctxand the newflb_output_set_test_flush_ctx_callbackdeclaration properly expose the testing infrastructure for upstream-aware ES tests.plugins/out_es/es_conf_parse.c (5)
38-73: LGTM! Cloud credentials parsing with proper OOM handling.The function correctly handles memory allocation failures and uses
flb_es_str_copy_strfor ownership-aware string copying.
142-192: LGTM! Cloud auth host extraction and assignment.The function properly handles the optional port parsing and correctly manages the
host.nameSDS lifetime by destroying the old value before assignment.
196-215: LGTM! AWS unsigned headers initialization.The function correctly allocates and initializes the unsigned headers list. The ownership transfer via
flb_es_list_move_listensures proper cleanup on error paths.
217-277: LGTM! STS provider setup with proper resource management.The function correctly:
- Generates and frees the session name after provider creation
- Creates a separate TLS context for STS
- Moves ownership of the base provider to avoid double-free
- Reports errors at appropriate points
279-345: LGTM! AWS provider configuration with proper initialization sequence.The function follows the established Fluent Bit pattern for AWS credential providers:
- Creates TLS context
- Creates standard chain provider
- Optionally wraps with STS provider for role assumption
- Initializes in sync mode then switches to async
- Sets upstream association
The ownership helpers (
flb_es_tls_move_tls,flb_es_aws_provider_move_provider) ensure consistent resource management.plugins/out_es/es_type.c (1)
1-239: Well-structured ownership wrapper implementation.The ownership-aware wrapper types provide a clean abstraction for managing shared vs owned resources. The consistent pattern across all types (destroy, set, copy/move) is good.
A few observations for consideration:
Inconsistent behavior between
flb_es_str_set_strandflb_es_sds_set_sds: The str version unconditionally setsdest->valueanddest->ownedafter destroy, while the sds version only assigns whensrcis non-NULL. This could lead to staleownedflags in the sds case whensrcis NULL.
flb_es_aws_provider_move(lines 217-228): After*dest = *src, the code setssrc->value = NULLbut doesn't clearsrc->owned. This leaves a staleownedflag in the source, which could cause confusion if the source is later inspected (though it's likely not a practical issue since value is NULL).These are minor consistency nits and the current implementation should work correctly in practice.
plugins/out_es/es_type.h (1)
1-402: Well-documented ownership wrapper API.The header provides comprehensive documentation for the ownership semantics, clearly explaining when direct member access is allowed and how the wrapper functions should be used. The consistent structure across all wrapper types (value + owned flag) makes the API predictable and easy to use.
The use of forward declarations keeps header coupling minimal while still providing all necessary type information.
plugins/out_es/es.h (1)
23-178: Clean restructuring with proper HA support.The separation between
flb_elasticsearch(main context) andflb_elasticsearch_config(per-node configuration) is well-designed. The use of wrapper types for fields that can be shared or owned between the main config and per-node configs addresses the double-free concerns mentioned in PR discussions.The forward declarations appropriately reduce header coupling as previously discussed.
plugins/out_es/es_conf.c (2)
606-634: Thorough resource cleanup implementation.The
elasticsearch_config_destroyfunction properly destroys all wrapper types in a consistent order. The cleanup handles both owned and non-owned resources correctly through the wrapper APIs.
636-745: Well-structured HA configuration with proper error handling.The
es_config_hafunction properly:
- Creates the main config context
- Sets up base properties
- Creates per-node configs with proper inheritance
- Validates each node's config
- Has comprehensive error cleanup that nullifies node data and destroys allocated configs on failure
The rollback logic (lines 722-738) correctly handles partial initialization failures.
tests/runtime/out_elasticsearch.c (3)
113-122: Good flush context extraction callback.The
cb_flush_contextcallback properly usesflb_elasticsearch_targetto extract the per-node configuration, enabling tests to verify that upstream node configurations are correctly applied during formatting.
1076-1137: Comprehensive upstream test implementation.The
flb_test_upstream_write_operationtest properly verifies that per-nodewrite_operationoverrides the plugin-level setting. The test:
- Sets plugin-level
Write_Operationto "Upsert"- Sets node-level
write_operationto "index" via upstream config- Verifies the output uses "index" operation
This pattern is consistently applied across all new upstream tests, providing good coverage for the configuration override mechanism.
1598-1624: Good test coverage expansion.The TEST_LIST properly includes all new upstream tests alongside existing ones, ensuring the new HA/upstream functionality is exercised in CI.
plugins/out_es/es.c (14)
33-48: LGTM!The new includes for
flb_upstream_node.handes_conf_prop.hare appropriate for the upstream server support feature. The forward declaration update correctly reflects the signature change.
50-81: LGTM!The separation of
ctx(for plugin instance/logging) andec(for configuration values) is clean and enables per-upstream-node AWS authentication configuration.
83-205: LGTM!The refactoring from
ctxtoecis correctly applied throughout the map/array content packing functions. The recursive calls properly propagate theecparameter.
212-240: LGTM!Configuration values are correctly read from
ecwhilectxis retained for logging purposes.
242-279: LGTM!The function correctly uses
ecfor configuration values. Since no logging is performed,ctxis not needed here.
325-325: LGTM!Using
flush_ctxto obtain the per-requestecconfiguration is the correct approach for upstream server support, allowing each flush to use the configuration specific to the target node.
652-674: LGTM!The target routing logic correctly handles both HA and non-HA modes. The function returns NULL when no valid configuration is available, and callers properly check for this condition. Based on learnings, the function contract approach (assuming valid parameters) is acceptable for this internal function.
835-876: LGTM!The refactoring correctly:
- Obtains the target configuration via
flb_elasticsearch_target- Handles upstream connection acquisition for both HA and non-HA modes
- Returns
FLB_RETRYwhen no target is available or no connection can be obtained
916-922: LGTM!The NULL check for
flb_http_clientwas correctly added as previously requested.
1040-1058: LGTM!The cleanup consolidation using
cleanup_and_returnandretrylabels is well-structured:
retis set before entering cleanup- Resources are conditionally cleaned up (HTTP client null-checked)
- The retry path correctly sets
ret = FLB_RETRYand jumps to the common cleanup
1086-1089: LGTM!The NULL check for
flb_http_dummy_clientwas correctly added as previously requested.
1148-1165: LGTM!The config map correctly uses the new
FLB_ES_CONFIG_PROPERTY_*macros and updates offsets to reference fields withinstruct flb_elasticsearch_config. The nested offset pattern for wrapper types (likeflb_es_str) is appropriate for tracking ownership.
1385-1389: LGTM!The new UPSTREAM property is correctly added with:
FLB_FALSE(not auto-applied, requires special handling)- Offset
0(handled specially in configuration parsing)
287-627: Clean refactoring of elasticsearch_format for per-node configuration.The function correctly uses
ecfromflush_ctxfor all configuration values while retainingctxfor plugin instance operations. Error handling and resource cleanup remain robust.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
Hi @cosmo0920, It's regarding #7608 (review):
Done in eaba388 with following differences:
Thank you. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@plugins/out_es/es_conf.c`:
- Around line 595-604: config_validate currently only rejects configs missing
index when index is NULL and both logstash_format and generate_id are false;
update it to also consider current_time_index so that validation requires at
least one of index (ec->index.value non-NULL), ec->logstash_format == FLB_TRUE,
or ec->current_time_index == FLB_TRUE to be true; change the if condition in
config_validate to include ec->current_time_index check and keep the same error
log (flb_plg_error on ctx->ins) and return -1 when none are set.
♻️ Duplicate comments (1)
plugins/out_es/es.c (1)
1074-1078: Fix misleading error message when target resolution fails.The warning message
"elasticsearch_target=%i"withret=0is misleading. At this point,retis still 0 (initialized at line 1066), and no operation that would setrethas occurred - the failure is inflb_elasticsearch_target. This appears similar to a previously flagged issue that was marked as addressed.Suggested fix
ec = flb_elasticsearch_target(ctx, &node); if (!ec) { - flb_plg_warn(ctx->ins, "elasticsearch_target=%i", ret); + flb_plg_warn(ctx->ins, "failed to get target configuration"); return -2; }
🧹 Nitpick comments (1)
plugins/out_es/es.c (1)
547-547: Minor: Extra space before comma.There's an extra space before the comma in the function call.
Suggested fix
- id_key_str = es_get_id_value(ctx, ec ,&map); + id_key_str = es_get_id_value(ctx, ec, &map);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
plugins/out_es/CMakeLists.txtplugins/out_es/es.cplugins/out_es/es.hplugins/out_es/es_conf.cplugins/out_es/es_conf_parse.cplugins/out_es/es_type.cplugins/out_es/es_type.h
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/out_es/es_conf_parse.c
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-08-20T02:04:27.220Z
Learnt from: mabrarov
Repo: fluent/fluent-bit PR: 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
📚 Learning: 2025-09-22T15:59:55.794Z
Learnt from: nicknezis
Repo: fluent/fluent-bit PR: 10882
File: plugins/out_http/http.c:112-116
Timestamp: 2025-09-22T15:59:55.794Z
Learning: When users consider bug fixes out of scope for their focused PRs, it's appropriate to create separate GitHub issues to track those concerns rather than expanding the current PR scope.
Applied to files:
plugins/out_es/es.c
📚 Learning: 2025-10-06T17:39:21.606Z
Learnt from: edsiper
Repo: fluent/fluent-bit PR: 10967
File: plugins/in_elasticsearch/in_elasticsearch_bulk_prot.c:266-272
Timestamp: 2025-10-06T17:39:21.606Z
Learning: In the in_elasticsearch plugin, payloads are JSON-based, so tag_key values will always be strings. There's no need to handle binary (FLB_RA_BINARY) values when extracting tags from records in this plugin.
Applied to files:
plugins/out_es/es.c
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 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:
plugins/out_es/es.h
📚 Learning: 2025-11-11T20:33:42.883Z
Learnt from: mabrarov
Repo: fluent/fluent-bit PR: 7608
File: plugins/out_es/es_conf.c:932-946
Timestamp: 2025-11-11T20:33:42.883Z
Learning: The mk_list_is_empty function in Fluent Bit returns 0 when a list IS empty and -1 when a list is NOT empty. Use `mk_list_is_empty(list) == 0` to check if empty, and `mk_list_is_empty(list) != 0` to check if not empty.
Applied to files:
plugins/out_es/es_conf.c
🧬 Code graph analysis (4)
plugins/out_es/es.c (6)
src/flb_record_accessor.c (1)
flb_ra_get_value_object(803-814)include/fluent-bit/flb_time.h (1)
flb_time_copy(81-85)plugins/out_es/es_conf.c (1)
flb_es_upstream_conf(887-901)src/flb_upstream_ha.c (1)
flb_upstream_ha_node_get(88-110)src/flb_upstream.c (1)
flb_upstream_conn_get(711-844)src/flb_http_client.c (2)
flb_http_client(866-911)flb_http_basic_auth(1394-1398)
plugins/out_es/es.h (1)
plugins/out_es/es.c (1)
flb_elasticsearch_target(652-674)
plugins/out_es/es_type.h (1)
plugins/out_es/es_type.c (19)
flb_es_str_destroy(30-37)flb_es_str_set_str(39-44)flb_es_str_copy_str(46-61)flb_es_sds_destroy(63-70)flb_es_sds_set_sds(72-79)flb_es_sds_copy_str(81-96)flb_es_list_destroy(98-106)flb_es_list_set_list(108-115)flb_es_list_move_list(117-124)flb_es_tls_destroy(126-133)flb_es_tls_set_tls(135-142)flb_es_tls_move_tls(144-151)flb_es_ra_destroy(153-160)flb_es_ra_set_ra(162-170)flb_es_ra_move_ra(172-180)flb_es_aws_provider_destroy(183-190)flb_es_aws_provider_set(192-203)flb_es_aws_provider_move(205-216)flb_es_aws_provider_move_provider(218-226)
plugins/out_es/es_type.c (6)
include/fluent-bit/flb_mem.h (1)
flb_free(126-128)src/flb_sds.c (2)
flb_sds_destroy(389-399)flb_sds_create(78-90)src/flb_slist.c (1)
flb_slist_destroy(327-339)src/tls/flb_tls.c (1)
flb_tls_destroy(258-277)src/flb_record_accessor.c (1)
flb_ra_destroy(232-248)src/aws/flb_aws_credentials.c (1)
flb_aws_provider_destroy(769-791)
⏰ 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). (31)
- 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 (-DSANITIZE_UNDEFINED=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_COMPILER_STRICT_POINTER_TYPES=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_ARROW=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=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 (-DSANITIZE_ADDRESS=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_COVERAGE=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_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_SMALL=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, 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: PR - fuzzing test
- 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-compile-without-cxx (3.31.6)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-22.04, clang-12)
- GitHub Check: pr-compile-centos-7
🔇 Additional comments (31)
plugins/out_es/CMakeLists.txt (1)
1-10: LGTM!The build integration correctly adds the new source files
es_type.candes_conf_parse.cto the plugin build.plugins/out_es/es_type.h (1)
1-402: Well-designed ownership-aware wrapper API.The wrapper types and their associated functions provide a clean abstraction for managing shared vs. owned resources between plugin-level config and per-upstream-node overrides. The documentation clearly explains ownership semantics and restrictions.
plugins/out_es/es_type.c (2)
30-227: Implementation is consistent and handles ownership correctly.The destroy/set/copy/move patterns are consistently implemented across all wrapper types. The self-assignment checks in AWS provider functions prevent double-free scenarios. The condition
ins->value && ins->owned != FLB_FALSEensures safe destruction even when move operations leave stale owned flags.
24-24: No action needed - unconditional include is safe.
flb_aws_credentials.his properly self-guarded with#ifdef FLB_HAVE_AWSat the header level. This allows unconditional includes throughout the codebase (as seen in core files likesrc/flb_signv4.c,src/aws/*, and other plugins). WhenFLB_HAVE_AWSis undefined, the include safely provides no declarations; when defined, it provides full AWS support. The AWS-using functions in this file are already guarded at the implementation level (line 182+), following the standard Fluent Bit pattern.Likely an incorrect or invalid review comment.
plugins/out_es/es.h (3)
23-28: Good reduction of header coupling.The header now uses forward declarations and includes only what's necessary (
mk_list.hand the locales_type.h), addressing the previous review feedback about reducing transitive rebuilds.
53-147: Per-node configuration structure is well-designed.The
flb_elasticsearch_configstructure cleanly separates fields that can be overridden per-upstream-node (using wrapper types with ownership tracking) from fixed fields (plain pointers to config-map-managed data). The_headlist entry enables tracking multiple configs in HA mode.
149-178: Main context structure and target selection API are appropriate.The
flb_elasticsearchcontext correctly separates HA-mode fields (ha_mode,ha,ha_upstream) from simple-mode fields (u), with a sharedconfigslist. Theflb_elasticsearch_targetfunction provides a clean abstraction for node selection.plugins/out_es/es_conf.c (14)
42-61: LGTM!Correctly creates a record accessor from
id_keywith proper ownership transfer and handles theGenerate_IDconflict appropriately.
63-98: LGTM!The write operation validation and
es_actionassignment to static strings are correct. The requirement forId_KeyorGenerate_Idwith update/upsert operations is properly enforced.
100-107: LGTM!The buffer size adjustment correctly maps the sentinel value to unlimited (0).
117-138: LGTM!URI construction with proper truncation detection using
snprintfreturn value.
140-177: LGTM!Correctly handles the logstash prefix key with optional
$prepending, proper memory management of the temporary buffer, and ownership transfer of the record accessor.
179-289: LGTM!Configuration property setup is comprehensive with proper error handling. The use of
flb_es_str_copy_strfor URI-derived values ensures owned copies are made.
291-357: Base configuration copy uses correct non-owning semantics.The
flb_es_*_set_*functions correctly establish non-owning references to the base configuration, avoiding double-free when both base and node configs are destroyed.
359-593: Node property overrides handle allocation failures correctly.Each property override that allocates memory returns
-1on failure, and the caller properly cleans up viaelasticsearch_config_destroy, which safely handles partially initialized configs due toflb_calloczero-initialization.
606-634: LGTM!Comprehensive cleanup of all owned resources in the config structure. Each destroy function handles NULL/zero values safely.
636-745: HA configuration setup is robust with comprehensive error handling.The function properly handles allocation failures at each step, with complete cleanup of partially created configs and upstream nodes on error. The separation of base config creation from per-node config creation enables proper error recovery.
747-817: LGTM!Simple (non-HA) mode configuration correctly sets defaults, creates the upstream, and handles all error paths with appropriate cleanup.
819-852: LGTM!The entry point correctly initializes the context, dispatches to the appropriate configuration mode, and handles errors appropriately.
854-885: LGTM!The destroy function correctly handles both HA and simple modes, safely iterates the configs list, and frees all resources.
887-901: LGTM!The config retrieval function correctly uses
mk_list_is_emptysemantics (returns 0 when empty) and properly handles both node-based lookup (HA mode) and fallback to the last config (simple mode). Based on learnings aboutmk_list_is_emptybehavior.plugins/out_es/es.c (10)
33-40: LGTM!The new includes for
flb_upstream_node.handes_conf_prop.hare appropriate for supporting the upstream servers feature and config property macros.
50-80: LGTM!The signature change to accept both
ctx(for logging viactx->ins) andec(for per-node AWS config) correctly supports the upstream servers feature while maintaining proper separation of concerns.
83-205: LGTM!The refactor to use
ecparameter in the recursive packing functions is consistent and correct. The mutual recursion betweenes_pack_map_contentandes_pack_array_contentcorrectly propagates the config context.
212-240: LGTM!The function correctly uses
ctxfor logging andecfor accessing the per-node configuration values (ra_id_key.value,id_key.value).
242-279: LGTM!The function signature change to use
ecis appropriate since it doesn't need logging access and only uses config values.
652-674: LGTM!The
flb_elasticsearch_targetfunction correctly implements HA-aware node selection. Based on learnings, the author prefers function contracts that assume valid parameters for internal functions, which is acceptable here since all call sites are controlled within this module.
855-876: LGTM!The HA-aware connection handling correctly differentiates between upstream mode (using
node->u) and non-HA mode (usingctx->u). The null checks for bothecandu_connare properly handled with appropriate error messages.
1040-1058: LGTM!The refactored cleanup logic using
cleanup_and_returnandretrylabels is well-structured. The conditional null check forcbefore destruction and the safeflb_free(pack)call (where pack is initialized to NULL) ensure proper resource cleanup.
1385-1389: LGTM!The new UPSTREAM configuration property is correctly defined. Using
FLB_FALSEwith offset0is appropriate since the upstream file path is processed separately ines_conf.crather than being directly stored in the config struct.
1149-1159: The nested offset pattern is valid and works correctly with flb_config_map.The
flb_config_map_setfunction (src/flb_config_map.c:638–666) uses the offset field viabase + m->offsetto write values to struct members. The nested offsetof patternoffsetof(struct flb_elasticsearch_config, index) + offsetof(struct flb_es_str, value)correctly computes the byte offset to the.valuefield within the wrapper struct. This is an unconventional but valid C technique that properly integrates with the config_map mechanism.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@plugins/out_es/es_conf_prop.h`:
- Line 5: Update the copyright year in the header comment of
plugins/out_es/es_conf_prop.h from 2015-2024 to 2015-2026 so it matches other
files in the PR; locate the top-of-file copyright comment (the line containing
"Copyright (C) 2015-2024 The Fluent Bit Authors") and change 2024 to 2026.
In `@plugins/out_es/es_conf.c`:
- Around line 595-604: The current config_validate allows index.value to be NULL
when logstash_format is false but current_time_index is true, which leads to a
NULL format passed to strftime in the code path that formats index names; update
config_validate (function config_validate and struct flb_elasticsearch_config
checks) so that if ec->logstash_format == FLB_FALSE then ec->index.value must be
non-NULL (i.e., change the failing condition to trigger when ec->index.value ==
NULL && ec->logstash_format == FLB_FALSE), preventing the
strftime(index_formatted, ..., ec->index.value, ...) call from receiving NULL.
♻️ Duplicate comments (1)
tests/runtime/out_elasticsearch.c (1)
22-111: Acceptable use of tmpnam() with flb_strdup for test code.The static buffer issue from past reviews was addressed by immediately duplicating the result with
flb_strdup(). "Never use these functions" - "Use mkstemp(3) or tmpfile(3) instead" per Linux man pages, but as discussed in prior review comments, this is test-only code and the author has notedmkstempis not part of C standard.Resource cleanup on all error paths (lines 58-61, 82-86, 96-98, 104-107) is properly handled with
remove()andflb_free().
🧹 Nitpick comments (1)
plugins/out_es/es_conf.c (1)
100-107: Subtle size_t comparison with-1.The comparison
buffer_size == -1works because-1is implicitly converted toSIZE_MAXwhen compared withsize_t. This is an established pattern in Fluent Bit for indicating "unlimited" buffer size. Consider adding a brief comment for clarity.Optional: Add clarifying comment
static size_t config_adjust_buffer_size(size_t buffer_size) { - /* HTTP Payload (response) maximum buffer size (0 == unlimited) */ + /* HTTP Payload (response) maximum buffer size (0 == unlimited) + * Note: -1 (SIZE_MAX when unsigned) indicates unlimited */ if (buffer_size == -1) { return 0; } return buffer_size; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
include/fluent-bit/flb_lib.hinclude/fluent-bit/flb_output.hplugins/out_es/CMakeLists.txtplugins/out_es/es.cplugins/out_es/es.hplugins/out_es/es_conf.cplugins/out_es/es_conf.hplugins/out_es/es_conf_parse.cplugins/out_es/es_conf_parse.hplugins/out_es/es_conf_prop.hplugins/out_es/es_type.cplugins/out_es/es_type.hsrc/flb_engine_dispatch.csrc/flb_lib.ctests/runtime/out_elasticsearch.c
🚧 Files skipped from review as they are similar to previous changes (4)
- plugins/out_es/es_conf_parse.h
- plugins/out_es/CMakeLists.txt
- plugins/out_es/es_conf.h
- include/fluent-bit/flb_lib.h
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-11-21T06:23:29.770Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 11171
File: include/fluent-bit/flb_lib.h:52-53
Timestamp: 2025-11-21T06:23:29.770Z
Learning: In Fluent Bit core (fluent/fluent-bit repository), function descriptions/documentation are not required for newly added functions in header files.
Applied to files:
include/fluent-bit/flb_output.h
📚 Learning: 2025-09-08T11:21:33.975Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 10851
File: include/fluent-bit/flb_simd.h:60-66
Timestamp: 2025-09-08T11:21:33.975Z
Learning: Fluent Bit currently only supports MSVC compiler on Windows, so additional compiler compatibility guards may be unnecessary for Windows-specific code paths.
Applied to files:
plugins/out_es/es_conf_parse.c
📚 Learning: 2025-08-20T02:04:27.220Z
Learnt from: mabrarov
Repo: fluent/fluent-bit PR: 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_conf_parse.cplugins/out_es/es.c
📚 Learning: 2025-12-17T18:45:02.283Z
Learnt from: edsiper
Repo: fluent/fluent-bit PR: 11286
File: src/flb_http_client.c:1657-1715
Timestamp: 2025-12-17T18:45:02.283Z
Learning: In the Fluent Bit OAuth2 implementation (src/flb_oauth2.c), the function flb_oauth2_get_access_token returns a pointer to the cached token stored in the OAuth2 context (ctx->access_token), not a new allocation. The OAuth2 context manages the token lifecycle, so callers must not free the returned token pointer.
Applied to files:
plugins/out_es/es_conf_parse.c
📚 Learning: 2025-09-22T15:59:55.794Z
Learnt from: nicknezis
Repo: fluent/fluent-bit PR: 10882
File: plugins/out_http/http.c:112-116
Timestamp: 2025-09-22T15:59:55.794Z
Learning: When users consider bug fixes out of scope for their focused PRs, it's appropriate to create separate GitHub issues to track those concerns rather than expanding the current PR scope.
Applied to files:
plugins/out_es/es_conf_parse.cplugins/out_es/es.c
📚 Learning: 2025-10-06T17:39:21.606Z
Learnt from: edsiper
Repo: fluent/fluent-bit PR: 10967
File: plugins/in_elasticsearch/in_elasticsearch_bulk_prot.c:266-272
Timestamp: 2025-10-06T17:39:21.606Z
Learning: In the in_elasticsearch plugin, payloads are JSON-based, so tag_key values will always be strings. There's no need to handle binary (FLB_RA_BINARY) values when extracting tags from records in this plugin.
Applied to files:
plugins/out_es/es.c
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 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:
plugins/out_es/es.h
📚 Learning: 2025-11-11T20:33:42.883Z
Learnt from: mabrarov
Repo: fluent/fluent-bit PR: 7608
File: plugins/out_es/es_conf.c:932-946
Timestamp: 2025-11-11T20:33:42.883Z
Learning: The mk_list_is_empty function in Fluent Bit returns 0 when a list IS empty and -1 when a list is NOT empty. Use `mk_list_is_empty(list) == 0` to check if empty, and `mk_list_is_empty(list) != 0` to check if not empty.
Applied to files:
plugins/out_es/es_conf.c
🧬 Code graph analysis (6)
plugins/out_es/es_conf_parse.c (8)
src/flb_utils.c (1)
flb_utils_split(464-467)plugins/out_es/es_type.c (5)
flb_es_str_copy_str(46-61)flb_es_list_move_list(117-124)flb_es_aws_provider_set(192-203)flb_es_aws_provider_move(205-216)flb_es_aws_provider_move_provider(218-226)src/flb_sds.c (2)
flb_sds_create(78-90)flb_sds_destroy(389-399)src/flb_slist.c (2)
flb_slist_create(27-31)flb_slist_add(70-84)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(289-292)src/aws/flb_aws_credentials.c (1)
flb_standard_chain_provider_create(269-327)
plugins/out_es/es_type.h (1)
plugins/out_es/es_type.c (19)
flb_es_str_destroy(30-37)flb_es_str_set_str(39-44)flb_es_str_copy_str(46-61)flb_es_sds_destroy(63-70)flb_es_sds_set_sds(72-79)flb_es_sds_copy_str(81-96)flb_es_list_destroy(98-106)flb_es_list_set_list(108-115)flb_es_list_move_list(117-124)flb_es_tls_destroy(126-133)flb_es_tls_set_tls(135-142)flb_es_tls_move_tls(144-151)flb_es_ra_destroy(153-160)flb_es_ra_set_ra(162-170)flb_es_ra_move_ra(172-180)flb_es_aws_provider_destroy(183-190)flb_es_aws_provider_set(192-203)flb_es_aws_provider_move(205-216)flb_es_aws_provider_move_provider(218-226)
plugins/out_es/es_type.c (6)
include/fluent-bit/flb_mem.h (1)
flb_free(126-128)src/flb_sds.c (2)
flb_sds_destroy(389-399)flb_sds_create(78-90)src/flb_slist.c (1)
flb_slist_destroy(327-339)src/tls/flb_tls.c (1)
flb_tls_destroy(258-277)src/flb_record_accessor.c (1)
flb_ra_destroy(232-248)src/aws/flb_aws_credentials.c (1)
flb_aws_provider_destroy(769-791)
tests/runtime/out_elasticsearch.c (2)
plugins/out_es/es.c (1)
flb_elasticsearch_target(652-674)src/flb_lib.c (7)
flb_output_set_test_flush_ctx_callback(617-649)flb_start(1017-1028)flb_lib_push(877-904)flb_input(266-276)flb_output(279-289)flb_output_set(520-551)flb_output_set_test(584-615)
plugins/out_es/es.h (1)
plugins/out_es/es.c (1)
flb_elasticsearch_target(652-674)
plugins/out_es/es_conf.c (4)
include/fluent-bit/flb_mem.h (2)
flb_free(126-128)flb_calloc(84-96)plugins/out_es/es_conf_parse.c (3)
flb_es_conf_set_cloud_auth(142-192)flb_es_conf_set_cloud_credentials(38-73)flb_es_set_aws_unsigned_headers(196-215)include/fluent-bit/flb_output.h (1)
flb_output_config_map_set(1321-1348)include/fluent-bit/flb_upstream_node.h (2)
flb_upstream_node_set_data(82-86)flb_upstream_node_get_data(88-91)
⏰ 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). (81)
- GitHub Check: PR - container builds / Windows container images (2022)
- GitHub Check: PR - container builds / Windows container images (2025)
- GitHub Check: PR - packages build Linux / ubuntu/22.04.arm64v8 package build and stage to S3
- GitHub Check: PR - packages build Linux / debian/trixie.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 / almalinux/8.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 / debian/buster package build and stage to S3
- GitHub Check: PR - packages build Linux / almalinux/10.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 / debian/buster.arm64v8 package build and stage to S3
- GitHub Check: PR - packages build Linux / debian/trixie 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 / almalinux/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 / debian/bookworm.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 / almalinux/9.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 / almalinux/10 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 / debian/bullseye.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 / rockylinux/10 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/2023 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 / rockylinux/9.arm64v8 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 / centos/10.arm64v8 package build and stage to S3
- GitHub Check: PR - packages build Linux / rockylinux/10.arm64v8 package build and stage to S3
- GitHub Check: PR - packages build Linux / centos/10 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 / centos/8.arm64v8 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 / centos/7.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 / amazonlinux/2.arm64v8 package build and stage to S3
- GitHub Check: PR - container builds / amd64/production container image build
- GitHub Check: PR - container builds / arm/v7/production container image build
- GitHub Check: PR - container builds / amd64/debug container image build
- GitHub Check: PR - container builds / arm/v7/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 - 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 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 MacOS / call-build-macos-package (Intel macOS runner, macos-14-large, 3.31.6)
- GitHub Check: PR - packages build MacOS / call-build-macos-package (Apple Silicon macOS runner, macos-14, 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 64bit, x64, x64-windows-static, 3.31.6)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 32bit, x86, x86-windows-static, 3.31.6)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_COMPILER_STRICT_POINTER_TYPES=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, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_ARROW=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, gcc, g++)
- 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, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-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_THREAD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, 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 (-DSANITIZE_ADDRESS=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_COVERAGE=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, gcc, g++)
- 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, 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-without-cxx (3.31.6)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-24.04, clang-14)
- GitHub Check: PR - fuzzing test
🔇 Additional comments (38)
plugins/out_es/es_type.h (7)
1-49: Well-structured ownership wrapper design.The
flb_es_strstructure and its accompanying API provide a clean encapsulation of ownership semantics. The documentation clearly explains when direct member access is permitted (initialization and modification whenownedisFLB_FALSE).
51-121: Consistent wrapper pattern across types.The
flb_es_sds_t,flb_es_list,flb_es_tls, andflb_es_record_accessorstructures follow the same ownership pattern. This consistency will make the codebase easier to maintain and understand.
123-141: AWS provider wrapper correctly guarded.The
flb_es_aws_providerstructure is properly wrapped in#ifdef FLB_HAVE_AWSto ensure it only compiles when AWS support is enabled.
143-185: Clear API documentation with intersection constraints.The documentation for
flb_es_str_set_strandflb_es_str_copy_strclearly documents the constraint that source and destination values must not intersect. This is important to prevent use-after-free when the destination is destroyed before assignment.
187-262: SDS and list APIs are complete and well-documented.The APIs for
flb_es_sds_tandflb_es_listprovide the necessary operations (destroy, set, copy/move) with clear ownership transfer semantics.
264-341: TLS and record accessor APIs follow established pattern.The
flb_es_tls_*andflb_es_ra_*functions provide consistent move semantics for transferring ownership of TLS contexts and record accessors.
343-407: AWS provider API provides additional flexibility.The
flb_es_aws_provider_set,flb_es_aws_provider_move, andflb_es_aws_provider_move_providerfunctions provide more granular control over ownership transfer, including a self-assignment check (dest == src). This is appropriate given the complexity of AWS provider lifecycle management.plugins/out_es/es_type.c (6)
30-61: String wrapper implementations are correct.The
flb_es_str_destroy,flb_es_str_set_str, andflb_es_str_copy_strfunctions correctly implement the ownership semantics:
destroyonly frees when ownedset_strdestroys first, then assigns without ownershipcopy_strcreates a duplicate before destroying to handle the allocation failure case gracefully
63-96: SDS wrapper implementations follow the same pattern.The SDS functions correctly use
flb_sds_destroyandflb_sds_createfrom the Fluent Bit API. The null check inflb_es_sds_set_sds(line 75) properly handles the case wheresrcis NULL by not settingdest->value.
98-124: List wrapper correctly handles both list and container memory.
flb_es_list_destroyproperly callsflb_slist_destroyto free list entries and thenflb_freeto release the list container itself. This matches the expected cleanup for dynamically allocatedmk_liststructures.
126-180: TLS and record accessor wrappers are correct.The implementations correctly delegate to
flb_tls_destroyandflb_ra_destroyfor cleanup, and properly manage the ownership flag during set and move operations.
182-227: AWS provider wrappers handle edge cases well.The
flb_es_aws_provider_movefunction correctly handles the struct copy and nulls the source value. The self-assignment check (dest == src) prevents issues when the same wrapper is passed as both source and destination.One minor observation:
flb_es_aws_provider_moveat line 213 does*dest = *srcwhich copies bothvalueandownedfields, which is the intended behavior for ownership transfer.
20-28: No build issue with this include. Theflb_aws_credentials.hheader itself is wrapped with#ifdef FLB_HAVE_AWS, so unconditional includes are safe—when the flag is not defined, the preprocessor skips the header's content. This pattern is used consistently across the codebase in kinesis, S3, OpenSearch, CloudWatch, and other AWS-related files without guards.Likely an incorrect or invalid review comment.
src/flb_engine_dispatch.c (2)
101-119: Clean implementation of dynamic flush context extraction.The changes to
test_run_formattercorrectly implement the callback-based flush context mechanism:
- Initializes
flush_ctxfromotf->flush_ctx(line 116)- If
flush_ctx_callbackis provided, invokes it to potentially override the context (lines 117-119)- The callback receives all necessary context: config, input instance, plugin context, and the base flush_ctx
This enables the ES output plugin tests to extract the appropriate per-upstream-node configuration during test execution.
179-190: Call site correctly updated.The call to
test_run_formatterat line 184 no longer passesflush_ctxas an argument, which aligns with the updated function signature.include/fluent-bit/flb_output.h (1)
154-171: Well-documented flush context callback addition.The
flush_ctx_callbackfunction pointer is properly documented and its parameters are clearly labeled:
struct flb_config *- Fluent Bit contextstruct flb_input_instance *- plugin that ingested recordsvoid *plugin_context- plugin instance contextvoid *flush_ctx- context for the callbackThe comment for
flush_ctx(line 154-155) is updated to reflect its role as context for the callback, maintaining clarity about its purpose.src/flb_lib.c (2)
584-615: Parameter rename improves API clarity.The rename from
test_ctxtoflush_ctx(line 587) and assignment too_ins->test_formatter.flush_ctx(line 608) better reflects the actual usage of this parameter as the flush context for the formatter callback.
617-649: Function designed for use afterflb_output_set_test—consider documenting this dependency.The
flb_output_set_test_flush_ctx_callbackfunction follows the intended design. It allows tests to register a dynamic flush context callback in addition to the test formatter callback initialized byflb_output_set_test. Every test case in the codebase (30+ instances in out_elasticsearch.c) callsflb_output_set_testfirst to initialize the formatter test callback, then calls this function to register the dynamic flush context callback.Calling
flb_output_set_test_flush_ctx_callbackalone would not be problematic from a crash perspective—the flush context callback is conditionally invoked at lines 117–119 intest_run_formatteronly if set. However, the test would not execute at all becausetest_run_formatteris only invoked iftest_formatter.callback != NULL(checked at line 181 in flb_engine_dispatch.c), a field set exclusively byflb_output_set_test. Documenting this dependency would prevent misuse.tests/runtime/out_elasticsearch.c (4)
113-122: LGTM!The callback correctly extracts the Elasticsearch configuration context using
flb_elasticsearch_target(), which aligns with the new per-node configuration model. The unusednodeoutput parameter is acceptable here since tests only need the configuration.
330-333: LGTM!The
flb_output_set_test_flush_ctx_callbackregistration is consistently added across all formatter tests. This is necessary for the ec-centric configuration model whereelasticsearch_format()receives the configuration viaflush_ctxparameter.
1076-1137: Well-structured upstream configuration override test.This test correctly verifies that per-node configuration (
write_operation: indexin upstream file) overrides plugin-level configuration (Write_Operation: Upsert). The callbackcb_check_write_op_indexvalidates the node-level setting takes precedence.Cleanup at lines 1135-1136 properly removes the temporary file and frees the allocated filename.
1618-1623: LGTM!The new upstream tests are properly registered in
TEST_LIST, enabling comprehensive testing of the upstream/HA configuration feature.plugins/out_es/es_conf_parse.c (3)
196-215: Potential memory leak onflb_slist_addfailure.If
flb_slist_add()fails at line 210, the function returns-1without destroying the already-allocated list atec->aws_unsigned_headers.value. The caller will clean up viaelasticsearch_config_destroy(), but that function checks ownership viaflb_es_list_destroy()which should handle this correctly.Actually, upon re-reading
flb_es_list_move_listusage and the destroy pattern, the cleanup is handled by the caller invokingelasticsearch_config_destroy. The owned flag ensures proper cleanup.
38-73: LGTM!OOM handling was addressed per past review comments. The function now properly checks
flb_es_str_copy_strreturn values (lines 57, 64) and freestoksbefore returning-1on allocation failure.
279-345: LGTM!The AWS provider configuration flow properly:
- Validates
aws_authandaws_regionsettings- Creates dedicated TLS contexts for AWS and STS providers
- Uses ownership-aware helpers (
flb_es_tls_move_tls,flb_es_aws_provider_move_provider)- Initializes credentials synchronously, then switches to async mode
- Wires the provider upstream to the instance
Error paths return
-1allowing the caller to cleanup viaelasticsearch_config_destroy().plugins/out_es/es_conf.c (2)
887-901: LGTM!The
mk_list_is_emptycheck is correct. Based on retrieved learnings,mk_list_is_emptyreturns0when the list IS empty and-1when NOT empty. The condition== 0correctly returnsNULLfor an empty list.
636-745: LGTM!The HA configuration setup is well-structured:
- Creates main
ecfrom plugin properties- Loads upstream nodes and iterates to create per-node
eccontexts- Each node
ecinherits from mainecthen applies node-specific overrides- On any failure, properly cleans up all allocated resources
- Registers node data for later retrieval via
flb_upstream_node_get_data()plugins/out_es/es_conf_prop.h (1)
20-65: LGTM!The property macro definitions provide a single source of truth for configuration property names, reducing the risk of typos and improving maintainability. AWS-specific properties are correctly guarded by
FLB_HAVE_AWS.plugins/out_es/es.c (5)
652-674: LGTM!The
flb_elasticsearch_targetfunction correctly handles both HA and non-HA modes:
- Non-HA: Returns the single config via
flb_es_upstream_conf(ctx, NULL)and sets*node = NULL- HA: Gets next node via round-robin (
flb_upstream_ha_node_get), retrieves node-specific configPer retrieved learnings, the author prefers function contracts that assume valid parameters rather than defensive NULL checks.
855-875: LGTM!The upstream connection acquisition correctly branches on
ha_mode:
- HA mode: Gets connection from the selected node's upstream (
node->u)- Non-HA mode: Gets connection from the single upstream (
ctx->u)The logic ensures
node != NULLwhenha_mode == TRUEbecauseflb_elasticsearch_targetreturnsNULL(caught at line 856) if no HA node is available.
1040-1057: LGTM!The unified cleanup path is well-structured:
- Conditional HTTP client destruction (handles NULL case from early errors)
- Pack buffer freed unconditionally (always allocated by this point)
- Upstream connection released
- Signature destroyed if allocated
The
retrylabel simply setsret = FLB_RETRYand falls through to cleanup.
1150-1159: LGTM!The nested offset calculation pattern is correct for the new ownership-aware wrapper types:
offsetof(struct flb_elasticsearch_config, index) + offsetof(struct flb_es_str, value)This allows
flb_config_map_setto directly populate the.valuemember while ownership tracking is managed separately by theflb_es_*helper functions.
1385-1389: LGTM!The new
Upstreamproperty enables the HA/upstream servers feature, allowing users to specify a path to an upstream configuration file that defines multiple Elasticsearch nodes.plugins/out_es/es.h (5)
23-26: LGTM!The includes are minimal and appropriate.
mk_list.his needed for the linked list member, andes_type.hprovides the wrapper types for ownership-aware fields. The forward declaration refactor from the previous review is properly applied.
28-28: Good catch fixing the default port typo.The Elasticsearch default HTTP port is 9200, not 92000. This was a critical typo that would have caused connection failures when relying on the default port.
149-161: LGTM!The struct cleanly separates HA mode fields (
ha_mode,ha_upstream,ha) from the single-mode upstream handler (u). Theconfigslist enables per-upstream-node configuration overrides, following the pattern established by the Forward output plugin.
163-178: Well-documented API declaration.The documentation clearly explains the behavior in both HA and non-HA modes, properly documents the output parameter semantics, and describes the return value conditions. The signature matches the implementation in
es.c.
53-147: Wrapper-based ownership tracking implementation is correct.The struct uses wrapper types (
flb_es_str,flb_es_sds_t,flb_es_record_accessor, etc.) for fields that can be overridden per upstream node, while keeping raw pointers for fields managed byflb_config_map. This design appropriately addresses ownership tracking.The
es_actionfield on line 127 is safely implemented as aconst char *that points to static constants (es_write_op_index,es_write_op_create,es_write_op_update,es_write_op_upsertdefined ines_conf.c). It is derived from thewrite_operationwrapper field and never independently allocated, making the raw pointer approach sound.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
plugins/out_es/es_conf.c (1)
100-107: Signed/unsigned comparison warning likely.At line 103, comparing
size_t buffer_size(unsigned) with-1(signed) may trigger compiler warnings. The comparison works because-1is implicitly converted toSIZE_MAX, but consider making this explicit for clarity.Optional fix for clarity
static size_t config_adjust_buffer_size(size_t buffer_size) { /* HTTP Payload (response) maximum buffer size (0 == unlimited) */ - if (buffer_size == -1) { + if (buffer_size == (size_t)-1) { return 0; } return buffer_size; }plugins/out_es/es_type.h (2)
143-183: Consider adding amovevariant forflb_es_strfor API consistency.The
flb_es_strtype hasdestroy,set_str, andcopy_str, but nomovefunction to transfer ownership of an existing allocated string. Other complex types (list, tls, record_accessor) providemovevariants.If callers ever need to transfer an already-allocated
char*into the wrapper with ownership, they would benefit from aflb_es_str_move_strfunction.
357-394: Self-assignment handling is inconsistent across types.The
flb_es_aws_provider_setandflb_es_aws_provider_movefunctions explicitly handle thedest == srccase by doing nothing. However, other types'setandmovefunctions don't document or handle self-assignment scenarios.For example, if someone calls
flb_es_str_set_str(&x, x.value)whenx.owned == FLB_TRUE, the implementation would freex.valuebefore assigning it—resulting in a dangling pointer.Consider either adding self-assignment protection to all types'
set/movefunctions, or explicitly documenting that self-assignment is undefined behavior for the other wrapper types.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
plugins/out_es/CMakeLists.txtplugins/out_es/es.cplugins/out_es/es.hplugins/out_es/es_conf.cplugins/out_es/es_conf_parse.cplugins/out_es/es_type.cplugins/out_es/es_type.h
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/out_es/es_type.c
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-08-20T02:04:27.220Z
Learnt from: mabrarov
Repo: fluent/fluent-bit PR: 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.cplugins/out_es/es_conf_parse.c
📚 Learning: 2025-09-22T15:59:55.794Z
Learnt from: nicknezis
Repo: fluent/fluent-bit PR: 10882
File: plugins/out_http/http.c:112-116
Timestamp: 2025-09-22T15:59:55.794Z
Learning: When users consider bug fixes out of scope for their focused PRs, it's appropriate to create separate GitHub issues to track those concerns rather than expanding the current PR scope.
Applied to files:
plugins/out_es/es.cplugins/out_es/es_conf_parse.c
📚 Learning: 2025-10-06T17:39:21.606Z
Learnt from: edsiper
Repo: fluent/fluent-bit PR: 10967
File: plugins/in_elasticsearch/in_elasticsearch_bulk_prot.c:266-272
Timestamp: 2025-10-06T17:39:21.606Z
Learning: In the in_elasticsearch plugin, payloads are JSON-based, so tag_key values will always be strings. There's no need to handle binary (FLB_RA_BINARY) values when extracting tags from records in this plugin.
Applied to files:
plugins/out_es/es.c
📚 Learning: 2025-09-08T11:21:33.975Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 10851
File: include/fluent-bit/flb_simd.h:60-66
Timestamp: 2025-09-08T11:21:33.975Z
Learning: Fluent Bit currently only supports MSVC compiler on Windows, so additional compiler compatibility guards may be unnecessary for Windows-specific code paths.
Applied to files:
plugins/out_es/es_conf_parse.c
📚 Learning: 2025-12-17T18:45:02.283Z
Learnt from: edsiper
Repo: fluent/fluent-bit PR: 11286
File: src/flb_http_client.c:1657-1715
Timestamp: 2025-12-17T18:45:02.283Z
Learning: In the Fluent Bit OAuth2 implementation (src/flb_oauth2.c), the function flb_oauth2_get_access_token returns a pointer to the cached token stored in the OAuth2 context (ctx->access_token), not a new allocation. The OAuth2 context manages the token lifecycle, so callers must not free the returned token pointer.
Applied to files:
plugins/out_es/es_conf_parse.c
📚 Learning: 2025-11-11T20:33:42.883Z
Learnt from: mabrarov
Repo: fluent/fluent-bit PR: 7608
File: plugins/out_es/es_conf.c:932-946
Timestamp: 2025-11-11T20:33:42.883Z
Learning: The mk_list_is_empty function in Fluent Bit returns 0 when a list IS empty and -1 when a list is NOT empty. Use `mk_list_is_empty(list) == 0` to check if empty, and `mk_list_is_empty(list) != 0` to check if not empty.
Applied to files:
plugins/out_es/es_conf.c
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 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:
plugins/out_es/es.h
🧬 Code graph analysis (4)
plugins/out_es/es.c (3)
plugins/out_es/es_conf.c (1)
flb_es_upstream_conf(887-901)src/flb_upstream_ha.c (1)
flb_upstream_ha_node_get(88-110)src/flb_upstream.c (1)
flb_upstream_conn_get(711-844)
plugins/out_es/es_conf_parse.c (4)
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/flb_slist.c (2)
flb_slist_create(27-31)flb_slist_add(70-84)include/fluent-bit/flb_mem.h (1)
flb_free(126-128)
plugins/out_es/es_conf.c (5)
plugins/out_es/es_type.c (5)
flb_es_ra_move_ra(172-180)flb_es_str_copy_str(46-61)flb_es_str_set_str(39-44)flb_es_ra_destroy(153-160)flb_es_str_destroy(30-37)plugins/out_es/es_conf_parse.c (2)
flb_es_conf_set_cloud_auth(142-192)flb_es_conf_set_cloud_credentials(38-73)include/fluent-bit/flb_output.h (1)
flb_output_config_map_set(1321-1348)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(656-698)
plugins/out_es/es.h (1)
plugins/out_es/es.c (1)
flb_elasticsearch_target(652-674)
⏰ 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). (81)
- GitHub Check: PR - container builds / Windows container images (2022)
- GitHub Check: PR - container builds / Windows container images (2025)
- GitHub Check: PR - packages build Linux / centos/8.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 / ubuntu/24.04.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 / almalinux/8 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 / ubuntu/22.04.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 / ubuntu/24.04 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 / debian/bookworm package build and stage to S3
- GitHub Check: PR - packages build Linux / debian/trixie.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 / rockylinux/8.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 / almalinux/9 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 / ubuntu/22.04 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 / centos/9.arm64v8 package build and stage to S3
- GitHub Check: PR - packages build Linux / debian/trixie package build and stage to S3
- GitHub Check: PR - packages build Linux / almalinux/10 package build and stage to S3
- GitHub Check: PR - packages build Linux / almalinux/10.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 / rockylinux/10.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 / centos/8 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 / rockylinux/10 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/10.arm64v8 package build and stage to S3
- GitHub Check: PR - packages build Linux / centos/10 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 / amazonlinux/2023 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/2.arm64v8 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/debug container image build
- GitHub Check: PR - container builds / arm64/production container image build
- GitHub Check: PR - container builds / amd64/debug 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 - container builds / arm/v7/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 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 - packages build Windows / call-build-windows-package (Windows 32bit, x86, x86-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: pr-windows-build / call-build-windows-package (Windows 64bit, x64, x64-windows-static, 3.31.6)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_ARROW=On, 3.31.6, gcc, g++)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-22.04, clang-12)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_COMPILER_STRICT_POINTER_TYPES=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=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, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=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_SANITIZE_MEMORY=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_JEMALLOC=Off, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, gcc, g++)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-24.04, clang-14)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=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_COVERAGE=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_SIMD=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 (-DSANITIZE_ADDRESS=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: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=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, gcc, g++, ubuntu-24.04, clang-14)
- GitHub Check: pr-compile-without-cxx (3.31.6)
- GitHub Check: PR - fuzzing test
🔇 Additional comments (28)
plugins/out_es/es_conf_parse.c (3)
38-73: Cloud credentials parsing looks correct.The OOM handling for
flb_strdupwas addressed per previous review feedback. The function properly cleans uptokson allocation failure and returns-1.Note: When
cloud_passwdallocation fails,cloud_userremains allocated with its ownership flag set. This is intentional - as confirmed in past reviews, the caller (elasticsearch_config_destroy) will clean up partial allocations based on the individual ownership flags.
83-140: Buffer overflow risk tracked in issue#11368.This is migrated code from the original
es_conf.c. The potential buffer overflow withstrcpy/strcatoperations oncloud_host_buf[256]has already been flagged and tracked in issue#11368for follow-up.
279-345: AWS provider setup is well-structured.The function properly:
- Validates AWS auth and region settings
- Creates TLS context with proper error handling
- Initializes credentials synchronously before switching to async mode
- Wires the provider upstream to Fluent Bit
plugins/out_es/CMakeLists.txt (1)
1-10: Build configuration correctly updated.The new source files
es_type.candes_conf_parse.care properly added to the CMake source list, enabling compilation of the new modular ES configuration and type wrapper infrastructure.plugins/out_es/es_conf.c (12)
36-61: Helperconfig_set_ra_id_keyis correct.Properly creates a record accessor from
id_key, handles NULL input, and warns whengenerate_idconflicts withid_key.
63-98: Write operation validation is thorough.The function correctly:
- Maps write operation strings to static constants
- Validates that
update/upsertoperations requireId_KeyorGenerate_Id- Returns clear error messages
117-138: Pipeline/path URI construction is safe.The function properly uses
snprintfwith bounds checking and returns-1if the URI would be truncated.
140-177: Prefix key record accessor setup is correct.Properly handles the case where the key doesn't start with
$by prepending it, and correctly frees the temporary buffer after creating the record accessor.
179-289: Main configuration property setup is comprehensive.The function properly orchestrates:
- Cloud ID/auth handling
- Config map population
- Buffer size adjustment
- Index/type from URI
- Pipeline and path setup
- Record accessor initialization
- AWS authentication (conditionally compiled)
Error handling is consistent with
-1returns on failure.
291-357: Base configuration copy uses proper non-owning references.The function correctly uses
flb_es_str_set_str,flb_es_sds_set_sds, andflb_es_ra_set_rato create non-owning references from the base config, avoiding double-free issues when both base and node configs share the same underlying data.
359-593: Per-node property overrides are well-structured.The function correctly:
- Uses non-owning
set_strfor properties that reference node-owned strings- Uses owning
copy_strfor properties that need per-node copies- Recreates AWS provider when any AWS-related setting changes per-node
The logic at lines 578-590 properly detects when a new AWS provider is needed based on auth changes or provider-specific settings.
595-604: Validation logic preserved from original code.As noted in past reviews, there's a potential edge case where
current_time_index == TRUEwithindex.value == NULLcould pass validation but cause issues instrftime. This is pre-existing behavior from the original code and outside the scope of this PR.
606-634: Resource cleanup is thorough.The destroy function properly releases all owned resources using the ownership-aware
flb_es_*_destroyhelpers, ensuring no double-frees when configurations share underlying data.
636-745: HA configuration setup with proper error recovery.The function:
- Creates main elasticsearch_config for base properties
- Loads upstream HA nodes from file
- Creates per-node configurations with property overrides
- Properly cleans up all allocated resources on any failure
The cleanup path (lines 722-738) correctly:
- Nullifies node data pointers
- Removes and destroys per-node configs from the list
- Destroys the HA upstream and base config
747-817: Simple (non-HA) configuration setup is correct.The function properly:
- Sets default host/port
- Creates single elasticsearch_config
- Handles TLS and IPv6 flags
- Creates upstream connection with proper cleanup on failure
887-901: Upstream config accessor correctly handles empty list.Per the learned behavior of
mk_list_is_empty(returns 0 when empty, -1 when not empty), the check at line 897 correctly returns NULL when the configs list is empty.Based on learnings,
mk_list_is_empty(&ctx->configs) == 0means the list IS empty.plugins/out_es/es.c (5)
46-85: AWS auth and pack helpers correctly use ec.The refactored functions now accept
struct flb_elasticsearch_config *ecand properly source AWS region, service name, unsigned headers, and provider from the ec context.
287-327: Format function correctly receives ec via flush_ctx.The
elasticsearch_formatfunction now receivesflush_ctx(the per-upstream elasticsearch_config) and uses it for all configuration lookups. This enables per-node configuration in HA mode.
652-674: Target selection with documented contract.The function correctly implements HA node selection via round-robin and single-mode fallback. The contract (ctx and node must be non-NULL) is documented in es.h lines 163-177.
Based on learnings, defensive null checks are intentionally omitted to maintain performance and code clarity.
835-876: Flush path correctly handles HA and non-HA modes.The refactored flush callback:
- Obtains target configuration via
flb_elasticsearch_target- Gets upstream connection from the appropriate source (node->u in HA mode, ctx->u otherwise)
- Provides clear error messages distinguishing HA node failures from single-mode failures
1149-1393: Config map correctly targets elasticsearch_config with nested offsets.The config map entries properly use:
FLB_ES_CONFIG_PROPERTY_*macros for property names- Nested
offsetofexpressions to target.valuefields within wrapper types (e.g.,offsetof(struct flb_elasticsearch_config, index) + offsetof(struct flb_es_str, value))- New
Upstreamproperty for HA configuration file pathplugins/out_es/es.h (3)
23-51: Header coupling reduced with forward declarations.As addressed per past review feedback, heavy includes were replaced with forward declarations for:
struct flb_upstreamstruct flb_upstream_hastruct flb_upstream_nodestruct flb_output_instanceThis reduces transitive rebuilds and compile time.
53-161: Configuration structures well-designed for HA mode.The separation between
flb_elasticsearch_config(per-node settings) andflb_elasticsearch(plugin instance with HA routing) enables:
- Shared base configuration with per-node overrides
- Safe resource cleanup via ownership-aware wrapper types
- List-based management of multiple configurations
The
_headmember enables linking configs for lifecycle management.
163-178: API documentation added forflb_elasticsearch_target.The documentation clearly specifies:
- Parameter contracts (non-NULL requirements)
- Output parameter behavior
- Return value semantics
This addresses past review feedback about documenting function contracts.
plugins/out_es/es_type.h (4)
33-141: Well-designed ownership wrapper types.The struct definitions follow a consistent pattern and are well-documented. The ownership flag approach addresses the configuration sharing concern between plugin-level and per-upstream-node configs. The documentation clearly specifies when direct member access is permitted.
396-411: Clear distinction between wrapper and raw pointer operations.The
flb_es_aws_provider_move_providerfunction clearly indicates it takes a rawflb_aws_provider*rather than a wrapper, distinguishing it fromflb_es_aws_provider_movewhich operates on wrapper-to-wrapper transfers. This naming convention is helpful.
1-32: Well-structured header with appropriate forward declarations.The header properly includes only what's needed (
flb_sds.h) and uses forward declarations for external types (mk_list,flb_tls,flb_record_accessor,flb_aws_provider). This minimizes header dependencies and compilation coupling.
20-21: Include guard naming is correct.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@plugins/out_es/es_conf.c`:
- Around line 385-575: The code allows per-node overrides of generate_id and
id_key without re-checking update/upsert rules because config_set_es_action is
only called when write_operation is overridden; fix by re-validating after
per-node overrides: when processing FLB_ES_CONFIG_PROPERTY_GENERATE_ID or
FLB_ES_CONFIG_PROPERTY_ID_KEY (or after all per-node properties are applied)
call the same validation used by config_set_es_action using
ec->write_operation.value, ec->ra_id_key.value and ec->generate_id (or invoke a
small helper that checks update/upsert require an _id) and return -1 on failure
so invalid per-node configs are rejected; references: generate_id, id_key,
config_set_es_action, FLB_ES_CONFIG_PROPERTY_GENERATE_ID,
FLB_ES_CONFIG_PROPERTY_ID_KEY, ec and ctx.
- Around line 833-848: The output context is set with
flb_output_set_context(ins, ctx) before calling es_config_ha/es_config_simple,
so if those return non-zero we free ctx but leave ins pointing at freed memory;
move or mirror the fix so the output instance never retains a dangling pointer
by either delaying flb_output_set_context(ins, ctx) until after ret == 0 (i.e.,
after successful es_config_ha/es_config_simple) or by calling
flb_output_set_context(ins, NULL) immediately before flb_free(ctx) when ret !=
0; update the init path around flb_output_set_context, ctx, es_config_ha, and
es_config_simple accordingly.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@plugins/out_es/es_conf_parse.c`:
- Around line 51-53: Guard against flb_utils_split returning NULL: check the
returned pointer (toks) immediately after calling flb_utils_split((const char
*)cloud_auth, ':', -1) and if it is NULL, log/return an error (or appropriate
failure code from the surrounding function) instead of proceeding; this prevents
mk_list_foreach(head, toks) from dereferencing a NULL pointer. Update the code
around the flb_utils_split call to perform the NULL check and handle the failure
path cleanly before using toks.
In `@plugins/out_es/es_type.h`:
- Around line 224-233: The doc comment for flb_es_slist_destroy incorrectly
references the type flb_es_list; update the parameter description to mention the
correct type flb_es_slist instead. Locate the comment block above the function
declaration flb_es_slist_destroy and replace "flb_es_list" with "flb_es_slist"
so the `@param` description and explanatory text consistently refer to the struct
flb_es_slist.
♻️ Duplicate comments (1)
tests/runtime/out_elasticsearch.c (1)
22-38: Avoidtmpnam(NULL); prefer a caller‑supplied buffer (ormkstemp).
tmpnam(NULL)relies on a static buffer and is prone to race conditions; if POSIX is acceptable,mkstempis safer, otherwise usetmpnam(buf).🔧 Suggested fix (C‑standard buffer usage)
- 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_filename = flb_malloc(L_tmpnam); + if (upstream_conf_filename == NULL) { + return NULL; + } + if (tmpnam(upstream_conf_filename) == NULL) { + flb_free(upstream_conf_filename); + return NULL; + }
|
Hi dear reviewers, I have addressed all comments in this pull request and I consider it ready for review. Thank you. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@plugins/out_es/es_conf.c`:
- Around line 369-506: Several boolean config assignments call flb_utils_bool()
and assign its result directly to flags (e.g., ec->suppress_type_name,
ec->generate_id, ec->has_aws_auth, ec->logstash_format, ec->time_key_nanos,
ec->include_tag_key, ec->replace_dots, ec->current_time_index, ec->trace_output,
ec->trace_error); flb_utils_bool() returns -1 on invalid input so you must treat
that as a config error. For each place where flb_utils_bool(tmp) is used, store
the result in a temporary int (e.g., int val = flb_utils_bool(tmp)); if val ==
-1 return -1 (abort); otherwise assign ec->field = val; apply this pattern to
all boolean overrides read via flb_upstream_node_get_property in this diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@plugins/out_es/es.c`:
- Around line 1279-1282: The config_map entry for logstash_dateformat is writing
the pointer at the wrong place because logstash_dateformat is a flb_es_sds_t and
its entry is missing the "+ offsetof(struct flb_es_sds_t, value)" adjustment;
update the config_map entry for logstash_dateformat (in the block where
FLB_ES_CONFIG_PROPERTY_LOGSTASH_DATEFORMAT is defined) to use offsetof(struct
flb_elasticsearch_config, logstash_dateformat) + offsetof(struct flb_es_sds_t,
value) so it matches the other flb_es_sds_t fields and prevents corrupting the
flb_es_sds_t wrapper.
🧹 Nitpick comments (5)
tests/runtime/out_elasticsearch.c (1)
22-112:create_upstream_conf_filehelper is functional but has a minor portability note.The
tmpnamusage with an owned buffer (per past review discussion) is acceptable for test code. The varargs-based config generation witharg_idx ^= 1toggling is a compact approach.One minor note: if
first_propertyis non-NULL but the varargs list has an odd number of entries (missing value for a property name), the loop will write a bare newline for the missing value and continue. This is unlikely in practice since tests use explicitNULLterminators, but documenting the contract (pairs of name/value, NULL-terminated) in a comment would help future test authors.src/flb_lib.c (1)
617-649:flb_output_set_test_flush_ctx_callbackoverwritesrt_ctx/rt_ffdbut notrt_out_callback/rt_data.This function sets
test_mode,rt_ctx,rt_ffd,flush_ctx, andflush_ctx_callback— but it does not setrt_out_callbackorrt_data. This means it's only safe to call afterflb_output_set_testhas been called (which is the current usage in all tests). If called standalone, the formatter test would run but the runtime result checker would be missing.Consider either documenting that
flb_output_set_testmust be called first, or simplifyingflb_output_set_test_flush_ctx_callbackto only set the two fields it's responsible for (flush_ctxandflush_ctx_callback) without touching othertest_formattermembers.Simplified version that only sets the callback-specific fields
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 *), void *flush_ctx) { struct flb_output_instance *o_ins; o_ins = out_instance_get(ctx, ffd); if (!o_ins) { return -1; } - /* - * Enabling a test, set the output instance in 'test' mode, so no real - * flush callback is invoked, only the desired implemented test. - */ - /* Formatter test */ if (strcmp(test_name, "formatter") == 0) { - o_ins->test_mode = FLB_TRUE; - o_ins->test_formatter.rt_ctx = ctx; - o_ins->test_formatter.rt_ffd = ffd; o_ins->test_formatter.flush_ctx = flush_ctx; o_ins->test_formatter.flush_ctx_callback = flush_ctx_callback; } else { return -1; } return 0; }plugins/out_es/es_conf.c (1)
100-107:size_tcompared with-1— works but is implicit.
buffer_size == -1wherebuffer_sizeissize_tcompiles tobuffer_size == SIZE_MAX. This is correct but may trigger-Wsign-comparewarnings on some compilers. Consider using an explicit constant or cast for clarity.Optional: make the sentinel explicit
static size_t config_adjust_buffer_size(size_t buffer_size) { - /* HTTP Payload (response) maximum buffer size (0 == unlimited) */ - if (buffer_size == -1) { + /* HTTP Payload (response) maximum buffer size (0 == unlimited). + * flb_utils_size_to_bytes returns -1 on error, which when cast + * to size_t becomes SIZE_MAX. */ + if (buffer_size == (size_t) -1) { return 0; } return buffer_size; }plugins/out_es/es.h (1)
53-147: Ownership model forflb_elasticsearch_configfields is clear but mixing raw and wrapper types warrants a brief note.Fields like
http_user,http_passwd,http_api_keyare plainchar *(owned byflb_config_map), whilecloud_user/cloud_passwdusestruct flb_es_strand logstash fields usestruct flb_es_sds_t(shared-ownership wrappers). This asymmetry is intentional per the PR discussion, but a short comment near the rawchar *group explaining why they don't need wrappers (e.g., "owned by config_map, never shared across upstream nodes") would help future maintainers.plugins/out_es/es.c (1)
312-315: Remove commented-out variable declarations.These four commented-out lines (
msgpack_unpacked result,msgpack_object root,msgpack_object *obj) appear to be leftover from refactoring.♻️ Proposed cleanup
- // msgpack_unpacked result; - // msgpack_object root; msgpack_object map; - // msgpack_object *obj;
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@plugins/out_es/es_conf_parse.c`:
- Around line 58-70: The code currently checks ec->cloud_user.value and
ec->cloud_passwd.value after calling flb_es_str_copy_str(), but
flb_es_str_copy_str() returns an error on OOM while preserving the old dest
value, so change the logic in the parsing branch that handles items==1 and
items==2 to capture and check the return value of
flb_es_str_copy_str(&ec->cloud_user, entry->value) and
flb_es_str_copy_str(&ec->cloud_passwd, entry->value); if the call returns
non‑zero, call flb_utils_split_free(toks) and return -1 to reliably detect
allocation failure instead of inspecting dest->value.
In `@plugins/out_es/es_conf.c`:
- Around line 508-511: The code currently obtains tmp via
flb_upstream_node_get_property(FLB_ES_CONFIG_PROPERTY_BUFFER_SIZE) and calls
config_adjust_buffer_size(flb_utils_size_to_bytes(tmp)), which masks invalid
inputs because flb_utils_size_to_bytes() returns -1 for invalid strings; detect
that case before casting/adjusting: call flb_utils_size_to_bytes(tmp), check for
a negative return (e.g. -1) and treat it as a configuration error (log an error
and return/fail the config load for the upstream/node) instead of proceeding to
config_adjust_buffer_size; update the logic around tmp, flb_utils_size_to_bytes,
and ec->buffer_size so only valid positive byte sizes are passed to
config_adjust_buffer_size and assigned to ec->buffer_size.
In `@plugins/out_es/es.c`:
- Around line 917-923: The code currently treats flb_http_client(...) failure as
permanent by setting ret = FLB_ERROR and dropping the chunk; instead treat
allocation/connect failures as transient and return a retry code so Fluent Bit
will requeue the data. In the block that checks if (!c) (the flb_http_client
call creating variable c with u_conn and ec->uri, using pack/pack_size), change
the error handling to log the failure via flb_plg_error(ctx->ins, ...) and set
ret = FLB_RETRY (not FLB_ERROR) before jumping to cleanup_and_return so the
chunk is retried rather than discarded.
|
Hi @cosmo0920 and @patrick-stephens, This PR is pending for more than 2.5 years. I appreciate if you could find time and review it again. I believe I have addressed comment of @cosmo0920 which was the only concern at the time the comment was added. My winter vacation has ended and I won't be able to spend much time on this PR till summer, especially, considering new pending PRs for the same plugin (e.g. #11422 and #11375) - reabasing such changes can be hard. Thank you. |
|
I believe that similar changes can be implemented / helpful for OpenSearch output plugin (in a dedicated PR). |
…wn to parser of Upstream node configuration section are implemented, e.g. "host" and "port" Signed-off-by: Marat Abrarov <abrarov@gmail.com>
…o the test callback based on configuration of Fluent Bit and based on configuration of plugin Signed-off-by: Marat Abrarov <abrarov@gmail.com>
…llback contract Signed-off-by: Marat Abrarov <abrarov@gmail.com>
…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 <abrarov@gmail.com>
Signed-off-by: Marat Abrarov <abrarov@gmail.com>
…o the test callback based on configuration of Fluent Bit and based on configuration of plugin Signed-off-by: Marat Abrarov <abrarov@gmail.com>
Signed-off-by: Marat Abrarov <abrarov@gmail.com>
…icate username or password used for AWS authentication Signed-off-by: Marat Abrarov <abrarov@gmail.com>
…on and retrying buffer flush Signed-off-by: Marat Abrarov <abrarov@gmail.com>
…ent creation failed Signed-off-by: Marat Abrarov <abrarov@gmail.com>
…iguration data Signed-off-by: Marat Abrarov <abrarov@gmail.com>
…tials Signed-off-by: Marat Abrarov <abrarov@gmail.com>
8 participants
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
Integrations
Configuration
Quality
Tests