From a109553909270e329b20b4c2b3e4a252023d7f85 Mon Sep 17 00:00:00 2001 From: ryan-crabbe-berri Date: Sat, 12 Sep 2026 11:58:44 -0700 Subject: [PATCH] fix(key): recover from a cascade-deleted key instead of failing the apply Deleting a team cascade-deletes its keys, so `terraform apply -replace` on a team left the key's `/key/update` 404ing and aborted the apply with the key resource stuck. The update now confirms the key is really gone and recreates it under the new team; a `team_id` change between two live teams stays an in-place update, and an unrelated failure still errors out. Rebased onto current staging, which added a typed `apiError` and `isNotFound`, so the recovery matches on the status code plus a re-read rather than on the error string. The metadata pre-read, which fails before `/key/update` is ever reached when the key is gone, routes through the same recovery. Original work by @matthowardcohere in #39747. Claude-Session: https://claude.ai/code/session_01XT1qsbjLwnhiN5sQ2hNUxr --- terraform/provider/CHANGELOG.md | 1 + terraform/provider/litellm/resource_key.go | 36 +++- .../provider/litellm/resource_key_test.go | 190 ++++++++++++++++++ 3 files changed, 222 insertions(+), 5 deletions(-) diff --git a/terraform/provider/CHANGELOG.md b/terraform/provider/CHANGELOG.md index e5e0a164a83..8c0ef5a8b15 100644 --- a/terraform/provider/CHANGELOG.md +++ b/terraform/provider/CHANGELOG.md @@ -37,6 +37,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 - **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_key.go b/terraform/provider/litellm/resource_key.go index 018d01f75a8..39546d588df 100644 --- a/terraform/provider/litellm/resource_key.go +++ b/terraform/provider/litellm/resource_key.go @@ -3,6 +3,7 @@ package litellm import ( "context" "encoding/json" + "errors" "fmt" "log" @@ -321,19 +322,42 @@ func resourceKeyUpdate(ctx context.Context, d *schema.ResourceData, m interface{ metadata, err := plannedKeyMetadata(c, d) if err != nil { - d.Partial(true) - return diag.FromErr(fmt.Errorf("error updating key: %s", err)) + return failedKeyUpdate(ctx, d, m, err) } key.Metadata = metadata if _, err := c.UpdateKey(key); err != nil { - d.Partial(true) - return diag.FromErr(fmt.Errorf("error updating key: %s", err)) + return failedKeyUpdate(ctx, d, m, err) } return resourceKeyRead(ctx, d, m) } +// Deleting a team cascade-deletes its keys, so an apply that moves a key onto a +// replacement team can find the key already gone, and recreating it is the only +// way forward. Confirming it is really gone keeps an unrelated 404 (a rejected +// project_id, say) a hard failure rather than silently orphaning a live key. +func failedKeyUpdate(ctx context.Context, d *schema.ResourceData, m interface{}, err error) diag.Diagnostics { + c := m.(*Client) + if d.HasChange("team_id") && keyIsGone(c, d.Id(), err) { + log.Printf("[WARN] Key %q no longer exists, most likely cascade-deleted with its previous team; recreating it under the new team_id", d.Id()) + return resourceKeyCreate(ctx, d, m) + } + d.Partial(true) + return diag.FromErr(fmt.Errorf("error updating key: %s", err)) +} + +func keyIsGone(c *Client, keyID string, err error) bool { + if errors.Is(err, errKeyGone) { + return true + } + if !isNotFound(err) { + return false + } + key, getErr := c.GetKey(keyID) + return getErr == nil && key == nil +} + func changedMap(d *schema.ResourceData, name string) map[string]interface{} { if !d.HasChange(name) { return nil @@ -341,6 +365,8 @@ func changedMap(d *schema.ResourceData, name string) map[string]interface{} { return d.Get(name).(map[string]interface{}) } +var errKeyGone = errors.New("no longer exists") + func plannedKeyMetadata(c *Client, d *schema.ResourceData) (map[string]interface{}, error) { if !d.HasChange("metadata") { return nil, nil @@ -350,7 +376,7 @@ func plannedKeyMetadata(c *Client, d *schema.ResourceData) (map[string]interface return nil, err } if current == nil { - return nil, fmt.Errorf("key %s no longer exists", d.Id()) + return nil, fmt.Errorf("key %s %w", d.Id(), errKeyGone) } oldDeclared, newDeclared := d.GetChange("metadata") return mergeKeyMetadata(current.Metadata, oldDeclared.(map[string]interface{}), newDeclared.(map[string]interface{})), nil diff --git a/terraform/provider/litellm/resource_key_test.go b/terraform/provider/litellm/resource_key_test.go index 66291eadcc5..fe708edd3d3 100644 --- a/terraform/provider/litellm/resource_key_test.go +++ b/terraform/provider/litellm/resource_key_test.go @@ -7,8 +7,10 @@ import ( "net/http" "net/http/httptest" "reflect" + "sync/atomic" "testing" + "github.com/hashicorp/terraform-plugin-sdk/v2/diag" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "github.com/hashicorp/terraform-plugin-sdk/v2/terraform" ) @@ -686,3 +688,191 @@ func TestKeyUpdateOmitsUnchangedDuration(t *testing.T) { t.Errorf("update payload unexpectedly contains duration = %v", v) } } + +// newKeyUpdateResourceData builds a *schema.ResourceData reflecting a real +// state -> config diff for team_id (unlike schema.TestResourceDataRaw, which +// has no notion of prior state), so d.HasChange("team_id") behaves the way it +// does during a real Update call. +func newKeyUpdateResourceData(t *testing.T, id, oldTeamID, newTeamID string) *schema.ResourceData { + t.Helper() + state := &terraform.InstanceState{ID: id, Attributes: map[string]string{"team_id": oldTeamID}} + diff := &terraform.InstanceDiff{Attributes: map[string]*terraform.ResourceAttrDiff{ + "team_id": {Old: oldTeamID, New: newTeamID}, + }} + d, err := schema.InternalMap(resourceKey().Schema).Data(state, diff) + if err != nil { + t.Fatalf("building ResourceData returned error: %v", err) + } + return d +} + +// keyRecoveryProxy fakes the two responses the cascade-delete recovery path +// turns on: what POST /key/update returns, and whether GET /key/info still +// finds the key afterwards. +type keyRecoveryProxy struct { + updateStatus int + updateBody string + staleKeyGone bool + updateCalls int32 + generateCalls int32 +} + +const keyNotFoundBody = `{"error":{"message":"Key not found.","type":"not_found_error","param":"key","code":"404"}}` + +func (p *keyRecoveryProxy) handler() http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch r.URL.Path { + case "/key/update": + atomic.AddInt32(&p.updateCalls, 1) + w.WriteHeader(p.updateStatus) + io.WriteString(w, p.updateBody) + case "/key/generate": + atomic.AddInt32(&p.generateCalls, 1) + io.WriteString(w, `{"key": "sk-new", "token_id": "new-token"}`) + case "/key/info": + requested := r.URL.Query().Get("key") + if p.staleKeyGone && requested != "new-token" { + w.WriteHeader(http.StatusNotFound) + io.WriteString(w, keyNotFoundBody) + return + } + json.NewEncoder(w).Encode(map[string]interface{}{ + "key": requested, + "info": map[string]interface{}{"team_id": "team-b"}, + }) + default: + http.NotFound(w, r) + } + } +} + +func runKeyUpdate(t *testing.T, p *keyRecoveryProxy, d *schema.ResourceData) diag.Diagnostics { + t.Helper() + srv := httptest.NewServer(p.handler()) + defer srv.Close() + return resourceKeyUpdate(context.Background(), d, NewClient(srv.URL, "test-key", true)) +} + +// Reassigning a key between two teams that both still exist is a plain +// in-place /key/update and must not be turned into a destroy/recreate. +func TestResourceKeyUpdateTeamReassignmentStaysInPlace(t *testing.T) { + proxy := &keyRecoveryProxy{updateStatus: http.StatusOK, updateBody: `{"key": "hash-1"}`} + d := newKeyUpdateResourceData(t, "hash-1", "team-a", "team-b") + + if diags := runKeyUpdate(t, proxy, d); diags.HasError() { + t.Fatalf("update returned error: %v", diags) + } + if got := atomic.LoadInt32(&proxy.generateCalls); got != 0 { + t.Errorf("a benign team reassignment must not recreate the key, got %d /key/generate calls", got) + } + if d.Id() != "hash-1" { + t.Errorf("Id = %q, want hash-1 unchanged", d.Id()) + } +} + +// The reported bug: the key was cascade-deleted along with its old team, so +// /key/update 404s and the apply must recover by recreating it. +func TestResourceKeyUpdateRecreatesCascadeDeletedKey(t *testing.T) { + proxy := &keyRecoveryProxy{updateStatus: http.StatusNotFound, updateBody: keyNotFoundBody, staleKeyGone: true} + d := newKeyUpdateResourceData(t, "stale-token", "team-a", "team-b") + + if diags := runKeyUpdate(t, proxy, d); diags.HasError() { + t.Fatalf("a cascade-deleted key must be recreated, not error: %v", diags) + } + if got := atomic.LoadInt32(&proxy.updateCalls); got != 1 { + t.Errorf("expected 1 /key/update attempt before recovering, got %d", got) + } + if got := atomic.LoadInt32(&proxy.generateCalls); got != 1 { + t.Errorf("expected exactly 1 /key/generate recreate, got %d", got) + } + if d.Id() != "new-token" { + t.Errorf("Id = %q, want the recreated key's new-token", d.Id()) + } +} + +// /key/update 404s for reasons other than a missing key, a rejected +// project_id among them. Recovering on the status code alone would orphan a +// key that is still live on the proxy, so the key's absence must be confirmed. +func TestResourceKeyUpdateNotFoundWithLiveKeyFailsLoudly(t *testing.T) { + proxy := &keyRecoveryProxy{ + updateStatus: http.StatusNotFound, + updateBody: `{"error":{"message":"Project not found, project_id=proj-1"}}`, + } + d := newKeyUpdateResourceData(t, "hash-1", "team-a", "team-b") + + if diags := runKeyUpdate(t, proxy, d); !diags.HasError() { + t.Fatal("a 404 on a key that still exists must stay an error") + } + if got := atomic.LoadInt32(&proxy.generateCalls); got != 0 { + t.Errorf("expected no recreate while the key is still live, got %d /key/generate calls", got) + } + if d.Id() != "hash-1" { + t.Errorf("Id = %q, want hash-1 untouched on a hard failure", d.Id()) + } +} + +// A key gone for some reason unrelated to a team move still fails loudly. +func TestResourceKeyUpdateNotFoundWithoutTeamChangeFailsLoudly(t *testing.T) { + proxy := &keyRecoveryProxy{updateStatus: http.StatusNotFound, updateBody: keyNotFoundBody, staleKeyGone: true} + d := newKeyUpdateResourceData(t, "gone-token", "team-a", "team-a") + + if diags := runKeyUpdate(t, proxy, d); !diags.HasError() { + t.Fatal("expected an error when team_id did not change") + } + if got := atomic.LoadInt32(&proxy.generateCalls); got != 0 { + t.Errorf("expected no recreate when team_id is unchanged, got %d /key/generate calls", got) + } + if d.Id() != "gone-token" { + t.Errorf("Id = %q, want gone-token untouched on a hard failure", d.Id()) + } +} + +// A transient failure must never be mistaken for a cascade-deleted key. +func TestResourceKeyUpdateServerErrorDoesNotRecreate(t *testing.T) { + proxy := &keyRecoveryProxy{ + updateStatus: http.StatusInternalServerError, + updateBody: `{"error":{"message":"Internal Server Error"}}`, + staleKeyGone: true, + } + d := newKeyUpdateResourceData(t, "hash-1", "team-a", "team-b") + + if diags := runKeyUpdate(t, proxy, d); !diags.HasError() { + t.Fatal("expected a 500 to surface as an error") + } + if got := atomic.LoadInt32(&proxy.generateCalls); got != 0 { + t.Errorf("expected no recreate for a transient error, got %d /key/generate calls", got) + } +} + +// The metadata pre-read fails before /key/update is ever reached when the key +// is gone, so that path needs the same recovery. +func TestResourceKeyUpdateRecreatesCascadeDeletedKeyWithMetadataChange(t *testing.T) { + proxy := &keyRecoveryProxy{updateStatus: http.StatusOK, updateBody: `{"key": "hash-1"}`, staleKeyGone: true} + state := &terraform.InstanceState{ID: "stale-token", Attributes: map[string]string{ + "team_id": "team-a", + "metadata.%": "1", + "metadata.tier": "gold", + }} + diff := &terraform.InstanceDiff{Attributes: map[string]*terraform.ResourceAttrDiff{ + "team_id": {Old: "team-a", New: "team-b"}, + "metadata.tier": {Old: "gold", New: "silver"}, + }} + d, err := schema.InternalMap(resourceKey().Schema).Data(state, diff) + if err != nil { + t.Fatalf("building ResourceData returned error: %v", err) + } + + if diags := runKeyUpdate(t, proxy, d); diags.HasError() { + t.Fatalf("a cascade-deleted key must be recreated, not error: %v", diags) + } + if got := atomic.LoadInt32(&proxy.updateCalls); got != 0 { + t.Errorf("expected the metadata pre-read to short-circuit /key/update, got %d calls", got) + } + if got := atomic.LoadInt32(&proxy.generateCalls); got != 1 { + t.Errorf("expected exactly 1 /key/generate recreate, got %d", got) + } + if d.Id() != "new-token" { + t.Errorf("Id = %q, want the recreated key's new-token", d.Id()) + } +}