Add BootstrapRefreshInterval #2052

ciaozhang · 2023-04-06T23:20:29Z

Add a new timer with exponential backoff for failing to fetch metadata at bootstrap til we get the first config (30s)

src/Microsoft.IdentityModel.Tokens/BaseConfigurationManager.cs

src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs

TimHannMSFT · 2023-04-10T22:12:50Z

src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs

+                        _bootstrapRefreshRetryCount = (_bootstrapRefreshRetryCount < int.MaxValue) ? _bootstrapRefreshRetryCount + 1 : int.MaxValue;
+
+                        if (_bootstrapRefreshRetryCount <= BootstrapRefreshMaxAttempt)
+                            _syncAfter = DateTimeUtil.Add(now.UtcDateTime, BootstrapRefreshInterval);


If we come in here we should log which retry # we're on at a fairly high log level, maybe max-1

TimHannMSFT · 2023-04-10T22:16:49Z

test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/ConfigurationManagerTests.cs

+            catch (Exception firstFetchMetadataFailure)
+            {
+                // Refresh interval is BootstrapRefreshInterval
+                var syncAfter = configManager.GetType().GetField("_syncAfter", BindingFlags.NonPublic | BindingFlags.Instance).GetValue(configManager);


Can we just expose an internal getter here for the tests? Using reflection seems unnecessarily brittle to me. If someone updates this we'll have to wait til runtime to catch the break.

src/Microsoft.IdentityModel.Tokens/BaseConfigurationManager.cs

TimHannMSFT

This looks pretty good. I think we need range validation on the new public controls. Given these are new configurable settings they should be exposed via the config layer too. Perhaps we can keep internal for now and then expose publicly when we're able to add via config?

TimHannMSFT · 2023-04-10T22:29:31Z

In general would also be good to ensure the description contains not only what we're doing (I think you cover that pretty well) but why we're doing the work (i.e. because some partners don't have network at bootstrap and thus would, before this fix, be stuck not able to validate tokens for 5 minutes)

TimHannMSFT · 2023-04-11T17:57:46Z

As discussed in standup, let's remove the public getter/setter for now and not have any new public parts AND let's switch to an exponential backoff with jitter up to 5 min

TimHannMSFT · 2023-04-12T17:41:38Z

src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs

                        if (_currentConfiguration == null) // Throw an exception if there's no configuration to return.
+                        {
+                            if (_bootstrapRefreshInterval < DefaultRefreshInterval)


This evaluation needs to be done AFTER we calculate the time for sync after. e.g. It's possible 1_bootstrapRefreshInterval` here will be 256 seconds which is less than 5 minutes but THEN we will double it on line 200 resulting in >8 minutes of wait time so it will go from ~4min of wait to ~8 min of wait, then back down to 5 minutes of wait. DefaultRefreshInterval should always be the max for the failure condition retry

Additionally, when this is done in the correct spot, I think it should be keying off he MinimulmRefreshInterval, not the default. Reason being we don't want this more aggressive strategy to be LESS aggressive than the configured default. We seem to currently allow folks to set this to retry every second which seems a bit dangerous but at least that won't be the default.

@brentschmaltz do you think we should look into a higher min refresh time? Allowing people to refresh every second on error seems awfully agressive. NOTE: we don't have to close on that here but want to flag it.

TimHannMSFT · 2023-04-12T21:02:09Z

src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs

+                            {
+                                // Adopt exponential backoff for bootstrap refresh interval with a jitter if it is not longer than 5 minutes.
+                                _bootstrapRefreshInterval += _bootstrapRefreshInterval;
+                                _syncAfter = DateTimeUtil.Add(now.UtcDateTime, _bootstrapRefreshInterval + TimeSpan.FromMilliseconds(new Random().Next((int)(_bootstrapRefreshInterval.TotalMilliseconds / 2))));


Have a look at the link @victorm-hernandez shared: https://aws.amazon.com/blogs/architecture/exponential-backoff-and-jitter/

It seems to suggest the "Full Jitter" or "Decorrelated Jitter" approach. I think what you have is something close to "Equal Jitter" which is mostly okay but let's make sure to cap it at the RefreshInterval to avoid cases where the more aggressive mode actually waits LONGER in between calls than the steady state.

TimHannMSFT

Would like to see us cap out at the RefreshInterval with exponential backoff til then. Current implementation has strange, unintuitive behavior from a client perspective.

victorm-hernandez · 2023-04-12T22:12:54Z

src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs

+                            if (_bootstrapRefreshInterval < DefaultRefreshInterval)
+                            {
+                                // Adopt exponential backoff for bootstrap refresh interval with a jitter if it is not longer than 5 minutes.
+                                _bootstrapRefreshInterval += _bootstrapRefreshInterval;


_bootstrapRefreshInterval

This increment is not enough to be called exponential. You can do something like Math.Pow(2, retryAttempt) #Closed

Isn't this identical? Doubling each time is the same as (2 ** retry attempt) if we start at 1

2 ** 0 == 1 == 1
2 ** 1 == 2 == 1+1
2 ** 2 == 4 == 2+2
2 * 3 == 8 == 4+4
...

Nevermind this, Tim pointed out this is actually equivalent. Since you are duplicating the value each time.

victorm-hernandez · 2023-04-12T22:14:12Z

src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs

                        if (_currentConfiguration == null) // Throw an exception if there's no configuration to return.
+                        {
+                            if (_bootstrapRefreshInterval < DefaultRefreshInterval)


_bootstrapRefreshInterval

We should check the type of exception, only transient HTTP errors should be included here not every single exception. #WontFix

victorm-hernandez · 2023-04-12T22:16:39Z

src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs

+                            {
+                                // Adopt exponential backoff for bootstrap refresh interval with a jitter if it is not longer than 5 minutes.
+                                _bootstrapRefreshInterval += _bootstrapRefreshInterval;
+                                _syncAfter = DateTimeUtil.Add(now.UtcDateTime, _bootstrapRefreshInterval + TimeSpan.FromMilliseconds(new Random().Next((int)(_bootstrapRefreshInterval.TotalMilliseconds / 2))));


_bootstrapRefreshInterval.TotalMilliseconds

The jitter could be increased as well, up to 3 secs maybe?

victorm-hernandez · 2023-04-12T22:18:38Z

src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs

+                            if (_bootstrapRefreshInterval < DefaultRefreshInterval)
+                            {
+                                // Adopt exponential backoff for bootstrap refresh interval with a jitter if it is not longer than 5 minutes.
+                                _bootstrapRefreshInterval += _bootstrapRefreshInterval;


_bootstrapRefreshInterval

This value is never reset, we should set it back to the default value upon successful retrieval of the config. #Closed

I think we only bootstrap once. Once we get a config we use that stale config forever and only retry every RefreshInterval

victorm-hernandez · 2023-04-12T22:21:22Z

test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/ConfigurationManagerTests.cs

+        {
+            var context = new CompareContext($"{this}.BootstrapRefreshIntervalTest");
+
+            var documentRetriever = new HttpDocumentRetriever(HttpResponseMessageUtils.SetupHttpClientThatReturns("OpenIdConnectMetadata.json", HttpStatusCode.NotFound));


Not found is not a transient error. We should not retry if this happens. This means that the URL we are using is wrong. #WontFix

We can have a collection of HTTP status codes that are transient and those that are permanent to decide if a retry is required.

In addition we should also have a list of .NET exceptions for scenarios like timeouts.

Agree we should take into consideration retriable vs non-retriable errors but IMO can wait til a subsequent interation.

victorm-hernandez · 2023-04-12T22:29:24Z

src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs

@@ -190,15 +191,32 @@ public async Task<T> GetConfigurationAsync(CancellationToken cancel)
                    catch (Exception ex)


Exception

The change here is limiting when the next refresh can be requested rather than performing another request.

Have we discussed the possibility of doing the retry in here? #WontFix

IIRC the initial fetch already has a retry so that would be multiple levels of retries. That's not to say we shouldn't do this but my preference would be to have that as a separate change.

victorm-hernandez

retry up to 4 times if no metadata has been successfully retrieved

f0a2ed2

ciaozhang changed the title ~~retry up to 4 times if no metadata has been successfully retrieved~~ Add BootstrapRefreshInterval Apr 6, 2023

update test

86421a0

TimHannMSFT reviewed Apr 10, 2023

View reviewed changes

src/Microsoft.IdentityModel.Tokens/BaseConfigurationManager.cs Outdated Show resolved Hide resolved

TimHannMSFT reviewed Apr 10, 2023

View reviewed changes

src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs Outdated Show resolved Hide resolved

TimHannMSFT reviewed Apr 10, 2023

View reviewed changes

src/Microsoft.IdentityModel.Tokens/BaseConfigurationManager.cs Outdated Show resolved Hide resolved

TimHannMSFT requested changes Apr 10, 2023

View reviewed changes

Brent Schmaltz added 2 commits April 11, 2023 15:58

address comment

642fafc

add jitter

5f55365

ciaozhang requested a review from TimHannMSFT April 12, 2023 17:16

TimHannMSFT reviewed Apr 12, 2023

View reviewed changes

TimHannMSFT requested changes Apr 12, 2023

View reviewed changes

victorm-hernandez reviewed Apr 12, 2023

View reviewed changes

Adopt decorrelated jitter

4eb774b

TimHannMSFT approved these changes Apr 12, 2023

View reviewed changes

victorm-hernandez approved these changes Apr 12, 2023

View reviewed changes

update test

db8c98a

ciaozhang merged commit 90da399 into dev Apr 13, 2023

This was referenced Apr 13, 2023

Issue with _currentConfiguration being null after the first call #2050

Open

Add BootstrapRefreshInterval into ConfigurationManager #2025

Closed

brentschmaltz deleted the Xiao/BootstrapRefresh branch June 2, 2023 21:51

ciaozhang mentioned this pull request Jun 14, 2023

[Bug] RefreshInterval is too long if no metadata has been retrieved (IDX20803) #1930

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add BootstrapRefreshInterval #2052

Add BootstrapRefreshInterval #2052

ciaozhang commented Apr 6, 2023 •

edited

Loading

TimHannMSFT Apr 10, 2023

TimHannMSFT Apr 10, 2023

TimHannMSFT left a comment

TimHannMSFT commented Apr 10, 2023

TimHannMSFT commented Apr 11, 2023

TimHannMSFT Apr 12, 2023

TimHannMSFT Apr 12, 2023

TimHannMSFT Apr 12, 2023

TimHannMSFT left a comment

victorm-hernandez Apr 12, 2023 •

edited

Loading

TimHannMSFT Apr 12, 2023

victorm-hernandez Apr 12, 2023

victorm-hernandez Apr 12, 2023 •

edited

Loading

victorm-hernandez Apr 12, 2023

victorm-hernandez Apr 12, 2023 •

edited

Loading

TimHannMSFT Apr 12, 2023

victorm-hernandez Apr 12, 2023 •

edited

Loading

victorm-hernandez Apr 12, 2023

TimHannMSFT Apr 12, 2023

victorm-hernandez Apr 12, 2023 •

edited

Loading

TimHannMSFT Apr 12, 2023

victorm-hernandez left a comment

		@@ -190,15 +191,32 @@ public async Task<T> GetConfigurationAsync(CancellationToken cancel)
		catch (Exception ex)

Add BootstrapRefreshInterval #2052

Add BootstrapRefreshInterval #2052

Conversation

ciaozhang commented Apr 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimHannMSFT left a comment

Choose a reason for hiding this comment

TimHannMSFT commented Apr 10, 2023

TimHannMSFT commented Apr 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimHannMSFT left a comment

Choose a reason for hiding this comment

victorm-hernandez Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victorm-hernandez Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victorm-hernandez Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victorm-hernandez Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victorm-hernandez Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victorm-hernandez left a comment

Choose a reason for hiding this comment

ciaozhang commented Apr 6, 2023 •

edited

Loading

victorm-hernandez Apr 12, 2023 •

edited

Loading

victorm-hernandez Apr 12, 2023 •

edited

Loading

victorm-hernandez Apr 12, 2023 •

edited

Loading

victorm-hernandez Apr 12, 2023 •

edited

Loading

victorm-hernandez Apr 12, 2023 •

edited

Loading