Skip to content

Commit 7461d63

Browse files
cnasikascnasikas
committed
PR feedback
1 parent a8304cd commit 7461d63

7 files changed

Lines changed: 111 additions & 33 deletions

File tree

‎x-pack/platform/plugins/shared/alerting_v2/server/lib/services/es_service/tokens.ts‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
/*
2+
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
3+
* or more contributor license agreements. Licensed under the Elastic License
4+
* 2.0; you may not use this file except in compliance with the Elastic License
5+
* 2.0.
6+
*/
7+
8+
import type { ElasticsearchClient } from '@kbn/core/server';
9+
import type { ServiceIdentifier } from 'inversify';
10+
11+
export const EsServiceInternalToken = Symbol.for(
12+
'alerting_v2.EsServiceInternal'
13+
) as ServiceIdentifier<ElasticsearchClient>;
14+
15+
export const EsServiceScopedToken = Symbol.for(
16+
'alerting_v2.EsServiceScoped'
17+
) as ServiceIdentifier<ElasticsearchClient>;

‎x-pack/platform/plugins/shared/alerting_v2/server/lib/services/resource_service/resource_initializer.test.ts‎

Lines changed: 30 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -7,15 +7,19 @@
77

88
import type { DiagnosticResult } from '@elastic/elasticsearch';
99
import { errors } from '@elastic/elasticsearch';
10-
import type { ElasticsearchClient } from '@kbn/core/server';
10+
import type { ElasticsearchClient, Logger } from '@kbn/core/server';
1111
import { elasticsearchServiceMock } from '@kbn/core-elasticsearch-server-mocks';
1212

1313
import type { ResourceDefinition } from '../../../resources/types';
1414
import { ResourceInitializer } from './resource_initializer';
1515
import type { DeeplyMockedApi } from '@kbn/core-elasticsearch-client-server-mocks';
16+
import { LoggerService } from '../logger_service/logger_service';
17+
import { loggerMock } from '@kbn/logging-mocks';
1618

