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

Update logstash pipeline management to use system index APIs #80405

Merged
merged 10 commits into from
Nov 11, 2020

Conversation

jaymode
Copy link
Member

@jaymode jaymode commented Oct 13, 2020

Summary

This change updates the logstash pipeline management plugin to use
pipeline management APIs in Elasticsearch rather than directly
accessing the .logstash index. In Elasticsearch 8.0, direct access to
system indices will no longer be allowed when using standard APIs.
Given this change, a new set of APIs has been created specifically for
the management of Logstash pipelines and this change makes use of the
APIs.

Relates elastic/elasticsearch#53350

Checklist

For maintainers

This change updates the logstash pipeline management plugin to use
pipeline management APIs in Elasticsearch rather than directly
accessing the .logstash index. In Elasticsearch 8.0, direct access to
system indices will no longer be allowed when using standard APIs.
Given this change, a new set of APIs has been created specifically for
the management of Logstash pipelines and this change makes use of the
APIs.

Relates elastic/elasticsearch#53350
@jaymode jaymode added the review label Oct 13, 2020
@kaisecheng
Copy link
Contributor

I have a concern about backward compatibility when this Kibana connects to Elasticsearch version < 7.9. /_logstash doesn't exist in the old version. In Logstash, we maintain both legacy API and the new system indices API for < 7.9 and 7.9+

For the role management, I am not quite sure but I think manage_logstash_pipeline need to add around here https://github.com/elastic/kibana/blob/master/x-pack/plugins/logstash/server/plugin.ts#L47

I am also new to Kibana. It would be great if a Kibana member can have a look.

@jaymode
Copy link
Member Author

jaymode commented Oct 14, 2020

I have a concern about backward compatibility when this Kibana connects to Elasticsearch version < 7.9. /_logstash doesn't exist in the old version

From the docs:

Running different major version releases of Kibana and Elasticsearch (e.g.
Kibana 5.x and Elasticsearch 2.x) is not supported, nor is running a minor
version of Kibana that is newer than the version of Elasticsearch (e.g. Kibana
5.1 and Elasticsearch 5.0).

Given this moving Kibana to use these APIs is ok. Elasticsearch will have the APIs in 7.10+ and Kibana should always be the same version as Elasticsearch or older within the same minor, which should only be temporary for upgrades.

For the role management, I am not quite sure but I think manage_logstash_pipeline need to add around here

Good catch.

@kaisecheng
Copy link
Contributor

@jaymode Will you continue your work on role management? Or you would like me to finish the rest of this PR?

@jaymode
Copy link
Member Author

jaymode commented Oct 14, 2020

@kaisecheng if you don't mind finishing the PR that would be great! I've added you as a collaborator to my repository.

@kaisecheng
Copy link
Contributor

@elasticmachine merge upstream

@roaksoax
Copy link

@elasticmachine merge upstream

@roaksoax
Copy link

@elasticmachine merge upstream

kaisecheng added a commit to kaisecheng/kibana that referenced this pull request Nov 2, 2020
@roaksoax
Copy link

roaksoax commented Nov 4, 2020

@elasticmachine merge upstream

index: INDEX_NAMES.PIPELINES,
id: request.params.id,
refresh: 'wait_for',
await client.callAsCurrentUser('transport.request', {
Copy link
Contributor

@mshustov mshustov Nov 10, 2020

Choose a reason for hiding this comment

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

  1. Logstash plugin needs to be migrated to the new Elasticsearch client https://github.com/elastic/elasticsearch-js as we are going to remove the old one in v8.0. Changelog: https://github.com/elastic/kibana/blob/master/src/core/MIGRATION_EXAMPLES.md#elasticsearch-client

  2. transport.request doesn't provide type safety.

  3. The current implementation doesn't prefix endpoints with _kibana as outlined in the migration path for 7.x Kibana's system indices #81536

In #82716, we are going to provide a separate Elasticsearch client to address 2 & 3 points.
Logstash plugin will:

  • migrate to provided SystemIndices Elasticsearch client
  • adopt all usage places to make sure compatibility with the new Elasticsearch client version
  • migrate to /_logstash/pipeline/ endpoints (as done in this PR)

How soon do you want to migrate to System Indices? Can you wait for #82716?
@jaymode @kaisecheng

Copy link
Member Author

Choose a reason for hiding this comment

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

Regarding 3, these APIs will not be prefixed with _kibana as they exist in their own plugin within Elasticsearch and not within the Kibana system index plugin.

Ideally, I'd like to see us move the Logstash UI to use the system index APIs sooner rather than later to get more of the system index work into users' hands.

Copy link
Contributor

Choose a reason for hiding this comment

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

Regarding 3, these APIs will not be prefixed with _kibana as they exist in their own plugin within Elasticsearch and not within the Kibana system index plugin.

Makes sense to merge it then. You just need to address 1st point then. Can be done in the follow-up.

Hopefully we can fix the 2nd problem as well with #80405 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

I will address 1st point as a follow-up issue

Copy link

@roaksoax roaksoax Nov 17, 2020

Choose a reason for hiding this comment

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

@restrry is there a meta issue/transition plan to support the new elasticsearch-js client library (not just for logstash)?

Copy link
Contributor

Choose a reason for hiding this comment

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

id: request.params.id,
refresh: 'wait_for',
await client.callAsCurrentUser('transport.request', {
path: '/_logstash/pipeline/' + encodeURIComponent(request.params.id),
Copy link
Contributor

Choose a reason for hiding this comment

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

@delvedor I can't find /_logstash support in the new Elasticsearch client. Why it so?
https://github.com/elastic/elasticsearch-js/blob/master/api/kibana.d.ts

Copy link
Member

@delvedor delvedor Nov 10, 2020

Choose a reason for hiding this comment

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

I'm not aware of that endpoint, and I wasn't able to find it in the rest-api-spec (and here). Are you sure it's a public API?

Copy link
Member Author

Choose a reason for hiding this comment

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

The intent for these Logstash APIs is that they are for communication between the stack components and Elasticsearch so we intentionally did not publish a rest api spec for these. If this is necessary, we can probably add specs for these APIs.

Copy link
Contributor

Choose a reason for hiding this comment

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

communication between the stack components and Elasticsearch so we intentionally did not publish a rest api spec for these.

Can we add a separate REST API spec for such cases? We need to make sure that all the changes in REST API are reflected in the Kibana code as well - we rely on TypeScript type definitions generated from REST API spec.

Copy link
Member

Choose a reason for hiding this comment

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

++ another vote on this. I'd be nice to leverage the safety net y'all created with the API spec, but for product to product APIs

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry for missing an update here, the ES team discussed this and came to the conclusion that we'd add a spec and docs for this API. This work hasn't been started yet.

Copy link
Member Author

Choose a reason for hiding this comment

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

The API spec files have been merged in elastic/elasticsearch#67788

@kaisecheng
Copy link
Contributor

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Distributable file count

id before after diff
default 42778 42776 -2

History

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

@kaisecheng kaisecheng merged commit b460414 into elastic:master Nov 11, 2020
gmmorris added a commit to gmmorris/kibana that referenced this pull request Nov 12, 2020
…ts-public

* upstream/master: (57 commits)
  Remove unused asciidoc file (elastic#83228)
  [Lens] Remove background from lens embeddable (elastic#83061)
  [Discover] Unskip flaky tests based on discover fixture index pattern (elastic#82991)
  Removing unnecessary trailing slash in CODEOWNERS
  Trying to fix CODEOWNERS again, where was a non-existent team prior (elastic#83236)
  Trying to fix CODEOWERS, missing a starting slash (elastic#83233)
  skip flaky suite (elastic#83231)
  Add enzyme rerender test helper (elastic#83208)
  Move Elasticsearch type definitions out of APM (elastic#83081)
  [ts/checkTsProjects] produce a more useful error message (elastic#83209)
  [kbnClient] retry updating config if necessary (elastic#83205)
  I accidentally removed this line in a recent PR (elastic#83201)
  Don't make the caller do work the function can do (elastic#83180)
  [App Search] Update EngineRouter & EngineNav to use EngineLogic (elastic#83138)
  [Workplace Search] Add routes for Sources (elastic#83125)
  Update logstash pipeline management to use system index APIs (elastic#80405)
  [ML] Replace EuiBasicTable with EuiInMemoryTable (elastic#83057)
  [Metrics UI] Add basic interaction and shell for node details overlay (elastic#82013)
  [App Search] Added the log retention confirmation modal to the Settings page (elastic#83009)
  [docs] Fix create map title in import geospatial page (elastic#83172)
  ...
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 80405 or prevent reminders by adding the backport:skip label.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Nov 13, 2020
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 80405 or prevent reminders by adding the backport:skip label.

kaisecheng pushed a commit to kaisecheng/kibana that referenced this pull request Nov 17, 2020
…#80405)

This change updates the logstash pipeline management plugin to use
pipeline management APIs in Elasticsearch rather than directly
accessing the .logstash index. In Elasticsearch 8.0, direct access to
system indices will no longer be allowed when using standard APIs.
Given this change, a new set of APIs has been created specifically for
the management of Logstash pipelines and this change makes use of the
APIs.

Co-authored-by: Kaise Cheng <kaise.cheng@elastic.co>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

ES:elastic#53350
LS:elastic#12291
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Nov 17, 2020
kaisecheng added a commit that referenced this pull request Nov 17, 2020
…#83526)

This change updates the logstash pipeline management plugin to use
pipeline management APIs in Elasticsearch rather than directly
accessing the .logstash index. In Elasticsearch 8.0, direct access to
system indices will no longer be allowed when using standard APIs.
Given this change, a new set of APIs has been created specifically for
the management of Logstash pipelines and this change makes use of the
APIs.

Co-authored-by: Kaise Cheng <kaise.cheng@elastic.co>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

ES:#53350
LS:#12291

Co-authored-by: Jay Modi <jaymode@users.noreply.github.com>
@jaymode jaymode deleted the logstash_system_index_apis branch January 28, 2021 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants