Add existing secrets to helm chart

[lhaussknecht]

This PR adds the possibility to use an existing kubernetes secret when installing sql-exporter with the helm chart.

Related to #396

[burningalchemist]

I've created a similar documentation at artifacthub.io - https://artifacthub.io/packages/helm/sql-exporter/sql-exporter#installing-the-chart. Let's put similar values everywhere just for consistency. There's a small typo, besides. 👍

[burningalchemist]

Just a small reminder to address this little issue here. 👍

[burningalchemist]

I'd put existingSecret under the config section for better context, but I have no strong opinion here.

@allanger Do you mind checking the PR as well whenever you have some time? 🙂

[burningalchemist]

Also, I've added helm-docs to generate README.md for chart values, and it's sensitive to the comments format (using them for description). So if you could remove delimiters (line 67 and 69), that'd be cool.

Actually, if you could run helm-docs --sort-values-order file command (from here) to regenerate README.md, that'd be amazing, so we could have a smooth release.

I'll add the notes to the contribution guide later. 🙂👍

[allanger]

@burningalchemist I'll check tomorrow

[allanger]

I also think that config related values should be under the config key. But the problem then is that currently we add the whole config object to the secret, I'd suggest to do something like this

config:
  existingSecret: SECRETNAME
  inline: 
    global: ...
    ...

And then, if Values.config.existingSecret is not empty, we mount the existing one, otherwise, we put Values.config.inline to the templated secret

[allanger]

And it also would mean the major version bump, because it's a breaking change

[burningalchemist]

I'm not concerned about breaking changes since we're under 0.x (zerover) so far, so breaking changes are intended, expected, but also should be announced. So we'd bump up the minor as usual.

I don't really like inline (although I understand the intention), let me think about it. 😉

[allanger]

Also we can do it like

config:
  from:
    kind:
    name:
    key:
  value: |
    ...

like here #399 (comment)

[burningalchemist]

Oh, this one looks good. 👍

[allanger]

I'm not sure that value: | is a good idea, I guess it's better to handle value as an object in this case, and cast it to yaml in the template already, then users will be able to replace parts of it.

I'd say that it should be

config:
  from:
    kind:
    name:
    key:
  value: {}
  # global: ...
    ...

And then in the template it still can be {{ toYaml .Values.config.value }}

[burningalchemist]

Fair enough, I was mostly looking at the structure. But yeah, agreed. 👍

[burningalchemist]

Hey @lhaussknecht, thank you for the contribution! 👍 I've left several comments above, but in general it's all good. We just need to agree on the existingSecret location in the values.yaml.

[burningalchemist]

Hey @lhaussknecht, could you please rebase the PR on the latest master whenever you're available? 🙂

[lhaussknecht]

Hey @burningalchemist . Will work on this today.

[allanger]

Hi, To me it looks like Values.config.value must have the highest priority, because it's something coming with a package, and external source should be used when the value is not provided.

if config.value 
  then secret from the chart
else 
  if kind == secret 
    use external secret 
  else if kind == configmap
    use external configmap

And then the same applies to the secret.configuration.yaml, it's created if value is provided.

Or don't you think so?

[lhaussknecht]

Yes, good idea to look at config.value instead of config.from. But how do we override values when using an existing secret?

[allanger]

Hm, I guess you can set it like this:

config:
  from: 
   kind: ...
  values: {}

But it doesn't look nice and user-friendly to me, so yeah, maybe it makes more sense to check for the from.kind in that case.
Maybe @burningalchemist has an idea about that

[lhaussknecht]

That's a shortcoming of Helm. You cannot override values with NULL.

[allanger]

Ok, you can do it with null I guess, not {}, but I still don't like it

I've checked like this:

# template 
apiVersion: v1
kind: Service
metadata:
  {{- if .Values.check }}
  name: check
  {{- else }}
  name: not-check
  {{- end }}
  labels:
    {{- include "test.labels" . | nindent 4 }}
spec:
  type: ClusterIP
  ports:
    - port: http
      targetPort: http
      protocol: TCP
      name: http
  selector:
    {{- include "test.selectorLabels" . | nindent 4 }}

default values:

check: 
  check: check:

$  helm template .
---
# Source: test/templates/service.yaml
apiVersion: v1
kind: Service
metadata:
  name: check
  labels:
    helm.sh/chart: test-0.1.0
    app.kubernetes.io/name: test
    app.kubernetes.io/instance: release-name
    app.kubernetes.io/version: "1.16.0"
    app.kubernetes.io/managed-by: Helm
