Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/platform/packages/shared/kbn-lock-manager/moon.yml
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ project:
dependsOn:
- '@kbn/logging'
- '@kbn/core'
- '@kbn/core-elasticsearch-client-server-mocks'
- '@kbn/core-logging-server-mocks'
tags:
- shared-server
- package
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,221 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the "Elastic License
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
* Public License v 1"; you may not use this file except in compliance with, at
* your election, the "Elastic License 2.0", the "GNU Affero General Public
* License v3.0 only", or the "Server Side Public License, v 1".
*/

import { errors } from '@elastic/elasticsearch';
import type { MappingTypeMapping } from '@elastic/elasticsearch/lib/api/types';
import { elasticsearchClientMock } from '@kbn/core-elasticsearch-client-server-mocks';
import { loggingSystemMock } from '@kbn/core-logging-server-mocks';
import {
removeLockIndexWithIncorrectMappings,
ensureTemplatesAndIndexCreated,
LOCKS_CONCRETE_INDEX_NAME,
} from './setup_lock_manager_index';

describe('removeLockIndexWithIncorrectMappings', () => {
let esClient: ReturnType<typeof elasticsearchClientMock.createInternalClient>;
const logger = loggingSystemMock.createLogger();

const correctMappings: MappingTypeMapping = {
dynamic: 'false',
properties: {
token: { type: 'keyword' },
expiresAt: { type: 'date' },
createdAt: { type: 'date' },
metadata: { enabled: false, type: 'object' },
},
};

const incorrectMappings: MappingTypeMapping = {
dynamic: 'false',
properties: {
token: { type: 'text' }, // incorrect - should be keyword
expiresAt: { type: 'text' }, // incorrect - should be date
createdAt: { type: 'date' },
metadata: { enabled: false, type: 'object' },
},
};

beforeEach(() => {
esClient = elasticsearchClientMock.createInternalClient();
});

describe('when index name matches LOCKS_CONCRETE_INDEX_NAME', () => {
it('does not delete index when mappings are correct', async () => {
esClient.indices.getMapping.mockResolvedValueOnce({
[LOCKS_CONCRETE_INDEX_NAME]: { mappings: correctMappings },
});

await removeLockIndexWithIncorrectMappings(esClient, logger);

expect(esClient.indices.delete).not.toHaveBeenCalled();
});

it('deletes index when mappings are incorrect', async () => {
esClient.indices.getMapping.mockResolvedValueOnce({
[LOCKS_CONCRETE_INDEX_NAME]: { mappings: incorrectMappings },
});

await removeLockIndexWithIncorrectMappings(esClient, logger);

expect(esClient.indices.delete).toHaveBeenCalledWith({ index: LOCKS_CONCRETE_INDEX_NAME });
});
});

describe('when index is accessed via alias (response key differs from requested index)', () => {
const reindexedIndexName = `${LOCKS_CONCRETE_INDEX_NAME}-reindexed-for-10`;

it('does not delete index when mappings are correct', async () => {
esClient.indices.getMapping.mockResolvedValueOnce({
[reindexedIndexName]: { mappings: correctMappings },
});

await removeLockIndexWithIncorrectMappings(esClient, logger);

expect(esClient.indices.delete).not.toHaveBeenCalled();
});

it('deletes index when mappings are incorrect', async () => {
esClient.indices.getMapping.mockResolvedValueOnce({
[reindexedIndexName]: { mappings: incorrectMappings },
});

await removeLockIndexWithIncorrectMappings(esClient, logger);

expect(esClient.indices.delete).toHaveBeenCalledWith({ index: LOCKS_CONCRETE_INDEX_NAME });
});
});

describe('when index does not exist (404 error)', () => {
it('returns early without error', async () => {
esClient.indices.getMapping.mockRejectedValueOnce(
new errors.ResponseError({
statusCode: 404,
body: { error: { type: 'index_not_found_exception' } },
headers: {},
meta: {} as any,
warnings: [],
})
);

await removeLockIndexWithIncorrectMappings(esClient, logger);

expect(esClient.indices.delete).not.toHaveBeenCalled();
});
});

describe('when getMapping returns empty response', () => {
it('returns early without error', async () => {
esClient.indices.getMapping.mockResolvedValueOnce({});

await removeLockIndexWithIncorrectMappings(esClient, logger);

expect(esClient.indices.delete).not.toHaveBeenCalled();
});
});

describe('when mappings object is empty', () => {
it('returns early without error', async () => {
esClient.indices.getMapping.mockResolvedValueOnce({
[LOCKS_CONCRETE_INDEX_NAME]: { mappings: undefined as any },
});

await removeLockIndexWithIncorrectMappings(esClient, logger);

expect(esClient.indices.delete).not.toHaveBeenCalled();
});
});

describe('when getMapping fails with non-404 error', () => {
it('returns without throwing', async () => {
esClient.indices.getMapping.mockRejectedValueOnce(
new errors.ResponseError({
statusCode: 500,
body: { error: { type: 'internal_server_error' } },
headers: {},
meta: {} as any,
warnings: [],
})
);

await removeLockIndexWithIncorrectMappings(esClient, logger);

expect(esClient.indices.delete).not.toHaveBeenCalled();
});
});

describe('when delete fails', () => {
it('does not throw', async () => {
esClient.indices.getMapping.mockResolvedValueOnce({
[LOCKS_CONCRETE_INDEX_NAME]: { mappings: incorrectMappings },
});
esClient.indices.delete.mockRejectedValueOnce(new Error('Delete failed'));

await expect(removeLockIndexWithIncorrectMappings(esClient, logger)).resolves.not.toThrow();
});
});
});

