[Security Solution][Endpoint][Guided onboarding] New extension point with the endpoint UI #142395

gergoabraham · 2022-09-30T17:29:49Z

Summary

Added new UI for onboarding users through setting up Elastic Defend.

Screenshots:

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
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

elasticmachine · 2022-09-30T17:32:27Z

Pinging @elastic/security-onboarding-and-lifecycle-mgt (Team:Onboarding and Lifecycle Mgt)

gergoabraham · 2022-10-03T09:26:59Z

@elasticmachine merge upstream

…n-point-with-the-endpoint-ui

elasticmachine · 2022-10-03T09:27:14Z

Pinging @elastic/fleet (Team:Fleet)

dasansol92

This is looking great! 🔥 Left some questions/suggestions, let me know if anything is not clear or needs more explanation. Happy to discuss about those 🙂

dasansol92 · 2022-10-03T09:49:00Z

...pages/policy/view/ingest_manager_integration/endpoint_policy_create_multi_step_extension.tsx

+      Read more about Endpoint security configuration in our {docsPage}."
+        values={{
+          docsPage: (
+            <EuiLink href="https://www.elastic.co/guide/en/security/master/configure-endpoint-integration-policy.html">


You have to use kibana docs service here. This will show the link with the stack version. You can check how it works here . You will also have to update kibana docs types to add the new one if it doesn't exists yet.

dasansol92 · 2022-10-03T09:50:12Z

x-pack/plugins/fleet/public/types/ui_extensions.ts

-  /** The integration policy being created */
-  newPolicy: NewPackagePolicy;
-}
+export type PackagePolicyCreateMultiStepExtensionComponent = ComponentType<{}>;


I would not remove these props as could be used in the future (or might be used for other integrations) when we add any kind of integration modification action.

As we discussed, I'd leave them removed to avoid unneeded complexity, and put them back when they are really needed.

dasansol92 · 2022-10-03T09:51:14Z

...pages/policy/view/ingest_manager_integration/endpoint_policy_create_multi_step_extension.tsx

+);
+
+const LargeSecurityLogo = () => (
+  <EuiIcon type="logoSecurity" style={{ width: '128px', height: '128px' }} />


suggestion: create a styled component instead so we don't have inline css styles defined.

dasansol92 · 2022-10-03T09:53:13Z

...pages/policy/view/ingest_manager_integration/endpoint_policy_create_multi_step_extension.tsx

-  memo<PackagePolicyCreateMultiStepExtensionComponentProps>(({ newPolicy }) => {
-    return <></>;
-  });
+export const EndpointPolicyCreateMultiStepExtension = memo(() => {


Can you move the final component at the end so we see first all the defined consts and finally the component that uses those?

Also, for functions that returns a component or JSX code I think it would be better to define those inside the component so all are rendered on component render and not on module import level. What do you think?

Can you move the final component at the end so we see first all the defined consts and finally the component that uses those?

Personally I like it better when the higher level components/functions are on the top of the file, so when one opens it, they can see the high level logic first, and if they want to see the details, they can go down in the file. But I see that your suggestion is the general style in our codebase, so of course! It's better to have an aligned code style.

Also, for functions that returns a component or JSX code I think it would be better to define those inside the component so all are rendered on component render and not on module import level. What do you think?

Great idea! Usually I'd keep small components out of the main component, so the functional components are not re-created on every re-render, but in this case with a static component, I agree that favouring a faster module load is probably better.

dasansol92 · 2022-10-03T09:57:46Z

...pages/policy/view/ingest_manager_integration/endpoint_policy_create_multi_step_extension.tsx

 EndpointPolicyCreateMultiStepExtension.displayName = 'EndpointPolicyCreateMultiStepExtension';
+
+const Container = styled('div')`


Can we avoid this using an EUI component with some spacing? It seems too much specific

ashokaditya

This is looking good, but I belive we can simplifiy the code further if we use CSS padding and margins instead of using EuiSpacer.

ashokaditya · 2022-10-03T14:13:19Z

...pages/policy/view/ingest_manager_integration/endpoint_policy_create_multi_step_extension.tsx

+const LargeCustomSpacer = () => (
+  <>
+    <EuiSpacer size="m" />
+    <EuiSpacer size="xxl" />
+  </>
+);
+
+const MediumCustomSpacer = () => (
+  <>
+    <EuiSpacer size="m" />
+    <EuiSpacer size="xl" />
+  </>
+);


Please merge these two separate components into a single one called CustomSpacer that takes sizes as an array and then spits out EuiSpacers based on that. Then you can just use something like <CustomSpacer spacers={['m', 'xxl']} /> or <CustomSpacer spacers={['m', 'xl']} /> where needed.

ashokaditya · 2022-10-03T14:20:07Z

...pages/policy/view/ingest_manager_integration/endpoint_policy_create_multi_step_extension.tsx

 EndpointPolicyCreateMultiStepExtension.displayName = 'EndpointPolicyCreateMultiStepExtension';
+
+const Container = styled('div')`
+  padding: 0 23px;


Yeah, why is this exact padding needed instead of using the Eui token values for padding? Also looks like either the Container should have padding that is equal to the LargeCustomSpacer instead of the custom spacer. Also, the EuiFlexGroup on line 23 should have a margin that is equal to MediumCustomSpacer instead.

dasansol92

Looks great! Left two more suggestions but this is ready 🔥 Thanks for applying all the changes!

dasansol92 · 2022-10-04T10:07:06Z

...pages/policy/view/ingest_manager_integration/endpoint_policy_create_multi_step_extension.tsx

+`;
+
+const LargeLogo = styled(EuiIcon)`
+  width: 128px;


Can we use any UI token here too?

dasansol92 · 2022-10-04T10:08:23Z

...pages/policy/view/ingest_manager_integration/endpoint_policy_create_multi_step_extension.tsx

+      Read more about Endpoint security configuration in our {docsPage}."
+          values={{
+            docsPage: (
+              <EuiLink href={docLinks.links.securitySolution.configureEndpointIntegrationPolicy}>


Wondering if we should open the link in a new tab:

<EuiLink href={docLinks.links.securitySolution.configureEndpointIntegrationPolicy} target="_blank" external> [...] </EuiLink>

cc: @caitlinbetz @kevinlog

Yes, let's open on a new tab so that it doesn't take the user from the onboarding flow

gergoabraham · 2022-10-04T11:16:01Z

@elasticmachine merge upstream

…n-point-with-the-endpoint-ui

dasansol92

🔥 🔥 🔥

ashokaditya

Looking good to 🚢

ashokaditya · 2022-10-04T14:26:57Z

...pages/policy/view/ingest_manager_integration/endpoint_policy_create_multi_step_extension.tsx

+          margin-bottom: ${marginSize};
+          margin-top: ${marginSize};


Should be padding and not margin. Conceptually, you're padding out the content so that there's spacing around it.

Cool, thanks! It also makes things easier, that's for sure.
50d0017

ashokaditya · 2022-10-04T14:30:40Z

...pages/policy/view/ingest_manager_integration/endpoint_policy_create_multi_step_extension.tsx

+          margin-top: ${marginSize};
+        `}
+      >
+        <CenteredEuiFlexItem grow={false}>{securityLogo}</CenteredEuiFlexItem>


I don't think you need this additional center alignment. You can just use the EuiFlexItem.

For the given design, you are right! The center alignment helps with the mobile view.

kellyemurphy · 2022-10-04T15:39:31Z

...pages/policy/view/ingest_manager_integration/endpoint_policy_create_multi_step_extension.tsx

+      <p>
+        <FormattedMessage
+          id="xpack.securitySolution.endpoint.policy.multiStepOnboarding.details"
+          defaultMessage="You can change this later by editing the Endpoint Security integration agent policy.


This is Janeen. Edit to @kellyemurphy 's suggestion because we updated the name to Elastic Defend in 8.5.

Suggested change

defaultMessage="You can change this later by editing the Endpoint Security integration agent policy.

defaultMessage="You can edit these settings later in the Elastic Defend integration policy.

kellyemurphy · 2022-10-04T15:40:30Z

...pages/policy/view/ingest_manager_integration/endpoint_policy_create_multi_step_extension.tsx

+        <FormattedMessage
+          id="xpack.securitySolution.endpoint.policy.multiStepOnboarding.details"
+          defaultMessage="You can change this later by editing the Endpoint Security integration agent policy.
+      Read more about Endpoint security configuration in our {docsPage}."


We can shorten this entire sentence to just "Learn more" that links to that doc page and a jumpout icon.

jmikell821

Added a comment for language. @joepeeples can you make sure we don't need to edit language anywhere else in this PR? Thanks!

gchaps

doclink is good.

joepeeples

One small nit about "macOS" vs. "Mac", otherwise LGTM!

joepeeples · 2022-10-04T17:00:08Z

...pages/policy/view/ingest_manager_integration/endpoint_policy_create_multi_step_extension.tsx

+          <p>
+            <FormattedMessage
+              id="xpack.securitySolution.endpoint.policy.multiStepOnboarding.feature"
+              defaultMessage="Windows, Mac, and Linux event collection"


Nit: "macOS" (capitalized as such) is the proper name of the operating system, and Apple usually uses "Mac" to refer to a piece of hardware

Suggested change

defaultMessage="Windows, Mac, and Linux event collection"

defaultMessage="Windows, macOS, and Linux event collection"

Great catch!

benironside

The UI copy looks good to me, once the other reviewers' comments have been included. Only other thing I would suggest considering is to make the header "Set up an Elastic Defend integration" (the current version doesn't have an "an"). Not crucial but I think it would be slightly more accurate.

gergoabraham · 2022-10-05T09:10:18Z

@kellyemurphy @jmikell821 @joepeeples
Thanks a lot for the feedback! What do you think of @benironside's suggestion? #142395 (review)

Now the UI looks like this:

ead759e

gergoabraham · 2022-10-05T11:40:24Z

@elasticmachine merge upstream

…n-point-with-the-endpoint-ui

…ocs'

joepeeples · 2022-10-05T13:27:49Z

What do you think of @benironside's suggestion? #142395 (review)

I agree, the heading needs an article. I might lean toward "Set up the Elastic Defend integration", but "an" probably works too.

kibana-ci · 2022-10-05T13:31:41Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: a5f7c9b

Failed CI Steps

Security Solution Tests #3

Test Failures

[job] [logs] Security Solution Tests #3 / All hosts table it renders risk column

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`securitySolution`	3148	3161	+13

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`fleet`	894	893	-1

Async chunks

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

id	before	after	diff
`fleet`	911.6KB	911.6KB	-11.0B
`securitySolution`	6.6MB	6.6MB	+8.5KB
total			+8.5KB

Page load bundle

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

id	before	after	diff
`core`	367.2KB	367.2KB	+93.0B
`securitySolution`	264.5KB	264.5KB	+56.0B
total			+149.0B

Unknown metric groups

API count

id	before	after	diff
`fleet`	998	996	-2

History

💔 Build #78313 failed 4c85a0d
💔 Build #78240 failed ead759e
💚 Build #77892 succeeded 42ce7b0
💚 Build #77852 succeeded c53eed7
💛 Build #77414 was flaky 55d7737
💛 Build #77253 was flaky 76cca8a

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

cc @gergoabraham

kellyemurphy · 2022-10-05T14:03:19Z

What do you think of @benironside's suggestion? #142395 (review)

I agree, the heading needs an article. I might lean toward "Set up the Elastic Defend integration", but "an" probably works too.

@joepeeples @benironside Would there ever be more than one? If it's singular, then "the" makes sense. If there could be multiple, then "an" is ok.

nchaulet

Fleet changes looks good to me

gergoabraham · 2022-10-06T09:11:45Z

@joepeeples @benironside Would there ever be more than one? If it's singular, then "the" makes sense. If there could be multiple, then "an" is ok.

@kellyemurphy I merge the current modification, because modifying the title is rather related to the overall onboarding flow, than the UI in this issue. Do you have an Epic/workflow to discuss the title change? In case not, please let me know if you would like me to create an issue for that change.

…with the endpoint UI (elastic#142395) * Add new UI for extension for onboarding * Remove unnecessary props * Improve layout for mobile browsers * use Kibana Docs service for documentation url * use styled components instead of inline style * move small components above/in the main component * use Eui components and tokens instead of custom ones * use Eui sizing tokens for logo size * open documentation link on new tab * use padding instead of margin * update texts * [CI] Auto-commit changed files from 'node scripts/build_plugin_list_docs' Co-authored-by: Kibana Machine <[email protected]>

gergoabraham added 3 commits September 30, 2022 19:14

Add new UI for extension for onboarding

6ea9956

Remove unnecessary props

9eb4ed0

Improve layout for mobile browsers

76cca8a

gergoabraham changed the title ~~[Security Solution][Endpoint] Guided onboarding: new extension point with the endpoint UI~~ [Security Solution][Endpoint][Guided onboarding] New extension point with the endpoint UI Sep 30, 2022

gergoabraham added Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v8.6.0 release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting labels Sep 30, 2022

gergoabraham marked this pull request as ready for review September 30, 2022 17:32

gergoabraham requested review from a team as code owners September 30, 2022 17:32

gergoabraham requested review from ashokaditya and dasansol92 September 30, 2022 17:32

gergoabraham self-assigned this Sep 30, 2022

gergoabraham added the OLM Sprint label Sep 30, 2022

Merge branch 'main' into feat/olm-5030-guided-onboarding-new-extensio…

55d7737

…n-point-with-the-endpoint-ui

botelastic bot added the Team:Fleet Team label for Observability Data Collection Fleet team label Oct 3, 2022

dasansol92 reviewed Oct 3, 2022

View reviewed changes

gergoabraham added 2 commits October 3, 2022 16:11

use Kibana Docs service for documentation url

209dde3

use styled components instead of inline style

c2a459a

ashokaditya reviewed Oct 3, 2022

View reviewed changes

gergoabraham added 2 commits October 3, 2022 17:08

move small components above/in the main component

6f440c6

use Eui components and tokens instead of custom ones

c53eed7

gergoabraham requested a review from a team as a code owner October 4, 2022 09:58

gergoabraham requested review from dasansol92 and ashokaditya October 4, 2022 10:04

dasansol92 reviewed Oct 4, 2022

View reviewed changes

kibanamachine and others added 3 commits October 4, 2022 05:16

Merge branch 'main' into feat/olm-5030-guided-onboarding-new-extensio…

54fc2b6

…n-point-with-the-endpoint-ui

use Eui sizing tokens for logo size

aa93233

open documentation link on new tab

42ce7b0

gergoabraham requested a review from dasansol92 October 4, 2022 12:20

dasansol92 approved these changes Oct 4, 2022

View reviewed changes

ashokaditya approved these changes Oct 4, 2022

View reviewed changes

kellyemurphy reviewed Oct 4, 2022

View reviewed changes

jmikell821 reviewed Oct 4, 2022

View reviewed changes

gchaps approved these changes Oct 4, 2022

View reviewed changes

joepeeples reviewed Oct 4, 2022

View reviewed changes

benironside reviewed Oct 4, 2022

View reviewed changes

gergoabraham added 2 commits October 5, 2022 10:33

use padding instead of margin

50d0017

update texts

ead759e

kibanamachine added 2 commits October 5, 2022 05:40

Merge branch 'main' into feat/olm-5030-guided-onboarding-new-extensio…

4c85a0d

…n-point-with-the-endpoint-ui

[CI] Auto-commit changed files from 'node scripts/build_plugin_list_d…

a5f7c9b

…ocs'

nchaulet approved these changes Oct 5, 2022

View reviewed changes

gergoabraham merged commit 46d87da into elastic:main Oct 6, 2022

gergoabraham deleted the feat/olm-5030-guided-onboarding-new-extension-point-with-the-endpoint-ui branch October 6, 2022 09:12

		EndpointPolicyCreateMultiStepExtension.displayName = 'EndpointPolicyCreateMultiStepExtension';

		const Container = styled('div')`

	defaultMessage="You can change this later by editing the Endpoint Security integration agent policy.
	defaultMessage="You can edit these settings later in the Elastic Defend integration policy.

	defaultMessage="Windows, Mac, and Linux event collection"
	defaultMessage="Windows, macOS, and Linux event collection"

[Security Solution][Endpoint][Guided onboarding] New extension point with the endpoint UI #142395

[Security Solution][Endpoint][Guided onboarding] New extension point with the endpoint UI #142395

Conversation

gergoabraham commented Sep 30, 2022 • edited Loading

Summary

Checklist

elasticmachine commented Sep 30, 2022

gergoabraham commented Oct 3, 2022

elasticmachine commented Oct 3, 2022

dasansol92 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gergoabraham Oct 4, 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

ashokaditya left a 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

dasansol92 left a 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

gergoabraham commented Oct 4, 2022

dasansol92 left a comment

Choose a reason for hiding this comment

ashokaditya left a 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

kellyemurphy Oct 4, 2022 • edited by jmikell821 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmikell821 left a comment

Choose a reason for hiding this comment

gchaps left a comment

Choose a reason for hiding this comment

joepeeples left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benironside left a comment

Choose a reason for hiding this comment

gergoabraham commented Oct 5, 2022

gergoabraham commented Oct 5, 2022

joepeeples commented Oct 5, 2022

kibana-ci commented Oct 5, 2022

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Page load bundle

API count

History

kellyemurphy commented Oct 5, 2022 • edited Loading

nchaulet left a comment

Choose a reason for hiding this comment

gergoabraham commented Oct 6, 2022

gergoabraham commented Sep 30, 2022 •

edited

Loading

gergoabraham Oct 4, 2022 •

edited

Loading

kellyemurphy Oct 4, 2022 •

edited by jmikell821

Loading

kellyemurphy commented Oct 5, 2022 •

edited

Loading