[Ingest Manager] Ingest setup upgrade #78081

jonathan-buttner · 2020-09-21T20:38:13Z

This PR refactors the /setup api to ensure that the default packages have the latest version installed. This way when a user navigates to the ingest manager app /setup will be called and it will perform an upgrade of the default packages if a later version exists.

This was chosen instead of having the UI perform a /packages/_bulk call on each tab that the user navigates to because that's a bit too frequent than necessary.

…pgrade

elasticmachine · 2020-09-22T19:10:17Z

Pinging @elastic/ingest-management (Feature:EPM)

jonathan-buttner · 2020-09-22T19:12:43Z

x-pack/plugins/ingest_manager/server/services/epm/packages/install.ts

+    } else {
+      // getInstallation can return undefined so we'll tack on the name so when we throw an error in setup we can
+      // reference the name of the package that we failed to retrieve the installation information for
+      const installation = getInstallationAndName(savedObjectsClient, resp.name);


Would it be better to await the getInstallation call here? If we did then I wouldn't have to change the return value and would just check if the result was undefined and throw the error here instead of in setup.ts.

jfsiii · 2020-09-22T19:31:45Z

x-pack/test/ingest_manager_api_integration/apis/epm/setup.ts

+  const supertest = getService('supertest');
+  const log = getService('log');
+
+  describe('setup api', async () => {


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?

Yeah so adding the failure case might be tricky. Any ideas on how to trigger a failure? This is what comes to mind:

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?

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

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 🤔

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.

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.

@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

…upgrade

jonathan-buttner · 2020-09-24T22:07:39Z

x-pack/plugins/ingest_manager/server/services/epm/packages/install.test.ts

@@ -6,7 +6,32 @@

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


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

I didn't at first, but I think we have it now. Take a look at jfsiii@a76006f

I moved the getInstallType suite into its own file to keep it from being affected by any mocking we're doing for install

After reading Testing exported modules in the same file jestjs/jest#1075 (comment) I moved bulkInstallPackages function from packages/index.ts to packages/bulk_install_packages.ts

I updated the mocks to roughly jfsiii@a76006f#diff-05fb9ef3e971ba31131288ee48dc8c51
+ jest.mock('./install'); + jest.mock('./bulk_install_packages');

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

Deal with issue described in jestjs/jest#1075 (comment) epm/packages/install has functions a, b, c which are independent but a can also call b and c function a() { b(); c(); } The linked FB issue describes the cause and rationale (Jest works on "module" boundary) but TL;DR: it's easier if you split up your files Some related links I found during this journey * https://medium.com/@qjli/how-to-mock-specific-module-function-in-jest-715e39a391f4 * https://stackoverflow.com/questions/52650367/jestjs-how-to-test-function-being-called-inside-another-function * https://stackoverflow.com/questions/50854440/spying-on-an-imported-function-that-calls-another-function-in-jest/50855968#50855968

jfsiii · 2020-09-24T22:59:42Z

x-pack/plugins/ingest_manager/server/errors/handlers.ts

@@ -52,6 +57,9 @@ const getHTTPResponseCode = (error: IngestManagerError): number => {
  if (error instanceof PackageNotFoundError) {
    return 404; // Not Found
  }
+  if (error instanceof BulkInstallPackagesError) {


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

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

jfsiii · 2020-09-24T23:15:06Z

x-pack/plugins/ingest_manager/server/errors/index.ts

@@ -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 {


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

jfsiii · 2020-09-25T17:17:27Z

x-pack/plugins/ingest_manager/server/services/epm/packages/install.test.ts

@@ -6,7 +6,32 @@

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


I didn't at first, but I think we have it now. Take a look at jfsiii@a76006f

I moved the getInstallType suite into its own file to keep it from being affected by any mocking we're doing for install

After reading Testing exported modules in the same file jestjs/jest#1075 (comment) I moved bulkInstallPackages function from packages/index.ts to packages/bulk_install_packages.ts

I updated the mocks to roughly jfsiii@a76006f#diff-05fb9ef3e971ba31131288ee48dc8c51
+ jest.mock('./install'); + jest.mock('./bulk_install_packages');

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

jfsiii · 2020-09-25T17:19:45Z

x-pack/plugins/ingest_manager/server/services/epm/packages/install.ts

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


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

Ingest setup upgrade

…upgrade

jfsiii

🚀 nice work. thanks for the collaboration. I learned a lot about Jest mocking and like how the tests turned out

jonathan-buttner · 2020-09-28T17:36:35Z

@elasticmachine merge upstream

kibanamachine · 2020-09-28T19:06:21Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 0427b9d

Metrics [docs]

distributable file count

id	value	diff	baseline
default	45672	+1	45671

History

💔 Build #77581 failed 7f02b5f
💔 Build #77469 failed 3f8295d
💔 Build #77402 failed 11b677d
💔 Build #77146 failed 037ba98
💚 Build #76291 succeeded 8a41396

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

* Adding bulk upgrade api * Addressing comments * Removing todo * Changing body field * Adding helper for getting the bulk install route * Adding request spec * Pulling in Johns changes * Removing test for same package upgraded multiple times * Adding upgrade to setup route * Adding setup integration test * Clean up error handling * Beginning to add tests * Failing jest mock tests * Break up tests & modules for easier testing. Deal with issue described in jestjs/jest#1075 (comment) epm/packages/install has functions a, b, c which are independent but a can also call b and c function a() { b(); c(); } The linked FB issue describes the cause and rationale (Jest works on "module" boundary) but TL;DR: it's easier if you split up your files Some related links I found during this journey * https://medium.com/@qjli/how-to-mock-specific-module-function-in-jest-715e39a391f4 * https://stackoverflow.com/questions/52650367/jestjs-how-to-test-function-being-called-inside-another-function * https://stackoverflow.com/questions/50854440/spying-on-an-imported-function-that-calls-another-function-in-jest/50855968#50855968 * Add test confirming update error result will throw * Keep orig error. Add status code in http handler * Leave error as-is * Removing accidental code changes. File rename. * Missed a function when moving to a new file * Add missing type imports * Lift .map lambda into named outer function * Adding additional test * Fixing type error Co-authored-by: John Schulz <john.schulz@elastic.co> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

* Adding bulk upgrade api * Addressing comments * Removing todo * Changing body field * Adding helper for getting the bulk install route * Adding request spec * Pulling in Johns changes * Removing test for same package upgraded multiple times * Adding upgrade to setup route * Adding setup integration test * Clean up error handling * Beginning to add tests * Failing jest mock tests * Break up tests & modules for easier testing. Deal with issue described in jestjs/jest#1075 (comment) epm/packages/install has functions a, b, c which are independent but a can also call b and c function a() { b(); c(); } The linked FB issue describes the cause and rationale (Jest works on "module" boundary) but TL;DR: it's easier if you split up your files Some related links I found during this journey * https://medium.com/@qjli/how-to-mock-specific-module-function-in-jest-715e39a391f4 * https://stackoverflow.com/questions/52650367/jestjs-how-to-test-function-being-called-inside-another-function * https://stackoverflow.com/questions/50854440/spying-on-an-imported-function-that-calls-another-function-in-jest/50855968#50855968 * Add test confirming update error result will throw * Keep orig error. Add status code in http handler * Leave error as-is * Removing accidental code changes. File rename. * Missed a function when moving to a new file * Add missing type imports * Lift .map lambda into named outer function * Adding additional test * Fixing type error Co-authored-by: John Schulz <john.schulz@elastic.co> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: John Schulz <john.schulz@elastic.co> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

…a into add-anomalies-to-timeline * 'add-anomalies-to-timeline' of github.com:phillipb/kibana: (89 commits) Aligns several module versions across the repository (elastic#78327) Empty prompt and loading spinner for service map (elastic#78382) Change progress bar to spinner (elastic#78460) [QA][Code Coverage] Coverage teams lookup w/o Additional Config (elastic#77111) Slim down core bundle (elastic#75912) [Alerting] retry internal OCC calls within alertsClient (elastic#77838) [kbn/optimizer] only build xpack examples when building xpack plugins (elastic#78656) [Ingest Manager] Ingest setup upgrade (elastic#78081) [Ingest Manager] Surface saved object client 10,000 limitation to bulk actions UI (elastic#78520) fix name without a category or if field end with .text (elastic#78655) [Security Solution] [Detections] Log message enhancements (elastic#78429) [ML]DF Analytics exploration: default filter of results page by `defaultIsTraining` value in url (elastic#78303) [Enterprise Search] Remove all instances of KibanaContext to Kea store (elastic#78513) [ML] DF Analytics creation: ensure job did not fail to start before showing results link (elastic#78200) fix createAppNavigationHandler to use `navigateToUrl` (elastic#78583) Fixing a11y test failure on discover app (elastic#59975) (elastic#77614) [Security Solution] Initiate endpoint package upgrade from security app (elastic#77498) [kbn/es] use a basic build process (elastic#78090) [kbn/optimizer] fix .json extension handling (elastic#78524) Fix APM lodash imports (elastic#78438) ...

* master: (365 commits) making expression debug info serializable (elastic#78727) fix lodahs imports in app-arch code (elastic#78582) Make Field a React.lazy export (elastic#78483) [Security Solution] Improves detections tests (elastic#77295) [TSVB] Different field format on different series is ignored (elastic#78138) RFC: Improve saved object migrations (elastic#66056) [Security Solution] Fixes url timeline flaky test (elastic#78556) adds retryability feature (elastic#78611) Aligns several module versions across the repository (elastic#78327) Empty prompt and loading spinner for service map (elastic#78382) Change progress bar to spinner (elastic#78460) [QA][Code Coverage] Coverage teams lookup w/o Additional Config (elastic#77111) Slim down core bundle (elastic#75912) [Alerting] retry internal OCC calls within alertsClient (elastic#77838) [kbn/optimizer] only build xpack examples when building xpack plugins (elastic#78656) [Ingest Manager] Ingest setup upgrade (elastic#78081) [Ingest Manager] Surface saved object client 10,000 limitation to bulk actions UI (elastic#78520) fix name without a category or if field end with .text (elastic#78655) [Security Solution] [Detections] Log message enhancements (elastic#78429) [ML]DF Analytics exploration: default filter of results page by `defaultIsTraining` value in url (elastic#78303) ...

jonathan-buttner added 11 commits September 17, 2020 16:43

Adding bulk upgrade api

76df74d

Addressing comments

6dea606

Removing todo

268c526

Resolving conflicts

04a39bd

Changing body field

f111406

Adding helper for getting the bulk install route

2846b76

Adding request spec

20357fd

Pulling in Johns changes

9b29802

Merge branch 'master' of github.com:elastic/kibana into ingest-bulk-u…

3d2a686

…pgrade

Removing test for same package upgraded multiple times

7cb80e9

Adding upgrade to setup route

6e333ab

jonathan-buttner added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.10.0 labels Sep 21, 2020

jonathan-buttner added 2 commits September 22, 2020 12:59

Fixing merge conflicts

99f6aee

Adding setup integration test

8a41396

jonathan-buttner added Feature:EPM Fleet team's Elastic Package Manager (aka Integrations) project Team:Fleet Team label for Observability Data Collection Fleet team labels Sep 22, 2020

jonathan-buttner requested review from jfsiii, neptunian, jen-huang and nchaulet September 22, 2020 19:10

jonathan-buttner marked this pull request as ready for review September 22, 2020 19:10

jonathan-buttner requested a review from a team September 22, 2020 19:10

jonathan-buttner commented Sep 22, 2020

View reviewed changes

jfsiii reviewed Sep 22, 2020

View reviewed changes

jonathan-buttner added 3 commits September 24, 2020 13:14

Clean up error handling

219cfa9

Beginning to add tests

ba00737

Merge branch 'master' of github.com:elastic/kibana into ingest-setup-…

267931b

…upgrade

Failing jest mock tests

037ba98

jonathan-buttner commented Sep 24, 2020

View reviewed changes

John Schulz added 8 commits September 25, 2020 12:55

Add test confirming update error result will throw

5c259d1

Keep orig error. Add status code in http handler

06cfadc

Leave error as-is

a3c77ce

Removing accidental code changes. File rename.

25c3cd0

Missed a function when moving to a new file

202b7a2

Add missing type imports

ee4c774

Lift .map lambda into named outer function

c906aea

jfsiii reviewed Sep 27, 2020

View reviewed changes

jonathan-buttner and others added 3 commits September 28, 2020 09:46

Merge pull request #7 from jfsiii/ingest-setup-upgrade

11b677d

Ingest setup upgrade

Merge branch 'master' of github.com:elastic/kibana into ingest-setup-…

4afbb56

…upgrade

Adding additional test

3f8295d

jfsiii approved these changes Sep 28, 2020

View reviewed changes

Fixing type error

7f02b5f

Merge branch 'master' into ingest-setup-upgrade

0427b9d

jonathan-buttner merged commit e75b36a into elastic:master Sep 28, 2020

jonathan-buttner deleted the ingest-setup-upgrade branch September 28, 2020 20:12

jonathan-buttner mentioned this pull request Sep 28, 2020

[7.x] [Ingest Manager] Ingest setup upgrade (#78081) #78680

Merged

jonathan-buttner mentioned this pull request Oct 1, 2020

[Security Solution][Exceptions] Add lowercase normalizer for case-insensitivity + deprecate _tags field (new OS field) #77379

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Ingest Manager] Ingest setup upgrade #78081

[Ingest Manager] Ingest setup upgrade #78081

jonathan-buttner commented Sep 21, 2020 •

edited

Loading

elasticmachine commented Sep 22, 2020

jonathan-buttner Sep 22, 2020

jfsiii Sep 22, 2020 •

edited

Loading

jonathan-buttner Sep 22, 2020

jfsiii Sep 22, 2020

jonathan-buttner Sep 23, 2020

jonathan-buttner Sep 23, 2020

jonathan-buttner Sep 24, 2020

jfsiii Sep 25, 2020

jfsiii Sep 24, 2020

jfsiii Sep 24, 2020

jfsiii Sep 25, 2020

jfsiii Sep 25, 2020

jfsiii left a comment

jonathan-buttner commented Sep 28, 2020

kibanamachine commented Sep 28, 2020

[Ingest Manager] Ingest setup upgrade #78081

[Ingest Manager] Ingest setup upgrade #78081

Conversation

jonathan-buttner commented Sep 21, 2020 • edited Loading

elasticmachine commented Sep 22, 2020

Choose a reason for hiding this comment

jfsiii Sep 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfsiii left a comment

Choose a reason for hiding this comment

jonathan-buttner commented Sep 28, 2020

kibanamachine commented Sep 28, 2020

💚 Build Succeeded

Metrics [docs]

distributable file count

History

jonathan-buttner commented Sep 21, 2020 •

edited

Loading

jfsiii Sep 22, 2020 •

edited

Loading