[Ingest Manager] Adding bulk packages upgrade api by jonathan-buttner · Pull Request #77827 · elastic/kibana

jonathan-buttner · 2020-09-17T20:55:32Z

This PR adds a new api POST /packages_bulk to handle bulk upgrading of packages. The api enforces that the latest version must be greater than the current version. It returns an array of entries corresponding to the requested package upgrades. An entry contains the information returned from the regular install function, the package name, and the old and new versions.

More info on the design is here: #77779

I refactored a couple functions that are shared between the bulk upgrade and the install package apis.

Once this is merged I'll have the Security Solution use this api to upgrade the endpoint package by refactoring the code here: #77498

And add similar functionality in the Ingest Manager's UI to upgrade the default packages when a user is navigating between the pages.

elasticmachine · 2020-09-17T20:55:35Z

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

elasticmachine · 2020-09-17T20:55:35Z

Pinging @elastic/ingest-management (Team:Ingest Management)

jonathan-buttner · 2020-09-17T20:56:15Z

x-pack/plugins/ingest_manager/common/constants/routes.ts

 export const LIMITED_CONCURRENCY_ROUTE_TAG = 'ingest:limited-concurrency';

 // EPM API routes
+const EPM_PACKAGES_BULK = `${EPM_API_ROOT}/packages_bulk`;


I was hoping to use POST /packages but that is used by the local package upload install API.

packages/_bulk like ES also occurred to me but I haven't looked into it.

I just put up a bulk actions PR #77690 where I added /bulk_reassign and /bulk_unenroll routes. does it make sense to agree on a convention here?

I was gonna ask the same thing. Seeing yours makes me say keep bulk_*.

Thanks for the feedback so are we thinking /bulk_packages then? Or should I go with /packages/_bulk if that works?

imo this points to the need to add a verb to this route. is my understanding correct that this route is only for upgrading existing packages, rather than being able to bulk install packages? if so, I think it makes sense to differentiate this from the install routes (/packages). what do you all think of using /packages/bulk_upgrade?:

BULK_UPGRADE_PATTERN: `${EPM_PACKAGES_MANY}/bulk_upgrade`

@jen-huang It can do both upgrades and installs. If the package hasn't already been installed, it'll just install the latest one available.

in that case I'm happy to leave the route as /packages_bulk, or simply switching to bulk_packages to follow more closely with the bulk actions API convention. although /_bulk is an ES convention, I've never felt that Kibana APIs need to follow ES patterns

@jen-huang @neptunian @jfsiii I'm happy to switch it to bulk_packages unless someone objects?

I'm not too fussed but I don't think bulk_packages really follows the convention because those are using bulk_(verb) like bulk_upgrade. Since we don't have the verb, it seems redundant to say packages/bulk_packages.

jonathan-buttner · 2020-09-17T20:56:58Z

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

