-
Notifications
You must be signed in to change notification settings - Fork 39
add support for connecting to Azure Front Door with Azure App Configuration store as an origin #601
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.
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
base: preview
Are you sure you want to change the base?
Changes from 28 commits
07d60c1
c2fd979
d16a344
c8b3535
f3d6b58
a2e1c5d
9b2bcc9
bf85303
7c4fddb
d323ecd
a2ff1b1
5f16867
65b81fc
0e2c621
8fc28e8
2d73ebb
ef09e70
b8ca58e
f1e4bed
47a27ea
491e948
782dc33
dec256b
186288b
dbfa697
cfc83ce
3631731
2a7f79b
d78de74
43a83a0
7ff8e94
73be7e9
bd941f0
b89f870
4f5108f
4fc69d3
1d62185
6528a36
a846f0f
c95e9ce
e1740c9
29eed77
66eccf6
326d85b
984650e
e64307a
f19cb5d
1803e6b
4a54b2e
e544048
ad8c49f
ad5ac9b
33a9fb5
de1dc29
1abb191
911f815
568a7e8
4880269
23acfff
bd487dc
2c2a413
d8f280a
ae0e448
feaec53
3f71dd4
c1d15aa
0292b78
be6d888
cfb5cf6
eef8180
cae49ce
39ddcae
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,7 @@ | |
using System.Net.Http; | ||
using System.Net.Sockets; | ||
using System.Security; | ||
using System.Security.Cryptography; | ||
samsadsam marked this conversation as resolved.
Show resolved
Hide resolved
|
||
using System.Text; | ||
using System.Threading; | ||
using System.Threading.Tasks; | ||
|
@@ -39,6 +40,11 @@ internal class AzureAppConfigurationProvider : ConfigurationProvider, IConfigura | |
private Dictionary<Uri, ConfigurationClientBackoffStatus> _configClientBackoffs = new Dictionary<Uri, ConfigurationClientBackoffStatus>(); | ||
private DateTimeOffset _nextCollectionRefreshTime; | ||
|
||
#region Cdn | ||
private string _configVersion = null; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All we should need is a single etag. Doesn't matter if it's configuration or a feature flag. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about when feature flag collection changes and the config collection doesn’t scenario? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After looking, this optimization seems valid to me. No need to reload configuration if flags are changing. |
||
private string _ffCollectionVersion = null; | ||
#endregion | ||
|
||
private readonly TimeSpan MinRefreshInterval; | ||
|
||
// The most-recent time when the refresh operation attempted to load the initial configuration | ||
|
@@ -280,8 +286,8 @@ public async Task RefreshAsync(CancellationToken cancellationToken) | |
List<KeyValueChange> keyValueChanges = null; | ||
Dictionary<string, ConfigurationSetting> data = null; | ||
Dictionary<string, ConfigurationSetting> ffCollectionData = null; | ||
bool ffCollectionUpdated = false; | ||
bool refreshAll = false; | ||
string ffCollectionUpdatedChangedEtag = null; | ||
string refreshAllChangedEtag = null; | ||
StringBuilder logInfoBuilder = new StringBuilder(); | ||
StringBuilder logDebugBuilder = new StringBuilder(); | ||
|
||
|
@@ -294,8 +300,8 @@ await ExecuteWithFailOverPolicyAsync(clients, async (client) => | |
keyValueChanges = new List<KeyValueChange>(); | ||
data = null; | ||
ffCollectionData = null; | ||
ffCollectionUpdated = false; | ||
refreshAll = false; | ||
ffCollectionUpdatedChangedEtag = null; | ||
refreshAllChangedEtag = null; | ||
logDebugBuilder.Clear(); | ||
logInfoBuilder.Clear(); | ||
Uri endpoint = _configClientManager.GetEndpointForClient(client); | ||
|
@@ -305,7 +311,17 @@ await ExecuteWithFailOverPolicyAsync(clients, async (client) => | |
// Get key value collection changes if RegisterAll was called | ||
if (isRefreshDue) | ||
{ | ||
refreshAll = await HaveCollectionsChanged( | ||
if (_options.IsCdnEnabled) | ||
{ | ||
if (_configVersion == null && _kvEtags.Count > 0) | ||
{ | ||
_configVersion = CalculateHash(_kvEtags.SelectMany(kvp => kvp.Value.Select(mc => mc.IfNoneMatch.ToString()))); | ||
} | ||
|
||
_options.CdnCacheBustingAccessor.CurrentToken = _configVersion; | ||
} | ||
|
||
refreshAllChangedEtag = await HaveCollectionsChanged( | ||
_options.Selectors.Where(selector => !selector.IsFeatureFlagSelector), | ||
_kvEtags, | ||
client, | ||
|
@@ -314,7 +330,17 @@ await ExecuteWithFailOverPolicyAsync(clients, async (client) => | |
} | ||
else | ||
{ | ||
refreshAll = await RefreshIndividualKvWatchers( | ||
if (_options.IsCdnEnabled) | ||
{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it possible for a deleted watched kv to have null etag. Then There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks, added a fallback |
||
if (_configVersion == null && _watchedIndividualKvs.Count > 0) | ||
{ | ||
_configVersion = CalculateHash(_watchedIndividualKvs.Select(kvp => kvp.Value.ETag.ToString())); | ||
} | ||
|
||
_options.CdnCacheBustingAccessor.CurrentToken = _configVersion; | ||
} | ||
|
||
refreshAllChangedEtag = await RefreshIndividualKvWatchers( | ||
jimmyca15 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
client, | ||
keyValueChanges, | ||
refreshableIndividualKvWatchers, | ||
|
@@ -324,22 +350,43 @@ await ExecuteWithFailOverPolicyAsync(clients, async (client) => | |
cancellationToken).ConfigureAwait(false); | ||
} | ||
|
||
if (refreshAll) | ||
if (refreshAllChangedEtag != null) | ||
{ | ||
// Trigger a single load-all operation if a change was detected in one or more key-values with refreshAll: true, | ||
// or if any key-value collection change was detected. | ||
kvEtags = new Dictionary<KeyValueSelector, IEnumerable<MatchConditions>>(); | ||
ffEtags = new Dictionary<KeyValueSelector, IEnumerable<MatchConditions>>(); | ||
ffKeys = new HashSet<string>(); | ||
|
||
if (_options.IsCdnEnabled) | ||
{ | ||
// | ||
// Bust cdn cache | ||
_options.CdnCacheBustingAccessor.CurrentToken = refreshAllChangedEtag; | ||
|
||
// Reset versions so that next watch request will not use stale versions. | ||
_configVersion = null; | ||
_ffCollectionVersion = null; | ||
samsadsam marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
data = await LoadSelected(client, kvEtags, ffEtags, _options.Selectors, ffKeys, cancellationToken).ConfigureAwait(false); | ||
watchedIndividualKvs = await LoadKeyValuesRegisteredForRefresh(client, data, cancellationToken).ConfigureAwait(false); | ||
logInfoBuilder.AppendLine(LogHelper.BuildConfigurationUpdatedMessage()); | ||
return; | ||
} | ||
|
||
// Get feature flag changes | ||
ffCollectionUpdated = await HaveCollectionsChanged( | ||
if (_options.IsCdnEnabled) | ||
{ | ||
if (_ffCollectionVersion == null && _ffEtags.Count > 0) | ||
{ | ||
_ffCollectionVersion = CalculateHash(_ffEtags.SelectMany(kvp => kvp.Value.Select(mc => mc.IfNoneMatch.ToString()))); | ||
} | ||
|
||
_options.CdnCacheBustingAccessor.CurrentToken = _ffCollectionVersion; | ||
} | ||
|
||
ffCollectionUpdatedChangedEtag = await HaveCollectionsChanged( | ||
refreshableFfWatchers.Select(watcher => new KeyValueSelector | ||
{ | ||
KeyFilter = watcher.Key, | ||
|
@@ -350,11 +397,20 @@ await ExecuteWithFailOverPolicyAsync(clients, async (client) => | |
client, | ||
cancellationToken).ConfigureAwait(false); | ||
|
||
if (ffCollectionUpdated) | ||
if (ffCollectionUpdatedChangedEtag != null) | ||
{ | ||
ffEtags = new Dictionary<KeyValueSelector, IEnumerable<MatchConditions>>(); | ||
ffKeys = new HashSet<string>(); | ||
|
||
if (_options.IsCdnEnabled) | ||
{ | ||
// | ||
// Bust cdn cache | ||
_options.CdnCacheBustingAccessor.CurrentToken = ffCollectionUpdatedChangedEtag; | ||
// Reset ff collection version so that next ff watch request will not use stale version. | ||
_ffCollectionVersion = null; | ||
} | ||
|
||
ffCollectionData = await LoadSelected( | ||
client, | ||
new Dictionary<KeyValueSelector, IEnumerable<MatchConditions>>(), | ||
|
@@ -373,6 +429,9 @@ await ExecuteWithFailOverPolicyAsync(clients, async (client) => | |
cancellationToken) | ||
.ConfigureAwait(false); | ||
|
||
bool refreshAll = !string.IsNullOrEmpty(refreshAllChangedEtag); | ||
bool ffCollectionUpdated = !string.IsNullOrEmpty(ffCollectionUpdatedChangedEtag); | ||
|
||
if (refreshAll) | ||
{ | ||
_mappedData = await MapConfigurationSettings(data).ConfigureAwait(false); | ||
|
@@ -940,7 +999,7 @@ private async Task<Dictionary<KeyValueIdentifier, ConfigurationSetting>> LoadKey | |
return watchedIndividualKvs; | ||
} | ||
|
||
private async Task<bool> RefreshIndividualKvWatchers( | ||
private async Task<string> RefreshIndividualKvWatchers( | ||
ConfigurationClient client, | ||
List<KeyValueChange> keyValueChanges, | ||
IEnumerable<KeyValueWatcher> refreshableIndividualKvWatchers, | ||
|
@@ -963,7 +1022,7 @@ private async Task<bool> RefreshIndividualKvWatchers( | |
if (_watchedIndividualKvs.TryGetValue(watchedKeyLabel, out ConfigurationSetting watchedKv)) | ||
{ | ||
await TracingUtils.CallWithRequestTracing(_requestTracingEnabled, RequestType.Watch, _requestTracingOptions, | ||
async () => change = await client.GetKeyValueChange(watchedKv, cancellationToken).ConfigureAwait(false)).ConfigureAwait(false); | ||
async () => change = await client.GetKeyValueChange(watchedKv, makeConditionalRequest: !_options.IsCdnEnabled, cancellationToken).ConfigureAwait(false)).ConfigureAwait(false); | ||
} | ||
else | ||
{ | ||
|
@@ -1000,7 +1059,14 @@ await CallWithRequestTracing( | |
|
||
if (kvWatcher.RefreshAll) | ||
{ | ||
return true; | ||
return change.Current.ETag.ToString(); | ||
} | ||
|
||
if (_options.IsCdnEnabled) | ||
{ | ||
// | ||
// even if the change is not refresh all, we still need to reset stale version. | ||
_configVersion = null; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm a bit confused as to what this is used for. It appears to be redundant with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is |
||
} | ||
} | ||
else | ||
|
@@ -1009,7 +1075,7 @@ await CallWithRequestTracing( | |
} | ||
} | ||
|
||
return false; | ||
return null; | ||
} | ||
|
||
private void SetData(IDictionary<string, string> data) | ||
|
@@ -1065,7 +1131,8 @@ private void SetRequestTracingOptions() | |
IsKeyVaultConfigured = _options.IsKeyVaultConfigured, | ||
IsKeyVaultRefreshConfigured = _options.IsKeyVaultRefreshConfigured, | ||
FeatureFlagTracing = _options.FeatureFlagTracing, | ||
IsLoadBalancingEnabled = _options.LoadBalancingEnabled | ||
IsLoadBalancingEnabled = _options.LoadBalancingEnabled, | ||
IsCdnEnabled = _options.IsCdnEnabled | ||
}; | ||
} | ||
|
||
|
@@ -1328,33 +1395,56 @@ private void UpdateClientBackoffStatus(Uri endpoint, bool successful) | |
_configClientBackoffs[endpoint] = clientBackoffStatus; | ||
} | ||
|
||
private async Task<bool> HaveCollectionsChanged( | ||
private async Task<string> HaveCollectionsChanged( | ||
IEnumerable<KeyValueSelector> selectors, | ||
Dictionary<KeyValueSelector, IEnumerable<MatchConditions>> pageEtags, | ||
ConfigurationClient client, | ||
CancellationToken cancellationToken) | ||
{ | ||
bool haveCollectionsChanged = false; | ||
string changedEtag = null; | ||
|
||
foreach (KeyValueSelector selector in selectors) | ||
{ | ||
if (pageEtags.TryGetValue(selector, out IEnumerable<MatchConditions> matchConditions)) | ||
{ | ||
await TracingUtils.CallWithRequestTracing(_requestTracingEnabled, RequestType.Watch, _requestTracingOptions, | ||
async () => haveCollectionsChanged = await client.HaveCollectionsChanged( | ||
async () => changedEtag = await client.HaveCollectionsChanged( | ||
selector, | ||
matchConditions, | ||
_options.ConfigurationSettingPageIterator, | ||
makeConditionalRequest: !_options.IsCdnEnabled, | ||
cancellationToken).ConfigureAwait(false)).ConfigureAwait(false); | ||
} | ||
|
||
if (haveCollectionsChanged) | ||
if (changedEtag != null) | ||
{ | ||
return true; | ||
// If we have a changed ETag, we can stop checking further selectors | ||
return changedEtag; | ||
} | ||
} | ||
|
||
return haveCollectionsChanged; | ||
return changedEtag; | ||
} | ||
|
||
private static string CalculateHash(IEnumerable<string> etags) | ||
samsadsam marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
Debug.Assert(etags != null && etags.Any()); | ||
|
||
StringBuilder inputBuilder = new StringBuilder(); | ||
|
||
// | ||
// purpose | ||
inputBuilder.Append("etags\n"); | ||
|
||
foreach (string etag in etags) | ||
{ | ||
inputBuilder.Append(etag); | ||
inputBuilder.Append('\n'); | ||
samsadsam marked this conversation as resolved.
Show resolved
Hide resolved
samsadsam marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
samsadsam marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
using SHA256 sha256 = SHA256.Create(); | ||
|
||
return sha256.ComputeHash(Encoding.UTF8.GetBytes(inputBuilder.ToString())).ToBase64Url(); | ||
} | ||
|
||
private async Task ProcessKeyValueChangesAsync( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
// Copyright (c) Microsoft Corporation. | ||
// Licensed under the MIT license. | ||
// | ||
namespace Microsoft.Extensions.Configuration.AzureAppConfiguration | ||
{ | ||
/// <summary> | ||
/// Implementation of ICdnCacheBustingAccessor that manages the current token for cache busting. | ||
/// </summary> | ||
internal class CdnCacheBustingAccessor : ICdnCacheBustingAccessor | ||
{ | ||
private string _currentToken; | ||
|
||
/// <summary> | ||
/// Gets or sets the current token value to be used for cache busting. | ||
/// When null, CDN cache busting is disabled. When not null, the token will be injected into requests. | ||
/// </summary> | ||
public string CurrentToken | ||
{ | ||
get => _currentToken; | ||
set => _currentToken = value; | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should make sure
ReplicaDiscoveryEnabled
is false when CDN is used.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't use this stuff when cdn is enabled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean we should throw exception, when people set
ReplicaDiscoveryEnabled = true
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not care about this stuff when cdn is enabled. We don’t need to throw since we don’t use this variable, we completely ignore it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look at CdnConfigurationClientManager vs ConfigurationClientManager
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jimmyca15 What if load balancing is enabled after calling ConnectCdn?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would need to throw in the provider
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, need to add another check in src/Microsoft.Extensions.Configuration.AzureAppConfiguration/AzureAppConfigurationSource.cs when creating provider class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, then we dont need the check in this method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done