Use values from secrets env #67
Conversation
rahulinux
force-pushed
the
storage-controller-add-secrets
branch
from
3814836
to
46f6048
Compare
rahulinux
force-pushed
the
storage-controller-add-secrets
branch
from
46f6048
to
cf2b678
Compare
|
@jcsp |
Updated new config after confirming with John. |
rahulinux
force-pushed
the
storage-controller-add-secrets
branch
from
cf2b678
to
0cdbc98
Compare
| secretKeyRef: | ||
| name: {{ include "neon-storage-controller.fullname" . }} | ||
| key: DATABASE_URL | ||
| - name: JWT_TOKEN |
There was a problem hiding this comment.
I think, we usually put secret key mounted somewhere and pass into the app location of that file.
Passing quite large key in env variable sounds strange to me
There was a problem hiding this comment.
Ok, JWT tokens are not actually that big, so this may be OK
| {{- end }} | ||
| {{- if .Values.settings.publicKey }} | ||
| - --database-url $(DATABASE_URL) | ||
| - --jwt-token $(JWT_TOKEN) |
There was a problem hiding this comment.
What does the $(JWT_TOKEN) tries to say?
It's executing binary with the name JWT_TOKEN currently.
But ${JWT_TOKEN} also wrong as IIRC it doesn't have that envs in the context of evaluation of command line
There was a problem hiding this comment.
If you want to hide this secret from command line, binary itself should load this from well-known env variables, and not trying to inject into command line
There was a problem hiding this comment.
env sets from secrets and it will pass to cmd args$(JWT_TOKEN) and yes it would be nice if controller support reading from config file. https://stackoverflow.com/a/50248608
@jcsp can we pass config file?
There was a problem hiding this comment.
env sets from secrets and it will pass to cmd args$(JWT_TOKEN) and yes it would be nice if controller support reading from config file. https://stackoverflow.com/a/50248608
Ok, then it is probably fine. But I never used this syntax, so you are on your own here.
| @@ -47,21 +47,10 @@ spec: | |||
| - -l | |||
| # In the container, use the same port as service. | |||
| - 0.0.0.0:{{ .Values.service.port }} | |||
| {{- if .Values.settings.databaseUrl }} | |||
There was a problem hiding this comment.
Also, you removed this condition, meaning flag always set.
Which means AWS secret manager is never used, right?
Then this comment become incorrect: https://github.com/neondatabase/helm-charts/blob/main/charts/neon-storage-controller/values.yaml#L27
There was a problem hiding this comment.
it shouldn't use secrets from AWS manager, we will be moving to GitOps with fluxcd where all secrets are in sops, also we use sops in terraform. secrets in AWS secrets can be change from UI easily which can create issue, using secrets (sops) in git is safe and single source of truth.
There was a problem hiding this comment.
Update values.yaml then, please
There was a problem hiding this comment.
I'll leave that to @jcsp
rahulinux
force-pushed
the
storage-controller-add-secrets
branch
3 times, most recently
from
389610f
to
414bfcc
Compare
There was a problem hiding this comment.
This sounds a lot like functionality that should not be a concern of packaging (helm) but app itself. This probably needs maintenance in the future, so it probably should be tested too, so it would be much more natural for that to live with the app in app repo - because that's where the development happens - rather than helm repo.
If the logic isn't built in with the app and has to be a separate script, could use initContainer for that for instance.
There was a problem hiding this comment.
Yes, this is temporary from script and once there is functionality then we can remove this script. also the reason I used post-install job because lifecycle.postStart doesn't have any retry mechanism. this script is copy of existing init script from ansble playbook and tested on dev.
rahulinux
force-pushed
the
storage-controller-add-secrets
branch
6 times, most recently
from
bc4e3f2
to
2f01383
Compare
|
@lassizci @jcsp I'm going to merge this to test helm chart on CI I'll open another PR if there is any change needed. |
rahulinux
force-pushed
the
storage-controller-add-secrets
branch
from
2f01383
to
8380bba
Compare
Currently secrets used in command args and it's visible in deployment. this change will read take values from secrets and sets in env variable.
Once this is merge then we can test deployment with this change. https://github.com/neondatabase/aws/pull/1070