Skip to content

Commit

Permalink
Update health check to handle concurrency (#3317)
Browse files Browse the repository at this point in the history
* use leases for health check; update not found error

* added blob type not supported

* update unit tests to reflect new health check behavior

* move exception extensions to core

* write health check blob to different files, and set expiry time on write

* update unit tests and add test for pipeline policy

* remove unnecessary nuget package

* update blob file store tests

* remove comment in appsettings

* Respond to PR comments

* add invalidauthenticationinfo as customer error

* respond to pr comments

* add both regex options
  • Loading branch information
jnlycklama authored Jan 29, 2024
1 parent 93c8eb2 commit 245e308
Show file tree
Hide file tree
Showing 12 changed files with 305 additions and 52 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
// -------------------------------------------------------------------------------------------------
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License (MIT). See LICENSE in the repo root for license information.
// -------------------------------------------------------------------------------------------------

using System;
using System.Linq;
using Azure.Core;
using Azure.Core.Pipeline;
using Microsoft.Health.Dicom.Blob.Features.ExternalStore;
using Microsoft.Health.Dicom.Blob.Utilities;
using NSubstitute;
using Xunit;

namespace Microsoft.Health.Dicom.Blob.UnitTests.Features.ExternalStore;

public class ExternalStoreHealthExpiryHttpPolicyTests
{
private readonly ExternalBlobDataStoreConfiguration _blobDataStoreConfiguration;
private readonly ExternalStoreHealthExpiryHttpPipelinePolicy _externalStoreHealthExpiryPolicy;

private readonly MockRequest _request;
private readonly HttpPipelinePolicy _mockPipeline = Substitute.For<HttpPipelinePolicy>();

public ExternalStoreHealthExpiryHttpPolicyTests()
{
_blobDataStoreConfiguration = new ExternalBlobDataStoreConfiguration()
{
BlobContainerUri = new Uri("https://myBlobStore.blob.core.net/myContainer"),
StorageDirectory = "DICOM",
HealthCheckFilePath = "healthCheck/health",
HealthCheckFileExpiry = TimeSpan.FromMinutes(1),
};

_request = new MockRequest();
_externalStoreHealthExpiryPolicy = new ExternalStoreHealthExpiryHttpPipelinePolicy(_blobDataStoreConfiguration);
}

[Theory]
[InlineData("PUT")]
[InlineData("POST")]
[InlineData("PATCH")]
public void GivenHealthCheckBlob_Proccess_AddsExpiryHeaders(string requestMethod)
{
RequestUriBuilder requestUriBuilder = new RequestUriBuilder();
requestUriBuilder.Reset(new Uri($"https://myBlobStore.blob.core.net/myContainer/DICOM/healthCheck/health{Guid.NewGuid()}.txt"));

_request.Uri = requestUriBuilder;
_request.Method = RequestMethod.Parse(requestMethod);
HttpMessage httpMessage = new HttpMessage(_request, new ResponseClassifier());

_externalStoreHealthExpiryPolicy.Process(httpMessage, new ReadOnlyMemory<HttpPipelinePolicy>(new HttpPipelinePolicy[] { _mockPipeline }));

_request.MockHeaders.Single(h => h.Name == "x-ms-expiry-time" && h.Value == $"{_blobDataStoreConfiguration.HealthCheckFileExpiry.TotalMilliseconds}");
_request.MockHeaders.Single(h => h.Name == "x-ms-expiry-option" && h.Value == "RelativeToNow");
}

[Theory]
[InlineData("PUT")]
[InlineData("POST")]
[InlineData("PATCH")]
[InlineData("GET")]
[InlineData("DELETE")]
public void GivenNonHealthCheckBlob_ProccessForAllRequestTypes_NoHeadersAdded(string requestMethod)
{
RequestUriBuilder requestUriBuilder = new RequestUriBuilder();
requestUriBuilder.Reset(new Uri($"https://myBlobStore.blob.core.net/myContainer/DICOM/healthCheck/health{Guid.NewGuid()}.txt/anotherBlob"));

_request.Uri = requestUriBuilder;
_request.Method = RequestMethod.Parse(requestMethod);
HttpMessage httpMessage = new HttpMessage(_request, new ResponseClassifier());

_externalStoreHealthExpiryPolicy.Process(httpMessage, new ReadOnlyMemory<HttpPipelinePolicy>(new HttpPipelinePolicy[] { _mockPipeline }));

Assert.Empty(_request.MockHeaders);
}

[Theory]
[InlineData("https://blob.com")]
[InlineData("https://myBlobStore.blob.core.net/myContainer/DICOM/anotherDirectory/healthCheck/health00000000-0000-0000-0000-000000000000/anotherBlob")]
[InlineData("https://myBlobStore.blob.core.net/myContainer/DICOM/anotherDirectory/healthCheck/health00000000-0000-0000-0000-000000000000.txt")]
[InlineData("https://myBlobStore.blob.core.net/myContainer/DICOM/healthCheck/health00000000-0000-0000-0000-000000000000")]
[InlineData("https://myBlobStore.blob.core.net/myContainer/DICOM/healthCheck/health")]
[InlineData("https://myBlobStore.blob.core.net/myContainer/DICOM/healthCheck")]
public void GivenNonHealthCheckBlob_ProccessDifferentBlobUris_NoHeadersAdded(string nonHealthCheckBlob)
{
RequestUriBuilder requestUriBuilder = new RequestUriBuilder();
requestUriBuilder.Reset(new Uri(nonHealthCheckBlob));

_request.Uri = requestUriBuilder;
_request.Method = RequestMethod.Put;
HttpMessage httpMessage = new HttpMessage(_request, new ResponseClassifier());

_externalStoreHealthExpiryPolicy.Process(httpMessage, new ReadOnlyMemory<HttpPipelinePolicy>(new HttpPipelinePolicy[] { _mockPipeline }));

Assert.Empty(_request.MockHeaders);
}

[Theory]
[InlineData("GET")]
[InlineData("DELETE")]
public void GivenHealthCheckBlobReadOrDelete_Proccess_AddsExpiryHeaders(string requestMethod)
{
RequestUriBuilder requestUriBuilder = new RequestUriBuilder();
requestUriBuilder.Reset(new Uri($"https://myBlobStore.blob.core.net/myContainer/DICOM/healthCheck/health{Guid.NewGuid()}.txt"));

_request.Uri = requestUriBuilder;
_request.Method = RequestMethod.Parse(requestMethod);
HttpMessage httpMessage = new HttpMessage(_request, new ResponseClassifier());

_externalStoreHealthExpiryPolicy.Process(httpMessage, new ReadOnlyMemory<HttpPipelinePolicy>(new HttpPipelinePolicy[] { _mockPipeline }));

Assert.Empty(_request.MockHeaders);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
// -------------------------------------------------------------------------------------------------
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License (MIT). See LICENSE in the repo root for license information.
// -------------------------------------------------------------------------------------------------

using System;
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using Azure.Core;

namespace Microsoft.Health.Dicom.Blob.UnitTests.Features.ExternalStore;

internal class MockRequest : Request
{
public MockRequest()
{
MockHeaders = new List<HttpHeader>();
}

public List<HttpHeader> MockHeaders { get; }

public override string ClientRequestId { get => throw new NotImplementedException(); set => throw new NotImplementedException(); }

public override void Dispose() => throw new NotImplementedException();

protected override void AddHeader(string name, string value)
{
MockHeaders.Add(new HttpHeader(name, value));
}

protected override bool ContainsHeader(string name)
{
return MockHeaders.Any(h => h.Name == name);
}

protected override IEnumerable<HttpHeader> EnumerateHeaders()
{
return MockHeaders;
}

protected override bool RemoveHeader(string name) => throw new NotImplementedException();
protected override bool TryGetHeader(string name, [NotNullWhen(true)] out string value) => throw new NotImplementedException();
protected override bool TryGetHeaderValues(string name, [NotNullWhen(true)] out IEnumerable<string> values) => throw new NotImplementedException();
}
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ public async Task GivenHealthyConnection_RunHealthCheck_ReturnsHealthy()
}

[Theory]
[InlineData(401, "InvalidAuthenticationInfo")]
[InlineData(403, "InvalidAuthenticationInfo")]
[InlineData(403, "AuthorizationFailure")]
[InlineData(403, "AuthorizationPermissionMismatch")]
[InlineData(403, "InsufficientAccountPermissions")]
Expand All @@ -69,7 +71,7 @@ public async Task GivenHealthyConnection_RunHealthCheck_ReturnsHealthy()
[InlineData(404, "FilesystemNotFound")]
[InlineData(409, "ContainerBeingDeleted")]
[InlineData(409, "ContainerDisabled")]
public async Task GivenRequestFailedExceptionForCustomerError_RunHealthCheck_ReturnsDegradedAndDeletesBlob(int statusCode, string blobErrorCode)
public async Task GivenRequestFailedExceptionForCustomerError_RunHealthCheck_ReturnsDegraded(int statusCode, string blobErrorCode)
{
_blockBlobClient.DownloadContentAsync(Arg.Any<CancellationToken>())
.Throws(new RequestFailedException(statusCode, "Error", blobErrorCode, new System.Exception()));
Expand All @@ -80,29 +82,26 @@ public async Task GivenRequestFailedExceptionForCustomerError_RunHealthCheck_Ret

Assert.Equal(HealthStatus.Degraded, result.Status);
Assert.Equal(HealthStatusReason.ConnectedStoreDegraded.ToString(), healthStatusReason.ToString());

await _blockBlobClient.Received(2).DeleteIfExistsAsync(Arg.Any<DeleteSnapshotsOption>(), Arg.Any<BlobRequestConditions>(), Arg.Any<CancellationToken>());
}

[Theory]
[InlineData(400, "SomeErrorFromDicomBug")]
[InlineData(401, "AuthErrorFromDicomBug")]
[InlineData(403, "AuthErrorFromDicomBug")]
[InlineData(404, "NotFoundDueToDicomBug")]
[InlineData(409, "ConflictDueToDicomBug")]
public async Task GivenRequestFailedExceptionForServiceError_RunHealthCheck_ThrowsExceptionAndCleansUpBlob(int statusCode, string blobErrorCode)
public async Task GivenRequestFailedExceptionForServiceError_RunHealthCheck_ThrowsException(int statusCode, string blobErrorCode)
{
_blockBlobClient.DownloadContentAsync(Arg.Any<CancellationToken>())
.Throws(new RequestFailedException(statusCode, "Error", blobErrorCode, new System.Exception()));

await Assert.ThrowsAsync<RequestFailedException>(async () => await _dicomConnectedStoreHealthCheck.CheckHealthAsync(null, CancellationToken.None));

await _blockBlobClient.Received(2).DeleteIfExistsAsync(Arg.Any<DeleteSnapshotsOption>(), Arg.Any<BlobRequestConditions>(), Arg.Any<CancellationToken>());
}

[Fact]
public async Task HostNotFound_RunHealthCheck_ReturnsDegraded()
{
_blockBlobClient.StageBlockAsync(Arg.Any<string>(), Arg.Any<Stream>(), Arg.Any<BlockBlobStageBlockOptions>(), Arg.Any<CancellationToken>())
_blockBlobClient.UploadAsync(Arg.Any<Stream>(), cancellationToken: Arg.Any<CancellationToken>())
.Throws(new AggregateException(new List<Exception>()
{
new Exception("No such host is known."),
Expand All @@ -119,13 +118,15 @@ public async Task HostNotFound_RunHealthCheck_ReturnsDegraded()
}

[Fact]
public async Task AllDeleteFails_RunHealthCheck_ReturnsDegradedAndExceptionIsNotThrown()
public async Task NameOrServiceNotKnown_RunHealthCheck_ReturnsDegraded()
{
_blockBlobClient.DeleteIfExistsAsync(Arg.Any<DeleteSnapshotsOption>(), Arg.Any<BlobRequestConditions>(), Arg.Any<CancellationToken>())
.Throws(new RequestFailedException(403, "Failure", BlobErrorCode.AuthorizationFailure.ToString(), new System.Exception()));

_blockBlobClient.DeleteAsync(Arg.Any<DeleteSnapshotsOption>(), Arg.Any<BlobRequestConditions>(), Arg.Any<CancellationToken>())
.Throws(new RequestFailedException(403, "Failure", BlobErrorCode.AuthorizationFailure.ToString(), new System.Exception()));
_blockBlobClient.UploadAsync(Arg.Any<Stream>(), cancellationToken: Arg.Any<CancellationToken>())
.Throws(new AggregateException(new List<Exception>()
{
new Exception("Name or service not known."),
new Exception("Name or service not known."),
new Exception("Name or service not known."),
}));

HealthCheckResult result = await _dicomConnectedStoreHealthCheck.CheckHealthAsync(null, CancellationToken.None);

Expand All @@ -136,14 +137,16 @@ public async Task AllDeleteFails_RunHealthCheck_ReturnsDegradedAndExceptionIsNot
}

[Fact]
public async Task DeleteBeforeAndAfterFails_RunHealthCheck_ReturnsHealthyAndExceptionIsNotThrown()
public async Task DeleteFails_RunHealthCheck_ReturnsDegradedAndExceptionIsNotThrown()
{
// the extra deletes before and after the checks fail, but everything else successfully finishes
_blockBlobClient.DeleteIfExistsAsync(Arg.Any<DeleteSnapshotsOption>(), Arg.Any<BlobRequestConditions>(), Arg.Any<CancellationToken>())
_blockBlobClient.DeleteAsync(Arg.Any<DeleteSnapshotsOption>(), Arg.Any<BlobRequestConditions>(), Arg.Any<CancellationToken>())
.Throws(new RequestFailedException(403, "Failure", BlobErrorCode.AuthorizationFailure.ToString(), new System.Exception()));

HealthCheckResult result = await _dicomConnectedStoreHealthCheck.CheckHealthAsync(null, CancellationToken.None);

Assert.Equal(HealthStatus.Healthy, result.Status);
result.Data.TryGetValue("Reason", out object healthStatusReason);

Assert.Equal(HealthStatus.Degraded, result.Status);
Assert.Equal(HealthStatusReason.ConnectedStoreDegraded.ToString(), healthStatusReason.ToString());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,9 @@ public class BlobFileStoreTests
{
private const string DefaultBlobName = "foo/123.dcm";
private const string DefaultStorageDirectory = "/test/";
private const string HealthCheckFilePath = "health";
private static readonly BlobFileStoreMeter BlobFileStoreMeter = new BlobFileStoreMeter();
private static readonly Uri BlobContainerUrl = new Uri("https://myBlobAccount.blob.core.net/myContainer");

private readonly FileProperties _defaultFileProperties = new FileProperties
{
Expand All @@ -57,7 +59,7 @@ public class BlobFileStoreTests
[InlineData("a%b")]
public void GivenInvalidStorageDirectory_WhenExternalStoreInitialized_ThenThrowExceptionWithRightMessageAndProperty(string storageDirectory)
{
ExternalBlobDataStoreConfiguration config = new ExternalBlobDataStoreConfiguration() { StorageDirectory = storageDirectory };
ExternalBlobDataStoreConfiguration config = new ExternalBlobDataStoreConfiguration() { BlobContainerUri = BlobContainerUrl, StorageDirectory = storageDirectory, HealthCheckFilePath = HealthCheckFilePath, HealthCheckFileExpiry = TimeSpan.FromMinutes(1) };
var results = new List<ValidationResult>();

Assert.False(Validator.TryValidateObject(config, new ValidationContext(config), results, validateAllProperties: true));
Expand All @@ -74,7 +76,7 @@ public void GivenInvalidStorageDirectory_WhenExternalStoreInitialized_ThenThrowE
[InlineData("a-b/c-d/")]
public void GivenValidStorageDirectory_WhenExternalStoreInitialized_ThenDoNotThrowException(string storageDirectory)
{
ExternalBlobDataStoreConfiguration config = new ExternalBlobDataStoreConfiguration() { StorageDirectory = storageDirectory };
ExternalBlobDataStoreConfiguration config = new ExternalBlobDataStoreConfiguration() { BlobContainerUri = BlobContainerUrl, StorageDirectory = storageDirectory, HealthCheckFilePath = HealthCheckFilePath, HealthCheckFileExpiry = TimeSpan.FromMinutes(1) };
var results = new List<ValidationResult>();

Assert.True(Validator.TryValidateObject(config, new ValidationContext(config), results, validateAllProperties: true));
Expand All @@ -83,7 +85,7 @@ public void GivenValidStorageDirectory_WhenExternalStoreInitialized_ThenDoNotThr
[Fact]
public void GivenInvalidStorageDirectorySegments_WhenExternalStoreInitialized_ThenThrowExceptionWithRightMessageAndProperty()
{
ExternalBlobDataStoreConfiguration config = new ExternalBlobDataStoreConfiguration() { StorageDirectory = string.Join("", Enumerable.Repeat("a/b", 255)) };
ExternalBlobDataStoreConfiguration config = new ExternalBlobDataStoreConfiguration() { BlobContainerUri = BlobContainerUrl, StorageDirectory = string.Join("", Enumerable.Repeat("a/b", 255)), HealthCheckFilePath = HealthCheckFilePath, HealthCheckFileExpiry = TimeSpan.FromMinutes(1) };
var results = new List<ValidationResult>();

Assert.False(Validator.TryValidateObject(config, new ValidationContext(config), results, validateAllProperties: true));
Expand All @@ -95,7 +97,7 @@ public void GivenInvalidStorageDirectorySegments_WhenExternalStoreInitialized_Th
[Fact]
public void GivenInvalidStorageDirectoryLength_WhenExternalStoreInitialized_ThenThrowExceptionWithRightMessageAndProperty()
{
ExternalBlobDataStoreConfiguration config = new ExternalBlobDataStoreConfiguration() { StorageDirectory = string.Join("", Enumerable.Repeat("a", 1025)) };
ExternalBlobDataStoreConfiguration config = new ExternalBlobDataStoreConfiguration() { BlobContainerUri = BlobContainerUrl, StorageDirectory = string.Join("", Enumerable.Repeat("a", 1025)), HealthCheckFilePath = HealthCheckFilePath, HealthCheckFileExpiry = TimeSpan.FromMinutes(1) };
var results = new List<ValidationResult>();

Assert.False(Validator.TryValidateObject(config, new ValidationContext(config), results, validateAllProperties: true));
Expand Down Expand Up @@ -501,6 +503,9 @@ private static void InitializeExternalBlobFileStore(out BlobFileStore blobFileSt
ConnectionString = "test",
ContainerName = "test",
StorageDirectory = DefaultStorageDirectory,
BlobContainerUri = BlobContainerUrl,
HealthCheckFilePath = HealthCheckFilePath,
HealthCheckFileExpiry = TimeSpan.FromMinutes(1),
});
var clientOptions = Substitute.For<IOptions<BlobServiceClientOptions>>();
clientOptions.Value.Returns(Substitute.For<BlobServiceClientOptions>());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,19 +6,25 @@
using System;
using System.Collections.Generic;
using System.Linq;
using Azure;
using Azure.Storage.Blobs.Models;
using Azure;

namespace Microsoft.Health.Dicom.Blob.Extensions;

internal static class AzureStorageErrorExtensions
internal static class BlobStorageErrorExtensions
{
private static readonly List<BlobErrorCode> Customer401ErrorCodes = new List<BlobErrorCode>
{
BlobErrorCode.InvalidAuthenticationInfo,
};

private static readonly List<BlobErrorCode> Customer403ErrorCodes = new List<BlobErrorCode>
{
BlobErrorCode.AuthorizationFailure,
BlobErrorCode.AuthorizationPermissionMismatch,
BlobErrorCode.InsufficientAccountPermissions,
BlobErrorCode.AccountIsDisabled,
BlobErrorCode.InvalidAuthenticationInfo,
"KeyVaultEncryptionKeyNotFound",
"KeyVaultAccessTokenCannotBeAcquired",
"KeyVaultVaultNotFound",
Expand All @@ -38,14 +44,9 @@ internal static class AzureStorageErrorExtensions

public static bool IsConnectedStoreCustomerError(this RequestFailedException rfe)
{
return (rfe.Status == 403 && Customer403ErrorCodes.Any(e => e.ToString().Equals(rfe.ErrorCode, StringComparison.OrdinalIgnoreCase))) ||
return (rfe.Status == 401 && Customer401ErrorCodes.Any(e => e.ToString().Equals(rfe.ErrorCode, StringComparison.OrdinalIgnoreCase))) ||
(rfe.Status == 403 && Customer403ErrorCodes.Any(e => e.ToString().Equals(rfe.ErrorCode, StringComparison.OrdinalIgnoreCase))) ||
(rfe.Status == 404 && Customer404ErrorCodes.Any(e => e.ToString().Equals(rfe.ErrorCode, StringComparison.OrdinalIgnoreCase))) ||
(rfe.Status == 409 && Customer409ErrorCodes.Any(e => e.ToString().Equals(rfe.ErrorCode, StringComparison.OrdinalIgnoreCase)));
}

public static bool IsStorageAccountUnknownHostError(this Exception exception)
{
return exception.Message.Contains("No such host is known", StringComparison.OrdinalIgnoreCase) ||
(exception is AggregateException ag && ag.InnerExceptions.All(e => e.Message.Contains("No such host is known", StringComparison.OrdinalIgnoreCase)));
}
}
Loading

0 comments on commit 245e308

Please sign in to comment.