-
Notifications
You must be signed in to change notification settings - Fork 40
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
Changes from 46 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
4a40a22
bdbd4ef
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.
Outdated
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; | ||
|
||
| 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,12 @@ await ExecuteWithFailOverPolicyAsync(clients, async (client) => | |
| // Get key value collection changes if RegisterAll was called | ||
| if (isRefreshDue) | ||
| { | ||
| refreshAll = await HaveCollectionsChanged( | ||
| if (_options.IsCdnEnabled) | ||
| { | ||
| _options.CdnTokenAccessor.Current = _configVersion; | ||
| } | ||
|
|
||
| refreshAllChangedEtag = await GetCollectionsChangeEtag( | ||
| _options.Selectors.Where(selector => !selector.IsFeatureFlagSelector), | ||
| _kvEtags, | ||
| client, | ||
|
|
@@ -314,7 +325,12 @@ await ExecuteWithFailOverPolicyAsync(clients, async (client) => | |
| } | ||
| else | ||
| { | ||
| refreshAll = await RefreshIndividualKvWatchers( | ||
| if (_options.IsCdnEnabled) | ||
| { | ||
|
Member
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
Contributor
Author
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 |
||
| _options.CdnTokenAccessor.Current = _configVersion; | ||
| } | ||
|
|
||
| refreshAllChangedEtag = await RefreshIndividualKvWatchers( | ||
jimmyca15 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| client, | ||
| keyValueChanges, | ||
| refreshableIndividualKvWatchers, | ||
|
|
@@ -324,22 +340,39 @@ 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) | ||
| { | ||
| // | ||
| // Break cdn cache | ||
| _options.CdnTokenAccessor.Current = refreshAllChangedEtag; | ||
|
|
||
| // | ||
| // Reset versions so that next watch request will not use stale versions. | ||
| _configVersion = refreshAllChangedEtag; | ||
| _ffCollectionVersion = refreshAllChangedEtag; | ||
| } | ||
|
|
||
| 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) | ||
| { | ||
| _options.CdnTokenAccessor.Current = _ffCollectionVersion; | ||
| } | ||
|
|
||
| ffCollectionUpdatedChangedEtag = await GetCollectionsChangeEtag( | ||
| refreshableFfWatchers.Select(watcher => new KeyValueSelector | ||
| { | ||
| KeyFilter = watcher.Key, | ||
|
|
@@ -350,11 +383,22 @@ 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) | ||
| { | ||
| // | ||
| // Break cdn cache | ||
| _options.CdnTokenAccessor.Current = ffCollectionUpdatedChangedEtag; | ||
|
|
||
| // | ||
| // Reset ff collection version so that next ff watch request will not use stale version. | ||
| _ffCollectionVersion = ffCollectionUpdatedChangedEtag; | ||
| } | ||
|
|
||
| ffCollectionData = await LoadSelected( | ||
| client, | ||
| new Dictionary<KeyValueSelector, IEnumerable<MatchConditions>>(), | ||
|
|
@@ -373,6 +417,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 +987,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 +1010,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 +1047,19 @@ await CallWithRequestTracing( | |
|
|
||
| if (kvWatcher.RefreshAll) | ||
| { | ||
| return true; | ||
| string changedEtag; | ||
|
|
||
| if (change.ChangeType == KeyValueChangeType.Deleted) | ||
| { | ||
| using SHA256 sha256 = SHA256.Create(); | ||
| changedEtag = sha256.ComputeHash(Encoding.UTF8.GetBytes($"ResourceDeleted\n{change.Previous.ETag}")).ToBase64Url(); | ||
| } | ||
| else | ||
| { | ||
| changedEtag = change.Current.ETag.ToString(); | ||
| } | ||
|
|
||
| return changedEtag; | ||
| } | ||
| } | ||
| else | ||
|
|
@@ -1009,7 +1068,7 @@ await CallWithRequestTracing( | |
| } | ||
| } | ||
|
|
||
| return false; | ||
| return null; | ||
| } | ||
|
|
||
| private void SetData(IDictionary<string, string> data) | ||
|
|
@@ -1065,7 +1124,8 @@ private void SetRequestTracingOptions() | |
| IsKeyVaultConfigured = _options.IsKeyVaultConfigured, | ||
| IsKeyVaultRefreshConfigured = _options.IsKeyVaultRefreshConfigured, | ||
| FeatureFlagTracing = _options.FeatureFlagTracing, | ||
| IsLoadBalancingEnabled = _options.LoadBalancingEnabled | ||
| IsLoadBalancingEnabled = _options.LoadBalancingEnabled, | ||
| IsCdnEnabled = _options.IsCdnEnabled | ||
| }; | ||
| } | ||
|
|
||
|
|
@@ -1328,33 +1388,35 @@ private void UpdateClientBackoffStatus(Uri endpoint, bool successful) | |
| _configClientBackoffs[endpoint] = clientBackoffStatus; | ||
| } | ||
|
|
||
| private async Task<bool> HaveCollectionsChanged( | ||
| private async Task<string> GetCollectionsChangeEtag( | ||
| 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.GetCollectionChangeEtag( | ||
| 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 async Task ProcessKeyValueChangesAsync( | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
| @@ -1,8 +1,10 @@ | ||||
| // Copyright (c) Microsoft Corporation. | ||||
| // Licensed under the MIT license. | ||||
| // | ||||
| using Azure.Core; | ||||
| using Azure.Data.AppConfiguration; | ||||
| using Microsoft.Extensions.Azure; | ||||
| using Microsoft.Extensions.Configuration.AzureAppConfiguration.Cdn; | ||||
| using System; | ||||
| using System.Collections.Generic; | ||||
| using System.Linq; | ||||
|
|
@@ -56,10 +58,25 @@ public IConfigurationProvider Build(IConfigurationBuilder builder) | |||
| } | ||||
| else | ||||
| { | ||||
| throw new ArgumentException($"Please call {nameof(AzureAppConfigurationOptions)}.{nameof(AzureAppConfigurationOptions.Connect)} to specify how to connect to Azure App Configuration."); | ||||
| throw new ArgumentException($"Please call {nameof(AzureAppConfigurationOptions)}.{nameof(AzureAppConfigurationOptions.Connect)} or {nameof(AzureAppConfigurationOptions)}.{nameof(AzureAppConfigurationOptions.ConnectCdn)} to specify how to connect to Azure App Configuration."); | ||||
| } | ||||
|
|
||||
| provider = new AzureAppConfigurationProvider(new ConfigurationClientManager(clientFactory, endpoints, options.ReplicaDiscoveryEnabled, options.LoadBalancingEnabled), options, _optional); | ||||
| if (options.IsCdnEnabled) | ||||
| { | ||||
| if (options.LoadBalancingEnabled) | ||||
| { | ||||
| throw new InvalidOperationException("Load balancing is not supported for CDN endpoint."); | ||||
| } | ||||
|
|
||||
| options.CdnTokenAccessor = new CdnTokenAccessor(); | ||||
| options.ClientOptions.AddPolicy(new CdnPolicy(options.CdnTokenAccessor), HttpPipelinePosition.PerCall); | ||||
|
|
||||
| provider = new AzureAppConfigurationProvider(new CdnConfigurationClientManager(clientFactory, endpoints.First()), options, _optional); | ||||
|
||||
| clientFactory ??= new AzureAppConfigurationClientFactory(options.Credential, options.ClientOptions); |
We do need a client factory, but I think we should always create our own instance of AzureAppConfigurationClientFactory instead of using the one passed in by users through SetClientFactory 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.
Can a user shoot themselves in the foot easily if they have their own custom client factory and are using CDN? I suppose the question is, what care does a user need to take if they were to use client factory and also want to use cdn?
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.
Its the cache breaking policy that would be missing from custom clients. If someone wants to use client factory with cdn, their refresh requests will not contain etag in the query params.
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.
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 still think that we should retain full control over the client used for connecting to CDN. We are already using a very customized AppConfig client to connect to FrontDoor service - which in itself is an unconventional practice. Abstracting the client logic helps prevent accidental misuse. To me, the current solutions feel off, and without a clear user scenario, it's hard to design this properly. We can always add support for a custom client factory with CDN later if customers need 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.
It's a good point. And thinking about it, one of the reasons to have the client factory method was to be able to use that same client that is loading configuration in startup to later read/write to App Configuration outside of the provider. However, with CDN, there is no write capability, and for any read calls outside of our provider there shouldn't be any desire to use our cache break mechanism that is relevant to the provider. So it seems the client factory scenario doesn't actually play well with cdn consumption.
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.
+1 to what @avanigupta said.
We're using a client designed for one service (App Configuration) to talk to another (Azure Front Door CDN), which is already a bit of a workaround/hack. I'm unsure how well we can support a custom App Configuration client factory in this context, given the unconventional setup.
We also have the ConfigureClientOptions API, but it's unclear to what extent Azure Front Door will respect those settings. Our stance may end up being: "If it works, great; if not, there's not much we can do." Therefore, I'm hesitant to support using custom clients with CDN.
On the other hand, I don't think the custom App Configuration client factory has too many users (we probably should collect telemetry), so I don't think we miss too much anyway even if we don't support them for CDN.
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
Uh oh!
There was an error while loading. Please reload this page.