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

Kibana request headers #79218

Merged
merged 14 commits into from
Oct 6, 2020
Merged

Kibana request headers #79218

merged 14 commits into from
Oct 6, 2020

Conversation

kobelb
Copy link
Contributor

@kobelb kobelb commented Oct 1, 2020

Elasticsearch would like a way to identify when a request is coming from Kibana. This would allow them to add an exception to their system-index deprecation logs, so we don't get a deprecation logged prior to Kibana switching to the new system index APIs.

Within this PR, I implemented the changes to hard-code a User-Agent: Kibana header which is specified whenever a request is made to Elasticsearch using either the legacy or new Elasticsearch client. However, there is a complication introduced by the existing elasticsearch.customHeaders and elasticsearch.requestHeadersWhitelist kibana.yml settings. These settings allow the user to either set a hard-coded User-Agent header on every request or forward the User-Agent from the incoming HTTP request to Kibana.

We could theoretically only specify the User-Agent header whenever it isn't specified within these settings and give the user's configured value priority; however, this would cause Elasticsearch to log deprecations. If we no longer allow the user to specify the User-Agent in either elasticsearch.customHeaders or elasticsearch.requestHeadersWhitelist, this is a breaking change

I can't think of any situations where I've seen end-users using these settings with the User-Agent header, but given our lack of telemetry on these settings, I'm hesitant to introduce this breaking change.

Instead of using the User-Agent header we could theoretically use a different header, X-Less-Likely-To-Have-Conflicts, to lessen the likelihood of this impacting end-users but in the strictest sense, this will still be treated as a breaking change.

@joshdover, can you think of any other option that I'm overlooking here?

@gwbrown, do we have other options here which don't introduce breaking changes? Can we just not classify the Kibana indices as system-indices before making the necessary changes?

We currently allow end-users to set whatever headers they'd like to be
forwarded to Elasticsearch with `elasticsearch.customHeaders` and
`elasticsearch.requestHeadersWhitelist`. This is potentially problematic
with us always specifying `User-Agent: kibana` as it could interfere
with what our end-users have done...
@gwbrown
Copy link

gwbrown commented Oct 1, 2020

Kibana's indices have already been registered as System Indices since 7.8 (relevant PR: elastic/elasticsearch#54858). I'm not sure off the top of my head whether changing would cause issues, but either way it would be preferable to issue the deprecation warnings when users access .kibana directly, just like the other System Indices.

I'll think on this and report back if I come up with anything, but my first impression is that choosing a sufficiently obscure X-Less-Likely-To-Have-Conflicts header and allowing the user's requests to take priority is a decent option if it's not too difficult to implement - this would theoretically allow Kibana to trigger deprecation warnings, but only in a very edge-case-y configuration.

@gwbrown
Copy link

gwbrown commented Oct 2, 2020

I haven't been able to come up with a noticeably better solution yet. It would also be a possibility, if we allowed user headers to take precedence and used a header along the lines of X-elastic-product-origin: kibana, to simply suppress the warning in Elasticsearch whenever the header is present, with any value. This way, the header would be usable for any other products which need similar functionality, and continue to work even if a different value is specified by the user and passed through Kibana to Elasticsearch.

@kobelb
Copy link
Contributor Author

kobelb commented Oct 2, 2020

I like that idea @gwbrown. I'll get this PR changed to implement that approach. Even if we just use a X-elastic-product-origin header, the likelihood that we'll have a collision seems negligible.

@kobelb kobelb marked this pull request as ready for review October 5, 2020 21:06
@kobelb kobelb requested a review from a team as a code owner October 5, 2020 21:06
@kobelb kobelb added v8.0.0 v7.10.0 release_note:skip Skip the PR/issue when compiling release notes labels Oct 5, 2020

export const KIBANA_HEADERS = {
'x-elastic-product-origin': 'kibana',
};
Copy link
Contributor

Choose a reason for hiding this comment

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

nits:

  • Can we wrap this with deepFreeze?
  • Would you mind renaming this to DEFAULT_HEADERS? Anything named "kibana" means everything and nothing anymore 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure :) I hated the name of this variable, but since it was meant to designate the request as "coming from Kibana", it seemed appropriate at the point in time.

