Skip to content

Commit 26057dd

Browse files
brettsamkashimiz
brettsam
authored and
kashimiz
committed
correctly wiring up OnChange for ScriptApplicationHostOptions (#8329)
1 parent 407cf39 commit 26057dd

File tree

6 files changed

+122
-25
lines changed

6 files changed

+122
-25
lines changed

‎src/WebJobs.Script.WebHost/Configuration/ScriptApplicationHostOptionsChangeTokenSource.cs‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
// Copyright (c) .NET Foundation. All rights reserved.
2+
// Licensed under the MIT License. See License.txt in the project root for license information.
3+
4+
using System;
5+
using System.Threading;
6+
using Microsoft.Extensions.FileProviders;
7+
using Microsoft.Extensions.Options;
8+
using Microsoft.Extensions.Primitives;
9+
10+
namespace Microsoft.Azure.WebJobs.Script.WebHost.Configuration
11+
{
12+
public class ScriptApplicationHostOptionsChangeTokenSource : IOptionsChangeTokenSource<ScriptApplicationHostOptions>, IDisposable
13+
{
14+
private readonly IDisposable _standbyOptionsOnChangeSubscription;
15+
16+
private CancellationTokenSource _cancellationTokenSource = new CancellationTokenSource();
17+
private IChangeToken _changeToken;
18+
19+
public ScriptApplicationHostOptionsChangeTokenSource(IOptionsMonitor<StandbyOptions> standbyOptions)
20+
{
21+
_changeToken = new CancellationChangeToken(_cancellationTokenSource.Token);
22+
23+
_standbyOptionsOnChangeSubscription = standbyOptions.OnChange(o =>
24+
{
25+
if (_cancellationTokenSource == null)
26+
{
27+
return;
28+
}
29+
30+
// This should only ever happen once, on specialization, so null everything out
31+
// when this fires.
32+
var tokenSource = Interlocked.Exchange(ref _cancellationTokenSource, null);
33+
34+
if (tokenSource != null &&
35+
!tokenSource.IsCancellationRequested)
36+
{
37+
var changeToken = Interlocked.Exchange(ref _changeToken, NullChangeToken.Singleton);
38+
39+
tokenSource.Cancel();
40+
41+
// Dispose of the token source so our change
42+
// token reflects that state
43+
tokenSource.Dispose();
44+
}
45+
});
46+
}
47+
48+
public string Name { get; }
49+
50+
public void Dispose() => _standbyOptionsOnChangeSubscription?.Dispose();
51+
52+
public IChangeToken GetChangeToken() => _changeToken;
53+
}
54+
}

‎src/WebJobs.Script.WebHost/Configuration/ScriptApplicationHostOptionsSetup.cs‎

Lines changed: 3 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -14,25 +14,21 @@
1414

1515
namespace Microsoft.Azure.WebJobs.Script.WebHost.Configuration
1616
{
17-
public class ScriptApplicationHostOptionsSetup : IConfigureNamedOptions<ScriptApplicationHostOptions>, IDisposable
17+
public class ScriptApplicationHostOptionsSetup : IConfigureNamedOptions<ScriptApplicationHostOptions>
1818
{
1919
public const string SkipPlaceholder = "SkipPlaceholder";
20-
private readonly IOptionsMonitorCache<ScriptApplicationHostOptions> _cache;
2120
private readonly IConfiguration _configuration;
2221
private readonly IOptionsMonitor<StandbyOptions> _standbyOptions;
23-
private readonly IDisposable _standbyOptionsOnChangeSubscription;
2422
private readonly IServiceProvider _serviceProvider;
2523
private readonly IEnvironment _environment;
2624

2725
public ScriptApplicationHostOptionsSetup(IConfiguration configuration, IOptionsMonitor<StandbyOptions> standbyOptions,
28-
IOptionsMonitorCache<ScriptApplicationHostOptions> cache, IServiceProvider serviceProvider, IEnvironment environment)
26+
IServiceProvider serviceProvider, IEnvironment environment)
2927
{
30-
_cache = cache ?? throw new ArgumentNullException(nameof(cache));
3128
_configuration = configuration ?? throw new ArgumentNullException(nameof(configuration));
3229
_standbyOptions = standbyOptions ?? throw new ArgumentNullException(nameof(standbyOptions));
3330
_serviceProvider = serviceProvider;
34-
// If standby options change, invalidate this options cache.
35-
_standbyOptionsOnChangeSubscription = _standbyOptions.OnChange(o => _cache.Clear());
31+
3632
_environment = environment ?? throw new ArgumentNullException(nameof(environment));
3733
}
3834

@@ -163,10 +159,5 @@ public virtual bool BlobExists(string url)
163159
return false;
164160
}
165161
}
166-
167-
public void Dispose()
168-
{
169-
_standbyOptionsOnChangeSubscription?.Dispose();
170-
}
171162
}
172163
}

