Conversation
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds OAuth2 and OAuth2-JWT support to OpenTelemetry plugins: input can validate incoming JWTs when enabled (requires issuer and JWKS URL); output can obtain, attach, and manage OAuth2 access tokens for outbound HTTP/gRPC requests, invalidating tokens on 401 or grpc-status=16 and retrying as needed. Changes
Sequence DiagramssequenceDiagram
participant Client
participant OTel_Input as OTel_Input
participant JWKS as JWKS_Service
participant Backend
Client->>OTel_Input: HTTP/gRPC request + Authorization header
OTel_Input->>JWKS: Validate JWT (issuer, jwks_url)
alt JWT valid
JWKS-->>OTel_Input: Valid
OTel_Input->>Backend: Forward request
Backend-->>Client: 200 OK
else JWT invalid
JWKS-->>OTel_Input: Invalid
OTel_Input-->>Client: 401 Unauthorized (gRPC or HTTP)
end
sequenceDiagram
participant OTel_Output as OTel_Output
participant OAuth2Svc as OAuth2_Client
participant Remote as Remote_Endpoint
OTel_Output->>OTel_Output: Check oauth2_config.enabled
alt OAuth2 enabled
OTel_Output->>OAuth2Svc: Acquire access token (token_url, client creds)
OAuth2Svc-->>OTel_Output: Access token
OTel_Output->>Remote: HTTP/gRPC request + Bearer token
alt 401 or grpc-status=16
Remote-->>OTel_Output: Unauthorized
OTel_Output->>OAuth2Svc: Invalidate token
OTel_Output->>OAuth2Svc: Re-acquire token / Retry
else Success
Remote-->>OTel_Output: 200 OK
end
else OAuth2 disabled
OTel_Output->>Remote: HTTP request + Basic Auth
Remote-->>OTel_Output: Response
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c0e2a7d2a
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (ctx->oauth2_ctx != NULL && response->status == 401) { | ||
| flb_oauth2_invalidate_token(ctx->oauth2_ctx); |
There was a problem hiding this comment.
When OAuth2 is enabled on the HTTP/2 code path, a 401 only triggers flb_oauth2_invalidate_token() and then falls through to the existing status handling, which treats 401 as non-retryable and returns FLB_ERROR; this drops the current chunk instead of retrying with a refreshed token. This regression affects OAuth2 users whenever the collector rejects an expired/revoked token on first use, so the request should be retried once (or at least returned as FLB_RETRY) after invalidation.
Useful? React with 👍 / 👎.
| if (ctx->oauth2_cfg.issuer) { | ||
| flb_sds_destroy(ctx->oauth2_cfg.issuer); | ||
| } |
There was a problem hiding this comment.
The teardown path frees ctx->oauth2_cfg string pointers when oauth2_ctx is NULL, but these fields are populated via config-map property binding and are still owned by ins->oauth2_jwt_properties; they are released again later by input-instance cleanup. In configurations where validation is disabled (or init fails before context creation), this introduces a double-free/use-after-free risk during shutdown.
Useful? React with 👍 / 👎.
![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
🧹 Nitpick comments (1)
plugins/out_opentelemetry/opentelemetry.h (1)
63-65: Useconst char *for the borrowedoauth2_auth_method.
plugins/out_opentelemetry/opentelemetry_conf.cstores the rawflb_kv_get_key_value()result in this field without cloning it, soflb_sds_toverstates the ownership and mutability here.♻️ Proposed fix
- flb_sds_t oauth2_auth_method; + const char *oauth2_auth_method;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/out_opentelemetry/opentelemetry.h` around lines 63 - 65, The oauth2_auth_method field currently declared as flb_sds_t in struct flb_oauth2_config/oauth2_ctx should be changed to const char * because opentelemetry_conf.c stores the borrowed pointer returned by flb_kv_get_key_value() without cloning; update the declaration of oauth2_auth_method in plugins/out_opentelemetry/opentelemetry.h to const char *oauth2_auth_method and then adjust any code that assumed ownership/mutability (e.g., remove any flb_sds_free or mutating calls and treat it as a borrowed C string wherever oauth2_auth_method is read, and ensure opentelemetry_conf.c assigns the flb_kv_get_key_value() result directly to this field).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/in_opentelemetry/opentelemetry_config.c`:
- Around line 72-91: The code currently destroys config-backed JWT strings
(ctx->oauth2_cfg.issuer, jwks_url, allowed_audience) only when ctx->oauth2_ctx
is NULL, which causes mismatched lifetimes and potential double-free; remove the
flb_sds_destroy calls in the else branch and stop freeing or resetting those
config-backed fields here so they remain managed by the input config teardown,
leaving the flb_oauth2_jwt_context_destroy call and any NULL assignments only
for oauth2_ctx-managed resources (refer to ctx->oauth2_ctx,
flb_oauth2_jwt_context_destroy, and ctx->oauth2_cfg.*).
In `@plugins/out_opentelemetry/opentelemetry.c`:
- Around line 619-621: The current code only invalidates the OAuth2 cache on
HTTP 401; extend this so when opentelemetry_check_grpc_status(...) returns GRPC
status 16 (UNAUTHENTICATED) you also call flb_oauth2_invalidate_token on
ctx->oauth2_ctx. Locate the place where
opentelemetry_check_grpc_status(response, ...) is used, and add a null-checked
call to flb_oauth2_invalidate_token(ctx->oauth2_ctx) when the function indicates
status 16, mirroring the existing invalidation logic that checks
response->status == 401.
---
Nitpick comments:
In `@plugins/out_opentelemetry/opentelemetry.h`:
- Around line 63-65: The oauth2_auth_method field currently declared as
flb_sds_t in struct flb_oauth2_config/oauth2_ctx should be changed to const char
* because opentelemetry_conf.c stores the borrowed pointer returned by
flb_kv_get_key_value() without cloning; update the declaration of
oauth2_auth_method in plugins/out_opentelemetry/opentelemetry.h to const char
*oauth2_auth_method and then adjust any code that assumed ownership/mutability
(e.g., remove any flb_sds_free or mutating calls and treat it as a borrowed C
string wherever oauth2_auth_method is read, and ensure opentelemetry_conf.c
assigns the flb_kv_get_key_value() result directly to this field).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e4ecce43-7305-44b0-84ba-f34fc4d87321
📒 Files selected for processing (7)
plugins/in_opentelemetry/opentelemetry.cplugins/in_opentelemetry/opentelemetry.hplugins/in_opentelemetry/opentelemetry_config.cplugins/in_opentelemetry/opentelemetry_prot.cplugins/out_opentelemetry/opentelemetry.cplugins/out_opentelemetry/opentelemetry.hplugins/out_opentelemetry/opentelemetry_conf.c
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/out_opentelemetry/opentelemetry.c`:
- Around line 170-172: The current logic invalidates the cached token
(flb_oauth2_invalidate_token(ctx->oauth2_ctx)) when grpc_status == 16 or HTTP
401 but then treats the response as non-retryable and returns FLB_ERROR; change
the control flow so that after invalidating ctx->oauth2_ctx you return a retry
signal (e.g., FLB_RETRY) so the chunk is resent with a fresh token instead of
being dropped. Locate the two spots referencing grpc_status == 16 and the HTTP
401 branch (the calls to flb_oauth2_invalidate_token) and replace the final
non-retry return with a retry return when ctx->oauth2_ctx was present and
invalidated. Ensure you keep the invalidation call and only adjust the return
value/path to trigger a retry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cd33a1dc-1ec4-4820-8bc4-c3f278ca3ef7
📒 Files selected for processing (4)
plugins/in_opentelemetry/opentelemetry_config.cplugins/out_opentelemetry/opentelemetry.cplugins/out_opentelemetry/opentelemetry.hplugins/out_opentelemetry/opentelemetry_conf.c
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/out_opentelemetry/opentelemetry.h
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/out_opentelemetry/opentelemetry_conf.c`:
- Around line 294-296: The early-return on oauth2 config-map parse failure
currently calls flb_free(ctx) and leaks ctx->oauth2_config; replace that direct
free with the structured teardown used elsewhere by calling
flb_opentelemetry_context_destroy(ctx) instead (the failure branch after
flb_config_map_set()/ret == -1 should invoke flb_opentelemetry_context_destroy
to free ctx->oauth2_config and other subfields before returning NULL).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 462e0bd0-765e-40d4-81a9-3c2f052aea43
📒 Files selected for processing (2)
plugins/out_opentelemetry/opentelemetry.cplugins/out_opentelemetry/opentelemetry_conf.c
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/out_opentelemetry/opentelemetry.c
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
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 `@plugins/out_opentelemetry/opentelemetry_conf.c`:
- Around line 457-463: After successfully creating ctx->oauth2_ctx via
flb_oauth2_create_from_config, ensure any subsequent early returns (e.g., the
invalid-compression branch that currently returns directly) properly clean up
the OAuth2/upstream resources by calling flb_opentelemetry_context_destroy (or
the specific oauth2 cleanup routine) before returning; locate the
invalid-compression return and add a call to
flb_opentelemetry_context_destroy(ctx) (or ctx->oauth2_ctx destroy) so
ctx->oauth2_ctx and related resources are freed on all error paths.
- Around line 290-303: The oauth2 parsing path can allocate resources into
ctx->oauth2_config but later code frees ctx raw without running structured
cleanup, leaking memory; fix this by ensuring the upstream initialization
failure path invokes the structured teardown (call
flb_opentelemetry_context_destroy(ctx) or an equivalent cleanup that frees
ctx->oauth2_config) instead of doing a raw free, or explicitly release all
allocations inside ctx->oauth2_config before freeing ctx so that resources set
by flb_config_map_set are properly cleaned up.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8aeeb811-fb4a-409b-bcf9-dca222f2e7f4
📒 Files selected for processing (1)
plugins/out_opentelemetry/opentelemetry_conf.c
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
1 participant
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