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 support for authenticating by bearer or JWT token to mimirtool #2146

Merged
merged 8 commits into from Jun 21, 2022

Conversation

javad-hajiani
Copy link
Contributor

@javad-hajiani javad-hajiani commented Jun 20, 2022

What this PR does

This PR allows mimirtool to request mimir api set behind gateway which authenticating by bearer or JWT token. at this point of time it only supports basic auth.

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@CLAassistant
Copy link

CLAassistant commented Jun 20, 2022

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@56quarters 56quarters left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! A few minor comments below.

pkg/mimirtool/client/client.go Outdated Show resolved Hide resolved
pkg/mimirtool/commands/alerts.go Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

Thank you.

pkg/mimirtool/client/client.go Outdated Show resolved Hide resolved
pkg/mimirtool/client/client.go Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
pkg/mimirtool/commands/alerts.go Outdated Show resolved Hide resolved
javad-hajiani and others added 3 commits June 20, 2022 16:23
Co-authored-by: Nick Pillitteri <56quarters@users.noreply.github.com>
Co-authored-by: Peter Štibraný <pstibrany@gmail.com>
@javad-hajiani
Copy link
Contributor Author

Question: After adding all above Flags I've ran make doc but it didn't update documentations for mimirtool placed here. anything else need to be done?

@pstibrany
Copy link
Member

Question: After adding all above Flags I've ran make doc but it didn't update documentations for mimirtool placed here. anything else need to be done?

I am afraid this is still updated manually.

Copy link
Member

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

Thank you!

@@ -11,6 +11,7 @@ This action is configured using environment variables defined in the workflow. T
| `MIMIR_ADDRESS` | URL address for the target Mimir cluster | `false` | N/A |
| `MIMIR_TENANT_ID` | ID for the desired tenant in the target Mimir cluster. Used as the username under HTTP Basic authentication. | `false` | N/A |
| `MIMIR_API_KEY` | Optional password that is required for password-protected Mimir clusters. An encrypted [github secret](https://help.github.com/en/actions/automating-your-workflow-with-github-actions/creating-and-using-encrypted-secrets) is recommended. Used as the password under HTTP Basic authentication. | `false` | N/A |
| `MIMIR_AUTH_TOKEN` | Optional bearer or JWT token that is required for Mimir clusters authenticating by bearer or JWT token. An encrypted [github secret](https://help.github.com/en/actions/automating-your-workflow-with-github-actions/creating-and-using-encrypted-secrets) is recommended. | `false` | N/A |
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
| `MIMIR_AUTH_TOKEN` | Optional bearer or JWT token that is required for Mimir clusters authenticating by bearer or JWT token. An encrypted [github secret](https://help.github.com/en/actions/automating-your-workflow-with-github-actions/creating-and-using-encrypted-secrets) is recommended. | `false` | N/A |
| `MIMIR_AUTH_TOKEN` | Optional bearer or JWT token that is required for Mimir clusters authenticating by bearer or JWT token. | `false` | N/A |

I would remove mention of "An encrypted github secret is recommended." because mimirtool user cannot control what the Mimir installation (or gateway in front of it) is using.

But I see this is copied from MIMIR_API_KEY description, so we can address both later.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The reason why the GitHub secret is mentioned here is different. This is the mimir-rules-action and it's suggesting to use a GitHub secret to hold this env variable value.

@pstibrany pstibrany merged commit 41e4d0d into grafana:main Jun 21, 2022
masonmei pushed a commit to udmire/mimir that referenced this pull request Jul 11, 2022
…rafana#2146)

* Add Auth token support to mimirtool rules/alert/alertmanager commands

* Update CHANGELOG.md

Co-authored-by: Nick Pillitteri <56quarters@users.noreply.github.com>

* Update pkg/mimirtool/commands/alerts.go

Co-authored-by: Peter Štibraný <pstibrany@gmail.com>

* make auth-token environment variable configurable

* Updated error message to be more meaningful

* Moved if condition to upper block for having better readability

* Moved all if blocks to single switch block

* Add docs for MIMIR_AUTH_TOKEN

Co-authored-by: Nick Pillitteri <56quarters@users.noreply.github.com>
Co-authored-by: Peter Štibraný <pstibrany@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants