-
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
Introduce preboot
lifecycle stage
#103636
Conversation
c1ac601
to
a608397
Compare
7962391
to
af0a654
Compare
af0a654
to
6751a1c
Compare
Pinging @elastic/kibana-core (Team:Core) |
Okay, I think I've replied to all you comments and handled everything that we already agreed on @pgayvallet @mshustov. There are three major potential improvements left that I've put proposals for (for this PR or follow-ups if it's OK for you), let me know what you think.
Thanks! |
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 haven't given this a thorough review, but I will rely on Pierre and Mikhail here as I already reviewed the RFC. I do have one comment regarding the testing of PluginsService
but otherwise, LGTM
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.
Security changes LGTM, core changes are well-aligned with RFC, and all works as expected when testing locally -- nice work!
credentialsSpecified: | ||
config.username !== undefined || | ||
config.password !== undefined || | ||
config.serviceAccountToken !== undefined, |
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.
question should we check for a client certificate here? If Kibana is configured for PKI-based auth, then checking for username
, password
, and serviceAccountToken
would miss this
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 was thinking we could just do something like this to cover PKI case since for PKI we'd need https
or I'm missing something?
const noPKI = core.elasticsearch.config.hosts.length === 1
&& core.elasticsearch.config.hosts[0] === 'http://localhost:9200';
http://localhost:9200
is a default, and if we have anything in hosts
except for this default I believe we'll have to skip user setup (including PKI case).
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 something like that could work. I was thinking about this in isolation, not about the bigger picture of "interactive setup mode". Someone looking at this config option might not understand that PKI-based credentials aren't considered as part of credentialsSpecified
. I'm happy to ignore this for now, as we don't have an explicit need for this.
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.
Fleet changes 🚀
💚 Build SucceededMetrics [docs]Public APIs missing comments
History
To update your PR or re-run it, just comment with: cc @azasypkin |
💔 Backport failed
To backport manually run: |
…y-show-migrate-to-authzd-users * 'master' of github.com:elastic/kibana: (187 commits) Space management page UX improvements (elastic#100448) [Reporting] Unskip flaky test when downloading CSV with "no data" (elastic#105252) Update dependency @elastic/charts to v33 (master) (elastic#105633) [Observability RAC] Improve alerts table columns (elastic#105446) Introduce `preboot` lifecycle stage (elastic#103636) [Security Solution] Invalid kql query timeline refresh bug (elastic#105525) skip flaky suite (elastic#106121) [Security Solution][Endpoint] Fix UI inconsistency between isolation forms and remove display of Pending isolation statuses (elastic#106118) docs: APM RUM Source map API (elastic#105332) [CTI] Adds indicator match rule improvements (elastic#97310) [Security Solution] update text for Isolation action submissions (elastic#105956) EP Meta Telemetry Perf (elastic#104396) [Metrics UI] Drop partial buckets from ALL Metrics UI queries (elastic#104784) Remove beta admonitions for Fleet docs (elastic#106010) [Observability RAC] Remove indexing of rule evaluation documents (elastic#104970) Parameterize migration test for kibana version (elastic#105417) [Alerting] Allow rule to execute if the value is 0 and that mets the condition (elastic#105626) [ML] Fix Index data visualizer sometimes shows wrong doc count for saved searches (elastic#106007) [Security Solution] UX fixes for Policy page and Case Host Isolation comment (elastic#106027) [Security Solution]Memory protection configuration card for policies integration. (elastic#101365) ... # Conflicts: # x-pack/plugins/reporting/public/management/report_listing.test.tsx # x-pack/plugins/reporting/public/management/report_listing.tsx
Summary
Implementation of the "RFC: Kibana
preboot
lifecycle stage.".How to test
Note to reviewers
Real
preboot
plugin will be implemented in the scope of #104068 and #102538.In this PR I tried to write and update as much tests as it was feasible, and I'm planning to introduce a set of API integration and functional tests once we have a real
preboot
plugin (interactive setup mode #104068).RFC: #99318.