out_azure_blob: add service principal auth#11889
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds service-principal OAuth2 auth and thread‑safe token handling, integrates Authorization header into HTTP client setup, and adds explicit error handling for azb_http_client_setup() failures across Azure Blob request helpers. ChangesService Principal OAuth2 Authentication
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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 unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)plugins/out_azure_blob/azure_blob.cplugins/out_azure_blob/azure_blob.c:20:10: fatal error: 'fluent-bit/flb_output_plugin.h' file not found ... [truncated 778 characters] ... x86_64-v1.2.0/lib/infer/facebook-clang-plugins/clang/install/lib/clang/18/include" 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: 68ab3396b7
ℹ️ 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".
|
|
||
| ret = flb_oauth2_payload_append(ctx->o, | ||
| "client_secret", 13, | ||
| ctx->client_secret, -1); |
There was a problem hiding this comment.
When an Azure service principal secret contains form-reserved characters such as +, %, or &, this sends the raw secret in an application/x-www-form-urlencoded OAuth2 body. flb_oauth2_payload_append() only concatenates key=value pairs (unlike the config-based OAuth2 builder in src/flb_oauth2.c, which URI-encodes values), so Azure AD will parse a different client_secret and reject token acquisition for valid generated secrets. Percent-encode these values or use the OAuth2 config path that encodes them.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Thanks for the catch,
azure client secret document mentions client secret is a combination of only
- a-z (not case-sensitive)
- 0-9
- dashes (-)
- underlines (_)
- dots (.)
- or tilde accents (~)
hence it would not involve +, %, or &
the documentation link is provided below.
https://learn.microsoft.com/en-us/purview/sit-defn-azure-ad-client-secret
Moreover, the existing implementation matches that of azure kusto implementation.
If changes are further required and we have to use config-based OAuth2 builder , we can make those changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/out_azure_blob/azure_blob_conf.c`:
- Around line 620-621: The remote config is applied before the auth_type is
parsed so flb_azure_blob_apply_remote_configuration(ctx) sees ctx->atype == 0
and mis-handles service_principal; move the auth_type parsing (the strcasecmp
branch that sets ctx->atype = AZURE_BLOB_AUTH_SERVICE_PRINCIPAL) to run before
calling flb_azure_blob_apply_remote_configuration(ctx), or alternatively modify
flb_azure_blob_apply_remote_configuration to accept the raw auth_type property
and use that when deciding branches; update the code paths that reference
ctx->atype (and any callers expecting the parsed value) to rely on the parsed
value being set first.
- Around line 646-659: The service-principal validation only checks for NULLs;
update the AZURE_BLOB_AUTH_SERVICE_PRINCIPAL branch to also reject empty strings
by using flb_sds_len(...) == 0 for ctx->tenant_id, ctx->client_id, and
ctx->client_secret (in addition to the existing NULL checks), and call
flb_plg_error with the same clear messages when any of those values are empty so
startup fails fast with a helpful error; reference ctx, ctx->atype,
AZURE_BLOB_AUTH_SERVICE_PRINCIPAL, tenant_id, client_id, client_secret, and
flb_sds_len in your changes.
In `@plugins/out_azure_blob/azure_blob_http.c`:
- Around line 485-490: The call sites that invoke azb_http_client_setup (and the
branch that now returns non-zero from azb_get_oauth2_authorization_header) must
check the function return value and abort the operation on any non-zero result;
update callers in azure_blob.c where azb_http_client_setup is used to test its
return and immediately return/propagate an error (do not continue the request
without Authorization). Locate azb_http_client_setup and the code paths that
handle AZURE_BLOB_AUTH_SERVICE_PRINCIPAL (and any callers that previously
ignored the result) and change them to bail out on non-zero return codes so
token refresh failures are surfaced to the caller.
In `@plugins/out_azure_blob/azure_blob.c`:
- Around line 1420-1446: The OAuth2 service-principal init paths leak resources
and skip full teardown; ensure every early exit funnels to the central cleanup
(call flb_azure_blob_conf_destroy(ctx)) and also properly destroys the mutex
created by pthread_mutex_init(&ctx->token_mutex, NULL) and frees token_url
(flb_sds_destroy) before cleanup: on token_url allocation failure, on
flb_sds_snprintf failure, and when flb_oauth2_create returns NULL, release
token_url if allocated, call pthread_mutex_destroy(&ctx->token_mutex) if
initialized, then call flb_azure_blob_conf_destroy(ctx) and return error; keep
the successful path unchanged.
🪄 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: 0fe197e1-4afd-4e21-b415-515a8e343144
📒 Files selected for processing (4)
plugins/out_azure_blob/azure_blob.cplugins/out_azure_blob/azure_blob.hplugins/out_azure_blob/azure_blob_conf.cplugins/out_azure_blob/azure_blob_http.c
Signed-off-by: Cheruvu.Sahithee.Vaibhav@ibm.com <Cheruvu.Sahithee.Vaibhav@ibm.com>
Vaibhav-C-S
force-pushed
the
out-azure-blob-service-principal
branch
from
68ab339 to
0eba4c2
Compare
Signed-off-by: Cheruvu.Sahithee.Vaibhav@ibm.com <Cheruvu.Sahithee.Vaibhav@ibm.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
plugins/out_azure_blob/azure_blob_blockblob.c (1)
323-329:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRelease the client and connection before retrying on send errors.
The
ret == -1branch returns withoutflb_http_client_destroy(c)orflb_upstream_conn_release(u_conn). In retry paths, that leaks both objects.Suggested fix
if (ret == -1) { flb_plg_error(ctx->ins, "error sending block_blob"); + flb_http_client_destroy(c); + flb_upstream_conn_release(u_conn); return FLB_RETRY; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/out_azure_blob/azure_blob_blockblob.c` around lines 323 - 329, The error path after flb_http_do(c, &b_sent) currently returns FLB_RETRY without releasing resources; modify the ret == -1 branch to first call flb_http_client_destroy(c) and flb_upstream_conn_release(u_conn) (or the appropriate destroy/release helpers used elsewhere in this file) before logging and returning FLB_RETRY so the HTTP client (c) and upstream connection (u_conn) are always cleaned up on send errors.plugins/out_azure_blob/azure_blob.c (2)
1365-1372:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDestroy the HTTP client on container-check send failures.
The
ret == -1branch releasesu_connbut never destroysc. Sinceensure_container()can run on every flush, repeated transport failures will leak clients.Suggested fix
if (ret == -1) { flb_plg_error(ctx->ins, "error requesting container properties"); + flb_http_client_destroy(c); flb_upstream_conn_release(u_conn); return FLB_FALSE; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/out_azure_blob/azure_blob.c` around lines 1365 - 1372, The error-handling path after flb_http_do(c, &b_sent) leaks the HTTP client `c` on transport failures; in the `ret == -1` branch (inside ensure_container()/container-check), call the HTTP client destructor (flb_http_client_destroy(c)) before releasing the upstream connection `u_conn` (flb_upstream_conn_release(u_conn)) and returning FLB_FALSE so the client is not repeatedly leaked on flush failures.
1454-1455:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCheck pthread_mutex_init() return code and fix ensure_container() client cleanup on flb_http_do() failure
cb_azure_blob_init()doesn’t checkpthread_mutex_init(&ctx->token_mutex, NULL);—if it fails, later code may use an uninitialized mutex; fail fast and reuse existing teardown.Suggested fix
if (ctx->atype == AZURE_BLOB_AUTH_SERVICE_PRINCIPAL) { - pthread_mutex_init(&ctx->token_mutex, NULL); + if (pthread_mutex_init(&ctx->token_mutex, NULL) != 0) { + flb_plg_error(ctx->ins, "failed to initialize token mutex"); + flb_azure_blob_conf_destroy(ctx); + return -1; + } flb_sds_t token_url; ``` </details>
ensure_container()returnsFLB_FALSEonflb_http_do(c, &b_sent)transport failure without callingflb_http_client_destroy(c), leaking the HTTP client; destroycbefore returning.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/out_azure_blob/azure_blob.c` around lines 1454 - 1455, In cb_azure_blob_init(), check the return value of pthread_mutex_init(&ctx->token_mutex, NULL) and on failure clean up any partially-initialized resources and return an error (match existing teardown path) so the uninitialized ctx->token_mutex is never used; in ensure_container(), if flb_http_do(c, &b_sent) fails, call flb_http_client_destroy(c) before returning FLB_FALSE to avoid leaking the HTTP client—ensure the cleanup path covers both success and failure branches and reference the local HTTP client variable c and ctx->token_mutex when applying these fixes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/out_azure_blob/azure_blob_blockblob.c`:
- Around line 313-320: azb_block_blob_put_block_list() is now returning
FLB_RETRY on HTTP setup failure which cb_azb_blob_file_upload() does not treat
as an error, leading to transient failures being treated as successful commits;
either make azb_block_blob_put_block_list() return the existing failure sentinel
(-1) for unrecoverable/failed commit paths (including when azb_http_client_setup
fails) or update cb_azb_blob_file_upload() to treat FLB_RETRY from
azb_block_blob_commit_file_parts()/azb_block_blob_put_block_list() as a failure
(same handling as -1) so it does not log success or delete the buffered file on
transient setup/auth errors. Ensure the change references
azb_block_blob_put_block_list, azb_block_blob_commit_file_parts, and
cb_azb_blob_file_upload so callers consistently interpret FLB_RETRY as an error
condition.
---
Outside diff comments:
In `@plugins/out_azure_blob/azure_blob_blockblob.c`:
- Around line 323-329: The error path after flb_http_do(c, &b_sent) currently
returns FLB_RETRY without releasing resources; modify the ret == -1 branch to
first call flb_http_client_destroy(c) and flb_upstream_conn_release(u_conn) (or
the appropriate destroy/release helpers used elsewhere in this file) before
logging and returning FLB_RETRY so the HTTP client (c) and upstream connection
(u_conn) are always cleaned up on send errors.
In `@plugins/out_azure_blob/azure_blob.c`:
- Around line 1365-1372: The error-handling path after flb_http_do(c, &b_sent)
leaks the HTTP client `c` on transport failures; in the `ret == -1` branch
(inside ensure_container()/container-check), call the HTTP client destructor
(flb_http_client_destroy(c)) before releasing the upstream connection `u_conn`
(flb_upstream_conn_release(u_conn)) and returning FLB_FALSE so the client is not
repeatedly leaked on flush failures.
- Around line 1454-1455: In cb_azure_blob_init(), check the return value of
pthread_mutex_init(&ctx->token_mutex, NULL) and on failure clean up any
partially-initialized resources and return an error (match existing teardown
path) so the uninitialized ctx->token_mutex is never used; in
ensure_container(), if flb_http_do(c, &b_sent) fails, call
flb_http_client_destroy(c) before returning FLB_FALSE to avoid leaking the HTTP
client—ensure the cleanup path covers both success and failure branches and
reference the local HTTP client variable c and ctx->token_mutex when applying
these fixes.
🪄 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: 33091bc1-d351-4b3f-aaad-4a9d05e4f0e5
📒 Files selected for processing (3)
plugins/out_azure_blob/azure_blob.cplugins/out_azure_blob/azure_blob_blockblob.cplugins/out_azure_blob/azure_blob_conf.c
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/out_azure_blob/azure_blob_conf.c
Signed-off-by: Cheruvu.Sahithee.Vaibhav@ibm.com <Cheruvu.Sahithee.Vaibhav@ibm.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
plugins/out_azure_blob/azure_blob.c (2)
1244-1252:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFree
urionflb_http_client()failures.Line 1248 and Line 1347 return after
flb_http_client()fails, but both helpers have already allocateduri. Under repeated setup failures that leaks one SDS per attempt.Suggested fix
c = flb_http_client(u_conn, FLB_HTTP_PUT, uri, NULL, 0, NULL, 0, NULL, 0); if (!c) { flb_plg_error(ctx->ins, "cannot create HTTP client context"); + flb_sds_destroy(uri); flb_upstream_conn_release(u_conn); return FLB_FALSE; }c = flb_http_client(u_conn, FLB_HTTP_GET, uri, NULL, 0, NULL, 0, NULL, 0); if (!c) { flb_plg_error(ctx->ins, "cannot create HTTP client context"); + flb_sds_destroy(uri); flb_upstream_conn_release(u_conn); return FLB_FALSE; }Also applies to: 1343-1351
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/out_azure_blob/azure_blob.c` around lines 1244 - 1252, The code returns on flb_http_client() failure after allocating the SDS uri, leaking memory; update both failure paths where c is NULL (the HTTP client creation blocks that set c via flb_http_client) to free/destroy the allocated uri before calling flb_plg_error/flb_upstream_conn_release and returning (use the appropriate SDS free function, e.g., flb_sds_destroy or flb_sds_free). Ensure you apply this fix in both occurrences around the flb_http_client() calls that check if (!c) so uri is always freed on error.
1424-1491:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUnwind buffering state on all post-store init failures.
Once Line 1428 succeeds, the returns in Lines 1429-1446 and the new OAuth2 failure paths in Lines 1456-1491 bypass
azure_blob_store_exit(ctx).cb_azure_blob_exit()already shows that store teardown is separate fromflb_azure_blob_conf_destroy(ctx), so these init failures can leave the buffering store partially initialized.Suggested cleanup pattern
static int cb_azure_blob_init(struct flb_output_instance *ins, struct flb_config *config, void *data) { int ret; + int store_initialized = FLB_FALSE; + int token_mutex_initialized = FLB_FALSE; struct flb_azure_blob *ctx = NULL; + flb_sds_t token_url = NULL; (void) ins; (void) config; (void) data; @@ if (ctx->buffering_enabled == FLB_TRUE) { ctx->retry_time = 0; /* Initialize local storage */ - int ret = azure_blob_store_init(ctx); + ret = azure_blob_store_init(ctx); if (ret == -1) { flb_plg_error(ctx->ins, "Failed to initialize kusto storage: %s", ctx->store_dir); - return -1; + goto init_error; } + store_initialized = FLB_TRUE; @@ if (ctx->file_size <= 0) { flb_plg_error(ctx->ins, "Failed to parse upload_file_size"); - return -1; + goto init_error; } if (ctx->file_size < 1000000) { flb_plg_error(ctx->ins, "upload_file_size must be at least 1MB"); - return -1; + goto init_error; } if (ctx->file_size > MAX_FILE_SIZE) { flb_plg_error(ctx->ins, "Max total_file_size must be lower than %ld bytes", MAX_FILE_SIZE); - return -1; + goto init_error; } @@ if (ctx->atype == AZURE_BLOB_AUTH_SERVICE_PRINCIPAL) { ret = pthread_mutex_init(&ctx->token_mutex, NULL); if (ret != 0) { flb_plg_error(ctx->ins, "failed to initialize token mutex"); - flb_azure_blob_conf_destroy(ctx); - return -1; + goto init_error; } + token_mutex_initialized = FLB_TRUE; - flb_sds_t token_url; - token_url = flb_sds_create_size(256); if (!token_url) { flb_plg_error(ctx->ins, "failed to allocate token URL"); - pthread_mutex_destroy(&ctx->token_mutex); - flb_azure_blob_conf_destroy(ctx); - return -1; + goto init_error; } @@ if (ret < 0) { flb_plg_error(ctx->ins, "failed to build token URL"); - flb_sds_destroy(token_url); - pthread_mutex_destroy(&ctx->token_mutex); - flb_azure_blob_conf_destroy(ctx); - return -1; + goto init_error; } @@ if (!ctx->o) { flb_plg_error(ctx->ins, "failed to create OAuth2 context"); - pthread_mutex_destroy(&ctx->token_mutex); - flb_azure_blob_conf_destroy(ctx); - return -1; + goto init_error; } } @@ flb_output_set_http_debug_callbacks(ins); return 0; + +init_error: + if (token_url != NULL) { + flb_sds_destroy(token_url); + } + if (token_mutex_initialized == FLB_TRUE) { + pthread_mutex_destroy(&ctx->token_mutex); + } + if (store_initialized == FLB_TRUE) { + azure_blob_store_exit(ctx); + } + flb_azure_blob_conf_destroy(ctx); + return -1; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/out_azure_blob/azure_blob.c` around lines 1424 - 1491, The buffering store is initialized by azure_blob_store_init(ctx) when ctx->buffering_enabled is true but some subsequent failure paths (file_size validation and OAuth2 setup branches) return without calling azure_blob_store_exit(ctx), leaving the store partially initialized; update all failure returns after azure_blob_store_init succeeds to call azure_blob_store_exit(ctx) before destroying config or returning (specifically add azure_blob_store_exit(ctx) in the file_size validation error returns and in each OAuth2 failure path after pthread_mutex_init/ flb_sds_create_size/ flb_sds_snprintf/ flb_oauth2_create failures, prior to calling pthread_mutex_destroy and flb_azure_blob_conf_destroy(ctx) or returning).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@plugins/out_azure_blob/azure_blob.c`:
- Around line 1244-1252: The code returns on flb_http_client() failure after
allocating the SDS uri, leaking memory; update both failure paths where c is
NULL (the HTTP client creation blocks that set c via flb_http_client) to
free/destroy the allocated uri before calling
flb_plg_error/flb_upstream_conn_release and returning (use the appropriate SDS
free function, e.g., flb_sds_destroy or flb_sds_free). Ensure you apply this fix
in both occurrences around the flb_http_client() calls that check if (!c) so uri
is always freed on error.
- Around line 1424-1491: The buffering store is initialized by
azure_blob_store_init(ctx) when ctx->buffering_enabled is true but some
subsequent failure paths (file_size validation and OAuth2 setup branches) return
without calling azure_blob_store_exit(ctx), leaving the store partially
initialized; update all failure returns after azure_blob_store_init succeeds to
call azure_blob_store_exit(ctx) before destroying config or returning
(specifically add azure_blob_store_exit(ctx) in the file_size validation error
returns and in each OAuth2 failure path after pthread_mutex_init/
flb_sds_create_size/ flb_sds_snprintf/ flb_oauth2_create failures, prior to
calling pthread_mutex_destroy and flb_azure_blob_conf_destroy(ctx) or
returning).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 70b487f2-e363-4598-afb0-234a0b3c6958
📒 Files selected for processing (2)
plugins/out_azure_blob/azure_blob.cplugins/out_azure_blob/azure_blob_blockblob.c
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/out_azure_blob/azure_blob_blockblob.c
Signed-off-by: Cheruvu.Sahithee.Vaibhav@ibm.com <Cheruvu.Sahithee.Vaibhav@ibm.com>
|
@cosmo0920 @edsiper Could you please review this PR when you have time? |
1 participant
Summary
This PR adds focused service principal authentication support for the Azure Blob output plugin.
It supports Microsoft Entra ID client credentials authentication for Azure public cloud using the default authority host:
https://login.microsoftonline.comRelated
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:
Result:
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
docs: add Azure Blob service principal authentication fluent-bit-docs#2592
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
Bug Fixes
Documentation