New production ready Helm chart

[Nathanael-Mtd]

After discussions in #1216, I made a new "next" Helm Chart with more options to have more flexibility in Kubernetes deployments.

Many changes in comparaison of the current chart, need some reviews.

Current features progress in this new chart:

• SQLite mode
• PostgreSQL mode
• Backend and frontend separation in different pods
• Ingress with custom TLS certificate option
• SecurityContexts (disabled by default)
• Change SQLite database PVC and file directory (env : SQLITE_FILE)
• Use dedicated PVC for local storage directory (env : LOCAL_STORAGE_DIRECTORY)
• Helm chart README.md generation from values.yaml file (using https://github.com/norwoodj/helm-docs)
• Add Liveness/Readiness check (not possible right now, needs to be implemented in CISO Assistant)
• Add initContainer on backend to wait when the database is ready before starting backend (delayed to a future PR)
• Automate Helm chart release when a new tag is created
• Other ideas ?

[eric-intuitem]

Definitely it is a good idea to use non-root user in the containers. I'll provide a PR for that.

[Nathanael-Mtd]

Definitely it is a good idea to use non-root user in the containers. I'll provide a PR for that.

Great ! Also, read-only filesystem can be nice to use.
When I tried to run with that option enabled, I got some issues with Matplotlib cache, but maybe some other things use default cache/temp folders located on homedir or code directory.

[Nathanael-Mtd]

Another thing I thinked about, that's using a GitHub Action to bump chart version and change appVersion when a new tag is made. Like that, we don't need to set manually the image tag version when upgrading.

I don't know too much about GH Actions but I found that : https://github.com/shini4i/helm-charts-updater

[Nathanael-Mtd]

@ab-smith @eric-intuitem Chart seems done. Need a review (and some tests on your side if possible).

I didn't added the GitHub Actions to generate and publish the chart, if you can check to add it, it can be nice (see the previous comment)
If you add GH Actions for release, can be a good idea to add a an action to fail the workflow if there are a change in values.yaml and the comitter forgot to launch helm-docs command to generate a new README.md.
Maybe instead it can be used to update automatically the README.md when a new tag is created in the same time of the GH action chart version bump and release.

[ab-smith]

we'll need to explicitly mention the helm dep build or something equivalent;
the helm-docs will pick it up on the dependencies but people might skip it.

@Nathanael-Mtd : how is it supposed to behave? user should install pg using specific values/settings first or it will be installed automatically as a dependency?

side note: helm has an annoying limitation, that can interfere with this, on helm dependency build where it stops if other non related repos are broken.

[ab-smith]

[Nathanael-Mtd]

we'll need to explicitly mention the helm dep build or something equivalent;

the helm-docs will pick it up on the dependencies but people might skip it.

side note: helm has an annoying limitation, that can interfere with this, on helm dependency build where it stops if other non related repos are broken.

Yeah but that's only if people wants to install helm chart from sources, mainly for dev purposes.

We should add the regular install guide on readme.md by using helm chart repo, and a part for install from sources.

[Nathanael-Mtd]

Oops, I will check later, maybe a wrong condition

[Nathanael-Mtd]

@ab-smith Warning fixed. I added a templating file to add custom instructions to install the chart from official helm chart repo (I supposed it will be ghcr.io/intuitem/ciso-assistant).

To test the build and push of the helm chart, you can use these commands :

helm dependency build charts/ciso-assistant-next
helm package charts/ciso-assistant-next
helm push ciso-assistant-X.Y.Z.tgz oci://ghcr.io/intuitem

But I think you can find an existing GH Action which can handle the release with these steps.

[ab-smith]

Options to explore:
https://github.com/helm/chart-releaser
https://github.com/appany/helm-oci-chart-releaser

[ab-smith]

candidate for 2.0.5 or 2.0.6

[Nathanael-Mtd]

One question, do I need to disable security contexts by default due to dockerfile app user commit revert ?

[ab-smith]

If we manage to bring back the non-root user properly before 2.0.6, no, otherwise yes and I’ll tag you accordingly ;)

[ab-smith]

Hello @Nathanael-Mtd yes let's disable k8s security context for this one and work on it on the next releases.
I'll merge it once done to have the code base on the branch to start working on the CI part.

[jgournet]

Hey there,
I'm looking forward for this PR, as this looks very good.

In the meantime, I tried to dumbly deploy it with this value file:

global:
  domain: ciso
backend:
    databaseType: externalPgsql
ingress:
  enabled: false
externalPgsql:
  host: "shared-postgres.mydomain"
  password: "XXXX"

That gave me 2 errors:

1. backend complained that there was no /tmp. I fixed it by adding a emptydir manually; but the chart might want to expose this ?
2. Then Django complained about the lack of a SECRET_KEY. Adding one into the ENV worked, but this is a bit surprising as the old chart did not need this. Might be worth checking ? (I had a very brief look at the code, and it should create one if none is found ?? weird)

Anyway, thank you in advance !

[Nathanael-Mtd]

@jgournet Thanks for the feedback and test, I will check tomorrow.
About the the /tmp, probably a good idea to add it if needed by CISO Assistant.
SECRET_KEY is probably needed only when using pgSQL instead of SQLite.
I didn't really made tests in pgSQL during helm chart development, I will do to check if everything works in all modes

[Nathanael-Mtd]

@jgournet I made the changes, if you want to test it !

[Nathanael-Mtd]

@ab-smith I added the idea to add liveness/readiness checks on chart, but I will not add now. It needs to be implemented in backend/frontend as HTTP endpoints.
It can be great to make backend ready when migrations and other steps are completed, because these steps seems slow to end.

[jgournet]

@Nathanael-Mtd : thank you ! those 2 issues have now been resolved !

However, I still have one issue though ( that I did not report previously, as I guess it's not fully related to this new helm chart):

After install, I set a new superuser email+pass (via poetry run python manage.py createsuperuser ).
When trying to login with it, I get a 403 with response:

{"message":"Cross-site POST form submissions are forbidden"}

Either I did something wrong, or it might be related to the split of backend/frontend to separate pods ?

Here's my values files:

cat ../custom-values.yaml 
global:
  domain: ciso.MY_DOMAIN
backend:
  config:
    databaseType: externalPgsql
ingress:
  enabled: false
externalPgsql:
  host: "shared-postgres.SOME_DOMAIN"
  password: "XXXXX"

Note: we don't use an ingress as we have istio, so we build our own ... that could be related too

[Nathanael-Mtd]

@jgournet You use the same domain name as the one set in global.domain value, for the backend and frontend ?
If yes, maybe you should enable django debug (backend.config.djangoDebug to true) and check headers to confirm that the backend recieve the right domain name.

PS : maybe it's an copy paste error, but the config: part of backend.config.databaseType you sent in your custom values is missing

[jgournet]

@Nathanael-Mtd : Good news, you can ignore this error
I was just missing "ingress.tls: true" as we use https - my bad for overlooking it, since I set "ingress.enabled: false", I just skipped the configuration in there.
(and yes, I did miss the "backend.config.databaseType" when copy/pasting :) )

=> it's all good for me now !

Just one last question:
the current helm chart creates a PVC for "localStorage" by default. I tried to disable it (it's probably a bad idea ? I don't know what it's used for) with:

backend:
  persistence:
    localStorage:
      enabled: false

but I got:

Error: INSTALLATION FAILED: 1 error occurred:
        * Deployment.apps "ciso-assistant-backend" is invalid: spec.template.spec.containers[0].volumeMounts[0].name: Not found: "tmp-data"

Reading the helm chart, it seems that the tmp-data volume is created only when there is some persistence enabled, but we always try to mount it.
=> might be worth creating it all the time ?

[Nathanael-Mtd]

@jgournet Oh wait, we should not ignore that error, because https need to be set on URLs if you want to use it but without ingress !
I will add a TLS option in global part and using that global value to handle TLS too in ingress one.

For the tmp-data thing, it's a mistake from my side. I added it inside a wrong conditional instruction, I will move it outside tomorrow.
About the localStorage, it's used for the reports and and maybe other documents if I'm not wrong, (@ab-smith if you can confirm).

[jgournet]

@Nathanael-Mtd : you probably have already noticed that template/_helpers.tpl needs to be changed from .Values.ingress.tls on line 73 (and in ./templates/ingress/tls-secret.yaml too)

I was a bit too eager to try out the helm chart after the last commit

[Nathanael-Mtd]

@jgournet Oops yes, I forgot to do a helm lint before pushing (and my VSCode search is broken)😶
Probably fine now !