Access Token Support for SqlConnection in Manual Tests #2654

dauinsight · 2024-07-08T21:25:00Z

In our manual tests we are changing the way we authenticate to Azure SQL DB by using managed identity.

In order to support this, when we make a connection from a macOS container, we must assign an access token to the connection. This authorizes the instance with access to the managed identity and hence the DB.

A few tests have been skipped (when running macOS) as we're unable to assign the access token to the connection from the test.

David-Engel · 2024-07-09T15:14:40Z

This seems like it would make it difficult to change test connection strings due to validation on the AccessToken property. What problem is this PR solving?

dauinsight · 2024-07-09T15:50:39Z

This seems like it would make it difficult to change test connection strings due to validation on the AccessToken property. What problem is this PR solving?

Since assigning a UAMI directly to an agent pool is not recommended + our Mac agent pool, we are using a service connection in ADO and have federated credentials to it on a UAMI which has access to the database. As a result, we have an Azure Cli step in ADO to retrieve the token and assign it to the connection to authenticate.

…where NRE comes from.

….com/dauinsight/SqlClient into dev/daniel/azure-assign-access-token

arellegue

Looks good to me.

DavoudEshtehari · 2024-07-17T18:42:47Z

src/Microsoft.Data.SqlClient/tests/ManualTests/ProviderAgnostic/ReaderTest/ReaderTest.cs