‎src/WebJobs.Script.WebHost/Security/KeyManagement/DefaultSecretManagerProvider.cs‎

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,10 +17,10 @@ public sealed class DefaultSecretManagerProvider : ISecretManagerProvider
1717
{
1818
private const string FileStorage = "Files";
1919
private readonly ILoggerFactory _loggerFactory;
20+
private readonly ILogger<DefaultSecretManagerProvider> _logger;
2021
private readonly IMetricsLogger _metricsLogger;
2122
private readonly IOptionsMonitor<ScriptApplicationHostOptions> _options;
2223
private readonly IHostIdProvider _hostIdProvider;
23-
private readonly IConfiguration _configuration;
2424
private readonly IEnvironment _environment;
2525
private readonly HostNameProvider _hostNameProvider;
2626
private readonly StartupContextProvider _startupContextProvider;
@@ -38,12 +38,13 @@ public DefaultSecretManagerProvider(IOptionsMonitor<ScriptApplicationHostOptions
3838

3939
_options = options ?? throw new ArgumentNullException(nameof(options));
4040
_hostIdProvider = hostIdProvider ?? throw new ArgumentNullException(nameof(hostIdProvider));
41-
_configuration = configuration ?? throw new ArgumentNullException(nameof(configuration));
4241
_environment = environment ?? throw new ArgumentNullException(nameof(environment));
4342
_hostNameProvider = hostNameProvider ?? throw new ArgumentNullException(nameof(hostNameProvider));
4443
_startupContextProvider = startupContextProvider ?? throw new ArgumentNullException(nameof(startupContextProvider));
4544

4645
_loggerFactory = loggerFactory;
46+
_logger = _loggerFactory.CreateLogger<DefaultSecretManagerProvider>();
47+
4748
_metricsLogger = metricsLogger ?? throw new ArgumentNullException(nameof(metricsLogger));
4849
_secretManagerLazy = new Lazy<ISecretManager>(Create);
4950
_secretsEnabledLazy = new Lazy<bool>(GetSecretsEnabled);
@@ -72,6 +73,8 @@ private void ResetSecretManager()
7273
{
7374
Interlocked.Exchange(ref _secretsEnabledLazy, new Lazy<bool>(GetSecretsEnabled));
7475
Interlocked.Exchange(ref _secretManagerLazy, new Lazy<ISecretManager>(Create));
76+
77+
_logger.LogDebug(new EventId(1, "ResetSecretManager"), "Reset SecretManager.");
7578
}
7679

7780
private ISecretManager Create() => new SecretManager(CreateSecretsRepository(), _loggerFactory.CreateLogger<SecretManager>(), _metricsLogger, _hostNameProvider, _startupContextProvider);
@@ -130,8 +133,7 @@ internal ISecretsRepository CreateSecretsRepository()
130133
$"For Blob Storage, please provide at least one of these. If you intend to use files for secrets, add an App Setting key '{EnvironmentSettingNames.AzureWebJobsSecretStorageType}' with value '{FileStorage}'.");
131134
}
132135

