-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Entity Analytics] Internal API to upload asset criticality records via CSV file #179930
[Entity Analytics] Internal API to upload asset criticality records via CSV file #179930
Conversation
bd8e992
to
71f393c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Detection and Response LGTM
@hop-dev The changes here aren't really owned by Detection and Response. I left comments about moving the code from common files to entity analytics domain where ownership is limited to entity analytics.
@@ -178,6 +179,7 @@ export const initRoutes = ( | |||
assetCriticalityGetRoute(router, logger); | |||
assetCriticalityDeleteRoute(router, logger); | |||
assetCriticalityPrivilegesRoute(router, getStartServices, logger); | |||
assetCriticalityCSVUploadRoute(router, logger, config, getStartServices); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While it's possible to register individual routes here it's much better to incapsulate the details. I recommend to move all assetCriticality...
route registrations into registerAssetCriticalityRoutes()
function. This function file should have explicit ownership.
@@ -272,6 +272,7 @@ export const RISK_ENGINE_SETTINGS_URL = `${RISK_ENGINE_URL}/settings`; | |||
export const ASSET_CRITICALITY_URL = `/internal/asset_criticality`; | |||
export const ASSET_CRITICALITY_PRIVILEGES_URL = `/internal/asset_criticality/privileges`; | |||
export const ASSET_CRITICALITY_STATUS_URL = `${ASSET_CRITICALITY_URL}/status`; | |||
export const ASSET_CRITICALITY_CSV_UPLOAD_URL = `${ASSET_CRITICALITY_URL}/upload_csv`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend to move ASSET_CRITICALITY...
constants into Asset criticality domain. Create common/asset_criticality
folder with constants.ts
. It will help to define proper ownership avoid expanding common/constants.ts
with not really common constants.
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: cc @hop-dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SDA LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
## Summary Create a new Asset Criticality page for updating asset criticality by file upload. Flaky test runner: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5662 Server side PR: #179930 https://github.com/elastic/kibana/assets/1490444/f524b5e8-8efa-40c7-8e43-45cf43decefb The new page has three steps. You can access the page by going to Security -> Manage -> Asset Criticality. <img src="https://github.com/elastic/kibana/assets/1490444/080a51bf-20e9-4f4b-84b2-13fe1cfdc1d5" width="400" /> ### File picker Step: <img src="https://github.com/elastic/kibana/assets/1490444/e3aea4b8-2083-49a4-b4bf-dbb645fb463b" width="400" /> ### File validation step <img src="https://github.com/elastic/kibana/assets/1490444/54b3018e-ef0e-4ac4-93b2-67ae02743eb8" width="400" /> ### Result step <img src="https://github.com/elastic/kibana/assets/1490444/aa47a7af-1108-4ad6-8dc0-f728e0187026" width="400" /> ### Checklist Delete any items that are not applicable to this PR. - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [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 - [x] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [x] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) a-docker) - [x] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) ## How to test it? * Open the page * Upload a valid CSV file * Check if everything is ok on the validation step * Click Assign * Check if the success message is displayed * Open the alert flyout for an updated asset and check if it has the new value ## What is not included? * Serverless * Disable the feature when asset criticality advanced setting is disabled ## Code owners files: <details> <summary>elastic/docs</summary> * packages/kbn-doc-links/src/get_doc_links.ts * packages/kbn-doc-links/src/types.ts </details> <details> <summary>elastic/security-defend-workflows</summary> * x-pack/plugins/security_solution/public/management/links.ts </details> <details> <summary>elastic/security-detection-engine</summary> * x-pack/test/security_solution_cypress/cypress/urls/navigation.ts </details> <details> <summary>elastic/security-detections-response</summary> * x-pack/test/security_solution_cypress/cypress/fixtures/asset_criticality.csv </details> <details> <summary>elastic/security-engineering-productivity</summary> * x-pack/test/security_solution_cypress/cypress/e2e/entity_analytics/asset_criticality_upload_page.cy.ts * x-pack/test/security_solution_cypress/cypress/fixtures/asset_criticality.csv * x-pack/test/security_solution_cypress/cypress/screens/asset_criticality.ts * x-pack/test/security_solution_cypress/cypress/tasks/asset_criticality.ts * x-pack/test/security_solution_cypress/cypress/urls/navigation.ts </details> <details> <summary>elastic/security-threat-hunting</summary> * x-pack/test/security_solution_cypress/cypress/fixtures/asset_criticality.csv </details> <details> <summary>elastic/security-threat-hunting-investigations</summary> * x-pack/plugins/security_solution/public/resolver/view/panels/node_list.tsx * x-pack/test/security_solution_cypress/cypress/urls/navigation.ts </details> --------- Co-authored-by: Mark Hopkin <[email protected]>
…e entity analytics team (#180702) ## Summary Closes #180531 This pull request moves entity analytics route registration and url definition into files owned by our team. Currently, to add a new route we require a code owners review from both the `security-detections-response` and `security-threat-hunting` teams unnecessarily. This is because we needed to change the following files: - `x-pack/plugins/security_solution/common/constants.ts` - `x-pack/plugins/security_solution/server/routes/index.ts` As recommended by @maximpn [here](#179930 (review)) I have also removed redundant feature flag checks for enabling risk scoring and risk engine privileges routes, these feature flags are enabled now. --------- Co-authored-by: Kibana Machine <[email protected]>
Summary
Adds a new internal API to provide a CSV file of asset criticality records to bulk upload.
The CSV has some constraints:
Here is an example of a good file:
We send the updates to elasticsearch using the bulk API stream helper, this allows us to configure the max request size and the number of retries on error. I have mapped these to config just incase we ever need to change them for a certain env. Here are the keys and their defaults:
Example Curl with response:
Errors are reported line by line, here are some examples: