Update Azure SQL Authentication to use Azure.Identity #14
Conversation
… with Azure.Identity
evochriso
changed the title
|
Thanks I'll pull this in when we drop support |
|
@droyad If you don't mind educating a fellow engineer, would you be willing to explain why it'd be better to wait for another major version bump? My understanding is that:
(Hope I didn't miss anything obvious.) |
@droyad this update shouldn't be a breaking change since it doesn't change behavior or API surface, just swaps a deprecated dependency for its replacement, and has no dependency on System.Data.SqlClient. Perhaps a minor version bump instead of wait for the next major release? |
There was a problem hiding this comment.
Just wanted to give my input on this after having written a CustomConnectionManager to be able to use Azure.Identity earlier. We used AzureCliCredential for dev environments and WorkloadIdentityCredential/ManagedIdentityCredential for production environments to skip the loop of having to check all possible types of credentials.
| @@ -28,12 +29,11 @@ public AzureSqlConnectionManager(string connectionString, string resource) | |||
| public AzureSqlConnectionManager(string connectionString, string resource, string tenantId, string azureAdInstance = "https://login.microsoftonline.com/") | |||
| : base(new DelegateConnectionFactory((log, dbManager) => | |||
| { | |||
| var tokenProvider = new DefaultAzureCredential(); | |||
There was a problem hiding this comment.
Having the caller pass an instance of TokenCredential would be optimal as DefaultAzureCredential iterates over all possible credential types until it fines one that works.
If this is going to be released on the next major, perhaps add this as a parameter?
There was a problem hiding this comment.
@Swampen my goal with this PR was to resolve the usage of a deprecated Nuget package, without introducing a breaking change. In my opinion, using the DefaultAzureCredential here makes sense so we can be as flexible as possible, and overhead of checking all possible types of credentials seems negligible for a database upgrader that will just make one connection to perform the upgrade. Changing or adding the constructors for the AzureSqlConnectionManager class to accept other parameters such as TokenCredential would be a breaking change.
I don't know what @droyad has planned for the next major release, but I agree we should change constructors in that release.
…to evochriso-main # Conflicts: # src/dbup-sqlserver/AzureSqlConnectionManager.cs # src/dbup-sqlserver/dbup-sqlserver.csproj
|
Alright how does that look? If ok, could you give it a quick manual test and I'll merge it. I don't have AAD Integrated DB on hand. |
|
Is it necessary to explicitly depend on |
It does not need it. I've removed it. |
|
@droyad consider the version of Azure.Identity - Microsoft.Data.SqlClient requires Azure.Identity 1.11.3 or greater, but that version of Azure.Identity has a vulnerability and should be bumped to 1.12.0 |
The |
|
The latest SqlClient references Identity |
Checklist
Description
Replace deprecated package Microsoft.Azure.Services.AppAuthentication with Azure.Identity
Resolves #12