-
Notifications
You must be signed in to change notification settings - Fork 289
Support for .akv files to AKV variable replacement. #2957
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
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.
Pull Request Overview
This PR refactors the configuration deserialization variable replacement mechanism and adds Azure Key Vault (AKV) variable replacement support. The changes replace boolean parameters with a structured DeserializationVariableReplacementSettings object that supports both environment variable and Azure Key Vault variable replacement, including local .akv file support.
Key changes:
- Introduced
DeserializationVariableReplacementSettingsclass to encapsulate variable replacement logic - Added
AzureKeyVaultOptionsConverterFactoryfor proper AKV configuration deserialization - Updated
AzureKeyVaultOptionswith constructor and user-provided flags for proper serialization - Replaced
replaceEnvVarboolean parameters withreplacementSettingsthroughout the codebase
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Config/DeserializationVariableReplacementSettings.cs | New class implementing variable replacement logic for both environment variables and Azure Key Vault secrets |
| src/Config/RuntimeConfigLoader.cs | Updated method signatures to use new settings object; added two-pass parsing for AKV options extraction |
| src/Config/ObjectModel/AzureKeyVaultOptions.cs | Added constructor and user-provided flags to control serialization behavior |
| src/Config/Converters/StringJsonConverterFactory.cs | Refactored to use replacement settings object and apply multiple replacement strategies |
| src/Config/Converters/AzureKeyVaultOptionsConverterFactory.cs | New converter factory for proper AKV options deserialization with variable replacement |
| src/Config/FileSystemRuntimeConfigLoader.cs | Updated to use replacement settings with backward compatibility support |
| src/Core/Configurations/RuntimeConfigProvider.cs | Updated Initialize method signature to accept replacement settings |
| src/Service/Controllers/ConfigurationController.cs | Updated to instantiate replacement settings with explicit parameters |
| src/Directory.Packages.props | Added Azure.Security.KeyVault.Secrets package dependency |
| Multiple converter files | Updated to accept and use replacement settings instead of boolean flag |
| Multiple test files | Updated to use new API with explicit or default-constructed replacement settings |
| src/Service.Tests/UnitTests/RuntimeConfigLoaderJsonDeserializerTests.cs | Added new test for AKV variable replacement from local file; removed unused import |
| src/Cli/ConfigGenerator.cs | Updated AKV options construction to use new constructors with proper flag setting |
Comments suppressed due to low confidence (5)
src/Service.Tests/UnitTests/RuntimeConfigLoaderJsonDeserializerTests.cs:92
- Setting
doReplaceAkvVar: truewithout providingazureKeyVaultOptionsis inconsistent. WhenazureKeyVaultOptionsis null, AKV replacement won't occur regardless of the flag (see line 72 of DeserializationVariableReplacementSettings.cs). Consider settingdoReplaceAkvVar: falsefor clarity, or add a comment explaining why this is intentional.
GetModifiedJsonString(repKeys, @"""@env('enumVarName')"""), out RuntimeConfig actualConfig, replacementSettings: new DeserializationVariableReplacementSettings(azureKeyVaultOptions: null, doReplaceEnvVar: replaceEnvVar, doReplaceAkvVar: true)),
src/Service.Tests/UnitTests/RuntimeConfigLoaderJsonDeserializerTests.cs:132
- Setting
doReplaceAkvVar: truewithout providingazureKeyVaultOptionsis inconsistent. WhenazureKeyVaultOptionsis null, AKV replacement won't occur regardless of the flag (see line 72 of DeserializationVariableReplacementSettings.cs). Consider settingdoReplaceAkvVar: falsefor clarity.
configWithEnvVar, out RuntimeConfig runtimeConfig, replacementSettings: new DeserializationVariableReplacementSettings(azureKeyVaultOptions: null, doReplaceEnvVar: replaceEnvVar, doReplaceAkvVar: true));
src/Service.Tests/UnitTests/RuntimeConfigLoaderJsonDeserializerTests.cs:180
- Setting
doReplaceAkvVar: truewithout providingazureKeyVaultOptionsis inconsistent. WhenazureKeyVaultOptionsis null, AKV replacement won't occur regardless of the flag (see line 72 of DeserializationVariableReplacementSettings.cs). Consider settingdoReplaceAkvVar: falsefor clarity.
configWithEnvVar, out RuntimeConfig runtimeConfig, replacementSettings: new DeserializationVariableReplacementSettings(azureKeyVaultOptions: null, doReplaceEnvVar: true, doReplaceAkvVar: true));
src/Config/DeserializationVariableReplacementSettings.cs:148
- Both branches of this 'if' statement return - consider using '?' to express intent better.
if (EnvFailureMode is EnvironmentVariableReplacementFailureMode.Throw)
{
return value is not null ? value :
throw new DataApiBuilderException(
message: $"Environmental Variable, {name}, not found.",
statusCode: System.Net.HttpStatusCode.ServiceUnavailable,
subStatusCode: DataApiBuilderException.SubStatusCodes.ErrorInInitialization);
}
else
{
return value ?? match.Value;
}
src/Config/DeserializationVariableReplacementSettings.cs:166
- Both branches of this 'if' statement return - consider using '?' to express intent better.
if (EnvFailureMode == EnvironmentVariableReplacementFailureMode.Throw)
{
return value is not null ? value :
throw new DataApiBuilderException(message: $"Azure Key Vault Variable, '{name}', not found.",
statusCode: System.Net.HttpStatusCode.ServiceUnavailable,
subStatusCode: DataApiBuilderException.SubStatusCodes.ErrorInInitialization);
}
else
{
return value ?? match.Value;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/Service.Tests/UnitTests/RuntimeConfigLoaderJsonDeserializerTests.cs
Outdated
Show resolved
Hide resolved
src/Service.Tests/UnitTests/RuntimeConfigLoaderJsonDeserializerTests.cs
Outdated
Show resolved
Hide resolved
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
souvikghosh04
left a comment
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.
LGTM
The base branch was changed.
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
1e10826 to
64658cb
Compare
|
/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
src/Service.Tests/UnitTests/RuntimeConfigLoaderJsonDeserializerTests.cs
Outdated
Show resolved
Hide resolved
Aniruddh25
left a comment
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.
LGTM, left a few suggestions.
…rTests.cs Co-authored-by: Aniruddh Munde <[email protected]>
…rTests.cs Co-authored-by: Aniruddh Munde <[email protected]>
…m:Azure/data-api-builder into dev/aaronburtle/AKVReplacementFileSupport
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
souvikghosh04
left a comment
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 couple of comments but those are easily addressable. Approving this as the rest all looks good.
src/Service.Tests/UnitTests/RuntimeConfigLoaderJsonDeserializerTests.cs
Outdated
Show resolved
Hide resolved
…m:Azure/data-api-builder into dev/aaronburtle/AKVReplacementFileSupport
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
Why make this change?
Closes #2748
What is this change?
Adds the option to use a local .akv file instead of Azure Key Vault for @akv('') replacement in the config file during deserialization. Similar to how we handle .env files.
How was this tested?
A new test was added that verifies we are able to do the replacement and get the correct resultant configuration.