Skip to content

Commit

Permalink
CUSTESC-36595: Try to prioritize active zone over non-active ones
Browse files Browse the repository at this point in the history
**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.
  • Loading branch information
aseure committed Mar 22, 2024
1 parent a5a3722 commit f9ddd7e
Show file tree
Hide file tree
Showing 4 changed files with 91 additions and 2 deletions.
4 changes: 3 additions & 1 deletion src/actions/zoneProvision.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import * as ActionTypes from '../constants/ActionTypes';
import { asyncZoneSetActiveZone } from './activeZone';
import { normalizeZoneGetAll } from '../constants/Schemas';
import { zoneFetch, zoneFetchSuccess } from './zones';
import { deduplicateOnActiveZones } from '../utils/utils';

/*
* Zone Provision actions still use reducers/zones.js as the reducer
Expand Down Expand Up @@ -136,7 +137,8 @@ function asyncSetHostAPIProvisionedDomainActive(domainName) {
zoneGetAll(function(error, response) {
if (response) {
dispatch(zoneFetchSuccess(response.body.result));
let normalizedZoneList = normalizeZoneGetAll(response.body.result);
const zoneList = deduplicateOnActiveZones(response.body.result);
let normalizedZoneList = normalizeZoneGetAll(zoneList);
dispatch(
asyncZoneSetActiveZone(normalizedZoneList.entities.zones[domainName])
);
Expand Down
7 changes: 6 additions & 1 deletion src/actions/zones.js
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,12 @@ export function asyncFetchZones() {
zoneGetAll(function(error, response) {
if (response) {
dispatch(zoneFetchSuccess(response.body.result));
if (response.body.result[0]) {
const activeZone = response.body.result.find(
zone => zone.status === 'active'
);
if (activeZone) {
dispatch(zoneSetActiveZoneIfEmpty(activeZone));
} else if (response.body.result[0]) {
dispatch(zoneSetActiveZoneIfEmpty(response.body.result[0]));
}
} else {
Expand Down
54 changes: 54 additions & 0 deletions src/test/utils/deduplicateZonesTest.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
import { deduplicateOnActiveZones } from '../../utils/utils';
import expect from 'expect';

describe('deduplicateZones', () => {
it('should not change a list of zones without any duplicate', () => {
expect(
deduplicateOnActiveZones([
{ id: '1', name: 'cloudflare.com', status: 'active' },
{ id: '2', name: 'blog.cloudflare.com', status: 'active' }
])
).toEqual([
{ id: '1', name: 'cloudflare.com', status: 'active' },
{ id: '2', name: 'blog.cloudflare.com', status: 'active' }
]);
});

it('should remove non-active zone in case of duplicates', () => {
expect(
deduplicateOnActiveZones([
{ id: '1', name: 'cloudflare.com', status: 'active' },
{ id: '2', name: 'blog.cloudflare.com', status: 'active' },
{ id: '3', name: 'cloudflare.com', status: 'purged' }
])
).toEqual([
{ id: '1', name: 'cloudflare.com', status: 'active' },
{ id: '2', name: 'blog.cloudflare.com', status: 'active' }
]);
});

it('should not remove duplicates when there is no active zone', () => {
expect(
deduplicateOnActiveZones([
{ id: '1', name: 'cloudflare.com', status: 'pending' },
{ id: '2', name: 'blog.cloudflare.com', status: 'active' },
{ id: '3', name: 'cloudflare.com', status: 'purged' }
])
).toEqual([
{ id: '1', name: 'cloudflare.com', status: 'pending' },
{ id: '2', name: 'blog.cloudflare.com', status: 'active' },
{ id: '3', name: 'cloudflare.com', status: 'purged' }
]);
});

it('should remove multiple duplicates when there is one active zone', () => {
expect(
deduplicateOnActiveZones([
{ id: '1', name: 'cloudflare.com', status: 'pending' },
{ id: '2', name: 'cloudflare.com', status: 'purged' },
{ id: '3', name: 'cloudflare.com', status: 'active' },
{ id: '4', name: 'cloudflare.com', status: 'moved' }
])
).toEqual([{ id: '3', name: 'cloudflare.com', status: 'active' }]);
});
});
28 changes: 28 additions & 0 deletions src/utils/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -70,3 +70,31 @@ export function formatMessageForIntegration(

return formatMessage({ id: messageId });
}

// deduplicateOnActiveZones was introduced as a potential fix for CUSTESC-36595. The goal of this function is to
// deduplicate the list of zones returned by the Cloudflare API based on the status of the zone. This way, we hope to
// guarantee that when normalizing the list of zones, only the active zone would show up in case of duplicates. For
// instance, in the mentioned tickets, two zones were potentially returned: a purged as well as an active zone.
export function deduplicateOnActiveZones(zones) {
let filteredZones = [];
const isActive = z => z.status === 'active';
const isSameZone = (z1, z2) => z1.name === z2.name;

zones.forEach(zone => {
if (isActive(zone)) {
// If the zone is active, we first remove all existing duplicates and then insert the active zone.
filteredZones = filteredZones.filter(fZone => !isSameZone(zone, fZone));
filteredZones.push(zone);
} else {
// If the zone is not active, we only insert it if there is no existing active zone.
const alreadyHasActiveZone = filteredZones.some(
fZone => isSameZone(zone, fZone) && isActive(fZone)
);
if (!alreadyHasActiveZone) {
filteredZones.push(zone);
}
}
});

return filteredZones;
}

0 comments on commit f9ddd7e

Please sign in to comment.