Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adjust for RefreshInterval not influencing AutomaticRefreshInterval. #2870

Merged
merged 3 commits into from
Oct 4, 2024
Merged

Adjust for RefreshInterval not influencing AutomaticRefreshInterval. #2870

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dcb785c
Do not update _syncAfter unless new configuration was received.
HP712 Oct 3, 2024
9a502c9
Rename method
HP712 Oct 4, 2024
e9f4fc3
Fixed Interlock ordering
HP712 Oct 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 25 additions & 54 deletions src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs
View file
Open in desktop
brentschmaltz marked this conversation as resolved.
Show resolved Hide resolved
brentschmaltz marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ public class ConfigurationManager<T> : BaseConfigurationManager, IConfigurationM
private readonly IConfigurationRetriever<T> _configRetriever;
private readonly IConfigurationValidator<T> _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
Expand Down Expand Up @@ -169,6 +168,7 @@ public virtual async Task<T> 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.
Expand All @@ -190,65 +190,29 @@ public virtual async Task<T> 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.
pmaytak marked this conversation as resolved.
Show resolved Hide resolved
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,
pmaytak marked this conversation as resolved.
Show resolved Hide resolved
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);
}
Expand Down Expand Up @@ -285,7 +249,7 @@ private void UpdateCurrentConfiguration()

if (_configValidator == null)
{
_currentConfiguration = configuration;
UpdateConfiguration(configuration);
}
else
{
Expand All @@ -298,7 +262,7 @@ private void UpdateCurrentConfiguration()
LogMessages.IDX20810,
result.ErrorMessage)));
else
_currentConfiguration = configuration;
UpdateConfiguration(configuration);
}
}
catch (Exception ex)
Expand All @@ -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)));
brentschmaltz marked this conversation as resolved.
Show resolved Hide resolved
}

/// <summary>
/// Obtains an updated version of Configuration.
/// </summary>
Expand All @@ -332,8 +302,9 @@ public override async Task<BaseConfiguration> GetBaseConfigurationAsync(Cancella
}

/// <summary>
/// Requests that then next call to <see cref="GetConfigurationAsync()"/> obtain new configuration.
/// <para>If it is a first force refresh or the last refresh was greater than <see cref="BaseConfigurationManager.RefreshInterval"/> then the next call to <see cref="GetConfigurationAsync()"/> will retrieve new configuration.</para>
/// Triggers updating metadata when:
/// <para>1. Called the first time.</para>
/// <para>2. The time between when this method was called and DateTimeOffset.Now is greater than <see cref="BaseConfigurationManager.RefreshInterval"/>.</para>
/// <para>If <see cref="BaseConfigurationManager.RefreshInterval"/> == <see cref="TimeSpan.MaxValue"/> then this method does nothing.</para>
/// </summary>
public override void RequestRefresh()
Expand All @@ -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;
}
}
}
Expand Down
134 changes: 121 additions & 13 deletions test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/ConfigurationManagerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ public class ConfigurationManagerTests
{
/// <summary>
/// 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
/// </summary>
/// <param name="theoryData"></param>
Expand Down Expand Up @@ -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<OpenIdConnectConfiguration>(
"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<OpenIdConnectConfiguration>(
"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<OpenIdConnectConfiguration>("OpenIdConnectMetadata.json", new OpenIdConnectConfigurationRetriever(), documentRetriever) { RefreshInterval = TimeSpan.FromSeconds(2) };
var documentRetriever = new HttpDocumentRetriever(
HttpResponseMessageUtils.SetupHttpClientThatReturns("OpenIdConnectMetadata.json", HttpStatusCode.NotFound));

var configManager = new ConfigurationManager<OpenIdConnectConfiguration>(
"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.");
Expand All @@ -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);
}
Expand Down Expand Up @@ -808,6 +902,20 @@ public static TheoryData<ConfigurationManagerTheoryData<OpenIdConnectConfigurati
{ "https://login.microsoftonline.com/common/discovery/v2.0/keys", OpenIdConfigData.AADCommonV2JwksString }
});

private static InMemoryDocumentRetriever InMemoryDocumentRetrieverWithEvents(ManualResetEvent waitEvent, ManualResetEvent signalEvent)
{
return new InMemoryDocumentRetriever(
new Dictionary<string, string>
{
{ "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<T> : TheoryDataBase where T : class
{
public ConfigurationManager<T> ConfigurationManager { get; set; }
Expand Down
5 changes: 0 additions & 5 deletions ...rosoft.IdentityModel.Protocols.OpenIdConnect.Tests/OpenIdConnectProtocolValidatorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
/// <summary>
Expand Down Expand Up @@ -1813,5 +1810,3 @@ protected override void OnEventWritten(EventWrittenEventArgs eventData)
}
}
}

#pragma warning restore CS3016 // Arrays as attribute arguments is not CLS-compliant
Loading
Loading