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.

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

[Cases] Case action refinements #175505

Merged
merged 13 commits into from
Jan 31, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ export const buildExecutor = <
logger: Logger;
configurationUtilities: ActionsConfigurationUtilities;
}): ExecutorType<Config, Secrets, ExecutorParams, unknown> => {
return async ({ actionId, params, config, secrets, services }) => {
return async ({ actionId, params, config, secrets, services, request }) => {
const subAction = params.subAction;
const subActionParams = params.subActionParams;

Expand All @@ -40,6 +40,7 @@ export const buildExecutor = <
configurationUtilities,
logger,
services,
request,
});

const subActions = service.getSubActions();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import { ElasticsearchClient } from '@kbn/core-elasticsearch-server';
import { finished } from 'stream/promises';
import { IncomingMessage } from 'http';
import { PassThrough } from 'stream';
import { KibanaRequest } from '@kbn/core-http-server';
import { assertURL } from './helpers/validators';
import { ActionsConfigurationUtilities } from '../actions_config';
import { SubAction, SubActionRequestParams } from './types';
Expand All @@ -39,6 +40,7 @@ export abstract class SubActionConnector<Config, Secrets> {
private axiosInstance: AxiosInstance;
private subActions: Map<string, SubAction> = new Map();
private configurationUtilities: ActionsConfigurationUtilities;
protected readonly kibanaRequest?: KibanaRequest;
protected logger: Logger;
protected esClient: ElasticsearchClient;
protected savedObjectsClient: SavedObjectsClientContract;
Expand All @@ -55,6 +57,7 @@ export abstract class SubActionConnector<Config, Secrets> {
this.esClient = params.services.scopedClusterClient;
this.configurationUtilities = params.configurationUtilities;
this.axiosInstance = axios.create();
this.kibanaRequest = params.request;
}

private normalizeURL(url: string) {
Expand Down
2 changes: 2 additions & 0 deletions x-pack/plugins/actions/server/sub_action_framework/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import type { Logger } from '@kbn/logging';
import type { LicenseType } from '@kbn/licensing-plugin/common/types';

import type { Method, AxiosRequestConfig } from 'axios';
import { KibanaRequest } from '@kbn/core-http-server';
import type { ActionsConfigurationUtilities } from '../actions_config';
import type {
ActionTypeParams,
Expand All @@ -30,6 +31,7 @@ export interface ServiceParams<Config, Secrets> {
logger: Logger;
secrets: Secrets;
services: Services;
request?: KibanaRequest;
}

export type SubActionRequestParams<R> = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import { CasesService } from './cases_service';
import { CasesConnectorError } from './cases_connector_error';
import { CaseError } from '../../common/error';
import { fullJitterBackoffFactory } from './full_jitter_backoff';
import { CoreKibanaRequest } from '@kbn/core/server';

jest.mock('./cases_connector_executor');
jest.mock('./full_jitter_backoff');
Expand All @@ -26,6 +27,7 @@ const fullJitterBackoffFactoryMock = fullJitterBackoffFactory as jest.Mock;
describe('CasesConnector', () => {
const services = actionsMock.createServices();
const logger = loggingSystemMock.createLogger();
const kibanaRequest = CoreKibanaRequest.from({ path: '/', headers: {} });

const groupingBy = ['host.name', 'dest.ip'];
const rule = {
Expand All @@ -38,16 +40,29 @@ describe('CasesConnector', () => {
const owner = 'cases';
const timeWindow = '7d';
const reopenClosedCases = false;
const maximumCasesToOpen = 5;

const mockExecute = jest.fn();
const getCasesClient = jest.fn().mockResolvedValue({ foo: 'bar' });
const getSpaceId = jest.fn().mockReturnValue('default');
// 1ms delay before retrying
const nextBackOff = jest.fn().mockReturnValue(1);

const backOffFactory = {
create: () => ({ nextBackOff }),
};

const casesParams = { getCasesClient, getSpaceId };
const connectorParams = {
configurationUtilities: actionsConfigMock.create(),
config: {},
secrets: {},
connector: { id: '1', type: CASES_CONNECTOR_ID },
logger,
services,
request: kibanaRequest,
};

let connector: CasesConnector;

beforeEach(() => {
Expand All @@ -63,15 +78,8 @@ describe('CasesConnector', () => {
fullJitterBackoffFactoryMock.mockReturnValue(backOffFactory);

connector = new CasesConnector({
casesParams: { getCasesClient },
connectorParams: {
configurationUtilities: actionsConfigMock.create(),
config: {},
secrets: {},
connector: { id: '1', type: CASES_CONNECTOR_ID },
logger,
services,
},
casesParams,
connectorParams,
});
});

Expand All @@ -83,13 +91,15 @@ describe('CasesConnector', () => {
rule,
timeWindow,
reopenClosedCases,
maximumCasesToOpen,
});

expect(CasesConnectorExecutorMock).toBeCalledWith({
logger,
casesClient: { foo: 'bar' },
casesOracleService: expect.any(CasesOracleService),
casesService: expect.any(CasesService),
spaceId: 'default',
});
});

Expand All @@ -101,6 +111,7 @@ describe('CasesConnector', () => {
rule,
timeWindow,
reopenClosedCases,
maximumCasesToOpen,
});

expect(mockExecute).toBeCalledWith({
Expand All @@ -110,6 +121,7 @@ describe('CasesConnector', () => {
rule,
timeWindow,
reopenClosedCases,
maximumCasesToOpen,
});
});

Expand All @@ -121,6 +133,7 @@ describe('CasesConnector', () => {
rule,
timeWindow,
reopenClosedCases,
maximumCasesToOpen,
});

expect(getCasesClient).toBeCalled();
Expand All @@ -137,11 +150,12 @@ describe('CasesConnector', () => {
rule,
timeWindow,
reopenClosedCases,
maximumCasesToOpen,
})
).rejects.toThrowErrorMatchingInlineSnapshot(`"Bad request"`);

expect(logger.error.mock.calls[0][0]).toBe(
'[CasesConnector][_run] Execution of case connector failed. Message: Bad request. Status code: 400'
'[CasesConnector][run] Execution of case connector failed. Message: Bad request. Status code: 400'
);
});

Expand All @@ -156,11 +170,12 @@ describe('CasesConnector', () => {
rule,
timeWindow,
reopenClosedCases,
maximumCasesToOpen,
})
).rejects.toThrowErrorMatchingInlineSnapshot(`"Forbidden"`);

expect(logger.error.mock.calls[0][0]).toBe(
'[CasesConnector][_run] Execution of case connector failed. Message: Forbidden. Status code: 500'
'[CasesConnector][run] Execution of case connector failed. Message: Forbidden. Status code: 500'
);
});

Expand All @@ -175,11 +190,12 @@ describe('CasesConnector', () => {
rule,
timeWindow,
reopenClosedCases,
maximumCasesToOpen,
})
).rejects.toThrowErrorMatchingInlineSnapshot(`"Server error"`);

expect(logger.error.mock.calls[0][0]).toBe(
'[CasesConnector][_run] Execution of case connector failed. Message: Server error. Status code: 500'
'[CasesConnector][run] Execution of case connector failed. Message: Server error. Status code: 500'
);
});

Expand All @@ -196,9 +212,36 @@ describe('CasesConnector', () => {
rule,
timeWindow,
reopenClosedCases,
maximumCasesToOpen,
});

expect(nextBackOff).toBeCalledTimes(2);
expect(mockExecute).toBeCalledTimes(3);
});

it('throws if the kibana request is not defined', async () => {
connector = new CasesConnector({
casesParams,
connectorParams: { ...connectorParams, request: undefined },
});

await expect(() =>
connector.run({
alerts: [],
groupingBy,
owner,
rule,
timeWindow,
reopenClosedCases,
maximumCasesToOpen,
})
).rejects.toThrowErrorMatchingInlineSnapshot(`"Kibana request is not defined"`);

expect(logger.error.mock.calls[0][0]).toBe(
'[CasesConnector][run] Execution of case connector failed. Message: Kibana request is not defined. Status code: 400'
);

expect(nextBackOff).toBeCalledTimes(0);
expect(mockExecute).toBeCalledTimes(0);
});
});
82 changes: 42 additions & 40 deletions x-pack/plugins/cases/server/connectors/cases/cases_connector.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
import type { ServiceParams } from '@kbn/actions-plugin/server';
import { SubActionConnector } from '@kbn/actions-plugin/server';
import type { KibanaRequest } from '@kbn/core-http-server';
import { CoreKibanaRequest } from '@kbn/core/server';
import { CASES_CONNECTOR_SUB_ACTION } from './constants';
import type { CasesConnectorConfig, CasesConnectorRunParams, CasesConnectorSecrets } from './types';
import { CasesConnectorRunParamsSchema } from './schema';
Expand All @@ -26,7 +25,10 @@ import { fullJitterBackoffFactory } from './full_jitter_backoff';

interface CasesConnectorParams {
connectorParams: ServiceParams<CasesConnectorConfig, CasesConnectorSecrets>;
casesParams: { getCasesClient: (request: KibanaRequest) => Promise<CasesClient> };
casesParams: {
getCasesClient: (request: KibanaRequest) => Promise<CasesClient>;
getSpaceId: (request?: KibanaRequest) => string;
};
}

export class CasesConnector extends SubActionConnector<
Expand All @@ -36,20 +38,14 @@ export class CasesConnector extends SubActionConnector<
private readonly casesOracleService: CasesOracleService;
private readonly casesService: CasesService;
private readonly retryService: CaseConnectorRetryService;
private readonly kibanaRequest: KibanaRequest;
private readonly casesParams: CasesConnectorParams['casesParams'];

