Skip to content
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
76df74d
Adding bulk upgrade api
jonathan-buttner Sep 17, 2020
6dea606
Addressing comments
jonathan-buttner Sep 18, 2020
268c526
Removing todo
jonathan-buttner Sep 18, 2020
04a39bd
Resolving conflicts
jonathan-buttner Sep 18, 2020
f111406
Changing body field
jonathan-buttner Sep 18, 2020
2846b76
Adding helper for getting the bulk install route
jonathan-buttner Sep 18, 2020
20357fd
Adding request spec
jonathan-buttner Sep 18, 2020
9b29802
Pulling in Johns changes
jonathan-buttner Sep 21, 2020
3d2a686
Merge branch 'master' of github.com:elastic/kibana into ingest-bulk-u…
jonathan-buttner Sep 21, 2020
7cb80e9
Removing test for same package upgraded multiple times
jonathan-buttner Sep 21, 2020
6e333ab
Adding upgrade to setup route
jonathan-buttner Sep 21, 2020
99f6aee
Fixing merge conflicts
jonathan-buttner Sep 22, 2020
8a41396
Adding setup integration test
jonathan-buttner Sep 22, 2020
219cfa9
Clean up error handling
jonathan-buttner Sep 24, 2020
ba00737
Beginning to add tests
jonathan-buttner Sep 24, 2020
267931b
Merge branch 'master' of github.com:elastic/kibana into ingest-setup-…
jonathan-buttner Sep 24, 2020
037ba98
Failing jest mock tests
jonathan-buttner Sep 24, 2020
a76006f
Break up tests & modules for easier testing.
Sep 25, 2020
5c259d1
Add test confirming update error result will throw
Sep 25, 2020
06cfadc
Keep orig error. Add status code in http handler
Sep 26, 2020
a3c77ce
Leave error as-is
Sep 27, 2020
25c3cd0
Removing accidental code changes. File rename.
Sep 27, 2020
202b7a2
Missed a function when moving to a new file
Sep 27, 2020
ee4c774
Add missing type imports
Sep 27, 2020
c906aea
Lift .map lambda into named outer function
Sep 27, 2020
11b677d
Merge pull request #7 from jfsiii/ingest-setup-upgrade
jonathan-buttner Sep 28, 2020
4afbb56
Merge branch 'master' of github.com:elastic/kibana into ingest-setup-…
jonathan-buttner Sep 28, 2020
3f8295d
Adding additional test
jonathan-buttner Sep 28, 2020
7f02b5f
Fixing type error
jonathan-buttner Sep 28, 2020
0427b9d
Merge branch 'master' into ingest-setup-upgrade
elasticmachine Sep 28, 2020
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 @@ -74,7 +74,7 @@ export interface InstallPackageResponse {
export interface IBulkInstallPackageError {
name: string;
statusCode: number;
error: string | Error;
error: string;
}

export interface BulkInstallPackageInfo {
Expand Down
39 changes: 39 additions & 0 deletions x-pack/plugins/ingest_manager/server/errors/handlers.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {
RegistryError,
PackageNotFoundError,
defaultIngestErrorHandler,
BulkInstallPackagesError,
} from './index';

const LegacyESErrors = errors as Record<string, any>;
Expand Down Expand Up @@ -120,6 +121,44 @@ describe('defaultIngestErrorHandler', () => {
expect(mockContract.logger?.error).toHaveBeenCalledWith(error.message);
});

it('500: BulkInstallPackagesError', async () => {
const error = new BulkInstallPackagesError(500, '123');
const response = httpServerMock.createResponseFactory();

await defaultIngestErrorHandler({ error, response });

// response
expect(response.ok).toHaveBeenCalledTimes(0);
expect(response.customError).toHaveBeenCalledTimes(1);
expect(response.customError).toHaveBeenCalledWith({
statusCode: 500,
body: { message: error.message },
});

// logging
expect(mockContract.logger?.error).toHaveBeenCalledTimes(1);
expect(mockContract.logger?.error).toHaveBeenCalledWith(error.message);
});

it('400: BulkInstallPackagesError', async () => {
const error = new BulkInstallPackagesError(400, '123');
const response = httpServerMock.createResponseFactory();

await defaultIngestErrorHandler({ error, response });

// response
expect(response.ok).toHaveBeenCalledTimes(0);
expect(response.customError).toHaveBeenCalledTimes(1);
expect(response.customError).toHaveBeenCalledWith({
statusCode: 400,
body: { message: error.message },
});

// logging
expect(mockContract.logger?.error).toHaveBeenCalledTimes(1);
expect(mockContract.logger?.error).toHaveBeenCalledWith(error.message);
});

it('400: IngestManagerError', async () => {
const error = new IngestManagerError('123');
const response = httpServerMock.createResponseFactory();
Expand Down
10 changes: 9 additions & 1 deletion x-pack/plugins/ingest_manager/server/errors/handlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,12 @@ import {
} from 'src/core/server';
import { errors as LegacyESErrors } from 'elasticsearch';
import { appContextService } from '../services';
import { IngestManagerError, RegistryError, PackageNotFoundError } from './index';
import {
IngestManagerError,
RegistryError,
PackageNotFoundError,
BulkInstallPackagesError,
} from './index';

type IngestErrorHandler = (
params: IngestErrorHandlerParams
Expand Down Expand Up @@ -52,6 +57,9 @@ const getHTTPResponseCode = (error: IngestManagerError): number => {
if (error instanceof PackageNotFoundError) {
return 404; // Not Found
}
if (error instanceof BulkInstallPackagesError) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's ignore type here and test 'statusCode' in error

Maybe return 'statusCode' in error ? error.statusCode : 400

return error.statusCode;
}

return 400; // Bad Request
};
Expand Down
6 changes: 6 additions & 0 deletions x-pack/plugins/ingest_manager/server/errors/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,9 @@ export class RegistryConnectionError extends RegistryError {}
export class RegistryResponseError extends RegistryError {}
export class PackageNotFoundError extends IngestManagerError {}
export class PackageOutdatedError extends IngestManagerError {}

export class BulkInstallPackagesError extends IngestManagerError {
Copy link
Contributor

Choose a reason for hiding this comment

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

We avoided this for RegistryResponse and the other types. I'd like to avoid it here if possible. At worst, I think it should be a single IBulkInstallError arg, but I'm hoping we can avoid all together

constructor(public readonly statusCode: number, message?: string) {
super(message);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,32 @@

import { ElasticsearchAssetType, Installation, KibanaAssetType } from '../../../types';
import { SavedObject } from 'src/core/server';
import { getInstallType } from './install';
jest.mock('./install', () => ({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jfsiii any ideas what I'm doing wrong here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't at first, but I think we have it now. Take a look at https://github.com/jfsiii/kibana/commit/a76006f88d8061545fa698c3b630345fb9463bd5

I'll add some comments over on that commit and I can submit a PR or you can pull in whatever you wish

Update I created jonathan-buttner#7

...(jest.requireActual('./install') as {}),
bulkInstallPackages: jest.fn(async () => {
return [
{
name: 'blah',
assets: [],
newVersion: '',
oldVersion: '',
statusCode: 200,
},
];
}),
}));

jest.mock('./get', () => ({
...(jest.requireActual('./get') as {}),
getInstallation: jest.fn(async () => {
return mockInstallation.attributes;
}),
}));
import { getInstallType, ensureInstalledDefaultPackages } from './install';

import { savedObjectsClientMock } from 'src/core/server/mocks';
import { appContextService } from '../../app_context';
import { createAppContextStartContractMock } from '../../../mocks';

const mockInstallation: SavedObject<Installation> = {
id: 'test-pkg',
Expand Down Expand Up @@ -41,6 +66,19 @@ const mockInstallationUpdateFail: SavedObject<Installation> = {
},
};
describe('install', () => {
describe('ensureInstalledDefaultPackages', () => {
beforeEach(async () => {
appContextService.start(createAppContextStartContractMock());
});
afterEach(async () => {
appContextService.stop();
});
it('should return an array of Installation objects when successful', async () => {
const soClient = savedObjectsClientMock.create();
const resp = await ensureInstalledDefaultPackages(soClient, jest.fn());
expect(resp).toEqual([mockInstallation.attributes]);
});
});
describe('getInstallType', () => {
it('should return correct type when installing and no other version is currently installed', () => {
const installTypeInstall = getInstallType({ pkgVersion: '1.0.0', installedPkg: undefined });
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ import {
IngestManagerError,
PackageOutdatedError,
ingestErrorToResponseOptions,
BulkInstallPackagesError,
} from '../../../errors';
import { getPackageSavedObjects } from './get';
import { installTransformForDataset } from '../elasticsearch/transform/install';
Expand All @@ -63,22 +64,36 @@ export async function installLatestPackage(options: {
}
}

function isBulkInstallError(resp: BulkInstallResponse): resp is IBulkInstallPackageError {
return 'error' in resp && resp.error !== undefined;
}

export async function ensureInstalledDefaultPackages(
savedObjectsClient: SavedObjectsClientContract,
callCluster: CallESAsCurrentUser
): Promise<Installation[]> {
const installations = [];
for (const pkgName in DefaultPackages) {
if (!DefaultPackages.hasOwnProperty(pkgName)) continue;
const installation = ensureInstalledPackage({
savedObjectsClient,
pkgName,
callCluster,
});
installations.push(installation);
const bulkResponse = await bulkInstallPackages({
savedObjectsClient,
packagesToUpgrade: Object.values(DefaultPackages),
callCluster,
});

for (const resp of bulkResponse) {
if (isBulkInstallError(resp)) {
throw new BulkInstallPackagesError(resp.statusCode, resp.error);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: the guard only checks for error so statusCode could be missing here

} else {
installations.push(getInstallation({ savedObjectsClient, pkgName: resp.name }));
}
}

return Promise.all(installations);
const retrievedInstallations = await Promise.all(installations);
return retrievedInstallations.map((installation, index) => {
if (!installation) {
throw new Error(`could not get installation ${bulkResponse[index].name} during setup`);
}
return installation;
});
}

export async function ensureInstalledPackage(options: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
export default function loadTests({ loadTestFile }) {
describe('EPM Endpoints', () => {
loadTestFile(require.resolve('./list'));
loadTestFile(require.resolve('./setup'));
loadTestFile(require.resolve('./get'));
loadTestFile(require.resolve('./file'));
//loadTestFile(require.resolve('./template'));
Expand Down
48 changes: 48 additions & 0 deletions x-pack/test/ingest_manager_api_integration/apis/epm/setup.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import expect from '@kbn/expect';
import { FtrProviderContext } from '../../../api_integration/ftr_provider_context';
import { skipIfNoDockerRegistry } from '../../helpers';
import { GetInfoResponse, Installed } from '../../../../plugins/ingest_manager/common';

export default function (providerContext: FtrProviderContext) {
const { getService } = providerContext;
const supertest = getService('supertest');
const log = getService('log');

describe('setup api', async () => {
Copy link
Contributor

@jfsiii jfsiii Sep 22, 2020

Choose a reason for hiding this comment

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

Can you add some tests that show the failure case(s)? I'm unclear what we intend to be returned from a POST /setup that has a failed upgrade at some point. Is it a 2xx, 4xx, or 5xx response? What's in the response body?

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 so adding the failure case might be tricky. Any ideas on how to trigger a failure? This is what comes to mind:

  1. Override the default packages variable used to define which packages are installed/updated during /setup so I can create a custom one and make it malformed. Is this possible with mocha?
  2. Use an invalid registry address so kibana fails to download the packages. I believe this would require creating a new x-pack/test directory because the tests in this directory all use the docker container registry
  3. Create a malformed endpoint package in this directory and make the version really big like 100000.0.0 so it's always the latest. The number needs to be large so it's unlikely to be superseded in production. The test would start failing as soon as we created a production package with version 100000.0.1 though 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

If we can map out the desired behavior I am happy to help with testing. I did a few different types recently working on Registry and /setup changes.

If this is run on every setup I really want to understand/document what it should/does do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll have to do some digging to figure out what should be returned in each failure scenario. After thinking about this more, the code changes will currently swallow the error :( because of this:

if (isBulkInstallError(resp)) {
      throw new Error(resp.error);
    }

from here: https://github.com/elastic/kibana/pull/78081/files#diff-bae6a46773afa98cb0927abf59bdd003R93

That's probably not what we want but in the current state if an error occurs it will always 500 and the response body will look like this:

body: {
  message: "some error"
}

https://github.com/elastic/kibana/blob/master/x-pack/plugins/ingest_manager/server/errors/handlers.ts#L100

The success case will always return:
200

{
  isInitialized: true
}

For the error case we could probably create a new error class that is a child of IngestManagerError and modify this function: https://github.com/elastic/kibana/blob/master/x-pack/plugins/ingest_manager/server/errors/handlers.ts#L48

to extract out the passed in status code. That way we can preserve the status code and message that occurred when the upgrade/install failed for a package and return that as a response for /setup. That way the failure functionality will change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@skh @neptunian any ideas on how to force a failure scenario in /setup? Is there a way to mock out DefaultPackages so that I can craft a fake package that causes a failure when being installed?
https://github.com/elastic/kibana/blob/master/x-pack/plugins/ingest_manager/common/types/models/epm.ts#L265

skipIfNoDockerRegistry(providerContext);
describe('setup performs upgrades', async () => {
const oldEndpointVersion = '0.13.0';
beforeEach(async () => {
await supertest
.post(`/api/ingest_manager/epm/packages/endpoint-${oldEndpointVersion}`)
.set('kbn-xsrf', 'xxxx')
.send({ force: true })
.expect(200);
});
it('upgrades the endpoint package from 0.13.0 to the latest version available', async function () {
let { body }: { body: GetInfoResponse } = await supertest
.get(`/api/ingest_manager/epm/packages/endpoint-${oldEndpointVersion}`)
.expect(200);
const latestEndpointVersion = body.response.latestVersion;
log.info(`Endpoint package latest version: ${latestEndpointVersion}`);
// make sure we're actually doing an upgrade
expect(latestEndpointVersion).not.eql(oldEndpointVersion);
await supertest.post(`/api/ingest_manager/setup`).set('kbn-xsrf', 'xxxx').expect(200);

({ body } = await supertest
.get(`/api/ingest_manager/epm/packages/endpoint-${latestEndpointVersion}`)
.expect(200));
expect(body.response).to.have.property('savedObject');
expect((body.response as Installed).savedObject.attributes.install_version).to.eql(
latestEndpointVersion
);
});
});
});
}