hedgedoc / container

HedgeDoc container image resources
https://docs.hedgedoc.org/setup/docker/
195 stars 52 forks source link

improve the way you can configure the database #114

Closed oupala closed 3 years ago

oupala commented 4 years ago

As discussed in another issue (codimd/server#364), @SISheogorath explained a bit about how to configure the database with environment variables (with a default hard-coded fallback).

This is due to the following lines:

https://github.com/codimd/container/blob/7cf4194ca07628ab19f2becc4ed60dd10815d5ec/resources/docker-entrypoint.sh#L21-L23

Hard-coding a default fallback seems to be a dev-oriented hack but should not appear in production, IMHO. There could be people declaring a postgres database with hackmd:hackmdpass login and pass to make it work. Hard-coding password is a bad habit and lowers security level. It should be avoided.

In addition, the codimd server documentation states that the default value of CMD_DB_URL is undefined (which is a good thing) while this default value changes in codimd container without being documented.

I clearly think that the codimd container should be usable for most of the usecases without requiring a custom build.

In my case the database credential are offered by the platform as individual variables:

and I don't have an easy way to concatenate them into a CMD_DB_URL as expected by the docker-entrypoint.sh script.

Could we make it clear in the issue to explain the pro and cons of each way of configuring the database?

We should also explain the constraints that exists with included libraries (I'm thinking about sequelize which might add some constraints.

In the end, I would love to be able to use automatic provisionning of my database in a k8s context. I currently have to use a 3 steps deployement:

  1. deploy the database
  2. get the automatically created credentials and manually concatenate them into a single CMD_DB_URL string
  3. deploy codimd with the previous concatenated string into an environment variable

Whereas I could use another feature of k8s to automatically inject maria db password into codimd configuration:

          - name: CMD_DB_PASSWORD
            valueFrom:
              secretKeyRef:
                key: MARIA_APP_PASSWORD
                name: codimd-db-staging-secret

This would add automation and security, both at one time.

What do you think?

hugopeixoto commented 3 years ago

I don't use k8s, so sorry if this sounds completely wrong, but could you define your pod like this?

      env:
          - name: CMD_DB_PASSWORD
            valueFrom:
              secretKeyRef:
                key: MARIA_APP_PASSWORD
                name: codimd-db-staging-secret
        - name: CMD_DB_URL
          value: "mysql://$(CMD_DB_LOGIN):$(CMD_DB_PASSWORD)@$(CMD_DB_HOSTANDPORT)/$(CMD_DB_NAME)"
oupala commented 3 years ago

No, this does not work as the variable does not get interpreted by k8s:

          - name: CMD_DB_URL
            value: 'mysql://${CMD_DB_USER}:${CMD_DB_PASSWORD}@mariadb-service-sql:3306/hedgedoc-db'
          - name: CMD_DB_USER
            value: 'hedgedoc-db-user'

leads to the following logs:

Parsed url mysql://${CMD_DB_USER}:*****@mariadb-service-sql:3306/hedgedoc-db

[ERROR:[Access denied for user '${CMD_DB_USER}'@'10.19.2.11' (using password: YES)

As a consequence, you cannot use a declared variable in another variable.

SISheogorath commented 3 years ago

While this doesn't work with K8s itself, but with templating mechanisms like helm, this should be easily achievable.

Also currently it is possible to put the dbURL into /run/secrets/dbURL https://github.com/hedgedoc/hedgedoc/blob/dfd710982a5b53055a1a327e5ee35bd29de3f5bb/lib/config/dockerSecret.js#L16

However, I can somewhat understand the desire to simplify the process. But I'm not sure how much effort it is worth to put into this.The probably easiest solution would be to extent the image, and patch the entrypoint script for such more-complex deployments. The maintenance effort is minimal and can be easily automated.

oupala commented 3 years ago

I've just test a small patch that solves the problem I had.

I then created a pull request: #153.

I consider my pull request as a work in progress as I've seen some other things to fix in the entrypoint shell file.

Here is the teaser of the pull request:

Add the ability to choose multiple type of configuration. It could be a configuration using CMD_DB_URL (which was the standard way up to now).

But it is now possible to send multiple environment variable to configure the database.

This makes the container easier to configure on many platform (including k8s) as the managed database is sometime offering credential as individual variable instead of a long url.

SISheogorath commented 3 years ago

This should be moved to the hedgedoc repository development, since all database handling has been inlined in the application. I would guess there are good chances that we get these:

https://github.com/hedgedoc/hedgedoc/blob/8b374d8c1972db2b09126e8f9cc10384552abf29/config.json.example#L37-L44

Exposed here:

https://github.com/hedgedoc/hedgedoc/blob/8b374d8c1972db2b09126e8f9cc10384552abf29/lib/config/environment.js

Since the container image, no longer needs to handle the database config in any way, this should be fine.