-  error,
-  response,
-}: IngestErrorHandlerParams): Promise<IKibanaResponse> => {
+export function handleIngestError(error: IngestManagerError | Boom | Error) {


Extracted this out to its own function so the upgrade api can use it to create an array of errors if an error occurs while upgrading a package.

I like the separate function. The only reason it doesn't exist yet is I couldn't come up with a name for the interface it returns.

We can keep thinking and update later.

To clarify, I'm OK with this as submitted. We can change now or later. Some things I was thinking about:

RequestHandler's and IngestErrorHandlers both end in a IKibanaResponse

This function (or this type of function) maps an error to a status code (number) or an interface that contains a status code an maybe some other info.

I've yet to figure out what to call it, and was leaning towards a shape like you have here but delayed worrying about it until it came up again.

Oh, maybe we could use CustomHttpResponseOptions or Pick<CustomHttpResponseOptions, 'body' | 'statusCode'>

kibana/src/core/server/http/router/response.ts

Lines 83 to 93 in feceb0f

/**

* HTTP response parameters for a response with adjustable status code.

* @public

*/

export interface CustomHttpResponseOptions<T extends HttpResponsePayload | ResponseError> {

/** HTTP message to send to the client */

body?: T;

/** HTTP Headers with additional information about response */

headers?: ResponseHeaders;

statusCode: number;

}

Ah ok, so just to make sure I'm understanding correctly, changing the function signature to this:

export function handleIngestError(error: IngestManagerError | Boom | Error): Pick<CustomHttpResponseOptions, 'body' | 'statusCode'> {

Is what you're suggesting?

jonathan-buttner · 2020-09-17T20:57:54Z

x-pack/plugins/ingest_manager/server/routes/epm/handlers.ts

-    // could have also done `return defaultIngestErrorHandler({ error: e, response })` at each of the returns,
-    // but doing it this way will log the outer/install errors before any inner/rollback errors
    const defaultResult = await defaultIngestErrorHandler({ error: e, response });
-    if (e instanceof IngestManagerError) {


Extracted this into a function that is shared between the installPackageFromRegistryHandler handler and the bulk upgrade handler.

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

jonathan-buttner · 2020-09-17T20:59:51Z

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

+    }
+
+    // TODO I think we want version here and not `install_version`?
+    if (!installedPkg || semver.gt(latestPackage.version, installedPkg.attributes.version)) {


Thoughts on wether this is the right version I'm using here?

yes, version is the currently installed version

jonathan-buttner · 2020-09-17T21:00:33Z

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

+        oldVersion: latestPackage.version,
+        assets: [
+          ...installedPkg.attributes.installed_es,
+          ...installedPkg.attributes.installed_kibana,


In the case where no upgrade actually occurs because it was already at the latest version, should we return the current assets? I think this is all the installed assets?

Good question. Currently we reinstall the package if its already installed, but this functionality would not, so it would be a bit inconsistent, though I'm not sure we should be reinstalling anyway. I think for now its fine to just return the assets.

jfsiii

Thanks so much for this PR. I left some comments and would like to make some changes, but the important bits look good and the tests will make it easier.

I'll follow up tomorrow

jfsiii · 2020-09-17T21:09:42Z

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

 }

+export async function handleInstallPackageFailure(
+  savedObjectsClient: SavedObjectsClientContract,


Please convert this to an object. 6 parameters is about 2x my max.

jfsiii · 2020-09-17T21:11:59Z

x-pack/plugins/ingest_manager/common/constants/routes.ts

 export const LIMITED_CONCURRENCY_ROUTE_TAG = 'ingest:limited-concurrency';

 // EPM API routes
+const EPM_PACKAGES_BULK = `${EPM_API_ROOT}/packages_bulk`;


packages/_bulk like ES also occurred to me but I haven't looked into it.

jfsiii · 2020-09-17T21:16:31Z

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

-  error,
-  response,
-}: IngestErrorHandlerParams): Promise<IKibanaResponse> => {
+export function handleIngestError(error: IngestManagerError | Boom | Error) {


I like the separate function. The only reason it doesn't exist yet is I couldn't come up with a name for the interface it returns.

We can keep thinking and update later.

jfsiii · 2020-09-17T21:18:55Z

x-pack/plugins/ingest_manager/common/types/rest_spec/epm.ts

  response: AssetReference[];
 }

+export interface UpgradePackageError {


This name reads like it's an actual Error. Mind prefixing with an I for IUpgradePackageError or adding something to the end?

jfsiii · 2020-09-17T21:28:50Z

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

+}): Promise<Array<UpgradePackageInfo | UpgradePackageError>> {
+  const res: Array<UpgradePackageInfo | UpgradePackageError> = [];
+
+  for (const pkgToUpgrade of packagesToUpgrade) {


Since we don't care want to stop on the first failure, I think we can use Promise.allSettled https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/allSettled and loop the results and use the status property to test success/failure

John and I talked about this and I think we're going to leave it as is for now because the code I came up with using allSettled wasn't super readable.

After we spoke, I took a pass at it #77974

As I said on the call, I'm happy to ship existing version but that PR is essentially what I was describing

Thanks John! I'll pull in your changes.

jfsiii · 2020-09-17T21:30:17Z

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

+        version: latestPackage.version,
+      });
+
+      try {


Same comment re: Promise.allSettled

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

jfsiii · 2020-09-18T00:22:39Z

x-pack/plugins/ingest_manager/common/types/rest_spec/epm.ts

+}
+
+export interface UpgradePackagesResponse {
+  response: Array<UpgradePackageInfo | UpgradePackageError>;


We have similar patterns elsewhere. Take a look at https://github.com/elastic/kibana/pull/77690/files#diff-f1874bde49bc5bca72d42584b63a5cf3R143-R147 & https://github.com/elastic/kibana/blob/71b9dedfc4eadd97db2b6b0dc038d0f640babf7b/x-pack/plugins/ingest_manager/common/types/rest_spec/package_policy.ts#L53-L57

I like error: Error if it's possible since it retains important info and can easily be handled/converted by others.

Do you think it makes more sense to have the response of the bulk endpoint be something like:

export interface UpgradePackageInfo { name: string; newVersion?: string; // this will be null if no package was present before the upgrade (aka it was an install) oldVersion?: string | null; assets?: AssetReference[]; statusCode: number; error?: Error; }

If there's an error then, newVersion, oldVersion, assets will be undefined.

export interface UpgradePackagesResponse { response: UpgradePackageInfo[]; }

neptunian · 2020-09-18T13:50:59Z

It looks like this change will install the latest package if it doesn't exist. Which makes it similar to our current route /packages/{pkg} for installing/upgrading a single package. But we don't call it upgrade, we just call it install. I was thinking we should follow similar naming for bulk install/updating.

jonathan-buttner · 2020-09-18T13:52:14Z

It looks like this change will install the latest package if it doesn't exist. Which makes it similar to our current route /packages/{pkg} for installing/upgrading a single package. But we don't call it upgrade, we just call it install. I was thinking we should follow similar naming for bulk install/updating.

Yeah good point, I can switch everything over to "install" instead of upgrade.

neptunian · 2020-09-18T13:55:20Z

There was an issue to change from POST to PUT to make the behavior of the route more clear but not sure we'll get around to that or how necessary it is: #62479

neptunian · 2020-09-18T20:37:33Z

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

+        expect(entry.oldVersion).equal('0.1.0');
+        expect(entry.newVersion).equal('0.3.0');
+      });
+      it('should the same package multiple times for upgrade', async function () {


description missing a word I think

Thanks I'll update.

x-pack/plugins/ingest_manager/server/routes/epm/handlers.ts

jfsiii · 2020-09-19T21:14:20Z

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

+        const { body }: { body: BulkInstallPackagesResponse } = await supertest
+          .post(`/api/ingest_manager/epm/packages/_bulk`)
+          .set('kbn-xsrf', 'xxxx')
+          .send({ packages: ['multiple_versions', 'multiple_versions'] })


Is there a user story/case for this? The test confirms it's a no-op. Why would a user submit the same name multiple times?

No, there's no user story. There's no real reason we need to support it. I was just trying to guard against the case where it happened accidentally. I can remove the test if you think it's confusing.

I removed the test.

jfsiii · 2020-09-19T21:42:08Z

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

+}): Promise<Array<UpgradePackageInfo | UpgradePackageError>> {
+  const res: Array<UpgradePackageInfo | UpgradePackageError> = [];
+
+  for (const pkgToUpgrade of packagesToUpgrade) {


After we spoke, I took a pass at it #77974

As I said on the call, I'm happy to ship existing version but that PR is essentially what I was describing

…pgrade

kibanamachine · 2020-09-22T16:37:18Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: df32308

Build metrics

page load bundle size

id	value	diff	baseline
ingestManager	514.5KB	+192.0B	514.3KB

History

💔 Build #76198 failed 6ef14cc
💚 Build #75880 succeeded 7cb80e9
💚 Build #75700 succeeded 20357fd
💔 Build #75677 failed 6dea606
💔 Build #75376 failed 76df74d

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 * Pulling in John's error handling changes * Fixing type error

Adding bulk upgrade api

76df74d

jonathan-buttner added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:EPM Fleet team's Elastic Package Manager (aka Integrations) project v7.10.0 Team:Fleet Team label for Observability Data Collection Fleet team labels Sep 17, 2020

jonathan-buttner requested review from a team, jfsiii and neptunian September 17, 2020 20:55

jonathan-buttner commented Sep 17, 2020

View reviewed changes

jonathan-buttner mentioned this pull request Sep 17, 2020

[Ingest Manager] Upgrade Package APIs #77779

Closed

jfsiii reviewed Sep 17, 2020

View reviewed changes

jfsiii reviewed Sep 18, 2020

View reviewed changes

jonathan-buttner added 6 commits September 18, 2020 14:33

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

jonathan-buttner mentioned this pull request Sep 18, 2020

[Security Solution] Initiate endpoint package upgrade from security app #77498

Merged

neptunian reviewed Sep 18, 2020

View reviewed changes

x-pack/plugins/ingest_manager/server/routes/epm/handlers.ts Show resolved Hide resolved

jfsiii mentioned this pull request Sep 19, 2020

[Ingest Manager] Bulk upgrade API #77974

Closed

jfsiii reviewed Sep 19, 2020

View reviewed changes

Pulling in Johns changes

9b29802

jonathan-buttner added 2 commits September 21, 2020 10:33

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

3d2a686

…pgrade

Removing test for same package upgraded multiple times

7cb80e9

jonathan-buttner mentioned this pull request Sep 21, 2020

[Ingest Manager] Ingest setup upgrade #78081

Merged

neptunian approved these changes Sep 22, 2020

View reviewed changes

jonathan-buttner added 3 commits September 22, 2020 09:58

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

ef51825

…pgrade

Pulling in John's error handling changes

6ef14cc

Fixing type error

df32308

jonathan-buttner merged commit 311805a into elastic:master Sep 22, 2020

jonathan-buttner deleted the ingest-bulk-upgrade branch September 22, 2020 16:50

jonathan-buttner mentioned this pull request Sep 22, 2020

[7.x] [Ingest Manager] Adding bulk packages upgrade api (#77827) #78180

Merged

	/**
	* HTTP response parameters for a response with adjustable status code.
	* @public
	*/
	export interface CustomHttpResponseOptions<T extends HttpResponsePayload \| ResponseError> {
	/** HTTP message to send to the client */
	body?: T;
	/** HTTP Headers with additional information about response */
	headers?: ResponseHeaders;
	statusCode: number;
	}

Conversation

jonathan-buttner commented Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Sep 17, 2020

Uh oh!

elasticmachine commented Sep 17, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonathan-buttner Sep 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonathan-buttner Sep 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonathan-buttner Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jfsiii Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jfsiii left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonathan-buttner commented Sep 17, 2020 •

edited

Loading

jonathan-buttner Sep 18, 2020 •

edited

Loading

jonathan-buttner Sep 18, 2020 •

edited

Loading

jonathan-buttner Sep 17, 2020 •

edited

Loading

jfsiii Sep 17, 2020 •

edited

Loading