[Fleet] Modal to allow user to force install an unverified package #136108

hop-dev · 2022-07-11T13:56:47Z

Summary

Part of #133822

When installing a package which fails package verification, allow the user to opt to force install the package.

force_install_demo.mov

Changes

create new force install modal (needs design review from @bmorelli25)
add force install modal to the following package install locations:
- install package assets
- create package policy
- create first package policy (cloud only) a.k.a ne multi page layout
when creating a package policy, ensure the package is installed first instead of in tandem with getting the info (this makes the verification logic a lot clearer, as opposed to having a race where the getPackageInfo call also tries to verify the package)

Known issues

If the user does not have the system integration installed, and the system integration fails verification, if they go to create an agent policy for the first time and enable system monitoring, the system integration would be installed without warning.

Screenshots

Install assets

Expand for screenshot

Create package policy

Expand for screenshot

Create first package policy (cloud only)

Expand for screenshot

Test steps

TBD - will add later this evening

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)

elasticmachine · 2022-07-12T16:38:22Z

Pinging @elastic/fleet (Team:Fleet)

hop-dev · 2022-07-12T16:42:18Z

x-pack/plugins/fleet/server/services/epm/packages/install.ts

@@ -259,6 +270,7 @@ async function installPackageFromRegistry({
  spaceId,
  force = false,
  ignoreConstraints = false,
+  neverIgnoreVerificationError = false,


From an API point of view, force means "ignore version check and verification check" However internally sometimes we want to skip the version check but not the verification check. For example on install when we allow a user to install an outdated package if they dont have a leter version installed https://github.com/elastic/kibana/pull/136108/files#diff-cf9cec44de2ad0a6a3b74cca05e5308231d57d5c3e180ae4bc5114c2bf9af4ebR134

The neverIgnore naming here seems a little confusing with the kind of double negative in place. Could this be a "positive" condition defaulting to true instead to make this more clear? Maybe like surfaceVerificationErrors or forceVerificationErrors or something along those lines?

I understand the idea here is something like

force by itself will suppress verification errors and allow installing outdated package version

force with an additional flag should still allow for installing any package version, but should still surface verification errors

hop-dev · 2022-07-12T16:43:51Z

x-pack/plugins/fleet/public/applications/integrations/hooks/use_package_install.tsx

 };
-type SetPackageInstallStatusProps = Pick<PackageInfo, 'name'> & PackageInstallItem;
+type SetPackageInstallStatusProps = Pick<PackageInfo, 'name'> &
+  PackageInstallItem & { error?: FleetErrorResponse };


I have added the latest install error to the package install status in order to display the modal based on the error response.

hop-dev · 2022-07-12T16:44:34Z

x-pack/plugins/fleet/public/components/unverified_package_modal.tsx

+      : i18n.translate('xpack.fleet.unverifiedPackageModal.calloutTitleNoPkg', {
+          defaultMessage: 'The integration has failed verification',
+        });
+  // TODO: add link to docs


covered in the implementation list of the parent ticket.

hop-dev · 2022-07-13T10:39:25Z

x-pack/plugins/fleet/common/types/rest_spec/package_policy.ts

@@ -33,7 +33,7 @@ export interface GetOnePackagePolicyResponse {
 }

 export interface CreatePackagePolicyRequest {
-  body: NewPackagePolicy;
+  body: NewPackagePolicy & { force?: boolean };


force is not a new parameter, its just the typing didn't have it

x-pack/plugins/fleet/common/types/rest_spec/error.ts

nchaulet · 2022-07-13T14:10:02Z

x-pack/plugins/fleet/public/applications/integrations/hooks/use_package_install.tsx

 };
-type SetPackageInstallStatusProps = Pick<PackageInfo, 'name'> & PackageInstallItem;
+type SetPackageInstallStatusProps = Pick<PackageInfo, 'name'> &
+  PackageInstallItem & { error?: FleetErrorResponse };

 function usePackageInstall({


I am wondering if we should show the confirm modal here using the overlay service, I see the useInstallPackage is used in a few different place and for upgrade too, I think it will make consuming that hook easier. What do you think?

Yeah I'd be interested to see if we can have a "global" modal instead of rendering it in a few different places. The hook can then toggle some context value to hide/show the global modal. Similar to how some of our flyouts work. The overlay service @nchaulet mentioned is also probably a good idea, but I haven't had experience using it yet.

an example on how we use the overlay service in Fleet already, it also make testing the hook quite easy, you just have to mock that service https://github.com/hop-dev/kibana/blob/c73ea10d74cc5210dcdbae5ec4177175f49ecd94/x-pack/plugins/fleet/public/applications/fleet/sections/agent_policy/edit_package_policy_page/hooks/index.tsx#L25

I agree that looks better, I'll have a go 👍

Seems like this went mostly well. Any gotchas we should be aware of here?

...olicy/create_package_policy_page/multi_page_layout/components/page_steps/add_integration.tsx

kpollich · 2022-07-13T14:37:58Z

x-pack/plugins/fleet/public/applications/integrations/hooks/use_package_install.tsx

 };
-type SetPackageInstallStatusProps = Pick<PackageInfo, 'name'> & PackageInstallItem;
+type SetPackageInstallStatusProps = Pick<PackageInfo, 'name'> &
+  PackageInstallItem & { error?: FleetErrorResponse };

 function usePackageInstall({


Yeah I'd be interested to see if we can have a "global" modal instead of rendering it in a few different places. The hook can then toggle some context value to hide/show the global modal. Similar to how some of our flyouts work. The overlay service @nchaulet mentioned is also probably a good idea, but I haven't had experience using it yet.

kpollich · 2022-07-13T14:41:46Z

x-pack/plugins/fleet/server/services/epm/packages/install.ts

@@ -259,6 +270,7 @@ async function installPackageFromRegistry({
  spaceId,
  force = false,
  ignoreConstraints = false,
+  neverIgnoreVerificationError = false,


The neverIgnore naming here seems a little confusing with the kind of double negative in place. Could this be a "positive" condition defaulting to true instead to make this more clear? Maybe like surfaceVerificationErrors or forceVerificationErrors or something along those lines?

I understand the idea here is something like

force by itself will suppress verification errors and allow installing outdated package version

force with an additional flag should still allow for installing any package version, but should still surface verification errors

x-pack/plugins/fleet/public/components/unverified_package_modal.tsx

x-pack/plugins/fleet/public/applications/integrations/hooks/use_package_install.tsx

hop-dev · 2022-07-14T13:41:16Z

x-pack/plugins/fleet/public/applications/integrations/hooks/use_package_install.tsx

@@ -159,13 +186,14 @@ function usePackageInstall({
        }
      }
    },
+    // eslint-disable-next-line react-hooks/exhaustive-deps


The only gnarly bit about the new code layout is that there is a cyclic dependency between optionallyForceInstall and installPackage, but omitting from the deps array works

@hop-dev what about putting optionallyForceInstall the installPackage function body, as I saw this is just here to make the code more readable right?

…l.tsx Co-authored-by: Brandon Morelli <[email protected]>

nchaulet · 2022-07-20T16:22:49Z

...olicy/create_package_policy_page/multi_page_layout/components/page_steps/add_integration.tsx

+      const { error } = await savePackagePolicy({ newPackagePolicy: packagePolicy, force });
+      if (error) {
+        if (isVerificationError(error)) {
+          setFormState('CONFIRM_FAILED_VERIFICATION');


There is no way to use the useConfirmForceInstall hook in this case too?

Yeah just had a go and it works 👍

kpollich

Things LGTM overall with the code. A few nitpicks here and there, and +1 to @nchaulet 's comments but otherwise 🚀

...olicy/create_package_policy_page/multi_page_layout/components/page_steps/add_integration.tsx

...ications/fleet/sections/agent_policy/create_package_policy_page/single_page_layout/index.tsx

kpollich · 2022-07-20T18:11:02Z

x-pack/plugins/fleet/public/applications/integrations/hooks/use_package_install.tsx

 };
-type SetPackageInstallStatusProps = Pick<PackageInfo, 'name'> & PackageInstallItem;
+type SetPackageInstallStatusProps = Pick<PackageInfo, 'name'> &
+  PackageInstallItem & { error?: FleetErrorResponse };

 function usePackageInstall({


Seems like this went mostly well. Any gotchas we should be aware of here?

hop-dev · 2022-07-20T20:23:04Z

@kpollich @nchaulet I've moved to using the hook for the add integration pages in b7c998d its a lot neater 👍

nchaulet

Thanks for addressing my comments 🚀

kibana-ci · 2022-07-20T21:15:59Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`fleet`	747	749	+2

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`fleet`	864.6KB	867.4KB	+2.8KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`fleet`	113.3KB	113.5KB	+215.0B

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`fleet`	51	52	+1

Total ESLint disabled count

id	before	after	diff
`fleet`	59	60	+1

History

💚 Build #59136 succeeded 7944899
💔 Build #57744 failed 221b82d496950913ccc7c86b34308f1b6358db19
💛 Build #57671 was flaky 346f8892924242f758c2ae6f9e9959ace42ba9b8
💚 Build #57129 succeeded c73ea10d74cc5210dcdbae5ec4177175f49ecd94
💔 Build #56982 failed bd3d885f6e6d9de13b6a22bef584e51ea710b7bf
💔 Build #56964 failed 6cbdb51ca483f689fcbf0f98a8a316f1e6532ed6

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

cc @hop-dev

hop-dev force-pushed the 133822-package-verification-modal branch 2 times, most recently from bd3d885 to c73ea10 Compare July 12, 2022 16:09

hop-dev requested a review from bmorelli25 July 12, 2022 16:35

hop-dev marked this pull request as ready for review July 12, 2022 16:35

hop-dev requested a review from a team as a code owner July 12, 2022 16:35

hop-dev self-assigned this Jul 12, 2022

hop-dev added Team:Fleet Team label for Observability Data Collection Fleet team release_note:feature Makes this part of the condensed release notes v8.4.0 labels Jul 12, 2022

hop-dev commented Jul 12, 2022

View reviewed changes

hop-dev changed the title ~~[Fleet] Display confirm modal when package fails verification~~ [Fleet] Modal to allow user to force install an unverified package Jul 12, 2022

nchaulet self-requested a review July 12, 2022 18:35

hop-dev commented Jul 13, 2022

View reviewed changes

nchaulet reviewed Jul 13, 2022

View reviewed changes

x-pack/plugins/fleet/common/types/rest_spec/error.ts Outdated Show resolved Hide resolved

nchaulet reviewed Jul 13, 2022

View reviewed changes

kpollich reviewed Jul 13, 2022

View reviewed changes

bmorelli25 reviewed Jul 13, 2022

View reviewed changes

x-pack/plugins/fleet/public/components/unverified_package_modal.tsx Outdated Show resolved Hide resolved

bmorelli25 reviewed Jul 13, 2022

View reviewed changes

x-pack/plugins/fleet/public/components/unverified_package_modal.tsx Outdated Show resolved Hide resolved

hop-dev force-pushed the 133822-package-verification-modal branch from 346f889 to 221b82d Compare July 14, 2022 13:37

hop-dev commented Jul 14, 2022

View reviewed changes

x-pack/plugins/fleet/public/applications/integrations/hooks/use_package_install.tsx Outdated Show resolved Hide resolved

hop-dev commented Jul 14, 2022

View reviewed changes

hop-dev added 6 commits July 20, 2022 11:33

show unverified modal on asset install

8290933

fix modal not closing

1314902

Move modal to common components

5988daf

modify types for package policy creation

ee2d37a

add missing export

28171a0

package policy modal

7979c92

hop-dev and others added 15 commits July 20, 2022 11:33

remove refs to FleetPackageErrorResponse

858f2e9

fix: Error types

314fb14

add storybook file

2174f02

change import to fix test

12967d2

ensure package is installed before getting package info

47b00b1

Update x-pack/plugins/fleet/public/components/unverified_package_moda…

68485a4

…l.tsx Co-authored-by: Brandon Morelli <[email protected]>

Update x-pack/plugins/fleet/public/components/unverified_package_moda…

950ec9a

…l.tsx Co-authored-by: Brandon Morelli <[email protected]>

make statusCode non optional

e9011e1

use overlay service for confirm modal

48d1531

move confirm force install to its own hook

c0e52cf

rename to ConfirmForceInstallModal

2513523

fix error validation types

f070d39

return optionallyForceInstall to prevent uncaught rejection

3685850

fix edit pkg policy unit tests

6e2bfc7

fix agent list tests

f56b3dd

hop-dev force-pushed the 133822-package-verification-modal branch from 221b82d to f56b3dd Compare July 20, 2022 14:39

fix package card tests

7944899

kpollich self-requested a review July 20, 2022 15:12

nchaulet self-requested a review July 20, 2022 15:18

nchaulet reviewed Jul 20, 2022

View reviewed changes

kpollich approved these changes Jul 20, 2022

View reviewed changes

hop-dev added 2 commits July 20, 2022 21:03

use useForceConfirmForceInstall for add integration pages

b7c998d

PR Feedback

0ec8235

hop-dev requested a review from nchaulet July 20, 2022 20:23

nchaulet approved these changes Jul 20, 2022

View reviewed changes

hop-dev merged commit 184f807 into elastic:main Jul 21, 2022

kibanamachine added the backport:skip This commit does not require backporting label Jul 21, 2022

hop-dev deleted the 133822-package-verification-modal branch July 21, 2022 08:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fleet] Modal to allow user to force install an unverified package #136108

[Fleet] Modal to allow user to force install an unverified package #136108

hop-dev commented Jul 11, 2022 •

edited

Loading

elasticmachine commented Jul 12, 2022

hop-dev Jul 12, 2022

kpollich Jul 13, 2022

hop-dev Jul 12, 2022

hop-dev Jul 12, 2022

hop-dev Jul 13, 2022 •

edited

Loading

nchaulet Jul 13, 2022

kpollich Jul 13, 2022

nchaulet Jul 13, 2022

hop-dev Jul 13, 2022

kpollich Jul 20, 2022

kpollich Jul 13, 2022

kpollich Jul 13, 2022

hop-dev Jul 14, 2022

nchaulet Jul 20, 2022

nchaulet Jul 20, 2022

hop-dev Jul 20, 2022

kpollich left a comment

kpollich Jul 20, 2022

hop-dev commented Jul 20, 2022 •

edited

Loading

nchaulet left a comment

kibana-ci commented Jul 20, 2022

ESLint disabled line counts

Total ESLint disabled count

[Fleet] Modal to allow user to force install an unverified package #136108

[Fleet] Modal to allow user to force install an unverified package #136108

Conversation

hop-dev commented Jul 11, 2022 • edited Loading

Summary

Changes

Known issues

Screenshots

Install assets

Create package policy

Create first package policy (cloud only)

Test steps

Checklist

elasticmachine commented Jul 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hop-dev Jul 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kpollich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hop-dev commented Jul 20, 2022 • edited Loading

nchaulet left a comment

Choose a reason for hiding this comment

kibana-ci commented Jul 20, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

ESLint disabled line counts

Total ESLint disabled count

History

hop-dev commented Jul 11, 2022 •

edited

Loading

hop-dev Jul 13, 2022 •

edited

Loading

hop-dev commented Jul 20, 2022 •

edited

Loading