Add Client Secret support into azure event hub integrations#17245
Add Client Secret support into azure event hub integrations#17245kaiyan-sheng merged 27 commits intomainfrom
Conversation
🚀 Benchmarks reportTo see the full report comment with |
There was a problem hiding this comment.
Thanks for working on this @kaiyan-sheng, it LGTM overall, no blockers.
IMO there's one aspect to consider.
In variable descriptions we use the internal field names, like auth_type or connection_string.
For example:
I have no problem in matching the UI elements to the internal name, but I see my brain performing a lookup somewhere for a split second.
I and think our users wouldn't find this complicated as well, but maybe we can remove this tiny friction without losing clarity.
What about (1) using actual field names that match the UI and (2) use bold/italics to field highlight dependencies?
Is a subtle difference, but IMO it can smooth over this friction until we get proper var group dependencies in 9.4.
|
@claude pls review the PR |
|
Claude finished @ishleenk17's task in 2m 35s —— View job PR Review Complete
I've reviewed this PR and submitted my feedback. The implementation looks solid overall. Summary:
Minor items noted:
No blockers identified - the PR is in good shape for merge. --- | View PR review Why is Claude responding? | Type Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not. |
There was a problem hiding this comment.
This PR adds client secret authentication support across 12 Azure Event Hub integrations. The implementation is well-structured and consistent across packages. A few observations:
Overall Quality
- The implementation follows the pattern from beats PR #47256 correctly
- Documentation is comprehensive with clear setup instructions for both authentication methods
- The template logic properly conditionally emits
authority_hostonly whenauth_typeisclient_secret - Changelogs are consistently updated across all 12 packages
Minor Considerations
-
Unresolved Review Thread: There's an unresolved comment from @zmoog asking whether
auth_typeshould berequired: true(currentlyrequired: falsewith defaultconnection_string). While the current approach maintains backward compatibility, making it required would be more explicit. Worth discussing. -
Test Coverage: The policy tests for
m365_defenderonly cover theconnection_stringpath. Consider adding a test case forclient_secretauth to verify the template correctly outputs the OAuth2 parameters and omitsconnection_string/storage_account_key. -
Pre-existing Issue (out of scope for this PR): The
packages/github/data_stream/audit/agent/stream/azure-eventhub.yml.hbsfile usesfilebeat-graphactivitylogs-{{eventhub}}as the default container name, which appears to be copy-pasted from the azure graphactivitylogs template. This is a pre-existing issue not introduced by this PR.
No Blockers - The PR is ready to merge from a technical standpoint. The implementation is solid and the changes are backward compatible.
Why is Claude responding? | Type @claude to interact further
Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.
@zmoog Good point! I think this is definitely worth improving. I'm changing them to use the actual parameter names in the UI instead of names in Filebeat! But for the README files I see we are using param names from Filebeat: Do you think it's worth changing too? |
|
/test |
💚 Build Succeeded
History
|
Yeah, makes sense. We should probably align this section with what we have at https://www.elastic.co/docs/reference/integrations/azure/events, adding the new options in the "Event Hub Processor v2 only" seciton. |
@zmoog and I talked about this in slack, the decision is to keep the new options with authentication section so all the params around auth are together. Also I see some benefits of keeping the config parameters in documentation instead of the UI names when we are trying to debug using config files instead of UI. So I think we can keep it as is for now (tbh maybe we should document both the UI name and the param name). |
|
Package azure - 1.36.0 containing this change is available at https://epr.elastic.co/package/azure/1.36.0/ |
|
Package azure_ai_foundry - 0.9.0 containing this change is available at https://epr.elastic.co/package/azure_ai_foundry/0.9.0/ |
|
Package azure_app_service - 1.1.0 containing this change is available at https://epr.elastic.co/package/azure_app_service/1.1.0/ |
|
Package azure_frontdoor - 2.3.0 containing this change is available at https://epr.elastic.co/package/azure_frontdoor/2.3.0/ |
|
Package azure_functions - 0.12.0 containing this change is available at https://epr.elastic.co/package/azure_functions/0.12.0/ |
|
Package azure_logs - 0.6.0 containing this change is available at https://epr.elastic.co/package/azure_logs/0.6.0/ |
|
Package azure_openai - 1.11.0 containing this change is available at https://epr.elastic.co/package/azure_openai/1.11.0/ |
|
Package github - 2.22.0 containing this change is available at https://epr.elastic.co/package/github/2.22.0/ |
|
Package kubernetes - 1.85.0 containing this change is available at https://epr.elastic.co/package/kubernetes/1.85.0/ |
|
Package m365_defender - 5.9.0 containing this change is available at https://epr.elastic.co/package/m365_defender/5.9.0/ |
|
Package microsoft_defender_cloud - 3.3.0 containing this change is available at https://epr.elastic.co/package/microsoft_defender_cloud/3.3.0/ |
|
Package microsoft_sentinel - 1.3.0 containing this change is available at https://epr.elastic.co/package/microsoft_sentinel/1.3.0/ |
Proposed commit message
Add OAuth2 client secret authentication support for Azure Event Hub integrations
This enhancement adds OAuth2 (client_secret) authentication support to all Azure integrations that use Event Hub, following the implementation in elastic/beats#47256.
Changes include:
Checklist
changelog.ymlfile.Author's Checklist
Affected 12 packages:
How to test this PR locally
Related issues
Screenshots