diff --git a/x-pack/plugins/remote_clusters/__jest__/client_integration/remote_clusters_list.test.js b/x-pack/plugins/remote_clusters/__jest__/client_integration/remote_clusters_list.test.js index d0d870d2a58e2..5dcf584436943 100644 --- a/x-pack/plugins/remote_clusters/__jest__/client_integration/remote_clusters_list.test.js +++ b/x-pack/plugins/remote_clusters/__jest__/client_integration/remote_clusters_list.test.js @@ -204,7 +204,10 @@ describe('', () => { describe('confirmation modal (delete remote cluster)', () => { test('should remove the remote cluster from the table after delete is successful', async () => { // Mock HTTP DELETE request - setDeleteRemoteClusterResponse(); + setDeleteRemoteClusterResponse({ + itemsDeleted: [remoteCluster1.name], + errors: [], + }); // Make sure that we have our 2 remote clusters in the table expect(rows.length).toBe(2); diff --git a/x-pack/plugins/remote_clusters/__jest__/client_integration/test_helpers.js b/x-pack/plugins/remote_clusters/__jest__/client_integration/test_helpers.js index 353eef4646dee..52652d31d5c04 100644 --- a/x-pack/plugins/remote_clusters/__jest__/client_integration/test_helpers.js +++ b/x-pack/plugins/remote_clusters/__jest__/client_integration/test_helpers.js @@ -112,7 +112,10 @@ export const registerHttpRequestMockHelpers = server => { }; const setDeleteRemoteClusterResponse = (response) => { - const defaultResponse = { success: true }; + const defaultResponse = { + itemsDeleted: [], + errors: [], + }; server.respondWith('DELETE', /api\/remote_clusters/, mockResponse(defaultResponse, response) diff --git a/x-pack/plugins/remote_clusters/public/store/actions/remove_clusters.js b/x-pack/plugins/remote_clusters/public/store/actions/remove_clusters.js index 4c17b2e2e2492..d72b6a5d1264a 100644 --- a/x-pack/plugins/remote_clusters/public/store/actions/remove_clusters.js +++ b/x-pack/plugins/remote_clusters/public/store/actions/remove_clusters.js @@ -19,49 +19,75 @@ import { import { closeDetailPanel } from './detail_panel'; import { getDetailPanelClusterName } from '../selectors'; +function getErrorTitle(count, name = null) { + if (count === 1) { + if (name) { + return i18n.translate('xpack.remoteClusters.removeAction.errorSingleNotificationTitle', { + defaultMessage: `Error removing remote cluster '{name}'`, + values: { name }, + }); + } + } else { + return i18n.translate('xpack.remoteClusters.removeAction.errorMultipleNotificationTitle', { + defaultMessage: `Error removing '{count}' remote clusters`, + values: { count }, + }); + } +} + export const removeClusters = (names) => async (dispatch, getState) => { dispatch({ type: REMOVE_CLUSTERS_START, }); - const removalSuccesses = []; - const removalErrors = []; - const removeClusterRequests = names.map(name => { - sendRemoveClusterRequest(name) - .then(() => removalSuccesses.push(name)) - .catch(() => removalErrors.push(name)); - }); + let itemsDeleted = []; + let errors = []; await Promise.all([ - ...removeClusterRequests, - // Wait at least half a second to avoid a weird flicker of the saving feedback. + sendRemoveClusterRequest(names.join(',')) + .then((response) => { + ({ itemsDeleted, errors } = response.data); + }), + // Wait at least half a second to avoid a weird flicker of the saving feedback (only visible + // when requests resolve very quickly). new Promise(resolve => setTimeout(resolve, 500)), - ]); + ]).catch(error => { + const errorTitle = getErrorTitle(names.length, names[0]); + toastNotifications.addDanger({ + title: errorTitle, + text: error.data.message, + }); + }); - if(removalErrors.length > 0) { - if (removalErrors.length === 1) { - toastNotifications.addDanger(i18n.translate('xpack.remoteClusters.removeAction.errorSingleNotificationTitle', { - defaultMessage: `Error removing remote cluster '{name}'`, - values: { name: removalErrors[0] }, - })); - } else { - toastNotifications.addDanger(i18n.translate('xpack.remoteClusters.removeAction.errorMultipleNotificationTitle', { - defaultMessage: `Error removing '{count}' remote clusters`, - values: { count: removalErrors.length }, - })); - } + if (errors.length > 0) { + const { + name, + error: { + output: { + payload: { + message, + }, + }, + }, + } = errors[0]; + + const title = getErrorTitle(errors.length, name); + toastNotifications.addDanger({ + title, + text: message, + }); } - if(removalSuccesses.length > 0) { - if (removalSuccesses.length === 1) { + if (itemsDeleted.length > 0) { + if (itemsDeleted.length === 1) { toastNotifications.addSuccess(i18n.translate('xpack.remoteClusters.removeAction.successSingleNotificationTitle', { defaultMessage: `Remote cluster '{name}' was removed`, - values: { name: removalSuccesses[0] }, + values: { name: itemsDeleted[0] }, })); } else { toastNotifications.addSuccess(i18n.translate('xpack.remoteClusters.removeAction.successMultipleNotificationTitle', { defaultMessage: '{count} remote clusters were removed', - values: { count: names.length }, + values: { count: itemsDeleted.length }, })); } } @@ -76,6 +102,6 @@ export const removeClusters = (names) => async (dispatch, getState) => { type: REMOVE_CLUSTERS_FINISH, // Send the cluster that have been removed to the reducers // and update the store immediately without the need to re-fetch from the server - payload: removalSuccesses, + payload: itemsDeleted, }); }; diff --git a/x-pack/plugins/remote_clusters/server/routes/api/remote_clusters/register_delete_route.js b/x-pack/plugins/remote_clusters/server/routes/api/remote_clusters/register_delete_route.js index 586925d26a74d..0071324cd6ed3 100644 --- a/x-pack/plugins/remote_clusters/server/routes/api/remote_clusters/register_delete_route.js +++ b/x-pack/plugins/remote_clusters/server/routes/api/remote_clusters/register_delete_route.js @@ -18,45 +18,83 @@ export function registerDeleteRoute(server) { const licensePreRouting = licensePreRoutingFactory(server); server.route({ - path: '/api/remote_clusters/{name}', + path: '/api/remote_clusters/{nameOrNames}', method: 'DELETE', + config: { + pre: [ licensePreRouting ] + }, handler: async (request) => { const callWithRequest = callWithRequestFactory(server, request); - const { name } = request.params; + const { nameOrNames } = request.params; + const names = nameOrNames.split(','); + + const itemsDeleted = []; + const errors = []; - // Check if cluster does exist - try { - const existingCluster = await doesClusterExist(callWithRequest, name); - if(!existingCluster) { - return wrapCustomError(new Error('There is no remote cluster with that name.'), 404); + // Validator that returns an error if the remote cluster does not exist. + const validateClusterDoesExist = async (name) => { + try { + const existingCluster = await doesClusterExist(callWithRequest, name); + if (!existingCluster) { + return wrapCustomError(new Error('There is no remote cluster with that name.'), 404); + } + } catch (error) { + return wrapCustomError(error, 400); } - } catch (err) { - return wrapCustomError(err, 400); - } - - try { - const deleteClusterPayload = serializeCluster({ name }); - const response = await callWithRequest('cluster.putSettings', { body: deleteClusterPayload }); - const acknowledged = get(response, 'acknowledged'); - const cluster = get(response, `persistent.cluster.remote.${name}`); - - if (acknowledged && !cluster) { - return { success: true }; + }; + + // Send the request to delete the cluster and return an error if it could not be deleted. + const sendRequestToDeleteCluster = async (name) => { + try { + const body = serializeCluster({ name }); + const response = await callWithRequest('cluster.putSettings', { body }); + const acknowledged = get(response, 'acknowledged'); + const cluster = get(response, `persistent.cluster.remote.${name}`); + + if (acknowledged && !cluster) { + return null; + } + + // If for some reason the ES response still returns the cluster information, + // return an error. This shouldn't happen. + return wrapCustomError(new Error('Unable to delete cluster, information still returned from ES.'), 400); + } catch (error) { + if (isEsError(error)) { + return wrapEsError(error); + } + + return wrapUnknownError(error); } + }; - // If for some reason the ES response still returns the cluster information, - // return an error. This shouldn't happen. - return wrapCustomError(new Error('Unable to delete cluster, information still returned from ES.'), 400); - } catch (err) { - if (isEsError(err)) { - return wrapEsError(err); + const deleteCluster = async (clusterName) => { + try { + // Validate that the cluster exists + let error = await validateClusterDoesExist(clusterName); + + if (!error) { + // Delete the cluster + error = await sendRequestToDeleteCluster(clusterName); + } + + if (error) { + throw error; + } + + // If we are here, it means that everything went well... + itemsDeleted.push(clusterName); + } catch (error) { + errors.push({ name: clusterName, error }); } + }; - return wrapUnknownError(err); - } - }, - config: { - pre: [ licensePreRouting ] + // Delete all our cluster in parallel + await Promise.all(names.map(deleteCluster)); + + return { + itemsDeleted, + errors, + }; } }); } diff --git a/x-pack/plugins/remote_clusters/server/routes/api/remote_clusters/register_delete_route.test.js b/x-pack/plugins/remote_clusters/server/routes/api/remote_clusters/register_delete_route.test.js index 8c90b5bb0cf54..f60a99eebf779 100644 --- a/x-pack/plugins/remote_clusters/server/routes/api/remote_clusters/register_delete_route.test.js +++ b/x-pack/plugins/remote_clusters/server/routes/api/remote_clusters/register_delete_route.test.js @@ -48,11 +48,11 @@ describe('[API Routes] Remote Clusters Delete', () => { registerDeleteRoute(server); const response = await routeHandler({ params: { - name: 'test_cluster' + nameOrNames: 'test_cluster' } }); - expect(response).toEqual({ success: true }); + expect(response).toEqual({ errors: [], itemsDeleted: ['test_cluster'] }); }); it('should return an error if the response does still contain cluster information', async () => { @@ -74,11 +74,14 @@ describe('[API Routes] Remote Clusters Delete', () => { registerDeleteRoute(server); const response = await routeHandler({ params: { - name: 'test_cluster' + nameOrNames: 'test_cluster' } }); - expect(response).toEqual(wrapCustomError(new Error('Unable to delete cluster, information still returned from ES.'), 400)); + expect(response.errors).toEqual([{ + name: 'test_cluster', + error: wrapCustomError(new Error('Unable to delete cluster, information still returned from ES.'), 400), + }]); }); it('should return an error if the cluster does not exist', async () => { @@ -86,11 +89,14 @@ describe('[API Routes] Remote Clusters Delete', () => { registerDeleteRoute(server); const response = await routeHandler({ params: { - name: 'test_cluster' + nameOrNames: 'test_cluster' } }); - expect(response).toEqual(wrapCustomError(new Error('There is no remote cluster with that name.'), 404)); + expect(response.errors).toEqual([{ + name: 'test_cluster', + error: wrapCustomError(new Error('There is no remote cluster with that name.'), 404), + }]); }); it('should forward an ES error', async () => { @@ -101,10 +107,13 @@ describe('[API Routes] Remote Clusters Delete', () => { registerDeleteRoute(server); const response = await routeHandler({ params: { - name: 'test_cluster' + nameOrNames: 'test_cluster' } }); - expect(response).toEqual(Boom.boomify(mockError)); + expect(response.errors).toEqual([{ + name: 'test_cluster', + error: Boom.boomify(mockError), + }]); }); }); diff --git a/x-pack/test/api_integration/apis/management/remote_clusters/remote_clusters.js b/x-pack/test/api_integration/apis/management/remote_clusters/remote_clusters.js index 6651a642fdc1d..5f8c769cb2b5b 100644 --- a/x-pack/test/api_integration/apis/management/remote_clusters/remote_clusters.js +++ b/x-pack/test/api_integration/apis/management/remote_clusters/remote_clusters.js @@ -48,6 +48,7 @@ export default function ({ getService }) { isConfiguredByNode: false, }); }); + it('should not allow us to re-add an existing remote cluster', async () => { const uri = `${API_BASE_PATH}`; @@ -142,7 +143,84 @@ export default function ({ getService }) { .set('kbn-xsrf', 'xxx') .expect(200); - expect(body).to.eql({ success: true }); + expect(body).to.eql({ + itemsDeleted: ['test_cluster'], + errors: [], + }); + }); + + it('should allow us to delete multiple remote clusters', async () => { + // Create clusters to delete. + await supertest + .post(API_BASE_PATH) + .set('kbn-xsrf', 'xxx') + .send({ + name: 'test_cluster1', + seeds: [ + NODE_SEED + ], + skipUnavailable: true, + }) + .expect(200); + + await supertest + .post(API_BASE_PATH) + .set('kbn-xsrf', 'xxx') + .send({ + name: 'test_cluster2', + seeds: [ + NODE_SEED + ], + skipUnavailable: true, + }) + .expect(200); + + const uri = `${API_BASE_PATH}/test_cluster1,test_cluster2`; + + const { + body: { itemsDeleted, errors } + } = await supertest + .delete(uri) + .set('kbn-xsrf', 'xxx') + .expect(200); + + expect(errors).to.eql([]); + + // The order isn't guaranteed, so we assert against individual names instead of asserting + // against the value of the array itself. + ['test_cluster1', 'test_cluster2'].forEach(clusterName => { + expect(itemsDeleted.includes(clusterName)).to.be(true); + }); + }); + + it(`should tell us which remote clusters couldn't be deleted`, async () => { + const uri = `${API_BASE_PATH}/test_cluster_doesnt_exist`; + + const { body } = await supertest + .delete(uri) + .set('kbn-xsrf', 'xxx') + .expect(200); + + expect(body).to.eql({ + itemsDeleted: [], + errors: [{ + name: 'test_cluster_doesnt_exist', + error: { + isBoom: true, + isServer: false, + data: null, + output: { + statusCode: 404, + payload: { + statusCode: 404, + error: 'Not Found', + message: 'There is no remote cluster with that name.', + }, + headers: {}, + }, + }, + }], + }); }); }); });