-
Notifications
You must be signed in to change notification settings - Fork 3
Support load configuration settings from Azure Front Door #223
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
…avaScriptProvider into zhiyuanliang/afd-support
…avaScriptProvider into zhiyuanliang/afd-support
…avaScriptProvider into zhiyuanliang/afd-support
…avaScriptProvider into zhiyuanliang/afd-support
…avaScriptProvider into zhiyuanliang/afd-support
f081ea1 to
4480562
Compare
src/common/errorMessages.ts
Outdated
| INVALID_LABEL_FILTER = "The characters '*' and ',' are not supported in label filters.", | ||
| INVALID_TAG_FILTER = "Tag filter must follow the format 'tagName=tagValue'", | ||
| CONNECTION_STRING_OR_ENDPOINT_MISSED = "A connection string or an endpoint with credential must be specified to create a client.", | ||
| REPLICA_DISCOVERY_NOT_SUPPORTED = "Replica discovery is not supported when loading from Azure Front Door.", |
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.
| REPLICA_DISCOVERY_NOT_SUPPORTED = "Replica discovery is not supported when loading from Azure Front Door.", | |
| REPLICA_DISCOVERY_NOT_SUPPORTED = "Replica discovery is not supported when loading from Azure Front Door. To leverage AppConfig replicas, add multiple replicas to your Front Door origin group. Lear more about [Origins and origin groups in Azure Front Door](https://learn.microsoft.com/en-us/azure/frontdoor/origin?pivots=front-door-standard-premium).", |
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.
This error message is too long. And I am not a fan of putting the doc link here.
Instead of pointing to AFD doc, I think the link should be pointed to one doc maintained by ourselves.
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.
This is a concept owned by AFD, we dont have any doc on this topic. When we choose not to support it ourselves, it’s important to provide users with any available alternatives. I agree its long, but its necessary in this case.
We can reduce the text to:
"Replica discovery isn’t supported via Azure Front Door. To use AppConfig replicas, add multiple replicas to your Front Door origin group. Learn more: https://learn.microsoft.com/en-us/azure/frontdoor/origin"
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.
cc @jimmyca15 If you agree, we should update the error message in .net provider too.
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.
This is a concept owned by AFD, we dont have any doc on this topic. When we choose not to support it ourselves, it’s important to provide users with any available alternatives. I agree its long, but its necessary in this case. We can reduce the text to:
"Replica discovery isn’t supported via Azure Front Door. To use AppConfig replicas, add multiple replicas to your Front Door origin group. Learn more: https://learn.microsoft.com/en-us/azure/frontdoor/origin"
I mean we can a doc that teach people how to use AFD with app config or a section of FAQ. Our doc can reference this AFD doc. We should have the full control of the doc.
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 don't mind putting a link, but it should be a link to our docs. Perhaps our geo-replication doc, and we add a blurb about when consuming through AFD, origin groups should be set up.
Something like "For guidance on how to take advantage of geo-replication when Azure Front Door is used, visit https://aka.ms/appconfig/geo-replication-with-afd"
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.
Sounds good. The only downside of using our doc is that provider will be released before we can make any doc updates. But it's not that bad since we will eventually update our docs after ignite.
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.
Doc PR: https://github.com/MicrosoftDocs/azure-docs-pr/pull/308036 @jimmyca15 @avanigupta Could you review and approve it?
I will take Jimmy's suggestion.
Change the error message to:
"Replica discovery is not supported when loading from Azure Front Door. For guidance on how to take advantage of geo-replication when Azure Front Door is used, visit https://learn.microsoft.com/azure/azure-app-configuration/howto-geo-replication#use-geo-replica-with-azure-front-door"
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.
Cool. In the error message, you can use the short link that Jimmy created.
https://aka.ms/appconfig/geo-replication-with-afd
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.
Wow. Did we have some redirect mechanism? How do we add path to aka.ms/appconfig?
src/common/errorMessages.ts
Outdated
| INVALID_TAG_FILTER = "Tag filter must follow the format 'tagName=tagValue'", | ||
| CONNECTION_STRING_OR_ENDPOINT_MISSED = "A connection string or an endpoint with credential must be specified to create a client.", | ||
| REPLICA_DISCOVERY_NOT_SUPPORTED = "Replica discovery is not supported when loading from Azure Front Door.", | ||
| LOAD_BALANCING_NOT_SUPPORTED = "Load balancing is not supported when loading from Azure Front Door.", |
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.
| LOAD_BALANCING_NOT_SUPPORTED = "Load balancing is not supported when loading from Azure Front Door.", | |
| LOAD_BALANCING_NOT_SUPPORTED = "Load balancing is not supported when loading from Azure Front Door. To load balance between AppConfig replicas, add multiple replicas to your Front Door origin group. Lear more about [Origins and origin groups in Azure Front Door](https://learn.microsoft.com/en-us/azure/frontdoor/origin?pivots=front-door-standard-premium).", |
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.
Same here, I can make my suggestion shorter:
"Load balancing isn’t supported via Azure Front Door. To load balance across AppConfig replicas, add multiple replicas to your Front Door origin group. Learn more: https://learn.microsoft.com/en-us/azure/frontdoor/origin"
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.
Change to
"Load balancing is not supported when loading from Azure Front Door. For guidance on how to take advantage of geo-replication when Azure Front Door is used, visit https://learn.microsoft.com/azure/azure-app-configuration/howto-geo-replication#use-geo-replica-with-azure-front-door"
0d167c6 to
564f16a
Compare
…avaScriptProvider into zhiyuanliang/afd-support
…avaScriptProvider into zhiyuanliang/afd-support
src/appConfigurationImpl.ts
Outdated
| } | ||
|
|
||
| const lastServerResponseTime = pageWatchers[i].lastServerResponseTime; | ||
| let isUpToDate = 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.
| let isUpToDate = true; | |
| let isChanged = false; |
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.
How about isReponseFresh? isChanged is not appropriate, this variable's value is serverResponseTime > lastServerResponseTime.
…avaScriptProvider into zhiyuanliang/afd-support
Why this PR?
Introduce a new API:
loadFromAzureFrontDoorwhich takes the Azure Front Door endpoint as parameter. The configuration provider will load configuration settings from the Azure Front Door.Behavior change
When
loadFromAzureFrontDooris used:authorizationheader will be removed from the requestsync-tokenwill not be attached to the request.Refresh logic
Use "
x-ms-dateresponse header from AppConfig - this is the time when server generated the responseThe provider will now track the page etags and the
x-ms-dateof each page response.A refresh is triggered only when the refresh engine detects that the page’s ETag has changed and the accompanying x-ms-date is later than the previous server response time. Once a refresh occurs, the system simply reloads whatever content is currently available from the CDN. This means it may temporarily load outdated pages, but because the provider serving the outdated content will still show a detectable change during subsequent refresh checks, the system will eventually update all pages. The underlying philosophy is that a refresh should, at minimum, achieve what an application restart would. Therefore, our refresh mechanism effectively reloads all configuration settings.