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.

Sign up for GitHub

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

Jump to bottom

[Security Solution][Endpoint] Consistent pending actions agent status label #117875

Merged
merged 9 commits into from
Nov 16, 2021
Merged

[Security Solution][Endpoint] Consistent pending actions agent status label #117875

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ed38d03
pull more than default 10 records!!
ashokaditya Nov 8, 2021
cf632cb
commit using ashokaditya@elastic.co
ashokaditya Nov 11, 2021
e3769a2
show correct isolation status badge when pending actions data is updated
ashokaditya Nov 11, 2021
3389b0c
fix typo
ashokaditya Nov 11, 2021
6b946d2
remove redundant dependencies
ashokaditya Nov 11, 2021
4f17df9
show `isolated` right after pending state is refreshed
ashokaditya Nov 11, 2021
b015338
Merge branch 'main' into fix/olm-pending_agent_status_label-2105
kibanamachine Nov 15, 2021
6058831
recompute only on prop change
ashokaditya Nov 15, 2021
0562eab
Merge branch 'main' into fix/olm-pending_agent_status_label-2105
kibanamachine Nov 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 31 additions & 19 deletions ...ution/public/common/components/endpoint/host_isolation/endpoint_host_isolation_status.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
* 2.0.
*/

import React, { memo, useMemo } from 'react';
import React, { memo, useMemo, useRef, useEffect } from 'react';
import { EuiBadge, EuiFlexGroup, EuiFlexItem, EuiTextColor, EuiToolTip } from '@elastic/eui';
import { FormattedMessage } from '@kbn/i18n/react';
import { useTestIdGenerator } from '../../../../management/components/hooks/use_test_id_generator';
Expand All @@ -28,12 +28,20 @@ export interface EndpointHostIsolationStatusProps {
export const EndpointHostIsolationStatus = memo<EndpointHostIsolationStatusProps>(
({ isIsolated, pendingIsolate = 0, pendingUnIsolate = 0, 'data-test-subj': dataTestSubj }) => {
const getTestId = useTestIdGenerator(dataTestSubj);
const isPendingStatuseDisabled = useIsExperimentalFeatureEnabled(
const isPendingStatusDisabled = useIsExperimentalFeatureEnabled(
'disableIsolationUIPendingStatuses'
);

const wasReleasing = useRef<boolean>(false);
const wasIsolating = useRef<boolean>(false);
Comment on lines +35 to +36
Copy link
Contributor

@paul-tavares paul-tavares Nov 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I'm having a hard time understanding why we need to useRef which given you a non-mutable const. Looking at this logic, it seem that these two variables are set only once - when the component renders for the very first time, and then they are never updated again. If that is truly what you mean with this change, then using useState() like this would do the trick:

Suggested change
const wasReleasing = useRef<boolean>(false);
const wasIsolating = useRef<boolean>(false);
const [wasReleasing] = useState<boolean>(pendingIsolate === 0 && pendingUnIsolate > 0);
const [wasIsolating] = useState<boolean>(pendingIsolate > 0 && pendingUnIsolate === 0);

but I have to ask: is that really what you expect? that these will never be updated even if the component is called with different props?

Also - since the entire returned value is already memoized, you might just want to move this logic inside of the useMemo()'s factory callback as:

const wasReleasing = pendingIsolate === 0 && pendingUnIsolate > 0;
const wasIsolating = pendingIsolate > 0 && pendingUnIsolate === 0;

NOTE: the reason I always pay closer attention when using useRef is that it can get you in trouble. Example: see my comment below for usage in useMemo dependencies

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, useRef returns a mutable object. Since, the component re-renders anyway when pendingIsolate and pendingUnIsolate change, passing the dependency array will have the same "effect".

Overall, the idea is when the component starts to render(before returning display nodes), we want to use the old state of these values (old isolated badge) to decide which badge to show next. The endpoint metadata doesn't update the Endpoint.state.isolation value as fast as the action response indices update the pending actions count.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right. And since you only want it to be set on first render, you don't really need a mutable object. You can just use a useState() with an initial value right (like the suggestion in my prior comment)?

I'm approving since the end result I think is the same and you and confirmed that you truly only want this value to be capture on the very first render and even if they differ in subsequent re-renders, they will not be seen.


useEffect(() => {
ashokaditya marked this conversation as resolved.
Show resolved Hide resolved
wasReleasing.current = pendingIsolate === 0 && pendingUnIsolate > 0;
wasIsolating.current = pendingIsolate > 0 && pendingUnIsolate === 0;
});

return useMemo(() => {
if (isPendingStatuseDisabled) {
if (isPendingStatusDisabled) {
// If nothing is pending and host is not currently isolated, then render nothing
if (!isIsolated) {
return null;
Expand All @@ -49,21 +57,23 @@ export const EndpointHostIsolationStatus = memo<EndpointHostIsolationStatusProps
);
}

// If nothing is pending and host is not currently isolated, then render nothing
if (!isIsolated && !pendingIsolate && !pendingUnIsolate) {
return null;
}

// If nothing is pending, but host is isolated, then show isolation badge
if (!pendingIsolate && !pendingUnIsolate) {
return (
<EuiBadge color="hollow" data-test-subj={dataTestSubj}>
<FormattedMessage
id="xpack.securitySolution.endpoint.hostIsolationStatus.isolated"
defaultMessage="Isolated"
/>
</EuiBadge>
);
// If nothing is pending
if (!(pendingIsolate || pendingUnIsolate)) {
// and host is either releasing and or currently released, then render nothing
if ((!wasIsolating.current && wasReleasing.current) || !isIsolated) {
return null;
}
// else host was isolating or is isolated, then show isolation badge
else if ((wasIsolating.current && !wasReleasing.current) || isIsolated) {
return (
<EuiBadge color="hollow" data-test-subj={dataTestSubj}>
<FormattedMessage
id="xpack.securitySolution.endpoint.hostIsolationStatus.isolated"
defaultMessage="Isolated"
/>
</EuiBadge>
);
}
}

// If there are multiple types of pending isolation actions, then show count of actions with tooltip that displays breakdown
Expand Down Expand Up @@ -136,9 +146,11 @@ export const EndpointHostIsolationStatus = memo<EndpointHostIsolationStatusProps
dataTestSubj,
getTestId,
isIsolated,
isPendingStatuseDisabled,
isPendingStatusDisabled,
pendingIsolate,
pendingUnIsolate,
wasIsolating,
wasReleasing,
ashokaditya marked this conversation as resolved.
Show resolved Hide resolved
]);
}
);
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/security_solution/server/endpoint/services/actions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,7 @@ const hasEndpointResponseDoc = async ({
.search<LogsEndpointActionResponse>(
{
index: ENDPOINT_ACTION_RESPONSES_INDEX,
size: 10000,
body: {
query: {
bool: {
Expand Down