Skip to content

Adding post create package callback #128744

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

Merged
merged 7 commits into from
Apr 13, 2022
Merged

Adding post create package callback #128744

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
348c5f7
draft PR for adding post create package callback
CohenIdo Mar 29, 2022
2abdcad
add new CB type
CohenIdo Apr 3, 2022
cc49725
add tests
CohenIdo Apr 4, 2022
cfa7ef2
fix types
CohenIdo Apr 11, 2022
b229fcf
update packagePolicy schema validator
CohenIdo Apr 11, 2022
c9cb7a4
update scheme and test
CohenIdo Apr 11, 2022
c213090
extend packagePolicy scheme validation
CohenIdo Apr 12, 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
1 change: 1 addition & 0 deletions x-pack/plugins/fleet/server/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ export type {
PostPackagePolicyDeleteCallback,
PostPackagePolicyCreateCallback,
FleetRequestHandlerContext,
PostPackagePolicyPostCreateCallback,
} from './types';
export { AgentNotFoundError, FleetUnauthorizedError } from './errors';

Expand Down
32 changes: 32 additions & 0 deletions x-pack/plugins/fleet/server/routes/package_policy/handlers.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,38 @@ describe('When calling package policy', () => {
});
});
});

describe('postCreate callback registration', () => {
it('should call to packagePolicyCreate and packagePolicyPostCreate call backs', async () => {
const request = getCreateKibanaRequest();
await routeHandler(context, request, response);

expect(response.ok).toHaveBeenCalled();
expect(packagePolicyService.runExternalCallbacks).toBeCalledTimes(2);

const firstCB = packagePolicyServiceMock.runExternalCallbacks.mock.calls[0][0];
const secondCB = packagePolicyServiceMock.runExternalCallbacks.mock.calls[1][0];

expect(firstCB).toEqual('packagePolicyCreate');
expect(secondCB).toEqual('packagePolicyPostCreate');
});

it('should not call packagePolicyPostCreate call back in case of packagePolicy create failed', async () => {
const request = getCreateKibanaRequest();

packagePolicyServiceMock.create.mockImplementationOnce(
async (soClient, esClient, newData) => {
throw new Error('foo');
}
);

await routeHandler(context, request, response);
const firstCB = packagePolicyServiceMock.runExternalCallbacks.mock.calls[0][0];

expect(firstCB).toEqual('packagePolicyCreate');
expect(packagePolicyService.runExternalCallbacks).toBeCalledTimes(1);
});
});
});

describe('update api handler', () => {
Expand Down
11 changes: 10 additions & 1 deletion x-pack/plugins/fleet/server/routes/package_policy/handlers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,16 @@ export const createPackagePolicyHandler: FleetRequestHandler<
force,
spaceId,
});
const body: CreatePackagePolicyResponse = { item: packagePolicy };

const enrichedPackagePolicy = await packagePolicyService.runExternalCallbacks(
Copy link
Member

@nchaulet nchaulet Apr 4, 2022

Choose a reason for hiding this comment

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

FYI Calling this in the handler means this will not work for preconfigured package policies, we already have the bug for other callbacks so it should probably not be addressed here #129383

Copy link
Contributor

@joshdover joshdover Apr 5, 2022

Choose a reason for hiding this comment

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

Great callout, agreed it'd be ok to defer on this and fix it for all callbacks in the same PR. That way they're all at least consistent.

Copy link
Contributor

@paul-tavares paul-tavares Apr 5, 2022

Choose a reason for hiding this comment

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

🤔 because the extension points are executed from the PackagePolicy service, I'm assuming the fix to the mentioned issue is to call the service method (runExternalCallbacks()) from the location where the preconfigured packages policies are added/updated while still leaving this code here in place.

Copy link
Contributor

@joshdover joshdover Apr 11, 2022

Choose a reason for hiding this comment

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

I think the idea would be to have packagePolicyService.create call runExternalCallbacks before returning the newly created policy to the caller. That way there is only a single call site that always runs regardless of how the package policy was created.

'packagePolicyPostCreate',
packagePolicy,
context,
request
);

