[SQL Health Check] Handle HTTP exception (#902)

* Modifying health checks to return the failure reason as string.

* Add HttpRequestException handling to Sql Health Check

* Created new health status reason.

* Test to validate new possible Health Check failure.

* Adding more logs to components used by schema manager.

* Addressing PR comments.

src/Microsoft.Health.Core/Features/Health/HealthStatusReason.cs

@@ -20,6 +20,7 @@ public enum HealthStatusReason
     /// Degraded status reasons, in order of most healthy to least healthy
     /// </summary>
     ServiceDegraded,
+    DataStoreConnectionDegraded,
     DataStoreStateDegraded,
     ConnectedStoreDegraded,
     CustomerManagedKeyAccessLost,

src/Microsoft.Health.SqlServer.UnitTests/Features/Health/SqlServerHealthCheckTests.cs

@@ -0,0 +1,86 @@
+// -------------------------------------------------------------------------------------------------
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License (MIT). See LICENSE in the repo root for license information.
+// -------------------------------------------------------------------------------------------------
+
+using System.Net;
+using System.Net.Http;
+using System.Threading;
+using System.Threading.Tasks;
+using Microsoft.Data.SqlClient;
+using Microsoft.Extensions.Diagnostics.HealthChecks;
+using Microsoft.Extensions.Logging;
+using Microsoft.Extensions.Options;
+using Microsoft.Health.Core.Features.Health;
+using Microsoft.Health.Encryption.Customer.Health;
+using Microsoft.Health.SqlServer.Configs;
+using Microsoft.Health.SqlServer.Features.Client;
+using Microsoft.Health.SqlServer.Features.Health;
+using Microsoft.Health.SqlServer.Features.Storage;
+using NSubstitute;
+using Xunit;
+
+namespace Microsoft.Health.SqlServer.UnitTests.Features.Health;
+
+public sealed class SqlServerHealthCheckTests
+{
+    private readonly ILogger<SqlServerHealthCheck> _logger;
+    private readonly SqlTransactionHandler _sqlTransactionHandler;
+    private readonly ISqlConnectionBuilder _sqlConnectionBuilder;
+    private readonly SqlRetryLogicBaseProvider _sqlRetryLogicBaseProvider;
+    private readonly IOptions<SqlServerDataStoreConfiguration> _sqlServerDataStoreConfiguration;
+    private readonly ValueCache<CustomerKeyHealth> _cache;
+
+    public SqlServerHealthCheckTests()
+    {
+        _logger = Substitute.For<ILogger<SqlServerHealthCheck>>();
+        _sqlTransactionHandler = Substitute.For<SqlTransactionHandler>();
+        _sqlConnectionBuilder = Substitute.For<ISqlConnectionBuilder>();
+        _sqlRetryLogicBaseProvider = Substitute.For<SqlRetryLogicBaseProvider>();
+
+        _sqlServerDataStoreConfiguration = Substitute.For<IOptions<SqlServerDataStoreConfiguration>>();
+        _sqlServerDataStoreConfiguration.Value.Returns(new SqlServerDataStoreConfiguration());
+
+        _cache = new ValueCache<CustomerKeyHealth>();
+        _cache.Set(new CustomerKeyHealth() { IsHealthy = true });
+    }
+
+    [Theory]
+    [InlineData(HttpStatusCode.Forbidden)]
+    [InlineData(HttpStatusCode.Unauthorized)]
+    public async Task GivenASqlHealthCheck_WhenSqlConnectionWrapperThrowsAnInvalidAccess_ThenHandlesItProperlyAsDegraded(HttpStatusCode statusCode)
+    {
+        HealthCheckResult healthCheckResult = await GetHealthCheckResultGivenAnErrorHttpStatusCodeAsync(statusCode);
+
+        Assert.Equal(HealthStatus.Degraded, healthCheckResult.Status);
+        Assert.Equal(HealthStatusReason.DataStoreConnectionDegraded.ToString(), healthCheckResult.Data["Reason"]);
+    }
+
+    [Theory]
+    [InlineData(HttpStatusCode.NotFound)]
+    public async Task GivenASqlHealthCheck_WhenSqlConnectionWrapperThrowsAnUnknownStatusCode_ThenHandlesItError(HttpStatusCode statusCode)
+    {
+        HttpRequestException httpException = await Assert.ThrowsAsync<HttpRequestException>(() => GetHealthCheckResultGivenAnErrorHttpStatusCodeAsync(statusCode));
+
+        Assert.Equal(statusCode, httpException.StatusCode);
+    }
+
+    private async Task<HealthCheckResult> GetHealthCheckResultGivenAnErrorHttpStatusCodeAsync(HttpStatusCode statusCode)
+    {
+        // Exception thrown by the SqlConnectionWrapperFactory.ObtainSqlConnectionWrapperAsync method.
+        var httpRequestException = new HttpRequestException("error", inner: null, statusCode: statusCode);
+
+        SqlConnectionWrapperFactory connectionWrapperFactory = Substitute.For<SqlConnectionWrapperFactory>(
+            _sqlTransactionHandler,
+            _sqlConnectionBuilder,
+            _sqlRetryLogicBaseProvider,
+            _sqlServerDataStoreConfiguration);
+
+        // Setting up the ObtainSqlConnectionWrapperAsync method to throw an exception.
+        connectionWrapperFactory.ObtainSqlConnectionWrapperAsync(Arg.Any<CancellationToken>()).Returns(Task.FromException<SqlConnectionWrapper>(httpRequestException));
+
+        var sqlHealthCheck = new SqlServerHealthCheck(connectionWrapperFactory, _cache, _logger);
+
+        return await sqlHealthCheck.CheckHealthAsync(new HealthCheckContext(), CancellationToken.None);
+    }
+}

src/Microsoft.Health.SqlServer/Features/Client/SqlConnectionWrapperFactory.cs

@@ -40,7 +40,7 @@ public SqlConnectionWrapperFactory(
     public string DefaultDatabase => _sqlConnectionBuilder.DefaultDatabase;
 
     [SuppressMessage("Reliability", "CA2000:Dispose objects before losing scope", Justification = "Callers are responsible for disposal.")]
-    public async Task<SqlConnectionWrapper> ObtainSqlConnectionWrapperAsync(CancellationToken cancellationToken, bool enlistInTransaction = false)
+    public virtual async Task<SqlConnectionWrapper> ObtainSqlConnectionWrapperAsync(CancellationToken cancellationToken, bool enlistInTransaction = false)
     {
         var sqlConnectionWrapper = new SqlConnectionWrapper(_sqlTransactionHandler, _sqlConnectionBuilder, _sqlRetryLogicBaseProvider, enlistInTransaction, _sqlServerDataStoreConfiguration);
         await sqlConnectionWrapper.InitializeAsync(cancellationToken: cancellationToken).ConfigureAwait(false);

src/Microsoft.Health.SqlServer/Features/Health/SqlServerHealthCheck.cs

@@ -4,6 +4,7 @@
 // -------------------------------------------------------------------------------------------------
 
 using System.Collections.Generic;
+using System.Net.Http;
 using System.Threading;
 using System.Threading.Tasks;
 using EnsureThat;
@@ -52,17 +53,30 @@ public override async Task<HealthCheckResult> CheckStorageHealthAsync(Cancellati
 
             return HealthCheckResult.Healthy("Successfully connected.");
         }
-        catch (SqlException e) when (e.IsCMKError())
+        catch (HttpRequestException httpEx) when (httpEx.IsInvalidAccess())
+        {
+            // Attempts to retrieve the connection string can fail with HTTP errors if the SQL Connection Wrapper relies on
+            // HTTP requests. For this reason, these HTTP errors must be caught and properly handled.
+
+            HealthStatusReason reason = HealthStatusReason.DataStoreConnectionDegraded;
+
+            return new HealthCheckResult(
+                HealthStatus.Degraded,
+                DegradedDescription,
+                httpEx,
+                new Dictionary<string, object> { { "Reason", reason.ToString() } });
+        }
+        catch (SqlException sqlEx) when (sqlEx.IsCMKError())
         {
             // Error 40925: "Can not connect to the database in its current state". This error can be for various DB states (recovering, inacessible) but we assume that our DB will only hit this for Inaccessible state
-            HealthStatusReason reason = e.Number is SqlErrorCodes.CannotConnectToDBInCurrentState
+            HealthStatusReason reason = sqlEx.Number is SqlErrorCodes.CannotConnectToDBInCurrentState
                 ? HealthStatusReason.DataStoreStateDegraded
                 : HealthStatusReason.CustomerManagedKeyAccessLost;
 
             return new HealthCheckResult(
                 HealthStatus.Degraded,
                 DegradedDescription,
-                e,
+                sqlEx,
                 new Dictionary<string, object> { { "Reason", reason.ToString() } });
         }
     }

src/Microsoft.Health.SqlServer/Features/Storage/HttpErrorExtensions.cs

@@ -0,0 +1,17 @@
+// -------------------------------------------------------------------------------------------------
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License (MIT). See LICENSE in the repo root for license information.
+// -------------------------------------------------------------------------------------------------
+
+using System.Net;
+using System.Net.Http;
+
+namespace Microsoft.Health.SqlServer.Features.Storage;
+
+public static class HttpErrorExtensions
+{
+    public static bool IsInvalidAccess(this HttpRequestException exception)
+    {
+        return exception?.StatusCode is HttpStatusCode.Forbidden or HttpStatusCode.Unauthorized;
+    }
+}

src/Microsoft.Health.SqlServer/Features/Storage/SqlServerSchemaDataStore.cs

@@ -7,6 +7,7 @@
 using System.Collections.Generic;
 using System.Globalization;
 using System.Linq;
+using System.Net.Http;
 using System.Threading;
 using System.Threading.Tasks;
 using EnsureThat;
@@ -44,24 +45,33 @@ public SqlServerSchemaDataStore(
     public async Task<CompatibleVersions> GetLatestCompatibleVersionsAsync(CancellationToken cancellationToken)
     {
         CompatibleVersions compatibleVersions;
-        using (SqlConnectionWrapper sqlConnectionWrapper = await _sqlConnectionWrapperFactory.ObtainSqlConnectionWrapperAsync(cancellationToken: cancellationToken).ConfigureAwait(false))
-        using (SqlCommandWrapper sqlCommandWrapper = sqlConnectionWrapper.CreateRetrySqlCommand())
-        {
-            SchemaShared.SelectCompatibleSchemaVersions.PopulateCommand(sqlCommandWrapper);
 
-            using (var dataReader = await sqlCommandWrapper.ExecuteReaderAsync(cancellationToken).ConfigureAwait(false))
+        try
+        {
+            using (SqlConnectionWrapper sqlConnectionWrapper = await _sqlConnectionWrapperFactory.ObtainSqlConnectionWrapperAsync(cancellationToken: cancellationToken).ConfigureAwait(false))
+            using (SqlCommandWrapper sqlCommandWrapper = sqlConnectionWrapper.CreateRetrySqlCommand())
             {
-                if (await dataReader.ReadAsync(cancellationToken).ConfigureAwait(false))
-                {
-                    compatibleVersions = new CompatibleVersions(ConvertToInt(dataReader.GetValue(0)), ConvertToInt(dataReader.GetValue(1)));
-                }
-                else
+                SchemaShared.SelectCompatibleSchemaVersions.PopulateCommand(sqlCommandWrapper);
+
+                using (var dataReader = await sqlCommandWrapper.ExecuteReaderAsync(cancellationToken).ConfigureAwait(false))
                 {
-                    throw new CompatibleVersionsNotFoundException(Resources.CompatibilityRecordNotFound);
+                    if (await dataReader.ReadAsync(cancellationToken).ConfigureAwait(false))
+                    {
+                        compatibleVersions = new CompatibleVersions(ConvertToInt(dataReader.GetValue(0)), ConvertToInt(dataReader.GetValue(1)));
+                    }
+                    else
+                    {
+                        throw new CompatibleVersionsNotFoundException(Resources.CompatibilityRecordNotFound);
+                    }
                 }
-            }
 
-            return compatibleVersions;
+                return compatibleVersions;
+            }
+        }
+        catch (HttpRequestException httpEx) when (httpEx.IsInvalidAccess())
+        {
+            _logger.LogError(httpEx, "Error while getting SQL connection on getting latest compatible version");
+            throw;
         }
 
         static int ConvertToInt(object o)
@@ -79,61 +89,74 @@ static int ConvertToInt(object o)
 
     public async Task<int> UpsertInstanceSchemaInformationAsync(string name, SchemaInformation schemaInformation, CancellationToken cancellationToken)
     {
-        using (SqlConnectionWrapper sqlConnectionWrapper = await _sqlConnectionWrapperFactory.ObtainSqlConnectionWrapperAsync(cancellationToken: cancellationToken).ConfigureAwait(false))
-        using (SqlCommandWrapper sqlCommandWrapper = sqlConnectionWrapper.CreateRetrySqlCommand())
+        try
         {
-            SchemaShared.UpsertInstanceSchema.PopulateCommand(
-                 sqlCommandWrapper,
-                 name,
-                 schemaInformation.MaximumSupportedVersion,
-                 schemaInformation.MinimumSupportedVersion,
-                 _configuration.SchemaOptions.InstanceRecordExpirationTimeInMinutes);
-            try
+            using (SqlConnectionWrapper sqlConnectionWrapper = await _sqlConnectionWrapperFactory.ObtainSqlConnectionWrapperAsync(cancellationToken: cancellationToken).ConfigureAwait(false))
+            using (SqlCommandWrapper sqlCommandWrapper = sqlConnectionWrapper.CreateRetrySqlCommand())
             {
+                SchemaShared.UpsertInstanceSchema.PopulateCommand(
+                     sqlCommandWrapper,
+                     name,
+                     schemaInformation.MaximumSupportedVersion,
+                     schemaInformation.MinimumSupportedVersion,
+                     _configuration.SchemaOptions.InstanceRecordExpirationTimeInMinutes);
+
                 return (int)await sqlCommandWrapper.ExecuteScalarAsync(cancellationToken).ConfigureAwait(false);
             }
-            catch (SqlException e)
+        }
+        catch (HttpRequestException httpEx) when (httpEx.IsInvalidAccess())
+        {
+            _logger.LogError(httpEx, "Error while getting SQL connection on upserting InstanceSchema information");
+            throw;
+        }
+        catch (SqlException sqlEx)
+        {
+            if (sqlEx.Number == SqlErrorCodes.CouldNotFoundStoredProc && schemaInformation.Current == null)
             {
-                if (e.Number == SqlErrorCodes.CouldNotFoundStoredProc && schemaInformation.Current == null)
-                {
-                    // this could happen during schema initialization until base schema is not executed
-                    throw;
-                }
-
-                _logger.LogError(e, "Error from SQL database on upserting InstanceSchema information");
+                // this could happen during schema initialization until base schema is not executed
                 throw;
             }
+
+            _logger.LogError(sqlEx, "Error from SQL database on upserting InstanceSchema information");
+            throw;
         }
     }
 
     public async Task DeleteExpiredInstanceSchemaAsync(CancellationToken cancellationToken)
     {
-        using (SqlConnectionWrapper sqlConnectionWrapper = await _sqlConnectionWrapperFactory.ObtainSqlConnectionWrapperAsync(cancellationToken: cancellationToken).ConfigureAwait(false))
-        using (SqlCommandWrapper sqlCommandWrapper = sqlConnectionWrapper.CreateRetrySqlCommand())
+        try
         {
-            SchemaShared.DeleteInstanceSchema.PopulateCommand(sqlCommandWrapper);
-            try
+            using (SqlConnectionWrapper sqlConnectionWrapper = await _sqlConnectionWrapperFactory.ObtainSqlConnectionWrapperAsync(cancellationToken: cancellationToken).ConfigureAwait(false))
+            using (SqlCommandWrapper sqlCommandWrapper = sqlConnectionWrapper.CreateRetrySqlCommand())
             {
+                SchemaShared.DeleteInstanceSchema.PopulateCommand(sqlCommandWrapper);
+
                 await sqlCommandWrapper.ExecuteNonQueryAsync(cancellationToken).ConfigureAwait(false);
             }
-            catch (SqlException e)
-            {
-                _logger.LogError(e, "Error from SQL database on deleting expired InstanceSchema records");
-                throw;
-            }
+        }
+        catch (HttpRequestException httpEx) when (httpEx.IsInvalidAccess())
+        {
+            _logger.LogError(httpEx, "Error while getting SQL connection on deleting expired InstanceSchema records");
+            throw;
+        }
+        catch (SqlException sqlEx)
+        {
+            _logger.LogError(sqlEx, "Error from SQL database on deleting expired InstanceSchema records");
+            throw;
         }
     }
 
     public async Task<List<CurrentVersionInformation>> GetCurrentVersionAsync(CancellationToken cancellationToken)
     {
         var currentVersions = new List<CurrentVersionInformation>();
-        using (SqlConnectionWrapper sqlConnectionWrapper = await _sqlConnectionWrapperFactory.ObtainSqlConnectionWrapperAsync(cancellationToken: cancellationToken).ConfigureAwait(false))
-        using (SqlCommandWrapper sqlCommandWrapper = sqlConnectionWrapper.CreateRetrySqlCommand())
-        {
-            SchemaShared.SelectCurrentVersionsInformation.PopulateCommand(sqlCommandWrapper);
 
-            try
+        try
+        {
+            using (SqlConnectionWrapper sqlConnectionWrapper = await _sqlConnectionWrapperFactory.ObtainSqlConnectionWrapperAsync(cancellationToken: cancellationToken).ConfigureAwait(false))
+            using (SqlCommandWrapper sqlCommandWrapper = sqlConnectionWrapper.CreateRetrySqlCommand())
             {
+                SchemaShared.SelectCurrentVersionsInformation.PopulateCommand(sqlCommandWrapper);
+
                 using (var dataReader = await sqlCommandWrapper.ExecuteReaderAsync(cancellationToken).ConfigureAwait(false))
                 {
                     if (dataReader.HasRows)
@@ -162,11 +185,16 @@ public async Task<List<CurrentVersionInformation>> GetCurrentVersionAsync(Cancel
                     }
                 }
             }
-            catch (SqlException e)
-            {
-                _logger.LogError(e, "Error from SQL database on retrieving current version information");
-                throw;
-            }
+        }
+        catch (HttpRequestException httpEx) when (httpEx.IsInvalidAccess())
+        {
+            _logger.LogError(httpEx, "Error while getting SQL connection on retrieving current version information");
+            throw;
+        }
+        catch (SqlException sqlEx)
+        {
+            _logger.LogError(sqlEx, "Error from SQL database on retrieving current version information");
+            throw;
         }
 
         return currentVersions;