Repository navigation
fix(terraform): keep prior state when a virtual key update is rejected - #40512
Conversation
🤖 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:
|
|
|
Greptile SummaryThis PR preserves the prior Terraform state when a
Confidence Score: 5/5The PR appears safe to merge; no actionable regressions or outstanding findings remain. The update failure path now preserves prior state, and the regression test verifies the rejected planned value is omitted while existing attributes remain. The previous source-comment finding was manually resolved without explanation, and the comments have been removed from the current head.
|
| Filename | Overview |
|---|---|
| terraform/provider/litellm/resource_key.go | Marks failed key updates as partial so Terraform retains the prior state. |
| terraform/provider/litellm/resource_key_test.go | Verifies through the Terraform SDK Apply path that rejected values are not persisted. |
Reviews (2): Last reviewed commit: "fix(terraform): keep prior state when a ..." | Re-trigger Greptile
resourceKeyUpdate now calls d.Partial(true) on a failed /key/update so the SDK does not persist the rejected planned values into state, which made the next plan report no changes and silently dropped the update. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
f76bcdc to
08152ee
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
@greptileai removed both explanatory comments per the repo comment policy; re-review the squashed head 08152ee please. |
fb603fd
into
litellm_internal_staging
BerriAI#40512 added d.Partial(true) to resourceKeyUpdate, but it landed on the plannedKeyMetadata error path rather than the c.UpdateKey one. The test it shipped with drives a 400 from /key/update, which returns through the UpdateKey branch, so TestResourceKeyUpdateFailureKeepsPriorState has been failing on litellm_internal_staging since it merged Set the flag on the UpdateKey branch too. Without it the SDK hands back the diff-applied state, so a rejected update persists the planned values and the next plan reports no changes, silently dropping the update
…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:
/key/updatestill writes the planned values into Terraform stateterraform plansays "No changes", so the update is silently lostHow it solves it:
resourceKeyUpdatecallsd.Partial(true)when the update failsApplyand fails without the fixUser Flow
Before: an operator changes a virtual key in Terraform, the proxy rejects it, and Terraform then behaves as if the change had been applied
budget_duration = "bad"on an existinglitellm_keyand runterraform applyInvalid budget_duration 'bad'andterraform applyexits 1 with that errorterraform state show litellm_key.demonevertheless showsbudget_duration = "bad"terraform planagain and get "No changes. Your infrastructure matches the configuration.""budget_duration": null, so state and the proxy disagree and the change is goneAfter: the same rejected update leaves state untouched, so Terraform keeps trying until the config is valid
budget_duration = "bad"on an existinglitellm_keyand runterraform applyInvalid budget_duration 'bad'andterraform applyexits 1 with that errorterraform state show litellm_key.demoshows nobudget_duration(the prior state)terraform planagain and it still proposes the in-place update (Plan: 0 to add, 1 to change, 0 to destroy)"budget_duration": null, matching stateRelevant issues
Upstream report: BerriAI/terraform-provider-litellm#55
Linear ticket
Resolves LIT-6398
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
Setup shared by both runs: Postgres in Docker plus the proxy from this checkout (
PYTHONPATH=<checkout> uv run --no-sync litellm --config <cfg> --port 16398 --use_prisma_db_push, onefake-modeldeployment). The provider binary is built with(cd terraform/provider && go build -o <out> .)from the merge base and from the PR tip, and Terraform 1.9.8 loads it throughdev_overrides.main.tfand the startingterraform.tfstateare byte-identical across both runs (sha256ba853a6f...andf5df4c7f...), and the proxy is the same process for both. The build serving each run is identified bygo version -m <binary>, which prints the git revision it was compiled fromBefore (b22ca7a)
Confirm the build
Starting point on the proxy
Apply an update the proxy rejects
State recorded the rejected value anyway
The next plan thinks everything is applied
The proxy never received the change
After (08152ee)
Confirm the build
Starting point on the proxy
Apply the same rejected update
State kept the prior values
The next plan still wants to apply the change
Proxy and state agree
Control: a valid update on the fixed build still goes through
Type
🐛 Bug Fix
Caveats (if any)
Medium
litellm_keyonly, as the ticket asksd.Partial(true), so they share the same SDK behaviorlitellm_teamwith a throwaway test: a 400 from/team/updatestill lands the plannedteam_aliasin stateLow
terraform applydoes already exit 1 with the proxy error. What is wrong is the state written after that error, which is what this PR fixesosv-scancheck is red for asmol-tomldev dependency inui/litellm-dashboard/package-lock.json. That lockfile is untouched here and the same check fails on every PR opened against the base branch in the same window, so it is base drift, not this changeFinal Attestation
Link to Devin session: https://app.devin.ai/sessions/7c22271f9676444688e115bbf10168ab
Open in Devin Desktop: https://app.devin.ai/desktop/session/7c22271f9676444688e115bbf10168ab?variant=devin
Requested by: @yassin-berriai