Skip to content

Commit

Permalink
Catching saved object not found error, do not delete referenced packa…
Browse files Browse the repository at this point in the history
…ge assets (elastic#146274)

## Summary

Fixes elastic#142112

Improvements coming out of SDHs, to handle Fleet errors more robustly.

### 1. Catching package assets not found error and logging a warning, to
prevent it blocking setup/install.
### 2. In assets cleanup logic, doing a check that the assets are not
referenced by packages before deleting them.

Verifying 1. and 2. hard, because I don't have a way to reproduce the
assets not found error. Tried to install/delete different versions of a
package, but didn't run into the issues.

### 3. Force removing a package deletes package policies as well

To verify:
- add at least one integration (e.g. system package)
- force delete integration with API
```
DELETE http://elastic:changeme@localhost:5601/julia/api/fleet/epm/packages/system/1.20.4
kbn-xsrf: kibana

{ "force": true }
```
- verify that package policies are deleted as well
- there is an info log added:
```
[2022-11-24T14:10:29.138+01:00][INFO ][plugins.fleet] deleting package policies of system package because force flag was enabled: 84a4e5d4-e363-4045-9240-9ab151f2b376,6d34a544-7467-48fd-a779-c2d30d808aa6
```

### 4. Catching saved object not found error when tagging package assets
To verify:
- follow the steps above to force delete a package
- add system integration again
- verify that it succeeds, and there is a warning log
```
[2022-11-24T14:07:47.960+01:00][WARN ][plugins.fleet] Error: Saved object [dashboard/system-01c54730-fee6-11e9-8405-516218e3d268] not found
```

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
(cherry picked from commit 7198faf)
  • Loading branch information
juliaElastic committed Nov 28, 2022
1 parent e1a7551 commit 0de6f54
Show file tree
Hide file tree
Showing 9 changed files with 231 additions and 39 deletions.
2 changes: 1 addition & 1 deletion x-pack/plugins/fleet/cypress/e2e/integrations_real.cy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ describe('Add Integration - Real API', () => {
setupIntegrations();
cy.getBySel(getIntegrationCategories('aws')).click();
cy.getBySel(INTEGRATIONS_SEARCHBAR.BADGE).contains('AWS').should('exist');
cy.getBySel(INTEGRATION_LIST).find('.euiCard').should('have.length', 28);
cy.getBySel(INTEGRATION_LIST).find('.euiCard').should('have.length', 29);

cy.getBySel(INTEGRATIONS_SEARCHBAR.INPUT).clear().type('Cloud');
cy.getBySel(INTEGRATION_LIST).find('.euiCard').should('have.length', 3);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ export async function storedPackagePoliciesToAgentPermissions(
const permissionEntries = (packagePolicies as PackagePolicy[]).map<Promise<[string, any]>>(
async (packagePolicy) => {
if (!packagePolicy.package) {
throw new Error(`No package for package policy ${packagePolicy.name}`);
throw new Error(`No package for package policy ${packagePolicy.name ?? packagePolicy.id}`);
}

const pkg = packageInfoCache.get(pkgToPkgKey(packagePolicy.package))!;
Expand Down
33 changes: 33 additions & 0 deletions x-pack/plugins/fleet/server/services/epm/archive/storage.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import { SavedObjectsErrorHelpers } from '@kbn/core-saved-objects-utils-server';

import { getAsset } from './storage';

jest.mock('../../app_context', () => {
return {
appContextService: {
getLogger: jest.fn().mockReturnValue({
warn: jest.fn(),
}),
},
};
});

describe('getAsset', () => {
it('should not throw error if saved object not found', async () => {
const soClientMock = {
get: jest.fn().mockRejectedValue(SavedObjectsErrorHelpers.createGenericNotFoundError()),
} as any;
const result = await getAsset({
savedObjectsClient: soClientMock,
path: 'path',
});
expect(result).toBeUndefined();
});
});
27 changes: 18 additions & 9 deletions x-pack/plugins/fleet/server/services/epm/archive/storage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { isBinaryFile } from 'isbinaryfile';
import mime from 'mime-types';
import uuidv5 from 'uuid/v5';
import type { SavedObjectsClientContract, SavedObjectsBulkCreateObject } from '@kbn/core/server';
import { SavedObjectsErrorHelpers } from '@kbn/core/server';

import { ASSETS_SAVED_OBJECT_TYPE } from '../../../../common';
import type {
Expand Down Expand Up @@ -157,16 +158,24 @@ export async function getAsset(opts: {
path: string;
}) {
const { savedObjectsClient, path } = opts;
const assetSavedObject = await savedObjectsClient.get<PackageAsset>(
ASSETS_SAVED_OBJECT_TYPE,
assetPathToObjectId(path)
);
const storedAsset = assetSavedObject?.attributes;
if (!storedAsset) {
return;
}
try {
const assetSavedObject = await savedObjectsClient.get<PackageAsset>(
ASSETS_SAVED_OBJECT_TYPE,
assetPathToObjectId(path)
);
const storedAsset = assetSavedObject?.attributes;
if (!storedAsset) {
return;
}

return storedAsset;
return storedAsset;
} catch (error) {
if (SavedObjectsErrorHelpers.isNotFoundError(error)) {
appContextService.getLogger().warn(error.message);
return;
}
throw error;
}
}

export const getEsPackage = async (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { taggableTypes } from '@kbn/saved-objects-tagging-plugin/common/constant
import type { IAssignmentService, ITagsClient } from '@kbn/saved-objects-tagging-plugin/server';

import type { KibanaAssetType } from '../../../../../common';
import { appContextService } from '../../../app_context';

import type { ArchiveAsset } from './install';
import { KibanaSavedObjectTypeMapping } from './install';
Expand Down Expand Up @@ -44,12 +45,20 @@ export async function tagKibanaAssets(opts: TagAssetsParams) {
ensurePackageTag(opts),
]);

await savedObjectTagAssignmentService.updateTagAssignments({
tags: [managedTagId, packageTagId],
assign: taggableAssets,
unassign: [],
refresh: false,
});
try {
await savedObjectTagAssignmentService.updateTagAssignments({
tags: [managedTagId, packageTagId],
assign: taggableAssets,
unassign: [],
refresh: false,
});
} catch (error) {
if (error.status === 404) {
appContextService.getLogger().warn(error.message);
return;
}
throw error;
}
}

function getTaggableAssets(kibanaAssets: TagAssetsParams['kibanaAssets']) {
Expand Down
57 changes: 47 additions & 10 deletions x-pack/plugins/fleet/server/services/epm/packages/cleanup.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,16 +30,35 @@ describe(' Cleanup old assets', () => {
let removeArchiveEntriesMock: jest.MockedFunction<typeof storage.removeArchiveEntries>;

function mockFindVersions(versions: string[]) {
soClient.find.mockResolvedValue({
page: 0,
per_page: 0,
total: 0,
saved_objects: [],
aggregations: {
versions: {
buckets: versions.map((v) => ({ key: '0.3.3' })),
},
},
soClient.find.mockImplementation((options: any): Promise<any> => {
if (options.type === 'epm-packages-assets') {
return Promise.resolve({
page: 0,
per_page: 0,
total: 0,
saved_objects: [],
aggregations: {
versions: {
buckets: versions.map((v) => ({ key: '0.3.3' })),
},
},
});
} else if (options.type === 'epm-packages') {
return Promise.resolve({
saved_objects: [
{
attributes: {
package_assets: [
{
id: 'asset1',
},
],
},
},
],
});
}
return Promise.resolve({});
});
}

Expand Down Expand Up @@ -80,6 +99,24 @@ describe(' Cleanup old assets', () => {
expect(removeArchiveEntriesMock).not.toHaveBeenCalled();
});

it('should not remove asset referened by epm-packages', async () => {
mockFindVersions(['0.3.3']);
packagePolicyServiceMock.list.mockResolvedValue({ total: 0, items: [], page: 0, perPage: 0 });
soClient.createPointInTimeFinder = jest.fn().mockResolvedValue({
close: jest.fn(),
find: function* asyncGenerator() {
yield { saved_objects: [{ id: 'asset1' }, { id: '2' }] };
},
});

await removeOldAssets({ soClient, pkgName: 'apache', currentVersion: '1.0.0' });

expect(removeArchiveEntriesMock).toHaveBeenCalledWith({
savedObjectsClient: soClient,
refs: [{ id: '2', type: 'epm-packages-assets' }],
});
});

it('should remove old assets from all pages', async () => {
mockFindVersions(['0.3.3']);
packagePolicyServiceMock.list.mockResolvedValue({ total: 0, items: [], page: 0, perPage: 0 });
Expand Down
25 changes: 21 additions & 4 deletions x-pack/plugins/fleet/server/services/epm/packages/cleanup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,11 @@ import type { SavedObjectsClientContract } from '@kbn/core/server';

import { removeArchiveEntries } from '../archive/storage';

import { ASSETS_SAVED_OBJECT_TYPE, PACKAGE_POLICY_SAVED_OBJECT_TYPE } from '../../../../common';
import {
ASSETS_SAVED_OBJECT_TYPE,
PACKAGE_POLICY_SAVED_OBJECT_TYPE,
PACKAGES_SAVED_OBJECT_TYPE,
} from '../../../../common';
import type { PackageAssetReference } from '../../../../common/types';
import { packagePolicyService } from '../../package_policy';
import { appContextService } from '../..';
Expand Down Expand Up @@ -37,15 +41,26 @@ export async function removeOldAssets(options: {
(obj: { key: string }) => obj.key
);

const packageAssetRefsRes = await soClient.find({
type: PACKAGES_SAVED_OBJECT_TYPE,
filter: `${PACKAGES_SAVED_OBJECT_TYPE}.attributes.name:${pkgName}`,
fields: [`${PACKAGES_SAVED_OBJECT_TYPE}.package_assets`],
});

const packageAssetRefs = (
(packageAssetRefsRes.saved_objects?.[0]?.attributes as any)?.package_assets ?? []
).map((ref: any) => ref.id);

for (const oldVersion of oldVersions) {
await removeAssetsFromVersion(soClient, pkgName, oldVersion);
await removeAssetsFromVersion(soClient, pkgName, oldVersion, packageAssetRefs);
}
}

async function removeAssetsFromVersion(
soClient: SavedObjectsClientContract,
pkgName: string,
oldVersion: string
oldVersion: string,
packageAssetRefs: string[]
) {
// check if any policies are using this package version
const { total } = await packagePolicyService.list(soClient, {
Expand Down Expand Up @@ -73,8 +88,10 @@ async function removeAssetsFromVersion(
const refs = assets.saved_objects.map(
(obj) => ({ id: obj.id, type: ASSETS_SAVED_OBJECT_TYPE } as PackageAssetReference)
);
// only delete epm-packages-assets that are not referenced by epm-packages
const unusedRefs = refs.filter((ref) => !packageAssetRefs.includes(ref.id));

await removeArchiveEntries({ savedObjectsClient: soClient, refs });
await removeArchiveEntries({ savedObjectsClient: soClient, refs: unusedRefs });
}
await finder.close();
}
69 changes: 69 additions & 0 deletions x-pack/plugins/fleet/server/services/epm/packages/remove.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import { packagePolicyService } from '../..';

import { removeInstallation } from './remove';

jest.mock('../..', () => {
return {
appContextService: {
getLogger: jest.fn().mockReturnValue({
info: jest.fn(),
error: jest.fn(),
}),
},
packagePolicyService: {
list: jest.fn().mockResolvedValue({ total: 1, items: [{ id: 'system-1' }] }),
delete: jest.fn(),
},
};
});

const mockPackagePolicyService = packagePolicyService as jest.Mocked<typeof packagePolicyService>;

describe('removeInstallation', () => {
let soClientMock: any;
const esClientMock = {} as any;
beforeEach(() => {
soClientMock = {
get: jest.fn().mockResolvedValue({ attributes: { installed_kibana: [], installed_es: [] } }),
delete: jest.fn(),
find: jest.fn().mockResolvedValue({ saved_objects: [] }),
bulkResolve: jest.fn().mockResolvedValue({ resolved_objects: [] }),
} as any;
});
it('should remove package policies when force', async () => {
await removeInstallation({
savedObjectsClient: soClientMock,
pkgName: 'system',
pkgVersion: '1.0.0',
esClient: esClientMock,
force: true,
});
expect(mockPackagePolicyService.delete).toHaveBeenCalledWith(
expect.anything(),
expect.anything(),
['system-1'],
{ force: true }
);
});

it('should throw when trying to remove package with package policies when not force', async () => {
await expect(
removeInstallation({
savedObjectsClient: soClientMock,
pkgName: 'system',
pkgVersion: '1.0.0',
esClient: esClientMock,
force: false,
})
).rejects.toThrowError(
`unable to remove package with existing package policy(s) in use by agent(s)`
);
});
});
34 changes: 26 additions & 8 deletions x-pack/plugins/fleet/server/services/epm/packages/remove.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,11 @@ import { DEFAULT_SPACE_ID } from '@kbn/spaces-plugin/common/constants';

import { SavedObjectsUtils, SavedObjectsErrorHelpers } from '@kbn/core/server';

import { PACKAGE_POLICY_SAVED_OBJECT_TYPE, PACKAGES_SAVED_OBJECT_TYPE } from '../../../constants';
import {
PACKAGE_POLICY_SAVED_OBJECT_TYPE,
PACKAGES_SAVED_OBJECT_TYPE,
SO_SEARCH_LIMIT,
} from '../../../constants';
import { ElasticsearchAssetType } from '../../../types';
import type {
AssetReference,
Expand Down Expand Up @@ -48,16 +52,30 @@ export async function removeInstallation(options: {
const installation = await getInstallation({ savedObjectsClient, pkgName });
if (!installation) throw Boom.badRequest(`${pkgName} is not installed`);

const { total } = await packagePolicyService.list(savedObjectsClient, {
const { total, items } = await packagePolicyService.list(savedObjectsClient, {
kuery: `${PACKAGE_POLICY_SAVED_OBJECT_TYPE}.package.name:${pkgName}`,
page: 0,
perPage: 0,
page: 1,
perPage: options.force ? SO_SEARCH_LIMIT : 0,
});

if (total > 0)
throw Boom.badRequest(
`unable to remove package with existing package policy(s) in use by agent(s)`
);
if (total > 0) {
if (options.force) {
// delete package policies
const ids = items.map((item) => item.id);
appContextService
.getLogger()
.info(
`deleting package policies of ${pkgName} package because force flag was enabled: ${ids}`
);
await packagePolicyService.delete(savedObjectsClient, esClient, ids, {
force: options.force,
});
} else {
throw Boom.badRequest(
`unable to remove package with existing package policy(s) in use by agent(s)`
);
}
}

// Delete the installed assets. Don't include installation.package_assets. Those are irrelevant to users
const installedAssets = [...installation.installed_kibana, ...installation.installed_es];
Expand Down

0 comments on commit 0de6f54

Please sign in to comment.