DSN is not optional for automigration

Dennor commented 4 months ago

Preflight checklist

[X] I could not find a solution in the existing issues, docs, nor discussions.
[X] I agree to follow this project's Code of Conduct.
[X] I have read and am following this repository's Contribution Guidelines.
[ ] I have joined the Ory Community Slack.
[ ] I am signed up to the Ory Security Patch Newsletter.

Ory Network Project

No response

Describe the bug

Version 0.42 of kratos chart no longer works with external secret. Due to the check on dsn field in migrate job, if the field was not set to some random stub, the chart won't set the dsn secret for the job and automigration will fail.

Reproducing the bug

kubectl create secret generic kratos --from-literal=dsn=postgres://user:password@postgres:5432/kratos

Create values file:

secret:
enabled: false
nameOverride: kratos
kratos:
config:
courier:
  smtp:
    connection_uri: "placeholder-secret-defined-outside"
identity:
  default_schema_id: default
  schemas:
    - id: default
      url: file:///etc/config/identity.default.schema.json
selfservice:
  default_browser_return_url: http://127.0.0.1:4455/
automigration:
enabled: true
identitySchemas:
"identity.default.schema.json": |
  {
    "$id": "https://schemas.ory.sh/presets/kratos/identity.email.schema.json",
    "$schema": "http://json-schema.org/draft-07/schema#",
    "title": "Person",
    "type": "object",
    "properties": {
      "traits": {
        "type": "object",
        "properties": {
          "email": {
            "type": "string",
            "format": "email",
            "title": "E-Mail",
            "ory.sh/kratos": {
              "credentials": {
                "password": {
                  "identifier": true
                }
              },
              "recovery": {
                "via": "email"
              },
              "verification": {
                "via": "email"
              }
            }
          }
        },
        "required": [
          "email"
        ],
        "additionalProperties": false
      }
    }
  }

helm install kratos ory/kratos -f values.yaml

Relevant log output

time=2024-05-15T12:08:01Z level=fatal msg=dsn must be set audience=application service_name=Ory Kratos service_version=v1.1.0

Relevant configuration

kratos:
  config:
    dsn: "placeholder-secret-defined-outside"

Version

0.42.0

On which operating system are you observing this issue?

Linux

In which environment are you deploying?

Kubernetes with Helm

Additional Context

There's a recent change #678 in recent 0.42.0 version which attempts to make dsn field non mendatory. Interestingly enough, it made it mendatory with external secret.

Demonsthere commented 4 months ago

So, if I read this correctly, the pod fails because it doesn't recognise dsn as being set. In your values.yaml you dont specify the dsn in the config, not supply it as an env. The issue i think comes from here https://github.com/ory/k8s/blob/master/helm/charts/kratos/templates/deployment-kratos.yaml#L151 between the interaction of a user supplied dsn vs external secret 🤔

Dennor commented 4 months ago

Actually, it fails even before that, it fails here https://github.com/ory/k8s/blob/598c91b76cec09f109bff0989d4697afab6c1624/helm/charts/kratos/templates/job-migration.yaml#L63 . I'm not sure if the deployment would work, as it did not get past automigration but I assume the result would be same.

Just to note, this is an existing setup, what is failing is not new install, it's the upgrade.

But overall, yes, you are correct. I'm supplying secret externally and referring to it through secret.nameOverride, until now, that was working, because there was no check for emptiness on dsn in config. Now, with that check there, I need to set dsn to whatever.

Of course, it's not a big issue, I've just set dsn to whatever in my values.yaml and it works again, but I'm not sure if that's the intent here :smile:

Dennor commented 4 months ago

Just one more note, in default values.yaml there's this: https://github.com/ory/k8s/blob/598c91b76cec09f109bff0989d4697afab6c1624/helm/charts/kratos/values.yaml#L82

Which I think implies that it's meant to be used the way I've used it? :thinking: Unless I've misread that comment :thinking:

Demonsthere commented 4 months ago

No, you understand correctly :) Those two options are there to support similar use cases:

The secretnameOverride allows you to use an existing secret that is sideloaded with the chart
The Env overrides allows for more customization like construction of the DSN from envs injected into the system at runtime, or even supporting different dsn for the main application and for example courier (you can set different connection parameters for them, as by default they would reuse the same dsn string)

I would say this is a small (as there is a simple workaround as you noticed) regression that we need to address in the upgrade pipeline 🤔

Demonsthere commented 4 months ago

Hi there! I pushed a fix to the generation logic, could you please verify from the master version if your case is supported properly? I updated the test cases to cover more options, and it looks ok, but would like real user feedback before making the next release 😉

ory / k8s