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 documentation for required privileges and unsupported processors. #72

Merged
merged 3 commits into from
Jul 28, 2023

Conversation

mashhurs
Copy link
Collaborator

@mashhurs mashhurs commented May 11, 2023

Description

Documentation doesn't mention about privileges the plugin requires but when Logstash runs with insufficient privileges, it raises an error. End users should have information about what privileges are required when running the plugin.

@mashhurs mashhurs added the documentation Improvements or additions to documentation label May 11, 2023
@mashhurs mashhurs self-assigned this May 11, 2023
@mashhurs mashhurs requested a review from yaauie May 11, 2023 21:03
Copy link
Member

@yaauie yaauie left a comment

Choose a reason for hiding this comment

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

I have made some suggestions for the privileges docs, and think that we may be better off splitting this into two tasks.

[id="plugins-{type}s-{plugin}-unsupported_ingest_processors"]
==== Unsupported Ingest Processors

Currently, following ingest processors are not supported due to limited {ls} ability to access {es} internal assets/processes.
Copy link
Member

Choose a reason for hiding this comment

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

As I have expressed with @roaksoax on tickets and in private chat, I strongly believe that we will be better off highlighting which integrations ARE supported by this plugin than attempting to provide an exhaustive list of which processors aren't included. A negative list will frequently become obsolete.

Even if we only look at Elasticsearch source for implementations of org.elasticsearch.ingest.IngestPlugin and org.elasticsearch.ingest.Processor, we find that we are already missing:

It also doesn't include user-installable first-party or third-party IngestPlugin implementations, none of which are included in this plugin.

Maintaining an exhaustive list of what processors we do NOT support is simply not practical.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The most dangerous part is, we need to guess which integration uses which ingest pipeline and this may change time-by-time.
I went through the list of integrations and docs do not mention about ingest pipelines and processors.

docs/index.asciidoc Outdated Show resolved Hide resolved
docs/index.asciidoc Show resolved Hide resolved
@mashhurs
Copy link
Collaborator Author

I have made some suggestions for the privileges docs, and think that we may be better off splitting this into two tasks.

Do you mean to split into PRs privilege and unsupported processors separately? If not, can you please clarify?

@yaauie
Copy link
Member

yaauie commented May 13, 2023

split into PRs privilege and unsupported processors separately

Yes. I think we solve one easily and would prefer to merge the privilege docs while we continue to find an approach that makes it easy for users to form accurate expectations about the processors and integrations this plugin supports.

docs/index.asciidoc Outdated Show resolved Hide resolved
docs/index.asciidoc Outdated Show resolved Hide resolved
docs/index.asciidoc Outdated Show resolved Hide resolved
Remove unsupported contents and update privileges content.

Co-authored-by: Ry Biesemeyer <[email protected]>

NOTE: When either connecting to an {es} cluster that has security features disabled or when not providing credentials, this plugin can not pre-verify that the anonymous user has the required privileges to use the necessary APIs.
In this case the plugin will start in an unsafe mode in which insufficient API permissions will be a runtime error that prevents events from being processed by their ingest pipeline.
Enabling security in {es} and using user authentication is recommended.
Copy link
Collaborator Author

@mashhurs mashhurs May 15, 2023

Choose a reason for hiding this comment

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

Checked locally, view is okay.

Copy link
Contributor

Choose a reason for hiding this comment

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

@karenzone does this make sense to you?

@mashhurs mashhurs requested a review from yaauie May 15, 2023 15:09
Copy link
Contributor

@karenzone karenzone left a comment

Choose a reason for hiding this comment

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

I left some comments inline for your consideration. We can iterate together.

Related: I opened #74 to track and resolve some doc issues that pre-date this PR.

docs/index.asciidoc Outdated Show resolved Hide resolved
Co-authored-by: Karen Metts <[email protected]>
Copy link
Contributor

@karenzone karenzone left a comment

Choose a reason for hiding this comment

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

@mashhurs Your work in this PR looks good, and so I will approve it.

However, there are some pre-existing problems, and this doc as it is now will not pass docs-CI when it is hooked up. I opened #74 to track and resolve some doc issues that pre-date this PR.

Please go ahead and merge this PR when you get all of the approvals you need so that this content will be part of the baseline for the next round of cleanup and edits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Logstash Ingest Pipelines - Documentation doesn't state the required Elasticsearch Permissions
4 participants