[stable/grafana] Enable extra init containers and extra emptyDir mounts

[jokogr]

What this PR does / why we need it:

This PR introduces two new parameters for the Grafana chart, extraInitContainers and extraEmptyDirMounts.

Since Grafana v6.0.0, it is possible to provision notification channels. These channels might have some sensitive parts (i.e. tokens) and therefore I would like to be able to have an init container which initializes somehow the necessary yaml files. The values can then be shared with the actual Grafana container via emptyDir volumes.

cc @zanhsieh @rtluckie @maorfr

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

• DCO signed
• Chart Version bumped
• Variables are documented in the README.md

[k8s-ci-robot]

Hi @jokogr. Thanks for your PR.

I'm waiting for a helm member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[zanhsieh]

@rtluckie @maorfr @jokogr
PR initial impression looks good. Just curious:

• How high chance would it be if Grafana pod gets auto-injected by other services, e.g. Jaeger? Do I worry too much?
• Should it out-weight the simplicity of the original chart?
• What other option we have here?

[maorfr]

Please take a look at these 2 PRs:
#12184
#12351

is this PR still needed?

[zanhsieh]

Please take a look at these 2 PRs:
#12184
#12351

is this PR still needed?

I think @jokogr probably needs to mount some empty volume.

[jokogr]

@zanhsieh thanks for reviewing this. I also agree that this PR adds complexity, but the Grafana chart is already complex: there are already other init containers and a fine-grained selection of volumes and their respective mounts (e.g. from ConfigMaps, secrets).

I am not aware of Jaeger and I do not have any other services auto-injecting Grafana pods. I came up with this PR because it suits to my case as explained above (and below). If you have a better way, just let me know.

@maorfr thanks for providing the PRs, I actually missed #12184, which does the first half of this PR. However, this PR is still needed to define and mount the emptyDir volume, as @zanhsieh pointed out.

Let me explain once again the reasoning behind this PR:

• Grafana introduced support for provisioning notifiers. This is well addressed and implemented in [stable/grafana] Notification channel provisioning (Resolves issue #12334) #12351.

• However, I want to be able to use a secret value in them (I consider the token part for the notifiers a secret) and currently Grafana does not support parsing environment variables in YAML files (see Support for environment variables in provisioning grafana/grafana#12896). I do not want to store the secret values directly to a ConfigMap.

• In the current chart I could mount a big secret with all the necessary configuration settings, but this grows to be inflexible for the deployment strategy I use. I prefer storing only secret values in Kubernetes secrets.

• So ideally I have an init container with some secrets mounted or as environment variables, which creates this notifiers file. The file is created on an emptyDir volume which is also mounted on the Grafana pod, so that data is shared.

I am going to rebase this PR and update the Chart version number.

[jokogr]

Rebase is done, chart version number got updated.

If you think so, I can skip the first commit (which introduces the extra init containers) and wait for #12184 to be merged. Just let me know here.

[maorfr]

/assign
/hold

hey @jokogr,

thanks for getting involved!

as I was stating in #12184, I am not fond of such general fields such as extraInitContainers and extraEmptyDirMounts, as they are very general, and I prefer that we provide our users with a simpler approach (as you said, this chart is already complex as it is). If there is a possibility to implement this feature while exposing a minimal configuration in values.yaml, I would prefer it. Take a look at some other init containers as references. The heavy lifting should be done in the templates and not in values.yaml.

What do you think?

[jokogr]

Hey @maorfr,

I share your concerns over over-generalizing the charts, but I have yet to find a more elegant way to avoid doing so.

From one hand, I would like to introduce some custom stuff that are possibly not interesting to anyone outside my company. On the other hand, I would like to use the official public charts, so that my deployments are up-to-date with minimal effort.

As such, my current approach is to use official, public charts stated in the requirements of a custom chart (so to have them as subcharts). If I want to make any expansions, e.g. write my own config maps, then I just add a new template in the custom chart I maintain. If I have to make a change in a pre-existing template, then eventually I have to rely on the subchart's (aka the official chart's) values to do so.

What do you think? Is there a more easy / elegant way to achieve what I am describing?

[maorfr]

@jokogr,

as i understand, and please correct me if i am wrong -
the problem you are describing is that you need to to have files for notifiers, but they need to include sensitive data (such as tokens) which you do not want to be included in a ConfigMap.

my proposed solution would be -
same way to can mount a ConfigMap, you can also mount a Secret. This can be a pre-existing secret with all the sensitive data (in this case, files to be used with the notifiers feature).

in this is indeed the case, you can use the extraSecretMounts field which already exists in the chart!

if this is not sufficient, this can be easily exposed using a single field in values.yaml, maybe under the notifiers section?

let me know what you think! :)

[jokogr]

@maorfr what you propose is indeed a valid option, i.e. to create the complete notifiers files as a Kubernetes secret and then mount it via extraSecretsMount. Thanks for taking the time to think about it!

Unfortunately I am currently against it, as I stated before:

In the current chart I could mount a big secret with all the necessary configuration settings, but this grows to be inflexible for the deployment strategy I use. I prefer storing only secret values in Kubernetes secrets.

The reason for this is that Kubernetes secrets on my production environment are handled manually / outside of the Helm scope. Maintaining a big notifiers file as a secret would be a bigger burden for me.

Anyway, I am adding one last point to backup this PR: Several official charts from stable repo share the same practice:

• ark ([stable/ark] Add init container support #11615)
• elasticsearch (Promote elasticsearch to stable #8476 - moved from incubation already with extra init containers)
• filebeat ([stable/filebeat] Allow for additional initContainers #7401)
• gocd ([stable/gocd] optional init containers #10397)
• keycloak ([stable/keycloak] Add option to specify extra init containers #5887)
• kibana ([stable/kibana] Add support for user-defined init containers #11199)
• mariadb ([stable/mariadb] Support for extraInitContainers like in other popular charts #10423)
• mattermost-team-edition ([stable/MM-TE] add extra initcontainers and envvars #9458)
• mongodb ([stable/mongodb] Add init container support #11949)
• mysql ([stable/mysql] Support extra{Volumes,VolumeMounts,InitContainers} #7187)
• nginx-ingress ([stable/nginx-ingress] Support initContainers #5801)
• neo4j (Neo4j init containers #3047)
• sonatype-nexus (Add support for additional secret, initContainers, postStart command and deployment annotations #8338)

If you still find this PR unwanted, please feel free to close it; no hard feelings. After all you are one of the maintainers, so I understand your burden and thank you for your time evaluating my PR :)

[maorfr]

/hold cancel
/ok-to-test

important to say - i am not so much against it, but i do feel that anyone who brings up a problem is likely representing more users who are having it as well. my approach is to simplify as much as possible.

it is obvious that you have given this much more thought then i have, and as a maintainer, one of the things that is in my top priorities is that this chart will be used!

lets go ahead and merge your change :)

[maorfr]

/approve
/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jokogr, maorfr

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• stable/grafana/OWNERS [maorfr]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[petrokashlikov]

Hi @jokogr , as now this change is live, do you have by any chance example how to use this, to create notifications file as you described?