[Fleet] optimizing package policy upgrade and dry run (#126088)

* optimizing package policy upgrade and dry run * optimizing package policy upgrade and dry run * optimizing package policy upgrade and dry run * missing params * fixed tests * removed unused import * fixed tests * fixed checks * reduced duplication, separated validate logic * reduced duplication, separated validate logic Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
elastic · Feb 23, 2022 · f96af16 · f96af16
1 parent 1a1a191
commit f96af16
Show file tree

Hide file tree

Showing 6 changed files with 371 additions and 250 deletions.
diff --git a/x-pack/plugins/fleet/server/mocks/index.ts b/x-pack/plugins/fleet/server/mocks/index.ts
@@ -99,9 +99,7 @@ export const xpackMocks = {
 
 export const createPackagePolicyServiceMock = (): jest.Mocked<PackagePolicyServiceInterface> => {
   return {
-    _compilePackagePolicyInputs: jest.fn(),
     buildPackagePolicyFromPackage: jest.fn(),
-    buildPackagePolicyFromPackageWithVersion: jest.fn(),
     bulkCreate: jest.fn(),
     create: jest.fn(),
     delete: jest.fn(),

diff --git a/x-pack/plugins/fleet/server/routes/package_policy/handlers.test.ts b/x-pack/plugins/fleet/server/routes/package_policy/handlers.test.ts
@@ -28,21 +28,16 @@ import type { PackagePolicy } from '../../types';
 
 import { registerRoutes } from './index';
 
-type PackagePolicyServicePublicInterface = Omit<
-  PackagePolicyServiceInterface,
-  'getUpgradePackagePolicyInfo'
->;
-
 const packagePolicyServiceMock = packagePolicyService as jest.Mocked<PackagePolicyServiceInterface>;
 
 jest.mock(
   '../../services/package_policy',
   (): {
-    packagePolicyService: jest.Mocked<PackagePolicyServicePublicInterface>;
+    packagePolicyService: jest.Mocked<PackagePolicyServiceInterface>;
   } => {
     return {
       packagePolicyService: {
-        _compilePackagePolicyInputs: jest.fn((registryPkgInfo, packageInfo, vars, dataInputs) =>
+        _compilePackagePolicyInputs: jest.fn((packageInfo, vars, dataInputs) =>
           Promise.resolve(dataInputs)
         ),
         buildPackagePolicyFromPackage: jest.fn(),

diff --git a/x-pack/plugins/fleet/server/services/managed_package_policies.test.ts b/x-pack/plugins/fleet/server/services/managed_package_policies.test.ts
@@ -45,25 +45,24 @@ describe('upgradeManagedPackagePolicies', () => {
   it('should upgrade policies for managed package', async () => {
     const esClient = elasticsearchServiceMock.createClusterClient().asInternalUser;
     const soClient = savedObjectsClientMock.create();
+    const packagePolicy = {
+      id: 'managed-package-id',
+      inputs: {},
+      version: '',
+      revision: 1,
+      updated_at: '',
+      updated_by: '',
+      created_at: '',
+      created_by: '',
+      package: {
+        name: 'managed-package',
+        title: 'Managed Package',
+        version: '0.0.1',
+      },
+    };
 
     (packagePolicyService.list as jest.Mock).mockResolvedValueOnce({
-      items: [
-        {
-          id: 'managed-package-id',
-          inputs: {},
-          version: '',
-          revision: 1,
-          updated_at: '',
-          updated_by: '',
-          created_at: '',
-          created_by: '',
-          package: {
-            name: 'managed-package',
-            title: 'Managed Package',
-            version: '0.0.1',
-          },
-        },
-      ],
+      items: [packagePolicy],
     });
 
     (packagePolicyService.getUpgradeDryRunDiff as jest.Mock).mockResolvedValueOnce({
@@ -89,7 +88,14 @@ describe('upgradeManagedPackagePolicies', () => {
       { packagePolicyId: 'managed-package-id', diff: [{ id: 'foo' }, { id: 'bar' }], errors: [] },
     ]);
 
-    expect(packagePolicyService.upgrade).toBeCalledWith(soClient, esClient, ['managed-package-id']);
+    expect(packagePolicyService.upgrade).toBeCalledWith(
+      soClient,
+      esClient,
+      ['managed-package-id'],
+      undefined,
+      packagePolicy,
+      '1.0.0'
+    );
   });
 
   it('should not upgrade policy if newer than installed package version', async () => {

diff --git a/x-pack/plugins/fleet/server/services/managed_package_policies.ts b/x-pack/plugins/fleet/server/services/managed_package_policies.ts
@@ -50,7 +50,7 @@ export const upgradeManagedPackagePolicies = async (
 
     for (const packagePolicy of packagePolicies) {
       if (isPolicyVersionLtInstalledVersion(packagePolicy, installedPackage)) {
-        await upgradePackagePolicy(soClient, esClient, packagePolicy.id, results);
+        await upgradePackagePolicy(soClient, esClient, packagePolicy, installedPackage, results);
       }
     }
   }
@@ -84,13 +84,19 @@ function isPolicyVersionLtInstalledVersion(
 async function upgradePackagePolicy(
   soClient: SavedObjectsClientContract,
   esClient: ElasticsearchClient,
-  packagePolicyId: string,
+  packagePolicy: PackagePolicy,
+  installedPackage: Installation,
   results: UpgradeManagedPackagePoliciesResult[]
 ) {
   // Since upgrades don't report diffs/errors, we need to perform a dry run first in order
   // to notify the user of any granular policy upgrade errors that occur during Fleet's
   // preconfiguration check
-  const dryRunResults = await packagePolicyService.getUpgradeDryRunDiff(soClient, packagePolicyId);
+  const dryRunResults = await packagePolicyService.getUpgradeDryRunDiff(
+    soClient,
+    packagePolicy.id,
+    packagePolicy,
+    installedPackage.version
+  );
 
   if (dryRunResults.hasErrors) {
     const errors = dryRunResults.diff
@@ -100,17 +106,24 @@ async function upgradePackagePolicy(
     appContextService
       .getLogger()
       .error(
-        new Error(`Error upgrading package policy ${packagePolicyId}: ${JSON.stringify(errors)}`)
+        new Error(`Error upgrading package policy ${packagePolicy.id}: ${JSON.stringify(errors)}`)
       );
 
-    results.push({ packagePolicyId, diff: dryRunResults.diff, errors });
+    results.push({ packagePolicyId: packagePolicy.id, diff: dryRunResults.diff, errors });
     return;
   }
 
   try {
-    await packagePolicyService.upgrade(soClient, esClient, [packagePolicyId]);
-    results.push({ packagePolicyId, diff: dryRunResults.diff, errors: [] });
+    await packagePolicyService.upgrade(
+      soClient,
+      esClient,
+      [packagePolicy.id],
+      undefined,
+      packagePolicy,
+      installedPackage.version
+    );
+    results.push({ packagePolicyId: packagePolicy.id, diff: dryRunResults.diff, errors: [] });
   } catch (error) {
-    results.push({ packagePolicyId, diff: dryRunResults.diff, errors: [error] });
+    results.push({ packagePolicyId: packagePolicy.id, diff: dryRunResults.diff, errors: [error] });
   }
 }
diff --git a/x-pack/plugins/fleet/server/services/package_policy.test.ts b/x-pack/plugins/fleet/server/services/package_policy.test.ts
@@ -39,7 +39,6 @@ import type {
   NewPackagePolicy,
   NewPackagePolicyInput,
   PackagePolicyPackage,
-  RegistryPackage,
 } from '../../common';
 import { packageToPackagePolicy } from '../../common';
 
@@ -50,9 +49,11 @@ import {
   updatePackageInputs,
   packagePolicyService,
   _applyIndexPrivileges,
+  _compilePackagePolicyInputs,
 } from './package_policy';
 import { appContextService } from './app_context';
-import { fetchInfo } from './epm/registry';
+
+import { getPackageInfo } from './epm/packages';
 
 async function mockedGetAssetsData(_a: any, _b: any, dataset: string) {
   if (dataset === 'dataset1') {
@@ -113,10 +114,6 @@ async function mockedGetPackageInfo(params: any) {
   return Promise.resolve(pkg);
 }
 
-function mockedRegistryInfo(): RegistryPackage {
-  return {} as RegistryPackage;
-}
-
 jest.mock('./epm/packages/assets', () => {
   return {
     getAssetsData: mockedGetAssetsData,
@@ -125,7 +122,7 @@ jest.mock('./epm/packages/assets', () => {
 
 jest.mock('./epm/packages', () => {
   return {
-    getPackageInfo: mockedGetPackageInfo,
+    getPackageInfo: jest.fn().mockImplementation(mockedGetPackageInfo),
     getInstallation: mockedGetInstallation,
   };
 });
@@ -170,18 +167,12 @@ jest.mock('./upgrade_sender', () => {
   };
 });
 
-const mockedFetchInfo = fetchInfo as jest.Mock<ReturnType<typeof fetchInfo>>;
-
 type CombinedExternalCallback = PutPackagePolicyUpdateCallback | PostPackagePolicyCreateCallback;
 
 describe('Package policy service', () => {
-  beforeEach(() => {
-    mockedFetchInfo.mockResolvedValue({} as RegistryPackage);
-  });
   describe('_compilePackagePolicyInputs', () => {
     it('should work with config variables from the stream', async () => {
-      const inputs = await packagePolicyService._compilePackagePolicyInputs(
-        mockedRegistryInfo(),
+      const inputs = await _compilePackagePolicyInputs(
         {
           data_streams: [
             {
@@ -244,8 +235,7 @@ describe('Package policy service', () => {
     });
 
     it('should work with a two level dataset name', async () => {
-      const inputs = await packagePolicyService._compilePackagePolicyInputs(
-        mockedRegistryInfo(),
+      const inputs = await _compilePackagePolicyInputs(
         {
           data_streams: [
             {
@@ -297,8 +287,7 @@ describe('Package policy service', () => {
     });
 
     it('should work with config variables at the input level', async () => {
-      const inputs = await packagePolicyService._compilePackagePolicyInputs(
-        mockedRegistryInfo(),
+      const inputs = await _compilePackagePolicyInputs(
         {
           data_streams: [
             {
@@ -361,8 +350,7 @@ describe('Package policy service', () => {
     });
 
     it('should work with config variables at the package level', async () => {
-      const inputs = await packagePolicyService._compilePackagePolicyInputs(
-        mockedRegistryInfo(),
+      const inputs = await _compilePackagePolicyInputs(
         {
           data_streams: [
             {
@@ -430,8 +418,7 @@ describe('Package policy service', () => {
     });
 
     it('should work with an input with a template and no streams', async () => {
-      const inputs = await packagePolicyService._compilePackagePolicyInputs(
-        mockedRegistryInfo(),
+      const inputs = await _compilePackagePolicyInputs(
         {
           data_streams: [],
           policy_templates: [
@@ -473,8 +460,7 @@ describe('Package policy service', () => {
     });
 
     it('should work with an input with a template and streams', async () => {
-      const inputs = await packagePolicyService._compilePackagePolicyInputs(
-        mockedRegistryInfo(),
+      const inputs = await _compilePackagePolicyInputs(
         {
           data_streams: [
             {
@@ -579,8 +565,7 @@ describe('Package policy service', () => {
     });
 
     it('should work with a package without input', async () => {
-      const inputs = await packagePolicyService._compilePackagePolicyInputs(
-        mockedRegistryInfo(),
+      const inputs = await _compilePackagePolicyInputs(
         {
           policy_templates: [
             {
@@ -596,8 +581,7 @@ describe('Package policy service', () => {
     });
 
     it('should work with a package with a empty inputs array', async () => {
-      const inputs = await packagePolicyService._compilePackagePolicyInputs(
-        mockedRegistryInfo(),
+      const inputs = await _compilePackagePolicyInputs(
         {
           policy_templates: [
             {
@@ -906,14 +890,16 @@ describe('Package policy service', () => {
         ...mockPackagePolicy,
         inputs: [],
       };
-
-      mockedFetchInfo.mockResolvedValue({
-        elasticsearch: {
-          privileges: {
-            cluster: ['monitor'],
+      (getPackageInfo as jest.Mock).mockImplementation(async (params) => {
+        return Promise.resolve({
+          ...(await mockedGetPackageInfo(params)),
+          elasticsearch: {
+            privileges: {
+              cluster: ['monitor'],
+            },
           },
-        },
-      } as RegistryPackage);
+        } as PackageInfo);
+      });
 
       savedObjectsClient.get.mockResolvedValue({
         id: 'test',