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] Simplify skipArchive behaviour for input packages #141671

Merged
merged 9 commits into from
Oct 13, 2022

Conversation

hop-dev
Copy link
Contributor

@hop-dev hop-dev commented Sep 23, 2022

Summary

Closes #137751

Input packages always have to get their package info from the archive, as they contain fields in their manifest that the package registry API does not return. These fields are needed to create the package policy so we need to get info from the archive even when using the GET package info API (as opposed to just at install time)

Previously, I was ensuring that input packages get their information from the archive by using (unintuitively) the skipArchive flag, which meant we went to the registry to get the packageInfo, then checked if it was an input package, and if so we go and get the info from the archive.

This PR simplifies that behaviour by always attempting to get the info from the registry first, regardless of skipArchive, then deciding whether to skip the archive.

I have also added integration tests for input packages:

  • Create valid pkg policy
  • Create invalid pkg policy
  • Get pkg info

Does this conflict with #142353 "rely on package content during installation"?

This changes the behaviour for get package info, we still rely on the archive for installation. However, we should move to not using the registry at all for the get info API to unblock #131609, but this will be more work, and maybe need some work on the EPR side, I have created an issue for this #143198

@hop-dev hop-dev requested a review from a team as a code owner September 23, 2022 15:44
@botelastic botelastic bot added the Team:Fleet Team label for Observability Data Collection Fleet team label Sep 23, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@hop-dev hop-dev added backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) release_note:skip Skip the PR/issue when compiling release notes v8.5.0 labels Sep 23, 2022
@hop-dev hop-dev changed the title [Fleet] More nput package API tests [Fleet] More input package API integration tests Sep 23, 2022
@hop-dev hop-dev marked this pull request as draft September 23, 2022 19:25
@hop-dev hop-dev force-pushed the input-pkg-api-tests branch from 1a62057 to 2307948 Compare October 4, 2022 10:16
@hop-dev hop-dev changed the title [Fleet] More input package API integration tests [Fleet] Fix input package upgrades Oct 4, 2022
@hop-dev hop-dev changed the title [Fleet] Fix input package upgrades [Fleet] Simplify skipArchive behaviour for input packages Oct 4, 2022
@hop-dev hop-dev marked this pull request as ready for review October 4, 2022 14:39
@nchaulet
Copy link
Member

nchaulet commented Oct 5, 2022

@hop-dev @criamico does this two PR are not conflicting #142353?

Why we would like (or need) to get the package info from the registry and not ES if we have the info?

@criamico
Copy link
Contributor

criamico commented Oct 5, 2022

@hop-dev @criamico does this two PR are not conflicting #142353?

Why we would like (or need) to get the package info from the registry and not ES if we have the info?

Yes I just saw your comment and was leaving a similar one. I think we need to review the whole flow and understand what we need to modify.

@hop-dev
Copy link
Contributor Author

hop-dev commented Oct 5, 2022

Are we moving to getting all information from the archive every time, or just at install time? If it's every time then I think skipArchive has to be removed as its a shortcut to get the information from the registry and build the assets paths from that instead of the archive.

@nchaulet
Copy link
Member

nchaulet commented Oct 5, 2022

I think the @criamico was for the install time, but it will probably be a good improvement to not call the registry when building the agent policy too, (it will remove a risk of failure here)

@criamico
Copy link
Contributor

criamico commented Oct 6, 2022

@nchaulet @hop-dev
#142353 is limited to the install time but reading the original ticket I thought that the aim is to decouple the registry from the spec and, on the long run, use the registry only for discovering packages.
If this understanding is correct, removing skipArchive it's a further step in this direction. Let me know, however, if I should revert that change from my PR.

@hop-dev hop-dev marked this pull request as draft October 10, 2022 15:30
@hop-dev hop-dev force-pushed the input-pkg-api-tests branch 4 times, most recently from 8508774 to e1909c8 Compare October 12, 2022 13:37
@hop-dev hop-dev added v8.6.0 and removed backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) v8.5.0 labels Oct 12, 2022
@hop-dev hop-dev force-pushed the input-pkg-api-tests branch 2 times, most recently from ff231d8 to 7ceb230 Compare October 13, 2022 08:57
@hop-dev hop-dev force-pushed the input-pkg-api-tests branch from 7ceb230 to 4162afd Compare October 13, 2022 09:12
@hop-dev hop-dev marked this pull request as ready for review October 13, 2022 09:15
@hop-dev hop-dev requested a review from criamico October 13, 2022 09:16
@hop-dev
Copy link
Contributor Author

hop-dev commented Oct 13, 2022

@criamico I have been back and forth on the scope of this change! And I am basically back where I started. Please read the PR description for a more in depth explanation! 👍

Copy link
Contributor

@criamico criamico left a comment

Choose a reason for hiding this comment

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

Thanks for investigating further into this, it's not an easy flow.

I left a minor comment, overall LGTM 👍

// We need to get input only packages from source to get all fields
// see https://github.com/elastic/package-registry/issues/864
if (packageInfo && packageInfo.type !== 'input') {
if (registryInfo && skipArchive && registryInfo.type !== 'input') {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: isn't this the same as skipArchive && registryInfo.type !== 'input' ?

@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

@hop-dev hop-dev merged commit fbacdc6 into elastic:main Oct 13, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Oct 13, 2022
@hop-dev hop-dev deleted the input-pkg-api-tests branch October 13, 2022 10:26
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:Fleet Team label for Observability Data Collection Fleet team v8.6.0
Projects
None yet
6 participants