-
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
[Fleet] Migrate ES client #92805
[Fleet] Migrate ES client #92805
Conversation
body: any; | ||
} = { | ||
method: 'PUT', | ||
path: `/${templateAPIPath}/${templateName}`, | ||
ignore: [404], | ||
body: content, | ||
}; | ||
// This uses the catch-all endpoint 'transport.request' because there is no | ||
// convenience endpoint using the new _index_template API yet. | ||
// The existing convenience endpoint `indices.putTemplate` only sends to _template, | ||
// which does not support v2 templates. | ||
// See src/core/server/elasticsearch/api_types.ts for available endpoints. |
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.
The new client should have a specific function to install v2 index templates:
The fallback to transport.request
shouldn't be necessary any more.
I pushed those changes. I also went through all |
@EricDavisX We have changed the way to access Kibana using the new ES client. I think any issue should be cached by the normal regression test. I don't think we need to add an explicitly test-plan label for this WDYT? |
@elasticmachine merge upstream |
@ph This PR basically changes, well, everything! If the regression test covers already all possible edge cases we should be good to go |
@elasticmachine merge upstream |
@afgomez @ph hi - I respect it changes 'pretty much everything' but I'd like to dig in to *some amount of literals here, please. :) I think with some of the broad changes we have brewing we could make use of the 'deploy my pr' and stand up an easily available server, and let the manual test team do a pre-merge check. doing it pre-merge is extremely valuable vs doing it post merge ad it clearly identifies who the author of the change is that adds a break (with the hope that the team is well aware of known issues already merged in, which we are not always, though we try). doing it pre-merge also makes the regression time go more smooth where we always are short of time, so this keeps the cycle more manageable. thanks for reaching out to us. I suggest when we are ready to push this and test it, that I can offer to run the jenkins job to deploy it and post access creds to the test group in slack. we can't run a full regression pre merge, but we can do a general smoke test, and target tests that relate... so that means... anywhere where Fleet 'talks accesses ES' I guess? What does that mean for us, if we can identify anything more specific that 'everything' we can focus there - but perhaps just the general smoke test is all we want to do? @dikshachauhan-qasource is our resource who can help test. I will start a deploy now, if the PR is in good shape enough for us to check it out? |
@EricDavisX I also think it makes sense to test before merging the PR. I'm struggling to get the build green but the code should be good to go. Let me know if I can be of any help 🙇 |
If I'm not mistaken, all places where we use the ES client are used during package installation. If we cover the installation of all types of elasticsearch assets during our tests (e2e and api integration tests) this change should be adequately tested. Edited to add: during package installation and for api keys. |
This is very helpful @skh thanks much. We do not have adequate e2e-testing scenarios for this, so we'll plan to test |
While working on #91715, I realized that the new index pattern service requires the new ES client, so getting this in would unblock me on that. @skh Would you be able to pull this down locally and test as part of your review? I think the changes here are low-level enough that if there are any issues, most would be caught during normal development by engineers after this gets merged into main branch: if things break from this change, they would break in big and obvious ways! It would still be great for QA to run a separate test after merge as @EricDavisX suggests, but I don't want to hold up the merge of this PR on requiring that. Thank you @afgomez for addressing this tech debt we've had for a while :) |
@afgomez @jen-huang - one of the goals with pre-check-in tests is to know who to pick and hunt for when a bug comes in unexpectedly. with this info, we know that it may be related to this change to the new client, so it is not a problem for me to proceed as long as it is reviewed and has been tested out. Also, as you note it is likely a more obvious break if at all, then that makes it easier. Cheers |
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.
How I tested:
- run locally from git checkout, open Kibana, navigate to Fleet
- observe in dev tools that the calls to the setup API endpoints return successfully
- navigate to "Agents" and click on "Create user and enable central management"
- enroll an agent, verify that the agent enrolls and appears as healthy in the list
Installing the endpoint
and system
packages during the Fleet setup process verifies that installation of the following ES assets works:
- Ingest Pipeline
- Index Template
- Transform
Enrolling an agent verifies that the generation of API keys works.
Alright! I will merge this once the build gets green. Please ping me if you find any issues 🙇 |
💚 Build SucceededMetrics [docs]
History
To update your PR or re-run it, just comment with: |
Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>
Summary
Implements #74111
Migrate to the new Elasticsearch client.
Checklist
Delete any items that are not applicable to this PR.
For maintainers