Cosmos managed Identity Authentication Changes #3950
Conversation
v-ajajvanu
added
the
Azure API for FHIR
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…agedidentity merge from main
…agedidentity i merge from main# the commit.
| private Task _backgroundLoopTask; | ||
|
|
||
| public CosmosDbCollectionPhysicalPartitionInfo( | ||
| IAccessTokenProvider aadTokenProvider, |
There was a problem hiding this comment.
What "add" in "aadTokenProvider" stands for?
There was a problem hiding this comment.
@abiisnn "aad" stands here for Azure Active Directory.
| string key = _dataStoreConfiguration.Key; | ||
|
|
||
| if (string.IsNullOrWhiteSpace(host)) | ||
| { | ||
| if (string.IsNullOrWhiteSpace(key)) | ||
| { | ||
| host = CosmosDbLocalEmulator.Host; | ||
| key = CosmosDbLocalEmulator.Key; | ||
| } | ||
| else | ||
| { | ||
| Ensure.That(host, $"{nameof(CosmosDataStoreConfiguration)}.{nameof(CosmosDataStoreConfiguration.Host)}").IsNotNullOrEmpty(); | ||
| } | ||
| } | ||
| else if (string.IsNullOrWhiteSpace(key)) | ||
| { | ||
| Ensure.That(key, $"{nameof(CosmosDataStoreConfiguration)}.{nameof(CosmosDataStoreConfiguration.Key)}").IsNotNullOrEmpty(); | ||
| } |
There was a problem hiding this comment.
I think we should keep the same behavior but using the new tokenProvider.
We might need to add a new variable called "token" in CosmosDBLocalEmuletor and make sure we clean up "key" in case it is not needed anymore. If "token" is not longer needed (because now you use new DefaultAzureCredential()), then can we clean up all key references?
I think we can keep the same structure because Ensure.That() will throw an exception in case host or token are null or white space, as now, it will just check host and not token.
There was a problem hiding this comment.
@abiisnn We had removed "Key" reference from everywhere.
Also, in CosmosDBLocalEmulator class, we do not need token property as we are using DefaultAzureCredentials, and it generates token internally.
| @@ -186,6 +164,23 @@ private static string GenerateAuthToken(string verb, string resourceType, string | |||
| return $"type=master&ver=1.0&sig={signature}"; | |||
| } | |||
|
|
|||
| private async Task<string> GenerateAccessToken(string host, CancellationToken cancellationToken) | |||
There was a problem hiding this comment.
Can we check that host is not null, empty or white space?
There was a problem hiding this comment.
@abiisnn We are already checking for "host" as not null and empty or white space in line number 101.
Only after the check is successful, GenerateAccessToken function will get invoked.
|
@abiisnn We had worked on the suggestions provided and changes are committed from our end. If possible, could you please review the same and provide feedbacks. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Covered by changes in #4026. The investigation/changes for |
Description
Made changes to support Managed Identity Authentication for Cosmos DB instead of using Secrets.
Related issues
user story AB#122541
Testing
Tested by creating FHIR server instance with code running from publishing new images to ACR that contain managed identity authentication changes.
FHIR Team Checklist
Semver Change (docs)
Patch|Skip|Feature|Breaking (reason)