-
Notifications
You must be signed in to change notification settings - Fork 321
[Draft] Azure Split - Step 3 - Tie Everything Together #3908
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
base: dev/paul/azure/step2
Are you sure you want to change the base?
Conversation
…ogether - Brought over all of the remaining code and project changes.
…ogether - Brought over the pipeline changes. - Removed unnecessary flexibility from the MDS official pipeline tree.
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.
Pull request overview
This pull request is the third step in the Azure Split work, which removes Azure authentication implementation from the main SqlClient project and establishes dependencies on the new Abstractions package. The PR removes Azure-specific authentication code, updates project dependencies, adds test authentication providers, and updates CI/CD pipelines to handle the new package structure.
Changes:
- Removes
ActiveDirectoryAuthenticationProvider,SqlAuthenticationProvider,SqlAuthenticationToken,SqlAuthenticationParameters, and related Azure authentication classes from the MDS source - Adds dependency on
Microsoft.Data.SqlClient.Extensions.Abstractionspackage across all MDS projects (src and ref) - Updates test infrastructure with
UsernamePasswordProviderandManagedIdentityProviderimplementations for manual tests - Updates all pipeline YAML files to include
AbstractionsPackageVersionparameter and propagate it through build and test steps
Reviewed changes
Copilot reviewed 45 out of 45 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/specs/Microsoft.Data.SqlClient.nuspec | Adds Abstractions package dependency to all target frameworks |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlAuthenticationProviderManager.cs | Implements dynamic loading of Azure extension package as default provider |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs | Refactors token acquisition retry logic to use new exception model |
| src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/*.cs | Adds test authentication providers |
| src/Microsoft.Data.SqlClient/**/Microsoft.Data.SqlClient.csproj | Removes Azure.* package references, adds Abstractions reference |
| eng/pipelines/**/*.yml | Adds AbstractionsPackageVersion parameter throughout |
| foreach (SqlAuthenticationMethod candidateMethod in Instance._authenticationsWithAppSpecifiedProvider) | ||
| { | ||
| if (candidateMethod == authenticationMethod) | ||
| { | ||
| _sqlAuthLogger.LogError(nameof(SqlAuthenticationProviderManager), methodName, $"Failed to add provider {GetProviderType(provider)} because a user-defined provider with type {GetProviderType(_providers[authenticationMethod])} already existed for authentication {authenticationMethod}."); | ||
| return false; // return here to avoid replacing user-defined provider | ||
| Instance._sqlAuthLogger.LogError(nameof(SqlAuthenticationProviderManager), methodName, $"Failed to add provider {GetProviderType(provider)} because a user-defined provider with type {GetProviderType(Instance._providers[authenticationMethod])} already existed for authentication {authenticationMethod}."); | ||
|
|
||
| // The app has already specified a Provider for this | ||
| // authentication method, so we won't override it. | ||
| return false; | ||
| } | ||
| } |
Copilot
AI
Jan 22, 2026
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.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
| // Use Connection timeout value to cancel token acquire request | ||
| // after certain period of time.(int) | ||
| if (_timeout.MillisecondsRemaining < int.MaxValue) | ||
| CancellationTokenSource cts = new(); |
Copilot
AI
Jan 22, 2026
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.
Disposable 'CancellationTokenSource' is created but not disposed.
| CancellationTokenSource cts = new(); | |
| using CancellationTokenSource cts = new(); |
src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/UsernamePasswordProvider.cs
Show resolved
Hide resolved
…ogether - Removed Azure parameters from CI test tree.
…ogether - Fixed missing Abstractions and MDS package version parameters.
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.
Pull request overview
Copilot reviewed 45 out of 45 changed files in this pull request and generated no new comments.
| int retryPeriod = | ||
| ex.RetryPeriod > 0 | ||
| ? ex.RetryPeriod | ||
| : defaultRetryPeriod * (2 ^ attempt); |
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.
| : defaultRetryPeriod * (2 ^ attempt); | |
| : defaultRetryPeriod * (1 << attempt); |
| var activeDirectoryAuthProvider = new ActiveDirectoryAuthenticationProvider(instance._applicationClientId); | ||
| instance.SetProvider(SqlAuthenticationMethod.ActiveDirectoryIntegrated, activeDirectoryAuthProvider); | ||
| // Try to load our Azure extension. | ||
| var assembly = Assembly.Load(assemblyName); |
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.
To back up your TODO, this'll need to be loaded with a strong name, otherwise a client which doesn't use Azure connectivity will load any assembly at [application base]/Microsoft.Data.SqlClient.Extensions.Azure/Microsoft.Data.SqlClient.Extensions.Azure.dll and pass this any Azure credentials.
With the assembly signature verified by including the public key token in the assembly name, maybe we could simplify the load process and directly use Type.GetType with a fully-qualified assembly name.
Description
This PR ties everything together:
PR series:
Testing