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

[Fleet] Don't fail on errors in 'update' or 'reupdate' operation in /setup #97404

Merged
merged 5 commits into from
Apr 19, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions x-pack/plugins/fleet/common/types/models/epm.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,11 @@ export enum InstallStatus {
uninstalling = 'uninstalling',
}

export interface DefaultPackagesInstallationError {
installType: InstallType;
error: Error;
}

export type InstallType = 'reinstall' | 'reupdate' | 'rollback' | 'update' | 'install' | 'unknown';
export type InstallSource = 'registry' | 'upload';

Expand Down
3 changes: 3 additions & 0 deletions x-pack/plugins/fleet/common/types/rest_spec/ingest_setup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,10 @@
* 2.0.
*/

import type { DefaultPackagesInstallationError } from '../models/epm';

export interface PostIngestSetupResponse {
isInitialized: boolean;
preconfigurationError?: { name: string; message: string };
nonFatalPackageUpgradeErrors?: DefaultPackagesInstallationError[];
}
7 changes: 7 additions & 0 deletions x-pack/plugins/fleet/public/applications/fleet/app.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,13 @@ export const WithPermissionsAndSetup: React.FC = memo(({ children }) => {
}),
});
}
if (setupResponse.data.nonFatalPackageUpgradeErrors) {
notifications.toasts.addError(setupResponse.data.nonFatalPackageUpgradeErrors, {
title: i18n.translate('xpack.fleet.setup.nonFatalPackageErrorsTitle', {
defaultMessage: 'One or more packages could not be successfully upgraded',
}),
});
}
} catch (err) {
setInitializationError(err);
}
Expand Down
6 changes: 5 additions & 1 deletion x-pack/plugins/fleet/server/routes/setup/handlers.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,11 @@ describe('FleetSetupHandler', () => {

it('POST /setup succeeds w/200 and body of resolved value', async () => {
mockSetupIngestManager.mockImplementation(() =>
Promise.resolve({ isInitialized: true, preconfigurationError: undefined })
Promise.resolve({
isInitialized: true,
preconfigurationError: undefined,
nonFatalPackageUpgradeErrors: [],
})
);
await fleetSetupHandler(context, request, response);

Expand Down
10 changes: 8 additions & 2 deletions x-pack/plugins/fleet/server/routes/setup/handlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,14 @@ export const fleetSetupHandler: RequestHandler = async (context, request, respon
try {
const soClient = context.core.savedObjects.client;
const esClient = context.core.elasticsearch.client.asCurrentUser;
const body: PostIngestSetupResponse = { isInitialized: true };
await setupIngestManager(soClient, esClient);
const setupStatus = await setupIngestManager(soClient, esClient);
const body: PostIngestSetupResponse = {
isInitialized: true,
};

if (setupStatus.nonFatalPackageUpgradeErrors.length > 0) {
body.nonFatalPackageUpgradeErrors = setupStatus.nonFatalPackageUpgradeErrors;
}

Copy link
Contributor Author

@skh skh Apr 18, 2021

Choose a reason for hiding this comment

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

This is unrelated to my change, but I noticed that preconfigurationError is not returned here. I didn't have time to dig into it but I'm not sure this is intentional. cc @Zacqary

Copy link
Member

Choose a reason for hiding this comment

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

I remember some discussion around what should happen on errors. I'm sure @Zacqary will follow up if needed.

return response.ok({
body,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,11 @@ export async function bulkInstallPackages({
skipPostInstall: true,
});
if (installResult.error) {
return { name: packageName, error: installResult.error };
return {
name: packageName,
error: installResult.error,
installType: installResult.installType,
};
} else {
return {
name: packageName,
Expand Down Expand Up @@ -75,7 +79,11 @@ export async function bulkInstallPackages({
const packageName = packagesToInstall[index];
if (result.status === 'fulfilled') {
if (result.value && result.value.error) {
return { name: packageName, error: result.value.error };
return {
name: packageName,
error: result.value.error,
installType: result.value.installType,
};
} else {
return result.value;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ describe('ensureInstalledDefaultPackages', () => {
];
});
const resp = await ensureInstalledDefaultPackages(soClient, jest.fn());
expect(resp).toEqual([mockInstallation.attributes]);
expect(resp.installations).toEqual([mockInstallation.attributes]);
});
it('should throw the first Error it finds', async () => {
class SomeCustomError extends Error {}
Expand Down
28 changes: 24 additions & 4 deletions x-pack/plugins/fleet/server/services/epm/packages/install.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,12 @@ import type { ElasticsearchClient, SavedObject, SavedObjectsClientContract } fro
import { generateESIndexPatterns } from '../elasticsearch/template/template';

import { defaultPackages } from '../../../../common';
import type { BulkInstallPackageInfo, InstallablePackage, InstallSource } from '../../../../common';
import type {
BulkInstallPackageInfo,
InstallablePackage,
InstallSource,
DefaultPackagesInstallationError,
} from '../../../../common';
import {
IngestManagerError,
PackageOperationNotSupportedError,
Expand Down Expand Up @@ -45,11 +50,17 @@ import { removeInstallation } from './remove';
import { getPackageSavedObjects } from './get';
import { _installPackage } from './_install_package';

export interface DefaultPackagesInstallationResult {
installations: Installation[];
nonFatalPackageUpgradeErrors: DefaultPackagesInstallationError[];
}

export async function ensureInstalledDefaultPackages(
savedObjectsClient: SavedObjectsClientContract,
esClient: ElasticsearchClient
): Promise<Installation[]> {
): Promise<DefaultPackagesInstallationResult> {
const installations = [];
const nonFatalPackageUpgradeErrors = [];
const bulkResponse = await bulkInstallPackages({
savedObjectsClient,
packagesToInstall: Object.values(defaultPackages),
Expand All @@ -58,19 +69,27 @@ export async function ensureInstalledDefaultPackages(

for (const resp of bulkResponse) {
if (isBulkInstallError(resp)) {
throw resp.error;
if (resp.installType && (resp.installType === 'update' || resp.installType === 'reupdate')) {
nonFatalPackageUpgradeErrors.push({ installType: resp.installType, error: resp.error });
} else {
throw resp.error;
}
} else {
installations.push(getInstallation({ savedObjectsClient, pkgName: resp.name }));
}
}

const retrievedInstallations = await Promise.all(installations);
return retrievedInstallations.map((installation, index) => {
const verifiedInstallations = retrievedInstallations.map((installation, index) => {
if (!installation) {
throw new Error(`could not get installation ${bulkResponse[index].name}`);
}
return installation;
});
return {
installations: verifiedInstallations,
nonFatalPackageUpgradeErrors,
};
}

async function isPackageVersionOrLaterInstalled(options: {
Expand Down Expand Up @@ -181,6 +200,7 @@ export async function handleInstallPackageFailure({
export interface IBulkInstallPackageError {
name: string;
error: Error;
installType?: InstallType;
}
export type BulkInstallResponse = BulkInstallPackageInfo | IBulkInstallPackageError;

Expand Down
13 changes: 9 additions & 4 deletions x-pack/plugins/fleet/server/services/setup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { i18n } from '@kbn/i18n';

import { DEFAULT_AGENT_POLICIES_PACKAGES, FLEET_SERVER_PACKAGE } from '../../common';

import type { PackagePolicy } from '../../common';
import type { PackagePolicy, DefaultPackagesInstallationError } from '../../common';

import { SO_SEARCH_LIMIT } from '../constants';

Expand All @@ -33,6 +33,7 @@ import { awaitIfFleetServerSetupPending } from './fleet_server';
export interface SetupStatus {
isInitialized: boolean;
preconfigurationError: { name: string; message: string } | undefined;
nonFatalPackageUpgradeErrors: DefaultPackagesInstallationError[];
}

export async function setupIngestManager(
Expand All @@ -46,7 +47,7 @@ async function createSetupSideEffects(
soClient: SavedObjectsClientContract,
esClient: ElasticsearchClient
): Promise<SetupStatus> {
const [installedPackages, defaultOutput] = await Promise.all([
const [defaultPackagesResult, defaultOutput] = await Promise.all([
// packages installed by default
ensureInstalledDefaultPackages(soClient, esClient),
outputService.ensureDefaultOutput(soClient),
Expand Down Expand Up @@ -142,7 +143,7 @@ async function createSetupSideEffects(
);
}

for (const installedPackage of installedPackages) {
for (const installedPackage of defaultPackagesResult.installations) {
const packageShouldBeInstalled = DEFAULT_AGENT_POLICIES_PACKAGES.some(
(packageName) => installedPackage.name === packageName
);
Expand Down Expand Up @@ -172,7 +173,11 @@ async function createSetupSideEffects(

await ensureAgentActionPolicyChangeExists(soClient, esClient);

return { isInitialized: true, preconfigurationError };
return {
isInitialized: true,
preconfigurationError,
nonFatalPackageUpgradeErrors: defaultPackagesResult.nonFatalPackageUpgradeErrors,
};
}

export async function ensureDefaultEnrollmentAPIKeysExists(
Expand Down