From c788769129e23261d7be091cac9aa036c2c04b09 Mon Sep 17 00:00:00 2001 From: Matthew Howard <78384492+matthowardcohere@users.noreply.github.com> Date: Mon, 14 Sep 2026 15:18:14 -0700 Subject: [PATCH 1/3] fix(terraform): recognize a credential name conflict and adopt the existing credential The proxy answered a duplicate credential_name with a 500 carrying Prisma's unique-constraint message, so terraform apply against a credential the state had lost died with an opaque database error. Classify that response as a conflict, adopt the existing credential with a PATCH that carries model_id, and set the resource ID only once the adopt succeeds so a failed PATCH does not taint a credential this run never owned Squashed from the commits on #39745 with authorship preserved Fixes https://github.com/BerriAI/terraform-provider-litellm/issues/8 --- terraform/provider/CHANGELOG.md | 1 + .../litellm/resource_credential_crud.go | 26 +++ .../litellm/resource_credential_crud_test.go | 157 ++++++++++++++++++ terraform/provider/litellm/utils.go | 32 ++++ 4 files changed, 216 insertions(+) diff --git a/terraform/provider/CHANGELOG.md b/terraform/provider/CHANGELOG.md index 8c0ef5a8b15..3daee3250b6 100644 --- a/terraform/provider/CHANGELOG.md +++ b/terraform/provider/CHANGELOG.md @@ -38,6 +38,7 @@ longer signal it. ### Fixed - **key**: An update that changes `team_id` and fails because the key was already cascade-deleted along with its previous team now recovers by recreating the key under the new team, instead of aborting the apply. The key's absence is confirmed against the proxy first, so an unrelated failure still errors out, and a `team_id` change between two teams that both still exist stays a plain in-place update +- **credential**: `litellm_credential` create now adopts an existing credential on a `credential_name` conflict instead of failing with a 500; `apply` is idempotent again once state loses track of a credential that still exists on the proxy - **team**: Read now decodes the `team_info` envelope `/team/info` actually returns, so team attributes refresh from the proxy instead of always falling back to the prior state - **key**: Read now unwraps the `info` envelope `/key/info` actually returns; previously reads mapped nothing back into state, so drift on a key was never detected - **key**: Read now picks up `model_rpm_limit`, `model_tpm_limit`, `guardrails`, `tags`, `enforced_params`, `allowed_passthrough_routes`, `rpm_limit_type`, `tpm_limit_type` and `prompts` from `info.metadata`, where the proxy actually stores them; previously they stayed empty in state, so a matching config showed a permanent phantom diff on them and out-of-band changes to them were never detected diff --git a/terraform/provider/litellm/resource_credential_crud.go b/terraform/provider/litellm/resource_credential_crud.go index dd9aef64f76..cb5031ee04e 100644 --- a/terraform/provider/litellm/resource_credential_crud.go +++ b/terraform/provider/litellm/resource_credential_crud.go @@ -88,6 +88,26 @@ func resourceLiteLLMCredentialCreate(d *schema.ResourceData, m interface{}) erro err = handleCredentialAPIResponse(resp, nil, client) if err != nil { + // If a credential with this name already exists, adopt it instead of + // failing: take ownership and update the existing credential's + // values (merged onto whatever it already had - not a full replace) + // rather than erroring on the unique-constraint conflict. See + // https://github.com/BerriAI/terraform-provider-litellm/issues/8. + if err.Error() == "credential_conflict" { + log.Printf("[WARN] Credential %q already exists; adopting it and updating to match configuration.", credentialName) + d.SetId(credentialName) + if updateErr := resourceLiteLLMCredentialUpdate(d, m); updateErr != nil { + // Adoption failed before this run took ownership of + // anything real. Clear the ID so create is reported as + // failed outright (matching pre-adoption behavior) instead + // of tainting state for a credential this run doesn't own - + // state that would otherwise get destroyed on the next + // apply. + d.SetId("") + return fmt.Errorf("failed to adopt existing credential %q: %w", credentialName, updateErr) + } + return nil + } return fmt.Errorf("failed to create credential: %w", err) } @@ -142,6 +162,7 @@ func resourceLiteLLMCredentialUpdate(d *schema.ResourceData, m interface{}) erro client := m.(*Client) credentialName := d.Id() + modelID := d.Get("model_id").(string) credentialInfo := d.Get("credential_info").(map[string]interface{}) credentialValues := d.Get("credential_values").(map[string]interface{}) @@ -157,8 +178,13 @@ func resourceLiteLLMCredentialUpdate(d *schema.ResourceData, m interface{}) erro credValuesMap[k] = v } + // model_id must travel with the update the same way it does on create, + // so the proxy's model-based credential resolution still applies. Without + // it, updating (or adopting) a model_id-scoped credential silently loses + // that association. credentialRequest := CredentialRequest{ CredentialName: credentialName, + ModelID: modelID, CredentialInfo: credInfoMap, CredentialValues: credValuesMap, } diff --git a/terraform/provider/litellm/resource_credential_crud_test.go b/terraform/provider/litellm/resource_credential_crud_test.go index 3398e58dd13..6e0b818fe33 100644 --- a/terraform/provider/litellm/resource_credential_crud_test.go +++ b/terraform/provider/litellm/resource_credential_crud_test.go @@ -3,6 +3,7 @@ package litellm import ( "encoding/json" "fmt" + "io" "net/http" "net/http/httptest" "sync/atomic" @@ -199,3 +200,159 @@ func TestRetryCredentialRead_ConnectionError(t *testing.T) { // Connection error should not be retried (not a "credential_not_found") fmt.Printf("connection error (expected): %v\n", err) } + +// conflictServer builds the shared conflict-then-recover mock used by the +// adoption tests below. patchStatus/patchBody control the PATCH response, so +// callers can exercise both the success and failure paths. +func conflictServer(t *testing.T, patchStatus int, patchBody string) (*httptest.Server, *int32, *int32, *[]byte) { + t.Helper() + var createCalls, patchCalls int32 + var capturedPatchBody []byte + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodPost && r.URL.Path == "/credentials": + atomic.AddInt32(&createCalls, 1) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusInternalServerError) + w.Write([]byte(`{"error":{"message":"Unique constraint failed on the fields: (` + "`credential_name`" + `)","type":"internal_server_error","code":"500"}}`)) + case r.Method == http.MethodPatch: + atomic.AddInt32(&patchCalls, 1) + if r.URL.Path != "/credentials/conflict-test" { + t.Errorf("PATCH went to %q, want /credentials/conflict-test", r.URL.Path) + } + body, _ := io.ReadAll(r.Body) + capturedPatchBody = body + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(patchStatus) + w.Write([]byte(patchBody)) + case r.Method == http.MethodGet: + if r.URL.Path != "/credentials/by_name/conflict-test" || r.URL.Query().Get("model_id") != "model-1" { + t.Errorf("GET went to %q (query %q), want /credentials/by_name/conflict-test?model_id=model-1", r.URL.Path, r.URL.RawQuery) + } + resp := CredentialResponse{CredentialName: "conflict-test", CredentialInfo: map[string]interface{}{}} + body, _ := json.Marshal(resp) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + w.Write(body) + default: + http.NotFound(w, r) + } + })) + return srv, &createCalls, &patchCalls, &capturedPatchBody +} + +// A credential that already exists in LiteLLM (created out of band, or left +// behind by a prior apply that dropped state) must be adopted on create +// instead of failing on the credential_name unique-constraint conflict, and +// the adopt PATCH must carry model_id so model-based credential resolution +// still applies (previously dropped - see +// https://github.com/BerriAI/litellm/pull/39745). +func TestResourceLiteLLMCredentialCreate_AdoptsOnConflict(t *testing.T) { + srv, createCalls, patchCalls, patchBody := conflictServer(t, http.StatusOK, `{}`) + defer srv.Close() + + client := NewClient(srv.URL, "test-key", true) + d := schema.TestResourceDataRaw(t, resourceLiteLLMCredential().Schema, map[string]interface{}{ + "credential_name": "conflict-test", + "model_id": "model-1", + "credential_info": map[string]interface{}{"custom_llm_provider": "bedrock"}, + "credential_values": map[string]interface{}{"aws_access_key_id": "val"}, + }) + + if err := resourceLiteLLMCredentialCreate(d, client); err != nil { + t.Fatalf("expected create to adopt the existing credential, got error: %v", err) + } + if d.Id() != "conflict-test" { + t.Fatalf("expected ID %q, got %q", "conflict-test", d.Id()) + } + if got := atomic.LoadInt32(createCalls); got != 1 { + t.Fatalf("expected exactly 1 POST /credentials call, got %d", got) + } + if got := atomic.LoadInt32(patchCalls); got != 1 { + t.Fatalf("expected the conflict to trigger exactly 1 PATCH (adopt-and-update), got %d", got) + } + + var sent map[string]interface{} + if err := json.Unmarshal(*patchBody, &sent); err != nil { + t.Fatalf("PATCH body was not valid JSON: %v (%s)", err, *patchBody) + } + if sent["credential_name"] != "conflict-test" { + t.Errorf("PATCH body credential_name = %v, want conflict-test", sent["credential_name"]) + } + if sent["model_id"] != "model-1" { + t.Errorf("PATCH body model_id = %v, want model-1 (adoption must not drop model-based credential resolution)", sent["model_id"]) + } + credInfo, _ := sent["credential_info"].(map[string]interface{}) + if credInfo["custom_llm_provider"] != "bedrock" { + t.Errorf("PATCH body credential_info = %v, want custom_llm_provider=bedrock", sent["credential_info"]) + } +} + +// If the adopt PATCH itself fails, create must not have set the resource ID +// for a credential this run doesn't own - otherwise Terraform taints the +// entry and the *next* apply destroys a credential nobody here created. +func TestResourceLiteLLMCredentialCreate_FailedAdoptDoesNotTaint(t *testing.T) { + srv, createCalls, patchCalls, _ := conflictServer(t, http.StatusInternalServerError, `{"error":{"message":"Internal Server Error"}}`) + defer srv.Close() + + client := NewClient(srv.URL, "test-key", true) + d := schema.TestResourceDataRaw(t, resourceLiteLLMCredential().Schema, map[string]interface{}{ + "credential_name": "conflict-test", + "credential_info": map[string]interface{}{}, + "credential_values": map[string]interface{}{"key": "val"}, + }) + + err := resourceLiteLLMCredentialCreate(d, client) + if err == nil { + t.Fatal("expected an error when the adopt PATCH fails, got nil") + } + if got := atomic.LoadInt32(createCalls); got != 1 { + t.Fatalf("expected exactly 1 POST /credentials call, got %d", got) + } + if got := atomic.LoadInt32(patchCalls); got != 1 { + t.Fatalf("expected exactly 1 PATCH attempt, got %d", got) + } + if d.Id() != "" { + t.Fatalf("resource ID must stay empty after a failed adopt, got %q (a tainted entry would be destroyed on the next apply)", d.Id()) + } +} + +// A non-conflict failure (a plain 500, for example) must return the original +// error and never attempt to adopt anything. +func TestResourceLiteLLMCredentialCreate_NonConflictErrorDoesNotAdopt(t *testing.T) { + var createCalls, patchCalls int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodPost && r.URL.Path == "/credentials": + atomic.AddInt32(&createCalls, 1) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusInternalServerError) + w.Write([]byte(`{"error":{"message":"Internal Server Error","type":"internal_server_error"}}`)) + case r.Method == http.MethodPatch: + atomic.AddInt32(&patchCalls, 1) + w.WriteHeader(http.StatusOK) + w.Write([]byte(`{}`)) + default: + http.NotFound(w, r) + } + })) + defer srv.Close() + + client := NewClient(srv.URL, "test-key", true) + d := schema.TestResourceDataRaw(t, resourceLiteLLMCredential().Schema, map[string]interface{}{ + "credential_name": "some-cred", + "credential_info": map[string]interface{}{}, + "credential_values": map[string]interface{}{"key": "val"}, + }) + + err := resourceLiteLLMCredentialCreate(d, client) + if err == nil { + t.Fatal("expected an error for a non-conflict failure, got nil") + } + if got := atomic.LoadInt32(&patchCalls); got != 0 { + t.Fatalf("expected no PATCH attempt for a non-conflict error, got %d", got) + } + if d.Id() != "" { + t.Fatalf("resource ID must stay empty on a non-conflict failure, got %q", d.Id()) + } +} diff --git a/terraform/provider/litellm/utils.go b/terraform/provider/litellm/utils.go index 5e81766d3f3..a123f5d350a 100644 --- a/terraform/provider/litellm/utils.go +++ b/terraform/provider/litellm/utils.go @@ -202,6 +202,35 @@ func isCredentialNotFoundError(errResp ErrorResponse) bool { return false } +// isCredentialConflictError checks if the error response indicates a credential +// name collision. LiteLLM surfaces this as a 500 carrying the underlying Prisma +// unique-constraint message on credential_name. See +// https://github.com/BerriAI/terraform-provider-litellm/issues/8. +func isCredentialConflictError(errResp ErrorResponse) bool { + if msg, ok := errResp.Error.Message.(string); ok { + if strings.Contains(msg, "Unique constraint failed") && strings.Contains(msg, "credential_name") { + return true + } + } + + if msgMap, ok := errResp.Error.Message.(map[string]interface{}); ok { + if errStr, ok := msgMap["error"].(string); ok { + if strings.Contains(errStr, "Unique constraint failed") && strings.Contains(errStr, "credential_name") { + return true + } + } + } + + // Check Detail.Error field for LiteLLM proxy error format + if errResp.Detail.Error != "" { + if strings.Contains(errResp.Detail.Error, "Unique constraint failed") && strings.Contains(errResp.Detail.Error, "credential_name") { + return true + } + } + + return false +} + // handleCredentialAPIResponse handles API responses specifically for credential operations func handleCredentialAPIResponse(resp *http.Response, result interface{}, client *Client) error { bodyBytes, err := io.ReadAll(resp.Body) @@ -219,6 +248,9 @@ func handleCredentialAPIResponse(resp *http.Response, result interface{}, client if isCredentialNotFoundError(errResp) { return fmt.Errorf("credential_not_found") } + if isCredentialConflictError(errResp) { + return fmt.Errorf("credential_conflict") + } } return fmt.Errorf("API request failed: Status: %s, Response: %s", resp.Status, client.redactSensitiveData(string(bodyBytes))) From 81806f33cf623e9bc96b553f87af641f8e378a3e Mon Sep 17 00:00:00 2001 From: ryan-crabbe-berri Date: Mon, 14 Sep 2026 15:18:21 -0700 Subject: [PATCH 2/3] fix(credentials): answer 409 on a name collision, let PATCH resolve values from model_id POST /credentials let a duplicate name hit the unique index and handed back Prisma's "Unique constraint failed" as a 500, so callers string-matched that message to tell a caller mistake from a server fault. The unique violation now maps to a 409 whose message names the PATCH route, two concurrent creates of one name agree on it, and the detection lives in a repository helper the five hand-rolled copies can move onto later PATCH /credentials/{name} took a CredentialItem body, so the model_id the Terraform adopt path sent was dropped. It now accepts UpdateCredentialItem and shares the deployment lookup with create. Both handlers take the router as a FastAPI dependency instead of reading the proxy global, which is what the tests override --- litellm/models/credentials.py | 9 ++ .../proxy/credential_endpoints/endpoints.py | 101 +++++++++---- litellm/repositories/base_repository.py | 10 ++ .../credential_endpoints/test_endpoints.py | 139 +++++++++++++++++- ui/litellm-dashboard/src/lib/http/schema.d.ts | 17 ++- 5 files changed, 242 insertions(+), 34 deletions(-) diff --git a/litellm/models/credentials.py b/litellm/models/credentials.py index 56836234898..0878eea5769 100644 --- a/litellm/models/credentials.py +++ b/litellm/models/credentials.py @@ -5,6 +5,8 @@ These are the canonical credential types for the proxy. They live in the model layer; ``litellm.types.utils`` re-exports them for backwards compatibility. """ +from collections.abc import Mapping + from pydantic import BaseModel, model_validator @@ -27,3 +29,10 @@ class CreateCredentialItem(CredentialBase): if not values.get("credential_values") and not values.get("model_id"): raise ValueError("Either credential_values or model_id must be set") return values + + +class UpdateCredentialItem(BaseModel): + credential_name: str + credential_info: Mapping[str, object] + credential_values: Mapping[str, object] | None = None + model_id: str | None = None diff --git a/litellm/proxy/credential_endpoints/endpoints.py b/litellm/proxy/credential_endpoints/endpoints.py index 66789748707..f99cce14722 100644 --- a/litellm/proxy/credential_endpoints/endpoints.py +++ b/litellm/proxy/credential_endpoints/endpoints.py @@ -2,25 +2,31 @@ CRUD endpoints for storing reusable credentials. """ +from collections.abc import Mapping from typing import ( + Annotated, Final, cast, # noqa: TID251 # jsonify_object in proxy/utils.py is annotated with a bare dict ) from fastapi import APIRouter, Depends, HTTPException, Path, Request, Response +from pydantic import TypeAdapter import litellm from litellm._logging import verbose_proxy_logger from litellm.litellm_core_utils.credential_accessor import CredentialAccessor from litellm.litellm_core_utils.litellm_logging import _get_masked_values +from litellm.models.credentials import UpdateCredentialItem from litellm.proxy._types import CommonProxyErrors, UserAPIKeyAuth from litellm.proxy.auth.user_api_key_auth import user_api_key_auth from litellm.proxy.common_utils.encrypt_decrypt_utils import encrypt_value_helper from litellm.proxy.utils import handle_exception_on_proxy, jsonify_object +from litellm.repositories.base_repository import is_unique_violation from litellm.repositories.credentials_repository import CredentialsRepository from litellm.types.utils import CreateCredentialItem, CredentialItem router: Final = APIRouter() +_CREDENTIAL_DICT_ADAPTER: Final = TypeAdapter(dict[str, object]) class CredentialHelperUtils: @@ -40,6 +46,33 @@ class CredentialHelperUtils: ) +def _credential_exists_detail(credential_name: str) -> str: + return ( + f"Credential '{credential_name}' already exists. " + f"Update it with PATCH /credentials/{credential_name}, or delete it first." + ) + + +def get_llm_router() -> litellm.Router | None: + from litellm.proxy.proxy_server import llm_router + + return llm_router + + +def _resolve_deployment_credentials(llm_router: litellm.Router | None, model_id: str) -> Mapping[str, object]: + if llm_router is None: + raise HTTPException( + status_code=500, + detail="LLM router not found. Please ensure you have a valid router instance.", + ) + if llm_router.get_deployment(model_id) is None: + raise HTTPException(status_code=404, detail="Model not found") + credential_values: Final = llm_router.get_deployment_credentials(model_id) + if credential_values is None: + raise HTTPException(status_code=404, detail="Model not found") + return _CREDENTIAL_DICT_ADAPTER.validate_python(credential_values) + + @router.post( "/credentials", dependencies=[Depends(user_api_key_auth)], @@ -50,13 +83,14 @@ async def create_credential( fastapi_response: Response, credential: CreateCredentialItem, user_api_key_dict: UserAPIKeyAuth = Depends(user_api_key_auth), + llm_router: Annotated[litellm.Router | None, Depends(get_llm_router)] = None, ): """ [BETA] endpoint. This might change unexpectedly. Stores credential in DB. Reloads credentials in memory. """ - from litellm.proxy.proxy_server import llm_router, prisma_client + from litellm.proxy.proxy_server import prisma_client try: if prisma_client is None: @@ -64,29 +98,19 @@ async def create_credential( status_code=500, detail={"error": CommonProxyErrors.db_not_connected_error.value}, ) - if credential.model_id: - if llm_router is None: - raise HTTPException( - status_code=500, - detail="LLM router not found. Please ensure you have a valid router instance.", - ) - # get model from router - model: Final = llm_router.get_deployment(credential.model_id) - if model is None: - raise HTTPException(status_code=404, detail="Model not found") - credential_values: Final = llm_router.get_deployment_credentials(credential.model_id) - if credential_values is None: - raise HTTPException(status_code=404, detail="Model not found") - credential.credential_values = credential_values - - if credential.credential_values is None: + credential_values: Final = ( + _resolve_deployment_credentials(llm_router, credential.model_id) + if credential.model_id + else credential.credential_values + ) + if credential_values is None: raise HTTPException( status_code=400, detail="Credential values are required. Unable to infer credential values from model ID.", ) processed_credential: Final = CredentialItem( credential_name=credential.credential_name, - credential_values=credential.credential_values, + credential_values=_CREDENTIAL_DICT_ADAPTER.validate_python(credential_values), credential_info=credential.credential_info, ) encrypted_credential: Final = CredentialHelperUtils.encrypt_credential_values(processed_credential) @@ -94,13 +118,18 @@ async def create_credential( credentials_dict_jsonified: Final = cast( # cast-ok: deep-copies a model_dump, so keys are str "dict[str, object]", jsonify_object(credentials_dict) ) - await CredentialsRepository(prisma_client).create( - data={ - **credentials_dict_jsonified, - "created_by": user_api_key_dict.user_id, - "updated_by": user_api_key_dict.user_id, - } - ) + try: + await CredentialsRepository(prisma_client).create( + data={ + **credentials_dict_jsonified, + "created_by": user_api_key_dict.user_id, + "updated_by": user_api_key_dict.user_id, + } + ) + except Exception as e: + if not is_unique_violation(e): + raise + raise HTTPException(status_code=409, detail=_credential_exists_detail(credential.credential_name)) ## ADD TO LITELLM ## CredentialAccessor.upsert_credentials([processed_credential]) @@ -300,9 +329,10 @@ def update_db_credential( async def update_credential( request: Request, fastapi_response: Response, - credential: CredentialItem, + credential: UpdateCredentialItem, credential_name: str = Path(..., description="The credential name, percent-decoded; may contain slashes"), user_api_key_dict: UserAPIKeyAuth = Depends(user_api_key_auth), + llm_router: Annotated[litellm.Router | None, Depends(get_llm_router)] = None, ): """ [BETA] endpoint. This might change unexpectedly. @@ -319,7 +349,16 @@ async def update_credential( db_credential: Final = await credentials_repository.find_by_name(credential_name) if db_credential is None: raise HTTPException(status_code=404, detail="Credential not found in DB.") - merged_credential: Final = update_db_credential(db_credential, credential) + patch: Final = CredentialItem( + credential_name=credential.credential_name, + credential_info=_CREDENTIAL_DICT_ADAPTER.validate_python(credential.credential_info), + credential_values=_CREDENTIAL_DICT_ADAPTER.validate_python( + _resolve_deployment_credentials(llm_router, credential.model_id) + if credential.model_id + else credential.credential_values or {} + ), + ) + merged_credential: Final = update_db_credential(db_credential, patch) credential_object_jsonified: Final = cast( # cast-ok: deep-copies a model_dump, so keys are str "dict[str, object]", jsonify_object(merged_credential.model_dump()) ) @@ -341,11 +380,11 @@ async def update_credential( if existing_in_memory is not None: in_memory_values: Final = dict(existing_in_memory.credential_values or {}) - if credential.credential_values: - in_memory_values.update(credential.credential_values) + if patch.credential_values: + in_memory_values.update(patch.credential_values) in_memory_info: Final = dict(existing_in_memory.credential_info or {}) - if credential.credential_info: - in_memory_info.update(credential.credential_info) + if patch.credential_info: + in_memory_info.update(patch.credential_info) updated_in_memory: Final = CredentialItem( credential_name=new_name, credential_values=in_memory_values, diff --git a/litellm/repositories/base_repository.py b/litellm/repositories/base_repository.py index 26c1c386138..065842b39e2 100644 --- a/litellm/repositories/base_repository.py +++ b/litellm/repositories/base_repository.py @@ -117,3 +117,13 @@ class BaseRepository(ABC, Generic[T]): """Check if a record exists.""" record: Final = await self.table.find_unique(where={id_field: id_value}) return record is not None + + +def is_unique_violation(exc: BaseException) -> bool: + try: + from prisma.errors import UniqueViolationError + except ImportError: + return "P2002" in str(exc) or "unique constraint" in str(exc).lower() + if isinstance(exc, UniqueViolationError): + return True + return getattr(exc, "code", None) == "P2002" diff --git a/tests/test_litellm/proxy/credential_endpoints/test_endpoints.py b/tests/test_litellm/proxy/credential_endpoints/test_endpoints.py index d67a9afdcc8..631767f52ae 100644 --- a/tests/test_litellm/proxy/credential_endpoints/test_endpoints.py +++ b/tests/test_litellm/proxy/credential_endpoints/test_endpoints.py @@ -1,5 +1,6 @@ """Tests for the credential management endpoints.""" +import json from unittest.mock import AsyncMock, MagicMock, patch import pytest @@ -9,6 +10,7 @@ from fastapi.testclient import TestClient import litellm from litellm.proxy._types import UserAPIKeyAuth from litellm.proxy.auth.user_api_key_auth import user_api_key_auth +from litellm.proxy.credential_endpoints.endpoints import get_llm_router from litellm.proxy.proxy_server import app from litellm.types.utils import CredentialItem @@ -47,23 +49,27 @@ def _list_credentials(): @pytest.fixture def credential_store(): """Stands the credential store up for one test: whether the database is reachable, what - the proxy is already serving from memory, and what each repository call hands back.""" + the proxy is already serving from memory, which router deployments resolve against, and + what each repository call hands back.""" def install( *, connected: bool = True, in_memory: tuple[object, ...] = (), + llm_router: object | None = None, **repository_calls: AsyncMock, ) -> None: patch("litellm.proxy.proxy_server.prisma_client", MagicMock() if connected else None).start() patch("litellm.proxy.proxy_server.master_key", "sk-test-master").start() patch.object(litellm, "credential_list", list(in_memory)).start() + app.dependency_overrides[get_llm_router] = lambda: llm_router repository = patch("litellm.proxy.credential_endpoints.endpoints.CredentialsRepository").start() for call_name, result in repository_calls.items(): setattr(repository.return_value, call_name, result) yield install patch.stopall() + app.dependency_overrides.pop(get_llm_router, None) def test_update_credential_answers_404_when_the_credential_does_not_exist(credential_store): @@ -122,7 +128,9 @@ def test_delete_credential_answers_404_when_the_credential_does_not_exist(creden response = _delete_credential("definitely-not-there") - assert response.status_code == 404, f"delete of a missing credential answered {response.status_code}: {response.text}" + assert response.status_code == 404, ( + f"delete of a missing credential answered {response.status_code}: {response.text}" + ) assert "definitely-not-there" in response.text @@ -195,3 +203,130 @@ def test_get_credentials_answers_an_error_status_when_the_listing_fails(credenti assert response.status_code == 500, f"failed listing answered {response.status_code}: {response.text}" assert response.json().get("success") is not True + + +def _create_credential(body: dict): + return _call_as_admin("POST", "/credentials", body) + + +class _UniqueViolation(Exception): + code = "P2002" + + +def test_create_credential_answers_409_when_the_name_is_already_taken(credential_store): + """Regression: the unique index used to surface as a Prisma 500 that callers string-matched.""" + credential_store( + create=AsyncMock(side_effect=_UniqueViolation("Unique constraint failed on the fields: (`credential_name`)")), + ) + + response = _create_credential( + {"credential_name": "aws_bedrock", "credential_values": {"aws_access_key_id": "new"}, "credential_info": {}}, + ) + + assert response.status_code == 409, f"name collision answered {response.status_code}: {response.text}" + message = response.json()["error"]["message"] + assert message == ( + "Credential 'aws_bedrock' already exists. Update it with PATCH /credentials/aws_bedrock, or delete it first." + ), f"the operator reads this message verbatim: {message}" + assert "Unique constraint" not in response.text, f"the Prisma internals must not leak: {response.text}" + + +def test_create_credential_still_answers_500_when_the_write_fails_for_another_reason(credential_store): + credential_store(create=AsyncMock(side_effect=Exception("connection reset by peer"))) + + response = _create_credential( + {"credential_name": "aws_bedrock", "credential_values": {"aws_access_key_id": "new"}, "credential_info": {}}, + ) + + assert response.status_code == 500, f"database fault answered {response.status_code}: {response.text}" + + +def test_create_credential_still_answers_200_for_a_name_that_is_free(credential_store): + find_by_name = AsyncMock() + credential_store(find_by_name=find_by_name, create=AsyncMock(return_value=None)) + + response = _create_credential( + {"credential_name": "brand_new", "credential_values": {"aws_access_key_id": "new"}, "credential_info": {}}, + ) + + assert response.status_code == 200, response.text + assert response.json()["success"] is True + find_by_name.assert_not_awaited(), "the unique index is the guard; create must not add a lookup" + + +def test_update_credential_resolves_credential_values_from_model_id_like_create(credential_store): + """Regression: PATCH dropped ``model_id`` from the body, so an update that named a + deployment instead of raw values wrote whatever the caller sent, or nothing.""" + stored = CredentialItem( + credential_name="from-deployment", + credential_values={"api_key": "sk-old"}, + credential_info={}, + ) + update_by_name = AsyncMock(return_value=None) + router = MagicMock() + router.get_deployment.return_value = {"model_name": "gpt-5.2"} + router.get_deployment_credentials.return_value = {"api_key": "sk-from-deployment"} + credential_store(find_by_name=AsyncMock(return_value=stored), update_by_name=update_by_name, llm_router=router) + + response = _patch_credential( + "from-deployment", + {"credential_name": "from-deployment", "model_id": "deployment-1", "credential_info": {}}, + ) + + assert response.status_code == 200, response.text + router.get_deployment_credentials.assert_called_once_with("deployment-1") + written = json.loads(update_by_name.await_args.kwargs["data"]["credential_values"]) + assert set(written) == {"api_key"} + assert written["api_key"] != "sk-old", "the deployment's values must replace the stored ones" + assert written["api_key"] != "sk-from-deployment", "values are encrypted before they reach the table" + + +def test_update_credential_answers_404_when_model_id_names_no_deployment(credential_store): + stored = CredentialItem( + credential_name="from-deployment", credential_values={"api_key": "sk-old"}, credential_info={} + ) + update_by_name = AsyncMock(return_value=None) + router = MagicMock() + router.get_deployment.return_value = None + credential_store(find_by_name=AsyncMock(return_value=stored), update_by_name=update_by_name, llm_router=router) + + response = _patch_credential( + "from-deployment", + {"credential_name": "from-deployment", "model_id": "no-such-deployment", "credential_info": {}}, + ) + + assert response.status_code == 404, response.text + update_by_name.assert_not_awaited() + + +def test_update_credential_answers_500_when_model_id_is_given_but_no_router_is_loaded(credential_store): + stored = CredentialItem( + credential_name="from-deployment", credential_values={"api_key": "sk-old"}, credential_info={} + ) + update_by_name = AsyncMock(return_value=None) + credential_store(find_by_name=AsyncMock(return_value=stored), update_by_name=update_by_name, llm_router=None) + + response = _patch_credential( + "from-deployment", + {"credential_name": "from-deployment", "model_id": "deployment-1", "credential_info": {}}, + ) + + assert response.status_code == 500, response.text + update_by_name.assert_not_awaited() + + +def test_update_credential_still_accepts_a_body_without_credential_values(credential_store): + """Renaming or re-tagging a credential sends only ``credential_info``; that must not 422.""" + stored = CredentialItem(credential_name="existing", credential_values={"api_key": "sk-old"}, credential_info={}) + update_by_name = AsyncMock(return_value=None) + credential_store(find_by_name=AsyncMock(return_value=stored), update_by_name=update_by_name) + + response = _patch_credential( + "existing", + {"credential_name": "existing", "credential_info": {"custom_llm_provider": "openai"}}, + ) + + assert response.status_code == 200, response.text + written = update_by_name.await_args.kwargs["data"] + assert json.loads(written["credential_info"]) == {"custom_llm_provider": "openai"} + assert set(json.loads(written["credential_values"])) == {"api_key"}, "stored values survive an info-only patch" diff --git a/ui/litellm-dashboard/src/lib/http/schema.d.ts b/ui/litellm-dashboard/src/lib/http/schema.d.ts index 0b0e3e18215..1a2d74063b7 100644 --- a/ui/litellm-dashboard/src/lib/http/schema.d.ts +++ b/ui/litellm-dashboard/src/lib/http/schema.d.ts @@ -37944,6 +37944,21 @@ export interface components { */ blocked_users: string[]; }; + /** UpdateCredentialItem */ + UpdateCredentialItem: { + /** Credential Info */ + credential_info: { + [key: string]: unknown; + }; + /** Credential Name */ + credential_name: string; + /** Credential Values */ + credential_values?: { + [key: string]: unknown; + } | null; + /** Model Id */ + model_id?: string | null; + }; /** * UpdateCustomerRequest * @description Update a Customer, use this to update customer budgets etc @@ -45535,7 +45550,7 @@ export interface operations { }; requestBody: { content: { - "application/json": components["schemas"]["CredentialItem"]; + "application/json": components["schemas"]["UpdateCredentialItem"]; }; }; responses: { From a7f180fdd8fca129ee58ffa1a24f6e32df0df21c Mon Sep 17 00:00:00 2001 From: ryan-crabbe-berri Date: Mon, 14 Sep 2026 15:18:22 -0700 Subject: [PATCH 3/3] feat(terraform): make credential adoption opt-in, escape names in request URLs Terraform's convention is that create does not seize a resource the configuration never made, and credential_values holds secrets that are never read back into state, so a silent takeover overwrites values no plan showed. A name collision now fails with the terraform import command that adopts the existing credential explicitly, and adopt_existing = true opts into taking it over during create. The provider detects the conflict by the proxy's 409 and keeps the Prisma string match as a fallback for older proxies Credential names and model_id went into URLs raw, so a name with a slash or a question mark hit the wrong route. Every credential URL is now built from a package const through fmt.Sprintf with url.PathEscape or url.QueryEscape, which the endpoint audit can resolve. Toggling adopt_existing alone no longer sends a PATCH, so it does not rewrite the stored secret --- terraform/provider/CHANGELOG.md | 4 +- .../provider/docs/resources/credential.md | 1 + .../provider/litellm/resource_credential.go | 9 + .../litellm/resource_credential_crud.go | 145 ++++---- .../litellm/resource_credential_crud_test.go | 350 +++++++++++++++--- terraform/provider/litellm/utils.go | 41 +- 6 files changed, 390 insertions(+), 160 deletions(-) diff --git a/terraform/provider/CHANGELOG.md b/terraform/provider/CHANGELOG.md index 3daee3250b6..06a39d8da20 100644 --- a/terraform/provider/CHANGELOG.md +++ b/terraform/provider/CHANGELOG.md @@ -38,7 +38,9 @@ longer signal it. ### Fixed - **key**: An update that changes `team_id` and fails because the key was already cascade-deleted along with its previous team now recovers by recreating the key under the new team, instead of aborting the apply. The key's absence is confirmed against the proxy first, so an unrelated failure still errors out, and a `team_id` change between two teams that both still exist stays a plain in-place update -- **credential**: `litellm_credential` create now adopts an existing credential on a `credential_name` conflict instead of failing with a 500; `apply` is idempotent again once state loses track of a credential that still exists on the proxy +- **credential**: create now reports a `credential_name` collision as a clear error naming the `terraform import` command that adopts the existing credential, instead of surfacing the proxy's raw 500 with a Prisma `Unique constraint failed` message. New `adopt_existing` argument (default `false`) opts into taking the existing credential over during create, which makes `apply` idempotent again once state loses track of a credential that still exists on the proxy. Requires a proxy that answers 409 on the collision; older proxies are still detected by their 500 message +- **credential**: credential names and `model_id` are now percent-encoded in request URLs, so a name containing `/`, `?`, `#` or spaces reaches the proxy intact instead of being cut at the first reserved character and read, updated or deleted as a different credential +- **credential**: update now sends `model_id`, so a `model_id`-scoped credential keeps resolving its values from that deployment on update and on adoption instead of being overwritten with the literal `credential_values`; needs a proxy from 1.102.0, older proxies ignore the field - **team**: Read now decodes the `team_info` envelope `/team/info` actually returns, so team attributes refresh from the proxy instead of always falling back to the prior state - **key**: Read now unwraps the `info` envelope `/key/info` actually returns; previously reads mapped nothing back into state, so drift on a key was never detected - **key**: Read now picks up `model_rpm_limit`, `model_tpm_limit`, `guardrails`, `tags`, `enforced_params`, `allowed_passthrough_routes`, `rpm_limit_type`, `tpm_limit_type` and `prompts` from `info.metadata`, where the proxy actually stores them; previously they stayed empty in state, so a matching config showed a permanent phantom diff on them and out-of-band changes to them were never detected diff --git a/terraform/provider/docs/resources/credential.md b/terraform/provider/docs/resources/credential.md index 554ac07c395..d75ee33140d 100644 --- a/terraform/provider/docs/resources/credential.md +++ b/terraform/provider/docs/resources/credential.md @@ -130,6 +130,7 @@ The following arguments are supported: * `credential_values` - (Required, Sensitive) Map of sensitive credential values such as API keys, tokens, etc. * `model_id` - (Optional) Model ID associated with this credential. * `credential_info` - (Optional) Map of additional non-sensitive information about the credential. +* `adopt_existing` - (Optional, default `false`) Take over a credential of this name that already exists on the proxy instead of failing. Turning this on overwrites the existing credential's values with the ones in this configuration. ## Attributes Reference diff --git a/terraform/provider/litellm/resource_credential.go b/terraform/provider/litellm/resource_credential.go index f668a46a324..d1e41f6cf56 100644 --- a/terraform/provider/litellm/resource_credential.go +++ b/terraform/provider/litellm/resource_credential.go @@ -39,6 +39,15 @@ func resourceLiteLLMCredential() *schema.Resource { Elem: &schema.Schema{Type: schema.TypeString}, Description: "Sensitive credential values (API keys, tokens, etc.)", }, + "adopt_existing": { + Type: schema.TypeBool, + Optional: true, + Default: false, + Description: "Take over a credential of this name that already exists on the proxy instead of failing. " + + "Off by default: create reports the conflict and points at `terraform import`, so an apply never " + + "silently overwrites a credential it does not manage. Turning this on overwrites the existing " + + "credential's values with the ones in this configuration.", + }, }, } } diff --git a/terraform/provider/litellm/resource_credential_crud.go b/terraform/provider/litellm/resource_credential_crud.go index cb5031ee04e..6b31a03d404 100644 --- a/terraform/provider/litellm/resource_credential_crud.go +++ b/terraform/provider/litellm/resource_credential_crud.go @@ -1,15 +1,23 @@ package litellm import ( + "errors" "fmt" "log" "net/http" + "net/url" "strings" "time" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" ) +const ( + endpointCredential = "/credentials/%s" + endpointCredentialByName = "/credentials/by_name/%s" + endpointCredentialByNameForModel = "/credentials/by_name/%s?model_id=%s" +) + // retryCredentialRead attempts to read a credential with exponential backoff. // If the read path clears the ID (e.g., transient 404 right after create), // we treat it as retryable instead of accepting an empty state. @@ -53,34 +61,28 @@ func retryCredentialRead(d *schema.ResourceData, m interface{}, maxRetries int) return err } -func resourceLiteLLMCredentialCreate(d *schema.ResourceData, m interface{}) error { - client := m.(*Client) - - credentialName := d.Get("credential_name").(string) - modelID := d.Get("model_id").(string) - credentialInfo := d.Get("credential_info").(map[string]interface{}) - credentialValues := d.Get("credential_values").(map[string]interface{}) - - // Convert credential_info to map[string]interface{} for JSON +func credentialRequestFromResource(d *schema.ResourceData, credentialName string) CredentialRequest { credInfoMap := make(map[string]interface{}) - for k, v := range credentialInfo { + for k, v := range d.Get("credential_info").(map[string]interface{}) { credInfoMap[k] = v } - - // Convert credential_values to map[string]interface{} for JSON credValuesMap := make(map[string]interface{}) - for k, v := range credentialValues { + for k, v := range d.Get("credential_values").(map[string]interface{}) { credValuesMap[k] = v } - - credentialRequest := CredentialRequest{ + return CredentialRequest{ CredentialName: credentialName, - ModelID: modelID, + ModelID: d.Get("model_id").(string), CredentialInfo: credInfoMap, CredentialValues: credValuesMap, } +} - resp, err := MakeRequest(client, "POST", "/credentials", credentialRequest) +func resourceLiteLLMCredentialCreate(d *schema.ResourceData, m interface{}) error { + client := m.(*Client) + credentialName := d.Get("credential_name").(string) + + resp, err := MakeRequest(client, "POST", "/credentials", credentialRequestFromResource(d, credentialName)) if err != nil { return fmt.Errorf("failed to create credential: %w", err) } @@ -88,45 +90,51 @@ func resourceLiteLLMCredentialCreate(d *schema.ResourceData, m interface{}) erro err = handleCredentialAPIResponse(resp, nil, client) if err != nil { - // If a credential with this name already exists, adopt it instead of - // failing: take ownership and update the existing credential's - // values (merged onto whatever it already had - not a full replace) - // rather than erroring on the unique-constraint conflict. See - // https://github.com/BerriAI/terraform-provider-litellm/issues/8. - if err.Error() == "credential_conflict" { - log.Printf("[WARN] Credential %q already exists; adopting it and updating to match configuration.", credentialName) - d.SetId(credentialName) - if updateErr := resourceLiteLLMCredentialUpdate(d, m); updateErr != nil { - // Adoption failed before this run took ownership of - // anything real. Clear the ID so create is reported as - // failed outright (matching pre-adoption behavior) instead - // of tainting state for a credential this run doesn't own - - // state that would otherwise get destroyed on the next - // apply. - d.SetId("") - return fmt.Errorf("failed to adopt existing credential %q: %w", credentialName, updateErr) - } - return nil + if errors.Is(err, errCredentialConflict) { + return handleCredentialNameConflict(d, m, credentialName) } return fmt.Errorf("failed to create credential: %w", err) } - // Set the resource ID to the credential name d.SetId(credentialName) log.Printf("[INFO] Credential created with name %s. Starting retry mechanism to read the credential...", credentialName) return retryCredentialRead(d, m, 5) } +func handleCredentialNameConflict(d *schema.ResourceData, m interface{}, credentialName string) error { + if !d.Get("adopt_existing").(bool) { + return fmt.Errorf( + "credential %q already exists on the proxy but is not in Terraform state. "+ + "Import it to manage it here:\n\n"+ + " terraform import litellm_credential. %s\n\n"+ + "The next apply then updates it to match this configuration. To take it over during "+ + "create instead, set adopt_existing = true on this resource, which overwrites the "+ + "existing credential's values with the ones configured here", + credentialName, shellSingleQuote(credentialName), + ) + } + + log.Printf("[WARN] Credential %q already exists; adopt_existing is set, so taking it over and updating it to match configuration.", credentialName) + d.SetId(credentialName) + if err := patchCredential(m.(*Client), d, credentialName); err != nil { + d.SetId("") + return fmt.Errorf("failed to adopt existing credential %q: %w", credentialName, err) + } + return retryCredentialRead(d, m, 5) +} + +func shellSingleQuote(s string) string { + return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'" +} + func resourceLiteLLMCredentialRead(d *schema.ResourceData, m interface{}) error { client := m.(*Client) credentialName := d.Id() - // Try to get credential by name first - modelID := d.Get("model_id").(string) - endpoint := fmt.Sprintf("/credentials/by_name/%s", credentialName) - if modelID != "" { - endpoint += fmt.Sprintf("?model_id=%s", modelID) + endpoint := fmt.Sprintf(endpointCredentialByName, url.PathEscape(credentialName)) + if modelID := d.Get("model_id").(string); modelID != "" { + endpoint = fmt.Sprintf(endpointCredentialByNameForModel, url.PathEscape(credentialName), url.QueryEscape(modelID)) } resp, err := MakeRequest(client, "GET", endpoint, nil) @@ -158,48 +166,28 @@ func resourceLiteLLMCredentialRead(d *schema.ResourceData, m interface{}) error return nil } -func resourceLiteLLMCredentialUpdate(d *schema.ResourceData, m interface{}) error { - client := m.(*Client) - credentialName := d.Id() - - modelID := d.Get("model_id").(string) - credentialInfo := d.Get("credential_info").(map[string]interface{}) - credentialValues := d.Get("credential_values").(map[string]interface{}) - - // Convert credential_info to map[string]interface{} for JSON - credInfoMap := make(map[string]interface{}) - for k, v := range credentialInfo { - credInfoMap[k] = v - } - - // Convert credential_values to map[string]interface{} for JSON - credValuesMap := make(map[string]interface{}) - for k, v := range credentialValues { - credValuesMap[k] = v - } - - // model_id must travel with the update the same way it does on create, - // so the proxy's model-based credential resolution still applies. Without - // it, updating (or adopting) a model_id-scoped credential silently loses - // that association. - credentialRequest := CredentialRequest{ - CredentialName: credentialName, - ModelID: modelID, - CredentialInfo: credInfoMap, - CredentialValues: credValuesMap, - } - - endpoint := fmt.Sprintf("/credentials/%s", credentialName) - resp, err := MakeRequest(client, "PATCH", endpoint, credentialRequest) +func patchCredential(client *Client, d *schema.ResourceData, credentialName string) error { + resp, err := MakeRequest(client, "PATCH", fmt.Sprintf(endpointCredential, url.PathEscape(credentialName)), credentialRequestFromResource(d, credentialName)) if err != nil { return fmt.Errorf("failed to update credential: %w", err) } defer resp.Body.Close() - err = handleCredentialAPIResponse(resp, nil, client) - if err != nil { + if err := handleCredentialAPIResponse(resp, nil, client); err != nil { return fmt.Errorf("failed to update credential: %w", err) } + return nil +} + +func resourceLiteLLMCredentialUpdate(d *schema.ResourceData, m interface{}) error { + if !d.HasChangesExcept("adopt_existing") { + return nil + } + + credentialName := d.Id() + if err := patchCredential(m.(*Client), d, credentialName); err != nil { + return err + } log.Printf("[INFO] Credential updated with name %s. Starting retry mechanism to read the credential...", credentialName) return retryCredentialRead(d, m, 5) @@ -209,8 +197,7 @@ func resourceLiteLLMCredentialDelete(d *schema.ResourceData, m interface{}) erro client := m.(*Client) credentialName := d.Id() - endpoint := fmt.Sprintf("/credentials/%s", credentialName) - resp, err := MakeRequest(client, "DELETE", endpoint, nil) + resp, err := MakeRequest(client, "DELETE", fmt.Sprintf(endpointCredential, url.PathEscape(credentialName)), nil) if err != nil { return fmt.Errorf("failed to delete credential: %w", err) } diff --git a/terraform/provider/litellm/resource_credential_crud_test.go b/terraform/provider/litellm/resource_credential_crud_test.go index 6e0b818fe33..02ae7ef0671 100644 --- a/terraform/provider/litellm/resource_credential_crud_test.go +++ b/terraform/provider/litellm/resource_credential_crud_test.go @@ -1,15 +1,18 @@ package litellm import ( + "context" "encoding/json" "fmt" "io" "net/http" "net/http/httptest" + "strings" "sync/atomic" "testing" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" + "github.com/hashicorp/terraform-plugin-sdk/v2/terraform" ) // newTestResourceData creates a *schema.ResourceData with the credential schema, @@ -201,10 +204,30 @@ func TestRetryCredentialRead_ConnectionError(t *testing.T) { fmt.Printf("connection error (expected): %v\n", err) } -// conflictServer builds the shared conflict-then-recover mock used by the -// adoption tests below. patchStatus/patchBody control the PATCH response, so -// callers can exercise both the success and failure paths. -func conflictServer(t *testing.T, patchStatus int, patchBody string) (*httptest.Server, *int32, *int32, *[]byte) { +type conflictBody struct { + status int + body string +} + +var ( + modernConflictBody = conflictBody{ + status: http.StatusConflict, + body: `{"error":{"message":"Credential 'conflict-test' already exists. Update it with PATCH /credentials/conflict-test, or delete it first.","type":"internal_server_error","param":"None","code":"409"}}`, + } + legacyConflictBody = conflictBody{ + status: http.StatusInternalServerError, + body: `{"error":{"message":"Unique constraint failed on the fields: (` + "`credential_name`" + `)","type":"internal_server_error","code":"500"}}`, + } +) + +type conflictServerOptions struct { + conflict conflictBody + patchStatus int + patchBody string + getStatus int +} + +func conflictServer(t *testing.T, opts conflictServerOptions) (*httptest.Server, *int32, *int32, *[]byte) { t.Helper() var createCalls, patchCalls int32 var capturedPatchBody []byte @@ -213,8 +236,8 @@ func conflictServer(t *testing.T, patchStatus int, patchBody string) (*httptest. case r.Method == http.MethodPost && r.URL.Path == "/credentials": atomic.AddInt32(&createCalls, 1) w.Header().Set("Content-Type", "application/json") - w.WriteHeader(http.StatusInternalServerError) - w.Write([]byte(`{"error":{"message":"Unique constraint failed on the fields: (` + "`credential_name`" + `)","type":"internal_server_error","code":"500"}}`)) + w.WriteHeader(opts.conflict.status) + w.Write([]byte(opts.conflict.body)) case r.Method == http.MethodPatch: atomic.AddInt32(&patchCalls, 1) if r.URL.Path != "/credentials/conflict-test" { @@ -223,12 +246,17 @@ func conflictServer(t *testing.T, patchStatus int, patchBody string) (*httptest. body, _ := io.ReadAll(r.Body) capturedPatchBody = body w.Header().Set("Content-Type", "application/json") - w.WriteHeader(patchStatus) - w.Write([]byte(patchBody)) + w.WriteHeader(opts.patchStatus) + w.Write([]byte(opts.patchBody)) case r.Method == http.MethodGet: if r.URL.Path != "/credentials/by_name/conflict-test" || r.URL.Query().Get("model_id") != "model-1" { t.Errorf("GET went to %q (query %q), want /credentials/by_name/conflict-test?model_id=model-1", r.URL.Path, r.URL.RawQuery) } + if opts.getStatus != 0 && opts.getStatus != http.StatusOK { + w.WriteHeader(opts.getStatus) + w.Write([]byte(`{"error":{"message":"Internal Server Error"}}`)) + return + } resp := CredentialResponse{CredentialName: "conflict-test", CredentialInfo: map[string]interface{}{}} body, _ := json.Marshal(resp) w.Header().Set("Content-Type", "application/json") @@ -241,66 +269,114 @@ func conflictServer(t *testing.T, patchStatus int, patchBody string) (*httptest. return srv, &createCalls, &patchCalls, &capturedPatchBody } -// A credential that already exists in LiteLLM (created out of band, or left -// behind by a prior apply that dropped state) must be adopted on create -// instead of failing on the credential_name unique-constraint conflict, and -// the adopt PATCH must carry model_id so model-based credential resolution -// still applies (previously dropped - see -// https://github.com/BerriAI/litellm/pull/39745). -func TestResourceLiteLLMCredentialCreate_AdoptsOnConflict(t *testing.T) { - srv, createCalls, patchCalls, patchBody := conflictServer(t, http.StatusOK, `{}`) - defer srv.Close() - - client := NewClient(srv.URL, "test-key", true) - d := schema.TestResourceDataRaw(t, resourceLiteLLMCredential().Schema, map[string]interface{}{ +func adoptTestData(t *testing.T, adoptExisting bool) *schema.ResourceData { + t.Helper() + return schema.TestResourceDataRaw(t, resourceLiteLLMCredential().Schema, map[string]interface{}{ "credential_name": "conflict-test", "model_id": "model-1", "credential_info": map[string]interface{}{"custom_llm_provider": "bedrock"}, "credential_values": map[string]interface{}{"aws_access_key_id": "val"}, + "adopt_existing": adoptExisting, }) +} - if err := resourceLiteLLMCredentialCreate(d, client); err != nil { - t.Fatalf("expected create to adopt the existing credential, got error: %v", err) - } - if d.Id() != "conflict-test" { - t.Fatalf("expected ID %q, got %q", "conflict-test", d.Id()) - } - if got := atomic.LoadInt32(createCalls); got != 1 { - t.Fatalf("expected exactly 1 POST /credentials call, got %d", got) - } - if got := atomic.LoadInt32(patchCalls); got != 1 { - t.Fatalf("expected the conflict to trigger exactly 1 PATCH (adopt-and-update), got %d", got) - } +func TestResourceLiteLLMCredentialCreate_AdoptsOnConflictWhenOptedIn(t *testing.T) { + for _, tc := range []struct { + name string + conflict conflictBody + }{ + {"typed 409", modernConflictBody}, + {"legacy 500 with unique-constraint message", legacyConflictBody}, + } { + t.Run(tc.name, func(t *testing.T) { + srv, createCalls, patchCalls, patchBody := conflictServer(t, conflictServerOptions{conflict: tc.conflict, patchStatus: http.StatusOK, patchBody: `{}`}) + defer srv.Close() - var sent map[string]interface{} - if err := json.Unmarshal(*patchBody, &sent); err != nil { - t.Fatalf("PATCH body was not valid JSON: %v (%s)", err, *patchBody) - } - if sent["credential_name"] != "conflict-test" { - t.Errorf("PATCH body credential_name = %v, want conflict-test", sent["credential_name"]) - } - if sent["model_id"] != "model-1" { - t.Errorf("PATCH body model_id = %v, want model-1 (adoption must not drop model-based credential resolution)", sent["model_id"]) - } - credInfo, _ := sent["credential_info"].(map[string]interface{}) - if credInfo["custom_llm_provider"] != "bedrock" { - t.Errorf("PATCH body credential_info = %v, want custom_llm_provider=bedrock", sent["credential_info"]) + client := NewClient(srv.URL, "test-key", true) + d := adoptTestData(t, true) + + if err := resourceLiteLLMCredentialCreate(d, client); err != nil { + t.Fatalf("expected create to adopt the existing credential, got error: %v", err) + } + if d.Id() != "conflict-test" { + t.Fatalf("expected ID %q, got %q", "conflict-test", d.Id()) + } + if got := atomic.LoadInt32(createCalls); got != 1 { + t.Fatalf("expected exactly 1 POST /credentials call, got %d", got) + } + if got := atomic.LoadInt32(patchCalls); got != 1 { + t.Fatalf("expected the conflict to trigger exactly 1 PATCH (adopt-and-update), got %d", got) + } + + var sent map[string]interface{} + if err := json.Unmarshal(*patchBody, &sent); err != nil { + t.Fatalf("PATCH body was not valid JSON: %v (%s)", err, *patchBody) + } + if sent["credential_name"] != "conflict-test" { + t.Errorf("PATCH body credential_name = %v, want conflict-test", sent["credential_name"]) + } + if sent["model_id"] != "model-1" { + t.Errorf("PATCH body model_id = %v, want model-1 (adoption must not drop model-based credential resolution)", sent["model_id"]) + } + credInfo, _ := sent["credential_info"].(map[string]interface{}) + if credInfo["custom_llm_provider"] != "bedrock" { + t.Errorf("PATCH body credential_info = %v, want custom_llm_provider=bedrock", sent["credential_info"]) + } + }) + } +} + +func TestResourceLiteLLMCredentialCreate_ConflictWithoutOptInFailsWithImportHint(t *testing.T) { + for _, tc := range []struct { + name string + conflict conflictBody + }{ + {"typed 409", modernConflictBody}, + {"legacy 500 with unique-constraint message", legacyConflictBody}, + } { + t.Run(tc.name, func(t *testing.T) { + srv, createCalls, patchCalls, _ := conflictServer(t, conflictServerOptions{conflict: tc.conflict, patchStatus: http.StatusOK, patchBody: `{}`}) + defer srv.Close() + + client := NewClient(srv.URL, "test-key", true) + d := adoptTestData(t, false) + + err := resourceLiteLLMCredentialCreate(d, client) + if err == nil { + t.Fatal("expected create to fail on the conflict when adopt_existing is unset, got nil") + } + if got := atomic.LoadInt32(createCalls); got != 1 { + t.Fatalf("expected exactly 1 POST /credentials call, got %d", got) + } + if got := atomic.LoadInt32(patchCalls); got != 0 { + t.Fatalf("expected no PATCH without adopt_existing - create must not overwrite an unmanaged credential - got %d", got) + } + if d.Id() != "" { + t.Fatalf("resource ID must stay empty when create refuses the conflict, got %q", d.Id()) + } + for _, want := range []string{ + "already exists", + `terraform import litellm_credential. 'conflict-test'`, + "adopt_existing = true", + } { + if !strings.Contains(err.Error(), want) { + t.Errorf("error must tell the operator how to proceed; missing %q in: %v", want, err) + } + } + }) } } -// If the adopt PATCH itself fails, create must not have set the resource ID -// for a credential this run doesn't own - otherwise Terraform taints the -// entry and the *next* apply destroys a credential nobody here created. func TestResourceLiteLLMCredentialCreate_FailedAdoptDoesNotTaint(t *testing.T) { - srv, createCalls, patchCalls, _ := conflictServer(t, http.StatusInternalServerError, `{"error":{"message":"Internal Server Error"}}`) + srv, createCalls, patchCalls, _ := conflictServer(t, conflictServerOptions{ + conflict: modernConflictBody, + patchStatus: http.StatusInternalServerError, + patchBody: `{"error":{"message":"Internal Server Error"}}`, + }) defer srv.Close() client := NewClient(srv.URL, "test-key", true) - d := schema.TestResourceDataRaw(t, resourceLiteLLMCredential().Schema, map[string]interface{}{ - "credential_name": "conflict-test", - "credential_info": map[string]interface{}{}, - "credential_values": map[string]interface{}{"key": "val"}, - }) + d := adoptTestData(t, true) err := resourceLiteLLMCredentialCreate(d, client) if err == nil { @@ -317,8 +393,6 @@ func TestResourceLiteLLMCredentialCreate_FailedAdoptDoesNotTaint(t *testing.T) { } } -// A non-conflict failure (a plain 500, for example) must return the original -// error and never attempt to adopt anything. func TestResourceLiteLLMCredentialCreate_NonConflictErrorDoesNotAdopt(t *testing.T) { var createCalls, patchCalls int32 srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -343,6 +417,7 @@ func TestResourceLiteLLMCredentialCreate_NonConflictErrorDoesNotAdopt(t *testing "credential_name": "some-cred", "credential_info": map[string]interface{}{}, "credential_values": map[string]interface{}{"key": "val"}, + "adopt_existing": true, }) err := resourceLiteLLMCredentialCreate(d, client) @@ -356,3 +431,166 @@ func TestResourceLiteLLMCredentialCreate_NonConflictErrorDoesNotAdopt(t *testing t.Fatalf("resource ID must stay empty on a non-conflict failure, got %q", d.Id()) } } + +func TestResourceLiteLLMCredentialCreate_AdoptKeepsIDWhenPostPatchReadFails(t *testing.T) { + srv, _, patchCalls, _ := conflictServer(t, conflictServerOptions{ + conflict: modernConflictBody, + patchStatus: http.StatusOK, + patchBody: `{}`, + getStatus: http.StatusInternalServerError, + }) + defer srv.Close() + + client := NewClient(srv.URL, "test-key", true) + d := adoptTestData(t, true) + + err := resourceLiteLLMCredentialCreate(d, client) + if err == nil { + t.Fatal("expected the failed post-adopt read to surface as an error, got nil") + } + if got := atomic.LoadInt32(patchCalls); got != 1 { + t.Fatalf("expected exactly 1 PATCH, got %d", got) + } + if d.Id() != "conflict-test" { + t.Fatalf("the PATCH already overwrote the remote credential, so the ID must stay set for Terraform to track it; got %q", d.Id()) + } +} + +func TestResourceLiteLLMCredentialImportHintQuotesTheNameForTheShell(t *testing.T) { + for _, tc := range []struct { + name string + want string + }{ + {"my cred", `'my cred'`}, + {"it's $HOME `id` \"x\"", `'it'\''s $HOME ` + "`id`" + ` "x"'`}, + } { + t.Run(tc.name, func(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusConflict) + w.Write([]byte(`{"error":{"message":"already exists","code":"409"}}`)) + })) + defer srv.Close() + + d := schema.TestResourceDataRaw(t, resourceLiteLLMCredential().Schema, map[string]interface{}{ + "credential_name": tc.name, + "credential_info": map[string]interface{}{}, + "credential_values": map[string]interface{}{"key": "val"}, + }) + + err := resourceLiteLLMCredentialCreate(d, NewClient(srv.URL, "test-key", true)) + if err == nil { + t.Fatal("expected the conflict to fail create, got nil") + } + want := "terraform import litellm_credential. " + tc.want + if !strings.Contains(err.Error(), want) { + t.Fatalf("import hint must single-quote the name for the shell; missing %q in: %v", want, err) + } + }) + } +} + +func TestCredentialRequestsEscapeReservedCharactersInTheName(t *testing.T) { + const name = "team/a?b c" + var paths []string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + paths = append(paths, r.Method+" "+r.URL.EscapedPath()+"?"+r.URL.RawQuery) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + w.Write([]byte(`{"credential_name":"` + name + `","credential_info":{}}`)) + })) + defer srv.Close() + + client := NewClient(srv.URL, "test-key", true) + d := schema.TestResourceDataRaw(t, resourceLiteLLMCredential().Schema, map[string]interface{}{ + "credential_name": name, + "model_id": "m&1", + "credential_info": map[string]interface{}{}, + "credential_values": map[string]interface{}{"key": "val"}, + }) + d.SetId(name) + + if err := resourceLiteLLMCredentialRead(d, client); err != nil { + t.Fatalf("read failed: %v", err) + } + if err := patchCredential(client, d, name); err != nil { + t.Fatalf("patch failed: %v", err) + } + if err := resourceLiteLLMCredentialDelete(d, client); err != nil { + t.Fatalf("delete failed: %v", err) + } + + want := []string{ + "GET /credentials/by_name/team%2Fa%3Fb%20c?model_id=m%261", + "PATCH /credentials/team%2Fa%3Fb%20c?", + "DELETE /credentials/team%2Fa%3Fb%20c?", + } + if strings.Join(paths, "\n") != strings.Join(want, "\n") { + t.Fatalf("request paths:\n%s\nwant:\n%s", strings.Join(paths, "\n"), strings.Join(want, "\n")) + } +} + +func TestResourceLiteLLMCredentialUpdate_TogglingAdoptExistingSendsNoPatch(t *testing.T) { + var patchCalls int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodPatch { + atomic.AddInt32(&patchCalls, 1) + } + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + w.Write([]byte(`{"credential_name":"cred-1","credential_info":{}}`)) + })) + defer srv.Close() + + res := resourceLiteLLMCredential() + priorData := schema.TestResourceDataRaw(t, res.Schema, map[string]interface{}{ + "credential_name": "cred-1", + "credential_info": map[string]interface{}{}, + "credential_values": map[string]interface{}{"api_key": "sk-secret"}, + "adopt_existing": false, + }) + priorData.SetId("cred-1") + prior := priorData.State() + + toggled := terraform.NewResourceConfigRaw(map[string]interface{}{ + "credential_name": "cred-1", + "credential_info": map[string]interface{}{}, + "credential_values": map[string]interface{}{"api_key": "sk-secret"}, + "adopt_existing": true, + }) + diff, err := res.Diff(context.Background(), prior, toggled, nil) + if err != nil { + t.Fatalf("diff failed: %v", err) + } + d, err := schema.InternalMap(res.Schema).Data(prior, diff) + if err != nil { + t.Fatalf("data failed: %v", err) + } + if err := resourceLiteLLMCredentialUpdate(d, NewClient(srv.URL, "test-key", true)); err != nil { + t.Fatalf("update failed: %v", err) + } + if got := atomic.LoadInt32(&patchCalls); got != 0 { + t.Fatalf("flipping adopt_existing alone must not rewrite the credential's secrets; got %d PATCH calls", got) + } + + rotated := terraform.NewResourceConfigRaw(map[string]interface{}{ + "credential_name": "cred-1", + "credential_info": map[string]interface{}{}, + "credential_values": map[string]interface{}{"api_key": "sk-rotated"}, + "adopt_existing": true, + }) + diff, err = res.Diff(context.Background(), prior, rotated, nil) + if err != nil { + t.Fatalf("diff failed: %v", err) + } + d, err = schema.InternalMap(res.Schema).Data(prior, diff) + if err != nil { + t.Fatalf("data failed: %v", err) + } + if err := resourceLiteLLMCredentialUpdate(d, NewClient(srv.URL, "test-key", true)); err != nil { + t.Fatalf("update failed: %v", err) + } + if got := atomic.LoadInt32(&patchCalls); got != 1 { + t.Fatalf("a real value change must still PATCH; got %d PATCH calls", got) + } +} diff --git a/terraform/provider/litellm/utils.go b/terraform/provider/litellm/utils.go index a123f5d350a..f8f66afba3c 100644 --- a/terraform/provider/litellm/utils.go +++ b/terraform/provider/litellm/utils.go @@ -5,6 +5,7 @@ import ( "crypto/sha256" "encoding/hex" "encoding/json" + "errors" "fmt" "io" "net/http" @@ -202,33 +203,21 @@ func isCredentialNotFoundError(errResp ErrorResponse) bool { return false } -// isCredentialConflictError checks if the error response indicates a credential -// name collision. LiteLLM surfaces this as a 500 carrying the underlying Prisma -// unique-constraint message on credential_name. See -// https://github.com/BerriAI/terraform-provider-litellm/issues/8. -func isCredentialConflictError(errResp ErrorResponse) bool { - if msg, ok := errResp.Error.Message.(string); ok { - if strings.Contains(msg, "Unique constraint failed") && strings.Contains(msg, "credential_name") { - return true - } - } +var errCredentialConflict = errors.New("credential_conflict") +func isLegacyCredentialConflictError(errResp ErrorResponse) bool { + isConflict := func(msg string) bool { + return strings.Contains(msg, "Unique constraint failed") && strings.Contains(msg, "credential_name") + } + if msg, ok := errResp.Error.Message.(string); ok && isConflict(msg) { + return true + } if msgMap, ok := errResp.Error.Message.(map[string]interface{}); ok { - if errStr, ok := msgMap["error"].(string); ok { - if strings.Contains(errStr, "Unique constraint failed") && strings.Contains(errStr, "credential_name") { - return true - } - } - } - - // Check Detail.Error field for LiteLLM proxy error format - if errResp.Detail.Error != "" { - if strings.Contains(errResp.Detail.Error, "Unique constraint failed") && strings.Contains(errResp.Detail.Error, "credential_name") { + if errStr, ok := msgMap["error"].(string); ok && isConflict(errStr) { return true } } - - return false + return isConflict(errResp.Detail.Error) } // handleCredentialAPIResponse handles API responses specifically for credential operations @@ -242,14 +231,18 @@ func handleCredentialAPIResponse(resp *http.Response, result interface{}, client return fmt.Errorf("credential_not_found") } + if resp.StatusCode == http.StatusConflict { + return errCredentialConflict + } + if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusCreated { var errResp ErrorResponse if err := json.Unmarshal(bodyBytes, &errResp); err == nil { if isCredentialNotFoundError(errResp) { return fmt.Errorf("credential_not_found") } - if isCredentialConflictError(errResp) { - return fmt.Errorf("credential_conflict") + if isLegacyCredentialConflictError(errResp) { + return errCredentialConflict } } return fmt.Errorf("API request failed: Status: %s, Response: %s",