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
elastic · Nov 5, 2021 · b7076e8 · b7076e8
1 parent 55d40a2
commit b7076e8
Show file tree

Hide file tree

Showing 5 changed files with 35 additions and 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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);
         });
       });
     });