Repository navigation
fix(terraform): preserve undeclared server-side key metadata on update - #40514
Conversation
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
|
|
@greptileai please review |
Greptile SummaryThis PR updates the Terraform provider to preserve undeclared server-side key metadata during updates.
Confidence Score: 5/5The PR appears safe to merge, with no outstanding findings or newly introduced issues since the previous review. The previously identified GET-to-update race was explicitly documented as a provider-side limitation and its thread was resolved; the current changes correctly prevent unrelated Terraform updates from replacing undeclared server metadata, and no additional changes have been made since that review.
|
| Filename | Overview |
|---|---|
| terraform/provider/litellm/client.go | Conditionally includes metadata and model-limit fields in key update requests so omitted fields retain their server-side values. |
| terraform/provider/litellm/resource_key.go | Tracks changed fields, merges declared metadata changes with live metadata, and limits state to Terraform-declared metadata keys. |
| terraform/provider/litellm/resource_key_test.go | Adds focused integration-style provider tests covering metadata preservation, merging, state filtering, and changed model limits. |
Reviews (2): Last reviewed commit: "fix(terraform): preserve undeclared serv..." | Re-trigger Greptile
|
@greptileai please re-review c119e27; the race window is documented under Caveats and cannot be closed provider-side. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
b64197c
into
litellm_internal_staging
…0527) The squash of #40512 onto a base that already carried #40514 left the Partial call on the metadata pre-read error path only, so a rejected /key/update again persisted the planned values into state and TestResourceKeyUpdateFailureKeepsPriorState fails on the default branch. Co-authored-by: yassin <yassin@berri.ai> Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
TLDR
Problem this solves:
litellm_keyupdate wiped metadata Terraform did not declareUpdateKeyalways sentmetadata,model_rpm_limit,model_tpm_limit; proxy replaces wholesaleHow it solves it:
metadataandmodel_*_limitfrom/key/updatewhen the plan leaves them unchangedUser Flow
Before: an operator who added metadata to a Terraform-managed key outside Terraform loses it the next time Terraform touches the key
terraform applyalitellm_keywithmetadata = { a = "1" }andmax_budget = 10{"key": "<token>", "metadata": {"a": "1", "server_side": "x"}}and get 200 with both entries backmax_budget = 20in config and runterraform plan; the plan showsmax_budget = 10 -> 20and also- "server_side" = "x" -> nullterraform apply; GET http://localhost:4000/key/info?key= returns"metadata": {"a": "1", "model_rpm_limit": {}, "model_tpm_limit": {}},server_sideis goneAfter: the same unrelated change leaves the server-side metadata alone
terraform applyalitellm_keywithmetadata = { a = "1" }andmax_budget = 10{"key": "<token>", "metadata": {"a": "1", "server_side": "x"}}and get 200 with both entries backmax_budget = 20in config and runterraform plan; the plan shows onlymax_budget = 10 -> 20terraform apply; GET http://localhost:4000/key/info?key= returns"metadata": {"a": "1", "server_side": "x"}and a follow-upterraform planreports no changesmetadatain config (a = "2", addb = "new"), the apply sets those keys andserver_sideis still thereRelevant issues
Fixes BerriAI/terraform-provider-litellm#60
Linear ticket
Resolves LIT-6402
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
uv run pytest tests/test_litellm/<your_test_file>.py -v. Leave the suites (make test-unit-*,make test-unit) to CI: it finishes in ~15 minutes where a laptop takes an hour or more@greptileaito re-request a review after pushing changes)Delays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
Screenshots / Proof of Fix
Root cause in one sentence:
resourceKeyUpdatealways sent"metadata": <declared map>(plus emptymodel_rpm_limit/model_tpm_limit) on/key/update, and the proxy'sprepare_metadata_fieldsonly keeps stored metadata when themetadatafield is absent from the request, so every update replaced the stored JSON with the Terraform-declared subset.Shared setup: proxy from this checkout (
PYTHONPATH=<checkout> uv run --no-sync litellm --config proxy_config.yaml --port 16402 --use_prisma_db_push, Postgres 16 in Docker), Terraform v1.13.3 with adev_overridesblock pointingBerriAI/litellmat the provider binary under test. Same proxy for both arms; only the provider binary differs. Build identity:strings <binary> | grep -c litellm.mergeKeyMetadataprints0for the base binary and1for the fixed one.main.tf.v1andmain.tf.v2are sha256-pinned and identical across arms; the only difference between them ismax_budget = 10vsmax_budget = 20.Both arms hit the same proxy branch on the unrelated update:
prepare_metadata_fieldswithmetadatapresent in the request (Before, wholesale replace) vsmetadataabsent (After,existing_metadata.copy()kept). The explicit metadata change case in After also hits themetadatapresent branch, on purpose, with the merged map.Before (b22ca7a)
cp main.tf.v1 main.tf && terraform apply -auto-approvecurl -s "http://localhost:16402/key/info?key=$TOKEN" -H "Authorization: Bearer $MASTER"curl -s -X POST http://localhost:16402/key/update -H "Authorization: Bearer $MASTER" -H "Content-Type: application/json" -d '{"key": "'$TOKEN'", "metadata": {"a": "1", "server_side": "x"}}'cp main.tf.v2 main.tf && terraform planterraform apply -auto-approvecurl -s "http://localhost:16402/key/info?key=$TOKEN" -H "Authorization: Bearer $MASTER"server_sideis gone and two empty proxy-owned maps were written by the providerAfter (c119e27)
cp main.tf.v1 main.tf && terraform apply -auto-approvecurl -s "http://localhost:16402/key/info?key=$TOKEN" -H "Authorization: Bearer $MASTER"curl -s -X POST http://localhost:16402/key/update -H "Authorization: Bearer $MASTER" -H "Content-Type: application/json" -d '{"key": "'$TOKEN'", "metadata": {"a": "1", "server_side": "x"}}'cp main.tf.v2 main.tf && terraform planterraform apply -auto-approvecurl -s "http://localhost:16402/key/info?key=$TOKEN" -H "Authorization: Bearer $MASTER"terraform planmain.tfwitha = "2"andb = "new", thenterraform plan && terraform apply -auto-approvecurl -s "http://localhost:16402/key/info?key=$TOKEN" -H "Authorization: Bearer $MASTER"main.tf.v2(dropsb),terraform apply -auto-approve, then the same GETProvider gates run locally from
terraform/provider:gofmt -l .(empty),go vet ./...,go build ./...,go test -timeout 120s ./...(ok), and the endpoint audit against a freshly dumped OpenAPI schema (OK: 136 request call sites verified against 620 proxy OpenAPI paths).Mutation check: the four new tests in
resource_key_test.godrive the realResource.Diff+Resource.Applyagainst an httptest proxy that mimics the wholesale replace. On the baseclient.go/resource_key.gothree of them fail (metadataand bothmodel_*_limitsent while unchanged,server_sidelost, Read pullsserver_sideinto state); with the fix all pass. Six targeted mutants (drop theHasChangegate, always or never send model limits, skip the Read filter, delta-only merge, merge that forgets removed keys) are each killed by at least one test.Type
🐛 Bug Fix
Caveats (if any)
Medium
metadatain config is deleted on the server; keys never declared are left aloneterraform plancannot show it/key/infoper update to read the live server map/key/updatehas no atomic merge or version check, so the provider cannot close this windowLow
Link to Devin session: https://app.devin.ai/sessions/96b831b532184cee8fd2228f2235e6f2
Open in Devin Desktop: https://app.devin.ai/desktop/session/96b831b532184cee8fd2228f2235e6f2?variant=devin
Requested by: @yassin-berriai