Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Ingest Manager] Remove fields from index pattern during package uninstall #80082

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it adds then immediately removes the package. Is that what's happening?

Copy link
Contributor Author

@jonathan-buttner jonathan-buttner Oct 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah this is just ensuring that the package was installed to begin with so the fields get added and then immediately removes it to setup the tests to validate the uninstall scenario. I got in a weird state locally when running the tests where the package was never actually installed the first time.

await uninstallPackage(pkgKey);
});
it('should have uninstalled the index templates', async function () {
Expand Down Expand Up @@ -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 () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused about which branch we expect to execute. The try or the catch?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both could happen. 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 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, interesting. Maybe we could add that as a comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah sure I'll put it as a comment.

// 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 {
Expand Down