From 60c78f3d6cbe12c5121df0517f6b6dc3e0273cdb Mon Sep 17 00:00:00 2001
From: Jonathan Buttner <56361221+jonathan-buttner@users.noreply.github.com>
Date: Mon, 12 Oct 2020 13:19:50 -0400
Subject: [PATCH] [Ingest Manager] Remove fields from index pattern during
 package uninstall (#80082)

* Correctly removing index pattern fields

* Adding comment about try/catch
---
 .../server/services/epm/packages/remove.ts    |  8 ++--
 .../apis/epm/install_remove_assets.ts         | 48 +++++++++++++++++++
 2 files changed, 53 insertions(+), 3 deletions(-)

diff --git a/x-pack/plugins/ingest_manager/server/services/epm/packages/remove.ts b/x-pack/plugins/ingest_manager/server/services/epm/packages/remove.ts
index 417f2871a6cbf5..8d5995f92c95f2 100644
--- a/x-pack/plugins/ingest_manager/server/services/epm/packages/remove.ts
+++ b/x-pack/plugins/ingest_manager/server/services/epm/packages/remove.ts
@@ -44,9 +44,6 @@ export async function removeInstallation(options: {
       `unable to remove package with existing package policy(s) in use by agent(s)`
     );
 
-  // recreate or delete index patterns when a package is uninstalled
-  await installIndexPatterns(savedObjectsClient);
-
   // Delete the installed assets
   const installedAssets = [...installation.installed_kibana, ...installation.installed_es];
   await deleteAssets(installedAssets, savedObjectsClient, callCluster);
@@ -55,6 +52,11 @@ export async function removeInstallation(options: {
   // could also update with [] or some other state
   await savedObjectsClient.delete(PACKAGES_SAVED_OBJECT_TYPE, pkgName);
 
+  // recreate or delete index patterns when a package is uninstalled
+  // this must be done after deleting the saved object for the current package otherwise it will retrieve the package
+  // from the registry again and reinstall the index patterns
+  await installIndexPatterns(savedObjectsClient);
+
   // remove the package archive and its contents from the cache so that a reinstall fetches
   // a fresh copy from the registry
   deletePackageCache(pkgName, pkgVersion);
diff --git a/x-pack/test/ingest_manager_api_integration/apis/epm/install_remove_assets.ts b/x-pack/test/ingest_manager_api_integration/apis/epm/install_remove_assets.ts
index 16a500ddf85ee1..cc6a384dcaafe6 100644
--- a/x-pack/test/ingest_manager_api_integration/apis/epm/install_remove_assets.ts
+++ b/x-pack/test/ingest_manager_api_integration/apis/epm/install_remove_assets.ts
@@ -35,6 +35,9 @@ export default function (providerContext: FtrProviderContext) {
       before(async () => {
         await installPackage(pkgKey);
       });
+      after(async () => {
+        await uninstallPackage(pkgKey);
+      });
       it('should have installed the ILM policy', async function () {
         const resPolicy = await es.transport.request({
           method: 'GET',
@@ -218,6 +221,9 @@ export default function (providerContext: FtrProviderContext) {
     describe('uninstalls all assets when uninstalling a package', async () => {
       skipIfNoDockerRegistry(providerContext);
       before(async () => {
+        // these tests ensure that uninstall works properly so make sure that the package gets installed and uninstalled
+        // and then we'll test that not artifacts are left behind.
+        await installPackage(pkgKey);
         await uninstallPackage(pkgKey);
       });
       it('should have uninstalled the index templates', async function () {
@@ -342,6 +348,48 @@ export default function (providerContext: FtrProviderContext) {
         }
         expect(resSearch.response.data.statusCode).equal(404);
       });
+      it('should have removed the fields from the index patterns', async () => {
+        // The reason there is an expect inside the try and inside the catch in this test case is to guard against two
+        // different scenarios.
+        //
+        // If a test case in another file calls /setup then the system and endpoint packages will be installed and
+        // will be present for the remainder of the tests (because they cannot be removed). If that is the case the
+        // expect in the try will work because the logs-* and metrics-* index patterns will still be present even
+        // after this test uninstalls its package.
+        //
+        // If /setup was never called prior to this test, when the test package is uninstalled the index pattern code
+        // checks to see if there are no packages installed and completely removes the logs-* and metrics-* index
+        // patterns. If that happens this code will throw an error and indicate that the index pattern being searched
+        // for was completely removed. In this case the catch's expect will test to make sure the error thrown was
+        // a 404 because all of the packages have been removed.
+        try {
+          const resIndexPatternLogs = await kibanaServer.savedObjects.get({
+            type: 'index-pattern',
+            id: 'logs-*',
+          });
+          const fields = JSON.parse(resIndexPatternLogs.attributes.fields);
+          const exists = fields.find((field: { name: string }) => field.name === 'logs_test_name');
+          expect(exists).to.be(undefined);
+        } catch (err) {
+          // if all packages are uninstalled there won't be a logs-* index pattern
+          expect(err.response.data.statusCode).equal(404);
+        }
+
+        try {
+          const resIndexPatternMetrics = await kibanaServer.savedObjects.get({
+            type: 'index-pattern',
+            id: 'metrics-*',
+          });
+          const fieldsMetrics = JSON.parse(resIndexPatternMetrics.attributes.fields);
+          const existsMetrics = fieldsMetrics.find(
+            (field: { name: string }) => field.name === 'metrics_test_name'
+          );
+          expect(existsMetrics).to.be(undefined);
+        } catch (err) {
+          // if all packages are uninstalled there won't be a metrics-* index pattern
+          expect(err.response.data.statusCode).equal(404);
+        }
+      });
       it('should have removed the saved object', async function () {
         let res;
         try {