Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Console] Move out of legacy + migrate server side to New Platform #55690

Merged
merged 19 commits into from
Jan 30, 2020
Merged

[Console] Move out of legacy + migrate server side to New Platform #55690

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
dec28e9
Initial move of public and setup of server skeleton
jloleysens Jan 21, 2020
2426fcf
Fix public paths and types
jloleysens Jan 21, 2020
29db598
Use new usage stats dependency directly in tracker also mark as an op…
jloleysens Jan 21, 2020
0e8fae1
WiP on getting server side working
jloleysens Jan 22, 2020
9f12049
Restore proxy route behaviour for base case, still need to test custo…
jloleysens Jan 22, 2020
d73a3e9
Add new type and lib files
jloleysens Jan 22, 2020
bbe729b
Clean up legacy start up code and add comment about issue in kibana.y…
jloleysens Jan 22, 2020
a3c85d9
Move console_extensions to new platform and introduce ConsoleSetup AP…
jloleysens Jan 23, 2020
212a1a3
Re-introduce injected elasticsearch variable and use it in public
jloleysens Jan 23, 2020
46c5cab
Don't pass stateSetter prop through to checkbox
jloleysens Jan 23, 2020
5f76f38
Merge branch 'master' of github.com:elastic/kibana into np/console/mo…
jloleysens Jan 28, 2020
4f7f3c5
Refactor of proxy route (split into separate files). Easier testing f…
jloleysens Jan 28, 2020
79a9803
headers.js test -> headers.test.ts and moved some of the proxy route …
jloleysens Jan 28, 2020
485a462
Finish migration of rest of proxy route test away from hapi
jloleysens Jan 28, 2020
541ecf5
Bring console application in line with https://github.com/elastic/kib…
jloleysens Jan 28, 2020
90d7b72
Update i18nrc file for console
jloleysens Jan 28, 2020
5d0082b
Merge branch 'master' into np/console/move-out-legacy
elasticmachine Jan 28, 2020
0c2dfde
Add setHeaders when passing back error response
jloleysens Jan 30, 2020
217d5ae
Merge branch 'master' into np/console/move-out-legacy
elasticmachine Jan 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
4 changes: 2 additions & 2 deletions .github/CODEOWNERS
View file
Open in desktop
Validating CODEOWNERS rules …
Original file line number Diff line number Diff line change
Expand Up @@ -148,9 +148,9 @@
**/*.scss @elastic/kibana-design

# Elasticsearch UI
/src/legacy/core_plugins/console/ @elastic/es-ui
/src/plugins/console/ @elastic/es-ui
/src/plugins/es_ui_shared/ @elastic/es-ui
/x-pack/legacy/plugins/console_extensions/ @elastic/es-ui
/x-pack/plugins/console_extensions/ @elastic/es-ui
/x-pack/legacy/plugins/cross_cluster_replication/ @elastic/es-ui
/x-pack/legacy/plugins/index_lifecycle_management/ @elastic/es-ui
/x-pack/legacy/plugins/index_management/ @elastic/es-ui
Expand Down
9 changes: 8 additions & 1 deletion src/core/server/http/router/response_adapter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import { ResponseObject as HapiResponseObject, ResponseToolkit as HapiResponseToolkit } from 'hapi';
import typeDetect from 'type-detect';
import Boom from 'boom';
import * as stream from 'stream';

import {
HttpResponsePayload,
Expand Down Expand Up @@ -112,8 +113,14 @@ export class HapiResponseAdapter {
return response;
}

private toError(kibanaResponse: KibanaResponse<ResponseError>) {
private toError(kibanaResponse: KibanaResponse<ResponseError | Buffer | stream.Readable>) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it may be more appropriate to add Buffer | stream.Readable to the ResponseError union type. @restrry WDYT?

Copy link
Contributor

@mshustov mshustov Jan 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When it's required? As I can see proxyRequest rejects a promise with an error message, not steam or a buffer

kibana/src/plugins/console/server/lib/request.ts

Line 92 in 0e8fae1

const onError = (e: Error) => reject(e);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

kibana/src/plugins/console/server/routes/api/console/proxy/create_handler.ts

Line 166 in 541ecf5

esIncomingMessage = await proxyRequest({

Copy link
Contributor

@mshustov mshustov Jan 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const { payload } = kibanaResponse;

// Special case for when we are proxying requests and want to enable streaming back error responses opaquely.
if (Buffer.isBuffer(payload) || payload instanceof stream.Readable) {
return this.responseToolkit.response(payload).code(kibanaResponse.status);
}

// we use for BWC with Boom payload for error responses - {error: string, message: string, statusCode: string}
const error = new Boom('', {
statusCode: kibanaResponse.status,
Expand Down
187 changes: 0 additions & 187 deletions src/legacy/core_plugins/console/index.ts
View file
Open in desktop

This file was deleted.

8 changes: 0 additions & 8 deletions src/legacy/core_plugins/console/package.json
View file
Open in desktop

This file was deleted.

53 changes: 0 additions & 53 deletions src/legacy/core_plugins/console/public/legacy.ts
View file
Open in desktop

This file was deleted.

Loading