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] Cache call to getBundledPackages #172640

Merged
merged 10 commits into from
Dec 8, 2023
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ export async function getLatestApmPackage({
APM_PACKAGE_NAME
);
const packageInfo =
'buffer' in latestPackage
'getBuffer' in latestPackage
? (await packageClient.readBundledPackage(latestPackage)).packageInfo
: latestPackage;
const {
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/fleet/common/types/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ export interface FleetConfigType {
disableRegistryVersionCheck?: boolean;
bundledPackageLocation?: string;
testSecretsIndex?: string;
disableBundledPackagesCache?: boolean;
};
internal?: {
disableILMPolicies: boolean;
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugins/fleet/common/types/models/epm.ts
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ export type ArchivePackage = PackageSpecManifest &
export interface BundledPackage {
name: string;
version: string;
buffer: Buffer;
getBuffer: () => Promise<Buffer>;
}

export type RegistryPackage = PackageSpecManifest &
Expand Down
3 changes: 3 additions & 0 deletions x-pack/plugins/fleet/server/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,9 @@ export const config: PluginConfigDescriptor = {
disableRegistryVersionCheck: schema.boolean({ defaultValue: false }),
allowAgentUpgradeSourceUri: schema.boolean({ defaultValue: false }),
bundledPackageLocation: schema.string({ defaultValue: DEFAULT_BUNDLED_PACKAGE_LOCATION }),
disableBundledPackagesCache: schema.boolean({
defaultValue: false,
}),
}),
packageVerification: schema.object({
gpgKeyPath: schema.string({ defaultValue: DEFAULT_GPG_KEY_PATH }),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -169,12 +169,16 @@ function getTest(
};
break;
case testKeys[5]:
const bundledPackage = { name: 'package name', version: '8.0.0', buffer: Buffer.from([]) };
const bundledPackage = {
name: 'package name',
version: '8.0.0',
getBuffer: () => Buffer.from([]),
};
test = {
method: mocks.packageClient.readBundledPackage.bind(mocks.packageClient),
args: [bundledPackage],
spy: jest.spyOn(epmArchiveParse, 'generatePackageInfoFromArchiveBuffer'),
spyArgs: [bundledPackage.buffer, 'application/zip'],
spyArgs: [bundledPackage.getBuffer(), 'application/zip'],
spyResponse: {
packageInfo: { name: 'readBundledPackage test' },
paths: ['/some/test/path'],
Expand Down
4 changes: 3 additions & 1 deletion x-pack/plugins/fleet/server/services/epm/package_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,9 @@ class PackageClientImpl implements PackageClient {

public async readBundledPackage(bundledPackage: BundledPackage) {
await this.#runPreflight(READ_PACKAGE_INFO_AUTHZ);
return generatePackageInfoFromArchiveBuffer(bundledPackage.buffer, 'application/zip');
const archiveBuffer = await bundledPackage.getBuffer();

return generatePackageInfoFromArchiveBuffer(archiveBuffer, 'application/zip');
}

public async getPackage(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,28 +7,37 @@

import fs from 'fs/promises';

import { omit } from 'lodash';

import { loggingSystemMock } from '@kbn/core-logging-server-mocks';

import { appContextService } from '../../app_context';

import { getBundledPackageByPkgKey, getBundledPackages } from './bundled_packages';
import {
getBundledPackageByPkgKey,
getBundledPackages,
_purgeBundledPackagesCache,
} from './bundled_packages';

jest.mock('fs/promises');
jest.mock('../../app_context');

describe('bundledPackages', () => {
beforeAll(() => {
beforeEach(() => {
jest.mocked(appContextService.getConfig).mockReturnValue({
developer: {
bundledPackageLocation: '/tmp/test',
},
} as any);
jest.mocked(appContextService.getLogger).mockReturnValue(loggingSystemMock.createLogger());
});
beforeEach(() => {
_purgeBundledPackagesCache();
jest.mocked(fs.stat).mockResolvedValue({} as any);
jest.mocked(fs.readdir).mockResolvedValue(['apm-8.8.0.zip', 'test-1.0.0.zip'] as any);
jest.mocked(fs.readFile).mockResolvedValue(Buffer.from('TEST'));
jest
.mocked(fs.readdir)
.mockReset()
.mockResolvedValue(['apm-8.8.0.zip', 'test-1.0.0.zip'] as any);

jest.mocked(fs.readFile).mockReset().mockResolvedValue(Buffer.from('TEST'));
});

afterEach(() => {
Expand All @@ -44,40 +53,76 @@ describe('bundledPackages', () => {
it('return packages in bundled directory', async () => {
const packages = await getBundledPackages();
expect(packages).toEqual([
{
expect.objectContaining({
name: 'apm',
version: '8.8.0',
buffer: Buffer.from('TEST'),
},
{
}),
expect.objectContaining({
name: 'test',
version: '1.0.0',
buffer: Buffer.from('TEST'),
},
}),
]);

expect(await packages[0]?.getBuffer()).toEqual(Buffer.from('TEST'));
expect(await packages[1]?.getBuffer()).toEqual(Buffer.from('TEST'));
});

it('should use cache if called multiple time', async () => {
const packagesRes1 = await getBundledPackages();
const packagesRes2 = await getBundledPackages();
expect(packagesRes1.map((p) => omit(p, 'getBuffer'))).toEqual(
packagesRes2.map((p) => omit(p, 'getBuffer'))
);
expect(fs.readdir).toBeCalledTimes(1);
});

it('should cache getBuffer if called multiple time in the scope of getBundledPackages', async () => {
const packagesRes1 = await getBundledPackages();

await packagesRes1[0].getBuffer();
await packagesRes1[0].getBuffer();
expect(fs.readFile).toBeCalledTimes(1);
});

it('should not use cache if called multiple time and cache is disabled', async () => {
jest.mocked(appContextService.getConfig).mockReturnValue({
developer: {
bundledPackageLocation: '/tmp/test',
disableBundledPackagesCache: true,
},
} as any);
await getBundledPackages();
await getBundledPackages();
expect(fs.readdir).toBeCalledTimes(2);
});
});
describe('getBundledPackageByPkgKey', () => {
it('should return package by name if no version is provided', async () => {
const pkg = await getBundledPackageByPkgKey('apm');

expect(pkg).toBeDefined();
expect(pkg).toEqual({
name: 'apm',
version: '8.8.0',
buffer: Buffer.from('TEST'),
});
expect(pkg).toEqual(
expect.objectContaining({
name: 'apm',
version: '8.8.0',
})
);

expect(await pkg?.getBuffer()).toEqual(Buffer.from('TEST'));
});

it('should return package by name and version if version is provided', async () => {
const pkg = await getBundledPackageByPkgKey('apm-8.8.0');

expect(pkg).toBeDefined();
expect(pkg).toEqual({
name: 'apm',
version: '8.8.0',
buffer: Buffer.from('TEST'),
});
expect(pkg).toEqual(
expect.objectContaining({
name: 'apm',
version: '8.8.0',
})
);

expect(await pkg?.getBuffer()).toEqual(Buffer.from('TEST'));
});

it('should return package by name and version if version is provided and do not exists', async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,36 @@
import fs from 'fs/promises';
import path from 'path';

import { once } from 'lodash';

import type { BundledPackage, Installation } from '../../../types';
import { BundledPackageLocationNotFoundError } from '../../../errors';
import { appContextService } from '../../app_context';
import { splitPkgKey, pkgToPkgKey } from '../registry';

let CACHE_BUNDLED_PACKAGES: BundledPackage[] | undefined;

export function _purgeBundledPackagesCache() {
CACHE_BUNDLED_PACKAGES = undefined;
}

function bundledPackagesFromCache() {
if (!CACHE_BUNDLED_PACKAGES) {
throw new Error('CACHE_BUNDLED_PACKAGES is not populated');
}

return CACHE_BUNDLED_PACKAGES.map(({ name, version, getBuffer }) => ({
name,
version,
getBuffer: once(getBuffer),
}));
}

export async function getBundledPackages(): Promise<BundledPackage[]> {
const config = appContextService.getConfig();
if (config?.developer?.disableBundledPackagesCache !== true && CACHE_BUNDLED_PACKAGES) {
return bundledPackagesFromCache();
}

const bundledPackageLocation = config?.developer?.bundledPackageLocation;

Expand All @@ -38,19 +61,21 @@ export async function getBundledPackages(): Promise<BundledPackage[]> {

const result = await Promise.all(
zipFiles.map(async (zipFile) => {
const file = await fs.readFile(path.join(bundledPackageLocation, zipFile));

const { pkgName, pkgVersion } = splitPkgKey(zipFile.replace(/\.zip$/, ''));

const getBuffer = () => fs.readFile(path.join(bundledPackageLocation, zipFile));
Copy link
Member

Choose a reason for hiding this comment

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

drive-by comment: maybe this could be wrapped in once()? Behaviour should be similar as before (this change), but no risk in reading from disk multiple times if the function is called multiple times for whatever reason as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

@dgieselaar with the once this will cache the file in memory forever right? it's probably something we want to avoid, we are trying to reduce the fleet memory footprint in kibana too.

Copy link
Member Author

Choose a reason for hiding this comment

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

what we try to achieve with that PR is to avoid to load all the bundled package in memory when we call getBundledPackage (that is called in a lot of place in fleet code)

Copy link
Member

Choose a reason for hiding this comment

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

with the once this will cache the file in memory forever right?
No, only in the scope of this call to getBundledPackages. Which would be the same as before. Wrapping this in once simply means that if downstream getBuffer is called twice, it is only read from disk once. Previously it was also read from disk once - this change introduces the possibility that it is read from disk multiple times for each getBundledPackages call.

Copy link
Member Author

Choose a reason for hiding this comment

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

@dgieselaar that PR changed the getBundledPackages to cache the results, so those bundled package object we are returning are now global, I made a change in d2c575e to make a copy for each getBundledPackages and use a once there does it make sense to you?

Copy link
Member

Choose a reason for hiding this comment

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

ah, yes, makes sense!


return {
name: pkgName,
version: pkgVersion,
buffer: file,
getBuffer,
};
})
);

return result;
CACHE_BUNDLED_PACKAGES = result;

return bundledPackagesFromCache();
} catch (err) {
const logger = appContextService.getLogger();
logger.warn(`Unable to read bundled packages from ${bundledPackageLocation}`);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,7 @@ describe('install', () => {
mockGetBundledPackageByPkgKey.mockResolvedValue({
name: 'test_package',
version: '1.0.0',
buffer: Buffer.from('test_package'),
getBuffer: async () => Buffer.from('test_package'),
});

const response = await installPackage({
Expand Down
4 changes: 3 additions & 1 deletion x-pack/plugins/fleet/server/services/epm/packages/install.ts
Original file line number Diff line number Diff line change
Expand Up @@ -762,10 +762,12 @@ export async function installPackage(args: InstallPackageParams): Promise<Instal
`found bundled package for requested install of ${pkgkey} - installing from bundled package archive`
);

const archiveBuffer = await matchingBundledPackage.getBuffer();

const response = await installPackageByUpload({
savedObjectsClient,
esClient,
archiveBuffer: matchingBundledPackage.buffer,
archiveBuffer,
contentType: 'application/zip',
spaceId,
version: matchingBundledPackage.version,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ describe('fetchInfo', () => {
mockGetBundledPackageByName.mockResolvedValueOnce({
name: 'test-package',
version: '1.0.0',
buffer: Buffer.from(''),
getBuffer: async () => Buffer.from(''),
});
MockArchive.generatePackageInfoFromArchiveBuffer.mockResolvedValueOnce({
paths: [],
Expand Down
3 changes: 2 additions & 1 deletion x-pack/plugins/fleet/server/services/epm/registry/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -198,8 +198,9 @@ export async function getBundledArchive(
const bundledPackage = await getBundledPackageByName(pkgName);

if (bundledPackage && bundledPackage.version === pkgVersion) {
const archiveBuffer = await bundledPackage.getBuffer();
const archivePackage = await generatePackageInfoFromArchiveBuffer(
bundledPackage.buffer,
archiveBuffer,
'application/zip'
);

Expand Down
8 changes: 4 additions & 4 deletions x-pack/plugins/fleet/server/services/preconfiguration.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -790,13 +790,13 @@ describe('policy preconfiguration', () => {
{
name: 'test_package',
version: '1.0.0',
buffer: Buffer.from('test_package'),
getBuffer: () => Promise.resolve(Buffer.from('test_package')),
},

{
name: 'test_package_2',
version: '1.0.0',
buffer: Buffer.from('test_package_2'),
getBuffer: () => Promise.resolve(Buffer.from('test_package_2')),
},
]);

Expand Down Expand Up @@ -834,7 +834,7 @@ describe('policy preconfiguration', () => {
{
name: 'test_package',
version: '1.0.0',
buffer: Buffer.from('test_package'),
getBuffer: () => Promise.resolve(Buffer.from('test_package')),
},
]);

Expand Down Expand Up @@ -875,7 +875,7 @@ describe('policy preconfiguration', () => {
{
name: 'test_package',
version: '1.0.0',
buffer: Buffer.from('test_package'),
getBuffer: () => Promise.resolve(Buffer.from('test_package')),
},
]);

Expand Down
1 change: 1 addition & 0 deletions x-pack/test/fleet_api_integration/config.base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ export default async function ({ readConfigFile }: FtrConfigProviderContext) {
`--xpack.fleet.packages.0.version=latest`,
...(registryPort ? [`--xpack.fleet.registryUrl=http://localhost:${registryPort}`] : []),
`--xpack.fleet.developer.bundledPackageLocation=${BUNDLED_PACKAGE_DIR}`,
`--xpack.fleet.developer.disableBundledPackagesCache=true`,
'--xpack.cloudSecurityPosture.enabled=true',
`--xpack.fleet.developer.maxAgentPoliciesWithInactivityTimeout=10`,
`--xpack.fleet.packageVerification.gpgKeyPath=${getFullPath(
Expand Down
Loading