out_azure_logs_ingestion: add support for Managed Identities#2062
out_azure_logs_ingestion: add support for Managed Identities#2062stefanoboriero wants to merge 7 commits into
Conversation
This change updates the documentation to document support for Managed Identities authentication. It tries to align with the documentation style and content for the similar feature for the out_azure_kusto plugin. Signed-off-by: Stefano Boriero <stefano.boriero@seqera.io>
stefanoboriero
force-pushed
the
azure-log-ingestion-msi
branch
from
15d5f84 to
8f065d6
Compare
esmerel
left a comment
esmerel
left a comment
There was a problem hiding this comment.
Stylistic updates for consistency. @fluent/fluent-bit-maintainers should review for technical accuracy.
esmerel
left a comment
esmerel
left a comment
There was a problem hiding this comment.
Stylistic updates for consistency. @fluent/fluent-bit-maintainers should review for technical accuracy.
eschabell
added
4.2.1
waiting-on-review
Signed-off-by: Eric D. Schabell <eric@schabell.org>
|
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)
📝 WalkthroughWalkthroughThe PR updates pipeline/outputs/azure_logs_ingestion.md to document authentication methods (Service Principal and Managed Identity), changes the ChangesAzure Logs Ingestion Authentication Documentation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Co-authored-by: Lynette Miles <6818907+esmerel@users.noreply.github.com> Signed-off-by: Eric D. Schabell <eric@schabell.org>
Co-authored-by: Lynette Miles <6818907+esmerel@users.noreply.github.com> Signed-off-by: Eric D. Schabell <eric@schabell.org>
Co-authored-by: Lynette Miles <6818907+esmerel@users.noreply.github.com> Signed-off-by: Eric D. Schabell <eric@schabell.org>
Co-authored-by: Lynette Miles <6818907+esmerel@users.noreply.github.com> Signed-off-by: Eric D. Schabell <eric@schabell.org>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pipeline/outputs/azure_logs_ingestion.md (1)
47-61:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd missing
auth_typeparameter to the configuration table.The configuration examples use
auth_type: managed_identity(lines 193, 232, 275, 314), but this parameter is not documented in the configuration parameters table.➕ Proposed addition
Add this row to the configuration parameters table after line 51:
| `auth_type` | Set the authentication type: `service_principal` or `managed_identity`. | `service_principal` |🤖 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 `@pipeline/outputs/azure_logs_ingestion.md` around lines 47 - 61, Add the missing configuration parameter `auth_type` to the parameters table (the table that lists `auth_url`, `client_id`, etc.) so it documents the authentication mode used in examples; include the allowed values (`service_principal` or `managed_identity`) and set the default to `service_principal`, and place this row near the other authentication fields (e.g., after `tenant_id` or `time_generated`) so readers can find it alongside `client_id`, `client_secret`, and `tenant_id`.
♻️ Duplicate comments (1)
pipeline/outputs/azure_logs_ingestion.md (1)
52-52:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate
client_iddescription to include managed identity usage.The parameter description should mention that
client_idis used differently depending on the authentication type. For managed identity, it should besystemfor system-assigned identity or the managed identity's client ID (GUID) for user-assigned identity.📝 Proposed fix
-| `client_id` | The client ID of the AAD application. | _none_ | +| `client_id` | The client ID of the AAD application. When using managed identity authentication, set this to `system` for system-assigned identity or provide the managed identity's client ID (GUID). Required for `service_principal` and `managed_identity` auth. | _none_ |🤖 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 `@pipeline/outputs/azure_logs_ingestion.md` at line 52, Update the `client_id` parameter description to explain its behavior for managed identities: state that when using managed identity authentication `client_id` should be "system" for system-assigned identity or the user-assigned managed identity's client ID (GUID); keep the existing meaning for service principal/OAuth scenarios (the AAD application/client ID) and clarify which auth type each value corresponds to so readers know when to use "system" vs a GUID vs the regular client ID.
🤖 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 `@pipeline/outputs/azure_logs_ingestion.md`:
- Line 42: The current bullet links to Kusto (Azure Data Explorer) managed
identity docs; replace that URL/text with the Log Analytics / Data Collection
Rule (DCR) managed identity documentation and update the link text to instruct
assigning the managed identity permissions to the Data Collection Rule (DCR) for
Azure Monitor Logs ingestion (i.e., swap the Kusto URL and "Kusto database"
wording with the Log Analytics/DCR managed identity docs and "Data Collection
Rule" wording).
- Line 34: The markdown link "[Authorize the app in your database]" points to
Kusto auth docs but should reference Azure Log Analytics/Data Collection Rule
permissions for the Logs Ingestion API; update the link in
azure_logs_ingestion.md to a document about granting permissions to a Data
Collection Rule and the Logs Ingestion API (replace the Kusto link with the Log
Analytics/DCR docs so users are guided to grant DCR ingestion permissions rather
than Kusto DB permissions).
- Around line 313-314: Update the fluent-bit .conf example to use Title_Case for
plugin keys: replace occurrences of "client_id" and "auth_type" with "Client_Id"
and "Auth_Type" respectively (and scan the same .conf block for other lowercase
plugin-specific keys and convert them to Title_Case) so the .conf examples
follow the classic fluent-bit .conf convention while leaving YAML examples
untouched.
---
Outside diff comments:
In `@pipeline/outputs/azure_logs_ingestion.md`:
- Around line 47-61: Add the missing configuration parameter `auth_type` to the
parameters table (the table that lists `auth_url`, `client_id`, etc.) so it
documents the authentication mode used in examples; include the allowed values
(`service_principal` or `managed_identity`) and set the default to
`service_principal`, and place this row near the other authentication fields
(e.g., after `tenant_id` or `time_generated`) so readers can find it alongside
`client_id`, `client_secret`, and `tenant_id`.
---
Duplicate comments:
In `@pipeline/outputs/azure_logs_ingestion.md`:
- Line 52: Update the `client_id` parameter description to explain its behavior
for managed identities: state that when using managed identity authentication
`client_id` should be "system" for system-assigned identity or the user-assigned
managed identity's client ID (GUID); keep the existing meaning for service
principal/OAuth scenarios (the AAD application/client ID) and clarify which auth
type each value corresponds to so readers know when to use "system" vs a GUID vs
the regular client ID.
🪄 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: d07ca1cb-d693-437b-a753-ac488dd93117
📒 Files selected for processing (1)
pipeline/outputs/azure_logs_ingestion.md
eschabell
left a comment
eschabell
left a comment
There was a problem hiding this comment.
@stefanoboriero thanks so much for the docs PR, just a few trivial vale issues I tagged that need your attention. Update these and tag me to re-review.
eschabell
added
waiting-for-user
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)
pipeline/outputs/azure_logs_ingestion.md (3)
49-61:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd missing
auth_typeparameter to configuration table.The new authentication examples use
auth_type: managed_identity, but this parameter is not documented in the configuration parameters table. Users need to know the available values and default.📝 Proposed addition
Add this row after the
auth_urlrow:| `auth_type` | The authentication method: `service_principal` or `managed_identity`. | `service_principal` |Based on learnings: The azure_kusto plugin documents
auth_typewith values includingservice_principal,managed_identity, andworkload_identity, withservice_principalas default.🤖 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 `@pipeline/outputs/azure_logs_ingestion.md` around lines 49 - 61, The config table is missing the auth_type parameter used by the examples; add a new row for `auth_type` (placed after `auth_url`) describing allowed values and default, e.g. "The authentication method: `service_principal`, `managed_identity`, or `workload_identity`" with default `service_principal`, mirroring how `auth_type` is documented in the azure_kusto plugin; update the Parameters table entries (`auth_url`, `auth_type`) so the examples using `auth_type: managed_identity` are covered.
52-52:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate
client_iddescription to document managed identity usage.The current description only mentions "The client ID of the AAD application," but doesn't explain that for managed identity authentication, this field has special semantics:
systemfor system-assigned identity or the managed identity's client ID (GUID) for user-assigned identity.📝 Proposed fix
-| `client_id` | The client ID of the AAD application. | _none_ | +| `client_id` | The client ID of the AAD application. When using managed identity authentication, set this to `system` for system-assigned identity or provide the managed identity's client ID (GUID) for user-assigned identity. | _none_ |Based on learnings: The azure_kusto plugin documents client_id with similar semantics for managed identity: "use
systemasclient_idforsystem-assigned identity, or specify the managed identity's client ID."🤖 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 `@pipeline/outputs/azure_logs_ingestion.md` at line 52, Update the `client_id` description to document managed identity semantics: state that `client_id` can be "system" to use the system-assigned managed identity or the specific managed identity's client ID (GUID) for a user-assigned identity, mirroring the azure_kusto plugin wording; edit the line defining `client_id` in pipeline/outputs/azure_logs_ingestion.md so the description clearly mentions these two modes and the expected values ("system" or the managed identity GUID).
231-237:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse Title_Case for plugin keys in .conf format.
The
.confformat should use Title_Case for plugin-specific keys. Lines 231-237 use lowercaseclient_id,auth_type,dce_url,dcr_id,table_name,time_generated, andtime_key, but these should be Title_Case.🔧 Proposed fix
- client_id XXXXXXXX-xxxx-yyyy-zzzz-xxxxyyyyzzzzxyzz - auth_type managed_identity - dce_url https://log-analytics-dce-XXXX.region-code.ingest.monitor.azure.com - dcr_id dcr-xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx - table_name ladcr_CL - time_generated true - time_key Time + Client_Id XXXXXXXX-xxxx-yyyy-zzzz-xxxxyyyyzzzzxyzz + Auth_Type managed_identity + Dce_Url https://log-analytics-dce-XXXX.region-code.ingest.monitor.azure.com + Dcr_Id dcr-xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx + Table_Name ladcr_CL + Time_Generated true + Time_Key TimeBased on learnings: classic fluent-bit.conf examples use Title_Case for plugin-specific keys (e.g., Client_Id, Auth_Type), while YAML fluent-bit.yaml examples use snake_case lowercase keys (e.g., client_id, auth_type).
🤖 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 `@pipeline/outputs/azure_logs_ingestion.md` around lines 231 - 237, The plugin keys in the .conf snippet are using snake_case but must be Title_Case for fluent-bit .conf files; update each key shown (client_id, auth_type, dce_url, dcr_id, table_name, time_generated, time_key) to Title_Case (e.g., Client_Id, Auth_Type, Dce_Url, Dcr_Id, Table_Name, Time_Generated, Time_Key) so the plugin-specific keys follow the .conf convention.
🤖 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 `@pipeline/outputs/azure_logs_ingestion.md`:
- Line 30: Fix the typo in the documentation sentence that reads "Service
principal authentication is the default method. To use it, you mst create an
Azure AD application:" by changing "mst" to "must" so the sentence becomes "To
use it, you must create an Azure AD application:"; update the text in
pipeline/outputs/azure_logs_ingestion.md where this exact phrase occurs.
---
Outside diff comments:
In `@pipeline/outputs/azure_logs_ingestion.md`:
- Around line 49-61: The config table is missing the auth_type parameter used by
the examples; add a new row for `auth_type` (placed after `auth_url`) describing
allowed values and default, e.g. "The authentication method:
`service_principal`, `managed_identity`, or `workload_identity`" with default
`service_principal`, mirroring how `auth_type` is documented in the azure_kusto
plugin; update the Parameters table entries (`auth_url`, `auth_type`) so the
examples using `auth_type: managed_identity` are covered.
- Line 52: Update the `client_id` description to document managed identity
semantics: state that `client_id` can be "system" to use the system-assigned
managed identity or the specific managed identity's client ID (GUID) for a
user-assigned identity, mirroring the azure_kusto plugin wording; edit the line
defining `client_id` in pipeline/outputs/azure_logs_ingestion.md so the
description clearly mentions these two modes and the expected values ("system"
or the managed identity GUID).
- Around line 231-237: The plugin keys in the .conf snippet are using snake_case
but must be Title_Case for fluent-bit .conf files; update each key shown
(client_id, auth_type, dce_url, dcr_id, table_name, time_generated, time_key) to
Title_Case (e.g., Client_Id, Auth_Type, Dce_Url, Dcr_Id, Table_Name,
Time_Generated, Time_Key) so the plugin-specific keys follow the .conf
convention.
🪄 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: 7c456c98-4836-4328-b059-b8478f5adc2b
📒 Files selected for processing (1)
pipeline/outputs/azure_logs_ingestion.md
eschabell
removed
the
waiting-for-user
…d cleanup - Fix typo: mst -> must in service principal section - Spell out AKS acronym on first use (Azure Kubernetes Service) - Replace GUID acronym with full term "globally unique identifier" - Replace Kusto auth link with DCR permissions tutorial link and update link text - Convert .conf plugin keys to Title_Case Applies to fluent#2062 Signed-off-by: Eric D. Schabell <eric@schabell.org>
|
@stefanoboriero pushed fixes for all the vale and AI review issues. |
eschabell
left a comment
eschabell
left a comment
There was a problem hiding this comment.
@stefanoboriero just waiting now on code PR merging.
3 participants
This change updates the documentation to document support for Managed Identities authentication. It tries to align with the documentation style and content for the similar feature for the out_azure_kusto plugin. The feature is implemented on PR fluent/fluent-bit#10867
Summary by CodeRabbit