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

Add isSystemRequest support to Kibana Platform #53734

Merged
merged 20 commits into from
Jan 24, 2020

Conversation

joshdover
Copy link
Contributor

@joshdover joshdover commented Dec 20, 2019

Summary

Fixes #43970

Adds two new options:

  • On the client, core.http.fetch now accepts an asSystemRequest option
  • On the server, KibanaRequest now has an isSystemRequest boolean property

This change also modifies how request interceptors work so that they now can only modify the HttpFetchOptions rather than the raw Request object. This enables Core to encapsulate its implementation details on how it converts HttpFetchOptions to a Request. It also reduces the API surface a bit by not needing to provide access to all of Request's methods.

This change was helpful to support exposing to request interceptors whether or not a request was called with asSystemRequest. This particular change could be split out as a separate PR if we think that is useful.

In renaming this concept from "system api" to "system request", I also renamed the header that is used from kbn-system-api to kbn-system-request to better match the code. The backend will continue to support kbn-system-api for legacy plugin support and to support the use case where the client is running an older version than the server, where requests may still contain the kbn-system-api header.

Opened #55203 to track removing support for the old header.

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@joshdover joshdover added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform release_note:skip Skip the PR/issue when compiling release notes v7.6.0 labels Dec 20, 2019
@joshdover joshdover requested a review from a team as a code owner December 20, 2019 21:51
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform (Team:Platform)

@kibanamachine
Copy link
Contributor

💔 Build Failed

History

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

@elastic elastic deleted a comment from elasticmachine Dec 22, 2019
src/core/public/http/fetch.ts Show resolved Hide resolved
src/core/public/http/types.ts Outdated Show resolved Hide resolved
src/core/public/http/fetch.test.ts Outdated Show resolved Hide resolved
src/core/public/http/fetch.test.ts Outdated Show resolved Hide resolved
src/core/utils/constants.ts Outdated Show resolved Hide resolved
src/core/server/http/router/request.ts Outdated Show resolved Hide resolved
@kibanamachine
Copy link
Contributor

💔 Build Failed

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

@kibanamachine
Copy link
Contributor

💔 Build Failed

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

@kibanamachine
Copy link
Contributor

💔 Build Failed

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

@joshdover joshdover requested a review from a team December 30, 2019 18:57
@joshdover joshdover requested a review from a team as a code owner December 30, 2019 18:57
@kibanamachine
Copy link
Contributor

💔 Build Failed

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

Copy link
Contributor

@streamich streamich left a comment

Choose a reason for hiding this comment

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

AppArch changes LGTM.

Copy link
Member

@azasypkin azasypkin left a comment

Choose a reason for hiding this comment

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

Security related changes LGTM (I'll migrate Security server side part as soon as this PR is merged).

src/core/public/http/types.ts Show resolved Hide resolved
src/core/public/http/fetch.test.ts Outdated Show resolved Hide resolved
src/core/public/http/fetch.test.ts Outdated Show resolved Hide resolved
src/core/public/http/types.ts Outdated Show resolved Hide resolved
src/core/public/http/types.ts Outdated Show resolved Hide resolved
src/core/public/http/types.ts Outdated Show resolved Hide resolved
src/core/public/http/fetch.ts Outdated Show resolved Hide resolved
src/core/server/http/router/request.test.ts Outdated Show resolved Hide resolved
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:New Platform release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate Kibana plugin "System API" to New Platform
7 participants