From 337da71e57efa0ad6dd38a425322138c83a0c216 Mon Sep 17 00:00:00 2001 From: criamico Date: Wed, 22 Nov 2023 11:44:14 +0100 Subject: [PATCH 01/10] [Fleet] Fix epm endpoints return errors --- .../fleet/server/routes/epm/handlers.ts | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/x-pack/plugins/fleet/server/routes/epm/handlers.ts b/x-pack/plugins/fleet/server/routes/epm/handlers.ts index 8ce1bf7006f6b..974f66d9e7fad 100644 --- a/x-pack/plugins/fleet/server/routes/epm/handlers.ts +++ b/x-pack/plugins/fleet/server/routes/epm/handlers.ts @@ -407,7 +407,12 @@ export const installPackageFromRegistryHandler: FleetRequestHandler< }; return response.ok({ body }); } else { - return await defaultFleetErrorHandler({ error: res.error, response }); + return response.customError({ + statusCode: 400, + body: { + message: res.error.message, + }, + }); } }; export const createCustomIntegrationHandler: FleetRequestHandler< @@ -548,7 +553,17 @@ export const installPackageByUploadHandler: FleetRequestHandler< }; return response.ok({ body }); } else { - return defaultFleetErrorHandler({ error: res.error, response }); + // return 500 only for bundled packages + if (res.installSource === 'bundled') { + return defaultFleetErrorHandler({ error: res.error, response }); + } + // all the other errors should return 400 + return response.customError({ + statusCode: 400, + body: { + message: res.error.message, + }, + }); } }; From 37a4f5f34b77903e710cc4ccbba8408646c10b6d Mon Sep 17 00:00:00 2001 From: criamico Date: Wed, 22 Nov 2023 17:05:35 +0100 Subject: [PATCH 02/10] Better handling of epm errors --- .../plugins/fleet/server/errors/handlers.ts | 18 ++++++++++++++- x-pack/plugins/fleet/server/errors/index.ts | 15 ++++++------ .../fleet/server/routes/epm/handlers.ts | 19 ++------------- .../fleet/server/services/epm/agent/agent.ts | 5 ++-- .../server/services/epm/package_service.ts | 4 ++-- .../services/epm/packages/bundled_packages.ts | 8 ++++--- .../fleet/server/services/epm/packages/get.ts | 9 +++++--- .../server/services/epm/packages/install.ts | 14 +++++------ .../server/services/epm/packages/remove.ts | 23 +++++++++---------- .../server/services/epm/packages/update.ts | 2 +- .../server/services/epm/registry/index.ts | 8 ++++++- .../apis/epm/install_update.ts | 2 +- 12 files changed, 70 insertions(+), 57 deletions(-) diff --git a/x-pack/plugins/fleet/server/errors/handlers.ts b/x-pack/plugins/fleet/server/errors/handlers.ts index edc34d50598ec..43aa493c353b1 100644 --- a/x-pack/plugins/fleet/server/errors/handlers.ts +++ b/x-pack/plugins/fleet/server/errors/handlers.ts @@ -34,6 +34,10 @@ import { PackagePolicyNotFoundError, FleetUnauthorizedError, PackagePolicyNameExistsError, + PackageOutdatedError, + PackageInvalidArchiveError, + BundledPackageLocationNotFoundError, + PackageRemovalError, } from '.'; type IngestErrorHandler = ( @@ -69,6 +73,15 @@ const getHTTPResponseCode = (error: FleetError): number => { if (error instanceof PackageFailedVerificationError) { return 400; // Bad Request } + if (error instanceof PackageOutdatedError) { + return 400; // Bad Request + } + if (error instanceof PackageInvalidArchiveError) { + return 400; // Bad Request + } + if (error instanceof PackageRemovalError) { + return 400; // Bad Request + } if (error instanceof ConcurrentInstallOperationError) { return 409; // Conflict } @@ -87,6 +100,9 @@ const getHTTPResponseCode = (error: FleetError): number => { if (error instanceof UninstallTokenError) { return 500; // Internal Error } + if (error instanceof BundledPackageLocationNotFoundError) { + return 500; // Internal Error + } return 400; // Bad Request }; @@ -115,7 +131,7 @@ export function fleetErrorToResponseOptions(error: IngestErrorHandlerParams['err }; } - // not sure what type of error this is. log as much as possible + // default response is 500 logger.error(error); return { statusCode: 500, diff --git a/x-pack/plugins/fleet/server/errors/index.ts b/x-pack/plugins/fleet/server/errors/index.ts index 0b2c6b0fc5e93..0d5ed8fb1e685 100644 --- a/x-pack/plugins/fleet/server/errors/index.ts +++ b/x-pack/plugins/fleet/server/errors/index.ts @@ -26,8 +26,9 @@ export class RegistryResponseError extends RegistryError { super(message); } } + +// Package errors export class PackageNotFoundError extends FleetError {} -export class PackageKeyInvalidError extends FleetError {} export class PackageOutdatedError extends FleetError {} export class PackageFailedVerificationError extends FleetError { constructor(pkgName: string, pkgVersion: string) { @@ -37,22 +38,22 @@ export class PackageFailedVerificationError extends FleetError { }; } } +export class PackageUnsupportedMediaTypeError extends FleetError {} +export class PackageInvalidArchiveError extends FleetError {} +export class PackageRemovalError extends FleetError {} +export class ConcurrentInstallOperationError extends FleetError {} +export class BundledPackageLocationNotFoundError extends FleetError {} + export class AgentPolicyError extends FleetError {} export class AgentPolicyNotFoundError extends FleetError {} export class AgentNotFoundError extends FleetError {} export class AgentActionNotFoundError extends FleetError {} export class AgentPolicyNameExistsError extends AgentPolicyError {} -export class PackageUnsupportedMediaTypeError extends FleetError {} -export class PackageInvalidArchiveError extends FleetError {} -export class PackageCacheError extends FleetError {} -export class PackageOperationNotSupportedError extends FleetError {} -export class ConcurrentInstallOperationError extends FleetError {} export class AgentReassignmentError extends FleetError {} export class PackagePolicyIneligibleForUpgradeError extends FleetError {} export class PackagePolicyValidationError extends FleetError {} export class PackagePolicyNameExistsError extends FleetError {} export class PackagePolicyNotFoundError extends FleetError {} -export class BundledPackageNotFoundError extends FleetError {} export class HostedAgentPolicyRestrictionRelatedError extends FleetError { constructor(message = 'Cannot perform that action') { super( diff --git a/x-pack/plugins/fleet/server/routes/epm/handlers.ts b/x-pack/plugins/fleet/server/routes/epm/handlers.ts index 974f66d9e7fad..8ce1bf7006f6b 100644 --- a/x-pack/plugins/fleet/server/routes/epm/handlers.ts +++ b/x-pack/plugins/fleet/server/routes/epm/handlers.ts @@ -407,12 +407,7 @@ export const installPackageFromRegistryHandler: FleetRequestHandler< }; return response.ok({ body }); } else { - return response.customError({ - statusCode: 400, - body: { - message: res.error.message, - }, - }); + return await defaultFleetErrorHandler({ error: res.error, response }); } }; export const createCustomIntegrationHandler: FleetRequestHandler< @@ -553,17 +548,7 @@ export const installPackageByUploadHandler: FleetRequestHandler< }; return response.ok({ body }); } else { - // return 500 only for bundled packages - if (res.installSource === 'bundled') { - return defaultFleetErrorHandler({ error: res.error, response }); - } - // all the other errors should return 400 - return response.customError({ - statusCode: 400, - body: { - message: res.error.message, - }, - }); + return defaultFleetErrorHandler({ error: res.error, response }); } }; diff --git a/x-pack/plugins/fleet/server/services/epm/agent/agent.ts b/x-pack/plugins/fleet/server/services/epm/agent/agent.ts index 077aa720b96d8..a665870303cce 100644 --- a/x-pack/plugins/fleet/server/services/epm/agent/agent.ts +++ b/x-pack/plugins/fleet/server/services/epm/agent/agent.ts @@ -10,6 +10,7 @@ import { safeLoad, safeDump } from 'js-yaml'; import type { PackagePolicyConfigRecord } from '../../../../common/types'; import { toCompiledSecretRef } from '../../secrets'; +import { FleetError } from '../../../errors'; const handlebars = Handlebars.create(); @@ -20,7 +21,7 @@ export function compileTemplate(variables: PackagePolicyConfigRecord, templateSt const template = handlebars.compile(templateStr, { noEscape: true }); compiledTemplate = template(vars); } catch (err) { - throw new Error(`Error while compiling agent template: ${err.message}`); + throw new FleetError(`Error while compiling agent template: ${err.message}`); } compiledTemplate = replaceRootLevelYamlVariables(yamlValues, compiledTemplate); @@ -78,7 +79,7 @@ function buildTemplateVariables(variables: PackagePolicyConfigRecord, templateSt let varPart = acc; for (const keyPart of keyParts) { if (!isValidKey(keyPart)) { - throw new Error('Invalid key'); + throw new FleetError(`Invalid key ${keyPart}`); } if (!varPart[keyPart]) { varPart[keyPart] = {}; diff --git a/x-pack/plugins/fleet/server/services/epm/package_service.ts b/x-pack/plugins/fleet/server/services/epm/package_service.ts index 39ca950af93db..e6f71cb7cb96c 100644 --- a/x-pack/plugins/fleet/server/services/epm/package_service.ts +++ b/x-pack/plugins/fleet/server/services/epm/package_service.ts @@ -29,7 +29,7 @@ import type { } from '../../types'; import type { FleetAuthzRouteConfig } from '../security/types'; import { checkSuperuser, getAuthzFromRequest, doesNotHaveRequiredFleetAuthz } from '../security'; -import { FleetUnauthorizedError } from '../../errors'; +import { FleetUnauthorizedError, FleetError } from '../../errors'; import { INSTALL_PACKAGES_AUTHZ, READ_PACKAGE_INFO_AUTHZ } from '../../routes/epm'; import { installTransforms, isTransform } from './elasticsearch/transform/install'; @@ -208,7 +208,7 @@ class PackageClientImpl implements PackageClient { const transformPaths = assetPaths.filter(isTransform); if (transformPaths.length !== assetPaths.length) { - throw new Error('reinstallEsAssets is currently only implemented for transform assets'); + throw new FleetError('reinstallEsAssets is currently only implemented for transform assets'); } if (transformPaths.length) { diff --git a/x-pack/plugins/fleet/server/services/epm/packages/bundled_packages.ts b/x-pack/plugins/fleet/server/services/epm/packages/bundled_packages.ts index 7078761a4a583..92f9674656103 100644 --- a/x-pack/plugins/fleet/server/services/epm/packages/bundled_packages.ts +++ b/x-pack/plugins/fleet/server/services/epm/packages/bundled_packages.ts @@ -9,7 +9,7 @@ import fs from 'fs/promises'; import path from 'path'; import type { BundledPackage, Installation } from '../../../types'; -import { FleetError } from '../../../errors'; +import { BundledPackageLocationNotFoundError } from '../../../errors'; import { appContextService } from '../../app_context'; import { splitPkgKey, pkgToPkgKey } from '../registry'; @@ -19,7 +19,9 @@ export async function getBundledPackages(): Promise { const bundledPackageLocation = config?.developer?.bundledPackageLocation; if (!bundledPackageLocation) { - throw new FleetError('xpack.fleet.developer.bundledPackageLocation is not configured'); + throw new BundledPackageLocationNotFoundError( + 'xpack.fleet.developer.bundledPackageLocation is not configured' + ); } // If the bundled package directory is missing, we log a warning during setup, @@ -51,7 +53,7 @@ export async function getBundledPackages(): Promise { return result; } catch (err) { const logger = appContextService.getLogger(); - logger.debug(`Unable to read bundled packages from ${bundledPackageLocation}`); + logger.warn(`Unable to read bundled packages from ${bundledPackageLocation}`); return []; } diff --git a/x-pack/plugins/fleet/server/services/epm/packages/get.ts b/x-pack/plugins/fleet/server/services/epm/packages/get.ts index 68a884fd5f198..2c642802602b6 100644 --- a/x-pack/plugins/fleet/server/services/epm/packages/get.ts +++ b/x-pack/plugins/fleet/server/services/epm/packages/get.ts @@ -44,7 +44,6 @@ import type { } from '../../../../common/types'; import type { Installation, PackageInfo, PackagePolicySOAttributes } from '../../../types'; import { - FleetError, PackageFailedVerificationError, PackageNotFoundError, RegistryResponseError, @@ -118,7 +117,7 @@ export async function getPackages( return createInstallableFrom({ ...packageInfo, id: pkg.id }, pkg); } catch (err) { if (err instanceof PackageInvalidArchiveError) { - logger.warn( + logger.error( `Installed package ${pkg.id} ${pkg.attributes.version} is not a valid package anymore` ); return null; @@ -417,12 +416,14 @@ export async function getPackageInfo({ ignoreUnverified?: boolean; prerelease?: boolean; }): Promise { + const logger = appContextService.getLogger(); const [savedObject, latestPackage] = await Promise.all([ getInstallationObject({ savedObjectsClient, pkgName }), Registry.fetchFindLatestPackageOrUndefined(pkgName, { prerelease }), ]); if (!savedObject && !latestPackage) { + logger.error(`[${pkgName}] package not installed or found in registry`); throw new PackageNotFoundError(`[${pkgName}] package not installed or found in registry`); } @@ -575,6 +576,7 @@ export async function getPackageFromSource(options: { logger.debug(`retrieved installed package ${pkgName}-${pkgVersion}`); } catch (error) { if (error instanceof PackageFailedVerificationError) { + logger.error(`package ${pkgName}-${pkgVersion} failed verification`); throw error; } // treating this is a 404 as no status code returned @@ -600,7 +602,8 @@ export async function getPackageFromSource(options: { } } if (!res) { - throw new FleetError(`package info for ${pkgName}-${pkgVersion} does not exist`); + logger.error(`Package info for ${pkgName}-${pkgVersion} does not exist`); + throw new PackageNotFoundError(`Package info for ${pkgName}-${pkgVersion} does not exist`); } return { paths: res.paths, diff --git a/x-pack/plugins/fleet/server/services/epm/packages/install.ts b/x-pack/plugins/fleet/server/services/epm/packages/install.ts index 3a7fc78096159..ac0be0b7fb34e 100644 --- a/x-pack/plugins/fleet/server/services/epm/packages/install.ts +++ b/x-pack/plugins/fleet/server/services/epm/packages/install.ts @@ -57,7 +57,7 @@ import { DATASET_VAR_NAME, } from '../../../../common/constants'; import { - type FleetError, + FleetError, PackageOutdatedError, PackagePolicyValidationError, ConcurrentInstallOperationError, @@ -202,7 +202,7 @@ export async function ensureInstalledPackage(options: { } const installation = await getInstallation({ savedObjectsClient, pkgName }); - if (!installation) throw new Error(`could not get installation ${pkgName}`); + if (!installation) throw new FleetError(`Could not get installation ${pkgName}`); return installation; } @@ -712,7 +712,7 @@ export type InstallPackageParams = { export async function installPackage(args: InstallPackageParams): Promise { if (!('installSource' in args)) { - throw new Error('installSource is required'); + throw new FleetError('installSource is required'); } const logger = appContextService.getLogger(); @@ -803,7 +803,7 @@ export async function installPackage(args: InstallPackageParams): Promise { + const logger = appContextService.getLogger(); const { savedObjectsClient, pkgName, pkgVersion, esClient } = options; const installation = await getInstallation({ savedObjectsClient, pkgName }); - if (!installation) throw Boom.badRequest(`${pkgName} is not installed`); + if (!installation) throw new PackageRemovalError(`${pkgName} is not installed`); const { total, items } = await packagePolicyService.list(savedObjectsClient, { kuery: `${PACKAGE_POLICY_SAVED_OBJECT_TYPE}.package.name:${pkgName}`, @@ -72,17 +72,16 @@ export async function removeInstallation(options: { if (options.force || items.every((item) => (item.agents ?? 0) === 0)) { // delete package policies const ids = items.map((item) => item.id); - appContextService - .getLogger() - .info( - `deleting package policies of ${pkgName} package because not used by agents or force flag was enabled: ${ids}` - ); + logger.info( + `Deleting package policies of ${pkgName} package because not used by agents or 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)` + logger.warn(`Unable to remove package with existing package policy(s) in use by agent(s)`); + throw new PackageRemovalError( + `Unable to remove package with existing package policy(s) in use by agent(s)` ); } } @@ -242,7 +241,7 @@ async function deleteIndexTemplate(esClient: ElasticsearchClient, name: string): try { await esClient.indices.deleteIndexTemplate({ name }, { ignore: [404] }); } catch { - throw new Error(`error deleting index template ${name}`); + throw new FleetError(`Error deleting index template ${name}`); } } } @@ -253,7 +252,7 @@ async function deleteComponentTemplate(esClient: ElasticsearchClient, name: stri try { await esClient.cluster.deleteComponentTemplate({ name }, { ignore: [404] }); } catch (error) { - throw new Error(`error deleting component template ${name}`); + throw new FleetError(`Error deleting component template ${name}`); } } } diff --git a/x-pack/plugins/fleet/server/services/epm/packages/update.ts b/x-pack/plugins/fleet/server/services/epm/packages/update.ts index 3072dfed86636..7159571c29d6d 100644 --- a/x-pack/plugins/fleet/server/services/epm/packages/update.ts +++ b/x-pack/plugins/fleet/server/services/epm/packages/update.ts @@ -29,7 +29,7 @@ export async function updatePackage( const installedPackage = await getInstallationObject({ savedObjectsClient, pkgName }); if (!installedPackage) { - throw new FleetError(`package ${pkgName} is not installed`); + throw new FleetError(`Package ${pkgName} is not installed`); } auditLoggingService.writeCustomSoAuditLog({ diff --git a/x-pack/plugins/fleet/server/services/epm/registry/index.ts b/x-pack/plugins/fleet/server/services/epm/registry/index.ts index 487faf55730bd..ca0d53092d294 100644 --- a/x-pack/plugins/fleet/server/services/epm/registry/index.ts +++ b/x-pack/plugins/fleet/server/services/epm/registry/index.ts @@ -43,6 +43,7 @@ import { PackageNotFoundError, RegistryResponseError, PackageFailedVerificationError, + PackageUnsupportedMediaTypeError, } from '../../../errors'; import { getBundledPackageByName } from '../packages/bundled_packages'; @@ -363,9 +364,14 @@ export async function getPackage( } function ensureContentType(archivePath: string) { + const logger = appContextService.getLogger(); const contentType = mime.lookup(archivePath); + if (!contentType) { - throw new Error(`Unknown compression format for '${archivePath}'. Please use .zip or .gz`); + logger.error(`Unknown compression format for '${archivePath}'. Please use .zip or .gz`); + throw new PackageUnsupportedMediaTypeError( + `Unknown compression format for '${archivePath}'. Please use .zip or .gz` + ); } return contentType; } diff --git a/x-pack/test/fleet_api_integration/apis/epm/install_update.ts b/x-pack/test/fleet_api_integration/apis/epm/install_update.ts index c36efd6066b6e..8dd99d4d99a95 100644 --- a/x-pack/test/fleet_api_integration/apis/epm/install_update.ts +++ b/x-pack/test/fleet_api_integration/apis/epm/install_update.ts @@ -32,7 +32,7 @@ export default function (providerContext: FtrProviderContext) { }); it('should return 404 if package does not exist', async function () { - await supertest + const response = await supertest .post(`/api/fleet/epm/packages/nonexistent/0.1.0`) .set('kbn-xsrf', 'xxxx') .expect(404); From 1cf37027dc0b13ba6151e3dfb77b71ef63db7829 Mon Sep 17 00:00:00 2001 From: criamico Date: Wed, 22 Nov 2023 17:37:46 +0100 Subject: [PATCH 03/10] fix linter --- x-pack/test/fleet_api_integration/apis/epm/install_update.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/test/fleet_api_integration/apis/epm/install_update.ts b/x-pack/test/fleet_api_integration/apis/epm/install_update.ts index 8dd99d4d99a95..c36efd6066b6e 100644 --- a/x-pack/test/fleet_api_integration/apis/epm/install_update.ts +++ b/x-pack/test/fleet_api_integration/apis/epm/install_update.ts @@ -32,7 +32,7 @@ export default function (providerContext: FtrProviderContext) { }); it('should return 404 if package does not exist', async function () { - const response = await supertest + await supertest .post(`/api/fleet/epm/packages/nonexistent/0.1.0`) .set('kbn-xsrf', 'xxxx') .expect(404); From 8fb60396d070f7961b7ff358652a8fd54f6e5976 Mon Sep 17 00:00:00 2001 From: criamico Date: Thu, 23 Nov 2023 10:17:32 +0100 Subject: [PATCH 04/10] Handle addtional errors --- .../plugins/fleet/server/errors/handlers.ts | 22 ++++++++++--- x-pack/plugins/fleet/server/errors/index.ts | 3 ++ .../server/services/epm/archive/storage.ts | 12 +++++-- .../services/epm/elasticsearch/ilm/install.ts | 4 ++- .../epm/elasticsearch/template/template.ts | 32 +++++++++++++------ .../services/epm/kibana/assets/install.ts | 15 +++++++-- .../fleet/server/services/epm/packages/get.ts | 2 +- .../server/services/epm/packages/reinstall.ts | 8 +++-- .../services/epm/packages/remove.test.ts | 3 +- 9 files changed, 77 insertions(+), 24 deletions(-) diff --git a/x-pack/plugins/fleet/server/errors/handlers.ts b/x-pack/plugins/fleet/server/errors/handlers.ts index 43aa493c353b1..8d12c17120eb8 100644 --- a/x-pack/plugins/fleet/server/errors/handlers.ts +++ b/x-pack/plugins/fleet/server/errors/handlers.ts @@ -38,6 +38,9 @@ import { PackageInvalidArchiveError, BundledPackageLocationNotFoundError, PackageRemovalError, + PackageESError, + KibanaSOReferenceError, + PackageAlreadyInstalledError, } from '.'; type IngestErrorHandler = ( @@ -64,9 +67,6 @@ const getHTTPResponseCode = (error: FleetError): number => { if (error instanceof PackageNotFoundError || error instanceof PackagePolicyNotFoundError) { return 404; // Not Found } - if (error instanceof AgentPolicyNameExistsError) { - return 409; // Conflict - } if (error instanceof PackageUnsupportedMediaTypeError) { return 415; // Unsupported Media Type } @@ -82,8 +82,8 @@ const getHTTPResponseCode = (error: FleetError): number => { if (error instanceof PackageRemovalError) { return 400; // Bad Request } - if (error instanceof ConcurrentInstallOperationError) { - return 409; // Conflict + if (error instanceof KibanaSOReferenceError) { + return 400; // Bad Request } if (error instanceof AgentNotFoundError) { return 404; @@ -94,15 +94,27 @@ const getHTTPResponseCode = (error: FleetError): number => { if (error instanceof FleetUnauthorizedError) { return 403; // Unauthorized } + if (error instanceof AgentPolicyNameExistsError) { + return 409; // Conflict + } + if (error instanceof ConcurrentInstallOperationError) { + return 409; // Conflict + } if (error instanceof PackagePolicyNameExistsError) { return 409; // Conflict } + if (error instanceof PackageAlreadyInstalledError) { + return 409; // Conflict + } if (error instanceof UninstallTokenError) { return 500; // Internal Error } if (error instanceof BundledPackageLocationNotFoundError) { return 500; // Internal Error } + if (error instanceof PackageESError) { + return 500; // Internal Error + } return 400; // Bad Request }; diff --git a/x-pack/plugins/fleet/server/errors/index.ts b/x-pack/plugins/fleet/server/errors/index.ts index 0d5ed8fb1e685..7f607f4692774 100644 --- a/x-pack/plugins/fleet/server/errors/index.ts +++ b/x-pack/plugins/fleet/server/errors/index.ts @@ -41,8 +41,11 @@ export class PackageFailedVerificationError extends FleetError { export class PackageUnsupportedMediaTypeError extends FleetError {} export class PackageInvalidArchiveError extends FleetError {} export class PackageRemovalError extends FleetError {} +export class PackageESError extends FleetError {} export class ConcurrentInstallOperationError extends FleetError {} export class BundledPackageLocationNotFoundError extends FleetError {} +export class KibanaSOReferenceError extends FleetError {} +export class PackageAlreadyInstalledError extends FleetError {} export class AgentPolicyError extends FleetError {} export class AgentPolicyNotFoundError extends FleetError {} diff --git a/x-pack/plugins/fleet/server/services/epm/archive/storage.ts b/x-pack/plugins/fleet/server/services/epm/archive/storage.ts index cfa110589a010..3f5fdff5f6125 100644 --- a/x-pack/plugins/fleet/server/services/epm/archive/storage.ts +++ b/x-pack/plugins/fleet/server/services/epm/archive/storage.ts @@ -19,6 +19,7 @@ import type { InstallSource, PackageAssetReference, } from '../../../../common/types'; +import { PackageInvalidArchiveError, PackageNotFoundError } from '../../../errors'; import { appContextService } from '../../app_context'; @@ -55,6 +56,7 @@ export async function archiveEntryToESDocument(opts: { version: string; installSource: InstallSource; }): Promise { + const logger = appContextService.getLogger(); const { path, buffer, name, version, installSource } = opts; const fileExt = extname(path); const contentType = mime.lookup(fileExt); @@ -70,13 +72,17 @@ export async function archiveEntryToESDocument(opts: { // validation: filesize? asset type? anything else if (dataUtf8.length > currentMaxAssetBytes) { - throw new Error( + logger.warn(`File at ${path} is larger than maximum allowed size of ${currentMaxAssetBytes}`); + throw new PackageInvalidArchiveError( `File at ${path} is larger than maximum allowed size of ${currentMaxAssetBytes}` ); } if (dataBase64.length > currentMaxAssetBytes) { - throw new Error( + logger.warn( + `After base64 encoding file at ${path} is larger than maximum allowed size of ${currentMaxAssetBytes}` + ); + throw new PackageInvalidArchiveError( `After base64 encoding file at ${path} is larger than maximum allowed size of ${currentMaxAssetBytes}` ); } @@ -113,7 +119,7 @@ export async function saveArchiveEntries(opts: { const bulkBody = await Promise.all( paths.map((path) => { const buffer = getArchiveEntry(path); - if (!buffer) throw new Error(`Could not find ArchiveEntry at ${path}`); + if (!buffer) throw new PackageNotFoundError(`Could not find ArchiveEntry at ${path}`); const { name, version } = packageInfo; return archiveEntryToBulkCreateObject({ path, buffer, name, version, installSource }); }) diff --git a/x-pack/plugins/fleet/server/services/epm/elasticsearch/ilm/install.ts b/x-pack/plugins/fleet/server/services/epm/elasticsearch/ilm/install.ts index 3aa86b526addd..fa70947fdfc0c 100644 --- a/x-pack/plugins/fleet/server/services/epm/elasticsearch/ilm/install.ts +++ b/x-pack/plugins/fleet/server/services/epm/elasticsearch/ilm/install.ts @@ -14,6 +14,7 @@ import { getAsset, getPathParts } from '../../archive'; import { updateEsAssetReferences } from '../../packages/install'; import { getESAssetMetadata } from '../meta'; import { retryTransientEsErrors } from '../retry'; +import { FleetError } from '../../../../errors'; export async function installILMPolicy( packageInfo: InstallablePackage, @@ -57,7 +58,8 @@ export async function installILMPolicy( { logger } ); } catch (err) { - throw new Error(err.message); + logger.warn(`Couldn't install ilm policies: ${err.message}`); + throw new FleetError(`Couldn't install ilm policies: ${err.message}`); } }) ); diff --git a/x-pack/plugins/fleet/server/services/epm/elasticsearch/template/template.ts b/x-pack/plugins/fleet/server/services/epm/elasticsearch/template/template.ts index 622538e152836..9872b61f4eac9 100644 --- a/x-pack/plugins/fleet/server/services/epm/elasticsearch/template/template.ts +++ b/x-pack/plugins/fleet/server/services/epm/elasticsearch/template/template.ts @@ -33,6 +33,7 @@ import { } from '../../../../constants'; import { getESAssetMetadata } from '../meta'; import { retryTransientEsErrors } from '../retry'; +import { FleetError, PackageESError } from '../../../../errors'; import { getDefaultProperties, histogram, keyword, scaledFloat } from './mappings'; @@ -102,7 +103,9 @@ export function getTemplate({ isIndexModeTimeSeries, }); if (template.template.settings.index.final_pipeline) { - throw new Error(`Error template for ${templateIndexPattern} contains a final_pipeline`); + throw new PackageESError( + `Error template for ${templateIndexPattern} contains a final_pipeline` + ); } const esBaseComponents = getBaseEsComponents(type, !!isIndexModeTimeSeries); @@ -405,8 +408,8 @@ function _generateMappings( matchingType = field.object_type_mapping_type ?? 'object'; break; default: - throw new Error( - `no dynamic mapping generated for field ${path} of type ${field.object_type}` + throw new FleetError( + `No dynamic mapping generated for field ${path} of type ${field.object_type}` ); } @@ -871,14 +874,21 @@ const getDataStreams = async ( })); }; -const rolloverDataStream = (dataStreamName: string, esClient: ElasticsearchClient) => { +const rolloverDataStream = ( + dataStreamName: string, + esClient: ElasticsearchClient, + logger: Logger +) => { try { // Do no wrap rollovers in retryTransientEsErrors since it is not idempotent return esClient.indices.rollover({ alias: dataStreamName, }); } catch (error) { - throw new Error(`cannot rollover data stream [${dataStreamName}] due to error: ${error}`); + logger.warn(`Cannot rollover data stream [${dataStreamName}] due to error: ${error}`); + throw new PackageESError( + `Cannot rollover data stream [${dataStreamName}] due to error: ${error}` + ); } }; @@ -983,7 +993,7 @@ const updateExistingDataStream = async ({ return; } else { logger.info(`Triggering a rollover for ${dataStreamName}`); - await rolloverDataStream(dataStreamName, esClient); + await rolloverDataStream(dataStreamName, esClient, logger); return; } } @@ -1007,7 +1017,7 @@ const updateExistingDataStream = async ({ logger.info( `Index mode or source type has changed for ${dataStreamName}, triggering a rollover` ); - await rolloverDataStream(dataStreamName, esClient); + await rolloverDataStream(dataStreamName, esClient, logger); } } @@ -1025,7 +1035,10 @@ const updateExistingDataStream = async ({ { logger } ); } catch (err) { - throw new Error(`could not update lifecycle settings for ${dataStreamName}: ${err.message}`); + logger.warn(`Could not update lifecycle settings for ${dataStreamName}: ${err.message}`); + throw new PackageESError( + `Could not update lifecycle settings for ${dataStreamName}: ${err.message}` + ); } } @@ -1048,6 +1061,7 @@ const updateExistingDataStream = async ({ { logger } ); } catch (err) { - throw new Error(`could not update index template settings for ${dataStreamName}`); + logger.warn(`Could not update index template settings for ${dataStreamName}`); + throw new PackageESError(`Could not update index template settings for ${dataStreamName}`); } }; diff --git a/x-pack/plugins/fleet/server/services/epm/kibana/assets/install.ts b/x-pack/plugins/fleet/server/services/epm/kibana/assets/install.ts index 20a0484c77a4a..51e5c1e70a9a6 100644 --- a/x-pack/plugins/fleet/server/services/epm/kibana/assets/install.ts +++ b/x-pack/plugins/fleet/server/services/epm/kibana/assets/install.ts @@ -35,6 +35,7 @@ import { savedObjectTypes } from '../../packages'; import { indexPatternTypes, getIndexPatternSavedObjects } from '../index_pattern/install'; import { saveKibanaAssetsRefs } from '../../packages/install'; import { deleteKibanaSavedObjectsAssets } from '../../packages/remove'; +import { KibanaSOReferenceError } from '../../../../errors'; import { withPackageSpan } from '../../packages/utils'; @@ -340,7 +341,12 @@ export async function installKibanaSavedObjects({ ); if (otherErrors?.length) { - throw new Error( + logger.warn( + `Encountered ${ + otherErrors.length + } errors creating saved objects: ${formatImportErrorsForLog(otherErrors)}` + ); + throw new KibanaSOReferenceError( `Encountered ${ otherErrors.length } errors creating saved objects: ${formatImportErrorsForLog(otherErrors)}` @@ -383,7 +389,12 @@ export async function installKibanaSavedObjects({ }); if (resolveErrors?.length) { - throw new Error( + logger.warn( + `Encountered ${ + resolveErrors.length + } errors resolving reference errors: ${formatImportErrorsForLog(resolveErrors)}` + ); + throw new KibanaSOReferenceError( `Encountered ${ resolveErrors.length } errors resolving reference errors: ${formatImportErrorsForLog(resolveErrors)}` diff --git a/x-pack/plugins/fleet/server/services/epm/packages/get.ts b/x-pack/plugins/fleet/server/services/epm/packages/get.ts index 2c642802602b6..ab61987447294 100644 --- a/x-pack/plugins/fleet/server/services/epm/packages/get.ts +++ b/x-pack/plugins/fleet/server/services/epm/packages/get.ts @@ -117,7 +117,7 @@ export async function getPackages( return createInstallableFrom({ ...packageInfo, id: pkg.id }, pkg); } catch (err) { if (err instanceof PackageInvalidArchiveError) { - logger.error( + logger.warn( `Installed package ${pkg.id} ${pkg.attributes.version} is not a valid package anymore` ); return null; diff --git a/x-pack/plugins/fleet/server/services/epm/packages/reinstall.ts b/x-pack/plugins/fleet/server/services/epm/packages/reinstall.ts index d2672b6089819..cb2b91de252f7 100644 --- a/x-pack/plugins/fleet/server/services/epm/packages/reinstall.ts +++ b/x-pack/plugins/fleet/server/services/epm/packages/reinstall.ts @@ -11,6 +11,8 @@ import { DEFAULT_SPACE_ID } from '@kbn/spaces-plugin/common/constants'; import type { Installation } from '../../../types'; import { pkgToPkgKey } from '../registry'; +import { PackageNotFoundError, PackageAlreadyInstalledError } from '../../../errors'; + import { getBundledPackageForInstallation } from './bundled_packages'; import { installPackage } from './install'; @@ -29,9 +31,11 @@ export async function reinstallPackageForInstallation({ const matchingBundledPackage = await getBundledPackageForInstallation(installation); if (!matchingBundledPackage) { if (installation.install_source === 'bundled') { - throw new Error(`Cannot reinstall: ${installation.name}, bundled package not found`); + throw new PackageNotFoundError( + `Cannot reinstall: ${installation.name}, bundled package not found` + ); } else { - throw new Error('Cannot reinstall an uploaded package'); + throw new PackageAlreadyInstalledError('Cannot reinstall an uploaded package'); } } } diff --git a/x-pack/plugins/fleet/server/services/epm/packages/remove.test.ts b/x-pack/plugins/fleet/server/services/epm/packages/remove.test.ts index f15043fe52923..badb1bc7c1f40 100644 --- a/x-pack/plugins/fleet/server/services/epm/packages/remove.test.ts +++ b/x-pack/plugins/fleet/server/services/epm/packages/remove.test.ts @@ -18,6 +18,7 @@ jest.mock('../..', () => { getLogger: jest.fn().mockReturnValue({ info: jest.fn(), error: jest.fn(), + warn: jest.fn(), }), }, packagePolicyService: { @@ -78,7 +79,7 @@ describe('removeInstallation', () => { force: false, }) ).rejects.toThrowError( - `unable to remove package with existing package policy(s) in use by agent(s)` + `Unable to remove package with existing package policy(s) in use by agent(s)` ); }); From ea6c281861786404b37867de825ff14e01a65301 Mon Sep 17 00:00:00 2001 From: criamico Date: Thu, 23 Nov 2023 10:51:08 +0100 Subject: [PATCH 05/10] More errors --- .../plugins/fleet/server/errors/handlers.ts | 66 ++++++++++--------- .../fleet/server/services/epm/agent/agent.ts | 16 +++-- .../server/services/epm/packages/install.ts | 9 ++- .../server/services/epm/packages/update.ts | 8 ++- 4 files changed, 58 insertions(+), 41 deletions(-) diff --git a/x-pack/plugins/fleet/server/errors/handlers.ts b/x-pack/plugins/fleet/server/errors/handlers.ts index 8d12c17120eb8..7bd380af258e2 100644 --- a/x-pack/plugins/fleet/server/errors/handlers.ts +++ b/x-pack/plugins/fleet/server/errors/handlers.ts @@ -54,36 +54,30 @@ interface IngestErrorHandlerParams { } // unsure if this is correct. would prefer to use something "official" // this type is based on BadRequest values observed while debugging https://github.com/elastic/kibana/issues/75862 - const getHTTPResponseCode = (error: FleetError): number => { - if (error instanceof RegistryResponseError) { - // 4xx/5xx's from EPR - return 500; - } - if (error instanceof RegistryConnectionError || error instanceof RegistryError) { - // Connection errors (ie. RegistryConnectionError) / fallback (RegistryError) from EPR - return 502; // Bad Gateway - } - if (error instanceof PackageNotFoundError || error instanceof PackagePolicyNotFoundError) { - return 404; // Not Found - } - if (error instanceof PackageUnsupportedMediaTypeError) { - return 415; // Unsupported Media Type - } + // Bad Request if (error instanceof PackageFailedVerificationError) { - return 400; // Bad Request + return 400; } if (error instanceof PackageOutdatedError) { - return 400; // Bad Request + return 400; } if (error instanceof PackageInvalidArchiveError) { - return 400; // Bad Request + return 400; } if (error instanceof PackageRemovalError) { - return 400; // Bad Request + return 400; } if (error instanceof KibanaSOReferenceError) { - return 400; // Bad Request + return 400; + } + // Unauthorized + if (error instanceof FleetUnauthorizedError) { + return 403; + } + // Not Found + if (error instanceof PackageNotFoundError || error instanceof PackagePolicyNotFoundError) { + return 404; } if (error instanceof AgentNotFoundError) { return 404; @@ -91,29 +85,41 @@ const getHTTPResponseCode = (error: FleetError): number => { if (error instanceof AgentActionNotFoundError) { return 404; } - if (error instanceof FleetUnauthorizedError) { - return 403; // Unauthorized - } + // Conflict if (error instanceof AgentPolicyNameExistsError) { - return 409; // Conflict + return 409; } if (error instanceof ConcurrentInstallOperationError) { - return 409; // Conflict + return 409; } if (error instanceof PackagePolicyNameExistsError) { - return 409; // Conflict + return 409; } if (error instanceof PackageAlreadyInstalledError) { - return 409; // Conflict + return 409; + } + // Unsupported Media Type + if (error instanceof PackageUnsupportedMediaTypeError) { + return 415; } + // Internal Server Error if (error instanceof UninstallTokenError) { - return 500; // Internal Error + return 500; } if (error instanceof BundledPackageLocationNotFoundError) { - return 500; // Internal Error + return 500; } if (error instanceof PackageESError) { - return 500; // Internal Error + return 500; + } + if (error instanceof RegistryResponseError) { + // 4xx/5xx's from EPR + return 500; + } + // Bad Gateway + if (error instanceof RegistryConnectionError || error instanceof RegistryError) { + // Connection errors (ie. RegistryConnectionError) / fallback (RegistryError) from EPR + return 502; } return 400; // Bad Request }; diff --git a/x-pack/plugins/fleet/server/services/epm/agent/agent.ts b/x-pack/plugins/fleet/server/services/epm/agent/agent.ts index a665870303cce..0bc220a500fb1 100644 --- a/x-pack/plugins/fleet/server/services/epm/agent/agent.ts +++ b/x-pack/plugins/fleet/server/services/epm/agent/agent.ts @@ -10,18 +10,18 @@ import { safeLoad, safeDump } from 'js-yaml'; import type { PackagePolicyConfigRecord } from '../../../../common/types'; import { toCompiledSecretRef } from '../../secrets'; -import { FleetError } from '../../../errors'; +import { PackageInvalidArchiveError } from '../../../errors'; const handlebars = Handlebars.create(); export function compileTemplate(variables: PackagePolicyConfigRecord, templateStr: string) { - const { vars, yamlValues } = buildTemplateVariables(variables, templateStr); + const { vars, yamlValues } = buildTemplateVariables(variables); let compiledTemplate: string; try { const template = handlebars.compile(templateStr, { noEscape: true }); compiledTemplate = template(vars); } catch (err) { - throw new FleetError(`Error while compiling agent template: ${err.message}`); + throw new PackageInvalidArchiveError(`Error while compiling agent template: ${err.message}`); } compiledTemplate = replaceRootLevelYamlVariables(yamlValues, compiledTemplate); @@ -65,7 +65,7 @@ function replaceVariablesInYaml(yamlVariables: { [k: string]: any }, yaml: any) return yaml; } -function buildTemplateVariables(variables: PackagePolicyConfigRecord, templateStr: string) { +function buildTemplateVariables(variables: PackagePolicyConfigRecord) { const yamlValues: { [k: string]: any } = {}; const vars = Object.entries(variables).reduce((acc, [key, recordEntry]) => { // support variables with . like key.patterns @@ -73,13 +73,17 @@ function buildTemplateVariables(variables: PackagePolicyConfigRecord, templateSt const lastKeyPart = keyParts.pop(); if (!lastKeyPart || !isValidKey(lastKeyPart)) { - throw new Error('Invalid key'); + throw new PackageInvalidArchiveError( + `Error while compiling agent template: Invalid key ${lastKeyPart}` + ); } let varPart = acc; for (const keyPart of keyParts) { if (!isValidKey(keyPart)) { - throw new FleetError(`Invalid key ${keyPart}`); + throw new PackageInvalidArchiveError( + `Error while compiling agent template: Invalid key ${keyPart}` + ); } if (!varPart[keyPart]) { varPart[keyPart] = {}; diff --git a/x-pack/plugins/fleet/server/services/epm/packages/install.ts b/x-pack/plugins/fleet/server/services/epm/packages/install.ts index ac0be0b7fb34e..abea52f8143e4 100644 --- a/x-pack/plugins/fleet/server/services/epm/packages/install.ts +++ b/x-pack/plugins/fleet/server/services/epm/packages/install.ts @@ -62,6 +62,7 @@ import { PackagePolicyValidationError, ConcurrentInstallOperationError, FleetUnauthorizedError, + PackageInvalidArchiveError, } from '../../../errors'; import { PACKAGES_SAVED_OBJECT_TYPE, MAX_TIME_COMPLETE_INSTALL } from '../../../constants'; import { dataStreamService, licenseService } from '../..'; @@ -202,7 +203,7 @@ export async function ensureInstalledPackage(options: { } const installation = await getInstallation({ savedObjectsClient, pkgName }); - if (!installation) throw new FleetError(`Could not get installation ${pkgName}`); + if (!installation) throw new FleetError(`Could not get installation for ${pkgName}`); return installation; } @@ -1273,7 +1274,7 @@ export async function installAssetsForInputPackagePolicy(opts: { if (pkgInfo.type !== 'input') return; const paths = await getArchiveFilelist(pkgInfo); - if (!paths) throw new FleetError(`No paths found for ${pkgInfo}`); + if (!paths) throw new PackageInvalidArchiveError(`No paths found for ${pkgInfo.name}`); const datasetName = packagePolicy.inputs[0].streams[0].vars?.[DATASET_VAR_NAME]?.value; const [dataStream] = getNormalizedDataStreams(pkgInfo, datasetName); @@ -1336,7 +1337,9 @@ export async function installAssetsForInputPackagePolicy(opts: { logger, }); if (!installedPkg) - throw new FleetError('Unable to find installed package while creating index templates'); + throw new FleetError( + `Error while creating index templates: unable to find installed package ${pkgInfo.name}` + ); await installIndexTemplatesAndPipelines({ installedPkg, paths, diff --git a/x-pack/plugins/fleet/server/services/epm/packages/update.ts b/x-pack/plugins/fleet/server/services/epm/packages/update.ts index 7159571c29d6d..d5924ac0df674 100644 --- a/x-pack/plugins/fleet/server/services/epm/packages/update.ts +++ b/x-pack/plugins/fleet/server/services/epm/packages/update.ts @@ -12,10 +12,12 @@ import type { ExperimentalIndexingFeature } from '../../../../common/types'; import { PACKAGES_SAVED_OBJECT_TYPE } from '../../../constants'; import type { Installation, UpdatePackageRequestSchema } from '../../../types'; -import { FleetError } from '../../../errors'; +import { PackageNotFoundError } from '../../../errors'; import { auditLoggingService } from '../../audit_logging'; +import { appContextService } from '../..'; + import { getInstallationObject, getPackageInfo } from './get'; export async function updatePackage( @@ -25,11 +27,13 @@ export async function updatePackage( keepPoliciesUpToDate?: boolean; } & TypeOf ) { + const logger = appContextService.getLogger(); const { savedObjectsClient, pkgName, keepPoliciesUpToDate } = options; const installedPackage = await getInstallationObject({ savedObjectsClient, pkgName }); if (!installedPackage) { - throw new FleetError(`Package ${pkgName} is not installed`); + logger.warn(`Error while updating package: ${pkgName} is not installed`); + throw new PackageNotFoundError(`Error while updating package: ${pkgName} is not installed`); } auditLoggingService.writeCustomSoAuditLog({ From 9fe511c51bd4ac03ae17ee0eafdceea1e69f8fbf Mon Sep 17 00:00:00 2001 From: criamico Date: Thu, 23 Nov 2023 12:08:21 +0100 Subject: [PATCH 06/10] Add some more loggers --- .../fleet/server/services/epm/archive/index.ts | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/x-pack/plugins/fleet/server/services/epm/archive/index.ts b/x-pack/plugins/fleet/server/services/epm/archive/index.ts index 330839b13dba9..37ab0471a5909 100644 --- a/x-pack/plugins/fleet/server/services/epm/archive/index.ts +++ b/x-pack/plugins/fleet/server/services/epm/archive/index.ts @@ -7,6 +7,7 @@ import type { AssetParts } from '../../../../common/types'; import { PackageInvalidArchiveError, PackageUnsupportedMediaTypeError } from '../../../errors'; +import { appContextService } from '../../app_context'; import { getArchiveEntry, @@ -62,8 +63,12 @@ export async function unpackBufferEntries( archiveBuffer: Buffer, contentType: string ): Promise { + const logger = appContextService.getLogger(); const bufferExtractor = getBufferExtractor({ contentType }); if (!bufferExtractor) { + logger.warn( + `Unsupported media type ${contentType}. Please use 'application/gzip' or 'application/zip'` + ); throw new PackageUnsupportedMediaTypeError( `Unsupported media type ${contentType}. Please use 'application/gzip' or 'application/zip'` ); @@ -74,6 +79,9 @@ export async function unpackBufferEntries( const addToEntries = (entry: ArchiveEntry) => entries.push(entry); await bufferExtractor(archiveBuffer, onlyFiles, addToEntries); } catch (error) { + logger.warn( + `Error during extraction of package: ${error}. Assumed content type was ${contentType}, check if this matches the archive type.` + ); throw new PackageInvalidArchiveError( `Error during extraction of package: ${error}. Assumed content type was ${contentType}, check if this matches the archive type.` ); @@ -82,6 +90,9 @@ export async function unpackBufferEntries( // While unpacking a tar.gz file with unzipBuffer() will result in a thrown error in the try-catch above, // unpacking a zip file with untarBuffer() just results in nothing. if (entries.length === 0) { + logger.warn( + `Archive seems empty. Assumed content type was ${contentType}, check if this matches the archive type.` + ); throw new PackageInvalidArchiveError( `Archive seems empty. Assumed content type was ${contentType}, check if this matches the archive type.` ); From 2af8450fa887f67f0db009d7992c14aef963e048 Mon Sep 17 00:00:00 2001 From: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Date: Thu, 23 Nov 2023 12:13:31 +0000 Subject: [PATCH 07/10] [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix' --- .../public/application/components/connectors_ingestion.tsx | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/x-pack/plugins/serverless_search/public/application/components/connectors_ingestion.tsx b/x-pack/plugins/serverless_search/public/application/components/connectors_ingestion.tsx index 6be9b28707dff..ede574abba12b 100644 --- a/x-pack/plugins/serverless_search/public/application/components/connectors_ingestion.tsx +++ b/x-pack/plugins/serverless_search/public/application/components/connectors_ingestion.tsx @@ -50,7 +50,10 @@ export const ConnectorIngestionPanel: React.FC<{ assetBasePath: string }> = ({ a - createConnector()}> + createConnector()} + > {i18n.translate( 'xpack.serverlessSearch.ingestData.alternativeOptions.setupConnectorLabel', { @@ -74,6 +77,7 @@ export const ConnectorIngestionPanel: React.FC<{ assetBasePath: string }> = ({ a From 595cef91f0d1df4aede6d89a04d35f9d95c73120 Mon Sep 17 00:00:00 2001 From: criamico Date: Thu, 23 Nov 2023 15:39:08 +0100 Subject: [PATCH 08/10] Address code review and remove duplicate logger --- .../server/services/epm/archive/index.ts | 11 --------- .../server/services/epm/archive/storage.ts | 5 ---- .../services/epm/elasticsearch/ilm/install.ts | 5 ++-- .../epm/elasticsearch/template/template.ts | 23 ++++++++----------- .../fleet/server/services/epm/packages/get.ts | 2 -- .../server/services/epm/packages/remove.ts | 4 ---- .../server/services/epm/packages/update.ts | 4 ---- .../server/services/epm/registry/index.ts | 2 -- 8 files changed, 12 insertions(+), 44 deletions(-) diff --git a/x-pack/plugins/fleet/server/services/epm/archive/index.ts b/x-pack/plugins/fleet/server/services/epm/archive/index.ts index 37ab0471a5909..330839b13dba9 100644 --- a/x-pack/plugins/fleet/server/services/epm/archive/index.ts +++ b/x-pack/plugins/fleet/server/services/epm/archive/index.ts @@ -7,7 +7,6 @@ import type { AssetParts } from '../../../../common/types'; import { PackageInvalidArchiveError, PackageUnsupportedMediaTypeError } from '../../../errors'; -import { appContextService } from '../../app_context'; import { getArchiveEntry, @@ -63,12 +62,8 @@ export async function unpackBufferEntries( archiveBuffer: Buffer, contentType: string ): Promise { - const logger = appContextService.getLogger(); const bufferExtractor = getBufferExtractor({ contentType }); if (!bufferExtractor) { - logger.warn( - `Unsupported media type ${contentType}. Please use 'application/gzip' or 'application/zip'` - ); throw new PackageUnsupportedMediaTypeError( `Unsupported media type ${contentType}. Please use 'application/gzip' or 'application/zip'` ); @@ -79,9 +74,6 @@ export async function unpackBufferEntries( const addToEntries = (entry: ArchiveEntry) => entries.push(entry); await bufferExtractor(archiveBuffer, onlyFiles, addToEntries); } catch (error) { - logger.warn( - `Error during extraction of package: ${error}. Assumed content type was ${contentType}, check if this matches the archive type.` - ); throw new PackageInvalidArchiveError( `Error during extraction of package: ${error}. Assumed content type was ${contentType}, check if this matches the archive type.` ); @@ -90,9 +82,6 @@ export async function unpackBufferEntries( // While unpacking a tar.gz file with unzipBuffer() will result in a thrown error in the try-catch above, // unpacking a zip file with untarBuffer() just results in nothing. if (entries.length === 0) { - logger.warn( - `Archive seems empty. Assumed content type was ${contentType}, check if this matches the archive type.` - ); throw new PackageInvalidArchiveError( `Archive seems empty. Assumed content type was ${contentType}, check if this matches the archive type.` ); diff --git a/x-pack/plugins/fleet/server/services/epm/archive/storage.ts b/x-pack/plugins/fleet/server/services/epm/archive/storage.ts index 3f5fdff5f6125..81d55c5fd3138 100644 --- a/x-pack/plugins/fleet/server/services/epm/archive/storage.ts +++ b/x-pack/plugins/fleet/server/services/epm/archive/storage.ts @@ -56,7 +56,6 @@ export async function archiveEntryToESDocument(opts: { version: string; installSource: InstallSource; }): Promise { - const logger = appContextService.getLogger(); const { path, buffer, name, version, installSource } = opts; const fileExt = extname(path); const contentType = mime.lookup(fileExt); @@ -72,16 +71,12 @@ export async function archiveEntryToESDocument(opts: { // validation: filesize? asset type? anything else if (dataUtf8.length > currentMaxAssetBytes) { - logger.warn(`File at ${path} is larger than maximum allowed size of ${currentMaxAssetBytes}`); throw new PackageInvalidArchiveError( `File at ${path} is larger than maximum allowed size of ${currentMaxAssetBytes}` ); } if (dataBase64.length > currentMaxAssetBytes) { - logger.warn( - `After base64 encoding file at ${path} is larger than maximum allowed size of ${currentMaxAssetBytes}` - ); throw new PackageInvalidArchiveError( `After base64 encoding file at ${path} is larger than maximum allowed size of ${currentMaxAssetBytes}` ); diff --git a/x-pack/plugins/fleet/server/services/epm/elasticsearch/ilm/install.ts b/x-pack/plugins/fleet/server/services/epm/elasticsearch/ilm/install.ts index fa70947fdfc0c..61a75d28b7999 100644 --- a/x-pack/plugins/fleet/server/services/epm/elasticsearch/ilm/install.ts +++ b/x-pack/plugins/fleet/server/services/epm/elasticsearch/ilm/install.ts @@ -14,7 +14,7 @@ import { getAsset, getPathParts } from '../../archive'; import { updateEsAssetReferences } from '../../packages/install'; import { getESAssetMetadata } from '../meta'; import { retryTransientEsErrors } from '../retry'; -import { FleetError } from '../../../../errors'; +import { PackageInvalidArchiveError } from '../../../../errors'; export async function installILMPolicy( packageInfo: InstallablePackage, @@ -58,8 +58,7 @@ export async function installILMPolicy( { logger } ); } catch (err) { - logger.warn(`Couldn't install ilm policies: ${err.message}`); - throw new FleetError(`Couldn't install ilm policies: ${err.message}`); + throw new PackageInvalidArchiveError(`Couldn't install ilm policies: ${err.message}`); } }) ); diff --git a/x-pack/plugins/fleet/server/services/epm/elasticsearch/template/template.ts b/x-pack/plugins/fleet/server/services/epm/elasticsearch/template/template.ts index 9781ad8d8f0e0..a26aa2c8e0114 100644 --- a/x-pack/plugins/fleet/server/services/epm/elasticsearch/template/template.ts +++ b/x-pack/plugins/fleet/server/services/epm/elasticsearch/template/template.ts @@ -33,7 +33,7 @@ import { } from '../../../../constants'; import { getESAssetMetadata } from '../meta'; import { retryTransientEsErrors } from '../retry'; -import { FleetError, PackageESError } from '../../../../errors'; +import { PackageESError, PackageInvalidArchiveError } from '../../../../errors'; import { getDefaultProperties, histogram, keyword, scaledFloat } from './mappings'; @@ -103,7 +103,7 @@ export function getTemplate({ isIndexModeTimeSeries, }); if (template.template.settings.index.final_pipeline) { - throw new PackageESError( + throw new PackageInvalidArchiveError( `Error template for ${templateIndexPattern} contains a final_pipeline` ); } @@ -430,7 +430,7 @@ function _generateMappings( matchingType = field.object_type_mapping_type ?? 'object'; break; default: - throw new FleetError( + throw new PackageInvalidArchiveError( `No dynamic mapping generated for field ${path} of type ${field.object_type}` ); } @@ -904,18 +904,13 @@ const getDataStreams = async ( })); }; -const rolloverDataStream = ( - dataStreamName: string, - esClient: ElasticsearchClient, - logger: Logger -) => { +const rolloverDataStream = (dataStreamName: string, esClient: ElasticsearchClient) => { try { // Do no wrap rollovers in retryTransientEsErrors since it is not idempotent return esClient.indices.rollover({ alias: dataStreamName, }); } catch (error) { - logger.warn(`Cannot rollover data stream [${dataStreamName}] due to error: ${error}`); throw new PackageESError( `Cannot rollover data stream [${dataStreamName}] due to error: ${error}` ); @@ -1023,7 +1018,7 @@ const updateExistingDataStream = async ({ return; } else { logger.info(`Triggering a rollover for ${dataStreamName}`); - await rolloverDataStream(dataStreamName, esClient, logger); + await rolloverDataStream(dataStreamName, esClient); return; } } @@ -1047,7 +1042,7 @@ const updateExistingDataStream = async ({ logger.info( `Index mode or source type has changed for ${dataStreamName}, triggering a rollover` ); - await rolloverDataStream(dataStreamName, esClient, logger); + await rolloverDataStream(dataStreamName, esClient); } } @@ -1065,7 +1060,8 @@ const updateExistingDataStream = async ({ { logger } ); } catch (err) { - logger.warn(`Could not update lifecycle settings for ${dataStreamName}: ${err.message}`); + // Check if this error can happen because of invalid settings; + // We are returning a 500 but in that case it should be a 400 instead throw new PackageESError( `Could not update lifecycle settings for ${dataStreamName}: ${err.message}` ); @@ -1091,7 +1087,8 @@ const updateExistingDataStream = async ({ { logger } ); } catch (err) { - logger.warn(`Could not update index template settings for ${dataStreamName}`); + // Same as above - Check if this error can happen because of invalid settings; + // We are returning a 500 but in that case it should be a 400 instead throw new PackageESError(`Could not update index template settings for ${dataStreamName}`); } }; diff --git a/x-pack/plugins/fleet/server/services/epm/packages/get.ts b/x-pack/plugins/fleet/server/services/epm/packages/get.ts index ab61987447294..2c4feb67a7023 100644 --- a/x-pack/plugins/fleet/server/services/epm/packages/get.ts +++ b/x-pack/plugins/fleet/server/services/epm/packages/get.ts @@ -423,7 +423,6 @@ export async function getPackageInfo({ ]); if (!savedObject && !latestPackage) { - logger.error(`[${pkgName}] package not installed or found in registry`); throw new PackageNotFoundError(`[${pkgName}] package not installed or found in registry`); } @@ -602,7 +601,6 @@ export async function getPackageFromSource(options: { } } if (!res) { - logger.error(`Package info for ${pkgName}-${pkgVersion} does not exist`); throw new PackageNotFoundError(`Package info for ${pkgName}-${pkgVersion} does not exist`); } return { diff --git a/x-pack/plugins/fleet/server/services/epm/packages/remove.ts b/x-pack/plugins/fleet/server/services/epm/packages/remove.ts index 577e096e05a99..99f5b44db213e 100644 --- a/x-pack/plugins/fleet/server/services/epm/packages/remove.ts +++ b/x-pack/plugins/fleet/server/services/epm/packages/remove.ts @@ -72,14 +72,10 @@ export async function removeInstallation(options: { if (options.force || items.every((item) => (item.agents ?? 0) === 0)) { // delete package policies const ids = items.map((item) => item.id); - logger.info( - `Deleting package policies of ${pkgName} package because not used by agents or force flag was enabled: ${ids}` - ); await packagePolicyService.delete(savedObjectsClient, esClient, ids, { force: options.force, }); } else { - logger.warn(`Unable to remove package with existing package policy(s) in use by agent(s)`); throw new PackageRemovalError( `Unable to remove package with existing package policy(s) in use by agent(s)` ); diff --git a/x-pack/plugins/fleet/server/services/epm/packages/update.ts b/x-pack/plugins/fleet/server/services/epm/packages/update.ts index d5924ac0df674..72c43b6dc688a 100644 --- a/x-pack/plugins/fleet/server/services/epm/packages/update.ts +++ b/x-pack/plugins/fleet/server/services/epm/packages/update.ts @@ -16,8 +16,6 @@ import { PackageNotFoundError } from '../../../errors'; import { auditLoggingService } from '../../audit_logging'; -import { appContextService } from '../..'; - import { getInstallationObject, getPackageInfo } from './get'; export async function updatePackage( @@ -27,12 +25,10 @@ export async function updatePackage( keepPoliciesUpToDate?: boolean; } & TypeOf ) { - const logger = appContextService.getLogger(); const { savedObjectsClient, pkgName, keepPoliciesUpToDate } = options; const installedPackage = await getInstallationObject({ savedObjectsClient, pkgName }); if (!installedPackage) { - logger.warn(`Error while updating package: ${pkgName} is not installed`); throw new PackageNotFoundError(`Error while updating package: ${pkgName} is not installed`); } diff --git a/x-pack/plugins/fleet/server/services/epm/registry/index.ts b/x-pack/plugins/fleet/server/services/epm/registry/index.ts index ca0d53092d294..24c81dd023244 100644 --- a/x-pack/plugins/fleet/server/services/epm/registry/index.ts +++ b/x-pack/plugins/fleet/server/services/epm/registry/index.ts @@ -364,11 +364,9 @@ export async function getPackage( } function ensureContentType(archivePath: string) { - const logger = appContextService.getLogger(); const contentType = mime.lookup(archivePath); if (!contentType) { - logger.error(`Unknown compression format for '${archivePath}'. Please use .zip or .gz`); throw new PackageUnsupportedMediaTypeError( `Unknown compression format for '${archivePath}'. Please use .zip or .gz` ); From 3500e8538722a94402ad2934bf40e7812c104744 Mon Sep 17 00:00:00 2001 From: criamico Date: Thu, 23 Nov 2023 17:05:46 +0100 Subject: [PATCH 09/10] more fix --- .../fleet/server/services/epm/kibana/assets/install.ts | 10 ---------- .../fleet/server/services/epm/packages/install.ts | 3 ++- 2 files changed, 2 insertions(+), 11 deletions(-) diff --git a/x-pack/plugins/fleet/server/services/epm/kibana/assets/install.ts b/x-pack/plugins/fleet/server/services/epm/kibana/assets/install.ts index 51e5c1e70a9a6..23327a2253f86 100644 --- a/x-pack/plugins/fleet/server/services/epm/kibana/assets/install.ts +++ b/x-pack/plugins/fleet/server/services/epm/kibana/assets/install.ts @@ -341,11 +341,6 @@ export async function installKibanaSavedObjects({ ); if (otherErrors?.length) { - logger.warn( - `Encountered ${ - otherErrors.length - } errors creating saved objects: ${formatImportErrorsForLog(otherErrors)}` - ); throw new KibanaSOReferenceError( `Encountered ${ otherErrors.length @@ -389,11 +384,6 @@ export async function installKibanaSavedObjects({ }); if (resolveErrors?.length) { - logger.warn( - `Encountered ${ - resolveErrors.length - } errors resolving reference errors: ${formatImportErrorsForLog(resolveErrors)}` - ); throw new KibanaSOReferenceError( `Encountered ${ resolveErrors.length diff --git a/x-pack/plugins/fleet/server/services/epm/packages/install.ts b/x-pack/plugins/fleet/server/services/epm/packages/install.ts index abea52f8143e4..72b8084f32325 100644 --- a/x-pack/plugins/fleet/server/services/epm/packages/install.ts +++ b/x-pack/plugins/fleet/server/services/epm/packages/install.ts @@ -63,6 +63,7 @@ import { ConcurrentInstallOperationError, FleetUnauthorizedError, PackageInvalidArchiveError, + PackageNotFoundError, } from '../../../errors'; import { PACKAGES_SAVED_OBJECT_TYPE, MAX_TIME_COMPLETE_INSTALL } from '../../../constants'; import { dataStreamService, licenseService } from '../..'; @@ -1337,7 +1338,7 @@ export async function installAssetsForInputPackagePolicy(opts: { logger, }); if (!installedPkg) - throw new FleetError( + throw new PackageNotFoundError( `Error while creating index templates: unable to find installed package ${pkgInfo.name}` ); await installIndexTemplatesAndPipelines({ From d98c30039bc537cb6ee508323db02d1e5e6d4fdd Mon Sep 17 00:00:00 2001 From: criamico Date: Mon, 27 Nov 2023 10:32:07 +0100 Subject: [PATCH 10/10] Fix type check errors --- x-pack/plugins/fleet/server/services/epm/packages/get.ts | 1 - x-pack/plugins/fleet/server/services/epm/packages/remove.ts | 1 - 2 files changed, 2 deletions(-) diff --git a/x-pack/plugins/fleet/server/services/epm/packages/get.ts b/x-pack/plugins/fleet/server/services/epm/packages/get.ts index 2c4feb67a7023..a3ed5f62d7f1e 100644 --- a/x-pack/plugins/fleet/server/services/epm/packages/get.ts +++ b/x-pack/plugins/fleet/server/services/epm/packages/get.ts @@ -416,7 +416,6 @@ export async function getPackageInfo({ ignoreUnverified?: boolean; prerelease?: boolean; }): Promise { - const logger = appContextService.getLogger(); const [savedObject, latestPackage] = await Promise.all([ getInstallationObject({ savedObjectsClient, pkgName }), Registry.fetchFindLatestPackageOrUndefined(pkgName, { prerelease }), diff --git a/x-pack/plugins/fleet/server/services/epm/packages/remove.ts b/x-pack/plugins/fleet/server/services/epm/packages/remove.ts index 99f5b44db213e..ba9bace6a0dee 100644 --- a/x-pack/plugins/fleet/server/services/epm/packages/remove.ts +++ b/x-pack/plugins/fleet/server/services/epm/packages/remove.ts @@ -53,7 +53,6 @@ export async function removeInstallation(options: { esClient: ElasticsearchClient; force?: boolean; }): Promise { - const logger = appContextService.getLogger(); const { savedObjectsClient, pkgName, pkgVersion, esClient } = options; const installation = await getInstallation({ savedObjectsClient, pkgName }); if (!installation) throw new PackageRemovalError(`${pkgName} is not installed`);