Update variable replacement during deserialization to use replacement settings class and add AKV replacement logic. #2882
Conversation
RubenCerna2079
left a comment
RubenCerna2079
left a comment
There was a problem hiding this comment.
Still need to get rid of envVar in some places and resolve missing logic inside of an if statement.
Aniruddh25
left a comment
Aniruddh25
left a comment
There was a problem hiding this comment.
The changes look good to me. This is a good refactor. However, unit testing still remains and so does the test in AKV environment.
src/Service.Tests/UnitTests/RuntimeConfigLoaderJsonDeserializerTests.cs
Outdated
Show resolved
Hide resolved
src/Service.Tests/UnitTests/PostgreSqlQueryExecutorUnitTests.cs
Outdated
Show resolved
Hide resolved
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
src/Service.Tests/UnitTests/RuntimeConfigLoaderJsonDeserializerTests.cs
Outdated
Show resolved
Hide resolved
| // This pattern greedy matches all characters that are not a part of @env() | ||
| // ie: @env('hello@env('goodbye')world') match: 'hello@env('goodbye')world' | ||
| public const string INNER_ENV_PATTERN = @"[^@env\(].*(?=\))"; | ||
| public const string INNER_AKV_PATTERN = @"[^@AKV\(].*(?=\))"; |
There was a problem hiding this comment.
this should be akv in lower case as per the enhancement. same comment for the outer pattern.
| private string ReplaceAkvVariable(Match match) | ||
| { | ||
| // strips first and last characters, ie: '''hello'' --> ''hello' | ||
| string name = Regex.Match(match.Value, INNER_AKV_PATTERN).Value[1..^1]; |
There was a problem hiding this comment.
the name of the AKV variable here should follow certain rules as per the enhancement: #2708
Look at section: Key rules (based on AKV docs)
|
|
||
| private static SecretClient CreateSecretClient(AzureKeyVaultOptions options) | ||
| { | ||
| if (string.IsNullOrWhiteSpace(options.Endpoint)) |
There was a problem hiding this comment.
Need to test if child configs do infact inherit the akv endpoint values from the parent config - see the multiple configs section of the enhancement.
| ReplaceEnvVariable); | ||
| } | ||
|
|
||
| if (DoReplaceAkvVar && _azureKeyVaultOptions is not null) |
There was a problem hiding this comment.
A consideration in the enhancement is to add an OTEL activity wrapping the replacement of the AKV variable. Can you check if this can be done? Could be in a followup task.
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
…a-api-builder into dev/aaronburtle/AKVReplacement
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
Why make this change?
Adds AKV variable replacement and expands our design for doing variable replacements to be more extensible when new variable replacement logic is added.
Closes #2708
Closes #2748
Related to #2863
What is this change?
Change the way that variable replacement is handled to instead of simply using a
boolto indicate that we want env variable replacement, we add a class which holds all of the replacement settings. This will hold whether or not we will do replacement for each kind of variable that we will handle replacement for during deserialization. We also include the replacement failure mode, and put the logic for handling the replacements into a strategy dictionary which pairs the replacement variable type with the strategy for doing that replacement.Because Azure Key Vault secret replacement requires having the retry and connection settings in order to do the AKV replacement, we must do a first pass where we only do non-AKV replacement and get the required settings so that if AKV replacement is used we have the required settings to do that replacement.
We also have to keep in mind that the legacy of the
Configuration Controllerwill ignore all variable replacement, so we construct the replacement settings for this code path to not use any variable replacement at all.How was this tested?
We have updated the logic for the tests to use the new system, however manual testing using an actual AKV is still required.
Sample Request(s)