constructor({ connectorParams, casesParams }: CasesConnectorParams) {
super(connectorParams);

this.casesOracleService = new CasesOracleService({
logger: this.logger,
/**
* TODO: Think about permissions etc.
* Should we use our own savedObjectsClient as we do
* in the cases client? Should we so the createInternalRepository?
*/
unsecuredSavedObjectsClient: this.savedObjectsClient,
savedObjectsClient: this.savedObjectsClient,
});

this.casesService = new CasesService();
Expand All @@ -60,12 +56,6 @@ export class CasesConnector extends SubActionConnector<
const backOffFactory = fullJitterBackoffFactory({ baseDelay: 5, maxBackoffTime: 2000 });
this.retryService = new CaseConnectorRetryService(this.logger, backOffFactory);

/**
* TODO: Get request from the actions framework.
* Should be set in the SubActionConnector's constructor
*/
this.kibanaRequest = CoreKibanaRequest.from({ path: '/', headers: {} });

this.casesParams = casesParams;

this.registerSubActions();
Expand All @@ -89,22 +79,30 @@ export class CasesConnector extends SubActionConnector<
}

public async run(params: CasesConnectorRunParams) {
/**
* TODO: Tell the task manager to not retry on non
* retryable errors
*/
if (!this.kibanaRequest) {
const error = new CasesConnectorError('Kibana request is not defined', 400);
this.handleError(error);
}

await this.retryService.retryWithBackoff(() => this._run(params));
}

private async _run(params: CasesConnectorRunParams) {
try {
const casesClient = await this.casesParams.getCasesClient(this.kibanaRequest);
/**
* The case connector will throw an error if the Kibana request
* is not define before executing the _run method
*/
const kibanaRequest = this.kibanaRequest as KibanaRequest;
Copy link
Contributor

Choose a reason for hiding this comment

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

I might not be understanding the full picture here, but since we removed private readonly kibanaRequest: KibanaRequest; at line 39,
shouldn't it be const kibanaRequest = this.connectorParams.kibanaRequest as KibanaRequest; here?

Copy link
Member Author

@cnasikas cnasikas Jan 30, 2024

Choose a reason for hiding this comment

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

If you check at the x-pack/plugins/actions/server/sub_action_framework/sub_action_connector.ts we set the kibanaRequest. It is already provided by the actions framework. The SubActionConnector class extends the SubActionConnector class. That's why we can access it like this. kibanaRequest .

const casesClient = await this.casesParams.getCasesClient(kibanaRequest);
const spaceId = this.casesParams.getSpaceId(kibanaRequest);

const connectorExecutor = new CasesConnectorExecutor({
logger: this.logger,
casesOracleService: this.casesOracleService,
casesService: this.casesService,
casesClient,
spaceId,
});

this.logDebugCurrentState('start', '[CasesConnector][_run] Executing case connector', params);
Expand All @@ -117,25 +115,7 @@ export class CasesConnector extends SubActionConnector<
params
);
} catch (error) {
if (isCasesConnectorError(error)) {
this.logError(error);
throw error;
}

if (isCasesClientError(error)) {
const caseConnectorError = new CasesConnectorError(
error.message,
error.boomify().output.statusCode
);

this.logError(caseConnectorError);
throw caseConnectorError;
}

const caseConnectorError = new CasesConnectorError(error.message, 500);
this.logError(caseConnectorError);

throw caseConnectorError;
this.handleError(error);
} finally {
this.logDebugCurrentState(
'end',
Expand All @@ -145,6 +125,28 @@ export class CasesConnector extends SubActionConnector<
}
}

private handleError(error: Error) {
if (isCasesConnectorError(error)) {
this.logError(error);
throw error;
}

if (isCasesClientError(error)) {
const caseConnectorError = new CasesConnectorError(
error.message,
error.boomify().output.statusCode
);

this.logError(caseConnectorError);
throw caseConnectorError;
}

const caseConnectorError = new CasesConnectorError(error.message, 500);
this.logError(caseConnectorError);

throw caseConnectorError;
}

private logDebugCurrentState(state: string, message: string, params: CasesConnectorRunParams) {
const alertIds = params.alerts.map(({ _id }) => _id);

Expand All @@ -163,7 +165,7 @@ export class CasesConnector extends SubActionConnector<

private logError(error: CasesConnectorError) {
this.logger.error(
`[CasesConnector][_run] Execution of case connector failed. Message: ${error.message}. Status code: ${error.statusCode}`,
`[CasesConnector][run] Execution of case connector failed. Message: ${error.message}. Status code: ${error.statusCode}`,
{
error: {
stack_trace: error.stack,
Expand Down
Loading