Update variable replacement during deserialization to use replacement settings class and add AKV replacement logic. #2882

aaronburtle · 2025-09-18T01:39:47Z

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 bool to 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 Controller will 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)

Example REST and/or GraphQL request to demonstrate modifications
Example of CLI usage to demonstrate modifications

RubenCerna2079 · 2025-10-03T21:29:39Z

src/Config/Converters/AKVRetryPolicyOptionsConverterFactory.cs

-    // Determines whether to replace environment variable with its
-    // value or not while deserializing.
-    private bool _replaceEnvVar;
+    // Settings for variable replacement during deserialization.


Maybe make the comment a bit more descriptive, to show that it can replace the variable with either AKV, an environment variable, or nothing during deserialization.

RubenCerna2079 · 2025-10-03T21:32:35Z

src/Config/Converters/AKVRetryPolicyOptionsConverterFactory.cs

-        // Determines whether to replace environment variable with its
-        // value or not while deserializing.
-        private bool _replaceEnvVar;
+        // Settings for variable replacement during deserialization.


Same as comment above, this applies to all the places where the variable type was changed

RubenCerna2079 · 2025-10-03T21:35:52Z

src/Config/Converters/EntityCacheOptionsConverterFactory.cs

    /// <param name="replaceEnvVar">Whether to replace environment variable with its
    /// value or not while deserializing.</param>
-    internal EntityCacheOptionsConverterFactory(bool replaceEnvVar)
+    internal EntityCacheOptionsConverterFactory(DeserializationVariableReplacementSettings? replacementSettings)


The param name comment should also be changed.

RubenCerna2079 · 2025-10-03T21:36:01Z

src/Config/Converters/EntityCacheOptionsConverterFactory.cs

        /// <param name="replaceEnvVar">Whether to replace environment variable with its
        /// value or not while deserializing.</param>
-        public EntityCacheOptionsConverter(bool replaceEnvVar)
+        public EntityCacheOptionsConverter(DeserializationVariableReplacementSettings? replacementSettings)


The param name comment should also be changed.

RubenCerna2079 · 2025-10-06T23:48:11Z

src/Config/Converters/StringJsonConverterFactory.cs

+                foreach (KeyValuePair<Regex, Func<Match, string>> strategy in _replacementSettings.ReplacementStrategies)
+                {
+                    value = strategy.Key.Replace(value, new MatchEvaluator(strategy.Value));
+                }
+
+                return value;


I am confused with how this section works, wouldn't this just give you the last replacement strategy since they are all saved in the same value?

RubenCerna2079 · 2025-10-07T00:05:30Z

src/Config/FileSystemRuntimeConfigLoader.cs


-            if (!string.IsNullOrEmpty(json) && TryParseConfig(json, out RuntimeConfig, connectionString: _connectionString, replaceEnvVar: replaceEnvVar))
+            if (!string.IsNullOrEmpty(json) && TryParseConfig(json, out RuntimeConfig, 
+                new DeserializationVariableReplacementSettings(azureKeyVaultOptions: null, doReplaceEnvVar: true, doReplaceAKVVar: true), logger: null, connectionString: _connectionString))


We also need to delete the parameter replaceEnvVar from the TryLoadConfig function since we don't use it anymore

RubenCerna2079 · 2025-10-07T00:14:09Z

src/Config/RuntimeConfigLoader.cs

+        // Add environment variable replacement if enabled
+        if (enableEnvReplacement)
+        {
+


I assume that there must be something missing inside this if

RubenCerna2079

Still need to get rid of envVar in some places and resolve missing logic inside of an if statement.

RubenCerna2079 · 2025-10-07T18:43:43Z

src/Config/Converters/AzureKeyVaultOptionsConverterFactory.cs

+                            break;
+
+                        default:
+                            reader.Skip();


Why do we want to skip an incorrect property? Shouldn't we want to throw an error?

RubenCerna2079 · 2025-10-07T18:44:54Z

src/Config/Converters/AzureKeyVaultOptionsConverterFactory.cs

+                    }
+                }
+            }
+            else if (reader.TokenType is JsonTokenType.Null)


nit: I think it would be better to put this on top so it is consistent with other ConverterFactories

RubenCerna2079 · 2025-10-07T18:59:47Z

src/Config/Converters/StringJsonConverterFactory.cs

            {
                string? value = reader.GetString();
-                return Regex.Replace(value!, ENV_PATTERN, new MatchEvaluator(ReplaceMatchWithEnvVariable));
+                if (string.IsNullOrEmpty(value))


Is it possible for the value to be white spaces? If so I it would be better to use IsNullOrWhiteSpace

RubenCerna2079 · 2025-10-07T20:21:51Z

src/Config/Converters/Utf8JsonReaderExtensions.cs

    public static string? DeserializeString(this Utf8JsonReader reader,
-        bool replaceEnvVar,
-        EnvironmentVariableReplacementFailureMode replacementFailureMode = EnvironmentVariableReplacementFailureMode.Throw)
+        DeserializationVariableReplacementSettings? replacementSettings)


Need to update the parameters in the summary section

RubenCerna2079 · 2025-10-07T20:23:42Z

src/Config/RuntimeConfigLoader.cs

 #pragma warning disable CA1024 // Use properties where appropriate
    public IChangeToken GetChangeToken()
-#pragma warning restore CA1024 // Use properties where appropriate
+#pragma warning restore CA1024 // Use properties where Appropriate


nit: Change this back to lower case

RubenCerna2079 · 2025-10-07T22:47:44Z

src/Config/DeserializationVariableReplacementSettings.cs

+                clientOptions.Retry.MaxRetries = options.RetryPolicy.MaxCount ?? 3;
+                clientOptions.Retry.Delay = TimeSpan.FromSeconds(options.RetryPolicy.DelaySeconds ?? 1);
+                clientOptions.Retry.MaxDelay = TimeSpan.FromSeconds(options.RetryPolicy.MaxDelaySeconds ?? 16);
+                clientOptions.Retry.NetworkTimeout = TimeSpan.FromSeconds(options.RetryPolicy.NetworkTimeoutSeconds ?? 30);


Shouldn't we use the DEFAULT variables found in AKVRetryPolicyOptions instead of using numbers?

RubenCerna2079 · 2025-10-07T23:13:19Z

src/Config/RuntimeConfigLoader.cs

+        catch
+        {
+            // If we can't extract AKV options, return null and proceed without AKV variable replacement
+            return null;
+        }
+
+        return null;


nit: Returning the nulls like this seems a bit redundant, do you think there is a better way to do this?

RubenCerna2079 · 2025-10-07T23:17:25Z

src/Config/DeserializationVariableReplacementSettings.cs

+    public class DeserializationVariableReplacementSettings
+    {
+        public bool DoReplaceEnvVar { get; set; } = true;
+        public bool DoReplaceAKVVar { get; set; } = true;


Suggested change

public bool DoReplaceAKVVar { get; set; } = true;

public bool DoReplaceAkvVar { get; set; } = true;

RubenCerna2079 · 2025-10-07T23:18:33Z

src/Config/RuntimeConfigLoader.cs

+            envFailureMode: replacementFailureMode);
+        options.Converters.Add(new StringJsonConverterFactory(envOnlySettings));


nit:

Suggested change

envFailureMode: replacementFailureMode);

options.Converters.Add(new StringJsonConverterFactory(envOnlySettings));

envFailureMode: replacementFailureMode);

options.Converters.Add(new StringJsonConverterFactory(envOnlySettings));

RubenCerna2079 · 2025-10-07T23:20:59Z

src/Config/RuntimeConfigLoader.cs

+            using JsonDocument doc = JsonDocument.Parse(json);
+            if (doc.RootElement.TryGetProperty("azure-key-vault", out JsonElement akvElement))
+            {
+                return JsonSerializer.Deserialize<AzureKeyVaultOptions>(akvElement.GetRawText(), options);
+            }


Why do we want to deserialize the azure-key-vault here? Don't we already do that with the ConverterFactory?

souvikghosh04

added comments

souvikghosh04 · 2025-09-18T11:20:08Z

src/Config/RuntimeConfigLoader.cs

+        catch
+        {
+            // If we can't extract AKV options, return null and proceed without AKV variable replacement
+            return null;


I don't see anything explicitly handled in catch. It returns null as same outside the catch

souvikghosh04 · 2025-09-18T11:21:04Z

src/Config/RuntimeConfigLoader.cs

                    updatedConnection = GetConnectionStringWithApplicationName(connectionValue);
                }
-                else if (ds.DatabaseType is DatabaseType.PostgreSQL && replaceEnvVar)
+                else if (ds.DatabaseType is DatabaseType.PostgreSQL && replacementSettings?.DoReplaceEnvVar == true)


nit: replacementSettings?.DoReplaceEnvVar is true ?

souvikghosh04 · 2025-10-08T03:14:27Z

src/Config/DeserializationVariableReplacementSettings.cs

+                clientOptions.Retry.Mode = retryMode;
+                clientOptions.Retry.MaxRetries = options.RetryPolicy.MaxCount ?? 3;
+                clientOptions.Retry.Delay = TimeSpan.FromSeconds(options.RetryPolicy.DelaySeconds ?? 1);
+                clientOptions.Retry.MaxDelay = TimeSpan.FromSeconds(options.RetryPolicy.MaxDelaySeconds ?? 16);


should the MaxDelay be more than the NetworkTimeout? also are these configurable or going to be fixed always?

aaronburtle added 12 commits June 20, 2025 15:55

allow unused cache-control options without error

2fc665d

Merge branch 'main' of github.com:Azure/data-api-builder

50e54aa

Merge branch 'main' of github.com:Azure/data-api-builder

455ff18

Merge branch 'main' of github.com:Azure/data-api-builder

e0574bf

Merge branch 'main' of github.com:Azure/data-api-builder

033eeb0

Merge branch 'main' of github.com:Azure/data-api-builder

c7ad08f

...

049b23e

extend variable replacment and include AKV

cfdc2b5

change some logic for first pass var replacement

5eb9f0e

fix old bools being used

bbcc159

more bool replacements

f6f92bd

refactor out bools

0dd7ad6

RubenCerna2079 reviewed Oct 7, 2025

View reviewed changes

RubenCerna2079 requested changes Oct 7, 2025

View reviewed changes

souvikghosh04 requested changes Oct 8, 2025

View reviewed changes

	public bool DoReplaceAKVVar { get; set; } = true;
	public bool DoReplaceAkvVar { get; set; } = true;

		envFailureMode: replacementFailureMode);
		options.Converters.Add(new StringJsonConverterFactory(envOnlySettings));

Update variable replacement during deserialization to use replacement settings class and add AKV replacement logic. #2882

Are you sure you want to change the base?

Update variable replacement during deserialization to use replacement settings class and add AKV replacement logic. #2882

Uh oh!

Conversation

aaronburtle commented Sep 18, 2025

Why make this change?

What is this change?

How was this tested?

Sample Request(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RubenCerna2079 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

souvikghosh04 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants