From d0fb841c848074fb4a005fbdf170ad71dc89b45b Mon Sep 17 00:00:00 2001 From: Matthew Howard <78384492+matthowardcohere@users.noreply.github.com> Date: Fri, 4 Sep 2026 11:00:21 -0400 Subject: [PATCH] test(credential): assert PATCH path/body, cover model_id, taint, and non-conflict cases Replaces the method-only PATCH/GET stubs (which stayed green even if adoption hit the wrong endpoint or dropped a field) with assertions on the actual request. Adds coverage for the two bugs fixed in this branch, plus the case where a non-conflict error must not adopt. --- .../litellm/resource_credential_crud_test.go | 140 +++++++++++++++--- 1 file changed, 120 insertions(+), 20 deletions(-) diff --git a/terraform/provider/litellm/resource_credential_crud_test.go b/terraform/provider/litellm/resource_credential_crud_test.go index 879f5904723..02b23a358b0 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" @@ -200,11 +201,13 @@ func TestRetryCredentialRead_ConnectionError(t *testing.T) { fmt.Printf("connection error (expected): %v\n", err) } -// 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. -func TestResourceLiteLLMCredentialCreate_AdoptsOnConflict(t *testing.T) { - var createCalls, updateCalls, readCalls int32 +// 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": @@ -213,12 +216,16 @@ func TestResourceLiteLLMCredentialCreate_AdoptsOnConflict(t *testing.T) { 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(&updateCalls, 1) + 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(http.StatusOK) - w.Write([]byte(`{}`)) + w.WriteHeader(patchStatus) + w.Write([]byte(patchBody)) case r.Method == http.MethodGet: - atomic.AddInt32(&readCalls, 1) resp := CredentialResponse{CredentialName: "conflict-test", CredentialInfo: map[string]interface{}{}} body, _ := json.Marshal(resp) w.Header().Set("Content-Type", "application/json") @@ -228,6 +235,61 @@ func TestResourceLiteLLMCredentialCreate_AdoptsOnConflict(t *testing.T) { 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) @@ -237,19 +299,57 @@ func TestResourceLiteLLMCredentialCreate_AdoptsOnConflict(t *testing.T) { "credential_values": map[string]interface{}{"key": "val"}, }) - if err := resourceLiteLLMCredentialCreate(d, client); err != nil { - t.Fatalf("expected create to adopt the existing credential, got error: %v", err) + err := resourceLiteLLMCredentialCreate(d, client) + if err == nil { + t.Fatal("expected an error when the adopt PATCH fails, got nil") } - 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 atomic.LoadInt32(&createCalls) != 1 { - t.Fatalf("expected exactly 1 POST /credentials call, got %d", createCalls) + if got := atomic.LoadInt32(patchCalls); got != 1 { + t.Fatalf("expected exactly 1 PATCH attempt, got %d", got) } - if atomic.LoadInt32(&updateCalls) != 1 { - t.Fatalf("expected the conflict to trigger exactly 1 PATCH (adopt-and-update), got %d", updateCalls) - } - if atomic.LoadInt32(&readCalls) < 1 { - t.Fatalf("expected the post-adopt retry read to run at least once, got %d", readCalls) + 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()) } }