-
-
Notifications
You must be signed in to change notification settings - Fork 60
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add existing secrets to helm chart #397
Add existing secrets to helm chart #397
Conversation
helm repo add burningalchemist https://burningalchemist.github.io | ||
helm intall burningalchemist/sql-exporter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a small reminder to address this little issue here. 👍
helm/values.yaml
Outdated
# --------------------------------------------------------------------- | ||
# -- Option to provide an existing secret with all the values from the config section | ||
# --------------------------------------------------------------------- | ||
existingSecret: {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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? 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. 🙂👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@burningalchemist I'll check tomorrow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And it also would mean the major version bump, because it's a breaking change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, this one looks good. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 }}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough, I was mostly looking at the structure. But yeah, agreed. 👍
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 |
Hey @lhaussknecht, could you please rebase the PR on the latest master whenever you're available? 🙂 |
Hey @burningalchemist . Will work on this today. |
0bf0ec1
to
009f8ec
Compare
items: | ||
- key: {{ .Values.config.from.key }} | ||
path: "sql_exporter.yml" | ||
{{- else }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, good idea to look at config.value
instead of config.from
. But how do we override values when using an existing secret?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a shortcoming of Helm. You cannot override values with NULL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Hey folks, I'll have a look tomorrow as well. 🙂👍 |
Hey gents, apologies, I was a bit sick last week. |
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 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. 🙂👍 |
Should I take care of it? |
Hey @allanger, if you have some time, could you please? 👍 Just pinging @lhaussknecht for visibility. 😃 |
Just arrived from holidays... @allanger are you already on it? I could work on it tomorrow. |
Nope, not yet :) |
…tion such as connection strings. Add hint in README.md to install the chart.
96e9454
to
5e89203
Compare
hey @lhaussknecht, looks like the template is breaking the test with the following error:
I guess we might need to update the test config as well. |
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. 😉 |
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. 👍 |
@burningalchemist I'll be afk for about a week, and then I can take care of it. |
@allanger sounds good to me, let's see if there's any progress while you're away. 👍 |
So, I'm back, will try to fix it by the end of the week |
@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 |
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. 👍 |
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 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 |
@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. 👍 |
Closed in favour of #446. |
This PR adds the possibility to use an existing kubernetes secret when installing sql-exporter with the helm chart.
Related to #396