Add grafana service account feature to Grafana controller

[NissesSenap]

In https://github.com/grafana/grafana-operator/blob/9f1b655fc9177f53b9a7c698ed895e5487e64734/docs/docs/proposals/003-grafanaserviceaccount-crd.md we have designed a way of managing grafana service accounts after discussions in this PR: #1413

This issue is to track this implementation.

[NissesSenap]

@MickeHedlund are you willing/able to help out to implement the feature as well?

[MickeHedlund]

I wish I could, but I doubt that I have the skills to get it done.

[NissesSenap]

That is fine. Thanks for the info

[ndk]

Would you mind if I worked on it?

[NissesSenap]

That would be awesome @ndk , please go ahead and feel free to reach out here or in slack if you need any pointers.

[weisdd]

I haven't tried it myself yet, but seems like external-secrets operator can now generate service accounts: external-secrets/external-secrets#4203

[solidDoWant]

Is the proposal still open for feedback despite being merged? It's listed as "Suggested" here, so I'm hoping it is.

Personally I'm deeply in favor of a separate resource for this. In the same way that storing dashboards in the Grafana resource would create tight coupling between dashboards and grafana instances, the current proposal adds tight coupling between external applications and grafana instances. This causes several problems with applications using Grafana, as they must either:

• Share a service account (very undesirable from security, auditing, and reliability perspectives)
• Use a separate grafana instance per application instance (increasing overhead, and removing some of the benefits of grafana)
• Update the grafana instance every time a new instance is deployed, removed, or needs permissions changed (removing the ability to automate deployments)

I completely get the cross-namespace concerns mentioned in the proposal PR. However, this can be solved several ways, as other projects have:

• Add a namespace selector field to the grafana resource. This would allow grafana administrators to choose what namespaces to accept service accounts from. Service account resources would have a similar field, and service accounts would only be deployed to instances where the grafana instance and service account selectors intersect. [This is how the k8s gateway API does it].(https://gateway-api.sigs.k8s.io/reference/spec/#gateway.networking.k8s.io/v1.AllowedRoutes).
• Only deploy the grafana service account if the creator k8s identity of the resource has a new, specific k8s RBAC permission. For example, the operator should reject (or fail via status condition) when the creator or updater of a resource does not have a create permission for a grafana/service-account resource on the targeted grafana instance. This is how cert-manager approval policy does it. This approach offloads approval/denial logic to the k8s control plane.

While having a separate resource for this would necessitate an additional controller, I think this is a much better approach for users due to the decoupling between grafana instance deployments, and application deployments.

[NissesSenap]

Hi @solidDoWant , I do see your point, I guess the biggest thing for us as maintainers is that we wanted to keep the complexity down. And in general, it's mostly the grafana owner that will need service accounts (but there are of course exceptions to this).
And in general, if not who ever wants a new SA should be able to do a PR to who ever manages the grafana instance (yes I assume everyone uses GitOps).

Sadly, RBAC generation is very limited in Kubebuilder, so it's hard to do any custom things around it like a custom role or if it should be cluster/namespace scoped.

But I personally like the AllowedRoutes solution of Gateway API.

But we already have a cross namespace "solution" that is very simple.
And I don' t think we should do any custom for the SA CRD and the current solution isn't good enough to support SA CRD as mentioned in the proposal.

So my suggestion would be to keep the design and @ndk can continue with hes implementation.

But I would suggestion that @solidDoWant create a new design proposal on how we could start using the AllowedRoutes for all our CRDs instead. It's way more flexible and it will keep us consistent.
This would of course be a breaking change and we would have to release a v1beta2 or a v1 version of the CRDs.

But I'm a very inactive maintainer, and this are just my thoughts. So lets see what the others think.
How do you feel about it?

[solidDoWant]

Thanks, that makes sense. I had considered proposing an AllowedRoutes type solution that would affect all resources, but figured that it'd be a large, complex, and sweeping change (needing new CRD versions as you mentioned). With that in mind I though that this might be a good first place for such an approach, as service account support is a new feature, and this solves a specific issue associated with them.

I'd be happy to file a separate proposal. Is there any specific documentation for proposal requirements and process, or should I just file a PR for a new doc that matches the format of these files? I checked the README and CONTRIBUTING docs and didn't see anything.

If any other users stumble across this issue while looking for a way to add service accounts as separate resources, it looks like External Secrets operator can now handle this out of the box, although there are currently no docs for this. I have a PR open to add basic auth support, which should allow ES to nicely integrate with Grafana operator Grafana instances, via the admin credentials.

[weisdd]

@solidDoWant a proposal doesn't have to fully match any of those documents, though they are likely to be a good reference (in a sense of a starting point). Since service accounts are not supported by the operator yet, we're pretty open to suggestions on how they should eventually get implemented, nothing is set in stone :)