Azure Key Vault Health check package and Unit tests project #19

CarlosLanderas · 2018-12-06T23:34:53Z

Azure Key Vault health check that allows to configure a secret collection to be verified against the Azure Service

Usage:

Default usage with Managed Service Identity:

services.AddHealthChecks()
               .AddAzureKeyVault(setup =>
               {
                   setup
                   .UseKeyVaultUrl("https://[vaultname].vault.azure.net/")                  
                   .AddSecret("surname");
               });

Usage usage client id and client secret:

  services.AddHealthChecks()
               .AddAzureKeyVault(setup =>
               {
                   setup
                   .UseKeyVaultUrl("https://[vaultname].vault.azure.net/")    
                   .UseClientSecrets("clientid", "clientsecret")
                   .AddSecret("surname");
               });

Sample error for a not found secret using UI.Client response writer

{
    "status": "Unhealthy",
    "totalDuration": "00:00:01.0434203",
    "entries": {
        "azurekeyvault": {
            "data": {},
            "description": "surname secret error - Operation returned an invalid status code 'NotFound'",
            "duration": "00:00:01.0403080",
            "exception": "surname secret error - Operation returned an invalid status code 'NotFound'",
            "status": "Unhealthy"
        }
    }
}

…ck for empty ctor params

unaizorrilla · 2018-12-09T10:46:19Z

build/dependencies.props

@@ -77,11 +80,12 @@
    <HealthCheckDynamoDb>2.2.0</HealthCheckDynamoDb>
    <HealthCheckDocumentDb>2.2.0</HealthCheckDocumentDb>
    <HealthCheckAzureStorage>2.2.0</HealthCheckAzureStorage>
-    <HealthCheckAzureServiceBus>2.2.0</HealthCheckAzureServiceBus>
+    <HealthCheckAzureServiceBus>2.2.1</HealthCheckAzureServiceBus>


why a new version on service bus

Because health checks contructor validations changed. It does not make sense to check only for null and not for empty string when initializing configuration towards an azure service

unaizorrilla · 2018-12-09T10:46:48Z

src/HealthChecks.AzureKeyVault/AzureKeyVaultHealthCheck.cs

+
+        public AzureKeyVaultHealthCheck(AzureKeyVaultOptions keyVaultOptions)
+        {
+            if (string.IsNullOrEmpty(keyVaultOptions.KeyVaultUrlBase)) throw new ArgumentNullException(keyVaultOptions.KeyVaultUrlBase);


please add backets

unaizorrilla · 2018-12-09T10:47:05Z

src/HealthChecks.AzureKeyVault/AzureKeyVaultHealthCheck.cs

+        }
+        public async Task<HealthCheckResult> CheckHealthAsync(HealthCheckContext context, CancellationToken cancellationToken = default)
+        {
+            string currentSecret = string.Empty;


unaizorrilla · 2018-12-09T10:47:52Z

src/HealthChecks.AzureKeyVault/AzureKeyVaultHealthCheck.cs

+                }
+
+                return HealthCheckResult.Healthy();
+


remove blank lines

unaizorrilla · 2018-12-09T10:48:11Z

src/HealthChecks.AzureKeyVault/AzureKeyVaultHealthCheck.cs

+                return new HealthCheckResult(context.Registration.FailureStatus, exception: secretException);
+            }
+        }
+


blank lines

unaizorrilla · 2018-12-09T10:53:01Z

...HealthChecks.AzureKeyVault/DependencyInjection/AzureKeyVaultHealthChecksBuilderExtensions.cs

