Skip to content

Commit

Permalink
[Dynamic Config] Fix bug when attempting dynamic config index creation (
Browse files Browse the repository at this point in the history 
#8184)

* Fix bug when attempting dynamic config index creation

Signed-off-by: Huy Nguyen <[email protected]>

* Changeset file for PR #8184 created/updated

---------

Signed-off-by: Huy Nguyen <[email protected]>
Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
(cherry picked from commit 678c644)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
  • Loading branch information
@github-actions @opensearch-changeset-bot
github-actions[bot] and opensearch-changeset-bot[bot] committed Sep 13, 2024
1 parent 24a919a commit 799bd46
Show file tree
Hide file tree
Showing 5 changed files with 327 additions and 24 deletions.
2 changes: 2 additions & 0 deletions changelogs/fragments/8184.yml
View file
Original file line number Diff line number Diff line change
@@ -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))
153 changes: 146 additions & 7 deletions src/core/server/config/service/config_store_client/opensearch_config_store.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand All @@ -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
Expand All @@ -44,6 +50,8 @@ describe('OpenSearchConfigStoreClient', () => {
isListConfig,
configDocuments,
existsAliasResult,
getAliasIndicesResult,
catIndicesResult,
}: OpenSearchClientMockProps) => {
const mockClient = opensearchClientMock.createOpenSearchClient();

Expand All @@ -53,6 +61,18 @@ describe('OpenSearchConfigStoreClient', () => {
})
);

mockClient.cat.indices.mockResolvedValue(
opensearchClientMock.createApiResponse<CatIndicesResponse>({
body: catIndicesResult,
})
);

mockClient.indices.getAlias.mockResolvedValue(
opensearchClientMock.createApiResponse<IndicesGetAliasResponse>({
body: getAliasIndicesResult,
})
);

// @ts-expect-error
mockClient.search.mockImplementation((request, options) => {
// Filters out results when the request is for getting/bulk getting configs
Expand Down Expand Up @@ -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: {
Expand Down Expand Up @@ -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);
}
);
});
Expand All @@ -175,6 +300,8 @@ describe('OpenSearchConfigStoreClient', () => {
isListConfig: false,
configDocuments: [configDocument],
existsAliasResult: false,
catIndicesResult: oneDynamicConfigIndexResult,
getAliasIndicesResult: validAliasIndicesResponse,
});
const configStoreClient = new OpenSearchConfigStoreClient(mockClient);

Expand Down Expand Up @@ -210,6 +337,8 @@ describe('OpenSearchConfigStoreClient', () => {
isListConfig: false,
configDocuments,
existsAliasResult: false,
catIndicesResult: oneDynamicConfigIndexResult,
getAliasIndicesResult: validAliasIndicesResponse,
});
const configStoreClient = new OpenSearchConfigStoreClient(mockClient);

Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -342,6 +473,8 @@ describe('OpenSearchConfigStoreClient', () => {
isListConfig: false,
configDocuments: newConfigDocuments,
existsAliasResult: false,
catIndicesResult: oneDynamicConfigIndexResult,
getAliasIndicesResult: validAliasIndicesResponse,
});
const configStoreClient = new OpenSearchConfigStoreClient(mockClient);
await configStoreClient.createConfig({
Expand Down Expand Up @@ -540,6 +673,8 @@ describe('OpenSearchConfigStoreClient', () => {
isListConfig: false,
configDocuments: existingConfigs,
existsAliasResult: false,
catIndicesResult: oneDynamicConfigIndexResult,
getAliasIndicesResult: validAliasIndicesResponse,
});
const configStoreClient = new OpenSearchConfigStoreClient(mockClient);
await configStoreClient.bulkCreateConfigs({
Expand All @@ -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' });
Expand Down Expand Up @@ -604,6 +741,8 @@ describe('OpenSearchConfigStoreClient', () => {
isListConfig: false,
configDocuments: [],
existsAliasResult: false,
catIndicesResult: oneDynamicConfigIndexResult,
getAliasIndicesResult: validAliasIndicesResponse,
});
const configStoreClient = new OpenSearchConfigStoreClient(mockClient);
await configStoreClient.bulkDeleteConfigs({
Expand Down
95 changes: 79 additions & 16 deletions src/core/server/config/service/config_store_client/opensearch_config_store_client.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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`
);
}
}
}

Expand Down Expand Up @@ -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<number> {
const configIndices = await this.#openSearchClient.cat.indices({
index: `${DYNAMIC_APP_CONFIG_INDEX_PREFIX}_*`,
format: 'json',
});

if (configIndices.body.length < 1) {
return 0;

Check warning on line 391 in src/core/server/config/service/config_store_client/opensearch_config_store_client.ts

View check run for this annotation

Codecov / codecov/patch

src/core/server/config/service/config_store_client/opensearch_config_store_client.ts#L391 

Added line #L391 was not covered by tests
}

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;
});
}
}
Loading

0 comments on commit 799bd46

Please sign in to comment.