-
Notifications
You must be signed in to change notification settings - Fork 55
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
Example App with automatic tailored service account setup #203
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gamab not entirely sure this PR was ready for review yet, but when trying it out locally I get the following error. So doesn't seem like the externalServiceRegistration
from plugin.json is registered correctly?
The plugin uses a [Grafana service account token](https://grafana.com/docs/grafana/latest/administration/service-accounts/#service-account-tokens) to authenticate against the Grafana API. To enable it, add the section below to your `plugin.json` file. | ||
|
||
```json | ||
"externalServiceRegistration": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know if this if documented somewhere? Could not find in the portal or grafana.com/docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll tackle that soon :)
Wow faster than ever 💯 No this is not up for review yet 🙏 If you want to test this out, I did a PoC branch in grafana, that I'm just now starting to split into smaller PRs to ease reviews and comments. Here is the epic for context: https://github.com/grafana/grafana-enterprise/issues/5824 🙂 I already DMed @andresmgot but I still need to notify and talk with the plugin squad for full approval 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the future, it may makes sense to merge this example with the existing one (since they are very similar) and maybe through documentation explain how to use it in on-behalf-of mode or just with a service account
@andresmgot given we'd now use the |
yeah, I am not that convinced about that now. I guess it's better to error on the side of being more verbose and have two different examples. It will also be more convenient if both features follow a different release schedule. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we actually meant to commit all these files? Should we git ignore them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, we should include them. These are needed to execute the different yarn
scripts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anything else you are planning to add here? Is this ready for review?
context: ./.config | ||
args: | ||
# Points to an unreleased v10.1 version of Grafana | ||
grafana_version: ${GRAFANA_VERSION:-10.0.3} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this docker-compose work? we probably need to at least update the GRAFANA_VERSION
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No it does not. I need to understand and test this out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, fixed it, but since 10.2 is not out yet, I had to run on latest main :)
GRAFANA_VERSION=main docker-compose up
Trying this with the docker-compose (using I have modified the action to BTW, we should also update the other example (on-behalf-of) to adapt it to the new format. I guess there is hurry though since the new format is not released yet. |
I did the same and it surprised me as well. It's something I wanted to discuss with my team. Service Accounts don't get Admin permissions on what they created 😢 Meanwhile I guess, for this example to make more sense, we can add Wdyt?
Yep 👍 It's at the bottom of the epic's task list. Do you want to do it? |
hmm, I am not sure I follow. The service account doesn't create the dashboard (I created it as an admin). I just want the service account to have permissions to list / get dashboards. I tried with
Sorry, there was a typo in my message. I wanted to say there is no hurry, you can do that when you see fit, I just didn't want to forget. |
Ah my apologies! My bad for the Thanks for the careful review! 💯 I investigated a bit more on why it was not working, this is an RBAC bug with the search function: https://github.com/grafana/grafana-enterprise/issues/5920 Hence I ended up granting broader permissions to the plugin. It should now work (and do more checkout the description). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Tested and working fine 👍
In this PR, through a new example, we show how a plugin can get a dedicated service account and a token using the
externalServiceRegistration
attribute in itsplugin.json
.For example, to allow a plugin to create dashboards, list/update all dashboards and folders, list users, teams, team members. The registration looks like this:
This is still an experimental feature (behind a feature flag) but can be used starting with Grafana
v10.2.0
.Since
v10.2.0
is not out yet, you'd need to run thedocker-compose
with latest main tag to test this now:Special note to reviewers:
I shamelessly copied the
app-with-on-behalf-flow
example plugin.I'm not entirely sure that's ok or if I should have ran init functions instead.
I also have in my backlog to update the plugin docs.
Ref for context:
https://github.com/grafana/grafana-enterprise/issues/5824
Example of requests with these permissions
Create a dashboards
POST /dashboards/db
Search dash & folders
GET /search
Modify an existing folder
PUT /folders/uid/:uid
List users
GET /org/users
List teams
GET /teams/search
List team members
GET /teams/1/members