Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Stop relying on index template for session index #134900

Merged
merged 15 commits into from
Jun 30, 2022
Merged

Stop relying on index template for session index #134900

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
c25e378
Remove session index template
legrego Jun 22, 2022
10c0ed1
Attempt to ensure session index exists before creating session document
legrego Jun 22, 2022
667c565
improved comments
legrego Jun 22, 2022
b890529
Attempt to use index alias + require_alias
legrego Jun 23, 2022
47f3393
more docs and testing
legrego Jun 23, 2022
f10e428
Merge branch 'main' of github.com:elastic/kibana into security/remove…
legrego Jun 23, 2022
33d26b6
Merge branch 'main' into security/remove-session-index-template
legrego Jun 27, 2022
3e34e6a
Apply suggestions from code review
legrego Jun 28, 2022
deb7d46
Merge branch 'main' of github.com:elastic/kibana into security/remove…
legrego Jun 28, 2022
0b37e65
update session via alias
legrego Jun 28, 2022
a69acc6
get session via alias
legrego Jun 28, 2022
9040699
invalidate session via alias
legrego Jun 28, 2022
ed48c6a
use alias in cleanup and getSessionValuesInBatches
legrego Jun 29, 2022
c4c0b4d
Merge branch 'main' of github.com:elastic/kibana into security/remove…
legrego Jun 29, 2022
1411a68
use alias for index refresh
legrego Jun 29, 2022
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
107 changes: 76 additions & 31 deletions x-pack/plugins/security/server/session_management/session_index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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,
});
}

Expand Down Expand Up @@ -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 () => {
Expand Down Expand Up @@ -862,7 +862,9 @@ 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',
Expand All @@ -888,6 +890,49 @@ describe('Session index', () => {
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',
_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).not.toHaveBeenCalled();

expect(mockElasticsearchClient.create).toHaveBeenCalledTimes(1);
expect(mockElasticsearchClient.create).toHaveBeenCalledWith({
id: sid,
Expand Down
150 changes: 83 additions & 67 deletions x-pack/plugins/security/server/session_management/session_index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
* 2.0.
*/

import type { IndicesCreateRequest } from '@elastic/elasticsearch/lib/api/types';
import type {
BulkOperationContainer,
SortResults,
Expand Down Expand Up @@ -59,34 +60,29 @@ 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): IndicesCreateRequest {
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: {
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' },
},
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,
},
});
}
Expand Down Expand Up @@ -213,17 +209,30 @@ export class SessionIndex {
'Attempted to create a new session while session index is initializing.'
);
await this.indexInitialization;
} else {
await this.ensureSessionIndexExists();
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't love the extra round-trip that this incurs, but I don't see a way around it given the design of ES's APIs.

}

// **************************************************
legrego marked this conversation as resolved.
Show resolved Hide resolved
// !! 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',
Expand Down Expand Up @@ -372,7 +381,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({
Expand All @@ -385,50 +394,21 @@ 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);
}
}

// 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({ index: 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();
Expand Down Expand Up @@ -509,6 +489,42 @@ export class SessionIndex {
}
}

/**
* Creates the session index if it doesn't already exist.
*/
private async ensureSessionIndexExists() {
Copy link
Member Author

Choose a reason for hiding this comment

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

note This was extracted from the initialize function above so that it can be reused in the create function.

// Check if required index exists.
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.
*/
Expand Down