-
Notifications
You must be signed in to change notification settings - Fork 38
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?
Conversation
src/Microsoft.Extensions.Configuration.AzureAppConfiguration/EmptyTokenCredential.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Extensions.Configuration.AzureAppConfiguration/AzureAppConfigurationOptions.cs
Outdated
Show resolved
Hide resolved
…zureAppConfigurationOptions.cs Co-authored-by: Avani Gupta <avanigupta@users.noreply.github.com>
…/AppConfiguration-DotnetProvider into user/samisadfa/connect-cdn
src/Microsoft.Extensions.Configuration.AzureAppConfiguration/AzureAppConfigurationOptions.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Extensions.Configuration.AzureAppConfiguration/AzureAppConfigurationOptions.cs
Outdated
Show resolved
Hide resolved
Note: Correlation-Context header should contain a special tag when |
src/Microsoft.Extensions.Configuration.AzureAppConfiguration/CdnApiVersionPolicy.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Extensions.Configuration.AzureAppConfiguration/CdnConfigurationClientManager.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Extensions.Configuration.AzureAppConfiguration/AzureAppConfigurationSource.cs
Outdated
Show resolved
Hide resolved
@samsadsam, @avanigupta led a portal design review recently in which it came up that we should use Azure Front Door as API terminology instead of CDN. There were a couple of reasons.
So instead of |
Any preference between |
According to their docs, it looks like the CDN part is implied because "Azure Front Door is Microsoft's advanced cloud Content Delivery Network (CDN)" In that case I'd say we go with |
src/Microsoft.Extensions.Configuration.AzureAppConfiguration/AzureAppConfigurationOptions.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Extensions.Configuration.AzureAppConfiguration/AzureAppConfigurationOptions.cs
Outdated
Show resolved
Hide resolved
...icrosoft.Extensions.Configuration.AzureAppConfiguration/Cdn/CdnConfigurationClientManager.cs
Outdated
Show resolved
Hide resolved
...icrosoft.Extensions.Configuration.AzureAppConfiguration/Cdn/CdnConfigurationClientManager.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Extensions.Configuration.AzureAppConfiguration/Cdn/CdnPolicy.cs
Outdated
Show resolved
Hide resolved
...icrosoft.Extensions.Configuration.AzureAppConfiguration/Constants/RequestTracingConstants.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Extensions.Configuration.AzureAppConfiguration/Extensions/PageExtensions.cs
Outdated
Show resolved
Hide resolved
Looks good, the only remaining work is to rename cdn->afd everywhere. |
I don’t think we need to do this for internal classes, I think cdn is better than afd as we can support other CDN providers in the future with same internal classes. My understanding is that we only need to do this for the public apis cc @jimmyca15 |
If it's easier to understand by linking the two with afd, then I'm fine with it. |
My thinking is that if we were starting from scratch today, knowing we’re adding a new API called |
@avanigupta @jimmyca15 does it look good now? |
src/Microsoft.Extensions.Configuration.AzureAppConfiguration/AzureAppConfigurationOptions.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Extensions.Configuration.AzureAppConfiguration/AzureAppConfigurationSource.cs
Outdated
Show resolved
Hide resolved
…zureAppConfigurationSource.cs Co-authored-by: Jimmy Campbell <jimmyca@microsoft.com>
…/AppConfiguration-DotnetProvider into user/samisadfa/connect-cdn
src/Microsoft.Extensions.Configuration.AzureAppConfiguration/AzureAppConfigurationOptions.cs
Outdated
Show resolved
Hide resolved
...icrosoft.Extensions.Configuration.AzureAppConfiguration/Afd/AfdConfigurationClientManager.cs
Outdated
Show resolved
Hide resolved
} | ||
|
||
[Fact] | ||
public async Task AfdTests_ParallelAppsHaveSameAfdTokenSequence() |
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.
what is the purpose of this testcase?
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.
What we really care about is whether both apps can eventually get the latest configuration from the cdn. I understand what this testcase is doing, but I don't think it is meaningful.
Usage Patterns
Collection Monitoring Pattern (Recommended)
Sentinel Key Pattern
How do we guarantee fresh config when refresh all is triggered?
In the case of when a refresh all is triggered, we add a new token param to the query that essentially triggers CDN to fetch from origin (Azure App Configuration) so that we get the latest configuration. This token is the first change etag we detected, the one that triggered a "refresh all" operation.
It is either a page new etag (Collection Monitoring Pattern) or sentinel key new etag (Sentinel Key Pattern). We treat feature flags the same as collection monitoring.
We keep using this token until a new change happens and a new etag is detected.