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

[Security Solution][Endpoint] Potential improvements in endpoint user artifact packager #160387

Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
7aed42e
Updates package policy in parallel
dasansol92 Jun 14, 2023
7f3cc68
Adds bulk delete artifacts and uses it in manifest manager
dasansol92 Jun 15, 2023
d768630
Sets as deprecated the cleanup function in manifest manager and remov…
dasansol92 Jun 20, 2023
11eed82
Query package policy ids one time and reuse it instead of querying it…
dasansol92 Jun 20, 2023
f54a82c
Stores exceptions in a cache to avoid performing too many requests
dasansol92 Jun 21, 2023
9608d18
Merge branch 'main' into feat/olm-potential_improvements_in_endpoint_…
dasansol92 Jun 21, 2023
14cd787
Merge branch 'main' into feat/olm-potential_improvements_in_endpoint_…
dasansol92 Jun 22, 2023
ca73b54
Fixes manifest manager unit test and other minor fixes
dasansol92 Jun 22, 2023
61b8331
Fixes lists tests and includes minor fixes
dasansol92 Jun 22, 2023
57df8f2
Adds unit tests for fleet artifacts bulk delete function
dasansol92 Jun 22, 2023
6c48302
Enables artifacts cleanup using bulk delete
dasansol92 Jun 23, 2023
805f152
Uses packageName from the instance and adds unit test for bulk delete…
dasansol92 Jun 23, 2023
287f839
Generates ids with package in fleet artifacts client and updates unit…
dasansol92 Jun 23, 2023
efca38d
Tracks bulkDeleteArtifacts errors
dasansol92 Jun 23, 2023
0f7ae0b
Returns always an array of errors for bulkDeleteArtifacts
dasansol92 Jun 23, 2023
cd628a2
Checks if there is any error before returning errors array on deleteA…
dasansol92 Jun 23, 2023
ad11a8f
Adds config value for package policy update concurrency with default …
dasansol92 Jun 26, 2023
26e99fc
Merge branch 'main' into feat/olm-potential_improvements_in_endpoint_…
kibanamachine Jun 26, 2023
07f122e
Increases search page size to 1000
dasansol92 Jun 27, 2023
74bca2b
Uses bulkUpdate for package policy updates
dasansol92 Jun 27, 2023
88d5d70
Uses map instead of flatMap
dasansol92 Jun 27, 2023
dce6775
Uses common method and refactor to use batch instead
dasansol92 Jun 27, 2023
92b521b
Adds elasticsearch client to manifest manager context and uses it for…
dasansol92 Jun 27, 2023
5c9ef42
Adds perPage argument on pageSuplier function
dasansol92 Jun 27, 2023
21041b6
Merge branch 'main' into feat/olm-potential_improvements_in_endpoint_…
kibanamachine Jun 28, 2023
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
49 changes: 49 additions & 0 deletions x-pack/plugins/fleet/server/services/artifacts/artifacts.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import {
} from './mocks';
import {
bulkCreateArtifacts,
bulkDeleteArtifacts,
createArtifact,
deleteArtifact,
encodeArtifactContent,
Expand Down Expand Up @@ -340,6 +341,54 @@ describe('When using the artifacts services', () => {
});
});

describe('and calling `bulkDeleteArtifacts()`', () => {
it('should delete single artifact', async () => {
bulkDeleteArtifacts(esClientMock, ['123']);

expect(esClientMock.bulk).toHaveBeenCalledWith({
refresh: 'wait_for',
body: [
{
delete: {
_id: '123',
_index: FLEET_SERVER_ARTIFACTS_INDEX,
},
},
],
});
});

it('should delete all the artifacts', async () => {
bulkDeleteArtifacts(esClientMock, ['123', '231']);

expect(esClientMock.bulk).toHaveBeenCalledWith({
refresh: 'wait_for',
body: [
{
delete: {
_id: '123',
_index: FLEET_SERVER_ARTIFACTS_INDEX,
},
},
{
delete: {
_id: '231',
_index: FLEET_SERVER_ARTIFACTS_INDEX,
},
},
],
});
});

it('should throw an ArtifactElasticsearchError if one is encountered', async () => {
setEsClientMethodResponseToError(esClientMock, 'bulk');

await expect(bulkDeleteArtifacts(esClientMock, ['123'])).rejects.toBeInstanceOf(
ArtifactsElasticsearchError
);
});
});

