-
Notifications
You must be signed in to change notification settings - Fork 32
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
Azure Managed Identity support - first version #25
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
// Copyright (c) .NET Foundation. All rights reserved. | ||
// Licensed under the MIT License. See LICENSE in the project root for license information. | ||
|
||
namespace DurableTask.SqlServer.AzureFunctions | ||
{ | ||
using System; | ||
using Newtonsoft.Json; | ||
|
||
class ManagedIdentityOptions | ||
{ | ||
[JsonProperty("authorityHost")] | ||
public Uri? AuthorityHost { get; set; } | ||
|
||
[JsonProperty("tenantId")] | ||
public string? TenantId { get; set; } | ||
|
||
[JsonProperty("useAzureManagedIdentity")] | ||
public bool UseAzureManagedIdentity { get; set; } | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,45 @@ | ||||||
// Copyright (c) .NET Foundation. All rights reserved. | ||||||
// Licensed under the MIT License. See LICENSE in the project root for license information. | ||||||
|
||||||
using System; | ||||||
using Newtonsoft.Json; | ||||||
|
||||||
namespace DurableTask.SqlServer | ||||||
{ | ||||||
/// <summary> | ||||||
/// Configuration options for Managed Identity. | ||||||
/// </summary> | ||||||
public class ManagedIdentitySettings | ||||||
{ | ||||||
public const string Resource = "https://database.windows.net/"; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Suggested change
|
||||||
|
||||||
/// <summary> | ||||||
/// Initializes a new instance of the <see cref="ManagedIdentitySettings"/> class. | ||||||
/// </summary> | ||||||
/// <param name="authorityHost">The host of the Azure Active Directory authority.</param> | ||||||
/// <param name="tenantId">The tenant id of the user to authenticate.</param> | ||||||
public ManagedIdentitySettings(Uri? authorityHost = null, string? tenantId = null) | ||||||
{ | ||||||
this.AuthorityHost = authorityHost; | ||||||
this.TenantId = tenantId; | ||||||
} | ||||||
|
||||||
/// <summary> | ||||||
/// The host of the Azure Active Directory authority. The default is https://login.microsoftonline.com/. | ||||||
/// </summary> | ||||||
[JsonProperty("authorityHost")] | ||||||
public Uri? AuthorityHost { get; set; } | ||||||
|
||||||
/// <summary> | ||||||
/// The tenant id of the user to authenticate. | ||||||
/// </summary> | ||||||
[JsonProperty("tenantId")] | ||||||
public string? TenantId { get; set; } | ||||||
|
||||||
/// <summary> | ||||||
/// Use Azure Managed Identity to connect to SQL Server | ||||||
/// </summary> | ||||||
[JsonProperty("useAzureManagedIdentity")] | ||||||
public bool UseAzureManagedIdentity { get; set; } | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
// Copyright (c) .NET Foundation. All rights reserved. | ||
// Licensed under the MIT License. See LICENSE in the project root for license information. | ||
|
||
using System.Threading.Tasks; | ||
|
||
using Azure.Core; | ||
using Azure.Identity; | ||
|
||
using Microsoft.Data.SqlClient; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just for stylistic consistency, it would be great if you could move these |
||
|
||
namespace DurableTask.SqlServer | ||
{ | ||
internal class SqlConnectionFactory | ||
{ | ||
readonly string connectionString; | ||
readonly ManagedIdentitySettings? managedIdentitySettings; | ||
|
||
public SqlConnectionFactory(string connectionString, ManagedIdentitySettings? managedIdentitySettings = null) | ||
{ | ||
this.connectionString = connectionString; | ||
this.managedIdentitySettings = managedIdentitySettings; | ||
} | ||
|
||
public async Task<SqlConnection> CreateConnection() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we rename this to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure. I guess I forgot about this naming pattern :) |
||
{ | ||
var connection = new SqlConnection(this.connectionString); | ||
if (this.managedIdentitySettings != null && this.managedIdentitySettings.UseAzureManagedIdentity) | ||
{ | ||
var azureCredentialOptions = new DefaultAzureCredentialOptions(); | ||
if (this.managedIdentitySettings.AuthorityHost != null) | ||
{ | ||
azureCredentialOptions.AuthorityHost = this.managedIdentitySettings.AuthorityHost; | ||
} | ||
if (!string.IsNullOrEmpty(this.managedIdentitySettings.TenantId)) | ||
{ | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it make sense to cache this |
||
var accessToken = await azureCredential.GetTokenAsync(requestContext); | ||
connection.AccessToken = accessToken.Token; | ||
} | ||
|
||
return connection; | ||
} | ||
} | ||
} |
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.
Let's add
[JsonProperty("managedIdentityOptions")]
to this new property.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.
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
, andazureManagedIdentityTenantId
. TheManagedIdentitySettings
class, however, can remain as-is.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.
Sure, let's do it. Thanks for letting me know.