[Fleet] Add warning if need root integrations trying to be used with unprivileged agents by juliaElastic · Pull Request #183283 · elastic/kibana

juliaElastic · 2024-05-13T12:55:14Z

Summary

Closes https://github.com/elastic/ingest-dev/issues/3252

Add integration

Added warning to Add integration when the integration requires root privilege and the selected existing agent policy has unprivileged agents enrolled.

To verify:

enroll an agent with docker (it has unprivileged: true)
try to add an integration that requires root e.g. auditd_manager
verify that when trying to save the integration, the warning callout is part of the confirm deploy modal

Add agent flyout

Added warning to Add agent flyout when an unprivileged agent is detected in combination with an agent policy that has integrations requiring root

To verify:

add an integration to an agent policy that requires root e.g. auditd_manager
open Add agent flyout, verify that the warning callout is visible

Open question:

Do we want to show the warning on Add agent flyout only for newly enrolled agents (in the last 10 mins like we query enrolled agents), or any unprivileged agents that are enrolled to this policy?
- Decision: No longer applicable as we decided to not show a count here

Checklist

Unit or functional tests were updated or added to match the most common scenarios

ghost · 2024-05-13T12:55:27Z

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

/oblt-deploy : Deploy a Kibana instance using the Observability test environments.
run docs-build : Re-trigger the docs validation. (use unformatted text in the comment!)

juliaElastic · 2024-05-13T12:55:44Z

/ci

juliaElastic · 2024-05-13T13:26:34Z

/ci

juliaElastic · 2024-05-14T07:16:21Z

/ci

juliaElastic · 2024-05-14T08:45:19Z

/ci

x-pack/plugins/fleet/server/services/agent_policy.ts

elasticmachine · 2024-05-14T08:47:12Z

Pinging @elastic/fleet (Team:Fleet)

kilfoyle

LGTM for the text! 👍

juliaElastic · 2024-05-14T15:03:44Z

...ications/fleet/sections/agent_policy/create_package_policy_page/single_page_layout/index.tsx

          </>
        )}
+        {isConfirmModalOpen ? (
+          <UnprivilegedConfirmModal


I'm going to add UI tests

above in this file, there is another modal nested under formState === 'CONFIRM'

I'm concerned about potential conflicts with that modal since this one is outside of that conditional. can formState be part of the condition for opening this modal?

btw, if we continue to use isConfirmModalOpen, it should probably be renamed because there are multiple confirm modals in this flow and this is only for the unprivileged modal

...cy/create_package_policy_page/multi_page_layout/components/page_steps/install_agent/types.ts

x-pack/plugins/fleet/server/services/agent_policy.ts

nchaulet

LGTM 🚀

jen-huang

A few UX suggestions.

For the Add agent flyout, I think it would be good to also notify the user before the attempt installation that the selected policy needs privileged agents. How about we put this just before the installation commands code block? That way it can appear on the standalone agent installation instructions too:

jen-huang · 2024-05-16T18:12:20Z

...ications/fleet/sections/agent_policy/create_package_policy_page/single_page_layout/index.tsx

            onCancel={() => setFormState('VALID')}
          />
        )}
