feature(helm): Added template support for config and collector files

[Avihais12344]

Added support to use helm templates at .Values.config and every key at .values.collectorFiles so you can better organize your values file.

For example, instead of doing this:

collectorFiles:
  pricing_data_freshness.collector.yml:
    collector_name: pricing_data_freshness
    metrics:
      - metric_name: pricing_update_time
        type: gauge
        help: 'Time when prices for a market were last updated.'
        key_labels:
          # Populated from the `market` column of each row.
          - Market
        static_labels:
          # Arbitrary key/value pair
          portfolio: income
        values: [LastUpdateTime]
        query: |
          SELECT Market, max(UpdateTime) AS LastUpdateTime
          FROM MarketPrices
          GROUP BY Market

You can put the config at a different file (my_collector.yaml for example):

collector_name: pricing_data_freshness
metrics:
  - metric_name: pricing_update_time
    type: gauge
    help: "Time when prices for a market were last updated."
    key_labels:
      # Populated from the `market` column of each row.
      - Market
    static_labels:
      # Arbitrary key/value pair
      portfolio: income
    values: [LastUpdateTime]
    query: |
      SELECT Market, max(UpdateTime) AS LastUpdateTime
      FROM MarketPrices
      GROUP BY Market

And use it like that:

collectorFiles:
  pricing_data_freshness.collector.yml: "{{ .Files.Get \"my_collector.yaml\" }}"

And it works same as the old one, and your files are also organized better.

[burningalchemist]

Hey @Avihais12344, I'm not sure I follow, but I think it already works the way you imply. sql_exporter has a discovery mechanism to search for external collector files (with globbing if needed) and use them.

As per the docs here:

It's also possible to define collectors (i.e. metrics and queries) in separate files, and specify the filenames in the collector_files list. For that we can use CollectorFiles field (check values.yaml for the available example).

So we can avoid unnecessary templating by using the native functionality, via attaching configmap resources to the designated location.

---

Adding @allanger for visibility.

[Avihais12344]

Hey @burningalchemist, sql_exporter can detect collector files (even as globs), but the files need to be at the k8s environment. I can add a seperate configmap and attach it to the container. But I want to manage everything through that chart. So I think it would be easier for the users if the chart would support templatingbat the chart.

It could also help users so they could use variables and values at the values files themselves, for example, matching the idle connection to the connection, using variables.

So, I don't think it's unecessary.

[allanger]

As I remember, the Files object can be used only with files that are stored in the chart archive, so it's I doubt it can be useful for users, it least it was working this way.

I don't feel like merging all the possible config files to this repo.

[burningalchemist]

@Avihais12344 @allanger

I guess we're referring to this helm topic: https://helm.sh/docs/chart_template_guide/accessing_files/. If I'm reading it right, it implies bundling the specific files in the chart, as @allanger mentioned above. Then I'm confused on how can we use it, when the new chart is released (without any pre-baked custom files, of course). 🤔

Correct me if I'm wrong, but I guess there is no native way to deploy extra yaml files together with the chart out of the box (unless you create some wrapper charts). It's still possible with some extra tooling (which sort of wraps/orchestrates helm and regular manifest deployment, like skaffold or alike), but then it really depends on the user environment, their tools and goals. So I'm not sure we need to solve the problem in the chart itself. 🤔

[Avihais12344]

@allanger:

As I remember, the Files object can be used only with files that are stored in the chart archive, so it's I doubt it can be useful for users, it least it was working this way.

I don't feel like merging all the possible config files to this repo.

You don't need to merge any possibility. A user that wishes to add a config file, just does something like that at the values file:

collectorFiles:
  pricing_data_freshness.collector.yml: "{{ .Files.Get \"my_collector.yaml\" }}"

And the user would also have the file my_collector.yaml at the working directory. And that's it. He can add the file's content as a collector file.

@burningalchemist I think when they say at the docs that:

[...]These files will be bundled.[...]

I think they mean that the content of the file would be part of the final release deployed on K8S (which is what we want). Because in the same paragraph they say:

[...]Be careful, though. Charts must be smaller than 1M because of the storage limitations of Kubernetes objects.

As they talk about the K8S objects that helm creates.

I think it's a small fix, to help many users, like me, who want to not put all the configs at the same values file, or use multiple values files and try to concatenating them together, or use additional tooling. I work in a limited environment, so I can't have all the fancy things in the world.

And even if files are not a strong argument for you, you can use templates to apply other magic for your values. For example, configure this at the values file:

config:
  global:
    # -- Scrape timeout
    scrape_timeout: "{{ .Values.serviceMonitor.interval }}"

So now, the scrape_timeout and the service monitor interval would be the same, and I would only need to change the service monitor interval for it.

In addition to that, I think that a user who wants to deploy a new instance of sql_exporter, just needs the chart, do helm install command, and that's it. They don't need to create other ConfigMaps, or external toolings (outside of kubectl and helm). So, we should make the chart as easy to use as possible, so adding the option to organize your values is a huge step in making the helm chart easy, and a small change from our side.

[allanger]