spec:
  type: ClusterIP
  ports:
    - port: http
      targetPort: http
      protocol: TCP
      name: http
  selector:
    app.kubernetes.io/name: test
    app.kubernetes.io/instance: release-name

another value file: values-new.yaml

$ cat values-new.yaml
check: null
$ helm template . -f values-new.yaml
---
# Source: test/templates/service.yaml
apiVersion: v1
kind: Service
metadata:
  name: not-check
  labels:
    helm.sh/chart: test-0.1.0
    app.kubernetes.io/name: test
    app.kubernetes.io/instance: release-name
    app.kubernetes.io/version: "1.16.0"
    app.kubernetes.io/managed-by: Helm
spec:
  type: ClusterIP
  ports:
    - port: http
      targetPort: http
      protocol: TCP
      name: http
  selector:
    app.kubernetes.io/name: test
    app.kubernetes.io/instance: release-name

[allanger]

So it either should be a _helper function, or we just leave it like that. And though I've suggested a change, after thinking about it, I guess, that it's better if your solution stays.

[burningalchemist]

Hey folks, I'll have a look tomorrow as well. 🙂👍

[burningalchemist]

Hey gents, apologies, I was a bit sick last week.

[burningalchemist]

Hey folks! I'd say, let's keep it as it is now (it can be a single comment in the end to clarify the precedence). Or we can get rid of config.value items completely for the time being.

The reason is there's a plan to implement a feature to have environment variables for config values (ideally, similar to how Grafana does it). This way, we address the precedence naturally: 'environment variables > config file'. It also might be easier to define parameters in the Helm template.

I'm ok with either. 🙂👍

[allanger]

Should I take care of it?

[burningalchemist]

Hey @allanger, if you have some time, could you please? 👍

Just pinging @lhaussknecht for visibility. 😃

[lhaussknecht]

Just arrived from holidays... @allanger are you already on it? I could work on it tomorrow.

[allanger]

Just arrived from holidays... @allanger are you already on it? I could work on it tomorrow.

Nope, not yet :)

[burningalchemist]

hey @lhaussknecht, looks like the template is breaking the test with the following error:

ts=2024-01-10T12:55:48.711Z caller=klog.go:147 level=error func=Fatalf
msg="Error creating exporter: exactly one of `jobs` and `target` must be defined"

I guess we might need to update the test config as well.

[burningalchemist]

Hey @lhaussknecht, would you find some time in the upcoming weeks to figure out the remaining issues? 🙂 There are small things left, so we're close to merge the PR. 😉

[burningalchemist]

Hey @allanger, do you mind actually taking care of this PR? I guess we can check the PR out and recreate it under another request, so all the pushed commits are preserved.

If you don't have any capacity, I can take care myself, so just asking. 👍

[allanger]

@burningalchemist I'll be afk for about a week, and then I can take care of it.

[burningalchemist]

@allanger sounds good to me, let's see if there's any progress while you're away. 👍

[allanger]

So, I'm back, will try to fix it by the end of the week

[allanger]

@burningalchemist I can't understand one thing. Is there really a difference between this PR and this one #432?

To me, it looks like in both cases, we're just mounting secrets/configmaps to the sql_exported pod. But since I'm not using the tool atm, I'm not sure if intentions are the same.

I would basically just close them both by letting secret/configmap mount

[burningalchemist]

Hey @allanger, I guess the last sentence seems cut. 🤔 But I assume #432 wants to have a generic interface to mount arbitrary files to the container, whereas this PR wants a more specific parameter which just takes the secret name to be mounted. On the functional side, I think they perform the same operation, the goal for each might be different.

I'm curious to hear your idea. And thanks for looking into it. 👍

[allanger]

I think I would just let mount everything like that:

# values
mounts:
  - kind: Secret
    name: sql-exporter-config
    path: /etc/sql_exporter/
  - kind: ConfigMap
    name: big-json-config
    path: /etc/big-config

Then the whole configuration can be done with external secrets, and the templates/secret.configuration.yaml can become optional. Only if config is set explicitly in values, it should be created.

UPD: But I also need to think about environment variables, I guess it might make sense to mount whole secrets/configmaps as well, when they are created for sql_exporter env only

[burningalchemist]

@allanger Really I'd just make it as simple as possible, we can always tighten the interface later if it's too much confusion, but what you've described makes sense.

Users want to mount files next to the binary, be it the configuration file or any complimentary file.

At this stage, I think it's easier to explain that the configuration file needs to be prepared in advance (as a secret) and mounted respectively (like in your example snippet). It also implies that the config file's lifecycle is separate from the helm chart, which sounds pretty pragmatic. 👍

[burningalchemist]

Closed in favour of #446.