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

support folder-permissions #809

Closed
wants to merge 7 commits into from
Closed

support folder-permissions #809

wants to merge 7 commits into from

Conversation

Iridias
Copy link
Contributor

@Iridias Iridias commented Jul 29, 2022

Description

In Grafana, Dashboard-permissions cannot be more restrictive, than the folders they're in.
Thus it's necessary, to be able to configure the folder-permissions.

I added a new type for folders (together with the obligatory controller, client and config) to do exactly that.

notes / known issues:

  • In Grafana the folder-title is not unique (it relies on the uid). As the folders are referenced from the GrafanaDashboards via customFolderName, the uid will be reproducible generated from the title, making it unique as well.
  • ATM the folders won't be deleted by the Reconciler (because we don't need that currently, and I'm not sure, if it might cause problems in certain circumstances ...and it can be implemented later anyway)
  • Support to set folder-permissions for teams or users is not yet implemented - only roles are supported ATM (but should be easy to implement later - it's just, that the json-payload requires a different type for it)

Bugfix:
Along the way, I added more log-output and in main() I moved printVersion() to be executed after the logger has been set/initialized. Its output was missing from the k8s-log otherwise.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Checklist

  • This change requires a documentation update
  • I have added tests that prove my fix is effective or that my feature works
  • I have added a test case that will be used to verify my changes
  • Verified independently on a cluster by reviewer

I added a few unit-tests, but not enough to claim, that it proves the effectiveness.

Verification steps

  • deploy the grafana-operator with the changes (don't forget to apply the new CRDs!)
  • add the k8s-config for the new kind GrafanaFolder to your configuration.
  • Configure the permissions to not include Viewers (There is an example how this should look like in: deploy/examples/folders/restricted-folder.yaml)
  • optional: put a GrafanaDashboard in the folder by referencing its title in customFolderName
  • deploy/apply it
  • open a web-browser of your choice and open the grafana-instance and navigate to "dashboards/manage" (or add /dashboards in the url-bar)
  • Verify that a folder with the configured title exists
  • hover over it and click on "Go to folder"
  • click on the tab "Permissions"
  • You should only see entries for "Admin" and "Editor" - compare with the default-folder (should have the title of the k8s-namespace its in), which also has "Viewer" configured

@HVBE
Copy link
Collaborator

HVBE commented Aug 8, 2022

@Iridias Thanks for the PR, we'll take a look at this shortly and discuss. I'll try to review and verify on my cluster. Initially though, the PR looks great!

@NissesSenap
Copy link
Collaborator

@Iridias could you recreate this PR and use another branch then master? I was going to fix the CI but I can't since it's your master branch.
Or you can of course fix it on your own. The controller-gen version have been updated, and due to how we download the binaries you will need to remove it.

Easiest way is to do rm bin/controller-gen from the grafana-operator folder.
That way you should pass the validation issues at least.

@NissesSenap
Copy link
Collaborator

We will continue this PR in #821

@NissesSenap NissesSenap closed this Sep 6, 2022
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

4 participants