describe('and calling `listArtifacts()`', () => {
beforeEach(() => {
esClientMock.search.mockResponse(generateArtifactEsSearchResultHitsMock());
Expand Down
33 changes: 33 additions & 0 deletions x-pack/plugins/fleet/server/services/artifacts/artifacts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,39 @@ export const deleteArtifact = async (esClient: ElasticsearchClient, id: string):
}
};

export const bulkDeleteArtifacts = async (
esClient: ElasticsearchClient,
ids: string[]
): Promise<Error[]> => {
try {
const body = ids.flatMap((id) => [
gergoabraham marked this conversation as resolved.
Show resolved Hide resolved
{
delete: { _index: FLEET_SERVER_ARTIFACTS_INDEX, _id: id },
},
]);

const res = await withPackageSpan(`Bulk delete fleet artifacts`, () =>
esClient.bulk({
body,
Copy link
Member

Choose a reason for hiding this comment

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

nit: body is deprecated over operations for bulk. You can rename body to operations. You can change the other bulk calls in the file as well.

Similarly, the create method (used in createArtifact) prefers document instead of a body in the request, and the search (used in listArtifacts) method accepts sort directly in the request instead of being nested inside a body.

refresh: 'wait_for',
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you need to wait_for on delete right?

Copy link
Contributor Author

@dasansol92 dasansol92 Jun 27, 2023

Choose a reason for hiding this comment

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

If we don't wait for the deletions to be done, then the clanup phase will try to remove artifacts that might be removed later. This is why I used here the wait_for, otherwise, the cleanup method in manifest manager will hit errors because it queried some orphan artifacts (not yet removed from the search) but already removed from the index.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. Thanks.

})
);
let errors: Error[] = [];
// Track errors of the bulk delete action
if (res.errors) {
errors = res.items.reduce<Error[]>((acc, item) => {
if (item.delete?.error) {
acc.push(new Error(item.delete.error.reason));
}
return acc;
}, []);
}
return errors;
} catch (e) {
throw new ArtifactsElasticsearchError(e);
}
};

export const listArtifacts = async (
esClient: ElasticsearchClient,
options: ListArtifactsProps = {}
Expand Down
21 changes: 21 additions & 0 deletions x-pack/plugins/fleet/server/services/artifacts/client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@

import { elasticsearchServiceMock } from '@kbn/core/server/mocks';

import { FLEET_SERVER_ARTIFACTS_INDEX } from '../../../common/constants';

import { ArtifactsClientAccessDeniedError, ArtifactsClientError } from '../../errors';

import { appContextService } from '../app_context';
Expand Down Expand Up @@ -155,6 +157,25 @@ describe('When using the Fleet Artifacts Client', () => {
});
});

describe('and calling `bulkDeleteArtifacts()`', () => {
it('should bulk delete the artifact', async () => {
setEsClientGetMock();
await artifactClient.bulkDeleteArtifacts(['123']);
expect(esClientMock.bulk).toHaveBeenCalledWith(
expect.objectContaining({
body: [
{
delete: {
_id: 'endpoint:123',
_index: FLEET_SERVER_ARTIFACTS_INDEX,
},
},
],
})
);
});
});

describe('and calling `listArtifacts()`', () => {
beforeEach(() => {
esClientMock.search.mockResponse(generateArtifactEsSearchResultHitsMock());
Expand Down
8 changes: 7 additions & 1 deletion x-pack/plugins/fleet/server/services/artifacts/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import type {
NewArtifact,
ListArtifactsProps,
} from './types';
import { relativeDownloadUrlFromArtifact } from './mappings';
import { relativeDownloadUrlFromArtifact, uniqueIdFromId } from './mappings';

import {
createArtifact,
Expand All @@ -28,6 +28,7 @@ import {
getArtifact,
listArtifacts,
bulkCreateArtifacts,
bulkDeleteArtifacts,
} from './artifacts';

/**
Expand Down Expand Up @@ -112,6 +113,11 @@ export class FleetArtifactsClient implements ArtifactsClientInterface {
}
}

async bulkDeleteArtifacts(ids: string[]): Promise<Error[]> {
const idsMappedWithPackageName = ids.map((id) => uniqueIdFromId(id, this.packageName));
return await bulkDeleteArtifacts(this.esClient, idsMappedWithPackageName);
}

/**
* Get a list of artifacts.
* NOTE that when using the `kuery` filtering param, that all filters property names should
Expand Down
4 changes: 4 additions & 0 deletions x-pack/plugins/fleet/server/services/artifacts/mappings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,3 +82,7 @@ export const uniqueIdFromArtifact = <
}: T): string => {
return `${packageName}:${identifier}-${decodedSha256}`;
};

export const uniqueIdFromId = (id: string, packageName: string): string => {
Copy link
Contributor

@juliaElastic juliaElastic Jun 28, 2023

Choose a reason for hiding this comment

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

nit: uniqueIdFromId is a bit confusing, is id the artifact id? We could rename the function to uniqueIdFromArtifactId

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The id of the artifact without the packageName, so it adds the packageName to the current id as an ES id: ${packageName}:${artifactId}.
Is the same thing uniqueIdFromArtifact does, but as we only have the id here I thought would be better to just create this new method instead of load the artifact to generate the unique Id with the existing method.

Copy link
Member

Choose a reason for hiding this comment

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

You can also rename this to packagePrefixedId. That might be less confusing to read. 😅

return `${packageName}:${id}`;
};
6 changes: 5 additions & 1 deletion x-pack/plugins/fleet/server/services/artifacts/mocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ export const createArtifactsClientMock = (): jest.Mocked<ArtifactsClientInterfac
createArtifact: jest.fn().mockResolvedValue(generateArtifactMock()),
bulkCreateArtifacts: jest.fn().mockResolvedValue({ artifacts: generateArtifactMock() }),
deleteArtifact: jest.fn(),
bulkDeleteArtifacts: jest.fn(),
listArtifacts: jest.fn().mockResolvedValue({
items: [generateArtifactMock()],
total: 1,
Expand Down Expand Up @@ -172,7 +173,10 @@ export const generateEsApiResponseMock = <TBody extends Record<string, any>>(
};

type EsClientMock = ReturnType<typeof elasticsearchServiceMock.createInternalClient>;
type EsClientMockMethods = keyof Pick<EsClientMock, 'get' | 'create' | 'delete' | 'search'>;
type EsClientMockMethods = keyof Pick<
EsClientMock,
'get' | 'create' | 'delete' | 'search' | 'bulk'
>;

export const setEsClientMethodResponseToError = (
esClientMock: EsClientMock,
Expand Down
2 changes: 2 additions & 0 deletions x-pack/plugins/fleet/server/services/artifacts/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,8 @@ export interface ArtifactsClientInterface {

deleteArtifact(id: string): Promise<void>;

bulkDeleteArtifacts(ids: string[]): Promise<Error[]>;

listArtifacts(options?: ListArtifactsProps): Promise<ListResult<Artifact>>;

encodeContent(content: ArtifactsClientCreateOptions['content']): Promise<ArtifactEncodedMetadata>;
Expand Down
Loading