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

[Fleet] [Managed Policies] Bulk actions changes #90437

Closed
9 tasks done
jfsiii opened this issue Feb 5, 2021 · 6 comments
Closed
9 tasks done

[Fleet] [Managed Policies] Bulk actions changes #90437

jfsiii opened this issue Feb 5, 2021 · 6 comments
Assignees
Labels
Team:Fleet Team label for Observability Data Collection Fleet team v7.13.0

Comments

@jfsiii
Copy link
Contributor

jfsiii commented Feb 5, 2021

From #76843

Current bulk behavior

If a list of agent ids includes any enrolled in a managed policy

  • Bulk reassign filters the managed agents from the list to be reassigned. It reassigns the permitted agents.
  • Bulk unenroll filters the managed agents from the list to be unenrolled. It unenrolls the permitted agents.
  • Bulk upgrade throws Cannot upgrade agent in managed policy ${policy.id}. It doesn't upgrade any agents.

Proposed bulk behavior

  • Bulk reassign, unenroll, and upgrade will all return an array of objects.
  • They return a 200 response not 4xx/5xx if any agent ids are enrolled in a managed policy
  • There will be one output array entry for each input
    • response shape(s) TBD, perhaps data?: T, error?: Error
    • expand existing pattern used by bulk_reassign
    export type PostBulkAgentReassignResponse = Record<
      Agent['id'],
      {
        success: boolean;
        error?: Error;
      }
    >;
    
@jfsiii jfsiii added Team:Fleet Team label for Observability Data Collection Fleet team v7.12.0 labels Feb 5, 2021
@jfsiii jfsiii self-assigned this Feb 5, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@jfsiii jfsiii changed the title [Fleet] Bulk API actions on agents should error if any item is forbidden [Fleet] [Managed Policies] Bulk actions changes Feb 5, 2021
@jfsiii jfsiii added v7.13.0 and removed v7.12.0 labels Feb 16, 2021
@ph
Copy link
Contributor

ph commented Feb 22, 2021

Discussion

  • Could we include the hostname and the Agent id?
  • Some part is inconsistent in the bulk action?
  • This is non-blocking because we already implemented the restrictions in 7.12. This issue is about how to improve the UX.

Actions

@mostlyjason @hbharding to lead product discussion for:

  • Should we mirror the Elasticsearch behavior for bulk action?
  • What is the UI for the bulk action?
  • @jfsiii to write down the current behavior. Updated description to include current behavior

@mostlyjason
Copy link
Contributor

mostlyjason commented Mar 3, 2021

@hbharding and I discussed this earlier today. I think we should make the behavior for the bulk actions consistent, both with each other and with Elasticsearch. That means we should switch the upgrade behavior to apply the upgrade to permitted agents. This will allow the user upgrade all the agents they are permitted to. The remaining agents can be upgraded in the external management solution.

To support this workflow in the UI, we discussed updating the existing bulk action confirmation dialog to show a message when the bulk action cannot apply to all the selected agents. It will say something like "2 of the agents you selected cannot be upgraded/reassigned/unenrolled because they are enrolled in an externally managed agent policy". The user can then choose to apply the action or cancel.

I believe @hbharding will add a mockup of the confirmation dialog

@hbharding
Copy link
Contributor

hbharding commented Mar 4, 2021

Update 3/29 As we discussed in Slack, we can simplify what I describe below and just disable the checkbox to prevent users from bulk selecting agents that use a hosted agent policy. Please disregard the screenshots below and use the ones provided here https://github.com/elastic/observability-design/issues/32


Screenshots for this below. My proposal is that we include a warning callout below the modal's title indicating "X of Y agents you have selected cannot be [action, past tense] because they are enrolled in an externally managed agent policy"

I noticed a small inconsistency with how we implemented bulk actions in the UI. Unenroll and upgrade both open modals, whereas the Assign to new policy action opens a fly out. When I worked on designs for this feature awhile ago, I had intended for these to all use modals and I think it was overlooked. The reason I point this out is that the current "Assign new agent policy" fly out does not give an indication as to how many agents will be affected if it's a bulk action, whereas the other actions do. I'm hoping this will be an easy change to make. If not, we can use the same callout in our existing "assign to new policy" flyout below the flyout header. Ideally we'd also include a count of how many agents will be affected in the flyout title and button in footer.

