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..36200936e48 100644 --- a/helm/litellm-helm/tests/migrations-job_tests.yaml +++ b/helm/litellm-helm/tests/migrations-job_tests.yaml @@ -130,6 +130,92 @@ 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. + # 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: + 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: 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")