+                using (SqlConnection con = (SqlConnection) provider.CreateConnection())
                {
+                    // Testing in macOS Azure SQL container requires access token authentication
+                    if (!DataTestUtility.AuthenticatingWithoutAccessToken)
+                    {
+                        con.AccessToken = DataTestUtility.AADAccessToken;
+                    }


If you introduce an extension method your change will be simpler without repeating yourself.

Suggested change

using (SqlConnection con = (SqlConnection) provider.CreateConnection())

{

// Testing in macOS Azure SQL container requires access token authentication

if (!DataTestUtility.AuthenticatingWithoutAccessToken)

{

con.AccessToken = DataTestUtility.AADAccessToken;

}

using (DbConnection con = provider.CreateConnectionEx())

static DbConnection CreateConnectionEx(this DbProviderFactory provider) { DbConnection conn = provider.CreateConnection(); // Testing in macOS Azure SQL container requires access token authentication if (!DataTestUtility.AuthenticatingWithoutAccessToken && conn is SqlConnection sqlConn) { sqlConn.AccessToken = DataTestUtility.AADAccessToken; return sqlConn; } return conn; }

DavoudEshtehari · 2024-07-17T18:50:12Z

src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/DataTestUtility.cs

+        public static bool AuthenticatingWithoutAccessToken
+        {
+            get
+            {
+                return !RuntimeInformation.IsOSPlatform(OSPlatform.OSX);


This makes more sense with positive naming. Feel free to modify the naming as you like.

Suggested change

public static bool AuthenticatingWithoutAccessToken

{

get

{

return !RuntimeInformation.IsOSPlatform(OSPlatform.OSX);

public static bool ForceAuthenticationWithAccessToken

{

get

{

return RuntimeInformation.IsOSPlatform(OSPlatform.OSX);

DavoudEshtehari · 2024-07-17T20:24:44Z

src/Microsoft.Data.SqlClient/tests/ManualTests/TracingTests/DiagnosticTest.cs

@@ -186,7 +186,8 @@ public void ExecuteReaderWithCommandBehaviorTest()
        }

        // Synapse: Parse error at line: 1, column: 27: Incorrect syntax near 'for'.
-        [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup), nameof(DataTestUtility.IsNotAzureSynapse))]
+        // Skip: Access token authentication does not work with RemoteExecutor (#31362)


Same thing is applied for the similar comments.

Suggested change

// Skip: Access token authentication does not work with RemoteExecutor (#31362)

// Skip: Access token authentication does not work with RemoteExecutor (active issue #31362)

DavoudEshtehari · 2024-07-17T22:38:21Z

src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/DataTestUtility.cs

            {
+                Console.WriteLine($"Generating new access token...");


Suggested change

Console.WriteLine($"Generating new access token...");

DavoudEshtehari · 2024-07-17T22:45:40Z

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlSchemaInfoTest/SqlSchemaInfoTest.cs

+            Console.WriteLine($"TCPConnectionString = {DataTestUtility.TCPConnectionString}");
+


Suggested change

Console.WriteLine($"TCPConnectionString = {DataTestUtility.TCPConnectionString}");

DavoudEshtehari · 2024-07-17T23:00:44Z

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlSchemaInfoTest/SqlSchemaInfoTest.cs

            {
                string currentDb = connection.Database;
-                SqlConnectionStringBuilder builder = new SqlConnectionStringBuilder(connection.ConnectionString);
+                SqlConnectionStringBuilder builder = new SqlConnectionStringBuilder(DataTestUtility.TCPConnectionString);


I believe the point of this test is building and reading the connection string properties from a SqlConnection.ConnectionString. So, instead of skipping these kind of tests you can force the using of the DataTestUtility.TCPConnectionString through the helper method by adding a new argument and bypassing the internal logic.

Suggested change

SqlConnectionStringBuilder builder = new SqlConnectionStringBuilder(DataTestUtility.TCPConnectionString);

SqlConnectionStringBuilder builder = new SqlConnectionStringBuilder(connection.ConnectionString);

DavoudEshtehari · 2024-07-17T23:14:11Z

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlBulkCopyTest/Helpers.cs

                {
+                    // Testing in macOS Azure SQL container requires access token authentication
+                    if (!DataTestUtility.AuthenticatingWithoutAccessToken)


You can add another property in DataTestUtility and keep it positive, instead of multiplying a negative property by adding a negative sign, which is a bit tricky to follow.

DavoudEshtehari · 2024-07-17T23:33:32Z

...Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlBulkCopyTest/AdjustPrecScaleForBulkCopy.cs

+        // Skip: We cannot assign an access token to the connection in this test (#31362)
+        [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup), nameof(DataTestUtility.AuthenticatingWithoutAccessToken))]


It doesn't make sense to me that authentication could affect this test. Could you double-check it?

DavoudEshtehari · 2024-07-18T18:31:52Z

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/LocalDBTest/LocalDBTest.cs

@@ -53,7 +53,7 @@ public static void LocalDBMarsTest()
        [ConditionalFact(nameof(IsLocalDBEnvironmentSet))]
        public static void InvalidLocalDBTest()
        {
-            using var connection = new SqlConnection(s_badConnectionString);
+            using var connection = DataTestUtility.GetSqlConnection(s_badConnectionString);


LocalDb doesn't require Access Token for authentication.

Suggested change

using var connection = DataTestUtility.GetSqlConnection(s_badConnectionString);

using var connection = new SqlConnection(s_badConnectionString);

DavoudEshtehari · 2024-07-18T18:32:12Z

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/LocalDBTest/LocalDBTest.cs

@@ -189,7 +189,7 @@ private static void ConnectionTest(string connectionString)

        private static void OpenConnection(string connString)
        {
-            using SqlConnection connection = new(connString);
+            using SqlConnection connection = DataTestUtility.GetSqlConnection(connString);


Suggested change

using SqlConnection connection = DataTestUtility.GetSqlConnection(connString);

using SqlConnection connection = new(connString);

DavoudEshtehari · 2024-07-18T18:45:26Z

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/KerberosTests/KerberosTest.cs

@@ -16,7 +16,7 @@ public class KerberosTests
        public void IsKerBerosSetupTestAsync(string connectionStr)
        {
            KerberosTicketManagemnt.Init(DataTestUtility.KerberosDomainUser, DataTestUtility.KerberosDomainPassword);
-            using SqlConnection conn = new(connectionStr);
+            using SqlConnection conn = DataTestUtility.GetSqlConnection(connectionStr);


This change shouldn't be required.

Suggested change

using SqlConnection conn = DataTestUtility.GetSqlConnection(connectionStr);

using SqlConnection conn = new(connectionStr);

DavoudEshtehari · 2024-07-18T18:48:07Z

...SqlClient/tests/ManualTests/SQL/IntegratedAuthenticationTest/IntegratedAuthenticationTest.cs

@@ -58,7 +58,7 @@ public static void IntegratedAuthenticationTest_ServerSPN()

        private static void TryOpenConnectionWithIntegratedAuthentication(string connectionString)
        {
-            using (SqlConnection connection = new SqlConnection(connectionString))
+            using (SqlConnection connection = DataTestUtility.GetSqlConnection(connectionString))


Integrated auth doesn't required access token.

Suggested change

using (SqlConnection connection = DataTestUtility.GetSqlConnection(connectionString))

using (SqlConnection connection = new SqlConnection(connectionString))

DavoudEshtehari · 2024-07-18T18:49:44Z

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/InstanceNameTest/InstanceNameTest.cs

This class skips Azure and could be reverted.

DavoudEshtehari · 2024-07-18T19:33:07Z

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ParameterTest/TvpTest.cs

@@ -43,7 +43,8 @@ public class TvpTest
        private readonly string _connStr;

        // Synapse: The statement failed. Column 'blob' has a data type that cannot participate in a columnstore index.
-        [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup), nameof(DataTestUtility.IsNotAzureSynapse))]
+        // TODO: Investigate access token authentication to Azure SQL DB (#31362)
+        [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup), nameof(DataTestUtility.IsNotAzureSynapse), nameof(DataTestUtility.AuthenticatingWithoutAccessToken))]


Could you revert all the TVPTest related changes under the ParameterTest folder since they've already skipped on Mac?

David-Engel · 2024-07-22T19:36:21Z

This seems like it would make it difficult to change test connection strings due to validation on the AccessToken property. What problem is this PR solving?

Since assigning a UAMI directly to an agent pool is not recommended + our Mac agent pool, we are using a service connection in ADO and have federated credentials to it on a UAMI which has access to the database. As a result, we have an Azure Cli step in ADO to retrieve the token and assign it to the connection to authenticate.

@dauinsight

Can we simply use Authentication=ActiveDirectoryDefault for the connection string on the mac jobs and take advantage of the fact that AzureCliCredential is part of the credential chain?

I feel like requiring all tests to wrap their connection strings in DataTestUtility.GetSqlConnection is adding unnecessary complexity to test configuration.

If AD Default doesn't work, another alternative that would be much simpler/cleaner is to create a custom authentication provider that overrode the ActiveDirectoryMSI option to return the access token configured on the job.

codecov · 2024-07-22T21:24:59Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.63%. Comparing base (f7ab115) to head (db38ffb).
Report is 29 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2654      +/-   ##
==========================================
- Coverage   72.81%   71.63%   -1.19%     
==========================================
  Files         311      308       -3     
  Lines       61694    61532     -162     
==========================================
- Hits        44922    44077     -845     
- Misses      16772    17455     +683

Flag	Coverage Δ
addons	`92.88% <ø> (ø)`
netcore	`75.94% <ø> (-1.25%)`	⬇️
netfx	`69.62% <ø> (-0.82%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dauinsight · 2024-07-22T21:53:44Z

This seems like it would make it difficult to change test connection strings due to validation on the AccessToken property. What problem is this PR solving?

Since assigning a UAMI directly to an agent pool is not recommended + our Mac agent pool, we are using a service connection in ADO and have federated credentials to it on a UAMI which has access to the database. As a result, we have an Azure Cli step in ADO to retrieve the token and assign it to the connection to authenticate.

@dauinsight

Can we simply use Authentication=ActiveDirectoryDefault for the connection string on the mac jobs and take advantage of the fact that AzureCliCredential is part of the credential chain?

I feel like requiring all tests to wrap their connection strings in DataTestUtility.GetSqlConnection is adding unnecessary complexity to test configuration.

If AD Default doesn't work, another alternative that would be much simpler/cleaner is to create a custom authentication provider that overrode the ActiveDirectoryMSI option to return the access token configured on the job.

@David-Engel I agree that this adds a little bit of complexity, but i haven't found another way to use our service connection in ADO to grant MI to the agent using an access token.

ActiveDirectoryDefault works locally if we login to our machine via az login but not in the pipeline.

For more context into the implementation of this PR: https://msazure.visualstudio.com/One/_wiki/wikis/Microsoft%20Graph%20Engineering/675144/SFI-MI?anchor=mi-for-e2es-in-dev-environment-and-azure-devops(ado)

David-Engel · 2024-07-22T22:56:50Z

@dauinsight

For more context into the implementation of this PR: https://msazure.visualstudio.com/One/_wiki/wikis/Microsoft%20Graph%20Engineering/675144/SFI-MI?anchor=mi-for-e2es-in-dev-environment-and-azure-devops(ado)

After reading that, in addition to the custom provider option I mentioned, prioritizing #2557 could also provide a better solution that could be used. As it is, I would NOT merge this PR. Dynamically modifying the connection string like that across all tests is just asking for maintenance headaches and added cost.

Additionally, I noticed the AZURE_DB_SP_TCP_CONN_STRING was removed from the test config variable group. That could have been used for macOS, too. We have an exception for that for continuing to test AD SP auth.

CC: @saurabh500 @cheenamalhotra

saurabh500 · 2024-07-22T23:02:27Z

I strongly agree with @David-Engel
Unintentional modification of the connection string puts too many hinderances in test enhancements.

dauinsight · 2024-07-23T02:37:43Z

Okay that makes sense, for now i'll look to have the service principal set up again.

dauinsight added 2 commits July 8, 2024 14:01

Add Access Token to config

d14d911

Replace SqlConnection() with GetSqlConnection()

f21ebce

dauinsight added the Area\Tests Issues that are targeted to tests or test projects label Jul 8, 2024

dauinsight added 2 commits July 8, 2024 16:11

Replace SqlConnection() with GetSqlConnection()

92a7c1b

Add default value to GetSqlConnection()

980a3bc

dauinsight and others added 23 commits July 9, 2024 10:32

Add Access Token if exists

32c1de7

Fix syntax

34af174

Update GetSqlConnection

2692ab6

Debug

adcf791

Write connection string

07eb9e7

Try remove RemoteExecutor

1c8ade1

Test remove diagnostics

724566c

Use new sqlconnection()

a1f0a09

Add connection string

a1c73ed

Use SqlConnection() in DiagnosticTest

9c5b021

Fix tests

1e63295

Merge branch 'main' into dev/daniel/azure-assign-access-token

feac3cf

Configure yaml pipeline

fb2764f

Fix spacing

e0def57

Set variable

da39ac2

Added Console.WriterLines in TestInitialCatalogStandardValues to see …

394efc0

…where NRE comes from.

Merge branch 'dev/daniel/azure-assign-access-token' of https://github…

13eaf16

….com/dauinsight/SqlClient into dev/daniel/azure-assign-access-token

Update config yaml

33a1230

Merge branch 'dev/daniel/azure-assign-access-token' of https://github…

7fad98c

….com/dauinsight/SqlClient into dev/daniel/azure-assign-access-token

Iterate property descriptor of sql connection string builder.

c604f45

Merge branch 'dev/daniel/azure-assign-access-token' of https://github…

a28e8fb

….com/dauinsight/SqlClient into dev/daniel/azure-assign-access-token

Try not using helper to create connection.

5c39d19

Update yaml connection string

260e4bb

Update authenticating with access token condition

16649e0

dauinsight requested review from JRahnama, DavoudEshtehari and arellegue July 16, 2024 23:23

arellegue approved these changes Jul 17, 2024

View reviewed changes

DavoudEshtehari reviewed Jul 17, 2024

View reviewed changes

Update condition and readme

7fb9121

DavoudEshtehari self-requested a review July 17, 2024 23:22

DavoudEshtehari reviewed Jul 17, 2024

View reviewed changes

Address PR comments

5505f97

DavoudEshtehari reviewed Jul 18, 2024

View reviewed changes

dauinsight added 12 commits July 19, 2024 09:11

PR comments

dca089f

Update yaml config

1355417

Address new test failures

8a9b8b6

Revert UserID cred key to be removed

e1dd58e

Find tests not using TCP connection string

96766dd

Find tests that contain auth

aae353a

Find non azure tests

52fa34c

Test azure tests

0159d8e

Revert Non azure tests

0e83f68

Debug ProcessCommandBatch

e76e0f9

Check result after

75ade72

Remove console writeline

db38ffb

dauinsight closed this Jul 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Access Token Support for SqlConnection in Manual Tests #2654

Access Token Support for SqlConnection in Manual Tests #2654

dauinsight commented Jul 8, 2024 •

edited

Loading

David-Engel commented Jul 9, 2024

dauinsight commented Jul 9, 2024

arellegue left a comment

DavoudEshtehari Jul 17, 2024

DavoudEshtehari Jul 17, 2024 •

edited

Loading

DavoudEshtehari Jul 17, 2024

DavoudEshtehari Jul 17, 2024

DavoudEshtehari Jul 17, 2024

DavoudEshtehari Jul 17, 2024

DavoudEshtehari Jul 17, 2024

DavoudEshtehari Jul 17, 2024

DavoudEshtehari Jul 18, 2024

DavoudEshtehari Jul 18, 2024

DavoudEshtehari Jul 18, 2024

DavoudEshtehari Jul 18, 2024

DavoudEshtehari Jul 18, 2024

DavoudEshtehari Jul 18, 2024

David-Engel commented Jul 22, 2024

codecov bot commented Jul 22, 2024

dauinsight commented Jul 22, 2024

David-Engel commented Jul 22, 2024

saurabh500 commented Jul 22, 2024

dauinsight commented Jul 23, 2024

	// Skip: Access token authentication does not work with RemoteExecutor (#31362)
	// Skip: Access token authentication does not work with RemoteExecutor (active issue #31362)

		Console.WriteLine($"TCPConnectionString = {DataTestUtility.TCPConnectionString}");

	SqlConnectionStringBuilder builder = new SqlConnectionStringBuilder(DataTestUtility.TCPConnectionString);
	SqlConnectionStringBuilder builder = new SqlConnectionStringBuilder(connection.ConnectionString);

		// Skip: We cannot assign an access token to the connection in this test (#31362)
		[ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup), nameof(DataTestUtility.AuthenticatingWithoutAccessToken))]

	using var connection = DataTestUtility.GetSqlConnection(s_badConnectionString);
	using var connection = new SqlConnection(s_badConnectionString);

	using SqlConnection connection = DataTestUtility.GetSqlConnection(connString);
	using SqlConnection connection = new(connString);

	using SqlConnection conn = DataTestUtility.GetSqlConnection(connectionStr);
	using SqlConnection conn = new(connectionStr);

	using (SqlConnection connection = DataTestUtility.GetSqlConnection(connectionString))
	using (SqlConnection connection = new SqlConnection(connectionString))

Access Token Support for SqlConnection in Manual Tests #2654

Access Token Support for SqlConnection in Manual Tests #2654

Conversation

dauinsight commented Jul 8, 2024 • edited Loading

David-Engel commented Jul 9, 2024

dauinsight commented Jul 9, 2024

arellegue left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavoudEshtehari Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

David-Engel commented Jul 22, 2024

codecov bot commented Jul 22, 2024

Codecov Report

dauinsight commented Jul 22, 2024

David-Engel commented Jul 22, 2024

saurabh500 commented Jul 22, 2024

dauinsight commented Jul 23, 2024

dauinsight commented Jul 8, 2024 •

edited

Loading

DavoudEshtehari Jul 17, 2024 •

edited

Loading