Added support for system-assigned identity to EventGrid output bindings

[andrewjw1995]

This change allows users to declare EventGrid bindings that use DefaultAzureCredential for authentication, so they can authenticate with the EventGrid topic using a system-assigned identity and RBAC rules instead of managing the access keys:

[FunctionName("Example")]
public async Task RunAsync(
    [TimerTrigger("0 * * * * *", RunOnStartup = true)] TimerInfo timerInfo,
    [EventGrid(TopicEndpointUri = "EventGridEndpoint", UseDefaultAzureCredential = true)] IAsyncCollector<CloudEvent> eventCollector
)

The original binding properties continue to function the same as before:

[FunctionName("Example")]
public async Task RunAsync(
    [TimerTrigger("0 * * * * *", RunOnStartup = true)] TimerInfo timerInfo,
    [EventGrid(TopicEndpointUri = "EventGridEndpoint", TopicKeySetting = "EventGridKey")] IAsyncCollector<CloudEvent> eventCollector
)

---

Looking for feedback on:

• Do we need a dedicated 'UseDefaultAzureCredential' property, or should that be the default behaviour if the 'TopicKeySetting' is null? I added the property to ensure backwards-compatible behaviour.
• The 'TopicKeySetting' property is decorated with the [AppSetting] attribute, and I've disabled the null-check when 'UseDefaultAzureCredential' is set to true. Will the functions runtime ignore a null value, or will this cause a runtime exception?

[github-actions]

Thank you for your contribution @andrewjw1995! We will review the pull request and get back to you soon.

[andrewjw1995]

@microsoft-github-policy-service agree

[JoshLove-msft]

Hi @andrewjw1995,
Thank you for the contribution! For Identity integration, we have established a pattern in several of the other extensions. Essentially, we defined a config pattern that can be used to configure an identity-based connection. It would be great if we can use that same pattern for the EventGrid output binding. It is a bit different for this binding from the other extensions we've worked on because EventGrid doesn't support connection strings, but I think we can still do it. I'm thinking we add a new Connection property that adheres to the semantics used by the other extensions. A user would specify the name of their Connection setting like so:

[FunctionName("Example")]
public async Task RunAsync(
    [TimerTrigger("0 * * * * *", RunOnStartup = true)] TimerInfo timerInfo,
    [EventGrid(Connection = "MyConnection")] IAsyncCollector<CloudEvent> eventCollector
)

In their app settings or local.settings.json, the minimal way to set up Identity, which would use DefaultAzureCredential would be the following:
MyConnection__TopicEndpointUri: "{Eg Endpoint}"

Users can optionally add other settings to further configure (these are the common settings among the various extensions):

MyConnection__credential: "managedIdentity",
MyConnection__clientId: "{clientId}"  

The actual credential can be created by passing in the configuration section to the AzureComponentFactory.CreateCredential method.

Here is an example of how this is done for the Service Bus extension.

Thinking out loud, if a user uses the existing TopicEndpointUri property then they are signaling that they are not using Identity and must also specific the TopicKeySetting. I would also be okay with allowing the TopicEndpointUri to be set in either the connection setting or in the explicit property, and we would use whether or not the TopicKeySetting is set to indicate whether we should use Identity or not. This seems the most user-friendly given that we already have the TopicEndpointUri property, so I suggest we do this instead.

The logic would work as follows:

1. If Connection is set, TopicKeySetting must not be set. Throw if the user has set both.
2. If Connection is set, the TopicEndpointUri can be included within the Connection OR the TopicEndpointUri property. Throw if both are set to different values.
3. When using a connection pass the connection section into the AzureComponentFactory.CreateCredential method.

Please let us know if you are interested in pursuing these changes. It sounds like a lot but I think it probably won't be much more code than you have right now.

[andrewjw1995]

Hi Josh, Thanks for the detailed reply. Yes, I'll have a go at implementing this over the weekend

…

On Thu, 25 May 2023, 5:16 pm JoshLove-msft, ***@***.***> wrote: Hi @andrewjw1995 <https://github.com/andrewjw1995>, Thank you for the contribution! For Identity integration, we have established a pattern <https://learn.microsoft.com/en-us/azure/azure-functions/functions-reference?tabs=blob#configure-an-identity-based-connection> in several of the other extensions. Essentially, we defined a config pattern that can be used to configure an identity-based connection. It would be great if we can use that same pattern for the EventGrid output binding. It is a bit different for this binding from the other extensions we've worked on because EventGrid doesn't support connection strings, but I think we can still do it. I'm thinking we add a new Connection property that adheres to the semantics used by the other extensions. A user would specify the name of their Connection setting like so: [FunctionName("Example")]public async Task RunAsync( [TimerTrigger("0 * * * * *", RunOnStartup = true)] TimerInfo timerInfo, [EventGrid(Connection = "MyConnection")] IAsyncCollector<CloudEvent> eventCollector) In their app settings or local.settings.json, the minimal way to set up Identity, which would use DefaultAzureCredential would be the following: MyConnection__TopicEndpointUri: "{Eg Endpoint}" Users can optionally add other settings to further configure: MyConnection__credential: "managedIdentity", MyConnection__clientId: "{clientId}" The actual credential can be created by passing in the configuration section to the AzureComponentFactory.CreateCredential method <https://learn.microsoft.com/en-us/dotnet/api/microsoft.extensions.azure.azurecomponentfactory.createtokencredential?view=azure-dotnet> . Here is an example <https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/servicebus/Microsoft.Azure.WebJobs.Extensions.ServiceBus/src/Config/ServiceBusClientFactory.cs#L84> of how this is done for the Service Bus extension. If a user uses the existing TopicEndpointUri property then they are signaling that they are not using Identity and must also specific the TopicKeySetting. I would also be okay with allowing the TopicEndpointUri to be set in *either* the connection setting *or* in the explicit property, and we would use whether or not the TopicKeySetting is set to indicate whether we should use Identity or not. This seems the most user-friendly given that we already have the TopicEndpointUri property. The logic would work as follows: 1. If Connection is set, TopicKeySetting must not be set. Throw if the user has set both. 2. If Connection is set, the TopicEndpointUri can be included within the Connection OR the TopicEndpointUri property. Throw if both are set to different values. 3. When using a connection pass the connection section into the AzureComponentFactory.CreateCredential method. Please let us know if you are interested in pursuing these changes. It sounds like a lot but I think it probably won't be much more code than you have right now. — Reply to this email directly, view it on GitHub <#36484 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACDLUZQY7RIBYCVW4DIVRUDXH3TI7ANCNFSM6AAAAAAYN5JHRE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[andrewjw1995]

@JoshLove-msft the attribute validator and the collector factory don't currently have access to the configuration. I've moved them into a dedicated 'EventGridAsyncCollectorFactory' class which has the configuration injected into it, and I moved the classes into a 'config' namespace so it's more similar to the ServiceBus example you pointed me to.

I'm calling builder.Services.AddAzureClientsCore(); to get the AzureComponentFactory registered. I can't see the source for this so I'm not sure if this is the most appropriate/specific method, but it's what the ServiceBus extension does.

Are you ok with:

• the namespace change (it only affects internal classes)
• A protected constructor on an internal class used solely for mocking - I could make an interface instead

I would also like to add some new unit tests to make sure the attribute validation works as expected, so I'll work on that some time over the next few days

[JoshLove-msft]

@JoshLove-msft the attribute validator and the collector factory don't currently have access to the configuration. I've moved them into a dedicated 'EventGridAsyncCollectorFactory' class which has the configuration injected into it, and I moved the classes into a 'config' namespace so it's more similar to the ServiceBus example you pointed me to.

I'm calling builder.Services.AddAzureClientsCore(); to get the AzureComponentFactory registered. I can't see the source for this so I'm not sure if this is the most appropriate/specific method, but it's what the ServiceBus extension does.

Are you ok with:

• the namespace change (it only affects internal classes)
• A protected constructor on an internal class used solely for mocking - I could make an interface instead

I would also like to add some new unit tests to make sure the attribute validation works as expected, so I'll work on that some time over the next few days

This all sounds good to me. One scenario that I hadn't covered is what we should do if a user sets the TopicEndpointUri property but nothing else. I think we can treat this as a scenario where DefaultAzureCredential can be used by just passing in an empty configuration section to the CreateCredential call, though I suppose we would need to create a dummy section class that implements IConfiguration. What do you think?

[andrewjw1995]

I've added the unit tests, and updated the behaviour to be more like you describe. No need for a dummy IConfiguration class, I'm just creating an empty ConfigurationBuilder and building it.

I notice in the existing examples, the uri is always a child of the 'Connection' section, not the direct value of the section. E.g:

{
  "IsEncrypted": false,
  "Values": {
    "<CONNECTION_NAME_PREFIX>__blobServiceUri": "<blobServiceUri>",
    "<CONNECTION_NAME_PREFIX>__fullyQualifiedNamespace": "<serviceBusFqdn>"
  }
}

The code I've written is currently looking for the event grid uri in <CONNECTION_NAME_PREFIX> but I think it would be more consistent to look for something like <CONNECTION_NAME_PREFIX>__eventGridTopicUri. What do you think?

[JoshLove-msft]

The code I've written is currently looking for the event grid uri in <CONNECTION_NAME_PREFIX> but I think it would be more consistent to look for something like <CONNECTION_NAME_PREFIX>__eventGridTopicUri. What do you think?

Agreed, the Uri should be nested. Since there is no connection string in Event Grid, users would never set the app setting to a string, but instead it should always be a JSON object.

[andrewjw1995]

Done. It now looks for <CONNECTION_NAME_PREFIX>__topicEndpointUri

[JoshLove-msft]

You will need to run the Export-Api.ps1 script and commit the updated API listing file:

eng\scripts\Export-API.ps1 eventgrid

[JoshLove-msft]

This looks awesome! Once the API listing is updated I will share with some folks on my team to get sign off on the API surface and then we should be able to merge. Thanks @andrewjw1995!

[JoshLove-msft]

/azp run net - eventgrid - tests

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[JoshLove-msft]

Fixes #23169