Added query param support and validation for export request. #438
Conversation
…he code and added corresponding UTs and E2E tests. Added ISecretStore and corresponding Azure KeyVault based implementation.
namadabu
force-pushed
the
personal/namadabu/params-support-for-export
branch
from
50e66f2 to
b86bfff
Compare
src/Microsoft.Health.Fhir.Api/Features/Filters/ValidateExportRequestFilterAttribute.cs
Show resolved
Hide resolved
src/Microsoft.Health.Fhir.Api/Features/Filters/ValidateExportRequestFilterAttribute.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Health.Fhir.Core/Features/Operations/Export/Models/ExportJobRecord.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Health.Fhir.Core/Features/SecretStore/InMemorySecretStore.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Health.Fhir.Core/Features/SecretStore/InMemorySecretStore.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Health.Fhir.Core/Features/SecretStore/InMemorySecretStore.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Health.Fhir.Core/Features/SecretStore/SecretWrapper.cs
Outdated
Show resolved
Hide resolved
...t.Health.Fhir.SecretStore/Registration/FhirServerBuilderSecretStoreRegistrationExtensions.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Health.Fhir.Core.UnitTests/Features/Export/CreateExportRequestHandlerTests.cs
Outdated
Show resolved
Hide resolved
| @@ -11,5 +13,10 @@ public class ExportConfiguration | |||
| /// Determines whether export is enabled or not. | |||
| /// </summary> | |||
| public bool Enabled { get; set; } | |||
|
|
|||
There was a problem hiding this comment.
I wonder if this needs to be explicit. E.g., if we have an interface and list of providers that implement the interface, then we can query the interface and see what detination it supports and it can be automatically registered through IoC container as opposed to register the code in IoC container and also update the settings to enable it.
| { | ||
| if (!_exportConfig.Enabled) | ||
| { | ||
| throw new RequestNotValidException(string.Format(Resources.UnsupportedOperation, OperationsConstants.Export)); | ||
| } | ||
|
|
||
| CreateExportResponse response = await _mediator.ExportAsync(_fhirRequestContextAccessor.FhirRequestContext.Uri); | ||
| CreateExportResponse response = await _mediator.ExportAsync(_fhirRequestContextAccessor.FhirRequestContext.Uri, destinationType, destinationConnectionString); |
There was a problem hiding this comment.
this could just be passed in as DestinationInfo?
There was a problem hiding this comment.
Wanted to keep DestinationInfo usage within Fhir.Core and avoid using it in Fhir.Api. I don't think it makes a huge difference either way.
src/Microsoft.Health.Fhir.Core/Features/Operations/Export/Models/DestinationInfo.cs
Show resolved
Hide resolved
src/Microsoft.Health.Fhir.Core/Features/SecretStore/InMemorySecretStore.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Health.Fhir.Core/Messages/Export/CreateExportRequest.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Health.Fhir.SecretStore/Configs/KeyVaultConfiguration.cs
Outdated
Show resolved
Hide resolved
...t.Health.Fhir.SecretStore/Registration/FhirServerBuilderSecretStoreRegistrationExtensions.cs
Outdated
Show resolved
Hide resolved
...t.Health.Fhir.SecretStore/Registration/FhirServerBuilderSecretStoreRegistrationExtensions.cs
Outdated
Show resolved
Hide resolved
| EnsureArg.IsNotNullOrWhiteSpace(secretName, nameof(secretName)); | ||
| EnsureArg.IsNotNullOrWhiteSpace(secretValue, nameof(secretValue)); | ||
|
|
||
| SecretBundle result = await _keyVaultClient.SetSecretAsync(_keyVaultUri.AbsoluteUri, secretName, secretValue); |
There was a problem hiding this comment.
Doesn't have to be now but we should also think about how to clean up these secrets after a while.
| EnsureArg.IsNotNull(configuration, nameof(configuration)); | ||
|
|
||
| var keyVaultConfig = new KeyVaultConfiguration(); | ||
| configuration.GetSection(KeyVaultConfigurationName).Bind(keyVaultConfig); |
There was a problem hiding this comment.
This whole section could be missing (e.g., KeyVault section is not in the config at all).
There was a problem hiding this comment.
Added some comments here. Also renamed InMemorySecretStore to InMemoryKeyVaultSecretStore and moved it to the Fhir.KeyVault project to make things clearer and avoid semantic confusion.
| @@ -25,7 +27,8 @@ public ExportJobRecord(Uri exportRequestUri) | |||
| SchemaVersion = 1; | |||
| Status = OperationStatus.Queued; | |||
| Id = Guid.NewGuid().ToString(); | |||
| QueuedTime = DateTimeOffset.UtcNow; | |||
| SecretName = SecretPrefix + Id; | |||
There was a problem hiding this comment.
We should think about this a bit more. Currently, I am planing to implement the dedup logic in the stored procedure which means in the case where there is matching job, the existing job will be returned, which would contain a different id than the one generated here (I was also going to move the id generation to the stored procedure).
There was a problem hiding this comment.
You don't need to change this right now in this PR.
| return new SecretWrapper(result.Id, result.Value); | ||
| } | ||
|
|
||
| public async Task<SecretWrapper> DeleteSecretAsync(string secretName) |
There was a problem hiding this comment.
I think we can add the logic in the worker to delete the secret once the job is finished (completed, failed, or cancelled). It could be best effort type of thing so if it fails, it fails. We can think about a separate process to go and clean up afterwards.
There was a problem hiding this comment.
It will be implemented when actually implementing the task.
Description
Added query param support and validation for export request. We support (and require) _destinationType and _destinationConnectionSettings. Added a new ISecretStore and corresponding Azure KeyVault based implementation to store this information.
Related issues
Addresses [issue AB#68715].
Testing
Existing and new UTs, E2E tests. Used KeyVault deployed to Azure to test confirm whether secrets are being stored.