Support managed identity for connecting to storage #87
Conversation
| "resources": [ | ||
| { | ||
| "type": "Microsoft.Storage/storageAccounts/providers/roleAssignments", | ||
| "apiVersion": "2020-03-01-preview", |
There was a problem hiding this comment.
Do we need to use the preview API? #Resolved
There was a problem hiding this comment.
Can you try 2020-04-08? That seems like the latest GA version.
In reply to: 564892943 [](ancestors = 564892943)
There was a problem hiding this comment.
I tried 2020-04-08 but it said that version didn't support role assignments, so will instead go with the latest non-preview version which is 2018-07-01
Status Message: No registered resource provider found for location 'northcentralus' and API version '2020-04-08' for
type 'roleAssignments'. The supported api-versions are '2014-04-01-preview, 2014-07-01-preview, 2014-10-01-preview,
2015-05-01-preview, 2015-06-01, 2015-07-01, 2016-07-01, 2017-05-01, 2017-09-01, 2017-10-01-preview,
2018-01-01-preview, 2018-07-01, 2018-09-01-preview, 2018-12-01-preview, 2019-04-01-preview, 2020-03-01-preview,
2020-04-01-preview'. #Resolved
There was a problem hiding this comment.
Thanks for checking Will. We should probably follow up with the Azure Storage team. We don't want to take a dependency on an API in preview if we can avoid it. Would you be able to create a work item for tracking?
In reply to: 567489805 [](ancestors = 567489805)
There was a problem hiding this comment.
Nevermind I misread, it looks like 2018-07-01 works, thanks for the update.
In reply to: 569032564 [](ancestors = 569032564,567489805)
src/lib/Microsoft.Health.Events/EventCheckpointing/StorageCheckpointClient.cs
Outdated
Show resolved
Hide resolved
src/lib/Microsoft.Health.Events/EventCheckpointing/StorageCheckpointClient.cs
Outdated
Show resolved
Hide resolved
src/lib/Microsoft.Health.Common/Auth/Provider/AzureCredentialProvider.cs
Show resolved
Hide resolved
| var ex = new Exception($"Unable to create blob container client for {blobUri}"); | ||
| log.LogError(ex); | ||
| throw ex; | ||
| } |
There was a problem hiding this comment.
Can this be internalized into the credential provider itself? I.E. creating a storage client for the provider should be a responsibility outside the StorageCheckpointClient and this class should just ask for the provider. This refactor is a large improvement though! #Closed
There was a problem hiding this comment.
Thanks for the feedback. Went with a factory approach and now pass a storage client in as an argument. #Closed
src/lib/Microsoft.Health.Common/Auth/Service/ICredentialOptions.cs
Outdated
Show resolved
Hide resolved
src/lib/Microsoft.Health.Common/Auth/Provider/AzureCredentialProvider.cs
Outdated
Show resolved
Hide resolved
|
|
||
| var storageOptions = new StorageCheckpointOptions(); | ||
| config.GetSection(StorageCheckpointOptions.Settings).Bind(storageOptions); | ||
| storageOptions.BlobPrefix = eventHub; |
There was a problem hiding this comment.
I wonder is there is a pattern here that would provide more flexibility in the future. What about creating an interface IStorageCheckpointClientProvider that has a GetClient(IConfiguration config). This would allow for different implementations of IStorageCheckpointClientProvider, the default could be for MI, but a consumer could provide an implementation that still uses a connection string.
This will provide flexibility by allowing consumers to create their own appsettings.json schema that works with their own implementation of IStorageCheckpointClientProvider
This pattern could be used for other clients.
There was a problem hiding this comment.
Yeah, that is a good idea. I ended up moving things around which should allow the customer to have more flexibility. I decided to create a BlobContainerClientFactory which can take an overload of IAzureCredentialProvider which can construct to blob client with some custom credentials implementation.
BlobContainerClientFactory will also by default support default azure credential and connection string, and the user can toggle between those by changing the config environment variables.
src/lib/Microsoft.Health.Common/Storage/BlobContainerClientFactory.cs
Outdated
Show resolved
Hide resolved
src/lib/Microsoft.Health.Events/EventCheckpointing/StorageCheckpointClient.cs
Show resolved
Hide resolved
dustinburson
deleted the
personal/wiyochum/inject-logging-and-managed-id
branch
This PR uses managed identity to connect to storage rather than connection strings.