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 slo-privileges.asciidoc #4643

Closed
wants to merge 2 commits into from

Conversation

AkashPrakashShinde
Copy link

Description

Documentation sets edited in this PR

Check all that apply.

  • Stateful (docs/en/observability/*)
  • Serverless (docs/en/serverless/*)
  • Integrations Developer Guide (docs/en/integrations/*)
  • None of the above

Related issue

Closes #

Checklist

  • Product/Engineering Review
  • Writer Review

Follow-up tasks

Select one.

  • This PR does not need to be ported to another doc set because:
    • The concepts in this PR only apply to one doc set (serverless or stateful)
    • The PR contains edits to both doc sets (serverless and stateful)
  • This PR needs to be ported to another doc set:
    • Port to stateful docs: <link to PR or tracking issue>
    • Port to serverless docs: <link to PR or tracking issue>

@AkashPrakashShinde AkashPrakashShinde requested a review from a team as a code owner December 9, 2024 14:21
Copy link
Contributor

github-actions bot commented Dec 9, 2024

A documentation preview will be available soon.

Request a new doc build by commenting
  • Rebuild this PR: run docs-build
  • Rebuild this PR and all Elastic docs: run docs-build rebuild

run docs-build is much faster than run docs-build rebuild. A rebuild should only be needed in rare situations.

If your PR continues to fail for an unknown reason, the doc build pipeline may be broken. Elastic employees can check the pipeline status here.

Image with correct SLO permissions
@colleenmcginnis colleenmcginnis added backport-8.x Automated backport to the 8.x branch with mergify backport-8.17 Automated backport with mergify backport-main Automated backport with mergify labels Dec 9, 2024
@@ -35,11 +35,12 @@ To create a role:
Set the following privileges for the SLO Editor role:

. Under *Index privileges* in the *Elasticsearch* section, add `.slo-observability-*` to the *Indices* field and `read`, `view_index_metadata`, `write`, and `manage` to the *Privileges* field.
. Under *Cluster privileges* add `manage_transform` as SLO most likely to create the transform respect to your SLO.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what you're trying to say here. Is there an issue or something else that explains why this privilege is needed?

Copy link
Contributor

@eedugon eedugon Dec 10, 2024

Choose a reason for hiding this comment

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

The reason of needing the privilege is that SLOs create transforms under the hood, so I would suggest something like:

Under Cluster privileges add manage_transform as SLOs rely on Elasticsearch transforms.

Or just the need without any explanation (we are not offering explanations in the other privileges required):

Under Cluster privileges add manage_transform.

Or something like SLOs need transforms privileges to work.

We have recently added a new doc section called Understanding SLO internals with an introduction explaining the relation between SLOs and transforms. We could link to that document to provide background (https://www.elastic.co/guide/en/observability/current/slo-troubleshoot-slos.html#slo-understanding-slos).

Copy link
Contributor

Choose a reason for hiding this comment

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

Users should not need manage_transform privileges in 8.16. We removed this requirement in an earlier version. Are you receiving reports of SLOs not working without manage_transforms? IF so, what version?

Copy link
Contributor

Choose a reason for hiding this comment

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

The text in this image is small and pretty difficult to read and there's a lot of whitespace. Can you reduce the width of your browser window before taking a screenshot? You can refer to the existing image for guidance on an appropriate width.

. Click *Add index privilege*.
. In the *Indices* field, add the indices for which you plan to create SLOs. Then, add `read` and `view_index_metadata` to the *Privileges* field. The following example shows `logs-*`, but you can specify any indices.
+
[role="screenshot"]
image::images/slo-es-priv-editor.png[Cluster and index privileges for SLO Editor role]
Copy link
Contributor

Choose a reason for hiding this comment

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

If this image is no longer needed, you should delete it. Alternatively you could rename your new image slo-es-priv-editor.png so it replaces the existing image.

@eedugon
Copy link
Contributor

eedugon commented Dec 11, 2024

After a conversation with @AkashPrakashShinde he has also realized that manage_pipeline privilege is also needed. Let's check this out with devs before moving forward.

@dominiqueclarke , if you remember we updated the privileges needed for SLOs a few months ago under this issue: #4229

It looks we might have missed cluster privileges manage_transform and manage_pipeline for the SLOs to work. This has been detected by @AkashPrakashShinde while working in support.

Would you please take a look and confirm if the privileges are needed? I'm pretty sure the manage_transform cluster privilege is needed, but @AkashPrakashShinde mentions something about manage_pipeline being optional and probably needed too (which makes sense considering SLOs create transforms and ingest pipelines under the hood).

As soon as you confirm we will fix the docs through this PR.

@eedugon
Copy link
Contributor

eedugon commented Dec 11, 2024

@dominiqueclarke : @AkashPrakashShinde also found another issue in the docs.
It looks for index privileges we are suggesting in the docs .slo-observability-* while SLOs logic is checking for .slo-observability.* (notice the dash vs dot difference).

Without fixing the privileges the user gets this type of error when accessing SLOs view:
Screenshot 2024-12-11 at 09 22 13

In summary, we need to clarify if we have to fix these items in the doc:

  • Change .slo-observability-* by .slo-observability.*.
  • Add manage_transform cluster privilege as needed.
  • Add manage_pipeline cluster privilege as needed.

If you prefer we can create an issue instead of discussing the problem in this PR.

Oh... now I see in #4229 it's mentioned:

In particular, there is no longer a requirement for SLO editor users to have cluster privileges manage_transform and manage_ingest_pipelines.

So we need to clarify if that statement was accurate, as apparently if a user without the manage_transform privilege creates a SLO using the API it ends up like:

Screenshot 2024-12-11 at 09 25 32

@eedugon
Copy link
Contributor

eedugon commented Jan 2, 2025

I'm closing this PR and created the issue #4720 to determine if we really need a docs change or if this should be treated as a product issue.

@eedugon eedugon closed this Jan 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-8.x Automated backport to the 8.x branch with mergify backport-8.17 Automated backport with mergify backport-main Automated backport with mergify blocked needs-dev-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants