mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-21 00:21:49 +00:00
Merge pull request #42008 from BerriAI/litellm_/litellm-e2e-buildkite-rc-694bbe
test(e2e): stop the config suite locking itself out of the shared proxy
This commit is contained in:
commit
0b145ea149
4 changed files with 100 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,67 @@ 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 types import MappingProxyType
|
||||
from typing import Final
|
||||
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: Final = MappingProxyType({"max_parallel_requests": 100, "proxy_config_reload_interval_seconds": 7})
|
||||
store: Final = SettingsStore("general_settings")
|
||||
store.load_yaml(file_settings)
|
||||
|
||||
fake_prisma: Final = MagicMock()
|
||||
fake_prisma.db.litellm_auditlog.create = AsyncMock()
|
||||
save_config: Final = AsyncMock(side_effect=lambda new_config: new_config)
|
||||
|
||||
async def _get_config():
|
||||
return {"general_settings": dict(file_settings)}
|
||||
|
||||
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: Final = client.post("/add/allowed_ip", json={"ip": "203.0.113.77"})
|
||||
assert resp.status_code == 200, resp.text
|
||||
|
||||
save_config.assert_awaited_once()
|
||||
persisted: Final = save_config.await_args.kwargs["new_config"]["general_settings"]
|
||||
changed, removed = changed_section_keys(file_settings, persisted)
|
||||
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