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] Adding bulk packages upgrade api #77827

Merged
merged 13 commits into from
Sep 22, 2020
Merged
2 changes: 2 additions & 0 deletions x-pack/plugins/ingest_manager/common/constants/routes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,12 @@ export const FLEET_API_ROOT = `${API_ROOT}/fleet`;
export const LIMITED_CONCURRENCY_ROUTE_TAG = 'ingest:limited-concurrency';

// EPM API routes
const EPM_PACKAGES_BULK = `${EPM_API_ROOT}/packages_bulk`;
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 was hoping to use POST /packages but that is used by the local package upload install API.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@jonathan-buttner jonathan-buttner Sep 18, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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`

Copy link
Contributor Author

@jonathan-buttner jonathan-buttner Sep 18, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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 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.

const EPM_PACKAGES_MANY = `${EPM_API_ROOT}/packages`;
const EPM_PACKAGES_ONE = `${EPM_PACKAGES_MANY}/{pkgkey}`;
const EPM_PACKAGES_FILE = `${EPM_PACKAGES_MANY}/{pkgName}/{pkgVersion}`;
export const EPM_API_ROUTES = {
BULK_UPGRADE_PATTERN: EPM_PACKAGES_BULK,
LIST_PATTERN: EPM_PACKAGES_MANY,
LIMITED_LIST_PATTERN: `${EPM_PACKAGES_MANY}/limited`,
INFO_PATTERN: EPM_PACKAGES_ONE,
Expand Down
20 changes: 20 additions & 0 deletions x-pack/plugins/ingest_manager/common/types/rest_spec/epm.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,26 @@ export interface InstallPackageResponse {
response: AssetReference[];
}

export interface UpgradePackageError {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

name: string;
statusCode: number;
body: {
message: string;
};
}

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[];
}

export interface UpgradePackagesResponse {
response: Array<UpgradePackageInfo | UpgradePackageError>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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[];
}

}

export interface MessageResponse {
response: string;
}
Expand Down
28 changes: 16 additions & 12 deletions x-pack/plugins/ingest_manager/server/errors/handlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,10 +56,7 @@ const getHTTPResponseCode = (error: IngestManagerError): number => {
return 400; // Bad Request
};

export const defaultIngestErrorHandler: IngestErrorHandler = async ({
error,
response,
}: IngestErrorHandlerParams): Promise<IKibanaResponse> => {
export function handleIngestError(error: IngestManagerError | Boom | Error) {
Copy link
Contributor Author

@jonathan-buttner jonathan-buttner Sep 17, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@jfsiii jfsiii Sep 17, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

/**
* 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;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

const logger = appContextService.getLogger();
if (isLegacyESClientError(error)) {
// there was a problem communicating with ES (e.g. via `callCluster`)
Expand All @@ -72,36 +69,43 @@ export const defaultIngestErrorHandler: IngestErrorHandler = async ({

logger.error(message);

return response.customError({
return {
statusCode: error?.statusCode || error.status,
body: { message },
});
};
}

// our "expected" errors
if (error instanceof IngestManagerError) {
// only log the message
logger.error(error.message);
return response.customError({
return {
statusCode: getHTTPResponseCode(error),
body: { message: error.message },
});
};
}

// handle any older Boom-based errors or the few places our app uses them
if (isBoom(error)) {
// only log the message
logger.error(error.output.payload.message);
return response.customError({
return {
statusCode: error.output.statusCode,
body: { message: error.output.payload.message },
});
};
}

// not sure what type of error this is. log as much as possible
logger.error(error);
return response.customError({
return {
statusCode: 500,
body: { message: error.message },
});
};
}

export const defaultIngestErrorHandler: IngestErrorHandler = async ({
jonathan-buttner marked this conversation as resolved.
Show resolved Hide resolved
error,
response,
}: IngestErrorHandlerParams): Promise<IKibanaResponse> => {
return response.customError(handleIngestError(error));
};
59 changes: 30 additions & 29 deletions x-pack/plugins/ingest_manager/server/routes/epm/handlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
*/
import { TypeOf } from '@kbn/config-schema';
import { RequestHandler, CustomHttpResponseOptions } from 'src/core/server';
import { appContextService } from '../../services';
import {
GetInfoResponse,
InstallPackageResponse,
Expand All @@ -14,6 +13,7 @@ import {
GetCategoriesResponse,
GetPackagesResponse,
GetLimitedPackagesResponse,
UpgradePackagesResponse,
} from '../../../common';
import {
GetCategoriesRequestSchema,
Expand All @@ -23,6 +23,7 @@ import {
InstallPackageFromRegistryRequestSchema,
InstallPackageByUploadRequestSchema,
DeletePackageRequestSchema,
BulkUpgradePackagesFromRegistryRequestSchema,
} from '../../types';
import {
getCategories,
Expand All @@ -34,9 +35,9 @@ import {
getLimitedPackages,
getInstallationObject,
} from '../../services/epm/packages';
import { IngestManagerError, defaultIngestErrorHandler } from '../../errors';
import { defaultIngestErrorHandler } from '../../errors';
import { splitPkgKey } from '../../services/epm/registry';
import { getInstallType } from '../../services/epm/packages/install';
import { handleInstallPackageFailure, upgradePackages } from '../../services/epm/packages/install';

export const getCategoriesHandler: RequestHandler<
undefined,
Expand Down Expand Up @@ -136,13 +137,11 @@ export const installPackageFromRegistryHandler: RequestHandler<
undefined,
TypeOf<typeof InstallPackageFromRegistryRequestSchema.body>
> = async (context, request, response) => {
const logger = appContextService.getLogger();
const savedObjectsClient = context.core.savedObjects.client;
const callCluster = context.core.elasticsearch.legacy.client.callAsCurrentUser;
const { pkgkey } = request.params;
const { pkgName, pkgVersion } = splitPkgKey(pkgkey);
const installedPkg = await getInstallationObject({ savedObjectsClient, pkgName });
const installType = getInstallType({ pkgVersion, installedPkg });
neptunian marked this conversation as resolved.
Show resolved Hide resolved
try {
const res = await installPackage({
savedObjectsClient,
Expand All @@ -155,36 +154,38 @@ export const installPackageFromRegistryHandler: RequestHandler<
};
return response.ok({ body });
} catch (e) {
// 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) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

return defaultResult;
}
await handleInstallPackageFailure(
savedObjectsClient,
e,
pkgName,
pkgVersion,
installedPkg,
callCluster
);

// if there is an unknown server error, uninstall any package assets or reinstall the previous version if update
try {
if (installType === 'install' || installType === 'reinstall') {
logger.error(`uninstalling ${pkgkey} after error installing`);
await removeInstallation({ savedObjectsClient, pkgkey, callCluster });
}
if (installType === 'update') {
// @ts-ignore getInstallType ensures we have installedPkg
const prevVersion = `${pkgName}-${installedPkg.attributes.version}`;
logger.error(`rolling back to ${prevVersion} after error installing ${pkgkey}`);
await installPackage({
savedObjectsClient,
pkgkey: prevVersion,
callCluster,
});
}
} catch (error) {
logger.error(`failed to uninstall or rollback package after installation error ${error}`);
}
return defaultResult;
}
};

export const upgradePackagesFromRegistryHandler: RequestHandler<
undefined,
undefined,
TypeOf<typeof BulkUpgradePackagesFromRegistryRequestSchema.body>
> = async (context, request, response) => {
const savedObjectsClient = context.core.savedObjects.client;
const callCluster = context.core.elasticsearch.legacy.client.callAsCurrentUser;
const res = await upgradePackages({
savedObjectsClient,
callCluster,
packagesToUpgrade: request.body.upgrade,
});
const body: UpgradePackagesResponse = {
response: res,
};
return response.ok({ body });
};

export const installPackageByUploadHandler: RequestHandler<
undefined,
undefined,
Expand Down
11 changes: 11 additions & 0 deletions x-pack/plugins/ingest_manager/server/routes/epm/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {
installPackageFromRegistryHandler,
installPackageByUploadHandler,
deletePackageHandler,
upgradePackagesFromRegistryHandler,
} from './handlers';
import {
GetCategoriesRequestSchema,
Expand All @@ -23,6 +24,7 @@ import {
InstallPackageFromRegistryRequestSchema,
InstallPackageByUploadRequestSchema,
DeletePackageRequestSchema,
BulkUpgradePackagesFromRegistryRequestSchema,
} from '../../types';

const MAX_FILE_SIZE_BYTES = 104857600; // 100MB
Expand Down Expand Up @@ -82,6 +84,15 @@ export const registerRoutes = (router: IRouter) => {
installPackageFromRegistryHandler
);

router.post(
{
path: EPM_API_ROUTES.BULK_UPGRADE_PATTERN,
validate: BulkUpgradePackagesFromRegistryRequestSchema,
options: { tags: [`access:${PLUGIN_ID}-all`] },
},
upgradePackagesFromRegistryHandler
);

router.post(
{
path: EPM_API_ROUTES.INSTALL_BY_UPLOAD_PATTERN,
Expand Down
119 changes: 117 additions & 2 deletions x-pack/plugins/ingest_manager/server/services/epm/packages/install.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@

import { SavedObject, SavedObjectsClientContract } from 'src/core/server';
import semver from 'semver';
import Boom from 'boom';
import { UpgradePackageError, UpgradePackageInfo } from '../../../../common';
import { PACKAGES_SAVED_OBJECT_TYPE, MAX_TIME_COMPLETE_INSTALL } from '../../../constants';
import {
AssetReference,
Expand All @@ -17,6 +19,7 @@ import {
EsAssetReference,
ElasticsearchAssetType,
InstallType,
RegistrySearchResult,
} from '../../../types';
import { installIndexPatterns } from '../kibana/index_pattern/install';
import * as Registry from '../registry';
Expand All @@ -32,11 +35,12 @@ import {
ArchiveAsset,
} from '../kibana/assets/install';
import { updateCurrentWriteIndices } from '../elasticsearch/template/template';
import { deleteKibanaSavedObjectsAssets } from './remove';
import { PackageOutdatedError } from '../../../errors';
import { deleteKibanaSavedObjectsAssets, removeInstallation } from './remove';
import { IngestManagerError, PackageOutdatedError } from '../../../errors';
import { getPackageSavedObjects } from './get';
import { installTransformForDataset } from '../elasticsearch/transform/install';
import { appContextService } from '../../app_context';
import { handleIngestError } from '../../../errors/handlers';

export async function installLatestPackage(options: {
savedObjectsClient: SavedObjectsClientContract;
Expand Down Expand Up @@ -95,6 +99,117 @@ export async function ensureInstalledPackage(options: {
return installation;
}

export async function handleInstallPackageFailure(
jonathan-buttner marked this conversation as resolved.
Show resolved Hide resolved
savedObjectsClient: SavedObjectsClientContract,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

error: IngestManagerError | Boom | Error,
pkgName: string,
pkgVersion: string,
installedPkg: SavedObject<Installation> | undefined,
callCluster: CallESAsCurrentUser
) {
if (error instanceof IngestManagerError) {
return;
}
const logger = appContextService.getLogger();
const pkgkey = Registry.pkgToPkgKey({
name: pkgName,
version: pkgVersion,
});

// if there is an unknown server error, uninstall any package assets or reinstall the previous version if update
try {
const installType = getInstallType({ pkgVersion, installedPkg });
if (installType === 'install' || installType === 'reinstall') {
logger.error(`uninstalling ${pkgkey} after error installing`);
await removeInstallation({ savedObjectsClient, pkgkey, callCluster });
}

if (installType === 'update') {
if (!installedPkg) {
logger.error(
`failed to rollback package after installation error ${error} because saved object was undefined`
);
return;
}
const prevVersion = `${pkgName}-${installedPkg.attributes.version}`;
logger.error(`rolling back to ${prevVersion} after error installing ${pkgkey}`);
await installPackage({
savedObjectsClient,
pkgkey: prevVersion,
callCluster,
});
}
} catch (e) {
logger.error(`failed to uninstall or rollback package after installation error ${e}`);
}
}

export async function upgradePackages({
savedObjectsClient,
packagesToUpgrade,
callCluster,
}: {
savedObjectsClient: SavedObjectsClientContract;
packagesToUpgrade: string[];
callCluster: CallESAsCurrentUser;
}): Promise<Array<UpgradePackageInfo | UpgradePackageError>> {
const res: Array<UpgradePackageInfo | UpgradePackageError> = [];

for (const pkgToUpgrade of packagesToUpgrade) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks John! I'll pull in your changes.

let installedPkg: SavedObject<Installation> | undefined;
let latestPackage: RegistrySearchResult | undefined;
try {
[installedPkg, latestPackage] = await Promise.all([
getInstallationObject({ savedObjectsClient, pkgName: pkgToUpgrade }),
Registry.fetchFindLatestPackage(pkgToUpgrade),
]);
} catch (e) {
res.push({ name: pkgToUpgrade, ...handleIngestError(e) });
continue;
}

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, version is the currently installed version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

const pkgkey = Registry.pkgToPkgKey({
name: latestPackage.name,
version: latestPackage.version,
});

try {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment re: Promise.allSettled

const assets = await installPackage({ savedObjectsClient, pkgkey, callCluster });
res.push({
name: pkgToUpgrade,
newVersion: latestPackage.version,
oldVersion: installedPkg?.attributes.version ?? null,
assets,
});
} catch (e) {
res.push({ name: pkgToUpgrade, ...handleIngestError(e) });
await handleInstallPackageFailure(
savedObjectsClient,
e,
latestPackage.name,
latestPackage.version,
installedPkg,
callCluster
);
}
} else {
// package was already at the latest version
res.push({
name: pkgToUpgrade,
newVersion: latestPackage.version,
oldVersion: latestPackage.version,
assets: [
...installedPkg.attributes.installed_es,
...installedPkg.attributes.installed_kibana,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

],
});
}
}
return res;
}

export async function installPackage({
savedObjectsClient,
pkgkey,
Expand Down
Loading