-
Notifications
You must be signed in to change notification settings - Fork 8.5k
[Lock Manager] Handle alias resolution when checking lock index mappings #244559
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 5 commits
9d892a1
6cade91
97d75b8
f385373
9fad13a
b95fe4a
a22de18
62e2b0a
790cc1c
3ac5d51
e521b62
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,220 @@ | ||
| /* | ||
| * 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 { 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 = { | ||
| dynamic: 'false', | ||
| properties: { | ||
| token: { type: 'keyword' }, | ||
| expiresAt: { type: 'date' }, | ||
| createdAt: { type: 'date' }, | ||
| metadata: { enabled: false, type: 'object' }, | ||
| }, | ||
| }; | ||
|
|
||
| const incorrectMappings = { | ||
| 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 | ||
|---|---|---|---|---|
|
|
@@ -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`; | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I never used
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. btw. if the index Based on this I think it will: kibana/src/platform/packages/shared/kbn-lock-manager/src/setup_lock_manager_index.ts Line 92 in 75d918c
Can you double check if that's the case? Because that could be problematic
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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.
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 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 aliasI 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
No, I like it 👍
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, I think if an alias exists and it points to a concrete index we should handle (swallow) the error, and skip error creation
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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', () => { | ||||
|
|
||||
There was a problem hiding this comment.
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!
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 aliasThere was a problem hiding this comment.
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
ensureTemplatesAndIndexCreatedshould create the alias.kibana_lockspointing 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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