+            string name = default, HealthStatus? failureStatus = default, IEnumerable<string> tags = default)
+        {
+            var azureKeyVaultOptions = new AzureKeyVaultOptions();
+


unaizorrilla · 2018-12-09T10:54:29Z

src/HealthChecks.AzureServiceBus/AzureEventHubHealthCheck.cs

@@ -13,8 +13,11 @@ public class AzureEventHubHealthCheck
        private readonly string _eventHubName;
        public AzureEventHubHealthCheck(string connectionString, string eventHubName)
        {
-            _connectionString = connectionString ?? throw new ArgumentNullException(nameof(connectionString));
-            _eventHubName = eventHubName ?? throw new ArgumentNullException(nameof(eventHubName));


probably mix features on different package is not a good idea for tracking project. Can you split in two different pull requests

New tests can't pass without this changes to the package. :/

unaizorrilla · 2018-12-09T10:54:44Z

test/UnitTests/DependencyInjection/HealthChecks.AzureKeyVault/AzureKeyVaultUnitTests.cs

+
+            registration.Name.Should().Be("azurekeyvault");
+            check.GetType().Should().Be(typeof(AzureKeyVaultHealthCheck));
+


blank lines

unaizorrilla · 2018-12-09T10:54:59Z

test/UnitTests/DependencyInjection/HeathChecks.AzureServiceBus/AzureServiceBusQueueUnitTests.cs

+
+            registration.Name.Should().Be("azurequeue");
+            check.GetType().Should().Be(typeof(AzureServiceBusQueueHealthCheck));
+


blank lines

unaizorrilla · 2018-12-09T10:55:26Z

test/UnitTests/DependencyInjection/HeathChecks.AzureServiceBus/AzureServiceBusQueueUnitTests.cs

+        {
+            var services = new ServiceCollection();
+            services.AddHealthChecks()
+                .AddAzureServiceBusQueue("", "");


use string.empty

unaizorrilla · 2018-12-09T10:57:38Z

Hi Carlos,
Thanks for summitting this PR, I review this and you need to fix some minor issues. Thanks for starting the new tests project for check registered healthchecks.

Some items to review:

Please add support for Azure MSI into the KeyVaultPackage
Review minor issues (backets and blank lines)
Split into two PR the fix for Servicebus HealthChecks preconditions changes

…e Review

omerlh · 2018-12-13T10:49:10Z

What about checking also keys that should exist?

CarlosLanderas · 2018-12-13T10:53:07Z

That is on the PR as well. Using the AddSecret fluent api

omerlh · 2018-12-13T11:15:51Z

I was talking about KeyVault encryption keys - it's a different feature of KeyVault. I looked on the code, look like it's not checked...

CarlosLanderas · 2018-12-13T11:52:13Z

That's not added. The idea was checking the existing secrets that the application might need. Feel free to add it if you consider :)

omerlh · 2018-12-13T11:53:28Z

Can I add it to the existing PR? Or on a different one?

CarlosLanderas · 2018-12-13T11:55:40Z

You can use the existing one

omerlh · 2018-12-13T15:36:55Z

How can I build the project on mac? Should I set env var like NetStandardTargetVersion?

unaizorrilla · 2018-12-13T16:48:59Z

Hi @omerlh

Right now on master, you can create a new PR for new features if you want!

unaizorrilla · 2018-12-13T16:49:25Z

Hi @CarlosLanderas

Thanks for this, PR is merged on master with some minor details

Azure Key Vault Health check package

20c496b

CarlosLanderas requested a review from unaizorrilla December 6, 2018 23:36

CarlosLanderas added 4 commits December 7, 2018 16:19

Add unit test project with azure key vault registration tests

9efb7a1

Add unit tests execution to build.ps1

36d8a9d

Add Azure EventHub, Service bus queue and topic DI unit tests and che…

3a2828e

…ck for empty ctor params

Merge master branch and fix dependencies.props conflict

560b44a

unaizorrilla reviewed Dec 9, 2018

View reviewed changes

CarlosLanderas force-pushed the azure-keyvault-healthcheck branch from 99e8b44 to 560b44a Compare December 9, 2018 16:15

CarlosLanderas added 2 commits December 9, 2018 17:24

Add support for Azure Key Vault with Managed Service Identity and Cod…

3ceb2c6

…e Review

Fix typo

6e95b81

CarlosLanderas changed the title ~~Azure Key Vault Health check package~~ Azure Key Vault Health check package and Unit tests project Dec 9, 2018

CarlosLanderas mentioned this pull request Dec 13, 2018

Add support for Azure KeyVault health check #27

Closed

unaizorrilla merged commit 6e95b81 into master Dec 13, 2018

CarlosLanderas deleted the azure-keyvault-healthcheck branch June 30, 2019 18:31

Riff451 mentioned this pull request Dec 16, 2019

Add Azure KeyVault cryptographic keys check #368

Merged

sungam3r mentioned this pull request Jul 30, 2023

V7 Series Release! #1593

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Azure Key Vault Health check package and Unit tests project #19

Azure Key Vault Health check package and Unit tests project #19

CarlosLanderas commented Dec 6, 2018 •

edited

Loading

unaizorrilla Dec 9, 2018

CarlosLanderas Dec 9, 2018

unaizorrilla Dec 9, 2018

unaizorrilla Dec 9, 2018

unaizorrilla Dec 9, 2018

unaizorrilla Dec 9, 2018

unaizorrilla Dec 9, 2018

unaizorrilla Dec 9, 2018

CarlosLanderas Dec 9, 2018

unaizorrilla Dec 9, 2018

unaizorrilla Dec 9, 2018

unaizorrilla Dec 9, 2018

unaizorrilla commented Dec 9, 2018

omerlh commented Dec 13, 2018

CarlosLanderas commented Dec 13, 2018

omerlh commented Dec 13, 2018

CarlosLanderas commented Dec 13, 2018

omerlh commented Dec 13, 2018

CarlosLanderas commented Dec 13, 2018

omerlh commented Dec 13, 2018

unaizorrilla commented Dec 13, 2018

unaizorrilla commented Dec 13, 2018


		registration.Name.Should().Be("azurekeyvault");
		check.GetType().Should().Be(typeof(AzureKeyVaultHealthCheck));


		registration.Name.Should().Be("azurequeue");
		check.GetType().Should().Be(typeof(AzureServiceBusQueueHealthCheck));

Azure Key Vault Health check package and Unit tests project #19

Azure Key Vault Health check package and Unit tests project #19

Conversation

CarlosLanderas commented Dec 6, 2018 • 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

unaizorrilla commented Dec 9, 2018

omerlh commented Dec 13, 2018

CarlosLanderas commented Dec 13, 2018

omerlh commented Dec 13, 2018

CarlosLanderas commented Dec 13, 2018

omerlh commented Dec 13, 2018

CarlosLanderas commented Dec 13, 2018

omerlh commented Dec 13, 2018

unaizorrilla commented Dec 13, 2018

unaizorrilla commented Dec 13, 2018

CarlosLanderas commented Dec 6, 2018 •

edited

Loading