@@ -108,6 +109,7 @@ export class ClusterClient implements ICustomClusterClient {
}

return {
...KIBANA_HEADERS,
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should also automatically forward this header from the incoming request. For instance, if elastic-agent makes a request to Kibana, what should the x-elastic-product-origin header be when it arrives at ES?

May be unnecessary for this initial change, but it could be useful for telemetry purposes in ES in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the moment, this is only being consumed to determine when to suppress deprecation logs when the Kibana system-indices are queried by Kibana. If the usage of this grows, I could see us making this change, but it doesn't seem necessary at this point-in-time.

import { deepFreeze } from '@kbn/std';

export const DEFAULT_HEADERS = deepFreeze({
'x-elastic-product-origin': 'kibana',
Copy link
Contributor

Choose a reason for hiding this comment

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

Think someone reading this code would benefit from a comment that explains the intention? E.g.

Elasticsearch uses this to identify when a request is coming from Kibana,
to allow Kibana deprecated access to system indices logging a warning.
We'll migrate to use new system index APIs in the future, which will allow
us to remove this header.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, adding that!

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

distributable file count

id before after diff
default 47110 47111 +1
oss 28597 28598 +1

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@kobelb kobelb merged commit 1ad68fd into elastic:master Oct 6, 2020
@kobelb kobelb deleted the kibana-request-header branch October 6, 2020 13:40
kobelb added a commit that referenced this pull request Oct 6, 2020
* Adding X-Kibana header for the legacy client requests

* Specifying `X-Kibana: true` header for all requests coming from Kibana

* Dev Tools now override the X-Kibana header to false

* DevTools doesn't need to do anything, it's not using the ES client

* Switching from `X-Kibana: true` to `User-Agent: Kibana`

* Switching from `X-Kibana: true` to `User-Agent: Kibana`

* Adding a constant

* Starting to add unit tests...

We currently allow end-users to set whatever headers they'd like to be
forwarded to Elasticsearch with `elasticsearch.customHeaders` and
`elasticsearch.requestHeadersWhitelist`. This is potentially problematic
with us always specifying `User-Agent: kibana` as it could interfere
with what our end-users have done...

* Switching from user-agent to X-elastic-product-origin

* Adding some tests

* Adding and updating the legacy client unit tests
T#

* /s/KIBANA_HEADERS/DEFAULT_HEADERS and a deepFreeze

* Adding comment for why `x-elastic-product-origin` exists
gmmorris added a commit that referenced this pull request Oct 8, 2020
…into feature/task_manager_429

* 'feature/task_manager_429' of github.com:elastic/kibana: (158 commits)
  Add license check to direct package upload handler. (#79653)
  [Ingest Manager] Rename API /api/ingest_manager => /api/fleet (#79193)
  [Security Solution][Resolver] Simplify CopyableField styling and add comments (#79594)
  Fine-tunes ML related text on Metrics UI (#79425)
  [ML] DF Analytics creation wizard: ensure job creation possible when model memory lower than estimate (#79229)
  Add new "Add Data" tutorials (#77237)
  Update APM telemetry docs (#79583)
  Revert "Add support for runtime field types to mappings editor. (#77420)" (#79611)
  Kibana request headers (#79218)
  ensure missing indexPattern error is bubbled up to error callout (#79378)
  Missing space fix (#79585)
  remove duplicate tab states (#79501)
  [data.ui] Lazy load UI components in data plugin. (#78889)
  Add generic type params to search dependency. (#79608)
  [Ingest Manager] Internal action for policy reassign (#78493)
  [ILM] Add index_codec to forcemerge action in hot and warm phases (#78175)
  [Ingest Manager] Update open API spec and add condition to agent upgrade endpoint (#79579)
  [ML] Hide Data Grid column options when histogram charts are enabled. (#79459)
  [Telemetry] Synchronous `setup` and `start` methods (#79457)
  [Observability] Persist time range across apps (#79258)
  ...
@kobelb kobelb mentioned this pull request Jan 29, 2021
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants