JDetmar / NLog.Extensions.AzureStorage

NLog Target for Azure Storage. Uses NLog batch write to optimize writes to Storage.
MIT License
31 stars 19 forks source link

Add support for user assigned managed identity #120

Closed CptButtercup closed 2 years ago

CptButtercup commented 2 years ago

I needed to use this package to access an event hub with a user assigned managed identity on Azure. I couldn't get it to work with the existing parameters so I modified it to support a clientIdentity parameter. This is used to access the managed identity. I figured I would send this modification upstream in case anyone else out there needs this.

snakefoot commented 2 years ago

Thank you for the contribution. Makes sense to change from the AzureServiceTokenProvider to Azure.Identity.

My plan was to replace AzureServiceTokenProvider with Azure.Identity ver. 1.6 when it becomes final (Currently v1.6 BETA-1)

Then Azure.Identity ver. 1.6 should be feature complete (Adds support for TenantId + ResourceId), and AzureServiceTokenProvider can be thrown away (Better support for exotic platforms like Xamarin)

snakefoot commented 2 years ago

Right now the AzureServiceTokenProvider requires ClientId to be provided by the ConnectionString:

RunAs=App;AppId={ClientId of user-assigned identity}

https://docs.microsoft.com/en-us/dotnet/api/overview/azure/service-to-service-authentication#connection-string-support

snakefoot commented 2 years ago

@CptButtercup Would it be possible for you to try out this pre-release build that combines ClientIdentity with the old AzureServiceTokenProvider:

Should be a drop-in-replacement of your current version made with this pull-request, as it also introduce the new ClientIdentiy-option.

CptButtercup commented 2 years ago

I finally had a chance to test this. In the package you sent, connectionString is still listed as a required parameter. nLog fails to initialize the target when no connection string is provided but if I provide a connection string then this extension uses that and skips the managed identity. When using a managed identity I should only have to provide serviceUri and clientId.

snakefoot commented 2 years ago

What happens if you cheat and specify ConnectionString="" (empty string)?

CptButtercup commented 2 years ago

It works when I pass in an empty string. I'm seeing logging arriving in my event hub. I would still request that you remove the required property. It makes it a bit confusing. I'm sure the next person that works on this won't know what to do with the connection string.

snakefoot commented 2 years ago

Yes. Will fix the issue with required ConnectionString before merging. Thank you for testing.

snakefoot commented 2 years ago

Now updated #121 to no longer have ConnectionString as required-parameter.

snakefoot commented 2 years ago

NLog.Extensions.AzureEventHub v3.2.0 has been released - https://www.nuget.org/packages/NLog.Extensions.AzureEventHub/