plugin_proxy: add config map to the plugin registration - #12058
Conversation
Signed-off-by: Ilia Petrov <ilia.yavorov.petrov@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds ChangesProxy Config Map Propagation
Estimated code review effort: 1 (Trivial) | ~5 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a2aacd87b2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| char *name; /* plugin short name */ | ||
| char *description; /* plugin description */ | ||
| int event_type; /* event type (logs/metrics/traces) */ | ||
| struct flb_config_map *config_map; /* plugin's configuration */ |
There was a problem hiding this comment.
Propagate proxy config maps to custom plugins
When a Go proxy registers as FLB_PROXY_CUSTOM_PLUGIN and sets this new field, it is dropped because flb_proxy_register_custom() only copies flags/name/description into struct flb_custom_plugin. The custom path does have config-map validation (flb_custom_plugin_property_check() gates on p->config_map), so those plugins will silently accept unknown properties and won't get defaults/dynamic-env handling even though input/output proxy plugins do.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@iypetrov We need to apply the same operation which is like copying config_map on the proxy of custom plugins. So, could you add copying operation line into flb_proxy_register_custom() as well?
|
@cosmo0920 when you have time, can you rerun the failing tests? |
|
@edsiper this PR implements the suggestion from #11788 (comment) - when this one got merged, I will close the PR with previous implementation in fluent-bit repo and will start working on the required change in fluent-bit-go. |
|
@cosmo0920 @edsiper this PR is related to the discussion we had here. |
cosmo0920
left a comment
There was a problem hiding this comment.
Mostly, I like this idea more than the previous one. One thing, we need to align the registration in each of plugin types. So, could you add the similar diffs in custom plugins' proxy?
| char *name; /* plugin short name */ | ||
| char *description; /* plugin description */ | ||
| int event_type; /* event type (logs/metrics/traces) */ | ||
| struct flb_config_map *config_map; /* plugin's configuration */ |
There was a problem hiding this comment.
@iypetrov We need to apply the same operation which is like copying config_map on the proxy of custom plugins. So, could you add copying operation line into flb_proxy_register_custom() as well?
Thanks for the feedback, I will make the adjustments. |
Signed-off-by: Ilia Petrov <ilia.yavorov.petrov@gmail.com>
|
@cosmo0920 when you have time can you rerun the failing test, I think it is a flake |
Extend the proxy plugin registration flow so input and output Go plugins can attach a
flb_config_mapschema, enabling the same property validation native plugins already receive. Custom plugins are excluded because they don't go through the property-check callback path. For more context can check this discussion.Issue #11776
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
Summary by CodeRabbit