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

[Security Solution][Detections] Extend alerts schema to accommodate the list of assigned users (#7647) #166845

Conversation

e40pud
Copy link
Contributor

@e40pud e40pud commented Sep 20, 2023

Summary

Closes https://github.com/elastic/security-team/issues/7647

This PR extends alert's schema. We add a new field kibana.alert.workflow_assignee_ids where assignees will live.

@e40pud e40pud added Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. release_note:feature Makes this part of the condensed release notes Team:Detection Engine Security Solution Detection Engine Area labels Sep 20, 2023
@e40pud e40pud self-assigned this Sep 20, 2023
@e40pud e40pud marked this pull request as ready for review September 20, 2023 19:39
@e40pud e40pud requested review from a team as code owners September 20, 2023 19:39
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

@e40pud e40pud requested review from yctercero, marshallmain and a team and removed request for a team September 20, 2023 19:41
@e40pud e40pud requested a review from a team September 21, 2023 13:11
export type { Ancestor890 as Ancestor8110 };

export interface BaseFields8110 extends BaseFields890 {
[ALERT_WORKFLOW_ASSIGNEE_IDS]: string[];
Copy link
Contributor

Choose a reason for hiding this comment

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

as ALERT_WORKFLOW_ASSIGNEE_IDS defined as not required

  [ALERT_WORKFLOW_ASSIGNEE_IDS]: {
    type: 'keyword',
    array: true,
    required: false,
  },

should it be added here as optional property?

Copy link
Contributor Author

@e40pud e40pud Sep 21, 2023

Choose a reason for hiding this comment

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

good question, I mimicked the same behaviour we have for workflow_tags that we introduced in 8.9 https://github.com/elastic/kibana/blob/main/x-pack/plugins/security_solution/common/api/detection_engine/model/alerts/8.9.0/index.ts#L29. It is also not required.

@dplumlee do you remember why we skipped making tags optional property?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made this field optional, @dplumlee let me know if there is a reason to keep it required here.

Copy link
Contributor

@dplumlee dplumlee Sep 22, 2023

Choose a reason for hiding this comment

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

I don't recall if there was a specific reason for not making the tags optional here, I believe since it was a new field, our use case just wanted to always count on it being there. I think the right way to go is keeping it aligned with the field maps definition as @vitaliidm points out though, I probably shouldn't have listed ours as optional in that file.

Copy link
Contributor

@stephmilovic stephmilovic left a comment

Choose a reason for hiding this comment

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

LGTM from explore side, thanks!

Copy link
Contributor

@vitaliidm vitaliidm left a comment

Choose a reason for hiding this comment

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

it looks good to me
Left comment on optional typing: with undefined it still will be required to pass everywhere, so proposing to use question mark
just one, note, once you make ALERT_WORKFLOW_ASSIGNEE_IDS I don't think you need to add empty array everywhere

@kibana-ci
Copy link
Collaborator

💔 Build Failed

Failed CI Steps

Metrics [docs]

‼️ ERROR: no builds found for mergeBase sha [d508c9e]

History

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

cc @e40pud

@e40pud e40pud merged commit 23f64ed into elastic:security/feature/alert-user-assignment Sep 22, 2023
32 of 33 checks passed
@e40pud e40pud deleted the security/feature/alert-user-assignment-7647 branch October 11, 2023 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:feature Makes this part of the condensed release notes Team:Detection Engine Security Solution Detection Engine Area Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants