Skip to content

Allow passing in a signal to abort a cluster client request#37563

Merged
lukasolson merged 4 commits intoelastic:masterfrom
lukasolson:clusterClientAbort
Jun 12, 2019
Merged

Allow passing in a signal to abort a cluster client request#37563
lukasolson merged 4 commits intoelastic:masterfrom
lukasolson:clusterClientAbort

Conversation

@lukasolson
Copy link
Copy Markdown
Contributor

@lukasolson lukasolson commented May 30, 2019 •
edited by joshdover
Loading

Resolves #37359.

This PR provides an additional optional option to the CallAPI options, signal, which represents a signal from an AbortController object. If provided, then controller.abort is invoked, then the request to ES itself will also be aborted.

Dev Docs

callWithRequest now accepts a AbortController signal for aborting requests to Elasticsearch. Example usage:

const { callWithRequest } = server.plugins.elasticsearch.getCluster('data');
const controller = new AbortController();
req.events.once('disconnect', () => controller.abort());
return await callWithRequest(req, 'search', params, { signal: controller.signal });

@lukasolson lukasolson added review Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// labels May 30, 2019
@lukasolson lukasolson requested review from azasypkin and joshdover May 30, 2019 21:55
@lukasolson lukasolson requested a review from a team as a code owner May 30, 2019 21:55
@lukasolson lukasolson self-assigned this May 30, 2019
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented May 30, 2019

Pinging @elastic/kibana-platform

@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented May 30, 2019

💔 Build Failed

@lukasolson lukasolson force-pushed the clusterClientAbort branch from 0ccd052 to 812b644 Compare May 30, 2019 22:29
eliperelman
eliperelman reviewed May 30, 2019
View reviewed changes
src/core/server/elasticsearch/cluster_client.ts Outdated
Copy link
Copy Markdown
Contributor

@eliperelman eliperelman May 30, 2019

Choose a reason for hiding this comment

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

Not a requirement, but if you want to achieve this without mutating signal and therefore options, you can use:

options.signal.addEventListener('abort', () => request.abort());

src/core/server/elasticsearch/cluster_client.test.ts Outdated
Copy link
Copy Markdown
Contributor

@eliperelman eliperelman May 30, 2019

Choose a reason for hiding this comment

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

Instead of testing the AbortSignal itself, can we call abort and ensure the call fails?

@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented May 30, 2019

💔 Build Failed

@lukasolson lukasolson mentioned this pull request Jun 12, 2019
Closed
4 tasks
@lukasolson lukasolson force-pushed the clusterClientAbort branch from 812b644 to 40cfb39 Compare June 12, 2019 18:44
@lukasolson
Copy link
Copy Markdown
Contributor Author

lukasolson commented Jun 12, 2019

@eliperelman Could you give this another look?

@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Jun 12, 2019

💔 Build Failed

@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Jun 12, 2019

💔 Build Failed

@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Jun 12, 2019

💚 Build Succeeded

@lukasolson lukasolson merged commit 9e472e6 into elastic:master Jun 12, 2019
@lukasolson lukasolson deleted the clusterClientAbort branch June 12, 2019 22:45
lukasolson added a commit that referenced this pull request Jun 12, 2019
@lukasolson
Allow passing in a signal to abort a cluster client request (#37563)
6ca6b1a 
* Allow passing in a signal to abort an Elasticsearch request using the cluster client

* Go back to using promises (which still return abort method) and update test

* Update docs

* Explicitly return Promise<any> instead of {}
@lukasolson
Copy link
Copy Markdown
Contributor Author

lukasolson commented Jun 12, 2019

7.x (7.3.0): 6ca6b1a

@joshdover joshdover added the release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. label Jul 9, 2019
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Jul 9, 2019

💚 Build Succeeded

@lukasolson lukasolson mentioned this pull request Aug 6, 2019
Merged
1 task
@lukasolson lukasolson mentioned this pull request Sep 26, 2019
Merged
4 tasks
@sorenlouv
Copy link
Copy Markdown
Contributor

sorenlouv commented Dec 9, 2019

@lukasolson I've started seeing the following error on master and traced it back to here:

server    log   [09:53:55.180] [error][http] { Error: [security_exception] action [indices:data/read/search] is unauthorized for user [elastic]
    at respond (/home/apm/elastic/kibana/node_modules/elasticsearch/src/lib/transport.js:349:15)
    at checkRespForFailure (/home/apm/elastic/kibana/node_modules/elasticsearch/src/lib/transport.js:306:7)
    at HttpConnector.<anonymous> (/home/apm/elastic/kibana/node_modules/elasticsearch/src/lib/connectors/http.js:173:7)
    at IncomingMessage.wrapper (/home/apm/elastic/kibana/node_modules/elasticsearch/node_modules/lodash/lodash.js:4929:19)
    at IncomingMessage.elasticAPMCallbackWrapper (/home/apm/elastic/kibana/node_modules/elastic-apm-node/lib/instrumentation/index.js:309:27)
    at IncomingMessage.emit (events.js:194:15)
    at endReadableNT (_stream_readable.js:1103:12)
    at process._tickCallback (internal/process/next_tick.js:63:19)
  status: 403,
  displayName: 'AuthorizationException',
  message:
   '[security_exception] action [indices:data/read/search] is unauthorized for user [elastic]',
  path: '/apm-*%2Capm-*%2C%2Capm-*/_search',
  query: { terminate_after: 1, ignore_throttled: true },
  body:
   { error:
      { root_cause: [Array],
        type: 'security_exception',
        reason:
         'action [indices:data/read/search] is unauthorized for user [elastic]',
        caused_by: [Object] },
     status: 403 },
  statusCode: 403,
  response:
   '{"error":{"root_cause":[{"type":"security_exception","reason":"action [indices:data/read/search] is unauthorized for user [elastic]"}],"type":"security_exception","reason":"action [indices:data/read/search] is unauthorized for user [elastic]","caused_by":{"type":"string_index_out_of_bounds_exception","reason":"String index out of range: 0"}},"status":403}',
  toString: [Function],
  toJSON: [Function] }
server   error  [09:53:55.047]  Error: Internal Server Error
    at HapiResponseAdapter.toInternalError (/home/apm/elastic/kibana/src/core/server/http/router/response_adapter.ts:55:19)
    at Router.handle (/home/apm/elastic/kibana/src/core/server/http/router/router.ts:297:34)
    at process._tickCallback (internal/process/next_tick.js:68:7)
TypeError: request.abort is not a function
    at AbortSignal.options.signal.addEventListener (/home/apm/elastic/kibana/src/core/server/elasticsearch/cluster_client.ts:67:19)
    at Timeout._onTimeout (/home/apm/elastic/kibana/node_modules/abortcontroller-polyfill/dist/cjs-ponyfill.js:153:27)
    at ontimeout (timers.js:436:11)
    at tryOnTimeout (timers.js:300:5)
    at listOnTimeout (timers.js:263:5)
    at Timer.processTimers (timers.js:223:10)
 server crashed  with status code 1

There are obviously some auth issues but the thing that makes the server crash is the request.abort is not a function error. Any idea?

@lukasolson
Copy link
Copy Markdown
Contributor Author

lukasolson commented Dec 9, 2019

Hmm, the object returned from calling the Elasticsearch JS client should always be returning something with an abort method (or so I thought). I wonder if the fact that the request is failing has something to do with why it's not there. Do you only get this with the associated auth issues?

@sorenlouv
Copy link
Copy Markdown
Contributor

sorenlouv commented Dec 9, 2019

Do you only get this with the associated auth issues?

It looks like it, yeah.

Since error tracking is now enabled in Kibana (opt-in) I was able to find the error with a bit more context. Does this give you anything?

image

@sorenlouv
Copy link
Copy Markdown
Contributor

sorenlouv commented Dec 10, 2019

@lukasolson Turns out this is caused by the APM node agent: #52617.
Sorry for the noise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@azasypkin azasypkin Awaiting requested review from azasypkin

@joshdover joshdover Awaiting requested review from joshdover

1 more reviewer

@eliperelman eliperelman eliperelman approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@lukasolson lukasolson

Labels

release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. review Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// v7.3.0 v8.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Ability to abort a request made with callWithRequest

5 participants

@lukasolson @elasticmachine @sorenlouv @eliperelman @joshdover