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

Introduce system index APIs for Kibana #52385

Merged
merged 26 commits into from
Mar 2, 2020

Conversation

jaymode
Copy link
Member

@jaymode jaymode commented Feb 14, 2020

This commit introduces a module for Kibana that exposes REST APIs that
will be used by Kibana for access to its system indices. These APIs are wrapped
versions of the existing REST endpoints. A new setting is also introduced since
the Kibana system indices' names are allowed to be changed by a user in case
multiple instances of Kibana use the same instance of Elasticsearch.

Additionally, the ThreadContext has been extended to indicate that the use of
system indices may be allowed in a request. This will be built upon in the future
for the protection of system indices.

This commit introduces a module for Kibana that exposes REST APIs that
will be used by Kibana for access to its system indices.
@jaymode jaymode added the :Core/Infra/Core Core issues without another label label Feb 14, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (:Core/Infra/Core)

@jaymode jaymode marked this pull request as ready for review February 20, 2020 19:02
@jaymode jaymode changed the title [WIP] Introduce system index APIs for Kibana Introduce system index APIs for Kibana Feb 20, 2020
@jaymode jaymode requested a review from gwbrown February 20, 2020 19:02

public class KibanaPlugin extends Plugin implements SystemIndexPlugin {

public static final Setting<List<String>> KIBANA_INDEX_NAMES_SETTING = Setting.listSetting("kibana.system_indices",
Copy link
Member Author

@jaymode jaymode Feb 26, 2020

Choose a reason for hiding this comment

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

@tylersmalley I was looking at elastic/kibana#49764 and I wanted to make sure I understood the request there for the paths where you have _kibana/{kibana.name}/ and kibana.name would be understood by elasticsearch based on settings.

Right now in this plugin, the setting I have would look like this in the elasticsearch.yml file:

kibana.system_indices: [ ".kibana", ".kibana_task_manager", ".reporting" ]

I think the Kibana issue is actually looking for something more like:

kibana:
  instances:
    my_kibana:
       kibana_index: ".my-kibana"
       task_manager_index: ".my-tm"
       reporting_index: ".my-reports"

Is this correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

The latter is correct. Ideally, I would like to define a list of names which we could derive each index off of, but that won't work today as users can configure them individually.

In that example, would the endpoint be _kibana/my_kibana/*?

It would probably also make sense to include a default so Kibana instance configuration is only necessary if they intend on having multiple instances.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes that would be the endpoint path. For the default, I think we will still want an instance name so maybe just ‘kibana’ for that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. Though, could we use default for the name of the default instance? I feel this should be clear as to what the instance is and avoid _kibana/kibana/*

Copy link
Contributor

Choose a reason for hiding this comment

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

What is the risk for not whitelisting kibana instances and indices? From a user's perspective it's error prone to have to adjust both the kibana and elasticsearch config files and keep them in sync. The status quo is that Kibana creates any instances (it's currently just an index prefix) and indices it wants to.

Copy link
Member Author

Choose a reason for hiding this comment

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

Every system index plugin needs to declare the system indices it is used for as a way to identify indices that are system indices. If you are ok with just creating all of your indices under a single prefix, then I think that would be fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

A question was also brought up as to if we would consider this a breaking change if it was introduced before 8.0. Currently, to migrate to the system indices the user would be required to update ES and Kibana configuration.

The prefixing is interesting idea worth exploring, but would require coming up with a plan to migrate to these index names, correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point on it being breaking. One thing we can do is allow the APIs under /_kibana to be unrestricted for the indices that could be used in 7.x.

Copy link
Member Author

Choose a reason for hiding this comment

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

For the time being, I have gone ahead and removed the limiting of indices accessible by the APIs until we figure out what we should do here regarding these indices and their names.

Copy link
Contributor

@gwbrown gwbrown left a comment

Choose a reason for hiding this comment

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

Left a few comments but overall looks really good.

}
} else {
for (String index : indices) {
// TODO this will not handle date math, is that OK? Do we need wildcard support?
Copy link
Contributor

Choose a reason for hiding this comment

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

We might need wildcard support eventually, although I don't think we do for Kibana (@tylersmalley do you know?). Date math support is more questionable. It might be worth trying to detect it and throwing an exception, similar to what we do here.

@jaymode jaymode requested a review from gwbrown February 28, 2020 15:03
Copy link
Contributor

@gwbrown gwbrown left a comment

Choose a reason for hiding this comment

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

LGTM, nice work on this.

@jaymode jaymode merged commit 4c0e8f1 into elastic:master Mar 2, 2020
jaymode added a commit to jaymode/elasticsearch that referenced this pull request Mar 2, 2020
This commit introduces a module for Kibana that exposes REST APIs that
will be used by Kibana for access to its system indices. These APIs are wrapped
versions of the existing REST endpoints. A new setting is also introduced since
the Kibana system indices' names are allowed to be changed by a user in case
multiple instances of Kibana use the same instance of Elasticsearch.

Additionally, the ThreadContext has been extended to indicate that the use of
system indices may be allowed in a request. This will be built upon in the future
for the protection of system indices.

Backport of elastic#52385
jaymode added a commit to jaymode/elasticsearch that referenced this pull request Mar 3, 2020
The system index module for Kibana has been merged to master in elastic#52385
and is being backported to 7.x in elastic#53035, which requires changes to the
serialization version. In order to avoid build failures, this commit
disables bwc tests. A subsequent commit will reenable these tests.
jaymode added a commit that referenced this pull request Mar 3, 2020
The system index module for Kibana has been merged to master in #52385
and is being backported to 7.x in #53035, which requires changes to the
serialization version. In order to avoid build failures, this commit
disables bwc tests. A subsequent commit will reenable these tests.
jaymode added a commit that referenced this pull request Mar 3, 2020
This commit introduces a module for Kibana that exposes REST APIs that
will be used by Kibana for access to its system indices. These APIs are wrapped
versions of the existing REST endpoints. A new setting is also introduced since
the Kibana system indices' names are allowed to be changed by a user in case
multiple instances of Kibana use the same instance of Elasticsearch.

Additionally, the ThreadContext has been extended to indicate that the use of
system indices may be allowed in a request. This will be built upon in the future
for the protection of system indices.

Backport of #52385
jaymode added a commit to jaymode/elasticsearch that referenced this pull request Mar 3, 2020
This commit updates the serialization for allowed system indices after
the backport of the work and also re-enables bwc compatibility tests.

Relates elastic#52385
Relates elastic#53035
Relates elastic#53062
jaymode added a commit that referenced this pull request Mar 4, 2020
This commit updates the serialization for allowed system indices after
the backport of the work and also re-enables bwc compatibility tests.

Relates #52385
Relates #53035
Relates #53062
rjernst added a commit to rjernst/elasticsearch that referenced this pull request Mar 20, 2020
This reverts commit 4c0e8f1.

It should be re-added once elastic#53909 is addressed.
rjernst added a commit that referenced this pull request Mar 23, 2020
This reverts commit 4c0e8f1.

It should be re-added once #53909 is addressed.
jaymode added a commit to jaymode/elasticsearch that referenced this pull request Apr 7, 2020
This change reintroduces the system index APIs for Kibana wihtout the
changes made for marking what system indices could be accessed using
these APIs. In essence, this is a partial revert of elastic#53912. The changes
for marking what system indices should be allowed access will be
handled in a separate change.

The APIs introduced here are wrapped versions of the existing REST
endpoints. A new setting is also introduced since the Kibana system
indices' names are allowed to be changed by a user in case multiple
instances of Kibana use the same instance of Elasticsearch.

Relates elastic#52385
jaymode added a commit that referenced this pull request Apr 8, 2020
This change reintroduces the system index APIs for Kibana without the
changes made for marking what system indices could be accessed using
these APIs. In essence, this is a partial revert of #53912. The changes
for marking what system indices should be allowed access will be
handled in a separate change.

The APIs introduced here are wrapped versions of the existing REST
endpoints. A new setting is also introduced since the Kibana system
indices' names are allowed to be changed by a user in case multiple
instances of Kibana use the same instance of Elasticsearch.

Relates #52385
jaymode added a commit to jaymode/elasticsearch that referenced this pull request Apr 8, 2020
This change reintroduces the system index APIs for Kibana without the
changes made for marking what system indices could be accessed using
these APIs. In essence, this is a partial revert of elastic#53912. The changes
for marking what system indices should be allowed access will be
handled in a separate change.

The APIs introduced here are wrapped versions of the existing REST
endpoints. A new setting is also introduced since the Kibana system
indices' names are allowed to be changed by a user in case multiple
instances of Kibana use the same instance of Elasticsearch.

Relates elastic#52385
jaymode added a commit that referenced this pull request Apr 8, 2020
This change reintroduces the system index APIs for Kibana without the
changes made for marking what system indices could be accessed using
these APIs. In essence, this is a partial revert of #53912. The changes
for marking what system indices should be allowed access will be
handled in a separate change.

The APIs introduced here are wrapped versions of the existing REST
endpoints. A new setting is also introduced since the Kibana system
indices' names are allowed to be changed by a user in case multiple
instances of Kibana use the same instance of Elasticsearch.

Relates #52385
Backport of #54858
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Core Core issues without another label >enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants