From c25e3780641a1acffeec1f86788dd89ded52eda2 Mon Sep 17 00:00:00 2001 From: Larry Gregory Date: Wed, 22 Jun 2022 08:12:25 -0400 Subject: [PATCH 01/11] Remove session index template --- .../session_management/session_index.test.ts | 60 ++++++++-------- .../session_management/session_index.ts | 68 +++++++++---------- 2 files changed, 63 insertions(+), 65 deletions(-) diff --git a/x-pack/plugins/security/server/session_management/session_index.test.ts b/x-pack/plugins/security/server/session_management/session_index.test.ts index f6405bf56bda7..55b708bb6238f 100644 --- a/x-pack/plugins/security/server/session_management/session_index.test.ts +++ b/x-pack/plugins/security/server/session_management/session_index.test.ts @@ -19,7 +19,7 @@ import type { AuditLogger } from '../audit'; import { auditLoggerMock } from '../audit/mocks'; import { ConfigSchema, createConfig } from '../config'; import { securityMock } from '../mocks'; -import { getSessionIndexTemplate, SessionIndex } from './session_index'; +import { getSessionIndexSettings, SessionIndex } from './session_index'; import { sessionIndexMock } from './session_index.mock'; describe('Session index', () => { @@ -55,7 +55,7 @@ describe('Session index', () => { name: indexTemplateName, }); expect(mockElasticsearchClient.indices.exists).toHaveBeenCalledWith({ - index: getSessionIndexTemplate(indexTemplateName, indexName).index_patterns[0], + index: getSessionIndexSettings(indexName).index, }); } @@ -88,71 +88,71 @@ describe('Session index', () => { expect(mockElasticsearchClient.indices.create).not.toHaveBeenCalled(); }); - it('deletes legacy index template if needed and creates both index template and index if they do not exist', async () => { + it('deletes legacy index template if needed and creates index if it does not exist', async () => { mockElasticsearchClient.indices.existsTemplate.mockResponse(true); mockElasticsearchClient.indices.existsIndexTemplate.mockResponse(false); mockElasticsearchClient.indices.exists.mockResponse(false); await sessionIndex.initialize(); - const expectedIndexTemplate = getSessionIndexTemplate(indexTemplateName, indexName); assertExistenceChecksPerformed(); expect(mockElasticsearchClient.indices.deleteTemplate).toHaveBeenCalledWith({ name: indexTemplateName, }); - expect(mockElasticsearchClient.indices.putIndexTemplate).toHaveBeenCalledWith( - expectedIndexTemplate + expect(mockElasticsearchClient.indices.create).toHaveBeenCalledWith( + getSessionIndexSettings(indexName) ); - expect(mockElasticsearchClient.indices.create).toHaveBeenCalledWith({ - index: expectedIndexTemplate.index_patterns[0], - }); }); - it('creates both index template and index if they do not exist', async () => { - mockElasticsearchClient.indices.existsTemplate.mockResponse(false); - mockElasticsearchClient.indices.existsIndexTemplate.mockResponse(false); + it('deletes legacy & modern index templates if needed and creates index if it does not exist', async () => { + mockElasticsearchClient.indices.existsTemplate.mockResponse(true); + mockElasticsearchClient.indices.existsIndexTemplate.mockResponse(true); mockElasticsearchClient.indices.exists.mockResponse(false); await sessionIndex.initialize(); - const expectedIndexTemplate = getSessionIndexTemplate(indexTemplateName, indexName); assertExistenceChecksPerformed(); - expect(mockElasticsearchClient.indices.deleteTemplate).not.toHaveBeenCalled(); - expect(mockElasticsearchClient.indices.putIndexTemplate).toHaveBeenCalledWith( - expectedIndexTemplate - ); - expect(mockElasticsearchClient.indices.create).toHaveBeenCalledWith({ - index: expectedIndexTemplate.index_patterns[0], + expect(mockElasticsearchClient.indices.deleteTemplate).toHaveBeenCalledWith({ + name: indexTemplateName, }); + expect(mockElasticsearchClient.indices.deleteIndexTemplate).toHaveBeenCalledWith({ + name: indexTemplateName, + }); + expect(mockElasticsearchClient.indices.create).toHaveBeenCalledWith( + getSessionIndexSettings(indexName) + ); }); - it('creates only index template if it does not exist even if index exists', async () => { + it('deletes modern index template if needed and creates index if it does not exist', async () => { mockElasticsearchClient.indices.existsTemplate.mockResponse(false); - mockElasticsearchClient.indices.existsIndexTemplate.mockResponse(false); - mockElasticsearchClient.indices.exists.mockResponse(true); + mockElasticsearchClient.indices.existsIndexTemplate.mockResponse(true); + mockElasticsearchClient.indices.exists.mockResponse(false); await sessionIndex.initialize(); assertExistenceChecksPerformed(); expect(mockElasticsearchClient.indices.deleteTemplate).not.toHaveBeenCalled(); - expect(mockElasticsearchClient.indices.putIndexTemplate).toHaveBeenCalledWith( - getSessionIndexTemplate(indexTemplateName, indexName) + expect(mockElasticsearchClient.indices.deleteIndexTemplate).toHaveBeenCalledWith({ + name: indexTemplateName, + }); + expect(mockElasticsearchClient.indices.create).toHaveBeenCalledWith( + getSessionIndexSettings(indexName) ); }); - it('creates only index if it does not exist even if index template exists', async () => { + it('creates index if it does not exist', async () => { mockElasticsearchClient.indices.existsTemplate.mockResponse(false); - mockElasticsearchClient.indices.existsIndexTemplate.mockResponse(true); + mockElasticsearchClient.indices.existsIndexTemplate.mockResponse(false); mockElasticsearchClient.indices.exists.mockResponse(false); await sessionIndex.initialize(); assertExistenceChecksPerformed(); expect(mockElasticsearchClient.indices.deleteTemplate).not.toHaveBeenCalled(); - expect(mockElasticsearchClient.indices.putIndexTemplate).not.toHaveBeenCalled(); - expect(mockElasticsearchClient.indices.create).toHaveBeenCalledWith({ - index: getSessionIndexTemplate(indexTemplateName, indexName).index_patterns[0], - }); + expect(mockElasticsearchClient.indices.deleteIndexTemplate).not.toHaveBeenCalled(); + expect(mockElasticsearchClient.indices.create).toHaveBeenCalledWith( + getSessionIndexSettings(indexName) + ); }); it('does not fail if tries to create index when it exists already', async () => { diff --git a/x-pack/plugins/security/server/session_management/session_index.ts b/x-pack/plugins/security/server/session_management/session_index.ts index 1103b9bb9c833..768226efbf4f9 100644 --- a/x-pack/plugins/security/server/session_management/session_index.ts +++ b/x-pack/plugins/security/server/session_management/session_index.ts @@ -59,35 +59,32 @@ const SESSION_INDEX_CLEANUP_BATCH_LIMIT = 10; const SESSION_INDEX_CLEANUP_KEEP_ALIVE = '5m'; /** - * Returns index template that is used for the current version of the session index. + * Returns index settings that are used for the current version of the session index. */ -export function getSessionIndexTemplate(templateName: string, indexName: string) { +export function getSessionIndexSettings(indexName: string) { return Object.freeze({ - name: templateName, - index_patterns: [indexName], - template: { - settings: { - index: { - number_of_shards: 1, - number_of_replicas: 0, - auto_expand_replicas: '0-1', - priority: 1000, - refresh_interval: '1s', - hidden: true, - }, + index: indexName, + settings: { + index: { + number_of_shards: 1, + number_of_replicas: 0, + auto_expand_replicas: '0-1', + priority: 1000, + refresh_interval: '1s', + hidden: true, }, - mappings: { - dynamic: 'strict', - properties: { - usernameHash: { type: 'keyword' }, - provider: { properties: { name: { type: 'keyword' }, type: { type: 'keyword' } } }, - idleTimeoutExpiration: { type: 'date' }, - lifespanExpiration: { type: 'date' }, - accessAgreementAcknowledged: { type: 'boolean' }, - content: { type: 'binary' }, - }, - } as const, }, + mappings: { + dynamic: 'strict', + properties: { + usernameHash: { type: 'keyword' }, + provider: { properties: { name: { type: 'keyword' }, type: { type: 'keyword' } } }, + idleTimeoutExpiration: { type: 'date' }, + lifespanExpiration: { type: 'date' }, + accessAgreementAcknowledged: { type: 'boolean' }, + content: { type: 'binary' }, + }, + } as const, }); } @@ -372,7 +369,7 @@ export class SessionIndex { } } - // Check if required index template exists. + // Check if index template exists. let indexTemplateExists = false; try { indexTemplateExists = await this.options.elasticsearchClient.indices.existsIndexTemplate({ @@ -385,17 +382,16 @@ export class SessionIndex { return reject(err); } - // Create index template if it doesn't exist. + // Delete index template if it does. if (indexTemplateExists) { - this.options.logger.debug('Session index template already exists.'); - } else { + this.options.logger.debug('Deleting unused session index template.'); try { - await this.options.elasticsearchClient.indices.putIndexTemplate( - getSessionIndexTemplate(sessionIndexTemplateName, this.indexName) - ); - this.options.logger.debug('Successfully created session index template.'); + await this.options.elasticsearchClient.indices.deleteIndexTemplate({ + name: sessionIndexTemplateName, + }); + this.options.logger.debug('Successfully deleted session index template.'); } catch (err) { - this.options.logger.error(`Failed to create session index template: ${err.message}`); + this.options.logger.error(`Failed to delete session index template: ${err.message}`); return reject(err); } } @@ -417,7 +413,9 @@ export class SessionIndex { this.options.logger.debug('Session index already exists.'); } else { try { - await this.options.elasticsearchClient.indices.create({ index: this.indexName }); + await this.options.elasticsearchClient.indices.create( + getSessionIndexSettings(this.indexName) + ); this.options.logger.debug('Successfully created session index.'); } catch (err) { // There can be a race condition if index is created by another Kibana instance. From 10c0ed1e37fba85e62f93f32413ca6c17ae619ed Mon Sep 17 00:00:00 2001 From: Larry Gregory Date: Wed, 22 Jun 2022 11:33:34 -0400 Subject: [PATCH 02/11] Attempt to ensure session index exists before creating session document --- .../session_management/session_index.test.ts | 47 +++++++++- .../session_management/session_index.ts | 87 ++++++++++--------- 2 files changed, 92 insertions(+), 42 deletions(-) diff --git a/x-pack/plugins/security/server/session_management/session_index.test.ts b/x-pack/plugins/security/server/session_management/session_index.test.ts index 55b708bb6238f..4263555b05ac2 100644 --- a/x-pack/plugins/security/server/session_management/session_index.test.ts +++ b/x-pack/plugins/security/server/session_management/session_index.test.ts @@ -862,7 +862,49 @@ describe('Session index', () => { ).rejects.toBe(failureReason); }); - it('properly stores session value in the index', async () => { + it('properly stores session value in the index, creating the index first if it does not exist', async () => { + mockElasticsearchClient.indices.exists.mockResolvedValue(false); + + mockElasticsearchClient.create.mockResponse({ + _shards: { total: 1, failed: 0, successful: 1, skipped: 0 }, + _index: 'my-index', + _id: 'W0tpsmIBdwcYyG50zbta', + _version: 1, + _primary_term: 321, + _seq_no: 654, + result: 'created', + }); + + const sid = 'some-long-sid'; + const sessionValue = { + usernameHash: 'some-username-hash', + provider: { type: 'basic', name: 'basic1' }, + idleTimeoutExpiration: null, + lifespanExpiration: null, + content: 'some-encrypted-content', + }; + + await expect(sessionIndex.create({ sid, ...sessionValue })).resolves.toEqual({ + ...sessionValue, + sid, + metadata: { primaryTerm: 321, sequenceNumber: 654 }, + }); + + expect(mockElasticsearchClient.indices.exists).toHaveBeenCalledTimes(1); + expect(mockElasticsearchClient.indices.create).toHaveBeenCalledTimes(1); + + expect(mockElasticsearchClient.create).toHaveBeenCalledTimes(1); + expect(mockElasticsearchClient.create).toHaveBeenCalledWith({ + id: sid, + index: indexName, + body: sessionValue, + refresh: 'wait_for', + }); + }); + + it('properly stores session value in the index, skipping index creation if it already exists', async () => { + mockElasticsearchClient.indices.exists.mockResolvedValue(true); + mockElasticsearchClient.create.mockResponse({ _shards: { total: 1, failed: 0, successful: 1, skipped: 0 }, _index: 'my-index', @@ -888,6 +930,9 @@ describe('Session index', () => { metadata: { primaryTerm: 321, sequenceNumber: 654 }, }); + expect(mockElasticsearchClient.indices.exists).toHaveBeenCalledTimes(1); + expect(mockElasticsearchClient.indices.create).not.toHaveBeenCalled(); + expect(mockElasticsearchClient.create).toHaveBeenCalledTimes(1); expect(mockElasticsearchClient.create).toHaveBeenCalledWith({ id: sid, diff --git a/x-pack/plugins/security/server/session_management/session_index.ts b/x-pack/plugins/security/server/session_management/session_index.ts index 768226efbf4f9..1111c77530224 100644 --- a/x-pack/plugins/security/server/session_management/session_index.ts +++ b/x-pack/plugins/security/server/session_management/session_index.ts @@ -5,6 +5,7 @@ * 2.0. */ +import type { IndicesCreateRequest } from '@elastic/elasticsearch/lib/api/types'; import type { BulkOperationContainer, SortResults, @@ -61,18 +62,16 @@ const SESSION_INDEX_CLEANUP_KEEP_ALIVE = '5m'; /** * Returns index settings that are used for the current version of the session index. */ -export function getSessionIndexSettings(indexName: string) { +export function getSessionIndexSettings(indexName: string): IndicesCreateRequest { return Object.freeze({ index: indexName, settings: { - index: { - number_of_shards: 1, - number_of_replicas: 0, - auto_expand_replicas: '0-1', - priority: 1000, - refresh_interval: '1s', - hidden: true, - }, + number_of_shards: 1, + number_of_replicas: 0, + auto_expand_replicas: '0-1', + priority: 1000, + refresh_interval: '1s', + hidden: true, }, mappings: { dynamic: 'strict', @@ -84,7 +83,7 @@ export function getSessionIndexSettings(indexName: string) { accessAgreementAcknowledged: { type: 'boolean' }, content: { type: 'binary' }, }, - } as const, + }, }); } @@ -210,6 +209,8 @@ export class SessionIndex { 'Attempted to create a new session while session index is initializing.' ); await this.indexInitialization; + } else { + await this.ensureSessionIndexExists(); } const { sid, ...sessionValueToStore } = sessionValue; @@ -396,37 +397,7 @@ export class SessionIndex { } } - // Check if required index exists. We cannot be sure that automatic creation of indices is - // always enabled, so we create session index explicitly. - let indexExists = false; - try { - indexExists = await this.options.elasticsearchClient.indices.exists({ - index: this.indexName, - }); - } catch (err) { - this.options.logger.error(`Failed to check if session index exists: ${err.message}`); - return reject(err); - } - - // Create index if it doesn't exist. - if (indexExists) { - this.options.logger.debug('Session index already exists.'); - } else { - try { - await this.options.elasticsearchClient.indices.create( - getSessionIndexSettings(this.indexName) - ); - this.options.logger.debug('Successfully created session index.'); - } catch (err) { - // There can be a race condition if index is created by another Kibana instance. - if (err?.body?.error?.type === 'resource_already_exists_exception') { - this.options.logger.debug('Session index already exists.'); - } else { - this.options.logger.error(`Failed to create session index: ${err.message}`); - return reject(err); - } - } - } + await this.ensureSessionIndexExists(); // Notify any consumers that are awaiting on this promise and immediately reset it. resolve(); @@ -507,6 +478,40 @@ export class SessionIndex { } } + private async ensureSessionIndexExists() { + // Check if required index exists. We cannot be sure that automatic creation of indices is + // always enabled, so we create session index explicitly. + let indexExists = false; + try { + indexExists = await this.options.elasticsearchClient.indices.exists({ + index: this.indexName, + }); + } catch (err) { + this.options.logger.error(`Failed to check if session index exists: ${err.message}`); + throw err; + } + + // Create index if it doesn't exist. + if (indexExists) { + this.options.logger.debug('Session index already exists.'); + } else { + try { + await this.options.elasticsearchClient.indices.create( + getSessionIndexSettings(this.indexName) + ); + this.options.logger.debug('Successfully created session index.'); + } catch (err) { + // There can be a race condition if index is created by another Kibana instance. + if (err?.body?.error?.type === 'resource_already_exists_exception') { + this.options.logger.debug('Session index already exists.'); + } else { + this.options.logger.error(`Failed to create session index: ${err.message}`); + throw err; + } + } + } + } + /** * Fetches session values from session index in batches of 10,000. */ From 667c565fcee2a0d50ed24f169b7a9520500c7b4a Mon Sep 17 00:00:00 2001 From: Larry Gregory Date: Wed, 22 Jun 2022 11:44:45 -0400 Subject: [PATCH 03/11] improved comments --- .../session_management/session_index.ts | 25 ++++++++++++++----- 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/x-pack/plugins/security/server/session_management/session_index.ts b/x-pack/plugins/security/server/session_management/session_index.ts index 1111c77530224..4bf160bcf46af 100644 --- a/x-pack/plugins/security/server/session_management/session_index.ts +++ b/x-pack/plugins/security/server/session_management/session_index.ts @@ -213,15 +213,26 @@ export class SessionIndex { await this.ensureSessionIndexExists(); } + // ************************************************** + // !! There is potential for a race condition here !! + // ************************************************** + // Prior to https://github.com/elastic/kibana/pull/134900, we maintained an index template with + // our desired settings and mappings for this session index. This allowed our index to almost + // always have the correct settings/mappings, even if the index was auto-created by this step. + // Now that the template is removed, we run the risk of the index being created without our desired + // settings/mappings, because they can't be specified as part of this `create` operation. + // + // The call to `this.ensureSessionIndexExists()` above attempts to mitigate this by creating the session index + // explicitly with our desired settings/mappings. A race condition exists because it's possible to delete the session index + // _after_ we've ensured it exists, but _before_ we make the call below to store the session document. + // + // The chances of this happening are very small. + const { sid, ...sessionValueToStore } = sessionValue; try { const { _primary_term: primaryTerm, _seq_no: sequenceNumber } = await this.options.elasticsearchClient.create({ id: sid, - // We cannot control whether index is created automatically during this operation or not. - // But we can reduce probability of getting into a weird state when session is being created - // while session index is missing for some reason. This way we'll recreate index with a - // proper name and alias. But this will only work if we still have a proper index template. index: this.indexName, body: sessionValueToStore, refresh: 'wait_for', @@ -478,9 +489,11 @@ export class SessionIndex { } } + /** + * Creates the session index if it doesn't already exist. + */ private async ensureSessionIndexExists() { - // Check if required index exists. We cannot be sure that automatic creation of indices is - // always enabled, so we create session index explicitly. + // Check if required index exists. let indexExists = false; try { indexExists = await this.options.elasticsearchClient.indices.exists({ From b890529164e49a570e374318dbda7e742b91a6bd Mon Sep 17 00:00:00 2001 From: Larry Gregory Date: Thu, 23 Jun 2022 09:13:16 -0400 Subject: [PATCH 04/11] Attempt to use index alias + require_alias --- .../session_management/session_index.test.ts | 87 ++++++++++------ .../session_management/session_index.ts | 98 +++++++++++++------ 2 files changed, 127 insertions(+), 58 deletions(-) diff --git a/x-pack/plugins/security/server/session_management/session_index.test.ts b/x-pack/plugins/security/server/session_management/session_index.test.ts index 4263555b05ac2..7a10392047d78 100644 --- a/x-pack/plugins/security/server/session_management/session_index.test.ts +++ b/x-pack/plugins/security/server/session_management/session_index.test.ts @@ -29,6 +29,7 @@ describe('Session index', () => { let sessionIndex: SessionIndex; let auditLogger: AuditLogger; const indexName = '.kibana_some_tenant_security_session_1'; + const aliasName = '.kibana_some_tenant_security_session'; const indexTemplateName = '.kibana_some_tenant_security_session_index_template_1'; beforeEach(() => { mockElasticsearchClient = elasticsearchServiceMock.createElasticsearchClient(); @@ -55,7 +56,7 @@ describe('Session index', () => { name: indexTemplateName, }); expect(mockElasticsearchClient.indices.exists).toHaveBeenCalledWith({ - index: getSessionIndexSettings(indexName).index, + index: getSessionIndexSettings({ indexName, aliasName }).index, }); } @@ -100,7 +101,7 @@ describe('Session index', () => { name: indexTemplateName, }); expect(mockElasticsearchClient.indices.create).toHaveBeenCalledWith( - getSessionIndexSettings(indexName) + getSessionIndexSettings({ indexName, aliasName }) ); }); @@ -119,7 +120,7 @@ describe('Session index', () => { name: indexTemplateName, }); expect(mockElasticsearchClient.indices.create).toHaveBeenCalledWith( - getSessionIndexSettings(indexName) + getSessionIndexSettings({ indexName, aliasName }) ); }); @@ -136,7 +137,7 @@ describe('Session index', () => { name: indexTemplateName, }); expect(mockElasticsearchClient.indices.create).toHaveBeenCalledWith( - getSessionIndexSettings(indexName) + getSessionIndexSettings({ indexName, aliasName }) ); }); @@ -151,7 +152,7 @@ describe('Session index', () => { expect(mockElasticsearchClient.indices.deleteTemplate).not.toHaveBeenCalled(); expect(mockElasticsearchClient.indices.deleteIndexTemplate).not.toHaveBeenCalled(); expect(mockElasticsearchClient.indices.create).toHaveBeenCalledWith( - getSessionIndexSettings(indexName) + getSessionIndexSettings({ indexName, aliasName }) ); }); @@ -863,16 +864,26 @@ describe('Session index', () => { }); it('properly stores session value in the index, creating the index first if it does not exist', async () => { - mockElasticsearchClient.indices.exists.mockResolvedValue(false); + mockElasticsearchClient.indices.exists.mockResponse(false); - mockElasticsearchClient.create.mockResponse({ - _shards: { total: 1, failed: 0, successful: 1, skipped: 0 }, - _index: 'my-index', - _id: 'W0tpsmIBdwcYyG50zbta', - _version: 1, - _primary_term: 321, - _seq_no: 654, - result: 'created', + let callCount = 0; + mockElasticsearchClient.create.mockResponseImplementation((req, opts) => { + callCount++; + if (callCount === 1) { + return { statusCode: 404 }; + } + return { + body: { + _shards: { total: 1, failed: 0, successful: 1, skipped: 0 }, + _index: 'my-index', + _id: 'W0tpsmIBdwcYyG50zbta', + _version: 1, + _primary_term: 321, + _seq_no: 654, + result: 'created', + }, + statusCode: 201, + }; }); const sid = 'some-long-sid'; @@ -893,13 +904,29 @@ describe('Session index', () => { expect(mockElasticsearchClient.indices.exists).toHaveBeenCalledTimes(1); expect(mockElasticsearchClient.indices.create).toHaveBeenCalledTimes(1); - expect(mockElasticsearchClient.create).toHaveBeenCalledTimes(1); - expect(mockElasticsearchClient.create).toHaveBeenCalledWith({ - id: sid, - index: indexName, - body: sessionValue, - refresh: 'wait_for', - }); + expect(mockElasticsearchClient.create).toHaveBeenCalledTimes(2); + expect(mockElasticsearchClient.create).toHaveBeenNthCalledWith( + 1, + { + id: sid, + index: aliasName, + body: sessionValue, + refresh: 'wait_for', + require_alias: true, + }, + { ignore: [404], meta: true } + ); + expect(mockElasticsearchClient.create).toHaveBeenNthCalledWith( + 2, + { + id: sid, + index: aliasName, + body: sessionValue, + refresh: 'wait_for', + require_alias: true, + }, + { ignore: [], meta: true } + ); }); it('properly stores session value in the index, skipping index creation if it already exists', async () => { @@ -930,16 +957,20 @@ describe('Session index', () => { metadata: { primaryTerm: 321, sequenceNumber: 654 }, }); - expect(mockElasticsearchClient.indices.exists).toHaveBeenCalledTimes(1); + expect(mockElasticsearchClient.indices.exists).not.toHaveBeenCalled(); expect(mockElasticsearchClient.indices.create).not.toHaveBeenCalled(); expect(mockElasticsearchClient.create).toHaveBeenCalledTimes(1); - expect(mockElasticsearchClient.create).toHaveBeenCalledWith({ - id: sid, - index: indexName, - body: sessionValue, - refresh: 'wait_for', - }); + expect(mockElasticsearchClient.create).toHaveBeenCalledWith( + { + id: sid, + index: aliasName, + body: sessionValue, + refresh: 'wait_for', + require_alias: true, + }, + { meta: true, ignore: [404] } + ); }); }); diff --git a/x-pack/plugins/security/server/session_management/session_index.ts b/x-pack/plugins/security/server/session_management/session_index.ts index 4bf160bcf46af..1a599f3d7dfa8 100644 --- a/x-pack/plugins/security/server/session_management/session_index.ts +++ b/x-pack/plugins/security/server/session_management/session_index.ts @@ -5,7 +5,7 @@ * 2.0. */ -import type { IndicesCreateRequest } from '@elastic/elasticsearch/lib/api/types'; +import type { CreateRequest, IndicesCreateRequest } from '@elastic/elasticsearch/lib/api/types'; import type { BulkOperationContainer, SortResults, @@ -62,7 +62,13 @@ const SESSION_INDEX_CLEANUP_KEEP_ALIVE = '5m'; /** * Returns index settings that are used for the current version of the session index. */ -export function getSessionIndexSettings(indexName: string): IndicesCreateRequest { +export function getSessionIndexSettings({ + indexName, + aliasName, +}: { + indexName: string; + aliasName: string; +}): IndicesCreateRequest { return Object.freeze({ index: indexName, settings: { @@ -73,6 +79,11 @@ export function getSessionIndexSettings(indexName: string): IndicesCreateRequest refresh_interval: '1s', hidden: true, }, + aliases: { + [aliasName]: { + is_write_index: true, + }, + }, mappings: { dynamic: 'strict', properties: { @@ -156,6 +167,11 @@ export class SessionIndex { */ private readonly indexName: string; + /** + * Name of the write alias to store session information in. + */ + private readonly aliasName: string; + /** * Promise that tracks session index initialization process. We'll need to get rid of this as soon * as Core provides support for plugin statuses (https://github.com/elastic/kibana/issues/41983). @@ -166,6 +182,7 @@ export class SessionIndex { constructor(private readonly options: Readonly) { this.indexName = `${this.options.kibanaIndexName}_security_session_${SESSION_INDEX_TEMPLATE_VERSION}`; + this.aliasName = `${this.options.kibanaIndexName}_security_session`; } /** @@ -209,36 +226,27 @@ export class SessionIndex { 'Attempted to create a new session while session index is initializing.' ); await this.indexInitialization; - } else { - await this.ensureSessionIndexExists(); } - // ************************************************** - // !! There is potential for a race condition here !! - // ************************************************** - // Prior to https://github.com/elastic/kibana/pull/134900, we maintained an index template with - // our desired settings and mappings for this session index. This allowed our index to almost - // always have the correct settings/mappings, even if the index was auto-created by this step. - // Now that the template is removed, we run the risk of the index being created without our desired - // settings/mappings, because they can't be specified as part of this `create` operation. - // - // The call to `this.ensureSessionIndexExists()` above attempts to mitigate this by creating the session index - // explicitly with our desired settings/mappings. A race condition exists because it's possible to delete the session index - // _after_ we've ensured it exists, but _before_ we make the call below to store the session document. - // - // The chances of this happening are very small. - - const { sid, ...sessionValueToStore } = sessionValue; try { - const { _primary_term: primaryTerm, _seq_no: sequenceNumber } = - await this.options.elasticsearchClient.create({ - id: sid, - index: this.indexName, - body: sessionValueToStore, - refresh: 'wait_for', - }); + let { body, statusCode } = await this.writeNewSessionDocument(sessionValue, { + ignore404: true, + }); - return { ...sessionValue, metadata: { primaryTerm, sequenceNumber } } as SessionIndexValue; + if (statusCode === 404) { + this.options.logger.warn( + 'Attempted to create a new session, but session index or alias was missing.' + ); + await this.ensureSessionIndexExists(); + ({ body, statusCode } = await this.writeNewSessionDocument(sessionValue, { + ignore404: false, + })); + } + + return { + ...sessionValue, + metadata: { primaryTerm: body._primary_term, sequenceNumber: body._seq_no }, + } as SessionIndexValue; } catch (err) { this.options.logger.error(`Failed to create session value: ${err.message}`); throw err; @@ -506,11 +514,20 @@ export class SessionIndex { // Create index if it doesn't exist. if (indexExists) { - this.options.logger.debug('Session index already exists.'); + this.options.logger.debug('Session index already exists. Attaching alias to index...'); + try { + await this.options.elasticsearchClient.indices.putAlias({ + index: this.indexName, + name: this.aliasName, + }); + } catch (err) { + this.options.logger.error(`Failed to attach alias to session index: ${err.message}`); + throw err; + } } else { try { await this.options.elasticsearchClient.indices.create( - getSessionIndexSettings(this.indexName) + getSessionIndexSettings({ indexName: this.indexName, aliasName: this.aliasName }) ); this.options.logger.debug('Successfully created session index.'); } catch (err) { @@ -525,6 +542,27 @@ export class SessionIndex { } } + private async writeNewSessionDocument( + sessionValue: Readonly>, + { ignore404 }: { ignore404: boolean } + ) { + const { sid, ...sessionValueToStore } = sessionValue; + + const { body, statusCode } = await this.options.elasticsearchClient.create( + { + id: sid, + // We write to the alias for `create` operations so that we can prevent index auto-creation in the event it is missing. + index: this.aliasName, + body: sessionValueToStore, + refresh: 'wait_for', + require_alias: true, + } as CreateRequest, + { meta: true, ignore: ignore404 ? [404] : [] } + ); + + return { body, statusCode }; + } + /** * Fetches session values from session index in batches of 10,000. */ From 47f3393174b2dc88331aee507906038093bc689f Mon Sep 17 00:00:00 2001 From: Larry Gregory Date: Thu, 23 Jun 2022 13:02:20 -0400 Subject: [PATCH 05/11] more docs and testing --- .../session_management/session_index.test.ts | 29 +++++++++++++++++++ .../session_management/session_index.ts | 9 +++++- 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/x-pack/plugins/security/server/session_management/session_index.test.ts b/x-pack/plugins/security/server/session_management/session_index.test.ts index 7a10392047d78..ba6aae97f93a0 100644 --- a/x-pack/plugins/security/server/session_management/session_index.test.ts +++ b/x-pack/plugins/security/server/session_management/session_index.test.ts @@ -86,6 +86,7 @@ describe('Session index', () => { expect(mockElasticsearchClient.indices.deleteTemplate).not.toHaveBeenCalled(); expect(mockElasticsearchClient.indices.putIndexTemplate).not.toHaveBeenCalled(); + expect(mockElasticsearchClient.indices.putAlias).toHaveBeenCalledTimes(1); expect(mockElasticsearchClient.indices.create).not.toHaveBeenCalled(); }); @@ -103,6 +104,7 @@ describe('Session index', () => { expect(mockElasticsearchClient.indices.create).toHaveBeenCalledWith( getSessionIndexSettings({ indexName, aliasName }) ); + expect(mockElasticsearchClient.indices.putAlias).not.toHaveBeenCalled(); }); it('deletes legacy & modern index templates if needed and creates index if it does not exist', async () => { @@ -122,6 +124,7 @@ describe('Session index', () => { expect(mockElasticsearchClient.indices.create).toHaveBeenCalledWith( getSessionIndexSettings({ indexName, aliasName }) ); + expect(mockElasticsearchClient.indices.putAlias).not.toHaveBeenCalled(); }); it('deletes modern index template if needed and creates index if it does not exist', async () => { @@ -136,11 +139,32 @@ describe('Session index', () => { expect(mockElasticsearchClient.indices.deleteIndexTemplate).toHaveBeenCalledWith({ name: indexTemplateName, }); + expect(mockElasticsearchClient.indices.putAlias).not.toHaveBeenCalled(); expect(mockElasticsearchClient.indices.create).toHaveBeenCalledWith( getSessionIndexSettings({ indexName, aliasName }) ); }); + it('attaches an alias to the index if the index already exists', async () => { + mockElasticsearchClient.indices.existsTemplate.mockResponse(false); + mockElasticsearchClient.indices.existsIndexTemplate.mockResponse(false); + mockElasticsearchClient.indices.exists.mockResponse(true); + + await sessionIndex.initialize(); + + assertExistenceChecksPerformed(); + + expect(mockElasticsearchClient.indices.deleteTemplate).not.toHaveBeenCalled(); + expect(mockElasticsearchClient.indices.deleteIndexTemplate).not.toHaveBeenCalled(); + expect(mockElasticsearchClient.indices.create).not.toHaveBeenCalled(); + + expect(mockElasticsearchClient.indices.putAlias).toHaveBeenCalledTimes(1); + expect(mockElasticsearchClient.indices.putAlias).toHaveBeenCalledWith({ + index: indexName, + name: aliasName, + }); + }); + it('creates index if it does not exist', async () => { mockElasticsearchClient.indices.existsTemplate.mockResponse(false); mockElasticsearchClient.indices.existsIndexTemplate.mockResponse(false); @@ -151,6 +175,7 @@ describe('Session index', () => { assertExistenceChecksPerformed(); expect(mockElasticsearchClient.indices.deleteTemplate).not.toHaveBeenCalled(); expect(mockElasticsearchClient.indices.deleteIndexTemplate).not.toHaveBeenCalled(); + expect(mockElasticsearchClient.indices.putAlias).not.toHaveBeenCalled(); expect(mockElasticsearchClient.indices.create).toHaveBeenCalledWith( getSessionIndexSettings({ indexName, aliasName }) ); @@ -869,9 +894,11 @@ describe('Session index', () => { let callCount = 0; mockElasticsearchClient.create.mockResponseImplementation((req, opts) => { callCount++; + // Fail the first create attempt because the index/alias doesn't exist if (callCount === 1) { return { statusCode: 404 }; } + // Pass the second create attempt return { body: { _shards: { total: 1, failed: 0, successful: 1, skipped: 0 }, @@ -903,6 +930,7 @@ describe('Session index', () => { expect(mockElasticsearchClient.indices.exists).toHaveBeenCalledTimes(1); expect(mockElasticsearchClient.indices.create).toHaveBeenCalledTimes(1); + expect(mockElasticsearchClient.indices.putAlias).not.toHaveBeenCalled(); expect(mockElasticsearchClient.create).toHaveBeenCalledTimes(2); expect(mockElasticsearchClient.create).toHaveBeenNthCalledWith( @@ -959,6 +987,7 @@ describe('Session index', () => { expect(mockElasticsearchClient.indices.exists).not.toHaveBeenCalled(); expect(mockElasticsearchClient.indices.create).not.toHaveBeenCalled(); + expect(mockElasticsearchClient.indices.putAlias).not.toHaveBeenCalled(); expect(mockElasticsearchClient.create).toHaveBeenCalledTimes(1); expect(mockElasticsearchClient.create).toHaveBeenCalledWith( diff --git a/x-pack/plugins/security/server/session_management/session_index.ts b/x-pack/plugins/security/server/session_management/session_index.ts index 1a599f3d7dfa8..718e5ea5f3a34 100644 --- a/x-pack/plugins/security/server/session_management/session_index.ts +++ b/x-pack/plugins/security/server/session_management/session_index.ts @@ -512,9 +512,16 @@ export class SessionIndex { throw err; } - // Create index if it doesn't exist. + // Initialize session index: + // Ensure the alias is attached to the already existing index, + // or create a new index if it doesn't exist. if (indexExists) { this.options.logger.debug('Session index already exists. Attaching alias to index...'); + + // Prior to https://github.com/elastic/kibana/pull/134900, sessions would be written directly against the session index. + // Now, we write sessions against a new session index alias. This call ensures that the alias exists, and is attached to the index. + // This operation is safe to repeat, even if the alias already exists. This seems safer than retrieving the index details, and inspecting + // it to see if the alias already exists. try { await this.options.elasticsearchClient.indices.putAlias({ index: this.indexName, From 3e34e6ab1fbf8b4e7280539a4c49d75efd9fad32 Mon Sep 17 00:00:00 2001 From: Larry Gregory Date: Tue, 28 Jun 2022 08:01:34 -0400 Subject: [PATCH 06/11] Apply suggestions from code review Co-authored-by: Aleh Zasypkin --- .../security/server/session_management/session_index.test.ts | 2 +- .../plugins/security/server/session_management/session_index.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/x-pack/plugins/security/server/session_management/session_index.test.ts b/x-pack/plugins/security/server/session_management/session_index.test.ts index ba6aae97f93a0..3d0269e701b17 100644 --- a/x-pack/plugins/security/server/session_management/session_index.test.ts +++ b/x-pack/plugins/security/server/session_management/session_index.test.ts @@ -892,7 +892,7 @@ describe('Session index', () => { mockElasticsearchClient.indices.exists.mockResponse(false); let callCount = 0; - mockElasticsearchClient.create.mockResponseImplementation((req, opts) => { + mockElasticsearchClient.create.mockResponseImplementation(() => { callCount++; // Fail the first create attempt because the index/alias doesn't exist if (callCount === 1) { diff --git a/x-pack/plugins/security/server/session_management/session_index.ts b/x-pack/plugins/security/server/session_management/session_index.ts index 718e5ea5f3a34..ce0ac590b92ab 100644 --- a/x-pack/plugins/security/server/session_management/session_index.ts +++ b/x-pack/plugins/security/server/session_management/session_index.ts @@ -238,7 +238,7 @@ export class SessionIndex { 'Attempted to create a new session, but session index or alias was missing.' ); await this.ensureSessionIndexExists(); - ({ body, statusCode } = await this.writeNewSessionDocument(sessionValue, { + ({ body } = await this.writeNewSessionDocument(sessionValue, { ignore404: false, })); } From 0b37e653cb8fb212f38f4978af2e2836584f5d4b Mon Sep 17 00:00:00 2001 From: Larry Gregory Date: Tue, 28 Jun 2022 08:27:54 -0400 Subject: [PATCH 07/11] update session via alias --- .../session_management/session_index.test.ts | 76 ++++++++++++++++++- .../session_management/session_index.ts | 51 +++++++++---- 2 files changed, 110 insertions(+), 17 deletions(-) diff --git a/x-pack/plugins/security/server/session_management/session_index.test.ts b/x-pack/plugins/security/server/session_management/session_index.test.ts index 3d0269e701b17..4e775cb454101 100644 --- a/x-pack/plugins/security/server/session_management/session_index.test.ts +++ b/x-pack/plugins/security/server/session_management/session_index.test.ts @@ -1064,13 +1064,14 @@ describe('Session index', () => { expect(mockElasticsearchClient.index).toHaveBeenCalledWith( { id: sid, - index: indexName, + index: aliasName, body: sessionValue, if_seq_no: 456, if_primary_term: 123, refresh: 'wait_for', + require_alias: true, }, - { ignore: [409], meta: true } + { ignore: [404, 409], meta: true } ); }); @@ -1105,11 +1106,80 @@ describe('Session index', () => { expect(mockElasticsearchClient.index).toHaveBeenCalledWith( { id: sid, - index: indexName, + index: aliasName, + body: sessionValue, + if_seq_no: 456, + if_primary_term: 123, + refresh: 'wait_for', + require_alias: true, + }, + { ignore: [404, 409], meta: true } + ); + }); + + it('properly stores session value in the index, recreating the index/alias if missing', async () => { + let callCount = 0; + mockElasticsearchClient.index.mockResponseImplementation(() => { + callCount++; + // Fail the first update attempt because the index/alias doesn't exist + if (callCount === 1) { + return { statusCode: 404 }; + } + // Pass the second update attempt + return { + body: { + _shards: { total: 1, failed: 0, successful: 1, skipped: 0 }, + _index: 'my-index', + _id: 'W0tpsmIBdwcYyG50zbta', + _version: 1, + _primary_term: 321, + _seq_no: 654, + result: 'created', + }, + statusCode: 201, + }; + }); + + const sid = 'some-long-sid'; + const metadata = { primaryTerm: 123, sequenceNumber: 456 }; + const sessionValue = { + usernameHash: 'some-username-hash', + provider: { type: 'basic', name: 'basic1' }, + idleTimeoutExpiration: null, + lifespanExpiration: null, + content: 'some-encrypted-content', + }; + + await expect(sessionIndex.update({ sid, metadata, ...sessionValue })).resolves.toEqual({ + ...sessionValue, + sid, + metadata: { primaryTerm: 321, sequenceNumber: 654 }, + }); + + expect(mockElasticsearchClient.index).toHaveBeenCalledTimes(2); + expect(mockElasticsearchClient.index).toHaveBeenNthCalledWith( + 1, + { + id: sid, + index: aliasName, + body: sessionValue, + if_seq_no: 456, + if_primary_term: 123, + refresh: 'wait_for', + require_alias: true, + }, + { ignore: [404, 409], meta: true } + ); + expect(mockElasticsearchClient.index).toHaveBeenNthCalledWith( + 2, + { + id: sid, + index: aliasName, body: sessionValue, if_seq_no: 456, if_primary_term: 123, refresh: 'wait_for', + require_alias: true, }, { ignore: [409], meta: true } ); diff --git a/x-pack/plugins/security/server/session_management/session_index.ts b/x-pack/plugins/security/server/session_management/session_index.ts index ce0ac590b92ab..c994b761865db 100644 --- a/x-pack/plugins/security/server/session_management/session_index.ts +++ b/x-pack/plugins/security/server/session_management/session_index.ts @@ -238,7 +238,7 @@ export class SessionIndex { 'Attempted to create a new session, but session index or alias was missing.' ); await this.ensureSessionIndexExists(); - ({ body } = await this.writeNewSessionDocument(sessionValue, { + ({ body, statusCode } = await this.writeNewSessionDocument(sessionValue, { ignore404: false, })); } @@ -258,19 +258,20 @@ export class SessionIndex { * @param sessionValue Session index value. */ async update(sessionValue: Readonly) { - const { sid, metadata, ...sessionValueToStore } = sessionValue; try { - const { body: response, statusCode } = await this.options.elasticsearchClient.index( - { - id: sid, - index: this.indexName, - body: sessionValueToStore, - if_seq_no: metadata.sequenceNumber, - if_primary_term: metadata.primaryTerm, - refresh: 'wait_for', - }, - { ignore: [409], meta: true } - ); + let { body: response, statusCode } = await this.updateExistingSessionDocument(sessionValue, { + ignore404: true, + }); + + if (statusCode === 404) { + this.options.logger.warn( + 'Attempted to update an existing session, but session index or alias was missing.' + ); + await this.ensureSessionIndexExists(); + ({ body: response, statusCode } = await this.updateExistingSessionDocument(sessionValue, { + ignore404: false, + })); + } // We don't want to override changes that were made after we fetched session value or // re-create it if has been deleted already. If we detect such a case we discard changes and @@ -280,7 +281,7 @@ export class SessionIndex { this.options.logger.debug( 'Cannot update session value due to conflict, session either does not exist or was already updated.' ); - return await this.get(sid); + return await this.get(sessionValue.sid); } return { @@ -570,6 +571,28 @@ export class SessionIndex { return { body, statusCode }; } + private async updateExistingSessionDocument( + sessionValue: Readonly, + { ignore404 }: { ignore404: boolean } + ) { + const { sid, metadata, ...sessionValueToStore } = sessionValue; + + const { body, statusCode } = await this.options.elasticsearchClient.index( + { + id: sid, + index: this.aliasName, + body: sessionValueToStore, + if_seq_no: metadata.sequenceNumber, + if_primary_term: metadata.primaryTerm, + refresh: 'wait_for', + require_alias: true, + }, + { ignore: ignore404 ? [404, 409] : [409], meta: true } + ); + + return { body, statusCode }; + } + /** * Fetches session values from session index in batches of 10,000. */ From a69acc629c93b6effbdd38e818bc783e351f7f09 Mon Sep 17 00:00:00 2001 From: Larry Gregory Date: Tue, 28 Jun 2022 08:28:49 -0400 Subject: [PATCH 08/11] get session via alias --- .../security/server/session_management/session_index.test.ts | 2 +- .../plugins/security/server/session_management/session_index.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/x-pack/plugins/security/server/session_management/session_index.test.ts b/x-pack/plugins/security/server/session_management/session_index.test.ts index 4e775cb454101..2d631d13a099d 100644 --- a/x-pack/plugins/security/server/session_management/session_index.test.ts +++ b/x-pack/plugins/security/server/session_management/session_index.test.ts @@ -863,7 +863,7 @@ describe('Session index', () => { expect(mockElasticsearchClient.get).toHaveBeenCalledTimes(1); expect(mockElasticsearchClient.get).toHaveBeenCalledWith( - { id: 'some-sid', index: indexName }, + { id: 'some-sid', index: aliasName }, { ignore: [404], meta: true } ); }); diff --git a/x-pack/plugins/security/server/session_management/session_index.ts b/x-pack/plugins/security/server/session_management/session_index.ts index c994b761865db..9b82d710731fa 100644 --- a/x-pack/plugins/security/server/session_management/session_index.ts +++ b/x-pack/plugins/security/server/session_management/session_index.ts @@ -194,7 +194,7 @@ export class SessionIndex { try { const { body: response, statusCode } = await this.options.elasticsearchClient.get( - { id: sid, index: this.indexName }, + { id: sid, index: this.aliasName }, { ignore: [404], meta: true } ); From 90406993c1fced90fc299dbac2081adb5f6fc873 Mon Sep 17 00:00:00 2001 From: Larry Gregory Date: Tue, 28 Jun 2022 08:58:29 -0400 Subject: [PATCH 09/11] invalidate session via alias --- .../server/session_management/session_index.test.ts | 12 ++++++------ .../server/session_management/session_index.ts | 4 ++-- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/x-pack/plugins/security/server/session_management/session_index.test.ts b/x-pack/plugins/security/server/session_management/session_index.test.ts index 2d631d13a099d..21acaa6f101c4 100644 --- a/x-pack/plugins/security/server/session_management/session_index.test.ts +++ b/x-pack/plugins/security/server/session_management/session_index.test.ts @@ -1207,7 +1207,7 @@ describe('Session index', () => { expect(mockElasticsearchClient.delete).toHaveBeenCalledTimes(1); expect(mockElasticsearchClient.delete).toHaveBeenCalledWith( - { id: 'some-long-sid', index: indexName, refresh: 'wait_for' }, + { id: 'some-long-sid', index: aliasName, refresh: 'wait_for' }, { ignore: [404], meta: true } ); }); @@ -1226,7 +1226,7 @@ describe('Session index', () => { expect(mockElasticsearchClient.deleteByQuery).toHaveBeenCalledTimes(1); expect(mockElasticsearchClient.deleteByQuery).toHaveBeenCalledWith({ - index: indexName, + index: aliasName, refresh: true, body: { query: { match_all: {} } }, }); @@ -1250,7 +1250,7 @@ describe('Session index', () => { expect(mockElasticsearchClient.deleteByQuery).toHaveBeenCalledTimes(1); expect(mockElasticsearchClient.deleteByQuery).toHaveBeenCalledWith({ - index: indexName, + index: aliasName, refresh: true, body: { query: { bool: { must: [{ term: { 'provider.type': 'basic' } }] } } }, }); @@ -1266,7 +1266,7 @@ describe('Session index', () => { expect(mockElasticsearchClient.deleteByQuery).toHaveBeenCalledTimes(1); expect(mockElasticsearchClient.deleteByQuery).toHaveBeenCalledWith({ - index: indexName, + index: aliasName, refresh: true, body: { query: { @@ -1291,7 +1291,7 @@ describe('Session index', () => { expect(mockElasticsearchClient.deleteByQuery).toHaveBeenCalledTimes(1); expect(mockElasticsearchClient.deleteByQuery).toHaveBeenCalledWith({ - index: indexName, + index: aliasName, refresh: true, body: { query: { @@ -1316,7 +1316,7 @@ describe('Session index', () => { expect(mockElasticsearchClient.deleteByQuery).toHaveBeenCalledTimes(1); expect(mockElasticsearchClient.deleteByQuery).toHaveBeenCalledWith({ - index: indexName, + index: aliasName, refresh: true, body: { query: { diff --git a/x-pack/plugins/security/server/session_management/session_index.ts b/x-pack/plugins/security/server/session_management/session_index.ts index 9b82d710731fa..0bb04cc706ce8 100644 --- a/x-pack/plugins/security/server/session_management/session_index.ts +++ b/x-pack/plugins/security/server/session_management/session_index.ts @@ -304,7 +304,7 @@ export class SessionIndex { // We don't specify primary term and sequence number as delete should always take precedence // over any updates that could happen in the meantime. const { statusCode } = await this.options.elasticsearchClient.delete( - { id: filter.sid, index: this.indexName, refresh: 'wait_for' }, + { id: filter.sid, index: this.aliasName, refresh: 'wait_for' }, { ignore: [404], meta: true } ); @@ -339,7 +339,7 @@ export class SessionIndex { try { const response = await this.options.elasticsearchClient.deleteByQuery({ - index: this.indexName, + index: this.aliasName, refresh: true, body: { query: deleteQuery }, }); From ed48c6a789901e15cd2c97122823e79866d24273 Mon Sep 17 00:00:00 2001 From: Larry Gregory Date: Wed, 29 Jun 2022 09:22:53 -0400 Subject: [PATCH 10/11] use alias in cleanup and getSessionValuesInBatches --- .../session_management/session_index.test.ts | 59 +++++++++++++++++-- .../session_management/session_index.ts | 28 +++++++-- 2 files changed, 77 insertions(+), 10 deletions(-) diff --git a/x-pack/plugins/security/server/session_management/session_index.test.ts b/x-pack/plugins/security/server/session_management/session_index.test.ts index 21acaa6f101c4..d0904b4e3cb48 100644 --- a/x-pack/plugins/security/server/session_management/session_index.test.ts +++ b/x-pack/plugins/security/server/session_management/session_index.test.ts @@ -270,6 +270,50 @@ describe('Session index', () => { expect(mockElasticsearchClient.closePointInTime).toHaveBeenCalledTimes(1); // since we attempted to delete sessions, we still refresh the index }); + it('creates the index/alias if missing', async () => { + mockElasticsearchClient.indices.exists.mockResponse(false); + + let callCount = 0; + mockElasticsearchClient.openPointInTime.mockResponseImplementation(() => { + callCount++; + if (callCount === 1) { + return { statusCode: 404 }; + } + + return { + body: { + id: 'PIT_ID', + } as OpenPointInTimeResponse, + }; + }); + + await sessionIndex.cleanUp(); + expect(mockElasticsearchClient.openPointInTime).toHaveBeenCalledTimes(2); + expect(mockElasticsearchClient.openPointInTime).toHaveBeenNthCalledWith( + 1, + { + index: aliasName, + keep_alive: '5m', + }, + { ignore: [404], meta: true } + ); + expect(mockElasticsearchClient.openPointInTime).toHaveBeenNthCalledWith( + 2, + { + index: aliasName, + keep_alive: '5m', + }, + { meta: true } + ); + + expect(mockElasticsearchClient.indices.exists).toHaveBeenCalledTimes(1); + expect(mockElasticsearchClient.indices.create).toHaveBeenCalledTimes(1); + expect(mockElasticsearchClient.indices.putAlias).not.toHaveBeenCalled(); + expect(mockElasticsearchClient.search).toHaveBeenCalledTimes(1); + expect(mockElasticsearchClient.bulk).toHaveBeenCalledTimes(1); + expect(mockElasticsearchClient.closePointInTime).toHaveBeenCalledTimes(1); // since we attempted to delete sessions, we still refresh the index + }); + it('when neither `lifespan` nor `idleTimeout` is configured', async () => { await sessionIndex.cleanUp(); @@ -328,9 +372,10 @@ describe('Session index', () => { expect(mockElasticsearchClient.bulk).toHaveBeenCalledTimes(1); expect(mockElasticsearchClient.bulk).toHaveBeenCalledWith( { - index: indexName, + index: aliasName, operations: [{ delete: { _id: sessionValue._id } }], refresh: false, + require_alias: true, }, { ignore: [409, 404], @@ -419,9 +464,10 @@ describe('Session index', () => { expect(mockElasticsearchClient.bulk).toHaveBeenCalledTimes(1); expect(mockElasticsearchClient.bulk).toHaveBeenCalledWith( { - index: indexName, + index: aliasName, operations: [{ delete: { _id: sessionValue._id } }], refresh: false, + require_alias: true, }, { ignore: [409, 404], @@ -506,9 +552,10 @@ describe('Session index', () => { expect(mockElasticsearchClient.bulk).toHaveBeenCalledTimes(1); expect(mockElasticsearchClient.bulk).toHaveBeenCalledWith( { - index: indexName, + index: aliasName, operations: [{ delete: { _id: sessionValue._id } }], refresh: false, + require_alias: true, }, { ignore: [409, 404], @@ -603,9 +650,10 @@ describe('Session index', () => { expect(mockElasticsearchClient.bulk).toHaveBeenCalledTimes(1); expect(mockElasticsearchClient.bulk).toHaveBeenCalledWith( { - index: indexName, + index: aliasName, operations: [{ delete: { _id: sessionValue._id } }], refresh: false, + require_alias: true, }, { ignore: [409, 404], @@ -748,9 +796,10 @@ describe('Session index', () => { expect(mockElasticsearchClient.bulk).toHaveBeenCalledTimes(1); expect(mockElasticsearchClient.bulk).toHaveBeenCalledWith( { - index: indexName, + index: aliasName, operations: [{ delete: { _id: sessionValue._id } }], refresh: false, + require_alias: true, }, { ignore: [409, 404], diff --git a/x-pack/plugins/security/server/session_management/session_index.ts b/x-pack/plugins/security/server/session_management/session_index.ts index 0bb04cc706ce8..31c7f832bc8a1 100644 --- a/x-pack/plugins/security/server/session_management/session_index.ts +++ b/x-pack/plugins/security/server/session_management/session_index.ts @@ -449,9 +449,11 @@ export class SessionIndex { if (operations.length > 0) { const bulkResponse = await elasticsearchClient.bulk( { - index: this.indexName, + index: this.aliasName, operations, refresh: false, + // delete operations do not respect `require_alias`, but we include it here for consistency. + require_alias: true, }, { ignore: [409, 404] } ); @@ -656,10 +658,26 @@ export class SessionIndex { }); } - const openPitResponse = await this.options.elasticsearchClient.openPointInTime({ - index: this.indexName, - keep_alive: SESSION_INDEX_CLEANUP_KEEP_ALIVE, - }); + let { body: openPitResponse, statusCode } = + await this.options.elasticsearchClient.openPointInTime( + { + index: this.aliasName, + keep_alive: SESSION_INDEX_CLEANUP_KEEP_ALIVE, + }, + { ignore: [404], meta: true } + ); + + if (statusCode === 404) { + await this.ensureSessionIndexExists(); + ({ body: openPitResponse, statusCode } = + await this.options.elasticsearchClient.openPointInTime( + { + index: this.aliasName, + keep_alive: SESSION_INDEX_CLEANUP_KEEP_ALIVE, + }, + { meta: true } + )); + } try { let searchAfter: SortResults | undefined; From 1411a68d6f00f5b5aafb64b0acd648a40384138d Mon Sep 17 00:00:00 2001 From: Larry Gregory Date: Wed, 29 Jun 2022 14:06:18 -0400 Subject: [PATCH 11/11] use alias for index refresh --- .../plugins/security/server/session_management/session_index.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugins/security/server/session_management/session_index.ts b/x-pack/plugins/security/server/session_management/session_index.ts index 31c7f832bc8a1..42c78fc07dafc 100644 --- a/x-pack/plugins/security/server/session_management/session_index.ts +++ b/x-pack/plugins/security/server/session_management/session_index.ts @@ -487,7 +487,7 @@ export class SessionIndex { // Only refresh the index if we have actually deleted one or more sessions. The index will auto-refresh eventually anyway, this just // ensures that searches after the cleanup process are accurate, and this only impacts integration tests. try { - await elasticsearchClient.indices.refresh({ index: this.indexName }); + await elasticsearchClient.indices.refresh({ index: this.aliasName }); logger.debug(`Refreshed session index.`); } catch (err) { logger.error(`Failed to refresh session index: ${err.message}`);