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

[EPM] Make API consistent for package installation and removal #48745

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
410465e
Adjust type names
skh Oct 21, 2019
8dbd67e
Install package in one go.
skh Oct 21, 2019
8809b55
Integration -> Package
skh Oct 21, 2019
92848d8
Really install all known assets
skh Oct 21, 2019
fc737b1
Remove ingest pipelines on package deletion.
skh Oct 21, 2019
51eddcb
[EPM] Replace image paths/handlers with generic ones for file (#48688)
Oct 21, 2019
7d9389b
Merge remote-tracking branch 'upstream/master' into feature-integrati…
Oct 23, 2019
3842acb
Merge branch 'feature-integrations-manager' into 47742-make-api-consi…
skh Oct 24, 2019
5a218a5
Fix merge error.
skh Oct 24, 2019
cbb8328
Type tuning
skh Oct 24, 2019
de72683
Adjust types
skh Oct 30, 2019
cb48028
Remove asset types from routes altogether
skh Oct 30, 2019
9c97d46
Merge remote-tracking branch 'upstream/feature-integrations-manager' …
skh Oct 30, 2019
2df30c5
Merge branch 'feature-integrations-manager' into 47742-make-api-consi…
skh Oct 30, 2019
b6c8d94
Add timelion-sheet back to types.
skh Oct 31, 2019
dcb32b3
Respond with full package info to install/delete
skh Oct 31, 2019
dd1ce82
Be specific in return type.
skh Oct 31, 2019
d25d942
Merge remote-tracking branch 'upstream/feature-integrations-manager' …
skh Nov 4, 2019
5a1b73c
Keep installAssets() as separate step
skh Nov 4, 2019
bfe6a21
Merge branch 'feature-integrations-manager' into 47742-make-api-consi…
skh Nov 4, 2019
d365f20
Merge remote-tracking branch 'upstream/feature-integrations-manager' …
skh Nov 12, 2019
efef740
Merge branch 'feature-integrations-manager' into 47742-make-api-consi…
skh Nov 12, 2019
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
19 changes: 7 additions & 12 deletions x-pack/legacy/plugins/integrations_manager/common/routes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,13 @@
* you may not use this file except in compliance with the Elastic License.
*/
import { PLUGIN } from './constants';
import { AssetType, CategoryId } from './types';
import { CategoryId } from './types';

export const API_ROOT = `/api/${PLUGIN.ID}`;
export const API_LIST_PATTERN = `${API_ROOT}/list`;
export const API_INFO_PATTERN = `${API_ROOT}/package/{pkgkey}`;
export const API_INSTALL_PATTERN = `${API_ROOT}/install/{pkgkey}/{asset?}`;
export const API_DELETE_PATTERN = `${API_ROOT}/delete/{pkgkey}/{asset?}`;
export const API_INSTALL_PATTERN = `${API_ROOT}/install/{pkgkey}`;
export const API_DELETE_PATTERN = `${API_ROOT}/delete/{pkgkey}`;
export const API_CATEGORIES_PATTERN = `${API_ROOT}/categories`;

export interface ListParams {
Expand All @@ -32,15 +32,10 @@ export function getInfoPath(pkgkey: string) {
export function getFilePath(filePath: string) {
return `${API_ROOT}${filePath}`;
}

export function getInstallPath(pkgkey: string, asset?: AssetType) {
return API_INSTALL_PATTERN.replace('{pkgkey}', pkgkey)
.replace('{asset?}', asset || '')
.replace(/\/$/, ''); // trim trailing slash
export function getInstallPath(pkgkey: string) {
return API_INSTALL_PATTERN.replace('{pkgkey}', pkgkey).replace(/\/$/, ''); // trim trailing slash
}

export function getRemovePath(pkgkey: string, asset?: AssetType) {
return API_DELETE_PATTERN.replace('{pkgkey}', pkgkey)
.replace('{asset?}', asset || '')
.replace(/\/$/, ''); // trim trailing slash
export function getRemovePath(pkgkey: string) {
return API_DELETE_PATTERN.replace('{pkgkey}', pkgkey).replace(/\/$/, ''); // trim trailing slash
}
17 changes: 9 additions & 8 deletions x-pack/legacy/plugins/integrations_manager/common/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,15 @@ export { Request, ResponseToolkit, Server, ServerRoute } from 'hapi';

export type InstallationStatus = Installed['status'] | NotInstalled['status'];

export type AssetType =
| 'config'
| 'dashboard'
| 'index-pattern'
| 'ingest-pipeline'
| 'search'
| 'timelion-sheet'
| 'visualization';
export enum AssetType {
Copy link
Contributor

Choose a reason for hiding this comment

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

I hope this works. It's what I wanted to do initially, but had some challenges. I think I've both fixed the things that were getting in the way and have a better hand on TS but I'll run locally to confirm. 🤞

Copy link
Contributor

Choose a reason for hiding this comment

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

Ran locally and things still work as expected. I was struggling with a way to have these strings as both a union type & some way to do "lookup" for the "does type x use saved objects?" question. I ended up using a Set for SAVED_OBJECT_TYPES and we can create that from a few different structures (this PR shows that) so seems ok.

Can you talk about why you changed it to an enum? Is it solely to be able to use code like AssetType.config (and have one linked property name vs N duplicated strings) or are there other benefits?

FWIW, I think I'm ok changing to this. I'm just documenting what I can remember about why I went one way, and I'd like to know more about the benefits and trade-offs. I'm still new to TS & enum so I don't have practical experience with any benefits/drawbacks.

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 wanted to be able to iterate over them, even if in the end I didn't need to. Also, for me it feels like the correct type -- we use them in the metrics UI for things like node types so I got used to using them there.

config = 'config',
dashboard = 'dashboard',
visualization = 'visualization',
search = 'search',
ingestPipeline = 'ingest-pipeline',
indexPattern = 'index-pattern',
timelionSheet = 'timelion-sheet',
Copy link
Member

Choose a reason for hiding this comment

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

Not for this PR but more general thought on this: The reason I prefixed the asset always by the service is to prevent conflicts in the future and make them unique. So if there is an index-pattern in Kibana and Elasticsearch, both is possible. Because of this I think we should start to refer to either elasticsearch/ingest-pipeline or have an asset type as a combination of the service and type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. At the moment some of these double as Kibana saved-object types (as defined here: https://www.elastic.co/guide/en/kibana/master/saved-objects-api-get.html) so we'll probably want to uncouple our asset types from those and only map between them.

}

// Registry's response types
// from /search
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,24 +27,19 @@ interface ListPackagesRequest extends Request {
query: Request['query'] & SearchParams;
}

interface PackageRequest extends Request {
interface PackageInfoRequest extends Request {
params: {
pkgkey: string;
};
}

interface InstallAssetRequest extends Request {
params: AssetRequestParams;
}

interface DeleteAssetRequest extends Request {
params: AssetRequestParams;
interface InstallDeletePackageRequest extends Request {
Copy link
Contributor

Choose a reason for hiding this comment

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

I liked that these were separate and am 50/50 on the name but we're not exporting and we can always split them up later.

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, the important change here is Asset -> Package. I can split them, but I couldn't think (for now) of a situation where they would be different.

params: {
pkgkey: string;
asset: AssetType;
};
}

type AssetRequestParams = PackageRequest['params'] & {
asset?: AssetType;
};

export async function handleGetCategories(req: Request, extra: Extra) {
return getCategories();
}
Expand All @@ -59,7 +54,7 @@ export async function handleGetList(req: ListPackagesRequest, extra: Extra) {
return packageList;
}

export async function handleGetInfo(req: PackageRequest, extra: Extra) {
export async function handleGetInfo(req: PackageInfoRequest, extra: Extra) {
const { pkgkey } = req.params;
const savedObjectsClient = getClient(req);
const packageInfo = await getPackageInfo({ savedObjectsClient, pkgkey });
Expand All @@ -81,26 +76,20 @@ export const handleGetFile = async (req: Request, extra: Extra) => {
return epmResponse;
};

export async function handleRequestInstall(req: InstallAssetRequest, extra: Extra) {
const { pkgkey, asset } = req.params;
if (!asset) throw new Error('Unhandled empty/default asset case');

export async function handleRequestInstall(req: InstallDeletePackageRequest, extra: Extra) {
const { pkgkey } = req.params;
const savedObjectsClient = getClient(req);
const callCluster = getClusterAccessor(extra.context.esClient, req);
const object = await installPackage({
return await installPackage({
savedObjectsClient,
pkgkey,
asset,
callCluster,
});

return object;
}

export async function handleRequestDelete(req: DeleteAssetRequest, extra: Extra) {
export async function handleRequestDelete(req: InstallDeletePackageRequest, extra: Extra) {
const { pkgkey } = req.params;
const savedObjectsClient = getClient(req);
const deleted = await removeInstallation({ savedObjectsClient, pkgkey });
const callCluster = getClusterAccessor(extra.context.esClient, req);
const deleted = await removeInstallation({ savedObjectsClient, pkgkey, callCluster });

return deleted;
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,12 @@ export * from './handlers';
export type CallESAsCurrentUser = ScopedClusterClient['callAsCurrentUser'];

export const SAVED_OBJECT_TYPES = new Set<AssetType>([
'config',
'dashboard',
'index-pattern',
'search',
'timelion-sheet',
'visualization',
AssetType.config,
AssetType.dashboard,
AssetType.indexPattern,
AssetType.search,
AssetType.timelionSheet,
AssetType.visualization,
]);

export function getClusterAccessor(esClient: IClusterClient, req: Request) {
Expand All @@ -40,6 +40,6 @@ export function createInstallableFrom<T>(from: T, savedObject?: Installation): I
};
}

export function assetUsesObjects(asset: AssetType) {
return SAVED_OBJECT_TYPES.has(asset);
export function assetUsesObjects(assetType: AssetType) {
return SAVED_OBJECT_TYPES.has(assetType);
}
101 changes: 34 additions & 67 deletions x-pack/legacy/plugins/integrations_manager/server/packages/install.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,35 +8,30 @@ import { SavedObject, SavedObjectsClientContract } from 'src/core/server/';
import { SAVED_OBJECT_TYPE } from '../../common/constants';
import { AssetReference, AssetType, InstallationAttributes } from '../../common/types';
import * as Registry from '../registry';
import { CallESAsCurrentUser, assetUsesObjects, getInstallationObject } from './index';
import { getInstallationObject, getPackageInfo } from './index';
import { getObjects } from './get_objects';

export async function installPackage(options: {
savedObjectsClient: SavedObjectsClientContract;
pkgkey: string;
asset: AssetType;
callCluster: CallESAsCurrentUser;
}) {
const { savedObjectsClient, pkgkey, asset, callCluster } = options;
// install any assets (in ES, as Saved Objects, etc) as required. Get references to them
const { savedObjectsClient, pkgkey } = options;

const toSave = await installAssets({
savedObjectsClient,
pkgkey,
asset,
callCluster,
});

if (toSave.length) {
// saved those references in the package manager's state object
const saved = await saveInstallationReferences({
// Save those references in the integration manager's state saved object
await saveInstallationReferences({
savedObjectsClient,
pkgkey,
toSave,
});
return saved;
}

return [];
return getPackageInfo({ savedObjectsClient, pkgkey });
}

// the function which how to install each of the various asset types
Expand All @@ -45,20 +40,20 @@ export async function installPackage(options: {
export async function installAssets(options: {
savedObjectsClient: SavedObjectsClientContract;
pkgkey: string;
asset: AssetType;
callCluster: CallESAsCurrentUser;
}) {
const { savedObjectsClient, pkgkey, asset, callCluster } = options;
if (assetUsesObjects(asset)) {
const references = await installObjects({ savedObjectsClient, pkgkey, asset });
return references;
}
if (asset === 'ingest-pipeline') {
const references = await installPipelines({ callCluster, pkgkey });
return references;
}
const { savedObjectsClient, pkgkey } = options;

// Only install certain Kibana assets during package installation.
// All other asset types need special handling
const typesToInstall = [AssetType.visualization, AssetType.dashboard, AssetType.search];
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these 3 different than the other SAVED_OBJECT_TYPES? Could this be

Suggested change
const typesToInstall = [AssetType.visualization, AssetType.dashboard, AssetType.search];
const typesToInstall = Array.from(SAVED_OBJECT_TYPES);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, they are different. I only included those Kibana assets here of which I'm more or less sure, for now, that we can just copy them over.

Index patterns will need to be generated from fields.yml (we have tasks for that), and the other Kibana asset types are not listed in https://github.com/elastic/package-registry/blob/master/ASSETS.md so it's not specified what to do with them.


const installationPromises = typesToInstall.map(async assetType =>
installKibanaSavedObjects({ savedObjectsClient, pkgkey, assetType })
);

return [];
// installKibanaSavedObjects returns AssetReference[], so .map creates AssetReference[][]
// call .flat to flatten into one dimensional array
return Promise.all(installationPromises).then(results => results.flat());
}

export async function saveInstallationReferences(options: {
Expand All @@ -85,57 +80,29 @@ export async function saveInstallationReferences(options: {
return results;
}

async function installObjects({
async function installKibanaSavedObjects({
savedObjectsClient,
pkgkey,
asset,
assetType,
}: {
savedObjectsClient: SavedObjectsClientContract;
pkgkey: string;
asset: AssetType;
}) {
const isSameType = ({ path }: Registry.ArchiveEntry) => asset === Registry.pathParts(path).type;
const toBeSavedObjects = await getObjects(pkgkey, isSameType);
const createResults = await savedObjectsClient.bulkCreate(toBeSavedObjects, { overwrite: true });
const createdObjects = createResults.saved_objects;
const installed = createdObjects.map(toAssetReference);

return installed;
}

async function installPipelines({
callCluster,
pkgkey,
}: {
callCluster: CallESAsCurrentUser;
pkgkey: string;
assetType: AssetType;
}) {
const isPipeline = ({ path }: Registry.ArchiveEntry) =>
Registry.pathParts(path).type === 'ingest-pipeline';
const paths = await Registry.getArchiveInfo(pkgkey, isPipeline);
const installationPromises = paths.map(path => installPipeline({ callCluster, path }));
const references = await Promise.all(installationPromises);

return references;
}
const isSameType = ({ path }: Registry.ArchiveEntry) =>
assetType === Registry.pathParts(path).type;

async function installPipeline({
callCluster,
path,
}: {
callCluster: CallESAsCurrentUser;
path: string;
}): Promise<AssetReference> {
const buffer = Registry.getAsset(path);
// sample data is invalid json. strip the offending parts before parsing
const json = buffer.toString('utf8').replace(/\\/g, '');
const pipeline = JSON.parse(json);
const { file, type } = Registry.pathParts(path);
const id = file.replace('.json', '');
// TODO: any sort of error, not "happy path", handling
await callCluster('ingest.putPipeline', { id, body: pipeline });

return { id, type };
const toBeSavedObjects = await getObjects(pkgkey, isSameType);
if (toBeSavedObjects.length === 0) {
return [];
} else {
const createResults = await savedObjectsClient.bulkCreate(toBeSavedObjects, {
overwrite: true,
});
const createdObjects = createResults.saved_objects;
const installed = createdObjects.map(toAssetReference);
return installed;
}
}

function toAssetReference({ id, type }: SavedObject) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,20 @@

import { SavedObjectsClientContract } from 'src/core/server/';
import { SAVED_OBJECT_TYPE } from '../../common/constants';
import { getInstallationObject } from './index';
import {
getInstallationObject,
assetUsesObjects,
CallESAsCurrentUser,
getPackageInfo,
} from './index';
import { AssetType } from '../../common/types';

export async function removeInstallation(options: {
savedObjectsClient: SavedObjectsClientContract;
pkgkey: string;
callCluster: CallESAsCurrentUser;
}) {
const { savedObjectsClient, pkgkey } = options;
const { savedObjectsClient, pkgkey, callCluster } = options;
const installation = await getInstallationObject({ savedObjectsClient, pkgkey });
const installedObjects = (installation && installation.attributes.installed) || [];

Expand All @@ -21,11 +28,24 @@ export async function removeInstallation(options: {
await savedObjectsClient.delete(SAVED_OBJECT_TYPE, pkgkey);

// Delete the installed assets
const deletePromises = installedObjects.map(async ({ id, type }) =>
savedObjectsClient.delete(type, id)
);
const deletePromises = installedObjects.map(async ({ id, type }) => {
if (assetUsesObjects(type as AssetType)) {
savedObjectsClient.delete(type, id);
} else if (type === AssetType.ingestPipeline) {
deletePipeline(callCluster, id);
}
});
await Promise.all(deletePromises);

// successful delete's in SO client return {}. return something more useful
return installedObjects;
const packageInfo = await getPackageInfo({ savedObjectsClient, pkgkey });

return packageInfo;
}

async function deletePipeline(callCluster: CallESAsCurrentUser, id: string): Promise<void> {
// '*' shouldn't ever appear here, but it still would delete all ingest pipelines
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we do something (log, error, etc) when we get an unexpected value?

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 think we should. I'll open an issue.

if (id && id !== '*') {
await callCluster('ingest.deletePipeline', { id });
}
}