Skip to content

Commit

Permalink
Attempt to use index alias + require_alias
Browse files Browse the repository at this point in the history
  • Loading branch information
@legrego
legrego committed Jun 23, 2022
1 parent 667c565 commit b890529
Show file tree
Hide file tree
Showing 2 changed files with 127 additions and 58 deletions.
87 changes: 59 additions & 28 deletions x-pack/plugins/security/server/session_management/session_index.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -55,7 +56,7 @@ describe('Session index', () => {
name: indexTemplateName,
});
expect(mockElasticsearchClient.indices.exists).toHaveBeenCalledWith({
index: getSessionIndexSettings(indexName).index,
index: getSessionIndexSettings({ indexName, aliasName }).index,
});
}

Expand Down Expand Up @@ -100,7 +101,7 @@ describe('Session index', () => {
name: indexTemplateName,
});
expect(mockElasticsearchClient.indices.create).toHaveBeenCalledWith(
getSessionIndexSettings(indexName)
getSessionIndexSettings({ indexName, aliasName })
);
});

Expand All @@ -119,7 +120,7 @@ describe('Session index', () => {
name: indexTemplateName,
});
expect(mockElasticsearchClient.indices.create).toHaveBeenCalledWith(
getSessionIndexSettings(indexName)
getSessionIndexSettings({ indexName, aliasName })
);
});

Expand All @@ -136,7 +137,7 @@ describe('Session index', () => {
name: indexTemplateName,
});
expect(mockElasticsearchClient.indices.create).toHaveBeenCalledWith(
getSessionIndexSettings(indexName)
getSessionIndexSettings({ indexName, aliasName })
);
});

Expand All @@ -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 })
);
});

Expand Down Expand Up @@ -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';
Expand All @@ -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 () => {
Expand Down Expand Up @@ -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] }
);
});
});

Expand Down
98 changes: 68 additions & 30 deletions x-pack/plugins/security/server/session_management/session_index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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: {
Expand All @@ -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: {
Expand Down Expand Up @@ -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).
Expand All @@ -166,6 +182,7 @@ export class SessionIndex {

constructor(private readonly options: Readonly<SessionIndexOptions>) {
this.indexName = `${this.options.kibanaIndexName}_security_session_${SESSION_INDEX_TEMPLATE_VERSION}`;
this.aliasName = `${this.options.kibanaIndexName}_security_session`;
}

/**
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand All @@ -525,6 +542,27 @@ export class SessionIndex {
}
}

private async writeNewSessionDocument(
sessionValue: Readonly<Omit<SessionIndexValue, 'metadata'>>,
{ 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.
*/
Expand Down

0 comments on commit b890529

Please sign in to comment.