Skip to content

Commit 9aa324d

Browse files
bart-vmwareTimHess
andauthored
Config Server stability improvements (#1667)
* Remove duplicate tests * Update tests using Sandbox to use MemoryFileProvider (and without delay), to reduce test flakiness * Update existing test to verify precedence between options and appsettings * Make ConfigServerClientOptions reactive to configuration changes `ConfigServerClientOptions` are now re-evaluated from initial options + configuration on every settings change, instead of being mutated in-place. This prevents stale or torn reads when the provider runs concurrently (e.g. polling timer vs reload). Key changes: - The provider clones options on each configuration reload, starting from the initial options passed via code, then applying configuration on top. This ensures code-level defaults are restored when keys are removed from configuration. - Client settings (spring:cloud:config:*) are no longer written into the provider's data dictionary, eliminating a circular feedback loop where the provider's own output could influence its input. - Discovery lookup results are tracked separately and applied on top of the options snapshot at load time, rather than mutating shared state. - Client certificate configuration is moved from the source's `Build` method into `ConfigureConfigServerClientOptions`, so it participates in the options pipeline. - `ConfigServerClientOptions.Clone()` is introduced to produce isolated snapshots, preventing tearing when options are read during a concurrent reload. A bug was fixed that prevented using global certificates. - An internal `HttpClientHandler` parameter is threaded through the source and builder extensions, enabling direct handler injection for tests without reflection. - `IOptionsChangeTokenSource<ConfigServerClientOptions>` is registered in DI so that `IOptionsMonitor` properly triggers on configuration changes. * Improve test coverage * Fix references to Config Server in comments * Fix threading bugs and resource leaks in Config Server provider The provider had several concurrency and lifecycle issues: - Vault token renewal timers were created unboundedly on every HTTP request that carried a token, leaking timers that were never disposed. Vault renewal is now managed as a single timer with the same lifecycle as the polling timer. - Timer management, handler configuration, and disposal could race with each other without synchronization. A lifecycle lock now guards these operations, while non-blocking try-enter locks prevent timer callbacks from queueing up. - The HTTP client handler's certificates were reconfigured on every HTTP request, racing with concurrent requests sharing the same handler. Certificate and validation configuration is now applied once per settings change under the lifecycle lock, with certificates cleared before re-adding to prevent unbounded accumulation across reloads. - Options cloning did not copy the certificate issuer chain, losing intermediate CA certificates across reloads. - Disposal is now coordinated via a volatile flag so that in-flight timer callbacks and Load() calls exit gracefully instead of throwing unobserved exceptions. * Fixed: use latest options snapshot during discovery in Config Server * Fixed: preserve original stack trace when load throws * Fix Config Server health contributor to act on a consistent snapshot * Fix torn reads in _lastDiscoveryLookupResult * Fixed: act on single snapshot of _httpClientHandler * Refresh discovery during polled reload and fix lazy initialization race Polled configuration reloads now refresh the discovery service lookup before fetching from Config Server, enabling detection of server address changes between polling intervals. Unlike initial load, polled reload does not apply FailFast or retry semantics. Fixed a race where concurrent calls to LoadInternalAsync could create multiple ConfigServerDiscoveryService instances, each constructing their own temporary DI container and discovery clients. * Fix change callback accumulation on repeated configuration reloads Each configuration reload re-registered a new change callback via RegisterChangeCallback, without disposing the previous one. Rapid reloads could accumulate stale callbacks, causing redundant OnSettingsChanged invocations on multiple threads. Replaced with a single ChangeToken.OnChange registration in the constructor, which automatically handles re-registration and is disposed on shutdown. * Fix HttpClientHandler mutation race and timer disposal race Replace shared mutable HttpClientHandler with a per-request factory, eliminating the race where OnSettingsChanged reconfigured a handler concurrently in use by HTTP requests. Production code creates and disposes a fresh handler per request; tests inject a factory for mocking. Replace _isDisposed flag with CancellationToken-based shutdown, enabling in-flight HTTP requests in timer callbacks to terminate promptly on disposal instead of running to completion. * Fix threadpool starvation during retries * Fix timer callbacks potentially using stale options when settings change * Fix stale reads in ConfigServerDiscoveryService * Add overloads to post-configure Config Server options * Cleanup existing tests * Various fixes - Improved log messages (and don't log multiple times), fix exception stack traces - Don't remote-fetch twice at startup (load + timer that immediately fired) - Moved options bind and remote-fetch from constructor to Load, fix combination with placeholder * Increase timeout to reduce failures in CI * Update src/Configuration/test/ConfigServer.Test/ConfigServerConfigurationProviderTest.Loading.cs Co-authored-by: Tim Hess <tim.hess@broadcom.com> * Review feedback: replace overrule with override * Review feedback: Change initial to default options * Review feedback: remove redundant settings in test * Review feedback: adjust test name * Review feedback: remove duplicate configuration * Review feedback: reformat JSON snippets in tests --------- Co-authored-by: Tim Hess <tim.hess@broadcom.com>
1 parent 972d11d commit 9aa324d

67 files changed

Lines changed: 3168 additions & 1917 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

src/Bootstrap/src/AutoConfiguration/BootstrapScanner.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,7 @@ public void ConfigureSteeltoe()
9191

9292
private void WireConfigServer()
9393
{
94-
_wrapper.AddConfigServer(_loggerFactory);
94+
_wrapper.AddConfigServer(null, _loggerFactory);
9595

9696
LogConfigServerConfigured();
9797
}

src/Common/src/Certificates/CertificateOptions.cs

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,20 @@ public sealed class CertificateOptions
1414
internal const string ConfigurationKeyPrefix = "Certificates";
1515

1616
public X509Certificate2? Certificate { get; set; }
17-
1817
public IList<X509Certificate2> IssuerChain { get; } = [];
18+
19+
internal CertificateOptions Clone()
20+
{
21+
var clone = new CertificateOptions
22+
{
23+
Certificate = Certificate
24+
};
25+
26+
foreach (X509Certificate2 issuer in IssuerChain)
27+
{
28+
clone.IssuerChain.Add(issuer);
29+
}
30+
31+
return clone;
32+
}
1933
}

src/Common/test/Certificates.Test/ConfigureCertificateOptionsTest.cs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -148,7 +148,7 @@ public async Task CertificateOptions_update_on_changed_contents(string certifica
148148
string secondPrivateKeyContent = await File.ReadAllTextAsync("secondInstance.key", TestContext.Current.CancellationToken);
149149
using var secondX509 = X509Certificate2.CreateFromPemFile("secondInstance.crt", "secondInstance.key");
150150
string appSettings = BuildAppSettingsJson(certificateName, certificateFilePath, privateKeyFilePath);
151-
string appSettingsPath = sandbox.CreateFile(MemoryFileProvider.DefaultAppSettingsFileName, appSettings);
151+
string appSettingsPath = sandbox.CreateFile("appsettings.json", appSettings);
152152
var configurationBuilder = new ConfigurationBuilder();
153153
configurationBuilder.AddJsonFile(appSettingsPath, false, true);
154154
IConfiguration configuration = configurationBuilder.Build();
@@ -167,7 +167,7 @@ public async Task CertificateOptions_update_on_changed_contents(string certifica
167167
await File.WriteAllTextAsync(privateKeyFilePath, secondPrivateKeyContent, TestContext.Current.CancellationToken);
168168

169169
using Task pollTask = WaitUntilCertificateChangedToAsync(secondX509, optionsMonitor, certificateName, TestContext.Current.CancellationToken);
170-
await pollTask.WaitAsync(TimeSpan.FromSeconds(1), TestContext.Current.CancellationToken);
170+
await pollTask.WaitAsync(TimeSpan.FromSeconds(5), TestContext.Current.CancellationToken);
171171

172172
optionsMonitor.Get(certificateName).Certificate.Should().Be(secondX509);
173173
}
@@ -185,7 +185,7 @@ public async Task CertificateOptions_update_on_changed_path(string certificateNa
185185
string firstPrivateKeyFilePath = sandbox.CreateFile(Guid.NewGuid() + ".key", firstPrivateKeyContent);
186186
using var secondX509 = X509Certificate2.CreateFromPemFile("secondInstance.crt", "secondInstance.key");
187187
string appSettings = BuildAppSettingsJson(certificateName, firstCertificateFilePath, firstPrivateKeyFilePath);
188-
string appSettingsPath = sandbox.CreateFile(MemoryFileProvider.DefaultAppSettingsFileName, appSettings);
188+
string appSettingsPath = sandbox.CreateFile("appsettings.json", appSettings);
189189
var configurationBuilder = new ConfigurationBuilder();
190190
configurationBuilder.AddJsonFile(appSettingsPath, false, true);
191191
IConfiguration configuration = configurationBuilder.Build();

src/Common/test/Common.Test/ApplicationInstanceInfoTest.cs

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@
77
using Microsoft.Extensions.DependencyInjection;
88
using Steeltoe.Common.Extensions;
99
using Steeltoe.Common.TestResources;
10-
using Steeltoe.Common.TestResources.IO;
1110

1211
namespace Steeltoe.Common.Test;
1312

@@ -24,7 +23,7 @@ public void ConstructorSetsDefaults()
2423
[Fact]
2524
public async Task ReadsApplicationConfiguration()
2625
{
27-
const string configJson = """
26+
const string appSettings = """
2827
{
2928
"Spring": {
3029
"Application": {
@@ -34,13 +33,11 @@ public async Task ReadsApplicationConfiguration()
3433
}
3534
""";
3635

37-
using var sandbox = new Sandbox();
38-
string path = sandbox.CreateFile(MemoryFileProvider.DefaultAppSettingsFileName, configJson);
39-
string directory = Path.GetDirectoryName(path)!;
40-
string fileName = Path.GetFileName(path);
36+
var fileProvider = new MemoryFileProvider();
37+
fileProvider.IncludeAppSettingsJsonFile(appSettings);
38+
4139
var builder = new ConfigurationBuilder();
42-
builder.SetBasePath(directory);
43-
builder.AddJsonFile(fileName);
40+
builder.AddInMemoryAppSettingsJsonFile(fileProvider);
4441
IConfiguration configuration = builder.Build();
4542

4643
var services = new ServiceCollection();

src/Common/test/TestResources/MemoryFileProvider.cs

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,6 @@ namespace Steeltoe.Common.TestResources;
1212

1313
public sealed class MemoryFileProvider : IFileProvider
1414
{
15-
public const string DefaultAppSettingsFileName = "appsettings.json";
16-
1715
private static readonly char[] DirectorySeparators =
1816
[
1917
Path.DirectorySeparatorChar,
Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the Apache 2.0 License.
3+
// See the LICENSE file in the project root for more information.
4+
5+
namespace Steeltoe.Common.TestResources;
6+
7+
public static class MemoryFileProviderAppSettingsExtensions
8+
{
9+
public static void IncludeAppSettingsJsonFile(this MemoryFileProvider fileProvider, string contents)
10+
{
11+
ArgumentNullException.ThrowIfNull(fileProvider);
12+
13+
fileProvider.IncludeFile(MemoryFileProviderConfigurationBuilderExtensions.AppSettingsJsonFileName, contents);
14+
}
15+
16+
public static void IncludeAppSettingsXmlFile(this MemoryFileProvider fileProvider, string contents)
17+
{
18+
ArgumentNullException.ThrowIfNull(fileProvider);
19+
20+
fileProvider.IncludeFile(MemoryFileProviderConfigurationBuilderExtensions.AppSettingsXmlFileName, contents);
21+
}
22+
23+
public static void IncludeAppSettingsIniFile(this MemoryFileProvider fileProvider, string contents)
24+
{
25+
ArgumentNullException.ThrowIfNull(fileProvider);
26+
27+
fileProvider.IncludeFile(MemoryFileProviderConfigurationBuilderExtensions.AppSettingsIniFileName, contents);
28+
}
29+
30+
public static void ReplaceAppSettingsJsonFile(this MemoryFileProvider fileProvider, string contents)
31+
{
32+
ArgumentNullException.ThrowIfNull(fileProvider);
33+
34+
fileProvider.ReplaceFile(MemoryFileProviderConfigurationBuilderExtensions.AppSettingsJsonFileName, contents);
35+
}
36+
}
Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,77 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the Apache 2.0 License.
3+
// See the LICENSE file in the project root for more information.
4+
5+
using Microsoft.Extensions.Configuration;
6+
using Microsoft.Extensions.Configuration.Ini;
7+
using Microsoft.Extensions.Configuration.Json;
8+
using Microsoft.Extensions.Configuration.Xml;
9+
10+
namespace Steeltoe.Common.TestResources;
11+
12+
public static class MemoryFileProviderConfigurationBuilderExtensions
13+
{
14+
internal const string AppSettingsJsonFileName = "appsettings.json";
15+
internal const string AppSettingsXmlFileName = "appsettings.xml";
16+
internal const string AppSettingsIniFileName = "appsettings.ini";
17+
18+
public static void AddInMemoryAppSettingsJsonFile(this IConfigurationBuilder builder, MemoryFileProvider fileProvider)
19+
{
20+
AddInMemoryJsonFile(builder, fileProvider, AppSettingsJsonFileName);
21+
}
22+
23+
public static void AddInMemoryJsonFile(this IConfigurationBuilder builder, MemoryFileProvider fileProvider, string path)
24+
{
25+
ArgumentNullException.ThrowIfNull(builder);
26+
ArgumentNullException.ThrowIfNull(fileProvider);
27+
ArgumentException.ThrowIfNullOrEmpty(path);
28+
29+
var source = new JsonConfigurationSource
30+
{
31+
FileProvider = fileProvider,
32+
Path = path,
33+
Optional = false,
34+
ReloadOnChange = true,
35+
// Turn off debounce, so the change token triggers immediately. Then we don't need to sleep in tests.
36+
ReloadDelay = 0
37+
};
38+
39+
builder.Add(source);
40+
}
41+
42+
public static void AddInMemoryAppSettingsXmlFile(this IConfigurationBuilder builder, MemoryFileProvider fileProvider)
43+
{
44+
ArgumentNullException.ThrowIfNull(builder);
45+
ArgumentNullException.ThrowIfNull(fileProvider);
46+
47+
var source = new XmlConfigurationSource
48+
{
49+
FileProvider = fileProvider,
50+
Path = AppSettingsXmlFileName,
51+
Optional = false,
52+
ReloadOnChange = true,
53+
// Turn off debounce, so the change token triggers immediately. Then we don't need to sleep in tests.
54+
ReloadDelay = 0
55+
};
56+
57+
builder.Add(source);
58+
}
59+
60+
public static void AddInMemoryAppSettingsIniFile(this IConfigurationBuilder builder, MemoryFileProvider fileProvider)
61+
{
62+
ArgumentNullException.ThrowIfNull(builder);
63+
ArgumentNullException.ThrowIfNull(fileProvider);
64+
65+
var source = new IniConfigurationSource
66+
{
67+
FileProvider = fileProvider,
68+
Path = AppSettingsIniFileName,
69+
Optional = false,
70+
ReloadOnChange = true,
71+
// Turn off debounce, so the change token triggers immediately. Then we don't need to sleep in tests.
72+
ReloadDelay = 0
73+
};
74+
75+
builder.Add(source);
76+
}
77+
}

src/Configuration/src/Abstractions/CompositeConfigurationProvider.cs

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -53,10 +53,9 @@ private void Load(bool isReload)
5353

5454
public IEnumerable<string> GetChildKeys(IEnumerable<string> earlierKeys, string? parentPath)
5555
{
56+
ArgumentNullException.ThrowIfNull(earlierKeys);
57+
5658
string[] earlierKeysArray = earlierKeys as string[] ?? earlierKeys.ToArray();
57-
#pragma warning disable S3236 // Caller information arguments should not be provided explicitly
58-
ArgumentNullException.ThrowIfNull(earlierKeysArray, nameof(earlierKeys));
59-
#pragma warning restore S3236 // Caller information arguments should not be provided explicitly
6059

6160
if (_logger.IsEnabled(LogLevel.Trace))
6261
{

src/Configuration/src/ConfigServer/ConfigServerClientOptions.cs

Lines changed: 56 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -17,10 +17,14 @@ public sealed class ConfigServerClientOptions : IValidateCertificatesOptions
1717
private const char CommaDelimiter = ',';
1818
internal const string ConfigurationPrefix = "spring:cloud:config";
1919

20-
internal CertificateOptions ClientCertificate { get; } = new();
2120
internal TimeSpan HttpTimeout => TimeSpan.FromMilliseconds(Timeout);
2221
internal bool IsMultiServerConfiguration => Uri != null && Uri.Contains(CommaDelimiter);
2322

23+
/// <summary>
24+
/// Gets or sets the client certificate used for mutual TLS authentication with the Config Server.
25+
/// </summary>
26+
internal CertificateOptions ClientCertificate { get; set; } = new();
27+
2428
/// <summary>
2529
/// Gets or sets a value indicating whether the Config Server provider is enabled. Default value: true.
2630
/// </summary>
@@ -35,7 +39,7 @@ public sealed class ConfigServerClientOptions : IValidateCertificatesOptions
3539
/// Gets or sets a comma-separated list of environments used when accessing configuration data. Default value: "Production".
3640
/// </summary>
3741
[ConfigurationKeyName("Env")]
38-
public string? Environment { get; set; } = "Production";
42+
public string? Environment { get; set; }
3943

4044
/// <summary>
4145
/// Gets or sets a comma-separated list of labels to request from the server.
@@ -96,17 +100,17 @@ public bool ValidateCertificatesAlt
96100
/// <summary>
97101
/// Gets retry settings.
98102
/// </summary>
99-
public ConfigServerRetryOptions Retry { get; } = new();
103+
public ConfigServerRetryOptions Retry { get; private set; } = new();
100104

101105
/// <summary>
102106
/// Gets service discovery settings.
103107
/// </summary>
104-
public ConfigServerDiscoveryOptions Discovery { get; } = new();
108+
public ConfigServerDiscoveryOptions Discovery { get; private set; } = new();
105109

106110
/// <summary>
107111
/// Gets health check settings.
108112
/// </summary>
109-
public ConfigServerHealthOptions Health { get; } = new();
113+
public ConfigServerHealthOptions Health { get; private set; } = new();
110114

111115
/// <summary>
112116
/// Gets or sets the address used by the provider to obtain a OAuth Access Token.
@@ -129,7 +133,7 @@ public bool ValidateCertificatesAlt
129133
public int TokenTtl { get; set; } = 300_000;
130134

131135
/// <summary>
132-
/// Gets or sets the vault token renew rate (in milliseconds). Default value: 60_000 (1 minute).
136+
/// Gets or sets the Vault token renew rate (in milliseconds). Default value: 60_000 (1 minute).
133137
/// </summary>
134138
public int TokenRenewRate { get; set; } = 60_000;
135139

@@ -141,7 +145,52 @@ public bool ValidateCertificatesAlt
141145
/// <summary>
142146
/// Gets headers that will be added to the Config Server request.
143147
/// </summary>
144-
public IDictionary<string, string> Headers { get; } = new Dictionary<string, string>();
148+
public IDictionary<string, string> Headers { get; private set; } = new Dictionary<string, string>();
149+
150+
internal ConfigServerClientOptions Clone()
151+
{
152+
return new ConfigServerClientOptions
153+
{
154+
ClientCertificate = ClientCertificate.Clone(),
155+
Enabled = Enabled,
156+
FailFast = FailFast,
157+
Environment = Environment,
158+
Label = Label,
159+
Name = Name,
160+
Uri = Uri,
161+
Username = Username,
162+
Password = Password,
163+
Token = Token,
164+
Timeout = Timeout,
165+
PollingInterval = PollingInterval,
166+
ValidateCertificates = ValidateCertificates,
167+
Retry = new ConfigServerRetryOptions
168+
{
169+
Enabled = Retry.Enabled,
170+
InitialInterval = Retry.InitialInterval,
171+
MaxInterval = Retry.MaxInterval,
172+
Multiplier = Retry.Multiplier,
173+
MaxAttempts = Retry.MaxAttempts
174+
},
175+
Discovery = new ConfigServerDiscoveryOptions
176+
{
177+
Enabled = Discovery.Enabled,
178+
ServiceId = Discovery.ServiceId
179+
},
180+
Health = new ConfigServerHealthOptions
181+
{
182+
Enabled = Health.Enabled,
183+
TimeToLive = Health.TimeToLive
184+
},
185+
AccessTokenUri = AccessTokenUri,
186+
ClientSecret = ClientSecret,
187+
ClientId = ClientId,
188+
TokenTtl = TokenTtl,
189+
TokenRenewRate = TokenRenewRate,
190+
DisableTokenRenewal = DisableTokenRenewal,
191+
Headers = new Dictionary<string, string>(Headers)
192+
};
193+
}
145194

146195
internal List<Uri> GetUris()
147196
{

0 commit comments

Comments
 (0)