Tests | Cleanup AAD Password auth from tests + code cleanup#4390
Tests | Cleanup AAD Password auth from tests + code cleanup#4390cheenamalhotra wants to merge 11 commits into
Conversation
cheenamalhotra
added
the
Area\Tests
There was a problem hiding this comment.
Pull request overview
Removes the Entra ID “password auth” connection-string configuration from the test suite (and CI templates) following the pipeline change in #4288, replacing it with a single “bare” AzureSqlConnectionString that tests extend at runtime to exercise specific authentication modes.
Changes:
- Renamed
AADPasswordConnectionString→AzureSqlConnectionStringacross test config (Config.cs/config.default.jsonc), docs, and Azure DevOps pipeline templates; updated pipeline variables accordingly. - Added shared test helpers under
tests/Common/(CommonUtils,StringExtensions) to compose auth/credential keywords onto a base connection string. - Updated ManualTests + Azure extensions tests to remove
ActiveDirectoryPasswordcoverage and to use managed identity / token-based flows with the new helpers.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| TESTGUIDE.md | Updates documented test config property name/meaning for AAD/Azure SQL tests. |
| src/Microsoft.Data.SqlClient/tests/tools/Microsoft.Data.SqlClient.TestUtilities/Microsoft.Data.SqlClient.TestUtilities.csproj | References the new shared test helper project. |
| src/Microsoft.Data.SqlClient/tests/tools/Microsoft.Data.SqlClient.TestUtilities/config.default.jsonc | Renames config field and clarifies Azure SQL base conn string expectation (no creds). |
| src/Microsoft.Data.SqlClient/tests/tools/Microsoft.Data.SqlClient.TestUtilities/Config.cs | Renames the config field to AzureSqlConnectionString. |
| src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectivityTests/AADConnectionTest.cs | Removes AAD password tests and rewrites AAD scenarios to use helpers and AzureSqlConnectionString. |
| src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectionPoolTest/ConnectionPoolTest.cs | Updates pooled AAD/token tests to use AzureSqlConnectionString and async token retrieval. |
| src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/AADFedAuthTokenRefreshTest/AADFedAuthTokenRefreshTest.cs | Switches the fed-auth refresh scenario to managed identity-based auth/provider usage. |
| src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/ManagedIdentityProvider.cs | Renames the custom provider class to clarify it targets user-assigned MI. |
| src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/DataTestUtility.cs | Renames config fields, adds MI-based token retrieval APIs, and removes AAD password token generation. |
| src/Microsoft.Data.SqlClient/tests/Common/StringExtensions.cs | Adds fluent helpers for composing auth/credential keywords and for removing auth/credential keywords. |
| src/Microsoft.Data.SqlClient/tests/Common/CommonUtils.cs | Adds shared random/name helpers used by updated tests. |
| src/Microsoft.Data.SqlClient.Extensions/Azure/test/Config.cs | Renames the Azure test config field read from config.jsonc. |
| src/Microsoft.Data.SqlClient.Extensions/Azure/test/AADConnectionTest.cs | Updates Azure extension tests to build auth modes from the base Azure SQL conn string. |
| eng/pipelines/jobs/test-azure-package-ci-job.yml | Switches pipeline config-writing to AzureSqlConnectionString and updates secret scoping. |
| eng/pipelines/dotnet-sqlclient-ci-core.yml | Switches CI template config-writing to AzureSqlConnectionString (WestUS2/EastUS). |
| eng/pipelines/common/templates/steps/update-config-file-step.yml | Renames template parameter and output property to AzureSqlConnectionString. |
| eng/pipelines/common/templates/jobs/ci-run-tests-job.yml | Renames config pass-through property to AzureSqlConnectionString. |
| .github/instructions/testing.instructions.md | Updates the test-config docs to match the renamed Azure SQL config property. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4390 +/- ##
==========================================
- Coverage 65.32% 63.85% -1.48%
==========================================
Files 285 281 -4
Lines 43373 66638 +23265
==========================================
+ Hits 28335 42550 +14215
- Misses 15038 24088 +9050
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
benrr101
left a comment
benrr101
left a comment
There was a problem hiding this comment.
I think the scope of these changes are a bit too broad. Don't mind cleanup where sensible, but I'm concerned about the intentions behind the connection string changes being more sweeping than we really want right now.
|
|
||
| namespace Microsoft.Data.SqlClient.Tests.Common; | ||
|
|
||
| public static class CommonUtils |
There was a problem hiding this comment.
A bucket of "common utils" in the "common" project seems a bit too general purpose. It would be nice to maybe split them into more purposeful classes of utilities. Like ConnectionStringUtilities, RandomUtilities.
| return string.Concat(prefix, path.Substring(0, Math.Min(length, path.Length))); | ||
| } | ||
|
|
||
| public static string GenerateObjectName() |
There was a problem hiding this comment.
We have a thing for this already, please use Paul/EdwardNeal's DatabaseObject.GetLongName/GetShortName
| /// <summary> | ||
| /// Adds a user provided or a dummy user to the connection string to test the connection string parsing logic | ||
| /// </summary> | ||
| public static string AddUserToConnString(this string connectionString, string? user = null) |
There was a problem hiding this comment.
Why are we doing this with raw string manipulation rather than using ConnectionStringBuilder to manipulate it?
| /// Extension methods for string class to add connection string keywords to connection string | ||
| /// for testing connection string parsing logic | ||
| /// </summary> | ||
| public static class StringExtensions |
There was a problem hiding this comment.
Yeah, these are all connection string utilities. Unless there's a really good reason not to, we should be using ConnectionStringBuilder to manipulate connection strings.
Additionally, the EnsureSeparator method should be moved to here since these are all ConnectionStringUtilities.
| /// <summary> | ||
| /// Adds user provided or a dummy password to the connection string to test the connection string parsing logic | ||
| /// </summary> | ||
| public static string AddPasswordToConnString(this string connectionString, string? password = null) |
There was a problem hiding this comment.
I thought the goal was to remove password auth from these tests 🤔
| { | ||
| return AKVBaseUri != null && !string.IsNullOrEmpty(UserManagedIdentityClientId) && !string.IsNullOrEmpty(AKVTenantId) && IsNotAzureSynapse(); | ||
| } | ||
| public static bool IsAKVSetupAvailable() => AKVBaseUri != null && !string.IsNullOrEmpty(UserManagedIdentityClientId) && !string.IsNullOrEmpty(AKVTenantId) && IsNotAzureSynapse(); |
There was a problem hiding this comment.
Can we put these conditions on separate lines for readability?
| return s_defaultCredential; | ||
| } | ||
| public static string GetUserIdentityConnectionString() | ||
| => AzureSqlConnectionString |
There was a problem hiding this comment.
This should be indented one more over
| | `AADPasswordConnectionString` | Entra ID password auth | | ||
| | `TCPConnectionString` | Primary TCP connection to on-premises SQL Server | | ||
| | `NPConnectionString` | Named Pipes connection to on-premises SQL Server | | ||
| | `AzureSqlConnectionString` | Entra ID connection string to Azure SQL Database | |
There was a problem hiding this comment.
I'm a bit concerned with making this change.
The PR pipeline uses two different test runs, one for localhost, one for azure. The differentiation is made by running it once with an azure connection string for TCPConnectionString/NPConnectionString and then using a localhost connection string. This allows us to run the same set of tests on both environments, since the tests just specify if it can't run on Azure (or I suppose only run on Azure) based on the TCP/NP connection string that was provided.
Changing this "AADPasswordConnectionString" to "AzureSqlConnectionString" and changing the comments for TCP and NP mixes up the ideas. The new guidance suggests that tests are either azure-specific or localhost-specific. But, aside from the handful being changed in this PR, they are not azure/localhost specific. And, worse, in order to enable azure/localhost specific tests, we would need to duplicate all the existing tests that use TCP/NP.
eg, we'd have to change from
[ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnectionStringsSetUp))]
public void MyTest() { /* ... test that uses TCPConnectionString ... */ }to
[ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.IsLocalhostConnectionStringsSetUp)]
public void MyTest_Localhost() { /* ... test that uses TCPConnectionString ... */ }
[ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.IsAzureConnectionStringsSetUp)]
public void MyTest_Azure() { /* ... test that uses AzureConnectionString ... */ }That seems like a really bad idea to me.
I've been percolating on a better way for us to handle multiple connection strings for our test scenarios. I'd really like a bit of runway to put together a prototype for how it could be used to improve our manual tests.
But anyhow, my general feedback for this would be ... maybe we shouldn't change the name and guidance on these connection strings right now. Maybe we should just make sure that we eliminate the AAD password auth from the tests, but not go further than that.
There was a problem hiding this comment.
👍 can we just remove the 'AADPasswordConnectionString'? why do we need a replacement?
| public void SqlVariantTest() | ||
| { | ||
| string tableName = DataTestUtility.GenerateObjectName(); | ||
| string tableName = CommonUtils.GenerateObjectName(); |
There was a problem hiding this comment.
These should be using DataTestUtility.GetShortName/GetLongName
github-project-automation
Bot
moved this from In review
to Waiting for customer
in SqlClient Board
mdaigle
left a comment
mdaigle
left a comment
There was a problem hiding this comment.
I have similar feedback to @benrr101. Looking for a few things:
- using TCP connection string instead of a new azure connection string
- using existing random generation helpers
- using connection string builder in place of direct string manipulation
The second two could be left as backlog items because you're just shuffling existing code around.
| | `AADPasswordConnectionString` | Entra ID password auth | | ||
| | `TCPConnectionString` | Primary TCP connection to on-premises SQL Server | | ||
| | `NPConnectionString` | Named Pipes connection to on-premises SQL Server | | ||
| | `AzureSqlConnectionString` | Entra ID connection string to Azure SQL Database | |
There was a problem hiding this comment.
👍 can we just remove the 'AADPasswordConnectionString'? why do we need a replacement?
| string connStr = RemoveKeysInConnStr(Config.PasswordConnectionString, removeKeys) + | ||
| $"Authentication=Active Directory Service Principal; User ID={Config.ServicePrincipalId}; PWD={Config.ServicePrincipalSecret};"; | ||
| ConnectAndDisconnect(connStr); | ||
| string connString = Config.AzureSqlConnString |
There was a problem hiding this comment.
I think we can just modify the TCP connection string as needed in each test.
|
|
||
| [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.IsAADPasswordConnStrSetup), nameof(DataTestUtility.IsAADAuthorityURLSetup))] | ||
| public static void AccessTokenConnectionPoolingTest() | ||
| [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.IsAzureConnStringSetup))] |
There was a problem hiding this comment.
Should be able to use IsAzure and then look at the TCP connection string.
| /// <param name="connStr">The connection string to modify.</param> | ||
| /// <param name="keysToRemove">The keys to remove from the connection string.</param> | ||
| /// <returns>The modified connection string with the specified keys removed.</returns> | ||
| public static string RemoveKeysInConnStr(this string connStr, string[] keysToRemove) |
There was a problem hiding this comment.
SqlConnectionStringBuilder provides this functionality.
Follow up changes to test suite after #4288 where AAD Password auth was disabled from pipelines. This PR removes the AAD Password Conn String parameter completely to make it easier for AAD tests to be managed.
What Changed
Pipeline / config
AAD_PASSWORD_CONN_STR(_eastus)withAZURE_SQL_CONN_STRING_WestUS2/AZURE_SQL_CONN_STRING_EastUS(bare conn string, no credentials).AADAuthorityURLconfig property, as its now unused.AADPasswordConnectionString→AzureSqlConnectionStringacrossConfig.cs,config.default.jsonc, and pipeline templates (update-config-file-step.yml,ci-run-tests-job.yml,dotnet-sqlclient-ci-core.yml,test-azure-package-ci-job.yml).AADServicePrincipalIdinside theIsFork == 'False'guard (matches existing policy forAADServicePrincipalSecret).New shared test helpers (
tests/Common/)CommonUtils.cs:EnsureSeparator,GenerateRandomSecureString,GenerateRandomCharacters,GenerateObjectName.StringExtensions.cs: fluentAdd*AuthenticationToConnStringextensions (Integrated, Interactive, DeviceCodeFlow, ServicePrincipal, WorkloadIdentity, MSI, Default) plusAddUserToConnString/AddPasswordToConnString. Tests now compose auth ontoAzureSqlConnectionStringdynamically — required for broker-aware test legs.Test updates
ManualTests/SQL/ConnectivityTests/AADConnectionTest.cs: removed allActiveDirectoryPasswordcases; rewrote MSI / Workload Identity / Device Code / Default / token-callback tests against the new helpers; applied target-typednew(), collection expressions,using SecureString,Assert.Equal(ConnectionState.Open, …).DataTestUtility.cs,AADFedAuthTokenRefreshTest,ConnectionPoolTest, andMicrosoft.Data.SqlClient.Extensions/Azure/test/AADConnectionTest.cs+Config.csupdated to match.TESTGUIDE.mdand.github/instructions/testing.instructions.mdupdated.Breaking change (CI only)
Downstream pipelines /
config.user.jsoncmust renameAADPasswordConnectionString→AzureSqlConnectionStringand supply a bare Azure SQL connection string (noAuthentication=,User ID=,Password=). The variablesAAD_PASSWORD_CONN_STR/_eastusare no longer consumed and can be retired.Validation
dotnet buildManualTests / net9.0 — 0 warnings, 0 errors.dotnet testUnitTests / net9.0 — 792 pass; 4 failures are pre-existing macOS-env issues (Windows-only SSPI + keychain-restricted AE fixtures), not regressions.Future TODOs