fix(charts): mongodb persistence + ingressClassName support and configuration improvements suggestion #273

Grraahaam · 2023-01-29T19:14:06Z

Description 📣

fix

Replaced metadata.annotations.kubernetes.io/ingress.class: "nginx" with the recommended spec.ingressClassName in the ingress resource (details here and there)
Simplified value paths in templates (e.g. .Values.backend to $backend) reducing the paths length

feat

MongoDB storage persistence by using PVC and linking the database storage to it

refacto

Refactored the values.yaml and the way we pass configuration variables to our pods by preferring native k8s secrets, and using envFrom

docs

Added more detailed installation instructions

I admit that this PR might contain a lot of different changes, but all related to our helm-charts. Ping me if you rather want me to split those changes into dedicated PRs

Type ✨


Bug fix	❌
New feature	✅
Breaking change	✅
Documentation	✅

Tests 🛠️

Helm

# Infisical
cd ./helm-charts/infisical
helm lint
helm template infisical .

# Infisical Secrets Operator
cd ./helm-charts/secrets-operator
helm lint
helm template secrets-operator .

I have read the contributing guide, agreed and acknowledged the code of conduct. 📝

Grraahaam · 2023-01-29T20:19:15Z

Another PR in progress to easily setup a local k8s development environment 👷🏽

maidul98 · 2023-01-31T00:07:03Z

Hey @Grraahaam I'll review this soon!

maidul98 · 2023-01-31T00:18:23Z

helm-charts/infisical/templates/backend-deployment.yaml

-        image: "{{ .Values.backend.image.repository }}:{{ .Values.backend.image.tag | default .Chart.AppVersion }}"
-        imagePullPolicy: {{ .Values.backend.image.pullPolicy }}
+      - name: {{ template "infisical.name" . }}-{{ $backend.name }}
+        image: "{{ $backend.image.repository }}:{{ $backend.image.tag | default .Chart.AppVersion }}"


I'm wondering if we should get rid of the default .Chart.AppVersion.Because we would never have a image tag that is the .Chart.AppVersion. Those aren't related. What do you think

Yeah, since the infisical chart contains several services it doesnt really make sense here ✌🏽

It could be like this instead, wdyt?

Suggested change

image: "{{ $backend.image.repository }}:{{ $backend.image.tag | default .Chart.AppVersion }}"

image: "{{ $backend.image.repository }}:{{ $backend.image.tag | default 'latest' }}"

Yes, perfect. Let's do the same for frontend as well

Fixed in 3a11898

maidul98 · 2023-01-31T00:20:41Z

helm-charts/infisical/templates/backend-deployment.yaml

        {{- end }}
        env:
        - name: MONGO_URL
          value: {{ include "infisical.mongodb.connectionString" . | quote }}
-        {{- if .Values.backendEnvironmentVariables }}
-        {{- range $key, $value := .Values.backendEnvironmentVariables }}


is there a reason why you deleted the ability to write envs right into the deployment file? I'm gussing because it is more secure and can be stored in kubeSecretRef? Or were you thinking or something else

Oh i see, you put them in a secret below

is there a reason why you deleted the ability to write envs right into the deployment file?

Storing secrets in a dedicated resource is a better alternative IMO, it allows you to fine-grain the access and do much more than if it was directly defined in the deployment, see https://kubernetes.io/docs/concepts/security/secrets-good-practices/ for best practices ✌️

I agree, this is better approach

I think the variable backendEnvironmentVariables was completely removed. This will break existing installations.

I think the variable backendEnvironmentVariables was completely removed. This will break existing installations.

Yes, IMO it makes more sense to define those variables under the backend (and frontend as well) key, since those are required for each service. That's why I've specified "Breaking change" in my PR description.

Since we're still in early stages, breaking changes won't impact a lot of people so far, and they'll still be able to use the previous chart version if ever they don't want to break their current installation, right?

I think it's best to define a good structure now and avoid supporting deprecated keys at this stage, what do you think?

I can still add support for backendEnvironmentVariables if ever we want to avoid breaking existing installations ✌️

well, it's up to @maidul98 if you want to keep this non breaking or not. I just wanted to point it out 😅

maidul98 · 2023-01-31T00:27:34Z

helm-charts/infisical/templates/backend-deployment.yaml

+  name: {{ $backend.kubeSecretRef | default "infisical-backend" }}
+type: Opaque
+data:
+  {{- range $key, $value := $backend.secrets }}


I'm curious, let's say I removed some secrets from the values file for backend. Would upgrading the helm chart also refect a one to one to this secret? Or do old values that no longer exist in the values file continue to exist but only only overwritten

Defined secrets in values.yaml are the one deployed in k8s. If you remove one from the values file, it'll remove it from the corresponding secret k8s resource as well ✌️ (just did the test locally to be sure)

maidul98 · 2023-01-31T00:41:03Z

helm-charts/infisical/templates/frontend-deployment.yaml

        readinessProbe:
          httpGet:
            path: /
            port: 3000
          initialDelaySeconds: 10
          periodSeconds: 10
