Skip to content

[Entity Analytics] Asset Criticality soft delete#193591

Merged
tiansivive merged 10 commits intoelastic:mainfrom
tiansivive:ea-entity-store-10531
Sep 25, 2024
Merged

[Entity Analytics] Asset Criticality soft delete#193591
tiansivive merged 10 commits intoelastic:mainfrom
tiansivive:ea-entity-store-10531

Conversation

@tiansivive
Copy link
Copy Markdown
Contributor

@tiansivive tiansivive commented Sep 20, 2024

Summary

This PR introduces a "soft delete" for Asset Criticality records. Instead of deleting the document, we now simply change the criticality field to the value "deleted".
This value is fully managed on Kibana and should not be exposed to the client side.

How to test

  1. Add some host/user data
  2. Make sure to enable the entityStoreEnabled feature flag
  3. Assign asset criticality to a user/host.
    • You may need to enable Asset Criticality in Kibana Advanced Settings
  4. Verify the criticality has been assigned by GET .asset-citicality*.
  5. Unassign the criticality for that host/user via the UI.
  6. GETing the criticality index should now still show the updated record with "deleted" criticality value
  7. The Ui should also show criticality as Unassigned for the deleted record.

Implements https://github.com/elastic/security-team/issues/10531, which is part of the overall Entity Store for 8.16 work

@tiansivive tiansivive marked this pull request as ready for review September 23, 2024 10:39
@tiansivive tiansivive requested a review from a team as a code owner September 23, 2024 10:39
@tiansivive tiansivive added 8.16 candidate release_note:skip Skip the PR/issue when compiling release notes backport:prev-minor Feature:Entity Analytics Security Solution Entity Analytics features Team:Entity Analytics Security Entity Analytics Team labels Sep 23, 2024
@elasticmachine
Copy link
Copy Markdown
Contributor

Pinging @elastic/security-entity-analytics (Team:Entity Analytics)

@tiansivive tiansivive added this to the 8.16 milestone Sep 23, 2024
@tiansivive tiansivive enabled auto-merge (squash) September 23, 2024 14:23
Copy link
Copy Markdown
Member

@machadoum machadoum left a comment

Choose a reason for hiding this comment

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

Hey!

I checked the code and dek-tested it. Everything looks great. 👏 👏 👏

I have a couple of questions:

  • Could you add a test that verifies if the risk score doesn't consider deleted asset criticalities?
  • Should we also support delete as a valid value for bulk upload? [not in the PR scope]

Thank you! 👏

} & Pick<Parameters<ElasticsearchClient['helpers']['bulk']>[0], 'flushBytes' | 'retries'>;

