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 SloGlobalDiagnosis check to SLO List and SLO Create pages #157488

Conversation

CoenWarmer
Copy link
Contributor

@CoenWarmer CoenWarmer commented May 12, 2023

Resolves #157317

📝 Summary

This adds the usage of the SLO Global Diagnosis endpoint in the SLO app on the SLO Welcome and SLO Create pages.

If the endpoint returns an error, the user will receive a toast with more detailed information as to what is the cause of the issue.

On the Welcome page the Create SLO button will be disabled. When the user visits the Create page via URL she will be navigated back to the Welcome page.

Screenshot 2023-05-12 at 14 41 35

✅ Checklist

  • If a user has required cluster permissions, the user should be able to click the Create SLO button on the welcome page
  • If a user has required cluster permissions, the user should be able to navigate to the Create SLO page
  • If a user does not have required cluster permissions, the user should not be able to click the Create SLO button on the welcome page
  • If a user does not have required cluster permissions, the user should be navigated to the Welcome page when the user attempts to visit the Create SLO page
  • If a user does not have required cluster permissions, the user should receive a helpful error message.

@CoenWarmer CoenWarmer requested a review from a team as a code owner May 12, 2023 12:51
@apmmachine
Copy link
Contributor

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@CoenWarmer CoenWarmer added release_note:skip Skip the PR/issue when compiling release notes backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) v8.8.0 labels May 12, 2023
@CoenWarmer CoenWarmer force-pushed the bug/157317-nicer-error-handling-with-insufficient-cluster-privileges branch from 118c5d9 to fd81f81 Compare May 12, 2023 12:57
userPrivileges,
sloResources,
};
try {
Copy link
Contributor

Choose a reason for hiding this comment

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

this try catch is not needed if we are just throwing the error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is needed.

const sloResources = await getSloResourcesDiagnosis(esClient); throws an error when ES has insufficient permissions, so if we don't throw here this function returns a normal object:

return {
    licenseAndFeatures: { something },
    userPrivileges: { something },
  	sloResources: { message: 'Some error message', statusCode: 403, ...etc }
};

if (error.cause.statusCode === 403) {
throw forbidden('Insufficient Elasticsearch cluster permissions to access feature.');
}
throw failedDependency(error);
Copy link
Contributor

Choose a reason for hiding this comment

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

what does this thing do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's one of HAPI Boom's methods to auto-format the response:

Boom.failedDependency([message], [data])
Returns a 424 Failed Dependency error where:

message - optional message.
data - optional additional error data.
Boom.failedDependency('an external resource failed');
Generates the following response payload:

{
    "statusCode": 424,
    "error": "Failed Dependency",
    "message": "an external resource failed"
}

https://hapi.dev/module/boom/api/?v=10.0.1#boomfaileddependencymessage-data

Comment on lines +67 to +72
onError: (error: Error) => {
toasts.addError(error, {
title: i18n.translate('xpack.observability.slo.globalDiagnosis.errorNotification', {
defaultMessage: 'You do not have the right permissions to use this feature.',
}),
});
Copy link
Contributor

Choose a reason for hiding this comment

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

So basically we rely on an error response coming from the API to consider the permission as insufficient?
I guess it works for now, but for 8.9 I would probably move this logic into the different SLO endpoints (create, update, etc..) and handle the 403 from there, returning a special error response, so we guarantee a universal experience on both UI and API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, this can be improved.

The bug report mentioned disabling the "Create SLO" button on the Welcome page when no permissions are found, so we at least need this call to do that, not only on mutating endpoints.

Copy link
Contributor

@kdelemme kdelemme left a comment

Choose a reason for hiding this comment

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

LGTM

@CoenWarmer CoenWarmer enabled auto-merge (squash) May 12, 2023 15:32
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
observability 408 410 +2

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
observability 972.1KB 972.9KB +809.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
securitySolution 400 404 +4
total +6

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
securitySolution 480 484 +4
total +6

History

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

@CoenWarmer CoenWarmer merged commit e753256 into elastic:main May 12, 2023
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request May 12, 2023
…c#157488)

Co-authored-by: Kevin Delemme <[email protected]>
Co-authored-by: kibanamachine <[email protected]>
(cherry picked from commit e753256)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.8

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request May 12, 2023
…157488) (#157514)

# Backport

This will backport the following commits from `main` to `8.8`:
- [Add SloGlobalDiagnosis check to SLO List and SLO Create pages
(#157488)](#157488)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Coen
Warmer","email":"[email protected]"},"sourceCommit":{"committedDate":"2023-05-12T15:45:16Z","message":"Add
SloGlobalDiagnosis check to SLO List and SLO Create pages
(#157488)\n\nCo-authored-by: Kevin Delemme
<[email protected]>\r\nCo-authored-by: kibanamachine
<[email protected]>","sha":"e75325653c88553454270bf412ddbed700fc022b","branchLabelMapping":{"^v8.9.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","backport:prev-minor","v8.8.0","v8.9.0"],"number":157488,"url":"https://github.com/elastic/kibana/pull/157488","mergeCommit":{"message":"Add
SloGlobalDiagnosis check to SLO List and SLO Create pages
(#157488)\n\nCo-authored-by: Kevin Delemme
<[email protected]>\r\nCo-authored-by: kibanamachine
<[email protected]>","sha":"e75325653c88553454270bf412ddbed700fc022b"}},"sourceBranch":"main","suggestedTargetBranches":["8.8"],"targetPullRequestStates":[{"branch":"8.8","label":"v8.8.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.9.0","labelRegex":"^v8.9.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/157488","number":157488,"mergeCommit":{"message":"Add
SloGlobalDiagnosis check to SLO List and SLO Create pages
(#157488)\n\nCo-authored-by: Kevin Delemme
<[email protected]>\r\nCo-authored-by: kibanamachine
<[email protected]>","sha":"e75325653c88553454270bf412ddbed700fc022b"}}]}]
BACKPORT-->

Co-authored-by: Coen Warmer <[email protected]>
jasonrhodes pushed a commit that referenced this pull request May 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) release_note:skip Skip the PR/issue when compiling release notes v8.8.0 v8.9.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Internal server error when creating SLO without sufficient cluster permissions
5 participants