-
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
[Ingest Manager] Remove fields from index pattern during package uninstall #80082
[Ingest Manager] Remove fields from index pattern during package uninstall #80082
Conversation
Pinging @elastic/ingest-management (Team:Ingest Management) |
Pinging @elastic/ingest-management (Feature:EPM) |
@ph should we backport this so it's fixed in 7.10? |
@@ -200,6 +203,7 @@ export default function (providerContext: FtrProviderContext) { | |||
describe('uninstalls all assets when uninstalling a package', async () => { | |||
skipIfNoDockerRegistry(providerContext); | |||
before(async () => { | |||
await installPackage(pkgKey); |
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.
This looks like it adds then immediately removes the package. Is that what's happening?
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.
yeah this is just ensuring that the package was installed to begin with so the fields get added and then immediately removes it to setup the tests to validate the uninstall scenario. I got in a weird state locally when running the tests where the package was never actually installed the first time.
@@ -324,6 +328,35 @@ export default function (providerContext: FtrProviderContext) { | |||
} | |||
expect(resSearch.response.data.statusCode).equal(404); | |||
}); | |||
it('should have removed the fields from the index patterns', async () => { |
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'm confused about which branch we expect to execute. The try or the catch?
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.
Both could happen. If a test case in another file calls /setup
then the system
and endpoint
packages will be installed and will be present for the remainder of the tests (because they cannot be removed). If that is the case the expect
in the try
will work because the logs-*
and metrics-*
index patterns will still be present even after this test uninstalls its package.
If /setup
was never called prior to this test, when the package is uninstalled the index pattern code checks to see if there are no packages installed and completely removes the logs-*
and metrics-*
index patterns. If that happens this code will throw an error and indicate that the index pattern being searched for was completely removed.
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.
Oh, interesting. Maybe we could add that as a comment?
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.
Yeah sure I'll put it as a comment.
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.
Good catch, 👍
💚 Build SucceededMetrics [docs]
History
To update your PR or re-run it, just comment with: |
…stall (elastic#80082) * Correctly removing index pattern fields * Adding comment about try/catch
…stall (elastic#80082) * Correctly removing index pattern fields * Adding comment about try/catch
…a-tier-migration * 'master' of github.com:elastic/kibana: (34 commits) Line Visualization improper scaling can result in gaps elastic#79663 (elastic#80135) [Security Solution][Timeline] Fix SelectableTimeline search (elastic#80128) move field_mapping util to saved_objects plugin (elastic#79918) [ML] Datagrid: Ensure column content with 'boolean' schema is not capitalized (elastic#80041) [CI] Correctly resolve repository root for JUnit reports (elastic#80226) [Ingest Manager] Fix package upgrade breaking after first rollover before new data has arrived (elastic#79887) [Security Solution] Fix positioning of Kibana banners list inside Sec… (elastic#80124) add missing await to fix test (elastic#80202) Revert test data changed in previous commit. (elastic#79479) [Security Solution] [Sourcerer] Jest beef up (elastic#79907) Re-enable transaction duration alert story (elastic#80187) [npm] remove canvas dep (elastic#80185) [DOCS] Redirects ILM docs to Elasticsearch reference (elastic#79798) [APM] Catch health status error from ML (elastic#80131) Fix layout and remove title for add alert popover. (elastic#77633) [Discover] Loading spinner cleanup (elastic#79819) [Security Solution] [Resolver] Remove related events api (elastic#79036) [Ingest Manager] Remove fields from index pattern during package uninstall (elastic#80082) do not refetch license if signature header absents from a response (elastic#79645) Only send agent data for non-opentelemetry agents (elastic#79587) ... # Conflicts: # x-pack/plugins/index_lifecycle_management/server/routes/api/nodes/register_list_route.ts
* upstream/master: (34 commits) Improve vis editor typings (elastic#80004) Line Visualization improper scaling can result in gaps elastic#79663 (elastic#80135) [Security Solution][Timeline] Fix SelectableTimeline search (elastic#80128) move field_mapping util to saved_objects plugin (elastic#79918) [ML] Datagrid: Ensure column content with 'boolean' schema is not capitalized (elastic#80041) [CI] Correctly resolve repository root for JUnit reports (elastic#80226) [Ingest Manager] Fix package upgrade breaking after first rollover before new data has arrived (elastic#79887) [Security Solution] Fix positioning of Kibana banners list inside Sec… (elastic#80124) add missing await to fix test (elastic#80202) Revert test data changed in previous commit. (elastic#79479) [Security Solution] [Sourcerer] Jest beef up (elastic#79907) Re-enable transaction duration alert story (elastic#80187) [npm] remove canvas dep (elastic#80185) [DOCS] Redirects ILM docs to Elasticsearch reference (elastic#79798) [APM] Catch health status error from ML (elastic#80131) Fix layout and remove title for add alert popover. (elastic#77633) [Discover] Loading spinner cleanup (elastic#79819) [Security Solution] [Resolver] Remove related events api (elastic#79036) [Ingest Manager] Remove fields from index pattern during package uninstall (elastic#80082) do not refetch license if signature header absents from a response (elastic#79645) ...
Summary
This PR addresses this issue: #80031
The
installIndexPatterns()
function relies on the saved objects being absent for removing the fields from a package during uninstall. Since the saved object is still present when the function is called it will collection the same fields as before instead of removing them fields for the package being uninstalled.Testing
/setup
0.1.1
packageNavigate back to the index pattern management page and observe that the crowdstrike fields exist
Uninstall the crowdstrike package