Skip to content

Commit b41cea5

Browse files
TimHessTimHess
committed
Add delay in filewatcher token
- take error handling back out of ConfigureCertificateOptions - reduce polling timeout in tests
1 parent cd6f434 commit b41cea5

4 files changed

Lines changed: 56 additions & 68 deletions

File tree

‎src/Common/src/Certificates/ConfigureCertificateOptions.cs‎

Lines changed: 8 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,10 @@
22
// The .NET Foundation licenses this file to you under the Apache 2.0 License.
33
// See the LICENSE file in the project root for more information.
44

5-
using System.Security.Cryptography;
65
using System.Security.Cryptography.X509Certificates;
76
using System.Text;
87
using System.Text.RegularExpressions;
98
using Microsoft.Extensions.Configuration;
10-
using Microsoft.Extensions.Logging;
119
using Microsoft.Extensions.Options;
1210

1311
namespace Steeltoe.Common.Certificates;
@@ -18,14 +16,12 @@ internal sealed class ConfigureCertificateOptions : IConfigureNamedOptions<Certi
1816
TimeSpan.FromSeconds(1));
1917

2018
private readonly IConfiguration _configuration;
21-
private readonly ILogger<ConfigureCertificateOptions> _logger;
2219

23-
public ConfigureCertificateOptions(IConfiguration configuration, ILogger<ConfigureCertificateOptions> logger)
20+
public ConfigureCertificateOptions(IConfiguration configuration)
2421
{
2522
ArgumentNullException.ThrowIfNull(configuration);
2623

2724
_configuration = configuration;
28-
_logger = logger;
2925
}
3026

3127
public void Configure(CertificateOptions options)
@@ -46,28 +42,16 @@ public void Configure(string? name, CertificateOptions options)
4642

4743
string? privateKeyFilePath = _configuration.GetValue<string>(GetConfigurationKey(name, "PrivateKeyFilePath"));
4844

49-
try
50-
{
51-
options.Certificate = privateKeyFilePath != null && File.Exists(privateKeyFilePath)
52-
? X509Certificate2.CreateFromPemFile(certificateFilePath, privateKeyFilePath)
53-
: new X509Certificate2(certificateFilePath);
45+
options.Certificate = privateKeyFilePath != null && File.Exists(privateKeyFilePath)
46+
? X509Certificate2.CreateFromPemFile(certificateFilePath, privateKeyFilePath)
47+
: new X509Certificate2(certificateFilePath);
5448

55-
X509Certificate2[] certificateChain = CertificateRegex.Matches(File.ReadAllText(certificateFilePath))
56-
.Select(x => new X509Certificate2(Encoding.ASCII.GetBytes(x.Value))).ToArray();
49+
X509Certificate2[] certificateChain = CertificateRegex.Matches(File.ReadAllText(certificateFilePath))
50+
.Select(x => new X509Certificate2(Encoding.ASCII.GetBytes(x.Value))).ToArray();
5751

58-
foreach (X509Certificate2 issuer in certificateChain.Skip(1))
59-
{
60-
options.IssuerChain.Add(issuer);
61-
}
62-
}
63-
catch (IOException ex)
64-
{
65-
_logger.LogDebug(ex, "Failed to load certificate for '{CertificateName}' from '{Path}'. Will retry on next reload.", name, certificateFilePath);
66-
}
67-
catch (CryptographicException ex)
52+
foreach (X509Certificate2 issuer in certificateChain.Skip(1))
6853
{
69-
_logger.LogWarning(ex, "Failed to parse file contents for '{CertificateName}' from '{Path}'. Will retry on next reload.", name,
70-
certificateFilePath);
54+
options.IssuerChain.Add(issuer);
7155
}
7256
}
7357

‎src/Common/src/Certificates/FilePathInOptionsChangeTokenSource.cs‎

Lines changed: 31 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -32,9 +32,6 @@ public void ChangePath(string filePath)
3232
{
3333
_filePath = filePath;
3434

35-
// Wait until the file is fully written to disk.
36-
Thread.Sleep(500);
37-
3835
ConfigurationReloadToken previousToken = Interlocked.Exchange(ref _changeFilePathToken, new ConfigurationReloadToken());
3936
previousToken.OnReload();
4037
}
@@ -44,8 +41,12 @@ public IChangeToken GetChangeToken()
4441
{
4542
IChangeToken watcherChangeToken = _fileWatcher.GetChangeToken(_filePath);
4643

44+
// Wrap the watcher token to delay signaling to the options monitor
45+
// -- avoids IOException when certificate and key change around the same time.
46+
IChangeToken debouncedToken = new DebouncedChangeToken(watcherChangeToken, TimeSpan.FromMilliseconds(200));
47+
4748
return new CompositeChangeToken([
48-
watcherChangeToken,
49+
debouncedToken,
4950
_changeFilePathToken
5051
]);
5152
}
@@ -126,4 +127,30 @@ private static string EnsureTrailingSlash(string path)
126127
return path.Length > 0 && path[^1] != Path.DirectorySeparatorChar ? $"{path}{Path.DirectorySeparatorChar}" : path;
127128
}
128129
}
130+
131+
private sealed class DebouncedChangeToken(IChangeToken inner, TimeSpan delay) : IChangeToken
132+
{
133+
private readonly IChangeToken _inner = inner;
134+
private readonly TimeSpan _delay = delay;
135+
136+
public bool HasChanged => _inner.HasChanged;
137+
138+
public bool ActiveChangeCallbacks => _inner.ActiveChangeCallbacks;
139+
140+
public IDisposable RegisterChangeCallback(Action<object?> callback, object? state)
141+
{
142+
return _inner.RegisterChangeCallback(async void (_) =>
143+
{
144+
try
145+
{
146+
await Task.Delay(_delay).ConfigureAwait(false);
147+
callback(state);
148+
}
149+
catch
150+
{
151+
// Swallow exceptions to avoid crashing the options infrastructure
152+
}
153+
}, state);
154+
}
155+
}
129156
}

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

Lines changed: 16 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,6 @@
77
using System.Text.Json;
88
using Microsoft.Extensions.Configuration;
99
using Microsoft.Extensions.DependencyInjection;
10-
using Microsoft.Extensions.Logging;
11-
using Microsoft.Extensions.Logging.Abstractions;
1210
using Microsoft.Extensions.Options;
1311
using Steeltoe.Common.TestResources;
1412
using Steeltoe.Common.TestResources.IO;
@@ -24,7 +22,8 @@ public sealed class ConfigureCertificateOptionsTest
2422
[InlineData(CertificateName)]
2523
public void ConfigureCertificateOptions_NoPath_NoCertificate(string certificateName)
2624
{
27-
var configureOptions = new ConfigureCertificateOptions(new ConfigurationBuilder().Build(), NullLogger<ConfigureCertificateOptions>.Instance);
25+
IConfiguration configuration = new ConfigurationBuilder().Build();
26+
var configureOptions = new ConfigureCertificateOptions(configuration);
2827
var options = new CertificateOptions();
2928

3029
configureOptions.Configure(certificateName, options);
@@ -43,7 +42,7 @@ public void ConfigureCertificateOptions_BadPath_NoCertificate(string certificate
4342
};
4443

4544
IConfiguration configuration = new ConfigurationBuilder().AddInMemoryCollection(appSettings).Build();
46-
var configureOptions = new ConfigureCertificateOptions(configuration, NullLogger<ConfigureCertificateOptions>.Instance);
45+
var configureOptions = new ConfigureCertificateOptions(configuration);
4746
var options = new CertificateOptions();
4847

4948
configureOptions.Configure(certificateName, options);
@@ -54,65 +53,43 @@ public void ConfigureCertificateOptions_BadPath_NoCertificate(string certificate
5453
[Theory]
5554
[InlineData("")]
5655
[InlineData(CertificateName)]
57-
public void ConfigureCertificateOptions_EmptyFile_Crash_logged(string certificateName)
56+
public void ConfigureCertificateOptions_ThrowsOnEmptyFile(string certificateName)
5857
{
59-
CapturingLoggerProvider loggerProvider = new()
60-
{
61-
IncludeStackTraces = true
62-
};
63-
64-
using var loggerFactory = new LoggerFactory([loggerProvider]);
65-
ILogger<ConfigureCertificateOptions> logger = loggerFactory.CreateLogger<ConfigureCertificateOptions>();
66-
6758
var appSettings = new Dictionary<string, string?>
6859
{
6960
[$"{GetConfigurationKey(certificateName, "CertificateFilePath")}"] = "empty.crt"
7061
};
7162

7263
IConfiguration configuration = new ConfigurationBuilder().AddInMemoryCollection(appSettings).Build();
73-
var configureOptions = new ConfigureCertificateOptions(configuration, logger);
64+
65+
var configureOptions = new ConfigureCertificateOptions(configuration);
7466
var options = new CertificateOptions();
7567

76-
configureOptions.Configure(certificateName, options);
68+
Action configureAction = () => configureOptions.Configure(certificateName, options);
69+
configureAction.Should().Throw<CryptographicException>();
7770

7871
options.Certificate.Should().BeNull();
79-
80-
loggerProvider.GetAll().Should().ContainSingle(message =>
81-
message.Contains(typeof(CryptographicException).FullName!, StringComparison.OrdinalIgnoreCase) && message.StartsWith(
82-
$"WARN {typeof(ConfigureCertificateOptions).FullName}: Failed to parse file contents for '{certificateName}' from 'empty.crt'. Will retry on next reload.",
83-
StringComparison.OrdinalIgnoreCase));
8472
}
8573

8674
[Theory]
8775
[InlineData("")]
8876
[InlineData(CertificateName)]
8977
public void ConfigureCertificateOptions_ThrowsOnInvalidKey(string certificateName)
9078
{
91-
CapturingLoggerProvider loggerProvider = new()
92-
{
93-
IncludeStackTraces = true
94-
};
95-
96-
using var loggerFactory = new LoggerFactory([loggerProvider]);
97-
ILogger<ConfigureCertificateOptions> logger = loggerFactory.CreateLogger<ConfigureCertificateOptions>();
98-
9979
var appSettings = new Dictionary<string, string?>
10080
{
10181
[$"{GetConfigurationKey(certificateName, "CertificateFilePath")}"] = "instance.crt",
10282
[$"{GetConfigurationKey(certificateName, "PrivateKeyFilePath")}"] = "invalid.key"
10383
};
10484

10585
IConfiguration configuration = new ConfigurationBuilder().AddInMemoryCollection(appSettings).Build();
106-
var configureOptions = new ConfigureCertificateOptions(configuration, logger);
86+
var configureOptions = new ConfigureCertificateOptions(configuration);
10787
var options = new CertificateOptions();
10888

109-
configureOptions.Configure(certificateName, options);
110-
options.Certificate.Should().BeNull();
89+
Action configureAction = () => configureOptions.Configure(certificateName, options);
90+
configureAction.Should().Throw<CryptographicException>();
11191

112-
loggerProvider.GetAll().Should().ContainSingle(message =>
113-
message.Contains(typeof(CryptographicException).FullName!, StringComparison.OrdinalIgnoreCase) && message.StartsWith(
114-
$"WARN {typeof(ConfigureCertificateOptions).FullName}: Failed to parse file contents for '{certificateName}' from 'instance.crt'. Will retry on next reload.",
115-
StringComparison.OrdinalIgnoreCase));
92+
options.Certificate.Should().BeNull();
11693
}
11794

11895
[Theory]
@@ -127,7 +104,7 @@ public void ConfigureCertificateOptions_ReadsP12File_CreatesCertificate(string c
127104

128105
IConfiguration configuration = new ConfigurationBuilder().AddInMemoryCollection(appSettings).Build();
129106
configuration[$"{GetConfigurationKey(certificateName, "CertificateFilePath")}"].Should().NotBeNull();
130-
var configureOptions = new ConfigureCertificateOptions(configuration, NullLogger<ConfigureCertificateOptions>.Instance);
107+
var configureOptions = new ConfigureCertificateOptions(configuration);
131108
var options = new CertificateOptions();
132109

133110
configureOptions.Configure(certificateName, options);
@@ -148,7 +125,7 @@ public void ConfigureCertificateOptions_ReadsPemFiles_CreatesCertificate(string
148125
};
149126

150127
IConfiguration configuration = new ConfigurationBuilder().AddInMemoryCollection(appSettings).Build();
151-
var configureOptions = new ConfigureCertificateOptions(configuration, NullLogger<ConfigureCertificateOptions>.Instance);
128+
var configureOptions = new ConfigureCertificateOptions(configuration);
152129
var options = new CertificateOptions();
153130

154131
configureOptions.Configure(certificateName, options);
@@ -191,7 +168,7 @@ public async Task CertificateOptions_update_on_changed_contents(string certifica
191168
await File.WriteAllTextAsync(privateKeyFilePath, secondPrivateKeyContent, TestContext.Current.CancellationToken);
192169

193170
using Task pollTask = WaitUntilCertificateChangedToAsync(secondX509, optionsMonitor, certificateName, TestContext.Current.CancellationToken);
194-
await pollTask.WaitAsync(TimeSpan.FromSeconds(4), TestContext.Current.CancellationToken);
171+
await pollTask.WaitAsync(TimeSpan.FromSeconds(1), TestContext.Current.CancellationToken);
195172

196173
optionsMonitor.Get(certificateName).Certificate.Should().Be(secondX509);
197174
}
@@ -228,7 +205,7 @@ public async Task CertificateOptions_update_on_changed_path(string certificateNa
228205
await File.WriteAllTextAsync(appSettingsPath, appSettings, TestContext.Current.CancellationToken);
229206

230207
using Task pollTask = WaitUntilCertificateChangedToAsync(secondX509, optionsMonitor, certificateName, TestContext.Current.CancellationToken);
231-
await pollTask.WaitAsync(TimeSpan.FromSeconds(4), TestContext.Current.CancellationToken);
208+
await pollTask.WaitAsync(TimeSpan.FromSeconds(1), TestContext.Current.CancellationToken);
232209

233210
optionsMonitor.Get(certificateName).Certificate.Should().Be(secondX509);
234211
}

‎src/Configuration/src/ConfigServer/ConfigServerConfigurationSource.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,7 @@ public IConfigurationProvider Build(IConfigurationBuilder builder)
119119

120120
if (!string.IsNullOrEmpty(clientCertificatePath) && DefaultOptions.ClientCertificate.Certificate == null)
121121
{
122-
var certificateConfigurer = new ConfigureCertificateOptions(Configuration, _loggerFactory.CreateLogger<ConfigureCertificateOptions>());
122+
var certificateConfigurer = new ConfigureCertificateOptions(Configuration);
123123

124124
var options = new CertificateOptions();
125125
certificateConfigurer.Configure("ConfigServer", options);

0 commit comments

Comments
 (0)