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

[EDR Workflows] Fix agent count for single agent policies #194294

Merged
merged 5 commits into from
Sep 27, 2024

Conversation

szwarckonrad
Copy link
Contributor

@szwarckonrad szwarckonrad commented Sep 27, 2024

Screen.Recording.2024-09-27.at.22.28.05.mov

The PR #193705 introduced an issue when counting active agents for integration policies with only one agent policy assigned. In such cases, query.policyIds was treated as a single string instead of an array of strings (as expected with multiple agent policy ids like /?policyIds=x&policyIds=y). This PR resolves the issue by ensuring consistent handling of policyIds, regardless of the number of associated agent policies.

@szwarckonrad szwarckonrad added release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v9.0.0 Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v8.16.0 backport:version Backport to applied version labels labels Sep 27, 2024
@szwarckonrad szwarckonrad self-assigned this Sep 27, 2024
@szwarckonrad szwarckonrad requested review from a team as code owners September 27, 2024 12:05
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@elasticmachine
Copy link
Contributor

Pinging @elastic/security-defend-workflows (Team:Defend Workflows)

Copy link
Contributor

@tomsonpl tomsonpl left a comment

Choose a reason for hiding this comment

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

lgtm 🚀 , thank you!

@@ -96,7 +96,7 @@ export const policySettingsMiddlewareRunner: MiddlewareRunner = async (

// Agent summary is secondary data, so its ok for it to come after the details
// page is populated with the main content
if (policyItem.policy_id) {
if (policyItem.policy_ids.length) {
Copy link
Contributor

Choose a reason for hiding this comment

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

does policy_ids always exist?

Copy link
Contributor Author

@szwarckonrad szwarckonrad Sep 27, 2024

Choose a reason for hiding this comment

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

Although we expect policy_ids to always be defined, I noticed some parts of the code where this check is still performed. I’ve added a safe .length check here: f9d85f6. Thanks for bringing this to my attention!

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.

Makes sense 👌. I haven't dug into it but is it possible to update the places where we're sending policyIds as a string instead of allowing policyIds to be string[] | string?

@szwarckonrad
Copy link
Contributor Author

Makes sense 👌. I haven't dug into it but is it possible to update the places where we're sending policyIds as a string instead of allowing policyIds to be string[] | string?

We are always sending policyIds as string[], however, in the request query field they would be parsed as strings if there was only one of them.

@@ -323,13 +323,22 @@ export const getAgentStatusForAgentPolicyHandler: FleetRequestHandler<
const [coreContext, fleetContext] = await Promise.all([context.core, context.fleet]);
const esClient = coreContext.elasticsearch.client.asInternalUser;
const soClient = fleetContext.internalSoClient;

const parsePolicyIds = (policyIds: string | string[] | undefined): string[] | undefined => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add some tests to cover these changes? It would be good especially to test the behaviour of getAgentStatusForAgentPolicy in the different cases. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@criamico getAgentStatusForAgentPolicy has coverage here -

describe('getAgentStatusForAgentPolicy', () => {

I've added a coverage for the change of the current PR - 51f9b81

Copy link
Member

@kpollich kpollich left a comment

Choose a reason for hiding this comment

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

LGTM - thank you for adding test coverage.

Copy link
Contributor

@criamico criamico left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Thanks for adding tests!

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #31 / Detection Engine - Exception workflows APIs @serverless @serverlessQA @ess rule exceptions execution creating rules with exceptions should be able to execute against an exception list that does include valid case sensitive entries and get back 0 alerts

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
securitySolution 20.5MB 20.5MB +80.0B
Unknown metric groups

References to deprecated APIs

id before after diff
securitySolution 459 457 -2

History

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

cc @szwarckonrad

@szwarckonrad szwarckonrad merged commit 847285b into elastic:main Sep 27, 2024
43 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/11077876606

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Sep 27, 2024
…4294)

https://github.com/user-attachments/assets/2b64c1e0-0e6d-4ef5-952d-e4364b4403c4

The PR elastic#193705 introduced an issue when counting active agents for
integration policies with only one agent policy assigned. In such cases,
`query.policyIds` was treated as a single string instead of an array of
strings (as expected with multiple agent policy ids like
`/?policyIds=x&policyIds=y`). This PR resolves the issue by ensuring
consistent handling of policyIds, regardless of the number of associated
agent policies.

(cherry picked from commit 847285b)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Sep 27, 2024
) (#194351)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[EDR Workflows] Fix agent count for single agent policies
(#194294)](#194294)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Konrad
Szwarc","email":"konrad.szwarc@elastic.co"},"sourceCommit":{"committedDate":"2024-09-27T21:22:28Z","message":"[EDR
Workflows] Fix agent count for single agent policies
(#194294)\n\nhttps://github.com/user-attachments/assets/2b64c1e0-0e6d-4ef5-952d-e4364b4403c4\r\n\r\n\r\n\r\nThe
PR #193705 introduced an issue when counting active agents
for\r\nintegration policies with only one agent policy assigned. In such
cases,\r\n`query.policyIds` was treated as a single string instead of an
array of\r\nstrings (as expected with multiple agent policy ids
like\r\n`/?policyIds=x&policyIds=y`). This PR resolves the issue by
ensuring\r\nconsistent handling of policyIds, regardless of the number
of associated\r\nagent
policies.","sha":"847285ba7191aa6d26fb3dccc06748e1c4a202b1","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Team:Fleet","v9.0.0","Team:Defend
Workflows","v8.16.0","backport:version"],"title":"[EDR Workflows] Fix
agent count for single agent
policies","number":194294,"url":"https://github.com/elastic/kibana/pull/194294","mergeCommit":{"message":"[EDR
Workflows] Fix agent count for single agent policies
(#194294)\n\nhttps://github.com/user-attachments/assets/2b64c1e0-0e6d-4ef5-952d-e4364b4403c4\r\n\r\n\r\n\r\nThe
PR #193705 introduced an issue when counting active agents
for\r\nintegration policies with only one agent policy assigned. In such
cases,\r\n`query.policyIds` was treated as a single string instead of an
array of\r\nstrings (as expected with multiple agent policy ids
like\r\n`/?policyIds=x&policyIds=y`). This PR resolves the issue by
ensuring\r\nconsistent handling of policyIds, regardless of the number
of associated\r\nagent
policies.","sha":"847285ba7191aa6d26fb3dccc06748e1c4a202b1"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/194294","number":194294,"mergeCommit":{"message":"[EDR
Workflows] Fix agent count for single agent policies
(#194294)\n\nhttps://github.com/user-attachments/assets/2b64c1e0-0e6d-4ef5-952d-e4364b4403c4\r\n\r\n\r\n\r\nThe
PR #193705 introduced an issue when counting active agents
for\r\nintegration policies with only one agent policy assigned. In such
cases,\r\n`query.policyIds` was treated as a single string instead of an
array of\r\nstrings (as expected with multiple agent policy ids
like\r\n`/?policyIds=x&policyIds=y`). This PR resolves the issue by
ensuring\r\nconsistent handling of policyIds, regardless of the number
of associated\r\nagent
policies.","sha":"847285ba7191aa6d26fb3dccc06748e1c4a202b1"}},{"branch":"8.x","label":"v8.16.0","branchLabelMappingKey":"^v8.16.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Konrad Szwarc <konrad.szwarc@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:version Backport to applied version labels 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.16.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Defend workflows][Serverless]Agent count is wrong on the policy details page
8 participants