diff --git a/changelogs/fragments/8184.yml b/changelogs/fragments/8184.yml new file mode 100644 index 00000000000..d2231309838 --- /dev/null +++ b/changelogs/fragments/8184.yml @@ -0,0 +1,2 @@ +fix: +- Fix bug when dynamic config index and alias are checked ([#8184](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/8184)) \ No newline at end of file diff --git a/src/core/server/config/service/config_store_client/opensearch_config_store.test.ts b/src/core/server/config/service/config_store_client/opensearch_config_store.test.ts index 967ed2900bb..7630bce4523 100644 --- a/src/core/server/config/service/config_store_client/opensearch_config_store.test.ts +++ b/src/core/server/config/service/config_store_client/opensearch_config_store.test.ts @@ -5,12 +5,16 @@ import { SearchResponse } from '../../../opensearch'; import { opensearchClientMock } from '../../../opensearch/client/mocks'; -import { DYNAMIC_APP_CONFIG_ALIAS } from '../../utils/constants'; +import { DYNAMIC_APP_CONFIG_ALIAS, DYNAMIC_APP_CONFIG_INDEX_PREFIX } from '../../utils/constants'; import { OpenSearchConfigStoreClient } from './opensearch_config_store_client'; import { ConfigDocument } from './types'; import _ from 'lodash'; import { ConfigBlob } from '../../types'; -import { BulkOperationContainer } from '@opensearch-project/opensearch/api/types'; +import { + BulkOperationContainer, + CatIndicesResponse, + IndicesGetAliasResponse, +} from '@opensearch-project/opensearch/api/types'; import { getDynamicConfigIndexName } from '../../utils/utils'; describe('OpenSearchConfigStoreClient', () => { @@ -32,10 +36,12 @@ describe('OpenSearchConfigStoreClient', () => { isListConfig: boolean; configDocuments: ConfigDocument[]; existsAliasResult: boolean; + getAliasIndicesResult: IndicesGetAliasResponse; + catIndicesResult: CatIndicesResponse; } /** - * Creates a new OpenSearch client mock complete with a mock for existsAlias() and search() results + * Creates a new OpenSearch client mock complete with a mock for existsAlias(), cat.indices(), and search() results * * @param param0 * @returns @@ -44,6 +50,8 @@ describe('OpenSearchConfigStoreClient', () => { isListConfig, configDocuments, existsAliasResult, + getAliasIndicesResult, + catIndicesResult, }: OpenSearchClientMockProps) => { const mockClient = opensearchClientMock.createOpenSearchClient(); @@ -53,6 +61,18 @@ describe('OpenSearchConfigStoreClient', () => { }) ); + mockClient.cat.indices.mockResolvedValue( + opensearchClientMock.createApiResponse({ + body: catIndicesResult, + }) + ); + + mockClient.indices.getAlias.mockResolvedValue( + opensearchClientMock.createApiResponse({ + body: getAliasIndicesResult, + }) + ); + // @ts-expect-error mockClient.search.mockImplementation((request, options) => { // Filters out results when the request is for getting/bulk getting configs @@ -83,6 +103,49 @@ describe('OpenSearchConfigStoreClient', () => { return mockClient; }; + const noDynamicConfigIndexResults: CatIndicesResponse = [ + { + index: `${DYNAMIC_APP_CONFIG_INDEX_PREFIX}_`, + }, + { + index: `${DYNAMIC_APP_CONFIG_INDEX_PREFIX}_foo`, + }, + { + index: `${DYNAMIC_APP_CONFIG_INDEX_PREFIX}_foo_2`, + }, + ]; + + const oneDynamicConfigIndexResult: CatIndicesResponse = [ + { + index: `${DYNAMIC_APP_CONFIG_INDEX_PREFIX}_1`, + }, + ]; + + const multipleDynamicConfigIndexResults: CatIndicesResponse = [ + { + index: `${DYNAMIC_APP_CONFIG_INDEX_PREFIX}_2`, + }, + { + index: `${DYNAMIC_APP_CONFIG_INDEX_PREFIX}_4`, + }, + { + index: `${DYNAMIC_APP_CONFIG_INDEX_PREFIX}_800`, + }, + ]; + + const validAliasIndicesResponse: IndicesGetAliasResponse = { + [`${DYNAMIC_APP_CONFIG_INDEX_PREFIX}_4`]: { aliases: { DYNAMIC_APP_CONFIG_ALIAS: {} } }, + }; + + const multipleAliasIndicesResponse: IndicesGetAliasResponse = { + [`${DYNAMIC_APP_CONFIG_INDEX_PREFIX}_4`]: { aliases: { DYNAMIC_APP_CONFIG_ALIAS: {} } }, + [`${DYNAMIC_APP_CONFIG_INDEX_PREFIX}_2`]: { aliases: { DYNAMIC_APP_CONFIG_ALIAS: {} } }, + }; + + const invalidAliasIndicesResponse: IndicesGetAliasResponse = { + [`.some_random_index_8`]: { aliases: { DYNAMIC_APP_CONFIG_ALIAS: {} } }, + }; + const configDocument: ConfigDocument = { config_name: 'some_config_name', config_blob: { @@ -143,26 +206,88 @@ describe('OpenSearchConfigStoreClient', () => { it.each([ { existsAliasResult: false, + catIndicesResult: noDynamicConfigIndexResults, + getAliasIndicesResult: validAliasIndicesResponse, numCreateCalls: 1, + numUpdateCalls: 0, + errorThrown: false, + }, + { + existsAliasResult: false, + catIndicesResult: multipleDynamicConfigIndexResults, + getAliasIndicesResult: validAliasIndicesResponse, + numCreateCalls: 0, + numUpdateCalls: 1, + errorThrown: false, + }, + { + existsAliasResult: false, + catIndicesResult: oneDynamicConfigIndexResult, + getAliasIndicesResult: validAliasIndicesResponse, + numCreateCalls: 0, + numUpdateCalls: 1, + errorThrown: false, }, { existsAliasResult: true, + catIndicesResult: multipleDynamicConfigIndexResults, + getAliasIndicesResult: {}, numCreateCalls: 0, + numUpdateCalls: 0, + errorThrown: true, + }, + { + existsAliasResult: true, + catIndicesResult: multipleDynamicConfigIndexResults, + getAliasIndicesResult: multipleAliasIndicesResponse, + numCreateCalls: 0, + numUpdateCalls: 0, + errorThrown: true, + }, + { + existsAliasResult: true, + catIndicesResult: multipleDynamicConfigIndexResults, + getAliasIndicesResult: invalidAliasIndicesResponse, + numCreateCalls: 0, + numUpdateCalls: 0, + errorThrown: true, + }, + { + existsAliasResult: true, + catIndicesResult: multipleDynamicConfigIndexResults, + getAliasIndicesResult: validAliasIndicesResponse, + numCreateCalls: 0, + numUpdateCalls: 0, + errorThrown: false, }, ])( - 'should create config index $numCreateCalls times when existsAlias() is $existsAliasResult', - async ({ existsAliasResult, numCreateCalls }) => { + 'should throw error should be $errorThrown, create() should be called $numCreateCalls times, and update() should be called $numUpdateCalls times', + async ({ + existsAliasResult, + catIndicesResult, + getAliasIndicesResult, + numCreateCalls, + numUpdateCalls, + errorThrown, + }) => { const mockClient = createOpenSearchClientMock({ isListConfig: false, configDocuments: [], existsAliasResult, + getAliasIndicesResult, + catIndicesResult, }); const configStoreClient = new OpenSearchConfigStoreClient(mockClient); - await configStoreClient.createDynamicConfigIndex(); + + if (errorThrown) { + expect(configStoreClient.createDynamicConfigIndex()).rejects.toThrowError(); + } else { + await configStoreClient.createDynamicConfigIndex(); + } expect(mockClient.indices.existsAlias).toBeCalled(); expect(mockClient.indices.create).toBeCalledTimes(numCreateCalls); - expect(mockClient.indices.updateAliases).toBeCalledTimes(numCreateCalls); + expect(mockClient.indices.updateAliases).toBeCalledTimes(numUpdateCalls); } ); }); @@ -175,6 +300,8 @@ describe('OpenSearchConfigStoreClient', () => { isListConfig: false, configDocuments: [configDocument], existsAliasResult: false, + catIndicesResult: oneDynamicConfigIndexResult, + getAliasIndicesResult: validAliasIndicesResponse, }); const configStoreClient = new OpenSearchConfigStoreClient(mockClient); @@ -210,6 +337,8 @@ describe('OpenSearchConfigStoreClient', () => { isListConfig: false, configDocuments, existsAliasResult: false, + catIndicesResult: oneDynamicConfigIndexResult, + getAliasIndicesResult: validAliasIndicesResponse, }); const configStoreClient = new OpenSearchConfigStoreClient(mockClient); @@ -274,6 +403,8 @@ describe('OpenSearchConfigStoreClient', () => { isListConfig: true, configDocuments: allConfigDocuments, existsAliasResult: false, + catIndicesResult: oneDynamicConfigIndexResult, + getAliasIndicesResult: validAliasIndicesResponse, }); const configStoreClient = new OpenSearchConfigStoreClient(mockClient); const actualMap = await configStoreClient.listConfigs(); @@ -342,6 +473,8 @@ describe('OpenSearchConfigStoreClient', () => { isListConfig: false, configDocuments: newConfigDocuments, existsAliasResult: false, + catIndicesResult: oneDynamicConfigIndexResult, + getAliasIndicesResult: validAliasIndicesResponse, }); const configStoreClient = new OpenSearchConfigStoreClient(mockClient); await configStoreClient.createConfig({ @@ -540,6 +673,8 @@ describe('OpenSearchConfigStoreClient', () => { isListConfig: false, configDocuments: existingConfigs, existsAliasResult: false, + catIndicesResult: oneDynamicConfigIndexResult, + getAliasIndicesResult: validAliasIndicesResponse, }); const configStoreClient = new OpenSearchConfigStoreClient(mockClient); await configStoreClient.bulkCreateConfigs({ @@ -565,6 +700,8 @@ describe('OpenSearchConfigStoreClient', () => { isListConfig: false, configDocuments: [], existsAliasResult: false, + catIndicesResult: oneDynamicConfigIndexResult, + getAliasIndicesResult: validAliasIndicesResponse, }); const configStoreClient = new OpenSearchConfigStoreClient(mockClient); await configStoreClient.deleteConfig({ name: 'some_config_name' }); @@ -604,6 +741,8 @@ describe('OpenSearchConfigStoreClient', () => { isListConfig: false, configDocuments: [], existsAliasResult: false, + catIndicesResult: oneDynamicConfigIndexResult, + getAliasIndicesResult: validAliasIndicesResponse, }); const configStoreClient = new OpenSearchConfigStoreClient(mockClient); await configStoreClient.bulkDeleteConfigs({ diff --git a/src/core/server/config/service/config_store_client/opensearch_config_store_client.ts b/src/core/server/config/service/config_store_client/opensearch_config_store_client.ts index 8cae9284a93..8163b5225a7 100644 --- a/src/core/server/config/service/config_store_client/opensearch_config_store_client.ts +++ b/src/core/server/config/service/config_store_client/opensearch_config_store_client.ts @@ -20,9 +20,15 @@ import { } from '../../types'; import { DYNAMIC_APP_CONFIG_ALIAS, + DYNAMIC_APP_CONFIG_INDEX_PREFIX, DYNAMIC_APP_CONFIG_MAX_RESULT_SIZE, } from '../../utils/constants'; -import { pathToString, getDynamicConfigIndexName } from '../../utils/utils'; +import { + pathToString, + getDynamicConfigIndexName, + isDynamicConfigIndex, + extractVersionFromDynamicConfigIndex, +} from '../../utils/utils'; import { ConfigDocument } from './types'; interface ConfigMapEntry { @@ -48,25 +54,52 @@ export class OpenSearchConfigStoreClient implements IDynamicConfigStoreClient { * TODO Add migration logic */ public async createDynamicConfigIndex() { - const existsResponse = await this.#openSearchClient.indices.existsAlias({ + const existsAliasResponse = await this.#openSearchClient.indices.existsAlias({ name: DYNAMIC_APP_CONFIG_ALIAS, }); - if (!existsResponse.body) { - await this.#openSearchClient.indices.create({ - index: getDynamicConfigIndexName(1), - }); - await this.#openSearchClient.indices.updateAliases({ - body: { - actions: [ - { - add: { - index: getDynamicConfigIndexName(1), - alias: DYNAMIC_APP_CONFIG_ALIAS, + + if (!existsAliasResponse.body) { + const latestVersion = await this.searchLatestConfigIndex(); + if (latestVersion < 1) { + await this.#openSearchClient.indices.create({ + index: getDynamicConfigIndexName(1), + body: { + aliases: { [DYNAMIC_APP_CONFIG_ALIAS]: {} }, + }, + }); + } else { + await this.#openSearchClient.indices.updateAliases({ + body: { + actions: [ + { + add: { + index: getDynamicConfigIndexName(latestVersion), + alias: DYNAMIC_APP_CONFIG_ALIAS, + }, }, - }, - ], - }, + ], + }, + }); + } + } else { + const results = await this.#openSearchClient.indices.getAlias({ + name: DYNAMIC_APP_CONFIG_ALIAS, }); + + const indices = Object.keys(results.body); + if (indices.length !== 1) { + throw new Error( + `Alias ${DYNAMIC_APP_CONFIG_ALIAS} is pointing to 0 or multiple indices. Please remove the alias(es) and restart the server` + ); + } + const numNonDynamicConfigIndices = indices.filter((index) => !isDynamicConfigIndex(index)) + .length; + + if (numNonDynamicConfigIndices > 0) { + throw new Error( + `Alias ${DYNAMIC_APP_CONFIG_ALIAS} is pointing to a non dynamic config index. Please remove the alias and restart the server` + ); + } } } @@ -342,4 +375,34 @@ export class OpenSearchConfigStoreClient implements IDynamicConfigStoreClient { }, }); } + + /** + * Finds the most updated dynamic config index + * + * @returns the latest version number or 0 if not found + */ + private async searchLatestConfigIndex(): Promise { + const configIndices = await this.#openSearchClient.cat.indices({ + index: `${DYNAMIC_APP_CONFIG_INDEX_PREFIX}_*`, + format: 'json', + }); + + if (configIndices.body.length < 1) { + return 0; + } + + const validIndices = configIndices.body + .map((hit) => hit.index?.toString()) + .filter((index) => index && isDynamicConfigIndex(index)); + + return validIndices.length === 0 + ? 0 + : validIndices + .map((configIndex) => { + return configIndex ? extractVersionFromDynamicConfigIndex(configIndex) : 0; + }) + .reduce((currentMax, currentNum) => { + return currentMax && currentNum && currentMax > currentNum ? currentMax : currentNum; + }); + } } diff --git a/src/core/server/config/utils/utils.test.ts b/src/core/server/config/utils/utils.test.ts index 1f0a3c0a5aa..1e45dbf2dd6 100644 --- a/src/core/server/config/utils/utils.test.ts +++ b/src/core/server/config/utils/utils.test.ts @@ -3,9 +3,16 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { createLocalStore, mergeConfigs, pathToString } from './utils'; +import { + createLocalStore, + extractVersionFromDynamicConfigIndex, + isDynamicConfigIndex, + mergeConfigs, + pathToString, +} from './utils'; import { Request } from 'hapi__hapi'; import { loggerMock } from '../../logging/logger.mock'; +import { DYNAMIC_APP_CONFIG_INDEX_PREFIX } from './constants'; describe('Utils', () => { test.each([ @@ -178,4 +185,77 @@ describe('Utils', () => { }); } ); + + test.each([ + { + index: `.sanity_check_2`, + result: false, + }, + { + index: `${DYNAMIC_APP_CONFIG_INDEX_PREFIX}_144b`, + result: false, + }, + { + index: `${DYNAMIC_APP_CONFIG_INDEX_PREFIX}_354.4`, + result: false, + }, + { + index: `${DYNAMIC_APP_CONFIG_INDEX_PREFIX}_-4`, + result: false, + }, + { + index: `${DYNAMIC_APP_CONFIG_INDEX_PREFIX}_asdfasdfasdf`, + result: false, + }, + { + index: `opensearch_dashboards_dynamic_config_2`, + result: false, + }, + { + index: `${DYNAMIC_APP_CONFIG_INDEX_PREFIX}3`, + result: false, + }, + { + index: `${DYNAMIC_APP_CONFIG_INDEX_PREFIX}`, + result: false, + }, + { + index: `${DYNAMIC_APP_CONFIG_INDEX_PREFIX}_0`, + result: true, + }, + { + index: `${DYNAMIC_APP_CONFIG_INDEX_PREFIX}_1`, + result: true, + }, + { + index: `${DYNAMIC_APP_CONFIG_INDEX_PREFIX}_141515`, + result: true, + }, + ])('isDynamicConfigIndex() should return $result when index is $index', ({ index, result }) => { + expect(isDynamicConfigIndex(index)).toBe(result); + }); + + test.each([ + { + index: `${DYNAMIC_APP_CONFIG_INDEX_PREFIX}_-10`, + result: 0, + }, + { + index: `${DYNAMIC_APP_CONFIG_INDEX_PREFIX}_0`, + result: 0, + }, + { + index: `${DYNAMIC_APP_CONFIG_INDEX_PREFIX}_1`, + result: 1, + }, + { + index: `${DYNAMIC_APP_CONFIG_INDEX_PREFIX}_5732`, + result: 5732, + }, + ])( + 'extractVersionFromDynamicConfigIndex() should extract number $result from index $index', + ({ index, result }) => { + expect(extractVersionFromDynamicConfigIndex(index)).toBe(result); + } + ); }); diff --git a/src/core/server/config/utils/utils.ts b/src/core/server/config/utils/utils.ts index 46d992fbc36..1241ab5d6e2 100644 --- a/src/core/server/config/utils/utils.ts +++ b/src/core/server/config/utils/utils.ts @@ -75,6 +75,25 @@ export const getDynamicConfigIndexName = (n: number) => { return `${DYNAMIC_APP_CONFIG_INDEX_PREFIX}_${n}`; }; +/** + * Basic check to ensure the index matches the pattern (will pass for ${DYNAMIC_APP_CONFIG_INDEX_PREFIX}_0) + * + * @param index + * @returns + */ +export const isDynamicConfigIndex = (index: string) => { + const regex = new RegExp(`^${DYNAMIC_APP_CONFIG_INDEX_PREFIX}_\\d+$`); + return regex.test(index); +}; + +export const extractVersionFromDynamicConfigIndex = (index: string) => { + if (!isDynamicConfigIndex(index)) { + return 0; + } + const indexSuffix = index.replace(`${DYNAMIC_APP_CONFIG_INDEX_PREFIX}_`, ''); + return Number(indexSuffix); +}; + export const createLocalStoreFromOsdRequest = ( logger: Logger, request: OpenSearchDashboardsRequest,