-        {{- if .Values.frontend.kubeSecretRef }}
+        {{- if $frontend.kubeSecretRef }}


It looks like the user has to define a kubeSecretRef + define the environment variables in values.frontend.secrets ? I feel like if they are defining values.frontend.secrets then we should just put those in a secret and auto reference them. Thoughts?

Good catch, my bad! Here's my suggestion (to be applied on front/backend)

# For the deployment ... envFrom: - secretRef: name: {{ $backend.kubeSecretRef | default "infisical-backend" }} ...

# For the secrets {{ if not $backend.kubeSecretRef }} apiVersion: v1 kind: Secret ... {{- end }}

# For the value file (reduce the `kubeSecretRef` indentation) ... backend: name: backend ... # Provide your own or leave it empty for default kubeSecretRef: "my-own-secrets"

Fixed in 8a66561

maidul98 · 2023-01-31T01:01:35Z

helm-charts/secrets-operator/templates/deployment.yaml

@@ -1,3 +1,4 @@
+{{- $ctrlManager := .Values.controllerManager }}


All changes for the secrets-operator helm chart gets auto generated by Kubebuilder. This means that these changes will be overwritten on next release. So feel free to remove or leave it as is

maidul98 · 2023-01-31T04:37:45Z

helm-charts/infisical/templates/frontend-deployment.yaml

+data:
+  {{- range $key, $value := $frontend.secrets }}
+  {{ $key }}: {{ $value | b64enc }}


I think we should wrap the value in quotes before b64enc otherwise we get

rror: UPGRADE FAILED: template: infisical/templates/frontend-deployment.yaml:76:26: executing "infisical/templates/frontend-deployment.yaml" at <b64enc>: wrong type for value; expected string; got float64

The alternative is peple have to put quotes around every value on their own

Fixed in 3ac9f98

What value did you try (redacted if needed)? I've never encountered this issue in my tests 👀

Just when you add booleans or numbers as values directly

Indeed, I was able to reproduce your issue! And the above fix patched it

maidul98 · 2023-01-31T04:41:13Z

Everything else looks great! I was able to run it on digital ocean. However, when i did

helm upgrade infisical .  --values /Users/maidulislam/Desktop/k8-values/dev/values.yaml --recreate-pods

I expected the data in the volume to presist, but reloading my page seems like the data was lost. Fyi this is on Digital ocean (I was able to see the PVC there so it definitely got created)

Grraahaam · 2023-01-31T17:23:00Z

helm upgrade infisical .  --values /Users/maidulislam/Desktop/k8-values/dev/values.yaml --recreate-pods

Ow, what Helm version are you running, helm version --short? 🤔 (Since --recreate-pods have been deprecated since Helm3, explained here)

I expected the data in the volume to persist, but reloading my page seems like the data was lost. Fyi this is on Digital ocean (I was able to see the PVC there so it definitely got created)

I've checked locally, and it works, here's the steps to reproduce what I did :

# Define variables (edit the values according your needs)
values="values.yaml"
select="{}, { email: 1, firstName: 1, lastName: 1 }"
insert="{
  email: '[email protected]',
  firstName: 'john',
  lastName: 'doe',
  publicKey: 'xxx',
  encryptedPrivateKey: 'xxx',
  iv: 'xxx',
  tag: 'xxx',
  salt: 'xxx',
  verifier: 'xxx',
  refreshVersion: 0,
}"

cd ./helm-charts/infisical

# install the infisical chart
helm upgrade --install --create-namespace -n infisical \
  -f $values --wait \
  infisical .

# get the mongo pod name/id
mongo_pod=$(kubectl get pod -n infisical -l component=mongodb -o jsonpath="{.items[0].metadata.name}")

# inserting a fake user
kubectl exec -i -n infisical $mongo_pod -- \
  mongosh -u root -p root --quiet --eval \
  "db.users.insertOne($insert)"

# check if it exists
kubectl exec -i -n infisical $mongo_pod -- \
  mongosh -u root -p root --quiet --eval \
  "db.users.find($select)"

# delete the pod
kubectl delete pod -n infisical $mongo_pod

# get the new mongo pod name/id
mongo_pod=$(kubectl get pod -n infisical -l component=mongodb -o jsonpath="{.items[0].metadata.name}")

# should still be able to fetch the above created fake user
kubectl exec -i -n infisical $mongo_pod -- \
  mongosh -u root -p root --quiet --eval \
  "db.users.find($select)"

jon4hz · 2023-01-31T20:29:13Z

helm-charts/infisical/templates/backend-deployment.yaml

+apiVersion: v1
+kind: Secret
+metadata:
+  name: {{ $backend.kubeSecretRef | default "infisical-backend" }}


Shouldn't the name default to include "infisical.backend.fullname" .? Otherwise multiple helm install without customization will have colliding names.

Good catch! Fixed in cf95466

jon4hz · 2023-01-31T20:31:54Z

helm-charts/infisical/templates/frontend-deployment.yaml

+kind: Secret
+metadata:
+  name: {{ $frontend.kubeSecretRef | default "infisical-frontend" }}


Same as for the backend secret. The default should probably be include "infisical.frontend.fullname" .

Good catch! Fixed in cf95466

jon4hz · 2023-01-31T20:41:13Z

While I appreciate the effort regarding the mongodb persistent, I suggest to refactor that part completely and implement #182 instead. Bitnamis mongodb chart is extremely good and offers much more flexibility.

If the bitnami mongodb chart is is on infisicals roadmap anyway, it would be easier to implement that now. Currently people probably don't use the mongo part of this chart due to the missing persistence. But if that changes, it will be much harder to switch later.

Grraahaam · 2023-02-01T00:28:23Z

Thanks @jon4hz ! Indeed, the current mongodb persistence was mostly a POC and I was already looking at Bitnami's MongoDB chart, I'll probably try to implement it here then (as a customizable dependency) , this way we'll benefit of a well-tested and maintained chart!

Once it'll be done, both #173 and #182 will be solved ✌️

zifeo · 2023-02-02T21:03:37Z

helm-charts/infisical/templates/mongodb-deployment.yaml

@@ -1,3 +1,4 @@
+{{- $mongodb := .Values.mongodb }}
 apiVersion: apps/v1


Can we make this deployment optional (in case we want to use an external database)?

I think what we can do is if externalMongoDBConnectionString is defined then no need to spin this deployment up. What do you think @Grraahaam

@zifeo @maidul98 As mentioned above, I'm re-structuring the chart to follow best practices (e.g. sub-charts, referencing), it'll allow you to fine-grain what service you want to enable (e.g. you'll be able to run only front/back if you need)

I'll try to do this without adding too many breaking changes, but it'll likely breaks existing installation at some point... But we'll still be able to point to the previous chart through its version ✌🏽

I think what we can do is if externalMongoDBConnectionString is defined then no need to spin this deployment up. What do you think @Grraahaam

It might be something like this, pretty similar in the idea I guess !

Grraahaam · 2023-02-27T08:10:19Z

Partly replaced by #314 and #353

I'll open a new PR for the Helm chart structure improvements ✌️

maidul98 reviewed Jan 31, 2023

View reviewed changes

Grraahaam added 10 commits January 31, 2023 09:27

fix(chart): truncated secrets-operator resource name

1eb4378

fix(chart): shorten names with variables + improved secrets support

fb5f6cc

fix(chart): ingress shorten names with variables + ingressClassName

80e6167

feat(chart): added persistence for mongodb + improved secrets support

374447c

fix(chart): updated default values file structure

e0141ab

chore(chart): breaking change version bump + docs

352b55d

fix(chart): uses envFrom only to inject secrets and conf

da60b35

fix(chart): kubeSecretRef disables and overwrites default secrets

02d2d94

fix(chart): image tags default to latest

0519240

fix(chart): wrap secret values into quotes automatically

3ac9f98

jon4hz reviewed Jan 31, 2023

View reviewed changes

fix(chart): default secret name to the service fullname

cf95466

zifeo reviewed Feb 2, 2023

View reviewed changes

Grraahaam mentioned this pull request Feb 8, 2023

feat(chart): mongodb persistence #314

Merged

5 tasks

Grraahaam marked this pull request as draft February 15, 2023 08:57

Grraahaam closed this Feb 27, 2023

maidul98 mentioned this pull request Feb 25, 2024

[Snyk] Upgrade @fastify/cookie from 9.2.0 to 9.3.1 #1465

Merged

	image: "{{ $backend.image.repository }}:{{ $backend.image.tag \| default .Chart.AppVersion }}"
	image: "{{ $backend.image.repository }}:{{ $backend.image.tag \| default 'latest' }}"

		@@ -1,3 +1,4 @@
		{{- $ctrlManager := .Values.controllerManager }}

		@@ -1,3 +1,4 @@
		{{- $mongodb := .Values.mongodb }}
		apiVersion: apps/v1

fix(charts): mongodb persistence + ingressClassName support and configuration improvements suggestion #273

fix(charts): mongodb persistence + ingressClassName support and configuration improvements suggestion #273

Conversation

Grraahaam commented Jan 29, 2023 • edited Loading

Description 📣

Type ✨

Tests 🛠️

Helm

Grraahaam commented Jan 29, 2023

maidul98 commented Jan 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maidul98 Jan 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Grraahaam Jan 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maidul98 Jan 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maidul98 Jan 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maidul98 commented Jan 31, 2023

Grraahaam commented Jan 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jon4hz commented Jan 31, 2023

Grraahaam commented Feb 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Grraahaam commented Feb 27, 2023

Grraahaam commented Jan 29, 2023 •

edited

Loading

maidul98 Jan 31, 2023 •

edited

Loading

Grraahaam Jan 31, 2023 •

edited

Loading

maidul98 Jan 31, 2023 •

edited

Loading

maidul98 Jan 31, 2023 •

edited

Loading