-
Notifications
You must be signed in to change notification settings - Fork 280
Support managed identity for Event Grid #3183
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
src/WebJobs.Extensions.DurableTask/EventGridLifeCycleNotificationHelper.cs
Show resolved
Hide resolved
src/WebJobs.Extensions.DurableTask/EventGridLifeCycleNotificationHelper.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Extensions.DurableTask/EventGridLifeCycleNotificationHelper.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Extensions.DurableTask/EventGridLifeCycleNotificationHelper.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Extensions.DurableTask/EventGridLifeCycleNotificationHelper.cs
Outdated
Show resolved
Hide resolved
…as eventGridKeySettingName
src/WebJobs.Extensions.DurableTask/EventGridLifeCycleNotificationHelper.cs
Show resolved
Hide resolved
src/WebJobs.Extensions.DurableTask/EventGridLifeCycleNotificationHelper.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! Left a comment. Also, can you remove those alerts about the PR? And have we done any e2e testing on Azure about msi?
src/WebJobs.Extensions.DurableTask/EventGridLifeCycleNotificationHelper.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Extensions.DurableTask/EventGridLifeCycleNotificationHelper.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Extensions.DurableTask/EventGridLifeCycleNotificationHelper.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Extensions.DurableTask/EventGridLifeCycleNotificationHelper.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm concerned about the large code churn in the latest iteration. I'm also concerned about the potential complexities and dangers of caching tasks, which have caused major outages for customers in other contexts.
Can we explore a simpler solution, such as keeping initialization as it was before, but calling SetUpAuthenticationAsync()
inside an existing async method that we always run, such as inside the StartTaskHubWorkerIfNotStartedAsync()
method?
src/WebJobs.Extensions.DurableTask/EventGridLifeCycleNotificationHelper.cs
Show resolved
Hide resolved
defaultAzureCredentialOptions.ManagedIdentityClientId = this.Options.ClientId; | ||
} | ||
|
||
defaultCredential = this.Options == null ? new DefaultAzureCredential() : new DefaultAzureCredential(defaultAzureCredentialOptions); // CodeQL [SM05137] Use DefaultAzureCredential explicitly for local development and is decided by the user |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One change we should consider in the future is the ability to choose a credential type other than DefaultAzureCredential
. This credential type is convenient but can cause problems or unexpected behavior in some setups because it may not always choose to use managed identity, even if managed identity is available.
This PR adds support for managed identity for publishing events to Event Grid.
I tested this by publishing Durable Functions events to Event Grid and then sent those events to an Azure Storage Queue.
For context, customers currently specify the event grid topic key through host.json and local.settings.json (https://learn.microsoft.com/en-us/azure/azure-functions/durable/durable-functions-event-publishing?tabs=csharp-script):
This PR adds the following configuration support.
System Assigned Managed identity configuration:
App Settings
EventGrid__topicEndpoint
app setting with the value as the Event Grid topic endpointConfiguration
User Assigned Managed identity configuration:
App Settings
EventGrid__topicEndpoint
app setting with the value as the Event Grid topic endpointEventGrid__credential
app setting with the valuemanagedidentity
EventGrid__clientId
app setting with the value of the user assigned managed identity client ID.Configuration
Resolves #2924
Pull request checklist
pending_docs.md
release_notes.md
/src/Worker.Extensions.DurableTask/AssemblyInfo.cs
dev
andmain
branches and will not be merged into thev2.x
branch.