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

[Monitoring] Handle setup mode if security is disabled #53306

Merged

Conversation

chrisronline
Copy link
Contributor

@chrisronline chrisronline commented Dec 17, 2019

Resolves #53129

To test, start up a fresh ES/Kibana with security disabled. Try clicking the Set up monitoring with Metricbeat

If we can get some resolution elastic/elasticsearch#50288, we might be able to simplify this code by simply looking at the exception message. As it stands, the exception message doesn't explicitly tell us security is not enabled.

@elasticmachine
Copy link
Contributor

Pinging @elastic/stack-monitoring (Team:Monitoring)

Copy link
Contributor

@igoristic igoristic left a comment

Choose a reason for hiding this comment

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

Great job 👍 Works as expected!

@chrisronline
Copy link
Contributor Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

History

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

@chrisronline
Copy link
Contributor Author

I'm going to hold off on this until elastic/elasticsearch#50288 is available in a testable snapshot

return get(response, 'has_all_requested', false);
} catch (err) {
if (
err.message === 'no handler found for uri [/_security/user/_has_privileges] and method [POST]'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kobelb I know we discussed this awhile back, but I'm revisiting this PR and wondering if you think this check is sufficient enough. We can either do this, or do a pre-flight check for the security feature on the cluster.

Copy link
Contributor

Choose a reason for hiding this comment

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

We've had to do similar checks elsewhere. It's not perfect, but it generally works. I would however encourage you to write an integration test against Elasticsearch to ensure that if they change their error messages, that the test begins to fail. These error messages are prone to change.

@chrisronline
Copy link
Contributor Author

@elasticmachine merge upstream

@elasticmachine elasticmachine requested a review from a team February 27, 2020 18:27
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@chrisronline chrisronline merged commit f62bda0 into elastic:master Feb 27, 2020
@chrisronline chrisronline deleted the monitoring/setup_mode_no_security branch February 27, 2020 20:34
chrisronline added a commit to chrisronline/kibana that referenced this pull request Feb 27, 2020
* Handle setup mode if security is disabled

* Rename so the test actually runs, and fix/add tests

* Use es.js api instead of transport.request

* Revert "Use es.js api instead of transport.request"

This reverts commit ae0e48f.

* Explicitly handle security not enabled

Co-authored-by: Elastic Machine <[email protected]>
chrisronline added a commit that referenced this pull request Feb 28, 2020
* Handle setup mode if security is disabled

* Rename so the test actually runs, and fix/add tests

* Use es.js api instead of transport.request

* Revert "Use es.js api instead of transport.request"

This reverts commit ae0e48f.

* Explicitly handle security not enabled

Co-authored-by: Elastic Machine <[email protected]>

Co-authored-by: Elastic Machine <[email protected]>
@chrisronline
Copy link
Contributor Author

Backport:

7.x: 535114d

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.

Out of the box Stack Monitoring experience feels broken on 7.5.0
6 participants