-
Notifications
You must be signed in to change notification settings - Fork 1.7k
out_es: add apikey to available auth types #10461
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
out_es: add apikey to available auth types #10461
Conversation
dd20b37
to
d3510d5
Compare
Configuration: service:
flush: 1
daemon: off
log_level: debug
pipeline:
inputs:
- name: forward
unix_path: /tmp/fluent.sock
tag: log
outputs:
- name: es
match: "*"
host: [ES HOST]
port: [ES PORT]
index: fluent-bit-test-10
http_api_key: [base 64 encoded key]
suppress_type_name: true
trace_error: true
trace_output: true
tls: true Response from debug log talking to elastic server: {
"errors": false,
"took": 0,
"items": [
{
"create": {
"_index": "fluent-bit-test-10",
"_id": "gLJ_YZcBuX40YbMfktq3",
"_version": 1,
"result": "created",
"_shards": {
"total": 2,
"successful": 2,
"failed": 0
},
"_seq_no": 8,
"_primary_term": 1,
"status": 201
}
},
{
"create": {
"_index": "fluent-bit-test-10",
"_id": "gbJ_YZcBuX40YbMfktq3",
"_version": 1,
"result": "created",
"_shards": {
"total": 2,
"successful": 2,
"failed": 0
},
"_seq_no": 9,
"_primary_term": 1,
"status": 201
}
},
{
"create": {
"_index": "fluent-bit-test-10",
"_id": "grJ_YZcBuX40YbMfktq3",
"_version": 1,
"result": "created",
"_shards": {
"total": 2,
"successful": 2,
"failed": 0
},
"_seq_no": 10,
"_primary_term": 1,
"status": 201
}
},
{
"create": {
"_index": "fluent-bit-test-10",
"_id": "g7J_YZcBuX40YbMfktq3",
"_version": 1,
"result": "created",
"_shards": {
"total": 2,
"successful": 2,
"failed": 0
},
"_seq_no": 11,
"_primary_term": 1,
"status": 201
}
},
{
"create": {
"_index": "fluent-bit-test-10",
"_id": "hLJ_YZcBuX40YbMfktq3",
"_version": 1,
"result": "created",
"_shards": {
"total": 2,
"successful": 2,
"f=ailed": 0
},
"_seq_no": 12,
"_primary_term": 1,
"status": 201
}
},
{
"create": {
"_index": "fluent-bit-test-10",
"_id": "hbJ_YZcBuX40YbMfktq3",
"_version": 1,
"result": "created",
"_shards": {
"total": 2,
"successful": 2,
"failed": 0
},
"_seq_no": 13,
"_primary_term": 1,
"status": 201
}
},
{
"create": {
"_index": "fluent-bit-test-10",
"_id": "hrJ_YZcBuX40YbMfktq3",
"_version": 1,
"result": "created",
"_shards": {
"total": 2,
"successful": 2,
"failed": 0
},
"_seq_no": 14,
"_primary_term": 1,
"status": 201
}
},
{
"create": {
"_index": "fluent-bit-test-10",
"_id": "h7J_YZcBuX40YbMfktq3",
"_version": 1,
"result": "created",
"_shards": {
"total": 2,
"successful": 2,
"failed": 0
},
"_seq_no": 15,
"_primary_term": 1,
"status": 201
}
},
{
"create": {
"_index": "fluent-bit-test-10",
"_id": "iLJ_YZcBuX40YbMfktq3",
"_version": 1,
"result": "created",
"_shards": {
"total": 2,
"successful": 2,
"failed": 0
},
"_seq_no": 16,
"_primary_term": 1,
"status": 201
}
}
]
} Full debug log:
Valgrind details:
|
Is there anything I can do to get this merged? I'd like to not have to use a fork of fluent-bit and username/password auth is difficult to manage from a secrets rotation perspective in elastic. |
Not to necro again, but Is there any way to get movement on this? |
Hi @tkennedy1-godaddy Thank you for adding this functionality. Hopefully, it gets reviewed soon. This is much-needed functionality. |
This would be exceptionally helpful for my large customers! |
Any chance this makes the cut for the next release? |
cd8258b
to
69caba0
Compare
Thank you @braydonk, feedback responded to |
WalkthroughAdds API key authentication to the Elasticsearch output plugin. Introduces a new http_api_key config option, stores it in the plugin context, and appends an Authorization: ApiKey header during flush when set. Updates struct definitions accordingly and adds a runtime test validating API key handling. Changes
Sequence Diagram(s)sequenceDiagram
participant Ingest as Record Ingest
participant ES as ES Output Plugin
participant HTTP as HTTP Client
Ingest->>ES: Flush request
alt HTTP Basic configured
ES->>HTTP: Authorization: Basic ...
else Cloud auth configured
ES->>HTTP: Authorization: Basic <cloud token>
else API key configured
ES->>HTTP: Authorization: ApiKey <key>
else AWS SigV4 configured
ES->>HTTP: Signed request (SigV4)
else
ES->>HTTP: No Authorization header
end
HTTP-->>ES: Response
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the "Integrations" page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ 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). (26)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
plugins/out_es/es.c (1)
917-927
: Reject combined AWS SigV4 and HTTP/Cloud/API key authentication
Mixingaws_auth
withhttp_user
/http_passwd
,http_api_key
orcloud_auth
leads to the SigV4 flow overwriting anyAuthorization
header. It’s safer to fail during init when these options are used together.• Location:
plugins/out_es/es.c
, insidecb_es_init
(after the existing index/logstash check)
• Add a guard that checksctx->has_aws_auth
alongsidectx->http_user
,ctx->http_api_key
orctx->cloud_auth
and returns-1
with an error.Proposed diff:
--- a/plugins/out_es/es.c +++ b/plugins/out_es/es.c @@ -630,6 +630,16 @@ static int cb_es_init(struct flb_output_instance *ins, if (ctx->index == NULL && ctx->logstash_format == FLB_FALSE && ctx->generate_id == FLB_FALSE) { flb_plg_error(ins, "cannot initialize plugin, index is not set and logstash_format and generate_id are both off"); + return -1; + } + + /* Prevent aws_sigv4 and other HTTP auth from being enabled together */ + if (ctx->has_aws_auth == FLB_TRUE && + (ctx->http_user != NULL || + ctx->http_api_key != NULL || + ctx->cloud_auth != NULL)) { + flb_plg_error(ins, + "invalid configuration: aws_auth cannot be used with http_user/http_passwd, http_api_key or cloud_auth"); + flb_es_conf_destroy(ctx); + return -1; } + flb_plg_debug(ctx->ins, "host=%s port=%i uri=%s index=%s type=%s", ins->host.name, ins->host.port, ctx->uri,
♻️ Duplicate comments (1)
tests/runtime/out_elasticsearch.c (1)
10-25
: This test doesn’t validate the Authorization header (duplicate of prior feedback)It only asserts the value of the
api_key
passed in viaout_callback_data
, which doesn’t prove the header was constructed or added. Consider exposing a small helper in the plugin to apply auth headers to a dummy HTTP client, then assert the presence/value ofAuthorization
by iteratingc->headers
in a response-test path.If you want, I can sketch a small static helper (e.g., es_apply_auth_headers(ctx, c)) and a response test that inspects the dummy client’s headers.
🧹 Nitpick comments (5)
plugins/out_es/es_conf.c (1)
146-147
: Remove unused local variable ‘http_api_key’ (triggers CI warnings)The local variable is never used and causes compiler warnings. The config_map already populates ctx->http_api_key, so this local isn’t needed.
Apply this diff to remove it:
- char *http_api_key = NULL;
tests/runtime/out_elasticsearch.c (2)
15-18
: Remove unused variable ‘expected_header’ to avoid warningsThe local
expected_header
is computed but never used.Apply this diff:
- char expected_header[256]; - - snprintf(expected_header, sizeof(expected_header), "ApiKey %s", api_key);
743-786
: Optional: Switch to an HTTP test that can inspect headersFormatter tests can’t observe HTTP headers. If you factor auth header creation into a helper callable from an HTTP test, you can then use
flb_output_set_http_test(..., "response", ...)
and assert the Authorization header value before/afterflb_http_do
.I can provide a patch to factor the header creation and a sample header-inspecting test if desired.
plugins/out_es/es.c (2)
827-829
: Avoid unused variable and initialize header SDSOnly one SDS is needed. Initialize it to NULL to simplify error handling.
Apply this diff:
- flb_sds_t header_buffer; - flb_sds_t header_line; + flb_sds_t header_line = NULL;
1130-1134
: Clarify http_api_key description (base64 expectation)Elasticsearch expects the API key credential in the Authorization header as “ApiKey <base64(id:key)>”. The description should tell users to supply the base64-encoded value.
Apply this diff:
- "API key for Elasticsearch" + "Base64-encoded API key credential for Elasticsearch (used as 'Authorization: ApiKey <value>')"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
plugins/out_es/es.c
(4 hunks)plugins/out_es/es.h
(1 hunks)plugins/out_es/es_conf.c
(1 hunks)tests/runtime/out_elasticsearch.c
(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/runtime/out_elasticsearch.c (2)
include/fluent-bit/flb_mem.h (1)
flb_free
(126-128)src/flb_lib.c (11)
flb_create
(138-220)flb_service_set
(647-673)flb_input
(261-271)flb_input_set
(300-330)flb_output
(274-284)flb_output_set
(515-546)flb_output_set_test
(579-610)flb_start
(914-925)flb_lib_push
(774-801)flb_stop
(942-985)flb_destroy
(223-258)
plugins/out_es/es.c (2)
src/flb_sds.c (3)
flb_sds_create_size
(92-95)flb_sds_printf
(336-387)flb_sds_destroy
(389-399)src/flb_http_client.c (1)
flb_http_add_header
(949-981)
🪛 GitHub Actions: Pull requests compile checks
plugins/out_es/es_conf.c
[warning] 146-146: Unused variable 'http_api_key' in es_conf_create.
plugins/out_es/es.c
[error] 910-910: Compilation error: 'goto error' references an undefined label 'error' at es.c:910 while building flb-plugin-out_es.
🪛 GitHub Actions: Run unit tests
plugins/out_es/es_conf.c
[warning] 146-146: plugins/out_es/es_conf.c:146:11: warning: ‘http_api_key’ may be used uninitialized in this function [-Wunused-variable]
plugins/out_es/es.c
[error] 910-910: es.c:910:17: error: label 'error' used but not defined
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: 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: pr-windows-build / call-build-windows-package (Windows 64bit, x64, x64-windows-static, 3.31.6)
- GitHub Check: PR - fuzzing test
🔇 Additional comments (3)
plugins/out_es/es.h (1)
54-58
: Add http_api_key field: looks correct and well placedThe new field is in the right section (HTTP Auth) and typed consistently with the other credentials.
tests/runtime/out_elasticsearch.c (1)
1078-1078
: Registering the http_api_key testRegistration is fine; once the test validates headers (per above), this entry can remain unchanged.
plugins/out_es/es.c (1)
32-34
: Includes for flb_log.h and flb_sds.h are appropriateBoth headers are required for logging and SDS usage in the new auth path.
Allow for using an API key as an authentication type to elastic. Signed-off-by: Todd Kennedy <[email protected]>
a18c5b9
to
d7dfdde
Compare
The architecture of the es plugin itself and how the http client is handled makes it extremely hard to validate the header in the way that the AI is requesting, without creating a lot of extra code. If this is something that we really want here, I'm happy to attempt it |
Allow for using an API key as an authentication type to elastic.
Enter
[N/A]
in the box, if an item is not applicable to your change.Testing
Before we can approve your change; please submit the following in a comment:
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
ok-package-test
label to test for all targets (requires maintainer to do).Documentation
fluent/fluent-bit-docs#1727
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