You don't need to merge any possibility. A user that wishes to add a config file, just does something like that at the values file:

collectorFiles:
pricing_data_freshness.collector.yml: "{{ .Files.Get "my_collector.yaml" }}"

And the user would also have the file my_collector.yaml at the working directory. And that's it. He can add the file's content as a collector file.

Have you tested it with a file that isn't stored in the same dir as the chart?

[Avihais12344]

Okay @allanger, I have tested it, and it doesn't work as I though it would. I am sorry, I am just regular to clone the chart, and work with the chart at it's directory.

So, even if it doesn't work with files outside the dir of the chart, I still think it's necessary. I think it's much easier to clone the chart, and add the files yourself, than using subcharts or deploying configMaps by yourself, especially if we have collector files at the chart already.

[allanger]

Charts are not supposed to be distributed in that way, I'm sorry, but I don't feel like adding some functionality that would mean that we need to shift the responsibility to maintain the chart to users. And it would end up in requests to add more configs to the upstream repo.

I've thought about adding templates to the config to make it possible:

config:
  global:
    # -- Scrape timeout
    scrape_timeout: "{{ .Values.serviceMonitor.interval }}"

But I'm not sure if it makes sense, as there are not so many values that are shared between k8s objects and the exporter config.

But I think it's not up to me to make decisions though

[Avihais12344]

@burningalchemist we may need your help.

[burningalchemist]

Hey folks, I'm out of capacity at the moment to respond quickly. I'll try to allocate some time in the coming days, hope it's ok for you.

[Avihais12344]

@burningalchemist still out of capacity?

[burningalchemist]

Hey @Avihais12344, @allanger,

Thank you for your patience. I've spent some time last week looking into the domain, and I bumped into several discussions (#3276 and the respective PR #10077) which are quite heated. I've read through them and I have a few thoughts on the matter. Overall, the problem of adding arbitrary files to Helm charts is pretty controversial and not solved yet.

If you treat Helm charts as a package manager, then it's not a good idea to allow arbitrary files to be added for many different reasons (including distribution, artifact integrity, security, maintenance, etc). However, if you treat Helm charts as a way to package up Kubernetes resources (in other words, just a template generator), then it's normal to expect arbitrary files to work out of the box. It seems there is no common consensus at the moment, which is an issue.

What we are trying to achieve in this PR doesn't look like a solid solution to me. On one hand it simplifies some steps for a limited group of users, but on another hand it's clearly a hack. In this case, having a sub-chart, despite its suboptimal nature is more common in the community, so it's easier to grasp and apply this practice.

Whenever there is a proper solution in the upstream on how to treat and inject arbitrary files, we should definitely adopt it. Until then, I'm not sure if we should be promoting hacks, as it also puts a burden on the maintainers to support it and the users.

Having said that, one possible alternative I see is to utilize --set-file parameter to pass the file content as a string to the chart. This way, we can avoid adding the file to the chart and still be able to pass the content to the template. It's not perfect, but it's a more idiomatic way to achieve the same result, I assume. I've checked a few projects, and in the absence of a better solution, maintainers adopt this approach if they don't want to deal with sub-charts. One of the snippets can be found here - helm/helm#3276 (comment). This way, we can render a ConfigMap object which would be populated based on the file(s) content provided via the helm parameters and mounted into the container. So, the helm chart remains integral and the user can still provide the file content.

---

On the parameter templating, I don't think it's needed here as well. The number of parameters is small, and it's not a big deal to provide them manually. I'd rather keep the chart simple, flexible, and easy to understand.

[Avihais12344]

I understand the problem with files, thank you for your research. But I disagree about the parameter templating, even if it's small, I still think it's good to support it to make the chart more flexible, and still easy to understand and use.

[burningalchemist]

Hey @Avihais12344, well, I don't want to be a blocker if you really think it provides the value and flexibility. Let's try it out then. :)

My only change request would be to mark that config parameter in docs as optional (so we have the compatibility) and remove the Files.Get mentions as I don't want to promote this practice in this project. If it works and advanced users can use these methods to configure their deployments - it's fine for everyone I guess. Maybe, we also need to bump up the minor version (to 0.11) given that we add a new chart parameter.

What I'd like to avoid personally is multiple Github issues (helm-related in this case), which oftentimes are mistakenly marked as bugs, though users just cannot configure their stuff or debug based on the error messages. It's pretty time-consuming and I don't have this time sadly and I don't want to use much of @allanger's time as well, so I tend to make it simple.

[Avihais12344]

@burningalchemist Sorry for the delay, I have done as you say. But I do not understand why make the config parameter optional?

[burningalchemist]

@Avihais12344 No worries at all, it's a holiday season. 👍 I only meant that to make sure that the previous configuration approach is still applicable, but seemed to introduce confusion. 🙂 If you think nothing really changes, we can remove that note. Configuration cannot be optional, of course. 👍

[Avihais12344]

@burningalchemist Thank you very much. I have removed the note about the optional config. And of course the old way still works, so no need for notes.

I am done now.

[burningalchemist]

Thank you too! 👍 I'm waiting for the checks to pass, and merge it.