alias: output: network: handle plugin aliases#11656
Open
alias: output: network: handle plugin aliases#11656
Conversation
📝 WalkthroughWalkthroughAdds a plugin-aliasing subsystem: new public header and implementation for alias lookup and URI rewriting, integrates alias handling into output creation and network parsing, updates build/test files, and adds unit tests for alias lookup, rewrite, parsing, and instantiation. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant OutputLayer as Output Layer
participant AliasResolver as Alias Resolver
participant NetworkLayer as Network Layer
participant Plugin as Plugin
User->>OutputLayer: flb_output_new(config, "elasticsearch")
OutputLayer->>AliasResolver: flb_plugin_alias_rewrite(FLB_PLUGIN_OUTPUT, "elasticsearch://host:port")
AliasResolver->>AliasResolver: extract scheme/alias, lookup "elasticsearch"
AliasResolver-->>OutputLayer: "es://host:port" or FLB_PLUGIN_ALIAS_ERR or NULL
alt rewritten URI returned
OutputLayer->>NetworkLayer: flb_net_host_set("es://host:port")
else no rewrite/NULL
OutputLayer->>NetworkLayer: flb_net_host_set("elasticsearch://host:port")
else FLB_PLUGIN_ALIAS_ERR
OutputLayer-->>User: return error (cleanup and NULL)
end
NetworkLayer->>NetworkLayer: detect "://", parse host and port
NetworkLayer-->>OutputLayer: host/port populated
OutputLayer->>Plugin: instantiate plugin with canonical name "es"
Plugin-->>User: plugin instance returned
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
cosmo0920
force-pushed
the
comso0920-handle-plugin-aliases
branch
from
f3f7908 to
7394a1c
Compare
cosmo0920
force-pushed
the
comso0920-handle-plugin-aliases
branch
from
7394a1c to
4efcaf7
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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/fluent-bit/flb_plugin_alias.h`:
- Around line 25-27: flb_plugin_alias_rewrite currently returns NULL for both
“no rewrite needed” and allocation/build failures; change its API and
implementation so callers can distinguish errors: return NULL only for “no
rewrite”, return a distinct sentinel pointer (e.g. (char *)-1 or a named macro
like FLB_PLUGIN_ALIAS_ERR) to indicate allocation/build failure, and continue
returning a real allocated string on success; update callers (e.g. the code that
calls flb_plugin_alias_rewrite in flb_output.c) to check for the sentinel and
treat it as an error (fail cleanly) instead of falling back to the original
plugin_reference, and document the new return contract next to the
flb_plugin_alias_rewrite and flb_plugin_alias_get declarations.
In `@src/flb_network.c`:
- Around line 178-184: In flb_net_host_set make scheme detection strict: do not
use strstr(address, "://") which can match "://" anywhere; instead find the
first ':' with strchr(address, ':'), verify it is not at address (non-empty
scheme) and that the following two chars are '/' and '/' (i.e. separator[1]=='/'
&& separator[2]=='/'), and only then set s = separator + 3; otherwise fall back
to s = address + len. Update the code around variables separator, address, and s
in flb_net_host_set accordingly so only a proper leading "scheme://" is
accepted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5c181e7c-7d06-484f-933f-359991ea075c
📒 Files selected for processing (7)
include/fluent-bit/flb_plugin_alias.hsrc/CMakeLists.txtsrc/flb_network.csrc/flb_output.csrc/flb_plugin_alias.ctests/internal/CMakeLists.txttests/internal/plugin_alias.c
cosmo0920
force-pushed
the
comso0920-handle-plugin-aliases
branch
from
4efcaf7 to
dd91460
Compare
cosmo0920
force-pushed
the
comso0920-handle-plugin-aliases
branch
from
dd91460 to
e095350
Compare
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
cosmo0920
force-pushed
the
comso0920-handle-plugin-aliases
branch
from
e095350 to
e19cc04
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/flb_network.c (1)
178-186:⚠️ Potential issue | 🟠 MajorKeep
flb_net_host_set()strict about the expected plugin prefix.
flb_net_host_set("es", ..., "http://127.0.0.1:9200")now succeeds because any leadingscheme://is stripped, not justplugin_name://. The aliased output path already canonicalizeselasticsearch://...toes://..., so this shared parser should still reject unrelated schemes instead of silently accepting them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/flb_network.c` around lines 178 - 186, In flb_net_host_set ensure you only strip a leading "plugin_name://"-style scheme: when you find separator = strchr(address, ':'), compare the prefix (address..separator) against the provided plugin_name and only if they match and separator[1]=='/' && separator[2]=='/' set s = separator + 3; otherwise leave s = address + len (do not accept arbitrary schemes like "http://"). Use the existing variables separator, address, s and the plugin_name parameter to perform the exact-match check before removing the scheme.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/flb_output.c`:
- Around line 832-839: When flb_plugin_alias_rewrite returns
FLB_PLUGIN_ALIAS_ERR you must fully unwind the partially-initialized instance:
if instance->callback is set invoke the plugin teardown callback (using the same
callback signature used elsewhere) so the plugin can free its state, free
instance->context if non-NULL, destroy instance->singleplex_queue only if it was
created/non-NULL (and/or when instance->flags & FLB_OUTPUT_SYNCHRONOUS), then
free instance->http_server_config and finally free instance; implement this
sequence in the error branch that handles output_uri == FLB_PLUGIN_ALIAS_ERR to
avoid leaking plugin state.
---
Duplicate comments:
In `@src/flb_network.c`:
- Around line 178-186: In flb_net_host_set ensure you only strip a leading
"plugin_name://"-style scheme: when you find separator = strchr(address, ':'),
compare the prefix (address..separator) against the provided plugin_name and
only if they match and separator[1]=='/' && separator[2]=='/' set s = separator
+ 3; otherwise leave s = address + len (do not accept arbitrary schemes like
"http://"). Use the existing variables separator, address, s and the plugin_name
parameter to perform the exact-match check before removing the scheme.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1375d157-6cca-4999-97d8-8c65196e634a
📒 Files selected for processing (7)
include/fluent-bit/flb_plugin_alias.hsrc/CMakeLists.txtsrc/flb_network.csrc/flb_output.csrc/flb_plugin_alias.ctests/internal/CMakeLists.txttests/internal/plugin_alias.c
✅ Files skipped from review due to trivial changes (4)
- tests/internal/CMakeLists.txt
- src/CMakeLists.txt
- include/fluent-bit/flb_plugin_alias.h
- tests/internal/plugin_alias.c
Comment on lines
+832
to
+839
| output_uri = flb_plugin_alias_rewrite(FLB_PLUGIN_OUTPUT, output_name); | ||
| if (output_uri == FLB_PLUGIN_ALIAS_ERR) { | ||
| if (instance->flags & FLB_OUTPUT_SYNCHRONOUS) { | ||
| flb_task_queue_destroy(instance->singleplex_queue); | ||
| } | ||
| flb_free(instance->http_server_config); | ||
| flb_free(instance); | ||
| return NULL; |
There was a problem hiding this comment.
Fully unwind instance on the new alias-rewrite error path.
By this point instance->callback is already live and non-core plugins may also have allocated instance->context. Returning here only frees the outer structs, so FLB_PLUGIN_ALIAS_ERR leaks initialized state; singleplex_queue also has not been created yet on this path.
💡 Suggested cleanup
output_uri = flb_plugin_alias_rewrite(FLB_PLUGIN_OUTPUT, output_name);
if (output_uri == FLB_PLUGIN_ALIAS_ERR) {
- if (instance->flags & FLB_OUTPUT_SYNCHRONOUS) {
+ if ((instance->flags & FLB_OUTPUT_SYNCHRONOUS) &&
+ instance->singleplex_queue != NULL) {
flb_task_queue_destroy(instance->singleplex_queue);
}
+ if (instance->callback != NULL) {
+ flb_callback_destroy(instance->callback);
+ }
+ if (plugin->type != FLB_OUTPUT_PLUGIN_CORE &&
+ instance->context != NULL) {
+ flb_free(instance->context);
+ }
flb_free(instance->http_server_config);
flb_free(instance);
return NULL;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/flb_output.c` around lines 832 - 839, When flb_plugin_alias_rewrite
returns FLB_PLUGIN_ALIAS_ERR you must fully unwind the partially-initialized
instance: if instance->callback is set invoke the plugin teardown callback
(using the same callback signature used elsewhere) so the plugin can free its
state, free instance->context if non-NULL, destroy instance->singleplex_queue
only if it was created/non-NULL (and/or when instance->flags &
FLB_OUTPUT_SYNCHRONOUS), then free instance->http_server_config and finally free
instance; implement this sequence in the error branch that handles output_uri ==
FLB_PLUGIN_ALIAS_ERR to avoid leaking plugin state.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We can instantiate out_es as output elasticsearch plugin but the input of elasticsearch plugin is in_elasticsearch.
To maintain naming coherence, we need to handle plugin alias at least of out_es as out_elasticsearch.
With this patch, we can instantiate out_elasticsearch as out_es.
Closes #9261.
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-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
Improvements
Tests