-
Notifications
You must be signed in to change notification settings - Fork 21
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
CUSTESC-36595: Try to prioritize active zone over non-active ones #200
Conversation
Following our previous release of the APO plugin, we essentially bumped the `cloudflare-plugin-frontend` version from `v3.6.0` to `v3.8.0`, which includes the following changes: cloudflare/cloudflare-plugin-frontend@v3.6.0...v3.8.0. However, the APO plugin seem to only load its dashboard partially, at least on our testing Wordpress instance, due to the following error: ``` compiled.js?ver=4.12.5:18 action ZONE_FETCH_SETTINGS_SUCCESS @ 12:10:45.851 compiled.js?ver=4.12.5:21 Uncaught TypeError: Cannot read properties of undefined (reading 'zone.automatic_platform_optimization') at kl.render (compiled.js?ver=4.12.5:21:512376) at u._renderValidatedComponentWithoutOwnerOrContext (compiled.js?ver=4.12.5:18:163911) at u._renderValidatedComponent (compiled.js?ver=4.12.5:18:164018) at u.performInitialMount (compiled.js?ver=4.12.5:18:159921) at u.mountComponent (compiled.js?ver=4.12.5:18:158971) at Object.mountComponent (compiled.js?ver=4.12.5:5:40635) at u.performInitialMount (compiled.js?ver=4.12.5:18:160082) at u.mountComponent (compiled.js?ver=4.12.5:18:158971) at Object.mountComponent (compiled.js?ver=4.12.5:5:40635) at u.performInitialMount (compiled.js?ver=4.12.5:18:160082) ``` After reading the change we made removing the Railgun plugin, it seems that the following change of the `src/reducers/zoneEntitlements.js` file was indesirable, as it changes the logic related to the handling of the entitlements: cloudflare@fff5c43#diff-1d811dd2703dec5a6ed09349810d7cfecd5b236c330d4802c2ab9cf6250c4603 While the error is about the `ZONE_UPDATE_SETTING_SUCCESS` action, and not about the entitlements, the code change seems to have removed some kind of merge operation, which is the most suspicious change which could cause that error.
3feaa3d
to
be0a162
Compare
**Problem** As part of an internal issue named CUSTESC-36595, one customer reported us that his Cloudflare Wordpress plugin was reporting that his dashboard was failing with the following error: ```json "errors.noActiveZoneSelected": "It looks like your domain {domain} is not provisioned with Cloudflare. Please continue to {link} to secure and speed up your website.", ``` The problem was that the customer does have two Cloudflare zones with the same name: one being active and the other got purged. **Previous investigations** We initially tried to fix this in the `Cloudflare-Wordpress` plugin itself, via cloudflare/Cloudflare-WordPress#532, by ensuring that the zone listing endpoint `/zones` was requested with proper `status=active` query parameter to only retrieve active zone, if any. However, the problem is not fixed on customers' side. **Current investigation** After looking once again at their HAR files, besides the endpoint itself, we noticed that the initiator of the call was actually coming from the compiled and minifed JavaScript file from this `cloudflare-plugin-frontend` project. This plugin also requests this zone listing endpoint `/zones` but it also seems to use the zone list, so I did not want to change the logic too much and retrieve only the active zones. Instead, I found out the two places where that list of zones was used and from which a single zone was being extracted. **Fixing `src/actions/zoneProvision.js`** For `src/actions/zoneProvision.js`, it seems we pass the list of zones from the Cloudflare API to a dependency called `normalizr` which normalize based on the name field here https://github.com/cloudflare/cloudflare-plugin-frontend/blob/master/src/constants/Schemas.js#L13-L15, which effectively removes duplicates, which was tested locally with: ```js import { normalize, Schema, arrayOf } from 'normalizr'; const zones = [ {'id': 1, 'name': 'cloudflare.com', 'status': 'purged'}, {'id': 2, 'name': 'cloudflare.com', 'status': 'active'}, ]; const zoneSchema = new Schema('zones', { idAttribute: 'name' }); const normalizedZones = normalize(zones, arrayOf(zoneSchema)); console.log(JSON.stringify(normalizedZones, '', null)); ``` which outputs the following: ``` When merging two zones, found unequal data in their "id" values. Using the earlier value. 1 2 When merging two zones, found unequal data in their "status" values. Using the earlier value. purged active {"entities":{"zones":{"cloudflare.com":{"id":1,"name":"cloudflare.com","status":"purged"}}},"result":["cloudflare.com","cloudflare.com"]} ``` This could indeed correlates with our problem considered that the purged zone of the customer was indeed created before the active zone. Should they discriminate based on their `id` field, the active zone would indeed be discarded. To prevent that issue, we are introducing a `deduplicateOnActiveZones()` helper which deduplicates the list of zones whenever at least one active zone is present. By doing so, the invocation to `normalizr` should now only retain the active zone, and the following `asyncZoneSetActiveZone` Redux dispatch receive that zone. **Fixing `src/actions/zones.js`** For `src/actions/zones.js`, it seems that only the first zone from the zone list is being used, if defined. For that one, we simply added an extra logic to attempt at extracting the first active zone first, if any, and fallback to the original logic if no active zone is found.
be0a162
to
f9ddd7e
Compare
cc @jacobbednarz if you could please review! |
export function deduplicateOnActiveZones(zones) { | ||
let filteredZones = []; | ||
const isActive = z => z.status === 'active'; | ||
const isSameZone = (z1, z2) => z1.name === z2.name; |
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.
should we do a falsy check here too? z1.name && z1.name === z2.name
?
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.
The name
field is the raw result coming from the Cloudflare API, so I assume it's properly populated to be honest. And nonetheless undefined === 'something'
is falsy.
@jacobbednarz I don't have the rights to merge that PR. Could you merge and release it please, so that I can give it a try in the Wordpress plugin. |
This commit includes the output of the `yarn build:production` of version 3.10.0 of the `cloudflare-plugin-frontend` plugin to include the potential fix introduced by cloudflare/cloudflare-plugin-frontend#200.
**🔖 Summary** This release includes cloudflare#542 which corresponds to the output of the `yarn build:production` of version `3.10.0` of the `cloudflare-plugin-frontend` plugin to include the potential fix introduced by cloudflare/cloudflare-plugin-frontend#200.
Problem
As part of an internal issue named CUSTESC-36595, one customer reported
us that his Cloudflare Wordpress plugin was reporting that his dashboard
was failing with the following error:
The problem was that the customer does have two Cloudflare zones with
the same name: one being active and the other got purged.
Previous investigations
We initially tried to fix this in the
Cloudflare-Wordpress
pluginitself, via cloudflare/Cloudflare-WordPress#532,
by ensuring that the zone listing endpoint
/zones
was requested withproper
status=active
query parameter to only retrieve active zone, ifany. However, the problem is not fixed on customers' side.
Current investigation
After looking once again at their HAR files, besides the endpoint
itself, we noticed that the initiator of the call was actually coming
from the compiled and minifed JavaScript file from this
cloudflare-plugin-frontend
project.This plugin also requests this zone listing endpoint
/zones
but italso seems to use the zone list, so I did not want to change the logic
too much and retrieve only the active zones. Instead, I found out the
two places where that list of zones was used and from which a single
zone was being extracted.
Fixing
src/actions/zoneProvision.js
For
src/actions/zoneProvision.js
, it seems we pass the list of zonesfrom the Cloudflare API to a dependency called
normalizr
whichnormalize based on the name field here
https://github.com/cloudflare/cloudflare-plugin-frontend/blob/master/src/constants/Schemas.js#L13-L15,
which effectively removes duplicates, which was tested locally with:
which outputs the following:
This could indeed correlates with our problem considered that the purged
zone of the customer was indeed created before the active zone. Should
they discriminate based on their
id
field, the active zone wouldindeed be discarded.
To prevent that issue, we are introducing a
deduplicateOnActiveZones()
helper which deduplicates the list of zones whenever at least one active
zone is present. By doing so, the invocation to
normalizr
should nowonly retain the active zone, and the following
asyncZoneSetActiveZone
Redux dispatch receive that zone.
Fixing
src/actions/zones.js
For
src/actions/zones.js
, it seems that only the first zone from thezone list is being used, if defined. For that one, we simply added an
extra logic to attempt at extracting the first active zone first, if
any, and fallback to the original logic if no active zone is found.