migrations incorrectly detects cluster routing allocation setting as …

…incompatible (elastic#131712) * Add reproducing test case * Fix and add integration test * Transient settings should take preference Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
rudolf · May 9, 2022 · 638bfbe · 638bfbe
1 parent 1f9047d
commit 638bfbe
Show file tree

Hide file tree

Showing 3 changed files with 220 additions and 120 deletions.
diff --git a/src/core/server/saved_objects/migrations/actions/initialize_action.test.ts b/src/core/server/saved_objects/migrations/actions/initialize_action.test.ts
@@ -6,6 +6,7 @@
  * Side Public License, v 1.
  */
 
+import * as Either from 'fp-ts/lib/Either';
 import { catchRetryableEsClientErrors } from './catch_retryable_es_client_errors';
 import { errors as EsErrors } from '@elastic/elasticsearch';
 jest.mock('./catch_retryable_es_client_errors');
@@ -16,16 +17,16 @@ describe('initAction', () => {
   beforeEach(() => {
     jest.clearAllMocks();
   });
-  const retryableError = new EsErrors.ResponseError(
-    elasticsearchClientMock.createApiResponse({
-      statusCode: 503,
-      body: { error: { type: 'es_type', reason: 'es_reason' } },
-    })
-  );
-  const client = elasticsearchClientMock.createInternalClient(
-    elasticsearchClientMock.createErrorTransportRequestPromise(retryableError)
-  );
   it('calls catchRetryableEsClientErrors when the promise rejects', async () => {
+    const retryableError = new EsErrors.ResponseError(
+      elasticsearchClientMock.createApiResponse({
+        statusCode: 503,
+        body: { error: { type: 'es_type', reason: 'es_reason' } },
+      })
+    );
+    const client = elasticsearchClientMock.createInternalClient(
+      elasticsearchClientMock.createErrorTransportRequestPromise(retryableError)
+    );
     const task = initAction({ client, indices: ['my_index'] });
     try {
       await task();
@@ -34,4 +35,88 @@ describe('initAction', () => {
     }
     expect(catchRetryableEsClientErrors).toHaveBeenCalledWith(retryableError);
   });
+  it('resolves right when persistent and transient cluster settings are compatible', async () => {
+    const clusterSettingsResponse = {
+      transient: { 'cluster.routing.allocation.enable': 'all' },
+      persistent: { 'cluster.routing.allocation.enable': 'all' },
+    };
+    const client = elasticsearchClientMock.createInternalClient(
+      new Promise((res) => res(clusterSettingsResponse))
+    );
+    const task = initAction({ client, indices: ['my_index'] });
+    const result = await task();
+    expect(Either.isRight(result)).toEqual(true);
+  });
+  it('resolves right when persistent and transient cluster settings are undefined', async () => {
+    const clusterSettingsResponse = {
+      transient: {},
+      persistent: {},
+    };
+    const client = elasticsearchClientMock.createInternalClient(
+      new Promise((res) => res(clusterSettingsResponse))
+    );
+    const task = initAction({ client, indices: ['my_index'] });
+    const result = await task();
+    expect(Either.isRight(result)).toEqual(true);
+  });
+  it('resolves right when persistent cluster settings are compatible', async () => {
+    const clusterSettingsResponse = {
+      transient: {},
+      persistent: { 'cluster.routing.allocation.enable': 'all' },
+    };
+    const client = elasticsearchClientMock.createInternalClient(
+      new Promise((res) => res(clusterSettingsResponse))
+    );
+    const task = initAction({ client, indices: ['my_index'] });
+    const result = await task();
+    expect(Either.isRight(result)).toEqual(true);
+  });
+  it('resolves right when transient cluster settings are compatible', async () => {
+    const clusterSettingsResponse = {
+      transient: { 'cluster.routing.allocation.enable': 'all' },
+      persistent: {},
+    };
+    const client = elasticsearchClientMock.createInternalClient(
+      new Promise((res) => res(clusterSettingsResponse))
+    );
+    const task = initAction({ client, indices: ['my_index'] });
+    const result = await task();
+    expect(Either.isRight(result)).toEqual(true);
+  });
+  it('resolves right when valid transient settings, incompatible persistent settings', async () => {
+    const clusterSettingsResponse = {
+      transient: { 'cluster.routing.allocation.enable': 'all' },
+      persistent: { 'cluster.routing.allocation.enable': 'primaries' },
+    };
+    const client = elasticsearchClientMock.createInternalClient(
+      new Promise((res) => res(clusterSettingsResponse))
+    );
+    const task = initAction({ client, indices: ['my_index'] });
+    const result = await task();
+    expect(Either.isRight(result)).toEqual(true);
+  });
+  it('resolves left when valid persistent settings, incompatible transient settings', async () => {
+    const clusterSettingsResponse = {
+      transient: { 'cluster.routing.allocation.enable': 'primaries' },
+      persistent: { 'cluster.routing.allocation.enable': 'alls' },
+    };
+    const client = elasticsearchClientMock.createInternalClient(
+      new Promise((res) => res(clusterSettingsResponse))
+    );
+    const task = initAction({ client, indices: ['my_index'] });
+    const result = await task();
+    expect(Either.isLeft(result)).toEqual(true);
+  });
+  it('resolves left when transient cluster settings are incompatible', async () => {
+    const clusterSettingsResponse = {
+      transient: { 'cluster.routing.allocation.enable': 'none' },
+      persistent: { 'cluster.routing.allocation.enable': 'all' },
+    };
+    const client = elasticsearchClientMock.createInternalClient(
+      new Promise((res) => res(clusterSettingsResponse))
+    );
+    const task = initAction({ client, indices: ['my_index'] });
+    const result = await task();
+    expect(Either.isLeft(result)).toEqual(true);
+  });
 });
diff --git a/src/core/server/saved_objects/migrations/actions/initialize_action.ts b/src/core/server/saved_objects/migrations/actions/initialize_action.ts
@@ -44,16 +44,15 @@ export const checkClusterRoutingAllocationEnabledTask =
         flat_settings: true,
       })
       .then((settings) => {
-        const clusterRoutingAllocations: string[] =
+        // transient settings take preference over persistent settings
+        const clusterRoutingAllocation =
           settings?.transient?.[routingAllocationEnable] ??
-          settings?.persistent?.[routingAllocationEnable] ??
-          [];
+          settings?.persistent?.[routingAllocationEnable];
 
-        const clusterRoutingAllocationEnabled =
-          [...clusterRoutingAllocations].length === 0 ||
-          [...clusterRoutingAllocations].every((s: string) => s === 'all'); // if set, only allow 'all'
+        const clusterRoutingAllocationEnabledIsAll =
+          clusterRoutingAllocation === undefined || clusterRoutingAllocation === 'all';
 
-        if (!clusterRoutingAllocationEnabled) {
+        if (!clusterRoutingAllocationEnabledIsAll) {
           return Either.left({
             type: 'unsupported_cluster_routing_allocation' as const,
             message: