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.

Sign up for GitHub

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] handle unremovable packages #64096

Merged
merged 2 commits into from
Apr 22, 2020
Merged

[EPM] handle unremovable packages #64096

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5303539
remove endpoint handles unremovable packages
neptunian Apr 21, 2020
d921fab
adjust UI to disallow removing of unremovable packages
neptunian Apr 21, 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
1 change: 1 addition & 0 deletions x-pack/plugins/ingest_manager/common/types/models/epm.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ export interface RegistryPackage {
icons?: RegistryImage[];
assets?: string[];
internal?: boolean;
removable?: boolean;
format_version: string;
datasets?: Dataset[];
datasources?: RegistryDatasource[];
Expand Down
12 changes: 10 additions & 2 deletions ...ingest_manager/public/applications/ingest_manager/sections/epm/screens/detail/content.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,18 @@ export function Content(props: ContentProps) {

type ContentPanelProps = PackageInfo & Pick<DetailParams, 'panel'>;
export function ContentPanel(props: ContentPanelProps) {
const { panel, name, version, assets, title } = props;
const { panel, name, version, assets, title, removable } = props;
switch (panel) {
case 'settings':
return <SettingsPanel name={name} version={version} assets={assets} title={title} />;
return (
<SettingsPanel
name={name}
version={version}
assets={assets}
title={title}
removable={removable}
/>
);
case 'data-sources':
return <DataSourcesPanel name={name} version={version} />;
case 'overview':
Expand Down
36 changes: 27 additions & 9 deletions ...manager/public/applications/ingest_manager/sections/epm/screens/detail/settings_panel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,19 +13,24 @@ import { InstallStatus, PackageInfo } from '../../../../types';
import { InstallationButton } from './installation_button';
import { useGetDatasources } from '../../../../hooks';

const NoteLabel = () => (
<FormattedMessage
id="xpack.ingestManager.integrations.settings.packageUninstallNoteDescription.packageUninstallNoteLabel"
defaultMessage="Note:"
/>
);
export const SettingsPanel = (
props: Pick<PackageInfo, 'assets' | 'name' | 'title' | 'version'>
props: Pick<PackageInfo, 'assets' | 'name' | 'title' | 'version' | 'removable'>
) => {
const getPackageInstallStatus = useGetPackageInstallStatus();
const { data: datasourcesData } = useGetDatasources({
perPage: 0,
page: 1,
kuery: `datasources.package.name:${props.name}`,
});
const { name, title } = props;
const { name, title, removable } = props;
const packageInstallStatus = getPackageInstallStatus(name);
const packageHasDatasources = !!datasourcesData?.total;

return (
<EuiText>
<EuiTitle>
Expand Down Expand Up @@ -89,12 +94,12 @@ export const SettingsPanel = (
<p>
<InstallationButton
{...props}
disabled={!datasourcesData ? true : packageHasDatasources}
disabled={!datasourcesData || removable === false ? true : packageHasDatasources}
/>
</p>
</EuiFlexItem>
</EuiFlexGroup>
{packageHasDatasources && (
{packageHasDatasources && removable === true && (
<p>
<FormattedMessage
id="xpack.ingestManager.integrations.settings.packageUninstallNoteDescription.packageUninstallNoteDetail"
Expand All @@ -103,10 +108,23 @@ export const SettingsPanel = (
title,
strongNote: (
<strong>
<FormattedMessage
id="xpack.ingestManager.integrations.settings.packageUninstallNoteDescription.packageUninstallNoteLabel"
defaultMessage="Note:"
/>
<NoteLabel />
</strong>
),
}}
/>
</p>
)}
{removable === false && (
<p>
<FormattedMessage
id="xpack.ingestManager.integrations.settings.packageUninstallNoteDescription.packageUninstallUninstallableNoteDetail"
defaultMessage="{strongNote} The {title} integration is installed by default and cannot be removed."
values={{
title,
strongNote: (
<strong>
<NoteLabel />
</strong>
),
}}
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/ingest_manager/server/saved_objects.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,7 @@ export const savedObjectMappings = {
name: { type: 'keyword' },
version: { type: 'keyword' },
internal: { type: 'boolean' },
removable: { type: 'boolean' },
es_index_patterns: {
dynamic: false,
type: 'object',
Expand Down
6 changes: 5 additions & 1 deletion x-pack/plugins/ingest_manager/server/services/epm/packages/install.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ export async function installPackage(options: {
// TODO: change epm API to /packageName/version so we don't need to do this
const [pkgName, pkgVersion] = pkgkey.split('-');
const registryPackageInfo = await Registry.fetchInfo(pkgName, pkgVersion);
const { internal = false } = registryPackageInfo;
const { internal = false, removable = true } = registryPackageInfo;

const installKibanaAssetsPromise = installKibanaAssets({
savedObjectsClient,
Expand Down Expand Up @@ -127,6 +127,7 @@ export async function installPackage(options: {
pkgName,
pkgVersion,
internal,
removable,
toSaveAssetRefs,
toSaveESIndexPatterns,
});
Expand Down Expand Up @@ -158,6 +159,7 @@ export async function saveInstallationReferences(options: {
pkgName: string;
pkgVersion: string;
internal: boolean;
removable: boolean;
toSaveAssetRefs: AssetReference[];
toSaveESIndexPatterns: Record<string, string>;
}) {
Expand All @@ -166,6 +168,7 @@ export async function saveInstallationReferences(options: {
pkgName,
pkgVersion,
internal,
removable,
toSaveAssetRefs,
toSaveESIndexPatterns,
} = options;
Expand All @@ -191,6 +194,7 @@ export async function saveInstallationReferences(options: {
name: pkgName,
version: pkgVersion,
internal,
removable,
},
{ id: pkgName, overwrite: true }
);
Expand Down
5 changes: 4 additions & 1 deletion x-pack/plugins/ingest_manager/server/services/epm/packages/remove.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,10 @@ export async function removeInstallation(options: {
// TODO: the epm api should change to /name/version so we don't need to do this
const [pkgName] = pkgkey.split('-');
const installation = await getInstallation({ savedObjectsClient, pkgName });
const installedObjects = installation?.installed || [];
if (!installation) throw new Error('integration does not exist');
if (installation.removable === false)
throw new Error(`The ${pkgName} integration is installed by default and cannot be removed`);
Copy link
Member

Choose a reason for hiding this comment

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

Could we throw BadRequest error so we have a 400 it's a user error?

Copy link
Contributor

Choose a reason for hiding this comment

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

If someone tries to do this through the API, what response code does he get currently?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ruflin 500

Copy link
Contributor

Choose a reason for hiding this comment

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

@jfsiii Any thoughts on which error code we should return?

Overall, I would not block the PR on which error code we should return. Which API error codes we return where we should probably have a general look.

Copy link
Contributor Author

@neptunian neptunian Apr 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@nchaulet Because NP http interface does not have native support for converting Boom exceptions into HTTP responses I am thinking we need some kind of helper for this. Otherwise we are having to check for each type of error? https://github.com/elastic/kibana/blob/master/x-pack/plugins/ingest_manager/server/routes/agent/handlers.ts#L57 . Perhaps we can always through Boom errors and always return custom errors that pass along Boom results?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think a 4xx is appropriate because it's a client error vs a server error. At no time will the request succeed, so we should make sure the client knows the issue is on their end.

Any of 400, 403, or 405 are good, IMO.

How about 400 for now and I/we can come back to it during the API consistency ticket for Beta?

Copy link
Member

Choose a reason for hiding this comment

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

Yes it need some custom code in the handler I did it like this for some fleet APIs https://github.com/elastic/kibana/blob/master/x-pack/plugins/ingest_manager/server/routes/agent/handlers.ts#L122

const installedObjects = installation.installed || [];

// Delete the manager saved object with references to the asset objects
// could also update with [] or some other state
Expand Down