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 client: allow calling API as a user so that deprecation warnings are surfaced #120201

Closed
rudolf opened this issue Dec 2, 2021 · 11 comments
Labels
impact:needs-assessment Product and/or Engineering needs to evaluate the impact of the change. loe:small Small Level of Effort Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.16.2 v7.17.0

Comments

@rudolf
Copy link
Contributor

rudolf commented Dec 2, 2021

Upgrade assistant will hide deprecation warnings for requests made with x-elastic-product-origin: kibana. However, deprecations from some API calls should explicitly be surfaced, e.g. api calls from the console app.

In order to do this, plugins will need to unset the x-elastic-product-origin: kibana header.

One way to achieve that would be to change the custom Kibana transport for the Elasticsearch-js client so that when a api request is made with headers: {'x-elastic-product-origin': null} we remove the x-elastic-product-origin header from the request altogether.

Related: #120043

@rudolf rudolf added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.16.1 labels Dec 2, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@afharo
Copy link
Member

afharo commented Dec 2, 2021

how about an isExternal flag? It can be used for every plugin that allow custom requests. This way we hide the implementation details (which headers we actually rely on), and we can even differentiate between Kibana-internal requests, and user-generated ones (for telemetry and deprecation purposes).

What do you think?

@pgayvallet
Copy link
Contributor

how about an isExternal flag? It can be used for every plugin that allow custom requests.

We're exposing vanilla instances of Client from @elastic/elasticsearch, not sure how/where you're suggested adding this flag?

@afharo
Copy link
Member

afharo commented Dec 3, 2021

That is a very strong point! I guess it's not worth overcomplicating this then.

So the scope of this issue is to allow the header "reset" and document that this is possible?

@rudolf
Copy link
Contributor Author

rudolf commented Dec 6, 2021

yeah, given the timelines and our architecture that seems like the most pragmatic.

@pgayvallet
Copy link
Contributor

to change the custom Kibana transport for the Elasticsearch-js client so that when a api request is made with headers: {'x-elastic-product-origin': null} we remove the x-elastic-product-origin header from the request altogether.

Hum, I thought we were the ones setting the x-elastic-product-origin in our custom transport, but TIL this is actually done from deeper in the client or transport. If using headers: {'x-elastic-product-origin': null} when calling an API of the client with current implementation doesn't remove the header, I'm unsure how we can adapt our transport to get to this behavior?

@delvedor maybe you can help us here?

@mshustov
Copy link
Contributor

mshustov commented Dec 7, 2021

In order to do this, plugins will need to unset the x-elastic-product-origin: kibana header.

x-elastic-product-origin is added to restrict access to system indices. from #81536

Additionally, Kibana can continue accessing these indices using the standard ES APIs
while specifying a special HTTP request header and value pair.
It's my understanding that the ES implementation will require the 'x-elastic-product-origin': 'kibana'
header to be specified to denote these requests as originating from Kibana.

It means that plugin setting headers: {'x-elastic-product-origin': null} won't have access to system indices. Is that okay?

Upgrade assistant will hide deprecation warnings for requests made with x-elastic-product-origin: kibana

IMO we are blurring the responsibility of this header. x-elastic-product-origin single responsibility is to indicate that a request is initiated by Kibana.
Maybe we need another header to distinguish between a required triggered by a user or Kibana itself?
We already use kbn-system-request to differentiate requests sent from the Kibana browser app

fetchOptions.headers['kbn-system-request'] = 'true';

I'd suggest to use API identical to the client-side to configure system requests:

client.seatch({...}, { asSystemRequest: true})

In theory we can extend TransportRequestOptions interface provided by elasticsearch-js to support a custom setting.

Another problem that we will need to solve is to enforce kbn-system-request usage when necessary. We didn't solve this problem for the client-side logic #79889 (comment)

However, deprecations from some API calls should explicitly be surfaced, e.g. api calls from the console app.

Hm. AFAIK Console app doesn't set x-elastic-product-origin: kibana header. #90123

@mshustov
Copy link
Contributor

mshustov commented Dec 10, 2021

@delvedor suggested deleting the x-elastic-product-origin header on the Transport level. I don't think it's possible (see https://github.com/elastic/elasticsearch-js/blob/7.16/lib/Transport.js#L382) because prepareRequest is called inside of Transport.request method.

Unfortunatelly, we can't work around it by setting the header as undefined.

client.get(..., { headers: { 'x-opaque-id': undefined } })

since the ES client throws TypeError: Invalid value "undefined" for header "x-elastic-product-origin"
But we can set the header conditionally in Transport

  class KibanaTransport extends Transport {
    request(params: TransportRequestParams, options?: TransportRequestOptions) {
      const opts = options || {};
      const opaqueId = getExecutionContext();
      if (opaqueId && !opts.opaqueId) {
        // rewrites headers['x-opaque-id'] if it presents
        opts.opaqueId = opaqueId;
      }
      if (!opts.headers || !('x-elastic-product-origin' in opts.headers)) {
        opts.headers = opts.headers || {};
        opts.headers['x-elastic-product-origin'] = 'kibana';
      }
      return super.request(params, opts);
    }

In theory we can extend TransportRequestOptions interface provided by elasticsearch-js to support a custom setting.

After thinking a bit more, I realized it's not the best option because we will have to add this global d.ts file to every TS project. Other alternatives aren't good either: to declare a custom TS client interface for the ES client or allow headers: { 'x-opaque-id': ... } }) implementation detail to leak into the plugin code.

Hm. AFAIK Console app doesn't set x-elastic-product-origin: kibana header

UPD: it doesn't set the header. #90123
it doesn't use elasticseach-js client either https://github.com/elastic/kibana/blob/ff929b8845cf3423d59312bdee637e90ff3751ee/src/plugins/console/server/lib/proxy_request.ts

@planadecu planadecu pinned this issue Dec 13, 2021
@planadecu planadecu unpinned this issue Dec 13, 2021
@exalate-issue-sync exalate-issue-sync bot added impact:needs-assessment Product and/or Engineering needs to evaluate the impact of the change. loe:small Small Level of Effort labels Jan 5, 2022
@lukeelmers
Copy link
Member

cc @thesmallestduck @arisonl for input on priority for this

@pgayvallet
Copy link
Contributor

@lukeelmers @rudolf is that something that may come back for 9.0? Or should we close the issue?

@rudolf
Copy link
Contributor Author

rudolf commented Apr 17, 2024

As far as I know there are no other use cases where Kibana should not add it's header, so it's only console. Everywhere else Kibana controls which APIs it calls so it's our responsibility to stop depending on the deprecated APIs before 9.

I double checked and what @mshustov wrote is still true, so the console app doesn't need any workarounds, it already doesn't include this header.

UPD: it doesn't set the header. #90123
it doesn't use elasticseach-js client either https://github.com/elastic/kibana/blob/ff929b8845cf3423d59312bdee637e90ff3751ee/src/plugins/console/server/lib/proxy_request.ts

So I'll close this.

@rudolf rudolf closed this as completed Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact:needs-assessment Product and/or Engineering needs to evaluate the impact of the change. loe:small Small Level of Effort Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.16.2 v7.17.0
Projects
None yet
Development

No branches or pull requests

6 participants