Interactive setup mode #106881
Merged
Interactive setup mode #106881
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
thomheymann
added
Team:Security
thomheymann
commented
Jul 27, 2021
| .createClient('data', { | ||
| hosts: request.body.hosts.map((host) => `https://${host}`), | ||
| ssl: { verificationMode: 'none' }, | ||
| // caFingerprint: request.body.caFingerprint, |
There was a problem hiding this comment.
Waiting for Elasticsearch client to be updated and integrated.
azasypkin
reviewed
Jul 28, 2021
azasypkin
reviewed
Jul 28, 2021
azasypkin
reviewed
Jul 28, 2021
azasypkin
reviewed
Jul 28, 2021
azasypkin
reviewed
Jul 28, 2021
azasypkin
reviewed
Jul 28, 2021
azasypkin
reviewed
Jul 28, 2021
azasypkin
reviewed
Jul 28, 2021
azasypkin
reviewed
Jul 28, 2021
azasypkin
reviewed
Jul 28, 2021
azasypkin
reviewed
Jul 28, 2021
azasypkin
reviewed
Jul 28, 2021
azasypkin
reviewed
Jul 28, 2021
azasypkin
reviewed
Jul 28, 2021
azasypkin
reviewed
Aug 25, 2021
azasypkin
reviewed
Aug 25, 2021
src/plugins/interactive_setup/public/cluster_configuration_form.test.tsx
Show resolved
Hide resolved
src/plugins/interactive_setup/public/cluster_configuration_form.tsx
Outdated
Show resolved
Hide resolved
azasypkin
reviewed
Aug 25, 2021
| await client.close(); | ||
| } | ||
|
|
||
| return { |
There was a problem hiding this comment.
question: can we just return caCert without duplicating other arguments? It'd be much easier for reader to understand the return result. I had to read the entire method to make sure we just return arguments and don't modify them along the way.
| username: 'kibana_system', | ||
| password: '', | ||
| }) | ||
| ).not.toThrowError(); |
There was a problem hiding this comment.
nit: let's replace .not.toThrowError() with toEqual to validate the actual validation result (and in the last assertion as well).
nit: can you please add a one more test case for password-only request bodySchema.validate({ host: 'http://localhost:9200', password: 'pass' })?
There was a problem hiding this comment.
nit: let's replace
.not.toThrowError()withtoEqualto validate the actual validation result (and in the last assertion as well).
What would this be adding to out unit tests? We shouldn't test that validate does what it's supposed to do, only that the schema we defined is correct.
nit: can you please add a one more test case for password-only request
bodySchema.validate({ host: 'http://localhost:9200', password: 'pass' })?
Sure
There was a problem hiding this comment.
What would this be adding to out unit tests? We shouldn't test that validate does what it's supposed to do, only that the schema we defined is correct.
It's just a good rule to follow: schema not only validates input, but also may add a new data (e.g. default values) or transform an input one (e.g. Duration or ByteSize). Changing default values in config schemas is the common source of unintentional breaking changes in Kibana historically - one should explicitly and thoughtfully update tests if they change default values.
| @@ -6,22 +6,76 @@ | |||
| * Side Public License, v 1. | |||
| */ | |||
|
|
|||
| import { EuiPageTemplate, EuiPanel, EuiText } from '@elastic/eui'; | |||
| import React from 'react'; | |||
| import './app.scss'; | |||
There was a problem hiding this comment.
note: not for this PR, just so that I don't forget about these two points we should do for alpha2:
- Redirect to original URL after successful configuration
- Automatically redirect (with a warning or something) to original URL if we detect that Kibana can connect to ES while user is in interactive setup UI
| * Side Public License, v 1. | ||
| */ | ||
|
|
||
| import { cloneDeep, cloneDeepWith, get, set } from 'lodash'; |
There was a problem hiding this comment.
Okay, seems like ESlint is broken. Soo, since your PR discovered that, @thomheymann can you please use safer-lodash here and file an issue for the broken eslint rule so that we can handle it separately?
| @@ -0,0 +1,39 @@ | |||
| /* | |||
There was a problem hiding this comment.
nit: that would be nice if we can add a test or two for this component here or in the follow-up.
|
Raised #110422 |
|
ACK: reviewing... |
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / general / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/ml/anomaly_detection/advanced_job·ts.machine learning anomaly detection advanced job with multiple metric detectors and custom datafeed settings job cloning runs the clone job and displays it correctly in the job listStandard OutStack TraceMetrics [docs]Module Count
Public APIs missing comments
Page load bundle
History
To update your PR or re-run it, just comment with: cc @thomheymann |
|
Hey @thomheymann, I noticed this was merged without the final review from design. Just wanted to mentioned I was unable to get the enrolment token working locally with the instructions I received so I never go to check the |
Sorry you haven't been able to test this. What issues did you have? All suggestions from design have been addressed including changing the connect anyways button to primary colour. |
|
@thomheymann thanks for the follow up and changes. In the future, let's not merge without a design approval when one has been requested. If you feel an approval from us is holding you up, then please feel free to mention kibana-design team here or on the kibana-design Slack channel. |
kibanamachine
added
the
backport missing
|
Friendly reminder: Looks like this PR hasn’t been backported yet. |
kibanamachine
removed
the
backport missing
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.
Resolves #102538
Summary
This PR introduces interactive setup mode allowing users to connect Kibana to a secure cluster using a guided getting started experience.
Enrollment flow
Manual configuration: Step 1
Manual configuration: Step 2
Case 1: Authentication and TLS enabled
Case 2: Authentication disabled
Case 2: TLS disabled
Testing
Interactive setup mode is automatically started when no Elasticsearch connection has been configured. (host or valid credentials). However, the plugin is still disabled by default so needs to be enabled manually. Ensure watch mode is disabled so that the process isn't restarted when we write to the config file:
When setup is complete the following config will be written to disc.
This PR requires elastic/elasticsearch#74890 with the following Elasticsearch config:
Certificates can be grabbed from https://github.com/elastic/elasticsearch/tree/f501712c3288960f7057a9fe5644eaae13bb133a/x-pack/plugin/security/src/test/resources/org/elasticsearch/xpack/security/action/enrollment and converted to PEM
openssl pkcs12 -in /path/to/httpCa.p12 -cacerts -nokeys.Checklist
Delete any items that are not applicable to this PR.
Risk Matrix
Delete this section if it is not applicable to this PR.
Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.
When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:
For maintainers