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

[Security Solution][Detections] Fix fetching package info from registry for installed integrations #134732

Merged
merged 2 commits into from
Jun 20, 2022
Merged

[Security Solution][Detections] Fix fetching package info from registry for installed integrations #134732

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
fa76212
Fix fetching package info from registry for installed integrations
banderror Jun 20, 2022
d6a6a46
Improve console logging
banderror Jun 20, 2022
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
50 changes: 40 additions & 10 deletions ...ection_engine/routes/fleet/get_installed_integrations/get_installed_integrations_route.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,25 @@
* 2.0.
*/

import { Logger } from '@kbn/core/server';
import { transformError } from '@kbn/securitysolution-es-utils';

import { initPromisePool } from '../../../../../utils/promise_pool';
import { buildSiemResponse } from '../../utils';
import type { SecuritySolutionPluginRouter } from '../../../../../types';

import { DETECTION_ENGINE_INSTALLED_INTEGRATIONS_URL } from '../../../../../../common/constants';
import { GetInstalledIntegrationsResponse } from '../../../../../../common/detection_engine/schemas/response/get_installed_integrations_response_schema';
import { buildSiemResponse } from '../../utils';
import { createInstalledIntegrationSet } from './installed_integration_set';

const MAX_CONCURRENT_REQUESTS_TO_PACKAGE_REGISTRY = 5;

/**
* Returns an array of installed Fleet integrations and their packages.
*/
export const getInstalledIntegrationsRoute = (router: SecuritySolutionPluginRouter) => {
export const getInstalledIntegrationsRoute = (
router: SecuritySolutionPluginRouter,
logger: Logger
) => {
router.get(
{
path: DETECTION_ENGINE_INSTALLED_INTEGRATIONS_URL,
Expand All @@ -39,17 +46,40 @@ export const getInstalledIntegrationsRoute = (router: SecuritySolutionPluginRout
set.addPackagePolicy(policy);
});

const registryPackages = await Promise.all(
set.getPackages().map((packageInfo) => {
return fleet.packages.getRegistryPackage(
const registryPackages = await initPromisePool({
concurrency: MAX_CONCURRENT_REQUESTS_TO_PACKAGE_REGISTRY,
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for adding a pool and capping the requests here! I think we discussed this in the initial PR for this endpoint, but I'd be curious to see what the upper bounds are here for large deployments leveraging integrations. Would be good to test with a large number of integration policies, and then also with a large number of installed integrations.

One issue we have with fetching package policies is we're not handling pagination, so we'll cap out at the max for that initial page. Then for fetching the individual packages, I'm a little worried if there's say 100+ installed packages and we end up making 100+ requests to fleet each time an individual user hits the Rules Mgmt or Details pages. We do have caching client-side through the react-query wrapper, but nothing here server side, so could get messy if there's a few different users bouncing around those pages.

If this becomes an issue in production, users can at least disable this feature and short-circuit this request on the Rule Mgmt page w/ the Kibana Advanced Setting, but definitely something we'll want to test further. Will be good to get feedback from the fleet folks about any other API's we might be able to use here to ease the pressure on the fleet side -- would be great if we could just get all installed packages in one request instead of going the integration policy route.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would be good to test with a large number of integration policies, and then also with a large number of installed integrations.

Agree, will do it 👍

One issue we have with fetching package policies is we're not handling pagination, so we'll cap out at the max for that initial page.

I'll check how this method works when the perPage is not specified. I thought it just returns all the policies, but it might also set a default page size instead. Anyway, definitely need to test this 👍

I'm a little worried if there's say 100+ installed packages and we end up making 100+ requests to fleet each time an individual user hits the Rules Mgmt or Details pages. We do have caching client-side through the react-query wrapper, but nothing here server side

FWIW Fleet service caches fetched packages on the server-side. So only the first request may be slow (it depends linearly on the number of installed packages). The subsequent requests should be fast and not generate outgoing HTTP requests to EPR. I'm not sure 100% about this though -- so will test it as well. 👍

I will also test it with a large number of installed packages and a large number of integration policies.

Will be good to get feedback from the fleet folks about any other API's we might be able to use here to ease the pressure on the fleet side -- would be great if we could just get all installed packages in one request instead of going the integration policy route.

I'll open a discussion issue and share it with Fleet folks. 👍

Thank you @spong, this is a lot of great points!

Copy link
Member

Choose a reason for hiding this comment

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

FWIW Fleet service caches fetched packages on the server-side. So only the first request may be slow (it depends linearly on the number of installed packages). The subsequent requests should be fast and not generate outgoing HTTP requests to EPR. I'm not sure 100% about this though -- so will test it as well. 👍

This makes me feel a lot better about the upper bound cases. I didn't realize the fleet service cached as well, thanks @banderror!

And sounds good with the other points as well, thank you for verifying here 👍

items: set.getPackages(),
executor: async (packageInfo) => {
const registryPackage = await fleet.packages.getRegistryPackage(
packageInfo.package_name,
packageInfo.package_version
);
})
);
return registryPackage;
},
});

if (registryPackages.errors.length > 0) {
const errors = registryPackages.errors.map(({ error, item }) => {
return {
error,
packageId: `${item.package_name}@${item.package_version}`,
};
});

const packages = errors.map((e) => e.packageId).join(', ');
logger.error(
`Unable to retrieve installed integrations. Error fetching packages from registry: ${packages}.`
);

errors.forEach(({ error, packageId }) => {
const logMessage = `Error fetching package info from registry for ${packageId}`;
const logReason = error instanceof Error ? error.message : String(error);
logger.debug(`${logMessage}. ${logReason}`);
});
}

registryPackages.forEach((registryPackage) => {
set.addRegistryPackage(registryPackage.packageInfo);
registryPackages.results.forEach(({ result }) => {
set.addRegistryPackage(result.packageInfo);
});

const installedIntegrations = set.getIntegrations();
Expand Down
15 changes: 10 additions & 5 deletions ...lib/detection_engine/routes/fleet/get_installed_integrations/installed_integration_set.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
* 2.0.
*/

import { flatten } from 'lodash';
import { capitalize, flatten } from 'lodash';
import { PackagePolicy, RegistryPackage } from '@kbn/fleet-plugin/common';
import {
InstalledIntegration,
Expand Down Expand Up @@ -35,7 +35,7 @@ export const createInstalledIntegrationSet = (): IInstalledIntegrationSet => {

const addPackagePolicy = (policy: PackagePolicy): void => {
const packageInfo = getPackageInfoFromPolicy(policy);
const integrationsInfo = getIntegrationsInfoFromPolicy(policy);
const integrationsInfo = getIntegrationsInfoFromPolicy(policy, packageInfo);
const packageKey = `${packageInfo.package_name}:${packageInfo.package_version}`;
const existingPackageInfo = packageMap.get(packageKey);

Expand Down Expand Up @@ -121,11 +121,16 @@ const getPackageInfoFromPolicy = (policy: PackagePolicy): InstalledPackageBasicI
};
};

const getIntegrationsInfoFromPolicy = (policy: PackagePolicy): InstalledIntegrationBasicInfo[] => {
const getIntegrationsInfoFromPolicy = (
policy: PackagePolicy,
packageInfo: InstalledPackageBasicInfo
): InstalledIntegrationBasicInfo[] => {
return policy.inputs.map((input) => {
const integrationName = normalizeString(input.policy_template); // e.g. 'cloudtrail'
const integrationTitle = `${packageInfo.package_title} ${capitalize(integrationName)}`; // e.g. 'AWS Cloudtrail'
return {
integration_name: normalizeString(input.policy_template),
integration_title: '', // this gets initialized later in addRegistryPackage()
integration_name: integrationName,
integration_title: integrationTitle, // title gets re-initialized later in addRegistryPackage()
is_enabled: input.enabled,
};
});
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugins/security_solution/server/routes/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ export const initRoutes = (

getRuleExecutionEventsRoute(router);

getInstalledIntegrationsRoute(router);
getInstalledIntegrationsRoute(router, logger);

createTimelinesRoute(router, config, security);
patchTimelinesRoute(router, config, security);
Expand Down