1719
describe('ResourceInitializer', () => {
1820
let esClient: DeeplyMockedApi<ElasticsearchClient>;
21+
let mockLogger: jest.Mocked<Logger>;
22+
let mockLoggerService: LoggerService;
1923

2024
const resourceDefinition: ResourceDefinition = {
2125
key: 'data_stream:.alerts-test',
@@ -45,8 +49,10 @@ describe('ResourceInitializer', () => {
4549
};
4650

4751
beforeEach(() => {
48-
esClient = elasticsearchServiceMock.createElasticsearchClient();
4952
jest.clearAllMocks();
53+
mockLogger = loggerMock.create();
54+
mockLoggerService = new LoggerService(mockLogger);
55+
esClient = elasticsearchServiceMock.createElasticsearchClient();
5056

5157
esClient.ilm.putLifecycle.mockResolvedValue({ acknowledged: true });
5258
esClient.cluster.putComponentTemplate.mockResolvedValue({ acknowledged: true });
@@ -55,7 +61,7 @@ describe('ResourceInitializer', () => {
5561
});
5662

5763
it('installs ILM policy, component template, index template, then creates the data stream', async () => {
58-
const initializer = new ResourceInitializer(esClient, resourceDefinition);
64+
const initializer = new ResourceInitializer(mockLoggerService, esClient, resourceDefinition);
5965

6066
await initializer.initialize();
6167

@@ -76,25 +82,41 @@ describe('ResourceInitializer', () => {
7682
new errors.ResponseError({ statusCode: 409 } as DiagnosticResult)
7783
);
7884

79-
const initializer = new ResourceInitializer(esClient, resourceDefinition);
85+
const initializer = new ResourceInitializer(mockLoggerService, esClient, resourceDefinition);
8086
await expect(initializer.initialize()).resolves.toBeUndefined();
8187
});
8288

8389
it('ignores 400 errors when creating the data stream', async () => {
8490
esClient.indices.createDataStream.mockRejectedValueOnce(
85-
new errors.ResponseError({ statusCode: 400 } as DiagnosticResult)
91+
new errors.ResponseError({
92+
statusCode: 400,
93+
body: { error: { type: 'resource_already_exists_exception' } },
94+
} as DiagnosticResult)
8695
);
8796

88-
const initializer = new ResourceInitializer(esClient, resourceDefinition);
97+
const initializer = new ResourceInitializer(mockLoggerService, esClient, resourceDefinition);
8998
await expect(initializer.initialize()).resolves.toBeUndefined();
9099
});
91100

101+
it('re-throws 400 errors other than resource_already_exists_exception when creating the data stream', async () => {
102+
esClient.indices.createDataStream.mockRejectedValueOnce(
103+
new errors.ResponseError({
104+
statusCode: 400,
105+
} as DiagnosticResult)
106+
);
107+
108+
const initializer = new ResourceInitializer(mockLoggerService, esClient, resourceDefinition);
109+
await expect(initializer.initialize()).rejects.toThrow();
110+
});
111+
92112
it('re-throws the rest of the errors when creating the data stream', async () => {
93113
esClient.indices.createDataStream.mockRejectedValueOnce(
94-
new errors.ResponseError({ statusCode: 500 } as DiagnosticResult)
114+
new errors.ResponseError({
115+
statusCode: 500,
116+
} as DiagnosticResult)
95117
);
96118

97-
const initializer = new ResourceInitializer(esClient, resourceDefinition);
119+
const initializer = new ResourceInitializer(mockLoggerService, esClient, resourceDefinition);
98120
await expect(initializer.initialize()).rejects.toThrow();
99121
});
100122
});

‎x-pack/platform/plugins/shared/alerting_v2/server/lib/services/resource_service/resource_initializer.ts‎

Lines changed: 26 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,10 @@ import type {
1111
IndicesPutIndexTemplateRequest,
1212
} from '@elastic/elasticsearch/lib/api/types';
1313
import { isResponseError } from '@kbn/es-errors';
14+
import { inject, injectable } from 'inversify';
1415
import type { ResourceDefinition } from '../../../resources/types';
16+
import { EsServiceInternalToken } from '../es_service/tokens';
17+
import { LoggerService } from '../logger_service/logger_service';
1518

1619
export interface IResourceInitializer {
1720
initialize(): Promise<void>;
@@ -22,15 +25,13 @@ export interface ResourceInitializerOptions {
2225
resourceDefinition: ResourceDefinition;
2326
}
2427

25-
function getEsErrorStatusCode(error: unknown): number | undefined {
26-
return isResponseError(error) ? error.statusCode : undefined;
27-
}
28-
2928
const TOTAL_FIELDS_LIMIT = 2500;
3029

30+
@injectable()
3131
export class ResourceInitializer implements IResourceInitializer {
3232
constructor(
33-
private readonly esClient: ElasticsearchClient,
33+
@inject(LoggerService) private readonly logger: LoggerService,
34+
@inject(EsServiceInternalToken) private readonly esClient: ElasticsearchClient,
3435
private readonly resourceDefinition: ResourceDefinition
3536
) {}
3637

@@ -81,12 +82,28 @@ export class ResourceInitializer implements IResourceInitializer {
8182
await this.esClient.indices.createDataStream({
8283
name: this.resourceDefinition.dataStreamName,
8384
});
84-
} catch (e) {
85-
const status = getEsErrorStatusCode(e);
85+
} catch (error) {
86+
if (!isResponseError(error)) {
87+
throw error;
88+
}
8689

87-
if (status !== 400 && status !== 409) {
88-
throw e;
90+
if (isResourceAlreadyExistsException(error)) {
91+
this.logger.debug({
92+
message: `Data stream already exists: ${this.resourceDefinition.dataStreamName}`,
93+
});
94+
95+
return;
8996
}
97+
98+
throw error;
9099
}
91100
}
92101
}
102+
103+
const isResourceAlreadyExistsException = (error: unknown): boolean => {
104+
return (
105+
isResponseError(error) &&
106+
((error.statusCode === 400 && error.body?.error.type === 'resource_already_exists_exception') ||
107+
error.statusCode === 409)
108+
);
109+
};

‎x-pack/platform/plugins/shared/alerting_v2/server/lib/services/resource_service/resource_manager.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ export class ResourceManager {
6767
// Fire-and-forget: initialization errors must NOT become unhandled rejections
6868
// (which would crash Kibana). The error is still stored on the resource state,
6969
// and consumers will fail fast when calling `waitUntilReady()` / `ensureResourceReady()`.
70-
void this.startResource(key).catch(() => {
70+
void this.initResource(key).catch(() => {
7171
this.logger.debug({
7272
message: `ResourceManager: Initialization for resource [${key}] failed`,
7373
});
@@ -97,10 +97,10 @@ export class ResourceManager {
9797
* If the resource permanently fails (even after retries), this rejects quickly for all callers.
9898
*/
9999
public async ensureResourceReady(key: string): Promise<void> {
100-
await this.startResource(key);
100+
await this.initResource(key);
101101
}
102102

103-
private async startResource(key: string): Promise<void> {
103+
private async initResource(key: string): Promise<void> {
104104
const state = this.resources.get(key);
105105

106106
if (!state?.initializer) {

‎x-pack/platform/plugins/shared/alerting_v2/server/resources/register_resources.ts‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,18 +12,26 @@ import type { ResourceManager } from '../lib/services/resource_service/resource_
1212
import type { ResourceDefinition } from './types';
1313
import { getAlertTransitionsResourceDefinition } from './alert_transitions';
1414
import { getAlertActionsResourceDefinition } from './alert_actions';
15+
import type { LoggerService } from '../lib/services/logger_service/logger_service';
1516

1617
export interface RegisterResourcesOptions {
1718
resourceManager: ResourceManager;
1819
esClient: ElasticsearchClient;
20+
logger: LoggerService;
1921
}
2022

21-
export function registerResources({ resourceManager, esClient }: RegisterResourcesOptions): void {
23+
export function initializeResources({
24+
resourceManager,
25+
esClient,
26+
logger,
27+
}: RegisterResourcesOptions): void {
2228
for (const resourceDefinition of getDataStreamResourceDefinitions()) {
23-
const initializer = new ResourceInitializer(esClient, resourceDefinition);
29+
const initializer = new ResourceInitializer(logger, esClient, resourceDefinition);
2430

2531
resourceManager.registerResource(resourceDefinition.key, initializer);
2632
}
33+
34+
resourceManager.startInitialization();
2735
}
2836

2937
function getDataStreamResourceDefinitions(): ResourceDefinition[] {

‎x-pack/platform/plugins/shared/alerting_v2/server/setup/bind_on_start.ts‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -7,20 +7,21 @@
77

88
import type { ContainerModuleLoadOptions } from 'inversify';
99
import { OnStart } from '@kbn/core-di';
10-
import { CoreStart } from '@kbn/core-di-server';
1110
import { ResourceManager } from '../lib/services/resource_service/resource_manager';
12-
import { registerResources } from '../resources/register_resources';
11+
import { initializeResources } from '../resources/register_resources';
12+
import { LoggerService } from '../lib/services/logger_service/logger_service';
13+
import { EsServiceInternalToken } from '../lib/services/es_service/tokens';
1314

1415
export function bindOnStart({ bind }: ContainerModuleLoadOptions) {
1516
bind(OnStart).toConstantValue((container) => {
1617
const resourceManager = container.get(ResourceManager);
17-
const esClient = container.get(CoreStart('elasticsearch')).client.asInternalUser;
18+
const logger = container.get(LoggerService);
19+
const esClient = container.get(EsServiceInternalToken);
1820

19-
registerResources({
21+
initializeResources({
22+
logger,
2023
resourceManager,
2124
esClient,
2225
});
23-
24-
resourceManager.startInitialization();
2526
});
2627
}

‎x-pack/platform/plugins/shared/alerting_v2/server/setup/bind_services.ts‎

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import {
2020
} from '../lib/services/storage_service/tokens';
2121
import type { AlertingServerStartDependencies } from '../types';
2222
import { RetryServiceToken } from '../lib/services/retry_service/tokens';
23+
import { EsServiceInternalToken, EsServiceScopedToken } from '../lib/services/es_service/tokens';
2324

2425
export function bindServices({ bind }: ContainerModuleLoadOptions) {
2526
bind(RulesClient).toSelf().inRequestScope();
@@ -29,6 +30,21 @@ export function bindServices({ bind }: ContainerModuleLoadOptions) {
2930
bind(LoggerService).toSelf().inSingletonScope();
3031
bind(ResourceManager).toSelf().inSingletonScope();
3132

33+
bind(EsServiceInternalToken)
34+
.toDynamicValue(({ get }) => {
35+
const elasticsearch = get(CoreStart('elasticsearch'));
36+
return elasticsearch.client.asInternalUser;
37+
})
38+
.inSingletonScope();
39+
40+
bind(EsServiceScopedToken)
41+
.toDynamicValue(({ get }) => {
42+
const request = get(Request);
43+
const elasticsearch = get(CoreStart('elasticsearch'));
44+
return elasticsearch.client.asScoped(request).asCurrentUser;
45+
})
46+
.inRequestScope();
47+
3248
bind(QueryService)
3349
.toDynamicValue(({ get }) => {
3450
const request = get(Request);
@@ -41,19 +57,16 @@ export function bindServices({ bind }: ContainerModuleLoadOptions) {
4157

4258
bind(StorageServiceScopedToken)
4359
.toDynamicValue(({ get }) => {
44-
const request = get(Request);
45-
const elasticsearch = get(CoreStart('elasticsearch'));
4660
const loggerService = get(LoggerService);
47-
const esClient = elasticsearch.client.asScoped(request).asCurrentUser;
61+
const esClient = get(EsServiceScopedToken);
4862
return new StorageService(esClient, loggerService);
4963
})
5064
.inRequestScope();
5165

5266
bind(StorageServiceInternalToken)
5367
.toDynamicValue(({ get }) => {
54-
const elasticsearch = get(CoreStart('elasticsearch'));
5568
const loggerService = get(LoggerService);
56-
const esClient = elasticsearch.client.asInternalUser;
69+
const esClient = get(EsServiceInternalToken);
5770
return new StorageService(esClient, loggerService);
5871
})
5972
.inSingletonScope();

0 commit comments

Comments
 (0)