From 30d5ed55304327eccff9167d9c4665196a11009e Mon Sep 17 00:00:00 2001 From: Julia Bardi <90178898+juliaElastic@users.noreply.github.com> Date: Fri, 23 Sep 2022 08:57:35 +0200 Subject: [PATCH] [Fleet] Bulk update agent tags improvements (#141376) * update tags: added action doc and results, updating selected list immediately * fix tests, fix tag rename and delete * added unit test --- .../fleet/common/types/models/agent.ts | 3 +- .../components/agent_activity_flyout.tsx | 7 ++- .../components/tag_options.tsx | 6 +- .../components/tags_add_remove.test.tsx | 59 +++++++++++++++++++ .../components/tags_add_remove.tsx | 42 +++++++++---- .../hooks/use_update_tags.test.tsx | 5 +- .../agent_list_page/hooks/use_update_tags.tsx | 7 ++- .../services/agents/unenroll_action_runner.ts | 2 +- .../services/agents/update_agent_tags.test.ts | 26 ++++++++ .../agents/update_agent_tags_action_runner.ts | 52 +++++++++++++++- 10 files changed, 186 insertions(+), 23 deletions(-) diff --git a/x-pack/plugins/fleet/common/types/models/agent.ts b/x-pack/plugins/fleet/common/types/models/agent.ts index 809e32c78c1b8..9ca69b5100625 100644 --- a/x-pack/plugins/fleet/common/types/models/agent.ts +++ b/x-pack/plugins/fleet/common/types/models/agent.ts @@ -36,7 +36,8 @@ export type AgentActionType = | 'SETTINGS' | 'POLICY_REASSIGN' | 'CANCEL' - | 'FORCE_UNENROLL'; + | 'FORCE_UNENROLL' + | 'UPDATE_TAGS'; type FleetServerAgentComponentStatusTuple = typeof FleetServerAgentComponentStatuses; export type FleetServerAgentComponentStatus = FleetServerAgentComponentStatusTuple[number]; diff --git a/x-pack/plugins/fleet/public/applications/fleet/sections/agents/agent_list_page/components/agent_activity_flyout.tsx b/x-pack/plugins/fleet/public/applications/fleet/sections/agents/agent_list_page/components/agent_activity_flyout.tsx index a2fdf6d145d55..89da21963092e 100644 --- a/x-pack/plugins/fleet/public/applications/fleet/sections/agents/agent_list_page/components/agent_activity_flyout.tsx +++ b/x-pack/plugins/fleet/public/applications/fleet/sections/agents/agent_list_page/components/agent_activity_flyout.tsx @@ -243,6 +243,11 @@ const actionNames: { completedText: 'force unenrolled', cancelledText: 'force unenrollment', }, + UPDATE_TAGS: { + inProgressText: 'Updating tags of', + completedText: 'updated tags', + cancelledText: 'update tags', + }, CANCEL: { inProgressText: 'Cancelling', completedText: 'cancelled', cancelledText: '' }, ACTION: { inProgressText: 'Actioning', completedText: 'actioned', cancelledText: 'action' }, }; @@ -362,7 +367,7 @@ const ActivityItem: React.FunctionComponent<{ action: ActionStatus }> = ({ actio

  {inProgressDescription(action.creationTime)} diff --git a/x-pack/plugins/fleet/public/applications/fleet/sections/agents/agent_list_page/components/tag_options.tsx b/x-pack/plugins/fleet/public/applications/fleet/sections/agents/agent_list_page/components/tag_options.tsx index 7e25a7a098dba..eaad48b6f58f5 100644 --- a/x-pack/plugins/fleet/public/applications/fleet/sections/agents/agent_list_page/components/tag_options.tsx +++ b/x-pack/plugins/fleet/public/applications/fleet/sections/agents/agent_list_page/components/tag_options.tsx @@ -25,7 +25,7 @@ import { sanitizeTag } from '../utils'; interface Props { tagName: string; isTagHovered: boolean; - onTagsUpdated: () => void; + onTagsUpdated: (tagsToAdd: string[], tagsToRemove: string[], hasCompleted?: boolean) => void; } export const TagOptions: React.FC = ({ tagName, isTagHovered, onTagsUpdated }: Props) => { @@ -65,7 +65,7 @@ export const TagOptions: React.FC = ({ tagName, isTagHovered, onTagsUpdat kuery, [newName], [tagName], - () => onTagsUpdated(), + (hasCompleted) => onTagsUpdated([newName], [tagName], hasCompleted), i18n.translate('xpack.fleet.renameAgentTags.successNotificationTitle', { defaultMessage: 'Tag renamed', }), @@ -81,7 +81,7 @@ export const TagOptions: React.FC = ({ tagName, isTagHovered, onTagsUpdat kuery, [], [tagName], - () => onTagsUpdated(), + (hasCompleted) => onTagsUpdated([], [tagName], hasCompleted), i18n.translate('xpack.fleet.deleteAgentTags.successNotificationTitle', { defaultMessage: 'Tag deleted', }), diff --git a/x-pack/plugins/fleet/public/applications/fleet/sections/agents/agent_list_page/components/tags_add_remove.test.tsx b/x-pack/plugins/fleet/public/applications/fleet/sections/agents/agent_list_page/components/tags_add_remove.test.tsx index 69c6f3c12823e..8c4f9f3003c82 100644 --- a/x-pack/plugins/fleet/public/applications/fleet/sections/agents/agent_list_page/components/tags_add_remove.test.tsx +++ b/x-pack/plugins/fleet/public/applications/fleet/sections/agents/agent_list_page/components/tags_add_remove.test.tsx @@ -226,6 +226,30 @@ describe('TagsAddRemove', () => { ); }); + it('should add to selected tags on add if action not completed immediately', async () => { + mockBulkUpdateTags.mockImplementation((agents, tagsToAdd, tagsToRemove, onSuccess) => { + onSuccess(false); + }); + const result = renderComponent(undefined, ''); + const getTag = (name: string) => result.getByText(name); + + fireEvent.click(getTag('tag2')); + + expect(result.getByTitle('tag2').getAttribute('aria-checked')).toEqual('true'); + }); + + it('should remove from selected tags on remove if action not completed immediately', async () => { + mockBulkUpdateTags.mockImplementation((agents, tagsToAdd, tagsToRemove, onSuccess) => { + onSuccess(false); + }); + const result = renderComponent(undefined, ''); + const getTag = (name: string) => result.getByText(name); + + fireEvent.click(getTag('tag1')); + + expect(result.getByTitle('tag1').getAttribute('aria-checked')).toEqual('false'); + }); + it('should add new tag when not found in search and button clicked - bulk selection', () => { const result = renderComponent(undefined, 'query'); const searchInput = result.getByRole('combobox'); @@ -257,4 +281,39 @@ describe('TagsAddRemove', () => { expect(result.queryByRole('button')).not.toBeInTheDocument(); }); + + it('should remove from selected and all tags on delete if action not completed immediately', async () => { + mockBulkUpdateTags.mockImplementation((agents, tagsToAdd, tagsToRemove, onSuccess) => { + onSuccess(false); + }); + const result = renderComponent('agent1'); + fireEvent.mouseEnter(result.getByText('tag1').closest('.euiFlexGroup')!); + + fireEvent.click(result.getByRole('button')); + + fireEvent.click(result.getByText('Delete tag').closest('button')!); + + expect(result.queryByTitle('tag1')).toBeNull(); + }); + + it('should update selected and all tags on rename if action not completed immediately', async () => { + mockBulkUpdateTags.mockImplementation((agents, tagsToAdd, tagsToRemove, onSuccess) => { + onSuccess(false); + }); + const result = renderComponent('agent1'); + fireEvent.mouseEnter(result.getByText('tag1').closest('.euiFlexGroup')!); + + fireEvent.click(result.getByRole('button')); + + const input = result.getByDisplayValue('tag1'); + fireEvent.input(input, { + target: { value: 'newName' }, + }); + fireEvent.keyDown(input, { + key: 'Enter', + }); + + expect(result.queryByTitle('tag1')).toBeNull(); + expect(result.getByText('newName')).toBeInTheDocument(); + }); }); diff --git a/x-pack/plugins/fleet/public/applications/fleet/sections/agents/agent_list_page/components/tags_add_remove.tsx b/x-pack/plugins/fleet/public/applications/fleet/sections/agents/agent_list_page/components/tags_add_remove.tsx index ccbb50d3eec71..a03ec3808e9ab 100644 --- a/x-pack/plugins/fleet/public/applications/fleet/sections/agents/agent_list_page/components/tags_add_remove.tsx +++ b/x-pack/plugins/fleet/public/applications/fleet/sections/agents/agent_list_page/components/tags_add_remove.tsx @@ -5,7 +5,7 @@ * 2.0. */ -import React, { Fragment, useCallback, useEffect, useState, useMemo } from 'react'; +import React, { Fragment, useEffect, useState, useMemo, useCallback } from 'react'; import { difference } from 'lodash'; import styled from 'styled-components'; import type { EuiSelectableOption } from '@elastic/eui'; @@ -54,16 +54,18 @@ export const TagsAddRemove: React.FC = ({ onClosePopover, }: Props) => { const labelsFromTags = useCallback( - (tags: string[]) => + (tags: string[], selected: string[]) => tags.map((tag: string) => ({ label: tag, - checked: selectedTags.includes(tag) ? 'on' : undefined, + checked: selected.includes(tag) ? 'on' : undefined, onFocusBadge: false, })), - [selectedTags] + [] ); - const [labels, setLabels] = useState>>(labelsFromTags(allTags)); + const [labels, setLabels] = useState>>( + labelsFromTags(allTags, selectedTags) + ); const [searchValue, setSearchValue] = useState(undefined); const [isPopoverOpen, setIsPopoverOpen] = useState(true); const [isTagHovered, setIsTagHovered] = useState<{ [tagName: string]: boolean }>({}); @@ -76,24 +78,42 @@ export const TagsAddRemove: React.FC = ({ // update labels after tags changing useEffect(() => { - setLabels(labelsFromTags(allTags)); - }, [allTags, labelsFromTags]); + setLabels(labelsFromTags(allTags, selectedTags)); + }, [allTags, labelsFromTags, selectedTags]); const isExactMatch = useMemo( () => labels.some((label) => label.label === searchValue), [labels, searchValue] ); + const handleTagsUpdated = ( + tagsToAdd: string[], + tagsToRemove: string[], + hasCompleted: boolean = true, + isRenameOrDelete = false + ) => { + if (hasCompleted) { + return onTagsUpdated(); + } + const newSelectedTags = difference(selectedTags, tagsToRemove).concat(tagsToAdd); + const allTagsWithNew = allTags.includes(tagsToAdd[0]) ? allTags : allTags.concat(tagsToAdd); + const allTagsWithRemove = isRenameOrDelete + ? difference(allTagsWithNew, tagsToRemove) + : allTagsWithNew; + setLabels(labelsFromTags(allTagsWithRemove, newSelectedTags)); + }; + const updateTags = async ( tagsToAdd: string[], tagsToRemove: string[], successMessage?: string, errorMessage?: string ) => { + const newSelectedTags = difference(selectedTags, tagsToRemove).concat(tagsToAdd); if (agentId) { updateTagsHook.updateTags( agentId, - difference(selectedTags, tagsToRemove).concat(tagsToAdd), + newSelectedTags, () => onTagsUpdated(), successMessage, errorMessage @@ -103,7 +123,7 @@ export const TagsAddRemove: React.FC = ({ agents!, tagsToAdd, tagsToRemove, - () => onTagsUpdated(), + (hasCompleted) => handleTagsUpdated(tagsToAdd, tagsToRemove, hasCompleted), successMessage, errorMessage ); @@ -133,7 +153,9 @@ export const TagsAddRemove: React.FC = ({ + handleTagsUpdated(tagsToAdd, tagsToRemove, hasCompleted, true) + } /> diff --git a/x-pack/plugins/fleet/public/applications/fleet/sections/agents/agent_list_page/hooks/use_update_tags.test.tsx b/x-pack/plugins/fleet/public/applications/fleet/sections/agents/agent_list_page/hooks/use_update_tags.test.tsx index 6a5eb8ef3e5e4..d5d72c3deaeeb 100644 --- a/x-pack/plugins/fleet/public/applications/fleet/sections/agents/agent_list_page/hooks/use_update_tags.test.tsx +++ b/x-pack/plugins/fleet/public/applications/fleet/sections/agents/agent_list_page/hooks/use_update_tags.test.tsx @@ -35,10 +35,11 @@ describe('useUpdateTags', () => { const mockOnSuccess = jest.fn(); beforeEach(() => { mockSendPutAgentTagsUpdate.mockReset(); + mockSendPostBulkAgentTagsUpdate.mockReset(); mockOnSuccess.mockReset(); }); it('should call onSuccess when update tags succeeds', async () => { - mockSendPutAgentTagsUpdate.mockResolvedValueOnce({}); + mockSendPutAgentTagsUpdate.mockResolvedValueOnce({ data: {} }); const { result } = renderHook(() => useUpdateTags()); await act(() => result.current.updateTags('agent1', ['tag1'], mockOnSuccess)); @@ -61,7 +62,7 @@ describe('useUpdateTags', () => { }); it('should call onSuccess when bulk update tags succeeds', async () => { - mockSendPostBulkAgentTagsUpdate.mockResolvedValueOnce({}); + mockSendPostBulkAgentTagsUpdate.mockResolvedValueOnce({ data: { actionId: 'action' } }); const { result } = renderHook(() => useUpdateTags()); await act(() => result.current.bulkUpdateTags('query', ['tag1'], [], mockOnSuccess)); diff --git a/x-pack/plugins/fleet/public/applications/fleet/sections/agents/agent_list_page/hooks/use_update_tags.tsx b/x-pack/plugins/fleet/public/applications/fleet/sections/agents/agent_list_page/hooks/use_update_tags.tsx index 3e98c817a881b..969b9caa4d02f 100644 --- a/x-pack/plugins/fleet/public/applications/fleet/sections/agents/agent_list_page/hooks/use_update_tags.tsx +++ b/x-pack/plugins/fleet/public/applications/fleet/sections/agents/agent_list_page/hooks/use_update_tags.tsx @@ -20,7 +20,7 @@ export const useUpdateTags = () => { const wrapRequest = useCallback( async ( requestFn: () => Promise, - onSuccess: () => void, + onSuccess: (hasCompleted?: boolean) => void, successMessage?: string, errorMessage?: string ) => { @@ -30,6 +30,7 @@ export const useUpdateTags = () => { if (res.error) { throw res.error; } + const hasCompleted = !res.data.actionId; const message = successMessage ?? i18n.translate('xpack.fleet.updateAgentTags.successNotificationTitle', { @@ -37,7 +38,7 @@ export const useUpdateTags = () => { }); notifications.toasts.addSuccess(message); - onSuccess(); + onSuccess(hasCompleted); } catch (error) { const errorTitle = errorMessage ?? @@ -73,7 +74,7 @@ export const useUpdateTags = () => { agents: string[] | string, tagsToAdd: string[], tagsToRemove: string[], - onSuccess: () => void, + onSuccess: (hasCompleted?: boolean) => void, successMessage?: string, errorMessage?: string ) => { diff --git a/x-pack/plugins/fleet/server/services/agents/unenroll_action_runner.ts b/x-pack/plugins/fleet/server/services/agents/unenroll_action_runner.ts index dfa7fac5e0e4c..f9ba2e4be44a1 100644 --- a/x-pack/plugins/fleet/server/services/agents/unenroll_action_runner.ts +++ b/x-pack/plugins/fleet/server/services/agents/unenroll_action_runner.ts @@ -144,7 +144,7 @@ export async function updateActionsForForceUnenroll( actionId: string, total: number ) { - // creating an unenroll so that force unenroll shows up in activity + // creating an action doc so that force unenroll shows up in activity await createAgentAction(esClient, { id: actionId, agents: [], diff --git a/x-pack/plugins/fleet/server/services/agents/update_agent_tags.test.ts b/x-pack/plugins/fleet/server/services/agents/update_agent_tags.test.ts index 7ac8e4e3f9e78..79df3f0779768 100644 --- a/x-pack/plugins/fleet/server/services/agents/update_agent_tags.test.ts +++ b/x-pack/plugins/fleet/server/services/agents/update_agent_tags.test.ts @@ -8,6 +8,7 @@ import type { SavedObjectsClientContract } from '@kbn/core/server'; import type { ElasticsearchClientMock } from '@kbn/core/server/mocks'; import { elasticsearchServiceMock, savedObjectsClientMock } from '@kbn/core/server/mocks'; +import { createClientMock } from './action.mock'; import { updateAgentTags } from './update_agent_tags'; jest.mock('./filter_hosted_agents', () => ({ @@ -105,6 +106,31 @@ describe('update_agent_tags', () => { await updateAgentTags(soClient, esClient, { agentIds: ['agent1'] }, ['newName'], []); expectTagsInEsBulk(['one', 'two', 'three', 'newName']); + + const actionResults = esClient.bulk.mock.calls[1][0] as any; + const resultIds = actionResults?.body + ?.filter((i: any) => i.agent_id) + .map((i: any) => i.agent_id); + expect(resultIds).toEqual(['agent1']); + + const action = esClient.create.mock.calls[0][0] as any; + expect(action.body.type).toEqual('UPDATE_TAGS'); + }); + + it('should write error action results for hosted agent', async () => { + const { esClient: esClientMock, agentInHostedDoc } = createClientMock(); + + await updateAgentTags( + soClient, + esClientMock, + { agentIds: [agentInHostedDoc._id] }, + ['newName'], + [] + ); + + const errorResults = esClientMock.bulk.mock.calls[1][0] as any; + const errorIds = errorResults?.body?.filter((i: any) => i.agent_id).map((i: any) => i.agent_id); + expect(errorIds).toEqual([agentInHostedDoc._id]); }); it('should add tag at the end when tagsToRemove not in existing tags', async () => { diff --git a/x-pack/plugins/fleet/server/services/agents/update_agent_tags_action_runner.ts b/x-pack/plugins/fleet/server/services/agents/update_agent_tags_action_runner.ts index 906566aee9f41..807f2ab1cf073 100644 --- a/x-pack/plugins/fleet/server/services/agents/update_agent_tags_action_runner.ts +++ b/x-pack/plugins/fleet/server/services/agents/update_agent_tags_action_runner.ts @@ -6,16 +6,19 @@ */ import type { SavedObjectsClientContract, ElasticsearchClient } from '@kbn/core/server'; - +import uuid from 'uuid'; import { difference, uniq } from 'lodash'; import type { Agent, BulkActionResult } from '../../types'; +import { appContextService } from '../app_context'; + import { ActionRunner } from './action_runner'; import { errorsToResults, bulkUpdateAgents } from './crud'; import { BulkActionTaskType } from './bulk_actions_resolver'; import { filterHostedPolicies } from './filter_hosted_agents'; +import { bulkCreateAgentActionResults, createAgentAction } from './actions'; export class UpdateAgentTagsActionRunner extends ActionRunner { protected async processAgents(agents: Agent[]): Promise<{ items: BulkActionResult[] }> { @@ -24,7 +27,12 @@ export class UpdateAgentTagsActionRunner extends ActionRunner { this.esClient, agents, {}, - { tagsToAdd: this.actionParams?.tagsToAdd, tagsToRemove: this.actionParams?.tagsToRemove }, + { + tagsToAdd: this.actionParams?.tagsToAdd, + tagsToRemove: this.actionParams?.tagsToRemove, + actionId: this.actionParams.actionId, + total: this.actionParams.total, + }, undefined, true ); @@ -47,6 +55,8 @@ export async function updateTagsBatch( options: { tagsToAdd: string[]; tagsToRemove: string[]; + actionId?: string; + total?: number; }, agentIds?: string[], skipSuccess?: boolean @@ -87,5 +97,43 @@ export async function updateTagsBatch( })) ); + const actionId = options.actionId ?? uuid(); + const total = options.total ?? givenAgents.length; + const errorCount = Object.keys(errors).length; + + // creating an action doc so that update tags shows up in activity + await createAgentAction(esClient, { + id: actionId, + agents: [], + created_at: new Date().toISOString(), + type: 'UPDATE_TAGS', + total, + }); + await bulkCreateAgentActionResults( + esClient, + filteredAgents.map((agent) => ({ + agentId: agent.id, + actionId, + })) + ); + + if (errorCount > 0) { + appContextService + .getLogger() + .info( + `Skipping ${errorCount} agents, as failed validation (cannot modified tags on hosted agents)` + ); + + // writing out error result for those agents that failed validation, so the action is not going to stay in progress forever + await bulkCreateAgentActionResults( + esClient, + Object.keys(errors).map((agentId) => ({ + agentId, + actionId, + error: errors[agentId].message, + })) + ); + } + return { items: errorsToResults(filteredAgents, errors, agentIds, skipSuccess) }; }