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

[Security Solution][Endpoint] New endpoint policy response UI and fleet UI for integrations in agent details page #133405

Conversation

dasansol92
Copy link
Contributor

@dasansol92 dasansol92 commented Jun 2, 2022

Summary

  • New endpoint policy response UI.
  • New UI for integrations in fleet agent details page.
  • Adds Needs attention button label when it's needed.
  • Endpoint policy response in fleet is behind a FF: policyResponseInFleetEnabled

Fleet changes without FF:

policy response new UI with FF disabled

Fleet changes with FF:

policy response new UI with FF enabled

Endpoint policy response changes in endpoint details flyout:

policy response new UI in endpoint details

For maintainers

@dasansol92 dasansol92 added release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v8.4.0 labels Jun 2, 2022
Copy link
Member

@ashokaditya ashokaditya left a comment

Choose a reason for hiding this comment

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

This is looking awesome, so much better than what we have now 🤩 I have a couple of suggestions and mostly nits.

Also, I suggested this to @joeypoon as well on his PR. Is there a way to expand all the error item logs when you click on the needsAttention button? I think it's useful to see all the errors with a single click vs drilling down individually. Currently, it expands only the top-level accordion as seen in the PR screenshot. How about something like this below. I took a screen grab as the state persists once you drill down to inner levels.

expand

Maybe out of scope or maybe we can do this in a new PR. Maybe we don't want to do it. Let me know what you think.

