-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Fleet] Handle Saved Object ID changes (#108959) #119527
[Fleet] Handle Saved Object ID changes (#108959) #119527
Conversation
37e3285
to
b742df6
Compare
@elasticmachine merge upstream |
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.
ML changes LGTM
Pinging @elastic/fleet (Team:Fleet) |
218f6da
to
edf56dd
Compare
I wonder if instead we should add an option to import to bypass this setting for our internal usage like this. I worry about any knock-on effects of making these types importable/exportable when they weren't before. I could be wrong, but I think objects that have I don't have any reason to believe that making these types exportable or importable is a bad change, but I think it should be done with full consideration rather than as a side-effect of this work. And maybe you've already done that :) Did we discuss with the ML and Security teams the implications of this change? EDIT: if we do go down the route of adding an option to import to allow us to bypass the types allowed, we should probably make that a separate PR to Core first. |
x-pack/plugins/fleet/server/services/epm/packages/_install_package.ts
Outdated
Show resolved
Hide resolved
HI @joshdover thanks for the review, I did raise this (via @jportner ) in |
@joshdover I've done some rough performance testing locally, generally the actual installing of the assets is about the same. However because we no longer install the index patterns and generate the fields as a separate stage, we are seeing some really good performance improvements on the total install time. Heres my results: https://gist.github.com/hop-dev/a76c4c6c718e0b014835ee09ba7606df |
8a5a664
to
ce573d2
Compare
@joshdover one other point, it seems visibleInManagement does exclude a saved object from the "export all" option in the UI which is good |
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.
Looks pretty good, but I did find one issue in manual testing:
The assets tab breaks (empty/broken links) after installing a package in a space and then reinstalling the integration in the default space. Steps to reproduce:
- Create a
test
space and switch to it - Install Apache integration
- Switch to
default
space - Uninstall Apache integration
- Install Apache integration
- Go to assets tab for Apache integration (still in default space), see broken links
I ran the following test as well to verify the full behavior and everything else worked:
- Start Kibana and ES 7.16.0
- Create
test
space - Install nginx 1.1.0 via API in
test
space - Stop Kibana and ES
- Start ES 8.1.0 using same
path.data
as 7.16.0 - Start this branch running from source
- Upgrade the nginx package to 1.2.0 in the
test
space
x-pack/plugins/fleet/server/services/epm/kibana/assets/install.ts
Outdated
Show resolved
Hide resolved
Not sure if this PR has had any interaction here (been a bit since I've tested this workflow), but Is this intended? Only way to see the number of assets installed seems to be clicking through to |
8382974
to
6db67bd
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.
Checked out, tested locally, and everything appears to be working as expected with the Prebuilt Security Detection Rules
integration. Was able to verify installation of the integration, and also confirmed that when installing rules from within the Security UI the SO's from the integration were used. Also verified assets weren't showing up in the SOM either, so all good there too!
Security changes LGTM! 👍 Thanks! 🙂
We were checking for an error before the import was complete.
1087909
to
c9f3ddf
Compare
c9f3ddf
to
5945b38
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.
Changes LGTM. As mentioned offline, we're still wanting to confirm the root cause of the bug identified in #119527 (review). We currently suspect it will be fixed by the fixing the uninstall step across Spaces which will be fixed as part of #74353
💚 Build Succeeded
Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: cc @hop-dev |
* Do not add fields to index patterns * remove redundant tests * install index patterns before package install * update remove comment * use import to create package assets Here I have also moved to importing all assets at once. This is essential when importing to ensure that all saved objects references are imported at once. There is also an efficiencey improvement. * Import index patterns * use resolve when deleting index patterns * fix: asset type validation * add option to override supported import types * make ml-module importable * Revert "add option to override supported import types" This reverts commit 1f48e6ee193fea5e5cb0f37c70cbfa7ae47eeab5. * remove hidden: false from ml-module * use resolve when deleting assets * make security-rule SO type importable * use bulkResolve to get package assets * fix tests * fix 'multiple' tests * remove unused function * create index patterns at the same time as other assets * remove unused test * Fix integration tests We were checking for an error before the import was complete. * tidy for PR * add missing test assets * do not attempt to delete missing assets * resolve any reference errors that occur on import * await installKibanaAssets immediately * show assets not found when assets installed in a different space * fix delete asset check on force upgrade * add comment about reference errors * remove a couple of appContextService dependencies
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
* Do not add fields to index patterns * remove redundant tests * install index patterns before package install * update remove comment * use import to create package assets Here I have also moved to importing all assets at once. This is essential when importing to ensure that all saved objects references are imported at once. There is also an efficiencey improvement. * Import index patterns * use resolve when deleting index patterns * fix: asset type validation * add option to override supported import types * make ml-module importable * Revert "add option to override supported import types" This reverts commit 1f48e6ee193fea5e5cb0f37c70cbfa7ae47eeab5. * remove hidden: false from ml-module * use resolve when deleting assets * make security-rule SO type importable * use bulkResolve to get package assets * fix tests * fix 'multiple' tests * remove unused function * create index patterns at the same time as other assets * remove unused test * Fix integration tests We were checking for an error before the import was complete. * tidy for PR * add missing test assets * do not attempt to delete missing assets * resolve any reference errors that occur on import * await installKibanaAssets immediately * show assets not found when assets installed in a different space * fix delete asset check on force upgrade * add comment about reference errors * remove a couple of appContextService dependencies Co-authored-by: Mark Hopkin <mark.hopkin@elastic.co>
* Do not add fields to index patterns * remove redundant tests * install index patterns before package install * update remove comment * use import to create package assets Here I have also moved to importing all assets at once. This is essential when importing to ensure that all saved objects references are imported at once. There is also an efficiencey improvement. * Import index patterns * use resolve when deleting index patterns * fix: asset type validation * add option to override supported import types * make ml-module importable * Revert "add option to override supported import types" This reverts commit 1f48e6ee193fea5e5cb0f37c70cbfa7ae47eeab5. * remove hidden: false from ml-module * use resolve when deleting assets * make security-rule SO type importable * use bulkResolve to get package assets * fix tests * fix 'multiple' tests * remove unused function * create index patterns at the same time as other assets * remove unused test * Fix integration tests We were checking for an error before the import was complete. * tidy for PR * add missing test assets * do not attempt to delete missing assets * resolve any reference errors that occur on import * await installKibanaAssets immediately * show assets not found when assets installed in a different space * fix delete asset check on force upgrade * add comment about reference errors * remove a couple of appContextService dependencies
Summary
Closes #108959.
In 8.0.0 the _id structure of space specific saved objects has changed, this means we now have to use
resolve
to look up saved objects before getting or deleting them, and we have to useimport
to create them (instead ofbulkCreate
).In Fleet's case this is only relevant to the asset saved objects as they are space-aware.
Using
import
presented a couple of challenges :import
validates SO references exist before allowing an object to be imported, this has meant a couple of changes:fields
property on anindex-pattern
saved object based on the installed packages data streams, I have removed this code so that we no longer generate thefields
(see implementation details below)import
will only import SO's withimportableAndExportable
set to true, I have therefore had to add this property to theml-module
andsecurity-rule
SO types. AddingvisibleInManagement: false
means that they do not show in theImplementation Details
Outstanding issues before merge:
metricbeat-*
index pattern on one of its dashboardsRemoving
fields
from index patternsPreviously, after installing a package, we extracted all fields from all data streams in the package amd added them to the
index-pattern
saved object. This was so that on an empty system, dashboards would not show errors for missing fields in the index pattern.However, as of 7.12, index patterns generate their fields dynamically at runtime, ignoring the
fields
key on the saved object, meaning these dashboard errors are visible. This has allowed me to remove all field generation code and move the index pattern creation to before the package is installed.Testing
Mostly manual for now, I have created an issue to automate the manual tests after FF #119621
Checklist
Delete any items that are not applicable to this PR.