[Fleet] refactor install registry and upload to extract common logic (#…

…150444) ## Summary Related to #148599 Closes #82007 Factored out common logic in `installPackageFromRegistry` and `installPackageByUpload` to a new function. This improves the upload flow as well to handle install failure. We might want to introduce a `force` flag for the upload API to reinstall a package, now the logic does not do anything if the installed version package is uploaded again. EDIT: found [here](#82007) that upload should work with an implicit `force` flag, so will update that. More manual and automated tests needed. ### Checklist - [ ] [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
elastic · Feb 8, 2023 · 67d1a46 · 67d1a46
1 parent c190a5f
commit 67d1a46
Show file tree

Hide file tree

Showing 2 changed files with 169 additions and 101 deletions.
diff --git a/x-pack/plugins/fleet/server/services/epm/packages/install.test.ts b/x-pack/plugins/fleet/server/services/epm/packages/install.test.ts
@@ -233,6 +233,50 @@ describe('install', () => {
         expect.objectContaining({ installSource: 'upload' })
       );
     });
+
+    it('should fetch latest version if version not provided', async () => {
+      jest.spyOn(licenseService, 'hasAtLeast').mockReturnValue(true);
+      const response = await installPackage({
+        spaceId: DEFAULT_SPACE_ID,
+        installSource: 'registry',
+        pkgkey: 'test_package',
+        savedObjectsClient: savedObjectsClientMock.create(),
+        esClient: {} as ElasticsearchClient,
+      });
+
+      expect(response.status).toEqual('installed');
+
+      expect(sendTelemetryEvents).toHaveBeenCalledWith(
+        expect.anything(),
+        undefined,
+        expect.objectContaining({
+          newVersion: '1.3.0',
+        })
+      );
+    });
+
+    it('should do nothing if same version is installed', async () => {
+      jest.spyOn(obj, 'getInstallationObject').mockImplementationOnce(() =>
+        Promise.resolve({
+          attributes: {
+            version: '1.2.0',
+            install_status: 'installed',
+            installed_es: [],
+            installed_kibana: [],
+          },
+        } as any)
+      );
+      jest.spyOn(licenseService, 'hasAtLeast').mockReturnValue(true);
+      const response = await installPackage({
+        spaceId: DEFAULT_SPACE_ID,
+        installSource: 'registry',
+        pkgkey: 'apache-1.2.0',
+        savedObjectsClient: savedObjectsClientMock.create(),
+        esClient: {} as ElasticsearchClient,
+      });
+
+      expect(response.status).toEqual('already_installed');
+    });
   });
 
   describe('upload', () => {

diff --git a/x-pack/plugins/fleet/server/services/epm/packages/install.ts b/x-pack/plugins/fleet/server/services/epm/packages/install.ts
@@ -30,6 +30,7 @@ import { FLEET_INSTALL_FORMAT_VERSION } from '../../../constants/fleet_es_assets
 import { generateESIndexPatterns } from '../elasticsearch/template/template';
 
 import type {
+  ArchivePackage,
   BulkInstallPackageInfo,
   EpmPackageInstallStatus,
   EsAssetReference,
@@ -293,27 +294,20 @@ async function installPackageFromRegistry({
   const logger = appContextService.getLogger();
   // TODO: change epm API to /packageName/version so we don't need to do this
   const { pkgName, pkgVersion: version } = Registry.splitPkgKey(pkgkey);
-  let pkgVersion = version;
-
-  // Workaround apm issue with async spans: https://github.com/elastic/apm-agent-nodejs/issues/2611
-  await Promise.resolve();
-  const span = apm.startSpan(`Install package from registry ${pkgName}@${pkgVersion}`, 'package');
+  let pkgVersion = version ?? '';
 
   // if an error happens during getInstallType, report that we don't know
   let installType: InstallType = 'unknown';
-
+  const installSource = 'registry';
   const telemetryEvent: PackageUpdateEvent = getTelemetryEvent(pkgName, pkgVersion);
 
   try {
     // get the currently installed package
     const installedPkg = await getInstallationObject({ savedObjectsClient, pkgName });
     installType = getInstallType({ pkgVersion, installedPkg });
 
-    span?.addLabels({
-      packageName: pkgName,
-      packageVersion: pkgVersion,
-      installType,
-    });
+    telemetryEvent.installType = installType;
+    telemetryEvent.currentVersion = installedPkg?.attributes.version || 'not_installed';
 
     const queryLatest = () =>
       Registry.fetchFindLatestPackageOrThrow(pkgName, {
@@ -340,6 +334,100 @@ async function installPackageFromRegistry({
     const installOutOfDateVersionOk =
       force || ['reinstall', 'reupdate', 'rollback'].includes(installType);
 
+    // if the requested version is out-of-date of the latest package version, check if we allow it
+    // if we don't allow it, return an error
+    if (semverLt(pkgVersion, latestPackage.version)) {
+      if (!installOutOfDateVersionOk) {
+        throw new PackageOutdatedError(
+          `${pkgkey} is out-of-date and cannot be installed or updated`
+        );
+      }
+      logger.debug(
+        `${pkgkey} is out-of-date, installing anyway due to ${
+          force ? 'force flag' : `install type ${installType}`
+        }`
+      );
+    }
+
+    return await installPackageCommon({
+      pkgName,
+      pkgVersion,
+      installSource,
+      installedPkg,
+      installType,
+      savedObjectsClient,
+      esClient,
+      spaceId,
+      force,
+      packageInfo,
+      paths,
+      verificationResult,
+    });
+  } catch (e) {
+    sendEvent({
+      ...telemetryEvent,
+      errorMessage: e.message,
+    });
+    return {
+      error: e,
+      installType,
+      installSource,
+    };
+  }
+}
+
+async function installPackageCommon(options: {
+  pkgName: string;
+  pkgVersion: string;
+  installSource: 'registry' | 'upload';
+  installedPkg?: SavedObject<Installation>;
+  installType: InstallType;
+  savedObjectsClient: SavedObjectsClientContract;
+  esClient: ElasticsearchClient;
+  spaceId: string;
+  force?: boolean;
+  packageInfo: ArchivePackage;
+  paths: string[];
+  verificationResult?: PackageVerificationResult;
+  telemetryEvent?: PackageUpdateEvent;
+}): Promise<InstallResult> {
+  const {
+    pkgName,
+    pkgVersion,
+    installSource,
+    installedPkg,
+    installType,
+    savedObjectsClient,
+    force,
+    esClient,
+    spaceId,
+    packageInfo,
+    paths,
+    verificationResult,
+  } = options;
+  let { telemetryEvent } = options;
+  const logger = appContextService.getLogger();
+
+  // Workaround apm issue with async spans: https://github.com/elastic/apm-agent-nodejs/issues/2611
+  await Promise.resolve();
+  const span = apm.startSpan(
+    `Install package from ${installSource} ${pkgName}@${pkgVersion}`,
+    'package'
+  );
+
+  if (!telemetryEvent) {
+    telemetryEvent = getTelemetryEvent(pkgName, pkgVersion);
+    telemetryEvent.installType = installType;
+    telemetryEvent.currentVersion = installedPkg?.attributes.version || 'not_installed';
+  }
+
+  try {
+    span?.addLabels({
+      packageName: pkgName,
+      packageVersion: pkgVersion,
+      installType,
+    });
+
     // if the requested version is the same as installed version, check if we allow it based on
     // current installed package status and force flag, if we don't allow it,
     // just return the asset references from the existing installation
@@ -348,44 +436,26 @@ async function installPackageFromRegistry({
       installedPkg?.attributes.install_status === 'installed'
     ) {
       if (!force) {
-        logger.debug(`${pkgkey} is already installed, skipping installation`);
+        logger.debug(`${pkgName}-${pkgVersion} is already installed, skipping installation`);
         return {
           assets: [
             ...installedPkg.attributes.installed_es,
             ...installedPkg.attributes.installed_kibana,
           ],
           status: 'already_installed',
           installType,
-          installSource: 'registry',
+          installSource,
         };
       }
     }
 
-    telemetryEvent.installType = installType;
-    telemetryEvent.currentVersion = installedPkg?.attributes.version || 'not_installed';
-
-    // if the requested version is out-of-date of the latest package version, check if we allow it
-    // if we don't allow it, return an error
-    if (semverLt(pkgVersion, latestPackage.version)) {
-      if (!installOutOfDateVersionOk) {
-        throw new PackageOutdatedError(
-          `${pkgkey} is out-of-date and cannot be installed or updated`
-        );
-      }
-      logger.debug(
-        `${pkgkey} is out-of-date, installing anyway due to ${
-          force ? 'force flag' : `install type ${installType}`
-        }`
-      );
-    }
-
     if (!licenseService.hasAtLeast(packageInfo.license || 'basic')) {
       const err = new Error(`Requires ${packageInfo.license} license`);
       sendEvent({
         ...telemetryEvent,
         errorMessage: err.message,
       });
-      return { error: err, installType, installSource: 'registry' };
+      return { error: err, installType, installSource };
     }
 
     const savedObjectsImporter = appContextService
@@ -415,7 +485,7 @@ async function installPackageFromRegistry({
       installType,
       spaceId,
       verificationResult,
-      installSource: 'registry',
+      installSource,
     })
       .then(async (assets) => {
         await removeOldAssets({
@@ -424,10 +494,10 @@ async function installPackageFromRegistry({
           currentVersion: packageInfo.version,
         });
         sendEvent({
-          ...telemetryEvent,
+          ...telemetryEvent!,
           status: 'success',
         });
-        return { assets, status: 'installed', installType, installSource: 'registry' };
+        return { assets, status: 'installed', installType, installSource };
       })
       .catch(async (err: Error) => {
         logger.warn(`Failure to install package [${pkgName}]: [${err.toString()}]`);
@@ -441,10 +511,10 @@ async function installPackageFromRegistry({
           esClient,
         });
         sendEvent({
-          ...telemetryEvent,
+          ...telemetryEvent!,
           errorMessage: err.message,
         });
-        return { error: err, installType, installSource: 'registry' };
+        return { error: err, installType, installSource };
       });
   } catch (e) {
     sendEvent({
@@ -454,7 +524,7 @@ async function installPackageFromRegistry({
     return {
       error: e,
       installType,
-      installSource: 'registry',
+      installSource,
     };
   } finally {
     span?.end();
@@ -469,16 +539,12 @@ async function installPackageByUpload({
   spaceId,
   version,
 }: InstallUploadedArchiveParams): Promise<InstallResult> {
-  // Workaround apm issue with async spans: https://github.com/elastic/apm-agent-nodejs/issues/2611
-  await Promise.resolve();
-  const span = apm.startSpan(`Install package from upload`, 'package');
-
-  const logger = appContextService.getLogger();
   // if an error happens during getInstallType, report that we don't know
   let installType: InstallType = 'unknown';
-  const telemetryEvent: PackageUpdateEvent = getTelemetryEvent('', '');
+  const installSource = 'upload';
   try {
     const { packageInfo } = await generatePackageInfoFromArchiveBuffer(archiveBuffer, contentType);
+    const pkgName = packageInfo.name;
 
     // Allow for overriding the version in the manifest for cases where we install
     // stack-aligned bundled packages to support special cases around the
@@ -487,23 +553,11 @@ async function installPackageByUpload({
 
     const installedPkg = await getInstallationObject({
       savedObjectsClient,
-      pkgName: packageInfo.name,
+      pkgName,
     });
 
     installType = getInstallType({ pkgVersion, installedPkg });
 
-    span?.addLabels({
-      packageName: packageInfo.name,
-      packageVersion: pkgVersion,
-      installType,
-    });
-
-    telemetryEvent.packageName = packageInfo.name;
-    telemetryEvent.newVersion = pkgVersion;
-    telemetryEvent.installType = installType;
-    telemetryEvent.currentVersion = installedPkg?.attributes.version || 'not_installed';
-
-    const installSource = 'upload';
     // as we do not verify uploaded packages, we must invalidate the verification cache
     deleteVerificationResult(packageInfo);
     const paths = await unpackBufferToCache({
@@ -519,55 +573,25 @@ async function installPackageByUpload({
       packageInfo,
     });
 
-    const savedObjectsImporter = appContextService
-      .getSavedObjects()
-      .createImporter(savedObjectsClient);
-
-    const savedObjectTagAssignmentService = appContextService
-      .getSavedObjectsTagging()
-      .createInternalAssignmentService({ client: savedObjectsClient });
-
-    const savedObjectTagClient = appContextService
-      .getSavedObjectsTagging()
-      .createTagClient({ client: savedObjectsClient });
-
-    // @ts-expect-error status is string instead of InstallResult.status 'installed' | 'already_installed'
-    return await _installPackage({
+    return await installPackageCommon({
+      pkgName,
+      pkgVersion,
+      installSource,
+      installedPkg,
+      installType,
       savedObjectsClient,
-      savedObjectsImporter,
-      savedObjectTagAssignmentService,
-      savedObjectTagClient,
       esClient,
-      logger,
-      installedPkg,
+      spaceId,
+      force: true, // upload has implicit force
+      packageInfo,
       paths,
-      packageInfo: { ...packageInfo, version: pkgVersion },
+    });
+  } catch (e) {
+    return {
+      error: e,
       installType,
       installSource,
-      spaceId,
-    })
-      .then((assets) => {
-        sendEvent({
-          ...telemetryEvent,
-          status: 'success',
-        });
-        return { assets, status: 'installed', installType };
-      })
-      .catch(async (err: Error) => {
-        sendEvent({
-          ...telemetryEvent,
-          errorMessage: err.message,
-        });
-        return { error: err, installType };
-      });
-  } catch (e) {
-    sendEvent({
-      ...telemetryEvent,
-      errorMessage: e.message,
-    });
-    return { error: e, installType, installSource: 'upload' };
-  } finally {
-    span?.end();
+    };
   }
 }