-
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
| 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.
| 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"
0d167c6 to
564f16a
Compare
| if (requestTracingOptions.isFailoverRequest) { | ||
| tags.push(FAILOVER_REQUEST_TAG); | ||
| } | ||
| if (requestTracingOptions.isAfdUsed) { |
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.
Reminder to update this to match .NET provider (add afd tag to "Features")
| } else { | ||
| header = response._response?.headers?.get(SERVER_TIMESTAMP_HEADER) ?? undefined; | ||
| } | ||
| return header ? new Date(header) : new Date(); |
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.
If the header is not a valid date, we should use the default 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.
I mean we need to handle the case if new Date(header) throws an error
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.
new Date(header) will not throw. It will become an invalid date. But I should handle the invalid case. Updated
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.
If the header is not a valid date, we should use the default value
What should be the default value, undefined or new Date() (current time)?
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.
Decide to use new Date() current time to make it consistent as .NET provider
| } | ||
|
|
||
| const lastServerResponseTime = pageWatchers[i].lastServerResponseTime; | ||
| const isUpToDate = lastServerResponseTime ? timestamp > lastServerResponseTime : 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.
lastServerResponseTime will never be null or undefined right?
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.
True. If the header is not found, we set it as the current time. Now I am leaning to if the header is not fond, we set the pageWatcher.lastServerResponstTime as undefined.
| if (isUpToDate && page._response.status === 200 && page.etag !== pageWatchers[i].etag) { | ||
| return 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.
This can be simplified if lastServerResponseTime is always defined
if (serverResponseTime >= lastServerResponseTime &&
page._response.status === 200 &&
page.etag !== pageWatchers[i].etag)
{
return true;
}
…avaScriptProvider into zhiyuanliang/afd-support
…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.