out_azure_kusto: add region-based auth for global and China cloud#11395
out_azure_kusto: add region-based auth for global and China cloud#11395venkatesu-t wants to merge 6 commits intofluent:masterfrom
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:
📝 WalkthroughWalkthroughAdds cloud-environment awareness to the Azure Kusto plugin: new cloud enum and per-cloud macros/helpers, detection of Global vs China from ingestion endpoints, and runtime selection of cloud-specific MSAL auth URL templates, OAuth scopes, and IMDS resources across config, MSI, and OAuth flows. Two tests added to validate cloud inference. Changes
Sequence DiagramsequenceDiagram
participant Config as Configuration Handler
participant Detector as Cloud Detector
participant Helpers as Cloud Helpers
participant Auth as OAuth/MSI Client
Config->>Detector: parse ingestion_endpoint
Detector-->>Config: return cloud_environment (GLOBAL / CHINA)
Config->>Helpers: request MSAL template, IMDS resource, scope (cloud_environment)
Helpers-->>Config: return template / resource / scope
Config->>Auth: build OAuth/MSI request with returned values
Auth->>Auth: append cloud-specific scope/resource and request token
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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
fluent-bit/plugins/out_azure_kusto/azure_kusto_conf.c
Lines 836 to 837 in 10ec400
Pass resource arg when formatting MSI auth URL
The MSI URL template now includes a third %s for the IMDS resource (FLB_AZURE_MSIAUTH_URL_TEMPLATE in azure_msiauth.h), but this call (and the user-assigned branch below) still passes only two arguments. With the extra %s, vsnprintf reads an invalid pointer, producing a corrupted URL or crashing, so managed-identity auth will fail (both global and China). Please pass the resource string (e.g., get_imds_resource(ctx->cloud_environment)) when formatting.
ℹ️ 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".
venkatesu-t
force-pushed
the
feature/azure-kusto-region-based-auth
branch
from
10ec400 to
ddde2b3
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
🤖 Fix all issues with AI agents
In `@plugins/out_azure_kusto/azure_msiauth.c`:
- Around line 180-188: The two flb_sds_cat calls in azure_msiauth.c are passing
-1 which causes memcpy overflow; update the first call to pass the literal byte
length of the constant (use (int)(sizeof("&scope=") - 1)) instead of -1, and
ensure the second flb_sds_cat uses (int)strlen(scope) (or equivalent) rather
than relying on -1/implicit conversion; modify the calls that construct body
(referencing flb_sds_cat and flb_azure_kusto_get_scope) to use explicit positive
int lengths for both the "&scope=" string and the scope variable.
In `@plugins/out_azure_kusto/azure_msiauth.h`:
- Around line 23-24: The FLB_AZURE_MSIAUTH_URL_TEMPLATE has three %s
placeholders but the two call sites in azure_kusto_conf.c use only two
arguments; update both sprintf/http URL constructions that reference
FLB_AZURE_MSIAUTH_URL_TEMPLATE to pass the resource parameter as the third
argument (e.g., use the existing resource variable in the context such as
ctx->resource or the constant used for Kusto/AAD resource) so the format
receives three strings: the optional prefix ("", "&client_id="), the client_id
when applicable, and the resource string.
7644c54 to
8ef09ed
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
venkatesu-t
changed the title
There was a problem hiding this comment.
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 (1)
plugins/out_azure_kusto/azure_kusto_conf.c (1)
833-868: Fix oauth_url sizing: pointer sizeof and missing imds_resource lengthLine 860 uses
sizeof(tmpl)wheretmplis aconst char*pointer (returns ~8 bytes instead of actual string length). Additionally, lines 834, 845, and 860 don't account forimds_resourcelength in size calculations. Thoughflb_sds_snprintfhas retry logic that auto-grows, undersized allocations trigger unnecessary reallocations.🛠️ Proposed fix
- ctx->oauth_url = flb_sds_create_size(sizeof(FLB_AZURE_MSIAUTH_URL_TEMPLATE) - 1); + ctx->oauth_url = flb_sds_create_size(sizeof(FLB_AZURE_MSIAUTH_URL_TEMPLATE) - 1 + + strlen(imds_resource)); ... - ctx->oauth_url = flb_sds_create_size(sizeof(FLB_AZURE_MSIAUTH_URL_TEMPLATE) - 1 + - sizeof("&client_id=") - 1 + - flb_sds_len(ctx->client_id)); + ctx->oauth_url = flb_sds_create_size(sizeof(FLB_AZURE_MSIAUTH_URL_TEMPLATE) - 1 + + sizeof("&client_id=") - 1 + + flb_sds_len(ctx->client_id) + + strlen(imds_resource)); ... - ctx->oauth_url = flb_sds_create_size(sizeof(tmpl) - 1 + flb_sds_len(ctx->tenant_id)); + ctx->oauth_url = flb_sds_create_size(strlen(tmpl) + flb_sds_len(ctx->tenant_id));
venkatesu-t
force-pushed
the
feature/azure-kusto-region-based-auth
branch
from
a84efca to
31f4547
Compare
There was a problem hiding this comment.
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 (1)
plugins/out_azure_kusto/azure_kusto_conf.c (1)
833-856: MSI OAuth URL buffer size ignoresimds_resourcelength.Line 834 and Line 845 allocate based on the template and client_id only. The template now includes a
%sforimds_resource, so the buffer can be too small andflb_sds_snprintfmay truncate the URL, breaking MSI auth (especially for China endpoints). Includestrlen(imds_resource)in the size.🐛 Proposed fix
- ctx->oauth_url = flb_sds_create_size(sizeof(FLB_AZURE_MSIAUTH_URL_TEMPLATE) - 1); + ctx->oauth_url = flb_sds_create_size(strlen(FLB_AZURE_MSIAUTH_URL_TEMPLATE) + + strlen(imds_resource) + 1); ... - ctx->oauth_url = flb_sds_create_size(sizeof(FLB_AZURE_MSIAUTH_URL_TEMPLATE) - 1 + - sizeof("&client_id=") - 1 + - flb_sds_len(ctx->client_id)); + ctx->oauth_url = flb_sds_create_size(strlen(FLB_AZURE_MSIAUTH_URL_TEMPLATE) + + strlen("&client_id=") + + flb_sds_len(ctx->client_id) + + strlen(imds_resource) + 1);
🧹 Nitpick comments (1)
plugins/out_azure_kusto/azure_kusto_conf.c (1)
35-52: Remove or use the unusedget_kusto_scopehelper.
get_kusto_scopeisn’t referenced in this file, so a-Wunused-functionbuild can warn/fail. Either wire it into call sites or drop it.♻️ Proposed cleanup
-static const char *get_kusto_scope(int cloud_env) -{ - return flb_azure_kusto_get_scope(cloud_env); -} -
venkatesu-t
force-pushed
the
feature/azure-kusto-region-based-auth
branch
from
31f4547 to
cbfaf63
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@cosmo0920 , @edsiper can you please review this PR as this fixes the issue of not able to connect to China azure cloud. |
cosmo0920
left a comment
cosmo0920
left a comment
There was a problem hiding this comment.
The patch mostly looks good but I found several coding style issues.
Could you take a look them?
|
Also, our commit linter complains as follows: We need to suppress these linter errors. |
venkatesu-t
force-pushed
the
feature/azure-kusto-region-based-auth
branch
2 times, most recently
from
ec08ade to
7d8800c
Compare
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 (1)
plugins/out_azure_kusto/azure_msiauth.c (1)
162-162:⚠️ Potential issue | 🟠 MajorAvoid logging the federated token at
infolevel.Line 162 logs the full federated token content via
%s. This is a bearer credential — logging it even atinfolevel risks credential leakage in production log files.Suggested fix
- flb_info("[azure workload identity] after read token from file %s", federated_token); + flb_debug("[azure workload identity] federated token read from file (len=%zu)", flb_sds_len(federated_token));
🧹 Nitpick comments (2)
plugins/out_azure_kusto/azure_msiauth.c (1)
180-189: Cloud inference from OAuth2 URL strings is fragile — consider passingcloud_envexplicitly.The
flb_oauth2context doesn't carrycloud_environment, so you're re-deriving it fromctx->host/ctx->uri. This works today because the MSAL host for China containschinacloudapi.cn, but it couples scope selection to URL substring matching rather than the authoritativecloud_environmentvalue already determined inflb_azure_kusto.Consider threading
cloud_env(or the scope string itself) through the function signature so it doesn't need to be re-inferred:Sketch
-int flb_azure_workload_identity_token_get(struct flb_oauth2 *ctx, const char *token_file, const char *client_id, const char *tenant_id) +int flb_azure_workload_identity_token_get(struct flb_oauth2 *ctx, const char *token_file, const char *client_id, const char *tenant_id, int cloud_env)Then use
cloud_envdirectly instead of thestrstrchecks.tests/runtime/out_azure_kusto.c (1)
219-285: Tests only verify startup — no assertion on inferredcloud_environmentvalue.Both new tests check that
flb_startsucceeds but never verify that the plugin actually inferredFLB_AZURE_CLOUD_GLOBALorFLB_AZURE_CLOUD_CHINA. A misconfigured inference (e.g., always defaulting to GLOBAL) would still pass these tests.If the test harness allows it, consider adding an assertion on the resolved cloud environment or at minimum checking a debug log message that confirms the inferred cloud. Otherwise, acknowledge these are startup-only smoke tests.
venkatesu-t
changed the title
Signed-off-by: thimmegowni.venkatesu <thimmegowni.venkatesu@mercedes-benz.com>
Signed-off-by: thimmegowni.venkatesu <thimmegowni.venkatesu@mercedes-benz.com>
Signed-off-by: thimmegowni.venkatesu <thimmegowni.venkatesu@mercedes-benz.com>
Signed-off-by: thimmegowni.venkatesu <thimmegowni.venkatesu@mercedes-benz.com>
Signed-off-by: thimmegowni.venkatesu <thimmegowni.venkatesu@mercedes-benz.com>
Signed-off-by: thimmegowni.venkatesu <thimmegowni.venkatesu@mercedes-benz.com>
venkatesu-t
force-pushed
the
feature/azure-kusto-region-based-auth
branch
from
b20c80f to
0da4ed2
Compare
|
@cosmo0920 , I have addressed the comments mentioned by you. Can you please recheck this PR |
2 participants
Added Authentication based on two different clouds, Azure global and Azure china
Direct log ingestion from Fluent Bit to Azure Data Explorer via the azure_kusto plugin previously supported only Azure Global, as token generation relied on the Global Azure authentication authority.
Azure China uses a different authentication host, which was not supported by the existing Fluent Bit image, preventing direct ingestion into China cloud clusters.
This change adds support for Azure China cloud by updating the authentication configuration, enabling direct ingestion to Azure Data Explorer in China cloud.
Testing
Before we can approve your change; please submit the following in a comment:
Use case1: Using Global ingestion endpoint
Config file used
Output :
Use case2: Using China ingestion endpoint
Config file used
Output:
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
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