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] Rely on package content during installation #142353

Merged
merged 15 commits into from
Oct 11, 2022

Conversation

criamico
Copy link
Contributor

@criamico criamico commented Sep 30, 2022

Summary

Closes #115032

Removing the call to getInfo in Registry.getRegistryPackage with generatePackageInfoFromArchiveBuffer to reduce the dependency from the registry.

Tests

  1. Try to install any package, it should work as usual
  2. Run this script: node x-pack/plugins/fleet/scripts/install_all_packages/index.js - Remember to add the static path to the kibana url if you use one:

Screenshot 2022-10-03 at 18 29 38

Checklist

Delete any items that are not applicable to this PR.

  • Unit or functional tests were updated or added to match the most common scenarios
  • If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list

@criamico criamico added the Team:Fleet Team label for Observability Data Collection Fleet team label Sep 30, 2022
@criamico criamico self-assigned this Sep 30, 2022
@criamico
Copy link
Contributor Author

criamico commented Oct 3, 2022

@elasticmachine merge upstream

@criamico
Copy link
Contributor Author

criamico commented Oct 4, 2022

@elasticmachine merge upstream

@criamico
Copy link
Contributor Author

criamico commented Oct 5, 2022

@elasticmachine merge upstream

archivePath,
verificationResult: latestVerificationResult,
} = await withPackageSpan('Fetch package archive from registry', () =>
fetchArchiveBuffer({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function internally calls getInfo to retrieve the ArchiveBuffer:

export async function fetchArchiveBuffer({
pkgName,
pkgVersion,
shouldVerify,
ignoreUnverified = false,
}: {
pkgName: string;
pkgVersion: string;
shouldVerify: boolean;
ignoreUnverified?: boolean;
}): Promise<{
archiveBuffer: Buffer;
archivePath: string;
verificationResult?: PackageVerificationResult;
}> {
const logger = appContextService.getLogger();
const { download: archivePath } = await getInfo(pkgName, pkgVersion);
const archiveUrl = `${getRegistryUrl()}${archivePath}`;
const archiveBuffer = await getResponseStream(archiveUrl).then(streamToBuffer);

I wonder if I should get the ArchiveBuffer from the cache instead and remove the calls to getInfo altogether.

@criamico criamico added v8.6.0 release_note:skip Skip the PR/issue when compiling release notes labels Oct 5, 2022
@criamico criamico marked this pull request as ready for review October 5, 2022 10:41
@criamico criamico requested review from a team as code owners October 5, 2022 10:41
@criamico criamico requested a review from banderror October 5, 2022 10:41
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@criamico
Copy link
Contributor Author

criamico commented Oct 5, 2022

@elasticmachine merge upstream

Copy link
Contributor

@banderror banderror left a comment

Choose a reason for hiding this comment

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

Changes in x-pack/plugins/security_solution/server/lib/detection_engine/routes/fleet/get_installed_integrations/installed_integration_set.ts LGTM 👍

@criamico
Copy link
Contributor Author

criamico commented Oct 6, 2022

@elasticmachine merge upstream

@@ -49,7 +50,7 @@ export interface PackageClient {
getRegistryPackage(
packageName: string,
packageVersion: string
): Promise<{ packageInfo: RegistryPackage; paths: string[] }>;
): Promise<{ packageInfo: ArchivePackage; paths: string[] }>;
Copy link
Member

Choose a reason for hiding this comment

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

it seems weird to me that getRegistryPackage return an ArchivePackage should this be a new method, or should rename it getPackage ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

calling it getPackage would be a good option. I've been in doubt about the naming because the whole flow is in Registry but it's not going to use the registry anymore.. I guess we'll have to refactor further

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 changed the name to GetPackage, I will need additional approvals from other teams that also use that method.

@criamico
Copy link
Contributor Author

criamico commented Oct 6, 2022

@elasticmachine merge upstream

@criamico criamico requested review from a team as code owners October 6, 2022 14:01
@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label Oct 6, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:APM)

@criamico criamico requested a review from nchaulet October 6, 2022 15:16
Copy link
Contributor

@juliaElastic juliaElastic left a comment

Choose a reason for hiding this comment

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

LGTM

@criamico
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Member

@sorenlouv sorenlouv left a comment

Choose a reason for hiding this comment

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

apm changes lgtm

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @criamico

Copy link
Contributor

@hop-dev hop-dev left a comment

Choose a reason for hiding this comment

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

Code changes look good to me, thanks for this!

Copy link
Contributor

@dasansol92 dasansol92 left a comment

Choose a reason for hiding this comment

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

OLM changes (getRegistryPackage rename) LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:APM All issues that need APM UI Team support Team:Fleet Team label for Observability Data Collection Fleet team v8.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Fleet] Rely on the content of a package when installing it
10 participants