describe('ensureTemplatesAndIndexCreated', () => {
let esClient: ReturnType<typeof elasticsearchClientMock.createInternalClient>;
const logger = loggingSystemMock.createLogger();

beforeEach(() => {
esClient = elasticsearchClientMock.createInternalClient();
});

it('creates index successfully', async () => {
await ensureTemplatesAndIndexCreated(esClient, logger);

expect(esClient.cluster.putComponentTemplate).toHaveBeenCalled();
expect(esClient.indices.putIndexTemplate).toHaveBeenCalled();
expect(esClient.indices.create).toHaveBeenCalledWith({ index: LOCKS_CONCRETE_INDEX_NAME });
});

it('handles resource_already_exists_exception without throwing', async () => {
esClient.indices.create.mockRejectedValueOnce(
new errors.ResponseError({
statusCode: 400,
body: { error: { type: 'resource_already_exists_exception' } },
headers: {},
meta: {} as any,
warnings: [],
})
);

await expect(ensureTemplatesAndIndexCreated(esClient, logger)).resolves.not.toThrow();
});

it('handles invalid_index_name_exception (index name exists as alias) without throwing', async () => {
esClient.indices.create.mockRejectedValueOnce(
new errors.ResponseError({
statusCode: 400,
body: { error: { type: 'invalid_index_name_exception' } },
headers: {},
meta: {} as any,
warnings: [],
})
);

await expect(ensureTemplatesAndIndexCreated(esClient, logger)).resolves.not.toThrow();
});

it('throws on other errors', async () => {
esClient.indices.create.mockRejectedValueOnce(
new errors.ResponseError({
statusCode: 500,
body: { error: { type: 'internal_server_error' } },
headers: {},
meta: {} as any,
warnings: [],
})
);

await expect(ensureTemplatesAndIndexCreated(esClient, logger)).rejects.toThrow();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,16 @@ export async function removeLockIndexWithIncorrectMappings(
return;
}

const { mappings } = res[LOCKS_CONCRETE_INDEX_NAME];
const indexMappings = Object.values(res);
if (indexMappings.length === 0) {
logger.warn(`No mappings found for "${LOCKS_CONCRETE_INDEX_NAME}"`);
return;
}
const { mappings } = indexMappings[0];
if (!mappings) {
logger.debug(`Mappings object is empty for "${LOCKS_CONCRETE_INDEX_NAME}"`);
return;
}
const hasIncorrectMappings =
mappings.properties?.token?.type !== 'keyword' ||
mappings.properties?.expiresAt?.type !== 'date';
Expand Down Expand Up @@ -96,7 +105,12 @@ export async function ensureTemplatesAndIndexCreated(
error instanceof errors.ResponseError &&
error.body.error.type === 'resource_already_exists_exception';

if (isIndexAlreadyExistsError) {
// Handle the case where the index name already exists as an alias (e.g., after a reindex operation during cluster upgrade)
const isIndexNameExistsAsAliasError =
error instanceof errors.ResponseError &&
error.body.error.type === 'invalid_index_name_exception';

if (isIndexAlreadyExistsError || isIndexNameExistsAsAliasError) {
Comment on lines +108 to +113
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sorenlouv I applied the changes I suggested for what you mentioned in this comment. Thought it's easier to iterate on it if it's seen in the works (and I no longer see the integ test errors). Let me know what you think!

Copy link
Member

@sorenlouv sorenlouv Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the existing integration test cover this? Does it fail without this change? (it should)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it fail without this change? (it should)

Yes, confirmed it fails with:

2)    Stateful Observability - Deployment-agnostic AI Assistant API integration tests
       observability AI Assistant
         LockManager
           index assets
             when lock index is accessed via alias (simulating reindexed scenario)
               should successfully acquire a lock when index is accessed via alias:

      ResponseError: invalid_index_name_exception
        Root causes:
                invalid_index_name_exception: Invalid index name [.kibana_locks-000001], already exists as alias

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw. not needed now, but I wonder if ensureTemplatesAndIndexCreated should create the alias .kibana_locks pointing to .kibana_locks-000001.
And if the alias (or index) already exists we do nothing.

But we'd have to consider if this is fully backwards compatible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a good, future-proof, suggestion, but because of the backwards compatibility considerations and my (just acquired) familiarity with the lock manager, I'd suggest to leave it out of this PR and track as a follow up?

If you're feeling strongly, happy to figure out, but may need more heads as I'm rotating out from SDH and we'll be focusing on AB offsite next week.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I agree. Let's leave that out

logger.debug(`Index ${LOCKS_CONCRETE_INDEX_NAME} already exists. Skipping creation.`);
return;
}
Expand Down
2 changes: 2 additions & 0 deletions src/platform/packages/shared/kbn-lock-manager/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -16,5 +16,7 @@
"kbn_references": [
"@kbn/logging",
"@kbn/core",
"@kbn/core-elasticsearch-client-server-mocks",
"@kbn/core-logging-server-mocks",
]
}
Original file line number Diff line number Diff line change
Expand Up @@ -804,6 +804,72 @@ export default function ApiTest({ getService }: DeploymentAgnosticFtrProviderCon
expect(indexExists).to.be(true);
});
});

describe('when lock index is accessed via alias (simulating reindexed scenario)', () => {
const reindexedIndexName = `${LOCKS_CONCRETE_INDEX_NAME}-reindexed-for-10`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for covering this scenario!


before(async () => {
await deleteLockIndexAssets(es, log);

// Create the actual index with correct mappings but different name
await es.indices.create({
index: reindexedIndexName,
settings: {
number_of_shards: 1,
auto_expand_replicas: '0-1',
hidden: true,
},
mappings: {
dynamic: false,
properties: {
token: { type: 'keyword' },
metadata: { enabled: false },
createdAt: { type: 'date' },
expiresAt: { type: 'date' },
},
},
});

// Add alias pointing LOCKS_CONCRETE_INDEX_NAME to the reindexed index
await es.indices.putAlias({
index: reindexedIndexName,
name: LOCKS_CONCRETE_INDEX_NAME,
});
});

after(async () => {
// Clean up alias and reindexed index
await es.indices.deleteAlias(
{ index: reindexedIndexName, name: LOCKS_CONCRETE_INDEX_NAME },
{ ignore: [404] }
);
await es.indices.delete({ index: reindexedIndexName }, { ignore: [404] });
});

it('should not throw when getMapping returns a different index name than requested', async () => {
// Verify the alias setup
const aliasExists = await es.indices.existsAlias({ name: LOCKS_CONCRETE_INDEX_NAME });
expect(aliasExists).to.be(true);

// This should NOT throw "Cannot destructure property 'mappings' of 'res[LOCKS_CONCRETE_INDEX_NAME]' as it is undefined"
try {
await setupLockManagerIndex(es, logger);
} catch (error) {
expect().fail(
`setupLockManagerIndex should not throw when index is accessed via alias, but got: ${error.message}`
);
}
Comment on lines +854 to +861
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this test. Do you expect it to throw or not?
The comment seems to say that it should not throw, but the code assumes that it throws because the expectation is in the catch block

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I never used expect().fail() but I suppose that fails the test if it reaches the catch block

Copy link
Member

@sorenlouv sorenlouv Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw. if the index ${LOCKS_CONCRETE_INDEX_NAME}-reindexed-for-10 exists, and setupLockManagerIndex is called, will that create a new index called ${LOCKS_INDEX_ALIAS}-000001?

Based on this I think it will:

await esClient.indices.create({ index: LOCKS_CONCRETE_INDEX_NAME });

Can you double check if that's the case? Because that could be problematic

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I never used expect().fail() but I suppose that fails the test if it reaches the catch block

Yeah.. it's kinda like re-throwing an exception and enriching it with more context. Happy to remove and just let fail if that's not our common practice.

btw. if the index ${LOCKS_CONCRETE_INDEX_NAME}-reindexed-for-10 exists, and setupLockManagerIndex is run, will that create a new index called ${LOCKS_INDEX_ALIAS}-000001?

About this, you're good to question this and it's what I've noticed when running the integration tests (hence why I left the PR still in draft).

After resolving the issue with mappings, I found the integ tests still to fail with invalid_index_name_exception:

Error: setupLockManagerIndex should not throw when index is accessed via alias, but got: invalid_index_name_exception
        Root causes:
                invalid_index_name_exception: Invalid index name [.kibana_locks-000001], already exists as alias

I guess this happens within the migration tooling during index migration (aliasing on the concrete index name) and will affect the index creation as you mention.

I got around this locally by catching the exception and skipping creation similarly to resource_already_exists_exception, as a usable alias already exists? Does that workaround sound good to you?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to remove and just let fail if that's not our common practice.

No, I like it 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got around this locally by catching the exception and skipping creation similarly to resource_already_exists_exception, as a usable alias already exists? Does that workaround sound good to you?

Yes, I think if an alias exists and it points to a concrete index we should handle (swallow) the error, and skip error creation

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks for confirming - I've added that change already (might just get lost in all the auto-generated moon changes 🌔 )

});

it('should successfully acquire a lock when index is accessed via alias', async () => {
const lockManager = new LockManager('alias_test_lock', es, logger);
const acquired = await lockManager.acquire();
expect(acquired).to.be(true);

// Cleanup
await lockManager.release();
});
});
});

describe('setup robustness', () => {
Expand Down