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.

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

Elasticsearch.js client in the new platform #12442

Closed
kimjoar opened this issue Jun 21, 2017 · 6 comments
Closed

Elasticsearch.js client in the new platform #12442

kimjoar opened this issue Jun 21, 2017 · 6 comments
Labels
Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@kimjoar
Copy link
Contributor

kimjoar commented Jun 21, 2017

Our current callWithRequest and callWithInternalUser have a couple problems, mainly:

  1. we either have to inject the request all over the place or we create lots of these helpers

    const callAdminCluster = (...args) => callWithRequest(req, ...args);

    that we then pass into services.

  2. we end up attaching "request-scoped" services directly on the request instead of the approach above. However, this doesn't scale nicely with our plugin system, as we want plugins to only receive values from direct dependencies, not transitive dependencies. Also, the only reason we're currently doing this is because we want to scope the esclient to a request.

  3. it's not possible to type them because they rely too much on strings and js dynamicness

As part of the new platform work I think we should find a better solution for this pattern.

So, what we want is basically: The ability to have a cluster object that's "preset" with a specific http request (or none, in the case of callWithInternalUser), and I want to be able to type the requests we do to Elasticsearch.

As of now, to perform the request we only need:

  • the headers defined in the config field elasticsearch.requestHeadersWhitelist, which defaults to [ 'authorization' ]
  • the request url in order to grab the current Space
    To still avoid passing around the request object, we can expose an API that constructs a permissions-defining object containing all the things from request that we need (that Security needs, for example).

Implementation

In #14980 we implemented a way to avoid passing the request object, by providing functions in the Elasticsearch Service that can create scoped and unscoped DataClient and AdminClient.

For example, in the request handler method of a plugin that depends on the ElasticsearchService, you define a client:

const client = await elasticsearch.getScopedDataClient(req.headers);

This is a DataClient which accesses the data cluster of Elasticsearch, but it is scoped to the user from the request header. This means every request that is handled by this request handler method will instantiate a new DataClient scoped to that request. But that's not all, keep reading: internally to the constructor that creates the DataClient object, it actually uses the same, single clients.data Observable created within ElasticsearchService when the service is created. That means we always only ever have at most two clients: an admin client, and a data client. But the AdminClient and DataClient objects exposed by the Elasticsearch Service are offered as ways to wrap those static clients with request-related information.

The PR has some examples of other clients we might need and where they're used.

Future

We are already aware of a future where we need the current Space from the request url to scope and restrict access to Kibana Saved Objects. This means that request handlers need more than just the request.headers for creating scoped ES clients. Something like:

// validate the request object before the handler gets it,
// so that only user and space id are exposed, not the whole URL or headers.
async handler (req, res) {
  ...
  const client = await elasticsearch.getScopedDataClient({ req.user, req.spaceId });
  ...
}

might suffice, and internally to the DataClient:

call(endpoint: string, clientParams = {}, options = {}): any {
    // scope to user access
    return this.client.search({
      // filter by space id here
    });
}

.. something to make searches automatically filtered by Space as well as user.

Maybe in the request validation, we can have the Router construct a permissions object like:

req.credentials = { user, spaceId, anythingElse, ... };

that would be automatically available on every request object passed to a handler:

async handler (req, res) {
  const { user, spaceId, anythingElse } = req.credentials;
  ...
}

Security

Whether or not X-Pack Security is enabled, we always will need to create scoped and unscoped admin and data clients, because of internal Kibana operations outside of security that need to do things. For example, as long as we have the health check, it uses the internal user against the data cluster, so it needs an unscoped DataClient.

The Security plugin will create its own version of the Elasticsearch client, then use that for searches, something like:

async securityHandler (req, res) {
  const client = await elasticsearch.createClient(... whatever security plugin needs ...);

  const securityService = new SecurityService(client, ...);

  const items = securityService.find({...});
}

I think most likely, it needs a way to create a client that hits the admin cluster with the current user, and possibly space if Spaces is enabled. A helper like createClient exposed on Elasticsearch Service might help with that.


First attempt (outdated)

This is one exploration I did that enables types (using the elasticsearch.js types from npm), but which still requires the http requests to be passed around.

const response = await adminCluster.withRequest(
  this.req,
  (client, headers) =>
    client.search({
      headers,
      index: kibanaIndex,
      type,
      size: perPage,
      from: perPage * (page - 1)
    })
);

Here you can see the types on the response:

ts-example

However, because we can't "preset" headers on the client we need to handle them ourselves, which makes for a really bad api. Also, this fully exposes the client, which I think we should try to avoid.

One idea we've discussed is to build a Kibana specific "wrapper" around client that can be typed and that can be preset with an http request (or just the specific headers). We don't have to "copy" the entire client api, just the requests we actually need as we go. There are pros and cons to this, though.

One con is what happens when someone needs access to an api that we haven't exposed yet, but I think we can solve this by giving access to a fully untyped api similar to what we have today.

Maybe getting a cluster could be something like this:

const cluster = elasticsearchService.getCluster('admin', request);
const cluster = elasticsearchService.getClusterWithInternalUser('admin');
const cluster = elasticsearchService.getCluster('data', request);

or maybe something like:

const scopedElasticsearchService = elasticsearchService.scopedTo(request);

const adminCluster = scopedElasticsearchService.getClusterOfType('admin');
const dataCluster = scopedElasticsearchService.getClusterOfType('data');

Or something entirely different.

Then we could add search and other apis directly on this cluster object, e.g.

cluster.search({
  index: kibanaIndex,
  type,
  size: perPage,
  from: perPage * (page - 1)
});

which would call through the underlying esClient.

It could also be worth exploring ways of receiving this cluster directly in our http handlers in some way.

@kimjoar kimjoar added the Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc label Jun 21, 2017
@kimjoar kimjoar changed the title Handling http requests and the Elasticsearch.js client Elasticsearch.js client in the new platform Sep 19, 2017
@kimjoar kimjoar self-assigned this Sep 19, 2017
@kimjoar kimjoar removed their assignment Nov 22, 2017
@rhoboat rhoboat self-assigned this Dec 4, 2017
@epixa
Copy link
Contributor

epixa commented May 5, 2018

Related to #18841

@epixa epixa added the discuss label May 5, 2018
@epixa epixa removed the discuss label May 6, 2018
@rhoboat rhoboat assigned rhoboat and unassigned rhoboat May 10, 2018
@rhoboat
Copy link

rhoboat commented May 11, 2018

The first step toward solving this issue is in Alternative to request scoped services which has been merged. Updating the issue description to reflect these changes.

@rhoboat
Copy link

rhoboat commented May 11, 2018

Just brainstorming...

Would it work to have the security plugin create its own esClient, given the discussion above? Does that even make sense? What does it need to add to the barebones elasticsearch-js client inside of Elasticsearch Service here?

does monitoring plugin need a special esClient too? what does it need?

Do other plugins declare security as a dependency, and when security is enabled, and do they need to use security's esClient?

Could this happen?

// init function for plugin
plugin: (kibana, deps) => {
  const { security } = deps;
  ...
  registerEndpoints(security, ...);
}

// when handling requests
function registerEndpoints(security, ...) {
  ...
  async handler (req, res) {
    // maybe it also needs its own client?
    const client = await elasticsearch.getScopedDataClient(creds);

    // maybe it needs the security client for some things?
    const items1 = await security.client.find({...});
    const items2 = await security.client.search({...});
    return { items1, items2 };
  }
}

@epixa
Copy link
Contributor

epixa commented May 11, 2018

Security should impact the clients that are provided through the es service without any knowledge from the plugins that use those clients. That could mean the security plugin interacts with extension points from the es service that change client behaviors, or could it mean the security plugin interacts with an extension point on the es service that outright replaces the clients in the es service, but it wouldn't want to register a new and special "security" client.

@rhoboat
Copy link

rhoboat commented May 11, 2018

Yup, I'm on board now. I have a new proposal coming...

@epixa
Copy link
Contributor

epixa commented Apr 23, 2019

I'm going to close this issue out now that #28344 is finished and the new elasticsearch-js library is published. I think these two things together will address most of what this issue describes, and we can create specific issues for anything else.

@epixa epixa closed this as completed Apr 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

No branches or pull requests

3 participants