const body: CreatePackagePolicyResponse = { item: enrichedPackagePolicy };

return response.ok({
body,
});
Expand Down
5 changes: 5 additions & 0 deletions x-pack/plugins/fleet/server/services/app_context.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import type {
ExternalCallbacksStorage,
PostPackagePolicyCreateCallback,
PostPackagePolicyDeleteCallback,
PostPackagePolicyPostCreateCallback,
PutPackagePolicyUpdateCallback,
} from '../types';
import type { FleetAppContext } from '../plugin';
Expand Down Expand Up @@ -183,6 +184,8 @@ class AppContextService {
? PostPackagePolicyCreateCallback
: T extends 'postPackagePolicyDelete'
? PostPackagePolicyDeleteCallback
: T extends 'packagePolicyPostCreate'
? PostPackagePolicyPostCreateCallback
: PutPackagePolicyUpdateCallback
>
| undefined {
Expand All @@ -192,6 +195,8 @@ class AppContextService {
? PostPackagePolicyCreateCallback
: T extends 'postPackagePolicyDelete'
? PostPackagePolicyDeleteCallback
: T extends 'packagePolicyPostCreate'
? PostPackagePolicyPostCreateCallback
: PutPackagePolicyUpdateCallback
>;
}
Expand Down
95 changes: 95 additions & 0 deletions x-pack/plugins/fleet/server/services/package_policy.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import type {
RegistryDataStream,
PackagePolicyInputStream,
PackagePolicy,
PostPackagePolicyPostCreateCallback,
} from '../types';
import { createPackagePolicyMock } from '../../common/mocks';

Expand Down Expand Up @@ -1356,6 +1357,100 @@ describe('Package policy service', () => {
});
});

describe('runPostPackagePolicyPostCreateCallback', () => {
let context: ReturnType<typeof xpackMocks.createRequestHandlerContext>;
let request: KibanaRequest;
const packagePolicy = {
id: '93ac25fe-0467-4fcc-a3c5-57a26a8496e2',
version: 'WzYyMzcsMV0=',
name: 'my-cis_kubernetes_benchmark',
namespace: 'default',
description: '',
package: {
name: 'cis_kubernetes_benchmark',
title: 'CIS Kubernetes Benchmark',
version: '0.0.3',
},
enabled: true,
policy_id: '1e6d0690-b995-11ec-a355-d35391e25881',
output_id: '',
inputs: [
{
type: 'cloudbeat',
policy_template: 'findings',
enabled: true,
streams: [
{
enabled: true,
data_stream: {
type: 'logs',
dataset: 'cis_kubernetes_benchmark.findings',
},
id: 'cloudbeat-cis_kubernetes_benchmark.findings-66b402b3-f24a-4018-b3d0-b88582a836ab',
compiled_stream: {
processors: [
{
add_cluster_id: null,
},
],
},
},
],
},
],
vars: {
dataYaml: {
type: 'yaml',
},
},
elasticsearch: undefined,
revision: 1,
created_at: '2022-04-11T12:44:43.385Z',
created_by: 'elastic',
updated_at: '2022-04-11T12:44:43.385Z',
updated_by: 'elastic',
};
const callbackCallingOrder: string[] = [];

beforeEach(() => {
context = xpackMocks.createRequestHandlerContext();
request = httpServerMock.createKibanaRequest();
appContextService.start(createAppContextStartContractMock());
});

afterEach(() => {
appContextService.stop();
jest.clearAllMocks();
callbackCallingOrder.length = 0;
});

it('should execute PostPackagePolicyPostCreateCallback external callbacks', async () => {
const callbackA: PostPackagePolicyPostCreateCallback = jest.fn(async (ds) => {
callbackCallingOrder.push('a');
return ds;
});

const callbackB: PostPackagePolicyPostCreateCallback = jest.fn(async (ds) => {
callbackCallingOrder.push('b');
return ds;
});

appContextService.addExternalCallback('packagePolicyPostCreate', callbackA);
appContextService.addExternalCallback('packagePolicyPostCreate', callbackB);

await packagePolicyService.runExternalCallbacks(
'packagePolicyPostCreate',
packagePolicy,
context,
request
);

expect(callbackA).toHaveBeenCalledWith(packagePolicy, context, request);
expect(callbackB).toHaveBeenCalledWith(packagePolicy, context, request);
expect(callbackCallingOrder).toEqual(['a', 'b']);
});
});

describe('preconfigurePackageInputs', () => {
describe('when variable is already defined', () => {
it('override original variable value', () => {
Expand Down
44 changes: 38 additions & 6 deletions x-pack/plugins/fleet/server/services/package_policy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,13 +53,15 @@ import {
PackagePolicyIneligibleForUpgradeError,
PackagePolicyValidationError,
} from '../errors';
import { NewPackagePolicySchema, UpdatePackagePolicySchema } from '../types';
import { NewPackagePolicySchema, PackagePolicySchema, UpdatePackagePolicySchema } from '../types';
import type {
NewPackagePolicy,
UpdatePackagePolicy,
PackagePolicy,
PackagePolicySOAttributes,
DryRunPackagePolicy,
PostPackagePolicyCreateCallback,
PostPackagePolicyPostCreateCallback,
} from '../types';
import type { ExternalCallback } from '..';

Expand Down Expand Up @@ -869,16 +871,24 @@ class PackagePolicyService implements PackagePolicyServiceInterface {
externalCallbackType: A,
packagePolicy: A extends 'postPackagePolicyDelete'
? DeletePackagePoliciesResponse
: A extends 'packagePolicyPostCreate'
? PackagePolicy
: NewPackagePolicy,
context: RequestHandlerContext,
request: KibanaRequest
): Promise<A extends 'postPackagePolicyDelete' ? void : NewPackagePolicy>;
): Promise<
A extends 'postPackagePolicyDelete'
? void
: A extends 'packagePolicyPostCreate'
? PackagePolicy
: NewPackagePolicy
>;
public async runExternalCallbacks(
externalCallbackType: ExternalCallback[0],
packagePolicy: NewPackagePolicy | DeletePackagePoliciesResponse,
packagePolicy: PackagePolicy | NewPackagePolicy | DeletePackagePoliciesResponse,
context: RequestHandlerContext,
request: KibanaRequest
): Promise<NewPackagePolicy | void> {
): Promise<PackagePolicy | NewPackagePolicy | void> {
if (externalCallbackType === 'postPackagePolicyDelete') {
return await this.runDeleteExternalCallbacks(packagePolicy as DeletePackagePoliciesResponse);
} else {
Expand All @@ -888,7 +898,21 @@ class PackagePolicyService implements PackagePolicyServiceInterface {
if (externalCallbacks && externalCallbacks.size > 0) {
let updatedNewData = newData;
for (const callback of externalCallbacks) {
const result = await callback(updatedNewData, context, request);
let result;
if (externalCallbackType === 'packagePolicyPostCreate') {
result = await (callback as PostPackagePolicyPostCreateCallback)(
updatedNewData as PackagePolicy,
context,
request
);
updatedNewData = PackagePolicySchema.validate(result);
} else {
result = await (callback as PostPackagePolicyCreateCallback)(
updatedNewData as NewPackagePolicy,
context,
request
);
}
if (externalCallbackType === 'packagePolicyCreate') {
updatedNewData = NewPackagePolicySchema.validate(result);
} else if (externalCallbackType === 'packagePolicyUpdate') {
Expand Down Expand Up @@ -1276,10 +1300,18 @@ export interface PackagePolicyServiceInterface {
externalCallbackType: A,
packagePolicy: A extends 'postPackagePolicyDelete'
? DeletePackagePoliciesResponse
: A extends 'packagePolicyPostCreate'
? PackagePolicy
: NewPackagePolicy,
context: RequestHandlerContext,
request: KibanaRequest
): Promise<A extends 'postPackagePolicyDelete' ? void : NewPackagePolicy>;
): Promise<
A extends 'postPackagePolicyDelete'
? void
: A extends 'packagePolicyPostCreate'
? PackagePolicy
: NewPackagePolicy
>;

runDeleteExternalCallbacks(deletedPackagePolicies: DeletePackagePoliciesResponse): Promise<void>;

Expand Down
12 changes: 12 additions & 0 deletions x-pack/plugins/fleet/server/types/extensions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import type {
DeletePackagePoliciesResponse,
NewPackagePolicy,
UpdatePackagePolicy,
PackagePolicy,
} from '../../common';

export type PostPackagePolicyDeleteCallback = (
Expand All @@ -25,13 +26,23 @@ export type PostPackagePolicyCreateCallback = (
request: KibanaRequest
) => Promise<NewPackagePolicy>;

export type PostPackagePolicyPostCreateCallback = (
packagePolicy: PackagePolicy,
context: RequestHandlerContext,
request: KibanaRequest
) => Promise<PackagePolicy>;

export type PutPackagePolicyUpdateCallback = (
updatePackagePolicy: UpdatePackagePolicy,
context: RequestHandlerContext,
request: KibanaRequest
) => Promise<UpdatePackagePolicy>;

export type ExternalCallbackCreate = ['packagePolicyCreate', PostPackagePolicyCreateCallback];
export type ExternalCallbackPostCreate = [
'packagePolicyPostCreate',
PostPackagePolicyPostCreateCallback
];
export type ExternalCallbackDelete = ['postPackagePolicyDelete', PostPackagePolicyDeleteCallback];
export type ExternalCallbackUpdate = ['packagePolicyUpdate', PutPackagePolicyUpdateCallback];

Expand All @@ -40,6 +51,7 @@ export type ExternalCallbackUpdate = ['packagePolicyUpdate', PutPackagePolicyUpd
*/
export type ExternalCallback =
| ExternalCallbackCreate
| ExternalCallbackPostCreate
| ExternalCallbackDelete
| ExternalCallbackUpdate;

Expand Down
21 changes: 21 additions & 0 deletions x-pack/plugins/fleet/server/types/models/package_policy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ const PackagePolicyStreamsSchema = {
})
)
),
compiled_stream: schema.maybe(schema.any()),
};

const PackagePolicyInputsSchema = {
Expand Down Expand Up @@ -150,4 +151,24 @@ export const PackagePolicySchema = schema.object({
...PackagePolicyBaseSchema,
id: schema.string(),
version: schema.maybe(schema.string()),
revision: schema.number(),
updated_at: schema.string(),
updated_by: schema.string(),
created_at: schema.string(),
created_by: schema.string(),
elasticsearch: schema.maybe(
schema.object({
privileges: schema.maybe(
schema.object({
cluster: schema.maybe(schema.arrayOf(schema.string())),
})
),
})
),
inputs: schema.arrayOf(
schema.object({
...PackagePolicyInputsSchema,
compiled_input: schema.maybe(schema.any()),
})
),
Copy link
Contributor Author

@CohenIdo CohenIdo Apr 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Hey @joshdover, I re-request your review according to this change, I tried to use this scheme for validation and found that is not aligned with the PackagePolicy type.

export interface PackagePolicy extends Omit<NewPackagePolicy, 'inputs'> {
  id: string;
  inputs: PackagePolicyInput[];
  version?: string;
  revision: number;
  updated_at: string;
  updated_by: string;
  created_at: string;
  created_by: string;
}

Copy link
Contributor

@joshdover joshdover Apr 13, 2022

Choose a reason for hiding this comment

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

Thanks for the ping, this change makes sense. At first I thought that maybe the elasticsearch key should be duplicated / moved to CreatePackagePolicyProps as well but that would allow setting it on the API which we explicitly do not support. Everything else LGTM, thanks @CohenIdo

In the future, we may want to try refactoring this to derive the TS types from the config schema using the TypeOf helper from kbn/config-schema.

});