feat(terraform): vendor terraform-provider-litellm as source of truth with endpoint drift CI - #32241
Conversation
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR vendors the LiteLLM Terraform provider into
Confidence Score: 5/5All correctness and security fixes are well-scoped, backed by regression tests that fail against the previous behavior, and the new CI drift detector adds durable protection against future endpoint mismatches. The two behavioral fixes (team member role propagation and MCP server env no-readback) are straightforward and their regression tests are tight. The broader vendoring changes are mechanical imports with no new proxy-side logic. No issues were found that affect correctness, security boundaries, or state integrity. No files require special attention.
|
| Filename | Overview |
|---|---|
| terraform/provider/litellm/resource_team_member.go | Team member Update now includes role in the payload; fixes silent role-downgrade regression. |
| terraform/provider/litellm/resource_team_member_test.go | New unit test captures the HTTP payload and asserts role is present; fails against the old implementation. |
| terraform/provider/litellm/resource_mcp_server_crud.go | updateSchemaFromResponse omits d.Set("env") so the Sensitive env attribute is never overwritten from server responses. |
| terraform/provider/litellm/resource_mcp_server_crud_test.go | Unit test verifies server-returned env values never land in state; fails against the previous behavior. |
| terraform/provider/litellm/resource_mcp_server.go | env TypeMap marked Sensitive:true; args intentionally read back from server per design. |
| terraform/provider/litellm/resource_vector_store_crud.go | resourceLiteLLMVectorStoreRead omits d.Set("litellm_params") so server-returned params never write to state. |
| terraform/provider/litellm/resource_vector_store_crud_test.go | Test wires httptest.Server and asserts api_key never leaks into state while config values are preserved. |
| terraform/provider/litellm/client.go | Empty-body guard covers POST/PATCH/PUT/DELETE; redactSensitiveData recursively masks nested fields; io/ioutil removed. |
| .github/workflows/test-terraform-provider.yml | New CI workflow with provider-checks and endpoint-drift jobs; drift job also triggers on litellm/proxy/** changes. |
| terraform/provider/tools/endpointaudit/main.go | Go AST auditor statically resolves every (method, path) call site and verifies against proxy OpenAPI; fails closed on unresolvable sites. |
Reviews (6): Last reviewed commit: "fix(terraform): send role on team member..." | Re-trigger Greptile
Greptile SummaryThis PR vendors the LiteLLM Terraform provider into
Confidence Score: 4/5Safe to merge; the core bug fixes and audit infrastructure are correct and well-tested, with end-to-end proof provided. The organisation and team CRUD methods in client.go are never called by their corresponding Terraform resources (both use MakeRequest directly), so ~8 of the 54 verified audit call sites are phantom. The sendRequest empty-body guard is also POST-only while live PATCH/DELETE paths go through the same function. Neither issue affects current correctness, but both are worth addressing before the provider sees heavy usage. terraform/provider/litellm/client.go — dead organisation/team CRUD methods and the narrow empty-body guard in sendRequest.
|
| Filename | Overview |
|---|---|
| .github/workflows/test-terraform-provider.yml | New CI workflow with two jobs: provider-checks (gofmt/vet/build/test) and endpoint-drift (OpenAPI schema validation); action hashes are pinned, permissions scoped to contents:read, concurrency group configured correctly. |
| terraform/provider/litellm/client.go | Central HTTP client; contains ~8 dead-code methods (org/team CRUD) never called by resources, inflating the endpoint audit count; empty-body guard in sendRequest only covers POST, leaving PATCH/DELETE paths unprotected. |
| terraform/provider/litellm/resource_organization.go | Fixes the core bug: update path changed from POST to PATCH /organization/update via MakeRequest; uses package-level endpoint consts that the audit tool can statically resolve. |
| terraform/provider/litellm/resource_organization_member.go | Uses client helper methods (sendRequest under the hood) for PATCH/DELETE operations; read is a no-op stub since no single-member read endpoint exists. |
| terraform/provider/tools/endpointaudit/main.go | Static AST extractor that resolves string literals, package consts, local reassignments, and fmt.Sprintf compositions; fails closed on unresolved or raw http.NewRequest call sites; logic is correct and well-tested. |
| terraform/provider/tools/dump_openapi.py | Minimal script to dump proxy OpenAPI schema; missing argument count check — IndexError on bare invocation with no path argument. |
| terraform/provider/litellm/utils.go | MakeRequest helper and not-found error detectors for model, MCP server, credential, and vector store resources; uses deprecated ioutil but otherwise clean. |
Comments Outside Diff (1)
-
terraform/provider/litellm/client.go, line 484-488 (link)Empty-body guard in
sendRequestonly covers POSTThe guard that handles empty or
"null"response bodies is gated onmethod == "POST". BothUpdateOrganizationMember(PATCH /organization/member_update) andDeleteOrganizationMember(DELETE /organization/member_delete) route throughsendRequest, and both are called by the live resource implementations. If the proxy ever returns an empty body for a PATCH or DELETE,json.Unmarshalwill fail and the error propagates as an unexpected failure rather than a clean success. Expanding the guard to coverPATCHandDELETEwould make the behaviour consistent.
Reviews (2): Last reviewed commit: "feat(terraform): vendor terraform-provid..." | Re-trigger Greptile
… with endpoint drift CI
b465f9b to
a32aebb
Compare
|
@greptileai addressed the empty-body fallback (now covers PATCH/PUT/DELETE alongside POST) and added an argument check to dump_openapi.py. The ioutil and redaction notes are pre-existing code imported verbatim from the split repo; keeping them out of this PR's scope |
|
Thanks for the follow-up. The two addressed items look good:
The decision to scope out the
Happy to mark both |
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 5 · PR risk: 0/10 |
|
@greptileai please review the current head (a32aebb); the previous review was pinned to the pre-amend commit b465f9b. The empty-body guard now covers PATCH/PUT/DELETE (client.go:360) and dump_openapi.py validates its argument count. On the other two threads: the redaction regex hardening is a fair catch and worth doing properly (balanced-brace handling, not a regex tweak), so I am tracking it with the ioutil cleanup as a follow-up in the monorepo rather than growing this import PR |
Replace deprecated io/ioutil with io. Remove the unused org/team CRUD client methods so the endpoint audit only tracks live call sites (54 -> 46). Redact request/response logs by parsing the JSON and recursively masking sensitive fields, which fixes the nested-object leak in the old credential_values regex, with a regex fallback for non-JSON payloads; covered by new unit tests. Docs: stop showing api_key inside vector store litellm_params and document that Sensitive attributes still persist in plaintext state, recommending litellm_credential_name and an encrypted state backend.
|
@greptileai please review the current head df520ad. It addresses the remaining threads: io/ioutil replaced with io, the dead org/team CRUD client methods removed (audit now verifies 46 live call sites), log redaction rewritten to parse the JSON and recursively mask sensitive fields with unit tests covering the nested credential_values case, and docs updated so no example puts api_key inside litellm_params and the secrets-in-state behavior is documented with litellm_credential_name as the recommended path |
…ector store state The vector store Read wrote litellm_params straight back from the API response into state. The proxy redacts secrets in those responses, so the readback overwrote user config with redaction sentinels and caused perpetual diffs, and against a server that returns raw values it would persist secrets into a non-Sensitive attribute. Read now preserves the config value like the credential and model resources do, litellm_params is marked Sensitive, and a regression test pins that a server-returned api_key never lands in state
|
@greptileai please review the current head 8ef3b51 (supersedes df520ad, which addressed the earlier threads). The new commit stops the vector store Read from persisting server-returned litellm_params into state, marks litellm_params Sensitive, and adds a regression test |
…erver env into MCP state The team member update payload omitted role, and the proxy leaves role unchanged when the field is absent, so a role downgrade reported as applied by Terraform never took effect on the proxy. The update now always sends the configured role (the attribute is Required). The MCP server resource wrote env straight back from API responses into a non-Sensitive attribute, pulling admin-visible secrets into state and, for sanitized responses, blanking user config. Read now preserves the config value, env is marked Sensitive, and the docs warn against passing secrets via args. Regression tests cover both fixes and fail against the previous behavior.
|
@greptileai please review the current head 5504387. Since your last review: 8ef3b51 stops the vector store Read from persisting server-returned litellm_params into state and marks it Sensitive; 5504387 fixes the team member update dropping the role field (a role downgrade never reached the proxy) and applies the same no-readback plus Sensitive treatment to the MCP server env attribute. Both commits add regression tests that fail against the previous behavior |
Relevant issues
Linear ticket
Resolves LIT-4212
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaiand received a Confidence Score of at least 4/5 before requesting a maintainer reviewScreenshots / Proof of Fix
The endpoint audit runs the exact pipeline CI runs, and caught a real bug in the imported source on its first run (the provider sent POST to an endpoint the proxy serves as PATCH only):
After the fix in this PR:
After the second commit removes the dead org/team client methods flagged in review, the audit tracks only live call sites:
End user proof against a live proxy (postgres on 5461, proxy on 4061 started with
python litellm/proxy/proxy_cli.py --config litellm/proxy/dev_config.yaml --port 4061), driving the real provider binary through terraform with adev_overridesCLI config:Updating the alias with the provider built from the current split repo source (before this PR):
Same update with the provider built from this PR:
Type
🆕 New Feature
🐛 Bug Fix
Changes
This PR makes
terraform/provider/the source of truth for the LiteLLM Terraform provider, imported from BerriAI/terraform-provider-litellm@f1aac99. The split repo becomes a thin release mirror, following the model already used for the aws/gcp Terraform modules underterraform/litellm/. The motivation is deterministic CI that fails when the provider and the proxy endpoints drift apart, which is impossible while they live in separate reposThe new
Terraform Providerworkflow has two jobs.provider-checksruns gofmt, go vet, go build, and the provider's unit tests onterraform/provider/**changes.endpoint-driftgenerates the proxy's OpenAPI schema (terraform/provider/tools/dump_openapi.py) and runsterraform/provider/tools/endpointaudit, a Go AST tool that statically resolves every (method, path) the provider sends through its two request helpers (string literals, package consts, local reassignments, and fmt.Sprintf compositions) and verifies each against the schema. It fails closed: call sites it cannot resolve statically and raw http.NewRequest calls outside the helpers are errors. The drift job also triggers onlitellm/proxy/**changes, so a litellm PR that removes or re-methods a management endpoint the provider depends on fails CI on this repo instead of breaking terraform users at the next releaseThe audit caught real drift on its first run: the proxy serves
/organization/updateand/organization/member_updateas PATCH only, but the provider sent POST, solitellm_organizationandlitellm_organization_memberupdates failed with a 405. Both call sites are fixed here (resource_organization.go,client.go) and the extractor's tests pin every resolution shape so the audit itself stays honestReview follow-ups folded into the first commit: the client's empty-body response fallback now covers PATCH, PUT, and DELETE alongside POST (relevant since organization updates now send PATCH), and dump_openapi.py validates its argument count instead of raising IndexError
The second commit addresses the remaining review threads. io/ioutil is replaced with io. The unused org/team CRUD client methods (and the validateUUID helper only they used) are removed so the audit tracks only live call sites, dropping the verified count from 54 to 46. Log redaction now parses the JSON payload and recursively masks sensitive fields instead of regexing the raw string, which fixes the nested credential_values leak flagged in review (the old regex also mangled vertex_credentials values containing escaped JSON and leaked part of them); a regex pass remains as fallback for non-JSON payloads, and new unit tests cover both paths and fail against the old implementation. On the secrets-in-state threads: the docs now state explicitly that Sensitive attributes persist in plaintext state, the vector store examples no longer show api_key inside litellm_params, and litellm_credential_name is the documented path for secret material. Converting the existing attributes to write-only is not possible as shaped (the SDK rejects WriteOnly on TypeMap, and the conversion would require Terraform 1.11+ and break rotation-by-diff), so the scalar _wo redesign is tracked as a provider follow-up instead
The third commit fixes the one real readback the security threads surfaced: the vector store Read wrote litellm_params straight back from the API response into a non-Sensitive attribute. The proxy redacts secrets in those responses, so in practice the readback replaced user config with redaction sentinels and caused perpetual diffs, and against a server returning raw values it would have persisted secrets into state unmarked. Read now preserves the config value like the credential and model resources do, litellm_params is marked Sensitive, and a regression test pins that a server-returned api_key never lands in state (it fails against the previous behavior)
The fourth commit resolves the security bot's follow-up round. The team member update payload omitted role, and the proxy leaves role unchanged when the field is absent, so a Terraform role downgrade (admin to user) reported as applied never took effect on the proxy; the update now always sends the configured role. The MCP server resource had the same readback problem as the vector store: env came straight back from API responses into a non-Sensitive attribute (raw for admin viewers, blanked for sanitized ones), so Read now preserves the config value, env is marked Sensitive, and the docs warn against passing secrets via args since argv is also visible in the server's process list. Regression tests cover both fixes and fail against the previous behavior
Import notes: the go.mod module path is renamed from the upstream fork author's namespace to
github.com/BerriAI/terraform-provider-litellm, the stale 1MBopenapi.jsoncommitted at the provider root is dropped (nothing referenced it), and README/RELEASING/CHANGELOG are updated for the new home and release flow. Publishing keeps working exactly as today (goreleaser on tag push in the mirror repo); the follow-up project-releaser PR adds the workflow that rsyncs this directory into the mirror and tags it