From 6771a494ffe2a361e4cf4e8044bfa2c22694722b Mon Sep 17 00:00:00 2001 From: BrentSchmaltz Date: Fri, 4 Oct 2024 16:56:14 -0700 Subject: [PATCH] Adjust for RefreshInterval not influencing AutomaticRefreshInterval. (#2870) * Do not update _syncAfter unless new configuration was received. Add random % of AutomaticRefreshInterval to avoid spike in trafic in all cases * Rename method * Fixed Interlock ordering --------- Co-authored-by: id4s --- .../Configuration/ConfigurationManager.cs | 79 ++++------- .../ConfigurationManagerTests.cs | 134 ++++++++++++++++-- .../OpenIdConnectProtocolValidatorTests.cs | 5 - .../InMemoryDocumentRetriever.cs | 23 ++- 4 files changed, 167 insertions(+), 74 deletions(-) diff --git a/src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs b/src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs index f747a37840..cd06188f0f 100644 --- a/src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs +++ b/src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs @@ -27,7 +27,6 @@ public class ConfigurationManager : BaseConfigurationManager, IConfigurationM private readonly IConfigurationRetriever _configRetriever; private readonly IConfigurationValidator _configValidator; private T _currentConfiguration; - private TimeSpan _bootstrapRefreshInterval = TimeSpan.FromSeconds(1); // task states are used to ensure the call to 'update config' (UpdateCurrentConfiguration) is a singleton. Uses Interlocked.CompareExchange. // metadata is not being obtained @@ -169,6 +168,7 @@ public virtual async Task GetConfigurationAsync(CancellationToken cancel) return _currentConfiguration; } +#pragma warning disable CA1031 // Do not catch general exception types try { // Don't use the individual CT here, this is a shared operation that shouldn't be affected by an individual's cancellation. @@ -190,65 +190,29 @@ public virtual async Task GetConfigurationAsync(CancellationToken cancel) result.ErrorMessage))); } - // Add a random amount between 0 and 5% of AutomaticRefreshInterval jitter to avoid spike traffic to IdentityProvider. - _syncAfter = DateTimeUtil.Add(DateTime.UtcNow, AutomaticRefreshInterval + - TimeSpan.FromSeconds(new Random().Next((int)AutomaticRefreshInterval.TotalSeconds / 20))); - - _currentConfiguration = configuration; + UpdateConfiguration(configuration); } catch (Exception ex) { fetchMetadataFailure = ex; - // In this case configuration was never obtained. - if (_currentConfiguration == null) - { - if (_bootstrapRefreshInterval < RefreshInterval) - { - // Adopt exponential backoff for bootstrap refresh interval with a decorrelated jitter if it is not longer than the refresh interval. - TimeSpan _bootstrapRefreshIntervalWithJitter = TimeSpan.FromSeconds(new Random().Next((int)_bootstrapRefreshInterval.TotalSeconds)); - _bootstrapRefreshInterval += _bootstrapRefreshInterval; - _syncAfter = DateTimeUtil.Add(DateTime.UtcNow, _bootstrapRefreshIntervalWithJitter); - } - else - { - _syncAfter = DateTimeUtil.Add( - DateTime.UtcNow, - AutomaticRefreshInterval < RefreshInterval ? AutomaticRefreshInterval : RefreshInterval); - } - - throw LogHelper.LogExceptionMessage( - new InvalidOperationException( - LogHelper.FormatInvariant( - LogMessages.IDX20803, - LogHelper.MarkAsNonPII(MetadataAddress ?? "null"), - LogHelper.MarkAsNonPII(_syncAfter), - LogHelper.MarkAsNonPII(ex)), - ex)); - } - else - { - _syncAfter = DateTimeUtil.Add( - DateTime.UtcNow, - AutomaticRefreshInterval < RefreshInterval ? AutomaticRefreshInterval : RefreshInterval); - - LogHelper.LogExceptionMessage( - new InvalidOperationException( - LogHelper.FormatInvariant( - LogMessages.IDX20806, - LogHelper.MarkAsNonPII(MetadataAddress ?? "null"), - LogHelper.MarkAsNonPII(ex)), - ex)); - } + LogHelper.LogExceptionMessage( + new InvalidOperationException( + LogHelper.FormatInvariant( + LogMessages.IDX20806, + LogHelper.MarkAsNonPII(MetadataAddress ?? "null"), + LogHelper.MarkAsNonPII(ex)), + ex)); } finally { _configurationNullLock.Release(); } +#pragma warning restore CA1031 // Do not catch general exception types } else { - if (Interlocked.CompareExchange(ref _configurationRetrieverState, ConfigurationRetrieverIdle, ConfigurationRetrieverRunning) != ConfigurationRetrieverRunning) + if (Interlocked.CompareExchange(ref _configurationRetrieverState, ConfigurationRetrieverRunning, ConfigurationRetrieverIdle) == ConfigurationRetrieverIdle) { _ = Task.Run(UpdateCurrentConfiguration, CancellationToken.None); } @@ -285,7 +249,7 @@ private void UpdateCurrentConfiguration() if (_configValidator == null) { - _currentConfiguration = configuration; + UpdateConfiguration(configuration); } else { @@ -298,7 +262,7 @@ private void UpdateCurrentConfiguration() LogMessages.IDX20810, result.ErrorMessage))); else - _currentConfiguration = configuration; + UpdateConfiguration(configuration); } } catch (Exception ex) @@ -313,12 +277,18 @@ private void UpdateCurrentConfiguration() } finally { - _syncAfter = DateTimeUtil.Add(DateTime.UtcNow, AutomaticRefreshInterval); Interlocked.Exchange(ref _configurationRetrieverState, ConfigurationRetrieverIdle); } #pragma warning restore CA1031 // Do not catch general exception types } + private void UpdateConfiguration(T configuration) + { + _currentConfiguration = configuration; + _syncAfter = DateTimeUtil.Add(DateTime.UtcNow, AutomaticRefreshInterval + + TimeSpan.FromSeconds(new Random().Next((int)AutomaticRefreshInterval.TotalSeconds / 20))); + } + /// /// Obtains an updated version of Configuration. /// @@ -332,8 +302,9 @@ public override async Task GetBaseConfigurationAsync(Cancella } /// - /// Requests that then next call to obtain new configuration. - /// If it is a first force refresh or the last refresh was greater than then the next call to will retrieve new configuration. + /// Triggers updating metadata when: + /// 1. Called the first time. + /// 2. The time between when this method was called and DateTimeOffset.Now is greater than . /// If == then this method does nothing. /// public override void RequestRefresh() @@ -343,10 +314,10 @@ public override void RequestRefresh() if (now >= DateTimeUtil.Add(_lastRequestRefresh.UtcDateTime, RefreshInterval) || _isFirstRefreshRequest) { _isFirstRefreshRequest = false; - _lastRequestRefresh = now; - if (Interlocked.CompareExchange(ref _configurationRetrieverState, ConfigurationRetrieverIdle, ConfigurationRetrieverRunning) != ConfigurationRetrieverRunning) + if (Interlocked.CompareExchange(ref _configurationRetrieverState, ConfigurationRetrieverRunning, ConfigurationRetrieverIdle) == ConfigurationRetrieverIdle) { _ = Task.Run(UpdateCurrentConfiguration, CancellationToken.None); + _lastRequestRefresh = now; } } } diff --git a/test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/ConfigurationManagerTests.cs b/test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/ConfigurationManagerTests.cs index d64a13649c..83d7f5d69c 100644 --- a/test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/ConfigurationManagerTests.cs +++ b/test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/ConfigurationManagerTests.cs @@ -26,7 +26,7 @@ public class ConfigurationManagerTests { /// /// This test reaches out the the internet to fetch the OpenIdConnectConfiguration from the specified metadata address. - /// There is no validaiton of the configuration. The validation is done in the OpenIdConnectConfigurationSerializationTests.Deserialize + /// There is no validation of the configuration. The validation is done in the OpenIdConnectConfigurationSerializationTests.Deserialize /// against values obtained 2/2/2024 /// /// @@ -209,25 +209,120 @@ public async Task FetchMetadataFailureTest() TestUtilities.AssertFailIfErrors(context); } + [Fact] + public async Task VerifyInterlockGuardForRequestRefresh() + { + ManualResetEvent waitEvent = new ManualResetEvent(false); + ManualResetEvent signalEvent = new ManualResetEvent(false); + InMemoryDocumentRetriever inMemoryDocumentRetriever = InMemoryDocumentRetrieverWithEvents(waitEvent, signalEvent); + + var configurationManager = new ConfigurationManager( + "AADCommonV1Json", + new OpenIdConnectConfigurationRetriever(), + inMemoryDocumentRetriever); + + // populate the configurationManager with AADCommonV1Config + TestUtilities.SetField(configurationManager, "_currentConfiguration", OpenIdConfigData.AADCommonV1Config); + + // InMemoryDocumentRetrieverWithEvents will block until waitEvent.Set() is called. + // The first RequestRefresh will not have finished before the next RequestRefresh() is called. + // The guard '_lastRequestRefresh' will not block as we set it to DateTimeOffset.MinValue. + // Interlocked guard will block. + // Configuration should be AADCommonV1Config + signalEvent.Reset(); + configurationManager.RequestRefresh(); + + // InMemoryDocumentRetrieverWithEvents will signal when it is OK to change the MetadataAddress + // otherwise, it may be the case that the MetadataAddress is changed before the previous Task has finished. + signalEvent.WaitOne(); + + // AADCommonV1Json would have been passed to the the previous retriever, which is blocked on an event. + configurationManager.MetadataAddress = "AADCommonV2Json"; + TestUtilities.SetField(configurationManager, "_lastRequestRefresh", DateTimeOffset.MinValue); + configurationManager.RequestRefresh(); + + // Set the event to release the lock and let the previous retriever finish. + waitEvent.Set(); + + // Configuration should be AADCommonV1Config + var configuration = await configurationManager.GetConfigurationAsync(); + Assert.True(configuration.Issuer.Equals(OpenIdConfigData.AADCommonV1Config.Issuer), + $"configuration.Issuer from configurationManager was not as expected," + + $"configuration.Issuer: '{configuration.Issuer}' != expected '{OpenIdConfigData.AADCommonV1Config.Issuer}'."); + } + + [Fact] + public async Task VerifyInterlockGuardForGetConfigurationAsync() + { + ManualResetEvent waitEvent = new ManualResetEvent(false); + ManualResetEvent signalEvent = new ManualResetEvent(false); + + InMemoryDocumentRetriever inMemoryDocumentRetriever = InMemoryDocumentRetrieverWithEvents(waitEvent, signalEvent); + waitEvent.Set(); + + var configurationManager = new ConfigurationManager( + "AADCommonV1Json", + new OpenIdConnectConfigurationRetriever(), + inMemoryDocumentRetriever); + + OpenIdConnectConfiguration configuration = await configurationManager.GetConfigurationAsync(); + + // InMemoryDocumentRetrieverWithEvents will block until waitEvent.Set() is called. + // The GetConfigurationAsync to update config will not have finished before the next GetConfigurationAsync() is called. + // The guard '_syncAfter' will not block as we set it to DateTimeOffset.MinValue. + // Interlocked guard should block. + // Configuration should be AADCommonV1Config + + waitEvent.Reset(); + signalEvent.Reset(); + + TestUtilities.SetField(configurationManager, "_syncAfter", DateTimeOffset.MinValue); + await configurationManager.GetConfigurationAsync(CancellationToken.None); + + // InMemoryDocumentRetrieverWithEvents will signal when it is OK to change the MetadataAddress + // otherwise, it may be the case that the MetadataAddress is changed before the previous Task has finished. + signalEvent.WaitOne(); + + // AADCommonV1Json would have been passed to the the previous retriever, which is blocked on an event. + configurationManager.MetadataAddress = "AADCommonV2Json"; + await configurationManager.GetConfigurationAsync(CancellationToken.None); + + // Set the event to release the lock and let the previous retriever finish. + waitEvent.Set(); + + // Configuration should be AADCommonV1Config + configuration = await configurationManager.GetConfigurationAsync(); + Assert.True(configuration.Issuer.Equals(OpenIdConfigData.AADCommonV1Config.Issuer), + $"configuration.Issuer from configurationManager was not as expected," + + $" configuration.Issuer: '{configuration.Issuer}' != expected: '{OpenIdConfigData.AADCommonV1Config.Issuer}'."); + } + [Fact] public async Task BootstrapRefreshIntervalTest() { var context = new CompareContext($"{this}.BootstrapRefreshIntervalTest"); - var documentRetriever = new HttpDocumentRetriever(HttpResponseMessageUtils.SetupHttpClientThatReturns("OpenIdConnectMetadata.json", HttpStatusCode.NotFound)); - var configManager = new ConfigurationManager("OpenIdConnectMetadata.json", new OpenIdConnectConfigurationRetriever(), documentRetriever) { RefreshInterval = TimeSpan.FromSeconds(2) }; + var documentRetriever = new HttpDocumentRetriever( + HttpResponseMessageUtils.SetupHttpClientThatReturns("OpenIdConnectMetadata.json", HttpStatusCode.NotFound)); + + var configManager = new ConfigurationManager( + "OpenIdConnectMetadata.json", + new OpenIdConnectConfigurationRetriever(), + documentRetriever) + { RefreshInterval = TimeSpan.FromSeconds(2) }; - // First time to fetch metadata. + // ConfigurationManager._syncAfter is set to DateTimeOffset.MinValue on startup + // If obtaining the metadata fails due to error, the value should not change try { var configuration = await configManager.GetConfigurationAsync(); } catch (Exception firstFetchMetadataFailure) { - // Refresh interval is BootstrapRefreshInterval - var syncAfter = configManager.GetType().GetField("_syncAfter", BindingFlags.NonPublic | BindingFlags.Instance).GetValue(configManager); - if ((DateTimeOffset)syncAfter > DateTime.UtcNow + TimeSpan.FromSeconds(2)) - context.AddDiff($"Expected the refresh interval is longer than 2 seconds."); + // _syncAfter should not have been changed, because the fetch failed. + var syncAfter = TestUtilities.GetField(configManager, "_syncAfter"); + if ((DateTimeOffset)syncAfter != DateTimeOffset.MinValue) + context.AddDiff($"ConfigurationManager._syncAfter: '{syncAfter}' should equal '{DateTimeOffset.MinValue}'."); if (firstFetchMetadataFailure.InnerException == null) context.AddDiff($"Expected exception to contain inner exception for fetch metadata failure."); @@ -243,11 +338,10 @@ public async Task BootstrapRefreshIntervalTest() if (secondFetchMetadataFailure.InnerException == null) context.AddDiff($"Expected exception to contain inner exception for fetch metadata failure."); - syncAfter = configManager.GetType().GetField("_syncAfter", BindingFlags.NonPublic | BindingFlags.Instance).GetValue(configManager); - - // Refresh interval is RefreshInterval - if ((DateTimeOffset)syncAfter > DateTime.UtcNow + configManager.RefreshInterval) - context.AddDiff($"Expected the refresh interval is longer than 2 seconds."); + // _syncAfter should not have been changed, because the fetch failed. + syncAfter = TestUtilities.GetField(configManager, "_syncAfter"); + if ((DateTimeOffset)syncAfter != DateTimeOffset.MinValue) + context.AddDiff($"ConfigurationManager._syncAfter: '{syncAfter}' should equal '{DateTimeOffset.MinValue}'."); IdentityComparer.AreEqual(firstFetchMetadataFailure, secondFetchMetadataFailure, context); } @@ -808,6 +902,20 @@ public static TheoryData + { + { "AADCommonV1Json", OpenIdConfigData.AADCommonV1Json }, + { "https://login.microsoftonline.com/common/discovery/keys", OpenIdConfigData.AADCommonV1JwksString }, + { "AADCommonV2Json", OpenIdConfigData.AADCommonV2Json }, + { "https://login.microsoftonline.com/common/discovery/v2.0/keys", OpenIdConfigData.AADCommonV2JwksString } + }, + waitEvent, + signalEvent); + } + public class ConfigurationManagerTheoryData : TheoryDataBase where T : class { public ConfigurationManager ConfigurationManager { get; set; } diff --git a/test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/OpenIdConnectProtocolValidatorTests.cs b/test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/OpenIdConnectProtocolValidatorTests.cs index 3b1560d7fd..5ea0076e2f 100644 --- a/test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/OpenIdConnectProtocolValidatorTests.cs +++ b/test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/OpenIdConnectProtocolValidatorTests.cs @@ -12,11 +12,8 @@ using System.Threading.Tasks; using Microsoft.IdentityModel.TestUtils; using Microsoft.IdentityModel.Tokens; -using Newtonsoft.Json; using Xunit; -#pragma warning disable CS3016 // Arrays as attribute arguments is not CLS-compliant - namespace Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests { /// @@ -1813,5 +1810,3 @@ protected override void OnEventWritten(EventWrittenEventArgs eventData) } } } - -#pragma warning restore CS3016 // Arrays as attribute arguments is not CLS-compliant diff --git a/test/Microsoft.IdentityModel.TestUtils/InMemoryDocumentRetriever.cs b/test/Microsoft.IdentityModel.TestUtils/InMemoryDocumentRetriever.cs index 1a1f81fe6f..1c88ba93cd 100644 --- a/test/Microsoft.IdentityModel.TestUtils/InMemoryDocumentRetriever.cs +++ b/test/Microsoft.IdentityModel.TestUtils/InMemoryDocumentRetriever.cs @@ -14,16 +14,25 @@ namespace Microsoft.IdentityModel.TestUtils /// public class InMemoryDocumentRetriever : IDocumentRetriever { - private readonly IDictionary _configurations; + private readonly Dictionary _configurations; + private ManualResetEvent _waitEvent; + private ManualResetEvent _signalEvent; /// /// Initializes a new instance of the class. /// - public InMemoryDocumentRetriever(IDictionary configuration) + public InMemoryDocumentRetriever(Dictionary configuration) { _configurations = configuration; } + public InMemoryDocumentRetriever(Dictionary configuration, ManualResetEvent waitEvent, ManualResetEvent signalEvent) + { + _configurations = configuration; + _waitEvent = waitEvent; + _signalEvent = signalEvent; + } + /// /// Returns the document passed in constructor in dictionary./> /// @@ -32,6 +41,16 @@ public InMemoryDocumentRetriever(IDictionary configuration) /// UTF8 decoding of bytes in the file. public async Task GetDocumentAsync(string address, CancellationToken cancel) { + // Some tests change the Metadata address on ConfigurationManger to test different scenarios. + // This event is used to let the test know that the GetDocumentAsync method has been called, and the test can now change the Metadata address. + if (_signalEvent != null) + _signalEvent.Set(); + + // This event lets the caller control when metadata can be returned. + // Useful when testing delays. + if (_waitEvent != null) + _waitEvent.WaitOne(); + return await Task.FromResult(_configurations[address]).ConfigureAwait(false); } }