From 708228e3ff29a89829ad28e7f63f0607deaabd83 Mon Sep 17 00:00:00 2001 From: Jun Su Date: Thu, 27 Oct 2022 21:25:55 +0800 Subject: [PATCH 01/13] Support authenticate against Azure MySQL service with AAD token --- src/Service.Tests/SqlTests/SqlTestBase.cs | 4 +- src/Service/Resolvers/MySqlQueryExecutor.cs | 135 ++++++++++++++++++++ src/Service/Startup.cs | 2 +- 3 files changed, 138 insertions(+), 3 deletions(-) create mode 100644 src/Service/Resolvers/MySqlQueryExecutor.cs diff --git a/src/Service.Tests/SqlTests/SqlTestBase.cs b/src/Service.Tests/SqlTests/SqlTestBase.cs index d066b22d3b..3579007188 100644 --- a/src/Service.Tests/SqlTests/SqlTestBase.cs +++ b/src/Service.Tests/SqlTests/SqlTestBase.cs @@ -261,11 +261,11 @@ protected static void SetUpSQLMetadataProvider() _sqlMetadataLogger); break; case TestCategory.MYSQL: - Mock>> mySqlQueryExecutorLogger = new(); + Mock> mySqlQueryExecutorLogger = new(); _queryBuilder = new MySqlQueryBuilder(); _defaultSchemaName = "mysql"; _dbExceptionParser = new MySqlDbExceptionParser(_runtimeConfigProvider); - _queryExecutor = new QueryExecutor( + _queryExecutor = new MySqlQueryExecutor( _runtimeConfigProvider, _dbExceptionParser, mySqlQueryExecutorLogger.Object); diff --git a/src/Service/Resolvers/MySqlQueryExecutor.cs b/src/Service/Resolvers/MySqlQueryExecutor.cs new file mode 100644 index 0000000000..76447fbbe0 --- /dev/null +++ b/src/Service/Resolvers/MySqlQueryExecutor.cs @@ -0,0 +1,135 @@ +using System.Data.Common; +using System.Threading.Tasks; +using Azure.Core; +using Azure.DataApiBuilder.Service.Configurations; +using Azure.Identity; +using Microsoft.Data.SqlClient; +using Microsoft.Extensions.Logging; +using MySqlConnector; + +namespace Azure.DataApiBuilder.Service.Resolvers +{ + /// + /// Specialized QueryExecutor for MsSql mainly providing methods to + /// handle connecting to the database with a managed identity. + /// /// + public class MySqlQueryExecutor : QueryExecutor + { + // This is the same scope for any Azure SQL database that is + // required to request a default azure credential access token + // for a managed identity. + public const string DATABASE_SCOPE = @"https://database.windows.net/.default"; + + /// + /// The managed identity Access Token string obtained + /// from the configuration controller. + /// + private readonly string? _accessTokenFromController; + + public DefaultAzureCredential AzureCredential { get; set; } = new(); + + /// + /// The saved cached access token obtained from DefaultAzureCredentials + /// representing a managed identity. + /// + private AccessToken? _defaultAccessToken; + + private bool _attemptToSetAccessToken; + + public MySqlQueryExecutor( + RuntimeConfigProvider runtimeConfigProvider, + DbExceptionParser dbExceptionParser, + ILogger> logger) + : base(runtimeConfigProvider, dbExceptionParser, logger) + { + _accessTokenFromController = runtimeConfigProvider.ManagedIdentityAccessToken; + _attemptToSetAccessToken = + ShouldManagedIdentityAccessBeAttempted(runtimeConfigProvider.GetRuntimeConfiguration().ConnectionString); + } + + /// + /// Modifies the properties of the supplied connection to support managed identity access. + /// In the case of MsSql, gets access token if deemed necessary and sets it on the connection. + /// The supplied connection is assumed to already have the same connection string + /// provided in the runtime configuration. + /// + /// The supplied connection to modify for managed identity access. + public override async Task SetManagedIdentityAccessTokenIfAnyAsync(DbConnection conn) + { + // Only attempt to get the access token if the connection string is in the appropriate format + if (_attemptToSetAccessToken) + { + //MySqlConnection sqlConn = (MySqlConnection)conn; + + // If the configuration controller provided a managed identity access token use that, + // else use the default saved access token if still valid. + // Get a new token only if the saved token is null or expired. + string? accessToken = _accessTokenFromController ?? + (IsDefaultAccessTokenValid() ? + ((AccessToken)_defaultAccessToken!).Token : + await GetAccessTokenAsync()); + + if (accessToken is not null) + { + MySqlConnectionStringBuilder connstr = new(conn.ConnectionString) + { + Password = accessToken + }; + conn.ConnectionString = connstr.ConnectionString; + } + } + } + + /// + /// Determines if managed identity access should be attempted or not. + /// It should only be attempted, + /// 1. If none of UserID, Password or Authentication + /// method are specified in the connection string since they have higher precedence + /// and any attempt to use an access token in their presence would lead to + /// a System.InvalidOperationException. + /// 2. It is NOT a Windows Integrated Security scenario. + /// + private static bool ShouldManagedIdentityAccessBeAttempted(string connString) + { + SqlConnectionStringBuilder connStringBuilder = new(connString); + return string.IsNullOrEmpty(connStringBuilder.UserID) && + string.IsNullOrEmpty(connStringBuilder.Password) && + connStringBuilder.Authentication == SqlAuthenticationMethod.NotSpecified && + !connStringBuilder.IntegratedSecurity; + } + + /// + /// Determines if the saved default azure credential's access token is valid and not expired. + /// + /// True if valid, false otherwise. + private bool IsDefaultAccessTokenValid() + { + return _defaultAccessToken is not null && + ((AccessToken)_defaultAccessToken).ExpiresOn.CompareTo(System.DateTimeOffset.Now) > 0; + } + + /// + /// Tries to get an access token using DefaultAzureCredentials. + /// Catches any CredentialUnavailableException and logs only a warning + /// since since this is best effort. + /// + /// The string representation of the access token if found, + /// null otherwise. + private async Task GetAccessTokenAsync() + { + try + { + _defaultAccessToken = + await AzureCredential.GetTokenAsync( + new TokenRequestContext(new[] { DATABASE_SCOPE })); + } + catch (CredentialUnavailableException ex) + { + QueryExecutorLogger.LogWarning($"Attempt to retrieve a managed identity access token using DefaultAzureCredential" + + $" failed due to: \n{ex}"); + } + + return _defaultAccessToken?.Token; + } + } +} diff --git a/src/Service/Startup.cs b/src/Service/Startup.cs index 5ea8f012f3..9590c0001b 100644 --- a/src/Service/Startup.cs +++ b/src/Service/Startup.cs @@ -110,7 +110,7 @@ public void ConfigureServices(IServiceCollection services) case DatabaseType.postgresql: return ActivatorUtilities.GetServiceOrCreateInstance>(serviceProvider); case DatabaseType.mysql: - return ActivatorUtilities.GetServiceOrCreateInstance>(serviceProvider); + return ActivatorUtilities.GetServiceOrCreateInstance(serviceProvider); default: throw new NotSupportedException( runtimeConfig.DatabaseTypeNotSupportedMessage); From 549bfd3b1def483e3b517b46b411d5f45f81c6ac Mon Sep 17 00:00:00 2001 From: Jun Su Date: Fri, 28 Oct 2022 10:26:02 +0800 Subject: [PATCH 02/13] Fix the namespace for MySQL AAD --- src/Service/Resolvers/MySqlQueryExecutor.cs | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/Service/Resolvers/MySqlQueryExecutor.cs b/src/Service/Resolvers/MySqlQueryExecutor.cs index 76447fbbe0..18a53825a1 100644 --- a/src/Service/Resolvers/MySqlQueryExecutor.cs +++ b/src/Service/Resolvers/MySqlQueryExecutor.cs @@ -3,7 +3,6 @@ using Azure.Core; using Azure.DataApiBuilder.Service.Configurations; using Azure.Identity; -using Microsoft.Data.SqlClient; using Microsoft.Extensions.Logging; using MySqlConnector; @@ -18,7 +17,7 @@ public class MySqlQueryExecutor : QueryExecutor // This is the same scope for any Azure SQL database that is // required to request a default azure credential access token // for a managed identity. - public const string DATABASE_SCOPE = @"https://database.windows.net/.default"; + public const string DATABASE_SCOPE = @"https://ossrdbms-aad.database.windows.net/.default"; /// /// The managed identity Access Token string obtained @@ -91,11 +90,9 @@ public override async Task SetManagedIdentityAccessTokenIfAnyAsync(DbConnection /// private static bool ShouldManagedIdentityAccessBeAttempted(string connString) { - SqlConnectionStringBuilder connStringBuilder = new(connString); - return string.IsNullOrEmpty(connStringBuilder.UserID) && - string.IsNullOrEmpty(connStringBuilder.Password) && - connStringBuilder.Authentication == SqlAuthenticationMethod.NotSpecified && - !connStringBuilder.IntegratedSecurity; + MySqlConnectionStringBuilder connStringBuilder = new(connString); + return !string.IsNullOrEmpty(connStringBuilder.UserID) && + string.IsNullOrEmpty(connStringBuilder.Password); } /// From c5d57c29aa34a6bd371db7e38251f87084b6a877 Mon Sep 17 00:00:00 2001 From: Jun Su Date: Fri, 28 Oct 2022 10:42:05 +0800 Subject: [PATCH 03/13] Add unittest for mysqlqueryexecutor --- .../Unittests/MySqlQueryExecutorUnitTests.cs | 184 ++++++++++++++++++ 1 file changed, 184 insertions(+) create mode 100644 src/Service.Tests/Unittests/MySqlQueryExecutorUnitTests.cs diff --git a/src/Service.Tests/Unittests/MySqlQueryExecutorUnitTests.cs b/src/Service.Tests/Unittests/MySqlQueryExecutorUnitTests.cs new file mode 100644 index 0000000000..9689d40714 --- /dev/null +++ b/src/Service.Tests/Unittests/MySqlQueryExecutorUnitTests.cs @@ -0,0 +1,184 @@ +using System; +using System.Collections.Generic; +using System.Data.Common; +using System.Net; +using System.Text.Json; +using System.Threading.Tasks; +using Azure.Core; +using Azure.DataApiBuilder.Service.Configurations; +using Azure.DataApiBuilder.Service.Exceptions; +using Azure.DataApiBuilder.Service.Resolvers; +using Azure.DataApiBuilder.Service.Tests.SqlTests; +using Azure.Identity; +using Microsoft.Extensions.Logging; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using Moq; +using MySqlConnector; + +namespace Azure.DataApiBuilder.Service.Tests.UnitTests +{ + [TestClass, TestCategory(TestCategory.MYSQL)] + public class MySqlQueryExecutorUnitTests + { + // Error code for semaphore timeout in MsSql. + private const int ERRORCODE_SEMAPHORE_TIMEOUT = 121; + /// + /// Validates managed identity token issued ONLY when connection string does not specify + /// User, Password, and Authentication method. + /// + [DataTestMethod] + [DataRow("Server =<>;Database=<>;User=xyz;Password=xxx", false, false, + DisplayName = "No managed identity access token when connection string specifies both User and Password.")] + [DataRow("Server =<>;Database=<>;User=xyz;Password=xxx;", false, false, + DisplayName = "No managed identity access token when connection string specifies User, Password.")] + [DataRow("Server =<>;Database=<>;", true, false, + DisplayName = "Managed identity access token from config used when connection string specifies none of User, Password.")] + [DataRow("Server =<>;Database=<>;", true, true, + DisplayName = "Default managed identity access token used when connection string specifies none of User, Password.")] + public async Task TestHandleManagedIdentityAccess( + string connectionString, + bool expectManagedIdentityAccessToken, + bool isDefaultAzureCredential) + { + RuntimeConfigProvider runtimeConfigProvider = TestHelper.GetRuntimeConfigProvider(TestCategory.MSSQL); + runtimeConfigProvider.GetRuntimeConfiguration().ConnectionString = connectionString; + Mock dbExceptionParser = new(runtimeConfigProvider, new HashSet()); + Mock> queryExecutorLogger = new(); + MySqlQueryExecutor msSqlQueryExecutor = new(runtimeConfigProvider, dbExceptionParser.Object, queryExecutorLogger.Object); + + const string DEFAULT_TOKEN = "Default access token"; + const string CONFIG_TOKEN = "Configuration controller access token"; + AccessToken testValidToken = new(accessToken: DEFAULT_TOKEN, expiresOn: DateTimeOffset.MaxValue); + if (expectManagedIdentityAccessToken) + { + if (isDefaultAzureCredential) + { + Mock dacMock = new(); + dacMock + .Setup(m => m.GetTokenAsync(It.IsAny(), + It.IsAny())) + .Returns(ValueTask.FromResult(testValidToken)); + msSqlQueryExecutor.AzureCredential = dacMock.Object; + } + else + { + runtimeConfigProvider.Initialize( + JsonSerializer.Serialize(runtimeConfigProvider.GetRuntimeConfiguration()), + schema: null, + connectionString: connectionString, + accessToken: CONFIG_TOKEN); + msSqlQueryExecutor = new(runtimeConfigProvider, dbExceptionParser.Object, queryExecutorLogger.Object); + } + } + + using MySqlConnection conn = new(connectionString); + await msSqlQueryExecutor.SetManagedIdentityAccessTokenIfAnyAsync(conn); + MySqlConnectionStringBuilder my = new(conn.ConnectionString); + + if (expectManagedIdentityAccessToken) + { + if (isDefaultAzureCredential) + { + Assert.AreEqual(expected: DEFAULT_TOKEN, actual: my.Password); + } + else + { + Assert.AreEqual(expected: CONFIG_TOKEN, actual: my.Password); + } + } + else + { + Assert.AreEqual(expected: "xxx", actual: my.Password); + } + } + + /// + /// Test to validate that when a query successfully executes within the allowed number of retries, a result is returned + /// and no further retries occur. + /// + [TestMethod, TestCategory(TestCategory.MYSQL)] + public async Task TestRetryPolicyExhaustingMaxAttempts() + { + int maxRetries = 5; + int maxAttempts = maxRetries + 1; // 1 represents the original attempt to execute the query in addition to retries. + RuntimeConfigProvider runtimeConfigProvider = TestHelper.GetRuntimeConfigProvider(TestCategory.MYSQL); + Mock>> queryExecutorLogger = new(); + DbExceptionParser dbExceptionParser = new MySqlDbExceptionParser(runtimeConfigProvider); + Mock queryExecutor = new(runtimeConfigProvider, dbExceptionParser, queryExecutorLogger.Object); + + // Mock the ExecuteQueryAgainstDbAsync to throw a transient exception. + queryExecutor.Setup(x => x.ExecuteQueryAgainstDbAsync( + It.IsAny(), + It.IsAny(), + It.IsAny>(), + It.IsAny, Task>>(), + It.IsAny>())) + .Throws(SqlTestHelper.CreateSqlException(ERRORCODE_SEMAPHORE_TIMEOUT)); + + // Call the actual ExecuteQueryAsync method. + queryExecutor.Setup(x => x.ExecuteQueryAsync( + It.IsAny(), + It.IsAny>(), + It.IsAny, Task>>(), + It.IsAny>())).CallBase(); + + DataApiBuilderException ex = await Assert.ThrowsExceptionAsync(async () => + { + await queryExecutor.Object.ExecuteQueryAsync( + sqltext: string.Empty, + parameters: new Dictionary(), + dataReaderHandler: null, + args: null); + }); + + Assert.AreEqual(HttpStatusCode.InternalServerError, ex.StatusCode); + + // For each attempt logger is invoked twice. Currently we have hardcoded the number of attempts. + // Once we have number of retry attempts specified in config, we will make it dynamic. + Assert.AreEqual(2 * maxAttempts, queryExecutorLogger.Invocations.Count); + } + + /// + /// Test to validate that when a query succcessfully executes within allowed number of retries, we get back the result + /// without giving anymore retries. + /// + [TestMethod, TestCategory(TestCategory.MSSQL)] + public async Task TestRetryPolicySuccessfullyExecutingQueryAfterNAttempts() + { + RuntimeConfigProvider runtimeConfigProvider = TestHelper.GetRuntimeConfigProvider(TestCategory.MSSQL); + Mock>> queryExecutorLogger = new(); + DbExceptionParser dbExceptionParser = new MsSqlDbExceptionParser(runtimeConfigProvider); + Mock queryExecutor = new(runtimeConfigProvider, dbExceptionParser, queryExecutorLogger.Object); + + // Mock the ExecuteQueryAgainstDbAsync to throw a transient exception. + queryExecutor.SetupSequence(x => x.ExecuteQueryAgainstDbAsync( + It.IsAny(), + It.IsAny(), + It.IsAny>(), + It.IsAny, Task>>(), + It.IsAny>())) + .Throws(SqlTestHelper.CreateSqlException(ERRORCODE_SEMAPHORE_TIMEOUT)) + .Throws(SqlTestHelper.CreateSqlException(ERRORCODE_SEMAPHORE_TIMEOUT)) + .CallBase(); + + // Call the actual ExecuteQueryAsync method. + queryExecutor.Setup(x => x.ExecuteQueryAsync( + It.IsAny(), + It.IsAny>(), + It.IsAny, Task>>(), + It.IsAny>())).CallBase(); + + string sqltext = "SELECT * from books"; + + await queryExecutor.Object.ExecuteQueryAsync( + sqltext: sqltext, + parameters: new Dictionary(), + dataReaderHandler: null, + args: null); + + // For each attempt logger is invoked twice. The query executes successfully in in 1st retry .i.e. 2nd attempt of execution. + // An additional information log is added when the query executes successfully in a retry attempt. + Assert.AreEqual(2 * 2 + 1, queryExecutorLogger.Invocations.Count); + } + } +} From cd3144d0c04342f559014e18b3a3e1a85b78ef2b Mon Sep 17 00:00:00 2001 From: Jun Su Date: Fri, 28 Oct 2022 10:46:19 +0800 Subject: [PATCH 04/13] Fix per unittest --- .../Unittests/MySqlQueryExecutorUnitTests.cs | 44 +++++++++---------- src/Service/Resolvers/MySqlQueryExecutor.cs | 4 +- 2 files changed, 23 insertions(+), 25 deletions(-) diff --git a/src/Service.Tests/Unittests/MySqlQueryExecutorUnitTests.cs b/src/Service.Tests/Unittests/MySqlQueryExecutorUnitTests.cs index 9689d40714..663c8f4f56 100644 --- a/src/Service.Tests/Unittests/MySqlQueryExecutorUnitTests.cs +++ b/src/Service.Tests/Unittests/MySqlQueryExecutorUnitTests.cs @@ -20,8 +20,6 @@ namespace Azure.DataApiBuilder.Service.Tests.UnitTests [TestClass, TestCategory(TestCategory.MYSQL)] public class MySqlQueryExecutorUnitTests { - // Error code for semaphore timeout in MsSql. - private const int ERRORCODE_SEMAPHORE_TIMEOUT = 121; /// /// Validates managed identity token issued ONLY when connection string does not specify /// User, Password, and Authentication method. @@ -40,11 +38,11 @@ public async Task TestHandleManagedIdentityAccess( bool expectManagedIdentityAccessToken, bool isDefaultAzureCredential) { - RuntimeConfigProvider runtimeConfigProvider = TestHelper.GetRuntimeConfigProvider(TestCategory.MSSQL); + RuntimeConfigProvider runtimeConfigProvider = TestHelper.GetRuntimeConfigProvider(TestCategory.MYSQL); runtimeConfigProvider.GetRuntimeConfiguration().ConnectionString = connectionString; Mock dbExceptionParser = new(runtimeConfigProvider, new HashSet()); Mock> queryExecutorLogger = new(); - MySqlQueryExecutor msSqlQueryExecutor = new(runtimeConfigProvider, dbExceptionParser.Object, queryExecutorLogger.Object); + MySqlQueryExecutor mySqlQueryExecutor = new(runtimeConfigProvider, dbExceptionParser.Object, queryExecutorLogger.Object); const string DEFAULT_TOKEN = "Default access token"; const string CONFIG_TOKEN = "Configuration controller access token"; @@ -58,7 +56,7 @@ public async Task TestHandleManagedIdentityAccess( .Setup(m => m.GetTokenAsync(It.IsAny(), It.IsAny())) .Returns(ValueTask.FromResult(testValidToken)); - msSqlQueryExecutor.AzureCredential = dacMock.Object; + mySqlQueryExecutor.AzureCredential = dacMock.Object; } else { @@ -67,12 +65,12 @@ public async Task TestHandleManagedIdentityAccess( schema: null, connectionString: connectionString, accessToken: CONFIG_TOKEN); - msSqlQueryExecutor = new(runtimeConfigProvider, dbExceptionParser.Object, queryExecutorLogger.Object); + mySqlQueryExecutor = new(runtimeConfigProvider, dbExceptionParser.Object, queryExecutorLogger.Object); } } using MySqlConnection conn = new(connectionString); - await msSqlQueryExecutor.SetManagedIdentityAccessTokenIfAnyAsync(conn); + await mySqlQueryExecutor.SetManagedIdentityAccessTokenIfAnyAsync(conn); MySqlConnectionStringBuilder my = new(conn.ConnectionString); if (expectManagedIdentityAccessToken) @@ -107,13 +105,13 @@ public async Task TestRetryPolicyExhaustingMaxAttempts() Mock queryExecutor = new(runtimeConfigProvider, dbExceptionParser, queryExecutorLogger.Object); // Mock the ExecuteQueryAgainstDbAsync to throw a transient exception. - queryExecutor.Setup(x => x.ExecuteQueryAgainstDbAsync( - It.IsAny(), - It.IsAny(), - It.IsAny>(), - It.IsAny, Task>>(), - It.IsAny>())) - .Throws(SqlTestHelper.CreateSqlException(ERRORCODE_SEMAPHORE_TIMEOUT)); + //queryExecutor.Setup(x => x.ExecuteQueryAgainstDbAsync( + // It.IsAny(), + // It.IsAny(), + // It.IsAny>(), + // It.IsAny, Task>>(), + // It.IsAny>())) + //.Throws(SqlTestHelper.CreateSqlException(ERRORCODE_SEMAPHORE_TIMEOUT)); // Call the actual ExecuteQueryAsync method. queryExecutor.Setup(x => x.ExecuteQueryAsync( @@ -151,15 +149,15 @@ public async Task TestRetryPolicySuccessfullyExecutingQueryAfterNAttempts() Mock queryExecutor = new(runtimeConfigProvider, dbExceptionParser, queryExecutorLogger.Object); // Mock the ExecuteQueryAgainstDbAsync to throw a transient exception. - queryExecutor.SetupSequence(x => x.ExecuteQueryAgainstDbAsync( - It.IsAny(), - It.IsAny(), - It.IsAny>(), - It.IsAny, Task>>(), - It.IsAny>())) - .Throws(SqlTestHelper.CreateSqlException(ERRORCODE_SEMAPHORE_TIMEOUT)) - .Throws(SqlTestHelper.CreateSqlException(ERRORCODE_SEMAPHORE_TIMEOUT)) - .CallBase(); + //queryExecutor.SetupSequence(x => x.ExecuteQueryAgainstDbAsync( + // It.IsAny(), + // It.IsAny(), + // It.IsAny>(), + // It.IsAny, Task>>(), + // It.IsAny>())) + //.Throws(SqlTestHelper.CreateSqlException(ERRORCODE_SEMAPHORE_TIMEOUT)) + //.Throws(SqlTestHelper.CreateSqlException(ERRORCODE_SEMAPHORE_TIMEOUT)) + //.CallBase(); // Call the actual ExecuteQueryAsync method. queryExecutor.Setup(x => x.ExecuteQueryAsync( diff --git a/src/Service/Resolvers/MySqlQueryExecutor.cs b/src/Service/Resolvers/MySqlQueryExecutor.cs index 18a53825a1..683601bfef 100644 --- a/src/Service/Resolvers/MySqlQueryExecutor.cs +++ b/src/Service/Resolvers/MySqlQueryExecutor.cs @@ -9,7 +9,7 @@ namespace Azure.DataApiBuilder.Service.Resolvers { /// - /// Specialized QueryExecutor for MsSql mainly providing methods to + /// Specialized QueryExecutor for MySql mainly providing methods to /// handle connecting to the database with a managed identity. /// /// public class MySqlQueryExecutor : QueryExecutor @@ -48,7 +48,7 @@ public MySqlQueryExecutor( /// /// Modifies the properties of the supplied connection to support managed identity access. - /// In the case of MsSql, gets access token if deemed necessary and sets it on the connection. + /// In the case of MySql, gets access token if deemed necessary and sets it on the connection. /// The supplied connection is assumed to already have the same connection string /// provided in the runtime configuration. /// From bef6d9c8b5730fa6df82c1a3db5dcdcb5253bd2f Mon Sep 17 00:00:00 2001 From: Jun Su Date: Fri, 28 Oct 2022 11:12:09 +0800 Subject: [PATCH 05/13] Fix test --- src/Service.Tests/Unittests/MySqlQueryExecutorUnitTests.cs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/Service.Tests/Unittests/MySqlQueryExecutorUnitTests.cs b/src/Service.Tests/Unittests/MySqlQueryExecutorUnitTests.cs index 663c8f4f56..f12363834d 100644 --- a/src/Service.Tests/Unittests/MySqlQueryExecutorUnitTests.cs +++ b/src/Service.Tests/Unittests/MySqlQueryExecutorUnitTests.cs @@ -8,7 +8,6 @@ using Azure.DataApiBuilder.Service.Configurations; using Azure.DataApiBuilder.Service.Exceptions; using Azure.DataApiBuilder.Service.Resolvers; -using Azure.DataApiBuilder.Service.Tests.SqlTests; using Azure.Identity; using Microsoft.Extensions.Logging; using Microsoft.VisualStudio.TestTools.UnitTesting; @@ -140,12 +139,12 @@ await queryExecutor.Object.ExecuteQueryAsync( /// Test to validate that when a query succcessfully executes within allowed number of retries, we get back the result /// without giving anymore retries. /// - [TestMethod, TestCategory(TestCategory.MSSQL)] + [TestMethod, TestCategory(TestCategory.MYSQL)] public async Task TestRetryPolicySuccessfullyExecutingQueryAfterNAttempts() { - RuntimeConfigProvider runtimeConfigProvider = TestHelper.GetRuntimeConfigProvider(TestCategory.MSSQL); + RuntimeConfigProvider runtimeConfigProvider = TestHelper.GetRuntimeConfigProvider(TestCategory.MYSQL); Mock>> queryExecutorLogger = new(); - DbExceptionParser dbExceptionParser = new MsSqlDbExceptionParser(runtimeConfigProvider); + DbExceptionParser dbExceptionParser = new MySqlDbExceptionParser(runtimeConfigProvider); Mock queryExecutor = new(runtimeConfigProvider, dbExceptionParser, queryExecutorLogger.Object); // Mock the ExecuteQueryAgainstDbAsync to throw a transient exception. From d0d196b69fa2bf299f3e68c454919811f9b2cc83 Mon Sep 17 00:00:00 2001 From: Jun Su Date: Fri, 28 Oct 2022 11:24:57 +0800 Subject: [PATCH 06/13] Fix format --- src/Service/Startup.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Service/Startup.cs b/src/Service/Startup.cs index 9590c0001b..3bc63d53af 100644 --- a/src/Service/Startup.cs +++ b/src/Service/Startup.cs @@ -26,7 +26,6 @@ using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Hosting; using Microsoft.Extensions.Logging; -using MySqlConnector; using Npgsql; namespace Azure.DataApiBuilder.Service From fbdedae11a2ec3f2db2ff80fef0858684270304a Mon Sep 17 00:00:00 2001 From: Jun Su Date: Fri, 28 Oct 2022 11:40:44 +0800 Subject: [PATCH 07/13] fix unittest --- .../Unittests/MySqlQueryExecutorUnitTests.cs | 93 +------------------ 1 file changed, 2 insertions(+), 91 deletions(-) diff --git a/src/Service.Tests/Unittests/MySqlQueryExecutorUnitTests.cs b/src/Service.Tests/Unittests/MySqlQueryExecutorUnitTests.cs index f12363834d..04480e8ac8 100644 --- a/src/Service.Tests/Unittests/MySqlQueryExecutorUnitTests.cs +++ b/src/Service.Tests/Unittests/MySqlQueryExecutorUnitTests.cs @@ -28,9 +28,9 @@ public class MySqlQueryExecutorUnitTests DisplayName = "No managed identity access token when connection string specifies both User and Password.")] [DataRow("Server =<>;Database=<>;User=xyz;Password=xxx;", false, false, DisplayName = "No managed identity access token when connection string specifies User, Password.")] - [DataRow("Server =<>;Database=<>;", true, false, + [DataRow("Server =<>;Database=<>;User=xyz;", true, false, DisplayName = "Managed identity access token from config used when connection string specifies none of User, Password.")] - [DataRow("Server =<>;Database=<>;", true, true, + [DataRow("Server =<>;Database=<>;User=xyz;", true, true, DisplayName = "Default managed identity access token used when connection string specifies none of User, Password.")] public async Task TestHandleManagedIdentityAccess( string connectionString, @@ -88,94 +88,5 @@ public async Task TestHandleManagedIdentityAccess( Assert.AreEqual(expected: "xxx", actual: my.Password); } } - - /// - /// Test to validate that when a query successfully executes within the allowed number of retries, a result is returned - /// and no further retries occur. - /// - [TestMethod, TestCategory(TestCategory.MYSQL)] - public async Task TestRetryPolicyExhaustingMaxAttempts() - { - int maxRetries = 5; - int maxAttempts = maxRetries + 1; // 1 represents the original attempt to execute the query in addition to retries. - RuntimeConfigProvider runtimeConfigProvider = TestHelper.GetRuntimeConfigProvider(TestCategory.MYSQL); - Mock>> queryExecutorLogger = new(); - DbExceptionParser dbExceptionParser = new MySqlDbExceptionParser(runtimeConfigProvider); - Mock queryExecutor = new(runtimeConfigProvider, dbExceptionParser, queryExecutorLogger.Object); - - // Mock the ExecuteQueryAgainstDbAsync to throw a transient exception. - //queryExecutor.Setup(x => x.ExecuteQueryAgainstDbAsync( - // It.IsAny(), - // It.IsAny(), - // It.IsAny>(), - // It.IsAny, Task>>(), - // It.IsAny>())) - //.Throws(SqlTestHelper.CreateSqlException(ERRORCODE_SEMAPHORE_TIMEOUT)); - - // Call the actual ExecuteQueryAsync method. - queryExecutor.Setup(x => x.ExecuteQueryAsync( - It.IsAny(), - It.IsAny>(), - It.IsAny, Task>>(), - It.IsAny>())).CallBase(); - - DataApiBuilderException ex = await Assert.ThrowsExceptionAsync(async () => - { - await queryExecutor.Object.ExecuteQueryAsync( - sqltext: string.Empty, - parameters: new Dictionary(), - dataReaderHandler: null, - args: null); - }); - - Assert.AreEqual(HttpStatusCode.InternalServerError, ex.StatusCode); - - // For each attempt logger is invoked twice. Currently we have hardcoded the number of attempts. - // Once we have number of retry attempts specified in config, we will make it dynamic. - Assert.AreEqual(2 * maxAttempts, queryExecutorLogger.Invocations.Count); - } - - /// - /// Test to validate that when a query succcessfully executes within allowed number of retries, we get back the result - /// without giving anymore retries. - /// - [TestMethod, TestCategory(TestCategory.MYSQL)] - public async Task TestRetryPolicySuccessfullyExecutingQueryAfterNAttempts() - { - RuntimeConfigProvider runtimeConfigProvider = TestHelper.GetRuntimeConfigProvider(TestCategory.MYSQL); - Mock>> queryExecutorLogger = new(); - DbExceptionParser dbExceptionParser = new MySqlDbExceptionParser(runtimeConfigProvider); - Mock queryExecutor = new(runtimeConfigProvider, dbExceptionParser, queryExecutorLogger.Object); - - // Mock the ExecuteQueryAgainstDbAsync to throw a transient exception. - //queryExecutor.SetupSequence(x => x.ExecuteQueryAgainstDbAsync( - // It.IsAny(), - // It.IsAny(), - // It.IsAny>(), - // It.IsAny, Task>>(), - // It.IsAny>())) - //.Throws(SqlTestHelper.CreateSqlException(ERRORCODE_SEMAPHORE_TIMEOUT)) - //.Throws(SqlTestHelper.CreateSqlException(ERRORCODE_SEMAPHORE_TIMEOUT)) - //.CallBase(); - - // Call the actual ExecuteQueryAsync method. - queryExecutor.Setup(x => x.ExecuteQueryAsync( - It.IsAny(), - It.IsAny>(), - It.IsAny, Task>>(), - It.IsAny>())).CallBase(); - - string sqltext = "SELECT * from books"; - - await queryExecutor.Object.ExecuteQueryAsync( - sqltext: sqltext, - parameters: new Dictionary(), - dataReaderHandler: null, - args: null); - - // For each attempt logger is invoked twice. The query executes successfully in in 1st retry .i.e. 2nd attempt of execution. - // An additional information log is added when the query executes successfully in a retry attempt. - Assert.AreEqual(2 * 2 + 1, queryExecutorLogger.Invocations.Count); - } } } From 5752d3c924aa9c7c1d2768e86f08e23f0e3c5345 Mon Sep 17 00:00:00 2001 From: Jun Su Date: Fri, 28 Oct 2022 13:48:35 +0800 Subject: [PATCH 08/13] Remove unneed using --- src/Service.Tests/Unittests/MySqlQueryExecutorUnitTests.cs | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/Service.Tests/Unittests/MySqlQueryExecutorUnitTests.cs b/src/Service.Tests/Unittests/MySqlQueryExecutorUnitTests.cs index 04480e8ac8..9e60b905f1 100644 --- a/src/Service.Tests/Unittests/MySqlQueryExecutorUnitTests.cs +++ b/src/Service.Tests/Unittests/MySqlQueryExecutorUnitTests.cs @@ -1,12 +1,9 @@ using System; using System.Collections.Generic; -using System.Data.Common; -using System.Net; using System.Text.Json; using System.Threading.Tasks; using Azure.Core; using Azure.DataApiBuilder.Service.Configurations; -using Azure.DataApiBuilder.Service.Exceptions; using Azure.DataApiBuilder.Service.Resolvers; using Azure.Identity; using Microsoft.Extensions.Logging; From 373dd9848e1a3bd98b87b06d120250a9350aafac Mon Sep 17 00:00:00 2001 From: Jun Su Date: Thu, 1 Dec 2022 19:53:55 +0800 Subject: [PATCH 09/13] Update src/Service.Tests/Unittests/MySqlQueryExecutorUnitTests.cs Co-authored-by: Aniruddh Munde --- src/Service.Tests/Unittests/MySqlQueryExecutorUnitTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Service.Tests/Unittests/MySqlQueryExecutorUnitTests.cs b/src/Service.Tests/Unittests/MySqlQueryExecutorUnitTests.cs index 9e60b905f1..ee8923d389 100644 --- a/src/Service.Tests/Unittests/MySqlQueryExecutorUnitTests.cs +++ b/src/Service.Tests/Unittests/MySqlQueryExecutorUnitTests.cs @@ -26,7 +26,7 @@ public class MySqlQueryExecutorUnitTests [DataRow("Server =<>;Database=<>;User=xyz;Password=xxx;", false, false, DisplayName = "No managed identity access token when connection string specifies User, Password.")] [DataRow("Server =<>;Database=<>;User=xyz;", true, false, - DisplayName = "Managed identity access token from config used when connection string specifies none of User, Password.")] + DisplayName = "Managed identity access token from config used when connection string specifies User but not the Password.")] [DataRow("Server =<>;Database=<>;User=xyz;", true, true, DisplayName = "Default managed identity access token used when connection string specifies none of User, Password.")] public async Task TestHandleManagedIdentityAccess( From 23f7508f74a63645c4e9aa1972f53a6c57e04ac0 Mon Sep 17 00:00:00 2001 From: Jun Su Date: Thu, 1 Dec 2022 19:54:03 +0800 Subject: [PATCH 10/13] Update src/Service/Resolvers/MySqlQueryExecutor.cs Co-authored-by: aaronburtle <93220300+aaronburtle@users.noreply.github.com> --- src/Service/Resolvers/MySqlQueryExecutor.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Service/Resolvers/MySqlQueryExecutor.cs b/src/Service/Resolvers/MySqlQueryExecutor.cs index 683601bfef..717519a49a 100644 --- a/src/Service/Resolvers/MySqlQueryExecutor.cs +++ b/src/Service/Resolvers/MySqlQueryExecutor.cs @@ -108,7 +108,7 @@ private bool IsDefaultAccessTokenValid() /// /// Tries to get an access token using DefaultAzureCredentials. /// Catches any CredentialUnavailableException and logs only a warning - /// since since this is best effort. + /// since this is best effort. /// /// The string representation of the access token if found, /// null otherwise. From 88d7848a9986df5ad3f1f00dc65c86a83e15577c Mon Sep 17 00:00:00 2001 From: Jun Su Date: Thu, 1 Dec 2022 19:54:14 +0800 Subject: [PATCH 11/13] Update src/Service/Resolvers/MySqlQueryExecutor.cs Co-authored-by: Sean Leonard --- src/Service/Resolvers/MySqlQueryExecutor.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Service/Resolvers/MySqlQueryExecutor.cs b/src/Service/Resolvers/MySqlQueryExecutor.cs index 717519a49a..68070d2b5e 100644 --- a/src/Service/Resolvers/MySqlQueryExecutor.cs +++ b/src/Service/Resolvers/MySqlQueryExecutor.cs @@ -58,7 +58,6 @@ public override async Task SetManagedIdentityAccessTokenIfAnyAsync(DbConnection // Only attempt to get the access token if the connection string is in the appropriate format if (_attemptToSetAccessToken) { - //MySqlConnection sqlConn = (MySqlConnection)conn; // If the configuration controller provided a managed identity access token use that, // else use the default saved access token if still valid. From a43c305e08885291ee1874a3094b76bdf421e30c Mon Sep 17 00:00:00 2001 From: Jun Su Date: Thu, 1 Dec 2022 20:02:14 +0800 Subject: [PATCH 12/13] Remove duplicate test --- src/Service.Tests/Unittests/MySqlQueryExecutorUnitTests.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/Service.Tests/Unittests/MySqlQueryExecutorUnitTests.cs b/src/Service.Tests/Unittests/MySqlQueryExecutorUnitTests.cs index ee8923d389..44921fb836 100644 --- a/src/Service.Tests/Unittests/MySqlQueryExecutorUnitTests.cs +++ b/src/Service.Tests/Unittests/MySqlQueryExecutorUnitTests.cs @@ -27,8 +27,6 @@ public class MySqlQueryExecutorUnitTests DisplayName = "No managed identity access token when connection string specifies User, Password.")] [DataRow("Server =<>;Database=<>;User=xyz;", true, false, DisplayName = "Managed identity access token from config used when connection string specifies User but not the Password.")] - [DataRow("Server =<>;Database=<>;User=xyz;", true, true, - DisplayName = "Default managed identity access token used when connection string specifies none of User, Password.")] public async Task TestHandleManagedIdentityAccess( string connectionString, bool expectManagedIdentityAccessToken, From 7c6433a57a77d6a783fab24ada6de638139c14bb Mon Sep 17 00:00:00 2001 From: Jun Su Date: Fri, 2 Dec 2022 07:11:52 +0800 Subject: [PATCH 13/13] Add one more unittest --- src/Service.Tests/Unittests/MySqlQueryExecutorUnitTests.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/Service.Tests/Unittests/MySqlQueryExecutorUnitTests.cs b/src/Service.Tests/Unittests/MySqlQueryExecutorUnitTests.cs index 44921fb836..15b33d916e 100644 --- a/src/Service.Tests/Unittests/MySqlQueryExecutorUnitTests.cs +++ b/src/Service.Tests/Unittests/MySqlQueryExecutorUnitTests.cs @@ -27,6 +27,8 @@ public class MySqlQueryExecutorUnitTests DisplayName = "No managed identity access token when connection string specifies User, Password.")] [DataRow("Server =<>;Database=<>;User=xyz;", true, false, DisplayName = "Managed identity access token from config used when connection string specifies User but not the Password.")] + [DataRow("Server =<>;Database=<>;User=xyz;", true, true, + DisplayName = "Managed identity access token from Default Azure Credential used when connection string specifies User but not the Password.")] public async Task TestHandleManagedIdentityAccess( string connectionString, bool expectManagedIdentityAccessToken,