image
image
Assign to new policy as a modal (as intended)
image
Current bulk reassign flyout
image

jfsiii pushed a commit that referenced this issue Mar 17, 2021
…94632)

## Problem
While working on changes for bulk reassign #90437, I found that the server has a runtime error and returns a 500 if given an invalid or missing id.

<details><summary>server error stack trace</summary>

```
   │ proc [kibana] server    log   [12:21:48.953] [error][fleet][plugins] TypeError: Cannot read property 'policy_revision_idx' of undefined
   │ proc [kibana]     at map (/Users/jfsiii/work/kibana/x-pack/plugins/fleet/server/services/agents/helpers.ts:15:34)
   │ proc [kibana]     at Array.map (<anonymous>)
   │ proc [kibana]     at getAgents (/Users/jfsiii/work/kibana/x-pack/plugins/fleet/server/services/agents/crud.ts:191:32)
   │ proc [kibana]     at runMicrotasks (<anonymous>)
   │ proc [kibana]     at processTicksAndRejections (internal/process/task_queues.js:93:5)
   │ proc [kibana]     at Object.reassignAgents (/Users/jfsiii/work/kibana/x-pack/plugins/fleet/server/services/agents/reassign.ts:91:9)
   │ proc [kibana]     at postBulkAgentsReassignHandler (/Users/jfsiii/work/kibana/x-pack/plugins/fleet/server/routes/agent/handlers.ts:314:21)
   │ proc [kibana]     at Router.handle (/Users/jfsiii/work/kibana/src/core/server/http/router/router.ts:272:30)
   │ proc [kibana]     at handler (/Users/jfsiii/work/kibana/src/core/server/http/router/router.ts:227:11)
   │ proc [kibana]     at exports.Manager.execute (/Users/jfsiii/work/kibana/node_modules/@hapi/hapi/lib/toolkit.js:60:28)
   │ proc [kibana]     at Object.internals.handler (/Users/jfsiii/work/kibana/node_modules/@hapi/hapi/lib/handler.js:46:20)
   │ proc [kibana]     at exports.execute (/Users/jfsiii/work/kibana/node_modules/@hapi/hapi/lib/handler.js:31:20)
   │ proc [kibana]     at Request._lifecycle (/Users/jfsiii/work/kibana/node_modules/@hapi/hapi/lib/request.js:370:32)
   │ proc [kibana]     at Request._execute (/Users/jfsiii/work/kibana/node_modules/@hapi/hapi/lib/request.js:279:9)
```
</details>

<details><summary>see test added in this PR fail on master</summary>

```
1)    Fleet Endpoints
       reassign agent(s)
         bulk reassign agents
           should allow to reassign multiple agents by id -- some invalid:

      Error: expected 200 "OK", got 500 "Internal Server Error"
```
</details>

## Root cause
Debugging runtime error in `searchHitToAgent` found some TS type mismatches for the ES values being returned. Perhaps from one or more of the recent changes to ES client & Fleet Server. Based on `test:jest` and `test:ftr`, it appears the possible types are `GetResponse` or `SearchResponse`, instead of only an `ESSearchHit`.

https://github.com/elastic/kibana/pull/94632/files#diff-254d0f427979efc3b442f78762302eb28fb9c8857df68ea04f8d411e052f939cL11

