[SecuritySolution][Detections] Adds SavedObject persistence to Signals Migrations #85690
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rylnd
added
release_note:enhancement
v8.0.0
v7.11.0
Team:Detections and Resp
Trying to forge a patterrn using io-ts to validate at runtime. I think I've got it working but I want to refactor the pipeline out into a reusable function(s).
* Defines a simple client that delegates to the base SO client with our SO type * Defines a service that consumes the simpler client, adding validations and data transforms on top.
As opposed to the previous ephemeral, encoded tokens, we now retrieve migration info from saved objects. At the API level, this means that both the create and finalize endpoints receive a list of concrete indices. No more passing around tokens. As both endpoints are idempotent, users can hammer them as much as they want with the same lists of indices. Redundant creates and finalizes will be met with inline 400 messages, and as one continues to poll the finalize endpoint they should see more and more indices respond with "completed: true"
* standardize assignment of responses (with types) * deletes migration SOs as test cleanup
This was getting big and unwieldy; this splits these into one file per endpoint.
…functions This will allow us to repurpose the service to compose more functionality and be more specifically useful, while keeping the component logic separate.
getMigrationStatus was really two separate aggregations, so I split them out and we recompose them in the necessary routes.
* migrationService exposes this as .finalize()
* adds an error field to our migration SO
* We currently only have one error that we persist there, but it would
be very time-consuming to track down that information were it not
there.
* migrationService leverages this function * adds new boolean to our savedObject * deletes unused function (deleteMigrationSavedObject)
* Adding/updating mocks/unit tests necessary to satisfy the things I need to test * I mainly wanted to test that the the status endpoint filtered out the deleted migrations; this was accomplished with a unit test after fleshing out some mocks/sample data.
This logic was previously moved out into component functions; this moves the tests accordingly.
Mocks out our migration functions, rather than stubbing ES calls directly.
Addresses functionality that hasn't been moved to finalizeMigration()
Fixes a bug where we weren't accounting for soft-deleted migrations. ALso updates our test migration SO to have a status of 'pending' as that's a more useful default.
These were failing due: * a change in the migration status API response * a bug I introduced in the finalize route
* unit tests * API integration tests * Caught/fixed bug with deleting a successful migration
Removes unused code.
rylnd
force-pushed
the
signals_migration_so
branch
from
e4196fb to
1ca0d24
Compare
If a user has an out of date index (v1) relative to the template (v2), but the template itself is out of date (newest is v3), then it's possible that the template is rolled over to v3 after the v1-v2 migration has been created but before the new index has been created. In such a case, the new index would receive the v3 mappings but would incorrectl be marked as v2. This shouldn't necessarily be an issue, but it's an unnecessary state that can easily be prevented with the guard introduced here.
In addition to the SOs themselves giving us observability into what migration actions were performed, this gives us the additional info of _who_ performed the action.
| ); | ||
| } | ||
| const finalizedMigration = await migrationService.finalize({ | ||
| migration: migrations[0], |
There was a problem hiding this comment.
Perhaps this finalize API should take an array of migration IDs instead of source index names to avoid possible ambiguity with a single source index having multiple migrations
| }, | ||
| async (context, request, response) => { | ||
| const siemResponse = buildSiemResponse(response); | ||
| const { index: indices } = request.body; |
There was a problem hiding this comment.
Similar to the finalize API, I think it would be safer to use a guaranteed unique identifier like the SO id when deleting the migration SOs.
| */ | ||
| export const getSignalVersionsByIndex = async ({ | ||
| esClient, | ||
| index, |
There was a problem hiding this comment.
nit: indices would be a clearer name
| */ | ||
| export const getIndexVersionsByIndex = async ({ | ||
| esClient, | ||
| index, |
|
|
||
| /** | ||
| * "Deletes" a completed migration: | ||
| * * soft-deletes the migration SO |
There was a problem hiding this comment.
What's the advantage of leaving the migration SO in a soft-deleted state here? In my mind we'd want the task document and the migration SO to have the same lifetime, but it seems that the SOs would live on indefinitely.
There was a problem hiding this comment.
updated to do a hard-delete in a1d2d6a
This will allow users to finalize a migration if they've lost the response to their POST call.
This disambiguates _which_ migrations we were finalizing if you passed an index (which was previously: the most recent migration).
Discussions with @marshallmain lead to the following refactor: * finalize does not delete tasks * finalize only applies cleanup policy to a failed migration * delete takes an array of migration ids (like finalize) * delete hard-deletes the SavedObject of a completed (failed or successful) migration This gives a bit more flexibility with the endpoints, as well as disambiguates the semantics: it just deletes migrations!
I removed some logic here but forgot the imports :(
In the case of a successful migration, application of the cleanup policy is done by the deletion endpoint. In the interest of data preservation, we do not delete a sourceIndex unless it is explicitly deleted.
spong
approved these changes
Dec 15, 2020
marshallmain
approved these changes
Dec 15, 2020
|
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]Module Count
Async chunks
Distributable file count
Page load bundle
Saved Objects .kibana field count
History
To update your PR or re-run it, just comment with: |
rylnd
added a commit
to rylnd/kibana
that referenced
this pull request
Dec 15, 2020
…s Migrations (elastic#85690) * Adds new SO type for persisting our signals migrations * WIP: Migration status SO client Trying to forge a patterrn using io-ts to validate at runtime. I think I've got it working but I want to refactor the pipeline out into a reusable function(s). * Implements our SavedObjects service for signals migrations * Defines a simple client that delegates to the base SO client with our SO type * Defines a service that consumes the simpler client, adding validations and data transforms on top. * Refactoring migration code to work with saved objects As opposed to the previous ephemeral, encoded tokens, we now retrieve migration info from saved objects. At the API level, this means that both the create and finalize endpoints receive a list of concrete indices. No more passing around tokens. As both endpoints are idempotent, users can hammer them as much as they want with the same lists of indices. Redundant creates and finalizes will be met with inline 400 messages, and as one continues to poll the finalize endpoint they should see more and more indices respond with "completed: true" * Fixing integration tests first, and anything upstream breaking them * Clean up API integration tests * standardize assignment of responses (with types) * deletes migration SOs as test cleanup * Split API tests into separate files This was getting big and unwieldy; this splits these into one file per endpoint. * Refactor: split existing migration service functionality into atomic functions This will allow us to repurpose the service to compose more functionality and be more specifically useful, while keeping the component logic separate. * WIP: moving logic into migrationService.create * Splitting get_migration_status into component functions getMigrationStatus was really two separate aggregations, so I split them out and we recompose them in the necessary routes. * Move finalization logic into function * migrationService exposes this as .finalize() * adds an error field to our migration SO * We currently only have one error that we persist there, but it would be very time-consuming to track down that information were it not there. * Adds function for migration "deletion" logic * migrationService leverages this function * adds new boolean to our savedObject * deletes unused function (deleteMigrationSavedObject) * Adds route for soft-deletion of migrations * Updating tests related to migration status * Adding/updating mocks/unit tests necessary to satisfy the things I need to test * I mainly wanted to test that the the status endpoint filtered out the deleted migrations; this was accomplished with a unit test after fleshing out some mocks/sample data. * Move old migration service tests to the relevant function tests This logic was previously moved out into component functions; this moves the tests accordingly. * Add some unit tests around our reindex call * Fix create migration route tests Mocks out our migration functions, rather than stubbing ES calls directly. * Updates finalize route unit tests Addresses functionality that hasn't been moved to finalizeMigration() * Unit tests our finalization logic Fixes a bug where we weren't accounting for soft-deleted migrations. ALso updates our test migration SO to have a status of 'pending' as that's a more useful default. * Fixes finalization integration tests These were failing due: * a change in the migration status API response * a bug I introduced in the finalize route * Adds tests for our migration deletion endpoint * unit tests * API integration tests * Caught/fixed bug with deleting a successful migration * Fixes types Removes unused code. * Prevent race condition due to template rollover during migration If a user has an out of date index (v1) relative to the template (v2), but the template itself is out of date (newest is v3), then it's possible that the template is rolled over to v3 after the v1-v2 migration has been created but before the new index has been created. In such a case, the new index would receive the v3 mappings but would incorrectl be marked as v2. This shouldn't necessarily be an issue, but it's an unnecessary state that can easily be prevented with the guard introduced here. * Add real usernames to migration savedObjects In addition to the SOs themselves giving us observability into what migration actions were performed, this gives us the additional info of _who_ performed the action. * Index minimal migration SO fields needed for current functionality * Add additional migration info to status endpoint This will allow users to finalize a migration if they've lost the response to their POST call. * Finalize endpoint receives an array of migration IDs, not indices This disambiguates _which_ migrations we were finalizing if you passed an index (which was previously: the most recent migration). * Fix type errors in tests after we threaded through username * Update responsibilities of migration finalize/delete endpoints Discussions with @marshallmain lead to the following refactor: * finalize does not delete tasks * finalize only applies cleanup policy to a failed migration * delete takes an array of migration ids (like finalize) * delete hard-deletes the SavedObject of a completed (failed or successful) migration This gives a bit more flexibility with the endpoints, as well as disambiguates the semantics: it just deletes migrations! * Fix tests that were broken during refactoring * Fix type errors I removed some logic here but forgot the imports :( * Move outdated integration test In the case of a successful migration, application of the cleanup policy is done by the deletion endpoint. In the interest of data preservation, we do not delete a sourceIndex unless it is explicitly deleted. Co-authored-by: Kibana Machine <[email protected]>
gmmorris
added a commit
to ymao1/kibana
that referenced
this pull request
Dec 15, 2020
* master: (66 commits) [Alerting] fixes broken Alerting Example plugin (elastic#85774) [APM] Service overview instances table (elastic#85770) [Security Solution] Unskip timeline creation Cypress test (elastic#85871) properly recognize enterprise licenses (elastic#85849) [SecuritySolution][Detections] Adds SavedObject persistence to Signals Migrations (elastic#85690) [TSVB] Fix functional tests flakiness and unskip them (elastic#85388) [Fleet] Change permissions for Fleet enroll role (elastic#85802) Gauge visualization can no longer be clicked to filter on values since Kibana 7.10.0 (elastic#84768) [Security Solution][Detections] Add alert source to detection rule action context (elastic#85488) [Discover] Don't display hide/show button for histogram when there's no time filter (elastic#85424) skip flaky suite (elastic#78553) License checks for alerts plugin (elastic#85649) skip flaky suite (elastic#84992) skip 'query return results valid for scripted field' elastic#78553 Allow action types to perform their own mustache variable escaping in parameter templates (elastic#83919) [ML] More machine learning links in doc_links_service.ts (elastic#85365) Removed Alerting & Event Log deprecated fields that should not be using (elastic#85652) Closes elastic#79995 by adding new tab in transaction details to show related trace logs. (elastic#85859) Fix outdated jest snapshot [Maps] Surface on prem EMS (elastic#85729) ...
rylnd
added a commit
that referenced
this pull request
Dec 15, 2020
…Signals Migrations (#85690) (#85920) * [SecuritySolution][Detections] Adds SavedObject persistence to Signals Migrations (#85690) * Adds new SO type for persisting our signals migrations * WIP: Migration status SO client Trying to forge a patterrn using io-ts to validate at runtime. I think I've got it working but I want to refactor the pipeline out into a reusable function(s). * Implements our SavedObjects service for signals migrations * Defines a simple client that delegates to the base SO client with our SO type * Defines a service that consumes the simpler client, adding validations and data transforms on top. * Refactoring migration code to work with saved objects As opposed to the previous ephemeral, encoded tokens, we now retrieve migration info from saved objects. At the API level, this means that both the create and finalize endpoints receive a list of concrete indices. No more passing around tokens. As both endpoints are idempotent, users can hammer them as much as they want with the same lists of indices. Redundant creates and finalizes will be met with inline 400 messages, and as one continues to poll the finalize endpoint they should see more and more indices respond with "completed: true" * Fixing integration tests first, and anything upstream breaking them * Clean up API integration tests * standardize assignment of responses (with types) * deletes migration SOs as test cleanup * Split API tests into separate files This was getting big and unwieldy; this splits these into one file per endpoint. * Refactor: split existing migration service functionality into atomic functions This will allow us to repurpose the service to compose more functionality and be more specifically useful, while keeping the component logic separate. * WIP: moving logic into migrationService.create * Splitting get_migration_status into component functions getMigrationStatus was really two separate aggregations, so I split them out and we recompose them in the necessary routes. * Move finalization logic into function * migrationService exposes this as .finalize() * adds an error field to our migration SO * We currently only have one error that we persist there, but it would be very time-consuming to track down that information were it not there. * Adds function for migration "deletion" logic * migrationService leverages this function * adds new boolean to our savedObject * deletes unused function (deleteMigrationSavedObject) * Adds route for soft-deletion of migrations * Updating tests related to migration status * Adding/updating mocks/unit tests necessary to satisfy the things I need to test * I mainly wanted to test that the the status endpoint filtered out the deleted migrations; this was accomplished with a unit test after fleshing out some mocks/sample data. * Move old migration service tests to the relevant function tests This logic was previously moved out into component functions; this moves the tests accordingly. * Add some unit tests around our reindex call * Fix create migration route tests Mocks out our migration functions, rather than stubbing ES calls directly. * Updates finalize route unit tests Addresses functionality that hasn't been moved to finalizeMigration() * Unit tests our finalization logic Fixes a bug where we weren't accounting for soft-deleted migrations. ALso updates our test migration SO to have a status of 'pending' as that's a more useful default. * Fixes finalization integration tests These were failing due: * a change in the migration status API response * a bug I introduced in the finalize route * Adds tests for our migration deletion endpoint * unit tests * API integration tests * Caught/fixed bug with deleting a successful migration * Fixes types Removes unused code. * Prevent race condition due to template rollover during migration If a user has an out of date index (v1) relative to the template (v2), but the template itself is out of date (newest is v3), then it's possible that the template is rolled over to v3 after the v1-v2 migration has been created but before the new index has been created. In such a case, the new index would receive the v3 mappings but would incorrectl be marked as v2. This shouldn't necessarily be an issue, but it's an unnecessary state that can easily be prevented with the guard introduced here. * Add real usernames to migration savedObjects In addition to the SOs themselves giving us observability into what migration actions were performed, this gives us the additional info of _who_ performed the action. * Index minimal migration SO fields needed for current functionality * Add additional migration info to status endpoint This will allow users to finalize a migration if they've lost the response to their POST call. * Finalize endpoint receives an array of migration IDs, not indices This disambiguates _which_ migrations we were finalizing if you passed an index (which was previously: the most recent migration). * Fix type errors in tests after we threaded through username * Update responsibilities of migration finalize/delete endpoints Discussions with @marshallmain lead to the following refactor: * finalize does not delete tasks * finalize only applies cleanup policy to a failed migration * delete takes an array of migration ids (like finalize) * delete hard-deletes the SavedObject of a completed (failed or successful) migration This gives a bit more flexibility with the endpoints, as well as disambiguates the semantics: it just deletes migrations! * Fix tests that were broken during refactoring * Fix type errors I removed some logic here but forgot the imports :( * Move outdated integration test In the case of a successful migration, application of the cleanup policy is done by the deletion endpoint. In the interest of data preservation, we do not delete a sourceIndex unless it is explicitly deleted. Co-authored-by: Kibana Machine <[email protected]> * Relax test assertions These 403 messages seem to be slightly different depending on the ES version/configuration as this one just failed on CI. Co-authored-by: Kibana Machine <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This branch moves a lot of files from #84721. In order to assist with review I've created a rebased copy of this work that allows one to see the entirety of these two PRS: diff
security-solution-signals-migrationsignals/migration, that:MigrationServicethat composes those functionsReview Steps (happy path)
SIGNALS_TEMPLATE_VERSIONconstant and then navigating to a detection engine pageGET [your signals-alias])TODO
Checklist
Delete any items that are not applicable to this PR.
For maintainers