trustification / trustify

Apache License 2.0
10 stars 19 forks source link

How to run the container image of trustify without --dev-mode? #931

Open carlosthe19916 opened 2 weeks ago

carlosthe19916 commented 2 weeks ago

The compose file below works with docker-compose up. However, If I remove the param --devmode from the command passed to the trustify container then I get the error:

[trustify] | Error: database error: Query Error: error returned from database: relation "importer" does not exist
[trustify] | Caused by:
[trustify] |    Query Error: error returned from database: relation "importer" does not exist
[trustify] |    error returned from database: relation "importer" does not exist
[trustify] |    error returned from database: relation "importer" does not exist
[trustify] |    relation "importer" does not exist
version: "3"

services:
  db:
    image: postgres:15
    ports:
      - "5432:5432"
    environment:
      POSTGRES_DB: mydb
      POSTGRES_USER: myuser
      POSTGRES_PASSWORD: mypassword
    healthcheck:
      test: ["CMD-SHELL", "pg_isready -U myuser -d mydb"]
      interval: 10s
      timeout: 5s
      retries: 5

  trustify:
    image: ghcr.io/trustification/trustd:latest
    command:
      - 'api'
      - '--devmode'
    ports:
      - "8080:8080"
    environment:
      HTTP_SERVER_BIND_ADDR: 0.0.0.0
      TRUSTD_DB_USER: myuser
      TRUSTD_DB_PASSWORD: mypassword
      TRUSTD_DB_HOST: db
      TRUSTD_DB_PORT: 5432
      TRUSTD_DB_NAME: mydb
      AUTH_DISABLED: "true"
    depends_on:
      db:
        condition: service_healthy

Could I get any advice on what should be the params to be passed to the trustify container in order to make it work? considering I'd like not to pass --dev-mode as my intention is to run the container in prod mode and not in dev mode

ctron commented 2 weeks ago

--devmode for the API will enable a few things, some of them not suitable for production. So I'd recommend avoiding this option.

One thing it does is to run DB migrations:

https://github.com/trustification/trustify/blob/03e263700585e62ec61f295aa74140790b7cbeef/server/src/lib.rs#L239-L241

Which probably shouldn't happen automatically that way anyway. But in a more controlled fashion. You should be able running those manually using: trustd db migrate (plus env-vars for DB config).

I'd recommend doing something similar as we did for the GUAC deployment: https://github.com/trustification/trustification/blob/a31c6881768321b328c8d3aced3d6e5ae447359b/deploy/k8s/charts/trustification/templates/services/guac/graphql/030-Deployment.yaml#L27-L48

The idea is, there are three steps in initializing the database. Each one being able to perform manually by a DB operator (human), so it is possible to opt-out. The steps are:

  1. Create application database
  2. Apply migrations/schema
  3. Run application

Dropping privileges along those steps:

  1. Requires full system/database permissions, for creating a new database
  2. Requires permissions for creating tables etc in that specific application database
  3. Requires only CRUD style operations for that specific application database

In a simple setup, those three can be the same credentials. In a more production style setup, the probably are not.

helio-frota commented 1 week ago

with a pre-populated DB running using (docker/podman) compose we can remove --dev-mode

         "SELECT count(*) from vulnerability;"

 count
--------
 23,804
1 row (14.871062ms)
cargo run --bin trustd api --db-password eggs --auth-disabled
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.71s
     Running `target/debug/trustd api --db-password eggs --auth-disabled`
RUST_LOG is unset, using default: 'info,actix_web_prom=error'
2024-10-28T11:26:58.678938Z  INFO ThreadId(01) trustify_server::embedded_oidc: Embedded OIDC server is not active
2024-10-28T11:26:58.679006Z  WARN ThreadId(01) trustify_server: Authentication is disabled
2024-10-28T11:27:01.163261Z  INFO ThreadId(01) trustify_infrastructure::infra: Infrastructure endpoint is disabled
2024-10-28T11:27:01.163486Z  INFO ThreadId(15) trustify_infrastructure::health::checks::local: received shutdown signal
2024-10-28T11:27:01.163616Z  INFO ThreadId(15) trustify_infrastructure::health::checks::local: check future returned
2024-10-28T11:27:01.167983Z  INFO ThreadId(01) trustify_infrastructure::app::http: JSON limit: 256.0 kiB
2024-10-28T11:27:01.168057Z  INFO ThreadId(01) trustify_infrastructure::app::http: Payload limit: 2.0 MiB
2024-10-28T11:27:01.168377Z  INFO ThreadId(01) trustify_infrastructure::app::http: Binding to: [::1]:8080
2024-10-28T11:27:01.168558Z  INFO ThreadId(01) actix_server::builder: starting 12 workers
2024-10-28T11:27:01.168632Z  INFO ThreadId(01) actix_server::server: Tokio runtime found; starting in existing Tokio runtime
2024-10-28T11:27:01.168661Z  INFO ThreadId(01) actix_server::server: starting service: "actix-web-service-[::1]:8080", workers: 12, listening on: [::1]:8080
carlosthe19916 commented 5 days ago

I know I started this ticket as a simple question but after giving it some thought I question the "design" of the current approach.

There were 3 steps mentioned:

  1. Create application database
    • This step is always managed outside the app. A user will provide an existing database anywhere. I fully agree that this will be potentially a human task. So there is nothing to discuss about this step. All good!
  2. Apply migrations/schema
  3. Run application

I do question step 2 and step 3 being designed in a way that are separate and independent, forcing whoever runs the app to decide how/when to run those steps.

  1. run trustd db create. Only once during the whole life-cycle of the app
  2. run trustd db migrate. Every time the Pod starts. This could be written in the InitContainer.
  3. run trustd api. Every time the Pod starts as this is the command to start the server.

The 3 steps described above, to me, is an unnecessary complexity. trustd api should be the only command needed to start the server (everything else should be managed by the app). DB Migration Frameworks were there for long enough and that is how they work.

The previous commands were taken from trustd db --help:

Usage: trustd db [OPTIONS] <COMMAND>

Commands:
  create   
  migrate  
  refresh  
  help     Print this message or the help of the given subcommand(s)

cc: @ctron @jcrossley3 @JimFuller-RedHat

What I wrote is just a personal opinion, you guys take an honest evaluation of the design and take a decision.

jcrossley3 commented 5 days ago

As a db admin, I'd want the migration command as a separate step. As an app developer, I'd want the migration to occur automatically when I start up the app.

:smile: I know that's no help, but I can see the desire to have it both ways.

Some migrations may take a LONG time, potentially causing a complete disruption in service, and multiple pods might get into race conditions attempting to use the pre/post migrated databases. In a production environment, I think we'd want the app pods to just assume the migration has already occurred. Honestly, I'm surprised the db service/image is in the same compose file as the trustify service/image. I'd assume we use some configmap to point the latter at the former.

But I haven't thought it through, obviously.

jcrossley3 commented 5 days ago

Would you feel better with a --db-migrate switch that only performed the migration instead of a --devmode switch that might do other stuff, too?

carlosthe19916 commented 5 days ago

Some migrations may take a LONG time, potentially causing a complete disruption in service, and multiple pods might get into race conditions attempting to use the pre/post migrated databases

This is a very valid point.

Would you feel better with a --db-migrate switch that only performed the migration

Yeah an optional parameter --db-migrate sounds good. --dev-mode is something that should only be used while developing the app, nowhere else. When executing the app we should always run it in production mode

ctron commented 5 days ago

Would you feel better with a --db-migrate switch that only performed the migration instead of a --devmode switch that might do other stuff, too?

I'd be against adding yet another flag like that. It adds more complexity, and that should be justified by adding real value.

In a production setup, neither --devmode nor --db-migrate should be used. In a case like that, use trustdb db create or trustd db migrate. Once, and in a controlled manner.

So if we'd add --db-migrate, who would be the user of that?