From 3db5ba01a12d32dbfb2fff0999c51ceb97664625 Mon Sep 17 00:00:00 2001 From: Bernedotcom2312 <88426124+Bernedotcom2312@users.noreply.github.com> Date: Sun, 20 Sep 2026 15:39:13 +0200 Subject: [PATCH 1/2] fix(helm): stop mangling the bundled postgres password in the migrations Job With db.deployStandalone the migrations Job built DATABASE_URL by interpolating postgresql.auth.password straight into the string. Any of : / ? # [ ] @ + in the password reshapes the URL, and the Job connects somewhere else or not at all: --set postgresql.auth.password='p@ss/w+rd=' -> postgresql://litellm:p@ss/w+rd=@rel-postgresql/litellm which urlparse reads as password "p" on host "ss". The real host and database are gone, so the Job fails with prisma P1013 and the release never gets a migrated schema. Reported in #24331, closed by the stale bot with the bug still present. Hand the four components over instead and let the entrypoint assemble the URL, which is what the proxy Deployment already does for this same case: construct_database_url_from_env_vars() percent-encodes username, password and database name before joining them. The Job and the Deployment now take one code path rather than two, so they cannot disagree about what the bundled database is called or how a password is escaped. Sourcing username and password from the dbcredentials Secret the chart already creates also keeps the password out of the rendered Job, where it was previously readable through `kubectl get job -o yaml` and in the Helm release secret. Adds three tests: the Job exposes the components and no DATABASE_URL, a password holding URL-special characters never reaches the rendered Job, and DATABASE_NAME follows postgresql.auth.database. Verified by mutation: restoring the old hand-built URL fails two of them, hardcoding the database name fails the third. Co-Authored-By: Claude Opus 5 --- .../templates/migrations-job.yaml | 24 +++++- .../tests/migrations-job_tests.yaml | 82 +++++++++++++++++++ 2 files changed, 104 insertions(+), 2 deletions(-) diff --git a/helm/litellm-helm/templates/migrations-job.yaml b/helm/litellm-helm/templates/migrations-job.yaml index 5a873cbb965..1bcdb7c1bb5 100644 --- a/helm/litellm-helm/templates/migrations-job.yaml +++ b/helm/litellm-helm/templates/migrations-job.yaml @@ -75,8 +75,28 @@ spec: - name: DATABASE_URL value: {{ .Values.db.url | quote }} {{- else if .Values.db.deployStandalone }} - - name: DATABASE_URL - value: postgresql://{{ .Values.postgresql.auth.username }}:{{ .Values.postgresql.auth.password }}@{{ .Release.Name }}-postgresql/{{ .Values.postgresql.auth.database }} + {{- /* + Hand the credentials over as separate variables and let the entrypoint + assemble DATABASE_URL, exactly as the proxy Deployment does. Building the + URL here instead interpolated the password raw, so any of : / ? # [ ] @ + + or a space in postgresql.auth.password silently reshaped the URL and the + Job failed to connect. Sourcing the two secret values also keeps the + password out of the rendered Job and the Helm release secret. + */}} + - name: DATABASE_USERNAME + valueFrom: + secretKeyRef: + name: {{ include "litellm.fullname" . }}-dbcredentials + key: username + - name: DATABASE_PASSWORD + valueFrom: + secretKeyRef: + name: {{ include "litellm.fullname" . }}-dbcredentials + key: password + - name: DATABASE_HOST + value: {{ .Release.Name }}-postgresql + - name: DATABASE_NAME + value: {{ .Values.postgresql.auth.database }} {{- end }} {{- if .Values.envVars }} {{- range $key, $val := .Values.envVars }} diff --git a/helm/litellm-helm/tests/migrations-job_tests.yaml b/helm/litellm-helm/tests/migrations-job_tests.yaml index dd4276ac60f..8a454440744 100644 --- a/helm/litellm-helm/tests/migrations-job_tests.yaml +++ b/helm/litellm-helm/tests/migrations-job_tests.yaml @@ -130,6 +130,88 @@ tests: name: DISABLE_SCHEMA_UPDATE value: "false" + - it: should hand deployStandalone credentials over as separate env vars, not a built URL + template: migrations-job.yaml + set: + migrationJob: + enabled: true + db: + deployStandalone: true + useExisting: false + asserts: + # Assembling the URL in the template interpolated the password raw, so a + # password holding any of : / ? # [ ] @ + reshaped the URL and the Job + # connected to the wrong host, or to none. The entrypoint builds the URL + # from these four instead, percent-encoding as it goes, which is what the + # proxy Deployment already relies on for this same case. + - notContains: + path: spec.template.spec.containers[0].env + content: + name: DATABASE_URL + any: true + - contains: + path: spec.template.spec.containers[0].env + content: + name: DATABASE_USERNAME + valueFrom: + secretKeyRef: + name: RELEASE-NAME-litellm-dbcredentials + key: username + - contains: + path: spec.template.spec.containers[0].env + content: + name: DATABASE_PASSWORD + valueFrom: + secretKeyRef: + name: RELEASE-NAME-litellm-dbcredentials + key: password + - contains: + path: spec.template.spec.containers[0].env + content: + name: DATABASE_HOST + value: RELEASE-NAME-postgresql + - contains: + path: spec.template.spec.containers[0].env + content: + name: DATABASE_NAME + value: litellm + + - it: should keep a password holding URL-special characters out of the rendered Job + template: migrations-job.yaml + set: + migrationJob: + enabled: true + db: + deployStandalone: true + useExisting: false + postgresql: + auth: + password: "p@ss/w+rd=" + asserts: + # The password now only ever travels through the dbcredentials Secret, so + # it is neither mangled into a URL nor readable in `kubectl get job -o yaml` + # and the Helm release secret. + - notMatchRegexRaw: + pattern: "p@ss/w\\+rd=" + + - it: should follow postgresql.auth.database for the standalone database name + template: migrations-job.yaml + set: + migrationJob: + enabled: true + db: + deployStandalone: true + useExisting: false + postgresql: + auth: + database: custom-db + asserts: + - contains: + path: spec.template.spec.containers[0].env + content: + name: DATABASE_NAME + value: custom-db + - it: should not include DATABASE_URL when deployStandalone is false template: migrations-job.yaml set: From b5a62d94ec5ee2b80e3f5bfb940c661e1e56a16a Mon Sep 17 00:00:00 2001 From: Bernedotcom2312 <88426124+Bernedotcom2312@users.noreply.github.com> Date: Sun, 20 Sep 2026 16:13:00 +0200 Subject: [PATCH 2/2] test(proxy): assert the assembled database URL round-trips, not how it is spelled MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review feedback on #42142: the chart tests asserted the shape of the rendered env block and nothing exercised what the resulting URL does. AGENTS.md asks for function over structure. The existing special-character test checks that certain percent-encoded substrings appear in the output, which still describes the spelling rather than the result. Adds a parametrized test that parses the assembled URL the way a client does and asserts the host, database, username and password come back out as they went in — the property that actually says the Job reaches the right database. Also pins the bug class in the Helm suite independently of URL spelling: the standalone migrations Job must render no connection string at all. Mutation results. Dropping quote_plus from the password in construct_database_url_from_env_vars fails 3 of the 4 parametrized cases; 'a:b@c' survives because urlparse splits userinfo on the last '@', so raw interpolation happens to parse back the same. Restoring the old hand-built URL in the chart fails 3 of the Helm assertions. Co-Authored-By: Claude Opus 5 --- .../tests/migrations-job_tests.yaml | 4 ++ .../proxy/utils/helpers/test_misc_helpers.py | 41 +++++++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/helm/litellm-helm/tests/migrations-job_tests.yaml b/helm/litellm-helm/tests/migrations-job_tests.yaml index 8a454440744..36200936e48 100644 --- a/helm/litellm-helm/tests/migrations-job_tests.yaml +++ b/helm/litellm-helm/tests/migrations-job_tests.yaml @@ -144,6 +144,10 @@ tests: # connected to the wrong host, or to none. The entrypoint builds the URL # from these four instead, percent-encoding as it goes, which is what the # proxy Deployment already relies on for this same case. + # The bug class, pinned independently of how a URL would be spelled: the + # Job must carry no connection string at all for this case. + - notMatchRegexRaw: + pattern: "postgresql://" - notContains: path: spec.template.spec.containers[0].env content: diff --git a/tests/unit/proxy/utils/helpers/test_misc_helpers.py b/tests/unit/proxy/utils/helpers/test_misc_helpers.py index 6267428e02e..7b858ee16ab 100644 --- a/tests/unit/proxy/utils/helpers/test_misc_helpers.py +++ b/tests/unit/proxy/utils/helpers/test_misc_helpers.py @@ -1,3 +1,5 @@ +from urllib.parse import unquote, urlparse + import pytest from fastapi import HTTPException @@ -168,6 +170,45 @@ def test_construct_database_url_from_env_vars_special_chars_encoded(monkeypatch) } +@pytest.mark.parametrize( + "password", + [ + "p@ss/w+rd=", + "s3cr#t?x", + "a:b@c", + "br[ack]ets", + ], +) +def test_construct_database_url_from_env_vars_round_trips_special_char_password(monkeypatch, password): + """A client parsing the assembled URL gets back the host, database and password we put in. + + The Helm chart's bundled-postgres path hands these four variables to the migrations Job + instead of building a URL itself, because interpolating a password holding any of + : / ? # [ ] @ + into a connection string silently moves the host and truncates the + password. Asserting the parse, rather than the encoded spelling, is what says the + resulting URL still addresses the right database. + """ + monkeypatch.setenv("DATABASE_HOST", "release-postgresql") + monkeypatch.setenv("DATABASE_USERNAME", "litellm") + monkeypatch.setenv("DATABASE_PASSWORD", password) + monkeypatch.setenv("DATABASE_NAME", "litellm") + monkeypatch.delenv("DATABASE_SCHEMA", raising=False) + + parsed = urlparse(construct_database_url_from_env_vars()) + + assert { + "hostname": parsed.hostname, + "database": parsed.path.lstrip("/"), + "username": unquote(parsed.username or ""), + "password": unquote(parsed.password or ""), + } == { + "hostname": "release-postgresql", + "database": "litellm", + "username": "litellm", + "password": password, + } + + def test_construct_database_url_from_env_vars_with_schema(monkeypatch): monkeypatch.setenv("DATABASE_HOST", "db.example.com") monkeypatch.setenv("DATABASE_USERNAME", "user")