-
Notifications
You must be signed in to change notification settings - Fork 385
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 proposal for grafanaserviceaccount crd #1413
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.
Makes sense overall, just a few small questions, no need to answer in super-technical detail, a high-level idea is enough (just so that we can reference it later for development once we have it all agreed).
If you can add the answers to those questions in the proposal file itself, that's great!
Thanks for writing this up! @MickeHedlund
id: grafana-sa | ||
roles: [Viewer/Editor/Admin] |
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.
Nice, +1 to aligning it to the upstream API object, this is pretty much our standard approach in the operator, thanks for doing it this way
permissions: | ||
- user: <users in the cluster/root user etc> | ||
permission: [Edit/Admin] |
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.
Can you clarify what this field is for?
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.
The spec.tokens whas my suggestion for how to map what secrets with tokens the operator should create and that belongs to this GSA.
spec.permissions was to try and map the same values that was able to be set when creating the GSA in the grafana gui. To be able to set specific permissions based on usernames and groups.
Today the Grafana Service Account is only held in memory if not using persistent, so when a pod is restarted or redeployed the grafana service account is removed. The grafana service accounts are also only possible to create using the Grafana GUI or with the HTTP-API and cannot be pre-defined before deploy or kept as IAC. | ||
|
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.
Bear in mind that the service account token will change once the grafana instance is redeployed (when the pod terminates), so the token for a specific instance might change (not often, but it's something to keep in mind)
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 dont see an issue with this as long as the k8s-secret is updated with the new token, then other applications that needs the token could read the updated secret to fetch the updated token.
My proposal is to handle grafana service account as a separate CRD that can be specified by the user even before setup and that can be included in a CICD pipeline. It will enable so that the operator can create predefined service accounts on deploy and store the token in a k8s-secret readable by other applications without any manual steps. | ||
|
||
### Defining what Grafana Service Account belongs to what grafana. | ||
I suggest that its done in the same way as dashboards and datasources are using instanceSelector to find the proper grafana, the same thing could be done with the grafana service accounts using the same labels etc. |
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.
+1 to instanceSelector, however, how would we handle the generation of the GSA token secret? Seeing as we will create a one-to-many relationship here, that means one-secret per selected Grafana
A number of other questions sprawl up for me:
-
What namespace do we create the resulting
grafana-sa-token-secret-<identifier>
in? woould it be the the namespace of theGrafana
we create it for, or the namespace within which theGrafanaServiceAccount
lives? -
How will we reference all the secrets from within the
GrafanaServiceAcccount
CR? looking at this from the use case of: if a user deletes theGSA
how will the operator be able to delete all subsequent serviceaccounts on all managedGrafana
instances, as well as their relevant secrets.
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.
Id say that if multiple tokens are specified in the grafanaserviceaccount cr then the operator would create those tokens as k8s-secrets with the name that is specified in the grafanaserviceaccount cr and that will be the reference for grafana what secret is used for what grafanaserviceaccount. The default would only be to create one with the default name, if spec.create.generateTokenSecret is true that is. Which should be default.
As for the other questions,
- I think it's preferable to have the secret residing in the same namespace as the Grafana CR due to how RBAC rules for secrets are often more restricted in the view scope, and a normal usage pattern would be to use said secret for other applications.
- I think that with the k8s-secrets containing the tokens as a list under spec.tokens the operator could remove the secrets when the GSA is removed. I could also see the use of a "Managed by: Grafana-operator"-label on the secrets, allowing the operator to find all its created secrets and compare if they belong to any GSA that still exists.
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.
To address the security concerns outlined by @NissesSenap & deletion mapping, I have the following idea:
Instead of handling serviceaccounts as separate resources, we could add them as a field to grafana instances directly. This would solve the security concers by restricting service account creation to the same group of users which can already modify the Grafana instance while also keeping everything in one place.
What are your takes on this?
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.
The good thing is that it's simple. The bad thing is that if you want a service account, the owner of the grafana instance will have to create it.
My guess is that this will be the case most of the time any way, so it's probably not a huge issue.
What do you think @MickeHedlund ?
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 dont see an issue with the owner of the grafana having to create service accounts. That how we would want it to work in our use cases.
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.
It would solve lots of issues and make us having to write one less controller which is always nice.
@MickeHedlund could you update the proposal to match this change?
More or less the same, but move everything over to the Grafana
CR
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 will update it, I will have to see if I have time to do it today, if not it will be updated early next week.
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 have pushed an updated document, let me know if I need to correct something.
From a security point of view I'm relatively worried about this, since anyone having the possibility to create a GSA CRD would have the possibility to become admin in any instance they want to. For a secret name, I'm leaning towards not letting the user providing the secret name where the token will reside. I think it would be good if we can get an example on how the secret should look like depending on the tokens. |
create: | ||
generateTokenSecret: [true/false] #Will create the k8s-secret with a default name if true. Defaults to true. | ||
serviceaccount: | ||
id: grafana-sa |
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 know that we let our users set id
in most places in the operator. Is it a good idea in the CRD though?
What would happen if two CRDs have the same ID? The tokens would be recreated over and over again, and it's a relatively big risk of creating an incident for the end users.
We could solve this by having admission controllers in the operator, the CEL language support isn't enough for separate CRDs. But adding this kind of functionality would make the operator much harder to get started with, since you need a ssl cert.
But we can just remove this issue by not letting the users setting this id and instead use the CRD name.
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 don't think it's super important to be able to set the id if that would cause issues. But it might be worth having the possibility to do it with a disclaimer of some sorts that this could happen if two of them would have the same ID? I feel that it isnt that different from working with for example flux where this happens if two resources have the same name etc.
But that said, Im only basing it on how we use it in our organization, and I dont have a strong need for being able to set ID so I dont see an issue with excluding it.
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.
This looks good to me. I would update the CRD as mentioned below, but other than that I think this is okay.
Do you need me to fix the trailing white spaces and update the branch? |
If you can it would be great |
I think everything should be fixed now |
My design document over a proposed Grafana Service Accounts as CRD as discussed in issue 1388