133-
ILogger logger = _loggerFactory.CreateLogger<DefaultSecretManagerProvider>();
134-
logger.LogInformation("Resolved secret storage provider {provider}", repository.Name);
136+
_logger.LogInformation(new EventId(3, "CreatedSecretRespository"), "Resolved secret storage provider {provider}", repository.Name);
135137

136138
return repository;
137139
}
@@ -185,7 +187,11 @@ internal bool TryGetSecretsRepositoryType(out Type repositoryType)
185187

186188
internal bool GetSecretsEnabled()
187189
{
188-
return TryGetSecretsRepositoryType(out _);
190+
bool secretsEnabled = TryGetSecretsRepositoryType(out Type repositoryType);
191+
192+
_logger.LogDebug(new EventId(2, "GetSecretsEnabled"), "SecretsEnabled evaluated to {secretsEnabled} with type {provider}.", secretsEnabled, repositoryType?.Name);
193+
194+
return secretsEnabled;
189195
}
190196
}
191197
}

‎src/WebJobs.Script.WebHost/WebHostServiceCollectionExtensions.cs‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -163,7 +163,13 @@ public static void AddWebJobsScriptHost(this IServiceCollection services, IConfi
163163
services.AddSingleton<IHostedService, HostedServiceManager>();
164164

165165
// Configuration
166+
167+
// ScriptApplicationHostOptions are special in that they need to be reset on specialization, but the reset
168+
// must happen after the StandbyOptions have reset. For this reason, we have a special ChangeTokenSource that
169+
// will reset the ScriptApplicationHostOptions only after StandbyOptions have been reset.
166170
services.ConfigureOptions<ScriptApplicationHostOptionsSetup>();
171+
services.AddSingleton<IOptionsChangeTokenSource<ScriptApplicationHostOptions>, ScriptApplicationHostOptionsChangeTokenSource>();
172+
167173
services.ConfigureOptions<StandbyOptionsSetup>();
168174
services.ConfigureOptions<LanguageWorkerOptionsSetup>();
169175
services.ConfigureOptionsWithChangeTokenSource<AppServiceOptions, AppServiceOptionsSetup, SpecializationChangeTokenSource<AppServiceOptions>>();

‎test/WebJobs.Script.Tests.Integration/WebHostEndToEnd/SpecializationE2ETests.cs‎

Lines changed: 45 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,17 +13,14 @@
1313
using Microsoft.ApplicationInsights.DataContracts;
1414
using Microsoft.AspNetCore.Hosting;
1515
using Microsoft.AspNetCore.TestHost;
16-
using Microsoft.Azure.WebJobs.Host;
1716
using Microsoft.Azure.WebJobs.Host.Config;
1817
using Microsoft.Azure.WebJobs.Logging;
1918
using Microsoft.Azure.WebJobs.Logging.ApplicationInsights;
2019
using Microsoft.Azure.WebJobs.Script.Configuration;
2120
using Microsoft.Azure.WebJobs.Script.Description;
2221
using Microsoft.Azure.WebJobs.Script.Grpc;
2322
using Microsoft.Azure.WebJobs.Script.WebHost;
24-
using Microsoft.Azure.WebJobs.Script.WebHost.Middleware;
2523
using Microsoft.Azure.WebJobs.Script.Workers.Rpc;
26-
using Microsoft.Diagnostics.JitTrace;
2724
using Microsoft.Extensions.Configuration;
2825
using Microsoft.Extensions.DependencyInjection;
2926
using Microsoft.Extensions.Hosting;
@@ -243,6 +240,51 @@ public async Task Specialization_ResetsSharedLoadContext()
243240
}
244241
}
245242

243+
[Fact]
244+
public async Task Specialization_ResetsSecretManagerRepository()
245+
{
246+
var builder = CreateStandbyHostBuilder("FunctionExecutionContext")
247+
.ConfigureLogging(logging =>
248+
{
249+
logging.AddFilter<TestLoggerProvider>(null, LogLevel.Debug);
250+
});
251+
252+
using (var testServer = new TestServer(builder))
253+
{
254+
var client = testServer.CreateClient();
255+
256+
var response = await client.GetAsync("api/warmup");
257+
response.EnsureSuccessStatusCode();
258+
259+
var provider = testServer.Host.Services.GetService<ISecretManagerProvider>();
260+
_ = provider.SecretsEnabled;
261+
_ = provider.SecretsEnabled;
262+
_ = provider.SecretsEnabled;
263+
264+
// Should only be evaluated once due to the Lazy
265+
var messages = _loggerProvider.GetAllLogMessages().Select(p => p.EventId.Name);
266+
Assert.Single(messages, "GetSecretsEnabled");
267+
268+
_environment.SetEnvironmentVariable(EnvironmentSettingNames.AzureWebsiteContainerReady, "1");
269+
_environment.SetEnvironmentVariable(EnvironmentSettingNames.AzureWebsitePlaceholderMode, "0");
270+
271+
// Force specialization
272+
response = await client.GetAsync("api/functionexecutioncontext");
273+
response.EnsureSuccessStatusCode();
274+
275+
_ = provider.SecretsEnabled;
276+
_ = provider.SecretsEnabled;
277+
_ = provider.SecretsEnabled;
278+
279+
messages = _loggerProvider.GetAllLogMessages().Select(p => p.EventId.Name);
280+
281+
// Should be re-evaluated one more time after reset
282+
Assert.Equal(2, messages.Where(p => p == "GetSecretsEnabled").Count());
283+
284+
Assert.Single(messages, "ResetSecretManager");
285+
}
286+
}
287+
246288
[Fact]
247289
public async Task StartAsync_SetsCorrectActiveHost_RefreshesLanguageWorkerOptions()
248290
{

‎test/WebJobs.Script.Tests/Configuration/ScriptApplicationHostOptionsSetupTests.cs‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@
55
using Microsoft.Azure.WebJobs.Script.WebHost;
66
using Microsoft.Azure.WebJobs.Script.WebHost.Configuration;
77
using Microsoft.Extensions.Configuration;
8-
using Microsoft.Extensions.Logging.Abstractions;
98
using Microsoft.Extensions.Options;
109
using Moq;
1110
using Xunit;
@@ -108,10 +107,9 @@ private void ConfigureOptions(ScriptApplicationHostOptions options, bool inStand
108107
var configuration = builder.Build();
109108

110109
var standbyOptions = new TestOptionsMonitor<StandbyOptions>(new StandbyOptions { InStandbyMode = inStandbyMode });
111-
var mockCache = new Mock<IOptionsMonitorCache<ScriptApplicationHostOptions>>();
112110
var mockServiceProvider = new Mock<IServiceProvider>();
113111
var mockEnvironment = environment ?? new TestEnvironment();
114-
var setup = new TestScriptApplicationHostOptionsSetup(configuration, standbyOptions, mockCache.Object, mockServiceProvider.Object, mockEnvironment)
112+
var setup = new TestScriptApplicationHostOptionsSetup(configuration, standbyOptions, mockServiceProvider.Object, mockEnvironment)
115113
{
116114
BlobExistsReturnValue = blobExists
117115
};
@@ -121,8 +119,8 @@ private void ConfigureOptions(ScriptApplicationHostOptions options, bool inStand
121119

122120
private class TestScriptApplicationHostOptionsSetup : ScriptApplicationHostOptionsSetup
123121
{
124-
public TestScriptApplicationHostOptionsSetup(IConfiguration configuration, IOptionsMonitor<StandbyOptions> standbyOptions, IOptionsMonitorCache<ScriptApplicationHostOptions> cache,
125-
IServiceProvider serviceProvider, IEnvironment environment) : base(configuration, standbyOptions, cache, serviceProvider, environment) { }
122+
public TestScriptApplicationHostOptionsSetup(IConfiguration configuration, IOptionsMonitor<StandbyOptions> standbyOptions,
123+
IServiceProvider serviceProvider, IEnvironment environment) : base(configuration, standbyOptions, serviceProvider, environment) { }
126124

127125
public bool BlobExistsReturnValue { get; set; }
128126

0 commit comments

Comments
 (0)