x-pack/plugins/fleet/public/types/ui_extensions.ts Outdated Show resolved Hide resolved
Comment on lines 148 to 178
// FIXME: for some reason it is not getting all messages items from DOM even those are rendered.
it.skip('should show an actions section for each configuration', async () => {
runMock();
const actionAccordions = await render().findAllByTestId(
'endpointDetailsPolicyResponseActionsAccordion'
const component = await renderOpenedTree();

const configs = await component.findAllByTestId('endpointPolicyResponseConfig');
const actions = await component.findAllByTestId('endpointPolicyResponseAction');
const statusAttentionHealth = await component.findAllByTestId(
'endpointPolicyResponseStatusAttentionHealth'
);
const action = await render().findAllByTestId('policyResponseAction');
const statusHealth = await render().findAllByTestId('policyResponseStatusHealth');
const message = await render().findAllByTestId('policyResponseMessage');
const statusSuccessHealth = await component.findAllByTestId(
'endpointPolicyResponseStatusSuccessHealth'
);
const messages = await component.findAllByTestId('endpointPolicyResponseMessage');

let expectedActionAccordionCount = 0;
Object.keys(commonPolicyResponse.Endpoint.policy.applied.response.configurations).forEach(
(key) => {
expectedActionAccordionCount +=
commonPolicyResponse.Endpoint.policy.applied.response.configurations[
key as keyof HostPolicyResponse['Endpoint']['policy']['applied']['response']['configurations']
].concerned_actions.length;
}
const configurationKeys = Object.keys(
commonPolicyResponse.Endpoint.policy.applied.response.configurations
);
configurationKeys.forEach((key) => {
expectedActionAccordionCount +=
commonPolicyResponse.Endpoint.policy.applied.response.configurations[
key as keyof HostPolicyResponse['Endpoint']['policy']['applied']['response']['configurations']
].concerned_actions.length;
});

expect(configs).toHaveLength(configurationKeys.length);
expect(actions).toHaveLength(expectedActionAccordionCount);
expect(messages).toHaveLength(expectedActionAccordionCount);
expect([...statusSuccessHealth, ...statusAttentionHealth]).toHaveLength(
expectedActionAccordionCount + configurationKeys.length + 1
Copy link
Member

Choose a reason for hiding this comment

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

Looks like one of the endpointPolicyResponseStatusAttentionHealth may not be expanded here and that's why the count is off by 1. 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not exactly, the +1 here is for the top one Policy response. I'm still looking on this test

@dasansol92
Copy link
Contributor Author

Also, I suggested this to @joeypoon as well on his PR. Is there a way to expand all the error item logs when you click on the needsAttention button? I think it's useful to see all the errors with a single click vs drilling down individually. Currently, it expands only the top-level accordion as seen in the PR screenshot. How about something like this below. I took a screen grab as the state persists once you drill down to inner levels.

This is not possible right now because an EuiTreeView limitation. Items can be expanded only at the first render. Spoke with @kevinlog and we are ok with this by now.

@dasansol92 dasansol92 requested a review from ashokaditya June 3, 2022 09:18
Copy link
Member

@ashokaditya ashokaditya 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 the changes. 🙏 There's a color bit that needs a change.

Comment on lines +125 to +126
children: packagePolicy.inputs.reduce(
(acc: Array<{ label: JSX.Element; id: string }>, current) => {
Copy link
Member

Choose a reason for hiding this comment

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

Nice 🔥 . No need to change but for TS you can also do .reduce<Array<{ label: JSX.Element; id: string }>>(acc, current).

@dasansol92 dasansol92 requested a review from ashokaditya June 3, 2022 11:11
Copy link
Member

@ashokaditya ashokaditya left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks for the changes again. Looking good to 🚢!

@dasansol92 dasansol92 marked this pull request as ready for review June 3, 2022 12:39
@dasansol92 dasansol92 requested review from a team as code owners June 3, 2022 12:39
@dasansol92 dasansol92 requested a review from pzl June 3, 2022 12:39
@elasticmachine
Copy link
Contributor

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

@dasansol92 dasansol92 requested a review from joeypoon June 3, 2022 12:39
@dasansol92
Copy link
Contributor Author

@elasticmachine merge upstream

@botelastic botelastic bot added the Team:Fleet Team label for Observability Data Collection Fleet team label Jun 3, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

Comment on lines +122 to +123
// actionButtonLabel="Do something" // TODO
// actionButtonOnClick={() => {}} // TODO
Copy link
Member

Choose a reason for hiding this comment

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

??

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 is going to be used in future pr's but is useless at this point. I can remove those comments if can create any confusion

Copy link
Member

Choose a reason for hiding this comment

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

fine to leave it in

Comment on lines +173 to +177
// FIXME: for some reason it is not getting all messages items from DOM even those are rendered.
// expect(messages).toHaveLength(expectedActionAccordionCount);
// expect([...statusSuccessHealth, ...statusAttentionHealth]).toHaveLength(
// expectedActionAccordionCount + configurationKeys.length + 1
// );
Copy link
Member

Choose a reason for hiding this comment

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

👌

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.

Fleet changes LGTM, could you actually see any log entries under one of the View logs links?

@dasansol92
Copy link
Contributor Author

dasansol92 commented Jun 3, 2022

Fleet changes LGTM, could you actually see any log entries under one of the View logs links?

@juliaElastic Thanks for taking a look! I'm using data generated by the generators. Do you have any generator I can use for that?

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 3013 3014 +1

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 1299 1300 +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 750.1KB 750.7KB +645.0B
securitySolution 5.1MB 5.1MB +816.0B
total +1.4KB
Unknown metric groups

API count

id before after diff
fleet 1425 1427 +2

History

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

@juliaElastic
Copy link
Contributor

Fleet changes LGTM, could you actually see any log entries under one of the View logs links?

@juliaElastic Thanks for taking a look! I'm using data generated by the generators. Do you have any generator I can use for that?

I'm not familiar with generators, do we have documentation on them?
Usually I test system logs from an elastic-agent installed locally.

Copy link
Member

@joeypoon joeypoon left a comment

Choose a reason for hiding this comment

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

🚀

@kevinlog
Copy link
Contributor

kevinlog commented Jun 3, 2022

@dasansol92 check it out and tried it with a real Agent/Endpoint - looks great!

image

I also verified @juliaElastic question - we can see logs when we navigate with real data:
image

image

@kevinlog kevinlog merged commit a20cb49 into elastic:main Jun 3, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jun 3, 2022
@kevinlog
Copy link
Contributor

kevinlog commented Jun 3, 2022

Merged with @dasansol92 permission

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:Defend Workflows “EDR Workflows” sub-team of Security Solution Team:Fleet Team label for Observability Data Collection Fleet team v8.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants