Migrate Console to use Node http instead of Hapi to support GET requests with bodies #46200

cjcenizal · 2019-09-19T22:37:02Z

Fixes #11125. Replaces #39170.

Summary

This approach uses Node's http library. It contains an earlier commit which used the request library.

Considerations

Effects upon users with read-only permissions

Per #36635 ("Autocomplete support for read only users"), some admins limit users to read-only access of a few select indices. How does this change affect the experience of users with these limited permissions? I would expect these users to experience problems due to their GET requests being converted to POST requests on master, and that these changes would address this problem.

Support for streaming large responses

How are large responses handled on master? Are they streamed? If so then we need to ensure this behavior is unaffected by these changes.

DONE: Auth header is sent with ES request

I verified that the required auth header is passed along by trying to access an index to which the user didn't have permission.

DONE: Correct error response is shown when ES is down

When ES is unavailable, the request to HEAD /?pretty=true should resolve in:

{
  "statusCode": 401,
  "error": "Unauthorized",
  "message": "Unauthorized"
}

DONE: HEAD requests behave as expected

Create a document and then send a HEAD request for it.

POST twitter/_doc/0
{ "a": "b" }

HEAD twitter/_doc/0

Expected response:

200 - OK

DONE: JSON parse failures are returned

Create a document with an incorrectly formed payload.

POST twitter/_doc/0
{
  "a:" "b"
}

Expected response:

{
  "error": {
    "root_cause": [
      {
        "type": "mapper_parsing_exception",
        "reason": "failed to parse"
      }
    ],
    "type": "mapper_parsing_exception",
    "reason": "failed to parse",
    "caused_by": {
      "type": "json_parse_exception",
      "reason": "Unexpected character ('\"' (code 34)): was expecting a colon to separate field name and value\n at [Source: org.elasticsearch.common.bytes.BytesReference$MarkSupportingStreamInputWrapper@4dc2f408; line: 2, column: 9]"
    }
  },
  "status": 400
}

DONE: SSL/TLS is still supported

Tested with an SSL setup and verified everything works as normal.

Figure out what `rejectUnauthorized` was supposed to do

The rejectUnauthorized variable was originally an option passed to Hapi, introduced in https://github.com/elastic/kibana/pull/10611/files. What did it do? Do we still need it?

DONE: DELETE requests should support bodies

This is needed by the scroll API.

DONE: `cat` API responses still support formatting

Send a [cat](https://www.elastic.co/guide/en/elasticsearch/reference/current/cat.html] request with formatting enabled:

GET _cat/master?v

Expect a response formatted like this:

id                     host      ip        node
WgJsUc7lQjiFR9oX9HU6MA 127.0.0.1 127.0.0.1 CJs-MacBook-Pro.attlocal.net

DONE: GET requests which don't support bodies are rejected

Some APIs don't support bodies with GET requests and we still need to make sure they're being rejected.

GET twitter/_doc/0
{
  "a": "b"
}

On master this will act as a POST and create a document, but it should be an error:

{
  "error": {
    "root_cause": [
      {
        "type": "illegal_argument_exception",
        "reason": "request [GET /twitter/_doc/0] does not support having a body"
      }
    ],
    "type": "illegal_argument_exception",
    "reason": "request [GET /twitter/_doc/0] does not support having a body"
  },
  "status": 400
}

`proxyConfig` and `proxyFilters` should still work

We should remove these settings for 8.0 (#39494), but for 7.x they're still supported. For more information on them see the 5.0 console docs.

cjcenizal · 2019-09-19T22:39:39Z

src/legacy/core_plugins/console/server/proxy_route.js

@@ -105,42 +103,54 @@ export const createProxyRoute = ({
      const { path, method } = query;
      const uri = resolveUri(baseUrl, path);

+      // Because this can technically be provided by a settings-defined proxy config, we need to
+      // preserve these property names to maintain BWC.
      const { timeout, rejectUnauthorized, agent, headers } = getConfigForReq(req, uri);


Note that rejectUnauthorized and agent are currently not being consumed. We should verify what kind of behavior depended upon them originally and ensure that behavior still exists.

src/legacy/core_plugins/console/server/proxy_route.js

… requests with bodies. - Remove payload configuration of endpoint.

elasticmachine · 2019-09-20T00:25:42Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-09-20T00:35:29Z

Pinging @elastic/es-ui

elasticmachine · 2019-09-20T01:34:30Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

Removed some whitespace Converted Elasticsearch proxy config to TS (now we can see the types with https.AgentOptions) Wrap request in util.promisify and refactor Use 'url' lib for parsing URLs Remove rejectUnauthorized from proxy_route.js (this is a TLS setting handled in agent setup)

jloleysens · 2019-09-24T10:59:40Z

@elasticmachine merge upstream

elasticmachine · 2019-09-24T12:45:52Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…t when piping to request.

elasticmachine · 2019-09-24T15:41:35Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…ing streaming.

elasticmachine · 2019-09-24T18:47:56Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…ng (oops)

jloleysens · 2019-09-27T08:31:57Z

@elasticmachine merge upstream

elasticmachine · 2019-09-27T09:39:16Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

jbudz · 2019-09-27T17:55:38Z

+1 on the non request implementation, ref request/request#3142. It looks like it's not used but still in the title/description

src/legacy/core_plugins/console/server/request.ts

jbudz · 2019-09-27T18:06:09Z

src/legacy/core_plugins/console/server/request.ts

+    headers: {
+      ...headers,
+      'content-type': 'application/json',
+      'transfer-encoding': 'chunked',


i don't feel very strongly about this, but think it's fine. the query may have a lot of data, but i would expect the ui to lock up before that's an issue. ace used to have a max of 65k lines but i'm a little out of date

cjcenizal

LGTM @jloleysens! I had some nitpicks on the code, but feel free to ignore them. I tested locally and marked everything with "DONE" that I was able to verify. Here are the things I haven't looked into because they're not things I'm familiar with:

Effects upon users with read-only permissions: we could possibly ignore this one since I was just pointing out that this change may actually address the reported issue
Support for streaming large responses: from @jbudz's comment it sounds like streaming is not actually a concern?
Figure out what rejectUnauthorized was supposed to do: not sure if it's worth digging into this since auth seems to behave as expected
proxyConfig and proxyFilters should still work: this is worth some light testing -- these features are deprecated and we stopped documenting them with 6.x, but we don't want to break things for anyone who still happens to be using this

cjcenizal · 2019-09-27T18:11:41Z

src/legacy/core_plugins/console/server/request.ts

+  headers: http.OutgoingHttpHeaders;
+}
+
+// We use a modified version of Hapi's Wreck because  Hapi, Axios, and Superagent don't support GET requests


Nit: got an extra space between "because" and "Hapi"

cjcenizal · 2019-09-27T18:14:35Z

src/legacy/core_plugins/console/server/request.ts

+import { URL } from 'url';
+
+interface Args {
+  method: string;


cjcenizal · 2019-09-27T18:23:34Z

src/legacy/core_plugins/console/server/request.ts

+  const req = client.request({
+    method: method.toUpperCase(),
+    host: uri.hostname,
+    port: uri.port !== '' ? Number(uri.port) : undefined,


Nit: I think this would be easier to scan if we avoid the negative comparison:

port: uri.port === '' ? undefined : Number(uri.port),

cjcenizal · 2019-09-27T18:24:51Z

src/legacy/core_plugins/console/server/request.ts

+    host: uri.hostname,
+    port: uri.port !== '' ? Number(uri.port) : undefined,
+    protocol: uri.protocol,
+    path: `${uri.pathname}${uri.search || ''}`,


Nit: the code might be easier to scan if we destructure all of these uri properties on line 40, so we don't need to keep referring to it.

jloleysens · 2019-09-30T10:19:51Z

@cjcenizal

Thanks for looking through and doing manual testing! I can confirm on the points you did not test:

I think this should address GETs with bodies. Which, to my understanding, should address the perms issues
I was slightly concerned that transfer-encoding header might not be compatible with what people are running in the wild. But it looks like this is a safe setting and should be the default fallback in current master too. I still think piping through incoming ES response streams is the behaviour we want here and shouldn't be an issue.
rejectUnauthorized is used in Wreck to create an http(s) agent with specific behaviour for throwing under certain conditions when an agent isn't provided or as an explicit per request override. The way we are setting up agents already does this legwork and so the behaviour is the same as before in this regard. I did notice that we weren't respecting setting this value via the legacy/deprecated proxy config so have re-enabled this.
Did a manual test with proxy config (set up a config to my local ES instance) and confirmed that it was work through that path too.

…tings

jloleysens · 2019-09-30T10:23:59Z

@elasticmachine merge upstream

elasticmachine · 2019-09-30T11:28:10Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…sts with bodies (#46200) (#46953) * Cleaned up use of es.send API - Converted Elasticsearch proxy config to TS (now we can see the types with https.AgentOptions) - Wrap request in util.promisify and refactor - Use 'url' lib for parsing URLs - Remove rejectUnauthorized from proxy_route.js (this is a TLS setting handled in agent setup) * Retained original proxying behavior * Re-enable support for setting rejectUnauthorized via proxy config settings * Updated tests.

elasticmachine · 2019-10-14T18:57:45Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 853af2f

cjcenizal commented Sep 19, 2019

View reviewed changes

src/legacy/core_plugins/console/server/proxy_route.js Outdated Show resolved Hide resolved

cjcenizal commented Sep 19, 2019

View reviewed changes

src/legacy/core_plugins/console/server/proxy_route.js Outdated Show resolved Hide resolved

Migrate Console to use request library instead of Hapi to support GET…

254c605

… requests with bodies. - Remove payload configuration of endpoint.

cjcenizal force-pushed the console/support-get-requests branch from b88fe9d to 254c605 Compare September 19, 2019 23:48

cjcenizal added Feature:Console Dev Tools Console Feature Feature:Dev Tools release_note:fix Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.5.0 v8.0.0 labels Sep 20, 2019

cjcenizal mentioned this pull request Sep 20, 2019

Remove coercion of Console GET requests to POST #39170

Closed

11 tasks

Merge branch 'master' into console/support-get-requests

2239109

jloleysens added 2 commits September 24, 2019 17:16

Back to streaming payloads - now reading headers off of payload objec…

89bc8a2

…t when piping to request.

Updated tests.

d3e5a43

Text-encoding: chunking may cause issues in some environments. Not us…

fe9ff1b

…ing streaming.

jloleysens marked this pull request as ready for review September 24, 2019 17:03

jloleysens added 2 commits September 27, 2019 10:28

Same proxying behavior as before (without request)

cf3e49e

Fixed tests and using uri.protocol variable instead of hardcoded stri…

e4e670c

…ng (oops)

Merge branch 'master' into console/support-get-requests

bfa1dae

jbudz reviewed Sep 27, 2019

View reviewed changes

src/legacy/core_plugins/console/server/request.ts Show resolved Hide resolved

jbudz reviewed Sep 27, 2019

View reviewed changes

cjcenizal commented Sep 27, 2019

View reviewed changes

jloleysens changed the title ~~Migrate Console to use request library instead of Hapi to support GET requests with bodies~~ Migrate Console to use node http instead of Hapi to support GET requests with bodies Sep 30, 2019

jloleysens changed the title ~~Migrate Console to use node http instead of Hapi to support GET requests with bodies~~ Migrate Console to use Node http instead of Hapi to support GET requests with bodies Sep 30, 2019

jloleysens added 3 commits September 30, 2019 12:21

implement feedback

a39096b

Added timeout and abort logic test for response.ts

a7cb2be

Re-enable support for setting rejectUnauthorized via proxy config set…

0419685

…tings

Merge branch 'master' into console/support-get-requests

853af2f

jloleysens approved these changes Sep 30, 2019

View reviewed changes

cjcenizal merged commit 3316d44 into elastic:master Sep 30, 2019

cjcenizal deleted the console/support-get-requests branch September 30, 2019 17:19

cjcenizal mentioned this pull request Sep 30, 2019

[7.x] Migrate Console to use Node http instead of Hapi to support GET requests with bodies (#46200) #46953

Merged

jloleysens mentioned this pull request Mar 3, 2020

[Console] Fix overriding of host header name #59143

Merged

2 tasks

jasimsulaiman mentioned this pull request Mar 10, 2020

GET a specific document in Sense with a request body overwrites it elastic/elasticsearch#27140

Closed

cjcenizal mentioned this pull request Oct 1, 2020

Kibana Dev Console: GET <index>/_doc/<id> request with payload create/updates document #77276

Closed

cjcenizal mentioned this pull request May 26, 2021

GET method becomes a POST if an object is declared together #92118

Closed

alisonelizabeth mentioned this pull request Jan 3, 2023

Update console.asciidoc note transform GET with body to POST #108265

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate Console to use Node http instead of Hapi to support GET requests with bodies #46200

Migrate Console to use Node http instead of Hapi to support GET requests with bodies #46200

cjcenizal commented Sep 19, 2019 •

edited

Loading

cjcenizal Sep 19, 2019

elasticmachine commented Sep 20, 2019

elasticmachine commented Sep 20, 2019

elasticmachine commented Sep 20, 2019

jloleysens commented Sep 24, 2019

elasticmachine commented Sep 24, 2019

elasticmachine commented Sep 24, 2019

elasticmachine commented Sep 24, 2019

jloleysens commented Sep 27, 2019

elasticmachine commented Sep 27, 2019

jbudz commented Sep 27, 2019

jbudz Sep 27, 2019

jbudz Sep 27, 2019

cjcenizal left a comment

cjcenizal Sep 27, 2019

cjcenizal Sep 27, 2019

cjcenizal Sep 27, 2019

cjcenizal Sep 27, 2019

jloleysens commented Sep 30, 2019

jloleysens commented Sep 30, 2019

elasticmachine commented Sep 30, 2019

elasticmachine commented Oct 14, 2019

Migrate Console to use Node http instead of Hapi to support GET requests with bodies #46200

Migrate Console to use Node http instead of Hapi to support GET requests with bodies #46200

Conversation

cjcenizal commented Sep 19, 2019 • edited Loading

Summary

Considerations

Effects upon users with read-only permissions

Support for streaming large responses

DONE: Auth header is sent with ES request

DONE: Correct error response is shown when ES is down

DONE: HEAD requests behave as expected

DONE: JSON parse failures are returned

DONE: SSL/TLS is still supported

Figure out what rejectUnauthorized was supposed to do

DONE: DELETE requests should support bodies

DONE: cat API responses still support formatting

DONE: GET requests which don't support bodies are rejected

proxyConfig and proxyFilters should still work

cjcenizal Sep 19, 2019

Choose a reason for hiding this comment

elasticmachine commented Sep 20, 2019

💔 Build Failed

elasticmachine commented Sep 20, 2019

elasticmachine commented Sep 20, 2019

💔 Build Failed

jloleysens commented Sep 24, 2019

elasticmachine commented Sep 24, 2019

💔 Build Failed

elasticmachine commented Sep 24, 2019

💔 Build Failed

elasticmachine commented Sep 24, 2019

💚 Build Succeeded

jloleysens commented Sep 27, 2019

elasticmachine commented Sep 27, 2019

💚 Build Succeeded

jbudz commented Sep 27, 2019

jbudz Sep 27, 2019

Choose a reason for hiding this comment

jbudz Sep 27, 2019

Choose a reason for hiding this comment

cjcenizal left a comment

Choose a reason for hiding this comment

cjcenizal Sep 27, 2019

Choose a reason for hiding this comment

cjcenizal Sep 27, 2019

Choose a reason for hiding this comment

cjcenizal Sep 27, 2019

Choose a reason for hiding this comment

cjcenizal Sep 27, 2019

Choose a reason for hiding this comment

jloleysens commented Sep 30, 2019

jloleysens commented Sep 30, 2019

elasticmachine commented Sep 30, 2019

💚 Build Succeeded

elasticmachine commented Oct 14, 2019

💚 Build Succeeded

cjcenizal commented Sep 19, 2019 •

edited

Loading

Figure out what `rejectUnauthorized` was supposed to do

DONE: `cat` API responses still support formatting

`proxyConfig` and `proxyFilters` should still work