Merge b5a62d94ec into 4ece6c9fb8

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 }}

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:

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")