type StoredAssetCriticalityRecord = {
[K in keyof AssetCriticalityRecord]: K extends 'criticality_level'
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Math_Lady_meme

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I got it!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yeah, idk, any suggestions to improve readability here? 😅
TS syntax is quite verbose for type computations, but maybe we can extract out the behaviour of "setting the type of a value for a specific key K"?

@tiansivive
Copy link
Copy Markdown
Contributor Author

  • Should we also support delete as a valid value for bulk upload? [not in the PR scope]

I don't see a use case for it off the top of my head. Maybe @jaredburgettelastic can think of something?

@tiansivive
Copy link
Copy Markdown
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Copy Markdown

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

Copy link
Copy Markdown
Member

@machadoum machadoum left a comment

Choose a reason for hiding this comment

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

LGTM!

A very soft PR! ❤️

@tiansivive tiansivive merged commit a8c7e06 into elastic:main Sep 25, 2024
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Sep 25, 2024
## Summary

This PR introduces a "soft delete" for Asset Criticality records.
Instead of deleting the document, we now simply change the criticality
field to the value `"deleted"`.
This value is fully managed on Kibana and should not be exposed to the
client side.

### How to test

1. Add some host/user data
2. Make sure to enable the `entityStoreEnabled` feature flag
3. Assign asset criticality to a user/host.
* You may need to enable Asset Criticality in Kibana Advanced Settings
5. Verify the criticality has been assigned by `GET .asset-citicality*`.
6. Unassign the criticality for that host/user via the UI.
7. `GET`ing the criticality index should now still show the updated
record with `"deleted"` criticality value
8. The Ui should also show criticality as `Unassigned` for the deleted
record.

Implements elastic/security-team#10531, which
is part of the overall [Entity Store for
8.16](elastic/security-team#10529) work

(cherry picked from commit a8c7e06)
@kibanamachine
Copy link
Copy Markdown
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 25, 2024
…4010)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[Entity Analytics] Asset Criticality soft delete
(#193591)](#193591)

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

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

<!--BACKPORT [{"author":{"name":"Tiago Vila
Verde","email":"tiago.vilaverde@elastic.co"},"sourceCommit":{"committedDate":"2024-09-25T13:54:30Z","message":"[Entity
Analytics] Asset Criticality soft delete (#193591)\n\n##
Summary\r\n\r\nThis PR introduces a \"soft delete\" for Asset
Criticality records.\r\nInstead of deleting the document, we now simply
change the criticality\r\nfield to the value `\"deleted\"`.\r\nThis
value is fully managed on Kibana and should not be exposed to
the\r\nclient side.\r\n\r\n\r\n### How to test\r\n\r\n1. Add some
host/user data\r\n2. Make sure to enable the `entityStoreEnabled`
feature flag\r\n3. Assign asset criticality to a user/host.\r\n* You may
need to enable Asset Criticality in Kibana Advanced Settings\r\n5.
Verify the criticality has been assigned by `GET
.asset-citicality*`.\r\n6. Unassign the criticality for that host/user
via the UI.\r\n7. `GET`ing the criticality index should now still show
the updated\r\nrecord with `\"deleted\"` criticality value\r\n8. The Ui
should also show criticality as `Unassigned` for the
deleted\r\nrecord.\r\n\r\n\r\n\r\nImplements
elastic/security-team#10531, which\r\nis part
of the overall [Entity Store
for\r\n8.16](elastic/security-team#10529)
work","sha":"a8c7e0659635994546874fb1f7ec1304fa4f8353","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","backport:prev-minor","Feature:Entity
Analytics","Team:Entity Analytics","8.16
candidate","v8.16.0"],"title":"[Entity Analytics] Asset Criticality soft
delete","number":193591,"url":"https://github.com/elastic/kibana/pull/193591","mergeCommit":{"message":"[Entity
Analytics] Asset Criticality soft delete (#193591)\n\n##
Summary\r\n\r\nThis PR introduces a \"soft delete\" for Asset
Criticality records.\r\nInstead of deleting the document, we now simply
change the criticality\r\nfield to the value `\"deleted\"`.\r\nThis
value is fully managed on Kibana and should not be exposed to
the\r\nclient side.\r\n\r\n\r\n### How to test\r\n\r\n1. Add some
host/user data\r\n2. Make sure to enable the `entityStoreEnabled`
feature flag\r\n3. Assign asset criticality to a user/host.\r\n* You may
need to enable Asset Criticality in Kibana Advanced Settings\r\n5.
Verify the criticality has been assigned by `GET
.asset-citicality*`.\r\n6. Unassign the criticality for that host/user
via the UI.\r\n7. `GET`ing the criticality index should now still show
the updated\r\nrecord with `\"deleted\"` criticality value\r\n8. The Ui
should also show criticality as `Unassigned` for the
deleted\r\nrecord.\r\n\r\n\r\n\r\nImplements
elastic/security-team#10531, which\r\nis part
of the overall [Entity Store
for\r\n8.16](elastic/security-team#10529)
work","sha":"a8c7e0659635994546874fb1f7ec1304fa4f8353"}},"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/193591","number":193591,"mergeCommit":{"message":"[Entity
Analytics] Asset Criticality soft delete (#193591)\n\n##
Summary\r\n\r\nThis PR introduces a \"soft delete\" for Asset
Criticality records.\r\nInstead of deleting the document, we now simply
change the criticality\r\nfield to the value `\"deleted\"`.\r\nThis
value is fully managed on Kibana and should not be exposed to
the\r\nclient side.\r\n\r\n\r\n### How to test\r\n\r\n1. Add some
host/user data\r\n2. Make sure to enable the `entityStoreEnabled`
feature flag\r\n3. Assign asset criticality to a user/host.\r\n* You may
need to enable Asset Criticality in Kibana Advanced Settings\r\n5.
Verify the criticality has been assigned by `GET
.asset-citicality*`.\r\n6. Unassign the criticality for that host/user
via the UI.\r\n7. `GET`ing the criticality index should now still show
the updated\r\nrecord with `\"deleted\"` criticality value\r\n8. The Ui
should also show criticality as `Unassigned` for the
deleted\r\nrecord.\r\n\r\n\r\n\r\nImplements
elastic/security-team#10531, which\r\nis part
of the overall [Entity Store
for\r\n8.16](elastic/security-team#10529)
work","sha":"a8c7e0659635994546874fb1f7ec1304fa4f8353"}},{"branch":"8.x","label":"v8.16.0","branchLabelMappingKey":"^v8.16.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Tiago Vila Verde <tiago.vilaverde@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

8.16 candidate Feature:Entity Analytics Security Solution Entity Analytics features release_note:skip Skip the PR/issue when compiling release notes Team:Entity Analytics Security Entity Analytics Team v8.16.0 v9.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants