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

Use authorisation if present, regardless of environment #138817

Merged
merged 5 commits into from
Oct 5, 2022

Conversation

lucasfcosta
Copy link
Contributor

@lucasfcosta lucasfcosta commented Aug 15, 2022

Summary

This PR allows Kibana to use the Authorization keys in its configuration file regardless of the environment in which its running.

Before this change you wouldn't be able to use Basic Auth in Elastic Cloud because it would be ignored due to this extra check.

Now, if the username and password settings exist, we do use them. That should be fine because it shouldn't be possible to set those keys unless doing tests on a particular region which allows for those keys to be set. Furthermore, one does need to have the credentials anyway.

How to test this PR

Locally

  1. Add valid username and password fields to your Kibana configs (xpack.uptime.service.username and xpack.uptime.service.password) and remove TLS settings.
  2. Try to access the service and create monitors

Once it goes to cloud (POST FF)

  1. Ensure you've deployed a SNAPSHOT version to Kibana to a CFT region
  2. Edit Kibana configs, add username and password keys (xpack.uptime.service.usernameandxpack.uptime.service.password) and use the service's dev manifest as the value for xpack.uptime.service.manifestUrl`.
  3. Try to access the service and create monitors in dev. They should all work okay.
  4. Remove the username and password keys, but keep the manifest URL. Everything should still work (monitors can be created and display results).
  5. Remove the manifest URL too and connect directly to the prod. version of the service (without username and password). Everything should still work (monitors can be created and display results).

Checklist

Delete any items that are not applicable to this PR.

  • If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list — the configurations we want to use are not in that list

Risk Matrix

  • Please do double check whether there's anything I'm missing here which could make this a problem. I couldn't find any reasons not to do this change, especially considering valid credentials would be needed anyway.

For maintainers

@lucasfcosta lucasfcosta added bug Fixes for quality problems that affect the customer experience Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability labels Aug 15, 2022
@lucasfcosta lucasfcosta requested a review from a team as a code owner August 15, 2022 13:55
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

@lucasfcosta lucasfcosta added the release_note:skip Skip the PR/issue when compiling release notes label Aug 15, 2022
@dominiqueclarke
Copy link
Contributor

@elasticmachine merge upstream`

@dominiqueclarke
Copy link
Contributor

@elasticmachine merge upstream

Copy link
Contributor

@dominiqueclarke dominiqueclarke left a comment

Choose a reason for hiding this comment

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

LGTM.

Works great on dev and qa. Continues not to work on prod and staging, as they are prohibited from using basic auth on the service side.

@dominiqueclarke
Copy link
Contributor

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

@lucasfcosta lucasfcosta merged commit 276cd3d into elastic:main Oct 5, 2022
@lucasfcosta lucasfcosta deleted the use-authorisation-if-present branch October 5, 2022 13:09
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Oct 5, 2022
@awahab07 awahab07 self-assigned this Oct 5, 2022
@awahab07
Copy link
Contributor

awahab07 commented Oct 5, 2022

Post FF Testing

  • On a prod cluster, with the following Kibana config:

    xpack.uptime.service.manifestUrl: <dev-service-url>
    xpack.uptime.service.username: <correct-ussername>
    xpack.uptime.service.password: <correct-password>
    xpack.uptime.service.showExperimentalLocations: true

    it works, whereas with the following config (when credentials are incorrect):

    xpack.uptime.service.manifestUrl: <dev-service-url>
    xpack.uptime.service.username: <correct-ussername>
    xpack.uptime.service.password: <incorrect-password>
    xpack.uptime.service.showExperimentalLocations: true

    it fails.

  • Only providing a manifest URL also works i.e.

    xpack.uptime.service.manifestUrl: <dev-service-url>
    xpack.uptime.service.showExperimentalLocations: true

Thus, the PR changes work as expected.

@awahab07 awahab07 removed their assignment Oct 5, 2022
WafaaNasr pushed a commit to WafaaNasr/kibana that referenced this pull request Oct 11, 2022
* fix: always send authorization to synth service when configured

* [CI] Auto-commit changed files from 'node scripts/build_plugin_list_docs'

Co-authored-by: Kibana Machine <[email protected]>
WafaaNasr pushed a commit to WafaaNasr/kibana that referenced this pull request Oct 14, 2022
* fix: always send authorization to synth service when configured

* [CI] Auto-commit changed files from 'node scripts/build_plugin_list_docs'

Co-authored-by: Kibana Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting backport bug Fixes for quality problems that affect the customer experience release_note:skip Skip the PR/issue when compiling release notes Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v8.5.0 v8.5.1 v8.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants