Azure Managed Identity support - first version #25
Conversation
| @@ -24,6 +24,8 @@ class SqlDurabilityOptions | |||
| [JsonProperty("taskEventBatchSize")] | |||
| public int TaskEventBatchSize { get; set; } = 10; | |||
|
|
|||
| public ManagedIdentityOptions? ManagedIdentityOptions { get; set; } | |||
There was a problem hiding this comment.
Let's add [JsonProperty("managedIdentityOptions")] to this new property.
There was a problem hiding this comment.
Unfortunately the Azure Functions configuration system doesn't like our use of nested options here. We'll need to flatten it in order for it to be recognized. My suggestion is that we add the three properties directly to the SqlDurabilityOptions class and rename them such that they all have the same prefix. For example, azureManagedIdentityEnabled, azureManagedIdentityAuthorityHost, and azureManagedIdentityTenantId. The ManagedIdentitySettings class, however, can remain as-is.
There was a problem hiding this comment.
Sure, let's do it. Thanks for letting me know.
| using Azure.Core; | ||
| using Azure.Identity; | ||
|
|
||
| using Microsoft.Data.SqlClient; |
There was a problem hiding this comment.
Just for stylistic consistency, it would be great if you could move these using statements inside the namespace block.
| azureCredentialOptions.InteractiveBrowserTenantId = this.managedIdentitySettings.TenantId; | ||
| } | ||
| var azureCredential = new DefaultAzureCredential(azureCredentialOptions); | ||
| var requestContext = new TokenRequestContext(new string[] { ManagedIdentitySettings.Resource }); |
There was a problem hiding this comment.
Would it make sense to cache this requestContext object?
| this.managedIdentitySettings = managedIdentitySettings; | ||
| } | ||
|
|
||
| public async Task<SqlConnection> CreateConnection() |
There was a problem hiding this comment.
Can we rename this to CreateConnectionAsync()? It's not a big deal, but we try to follow this naming convention for methods that return tasks (which is the case now that we may fetch an Azure AD token).
There was a problem hiding this comment.
Sure. I guess I forgot about this naming pattern :)
| /// </summary> | ||
| public class ManagedIdentitySettings | ||
| { | ||
| public const string Resource = "https://database.windows.net/"; |
There was a problem hiding this comment.
In my testing, I noticed that I needed to add ".default" to the end of this URL before I could successfully fetch a token from Azure AD. I think this might be a newer requirement in the more modern versions of Azure.Identity.
| public const string Resource = "https://database.windows.net/"; | |
| public const string Resource = "https://database.windows.net/.default"; |
|
I'd like to take this change today, so I'll go ahead and merge this PR and then add my own changes as a separate PR. |
These changes are to resolve issue #18