While a `.search` result will include return matched values, a `.get` or `.mget` will return a row for each input and a `found: boolean`. e.g. `{ _id: "does-not-exist", found: false }`. The error occurs when [`searchHitToAgent`](https://github.com/jfsiii/kibana/blob/1702cf98f018c41ec0a080d829a12403168ac242/x-pack/plugins/fleet/server/services/agents/helpers.ts#L11) is run on a get miss instead of a search hit.

## PR Changes
* Added a test to ensure it doesn't fail if invalid or missing IDs are given
* Moved the `bulk_reassign` tests to their own test section
* Filter out any missing results before calling `searchHitToAgent`, to match current behavior
* Consolidate repeated arguments into and code for getting agents into single [function](https://github.com/elastic/kibana/pull/94632/files#diff-f7377ed9ad56eaa8ea188b64e957e771ccc7a7652fd1eaf44251c25b930f8448R70-R87):  and [TS type](https://github.com/elastic/kibana/pull/94632/files#diff-f7377ed9ad56eaa8ea188b64e957e771ccc7a7652fd1eaf44251c25b930f8448R61-R68)
* Rename some agent service functions to be more explicit (IMO) but behavior maintained. Same API names exported.

This moves toward the "one result (success or error) per given id" approach for #90437

### Checklist
- [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
jfsiii pushed a commit that referenced this issue Mar 17, 2021
…94632) (#94774)

## Problem
While working on changes for bulk reassign #90437, I found that the server has a runtime error and returns a 500 if given an invalid or missing id.

<details><summary>server error stack trace</summary>

```
   │ proc [kibana] server    log   [12:21:48.953] [error][fleet][plugins] TypeError: Cannot read property 'policy_revision_idx' of undefined
   │ proc [kibana]     at map (/Users/jfsiii/work/kibana/x-pack/plugins/fleet/server/services/agents/helpers.ts:15:34)
   │ proc [kibana]     at Array.map (<anonymous>)
   │ proc [kibana]     at getAgents (/Users/jfsiii/work/kibana/x-pack/plugins/fleet/server/services/agents/crud.ts:191:32)
   │ proc [kibana]     at runMicrotasks (<anonymous>)
   │ proc [kibana]     at processTicksAndRejections (internal/process/task_queues.js:93:5)
   │ proc [kibana]     at Object.reassignAgents (/Users/jfsiii/work/kibana/x-pack/plugins/fleet/server/services/agents/reassign.ts:91:9)
   │ proc [kibana]     at postBulkAgentsReassignHandler (/Users/jfsiii/work/kibana/x-pack/plugins/fleet/server/routes/agent/handlers.ts:314:21)
   │ proc [kibana]     at Router.handle (/Users/jfsiii/work/kibana/src/core/server/http/router/router.ts:272:30)
   │ proc [kibana]     at handler (/Users/jfsiii/work/kibana/src/core/server/http/router/router.ts:227:11)
   │ proc [kibana]     at exports.Manager.execute (/Users/jfsiii/work/kibana/node_modules/@hapi/hapi/lib/toolkit.js:60:28)
   │ proc [kibana]     at Object.internals.handler (/Users/jfsiii/work/kibana/node_modules/@hapi/hapi/lib/handler.js:46:20)
   │ proc [kibana]     at exports.execute (/Users/jfsiii/work/kibana/node_modules/@hapi/hapi/lib/handler.js:31:20)
   │ proc [kibana]     at Request._lifecycle (/Users/jfsiii/work/kibana/node_modules/@hapi/hapi/lib/request.js:370:32)
   │ proc [kibana]     at Request._execute (/Users/jfsiii/work/kibana/node_modules/@hapi/hapi/lib/request.js:279:9)
```
</details>

<details><summary>see test added in this PR fail on master</summary>

```
1)    Fleet Endpoints
       reassign agent(s)
         bulk reassign agents
           should allow to reassign multiple agents by id -- some invalid:

      Error: expected 200 "OK", got 500 "Internal Server Error"
```
</details>

## Root cause
Debugging runtime error in `searchHitToAgent` found some TS type mismatches for the ES values being returned. Perhaps from one or more of the recent changes to ES client & Fleet Server. Based on `test:jest` and `test:ftr`, it appears the possible types are `GetResponse` or `SearchResponse`, instead of only an `ESSearchHit`.

https://github.com/elastic/kibana/pull/94632/files#diff-254d0f427979efc3b442f78762302eb28fb9c8857df68ea04f8d411e052f939cL11

While a `.search` result will include return matched values, a `.get` or `.mget` will return a row for each input and a `found: boolean`. e.g. `{ _id: "does-not-exist", found: false }`. The error occurs when [`searchHitToAgent`](https://github.com/jfsiii/kibana/blob/1702cf98f018c41ec0a080d829a12403168ac242/x-pack/plugins/fleet/server/services/agents/helpers.ts#L11) is run on a get miss instead of a search hit.

## PR Changes
* Added a test to ensure it doesn't fail if invalid or missing IDs are given
* Moved the `bulk_reassign` tests to their own test section
* Filter out any missing results before calling `searchHitToAgent`, to match current behavior
* Consolidate repeated arguments into and code for getting agents into single [function](https://github.com/elastic/kibana/pull/94632/files#diff-f7377ed9ad56eaa8ea188b64e957e771ccc7a7652fd1eaf44251c25b930f8448R70-R87):  and [TS type](https://github.com/elastic/kibana/pull/94632/files#diff-f7377ed9ad56eaa8ea188b64e957e771ccc7a7652fd1eaf44251c25b930f8448R61-R68)
* Rename some agent service functions to be more explicit (IMO) but behavior maintained. Same API names exported.

This moves toward the "one result (success or error) per given id" approach for #90437

### Checklist
- [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

# Conflicts:
#	x-pack/plugins/fleet/server/services/agents/crud.ts
#	x-pack/plugins/fleet/server/services/index.ts
jfsiii pushed a commit that referenced this issue Mar 18, 2021
## Summary

Refactoring in progress towards #90437 

Collapse `forceUninstallAgents` into `uninstallAgents` into one function with an option. The pseudocode diff is

```diff
- function uninstallAgents() {
-   // filtering logic A
-   // side effects A
- }
- function forceUninstallAgents() {
-   // filtering logic A
-   // side effects B
- }

+ function uninstallAgents({ flag: boolean}) {
+   // filtering logic A
+   // if flag: side effects B
+   // else: side effects A
+ }
```

actually, there is [_one difference_](https://github.com/elastic/kibana/pull/94848/files/5564f253831a8a94e2b2bbb808afe6d2019016d7#diff-ecc3c625f2366949f1723e56b8477f6afb552ccfbcf3a71e0c28b2062fd05195) in the filtering logic
 
### Checklist
- [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
kibanamachine pushed a commit to kibanamachine/kibana that referenced this issue Mar 18, 2021
## Summary

Refactoring in progress towards elastic#90437 

Collapse `forceUninstallAgents` into `uninstallAgents` into one function with an option. The pseudocode diff is

```diff
- function uninstallAgents() {
-   // filtering logic A
-   // side effects A
- }
- function forceUninstallAgents() {
-   // filtering logic A
-   // side effects B
- }

+ function uninstallAgents({ flag: boolean}) {
+   // filtering logic A
+   // if flag: side effects B
+   // else: side effects A
+ }
```

actually, there is [_one difference_](https://github.com/elastic/kibana/pull/94848/files/5564f253831a8a94e2b2bbb808afe6d2019016d7#diff-ecc3c625f2366949f1723e56b8477f6afb552ccfbcf3a71e0c28b2062fd05195) in the filtering logic
 
### Checklist
- [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
kibanamachine added a commit that referenced this issue Mar 18, 2021
## Summary

Refactoring in progress towards #90437 

Collapse `forceUninstallAgents` into `uninstallAgents` into one function with an option. The pseudocode diff is

```diff
- function uninstallAgents() {
-   // filtering logic A
-   // side effects A
- }
- function forceUninstallAgents() {
-   // filtering logic A
-   // side effects B
- }

+ function uninstallAgents({ flag: boolean}) {
+   // filtering logic A
+   // if flag: side effects B
+   // else: side effects A
+ }
```

actually, there is [_one difference_](https://github.com/elastic/kibana/pull/94848/files/5564f253831a8a94e2b2bbb808afe6d2019016d7#diff-ecc3c625f2366949f1723e56b8477f6afb552ccfbcf3a71e0c28b2062fd05195) in the filtering logic
 
### Checklist
- [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

Co-authored-by: John Schulz <[email protected]>
jfsiii pushed a commit that referenced this issue Mar 23, 2021
## Summary

0cbbb41 is just a rearrangement of the tests.  5cad301 has the real changes: 
* Bug fix: `force: true` should bypass any restrictions re: managed policies
* Refactoring towards new response shape coming as part of #90437
* Added test to confirm new behavior


### Checklist
- [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
kibanamachine pushed a commit to kibanamachine/kibana that referenced this issue Mar 23, 2021
## Summary

elastic@0cbbb41 is just a rearrangement of the tests.  elastic@5cad301 has the real changes: 
* Bug fix: `force: true` should bypass any restrictions re: managed policies
* Refactoring towards new response shape coming as part of elastic#90437
* Added test to confirm new behavior


### Checklist
- [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
kibanamachine added a commit that referenced this issue Mar 23, 2021
## Summary

0cbbb41 is just a rearrangement of the tests.  5cad301 has the real changes: 
* Bug fix: `force: true` should bypass any restrictions re: managed policies
* Refactoring towards new response shape coming as part of #90437
* Added test to confirm new behavior


### Checklist
- [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

Co-authored-by: John Schulz <[email protected]>
jfsiii pushed a commit that referenced this issue Mar 26, 2021
## Summary
`/agents/bulk_unenroll` should return a response with a result for each agent given; including invalid or missing ids. It currently returns an empty object. #90437


[TS type diff for response](https://github.com/jfsiii/kibana/blob/dd34e4c5ef9266a9c5162e33ca0efb4affd9b25b/x-pack/plugins/fleet/common/types/rest_spec/agent.ts#L124-L130)

```diff
- // eslint-disable-next-line @typescript-eslint/no-empty-interface
- export interface PostBulkAgentUnenrollResponse {}
+ export type PostBulkAgentUnenrollResponse = Record<
+   Agent['id'],
+   {
+     success: boolean;
+     error?: string;
+   }
+ >;
```

### Checklist

- [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
kibanamachine pushed a commit to kibanamachine/kibana that referenced this issue Mar 26, 2021
…ic#95571)

## Summary
`/agents/bulk_unenroll` should return a response with a result for each agent given; including invalid or missing ids. It currently returns an empty object. elastic#90437


[TS type diff for response](https://github.com/jfsiii/kibana/blob/dd34e4c5ef9266a9c5162e33ca0efb4affd9b25b/x-pack/plugins/fleet/common/types/rest_spec/agent.ts#L124-L130)

```diff
- // eslint-disable-next-line @typescript-eslint/no-empty-interface
- export interface PostBulkAgentUnenrollResponse {}
+ export type PostBulkAgentUnenrollResponse = Record<
+   Agent['id'],
+   {
+     success: boolean;
+     error?: string;
+   }
+ >;
```

### Checklist

- [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
kibanamachine added a commit that referenced this issue Mar 26, 2021
… (#95593)

## Summary
`/agents/bulk_unenroll` should return a response with a result for each agent given; including invalid or missing ids. It currently returns an empty object. #90437


[TS type diff for response](https://github.com/jfsiii/kibana/blob/dd34e4c5ef9266a9c5162e33ca0efb4affd9b25b/x-pack/plugins/fleet/common/types/rest_spec/agent.ts#L124-L130)

```diff
- // eslint-disable-next-line @typescript-eslint/no-empty-interface
- export interface PostBulkAgentUnenrollResponse {}
+ export type PostBulkAgentUnenrollResponse = Record<
+   Agent['id'],
+   {
+     success: boolean;
+     error?: string;
+   }
+ >;
```

### Checklist

- [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

Co-authored-by: John Schulz <[email protected]>
@hbharding
Copy link
Contributor

I provided updated designs for this here elastic/observability-design#32. I wanted to create a separate issue to track work on our obs:design 7.13 board.

@jfsiii
Copy link
Contributor Author

jfsiii commented Apr 2, 2021

Closing this ticket. The API work is done and UI issues will be in separate issues/PRs like #96087 based on specs in https://github.com/elastic/observability-design/issues/32

@jfsiii jfsiii closed this as completed Apr 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Fleet Team label for Observability Data Collection Fleet team v7.13.0
Projects
None yet
Development

No branches or pull requests

5 participants