[Security Solution][Detections] Signals Migration API #84721
Merged
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
rylnd
force-pushed
the
signals_migration
branch
2 times, most recently
from
1263fd1 to
accc933
Compare
* Removes obsolete endpoints/functions * Adds endpoint for checking the migration status of signals indices * Adds helper functions to represent the logical pieces of answering that question
* triggers reindex for each index * starts implementing followup endpoint to "finalize" after reindexing is finished
Still moving logic around a bunch.
Instead of e.g. `.siem-signals-default-000001-r5`, this will generate `.siem-signals-default-000001-r000005`. This shouldn't matter much, but it may make it easier for users at a glance to see the story of each index.
* Verifies that task matches the specified parameters * Verifies that document counts are the same * updates aliases * finalization endpoint requires both source/dest indexes since we can't determine that from the task itself.
After upgrading a particular signals index, we're left with both the old and new copies of the index. While the former is unlinked, it's still taking up disk space; this ensures that it will eventually be deleted, but gives users enough time to recover data if necessary. This also ensures that, as with the normal signals ILM policy, it is present during our normal sanity checks.
* Moves migration-related routes under signals/ to match their routing * Generalizes migration-agnostic helpers, moves them to appropriate folders (namely index/) * Inlined getMigrationStatusInRange, a hyper-specific function with limited utility elsewhere
This is as much to get my thoughts in order as it is for posterity. Next: tests!
* Adds io-ts schema for route params * Adds es_archiver data to represent an outdated signals index
* Adds io-ts schema for route params * Adds second signals index archive, updates docs * Adds test helper to wait for a given index to have documents * Adds test helper to retrieve the relevant index name from a call to esArchive.load
We're no longer making a distinction between an upgrade vs. an update vs. a migration vs. a reindex: a migration is the concept that encompasses this work. Both an index and individual documents can require a migration, but both follow the same code path to migrate.
This will be a slightly better API: rather than having to pass all three fields to finalize the migration, API users can instead send the token.
These often contain detailed information that we were previously dropping. This will give better info on the migration finalization endpoint, but should give more information across all detection_engine endpoints in the case of an es client error.
This lead to a few changes in the responses from our different endpoints; mainly, we pass both the migration token AND its constituent parts to aid in debugging.
This would be really hard to reproduce with an integration test since we'd need to generate a specific reindex failure. Much easier to stub some ES calls to exercise that code in a unit test.
We now record a single document-level version field. This represents the version of the document's _source, which is generated by our rule execution. When either a mapping _or_ a transformation is added, this version will be bumped such that new signals will contain the newest version, while the index itself may still contain the old mappings. The transformation pipeline will use the signal version to short-circuit unnecessary transformations.
This can be determined programatically, but for users manually interpreting this response, the qualification will help.
* getIndexVersion always returns a number * version comparisons use isOutdated
We now generate a version field to indicate the version under which the signal was created/migrated.
Rather than having to perform a manual reindex, this should give API users some control over the performance of their automated migration.
These were failing on our new signal field.
Since this is ultimately just an aggregation query there's not much else to test.
rylnd
force-pushed
the
signals_migration
branch
from
1d1d674 to
70031ad
Compare
Conflicts: x-pack/plugins/security_solution/server/lib/detection_engine/routes/index/get_signals_template.ts
* Treat write indices as any other index in migration status endpoint * Migration API rejects requests containing write indices * Migration API rejects requests containing unknown/non-signals indices
rylnd
commented
Dec 7, 2020
| version: number; | ||
| }): Promise<string> => { | ||
| const paddedVersion = `${version}`.padStart(6, '0'); | ||
| const destinationIndexName = `${index}-r${paddedVersion}`; |
There was a problem hiding this comment.
Here's how we're naming our destination indexes, so e.g. migrating .siem-signals-default-000002 to template 14 would reindex into .siem-signals-default-000002-r000014.
Note that this is additive as well, so if on a subequent upgrade the user migrated this index again, it'd be something like .siem-signals-default-000002-r000014-r000021 etc. This allows us to track all the migrations that have been performed on a given index, but it also places a limit on the number of times they can migrate before hitting the index name limit.
There was a problem hiding this comment.
Do we need the 0 padding on the version numbers? Could reduce some noise and allow them to migrate more before hitting the name length limit.
There was a problem hiding this comment.
The padding was added to assist sorting these indexes by name. It's not strictly necessary and not being relied upon anywhere, so we could definitely remove it.
marshallmain
reviewed
Dec 8, 2020
There was a problem hiding this comment.
Looks good overall. Left a couple comments about edge cases.
One extra API we may need is an API to list all in-progress migration tasks. It seems brittle if the only way to get the migration token necessary to finalize the migration is from the create migration API. That puts more burden on API users and the eventual frontend code to store any migration tokens until completion.
| return ( | ||
| signalVersion.doc_count > 0 && isOutdated({ current: signalVersion.key, target: version }) | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Do we ever expect to find signals with version < the index version during normal operation (i.e. users haven't gone in and messed with the index versions)?
There was a problem hiding this comment.
I believe that this could happen as part of a multi-kibana setup, where one instance has been upgraded but another is still generating signals on an old version (and both share the same signals index).
In those situations, I believe the migration recommendation will be to perform the migration on one instance after disabling jobs in all other instances, and then iteratively upgrading/re-enabling jobs on each subsequent instance.
There was a problem hiding this comment.
Regardless of how outdated signals make it into one of these indexes, they have the potential to break certain features, and the check is straightforward, so I decided to notify the user of that situation.
x-pack/plugins/security_solution/server/lib/detection_engine/migrations/migrate_signals.ts
Outdated
Show resolved
Hide resolved
...curity_solution/server/lib/detection_engine/routes/signals/create_signals_migration_route.ts
Outdated
Show resolved
Hide resolved
...rity_solution/server/lib/detection_engine/routes/signals/finalize_signals_migration_route.ts
Show resolved
Hide resolved
Without this phase, ILM gets confused as it tries to move to the delete phase and fails.
The referenced field has changed.
If we have a recoverable error: e.g. the destination index already exists, or a specified index is a write index, we now report those errors as part of the normal 200 response as these do not preclude other specified indices from being migrated. However, if non-signals indices are specified, we do continue to reject the entire request, as that's indicative of misuse of the endpoint.
💚 Build SucceededMetrics [docs]Distributable file count
Page load bundle
History
To update your PR or re-run it, just comment with: |
marshallmain
approved these changes
Dec 10, 2020
There was a problem hiding this comment.
LGTM
We may need to discuss with the elasticsearch team on how best to manage tasks. It appears that completed tasks aren't returned in the _tasks API unless you provide the specific task ID, so that adds some more complexity to cleaning up orphaned tasks - it may not be worth the effort. An additional API that lists out in-progress signals reindexing tasks would be convenient in the future, but not essential right away.
Yep, persisting migrations as Saved Objects fixes this. I'll reference that one here when it's up 👍 |
rylnd
added a commit
that referenced
this pull request
Dec 10, 2020
* WIP: basic reindexing works, lots of edge cases and TODOs to tackle * Add note * Add version metadata to signals documents * WIP: Starting over from the ground up * Removes obsolete endpoints/functions * Adds endpoint for checking the migration status of signals indices * Adds helper functions to represent the logical pieces of answering that question * Fleshing out upgrade of signals * triggers reindex for each index * starts implementing followup endpoint to "finalize" after reindexing is finished * Fleshing out more of the upgrade path Still moving logic around a bunch. * Pad the version number of our destination migration index Instead of e.g. `.siem-signals-default-000001-r5`, this will generate `.siem-signals-default-000001-r000005`. This shouldn't matter much, but it may make it easier for users at a glance to see the story of each index. * Fleshing out more upgrade finalization * Verifies that task matches the specified parameters * Verifies that document counts are the same * updates aliases * finalization endpoint requires both source/dest indexes since we can't determine that from the task itself. * Ensure that new signals are generated with an appropriate schema_version * Apply migration cleanup policy to obsolete signals indexes After upgrading a particular signals index, we're left with both the old and new copies of the index. While the former is unlinked, it's still taking up disk space; this ensures that it will eventually be deleted, but gives users enough time to recover data if necessary. This also ensures that, as with the normal signals ILM policy, it is present during our normal sanity checks. * Move more logic into component functions * Fix type errors * Refactor to make things a little more organized * Moves migration-related routes under signals/ to match their routing * Generalizes migration-agnostic helpers, moves them to appropriate folders (namely index/) * Inlined getMigrationStatusInRange, a hyper-specific function with limited utility elsewhere * Add some JSDoc comments around our new functions This is as much to get my thoughts in order as it is for posterity. Next: tests! * Adds integration tests around migration status route * Adds io-ts schema for route params * Adds es_archiver data to represent an outdated signals index * Adds API integration tests for our signals upgrade endpoint * Adds io-ts schema for route params * Adds second signals index archive, updates docs * Adds test helper to wait for a given index to have documents * Adds test helper to retrieve the relevant index name from a call to esArchive.load * WIP: Fleshing out finalization tests * Consolidate terminalogy around a migration We're no longer making a distinction between an upgrade vs. an update vs. a migration vs. a reindex: a migration is the concept that encompasses this work. Both an index and individual documents can require a migration, but both follow the same code path to migrate. * Implement encoding of migration details This will be a slightly better API: rather than having to pass all three fields to finalize the migration, API users can instead send the token. * Better transformation of errors thrown from the elasticsearch client These often contain detailed information that we were previously dropping. This will give better info on the migration finalization endpoint, but should give more information across all detection_engine endpoints in the case of an es client error. * Finishing integration tests around finalization endpoint This lead to a few changes in the responses from our different endpoints; mainly, we pass both the migration token AND its constituent parts to aid in debugging. * Test an error case due to a reindexing failure This would be really hard to reproduce with an integration test since we'd need to generate a specific reindex failure. Much easier to stub some ES calls to exercise that code in a unit test. * Remove unnecessary version info from signals documents We now record a single document-level version field. This represents the version of the document's _source, which is generated by our rule execution. When either a mapping _or_ a transformation is added, this version will be bumped such that new signals will contain the newest version, while the index itself may still contain the old mappings. The transformation pipeline will use the signal version to short-circuit unnecessary transformations. * Migrate an index relative to the ACTUAL template version This handles the case where a user is attempting to migrate, but has not yet rolled over to the newest template. Running rules may insert "new" signals into an "old" index, but from the perspective of the app no migration is necessary in that case. If/when they roll over, the aforementioned index (and possibly older ones) will be qualified as outdated, and can be migrated. * Enrich our migration_status endpoint with an is_outdated qualification This can be determined programatically, but for users manually interpreting this response, the qualification will help. * Update migration scripts * More uniform version checking * getIndexVersion always returns a number * version comparisons use isOutdated * Fix signal generation unit tests We now generate a version field to indicate the version under which the signal was created/migrated. * Support reindex options to be sent to create_migration endpoint Rather than having to perform a manual reindex, this should give API users some control over the performance of their automated migration. * Fix signal generation integration tests These were failing on our new signal field. * Add unit tests for getMigrationStatus * Add a basic test for getSignalsIndicesInRange Since this is ultimately just an aggregation query there's not much else to test. * Add unit test for the naming of our destination migration index * Handle write indices in our migration logic * Treat write indices as any other index in migration status endpoint * Migration API rejects requests containing write indices * Migration API rejects requests containing unknown/non-signals indices * Add original hot phase to migration cleanup policy Without this phase, ILM gets confused as it tries to move to the delete phase and fails. * Update old comment The referenced field has changed. * Delete task document as part of finalization * Accurately report recoverable errors on create_signals_migration route If we have a recoverable error: e.g. the destination index already exists, or a specified index is a write index, we now report those errors as part of the normal 200 response as these do not preclude other specified indices from being migrated. However, if non-signals indices are specified, we do continue to reject the entire request, as that's indicative of misuse of the endpoint.
8 tasks
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 implements the initial API to drive migration of signals indices.
It's composed of three main endpoints:
There are also dev scripts corresponding to each of these endpoints.
Additionally, this PR adds a
signal._meta.versionfield to signals documents, which represents the version of the detection engine that generated the signal. New signals are written with the value fromSIGNALS_TEMPLATE_VERSION, and old signals have this field populated when they are migrated.For review purposes, I would suggest starting with the API integration tests as they provide the best overview of this work.
Outstanding questions/notes:
signalsin e.g. the API path. Do we continue referring to signals in the API, or should we take steps to migrate toalertshere?task.description: "to [${signalsAlias}-*-r*]", but even that is no guarantee.TODO
Checklist
Delete any items that are not applicable to this PR.
For maintainers