-
Notifications
You must be signed in to change notification settings - Fork 285
Update variable replacement during deserialization to use replacement settings class and add AKV replacement logic. #2882
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still need to get rid of envVar in some places and resolve missing logic inside of an if statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good to me. This is a good refactor. However, unit testing still remains and so does the test in AKV environment.
| { | ||
| Assert.IsTrue(RuntimeConfigLoader.TryParseConfig( | ||
| GetModifiedJsonString(repValues, @"""postgresql"""), out expectedConfig, replaceEnvVar: replaceEnvVar), | ||
| GetModifiedJsonString(repValues, @"""postgresql"""), out expectedConfig, replacementSettings: new DeserializationVariableReplacementSettings(azureKeyVaultOptions: null, doReplaceEnvVar: replaceEnvVar, doReplaceAKVVar: true)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add similar tests for AKV replacement checks
src/Service.Tests/UnitTests/PostgreSqlQueryExecutorUnitTests.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the configuration deserialization system to introduce Azure Key Vault (AKV) variable replacement support alongside environment variable replacement. The key change is replacing individual boolean parameters (replaceEnvVar, replacementFailureMode) with a unified DeserializationVariableReplacementSettings object that encapsulates all variable replacement configuration, including the new AKV functionality.
- Introduces
DeserializationVariableReplacementSettingsclass to centralize variable replacement configuration - Adds Azure Key Vault secret resolution support with configurable retry policies
- Updates all JSON converters and configuration loading methods to use the new settings object
Reviewed Changes
Copilot reviewed 33 out of 33 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
src/Config/DeserializationVariableReplacementSettings.cs |
New class that encapsulates variable replacement logic for both environment variables and Azure Key Vault secrets |
src/Config/RuntimeConfigLoader.cs |
Updated to use new settings object and extract AKV options with two-pass parsing |
src/Config/Converters/StringJsonConverterFactory.cs |
Refactored to use pluggable replacement strategies from settings object |
src/Config/Converters/*ConverterFactory.cs |
Updated multiple converter factories to accept DeserializationVariableReplacementSettings instead of boolean parameters |
src/Config/ObjectModel/AzureKeyVaultOptions.cs |
Added constructor and flags to track user-provided properties |
src/Directory.Packages.props |
Added Azure.Security.KeyVault.Secrets package dependency |
src/Service/Controllers/ConfigurationController.cs |
Updated Initialize call with new settings object |
| Test files | Updated test code to use new settings object instead of boolean parameters |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/Config/Converters/DatasourceHealthOptionsConvertorFactory.cs
Outdated
Show resolved
Hide resolved
src/Config/Converters/DatasourceHealthOptionsConvertorFactory.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to carefully look at what the default value of replaceEnvVar property should be when replaceMentSettings is null or new(). And ensure no regressions
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
|
/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)