-
Notifications
You must be signed in to change notification settings - Fork 1.8k
out_opentelemetry: add AWS authentication support #10776
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?
Conversation
Signed-off-by: Eduardo Silva <[email protected]>
WalkthroughAdds optional AWS SigV4 request signing to the OpenTelemetry output plugin, gated by FLB_HAVE_SIGNV4 and FLB_HAVE_AWS. Introduces config options and context fields, initializes/destroys an AWS credential provider, and signs requests in both legacy HTTP POST and HTTP/2/gRPC paths with retry/error handling and cleanup. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Cfg as Config
participant Ctx as OpenTelemetry Context
participant AWS as AWS Credential Provider
Note over Cfg,Ctx: Initialization (feature-gated)
Cfg->>Ctx: Parse aws_auth, aws_service, aws_region
alt aws_auth enabled
Ctx->>AWS: Create managed chain provider (prefix "aws_")
AWS-->>Ctx: Provider handle / credentials on demand
else
Note right of Ctx: No AWS provider
end
sequenceDiagram
autonumber
participant OT as OpenTelemetry Output
participant Sig as SigV4 Signer
participant S as Server
Note over OT,S: Legacy HTTP POST path
OT->>OT: Build HTTP request
alt aws_auth enabled
OT->>Sig: Sign request (V4) with region/service
alt signing fails
Sig-->>OT: Error
OT->>OT: Log error, cleanup
OT-->>OT: Return FLB_RETRY
else signing ok
Sig-->>OT: Signature applied
end
end
OT->>S: Send HTTP request
S-->>OT: Response
OT->>OT: Cleanup resources
sequenceDiagram
autonumber
participant OT as OpenTelemetry Output (HTTP/2/gRPC)
participant Sig as SigV4 Signer
participant S as Endpoint
OT->>OT: Prepare HTTP/2/gRPC request
alt aws_auth enabled
OT->>Sig: Perform SigV4 signing
alt signing fails
Sig-->>OT: Error
OT-->>OT: Cleanup and FLB_RETRY
else
Sig-->>OT: Signed request
end
end
OT->>S: Execute request
S-->>OT: Response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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. ✨ 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
plugins/out_opentelemetry/opentelemetry.c (2)
426-443
: HTTP/2 path: also gate Basic Auth when aws_auth is enabledSame conflict here: Basic adds Authorization, and SigV4 will add its own Authorization header. Gate Basic in HTTP/2 path as well.
Apply this diff:
- if (ctx->http_user != NULL && - ctx->http_passwd != NULL) { - result = flb_http_request_set_parameters(request, - FLB_HTTP_CLIENT_ARGUMENT_BASIC_AUTHORIZATION( - ctx->http_user, - ctx->http_passwd)); - if (result != 0) { - flb_plg_error(ctx->ins, "error setting http authorization data"); - - return FLB_RETRY; - } - - flb_http_request_set_authorization(request, - HTTP_WWW_AUTHORIZATION_SCHEME_BASIC, - ctx->http_user, - ctx->http_passwd); - } + if (ctx->has_aws_auth != FLB_TRUE && + ctx->http_user != NULL && + ctx->http_passwd != NULL) { + result = flb_http_request_set_parameters(request, + FLB_HTTP_CLIENT_ARGUMENT_BASIC_AUTHORIZATION( + ctx->http_user, + ctx->http_passwd)); + if (result != 0) { + flb_plg_error(ctx->ins, "error setting http authorization data"); + return FLB_RETRY; + } + flb_http_request_set_authorization(request, + HTTP_WWW_AUTHORIZATION_SCHEME_BASIC, + ctx->http_user, + ctx->http_passwd); + } + else if (ctx->has_aws_auth == FLB_TRUE && + ctx->http_user != NULL && + ctx->http_passwd != NULL) { + flb_plg_warn(ctx->ins, "aws_auth enabled; ignoring http_user/http_passwd Basic auth"); + }
175-179
: Avoid Basic Auth when aws_auth is enabled (conflicting Authorization headers)We’ve identified two code paths in
plugins/out_opentelemetry/opentelemetry.c
that unconditionally add Basic-Auth headers. Both must be gated behindctx->has_aws_auth
to prevent duplicateAuthorization
headers when SigV4 is in use:• Lines 175–179 (uses
flb_http_basic_auth
)
• Lines 426–431 (usesFLB_HTTP_CLIENT_ARGUMENT_BASIC_AUTHORIZATION
)Proposed fixes:
--- a/plugins/out_opentelemetry/opentelemetry.c +++ b/plugins/out_opentelemetry/opentelemetry.c @@ -172,9 +172,17 @@ /* Basic Auth headers */ - if (ctx->http_user != NULL && - ctx->http_passwd != NULL) { - flb_http_basic_auth(c, ctx->http_user, ctx->http_passwd); - } + if (ctx->has_aws_auth != FLB_TRUE && + ctx->http_user != NULL && + ctx->http_passwd != NULL) { + flb_http_basic_auth(c, ctx->http_user, ctx->http_passwd); + } + else if (ctx->has_aws_auth == FLB_TRUE && + ctx->http_user != NULL && + ctx->http_passwd != NULL) { + flb_plg_warn(ctx->ins, + "aws_auth enabled; ignoring http_user/http_passwd Basic auth"); + }--- a/plugins/out_opentelemetry/opentelemetry.c +++ b/plugins/out_opentelemetry/opentelemetry.c @@ -423,11 +423,19 @@ /* set basic-auth via HTTP client args */ - if (ctx->http_user != NULL && - ctx->http_passwd != NULL) { - result = flb_http_request_set_parameters(request, - FLB_HTTP_CLIENT_ARGUMENT_BASIC_AUTHORIZATION( - ctx->http_user, - ctx->http_passwd)); - if (result != 0) { + if (ctx->has_aws_auth != FLB_TRUE && + ctx->http_user != NULL && + ctx->http_passwd != NULL) { + result = flb_http_request_set_parameters(request, + FLB_HTTP_CLIENT_ARGUMENT_BASIC_AUTHORIZATION( + ctx->http_user, + ctx->http_passwd)); + if (result != 0) { flb_plg_error(ctx->ins, "failed to add Basic authorization header"); status = FLB_FALSE; - } + } + } + else if (ctx->has_aws_auth == FLB_TRUE && + ctx->http_user != NULL && + ctx->http_passwd != NULL) { + flb_plg_warn(ctx->ins, + "aws_auth enabled; ignoring http_user/http_passwd Basic auth"); + }Please apply these changes to both spots to ensure Basic-Auth is never added when SigV4 (
aws_auth
) is active.
🧹 Nitpick comments (2)
plugins/out_opentelemetry/opentelemetry.h (1)
61-70
: AWS fields added to context look consistent; minor nit on boolean typeThe new fields map cleanly to the config and usage sites. Optional: consider using flb_bool for has_aws_auth for consistency with other boolean flags, but not a blocker.
plugins/out_opentelemetry/opentelemetry_conf.c (1)
307-334
: Emit a warning when Basic Auth is set alongside aws_authIn plugins/out_opentelemetry/opentelemetry_conf.c (around lines 307–334), users who set
http_user
/http_passwd
will silently have their Basic credentials ignored onceaws_auth
is enabled. For better UX, surface a one-time warning immediately when both mechanisms are present:Apply this diff inside the
if (ctx->has_aws_auth)
block, before the service-check:#ifdef FLB_HAVE_SIGNV4 #ifdef FLB_HAVE_AWS if (ctx->has_aws_auth) { + if (ctx->http_user && ctx->http_passwd) { + flb_plg_warn(ins, + "aws_auth enabled; ignoring http_user/http_passwd Basic auth"); + } if (!ctx->aws_service) { flb_plg_error(ins, "aws_auth option requires " FLB_OPENTELEMETRY_AWS_CREDENTIAL_PREFIX "service to be set"); flb_opentelemetry_context_destroy(ctx); return NULL; }This adds no control-flow change but helps users discover that Basic Auth is dropped under SigV4 auth.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
plugins/out_opentelemetry/opentelemetry.c
(6 hunks)plugins/out_opentelemetry/opentelemetry.h
(2 hunks)plugins/out_opentelemetry/opentelemetry_conf.c
(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
plugins/out_opentelemetry/opentelemetry_conf.c (3)
src/aws/flb_aws_credentials.c (2)
flb_managed_chain_provider_create
(329-518)flb_aws_provider_destroy
(769-791)src/aws/flb_aws_util.c (1)
flb_aws_client_generator
(280-283)src/flb_output.c (1)
flb_output_get_property
(1096-1099)
plugins/out_opentelemetry/opentelemetry.c (4)
src/flb_signv4.c (1)
flb_signv4_do
(1145-1248)src/flb_sds.c (1)
flb_sds_destroy
(389-399)src/flb_http_common.c (1)
flb_http_request_perform_signv4_signature
(784-816)src/flb_http_client.c (1)
flb_http_client_request_destroy
(2139-2149)
🔇 Additional comments (9)
plugins/out_opentelemetry/opentelemetry.h (1)
27-32
: Correct conditional include and credential prefix macro placementIncluding AWS credentials only when FLB_HAVE_SIGNV4 and FLB_HAVE_AWS are defined is correct. Defining FLB_OPENTELEMETRY_AWS_CREDENTIAL_PREFIX here centralizes the prefix used across files and avoids string duplication. LGTM.
plugins/out_opentelemetry/opentelemetry_conf.c (2)
26-30
: Scoped include for AWS creds header is correctHeader inclusion is properly gated by the feature flags. No issues.
798-805
: Proper managed cleanup of AWS providerDestroying ctx->aws_provider in context_destroy matches the managed creation path and avoids TLS/provider leaks. LGTM.
plugins/out_opentelemetry/opentelemetry.c (6)
44-48
: Correct conditional include for SigV4The include of flb_signv4.h is properly gated and keeps non-AWS builds unaffected.
95-96
: Signature buffer lifecycle is correctDeclaring signature and destroying it on success, with a single cleanup path on failure, avoids leaks. LGTM.
192-215
: SigV4 signing in legacy path is correct; unified cleanup is goodSigning after headers/body are set is correct. The cleanup label ensures consistent release on failure. Once Basic-Auth gating is applied (see prior comment), this block should work reliably across targets.
289-301
: Unified cleanup path prevents leaks on early exitsFreeing compressed body, destroying the client, and releasing the upstream in a single path reduces error-handling duplication. LGTM.
445-460
: SigV4 signing on HTTP/2/gRPC requests is correctly placedSigning after request construction ensures headers and payload are considered by canonicalization. Error handling returns FLB_RETRY, which is appropriate for transient credential or signing issues. LGTM, assuming Basic gating is added.
926-941
: Config map wiring for AWS options looks correct; ensure docs/examples reflect usageaws_auth and aws_service are correctly mapped, and the credential base map exposes aws_region, aws_role_arn, aws_external_id, aws_sts_endpoint, and aws_profile with the "aws_" prefix. Please update the plugin docs and add an example showing a typical config for logs/metrics/traces, since default service "logs" may not fit every endpoint.
If you want, I can draft a docs snippet with example configurations for CloudWatch Logs (service=logs), X-Ray (service=xray), and AMP (service=aps), including role assumption via aws_role_arn.
WIP: AWS auth support for OpenTelemetry output
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