mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-21 00:21:49 +00:00
test(e2e): stop the config suite locking itself out of the shared proxy
Two tests in the config/misc management suite were failing every run against the Buildkite e2e stack, and one of them took the rest of the build with it. test_add_allowed_ip_does_not_store_unrelated_config_value posted 127.0.0.1 to /add/allowed_ip. That route sets the live general_settings["allowed_ips"] that auth_utils._check_valid_ip reads before it persists anything, and the check is exact string membership with no CIDR support, so from the moment the POST returns only 127.0.0.1 can reach the proxy. The runner 403s on its very next call, and the deferred /delete/allowed_ip sits behind the same auth dependency, so the cleanup is locked out too and every later test in the build 403s. Build 254's first attempt lost 459 of its 465 failures to that one cascade. There is no safe way to exercise the route against a shared proxy: nothing reports the caller's address as the proxy sees it, so a test cannot allowlist itself first. Move the claim to the route's own TestClient suite, where the auth dependency is overridden and general_settings is per-test, and record the route in the module docstring beside /cache/settings and the Vault override so it is not re-added. save_config's end of the contract was already covered by test_ProxyConfig_save_config_merges_changed_keys_without_copying_file_settings; the new test covers the route's end, that what it hands save_config differs from the loaded config in allowed_ips and nothing else. The unrelated-key probe also only ever worked on one lane: max_parallel_requests was added to tests/e2e/gateway/stage_mirror_ci_config.yml and never to the Buildkite stack's config, where resolve() reports it as "unset" rather than "config". That key is now unused, so drop it again. test_config_update_persists_router_setting_to_get wrote router_settings. num_retries, which both lanes declare in their config file, so the config- ownership work correctly refuses it with a 400. Switch to retry_after, which is declared by neither lane, is accepted by /config/update, and is reported back by GET /router/settings. Verified against a live proxy: max_fallbacks also takes the write but never reads back, so the read-back poll is what picks the key.
This commit is contained in:
Yuneng Jiang
parent
b946d12ffd
commit
bf9c717d77
4 changed files with 102 additions and 112 deletions
|
|
@ -72,7 +72,6 @@
|
|||
- {id: mgmt.callback.list.happy_path, module: mgmt, tier: P2, surface: api, assertions: [happy_path], source: "callback_management_endpoints.py", rationale: "Callback config (smoke)"}
|
||||
- {id: mgmt.cost_tracking.estimate.happy_path, module: mgmt, tier: P2, surface: api, assertions: [happy_path], source: "cost_tracking_settings.py", rationale: "Cost estimate (smoke)"}
|
||||
- {id: mgmt.router_settings.update.happy_path, module: mgmt, tier: P2, surface: api, assertions: [happy_path], source: "router_settings_endpoints.py", rationale: "Router config (smoke)"}
|
||||
- {id: mgmt.config.allowed_ip.changed_key_only, module: mgmt, tier: P2, surface: api, assertions: [persists], source: "proxy_setting_endpoints.py:496", rationale: "An allowed-IP change leaves unrelated file settings out of the DB row"}
|
||||
- {id: mgmt.jwt_key_mapping.new.happy_path, module: mgmt, tier: P2, surface: api, assertions: [happy_path], source: "jwt_key_mapping_endpoints.py", rationale: "JWT->key mapping (smoke)"}
|
||||
- {id: mgmt.compliance.gdpr.happy_path, module: mgmt, tier: P2, surface: api, assertions: [happy_path], source: "compliance_endpoints.py", rationale: "GDPR ops (smoke)"}
|
||||
- {id: mgmt.tool_management.list.happy_path, module: mgmt, tier: P2, surface: api, assertions: [happy_path], source: "tool_management_endpoints.py", rationale: "Tool inventory (smoke)"}
|
||||
|
|
|
|||
|
|
@ -1,5 +1,4 @@
|
|||
general_settings:
|
||||
max_parallel_requests: 100
|
||||
proxy_batch_write_at: 5
|
||||
enable_jwt_auth: true
|
||||
litellm_jwtauth:
|
||||
|
|
|
|||
|
|
@ -7,13 +7,18 @@ so a read-back reflects the change. Router settings, which mutate global proxy
|
|||
state, are exercised with a benign, self-restoring change so a shared proxy is left
|
||||
as it was found.
|
||||
|
||||
Cache settings and the Vault config override are deliberately not covered here.
|
||||
Both routes reconfigure the whole proxy: /cache/settings persists what it receives
|
||||
into a row that outranks the YAML cache_params and is re-applied on a timer, and
|
||||
/config_overrides/hashicorp_vault swaps the process-wide secret manager. Neither can
|
||||
be exercised safely against the shared proxy the suites run on, so they need an
|
||||
isolated proxy before a test lands. Do not add a read-then-write-back test for
|
||||
either one.
|
||||
Cache settings, the Vault config override and the allowed-IP routes are deliberately
|
||||
not covered here. All three reconfigure the whole proxy: /cache/settings persists what
|
||||
it receives into a row that outranks the YAML cache_params and is re-applied on a timer,
|
||||
/config_overrides/hashicorp_vault swaps the process-wide secret manager, and
|
||||
/add/allowed_ip mutates the live general_settings["allowed_ips"] that
|
||||
auth_utils._check_valid_ip reads, so the first call locks every other client out of the
|
||||
shared proxy. The allowlist is an exact string match with no CIDR support, and no route
|
||||
reports the caller's address as the proxy sees it, so a test cannot allowlist itself
|
||||
first; /delete/allowed_ip sits behind the same auth dependency, so the cleanup is locked
|
||||
out too and the proxy stays poisoned for the rest of the build. None of the three can be
|
||||
exercised safely against the shared proxy the suites run on, so they need an isolated
|
||||
proxy before a test lands. Do not add a read-then-write-back test for any of them.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
|
@ -21,10 +26,9 @@ from __future__ import annotations
|
|||
import math
|
||||
import time
|
||||
from collections.abc import Callable
|
||||
from typing import Final
|
||||
|
||||
import pytest
|
||||
from pydantic import BaseModel, JsonValue, RootModel
|
||||
from pydantic import BaseModel
|
||||
|
||||
from e2e_config import unique_marker
|
||||
from e2e_http import NoBody, Success, unwrap, unwrap_status
|
||||
|
|
@ -188,7 +192,7 @@ class JwtKeyMappingResponse(BaseModel):
|
|||
|
||||
|
||||
class RouterSettingsPatch(BaseModel):
|
||||
num_retries: int
|
||||
retry_after: int
|
||||
|
||||
|
||||
class ConfigUpdateBody(BaseModel):
|
||||
|
|
@ -199,39 +203,8 @@ class ConfigUpdateResponse(BaseModel):
|
|||
message: str
|
||||
|
||||
|
||||
class AllowedIpBody(BaseModel):
|
||||
ip: str
|
||||
|
||||
|
||||
class ConfigFieldInfoParams(BaseModel):
|
||||
field_name: str
|
||||
|
||||
|
||||
class ConfigFieldInfoResponse(BaseModel):
|
||||
field_name: str
|
||||
field_value: JsonValue
|
||||
source: str
|
||||
editable: bool
|
||||
|
||||
|
||||
class ConfigListParams(BaseModel):
|
||||
config_type: str
|
||||
|
||||
|
||||
class ConfigListEntry(BaseModel):
|
||||
field_name: str
|
||||
field_value: JsonValue
|
||||
stored_in_db: bool | None
|
||||
source: str
|
||||
editable: bool
|
||||
|
||||
|
||||
class ConfigListResponse(RootModel[list[ConfigListEntry]]):
|
||||
pass
|
||||
|
||||
|
||||
class RouterCurrentValues(BaseModel):
|
||||
num_retries: int | None = None
|
||||
retry_after: int | None = None
|
||||
|
||||
|
||||
class RouterSettingsResponse(BaseModel):
|
||||
|
|
@ -493,17 +466,25 @@ class TestRouterSettings:
|
|||
) -> None:
|
||||
"""/config/update is the only write path for router_settings (there is no
|
||||
dedicated router-settings write route). The change is restored on teardown so
|
||||
the shared proxy keeps its original retry policy."""
|
||||
original = self._read_num_retries(client)
|
||||
assert original is not None, "GET /router/settings did not report num_retries; cannot prove a change"
|
||||
resources.defer(lambda: self._write_num_retries(client, original))
|
||||
the shared proxy keeps its original retry policy.
|
||||
|
||||
target = original + 5
|
||||
retry_after is the subject because it satisfies all three constraints at once:
|
||||
no lane's config file declares it, so the database owns it and the write is not
|
||||
refused as config-owned; it is in RUNTIME_UPDATABLE_ROUTER_SETTINGS, so
|
||||
/config/update accepts it; and it is in ROUTER_SETTINGS_FIELDS backed by an
|
||||
always-set Router attribute, so GET /router/settings reports it for the
|
||||
read-back. Bumping it by one second is the smallest change that proves the
|
||||
round-trip without slowing a concurrent test that hits a retry."""
|
||||
original = self._read_retry_after(client)
|
||||
assert original is not None, "GET /router/settings did not report retry_after; cannot prove a change"
|
||||
resources.defer(lambda: self._write_retry_after(client, original))
|
||||
|
||||
target = original + 1
|
||||
response = unwrap(
|
||||
client.proxy.transport.post(
|
||||
"/config/update",
|
||||
headers=client.proxy.transport.master,
|
||||
json=ConfigUpdateBody(router_settings=RouterSettingsPatch(num_retries=target)),
|
||||
json=ConfigUpdateBody(router_settings=RouterSettingsPatch(retry_after=target)),
|
||||
response_type=ConfigUpdateResponse,
|
||||
)
|
||||
)
|
||||
|
|
@ -513,20 +494,20 @@ class TestRouterSettings:
|
|||
|
||||
_ = _poll(
|
||||
client,
|
||||
lambda: True if self._read_num_retries(client) == target else None,
|
||||
f"GET /router/settings never reported num_retries {target} after /config/update",
|
||||
lambda: True if self._read_retry_after(client) == target else None,
|
||||
f"GET /router/settings never reported retry_after {target} after /config/update",
|
||||
)
|
||||
|
||||
self._write_num_retries(client, original)
|
||||
self._write_retry_after(client, original)
|
||||
restored = _poll(
|
||||
client,
|
||||
lambda: original if self._read_num_retries(client) == original else None,
|
||||
f"GET /router/settings never returned to the original num_retries {original} after the restore",
|
||||
lambda: original if self._read_retry_after(client) == original else None,
|
||||
f"GET /router/settings never returned to the original retry_after {original} after the restore",
|
||||
)
|
||||
assert restored == original, f"router num_retries left at {restored}, expected the original {original}"
|
||||
assert restored == original, f"router retry_after left at {restored}, expected the original {original}"
|
||||
|
||||
@staticmethod
|
||||
def _read_num_retries(client: ManagementClient) -> int | None:
|
||||
def _read_retry_after(client: ManagementClient) -> int | None:
|
||||
return unwrap(
|
||||
client.proxy.transport.get(
|
||||
"/router/settings",
|
||||
|
|
@ -534,72 +515,20 @@ class TestRouterSettings:
|
|||
params=NoBody(),
|
||||
response_type=RouterSettingsResponse,
|
||||
)
|
||||
).current_values.num_retries
|
||||
).current_values.retry_after
|
||||
|
||||
@staticmethod
|
||||
def _write_num_retries(client: ManagementClient, value: int) -> None:
|
||||
def _write_retry_after(client: ManagementClient, value: int) -> None:
|
||||
_ = unwrap(
|
||||
client.proxy.transport.post(
|
||||
"/config/update",
|
||||
headers=client.proxy.transport.master,
|
||||
json=ConfigUpdateBody(router_settings=RouterSettingsPatch(num_retries=value)),
|
||||
json=ConfigUpdateBody(router_settings=RouterSettingsPatch(retry_after=value)),
|
||||
response_type=ConfigUpdateResponse,
|
||||
)
|
||||
)
|
||||
|
||||
|
||||
class TestConfigPersistence:
|
||||
@pytest.mark.covers("mgmt.config.allowed_ip.changed_key_only")
|
||||
def test_add_allowed_ip_does_not_store_unrelated_config_value(
|
||||
self, client: ManagementClient, resources: ResourceManager
|
||||
) -> None:
|
||||
allowed_ip: Final = "127.0.0.1"
|
||||
added: Final = unwrap(
|
||||
client.proxy.transport.post(
|
||||
"/add/allowed_ip",
|
||||
headers=client.proxy.transport.master,
|
||||
json=AllowedIpBody(ip=allowed_ip),
|
||||
response_type=ConfigUpdateResponse,
|
||||
)
|
||||
)
|
||||
resources.defer(
|
||||
lambda: unwrap(
|
||||
client.proxy.transport.post(
|
||||
"/delete/allowed_ip",
|
||||
headers=client.proxy.transport.master,
|
||||
json=AllowedIpBody(ip=allowed_ip),
|
||||
response_type=ConfigUpdateResponse,
|
||||
)
|
||||
)
|
||||
)
|
||||
assert added.message == f"IP {allowed_ip} address added successfully"
|
||||
|
||||
listed: Final = unwrap(
|
||||
client.proxy.transport.get(
|
||||
"/config/list",
|
||||
headers=client.proxy.transport.master,
|
||||
params=ConfigListParams(config_type="general_settings"),
|
||||
response_type=ConfigListResponse,
|
||||
)
|
||||
)
|
||||
unrelated: Final = next(entry for entry in listed.root if entry.field_name == "max_parallel_requests")
|
||||
assert unrelated.stored_in_db is not True
|
||||
assert unrelated.source == "config"
|
||||
assert unrelated.editable is False
|
||||
|
||||
field_info: Final = unwrap(
|
||||
client.proxy.transport.get(
|
||||
"/config/field/info",
|
||||
headers=client.proxy.transport.master,
|
||||
params=ConfigFieldInfoParams(field_name="max_parallel_requests"),
|
||||
response_type=ConfigFieldInfoResponse,
|
||||
)
|
||||
)
|
||||
assert field_info.source == "config"
|
||||
assert field_info.editable is False
|
||||
assert field_info.field_value == unrelated.field_value
|
||||
|
||||
|
||||
class TestMcpServerSubmission:
|
||||
@pytest.mark.covers("mgmt.mcp_server.register.happy_path")
|
||||
def test_register_submits_pending_server(self, client: ManagementClient, resources: ResourceManager) -> None:
|
||||
|
|
|
|||
|
|
@ -2604,6 +2604,69 @@ def test_add_allowed_ip_writes_audit_log(mock_proxy_config, monkeypatch):
|
|||
app.dependency_overrides.pop(user_api_key_auth, None)
|
||||
|
||||
|
||||
def test_add_allowed_ip_hands_save_config_only_the_changed_general_setting(monkeypatch):
|
||||
"""An allowed-IP write must not drag the config file's own general_settings into
|
||||
the database row. This covers the route end of that contract: what /add/allowed_ip
|
||||
hands save_config differs from the loaded config in allowed_ips and nothing else.
|
||||
save_config's end -- that the row it writes holds only those changed keys -- is
|
||||
covered by test_ProxyConfig_save_config_merges_changed_keys_without_copying_file_settings.
|
||||
|
||||
This lives here rather than in the e2e suite because /add/allowed_ip mutates the
|
||||
live general_settings["allowed_ips"] that auth_utils._check_valid_ip reads, so on a
|
||||
shared proxy the first call locks every later request out, cleanup included.
|
||||
"""
|
||||
from copy import deepcopy
|
||||
from unittest.mock import AsyncMock, MagicMock
|
||||
|
||||
import litellm.proxy.proxy_server as proxy_server_module
|
||||
from litellm.proxy._types import UserAPIKeyAuth
|
||||
from litellm.proxy.auth.user_api_key_auth import user_api_key_auth
|
||||
from litellm.proxy.config_resolvers.changed_section_keys import changed_section_keys
|
||||
from litellm.proxy.config_resolvers.settings_store import SettingsStore
|
||||
|
||||
file_settings = {"max_parallel_requests": 100, "proxy_config_reload_interval_seconds": 7}
|
||||
store = SettingsStore("general_settings")
|
||||
store.load_yaml(file_settings)
|
||||
saved = []
|
||||
|
||||
fake_prisma = MagicMock()
|
||||
fake_prisma.db.litellm_auditlog.create = AsyncMock()
|
||||
|
||||
async def _get_config():
|
||||
return {"general_settings": deepcopy(file_settings)}
|
||||
|
||||
async def _save_config(new_config=None):
|
||||
saved.append(new_config)
|
||||
return new_config
|
||||
|
||||
monkeypatch.setattr(proxy_server_module, "prisma_client", fake_prisma)
|
||||
monkeypatch.setattr(proxy_server_module, "store_model_in_db", True)
|
||||
monkeypatch.setattr(proxy_server_module, "premium_user", True)
|
||||
monkeypatch.setattr(proxy_server_module, "general_settings", store)
|
||||
monkeypatch.setattr(proxy_server_module.proxy_config, "get_config", _get_config)
|
||||
monkeypatch.setattr(proxy_server_module.proxy_config, "save_config", _save_config)
|
||||
|
||||
async def _admin_auth():
|
||||
return UserAPIKeyAuth(
|
||||
user_id="config-admin",
|
||||
api_key="hashed-admin-key",
|
||||
user_role=LitellmUserRoles.PROXY_ADMIN,
|
||||
)
|
||||
|
||||
app.dependency_overrides[user_api_key_auth] = _admin_auth
|
||||
try:
|
||||
resp = client.post("/add/allowed_ip", json={"ip": "203.0.113.77"})
|
||||
assert resp.status_code == 200, resp.text
|
||||
|
||||
assert len(saved) == 1, f"expected exactly one save_config call, got {len(saved)}"
|
||||
changed, removed = changed_section_keys(file_settings, saved[0]["general_settings"])
|
||||
assert dict(changed) == {"allowed_ips": ["203.0.113.77"]}
|
||||
assert removed == frozenset()
|
||||
assert store["allowed_ips"] == ["203.0.113.77"]
|
||||
finally:
|
||||
app.dependency_overrides.pop(user_api_key_auth, None)
|
||||
|
||||
|
||||
def test_delete_allowed_ip_writes_deleted_audit_log(monkeypatch):
|
||||
"""Removing an allowed IP must be audited as a deletion, symmetric with the
|
||||
add path."""
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue