search

grafana / grafana-operator

An operator for Grafana that installs and manages Grafana instances, Dashboards and Datasources through Kubernetes/OpenShift CRs
https://grafana.github.io/grafana-operator/
Apache License 2.0
863 stars 384 forks source link

Feat(helm): Add possibility to attach volume (cm, secret...) to the operator pod #1590

Closed aboulay-numspot closed 2 months ago

aboulay-numspot commented 2 months ago

Aim of the merge request:

This MR updates the HelmChart of the grafana-operator to permits user to attach volume to the container deployed by the HelmChart. To do this, we implement extraVolumes and extraVolumeMounts field in the HelmChart.

Implementation details

We choose to implement these field like this because it gives flexibility with the env field to use variable from mounted secret. If you have any comment about it, let me know. :)

Additional comments

To be fully honest, I don't understand how you handle version update of the HelmChart, let's discuss about this here.

NissesSenap commented 2 months ago

Why is this feature needed @aboulay-numspot? I can't come up with any reasons why the operator itself needs this kind of feature, but I would love to understand why.

aboulay-numspot commented 2 months ago

Why is this feature needed @aboulay-numspot? I can't come up with any reasons why the operator itself needs this kind of feature, but I would love to understand why.

@NissesSenap: Sure, Let me give you first a bit of context. Currently, I work in an environment where every service (Grafana too) are using a custom certificate (self-signed) for tls. The Grafana instance I want to target is outside of the cluster and required a full domain to connect with.

After looking for a solution to make the operator trust a new CA bundle, I have found that the operator does not handle ssl specification for a certificate (aka no_verify or better give it a new certificate bundle to thrust the target) with configuration and will probably require to pass by the container certificate. (I asked on the slack channel and @weisdd answer seems to be going that way).

So I look for the container build method of ko and found this page which explain that the default container is cgr.dev/chainguard/static.

Then, I dug for the container info here and found an interesting variable SSL_CERT_FILE which look to be the path of the ssl certificate bundle.

And then, I found that I could not give secrets nor configmap to the operator to validate my hypothesis. Because this feature was not costly, I implemented it and submitted here. Feel free to refuse it if you want. :)

After this MR, I would be able to update this certificate bundle and probably (I cannot test yet), being able to give the CA associated to the Grafana instance to the operator container.

weisdd commented 2 months ago

@NissesSenap yeah, the idea is to have a way to modify system CA bundle, which should be possible by mounting a custom bundle into a pod. Of course, we need to see if it works or requires some additional manipulations. It should potentially be more convenient than repackaging images.

@aboulay-numspot btw, since you have a locally modified copy of a chart, you can already try to replace the bundle.

NissesSenap commented 2 months ago

Ah of course, sounds like a good feature assuming that it works.

But it would be nice to add some documentation that explains how to add your own custom CA.

aboulay-numspot commented 2 months ago

Ah of course, sounds like a good feature assuming that it works.

But it would be nice to add some documentation that explains how to add your own custom CA.

@NissesSenap I need to test it on my workstation. To be fully honest, I don't think this is a good way to implement this functionality because, for me, this should be handled in the Grafana CR with a SSL block in the external resource to permits the TLS connection to the Grafana target. It's just a (potential) quick hack which should permits this if it work.

weisdd commented 2 months ago

@aboulay-numspot I haven't seen any examples with other software where you would be able to import CA bundles. Switching on/off TLS verification gets offered, but that is not necessarily a good practice. From what I've seen in many companies that have PKI, people just have pipelines that help to repackage certain images.

aboulay-numspot commented 2 months ago

@aboulay-numspot I haven't seen any examples with other software where you would be able to import CA bundles. Switching on/off TLS verification gets offered, but that is not necessarily a good practice. From what I've seen in many companies that have PKI, people just have pipelines that help to repackage certain images.

@weisdd I think a good example could be Grafana Alloy which permits it to send information to Loki, Mimir... Check this https://grafana.com/docs/alloy/latest/reference/components/loki.write/#tls_config-block For each client type, you can select a dedicated CA bundle to connect with.

I can talk with you about this at the maintainer point if you want. :)

NissesSenap commented 2 months ago

Let's not implement any short term solution that we have to maintain long term.

If we want it to be a part of the grafana CR client request, create a PR that does that instead, it shouldn't be too complicated to implement.

aboulay-numspot commented 2 months ago

Ok, I understand this implementation does not interest you. Can you please write a message and close the MR?

pb82 commented 2 months ago

@NissesSenap is your idea to apply the bundle per CR when a new Grafana client is created (e.g. https://github.com/grafana/grafana-operator/blob/master/controllers/dashboard_controller.go#L107). That solution would also be specific to bundles and not allow mounting arbitrary volumes.

weisdd commented 2 months ago

@NissesSenap Well, I cannot see any downsides in having support for custom volumes as part of the chart. It's something very standard and would not hurt even if not widely used. :) As for a more complex solution, the idea was not discussed in details and we don't have any ETAs yet, so, even if it gets implemented, it could easily be months from now. So, personally, I'm more in favour of merging the current PR and having a proper discussion in a dedicated issue or PR with a design document (@aboulay-numspot I guess you can prepare something like that).

aboulay-numspot commented 2 months ago

@NissesSenap is your idea to apply the bundle per CR when a new Grafana client is created (e.g. https://github.com/grafana/grafana-operator/blob/master/controllers/dashboard_controller.go#L107). That solution would also be specific to bundles and not allow mounting arbitrary volumes.

@pb82 For me, the most elegant solution should be able to pass it by extending the Grafana CR external block to give it TLS specification and use the same mechanism than credentials to retrieve the certificate from a dedicated secret. I think it is more in the operator's "philosophy". Doing this way, we do not need to mount a volume into the operator pod.

I think this solution can be hard to implement because it touches to every part of the operator (grafana client, kubernetes secret retrieve mechanism...). But, I cannot be sure of this before trying an implementation.

@NissesSenap Well, I cannot see any downsides in having support for custom volumes as part of the chart. It's something very standard and would not hurt even if not widely used. :) As for a more complex solution, the idea was not discussed in details and we don't have any ETAs yet, so, even if it gets implemented, it could easily be months from now. So, personally, I'm more in favour of merging the current PR and having a proper discussion in a dedicated issue or PR with a design document (@aboulay-numspot I guess you can prepare something like that).

If the idea on the first paragraph above match with your vision, I can redact you a document about it to illustrate this and discuss about it with you during the Monday's morning maintainer call. Is this plan fit for you?

Oh, and a last thing (again). This MR took me 10min to implement so feel free to refuse it if you don't think it is useful for the operator or if it mismatches with your vision.

NissesSenap commented 2 months ago

What @aboulay-numspot around this is my thought as well.

@pb82 For me, the most elegant solution should be able to pass it by extending the Grafana CR external block to give it TLS specification and use the same mechanism than credentials to retrieve the certificate from a dedicated secret. I think it is more in the operator's "philosophy". Doing this way, we do not need to mount a volume into the operator pod.

@weisdd my issue with it is that I just don't see any upside with it. @aboulay-numspot is the first person that have asked for this feature and what I learned over the years is that we should resist merging stuff that only single users asks for. I mainly don't like the thought of doing a workaround instead of solving it long term. Sure, this PR won't be hard to maintain, but something something slippery slope. But this is not the hill I will die on ;) I have missed the last maintainer meetings and if you have had a discussion around this, and feel that we should merge it, go for it.

I will try to make it to the next maintainer meeting, but feel free to merge it beforehand.

theSuess commented 2 months ago

Closing this in favour of #1594