[Console] Move out of legacy + migrate server side to New Platform #55690
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
…tional dependency
…ml config for console
…I for extending autocomplete Add TODO regarding exposing legacy ES config
jloleysens
added
release_note:roadmap
Team:Kibana Management
|
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
joshdover
reviewed
Jan 27, 2020
src/plugins/console/public/plugin.ts
Outdated
| const elasticsearchUrl = injectedMetadata.getInjectedVar( | ||
| 'elasticsearchUrl', | ||
| 'http://localhost:9200' | ||
| ) as string; | ||
| render( |
There was a problem hiding this comment.
You may want to take a look at the conventions for applications. The main difference between the convention and what you have here is that the ./application module should export a single renderApp function that encapsulates the entire rendering logic (invoking React).
This should be a minor change, but makes this a bit more consistent across the code base.
Comment on lines
+60
to
+63
| const elasticsearchUrl = injectedMetadata.getInjectedVar( | ||
| 'elasticsearchUrl', | ||
| 'http://localhost:9200' | ||
| ) as string; |
There was a problem hiding this comment.
Guess I'm a bit surprised we expose this to the client. Is this necessary? Seems like a potential low-severity security issue? If it can be removed, can be addressed separately from this PR.
There was a problem hiding this comment.
Yeah, it is a bit awkward :c. The reason it exists is for the copy as cURL functionality in console. So that we can provide a better experience for going from Console to the terminal.
I agree that this qualifies as low-severity security concern. Do you think it merits being removed for that reason? Misread your message. To be clear, my understanding of the security issue is that we are leaking DNS entry + port to client side and can't assume we are in a secured environment (like a VPC)?
| proxyFilter: schema.arrayOf(schema.string(), { defaultValue: ['.*'] }), | ||
| ssl: schema.object({ verify: schema.boolean({ defaultValue: false }) }, {}), | ||
|
|
||
| // This does not actually work, track this issue: https://github.com/elastic/kibana/issues/55576 |
There was a problem hiding this comment.
I'm guessing this issue blocks this PR?
There was a problem hiding this comment.
I don't think it needs to block this PR, but it would be nice to do the testing here if we can :). Otherwise, I can do it in post-merging too.
There was a problem hiding this comment.
Has been solved already! 😄 Thanks @pgayvallet!
| ) { | ||
| addExtensionSpecFilePath(join(__dirname, 'spec/')); | ||
| processors.forEach(processor => addProcessorDefinition(processor)); | ||
| this.log.info('Installed console autocomplete extensions.'); |
There was a problem hiding this comment.
nit: I think this should probably be debug instead of info
| @@ -112,8 +113,14 @@ export class HapiResponseAdapter { | |||
| return response; | |||
| } | |||
|
|
|||
| private toError(kibanaResponse: KibanaResponse<ResponseError>) { | |||
| private toError(kibanaResponse: KibanaResponse<ResponseError | Buffer | stream.Readable>) { | |||
There was a problem hiding this comment.
I think it may be more appropriate to add Buffer | stream.Readable to the ResponseError union type. @restrry WDYT?
There was a problem hiding this comment.
When it's required? As I can see proxyRequest rejects a promise with an error message, not steam or a buffer
There was a problem hiding this comment.
Hey @restrry ! Thanks for weighing in!
This is the flow:
User issues request in browser -> console streams to ES -> ES responds (e.g., status code 400!) -> console streams back to browser -> user sees error.
With the current implementation console cannot stream the error response ES gave us back to browser because the current implementation wants to send back JS object encoded as JSON when we haven't read the body into memory for that.
The error handler you linked to is for lower-level errors on http request - like if the socket gets hung up. Not for a 4xx or 5xx status code. You can see here how the response is handled:
There was a problem hiding this comment.
Ok, I think we can merge the current changes. Could you add headers configuration as done in https://github.com/elastic/kibana/pull/55690/files/46c5cab221d6b63a8b442f3ba8a3a7bdc4c03f4d#diff-92d94ad4d163809567c4258f95ff37e2R96 ?
I created an issue to add tests for this API #56305
There was a problem hiding this comment.
I think it may be more appropriate to add Buffer | stream.Readable to the ResponseError union type. @restrry WDYT?
agree, can be done in #56305
…ve-out-legacy * 'master' of github.com:elastic/kibana: (187 commits) [ML] Reseting categorization validation if category field is cleared (elastic#56029) [SIEM] Fields browser readable (elastic#56000) [docs] Remove unused callout (elastic#56032) Refactor saved object management registry usage (elastic#54155) [SIEM][Detection Engine] critical blocker, updates the pre-packaged rules, removes dead ones, adds license file (elastic#56090) Fix failing snapshot artifact tests when using env var (elastic#56063) Fix Github PR comment formatting (elastic#56078) [Maps] fix join metric field selection bugs (elastic#56044) Create a new menu for observability links (elastic#54847) [SIEM] [Detection Engine] Fixes histogram intervals (elastic#55969) make test less flaky by retrying if list is re-rendered (elastic#55949) Remove matrix build support (elastic#54202) Add animation to service map layout (elastic#56042) [Canvas] Remove Angular and unnecessary reporting config from Canvas (elastic#54050) [Uptime] Simplify snapshot max to Infinity (elastic#55931) [Uptime] Reintroduce a column for url (elastic#55451) Cleanup action task params objects after successful execution (elastic#55227) [CI] Retry flaky tests (elastic#53961) Expose NP FieldFormats service to server side (elastic#55419) [Endpoint] EMT-65: make endpoint data types common, restructure (elastic#54772) ... # Conflicts: # src/legacy/core_plugins/console/public/np_ready/application/components/split_panel/__snapshots__/split_panel.test.tsx.snap # src/legacy/core_plugins/console/public/np_ready/application/components/split_panel/containers/panel.tsx # src/legacy/core_plugins/console/public/np_ready/application/components/split_panel/context.tsx # src/legacy/core_plugins/console/public/np_ready/application/components/split_panel/index.ts # src/legacy/core_plugins/console/public/np_ready/application/components/split_panel/split_panel.test.tsx # src/legacy/ui/public/vis/editors/default/default_editor.tsx # src/plugins/console/public/application/components/split_panel/__snapshots__/split_panel.test.tsx.snap # src/plugins/console/public/application/components/split_panel/components/resizer.tsx # src/plugins/console/public/application/components/split_panel/containers/panel.tsx # src/plugins/console/public/application/components/split_panel/containers/panel_container.tsx # src/plugins/console/public/application/components/split_panel/context.tsx # src/plugins/console/public/application/components/split_panel/index.ts # src/plugins/console/public/application/components/split_panel/registry.ts # src/plugins/console/public/application/components/split_panel/split_panel.test.tsx # src/plugins/kibana_react/public/split_panel/__snapshots__/split_panel.test.tsx.snap # src/plugins/kibana_react/public/split_panel/containers/panel.tsx # src/plugins/kibana_react/public/split_panel/context.tsx # src/plugins/kibana_react/public/split_panel/index.ts # src/plugins/kibana_react/public/split_panel/split_panel.test.tsx
|
@joshdover There is one other thing that is worth looking at for the Console migration and that is the proxy route ( Not sure what a better work around for this could be until we expose this information via |
…or now. Refactor file name of request.ts -> proxy_request.ts. This is consistent with the exported function now Started fixing server side tests for the proxy route - Migrated away from sinon - Completed the body.js -> body.test.ts. Still have to do the rest
…mocking logic to a common space
mshustov
reviewed
Jan 28, 2020
| @@ -37,7 +37,7 @@ interface Args { | |||
| // We use a modified version of Hapi's Wreck because Hapi, Axios, and Superagent don't support GET requests | |||
| // with bodies, but ES APIs do. Similarly with DELETE requests with bodies. Another library, `request` | |||
| // diverged too much from current behaviour. | |||
| export const sendRequest = ({ | |||
| export const proxyRequest = ({ | |||
There was a problem hiding this comment.
@joshdover I'm wondering if the platform should provide proxying functionality out of the box. The current implementation stops working when we merge #55670 Should I re-open #41960 ?
There was a problem hiding this comment.
Makes sense to me, but I think we should let this PR move forward and refactor it once we have a solution for #41960.
IMO, we could even push this work off until after NP migrations are complete, but before 8.0.
Add test for custom route validation
…ana/blob/master/src/core/CONVENTIONS.md#applications Change log from info level to debug level for console_extensions plugin
ryankeairns
approved these changes
Jan 28, 2020
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
jloleysens
added a commit
to jloleysens/kibana
that referenced
this pull request
Jan 30, 2020
…lastic#55690) * Initial move of public and setup of server skeleton * Fix public paths and types * Use new usage stats dependency directly in tracker also mark as an optional dependency * WiP on getting server side working * Restore proxy route behaviour for base case, still need to test custom proxy and SSL * Add new type and lib files * Clean up legacy start up code and add comment about issue in kibana.yml config for console * Move console_extensions to new platform and introduce ConsoleSetup API for extending autocomplete Add TODO regarding exposing legacy ES config * Re-introduce injected elasticsearch variable and use it in public * Don't pass stateSetter prop through to checkbox * Refactor of proxy route (split into separate files). Easier testing for now. Refactor file name of request.ts -> proxy_request.ts. This is consistent with the exported function now Started fixing server side tests for the proxy route - Migrated away from sinon - Completed the body.js -> body.test.ts. Still have to do the rest * headers.js test -> headers.test.ts and moved some of the proxy route mocking logic to a common space * Finish migration of rest of proxy route test away from hapi Add test for custom route validation * Bring console application in line with https://github.com/elastic/kibana/blob/master/src/core/CONVENTIONS.md#applications Change log from info level to debug level for console_extensions plugin * Update i18nrc file for console * Add setHeaders when passing back error response Co-authored-by: Elastic Machine <[email protected]> # Conflicts: # .github/CODEOWNERS # src/legacy/core_plugins/console/server/__tests__/proxy_route/params.js # src/legacy/core_plugins/console/server/__tests__/proxy_route/query_string.js # src/plugins/console/server/lib/spec_definitions/spec/generated/get_script_context.json # x-pack/plugins/console_extensions/server/spec/overrides/security.delete_privileges.json # x-pack/plugins/console_extensions/server/spec/overrides/security.put_privileges.json
jloleysens
added a commit
to jloleysens/kibana
that referenced
this pull request
Jan 30, 2020
…lways-use-first-text-object * 'master' of github.com:elastic/kibana: [Console] Move out of legacy + migrate server side to New Platform (elastic#55690)
jloleysens
added a commit
that referenced
this pull request
Jan 30, 2020
…55690) (#56360) * Initial move of public and setup of server skeleton * Fix public paths and types * Use new usage stats dependency directly in tracker also mark as an optional dependency * WiP on getting server side working * Restore proxy route behaviour for base case, still need to test custom proxy and SSL * Add new type and lib files * Clean up legacy start up code and add comment about issue in kibana.yml config for console * Move console_extensions to new platform and introduce ConsoleSetup API for extending autocomplete Add TODO regarding exposing legacy ES config * Re-introduce injected elasticsearch variable and use it in public * Don't pass stateSetter prop through to checkbox * Refactor of proxy route (split into separate files). Easier testing for now. Refactor file name of request.ts -> proxy_request.ts. This is consistent with the exported function now Started fixing server side tests for the proxy route - Migrated away from sinon - Completed the body.js -> body.test.ts. Still have to do the rest * headers.js test -> headers.test.ts and moved some of the proxy route mocking logic to a common space * Finish migration of rest of proxy route test away from hapi Add test for custom route validation * Bring console application in line with https://github.com/elastic/kibana/blob/master/src/core/CONVENTIONS.md#applications Change log from info level to debug level for console_extensions plugin * Update i18nrc file for console * Add setHeaders when passing back error response Co-authored-by: Elastic Machine <[email protected]> # Conflicts: # .github/CODEOWNERS # src/legacy/core_plugins/console/server/__tests__/proxy_route/params.js # src/legacy/core_plugins/console/server/__tests__/proxy_route/query_string.js # src/plugins/console/server/lib/spec_definitions/spec/generated/get_script_context.json # x-pack/plugins/console_extensions/server/spec/overrides/security.delete_privileges.json # x-pack/plugins/console_extensions/server/spec/overrides/security.put_privileges.json
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / kibana-xpack-agent / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/machine_learning/anomaly_detection/advanced_job·ts.machine learning anomaly detection advanced job with categorization detector and default datafeed settings job creation displays details for the created job in the job listStandard OutStack TraceHistory
To update your PR or re-run it, just comment with: |
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.
Summary
Continuation of #43346. Move the
consoleANDconsole_extensionsto New Platform (NP).How to review
src/legacy/core_plugins/console_legacy/**/*src/plugins/console/public/*.tssrc/plugins/console/server/*.tsTesting
Because server has been migrated to NP in this PR it will be important to test thoroughly, we need to make sure that the
kibana.ymlconfig works as expected and that the proxying behaviour works as expected including fallback to nodes.Some things to run through (besides the Test Rail test case CC @cuff-links)
Outstanding/known issues:
Boom. This does not work with Console's proxying behaviour. This PR updates this behaviour insrc/core/server/http/router/response_adapter.ts. This needs independent review as it is a platform core update.Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.