From b7076e82e054cf082ddaa03e4014b7d556a9f737 Mon Sep 17 00:00:00 2001 From: Kyle Pollich Date: Fri, 5 Nov 2021 13:05:42 -0400 Subject: [PATCH] Consider all errors non-fatal during dry runs (#117594) Since we've enabled auto upgrades for some packages in 7.16, we need to revert a previous change that only considered policy validation errors non-fatal. Any error encountered during the dry-run/upgrade process should be considered non-fatal so that users can still access the Fleet UI if setup fails. Fixes #116985 --- .../common/types/rest_spec/package_policy.ts | 6 ++++++ .../server/routes/package_policy/handlers.ts | 21 +++++++++++++++++++ .../services/managed_package_policies.ts | 5 ++++- .../fleet/server/services/package_policy.ts | 11 ---------- .../apis/package_policy/upgrade.ts | 12 ++++------- 5 files changed, 35 insertions(+), 20 deletions(-) diff --git a/x-pack/plugins/fleet/common/types/rest_spec/package_policy.ts b/x-pack/plugins/fleet/common/types/rest_spec/package_policy.ts index ed5f8e07098d4..b050a7c798a0b 100644 --- a/x-pack/plugins/fleet/common/types/rest_spec/package_policy.ts +++ b/x-pack/plugins/fleet/common/types/rest_spec/package_policy.ts @@ -67,6 +67,12 @@ export type DeletePackagePoliciesResponse = Array<{ export interface UpgradePackagePolicyBaseResponse { name?: string; + + // Support generic errors + statusCode?: number; + body?: { + message: string; + }; } export interface UpgradePackagePolicyDryRunResponseItem extends UpgradePackagePolicyBaseResponse { diff --git a/x-pack/plugins/fleet/server/routes/package_policy/handlers.ts b/x-pack/plugins/fleet/server/routes/package_policy/handlers.ts index 58463bfa5569d..f61890f852798 100644 --- a/x-pack/plugins/fleet/server/routes/package_policy/handlers.ts +++ b/x-pack/plugins/fleet/server/routes/package_policy/handlers.ts @@ -192,6 +192,8 @@ export const deletePackagePolicyHandler: RequestHandler< } }; +// TODO: Separate the upgrade and dry-run processes into separate endpoints, and address +// duplicate logic in error handling as part of https://github.com/elastic/kibana/issues/63123 export const upgradePackagePolicyHandler: RequestHandler< unknown, unknown, @@ -212,6 +214,16 @@ export const upgradePackagePolicyHandler: RequestHandler< ); body.push(result); } + + const firstFatalError = body.find((item) => item.statusCode && item.statusCode !== 200); + + if (firstFatalError) { + return response.customError({ + statusCode: firstFatalError.statusCode!, + body: { message: firstFatalError.body!.message }, + }); + } + return response.ok({ body, }); @@ -222,6 +234,15 @@ export const upgradePackagePolicyHandler: RequestHandler< request.body.packagePolicyIds, { user } ); + + const firstFatalError = body.find((item) => item.statusCode && item.statusCode !== 200); + + if (firstFatalError) { + return response.customError({ + statusCode: firstFatalError.statusCode!, + body: { message: firstFatalError.body!.message }, + }); + } return response.ok({ body, }); diff --git a/x-pack/plugins/fleet/server/services/managed_package_policies.ts b/x-pack/plugins/fleet/server/services/managed_package_policies.ts index 306725ae01953..e78bc096b8711 100644 --- a/x-pack/plugins/fleet/server/services/managed_package_policies.ts +++ b/x-pack/plugins/fleet/server/services/managed_package_policies.ts @@ -72,7 +72,10 @@ export const upgradeManagedPackagePolicies = async ( ); if (dryRunResults.hasErrors) { - const errors = dryRunResults.diff?.[1].errors; + const errors = dryRunResults.diff + ? dryRunResults.diff?.[1].errors + : dryRunResults.body?.message; + appContextService .getLogger() .error( diff --git a/x-pack/plugins/fleet/server/services/package_policy.ts b/x-pack/plugins/fleet/server/services/package_policy.ts index 985351c3e981b..c4ef15f4e7ed9 100644 --- a/x-pack/plugins/fleet/server/services/package_policy.ts +++ b/x-pack/plugins/fleet/server/services/package_policy.ts @@ -620,13 +620,6 @@ class PackagePolicyService { success: true, }); } catch (error) { - // We only want to specifically handle validation errors for the new package policy. If a more severe or - // general error is thrown elsewhere during the upgrade process, we want to surface that directly in - // order to preserve any status code mappings, etc that might be included w/ the particular error type - if (!(error instanceof PackagePolicyValidationError)) { - throw error; - } - result.push({ id, success: false, @@ -704,10 +697,6 @@ class PackagePolicyService { hasErrors, }; } catch (error) { - if (!(error instanceof PackagePolicyValidationError)) { - throw error; - } - return { hasErrors: true, ...ingestErrorToResponseOptions(error), diff --git a/x-pack/test/fleet_api_integration/apis/package_policy/upgrade.ts b/x-pack/test/fleet_api_integration/apis/package_policy/upgrade.ts index a61a77fd37f6b..5a61d69ed8ba6 100644 --- a/x-pack/test/fleet_api_integration/apis/package_policy/upgrade.ts +++ b/x-pack/test/fleet_api_integration/apis/package_policy/upgrade.ts @@ -570,16 +570,14 @@ export default function (providerContext: FtrProviderContext) { describe('upgrade', function () { it('fails to upgrade package policy', async function () { - const { body }: { body: UpgradePackagePolicyResponse } = await supertest + await supertest .post(`/api/fleet/package_policies/upgrade`) .set('kbn-xsrf', 'xxxx') .send({ packagePolicyIds: [packagePolicyId], dryRun: false, }) - .expect(200); - - expect(body[0].success).to.be(false); + .expect(400); }); }); }); @@ -672,16 +670,14 @@ export default function (providerContext: FtrProviderContext) { describe('upgrade', function () { it('fails to upgrade package policy', async function () { - const { body }: { body: UpgradePackagePolicyResponse } = await supertest + await supertest .post(`/api/fleet/package_policies/upgrade`) .set('kbn-xsrf', 'xxxx') .send({ packagePolicyIds: [packagePolicyId], dryRun: false, }) - .expect(200); - - expect(body[0].success).to.be(false); + .expect(400); }); }); });