+        {isUnprivilegedModalOpen && !(formState === 'CONFIRM' && agentPolicy) ? (


when I add auditd to an existing policy with an unprivileged agent is such that I get this modal first, I click Add integration, and I get the second Save and deploy changes confirmation modal. this seems like a UX anti-pattern.

is it possible to merge these two modals? since both modals are conditional on if there are already agents enrolled into the selected policy, we could have the unprivileged warning callout below the blue callout if there are some unprivileged agents:

WDYT? and sorry for the review churn - I should have commented on this behavior in my initial review 😅

Good point, showing two modals is not ideal indeed. I'll update.

Merged the two modals:

juliaElastic · 2024-05-17T07:17:41Z

For the Add agent flyout, I think it would be good to also notify the user before the attempt installation that the selected policy needs privileged agents. How about we put this just before the installation commands code block? That way it can appear on the standalone agent installation instructions too:

Good point, I'll move it.
I think we should eventually add to the instruction how to enroll an agent in unprivileged mode.

I moved the callout before the install commands, do we still want to show how many unprivileged agents are enrolled? If there are no unprivileged agents yet, we can show a generic warning:

If there are unprivileged agents:

kpollich · 2024-05-17T12:49:22Z

do we still want to show how many unprivileged agents are enrolled?

I don't know how useful the count is in this scenario without also listing out each agent ID or something along those lines. Maybe we can opt to exclude it for now and revisit this callout with some technical writing/UX eyes. One thing that comes to mind: would we want to give the user a link to a pre-filtered agents table view that shows the agents in question so they can take action and then come back to the enrollment flow? As it stands, I think unless we are providing clear call to actions this callout is mainly informative, and we're not driving the user to take action. This is okay for the initial scope here, imo.

@juliaElastic - would it be possible to render the list of integrations in a <ul> instead of as a comma separate string? I foresee an edge case where there 10+ integrations that fall into this case on a large policy and callout starts to look a bit messy. We could put the list of offending integrations after the main callout text, maybe with some wording tweaks e.g.

This agent policy contains the following integrations that require Elastic Agents to have root privileges.
To ensure that all data required by the integrations can be collected, reenroll the agents using an account with root privileges. 

- Auditd Manager
- ...
- ...

juliaElastic · 2024-05-17T13:08:05Z

I don't know how useful the count is in this scenario without also listing out each agent ID or something along those lines. Maybe we can opt to exclude it for now and revisit this callout with some technical writing/UX eyes. One thing that comes to mind: would we want to give the user a link to a pre-filtered agents table view that shows the agents in question so they can take action and then come back to the enrollment flow? As it stands, I think unless we are providing clear call to actions this callout is mainly informative, and we're not driving the user to take action. This is okay for the initial scope here, imo.

Thanks for the suggestions, I agree that it's not that useful to show the count of unprivileged agents if not actionable. I'll remove it for now. cc @nimarezainia

I'll change the callout to show a list of integrations.

Updated:

jen-huang

The UI changes look great, thanks for making them @juliaElastic :)
Agree with the decision to remove the count from agent enrollment flyout.

I pulled the screenshots you provided to the PR description.

I tested locally 👍

nimarezainia · 2024-05-20T00:19:35Z

I moved the callout before the install commands, do we still want to show how many unprivileged agents are enrolled? If there are no unprivileged agents yet, we can show a generic warning:

IMO it doesn't hurt to show the number of agents. More information is always better. Also could help with troubleshooting if there are issues.

… src/core/server/integration_tests/ci_checks'

juliaElastic · 2024-05-21T07:06:18Z

IMO it doesn't hurt to show the number of agents. More information is always better. Also could help with troubleshooting if there are issues.

I would leave it as is for now, we can adjust the UI later if needed.

kibana-ci · 2024-05-21T10:45:01Z

💚 Build Succeeded

Buildkite Build
Commit: 16debbc

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`fleet`	1011	1013	+2

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`	1187	1188	+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`	1.3MB	1.3MB	+4.0KB

Page load bundle

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

id	before	after	diff
`fleet`	164.4KB	164.6KB	+189.0B

Unknown metric groups

API count

id	before	after	diff
`fleet`	1308	1309	+1

History

💔 Build #211117 failed 25de4b1
💚 Build #210782 succeeded dc54eff
💔 Build #210758 failed dbcf8a3
💚 Build #210714 succeeded 2b2fe93
💔 Build #210661 failed 1841922

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

cc @juliaElastic

added warning to add integration

87f76b5

juliaElastic added the release_note:feature Makes this part of the condensed release notes label May 13, 2024

juliaElastic self-assigned this May 13, 2024

updated text

f008f6b

Merge branch 'main' into unprivileged-warning

cc31e3f

juliaElastic added 2 commits May 14, 2024 10:32

flyout warning

d4707fb

added title

104f770

juliaElastic marked this pull request as ready for review May 14, 2024 08:45

juliaElastic requested a review from a team as a code owner May 14, 2024 08:45

juliaElastic commented May 14, 2024

View reviewed changes

x-pack/plugins/fleet/server/services/agent_policy.ts Outdated Show resolved Hide resolved

botelastic bot added the Team:Fleet Team label for Observability Data Collection Fleet team label May 14, 2024

juliaElastic and others added 3 commits May 14, 2024 12:35

Merge branch 'main' into unprivileged-warning

17e1e25

fix checks

86ac37b

api integration test and openapi

be9a989

juliaElastic requested a review from a team as a code owner May 14, 2024 11:19

juliaElastic added 3 commits May 14, 2024 13:21

remove commented code

28a6899

fix tests

3be0b42

poll unprivileged agent count to show warning for newly enrolled agents

3f2eb0a

kilfoyle approved these changes May 14, 2024

View reviewed changes

juliaElastic added 2 commits May 14, 2024 16:39

fix test

6afd30f

fix test

f69b8aa

juliaElastic commented May 14, 2024

View reviewed changes

fix test

979dfdd

jen-huang reviewed May 14, 2024

View reviewed changes

...cy/create_package_policy_page/multi_page_layout/components/page_steps/install_agent/types.ts Outdated Show resolved Hide resolved

x-pack/plugins/fleet/server/services/agent_policy.ts Outdated Show resolved Hide resolved

kpollich mentioned this pull request May 16, 2024

[System] Mark logs-system.syslog data stream as requires root elastic/integrations#9893

Merged

4 tasks

nchaulet approved these changes May 16, 2024

View reviewed changes

juliaElastic requested a review from jloleysens May 16, 2024 12:34

jen-huang reviewed May 16, 2024

View reviewed changes

Merge branch 'main' into unprivileged-warning

bcebd4f

juliaElastic added 5 commits May 17, 2024 10:04

moved add agent flyout warning to install instructions

52b86f2

merge two modals

c762d9c

fix

5d7edae

added unit test

1841922

fix checks

2b2fe93

jeramysoucy mentioned this pull request May 17, 2024

[EDR Workflows] Add new backfill function to set on_write_scan to false if Malware protection is off #182598

Merged

1 task

pgayvallet approved these changes May 17, 2024

View reviewed changes

removed unprivileged agent count, updated callout

dbcf8a3

juliaElastic requested a review from jen-huang May 17, 2024 13:53

fix checks

dc54eff

jen-huang approved these changes May 17, 2024

View reviewed changes

juliaElastic and others added 3 commits May 21, 2024 08:24

Merge branch 'main' into unprivileged-warning

c890fa3

fix lint

58d11a2

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

25de4b1

… src/core/server/integration_tests/ci_checks'

juliaElastic enabled auto-merge (squash) May 21, 2024 07:10

fix test

16debbc

juliaElastic merged commit ad03dfb into elastic:main May 21, 2024

kibanamachine added v8.15.0 backport:skip This PR does not require backporting labels May 21, 2024

Conversation

juliaElastic commented May 13, 2024 • edited by jen-huang Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Add integration

Add agent flyout

Open question:

Checklist

Uh oh!

ghost commented May 13, 2024

🤖 GitHub comments

Uh oh!

juliaElastic commented May 13, 2024

Uh oh!

juliaElastic commented May 13, 2024

Uh oh!

juliaElastic commented May 14, 2024

Uh oh!

juliaElastic commented May 14, 2024

Uh oh!

Uh oh!

elasticmachine commented May 14, 2024

Uh oh!

kilfoyle left a comment

Choose a reason for hiding this comment

Uh oh!

juliaElastic May 14, 2024

Choose a reason for hiding this comment

Uh oh!

jen-huang May 14, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nchaulet left a comment

Choose a reason for hiding this comment

Uh oh!

jen-huang left a comment

Choose a reason for hiding this comment

Uh oh!

jen-huang May 16, 2024

Choose a reason for hiding this comment

Uh oh!

juliaElastic May 17, 2024

Choose a reason for hiding this comment

Uh oh!

juliaElastic May 17, 2024

Choose a reason for hiding this comment

Uh oh!

juliaElastic commented May 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kpollich commented May 17, 2024

Uh oh!

juliaElastic commented May 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jen-huang left a comment

Choose a reason for hiding this comment

Uh oh!

nimarezainia commented May 20, 2024

Uh oh!

juliaElastic commented May 21, 2024

Uh oh!

kibana-ci commented May 21, 2024

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Page load bundle

API count

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

juliaElastic commented May 13, 2024 •

edited by jen-huang

Loading

juliaElastic commented May 17, 2024 •

edited

Loading

juliaElastic commented May 17, 2024 •

edited

Loading