out_azure_kusto: add support for Azure sovereign and custom clouds#11447
out_azure_kusto: add support for Azure sovereign and custom clouds#11447tanmaya-panda1 wants to merge 1 commit 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 multi-cloud support to the Azure Kusto output plugin: introduces cloud configuration options, per-cloud login/scope/resource resolution at init, threads dynamic Kusto OAuth scope/resource through MSI and OAuth token requests, and updates related signatures and URL templates. Changes
Sequence Diagram(s)sequenceDiagram
participant Config as Configuration
participant ConfInit as KustoConf Init
participant Resolver as Cloud Resolver
participant AuthModule as Auth (MSI/OAuth)
participant TokenEndpoint as Token Service
Config->>ConfInit: flb_azure_kusto_conf_create()
ConfInit->>Resolver: azure_kusto_resolve_cloud_endpoints(ctx)
Resolver->>Resolver: determine login_host, kusto_scope, kusto_resource
Resolver-->>ConfInit: resolved endpoints
ConfInit->>AuthModule: request token (pass login_host, kusto_scope, kusto_resource)
AuthModule->>TokenEndpoint: POST token request (includes dynamic scope/resource)
TokenEndpoint-->>AuthModule: access token
AuthModule-->>ConfInit: token returned for Kusto operations
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
tanmaya-panda1
force-pushed
the
master
branch
from
81a0344 to
4734f37
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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_kusto/azure_msiauth.c (2)
163-163:⚠️ Potential issue | 🟠 MajorSecurity: Do not log the federated token at
infolevel.This logs the full federated token (a JWT client assertion) which is a sensitive credential. Even at debug level this would be risky, but at
infolevel it will appear in production logs by default. Remove or redact this log statement.Proposed fix
- flb_info("[azure workload identity] after read token from file %s", federated_token); + flb_debug("[azure workload identity] successfully read federated token from file");
173-189:⚠️ Potential issue | 🔴 CriticalUnchecked
flb_sds_catreturn values can lead to NULL dereference.If any intermediate
flb_sds_catcall fails (returns NULL), the next call receives NULL as input, causing a crash inflb_sds_len()/flb_sds_avail(). The NULL check at line 184 only catches the final call's failure. Each call (or at least the chain) needs early-exit on NULL.Proposed fix — bail out after each cat
body = flb_sds_cat(body, "client_id=", 10); + if (!body) goto body_error; body = flb_sds_cat(body, client_id, strlen(client_id)); + if (!body) goto body_error; /* Use the correct grant_type and length for workload identity */ body = flb_sds_cat(body, "&grant_type=client_credentials", 30); + if (!body) goto body_error; body = flb_sds_cat(body, "&client_assertion_type=urn:ietf:params:oauth:client-assertion-type:jwt-bearer", 77); + if (!body) goto body_error; body = flb_sds_cat(body, "&client_assertion=", 18); + if (!body) goto body_error; body = flb_sds_cat(body, federated_token, flb_sds_len(federated_token)); + if (!body) goto body_error; /* Use the cloud-specific scope for Kusto */ body = flb_sds_cat(body, "&scope=", 7); + if (!body) goto body_error; body = flb_sds_cat(body, scope, strlen(scope)); - - if (!body) { - /* This check might be redundant if flb_sds_cat handles errors, but safe */ - flb_error("[azure workload identity] failed to build request body"); - flb_sds_destroy(federated_token); - return -1; - } + if (!body) goto body_error;Then add the error label before the upstream connection block:
/* fall through to upstream connection ... */ goto body_ok; body_error: flb_error("[azure workload identity] failed to build request body"); flb_sds_destroy(federated_token); return -1; body_ok:
🧹 Nitpick comments (2)
plugins/out_azure_kusto/azure_msiauth.c (1)
149-149: Consider lowering log level frominfotodebugfor routine entry-point tracing.
flb_infoon every token fetch call adds noise to production logs. This is a tracing statement better suited forflb_debug.plugins/out_azure_kusto/azure_kusto.h (1)
54-55: Remove unused macroFLB_AZURE_KUSTO_SCOPE.The macro at line 55 is dead code—it's not referenced anywhere in the codebase. It has been replaced by
FLB_AZURE_KUSTO_SCOPE_PUBLIC(line 68) and the per-cloud variants, along with dynamic scope resolution viactx->kusto_scope. Delete line 55 to reduce confusion.
tanmaya-panda1
force-pushed
the
master
branch
from
4734f37 to
2096ca5
Compare
tanmaya-panda1
force-pushed
the
master
branch
from
2096ca5 to
ba9443d
Compare
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 (1)
plugins/out_azure_kusto/azure_msiauth.c (1)
163-163:⚠️ Potential issue | 🟠 MajorFederated token logged at
infolevel — credential leakage risk.Line 163 logs the full federated token content at
flb_infolevel. This token is a security credential and should not appear in production logs. Consider changing toflb_debugor removing the token value from the message entirely.
🤖 Fix all issues with AI agents
In `@plugins/out_azure_kusto/azure_msiauth.c`:
- Around line 180-182: The chained flb_sds_cat calls that append "&scope=" and
scope to body risk dereferencing NULL on OOM; after each flb_sds_cat invocation
(including the new calls that append "&scope=" and the scope string) check that
body != NULL before making the next flb_sds_cat or using body, and if NULL
perform the same cleanup/early-return path used elsewhere in this function
(match existing error handling in this file). Specifically, add a NULL check for
body immediately after the flb_sds_cat that appends "&scope=" and again after
the flb_sds_cat that appends scope so the function (the azure MSIAUTH flow using
variable body and scope) does not continue with a NULL sds.
🧹 Nitpick comments (1)
plugins/out_azure_kusto/azure_kusto.h (1)
54-55: Remove dead code macroFLB_AZURE_KUSTO_SCOPE.This macro is unused and has been superseded by the per-cloud variants (
FLB_AZURE_KUSTO_SCOPE_PUBLIC,FLB_AZURE_KUSTO_SCOPE_CHINA,FLB_AZURE_KUSTO_SCOPE_US_GOVERNMENT,FLB_AZURE_KUSTO_SCOPE_GERMAN), which are actively used throughout the codebase. Delete the definition at line 55 to reduce confusion.
tanmaya-panda1
changed the title
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 (2)
plugins/out_azure_kusto/azure_msiauth.c (2)
163-163:⚠️ Potential issue | 🟠 MajorSensitive federated token logged at
infolevel — credential leak risk.
flb_infowill emit the raw JWT federated token into logs under normal operation. This token grants authentication capabilities and should never appear in logs. At minimum, remove this line or downgrade toflb_debugwith a truncated/redacted representation.Proposed fix
- flb_info("[azure workload identity] after read token from file %s", federated_token); + flb_debug("[azure workload identity] federated token read successfully (length=%zu)", flb_sds_len(federated_token));
220-220:⚠️ Potential issue | 🟠 MajorDebug log exposes
client_assertion(federated token) in request body.The body contains
client_assertion=<JWT>, which is a sensitive credential. Even at debug level, this risks leaking credentials to log aggregation systems. Consider logging only the body length or a sanitized version.Proposed fix
- flb_debug("[azure workload identity] Sending request body (len=%zu): %s", flb_sds_len(body), body); + flb_debug("[azure workload identity] Sending request body (len=%zu)", flb_sds_len(body));
🤖 Fix all issues with AI agents
In `@plugins/out_azure_kusto/azure_kusto.h`:
- Around line 46-77: Remove the unused dead macro FLB_AZURE_KUSTO_SCOPE and
eliminate all references to the retired German sovereign cloud by deleting
FLB_AZURE_CLOUD_GERMAN and the German-specific constants
FLB_AZURE_LOGIN_HOST_GERMAN, FLB_AZURE_KUSTO_SCOPE_GERMAN, and
FLB_AZURE_KUSTO_RESOURCE_GERMAN from azure_kusto.h; ensure any switch/selection
logic elsewhere only handles PUBLIC, CHINA and US_GOVERNMENT and update any
documentation/comments that mention the removed German cloud constants.
ba9443d to
4982395
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)
163-163:⚠️ Potential issue | 🟠 MajorSecurity: Federated token logged at INFO level — leaks credential to logs.
Line 163 logs the full federated token content via
flb_info. This token is a bearer credential and should never be logged, especially at INFO level which is enabled in production.Suggested fix
- flb_info("[azure workload identity] after read token from file %s", federated_token); + flb_debug("[azure workload identity] successfully read federated token from file (len=%zu)", flb_sds_len(federated_token));
🧹 Nitpick comments (3)
plugins/out_azure_kusto/azure_msiauth.c (3)
173-215: OOM inflb_sds_catleaks the original SDS buffer.When
flb_sds_catfails (returns NULL), the originalbodypointer is overwritten and lost. Thebody_errorlabel doesn't (and can't) free it sincebodyis already NULL. This is a minor memory leak on OOM conditions. The NULL checks themselves are good and address the previous review concern about crash-on-OOM.To fully fix the leak, use a temporary variable pattern:
flb_sds_t tmp; tmp = flb_sds_cat(body, "client_id=", 10); if (!tmp) { flb_sds_destroy(body); goto body_error; } body = tmp;However, this is a pervasive pattern in the Fluent Bit codebase and OOM is a catastrophic condition, so this can be deferred.
197-205: Useflb_sds_len(scope)instead ofstrlen(scope)for consistency.On line 202,
strlen(scope)is used where the caller passesctx->kusto_scope(anflb_sds_t). While functionally equivalent (SDS strings are NUL-terminated), usingstrlenon anflb_sds_tbypasses the cached length, and is inconsistent with theflb_sds_len(federated_token)usage on line 193.Suggested fix
- body = flb_sds_cat(body, scope, strlen(scope)); + body = flb_sds_cat(body, scope, flb_sds_len(scope));Note: This would require changing the parameter type from
const char *scopetoflb_sds_t scope(orconst flb_sds_t scope), or keepingstrlenif the function must accept plain C strings.
208-215: Thegoto body_ok/body_errorpattern is unconventional and harms readability.Jumping forward over an error label with
goto body_okis an unusual control flow that makes the function harder to follow. A simpler structure would be to check for errors inline and jump to a single cleanup label at the end of the function.
Add cloud_name configuration property to support multiple Azure cloud environments -- AzureCloud (default), AzureChinaCloud, and AzureUSGovernmentCloud. Additionally, add cloud_login_host, cloud_kusto_scope, and cloud_kusto_resource properties for private/sovereign clouds (e.g. USSEC, USNAT, BLEU) where the endpoints are not publicly known. When all three custom properties are provided, they override cloud_name. Each cloud resolves to the correct login host, OAuth scope, and IMDS resource URL for authentication and ingestion. Add NULL checks after each flb_sds_cat call in the workload identity token request body construction to prevent crash on OOM. Signed-off-by: Tanmaya Panda <tanmayapanda@microsoft.com>
tanmaya-panda1
force-pushed
the
master
branch
from
4982395 to
63d045b
Compare
1 participant
This pull request adds support for multiple Azure cloud environments (AzureCloud, AzureChinaCloud, AzureUSGovernmentCloud) to the Azure Kusto output plugin. It introduces cloud-specific endpoint resolution, updates the authentication logic and configuration to be cloud-aware, and adds comprehensive tests for each supported cloud environment and error handling for invalid configurations.
The most important changes are:
Cloud Environment Support and Endpoint Resolution
cloud_nameconfiguration option and associated logic to resolve cloud-specific endpoints (login host, Kusto scope, and resource) based on the selected Azure environment. This enables the plugin to work with Azure public, China, US Government, and German clouds. (azure_kusto.c,azure_kusto.h,azure_kusto_conf.c) [1] [2] [3] [4] [5]azure_kusto.c,azure_msiauth.c,azure_msiauth.h) [1] [2] [3] [4] [5] [6]Configuration and Resource Management
azure_kusto.h,azure_kusto_conf.c) [1] [2]cloud_nameparameter with validation and documentation. (azure_kusto.c)Testing
out_azure_kusto.c) [1] [2] [3]These changes make the Azure Kusto output plugin more flexible and robust for users operating in various Azure cloud environments.
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
New Features
Bug Fixes / Improvements